detect-review-request: post-merge findings from reviewing #341

[dem-extra1]

Post-merge review of #341 (the matcher itself is a clear improvement -- these are follow-ups, not a request to revert).
Each item below was verified against main at 16c77f5 by running the script and the new test table locally.

1. The widening opens a false-positive class the design rationale was written to prevent

The closed-set argument in detect-review-request.sh constrains what may appear before review, but nothing constrains what follows.
All four of these were false under the old @claude[[:space:]]+review pattern and are true now:

@claude can you review this and also fix the failing test?
@claude please review my reasoning in the issue description above
@claude, can you review why the coverage job is flaky and patch it?
@claude could you review the docs and update them if wrong

These are ordinary requests to the agent that happen to use "review" as a verb.

Consequence chain, confirmed at .github/workflows/claude.yml:844: match == 'true' on a PR suppresses the "Post Claude's response if no code was committed" step.
The agent still runs and can still push commits, so a "review and fix" request is not fully lost -- but any prose-only answer is swallowed, and a paid review run is spent on top of the agent run.
@claude please review my reasoning ... is the pure case: routed to a read-only PR code reviewer, question never answered, no signal to the requester.

This is the same harm the closed-set design was chosen to avoid, arriving through the other end of the pattern.
The test table has no case of this shape, so the contract it documents does not cover it.

Two ways out:

• Constrain what may follow review -- a terminator, or a short object such as this / this PR / the latest push. This keeps every currently-passing case (@claude please review this PR, @claude would you review the latest push) while rejecting the four above.
• Or keep the behavior and add these cases to run-detect-review-request-tests.sh as accepted, so the trade is on record rather than untested.

2. sed 's/\r$//' is dead code, and the one construct with a portability hazard

Removing the clause and re-running the full 27-case table plus CRLF probes (@claude, please review\r\n\r\nthanks\r, > @claude review\r\n> yes\r) gives identical results on every case.

It cannot matter: \r is already in [[:space:]] for the separator class and in [^[:alnum:]] for the trailing word-boundary class, and the blockquote deletion anchors at line start.

Meanwhile \r in a sed BRE is a GNU extension -- BSD/macOS sed reads it as a literal r and strips trailing rs from every line.
Given #341 rejected jq --raw-output0 specifically because runs-on is a consumer-settable input, this is an inconsistency in its own stated standard.

Simplest fix is deletion (CLAUDE.md item 9, unnecessary complexity).

3. Same standard, but this one is load-bearing: base64 -d in the composite

.github/actions/detect-review-request/action.yml's decode loop uses base64 -d.
GNU coreutils accepts -d; older BSD/macOS base64 wants -D.

Unlike item 2 this is on the live path for the late-comment rescan, and per the composite's deliberate no-|| true policy a decode failure reddens the whole calling job.
Either use a portable decoder, or state the GNU-coreutils assumption next to the jq-version note it already sits beside, so the two portability arguments in that file agree.

Also noted, lower priority

• Contradictory failure policies across the late-dispatch path. The "Collect comments posted after the trigger" step ends || : + touch so a transient gh api error degrades to "no late review" rather than failing the run. The very next step does the opposite by design, under set -euo pipefail with no continue-on-error -- so the same optional nicety can still redden the claude job. Low probability given the touch, but continue-on-error: true on the detect step would keep the red-step annotation while letting outputs.match come back empty, matching the collect step's policy.
• Observability regression. The old step printed No late @claude review requests. / Found N late .... A false result now logs nothing anywhere -- the dispatch step simply renders as skipped.
• Fenced and inline code are not stripped, though blockquotes now are. Both of these match: The docs say to comment `@claude review` on the PR and a fenced block containing @claude review. Not a regression (the old pattern matched them too), but it is the sibling of the citing-not-requesting case Recognize punctuated and polite @claude review requests #341 just added handling for, and this repo's own PR threads discuss the trigger phrase in backticks constantly.

[github-actions]

👀 Picked up by workflow run #30379008553. Setup runs first; Claude itself responds after that.

[github-actions]

Opened draft PR up front: #347 — Claude is working on it now.

[dem-extra1]

The code-span item fired for real, and #348 partially fixes it

(Every mention below is written @BOT rather than the literal bot handle -- quoting the real one in this comment would trigger another run, which is half the point.)

The last "also noted" item above -- code spans are not stripped, though blockquotes now are -- stopped being theoretical. My self-review comment on #348 discussed the matcher, quoting @BOT review ... in backticks several times, and that comment dispatched a review.

Run 30381851377, step by step:

step	conclusion
Detect @BOT review request	success (matched)
Post the agent's response if no code was committed	skipped
Dispatch code review on @BOT review comment	success

So the documented consequence played out exactly: the match suppressed the agent's reply and spent a review run. Cost was one agent run plus one dispatched review (which then failed on the ongoing SDK outage, adding two red checks).

The practical argument for prioritizing this is that it is self-perpetuating. Any comment quoting the trigger phrase re-triggers it, so the matcher cannot be discussed, documented, or reviewed in its own repository without firing. That is a maintenance tax on exactly the people working on it.

#348 narrows the hole more than expected

The end-of-line requirement in #348 was aimed at a different problem, but it incidentally kills most code-span false positives, because inline code usually sits mid-sentence. Testing five real fragments from that comment against both patterns:

fragment	released matcher	with #348
so comment @BOT review on the PR still matches.	match	no match
``	@BOT review the changes I just pushed	dispatch
`@BOT review` to request a review.	match	no match
- `@BOT review this PR`, `@BOT please review the latest changes`	match	match
Try this: `@BOT review`	match	match

Three of five stop matching. The two that survive are the ones where the code span happens to end its line -- which is also the shape a fenced block always has.

So the remaining gap is narrower and more specific than this issue originally described: it is code spans and fenced blocks at end of line, not code spans generally. Worth restating that way before anyone implements it, since a fix aimed at the general case would be doing more work than the residue justifies.

Suggested direction, unchanged in kind: strip inline code spans and fenced blocks in normalize_body, the same category of transformation as the existing blockquote strip, with the fragments above as test cases. Still out of scope for #348.

---

Generated by Claude Code