111 lines
6.2 KiB
Markdown
111 lines
6.2 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 two correctness gaps below have been remediated. This document preserves
|
|
the review evidence and resolution, not a new public contract.
|
|
|
|
## 1. Isolate Observation State Across Public Invocations (P2)
|
|
|
|
**Status:** Resolved.
|
|
|
|
**Location:** [Engine.Run](../../engine.go);
|
|
[Runner.Run](../../internal/usecase/runner.go);
|
|
[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.
|
|
|
|
**Resolution:** Preparation and execution facade entry points mask inherited
|
|
recorder/claim state before installing any invocation-owned state. The context
|
|
boundary preserves caller values, deadlines, cancellation, and cancellation
|
|
causes. Public regression tests cover observed and unobserved delegated ordinary
|
|
and prepared runs, preparation isolation, independent accounting/identities,
|
|
and concurrent delegation. A focused recorder test checks context preservation
|
|
and claim-sink isolation.
|
|
|
|
## 2. Preserve Injected Failure Categories In Attempt Events (P2)
|
|
|
|
**Status:** Resolved.
|
|
|
|
**Location:** [public client adapter](../../llm_adapter.go);
|
|
[attempt classification](../../internal/generation/coordinator.go);
|
|
[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.
|
|
|
|
**Resolution:** The facade adapter adds internal classification identities for
|
|
explicit public transport/malformed errors, retaining the original diagnostic
|
|
and error chain. Nil success uses the same normalization path. Public tests
|
|
check attempt/terminal agreement, `errors.Is`/`errors.As`, single dispatch even
|
|
with a positive retry budget, arbitrary errors, and context-first classification.
|
|
|
|
## Validation And Review Outcome
|
|
|
|
- `./scripts/release-check.sh` passed on the review baseline and remediated tree, including
|
|
race-enabled tests, vet, the three offline examples, formatting, documentation
|
|
links, and repository hygiene checks.
|
|
- Temporary public-API probes reproduced both findings during review. They were
|
|
removed and replaced by focused permanent regression coverage during remediation.
|
|
- The existing tests provide useful coverage of retries, repair interaction,
|
|
capacity, prepared claims, usage presence, and redaction, but did not protect
|
|
these two observation-boundary cases before remediation. The new
|
|
[boundary tests](../../observation_boundary_contract_test.go) protect them.
|
|
- No other must-fix issue was identified in this review. This does not establish
|
|
live-provider behavior; all verification was offline.
|