96 lines
5.3 KiB
Markdown
96 lines
5.3 KiB
Markdown
# 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.
|