191 lines
10 KiB
Markdown
191 lines
10 KiB
Markdown
# Implementation Review Remediation
|
||
|
||
Status: All recorded findings have been resolved in separate remediation units.
|
||
|
||
## 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)
|
||
|
||
**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](../../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)
|
||
|
||
**Status: Resolved.** The duplicate fallback traversal was removed. A focused
|
||
source-relative work regression failed before the fix (4,096 accesses for a
|
||
277-byte expression) and passed afterward, preserving partial-result metadata.
|
||
No production limits or caches were added; prompt/use-case race tests passed.
|
||
|
||
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)
|
||
|
||
**Status: Resolved.** Request and method GoDoc now expressly distinguish caller
|
||
values from intentionally reported resolved input identifiers. Existing direct
|
||
name and privacy coverage passed in the assembled verification; no runtime
|
||
behavior or confidentiality mechanism changed.
|
||
|
||
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)
|
||
|
||
**Status: Resolved.** The obsolete heading instruction was removed without
|
||
changing note title, summary, compatibility, or licensing-footer requirements.
|
||
The adoption roadmap records completion and past integration verification,
|
||
links to current canonical owners, and identifies its original baseline as
|
||
historical. No release checks or publication gates changed.
|
||
|
||
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.
|