Skip to content

Promote the wait failure clause, the five review outcomes, and the pickup-event tooling to main - #528

Merged
ptr727 merged 6 commits into
mainfrom
develop
Aug 3, 2026
Merged

Promote the wait failure clause, the five review outcomes, and the pickup-event tooling to main#528
ptr727 merged 6 commits into
mainfrom
develop

Conversation

@ptr727

@ptr727 ptr727 commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Promotes four commits to main. Two of them change carried files, so downstream repos read the stale text until this lands, which is what makes the promotion the delivery step rather than bookkeeping.

What this delivers to the fleet

Carried rule text, picked up by every repo on its next re-vendor:

PR File and section Change
#526 AGENTS.md, Context and Delegation Discipline (verbatim) A wait separates three outcomes and says which one it reached: run the command in the foreground before backgrounding it, never let || echo '[]', || true or 2>/dev/null stand in for a failure, emit on failure, and bound the wait. The session rule also stops ending a session on a third review round, which read as license to leave a loop open while it was still producing defects
#526 GOVERNANCE.md, Verification Discipline (verbatim) A launched process is not a result, and a cause nobody observed is not a diagnosis
#526 GOVERNANCE.md, PR Review Etiquette (verbatim) Every finding ends in one of five actions rather than at a round count: fixed, disproven with proof the reviewer can read, deferred against a filed issue, declined with the maintainer's explicit answer, or fixed as a class where the code keeps earning it
#520 CODESTYLE.md and the prose gate The comment rules the gate now reaches, carried to the fleet through a public composite action
#526, #527 .github/copilot-instructions.md A quota or rate-limit answer is terminal rather than pending; a request pending with no pickup is a third state with a recovery recipe; three corrections below

Runbook corrections, each one a path an agent followed to a wrong answer this week:

  • gh pr view --json reviewRequests omits a Bot reviewer entirely, reporting an empty set while Copilot sits in it. This is how a live stall was misdiagnosed to the maintainer as no request having been made.
  • Removal was called impossible for want of a named mutation. requestReviews replaces the reviewer set when union is false, which is the clear half of the recovery, and it resolved a real thirteen-and-a-half-hour stall in 35 seconds.
  • The reviewer login has a third spelling. A timeline review_requested carries login Copilot with type Bot, against GraphQL's copilot-pull-request-reviewer and the [bot] suffix REST user objects add. A filter keyed to either documented form selects nothing there.

Hub-only tooling (scripts/, not carried): pr_review.py gains exit 40 for a reviewer answer that carries no commit and exit 50 for a request nothing picked up, with the window and interval guards those needed. prose_lint.py and the gate queue changes from #520 and #522 ride along.

Why the rules moved

Three stalls in one day, each reported as waiting on the reviewer, none of them that. A CI watcher whose command did not exist on the installed gh and whose fallback turned every error into "nothing yet". A stall explained afterwards with a throttle that appears nowhere in the record. And a review request that was pending while nothing acted on it. The common shape is a wait that cannot tell "not yet" from "never", and a report of patience standing in for a reading nobody took.

Verification

scripts/test_pr_review.py 59 pass, scripts/test_prose_lint.py 153 pass, scripts/test_repo_gate.py 23 pass, spec/audit.py --selftest pass, spec/validate.py clean, repo_gate.py clean, both prose_lint.py invocations clean, editorconfig-checker clean. Both source pull requests were driven to a clean Copilot round: #526 over seven rounds and ten findings, #527 over four rounds and five findings, every one of the fifteen real and answered.

🤖 Generated with Claude Code

ptr727 and others added 4 commits August 2, 2026 15:14
Comment shape is the most frequently regressed rule in agent-authored
work, and this repo's answer to it was a rule with nothing running it.
Two independent causes, both fixed here, plus the governance and
host-setup items that came out of the same session.

Closes part of #519.

## The gate did not check comments

`comment-wrap` and `comment-case` sat outside `DEFAULT_RULES`, so
`prose_lint.py .` never checked them. That is why #519's whole-tree
figure splits cleanly into `dash` and `semicolon` with no comment
findings, and why the rule read as enforced while nothing ran it.
Enabling them exposes **424 findings the tree already carried**, worst
in `spec/audit.py` (103) and `host-setup/agent-safety/gh-write-guard.py`
(56).

