Skip to content

Commit f610e42

Browse files
authored
Promote Write Guard Threat Model and Review Cost Changes to Main (#3405)
## Summary Promotes these pull requests from develop. Each carries a maintainer decision recorded on #3391. - [#3399](#3399): states the write guard's threat model and documents two shapes it keeps allowed, a quoted run-time `-C` value and an `xargs` replace string run as a shell script (item 1). - [#3401](#3401): lists a pre-existing review finding in the pull request body instead of filing an issue for it (item 2). - [#3402](#3402): keeps one standing handoff issue per track, edited in place each round, with a `close` subcommand that ends a track (item 5). - [#3400](#3400): adds a weekly backlog and review-cost report script. - [#3403](#3403): runs the Copilot review once per change, at the promotion, with the attested local pass covering a pull request into develop (item 4). - [#3404](#3404): aligns the remaining review rule text with the promotion-only Copilot review (items 2 and 4). - [#3406](#3406): fixes the findings this promotion's first Copilot round raised on the content above, in the `wait` note for a head no local pass covers, the pr-review-conduct heading, and the backlog report's label line and cost scoping. Closes #3382 Closes #3385 ## Pre-existing findings, not filed - The [Copilot round on 4a77a1d](#3405 (review)) headline says documentation does not consistently describe current-head Copilot refusal handling. `local_cover` in `scripts/pr_review.py` refuses an attested head whose current Copilot round is a refusal, and `wait` exits `41` there. The `wait` entry of the `pr_review.py` docstring ("An attested head is covered at once.") and the `scripts/README.md` paragraph on the defined review moments ("Where it is attested, `wait` polls the head's checks instead.") leave that exception out. `pr-review-conduct` Merge Gate item 2 states it ("A refusal is not that coverage"). Both omissions are on `main` already, so this promotion did not introduce them. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 parents 4b3755f + 4a77a1d commit f610e42

41 files changed

Lines changed: 2391 additions & 1382 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.agents/skills/agent-conduct/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ The checks that separate work actually done from work that merely reports succes
5959
- **A launched process is not a result, and a cause nobody observed is not a diagnosis.** "The watcher is armed" names a process rather than a finding, so what gets reported is the output that process produced, and where it produced none, that absence is the report. The failure it prevents is an agent standing still on a condition that was met half an hour earlier, having announced the wait and never read it. Naming an external cause for such a stall afterwards, a throttle or a quota that appears nowhere in the record, turns a local defect into a story about someone else and closes the investigation on the wrong party, so read the record for the cause before naming one, and where the record does not carry it, report the cause as unknown.
6060
- **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else, because `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run.
6161
- **Platform-specific code is "verified" only on the platform it runs on.** PowerShell on Windows, a macOS-only `mktemp`/`ssh-agent` behavior, a WSL-specific path quirk: an agent reasoning about such code from a different host, however carefully, has not executed it, and reasoning by structural analogy to an already-tested equivalent on another platform ("the POSIX version works, so the PowerShell version should too") is a plausible first pass, not verification. State it as exactly that, an unverified structural match, and never in the same words used for a tested fact. When no agent in the loop has access to the target platform, say so, and either defer the platform-specific portion to a human or an agent that has that access, or ship it clearly labeled unverified.
62-
- **A review flags an instance, so a fix covers the class, bounded to what this change touched or broke.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, and the finding is being fixed, sweep for its siblings before replying, since reviewers sample rather than enumerate, and fix each sibling that sits in a file the diff already touches. A sibling the change itself put in disagreement is this change's to fix wherever it sits, because the change made it wrong. A sibling that was wrong before the change and sits in a file the diff does not touch is filed rather than folded in, because every file the diff grows into is one more that each round reads again, so a sweep that widens the diff widens the loop it was meant to close.
62+
- **A review flags an instance, so a fix covers the class, bounded to what this change touched or broke.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, and the finding is being fixed, sweep for its siblings before replying, since reviewers sample rather than enumerate, and fix each sibling that sits in a file the diff already touches. A sibling the change itself put in disagreement is this change's to fix wherever it sits, because the change made it wrong. A sibling that was wrong before the change and sits in a file the diff does not touch stays out of the diff, because every file the diff grows into is one more that each round reads again, so a sweep that widens the diff widens the loop it was meant to close. Such a sibling takes the outcome a `pre-existing` finding takes under the `pr-review-conduct` Skill's "Every finding ends in a listed outcome" item 4.
6363

6464
`GOVERNANCE.md` "Verification Discipline" keeps the full rules, and the `agent-conduct` Skill at `.agents/skills/agent-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it at its decision moment.
6565

‎.agents/skills/drive-pr/SKILL.md‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,11 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent.
9797
obstacle to route around, and that
9898
skill's refusal table says what each refusal means and what clears it.
9999
3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per
100-
"Disposing of Every Finding" below.
100+
"Disposing of Every Finding" below. Request no Copilot round on this feature PR. Its `wait` reads
101+
step 2's attestation as `coverage=local` and requests none, unless a partial round or a truncated
102+
review history is on record. Exit `49` means the head carries no attestation, so run and record a
103+
pass over it, push, and attest. A promotion re-reviews content its feature PRs already carried,
104+
so the hosted review runs once per change, at the promotion in step 6.
101105
4. Merge the feature PR into
102106
develop, `gh pr merge <number> --squash --repo <owner>/<repo>`. Never `--delete-branch` on this
103107
call, it is run from inside the task's own worktree per step 1, where the feature branch is
@@ -153,8 +157,8 @@ maintainer when".
153157

154158
1. **Real, so fix it, and fix the class rather than the instance.** A reviewer samples rather
155159
than enumerates, so sweep for the finding's siblings before replying and fix each one sitting
156-
in a file the diff already touches or that this change itself made wrong, filing the rest, per
157-
`GOVERNANCE.md` "Verification Discipline". That sweep is owed the first time the finding is
160+
in a file the diff already touches or that this change itself made wrong. Each of the rest takes
161+
the outcome a `pre-existing` finding takes under outcome 4, per `GOVERNANCE.md` "Verification Discipline". That sweep is owed the first time the finding is
158162
raised, not once it recurs. Take the fix through `local-strict-review` the same way the push
159163
that opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply
160164
with the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which
@@ -183,11 +187,40 @@ maintainer when".
183187
turn, before moving to other work. A plan to ask later is resolution by silence the moment
184188
attention moves elsewhere. If the maintainer is not reachable right now, leave the thread open
185189
and say so, rather than treating the intention to ask as the asking.
186-
4. **Real and worth doing later, so file the issue first, then reply with its link.** A deferral
187-
noted only in a thread is lost the moment the PR merges. File it in the repository where the
188-
fix has to land, which for a finding against carried content is the repository that authors
189-
that content rather than the one carrying it, since an issue filed where nobody may make the
190-
fix is a deferral nobody can close.
190+
4. **Deferred past this pull request, or on text it did not touch, so record it by its class.**
191+
An `introduced` finding fixable here and deliberately left as is takes outcome 3 instead.
192+
The class is the one `local-strict-review` "Disposing of Findings" defines, and the first
193+
bullet that fits decides:
194+
- A `pre-existing` finding on a canonical Markdown unit keeps the per-unit tracker
195+
`pr-review-conduct` "Merge Gate" item 3 describes, since that already costs one link per unit.
196+
- A `pre-existing` correctness defect with a stated failure is filed once as its own issue,
197+
labeled `pre-existing`. Such a finding names an input or a state and the wrong output or the
198+
crash it produces. Its body line goes under a `Pre-existing findings, filed` heading and
199+
links that issue. A thread raising it is answered with that issue's link and resolved.
200+
- A `style`, wording, consolidation, or restatement finding on text this change did not touch
201+
is dropped. It gets one line under a `Dropped findings` heading and nothing else. It gets no
202+
decline text, no issue, and no reply beyond a pointer to that line, which resolves any
203+
thread.
204+
- Any other `pre-existing` finding, from a local pass or a PR-hosted review, is listed in the
205+
pull request body with its location and its class. No issue is filed for it. The body line
206+
is its outcome, and a thread raising it is answered with a pointer to that line and resolved.
207+
- Any other deferral is filed as an issue first, and the reply carries its link. A deferral
208+
noted only in a thread is lost the moment the PR merges.
209+
210+
The body keeps every other unfiled `pre-existing` line, the fourth bullet's, under one
211+
heading, `Pre-existing findings, not filed`. Each line under these headings carries its
212+
location and its class. The headings stay apart from the open `introduced` findings a spent
213+
edit budget puts in the body, per `local-strict-review` "Disposing of Findings". A reviewer's
214+
suppressed finding has no thread, so its pointer is an answer per `pr-review-conduct`
215+
"Answering a suppressed finding". That answer is `Listed in the body` for a listed or dropped
216+
finding, and `Deferred to <issue>` for a filed one. A local-pass finding owes no pointer
217+
beyond its body line. A sibling that outcome 1's class sweep leaves unfixed takes the
218+
bullet a `pre-existing` finding would take. An issue goes in the repository where
219+
the fix has to land. For a finding against carried content, that is the repository authoring
220+
the content rather than the one carrying it. An issue filed where nobody may make the fix is
221+
a deferral nobody can close. Filing every `pre-existing` finding is the expensive path. From
222+
2026-10-16 to 2026-10-23, 108 of 329 non-handoff issues filed across four fleet repositories
223+
carried `pre-existing`. Each one costs a lane, a pull request, and a review once worked.
191224
5. **Keeps recurring although the class was swept, so the rule is what needs fixing.** A finding
192225
raised repeatedly against correct code means the code is not communicating something: add the
193226
comment, sharpen the name, narrow the interface, or fix the rule if the rule is wrong.

‎.agents/skills/local-strict-review/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ Each bullet is a rule down to its `Why:` line, which is rationale rather than ru
148148
- `Why:` a local finding and a PR-hosted one deserve the same dispositions, and one home for the list is what stops two copies of it drifting apart.
149149
- **The agent disposing of a pass's findings classes each one `style`, `introduced`, or `pre-existing`, in that order.** `style` is a preference between defensible forms. `introduced` is any other finding on text this change wrote, rewrote, or removed, on text this change should have written, on a precondition this change left false elsewhere, or load-bearing for a decision this change puts to the maintainer. `pre-existing` is every other finding.
150150
- `Why:` the reviewer is asked to omit preferences and returns some anyway, and `style` is classed first so that a preference on text this change wrote is not owed a fix.
151-
- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, handed to the pull request once that budget is spent, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so. A `pre-existing` one is filed once and blocks nothing, and a `style` one is declined with evidence, per `pr-review-conduct` "Every finding ends in a listed outcome", the evidence being `fleet-code-review` "Review the Change"'s own rule to omit preferences.
151+
- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, handed to the pull request once that budget is spent, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so. A `pre-existing` one blocks nothing, and neither does a `style` one on text this change did not touch. `pr-review-conduct` "Every finding ends in a listed outcome" outcome 4 says how each is recorded. A `style` one on text this change wrote is declined with evidence per that section. The evidence is `fleet-code-review` "Review the Change"'s own rule to omit preferences.
152152
- `Why:` a finding count over prose never reaches zero, so a loop closing on "did it find anything" does not close, where one closing on the false claim, the unfollowable instruction, or the wrong behavior this change put there does.
153153
- **Two rounds of edits answer a pass, one budget per push and one per sweep issue.** Where an `introduced` finding is still open after the second round, editing stops. The pull request's body lists each remaining finding with its class. Its review loop then gives each one an outcome under `pr-review-conduct` before it merges, whether or not a hosted reviewer raises it again. A spent budget is not an escalation. A remaining finding goes to the maintainer only where `pr-review-conduct` "Escalate to the maintainer when" names its case.
154154
- `Why:` past the second round nearly every finding is against text the previous round's fix wrote, so the rounds are producing the defects they find rather than removing them.

0 commit comments

Comments
 (0)