Add a Stop hook catching a PR opened without a reviewer request

[dem-extra1]

Closes #1041.

What happened

I opened #1038 and #1040, wrote "Still owed: reviews requested on #1038 and #1040" into a status report, and moved on. Neither had a reviewer.

The prose rule to always request one already existed --- so the rule is what failed, and a stronger rule would fail the same way. This is the mechanism.

Why it is invisible, not just careless

Opening a PR auto-triggers the repo's own review workflow but does not summon Copilot. So review genuinely is in motion for that half, and the PR shows review-shaped activity in its checks while nothing has read the diff. The half that runs is what disguises the half that doesn't.

Compounding it: claude-review currently fails repo-wide on context size (#897), so Copilot is often the only working reviewer. Measured --- #1038 and #1040 sat with zero reviews for ~10 minutes until the user asked, and claude-review had failed on every attempt across #1029, so neither would have been reviewed by anything.

The check

Exactly decidable from the transcript, which is what makes it a hook rather than a rule to remember:

a PR was created or readied this session
AND no reviewer request came after it

Modelled on hooks/no-stale-pr-status.py: same Stop-hook shape, transcript scan, fail-open, once-per-message sentinel.

The draft carve-out is load-bearing

A draft PR deliberately defers review --- a draft doesn't trigger the review bot (pr-on-claim). A guard that fired on drafts would be wrong on the commonest correct workflow in this repo, get switched off, and take the real case with it.

Ordering is covered both ways: drafting then readying re-arms the guard; readying then converting back to draft (the draft-gating in CLAUDE.md) silences it --- but only when the conversion actually succeeds (a failed draft:true/--undo keeps the PR tracked).

Round 3: a rewrite of the identity + correlation model

Round-3 review (Copilot + claude) reproduced four correctness bugs in the round-1 scalar/number-keyed model. scan() was rewritten around a per-obligation list keyed by PR identity resolved from tool_use results, fixing all four:

• Wildcard key never matched. A gh pr create command carries no PR number --- the number only appears in its result --- so the old model keyed the open "*" and a later numbered request never cleared it. The open is now keyed from its own result (URL or number field), correlated by tool_use id.
• Positional tool_result correlation. Results are now matched to their originating tool_use by id, so an unrelated 4xx result batched with a real request can no longer misattribute failure to it.
• Read-only GET discharge. A bare gh api .../requested_reviewers (a GET) no longer counts as a request; only a mutating POST or a --reviewer/--add-reviewer/request_copilot_review form does. (The short -r reviewer flag is now matched case-sensitively so it does not collide with -R, the repo flag.)
• Multi-repo identity. Obligations carry owner/repo, so the same PR number in two repositories is two obligations.

The test fixtures were rewritten to mirror real transcripts --- every tool_use carries an id, every result references it, and the create fixtures keep the PR number out of the command and in the result, the exact shape the old model got wrong.

Rounds 4-6: quoted/embedded commands

This repo's own docs and this hook's own block-reason text are full of literal gh pr create / requested_reviewers -X POST examples, so a gh pr comment --body "...", a heredoc, a herestring, or a bare echo quoting one could forge an obligation or --- worse --- silently discharge a real, unreviewed one. Two matching surfaces, defended differently because they key on structurally different things:

• Open/draft detection blanks every quoted span and heredoc body before matching (_scrub_all), since gh pr create/ready is always a leading command word, never legitimately quoted.
• Request detection is structural (request_ident): the command is split into simple commands on shell operators, each tokenized with shlex, and the request tokens (requested_reviewers+-X POST, --add-reviewer, --reviewer) count only as the argv of an actual gh api/gh pr edit/gh pr create invocation --- never the value of a string argument to echo/gh pr comment/a heredoc/herestring. Identity comes from the request command itself. \-continuations are joined first so the hook's own multi-line recovery command parses as one request. Parsing fails toward not-a-request, so it never silently discharges.

Rounds 7-11: attributing a combined result blob

A single Bash tool_result (or a combined structured call) is one opaque blob covering possibly-several actions, so a failure anywhere in it cannot be cleanly attributed to a specific one, and any state change made on a tool_use before its result is known can be wrong if that result fails. Rounds 7-11 (plus a follow-up sweep) converged on this --- every one a dangerous-direction (silent-discharge) or safe-direction (over-nag) bug reproduced against scan():

• Round 7 (dangerous): a trailing request's failure was read as the create failing, silently dropping a genuinely-opened, unreviewed PR.
• Round 8 (safe): an unrelated command's shell noise suppressed a successful request's discharge (nag). Round 9 (safe): is_error is the whole call's exit status, so a successful request followed by a failing command still nagged. Round 10 (dangerous): the round-9 fix dropped is_error for the gh api form, so a sole request failing with a non-4xx error (network/5xx/auth) silently discharged.
• Round 11 (dangerous): the round-7 create-attribution drop also fired for update_pull_request, whose PR number is known from the input --- so a draft=False + reviewers=[...] edit whose reviewer-add failed with a bare {"status":422} silently discharged a genuinely-ready PR.
• Follow-up (dangerous): a draft transition (gh pr ready --undo / update_pull_request(draft=true)) cleared the obligation at tool_use time --- so a conversion that failed left the PR ready but forgotten. Now deferred to the transition's own non-failed result.

Rounds 8-10's discharge patches were replaced by one fail-safe invariant. A discharge asserts the PR got a reviewer, so it fires only on positive evidence the request itself succeeded. The one reliable success signal is is_error, authoritative only when the request is the last simple command (its exit status is the whole call's) or the call is a single atomic structured tool:

