diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..bb8118c --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,95 @@ +# Reliable Execution Review Findings + +Review baseline: `cef8524`, following the completed +[implementation plan](implementation.md) and the +[reliable execution roadmap](reliable-execution.md). + +The intended architecture is substantially implemented: the public facade +adapts contracts, the runner owns execution and repair, one generation +coordinator owns retries and active permits, the HTTP adapter freezes requests +and decodes responses, and an invocation-local recorder produces observations. +The following two correctness gaps need remediation before release. This is a +review record, not an implementation change or a new public contract. + +## 1. Isolate Observation State Across Public Invocations (P2) + +**Status:** Open. + +**Location:** [Engine.Run](../../engine.go), lines 535–547; +[Runner.Run](../../internal/usecase/runner.go), lines 138–145; +[recorder context propagation](../../internal/observation/recorder.go). + +The observer is request-scoped, but its recorder is attached to the context +passed to injected collaborators. If an injected client delegates to another +engine using that context and a request without an observer, `Engine.Run` +passes the inherited context through unchanged. The nested runner discovers +the outer recorder and treats it as its own. + +A focused public-API reproduction with two engines and one nested injected +client produced identical inner and outer result RunIDs, four outer events +instead of three, and two outer attempts instead of one. The inner call had no +observer. Nested selection and usage also flow into the shared recorder; +parallel delegated calls could concurrently mutate state intended to be local +to one sequential invocation. This is not a request to add nested tracing: +injected-client work is intentionally opaque to its caller's accounting. + +**Remediation direction:** Establish an observation-context boundary for each +public operation, retaining caller cancellation, deadlines, and unrelated +context values while preventing inherited Promptkit recorder/claim state from +being reused. Install only that invocation's own observation state when +appropriate. Check `RunPrepared` without a retained observer as well as +preparation operations, which must not mutate an enclosing run's selected +identities. Do not prohibit delegation or add a global recorder/locking layer. + +**Regression evidence required:** Exercise delegation through the public facade +with the received context. Separate calls must have distinct result identities; +an unobserved child must not emit to or inflate its parent's observations; a +child with its own observer must have its own lifecycle. Include focused +prepared/preparation coverage where the same context plumbing is used, and +race-enabled coverage for concurrent delegation without sharing recorder state. + +## 2. Preserve Injected Failure Categories In Attempt Events (P2) + +**Status:** Open. + +**Location:** [public client adapter](../../llm_adapter.go), lines 14–20; +[attempt classification](../../internal/generation/coordinator.go), lines +170–172 and 235–247; [terminal classification](../../observations.go). + +The public adapter returns public error sentinels, including +`ErrMalformedResponse` for a successful nil response. The coordinator recognizes +only internal LLM sentinels. As a result, attempt observations report the broad +`generation` category while the terminal observation reports the specific +`malformed_response` or `transport` category for the very same failure. + +Focused public-API reproductions confirmed all three cases: injected `(nil, +nil)`, an explicit public `ErrMalformedResponse`, and an explicit public +`ErrTransport`. Each emitted `generation` on the attempt and the expected +specific category on the terminal event. Existing repair-failure tests check +the terminal category but do not assert the failing attempt's category. + +**Remediation direction:** Normalize deliberately supplied public failure +identities at the facade adapter boundary into metadata or internal identities +the coordinator understands, preserving the original error for `errors.Is` and +`errors.As`. Classify nil success consistently through the same boundary. +Internal packages must not import the public package. Do not infer categories +from arbitrary error text or enable retries for injected clients. + +**Regression evidence required:** A compact public-boundary table covering nil +success, wrapped public malformed/transport sentinels, and an arbitrary injected +error should assert both attempt and terminal classifications, preserved error +identity, and single dispatch. Retain context-first classification for wrapped +cancellation/deadline causes. + +## Validation And Review Outcome + +- `./scripts/release-check.sh` passed on the committed implementation, including + race-enabled tests, vet, the three offline examples, formatting, documentation + links, and repository hygiene checks. +- Temporary public-API probes reproduced both findings. They were removed after + review; no implementation or permanent test changes are included here. +- The existing tests provide useful coverage of retries, repair interaction, + capacity, prepared claims, usage presence, and redaction, but do not protect + these two observation-boundary cases. +- No other must-fix issue was identified in this review. This does not establish + live-provider behavior; all verification was offline.