feat(pr-review): add REVIEW_MCP_DEBUG knob to surface the MCP handshake by don-petry · Pull Request #892 · petry-projects/.github-private

don-petry · 2026-06-21T13:17:59Z

Summary

The MCP review integration is "fail loud, never fake": engine.sh emits a ::warning:: only when an MCP server fails to connect, and is deliberately silent on success (_emit_mcp_failure_warning). That's correct for production, but it means a healthy MCP run is provable only by the absence of a warning — there's no affirmative "Context7 connected" signal in the CI log.

This adds an opt-in REVIEW_MCP_DEBUG knob (off by default) so a one-off run can prove connectivity:

engine.sh threads --debug mcp into the MCP-enabled claude tiers (deep + duck) when REVIEW_MCP_DEBUG is set, so the server handshake is logged to the captured stderr.
_emit_mcp_failure_warning surfaces a successful MCP server "…": Successfully connected … line as a ::notice::[mcp] …. Control flow and the review verdict are untouched.
pr-review.yml forwards vars.REVIEW_MCP_DEBUG so CI can toggle it with a repo variable — no code change needed.

Default behavior is byte-for-byte unchanged (the flag and notice only appear when both MCP is configured and REVIEW_MCP_DEBUG is non-empty).

Why

Verifying the v1.8.0 (pr-review/next) ring-0 rollout, the live runs showed no MCP-failure warning — good by the fail-loud contract, but no positive proof Context7 actually connected. This knob closes that gap.

Verification

Run locally against the committed .github/review-mcp.json (identical to what v1.8.0 ships), mirroring engine.sh's deep-tier flags:

MCP server "context7": Successfully connected (transport: http) in 333ms
Connection established with capabilities: {"hasTools":true,...,"version":"3.2.0"}
MCP server "context7": Calling MCP tool: resolve-library-id
MCP server "context7": Tool 'resolve-library-id' completed successfully in 1s

The model returned TOOL_OK — Context7 connects and its tools execute end-to-end.

Tests

Adds 5 regression tests to test_engine_mcp.bats (flag threading on/off, MCP-off no-op, affirmative ::notice::, opt-in gating). Full suite 29/29 green; shellcheck scripts/engine.sh clean.

🤖 Generated with Claude Code

Generated by Claude Code