req_failed = (not last) or err or RX_REQ_FAILED(body)

• A sole/last request discharges only if it neither errored nor returned a 4xx body --- so a non-4xx failure now blocks (round 10 fixed), and one rule covers every request form.
• A request chained ahead of anything is ambiguous (is_error belongs to a later command) so it never discharges --- a deliberate over-warn, since over-warning is annoying but silently losing protection defeats the hook.

The two state-change-on-tool_use paths were made result-gated the same way: the round-11 resolution drop is scoped to when the PR's identity is unknown from every source (failed and rnum is None and ob["num"] is None), and every draft transition clears its PR only on a non-failed result.

Verification

55 tests, mutation-checked --- reverting result-keying, id-correlation, the POST-only request check, the owner/repo identity, the draft ordering, the draft exemption, the transcript-scoped sentinel, the round-6 structural parse, the round-7 create-attribution fix, the round-11 identity-scoped resolution drop, the result-gated draft clear (both sites), or any of the three terms of the round-10 discharge invariant (the last-command guard, the ambiguity term, or err) each fails exactly its own case.

Not activated

Per README.md line 273: authoring a hook needs no permission, registering it in ~/.claude/settings.json waits for this to merge. I have not run install-hooks.py --fix.

[Copilot]

🟡 Not ready to approve

The hook conflates events across PRs and contains several false-positive and false-negative matcher paths.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds a Stop hook intended to prevent ready pull requests from ending a session without a reviewer request.

Changes:

• Adds transcript-based PR and reviewer-request detection.
• Exempts draft pull requests.
• Adds hook registration metadata and 14 test cases.

File summaries

File	Description
hooks/no-unreviewed-pr.py	Implements the Stop hook.
hooks/test-no-unreviewed-pr.py	Tests request, draft, and ordering behavior.
hooks/hooks.json	Registers the hook’s event metadata.

Review details

Suppressed comments (5)

hooks/no-unreviewed-pr.py:112

• These comparisons are global rather than PR-specific. If PR A and PR B are opened and only A receives a later review request, last_request > last_open can allow the reply even though B remains unreviewed; likewise, drafting an unrelated PR can clear the obligation for a ready PR. Track outstanding review obligations by PR identity (or at minimum by unmatched open/request events) and require every ready PR to be discharged.

    if last_request > last_open:
        return 0
    # The most recent action was drafting, which legitimately defers review.
    if last_draft > last_open:
        return 0

hooks/no-unreviewed-pr.py:52

• The CLI operation that converts a ready PR back to draft is gh pr ready --undo, but this pattern does not recognize it. Consequently the documented draft-gating workflow still blocks at Stop; add the actual CLI form and a regression case using it.

RX_DRAFT = re.compile(r"\"?draft\"?\s*[:=]\s*true|--draft\b", re.I)

hooks/no-unreviewed-pr.py:115

• The sentinel key contains only the reply text, while the file lives in the process-wide temporary directory. A later session that happens to use the same common recap (for example, “Opened the PR.”) will find the old sentinel and silently skip the guard. Include a session-unique value such as transcript_path in the key.

    key = hashlib.sha256(text.encode()).hexdigest()[:16]
    sentinel = os.path.join(tempfile.gettempdir(), f".claude-unreviewed-pr-{key}")

hooks/no-unreviewed-pr.py:82

• Scanning every tool's serialized input for command substrings creates false opens: an rg/search call for create_pull_request, an echo 'gh pr create ...', or a failed create attempt all set last_open. This Stop hook will then block an ordinary reply even though no PR exists. Match harness tools by exact tool name and correlate CLI invocations with successful tool results rather than treating arbitrary input text as proof of creation.

                    blob = (b.get("name") or "") + " " + json.dumps(
                        b.get("input") or {})
                    # Draft is checked first: `gh pr create --draft` matches
                    # both patterns, and it is the draft that decides.
                    if RX_DRAFT.search(blob):

hooks/no-unreviewed-pr.py:135

• This user-facing block reason repeats the unsupported claim that claude-review fails repo-wide due to context size. #1029 explicitly records successful runs on other branches and an unconfirmed cause, so the hook would present speculation as fact. Use a conditional, durable explanation instead.

            "diff. In this repo `claude-review` currently fails repo-wide on "
            "context size (ai-config#897), so Copilot is often the only "
            "working reviewer and the PR gets no review at all.\n\n"

• Files reviewed: 3/3 changed files
• Comments generated: 4
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The hook can miss unreviewed PRs and misclassify valid draft or reviewer operations.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (10)

hooks/no-unreviewed-pr.py:85

