Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,8 @@ Issue-level Copilot comments (those in `issues/<N>/comments`) have no resolution
### PR Edits and Merge-State Gotchas

- **`gh pr edit --title/--body` is broken here.** It touches the deprecated Projects-classic `projectCards` GraphQL field and **exits non-zero without applying the change** (a stale PR description then survives review rounds). Edit the title/body via the API and verify it took: GraphQL `updatePullRequest(input: { pullRequestId, title, body })`, or REST `gh api -X PATCH repos/<owner>/<repo>/pulls/<N> -F body=@body.md` (the `@` reads the body from a file, so name it explicitly, not the literal `file`).
- **`main`/`develop` use rulesets, not classic branch protection.** The classic protection REST endpoint (`repos/.../branches/<b>/protection`) 404s, so read the ruleset instead. A `mergeStateStatus` of `BLOCKED` on a green PR is usually just **unresolved review threads** (the ruleset requires thread resolution); resolving them moves it to `CLEAN`. (`BLOCKED` is a `mergeStateStatus` value; don't confuse it with the separate `mergeable` field's `MERGEABLE`/`CONFLICTING`, which reports merge conflicts, not review gates.)
- **`main`/`develop` use rulesets, not classic branch protection.** The classic protection REST endpoint (`repos/.../branches/<b>/protection`) 404s, so read the ruleset instead. A `mergeStateStatus` of `BLOCKED` on a green PR is most often just **unresolved review threads**, since the ruleset requires thread resolution, and resolving them moves it to `CLEAN`. (`BLOCKED` is a `mergeStateStatus` value. Do not confuse it with the separate `mergeable` field's `MERGEABLE`/`CONFLICTING`, which reports merge conflicts rather than review gates.)
- **`BLOCKED` never says which gate, so never infer one.** The same word covers a red check, a required check nothing is running, an unresolved thread, and a missing approval, and the bullet above says "most often" rather than "always" for that reason. Read the checks instead of guessing: `pr_review.py status` prints `checks=N/M` beside the merge word and names a stuck one, and it exits `42` from `wait` where the review loop closed but a check is starved, running far past what the job costs, or failed. A **queued check with no runner** is the case that reads exactly like patience: a run here polled `BLOCKED` for twenty-five minutes on a pull request whose only unfinished check was an aggregator job GitHub dispatched and never assigned a runner, and the cause came from the maintainer rather than from any field. Nothing agent-side starts that job, because the pool is GitHub-hosted, so the remedy is a re-run of the workflow or waiting on that capacity, and it is **not** a re-request, a rebase, or an empty commit. A job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so a queued check is never a dependency waiting its turn.
- **Push -> head-SHA read race.** A `headRefOid` read taken immediately after a push can return the **old** head, so re-read after the push registers, or a coverage poll evaluates the stale SHA.
- **Copilot is sometimes factually wrong** (e.g. it claimed `actionlint -color` "requires a value" when it is a boolean flag). Verify a finding before fixing, and decline with evidence when it is wrong, which is distinct from dismissing a still-present finding as stale.

Expand Down
4 changes: 3 additions & 1 deletion GOVERNANCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,15 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom

**Do not merge, and do not enable auto-merge, unless ALL of these hold:**

1. Required status checks are green (`mergeStateStatus: CLEAN`), **and**
1. Required status checks are green (`mergeStateStatus: CLEAN`), and where they are not, the reason is **read** rather than inferred, because `BLOCKED` is one word for a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, so the response to it is decided from the check states and never from the word, **and**
2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, **and**
3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and**
4. The maintainer has given **explicit** permission to merge.

`mergeStateStatus: CLEAN` reflects **only** required statuses, and never open bot review comments, so `CLEAN` alone is **never** sufficient to merge. A green/`CLEAN` PR with an unresolved Copilot finding fails this gate; treat it as "not mergeable" no matter what the merge-state field says. The agent never merges on its own (consistent with "default to staging"; merging is maintainer-authorized).

**`BLOCKED` is not self-explaining either, and the remedy differs by cause.** A failed check is a defect to fix, an unresolved thread is a finding to close, and a required check sitting queued with no runner assigned is neither: it is hosted-runner capacity, which no re-request, rebase, or empty commit clears, and which a reader waiting on it cannot tell from patience. Report a blocked merge by naming the blocking check and its state rather than reporting the word, and where the cause is capacity, say so and stop rather than pushing at it, since the bypass is the maintainer's to run and a starved runner is not a reason to weaken a gate.

**Merging is not releasing.** A merge to a release branch does **not** by itself publish. Publishing is a separate, explicitly configured step in the repo's release pipeline (e.g. a scheduled run, a manual dispatch, or an opted-in publish-on-merge trigger), not an automatic consequence of merging. Never describe a merge as cutting a release, and never trigger a publish without explicit maintainer instruction.

### Expected Review Loop
Expand Down
2 changes: 1 addition & 1 deletion TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ The gates in [`scripts/`][scripts] are hub-only and are the fleet's main defense

Everything here changes a file the fleet holds a copy of, so each entry costs a hub edit plus a sweep, and the sweep is the part that gets skipped.

- Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering five files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. In `AGENTS.md`, "Context and Delegation Discipline" carries the wait rule's failure clause and "Where the Rules Live" carries a row for the new section named below. In `GOVERNANCE.md`, "Verification Discipline" carries the rule that a launched process is not a result, "PR Review Etiquette" carries the five outcomes that close a finding, "Repository Boundaries and Write Safety" carries the rule that a refused write is reported rather than re-shaped, and "Representative Data in Agent-Authored Text" is an entirely new carried section that no downstream repo holds, which the audit reports as a missing section rather than as drift. Three further `GOVERNANCE.md` sections differ by a single word each, "Documentation Style Conventions", "Communicating with the User" and "Repository Details", where the format's name was capitalized to the convention `CODESTYLE.md` "Markdown and Spelling" now states, so they are byte-mismatched for a reason a reader of the diff would otherwise call cosmetic. Two comment lines in `.markdownlint-cli2.jsonc` took the same capitalization, and that file is `verbatim` and `whole`, so every downstream copy is byte-mismatched on a config nothing else changed about. `CODESTYLE.md` carries the new item and is the fifth file, at `intent` rather than `verbatim` fidelity, so it reaches the fleet as a rule each repo adopts in its own copy rather than as bytes to match, and the same mixed spelling is waiting in every downstream tree. Every repo already holding a copy of a changed section is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. This sweep is also the follow-through [#489][issue-489] and [#379][issue-379] were waiting on, and the `.editorconfig` line in [#353][issue-353] rides the same visit to each repo. Regenerate [reports/divergences.md][divergences-report] before using it as the work list, since the committed copy is dated 2026-07-22 and therefore predates the router split, which shows in it naming "Repository Boundaries and Write Safety", "Git and Commit Rules" and "Verification Discipline" as `AGENTS.md` sections when all three now live in `GOVERNANCE.md`. A stale ledger is the same hazard as a stale exemption, in that it hands out a work list measured against a tree that no longer exists.
- Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering five files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. In `AGENTS.md`, "Context and Delegation Discipline" carries the wait rule's failure clause and "Where the Rules Live" carries a row for the new section named below. In `GOVERNANCE.md`, "Verification Discipline" carries the rule that a launched process is not a result, "PR Review Etiquette" carries the five outcomes that close a finding and now also a merge-gate condition 1 that reads why a merge is blocked rather than inferring it, plus the paragraph saying `BLOCKED` is no more self-explaining than `CLEAN` is sufficient, "Repository Boundaries and Write Safety" carries the rule that a refused write is reported rather than re-shaped, and "Representative Data in Agent-Authored Text" is an entirely new carried section that no downstream repo holds, which the audit reports as a missing section rather than as drift. Three further `GOVERNANCE.md` sections differ by a single word each, "Documentation Style Conventions", "Communicating with the User" and "Repository Details", where the format's name was capitalized to the convention `CODESTYLE.md` "Markdown and Spelling" now states, so they are byte-mismatched for a reason a reader of the diff would otherwise call cosmetic. Two comment lines in `.markdownlint-cli2.jsonc` took the same capitalization, and that file is `verbatim` and `whole`, so every downstream copy is byte-mismatched on a config nothing else changed about. `CODESTYLE.md` carries the new item and is the fifth file, at `intent` rather than `verbatim` fidelity, so it reaches the fleet as a rule each repo adopts in its own copy rather than as bytes to match, and the same mixed spelling is waiting in every downstream tree. Every repo already holding a copy of a changed section is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. This sweep is also the follow-through [#489][issue-489] and [#379][issue-379] were waiting on, and the `.editorconfig` line in [#353][issue-353] rides the same visit to each repo. Regenerate [reports/divergences.md][divergences-report] before using it as the work list, since the committed copy is dated 2026-07-22 and therefore predates the router split, which shows in it naming "Repository Boundaries and Write Safety", "Git and Commit Rules" and "Verification Discipline" as `AGENTS.md` sections when all three now live in `GOVERNANCE.md`. A stale ledger is the same hazard as a stale exemption, in that it hands out a work list measured against a tree that no longer exists.
- Drop the unused `GITHUB_TOKEN` grants from the merge-bot workflow, per [#521][issue-521], which the tree confirms is still open, since all four jobs still carry `contents: write` and `pull-requests: write` while every write in the file authenticates with the App token. The finding is least privilege on the highest-blast-radius shape in the fleet, a `pull_request_target` workflow holding an App private key, where the grant is not exploitable today only because no step consumes it. Three things to settle: whether to drop the job-level blocks or set a workflow-level `permissions: {}`, whether the audit should compare permissions at all given the file is `interface` fidelity with only a `requiredJobKeys` contract, and the re-vendor, since every repo carries the file. Note that [`spec/files.json`][files] now declares this workflow at `appliesTo: "*"`, which closes the separate gap [#456][issue-456] raised, that D8.1 graded a file the file spec never required.
- State the rule that an agent never assumes a Docker image is present locally, however recently it pulled one, because a background prune can remove an image between two commands of the same session and the run then fails on something that worked minutes earlier. The prescription is the always-pull flag by default, and an explicit `docker pull <image>:<tag>` before use where the flag does not apply. The four documented lint invocations already carry `--pull=always`, in [`GOVERNANCE.md`][governance] "Running the Linters Locally", mirrored in [`OPERATIONS.md`][operations] and both VS Code task snippets, so what is missing is the rule rather than the one-liners: an agent composing an ad-hoc `docker run` drops the flag precisely because it believes the image is already cached. Two things to settle when writing it. Where it lives, since that section is scoped to the four lint tools while the rule covers any container an agent starts, which argues for placing it with the Docker or verification guidance instead, and whether it is carried to the fleet, since every repo runs the same images from the same instructions. Note the limit before writing it, so the flag does not read as the whole answer: `docker run` against a registry tag re-pulls an absent image on its own, so the cases that actually break are a locally built tag with no registry to pull from, and any command that branches on the image being present (`docker image inspect`, `docker images`) rather than simply running it.
- Retire the downstream `repo-config/configure.sh` copies, which is the sweep the [#580][issue-580] decision leaves. [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" settles where a carried file may name hub machinery, and it settles it in the third direction the entry below proposed rather than either of the two this entry originally offered: the hub hosts the tool, a carried rule names it as the hub's, and no repository holds a copy. The manifest entry is gone from [`spec/files.json`][files], so nothing asks a repo for the file and nothing reports its absence, which makes this a visit-ordered chore rather than a gate. Delete the copy as each repo is next worked on and run the hub's script against it by name. For the size of the sweep, regenerate [reports/divergences.md][divergences-report] rather than reading the committed copy, which predates this decision and splits the carriers into a matching set and a divergent one under a re-vendor disposition that no longer applies to either. The ledger entry in [`spec/divergences.json`][divergences] carries the per-repo detail under the new `retire` disposition.
Expand Down
Loading
Loading