Repository navigation
feat: implement issue #402 — [Fleet Monitor] petry-projects/.github-private — pr-review.yml - #486
Conversation
…rivate — pr-review.yml
|
Warning Review limit reached
More reviews will be available in 24 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the single-review.md prompt to instruct the model to write its final verdict JSON to $OUTPUT_FILE using jq instead of printing to stdout, preventing output loss. It also adds corresponding regression tests in test_cli_error_routing.bats. The review feedback suggests using jq for skip verdicts to avoid escaping issues, and dynamically computing the escalate_to_ai flag based on environment variables and risk level rather than hardcoding it to false.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c37825403c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — rate-limited (intent: review-changes)PR: #486 |
|
Note @don-petry I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. |
Dev-Lead — rate-limited (intent: fix-bot-comment)PR: #486 |
|
Note @don-petry I received your request but all AI engines are currently rate-limited. Please re-mention |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8811082e27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a702ee422
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 651d1a1cec47ace8637e5cec1ebf25111979b10d
Review mode: triage-approved (single reviewer)
Summary
Confirmation pass over a triage-approved PR. The change is a focused bug fix for the single-reviewer pipeline: prompts/single-review.md is rewritten so the model writes its verdict JSON to $OUTPUT_FILE via jq (the same pattern already used by cascade-action.md, deep-review.md, rubber-duck.md, and triage.md), and scripts/engine.sh stops teeing Copilot stdout into $OUTPUT_FILE in the run_agentic and run_duck copilot branches — that tee was overwriting the JSON the model now writes directly. Tests are added to tests/test_cli_error_routing.bats. All 25 CI checks are green.
Linked issue analysis
Closes #402 — a Fleet Monitor degradation alert for pr-review.yml (25.2% failure rate). The PR addresses a plausible root cause: when single-review runs under the Copilot CLI in JSON mode, model Bash echoes do not surface in the result field, so a prompt that says "Output ONLY valid JSON to stdout" produced empty verdicts; extract_verdict_json would then fall through and emit no decision. Rewriting the prompt to write to $OUTPUT_FILE (Style 1 in extract_verdict_json) and removing the corrupting tee in engine.sh is the right fix and aligns single-review with the rest of the prompt suite.
Findings
Core fix — looks good:
prompts/single-review.md: new Output section matches the well-tested pattern fromprompts/cascade-action.md(lines 85–105): build body in a temp file, escape viajq -n --arg body, verify with stderr-only echoes so Copilot'steecannot corrupt$OUTPUT_FILE. The dynamically-computedescalate_to_aiis a nice improvement over the prior hard-codedfalse. The skip-verdict paths also now go throughjq, avoiding manual escaping pitfalls.scripts/engine.sh: removing| tee "$OUTPUT_FILE"from the copilot branches ofrun_agentic(deep-review path) andrun_duck(rubber-duck path) is consistent — bothprompts/deep-review.mdandprompts/rubber-duck.mdalready instruct the model to write verdict JSON to$OUTPUT_FILEdirectly, so the tee was at best redundant and at worst (under--output-format json) destructive._tok_tmpis still preserved for token accounting. The unrelatedtee "$_tmp"inrun_action(line ~899) is untouched, as it should be.extract_verdict_json(engine.sh:661): unchanged — Style 1 (dest already valid) is correctly preferred, which is exactly the new path the prompt drives.
Non-blocking observations on the new tests (worth a follow-up, not a block on this PR):
- The added regression tests live in
tests/test_cli_error_routing.bats, but thebatsjob in.github/workflows/lint.ymlinvokes a hard-coded list —bats tests/fleet_report.bats tests/token_report.bats tests/model_pricing.bats tests/test_review_cycle.bats tests/test_ci_status.bats— that does not includetest_cli_error_routing.bats. The new regression tests therefore do not run in CI and will not catch a future regression of either the prompt or the engine tee. Addingtests/test_cli_error_routing.batsto that list (or switching the job tobats tests/) would close the loop the tests are intended to protect. - The test
engine.sh: run_agentic copilot branch does not tee stdout to OUTPUT_FILErunsgrep -n 'tee.*OUTPUT_FILE\|OUTPUT_FILE.*tee' "$engine_file"and asserts the output is empty. The replacement comment now inscripts/engine.sh(lines 629 and 737) reads# Do NOT tee stdout to OUTPUT_FILE: ...— which matchestee.*OUTPUT_FILE. If/when this test is wired into CI, it will fail until the regex is tightened to ignore comment lines (e.g.,grep -n '^[[:space:]]*[^#].*tee.*OUTPUT_FILE') or scoped to the copilot branch only.
Neither observation reverses the core fix; both are about strengthening the safety net the PR adds.
CI status
All 25 status checks are SUCCESS (or SKIPPED for non-applicable dependency-audit ecosystems): AgentShield, Agent Security Scan, Lint, ShellCheck, shellcheck, CodeQL (actions+python), Secret scan (gitleaks), SonarCloud, CodeRabbit, validate-agent-profiles, gh-aw-compile, Compile agentic workflows, unit-tests, bats, review, and dependency-audit gates. Mergeable: MERGEABLE; merge state BLOCKED only because the requested org-leads review is outstanding.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: b9f01b3a35d185f7860c7c054bfb15a504487f17
Review mode: triage-approved (single reviewer)
Summary
Confirmation review of a triage-approved PR. The change is a focused bug fix for the single-reviewer pipeline:
prompts/single-review.mdis rewritten so the model writes its verdict JSON to$OUTPUT_FILEviajq(matching the pattern already used bycascade-action.md,deep-review.md,rubber-duck.md, andtriage.md). Skip verdicts are also produced viajqto avoid escaping pitfalls, andescalate_to_aiis computed fromAI_DELEGATION_ENABLED/decision/risk instead of being hardcoded.scripts/engine.shno longer tees Copilot stdout into$OUTPUT_FILEin eitherrun_agenticorrun_duck— with--output-format json, assistant text and tool transcripts would otherwise overwrite the model-authored JSON file. The token-usage tee path is preserved.tests/test_cli_error_routing.batsadds five regression tests that guard the prompt wording, the engine.sh tee removal, and the twoextract_verdict_jsonstyles.
Linked issue analysis
Issue #402 reports pr-review.yml as DEGRADED (25.2% failure rate). The reported failure mode — empty result field when the model echoed JSON only via Bash tool output, leading to extract_verdict_json finding no decision — is directly addressed by switching the prompt to write $OUTPUT_FILE and eliminating the stdout-tee that would clobber it.
Findings
No blocking issues.
- The change is well-scoped and aligns single-review with the existing convention used by the rest of the prompt suite.
- The new bats tests are good guardrails: they assert the prompt mentions
OUTPUT_FILE, no longer says "Output ONLY valid JSON to stdout", includes the "Do NOT print" guard, and thatengine.shno longer containstee.*OUTPUT_FILEin the relevant branches. - A prior cascade review (donpetry-bot) already approved at SHA
651d1a1c. Commits since then are onlyMerge branch 'main'plus two unrelated main-side feature merges (#473, #489) — no further changes to the three files this PR touches. - No security-sensitive surface (auth, secrets, migrations, CI security smells) is touched; SonarCloud, CodeQL, gitleaks, shellcheck, bats, and AgentShield are all green.
CI status
All required checks COMPLETED/SUCCESS (Lint, ShellCheck, shellcheck, bats, unit-tests, CodeQL, Analyze actions/python, SonarCloud, Secret scan (gitleaks), AgentShield, validate-agent-profiles, Compile agentic workflows, gh-aw-compile, Agent Security Scan, review). dependency-audit ecosystem jobs SKIPPED as expected for this change set. mergeStateStatus is BLOCKED only on outstanding required-review approvals.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 8798d07a6bbdf503fcbe8c057e1cf4879d6dc27e
Review mode: triage-approved (single reviewer)
Summary
Confirmation pass over a triage-cleared, low-risk PR that fixes the PR Review Agent itself: prompts/single-review.md is restructured so the model writes verdict JSON directly to $OUTPUT_FILE via jq (instead of relying on stdout, which was being lost when --output-format json is in effect), and scripts/engine.sh no longer tees Copilot stdout into $OUTPUT_FILE (which would corrupt the file the model just wrote). Five new bats regression tests lock in both halves of the fix. Triage assessment confirmed — no missed risk surface.
Linked issue analysis
Closes #402 — a Fleet Monitor health alert reporting a 25.2% failure rate (33/131 runs) on pr-review.yml. The PR targets the documented root cause: the prior prompt instructed the model to Output ONLY valid JSON to stdout, but under --output-format json the model's Bash-tool echoes do not surface in the result field, so extract_verdict_json saw empty bodies and reviews silently failed. Routing the verdict through $OUTPUT_FILE mirrors the working pattern already used by cascade-action.md. The change is well-scoped to the reported failure mode.
Findings
- No blocking issues. Diff is contained to the PR-review tooling (
prompts/single-review.md,scripts/engine.sh,tests/test_cli_error_routing.bats) — no auth, secrets, migrations, dependency, or CI-permission changes. - Engine change is symmetric across
run_agenticandrun_duckcopilot branches, both with explaining comments. Token-logteeis preserved. - Prompt now exposes
$OUTPUT_FILE, gives a worked example usingjq(proper escaping), dynamically derivesescalate_to_aifrom$AI_DELEGATION_ENABLED/$DECISION/$RISK, and pushes verification echoes to stderr so they cannot corrupt the verdict file — addressing earlier reviewer feedback. - Skip-path verdicts (
isDraft,head-sha-changed) were also migrated tojq > "$OUTPUT_FILE", keeping a single output contract. - Tests cover both regression directions: the prompt must reference
OUTPUT_FILEand must NOT contain the old "Output ONLY valid JSON to stdout" string;engine.shmust NOT containtee … OUTPUT_FILE;extract_verdict_jsonStyle 1 (dest-written) is the preferred path; and the pre-fix bug scenario (emptyresult+ missing dest) is pinned as the failure mode. - CodeRabbit gave an APPROVED state on the head commit; Sonar/CodeQL/gitleaks/AgentShield/shellcheck/bats all green.
CI status
All required checks green or appropriately skipped:
- ✅ CI (Lint, ShellCheck, Agent Security Scan, Compile agentic workflows, Secret scan/gitleaks)
- ✅ Lint (shellcheck, bats, validate-agent-profiles, gh-aw-compile)
- ✅ CodeQL (actions, python)
- ✅ AgentShield, SonarCloud, Dev-Lead dispatch, unit-tests
- ✅ PR Review Agent (latest run)
- ⏭️ Dependency-audit ecosystem jobs (no manifests changed), Dependabot, ci-relay (skip ci-relay marker)
- Two earlier "review" runs are CANCELLED — superseded by the green run; not a regression signal.
mergeStateStatus is BLOCKED only because REVIEW_REQUIRED is unsatisfied; this approval clears that gate.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
|
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #486 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 1215ef6f62a24d3d3bb8d97ab3e7974f677a6c47
Review mode: triage-approved (single reviewer)
Summary
Confirms the triage-approved verdict for a small, well-scoped bug fix to the single-reviewer review pipeline. The prompt is updated to write the verdict JSON to $OUTPUT_FILE via jq (matching the cascade-action.md pattern), engine.sh stops teeing Copilot stdout into $OUTPUT_FILE (which would interleave assistant text/tool transcripts and corrupt the JSON), and bats regression tests pin both the prompt contract and the engine.sh invariant. Token-log teeing (_tok_tmp) is preserved, and non-Copilot CLI branches are untouched.
Linked issue analysis
Closes #402 — Fleet Monitor flagged pr-review.yml as DEGRADED (25.2% failure rate, with many cancelled runs). The PR addresses the root cause: when TOKEN_LOG_FILE forced --output-format=json, the model's Bash echoes did not populate the result field, so the prior prompt (which told the model to "Output ONLY valid JSON to stdout") produced an empty verdict, and the engine.sh tee of stdout into $OUTPUT_FILE overwrote the model-written JSON with assistant chatter. Fix is substantive and matches the established pattern used by cascade-action.md.
Findings
- prompts/single-review.md — adds $OUTPUT_FILE to inputs, rewrites the Output section to write via jq, and rewrites the skip/draft branches accordingly. Body markdown template is preserved.
- scripts/engine.sh — removes
tee "$OUTPUT_FILE"from bothrun_agenticandrun_duckCopilot branches; keeps$_tok_tmpteeing intact; non-Copilot branches unchanged. Two# Do NOT tee…comment lines retain the twotee.*OUTPUT_FILEliteral substrings, but the regression test's regex (^[[:space:]]*[^[:space:]#]…) intentionally excludes comments, so the test passes. - tests/test_cli_error_routing.bats — adds 5 regression tests pinning: (1) prompt mentions OUTPUT_FILE, (2) the old "Output ONLY valid JSON to stdout" phrasing is gone, (3) the "Do NOT print" guard is present, (4) engine.sh copilot branch no longer tees to OUTPUT_FILE, (5) extract_verdict_json prefers the dest file (Style 1) over a claude-API-wrapper raw with empty result. No nits worth blocking on.
CI status
All required checks green: Lint, ShellCheck, bats, unit-tests, SonarCloud, CodeQL (actions + python), AgentShield, Agent Security Scan, validate-agent-profiles, Secret scan (gitleaks), gh-aw-compile, dependency-audit (Detect ecosystems success; ecosystem-specific jobs skipped as no relevant manifests). The only non-success entries are the current in-progress review job (this run) and the prior cancelled review job — both expected. reviewDecision is APPROVED (coderabbitai + donpetry-bot).
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 1215ef6f62a24d3d3bb8d97ab3e7974f677a6c47
Review mode: triage-approved (single reviewer)
Summary
Confirmation pass over triage's low-risk verdict. The PR fixes a self-corruption bug in the single-reviewer pipeline: the prompt now instructs the model to write verdict JSON directly to $OUTPUT_FILE via jq, and engine.sh no longer tees Copilot stdout into the same file (which was overwriting the JSON with assistant text/tool transcripts). Three regression tests guard the fix. Triage's assessment is correct.
Linked issue analysis
Closes #402 (Fleet Monitor: pr-review.yml degraded at 25.2% failure rate). The PR addresses the root cause described by Fleet Monitor: when TOKEN_LOG_FILE was set, engine.sh added --output-format json to Copilot, which prevented Bash-tool echoes from appearing in the result field. Combined with the prior tee of stdout into $OUTPUT_FILE, the verdict JSON was being clobbered. The fix mirrors the proven pattern in cascade-action.md (model writes via jq > "$OUTPUT_FILE") and removes the conflicting tee on the engine side. This should restore pr-review reliability.
Findings
No blocking issues.
- prompts/single-review.md: Skip-path branches (draft, head-sha-changed) and the main output flow are all updated consistently to write to $OUTPUT_FILE via jq; instructions are explicit about not printing JSON to stdout.
- scripts/engine.sh: Both run_agentic and run_duck copilot branches are correctly simplified — token-log tee preserved, OUTPUT_FILE tee removed; comments explain the rationale. No other call sites depend on the removed behavior.
- tests/test_cli_error_routing.bats: Adds five regression tests covering the prompt instructions, the engine.sh tee removal, and extract_verdict_json's preference for the dest file. Tests are positive and negative (including a 'bug scenario' guard).
Minor (non-blocking) note: the new shell-snippet in the prompt teaches the model to derive DECISION_MARKER, ESCALATE_TO_AI, etc. via embedded bash — slightly more surface area than the previous JSON-only output, but it matches the cascade-action.md pattern already in use, so consistency wins.
CI status
All required checks green: CI/Lint, ShellCheck, bats, unit-tests, validate-agent-profiles, gh-aw-compile, Compile agentic workflows, Agent Security Scan, Secret scan (gitleaks), CodeQL (actions + python), SonarCloud, AgentShield, dependency-audit, CodeRabbit. The only non-green entries are the currently in-progress review run (this one) and the prior cancelled review (superseded). reviewDecision is APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
Closes #402
Implemented by dev-lead agent. Please review.