fix: assert the gh api guard by behaviour, not by spelling

[johanzander]

Summary

• quality-check.sh failed on a clean origin/main. The gate now asserts the
  property it cares about instead of the spelling that happened to
  satisfy it.
• Relaxing it surfaced a live hole: gh api --field / --raw-field were
  reaching allow — GitHub writes that never prompted. Now guarded.

Root cause

The permission gate demanded the literal strings Bash(gh api) and
Bash(gh api *) in permissions.ask. #657 replaced that blanket with one rule
per write flag, so gh api reads would stop prompting on every call. The
property the gate existed to protect still held — a gh api write still could
not reach allow — but the spelling it pinned was gone:

❌ permissions.ask is missing Bash(gh api)
❌ permissions.ask is missing Bash(gh api *)

Reproduced against a clean export, so it is main's own state and not a local
artefact:

git archive origin/main | tar -x -C /tmp/x && cd /tmp/x && ./scripts/quality-check.sh

Pushes to main do not run the test matrix, so this was latent: it surfaces on
the next PR that merges main. That is how it was found — #653 merged
origin/main and went red on a docs-only diff touching none of this.

Fix

Drop the two literal entries from REQUIRED["ask"]. Whether a gh api write
can reach allow is already asserted by command string in MUST_BE_GUARDED,
and those pins survive a respelling; the literal ones did not.

The hole this uncovered

gh api --help documents two field flags, each with a long form:

-F, --field key=value
-f, --raw-field key=value

Only the short forms were enumerated, so --field fell through to
Bash(gh api *) in allow:

AssertionError: 'gh api …/pulls/614/comments --field body=hi' must ask
assert 'allow' == 'ask'

Merely relaxing the gate would have blessed that. Both long forms are now
pinned in MUST_BE_GUARDED in both argument positions, with matching ask rules
in settings.json.

Scope assessment

Local. The diff adds no parameter, flag, fallback, second construction site
or extra trigger. It removes an assertion that pinned the wrong thing and adds
four pins for spellings that were genuinely unguarded.

Two pieces of forced scope, both stated rather than folded in silently:

• Six type annotations in test_agent_permissions.py. The file had none;
  fix: enforce mypy on changed files, and make the PR review bot always submit a verdict #614's mypy-on-changed-files gate pulls in all six pre-existing errors the
  moment the file is touched.
• docs/agents/local-agent-environment.md. Two claims this makes false:
  that the ask list covers "every gh api", and that quality-check.sh
  "requires the blanket spelling". Both corrected, per the mandatory Step 9
  documentation check.

No CHANGELOG.md entry: agent tooling, zero user-visible effect on the add-on.

Test plan

• ./scripts/quality-check.sh — Errors: 0, Warnings: 0, on the branch
  with origin/main merged in
• test_agent_permissions.py — 48 passed
• Full fast suite via the gate — 2146 passed, 50 skipped

Evidence the test discriminates

Written RED first, against the real settings.json:

• Added --field / --raw-field cases to
  test_gh_api_writes_ask_in_either_flag_position before adding any rule.
• Result: FAILED … assert 'allow' == 'ask' — proving the flag reached
  allow, i.e. the hole was real and not theoretical.
• Added the four ask rules → 48 passed.

The gate itself was mutated the same way: with the settings rules absent it
reported the four MUST_BE_GUARDED strings as unmatched, confirming the new
pins fail closed rather than being decorative.

Outcome-level coverage

MUST_BE_GUARDED in quality-check.sh, plus
test_gh_api_writes_ask_in_either_flag_position. Both assert by command
string — the decision a real invocation would get — not by which rule pattern
happens to catch it, so a future respelling of the rules cannot silently drop
the guard.

[johanzander]

@claude-bot review

[johanzander]

@claude-bot review

[johanzander]

@claude-bot review

[johanzander]

@claude-bot review

[bess-manager-claude-bot]

Review — PR #665

The core change is right and the evidence is real. REQUIRED["ask"] demanded
Bash(gh api) / Bash(gh api *), #657 removed those spellings from
settings.json while keeping the property, and the gate went red on a clean
origin/main. Dropping the two literals and leaning on MUST_BE_GUARDED
(which asserts by command string) is the correct assertion to keep. CI agrees:
Code quality: pass on 73305e1.

One blocking finding, below. It is not a regression this PR introduces — but
it is the same hole class the PR exists to close, in files this diff already
touches, and the doc line it adds will lead the next person to close only half
of it.

---

🔴 Blocker — the attached-value spellings still reach allow

scripts/quality-check.sh:339-344 and .claude/settings.json:155-158 pin the
long forms only in their space-separated spelling:

