PAN-3377 - #3380
Conversation
![panopticon-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Issue: PAN-3377 Co-Authored-By: Claude <noreply@anthropic.com>
Issue: PAN-3377 Co-Authored-By: Claude <noreply@anthropic.com>
Issue: PAN-3377 Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds review-cycle and commit-aware stale-verdict protection across status updates, pipeline restoration, journal reconciliation, and workspace fallback handling. It also clears superseded infrastructure-failure state when a new review starts and centralizes note truncation. ChangesReview verdict freshness safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LiveReview
participant PipelineSnapshot
participant VerdictMerge
participant ReviewStatusStore
LiveReview->>VerdictMerge: provide current review cycle and HEAD
PipelineSnapshot->>VerdictMerge: provide snapshot cycle and HEAD
VerdictMerge->>VerdictMerge: detect stale terminal verdict
VerdictMerge->>ReviewStatusStore: preserve current status or apply valid verdict
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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: 2
🧹 Nitpick comments (4)
src/lib/review-status-read.ts (1)
32-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate stale-check-and-warn pattern.
This block (compute
staleVerdictSnapshotAgainstLiveCycle, log a warning with live/snapshot cycle and HEAD, then refuse) repeats near-verbatim indrainWorkspaceVerdictFallbackandprojectJournalStatusinsrc/lib/overdeck/review-status-record-sync.ts, and inrestoreOneIssueinsrc/lib/pan-dir/verdict-restore.ts. Extract a shared helper inpipeline-verdict-merge.tsthat performs the check and the warning, and call it from all four sites.🤖 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-status-read.ts` around lines 32 - 46, Extract the duplicated stale-verdict detection and warning logic into a shared helper in pipeline-verdict-merge.ts, including the staleVerdictSnapshotAgainstLiveCycle check, contextual warning, and refusal result. Replace the inline blocks in the current review-status read flow, drainWorkspaceVerdictFallback, projectJournalStatus, and restoreOneIssue with calls to that helper, preserving each caller’s existing return behavior.src/lib/pan-dir/pipeline-verdict-merge.ts (1)
215-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate cycle-comparison block.
The cycle-vs-write-time comparison (
journalCycle,fallbackCycle,fallbackWrittenMs, and the compound condition) is identical to the block infindFallbackVerdictConflictsat lines 258-266. Extract a shared helper, e.g.journalCycleSupersedesFallback(journal, fallback): boolean, and call it from both functions.♻️ Proposed extraction
+function journalCycleSupersedesFallback( + journal: PanIssuePipelineRecord, + fallback: { updatedAt: string; pipeline: PipelineFields }, +): boolean { + const journalCycle = reviewCycleMs(fields(journal)); + const fallbackCycle = reviewCycleMs(fallback.pipeline); + const fallbackWrittenMs = cycleMs(fallback.updatedAt); + return journalCycle !== undefined + && ((fallbackCycle !== undefined && journalCycle > fallbackCycle) + || (fallbackCycle === undefined && fallbackWrittenMs !== undefined && journalCycle > fallbackWrittenMs)); +} + export function pipelineCoversFallbackVerdicts( journal: PanIssuePipelineRecord, fallback: { updatedAt: string; pipeline: PipelineFields }, ): boolean { - const journalCycle = reviewCycleMs(fields(journal)); - const fallbackCycle = reviewCycleMs(fallback.pipeline); - const fallbackWrittenMs = cycleMs(fallback.updatedAt); - if ( - journalCycle !== undefined - && ((fallbackCycle !== undefined && journalCycle > fallbackCycle) - || (fallbackCycle === undefined && fallbackWrittenMs !== undefined && journalCycle > fallbackWrittenMs)) - ) { + if (journalCycleSupersedesFallback(journal, fallback)) { return true; }🤖 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/pan-dir/pipeline-verdict-merge.ts` around lines 215 - 222, Extract the duplicated cycle-versus-write-time comparison into a shared helper such as journalCycleSupersedesFallback(journal, fallback), including the journalCycle, fallbackCycle, fallbackWrittenMs calculations and compound condition. Replace the equivalent logic in both the current merge flow and findFallbackVerdictConflicts with calls to the helper, preserving the existing boolean behavior.src/lib/overdeck/review-status-record-sync.ts (1)
475-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate stale-check-and-warn pattern.
This block repeats in
projectJournalStatusbelow (lines 627-643), inresolveJournalReconciledReviewStatusSync(src/lib/review-status-read.tslines 32-46), and inrestoreOneIssue(src/lib/pan-dir/verdict-restore.tslines 106-120). Extract a shared "check and warn" helper next tostaleVerdictSnapshotAgainstLiveCycleinpipeline-verdict-merge.ts.🤖 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/review-status-record-sync.ts` around lines 475 - 490, Extract the repeated stale-snapshot detection, warning, and boolean-result behavior into a shared helper beside staleVerdictSnapshotAgainstLiveCycle in pipeline-verdict-merge.ts. Update projectJournalStatus and the corresponding flows in resolveJournalReconciledReviewStatusSync and restoreOneIssue to call the helper, preserving each caller’s existing cleanup and return behavior while removing duplicated logging logic.src/lib/pan-dir/verdict-restore.ts (1)
106-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate stale-check-and-warn pattern.
This mirrors the same compute-and-warn block flagged in
src/lib/review-status-read.ts(lines 32-46) andsrc/lib/overdeck/review-status-record-sync.ts(lines 475-490 and 627-643). A shared helper inpipeline-verdict-merge.tswould remove the repetition and keep the warning format from drifting across sites.🤖 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/pan-dir/verdict-restore.ts` around lines 106 - 120, The stale snapshot computation and warning in the verdict restore flow duplicates logic from review-status-read.ts and review-status-record-sync.ts. Extract or reuse a shared helper in pipeline-verdict-merge.ts that performs the stale-check and preserves the existing warning format, then update the relevant call sites including the block around staleVerdictSnapshotAgainstLiveCycle and remove their duplicated formatting logic.
🤖 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/pan-dir/pipeline-verdict-merge.ts`:
- Around line 95-135: Update liveCycleActive in
staleVerdictSnapshotAgainstLiveCycle to recognize any live review status with an
unconsumed request, not only 'pending': retain 'reviewing' as active and treat
terminal or other statuses as active when liveRequested is defined and is newer
than liveSpawned (or liveSpawned is undefined). Preserve the existing cycle and
terminal-snapshot checks.
In `@src/lib/review-verdict-guards.ts`:
- Around line 38-67: The reviewer evidence validation in
findVerdictEvidenceHeadMismatch currently checks only reviewerEvidenceHeads[0].
Update this function to compare every collected atCommit value against
status.lastVerifiedCommit, while retaining reviewedAtCommit as the primary
anchor when it is present and preserving the existing mismatch result shape and
terminal-review conditions.
---
Nitpick comments:
In `@src/lib/overdeck/review-status-record-sync.ts`:
- Around line 475-490: Extract the repeated stale-snapshot detection, warning,
and boolean-result behavior into a shared helper beside
staleVerdictSnapshotAgainstLiveCycle in pipeline-verdict-merge.ts. Update
projectJournalStatus and the corresponding flows in
resolveJournalReconciledReviewStatusSync and restoreOneIssue to call the helper,
preserving each caller’s existing cleanup and return behavior while removing
duplicated logging logic.
In `@src/lib/pan-dir/pipeline-verdict-merge.ts`:
- Around line 215-222: Extract the duplicated cycle-versus-write-time comparison
into a shared helper such as journalCycleSupersedesFallback(journal, fallback),
including the journalCycle, fallbackCycle, fallbackWrittenMs calculations and
compound condition. Replace the equivalent logic in both the current merge flow
and findFallbackVerdictConflicts with calls to the helper, preserving the
existing boolean behavior.
In `@src/lib/pan-dir/verdict-restore.ts`:
- Around line 106-120: The stale snapshot computation and warning in the verdict
restore flow duplicates logic from review-status-read.ts and
review-status-record-sync.ts. Extract or reuse a shared helper in
pipeline-verdict-merge.ts that performs the stale-check and preserves the
existing warning format, then update the relevant call sites including the block
around staleVerdictSnapshotAgainstLiveCycle and remove their duplicated
formatting logic.
In `@src/lib/review-status-read.ts`:
- Around line 32-46: Extract the duplicated stale-verdict detection and warning
logic into a shared helper in pipeline-verdict-merge.ts, including the
staleVerdictSnapshotAgainstLiveCycle check, contextual warning, and refusal
result. Replace the inline blocks in the current review-status read flow,
drainWorkspaceVerdictFallback, projectJournalStatus, and restoreOneIssue with
calls to that helper, preserving each caller’s existing return behavior.
🪄 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: 9b066eb7-2d64-47ee-a252-6272e7b29a74
📒 Files selected for processing (12)
src/lib/cloister/__tests__/review-agent.test.tssrc/lib/cloister/review-agent.tssrc/lib/overdeck/review-status-record-sync.tssrc/lib/pan-dir/pipeline-verdict-merge.tssrc/lib/pan-dir/verdict-restore.tssrc/lib/review-status-limits.tssrc/lib/review-status-read.tssrc/lib/review-status.tssrc/lib/review-verdict-guards.tstests/unit/lib/overdeck/review-status-record-sync-drain.test.tstests/unit/lib/pan-dir/verdict-restore.test.tstests/unit/lib/review-status.test.ts
| export function staleVerdictSnapshotAgainstLiveCycle( | ||
| live: PipelineFields, | ||
| snapshot: PipelineFields, | ||
| ): StaleVerdictSnapshot | null { | ||
| const liveReviewStatus = live.reviewStatus; | ||
| const liveSpawned = cycleMs(live.reviewSpawnedAt); | ||
| const liveRequested = cycleMs(live.reviewRequestedAt); | ||
| const liveCycle = reviewCycleMs(live); | ||
| const snapshotCycle = reviewCycleMs(snapshot); | ||
| const liveCycleActive = liveReviewStatus === 'reviewing' | ||
| || (liveReviewStatus === 'pending' | ||
| && liveRequested !== undefined | ||
| && (liveSpawned === undefined || liveRequested > liveSpawned)); | ||
|
|
||
| if ( | ||
| !liveCycleActive | ||
| || liveCycle === undefined | ||
| || snapshotCycle === undefined | ||
| || !carriesTerminalVerdict(snapshot) | ||
| || snapshotCycle >= liveCycle | ||
| ) return null; | ||
|
|
||
| return { | ||
| liveCycle, | ||
| snapshotCycle, | ||
| liveHead: typeof live.prHeadSha === 'string' | ||
| ? live.prHeadSha | ||
| : typeof live.lastVerifiedCommit === 'string' | ||
| ? live.lastVerifiedCommit | ||
| : typeof live.reviewedAtCommit === 'string' | ||
| ? live.reviewedAtCommit | ||
| : undefined, | ||
| snapshotHead: typeof snapshot.reviewedAtCommit === 'string' | ||
| ? snapshot.reviewedAtCommit | ||
| : typeof snapshot.lastVerifiedCommit === 'string' | ||
| ? snapshot.lastVerifiedCommit | ||
| : typeof snapshot.prHeadSha === 'string' | ||
| ? snapshot.prHeadSha | ||
| : undefined, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
liveCycleActive misses a terminal review status with an unconsumed newer request.
liveCycleActive treats only 'reviewing' or 'pending' (with reviewRequestedAt > reviewSpawnedAt) as an active cycle. setReviewStatusSync explicitly preserves reviewRequestedAt on a terminal transition when the request arrived after the spawn, to represent "an explicit re-review requested while the current review was running." That means a live row can carry a terminal reviewStatus ('blocked', 'failed', 'passed', 'skipped') together with a reviewRequestedAt newer than reviewSpawnedAt, and liveCycleActive does not recognize this as an active cycle.
In that state, staleVerdictSnapshotAgainstLiveCycle returns null even when reviewCycleMs(live) (driven by the newer reviewRequestedAt) exceeds the snapshot's cycle. An older terminal snapshot (from readJournalStatusSync, drainWorkspaceVerdictFallback, or restoreOneIssue) can then overlay stale reviewNotes/reviewedAtCommit fields onto a row that already recorded a newer review obligation.
Broaden the check to any status with an unconsumed request, not only 'pending'.
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" — fixing this in the shared function protects all three call sites at once.
🐛 Proposed fix for `liveCycleActive`
const liveCycleActive = liveReviewStatus === 'reviewing'
- || (liveReviewStatus === 'pending'
- && liveRequested !== undefined
+ || (liveRequested !== undefined
&& (liveSpawned === undefined || liveRequested > liveSpawned));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function staleVerdictSnapshotAgainstLiveCycle( | |
| live: PipelineFields, | |
| snapshot: PipelineFields, | |
| ): StaleVerdictSnapshot | null { | |
| const liveReviewStatus = live.reviewStatus; | |
| const liveSpawned = cycleMs(live.reviewSpawnedAt); | |
| const liveRequested = cycleMs(live.reviewRequestedAt); | |
| const liveCycle = reviewCycleMs(live); | |
| const snapshotCycle = reviewCycleMs(snapshot); | |
| const liveCycleActive = liveReviewStatus === 'reviewing' | |
| || (liveReviewStatus === 'pending' | |
| && liveRequested !== undefined | |
| && (liveSpawned === undefined || liveRequested > liveSpawned)); | |
| if ( | |
| !liveCycleActive | |
| || liveCycle === undefined | |
| || snapshotCycle === undefined | |
| || !carriesTerminalVerdict(snapshot) | |
| || snapshotCycle >= liveCycle | |
| ) return null; | |
| return { | |
| liveCycle, | |
| snapshotCycle, | |
| liveHead: typeof live.prHeadSha === 'string' | |
| ? live.prHeadSha | |
| : typeof live.lastVerifiedCommit === 'string' | |
| ? live.lastVerifiedCommit | |
| : typeof live.reviewedAtCommit === 'string' | |
| ? live.reviewedAtCommit | |
| : undefined, | |
| snapshotHead: typeof snapshot.reviewedAtCommit === 'string' | |
| ? snapshot.reviewedAtCommit | |
| : typeof snapshot.lastVerifiedCommit === 'string' | |
| ? snapshot.lastVerifiedCommit | |
| : typeof snapshot.prHeadSha === 'string' | |
| ? snapshot.prHeadSha | |
| : undefined, | |
| }; | |
| } | |
| export function staleVerdictSnapshotAgainstLiveCycle( | |
| live: PipelineFields, | |
| snapshot: PipelineFields, | |
| ): StaleVerdictSnapshot | null { | |
| const liveReviewStatus = live.reviewStatus; | |
| const liveSpawned = cycleMs(live.reviewSpawnedAt); | |
| const liveRequested = cycleMs(live.reviewRequestedAt); | |
| const liveCycle = reviewCycleMs(live); | |
| const snapshotCycle = reviewCycleMs(snapshot); | |
| const liveCycleActive = liveReviewStatus === 'reviewing' | |
| || (liveRequested !== undefined | |
| && (liveSpawned === undefined || liveRequested > liveSpawned)); | |
| if ( | |
| !liveCycleActive | |
| || liveCycle === undefined | |
| || snapshotCycle === undefined | |
| || !carriesTerminalVerdict(snapshot) | |
| || snapshotCycle >= liveCycle | |
| ) return null; | |
| return { | |
| liveCycle, | |
| snapshotCycle, | |
| liveHead: typeof live.prHeadSha === 'string' | |
| ? live.prHeadSha | |
| : typeof live.lastVerifiedCommit === 'string' | |
| ? live.lastVerifiedCommit | |
| : typeof live.reviewedAtCommit === 'string' | |
| ? live.reviewedAtCommit | |
| : undefined, | |
| snapshotHead: typeof snapshot.reviewedAtCommit === 'string' | |
| ? snapshot.reviewedAtCommit | |
| : typeof snapshot.lastVerifiedCommit === 'string' | |
| ? snapshot.lastVerifiedCommit | |
| : typeof snapshot.prHeadSha === 'string' | |
| ? snapshot.prHeadSha | |
| : undefined, | |
| }; | |
| } |
🤖 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/pan-dir/pipeline-verdict-merge.ts` around lines 95 - 135, Update
liveCycleActive in staleVerdictSnapshotAgainstLiveCycle to recognize any live
review status with an unconsumed request, not only 'pending': retain 'reviewing'
as active and treat terminal or other statuses as active when liveRequested is
defined and is newer than liveSpawned (or liveSpawned is undefined). Preserve
the existing cycle and terminal-snapshot checks.
Source: Coding guidelines
| function findVerdictEvidenceHeadMismatch( | ||
| status: ReviewGuardStatus, | ||
| update: ReviewGuardUpdate, | ||
| ): VerdictEvidenceHeadMismatch | null { | ||
| const terminalReview = update.reviewStatus !== undefined | ||
| && ['passed', 'blocked', 'failed', 'skipped'].includes(update.reviewStatus); | ||
| const reviewerEvidenceHeads = Object.values(update.reviewerVerdicts ?? {}) | ||
| .flatMap((verdict) => verdict?.atCommit ? [verdict.atCommit] : []); | ||
| const reviewEvidenceHead = update.reviewedAtCommit ?? reviewerEvidenceHeads[0]; | ||
| if ( | ||
| terminalReview | ||
| && reviewEvidenceHead | ||
| && status.lastVerifiedCommit | ||
| && reviewEvidenceHead !== status.lastVerifiedCommit | ||
| ) { | ||
| return { gate: 'review', evidenceHead: reviewEvidenceHead, targetHead: status.lastVerifiedCommit }; | ||
| } | ||
|
|
||
| const terminalTest = update.testStatus !== undefined | ||
| && ['passed', 'failed'].includes(update.testStatus); | ||
| if ( | ||
| terminalTest | ||
| && update.lastVerifiedCommit | ||
| && status.reviewedAtCommit | ||
| && update.lastVerifiedCommit !== status.reviewedAtCommit | ||
| ) { | ||
| return { gate: 'test', evidenceHead: update.lastVerifiedCommit, targetHead: status.reviewedAtCommit }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "reviewerVerdicts" src -g '*.ts' -C3Repository: eltmon/overdeck
Length of output: 16421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== review-verdict-guards outline =="
ast-grep outline src/lib/review-verdict-guards.ts --view expanded || true
echo "== review-verdict-guards relevant section =="
sed -n '1,140p' src/lib/review-verdict-guards.ts
echo "== review-status reconcile relevant section =="
sed -n '220,270p' src/lib/review-status.ts
sed -n '433,480p' src/lib/__tests__/review-status-reconcile.test.ts
echo "== tests mentioning findVerdictEvidenceHeadMismatch / lastVerifiedCommit =="
rg -n "findVerdictEvidenceHeadMismatch|lastVerifiedCommit|reviewerVerdicts" src/lib src/cli src/lib/__tests__ -g '*.ts' -C 2Repository: eltmon/overdeck
Length of output: 43823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find call sites of rejectVerdictEvidenceHeadMismatch =="
rg -n "rejectVerdictEvidenceHeadMismatch" src -g '*.ts' -C 4
echo "== workspace-anchor-drift guard context =="
sed -n '1,220p' src/lib/workspace-anchor-drift.tsRepository: eltmon/overdeck
Length of output: 7855
Check every reviewer evidence head, not only the first one.
Partial reviewerVerdicts updates merge with carried-forward entries, and partial anchor updates can omit reviewedAtCommit. When reviewerEvidenceHeads has multiple entries, reviewEvidenceHead only uses [0], so stale sub-reviewer verdict anchors beyond index 0 can pass this gate undetected. Compare the full set of atCommit values against status.lastVerifiedCommit.
🤖 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-verdict-guards.ts` around lines 38 - 67, The reviewer evidence
validation in findVerdictEvidenceHeadMismatch currently checks only
reviewerEvidenceHeads[0]. Update this function to compare every collected
atCommit value against status.lastVerifiedCommit, while retaining
reviewedAtCommit as the primary anchor when it is present and preserving the
existing mismatch result shape and terminal-review conditions.
1 participant
Issue: #3377
Summary by CodeRabbit
Bug Fixes
Tests