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

scripts/engine.sh

@@ -315,6 +315,52 @@ is_diff_too_large() {
   printf '%s\n' "$text" | grep -qiE "(HTTP 406|exceeded the maximum number of files|diff exceeded the maximum)"
 }
 
+# _mcp_failure_pattern
+# Single source of truth for the MCP connection/init-failure regex. The claude
+# CLI surfaces an unreachable/failed MCP server in its stdout/stderr but still
+# exits 0 and produces a verdict, so this is a *warn-and-continue* signal, not a
+# fatal one. Tokens are intentionally kept clear of _rate_limit_pattern's
+# vocabulary (no "limit"/"quota"/"429"/"overload"/"exhaust") so a degraded MCP
+# server is never misclassified as a provider rate-limit and routed into the
+# cross-engine fallback — mirrors the caution at the chain-throttle warning.
+_mcp_failure_pattern() {
+  local _pat
+  _pat='mcp server [^[:space:]]*[[:space:]]*("[^"]*"[[:space:]]*)?(failed|error)'   # `MCP server "x" failed`
+  _pat="$_pat"'|failed to (connect|initialize|reconnect|start)[^.]*mcp'             # `failed to connect ... MCP`
+  _pat="$_pat"'|mcp[^.]*(connection|initializ)[^.]*(fail|error)'                    # `MCP connection failed`
+  _pat="$_pat"'|could not (connect to|start) mcp server'
+  printf '%s' "($_pat)"
+}
+
+# _emit_mcp_failure_warning <file>...
+# Graceful degradation (Fail Loud, Never Fake): when MCP is configured and any
+# captured CLI output file shows an MCP server connection/init failure, emit a
+# single ::warning:: naming the affected server(s) so the failure degrades
+# visibly in the Actions log / step summary. NEVER alters control flow — the
+# caller's exit code and the model's verdict are left untouched, so the review
+# completes on the model's base capabilities instead of aborting or faking an
+# "all clear". Inert (no scan, no output) when REVIEW_MCP_CONFIG is unset, so no
+# MCP warnings appear for runs that never configured MCP.
+_emit_mcp_failure_warning() {
+  # Guard: only meaningful when MCP was actually configured for this run.
+  [ -n "${REVIEW_MCP_CONFIG:-}" ] || return 0
+  local files=() f
+  for f in "$@"; do
+    [ -n "$f" ] && [ -f "$f" ] && files+=("$f")
+  done
+  [ "${#files[@]}" -eq 0 ] && return 0
+  grep -qiE "$(_mcp_failure_pattern)" "${files[@]}" || return 0
+  # Best-effort: name the server(s) from a quoted token near "MCP server".
+  local servers
+  servers="$(grep -hoiE 'mcp server "[^"]+"' "${files[@]}" 2>/dev/null \
+    | sed -E 's/.*"([^"]+)".*/\1/' | sort -u | paste -sd, - || true)"
+  if [ -n "$servers" ]; then
+    echo "::warning::[mcp] server(s) unavailable: ${servers} — review continues on the model's base capabilities (no MCP tool context)" >&2
+  else
+    echo "::warning::[mcp] an MCP server was unavailable — review continues on the model's base capabilities (no MCP tool context)" >&2
+  fi
+}
+
 # is_transient_failure <exit_code>
 # Returns 0 (true) for exit codes suggesting a flaky network/process state:
 # 124 (GNU timeout) and 137/143 (signal kills). JSON parse failures and
@@ -519,6 +565,13 @@ _claude_chain_invoke() {
     final_rc=2
   fi
 
+  # Graceful degradation (issue #678): if MCP was configured and the CLI reported
+  # an MCP server connection/init failure, surface a ::warning:: but do NOT touch
+  # final_rc — the review proceeds to its normal verdict on the model's base
+  # capabilities rather than fatal-exiting or faking an "all clear". Inert when
+  # the MCP knob is unset. Scans the captured files while they still exist.
+  _emit_mcp_failure_warning "$final_stdout" "$final_stderr"
+
   # Emit the final attempt's captured output to the caller. In JSON-usage mode,
   # parse the usage block and emit the extracted .result text (so consumers still
   # receive plain text); fall back to the raw payload if extraction is empty or

tests/dev-lead/unit/test_engine_mcp.bats

@@ -165,3 +165,67 @@ _source_engine() {
   ! grep -q -- "--allowed-tools" "$ARGS_RECORD"
   grep -q -- "--disallowed-tools" "$ARGS_RECORD"
 }
+
+# ── Graceful degradation: warn (never fake) on MCP server failure (issue #678) ─
+# The claude CLI surfaces an MCP connection/init failure in its captured output
+# but still exits 0 and produces a verdict. engine.sh must warn (naming the
+# server) and let the review complete — no fatal exit, no fabricated "all clear".
+
+@test "agentic: MCP failure marker + knob set → ::warning:: names server, verdict still emitted" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export STUB_ENGINE_RESPONSE=$'MCP server "context7" failed to connect after 3 attempts\n{"decision":"approve","summary":"looks good"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  # AC #2: never a fatal exit on MCP failure.
+  [ "$status" -eq 0 ]
+  # AC #1: a warning is emitted naming the affected server.
+  [[ "$output" == *"::warning::"* ]]
+  [[ "$output" == *"mcp"* ]]
+  [[ "$output" == *"context7"* ]]
+  # AC #1/#2: the review still returns its normal, non-empty verdict.
+  [[ "$output" == *'"decision":"approve"'* ]]
+}
+
+@test "agentic: MCP failure marker but knob UNSET → no MCP warning (AC #3)" {
+  _source_engine "claude"
+  unset REVIEW_MCP_CONFIG
+  export STUB_ENGINE_RESPONSE=$'MCP server "context7" failed to connect after 3 attempts\n{"decision":"approve","summary":"looks good"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  # No MCP-related warning when MCP was never configured.
+  ! [[ "$output" == *"::warning::[mcp]"* ]]
+  # Verdict still flows through untouched.
+  [[ "$output" == *'"decision":"approve"'* ]]
+}
+
+@test "agentic: knob set but no MCP failure in output → no false-positive warning" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export STUB_ENGINE_RESPONSE='{"decision":"approve","summary":"clean run, MCP healthy"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  ! [[ "$output" == *"::warning::[mcp]"* ]]
+  [[ "$output" == *'"decision":"approve"'* ]]
+}
+
+@test "agentic: MCP failure without quoted server name + knob set → generic warning, review continues" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export STUB_ENGINE_RESPONSE=$'Failed to connect to MCP server\n{"decision":"comment","summary":"degraded"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"::warning::[mcp]"* ]]
+  [[ "$output" == *'"decision":"comment"'* ]]
+}
+
+@test "duck: MCP failure marker + knob set → ::warning:: emitted, duck verdict continues" {
+  _source_engine "claude"
+  export DUCK_ENGINE="claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export STUB_ENGINE_RESPONSE=$'MCP server "context7" failed to connect\n{"decision":"approve","summary":"duck ok"}'
+  run run_duck "$TEST_PROMPT" "claude-sonnet-4-6"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"::warning::[mcp]"* ]]
+  [[ "$output" == *"context7"* ]]
+  [[ "$output" == *'"decision":"approve"'* ]]
+}