From 68226974bb1382fd54f71869c50a2d027b5265d8 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 27 Oct 2026 16:14:54 -0700 Subject: [PATCH 1/5] Make the Model Tiers Bullets Agree on What a Dispatch Naming No Model Runs At (#3538) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `AGENTS.md` "Model Tiers" gave two answers for a dispatch that names no model. The `highest` bullet states the conditional form: the agent definition's model where one is set, otherwise the session's own. Two later bullets stated only the session fallback. - The "A tier resolves only through this table" bullet loses its trailing clause. It now says only that such a session names no model. - The "An orchestrating seat" bullet loses its restating sentence. Its rationale, "a choice nobody made", is false where an agent definition pins a model. The `highest` bullet stays the single statement of the result. `scripts/build_dist.py` regenerated the include regions in `backlog-burndown`, `local-strict-review`, and `unattended-handoff`, plus their generated copies. ## Pre-existing findings, filed - `AGENTS.md` "Model Tiers", the paragraph under the tier table, and the `spec/model-tiers.json` note: an unconditional "session's own model" claim for a tool with no row. Class `pre-existing`, filed as #3537, since the two change together and the spec file is outside this change. Closes on promotion: #3529 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .agents/skills/backlog-burndown/SKILL.md | 4 ++-- .agents/skills/local-strict-review/SKILL.md | 4 ++-- .agents/skills/unattended-handoff/SKILL.md | 4 ++-- .claude-plugin/fleet-skills/.source-digests/backlog-burndown | 2 +- .../fleet-skills/.source-digests/local-strict-review | 2 +- .../fleet-skills/.source-digests/unattended-handoff | 2 +- .claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md | 4 ++-- .../fleet-skills/skills/local-strict-review/SKILL.md | 4 ++-- .../fleet-skills/skills/unattended-handoff/SKILL.md | 4 ++-- .github/skills/backlog-burndown/SKILL.md | 4 ++-- .github/skills/local-strict-review/SKILL.md | 4 ++-- .github/skills/unattended-handoff/SKILL.md | 4 ++-- AGENTS.md | 4 ++-- 13 files changed, 23 insertions(+), 23 deletions(-) diff --git a/.agents/skills/backlog-burndown/SKILL.md b/.agents/skills/backlog-burndown/SKILL.md index 1e5dede7..8aaa0ddc 100644 --- a/.agents/skills/backlog-burndown/SKILL.md +++ b/.agents/skills/backlog-burndown/SKILL.md @@ -557,8 +557,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 144f2d89..26e94843 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -196,8 +196,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.agents/skills/unattended-handoff/SKILL.md b/.agents/skills/unattended-handoff/SKILL.md index 218c4d76..ea7963c4 100644 --- a/.agents/skills/unattended-handoff/SKILL.md +++ b/.agents/skills/unattended-handoff/SKILL.md @@ -203,8 +203,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown index 2d14be62..c40c3534 100644 --- a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown +++ b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown @@ -1 +1 @@ -e1af7dadfb2070a2 +28fb141c0bacd468 diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index b2cd43dd..58caf9a1 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -9dc270f21b0302cd +5e933f569ef7e7ac diff --git a/.claude-plugin/fleet-skills/.source-digests/unattended-handoff b/.claude-plugin/fleet-skills/.source-digests/unattended-handoff index 106cab16..44e8a504 100644 --- a/.claude-plugin/fleet-skills/.source-digests/unattended-handoff +++ b/.claude-plugin/fleet-skills/.source-digests/unattended-handoff @@ -1 +1 @@ -eddb00a5222e6b31 +5bbb7f15ec4e23c4 diff --git a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md index 1e5dede7..8aaa0ddc 100644 --- a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md @@ -557,8 +557,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md index 144f2d89..26e94843 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -196,8 +196,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.claude-plugin/fleet-skills/skills/unattended-handoff/SKILL.md b/.claude-plugin/fleet-skills/skills/unattended-handoff/SKILL.md index 218c4d76..ea7963c4 100644 --- a/.claude-plugin/fleet-skills/skills/unattended-handoff/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/unattended-handoff/SKILL.md @@ -203,8 +203,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.github/skills/backlog-burndown/SKILL.md b/.github/skills/backlog-burndown/SKILL.md index 1e5dede7..8aaa0ddc 100644 --- a/.github/skills/backlog-burndown/SKILL.md +++ b/.github/skills/backlog-burndown/SKILL.md @@ -557,8 +557,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 144f2d89..26e94843 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -196,8 +196,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/.github/skills/unattended-handoff/SKILL.md b/.github/skills/unattended-handoff/SKILL.md index 218c4d76..ea7963c4 100644 --- a/.github/skills/unattended-handoff/SKILL.md +++ b/.github/skills/unattended-handoff/SKILL.md @@ -203,8 +203,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. diff --git a/AGENTS.md b/AGENTS.md index 4138e045..a9305fbb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -69,8 +69,8 @@ A rule, a Skill, or a brief names a model tier, never a model. A model release i Each cell is the name that tool's subagent dispatch takes, which follows the latest model of its family. A tool with no row here, or one that cannot name a model per subagent, runs every tier at the session's own model. -- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one, so the subagent runs at the session's own model. -- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. A dispatch that names no model inherits whatever the session runs on, which is a choice nobody made. +- **A tier resolves only through this table.** A word such as "frontier", "strongest", or "most capable" never resolves to `highest`. The industry's "frontier model" is the opposite of what this tier means here. A session that cannot see this table names no model rather than guessing one. +- **An orchestrating seat runs at `frontier` and names a tier on every dispatch.** It defaults a subagent to `frontier`, and picks `standard` or `bulk` deliberately for execution or mechanical work. ### Delegation From 4752a2e1aaa92bebf8ea411e91beb6f147d64ec5 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 27 Oct 2026 16:15:46 -0700 Subject: [PATCH 2/5] Bring the Durable Knowledge Handoff Bullets Onto the Standing-Issue Lifecycle (#3539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `GOVERNANCE.md` "Durable Knowledge and Self-Improvement" described the old per-link handoff lifecycle in two bullets. This brings both onto the standing-issue lifecycle that `AGENTS.md` "Session Scope" and the `session-handoff` skill state. - The blocker bullet said a handoff "closes with its successor". It now says the next round replaces the handoff's body in place, which is why a wait belongs in the backlog instead. - The `blocked` bullet said the label comes off "when a session hands the link back to be worked". It now comes off once a session writes the next round, matching `session-handoff`'s resume step 6. - The `agent-conduct` include region and its generated copies are regenerated with `scripts/build_dist.py`. ## Dropped findings - `GOVERNANCE.md:20` (and line 103 of the three `agent-conduct` copies), `style`: five untouched sentences in the blocker bullet exceed 25 words. The diff-mode sentence-length check reads the whole changed line. A later sentence-length round owns them. Closes on promotion: #3504 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .agents/skills/agent-conduct/SKILL.md | 4 ++-- .claude-plugin/fleet-skills/.source-digests/agent-conduct | 2 +- .claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md | 4 ++-- .github/skills/agent-conduct/SKILL.md | 4 ++-- GOVERNANCE.md | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.agents/skills/agent-conduct/SKILL.md b/.agents/skills/agent-conduct/SKILL.md index 1ab58bc3..318011df 100644 --- a/.agents/skills/agent-conduct/SKILL.md +++ b/.agents/skills/agent-conduct/SKILL.md @@ -100,9 +100,9 @@ Where a lesson lands, when it earns a mechanical hook, and where work blocked on - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`GOVERNANCE.md` for a cross-cutting rule, `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog the repository already keeps). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream rather than patching the local copy. A local patch leaves every sibling repo with the same trap. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. -- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, and it belongs to one track and closes with its successor, where the wait outlives every session that met it and belongs in the backlog the whole repository reads. +- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, but it belongs to one track and closes when that track's work is done. The wait can outlive that track, so it belongs in the backlog the whole repository reads. - **The blocker record is written under the ordinary write rules, and the label on it is taken off deliberately.** Filing an issue and commenting on another are state-changing calls, so "Repository Boundaries and Write Safety" binds each of them exactly as it binds any other write, which keeps the upstream issue inside this owner and makes a blocker under a different owner a matter of explicit permission rather than of this rule. The `blocked` label is what every reader of this record selects on, so a repository not carrying it cannot host one until the fleet label set is applied there, which is a change to that repository's configuration rather than anything this rule writes, and a session that finds it missing reports that. The label comes off when the blocker clears, and the session closing the upstream issue is best placed to take it off, since the backlink comments naming every waiting repository are on the issue it is closing, while any later session that finds it cleared takes it off instead. A fix can land well before either of those, so the label lags the fix rather than tracking it, which is why a session meeting a `blocked` issue reads the state of the issue that issue's body names rather than the label. The local issue stays open when the label comes off, because the work it records still has to be done and is ordinary backlog from that moment on. -- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only when a session hands the link back to be worked, so a loop running meanwhile never takes a link a present maintainer is still working. +- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only once a session writes the next round, so a running loop never takes a handoff a present maintainer is still working. - **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** The conditions below must hold together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all of them (it recurred under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. Those conditions gate promotion to a **host** hook, the involuntary layer that fires in every session under the maintainer's own credentials and that only the maintainer can grant an exemption from, which is why the bar there is destructive harm. A **committed** hook in the repository's own tree is a third layer between prose and that one, and it is earned on weaker grounds: it is opt-in per clone, visible in the tree, bypassable by design, and it therefore fits a rule whose harm is a quality miss rather than a destruction. The second condition still binds it, since a hook that cannot decide its own trigger is a hook that nags, so what earns the layer is finding the decidable half of a rule whose other half is judgment. The local-review rule under `GOVERNANCE.md` "Verification Discipline" is the worked example: whether a review's findings were rightly disposed of is judgment no hook can decide and stays prose, while whether a review pass ran over exactly the content being pushed is a receipt comparison, which the hub's own `.husky/pre-push` decides. `GOVERNANCE.md` "Durable Knowledge and Self-Improvement" 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 moments. diff --git a/.claude-plugin/fleet-skills/.source-digests/agent-conduct b/.claude-plugin/fleet-skills/.source-digests/agent-conduct index b4b98126..19f30aee 100644 --- a/.claude-plugin/fleet-skills/.source-digests/agent-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/agent-conduct @@ -1 +1 @@ -35cbb6bb71550038 +044a3bbb4da33402 diff --git a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md index 1ab58bc3..318011df 100644 --- a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md @@ -100,9 +100,9 @@ Where a lesson lands, when it earns a mechanical hook, and where work blocked on - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`GOVERNANCE.md` for a cross-cutting rule, `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog the repository already keeps). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream rather than patching the local copy. A local patch leaves every sibling repo with the same trap. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. -- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, and it belongs to one track and closes with its successor, where the wait outlives every session that met it and belongs in the backlog the whole repository reads. +- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, but it belongs to one track and closes when that track's work is done. The wait can outlive that track, so it belongs in the backlog the whole repository reads. - **The blocker record is written under the ordinary write rules, and the label on it is taken off deliberately.** Filing an issue and commenting on another are state-changing calls, so "Repository Boundaries and Write Safety" binds each of them exactly as it binds any other write, which keeps the upstream issue inside this owner and makes a blocker under a different owner a matter of explicit permission rather than of this rule. The `blocked` label is what every reader of this record selects on, so a repository not carrying it cannot host one until the fleet label set is applied there, which is a change to that repository's configuration rather than anything this rule writes, and a session that finds it missing reports that. The label comes off when the blocker clears, and the session closing the upstream issue is best placed to take it off, since the backlink comments naming every waiting repository are on the issue it is closing, while any later session that finds it cleared takes it off instead. A fix can land well before either of those, so the label lags the fix rather than tracking it, which is why a session meeting a `blocked` issue reads the state of the issue that issue's body names rather than the label. The local issue stays open when the label comes off, because the work it records still has to be done and is ordinary backlog from that moment on. -- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only when a session hands the link back to be worked, so a loop running meanwhile never takes a link a present maintainer is still working. +- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only once a session writes the next round, so a running loop never takes a handoff a present maintainer is still working. - **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** The conditions below must hold together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all of them (it recurred under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. Those conditions gate promotion to a **host** hook, the involuntary layer that fires in every session under the maintainer's own credentials and that only the maintainer can grant an exemption from, which is why the bar there is destructive harm. A **committed** hook in the repository's own tree is a third layer between prose and that one, and it is earned on weaker grounds: it is opt-in per clone, visible in the tree, bypassable by design, and it therefore fits a rule whose harm is a quality miss rather than a destruction. The second condition still binds it, since a hook that cannot decide its own trigger is a hook that nags, so what earns the layer is finding the decidable half of a rule whose other half is judgment. The local-review rule under `GOVERNANCE.md` "Verification Discipline" is the worked example: whether a review's findings were rightly disposed of is judgment no hook can decide and stays prose, while whether a review pass ran over exactly the content being pushed is a receipt comparison, which the hub's own `.husky/pre-push` decides. `GOVERNANCE.md` "Durable Knowledge and Self-Improvement" 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 moments. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index 1ab58bc3..318011df 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -100,9 +100,9 @@ Where a lesson lands, when it earns a mechanical hook, and where work blocked on - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`GOVERNANCE.md` for a cross-cutting rule, `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog the repository already keeps). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream rather than patching the local copy. A local patch leaves every sibling repo with the same trap. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. -- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, and it belongs to one track and closes with its successor, where the wait outlives every session that met it and belongs in the backlog the whole repository reads. +- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, but it belongs to one track and closes when that track's work is done. The wait can outlive that track, so it belongs in the backlog the whole repository reads. - **The blocker record is written under the ordinary write rules, and the label on it is taken off deliberately.** Filing an issue and commenting on another are state-changing calls, so "Repository Boundaries and Write Safety" binds each of them exactly as it binds any other write, which keeps the upstream issue inside this owner and makes a blocker under a different owner a matter of explicit permission rather than of this rule. The `blocked` label is what every reader of this record selects on, so a repository not carrying it cannot host one until the fleet label set is applied there, which is a change to that repository's configuration rather than anything this rule writes, and a session that finds it missing reports that. The label comes off when the blocker clears, and the session closing the upstream issue is best placed to take it off, since the backlink comments naming every waiting repository are on the issue it is closing, while any later session that finds it cleared takes it off instead. A fix can land well before either of those, so the label lags the fix rather than tracking it, which is why a session meeting a `blocked` issue reads the state of the issue that issue's body names rather than the label. The local issue stays open when the label comes off, because the work it records still has to be done and is ordinary backlog from that moment on. -- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only when a session hands the link back to be worked, so a loop running meanwhile never takes a link a present maintainer is still working. +- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only once a session writes the next round, so a running loop never takes a handoff a present maintainer is still working. - **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** The conditions below must hold together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all of them (it recurred under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. Those conditions gate promotion to a **host** hook, the involuntary layer that fires in every session under the maintainer's own credentials and that only the maintainer can grant an exemption from, which is why the bar there is destructive harm. A **committed** hook in the repository's own tree is a third layer between prose and that one, and it is earned on weaker grounds: it is opt-in per clone, visible in the tree, bypassable by design, and it therefore fits a rule whose harm is a quality miss rather than a destruction. The second condition still binds it, since a hook that cannot decide its own trigger is a hook that nags, so what earns the layer is finding the decidable half of a rule whose other half is judgment. The local-review rule under `GOVERNANCE.md` "Verification Discipline" is the worked example: whether a review's findings were rightly disposed of is judgment no hook can decide and stays prose, while whether a review pass ran over exactly the content being pushed is a receipt comparison, which the hub's own `.husky/pre-push` decides. `GOVERNANCE.md` "Durable Knowledge and Self-Improvement" 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 moments. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 21040ca8..b4164820 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -17,9 +17,9 @@ The specific rules in this file implement a few governing principles. Read these - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`GOVERNANCE.md` for a cross-cutting rule, `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog the repository already keeps). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream rather than patching the local copy. A local patch leaves every sibling repo with the same trap. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. -- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, and it belongs to one track and closes with its successor, where the wait outlives every session that met it and belongs in the backlog the whole repository reads. +- **A blocker filed in another repository is recorded in the repository whose work it blocks.** The binding moment is the one where the upstream issue is filed or, where it already exists, found, because that session is the one that knows what stopped and why. It owes a second issue in the repository that is waiting rather than only the first, and where the upstream issue already exists the local issue names that one and no second upstream issue is filed. The local issue states what this repository cannot do and why, in its own terms rather than as a pointer to read elsewhere, since a reader who has to open the upstream issue to learn whether it affects them opens every one of them. The local issue names the upstream one as its blocker, carries the `blocked` label, and carries whatever labels its own work would carry anyway. A comment on the upstream issue then names the local one in return, a comment rather than an edit to the body because a second repository may join the same blocker later and because the body is often not this session's to rewrite. That order, the upstream issue and then the local issue and then the backlink comment, leaves a partial failure as a record naming its blocker rather than as a blocker naming a record nobody wrote. **A handoff does not do this job.** It carries the blockers a round met, but it belongs to one track and closes when that track's work is done. The wait can outlive that track, so it belongs in the backlog the whole repository reads. - **The blocker record is written under the ordinary write rules, and the label on it is taken off deliberately.** Filing an issue and commenting on another are state-changing calls, so "Repository Boundaries and Write Safety" binds each of them exactly as it binds any other write, which keeps the upstream issue inside this owner and makes a blocker under a different owner a matter of explicit permission rather than of this rule. The `blocked` label is what every reader of this record selects on, so a repository not carrying it cannot host one until the fleet label set is applied there, which is a change to that repository's configuration rather than anything this rule writes, and a session that finds it missing reports that. The label comes off when the blocker clears, and the session closing the upstream issue is best placed to take it off, since the backlink comments naming every waiting repository are on the issue it is closing, while any later session that finds it cleared takes it off instead. A fix can land well before either of those, so the label lags the fix rather than tracking it, which is why a session meeting a `blocked` issue reads the state of the issue that issue's body names rather than the label. The local issue stays open when the label comes off, because the work it records still has to be done and is ordinary backlog from that moment on. -- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only when a session hands the link back to be worked, so a loop running meanwhile never takes a link a present maintainer is still working. +- **A handoff parked on a maintainer decision carries the `blocked` label too.** Its blocker is a `decision` issue in this same repository rather than an issue elsewhere, and the parking comment on the handoff names it. A reader reads the named issue rather than the label, and the blocker clears when that issue loses its `decision` label. Unlike a cross-repository blocker, a session finding it cleared does not take the label off. It comes off only once a session writes the next round, so a running loop never takes a handoff a present maintainer is still working. - **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** The conditions below must hold together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all of them (it recurred under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. Those conditions gate promotion to a **host** hook, the involuntary layer that fires in every session under the maintainer's own credentials and that only the maintainer can grant an exemption from, which is why the bar there is destructive harm. A **committed** hook in the repository's own tree is a third layer between prose and that one, and it is earned on weaker grounds: it is opt-in per clone, visible in the tree, bypassable by design, and it therefore fits a rule whose harm is a quality miss rather than a destruction. The second condition still binds it, since a hook that cannot decide its own trigger is a hook that nags, so what earns the layer is finding the decidable half of a rule whose other half is judgment. The local-review rule under `GOVERNANCE.md` "Verification Discipline" is the worked example: whether a review's findings were rightly disposed of is judgment no hook can decide and stays prose, while whether a review pass ran over exactly the content being pushed is a receipt comparison, which the hub's own `.husky/pre-push` decides. `GOVERNANCE.md` "Durable Knowledge and Self-Improvement" 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 moments. From 945712892b4af281362f6f470cae861ac4e0118c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 27 Oct 2026 17:02:47 -0700 Subject: [PATCH 3/5] Keep Hub tests/ Paths Out of the Hub-Only Detector (#3540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `spec/audit.py` reported a carrier's own `tests/.py` as `hub-only:` whenever the hub has a test module of the same name. Its remedy then suggested deleting the file. The hub's `tests/` tree is never carried, so such a hit is always a path collision. This closes the class rather than adding one `gaps` entry per instance. - `spec/files.json` gains a `repoOwnedTrees` declaration, listing `tests` with its reason. - `spec/files.schema.json` describes the new array. - `hub_only_paths` drops every hub path below a declared repo-owned tree. Both of its consumers inherit this: the audit's hub-only check and the fidelity-honesty manifest gap pass. - A new `hub_paths_below` helper holds the declared-source walk, shared by both `hub_only_paths` loops and `check_tree_verbatim`. - `AUDIT.md`'s carried-scope bullet, the `carried.hubonly.absent` assertion in `spec/project-types.json`, and `RESYNC.md` section 4 each name the exclusion. - `spec/validate.py` checks the `repoOwnedTrees` shape beside its `trees` checks. It refuses a missing source or reason, a source at or above the repository root or not in normalized form, a source with a symlinked component, and a source that is not a directory. - `tests/test_audit.py` adds a case where a carrier's `tests/test_a.py` shares a constructed hub test's name and is not reported. A control shows a hub-only path outside `tests/` is still reported. Its fixture loads the shipped declaration, so a malformed entry or a dropped `tests` fails the suite. - `tests/test_spec_validate.py` adds `RepoOwnedTreeGateCase`, running the real gate against a scratch tree for each refusal and a plain entry. It shares a new `run_gate_on_files` runner with `TreePathGateCase`. ## Verification - The full unittest suite passes under the OPERATIONS.md invocation. - With the `tests` declaration renamed, both new audit cases fail. - `ruff check`, `ruff format --check`, `mypy`, `spec/validate.py`, and `scripts/prose_lint.py --diff origin/develop` are clean. ## Pre-existing findings, not filed - `AUDIT.md:88`, `pre-existing`: `prose_lint.py --check sentence-length` flags three sentences of 36, 53, and 35 words in the carried-scope bullet. This change added one 25-word sentence there and wrote none of the three. - `RESYNC.md:92`, `pre-existing`: the same check flags a 41-word sentence in section 4. This change added a shorter sentence before it. - `AUDIT.md:88`, `RESYNC.md:92`, `spec/project-types.json:161`, `pre-existing`: each derivation names the baseline and now the repo-owned exclusion, but not the carried `trees` exclusion `hub_only_paths` has always applied. ## Dropped findings - `scripts/canonical_review.py:303`, consolidation on text this change did not touch: its tree walk could reuse `hub_paths_below`. Closes on promotion: #3528 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- AUDIT.md | 2 +- RESYNC.md | 4 +- spec/audit.py | 38 ++++++------ spec/files.json | 3 + spec/files.schema.json | 13 ++++ spec/project-types.json | 2 +- spec/validate.py | 30 ++++++++++ tests/test_audit.py | 44 ++++++++++++-- tests/test_spec_validate.py | 116 +++++++++++++++++++++++++++--------- 9 files changed, 199 insertions(+), 53 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index 9e18a647..ff98b89e 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -85,7 +85,7 @@ A check with `intentRef`/`workflowRef` points at the prose section that owns the - **docker** - registry layer cache (`buildcache-`, never `type=gha`), the size-limited Docker Hub README is published via the docker-readme task, and the image always re-pushes on publish. - **hugo** - the build fails on a generator warning, the URL-parity gate asserts a length floor before comparing, the rendered output is untracked, the generator is pinned by version and checksum and declared once, a vendored tree records its upstream ref, and the deploy asserts what the host serves (the release id and the environment). Retention is bounded by a declared count with one side recorded as owning the prune, which is the deploy where its credential can observe the destination and the host where that credential is confined write-only, so grade which shape the repo uses rather than looking for a prune step. Deploy credentials are per-environment, which `spec/secrets.json` cannot express, so a clean **repo-setup** verdict says nothing about whether the environments are configured. - **branch-model** - `main` and `develop` both exist and are protected, and the live rulesets match [`repo-config/*.json`][repo-config] by normalized diff (below). -- **carried-scope** - the repo carries no file the hub hosts rather than carries. The set is derived, not listed: the hub's git-tracked paths minus the [`spec/files.json`][files] baseline, so a file dropped from the manifest starts being reported on the next run with no retirement list to remember to edit. The remedy is the opposite of every other file finding, a **deletion**, since the repo reaches the hub's copy per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. The match is on path alone, so a hit is a candidate and not a verdict: a repo's own content at a path the hub also uses matches while carrying nothing of the hub's, which the first fleet run showed twice, a KiCad tooling doc at `scripts/README.md` and per-repo formatting hooks at `.husky/pre-commit`. A [`spec/divergences.json`][divergences] `gaps` disposition decides which case a hit is, so only `retire` asserts a deletion, `accepted` closes a collision or a repo-owned file, and an untriaged hit is read before it is acted on. +- **carried-scope** - the repo carries no file the hub hosts rather than carries. The set is derived, not listed: the hub's git-tracked paths minus the [`spec/files.json`][files] baseline, so a file dropped from the manifest starts being reported on the next run with no retirement list to remember to edit. The manifest's `repoOwnedTrees` entries name hub directories no repo carries, such as `tests`, and every hub path below one is left out of the set. The remedy is the opposite of every other file finding, a **deletion**, since the repo reaches the hub's copy per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. The match is on path alone, so a hit is a candidate and not a verdict: a repo's own content at a path the hub also uses matches while carrying nothing of the hub's, which the first fleet run showed twice, a KiCad tooling doc at `scripts/README.md` and per-repo formatting hooks at `.husky/pre-commit`. A [`spec/divergences.json`][divergences] `gaps` disposition decides which case a hit is, so only `retire` asserts a deletion, `accepted` closes a collision or a repo-owned file, and an untriaged hit is read before it is acted on. - **verbatim-tree** - every applicable `trees[]` declaration in [`spec/files.json`][files] owns its target tree. The audit reports missing files as letter findings, stale or modified bytes as drift, and extra files under a pruned target as drift. An unreadable or truncated repository tree is undecided and produces drift rather than a clean result. - **repo-setup** - every required secret for the repo's publish mechanisms is configured, and no forbidden secret is present (per [`spec/secrets.json`][secrets]). - **runtime-secrets** - a repo-scoped runtime-secrets directory, when present, is named `.secrets/` (dotted, not a bare `secrets/`), and a single opaque credential file carries no extension. Every real secret file has a tracked `.example` beside it cataloged in `.secrets/README.md`. Those parts are N/A for a repo with no such directory. See [GOVERNANCE.md "Repo-Scoped Secrets"][governance-repo-scoped-secrets]. Every env and secret file the repo names, tracked or not, follows the env and secret file rule of [CODESTYLE.md "Script and Utility File Names"][codestyle-file-names]. `spec/audit.py` reports a tracked name that breaks the rule's form at **drift**. It reads each file directly in a `.secrets/` directory and each `.env` file elsewhere whose name does not open on a dot. It sets a trailing `.example` aside first, and the directory's own `README.md`, `.gitignore`, and `.gitkeep` are not secret files. The rest is judged by hand. A hyphen joins words only inside a host field, which takes the DNS label form. The check passes a hyphenated word in any field, since it cannot tell which field it is. A host field is a short name, never a fully qualified one. The check cannot tell an extension from a dot-joined last field of an extensionless name, a two-label host included. A consumer limit applies only where a name is used as a Docker secret or maps to a variable. A host-side name is judged where the repo's docs and scripts spell it out, never by reading a host. An `ENV_FILE` path and a compose `env_file` are such places. Renaming a file on a host is the maintainer's step, listed in the resync pull request. diff --git a/RESYNC.md b/RESYNC.md index ff1e4e49..255a7ad0 100644 --- a/RESYNC.md +++ b/RESYNC.md @@ -89,11 +89,11 @@ The order is load-bearing. Each step below either changes the rules the later st Every other finding in this procedure is satisfied by adding or replacing content. This one is satisfied by removing it, which makes it the only class where acting on a wrong finding loses something. -The detector is derived rather than listed: the hub's git-tracked paths minus the [`spec/files.json`][files] baseline is what the hub hosts and no repository carries. That means a file dropped from the manifest starts being reported on the next run with no retirement list to maintain, and it also means **the match is on path alone**, so a hit is a candidate and not a verdict. +The detector is derived rather than listed: the hub's git-tracked paths minus the [`spec/files.json`][files] baseline is what the hub hosts and no repository carries. Every hub path below a `repoOwnedTrees` entry in that file, such as `tests`, is left out. That means a file dropped from the manifest starts being reported on the next run with no retirement list to maintain, and it also means **the match is on path alone**, so a hit is a candidate and not a verdict. ```mermaid flowchart LR - candidate["hub git-tracked path, not in spec/files.json"] --> disp["check spec/divergences.json disposition"] + candidate["hub git-tracked path, not in spec/files.json or a repo-owned tree"] --> disp["check spec/divergences.json disposition"] disp -->|"retire"| delete["delete the file"] disp -->|"accepted"| keep["keep, closed permanently"] disp -->|"untriaged"| read["read before touching"] diff --git a/spec/audit.py b/spec/audit.py index 5e3d85d0..dece2676 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -99,19 +99,29 @@ def hub_tracked(rev=None): return frozenset(paths) +def hub_paths_below(source, include=("**/*",), rev=None): + """Hub-tracked paths below a declared source directory, mapped from the path relative to it, that match its include patterns.""" + root = source.rstrip("/") + "/" + return { + path.removeprefix(root): path + for path in hub_tracked(rev) + if path.startswith(root) and tree_path_included(path.removeprefix(root), include) + } + + def hub_only_paths(spec, rev=None): - """Hub-tracked paths the manifest does not declare, so a downstream copy is hub-hosted content rather than a carry.""" - declared = {e["path"] for e in spec["files"]["baseline"]} - tree_paths = set() + """Hub-tracked paths the manifest does not declare, so a downstream copy is hub-hosted content rather than a carry. + + A path below a declared repo-owned tree is left out, since a downstream file there is that repo's own. + """ + excluded = {e["path"] for e in spec["files"]["baseline"]} for declaration in spec["files"].get("trees", []): - root = declaration["source"].rstrip("/") + "/" - tree_paths.update( - path - for path in hub_tracked(rev) - if path.startswith(root) - and tree_path_included(path.removeprefix(root), declaration["include"]) + excluded.update( + hub_paths_below(declaration["source"], declaration["include"], rev).values() ) - return hub_tracked(rev) - declared - tree_paths + for declaration in spec["files"].get("repoOwnedTrees", []): + excluded.update(hub_paths_below(declaration["source"], rev=rev).values()) + return hub_tracked(rev) - excluded def gap_dispositions(spec): @@ -2079,14 +2089,8 @@ def check_tree_verbatim(run): for declaration in run.spec["files"].get("trees", []): if not applies(declaration.get("appliesTo", "*"), run.sel): continue - source_root = declaration["source"].rstrip("/") target_root = declaration["target"].rstrip("/") - source_files = { - path.removeprefix(source_root + "/"): path - for path in hub_tracked() - if path.startswith(source_root + "/") - and tree_path_included(path.removeprefix(source_root + "/"), declaration["include"]) - } + source_files = hub_paths_below(declaration["source"], declaration["include"]) target_files = { path.removeprefix(target_root + "/"): path for path in run.entries diff --git a/spec/files.json b/spec/files.json index e29a3b69..94dbb0e7 100644 --- a/spec/files.json +++ b/spec/files.json @@ -5,6 +5,9 @@ { "source": ".github/skills", "target": ".github/skills", "fidelity": "verbatim-tree", "appliesTo": "*", "include": ["**/*"], "prune": true }, { "source": "lib/sh", "target": "lib/fleet/sh", "fidelity": "verbatim-tree", "appliesTo": ["host-secrets"], "include": ["**/*"], "prune": true } ], + "repoOwnedTrees": [ + { "source": "tests", "reason": "The hub's own test suite is never carried. Every repo with code keeps its own tests here, so a shared file name is a path collision rather than a carry." } + ], "baseline": [ { "path": "CLAUDE.md", "fidelity": "verbatim", "whole": true, "appliesTo": "*" }, { "path": "AGENTS.md", "fidelity": "intent", "sections": [{ "name": "Fleet Bootstrap", "fidelity": "verbatim" }, diff --git a/spec/files.schema.json b/spec/files.schema.json index d363e6a8..34142d73 100644 --- a/spec/files.schema.json +++ b/spec/files.schema.json @@ -24,6 +24,19 @@ } } }, + "repoOwnedTrees": { + "description": "Hub directories no repo carries, where a downstream file is that repo's own content. The hub-only check in spec/audit.py skips every hub path below one.", + "type": "array", + "items": { + "type": "object", + "required": ["source", "reason"], + "additionalProperties": false, + "properties": { + "source": { "type": "string", "minLength": 1 }, + "reason": { "type": "string", "minLength": 1 } + } + } + }, "baseline": { "type": "array", "items": { diff --git a/spec/project-types.json b/spec/project-types.json index 16454ef8..ca2cac24 100644 --- a/spec/project-types.json +++ b/spec/project-types.json @@ -158,7 +158,7 @@ "carried-scope": { "appliesTo": "*", "checks": [ - { "id": "carried.hubonly.absent", "verdict": "letter", "assert": "The repo carries no file the hub hosts rather than carries. spec/audit.py derives the set as the hub's git-tracked paths minus the spec/files.json baseline, and reports each one the repo's ground-truth branch also holds. The remedy is a deletion rather than a re-vendor, since the repo reaches the hub's copy per GOVERNANCE.md 'Hub-Hosted Tooling'. The match is on path alone, so a repo's own content at a path the hub also uses matches without carrying anything of the hub's, and a spec/divergences.json 'gaps' disposition decides which case a hit is: 'retire' asserts a deletion, 'accepted' closes a path collision or a repo-owned file, and an untriaged hit is read before it is acted on.", "intentRef": "GOVERNANCE.md#hub-hosted-tooling" } + { "id": "carried.hubonly.absent", "verdict": "letter", "assert": "The repo carries no file the hub hosts rather than carries. spec/audit.py derives the set as the hub's git-tracked paths minus the spec/files.json baseline, and reports each one the repo's ground-truth branch also holds. Every hub path below a spec/files.json 'repoOwnedTrees' entry, such as tests, is left out of the set, since no repo carries it. The remedy is a deletion rather than a re-vendor, since the repo reaches the hub's copy per GOVERNANCE.md 'Hub-Hosted Tooling'. The match is on path alone, so a repo's own content at a path the hub also uses matches without carrying anything of the hub's, and a spec/divergences.json 'gaps' disposition decides which case a hit is: 'retire' asserts a deletion, 'accepted' closes a path collision or a repo-owned file, and an untriaged hit is read before it is acted on.", "intentRef": "GOVERNANCE.md#hub-hosted-tooling" } ] }, "repo-setup": { diff --git a/spec/validate.py b/spec/validate.py index c368e0dc..707eba4a 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -1457,6 +1457,36 @@ def check_selector(where, applies_to): f"files.json: tree targets {left_target} and {right_target} have overlapping ownership" ) + owned_trees = files.get("repoOwnedTrees", []) + if not isinstance(owned_trees, list): + errors.append("files.json: 'repoOwnedTrees' must be an array") + owned_trees = [] + for owned in owned_trees: + if not isinstance(owned, dict): + errors.append(f"files.json: repo-owned tree {owned!r} is not an object") + continue + source = owned.get("source") + if not isinstance(source, str) or not source: + errors.append(f"files.json: repo-owned tree has an invalid source: {owned!r}") + continue + reason = owned.get("reason") + if not isinstance(reason, str) or not reason: + errors.append(f"files.json: repo-owned tree {source} reason must be a non-empty string") + if ( + reduces_to_repo_root(source) + or escapes_repo_root(source) + or pathlib.PurePosixPath(source).as_posix() != source + ): + errors.append( + f"files.json: repo-owned tree {source} source must be a normalized path below the repository root" + ) + elif (link := symlinked_component(ROOT, source)) is not None: + errors.append( + f"files.json: repo-owned tree {source} has a symlinked component {link}, below which git tracks no file" + ) + elif not (ROOT / source).is_dir(): + errors.append(f"files.json: repo-owned tree {source} is missing or not a directory") + copilot_path = ROOT / ".github/copilot-instructions.md" try: copilot_instructions = copilot_path.read_text(encoding="utf-8") diff --git a/tests/test_audit.py b/tests/test_audit.py index 5207b47c..a19d6336 100755 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -79,23 +79,30 @@ def __call__(self, path: str, ok404: bool = False) -> Any: def run_audit( - tree: set[str], modes: dict[str, str] | None = None, unread: frozenset[str] = frozenset() + tree: set[str], + modes: dict[str, str] | None = None, + unread: frozenset[str] = frozenset(), + hub: frozenset[str] = frozenset(), ) -> tuple[list[tuple[str, str]], FakeGh]: """Audit a Python repository carrying `tree`, where a test module makes codecov.yml owed. - The hub's tracked paths come from a fetch of its origin. Answering them as empty keeps the - run offline and leaves the checkout the suite runs in untouched. + The hub's tracked paths come from a fetch of its origin. Answering them with `hub` keeps the + run offline and leaves the checkout the suite runs in untouched. The repo-owned trees are the + shipped declaration, so a test reads the exclusion the real audit applies. """ spec = { "registry": {"repos": [ENTRY], "defaults": {}}, "settings": audit.load("repo-config/settings.json"), "secrets": audit.load("spec/secrets.json"), - "files": {"baseline": [CODECOV]}, + "files": { + "baseline": [CODECOV], + "repoOwnedTrees": audit.load("spec/files.json")["repoOwnedTrees"], + }, } fake = FakeGh(tree, modes, unread) with ( mock.patch.object(audit, "gh", fake), - mock.patch.object(audit, "hub_tracked", lambda rev=None: frozenset()), + mock.patch.object(audit, "hub_tracked", lambda rev=None: hub), ): findings, _ = audit.audit_repo(ENTRY, spec) return findings, fake @@ -195,5 +202,32 @@ def test_a_failed_shebang_read_raises_rather_than_reading_as_undecided(self) -> run_audit(tree, {"bin/Run_Backup": "100755"}, frozenset({"bin/Run_Backup"})) +class AuditRepoHubOnlyCase(unittest.TestCase): + """audit_repo leaves a carrier's own test module out of the hub-only check. + + The hub's tests are never carried, so a carrier's file at a hub test path is a collision. + Reporting it would ask for the deletion of that repo's own suite. + """ + + HUB = frozenset({"tests/test_a.py", "tools/hub_tool.py"}) + + def hub_only_findings(self, findings: list[tuple[str, str]]) -> list[str]: + return [text for _, text in findings if text.startswith("hub-only:")] + + def test_a_test_module_sharing_a_hub_test_name_is_not_reported(self) -> None: + findings, _ = run_audit(CARRIED | {".codecov.yml"}, hub=self.HUB) + + self.assertEqual(self.hub_only_findings(findings), []) + + def test_a_hub_only_path_outside_tests_is_still_reported(self) -> None: + """The control, showing the check under test ran and reports a hub-only path it sees.""" + findings, _ = run_audit(CARRIED | {".codecov.yml", "tools/hub_tool.py"}, hub=self.HUB) + + hub_only = self.hub_only_findings(findings) + self.assertTrue( + any(text.startswith("hub-only: tools/hub_tool.py ") for text in hub_only), hub_only + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_spec_validate.py b/tests/test_spec_validate.py index 5a10c594..2ce5fe11 100755 --- a/tests/test_spec_validate.py +++ b/tests/test_spec_validate.py @@ -568,6 +568,34 @@ def test_accepts_a_source_below_the_root(self) -> None: self.assertFalse(validate.reduces_to_repo_root(value)) +def run_gate_on_files(overrides: dict[str, object], make_tree=None) -> str: + """Run the real spec gate against a scratch tree whose files.json carries `overrides`, returning its output. + + The scratch tree holds spec/, registry/, an empty real/sub directory for a declared source to name, and the shipped repo-owned tests directory. + """ + with tempfile.TemporaryDirectory() as d: + root = Path(d) + shutil.copytree(validate.ROOT / "spec", root / "spec") + shutil.copytree(validate.ROOT / "registry", root / "registry") + (root / "real" / "sub").mkdir(parents=True) + (root / "tests").mkdir() + if make_tree is not None: + make_tree(root) + files_path = root / "spec" / "files.json" + files = json.loads(files_path.read_text(encoding="utf-8")) + files.update(overrides) + files_path.write_text(json.dumps(files), encoding="utf-8") + result = subprocess.run( + [sys.executable, str(root / "spec" / "validate.py")], + capture_output=True, + text=True, + encoding="utf-8", + timeout=60, + check=False, + ) + return result.stdout + result.stderr + + class TreePathGateCase(unittest.TestCase): """A tree path `scripts/carry.py` refuses is refused here too, run as the real script against a scratch tree. @@ -576,33 +604,14 @@ class TreePathGateCase(unittest.TestCase): """ def run_against(self, trees: list[dict], make_tree=None) -> str: - with tempfile.TemporaryDirectory() as d: - root = Path(d) - shutil.copytree(validate.ROOT / "spec", root / "spec") - shutil.copytree(validate.ROOT / "registry", root / "registry") - (root / "real" / "sub").mkdir(parents=True) - if make_tree is not None: - make_tree(root) - files_path = root / "spec" / "files.json" - files = json.loads(files_path.read_text(encoding="utf-8")) - marker = self.tree("real", "marker-target", fidelity="verbatim") - files["trees"] = trees + [marker] - files_path.write_text(json.dumps(files), encoding="utf-8") - result = subprocess.run( - [sys.executable, str(root / "spec" / "validate.py")], - capture_output=True, - text=True, - encoding="utf-8", - timeout=60, - check=False, - ) - output = result.stdout + result.stderr - self.assertIn( - "files.json: tree real fidelity must be 'verbatim-tree'", - output, - "the tree loop was never reached, so an absence assertion would be vacuous", - ) - return output + marker = self.tree("real", "marker-target", fidelity="verbatim") + output = run_gate_on_files({"trees": trees + [marker]}, make_tree) + self.assertIn( + "files.json: tree real fidelity must be 'verbatim-tree'", + output, + "the tree loop was never reached, so an absence assertion would be vacuous", + ) + return output @staticmethod def tree(source: str, target: str, fidelity: str = "verbatim-tree") -> dict: @@ -671,6 +680,59 @@ def test_every_tree_the_live_manifest_declares_passes_the_path_checks(self) -> N self.assertIsNone(validate.symlinked_component(validate.ROOT, value)) +class RepoOwnedTreeGateCase(unittest.TestCase): + """A repo-owned tree declaration the audit cannot use is refused, run as the real script against a scratch tree. + + Every run adds a marker entry with no reason, which the loop always reports, so each absence assertion proves the loop ran. + """ + + def run_against(self, owned: list[object], make_tree=None) -> str: + output = run_gate_on_files({"repoOwnedTrees": [*owned, {"source": "real"}]}, make_tree) + self.assertIn( + "files.json: repo-owned tree real reason must be a non-empty string", + output, + "the repo-owned tree loop was never reached, so an absence assertion would be vacuous", + ) + return output + + def test_a_misspelled_source_is_refused(self) -> None: + output = self.run_against([{"source": "rael", "reason": "r"}]) + self.assertIn("files.json: repo-owned tree rael is missing or not a directory", output) + + def test_a_root_escaping_or_unnormalized_source_is_refused(self) -> None: + for source in (".", "..", "../real", "./real", "real/.", "real//", "real/"): + with self.subTest(source=source): + self.assertIn( + f"files.json: repo-owned tree {source} source must be a normalized path below the repository root", + self.run_against([{"source": source, "reason": "r"}]), + ) + + def test_a_symlinked_source_component_is_refused(self) -> None: + for source in ("alias", "alias/sub"): + with self.subTest(source=source): + self.assertIn( + f"files.json: repo-owned tree {source} has a symlinked component alias, below which git tracks no file", + self.run_against( + [{"source": source, "reason": "r"}], TreePathGateCase.link_alias + ), + ) + + def test_an_entry_without_a_source_is_refused(self) -> None: + output = self.run_against([{"path": "real", "reason": "r"}]) + self.assertIn("files.json: repo-owned tree has an invalid source:", output) + + def test_a_plain_entry_draws_no_error(self) -> None: + output = self.run_against([{"source": "real/sub", "reason": "r"}]) + self.assertNotIn("repo-owned tree real/sub", output) + + def test_the_shipped_declaration_draws_no_error(self) -> None: + shipped = json.loads((validate.ROOT / "spec" / "files.json").read_text(encoding="utf-8")) + output = self.run_against(shipped["repoOwnedTrees"]) + for owned in shipped["repoOwnedTrees"]: + with self.subTest(source=owned["source"]): + self.assertNotIn(f"repo-owned tree {owned['source']} ", output) + + class RegistryNameUniquenessCase(unittest.TestCase): """A registry name is the key both configure.sh and audit.py resolve an entry by. From b472126d73923d269c9dafc2c4ce7c8eb4f52e53 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 27 Oct 2026 17:14:42 -0700 Subject: [PATCH 4/5] Re-Request Once on a Transient Copilot Fetch Error (#3542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What `pr_review.py` read every Copilot error refusal as a possible quota hit, so `wait` stopped and handed the drive to the maintainer. The same body is posted for a transient fetch failure that one re-request clears. `wait` and `status` now read the reviewer's run log for a `too many files changed` line before taking the quota reading. - `run_cause` also returns the log's transient-failure line, beside the error type and rate-limit sentence it already read. A logged rate limit still wins. - One primitive, `transient_state`, says where an error refusal stands on its one re-request: `owed`, `pending`, `sent`, or `spent`. `wait`'s request, its exit, and the digest line all read it. - `wait` sends the one re-request while the state is `owed`. A request made after the refusal spends it, so a request that vanished is not sent again. Two error refusals in a row spend it too. - A pending re-request polls until the next round lands, then exits on that round. With no round by the timeout it exits `30`, `status=PENDING`. - A held head, into a branch other than the default, exits `41` as `COPILOT_ERROR_TRANSIENT`, naming `--request` as what sends the re-request. Elsewhere an owed refusal steps aside, so an unrecorded request, an outside answer, or the repo-wide quota keeps its own status. - A spent or sent-and-unanswered re-request, an unread log, or a log naming neither cause keeps `COPILOT_ERROR_POSSIBLE_QUOTA`. Its status line now says which reading was taken. - `status` prints a `RUN LOG READING` line under an `ERROR` refusal, naming the reading taken. No exit code is added. The transient state rides the existing `0`, `30`, `40`, and `41` outcomes and their status lines. The help text, `scripts/README.md`, and `pr-review-conduct` describe the transient exception. The skill's generated copies are regenerated. `drive-pr` carries no changed section, so its copies are unchanged. ## Tests `tests/test_pr_review.py` covers the reading with constructed run logs and request histories: the transient line parsed with and without an error type, a rate limit outranking it, one re-request reviewed normally on the current head and on an earlier one, a second refusal ending the poll as quota, two refusals in a row, a pending re-request at the timeout, a re-request already sent, an unrecorded re-request, a second wait on a pending re-request, a held head, an unread log, and the digest's readings. ## Local strict review findings The first push left two `introduced` findings open once its edit budget was spent. Both are fixed in dc2bbe50, and that push's pass raised none. - `scripts/README.md:225`, `introduced`, fixed: it said `wait` exits `41` as `COPILOT_ERROR_TRANSIENT` wherever it sent none, while only a held head reaches that exit. - `scripts/pr_review.py` `transient_state`, `introduced`, fixed: the digest and the exit each read the request history. The read is now kept on the payload, so they share one answer. ## Filed elsewhere - #3541: the digest's fixed `COPILOT REFUSED THIS ROUND` text says "re-requesting repeats it", which a transient refusal contradicts, and the `RUN LOG READING` line belongs inside that block. Both need `scripts/pr_review_digest.py`, outside this change's files. Closes on promotion: #3510 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 20 +- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 20 +- .github/skills/pr-review-conduct/SKILL.md | 20 +- scripts/README.md | 2 +- scripts/pr_review.py | 164 ++++++++++++++-- tests/test_pr_review.py | 183 ++++++++++++++++++ 7 files changed, 373 insertions(+), 38 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 671226d5..aa65b01a 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -73,7 +73,12 @@ visible comments, routinely still carries a finding nobody has answered. Treatin `refusal=QUOTA` or `refusal=ERROR`, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, is the one nothing the loop does clears, which is the case "Which Reviewers a - Repository Actually Has" below states. `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same + Repository Actually Has" below states. An error refusal whose `RUN LOG READING` line says + transient is the exception. `wait` re-requests it once and exits `30` while that request is + pending. A second error refusal in a row, or a re-request made with no round answering it, + reads as possible quota. A held head exits `41` as `COPILOT_ERROR_TRANSIENT`, its request + waiting on `--request`. + `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and a refusal holding across several heads with no cause its body names reaches it the slower way. That exit is `wait`'s alone: `status` exits 0 over a refusal, carrying it as @@ -229,8 +234,9 @@ must have done. encountered an error, which is what the weekly rate limit posts. Each covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. Each refusal goes to the - maintainer, rather than into a wait. + reviewer's run log names a reset time `pr_review.py` reports it. Where it names a transient + fetch failure instead, `pr_review.py wait` re-requests once. Every other refusal, a repeat of + that one included, goes to the maintainer rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories @@ -267,13 +273,15 @@ Run `local-strict-review` against the branch's current diff before every push th deserves a Copilot round anyway. `wait` also skips the request where a review already covers the head, where the answer came outside a formal review, where it detects drift, under a quota or error refusal, and where something is already in the request set, which - is the condition the recovery below clears. Requesting in the pull request UI is the maintainer's - route rather than this loop's. + is the condition the recovery below clears. An error refusal whose run log reads transient is + the exception, owed one re-request per Merge Gate item 2. Requesting in the pull request UI is + the maintainer's route rather than this loop's. 4. Run a bounded `uv run --no-project --script scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one exception, and it is terminal the other way. Nothing follows it, and re-requesting the same - head only repeats the decline. + head only repeats the decline. A transient error refusal under Merge Gate item 2 is owed its one + re-request first. 5. Triage findings (see below). 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 3f76c820..aaea8af8 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -184af41cfc570242 +8743445c185bcca7 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 671226d5..aa65b01a 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -73,7 +73,12 @@ visible comments, routinely still carries a finding nobody has answered. Treatin `refusal=QUOTA` or `refusal=ERROR`, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, is the one nothing the loop does clears, which is the case "Which Reviewers a - Repository Actually Has" below states. `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same + Repository Actually Has" below states. An error refusal whose `RUN LOG READING` line says + transient is the exception. `wait` re-requests it once and exits `30` while that request is + pending. A second error refusal in a row, or a re-request made with no round answering it, + reads as possible quota. A held head exits `41` as `COPILOT_ERROR_TRANSIENT`, its request + waiting on `--request`. + `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and a refusal holding across several heads with no cause its body names reaches it the slower way. That exit is `wait`'s alone: `status` exits 0 over a refusal, carrying it as @@ -229,8 +234,9 @@ must have done. encountered an error, which is what the weekly rate limit posts. Each covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. Each refusal goes to the - maintainer, rather than into a wait. + reviewer's run log names a reset time `pr_review.py` reports it. Where it names a transient + fetch failure instead, `pr_review.py wait` re-requests once. Every other refusal, a repeat of + that one included, goes to the maintainer rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories @@ -267,13 +273,15 @@ Run `local-strict-review` against the branch's current diff before every push th deserves a Copilot round anyway. `wait` also skips the request where a review already covers the head, where the answer came outside a formal review, where it detects drift, under a quota or error refusal, and where something is already in the request set, which - is the condition the recovery below clears. Requesting in the pull request UI is the maintainer's - route rather than this loop's. + is the condition the recovery below clears. An error refusal whose run log reads transient is + the exception, owed one re-request per Merge Gate item 2. Requesting in the pull request UI is + the maintainer's route rather than this loop's. 4. Run a bounded `uv run --no-project --script scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one exception, and it is terminal the other way. Nothing follows it, and re-requesting the same - head only repeats the decline. + head only repeats the decline. A transient error refusal under Merge Gate item 2 is owed its one + re-request first. 5. Triage findings (see below). 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 671226d5..aa65b01a 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -73,7 +73,12 @@ visible comments, routinely still carries a finding nobody has answered. Treatin `refusal=QUOTA` or `refusal=ERROR`, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, is the one nothing the loop does clears, which is the case "Which Reviewers a - Repository Actually Has" below states. `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same + Repository Actually Has" below states. An error refusal whose `RUN LOG READING` line says + transient is the exception. `wait` re-requests it once and exits `30` while that request is + pending. A second error refusal in a row, or a re-request made with no round answering it, + reads as possible quota. A held head exits `41` as `COPILOT_ERROR_TRANSIENT`, its request + waiting on `--request`. + `status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and a refusal holding across several heads with no cause its body names reaches it the slower way. That exit is `wait`'s alone: `status` exits 0 over a refusal, carrying it as @@ -229,8 +234,9 @@ must have done. encountered an error, which is what the weekly rate limit posts. Each covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. Each refusal goes to the - maintainer, rather than into a wait. + reviewer's run log names a reset time `pr_review.py` reports it. Where it names a transient + fetch failure instead, `pr_review.py wait` re-requests once. Every other refusal, a repeat of + that one included, goes to the maintainer rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories @@ -267,13 +273,15 @@ Run `local-strict-review` against the branch's current diff before every push th deserves a Copilot round anyway. `wait` also skips the request where a review already covers the head, where the answer came outside a formal review, where it detects drift, under a quota or error refusal, and where something is already in the request set, which - is the condition the recovery below clears. Requesting in the pull request UI is the maintainer's - route rather than this loop's. + is the condition the recovery below clears. An error refusal whose run log reads transient is + the exception, owed one re-request per Merge Gate item 2. Requesting in the pull request UI is + the maintainer's route rather than this loop's. 4. Run a bounded `uv run --no-project --script scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one exception, and it is terminal the other way. Nothing follows it, and re-requesting the same - head only repeats the decline. + head only repeats the decline. A transient error refusal under Merge Gate item 2 is owed its one + re-request first. 5. Triage findings (see below). 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the diff --git a/scripts/README.md b/scripts/README.md index 40c8a91a..dcff52f7 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -222,7 +222,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `wait` does not classify a request from `copilot_work_started`, since GitHub can complete an effort-labeled review without that event. A request with no review or answer remains `PENDING` until the bounded timeout. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then requests afresh. The clear replaces that set rather than adding to it, so a stall on a pull request with a human or team reviewer requested goes to the maintainer, the mutation itself being in [`pr-reviewer-reference.md`][pr-reviewer-reference]. `--pickup-grace` remains accepted for command compatibility and has no effect. -`wait` exits `41` when the newest round is a refusal or a quota state. A refusal is a formal review carrying the head and no threads, which is the clean pass byte for byte, so it is not coverage: `review_on_head` reports `NO` and `refusal=` carries its kind. A quota refusal reads `refusal=QUOTA`. An error refusal is the body the weekly rate limit posts, so the job log of the reviewer's own failed run on that commit is read, a logged rate limit reading as `QUOTA` with its reset time and anything else as `ERROR`. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal on any head, and none where the reviewer's newest activity elsewhere in the repository is a quota refusal nothing answered since. Pass `--ignore-quota-signal` once the quota is believed to have reset. A file-count refusal is cleared by splitting the pull request. `status` exits `0` over a refusal and carries it in `refusal=`. +`wait` exits `41` when the newest round is a refusal or a quota state. A refusal is a formal review carrying the head and no threads, which is the clean pass byte for byte, so it is not coverage: `review_on_head` reports `NO` and `refusal=` carries its kind. A quota refusal reads `refusal=QUOTA`. An error refusal is the body the weekly rate limit posts, so the job log of the reviewer's own failed run on that commit is read, a logged rate limit reading as `QUOTA` with its reset time and anything else as `ERROR`. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal on any head, and none where the reviewer's newest activity elsewhere in the repository is a quota refusal nothing answered since. The exception is an error refusal whose run log names `too many files changed`, a transient fetch failure one re-request clears. `wait` sends that one re-request, unless the round before it was an error refusal too. A re-request already made after the refusal is never sent again, even where the request has since vanished. It exits `30` while that request is pending, and `41` as `COPILOT_ERROR_TRANSIENT` on a held head, where only `--request` sends it. A `RUN LOG READING` line in the digest names the reading taken, and a log unread or naming neither cause keeps the possible-quota reading. Pass `--ignore-quota-signal` once the quota is believed to have reset. A file-count refusal is cleared by splitting the pull request. `status` exits `0` over a refusal and carries it in `refusal=`. The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max`, with `effort_source=default` for `Default ()` and `explicit` for a bare level, and `unknown` for both where the body states none. Effort never changes a verdict, and the workflow never selects or changes the user-controlled setting. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 88f58f8e..6fd66235 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -77,8 +77,9 @@ `other_rate_limited` names one whose newest comment or review is its own rate-limit notice. `refusal=` reads the newest Copilot review where it is a refusal: `QUOTA` for the account quota or an error whose run log names a rate limit, `ERROR` for any other - error refusal, `YES` for any other refusal. `status` exits 0 over a refusal, and `wait` - gives it 41. + error refusal, `YES` for any other refusal. Under `ERROR`, a `RUN LOG READING` line + names the reading taken: transient where the run log names a fetch failure, else + possible quota. `status` exits 0 over a refusal, and `wait` gives it 41. `unresolved` counts every open thread whoever opened it, its breakdown attributing the known logins. Both `threads=` and `unresolved=` read one 100-thread page, a trailing `+` and a `THREADS TRUNCATED` block naming a cut page. `suppressed=`, @@ -106,7 +107,10 @@ current head, then print the digest. The auto-request is skipped once a review past --min-rounds covers the head, once Copilot has answered outside a formal review, once one is pending, and under a refusal or quota reading below, since a request into a - reached limit spends quota and returns the same refusal. It is skipped on a pull + reached limit spends quota and returns the same refusal. An error refusal whose run log + names a transient fetch failure is owed one re-request, unless the round before it was + an error refusal too. A re-request already made after the refusal is never sent again. + A held head names it under COPILOT_ERROR_TRANSIENT instead. It is skipped on a pull request into a branch other than the default too, whose every head an attested local pass covers: an attested head has its checks polled instead while they can still move its merge, and one with no attestation exits 49. --request asks for a round anyway. @@ -139,7 +143,7 @@ reviewer's newest activity elsewhere in the repository being a quota refusal nothing answered since while this head carries none of its own. Proceed on the other reviewers' coverage rather than retrying, and pass --ignore-quota-signal once the quota is believed - to have reset. 42 and 50 = the review landed and `status`'s readings + to have reset. A transient error refusal whose one re-request is pending exits 30 instead. 42 and 50 = the review landed and `status`'s readings apply to it. 44 = the review loop closed, the merge reads BLOCKED, and a required check is in a shape no wait clears. It is queued with nothing acting on it, expected and never posted, running far past the job's cost, held by its environment, or failed. On @@ -220,6 +224,8 @@ local_cover, matching_threads, newest_first, + newest_of, + newest_request_event, open_threads, possible_quota, quota_refusal, @@ -245,7 +251,9 @@ RUN_RATE_LIMIT = re.compile( r"(You.ve reached your [^\n]*?rate limit\.[^\n]*?)(?= or switch| Learn More|$)", re.MULTILINE ) +RUN_TRANSIENT = re.compile(r"^.*too many files changed.*$", re.MULTILINE) ANSI = re.compile(r"\x1b\[[0-9;]*m") +REQUESTS_KEY = "requestState" ISO_STAMP = re.compile(r"\d{4}-\d\d-\d\dT\d\d:\d\d:\d\dZ") @@ -306,7 +314,7 @@ REQUEST_STATE = """ reviewRequests(first:100){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } timelineItems(last:100, itemTypes:[REVIEW_REQUESTED_EVENT]){ nodes{ - ... on ReviewRequestedEvent{ id requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } + ... on ReviewRequestedEvent{ id createdAt requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } """ Q_REQUEST_STATE = """ query($o:String!,$r:String!,$n:Int!){ @@ -588,11 +596,12 @@ def request_copilot_review( @functools.cache -def run_cause(owner: str, repo: str, oid: str, before: str) -> tuple[str, str] | None: - """The error type and rate-limit sentence the reviewer's own failed run logged, or None. +def run_cause(owner: str, repo: str, oid: str, before: str) -> tuple[str, str, str] | None: + """The error type, rate-limit sentence, and transient-failure line the reviewer's failed run logged. - The run read is the newest failed one on that commit created no later than the review. None - wherever any read fails or finds nothing. Memoized for the run. + The run read is the newest failed one on that commit created no later than the review. Each + part is empty where the log holds none of it. None wherever any read fails or finds nothing. + Memoized for the run. """ if not oid or not ISO_STAMP.fullmatch(before): return None @@ -613,24 +622,89 @@ def run_cause(owner: str, repo: str, oid: str, before: str) -> tuple[str, str] | return None log = ANSI.sub("", proc.stdout) kinds, said = RUN_ERROR_TYPE.findall(log), RUN_RATE_LIMIT.search(log) - if not kinds: + fetch = RUN_TRANSIENT.findall(log) + if not kinds and not fetch: return None - kind = "rate_limit" if "rate_limit" in kinds else kinds[-1] - return kind, said.group(1).strip() if said else "" + kind = "rate_limit" if "rate_limit" in kinds else kinds[-1] if kinds else "" + return kind, said.group(1).strip() if said else "", fetch[-1].strip() if fetch else "" -def confirmed_quota(owner: str, repo: str, node: dict) -> str: - """The run log's rate-limit sentence where an error refusal's run logged a rate limit, else "".""" +def node_cause(owner: str, repo: str, node: dict) -> tuple[str, str, str] | None: + """`run_cause` for an error refusal's own commit and submission time, else None.""" if not possible_quota(node): - return "" - cause = run_cause( + return None + return run_cause( owner, repo, (node.get("commit") or {}).get("oid") or "", node.get("submittedAt") or "" ) + + +def confirmed_quota(owner: str, repo: str, node: dict) -> str: + """The run log's rate-limit sentence where an error refusal's run logged a rate limit, else "".""" + cause = node_cause(owner, repo, node) if cause is None or cause[0] != "rate_limit": return "" return cause[1] or "the run logged a rate limit and stated no reset time" +def transient_error(owner: str, repo: str, node: dict) -> str: + """The run log's transient-failure line where an error refusal's run logged no rate limit, else "". + + A logged rate limit wins, since a re-request into a reached limit spends quota. + """ + cause = node_cause(owner, repo, node) + if cause is None or cause[0] == "rate_limit": + return "" + return cause[2] + + +def transient_state(owner: str, repo: str, num: int, pr: dict, node: dict) -> tuple[str, str]: + """Where an error refusal stands on its one re-request, and its run log's transient line. + + The state is `owed` until a request postdates the refusal, then `pending` while one is + requested, else `sent`. It is `spent` where the round before was an error refusal too. Both + parts are empty where the refusal is no error refusal or its log names no transient failure. + The request read is kept on `pr`, so the digest and the exit read one answer. + """ + line = transient_error(owner, repo, node) if possible_quota(node) else "" + if not line: + return "", "" + revs = reviewer_nodes(pr, "reviews") + at = next((i for i, n in enumerate(revs) if n is node), 0) + before = newest_of(revs[:at]) + if before is not None and possible_quota(before): + return "spent", line + if REQUESTS_KEY not in pr: + pr[REQUESTS_KEY] = gh_graphql(Q_REQUEST_STATE, o=owner, r=repo, n=num)["repository"][ + "pullRequest" + ] + state = pr[REQUESTS_KEY] + newest = newest_request_event(state) + made = next( + ( + n.get("createdAt") or "" + for n in state["timelineItems"]["nodes"] + if n.get("id") == newest + ), + "", + ) + if made <= (node.get("submittedAt") or ""): + return "owed", line + return ("pending" if reviewer_requested(pr) else "sent"), line + + +def run_log_reading(state: str, line: str) -> str: + """The digest line naming the reading `transient_state` gave, empty where it gave none.""" + readings = { + "owed": "transient. The run log names a fetch failure, not a rate limit, so one " + "re-request is owed", + "pending": "transient. The one re-request it is owed is pending", + "sent": "possible quota. The one re-request it was owed was sent and nothing answered it", + "spent": "possible quota. The run log names a fetch failure, but the round before was " + "an error refusal too", + } + return f" RUN LOG READING: {readings[state]}: {line}" if state else "" + + def full_read(owner: str, repo: str, num: int, pr: dict | None = None) -> dict: """The full payload with its changed files paged, read again while that list is unread.""" @@ -769,7 +843,16 @@ def digest( checks = check_nodes(pr) if checks is None else checks refusal = digest_refusal(pr) quota = confirmed_quota(owner, repo, refusal) if refusal else "" - return render(owner, repo, num, pr, checks, now, grace, stall, quota, seen, HUB, WINDOW) + out, unresolved = render( + owner, repo, num, pr, checks, now, grace, stall, quota, seen, HUB, WINDOW + ) + reading = "" + if refusal and possible_quota(refusal) and not quota: + reading = run_log_reading(*transient_state(owner, repo, num, pr, refusal)) or ( + " RUN LOG READING: possible quota. The run log was unread or names neither a rate " + "limit nor a transient fetch failure" + ) + return (f"{out}\n{reading}" if reading else out), unresolved def report_verdict(pr: dict) -> int: @@ -1448,6 +1531,12 @@ def run_wait(owner: str, repo: str, a: argparse.Namespace) -> int: and holds(snapshot) and (not answer or attested(snapshot)) ) + retry, watch = "", False + if snapshot and stopped and not held: + state, line = transient_state(owner, repo, a.number, snapshot, stopped) + retry, watch = (line if state == "owed" else ""), state in {"owed", "pending"} + if retry: + stopped = None # Request before the first poll, so `wait` makes a review happen rather than only watching for one. # Skipped once a round past --min-rounds covers the head, Copilot answered outside a review, or a request is pending. recorded: bool | None = None @@ -1471,6 +1560,11 @@ def run_wait(owner: str, repo: str, a: argparse.Namespace) -> int: else: line += ", and the request recorded nothing on the pull request" print(f"auto-request: {line}") + if retry and recorded is not False: + print( + "note: the newest Copilot review is an error refusal whose run log names a " + f"transient fetch failure, so this is its one re-request: {retry}" + ) if recorded is False: print( "note: the review request returned success and recorded nothing on this pull " @@ -1541,6 +1635,14 @@ def held_read() -> dict: "rather than polling --timeout out against the same account state. Pass " "--ignore-quota-signal to poll anyway, once the quota is believed to have reset." ) + elif watch: + + def read_live() -> tuple[bool, dict | None, list[str], bool]: + live = gql(Q_LIVE, owner, repo, a.number) + fresh = len(reviewer_nodes(live, "reviews")) > rounds_before + return (*liveness(live, a.min_rounds), fresh) + + backoff((done, answer, drift, False), read_live, lambda s: not any(s), start, a.timeout) else: backoff( (done, answer, drift), @@ -1632,6 +1734,23 @@ def held_read() -> dict: refusal = refusing_review(final) if refusal is None and not a.ignore_quota_signal: refusal = stopping_refusal(final) + state, line = transient_state(owner, repo, a.number, final, refusal) if refusal else ("", "") + if state == "pending" or (state == "owed" and reviewer_requested(final)): + print( + "status=PENDING the newest Copilot review is an error refusal whose run log names a " + "transient fetch failure. Its one re-request is pending with no round by the timeout, " + f"so run wait again: {line}" + ) + return 30 + if state == "owed" and held: + print( + "status=COPILOT_ERROR_TRANSIENT the newest Copilot review is an error refusal whose " + "run log names a transient fetch failure, which one re-request clears. This wait " + f"sent none, so pass --request to send it: {line}" + ) + return 41 + if state == "owed": + refusal = None if refusal and quota_refusal(refusal): print( "status=COPILOT_QUOTA_EXHAUSTED the newest Copilot review declined because the " @@ -1651,10 +1770,19 @@ def held_read() -> dict: ) return 41 if refusal and possible_quota(refusal): + why = { + "sent": "Its run log names a transient fetch failure, and its one re-request was " + "sent with no round answering it.", + "spent": "Its run log names a transient fetch failure, but the round before it was an " + "error refusal too.", + }.get( + state, "Its run log was unread or names neither a rate limit nor a transient failure." + ) print( "status=COPILOT_ERROR_POSSIBLE_QUOTA the newest Copilot review on this pull request " "says it encountered an error and did not review, printed above under COPILOT " - "REFUSED THIS ROUND, which is the body the weekly rate limit posts. Read it as a " + "REFUSED THIS ROUND, which is the body the weekly rate limit posts. " + f"{why} Read it as a " "possible quota hit: do not re-request, proceed on the coverage the other reviewers " "already gave, and hand the state to the maintainer, who can pass " "--ignore-quota-signal once the limit is believed to have reset" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index d0cbff72..9e55e9d9 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -261,6 +261,24 @@ def summarized(paths: list[str], covers: str = COVERED) -> str: "2026-08-02T10:59:00.0000000Z errorType: 'rate_limit',\n" ) +TRANSIENT_LOG = ( + "2026-08-02T10:59:00.0000000Z HttpError: The request could not be processed because too " + "many files changed - https://docs.example.test/pulls: Failed to fetch PR metadata for o/r#7\n" +) +TRANSIENT_LINE = TRANSIENT_LOG.strip() +SENT_AFTER = { + "reviewRequests": {"nodes": []}, + "timelineItems": { + "nodes": [ + { + "id": "RRE_1", + "createdAt": "2026-08-02T10:30:00Z", + "requestedReviewer": {"__typename": "Bot", "login": pr_review_rounds.REVIEWER}, + } + ] + }, +} + # Quoted from the corpus rather than invented: the body a refused review carried, byte for byte. QUOTA_REFUSED = ( "Copilot was unable to review this pull request because the user who requested the " @@ -4536,6 +4554,171 @@ def test_the_digest_reads_an_error_round_by_what_its_run_logged(self) -> None: self.assertIn(field, out) self.assertEqual(line, "RATE LIMIT FROM THE REVIEWER'S RUN LOG" in out) + def test_a_transient_fetch_failure_in_the_run_log_is_its_own_cause(self) -> None: + for log, want in ( + (TRANSIENT_LOG, ("", "", TRANSIENT_LINE)), + ("x errorType: 'internal',\n" + TRANSIENT_LOG, ("internal", "", TRANSIENT_LINE)), + ("x errorType: 'internal',\n", ("internal", "", "")), + ("nothing the reader names\n", None), + ): + with self.subTest(want=want): + pr_review.run_cause.cache_clear() + self.run_log(log) + self.assertEqual(want, pr_review.run_cause("o", "r", HEAD, LATE)) + + def test_a_logged_rate_limit_outranks_a_transient_line(self) -> None: + self.run_log(TRANSIENT_LOG + RATE_LIMIT_LOG) + node = review(body=ERROR_REFUSED, at=LATE) + self.assertEqual("", pr_review.transient_error("o", "r", node)) + self.assertIn("reset on August 9", pr_review.confirmed_quota("o", "r", node)) + + def test_a_transient_error_round_is_re_requested_once_and_reviewed(self) -> None: + """The refusal on the current head is the issue's own case, and an earlier head behaves alike.""" + for oid in (HEAD, OLD): + with self.subTest(head=oid == HEAD): + pr_review.run_cause.cache_clear() + self.out.seek(0) + self.out.truncate() + refused = review(oid=oid, body=ERROR_REFUSED, at=EARLY, rid="PRR_a") + answered = payload([refused, review(at=LATE, rid="PRR_b")]) + self.answer(self.bodyless(OLD), payload([refused]), answered) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + self.assertEqual(1, slept.call_count) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("so this is its one re-request: " + TRANSIENT_LINE, out) + self.assertNotIn("COPILOT_ERROR_POSSIBLE_QUOTA", out) + + def test_a_second_error_round_after_the_re_request_ends_the_poll_as_quota(self) -> None: + """The second refusal is no head review, so only the round count ends the poll early.""" + first = review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_a") + again = payload([first, review(body=ERROR_REFUSED, at=LATE, rid="PRR_b")]) + self.answer(self.bodyless(OLD), payload([first]), again) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(41, self.cli(["wait", "7"])) + self.assertEqual(1, slept.call_count) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("status=COPILOT_ERROR_POSSIBLE_QUOTA", out) + self.assertIn("but the round before it was an error refusal too", out) + + def test_two_error_rounds_in_a_row_request_nothing_whatever_the_log_says(self) -> None: + rounds = [ + review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_a"), + review(oid=OLD, body=ERROR_REFUSED, at=LATE, rid="PRR_b"), + ] + self.answer(self.bodyless(OLD), payload(rounds)) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(41, self.cli(["wait", "7", "--timeout", "0"])) + slept.assert_not_called() + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("but the round before it was an error refusal too", self.out.getvalue()) + + def test_a_pending_re_request_with_no_round_by_the_timeout_is_pending(self) -> None: + refused = [review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_a")] + self.answer(self.bodyless(OLD), payload(refused), payload(refused, pending=True)) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + reads = [q for q, _ in calls if "REVIEW_REQUESTED_EVENT" in q and "requestReviews" not in q] + self.assertEqual(3, len(reads), "the first payload, the request, then one the last shares") + out = self.out.getvalue() + self.assertIn("status=PENDING the newest Copilot review is an error refusal", out) + self.assertNotIn("COPILOT_ERROR_POSSIBLE_QUOTA", out) + + def test_a_re_request_already_sent_is_not_sent_again(self) -> None: + """The bound counts requests rather than rounds, so a request that vanished still spends it.""" + sent = SENT_AFTER + self.answer(self.bodyless(OLD), payload([review(oid=OLD, body=ERROR_REFUSED, at=EARLY)])) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)], (sent, sent)) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(41, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("one re-request was sent with no round answering it", self.out.getvalue()) + + def test_a_re_request_recording_nothing_is_reported_as_unrecorded(self) -> None: + refused = payload([review(oid=OLD, body=ERROR_REFUSED, at=EARLY)]) + self.answer(self.bodyless(OLD), refused) + self.wire_history([hist_review(7, ERROR_REFUSED)], (request_state(), request_state())) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(40, self.cli(["wait", "7", "--timeout", "0"])) + out = self.out.getvalue() + self.assertIn("status=REQUEST_NOT_RECORDED", out) + self.assertNotIn("this is its one re-request", out) + self.assertNotIn("COPILOT_ERROR_TRANSIENT", out) + + def test_a_wait_on_a_pending_re_request_ends_on_the_next_round(self) -> None: + """A second refusal is no head review, so the round count is what ends this poll early.""" + first = review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_a") + live = payload([{k: v for k, v in first.items() if k != "body"}], pending=True) + again = payload([first, review(body=ERROR_REFUSED, at=LATE, rid="PRR_b")]) + self.answer(live, payload([first], pending=True), again) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)], (SENT_AFTER, SENT_AFTER)) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(41, self.cli(["wait", "7"])) + self.assertEqual(1, slept.call_count) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("but the round before it was an error refusal too", self.out.getvalue()) + + def test_a_held_head_names_the_owed_re_request_rather_than_quota(self) -> None: + self.answer(self.into(payload([review(oid=OLD, body=ERROR_REFUSED)]))) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(TRANSIENT_LOG) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(41, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("status=COPILOT_ERROR_TRANSIENT", out) + self.assertIn("pass --request to send it", out) + + def test_an_unread_run_log_keeps_the_quota_reading_and_says_so(self) -> None: + self.answer(self.bodyless(OLD), payload([review(oid=OLD, body=ERROR_REFUSED)])) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(None) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(41, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("Its run log was unread or names neither", self.out.getvalue()) + + def test_the_digest_names_the_run_log_reading_it_took(self) -> None: + one = [review(body=ERROR_REFUSED, at=LATE, rid="PRR_a")] + two = [review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_z"), *one] + early = [review(body=ERROR_REFUSED, at=EARLY, rid="PRR_a")] + transient = "RUN LOG READING: transient. The run log names" + spent = "possible quota. The run log names a fetch failure, but the round" + sent = "possible quota. The one re-request it was owed was sent" + unread = "RUN LOG READING: possible quota. The run log was unread" + for rounds, log, states, line in ( + (one, TRANSIENT_LOG, RECORDED, transient), + (two, TRANSIENT_LOG, RECORDED, spent), + (early, TRANSIENT_LOG, (SENT_AFTER, SENT_AFTER), sent), + (one, None, RECORDED, unread), + (one, RATE_LIMIT_LOG, RECORDED, None), + ): + with self.subTest(line=line): + pr_review.run_cause.cache_clear() + self.answer(payload(rounds)) + self.wire_history([], states) + self.run_log(log) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=QUOTA" if line is None else "refusal=ERROR", out) + if line is None: + self.assertNotIn("RUN LOG READING", out) + else: + self.assertIn(line, out) + class TestCopilotHistoryReadings(GqlCase): """The readings built from the reviewer's own review history across the repository.""" From e1c3a01876be775e9c0d333f145d5fdbf319bb9e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 27 Oct 2026 17:48:56 -0700 Subject: [PATCH 5/5] Refuse a Repo-Owned Tree Overlapping Carried Content (#3545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This is a promotion fix for review thread `PRRT_kwDOQ5caqM6w8BW3` on #3544. `spec/validate.py` now refuses a `repoOwnedTrees` source that overlaps carried content. The audit skips every hub file below a repo-owned source. An overlap would therefore hide hub-only findings while the manifest still claims the content is carried. The check refuses three shapes: - a source equal to a `trees[].source` - a source that is an ancestor or a descendant of a `trees[].source` - a source with a `baseline` path at or below it `RepoOwnedTreeGateCase` runs the real gate on a scratch tree for each refused shape, plus a control beside the carried content. The fixture paths are constructed. The shipped `spec/files.json` still passes. ## Local review The pass raised one finding, declined on evidence. - `spec/validate.py`, the new tree-source comparison, `introduced`, design. The reviewer asked for one `paths_overlap` helper shared with the tree-target loop. Declined: no such helper exists to reuse, and the repeated expression is correct rather than a defect class. Moving the tree-target site is outside this change's claim, which is the `repoOwnedTrees` check only. Closes on promotion: #3528 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- spec/validate.py | 22 +++++++++++++- tests/test_spec_validate.py | 57 +++++++++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/spec/validate.py b/spec/validate.py index 707eba4a..aa303b8d 100755 --- a/spec/validate.py +++ b/spec/validate.py @@ -1480,12 +1480,32 @@ def check_selector(where, applies_to): errors.append( f"files.json: repo-owned tree {source} source must be a normalized path below the repository root" ) - elif (link := symlinked_component(ROOT, source)) is not None: + continue + if (link := symlinked_component(ROOT, source)) is not None: errors.append( f"files.json: repo-owned tree {source} has a symlinked component {link}, below which git tracks no file" ) elif not (ROOT / source).is_dir(): errors.append(f"files.json: repo-owned tree {source} is missing or not a directory") + owned_path = pathlib.PurePosixPath(source) + for tree in validated_trees: + tree_source = pathlib.PurePosixPath(tree["source"]) + if ( + owned_path == tree_source + or owned_path in tree_source.parents + or tree_source in owned_path.parents + ): + errors.append( + f"files.json: repo-owned tree {source} and tree source {tree_source} have overlapping ownership" + ) + for item in baseline: + if not isinstance(item, dict) or not isinstance(item.get("path"), str): + continue + item_path = pathlib.PurePosixPath(item["path"]) + if item_path == owned_path or owned_path in item_path.parents: + errors.append( + f"files.json: repo-owned tree {source} and baseline path {item_path} have overlapping ownership" + ) copilot_path = ROOT / ".github/copilot-instructions.md" try: diff --git a/tests/test_spec_validate.py b/tests/test_spec_validate.py index 2ce5fe11..7fa2009c 100755 --- a/tests/test_spec_validate.py +++ b/tests/test_spec_validate.py @@ -15,6 +15,7 @@ import unittest import unittest.mock from pathlib import Path +from typing import ClassVar sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "spec")) import audit @@ -686,8 +687,12 @@ class RepoOwnedTreeGateCase(unittest.TestCase): Every run adds a marker entry with no reason, which the loop always reports, so each absence assertion proves the loop ran. """ - def run_against(self, owned: list[object], make_tree=None) -> str: - output = run_gate_on_files({"repoOwnedTrees": [*owned, {"source": "real"}]}, make_tree) + def run_against( + self, owned: list[object], make_tree=None, overrides: dict[str, object] | None = None + ) -> str: + output = run_gate_on_files( + {**(overrides or {}), "repoOwnedTrees": [*owned, {"source": "real"}]}, make_tree + ) self.assertIn( "files.json: repo-owned tree real reason must be a non-empty string", output, @@ -725,6 +730,54 @@ def test_a_plain_entry_draws_no_error(self) -> None: output = self.run_against([{"source": "real/sub", "reason": "r"}]) self.assertNotIn("repo-owned tree real/sub", output) + CARRIED: ClassVar[dict[str, object]] = { + "trees": [ + { + "source": "carried/kit", + "target": "carried/kit", + "fidelity": "verbatim-tree", + "appliesTo": "*", + "include": ["**/*"], + "prune": True, + } + ], + "baseline": [ + {"path": "shared/doc.md", "fidelity": "verbatim", "whole": True, "appliesTo": "*"} + ], + } + + @staticmethod + def make_carried(root: Path) -> None: + (root / "carried" / "kit" / "inner").mkdir(parents=True) + (root / "shared").mkdir() + (root / "shared" / "doc.md").write_text("doc\n", encoding="utf-8") + + def run_carried(self, source: str) -> str: + return self.run_against( + [{"source": source, "reason": "r"}], self.make_carried, self.CARRIED + ) + + def test_a_source_overlapping_a_tree_source_is_refused(self) -> None: + for source in ("carried/kit", "carried", "carried/kit/inner"): + with self.subTest(source=source): + self.assertIn( + f"files.json: repo-owned tree {source} and tree source carried/kit have overlapping ownership", + self.run_carried(source), + ) + + def test_a_source_holding_a_baseline_path_is_refused(self) -> None: + for source in ("shared", "shared/doc.md"): + with self.subTest(source=source): + self.assertIn( + f"files.json: repo-owned tree {source} and baseline path shared/doc.md have overlapping ownership", + self.run_carried(source), + ) + + def test_a_source_beside_carried_content_draws_no_error(self) -> None: + output = self.run_carried("real/sub") + self.assertNotIn("repo-owned tree real/sub", output) + self.assertNotIn("overlapping ownership", output) + def test_the_shipped_declaration_draws_no_error(self) -> None: shipped = json.loads((validate.ROOT / "spec" / "files.json").read_text(encoding="utf-8")) output = self.run_against(shipped["repoOwnedTrees"])