promptkit/docs/roadmap/remediation.md

# Implementation Review Remediation

Status: All recorded findings have been resolved in separate remediation units.

## Computational Efficiency Review

Review baseline: `8a5c0de`, compared with planning commit `3fde1f6`, the
[performance roadmap](performance.md), and [implementation plan](implementation.md).

### Finding 1 — Release A Stale File Reservation For An Empty Body (P3)

**Status: Resolved.** Completed empty bodies now release stale file-size
reservations using the existing compaction path. The stale-size table covers
empty content, metadata, and hashes; the retained-artifact benchmark includes
an empty-body fixture without enforcing capacity or allocation thresholds.
Three offline, one-iteration samples for four retained empty artifacts changed
from 4,194,928–4,194,976 retained bytes to 624–640 bytes. Initial reservation
allocations still occur; this fix reduces retained storage, not cumulative
allocation. The nonempty benchmark remains near 1 MiB for four 256 KiB bodies.
Artifact package race tests and vet passed, and the internal documentation
now accurately describes the compaction condition.

Location: [file reader](../../internal/artifact/reader.go), lines 137–166.

The new file-size reservation is compacted only when the completed body is
nonempty. If the opened file reports a positive size but yields no bytes
(for example, because it is truncated before the read), the empty artifact
retains the entire reservation. An offline diagnostic using the existing
opened-file test seam reported a 16 MiB size and returned immediate EOF:
the result had zero body bytes and size zero, but retained a 1 MiB backing
array. The previous reader did not reserve that storage for an empty result.

This is a retained-memory edge case, not incorrect content, a permanent leak,
or a problem with ordinary empty files whose size hint is zero. It leaves
Stage 4's stale-hint cleanup incomplete.

Remediation: release the reserved backing storage when the completed body is
empty, preserving existing empty-artifact semantics. Keep size and hash based
on the actual body, and retain the existing EOF and cancellation behavior.
No new input limit or public capacity guarantee is needed.

Verification: extend the stale-size cases to include an empty actual file
with a positive reported size; check accepted content, metadata, and hash.
Use a focused retained-memory diagnostic or benchmark to verify that the
reservation is released, rather than making an exact capacity or allocation
count a durable test requirement.

Also correct the accompanying wording in
[Ordinary Artifacts](../internal/sources.md#ordinary-artifacts): the current
predicate compacts when unused capacity exceeds the body length (total
capacity exceeds twice the body length), not when unused capacity exceeds
twice the body length.

Review verification: the complete offline race suite, repository-wide Go vet,
and maintained consumer examples passed. No additional actionable architecture
gaps or strengthened public contracts were identified in this review.

## 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.