wrapper-guard: make subresource ownership an inventory, and check the advice
ci/woodpecker/pr/ci Pipeline was canceled

Round seven fixed "wrong wrapper advice" by letting every path under a numbered
issue or PR flow through, on the stated reasoning that no wrapper owned any of
them. Review checked that reasoning against the directory and it was false:

  gh api -X PATCH repos/a/b/issues/1 -f title=x
  curl -X PATCH -d @b https://host/api/v1/repos/a/b/issues/1
  gh api -X PATCH repos/a/b/issues/1/labels -f labels[]=bug
  gh api -X POST  repos/a/b/issues/1/assignees -f assignees[]=u

issue-edit.sh takes --title/--body/--labels/--milestone and issue-assign.sh
takes assignee/labels/milestone, so all four are wrapped calls and all four
returned 0. The guard answered "allow" because wrapper ownership had been
ASSUMED absent rather than looked up — the same absence-driven allow this file
exists to remove, committed inside the fix for it. I withdraw the round-seven
departure: the reviewer's position was right on the evidence, and my argument
for it was sound reasoning applied to a fact I never checked.

The endpoint map is now an inventory read off tools/git/*.sh and their flags:
assignees to issue-assign.sh, labels to issue-edit.sh (naming issue-assign.sh
alongside it, since both set them), a numbered issue to issue-edit.sh (naming
issue-close.sh/issue-reopen.sh for state), a numbered PR to pr-close.sh (with
the PR title/body gap stated in the message rather than papered over), and
/milestones/{n} to milestone-close.sh instead of the create wrapper.

The residue is defined by SUBTRACTION, not by listing provider API surface:
everything a wrapper owns is consumed by an arm above, so a numbered path that
reaches the end is owned by nothing and still flows through — times, stopwatch,
reactions, a comment edit at /issues/comments/{id}. A list would rot the moment
a provider adds an endpoint, and rot in the blocking direction with wrong advice.

That residue test is a regex, deliberately. `case` globs cannot express a path
SEGMENT, so the natural allow arm *"/issues/"[0-9]*"/"* clears

  gh api -X PATCH repos/a/b/issues/1 -f body="see /docs"

on the strength of a slash inside the body. An allow decided by a glob over the
whole command is the fail-open shape again; the regex pins the segment to the
number, and that command is pinned as a fixture.

Also: `-f labels[]=bug` was not read as a body at all, because the key class
stopped at the bracket. The array spelling is what the provider CLIs use for
repeated fields, so an implicit POST carrying only array fields was invisible.

And the reason six rounds of this were invisible: the harness read the exit code
and nothing else, so a block naming the WRONG wrapper passed every run. Fixtures
may now state the wrapper the message must name, and the wrapped ones do. The
assertion was negative-controlled — pointing one fixture at the wrong wrapper
fails that fixture and only that fixture.

89/89 (was 79), locally and in ci-base. All eight sanitization commands green
in-image. The 18-command ordinary sweep blocks the same three round-six flips
and nothing new, so the tighter map cost nothing on ordinary work.

Gates: sanitization (all eight green in ci-base), shellcheck clean at warning+.
This commit is contained in:
Hermes Agent
2026-08-12 18:25:13 -05:00
parent b4578dcd0a
commit a3cacac7fb
2 changed files with 133 additions and 32 deletions
@@ -25,7 +25,10 @@ TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT
FIXTURES="$TMP/fixtures.tsv"
# Each line: <expected-exit> TAB <hook payload> TAB <what it proves>
# 0 = allowed, 2 = blocked.
# [ TAB <substring the block message must contain> ]
# 0 = allowed, 2 = blocked. The optional fourth field is how the remediation
# itself gets checked; without it a block is only asserted to have happened,
# not to have been useful.
{
printf '2\t{"tool_input":{"command":"git clone https://example.invalid/x ~/wt"}}\tcheckout into $HOME is refused\n'
printf '2\t{"tool_input":{"command":"git worktree add ~/wt topic"}}\tworktree into $HOME is refused\n'
@@ -154,16 +157,31 @@ FIXTURES="$TMP/fixtures.tsv"
printf '0\t{"tool_input":{"command":"grep -f patterns.txt /src/api/v1/repos/a/b/issues.log"}}\tsame, on the flag agents actually collide with\n'
# Wrong remediation is its own defect: /issues/1/labels used to block with
# "use issue-create.sh", which is not the wrapper for that call. No wrapper
# owns an issue/PR subresource, so these flow through like every other
# unwrapped endpoint — and the two arms above them must keep blocking.
printf '0\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/labels"}}\tno wrapper owns issue labels, so there is nothing to enforce\n'
printf '0\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/assignees"}}\tnor assignees\n'
printf '0\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/issues/comments/5"}}\tediting a comment has no wrapper either; only creating one does\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\tand the wrapped subresource must not fall through the arm above it\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues"}}\tnor may issue creation\n'
printf '0\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/labels"}}\tPR labels are unwrapped on the same reasoning as issue labels\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls"}}\tPR creation is wrapped and must not fall through with them\n'
# "use issue-create.sh", which is not the wrapper for that call. Round seven
# answered that by letting EVERY path under a numbered issue or PR through,
# and review showed the reasoning ("no wrapper owns these") was false in this
# tree. These are the reported repros, all rc 0 before round eight, and each
# asserts the wrapper the advice must name — not merely that a block happened.
printf '2\t{"tool_input":{"command":"gh api -X PATCH repos/a/b/issues/1 -f title=x"}}\tan issue edit is issue-edit.sh, not a wrapper gap\tissue-edit.sh\n'
printf '2\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1"}}\tsame call through curl, same wrapper\tissue-edit.sh\n'
printf '2\t{"tool_input":{"command":"gh api -X PATCH repos/a/b/issues/1/labels -f labels[]=bug"}}\tlabels are wrapped, and the advice says by which\tissue-edit.sh\n'
printf '2\t{"tool_input":{"command":"gh api -X POST repos/a/b/issues/1/assignees -f assignees[]=u"}}\tassignees are issue-assign.sh\tissue-assign.sh\n'
printf '2\t{"tool_input":{"command":"gh api repos/a/b/issues/1/assignees -f assignees[]=u"}}\tthe array field spelling is a body with no -X at all\tissue-assign.sh\n'
printf '2\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/labels"}}\ta PR is an issue where labels live, so the issue wrapper owns them\tissue-edit.sh\n'
printf '2\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1"}}\tPR state is pr-close.sh, and the gap in that arm is stated\tpr-close.sh\n'
printf '2\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/milestones/4"}}\ta milestone state change is milestone-close.sh, not the create wrapper\tmilestone-close.sh\n'
# The residue: still genuinely owned by nothing, and still flowing through.
printf '0\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/issues/comments/5"}}\tediting a comment has no wrapper; only creating one does\n'
printf '0\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/stopwatch/start"}}\tno wrapper owns a stopwatch, and none is invented for it\n'
printf '0\t{"tool_input":{"command":"gh api -X POST repos/a/b/issues/1/times -f time=60"}}\tnor time tracking\n'
printf '0\t{"tool_input":{"command":"gh api -X POST repos/a/b/issues/1/reactions -f content=+1"}}\tnor reactions\n'
# ...and the residue must be decided by the SEGMENT, never by a stray slash.
printf '2\t{"tool_input":{"command":"gh api -X PATCH repos/a/b/issues/1 -f body=see-/docs/x"}}\ta slash inside the body is not a subresource\tissue-edit.sh\n'
# The arms above the numbered ones must keep blocking, with their own wrappers.
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\tthe wrapped subresource must not fall through\tissue-comment.sh\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues"}}\tnor may issue creation\tissue-create.sh\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls"}}\tPR creation is wrapped and must not fall through with them\tpr-create.sh\n'
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tand a review still names the review wrapper\tpr-review.sh\n'
# The APPROVE trap, in the spelling a provider CLI uses, and the value that
# must never trip it.
@@ -173,17 +191,26 @@ FIXTURES="$TMP/fixtures.tsv"
} > "$FIXTURES"
fail=0 n=0
while IFS=$'\t' read -r want payload why; do
while IFS=$'\t' read -r want payload why remedy; do
[ -n "${want:-}" ] || continue
n=$((n + 1))
printf '%s' "$payload" | "$GUARD" >/dev/null 2>&1
out="$(printf '%s' "$payload" | "$GUARD" 2>&1)"
got=$?
if [ "$got" = "$want" ]; then
printf 'ok %s\n' "$why"
else
if [ "$got" != "$want" ]; then
printf 'FAIL %s (want exit %s, got %s)\n' "$why" "$want" "$got"
fail=1
continue
fi
# A block that names the wrong wrapper is a defect in its own right, and until
# now it was invisible here: the harness read the exit code and nothing else,
# so /issues/1/labels blocking with "use issue-create.sh" passed every run for
# six rounds. Where a fixture states the remediation it expects, assert it.
if [ -n "${remedy:-}" ] && ! printf '%s' "$out" | grep -Fq -- "$remedy"; then
printf 'FAIL %s (blocked, but the advice does not name %s)\n' "$why" "$remedy"
fail=1
continue
fi
printf 'ok %s\n' "$why"
done < "$FIXTURES"
printf '\n'
@@ -173,9 +173,13 @@ if printf '%s' "$CMD" | grep -Eq "$API_SHAPED"; then
'(^|[[:space:]])(-d|-F|-T)|--data([-a-z]*)?[[:space:]=]|--json[[:space:]=]|--form|--upload-file|--post-(data|file)[[:space:]=]' && is_write=1
# The provider CLIs POST implicitly the same way curl does, when handed a
# field. Matched only in `-f key=value` shape, so the far more common `rm -f`
# and `grep -f` cannot be read as a body.
# and `grep -f` cannot be read as a body. The trailing `[]` is the array
# spelling the provider CLIs use for repeated fields (`-f labels[]=bug`), and
# without it the key class stopped at the bracket and the field was not seen
# as a body at all — found while pinning the labels/assignees repros, both of
# which carry it.
printf '%s' "$CMD" | grep -Eq -- \
'(^|[[:space:]])(-f|--field|--raw-field)[[:space:]]+[A-Za-z_][A-Za-z0-9_.-]*=|--input[[:space:]=]' && is_write=1
'(^|[[:space:]])(-f|--field|--raw-field)[[:space:]]+[A-Za-z_][A-Za-z0-9_.-]*(\[\])?=|--input[[:space:]=]' && is_write=1
# ...and a library call is a write without any flag at all.
#
# Second documented over-block, and broader than the body flags because it
@@ -187,30 +191,90 @@ if printf '%s' "$CMD" | grep -Eq "$API_SHAPED"; then
'\.(post|put|patch|delete)\(' && is_write=1
if [ "$is_write" -eq 1 ]; then
endpoint=""; wrapper=""
# The endpoint map, and the rule that keeps it honest: an arm exists here
# ONLY because a wrapper in this directory owns that call. It is an
# inventory, not a model — read off `ls tools/git/*.sh` and the flags each
# script accepts, so it can be re-derived and checked rather than believed.
#
# /pulls/{n}/reviews, /requested_reviewers pr-review.sh
# /pulls/{n}/merge pr-merge.sh
# /issues/{n}/comments issue-comment.sh (create)
# /issues|pulls/{n}/assignees issue-assign.sh
# /issues|pulls/{n}/labels issue-edit.sh, issue-assign.sh
# /milestones/{n} milestone-close.sh
# /issues/{n} issue-edit.sh; issue-close.sh,
# issue-reopen.sh for state
# /pulls/{n} pr-close.sh (state only)
# /pulls, /issues, /milestones the create wrappers
#
# Round seven had a single arm allowing EVERY path under a numbered issue or
# PR, on the reasoning that no wrapper owned any of them. Review showed that
# was false in this tree — issue-edit.sh takes --title/--body/--labels/
# --milestone and issue-assign.sh takes assignee/labels/milestone — so the
# guard was answering "allow" because wrapper ownership had been ASSUMED
# absent instead of looked up. That is the same absence-driven allow the
# whole file exists to remove, committed inside the fix for it. The lesson
# is not "block more"; it is that ownership is an inventory question and an
# inventory has to be read.
#
# Wrong advice remains its own defect — a block an agent cannot comply with
# teaches that the hook is broken and the override is routine, and a routine
# override is a guard that is off. So the fix is precision in BOTH
# directions: every arm names the wrapper that actually owns the call, and
# anything genuinely unowned still flows through (below).
endpoint=""; wrapper=""; alsoown=""
case "$CMD" in
*"/pulls/"*"/reviews"*|*"/pulls/"*"/requested_reviewers"*)
endpoint="pull-request review"; wrapper="pr-review.sh" ;;
*"/pulls/"*"/merge"*) endpoint="pull-request merge"; wrapper="pr-merge.sh" ;;
# A comment EDIT/DELETE lives at /issues/comments/{id} — a sibling of the
# numbered issue, not a child of it. issue-comment.sh only creates, so
# nothing owns this one. Placed above the create arm so it cannot be
# refused with "use issue-comment.sh", which would be the wrong call.
*"/issues/comments/"*) : ;;
*"/issues/"*"/comments"*) endpoint="issue comment"; wrapper="issue-comment.sh" ;;
# Any OTHER path UNDER a numbered issue or PR — labels, assignees, times,
# a comment edit at /issues/comments/{id}, a PATCH of the issue itself.
# No wrapper owns these, and this arm exists so the guard does not claim
# one does: before it, /issues/1/labels fell through to the generic arm
# below and was refused with "use issue-create.sh", which is the wrong
# call. Wrong remediation is worse than no remediation, because a block an
# agent cannot comply with teaches it that the hook is broken and the
# override is routine — and an override that is routine is a guard that is
# off. So these flow through, exactly as /releases and every other
# unwrapped endpoint already does. That is the standing rule here: this
# hook enforces "use the wrapper", and where there is no wrapper it has
# nothing to enforce. The gap belongs in the wrapper set, not in a block.
*"/issues/"?*|*"/pulls/"?*) : ;;
*"/issues/"*"/assignees"*|*"/pulls/"*"/assignees"*)
endpoint="issue assignee"; wrapper="issue-assign.sh" ;;
*"/issues/"*"/labels"*|*"/pulls/"*"/labels"*)
endpoint="issue label"; wrapper="issue-edit.sh"
alsoown="issue-assign.sh -l sets labels too (and the milestone)." ;;
*"/milestones/"[0-9]*) endpoint="milestone state"; wrapper="milestone-close.sh" ;;
# The numbered object itself. These two arms are the fuzzy ones — they
# match a number and then anything — so they are refined immediately
# below rather than trusted as written.
*"/issues/"[0-9]*) endpoint="issue edit"; wrapper="issue-edit.sh"
alsoown="issue-close.sh and issue-reopen.sh own the state change." ;;
*"/pulls/"[0-9]*) endpoint="pull-request edit"; wrapper="pr-close.sh"
alsoown="pr-close.sh owns state=closed. A PR's labels, assignee and
milestone are the ISSUE object on both providers, so issue-edit.sh and
issue-assign.sh own those at the same number. Nothing wraps a PR title/body
edit — that one is a real wrapper gap, and the override exists for it." ;;
*"/pulls"*) endpoint="pull request"; wrapper="pr-create.sh" ;;
*"/issues"*) endpoint="issue"; wrapper="issue-create.sh" ;;
*"/milestones"*) endpoint="milestone"; wrapper="milestone-create.sh" ;;
esac
# Refine the two fuzzy arms, and note WHY this is a regex and not another
# case arm: `case` globs cannot express a path SEGMENT, so an allow arm
# written as *"/issues/"[0-9]*"/"* would clear
# gh api -X PATCH repos/a/b/issues/1 -f body="see /docs"
# on the strength of a slash inside the body. An allow decided by a glob
# over the whole command is exactly the fail-open shape this file keeps
# finding; the regex pins the segment to the number.
#
# The residue is defined by SUBTRACTION rather than by listing provider API
# surface: every subresource a wrapper owns was consumed by an arm above, so
# whatever still carries /issues|pulls/{n}/<segment> here is owned by
# nothing — times, stopwatch, reactions, subscriptions, dependencies, a PR's
# files or commits. Listing them instead would rot the moment a provider
# adds one, and rot in the blocking direction with wrong advice.
case "$endpoint" in
"issue edit"|"pull-request edit")
if printf '%s' "$CMD" | grep -Eq '/(issues|pulls)/[0-9]+/[A-Za-z_]'; then
endpoint=""; wrapper=""; alsoown=""
fi ;;
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
@@ -266,6 +330,13 @@ EOF
$W/$wrapper
Run \`$wrapper --help\` for the flags."
# Several wrappers can own one endpoint (labels are settable from both
# issue-edit.sh and issue-assign.sh; state has its own pair). Naming
# only one of them is how a correct block still ends up reading as
# wrong advice, so say which wrapper owns which part of the call.
[ -n "$alsoown" ] && remedy="$remedy
$alsoown"
else
remedy="The wrapper that covers this endpoint is \`$wrapper\`, and it is NOT
present or not executable at:
@@ -274,6 +345,9 @@ present or not executable at:
That is a broken or incomplete install, not permission to send the call raw.
Repair the install (\`mosaic doctor\`) and use the wrapper."
[ -n "$alsoown" ] && remedy="$remedy
$alsoown"
fi
cat <<EOF
BLOCKED: raw provider API write to the $endpoint endpoint.