From 8a901cc19aa1e6088354bf6de4a466e295b10ba0 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Wed, 12 Aug 2026 17:26:09 -0500 Subject: [PATCH] guard: read command position, refuse unreadable URLs, survive pipefail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two of the same independent review. Three findings, all real, and the first two share a root cause: the guard was reading command TEXT as though it were a command. 1. Splitting the endpoint token itself defeats fragment matching outright — `a=/api/v1/repos/o/r/iss; b=ues/1/comments` leaves no fragment contiguous. Round one fixed one spelling of this and the reviewer produced the general form immediately. It is not winnable by more fragments: the endpoint does not exist until the shell expands it, and this hook runs first. So the guard stops pretending to read it. A write whose URL contains an expansion, on a visibly forge-shaped command, is now BLOCKED as unreadable — because "I could not find an endpoint" must not mean "there is no endpoint". Opaque URLs that are not forge-shaped (webhooks, artifact stores) still pass. 2. The broadened body detection false-blocked ordinary work: `grep -R "curl -d https://.../issues" docs/`, `echo "curl -d ..." > note.txt`, printing an example from python. Talking about a call is not making one, and this is the direction that actually kills a control — an over-blocking hook gets turned off, and an off hook permits everything. The client must now appear at COMMAND POSITION: line start or after a shell operator, optionally behind VAR=value. In every false positive it sat behind a quote instead. Quotes are deliberately NOT stripped before matching; real calls quote their URLs. 3. `wt_precious()` aborted `cmd_rm` under `set -euo pipefail`: `grep -v` exits 1 when it filters everything out, which is exactly the disposable-only case, so a SAFE worktree failed to remove with no message. Fixed, and the same defect was latent one step upstream in `wt_dirty()`, where `head -200` SIGPIPEs git on any worktree with 201 changed files. The cap is gone — counting is cheap and the cap only ever truncated output that is no longer printed. Seven new fixtures pin all of it, in both directions. 27/27. --- .../mosaic/framework/defaults/STANDARDS.md | 10 +-- .../framework/guides/TOOLS-REFERENCE.md | 74 +++++++++---------- .../framework/tools/git/mosaic-worktree.sh | 32 ++++++-- .../framework/tools/git/test-wrapper-guard.sh | 15 ++++ .../framework/tools/git/wrapper-guard.sh | 60 ++++++++++++++- 5 files changed, 143 insertions(+), 48 deletions(-) diff --git a/packages/mosaic/framework/defaults/STANDARDS.md b/packages/mosaic/framework/defaults/STANDARDS.md index a1cf2bc0..43d1c8ba 100644 --- a/packages/mosaic/framework/defaults/STANDARDS.md +++ b/packages/mosaic/framework/defaults/STANDARDS.md @@ -61,11 +61,11 @@ produces a review that passes and proves nothing. Both are defects. Tiers are named by **capability class**, so the standard survives a model generation. An operator binds each class to a concrete model id. -| Class | Use for | -| --- | --- | -| `search` | grep/glob, file location, status and health checks, one-line mechanical edits | -| `build` | feature implementation, test writing, bugfixes, routine refactors | -| `judge` | code review, planning, API/compat-sensitive changes | +| Class | Use for | +| ------------- | ----------------------------------------------------------------------------------------- | +| `search` | grep/glob, file location, status and health checks, one-line mechanical edits | +| `build` | feature implementation, test writing, bugfixes, routine refactors | +| `judge` | code review, planning, API/compat-sensitive changes | | `adversarial` | security review, ambiguous architecture, anything where a wrong "looks fine" is expensive | Rules: diff --git a/packages/mosaic/framework/guides/TOOLS-REFERENCE.md b/packages/mosaic/framework/guides/TOOLS-REFERENCE.md index 5ba36d1e..df2379ad 100644 --- a/packages/mosaic/framework/guides/TOOLS-REFERENCE.md +++ b/packages/mosaic/framework/guides/TOOLS-REFERENCE.md @@ -19,51 +19,51 @@ from a wrapper that was never written — which is how the APPROVE/APPROVED inci Every command takes `--help`. All of them accept `--login ` to pin the acting identity; supply it explicitly on any host where the provider CLI's default account is an admin. -| Issues | | -| --- | --- | -| `issue-create.sh` | Create an issue (Gitea or GitHub) | -| `issue-view.sh` | Show one issue | -| `issue-list.sh` | List issues | -| `issue-edit.sh` | Edit title/body/labels/milestone | -| `issue-comment.sh` | Add a comment | -| `issue-assign.sh` | Assign or unassign | -| `issue-close.sh` | Close an issue | -| `issue-reopen.sh` | Reopen a closed issue | +| Issues | | +| ------------------ | --------------------------------- | +| `issue-create.sh` | Create an issue (Gitea or GitHub) | +| `issue-view.sh` | Show one issue | +| `issue-list.sh` | List issues | +| `issue-edit.sh` | Edit title/body/labels/milestone | +| `issue-comment.sh` | Add a comment | +| `issue-assign.sh` | Assign or unassign | +| `issue-close.sh` | Close an issue | +| `issue-reopen.sh` | Reopen a closed issue | -| Pull requests | | -| --- | --- | -| `pr-create.sh` | Open a pull request | -| `pr-view.sh` | Show one PR | -| `pr-list.sh` | List PRs | -| `pr-diff.sh` | Fetch a PR's diff | +| Pull requests | | +| ---------------- | --------------------------------------------------------- | +| `pr-create.sh` | Open a pull request | +| `pr-view.sh` | Show one PR | +| `pr-list.sh` | List PRs | +| `pr-diff.sh` | Fetch a PR's diff | | `pr-metadata.sh` | PR metadata as JSON (head SHA, base, state, mergeability) | -| `pr-review.sh` | **Place a review verdict — see the dialect note below** | -| `pr-ci-wait.sh` | Block until the PR's CI reaches a terminal state | -| `pr-merge.sh` | Merge a PR | -| `pr-close.sh` | Close a PR without merging | +| `pr-review.sh` | **Place a review verdict — see the dialect note below** | +| `pr-ci-wait.sh` | Block until the PR's CI reaches a terminal state | +| `pr-merge.sh` | Merge a PR | +| `pr-close.sh` | Close a PR without merging | -| Milestones | | -| --- | --- | +| Milestones | | +| --------------------- | ------------------ | | `milestone-create.sh` | Create a milestone | -| `milestone-list.sh` | List milestones | -| `milestone-close.sh` | Close a milestone | +| `milestone-list.sh` | List milestones | +| `milestone-close.sh` | Close a milestone | -| Gates and guards | | -| --- | --- | -| `ci-queue-wait.sh` | CI queue guard — required before push/merge (see below) | -| `push-guard.sh` | Refuse verifications that pass for the wrong reason (e.g. green against an unpushed tree) | -| `mutate-push-guard.sh` | Regenerate the guard's mutation-coverage table from measurement, so the table cannot drift from the guard | -| `verify-clean-clone.sh` | Prove the **committed** artifact runs, from a clean clone — not the working tree | +| Gates and guards | | +| ----------------------- | --------------------------------------------------------------------------------------------------------- | +| `ci-queue-wait.sh` | CI queue guard — required before push/merge (see below) | +| `push-guard.sh` | Refuse verifications that pass for the wrong reason (e.g. green against an unpushed tree) | +| `mutate-push-guard.sh` | Regenerate the guard's mutation-coverage table from measurement, so the table cannot drift from the guard | +| `verify-clean-clone.sh` | Prove the **committed** artifact runs, from a clean clone — not the working tree | -| Context | | -| --- | --- | +| Context | | +| -------------------- | ---------------------------------------------------------------------------------------- | | `detect-platform.sh` | Resolve the provider (Gitea vs GitHub) for the current repo; every other wrapper uses it | -| `lane-brief.sh` | Live dispatch brief for a repo "lane" (milestone/label) straight from the provider | +| `lane-brief.sh` | Live dispatch brief for a repo "lane" (milestone/label) straight from the provider | -| Workspace | | -| --- | --- | +| Workspace | | +| -------------------- | ------------------------------------------------------------------------ | | `mosaic-worktree.sh` | Create/list/remove git worktrees — **the only supported way**; see below | -| `wrapper-guard.sh` | PreToolUse hook that enforces the two rules above; not called by hand | +| `wrapper-guard.sh` | PreToolUse hook that enforces the two rules above; not called by hand | **Workspace placement is derived, not chosen.** `mosaic-worktree.sh new ` takes a branch name and nothing else. Every path comes out of `git worktree list --porcelain` — main worktree, @@ -83,7 +83,7 @@ anyway, under five simultaneous conventions on a single host. Worktrees rather than clones, because `git worktree list` makes every checkout enumerable — a bare clone dropped somewhere on disk can never be safely reclaimed, so it is never reclaimed. `rm` and `gc` decide by **evidence, never by size or age**: a worktree is reclaimable only when -`git status --porcelain` is empty *and* `git rev-list --count HEAD --not --remotes` is 0. Anything +`git status --porcelain` is empty _and_ `git rev-list --count HEAD --not --remotes` is 0. Anything else is preserved and reported. `--force` exists and is yours to type deliberately. `wrapper-guard.sh` is registered as a Claude Code `PreToolUse` hook on `Bash` (see diff --git a/packages/mosaic/framework/tools/git/mosaic-worktree.sh b/packages/mosaic/framework/tools/git/mosaic-worktree.sh index a770f798..5b3cf936 100755 --- a/packages/mosaic/framework/tools/git/mosaic-worktree.sh +++ b/packages/mosaic/framework/tools/git/mosaic-worktree.sh @@ -118,14 +118,36 @@ configuration, credentials, state and caches — not checkouts." ;; # The list stays short and conservative for that reason. DISPOSABLE_RE='(^|/)(node_modules|\.venv|venv|__pycache__|\.mypy_cache|\.pytest_cache|\.ruff_cache|\.turbo|\.cache|\.parcel-cache|\.gradle|dist|build|out|target|coverage|\.next|\.nuxt|\.svelte-kit)(/|$)|\.(pyc|pyo|o|class)$' -wt_dirty() { git -C "$1" status --porcelain 2>/dev/null | head -200 | wc -l; } -wt_unpushed() { git -C "$1" rev-list --count HEAD --not --remotes 2>/dev/null || echo "?"; } +# These three run under `set -euo pipefail` inside command substitution, which +# makes any nonzero exit ANYWHERE in the pipeline abort the calling function +# silently. Two ways that bites, one of which shipped: +# +# * `grep -v` exits 1 when it filters everything out. A worktree whose only +# ignored entry is `node_modules/` is exactly the SAFE case, and it made +# `rm` exit 1 with no message and no removal — found by review. +# * `head -200` closes the pipe, SIGPIPEs the producer, and turns a worktree +# with 201 dirty files into the same silent abort. Not reported; it is the +# same defect one step upstream, so the cap is gone. Counting is cheap; +# the cap only ever protected output that is now never printed. +# +# Every one of them therefore ends in a total, and every stage that can +# legitimately exit nonzero says so explicitly. +wt_dirty() { + local out + out="$(git -C "$1" status --porcelain 2>/dev/null || true)" + if [ -n "$out" ]; then printf '%s\n' "$out" | wc -l; else printf '0'; fi +} + +wt_unpushed() { git -C "$1" rev-list --count HEAD --not --remotes 2>/dev/null || printf '?'; } + # Default --ignored (not =matching) so a 40k-file node_modules collapses to one # directory entry instead of being enumerated and then discarded. wt_precious() { - git -C "$1" status --porcelain --ignored 2>/dev/null \ - | awk '/^!! /{print substr($0,4)}' \ - | grep -Ev "$DISPOSABLE_RE" | head -200 | wc -l + local ignored + ignored="$(git -C "$1" status --porcelain --ignored 2>/dev/null \ + | awk '/^!! /{print substr($0,4)}' || true)" + [ -n "$ignored" ] || { printf '0'; return 0; } + printf '%s\n' "$ignored" | grep -Ecv "$DISPOSABLE_RE" || true } wt_state() { diff --git a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh index 3782464e..60f51241 100755 --- a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh @@ -52,6 +52,21 @@ FIXTURES="$TMP/fixtures.tsv" printf '0\t{"tool_input":{"command":"curl -s https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tno body and no verb is a read\n' printf '0\t{"tool_input":{"command":"grep -rn /pulls/ src/ | head -20"}}\ta path fragment in a grep is not an API call\n' printf '0\t{"tool_input":{"command":"curl -X POST -d @b https://registry.example.invalid/v2/x/manifests/latest"}}\tan unwrapped API is not this guard'"'"'s business\n' + # --- round two of the same review. Splitting the ENDPOINT TOKEN defeats any + # amount of fragment matching, because the endpoint does not exist until the + # shell expands it. The guard now refuses to clear a write whose URL it cannot + # read, rather than pretending it read one. + printf '2\t{"tool_input":{"command":"a=/api/v1/repos/a/b/iss; b=ues/1/comments; curl -d@body https://git.example.invalid${a}${b}"}}\tan endpoint token split across variables is unreadable, not absent\n' + printf '2\t{"tool_input":{"command":"a=/api/v1/repos/a/b/pu; b=lls/1/reviews; curl -d@body https://git.example.invalid${a}${b}"}}\tsame split, review endpoint\n' + printf '0\t{"tool_input":{"command":"curl -X POST -d @payload https://hooks.example.invalid/services/${WEBHOOK_ID}"}}\tan opaque URL that is not forge-shaped stays allowed\n' + # And the other direction, which is the failure mode that gets a hook deleted: + # discussing a call is not making one. In each of these the client sits behind + # a quote, never at command position. + printf '0\t{"tool_input":{"command":"grep -R \\"curl -d https://git.example.invalid/api/v1/repos/a/b/issues\\" docs/"}}\tgrepping for an example is not calling it\n' + printf '0\t{"tool_input":{"command":"echo \\"curl -d https://git.example.invalid/api/v1/repos/a/b/pulls\\" > note.txt"}}\twriting an example into a file is not calling it\n' + printf '0\t{"tool_input":{"command":"python3 -c '"'"'print(\\"curl -d https://git.example.invalid/api/v1/repos/a/b/issues\\")'"'"'"}}\tprinting an example is not calling it\n' + # Command position must still catch the real thing behind operators and env. + printf '2\t{"tool_input":{"command":"cd /tmp && GITEA_TOKEN=$T curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge"}}\ta real call behind && and an assignment is still a call\n' } > "$FIXTURES" fail=0 n=0 diff --git a/packages/mosaic/framework/tools/git/wrapper-guard.sh b/packages/mosaic/framework/tools/git/wrapper-guard.sh index a3e8b445..35844b6d 100755 --- a/packages/mosaic/framework/tools/git/wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/wrapper-guard.sh @@ -85,7 +85,23 @@ fi # and the host-anchored literal never appears, so the check read clean while the # write went through. The endpoint fragments below survive it, because the # fragment has to appear somewhere for the URL to be constructible at all. -if printf '%s' "$CMD" | grep -Eq 'curl|wget|http(ie)?[[:space:]]' \ +# The client must be at COMMAND POSITION — start of the command, or directly +# after a shell operator, optionally behind VAR=value assignments. Substring +# presence is not enough, and this is the second thing review caught: with a +# bare substring test, +# grep -R "curl -d https://host/api/v1/repos/a/b/issues" docs/ +# echo "curl -d https://host/api/v1/repos/a/b/pulls" > note.txt +# were both BLOCKED. Talking about a call is not making one. Over-blocking is +# not the safe direction here — a guard that blocks ordinary work gets switched +# off, and a guard that is off permits everything. +# +# Quoting is what separates the two: in every false positive the client sits +# immediately after a quote character, never after an operator. Note the +# deliberate absence of quote-stripping: the URL is frequently quoted in REAL +# calls, so stripping quotes before matching would blind the endpoint check. +CLIENT_AT_CMD_POS='(^|[;&|(){}]|`|\$\()[[:space:]]*([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+)*(curl|wget|httpie|http)([[:space:]]|$)' + +if printf '%s' "$CMD" | grep -Eq "$CLIENT_AT_CMD_POS" \ && printf '%s' "$CMD" | grep -Eq 'https?://'; then # Write detection. Every spelling curl accepts, because the guard is defeated @@ -110,6 +126,48 @@ if printf '%s' "$CMD" | grep -Eq 'curl|wget|http(ie)?[[:space:]]' \ *"/milestones"*) endpoint="milestone"; wrapper="milestone-create.sh" ;; esac + # A URL the guard cannot READ is a URL the guard must not CLEAR. + # + # Round one fixed one spelling of this and review immediately produced the + # general form: split the endpoint token itself across two variables — + # a=/api/v1/repos/o/r/iss; b=ues/1/comments + # curl -d@body "https://host${a}${b}" + # — and no fragment above ever appears contiguously. Chasing that with more + # fragments is unwinnable: the endpoint does not exist until the shell + # expands it, and this hook runs before that. + # + # So stop pretending to read it. If a write's URL contains an expansion, + # the guard has no endpoint to judge, and "no endpoint" must not mean + # "allowed" — that is the same absence-driven allow as the missing-wrapper + # case, wearing different clothes. + # + # Scoped to commands that are visibly forge-shaped, so an opaque webhook or + # artifact POST is untouched. A caller who splits `/api/` and the hostname + # as well does get through; that is no longer a mistake anyone makes by + # accident, and this hook stops mistakes. It is not a sandbox, and pretending + # otherwise is how you get a control nobody can trust the boundaries of. + if [ -z "$endpoint" ] \ + && printf '%s' "$CMD" | grep -Eq 'https?://[^[:space:]"'"'"'|;&)]*[$`]' \ + && printf '%s' "$CMD" | grep -Eq '/api/v[0-9]|/repos/|git\.|gitea|github\.com|gitlab|forgejo'; then + cat <