PAN-3206 - #3213
Conversation
![panopticon-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Prompt-Change: correct review artifact paths to the .pan runtime directory
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughReview verdict artifacts now use ChangesReview verdict recovery
Ratchet audit scope
Spawn provenance contract
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewAgent
participant ReviewFiles
participant DeaconPatrol
participant Workspace
participant ReviewSession
participant ReviewStatus
ReviewAgent->>ReviewFiles: write synthesis.md or review.md
DeaconPatrol->>ReviewFiles: discover and parse verdict
DeaconPatrol->>Workspace: compare reviewed HEAD
DeaconPatrol->>ReviewSession: send reconciliation nudge when live
DeaconPatrol->>ReviewStatus: apply verdict after checks
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (2)
tests/unit/scripts/lint-ratchet-audit.test.ts (1)
71-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake these tests prove the narrowed audit scope.
Because
writeFileSizeAllowlist()always adds# PAN-3116, both tests pass with or without file-size issue-reference enforcement. Allow the helper to create a row without an issue reference, then assert thatlint-ratchet-audit.shstill ignores it; keep malformed-row validation covered bylint-file-size.sh.
tests/unit/scripts/lint-ratchet-audit.test.ts#L71-L82: use a file-size row without an issue reference to verify range-mode exclusion.tests/unit/scripts/lint-ratchet-audit.test.ts#L140-L150: use the same distinction in last-commit mode.🤖 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/scripts/lint-ratchet-audit.test.ts` around lines 71 - 82, Update writeFileSizeAllowlist and the range-mode test at tests/unit/scripts/lint-ratchet-audit.test.ts:71-82 to create a file-size row without an issue reference, proving lint-ratchet-audit.sh ignores it; retain malformed-row validation in lint-file-size.sh. Apply the same no-reference distinction to the last-commit test at tests/unit/scripts/lint-ratchet-audit.test.ts:140-150, while preserving the existing audit pass assertions.src/lib/cloister/deacon-review-unsignaled.ts (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
reconcileUnappliedReviewVerdictslargely duplicatescheckCompletedButUnsignaledReviews.Both functions independently implement "find latest verdict report → wait for settle window → nudge live session once → auto-apply after grace/on dead session," differing only in the triggering
reviewStatusand a couple of side effects (blocked-feedback delivery, head-anchor validation). This ~130-line duplication means future fixes (e.g. the try/catch and quoting issues above) need to be applied twice.Consider factoring the shared "select latest report + settle + nudge-once-then-apply" logic into a common helper parameterized by the differing bits (trigger status, extra validations, post-apply side effects).
Also applies to: 163-290
🤖 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/deacon-review-unsignaled.ts` at line 117, Extract the shared “latest verdict report → settle window → nudge live session once → auto-apply after grace or dead session” flow from reconcileUnappliedReviewVerdicts and checkCompletedButUnsignaledReviews into a common helper. Parameterize the differing reviewStatus trigger, head-anchor validation, blocked-feedback delivery, and post-apply side effects, then have both callers use it while preserving existing behavior.
🤖 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/cloister/deacon-review-unsignaled.ts`:
- Around line 172-198: Update the reconciliation loop around
Object.entries(statuses) so each issue’s processing, including
resolveProjectFromIssueSync, findWorkspacePath, readdirSync, findVerdictReport,
and statSync, is isolated in its own try/catch. On an issue-specific failure,
record the error using the existing logging/error-handling convention and
continue processing subsequent issues instead of letting the outer catch abort
the loop.
- Around line 263-276: The reconciled verdict handling only delivers feedback
for parsed blocked verdicts, dropping failed verdicts. Update the conditional
around parsed.verdict and the deliverReviewVerdictFeedback call to handle both
blocked and failed outcomes, passing parsed.verdict as the verdict while
preserving the existing notes and error handling.
In `@src/lib/cloister/review-verdict-report.ts`:
- Around line 27-36: Update parseVerdictReport so the APPROVED/PASSED and FAILED
checks accept only verdict text followed by whitespace or the end of the line,
rejecting trailing punctuation or additional text such as “— blocker” and “:
draft”. Add tests covering these invalid headings and preserve parsing for exact
valid verdict headings.
---
Nitpick comments:
In `@src/lib/cloister/deacon-review-unsignaled.ts`:
- Line 117: Extract the shared “latest verdict report → settle window → nudge
live session once → auto-apply after grace or dead session” flow from
reconcileUnappliedReviewVerdicts and checkCompletedButUnsignaledReviews into a
common helper. Parameterize the differing reviewStatus trigger, head-anchor
validation, blocked-feedback delivery, and post-apply side effects, then have
both callers use it while preserving existing behavior.
In `@tests/unit/scripts/lint-ratchet-audit.test.ts`:
- Around line 71-82: Update writeFileSizeAllowlist and the range-mode test at
tests/unit/scripts/lint-ratchet-audit.test.ts:71-82 to create a file-size row
without an issue reference, proving lint-ratchet-audit.sh ignores it; retain
malformed-row validation in lint-file-size.sh. Apply the same no-reference
distinction to the last-commit test at
tests/unit/scripts/lint-ratchet-audit.test.ts:140-150, while preserving the
existing audit pass 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: 8446a9e9-02ac-4895-ae48-8234046bb417
📒 Files selected for processing (19)
docs/REVIEW-AGENT-ARCHITECTURE.mdroles/review.mdscripts/file-size-allowlist.txtscripts/lint-ratchet-audit.shsrc/lib/__tests__/spawn-provenance-contract.test.tssrc/lib/agents/agent-state-source.tssrc/lib/cloister/deacon-review-signals.tssrc/lib/cloister/deacon-review-unsignaled.tssrc/lib/cloister/deacon-review.tssrc/lib/cloister/deacon.tssrc/lib/cloister/feedback-target.tssrc/lib/cloister/review-verdict-feedback.tssrc/lib/cloister/review-verdict-report.tssrc/lib/overdeck/agent-state-sync.tstests/unit/lib/cloister/deacon-review-unapplied-verdicts.test.tstests/unit/lib/cloister/feedback-target.test.tstests/unit/lib/cloister/review-verdict-report.test.tstests/unit/lib/head-anchor-write-sites.test.tstests/unit/scripts/lint-ratchet-audit.test.ts
| if (parsed.verdict === 'blocked') { | ||
| try { | ||
| await Effect.runPromise(deliverReviewVerdictFeedback({ | ||
| issueId, | ||
| verdict: 'blocked', | ||
| notes: parsed.topBlocker || reviewNotes, | ||
| workspacePath: wsPath, | ||
| prUrl: status.prUrl, | ||
| runId: basename(latestDir), | ||
| })); | ||
| } catch (err: unknown) { | ||
| console.error(`[deacon] Failed to deliver reconciled review verdict for ${issueId}:`, err instanceof Error ? err.message : String(err)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the verdict vocabulary and deliverReviewVerdictFeedback's accepted verdict values
rg -n "verdict" src/lib/cloister/review-verdict-report.ts src/lib/cloister/review-verdict-feedback.ts -A3 -B3
rg -n "'blocked'|'failed'|'passed'|'approved'" src/lib/cloister/review-verdict-report.ts src/lib/cloister/review-verdict-feedback.tsRepository: eltmon/overdeck
Length of output: 15562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file outline =="
ast-grep outline src/lib/cloister/deacon-review-unsignaled.ts --view expanded || true
echo
echo "== relevant target sections =="
sed -n '200,290p' src/lib/cloister/deacon-review-unsignaled.ts | cat -n
echo
echo "== nudge command references and verdict handling =="
rg -n "parseVerdictReport|parsed\.verdict|--notes|deliverReviewVerdictFeedback|reviewedAtCommit|verdict" src/lib/cloister/deacon-review-unsignaled.ts tests src/lib/cloister -g '!*feedback*' -A5 -B5 || true
echo
echo "== test files mentioning deacon and failed/approved/passed =="
git ls-files 'tests/*' 'src/*' | rg 'deacon-review-unsignaled|deacon-review-unapplied-verdicts|unapplied-verdicts|unsignaled' || true
sed -n '1,260p' tests/unit/lib/cloister/deacon-review-unapplied-verdicts.test.ts | cat -nRepository: eltmon/overdeck
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file locations =="
git ls-files | rg 'deacon-review-unsignaled|deacon-review-unapplied-verdicts|review-verdict-feedback|review-verdict-report' || true
echo
echo "== deacon-review-unsignaled relevant section =="
sed -n '200,290p' src/lib/cloister/deacon-review-unsignaled.ts | cat -n
echo
echo "== parseVerdictReport and deliverReviewVerdictFeedback imports/type declarations =="
rg -n "import .*parseVerdictReport|parseVerdictReport|deliverReviewVerdictFeedback|type DeliverReviewVerdictFeedbackOptions|verdict: ReviewVerdict|Outcome:" src/lib/cloister/deacon-review-unsignaled.ts src/lib/cloister/review-verdict-feedback.ts src/lib/cloister/review-verdict-report.ts -A3 -B3 || true
echo
echo "== deacon-review-unapplied-verdicts targeted test file =="
file=$(git ls-files | grep 'deacon-review-unapplied-verdicts.test.ts' | head -n1)
echo "[[FILE]] $file"
sed -n '1,240p' "$file" | cat -n
echo
rg -n "blocked|failed|passed|FAILED|changes-requested|deliverReviewVerdictFeedback|reviewedAtCommit|verdict" "$file" -A3 -B3 || true
echo
echo "== focused deacon review tests mentioning failed/blocked verdict =="
rg -n "failed|blocked|deliverReviewVerdictFeedback|reviewedAtCommit|parseVerdictReport|FAILED" tests/unit/lib/cloister src/lib/cloister -g '*deacon*test*' -g '*unapplied*test*' -g '*review*' -A4 -B4 || true
echo
echo "== deterministic parser/label mapping probe =="
python3 - <<'PY'
from pathlib import Path
src = Path('src/lib/cloister/review-verdict-report.ts').read_text()
fb = Path('src/lib/cloister/review-verdict-feedback.ts').read_text()
for label in ['APPROVED','PASSED','FAILED','CHANGES REQUESTED','CHANGES REQUESTED — something']:
# Mirror reported parser patterns: APPROVED/PASSED, FAILED, changes-requested section with optional suffix
if label.strip().upper() in ('APPROVED','PASSED'):
verdict='passed', blocker=''
elif label.strip().upper() == 'FAILED':
verdict='failed', blocker=''
elif 'CHANGES REQUESTED' in label.strip().upper():
verdict='blocked', blocker=label.split('-',1)[1].strip(" —–:") if '-' in label else ''
else:
verdict, blocker = None, ''
feedback_label = 'CHANGES REQUESTED' if verdict == 'blocked' else 'FAILED'
feedback_outcome = 'changes-requested' if verdict == 'blocked' else 'failed'
print(f'{label:30} -> parser verdict={verdict!r:10} blocker={blocker!r:15} feedback label={feedback_label} outcome={feedback_outcome}')
PYRepository: eltmon/overdeck
Length of output: 50372
Deliver feedback for reconciled failed review verdicts.
parseVerdictReport() accepts FAILED as 'failed', and the nudge path includes notes for both blocked and failed, but the dead-session/unresponsiveness auto-complete path only calls deliverReviewVerdictFeedback(..., verdict: 'blocked', ...) for parsed.verdict === 'blocked'. As a result, FAILED verdicts discovered in this path update status but never write/re-surface feedback to the work agent. Deliver feedback using parsed.verdict for both blocked and failed verdicts.
🤖 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/deacon-review-unsignaled.ts` around lines 263 - 276, The
reconciled verdict handling only delivers feedback for parsed blocked verdicts,
dropping failed verdicts. Update the conditional around parsed.verdict and the
deliverReviewVerdictFeedback call to handle both blocked and failed outcomes,
passing parsed.verdict as the verdict while preserving the existing notes and
error handling.
| export function parseVerdictReport(content: string): ParsedVerdictReport | null { | ||
| const verdictLine = content.match(/^##\s*Verdict:\s*(.+)$/im); | ||
| if (!verdictLine) return null; | ||
|
|
||
| const verdictText = verdictLine[1]!.trim(); | ||
| if (/^(?:APPROVED|PASSED)\b/i.test(verdictText)) { | ||
| return { verdict: 'passed', topBlocker: '' }; | ||
| } | ||
| if (/^FAILED\b/i.test(verdictText)) { | ||
| return { verdict: 'failed', topBlocker: '' }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed on non-exact passed and failed headings.
APPROVED — blocker and FAILED: draft currently parse as valid because \b accepts trailing punctuation. That can persist an unintended recovered verdict despite violating the documented heading contract. Require only trailing whitespace and add rejection tests.
Proposed fix
- if (/^(?:APPROVED|PASSED)\b/i.test(verdictText)) {
+ if (/^(?:APPROVED|PASSED)\s*$/i.test(verdictText)) {
return { verdict: 'passed', topBlocker: '' };
}
- if (/^FAILED\b/i.test(verdictText)) {
+ if (/^FAILED\s*$/i.test(verdictText)) {
return { verdict: 'failed', topBlocker: '' };
}📝 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 parseVerdictReport(content: string): ParsedVerdictReport | null { | |
| const verdictLine = content.match(/^##\s*Verdict:\s*(.+)$/im); | |
| if (!verdictLine) return null; | |
| const verdictText = verdictLine[1]!.trim(); | |
| if (/^(?:APPROVED|PASSED)\b/i.test(verdictText)) { | |
| return { verdict: 'passed', topBlocker: '' }; | |
| } | |
| if (/^FAILED\b/i.test(verdictText)) { | |
| return { verdict: 'failed', topBlocker: '' }; | |
| export function parseVerdictReport(content: string): ParsedVerdictReport | null { | |
| const verdictLine = content.match(/^##\s*Verdict:\s*(.+)$/im); | |
| if (!verdictLine) return null; | |
| const verdictText = verdictLine[1]!.trim(); | |
| if (/^(?:APPROVED|PASSED)\s*$/i.test(verdictText)) { | |
| return { verdict: 'passed', topBlocker: '' }; | |
| } | |
| if (/^FAILED\s*$/i.test(verdictText)) { | |
| return { verdict: 'failed', topBlocker: '' }; |
🤖 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-verdict-report.ts` around lines 27 - 36, Update
parseVerdictReport so the APPROVED/PASSED and FAILED checks accept only verdict
text followed by whitespace or the end of the line, rejecting trailing
punctuation or additional text such as “— blocker” and “: draft”. Add tests
covering these invalid headings and preserve parsing for exact valid verdict
headings.
Review CHANGES REQUESTED for PAN-3206Recovered verdicts omit passed anchors, fail polyrepo HEAD matching, and can misroute or inject feedback commands Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Issue: #3206
Acceptance Criteria
Summary by CodeRabbit
New Features
Bug Fixes
Documentation