Package PR Review Conduct and Copilot Instructions Upkeep as Skills - #677
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR continues the hub-docs-to-Skills migration by packaging the PR review-loop contract and Copilot-instructions upkeep guidance as Skills, then updating governance pointers and regenerating the Claude plugin distribution so the skills can be installed and discovered consistently.
Changes:
- Adds two new Skills:
pr-review-conductandcopilot-instructions-keeper(source under.agents/skills/and generated copies under.claude-plugin/fleet-skills/). - Replaces the full
GOVERNANCE.md“PR Review Etiquette” contract text with a concise pointer to thepr-review-conductskill. - Updates
AGENTS.mdrouting text and regenerates the Claude plugin manifest + digest to include the new skills.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| GOVERNANCE.md | Shrinks “PR Review Etiquette” to a pointer paragraph directing readers to pr-review-conduct. |
| AGENTS.md | Updates “Where the Rules Live” routing to reference the new skill(s). |
| .agents/skills/pr-review-conduct/SKILL.md | Adds the canonical PR review-loop contract as a skill. |
| .agents/skills/copilot-instructions-keeper/SKILL.md | Adds the canonical “Copilot instructions upkeep” skill, emphasizing preservation of repo-local “Disproved Claims”. |
| .claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md | Generated Claude-plugin copy of the pr-review-conduct skill. |
| .claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md | Generated Claude-plugin copy of the copilot-instructions-keeper skill. |
| .claude-plugin/fleet-skills/.claude-plugin/plugin.json | Registers the two new skills in the Claude plugin manifest. |
| .claude-plugin/fleet-skills/.source-digest | Updates the source digest to reflect the new generated content. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…inks - 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.agents/skills/copilot-instructions-keeper/SKILL.md:13
- The YAML front-matter description still says the mechanical audit checks
.github/copilot-instructions.md“for presence only”. That contradicts the body below (which correctly notes the audit also checks required section headings). Align the description so it doesn’t reintroduce the earlier inaccuracy.
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.
…atter 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.
|
Answering the suppressed finding (1) from review round 2:
Fixed in 89ad6be. The body text was corrected in b8e3477 (round 1's fix for the same claim), but the YAML |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
GOVERNANCE.md:262
- This sentence is very long and reads as a run-on (multiple independent clauses joined by commas), which makes the new pointer section harder to parse quickly. Splitting it into a couple of sentences (and re-wrapping) would improve clarity without changing meaning.
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.
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).
|
Answering the suppressed findings (2) from review round 3:
No change needed here, already fixed in 89ad6be and answered in the round-2 comment above. Round 3's suppressed block is re-listing round 2's finding by its origin round, as documented, a suppressed finding has no resolved state so it keeps appearing until superseded, it was already closed before this round ran.
Fixed in 30eb462. Split the paragraph into two sentences per idea (the loop steps, then the never-merge-on-green warning; the skill pointer, then what to read it for), no semicolons or em-dashes introduced, matching this file's existing prose style. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.agents/skills/copilot-instructions-keeper/SKILL.md:43
- This refers to the hub’s canonical file as
copilot-instructions.md, but elsewhere (including earlier in this skill) the file is consistently named.github/copilot-instructions.md. Using the full path here avoids ambiguity (especially since multiple repos may have othercopilot-instructions.mdfiles).
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:
.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md:43
- This refers to the hub’s canonical file as
copilot-instructions.md, but elsewhere (including earlier in this skill) the file is consistently named.github/copilot-instructions.md. Using the full path here avoids ambiguity (especially since multiple repos may have othercopilot-instructions.mdfiles).
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:
.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.
|
Answering the suppressed findings (4, 2 new) from review round 4:
No change needed, already fixed in 89ad6be and answered in the round-2 comment. Still re-listed by origin round, as expected for a suppressed finding with no resolved state.
No change needed, already fixed in 30eb462 and answered in the round-3 comment.
Fixed in 217afac. Named the file by its full path at that one spot, matching every other reference in the same skill. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.agents/skills/pr-review-conduct/SKILL.md:27
- This paragraph states “
mergeStateStatus: CLEANreports only required status checks,” but the repo’s own runbook notesmergeStateStatuscan also reflect review-thread resolution / missing approvals (i.e., more than status checks). Consider rewording to focus on the real point:mergeStateStatusdoesn’t prove the latest head SHA was reviewed, the full diff was covered, or that suppressed body-only findings were triaged. After updating the source skill, regenerate the.claude-plugin/fleet-skills/copy via the normal build step so both stay in sync.
`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.
GOVERNANCE.md:262
- The sentence “merge state … reflects required status checks only” is misleading/inaccurate: in this repo’s own Copilot runbook,
mergeStateStatuscan also beBLOCKEDdue to unresolved review threads or missing approvals (not status checks), andCLEANstill doesn’t guarantee a re-review of the latest head SHA, full-diff coverage, or absence of suppressed body-only findings. Reword this to avoid claimingmergeStateStatusis status-check-only and instead state the specific gaps it doesn’t cover.
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.
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.
|
Answering the suppressed findings (5, 2 new) from review round 5: Three re-listed from earlier rounds (b8e3477's "presence only" claim, 89ad6be's run-on sentence, 30eb462's bare filename) — all already fixed and answered in prior round comments, no change needed.
Confirmed and fixed in 5925bcf. This is real, and this PR's own history proves it: round 1 had 3 unresolved threads and |
Round 3 review of #678. - GOVERNANCE.md's three skill-pointer paragraphs (Pull Request Title and Commit Message Conventions, Documentation Style Conventions, and the pre-existing PR Review Etiquette from #677) each said the section "only points to it now rather than carrying the contract text itself" directly under a paragraph that does summarize the contract, reading as a self-contradiction. Reworded all three the same way: the paragraph above is a sketch, the skill carries the full text. Swept the pre-existing PR Review Etiquette instance too, not just the two new ones the review flagged, since it is the same construction in the same file. - Renamed TestGovernanceCoupling to TestCommentAndDocStyleSkillCoupling in test_prose_lint.py, since the prior commit repointed it at the Skill and the old name no longer describes what it couples to. ## Verification performed scripts/test_prose_lint.py (223 tests), scripts/prose_lint.py, editorconfig-checker (docker), markdownlint-cli2, spec/validate.py all clean. CRLF byte-integrity confirmed on GOVERNANCE.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Package Comment and Doc Style as a Skill Phase 3 of the hub-docs-to-Skills conversion (Phase 0/1 shipped in #676, Phase 2 in #677). ## What this adds - comment-and-doc-style, extracted from CODESTYLE.md's General section (Tooling Names and Casing, Markdown and Spelling) plus GOVERNANCE.md's Documentation Style Conventions and Pull Request Title and Commit Message Conventions sections: naming tools in prose, Markdown linting and spelling, Markdown formatting, comment philosophy, the ASCII character-set tiers, line endings, the carried-file coordination reference ban, and PR title/commit message conventions. - Three references/ files for progressive disclosure: line-endings.md (the full CRLF/LF policy), carried-doc-references.md (the full coordination-reference rule and its two exceptions), and markdown-links.md (the full reference-style-link convention). ## What changed in existing files - CODESTYLE.md's General section: Tooling Names and Casing and Markdown and Spelling shrink to pointers at the skill. Clean-Compile Verification and Analyzer Diagnostics and Suppressions are untouched, they are build-gate topics, not comment or doc style. - GOVERNANCE.md's Pull Request Title and Commit Message Conventions and Documentation Style Conventions sections shrink to pointers, per this project's standing decision that a skill becomes the sole canonical content for its topic once one exists. The Comments, Character Set, and Line Endings headings stay in place (spec/files.json and spec/project-types.json anchor to them by name), their content is now a one-line pointer each. - AGENTS.md's Where the Rules Live table routes both rows to the new skill. - .claude-plugin/fleet-skills/ regenerated via scripts/build_dist.py to carry the new skill alongside the three from Phases 1 and 2. ## Verification performed - scripts/prose_lint.py, CRLF byte-integrity (all touched files remain 100% CRLF), editorconfig-checker (docker), spec/validate.py, markdownlint-cli2, scripts/test_build_dist.py + scripts/test_skills_install.py (46 tests) all clean. - scripts/build_dist.py --check confirms the generated plugin is current relative to .agents/skills/. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Phase 4 of the hub-docs-to-Skills conversion (see #676, #677, #678 for Phases 0-3). ## What this adds - **`resync-a-repo`** (hub-only): packages `RESYNC.md` section 3's apply order for a hub-context session driving a named repo's resync. The `carried-instruction-file-guard` skill's distinctive-phrase probe is now a mandatory, non-skippable step before any verbatim re-vendor of an instruction file, the direct structural fix for the AGENTS.md-overwrite bug class rather than a rewording of existing advisory language. - **`fleet-conformance-check`** (fleet-wide, `appliesTo: "*"`, new content): the downstream-facing counterpart. Runs from inside a repo's own session with no hub checkout and no named target other than the repo itself. Confirms the local Skills install is current, confirms `AGENTS.md`'s pointer text matches the hub, self-applies what is safe (re-running the installer), and escalates anything ambiguous (a genuine local addition, a settings/workflow drift) to a hub-driven `resync-a-repo` run rather than touching it. ## Supporting changes - `RESYNC.md` section 3 step 1 gains the mandatory probe requirement and the `AGENTS.md` skill-dependency pointer as one more verbatim-carried unit in the same step. - `AGENTS.md`'s "Where the Rules Live" table and pointer paragraph route to both new skills. - `.claude-plugin/fleet-skills/` rebuilt via `build_dist.py` (now 6 skills). ## Verification - `python3 -m unittest discover -s scripts -p "test_*.py"`: 628 tests, all pass. - `python3 scripts/prose_lint.py` on all touched files: clean. - `python3 spec/validate.py`: clean. - `docker run ... mstruebing/editorconfig-checker:latest`: clean. - CRLF preserved on all touched Markdown. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Phase 5 of the hub-docs-to-Skills conversion (Phase 0/1 shipped in #676, Phase 2 in #677, Phase 3 in #678, Phase 4 in #679). ## What this adds - `dotnet-codestyle` (appliesTo csharp), `python-codestyle` (appliesTo python), and `shell-codestyle` (appliesTo *, since a shell script can appear in any repo), extracted from CODESTYLE.md's .NET, Python, and Shell sections respectively: build/analyzer requirements, language and naming conventions, logging and testing conventions, the Python build-versus-lint-only profile split and toolchain, and the shell `set -Eeuo pipefail` / pipefail-early-reader / shellcheck rules. - This is the first phase gating a skill by `appliesTo` rather than fleet-wide `*`, expressed through each skill's own trigger wording (file extensions and project shape) rather than a mechanical field, since SKILL.md frontmatter carries no such field. ## What changed in existing files - CODESTYLE.md's .NET, Python, and Shell sections shrink to pointers, per this project's standing decision that a skill becomes the sole canonical content for its topic once one exists. No spec file or test references any subheading inside these three sections, so each collapses fully rather than keeping empty subheadings. Six now-orphaned link reference definitions (MD053) are removed with them. - CODESTYLE.md's and AGENTS.md's own descriptions of the file's structure are corrected to mention the pre-existing Shell section, which neither previously named. - `.claude-plugin/fleet-skills/` regenerated via `scripts/build_dist.py` to carry the three new skills alongside the six from Phases 1-4. ## Dropped - `docker-codestyle` (Phase 5b): a content-discovery pass found no dedicated Docker style section in CODESTYLE.md, only scattered linter-invocation mentions in OPERATIONS.md and release mechanics in GOVERNANCE.md, neither of which is Docker coding style. Not authored, per the plan's own don't-invent-content instruction. ## Verification performed - `scripts/prose_lint.py`, CRLF byte-integrity (all touched/new files 100% CRLF), `editorconfig-checker` (docker), `markdownlint-cli2` (docker, 72 files, 0 issues), `spec/validate.py`, `scripts/test_prose_lint.py` (223 tests), `scripts/test_build_dist.py` (19 tests), and `scripts/test_skills_install.py` all clean. - `scripts/build_dist.py --check` confirms the generated plugin is current relative to `.agents/skills/`. - Grepped `scripts/test_*.py` and `spec/*.json` for any coupling to the trimmed CODESTYLE.md subheadings before trimming: none found. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…Python CI Gates (#718) Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298. **Merge with a merge commit, never a squash, and never with `--delete-branch`.** This pull request's head is `develop` itself. ## What lands **Fleet Skills.** The `.agents/skills/` source tree, the generated `.claude-plugin/` distribution, `scripts/build_dist.py` with its `--check` gate, and `scripts/skills_install.py` with its host stamp (#676). Packaged as skills on top of the scaffold: PR review conduct and Copilot instructions upkeep (#677), comment and doc style (#678), resync-a-repo and fleet-conformance-check (#679), the per-language codestyles (#680), git commit conventions and operational vs release workflow (#681), stand up a repo (#683), and repo-worktree (#717). Coverage gaps closed in three passes (#690, #691, #692) plus the P4 sentence-length opt-in (#697). **Host setup.** The Windows host-setup tooling and its PowerShell gate (#674), the Windows bootstrap loader (#682), Docker install and upgrade on Linux and Windows with a version floor (#701, #705), a `uv` floor in `spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`, or `git-restore-mtime` copy (#689), node's real winget package id (#696), and a README for the Linux host-setup nuances (#710). **Python and CI.** Python tooling in CI with the script tests moved to `scripts/tests` (#704), `ruff format` adopted and gated (#709), and the PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files (#686). **Conduct rules.** Triage-order and scope guardrails in pr-review-conduct (#684), `pr_review.py wait` requesting a review rather than only polling for one (#685), a tech-agnostic signed-commit verification (#708), execution rather than analogy to verify platform-specific code (#715), and a unique worktree for every task (#717). **Docs.** The fleet map and gap register with peer messaging declared (#687), mermaid flow diagrams in the kept-authority docs (#702), and the map pointed at the shipped diagrams and current tooling (#703). ## Issues this promotion closes Each landed on `develop` on its own pull request. The keyword fires only on a merge into `main`, so it sits here rather than on the feature pull requests. Closes #700 Closes #707 Closes #711 Closes #712 Closes #714 Closes #688 #699 stays open on purpose: #717 shipped the layout convention and the skill, and the physical migration of existing checkouts is still tracked there. ## Review record Every squash closed its own Copilot loop on its own pull request before merging to `develop`. This promotion carries no new content of its own, so its review is the merged tree as a whole. ## Consequence worth stating The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed become the canonical the moment this reaches `main`, and every carrying repository reads as drifted from that point until it resyncs. That is the ordinary consequence of a canonical moving rather than a defect. The Skills installer added here is also how a machine picks the new skills up, so a session that keeps restating a rule already packaged as a skill is the signal to run it.
…tions.md (#730) Closes #720 Closes #722 Both defects are in `.github/copilot-instructions.md`, which every fleet repo carries whole, so each one reaches every carrier rather than staying in the hub. #722 is the same dead anchor as #720's second item, filed separately from a different downstream pass. **The hub slug (#720 item 1).** The "A Shape Nothing Recognizes Blocks the Loop and Earns an Issue" step named the hub as `ptr727/ProjectTemplate`. The file declares no verbatim section, so the audit's `carried:` check flags that name in any downstream copy, and a byte-faithful carry trips it: `audit.py --branch hub-resync-2026-08-15 PlexCleaner` reported exactly that on ptr727/PlexCleaner#922. The rule's own exception, in `comment-and-doc-style`'s `references/carried-doc-references.md`, is scoped to a verbatim region, so the canonical is what needs fixing rather than the check needing an exception. The step now routes the reader through the `AGENTS.md` "Fleet Bootstrap" section, which is byte-locked and whose whole function is to name the hub, matching the wording PlexCleaner already reworded its copy to. **The dead anchor (#720 item 2, #722).** The Disproved Claims section linked `../GOVERNANCE.md#every-finding-ends-in-an-action`. That heading left `GOVERNANCE.md` when the review-loop contract was packaged as the `pr-review-conduct` Skill (#677), where the rule now reads "Every finding ends in one of five outcomes", so the link resolves nowhere in the hub or in any carrier. It now points at "PR Review Etiquette", the section that routes to that Skill, and names the Skill. A sweep of every relative Markdown anchor in the tree (101 files) found this as the only dead one, so the fix is complete rather than the first of a batch. Prose gate and markdownlint clean on the file.
Phase 2 of the hub-docs-to-Skills conversion (Phase 0/1 shipped in #676).
What this adds
pr-review-conduct— the merge gate, expected review loop, the five finding-outcomes, triage, and suppressed-finding answering rules, extracted fromGOVERNANCE.md"PR Review Etiquette".copilot-instructions-keeper— new content (not derived from existing prose) for keeping a downstream repo's.github/copilot-instructions.mdin sync with the hub canonical without losing that repo's own "Disproved Claims" ledger entries, which the file's own rules say are never carried between repos.What changed in existing files
GOVERNANCE.md"PR Review Etiquette" shrinks from the full contract text to a two-paragraph pointer at thepr-review-conductskill, per this project's standing decision that a skill becomes the sole canonical content for its topic once one exists (see Scaffold fleet Skills: agents/skills source, dist build, installer #676's plan doc)..github/copilot-instructions.mdis untouched — the Copilot review bot reads it directly and cannot read a Skill, so its runbook content stays fully intact by design.AGENTS.md"Where the Rules Live" table and closing paragraph updated to route to both new skills..claude-plugin/fleet-skills/regenerated viascripts/build_dist.pyto carry the two new skills alongsidecarried-instruction-file-guardfrom Phase 1.Verification performed
scripts/prose_lint.py, CRLF byte-integrity,editorconfig-checker(docker),spec/validate.py,scripts/test_build_dist.py+scripts/test_skills_install.py(46 tests) all clean.scripts/build_dist.py --checkconfirms the generated plugin is current relative to.agents/skills/.claude --plugin-dir .claude-plugin/fleet-skills -p "..."against realistic prompts ("it's all green, go ahead and merge" forpr-review-conduct; "just a routine copy-over, right?" forcopilot-instructions-keeper) — both fired correctly and produced accurate, grounded answers without the rule being restated manually.🤖 Generated with Claude Code