Require an explicit repository on the review digest - #575
Merged
Conversation
`--repo` defaulted to this repository, so a run that omitted it read a pull request here rather than the one in front of it. The number resolved, the digest rendered, and no field in the output disagreed. Two runs in other repositories digested a pull request here before the maintainer caught it, one of them recovering with "my run digested the hub's #107". A default cannot be made safe for this argument. Every pull request number is valid in every repository, so the wrong target is never a lookup failure and never surfaces as one. ## Two changes, because the argument and the output each hid the miss `--repo` is now required and carries no default, and its value must be `OWNER/NAME`. A bare `ProjectTemplate` is the near-miss a required argument still admits, and it previously raised an unpacking traceback rather than naming which half was missing. The digest line now leads with `repo=OWNER/NAME`, since a digest of the wrong pull request is well-formed and a number alone reads as correct anywhere. Naming the repository is what makes a misdirected run visible in its own output. ## Coverage `TestCli` runs go through a helper that supplies the repository, and three cases were added: a run naming none is rejected, each malformed shape is rejected by name, and the digest names what it read. The `--pickup-grace -1` case now passes `--repo` and asserts on the grace error, because it was about to exit on the missing argument instead and pass for the wrong reason. `GOVERNANCE.md` "PR Review Etiquette" and `scripts/README.md` update their invocations to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens scripts/pr_review.py against silently digesting the wrong repository by requiring an explicit --repo OWNER/NAME and making the digest output name the repository up front.
Changes:
- Make
--reporequired (no default) and validate it asOWNER/NAMEwith a targeted argparse error. - Prefix the digest summary line with
repo=OWNER/NAMEso misdirected runs are visible in output. - Update CLI tests and documentation/runbook invocations to always pass
--repo.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/pr_review.py | Require/validate --repo and include repo=... in the digest summary line. |
| scripts/test_pr_review.py | Add coverage for missing/malformed --repo, update CLI helper to always supply a repo, and adjust a grace test to assert the intended error path. |
| scripts/README.md | Update documented invocations to include --repo and explain why it is required. |
| GOVERNANCE.md | Update the PR Review Etiquette section’s pr_review.py invocation to include --repo and note the repo is named in the digest. |
It ended on "resolves in any", which reads as a truncated string in `--help` rather than as a clause. The text now says what the reader needs, that a pull request number identifies no repository on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 6, 2026
Two days of onboarding more repositories to the hub produced seventeen recorded observations. Each was checked against `develop` at `b82c1a3`, the committed `TODO.md`, and the eleven open issues before being written down, because the value of a feedback pass is in what it genuinely uncovers rather than in the count of what it recorded. Ten are new entries. Three amend an entry that had already reasoned about the same surface from a single example and needed the second. One was already fully covered and is left untouched. Two are rules that already exist, already state the common case, and leave unstated exactly the case that keeps recurring. ## Disposition | # | Observation | Disposition | | --- | --- | --- | | 1, 2 | Shorthand for the hub, and for the review loop | New entry, Carried Content | | 3 | `pr_review.py --repo` default | Already shipped as #575 | | 4, 6 | Hub-hosted tooling, drop duplicated downstream config scripts | Amends "Reduce the surface that is copied downstream at all" | | 5 | Agents hand-rolling checks the hub already ships | Amends "Decide where a carried file may name hub-only machinery" | | 7 | A table of contents for a long document | New entry, Carried Content | | 8 | The README intro above the tagline | New entry, Audit and Spec Model | | 9 | An operational repository still opens a pull request for a large change | New entry, Carried Content | | 10 | How a `hugo` repository carries its theme | Amends the `hugo` type entry | | 11 | Local defensive-review subagents ahead of the remote reviewer | New entry, Fleet Chores | | 12 | Shared actions to stop duplication | Already covered, no edit | | 13 | Shell against Python with unit tests | New entry, Gate Honesty | | 14 | Helper functions for the commands that keep failing | New entry, Gate Honesty | | 15 | When an issue is closed by hand | New entry, Carried Content | | 16 | The clickable link ahead of the prompt | New entry, Carried Content | | 17 | A register of disproved review findings | New entry, Fleet Chores | ## The three amendments each add the example their entry was missing Every one of the three had already reasoned correctly from one instance. What each lacked was the second instance that shows the reasoning generalizes, which is what turns a worked example into a decision worth making. The copied-surface entry argued the case for `repo-config/configure.sh` alone. It now states the wanted end state, that a script identical everywhere lives in the hub once and a downstream repository calls it, and that the fleet already runs both models without either having been chosen against the other. It also separates out the half that needs no decision to start on, which is deleting a downstream config script whose content is only ever the hub's. The hub-only-machinery entry had the reader-follows-a-dead-pointer problem and no evidence that anyone had. It now carries the measured cost: several sessions had to be redirected by hand to `scripts/pr_review.py` and `scripts/prose_lint.py` after building worse versions of them, and none of those agents did anything wrong, since the tool they were pointed at is absent from their tree and nothing they can read says how to reach it. The `hugo` entry recorded that Blog's theme is vendored with no upstream ref and never asked whether that is what the type should require. The question is framed as one about the type rather than about Blog, because whatever is decided becomes the type's contract and the second generator inherits it. ## Two rules already exist, and the wording is the defect `Closes #N` is stated at `GOVERNANCE.md` "Git and Commit Rules" and is carried fleet-wide, so downstream agents re-deriving it and reporting it as a discovery are missing it rather than lacking it. What is genuinely absent is the case that produces the confusion, which is work complete on `develop` while a promotion is not imminent, leaving the issue open with nothing recording that it is done. The current text licenses a hand-close only once a promotion has merged without the keyword, which is the narrower case. The clickable-link rule gets the hard part right, that an interactive prompt renders neither a Markdown link nor a bare URL, so the reference inside it is a bare number and the link goes in the message that accompanies it. It says accompanying rather than preceding, and a message emitted after the prompt is not read before the question is answered, which is the entire outcome the rule exists to produce. Reading both recurrences as underspecified wording rather than as the rule being ignored is what makes either of them fixable, and it is the same diagnosis in both cases. ## One entry records a live defect rather than a preference `spec/readme-structure.md` item 1 reads as though the canonical description **is** the paragraph after the H1, and `CODESTYLE.md` then has `HISTORY.md` copy "the same intro paragraph" verbatim. Together they forbid a README from saying anything further about itself above the fold, and `spec/audit.py` measures the first non-empty line, so a legitimate second paragraph is reported as a finding. That is the same axis as declaring the description in `registry/repos.json`, so the entry says the two want one decision rather than two taken in sequence. ## Verification `prose_lint.py --diff`, `repo_gate.py`, and `markdownlint-cli2` are clean over the change. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 6, 2026
…e onboarding-feedback backlog (#582) Promotes the two commits `develop` has carried since the previous promotion (#574) to `main`. Merge commit only, no squash, and `develop` is not deleted. ## What lands - **#575** Require an explicit `--repo` on `scripts/pr_review.py`, since the old default silently digested a pull request in the hub when a run in another repository omitted it, and lead the digest summary with the repository it read so a misdirected run is visible in its own output. - **#576** Fold the two-day onboarding-feedback pass into `TODO.md`: ten new entries, three amendments that each supply the second example their entry was reasoning from one instance without, and two recurring cases traced to underspecified wording in rules that already exist. ## Issues closed None. Neither constituent pull request carried a closing keyword, and the four issues #576 filed (#577, #578, #579, #580) record work that is still open rather than work this promotion completes. ## Verification `git merge-tree --write-tree origin/main origin/develop` reports no conflict, so this promotion needs no throwaway resolution branch, and the tree diff against `main` is exactly the five files the two commits touch. Both constituent pull requests merged green with their review loops closed.
ptr727
added a commit
that referenced
this pull request
Aug 6, 2026
Copilot answers a pull request it will not take on with a formal review,
`state: COMMENTED`, carrying the correct `commit.oid` and zero inline
threads, whose whole body is a refusal. `pr_review.py` treated a review
carrying the head as coverage, so the digest read
pr=108 head=9f56a472 rounds=1 review_on_head=yes threads=0
unresolved=0 suppressed=0 answered_outside_review=no requested=no
merge=CLEAN
which is the clean pass byte for byte, and `wait` exited 0. Observed on a
pull request of 301 changed files, one over the reviewer's limit of 300,
which was one command from merging on a round that never ran.
The existing guard does not reach it. Exit 40 rests on a comment carrying
no commit, so it satisfies no coverage check whatever it says. This
carries the right commit. Only the delivery mechanism differs, and the
mechanism is what the guard keys on.
## The reading
A refusal is not coverage. `reviewed_head` and the digest's
`review_on_head` both exclude it, so the state reports as NO, and the
summary line carries a `refusal=` field of its own, because
`rounds=1 review_on_head=NO` is equally what a stale round looks like and
the two want opposite responses. `wait` exits 41 and the body prints
whole, since the wording is the only thing separating a file-count
refusal, cleared by splitting the pull request, from a quota one, cleared
by waiting. The script reads neither cause.
Head-scoped, unlike a suppressed finding: a refusal is a statement about
one commit that a push retires, and a genuine review of the same head
outranks it. Both surfaces spend it on that coverage, the exit code and
the field alike, or the line reads `review_on_head=yes refusal=YES` and
tells a reader to split a pull request the reviewer has just reviewed.
The liveness query still carries no bodies, so a refusal reads there as
ordinary coverage, which ends the wait, and the full read every wait
finishes with is what tells the two apart. No exit code comes from the
cheaper reading, so the per-poll cost is unchanged.
## The unit is the body's opening line
Matching anywhere in the body reports this pull request as a refusal of
itself, the same false positive the suppressed matcher already had once.
Two lines is not enough either: a review's first line is its heading and
its second is the overview prose, which is exactly where a review
describing this check states the wording. A case pins that, and it is the
case that failed while every other one passed. The cost runs the other
way, that a refusal introduced by a heading would be missed.
The alternation is the runbook's, and a case asserts the script's pattern
is the published one rather than a copy that can drift. The dot spans
both spellings of the apostrophe and keeps the published `--jq` filter
usable inside single quotes, which neither spelling survives.
## Docs
The same guarantee was stated on four surfaces, so each moves: the
runbook's clean-pass rule and its coverage-confirmation paragraph, and
`GOVERNANCE.md` merge-gate condition 2 and review-loop step 3, which now
say the matched review is read rather than only counted. The runbook also
records the reviewer-side fact an agent hitting this needs, that the
limit is 300 changed files and the remedy is to split the pull request.
`GOVERNANCE.md` is verbatim-carried, so a fleet re-vendor is owed.
The issue's second point, `--repo` defaulting to this repository, was
fixed in #575 and needs nothing here.
Fixes #581.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scripts/pr_review.py --repodefaulted toptr727/ProjectTemplate, so a run that omitted it read a pull request here rather than the one in front of it. The number resolved, the digest rendered, and no field in the output disagreed. Two runs in other repositories digested a pull request here before the maintainer caught it, one of them recovering with "my run digested the hub's #107".A default cannot be made safe for this argument. Every pull request number is valid in every repository, so the wrong target is never a lookup failure and never surfaces as one.
Two changes, because the argument and the output each hid the miss
scripts/pr_review.pyargument--repois required with no default, and its value must beOWNER/NAMEscripts/pr_review.pydigestrepo=OWNER/NAMEA bare
ProjectTemplateis the near-miss a required argument still admits, and it previously raised an unpacking traceback rather than naming which half was missing. It is now rejected as--repo takes OWNER/NAME, not 'ProjectTemplate'.The digest names the repository because a digest of the wrong pull request is well-formed, and a number alone reads as correct anywhere. That is what makes a misdirected run visible in its own output rather than only to a reader who already suspects it.
Coverage
TestCliruns go through a helper that supplies the repository, and three cases were added: a run naming none is rejected, each malformed shape is rejected by name, and the digest names what it read.The
--pickup-grace -1case now passes--repoand asserts on the grace error. It was about to exit on the missing argument instead, which is the sameSystemExitand would have passed for the wrong reason.Carried surfaces
GOVERNANCE.md"PR Review Etiquette" andscripts/README.mdupdate their invocations to match. TheGOVERNANCE.mdsection is carried verbatim fleet-wide, so it reaches downstream repositories on the next re-vendor, where the corrected invocation matters most.Verification
Not addressed:
TODO.mdstill records the open question of whetherGOVERNANCE.mdmay namescripts/pr_review.pyat all, given the fleet carries the section but not the script. This makes that pointer correct without settling it.