[Phase 1] Graceful degradation: warn (never fake) when an MCP server is unavailable

[github-actions]

Story

As a maintainer of the pr-review engine,
I want detect an MCP server connection/init failure during a review and emit a GitHub warning annotation while the review still completes,
so that an unavailable MCP server degrades visibly (Fail Loud, Never Fake) instead of silently dropping context or emitting a fabricated "all clear" verdict.

Acceptance Criteria

1. When the claude CLI reports an MCP server failed to connect/initialize, engine.sh emits a ::warning:: annotation naming the affected server and the review continues to completion using the model's base capabilities.
2. An MCP failure never causes a fatal exit (no code-1 abort) and never short-circuits to an empty or fabricated "all clear" verdict.
3. When MCP is not configured (knob unset), no MCP-related warnings are emitted.
4. Behavior is covered by a test that simulates an unreachable server / failure marker in captured CLI output.

Tasks / Subtasks

• Detect a stable MCP-failure marker in the claude CLI output captured by _claude_chain_invoke (final_stdout/final_stderr). (AC: test issue from agent #1)
• Emit a ::warning:: naming the server and ensure the review proceeds and returns its normal verdict (no fatal exit, no empty verdict). (AC: test issue from agent #1, Go-live improvements for PR review agent #2)
• Guard the detection so it is inert when the MCP knob is unset. (AC: Add @claude delegation, auto-merge, and rebase handling #3)
• Add a test feeding a synthetic MCP-failure transcript and asserting the warning + continued, non-empty verdict. (AC: Optimize review: small-PR and incremental fast paths #4)

Dev Notes

• Builds directly on story 1's MCP plumbing in engine.sh.
• Pattern to mirror: engine.sh already emits ::warning:: for throttled models (engine.sh:494) and the review pipeline already degrades-without-fatal-exit on oversized diffs (AGENTS.md 'Oversized PRs': the agent never exits 1 on a 406). Apply the same fail-loud-but-continue shape.
• MCP connection/init errors surface in the CLI's stdout/stderr, captured in _claude_chain_invoke's final_stdout/final_stderr. When choosing a detection marker, AVOID tokens that collide with is_rate_limited()/_rate_limit_pattern — see the explicit caution at engine.sh:489-493 where fallback phrasing is kept clear of rate-limit tokens so downstream callers don't misclassify.
• 'Graceful degradation (Fail Loud, Never Fake)' with disclosed warning annotations is a stated design requirement of MCP-Powered CI Review Enhancement — Standard #650; keep the warning human-readable in the Actions log/step summary.

Project Structure Notes

Confined to scripts/engine.sh and a bats test. No workflow changes.

References

• scripts/engine.sh:494 (existing ::warning:: on model throttle)
• scripts/engine.sh:489 (caution: avoid rate-limit token collisions)
• AGENTS.md#oversized-prs-300-changed-files (never fatal-exit, degrade with note)
• discussion MCP-Powered CI Review Enhancement — Standard #650 (Design item 3: graceful degradation)

Likely target surface

• scripts/engine.sh
• tests/

Story prepared by the BMAD Scrum Master (Bob) for epic #676. Status: ready-for-dev.

[don-petry]

Dev-Lead Implementation Plan

Issue: #678 — [Phase 1] Graceful degradation: warn (never fake) when an MCP server is unavailable

Scope

Detect an MCP server connection/init failure in the claude CLI output captured by _claude_chain_invoke and emit a ::warning:: (naming the server) while the review continues to its normal verdict — inert when the MCP knob (REVIEW_MCP_CONFIG) is unset.

Implementation Checklist

• Add _mcp_failure_pattern() to scripts/engine.sh — single source of truth for the MCP-failure regex, deliberately kept clear of _rate_limit_pattern tokens (no "limit"/"quota"/"429"/"overload"/"exhaust") so a degraded server is never misclassified as a provider rate-limit.
• Add _emit_mcp_failure_warning <files...> to scripts/engine.sh — guarded by REVIEW_MCP_CONFIG (inert when unset); scans captured stdout/stderr; emits one ::warning:: naming the affected server(s); never alters control flow or exit code.
• Call _emit_mcp_failure_warning "$final_stdout" "$final_stderr" inside _claude_chain_invoke before the captured output is flushed/removed — so the warning fires but final_rc and the verdict are untouched (Fail Loud, Never Fake; no fatal exit, no fabricated "all clear").

Tests to Write (TDD, in tests/dev-lead/unit/test_engine_mcp.bats)

• MCP-failure marker in CLI output + knob set → ::warning:: naming the server is emitted AND the verdict output is still present, status 0 (AC test issue from agent #1, Go-live improvements for PR review agent #2).
• MCP-failure marker present but knob UNSET → no MCP warning emitted (AC Add @claude delegation, auto-merge, and rebase handling #3).
• No MCP-failure marker + knob set → no false-positive MCP warning.
• MCP-failure marker without a quoted server name + knob set → generic ::warning:: still emitted, review continues.

Test Command

bats tests/dev-lead/unit/test_engine_mcp.bats (full suite: bats tests/dev-lead/unit/), plus bash .dev-lead/scripts/dev-lead-lint.sh

[don-petry]

Dev-Lead: Implementation Complete

Plan Execution

• Added _mcp_failure_pattern() to scripts/engine.sh — single-source MCP-failure regex, kept clear of _rate_limit_pattern tokens so a degraded server is never misclassified as a provider rate-limit.
• Added _emit_mcp_failure_warning <files...> — guarded by REVIEW_MCP_CONFIG (inert when unset); scans captured stdout/stderr; emits one ::warning:: naming the affected server(s); never alters control flow.
• Wired the call into _claude_chain_invoke before the captured output is flushed — warning fires but final_rc and the verdict are untouched (no fatal exit, no fabricated "all clear").
• Added 5 bats cases to tests/dev-lead/unit/test_engine_mcp.bats (TDD: written failing first, then made green).

How it satisfies the Acceptance Criteria

• AC test issue from agent #1 — On an MCP connect/init failure in CLI output, a ::warning::[mcp] server(s) unavailable: <name> … is emitted and the review continues to its normal verdict on the model's base capabilities.
• AC Go-live improvements for PR review agent #2 — Detection never touches final_rc and never empties the output → no code-1 abort, no short-circuit to an empty/fabricated verdict.
• AC Add @claude delegation, auto-merge, and rebase handling #3 — _emit_mcp_failure_warning returns early when REVIEW_MCP_CONFIG is unset → no MCP warnings when MCP isn't configured.
• AC Optimize review: small-PR and incremental fast paths #4 — New tests feed a synthetic MCP-failure transcript and assert the warning + a continued, non-empty verdict (named-server, generic-no-quote, and duck-tier variants), plus negative tests (knob unset / no marker).

Test Results

$ bats tests/dev-lead/unit/test_engine_mcp.bats
1..15
ok 1 agentic: no MCP knob → no MCP flags, default allowed-tools unchanged
...
ok 10 triage: MCP knob set → triage still uses --disallowed-tools only, no MCP
ok 11 agentic: MCP failure marker + knob set → ::warning:: names server, verdict still emitted
ok 12 agentic: MCP failure marker but knob UNSET → no MCP warning (AC #3)
ok 13 agentic: knob set but no MCP failure in output → no false-positive warning
ok 14 agentic: MCP failure without quoted server name + knob set → generic warning, review continues
ok 15 duck: MCP failure marker + knob set → ::warning:: emitted, duck verdict continues

$ bats tests/dev-lead/unit/   →  passing: 468  failing: 0
$ bash .dev-lead/scripts/dev-lead-lint.sh   →  [lint] all checks passed

Files Changed

• scripts/engine.sh: new _mcp_failure_pattern() + _emit_mcp_failure_warning() helpers; one call site in _claude_chain_invoke (warn-and-continue).
• tests/dev-lead/unit/test_engine_mcp.bats: +5 cases covering the warn/continue path and the knob-unset / no-marker negatives.

Notes

• Detection lives in _claude_chain_invoke (per the issue's Dev Notes) so it covers the deep and duck claude tiers; the triage tier never configures MCP servers, so the knob-guard plus "no marker in output" keeps it free of false positives.
• The warning text and the detection regex both deliberately avoid rate-limit vocabulary, mirroring the caution at the chain-throttle warning, so neither the CLI marker nor our warning is misclassified by downstream is_rate_limited scans of engine stdout/stderr.
• Confined to scripts/engine.sh + a bats test; no workflow changes. Server-name extraction is best-effort (named when the CLI quotes the server, generic otherwise).