diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..dc37a0d --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,231 @@ +# Audit Remediation Implementation Review + +## Scope And Status + +Reviewed all 33 stages of the [implementation plan](implementation.md) against +the [selected roadmap](audit-remediation.md), its accepted catalog decisions, +and the implementation through `6673284312de23bde86c20eb8de4f3865b157471`. +The implementation changes begin after planning commit `6a1a32a`. + +The broad architecture and selected simplifications are in place. Four findings +remain open below: two correctness/completeness issues, one error-projection +efficiency issue, and one documentation-status gap. This review implements none +of their remedies. The existing race suite passing does not cover the two new +failing edge cases. + +The accepted artifact ownership, supported convenience-source types, and +self-contained schema changes are intentional compatibility changes. No +inadvertently strengthened contract was identified beyond those deliberately +selected changes. Historical measurements are observations, not performance +contracts. + +## Findings + +### 1. Preserve Caller Cancellation When Authentication Also Overflows [P2] + +**Status:** Open. **Stages:** 7, 21–22. + +**Locations:** [authentication adapter](../../internal/llm/openrouter_authentication.go), +`ProbeOpenRouterKey`, lines 61–67; +[outcome mapping](../../internal/usecase/smoke_authentication.go), +`authenticationProbe.run`; +[completion](../../internal/usecase/smoke_tester.go), `finishHTTP`. + +The adapter checks the number of returned bytes before the read error. A legal +`io.Reader` can return the allowance-plus-one byte and `context.Canceled` in the +same read. The adapter sets `Overflow` and returns nil, discarding cancellation. +The use case then selects `ExecutionFailureOther`; `finishHTTP` does not apply +caller cancellation because it only does so for context-classified failures. + +**Verified reproduction:** An offline public `SmokeTest` uses an HTTP 200 body +that cancels the caller and returns `context.Canceled` with its final bytes. +With 1,024 bytes it returns the cancellation and an incomplete report. With +1,048,577 bytes it instead returns: + +- nil Go error despite `ctx.Err() == context.Canceled`; +- `Completed == true`; +- failure `other` and reason `authentication_response_limit_exceeded`. + +The body is closed and the read allowance is honored in both cases; those parts +of the remedy work. The completed overflow result violates Stage 7's explicit +exception for caller cancellation. + +**Required correction:** Preserve context interruption across the adapter and +outcome mapping when overflow and interruption coincide. Retain independently +observed HTTP facts and unknown credential acceptance. Ordinary overflow must +still be a completed diagnostic with nil Go error. Do not change the established +treatment of a known, concluded authentication result merely because cancellation +arrives later. + +**Regression:** Exercise bytes-plus-cancellation at the overflow boundary through +the public diagnostic path, alongside ordinary overflow and interrupted +below-limit controls. Keep caller deadline handling consistent with the same +existing cancellation policy. + +### 2. Check Schema Self-Containment Independently Of Its Source Filename [P2] + +**Status:** Open. **Stages:** 11–13. + +**Locations:** [schema compilation](../../internal/validate/standard_validator.go), +`compileSchemaDocument`, lines 426–445, and its host/`fs.FS` callers; +[public wire regression coverage](../../schema_file_contract_test.go). + +The compiler registers the document at its physical file or source-relative +retrieval URI. Its denying loader rejects unknown resources, but references +back to that registered retrieval URI do not invoke the loader. Consequently, +successful local compilation does not establish that the outbound root is +self-contained without PromptKit's source context. + +**Verified reproduction:** Load a file named `schema.json` containing: + +```json +{"$defs":{"value":{"type":"integer"}},"$ref":"schema.json#/$defs/value"} +``` + +Public `PrepareExecution` succeeds. An offline HTTP client captures the actual +native structured-output payload, returns `3`, and `RunPrepared` passes local +validation. However, compiling the captured schema at a neutral submission URI, +`https://provider.invalid/submitted-schema`, with external loading disabled +attempts to load `https://provider.invalid/schema.json` and fails. The source +retrieval URI was never sent to the provider. Changing the reference to +`#/$defs/value` succeeds both locally and at the neutral URI. + +This is a packaging gap in the selected self-contained-schema remedy, not a +claim about any particular provider's schema support or enforcement. + +**Required correction:** Establish self-containment without relying on the +source retrieval URI being available upstream. Detect references that depend +on that unsent filename/path and provide an actionable error. Preserve valid +fragment references, explicit/embedded IDs, anchors, recursion, dialect handling, +and checks of otherwise unused schema declarations. Keep source location for +root loading and diagnostics; do not add external loading, a lexical ban on all +non-fragment references, a bundler, or a provider-acceptance guarantee. + +**Regression:** Cover host-file and `fs.FS` source-name dependencies, retain +explicit-ID and fragment controls, and evaluate the actual outbound schema +without the original source base. Preparation must fail before dispatch when +the selected self-contained contract is not satisfied. + +### 3. Avoid Duplicating An Already-Retained Error Cause [P3] + +**Status:** Open. **Stages:** 3–4. + +**Location:** [public error projection](../../errors.go), `mapPublicError`, +particularly lines 26–38. + +The ordinary operation branch joins the owner, the mapped cause, and the +original cause. For ordinary errors, the mapped cause already retains the +original through wrapping or is that same error. This introduces two paths to +the same cause. When an injected client returns an error from another PromptKit +engine, each additional enclosing engine repeats the previous cause tree in +both branches. Formatting duplicates diagnostics; unsuccessful error-tree +searches can also revisit the repeated branches. + +**Verified reproduction:** Begin with a public missing-prompt error, then run a +valid parent whose injected client returns that error; repeat with each parent's +result. Owner and exact-cause matching remain correct, but formatted byte counts +at depths zero through eight are: + +```text +158, 361, 767, 1579, 3203, 6451, 12947, 25939, 51923 +``` + +The input is a linear chain of consumer operations; the projection creates +exponential repeated text/traversal. Even one ordinary error repeats its cause +unnecessarily. This is introduced by the new join, not required to preserve the +owning category. + +**Required correction:** Retain the owning category and each necessary cause +without adding an original cause that the projection already retains. Preserve +`errors.Is`/`errors.As`, specialized capacity/provider/retry projections, and +redacted transport formatting. Do not require category exclusivity, discard +consumer causes, or impose a new nesting limit. + +**Regression:** Extend composed-error coverage across several enclosing +operations. Verify owner and exact-cause matching and that an independent leaf +diagnostic is not duplicated at every boundary. Protect this relationship rather +than exact complete error wording or a timing threshold. + +### 4. Reconcile The Feature Roadmap's Implementation Status [P3] + +**Status:** Open. **Stage:** 33. + +**Location:** [feature roadmap](audit-remediation.md), Purpose And Status and +workstream completion reporting. + +The feature roadmap still says neither document describes completed remediation +and all workstreams are pending. The implementation plan now records all 33 +completed stages and post-change +measurements. Stage 33 specifically required actual completion/evidence to be +recorded in the roadmap. Its current status contradicts the delivered work and +makes it difficult to distinguish original findings from remaining review work. + +**Required correction:** Record that the implementation landed, point to the +stage evidence and outstanding findings here, and distinguish historical +assessment-only statements from current overall status. Preserve the original +baseline measurements and decision provenance as historical evidence. Do not +claim all issues are resolved until the findings are addressed; normal roadmap +retirement can follow when appropriate. + +## Stage-By-Stage Review Record + +Each stage was checked against its requested outcome, changed implementation, +relevant tests, and documentation. The entries below identify the review focus; +“no additional finding” is a review result, not a guarantee of defect-free code. + +| Stage | Current evidence and result | +| --- | --- | +| 1 | Development guidance selects the latest available local toolchain independently of the module minimum; validation used Go 1.27.3 and `go.mod` remains at 1.27.0. No additional finding. | +| 2 | Both source helpers hand provider maps directly to ConfigKit; the converter is deleted. Source tests cover capture, unsupported values, cycles, ordering, and inheritance. No additional finding. | +| 3 | Internal operation errors carry explicit owner/cause with `Is`/`Unwrap`; lookup and execution boundaries use them. See finding 3 for facade composition. | +| 4 | Public nested-error and specialized-capacity tests retain owner/cause. Repeated cause retention remains inefficient: finding 3. | +| 5 | Response-reader errors are recorded below the limited reader and distinguished from parser/framing failures at both decode points; cause/redaction tests cover bytes with errors. No additional finding. | +| 6 | Built-in preparation has explicit typed error/evidence markers; inference diagnostics check cancellation first and report construction errors without dispatch. Injected clients retain their separate semantics. No additional finding. | +| 7 | Authentication reads at most allowance plus one, closes bodies, and reports explicit overflow. Simultaneous cancellation remains incorrect: finding 1. | +| 8 | Artifact GoDoc, defensive adapter copying, migration guidance, and concurrent tests follow the accepted storage transfer. No additional finding. | +| 9 | Prepared handles retain an immutable non-zero-sized identity token, not the runner; claim/discard locking remains. Normalized session IDs clone accepted storage. No additional finding. | +| 10 | Runner and engine declare `ValidationPreparer`; concrete validator constructors preserve direct validation and nil-plan behavior. No additional finding. | +| 11 | Host and `fs.FS` paths share compilation, retain root loading, and deny external loader requests. Source-base dependence remains: finding 2. | +| 12 | Declared schema traversal uses compiler-resolved dialects, escaped pointers, and schema-bearing positions, excluding annotations. It does not resolve finding 2. No separate traversal finding. | +| 13 | Public schema-file precedence/errors, freezing, and native submission are tested; the unused helper result is gone. Outbound relocation coverage misses finding 2. | +| 14 | Overlay clones the lower map once, merges owned descendants, and copies replacements. Isolation/null/array cases and comparable allocation results are recorded. No additional finding. | +| 15 | Trusted projection copying moved to `internal/jsonvalue`; fallible runtime input copying remains distinct. Type/nil/number/schema-key behavior is covered. No additional finding. | +| 16 | Owned evidence sets merge smaller into larger; callers consume values/flags rather than independent map snapshots. Work-count evidence and requirements tests support the change. No additional finding. | +| 17 | Shared branch orchestration preserves `with` body-dot versus else-dot behavior, inactive paths, and conservative inspection. Independent rendering comparisons remain. No additional finding. | +| 18 | Bounded normalization preserves invalid-byte-run replacement and rune boundaries; encoding delegates to standard JSON marshaling. No additional finding. | +| 19 | Complete/omission feedback paths use bounded prefixes and retain complete stored diagnostics/candidate behavior. Boundary/escaping tests and comparable allocation results are recorded. No additional finding. | +| 20 | Scope resolves before inventory selection; named ranges use binary search, All copies once, and unqualified selection scans without a full prior copy. Ordering, ownership, cancellation, and allocation evidence remain. No additional finding. | +| 21 | OpenRouter endpoint derivation and account-response interpretation live in the outbound adapter behind a narrow capability. Finding 1 remains in its read/error ordering. | +| 22 | Use cases retain selection/applicability/reporting; target-only detachment replaces unused broad cloners. Overflow/context outcome composition contributes to finding 1. | +| 23 | Extracted record validators retain final-handoff traversal, resource indexes, field paths, and existing checks. No raw-source validation phase was added. No additional finding. | +| 24 | Input metadata, message/template capture, output conversion, and effective targets are separated while preserving presence, source-relative reads, lazy origins, and final hash assembly. No additional finding. | +| 25 | Attempt results replace parallel returns without moving acquisition/release outside attempt scope. Built-in and injected dispatch remain distinct. No additional finding. | +| 26 | Shared accounting runs after permit release and preserves distinct HTTP/usage facts; observation and scheduling tests exercise the real coordinator. No additional finding. | +| 27 | Synthetic markers exercise value/pointer formatting and direct-key JSON omission; credential, identity, listing, and provider-map documentation is corrected. No additional finding. | +| 28 | One internal test owns the complete bundled asset oracle; smaller public option/wire/smoke tests retain distinct assembled behavior. No additional finding. | +| 29 | Artifact tests compare same-body inline/file opaque hashes and actual byte/size behavior; benchmark correctness setup remains outside timing. No additional finding. | +| 30 | Unconsumed backend presence/queue state, endpoint echo, helper result, and named orphan helpers are removed; effective policy/provenance and public features remain. No additional finding. | +| 31 | Runtime definitions use the output contract and captured message content without duplicate fields. Configuration `content_file`, override defaulting, hash relations, and migration guidance remain. No additional finding. | +| 32 | Runner scheduling uses `generation.NewInjected`; the production wrapper is deleted. Useful manager scenarios use test-only one-shot scaffolding. No additional finding. | +| 33 | Current GoDoc/consumer/format/integration/internal guidance largely matches the code; validation passes. Feature-roadmap completion reporting remains stale: finding 4. | + +## Validation And Limits + +At the reviewed commit, using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, +and `GOPROXY=off`: + +- `go test -race -count=1 ./...` passed, including the maintained examples. +- `go vet ./...` passed. +- Tracked Go files produced no `gofmt -l` output. +- `git diff --check 6a1a32a..HEAD` passed. +- Additional temporary Go test overlays outside the repository reproduced + findings 1–3. The cancellation and relocated-wire-schema assertions fail as + described; the nested-error probe measured the duplicated text while retaining + correct category/cause matching. + +The probes use controlled readers, HTTP transports, and the pinned schema +compiler. No credentials, provider calls, external schema retrieval, dependency +changes, or publication were needed. They establish local failure mechanisms, +not universal provider behavior. Optimization review checked source changes and +the recorded comparable measurements; it did not add benchmark thresholds or +new release gates.