diff --git a/.github/workflows/daily-pr-review-health.yml b/.github/workflows/daily-pr-review-health.yml index 6586b3ea5..946c76aa1 100644 --- a/.github/workflows/daily-pr-review-health.yml +++ b/.github/workflows/daily-pr-review-health.yml @@ -100,3 +100,18 @@ jobs: if: env.HAS_FAILURES != 'true' run: | echo "::notice::Health check passed — all runs in the last ${LOOKBACK_DAYS} day(s) are healthy." + + # Durable MCP/Context7 connectivity monitor (#903, part of #676). Asserts the + # handshake through the same flags engine.sh threads into its claude tiers + # (--mcp-config --strict-mcp-config --debug mcp). No-op when MCP is + # unconfigured (no behavior change for non-MCP repos); fails the health check + # when MCP is configured but unreachable. `if: always()` keeps this monitor + # independent of the PR-review-failure analysis above so an MCP outage never + # masks (nor is masked by) that report. + - name: Assert MCP connectivity + if: always() + env: + # Reuse the #892 affirmative-diagnostics knob: a healthy handshake is + # surfaced as a ::notice::[mcp] success signal. + REVIEW_MCP_DEBUG: '1' + run: bash scripts/mcp_connectivity_check.sh diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b079d1467..38e80a6b9 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -83,7 +83,8 @@ jobs: tests/test_downstream_impact_assemble.bats \ tests/test_downstream_impact_triage_inline.bats \ tests/test_downstream_impact_regression.bats \ - tests/test_engine_unavailable_notice.bats + tests/test_engine_unavailable_notice.bats \ + tests/test_mcp_connectivity_check.bats validate-agent-profiles: runs-on: ubuntu-latest diff --git a/scripts/mcp_connectivity_check.sh b/scripts/mcp_connectivity_check.sh new file mode 100644 index 000000000..f770e3fb9 --- /dev/null +++ b/scripts/mcp_connectivity_check.sh @@ -0,0 +1,202 @@ +#!/usr/bin/env bash +# mcp_connectivity_check.sh — durable MCP/Context7 connectivity preflight (#903). +# +# The PR-review pipeline only surfaces MCP health as a fail-loud +# `::warning::[mcp]` on a *real* review (engine.sh:_emit_mcp_failure_warning) — +# a healthy run logs nothing and a silent outage is invisible until a review +# happens to run. This script is the affirmative, durable monitor: invoked by +# the daily health check, it drives the configured MCP server(s) through the +# SAME flags engine.sh threads into its claude tiers +# (`--mcp-config --strict-mcp-config --debug mcp`) and asserts the handshake +# (`MCP server "…": Successfully connected`). +# +# Behavior contract (mirrors the engine's opt-in MCP gating): +# * MCP NOT configured (no readable config) → no-op, ::notice:: skip, exit 0. +# Non-MCP repos see no behavior change. +# * MCP configured AND reachable → ::notice::[mcp] per handshake, exit 0. +# * MCP configured but UNREACHABLE → ::error::[mcp], exit 1 (fails the health +# check). Fail Loud, Never Fake. +# +# Config resolution mirrors engine.sh: an explicit REVIEW_MCP_CONFIG wins; an +# explicit empty value disables; otherwise the conventional committed path +# (REVIEW_MCP_CONFIG_DEFAULT_PATH, default .github/review-mcp.json) is used when +# it exists. +# +# Env vars consumed: +# REVIEW_MCP_CONFIG — explicit config path (honored as-is; empty disables) +# REVIEW_MCP_CONFIG_DEFAULT_PATH — conventional fallback (default .github/review-mcp.json) +# REVIEW_MCP_ALLOWED_TOOLS — explicit allowed-tools glob(s); else derived +# from the config's server names (mcp____*) +# MCP_PROBE_MODEL — claude model for the probe (default claude-sonnet-4-6) +# MCP_PROBE_TIMEOUT_SEC — hard timeout for the probe (default 120) +# CLAUDE_CODE_OAUTH_TOKEN — passed through to the claude CLI + +set -euo pipefail + +# --------------------------------------------------------------------------- +# Pure helpers (no network / no CLI — unit-tested in +# tests/test_mcp_connectivity_check.bats) +# --------------------------------------------------------------------------- + +# _mcp_resolve_config +# Echoes the config path the monitor should probe, or nothing when MCP is simply +# not configured. Precedence: explicit REVIEW_MCP_CONFIG (empty ⇒ deliberately +# disabled) > conventional committed path when present. +# +# Fail-loud distinction (this is a monitor, not the graceful-degradation engine): +# * Unset + conventional path ABSENT → not configured → echo nothing, exit 0 +# (no behavior change for non-MCP repos). +# * Explicitly pointed at a config (REVIEW_MCP_CONFIG non-empty) but the file is +# missing/unreadable → a misconfiguration the monitor must surface → ::error::, +# exit 1. Silently skipping would mask exactly the kind of silent breakage the +# monitor exists to catch. +# * Conventional path PRESENT but unreadable → same: misconfiguration → exit 1. +_mcp_resolve_config() { + local default_path="${REVIEW_MCP_CONFIG_DEFAULT_PATH:-.github/review-mcp.json}" + if [ -n "${REVIEW_MCP_CONFIG+x}" ]; then + # Explicit value set (possibly empty). Empty ⇒ MCP deliberately disabled. + [ -z "$REVIEW_MCP_CONFIG" ] && return 0 + if [ ! -f "$REVIEW_MCP_CONFIG" ] || [ ! -r "$REVIEW_MCP_CONFIG" ]; then + echo "::error::[mcp] REVIEW_MCP_CONFIG='$REVIEW_MCP_CONFIG' is set but is not a readable file" >&2 + return 1 + fi + printf '%s' "$REVIEW_MCP_CONFIG" + return 0 + fi + # No explicit override: the conventional committed path is optional. + if [ -f "$default_path" ]; then + if [ ! -r "$default_path" ]; then + echo "::error::[mcp] default MCP config '$default_path' exists but is not readable" >&2 + return 1 + fi + printf '%s' "$default_path" + fi + return 0 +} + +# _mcp_allowed_tools +# Echoes the comma-separated allowed-tools glob for the probe. Honors an explicit +# REVIEW_MCP_ALLOWED_TOOLS; otherwise derives one glob per configured server +# (mcp____*) from the config JSON. Falls back to mcp__* if no server names +# can be read (e.g. jq unavailable or odd shape) so the probe still permits MCP. +_mcp_allowed_tools() { + local cfg="$1" + if [ -n "${REVIEW_MCP_ALLOWED_TOOLS:-}" ]; then + printf '%s' "$REVIEW_MCP_ALLOWED_TOOLS" + return 0 + fi + local derived="" + if command -v jq >/dev/null 2>&1; then + derived="$(jq -r '(.mcpServers // {}) | keys[] | "mcp__\(.)__*"' "$cfg" 2>/dev/null \ + | paste -sd, - || true)" + fi + printf '%s' "${derived:-mcp__*}" +} + +# _mcp_failure_regex +# Connection/init-failure regex kept in lockstep with engine.sh:_mcp_failure_pattern. +# A degraded MCP server surfaces one of these in the CLI output even though the +# CLI itself exits 0. +_mcp_failure_regex() { + local _pat + _pat='mcp server [^[:space:]]*[[:space:]]*("[^"]*"[[:space:]]*)?(failed|error)' + _pat="$_pat"'|failed to (connect|initialize|reconnect|start)[^.]*mcp' + _pat="$_pat"'|mcp[^.]*(connection|initializ)[^.]*(fail|error)' + _pat="$_pat"'|could not (connect to|start) mcp server' + printf '%s' "($_pat)" +} + +# _mcp_assert_connected +# Verdict over captured probe output (combined stdout+stderr). Emits ::notice:: +# per successful handshake and returns 0; emits ::error:: and returns 1 when a +# failure marker is present OR no handshake line is found at all. +_mcp_assert_connected() { + local out="$1" + [ -f "$out" ] || { echo "::error::[mcp] connectivity probe produced no output" >&2; return 1; } + + if grep -qiE "$(_mcp_failure_regex)" "$out"; then + local servers + servers="$(grep -hoiE 'mcp server "[^"]+"' "$out" 2>/dev/null \ + | sed -E 's/.*"([^"]+)".*/\1/' | sort -u | paste -sd, - || true)" + if [ -n "$servers" ]; then + echo "::error::[mcp] server(s) unreachable: ${servers} — MCP is configured (review-mcp.json) but failed to connect" >&2 + else + echo "::error::[mcp] an MCP server is configured but failed to connect" >&2 + fi + return 1 + fi + + local handshakes + handshakes="$(grep -hoiE 'mcp server "[^"]+": successfully connected.*' "$out" 2>/dev/null | sort -u || true)" + if [ -z "$handshakes" ]; then + echo "::error::[mcp] no MCP handshake observed — server is configured but did not report 'Successfully connected'" >&2 + return 1 + fi + sed 's/^/::notice::[mcp] /' <<< "$handshakes" >&2 + return 0 +} + +# --------------------------------------------------------------------------- +# main — resolves config, runs the claude probe, asserts the handshake. +# --------------------------------------------------------------------------- +main() { + local cfg + # Propagate a misconfiguration (unreadable/missing explicit config) as a hard + # failure rather than letting set -e swallow it inside the substitution. + cfg="$(_mcp_resolve_config)" || return 1 + if [ -z "$cfg" ]; then + echo "::notice::[mcp] no MCP config present — connectivity assertion skipped (no behavior change for non-MCP repos)" + return 0 + fi + + local allowed_tools probe_model timeout_sec + allowed_tools="$(_mcp_allowed_tools "$cfg")" + probe_model="${MCP_PROBE_MODEL:-claude-sonnet-4-6}" + timeout_sec="${MCP_PROBE_TIMEOUT_SEC:-120}" + + echo "=== MCP connectivity preflight ===" + echo " Config: $cfg" + echo " Allowed tools: $allowed_tools" + echo " Probe model: $probe_model" + echo "" + + local out rc=0 + out="$(mktemp)" + # Clean up the capture file even if the probe is interrupted (single quotes ⇒ + # $out is expanded when the trap fires). + trap 'rm -f "$out"' EXIT + # Same flags engine.sh threads into its MCP claude tiers, plus --debug mcp so + # the handshake lands in the captured output. Combine stdout+stderr; the + # handshake is written to stderr by `--debug mcp`. A non-zero/timeout exit is + # captured and folded into the assertion (a hung connect ⇒ unreachable). + timeout "$timeout_sec" claude --print \ + --model "$probe_model" \ + --no-session-persistence \ + --max-turns 1 \ + --mcp-config "$cfg" \ + --strict-mcp-config \ + --debug mcp \ + --allowedTools "$allowed_tools" \ + -p "MCP connectivity preflight. Reply with the single word: OK." \ + > "$out" 2>&1 || rc=$? + + if [ "$rc" -ne 0 ]; then + echo "::warning::[mcp] probe CLI exited non-zero (rc=$rc) — evaluating captured output for a handshake" >&2 + fi + + # Surface the captured probe output to the Actions log for diagnosis. + echo "--- probe output ---" + cat "$out" + echo "--- end probe output ---" + + if _mcp_assert_connected "$out"; then + echo "MCP connectivity OK." + return 0 + fi + return 1 +} + +# Only run main when executed directly (not when sourced by tests). +if [ "${BASH_SOURCE[0]:-$0}" = "$0" ]; then + main "$@" +fi diff --git a/tests/test_mcp_connectivity_check.bats b/tests/test_mcp_connectivity_check.bats new file mode 100644 index 000000000..b105943aa --- /dev/null +++ b/tests/test_mcp_connectivity_check.bats @@ -0,0 +1,219 @@ +#!/usr/bin/env bats +# Tests for scripts/mcp_connectivity_check.sh — the durable MCP connectivity +# preflight (#903, part of #676). +# +# Verdict logic (_mcp_assert_connected) and config/tool resolution are pure and +# tested directly. The end-to-end main() path is exercised with a stubbed +# `claude` on PATH so no real CLI / network is needed. +# +# Run locally: bats tests/test_mcp_connectivity_check.bats + +SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" +CHECK_SCRIPT="$SCRIPT_DIR/scripts/mcp_connectivity_check.sh" + +setup() { + # shellcheck source=scripts/mcp_connectivity_check.sh + source "$CHECK_SCRIPT" + + unset REVIEW_MCP_CONFIG REVIEW_MCP_CONFIG_DEFAULT_PATH REVIEW_MCP_ALLOWED_TOOLS + + TMP="$(mktemp -d)" + export TMP + + # A readable Context7-shaped MCP config. + MCP_CONFIG="$TMP/review-mcp.json" + cat > "$MCP_CONFIG" <<'JSON' +{"mcpServers":{"context7":{"type":"http","url":"https://mcp.context7.com/mcp"}}} +JSON + + # Stub `claude` on PATH; behavior controlled by STUB_CLAUDE_OUT / STUB_CLAUDE_RC. + STUB_BIN="$TMP/bin" + mkdir -p "$STUB_BIN" + cat > "$STUB_BIN/claude" <<'STUB' +#!/usr/bin/env bash +printf '%s\n' "${STUB_CLAUDE_OUT:-}" +exit "${STUB_CLAUDE_RC:-0}" +STUB + chmod +x "$STUB_BIN/claude" + export PATH="$STUB_BIN:$PATH" +} + +teardown() { + rm -rf "$TMP" + unset REVIEW_MCP_CONFIG REVIEW_MCP_CONFIG_DEFAULT_PATH REVIEW_MCP_ALLOWED_TOOLS + unset STUB_CLAUDE_OUT STUB_CLAUDE_RC +} + +# ── _mcp_resolve_config ────────────────────────────────────────────────────── + +@test "resolve: explicit REVIEW_MCP_CONFIG to readable file is used" { + export REVIEW_MCP_CONFIG="$MCP_CONFIG" + run _mcp_resolve_config + [ "$status" -eq 0 ] + [ "$output" = "$MCP_CONFIG" ] +} + +@test "resolve: explicit empty REVIEW_MCP_CONFIG disables (no path)" { + export REVIEW_MCP_CONFIG="" + export REVIEW_MCP_CONFIG_DEFAULT_PATH="$MCP_CONFIG" # present but must be ignored + run _mcp_resolve_config + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "resolve: no env + conventional path present → conventional path used" { + export REVIEW_MCP_CONFIG_DEFAULT_PATH="$MCP_CONFIG" + run _mcp_resolve_config + [ "$status" -eq 0 ] + [ "$output" = "$MCP_CONFIG" ] +} + +@test "resolve: no env + conventional path absent → empty (MCP off)" { + export REVIEW_MCP_CONFIG_DEFAULT_PATH="$TMP/does-not-exist.json" + run _mcp_resolve_config + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "resolve: explicit path to a missing file → fail loud (status 1 + ::error::)" { + export REVIEW_MCP_CONFIG="$TMP/nope.json" + run _mcp_resolve_config + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +} + +@test "resolve: conventional path present but unreadable → fail loud (status 1)" { + [ "$(id -u)" -eq 0 ] && skip "chmod 000 is not enforced for root — readability gate is a no-op" + local cfg="$TMP/unreadable.json" + echo '{"mcpServers":{}}' > "$cfg" + chmod 000 "$cfg" + export REVIEW_MCP_CONFIG_DEFAULT_PATH="$cfg" + run _mcp_resolve_config + chmod 644 "$cfg" + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +} + +@test "main: explicit config missing → fail loud, exit 1 (no silent skip)" { + export REVIEW_MCP_CONFIG="$TMP/nope.json" + run main + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +} + +# ── _mcp_allowed_tools ─────────────────────────────────────────────────────── + +@test "tools: derived from server names as mcp____*" { + run _mcp_allowed_tools "$MCP_CONFIG" + [ "$status" -eq 0 ] + [ "$output" = "mcp__context7__*" ] +} + +@test "tools: multiple servers join comma-separated" { + local cfg="$TMP/multi.json" + echo '{"mcpServers":{"context7":{},"lsp":{}}}' > "$cfg" + run _mcp_allowed_tools "$cfg" + [ "$status" -eq 0 ] + [ "$output" = "mcp__context7__*,mcp__lsp__*" ] +} + +@test "tools: explicit REVIEW_MCP_ALLOWED_TOOLS wins over derivation" { + export REVIEW_MCP_ALLOWED_TOOLS="mcp__context7__*" + local cfg="$TMP/multi.json" + echo '{"mcpServers":{"a":{},"b":{}}}' > "$cfg" + run _mcp_allowed_tools "$cfg" + [ "$status" -eq 0 ] + [ "$output" = "mcp__context7__*" ] +} + +@test "tools: no servers in config → mcp__* fallback" { + local cfg="$TMP/empty.json" + echo '{"mcpServers":{}}' > "$cfg" + run _mcp_allowed_tools "$cfg" + [ "$status" -eq 0 ] + [ "$output" = "mcp__*" ] +} + +# ── _mcp_assert_connected ──────────────────────────────────────────────────── + +@test "assert: healthy handshake → ::notice::, exit 0" { + local out="$TMP/o" + printf 'MCP server "context7": Successfully connected (transport: http) in 333ms\nOK\n' > "$out" + run _mcp_assert_connected "$out" + [ "$status" -eq 0 ] + [[ "$output" == *"::notice::[mcp]"* ]] + [[ "$output" == *"context7"* ]] + [[ "$output" == *"Successfully connected"* ]] +} + +@test "assert: failure marker → ::error:: naming server, exit 1" { + local out="$TMP/o" + printf 'MCP server "context7" failed to connect after 3 attempts\n' > "$out" + run _mcp_assert_connected "$out" + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] + [[ "$output" == *"context7"* ]] +} + +@test "assert: no handshake at all → ::error::, exit 1" { + local out="$TMP/o" + printf 'some unrelated CLI output with no mcp handshake\n' > "$out" + run _mcp_assert_connected "$out" + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] + [[ "$output" == *"no MCP handshake"* ]] +} + +@test "assert: handshake present but a later failure line → ::error:: (fail loud)" { + local out="$TMP/o" + printf 'MCP server "a": Successfully connected\nMCP server "b" failed to connect\n' > "$out" + run _mcp_assert_connected "$out" + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +} + +# ── main (stubbed claude) ──────────────────────────────────────────────────── + +@test "main: MCP unconfigured → skip notice, exit 0 (no claude call)" { + export REVIEW_MCP_CONFIG_DEFAULT_PATH="$TMP/does-not-exist.json" + run main + [ "$status" -eq 0 ] + [[ "$output" == *"::notice::[mcp]"* ]] + [[ "$output" == *"connectivity assertion skipped"* ]] +} + +@test "main: configured + healthy handshake → exit 0" { + export REVIEW_MCP_CONFIG="$MCP_CONFIG" + export STUB_CLAUDE_OUT='MCP server "context7": Successfully connected (transport: http) in 333ms' + run main + [ "$status" -eq 0 ] + [[ "$output" == *"::notice::[mcp]"* ]] + [[ "$output" == *"MCP connectivity OK"* ]] +} + +@test "main: configured but unreachable → exit 1" { + export REVIEW_MCP_CONFIG="$MCP_CONFIG" + export STUB_CLAUDE_OUT='MCP server "context7" failed to connect after 3 attempts' + run main + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +} + +@test "main: configured, claude exits non-zero but handshake present → exit 0" { + export REVIEW_MCP_CONFIG="$MCP_CONFIG" + export STUB_CLAUDE_OUT='MCP server "context7": Successfully connected (transport: http) in 90ms' + export STUB_CLAUDE_RC=1 + run main + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::[mcp]"* ]] # non-zero rc surfaced + [[ "$output" == *"MCP connectivity OK"* ]] +} + +@test "main: configured, claude exits non-zero with no handshake → exit 1" { + export REVIEW_MCP_CONFIG="$MCP_CONFIG" + export STUB_CLAUDE_OUT='error: unknown flag' + export STUB_CLAUDE_RC=2 + run main + [ "$status" -eq 1 ] + [[ "$output" == *"::error::[mcp]"* ]] +}