"Bash(gh api --field *)",
"Bash(gh api * --field *)",
"Bash(gh api --raw-field *)",
"Bash(gh api * --raw-field *)",

gh is cobra/pflag, so --field=k=v is an equally valid invocation, and a
prefix glob ending in --field (with the space) cannot reach it. Same for the
attached shorthand forms. Running the gate's own decide() against this
branch's settings.json:

      ask  gh api repos/o/r/pulls/1/comments --field body=hi
    allow  gh api repos/o/r/pulls/1/comments --field=body=hi
    allow  gh api --field=body=hi repos/o/r/pulls/1/comments
    allow  gh api repos/o/r/pulls/1/comments --raw-field=body=hi
    allow  gh api repos/o/r/pulls/1/comments --method=PUT
    allow  gh api --method=PUT repos/o/r/pulls/1/comments
    allow  gh api repos/o/r/pulls/1/comments -XPUT
    allow  gh api repos/o/r/pulls/1/comments -fbody=hi

These are not theoretical spellings — gh 2.97.0 parses all of them (each
reaches cobra's arg validation, accepts 1 arg(s), received 0, whereas a
genuinely unknown flag gives unknown flag: --nosuchflag):

$ gh api --field=a=b   → accepts 1 arg(s), received 0
$ gh api -XPUT         → accepts 1 arg(s), received 0
$ gh api -fbody=hi     → accepts 1 arg(s), received 0

So gh api <path> --field=body=hi and gh api <path> -XPUT are GitHub writes
that never prompt — by exactly the argument the PR body itself makes:

Only the short forms were enumerated, so --field fell through to
Bash(gh api *) in allow

Checklist item 2 (.github/claude-bot.md → Fitness of approach) is "does it
hold for all valid inputs … not just the case that triggered it?" Here it does
not. The remediation is the same shape as the fix already in the diff — add
--field=* / --raw-field=* / --method=* / -X* / -f* / -F* forms in
both argument positions to .claude/settings.json, and their command strings
to MUST_BE_GUARDED.

Caveat, stated rather than glossed: the module docstring at
backend/tests/test_agent_permissions.py:14-21 disclaims that this models the
real matcher. If the harness normalises --flag=value to --flag value before
matching, this specific gap closes at the harness level. I could not verify
that, and the PR's own RED-test evidence treats decide() as authoritative —
so by the standard this PR sets for itself, the gap is live.

🟡 The doc rule added is under-specified

docs/agents/local-agent-environment.md:146-148:

When you add a gh api write flag, add its command string to
MUST_BE_GUARDED too, in both argument positions.

Position is not the only axis the glob is blind to — spelling is, and that is
what this PR just got bitten by. Worth saying "in both argument positions and
both --flag value / --flag=value spellings", otherwise the next flag gets
half-guarded by a doc that reads as complete.

🟡 PR body describes scope that is not in the diff

The Scope assessment claims:

Six type annotations in test_agent_permissions.py. The file had none

git diff origin/main...origin/fix/gh-api-gate-spelling -- backend/tests/test_agent_permissions.py
is +9/-0 and contains only the four new test cases plus their comment — the
annotations are already on main. Stale body text, not a code problem, but it
makes the diff look larger than it is.

---

Checklist

• Matches the root cause — yes. The gate pinned a spelling; #657 changed
  the spelling while preserving the property; the fix removes the spelling pin
  and keeps the property pin. Reproduction against git archive origin/main
  is the right evidence.
• Would the test catch a regression — for the space-separated long forms,
  yes: test_gh_api_writes_ask_in_either_flag_position
  (backend/tests/test_agent_permissions.py:206-213) asserts by command
  string, and the RED-first evidence (assert 'allow' == 'ask') shows it
  discriminates. For the attached-value forms, no — that is the blocker above.
• Minimal — yes. No new parameter, flag, fallback, second construction
  site or extra trigger; the workaround check passes. The doc edit is required
  by the diff (it made two doc claims false), not scope creep.
• Rule compliance — clean. No Optional[x], no hasattr/getattr
  fallback, no exception-message matching, no new class, no hardcoded entity
  IDs, no secrets. Comments explain WHY (the leak history), not what. No
  CHANGELOG.md entry is correct for agent tooling with no add-on-visible
  effect.
• Scope assessment stated — yes, and correctly categorised as local.

Happy to approve as soon as the attached-value spellings are pinned; the rest
of the diff is good work and the comment archaeology in quality-check.sh is
genuinely useful to the next reader.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Review — PR #665 (re-review of 09ac9be)

