diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index aa932d959..9a2d82641 100644 --- a/.github/workflows/pr-review.yml +++ b/.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 diff --git a/scripts/engine.sh b/scripts/engine.sh index 84446108f..bc151feee 100644 --- a/scripts/engine.sh +++ b/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 diff --git a/tests/dev-lead/unit/test_engine_mcp.bats b/tests/dev-lead/unit/test_engine_mcp.bats index fd2af4ade..181796966 100644 --- a/tests/dev-lead/unit/test_engine_mcp.bats +++ b/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"'* ]] +}