diff --git a/.agents/skills/backlog-burndown/SKILL.md b/.agents/skills/backlog-burndown/SKILL.md index 47c14be1..ad15165b 100644 --- a/.agents/skills/backlog-burndown/SKILL.md +++ b/.agents/skills/backlog-burndown/SKILL.md @@ -58,7 +58,7 @@ it was invoked for is narrower than the rule requires, deliberately. - **The grant is bounded by the session it was named in.** A run interrupted and resumed in a new session needs the skill named again, which costs one sentence and is the difference between a grant and a mode. A grant read back from a note is one nobody gave. -- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's item 5 for +- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's explicit-permission item for this run's feature -> develop merges and nothing else, so a pull request with one open finding still does not merge. - It is never authorization to merge a develop -> main promotion pull request, to dispatch a @@ -210,15 +210,16 @@ Brief on `AGENTS.md` "Context and Delegation Discipline"'s subagent shape. - **The worker drives its group to a develop merge**, by invoking `drive-pr` with the target stated as develop only. That skill owns the review loop, the finding disposition, and the merge, so brief the group and the bounds rather than restating the loop. -- **The worker creates its own worktree**, always, as `drive-pr` step 1 and `repo-worktree`'s - task-start mandate already require of the task itself. No worker inherits another's worktree, +- **The worker creates its own worktree**, always, as `drive-pr`'s worktree isolation and + `repo-worktree`'s task-start mandate already require of the task itself. No worker inherits another's worktree, which is why "Bounding the Wait on a Worker" either removes a dead worker's tree and its branch or leaves that tree untouched for the maintainer, and never passes it on. -- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr` step 4 - and of `repo-worktree`'s post-merge procedure. Say so in the brief, because a worker following - either alone will clean up. The worker still performs step 4's merge itself, and what the override - moves is that step's two cleanup halves, the worktree procedure and the verify-then-delete of the - merged remote branch, **both** rather than only the first. "Cleanup Is the Orchestrator's" below, in this +- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr`'s + post-merge cleanup and of `repo-worktree`'s post-merge procedure. Say so in the brief, because + a worker following either alone will clean up. The worker still performs the merge itself, and + what the override moves is the two cleanup halves `drive-pr` runs after it, the worktree + procedure and the verify-then-delete of the merged remote branch, **both** rather than only the + first. "Cleanup Is the Orchestrator's" below, in this same section, says why and what it covers. - **The worker runs `local-strict-review` before every push**, including one that only fixes a review finding. That pass dispatches a reviewer of its own, so a harness where a subagent cannot @@ -260,8 +261,8 @@ finish the procedure from inside its own worktree, since removing that worktree working directory in which to delete its branch. So the whole procedure moves to the orchestrator, which runs it from the base clone at the round's -cleanup step, while no worker is live in a tree it touches. It carries `drive-pr` step 4's remote -half too, verifying the merged branch's tip against the pull request's `headRefOid` before +cleanup step, while no worker is live in a tree it touches. It carries the remote half of `drive-pr`'s +post-merge cleanup too, verifying the merged branch's tip against the pull request's `headRefOid` before `git push origin --delete`, since taking that step from the worker without naming a new owner would leave a live remote branch behind every group. It covers every group that is done with its tree, which is the finished ones **and the abandoned ones**: a group told to abandon its branch keeps a @@ -398,11 +399,13 @@ has carried. can still owe a promotion pull request, for work an earlier round landed and no promotion has yet carried. A count of zero is the only case with nothing to promote, and the round reports that instead of attempting one. -2. Drive its review loop per `drive-pr` steps 5 through 8, **with a review-round budget set before - the first one**, the same discipline "Bounding a Prose Group" applies to a feature branch. That - loop repeats until the promotion pull request carries no open finding, and nothing in it - terminates on its own, so when the budget is reached, stop and put the state to the maintainer - rather than continuing to spend the run's only forward gear on one pull request. +2. Drive its review loop per the promotion half of `drive-pr` "The Drive Loop", **with a + review-round budget set before the first one**, the same discipline "Bounding a Prose Group" + applies to a feature branch. That loop repeats until the promotion pull request meets every + `pr-review-conduct` Merge Gate item except the maintainer's explicit permission to merge, and + nothing in that loop terminates on its own, so when the budget is reached, stop and put the + state to the maintainer rather than continuing to spend the run's only forward gear on one pull + request. 3. Put the ready pull request to the maintainer through the interface's own prompt mechanism, naming the merge as the action that unblocks the run. The maintainer's merge is the run's clock, so one reported in a closing paragraph and never actually asked about stalls every round behind it. @@ -411,8 +414,8 @@ has carried. it lands as its own feature -> develop pass, and that landing moving its head is expected, since its head **is** develop. **That pass is dispatched as a worker like any other**, which is the one push the freeze permits and the reason the orchestrator still opens no branch of its own. - `drive-pr` step 6 sends the seat driving a promotion pull request back through its own steps 1 - to 4 for such a fix, and here that seat dispatches rather than drives it. + `drive-pr` "The Drive Loop" sends the seat driving a promotion pull request back through its + own feature -> develop pass for such a fix, and here that seat dispatches rather than drives it. 5. **A promotion fix outranks any file claim.** A group holding a file it needs yields, because the promotion pull request is what the whole run is queued behind. A holder that is merely parked yields by handing the file over. A holder that already pushed and has an open pull request @@ -430,7 +433,7 @@ has carried. 6. **Nothing else pushes, and nothing else is dispatched.** The promotion fix of step 4 is the one exception to both, and everything in this step is said of the next round's work rather than of it. That round's preparation is orchestrator work and continues: rank, group, and verify claims. - Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, whose second step + Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, which pushes and opens a pull request, so a next-round worker dispatched under the freeze would either break it or sit in a state that procedure does not describe. None is left running across the wait either, since a worker held idle for an unbounded maintainer wait is one doing nothing at a diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index b234fb29..9ea24f53 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -2,19 +2,19 @@ name: drive-pr description: >- Drives a ptr727/ProjectTemplate fleet pull request through its review loop, feature branch into - develop and, when asked, on to a mergeable develop -> main promotion PR, applying the - pr-review-conduct disposition to every reviewer finding along the way: fix it, decline it with - evidence, defer it behind a filed issue, or put the call to the maintainer and wait for an - explicit answer in the same turn, escalating to whoever dispatched the drive instead where the - maintainer cannot be reached from that seat. Use this whenever asked to drive, land, take, chase, or push + develop and, when asked, on to a mergeable develop -> main promotion PR, disposing of every + reviewer finding along the way under pr-review-conduct's outcomes, carried here whole as a + generated include, and escalating to whoever dispatched the drive where the drive's own seat + cannot reach the maintainer. Use this whenever asked to drive, land, take, chase, or push a PR toward develop or main, or to run the review loop hands off instead of narrating each round. When the request does not say how far ("drive this PR", "land it"), ask once whether the target is develop or a mergeable main promotion PR, rather than guessing. Triggers even when only one PR is named, because a finding raised against the develop -> main promotion PR routinely needs its own feature -> develop fix cycle before the promotion PR can go green, and stopping at the first promotion-PR finding is the early exit this skill exists to prevent. Ends - at develop merged, or at a promotion PR meeting the pr-review-conduct Merge Gate, never merges - main itself, that is the separate merge-and-release skill, its own go-ahead. + at develop merged, or at a promotion PR meeting every pr-review-conduct Merge Gate item except + the maintainer's explicit permission to merge, never merges main itself, that is the separate + merge-and-release skill, its own go-ahead. --- # Drive PR @@ -128,32 +128,57 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. 1 to 4 in its own worktree and branch, then return here. 7. The fix landing on develop updates the promotion PR's diff and head SHA on its own, re-request a review on the new head and continue the loop. -8. Repeat 6 and 7 until the promotion PR itself carries no open finding and its checks are green - on the current head. +8. Repeat 6 and 7 until the promotion PR meets every pr-review-conduct Merge Gate item except the + maintainer's explicit permission to merge. 9. Report the promotion PR number and its ready state. Do not merge it. ## Disposing of Every Finding -pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: - -- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA - (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and - the branch was already reviewed once, and a fix push carries content no pass has read exactly as - the first push did. -- Not real, or real but out of scope here, so decline in the thread with evidence: the command - and its output, the code path, or the rule that governs it. An assertion never closes a finding - on its own (outcome 2). -- Real and worth doing, but later, so file the issue first, then reply with its link (outcome 4). -- Real, fixable, but a value call rather than a scope boundary, or the agent genuinely does not - know which of the above applies, so ask the maintainer directly, whatever the runtime's own - interactive-question mechanism is, and get an explicit answer in the same turn, a plan to ask - later is resolution by silence (outcome 3). A drive that cannot reach the - maintainer directly, a dispatched one being the ordinary case, escalates to whoever dispatched - it and stops that unit of work there instead, per `pr-review-conduct`, which owns what the - receiving seat then does and how far the escalation travels. -- The same finding keeps recurring against correct code, fix the class, sharpen a name, add a - comment, or take the rule itself to the maintainer, rather than re-arguing the instance every - round (outcome 5). +The rule below is a generated include, so a defect in it is fixed in `pr-review-conduct` and +regenerated rather than edited here. A drive that cannot reach the maintainer directly, a +dispatched one being the ordinary case, escalates per `pr-review-conduct` "Escalate to the +maintainer when". + + + +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per + `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent + elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. +2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** + Disprove a wrong finding with the command and its output, the code path that makes it + impossible, or the rule that governs it. A finding that is factually correct but not this + repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that + sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape + closes the thread on its own evidence. An assertion ("this is fine") does not close a finding, + a decline needs evidence the reviewer itself could check. +3. **Real, fixable here, but deliberately left as is, a value call rather than a scope + boundary, so it is the maintainer's, not the agent's.** Reach for this only once outcome 2 is + ruled out, since a scope boundary declines on its own evidence and never needs this outcome at + all. State the finding and why the fix is unwanted, and get an explicit answer in the same + turn, before moving to other work. A plan to ask later is resolution by silence the moment + attention moves elsewhere. If the maintainer is not reachable right now, leave the thread open + and say so, rather than treating the intention to ask as the asking. +4. **Real and worth doing later, so file the issue first, then reply with its link.** A deferral + noted only in a thread is lost the moment the PR merges. +5. **Keeps recurring, so fix the class, not the instance.** A finding raised repeatedly against + correct code means the code is not communicating something: add the comment, sharpen the name, + narrow the interface, or fix the rule if the rule is wrong. Bouncing the same point across + rounds is the signal to escalate the rule itself, not to keep re-arguing it. + +**A disposition decided on one PR does not carry to the next.** The same finding shape recurring +on a sibling repo or PR, even within one batch or one session, gets its own outcome: its own +evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior +instance's outcome is context for the new one, never a standing answer to reuse in its place. + +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + + ## Mechanics Live Elsewhere diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 99686dfb..fa1c1a86 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -129,11 +129,11 @@ The digest is bound to the read for the same reason `--expect-digest` is above: Each bullet is a rule down to its `Why:` line, which is rationale rather than rule, so a stale rationale is a cleanup rather than a defect. -- **Every finding ends in one of the five outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** +- **Every finding ends in one of the outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** - `Why:` a local finding and a PR-hosted one deserve the same dispositions, and one home for the list is what stops two copies of it drifting apart. - **The agent disposing of a pass's findings classes each one `style`, `introduced`, or `pre-existing`, in that order.** `style` is a preference between defensible forms. `introduced` is any other finding on text this change wrote, rewrote, or removed, on text this change should have written, on a precondition this change left false elsewhere, or load-bearing for a decision this change puts to the maintainer. `pre-existing` is every other finding. - `Why:` the reviewer is asked to omit preferences and returns some anyway, and `style` is classed first so that a preference on text this change wrote is not owed a fix. -- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined under outcome 2 on `code-review`'s own rule to omit preferences. +- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined with evidence, per `pr-review-conduct` "Every finding ends in one of five outcomes", the evidence being `code-review` "Review the Change"'s own rule to omit preferences. - `Why:` a finding count over prose never reaches zero, so a loop closing on "did it find anything" does not close, where one closing on the false claim, the unfollowable instruction, or the wrong behavior this change put there does. - **A push allows two rounds of edits in answer to the passes it owes, one budget across both.** Where an `introduced` finding is still open after the second round, editing stops and what remains goes to the maintainer with its counts per class, per `pr-review-conduct` "Escalate to the maintainer when". - `Why:` past the second round nearly every finding is against text the previous round's fix wrote, so the rounds are producing the defects they find rather than removing them. @@ -142,8 +142,8 @@ Each bullet is a rule down to its `Why:` line, which is rationale rather than ru ## When to Run It -- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). -- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Before the first push toward a pull request, the push that opens it in `drive-pr` "The Drive Loop" and in `pr-review-conduct` "Expected review loop". +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (the fix outcome of `pr-review-conduct` "Every finding ends in one of five outcomes", which `drive-pr` "Disposing of Every Finding" carries). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. - Before pushing a change that edits canonical content other repositories carry, or that newly carries some by widening the manifest, over each unit `check` names, per "The Carried-Content Pass" above. diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index b8b0644f..b6f9f91a 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -129,11 +129,12 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push - went, then reply with the fixing commit SHA. A branch already reviewed once - has not been reviewed for the fix, which is the round this gets dropped on and the churn - `local-strict-review` exists to stop. For a finding on platform-specific code - (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. 2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** @@ -162,6 +163,9 @@ on a sibling repo or PR, even within one batch or one session, gets its own outc evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior instance's outcome is context for the new one, never a standing answer to reuse in its place. +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + ## Triaging findings **A low-confidence (suppressed) finding is not a low-value one.** Judge each against the code, diff --git a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown index 90ee3d3d..e80dfa25 100644 --- a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown +++ b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown @@ -1 +1 @@ -55005855f2e13528 +ab68be335de13bfa diff --git a/.claude-plugin/fleet-skills/.source-digests/drive-pr b/.claude-plugin/fleet-skills/.source-digests/drive-pr index eb9aca11..e2fd732f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/drive-pr +++ b/.claude-plugin/fleet-skills/.source-digests/drive-pr @@ -1 +1 @@ -1a7709724fbde0b5 +cdb7964c85903121 diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index 5508b9ba..aef8c7b7 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -1e35569389d3a880 +d8a070025e2cff17 diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 9dfe0191..d907d02a 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -b72736e1747ad6bc +8aacb5724e76037a diff --git a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md index 47c14be1..ad15165b 100644 --- a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md @@ -58,7 +58,7 @@ it was invoked for is narrower than the rule requires, deliberately. - **The grant is bounded by the session it was named in.** A run interrupted and resumed in a new session needs the skill named again, which costs one sentence and is the difference between a grant and a mode. A grant read back from a note is one nobody gave. -- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's item 5 for +- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's explicit-permission item for this run's feature -> develop merges and nothing else, so a pull request with one open finding still does not merge. - It is never authorization to merge a develop -> main promotion pull request, to dispatch a @@ -210,15 +210,16 @@ Brief on `AGENTS.md` "Context and Delegation Discipline"'s subagent shape. - **The worker drives its group to a develop merge**, by invoking `drive-pr` with the target stated as develop only. That skill owns the review loop, the finding disposition, and the merge, so brief the group and the bounds rather than restating the loop. -- **The worker creates its own worktree**, always, as `drive-pr` step 1 and `repo-worktree`'s - task-start mandate already require of the task itself. No worker inherits another's worktree, +- **The worker creates its own worktree**, always, as `drive-pr`'s worktree isolation and + `repo-worktree`'s task-start mandate already require of the task itself. No worker inherits another's worktree, which is why "Bounding the Wait on a Worker" either removes a dead worker's tree and its branch or leaves that tree untouched for the maintainer, and never passes it on. -- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr` step 4 - and of `repo-worktree`'s post-merge procedure. Say so in the brief, because a worker following - either alone will clean up. The worker still performs step 4's merge itself, and what the override - moves is that step's two cleanup halves, the worktree procedure and the verify-then-delete of the - merged remote branch, **both** rather than only the first. "Cleanup Is the Orchestrator's" below, in this +- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr`'s + post-merge cleanup and of `repo-worktree`'s post-merge procedure. Say so in the brief, because + a worker following either alone will clean up. The worker still performs the merge itself, and + what the override moves is the two cleanup halves `drive-pr` runs after it, the worktree + procedure and the verify-then-delete of the merged remote branch, **both** rather than only the + first. "Cleanup Is the Orchestrator's" below, in this same section, says why and what it covers. - **The worker runs `local-strict-review` before every push**, including one that only fixes a review finding. That pass dispatches a reviewer of its own, so a harness where a subagent cannot @@ -260,8 +261,8 @@ finish the procedure from inside its own worktree, since removing that worktree working directory in which to delete its branch. So the whole procedure moves to the orchestrator, which runs it from the base clone at the round's -cleanup step, while no worker is live in a tree it touches. It carries `drive-pr` step 4's remote -half too, verifying the merged branch's tip against the pull request's `headRefOid` before +cleanup step, while no worker is live in a tree it touches. It carries the remote half of `drive-pr`'s +post-merge cleanup too, verifying the merged branch's tip against the pull request's `headRefOid` before `git push origin --delete`, since taking that step from the worker without naming a new owner would leave a live remote branch behind every group. It covers every group that is done with its tree, which is the finished ones **and the abandoned ones**: a group told to abandon its branch keeps a @@ -398,11 +399,13 @@ has carried. can still owe a promotion pull request, for work an earlier round landed and no promotion has yet carried. A count of zero is the only case with nothing to promote, and the round reports that instead of attempting one. -2. Drive its review loop per `drive-pr` steps 5 through 8, **with a review-round budget set before - the first one**, the same discipline "Bounding a Prose Group" applies to a feature branch. That - loop repeats until the promotion pull request carries no open finding, and nothing in it - terminates on its own, so when the budget is reached, stop and put the state to the maintainer - rather than continuing to spend the run's only forward gear on one pull request. +2. Drive its review loop per the promotion half of `drive-pr` "The Drive Loop", **with a + review-round budget set before the first one**, the same discipline "Bounding a Prose Group" + applies to a feature branch. That loop repeats until the promotion pull request meets every + `pr-review-conduct` Merge Gate item except the maintainer's explicit permission to merge, and + nothing in that loop terminates on its own, so when the budget is reached, stop and put the + state to the maintainer rather than continuing to spend the run's only forward gear on one pull + request. 3. Put the ready pull request to the maintainer through the interface's own prompt mechanism, naming the merge as the action that unblocks the run. The maintainer's merge is the run's clock, so one reported in a closing paragraph and never actually asked about stalls every round behind it. @@ -411,8 +414,8 @@ has carried. it lands as its own feature -> develop pass, and that landing moving its head is expected, since its head **is** develop. **That pass is dispatched as a worker like any other**, which is the one push the freeze permits and the reason the orchestrator still opens no branch of its own. - `drive-pr` step 6 sends the seat driving a promotion pull request back through its own steps 1 - to 4 for such a fix, and here that seat dispatches rather than drives it. + `drive-pr` "The Drive Loop" sends the seat driving a promotion pull request back through its + own feature -> develop pass for such a fix, and here that seat dispatches rather than drives it. 5. **A promotion fix outranks any file claim.** A group holding a file it needs yields, because the promotion pull request is what the whole run is queued behind. A holder that is merely parked yields by handing the file over. A holder that already pushed and has an open pull request @@ -430,7 +433,7 @@ has carried. 6. **Nothing else pushes, and nothing else is dispatched.** The promotion fix of step 4 is the one exception to both, and everything in this step is said of the next round's work rather than of it. That round's preparation is orchestrator work and continues: rank, group, and verify claims. - Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, whose second step + Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, which pushes and opens a pull request, so a next-round worker dispatched under the freeze would either break it or sit in a state that procedure does not describe. None is left running across the wait either, since a worker held idle for an unbounded maintainer wait is one doing nothing at a diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index b234fb29..9ea24f53 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -2,19 +2,19 @@ name: drive-pr description: >- Drives a ptr727/ProjectTemplate fleet pull request through its review loop, feature branch into - develop and, when asked, on to a mergeable develop -> main promotion PR, applying the - pr-review-conduct disposition to every reviewer finding along the way: fix it, decline it with - evidence, defer it behind a filed issue, or put the call to the maintainer and wait for an - explicit answer in the same turn, escalating to whoever dispatched the drive instead where the - maintainer cannot be reached from that seat. Use this whenever asked to drive, land, take, chase, or push + develop and, when asked, on to a mergeable develop -> main promotion PR, disposing of every + reviewer finding along the way under pr-review-conduct's outcomes, carried here whole as a + generated include, and escalating to whoever dispatched the drive where the drive's own seat + cannot reach the maintainer. Use this whenever asked to drive, land, take, chase, or push a PR toward develop or main, or to run the review loop hands off instead of narrating each round. When the request does not say how far ("drive this PR", "land it"), ask once whether the target is develop or a mergeable main promotion PR, rather than guessing. Triggers even when only one PR is named, because a finding raised against the develop -> main promotion PR routinely needs its own feature -> develop fix cycle before the promotion PR can go green, and stopping at the first promotion-PR finding is the early exit this skill exists to prevent. Ends - at develop merged, or at a promotion PR meeting the pr-review-conduct Merge Gate, never merges - main itself, that is the separate merge-and-release skill, its own go-ahead. + at develop merged, or at a promotion PR meeting every pr-review-conduct Merge Gate item except + the maintainer's explicit permission to merge, never merges main itself, that is the separate + merge-and-release skill, its own go-ahead. --- # Drive PR @@ -128,32 +128,57 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. 1 to 4 in its own worktree and branch, then return here. 7. The fix landing on develop updates the promotion PR's diff and head SHA on its own, re-request a review on the new head and continue the loop. -8. Repeat 6 and 7 until the promotion PR itself carries no open finding and its checks are green - on the current head. +8. Repeat 6 and 7 until the promotion PR meets every pr-review-conduct Merge Gate item except the + maintainer's explicit permission to merge. 9. Report the promotion PR number and its ready state. Do not merge it. ## Disposing of Every Finding -pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: - -- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA - (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and - the branch was already reviewed once, and a fix push carries content no pass has read exactly as - the first push did. -- Not real, or real but out of scope here, so decline in the thread with evidence: the command - and its output, the code path, or the rule that governs it. An assertion never closes a finding - on its own (outcome 2). -- Real and worth doing, but later, so file the issue first, then reply with its link (outcome 4). -- Real, fixable, but a value call rather than a scope boundary, or the agent genuinely does not - know which of the above applies, so ask the maintainer directly, whatever the runtime's own - interactive-question mechanism is, and get an explicit answer in the same turn, a plan to ask - later is resolution by silence (outcome 3). A drive that cannot reach the - maintainer directly, a dispatched one being the ordinary case, escalates to whoever dispatched - it and stops that unit of work there instead, per `pr-review-conduct`, which owns what the - receiving seat then does and how far the escalation travels. -- The same finding keeps recurring against correct code, fix the class, sharpen a name, add a - comment, or take the rule itself to the maintainer, rather than re-arguing the instance every - round (outcome 5). +The rule below is a generated include, so a defect in it is fixed in `pr-review-conduct` and +regenerated rather than edited here. A drive that cannot reach the maintainer directly, a +dispatched one being the ordinary case, escalates per `pr-review-conduct` "Escalate to the +maintainer when". + + + +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per + `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent + elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. +2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** + Disprove a wrong finding with the command and its output, the code path that makes it + impossible, or the rule that governs it. A finding that is factually correct but not this + repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that + sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape + closes the thread on its own evidence. An assertion ("this is fine") does not close a finding, + a decline needs evidence the reviewer itself could check. +3. **Real, fixable here, but deliberately left as is, a value call rather than a scope + boundary, so it is the maintainer's, not the agent's.** Reach for this only once outcome 2 is + ruled out, since a scope boundary declines on its own evidence and never needs this outcome at + all. State the finding and why the fix is unwanted, and get an explicit answer in the same + turn, before moving to other work. A plan to ask later is resolution by silence the moment + attention moves elsewhere. If the maintainer is not reachable right now, leave the thread open + and say so, rather than treating the intention to ask as the asking. +4. **Real and worth doing later, so file the issue first, then reply with its link.** A deferral + noted only in a thread is lost the moment the PR merges. +5. **Keeps recurring, so fix the class, not the instance.** A finding raised repeatedly against + correct code means the code is not communicating something: add the comment, sharpen the name, + narrow the interface, or fix the rule if the rule is wrong. Bouncing the same point across + rounds is the signal to escalate the rule itself, not to keep re-arguing it. + +**A disposition decided on one PR does not carry to the next.** The same finding shape recurring +on a sibling repo or PR, even within one batch or one session, gets its own outcome: its own +evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior +instance's outcome is context for the new one, never a standing answer to reuse in its place. + +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + + ## Mechanics Live Elsewhere 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 99686dfb..fa1c1a86 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -129,11 +129,11 @@ The digest is bound to the read for the same reason `--expect-digest` is above: Each bullet is a rule down to its `Why:` line, which is rationale rather than rule, so a stale rationale is a cleanup rather than a defect. -- **Every finding ends in one of the five outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** +- **Every finding ends in one of the outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** - `Why:` a local finding and a PR-hosted one deserve the same dispositions, and one home for the list is what stops two copies of it drifting apart. - **The agent disposing of a pass's findings classes each one `style`, `introduced`, or `pre-existing`, in that order.** `style` is a preference between defensible forms. `introduced` is any other finding on text this change wrote, rewrote, or removed, on text this change should have written, on a precondition this change left false elsewhere, or load-bearing for a decision this change puts to the maintainer. `pre-existing` is every other finding. - `Why:` the reviewer is asked to omit preferences and returns some anyway, and `style` is classed first so that a preference on text this change wrote is not owed a fix. -- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined under outcome 2 on `code-review`'s own rule to omit preferences. +- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined with evidence, per `pr-review-conduct` "Every finding ends in one of five outcomes", the evidence being `code-review` "Review the Change"'s own rule to omit preferences. - `Why:` a finding count over prose never reaches zero, so a loop closing on "did it find anything" does not close, where one closing on the false claim, the unfollowable instruction, or the wrong behavior this change put there does. - **A push allows two rounds of edits in answer to the passes it owes, one budget across both.** Where an `introduced` finding is still open after the second round, editing stops and what remains goes to the maintainer with its counts per class, per `pr-review-conduct` "Escalate to the maintainer when". - `Why:` past the second round nearly every finding is against text the previous round's fix wrote, so the rounds are producing the defects they find rather than removing them. @@ -142,8 +142,8 @@ Each bullet is a rule down to its `Why:` line, which is rationale rather than ru ## When to Run It -- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). -- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Before the first push toward a pull request, the push that opens it in `drive-pr` "The Drive Loop" and in `pr-review-conduct` "Expected review loop". +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (the fix outcome of `pr-review-conduct` "Every finding ends in one of five outcomes", which `drive-pr` "Disposing of Every Finding" carries). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. - Before pushing a change that edits canonical content other repositories carry, or that newly carries some by widening the manifest, over each unit `check` names, per "The Carried-Content Pass" above. 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 b8b0644f..b6f9f91a 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -129,11 +129,12 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push - went, then reply with the fixing commit SHA. A branch already reviewed once - has not been reviewed for the fix, which is the round this gets dropped on and the churn - `local-strict-review` exists to stop. For a finding on platform-specific code - (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. 2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** @@ -162,6 +163,9 @@ on a sibling repo or PR, even within one batch or one session, gets its own outc evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior instance's outcome is context for the new one, never a standing answer to reuse in its place. +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + ## Triaging findings **A low-confidence (suppressed) finding is not a low-value one.** Judge each against the code, diff --git a/.github/skills/backlog-burndown/SKILL.md b/.github/skills/backlog-burndown/SKILL.md index 47c14be1..ad15165b 100644 --- a/.github/skills/backlog-burndown/SKILL.md +++ b/.github/skills/backlog-burndown/SKILL.md @@ -58,7 +58,7 @@ it was invoked for is narrower than the rule requires, deliberately. - **The grant is bounded by the session it was named in.** A run interrupted and resumed in a new session needs the skill named again, which costs one sentence and is the difference between a grant and a mode. A grant read back from a note is one nobody gave. -- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's item 5 for +- The grant does not weaken the `pr-review-conduct` Merge Gate. It answers that gate's explicit-permission item for this run's feature -> develop merges and nothing else, so a pull request with one open finding still does not merge. - It is never authorization to merge a develop -> main promotion pull request, to dispatch a @@ -210,15 +210,16 @@ Brief on `AGENTS.md` "Context and Delegation Discipline"'s subagent shape. - **The worker drives its group to a develop merge**, by invoking `drive-pr` with the target stated as develop only. That skill owns the review loop, the finding disposition, and the merge, so brief the group and the bounds rather than restating the loop. -- **The worker creates its own worktree**, always, as `drive-pr` step 1 and `repo-worktree`'s - task-start mandate already require of the task itself. No worker inherits another's worktree, +- **The worker creates its own worktree**, always, as `drive-pr`'s worktree isolation and + `repo-worktree`'s task-start mandate already require of the task itself. No worker inherits another's worktree, which is why "Bounding the Wait on a Worker" either removes a dead worker's tree and its branch or leaves that tree untouched for the maintainer, and never passes it on. -- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr` step 4 - and of `repo-worktree`'s post-merge procedure. Say so in the brief, because a worker following - either alone will clean up. The worker still performs step 4's merge itself, and what the override - moves is that step's two cleanup halves, the worktree procedure and the verify-then-delete of the - merged remote branch, **both** rather than only the first. "Cleanup Is the Orchestrator's" below, in this +- **The worker does no cleanup**, which is this skill's one stated override of `drive-pr`'s + post-merge cleanup and of `repo-worktree`'s post-merge procedure. Say so in the brief, because + a worker following either alone will clean up. The worker still performs the merge itself, and + what the override moves is the two cleanup halves `drive-pr` runs after it, the worktree + procedure and the verify-then-delete of the merged remote branch, **both** rather than only the + first. "Cleanup Is the Orchestrator's" below, in this same section, says why and what it covers. - **The worker runs `local-strict-review` before every push**, including one that only fixes a review finding. That pass dispatches a reviewer of its own, so a harness where a subagent cannot @@ -260,8 +261,8 @@ finish the procedure from inside its own worktree, since removing that worktree working directory in which to delete its branch. So the whole procedure moves to the orchestrator, which runs it from the base clone at the round's -cleanup step, while no worker is live in a tree it touches. It carries `drive-pr` step 4's remote -half too, verifying the merged branch's tip against the pull request's `headRefOid` before +cleanup step, while no worker is live in a tree it touches. It carries the remote half of `drive-pr`'s +post-merge cleanup too, verifying the merged branch's tip against the pull request's `headRefOid` before `git push origin --delete`, since taking that step from the worker without naming a new owner would leave a live remote branch behind every group. It covers every group that is done with its tree, which is the finished ones **and the abandoned ones**: a group told to abandon its branch keeps a @@ -398,11 +399,13 @@ has carried. can still owe a promotion pull request, for work an earlier round landed and no promotion has yet carried. A count of zero is the only case with nothing to promote, and the round reports that instead of attempting one. -2. Drive its review loop per `drive-pr` steps 5 through 8, **with a review-round budget set before - the first one**, the same discipline "Bounding a Prose Group" applies to a feature branch. That - loop repeats until the promotion pull request carries no open finding, and nothing in it - terminates on its own, so when the budget is reached, stop and put the state to the maintainer - rather than continuing to spend the run's only forward gear on one pull request. +2. Drive its review loop per the promotion half of `drive-pr` "The Drive Loop", **with a + review-round budget set before the first one**, the same discipline "Bounding a Prose Group" + applies to a feature branch. That loop repeats until the promotion pull request meets every + `pr-review-conduct` Merge Gate item except the maintainer's explicit permission to merge, and + nothing in that loop terminates on its own, so when the budget is reached, stop and put the + state to the maintainer rather than continuing to spend the run's only forward gear on one pull + request. 3. Put the ready pull request to the maintainer through the interface's own prompt mechanism, naming the merge as the action that unblocks the run. The maintainer's merge is the run's clock, so one reported in a closing paragraph and never actually asked about stalls every round behind it. @@ -411,8 +414,8 @@ has carried. it lands as its own feature -> develop pass, and that landing moving its head is expected, since its head **is** develop. **That pass is dispatched as a worker like any other**, which is the one push the freeze permits and the reason the orchestrator still opens no branch of its own. - `drive-pr` step 6 sends the seat driving a promotion pull request back through its own steps 1 - to 4 for such a fix, and here that seat dispatches rather than drives it. + `drive-pr` "The Drive Loop" sends the seat driving a promotion pull request back through its + own feature -> develop pass for such a fix, and here that seat dispatches rather than drives it. 5. **A promotion fix outranks any file claim.** A group holding a file it needs yields, because the promotion pull request is what the whole run is queued behind. A holder that is merely parked yields by handing the file over. A holder that already pushed and has an open pull request @@ -430,7 +433,7 @@ has carried. 6. **Nothing else pushes, and nothing else is dispatched.** The promotion fix of step 4 is the one exception to both, and everything in this step is said of the next round's work rather than of it. That round's preparation is orchestrator work and continues: rank, group, and verify claims. - Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, whose second step + Its dispatch waits, because a worker has exactly one procedure, `drive-pr`, which pushes and opens a pull request, so a next-round worker dispatched under the freeze would either break it or sit in a state that procedure does not describe. None is left running across the wait either, since a worker held idle for an unbounded maintainer wait is one doing nothing at a diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index b234fb29..9ea24f53 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -2,19 +2,19 @@ name: drive-pr description: >- Drives a ptr727/ProjectTemplate fleet pull request through its review loop, feature branch into - develop and, when asked, on to a mergeable develop -> main promotion PR, applying the - pr-review-conduct disposition to every reviewer finding along the way: fix it, decline it with - evidence, defer it behind a filed issue, or put the call to the maintainer and wait for an - explicit answer in the same turn, escalating to whoever dispatched the drive instead where the - maintainer cannot be reached from that seat. Use this whenever asked to drive, land, take, chase, or push + develop and, when asked, on to a mergeable develop -> main promotion PR, disposing of every + reviewer finding along the way under pr-review-conduct's outcomes, carried here whole as a + generated include, and escalating to whoever dispatched the drive where the drive's own seat + cannot reach the maintainer. Use this whenever asked to drive, land, take, chase, or push a PR toward develop or main, or to run the review loop hands off instead of narrating each round. When the request does not say how far ("drive this PR", "land it"), ask once whether the target is develop or a mergeable main promotion PR, rather than guessing. Triggers even when only one PR is named, because a finding raised against the develop -> main promotion PR routinely needs its own feature -> develop fix cycle before the promotion PR can go green, and stopping at the first promotion-PR finding is the early exit this skill exists to prevent. Ends - at develop merged, or at a promotion PR meeting the pr-review-conduct Merge Gate, never merges - main itself, that is the separate merge-and-release skill, its own go-ahead. + at develop merged, or at a promotion PR meeting every pr-review-conduct Merge Gate item except + the maintainer's explicit permission to merge, never merges main itself, that is the separate + merge-and-release skill, its own go-ahead. --- # Drive PR @@ -128,32 +128,57 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. 1 to 4 in its own worktree and branch, then return here. 7. The fix landing on develop updates the promotion PR's diff and head SHA on its own, re-request a review on the new head and continue the loop. -8. Repeat 6 and 7 until the promotion PR itself carries no open finding and its checks are green - on the current head. +8. Repeat 6 and 7 until the promotion PR meets every pr-review-conduct Merge Gate item except the + maintainer's explicit permission to merge. 9. Report the promotion PR number and its ready state. Do not merge it. ## Disposing of Every Finding -pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: - -- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA - (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and - the branch was already reviewed once, and a fix push carries content no pass has read exactly as - the first push did. -- Not real, or real but out of scope here, so decline in the thread with evidence: the command - and its output, the code path, or the rule that governs it. An assertion never closes a finding - on its own (outcome 2). -- Real and worth doing, but later, so file the issue first, then reply with its link (outcome 4). -- Real, fixable, but a value call rather than a scope boundary, or the agent genuinely does not - know which of the above applies, so ask the maintainer directly, whatever the runtime's own - interactive-question mechanism is, and get an explicit answer in the same turn, a plan to ask - later is resolution by silence (outcome 3). A drive that cannot reach the - maintainer directly, a dispatched one being the ordinary case, escalates to whoever dispatched - it and stops that unit of work there instead, per `pr-review-conduct`, which owns what the - receiving seat then does and how far the escalation travels. -- The same finding keeps recurring against correct code, fix the class, sharpen a name, add a - comment, or take the rule itself to the maintainer, rather than re-arguing the instance every - round (outcome 5). +The rule below is a generated include, so a defect in it is fixed in `pr-review-conduct` and +regenerated rather than edited here. A drive that cannot reach the maintainer directly, a +dispatched one being the ordinary case, escalates per `pr-review-conduct` "Escalate to the +maintainer when". + + + +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per + `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent + elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. +2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** + Disprove a wrong finding with the command and its output, the code path that makes it + impossible, or the rule that governs it. A finding that is factually correct but not this + repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that + sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape + closes the thread on its own evidence. An assertion ("this is fine") does not close a finding, + a decline needs evidence the reviewer itself could check. +3. **Real, fixable here, but deliberately left as is, a value call rather than a scope + boundary, so it is the maintainer's, not the agent's.** Reach for this only once outcome 2 is + ruled out, since a scope boundary declines on its own evidence and never needs this outcome at + all. State the finding and why the fix is unwanted, and get an explicit answer in the same + turn, before moving to other work. A plan to ask later is resolution by silence the moment + attention moves elsewhere. If the maintainer is not reachable right now, leave the thread open + and say so, rather than treating the intention to ask as the asking. +4. **Real and worth doing later, so file the issue first, then reply with its link.** A deferral + noted only in a thread is lost the moment the PR merges. +5. **Keeps recurring, so fix the class, not the instance.** A finding raised repeatedly against + correct code means the code is not communicating something: add the comment, sharpen the name, + narrow the interface, or fix the rule if the rule is wrong. Bouncing the same point across + rounds is the signal to escalate the rule itself, not to keep re-arguing it. + +**A disposition decided on one PR does not carry to the next.** The same finding shape recurring +on a sibling repo or PR, even within one batch or one session, gets its own outcome: its own +evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior +instance's outcome is context for the new one, never a standing answer to reuse in its place. + +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + + ## Mechanics Live Elsewhere diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 99686dfb..fa1c1a86 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -129,11 +129,11 @@ The digest is bound to the read for the same reason `--expect-digest` is above: Each bullet is a rule down to its `Why:` line, which is rationale rather than rule, so a stale rationale is a cleanup rather than a defect. -- **Every finding ends in one of the five outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** +- **Every finding ends in one of the outcomes that `pr-review-conduct` "Every finding ends in one of five outcomes" enumerates, reached here with no thread to reply in.** - `Why:` a local finding and a PR-hosted one deserve the same dispositions, and one home for the list is what stops two copies of it drifting apart. - **The agent disposing of a pass's findings classes each one `style`, `introduced`, or `pre-existing`, in that order.** `style` is a preference between defensible forms. `introduced` is any other finding on text this change wrote, rewrote, or removed, on text this change should have written, on a precondition this change left false elsewhere, or load-bearing for a decision this change puts to the maintainer. `pre-existing` is every other finding. - `Why:` the reviewer is asked to omit preferences and returns some anyway, and `style` is classed first so that a preference on text this change wrote is not owed a fix. -- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined under outcome 2 on `code-review`'s own rule to omit preferences. +- **Another round is owed only while an `introduced` finding is open.** Unless evidence disproves it, an `introduced` finding is fixed within the budget below, or escalated where `pr-review-conduct` "Escalate to the maintainer when" says so, a `pre-existing` one is filed once and blocks nothing, and a `style` one is declined with evidence, per `pr-review-conduct` "Every finding ends in one of five outcomes", the evidence being `code-review` "Review the Change"'s own rule to omit preferences. - `Why:` a finding count over prose never reaches zero, so a loop closing on "did it find anything" does not close, where one closing on the false claim, the unfollowable instruction, or the wrong behavior this change put there does. - **A push allows two rounds of edits in answer to the passes it owes, one budget across both.** Where an `introduced` finding is still open after the second round, editing stops and what remains goes to the maintainer with its counts per class, per `pr-review-conduct` "Escalate to the maintainer when". - `Why:` past the second round nearly every finding is against text the previous round's fix wrote, so the rounds are producing the defects they find rather than removing them. @@ -142,8 +142,8 @@ Each bullet is a rule down to its `Why:` line, which is rationale rather than ru ## When to Run It -- Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). -- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). +- Before the first push toward a pull request, the push that opens it in `drive-pr` "The Drive Loop" and in `pr-review-conduct` "Expected review loop". +- Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (the fix outcome of `pr-review-conduct` "Every finding ends in one of five outcomes", which `drive-pr` "Disposing of Every Finding" carries). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. - Before pushing a change that edits canonical content other repositories carry, or that newly carries some by widening the manifest, over each unit `check` names, per "The Carried-Content Pass" above. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index b8b0644f..b6f9f91a 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -129,11 +129,12 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push - went, then reply with the fixing commit SHA. A branch already reviewed once - has not been reviewed for the fix, which is the round this gets dropped on and the churn - `local-strict-review` exists to stop. For a finding on platform-specific code - (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way the push that + opened the pull request went, per `pr-review-conduct` "Expected review loop", then reply with + the fixing commit SHA. A branch already reviewed once has not been reviewed for the fix, which + is the round the `local-strict-review` pass gets dropped on and the churn `local-strict-review` + exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only + path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. 2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** @@ -162,6 +163,9 @@ on a sibling repo or PR, even within one batch or one session, gets its own outc evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior instance's outcome is context for the new one, never a standing answer to reuse in its place. +`pr-review-conduct` "Every finding ends in one of five outcomes" keeps the full rule, and the +`drive-pr` Skill carries it whole as a generated include, applying it while driving. + ## Triaging findings **A low-confidence (suppressed) finding is not a low-value one.** Judge each against the code, diff --git a/reports/canonical-review.json b/reports/canonical-review.json index 4a0462c9..30fb7877 100644 --- a/reports/canonical-review.json +++ b/reports/canonical-review.json @@ -75,11 +75,11 @@ }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > Dispatching a Worker", - "digest": "sha256:bd825f92555e4d0a762afe4a2de5ac6a9873b14eed1221a4c2f87d9a70f71489", + "digest": "sha256:26cd85e54bda73b3395a2fb4ab3df1441585712134353d1e83b033c3e70a205d", "reviewer": "agent-skill", - "findings": 3, - "hubCommit": "16f2e32a037857dfcd68b114f006ca2d0cf12ada", - "stamp": "2026-09-04T02:41:40Z" + "findings": 4, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:48Z" }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > Ending the Run", @@ -139,11 +139,11 @@ }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > The Promotion Boundary", - "digest": "sha256:4b802ff18e55a38f6027b443bebe63c0721e84bf0234b7262aaad726dc005f77", + "digest": "sha256:7819aeb3ed274bce5e619b67491aaca69d2967abb45faf463b0353d52fbcdd64", "reviewer": "agent-skill", "findings": 8, - "hubCommit": "16f2e32a037857dfcd68b114f006ca2d0cf12ada", - "stamp": "2026-09-04T00:33:58Z" + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:55:09Z" }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > The Round", @@ -163,11 +163,11 @@ }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > What Invoking This Skill Authorizes", - "digest": "sha256:00d0aff29a7a73bf5575aa0753aac923fdc1364c17968d8e87c96f3658e3ef77", + "digest": "sha256:b6e8b635c80e201590f8babbebd29bf9ef63c931466241e6a599a29ce6dd7c30", "reviewer": "agent-skill", - "findings": 0, - "hubCommit": "136661da1b978f793fd5bbd7daa1d95b7338a919", - "stamp": "2026-09-03T19:44:52Z" + "findings": 3, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:49Z" }, { "unit": ".agents/skills/backlog-burndown/SKILL.md > Why This Exists", @@ -211,19 +211,19 @@ }, { "unit": ".agents/skills/drive-pr/SKILL.md > (preamble)", - "digest": "sha256:5c6af4cd25e9aeaafbfe460691d87bbddf80b4c164f29791fe42df931749e5bb", + "digest": "sha256:71a5535d07debc9fa5cd679296a83c096b9eef5280aa2b3fc8ae707b86c67335", "reviewer": "agent-skill", - "findings": 4, - "hubCommit": "16f2e32a037857dfcd68b114f006ca2d0cf12ada", - "stamp": "2026-09-04T00:58:37Z" + "findings": 0, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:49Z" }, { "unit": ".agents/skills/drive-pr/SKILL.md > Disposing of Every Finding", - "digest": "sha256:ae3a4a85828fbe73e68906cecfab20bf0c801c22482126c3e16f530aa8b71737", + "digest": "sha256:883689d8b9529430556667c1eba4d2d9aeba2760982697d999244a486a252a73", "reviewer": "agent-skill", - "findings": 4, - "hubCommit": "16f2e32a037857dfcd68b114f006ca2d0cf12ada", - "stamp": "2026-09-04T00:58:38Z" + "findings": 2, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:30:52Z" }, { "unit": ".agents/skills/drive-pr/SKILL.md > How Far to Drive", @@ -235,11 +235,11 @@ }, { "unit": ".agents/skills/drive-pr/SKILL.md > The Drive Loop", - "digest": "sha256:f05bc847db251e692cf5e3a4e9aecd74448705aa7b9f478efbdb6908721c99be", + "digest": "sha256:d1a39100cc4e8a03f6386e763f734a1b8298897be960074f51d01566a143192c", "reviewer": "agent-skill", - "findings": 0, - "hubCommit": "dded739ff0ae8614b52b187a95b00460c0fad8ce", - "stamp": "2026-09-01T17:57:39Z" + "findings": 7, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:49Z" }, { "unit": ".agents/skills/drive-pr/SKILL.md > What Invoking This Skill Authorizes", @@ -259,11 +259,11 @@ }, { "unit": ".agents/skills/local-strict-review/SKILL.md > Disposing of Findings", - "digest": "sha256:5da8b49aff872a406122fb74abe744e92f7a4f0d23255bb9902d77b49b8bd12c", + "digest": "sha256:04502d7245f620e87df6a5d28e7ee6e4b92dd748c1dc86b9a884f0310b697d62", "reviewer": "agent-skill", - "findings": 3, - "hubCommit": "28872b4ab08d10d48e8dc0eecd94cdf257633e7d", - "stamp": "2026-09-04T23:42:40Z" + "findings": 2, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:49Z" }, { "unit": ".agents/skills/local-strict-review/SKILL.md > Mechanics Live Elsewhere", @@ -307,11 +307,11 @@ }, { "unit": ".agents/skills/local-strict-review/SKILL.md > When to Run It", - "digest": "sha256:2de88c5663772d0b2c255fa44a567d6ab04d3b50c67f038af194e910650c83ab", + "digest": "sha256:b0c7bff2d7fc345ab99ca9cbf33208fcb3afdd8d985418995f1d3186d4df3f4c", "reviewer": "agent-skill", - "findings": 0, - "hubCommit": "44ae00bd432e1eeaafeb00208d8e713a22f49076", - "stamp": "2026-09-01T18:51:57Z" + "findings": 3, + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:11:49Z" }, { "unit": ".agents/skills/operational-vs-release-workflow/SKILL.md > (preamble)", @@ -371,11 +371,11 @@ }, { "unit": ".agents/skills/pr-review-conduct/SKILL.md > Every finding ends in one of five outcomes", - "digest": "sha256:f7292e072c812adc3d9f3d7302181978eaf5c313c6c60efd6c7deece6578b1da", + "digest": "sha256:26a9201900a60d33f5b89ed5a7b274d430b62dad1136a0b80ef45af4dfc8c9f6", "reviewer": "agent-skill", "findings": 4, - "hubCommit": "0e6aacf45334149f6158aa23b8132d6aee1d1eb0", - "stamp": "2026-09-01T16:06:00Z" + "hubCommit": "fbf1f6d9dc713d6e187d2c3d6542490a2839fe55", + "stamp": "2026-09-05T16:30:52Z" }, { "unit": ".agents/skills/pr-review-conduct/SKILL.md > Expected review loop",