guard: read command position, refuse unreadable URLs, survive pipefail
ci/woodpecker/pr/ci Pipeline was canceled
ci/woodpecker/pr/ci Pipeline was canceled
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.
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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 <account>` 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 <branch>` 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
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <<EOF
|
||||
BLOCKED: raw provider API write whose URL this guard cannot read.
|
||||
|
||||
The URL is assembled from shell expansions, so the endpoint it names does not
|
||||
exist until the shell builds it — after this check runs. The guard cannot tell
|
||||
whether it is a wrapped endpoint, and an unreadable URL is not a cleared one.
|
||||
|
||||
$W/ <- the wrappers; use the one for the endpoint you are calling
|
||||
|
||||
If you are calling a wrapped endpoint (reviews, merges, comments, pulls,
|
||||
issues, milestones), use the wrapper — it also resolves identity explicitly,
|
||||
which matters on a host whose default provider login is an admin account.
|
||||
|
||||
If this is genuinely not a provider endpoint, either write the URL literally so
|
||||
the guard can see what it is, or prefix MOSAIC_WRAPPER_OVERRIDE=1.
|
||||
EOF
|
||||
exit 2
|
||||
fi
|
||||
|
||||
# Block on the ENDPOINT, never on whether the wrapper file happens to exist.
|
||||
# The previous version required `[ -x "$W/$wrapper" ]`, which meant a host
|
||||
# with a broken or absent install allowed exactly the raw writes the guard
|
||||
|
||||
Reference in New Issue
Block a user