diff --git a/.claude/commands/plan-fleet.md b/.claude/commands/plan-fleet.md index 6bf957f8a..d8af23ee6 100644 --- a/.claude/commands/plan-fleet.md +++ b/.claude/commands/plan-fleet.md @@ -99,13 +99,22 @@ all of them things a child must be told:** Call the Claude Code Remote `create_session` tool with `source_url: https://github.com/button-inc/batten`, `model: claude-opus-5`, -`permission_mode: "plan"`, tags `["batten-bundle", "ready-queue-"]`, +tags `["batten-bundle", "ready-queue-"]`, and a title of the form `BUNDLE — CLOUD- → CLOUD-`. -**Plan mode is correct here and is also this environment's default.** The child -plans, a human approves in the web UI, and it then works on. Its one failure -mode is a child nobody intends to approve — that stalls indefinitely, so only -omit it when dispatching genuinely fire-and-forget work. +**Whether to pass `permission_mode: "plan"` is a property of the DISPATCH, not +of the bundle.** Pass it when a human is standing by to approve in the web UI; +omit it when the dispatch is fire-and-forget. The same bundle takes opposite +answers depending on which is true, so the choice is made per dispatch and not +once per ticket. + +It is **not** a default, and **not** this environment's: `create_session` +inherits the caller's mode, and dispatchers here run `auto`, so omitting it +yields `PERMISSION_MODE_AUTO`. Unattended, plan mode is not a free extra gate — +one child parked 91 minutes on `AskUserQuestion` for no commit, branch or PR. +Attended it is the cheapest review point there is: the five CLOUD-607 bundles +dispatched in plan mode with the owner approving all reached `review_ready`. +Reasoning in `mem:workflow/agent-fanout`; measurements on CLOUD-672. Reasoning effort is **not** a `create_session` parameter; children inherit the dispatching session's. Dispatch from a session at the effort you want. @@ -121,7 +130,9 @@ Each prompt is standalone — the child starts from nothing — and carries: `mem:workflow/board-states`, the `.claude/rules/` file matching the surface, and any house-style section that governs the surface it touches. 4. **The per-ticket loop**, verbatim in shape: `claim-check` → claim → plan - **this ticket only** and wait for approval → build, `verify`, `linear-check`, + **this ticket only** — waiting for approval only if this was a plan-mode + dispatch, since an unattended child told to wait never proceeds (CLOUD-672) + → build, `verify`, `linear-check`, draft PR, `land` backgrounded → next ticket. For a bundle dispatched under the one-PR shape (step 3b), the loop is `claim-check` → claim → plan **this ticket only** → build → commit → next ticket, with the draft PR, `verify`, diff --git a/.claude/settings.json b/.claude/settings.json index 395aeb468..c7fe81f6f 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -67,6 +67,15 @@ "command": "$CLAUDE_PROJECT_DIR/mise-tasks/board-move-guard" } ] + }, + { + "matcher": ".*(subscribe_pr_activity|send_later|create_trigger)", + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/mise-tasks/connector-verb-guard" + } + ] } ], "PostToolUse": [ diff --git a/.serena/memories/workflow/agent-fanout.md b/.serena/memories/workflow/agent-fanout.md index b2677c24f..75c3c6be3 100644 --- a/.serena/memories/workflow/agent-fanout.md +++ b/.serena/memories/workflow/agent-fanout.md @@ -242,6 +242,30 @@ invisible until they bite: ExitPlanMode". That park is the feature — the child plans, a human approves, and it works on from there. It is only a defect for work nobody intends to approve, which then stalls forever. Pass it explicitly so the intent is legible. + + **Both halves are false. Superseded 2026-08-18 (CLOUD-672).** `create_session` + inherits the **caller's** mode — the tool contract says "omit to inherit it" — + and dispatchers on this account run `auto`: two children dispatched with no + `permission_mode` came up `PERMISSION_MODE_AUTO`, not plan. The 2026-08-11 + reading was taken from a dispatcher already in plan mode, so it measured + inheritance and recorded it as an environment property. The "only a defect for + work nobody intends to approve" bound is the normal case, not the edge: one + child parked **91 minutes** on `AskUserQuestion` — not `ExitPlanMode` — for + zero commits, branches or PRs, and the same ticket re-dispatched without the + parameter reached a draft PR. Plan mode does not add one gate at the end; it + raises the child's propensity to hand control back at all. + + **The criterion is a property of the DISPATCH, not the ticket:** pass it when a + human is standing by to approve, omit it when the dispatch is fire-and-forget. + The same bundle takes opposite answers — plan mode was _correct_ for the five + CLOUD-607 BUNDLE children on 2026-08-14, all of which reached `review_ready` + with the owner approving in the web UI. That is the criterion working, not an + exception to it. + + **Unmeasured, so do not lean on "works on from there":** one child declined a + write at end-of-session citing plan mode, hours after its plan was approved. + Whether the mode persists past approval or was re-entered is not established. + - **Reasoning effort is not a `create_session` parameter.** Children inherit the dispatcher's, so dispatch from a session at the effort you want them to run at. - **In-process subagents are the wrong tool here, for a reason unrelated to diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index c7128784b..fa91b0238 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -87,7 +87,7 @@ checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" [[package]] name = "batten" -version = "0.0.80" +version = "0.0.81" dependencies = [ "anyhow", "clap", diff --git a/mise-tasks/connector-verb-guard b/mise-tasks/connector-verb-guard new file mode 100755 index 000000000..ac41d83fd --- /dev/null +++ b/mise-tasks/connector-verb-guard @@ -0,0 +1,151 @@ +#!/usr/bin/env bash +#MISE description="PreToolUse hook body: decide the harness-connector verbs AGENTS.md already rules on, by tool-name SUFFIX, so the verdict survives the exposed server name changing (CLOUD-178)" +# +# CLOUD-178. A `.claude/settings.json` permission rule must name its server as a +# LITERAL — `mcp-allow-check` refuses a glob in that segment, correctly, because +# the CLI skips such a rule with a warning and it grants nothing. But a claude.ai +# connector's exposed name is chosen per registration episode by the HOST: +# readable at session start (`mcp__Linear__save_issue`), a UUID after a reconnect +# (`mcp__4db58e41-…__save_issue`). The literal that would match the second is an +# account-specific identifier, which non-negotiable rule 1 keeps out of committed +# config. +# +# So a permission rule cannot express these verdicts portably. CLOUD-178 concluded +# from that "no gate is possible for this one". That conclusion was wrong, and the +# counter-example was already shipping beside it: `issue-read-guard` and +# `board-move-guard` match `*save_issue` on the SUFFIX and never read the server +# segment at all. This file is that idiom applied to the verbs AGENTS.md rules on. +# +# MEASURED 2026-08-19, and it is why this exists rather than a fifth permission +# rule. `.claude/settings.json` on `main` allows `mcp__Claude_Code_Remote__*` and +# `mcp__Linear__*`; in the session that landed it the host exposed those same two +# connectors as `mcp__bf7c680d-…__*` and `mcp__cc451d34-…__*`, so +# `…__get_session` and `…__list_comments` both answered "requires approval" from +# inside an allow list that reads as covering them. A second literal spelling is +# still a literal. +# +# WHAT IT DECIDES, and every one is an existing AGENTS.md rule rather than a new +# one this file invents: +# +# unsubscribe_pr_activity ALLOW — dropping a subscription only ever narrows +# what reaches the session, and `land`'s +# `pr-unsubscribed` gate REQUIRES the drop +# before a runner is spent. An enforced path +# that prompts on every PR is a path that +# gets worked around instead of satisfied. +# subscribe_pr_activity DENY — "No heartbeats … to babysit a PR." +# send_later DENY — "No heartbeats (`send_later`/Routines/ +# create_trigger DENY timers) to babysit a PR … the ban is on +# timers." +# +# The last two already sit in `permissions.deny` under a literal server name, and +# that is precisely the shape measured inert above: a deny that under-matches +# reads as a prohibition and enforces NOTHING. An allow that under-matches fails +# the other way — into a prompt a human sees — which is why `mcp-allow-check`'s +# new predicate binds denies and not allows. +# +# THE SUBSTRING HAZARD, AND WHICH DEFENCE ACTUALLY CARRIES IT. +# `unsubscribe_pr_activity` ENDS WITH `subscribe_pr_activity`, so an unbounded +# `*subscribe_pr_activity` test matches BOTH and denies the very call this file +# exists to free — identical to the hazard `closing-key-check` records against +# its own `DO-NOT-CLOSE` marker, where "the opt-out was unusable exactly when it +# was written correctly, and it failed as the inverse of the author's intent +# rather than as a refusal, which is the silent direction." +# +# Two independent defences, and the first is the structural one: the deny arm is +# LEFT-BOUNDED on `__`, so `…__unsubscribe_pr_activity` cannot match +# `*__subscribe_pr_activity` at all — the boundary character is `u`, not `s`. +# Testing the allow arm FIRST is the second, and it is redundancy rather than the +# mechanism. Said plainly because an earlier draft of this header claimed the +# ordering was what carried it, and the mutation run disproved that: with the +# allow arm deleted the unsubscribe rows fail by going UNDECIDED, never by being +# denied. Neither defence alone is observable through the other, which is why the +# suite carries a row for the bound (`…_v2`, `resubscribe_…`) as well as the +# ordering. +# +# ALLOW IS A DECISION HERE, not an omission, and it is why this is a by-path guard +# rather than a `batten.toml` row. The engine models `Decision::Allow` as "emit +# nothing, exit 0", which leaves the ordinary permission flow to prompt. Actually +# pre-approving needs `hookSpecificOutput.permissionDecision: "allow"` on stdout, +# a variant the engine has no representation for. That is a capability gap on +# CLOUD-312's ledger — a missing decision variant, not a missing rule kind. +# +# Pointer-only (non-negotiable 4): the decision names the verb and the rule, never +# the payload. +# +# `--covers` prints the tool suffixes this file decides, one per line, so +# `mcp-allow-check` can bind a deny rule to its coverage without grepping this +# file's `case` statements. One authority per fact. +# +# Exit 0 always — a guard renders its verdict in the JSON, never in the status. +# FAILS OPEN on everything it cannot establish, and on BATTEN_CONNECTOR_VERB_BYPASS=1. +# +# The mutation drops the allow arm, so an unsubscribe falls through UNDECIDED and +# the approval prompt this file exists to remove comes back. Every deny row still +# passes under it — only the unsubscribe rows can catch it, which is what makes +# them the suite's spine and why they are written under both spellings. +#MUTANT unsubscribe-not-preapproved|s@^ALLOW_SUFFIXES=.*@ALLOW_SUFFIXES=""@|an unsubscribe must be pre-approved, not merely left to the permission flow +set -uo pipefail + +ALLOW_SUFFIXES="unsubscribe_pr_activity" +DENY_SUFFIXES="subscribe_pr_activity send_later create_trigger" + +if [ "${1:-}" = "--covers" ]; then + for suffix in $ALLOW_SUFFIXES $DENY_SUFFIXES; do + printf '%s\n' "$suffix" + done + exit 0 +fi + +# CLOUD-479's pairing, declared rather than discovered: registered BY PATH, so +# mise's `"aqua:jqlang/jq"` pin does not reach it and `jq` is whatever the ambient +# PATH holds. Every read below fails open, so an absent parser would ALLOW +# silently — the one outcome a guard must never reach by accident. Say so on +# stderr and get out of the way; exit 2 is a DENY on this event and a broken +# toolchain must not take that channel. +#PIN-OK: jq +if ! command -v jq >/dev/null 2>&1; then + echo "::error:: connector-verb-guard: no jq on PATH — this guard is registered by path, so it does not get mise's pinned jq. It is checking NOTHING and allowing every call. Run: mise install" >&2 + exit 0 +fi + +[ -n "${BATTEN_CONNECTOR_VERB_BYPASS:-}" ] && exit 0 + +raw=$(cat) || exit 0 + +tool=$(printf '%s' "$raw" | jq -r '.tool_name // empty' 2>/dev/null) || exit 0 +[ -n "$tool" ] || exit 0 + +decide() { + jq -n --arg d "$1" --arg r "$2" '{ + hookSpecificOutput: { + hookEventName: "PreToolUse", + permissionDecision: $d, + permissionDecisionReason: $r + } +}' + exit 0 +} + +# ALLOW FIRST. See the order note above: the deny suffix is a substring of this +# one, so testing the other way round denies what this exists to permit. +for suffix in $ALLOW_SUFFIXES; do + case "$tool" in + *__"$suffix" | "$suffix") + decide allow "connector-verb-guard: $suffix is pre-approved. Dropping a PR subscription only narrows what reaches this session, and \`mise run land\` refuses to land a PR whose subscription is still armed — so this must never wait on an approval. Record the drop afterwards: mise run pr-unsubscribed record ." + ;; + esac +done + +# Left-bounded on `__` so it cannot reach back across the word boundary into +# `unsubscribe_pr_activity`. The bare-name arm is for a host that exposes a verb +# with no server prefix at all. +for suffix in $DENY_SUFFIXES; do + case "$tool" in + *__"$suffix" | "$suffix") + decide deny "connector-verb-guard: $suffix is denied by AGENTS.md — no heartbeats (send_later/Routines/timers) and no babysitting a PR. Landing is driven by \`mise run land\`, which laps fetch -> rebase -> verify -> push -> ci-wait -> fast-forward by itself; a webhook's silence is not success. Bypass with BATTEN_CONNECTOR_VERB_BYPASS=1 when a human has asked for this specific call." + ;; + esac +done + +exit 0 diff --git a/mise-tasks/mcp-allow-check b/mise-tasks/mcp-allow-check index 6b39aae94..4bc71b270 100755 --- a/mise-tasks/mcp-allow-check +++ b/mise-tasks/mcp-allow-check @@ -5,7 +5,7 @@ # symptom is an approval prompt on every call, which reads as harness behaviour # rather than as a settings bug. That silence is what earns a gate. # -# Two predicates, both pure functions of the settings file: +# Three predicates, all pure functions of the settings file: # # unanchored-allow-glob an allow rule whose glob reaches the server # segment. The CLI accepts a tool-name glob only @@ -24,8 +24,16 @@ # mandates stalled behind an approval prompt this # repo had already decided to give. # -# Deny rules are deliberately not checked: a deny may glob freely, and a deny -# that over-matches fails closed rather than silently. +# uncovered-connector-deny a deny naming ONE host-supplied server and ONE +# tool, both literally, that no suffix-matching +# guard covers. Detail and the measurement are +# beside the predicate at the foot of this file. +# +# A deny may still glob freely — `mcp__*` is a broad prohibition rather than a +# misspelled narrow one, and a deny that OVER-matches fails closed. What the +# third predicate added is the opposite direction: a deny that UNDER-matches, +# because it spells a connector name the host has since changed, fails open and +# silently. That distinction is the whole of its scope. # # Reads no network and no live MCP state — the settings file is the whole input, # so the same command answers the same way in a hook, on CI, and in a test. @@ -110,5 +118,75 @@ done < <(jq -r '(.enabledMcpjsonServers // empty) | if type == "array" then .[] # A gate may only assert what it can verify from the repo. The connector-name # question cannot be, and belongs in user-level settings — see CLOUD-178. -[ "$fail" = 0 ] && echo "mcp-allow-check: every allow rule in $settings names a tool it can match, and every enabled server has a grant" +# --- an under-matching DENY, which is the direction that fails open ----------- +# +# The paragraph above still holds: this gate cannot know which name a connector +# is live under. What it CAN decide from the repo alone is whether a rule is +# being RELIED ON for a server the repo never declares. +# +# MEASURED 2026-08-19, on this gate's own settings file at `d671184`. Four deny +# rules named `mcp__Claude_Code_Remote__*` and `mcp__github__*`; the session that +# wrote them had those connectors exposed as `mcp__bf7c680d-…__*` and +# `mcp__cc451d34-…__*`, so three of the four matched nothing. The gate passed, +# because every rule was well-SHAPED. A deny that names no live tool reads as a +# prohibition and enforces nothing. +# +# WHY DENIES AND NOT ALLOWS, which is the whole of the new predicate's scope. The +# two fail in opposite directions. An under-matching ALLOW fails closed — the call +# stops at an approval prompt, a human sees it, and the compensating control +# CLOUD-178 already records ("the failure is loud to the human even though it is +# silent to the agent") applies. An under-matching DENY fails OPEN, silently, and +# no one is prompted about a prohibition that quietly stopped applying. Only the +# second needs a gate, and scoping to it is what lets this land without a waiver +# for every pre-existing allow rule. +# +# COVERAGE IS DECLARED, NEVER PARSED. A guard that decides a verb by tool-name +# SUFFIX works under any exposed server name, which is the portable form of this +# rule — `issue-read-guard` and `board-move-guard` have matched `*save_issue` that +# way all along. Each such guard prints the suffixes it owns via `--covers`, and +# this gate takes the union. Grepping a neighbour's `case` statements instead +# would be a second authority for a fact the guard already states. +# +# A server the repo DOES declare (`.mcp.json` / `enabledMcpjsonServers`) keeps a +# stable name by construction, so its deny rules need no guard. +declared=$( ( + jq -r '(.enabledMcpjsonServers // empty) | if type == "array" then .[] else empty end' "$settings" 2>/dev/null + jq -r '(.mcpServers // {}) | keys[]' .mcp.json 2>/dev/null +) | sort -u) + +# STDIN IS CLOSED FOR THE PROBE, and that is not a tidiness detail. A guard that +# does not know `--covers` falls through to its `raw=$(cat)` and blocks forever +# waiting for a payload that will never come — this gate hung the whole suite +# exactly once, at the two-minute harness kill, before the redirect was added. +# The output is filtered to bare tool-name shapes so a guard that answers with a +# usage line or an `::error::` cannot inject a spurious "coverage". +covered=$(for guard in "$(dirname "$0")"/*-guard; do + [ -x "$guard" ] || continue + "$guard" --covers /dev/null || true +done | grep -E '^[a-z][a-z0-9_]*$' | sort -u) + +while IFS= read -r rule; do + [ -n "$rule" ] || continue + case "$rule" in + mcp__*) ;; + *) continue ;; + esac + rule_server=${rule#mcp__} + rule_server=${rule_server%%__*} + suffix=${rule##*__} + # A GLOB IN EITHER SEGMENT IS THE OTHER QUESTION, and the header above already + # answers it: "a deny may glob freely". `mcp__*` is a broad prohibition, not a + # misspelled narrow one, and there is no suffix for a guard to cover. The + # predicate here is only about a deny that names ONE server and ONE tool, both + # literally — the shape that looks precise and silently addresses nothing. + case "$rule_server$suffix" in + *'*'*) continue ;; + esac + # Declared server: the name is the repo's own and cannot drift under it. + grep -qxF "$rule_server" <<<"$declared" && continue + grep -qxF "$suffix" <<<"$covered" && continue + report "$rule — a deny on a host-supplied connector, whose exposed name the host chooses per registration episode (CLOUD-178). No suffix-matching guard covers \`$suffix\`, so this rule reads as a prohibition and enforces nothing the moment the connector comes back under a different name." +done < <(jq -r '[.permissions.deny // [] | .[] | select(type == "string")] | .[]' "$settings") + +[ "$fail" = 0 ] && echo "mcp-allow-check: every allow rule in $settings names a tool it can match, every enabled server has a grant, and every deny on a host-supplied connector is backed by a suffix-matching guard" exit "$fail" diff --git a/tests/connector-verb-guard.bats b/tests/connector-verb-guard.bats new file mode 100644 index 000000000..ebacf384b --- /dev/null +++ b/tests/connector-verb-guard.bats @@ -0,0 +1,151 @@ +#!/usr/bin/env bats +# CLOUD-178. The guard that decides the harness-connector verbs by tool-name +# SUFFIX, so the verdict survives the exposed server name changing. +# +# THE SPINE OF THIS SUITE IS THE UUID ROWS. A permission rule in +# `.claude/settings.json` names its server as a literal, and the host re-exposes a +# connector under a UUID after a reconnect — measured on `main` at `d671184`, +# where `mcp__Claude_Code_Remote__*` grants sat inert while the live tools were +# `mcp__bf7c680d-…__*`. A suite that only exercises the readable spelling grades +# the case that already worked and says nothing about the one that did not. +# +# THE SECOND SPINE IS THE UN/SUBSCRIBE DISCRIMINATION. +# `unsubscribe_pr_activity` ends with `subscribe_pr_activity`, so the natural +# implementation denies the call the guard exists to permit — the +# `closing-key-check` / `DO-NOT-CLOSE` hazard, which failed as the inverse of its +# author's intent rather than as a refusal. The `#MUTANT` on the guard drops the +# allow arm precisely so those rows must go red. + +setup() { + GUARD="$BATS_TEST_DIRNAME/../mise-tasks/connector-verb-guard" +} + +# The payload goes through a FILE and the helper prints its path — embedding JSON +# into a `bash -c` string lets the shell reinterpret its braces before the guard +# sees it, which `issue-search-guard.bats` records as having failed nine of +# fifteen rows there. +payload() { + jq -nc --arg t "$1" '{tool_name: $t}' >"$BATS_TEST_TMPDIR/payload.json" + printf '%s\n' "$BATS_TEST_TMPDIR/payload.json" +} + +decision() { + run bash -c "'$GUARD' <'$(payload "$1")'" + [ "$status" -eq 0 ] + # No output at all is the third verdict: the guard got out of the way and the + # ordinary permission flow decides. + if [ -z "$output" ]; then + printf 'none\n' + return 0 + fi + jq -r '.hookSpecificOutput.permissionDecision' <<<"$output" +} + +# --- the verbs, under the readable spelling ----------------------------------- + +@test "an unsubscribe under the readable name is pre-approved" { + [ "$(decision mcp__github__unsubscribe_pr_activity)" = allow ] +} + +@test "a subscribe under the readable name is denied" { + [ "$(decision mcp__github__subscribe_pr_activity)" = deny ] +} + +@test "send_later under the readable name is denied" { + [ "$(decision mcp__Claude_Code_Remote__send_later)" = deny ] +} + +@test "create_trigger under the readable name is denied" { + [ "$(decision mcp__Claude_Code_Remote__create_trigger)" = deny ] +} + +# --- the same verbs under a UUID: the rows a permission rule cannot reach ------ + +@test "an unsubscribe under a UUID server name is pre-approved" { + [ "$(decision mcp__bf7c680d-5fdc-5ef4-b4a0-abadb619bf0a__unsubscribe_pr_activity)" = allow ] +} + +@test "a subscribe under a UUID server name is denied" { + [ "$(decision mcp__bf7c680d-5fdc-5ef4-b4a0-abadb619bf0a__subscribe_pr_activity)" = deny ] +} + +@test "send_later under a UUID server name is denied" { + [ "$(decision mcp__bf7c680d-5fdc-5ef4-b4a0-abadb619bf0a__send_later)" = deny ] +} + +# --- the discrimination itself ------------------------------------------------ + +@test "the unsubscribe suffix is not swallowed by the subscribe suffix" { + # Stated as its own row rather than left implicit in the rows above: this is + # the one property the mutation removes, and it must fail loudly when it does. + [ "$(decision mcp__github__unsubscribe_pr_activity)" = allow ] + [ "$(decision mcp__github__subscribe_pr_activity)" = deny ] +} + +@test "a verb with no server prefix is still decided" { + [ "$(decision unsubscribe_pr_activity)" = allow ] + [ "$(decision subscribe_pr_activity)" = deny ] +} + +@test "a tool merely CONTAINING a decided verb is not decided" { + # Left-bounded on `__`: a host tool called `resubscribe_pr_activity_v2` or + # `describe_pr_activity` is somebody else's question, and a guard that + # swallowed it would be the over-match that gets a guard switched off. + [ "$(decision mcp__github__subscribe_pr_activity_v2)" = none ] + [ "$(decision mcp__github__resubscribe_pr_activity)" = none ] +} + +# --- everything it cannot establish, and the bypass --------------------------- + +@test "an unrelated tool gets no decision" { + [ "$(decision mcp__Linear__save_issue)" = none ] + [ "$(decision Bash)" = none ] +} + +@test "a payload with no tool_name gets no decision" { + printf '{}' >"$BATS_TEST_TMPDIR/empty.json" + run bash -c "'$GUARD' <'$BATS_TEST_TMPDIR/empty.json'" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "unparseable stdin gets no decision rather than a deny" { + printf 'not json at all' >"$BATS_TEST_TMPDIR/junk.json" + run bash -c "'$GUARD' <'$BATS_TEST_TMPDIR/junk.json'" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "the bypass silences every arm, including the allow" { + run bash -c "BATTEN_CONNECTOR_VERB_BYPASS=1 '$GUARD' <'$(payload mcp__github__subscribe_pr_activity)'" + [ "$status" -eq 0 ] + [ -z "$output" ] + run bash -c "BATTEN_CONNECTOR_VERB_BYPASS=1 '$GUARD' <'$(payload mcp__github__unsubscribe_pr_activity)'" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +# --- the coverage declaration `mcp-allow-check` binds to ---------------------- + +@test "--covers prints every suffix the guard decides, and nothing else" { + run "$GUARD" --covers + [ "$status" -eq 0 ] + [ "$(printf '%s\n' "$output" | sort)" = "$(printf 'create_trigger\nsend_later\nsubscribe_pr_activity\nunsubscribe_pr_activity\n' | sort)" ] +} + +@test "every deny rule in the committed settings names a covered suffix" { + # The pairing `mcp-allow-check` enforces, asserted here too so the guard's own + # suite fails if a deny rule is added to the settings without coverage — the + # defect this whole change exists to stop being silent. + covers=$("$GUARD" --covers) + while IFS= read -r rule; do + [ -n "$rule" ] || continue + suffix=${rule##*__} + grep -qxF "$suffix" <<<"$covers" || + { + echo "deny rule $rule names suffix $suffix, which no guard covers" >&2 + return 1 + } + done < <(jq -r '[.permissions.deny // [] | .[] | select(startswith("mcp__"))] | .[]' \ + "$BATS_TEST_DIRNAME/../.claude/settings.json") +} diff --git a/tests/mcp-allow-check.bats b/tests/mcp-allow-check.bats index a59a5373d..bd9b23d35 100644 --- a/tests/mcp-allow-check.bats +++ b/tests/mcp-allow-check.bats @@ -138,3 +138,57 @@ enabled() { [ "$status" -eq 0 ] [[ "$output" == *"nothing to check"* ]] } + +# --- the third predicate: an under-matching DENY (CLOUD-178) ------------------ +# +# Added after the measured defect on `d671184`: four deny rules, three of them +# naming a server the host had re-exposed under a UUID, and this gate passed them +# all because every rule was well-SHAPED. A deny that names no live tool reads as +# a prohibition and enforces nothing. + +# Writes a fixture whose permissions.deny is the given JSON array. +denies() { + printf '{"permissions":{"allow":[],"deny":%s}}\n' "$1" >"$FIXTURE" +} + +@test "a deny on a host-supplied connector with no guard coverage fails" { + denies '["mcp__Claude_Code_Remote__archive_session"]' + run "$GATE" "$FIXTURE" + [ "$status" -eq 1 ] + [[ "$output" == *"archive_session"* ]] + [[ "$output" == *"enforces nothing"* ]] +} + +@test "a deny whose suffix a guard covers passes under any server spelling" { + # The point of the predicate: the same verb under the readable name and under + # a UUID are one rule as far as coverage is concerned, because the guard that + # backs them never reads the server segment. + denies '["mcp__Claude_Code_Remote__send_later","mcp__bf7c680d-5fdc-5ef4-b4a0-abadb619bf0a__send_later"]' + run "$GATE" "$FIXTURE" + [ "$status" -eq 0 ] +} + +@test "a deny on a server the repo itself declares needs no guard" { + # `.mcp.json` and enabledMcpjsonServers are the repo's own declarations, so + # those names cannot drift under it — the predicate must not demand coverage + # it has no reason to want. + printf '{"enabledMcpjsonServers":["serena"],"permissions":{"allow":["mcp__serena__*"],"deny":["mcp__serena__delete_memory"]}}\n' >"$FIXTURE" + run "$GATE" "$FIXTURE" + [ "$status" -eq 0 ] +} + +@test "an under-matching ALLOW is deliberately not failed" { + # The asymmetry that scopes this predicate: an allow that matches nothing + # fails CLOSED, into an approval prompt a human sees. Only the deny fails + # open and silently. Asserted so a later author does not "improve" the gate by + # symmetry and make it unlandable against every pre-existing grant. + allow '["mcp__Claude_Code_Remote__get_session"]' + run "$GATE" "$FIXTURE" + [ "$status" -eq 0 ] +} + +@test "a non-MCP deny is not this predicate's business" { + denies '["Bash(rm -rf *)"]' + run "$GATE" "$FIXTURE" + [ "$status" -eq 0 ] +}