diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 95a9cd5..4a46c7a 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,134 @@ -# Audit Remediation Implementation Review +# Implementation Review Findings -## Scope And Status +## Responsibility Simplification Review + +Reviewed all 17 stages of the current [implementation plan](implementation.md) +against [responsibility-simplification.md](responsibility-simplification.md) +and its accepted audit decisions. The implementation spans `448e64d` through +`5ee602cc4b5a3afe4e7d361eb77181ef0940c7cd`, after planning commit `de46935`. + +The targeted architecture is substantially implemented. One ownership regression +remains open below. No additional unintended restriction or public guarantee was +identified; removing copier-derived schema limits is the selected acceptance +change, while runtime provider-value support remains in place. The earlier +33-stage review and its five completed findings are retained separately below. + +### R1. Isolate Custom JSON Encoders From Retained Metadata [P2] + +**Status:** Open. **Stages:** 2, 13–15; particularly the shared prepared snapshot +and removal of private smoke-target detachment. + +**Locations:** [provider payload construction](../../internal/llm/openai_compatible_client.go), +`openAIChatRequestPayload`, lines 468–475, and `PrepareGeneration`, lines 133–139; +[prepared metadata sharing](../../internal/usecase/prepared_execution.go), +`PrepareExecution`, lines 52–57; +[smoke report target](../../internal/usecase/smoke_tester.go), `smokeReportTarget`. + +Supported named map/slice values can implement `json.Marshaler`. The acceptance +copier preserves compatible concrete types, including their methods. The built-in +payload builder places provider values into the wire map by reference, and +`json.Marshal` can invoke those methods on retained values. For example: + +```go +type mutatingMap map[string]string + +func (m mutatingMap) MarshalJSON() ([]byte, error) { + m["value"] = "changed by encoder" + return json.Marshal(map[string]string(m)) +} +``` + +This is an accepted finite, acyclic string-keyed map. Its custom encoding method +mutates only its receiver, not external state. No concurrent caller mutation or +invalid injected collaborator is involved. + +**Verified reproduction:** An offline public-API probe constructs an ordinary +engine with the built-in LLM client and an in-memory HTTP transport returning a +valid response. It supplies `ExtraParams["options"] = mutatingMap{"value": +"original"}`. Preparation succeeds. It compares details before/after +`RunPrepared`, then separately inspects a text-inference smoke report using the +same kind of runtime override. Both operations return nil Go errors: + +| Observation | Current implementation | With pre-sprint detachment restored via Go overlay | +| --- | --- | --- | +| Prepared details before execution | `original` | `original` | +| Prepared details after execution | `changed by encoder` | `original` | +| Smoke report's effective provider setting | `changed by encoder` | `original` | +| Caller-owned input map | `original` | `original` | + +The comparison overlays only `internal/usecase/prepared_execution.go` and +`internal/usecase/smoke_tester.go` from `de46935` onto the current code. It +isolates the lost metadata ownership separation; it is not a test of the entire +old revision. The probe and overlays live outside the repository. + +**Cause and impact:** Removing the second prepared snapshot is correct only if +all consumers treat the shared snapshot as immutable. Injected clients receive +independent public requests, but built-in encoding is another callback boundary: +custom encoding code can mutate its input. The new sharing exposes that mutation +through retained details and diagnostic settings. It also creates an unsynchronized +mutation path alongside otherwise supported concurrent details reads. The +existing mutating-client regression exercises an injected client, so it does not +cover this built-in path. Encoder failure diagnostics are relevant too: +`PrepareGeneration` may separately marshal original extra-parameter values when +whole-payload encoding fails. + +**Required correction:** Keep the single immutable metadata snapshot, but detach +mutable accepted provider values at the built-in encoding boundary before any +custom encoder runs. Ensure both payload encoding and per-value encoding-failure +diagnostics operate on detached values. Reuse trusted copying where appropriate; +do not reinstate the duplicate details snapshot or reintroduce input validation +merely to copy accepted data. + +Retain current named/typed input and custom wire-encoding behavior, native retry +payload freezing, error classification, and the outward public ownership copies. +Rejecting these types, stripping their methods through JSON-native normalization, +or requiring nonmutating custom encoders would narrow the selected runtime +contract instead of repairing ownership. + +**Regression coverage:** Exercise a supported named provider map with a +receiver-mutating `MarshalJSON` through the built-in offline transport. Check +unchanged prepared details and smoke effective settings, independent caller +input, and the expected custom wire representation. Include the encoding-failure +path so its per-value diagnostic cannot mutate retained state. Retain existing +injected initial/repair mutation and concurrent prepared lifecycle protection. + +### Stage-By-Stage Review + +| Stage | Current evidence and result | +| --- | --- | +| 1 | Constructor routing defaults and their only error path are removed. Resolved endpoint/model checks and HTTP-client timeout ownership remain. No additional finding. | +| 2 | One root payload builder preserves field omission/presence, reserved extensions, session/native schema handling, and encoded-body freezing. Its borrowed provider values participate in R1. | +| 3 | Private repositories explicitly require captured definitions, metadata, requirements, and profile identity; capability/hash fallbacks are removed. No additional finding. | +| 4 | Repository fixtures implement captured operations; metadata-only and selection/error coverage remain. No additional finding. | +| 5 | Rendering uses captured templates only, with per-call clones/functions and retained session/input/message behavior. No additional finding. | +| 6 | Renderer fixtures/benchmarks supply captured parses; syntax, isolation, cancellation, artifact-text, and requirements comparisons remain covered. No additional finding. | +| 7 | Smoke syntax and schema checks use separate prepared plans; malformed syntax leaves schema matching unevaluated, including scalar/nonmatching controls. No additional finding. | +| 8 | Direct validator interface/methods and on-demand callbacks are deleted; shared instance validation and eager source-specific preparation remain. No additional finding. | +| 9 | Tests use prepared plans and retain source containment, portability, references, precision, freezing, and synchronous cancellation coverage. No additional finding. | +| 10 | Trusted schema detachment removes provider-copy budgets. Deep schema isolation and a real schema with over 100,000 annotation values are covered. No additional finding. | +| 11 | Registry/profile lookup uses trusted detachment after final acceptance, while configuration validation/overlay and runtime acceptance checks remain. No additional finding. | +| 12 | Private target resolution transfers owned maps, preserving empty-map inheritance and nil projections. Outward/injected projections still detach values. No additional finding. | +| 13 | One redacted metadata snapshot is shared, and cleanup no longer writes it; owner/claim/discard state and outward copying remain. Built-in custom encoding can still mutate the snapshot: R1. | +| 14 | Smoke projection is infallible and outward values are detached; credentials/endpoints remain sanitized. Built-in encoding can mutate effective settings before final report projection: R1. | +| 15 | Added tests cover injected initial/repair mutation, returned details/results, named/numeric runtime values, and concurrent lifecycle reads. The built-in custom-encoder path is missing: R1. | +| 16 | Canonical format, integration, model-client, runner, source, and component guidance reflects the selected changes without new resource/provider promises. The immutability description is not fully realized until R1 is fixed. | +| 17 | Fresh full race tests and vet pass, but those checks do not cover R1. Documentation/history links and delivered scope were reviewed; completion still requires the ownership correction. | + +### Current Validation And Limits + +Using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, and `GOPROXY=off`: + +- `go test -race ./...` passed, including the maintained offline examples. +- `go vet ./...` passed. +- The public ownership probe reproduced both metadata mutations, with nil Go + errors and unchanged caller values; the two-file historical overlay prevented + both mutations. + +No production code or repository tests were changed. The source review and +targeted probe establish R1 despite the passing existing suite. The review does +not claim universal callback safety, provider compatibility, or defect-free code. + +## Earlier Audit Remediation 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, @@ -23,7 +151,7 @@ inadvertently strengthened contract was identified beyond those deliberately selected changes. Historical measurements are observations, not performance contracts. -## Findings +## Earlier Audit Findings ### 1. Preserve Caller Cancellation When Authentication Also Overflows [P2] @@ -262,7 +390,7 @@ prompt/profile classification and existing cancellation/provider cases. Do not require universal attempt/terminal equality: later validation or cancellation can legitimately change an operation's final outcome. -## Stage-By-Stage Review Record +## Earlier 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; @@ -304,7 +432,7 @@ relevant tests, and documentation. The entries below identify the review focus; | 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 +## Earlier Validation And Limits At the reviewed commit, using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, and `GOPROXY=off`: @@ -325,7 +453,7 @@ not universal provider behavior. Optimization review checked source changes and the recorded comparable measurements; it did not add benchmark thresholds or new release gates. -## Integration Follow-Up Coverage +## Earlier Integration Follow-Up Coverage The follow-up focused on interactions between the changed components and the existing consumer workflows, rather than repeating the stage-local review. @@ -363,7 +491,7 @@ was controlled offline. The prior full-suite/vet evidence above remains applicable to the unchanged implementation; this follow-up adds focused integration evidence rather than a new release gate. -## Algorithmic Follow-Up Coverage +## Earlier Algorithmic Follow-Up Coverage A further source-based review at `7203e5d` focused on algorithmic correctness, efficiency, and scalability of the implemented stages. It confirms the