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

.github/workflows/pr-review.yml

@@ -178,6 +178,10 @@ jobs:
       # server yet blocks the tool calls ("not permitted"); set the repo variable
       # REVIEW_MCP_ALLOWED_TOOLS (e.g. `mcp__github__*`) to permit them.
       REVIEW_MCP_ALLOWED_TOOLS: ${{ vars.REVIEW_MCP_ALLOWED_TOOLS || '' }}
+      # Diagnostics opt-in: set repo variable REVIEW_MCP_DEBUG=1 to log the MCP
+      # server handshake (`--debug mcp`) and surface a ::notice:: for a healthy
+      # connection. Off by default; never changes the review verdict.
+      REVIEW_MCP_DEBUG: ${{ vars.REVIEW_MCP_DEBUG || '' }}
       # Downstream-impact pass (epic #748 / discussion #653): annotate a review
       # with the consumer repos that pin a shared surface the PR changes. Enabled
       # by default here; set the repo variable DOWNSTREAM_IMPACT_ENABLED=false to

scripts/engine.sh

@@ -53,6 +53,10 @@ RETRY_BASE_DELAY_SEC="${RETRY_BASE_DELAY_SEC:-5}"
 # REVIEW_MCP_ALLOWED_TOOLS — comma-separated MCP tool names (e.g.
 #   "mcp__context7__*") merged into the per-tier --allowed-tools list so the
 #   configured MCP tools are actually permitted. Default empty.
+# REVIEW_MCP_DEBUG — when set to a non-empty value, the MCP-enabled claude tiers
+#   add `--debug mcp` so the server handshake is logged to stderr, and a healthy
+#   connection is surfaced as a ::notice:: (the failure path already emits a
+#   ::warning::). Off by default — diagnostics only; never changes the verdict.
 # Only the deep (run_agentic) and rubber-duck (run_duck) claude tiers receive
 # this — the triage tier stays fast/restricted (--disallowed-tools only).
 #
@@ -69,10 +73,11 @@ if [ -z "${REVIEW_MCP_CONFIG+x}" ] && [ -f "$REVIEW_MCP_CONFIG_DEFAULT_PATH" ];
 fi
 REVIEW_MCP_CONFIG="${REVIEW_MCP_CONFIG:-}"
 REVIEW_MCP_ALLOWED_TOOLS="${REVIEW_MCP_ALLOWED_TOOLS:-}"
+REVIEW_MCP_DEBUG="${REVIEW_MCP_DEBUG:-}"
 # Export so the resolved knob survives into any review subprocess. review-one-pr.sh
 # and review-batch.sh source this file, so run_agentic/run_duck see it in-process;
 # the export also covers any future invocation via a separate child shell.
