Implementation Review Remediation

Effective Requirement Inspection Review

Review baseline: a051edd82d9d20cdc82dfebf25adb689dc58b01f. Scope: implementation commits after a6e0ffe, compared with the simplified feature roadmap and implementation plan. The existing ReleaseKit documentation finding is retained below.

Finding 1 — Report Lost Caller-Map Scope As Incomplete (P2)

Status: Resolved. Root-map with identity is retained; unknown local-map accesses produce structured limitations and affect optional variable entries. Focused regressions cover the reproduced forms, simple parentheses, and inactive scopes. The package/race suite passed before completion of this unit.

Locations: requirement walker, visit's WithNode handling (lines 481–503), visitCommand (lines 675–690), and visitVariable (lines 694–708).

The walker treats every with body as nonroot and suppresses an unresolved condition issue unless a recognizable dependency was recorded in that body. Unsupported local-map accesses are similarly discarded without an issue when the dependency counter does not change. Losing the ability to recognize a caller key can therefore be mistaken for proof that no key is referenced.

With Vars: {"audience": "visible"}, all six of these valid templates render visible, but inspection reports Complete: true, no variables, and no issues:

{{with .}}{{.audience}}{{end}}
{{with $}}{{.audience}}{{end}}
{{$alias := .}}{{with $alias}}{{.audience}}{{end}}
{{with .}}{{index . "audience"}}{{end}}
{{$alias := .}}{{index $alias "audience"}}
{{$alias := .}}{{$alias.audience}}

This is not merely accepted failure to enumerate a complex reference: the result incorrectly claims complete discovery. Consumers can consequently accept an incomplete variable inventory as exhaustive.

Remediation: distinguish known nonroot values from unknown scope. Preserve straightforward root identity where already determinable, or report an unsupported-scope limitation when a reachable access may hide caller keys. Unrecognized access must not depend on successfully recording a named dependency before it can mark discovery incomplete. Apply the existing unknown caller-key effect handling to affected optional entries as needed. Do not add alias-value propagation or general assignment interpretation; conservative partial results satisfy the approved scope.

Verification: add focused package cases for root-map with, local-map field access, and local-map index. A result may enumerate the known key correctly or remain explicitly incomplete, but must not silently omit it from complete discovery. Retain known-string dot isolation and inactive-path behavior.

Finding 2 — Avoid Recursive Rescanning Of Unsupported Names (P2)

Location: requirement walker, evaluateInputName (lines 231–244), through evaluateValue, evaluateCommand, and visitCommand.

Unsupported name evaluation already scans the expression for recognizable references. evaluateInputName then calls visit on that same subtree again. When a computed name contains another input with an unsupported computed name, both traversals recursively repeat the same fallback. Work grows exponentially even though the source grows linearly.

An isolated diagnostic starts with .selector and repeatedly wraps it as input (printf "%s" (EXPRESSION)). Observed visits to that single caller key are 2, 4, 8, 64, and 1,024 at depths 1, 2, 3, 6, and 10 respectively. The depth-10 source is only 233 bytes. Result deduplication masks the repeated work.

Remediation: collect recognizable references once when simple name evaluation falls back to syntax enumeration. Use one conservative traversal rather than recursively reevaluating the same unsupported subtree. Preserve classifications, known references, and safe issues; more expression support, memoization machinery, or new size/depth/work restrictions are unnecessary.

Verification: retain existing computed-name results and exercise a nested unsupported expression. Use a small growth benchmark or equivalent focused work check to guard against repeated recursive scans, without making exact visit counts or a wall-clock threshold a public guarantee.

Finding 3 — Qualify The Public Variable-Disclosure Statement (P3)

Location: public request GoDoc, Vars (lines 22–24).

The comment says that supplied values are not returned in inspection. However, {{input .binding}} with Vars: {"binding": "transcript"} intentionally returns transcript as an input identifier. The approved roadmap explicitly permits that necessary identifier disclosure while excluding unrelated values. The current wording can be read as a stronger privacy guarantee than was requested or implemented.

Remediation: state that caller values are not returned as values, while a resolved input identifier can equal its supplying variable value. Put this exception in the canonical GoDoc; preserve existing unrelated-value and diagnostic disclosure protections. Do not redact legitimate binding names or introduce a new confidentiality mechanism.

Verification: review GoDoc against a direct dynamic-name result and the existing privacy contract test. A documentation-only correction does not need another full-suite run.

Review Evidence And Architecture

The root facade owns exported values, request copying, result copying, and error conversion. The use-case runner reuses captured prompt/profile selection and retrieves the exact snapshot without readiness checks.
Configuration captures effective input metadata presence at the final ConfigKit handoff. Parsed templates are shared read-only; request analysis and inventories remain operation-local in internal/prompt.
Inspection avoids artifact/schema loading, credentials, provider calls, admission, observations, and run IDs. Required declarations, inactive input and variable inventories, exact names, session bypass, and independent returned metadata are implemented and covered by focused tests.
The existing full offline race suite passed: go test -race -vet=off -mod=readonly -count=1 ./.... go vet -mod=readonly ./... and python3 scripts/check-examples.py also passed. Commands used GOTOOLCHAIN=local GOWORK=off GOPROXY=off GONOPROXY=none GOSUMDB=off.
Temporary Go overlay probes, outside the repository, reproduced Finding 1 against real template execution and quantified the repeated work in Finding 2. No implementation or maintained test files were changed during review.
No new template rejection, caller limit, input-format constraint, readiness requirement, or ConfigKit-boundary expansion was found in these commits. Finding 3 identifies the one overstated documentation guarantee.

Earlier ReleaseKit Adoption Review

Review baseline: d5f3464. The release implementation matches the selected architecture; no runtime defect or additional release gate was identified.

Finding 1 — Complete Documentation Alignment (P3)

Two descriptions were left behind during the cutover:

docs/policy/release.md, under Release Documentation, still says to use the “Promptkit section structure below.” The former fixed-heading structure was removed, and the adopted policy and runbook expressly require no fixed section headings. This reference leaves the note-writing instruction unclear.
docs/roadmap/releasekit.md still describes remaining adoption work as pending and integration validation as future work. The implementation plan records completion, and the review verified the assembled workflow.

Remove the obsolete section-structure instruction while preserving the title, summary, applicable compatibility guidance, and licensing-footer requirements. Update the feature roadmap's implementation status, or retire it together with the completed implementation plan during release preparation and repair links. Do not reintroduce a heading validator or add a publication gate.

Verification: review the affected descriptions for agreement with the actual profile and runbook, and check changed local links. The disposable fixture already demonstrates acceptance of a note without fixed section headings; these prose corrections do not require repeating the complete Go suite.

Review Evidence

python3 scripts/test-release.py passed: successful annotated publication, one race-enabled suite and additional check, same-Go tool build, unchanged module metadata, failed additional validation preventing publication, and native usage-error classification.
A disposable clone of the review baseline, with only a synthetic release note added, passed scripts/releasekit.sh check v0.0.0 offline with the real profile: race-enabled tests, full vet, Go formatting, Markdown navigation, module-distribution checks, and the three real consumer examples.
Shell syntax checks passed; the runtime root package's dependency list has no ReleaseKit import. No runtime Go code or module metadata changed during the implementation commits after 1889915.
No real Promptkit publication was performed. The adopted checks and retired gates match the agreed scope; no substantive contract strengthening was found.

9.2 KiB Raw Blame History Unescape Escape