67 lines
4.0 KiB
Markdown
67 lines
4.0 KiB
Markdown
|
# Execution Controls Review
|
|||
|
|
|
|||
|
|
Reviewed implementation stages 1–8 against commit `57b62d3`, the
|
|||
|
|
[implementation plan](implementation.md), the accepted
|
|||
|
|
[feature roadmap](execution-controls.md), and the applicable architecture,
|
|||
|
|
testing, documentation, and agent policies.
|
|||
|
|
|
|||
|
|
## 1. Complete The Public Prepared-Notification Contract
|
|||
|
|
|
|||
|
|
Priority: P3. Status: open. Owning stage: Stage 8.
|
|||
|
|
|
|||
|
|
Location: [RunRequest.OnPrepared GoDoc](../../types.go#L97).
|
|||
|
|
|
|||
|
|
The public GoDoc describes synchronous delivery, detached ownership, held
|
|||
|
|
admission, and split preparation, but omits two explicitly selected lifecycle
|
|||
|
|
facts:
|
|||
|
|
|
|||
|
|
- Notification occurs at most once per logical execution, when execution
|
|||
|
|
reaches that point before the first generation. Transient retries and
|
|||
|
|
structural repair do not repeat it.
|
|||
|
|
- The callback receives the operation context, including the selected operation
|
|||
|
|
deadline when enabled and inherited parent cancellation. In coordination this
|
|||
|
|
is distinct from the parent context delivered to the next-turn callback.
|
|||
|
|
|
|||
|
|
These facts are already implemented by the
|
|||
|
|
[shared execution boundary](../../internal/usecase/runner.go#L186) and covered
|
|||
|
|
by the notification, retry/repair, and coordinator contract tests. They are also
|
|||
|
|
expressly selected in the roadmap's
|
|||
|
|
[accepted callback decision](execution-controls.md#3-what-is-the-prepared-details-callback-contract).
|
|||
|
|
However, Go declarations and GoDoc own the exact public API contract; consumers
|
|||
|
|
should not need a temporary roadmap or implementation inspection to discover
|
|||
|
|
invocation cardinality or the scope of their callback's context.
|
|||
|
|
|
|||
|
|
Required remediation: add these two facts to the field's GoDoc. Preserve
|
|||
|
|
conditional delivery and cooperative cancellation; do not promise notification
|
|||
|
|
for every failed request, forced callback interruption, or terminal observation
|
|||
|
|
after panic. This is a documentation completion, not a runtime change. Existing
|
|||
|
|
behavioral tests already cover these semantics; duplicate tests are unnecessary.
|
|||
|
|
|
|||
|
|
## Stage Review Summary
|
|||
|
|
|
|||
|
|
| Stage | Result |
|
|||
|
|
| --- | --- |
|
|||
|
|
| 1 — API and conversion | Selected fields and sentinel are present; callback conversion captures the chosen function and detaches details only when selected. |
|
|||
|
|
| 2 — Ordinary timeout | One internal scope covers ordinary execution; per-invocation cause attribution and public error projection precede terminal observation. |
|
|||
|
|
| 3 — Split preparation | Handles retain duration and callback without an idle timer; claim/discard and cleanup behavior are preserved. |
|
|||
|
|
| 4 — Prepared notification | Both execution paths use the actual shared snapshot, with context checks and held admission; retries and repair do not notify again. |
|
|||
|
|
| 5 — Phase budgets | Tests cover preparation, capacity waiting, retry backoff, generation/repair, validation, notification time, and stable terminal outcomes. |
|
|||
|
|
| 6 — Ownership and failures | Public tests cover detached messages, cache metadata, nested parameters, schema, credentials, frozen sources, retained diagnostics, and panic cleanup. |
|
|||
|
|
| 7 — Coordination | Fresh turn budgets, parent-context assessment, released capacity, sibling use, observer composition, and explicit grouping are covered. |
|
|||
|
|
| 8 — Documentation and example | Maintained example and consumer/internal documentation reflect the feature; complete the canonical GoDoc as described in finding 1. |
|
|||
|
|
|
|||
|
|
No runtime correctness defect, additional architectural gap, or inadvertently
|
|||
|
|
strengthened caller restriction or guarantee was identified in this review.
|
|||
|
|
|
|||
|
|
## Validation
|
|||
|
|
|
|||
|
|
- The complete package suite passed under `go test -race ./...` when combining
|
|||
|
|
the initial run with successful reruns of `internal/configuration` and
|
|||
|
|
`internal/llm`. Those two packages initially failed solely because the sandbox
|
|||
|
|
disallowed their local `httptest` listeners; their reruns permitted listeners.
|
|||
|
|
- `go vet ./...` passed using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`,
|
|||
|
|
and offline module resolution.
|
|||
|
|
- Changed Go files are formatted. `git diff --check` and local document-target
|
|||
|
|
checks passed. No implementation, dependency, policy, or release files were
|
|||
|
|
changed during the review.
|