PAN-3511 - #3529
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds host-attested review artifacts, trusted verdict reads, bounded memoization, safe verdict restoration, and artifact-first recovery across review completion, journal reconciliation, Deacon recovery, feedback delivery, and stall sweeping. ChangesAttested verdict recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/lib/cloister/__tests__/verdict-restore-breaker.test.ts (2)
111-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the real breaker threshold instead of duplicating it.
BREAKER_THRESHOLD = 3duplicatesREVIEW_INFRA_BREAKER_THRESHOLD. If the production constant changes,status()no longer builds a row that reaches the breaker, and these tests pass while covering a different branch. Import the exported constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/verdict-restore-breaker.test.ts` around lines 111 - 112, Replace the local BREAKER_THRESHOLD constant in the verdict-restore-breaker tests with the exported REVIEW_INFRA_BREAKER_THRESHOLD production constant, importing it from its existing module and using it wherever the test constructs the breaker threshold.
218-257: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a breaker case for a head-mismatched artifact.
No artifact in this suite writes
context.json, so every artifact carries no head evidence andrestoreWouldTripHeadGuard()can never trip. Theblocked-by-head-guardbranch ofartifactSupersededBreakeris therefore untested at both breaker sites, and that branch decides whether the stuck mark is skipped. Add a case that writescontext.jsonwith a head that differs from the row'slastVerifiedCommit, then assert the intendedmarkWorkspaceStuckbehavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/verdict-restore-breaker.test.ts` around lines 218 - 257, The ac3 breaker suite does not cover the head-mismatch path that produces the blocked-by-head-guard result. Add a test case using writeArtifact to create context.json with a head different from the row’s lastVerifiedCommit, exercise the relevant coordinator-death or orphan-status function, and assert the intended markWorkspaceStuck behavior for artifactSupersededBreaker; cover the breaker site applicable to the fixture while preserving existing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/PARKED-POPULATION.md`:
- Around line 47-54: Update the “stuck-flag” orbit documentation to state that a
fresh passed artifact restores the verdict and clears the stuck flag only when
its recorded head matches the row anchor. Document that a head mismatch leaves
the row parked with the stuck flag set, while preserving the existing behavior
for matching passed artifacts and other verdict states.
In `@src/lib/cloister/deacon-review-status.ts`:
- Around line 346-366: Update src/lib/cloister/deacon-review-status.ts lines
346-366 in artifactSupersededBreaker so it returns true only when
restore.outcome is restored; blocked restores must return false so both breaker
call sites still mark the workspace stuck. Update
src/lib/cloister/feedback-target.ts lines 297-306 to use the same restored-only
check, ensuring blocked restores continue applying
FEEDBACK_DELIVERY_STUCK_REASON.
- Around line 616-623: Update the blocked-by-head-guard branch in
checkOrphanedReviewStatuses so repeated blocked patrols cannot leave the
reviewing row permanently unreachable. Track or increment reviewRetryCount and,
once the existing bounded escalation threshold is reached, discharge or
transition the row through the established safe write path; preserve the current
report-only behavior before that limit and do not reset the finished artifact.
In `@src/lib/cloister/stall-sweeper.ts`:
- Around line 377-389: Update the artifact flow in the passed-artifact branch
around readArtifact and restoreVerdict so the decision and restoration use the
same artifact snapshot. Pass the selected artifact into restoreVerdict if
supported, or require restore.artifact.verdict to remain passed before clearing
the stuck flag and emitting sweep.unparked, using that artifact for event data;
otherwise retain the parked outcome. Add coverage for differing verdicts across
the two reads.
In `@src/lib/cloister/synthesis-verdict.ts`:
- Around line 124-130: Update the cache-hit logic in the verdict memoization
flow around artifactVerdictMemo and readLatestSynthesisVerdict so non-null
cached verdicts expire at the earlier of ARTIFACT_VERDICT_MEMO_TTL_MS and the
artifact freshness boundary; preserve null-entry behavior as appropriate. Add a
boundary test covering an artifact that becomes stale while still within the
memo TTL.
In `@src/lib/cloister/verdict-restore.ts`:
- Around line 148-155: Update the default dependency wiring used by
attemptArtifactVerdictRestore to read artifacts through
readMemoizedArtifactVerdict instead of readLatestSynthesisVerdict, and ensure
the deacon-review-status override uses that memoized reader rather than
replacing it with the non-memoized implementation.
In `@src/lib/review-status-read.ts`:
- Around line 52-62: Update the stale-journal branch in the review-status read
flow to require the matching artifact’s headSha to pass the same live-status
head-anchor validation used by attemptArtifactVerdictRestore before calling
reconcileJournalIntoCacheSync. Preserve the existing verdict-match requirement,
and add a test covering an artifact with a mismatched headSha to ensure
reconciliation is not performed.
---
Nitpick comments:
In `@src/lib/cloister/__tests__/verdict-restore-breaker.test.ts`:
- Around line 111-112: Replace the local BREAKER_THRESHOLD constant in the
verdict-restore-breaker tests with the exported REVIEW_INFRA_BREAKER_THRESHOLD
production constant, importing it from its existing module and using it wherever
the test constructs the breaker threshold.
- Around line 218-257: The ac3 breaker suite does not cover the head-mismatch
path that produces the blocked-by-head-guard result. Add a test case using
writeArtifact to create context.json with a head different from the row’s
lastVerifiedCommit, exercise the relevant coordinator-death or orphan-status
function, and assert the intended markWorkspaceStuck behavior for
artifactSupersededBreaker; cover the breaker site applicable to the fixture
while preserving existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3264fbbe-ed99-468a-9e54-3a20a585eb88
📒 Files selected for processing (17)
docs/PARKED-POPULATION.mddocs/REVIEW-AGENT-ARCHITECTURE.mdpackages/contracts/src/events.tsroles/review.mdscripts/file-size-allowlist.txtsrc/lib/__tests__/review-status-read-artifact.test.tssrc/lib/cloister/__tests__/feedback-target.test.tssrc/lib/cloister/__tests__/stall-sweeper.test.tssrc/lib/cloister/__tests__/verdict-restore-breaker.test.tssrc/lib/cloister/__tests__/verdict-restore.test.tssrc/lib/cloister/deacon-review-status.tssrc/lib/cloister/feedback-target.tssrc/lib/cloister/stall-sweeper.tssrc/lib/cloister/synthesis-verdict.tssrc/lib/cloister/verdict-restore.tssrc/lib/review-status-read.tstests/unit/lib/head-anchor-write-sites.test.ts
![panopticon-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Adds src/lib/cloister/verdict-restore.ts — the single read-side decision point every recovery path will call before it marks, resets, or re-drives a review row. No call sites yet; this item is the contract the next four items adopt. attemptArtifactVerdictRestore() reads the freshest verdict artifact through the already-landed readLatestSynthesisVerdict(), predicts whether writing it would be rejected for disagreeing with the row's anchor, and either restores the verdict through setReviewStatusSync or reports the refusal. It writes nothing on 'no-artifact' and nothing on 'blocked-by-head-guard', so an absent artifact never invents approval (NFR-4). restoreWouldTripHeadGuard() is the prediction, kept as one exported pure function so a future guard change has exactly one place to land (hazard H3). Blocked restores report through emitActivityEntryOnce with an id keyed on the exact condition, so a persistently-rejected restore tells the operator once rather than once per ~60s patrol (hazard H7), while the domain event still appends every time. Extra file beyond files_scope, and why: packages/contracts/src/events.ts — CloisterEventStore.append() is typed to the contracts DomainEvent union, so review.verdict_restore_blocked has to be registered there to be a first-class domain event rather than an unvalidated blob. This mirrors what PAN-3512 did for review.verdict_rejected and review.verdict_dispatched. Deviations from the item text, both deliberate: 1. The item cites "the review arm of findVerdictEvidenceHeadMismatch at src/lib/review-verdict-guards.ts:42-54". That arm no longer exists — PAN-3512 (fd48e3a, already on main and in this branch's history) deleted it, and the head-anchoring semantic now lives in the verdict write door at review-verdict-writer.ts:151-213. The module documents the live location rather than the stale one. The predicate itself is unchanged from what decision D3 specifies, and is deliberately CONSERVATIVE: it blocks on any two differing heads, where the write door would additionally classify a differing head as fresh/indeterminate and land it. Making the loss visible is this issue; making it land is PAN-3512. 2. The helper restores whatever verdict the artifact carries, not just 'passed', per the item's "reviewStatus: artifact.verdict". The landed orphan restore at deacon-review-status.ts:594 currently gates on 'passed' only, so the orphan-restore-adopt item must decide whether to preserve that gate at its call site — the helper does not impose it. Verification: 20 new tests pass, typecheck clean, eslint clean, file-size guard clean. Fixtures exercise BOTH artifact filenames through the real reader — synthesis.md (convoy) and review.md (quick self-review, the fleet default) — per hazard H5, including the no-head-evidence path that most production restores take, and CHANGES REQUESTED as quick mode's blocked vocabulary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
…fusal (PAN-3511) The advancing-journal reconcile is the fourth recovery path that acted on a review row without consulting the verdict of record. A reviewer writes its artifact seconds to minutes before that verdict syncs into the row, so a journal replay that merely LOOKS stale against the live cycle may in fact be the finished review — and the resolver refused it. Two changes, both sized by the fact that the resolver is the hot read path: 1. synthesis-verdict.ts gains readMemoizedArtifactVerdict() — a 60s per-issue TTL over readLatestSynthesisVerdict, with an explicit `now` so the TTL is testable without wall-clock drift, and __resetArtifactVerdictMemo() so the module-level Map cannot leak between tests (hazard H4). Absence is memoized too, since "no artifact" is the common case and re-scanning for a missing file every read is the exact cost this avoids. 2. review-status-read.ts consults it ONLY inside the branch that already refuses the replay. When a fresh artifact carries the SAME terminal verdict the journal is replaying, the artifact independently corroborates it and the reconcile proceeds; an absent or disagreeing artifact leaves the refusal exactly as it was. The consult is strictly one-directional — it can only lift a refusal the resolver was already making, never invent an approval, which is what keeps NFR-4 true. Because it sits behind an already-rare branch and is memoized, the common read path performs zero filesystem work and a refusing path at most one scan per issue per minute. Import direction verified acyclic: review-status-read.ts -> cloister/ synthesis-verdict.ts -> projects.ts, which reaches errors/paths/issue-id only and never re-enters review-status. This matters because the dashboard runs strict ESM and rejects circular imports at runtime, where typecheck would not. Verification: 9 new tests pass (29 across both PAN-3511 suites), typecheck clean, eslint clean, file-size guard clean. The TTL cases prove the no-restat property by deleting the artifact between reads and asserting the memo still serves it, rather than by spying on fs. Pre-existing red observed while checking for regressions, NOT caused by this change and deliberately not fixed here: tests/unit/lib/review-status.test.ts > "rejects a review verdict whose evidence HEAD differs from the verified target HEAD" fails on main. It asserts the review arm of findVerdictEvidenceHeadMismatch, which PAN-3512 deleted in fd48e3a without updating the test (last touched by PAN-3377). Related reds in tests/cli/commands/specialists/done.test.ts ("Review verdict rejected: issue-not-found") and review-verdict-writer.test.ts (0 tests collected) are the same PAN-3512 surface. None of those files import the modules changed here. Left for PAN-3512 to finish rather than patched into this item's scoped diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
… door (PAN-3511) Replaces the inline race-guard block landed by 93b88ee with a call to attemptArtifactVerdictRestore. Behavior-preserving on the writable path, plus the AC-READ-GUARD emit the inline block had no way to produce. The orphan patrol installs a passed-only reader through the helper's injection seam, so ONLY an approved artifact short-circuits the reset — exactly as before. A blocked or failed artifact still yields 'no-artifact' and falls through to the unchanged pending reset. Restoring a non-approved verdict here would be a real behavior change this item does not carry; the helper itself stays general and the gate lives at the call site. On 'blocked-by-head-guard' the patrol now returns instead of resetting. The artifact proves a review FINISHED, so falling through to pending would be the PAN-1577 wipe by another name — the exact failure this issue exists to close. Line count: 978 before, 978 after (AC4 — no more lines than before), so the file stays at its 1000-line ratchet with no allowlist row needed. The inline block collapsed by 2 lines, which paid for the new import and the blocked-branch logging. Test scope, stated plainly: reconcileReviewStatusOrphan is not exported and needs the whole patrol surface (agents, tmux, status) to drive end-to-end, and no existing suite covers it — it was untested machinery, which is part of why the wipe went unnoticed. The four new cases therefore pin the decision the call site delegates (passed restores and clears the infra gate, mismatched head reports and writes nothing, blocked/failed and absent both fall through), not the patrol function itself. End-to-end orphan-patrol coverage remains a gap. Verification: 25 tests pass in verdict-restore.test.ts, typecheck clean, eslint clean, file-size guard clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
…ck mark (PAN-3511) surfaceIssueFeedbackNeedsYou marked an issue feedback_delivery_needs_you whenever no live feedback target could be resurrected — including when the review had already FINISHED and only its delivery failed. That strands a completed review behind an operator gate for a transport problem the delivery machinery retries on its own. The consult now runs before the mark. On 'restored' or 'blocked-by-head-guard' the stuck mark is skipped, because either outcome means an artifact exists and a verdict was reached; only 'no-artifact' falls through to the existing markWorkspaceStuck call with its arguments unchanged. The consult fails TOWARD the stuck mark. It sits in its own try/catch that degrades a reader error to 'no-artifact' rather than letting it escape to the outer catch, which would have swallowed the mark entirely and silently removed the protection that exists today (ac4 pins this). Tests cover both artifact shapes through the real reader — synthesis.md for a convoy and review.md for quick self-review, which is the fleet default — plus the no-artifact, reader-throws, and head-mismatch paths. The suite's existing existsSync mock is registered with each real artifact path so the reader's findVerdictReport resolves it. Verification: 14 tests pass (9 pre-existing, 5 new), typecheck clean, eslint clean, file-size guard clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
… trips (PAN-3511) Both review-infrastructure breaker sites in deacon-review-status.ts marked a row stuck once reviewRetryCount crossed the threshold, without ever asking whether a verdict already existed on disk. Retries exhausted while the reviewer's artifact is sitting there means recovery was chasing a review that had already FINISHED — stranding it behind an operator gate is the PAN-1577 wipe by another name. A shared artifactSupersededBreaker() helper runs the restore before markWorkspaceStuck at both sites (handleReviewCoordinatorDied and the orphan re-dispatch path). It fails toward the mark: a reader error returns false so today's protection survives unchanged. verdict-restore-breaker.test.ts drives both sites end to end through their real exported entry points, with a real artifact on disk read by the real reader, so ac4's quick-mode review.md claim is genuinely proven rather than mocked. The no-artifact cases assert the unchanged markWorkspaceStuck details payload at each site, which is what makes the artifact cases a real differential. Two fixes to fallout from 2efeea3 in the same commit: - verdict-restore.ts bound emitActivityEntryOnce at import time, so importing it forced every transitive importer's test to mock an export only the blocked path uses. Nine suites across cloister/ and tests/lib/ died at module load. Calling through instead of binding removes the import-time coupling; no test file needed patching once the root cause was gone. - The HeadAnchor write-site inventory listed the artifact stamp under deacon-review-status.ts, where it no longer lives. Re-registered at its real home in verdict-restore.ts, and restoreWouldTripHeadGuard's row parameter is renamed lastVerifiedCommit -> rowHead so a READ stops registering as a write in an inventory whose whole job is to be truthful about writes. deacon-review-status.ts lands at 1001 lines against an origin/main baseline of 978, so scripts/file-size-allowlist.txt carries a matching PAN-3511 row. Two inventory violations remain and are NOT from this branch: legacy-routes.ts and review-verdict-writer.ts are PAN-3512 sites that origin/main has already registered. This branch is simply behind main; a sync clears them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
…g orbit (PAN-3511) The stall sweeper's stuck-flag orbit re-drove work agents and re-dispatched reviews without ever asking whether a verdict already existed on disk. The consult now runs BEFORE every branch — infra-failure re-dispatch, the feedback-delivery / verification rework re-drive, and the operator resurface. Ordering is the whole point: consulting later would let a passed artifact still push a rework message at the agent before the restore short-circuited. On a fresh passed artifact the sweeper restores, clears the stuck flag, emits sweep.unparked with action 'verdict-restored', and returns having sent zero messages. A passed artifact the head guard blocks also stands the sweeper down — the artifact still proves a review finished, and the restore door already reported the mismatch. On a blocked/failed artifact the existing re-drive stays, but the feedback body now carries the reviewer's actual blocker instead of pointing the agent at a directory to go read. No artifact changes nothing. 23 cases pass (17 existing, unchanged, plus 6 new). Also fixes an import cycle this branch introduced in af3a9fc, which npm run lint caught here and which Node's strict ESM would reject at runtime: review-status.ts reaches up into cloister/feedback-target.ts, and feedback-target now calls the restore door, so verdict-restore's own review-status import closed the loop six ways. verdict-restore no longer imports review-status at all — it declares the two row fields it reads and the update it writes structurally, and getStatus/setStatus become required deps that each caller lends from its own import. That is also the honest contract: the door is a decision function over injected state access, not a store client. madge counts type-only and dynamic imports alike, so nothing short of removing the edge would have worked. deacon-review-status.ts grows 1001 -> 1005 for the lent accessors; its PAN-3511 allowlist row moves with it in this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
docs/REVIEW-AGENT-ARCHITECTURE.md gains a "Verdict of record" section carrying the four clauses, the two doors (readLatestSynthesisVerdict to read, attemptArtifactVerdictRestore to decide), the one-directional rule that an absent artifact is never an approval, and a table of the five recovery sites with what each one did before the consult existed. A review-mode table follows, because the fleet default is quick self-review and a reader that only understood synthesis.md would be blind to most production reviews. The AC-READ-GUARD limitation is stated plainly rather than left implicit: restoreWouldTripHeadGuard is deliberately stricter than the write door, so a restore it blocks is surfaced through review.verdict_restore_blocked and never dropped. Landing those is PAN-3512's write door, which is already shipped — the plan for this item described that clause as unshipped, which was true when the plan was written and is not true now, so it is documented as PAN-3512's scope rather than as future work. docs/PARKED-POPULATION.md records the stuck-flag orbit's consult-first ordering. roles/review.md's SELF-REVIEW banner now carries a staleness warning. This is verified, not assumed: resolveReviewMode() defaults to quick, but the operator's resolved config sets roles.review.mode: full, so convoys really do run while the banner tells the reviewer they are disabled. The note tells the reviewer to trust the spawn prompt over the banner and flags reconciling mode policy as a separate follow-up this issue deliberately does not take on. Prompt-Change: flag the stale SELF-REVIEW banner; trust the spawn prompt over it Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
…ranch (PAN-3511) Both failures predate this branch — the frontend tree here is byte-identical to origin/main — but they blocked PAN-3511's verification gate, so they are fixed rather than waited out. system-health-ui-no-loss.test.tsx: three assertions failed with "Found multiple elements", not "Unable to find". Every audited affordance is present; the queries just could not survive normal DOM nesting. The summary line renders into a leaf div inside a bordered wrapper whose only child it is, so both nodes match even when the regex is anchored, and the consumer and attention rows legitimately match several elements each. Switched to getAllBy* asserting presence, which is the property the no-loss audit is actually about — getBy* was failing on the section being MORE populated, never less. The audit is not weakened: each assertion still requires the named content to be visible in the dialog. tiered-crews.test.ts: blendedCost expected 6.525, which was the weighted blend while gpt-5.6-terra was priced 8.75. PAN-3388 (e5da1b5) repriced it to 7 for the 272K billing tier and did not update this literal. Recomputed against the catalog — haiku 1x10 + sonnet 6x40 + terra 7x30 + gemini 7x20 = 600 over weight 100 — so the expectation is 6. The catalog is the source of truth for price and this test asserts the blending math, so the literal tracks it; the reasoning is recorded inline so the next repricing is a one-line update, not an excavation. Frontend suite is now fully green: 306 files, 2993 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2nHLZQDncBvcnH7VicyLC
3eec512 to
71c3951
Compare
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-04T02:44:36ZVerdict: CHANGES REQUESTED — workspace-writable verdict artifacts can forge review approvalContext
Convoy Status
Blocking Findings[security] Workspace-writable verdict artifacts can forge review approval —
|
1 similar comment
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-04T02:44:36ZVerdict: CHANGES REQUESTED — workspace-writable verdict artifacts can forge review approvalContext
Convoy Status
Blocking Findings[security] Workspace-writable verdict artifacts can forge review approval —
|
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/cloister/__tests__/synthesis-artifact-verdict.test.ts (1)
83-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the two remaining rejection branches.
The suite covers the forged capability, the mismatched context, and the freshness boundary. Two new rejection branches in
resolveTrustedReviewRunandreadRunContexthave no test:
- A
runIdthat failsbasename(runId) !== runIdor theagent-<issue>-reviewprefix check.- A run directory with a report but no
context.json.Both branches guard artifact authorization, so a regression in either would silently widen what recovery accepts. Also consider a case where the marker appears on the second line, which pins the first-line-only comparison.
💚 Proposed additional cases
it('returns null for a run without a verdict line', () => { const runId = 'agent-pan-1-review-empty'; runDir(runId, '# no verdict here\n'); expect(read(runId)).toBeNull(); }); + + it('rejects a runId that escapes the review directory or misses the run prefix', () => { + expect(read('../../../etc')).toBeNull(); + expect(read('agent-pan-2-review-other')).toBeNull(); + }); + + it('rejects a run directory with no context.json', () => { + const runId = 'agent-pan-1-review-nocontext'; + runDir(runId, '## Verdict: APPROVED\n'); + rmSync(join(root, '.pan', 'review', runId, 'context.json')); + expect(read(runId)).toBeNull(); + }); + + it('rejects a marker that is not the first line', () => { + const runId = 'agent-pan-1-review-latemarker'; + const dir = join(root, '.pan', 'review', runId); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, 'synthesis.md'), + `# heading\n${reviewArtifactCapabilityMarker(CAPABILITY)}\n## Verdict: APPROVED\n`, + ); + writeFileSync(join(dir, 'context.json'), JSON.stringify({ issueId: ISSUE, runId })); + expect(read(runId)).toBeNull(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/synthesis-artifact-verdict.test.ts` around lines 83 - 108, Add three test cases to cover untested rejection branches: add a test for a runId that fails the basename or agent-<issue>-review prefix validation in resolveTrustedReviewRun, add a test for a run directory with a report file but no context.json file in readRunContext, and add a test where the verdict marker appears on the second line instead of the first line. Each case should verify that read returns null, consistent with the rejection behavior for the other branches already tested above these additions.src/lib/cloister/synthesis-verdict.ts (1)
67-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake artifact provenance resolution fail safe on the review-status hot path.
resolveJournalReconciledReviewStatusSynccallsreadMemoizedArtifactVerdict(issueId)without atry. That callsresolveTrustedReviewRun, which invokes bothgetReviewArtifactProvenanceSyncandresolveProjectFromIssueSyncbefore thetryinreadLatestSynthesisVerdict. MakereadMemoizedArtifactVerdictreturnnullwhen provenance resolution fails, or wrap the artifact consult with an equivalent boundary catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/synthesis-verdict.ts` around lines 67 - 84, Make the artifact lookup fail safe by catching errors from resolveTrustedReviewRun within readMemoizedArtifactVerdict and returning null when provenance resolution fails. Ensure resolveJournalReconciledReviewStatusSync cannot propagate these errors before readLatestSynthesisVerdict’s existing try boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/overdeck/infra.ts`:
- Around line 196-197: Update the v54→v55 migration so the agents table includes
review_artifact_capability before backfillAgentsFromStateJsonSync() prepares its
query. Make this part of the v55 table definition or perform an idempotent ALTER
TABLE before the backfill, while retaining the existing runSchemaTopUp call only
as a repair path rather than the primary migration.
---
Nitpick comments:
In `@src/lib/cloister/__tests__/synthesis-artifact-verdict.test.ts`:
- Around line 83-108: Add three test cases to cover untested rejection branches:
add a test for a runId that fails the basename or agent-<issue>-review prefix
validation in resolveTrustedReviewRun, add a test for a run directory with a
report file but no context.json file in readRunContext, and add a test where the
verdict marker appears on the second line instead of the first line. Each case
should verify that read returns null, consistent with the rejection behavior for
the other branches already tested above these additions.
In `@src/lib/cloister/synthesis-verdict.ts`:
- Around line 67-84: Make the artifact lookup fail safe by catching errors from
resolveTrustedReviewRun within readMemoizedArtifactVerdict and returning null
when provenance resolution fails. Ensure
resolveJournalReconciledReviewStatusSync cannot propagate these errors before
readLatestSynthesisVerdict’s existing try boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3e54f53-afc8-42f4-b83d-a5c15418fe54
📒 Files selected for processing (33)
docs/PARKED-POPULATION.mddocs/REVIEW-AGENT-ARCHITECTURE.mddrizzle/overdeck/0000_overdeck_init.sqlpackages/contracts/src/events.tsroles/review.mdscripts/file-size-allowlist.txtsrc/lib/__tests__/review-status-read-artifact.test.tssrc/lib/agents/agent-state.tssrc/lib/cloister/__tests__/feedback-target.test.tssrc/lib/cloister/__tests__/stall-sweeper.test.tssrc/lib/cloister/__tests__/synthesis-artifact-verdict.test.tssrc/lib/cloister/__tests__/verdict-restore-breaker.test.tssrc/lib/cloister/__tests__/verdict-restore.test.tssrc/lib/cloister/deacon-review-signals.tssrc/lib/cloister/deacon-review-status.tssrc/lib/cloister/feedback-target.tssrc/lib/cloister/review-agent.tssrc/lib/cloister/review-artifact-capability.tssrc/lib/cloister/stall-sweeper.tssrc/lib/cloister/synthesis-verdict.tssrc/lib/cloister/verdict-head-guard.tssrc/lib/cloister/verdict-restore.tssrc/lib/database/agent-backfill.tssrc/lib/database/agent-mappers.tssrc/lib/database/agents-db.tssrc/lib/database/schema.tssrc/lib/overdeck/agent-review-provenance.tssrc/lib/overdeck/agent-state-sync.tssrc/lib/overdeck/agents.tssrc/lib/overdeck/infra.tssrc/lib/review-status-read.tstests/unit/lib/head-anchor-write-sites.test.tstests/unit/lib/overdeck/agent-discovery-columns.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- scripts/file-size-allowlist.txt
- tests/unit/lib/head-anchor-write-sites.test.ts
- src/lib/cloister/tests/verdict-restore-breaker.test.ts
- packages/contracts/src/events.ts
- src/lib/cloister/feedback-target.ts
- src/lib/cloister/tests/feedback-target.test.ts
- src/lib/cloister/tests/stall-sweeper.test.ts
- src/lib/cloister/tests/verdict-restore.test.ts
- docs/PARKED-POPULATION.md
- src/lib/cloister/stall-sweeper.ts
- roles/review.md
- src/lib/review-status-read.ts
- src/lib/tests/review-status-read-artifact.test.ts
- docs/REVIEW-AGENT-ARCHITECTURE.md
| // PAN-3511: host-issued capability binds verdict recovery to the active review run. | ||
| runSchemaTopUp(db, 'ALTER TABLE `agents` ADD COLUMN `review_artifact_capability` text'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add the capability column before the v55 backfill runs.
The v54→v55 migration creates agents without review_artifact_capability. It then calls backfillAgentsFromStateJsonSync(). The changed backfill query includes this column, so statement preparation fails before this runtime top-up executes. The migration catches the error and does not retry the backfill. Existing agent state can therefore be missing after upgrade.
Add the column to the v55 table definition or run an idempotent ALTER TABLE before the backfill. Keep this top-up as a repair path, not as the primary migration. As per coding guidelines, fix broken behavior at its root cause; do not add a downstream workaround that masks the symptom.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/overdeck/infra.ts` around lines 196 - 197, Update the v54→v55
migration so the agents table includes review_artifact_capability before
backfillAgentsFromStateJsonSync() prepares its query. Make this part of the v55
table definition or perform an idempotent ALTER TABLE before the backfill, while
retaining the existing runSchemaTopUp call only as a repair path rather than the
primary migration.
Source: Coding guidelines
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-04T15:22:00ZVerdict: CHANGES REQUESTED — the persisted capability remains forgeable by the agents it is meant to excludeContext
Convoy Status
Blocking Findings[security] The review capability is readable by the agents it is supposed to exclude —
|
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
src/lib/cloister/__tests__/feedback-target.test.ts (1)
261-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the absence of a redundant review write.
The test name claims the delivery gate survives when review already passed. The body only checks
markWorkspaceStuck. Add an assertion thatsetReviewStatusSyncwas not called, so the test actually proves that an already-passed row is not rewritten from the artifact.💚 Proposed addition
expect(reviewStatus.markWorkspaceStuck).toHaveBeenCalledWith( ISSUE, 'feedback_delivery_needs_you', { reason: 'test feedback could not be delivered', phase: 'test' }, ); + expect(reviewStatus.setReviewStatusSync).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/feedback-target.test.ts` around lines 261 - 272, Extend the test case “preserves a test-feedback delivery gate after review already passed” to assert that reviewStatus.setReviewStatusSync is not called, while retaining the existing markWorkspaceStuck expectation.src/lib/cloister/synthesis-verdict.ts (2)
88-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed failure before returning null.
This
catchcoversresolveTrustedReviewRun, which performs a SQLite read and a project resolution. A database or configuration failure is indistinguishable from "no fresh artifact exists". Recovery then stays blocked with no signal. Emit a warning with the issue id and the error, and keep returningnull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/synthesis-verdict.ts` around lines 88 - 91, Update the catch handling around resolveTrustedReviewRun to capture the thrown error and emit a warning containing the issue id and error details before returning null. Preserve the existing null return behavior and do not alter successful resolution.
16-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
mtimeMsor correct its documentation.The field now carries
Date.parse(valid.issuedAt)from the attestation, not the file mtime.readAttestedReviewReportssets it that way insrc/lib/cloister/review-artifact-attestation.tsLine 234. The comment at Line 22 still says "mtime (ms) of the verdict artifact". A caller that compares this value against a real file mtime gets a wrong result. Rename the field toattestedAtMs, or update the comment to state that it is the attestation issue time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/synthesis-verdict.ts` around lines 16 - 24, Rename SynthesisArtifactVerdict.mtimeMs to attestedAtMs and update all references, including readAttestedReviewReports, to reflect that the value is Date.parse(valid.issuedAt) from the attestation rather than the artifact file mtime.src/cli/commands/specialists/done.ts (1)
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd braces to the guarded
try.
if (!workspaceHead) try { ... } catch { ... }is valid, but the brace-lessifmakes the guarded region hard to see and easy to break during a later edit. Wrap thetryin a block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/specialists/done.ts` around lines 176 - 177, Wrap the try/catch guarded by if (!workspaceHead) in braces in the workspaceHead initialization flow, preserving the existing try/catch behavior and all statements within its guarded region.src/lib/cloister/__tests__/review-artifact-attestation.test.ts (1)
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the attestation key environment variable after the suite.
installTestReviewAttestationKeywritesOVERDECK_REVIEW_ATTESTATION_KEYintoprocess.envand nothing removes it. The value then persists for other suites in the same Vitest worker. Usevi.stubEnvwithvi.unstubAllEnvs, or delete the variable inafterEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/review-artifact-attestation.test.ts` around lines 21 - 28, Update the setup and teardown around installTestReviewAttestationKey in the review attestation test suite to restore OVERDECK_REVIEW_ATTESTATION_KEY after each test. Prefer vi.stubEnv with vi.unstubAllEnvs, or explicitly delete the environment variable in afterEach while preserving the existing temporary workspace cleanup.tests/cli/commands/specialists/done.test.ts (1)
137-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for a mismatched identity and a failed attestation.
The new tests cover a missing token and the success path. Two rejection branches in
done.tsLines 80-98 stay untested: acallerAgentIdthat does not equalagent-<issue>-review, and a non-ok attestation response. Both must throw before any review status write. Add a case for each.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/commands/specialists/done.test.ts` around lines 137 - 177, Add two negative tests alongside the existing attestation tests for doneCommand: one with a callerAgentId that does not match the expected agent-<issue>-review identity, and one where the attestation fetch returns a non-OK response. Assert each rejects and verify mockSetReviewStatus is not called in both cases.src/lib/cloister/__tests__/review-artifact-test-helpers.ts (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not write
headShawhenreposis supplied.The fixture always writes
headSha, and addsreposon top.canonicalReviewedHeadFromContextprefersrepos, so theheadShavalue is silently ignored. A polyrepo fixture then carries a monorepo anchor that no production context would contain. OmitheadShawhenreposis present, so the fixture matches the real manifest shape.♻️ Proposed refactor
writeFileSync(contextPath, JSON.stringify({ issueId: options.issueId, runId: options.runId, - headSha: options.headSha ?? TEST_REVIEW_HEAD, - ...(options.repos ? { repos: options.repos } : {}), + ...(options.repos + ? { repos: options.repos } + : { headSha: options.headSha ?? TEST_REVIEW_HEAD }), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/review-artifact-test-helpers.ts` around lines 32 - 37, Update the fixture construction in the writeFileSync call so headSha is included only when options.repos is absent, while preserving the existing default TEST_REVIEW_HEAD for monorepo fixtures and the repos field for polyrepo fixtures.src/lib/cloister/review-artifact-attestation.ts (1)
164-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord why a candidate report was skipped.
The
catchblock at Line 180 discards every failure, including a corrupt read or astatSyncerror. WhenattestReviewReportthen throwsno ${expectedVerdict} review report exists, the operator has no evidence about the real cause. Log the filename and the error at debug level before continuing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/review-artifact-attestation.ts` around lines 164 - 186, Update the catch block in the candidate-report loop to log a debug-level message containing the skipped filename and caught error before continuing to the next report. Preserve the existing fallback behavior and final no-report error in the surrounding review report selection flow.src/lib/review-attestation-key.ts (1)
57-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
verifyReviewAttestationSignaturefor token verification.
verifyReviewAgentAttestationTokenrepeats the length check andtimingSafeEqualcompare fromverifyReviewAttestationSignature. Delegate instead, so one compare implementation stays authoritative.♻️ Proposed refactor
export function verifyReviewAgentAttestationToken( agentId: string, runId: string, token: string, env: NodeJS.ProcessEnv = process.env, ): boolean { - const expected = createReviewAgentAttestationToken(agentId, runId, env); - if (!expected) return false; - const expectedBytes = Buffer.from(expected); - const actualBytes = Buffer.from(token); - return expectedBytes.length === actualBytes.length && timingSafeEqual(expectedBytes, actualBytes); + return verifyReviewAttestationSignature(reviewAgentTokenPayload(agentId, runId), token, env); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/review-attestation-key.ts` around lines 57 - 68, Update verifyReviewAgentAttestationToken to delegate token comparison to verifyReviewAttestationSignature after generating the expected token, preserving the existing false result when token creation fails. Remove the duplicated length check and timingSafeEqual logic so verifyReviewAttestationSignature remains the single authoritative comparison implementation.src/dashboard/server/main.ts (1)
117-119: 🩺 Stability & Availability | 🔵 TrivialNote the restart behavior of the in-memory key.
ensureReviewAttestationKeygenerates a new key on every dashboard start unless the environment already supplies one. After a restart, every attestation signed by the previous process fails verification, so in-flight review artifacts stop being recovery authority. The 30 minute freshness window limits the exposure, but a restart during an active review cycle discards otherwise valid evidence silently.Consider logging at startup when a key is generated rather than inherited, so an operator can correlate a restart with lost artifact authority.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/server/main.ts` around lines 117 - 119, Update the startup flow around ensureReviewAttestationKey to log when the signing key is generated locally rather than inherited from the environment. Preserve the existing key selection behavior, and emit the startup message only for newly generated in-memory keys so operators can correlate restarts with invalidated prior artifacts.src/lib/cloister/__tests__/verdict-restore.test.ts (1)
370-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the duplicated
writeArtifactwrapper into the shared helper.The same wrapper body exists in
src/lib/cloister/__tests__/verdict-restore-breaker.test.ts,src/lib/cloister/__tests__/synthesis-artifact-verdict.test.ts, andsrc/lib/cloister/__tests__/feedback-target.test.ts. Export a factory fromreview-artifact-test-helpers.tsthat bindsworkspacePath,issueId, andrunIdonce, then call it from each suite.♻️ Proposed helper addition
// src/lib/cloister/__tests__/review-artifact-test-helpers.ts export function attestedArtifactWriter(binding: { workspacePath: () => string; issueId: string; runId: string; }) { return (filename: VerdictReportFilename, body: string, headSha?: string): string => writeAttestedReviewArtifact({ workspacePath: binding.workspacePath(), issueId: binding.issueId, runId: binding.runId, filename, body, ...(headSha ? { headSha } : {}), }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cloister/__tests__/verdict-restore.test.ts` around lines 370 - 379, Move the duplicated writeArtifact wrapper into review-artifact-test-helpers.ts by exporting an attestedArtifactWriter factory that binds workspacePath, issueId, and runId and preserves optional headSha handling and the writer’s return value. Update writeArtifact usage in verdict-restore.test.ts, verdict-restore-breaker.test.ts, synthesis-artifact-verdict.test.ts, and feedback-target.test.ts to use the shared factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/specialists/done.ts`:
- Around line 99-101: Validate attested.reviewedHead against the expected
HeadAnchor shape before passing it to rehydrateHeadAnchor in the done command.
Reject malformed values, not merely missing or empty strings, and only assign
the validated anchor to attestedEvidenceHead so reviewedAtCommit and subsequent
comparisons cannot receive an unchecked value.
- Around line 84-98: The attestation fetch in
src/cli/commands/specialists/done.ts lines 84-98 needs a wall-clock timeout: add
an AbortSignal.timeout(...) signal to the request options. Update the exact
request assertion in tests/cli/commands/specialists/done.test.ts lines 160-171
to include the signal property, or use expect.objectContaining while preserving
the existing assertions.
In `@src/lib/cloister/review-agent.ts`:
- Around line 591-596: The context manifest failure catch block currently clears
the path and continues dispatching agents. In the review-agent flow around
attestReviewContextManifest and the existing outer failure handler, rethrow or
otherwise propagate ctxErr after logging so launch aborts before agent spawning;
preserve successful manifest handling and add a regression test covering
attestation failure.
- Around line 578-579: Update the run-ID initialization around runNonce/runId
and the later reviewRunId assignment to capture the previously persisted ID
before mutation, then detect and reuse it when recovering an active review cycle
instead of generating a new nonce-based ID. Preserve new nonce generation for
genuine re-reviews so the full-review comparison can select
spawnConvoyReviewers() only for a new cycle, and add coverage for both recovery
and re-review paths.
In `@src/lib/cloister/review-artifact-attestation.ts`:
- Around line 99-101: Update the SHA validation in the entry-processing logic
and the single-repository headSha validation to accept only exactly 40 or 64
hexadecimal characters, rejecting lengths from 41 through 63 while preserving
the existing canonical anchor behavior.
In `@tests/unit/dashboard/server/routes/specialists-review-attestation.test.ts`:
- Around line 65-78: Update the test setup around beforeEach and afterEach to
use vi.stubEnv for REVIEW_ATTESTATION_KEY_ENV instead of directly mutating
process.env, and call vi.unstubAllEnvs() during teardown so any pre-existing
OVERDECK_REVIEW_ATTESTATION_KEY value is restored.
---
Nitpick comments:
In `@src/cli/commands/specialists/done.ts`:
- Around line 176-177: Wrap the try/catch guarded by if (!workspaceHead) in
braces in the workspaceHead initialization flow, preserving the existing
try/catch behavior and all statements within its guarded region.
In `@src/dashboard/server/main.ts`:
- Around line 117-119: Update the startup flow around ensureReviewAttestationKey
to log when the signing key is generated locally rather than inherited from the
environment. Preserve the existing key selection behavior, and emit the startup
message only for newly generated in-memory keys so operators can correlate
restarts with invalidated prior artifacts.
In `@src/lib/cloister/__tests__/feedback-target.test.ts`:
- Around line 261-272: Extend the test case “preserves a test-feedback delivery
gate after review already passed” to assert that
reviewStatus.setReviewStatusSync is not called, while retaining the existing
markWorkspaceStuck expectation.
In `@src/lib/cloister/__tests__/review-artifact-attestation.test.ts`:
- Around line 21-28: Update the setup and teardown around
installTestReviewAttestationKey in the review attestation test suite to restore
OVERDECK_REVIEW_ATTESTATION_KEY after each test. Prefer vi.stubEnv with
vi.unstubAllEnvs, or explicitly delete the environment variable in afterEach
while preserving the existing temporary workspace cleanup.
In `@src/lib/cloister/__tests__/review-artifact-test-helpers.ts`:
- Around line 32-37: Update the fixture construction in the writeFileSync call
so headSha is included only when options.repos is absent, while preserving the
existing default TEST_REVIEW_HEAD for monorepo fixtures and the repos field for
polyrepo fixtures.
In `@src/lib/cloister/__tests__/verdict-restore.test.ts`:
- Around line 370-379: Move the duplicated writeArtifact wrapper into
review-artifact-test-helpers.ts by exporting an attestedArtifactWriter factory
that binds workspacePath, issueId, and runId and preserves optional headSha
handling and the writer’s return value. Update writeArtifact usage in
verdict-restore.test.ts, verdict-restore-breaker.test.ts,
synthesis-artifact-verdict.test.ts, and feedback-target.test.ts to use the
shared factory.
In `@src/lib/cloister/review-artifact-attestation.ts`:
- Around line 164-186: Update the catch block in the candidate-report loop to
log a debug-level message containing the skipped filename and caught error
before continuing to the next report. Preserve the existing fallback behavior
and final no-report error in the surrounding review report selection flow.
In `@src/lib/cloister/synthesis-verdict.ts`:
- Around line 88-91: Update the catch handling around resolveTrustedReviewRun to
capture the thrown error and emit a warning containing the issue id and error
details before returning null. Preserve the existing null return behavior and do
not alter successful resolution.
- Around line 16-24: Rename SynthesisArtifactVerdict.mtimeMs to attestedAtMs and
update all references, including readAttestedReviewReports, to reflect that the
value is Date.parse(valid.issuedAt) from the attestation rather than the
artifact file mtime.
In `@src/lib/review-attestation-key.ts`:
- Around line 57-68: Update verifyReviewAgentAttestationToken to delegate token
comparison to verifyReviewAttestationSignature after generating the expected
token, preserving the existing false result when token creation fails. Remove
the duplicated length check and timingSafeEqual logic so
verifyReviewAttestationSignature remains the single authoritative comparison
implementation.
In `@tests/cli/commands/specialists/done.test.ts`:
- Around line 137-177: Add two negative tests alongside the existing attestation
tests for doneCommand: one with a callerAgentId that does not match the expected
agent-<issue>-review identity, and one where the attestation fetch returns a
non-OK response. Assert each rejects and verify mockSetReviewStatus is not
called in both cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4deb48a9-e660-47a2-b47e-7207efb6f8db
📒 Files selected for processing (35)
docs/REVIEW-AGENT-ARCHITECTURE.mdsrc/cli/commands/specialists/done.tssrc/dashboard/server/main.tssrc/dashboard/server/routes/specialists/legacy-routes.tssrc/lib/__tests__/agent-state-role.test.tssrc/lib/__tests__/review-status-read-artifact.test.tssrc/lib/agents/resume.tssrc/lib/agents/spawn-prep.tssrc/lib/agents/spawn.tssrc/lib/child-env.tssrc/lib/cloister/__tests__/deacon-stash-janitor.test.tssrc/lib/cloister/__tests__/feedback-target.test.tssrc/lib/cloister/__tests__/review-agent.test.tssrc/lib/cloister/__tests__/review-artifact-attestation.test.tssrc/lib/cloister/__tests__/review-artifact-test-helpers.tssrc/lib/cloister/__tests__/synthesis-artifact-verdict.test.tssrc/lib/cloister/__tests__/verdict-restore-breaker.test.tssrc/lib/cloister/__tests__/verdict-restore.test.tssrc/lib/cloister/deacon-review-signals.tssrc/lib/cloister/feedback-target.tssrc/lib/cloister/review-agent.tssrc/lib/cloister/review-artifact-attestation.tssrc/lib/cloister/synthesis-verdict.tssrc/lib/cloister/verdict-restore.tssrc/lib/overdeck/agent-review-provenance.tssrc/lib/review-attestation-key.tstests/cli/commands/specialists/done.test.tstests/integration/post-review-rebase-scenario.test.tstests/lib/child-env.test.tstests/lib/cloister/review-agent.test.tstests/unit/dashboard/deacon-supervisor.test.tstests/unit/dashboard/server/routes/specialists-review-attestation.test.tstests/unit/lib/cloister/guard-advancing-dispatch.test.tstests/unit/lib/head-anchor-write-sites.test.tstests/unit/lib/overdeck/agent-discovery-columns.test.ts
💤 Files with no reviewable changes (2)
- tests/unit/lib/overdeck/agent-discovery-columns.test.ts
- src/lib/cloister/verdict-restore.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/cloister/tests/verdict-restore-breaker.test.ts
- tests/unit/lib/head-anchor-write-sites.test.ts
- docs/REVIEW-AGENT-ARCHITECTURE.md
- src/lib/tests/review-status-read-artifact.test.ts
- src/lib/cloister/tests/synthesis-artifact-verdict.test.ts
| attestReviewContextManifest(contextManifestPath); | ||
| tier1Summary = formatTier1Summary(manifest); | ||
| console.log(`[review-agent] Context manifest built: ${contextManifestPath} (${manifest.changedFiles.length} files)`); | ||
| console.log(`[review-agent] Context manifest built and attested: ${contextManifestPath} (${manifest.changedFiles.length} files)`); | ||
| } catch (ctxErr) { | ||
| console.warn(`[review-agent] Context manifest build failed for ${opts.issueId} — reviewers will block on missing shared context:`, ctxErr); | ||
| contextManifestPath = undefined; | ||
| console.warn(`[review-agent] Context manifest build or attestation failed for ${opts.issueId} — reviewers will block on missing trusted context:`, ctxErr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort dispatch if context attestation fails.
Lines 591-596 clear the error and continue to launch a review. attestReviewReport() rejects a run without a valid signed context manifest, so the Deacon fallback cannot persist trusted terminal evidence for this launch. Propagate the error to the existing outer failure handler before spawning agents. Add a regression test for manifest-attestation failure.
As per coding guidelines, fix broken behavior at its root cause; never add workarounds, hacks, fallback chains, or downstream defensive handling that merely masks symptoms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/cloister/review-agent.ts` around lines 591 - 596, The context
manifest failure catch block currently clears the path and continues dispatching
agents. In the review-agent flow around attestReviewContextManifest and the
existing outer failure handler, rethrow or otherwise propagate ctxErr after
logging so launch aborts before agent spawning; preserve successful manifest
handling and add a regression test covering attestation failure.
Source: Coding guidelines
| if (typeof repoKey !== 'string' || !repoKey || /[\s@]/.test(repoKey)) return undefined; | ||
| if (typeof headSha !== 'string' || !/^[0-9a-f]{40,64}$/i.test(headSha)) return undefined; | ||
| entries.push(`${repoKey}@${headSha}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the SHA validation to real git lengths.
/^[0-9a-f]{40,64}$/i accepts 41 to 63 character values, which are not valid git object IDs. Accept only 40 or 64 characters. This keeps the canonical anchor free of malformed values.
🛡️ Proposed fix
- if (typeof headSha !== 'string' || !/^[0-9a-f]{40,64}$/i.test(headSha)) return undefined;
+ if (typeof headSha !== 'string' || !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headSha)) return undefined;Apply the same pattern at Line 105 for the single-repository headSha.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/cloister/review-artifact-attestation.ts` around lines 99 - 101,
Update the SHA validation in the entry-processing logic and the
single-repository headSha validation to accept only exactly 40 or 64 hexadecimal
characters, rejecting lengths from 41 through 63 while preserving the existing
canonical anchor behavior.
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| process.env[REVIEW_ATTESTATION_KEY_ENV] = 'route-test-review-attestation-key-material-123456789'; | ||
| mocks.getProvenance.mockReturnValue({ workspace: '/workspace', reviewRunId: RUN_ID }); | ||
| mocks.attestReport.mockReturnValue({ | ||
| filename: 'synthesis.md', | ||
| verdict: 'passed', | ||
| reviewedHead: 'a'.repeat(40), | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| delete process.env[REVIEW_ATTESTATION_KEY_ENV]; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
fd -a 'specialists-review-attestation.test.ts' . || true
echo "== File excerpt =="
file="$(fd 'specialists-review-attestation.test.ts' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,130p' "$file" | cat -n
fi
echo "== Search REEVIEW_ATTESTATION_KEY_ENV usages =="
rg -n "REVIEW_ATTESTATION_KEY_ENV|review-attestation|reviewAttestation|attestation" -S . \
-g '!node_modules' -g '!dist' -g '!build' | head -n 120
echo "== Search vi.stubEnv/unstubAllEnvs usage =="
rg -n "vi\.stubEnv|vi\.unstubAllEnvs" . -g '!node_modules' -g '!dist' -g '!build' | head -n 80Repository: eltmon/overdeck
Length of output: 23950
🌐 Web query:
Vitest vi.stubEnv vi.unstubAllEnvs documentation
💡 Result:
In Vitest, vi.stubEnv and vi.unstubAllEnvs are utilities used to modify and restore environment variables during testing [1][2]. vi.stubEnv(name, value) This function changes the value of an environment variable on both process.env and import.meta.env [1][3]. - Parameters: Takes the name of the environment variable (string) and the value to set (string, boolean, or undefined) [1][2]. - Note: If you modify environment variables by direct assignment (e.g., import.meta.env.MODE = 'test'), Vitest will not be able to use vi.unstubAllEnvs to restore the original value [1][2]. vi.unstubAllEnvs This function restores all environment variables changed via vi.stubEnv to their original values that existed before the first stub was created [1][3]. - Usage: It is typically used manually in hooks like beforeEach or afterEach [4][5]. Configuration Option: unstubEnvs You can enable automatic restoration of environment variables by setting the unstubEnvs configuration option to true in your vitest.config.ts file [4][6]. When enabled, Vitest automatically calls vi.unstubAllEnvs before each test [6]. - Caution: This setting may cause issues with concurrent tests, as the completion of one test will restore environment variables that might still be in use by other concurrent tests [6]. Example of configuration: // vitest.config.ts import { defineConfig } from 'vitest/config' export default defineConfig({ test: { unstubEnvs: true, }, })
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/vi.md
- 4: https://vitest.dev/guide/mocking
- 5: https://github.com/vitest-dev/vitest/blob/main/docs/guide/mocking.md
- 6: https://tangled.org/vitest.dev/vitest/blob/b7902bcb409882a445aa5f9a8675cb1a71696ad2/docs/config/unstubenvs.md
Restore the original attestation-key value with vi.stubEnv.
Direct mutation and delete process.env[...] do not restore a pre-existing OVERDECK_REVIEW_ATTESTATION_KEY. Use vi.stubEnv(REVIEW_ATTESTATION_KEY_ENV, ...) in beforeEach and vi.unstubAllEnvs() in afterEach.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/dashboard/server/routes/specialists-review-attestation.test.ts`
around lines 65 - 78, Update the test setup around beforeEach and afterEach to
use vi.stubEnv for REVIEW_ATTESTATION_KEY_ENV instead of directly mutating
process.env, and call vi.unstubAllEnvs() during teardown so any pre-existing
OVERDECK_REVIEW_ATTESTATION_KEY value is restored.
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-04T22:51:46ZVerdict: CHANGES REQUESTED — all four convoy reviewers timed out without producing reportsContext
Convoy Status
Blocking Findings[security] Reviewer timed out without a usable reportThe security reviewer exceeded its deadline and exhausted retries. No report exists for this run, so the security dimension is unreviewed and the change cannot be approved. [correctness] Reviewer timed out without a usable reportThe correctness reviewer exceeded its deadline and exhausted retries. No report exists for this run, so the correctness dimension is unreviewed and the change cannot be approved. [performance] Reviewer timed out without a usable reportThe performance reviewer exceeded its deadline and exhausted retries. No report exists for this run, so the performance dimension is unreviewed and the change cannot be approved. [requirements] Reviewer timed out without a usable reportThe requirements reviewer exceeded its deadline and exhausted retries. No report exists for this run, so the acceptance-criteria dimension is unreviewed and the change cannot be approved. Non-blocking FindingsNone. Clean Sub-rolesNone; all four sub-roles timed out before producing usable reports. Source: /home/eltmon/Projects/overdeck/workspaces/feature-pan-3511/.pan/review/agent-pan-3511-review-a9b92991/synthesis.md Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-04T23:00:27ZVerdict: CHANGES REQUESTED — dashboard restarts invalidate active review attestation authorityContext
Convoy Status
Blocking Findings[correctness] Dashboard restarts discard the key needed to verify review evidence —
|
Co-Authored-By: Claude <noreply@anthropic.com>
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — the persisted attestation key remains readable by workspace-controlled processesContext
Convoy Status
Blocking Findings[security] Persisting the signing key at a same-UID path does not make it host-only —
|
# Conflicts: # src/lib/cloister/__tests__/stall-sweeper.test.ts # src/lib/cloister/stall-sweeper.ts
# Conflicts: # src/lib/cloister/stall-sweeper.ts
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — Workspace-writable artifacts can independently set a passed review verdictContext
Convoy Status
Blocking Findings[security] Workspace-controlled artifacts can bypass independent review —
|
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — The revision removes three required verdict-recovery behaviorsContext
Convoy Status
Blocking Findings[requirements] The shared restore helper and head-guard visibility path are absent —
|
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — recovery again promotes workspace-controlled evidence and reintroduces forbidden synchronous status-read I/OContext
Convoy Status
Blocking Findings[security] A workspace-controlled active-run artifact can forge a terminal approval —
|
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — the stale-journal guard rejects the default headless quick-review artifact shapeContext
Convoy Status
Blocking Findings[correctness] Headless quick-review artifacts cannot corroborate a terminal journal verdict —
|
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — headless workspace evidence can replay a stale approvalContext
Convoy Status
Blocking Findings[security] Headless workspace artifact can authorize stale verdict replay —
|
Review CHANGES REQUESTED for PAN-3511Review Synthesis — PAN-3511 — 2026-08-05Verdict: CHANGES REQUESTED — a workspace-provided head can still replay a stale approvalContext
Convoy Status
Blocking Findings[security] Workspace-controlled artifact head can still authorize stale verdict replay —
|
Co-Authored-By: Claude <noreply@anthropic.com>
Main independently greened itself against the PAN-3551 guard fallout (e752688): it baselined the old introspecting stall-sweeper test and added its own review_parent_stalled_needs_you park copy. This branch had already solved both differently (test rewritten to runtime behavior, copy entry added earlier), so the merge produced a duplicate STUCK_REASON_COPY key and a stale introspection-baseline entry. Keep the branch's versions: drop the duplicated copy entry and the stale baseline line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Issue: #3511
Acceptance Criteria
Summary by CodeRabbit
New Features
Bug Fixes