fix(sage): bind review results to worker responses

[Premshay]

Problem / Motivation

Sage workers currently write a shared staging result file. Record shape and change ID are validated, but the driver cannot prove the file came from the worker it dispatched.

Why it matters

A concurrent or stale writer can race result attribution. Completed reviewer work is also safer when the driver persists it immediately into the run record.

What changed

Workers return a bounded JSON envelope carrying a driver-issued capability and exact change ID. The driver validates it, atomically writes the result directly to run scope, and binds coverage follow-ups to the immutable first-pass digest. Invalid follow-ups retain the first-pass record. Pool concurrency, schema, and posting behavior are unchanged.

Pattern harvest

Rule candidate: an untrusted worker result needs a driver-issued capability and immediate driver-owned persistence before concurrent work can proceed.

Tests

python -m pytest -q src/kiro_crew/apps/builtins/code_review_sage/tests — 810 passed. Flake8 and git diff --check pass.

What changed (motivation → approach → change)

N/A — covered by the existing ## What changed section.

Manual verification

N/A — focused automated coverage is sufficient.

Related Issues

N/A.

Checklist

• Existing tests pass and regression coverage is included.
• Self-review completed; code follows project style guidelines.
• Documentation updated where applicable.
• No secrets, credentials, or internal references in the diff.

Contribution License Agreement

N/A — template placeholder; no CLA wording is supplied.

[github-actions]

Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)

Design-level review of c6563dc8bf0f490091313a76c0fb7734351eddcc via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: BLOCK

The envelope is parsed from "".join(parts) of the whole 90-minute turn, but the design assumes a clean final message — and the PR verifies neither.

Blockers

The persistence channel's real semantics are never established, and manual verification was waived. _persist_worker_response runs json.loads(raw) on the entire dispatch output and rejects anything that isn't exactly one JSON object ("no Markdown fence or prose"). But the real dispatch (review_pool.send, review_pool.py:561) returns "".join(parts) — the concatenation of every EVENT_TEXT_CHUNK across the multi-tool review turn, not the final message. Any interim narration a worker emits between tool calls (near-certain over a long agentic turn), or one Markdown fence, makes the whole output non-JSON → every real review fails as no_review_recorded → Sage produces zero reviews. Every test mocks dispatch with a clean JSON string, and the description says "Manual verification: N/A — focused automated coverage is sufficient" for exactly the branch CI cannot exercise. Fix: extract the trailing/last JSON envelope from the output (or make the pool surface the final assistant message separately), and run one real-worker review.
Clears when: a real (non-mocked) pool-dispatched review demonstrably persists a record, or the parser tolerantly extracts the envelope from whole-turn output with a test pinning interim-prose-plus-envelope input.

Watch

The follow-up contract requires the LLM to round-trip every first-pass finding byte-exactly (prefix equality against base_record); any paraphrase silently drops the coverage backstop — the feature the follow-up exists for — with no retry. The driver already holds the base record, so a delta-only contract (return net-new findings + coverage fields; driver appends) removes the fragile echo entirely.
Clears when: the follow-up envelope carries only net-new findings, or real-worker evidence shows exact round-tripping is reliable.

[DESIGN-REVIEWED] c6563dc

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of c6563dc8bf0f490091313a76c0fb7734351eddcc via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Confirmed: send() spawns a fresh isolated session per task and returns that session's own final text — the return channel is strictly point-to-point, with no shared write path and no cross-task output multiplexing.

First-Principles-Verdict: CONCERNS

The capability token and base_digest echo authenticate nothing: the return channel is already point-to-point, and the only entity that produces this dispatch's output holds the token in its own prompt.

Not justified as shipped

• Item 2 (capability token) — rides along, inherited premise. send() (review_pool.py:479) spawns a fresh session per task and returns that session's text; dispatch(prompt) output cannot come from any other worker. change_id validation catches a cross-change misroute; the driver rebuilds metadata from its own in-memory base_record (review_driver.py:203). A prompt-injected worker (untrusted-diff boundary) holds capability/base_digest in its prompt and echoes them trivially — so hmac.compare_digest guards against an actor that does not exist on this channel. The remaining defense is "a pool bug might misroute output," which is "it seems safer," not a named harm.

What this change ships

Intent: stop the driver from adopting a shared result file it cannot attribute to the worker it dispatched. FIX.

1. Workers return a JSON envelope in their response instead of writing data/results/<id>.json — justified (deletes the shared-dir trust boundary + its symlink/stake/adopt machinery)
2. Driver mints a per-dispatch capability the worker must echo, checked via hmac — rides along (authenticates nothing over the point-to-point channel + change_id)
3. Follow-up echoes base_digest; driver rebuilds metadata from its own base_record, taking only findings/coverage/summary — driver-side rebuild justified; base_digest echo shares item 2's flaw
4. validate_result now rejects a non-list findings even when falsy (was skipped) — justified, undeclared
5. Failure error text now surfaces the specific parse/validation reason — rides along
6. SKILL.md rewritten to the return-envelope contract — justified

Watch

• Capability/base_digest are echoed secrets held by the only possible producer; they add validation surface without a nameable threat the change_id check and direct return don't cover. Not a one-way door (internal prompt contract), so reversible.
  Clears when: a scenario is named where dispatch(prompt) returns output from a session other than the one spawned for that prompt, or where the capability check refuses a response the change_id+metadata-rebuild path would have accepted.