A test now asserts a bare run catches a wrapped comment, so the rule
cannot go quiet again.

## The gate did not reach downstream repos

It is hub-only by design, so no fleet repo runs it.
`.github/actions/prose-gate` lets a repo consume the rules from here
instead of vendoring a 934-line script into twenty copies, which is
possible only because this repo is now public: a public repository's
composite actions are consumable by any repository, with no organization
account required.

The ref design, which is the part worth reviewing:

- The `uses:` pin is **one literal, identical on both branches**.
`uses:` accepts no expressions, so a per-branch ref would be a permanent
divergence that every `develop -> main` promotion has to preserve,
fighting the branching model.
- **Branch-dependent behavior lives inside the action**, where
expressions are legal. A develop-targeted run reads the rules from hub
`develop`, so an unpromoted rule change is exercised fleet-wide before
it reaches `main`. Every other run uses the copy bundled at the pinned
SHA, so a released repo's gate is reproducible and a hub commit cannot
retroactively fail a re-run.
- This satisfies GOVERNANCE.md "Action pinning" as written, since the
downstream `uses:` is SHA-pinned. Dependabot bumps it like any other
action pin, because the hub publishes real releases (latest `2.0.200`)
and the `github-actions` ecosystem resolves any GitHub-hosted action by
`owner/repo` rather than through the marketplace.

Accepted trade: a hub `develop` commit can redden downstream develop CI
fleet-wide. That is what the staging tier is for, and diff-scoping means
only lines a change touches can fail.

## Two exemptions, both because a finding named no possible edit

- **Generated trees are skipped when a wide scan expands into them**,
currently `reports/`. `spec/audit.py` writes it, so a finding there is
the engine's phrasing rather than an author's. This answers #519's first
question, and it is why the hub's own number was 549 generated to 119
authored. Naming the path directly still reads it, so nothing becomes
uncheckable.
- **An unpunctuated markdown HTML comment is a structural marker, not
commentary.** The reference-link group headers, the ToC-omit directive,
and the `agent-safety` install markers are each matched verbatim by a
tool, so capitalizing one or splitting an adjacent pair breaks what
reads it. A markdown comment that does punctuate a sentence stays judged
as prose, and the existing test asserting that still passes. This
follows the precedent the URI exemption already set.

## Governance and host-setup

- **"Commit" means commit and push.** Nothing reviews a local commit:
the Copilot loop, the required checks, and the maintainer all read the
remote, so stopping at `git commit` reads as progress while no gate has
run.
- **A Copilot decline carries proof, not a rationale.** The command and
its output, the code path that makes the concern impossible, or the rule
that governs it. Disagreeing without evidence is not addressing a
finding, so a thread is not resolved on one.
- **A low-confidence finding is not a low-value one.** Copilot collapses
the findings it is least sure of into the review body, where they reach
no thread, and they are right the large majority of the time.
- **The authorization-scope backstop moved into the committed
canonical.** It was hand-written into one host's `CLAUDE.md`, which is
the exact defect it describes: a rule that exists only where the history
happens to be. `install.py` now syncs it, so it reaches a new machine.
Three pre-existing spaced hyphens in that file are fixed in passing,
matching the phrasing GOVERNANCE.md already uses.

## Still outstanding on this branch

