feat(sage): safely parallelize run-scoped reviews

[Premshay]

Problem / Motivation

Code Review Sage previously serialized independent run-scoped reviews because workers wrote through a shared result path. That protected result attribution, but it also prevented unrelated claimed reviews from using the existing bounded review pool concurrently.

Why it matters

Independent reviews unnecessarily queued behind each other, increasing turnaround time. Restoring concurrency must not let a worker's result be adopted by a different review dispatch.

What changed (motivation → approach → change)

The shared-path adoption boundary now binds each dispatched worker to an unguessable result capability. A record is adopted into a run only when its capability matches the receiving dispatch, so independent claimed reviews can use the bounded pool without cross-run attribution.

The run lock now protects only claim acquisition. Run-scoped reviews delegate concurrency to the pool, while legacy calls without a run scope remain serial. The rebased route keeps failure-reason reporting intact in the lock-free control flow.

Tests

• Route coverage proves independent run bodies overlap and run-scoped reviews delegate concurrency to the bounded pool.
• Record-adoption coverage rejects a mismatched capability, including an attempt to plant a later change's shared result slot.
• Prompt-contract coverage requires the result capability in both initial and follow-up reviewer tasks.
• Focused Sage suite: 302 passed.

Manual verification

N/A — the concurrency and adoption boundary are exercised by deterministic backend and driver tests.

Related Issues