-export REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS
+export REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS REVIEW_MCP_DEBUG
 
 set_engine_config() {
   case "$REVIEW_ENGINE" in
@@ -378,6 +383,19 @@ _emit_mcp_failure_warning() {
     [ -n "$f" ] && [ -f "$f" ] && files+=("$f")
   done
   [ "${#files[@]}" -eq 0 ] && return 0
+  # Opt-in affirmative diagnostics (REVIEW_MCP_DEBUG): surface the successful
+  # handshake so a healthy MCP run is provable, not merely inferred from the
+  # absence of the failure ::warning:: below. `--debug mcp` (added in
+  # _mcp_review_flags) writes these "Successfully connected" lines to the
+  # captured stderr. Never alters control flow.
+  if [ -n "${REVIEW_MCP_DEBUG:-}" ]; then
+    grep -hoiE 'mcp server "[^"]+": successfully connected.*' "${files[@]}" 2>/dev/null \
+      | sort -u | while IFS= read -r _ln; do
+        # printf (not echo): _ln is an arbitrary captured log line, so avoid
+        # echo's inconsistent handling of leading hyphens / backslashes.
+        printf '::notice::[mcp] %s\n' "${_ln}" >&2
+      done
+  fi
   grep -qiE "$(_mcp_failure_pattern)" "${files[@]}" || return 0
   # Best-effort: name the server(s) from a quoted token near "MCP server".
   local servers
@@ -805,6 +823,12 @@ _mcp_review_flags() {
   _MCP_ALLOWED_TOOLS="$base"
   if [ -n "${REVIEW_MCP_CONFIG:-}" ] && [ -f "${REVIEW_MCP_CONFIG}" ] && [ -r "${REVIEW_MCP_CONFIG}" ]; then
     _MCP_FLAGS=(--mcp-config "$REVIEW_MCP_CONFIG" --strict-mcp-config)
+    if [ -n "${REVIEW_MCP_DEBUG:-}" ]; then
+      # Surface the MCP server handshake in the CLI's stderr (captured + scanned
+      # by _emit_mcp_failure_warning). `--debug mcp` scopes debug to the MCP
+      # subsystem only, so the log stays readable. Diagnostics-only opt-in.
+      _MCP_FLAGS+=(--debug mcp)
+    fi
     if [ -n "${REVIEW_MCP_ALLOWED_TOOLS:-}" ]; then
       _MCP_ALLOWED_TOOLS="${base},${REVIEW_MCP_ALLOWED_TOOLS}"
     fi

tests/dev-lead/unit/test_engine_mcp.bats

@@ -17,7 +17,7 @@ setup() {
 
   unset CLAUDE_TRIAGE_MODEL_CHAIN CLAUDE_DEEP_MODEL_CHAIN CLAUDE_AUDIT_MODEL_CHAIN
   unset CLAUDE_ACTION_MODEL_CHAIN CLAUDE_SINGLE_MODEL_CHAIN
-  unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS REVIEW_MCP_CONFIG_DEFAULT_PATH
+  unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS REVIEW_MCP_CONFIG_DEFAULT_PATH REVIEW_MCP_DEBUG
 
   STUB_BIN_DIR="$(mktemp -d)"
   cp "$STUB_ENGINES_DIR/stub-claude" "$STUB_BIN_DIR/claude"
@@ -46,7 +46,7 @@ teardown() {
   rm -f "$GITHUB_ENV" "$GITHUB_OUTPUT" "$TEST_PROMPT" "$ARGS_RECORD" "$MCP_CONFIG_FILE"
   rm -rf "$STUB_BIN_DIR"
   unset STUB_ENGINE_RECORD_ARGS STUB_ENGINE_EXIT
-  unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS REVIEW_MCP_CONFIG_DEFAULT_PATH
+  unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS REVIEW_MCP_CONFIG_DEFAULT_PATH REVIEW_MCP_DEBUG
 }
 
 _source_engine() {
@@ -322,3 +322,61 @@ _source_engine() {
   [[ "$output" == *"context7"* ]]
   [[ "$output" == *'"decision":"approve"'* ]]
 }
+
+# ── Affirmative debug diagnostics (REVIEW_MCP_DEBUG) ──────────────────────────
+# The failure path is "fail loud, never fake" — but a HEALTHY MCP run only logs
+# nothing, so connectivity could only be inferred from the absence of a warning.
+# REVIEW_MCP_DEBUG opt-in: thread `--debug mcp` into the MCP claude tiers and
+# surface the handshake as a ::notice::. Off by default; never alters the verdict.
+
+@test "debug: REVIEW_MCP_DEBUG set + MCP config → --debug mcp threaded into claude call" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export REVIEW_MCP_DEBUG=1
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  grep -q -- "--debug mcp" "$ARGS_RECORD"
+  grep -q -- "--mcp-config $MCP_CONFIG_FILE" "$ARGS_RECORD"
+}
+
+@test "debug: REVIEW_MCP_DEBUG set but MCP off → no --debug flag (debug rides MCP flags only)" {
+  export REVIEW_MCP_CONFIG_DEFAULT_PATH="$(mktemp -u)"  # no conventional file
+  _source_engine "claude"
+  export REVIEW_MCP_DEBUG=1
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  ! grep -q -- "--debug mcp" "$ARGS_RECORD"
+  ! grep -q -- "--mcp-config" "$ARGS_RECORD"
+}
+
+@test "debug: MCP knob set but REVIEW_MCP_DEBUG unset → no --debug flag (default unchanged)" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  ! grep -q -- "--debug mcp" "$ARGS_RECORD"
+}
+
+@test "debug: REVIEW_MCP_DEBUG set + healthy handshake in output → ::notice:: surfaced, no warning" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export REVIEW_MCP_DEBUG=1
+  export STUB_ENGINE_RESPONSE=$'MCP server "context7": Successfully connected (transport: http) in 333ms\n{"decision":"approve","summary":"mcp healthy"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"::notice::[mcp]"* ]]
+  [[ "$output" == *"context7"* ]]
+  [[ "$output" == *"Successfully connected"* ]]
+  ! [[ "$output" == *"::warning::[mcp]"* ]]
+  [[ "$output" == *'"decision":"approve"'* ]]
+}
+
+@test "debug: healthy handshake but REVIEW_MCP_DEBUG unset → no ::notice:: (opt-in only)" {
+  _source_engine "claude"
+  export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE"
+  export STUB_ENGINE_RESPONSE=$'MCP server "context7": Successfully connected (transport: http) in 333ms\n{"decision":"approve","summary":"mcp healthy"}'
+  run run_agentic "$TEST_PROMPT" "claude-opus-4-8" "deep"
+  [ "$status" -eq 0 ]
+  ! [[ "$output" == *"::notice::[mcp]"* ]]
+  [[ "$output" == *'"decision":"approve"'* ]]
+}