Skip to content

Remove the shape a review reply keeps failing in - #599

Merged
ptr727 merged 1 commit into
developfrom
feature/reply-and-resolve-helper
Aug 7, 2026
Merged

Remove the shape a review reply keeps failing in#599
ptr727 merged 1 commit into
developfrom
feature/reply-and-resolve-helper

Conversation

@ptr727

@ptr727 ptr727 commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Implements the decision recorded on #580, which closed with the reply-and-resolve helper carried forward as its own TODO.md cluster and the dependency it waited on already met.

Why a helper rather than a firmer rule

A reply-and-resolve run by hand keeps failing the same way: a GitHub node id typed into a mutation. Three instances are on record. The third happened while preparing this cluster, when gh-write-guard refused a resolveReviewThread mutation carrying an id hand-typed out of the previous query's output, roughly an hour after the same session quoted that exact rule in a pull request body.

The rule is stated in GOVERNANCE.md and again in the host-level guidance, and the agents that broke it had read both. That says the shape of the operation fails rather than the reader's knowledge of the rule, so this changes the shape.

What reply does

python3 scripts/pr_review.py reply <N> --repo <owner>/<name> \
  --match "<words from the finding>" --body "<answer>" --resolve

It queries the thread id itself and passes it straight to the mutation. There is deliberately no argument an id fits in, and a case asserts the id each mutation carries is the one that run just read.

  • Selects on the finding's own words, not a line number. A fix push moves the line, which is how three replies once posted against nothing while the resolves still succeeded, closing the threads carrying no answer. Matching is case-insensitive, since the text is quoted back out of a digest by a reader.
  • Every failure stops rather than falls back, because each alternative closes a finding while leaving it unanswered. No match exits 60 and prints the open threads, since a no-match and an already-answered thread read identically from here. More than one match exits 61 and prints both candidates rather than taking the first. A reply whose response carries no url, or whose body came back empty, exits 62 without resolving. A resolve that does not confirm exits 63, with the reply already posted.
  • --resolve is opt-in, since a decline is resolved only once its evidence is in the thread.
  • A target under another owner exits 64 before anything is read.

The trade this makes, stated rather than glossed

The script carried test_no_mutation_reaches_this_script, locking it read-only so that mutations stayed visible to the gh-write-guard PreToolUse hook. This reverses that, so the test is narrowed rather than deleted: every other state-changing call is still banned tree-wide and the mutation-document count is pinned at exactly two, so a third arriving is a write nobody reviewed as one.

The hook genuinely cannot see a write this script performs, since it sees python3 pr_review.py reply and no gh write. That is a real loss of a second pair of eyes. It is taken because what the hook guards against there is a fabricated id, and this removes that at the source instead of catching it afterwards.

The guard's other rule is re-implemented rather than assumed. in_scope refuses a target under an owner other than this checkout's, and it takes no GH_WRITE_GUARD_ALLOW escape: a grant this process can be handed is one the caller sets on the command that runs it, and a grant the caller writes for itself is not a grant. The cross-owner case goes through the runbook's explicit gh path, where the hook reads the maintainer's grant from the session instead. It is honest in the docs that this stops a mistake rather than a determined caller.

Corrections carried in this change

  • The cluster's cost line was wrong. .github/copilot-instructions.md is carried at intent fidelity per spec/files.json, so the runbook edit is not hub-only. A sixth Detail under the Fleet Sweeps re-vendor entry records it, noting a repo still on the old copy is not broken, since the mutations that copy documents still work.
  • OPERATIONS.md scoped the gh pr edit breakage to --base, narrower than this repo's own runbook, which states the same failure against --title/--body. The failure is in the mutation the command builds rather than the field asked for. Widened, with the runbook cross-referenced. Not exercised live here, since gh pr edit is a write.
  • TODO.md carried a clause naming "The Prose Gate Scope Floor", a cluster that shipped and was deleted; the phrase appeared nowhere else in the tree.

