diff --git a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh index 544f8313..d42cc12c 100755 --- a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh @@ -25,7 +25,10 @@ TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT FIXTURES="$TMP/fixtures.tsv" # Each line: TAB TAB -# 0 = allowed, 2 = blocked. +# [ TAB ] +# 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' diff --git a/packages/mosaic/framework/tools/git/wrapper-guard.sh b/packages/mosaic/framework/tools/git/wrapper-guard.sh index b9cc393a..cc745ac6 100755 --- a/packages/mosaic/framework/tools/git/wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/wrapper-guard.sh @@ -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}/ 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 <