fix: BOT_REVIEW posted nothing when /code-review wrote its POST flag-first

[springfall2008]

The bug

The triage bot's BOT_REVIEW-on-PR flow ran, produced findings, and posted none of them — then cleared its own label as if it had succeeded.

PR #4758 is what that looked like: /code-review 4758 high --comment ran to completion, produced nine findings, had every posting call denied, printed the findings into logs/pr-4758-review.log instead, and exited 0. process_bot_review_pr read the zero exit as success and removed BOT_REVIEW. The PR merged with "reviews": 0, "comments": 0, "labels": [].

Root cause

The scoped grant for the review and cleanup flows was a single prefix glob:

_REVIEW_EXTRA_ALLOWED = [f"Bash(gh api repos/{REPO}/*)"]

Permission rules match a literal command prefix, so this only fires when the endpoint is the very next token, bare. /code-review --comment writes its inline comments as gh api --method POST repos/... — the canonical form for a POST — which misses the rule and is denied outright under dontAsk.

PR #4759 posted successfully the same evening purely because its POST happened to be endpoint-first. Which of the two reviews landed came down to the command spelling the model picked.

The existing comment above the rule had already predicted this: "Prefix-glob matching can't parse flags, so this is a heuristic, not a guarantee."

Verification

Ran claude -p --permission-mode dontAsk against both allowlists with the same command, using a deliberately non-existent endpoint so a permitted call creates nothing:

Allowlist	gh api --method POST repos/springfall2008/batpred/…
before	denied by the permission layer, never reached GitHub
after	permitted → HTTP 404 from GitHub

Also confirmed from the session transcripts that a quoted endpoint (gh api "repos/...") is denied for the same reason, and that redirecting output outside the clone or scratch directory is denied while piping into an allowlisted reader is fine.

The fix

Enumerate the realistic command spellings — {bare, --method POST/PATCH, -X POST/PATCH} × {bare, ", '}. Only POST and PATCH (the flow creates and edits comments; it never needs DELETE or PUT), and every variant stays pinned to this repo — this widens the accepted spelling, not the reach.

Pin the form with --append-system-prompt on both flows holding the grant. /code-review is a built-in skill, so this can't live in a SKILL.md we own. It also asks for a denial to be stated plainly rather than quietly degraded into printing the comments that couldn't be posted.

Verify before labelling — pr_review_activity_count() samples reviews, inline comments and PR comments either side of the run. An unchanged count now means BOT_FAILED with a specific reason, because a zero exit status cannot show whether the posting step was permitted.

The first two are belt and braces: the allowlist covers the spellings we enumerated, the prompt keeps the agent on the one that's certain to be covered.

Tests

24 new tests, written before the fix and confirmed failing for the right reasons — the denied command forms, the repo pinning, the absence of DELETE/PUT, the appended prompt on both flows, the activity counter's pagination and empty-response handling, and the posted-nothing path.

108/108 pass. pre-commit clean on both files (ruff, black, cspell, the daemon test hook).

Note for deploying

The running daemon won't pick this up on its own — sync_repo() resets the clone on disk, but the already-loaded triage_daemon module stays as it was. It needs a restart after this merges.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The new activity-count verification introduces uncaught failure paths (and the appended prompt text is now misleading vs the expanded allowlist), which can leave BOT_REVIEW stuck/retried or confuse the agent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the triage daemon’s BOT_REVIEW PR-review flow so /code-review … --comment reliably has permission to post its findings (regardless of gh api flag ordering / quoting), and so the bot doesn’t clear BOT_REVIEW when nothing was actually posted.

Changes:

• Expand the scoped gh api allowlist to cover realistic POST/PATCH spellings (endpoint-first, method-flag-first, and quoted endpoint variants) while staying pinned to springfall2008/batpred.
• Append a system prompt to /code-review and /pr-cleanup runs to steer the agent toward the endpoint-first gh api form and to require explicit reporting on permission denials.
• Add pr_review_activity_count() and use it in process_bot_review_pr() to detect “exited 0 but posted nothing” and mark such runs as BOT_FAILED instead of removing BOT_REVIEW.

File summaries

File	Description
tools/triage_daemon.py	Broadens the scoped gh api allowlist, adds an appended prompt to guide gh api usage, and verifies posted activity before clearing BOT_REVIEW.
tools/test_triage_daemon.py	Adds regression tests covering the expanded allowlist forms, the appended prompt, and the new activity-count verification logic.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The current repo-scoped gh api allow patterns still permit destructive verbs when flags are placed after the endpoint, and the related test asserts a guarantee that the permission model does not actually enforce.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tools/test_triage_daemon.py:491

• test_scoped_gh_api_grants_never_allow_destructive_methods currently checks that the allowed tool patterns don’t contain the strings "DELETE"/"PUT", but the endpoint-first patterns still allow a caller to append -X DELETE / --method PUT after the endpoint (so the absence of those substrings in the allowlist doesn’t actually prove the verbs are blocked).

If you add explicit destructive-method denials (recommended), the test should assert those denials are present in DISALLOWED_TOOLS_REVIEW/DISALLOWED_TOOLS_CLEANUP instead.

        """Only POST and PATCH are enumerated - the review flow creates and edits comments,
        it never needs DELETE or PUT, and spelling those out would hand it the REST routes to
        remove reviews or replace branch contents."""
        for flow in [triage_daemon.ALLOWED_TOOLS_REVIEW, triage_daemon.ALLOWED_TOOLS_CLEANUP]:
            for entry in flow.split(","):

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite