base-layer/promptkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 4 Wiki Activity

Record the responsibility simplification review

Browse Source
This commit is contained in:
Eric Rakestraw
2026-11-06 01:01:14 +00:00
parent 5ee602cc4b
commit d8c98ac575
1 changed files with 135 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

142
docs/roadmap/remediation.md
View File

@@ -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