From b514879d5d7c6a401bfeaa6ae83e676bc93c3c9b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:13:58 -0700 Subject: [PATCH 1/6] Package PR review conduct and copilot-instructions upkeep as Skills Phase 2 of the hub-docs-to-Skills conversion (see PR #676 for Phase 0/1). Adds two Claude Code / opencode / Codex Skills to .agents/skills/: - pr-review-conduct: the merge gate, expected review loop, the five finding-outcomes, triage, and suppressed-finding answering rules, extracted from GOVERNANCE.md "PR Review Etiquette". - copilot-instructions-keeper: new content for keeping a downstream repo's .github/copilot-instructions.md in sync with the hub without losing that repo's own "Disproved Claims" ledger entries, which are repo-local proofs the file's own rules say are never carried. GOVERNANCE.md "PR Review Etiquette" shrinks to a two-paragraph pointer at the skill, per this project's standing decision that a skill becomes the sole canonical content for its topic once one exists. .github/copilot-instructions.md is untouched, since the Copilot bot reads it directly and cannot read a Skill. AGENTS.md "Where the Rules Live" updated to route to both skills. .claude-plugin/fleet-skills/ regenerated via build_dist.py. --- .../copilot-instructions-keeper/SKILL.md | 94 +++++++++++++ .agents/skills/pr-review-conduct/SKILL.md | 126 ++++++++++++++++++ .../fleet-skills/.claude-plugin/plugin.json | 4 +- .claude-plugin/fleet-skills/.source-digest | 2 +- .../copilot-instructions-keeper/SKILL.md | 94 +++++++++++++ .../skills/pr-review-conduct/SKILL.md | 126 ++++++++++++++++++ AGENTS.md | 4 +- GOVERNANCE.md | 79 +---------- 8 files changed, 448 insertions(+), 81 deletions(-) create mode 100644 .agents/skills/copilot-instructions-keeper/SKILL.md create mode 100644 .agents/skills/pr-review-conduct/SKILL.md create mode 100644 .claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md create mode 100644 .claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md diff --git a/.agents/skills/copilot-instructions-keeper/SKILL.md b/.agents/skills/copilot-instructions-keeper/SKILL.md new file mode 100644 index 00000000..964628c9 --- /dev/null +++ b/.agents/skills/copilot-instructions-keeper/SKILL.md @@ -0,0 +1,94 @@ +--- +name: copilot-instructions-keeper +description: >- + Helps keep a repo's .github/copilot-instructions.md in sync with the ptr727/ProjectTemplate hub + canonical, and stops the one mistake specific to this file: silently wiping its repo-local + "Disproved Claims" ledger entries during a resync. Use this whenever about to edit, overwrite, + re-vendor, or carry .github/copilot-instructions.md into a repo, whenever checking a repo for + drift against the hub or running a conformance sweep that touches this file, whenever GitHub + Copilot's review mechanics in this file look stale, wrong, or missing something the fleet + runbook should cover, or whenever standing up a new repo and carrying this file for the first + time. Also triggers on "why isn't the audit catching that this file is out of date," since the + fleet's mechanical audit checks this file for presence only, at intent fidelity, never for + content drift, so nothing else notices staleness here except a live check like this one. +--- + +# Copilot Instructions Keeper + +## Why this exists + +`.github/copilot-instructions.md` is read directly by the GitHub Copilot review bot, which +cannot read a `SKILL.md`. Unlike other carried governance content, its rule text is never +pointer-ized into a skill, this file always stays fully intact in every repo that carries it. +This skill is about maintaining that file's carried copy, not a substitute for it. + +`spec/files.json` declares it `intent` fidelity, `whole: true`, covering three named sections +(`Commit Messages and Pull Request Titles`, `Reviewing Carried Fleet Content`, `GitHub Copilot +Review Runbook`), with ``, ``, and `` placeholders filled per repo. **`intent` +fidelity is checked by the fleet audit for presence only, never content** (`RESYNC.md`), so a hub +revision to this file produces no finding anywhere in the mechanical audit. If this file is +stale, nothing else in the fleet's tooling will say so, noticing has to happen in a live session +like this one. + +## The one thing this file has that others don't: repo-local ledger entries + +The file's own "Disproved Claims" section states its rule plainly. **The section's shape and +governing rules are carried, but its entries are not.** Each entry records a finding that was +raised against this specific repository and disproved against this repository's code at a named +revision. A repository carrying a copy of this file carries the shape and rules, deletes any +entry whose subject it does not hold, and records what it has proved for itself. + +This means a blind re-vendor of the hub's canonical `copilot-instructions.md` over a downstream +repo's copy is wrong in both directions: + +- Copying the hub's own "Disproved Claims" entries (about `ProjectTemplate` itself) into a + downstream repo attaches proofs about code that repo does not carry. +- Overwriting a downstream repo's copy wholesale deletes any entries that repo itself has earned, + a live disproof, run against that repo's own tree, thrown away with no record. + +**Before touching this file in any repo other than the hub itself:** + +1. Read the current "Disproved Claims" section in that repo's copy, if it has one, and preserve + every entry that names a file or behavior that repo actually carries. +2. Update everything else, the runbook mechanics, the three named sections, the rule text, to + match the hub canonical. +3. Never carry the hub's own repo-specific "Disproved Claims" entries downstream. They name + `ProjectTemplate`'s own files and revisions, not the target repo's. +4. If in doubt whether an entry is still valid for the current tree, treat it per the guard skill + below rather than guessing. + +This is the same failure class [`carried-instruction-file-guard`](../carried-instruction-file-guard/SKILL.md) +exists to stop for `AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, and `WORKFLOW.md`: a +routine-sounding overwrite silently deleting content that is not a stale copy of the hub. Run +that skill's distinctive-phrase probe against this file too before any full-file replace. It is +not in that skill's own file list because its failure mode, ledger entries rather than fleet +rules, is specific enough to warrant its own skill, but the underlying discipline, probe before +overwrite, give a local addition a destination rather than deleting it, is the same. + +## Checking a repo's copy for drift + +1. Fetch the hub (`github.com/ptr727/ProjectTemplate`) `main` branch fresh. A stale local clone + answers confidently instead of failing. +2. Compare the target repo's `.github/copilot-instructions.md` against the hub's, section by + section, at **intent** fidelity, judged by meaning, not by byte match. A content-identical + file with different ``/`` placeholder fills is current, not drifted. +3. Read the "Disproved Claims" section separately from the rest. Judge its **shape and rules** + against the hub, and judge its **entries** only against what that repo itself carries (see + above), never against the hub's own entries. +4. Report what is actually stale (a runbook mechanic that changed, a rule that moved, a new + section) versus what only looks different because it is correctly repo-specific. + +## Carrying it fresh, new repo or full resync + +Follow `RESYNC.md`'s general apply order for carried files, with the ledger rule above applied at +the point this file is touched: carry the hub's current rule text and runbook mechanics, keep the +target repo's own "Disproved Claims" entries (if any existed pre-resync) rather than replacing +them with the hub's, and start a new repo's ledger empty rather than seeded from the hub's own +proofs. + +## What this skill does not cover + +Content-style rules for other carried files (`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, +`WORKFLOW.md`) are `carried-instruction-file-guard`'s job. The review-loop contract this file's +runbook implements, the merge gate, triage, escalation, is `pr-review-conduct`'s job. This skill +is narrowly about keeping this one file's carried copy correct. diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md new file mode 100644 index 00000000..08a73905 --- /dev/null +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -0,0 +1,126 @@ +--- +name: pr-review-conduct +description: >- + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate + fleet repo: requesting a review after a push, triaging findings (including suppressed + low-confidence ones), replying and resolving threads, and deciding whether a PR is actually + mergeable. Use this whenever about to merge a PR, enable auto-merge, ask the maintainer for + merge permission, push a fix and move on without re-checking review state, or judge a PR + "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds + routine, such as "merge this" or "it's all green, go ahead," because mergeStateStatus: CLEAN + reflects only required status checks and never open review findings, so a clean-looking PR can + still carry an unresolved or suppressed finding. Also triggers when a review loop looks stuck + (no review landing, findings that keep reappearing) or when deciding a finding is real, false, + deferred, or a deliberate decline. Provider-specific mechanics (GitHub Copilot's request/poll/ + reply API calls) live in .github/copilot-instructions.md's "GitHub Copilot Review Runbook," + this skill is the contract that runbook implements, not a replacement for it. +--- + +# PR Review Conduct + +## Why this exists + +`mergeStateStatus: CLEAN` reports only required status checks. It says nothing about an open +review thread, a suppressed low-confidence finding sitting uncollapsed in a review body, or a +review that covered the head SHA but read only part of the diff. A PR that looks done, green +checks, no visible comments, routinely still carries a finding nobody has answered. Treating +"green" as "mergeable" is the single most common way this loop gets skipped. + +## Merge Gate, check this before merging or enabling auto-merge + +**Do not merge, and do not enable auto-merge, unless ALL of these hold:** + +1. Required status checks are green, and where they are not, the reason is **read**, never + inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved + thread, and a missing approval alike, and the response differs by cause. +2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed + from a green merge-state. A push makes checks go green *before* the re-review lands, and the + matched review is **read**, not just counted. A review can carry the head SHA and still decline + the PR outright, or say it read only part of the changed files. +3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which + have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed + in the review body investigated and answered. Those appear in no thread, so polling threads + alone reports a clean pass while they stand. +4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved + section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own. + File an issue naming it and quoting the body, rather than guessing what the new wording + probably meant. +5. The maintainer has given **explicit** permission to merge. + +The agent never merges on its own. A green or CLEAN PR with one open finding is not mergeable, +full stop, whatever the merge-state field says. + +## Expected review loop + +1. Push changes to the PR branch. +2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it + explicitly (mechanics in the Copilot runbook). The UI is a fallback only. +3. Wait for review activity on that head. A completed review raising **no findings** is a valid + terminal outcome, so don't 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 just repeats the decline. +4. Triage findings (see below). +5. Apply fixes or write a rationale for declines. +6. Reply to each thread and resolve what was addressed. +7. Re-run the loop after every fix push until no actionable finding remains. + +Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, +then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping +condition, and neither is patience running out. + +## Every finding ends in one of five outcomes + +1. **Real, so fix it.** Reply with the fixing commit SHA. +2. **Not real, so disprove it in the thread**, with the command and its output, the code path + that makes it impossible, or the rule that governs it. An assertion ("this is fine") does not + close a finding, a decline needs evidence the reviewer itself could check. +3. **Real, but deliberately not fixed, the maintainer's call, not the agent's.** State the + finding and why the fix is unwanted, and get an explicit answer. Never resolve this one by + silence. +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. + +## Triaging findings + +**A low-confidence (suppressed) finding is not a low-value one.** Judge each against the code, +never against its confidence label. Classify before responding: + +- **Bug**, wrong behavior, missing coverage, a real code or doc divergence. Fix it. +- **Style or convention**. If the cited rule matches the existing tree, fix the code. If the rule + contradicts the tree or industry norm, **fix the rule, not the code**, and take it to the + maintainer (outcome 5) rather than bouncing the same code across rounds. +- **Architectural opinion**, a proposed redesign. Surface it with a recommendation, never apply + it unilaterally. + +## Answering a suppressed finding + +A suppressed finding has no thread and no resolved or unresolved state, so an answer needs to +carry its own context: quote the finding (with its `file:line` anchor and enough of the +reviewer's own words to identify it), give one bold verdict per finding (`Fixed in `, +`Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be +checked against findings, and link the review round. **Read every round, not only the head.** A +suppressed finding does not retire when a later push supersedes it, it just stops showing up in a +head-scoped query while still unanswered. + +## Escalate to the maintainer when + +- A genuine design trade-off surfaces (fail-open vs. fail-closed, refactor scope). +- A finding keeps recurring. Bring the pattern and a recommended fix (rule change or code + change), don't keep silently re-declining it. +- A finding is judged real but should not be fixed. That decision is never the agent's alone. +- An architectural redesign is proposed rather than a bug fix. + +## Mechanics live elsewhere + +This skill is the provider-agnostic contract. For the actual GitHub API calls, requesting a +Copilot review, polling for it, matching the suppressed-findings heading (its wording has moved +more than once), verifying head-SHA and diff coverage, replying and resolving threads without a +hand-typed id, see `.github/copilot-instructions.md` "GitHub Copilot Review Runbook" in the repo +being worked in, and reach for `scripts/pr_review.py` (hub-hosted, invoked from a hub checkout) +before hand-rolling any of it. `status` reports coverage, threads, suppressed findings, and shapes +in one call, `wait` backs off in-process, and `reply` resolves a thread by matching the finding's +own words rather than a line number a fix push can move. diff --git a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json index edc0fe12..de44ba5f 100644 --- a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json +++ b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json @@ -6,6 +6,8 @@ "name": "ptr727" }, "skills": [ - "./skills/carried-instruction-file-guard" + "./skills/carried-instruction-file-guard", + "./skills/copilot-instructions-keeper", + "./skills/pr-review-conduct" ] } diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 66d4887c..16a2cb8b 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -948c01300ae03ac7 +7dbd5d23c3fd2a8d diff --git a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md new file mode 100644 index 00000000..964628c9 --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md @@ -0,0 +1,94 @@ +--- +name: copilot-instructions-keeper +description: >- + Helps keep a repo's .github/copilot-instructions.md in sync with the ptr727/ProjectTemplate hub + canonical, and stops the one mistake specific to this file: silently wiping its repo-local + "Disproved Claims" ledger entries during a resync. Use this whenever about to edit, overwrite, + re-vendor, or carry .github/copilot-instructions.md into a repo, whenever checking a repo for + drift against the hub or running a conformance sweep that touches this file, whenever GitHub + Copilot's review mechanics in this file look stale, wrong, or missing something the fleet + runbook should cover, or whenever standing up a new repo and carrying this file for the first + time. Also triggers on "why isn't the audit catching that this file is out of date," since the + fleet's mechanical audit checks this file for presence only, at intent fidelity, never for + content drift, so nothing else notices staleness here except a live check like this one. +--- + +# Copilot Instructions Keeper + +## Why this exists + +`.github/copilot-instructions.md` is read directly by the GitHub Copilot review bot, which +cannot read a `SKILL.md`. Unlike other carried governance content, its rule text is never +pointer-ized into a skill, this file always stays fully intact in every repo that carries it. +This skill is about maintaining that file's carried copy, not a substitute for it. + +`spec/files.json` declares it `intent` fidelity, `whole: true`, covering three named sections +(`Commit Messages and Pull Request Titles`, `Reviewing Carried Fleet Content`, `GitHub Copilot +Review Runbook`), with ``, ``, and `` placeholders filled per repo. **`intent` +fidelity is checked by the fleet audit for presence only, never content** (`RESYNC.md`), so a hub +revision to this file produces no finding anywhere in the mechanical audit. If this file is +stale, nothing else in the fleet's tooling will say so, noticing has to happen in a live session +like this one. + +## The one thing this file has that others don't: repo-local ledger entries + +The file's own "Disproved Claims" section states its rule plainly. **The section's shape and +governing rules are carried, but its entries are not.** Each entry records a finding that was +raised against this specific repository and disproved against this repository's code at a named +revision. A repository carrying a copy of this file carries the shape and rules, deletes any +entry whose subject it does not hold, and records what it has proved for itself. + +This means a blind re-vendor of the hub's canonical `copilot-instructions.md` over a downstream +repo's copy is wrong in both directions: + +- Copying the hub's own "Disproved Claims" entries (about `ProjectTemplate` itself) into a + downstream repo attaches proofs about code that repo does not carry. +- Overwriting a downstream repo's copy wholesale deletes any entries that repo itself has earned, + a live disproof, run against that repo's own tree, thrown away with no record. + +**Before touching this file in any repo other than the hub itself:** + +1. Read the current "Disproved Claims" section in that repo's copy, if it has one, and preserve + every entry that names a file or behavior that repo actually carries. +2. Update everything else, the runbook mechanics, the three named sections, the rule text, to + match the hub canonical. +3. Never carry the hub's own repo-specific "Disproved Claims" entries downstream. They name + `ProjectTemplate`'s own files and revisions, not the target repo's. +4. If in doubt whether an entry is still valid for the current tree, treat it per the guard skill + below rather than guessing. + +This is the same failure class [`carried-instruction-file-guard`](../carried-instruction-file-guard/SKILL.md) +exists to stop for `AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, and `WORKFLOW.md`: a +routine-sounding overwrite silently deleting content that is not a stale copy of the hub. Run +that skill's distinctive-phrase probe against this file too before any full-file replace. It is +not in that skill's own file list because its failure mode, ledger entries rather than fleet +rules, is specific enough to warrant its own skill, but the underlying discipline, probe before +overwrite, give a local addition a destination rather than deleting it, is the same. + +## Checking a repo's copy for drift + +1. Fetch the hub (`github.com/ptr727/ProjectTemplate`) `main` branch fresh. A stale local clone + answers confidently instead of failing. +2. Compare the target repo's `.github/copilot-instructions.md` against the hub's, section by + section, at **intent** fidelity, judged by meaning, not by byte match. A content-identical + file with different ``/`` placeholder fills is current, not drifted. +3. Read the "Disproved Claims" section separately from the rest. Judge its **shape and rules** + against the hub, and judge its **entries** only against what that repo itself carries (see + above), never against the hub's own entries. +4. Report what is actually stale (a runbook mechanic that changed, a rule that moved, a new + section) versus what only looks different because it is correctly repo-specific. + +## Carrying it fresh, new repo or full resync + +Follow `RESYNC.md`'s general apply order for carried files, with the ledger rule above applied at +the point this file is touched: carry the hub's current rule text and runbook mechanics, keep the +target repo's own "Disproved Claims" entries (if any existed pre-resync) rather than replacing +them with the hub's, and start a new repo's ledger empty rather than seeded from the hub's own +proofs. + +## What this skill does not cover + +Content-style rules for other carried files (`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, +`WORKFLOW.md`) are `carried-instruction-file-guard`'s job. The review-loop contract this file's +runbook implements, the merge gate, triage, escalation, is `pr-review-conduct`'s job. This skill +is narrowly about keeping this one file's carried copy correct. diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md new file mode 100644 index 00000000..08a73905 --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -0,0 +1,126 @@ +--- +name: pr-review-conduct +description: >- + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate + fleet repo: requesting a review after a push, triaging findings (including suppressed + low-confidence ones), replying and resolving threads, and deciding whether a PR is actually + mergeable. Use this whenever about to merge a PR, enable auto-merge, ask the maintainer for + merge permission, push a fix and move on without re-checking review state, or judge a PR + "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds + routine, such as "merge this" or "it's all green, go ahead," because mergeStateStatus: CLEAN + reflects only required status checks and never open review findings, so a clean-looking PR can + still carry an unresolved or suppressed finding. Also triggers when a review loop looks stuck + (no review landing, findings that keep reappearing) or when deciding a finding is real, false, + deferred, or a deliberate decline. Provider-specific mechanics (GitHub Copilot's request/poll/ + reply API calls) live in .github/copilot-instructions.md's "GitHub Copilot Review Runbook," + this skill is the contract that runbook implements, not a replacement for it. +--- + +# PR Review Conduct + +## Why this exists + +`mergeStateStatus: CLEAN` reports only required status checks. It says nothing about an open +review thread, a suppressed low-confidence finding sitting uncollapsed in a review body, or a +review that covered the head SHA but read only part of the diff. A PR that looks done, green +checks, no visible comments, routinely still carries a finding nobody has answered. Treating +"green" as "mergeable" is the single most common way this loop gets skipped. + +## Merge Gate, check this before merging or enabling auto-merge + +**Do not merge, and do not enable auto-merge, unless ALL of these hold:** + +1. Required status checks are green, and where they are not, the reason is **read**, never + inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved + thread, and a missing approval alike, and the response differs by cause. +2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed + from a green merge-state. A push makes checks go green *before* the re-review lands, and the + matched review is **read**, not just counted. A review can carry the head SHA and still decline + the PR outright, or say it read only part of the changed files. +3. **Every** finding on that head SHA is closed: threads resolved, issue-level comments (which + have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed + in the review body investigated and answered. Those appear in no thread, so polling threads + alone reports a clean pass while they stand. +4. Nothing in the review was a shape the tooling could not read (an unrecognized heading, a moved + section, an unfamiliar coverage wording). An unrecognized shape blocks the gate on its own. + File an issue naming it and quoting the body, rather than guessing what the new wording + probably meant. +5. The maintainer has given **explicit** permission to merge. + +The agent never merges on its own. A green or CLEAN PR with one open finding is not mergeable, +full stop, whatever the merge-state field says. + +## Expected review loop + +1. Push changes to the PR branch. +2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it + explicitly (mechanics in the Copilot runbook). The UI is a fallback only. +3. Wait for review activity on that head. A completed review raising **no findings** is a valid + terminal outcome, so don't 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 just repeats the decline. +4. Triage findings (see below). +5. Apply fixes or write a rationale for declines. +6. Reply to each thread and resolve what was addressed. +7. Re-run the loop after every fix push until no actionable finding remains. + +Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, +then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping +condition, and neither is patience running out. + +## Every finding ends in one of five outcomes + +1. **Real, so fix it.** Reply with the fixing commit SHA. +2. **Not real, so disprove it in the thread**, with the command and its output, the code path + that makes it impossible, or the rule that governs it. An assertion ("this is fine") does not + close a finding, a decline needs evidence the reviewer itself could check. +3. **Real, but deliberately not fixed, the maintainer's call, not the agent's.** State the + finding and why the fix is unwanted, and get an explicit answer. Never resolve this one by + silence. +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. + +## Triaging findings + +**A low-confidence (suppressed) finding is not a low-value one.** Judge each against the code, +never against its confidence label. Classify before responding: + +- **Bug**, wrong behavior, missing coverage, a real code or doc divergence. Fix it. +- **Style or convention**. If the cited rule matches the existing tree, fix the code. If the rule + contradicts the tree or industry norm, **fix the rule, not the code**, and take it to the + maintainer (outcome 5) rather than bouncing the same code across rounds. +- **Architectural opinion**, a proposed redesign. Surface it with a recommendation, never apply + it unilaterally. + +## Answering a suppressed finding + +A suppressed finding has no thread and no resolved or unresolved state, so an answer needs to +carry its own context: quote the finding (with its `file:line` anchor and enough of the +reviewer's own words to identify it), give one bold verdict per finding (`Fixed in `, +`Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be +checked against findings, and link the review round. **Read every round, not only the head.** A +suppressed finding does not retire when a later push supersedes it, it just stops showing up in a +head-scoped query while still unanswered. + +## Escalate to the maintainer when + +- A genuine design trade-off surfaces (fail-open vs. fail-closed, refactor scope). +- A finding keeps recurring. Bring the pattern and a recommended fix (rule change or code + change), don't keep silently re-declining it. +- A finding is judged real but should not be fixed. That decision is never the agent's alone. +- An architectural redesign is proposed rather than a bug fix. + +## Mechanics live elsewhere + +This skill is the provider-agnostic contract. For the actual GitHub API calls, requesting a +Copilot review, polling for it, matching the suppressed-findings heading (its wording has moved +more than once), verifying head-SHA and diff coverage, replying and resolving threads without a +hand-typed id, see `.github/copilot-instructions.md` "GitHub Copilot Review Runbook" in the repo +being worked in, and reach for `scripts/pr_review.py` (hub-hosted, invoked from a hub checkout) +before hand-rolling any of it. `status` reports coverage, threads, suppressed findings, and shapes +in one call, `wait` backs off in-process, and `reply` resolves a thread by matching the finding's +own words rather than a line number a fix push can move. diff --git a/AGENTS.md b/AGENTS.md index f9acb0d2..171b5786 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,7 +79,7 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions` | | Any prose, comment, doc, or line-ending change | `Documentation Style Conventions` | | Proving work actually happened | `Verification Discipline` | -| Requesting, answering, or closing a review | `PR Review Etiquette` | +| Requesting, answering, or closing a review | `PR Review Etiquette`, packaged as the `pr-review-conduct` Skill | | Reporting progress or asking the user something | `Communicating with the User` | | Editing a workflow YAML file | `Workflow YAML Conventions` | | Choosing an OS, runtime, or toolchain target | `Supported Development Platforms` | @@ -88,4 +88,4 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | The About panel, description, or repo toggles | `Repository Details` | | Where a file belongs in the tree | `Repository Layout` | -Some of the rules above are also packaged as Claude Code / opencode / Codex Skills, hand-authored at [`.agents/skills/`](./.agents/skills/) in the hub, so they surface automatically instead of needing to be re-read every session. `scripts/` is hub-hosted and reached rather than carried, per "Hub-Hosted Tooling", so run the installer from a hub checkout: `python3 scripts/skills_install.py` (or the `.sh`/`.ps1` wrapper) once per machine, from `github.com/ptr727/ProjectTemplate`, installs them for every repo touched from that machine. `python3 scripts/skills_install.py --report`, also from a hub checkout, says whether this machine is current. A rule that keeps needing to be restated is a sign the install is missing or stale, not that the rule does not exist. +Some of the rules above are also packaged as Claude Code / opencode / Codex Skills, hand-authored at [`.agents/skills/`](./.agents/skills/) in the hub, so they surface automatically instead of needing to be re-read every session. `scripts/` is hub-hosted and reached rather than carried, per "Hub-Hosted Tooling", so run the installer from a hub checkout: `python3 scripts/skills_install.py` (or the `.sh`/`.ps1` wrapper) once per machine, from `github.com/ptr727/ProjectTemplate`, installs them for every repo touched from that machine. `python3 scripts/skills_install.py --report`, also from a hub checkout, says whether this machine is current. A rule that keeps needing to be restated is a sign the install is missing or stale, not that the rule does not exist. Keeping a repo's own carried `.github/copilot-instructions.md` in sync with the hub, without losing that repo's own "Disproved Claims" ledger entries in the process, is `copilot-instructions-keeper`, a skill about maintaining that file rather than a rule extracted from it, since the file itself is read directly by the Copilot bot and stays fully intact everywhere it is carried. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 37398c04..fb028b46 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -259,84 +259,9 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -> This "PR Review Etiquette" section is the provider-agnostic review-loop *contract* every fleet repo follows, alongside the [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook" that implements it. Without both in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to ad-hoc (and known-broken) behavior. +The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, reply and resolve, and never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. This is packaged as the `pr-review-conduct` Skill at [`.agents/skills/pr-review-conduct/SKILL.md`](./.agents/skills/pr-review-conduct/SKILL.md), read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself. -The repo runs a review loop on every PR: local agent iteration plus remote automated review (GitHub Copilot is the configured reviewer). Treat this as a contract regardless of which local agent authored the changes. - -### Merge Gate (read this first) - -**Do not merge, and do not enable auto-merge, unless ALL of these hold:** - -1. Required status checks are green (`mergeStateStatus: CLEAN`), and where they are not, the reason is **read** rather than inferred, because `BLOCKED` is one word for a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, so the response to it is decided from the check states and never from the word, **and** -2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, and because a review can carry the head SHA and still say it read only part of the diff, which is the same clean pass in everything a SHA match can see, so the body's own count of the files it read is checked against the files the pull request changed, **and** -3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and** -4. Nothing in the reviewer's output was a shape the review tooling could not read, since every reader keys on a marker and a marker that changes spelling is a section the reader stops finding and reports as absent, which is how three separate misreadings each reported a clean pass over a review they had not understood. An unrecognized shape blocks this gate on its own, and the remedy is an issue filed against the repository hosting the reader, naming the shape and quoting the body it came from, rather than a judgment about what the new wording probably meant, **and** -5. The maintainer has given **explicit** permission to merge. - -`mergeStateStatus: CLEAN` reflects **only** required statuses, and never open bot review comments, so `CLEAN` alone is **never** sufficient to merge. A green/`CLEAN` PR with an unresolved Copilot finding fails this gate, so treat it as "not mergeable" no matter what the merge-state field says. The agent never merges on its own (consistent with "default to staging", and merging is maintainer-authorized). - -**`BLOCKED` is not self-explaining either, and the remedy differs by cause.** A failed check is a defect to fix, an unresolved thread is a finding to close, and a required check sitting queued with no runner assigned is neither: it is hosted-runner capacity, which no re-request, rebase, or empty commit clears, and which a reader waiting on it cannot tell from patience. Report a blocked merge by naming the blocking check and its state rather than reporting the word, and where the cause is capacity, say so and stop rather than pushing at it, since the bypass is the maintainer's to run and a starved runner is not a reason to weaken a gate. - -**Merging is not releasing.** A merge to a release branch does **not** by itself publish. Publishing is a separate, explicitly configured step in the repo's release pipeline (e.g. a scheduled run, a manual dispatch, or an opted-in publish-on-merge trigger), not an automatic consequence of merging. Never describe a merge as cutting a release, and never trigger a publish without explicit maintainer instruction. - -### Expected Review Loop - -1. Push changes to the PR branch. -2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (reliable end-to-end, per the runbook). The UI is only a fallback. -3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. A review whose body says it did not review is the one exception, and it is terminal in the other direction: nothing follows it, re-requesting the same head repeats it, and the body names what has to change first. -4. Triage findings. -5. Apply fixes or write a rationale for declines. -6. Reply to each thread and resolve what was addressed. -7. Re-run the loop after every fix push until no actionable findings remain. - -Drive the loop to green, meaning a review confirmed on the latest head SHA and every actionable finding closed, then stop and apply the **Merge Gate** above: all five preconditions must hold, and `mergeStateStatus: CLEAN` alone never satisfies it. - -For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract, and that file owns the mechanics. - -### Every Finding Ends in an Action - -**A finding is closed by one of five outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. - -1. **It is real, so fix it.** Reply with the fixing commit SHA. -2. **It is not real, so disprove it in the thread**, with the command and its output, the code path that makes it impossible, or the rule that governs it. The proof is addressed to the reviewer as much as to the maintainer, since a decline it can read is what stops it raising the same thing next round. An assertion is not a proof and does not close a finding. -3. **It is real and deliberately not being fixed, which is the maintainer's call and not the agent's.** Say what the finding is, why the fix is unwanted, and get an explicit answer. Never suppress one by silence, by resolving the thread, or by an answer that reads as a decline while conceding the point. -4. **It is real and worth doing later, so file the issue first and reply with its link.** A deferral recorded only in a thread is lost the moment the pull request merges, so the issue is what carries it and the link is what proves it exists rather than being intended. This is for work the change did not create: an adjacent defect the reviewer noticed in passing, or a fix too large to ride along. It does not cover a defect in the change under review, because filing an issue about a bug you are about to merge is outcome 3 in other clothes, and that one is the maintainer's to decide. -5. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. - -### Triaging Review Comments - -**A low-confidence finding is not a low-value one.** Copilot collapses the findings it is least sure of into the review body instead of raising a thread, and in this fleet's experience those are right the large majority of the time. Judge each one against the code, never against its confidence label. They are also the easiest to lose, because they appear in no thread, so a loop that polls threads alone reports a clean pass while they stand (see the Merge Gate, condition 3). - -For each comment, classify before responding: - -- **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. -- **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: - - The cited rule matches what the existing codebase already does -> fix the offending code. - - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 5 above), rather than counting rounds until some threshold licenses it. -- **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation, and don't apply it unilaterally. - -### Responding and Resolution Expectations - -Reply inline with either the fixing commit SHA (for accepted issues) or the evidence that disproves it (for declines). **A decline carries proof rather than an assertion**, meaning the command and its output, the code path that makes the concern impossible, or the rule that governs it. "This is fine" is not a reply, and disagreeing without evidence is not addressing a finding, so a thread is not resolved on one. Resolve review threads when addressed, or when declined with that evidence recorded in the thread. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action, so acknowledge with a reply if needed and move on. - -After the final push on a PR, sweep older threads from earlier rounds whose code paths no longer exist, or stale unresolved markers remain in the review UI. - -**Answering a suppressed finding is a different act from replying in a thread, and it carries its own pairing.** A threaded reply sits under the comment it answers and the UI shows whether it is resolved. A suppressed finding has neither, so an answer that does not carry its own context is unverifiable: the maintainer cannot tell that it was seen, which finding it addresses, or whether any were skipped, and has to ask. An answer therefore **quotes the finding** in a blockquote, with its `file:line` anchor and enough of Copilot's own words to identify it, **carries one bold verdict per finding** (`Fixed in `, `Disproven`, or `No change needed`) so the outcomes are scannable without reading prose, **states the `(N)` count** the block heading gives so N answers can be checked against N findings, and **links the review** that raised them, since a PR accumulates rounds and an unlinked answer is ambiguous about which one it closes. One comment per review round keeps the answers together. - -**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. The hub's `scripts/pr_review.py status --repo /` reports every round and marks which are from earlier ones, and it names the repository because a pull request number resolves in every repository and a digest of the wrong one is well-formed. That script is hub-hosted rather than carried, so no repository holds a copy to run and it is invoked from a hub checkout per "Hub-Hosted Tooling" above. - -**The review's own overview cannot be trusted to say whether findings exist.** A body that reads "Copilot reviewed N out of N changed files and generated no new comments" routinely carries a collapsed block of suppressed findings directly beneath that sentence. Read the body for the block rather than the summary line, because the summary line and `reviewDecision` and an empty unresolved-thread list all agree that a review with four outstanding findings is clean. - -### Escalating to the User - -Bring the user in when: - -- **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. -- **A recurring finding** the code keeps attracting, which is the fix-the-class signal. Summarize the pattern and bring the remedy, whether that is the rule change or what the code has to say differently to stop earning it. -- **A finding you judge real but do not want fixed**, which is outcome 3 above and is never the agent's call to make quietly. -- **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation, and never apply it unilaterally. - -Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. +The provider-specific mechanics this contract needs to actually drive GitHub Copilot, how to request a review, poll for it, match the suppressed-findings heading, verify coverage, and reply or resolve a thread without a hand-typed id, live in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook", unaffected by this pointer. That content is read directly by the Copilot reviewer bot, which cannot read a Skill, so it stays fully written out rather than being packaged the same way. ## Communicating with the User From b8e347731bf48532fed5f072bf14b1762650373b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:20:52 -0700 Subject: [PATCH 2/6] Fix review round 1: accurate audit-coverage claim, no dead hub-only links - copilot-instructions-keeper: the fleet audit checks an intent file's file presence AND each named section's heading (spec/audit.py required_sections()), not presence alone. Reworded to state that precisely, keeping the real gap (content drift inside a present section goes unnoticed). - GOVERNANCE.md / AGENTS.md: dropped the repo-relative markdown link syntax around .agents/skills/ paths. That directory is hub-local and never carried into a downstream repo, so a markdown link to it inside byte-locked, fleet-carried text 404s everywhere except the hub. Named the path in backticks instead, the fleet's existing convention for a hub-only path referenced from carried content (scripts/*.py). Swept both instances rather than fixing only the one flagged. --- .agents/skills/copilot-instructions-keeper/SKILL.md | 10 +++++----- .claude-plugin/fleet-skills/.source-digest | 2 +- .../skills/copilot-instructions-keeper/SKILL.md | 10 +++++----- AGENTS.md | 2 +- GOVERNANCE.md | 2 +- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/.agents/skills/copilot-instructions-keeper/SKILL.md b/.agents/skills/copilot-instructions-keeper/SKILL.md index 964628c9..a2c79ebe 100644 --- a/.agents/skills/copilot-instructions-keeper/SKILL.md +++ b/.agents/skills/copilot-instructions-keeper/SKILL.md @@ -24,11 +24,11 @@ This skill is about maintaining that file's carried copy, not a substitute for i `spec/files.json` declares it `intent` fidelity, `whole: true`, covering three named sections (`Commit Messages and Pull Request Titles`, `Reviewing Carried Fleet Content`, `GitHub Copilot -Review Runbook`), with ``, ``, and `` placeholders filled per repo. **`intent` -fidelity is checked by the fleet audit for presence only, never content** (`RESYNC.md`), so a hub -revision to this file produces no finding anywhere in the mechanical audit. If this file is -stale, nothing else in the fleet's tooling will say so, noticing has to happen in a live session -like this one. +Review Runbook`), with ``, ``, and `` placeholders filled per repo. **The fleet +audit checks an `intent` file for file presence and each named section's heading, never for +content drift inside a section.** A section that is present but has fallen out of date against +the hub, the exact gap this skill exists to catch, produces no finding anywhere in the mechanical +audit. Noticing that has to happen in a live session like this one. ## The one thing this file has that others don't: repo-local ledger entries diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 16a2cb8b..1f29e6f7 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -7dbd5d23c3fd2a8d +81669a9895a1f458 diff --git a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md index 964628c9..a2c79ebe 100644 --- a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md @@ -24,11 +24,11 @@ This skill is about maintaining that file's carried copy, not a substitute for i `spec/files.json` declares it `intent` fidelity, `whole: true`, covering three named sections (`Commit Messages and Pull Request Titles`, `Reviewing Carried Fleet Content`, `GitHub Copilot -Review Runbook`), with ``, ``, and `` placeholders filled per repo. **`intent` -fidelity is checked by the fleet audit for presence only, never content** (`RESYNC.md`), so a hub -revision to this file produces no finding anywhere in the mechanical audit. If this file is -stale, nothing else in the fleet's tooling will say so, noticing has to happen in a live session -like this one. +Review Runbook`), with ``, ``, and `` placeholders filled per repo. **The fleet +audit checks an `intent` file for file presence and each named section's heading, never for +content drift inside a section.** A section that is present but has fallen out of date against +the hub, the exact gap this skill exists to catch, produces no finding anywhere in the mechanical +audit. Noticing that has to happen in a live session like this one. ## The one thing this file has that others don't: repo-local ledger entries diff --git a/AGENTS.md b/AGENTS.md index 171b5786..c47393b2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,4 +88,4 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | The About panel, description, or repo toggles | `Repository Details` | | Where a file belongs in the tree | `Repository Layout` | -Some of the rules above are also packaged as Claude Code / opencode / Codex Skills, hand-authored at [`.agents/skills/`](./.agents/skills/) in the hub, so they surface automatically instead of needing to be re-read every session. `scripts/` is hub-hosted and reached rather than carried, per "Hub-Hosted Tooling", so run the installer from a hub checkout: `python3 scripts/skills_install.py` (or the `.sh`/`.ps1` wrapper) once per machine, from `github.com/ptr727/ProjectTemplate`, installs them for every repo touched from that machine. `python3 scripts/skills_install.py --report`, also from a hub checkout, says whether this machine is current. A rule that keeps needing to be restated is a sign the install is missing or stale, not that the rule does not exist. Keeping a repo's own carried `.github/copilot-instructions.md` in sync with the hub, without losing that repo's own "Disproved Claims" ledger entries in the process, is `copilot-instructions-keeper`, a skill about maintaining that file rather than a rule extracted from it, since the file itself is read directly by the Copilot bot and stays fully intact everywhere it is carried. +Some of the rules above are also packaged as Claude Code / opencode / Codex Skills, hand-authored at `.agents/skills/` in the hub (not a repo-relative link here, since that path is hub-local and not carried into every fleet repo), so they surface automatically instead of needing to be re-read every session. `scripts/` is hub-hosted and reached rather than carried, per "Hub-Hosted Tooling", so run the installer from a hub checkout: `python3 scripts/skills_install.py` (or the `.sh`/`.ps1` wrapper) once per machine, from `github.com/ptr727/ProjectTemplate`, installs them for every repo touched from that machine. `python3 scripts/skills_install.py --report`, also from a hub checkout, says whether this machine is current. A rule that keeps needing to be restated is a sign the install is missing or stale, not that the rule does not exist. Keeping a repo's own carried `.github/copilot-instructions.md` in sync with the hub, without losing that repo's own "Disproved Claims" ledger entries in the process, is `copilot-instructions-keeper`, a skill about maintaining that file rather than a rule extracted from it, since the file itself is read directly by the Copilot bot and stays fully intact everywhere it is carried. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index fb028b46..bb94c339 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -259,7 +259,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, reply and resolve, and never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. This is packaged as the `pr-review-conduct` Skill at [`.agents/skills/pr-review-conduct/SKILL.md`](./.agents/skills/pr-review-conduct/SKILL.md), read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself. +The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, reply and resolve, and never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself. The provider-specific mechanics this contract needs to actually drive GitHub Copilot, how to request a review, poll for it, match the suppressed-findings heading, verify coverage, and reply or resolve a thread without a hand-typed id, live in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook", unaffected by this pointer. That content is read directly by the Copilot reviewer bot, which cannot read a Skill, so it stays fully written out rather than being packaged the same way. From 89ad6be062ec686bbd753e137995e3713f9b7a7e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:24:50 -0700 Subject: [PATCH 3/6] Fix round 2: same audit-coverage inaccuracy in the skill's own frontmatter The body text was fixed in b8e3477 but the YAML description reintroduced the same 'presence only' claim the earlier fix corrected. Reworded to match: the audit checks file presence and each named section's heading, never content drift inside a section. --- .agents/skills/copilot-instructions-keeper/SKILL.md | 5 +++-- .claude-plugin/fleet-skills/.source-digest | 2 +- .../fleet-skills/skills/copilot-instructions-keeper/SKILL.md | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.agents/skills/copilot-instructions-keeper/SKILL.md b/.agents/skills/copilot-instructions-keeper/SKILL.md index a2c79ebe..709346c9 100644 --- a/.agents/skills/copilot-instructions-keeper/SKILL.md +++ b/.agents/skills/copilot-instructions-keeper/SKILL.md @@ -9,8 +9,9 @@ description: >- Copilot's review mechanics in this file look stale, wrong, or missing something the fleet runbook should cover, or whenever standing up a new repo and carrying this file for the first time. Also triggers on "why isn't the audit catching that this file is out of date," since the - fleet's mechanical audit checks this file for presence only, at intent fidelity, never for - content drift, so nothing else notices staleness here except a live check like this one. + fleet's mechanical audit checks this file, at intent fidelity, for file presence and each named + section's heading, never for content drift inside a section, so nothing else notices a stale + section here except a live check like this one. --- # Copilot Instructions Keeper diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 1f29e6f7..2e7d269c 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -81669a9895a1f458 +78bedec46641e2bb diff --git a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md index a2c79ebe..709346c9 100644 --- a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md @@ -9,8 +9,9 @@ description: >- Copilot's review mechanics in this file look stale, wrong, or missing something the fleet runbook should cover, or whenever standing up a new repo and carrying this file for the first time. Also triggers on "why isn't the audit catching that this file is out of date," since the - fleet's mechanical audit checks this file for presence only, at intent fidelity, never for - content drift, so nothing else notices staleness here except a live check like this one. + fleet's mechanical audit checks this file, at intent fidelity, for file presence and each named + section's heading, never for content drift inside a section, so nothing else notices a stale + section here except a live check like this one. --- # Copilot Instructions Keeper From 30eb4622048717107f9899229dbbc58232d82aeb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:29:06 -0700 Subject: [PATCH 4/6] Fix round 3: split a run-on sentence in the PR Review Etiquette pointer GOVERNANCE.md's pointer paragraph joined too many independent clauses with commas, hard to parse quickly for a section whose whole job now is to be a short pointer. Split into two sentences per paragraph, no semicolons or em-dashes introduced (still matching this file's own prose style). --- GOVERNANCE.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index bb94c339..905f3e94 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -259,7 +259,9 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, reply and resolve, and never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself. +The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, and reply and resolve. Never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. + +This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself. The provider-specific mechanics this contract needs to actually drive GitHub Copilot, how to request a review, poll for it, match the suppressed-findings heading, verify coverage, and reply or resolve a thread without a hand-typed id, live in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook", unaffected by this pointer. That content is read directly by the Copilot reviewer bot, which cannot read a Skill, so it stays fully written out rather than being packaged the same way. From 217afacc2fe365977d456c7b17276f8bfbe7e045 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:32:57 -0700 Subject: [PATCH 5/6] Fix round 4: name the file by its full path consistently .agents/skills/copilot-instructions-keeper/SKILL.md used the bare 'copilot-instructions.md' once, inconsistent with every other reference in the same file, which name '.github/copilot-instructions.md'. Ambiguous since more than one repo in the fleet could carry an unrelated file of that bare name. --- .agents/skills/copilot-instructions-keeper/SKILL.md | 2 +- .claude-plugin/fleet-skills/.source-digest | 2 +- .../fleet-skills/skills/copilot-instructions-keeper/SKILL.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.agents/skills/copilot-instructions-keeper/SKILL.md b/.agents/skills/copilot-instructions-keeper/SKILL.md index 709346c9..e16f76c5 100644 --- a/.agents/skills/copilot-instructions-keeper/SKILL.md +++ b/.agents/skills/copilot-instructions-keeper/SKILL.md @@ -39,7 +39,7 @@ raised against this specific repository and disproved against this repository's revision. A repository carrying a copy of this file carries the shape and rules, deletes any entry whose subject it does not hold, and records what it has proved for itself. -This means a blind re-vendor of the hub's canonical `copilot-instructions.md` over a downstream +This means a blind re-vendor of the hub's canonical `.github/copilot-instructions.md` over a downstream repo's copy is wrong in both directions: - Copying the hub's own "Disproved Claims" entries (about `ProjectTemplate` itself) into a diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 2e7d269c..ab573f7d 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -78bedec46641e2bb +e8dab93ba0ba24e1 diff --git a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md index 709346c9..e16f76c5 100644 --- a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md @@ -39,7 +39,7 @@ raised against this specific repository and disproved against this repository's revision. A repository carrying a copy of this file carries the shape and rules, deletes any entry whose subject it does not hold, and records what it has proved for itself. -This means a blind re-vendor of the hub's canonical `copilot-instructions.md` over a downstream +This means a blind re-vendor of the hub's canonical `.github/copilot-instructions.md` over a downstream repo's copy is wrong in both directions: - Copying the hub's own "Disproved Claims" entries (about `ProjectTemplate` itself) into a From 5925bcfdbec0c7be1dccc29ae7955b54fbbb015b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 12 Aug 2026 22:37:53 -0700 Subject: [PATCH 6/6] Fix round 5: mergeStateStatus does reflect unresolved review threads Both the new skill and the GOVERNANCE.md pointer claimed mergeStateStatus: CLEAN 'reflects only required status checks,' carried forward from the pre-existing pre-trim GOVERNANCE.md text. This repo's own copilot-instructions.md runbook documents the opposite: BLOCKED is most often caused by unresolved review threads, which the ruleset's conversation-resolution requirement folds into the merge-state field. This PR's own round 1 -> round 2 transition confirms it directly (3 unresolved threads and merge=BLOCKED, then 0 unresolved and merge=CLEAN, with no check ever changing). Reworded to state the real gap precisely: CLEAN can reflect every *known* thread resolved while saying nothing about whether the review that resolved them covered the current head SHA, read the full diff, or left a suppressed low-confidence finding (which opens no thread at all) unanswered. --- .agents/skills/pr-review-conduct/SKILL.md | 18 +++++++++++------- .claude-plugin/fleet-skills/.source-digest | 2 +- .../skills/pr-review-conduct/SKILL.md | 18 +++++++++++------- GOVERNANCE.md | 2 +- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 08a73905..66affda8 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -8,8 +8,10 @@ description: >- merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, such as "merge this" or "it's all green, go ahead," because mergeStateStatus: CLEAN - reflects only required status checks and never open review findings, so a clean-looking PR can - still carry an unresolved or suppressed finding. Also triggers when a review loop looks stuck + can go clean once checks pass and every known thread is resolved, while still saying nothing + about whether the review that resolved those threads covered the current head SHA, read the + full diff, or left a suppressed low-confidence finding, which opens no thread at all, + unanswered. Also triggers when a review loop looks stuck (no review landing, findings that keep reappearing) or when deciding a finding is real, false, deferred, or a deliberate decline. Provider-specific mechanics (GitHub Copilot's request/poll/ reply API calls) live in .github/copilot-instructions.md's "GitHub Copilot Review Runbook," @@ -20,11 +22,13 @@ description: >- ## Why this exists -`mergeStateStatus: CLEAN` reports only required status checks. It says nothing about an open -review thread, a suppressed low-confidence finding sitting uncollapsed in a review body, or a -review that covered the head SHA but read only part of the diff. A PR that looks done, green -checks, no visible comments, routinely still carries a finding nobody has answered. Treating -"green" as "mergeable" is the single most common way this loop gets skipped. +`mergeStateStatus: CLEAN` reflects required status checks and any review thread the ruleset's +conversation-resolution requirement already tracks as resolved. It says nothing about whether the +review that resolved those threads actually covered the **current** head SHA, whether it read the +full diff rather than part of it, or whether a suppressed low-confidence finding, which never +opens a thread for the ruleset to see, was ever answered. A PR that looks done, green checks, no +visible comments, routinely still carries a finding nobody has answered. Treating "green" as +"mergeable" is the single most common way this loop gets skipped. ## Merge Gate, check this before merging or enabling auto-merge diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index ab573f7d..e71b9dc0 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -e8dab93ba0ba24e1 +5fb858e6b994c4d2 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 08a73905..66affda8 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -8,8 +8,10 @@ description: >- merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, such as "merge this" or "it's all green, go ahead," because mergeStateStatus: CLEAN - reflects only required status checks and never open review findings, so a clean-looking PR can - still carry an unresolved or suppressed finding. Also triggers when a review loop looks stuck + can go clean once checks pass and every known thread is resolved, while still saying nothing + about whether the review that resolved those threads covered the current head SHA, read the + full diff, or left a suppressed low-confidence finding, which opens no thread at all, + unanswered. Also triggers when a review loop looks stuck (no review landing, findings that keep reappearing) or when deciding a finding is real, false, deferred, or a deliberate decline. Provider-specific mechanics (GitHub Copilot's request/poll/ reply API calls) live in .github/copilot-instructions.md's "GitHub Copilot Review Runbook," @@ -20,11 +22,13 @@ description: >- ## Why this exists -`mergeStateStatus: CLEAN` reports only required status checks. It says nothing about an open -review thread, a suppressed low-confidence finding sitting uncollapsed in a review body, or a -review that covered the head SHA but read only part of the diff. A PR that looks done, green -checks, no visible comments, routinely still carries a finding nobody has answered. Treating -"green" as "mergeable" is the single most common way this loop gets skipped. +`mergeStateStatus: CLEAN` reflects required status checks and any review thread the ruleset's +conversation-resolution requirement already tracks as resolved. It says nothing about whether the +review that resolved those threads actually covered the **current** head SHA, whether it read the +full diff rather than part of it, or whether a suppressed low-confidence finding, which never +opens a thread for the ruleset to see, was ever answered. A PR that looks done, green checks, no +visible comments, routinely still carries a finding nobody has answered. Treating "green" as +"mergeable" is the single most common way this loop gets skipped. ## Merge Gate, check this before merging or enabling auto-merge diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 905f3e94..fd78a56a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -259,7 +259,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, and reply and resolve. Never merge on a green or CLEAN merge state alone, since that field reflects required status checks only and never an open review finding. +The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, and reply and resolve. Never merge on a green or CLEAN merge state alone, since that field can go clean once checks pass and every known thread is resolved while still saying nothing about whether the review covered the current head SHA, read the full diff, or left a suppressed finding, which opens no thread at all, unanswered. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the merge gate, the expected loop, and how a finding is closed, since this section only points to it now rather than carrying the contract text itself.