diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5d084b6..a7ed58b 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,9 +1,133 @@ -# ReleaseKit Adoption Review Remediation +# Implementation Review Remediation + +## Effective Requirement Inspection Review + +Review baseline: `a051edd82d9d20cdc82dfebf25adb689dc58b01f`. +Scope: implementation commits after `a6e0ffe`, compared with the simplified +[feature roadmap](inspection.md) and [implementation plan](implementation.md). +The existing ReleaseKit documentation finding is retained below. + +### Finding 1 — Report Lost Caller-Map Scope As Incomplete (P2) + +Locations: [requirement walker](../../internal/prompt/requirements.go), +`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: + +```gotemplate +{{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](../../internal/prompt/requirements.go), +`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](../../requirements.go), `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) +### Finding 1 — Complete Documentation Alignment (P3) Two descriptions were left behind during the cutover: @@ -26,7 +150,7 @@ 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 +### Review Evidence - `python3 scripts/test-release.py` passed: successful annotated publication, one race-enabled suite and additional check, same-Go tool build, unchanged