Subtractions

• Drop the capability field, secrets.token_urlsafe mint, and the hmac.compare_digest block in _persist_worker_response (1 consumer: the check itself); keep the change_id envelope/record match — attribution already holds on the point-to-point return.
• Drop the base_digest echo/compare in the follow-up path; the driver already rebuilds accepted_record from its own base_record, so the echo adds no guarantee.

[FIRST-PRINCIPLES-REVIEWED] c6563dc

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed c6563dc8bf0f490091313a76c0fb7734351eddcc via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_driver.py:1322 -- Legacy runs still trust worker-writable shared results
response_error = _persist_worker_response(...)
rev_rec = results.read_result(change_id, root, run_id)
Standalone concurrent worker writes another change’s shared record -> run_id=None reads it after dispatch -> attacker-controlled findings are attributed and posted to the victim change.
Anchor: residual/security
Fix: Persist and consume responses from a driver-owned private run scope, including standalone runs.

[BLOCK-MERGE] c6563dc
[GPT-REVIEWED] c6563dc

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable input block is empty (0 findings). One fenced finding, F1.

F1 assessment. The finding targets the new response-bound handoff at review_driver.py where _persist_worker_response(...) is followed by results.read_result(change_id, root, run_id) (diff hunk at old-line 1319–1322). I confirmed the residual conditions from code opened this run:

• run_id=None (standalone) routes both write and read to the shared, worker-writable data/results/<id>.json — results.py:39-48 (results_dir), results.py:254-257 (read_result). Reachable in real operation: the CLI _main calls run_review with no run_id and --concurrency default 0 → auto pool cap, over a batch of changes (review_driver.py:1513-1514, 1495-1503).
• read_result validates only the result contract, not that the file's internal change_id matches the requested one (results.py:254-273), so a foreign record sitting at CR-B.json is returned as B's record.
• _persist_worker_response overwrites the path via write_result only when B's own response is valid; on an invalid/absent B response it returns an error and writes nothing (diff lines 149-214), leaving any planted sibling record in place to be read.
• Recovery path: none — the adopted foreign findings are attributed to the victim change and, when posting is enabled, published to it.

The worker is the untrusted boundary the removed stake_shared/adopt_from_shared machinery existed to police; the conditions (standalone shared-dir run + a compromised/injected worker writing a sibling path + that sibling's own review not producing a valid record) are each individually plausible, not mutually contradicting or writer-impossible. I cannot complete a record showing the risk is extreme enough that a human would accept it. Harm rung: UNBOUNDED (cross-change finding attribution / posting attacker-controlled content to a victim PR). Torn → UPHOLD-FENCED.

[ADJUDICATION] c6563dc total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] c6563dc

[ADJUDICATION-FENCED] c6563dc fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_driver.py:1322 -- Standalone (run_id=None) reads/writes the worker-writable shared results dir, and read_result never rechecks the file's internal change_id, so a sibling worker's plant is attributed to the victim change with no recovery; conditions are reachable via the concurrent CLI batch path.
[GPT-ADJUDICATED-FENCED] c6563dc

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c6563dc8bf0f490091313a76c0fb7734351eddcc via the fork AI-review pipeline; updated in place on each push.

Review details

Both candidates rest on unestablished premises. Candidate 1 requires a review record exceeding 1 MB "occurring in practice," which its own evidence admits it cannot confirm (a → "could"). Candidate 2's harm ("LLM reformats prior findings") is speculative model behavior it admits it "cannot measure," and the code is fail-closed (the first pass is preserved). Neither re-derives (a)/(b)/(c) cleanly. I verified the neighboring logic — _finding_counts compares severity == "red"/"yellow", which matches the stored contract values (pipeline.py:318, report.py:359 read the same words; emoji is display-only), and the validate_result change only strengthens rejection of a falsy non-list findings. No grounded Step-2 defect surfaced.

No findings.

[OPUS-REVIEWED] c6563dc

[bolichen97]

@Premshay this PR and #9087 are complementary halves of the same durability theme, and both can land. Audited at fb80274; the head has since moved to c6563dc, but the 7-file set is unchanged.

Overlap with #9087: both edit src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_driver.py and tests/test_post_comments.py, but the functions are disjoint. This PR rewrites build_review_task, build_review_followup_task and the run_review dispatch site, and adds _persist_worker_response with the secrets.token_urlsafe capability and _record_digest binding. #9087 rewrites build_post_task and post_recorded for the delivery-intent outbox. Neither touches the other's functions, so the conflicts are textual only: both add a hashlib import, and both add scaffolding to tests/test_post_comments.py, where this PR rewrites the dispatch fakes that #9087's new TestDeliveryOutbox class sits beside. Both PRs are yours, so please just tell us the landing order and rebase the second one.

The real blocker is #8143, also yours. It replaces the same shared-staging result channel at the same dispatch sites, and it additionally converts the poster path, the review pool and backend/routes.py, so whichever of #9077 and #8143 merges second has to be rewritten rather than merged. Please pick one mechanism, tolerant marker extraction or the strict whole-output envelope, decide whether the per-dispatch capability and first-pass digest binding stay, and fold the hardening into that single change.

Two undeclared changes worth adding to the body: results.py now rejects any non-list findings for every record write app-wide, and results.stake_shared and clear_staged lose their last production callers here without being deleted.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.