232 lines
16 KiB
Markdown
232 lines
16 KiB
Markdown
# 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.
|