Conversation
Three things that cost a wrong turn while driving promotion #460, none of them written down anywhere. Each is recorded as the general rule plus the concrete symptom, so the next agent recognizes it rather than rediscovering it. ## A closing keyword never fires under this branching model GitHub closes a referenced issue only on a merge into the repository's **default** branch, and every feature pull request here targets `develop`. #464 carried `Closes #462` and merged, and #462 stayed open, reading as unfixed until it was closed by hand. The promotion that later reaches `main` carries the commit rather than the keyword, so it does not close it either. This is fleet law rather than a Copilot mechanic, and it binds any agent regardless of provider, so per this file's own rule it belongs in [`GOVERNANCE.md`](../GOVERNANCE.md) rather than the runbook. It is a **top-level** branching-model rule rather than a third entry under "Executing a `develop -> main` promotion safely", because the pull request it bites is a feature one merging into `develop`, not the promotion itself. That parent says "two traps" and means it. ## `gh pr edit` is broken by the classic-Projects sunset It fails with `GraphQL: Projects (classic) is being deprecated ... (repository.pullRequest.projectCards)` and mutates nothing. That matters during a long review loop, where the promotion body goes stale as fixes land under it. The runbook now names `gh api -X PATCH \"repos/<owner>/<repo>/pulls/<N>\" -F body=@<file>` and says to read the body back afterwards, since a failed call is not evidence the pull request is untouched. Filed as its own list rather than added to the existing one, which is specifically about **review request** paths. This is an edit the loop makes between rounds. ## A poll that tests a captured result against `!= \"0\"` reads empty as success An empty string is exactly what a mis-written `--jq` filter returns, so the two cases that must be distinguished, a query finding nothing and a query running wrong, both satisfy the test. One such poll during #460 reported a review that had not landed, and the next message said so before the correction. Counting matches and testing `-gt 0` makes them read alike. The specific trap named with it: `--arg` is a `gh api graphql` flag. Passing it to a plain `gh api --jq` call fails with `accepts 1 arg(s), received 4` while the surrounding `$(...)` still yields the empty string. Placed in "Verify Review Covered Current Head", one paragraph above the existing warning about exiting on `mergeStateStatus`, since both are ways a poll exits early on a false signal. ## Verification Documentation only, no behavior change, so no test accompanies it. 71 self-tests and 19 repo_gate tests pass, `repo_gate` is clean, `charset` and `dupword` exit 0, and the warn-only backlog is unchanged at dash 962 and semicolon 388, so the new prose adds no findings of its own. ## Fidelity note Both files carry verbatim sections, so downstream copies are **stale** until re-vendored. That is already true of them from #460. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR promotes previously-recorded promotion-loop mechanics from develop to main, updating the repo’s governance and Copilot runbook so future promotions avoid known workflow traps.
Changes:
- Adds a Branching Model rule documenting why issue-closing keywords don’t work for feature PRs targeting
develop. - Extends the Copilot runbook with
gh pr editdeprecation guidance and a safer polling pattern for review coverage detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
GOVERNANCE.md |
Documents the issue-closing keyword behavior under the repo’s branching model. |
.github/copilot-instructions.md |
Adds runbook guidance for PR body edits and more robust polling for review coverage. |
Both findings from Copilot's review of promotion #469, against the mechanics #468 had just recorded. Both were written from inference rather than from checking, in the commit whose subject was recording mechanics accurately. ## The closing keyword is not "never" Verified rather than reasoned: ``` gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name' -> main ``` A keyword fires whenever the pull request **itself** merges into the default branch. A promotion, or a Dependabot security update opened against `main`, therefore closes what it references. Only a feature pull request, which targets `develop`, cannot. The rule is inverted to state when the keyword *does* work, so the absolute has nowhere to hide, and it names the promotion and bot cases explicitly. It also adds something the original missed: a promotion body can carry a keyword deliberately, which is a real option rather than a trap. ## `--arg` is jq's flag, not gh's Copilot filed this as "likely to go stale or be incorrect across GitHub CLI versions". It was incorrect on arrival, which is the more useful finding: ``` gh api graphql --help -> --cache, --hostname, --input, --paginate, --silent, --verbose jq --help -> --arg name value set $name to the string value ``` `gh api graphql` takes `-f` and `-F`. The `accepts 1 arg(s), received 4` error came from `gh` treating the extra tokens as positional arguments, and I reasoned the flag's ownership from that error instead of reading `gh api --help`. The paragraph now leads with the durable general rule, that a failed command writes to stderr and leaves stdout empty so the surrounding `$(...)` yields the empty string the test reads as success. The error string stays as a recognizable symptom, without the claim about which tool owns the flag. ## Why this matters more than its size `GOVERNANCE.md` and `.github/copilot-instructions.md` are vendored verbatim across the fleet. A false statement in either propagates to every repository that re-vendors, and it arrived in the commit that exists specifically to stop the next agent reasoning from a symptom. Both corrections came from a review round rather than from me re-reading my own work. ## Verification Documentation only, no behavior change, so no test accompanies it. 71 self-tests and 19 repo_gate tests pass, `repo_gate` is clean, `charset` and `dupword` exit 0, and the warn-only backlog is unchanged at dash 962, comment-wrap 454, semicolon 388, comment-case 56. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/copilot-instructions.md:110
- This adds a second
gh pr edit/PR-body-editing guidance block, but the same topic is already covered later under "### PR Edits and Merge-State Gotchas". Duplicating the instructions increases the chance the two sections drift and provide conflicting guidance over time; consider keeping a single canonical section and linking to it here instead.
Known non-working paths for the PR edits the loop makes between rounds, as opposed to the review request itself:
- `gh pr edit` fails with `GraphQL: Projects (classic) is being deprecated ... (repository.pullRequest.projectCards)` and mutates nothing, so a PR body that grew stale as fixes landed cannot be refreshed with it. Use `gh api -X PATCH "repos/<owner>/<repo>/pulls/<N>" -F body=@<file>` instead. Read the body back afterwards rather than assuming a failed call left the PR untouched.
|
Answering the round-two low-confidence finding, and the two threads still open from round one. The duplicate finding is right, and led to a second one
#468 asserted these were "none of them written down before". I did not grep the files before adding to them. Both duplicates are removed in #472, the surviving release-model rule gains the fallback for a develop pull request that already merged with a keyword on it, and the polling guard stays as the one genuinely new thing in #468. The two round-one threadsBoth were fixed in #470, squashed to
This pull request picks up #472 when it squashes in, and I will re-request a review on the new head. |
From the low-confidence block of Copilot's second review of promotion #469. It flagged one duplicate. Checking the other two additions from #468 found a second, and the second is worse than a duplicate. ## What #468 got wrong That pull request said it was recording three mechanics "none of them written down before". I did not grep the files before writing to them. **`gh pr edit`** was already covered at `.github/copilot-instructions.md` "PR Edits and Merge-State Gotchas", which gives **both** the GraphQL and the REST form and says to verify the edit took. The existing entry is better than the one I added beside it, so the addition goes and the original stays untouched. **Issue-closing keywords** were already covered in `GOVERNANCE.md` under the release model, and that entry is not merely earlier but **correct where mine was not**: > Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not the feature -> develop PR. That is a working mechanism. My branching-model bullet said to close the issue by hand instead. Two bullets, one topic, **different procedures**, which is a contradiction in fleet law rather than a repetition, and exactly the drift Copilot warned the duplicate would cause. It also means my reply on #470, that the promotion mechanism was untested and so the rule should stay silent on it, was answering a question this repository had already answered. #462 could have been closed by putting the keyword on promotion #460. ## What survives The release-model rule gains the one thing mine had that it lacked: the fallback for a develop pull request that already merged with a keyword on it. That is the case #462 actually hit, and without it a reader who has already made the mistake finds no instruction. The polling guard from #468 **stays**. Guarding an empty bot node id before a mutation is documented in two places already, but the **exit test of a poll loop** is not, and the numeric comparison is what stops an empty result reading as a landed review. Kept as the one genuinely new thing in that commit. ## Why this happened, and what would prevent it The closing-keyword rule lives under "Release Model" while the question I was answering was a branching one, so reading the Branching Model section did not surface it. That is a findability problem in a 400-line governance file rather than an excuse: a `grep` for the topic would have found it in either section, and adding to a rules file without grepping it first is the actual failure. Not proposing a reorganization here. Moving committed rule text between sections changes two verbatim sections at once and every downstream copy with them, which is not something to ride along with a fix. ## Verification ``` gh pr edit mentions -> 1 (copilot-instructions.md:209) closing-keyword bullets -> 1 (GOVERNANCE.md:83) ``` 71 self-tests and 19 repo_gate tests pass, `repo_gate` is clean, `charset` and `dupword` exit 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
GOVERNANCE.md:83
- In this bullet,
developis referenced in plain text (feature -> develop PR), but elsewhere branch names are consistently formatted as code (e.g.,feature ->develop->main`` in GOV:41-43). Using backticks here too would keep formatting consistent and reduce ambiguity about whether “develop PR” means “PR targetingdevelop”.
- **Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not the feature -> develop PR.** GitHub auto-closes an issue only when the closing keyword merges into the **default branch** (`main`). A feature/develop PR merges into `develop`, so the keyword never fires there. Reference the issue in the develop PR body if useful, but put the actual closing keyword on the promotion PR. If a develop PR merges with the keyword on it, the keyword does nothing and the issue stays open, so put it on the promotion PR body instead. Close the issue by hand citing the squash SHA only when the promotion has already merged without it.
|
Answering the round-three low-confidence finding. This one I am declining, with evidence, rather than fixing. The finding says branch names are "consistently formatted as code" and cites GOV:41-43 as the example. Line 43 is the counter-example:
That single line backticks the branch twice as a literal ref and then writes it as a plain word three times: "develop's commit list", "the develop commits", "the develop ruleset". It also writes "main" plain twice while backticking it elsewhere. Document-wide: So there is no convention that branch names are always code-formatted. The pattern the document actually follows is backticks when naming the branch as a literal ref, plain prose when using it adjectivally or possessively, which is why "the develop ruleset", "the develop PR body", and "develop's commit list" all read as prose while `develop -> main` reads as a ref. The sentence I added, "If a develop PR merges with the keyword on it", is adjectival and matches "the develop PR body" in the clause immediately before it, which is pre-existing text. Backticking mine alone would make the bullet internally inconsistent, and backticking the whole bullet would diverge it from the surrounding section. Not a defect, so no change. Recording the reasoning here rather than resolving silently, because the underlying observation is a fair one to raise and the answer is only obvious once the counts are in front of you. |
Promotes
f35e8c7(#468),a5e12a2(#470), and7477c42(#472). Conflict-free, three commits ahead.The net change against
mainis three lines in two files. Reviewing this promotion is what reduced it to that, so the history and the result are described separately below.What actually lands
A polling guard, in the Copilot review runbook. A poll that captures a
gh api --jqresult and exits on[ "$found" != "0" ]treats an empty string as a landed review, and an empty string is what a mis-written filter returns. Counting matches and testing-gt 0makes a query that finds nothing and a query that ran wrong read alike. One such poll during #460 reported a review that had not landed.A fallback on the existing issue-closing rule, in
GOVERNANCE.md. The rule already said to putCloses #Non the promotion pull request rather than the feature one. It did not say what to do once a develop pull request has already merged carrying the keyword. It now does: move the keyword to the promotion body, and close by hand only when the promotion has merged without it. That is the case #462 hit.What was reverted, and why that matters more
#468 opened claiming three mechanics "none of them written down before". Two of them were, in the files it edited.
gh pr editbeing broken by the classic-Projects sunset was already documented under "PR Edits and Merge-State Gotchas", with both the GraphQL and the REST form. The addition was a plain duplicate and is gone.Issue-closing keywords were already documented under the release model, and that entry is correct where the addition was wrong. It gives a working mechanism, the keyword on the promotion pull request. The added branching-model bullet said to close the issue by hand instead. Same topic, two sections, different procedures, which is a contradiction in fleet law rather than a repetition, and is the drift a duplicate is supposed to risk only later. That bullet is gone and the surviving rule absorbed the one thing it lacked.
Two further corrections landed on the way. The closing-keyword bullet first said a keyword "never fires under this model", which is false because a pull request merging into
maincloses what it references. The polling note called--argagh api graphqlflag, which is false because--argbelongs tojqandgh api graphqltakes-fand-F. Both files are vendored verbatim across the fleet, so either statement would have propagated on the next re-vendor.Why promote now
What survives is small and true. The polling guard is the only new mechanic in the set, and it is the one that produced a wrong report during #460 rather than a hypothetical. The
GOVERNANCE.mdsentence completes a rule that was already right by covering the state a reader reaches only after getting it wrong.mainis what a newly scaffolded or realigning repository carries, and what the fleet audit reads as ground truth, so a rule that is complete ondevelopand partial onmainis a rule that argues with itself across the fleet.Fidelity note
GOVERNANCE.mdand.github/copilot-instructions.mdboth carry verbatim sections, so downstream copies stay stale until re-vendored. They were already stale from #460 and this does not change that state, only its size, which is now three lines rather than the eleven #468 first proposed.Documentation only, no behavior change. 71 self-tests and 19 repo_gate tests pass,
repo_gateis clean,charsetanddupwordexit 0, and the warn-only backlog is unchanged at dash 962, comment-wrap 454, semicolon 388, comment-case 56.🤖 Generated with Claude Code