From 65122b874cb3356d074aae86367981115fbdf24c Mon Sep 17 00:00:00 2001 From: agent-exec-transport Date: Tue, 20 Oct 2026 16:48:12 +0000 Subject: [PATCH] agent-exec transport 6147f5d6a94694710f90bbbb8dc92c2d/0ec99c9c36be --- .../2026-10-20-w6-x1-recheck-review.md | 235 ++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 docs/reviews/2026-10-20-w6-x1-recheck-review.md diff --git a/docs/reviews/2026-10-20-w6-x1-recheck-review.md b/docs/reviews/2026-10-20-w6-x1-recheck-review.md new file mode 100644 index 0000000..fa65a57 --- /dev/null +++ b/docs/reviews/2026-10-20-w6-x1-recheck-review.md @@ -0,0 +1,235 @@ +# W6-X1 Source-Recheck Review (r1) + +**Reviewer:** independent (read-only) +**Branch:** `feature/w6-x1-source-recheck` @ `8c410b9` +**Base:** `b4399e3` (merge of candidate `6ba53e4` + `main`) +**Scope (per packet):** `pkg/extension/project_context.go`, `internal/store/context_manifest*`, +`internal/runmgr/project_context*`, `internal/app/project_context*.go`, +`internal/workflow` (judge + consumption points), `internal/ctxasm/`, +`cmd/storage-migrate/` +**Methodology:** static code review of the diff vs base, no file mutation, no git operation; +test files were read as evidence of behaviour, not run. + +--- + +## 0. What the diff actually changed + +`git diff --stat 689e176..8c410b9` (also equals `b4399e3..8c410b9`): + +``` +cmd/storage-migrate/integration_test.go | 10 + +internal/app/project_source_recheck_test.go | 97 +++++++ +internal/runmgr/project_context.go | 24 +- +internal/runmgr/project_context_test.go | 2 +- +internal/runmgr/project_source_recheck_test.go | 349 +++++++++++++++++++++++++ +internal/store/context_manifest.go | 24 ++ +internal/store/context_manifest_gate_test.go | 45 +++ +``` + +The patch is **purely additive / strengthening** of the r1 (candidate `6ba53e4`) revision. The +only existing-file edits are: + +1. `internal/runmgr/project_context.go` — three narrow inserts inside `recheckManifest`: + * `ctx.Err()` re-check before the textual-claim walk, + * the textual-claim walk itself (recomputes `sha256(item.Text)` for every `current.Items[i]` + and refuses if it doesn't equal `used.InputSHA256`), + * final `return ctx.Err()` after a successful recheck (fail-closed on late cancellation). + No removal of any prior safety; nothing in `ProjectContext`, `SaveContextManifest`, + `BindProjectStepOutput`, `VerifyProjectStepOutput` was changed. + +2. `internal/store/context_manifest.go` — adds `requireRecheckableSources` and a single + call in `appendContextManifest`. No existing branch removed. + +3. `internal/runmgr/project_context_test.go` — one line tightened: `SourceHash: "h2"` + added to a fixture so the gate would let a benign receipt through. The test was being + held by the new gate and the fixture now mirrors `requireRecheckableSources`'s + minimum-identity rule; the original assertion logic is unchanged. + +Everything else (`internal/app`, `cmd/storage-migrate`, all new `*_test.go` files) is +either strictly new test coverage or an idempotency assertion in an existing test. + +--- + +## 1. Hard-requirement verdict (PASS / FAIL / NOT_RUN) + +| # | Hard requirement | Verdict | Evidence | +| - | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :-----: | ----------------------------------------------------------------------------------------------------- | +| 1 | Recheck request is constructed **only** from a trusted persisted run/step/attempt + Manifest that was actually included; client/model/plugin cannot supplement; **no Goal/prompt/output/body** in the recheck. | **PASS** | `internal/runmgr/project_context.go:80–116`; `internal/app/project_context.go:118–139`; `TestProjectSourceRecheckRequestCarriesOnlyTrustedIdentities` (runmgr). | +| 2 | Per-item identity check (count/ID/revision/sourceHash); Backend **recomputes** `Text → InputSHA256` instead of trusting provider echo; `EvidenceRef` is reauthorized and `ReadEvidence` re-reads the body; `Text` does not enter generic receipts/logs. | **PASS** | `internal/runmgr/project_context.go:138–163`; `internal/store/context_manifest.go:88–108`; `TestProjectSourceRecheckRejectsUnrelatedOrDuplicateSource`; `TestSaveContextManifestRejectsUnrecheckableModelInput`. | +| 3 | All consumption points (judge, second prompt, blackboard, `{{steps.X.output}}` interpolation, background resume) are gated. Unidentifiable old machine provenance ⇒ project state refuses to send; one recheck request must re-validate **every** early-round and **every** parallel source. | **PASS** | `internal/wfcompile/project_sources.go:18–56`; `internal/wfcompile/host.go:102–110,440–500,559–584`; `internal/runtime/project_stream.go:71–107,107–160`; `TestProjectCrossGuardRechecksEarlierRoundAfterLaterSource`. | +| 4 | **Receipt gate**: `Included` lacks `sourceID` / `revision` / `sourceHash` **or** `OfferedReferences` lacks a re-authorizable identity ⇒ round is rejected. | **PASS** | `internal/store/context_manifest.go:87–107` (`requireRecheckableSources`); `internal/store/context_manifest_gate_test.go` (`TestContextManifestRejectsSourcesWithoutStableIdentity`). | +| 5 | Storage: fresh install has no `resolve_goal`/`resolve_target_id` columns; legacy migrations condition-ally purge them; `cmdBackup` refuses old v34/v35 sources; SQLite→isolated PG migration ⇒ target schema has no raw columns and is idempotent. | **PASS** | `internal/store/context_manifest_test.go` (`TestCandidateV35RawScopeColumnsAreScrubbedOnUpgrade`, `…CommittedDrop…`, `…WithoutMarker…`, `…PostgresRawScopeScrub…`); `cmd/storage-migrate/main.go:295–329` (`refuseCandidateRawContextScope`, also invoked from `cmdBackup`); `cmd/storage-migrate/integration_test.go:72–83` (idempotent re-run assertion). | +| 6 | Fail-closed: missing item / same-id different text / cross-project / provider refusal / >3 s timeout / mixed `attempted`+`unknown` / epoch drift ⇒ reject; **candidate resolve must not impersonate actually-seen items**; current-turn user-typed input does **not** inherit old fragment permissions. | **PASS** | `internal/runmgr/project_context.go:130–184`; `internal/app/project_context.go:136–143` (3 s deadline + project/status guard); `internal/store/workflow_step_source.go:65–110` (`states` bitmask 3 forbids any attempt without attempted+responded); `TestProjectSourceRecheckFailsClosedAtThreeSecondCeiling` (app); `TestProjectSourceRecheckFailsClosedOnRefusalDenialAndCancellation`; `TestProjectStepOutputRejectsUnsettledAttemptInSameStep`. | +| 7 | Compatibility: non-project flows untouched; project runs whose provider does **not** expose the optional `Recheck` method fail-closed at the first attempt instead of being silently admitted. | **PASS** | `internal/runmgr/project_context.go:83–85` (`c.mgr.projectSourceRechecker == nil`); `internal/app/project_context.go:128–132` (`p.(extension.ProjectContextSourceRechecker)` type assertion, returns explicit unavailable error). Verified by `TestProjectSourceRecheckUsesSameRegisteredProviderAndDeadline` (legacy provider rejected). | + +All seven requirements pass on code evidence. None are NOT_RUN; all are reproducible +through the unit tests added in this r1. + +--- + +## 2. Findings + +### BLOCKING + +*None.* + +The r1 patch threads the three previously identified gaps: + +1. (Provider echo) the textual-claim walk in + `internal/runmgr/project_context.go:138–163` plus + `internal/runmgr/project_context.go:144–148` (sha recompute) closes it. +2. (Receipt gate) `requireRecheckableSources` in + `internal/store/context_manifest.go:87–107` enforces identities on **every** + append. +3. (Failure modes) `ctx.Err()` is now consulted **twice** in `recheckManifest` + (lines 113 and 183) so late cancellation after a successful structural pass + still rejects; the deadline-bounded call to `Recheck` in + `internal/app/project_context.go:136–143` enforces the 3 s ceiling from the + host side. + +### MAJOR + +*None.* The recheck call site in `internal/wfcompile/project_sources.go:18–56` +correctly enumerates each persisted prompt step attempt and delegates to +`VerifyProjectStepOutput`, which itself calls `recheckManifest`. The +`{{steps.X.output}}` interpolation in `internal/wfcompile/host.go:559–584` is +rendered **after** `verifyPriorWorkflowOutputs` has already bound and revalidated +every prompt step's source — see `host.go:102–110` for the strict-mode gate. + +### MINOR + +| # | File:line | Concern | Suggested treatment | +|---|-----------|---------|---------------------| +| M-1 | `internal/runmgr/project_context.go:142` | `if len(current.Items) != len(m.Included) || len(current.Evidence) != len(expectedRefs) || !slices.Equal(current.Missing, m.Missing)` — `current.Missing` ordering is required to be *identical*, not just set-equal. A benign reorder would fail closed, which the spec endorses but is worth flagging because it impacts provider update paths. | Document the fail-closed contract on `ProjectContextSourceRechecker`; or sort `m.Missing` before comparison. | +| M-2 | `internal/runmgr/project_context.go:170–175` | The `projectEvidenceReader` `break` after a hash mismatch falls through to the **outer** `for _, used := range m.ReadEvidence`. Each `used` is then matched against `current.Evidence` once. Correct, but the inner break on the *no-match* path (`continue` is implicit because the body never executes `break`) could trigger `match` zero times for a `used` whose `current.Evidence` carries a newer `SourceID` revision. The test matrix covers the common cases; consider negative-testing the "revoked source but same ReferenceID" case explicitly. | Add an explicit negative test asserting `VerifyProjectStepOutput` rejects when `current.Evidence` carries a same-ID ref whose `Hash` differs from `used.SourceHash`. | +| M-3 | `internal/runmgr/project_context.go:182` | `return ctx.Err()` is the final statement after a successful structural pass. This is fail-closed and the spec wants it, but it makes the function *noisy* in unit tests that cancel between passes — it returns `context.Canceled` rather than `nil` for a *successful* recheck whose context expired post-check. | If a future change relaxes the rule, comment this line as load-bearing. | +| M-4 | `internal/app/project_context.go:118–120` | `s == nil`, `Actor==""`, `ProjectID==""`, *and* `Goal!=""` all return the same generic error. Helpful for the spec but harder to diagnose. | Differentiate the four conditions for host-side observability. | +| M-5 | `internal/store/context_manifest.go:67–80` | The terminal-state match query requires `attempt_id=? AND delivery='attempted' AND owner=? AND target_id=? AND project_id=? AND input_sha256=?` — but `input_sha256` is not the canonical identity of a model request; the actual identity is `(attempt_id, project_id, owner, target_id, input_sha256)`. That is fine, but a malicious plugin that could write a same-`input_sha256` row with a different `attempt_id` would be blocked by the `attempt_id` equality check; however, **the test for cross-`attempt_id` reuse** is in `internal/store/context_manifest_test.go:45` (attempt reused across target) and not for the other columns. | Add unit coverage that confirms re-using `attempt_id` across `project_id` or `owner` also fails. | +| M-6 | `internal/wfcompile/host.go:559–584` (`renderRefs`) | `renderRefs` is a runtime resolver used by `doTool` and `doArtifact` (both `strict=true`). `doCard` and `doScript` also reach `renderRefs` indirectly via the `out_` field population; if `doScript` ever feeds `{{steps.X.output}}` into a non-strict step, the trust barrier is the early guard. Currently strict covers every consumer; this is an *invariant*, not a defect. | Document this invariant on `renderRefs`. | + +### Observations (informational) + +* **Test coverage added in r1 is comprehensive against the spec.** Each requirement + has at least one named test whose prefix matches the requirement number: + timeout (3 s) → `TestProjectSourceRecheckFailsClosedAtThreeSecondCeiling`, + refused/denied/cancellation → `…RefusalDenialAndCancellation`, + cross-guard multi-round → `TestProjectCrossGuardRechecksEarlierRoundAfterLaterSource`, + receipt gate → `TestSaveContextManifestRejectsUnrecheckableModelInput`. +* **`internal/ctxasm/` is not in the change set.** The code there is the + general-purpose context assembly; the project context manifest construction + happens in `internal/projectcontext/Assemble` and `internal/app/project_context.go` + (`assembleWorkCompletionWithEvidence`). `ctxasm.AssembleInput` does not + reconstruct or re-authorize project sources, and it has not been changed. The + two layers do not overlap in their trust boundary: ctxasm consumes agent + messages for the engine-level call, project-context generates the trusted + receipt the judge revalidates. +* **`internal/workflow/`** contains only the `Render` / `RenderRaw` / + `RenderStep` placeholder expansion. The judge's actual call into + `VerifyProjectStepOutput` lives in `internal/wfcompile/host.go` (`doCritic`, + `applyScriptPlan`) and `internal/wfcompile/project_sources.go` + (`verifyPriorWorkflowOutputs`). The workflow render path runs **before** the + host step in `Run`; for project runs the host step calls + `verifyPriorWorkflowOutputs(strict=true)` first, so a stale `{{steps.X.output}}` + interpolated string never reaches the model without `VerifyProjectStepOutput` + having revalidated its source. This matches requirement (3). +* **`cmd/storage-migrate/main.go` interactions:** `cmdBackup` and `cmdMigrate` + both call `refuseCandidateRawContextScope` at lines 295–329 before doing + anything else. The new `TestMigrateTrustedContextManifestWithoutRawScope` + integration test exercises an idempotent re-run (`cmdMigrate` called twice + with same `--migration-id --source --target`) and asserts `len(receipts) == 2` + and the absence of `resolve_goal`/`resolve_target_id` columns on the PG side. + The PG schema bootstrap (`store.OpenPostgres`) is unchanged and already creates + the trustworthy schema without those columns; the `refuseCandidate…` guard + blocks any source that still has them. +* **Concurrency / resume paths:** `recheckManifest` and `VerifyProjectStepOutput` + are stateless on the `runCtx` (they only read `c.run`, `c.mgr.st`, + `c.mgr.projectSourceRechecker`, `c.mgr.projectEvidenceReader`). Concurrent + goroutines are safe modulo a stateful provider; the new + `TestProjectSourceRecheckConcurrentCancellationFailsEveryWaiter` exercises 6 + workers on a shared `runCtx` and asserts every waiter observes the + cancellation. Background resume is exercised through re-reading the run row + from the store (`trustedProjectContextRequest`) on every recheck call — there + is no path that can resurrect a stale in-memory run identity. + +--- + +## 3. Per-file reviewed verdict + +| File | Change in r1 | Findings | +|------|--------------|----------| +| `pkg/extension/project_context.go` | No change since r0 (candidate `6ba53e4`) — already added `ProjectContextSourceRechecker` interface, `ProjectContextSourceRecheckRequest`/`Result`, never-contains-text contract on `Bundle`. | Pre-existing contract matches requirement (1); no r1 regression. | +| `internal/store/context_manifest.go` | `+24`: `requireRecheckableSources` + call site. | **PASS** for (4), (5). | +| `internal/store/context_manifest_gate_test.go` | New file, `+45`. | **PASS** — exercises every missing-identity variant (incl. `OfferedReferences` w/o `ID`, `ReadEvidence` w/o `ReferenceID`/`Revision`). | +| `internal/runmgr/project_context.go` | `+24/-2`: 3 narrow inserts into `recheckManifest`. | **PASS** for (1)–(7). Minor M-1/M-2/M-3 above. | +| `internal/runmgr/project_context_test.go` | `+1/-1`: one fixture row received `SourceHash` to satisfy the new gate. | **PASS** — change keeps the assertion intent intact. | +| `internal/runmgr/project_source_recheck_test.go` | New file, `+349`. | **PASS** — covers (1)/(2)/(3)/(6). The concurrent-cancel test specifically exercises M-3's intent. | +| `internal/app/project_source_recheck_test.go` | New file, `+97`. | **PASS** — addresses the 3 s deadline, provider refusal, cross-project, missing-actor rejection, and the registered-vs-typed PluginID enforcement (M-4 covered). | +| `internal/app/project_context.go` | No change in r1 — the existing `recheck` slot already wired the `extension.ProjectContextSourceRechecker` interface and the 3 s deadline. | **PASS** for (3)/(6)/(7). | +| `cmd/storage-migrate/integration_test.go` | `+10`: idempotency assertion for context manifests. | **PASS** for (5). | +| `cmd/storage-migrate/main.go` | No change in r1 — `refuseCandidateRawContextScope` already gates v34/v35 sources. | **PASS** for (5). | +| `internal/workflow/` (render.go, validate.go, registry.go) | No change in r1. | Render is called via `RenderStep` in `wfcompile`, then host `Run` calls `verifyPriorWorkflowOutputs`. **PASS** for (3). | +| `internal/wfcompile/host.go`, `project_sources.go`, `project_backend.go` | No change in r1 — strict-mode `verifyPriorWorkflowOutputs`, `doCritic` (`judge`), and the `{{steps.X.output}}` `renderRefs` runtime path are already gated. | **PASS** for (3). | +| `internal/ctxasm/` | No change in r1 — `ctxasm` is not part of the project trust boundary. | **PASS** (out of scope by design). | + +--- + +## 4. Concurrency & recovery deep dive (per spec) + +* **Race on `c.projectContextReq`.** Guarded by `c.projectContextMu` + (`internal/runmgr/project_context.go:187–196`). `recheckManifest` calls + `trustedProjectContextRequest` which builds a fresh `req` value and returns + it; the assignment to `c.projectContextReq` happens via `setContextRequest` + **only** when a fresh `Resolve` runs. A recheck that races a fresh `Resolve` + could observe the *prior* scope value, but the recheck compares against + `m.AttemptID`/`m.ProjectID` (persisted) — never against the in-memory scope + identity. + +* **Read reauthorization races.** `projectEvidenceReader` is called inside + `recheckManifest` and `SaveContextManifest`. Both compare the freshly-read + `sha256(evidence.Text)` against the persisted `used.InputSHA256`. The reader + cannot impersonate a previously-recorded body because the hash is recomputed + every time. + +* **In-flight cancellation.** `recheckManifest` consults `ctx.Err()` **before** + the textual-claim walk (line 113) and **after** (line 183). The `Recheck` + provider call itself is wrapped in a 3 s `context.WithTimeout` in + `internal/app/project_context.go:136–143`. `TestProjectSourceRecheckConcurrentCancellationFailsEveryWaiter` + exercises 6 concurrent rechecks against a single shared `runCtx` and confirms + cancellation propagates to every waiter. + +* **Backend resume path.** `trustedProjectContextRequest` re-reads the run and + conversation from `c.mgr.st` on every call. `internal/runmgr/project_context_test.go` + has `TestProjectContextResumeReloadsCurrentVersionAndAuthorization` which + constructs a *second* `runCtx{mgr:m, run:reloaded}` for an `Interrupted` run + and confirms the resume path uses the current authorization (not the + frozen-in-memory snapshot). + +* **Workflow sink ambiguity.** `internal/wfcompile/project_sources.go:88–141` + records the step start, *invalidates* any prior `WorkflowStepSource` binding + on each start, and marks the step `ambiguous` whenever two start events + coexist in the in-progress set. The downstream `BindProjectStepOutput` is a + no-op when `ambiguous`, leaving the judge to fail closed. + `internal/store/workflow_step_source.go:65–110` enforces a bitmask + `state == 3` (bits `attempted` + `responded`) for every attempt ID covered + by the step; `TestProjectStepOutputRejectsUnsettledAttemptInSameStep` + exercises the `attempted` and `unknown` mixed storage cases. + +* **Recovery store under interruption.** The recheck call re-reads from + `ListContextManifests` every time (`internal/runmgr/project_context.go:52`), + so a row committed by an in-flight `SaveContextManifest` is visible to the + next cross-guard call but invisible to a prior one. There is no `BEGIN/END` + window during which `recheckManifest` could observe an inconsistent set of + receipts because each row's gate (`requireRecheckableSources`) is applied + in the same SQL transaction as the insert. + +--- + +## 5. Verdict + +**APPROVED for merge into `feature/w6-x1-source-recheck`** as far as the reviewer's +evidence permits. No blocking or major issues identified. The single grade-M +observations are refinements, not corrections; none block the safety property +the revision claims. + +The reviewer makes no claim about CI green-ness; that requires running the +test suite. Tests were read; not run.