The blocker from the previous round is resolved and verified. Approving.

Verification of the fix

I ran the gate's own decide() (the exact model in
backend/tests/test_agent_permissions.py:51-57) over origin/main's
settings.json and this branch's, for every spelling the last review called
out:

command                                            main    pr665
gh api <path> --field body=hi                      allow   ask
gh api <path> --field=body=hi                      allow   ask
gh api --field=body=hi <path>                      allow   ask
gh api <path> --raw-field=body=hi                  allow   ask
gh api <path> --method=PUT                         allow   ask
gh api --method=PUT <path>                         allow   ask
gh api <path> -XPUT                                allow   ask
gh api <path> -fbody=hi                            allow   ask
gh api <path> -Fbody=hi                            allow   ask
gh api <path> --jq .[]                             allow   allow   <- read, still unattended
gh api <path>                                      allow   allow   <- read, still unattended

Every one of the 16 new cases in
test_gh_api_writes_ask_in_either_flag_position is allow on main and ask
here — the test genuinely discriminates, RED without the settings.json half
of the diff. And the reads the whole #657 trade exists to protect are
untouched, which is the thing the broadened globs (-f*, -F*, -X*) could
plausibly have broken and did not.

The shell gate agrees on the branch export:

$ git archive pr/665 | tar -x -C /tmp/pr665 && cd /tmp/pr665 && <gate block>
✅ Permission surface intact (92 command shapes checked, 20 require deny, 26 must stay unattended)

CI on 09ac9be: Code quality pass, Frontend pass, Docker pass; Fast tests
and E2E still running at review time.

Checklist

• Matches the root cause — yes. REQUIRED["ask"] pinned the spelling
  (Bash(gh api) / Bash(gh api *)) that #657 removed while preserving the
  property; the two literals are dropped and the property stays pinned by
  command string in MUST_BE_GUARDED.
• Would the test catch a regression — yes, now for both the
  space-separated and attached-value spellings; verified above rather than
  taken on the body's word.
• Minimal — yes. No parameter, flag, default-fallback, second construction
  site or extra trigger; the workaround check passes. The doc edit is forced by
  the diff (it made two doc claims false), not scope creep.
• Rule compliance — clean. No Optional[x], no hasattr/getattr
  fallback, no exception-message matching, no new class, no hardcoded entity
  IDs, no secrets. Comments state WHY (the leak history), not what. No
  CHANGELOG.md entry is correct for agent tooling with no add-on-visible
  effect.
• Scope assessment stated — yes, and correctly categorised as local.

---

🟡 Nits (non-blocking)

1. Grouped shorthand still reaches allow. Same class as the fixed hole,
one axis further out. gh is cobra/pflag, so a bool shorthand may be grouped
ahead of a value-taking one — verified against gh in this environment:

$ gh api -iXPUT      → accepts 1 arg(s), received 0     (parsed; -i then -X=PUT)
$ gh api -iFbody=hi  → accepts 1 arg(s), received 0
$ gh api --nosuchflag → unknown flag: --nosuchflag      (control)

and through decide() on this branch:

gh api repos/o/r/pulls/1/merge -iXPUT       → allow
gh api repos/o/r/pulls/1/comments -iFbody=hi → allow

Not a regression this PR introduces (identical on main), and no agent or
script emits that spelling, so I would not hold the merge for it. But it is the
concrete shape of the "standing risk" the new doc paragraph names, and it means
the rule the doc adds at docs/agents/local-agent-environment.md:145-148 —

in both argument positions and both the --flag value and --flag=value
spellings

— is still one axis short of complete. Either widen the instruction or say
explicitly that grouped shorthand is an accepted residual, so the next person
doesn't read the list as exhaustive.

2. Six now-dead rules in .claude/settings.json. The new attached-value
globs are strict supersets of the older space-separated twins:
Bash(gh api -X*) (line 161) subsumes Bash(gh api -X *) (145),
-f* (163) subsumes -f * (149), -F* (165) subsumes -F * (151), plus the
three * variants. Nothing breaks — ask is ask either way — but this file
is the guard's readability surface, and quality-check.sh says of exactly this
list: "a rule absent from here is a rule that can be silently removed." Six
entries that can never fire make the next audit harder. (Same for
--field * + --field=*, which a single --field* would cover.)

3. PR body still describes scope that is not in the diff. The Scope
assessment says:

Six type annotations in test_agent_permissions.py. The file had none

git diff origin/main...pr/665 -- backend/tests/test_agent_permissions.py is
+25/-0 and contains only the new test cases and their comments — the
annotations are on main already. Stale text carried over from 711ff72, not
a code problem.