N/A.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat: safely parallelize run-scoped reviews)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (not applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

The template's exact CLA wording has not yet been supplied by OSPO.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #5274 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5274: REBASE. Complementary goals (per-run model pinning vs cross-run concurrency safety) that meet in one function and in the pool's reuse guard. Whoever lands second must reconcile the hunk and decide what an overlapping mismatched-model review does: queue, or surface the busy error. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.
• PR #6802 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6802: CONTINUE_DEVELOPMENT. Same file and same lock region, different goals, verified textual conflict. Both fixes are wanted; whichever lands second must re-place the _CANCELLED recheck against the other's lock scope rather than reapply its own hunk. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.
• This PR is OVERLAPPING with PR #8186. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8143: REBASE. Unrelated goal (failure-cause token vs parallelization) but a direct textual and semantic collision in the rewritten block; the rebase must re-add PR #8186's run["reason"] handling in the new, lock-free control flow. Files: src/kiro_crew/apps/builtins/code_review_sage/backend/routes.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

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

Reviewed 81a860d4509cfc97c12d3c383cf67b1e6e7d5661 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:779 -- Posting deletes the durable retry record
results.result_path(change_id, root, run_id).unlink()
Post request -> record deletion -> gateway termination during dispatch -> findings permanently unavailable for retry.
Anchor: residual/crash-data-loss-corruption
Fix: Keep the record on disk and atomically persist the posting outcome afterward.
[BLOCK-MERGE] 81a860d
[GPT-REVIEWED] 81a860d

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

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

Confirmed from the diff and base tree: on the retry-post path (_post_comments_bg → post_recorded with record is None and run_id is not None, routes.py:1068), the durable record is unlink()ed up front (review_driver.py:779 new), held only in memory as cur/after across the poster dispatch(post_prompt, timeout) (timeout DEFAULT_TASK_TIMEOUT = 5400), and re-persisted only at the very end (if record is None and run_id is not None: results.write_result(after, ...)). A process death anywhere in that up-to-90-minute dispatch window leaves the sole durable copy of the findings gone.

Recovery path: a later retry reads no record → "no recorded review for this change" and cannot post; the PR is typically already in the dedup index from the original run's _record_reviewed, so future repo-reviews skip it — no automated self-correction, only a forced full re-review. The window is a normal user action (manual retry) crossed with an ordinary crash over a long dispatch — not a contradicting-timing / unproducible-input / legacy-only combination. I cannot name a recovery path or argue the conditions are extreme, so the FLAG record cannot be completed. Torn on an unbounded-harm class → UPHOLD-FENCED.

[ADJUDICATION] 81a860d total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 81a860d
[ADJUDICATION-FENCED] 81a860d fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_driver.py:779 -- Retry-post unlinks the only durable findings record and holds it in memory across an up-to-90-min poster dispatch; a crash in that ordinary window loses the findings with no automated recovery and the PR already indexed as reviewed, so the conditions are not extreme enough to FLAG.
[GPT-ADJUDICATED-FENCED] 81a860d

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fix — response-bound handoff genuinely closes the shared-path race — but the code and description document a "result capability" mechanism that does not exist.

Watch

• Phantom mechanism: the new _run_review_bg docstring ("Each worker receives an unguessable result capability; adoption checks that capability"), the PR description, and test docstrings all describe a per-dispatch capability token, but no token is ever generated — validate_result even rejects a result_capability key nothing produces, and tests assert the prompts contain none. The real guarantee is response-channel binding plus exact change_id match (which is adequate, arguably stronger). Leaving a security boundary documented as a mechanism that isn't there means future changes (e.g. reintroducing a file fallback for run-scoped records) get judged against protection that doesn't exist. Rewrite the docstrings/description to name the actual mechanism and drop the dead result_capability rejection.
• Durability regression: records now live only in gateway memory until every worker turn in the run finishes ("Persist its record only after every worker turn has completed"), and the retry-post path unlink()s the durable record before a poster turn of up to 90 minutes and rewrites it only afterwards. A crash/restart mid-run or mid-post loses completed reviews and the posted_keys ledger for comments already delivered — the old flow wrote through at each step. Since the driver never re-reads the file during the run, the on-disk copy is untrusted anyway; deferring/deleting buys little and costs crash recovery.

Suggestions

• Persist each change's record as its _one() completes (the final write_result already overwrites anything a sibling planted), keeping the response channel authoritative without the end-of-run window.

[DESIGN-REVIEWED] 81a860d

[github-actions]

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

Premise-level review of 81a860d4509cfc97c12d3c383cf67b1e6e7d5661 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.

All greps are done and the evidence is in hand. Here is the review.

First-Principles-Verdict: CONCERNS

The described "unguessable result capability" mechanism does not exist in this diff — the code ships a response-channel handoff and explicitly rejects the capability field.

What this change ships

Intent: let independent claimed reviews run concurrently without one run adopting another's findings — an ADDITION (removes a deliberate serialization).

1. Independent run-scoped reviews now overlap; the run lock guards only claim acquisition — justified, cause-level
2. Reviewer returns its record inline in its response; the shared data/results/ staging and its adopt/stake/publish guards are deleted — justified (net deletion of the root cause)
3. Follow-up reviewer gets the prior record in-prompt and returns a replacement inline — justified
4. Poster gets the redacted payload in-prompt and reports delivery via response tags; GitHub read-back stays authoritative — justified
5. Run-dir records persist only after all worker turns finish — justified
6. Standalone (no run_id) runs forced serial; explicit --concurrency silently collapses to 1 — declared, symptom-level
7. Records carrying result_capability are rejected at validation — zero producers

Watch

• Framing: the description says "binds each dispatched worker to an unguessable result capability" and "Prompt-contract coverage requires the result capability in both initial and follow-up reviewer tasks", but the shipped test test_prompt_exposes_no_result_file_or_capability asserts assertNotIn("result_capability", task) and validate_result rejects the field. The narrative describes an abandoned design; a human approving this security relaxation on the description would verify a mechanism that isn't there. The actual mechanism (per-dispatch response channel) is sound — fix the description, not the code.
• Item 6 sits at symptom level with the cause in scope: this same PR rewrites the prompt and SKILL.md so no worker writes data/results/ anymore, so the no-run_id file fallback (if rev_rec is None and not run_id) has zero shipped producers (grep: only test fakes write it) — and it alone forces concurrency = 1, making the CLI's advertised --concurrency flag (review_driver.py _main) a no-op.

Subtractions

• Delete the result_capability rejection branch in results.validate_result — grep result_capability over src/kiro_crew: 0 occurrences in the base tree; in this diff only the validator and two tests. It guards a field from the abandoned capability design that nothing can produce.
• Delete the no-run_id file-read fallback in run_review plus the if not run_id: concurrency = 1 override, restoring the --concurrency flag's meaning — or delete the flag; keeping both ships a knob the code silently ignores.

[FIRST-PRINCIPLES-REVIEWED] 81a860d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both candidates are self-labeled low confidence, and both fail the grounding bar under falsification:

Candidate 1 (unlink-before-post loses the record on a raise): the loss window only opens if an uncaught exception fires between the unlink() and one of the re-write points. It names build_pending_comments and review_payload_units. read_result returns only records that passed validate_result, which pins findings[i].line to a number (results.py:182-185), so int(f.get("line", 0) or 0) in build_pending_comments cannot raise; review_payload_units is pure len()/truthiness over the just-built github_review_payload. On every reachable non-exception path (not new, github_review_payload ValueError, AdapterError, and the terminal if record is None and run_id is not None: write_result(after)) cur is re-persisted. No concrete input (a) exists — confirmed dead.

Candidate 2 (unbounded poster payload in the prompt): no concrete input, no established runtime size limit, and the same-size record already flows through the dispatcher in the review/followup direction (build_review_followup_task inlines json.dumps(record)). Pure "might" on (a) and (c) — dead.

No grounded Step-2 finding meets the bar: the response-bound handoff (result_from_handoff binding to the dispatcher turn, in-memory record= on the posting path, and the post-loop clear_results+write_result) is internally consistent, and records are on disk before report generation.

No findings.

[OPUS-REVIEWED] 81a860d

[Premshay]

The branch now binds reviewer and poster outputs to the dispatcher's response channel; it does not use a result capability or shared staging path. It parses the outer handoff delimiters, persists only after the driver receives a valid response, and retains GitHub read-back as delivery confirmation. The Sage suite passes 773 tests.

[bolichen97]

@Premshay this PR overlaps two of your own newer open PRs in the same files, and the two overlaps pull in opposite directions, so the split is worth settling before further review. Audited at 81a860d.

vs #9077 (response handoff). Shared: sage_lib/results.py, sage_lib/review_driver.py, skills/sage-review/SKILL.md, and four tests under tests/. Both delete the same staging block (clear_staged, stake_shared, adopt_from_shared, publish_to_shared), rewrite build_review_task step 7, and rewrite the same SKILL.md section. #9077 is the more complete version: it issues a per dispatch secrets.token_urlsafe(32) capability compared with hmac.compare_digest and persists each record immediately. This PR uses plain <code-review-sage-result> text delimiters with an exact change_id match and no capability at all: results.validate_result rejects a result_capability key, tests/test_record_adoption.py::test_prompt_exposes_no_result_file_or_capability asserts the prompt carries none, and every write_result is deferred to the end. Suggestion: land the handoff once via #9077 and drop that half here.

vs #9087 (interrupted posting). Shared: backend/routes.py, sage_lib/review_driver.py, tests/test_post_comments.py, tests/test_backend_routes.py; both rewrite post_recorded and build_post_task. #9087's correctness rests on a durable on disk record existing during the poster turn, and its restart recovery walks those records. This PR unlinks the record before dispatching the poster and keeps it only in memory for a turn bounded by DEFAULT_TASK_TIMEOUT=5400. One durability model has to win; please decide it in #9087.

That leaves this PR as the _RUN_LOCK narrowing plus concurrency=0 delegation. Please rebase to that delta; it is 486 commits behind main and the same block also needs re-placing against #6802's _CANCELLED recheck and #5274's model pinned dispatch.

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