236 lines
22 KiB
Markdown
236 lines
22 KiB
Markdown
# 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_<id>` 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.
|