GOVERNANCE.md is deliberately untouched. It owns the review contract and routes mechanics to .github/copilot-instructions.md, so naming the helper there would add a verbatim re-vendor for nothing.

Verification

Run from this checkout on the branch head:

  • python3 scripts/test_pr_review.py — 103 tests, OK. The suite gains cases for selection without an id, cursor-followed pagination, ambiguity and no-match refusal, reply confirmation before resolve, owner scope, and argument validation.
  • python3 scripts/test_prose_lint.py (190), python3 scripts/test_repo_gate.py (23), python3 spec/audit.py --selftest, python3 spec/validate.py, python3 scripts/repo_gate.py — all pass.
  • python3 scripts/prose_lint.py . --check charset --check dupword --check spelling and --diff origin/develop --summary — both clean.
  • markdownlint and editorconfig-checker over the tree — clean.
  • python3 scripts/pr_review.py status 588 --repo ptr727/ProjectTemplate — a live read confirming the transport refactor, which routes gql through a shared gh_graphql that also checks the errors key rather than reading the null it leaves.

Not verified live: every success path of reply posts a public comment, so it has not been run against a real pull request. The logic is covered by mocked cases only, and the two refusal paths that make no network call were exercised directly. This pull request's own review round is the natural first live exercise.

Closes the TODO.md cluster "The Reply-and-Resolve Helper", deleted here per the file's own rule 9.

🤖 Generated with Claude Code

A reply-and-resolve run by hand keeps failing the same way: a node id typed into a mutation. Three instances are on record, the last refused by the gh-write-guard hook an hour after the agent quoted that same rule in a PR body. A shape that fails while the reader knows the rule is a shape to remove, not a rule to restate, so pr_review.py gains a reply subcommand that queries the id itself and offers no argument an id fits in. It selects on the finding's words rather than a line number, since a fix push moves the line, and it refuses on no match, on more than one, and on a reply whose response carries no url, rather than resolving a thread that would then read as addressed while carrying nothing.

This reverses the script's read-only contract, so the test that locked it is narrowed rather than deleted: every other state-changing call is still banned and the mutation-document count is pinned at two. The hook cannot see a write this script performs, which is a real loss. It is taken because what the hook guards there is a fabricated id, which this removes at the source, and the guard's owner rule is re-implemented in-process with no environment-variable escape, since a grant the caller sets on the command that runs it is not a grant.

Two carried debts ride along. OPERATIONS.md scoped the gh pr edit breakage to --base, narrower than the runbook's own --title/--body claim it now cross-references, and TODO.md carried a clause naming a cluster that shipped and was deleted. The copilot-instructions.md edit is carried at intent fidelity, so the cluster's hub-only cost line was wrong and the Fleet Sweeps entry records it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 7, 2026 03:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a hub-hosted reply helper to scripts/pr_review.py so review-thread replies and optional resolves can be performed without ever hand-typing a GitHub node id, and updates docs/tests accordingly.

Changes:

  • Implement pr_review.py reply with thread selection by finding text (and optional --path) plus explicit refusal modes (no match / ambiguous / not confirmed / out of scope).
  • Refactor GraphQL transport into gh_graphql() with explicit errors handling and safe variable passing (-f for strings).
  • Update documentation/runbooks and tests; remove the now-landed TODO cluster entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
TODO.md Removes the completed “Reply-and-Resolve Helper” cluster and cleans up a stale clause.
scripts/pr_review.py Adds the reply subcommand, owner-scope guard, thread pagination/matching, and shared gh_graphql() transport.
scripts/test_pr_review.py Adds coverage for selection/matching, pagination, refusal codes, confirmation-before-resolve, and scope enforcement.
scripts/README.md Documents the new reply command and updates the script’s write-surface contract description.
OPERATIONS.md Broadens the gh pr edit limitation documentation and points to the runbook section.
.github/copilot-instructions.md Adds the recommended “use the hub’s helper” path before the hand-run mutation workflow.