The `catalog/snippets/**` cleanup (#519's second question) is **not** in
this commit. It is 184 findings across 17 files and lands as follow-up
commits here rather than a separate PR. It is the highest-priority
content fix, because a downstream repo copying `build-release-task.yml`
inherits its 37 comment violations and is then flagged for lines it did
not write.

## Verification

`test_prose_lint.py` 148 tests, `test_repo_gate.py` 23,
`test_pr_review.py` 27, `spec/audit.py --selftest`,
`scripts/repo_gate.py`, `spec/validate.py`, and markdownlint all pass.
`prose_lint.py . --diff origin/develop` reports zero on every line this
change touches.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#522)

Four backlog entries, each written from a failure that happened rather
than one imagined. Documentation only, no behavior change.

## Why these four

A session that shipped four merged pull requests also produced a set of
repeated mistakes, and the useful split is between the ones a gate
already caught and the ones nothing was watching for. Comment wrapping
was caught every time, by a gate, and still reached commits because the
gate ran after them. Suppressed review findings went unanswered because
the tool that surfaces them was scoped to the head. A fabricated SHA
reached a file and was caught by hand. Three pull request descriptions
contradicted their own branches and were caught by a reviewer.

## The entries

**A floor assertion in `prose_lint.py`.** This one applies to the gate a
rule `GOVERNANCE.md` already states: "a gate that finds nothing is
indistinguishable from a gate with nothing to find, so assert a floor on
what a healthy run covers." Four separate routes to the same false clean
appeared in one session, an unresolvable diff base, a multi-line `paths`
input, a cross-repository diff, and a path under no repository. Each was
fixed with its own guard, which is the wrong shape: the fifth route will
need a fifth guard and will be found the way the first four were, by a
reviewer. The entry records the honest limit too, since a change
touching only files the gate does not read legitimately scopes to zero.

**Resolvability in the `sha-pin` check.** The check validates the shape
of a pin, and forty hex characters is a shape any fabricated string
satisfies. It would also catch a pin whose commit is reachable only from
a branch that has since been squashed and deleted, which breaks a
downstream gate long after the change that caused it. The entry notes
why `gh-write-guard` cannot cover this: it watches Bash, and an editor
tool writing the same string into a file never reaches it.

**A pull request description drift check.** The most frequent review
finding of the session, six findings across three descriptions. Scoped
deliberately to SHAs and `uses:` refs quoted in the body rather than to
prose claims, because judging prose needs a similarity heuristic and
`spec/section-model.md` already rejects that approach for the same
reason it would fail here.

**Reopening the pre-commit doc-gate decision.** `scripts/README.md`
records that doc linters stay out of the hook so it stays fast, which
was sound when the only mode was a whole-tree sweep. The diff-scoped
mode reads one commit's lines in about a second, so the trade has moved.
Framed as a decision to revisit rather than one to reverse, and it names
the risk of the change too, since a hook running the gate from the wrong
directory is its own false clean.

## Verification

Prose gate, markdownlint and editorconfig-checker all clean on the
changed lines. One British spelling (`neighbouring`) was caught by the
gate and corrected.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A backgrounded wait that emits nothing looks the same whether its condition has not happened yet, its own command is broken, or the thing it waits for is never coming. One session polled a flag the installed tool does not support, took its fallback on every error, and reported "still waiting" for 25 minutes against green CI.

AGENTS.md carries the failure clause for the wait mechanics, GOVERNANCE.md the reporting half, that a launched process is not a result and a cause nobody observed is not a diagnosis. The Copilot runbook gains the terminal case, and `pr_review.py wait` implements it structurally rather than by matching a phrase, exiting 40 on a reviewer answer that carries no commit and printing it whole for the reader to judge.

The review loop's own rule is replaced in the same pass. A round count closed nothing, so a finding now ends in one of five actions: fixed, disproven with proof the reviewer can read, deferred against a filed issue, declined with the maintainer's explicit answer, or fixed as a class where the code keeps earning it.

Closes #524. Closes #525.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…527)

A pull request sat for thirteen and a half hours reading as waiting on the reviewer. The request was pending and nothing was acting on it, a state invisible from the reviews alone and indistinguishable from patience, since a slow round also shows no review.

Copilot raises `copilot_work_started` within about half a minute of accepting a request. A request that never draws one stays that way indefinitely, so the wait reads that event rather than the clock and exits 50 when nothing picked the request up. The read runs on its own interval rather than per poll, asks for the largest page, and counts only the reviewer's own requests, since a human requested afterwards is a different request and reading it as this one reports a picked-up review as never picked up.

Two readings this depended on were wrong and are corrected in the runbook. `gh pr view --json reviewRequests` omits a Bot reviewer outright, reporting an empty set while the reviewer sits in it, which is how the stall was first misdiagnosed to the maintainer as no request at all. And removal was called impossible for want of a named mutation, where `requestReviews` replaces the set when `union` is false, which is the clear half of the recovery the runbook now carries. Run against the stalled pull request, that recovery drew a pickup 35 seconds later and the review that followed raised a real defect.

The reviewer login has a third spelling, login `Copilot` with type `Bot` in a timeline event, against GraphQL's `copilot-pull-request-reviewer` and the `[bot]` suffix REST user objects carry. A filter keyed to either documented form selects nothing there, so the predicate is the type plus a loose login match and the runbook records all three.

Recovery stays out of `pr_review.py`, which keeps its no-mutation contract: the digest names the state and the mutations stay visible to the write guard and to review.

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

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

Promotes recent governance/runbook and tooling updates to main, including improved “wait” semantics (distinguishing pending vs terminal vs stuck), expanded PR review outcome guidance, and a fleet-consumable prose gate so downstream repos can reuse the hub’s lint rules without vendoring scripts.

Changes:

  • Extend scripts/pr_review.py to detect (and exit distinctly on) “answered outside formal review” and “request not picked up” states, plus improve suppressed-finding reporting across all rounds.
  • Strengthen scripts/prose_lint.py default coverage and safety guards (comment-shape defaults, generated-tree skipping, cross-repo diff refusal, and erroring on unusable diffs), with corresponding new tests and documentation.
  • Add a reusable composite action (.github/actions/prose-gate) and supporting docs/config so downstream repos can consume the hub’s prose gate behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TODO.md Updates hub backlog items and link references related to re-vendoring and gate improvements.
scripts/test_prose_lint.py Adds tests for new prose-lint behaviors (markers carve-out, generated-tree skipping, diff guards, default rules).
scripts/test_pr_review.py Adds tests for new pr-review states (answered-outside-review, pickup detection, window guards, digest reporting).
scripts/README.md Documents updated prose-lint defaults and pr-review wait/digest behavior and exit codes.
scripts/prose_lint.py Expands default rules, adds generated-tree handling, adds cross-repo diff guardrails, and makes unusable diffs a hard error.
scripts/pr_review.py Adds exit codes 40/50, timeline pickup reading, “answered outside review” detection, and expanded suppressed-finding reporting.
host-setup/agent-safety/claude-md-safety.md Tightens/extends host safety guidance (write safety + authorization/memory hygiene section).
GOVERNANCE.md Updates verification and PR review etiquette rules (process-vs-result, five outcomes, suppressed-finding handling, commit meaning push).
AGENTS.md Updates session-scope and wait-mechanics guidance (no longer ends session on “third review round”; wait must report failure modes).
.github/copilot-instructions.md Updates runbook mechanics for suppressed findings across rounds and pending/pickup recovery recipes.
.github/actions/prose-gate/action.yml Introduces composite action for fleet prose gate with develop-vs-main rules sourcing behavior.
.editorconfig Enforces LF for composite action YAML files under .github/actions/**.

Comment thread scripts/pr_review.py Outdated
`wait` decided its exit code from the last liveness poll while printing a digest built from a later fetch, so a review landing between the two printed `review_on_head=yes` and returned 40 or 50, and the same gap at the other exit printed coverage and returned 30. A reader holding both resolves the disagreement by believing the code, which drops the review it was just shown and reads a covered pull request as stalled.

Both exits now leave the loop by breaking, and one payload decides the digest and the exit code together. The stall behind the code is re-read from that same payload rather than carried out of the loop, since a request picked up since the loop's reading would still report as picked up by nothing, and `digest` was separately re-reading the timeline to reach a possibly different answer. Coverage wins over both a stalled request and the timeout, and an answer outranks a stall, because the reviewer saying something is worth more to the reader than the reviewer saying nothing.

Where the loop breaks on a stall that the end-of-wait re-read clears, the wait reports pending and exits 30 rather than polling on. That is honest about what it observed and re-running resumes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 3, 2026 14:17
…530)

An agent illustrating a review finding on a downstream pull request quoted real paths from the maintainer's own filesystem, which carried family members' names, into a comment on a public repository. Nothing about the finding required them.

The entry records what the rule has to say rather than only that one is owed. Synthetic evidence is the better evidence and not a concession, since a constructed filename demonstrates a filename defect and any reader can re-run it, where real data proves the same thing and can never be re-run. The exposure is one-way, because a public comment is fetched, cached, and indexed the moment it posts, so an edit afterwards is mitigation rather than a fix and the decision belongs to the maintainer. And no checker closes it, since the data that leaked was name-shaped: a path pattern finds nothing on the offending comment while the names sit in plain sight, so a gate over paths is a floor rather than an answer.

Scoped fleet-wide, since every repo is public and each is worked by agents reading the same carried rules. The rule itself belongs in a carried section and re-vendors when it lands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

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

@ptr727
ptr727 merged commit 275edbf into main Aug 3, 2026
6 checks passed
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