From 69d4bf7e6f15197d1d5daf0a561599029be2ccd8 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Wed, 19 Aug 2026 01:30:20 +0000 Subject: [PATCH 1/2] fix(hooks): decide the connector verbs by tool suffix, not by server name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A permission rule in `.claude/settings.json` must name its MCP server as a literal — `mcp-allow-check` refuses a glob there, 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, a UUID after a reconnect (CLOUD-178). The literal that would match the second is an account-specific identifier that non-negotiable rule 1 keeps out of committed config. So the four PR-subscription rules added in d671184 are half inert. Measured on that commit: the session that wrote them had the two connectors exposed as `mcp__bf7c680d-…__*` and `mcp__cc451d34-…__*`, so three of the four denies matched nothing, and `…__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. CLOUD-178 concluded from this that no gate is possible. 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, so they work under either name with no account identifier in the repo. `mise-tasks/connector-verb-guard` applies that idiom to the verbs AGENTS.md already rules on — `unsubscribe_pr_activity` pre-approved so the drop `land` requires never waits on a person, and `subscribe_pr_activity`, `send_later` and `create_trigger` denied as the no-heartbeat rule already says. It emits `permissionDecision: "allow"`, which the engine has no representation for (`Decision::Allow` means "emit nothing"), so it is a by-path guard rather than a `batten.toml` row — a capability gap for CLOUD-312's ledger. `mcp-allow-check` gains the predicate that would have caught the inert rules at commit time: a deny naming one host-supplied server and one tool, both literally, must be covered by a suffix-matching guard. Scoped to denies because the two directions fail differently — an under-matching allow fails closed into a prompt a human sees, an under-matching deny fails open and silently. Coverage is declared by each guard's `--covers` rather than parsed out of its `case` statements, so there is one authority per fact. Two bugs found while building it, both by the repo's own gates rather than by review: probing `--covers` across every `*-guard` hung the whole suite at the two-minute kill, because a guard that does not know the flag falls through to `raw=$(cat)` and blocks on stdin; and two `printf | grep -q` pairs were the early-exit-under-pipefail shape `pipefail-grep-check` exists to refuse. Refs: CLOUD-178, CLOUD-518 --- .claude/settings.json | 9 ++ fuzz/Cargo.lock | 2 +- mise-tasks/connector-verb-guard | 151 ++++++++++++++++++++++++++++++++ mise-tasks/mcp-allow-check | 86 +++++++++++++++++- tests/connector-verb-guard.bats | 151 ++++++++++++++++++++++++++++++++ tests/mcp-allow-check.bats | 54 ++++++++++++ 6 files changed, 448 insertions(+), 5 deletions(-) create mode 100755 mise-tasks/connector-verb-guard create mode 100644 tests/connector-verb-guard.bats 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/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 ] +} From 42a73ae8245ded1ecf09d5e555b20854b448171f Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Wed, 19 Aug 2026 01:34:54 +0000 Subject: [PATCH 2/2] docs(plan-fleet): plan mode is a property of the dispatch, not a default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 4 prescribed `permission_mode: "plan"` on every `create_session` and justified it as "also this environment's default". Both halves are false. `create_session` inherits the caller's mode — the tool contract says "omit to inherit it" — and dispatchers on this account run `auto`, so two children dispatched with the parameter omitted came up `PERMISSION_MODE_AUTO`. The 2026-08-11 measurement in `mem:workflow/agent-fanout` was taken from a dispatcher already in plan mode: it measured inheritance and recorded it as an environment property. That memory's bound — "only a defect for work nobody intends to approve" — is the normal case rather than the edge. One child parked 91 minutes on `AskUserQuestion`, not `ExitPlanMode`, for no commit, branch or PR; the same ticket re-dispatched without the parameter reached a draft PR. Attended, plan mode is still the cheapest review point available: the five CLOUD-607 bundles dispatched under it all reached review_ready with the owner approving in the web UI. So the criterion is a property of the dispatch, not of the ticket, and it now sits where the dispatch is made. The memory keeps the false measurement and supersedes it in place, in the file's existing style, so the next reader meets the correction alongside the claim rather than the claim alone. Step 4's per-ticket loop is made conditional in the same change: it told every child to plan "and wait for approval", which under a fire-and-forget dispatch is precisely the stall documented above. Refs: CLOUD-672 --- .claude/commands/plan-fleet.md | 23 ++++++++++++++++------ .serena/memories/workflow/agent-fanout.md | 24 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) 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/.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