The MCP integration is "fail loud, never fake": engine.sh emits a ::warning:: only when an MCP server fails to connect and stays silent on success, so a healthy MCP run was provable only by the *absence* of a warning. Add an opt-in REVIEW_MCP_DEBUG knob (off by default): - engine.sh threads `--debug mcp` into the MCP-enabled claude tiers so the server handshake is logged to the captured stderr. - _emit_mcp_failure_warning surfaces a successful "Successfully connected" handshake as a ::notice:: (verdict and control flow untouched). - pr-review.yml forwards vars.REVIEW_MCP_DEBUG so CI can toggle it without a code change. Verified locally against the committed .github/review-mcp.json: claude connects to context7 (transport http, 333ms) and a resolve-library-id tool call succeeds. Adds 5 regression tests (29/29 green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VRMMWmqW9nW81jygmea3e

chatgpt-codex-connector · 2026-06-21T13:18:04Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

coderabbitai · 2026-06-21T13:18:08Z

Warning

Review limit reached

@don-petry, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 52 minutes and 41 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3299b69c-2d65-439f-9774-e2f821786aa9

📥 Commits

Reviewing files that changed from the base of the PR and between 42913ab and 8203876.

📒 Files selected for processing (3)

.github/workflows/pr-review.yml
scripts/engine.sh
tests/dev-lead/unit/test_engine_mcp.bats

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch claude/planner-scope-updates-jxwwzh

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request introduces a new environment variable REVIEW_MCP_DEBUG to enable opt-in diagnostics for MCP-enabled Claude tiers. When set, it appends --debug mcp to the CLI flags and parses captured stderr to surface successful handshakes as GitHub Actions notices. Unit tests have been added to verify this behavior. The reviewer suggested using printf instead of echo for better portability and robustness when outputting the handshake notices.

Use printf for the REVIEW_MCP_DEBUG ::notice:: line — _ln is an arbitrary captured log line, so avoid echo's inconsistent handling of leading hyphens and backslashes across shells. Addresses gemini-code-assist review on #892. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VRMMWmqW9nW81jygmea3e

sonarqubecloud · 2026-06-21T13:21:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

don-petry · 2026-06-21T13:25:21Z

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #892
No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically. Next attempt after: 2026-06-21T13:55:19Z

don-petry · 2026-06-21T13:25:21Z

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.
Next attempt after: 2026-06-21T13:55:19Z

don-petry · 2026-06-21T13:26:24Z

Dev-Lead — fix-reviews (no-changes)

Agent reasoning

Addressed 0 threads:
(none — open thread list was empty)
CI blockers: none (all checks passed or skipped; one in_progress dispatch job is not a failure)
CHANGES_REQUESTED reviews: none
Test verification: skipped — no code changes made
Files changed: none
```

donpetry-bot

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: 8203876e5b0718dd3d672fed4eec8394e5d3729d
Review mode: triage-approved (single reviewer)

Summary

Adds an opt-in REVIEW_MCP_DEBUG knob so a healthy MCP run is provable in CI. engine.sh threads --debug mcp into the MCP-enabled claude tiers (deep + duck) only when both REVIEW_MCP_CONFIG and REVIEW_MCP_DEBUG are set, and surfaces a successful handshake as a ::notice::[mcp]; pr-review.yml forwards vars.REVIEW_MCP_DEBUG (default empty). Default behavior is byte-for-byte unchanged. +89/-3 across 3 files, with 5 new regression tests.

Linked issue analysis

No linked issue (closingIssuesReferences empty); not required for a diagnostics-only enhancement. Motivation is stated in the PR: the v1.8.0 (pr-review/next) ring-0 rollout showed no MCP-failure warning but no affirmative proof of Context7 connectivity — this knob closes that observability gap.

Findings

• Security: the new ::notice:: path printf-formats a captured stderr line with %s (no eval/word-splitting), and grep -hoiE matches single lines only, so a malicious payload cannot inject a new GitHub Actions workflow-command line. No secrets touched; secret scan (gitleaks) CI check green. (GitHub Secret Protection MCP tool not exposed in this environment — gitleaks remains authoritative.)
• Shared-surface (downstream-impact): pr-review.yml is pinned by 5 consumer repos, but the change is purely additive with an empty default (vars.REVIEW_MCP_DEBUG || ''), so consumers are unaffected unless they opt in. No breaking change.
• Diagnostics scope: --debug mcp is scoped to the MCP subsystem and gated behind an admin-set repo variable; it rides the MCP flags only (no MCP config → no --debug), verified by test.
• Default-unchanged and opt-in gating are both covered by the 5 new bats tests (flag threading on/off, MCP-off no-op, affirmative ::notice::, opt-in-only).
• Prior bot feedback: gemini-code-assist's printf-over-echo suggestion was addressed in the second commit (8203876); CodeRabbit APPROVED; SonarCloud quality gate passed.

CI status

All required checks green: CodeQL (actions+python), SonarCloud, Secret scan (gitleaks), ShellCheck, bats, unit-tests, validate-fixtures, AgentShield, Holdout Guard, and the Test Dev-Lead Agent suite. Two dev-lead/dispatch and dev-lead/ci-relay runs show CANCELLED but were superseded by later SUCCESS runs of the same jobs (agent self-retry, not a failure). mergeStateStatus is BLOCKED only on the pending org-leads human review request, which is the expected gate — not a check failure.

Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.

…ck (#903) (#905) * Add MCP/Context7 connectivity assertion to daily health check (#903) The PR-review pipeline only surfaced MCP health as a fail-loud ::warning::[mcp] on a *real* review (engine.sh:_emit_mcp_failure_warning) — a healthy run logged nothing, so a silent Context7 outage stayed invisible until a review happened to run. There was no durable, affirmative monitor. Add scripts/mcp_connectivity_check.sh: a self-contained preflight that drives the configured MCP server(s) through the SAME flags engine.sh threads into its claude tiers (--mcp-config <f> --strict-mcp-config --debug mcp) and asserts the handshake (MCP server "...": Successfully connected). Behavior contract mirrors the engine's opt-in MCP gating: - MCP unconfigured (no readable config) -> no-op ::notice:: skip, exit 0 (no behavior change for non-MCP repos). - configured + reachable -> ::notice::[mcp] per handshake, exit 0. - configured + unreachable -> ::error::[mcp], exit 1 (fails the health check). Config resolution mirrors engine.sh (explicit REVIEW_MCP_CONFIG wins; empty disables; else conventional .github/review-mcp.json). Allowed-tools are derived per configured server (mcp__<name>__*). Wire it into daily-pr-review-health.yml as an `if: always()` step so the MCP monitor stays independent of the PR-review-failure analysis (neither masks the other), reusing the #892 REVIEW_MCP_DEBUG affirmative-diagnostics signal shape. Add tests/test_mcp_connectivity_check.bats (stubbed claude) and register it in lint.yml. Part of #676. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr * Harden MCP connectivity check per review feedback (#903) Address gemini-code-assist review on #905: - _mcp_resolve_config now fails loud (::error:: + exit 1) when MCP is *explicitly* configured (REVIEW_MCP_CONFIG non-empty, or the conventional committed path present) but the file is missing/unreadable. Silently skipping a misconfigured-but-intended MCP setup would mask exactly the silent breakage this monitor exists to catch. The genuine "not configured" no-op (unset + conventional path absent; or explicit empty) still exits 0, so non-MCP repos are unchanged. - main() propagates that failure (`cfg="$(_mcp_resolve_config)" || return 1`) instead of letting set -e swallow it inside the substitution. - Use a `trap '... ' EXIT` to clean up the capture file even on interrupt and drop the now-redundant manual rm calls. - Replace the while-read notice loop with an idiomatic `sed` prefix. Tests: add fail-loud coverage for the missing/unreadable explicit-config paths (the chmod-000 readability case skips under root, where the gate is a no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr --------- Co-authored-by: Claude <noreply@anthropic.com>

…n latency gap (#898) (#914) * pr-review-sweep: event-driven re-trigger to close the ci-pending→green latency gap (#898) Reframes #898 from its (incorrect) marker hypothesis to the real cause. The ci-pending skip in review-one-pr.sh exits 100 BEFORE the idempotency check and writes no marker, so it is already non-terminal; the selection logic that re-reviews a stuck-green PR (REVIEW_REQUIRED + CI green + no marker) is correct and already covered by tests. What stalled #892 was purely LATENCY: the only thing that re-reviews a PR which goes green after the last trigger fired was the hourly stuck-review sweep (#573), so the PR could sit green-but-unreviewed for up to an hour (the human force-reviewed before the next tick). Close the gap with two robust, layered triggers on pr-review-sweep.yml: - Scheduled cron tightened from hourly to every 15 min — the GUARANTEED backstop bounding worst-case re-review latency, with zero fragility. - A `workflow_run: completed` fast path keyed on the broadly-run CI workflows. When CI finishes, the sweep runs SCOPED to just that run's PR(s) — sweep-stuck-reviews.sh derives them from the event payload (`prs_from_workflow_run_event`, pure/unit-tested) instead of enumerating the fleet — so a PR that just went green is re-reviewed in seconds. Per-branch `cancel-in-progress` concurrency collapses the multi-workflow burst so the sweep effectively runs once, after the last-finishing workflow (most likely to observe green). The fast path can never strand a PR: if it matches nothing (fork PR, branch push, CI still pending, or a workflow rename) the scheduled sweep still catches it, and the same REVIEW_REQUIRED + green + no-marker gate decides in every path. Tests: 5 new cases in tests/test_sweep_stuck_reviews.bats exercise the workflow_run path — ci-pending→green dispatches (scoped to the event), empty pull_requests is a clean no-op, a too-early fire (CI still pending) does not dispatch, an already-reviewed-at-head PR is not re-dispatched, and a multi-PR run evaluates each independently. shellcheck clean; existing 21 sweep tests unchanged and green. AGENTS.md exception note updated to describe both triggers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr * sweep: fold prs_from_workflow_run_event into a single jq pass (review) Address gemini-code-assist's medium-priority note on #914: parse the workflow_run event payload exactly once. Bind .repository.full_name inside the jq program and `select` it out (empty stream) when absent, instead of a separate jq invocation + shell guard — one process, one read, same behavior. The positional-arg guard (`${1:-}`, set -u-safe) and the URL output are unchanged; all 26 sweep tests still pass and shellcheck is clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr --------- Co-authored-by: Claude <noreply@anthropic.com>

…ck (#903) (#905) * Add MCP/Context7 connectivity assertion to daily health check (#903) The PR-review pipeline only surfaced MCP health as a fail-loud ::warning::[mcp] on a *real* review (engine.sh:_emit_mcp_failure_warning) — a healthy run logged nothing, so a silent Context7 outage stayed invisible until a review happened to run. There was no durable, affirmative monitor. Add scripts/mcp_connectivity_check.sh: a self-contained preflight that drives the configured MCP server(s) through the SAME flags engine.sh threads into its claude tiers (--mcp-config <f> --strict-mcp-config --debug mcp) and asserts the handshake (MCP server "...": Successfully connected). Behavior contract mirrors the engine's opt-in MCP gating: - MCP unconfigured (no readable config) -> no-op ::notice:: skip, exit 0 (no behavior change for non-MCP repos). - configured + reachable -> ::notice::[mcp] per handshake, exit 0. - configured + unreachable -> ::error::[mcp], exit 1 (fails the health check). Config resolution mirrors engine.sh (explicit REVIEW_MCP_CONFIG wins; empty disables; else conventional .github/review-mcp.json). Allowed-tools are derived per configured server (mcp__<name>__*). Wire it into daily-pr-review-health.yml as an `if: always()` step so the MCP monitor stays independent of the PR-review-failure analysis (neither masks the other), reusing the #892 REVIEW_MCP_DEBUG affirmative-diagnostics signal shape. Add tests/test_mcp_connectivity_check.bats (stubbed claude) and register it in lint.yml. Part of #676. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr * Harden MCP connectivity check per review feedback (#903) Address gemini-code-assist review on #905: - _mcp_resolve_config now fails loud (::error:: + exit 1) when MCP is *explicitly* configured (REVIEW_MCP_CONFIG non-empty, or the conventional committed path present) but the file is missing/unreadable. Silently skipping a misconfigured-but-intended MCP setup would mask exactly the silent breakage this monitor exists to catch. The genuine "not configured" no-op (unset + conventional path absent; or explicit empty) still exits 0, so non-MCP repos are unchanged. - main() propagates that failure (`cfg="$(_mcp_resolve_config)" || return 1`) instead of letting set -e swallow it inside the substitution. - Use a `trap '... ' EXIT` to clean up the capture file even on interrupt and drop the now-redundant manual rm calls. - Replace the while-read notice loop with an idiomatic `sed` prefix. Tests: add fail-loud coverage for the missing/unreadable explicit-config paths (the chmod-000 readability case skips under root, where the gate is a no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr --------- Co-authored-by: Claude <noreply@anthropic.com>

…ke (#892) * feat(pr-review): add REVIEW_MCP_DEBUG knob to surface MCP handshake The MCP integration is "fail loud, never fake": engine.sh emits a ::warning:: only when an MCP server fails to connect and stays silent on success, so a healthy MCP run was provable only by the *absence* of a warning. Add an opt-in REVIEW_MCP_DEBUG knob (off by default): - engine.sh threads `--debug mcp` into the MCP-enabled claude tiers so the server handshake is logged to the captured stderr. - _emit_mcp_failure_warning surfaces a successful "Successfully connected" handshake as a ::notice:: (verdict and control flow untouched). - pr-review.yml forwards vars.REVIEW_MCP_DEBUG so CI can toggle it without a code change. Verified locally against the committed .github/review-mcp.json: claude connects to context7 (transport http, 333ms) and a resolve-library-id tool call succeeds. Adds 5 regression tests (29/29 green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VRMMWmqW9nW81jygmea3e * refactor(pr-review): printf over echo for MCP debug notice Use printf for the REVIEW_MCP_DEBUG ::notice:: line — _ln is an arbitrary captured log line, so avoid echo's inconsistent handling of leading hyphens and backslashes across shells. Addresses gemini-code-assist review on #892. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VRMMWmqW9nW81jygmea3e --------- Co-authored-by: Claude <noreply@anthropic.com>

don-petry requested a review from a team as a code owner June 21, 2026 13:18

gemini-code-assist Bot reviewed Jun 21, 2026

View reviewed changes

Comment thread scripts/engine.sh Outdated

don-petry enabled auto-merge (squash) June 21, 2026 13:25

coderabbitai Bot approved these changes Jun 21, 2026

View reviewed changes

don-petry disabled auto-merge June 21, 2026 13:26

don-petry enabled auto-merge (squash) June 21, 2026 13:26

donpetry-bot approved these changes Jun 21, 2026

View reviewed changes

don-petry merged commit e634fdb into main Jun 21, 2026
43 of 47 checks passed

don-petry deleted the claude/planner-scope-updates-jxwwzh branch June 21, 2026 14:59

don-petry mentioned this pull request Jun 23, 2026

pr-review-sweep: event-driven re-trigger to close the ci-pending→green latency gap (#898) #914

Merged

donpetry-bot mentioned this pull request Jun 24, 2026

feat: implement issue #898 — pr-review self-review deadlock: ci-pending skip + "already reviewed" marker blocks approval once CI goes green #899

Merged

github-actions Bot mentioned this pull request Jun 24, 2026

petry-projects — workflow failures detected 2026-06-24 #917

Closed

This was referenced Jun 2, 2026

petry-projects — workflow failures detected 2026-06-02 #419

Closed

petry-projects — workflow failures detected 2026-06-16 #764

Closed

github-actions Bot mentioned this pull request Jul 27, 2026

📊 Token Cost Observatory — Weekly Report #464

Open

don-petry mentioned this pull request Aug 21, 2026

dev-lead reports status=applied / 'Changes committed and pushed' when the requested review changes were not applied #1567

Closed

github-actions Bot mentioned this pull request Sep 13, 2026

petry-projects — workflow failures detected 2026-09-13 #1810

Closed

Navigation Menu

feat(pr-review): add REVIEW_MCP_DEBUG knob to surface the MCP handshake - #892

feat(pr-review): add REVIEW_MCP_DEBUG knob to surface the MCP handshake#892
don-petry merged 2 commits into
mainfrom
claude/planner-scope-updates-jxwwzh

don-petry commented Jun 21, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 21, 2026

Uh oh!

coderabbitai Bot commented Jun 21, 2026 •

edited

Loading

Review limit reached

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 21, 2026

Uh oh!

don-petry commented Jun 21, 2026

Uh oh!

don-petry commented Jun 21, 2026

Uh oh!

don-petry commented Jun 21, 2026

Uh oh!

donpetry-bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

don-petry commented Jun 21, 2026

Summary

Why

Verification

Tests

Uh oh!

chatgpt-codex-connector Bot commented Jun 21, 2026

Uh oh!

coderabbitai Bot commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 21, 2026

Quality Gate passed

Uh oh!

don-petry commented Jun 21, 2026

Dev-Lead — waiting on PR blockers (intent: review-changes)

Uh oh!

don-petry commented Jun 21, 2026

Uh oh!

don-petry commented Jun 21, 2026

Dev-Lead — fix-reviews (no-changes)

Uh oh!

donpetry-bot left a comment

Choose a reason for hiding this comment

Automated review — APPROVED ✓

Summary

Linked issue analysis

Findings

CI status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jun 21, 2026 •

edited

Loading