• The scanner applies these patterns to the serialized input of every tool, not just an actual Bash command or the matching GitHub tool. An Edit/Write call whose file content mentions gh pr create will therefore create a fake obligation, while content mentioning requested_reviewers can falsely discharge one. This is the exact documentation/heredoc false-positive shape warned about in README.md:265-271; gate CLI matches on the Bash command field and MCP matches on the exact tool name plus structured arguments.

                    blob = (b.get("name") or "") + " " + json.dumps(
                        b.get("input") or {})
                    # Draft is checked first: `gh pr create --draft` matches
                    # both patterns, and it is the draft that decides.
                    if RX_DRAFT.search(blob):
                        last_draft = i
                    elif RX_OPEN.search(blob):
                        last_open = i

hooks/no-unreviewed-pr.py:87

• A request attempt is treated as successful without inspecting its tool result. For example, the documented 422 response when the PR author is the requested reviewer still advances last_request, so the hook permits the session to stop with no reviewer attached. Correlate each request with a successful tool result or a subsequent verification result before clearing the obligation.

                    if RX_REQUEST.search(blob):
                        last_request = i

hooks/no-unreviewed-pr.py:111

• These scalar timestamps lose obligations when a session handles multiple PRs. Opening A, opening B, then requesting review only for B makes last_request > last_open and silently forgets A; similarly, opening ready A and then opening draft B makes last_draft > last_open silence A. Track obligations by PR identity and clear only the matching PR, then block while any non-draft PR remains unreviewed.

    # A request after the open is exactly what discharges the obligation.
    if last_request > last_open:
        return 0
    # The most recent action was drafting, which legitimately defers review.
    if last_draft > last_open:

hooks/test-no-unreviewed-pr.py:96

• This fixture does not convert the ready PR back to draft: DRAFT_TOOL is another create_pull_request call, so it opens a second draft PR. The test therefore blesses the global-state bug where a later draft silences an earlier ready PR. Use update_pull_request with draft: true for the same PR, and add a separate two-PR case proving that a new draft does not clear the first PR's obligation.

    ([CREATE_CLI, DRAFT_TOOL, say("Held as a draft behind #1029.")], False,

hooks/no-unreviewed-pr.py:141

• Both commands are invalid when copied with these placeholders because Bash treats an unquoted < as redirection, including inside the API path. Quote every placeholder-bearing argument so the recovery instructions can actually run.

            "    gh api repos/<owner>/<repo>/pulls/<N>/requested_reviewers \\\n"
            "      -X POST -f 'reviewers[]=copilot-pull-request-reviewer[bot]' "
            "\\\n      --jq '.requested_reviewers[].login'\n\n"
            "Then verify it landed:\n\n"
            "    gh pr view <N> --json reviewRequests "

hooks/no-unreviewed-pr.py:13

• The cited #897 does not establish this scope or cause: it calls prompt overflow only the leading hypothesis, says the literal error is unconfirmed, and reports 15 successful runs on six other branches. State the observed failures without presenting a repo-wide context-size diagnosis as fact.

That matters more in this repo than it would elsewhere, because
`claude-review` currently fails repo-wide on context size (ai-config#897).
When it is down, Copilot is the ONLY working reviewer, so a PR opened without
an explicit request gets no review at all.

hooks/no-unreviewed-pr.py:135

• This repeats a diagnosis that #897 explicitly leaves unconfirmed: context size is only the leading hypothesis, and the cited record includes successful runs on other branches. Keep the warning grounded in the observed review failures rather than asserting a repo-wide cause.

            "diff. In this repo `claude-review` currently fails repo-wide on "
            "context size (ai-config#897), so Copilot is often the only "
            "working reviewer and the PR gets no review at all.\n\n"

hooks/hooks.json:102

• The manifest also hardens the unconfirmed #897 hypothesis into fact. #897 reports branch-specific failures alongside successful runs elsewhere and does not confirm context size as the cause, so this rationale should describe the observed failures without claiming a repo-wide diagnosis.

      "why": "CLAUDE.md 'Open a PR for every pushed feature branch' + skills/ardi 'always request another review' -- opening a PR auto-triggers the repo's own review workflow but does NOT summon Copilot, and that auto-triggered half is what disguises the missing one. With claude-review failing repo-wide on context size (ai-config#897), an unrequested PR gets no review at all."

hooks/no-unreviewed-pr.py:44

• The request surfaces are inverted here: supported forms such as gh pr create --reviewer ... and the repository's documented update_pull_request with a reviewers argument are not recognized, while gh pr review --request is not a valid GitHub CLI option. This causes false blocks after real requests and lets a failed nonexistent command look like a request. Detect the supported create/edit/MCP forms structurally and remove the nonexistent form.

RX_REQUEST = re.compile(
    r"requested_reviewers|request_copilot_review|"
    r"gh\s+pr\s+edit[^\n]*--add-reviewer|"
    r"gh\s+pr\s+review[^\n]*--request",

hooks/no-unreviewed-pr.py:52

• The standard CLI operation for converting a ready PR back to draft is gh pr ready --undo. It does not match this draft detector, so it is instead treated as an open action by RX_OPEN and the advertised draft-gating carve-out behaves in exactly the wrong direction for CLI users. Classify this form as a draft action and add a regression case.

This issue also appears in the following locations of the same file:

• line 78
• line 86
• line 107
• line 137

RX_DRAFT = re.compile(r"\"?draft\"?\s*[:=]\s*true|--draft\b", re.I)

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

PR identity, request-result correlation, read-only API matching, and sentinel scope can produce both missed and spurious blocks.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

hooks/no-unreviewed-pr.py:53

• A read-only gh api repos/o/r/pulls/1038/requested_reviewers still matches the first alternative. Because that GET succeeds, the next tool_result removes the outstanding obligation even though no reviewer was requested. Match this endpoint only when the command is mutating (for example, POST/a reviewer field), and add the negative GET case that the prior review called for.

RX_REQUEST = re.compile(
    r"requested_reviewers|request_copilot_review|"
    r"gh\s+pr\s+edit[^|;&]*--add-reviewer|"
    r"gh\s+pr\s+create[^|;&]*(?:--reviewer|-r\s)",
    re.I,

hooks/no-unreviewed-pr.py:149

• Real gh pr create calls do not contain the new PR number; the number arrives in the command result (as request-pr-review/SKILL.md:33-34 notes). This therefore records "*", while the subsequent reviewer request is keyed by the returned number, so a successfully reviewed PR still remains outstanding and the Stop hook blocks. The structured create tool has the same shape. Correlate the create result and replace the provisional key with the returned PR identity; the tests currently hide this by appending # pulls/1038 to every create command.

                    if name in OPEN_TOOLS:
                        if not inp.get("draft"):
                            open_prs[pr_key(blob)] = True
                        continue

hooks/no-unreviewed-pr.py:138

• Every tool_result is applied to every pending reviewer request, without checking its tool_use_id. If a reviewer request is issued alongside another tool call, the unrelated result can arrive first; an unrelated success then clears the request, or an unrelated failure prevents a successful request from clearing it. Track pending requests by tool-use ID and consume only the matching result.

                if kind == "tool_result":
                    # Only a SUCCESSFUL request discharges. A 422 still
                    # produces a tool_use, so trusting the attempt alone lets
                    # the session stop with no reviewer attached.
                    body = json.dumps(b.get("content") or "")
                    for _, key in pending:
                        if not RX_FAILED.search(body):
                            open_prs.pop(key, None)
                            if key == "*":
                                open_prs.clear()
                    pending = []

hooks/no-unreviewed-pr.py:200

• This sentinel is global to the machine's temporary directory because its key omits the transcript/session path. If a later session ends with the same text and PR label, the sentinel from the earlier session silently suppresses the guard. Include the transcript path in the hash, as remind-ums-after-error.py does for per-session sentinel scope.

    key = hashlib.sha256((text + which).encode()).hexdigest()[:16]
    sentinel = os.path.join(tempfile.gettempdir(), f".claude-unreviewed-pr-{key}")

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[claude]

Code review

Re-reviewed hooks/no-unreviewed-pr.py and hooks/test-no-unreviewed-pr.py at head 5bcf293 (7 new commits since the last reviewed head 9ac0cb18).

Round 10's finding is fixed. The known-identity numberless-failure drop condition now reads if failed and rnum is None and ob["num"] is None and not ob["self"] (guarding on ob["num"], not just rnum), so an update_pull_request obligation whose identity is already known from pull_number is no longer dropped on a numberless failure body. Traced through the exact scenario round 10 raised and confirmed it now keeps the PR tracked; the full 65-test suite (up from 51) passes.

I also traced through the new draft-transition commits (scoping _argv_draft to real gh invocations, dropping the gh api -f draft=true branch since REST PATCH can't convert draft state, and guarding the deferred draft-clear with the same last-command ordering as the request discharge) against several adversarial orderings (a draft-token decoy before/after the real transition, a bare gh pr create --draft racing a failed --undo) -- all traced correctly to the safe outcome, matching the shipped tests.

However, I found a new, reproducible silent-discharge bug -- filed as an inline comment on hooks/no-unreviewed-pr.py:613-632. Summary: for update_pull_request(pull_number=N, draft=False, reviewers=[...]), the same tid is registered in both obligations (self-flagged, discharged via the broad RX_FAILED regex) and pending (discharged via the narrower RX_REQ_FAILED regex). When the reviewer-add sub-action genuinely fails with plain-language text ("failed"/"error"/"not found") but no explicit 4xx/HTTP shape, and is_error is unset, the broad check correctly keeps the obligation tracked while the narrow pending discharge fires _clear() on the same, still-present obligation immediately afterward -- silently discharging a genuinely-unreviewed PR. This is the exact dangerous class every prior round of this PR has treated as blocking. The existing update_pull_request failure test doesn't catch it because it uses a body with an explicit "status":422 and err=True, either of which independently masks the discrepancy.

I traced this via careful static reading of the exact code paths (execution/testing tools were unavailable in this review sandbox), with precise line references and a worked example in the inline comment -- I'd encourage verifying it against scan() directly before dispositioning.

Verdict

Needs more work.

[github-actions]

Claude finished review — View run

---

Posted successfully. Summary of this review round:

Findings posted:

1. Inline comment on hooks/no-unreviewed-pr.py:613-632 — a new, reproducible silent-discharge bug: update_pull_request(draft=False, reviewers=[...]) double-registers the same tool_use_id in both obligations (discharged via the broad RX_FAILED regex) and pending (discharged via the narrower RX_REQ_FAILED regex). When a reviewer-add sub-action genuinely fails with plain-language text ("failed"/"error"/"not found") but no explicit 4xx/HTTP shape, and is_error is unset, the broad check correctly keeps the obligation tracked while the narrower pending discharge overrides it and silently clears it — reporting a genuinely-unreviewed PR as reviewed.

Verified fixed: Round 10's finding (the known-identity numberless-failure drop condition) is correctly resolved in the current head, and the shipped 65-test suite passes.

Verdict: Needs more work (posted to the PR, along with the inline finding).

One caveat worth flagging to you directly: this sandbox had no working git fetch, gh api, Write, or Bash-file-write access, so I couldn't execute the hook's own test harness against a new adversarial case the way prior review rounds did ("I reproduced this directly against scan()"). I instead traced the bug through the exact source lines by hand and cross-checked it against the shipped test fixtures to confirm the gap is real and untested. I noted this limitation explicitly in the posted review so the confirmation step isn't skipped.

[github-actions]

💰 Cost: $7.6437 (review) — run

[claude]

Code review

Re-reviewed hooks/no-unreviewed-pr.py, hooks/test-no-unreviewed-pr.py, hooks/hooks.json, and README.md at head be3214d12210c4cd3196cee7fec0923b246fee3f.

CLAUDE.md / lab-manual compliance: clean. Two independent passes checked hook-registration conventions (hooks.json/README.md shape matches sibling hooks), banned-punctuation rules, fail-fast.md's silent-failure requirements (the module's except Exception: blocks are documented, deliberate, and match the identical pattern in existing merged hooks), one-function-per-file.md (defensible under the existing single-file-per-hook convention), and algorithmatize-checks.md/deterministic-tools.md. No violations found.

Bugs — two new, confirmed, reproducible issues, both posted inline:

1. _simple_commands()'s separator set is wrong in both directions (hooks/no-unreviewed-pr.py:138-167). It's missing \n as a separator (so a newline-joined request+trailing-command collapses into one simple command, flipping last from False to True and enabling a silent discharge — the dangerous direction), and it wrongly includes </> as separators (so an ordinary trailing redirect like > /dev/null on a genuinely sole, successful request makes it register as last=False, causing it to never discharge — a severe permanent-nag bug). Both reproduced end-to-end against scan(); neither is covered by the 51-test suite, which only exercises ;/&&/||/\-continuation.

2. A bare gh pr ready (no PR number — ordinary usage from the PR's own branch) creates a permanently undischargeable obligation (hooks/no-unreviewed-pr.py:422-434). RX_CMD_VERB requires a digit, so the command never yields a number; gh's actual owner/repo#N success text isn't recognized by any of the result-parsing regexes either, so the number never backfills. _clear() then refuses to discharge any obligation with num is None — so this specific obligation can't be cleared by any subsequent successful reviewer request, by a re-ready, or by --undo. This directly contradicts the module's own docstring claim (L41-42) that the hook "cannot wedge a session," and the resulting block message can't even name which PR it's complaining about (named is empty).

A third candidate (RX_REQ_FAILED's "status": 4xx alternative can't match the escaped-quote shape json.dumps() produces for string/text-block tool-result content, making that alternative effectively dead code) was investigated and confirmed as a real inconsistency, but not posted as a blocking finding — for the actual gh CLI path, a failing request also sets is_error (non-zero exit) or matches the quote-free HTTP 4\d\d alternative, so it's caught in practice by defense-in-depth. Worth a cheap fix (mirror RX_RES_NUM's \\?\" pattern on both quotes) but not merge-blocking.

Both confirmed bugs sit in code that predates this round's own commits (not introduced by the latest push) but were not flagged in any prior review round, and both are reproducible with ordinary, non-contrived shell usage rather than adversarial input.

Verdict

Needs more work.

[github-actions]

Claude finished review — View run

---

Review complete and posted to PR #1042.

Summary

CLAUDE.md / lab-manual compliance: Clean — two independent passes found no violations (hook registration conventions, banned-punctuation rules, fail-fast documentation, one-function-per-file, algorithmatize-checks).

Confirmed bugs (posted inline, both independently reproduced by execution against scan()):

1. _simple_commands()'s separator set is wrong in both directions (hooks/no-unreviewed-pr.py:138-167) — missing \n as a separator causes a silent discharge (dangerous direction) when a request and a following command are newline-joined instead of ;-joined; wrongly treating </> as separators causes a genuinely successful, sole request with an ordinary trailing redirect to never discharge (permanent nag).
2. Bare gh pr ready (no PR number) creates a permanently undischargeable obligation (hooks/no-unreviewed-pr.py:422-434) — no code path can ever resolve or clear it, directly contradicting the module's own "cannot wedge a session" claim.

A third candidate (RX_REQ_FAILED's escaping issue) was investigated and confirmed as a real but non-exploitable inconsistency (defense-in-depth via is_error and the HTTP 4xx alternative catches it in the actual gh CLI path) — mentioned in the summary as a minor non-blocking note, not posted as a blocking finding, per the high-signal bar.

Verdict: Needs more work.

[github-actions]

💰 Cost: $14.7605 (review) — run

[github-actions]

Claude finished review — View run

---

Code review summary — #1042 (head 97bf7d475a8e4a698a1f32a9ad1d7ac9f13ace77)

I re-verified this PR against the full prior review history (10 rounds shown in context) plus the 9 commits pushed since the last shown review (round 10, head 9ac0cb18). All previously-found bugs (wildcard PR keys, positional tool_result correlation, GET-vs-POST discharge, quoted/heredoc forgery, the round-7 through round-11 discharge-attribution bugs) are correctly fixed at the current head — I traced the code and ran the shipped test suite (python3 hooks/test-no-unreviewed-pr.py hooks/no-unreviewed-pr.py): 75/75 tests pass.

However, I found one new, high-confidence, reproduced bug in code that has not been reviewed in any prior round (it's in the tail of commits pushed after round 10, specifically the identity-resolution path for gh pr create/gh pr ready opens).

Finding: cmd_ident() is not scoped to the actual open/ready command, so a decoy PR number earlier in a chained shell command silently misattributes — and later discharges — the wrong PR's obligation

Location: hooks/no-unreviewed-pr.py, cmd_ident() at lines 415–427, consumed at line 696 and used to build the obligation at lines 719–721.

The bug: cmd_ident(cmd) runs RX_CMD_API.search(cmd) and RX_CMD_VERB.search(cmd) (line 363: \bpr\s+(?:ready|edit|view|comment|review|merge|close|diff|checks)\s+#?(\d+)) against the whole raw command string, not scoped to the specific simple command that is actually the open/ready action. Unlike request_ident/draft_ident (which structurally parse into simple commands via _simple_commands() and correctly locate identity from the matching command), the OPEN path (elif opened: obligations.append({"num": num, ...})) has no structural fallback at all — it trusts whatever cmd_ident finds first in the string.

So a perfectly ordinary chained command like:

gh pr checks 1029 && gh pr ready

— checking an unrelated/older PR's CI status before readying the current branch's own PR (bare gh pr ready, no number) — causes cmd_ident to return num="1029" (the decoy), because RX_CMD_VERB matches pr checks 1029 before it ever reaches pr ready. I confirmed this directly:

>>> hook.cmd_ident(hook._scrub_payload('gh pr checks 1029 && gh pr ready'))
('1029', None)
>>> hook.cmd_ident(hook._scrub_payload('gh pr view 1029 --json state && gh pr ready'))
('1029', None)

Since ob["num"] is now non-None from this wrong value, the later backfill guard at line ~591 (if ob["num"] is None and rnum: ob["num"] = rnum) never corrects it from the ready/create's own success message — the obligation stays permanently mislabeled.

End-to-end reproduction (dangerous direction — a genuinely unreviewed PR is silently marked reviewed):

events = [
    bash("gh pr view 42 && gh pr ready", tid="c"),
    res("c", 'Pull request o/r#1038 is marked as "ready for review"'),
    say("Checked #42, readied the current PR."),
    # later, unrelated: a genuine re-request of review on the OLDER PR #42
    bash("gh api repos/o/r/pulls/42/requested_reviewers -X POST -f 'reviewers[]=copilot-pull-request-reviewer[bot]'", tid="q"),
    res("q", '{"requested_reviewers":[{"login":"Copilot"}]}'),
    say("Re-requested review on #42."),
]
obligations, text = hook.scan(path)
# obligations == []   <-- the guard reports the session clean

gh pr ready genuinely readied PR #1038, which never had a reviewer requested. Because it was mislabeled as #42 (from the decoy gh pr view 42 earlier in the same chained command), the later legitimate — but entirely unrelated — re-request for #42 clears the mislabeled obligation via _clear(). scan() returns obligations == [], so the Stop hook does not block: the session ends with PR #1038 genuinely unreviewed and the guard reporting all-clear.

This is the exact dangerous-direction (silent-discharge) class every prior round (7–11) has treated as blocking, arriving through a code path (cmd_ident, the OPEN identity resolver) that none of those rounds exercised — the existing test suite's batched/id-correlation tests (lines ~452–474) all use separate tool_use calls with distinct tids for the decoy verb, never a single chained shell command combining a decoy verb with the open/ready action, which is the actual gap.

Suggested fix direction: scope cmd_ident's search the same way _argv_draft/_verb_ident already do — parse into simple commands via _simple_commands(), and resolve identity only from the specific simple command matching gh pr create/gh pr ready (falling back to None when that specific command carries no number, as the bare-ready case already correctly does via the result-based backfill). This is a moderate-size structural fix (mirrors existing _argv_draft code), not a one-line change, so I have not proposed a committable suggestion.

Verdict

Needs more work.

Everything else at this head is solid: all prior rounds' fixes hold, the 75-test suite passes, hooks.json's new entry is valid and consistent in shape, README's table addition is accurate, and no banned punctuation appears in the diff. The one finding above is real, reproduced, and in the dangerous (silent-discharge) direction this PR has consistently held itself to fixing before merge.

No --comment argument was provided, so per the review instructions I'm stopping here with this terminal summary rather than posting GitHub comments.

[github-actions]

💰 Cost: $9.6284 (review) — run

[dem-ucdh]

Round 8 finding (cmd_ident decoy misattribution) — confirmed and fixed in 178668d.

Reproduced against scan() first: gh pr view 42 && gh pr ready whose result is Pull request o/r#1038 is marked as "ready for review", followed by an unrelated genuine request for #42, returned obligations == [] — the real, unreviewed #1038 silently discharged because cmd_ident matched the decoy pr view 42 from the whole command string and mislabeled the obligation as #42.

Fix (your suggested direction — structural, mirroring _argv_draft): a new open_ident() locates the gh pr create/gh pr ready simple command via _simple_commands() and takes identity from that command's own argv — None for create (number is in the result), the explicit number for gh pr ready <N>, and None for a bare gh pr ready (backfilled from its own owner/repo#N result). A decoy verb chained ahead can no longer misattribute the number.

Since open_ident is structural, the whole-string cmd_ident became dead — and so did RX_CMD_VERB, RX_CMD_REPO, and _scrub_payload/RX_PAYLOAD (used only by cmd_ident), all removed. The draft and request paths also dropped their or num/or repo cmd_ident fallbacks: those were the same decoy surface with no legitimate use (structural identity already resolves every real case — e.g. gh pr edit <N> --add-reviewer gets <N> from the edit command, a gh api .../pulls/<N>/requested_reviewers from its URL). So the fix is a net removal of the whole-string-identity surface, not an addition. RX_CMD_API stays (still used by _url_ident).

Verification: the dangerous decoy stays tracked; a request for the actual readied PR (#1038) still discharges (proving identity resolved to #1038); and gh pr checks 1029 && gh pr ready 1038 takes #1038, not the decoy #1029. +3 tests, mutation-checked (matching decoy verbs as opens fails exactly those three, and nothing else). 78 pass, repo hook suite 9/9.

You're right this was a reachable dangerous-direction path none of rounds 1–7 exercised (the batched/id-correlation tests all used separate tool_use calls, never a single chained shell command combining a decoy verb with the open). Thank you for the continued rigor.

---

Generated by Claude Code

[claude]

Code review

Re-reviewed hooks/no-unreviewed-pr.py, hooks/test-no-unreviewed-pr.py, hooks/hooks.json, and README.md at head 178668d552cbe49dbda2eaa8f04754b777af5a9c.

Round 10's finding is fixed. The prior review's finding -- if failed and rnum is None: continue incorrectly dropping an update_pull_request obligation whose number was already known from pull_number input -- was fixed in 8ac62cea (failed and rnum is None and ob["num"] is None), and I re-traced both the update_pull_request(draft=False, reviewers=[...]) failure case and its success mirror through the current code: both now behave correctly.

However, I found a new, real silent-discharge bug in the self (create+reviewer) obligation path, filed as an inline comment on hooks/no-unreviewed-pr.py:572-580. Summary: the self flag on a newly-appended obligation is set from requested = request_ident(cmd_raw), which scans every simple command in the whole raw Bash string for anything request-shaped -- not just the create's own argv, and with no check that the matched request even targets the same PR being opened. Once self=True, discharge is gated only by the coarse whole-body failed flag, with no "last simple command" guard -- unlike the parallel pending[tid] path a few lines below, which explicitly requires the request to be the last simple command before trusting is_error. This is the same silent-discharge bug class that rounds 7-10 fixed for pending[tid] and that round 13 (178668d55) fixed for open_ident, just never given the equivalent guard on the self path.

I verified two concrete, hand-traced reproductions, independently confirmed by two separate review passes: (1) a gh pr create && gh pr edit --add-reviewer X; gh pr view --json url chain where the add-reviewer genuinely fails with GraphQL error text (matching none of RX_FAILED's alternatives) but the trailing gh pr view succeeds -- err=False, failed=False, obligation silently discharged despite the reviewer-add having failed. (2) An even sharper variant needing no special failure text at all: gh pr edit 999 --add-reviewer someone && gh pr create --title x --body y -- the unrelated PR #999's request satisfies requested=True for the new PR's obligation, so the brand-new PR self-discharges on ordinary success with no reviewer ever requested for it. Neither case is exercised by the 65+ existing tests. Full trace and both repro cases are in the inline comment.

I also re-confirmed the rest of the file is solid: open_ident's structural resolution (the round-13 fix) correctly handles decoy-verb chains and quoted PR numbers; the draft-clear paths (pending_clear) correctly gate on dlast/structural draft_ident even when the regex-based draft boolean and the structural detector disagree (the gh api -f draft=true no-op case); the EDIT_TOOLS withheld-pending-registration fix (5bb834d) is correct; and no banned punctuation appears in the diff.

Verdict

Needs more work.

[github-actions]

Claude finished review — View run

---

Code review

Re-reviewed hooks/no-unreviewed-pr.py, hooks/test-no-unreviewed-pr.py, hooks/hooks.json, and README.md at head 178668d552cbe49dbda2eaa8f04754b777af5a9c.

Round 10's finding is fixed. The prior review's finding -- if failed and rnum is None: continue incorrectly dropping an update_pull_request obligation whose number was already known from pull_number input -- was fixed in 8ac62cea (failed and rnum is None and ob["num"] is None), and I re-traced both the update_pull_request(draft=False, reviewers=[...]) failure case and its success mirror through the current code: both now behave correctly.

However, I found a new, real silent-discharge bug in the self (create+reviewer) obligation path, filed as an inline comment on hooks/no-unreviewed-pr.py:572-580. Summary: the self flag on a newly-appended obligation is set from requested = request_ident(cmd_raw), which scans every simple command in the whole raw Bash string for anything request-shaped -- not just the create's own argv, and with no check that the matched request even targets the same PR being opened. Once self=True, discharge is gated only by the coarse whole-body failed flag, with no "last simple command" guard -- unlike the parallel pending[tid] path a few lines below, which explicitly requires the request to be the last simple command before trusting is_error. This is the same silent-discharge bug class that rounds 7-10 fixed for pending[tid] and that round 13 (178668d55) fixed for open_ident, just never given the equivalent guard on the self path.

I verified two concrete, hand-traced reproductions, independently confirmed by two separate review passes: (1) a gh pr create && gh pr edit --add-reviewer X; gh pr view --json url chain where the add-reviewer genuinely fails with GraphQL error text (matching none of RX_FAILED's alternatives) but the trailing gh pr view succeeds -- err=False, failed=False, obligation silently discharged despite the reviewer-add having failed. (2) An even sharper variant needing no special failure text at all: gh pr edit 999 --add-reviewer someone && gh pr create --title x --body y -- the unrelated PR #999's request satisfies requested=True for the new PR's obligation, so the brand-new PR self-discharges on ordinary success with no reviewer ever requested for it. Neither case is exercised by the 65+ existing tests. Full trace and both repro cases are in the inline comment.

I also re-confirmed the rest of the file is solid: open_ident's structural resolution (the round-13 fix) correctly handles decoy-verb chains and quoted PR numbers; the draft-clear paths (pending_clear) correctly gate on dlast/structural draft_ident even when the regex-based draft boolean and the structural detector disagree (the gh api -f draft=true no-op case); the EDIT_TOOLS withheld-pending-registration fix (5bb834d) is correct; and no banned punctuation appears in the diff.

Verdict

Needs more work.

[github-actions]

💰 Cost: $13.6781 (review) — run

[claude]

Code review

Re-reviewed hooks/no-unreviewed-pr.py, hooks/test-no-unreviewed-pr.py, hooks/hooks.json, and README.md at head 220d2fe.

The prior review's finding (self-discharge missing the last-command/same-PR guard) is fixed. Since git fetch/gh api were not available in this session, I reconstructed the full hooks/no-unreviewed-pr.py from gh pr diff (it's an all-new file) and ran it directly against synthetic transcripts reproducing both cases the prior review found:

1. gh pr create && gh pr edit --add-reviewer X; gh pr view --json url (add-reviewer fails with GraphQL text, non-last, trailing gh pr view succeeds) -- traced request_ident() and the obligations loop by hand: the request is not the last simple command (rlast=False), so the new ob["slast"] guard withholds the discharge and the obligation for the created PR stays tracked. Correct.
2. gh pr edit 999 --add-reviewer someone && gh pr create --title x --body y (unrelated PR's request satisfies requested=True for the new PR) -- traced the same path: srnum='999' while the created PR resolves to a different number, so same_pr is False and the obligation stays tracked regardless of slast. Correct.

I also traced the new same_pr-in-isolation test (gh pr create && gh pr edit 999 --add-reviewer someone, request now last) and its discharging control (gh pr create && gh api .../pulls/1038/requested_reviewers -X POST, last and same-PR) -- both behave as the new tests in hooks/test-no-unreviewed-pr.py expect, and both structured-tool paths (create_pull_request/update_pull_request with reviewers=[...]) are unaffected since they're atomic (slast=True) and inherently same-PR (srnum=None or srnum=num).

I did not find a new regression in the cases I traced (chained ready+request targeting a different PR; a sole/unchained gh pr create --reviewer X; a structured create whose reviewer-add fails). No banned punctuation appears on added lines.

Verdict

Ready for merge.

[github-actions]

Claude finished review — View run

---

Review posted successfully.

Summary

Reviewed PR #1042 at head 220d2fea52 (the latest commit, "guard the self (create+reviewer) discharge like the pending[tid] path"), which fixes a real silent-discharge bug the previous review round found in the self-obligation discharge path.

Since git fetch and gh api were both blocked/unavailable in this session, I reconstructed the complete hooks/no-unreviewed-pr.py from gh pr diff output (it's a new file, so the diff contains the full content) and hand-traced the logic against synthetic transcripts reproducing both concrete cases the prior review flagged:

1. A create chained with a non-last, failing --add-reviewer (masked by a trailing successful command) — the new slast guard correctly withholds discharge.
2. An unrelated PR's request satisfying the new PR's self flag — the new same_pr check correctly withholds discharge.

I also traced the additional edge cases covered by the round's new tests (same-PR-but-last, and the discharging control case) and confirmed the fix's invariant holds without introducing a new regression in the cases I checked (structured-tool paths, ready+different-PR chains, sole unchained requests).

Verdict: Ready for merge.

[github-actions]

💰 Cost: $10.1315 (review) — run