@ptr727
ptr727 merged commit a6d7a4b into develop Aug 7, 2026
7 checks passed
@ptr727
ptr727 deleted the feature/reply-and-resolve-helper branch August 7, 2026 03:22
ptr727 added a commit that referenced this pull request Aug 8, 2026
… that blocks on what it cannot read (#609)

Promotes 14 commits from `develop`. Merge with a **merge commit** (`gh
pr merge --merge`), never a squash, and **without `--delete-branch`**,
since this pull request's head is `develop` itself.

Closes #607 through the closing keyword already carried in `530cf71`,
which is why it is not repeated here.

## The prose backlog, cleared end to end

`#600`, `#604`, `#605`, `#606` took the tree from **557 findings across
45 files to 0 across 0**, in four batches ordered by surface: snippets,
comments, hub-only Markdown, then the carried files. Each batch measured
the checker's own exemption against the live corpus *before* sweeping,
and twice the measured answer was **"do not change the checker"**, which
is a result of that pass rather than a skipped one.

`#594` added the floor that makes those numbers trustworthy: a
diff-scoped run now asserts what it actually scanned, since a check
whose scan matches nothing reports zero findings and reads exactly like
a pass.

One finding from that work is worth carrying up: an exemption that is
too **loose** produces silence rather than false positives. #519
recorded the governance files as clean; today's checker reports 38
findings against those same files as they stood at the commit that
measured them.

## A review loop that fails closed

`#599`, `#601`, `#602`, `#603` and `#608` are one arc on
`scripts/pr_review.py`, each removing a shape in which the loop reported
a clean pass over a review it had misread:

- **`#599`** removed the shape a reply kept failing in, by taking the
thread's *words* rather than an id, so there is no argument a hand-typed
`PRRT_...` fits in.
- **`#602`** made `claims` resolve what a description points at rather
than what it looks like.
- **`#603`** gave a disproved claim a home the next round reads.
- **`#608`** reads the file-coverage line, and then generalizes: every
reader keys on a structural marker, so a marker that changes spelling is
a section the reader stops finding and reports as absent. The digest now
vets headings, `<summary>` texts, metadata labels, coverage wordings and
the reviewer login against an inventory measured from **332 review
bodies**, and **blocks on anything outside it**, exit `43`, with the
remedy stated as filing an issue on the hub. Whether to merge regardless
is the maintainer's decision.

`GOVERNANCE.md` merge gate went from four preconditions to **five**
accordingly.

## Governance and tooling

- **`#593`** states which checkout an agent works in and what the hub
is, which is the host-wide routing the repositories that most need it
cannot carry.
- **`#596`** gates the pattern-detectable half of the
representative-data rule, honest that no pattern closes the name-shaped
case.
- **`#598`** declares where a repository states what CI cannot verify.
- **`#592`** regrouped `TODO.md` by what ships rather than by what it
touches, so a `###` heading is one pull request.
- **`#601`** ended a `gh push` argument list at a newline rather than
only at `&&`, fixing a write-guard over-block.

## Verification

Run on `develop` at `530cf71` immediately before opening this:
`test_pr_review.py` (174), `test_prose_lint.py`, `test_repo_gate.py`,
`spec/audit.py --selftest`, `gh-write-guard.py --selftest`,
`spec/validate.py`, `repo_gate.py`, the prose gate in both CI
invocations, markdownlint and editorconfig-checker. All clean.

## Not carried by this promotion

- **#519 is complete and still open.** `TODO.md` holds its closing
evidence under "Verified Complete, Awaiting Close". Closing it is the
maintainer's call, so no keyword for it appears here.
- **The re-vendor debt is now nine files.** `#606` queued seven, and
`#608` changed `GOVERNANCE.md` "PR Review Etiquette" (`verbatim`) and
`.github/copilot-instructions.md` (`intent`) on top. The `intent` half
produces no hash and therefore no audit finding, which is why the Fleet
Sweeps entry names those files by hand.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants