From f8d04d1bf4c37960c2c4725ae006011f41769c2e Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 13 Aug 2026 01:57:07 -0500 Subject: [PATCH] wrapper-guard: decide allows on the shell's reading, not on the whole command text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four blockers from adversarial review, and three are one defect wearing three hats: a test over the WHOLE command text deciding an ALLOW. That is the fail-open shape this file keeps rediscovering, and it had reached the break-glass itself. - Break-glass read POSITIONALLY. `case $CMD in *MOSAIC_WRAPPER_OVERRIDE=1*)` cleared the entire command if that string appeared anywhere in it, so quoting the override in a note, naming a variable after it, or writing =10 disabled the guard for the call sitting beside it. Now only leading NAME=value assignments count, exactly where the shell would honour one. Cost, pinned as a fixture: an override after `&&` no longer arms. - $HOME resolved once, and unset / empty / "/" refused. This was filed as a checkout-arm defect and is larger: under `set -u` the old file died at line 62 on EVERY command with HOME unset, exit 1, before the API arms or the APPROVE trap ran. A seat with no HOME (systemd unit, container, env -i) had no guard at all. A checkout whose question cannot be asked now blocks; the blast radius is asserted to be that one command shape and not the session. - Subresource refinement inverted. It asked whether a subresource appears anywhere in the command, so `gh api -X PATCH .../issues/1 -f body=cf-/pulls/2/files` was cleared on the strength of text in its own body. It now clears only when EVERY numbered-object occurrence carries a subresource. - -K/--config refused. curl reads the method, body, headers and URL from that file, so none of them are in the command: every write test read 0 and the call went through. An unreadable request is not a cleared one. mosaic-worktree: never close a git pipe early resolve_repo took the first porcelain line with `awk ... exit`, which closes the read end while git is still writing. git takes SIGPIPE, pipefail returns 141, and the function aborts SILENTLY — no message, no path, exit 141. It fires as a function of REPO SIZE: fine on three worktrees, reliable on seventy. Measured at 73 worktrees (10 KB of porcelain): rc=141, no output. The file already removed a `head -200` for this exact reason; the rule is now uniform. Evidence — every new case run against 029af418, the tree before these fixes: test-wrapper-guard.sh 130/130 pass here; 15 FAIL against 029af418 test-mosaic-worktree-large-repo.sh 2/2 pass here; 2 FAIL against 029af418 (got rc=141 and empty output, the signature) No pre-existing fixture changed behaviour on the old guard, so the new cases are the whole delta. The size dependence is stubbed out rather than inherited: a test that ran against whatever repo it sits in would have PASSED on the broken tree. --- .../framework/tools/git/mosaic-worktree.sh | 19 ++- .../git/test-mosaic-worktree-large-repo.sh | 95 +++++++++++ .../framework/tools/git/test-wrapper-guard.sh | 98 ++++++++++++ .../framework/tools/git/wrapper-guard.sh | 149 +++++++++++++++++- 4 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 packages/mosaic/framework/tools/git/test-mosaic-worktree-large-repo.sh diff --git a/packages/mosaic/framework/tools/git/mosaic-worktree.sh b/packages/mosaic/framework/tools/git/mosaic-worktree.sh index 5b3cf936..926365eb 100755 --- a/packages/mosaic/framework/tools/git/mosaic-worktree.sh +++ b/packages/mosaic/framework/tools/git/mosaic-worktree.sh @@ -65,7 +65,24 @@ resolve_repo() { local start="${REPO_HINT:-$PWD}" git -C "$start" rev-parse --git-dir >/dev/null 2>&1 \ || die "not inside a git repository: $start" - MAIN_WT="$(git -C "$start" worktree list --porcelain | awk '/^worktree /{print substr($0,10); exit}')" + # Take the first entry WITHOUT closing the pipe early. `awk ... exit` on the + # first match closes the read end while git is still writing, git takes SIGPIPE, + # and under `set -euo pipefail` the command substitution returns 141 and this + # function aborts SILENTLY — no message, no worktree, and `new` exits 141 while + # printing nothing at all. + # + # Whether it happens depends on how much git still had to write when awk left, + # so the failure is a function of REPO SIZE: fine on a repo with three + # worktrees, reliably broken on one with seventy. That is backwards — the repos + # this helper exists to serve are exactly the ones that accumulated worktrees, + # and it silently did nothing on those while working everywhere it was tried. + # Measured on a repo with 73 worktrees (10 KB of porcelain): rc=141, no output. + # + # The file's own comment block below already names this class for `head -200` + # and removed that cap for the same reason. The `exit` here is the same defect + # in the same file, so the rule is now uniform: nothing in this script closes a + # git pipe early. Dropping `exit` costs one pass over a few KB. + MAIN_WT="$(git -C "$start" worktree list --porcelain | awk '/^worktree /&&!seen{print substr($0,10); seen=1}')" [ -n "$MAIN_WT" ] || die "could not resolve the main worktree" REPO_NAME="$(basename -- "$MAIN_WT")" REPO_PARENT="$(dirname -- "$MAIN_WT")" diff --git a/packages/mosaic/framework/tools/git/test-mosaic-worktree-large-repo.sh b/packages/mosaic/framework/tools/git/test-mosaic-worktree-large-repo.sh new file mode 100644 index 00000000..5b2eef7b --- /dev/null +++ b/packages/mosaic/framework/tools/git/test-mosaic-worktree-large-repo.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# test-mosaic-worktree-large-repo.sh — the helper must work on the repos it exists for. +# +# resolve_repo() took the first line of `git worktree list --porcelain` with +# `awk '/^worktree /{print substr($0,10); exit}'`. The `exit` closes the read end +# of the pipe while git is still writing, git takes SIGPIPE, and under +# `set -euo pipefail` the command substitution returns 141 — so the assignment +# fails, `set -e` aborts the function, and the script dies printing NOTHING. No +# message, no path, no worktree, exit 141. +# +# What makes it worth a dedicated test rather than a fixture line is WHEN it +# fires. If git finishes writing before awk leaves, there is no SIGPIPE and +# everything works. So the failure is a function of how much porcelain the repo +# produces: invisible on a three-worktree repo, reliable on a seventy-worktree +# one. It was measured on a repo with 73 worktrees (10 KB of porcelain) — rc=141, +# no output — and it had passed every hand-check before that, on small repos. +# +# A test that ran `git worktree list` against whatever repo it happens to sit in +# would inherit that same size dependence and would have PASSED on the tree that +# was broken. So git is stubbed on PATH and made to emit a large porcelain +# stream, which turns "depends on the repo you are standing in" into "always". +# +# Exit: 0 = the helper resolved the repo · 1 = it did not + +set -uo pipefail + +HERE="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" +TOOL="${1:-$HERE/mosaic-worktree.sh}" +[ -x "$TOOL" ] || { printf 'test-mosaic-worktree-large-repo: not executable: %s\n' "$TOOL" >&2; exit 2; } + +TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT +mkdir -p "$TMP/bin" + +# The stub answers exactly the two calls resolve_repo makes, and answers the +# porcelain one with ~450 KB — comfortably past a 64 KB pipe buffer, so the +# writer is still writing when a reader that quits early goes away. Anything +# else exits non-zero rather than pretending to be git. +cat > "$TMP/bin/git" <<'STUB' +#!/bin/sh +while [ $# -gt 0 ]; do + case "$1" in -C) shift 2 ;; *) break ;; esac +done +case "$*" in + "rev-parse --git-dir") + echo .git; exit 0 ;; + "worktree list --porcelain") + # The first entry is the main worktree. That single line is all the helper + # needs, and it is exactly what it stopped receiving. + printf 'worktree /src/fakerepo\nHEAD %040d\nbranch refs/heads/main\n\n' 0 + awk 'BEGIN{ for (i = 0; i < 4000; i++) + printf "worktree /src/fakerepo-worktrees/w%d\nHEAD %040d\nbranch refs/heads/topic-%d\n\n", i, 0, i }' + # NOT `exit 0`. Real git dies of SIGPIPE here and reports 141, and pipefail + # in the caller is what turns that into the silent abort. A stub that exits 0 + # regardless hands the caller a clean status and the probe passes on the + # broken tree — which is how this test failed to be a test on its first run. + exit $? ;; +esac +exit 1 +STUB +chmod +x "$TMP/bin/git" + +fail=0 +check() { + local why="$1" want="$2" got="$3" + if [ "$want" = "$got" ]; then + printf 'ok %s\n' "$why" + else + printf 'FAIL %s\n want: %s\n got: %s\n' "$why" "$want" "$got" + fail=1 + fi +} + +out="$(PATH="$TMP/bin:$PATH" "$TOOL" path feat/workspace-hygiene 2>&1)" +rc=$? + +# Both halves are asserted. rc alone would pass if the helper started printing a +# usage error, and output alone would miss a non-zero exit — and the defect's +# signature is precisely a non-zero exit with no output, which only the pair +# distinguishes from every other way this could go wrong. +check 'resolving a repo with a large worktree list exits 0' 0 "$rc" +check 'and derives the path from the main worktree' /src/fakerepo-worktrees/feat-workspace-hygiene "$out" + +printf '\n' +if [ "$fail" -eq 0 ]; then + printf 'mosaic-worktree: resolves against a large porcelain stream.\n' +else + cat <<'EOF' +mosaic-worktree could not resolve the repository. + +An empty output with a non-zero exit is the SIGPIPE signature: a reader that +quits early (`awk ... exit`, `head -n`) kills the producer, and pipefail turns +that into a silent abort. Nothing in this script may close a git pipe early. +EOF +fi +exit "$fail" diff --git a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh index ef1c77fe..aaccc77f 100755 --- a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh @@ -235,6 +235,44 @@ FIXTURES="$TMP/fixtures.tsv" printf '0\t{"tool_input":{"command":"curl -X POST -d {\\"event\\":\\"APPROVED\\"} https://git.example.invalid/api/v1/repos/a/b/releases"}}\tAPPROVED is the correct value and is never the trap\n' # Documented over-block, pinned so it is a known boundary and not a surprise. printf '2\t{"tool_input":{"command":"python3 -c '"'"'print(\\"https://git.example.invalid/api/v1/repos/a/b/issues/1/comments .post(\\")'"'"'"}}\tprose carrying .post( near a wrapped URL is refused, by the same payload rule\n' + + # --- round nine, all four from one adversarial pass, and three of them are + # the same shape: a test written over the WHOLE command text deciding an + # ALLOW. That is the fail-open form this file keeps rediscovering, and it had + # reached the break-glass itself. + # + # BREAK-GLASS. `case "$CMD" in *MOSAIC_WRAPPER_OVERRIDE=1*)` cleared the entire + # command if that string appeared anywhere in it — so quoting the override in a + # note, or naming a variable after it, disabled the guard for the call sitting + # beside it. The override is now read POSITIONALLY: leading `NAME=value` + # assignments only, exactly where the shell would honour one. + printf '2\t{"tool_input":{"command":"echo \\"MOSAIC_WRAPPER_OVERRIDE=1 curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews\\" >> notes.md"}}\tquoting the override in a document does not arm it\n' + printf '2\t{"tool_input":{"command":"NOTES=MOSAIC_WRAPPER_OVERRIDE=1 curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tan assignment whose VALUE is the override is not the override\n' + printf '2\t{"tool_input":{"command":"MOSAIC_WRAPPER_OVERRIDE=10 curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\t=10 matched the old substring test and is not the value 1\n' + printf '0\t{"tool_input":{"command":"GITEA_TOKEN=$T MOSAIC_WRAPPER_OVERRIDE=1 curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tthe override still works behind other assignments, as the shell reads it\n' + # The cost, pinned rather than discovered later: positional means positional. + printf '2\t{"tool_input":{"command":"cd /tmp && MOSAIC_WRAPPER_OVERRIDE=1 curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge"}}\tan override after && is not in command position and does not arm\n' + + # SUBRESOURCE REFINEMENT, same defect one arm lower. It asked whether a + # subresource appears ANYWHERE in the command, so a numbered-object write was + # cleared on the strength of text in its own BODY. Inverted: clear only when + # EVERY numbered-object occurrence carries a subresource. + printf '2\t{"tool_input":{"command":"gh api -X PATCH repos/a/b/issues/1 -f body=cf-/pulls/2/files"}}\ta subresource in the body does not clear a write to the numbered issue\tissue-edit.sh\n' + printf '2\t{"tool_input":{"command":"gh api -X PATCH repos/a/b/issues/1 -f body=cf-/issues/3/reactions"}}\tsame, quoting a subresource of the same object type\tissue-edit.sh\n' + # ...and its documented cost, in the safe direction. + printf '2\t{"tool_input":{"command":"gh api -X POST repos/a/b/issues/1/reactions -f content=cf-/issues/2"}}\tan unwrapped subresource write that quotes a bare issue is refused\n' + + # -K/--config. curl reads the method, the body, the headers AND the URL from + # that file, so none of them are in the command: every write test above read 0 + # and the call went through. An unreadable request is not a cleared one. + printf '2\t{"tool_input":{"command":"curl --config /tmp/req https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tthe request in a config file is unreadable, so it is refused\t--config/-K\n' + printf '2\t{"tool_input":{"command":"curl -K /tmp/req https://git.example.invalid/api/v1/repos/a/b/issues"}}\tthe short spelling, same answer\t--config/-K\n' + # Cost, stated: the scope gate admits any https URL, so this refuses a + # --config read against a host that has nothing to do with a forge. The + # alternative is to require a forge marker in a command whose URL may itself + # be in the file, which is the hole again. + printf '2\t{"tool_input":{"command":"curl --config /tmp/req https://example.invalid/anything"}}\tan unrelated https URL with --config is refused too, by decision\t--config/-K\n' + printf '0\t{"tool_input":{"command":"eslint --config .eslintrc.json src/"}}\t--config outside an API-shaped command is nobody'"'"'s business\n' } > "$FIXTURES" fail=0 n=0 @@ -260,6 +298,66 @@ while IFS=$'\t' read -r want payload why remedy; do printf 'ok %s\n' "$why" done < "$FIXTURES" +# ---- $HOME resolution ------------------------------------------------------ +# These cannot be fixtures. Every case above varies the COMMAND; this defect +# varies the ENVIRONMENT, and the loop has no way to express that. +# +# The checkout arm built its pattern from "$HOME" without asking whether $HOME +# was a usable value. Three values it is not: unset (which is a crash under +# `set -u`, not a decision), empty (the pattern collapses to `/`, so `~|\$HOME|` +# matches whatever the empty alternative touches), and "/" (every absolute path +# is under it, so the comparison stops discriminating). An agent seat running +# with no HOME — a systemd unit without one, a container, `env -i` — got the +# checkout question answered by accident rather than on the merits. +# +# The fix resolves $HOME once, rejects all three, and BLOCKS the checkout it +# cannot adjudicate. A guard may not clear a question it was unable to ask. The +# blast radius of that fail-closed arm is asserted below to be one command shape +# and not the session: with no HOME at all, ordinary commands still pass and the +# API arms still block. +home_case() { + local why="$1" want="$2" homeval="$3" cmd="$4" needle="${5:-}" + local out got + n=$((n + 1)) + if [ "$homeval" = "@unset" ]; then + out="$(printf '%s' "{\"tool_input\":{\"command\":\"$cmd\"}}" | env -u HOME "$GUARD" 2>&1)" + else + out="$(printf '%s' "{\"tool_input\":{\"command\":\"$cmd\"}}" | env HOME="$homeval" "$GUARD" 2>&1)" + fi + got=$? + if [ "$got" != "$want" ]; then + printf 'FAIL %s (want exit %s, got %s)\n' "$why" "$want" "$got" + fail=1 + return + fi + if [ -n "$needle" ] && ! printf '%s' "$out" | grep -Fq -- "$needle"; then + printf 'FAIL %s (exit %s, but the message does not say %s)\n' "$why" "$got" "$needle" + fail=1 + return + fi + printf 'ok %s\n' "$why" +} + +home_case 'HOME unset: a checkout is refused, not adjudicated' \ + 2 '@unset' 'git clone https://example.invalid/x /src/wt' 'unset or unusable' +home_case 'HOME empty: same, and it is not the same thing as unset' \ + 2 '' 'git clone https://example.invalid/x /src/wt' 'unset or unusable' +home_case 'HOME=/ : every path is under it, so it discriminates nothing' \ + 2 '/' 'git clone https://example.invalid/x /src/wt' 'unset or unusable' +home_case 'a usable HOME still allows a checkout onto a work filesystem' \ + 0 '/home/tester' 'git clone https://example.invalid/x /src/wt' +home_case 'a usable HOME still catches the literal path' \ + 2 '/home/tester' 'git clone https://example.invalid/x /home/tester/wt' 'checks a repository out under' +home_case 'and the unexpanded $HOME spelling, which needs no resolution at all' \ + 2 '/home/tester' 'git worktree add $HOME/wt topic' 'checks a repository out under' +# The fail-closed arm is scoped to checkouts. If it were not, a seat with no +# HOME would have every command it runs refused, which is how a guard gets +# disabled rather than fixed. +home_case 'HOME unset does not block an ordinary command' \ + 0 '@unset' 'ls -la /src' +home_case 'HOME unset does not stop the API arms doing their job' \ + 2 '@unset' 'curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews' 'pr-review.sh' + printf '\n' if [ "$fail" -eq 0 ]; then printf 'wrapper-guard: %d/%d fixtures behaved as specified.\n' "$n" "$n" diff --git a/packages/mosaic/framework/tools/git/wrapper-guard.sh b/packages/mosaic/framework/tools/git/wrapper-guard.sh index 666caa40..423ece4b 100755 --- a/packages/mosaic/framework/tools/git/wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/wrapper-guard.sh @@ -48,8 +48,50 @@ CMD="$(printf '%s' "$INPUT" | jq -r '.tool_input.command // empty' 2>/dev/null | # joined form, because that is the command. CMD="$(printf '%s' "$CMD" | sed -e ':a' -e 'N' -e '$!ba' -e 's/\\\n//g')" -# Honour the override only when it is set in the command itself or the env. -case "$CMD" in *MOSAIC_WRAPPER_OVERRIDE=1*) exit 0 ;; esac +# Honour the override only where a shell would actually TREAT it as one: the +# environment-assignment run at the head of the command, or this process's own +# environment. The first version asked whether the token appeared ANYWHERE in the +# command text. That is not a test of what the shell does; it is a test of what +# the string contains, and three shapes turned the whole guard off silently, each +# with exit 0 and no message: +# +# curl -d '{"body":"... MOSAIC_WRAPPER_OVERRIDE=1 ..."}' .../issues/1/comments +# a quoted BODY disabling the guard for its own write — and the bodies most +# likely to carry the token are this file's own documentation, a relayed +# block message, or a commit message quoting a previous refusal; +# NOTES=MOSAIC_WRAPPER_OVERRIDE=1 curl ... +# the token as another variable's VALUE, which sets nothing; +# ... MOSAIC_WRAPPER_OVERRIDE=10 ... +# `*=1*` matched `=10`, `=1x`, `=123`; the glob never bounded the value. +# +# A control that is off is worse than no control, because the block message is +# what tells an agent the control exists. So the override is now read +# POSITIONALLY, by the rule a shell uses: leading assignments only, up to the +# first token that is not one. A body can never occupy that position, and the +# value must be exactly 1. +# +# Deliberate cost, stated rather than discovered: `cd /x && MOSAIC_WRAPPER_OVERRIDE=1 +# curl ...` is NOT honoured — only the head of the command is, and only its first +# line, because an override applies to the command it prefixes and not to a later +# one. Putting the override first is the remedy, and this direction fails closed. +override_prefixed() { + local first tok + first="${CMD%%$'\n'*}" + local IFS=$' \t' + set -f + # shellcheck disable=SC2086 + set -- $first + set +f + for tok in "$@"; do + case "$tok" in + MOSAIC_WRAPPER_OVERRIDE=1) return 0 ;; + [A-Za-z_]*=*) ;; + *) return 1 ;; + esac + done + return 1 +} +override_prefixed && exit 0 [ "${MOSAIC_WRAPPER_OVERRIDE:-0}" = "1" ] && exit 0 # The wrappers this guard points at are its own siblings. Resolving relative to @@ -62,9 +104,53 @@ W="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" [ -x "$W/pr-review.sh" ] || W="$HOME/.config/mosaic/tools/git" # ---- 1. checkout into $HOME ------------------------------------------------ +# $HOME has to be RESOLVED before anything can be compared against it, and the +# first version interpolated it directly into the pattern. Both ways of it being +# absent were wrong, in OPPOSITE directions, which is why neither showed up as a +# simple "it stopped working": +# +# HOME unset under `set -u` the expansion aborts the script. A PreToolUse +# hook exiting nonzero-but-not-2 is a non-blocking error, so the +# checkout it was asked about is ALLOWED. The guard failed open in +# precisely the case where it could not answer the question. +# HOME='' the alternation gained an EMPTY branch — (~|\$HOME|)/ — which +# matches any slash at all, so a legitimate /src checkout was +# refused. Unusable in the other direction. +# +# Empty and unset are different values and neither one is a home directory. '/' +# is rejected for the same reason as '': every path is under it, so the +# comparison stops discriminating at all. Where the literal path cannot be +# established the `~` and `$HOME` spellings are still checked, and a checkout +# left unresolved BLOCKS rather than clears — a guard may not clear a question it +# was unable to ask. The blast radius of that fail-closed arm is exactly one +# command shape (clone / worktree add), not the session. +home_re='~|\$HOME' +home_known=0 +case "${HOME-}" in + /?*) home_re="$home_re|$(printf '%s' "$HOME" | sed 's/[][\.*^$+?(){}|]/\\&/g')" + home_known=1 ;; +esac + if printf '%s' "$CMD" | grep -Eq 'git[^|;&]*(clone|worktree[[:space:]]+add)'; then + if [ "$home_known" -eq 0 ]; then + cat < +EOF + exit 2 + fi # Any argument that resolves under $HOME and is not under a work filesystem. - if printf '%s' "$CMD" | grep -Eq "(^|[[:space:]=\"'])(~|\\\$HOME|$HOME)/"; then + if printf '%s' "$CMD" | grep -Eq "(^|[[:space:]=\"'])($home_re)/"; then cat <