diff --git a/.agents/skills/README.md b/.agents/skills/README.md index bdcc3096..aeffa88e 100644 --- a/.agents/skills/README.md +++ b/.agents/skills/README.md @@ -2,6 +2,6 @@ Canonical source for the fleet's Claude Code / opencode / Codex Skills, one directory per skill: `/SKILL.md` plus optional `scripts/` and `references/`. This is the only place a skill's content is hand-authored. Everything else derived from it is generated, never hand-edited. -Codex and opencode read this directory directly (`.agents/skills//SKILL.md`), no install step required, walking from a downstream repo's working directory up to its own repository root. Claude Code does not scan this path. `scripts/build_dist.py` generates a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. +Codex and opencode read this directory directly (`.agents/skills//SKILL.md`), no install step required, walking from a downstream repo's working directory up to its own repository root. Claude Code does not scan this path. GitHub Copilot discovers repository skills under `.github/skills/`. `scripts/build_dist.py` generates both the GitHub tree and a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. See `AGENTS.md` for how a repo depends on these skills and `scripts/README.md` for `build_dist.py` and the installer. The procedure for creating, changing, or retiring a skill is the `skill-lifecycle` skill (`skill-lifecycle/SKILL.md` here), which this README defers to. diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md new file mode 100644 index 00000000..af775184 --- /dev/null +++ b/.agents/skills/code-review/SKILL.md @@ -0,0 +1,71 @@ +--- +name: code-review +description: >- + Reviews a pull request or change set against the repository's contracts, with explicit diff + coverage and no suppressed findings. Use this whenever asked to review code, a pull request, + a patch, or a proposed change, and whenever GitHub Copilot performs code review. Triggers even + when the diff is documentation-only or workflow-only, because the review must load the + applicable general, language, documentation, and workflow skills before judging the change. +--- + +# Code Review + +## Establish the Contract + +1. Read the root `AGENTS.md` and the sections it routes to for the changed paths. +2. Read the complete diff and enumerate every changed file before forming findings. +3. Load every applicable sibling skill from the current skill distribution: + - `comment-and-doc-style` for Markdown, prose, comments, commit messages, and PR titles. + - `dotnet-codestyle` for C# and .NET changes. + - `python-codestyle` for Python changes. + - `shell-codestyle` for shell changes. + - `workflow-ci-contract` for GitHub Actions and CI/CD changes. +4. Treat a missing executable on `PATH` as no evidence that its check is unavailable. Read the + repository's documented local invocation before reporting a check as skipped. + +Do not substitute a familiar convention for the repository's written contract. Report a +conflict between instructions instead of silently choosing one. + +## Review the Change + +Review for correctness, regressions, security, compatibility, error handling, concurrency, +resource lifetime, tests, and contract drift. Follow data and control flow beyond the edited +lines when the behavior depends on unchanged callers or consumers. + +For each candidate finding: + +1. Verify it against the current head tree, not an unfetched checkout or the base branch. +2. Identify the concrete failing behavior and the conditions that reach it. +3. Confirm that the repository does not already prevent it elsewhere. +4. Prefer one root-cause finding over several symptoms of the same defect. +5. Omit pure preferences that no repository rule or user-visible risk supports. + +Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one +downstream repository does not carry is not a broken link. A substantive defect in canonical +content remains a finding, with the fix located at its canonical source. + +## Publish Every Finding + +Never suppress or hide a finding because confidence is low. Investigate until it is supported +or discard it. Publish every supported finding as an inline review comment when a changed line +can anchor it. Use the review body only when no valid inline anchor exists. + +Each finding states: + +- A concise imperative title with a severity. +- The file and smallest useful line range. +- The behavior that fails and the input or state that triggers it. +- Why the change causes the failure. +- A bounded direction for the fix when one is known. + +Do not report a clean review until every changed file has been read. End the review body with +exactly one ASCII marker, replacing the numbers with measured counts: + +```text + +``` + +`reviewed` is the number of changed files actually reviewed. `changed` is the total number of +changed files. `findings` is the number of published findings, including body-only findings. +Never emit `reviewed=changed` as a placeholder. If full coverage is impossible, emit the actual +counts and explain the limitation in the review body. diff --git a/.agents/skills/copilot-instructions-keeper/SKILL.md b/.agents/skills/copilot-instructions-keeper/SKILL.md index b9d0154d..e4b69c61 100644 --- a/.agents/skills/copilot-instructions-keeper/SKILL.md +++ b/.agents/skills/copilot-instructions-keeper/SKILL.md @@ -18,10 +18,10 @@ description: >- ## 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. +`.github/copilot-instructions.md` is read directly by GitHub Copilot and bootstraps the shared +`AGENTS.md` instruction set and review-focused skills. Its Copilot-specific rules stay fully +intact in every repo that carries it. This skill maintains that carried copy, it does not replace +the bootstrap. `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 diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 3e323cee..2880da22 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -13,9 +13,9 @@ description: >- 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," - this skill is the contract that runbook implements, not a replacement for it. + deferred, or a deliberate decline. Provider-specific mechanics are implemented by + scripts/pr_review.py and bootstrapped by .github/copilot-instructions.md. This skill is the + contract those surfaces implement, not a replacement for them. --- # PR Review Conduct @@ -134,13 +134,11 @@ head-scoped query while still unanswered. - 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 +## 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. +This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for +the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and +shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching +the finding's own words instead of a line number a fix push can move. The repository's +`.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable +coverage marker. Do not reconstruct the API operations by hand. diff --git a/.agents/skills/skill-lifecycle/SKILL.md b/.agents/skills/skill-lifecycle/SKILL.md index 85a73c51..720c59ac 100644 --- a/.agents/skills/skill-lifecycle/SKILL.md +++ b/.agents/skills/skill-lifecycle/SKILL.md @@ -1,7 +1,7 @@ --- name: skill-lifecycle description: >- - Governs the lifecycle of the fleet's own skills in ptr727/ProjectTemplate: creating, changing, splitting, and retiring a skill under .agents/skills/, the source-versus-generated split with .claude-plugin/, the regenerate and --check semantics of scripts/build_dist.py, the install and stamp semantics of scripts/skills_install.py, the doc-packaging pattern that keeps a law doc and its skill in agreement, and the trigger-description conventions that make a skill fire. Use this whenever about to create, edit, move, or delete anything under .agents/skills/ or .claude-plugin/, whenever packaging a doc or a doc section as a skill, and whenever deciding whether a topic deserves a skill at all. Triggers even when the edit looks trivial, such as fixing a typo in one SKILL.md, because the generated plugin desyncs the moment the source changes without a build_dist.py run, and CI fails the pull request on exactly that. Hub-context only, since .agents/skills/ exists only in the hub. + Governs the lifecycle of the fleet's own skills in ptr727/ProjectTemplate: creating, changing, splitting, and retiring a skill under .agents/skills/, the source-versus-generated split with .github/skills/ and .claude-plugin/, the regenerate and --check semantics of scripts/build_dist.py, the install and stamp semantics of scripts/skills_install.py, the doc-packaging pattern that keeps a law doc and its skill in agreement, and the trigger-description conventions that make a skill fire. Use this whenever about to create, edit, move, or delete anything under .agents/skills/, .github/skills/, or .claude-plugin/, whenever packaging a doc or a doc section as a skill, and whenever deciding whether a topic deserves a skill at all. Triggers even when the edit looks trivial, such as fixing a typo in one SKILL.md, because the generated distributions desync the moment the source changes without a build_dist.py run, and CI fails the pull request on exactly that. Hub-context only, since .agents/skills/ exists only in the hub. --- # Skill Lifecycle @@ -13,7 +13,7 @@ The agent most likely to get a skill wrong is the one editing a skill, and befor ## The Pipeline - **`.agents/skills//SKILL.md` is the only hand-authored source**, with optional `references/` and `scripts/` directories beside it. Codex and opencode read this tree directly, project-local, and also read the global `~/.agents/skills/` copy the installer materializes. -- **Claude Code never scans that path.** `scripts/build_dist.py` generates a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. The generated tree is never hand-edited, and `build_dist.py --check` exits non-zero when it was built from different source bytes than `.agents/skills/` currently holds. +- **Generated distributions serve GitHub Copilot and Claude Code.** `scripts/build_dist.py` generates `.github/skills/` for GitHub Copilot and a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. Neither generated tree is hand-edited, and `build_dist.py --check` exits non-zero when either tree differs from `.agents/skills/`. - **The skill set is implicit.** Every `.agents/skills//` directory carrying a `SKILL.md` is a skill, and the generated `plugin.json` derives its list from those directories, so adding or retiring a skill edits no manifest by hand. `marketplace.json` names the plugin, not the skills, and is untouched by ordinary lifecycle work. - **`scripts/skills_install.py`, run from a hub checkout, installs both forms per machine**: an overlay copy into `~/.agents/skills/` for Codex and opencode, marked per skill so a retired skill is removed on the next run and a foreign skill is never touched, and a user-scope plugin install for Claude Code via the `claude` CLI. Each run stamps the hub commit into `~/.agents/skills-install-stamp.json`, and `--report` reads that stamp against the checkout and exits non-zero when the machine is behind. The install is global per user, and per-repo pinning is a settled non-goal (`docs/fleet-map.md` "Skills Install Model"). @@ -28,13 +28,13 @@ A skill surfaces at a trigger moment. A rule that binds every action all the tim 3. **Author the body per the `comment-and-doc-style` skill**: LF (the repo default), present tense, ASCII tiers, no semicolon in prose. Name hub paths as plain code spans rather than repo-relative links, because an installed copy resolves no repo path, and say "from a hub checkout" for anything the reader must run. 4. **Split bulk into `references/`** when the source doc is large: the SKILL.md carries the summary and the binding rules, and each `references/*.md` carries one topic read on demand, the shape `comment-and-doc-style` uses. 5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. -6. **Regenerate and commit both trees together**: `python3 scripts/build_dist.py`, then commit the source and the regenerated `.claude-plugin/` in one commit. CI runs `--check` on every pull request and fails a desynced pair. `python3 scripts/tests/test_build_dist.py` covers the generator itself. +6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then commit the source and both generated trees in one commit. CI runs `--check` on every pull request and fails a desynced distribution. `python3 scripts/tests/test_build_dist.py` covers the generator itself. 7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. 8. **Refresh the machines after merge**: re-run `python3 scripts/skills_install.py` per machine, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then every machine serves the previous skill set, which `--report` says. ## Changing or Retiring a Skill -- **Edit only the source tree.** Any content change under `.claude-plugin/` that did not come from a `build_dist.py` run is a defect, whatever it fixes. +- **Edit only the source tree.** Any skill-content change under `.github/skills/` or `.claude-plugin/` that did not come from a `build_dist.py` run is a defect, whatever it fixes. - **Retiring is deleting the source directory and regenerating.** The derived `plugin.json` list shrinks with it, and the installer's per-skill markers remove the retired skill from `~/.agents/skills/` on each machine's next run. - **A deletion sweeps the prose that references the skill**, in the same change rather than as follow-up: the `AGENTS.md` map row or paragraph naming it, any law-doc packaging pointer to it, and any sibling skill that disambiguates against it. A law-doc section that had moved its full rules into the skill takes them back, or is retired with it, so no rule is silently lost with the skill that carried it. - **Renaming is a retire plus a create** as far as the installer's markers and the plugin list are concerned, so sweep references the same way. diff --git a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json index 0838d530..dbe5a08a 100644 --- a/.claude-plugin/fleet-skills/.claude-plugin/plugin.json +++ b/.claude-plugin/fleet-skills/.claude-plugin/plugin.json @@ -9,6 +9,7 @@ "./skills/agent-conduct", "./skills/audit-a-repo", "./skills/carried-instruction-file-guard", + "./skills/code-review", "./skills/comment-and-doc-style", "./skills/copilot-instructions-keeper", "./skills/dotnet-codestyle", diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index cf7f4bef..b03fb574 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -2796eec540f960cb +cf555123c5d2982d diff --git a/.claude-plugin/fleet-skills/skills/code-review/SKILL.md b/.claude-plugin/fleet-skills/skills/code-review/SKILL.md new file mode 100644 index 00000000..af775184 --- /dev/null +++ b/.claude-plugin/fleet-skills/skills/code-review/SKILL.md @@ -0,0 +1,71 @@ +--- +name: code-review +description: >- + Reviews a pull request or change set against the repository's contracts, with explicit diff + coverage and no suppressed findings. Use this whenever asked to review code, a pull request, + a patch, or a proposed change, and whenever GitHub Copilot performs code review. Triggers even + when the diff is documentation-only or workflow-only, because the review must load the + applicable general, language, documentation, and workflow skills before judging the change. +--- + +# Code Review + +## Establish the Contract + +1. Read the root `AGENTS.md` and the sections it routes to for the changed paths. +2. Read the complete diff and enumerate every changed file before forming findings. +3. Load every applicable sibling skill from the current skill distribution: + - `comment-and-doc-style` for Markdown, prose, comments, commit messages, and PR titles. + - `dotnet-codestyle` for C# and .NET changes. + - `python-codestyle` for Python changes. + - `shell-codestyle` for shell changes. + - `workflow-ci-contract` for GitHub Actions and CI/CD changes. +4. Treat a missing executable on `PATH` as no evidence that its check is unavailable. Read the + repository's documented local invocation before reporting a check as skipped. + +Do not substitute a familiar convention for the repository's written contract. Report a +conflict between instructions instead of silently choosing one. + +## Review the Change + +Review for correctness, regressions, security, compatibility, error handling, concurrency, +resource lifetime, tests, and contract drift. Follow data and control flow beyond the edited +lines when the behavior depends on unchanged callers or consumers. + +For each candidate finding: + +1. Verify it against the current head tree, not an unfetched checkout or the base branch. +2. Identify the concrete failing behavior and the conditions that reach it. +3. Confirm that the repository does not already prevent it elsewhere. +4. Prefer one root-cause finding over several symptoms of the same defect. +5. Omit pure preferences that no repository rule or user-visible risk supports. + +Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one +downstream repository does not carry is not a broken link. A substantive defect in canonical +content remains a finding, with the fix located at its canonical source. + +## Publish Every Finding + +Never suppress or hide a finding because confidence is low. Investigate until it is supported +or discard it. Publish every supported finding as an inline review comment when a changed line +can anchor it. Use the review body only when no valid inline anchor exists. + +Each finding states: + +- A concise imperative title with a severity. +- The file and smallest useful line range. +- The behavior that fails and the input or state that triggers it. +- Why the change causes the failure. +- A bounded direction for the fix when one is known. + +Do not report a clean review until every changed file has been read. End the review body with +exactly one ASCII marker, replacing the numbers with measured counts: + +```text + +``` + +`reviewed` is the number of changed files actually reviewed. `changed` is the total number of +changed files. `findings` is the number of published findings, including body-only findings. +Never emit `reviewed=changed` as a placeholder. If full coverage is impossible, emit the actual +counts and explain the limitation in the review body. 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 b9d0154d..e4b69c61 100644 --- a/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/copilot-instructions-keeper/SKILL.md @@ -18,10 +18,10 @@ description: >- ## 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. +`.github/copilot-instructions.md` is read directly by GitHub Copilot and bootstraps the shared +`AGENTS.md` instruction set and review-focused skills. Its Copilot-specific rules stay fully +intact in every repo that carries it. This skill maintains that carried copy, it does not replace +the bootstrap. `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 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 3e323cee..2880da22 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -13,9 +13,9 @@ description: >- 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," - this skill is the contract that runbook implements, not a replacement for it. + deferred, or a deliberate decline. Provider-specific mechanics are implemented by + scripts/pr_review.py and bootstrapped by .github/copilot-instructions.md. This skill is the + contract those surfaces implement, not a replacement for them. --- # PR Review Conduct @@ -134,13 +134,11 @@ head-scoped query while still unanswered. - 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 +## 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. +This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for +the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and +shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching +the finding's own words instead of a line number a fix push can move. The repository's +`.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable +coverage marker. Do not reconstruct the API operations by hand. diff --git a/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md b/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md index 85a73c51..720c59ac 100644 --- a/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md @@ -1,7 +1,7 @@ --- name: skill-lifecycle description: >- - Governs the lifecycle of the fleet's own skills in ptr727/ProjectTemplate: creating, changing, splitting, and retiring a skill under .agents/skills/, the source-versus-generated split with .claude-plugin/, the regenerate and --check semantics of scripts/build_dist.py, the install and stamp semantics of scripts/skills_install.py, the doc-packaging pattern that keeps a law doc and its skill in agreement, and the trigger-description conventions that make a skill fire. Use this whenever about to create, edit, move, or delete anything under .agents/skills/ or .claude-plugin/, whenever packaging a doc or a doc section as a skill, and whenever deciding whether a topic deserves a skill at all. Triggers even when the edit looks trivial, such as fixing a typo in one SKILL.md, because the generated plugin desyncs the moment the source changes without a build_dist.py run, and CI fails the pull request on exactly that. Hub-context only, since .agents/skills/ exists only in the hub. + Governs the lifecycle of the fleet's own skills in ptr727/ProjectTemplate: creating, changing, splitting, and retiring a skill under .agents/skills/, the source-versus-generated split with .github/skills/ and .claude-plugin/, the regenerate and --check semantics of scripts/build_dist.py, the install and stamp semantics of scripts/skills_install.py, the doc-packaging pattern that keeps a law doc and its skill in agreement, and the trigger-description conventions that make a skill fire. Use this whenever about to create, edit, move, or delete anything under .agents/skills/, .github/skills/, or .claude-plugin/, whenever packaging a doc or a doc section as a skill, and whenever deciding whether a topic deserves a skill at all. Triggers even when the edit looks trivial, such as fixing a typo in one SKILL.md, because the generated distributions desync the moment the source changes without a build_dist.py run, and CI fails the pull request on exactly that. Hub-context only, since .agents/skills/ exists only in the hub. --- # Skill Lifecycle @@ -13,7 +13,7 @@ The agent most likely to get a skill wrong is the one editing a skill, and befor ## The Pipeline - **`.agents/skills//SKILL.md` is the only hand-authored source**, with optional `references/` and `scripts/` directories beside it. Codex and opencode read this tree directly, project-local, and also read the global `~/.agents/skills/` copy the installer materializes. -- **Claude Code never scans that path.** `scripts/build_dist.py` generates a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. The generated tree is never hand-edited, and `build_dist.py --check` exits non-zero when it was built from different source bytes than `.agents/skills/` currently holds. +- **Generated distributions serve GitHub Copilot and Claude Code.** `scripts/build_dist.py` generates `.github/skills/` for GitHub Copilot and a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. Neither generated tree is hand-edited, and `build_dist.py --check` exits non-zero when either tree differs from `.agents/skills/`. - **The skill set is implicit.** Every `.agents/skills//` directory carrying a `SKILL.md` is a skill, and the generated `plugin.json` derives its list from those directories, so adding or retiring a skill edits no manifest by hand. `marketplace.json` names the plugin, not the skills, and is untouched by ordinary lifecycle work. - **`scripts/skills_install.py`, run from a hub checkout, installs both forms per machine**: an overlay copy into `~/.agents/skills/` for Codex and opencode, marked per skill so a retired skill is removed on the next run and a foreign skill is never touched, and a user-scope plugin install for Claude Code via the `claude` CLI. Each run stamps the hub commit into `~/.agents/skills-install-stamp.json`, and `--report` reads that stamp against the checkout and exits non-zero when the machine is behind. The install is global per user, and per-repo pinning is a settled non-goal (`docs/fleet-map.md` "Skills Install Model"). @@ -28,13 +28,13 @@ A skill surfaces at a trigger moment. A rule that binds every action all the tim 3. **Author the body per the `comment-and-doc-style` skill**: LF (the repo default), present tense, ASCII tiers, no semicolon in prose. Name hub paths as plain code spans rather than repo-relative links, because an installed copy resolves no repo path, and say "from a hub checkout" for anything the reader must run. 4. **Split bulk into `references/`** when the source doc is large: the SKILL.md carries the summary and the binding rules, and each `references/*.md` carries one topic read on demand, the shape `comment-and-doc-style` uses. 5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. -6. **Regenerate and commit both trees together**: `python3 scripts/build_dist.py`, then commit the source and the regenerated `.claude-plugin/` in one commit. CI runs `--check` on every pull request and fails a desynced pair. `python3 scripts/tests/test_build_dist.py` covers the generator itself. +6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then commit the source and both generated trees in one commit. CI runs `--check` on every pull request and fails a desynced distribution. `python3 scripts/tests/test_build_dist.py` covers the generator itself. 7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. 8. **Refresh the machines after merge**: re-run `python3 scripts/skills_install.py` per machine, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then every machine serves the previous skill set, which `--report` says. ## Changing or Retiring a Skill -- **Edit only the source tree.** Any content change under `.claude-plugin/` that did not come from a `build_dist.py` run is a defect, whatever it fixes. +- **Edit only the source tree.** Any skill-content change under `.github/skills/` or `.claude-plugin/` that did not come from a `build_dist.py` run is a defect, whatever it fixes. - **Retiring is deleting the source directory and regenerating.** The derived `plugin.json` list shrinks with it, and the installer's per-skill markers remove the retired skill from `~/.agents/skills/` on each machine's next run. - **A deletion sweeps the prose that references the skill**, in the same change rather than as follow-up: the `AGENTS.md` map row or paragraph naming it, any law-doc packaging pointer to it, and any sibling skill that disambiguates against it. A law-doc section that had moved its full rules into the skill takes them back, or is retired with it, so no rule is silently lost with the skill that carried it. - **Renaming is a retire plus a create** as far as the installer's markers and the plugin list are concerned, so sweep references the same way. diff --git a/.github/actions/validate/action.yml b/.github/actions/validate/action.yml index cee7a16b..caf2ed79 100644 --- a/.github/actions/validate/action.yml +++ b/.github/actions/validate/action.yml @@ -37,7 +37,7 @@ runs: uvx coverage@latest run --source=scripts,spec,host-setup --append host-setup/agent-safety/test_install.py uvx coverage@latest report - # Read-only: fails if .claude-plugin/fleet-skills/ was not regenerated from .agents/skills/. + # Read-only: fails if a generated skill distribution differs from .agents/skills/. - name: Check fleet skills are current step shell: bash run: python3 scripts/build_dist.py --check diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 14662b5d..e3e4ae82 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,319 +1,51 @@ # Copilot Instructions -Repository conventions for GitHub Copilot (and any other AI agent reading this file). +Repository-wide instructions for GitHub Copilot. -The **canonical guide is [AGENTS.md](../AGENTS.md)** at the repo root. Read it first, then the [PR Review Etiquette](../GOVERNANCE.md#pr-review-etiquette) review-loop contract this file's runbook implements. This file is intentionally narrow: commit/PR-title conventions (summarized inline so VS Code's commit-message and PR-title generators have them), guidance for reviewing carried fleet content, plus the GitHub Copilot Review Runbook. +Read [AGENTS.md](../AGENTS.md) first. It routes every standing repository rule to its canonical +document. When performing code review, load and follow the `code-review` skill in +`.github/skills/code-review/SKILL.md`, then load every language, documentation, or workflow skill +that it selects for the changed files. GitHub Copilot reads these files from the pull request's +head branch, so review the instructions in that tree. -For code-style rules, see [`CODESTYLE.md`](../CODESTYLE.md) at the repo root, one guide with a General section plus a section per language the repo uses. - -Do not duplicate language-specific rules here. **Project-specific conventions and API/behavioral contracts also belong in [GOVERNANCE.md](../GOVERNANCE.md), not here.** This file is intentionally limited to the inline commit/PR-title summary, the guidance for reviewing carried fleet content, and the GitHub Copilot Review Runbook. Non-Copilot agents (Claude Code, Codex, Cursor, ...) are not directed to this file and don't read it by default, so any rule a reviewer must honor has to live in `GOVERNANCE.md`, routed to from `AGENTS.md`, to be provider-independent. +Do not duplicate rules from `AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, or `WORKFLOW.md` here. +This file contains only Copilot-specific bootstrap and output requirements. ## Commit Messages and Pull Request Titles -Summarized for VS Code's generators. The full rules, rationale, and examples are in [GOVERNANCE.md "Pull Request Title and Commit Message Conventions"](../GOVERNANCE.md#pull-request-title-and-commit-message-conventions). - -- Imperative subject, <= 72 characters, no trailing period, with an optional blank-line-separated body for the non-obvious *why*. -- US English, title case with lowercase short bind words. No vague titles, no `Co-Authored-By:` unless asked, no release-bump magnitude (NBGV handles versioning). Dependabot's `Bump X from Y to Z` titles are fine. -- develop PRs squash-merge (`gh pr merge --squash`), main PRs merge-commit (`--merge`). A mismatched flag is rejected by branch protection. +Use an imperative subject of at most 72 characters with no trailing period. Use US English and +title case with lowercase short bind words. Do not add `Co-Authored-By:` unless requested. Do not +put a release-bump magnitude in the title. The full contract is in +[GOVERNANCE.md "Pull Request Title and Commit Message Conventions"](../GOVERNANCE.md#pull-request-title-and-commit-message-conventions). ## Reviewing Carried Fleet Content -Several of this repository's governance files are carried from a shared template and kept in sync across a fleet of sibling repositories, among them `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, this file, and the `repo-config/` rulesets. Most of `GOVERNANCE.md` is universal fleet law: every section that states a rule, as opposed to the two that describe this repository's own directory tree and devcontainer, is byte-locked and verified by an automated byte-for-byte match against the template canonical, not by line-by-line review. `AGENTS.md` is the thin router and carries three byte-locked sections of its own, `Fleet Bootstrap`, `Context and Delegation Discipline`, and `Where the Rules Live`. Everything above the first of those headings is this repository's own prose, saying what this repository is and which local doc owns which subject, and it is editable like any other repository content. - -Three constraints follow when reviewing that content. - -- **A reference inside byte-locked text to a path or section this repository does not carry is intentional, not a broken link.** Universal rule text names shared infrastructure (a fleet registry, a reusable config snippet, the other workflow model's ruleset payload) that a given repository legitimately may not contain. Editing the text to "fix" such a reference would break the fleet audit that governs it, so the reference is correct as written. Do not report it as a dead link, a missing file, or a broken cross-reference. -- **A genuine substantive defect is still worth raising.** Byte-locked is not unreviewable. A self-contradiction, a factual error, or a real typo in the canonical prose is a valid finding, but note that the fix lands at the template and re-vendors to every repository, rather than proposing a local edit the audit would reject. -- **A reference to a hub script is a pointer to follow, not a broken local path.** The fleet's gates live in one place and a repository runs them from a checkout of that place rather than holding a copy, so `scripts/prose_lint.py` (prose the CI linters pass on), `scripts/repo_gate.py` (repository settings and action pins), `scripts/pr_review.py` (the review digest, and reply plus resolve without a hand-typed id), and `spec/audit.py` (the conformance audit) resolve there and in none of the repositories they measure. [GOVERNANCE.md "Documentation Style Conventions"](../GOVERNANCE.md#documentation-style-conventions) carries the exception that permits such a pointer inside carried text, and [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling) states how one is reached and what to report when it cannot be. Reach for them before writing a check of your own, since a reconstructed gate encodes its author's reading of a rule rather than the rule, and agrees with no other repository. +Follow the fidelity declared for the file. A byte-locked reference to shared infrastructure that +this repository does not carry is intentional, not a broken link. Raise substantive defects in +canonical content, but locate the fix at its canonical source instead of proposing a local edit +that its fidelity rejects. ## GitHub Copilot Review Runbook -> This runbook implements the [GOVERNANCE.md "PR Review Etiquette"](../GOVERNANCE.md#pr-review-etiquette) review-loop contract for GitHub Copilot. Without it in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to known-broken paths (the no-op `POST /requested_reviewers`, the wrong bot-login filter). In the API snippets below, fill the `` / `` / `` placeholders. - -Use this section for provider-specific mechanics. The expected review loop *contract* (request review on every push, verify head-SHA coverage, triage findings, reply + resolve, escalate when stuck) is defined in [GOVERNANCE.md -> PR Review Etiquette](../GOVERNANCE.md#pr-review-etiquette). This section only describes how to make GitHub Copilot reliably execute it. - -### Triggering and Polling - -Auto-review on push is configured (via the branch ruleset's `copilot_code_review` rule with `review_on_push: true`) but fires inconsistently in practice, so treat it as best-effort, not guaranteed. After every push, **re-request a review programmatically** via the GraphQL `requestReviews` mutation, passing the Copilot reviewer's bot node id in `botIds`. This drives the loop end-to-end without a UI hand-off. - -**A review with no inline comments is still a completed review, not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop, and it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent. - -**The one exception is a review that says it did not review, and it is delivered in exactly that shape.** Copilot answers a pull request it will not take on with a formal review, `state: COMMENTED`, carrying the correct `commit.oid` and **zero** inline threads, whose whole body is a refusal: "Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again." Every coverage check passes, the rule above says an empty review is the clean pass, and the two together read a round that never happened as a round that found nothing. Observed on a pull request of 301 changed files, one over the limit, which was one command from merging on it. **The limit is 300 changed files and the remedy is to split the pull request**, since re-requesting the same head repeats the refusal: the file count is what it declined on and re-requesting does not change it. A repository committing binary or generated data alongside code crosses that line easily. Match the refusal on the body's **opening line** rather than anywhere in it, because a review discussing the wording is not one carrying it, and one line rather than two, because a review's first line is its heading and its second is the overview prose where such a description sits. Match an alternation for the same reason the suppressed heading takes one: - -```sh -# A review whose opening line declines the round. That line is the unit, since a refusal is -# the whole body and a match further down is a review quoting the wording rather than refusing. -# The dot spans both spellings of the apostrophe, the typographic one Copilot writes and the -# ASCII one, and it also keeps this filter usable inside single quotes, which neither survives. -gh api repos///pulls//reviews --jq \ - '.[] | select([(.body // "") | split("\n")[] | select(. != "")][0] // "" - | test("wasn.t able to review|was not able to review|unable to review")) | {commit_id, body}' -``` - -**Read the low-confidence findings, which are not inline threads.** A review body can carry a collapsed `
` block of findings Copilot withheld from the inline threads, and those findings appear nowhere in `reviewThreads`, so a loop that polls threads alone never sees them and reports a clean pass. **Match the block on more than one phrasing.** Its heading has appeared both as `Suppressed comments (N)` and as "Comments suppressed due to low confidence", so a filter keyed on either one alone silently reports zero suppressed findings on a review that has them, the same false clean this rule exists to prevent, one level up in the detection. **The section moves as well as it is worded, so match the heading wherever it sits.** It has appeared as its own `
` wrapper with a matching ``, as a bare heading in the body, and as a Markdown heading nested inside the `Review details` wrapper, whose `` names the wrapper and not the section. A filter reading a wrapper's `` reports zero on the nested shape, and the count it needs is the heading's own `(N)` rather than the wrapper's. They have been right repeatedly, including a rule stated more broadly than its check enforced and a check that skipped fenced blocks in every rule but one. Read the body of every review, investigate each suppressed finding on the same footing as an inline one, and answer it in the PR conversation, since a suppressed finding has no thread to reply on or resolve. - -```sh -# `test` with an alternation, not `contains` on one phrasing: the heading wording has changed. -gh api repos///pulls//reviews --jq \ - '.[] | select(.body | test("Suppressed comments|low confidence")) | .body' - -# Read every round, not only the head. A suppressed finding has no resolved state, so a push -# does not retire it: it simply stops appearing in a head-scoped query while still unanswered. -# Head-scoping this query is how four rounds went unanswered across three pull requests in a day. -gh api repos///pulls//reviews --jq \ - '[.[] | select(.body | test("Suppressed comments|low confidence"))] | length' - -# Mark which round each came from, since a finding on an older round may since be moot. -PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') -gh api repos///pulls//reviews --jq \ - "[.[] | select(.body | test(\"Suppressed comments|low confidence\")) - | {round: (if .commit_id == \"$PR_HEAD\" then \"head\" else \"earlier\" end), id}]" -``` - -**Round 1 is normally auto-seeded, so poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes, and on some pull requests it never fires at all. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. Where it never lands, drive round 1 with the same `requestReviews` mutation every later round uses, which needs nothing this PR has to produce first. A round 1 carrying no review therefore means "wait, then request it yourself," **not** "ask the maintainer to kick it off." - -> **The reviewer login differs by API, in three forms rather than two.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. In a REST **timeline** `review_requested` event the `requested_reviewer` is a third spelling again, login `Copilot` with `type` `Bot`, so a filter written against either of the other two selects nothing there and reports a pull request with requests as having none. Match on the type plus a loose login test rather than on any one spelling, and each query below uses the correct form for its API. - -```sh -# 1. PR node id, plus the reviewer bot's node id read across the repo's recent PRs. -# The bot id is the reviewer account's own, so every PR in the repo carries the same one. -# The reviewer login is `copilot-pull-request-reviewer` in GraphQL. -PR_NODE=$(gh pr view --json id --jq '.id') -BOT_ID=$(gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequests(first: 20, orderBy: { field: CREATED_AT, direction: DESC }) { - nodes { reviews(first: 20) { nodes { author { __typename login ... on Bot { id } } } } } - } - } -}' --jq '[.data.repository.pullRequests.nodes[].reviews.nodes[] - | select(.author.login == "copilot-pull-request-reviewer") - | .author.id] | first // empty') -if [ -z "$BOT_ID" ]; then - echo "no Copilot review in the 20 most recent PRs, so widen the window" >&2 - return 1 2>/dev/null || exit 1 # Stop. Do NOT call requestReviews with an empty id. -fi - -# 2. Re-request a Copilot review on the current head. -gh api graphql -f query=' -mutation($pr: ID!, $bot: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { - pullRequest { id } - } -}' -F pr="$PR_NODE" -F bot="$BOT_ID" -``` - -**The bot node id belongs to the reviewer account, not to a pull request**, and it is the same id on **every PR in the repo**, so nothing has to land on this PR before step 1 can read it. A PR opened a minute ago, with no review and no comment of its own, needs no UI seeding to bootstrap the id and no prior review to source it from: any Copilot review anywhere in the repo carries it. Query the **most recent** PRs, since a plain `last: 20` returns the *oldest* ones, which may predate Copilot on the repo. **Guard for an empty result**, because an empty `$BOT_ID` says only that none of the PRs sampled carry a Copilot review, so widen the window (raise the count or paginate) before concluding the repo has never had one. Never pass an empty id to the mutation. - -A read scoped to this PR (`pullRequest(number: ) { reviews }`) returns the same id once a review has landed here, and it buys nothing over the repo-wide read while failing on exactly the round the repo-wide read handles. Where the repo's only Copilot artifact is an issue comment rather than a formal review, read the id from that comment's author instead (`pullRequest.comments` -> author `... on Bot { id }`). Manual UI seeding is the last resort, needed only for a repo that has **never** had a Copilot review, so no prior id exists anywhere to read. - -**Do NOT post `@Copilot review` as a PR comment.** That comment triggers the Copilot *coding agent* (`copilot-swe-agent[bot]`), which makes code changes rather than posting a review. - -Known non-working request paths (don't rely on them, and use the `requestReviews` mutation above instead): - -- `POST /requested_reviewers` with `reviewers=[Copilot]` can return 200 but no-op. -- `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. -- `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node`, because the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`. -- `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer`, so do not source the reviewer's bot node id there. Read it from an existing review per step 1 above. -- There is no `removePullRequestFromReviewRequest` mutation, but removal is not therefore impossible: `requestReviews` **replaces** the reviewer set when `union` is false (the schema describes `union` as "add users to the set rather than replace"), so an empty `botIds` with `union: false` removes the pending request. Reach for it only in the stuck case below, since `union: true` re-fires a review on the current head without it. -- `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. Read the pending set through GraphQL `reviewRequests`, which returns the `Bot` node, because the REST-backed projection makes a pending request read as no request at all. - -### Verify Review Covered Current Head - -Before merging, confirm Copilot reviewed the current PR head SHA. Copilot may respond as either a formal review (carries an exact commit SHA) or an issue comment (no SHA, so use the most recent Copilot comment for manual confirmation). Check both. - -**Count matches and compare numerically, so an empty result cannot read as success.** A poll that captures a `gh api --jq` result and exits on `[ "$found" != "0" ]` treats an **empty** string as a landed review, and an empty string is exactly what a mis-written filter returns. Pipe the matches through `wc -l` and test `-gt 0`, so a query that finds nothing and a query that ran wrong both read as "not yet". A `gh` call that fails to run reaches the test the same way, because it writes its message to stderr and prints nothing to stdout, so the `$(...)` around it still yields the empty string. A mistyped or unsupported flag is the usual cause, and `gh` reports one as `accepts 1 arg(s), received 4` rather than as anything resembling a review verdict. - -**Check head coverage before reading merge-state, never the reverse.** A push makes the required checks go green before Copilot re-reviews the new head, so `mergeStateStatus` can read `CLEAN` in the window before any formal review covers the head. A poll that exits on `CLEAN` merges into that gap. Gate on a formal review whose `commit.oid` equals the current head SHA first, then on zero unresolved threads, and only then read merge-state. - -```sh -PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') - -# 1. Formal review - exact SHA match. -gh pr view --json reviews --jq \ - '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .commit.oid' \ - | grep -q "$PR_HEAD" && echo "covered via formal review" - -# 2. Issue comment - show the most recent Copilot comment for manual -# confirmation. This is the REST API, so the login carries the `[bot]` suffix. -gh api repos///issues//comments --jq \ - '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' -``` - -Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. The exception is the refusal above, which is a formal review on the head with no inline comments and covers nothing, so path (1) exits 0 over a round that never ran. Read the body of the review the SHA matched, not only the SHA. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. - -**Coverage of the head is not coverage of the diff, and the second one is stated in a line nothing above reads.** A review body says how many of the pull request's changed files it read, and a round that read fewer than the pull request changed is byte for byte the clean pass in everything else: the same `commit.oid`, the same absent threads, the same "generated no comments". Measured over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed, and all three merged. One of them changed three files, left one unread across **both** its rounds, and reported no comments each time. This is the third instance of the shape the refusal above and the suppressed block below are the first two, so read it the same way: **fail closed on a wording you do not recognize**, since a gate that allows whatever it does not recognize stops gating as the wording drifts, and both of those wordings have drifted once already. - -Two spellings carry the count, and both are current rather than one superseding the other. Each opens its own line, which is what separates the round stating its coverage from prose mentioning changed files, that prose being what a review of a change to this rule looks like: - -```text -Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments. -- **Files reviewed:** 2/3 changed files -``` +For every review: -The sentence tail after the first spelling reports how many comments the round raised and appears in four wordings. It is not coverage, so it is not part of what has to be recognized, and the counts are. Read them into three verdicts and two exemptions: +1. Read the full pull request diff and count its changed files. +2. Follow `.github/skills/code-review/SKILL.md` and every skill it selects. +3. Publish every supported finding. Never suppress a finding or place it in a low-confidence or + hidden findings block. +4. Use an inline comment when a changed line can anchor the finding. Use the review body only when + no valid inline anchor exists. +5. End the review body with the exact machine-readable marker required by the `code-review` skill. -- **Counts equal** - the round read the whole diff. This is the clean pass. -- **Counts unequal** - files in the diff have no review at all. Do **not** treat a re-request as the remedy: measured over four pull requests and seven rounds on this repository, every partial round stayed partial at the identical ratio and no round ever recovered, so re-requesting spends a round and changes nothing. Splitting works where it applies and does not apply to a promotion, whose head is `develop`. **The file table in the body does not tell you which file went unread**, and it looks as though it should, which is why it is written down here: measured over 348 review bodies on this repository and 121 on another in this fleet, that table names the whole changed set on partial and fully covered rounds alike, so a table naming every changed file is what a full round carries too and contradicts nothing. One round of the seven is the exception, stating 16 of 17 and naming 16, omitting `GOVERNANCE.md`, and `status` names an omitted file only in that shape, where the table is short by exactly what the counts leave unread and names nothing outside the diff. Treat that as a lead to check rather than a verdict, one round here naming `GOVENANCE.md`, the reviewer's own spelling and a path no diff carries. Report the state and hand the merge decision to the maintainer. -- **Coverage-shaped and unreadable** - the remedy is to fix the reader, not to read past it. The vetted spellings live in `scripts/pr_review.py` and here, and they stay in step because a case reads them out of this file. -- **Exempt: a body stating no coverage at all.** 28 of those 332 bodies are an overview and a change list and nothing more. That shape is current, interleaves with the counted one throughout, and one pull request carries both across its two rounds, so treating it as a failure cries wolf on about one review in twelve and teaches an agent to work around the gate. It reads as `coverage=unstated`, never as a pass and never as a failure. -- **Exempt: a refusal.** It carries no coverage line by design, and the refusal rule above has already classified it. Read it here as well and every refusal grows a spurious second failure on top of the one that names its remedy. +The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, +and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review +identifiers by hand. Its status gate verifies the current head, diff coverage, output shape, +inline threads, body-only findings, and required checks. -`scripts/pr_review.py status --repo /` reports this as `coverage=full`, `coverage=PARTIAL`, `coverage=UNVETTED` or `coverage=unstated`, and exits `42` on a partial round. An unreadable wording exits `43` instead, as one of the unrecognized shapes below rather than as a case of its own, since both say the reader is what needs fixing. Read it by hand as: - -```sh -gh pr view --json reviews --jq \ - '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .body - | split("\n")[] | select(test("^(Copilot|[-*] \\*\\*Files reviewed:).*changed files?"))' -``` - -### A Shape Nothing Recognizes Blocks the Loop and Earns an Issue - -**Every rule above keys on a marker in what Copilot sent, so a marker that changes spelling is a section the reader stops finding and reports as absent.** That is not a hypothetical: all three failures on record here have exactly that shape. The suppressed heading was reworded and the count went to zero. The suppressed section moved inside another wrapper and the count went to zero again. The coverage line was never read at all. Each one reported a clean pass over a review it had misread, and each was caught by the maintainer after it had already landed, rather than by the gate. - -**So an unrecognized shape is a blocking outcome, and its remedy is an issue rather than a judgment call.** When any reader here meets a heading, a collapsed section, a metadata line, a coverage wording or a reviewer login it has no vetted spelling for, the review loop **does not close**, whatever else the digest says. Do not read past it, do not infer what the new wording probably means, and do not treat a body that looks clean as a clean review, because "looks clean" is precisely what a misread review looks like. Two things follow, in this order: - -1. **File an issue on the hub**, the repository `AGENTS.md` "Fleet Bootstrap" names, which hosts `scripts/pr_review.py` and holds the vetted inventory. Name each unrecognized shape and quote the review body it came from, so the fix is made against the real wording rather than a paraphrase. The issue is filed even when the shape turns out to be cosmetic, since "cosmetic" is a conclusion drawn after reading the body and not before. -2. **The merge decision is the maintainer's**, not the agent's and not the script's. An unrecognized shape does not mean the pull request is bad, it means nothing here can vouch for the review of it. Report the state, hand it over, and stop. - -`scripts/pr_review.py status --repo /` reports this as `shapes=UNRECOGNIZED`, lists each shape under a marker naming the remedy, and exits `43`. `wait` carries the same code, so a wait cannot end on a clean zero over output nothing read. The vetted inventory lives in that script and is small on purpose: measured over 332 Copilot review bodies on this repository, with fenced blocks dropped and text reduced to ASCII, the whole corpus is seven headings, six `` texts and three metadata labels, and every body carries at least one of them. A body carrying none is itself the unrecognized shape, which is what catches a rewrite that changes everything at once, the refusal wording drifting among it. - -### Bounded Retry Workflow - -This path is only for a **genuinely missing** review, meaning no Copilot review (formal *or* issue comment) covers the current head SHA after polling. A review that covered the head but produced no comments is a clean pass, not a missing review, so do not enter this retry path for it. - -**A slow review is pending, not missing, so poll with backoff and never escalate on a timeout alone.** Copilot can lag far beyond the usual one-to-three minutes when it has been re-requested many times in quick succession, because it throttles under load, and a re-review landing tens of minutes after the request is normal. A poll that times out is therefore evidence only that the review has not landed *yet*, not that Copilot is done or unresponsive. Report the status as "review still pending" and keep polling on a widening interval (for example 20s steps, then a few minutes) rather than stopping. Enter the escalation step below only when the `requestReviews` mutation itself no-ops or errors, or after a genuinely long wait with the request confirmed accepted, never merely because one fixed poll window elapsed. - -**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through. - -**A pending request nothing picked up is a third state, and it is the one that looks most like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting a request, and submits its review a few minutes later. A request that never draws one is not a slow review, it is a request nothing is acting on, and it stays that way indefinitely: one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot tell the two apart, since a genuinely slow round also shows no review, so read the event rather than the clock. `copilot_work_started` appears in the REST timeline only, and no GraphQL timeline item carries it: - -```sh -# The pending set (GraphQL, since the `gh pr view` projection cannot see a Bot reviewer). -gh api graphql -f query=' -{ repository(owner:"",name:""){ pullRequest(number:){ - reviewRequests(first:10){ totalCount - nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } } }' - -# The request and pickup events, newest last. A `review_requested` with no later -# `copilot_work_started` is the stuck state. Requests are filtered to the reviewer's own, -# since a human requested afterwards is a different request and reading it as this one -# reports a picked-up review as never picked up. `per_page` is the pagination cost. -gh api --paginate 'repos///issues//timeline?per_page=100' \ - --jq '.[] | select(.event == "copilot_work_started" or (.event == "review_requested" - and .requested_reviewer.type == "Bot" - and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot")))) - | "\(.event) \(.created_at)"' -``` - -**Recover it by clearing the request and requesting again**, because the pull request UI offers no re-request control while a request is pending, and `requestReviews` with `union: true` adds a reviewer already in the set, which changes nothing. Read the pending set first, since `union: false` replaces the whole set and would drop a human reviewer requested alongside the bot. Where the clear-and-request does not draw a `copilot_work_started` within a minute or so, push a commit instead, since a new head raises a fresh request rather than poking a stale one. - -```sh -PR_NODE=$(gh pr view --json id --jq '.id') -# 1. Clear. `union: false` replaces the set, so an empty botIds removes the pending request. -gh api graphql -f query=' -mutation($pr: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [], union: false }) { - pullRequest { reviewRequests(first: 10) { totalCount } } } -}' -F pr="$PR_NODE" -# 2. Request again, against a now-empty set, with $BOT_ID read as in "Triggering and Polling". -gh api graphql -f query=' -mutation($pr: ID!, $bot: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { - pullRequest { reviewRequests(first: 10) { totalCount } } } -}' -F pr="$PR_NODE" -F bot="$BOT_ID" -``` - -If a review did not run on the current head, retry: - -1. Wait briefly and check head-SHA coverage (see above). -1. Re-request the review via the `requestReviews` mutation (see "Triggering and Polling"), falling back to the GitHub PR UI only if the mutation no-ops. -1. Retry up to two more times (three total). -1. If still missing, mark review as blocked and escalate to the user/maintainer with what was attempted. - -### Reply and Thread Resolution Workflow - -Every id below is captured from a live query into a variable and passed from there, never hand-typed, guessed, or pasted as a `PRRT_...` literal. A node id resolves globally, so a fabricated or stale id does not fail, it writes to a real thread on an unrelated repository. This runbook implements [GOVERNANCE.md "Repository Boundaries and Write Safety"](../GOVERNANCE.md#repository-boundaries-and-write-safety): write only to this repo, capture every id from a live query, and never suppress a mutation's output. - -**Use the hub's helper, which has nowhere to type an id.** `scripts/pr_review.py reply --repo / --match "" --body "" --resolve` queries the thread id itself and passes it straight to the mutation. That rule is known and read by the agents that break it anyway, three times so far, so the shape is what changes rather than the wording. It selects on the finding's own words rather than a line number, since a fix push moves the line; it refuses on no match and on more than one rather than picking; and it does not resolve a thread whose reply came back without a `url`. Cross-owner targets it refuses outright, which is where the hand-run form below applies, and there the `gh-write-guard` hook is what reads the maintainer's grant. It is hub-hosted per [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling), so it is invoked from a hub checkout and never rebuilt locally. - -The hand-run form is below, for a cross-owner target and for the case where the hub cannot be reached and the work cannot wait. - -List unresolved threads. Use `first: 100` with cursor-based pagination, and where `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: - -```sh -gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequest(number: ) { - reviewThreads(first: 100) { - nodes { - id isResolved path - comments(first: 1) { nodes { author { login } body } } - } - pageInfo { hasNextPage endCursor } - } - } - } -}' | jq ' - .data.repository.pullRequest.reviewThreads | - (.pageInfo | "hasNextPage=\(.hasNextPage) endCursor=\(.endCursor)"), - (.nodes[] | select(.isResolved == false)) -' -``` - -Reply on a thread, then resolve it. Capture the target thread's id into `$TID` from the listing query above, filtering to the thread being answered by its `path`, and guard for an empty result so a mutation never runs on a guessed id. When a file carries more than one unresolved thread, `path` alone is ambiguous and `head -n 1` would pick the wrong one, so narrow by first-comment body (the query already fetches `comments(first: 1)` for this) by adding `and (.comments.nodes[0].body | contains(""))` to the `select`: - -```sh -TID=$(gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequest(number: ) { - reviewThreads(first: 100) { - nodes { id isResolved path comments(first: 1) { nodes { body } } } - } - } - } -}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] - | select(.isResolved == false and .path == "") - | .id' | head -n 1) -[ -n "$TID" ] || { echo "no matching unresolved thread on - do not guess an id" >&2; return 1 2>/dev/null || exit 1; } - -# Show the mutation's output. Never append an output-discard or force-success tail -# (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo) to a write. -gh api graphql -f query=' -mutation($threadId: ID!, $body: String!) { - addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { - comment { id url } - } -}' -F threadId="$TID" -F body="Fixed in : ." - -# Confirm isResolved: true in this response before treating the thread as closed - a write that -# appears to fail may have taken on the server. -gh api graphql -f query=' -mutation($threadId: ID!) { - resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } -}' -F threadId="$TID" -``` - -Issue-level Copilot comments (those in `issues//comments`) have no resolution action, since GitHub provides no API or UI to resolve them. Reply if the finding warrants it, but no resolution step is needed or possible. - -### PR Edits and Merge-State Gotchas - -- **`gh pr edit --title/--body` is broken on `gh` 2.45.x and 2.46.x, and works from 2.47 up.** Those releases touch the deprecated Projects-classic `projectCards` GraphQL field and **exit non-zero without applying the change** (a stale PR description then survives review rounds), which the GitHub CLI maintainers name as broken by deprecated APIs. A distribution package is where that version comes from, so check `gh --version` before concluding the command is unusable, and install from the official repository rather than working around it. Where a host is genuinely stuck on one, edit via the API and verify it took: GraphQL `updatePullRequest(input: { pullRequestId, title, body })`, or REST `gh api -X PATCH repos///pulls/ -F body=@body.md` (the `@` reads the body from a file, so name it explicitly, not the literal `file`). The same version range carries no `--json` flag on `gh pr checks`, so a watcher built on it prints nothing and a quiet result reads as a passing one. -- **`main`/`develop` use rulesets, not classic branch protection.** The classic protection REST endpoint (`repos/.../branches//protection`) 404s, so read the ruleset instead. A `mergeStateStatus` of `BLOCKED` on a green PR is most often just **unresolved review threads** (the ruleset requires thread resolution), and resolving them moves it to `CLEAN`. (`BLOCKED` is a `mergeStateStatus` value, so don't confuse it with the separate `mergeable` field's `MERGEABLE`/`CONFLICTING`, which reports merge conflicts, not review gates.) -- **`BLOCKED` never says which gate, so never infer one.** The same word covers a red check, a required check nothing is running, an unresolved thread, and a missing approval, and the bullet above says "most often" rather than "always" for that reason. Read the checks instead of guessing: `pr_review.py status` prints `checks=N/M` beside the merge word and names a stuck one, and it exits `44` from `wait` where the merge reads `BLOCKED`, the review loop closed, and a check is starved, expected and never posted, running far past what the job costs, or failed. A **queued check with no runner** is the case that reads exactly like patience: a run here polled `BLOCKED` for twenty-five minutes on a pull request whose only unfinished check was an aggregator job GitHub dispatched and never assigned a runner, and the cause came from the maintainer rather than from any field. Nothing agent-side starts that job, because the pool is GitHub-hosted, so the remedy is a re-run of the workflow or waiting on that capacity, and it is **not** a re-request, a rebase, or an empty commit. A job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so a queued check is never a dependency waiting its turn. -- **Push -> head-SHA read race.** A `headRefOid` read taken immediately after a push can return the **old** head, so re-read after the push registers, or a coverage poll evaluates the stale SHA. -- **Copilot is sometimes factually wrong** (e.g. it claimed `actionlint -color` "requires a value" when it is a boolean flag). Verify a finding before fixing, and decline with evidence when it is wrong, which is distinct from dismissing a still-present finding as stale. The evidence goes under [Disproved Claims](#disproved-claims) as well as in the thread, because the thread closes with the pull request and the next round starts without it. - -Reply-body conventions: - -- Accepted bug/style fix: include fixing commit SHA and a one-line summary. -- Declined style comment: cite the rule (GOVERNANCE.md or the CODESTYLE.md language section) and the existing-tree precedent. -- Declined architecture proposal: one-sentence rationale. -- Declined false positive on carried fleet content (a broken-link or dead-cross-reference flag inside byte-locked rule text): cite the "Reviewing Carried Fleet Content" section, since the reference is intentional and the text cannot be edited locally. - -After the final push, sweep-resolve stale older threads for removed code paths. +A formal review with no findings is complete only when it covers the current head and states full +diff coverage. A refusal, partial or absent coverage statement, unrecognized output shape, +unresolved thread, or body-only finding blocks the review loop. Re-run the loop after every fix +push. Never infer review completion from `mergeStateStatus: CLEAN`. ### Disproved Claims @@ -363,6 +95,5 @@ After the final push, sweep-resolve stale older threads for removed code paths. ## When in Doubt -Read [AGENTS.md](../AGENTS.md) to find the section that governs your change, and [GOVERNANCE.md](../GOVERNANCE.md) for the rule text itself. For code-style rules, [`CODESTYLE.md`](../CODESTYLE.md) (its General section plus the relevant language section) is authoritative. Don't restate any of these files' rules in commit bodies or PR descriptions, and keep those focused on the change itself. - -If you find a gap in the governance itself (this file, AGENTS.md, or GOVERNANCE.md is out of date, a rule is missing, something bit this repo and would bite the next), fix it in the governance docs as part of your change rather than only working around it locally. +Stop and report the uncertainty. Do not guess at an instruction, suppress a possible finding, or +claim coverage that the review did not perform. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md new file mode 100644 index 00000000..7d189ca9 --- /dev/null +++ b/.github/skills/agent-conduct/SKILL.md @@ -0,0 +1,44 @@ +--- +name: agent-conduct +description: >- + Surfaces the ptr727/ProjectTemplate fleet's conduct rules at the three decision moments they are violated: about to claim work is done, verified, green, or fixed, about to proceed on an assumption the user could cheaply confirm, and a failure or review finding just surfaced a durable lesson. Use this whenever about to report success or completion of any task, whenever about to pick a default, guess an intent, or resolve an ambiguity without asking, whenever work is blocked on a decision or authorization only the user can give, and whenever an incident, a wrong answer, or a repeated correction just taught something a future session must honor. Deliberately narrow: the carried AGENTS.md sections are the always-on layer, and this skill fires at the moments rather than duplicating them, so do not load it as general background. Where a sibling skill owns the moment, it wins: git-commit-conventions for committing, pr-review-conduct for review and merge claims, comment-and-doc-style for prose. The GOVERNANCE.md sections this skill summarizes keep the full rules. +--- + +# Agent Conduct + +## Why This Exists + +The fleet's conduct rules (verification before claiming done, asking instead of assuming, recording lessons) lived only in doc sections nothing surfaced at the moment of violation, so they were honored by whoever happened to have read them recently. This skill is the decision-moment surface. The full rules stay in `GOVERNANCE.md` ("Verification Discipline", "Communicating with the User", "Durable Knowledge and Self-Improvement"), which keeps authority, and in the carried `AGENTS.md` "Context and Delegation Discipline" section, which is the always-on layer. + +## Before Claiming Done + +Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anything non-trivial. Its unifying property: every failure it lists is green. The checks that bind here: + +- **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in an aggregated required check, so confirm from the log that the job ran and produced what it promises. +- **Locate every check the change owes before running any**, from what the repository declares (`OPERATIONS.md` "Local Verification" beside the workflows), not from what the pipeline happens to run, since part of a contract is routinely unreachable from a runner and green is then the precise signal it was skipped. +- **Run the repo's whole lint gate before every push**, not the parts that look relevant, because the tool most likely to catch a change is often the one it seems least about. +- **A launched process is not a result.** Report the output the wait produced, and where it produced none, that absence is the report. Never name an external cause the record does not carry. +- **A local clone is not the branch it names.** Fetch immediately before reading, or read the live ref, and name the ref and commit in any finding a local read produced. +- **A "does not exist" claim names the branch it was checked against.** A worktree's default branch is not necessarily the one the content lives on: in-flight content on a `release`-model repo lands on `develop` before `main`, per `GOVERNANCE.md` "Branching Model," so check that branch before reporting anything absent repo-wide. +- **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. +- **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. + +Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. + +## Before Assuming + +- **Ask when the user can cheaply confirm.** An assumption that saves one question and is wrong costs the rework plus the trust, so a genuine ambiguity in intent, scope, or authorization is raised, not resolved by picking the likelier reading. Rules that already answer the question (the committed instruction set) are not ambiguity, so read them first rather than asking what they state. +- **Raise blocked work as a direct interactive prompt** at the point the work stops, per `GOVERNANCE.md` "Communicating with the User": the blocked item is the message, the options offered are the actions themselves, and a handoff buried in a summary paragraph is a handoff that did not happen. Numbered lists are the fallback where no prompt mechanism exists. +- **References are clickable where they are read**: a pull request, issue, or commit on a Markdown surface is a Markdown link, and on a surface that renders neither, a bare `#123` with the link in the message before the prompt. +- **Capability is not permission.** A token's reach, a tool that happens to work, or a similar grant in a past session authorizes nothing, and the irreversible step (merge, publish, release, delete) stays the maintainer's. + +## When a Failure Surfaces a Lesson + +- **Durable knowledge lands in the committed docs, not in agent memory**, as part of the change that surfaced it, per `GOVERNANCE.md` "Durable Knowledge and Self-Improvement". Memory does not survive a new session or machine, so it holds only environment nuance and in-flight state. +- **Where the governing doc is carried from the hub, propose the fix upstream** rather than only patching it locally, since a local fix leaves every sibling repo with the same trap. +- **A review flags an instance, so fix the class**: sweep for the siblings before replying, because reviewers sample rather than enumerate. +- **A rule that keeps needing restating** is usually a stale or missing skills install, so run `python3 scripts/skills_install.py --report` from a hub checkout (the `fleet-conformance-check` skill) before concluding the rule does not exist. + +## Delegation, in One Paragraph + +The always-on rules live in `AGENTS.md` "Context and Delegation Discipline" and are not restated here. The two that intersect conduct: brief a subagent so it never needs a governance file, since anything it must honor has to be in its prompt, and never tier down the seat holding the judgment, because governance wording and the decision to decline a review finding are fleet-wide and durable when wrong. diff --git a/.github/skills/audit-a-repo/SKILL.md b/.github/skills/audit-a-repo/SKILL.md new file mode 100644 index 00000000..01ee0b2b --- /dev/null +++ b/.github/skills/audit-a-repo/SKILL.md @@ -0,0 +1,37 @@ +--- +name: audit-a-repo +description: >- + Drives AUDIT.md's read-only measurement of a named ptr727 fleet repo against the fleet ground truth, ending in a committed report, never an edit to the repo being measured. Use this whenever asked to audit, measure, or verify conformance of a named repo, to judge a conformance claim someone else made, or to decide whether an onboarding is actually complete. Run from a hub checkout of ptr727/ProjectTemplate against the named target. Triggers even when the repo believes it is conformant, because conformance asserted without a committed report is conformance nobody can check, and that is the case most often skipped. This completes the procedure triangle: standup-a-repo creates a repo, resync-a-repo applies findings to one already stood up, and this skill measures, while fleet-conformance-check is the in-repo self-check with no named target and no standing hub checkout. AUDIT.md keeps authority over the procedure, this skill is the summary that routes into it. +--- + +# Audit a Repo + +## Why This Exists + +The audit is the fleet's measurement procedure, and the two failure shapes it guards against are both silent: a repo judged conformant with no committed evidence, and an audit that quietly edits what it was supposed to measure. `AUDIT.md` in the hub is the procedure and keeps authority. This skill carries the rules that get skipped in practice and says which section owns each step. + +## Before Measuring Anything + +- **Route first.** A repo with no carried instruction set, or a partial one, has a baseline that never arrived rather than drift to report, so it goes to `STANDUP.md` sections 1A and 2 first (`AUDIT.md` section 0). Auditing it anyway produces a report that is all absences and reads as catastrophe. +- **Verify the host.** Run `python3 scripts/host_gate.py --repo ` from the hub checkout before any hub tool, and pass `--repo`, since a bare run skips the target's own `host-tools.json` overlay. A stale tool answers `--version`, looks healthy, and produces a wrong answer. +- **Read `main` as ground truth**, for both workflow models, and read `develop` only to detect divergence (`AUDIT.md` section 1). An `operational` repo's `develop` is mid-flight by design, so conformance work sitting there is un-promoted work, not a defect, and it counts when it reaches `main`. Use `spec/audit.py --branch ` to preview in-flight work, which stamps the override so the finding cannot be mistaken for one against ground truth. + +## Measuring + +- **Resolve the repo's types from `registry/repos.json`** and classify a `classificationPending` entry from the tree (`AUDIT.md` section 2). The applicability gate is `WORKFLOW.md` section 1: a check governing an absent construct is N/A, excluded from the verdict, and never a defect (`AUDIT.md` section 3). +- **Know what the runner does and does not prove.** `spec/audit.py` mechanizes the deterministic subset only: settings, rulesets, secret names, file and section presence, verbatim hashing, interface wiring, Dependabot coverage, branch facts. It evaluates no check under a type in `spec/project-types.json`, so every per-type check is judged by hand, and a clean run is no evidence for them (`AUDIT.md` section 4). Silence from a tool that was never looking reads exactly like a pass. +- **Judge letter and intent per check** and keep the vocabulary: letter miss with intent satisfied is a drift finding, both missing is a defect, and operational is binary over the applicable set (`AUDIT.md` sections 4 and 7). Do not invent a parallel scheme. +- **Assert the Actions implement `WORKFLOW.md`** by outcome, not by matching catalog snippets byte for byte: the 5A static audit with a `file:line` citation per applicable guarantee, then the 5B trace scenarios (`AUDIT.md` section 5). The `workflow-ci-contract` skill summarizes that contract. +- **Diff live settings, rulesets, and secrets** against `repo-config/` with the normalized comparisons `AUDIT.md` section 6 gives, using its commands rather than hand-rolled ones, since the normalization details (rule sorting, `bypass_actors` deliberately excluded, the operational develop payload) are where hand-rolled diffs go wrong. + +## Reporting + +- **Write `reports//audit.md` from `reports/_template.md`**, findings ranked most severe first, each with the `file:line` it was judged against, and quote the run stamp, since findings are a point-in-time snapshot (`AUDIT.md` section 8). +- **The hub authors the report.** A downstream repo never opens a hub pull request to write its own, which would be self-certification. Downstream context goes into issues filed against the hub instead. +- **Generate a convergence issue, never compose one**: `spec/audit.py --issue ` emits it from live findings. An agent picking such an issue up re-runs the audit first and acts on the live result, not the pasted findings. +- **Reconcile registry `driftNotes` in the same pass**: a resolved deviation's note is deleted, not left describing finished work, and a note naming a check id is retired by a person, not by a run (`AUDIT.md` section 8). +- **Stale-versus-modified classification needs a full hub clone with git history.** Without one, compare against the current hub canonical on `main`, which decides current-match only. + +## After the Report + +Measuring and fixing are separate phases. Converging is `AUDIT.md` section 10: fixes ship as pull requests on the target repo, one focused pull request per drift class, the Copilot loop driven to green per the `pr-review-conduct` skill, and the maintainer merges. For a repo already stood up, `RESYNC.md` sequences the findings, since order matters (a deletion lands before the re-vendor that would refresh it). Systemic drift shared by many repos is fixed in the hub spec, not hand-patched per repo, and spec questions are escalated rather than resolved silently (`AUDIT.md` section 9). diff --git a/.github/skills/carried-instruction-file-guard/SKILL.md b/.github/skills/carried-instruction-file-guard/SKILL.md new file mode 100644 index 00000000..47f88033 --- /dev/null +++ b/.github/skills/carried-instruction-file-guard/SKILL.md @@ -0,0 +1,31 @@ +--- +name: carried-instruction-file-guard +description: >- + Stops a blind overwrite of a downstream repo's AGENTS.md, GOVERNANCE.md, CODESTYLE.md, or WORKFLOW.md when resyncing or updating it to match the ptr727/ProjectTemplate hub template. Use this whenever about to edit, replace, re-vendor, or sync-to-match-the-hub any of those four files in a repository that is not ProjectTemplate itself, or whenever asked to bring a repo's instruction set up to date, run a conformance sweep, or fix drift against the hub. Triggers even when the request sounds routine, such as copying the hub's AGENTS.md over or resyncing a repo's docs, because that phrasing is exactly how a real incident happened, where a downstream repo's local rules were silently deleted by a full-file overwrite. Do not skip this just because the task looks mechanical. +--- + +# Carried Instruction File Guard + +## Why this exists + +A downstream repo's `AGENTS.md`/`GOVERNANCE.md`/`CODESTYLE.md`/`WORKFLOW.md` can hold two different kinds of content mixed in one file: sections that are stale copies of the hub's fleet-wide rules, and local rules the repo wrote for a fault the fleet has never seen elsewhere. Re-vendoring the hub's canonical version over the whole file deletes the second kind silently, because nothing about the diff looks wrong. This has actually happened: a resync replaced a repo's `AGENTS.md` wholesale with the hub's, and the repo's own local additions were gone with no error, no warning, and no review comment calling it out. + +The fix is not "be careful." Being careful is what failed the first time. The fix is a mechanical check you run before any overwrite touches one of these four files, every time, regardless of how routine the request sounds. + +## Before you touch any of these four files + +1. **Check whether the file's content is declared `verbatim` or `intent`.** The hub's `spec/section-model.md` (fetch it from a hub checkout, `github.com/ptr727/ProjectTemplate`, if you don't have one) names, section by section, which parts of `AGENTS.md` and `GOVERNANCE.md` are universal fleet law (safe to byte-match against the hub) and which describe the repo itself (never safe to overwrite from another repo). `CODESTYLE.md` and `WORKFLOW.md` are carried whole at `intent` fidelity, judged by meaning, not hashed. +2. **If any part of the file is `intent`, or if the file predates a clean split into hub-governed sections, do not diff-and-replace. Probe instead.** For each rule or paragraph in the current file that is not obviously boilerplate: + - Pick the phrase in it that is most peculiar to this repo, not generic governance vocabulary. A rule about "always sign commits" is generic. A rule about "this repo's Docker image pins Alpine 3.19 because 3.20 broke the s6 supervisor" is peculiar. + - Grep the hub's canonical copy of the same file for that peculiar phrase. + - **Absent from the hub canonical means it is a local addition.** It is never dropped because it looks similar to something else, and never dropped because a merge or overwrite would be simpler without it. +3. **A local addition found by the probe gets a destination, not a deletion.** Either it names a rule that should apply fleet-wide (flag it for the maintainer to promote into the hub), or it is genuinely specific to this repo and moves to the repo's own topical doc before the carried file is touched: `CODESTYLE.md` for a language/formatting convention, `ARCHITECTURE.md` for a design decision, `OPERATIONS.md` for a runbook or operational note, `TODO.md` for backlog. Move it, confirm it is not lost, and only then proceed with the carry. +4. **Do not trust a similarity or word-overlap check for step 2.** A repo-specific rule written in ordinary governance language reads as a reworded duplicate of an unrelated hub rule to that kind of check, and it will confidently tell you the local content is redundant when it is not. Exact phrase presence or absence is the only check that has held up. + +## What is actually safe to overwrite without this procedure + +A section `spec/section-model.md` names as `verbatim`, in a file that is already cleanly split (the file carries only that declared section, nothing else mixed in), can be re-vendored directly: byte-matching it against the hub canonical is the point of `verbatim` fidelity, and the audit already checks it that way. The guard above is for everything else: `intent`-fidelity content, a file that has not been split yet, or any file you are not certain is clean. + +## If you are not sure which case you are in + +Stop and say so, rather than guessing. Naming the uncertainty costs one sentence. Silently overwriting the wrong thing costs someone's local rules with no way to notice until much later. diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md new file mode 100644 index 00000000..af775184 --- /dev/null +++ b/.github/skills/code-review/SKILL.md @@ -0,0 +1,71 @@ +--- +name: code-review +description: >- + Reviews a pull request or change set against the repository's contracts, with explicit diff + coverage and no suppressed findings. Use this whenever asked to review code, a pull request, + a patch, or a proposed change, and whenever GitHub Copilot performs code review. Triggers even + when the diff is documentation-only or workflow-only, because the review must load the + applicable general, language, documentation, and workflow skills before judging the change. +--- + +# Code Review + +## Establish the Contract + +1. Read the root `AGENTS.md` and the sections it routes to for the changed paths. +2. Read the complete diff and enumerate every changed file before forming findings. +3. Load every applicable sibling skill from the current skill distribution: + - `comment-and-doc-style` for Markdown, prose, comments, commit messages, and PR titles. + - `dotnet-codestyle` for C# and .NET changes. + - `python-codestyle` for Python changes. + - `shell-codestyle` for shell changes. + - `workflow-ci-contract` for GitHub Actions and CI/CD changes. +4. Treat a missing executable on `PATH` as no evidence that its check is unavailable. Read the + repository's documented local invocation before reporting a check as skipped. + +Do not substitute a familiar convention for the repository's written contract. Report a +conflict between instructions instead of silently choosing one. + +## Review the Change + +Review for correctness, regressions, security, compatibility, error handling, concurrency, +resource lifetime, tests, and contract drift. Follow data and control flow beyond the edited +lines when the behavior depends on unchanged callers or consumers. + +For each candidate finding: + +1. Verify it against the current head tree, not an unfetched checkout or the base branch. +2. Identify the concrete failing behavior and the conditions that reach it. +3. Confirm that the repository does not already prevent it elsewhere. +4. Prefer one root-cause finding over several symptoms of the same defect. +5. Omit pure preferences that no repository rule or user-visible risk supports. + +Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one +downstream repository does not carry is not a broken link. A substantive defect in canonical +content remains a finding, with the fix located at its canonical source. + +## Publish Every Finding + +Never suppress or hide a finding because confidence is low. Investigate until it is supported +or discard it. Publish every supported finding as an inline review comment when a changed line +can anchor it. Use the review body only when no valid inline anchor exists. + +Each finding states: + +- A concise imperative title with a severity. +- The file and smallest useful line range. +- The behavior that fails and the input or state that triggers it. +- Why the change causes the failure. +- A bounded direction for the fix when one is known. + +Do not report a clean review until every changed file has been read. End the review body with +exactly one ASCII marker, replacing the numbers with measured counts: + +```text + +``` + +`reviewed` is the number of changed files actually reviewed. `changed` is the total number of +changed files. `findings` is the number of published findings, including body-only findings. +Never emit `reviewed=changed` as a placeholder. If full coverage is impossible, emit the actual +counts and explain the limitation in the review body. diff --git a/.github/skills/comment-and-doc-style/SKILL.md b/.github/skills/comment-and-doc-style/SKILL.md new file mode 100644 index 00000000..dc834f25 --- /dev/null +++ b/.github/skills/comment-and-doc-style/SKILL.md @@ -0,0 +1,231 @@ +--- +name: comment-and-doc-style +description: >- + Governs prose, comment, Markdown, character-set, line-ending, and PR-title/commit-message + conventions for every ptr727/ProjectTemplate fleet repo. Use this whenever writing or editing a + code comment, workflow comment, Markdown doc, commit message, or PR title, whenever choosing + which characters to type in agent-authored text, whenever the file being edited is CRLF, and + whenever naming a tool in prose or docs. Triggers even when the task looks purely mechanical, + such as "just fix a typo" or "add a one-line comment", because the fleet's ASCII character-set + tiers, no-semicolon rule, comment-growth discipline, and CRLF-preservation rule are each easy to + violate without noticing: an em dash slipped into a sentence, a comment that grew by one more + clause, or a text-mode edit that silently flattens a CRLF file to LF. Also triggers when + authoring a new Markdown file (reference-style links, Table of Contents, present tense), when a + carried instruction file (AGENTS.md, GOVERNANCE.md, CODESTYLE.md, WORKFLOW.md, + .github/copilot-instructions.md) is being edited (no coordination references to the template or + a sibling repo), and when writing a PR title or commit message (imperative subject, no vague + titles, no unsolicited Co-Authored-By, no release-bump magnitude). +--- + +# Comment and Doc Style + +## Why this exists + +These are the fleet's mechanical prose rules, kept in one place instead of re-derived per repo or +per session: how to write a comment, which characters an agent may type, how a Markdown file is +structured, how a carried instruction file may reference the hub, and how a PR title or commit +message reads. None of these are matters of taste. Each is checked, by `prose_lint.py`, +`editorconfig-checker`, `markdownlint`, `cspell`, or a human reviewer, and each has been the exact +subject of a real review finding. + +## Naming tools in prose + +Use each tool's official casing in task labels, docs, and prose: `.NET` (not `.Net`), +`CSharpier`, `ruff`, `pyright`, `uv`. Do not invent personal variants. + +## Markdown files: linting and spelling + +- **Markdown lints clean, repo-wide.** Every `.md` file is error and warning free via + `markdownlint-cli2` against the shared `.markdownlint-cli2.jsonc`. A rule it deliberately + disables (for example `MD013` line length) stays disabled, do not "fix" it. `MD033` inline HTML + stays enabled: HTML comments, and `details`/`summary` (no Markdown equivalent for a + collapsible), are allowed, everything else with a native Markdown equivalent uses the Markdown. +- **Spelling is US English**, checked by CSpell against the shared `cspell.json` + (`"language": "en-US"`, so a British spelling is flagged). Add a project term to `cspell.json`'s + `words` list, never to a `.code-workspace`'s own `cspell.words` block. +- **CI's spelling gate covers `README.md` and `HISTORY.md` only**, deliberately not every `.md` + file, so a new topical doc is not spell-gated in CI (the editor extension still flags it live). + A repo may widen its own CI list, README plus HISTORY is the default. A repo shipping no + `HISTORY.md` drops it from the CI workflow, the `Lint: Spelling` task, and the GOVERNANCE.md + cspell line together, all three or none. +- **`HISTORY.md` mirrors the README's opening**: the same `# `, the same tagline verbatim + (the first line after the README's H1), then its own `## Release History`. It never repeats a + paragraph below the README's tagline. +- **"Markdown" is a proper noun in prose** (a Markdown file, a Markdown-only repo), lowercase only + for what a machine reads: a tool or package name (`markdownlint`), a settings key, a heading + anchor, a file extension. + +## Markdown formatting + +- **Reference-style links everywhere**, except the four files read one section at a time rather + than end to end: `AGENTS.md`, `GOVERNANCE.md`, `OPERATIONS.md`, `.github/copilot-instructions.md`. + Those keep inline links so a target resolves where it is read. Every other Markdown file defines + every URI at the bottom, grouped by type under an HTML-comment header, each group alphabetized + by reference name rather than by the full definition line (a name that is a prefix of another + sorts first, `[governance]` above `[governance-branching-model]`). A URL inside a fenced code + block stays inline. See `references/markdown-links.md` for the full grouping and naming + convention. +- **Table of Contents**: generated by the Markdown All in One extension on save, never + hand-authored or hand-edited. Exclude a heading with an inline `<!-- omit from toc -->` marker. +- **One logical paragraph per line**, no hard-wrap line-length limit. For an intentional line + break within a block (stacked badges, status lines), end the line with a trailing backslash + rather than trailing whitespace. +- **Headings use the PR-title casing rule** below. +- **Write in the present tense.** State what *is*, never a change from a prior state ("X does Y", + not "X now does Y" or "X no longer does Z"). This applies to docs and code/workflow comments + alike. Before/after framing belongs in changelogs, commit messages, and PR descriptions, where + the prior state is the point. +- **When a behavior changes, grep for prose asserting the old one.** Comments, diagram labels, + workflow-input descriptions, and audit statements elsewhere may still describe the prior + behavior, and each was accurate when written. No linter catches a claim that is merely untrue, + so this sweep is the only mechanism that will. + +## Sentence structure + +The structural half of ASD-STE100 is the adopted house style for agent-authored prose, and the +controlled dictionary is deliberately not adopted: vocabulary stays unrestricted, structure is +restricted. Each structural rule a pattern can reach lands as a `prose_lint.py` check +incrementally, and this section names each check as it ships. + +- **Short sentences: at most 25 words in one sentence**, ASD-STE100's descriptive cap, checked by + the `sentence-length` rule in `prose_lint.py`. The check is opt-in like `sentence-split`, + because the existing corpus predates the cap and a default gate would fail whole files nobody + is editing. Write new prose under the cap, and scope a run to a change with + `--check sentence-length --diff <base>`. +- **One instruction per sentence.** A procedure step states one action, and a second action is a + second step. No pattern reaches this, so it is authoring discipline with no check. +- **Active voice, imperative mood for procedure steps.** Write "run the gate", never "the gate + should be run". Also authoring discipline, since a reliable passive-voice pattern does not + exist. + +## Comments + +Applies to code and workflow (`#`) comments alike. + +- Comment only when the code does not explain itself, or the logic is genuinely complex. + Self-evident code needs no comment. +- State only the non-obvious *why*, for the human reading *this* project's code now. No + cross-project references, no historic or design narrative, no rule citations. Governance lives + in the fleet's own instruction set, not echoed inline. +- **Keep it short**: one line is the default. A second line is earned only by a constraint the + code cannot otherwise carry. +- **Structured, not prose**: one sentence per line, never wrapped across lines, never a + multi-sentence run-on. A comment that genuinely needs several sentences is several lines, each + one sentence. +- A comment line opening prose starts with a capital. A trailing label, or the version pin an + action-pinning rule requires, does not. +- Mark a sub-topic with `-` after the comment marker (`# -`), only for genuine parallel sub-items + hanging off a lead line, never a continuation of one thought. +- **No file, class, or type header summary blocks.** A type or file gets a comment only for a + specific non-obvious point, never a block restating what it contains (a license or provenance + header a tool or policy requires is not a summary and is unaffected). +- **Never let a comment grow across edits.** Touching code near an existing comment means the + comment comes out the same length or shorter, never one more clause of rationale appended. + +A continuation stays unindented, one sentence per line: + +```text +# Change gate for the compile tests. +# An esp-idf build costs minutes, so gate on what each test covers. +# A diff that cannot be computed runs everything. +``` + +Sub-topics take a `-` after the comment marker, each elaborating a distinct item named in the lead: + +```text +# Source lint plus change-gated compile tests. +# - compile-test builds the external component. +# - template-compile-test builds one example device per template. +``` + +## Character set + +Agent-authored text is ASCII by default: documentation, code, comments, commit messages, and PR +descriptions. A non-ASCII character is read against three tiers, because whether one is +typography or meaning depends on where it sits. A character in no tier is a finding rather than a +silent pass. + +- **Tier 1, never legitimate.** Typography carrying no meaning its ASCII form loses. Remove on + sight: + - em dash (U+2014) and en dash (U+2013) to a restructured sentence, two sentences or a comma, + never a spaced hyphen + - right arrow (U+2192) to `->`, double arrow (U+21D2) to `=>` + - curly quotes (U+2018/U+2019/U+201C/U+201D) to straight `'` and `"` + - ellipsis (U+2026) to `...`, bullet (U+2022) to `-` + - no-break space (U+00A0) to a space, non-breaking hyphen (U+2011) to `-` +- **Tier 2, legitimate only next to a number.** Relational and arithmetic operators: U+2264, + U+2265, U+2260, U+00B1, U+2212, U+00D7, U+00F7, U+00B7. Keep one when an adjacent non-space token + is a number, a tier-3 symbol, or another tier-2 operator, so a threshold table or a measured + range reads as the range it is. In flowing prose write the ASCII form: `<=`, `>=`, `!=`, `+/-`, + `-`, `x`, `/`. A tier-2 operator directly before a number in a table of thresholds is the range + it describes and stays, the same character between two words in a sentence is prose and takes + the ASCII form. +- **Tier 3, always legitimate.** Scientific and unit symbols whose ASCII form would be a lie: + micro (U+00B5), degree (U+00B0), ohm (U+2126), pi (U+03C0), superscript two and three (U+00B2, + U+00B3), section (U+00A7). Keep the symbol, never approximate it away or spell it out. +- **Unicode a developer deliberately typed** stays regardless of tier, such as emoji used for + emphasis or as callout markers. Never strip a developer's own characters, this is developer + authored text and not a license for the agent to add its own. +- **An unrecognized non-ASCII character is reported, not allowed.** Classify it into a tier above + before using it. +- **No semicolon in agent-authored prose.** Recast a mid-sentence semicolon as a comma or as two + sentences. A semicolon separating items in a list that already contains commas, or a statement + terminator in code, is unaffected. +- **No spaced hyphen joining or interrupting a sentence** (` - `, or the paired aside ` - x - `). + Recast as a comma, two sentences, or parentheses. A hyphen inside a compound word, a leading + list marker, a range, and the `- **Label** - explanation` bullet separator are unaffected. +- **In carried verbatim content, fix the whole class at the hub**, not one instance, since a + downstream repo cannot edit a section byte-matched against the hub. Everywhere else, correct as + each file is next edited, not swept. + +## Line endings + +This repo's default is LF (`[*] end_of_line = lf` in `.editorconfig`), with CRLF pinned only for +`*.bat` and `*.cmd`, the one type Windows itself requires it for. +**Preserve a file's existing line ending when editing it, never reflow as a side effect of a +content change.** A text-mode tool, including a naive programmatic write, can silently flip CRLF +to LF and turn a one-line change into a whole-file diff. After any programmatic edit, verify with +`git diff --stat` (it should touch only the lines you changed) and a byte scan, `file` and a naive +`git ls-files --eol` are both unreliable here. Idempotent normalize: +`b.replace(b"\r\n", b"\n").replace(b"\n", b"\r\n")`. The full policy, choosing an ending for a new +file type, operational-repo overrides, extensionless-script pins, and auditing, is in +`references/line-endings.md`. + +## Carried files reference no coordination machinery + +`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, +the `repo-config/` and `spec/` files, and the carried `AUDIT.md` never reference the template repo +(in prose or a link), and never name a sibling fleet repo as an illustrative example. State the +behavior a carried rule needs, not the coordination flow that produced it, the maintainer supplies +the destination out of band. A contextually relevant link to a related project (the image this +config feeds, a library this depends on) is not a coordination reference and is expected. The full +exceptions, a verbatim section that must name the hub to do its job, and a pointer to a +hub-hosted tool the reader runs, are in `references/carried-doc-references.md`. + +## PR titles and commit messages + +- **Format**: an imperative subject, 72 characters or fewer, no trailing period ("Add 24-hour + PM2.5 average sensor", not "Added X" or "Adds X"). An optional body, blank-line separated, + explains *why* the change is being made when that is non-obvious, the diff already shows *what*. +- **Rules**: no vague titles (`update stuff`, `wip`). Dependabot's default `Bump X from Y to Z` + titles are fine as-is. No `Co-Authored-By:` lines unless the developer explicitly asks. No + release-bump magnitude in the title ("minor", "patch", "release v0.2.0"), Nerdbank.GitVersioning + computes the next version from `version.json` and git history, a dependency version in a + dependency-bump title is fine and expected. US English spelling, and title case with lowercase + short bind words (a, an, the, and, but, or, of, in, on, at, to, by, for, from), a hyphenated + compound capitalizes both parts unless the second is a short preposition (*Built-in*, + *EPA-Corrected*, *24-Hour*). + +```text +Add structured logging extensions to library +Pin softprops/action-gh-release to commit SHA +Drop net8.0 multi-targeting from console project +Bump xunit.v3 from 3.2.2 to 3.3.0 +Clarify devcontainer setup steps in README +``` + +## Quantitative claims + +A quantitative claim in `README.md` (a count, a size, a version floor, a supported-platform list) +is verified against current code before it is written. When a doc number is derived from a code +constant, mark the dependency in a source-code comment so the next editor knows to update both. diff --git a/.github/skills/comment-and-doc-style/references/carried-doc-references.md b/.github/skills/comment-and-doc-style/references/carried-doc-references.md new file mode 100644 index 00000000..972c63f5 --- /dev/null +++ b/.github/skills/comment-and-doc-style/references/carried-doc-references.md @@ -0,0 +1,61 @@ +# Carried Files Carry No Coordination References + +Full detail for the "Carried files reference no coordination machinery" rule in `SKILL.md`. Load +this when editing one of the carried files themselves, not when writing an ordinary repo-owned +doc. + +## Which files this governs + +`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, +the `repo-config/` and `spec/` files, and the carried `AUDIT.md`, the files the fleet carries +verbatim or at `intent` fidelity from the hub into every repo. This rule governs carried template +content only. A repo's own `README.md` and topical docs are its own content, never carried +verbatim, and this rule does not reach them. + +## What is banned + +Two things, in the files above: + +1. **Any reference to the template repo**, in prose or in a link. The coordination flow that + produced a carried file is machinery a consumer of that repo should never have to see, and + naming where a file came from is exactly the derived-from framing the present-tense rule (in + `SKILL.md`'s "Markdown formatting" section) independently forbids. Where a carried file must + express a template-level behavior ("report a rule discrepancy upstream"), state the behavior + rather than the destination. The maintainer supplies the destination out of band. +2. **A sibling fleet repo named as an illustrative example** ("repo X does it this way", "see repo + Y's adoption"), which couples the repos and rots as they diverge. To point at a current good + example, name it in the onboarding or conformance issue, never in a carried doc. + +## The two exceptions + +**The first exception is a verbatim section**, and `AGENTS.md` "Fleet Bootstrap" is why it exists. +That section's whole function is to name where the canonical rules live, for an agent in a +repository whose carried copies are stale, partial, or absent, which is exactly when no other file +present can say it. Its bytes are fixed fleet-wide, so a repository cannot edit the reference out +without failing the verbatim check instead, and a rule banning it would be unsatisfiable rather +than merely strict. The exception is scoped to the verbatim region and never leaks past it: the +same document's own prose, outside that region, is governed normally. A reference that reaches a +verbatim section is a defect in the canonical, fixed once at the source rather than reported +against every repository carrying it. + +**The second exception is a hub-hosted tool the reader is told to run**, which is a different kind +of reference. A rule naming a gate, a script, or a reference snippet the reader executes or copies +states an instruction rather than a provenance, and an instruction with no destination is +unfollowable, which is precisely how a pointer in carried text comes to read as decorative. The +test is whether the reference is something the reader *does* or something that *happened to this +file*: where the content came from stays out, what the reader runs stays in. Such a pointer names +the hub's canonical rather than this repository's provenance, so it is the hub's to keep resolving +and never a repository's to edit out or re-point at a local path. What is reached rather than +carried, and how, is `GOVERNANCE.md` "Hub-Hosted Tooling". In `AGENTS.md` and `GOVERNANCE.md` this +belongs in verbatim rule text, the same region the first exception already covers, so the whole +fleet reads one wording and no repository is asked to answer for a reference it did not write. + +## What is not a coordination reference + +**A contextually relevant link to a related project is expected, not banned.** Where another repo +is part of this repo's subject matter (the image that consumes this config, the builder that +generates this hardware, a library this depends on), link it normally. The test is whether the +link serves a reader of *this* repo's content, not whether the target happens to be in the fleet. + +This pairs with the present-tense rule: state the current shape, not a history of which repo it +came from. diff --git a/.github/skills/comment-and-doc-style/references/line-endings.md b/.github/skills/comment-and-doc-style/references/line-endings.md new file mode 100644 index 00000000..7f54f939 --- /dev/null +++ b/.github/skills/comment-and-doc-style/references/line-endings.md @@ -0,0 +1,135 @@ +# Line Ending Policy + +Full detail for the "Line endings" rule in `SKILL.md`. Load this when choosing an ending for a +new file type, working in an operational (config) repo, pinning an extensionless executable, or +auditing a repo's endings, not for an ordinary content edit to an existing file (the SKILL.md +summary, preserve the existing ending and verify with a byte scan, covers that case). + +## The defaults + +- **`.editorconfig` sets the line ending.** `[*] end_of_line = lf` is the default, every file + type is LF unless pinned otherwise, with CRLF pinned for the one exception Windows requires: + `*.bat` and `*.cmd` (cmd.exe's line handling is unreliable on LF). Only the CRLF exception is + declared, the redundant per-type LF rules are intentionally omitted, since the default already + gives shell scripts, Dockerfiles, workflow YAML, `uv.lock`, and every shebang-executed `.py` + the ending they need without a path-specific pin. +- **`.gitattributes` mirrors the execution-sensitive classes**: `* -text` (git stores the exact + bytes committed and does not normalize) plus explicit `text eol=lf` pins for `*.sh`, Dockerfiles, + `uv.lock`, and the shebang-executed `.py` by-path list. Those pins are now redundant with the + `[*]` LF default above, since every path they name already gets LF from it, and they stay + regardless, as git-level enforcement independent of the editor (a re-normalization tool or a + CRLF-configured git client that does not consult `.editorconfig`). Do not add a new + `.gitattributes` pin for a path that only needs the default, `.editorconfig`'s `[*]` already + covers it. Add one only for a genuinely execution-sensitive path where losing LF breaks + execution, matching the existing classes. +- **Both files are required together.** `.editorconfig` governs the editor, `.gitattributes` + governs git (checkout, commit, `--renormalize`). A repo missing either file, or whose + `.editorconfig` sets no global `end_of_line` default (for example declares it only under + `[*.md]`), accumulates files mixed between LF and CRLF, the exact failure these two files + prevent together. Carry both files whole (an inert `[*.cs]` block costs nothing in a non-.NET + repo), including the `*.sh text eol=lf` pin and any extensionless-script path pins. + +## Choosing an ending for a new file type + +LF is the default, since it is what every tool, CI runner, and Dependabot bump produces, and +Windows GUI editors (VS Code, Visual Studio, Notepad, WordPad) all read and write it cleanly. Pin +CRLF only for a type Windows itself requires it for: `*.bat` and `*.cmd`. Everything else, +including YAML (workflow and non-workflow alike, no distinction needed now that both are LF), +`.gitignore`, `.dockerignore`, and a tool-owned format with a native LF ending (KiCad), takes the +`[*]` default with no override. + +## Operational (config) repos + +The global default follows the consuming application's native platform, not the fleet LF default. +A config repo (registry `workflowModel: operational`) is a view into an application's +configuration directory, often the exact tree mounted into that app's container, so its files use +the ending the app itself reads and writes, and forcing the fleet LF default would fight an app +that needs CRLF. Set the `[*] end_of_line` default to the app's native ending and record it in the +registry `lineEndings` field (`lf` or `crlf`): the field is still required for every operational +repo regardless of value, since a config repo's ending is a load-bearing decision tied to its +consuming app rather than something to leave implicit, exactly the reasoning this fleet-wide +default flip demonstrates. Most operational repos need no override today: a Linux-native app whose +config lives in a Linux container (ESPHome, Home Assistant, a devcontainer-only or HACS config) +already matches the new `lf` fleet default. CRLF is still declared for a genuinely Windows-native +editor, for example Vantage InFusion config edited by Design Center on Windows. `release` repos +keep the `[*] end_of_line = lf` fleet default above. Do not re-normalize an operational repo to +the fleet default, that is exactly the over-normalization these per-repo endings exist to +prevent, whichever direction the fleet default currently points. + +**Mixed-consumer config: prefer to split by platform into single-platform repos, not one mixed +repo.** When a config repo would be consumed on two platforms (a Linux app plus a Windows-edited +subtree), the clean answer is a repo per consumer, each single-platform with its own +`lineEndings`. For example a controller config edited by a Windows-native editor lives in its own +CRLF repo, not as a subtree inside a Linux `lf` config repo. Fallback only if a subtree genuinely +cannot be split out: keep the global default at the primary consumer and pin the odd subtree with +an `.editorconfig` path override (for example `[<subtree>/**] end_of_line = crlf`) matching its +consumer. The global `* -text` in `.gitattributes` already preserves those bytes, no extra git pin +is needed. + +## Scripts and extensionless executables + +Must be LF. A CRLF shebang (`#!/usr/bin/env bash\r`) breaks execution, and the `[*]` LF default +already covers an extensionless executable (s6 service scripts `run`/`up`/`finish`, husky or git +hook scripts like `.husky/pre-commit`) without needing a path-specific `.editorconfig` override, +since EditorConfig's `[*]` matches any filename including one with no extension. The remaining +risk is `.gitattributes`, whose `* -text` enforces nothing on its own, so a broad normalization +pass or a CRLF-configured git client can still flip such a file on checkout regardless of what +the editor writes. Pin it there: any repo whose tooling ships extensionless scripts adds the +matching path pin (`Docker/s6-overlay/** text eol=lf` for s6 init, `.husky/pre-commit text +eol=lf` for husky hooks), so git holds them at LF on checkout and `--renormalize` independent of +the editor default. This pin is mandatory for any repo that overrides s6 init, uses husky or git +hooks, or otherwise ships executable scripts. The same explicit-pin rule extends to tool-owned +file formats the base config does not key on: pin them to whatever ending the tool reads and +writes, for example KiCad project, footprint, and 3D files (`*.kicad_mod`, `*.kicad_sym`, +`*.step`), which KiCad writes LF (already the fleet default, so these need no override either +today, but stay pinned since a tool-owned ending is a fact about the tool, not a coincidence with +whatever the fleet default currently is). + +For a type that genuinely needs an ending the `[*]` default no longer supplies (a Windows-native +tool-owned format outside `.bat`/`.cmd`, or a byte-preserve data directory whose exact bytes the +consumer may depend on), still pair a `.gitattributes` pin with a matching `.editorconfig` +override, since the git pin alone is not enough there, `.gitattributes` governs git while the +editor follows `.editorconfig`. For a byte-preserve directory, disable all editor normalization, +not just EOL: `[<dir>/*]` with `charset = unset`, `end_of_line = unset`, `insert_final_newline = +false`, `trim_trailing_whitespace = false` (`unset` is EditorConfig's spec-defined special value +that removes an inherited property). + +## Editing discipline + +- **New files**: create with the `.editorconfig`-mandated ending. +- **Editing an existing file**: preserve its current line endings, do not reflow them as a side + effect of a content change, even if the file is already non-compliant. A tool that rewrites a + file in text mode (a script, a bulk find/replace) can silently flip CRLF to LF and turn a + one-line change into a whole-file diff. After any programmatic edit, verify before staging: + `git diff --stat` should touch only the lines you changed, and a byte check should confirm the + expected ending. If a diff balloons to the whole file, the endings flipped, restore them and + re-stage. +- **Fixing a non-compliant file**: bring it to its `.editorconfig` ending as a deliberate change, + and prefer to isolate it in its own EOL-only commit so the churn is reviewable. When a broader + maintenance change has to normalize endings alongside content edits, call it out explicitly in + the commit or PR description and verify the content separately with + `git diff --ignore-cr-at-eol`. + +## Auditing + +Don't trust `file` or a naive `git ls-files --eol`. The authoritative check is a byte scan that +classifies by which endings are present: CRLF-only (every `\n` preceded by `\r`), LF-only (no +`\r`), or mixed (both forms present). Flag mixed explicitly rather than lumping it in with CRLF, +and skip binaries via a NUL-byte check. `file` mislabels some types (it reports a CRLF `.json` or +`.code-workspace` as plain "JSON text data" with no CRLF note), and `git ls-files --eol`'s `attr/` +column holds multiple tokens that shift naive field-splitting into false positives. Scope a +repo-wide audit to `git ls-files` plus `git ls-files --others --exclude-standard`, never a raw +`find`, which sweeps self-ignoring caches (`.mypy_cache`, `.artifacts`). + +Idempotent normalize: `b.replace(b"\r\n", b"\n").replace(b"\n", b"\r\n")`. A single within-line +string replace is EOL-safe, but a tool that inserts multiple lines or writes a new file into a +CRLF file must emit `\r\n`, since a naive `\n` insert creates mixed endings. `.code-workspace` is +JSONC (it has `//` comments), so strip them before JSON-parsing it. + +Editing CRLF files programmatically with a regex has a sharper trap: `.` matches `\r`, so a +captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode +rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits +(`splitlines(keepends=True)`) or literal replacement over regex reassembly. In Python the +text-mode failure is the default: `Path.read_text()` decodes through universal newlines and +`write_text()` writes `\n` back, so a read-edit-write round trip flattens the whole file while the +edit itself looks correct. Pass `newline=''` to both, or work in bytes. diff --git a/.github/skills/comment-and-doc-style/references/markdown-links.md b/.github/skills/comment-and-doc-style/references/markdown-links.md new file mode 100644 index 00000000..c3cd7e1a --- /dev/null +++ b/.github/skills/comment-and-doc-style/references/markdown-links.md @@ -0,0 +1,64 @@ +# Reference-Style Links + +Full detail for the "Markdown formatting" reference-style-links rule in `SKILL.md`. Load this +when actually authoring or reorganizing a Markdown file's link definitions, not for a small +in-place prose edit. + +## Where the rule applies + +Every Markdown file in the repo uses reference-style links only, except the four files that are +read one section at a time rather than end to end: `AGENTS.md`, `GOVERNANCE.md`, `OPERATIONS.md`, +and `.github/copilot-instructions.md`. Those keep inline `[text](uri)` links, since a reader +jumping straight to one section needs the target to resolve where it is, while a definition parked +at the bottom of the file is never reached. The exception is that closed list of four files, never +a category to argue from case by case. Every other Markdown file follows the rule regardless of +its audience. + +## The definition block + +Every URI, an internal path, an anchor, an external URL, or a shield image, is defined at the +bottom of the file, split into groups by type under an HTML-comment header, for example: + +```markdown +<!-- Shields --> + +[license-shield]: https://img.shields.io/... + +<!-- Repo --> + +[governance]: ./GOVERNANCE.md +[governance-branching-model]: ./GOVERNANCE.md#branching-model + +<!-- External --> + +[markdownlint-cli2]: https://github.com/DavidAnson/markdownlint-cli2 +``` + +Within a group, definitions are alphabetized by **reference name alone**, the text inside the +brackets, never by the whole definition line. Where one name is a prefix of another, the shorter +one sorts first: `[governance]` above `[governance-branching-model]`, `[repo-config]` above +`[repo-config-settings]`. Sorting the full line instead inverts every such pair, because `-` +precedes `]` in byte order, so the two readings disagree on exactly the names a reader looks up +together, and a plain `sort -c` over the block passes on the inverted order regardless. + +## Naming a reference + +Reference names are contextual and encode both the target and its group: + +- `foo-shield` for a shield image +- `foo-link` for an external URL +- a bare `foo` for a local path or anchor + +For example `[license-shield]`, `[releases-link]`, `[repo-config]`. Never a numeric name (`[1]`) +and never an opaque one. + +## Mechanics + +- No inline `[text](uri)` targets in prose, in any file outside the four-file exception above. +- **A URL inside a fenced code block stays inline.** Reference links do not resolve inside a code + block, so do not extract it there, and exclude fenced code from any link-integrity check + (bracket literals like `["a", "b"]` otherwise read as undefined references). +- **Removing a link also removes its reference definition.** An orphaned definition fails the + no-unused-defs rule. +- The one exception to "no inline links" is the Table of Contents, whose entries stay inline + anchor links, since the ToC extension generates them that way and they are never hand-edited. diff --git a/.github/skills/copilot-instructions-keeper/SKILL.md b/.github/skills/copilot-instructions-keeper/SKILL.md new file mode 100644 index 00000000..e4b69c61 --- /dev/null +++ b/.github/skills/copilot-instructions-keeper/SKILL.md @@ -0,0 +1,95 @@ +--- +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, 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 + +## Why this exists + +`.github/copilot-instructions.md` is read directly by GitHub Copilot and bootstraps the shared +`AGENTS.md` instruction set and review-focused skills. Its Copilot-specific rules stay fully +intact in every repo that carries it. This skill maintains that carried copy, it does not replace +the bootstrap. + +`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 `<owner>`, `<repo>`, and `<N>` 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 + +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 `.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 + 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 `<owner>`/`<repo>` 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/.github/skills/dotnet-codestyle/SKILL.md b/.github/skills/dotnet-codestyle/SKILL.md new file mode 100644 index 00000000..ee8d9a96 --- /dev/null +++ b/.github/skills/dotnet-codestyle/SKILL.md @@ -0,0 +1,210 @@ +--- +name: dotnet-codestyle +description: >- + Governs C#/.NET code style for ptr727/ProjectTemplate fleet repos: the zero-warnings build + policy and its three-task clean-compile chain, central Directory.Build.props/ + Directory.Packages.props configuration, C# language and naming conventions, XML documentation, + analyzer suppression scope, the library-versus-application logging split, async and + error-handling patterns, xUnit v3 + AwesomeAssertions testing conventions, and AOT-compatible + project configuration. Use this whenever writing, reviewing, or editing a .cs file, a .csproj, + Directory.Build.props, or Directory.Packages.props, whenever choosing where to suppress an + analyzer diagnostic, whenever a NuGet library needs to log without depending on Serilog + directly, or whenever writing or reviewing an xUnit test. Triggers even when the task looks + like a small local fix ("just silence this warning", "add a quick log line", "bump a package + version"), because the zero-warnings policy, the suppression-scope order, the central-package- + management rule, and the library/application logging split are each easy to violate one file at + a time without the pattern ever showing up as a single obvious diff. Applies only to a repo's + .NET side, a repo with no .NET projects has no use for this Skill. +--- + +# .NET Codestyle + +## Why this exists + +This is the .NET-specific half of the fleet's code style guide, kept in one place instead of +re-derived per repo or per session. CODESTYLE.md's General section still owns the rules every +language shares (clean-compile verification as a concept, the suppression-scope order, tooling +casing in prose), this Skill is everything specific to a C#/.NET project on top of that: the +concrete `.NET Format` task chain, the analyzer configuration that makes the zero-warnings policy +real, and the language, naming, logging, and testing conventions. + +## Build requirements + +### Zero warnings policy + +All builds must complete without warnings, enforced three ways: + +- **The `.NET Format` clean-compile task.** It chains `CSharpier Format` -> `.NET Build` -> + `dotnet format style --verify-no-changes`. A repo carries those three task definitions in its + own `.vscode/tasks.json`, matching the canonical `vscode-tasks.json` snippet at + `github.com/ptr727/ProjectTemplate/blob/main/catalog/snippets/configs/vscode-tasks.json`. Run + the `.NET Format` task after any code change, before commit. To run it natively instead, + reproduce that exact task chain (`CSharpier Format`, then `.NET Build`, then + `dotnet format style --verify-no-changes --severity=info --verbosity=detailed`) without dropping + or loosening any argument, reading it from that same canonical snippet. Bare `dotnet format` + alone, skipping CSharpier or the build, is not sufficient. +- **Analyzer configuration.** `<EnableNETAnalyzers>true</EnableNETAnalyzers>` with + `<AnalysisLevel>latest-all</AnalysisLevel>` and `<AnalysisMode>All</AnalysisMode>` (the full + analyzer set), plus `<TreatWarningsAsErrors>true</TreatWarningsAsErrors>`, so any diagnostic + surfaced as a warning fails the build and must be fixed or deliberately suppressed at the + narrowest scope that fits (see Analyzer suppressions below), never left to accumulate. +- **CI lint backstop.** CI runs the clean-compile checks on every PR as the authoritative gate. + Git hooks are optional, and a repo may wire a local runner (Husky.Net, with `dotnet husky run` + as a style step) for pre-commit enforcement, but CI is the gate that matters. + +**A new port is not a license to silence diagnostics.** Brownfield or just-ported status never +justifies relaxing analyzer severities or muting newly surfaced warnings. Fix them. (The only +brownfield allowance in the fleet is the one-time git-signing / line-ending migration described in +GOVERNANCE.md and README.md, which has nothing to do with code analysis.) + +### Central build and package configuration + +Shared MSBuild configuration is centralized at the repository root, never duplicated per project: + +- **`Directory.Build.props`** carries the properties every project shares: the analyzer set and + `TreatWarningsAsErrors` from the zero-warnings policy above, plus `LangVersion`, + `TargetFramework` where uniform, and any repo-wide build metadata. A `.csproj` carries only what + is genuinely project-specific (`OutputType`, `IsPackable`, project references). +- **`Directory.Packages.props`** owns central package management: it sets + `ManagePackageVersionsCentrally` to `true` (in this file, not `Directory.Build.props`) and + declares every dependency version once as a `PackageVersion` item, so a `.csproj`'s + `PackageReference` items are versionless. One file to review on a bump, one Dependabot surface, + no version skew between projects. + +A repo whose projects still carry per-project analyzer settings or versioned `PackageReference` +items is drifted, move the shared property or version up to the root file rather than editing it +in place. + +### Build tasks + +Run these from VS Code's task runner (Terminal -> Run Task) or an agent's task-running tool. The +three clean-compile tasks are carried verbatim, and a repo adds its own convenience tasks (tool +updates, dependency upgrades, benchmarks) on top: + +- `.NET Build`: build with diagnostic verbosity *(clean-compile)* +- `CSharpier Format`: auto-format code with CSharpier *(clean-compile)* +- `.NET Format`: run CSharpier and build, then verify formatting and style with + `--verify-no-changes` *(clean-compile, the task to run after edits)* + +## Tooling and editor + +- **CSharpier** is the primary code formatter, invoked by the `CSharpier Format` task or + `dotnet csharpier format --log-level=debug .`. +- **`dotnet format`** verifies style: + `dotnet format style --verify-no-changes --severity=info --verbosity=detailed`. +- **`dotnet-outdated-tool`** checks for dependency updates, and Nerdbank.GitVersioning owns + version management. +- CI is the authoritative lint backstop. Local pre-commit hooks are optional, wire Husky.Net (or + another runner) if you want local enforcement. +- **Required VS Code extensions**: CSharpier, markdownlint, CSpell. Use the workspace settings + without overrides. + +## Coding standards and conventions + +Key rules: no `var` (always explicit types), file-scoped namespaces, Allman braces, Nullable +enabled, modern C# features (primary constructors, pattern matching, collection expressions). Every +public surface has XML documentation. Private fields use `_camelCase`, static fields `s_camelCase`, +constants PascalCase. Member ordering follows StyleCop SA1201. + +For language features, naming, code structure, and XML documentation examples, see +`references/conventions.md`. + +## Analyzer suppressions (.NET) + +CODESTYLE.md's General section sets the suppression-scope order fleet-wide: narrowest scope first, +symbol-scoped before project-scoped before repo-wide, and only for a genuine false-positive or a +deliberate, documented exception, never a blanket relaxation to get a brownfield port to build. +The .NET mechanics, narrowest first: + +- **Never use `#pragma warning disable`** to silence an analyzer. +- **Symbol-scoped**: a `[System.Diagnostics.CodeAnalysis.SuppressMessage(...)]` attribute with a + `Justification`, on the specific member or type: + + ```csharp + [System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "https://github.com/dotnet/sdk/issues/51681" + )] + ``` + +- **Project-scoped** (e.g. a test project): a `dotnet_diagnostic.<RULE>.severity` entry in that + project's own `.editorconfig`, with a comment explaining why. +- **Repo-wide**: a `dotnet_diagnostic.<RULE>.severity` entry in the root `.editorconfig`, only + when the rule is genuinely not applicable to any project. Relaxing a batch of `CA*` rules (or + `dotnet_analyzer_diagnostic.severity`) to push a brownfield port through the build is exactly + what this forbids. + +## Error handling and logging + +1. **Structured logging**: use structured message templates. Serilog is the application's concrete + backend, and a library never references it directly (see item 2): + + ```csharp + logger.LogError(exception, "{Function}", function); + ``` + +2. **Libraries log through abstractions, never a concrete backend.** A NuGet library depends only + on `Microsoft.Extensions.Logging.Abstractions` and exposes an `ILoggerFactory` seam: a settable + global factory defaulting to `NullLoggerFactory.Instance` (fallback `NullLogger.Instance`) with + `SetFactory`/`TrySetFactory`, and/or an `ILoggerFactory`/`ILogger` parameter in its API. It + must not reference Serilog or any sink, which would force a logging framework on every consumer + and drag in AOT-incompatible dependencies. The consuming application owns the concrete logger + (Serilog is fine there), bridges it to `ILoggerFactory` (e.g. `SerilogLoggerFactory` from + `Serilog.Extensions.Logging`), and injects it. Reference pattern: a `LogOptions` seam in the + library, against which the consuming CLI builds the Serilog-backed factory and injects it via + `LogOptions.SetFactory`. +3. **CallerMemberName**: use for automatic function name tracking: + + ```csharp + public bool LogAndPropagate( + Exception exception, + [CallerMemberName] string function = "unknown" + ) + ``` + +4. **Logger extensions**: use `Extensions.cs` for logger and other extension methods: + + ```csharp + extension(ILogger logger) + { + public bool LogAndPropagate(Exception exception, ...) { } + } + ``` + +5. **Exceptions**: do not swallow exceptions, either log and rethrow or translate to a + domain-specific exception. + +## Code patterns + +1. **Guard clauses**: prefer early returns for validation and error handling. +2. **Async all the way**: avoid blocking calls (`.Result`, `.Wait()`), use `async`/`await`. +3. **Cancellation tokens**: accept `CancellationToken` as the last parameter and pass it through. +4. **ConfigureAwait**: in library code, use `ConfigureAwait(false)` unless context is required. Do + not call `ConfigureAwait(false)` in xUnit tests (see xUnit1030). +5. **Disposables**: use `await using` for async disposables, prefer `using` declarations. +6. **LINQ vs loops**: use LINQ for clarity, loops for hot paths or allocations. +7. **HTTP**: reuse `HttpClient` via factory, never per-request instantiation. +8. **Collections**: prefer `IReadOnlyList<T>`/`IReadOnlyCollection<T>` for public APIs. +9. **Immutability**: prefer immutable records, use init-only setters when records are not + suitable, and prefer immutable or frozen collections for read-only data. +10. **Exceptions as control flow**: avoid using exceptions for expected flow. +11. **Sealing classes**: seal classes that are not designed for inheritance. +12. **Lazy initialization**: use `Lazy<T>` for static, thread-safe instantiation (e.g. a logger + factory, an HTTP factory). + +## Testing conventions + +xUnit v3 (`xunit.v3`, not the legacy `xunit`) + AwesomeAssertions (`.Should()` API, never native +asserts). Arrange-Act-Assert pattern, descriptive underscore names, `[Theory]`/`[InlineData]` for +parameterized tests. See `references/testing.md` for the framework setup template. + +## Project configuration + +.NET 10.0 target, AOT-compatible (`IsAotCompatible=true`, `VerifyReferenceAotCompatibility=true`), +SourceLink, embedded untracked sources, `InternalsVisibleTo` for test/benchmark access. See +`references/project-config.md` for the full property list. + +## Best practices + +All changes go through pull requests. diff --git a/.github/skills/dotnet-codestyle/references/conventions.md b/.github/skills/dotnet-codestyle/references/conventions.md new file mode 100644 index 00000000..5eb48540 --- /dev/null +++ b/.github/skills/dotnet-codestyle/references/conventions.md @@ -0,0 +1,126 @@ +# .NET Coding Standards and Conventions + +Code snippets below are illustrative examples only, replace namespaces and types to match your +project. + +## C# language features + +1. **File-scoped namespaces**: + + ```csharp + namespace Example.Project.Library; + ``` + +2. **Nullable reference types**: enabled (`<Nullable>enable</Nullable>`), use nullable annotations + appropriately, use `required` for mandatory properties. +3. **Modern C# features**: prefer modern language constructs, primary constructors when + appropriate, top-level statements for console apps, pattern matching over traditional checks, + collection expressions when types loosely match, extension methods (the classic + `this`-parameter form or an `extension(<receiver>) { ... }` block on C# 14+), implicit object + creation when the type is apparent, range and index operators. +4. **Expression-bodied members**: use for applicable methods, properties, accessors, operators, + lambdas, local functions. +5. **`var` keyword**: do NOT use `var`, always use explicit types: + + ```csharp + // Correct + int count = 42; + string name = "test"; + + // Incorrect + var count = 42; + var name = "test"; + ``` + +## Naming conventions + +1. **Private fields**: underscore prefix with camelCase: + + ```csharp + private readonly HttpClient _httpClient; + private int _counter; + ``` + +2. **Static fields**: `s_` prefix with camelCase: + + ```csharp + private static int s_instanceCount; + ``` + +3. **Constants**: PascalCase: + + ```csharp + private const int MaxRetries = 3; + ``` + +## Code structure + +1. **Global usings**: use `GlobalUsings.cs` for common namespaces: + + ```csharp + global using System; + global using System.Net.Http; + global using System.Threading.Tasks; + global using Microsoft.Extensions.Logging; + ``` + +2. **Usings placement**: outside the namespace, sorted with `System` directives first: + + ```csharp + using System.CommandLine; + using System.Runtime.CompilerServices; + using Example.Project.Library; + + namespace Example.Project.Console; + ``` + +3. **Braces**: Allman style: + + ```csharp + public void Method() + { + if (condition) + { + // code + } + } + ``` + +4. **Indentation**: C# files 4 spaces, XML/csproj files 2 spaces, YAML files 2 spaces, JSON files + 4 spaces. +5. **Line endings**: not specified here, governed per repo by `.editorconfig` / `.gitattributes` + per GOVERNANCE.md's "Line Endings" section. +6. **`#region`**: do not use regions, prefer logical file/folder/namespace organization. +7. **Member ordering (StyleCop SA1201)**: const -> static readonly -> static fields -> instance + readonly fields -> instance fields -> constructors -> public (events -> properties -> indexers + -> methods -> operators) -> non-public in same order -> nested types. + +## Comments and documentation + +XML documentation is on: `<GenerateDocumentationFile>true</GenerateDocumentationFile>`, and +missing XML comments for public APIs are suppressed in `.editorconfig`. Every public surface must +still be documented: a single-line summary, additional details in remarks, documented input +parameters, return values, exceptions, and crefs. + +```csharp +/// <summary> +/// Example of a single line summary. +/// </summary> +/// <remarks> +/// Additional important details about usage. +/// Multiple lines if needed. +/// </remarks> +/// <param name="category"> +/// The quote category to request +/// </param> +/// <param name="cancellationToken"> +/// A <see cref="System.Threading.CancellationToken"/> that can be used to cancel the request. +/// </param> +/// <returns> +/// A <see cref="string"/> containing the quote text. +/// </returns> +/// <exception cref="System.ArgumentException"> +/// Thrown when <paramref name="category"/> is not a supported value. +/// </exception> +public async Task<string> GetQuoteOfTheDayAsync(string category, CancellationToken cancellationToken) {} +``` diff --git a/.github/skills/dotnet-codestyle/references/project-config.md b/.github/skills/dotnet-codestyle/references/project-config.md new file mode 100644 index 00000000..8f6e8388 --- /dev/null +++ b/.github/skills/dotnet-codestyle/references/project-config.md @@ -0,0 +1,17 @@ +# .NET Project Configuration + +1. **Target framework**: .NET 10.0 (`<TargetFramework>net10.0</TargetFramework>`). +2. **AOT compatibility**: `<IsAotCompatible>true</IsAotCompatible>`, + `<VerifyReferenceAotCompatibility>true</VerifyReferenceAotCompatibility>`. +3. **Assembly information**: use semantic versioning, include SourceLink + (`<PublishRepositoryUrl>true</PublishRepositoryUrl>`), embed untracked sources + (`<EmbedUntrackedSources>true</EmbedUntrackedSources>`). +4. **Internal visibility**: use `InternalsVisibleTo` for test and benchmark access (adapt the + project names to your repo's test/benchmark projects): + + ```xml + <ItemGroup> + <InternalsVisibleTo Include="YourBenchmarkProject" /> + <InternalsVisibleTo Include="YourTestProject" /> + </ItemGroup> + ``` diff --git a/.github/skills/dotnet-codestyle/references/testing.md b/.github/skills/dotnet-codestyle/references/testing.md new file mode 100644 index 00000000..5a84a178 --- /dev/null +++ b/.github/skills/dotnet-codestyle/references/testing.md @@ -0,0 +1,25 @@ +# .NET Testing Conventions + +1. **Framework**: xUnit v3 or later (the `xunit.v3` package, never the legacy v2 `xunit` package) + with AwesomeAssertions for every assertion. Native xUnit asserts (`Assert.Equal`, + `Assert.True`, ...) are not allowed, use the fluent `.Should()` API. Dynamic test skipping + (`Assert.Skip`, `Assert.SkipWhen`) is control flow, not an assertion, and stays native: + + ```csharp + [Fact] + public void MethodName_Scenario_ExpectedBehavior() + { + // Arrange + int expected = 42; + + // Act + int actual = GetValue(); + + // Assert + actual.Should().Be(expected); + } + ``` + +2. **Organization**: Arrange-Act-Assert pattern. +3. **Naming**: descriptive names with underscores. +4. **Theory tests**: use `[Theory]` with `[InlineData]`. diff --git a/.github/skills/fleet-conformance-check/SKILL.md b/.github/skills/fleet-conformance-check/SKILL.md new file mode 100644 index 00000000..2b6e8406 --- /dev/null +++ b/.github/skills/fleet-conformance-check/SKILL.md @@ -0,0 +1,74 @@ +--- +name: fleet-conformance-check +description: >- + Checks, from inside a downstream repo's own session, whether this repo and this machine are + current against the ptr727/ProjectTemplate hub, and safely self-applies what it can. Use this + whenever asked to check if this repo is up to date with the hub, whenever a fleet rule or Skill + seems to not be applying and the cause is unclear, or whenever about to work in a fleet repo and + wanting to confirm the ground under that work is current before trusting it. Needs no standing + hub checkout of its own and no named target repo, only the repo the session is already in, + though the check itself fetches a hub checkout to reach scripts/skills_install.py, since + scripts/ is hub-hosted rather than carried. This is the counterpart to resync-a-repo, which + needs both a hub checkout already in hand and a named external target to drive change from the + hub side instead. Also triggers on "why do I have to keep restating this rule every session," + since a stale or missing Skills install is the most common cause and the cheapest one to rule + out first. +--- + +# Fleet Conformance Check + +## Why this exists + +A downstream repo today only finds out it has drifted when someone runs a hub-driven resync +against it by name. Nothing notices from the inside on its own. This skill is that inside check, +run with no hub-side operator watching, so a stale Skills install or an out-of-date `AGENTS.md` +pointer gets noticed and fixed without waiting for a fleet-wide sweep to reach this particular +repo. + +## What it checks + +1. **Is the Skills install current on this machine.** `scripts/` is hub-hosted and reached rather + than carried, per GOVERNANCE.md "Hub-Hosted Tooling", so fetch a hub checkout + (`github.com/ptr727/ProjectTemplate`, `main` branch, fetched fresh) and run + `python3 scripts/skills_install.py --report` from it. A stale or missing stamp is very often + the direct answer to "why isn't a fleet rule applying": the harness never loaded the current + content in the first place, and no amount of re-reading `GOVERNANCE.md` fixes that. +2. **Does this repo's own carried content still match the hub.** Compare `AGENTS.md`'s + "Where the Rules Live" pointer text, and any other verbatim `AGENTS.md`/`GOVERNANCE.md` section + this repo carries, against the same hub checkout's current wording, by reading the text rather + than by feel. + +## What it is safe to fix on its own + +- **Re-run the installer**, `python3 scripts/skills_install.py`, when the stamp reports stale. + This is a per-machine, local-only change, nothing in it touches this repo's git history or + needs a review. + +Nothing else. This skill never re-vendors a carried file, never deletes one, and never applies a +setting or ruleset. Those are `resync-a-repo`'s job, driven from the hub with a named target, +never a downstream repo acting on itself. + +## Refresh cadence + +Re-run the installer when `--report` exits non-zero, and after any hub merge that touches +`.agents/skills/`. Session entry runs no automatic check, by design: the trigger is suspicion, +and the restated-rule symptom below is the loudest form of it. `docs/host-setup.md` +"Fleet Skills Install" in the hub states the same cadence for the host side, and an automated +refresh stays out of scope until the fleet has evidence the manual cadence fails. + +## What it escalates instead of touching + +- **A carried section that differs from the hub in a way that reads as a genuine local addition** + rather than plain staleness, the exact case `carried-instruction-file-guard` exists to protect. + Report precisely what differs and stop there. Per AUDIT.md, a downstream repo does not write its + own audit report or resync itself against the hub, it names what it found and points at + `resync-a-repo`, run from a hub checkout, as the next step. +- **Anything the installer alone cannot resolve**, a broken `claude` CLI marketplace + registration, a settings or ruleset drift, a workflow interface mismatch. Name it and hand it to + the maintainer or a hub-driven resync rather than patching around it locally. + +## Answering "why isn't a fleet rule applying" + +Check the install stamp first, before assuming a Skill's description is worded wrong or that the +rule was never carried to this repo at all. It is the most common cause, and it is the cheapest +one to confirm. diff --git a/.github/skills/git-commit-conventions/SKILL.md b/.github/skills/git-commit-conventions/SKILL.md new file mode 100644 index 00000000..85e46eb3 --- /dev/null +++ b/.github/skills/git-commit-conventions/SKILL.md @@ -0,0 +1,169 @@ +--- +name: git-commit-conventions +description: >- + Governs how an agent stages, commits, signs, and pushes in a ptr727/ProjectTemplate fleet repo: + default-to-staging vs. explicit commit authorization, why "commit" means commit-and-push, the + mandatory signed-commit and noreply-identity checks, never force-pushing, how a history rewrite + must re-identify a commit that is not the agent's own, and the destructive-git-command ban. Use + this whenever about to run git add/commit/push, whenever authorization to commit is ambiguous + ("fix this" versus "commit this"), whenever about to configure or verify commit signing or + git user.email, whenever a merge conflict or a stale branch tempts a force-push or a hard reset, + and whenever rewriting history (filter-repo, an interactive rebase equivalent) touches a commit + authored or committed by someone else. Triggers even when the task looks like routine + housekeeping, such as "clean up this branch" or "just push it", because a scope-widened commit + authorization, an unsigned commit, a fabricated identity, or a force-push are each easy to do by + habit and each one is a hard-to-reverse mistake on a shared branch. +--- + +# Git Commit Conventions + +## Why this exists + +These are the fleet's mechanical git rules for producing a commit, kept in one place instead of +re-derived per repo or per session: whether to commit at all, what committing implies, how +signing and identity are verified rather than configured, and which commands are never run +without being asked. None of these are style preferences. Branch protection enforces several of +them at push time, and the rest guard against damage a rejected push does not undo (a +scope-widened commit, a rewritten shared history, a destructive reset). + +## Staging versus committing + +- **Default to staging, not committing.** Stage with `git add` and leave `git commit` to the + developer unless the developer has explicitly authorized committing for the current ask ("commit + this", "open a PR"). Authorization is scope-bound: it covers the commits that specific task + needs, not a blanket license for the rest of the session. +- **Stage by explicit path, never `git add -A` or `git add .`.** A blanket add stages whatever + else happens to be in the tree, and what it sweeps in is another task's uncommitted work, + landing in a commit whose subject never mentions it, committed by a session that never saw it. + That sweep has happened, which is why task isolation exists (the `repo-worktree` skill), and + isolation makes a shared tree rare rather than impossible. Name the files this task changed, + and let anything else stay unstaged. +- **"Commit" means commit and push.** An authorization to commit carries the push to the feature + branch the work belongs on, because nothing reviews a local commit. The Copilot review loop, the + required status checks, and the maintainer all read the remote, so stopping at `git commit` + leaves the review unstarted and the branch's state private to one machine, which reads as + progress while none of the gates have run. Push to the feature branch, never to a protected + branch, and never with `--force`. Holding a commit locally is the narrower case: it happens when + the developer asks for it, not by default. +- **Check `git status` before committing, and treat any change this session did not make as a + stop.** The maintainer hand-edits files live, often `README.md`/`HISTORY.md`, sometimes with an + editor's LF -> CRLF flip on top, and a sibling agent session sharing the tree leaves its edits + the same way. Whoever the author is, a change this session did not make is never bundled: ask + whether to include it, or leave it unstaged and say so, rather than committing half-finished + work or stranding it in an unrelated commit. An unexpected change in the tree is also the + signal to re-check isolation per the `repo-worktree` skill, since it may mean another task is + live in this checkout. + +## Signing, verified not configured + +- **Every commit must be cryptographically signed (SSH or GPG).** Branch protection enforces this + on every fleet branch, and an unsigned commit is rejected on push. Signing depends on + environment configuration (`commit.gpgsign`, `user.signingkey`, `gpg.format`), but none of those + values prove signing actually works: `gpg.format=ssh` can sign straight from a key file with no + `ssh-agent` running at all (the common case on Git for Windows), just as GPG can sign + agent-backed or straight from a keyring. **Probing agent liveness (`ssh-add -L`, a `gpg-agent` + check) is not a valid test and must not be used.** It tests one specific delivery path, not + whether a commit actually ends up signed, and a host that signs straight from a key file fails + that probe while signing correctly. +- **Verify with a real scratch commit, read back with git's own verdict, not a text grep.** This + single probe is tech-agnostic (SSH agent-backed, SSH key-file, GPG agent-backed, and GPG keyring + all exercise the same code path) and doubles as the identity check below. Run it once before the + first agent-authored commit of a session. Don't assume a prior session left config correct. The + commit below is plain, deliberately no `-S`: forcing it would still succeed on a host where + `commit.gpgsign` is unset or false, which is the exact default-config gap this probe exists to + catch, since every real commit an agent makes is plain too: + + The probe is one physical line, not backslash-joined ones, so it copy-pastes cleanly into a + shell: + + ```sh + d=$(mktemp -d "${TMPDIR:-/tmp}/sign-check.XXXXXX") && ( trap 'rm -rf "$d"' 0; email=$(git config --global --get user.email) && git init -q "$d" && git -C "$d" commit --allow-empty -q -m check && out=$(git -C "$d" log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>') && echo "$out" && ae=$(git -C "$d" log -1 --format='%ae') && ce=$(git -C "$d" log -1 --format='%ce') && case "$out" in sig=G\ *|sig=U\ *) true ;; *) false ;; esac && case "$email" in *@users.noreply.github.com) true ;; *) false ;; esac && [ "$ae" = "$email" ] && [ "$ce" = "$email" ] ) + ``` + + PowerShell equivalent: + + ```powershell + $d = Join-Path $env:TEMP ([guid]::NewGuid()) + try { + $email = git config --global --get user.email + git init -q "$d" ` + && git -C "$d" commit --allow-empty -q -m check + $out = git -C "$d" log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>' + $out + $ae = git -C "$d" log -1 --format='%ae' + $ce = git -C "$d" log -1 --format='%ce' + if ($out -notmatch '^sig=[GU] ' -or $email -notmatch '@users\.noreply\.github\.com$' ` + -or $ae -ne $email -or $ce -ne $email) { + throw "signing/identity check failed: $out" + } + } finally { + if (Test-Path "$d") { Remove-Item -Recurse -Force "$d" } + } + ``` + + `sig` must read `G` (good signature) or `U` (good signature, unrecognized signer). For GPG, `U` + is a valid signature from a key whose trust level is merely undefined, common right after + generating a new key. For SSH, it's a valid signature from a key not found in the local + `allowed_signers` file, which doesn't affect whether GitHub itself verifies the commit, only + local `git verify-commit` output. `sig` is git's own verdict char. Don't grep localized + "Good" text, since that varies by git version and locale. Anything else, or the commit failing + outright, means **do not commit**: surface the actual error to the developer and stop at + `git add`. Nothing else is contrary evidence: not an unreachable agent, not a config value, not a + signature type you can't otherwise explain in past history (see below). +- **A mix of SSH- and GPG-signed commits in history is structural, not a host to track down.** + `git log --pretty='%G? %GK'` shows two distinct shapes, not two health states: a commit committed + by the PR's own author carries that host's own signature type, while a commit committed by + `GitHub <noreply@github.com>` is a squash-merge: GitHub creates and signs that commit itself, + server-side, with GitHub's own GPG key, regardless of what the PR author signed with locally. + Every commit on `develop`/`main` past its first squash-merge shows `GitHub` as committer and a + GPG signature. That's expected on every fleet repo, on every host, and is not evidence anything + is misconfigured. Check `commit.committer.name` before treating a differing signature type as a + clue worth chasing. +- **Signing must be live before the *first* commit, not retrofitted.** Turning on a + require-signed-commits rule against a branch that already carries unsigned commits forces a + rewrite of that entire history to re-sign it, changing every commit SHA and making whoever does + the rewrite the committer and signer of every commit in it (a rebase preserves `author` but not + the original signatures, and one contributor cannot sign for another). During new-repo setup, + never create commits until signing is verified. + +## Identity, verified not set + +**Commit under the committing account's own GitHub `noreply` identity, never a private, personal, +or invented address.** `author` and `committer` on every agent-authored commit are the GitHub +`noreply` address of the account whose key signs the commit, in `username@users.noreply.github.com` +or `ID+username@users.noreply.github.com` form. **Verify it, do not set it**: the scratch commit +from the signing check above already proves this end-to-end. Read its `author=`/`committer=` +output rather than trusting `git config --get user.email` alone, since a global config value +doesn't prove what actually lands on a commit object, and read both rather than the author alone +since a rebase, amend, or cherry-pick can rewrite the committer while leaving the author +untouched. Match both against that address before committing, rather than +writing a repo-local override. The identity is host configuration set globally once, so a repo-local +`user.email` is redundant where the global is right and a silently-shadowing wrong identity where +it is not. A mismatch is a host fault to surface to the maintainer, not to patch per repo, because +a local override hides a broken host that then commits wrong in every other repo on that machine. +A wrong identity is not cosmetic: a private email trips GitHub's email-privacy push protection, and +an invented author pollutes history. It is also a distinct failure from signing (a wrong author +does not by itself fail the signature check), though the ad-hoc identities that produce one are +typically also unsigned, which the signing rule above then rejects independently. + +## Never force push + +Do not run `git push --force` or `git push --force-with-lease` under any circumstances. Force +pushing rewrites shared history and can cause data loss. This holds regardless of how confident +the rewrite looks, a rejected push is recoverable, a force-pushed one is not. + +## History rewrites re-identify only what changed + +**Do not rewrite a commit that does not need to change.** A history rewrite (e.g. `git filter-repo` +to strip PII) re-signs every touched commit with the rewriter's key. If that commit is still +committed by a bot (`dependabot[bot]`, `github-actions[bot]`) or GitHub's own web-flow, the +signature will not match the committer and the require-signed-commits rule rejects it. Scope the +rewrite to only the commits that must change. Set `committer` (and `author`) to the rewriter's +identity on any non-own commit that must be modified. Verify with `git log --show-signature` after +any rewrite. See `references/history-rewrite.md` for the full two-gate rule. + +## Never run destructive git commands without being asked + +`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`, and anything else that +discards uncommitted work runs only on explicit developer instruction, never as a convenience step +inside a larger task. diff --git a/.github/skills/git-commit-conventions/references/history-rewrite.md b/.github/skills/git-commit-conventions/references/history-rewrite.md new file mode 100644 index 00000000..b331a557 --- /dev/null +++ b/.github/skills/git-commit-conventions/references/history-rewrite.md @@ -0,0 +1,24 @@ +# History Rewrites: Re-identification Rules + +**A history rewrite includes only the commits that must change, and re-identifies any commit it +rewrites that is not the agent's own.** Filtering history (`git filter-repo` or an equivalent, for +example to strip PII) re-signs every commit it touches with the rewriter's own key, while the +tooling preserves each commit's original `author`/`committer` unless told otherwise. GitHub +verifies a signature against the commit's `committer` identity, so a signature from the rewriter's +key over a commit still committed by a bot (`dependabot[bot]`, `github-actions[bot]`) or GitHub's +own web-flow does not match its committer and lands `unknown_key`/unverified, which a +require-signed-commits rule then rejects. + +Two gates keep committer and signature aligned: + +1. **Scope the rewrite to only the commits that must be modified.** By default those are the + rewriter's own, whose committer already matches, so a commit that needs no change stays out of + the rewrite entirely and its identity and signature are never touched. +2. **If a commit that must change is not the rewriter's own, set its `committer` to the rewriter's + own signing identity before re-signing** (and its `author` too, since a rewrite that alters + content should not keep attributing it to the bot). The original bot attribution is deliberately + given up as the cost of having to rewrite it. + +Never leave a signature over a commit committed by another identity. Verify after any rewrite that +every rewritten commit is signed and committed under the correct identity +(`git log --show-signature`). diff --git a/.github/skills/operational-vs-release-workflow/SKILL.md b/.github/skills/operational-vs-release-workflow/SKILL.md new file mode 100644 index 00000000..0834b424 --- /dev/null +++ b/.github/skills/operational-vs-release-workflow/SKILL.md @@ -0,0 +1,155 @@ +--- +name: operational-vs-release-workflow +description: >- + Governs how a ptr727/ProjectTemplate fleet repo branches, promotes, and publishes: the + feature -> develop -> main flow, squash-only vs. merge-commit-only branch protection, the two + develop -> main promotion traps (never delete develop, EOL-only conflicts), the two-phase + publish model (PRs smoke-test only, a human merge never auto-publishes), NBGV semantic + versioning, and the operational-repo delta (direct-to-develop commits, advisory CI, dispatch-only + release) that applies instead whenever the registry's workflowModel field for this repo reads + operational rather than release. Use this whenever choosing a target branch for a change, + promoting develop to main, resolving a develop -> main merge conflict, deciding whether a + release repo's config change needs a PR versus an operational repo's config change can commit + straight to develop, bumping version.json, adding or dropping a release target, or reasoning + about why a merge did or didn't trigger a publish. Triggers even when the request sounds like + ordinary git housekeeping ("just push this config fix", "merge develop into main", "cut a + release"), because the two workflow models genuinely differ (a direct-to-develop commit that is + correct in an operational repo is a rule violation in a release repo, and vice versa) and + applying the wrong one is not obviously wrong to a reader who only knows one of the two. +--- + +# Operational vs. Release Workflow + +## Why this exists + +Two workflow models exist because the underlying repos are two different things. Most fleet repos +ship versioned units of delivery, so they earn a feature -> `develop` -> `main` flow with real +release gates. A handful of repos instead track a live service's running state (Home Assistant, +ESPHome, Vantage, home automation configs) where the "release" is the config already committed, +not something built and shipped later. Applying the release model's ceremony to an operational +repo, or skipping the release model's gates on a repo that actually ships versioned artifacts, is +each wrong in its own repo and correct in the other, which is why this is one skill keyed on which +repo you're in rather than two skills that never talk to each other. + +## Which model this repo uses + +Read the registry `workflowModel` field for this repo (`release`, the default, or `operational`). +The rest of this skill's "Branching" and "Publishing" sections describe the `release` model. The +"Operational repositories" section below is the complete delta for `operational` repos. Anything +not mentioned there is unchanged. When in doubt which one applies, check `registry/repos.json` +rather than guessing from the repo's contents. + +## Branching (release model) + +- **GitHub's repository setting for "default branch" reads `main`, but `develop` is where work starts and where in-flight content lives.** A worktree or clone that defaults to "the default branch" lands on `main` and can silently miss content that has merged to `develop` but not yet been promoted. Before branching off a change, or asserting something absent from this repo, check `develop`, not just whichever branch a tool defaulted to. See GOVERNANCE.md "Verification Discipline" on naming the branch a "does not exist" claim was checked against, and the `repo-worktree` skill, which owns the worktree-creation moment this base-branch choice is made at. +- `develop` is the integration branch. Feature branches -> `develop` is **squash-only**, which + keeps `develop` linear. +- `develop -> main` is **merge-commit only** (no squash, no rebase). Merge commits preserve + `develop`'s commit list as a real second-parent reference on `main`, which lets the release + model attribute releases to the develop commits that produced them. Branch protection enforces + this: the `develop` ruleset allows only `squash`, the `main` ruleset allows only `merge`. +- All commits on both branches must be cryptographically signed (SSH or GPG), see + `git-commit-conventions`. Squash and merge commits created via the GitHub UI are signed by + GitHub's web-flow key. +- **`develop` is forward-only, with no `main -> develop` back-merges.** The `develop` ruleset's + squash-only setting physically blocks merge commits on `develop`. Any historical back-merge + commits in `git log` predate this rule and must not be repeated. +- **Never delete `develop`, and take the EOL-only conflict by taking develop's side.** A + promotion PR's head *is* `develop`, so `--delete-branch` deletes it. An EOL-only conflict on a + workflow YAML file resolves on a throwaway branch off `main`, not on `develop`. Full recovery and + conflict-resolution commands: `references/branch-protection-and-promotion.md`. +- **Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not + the feature -> `develop` PR.** GitHub auto-closes an issue only when the closing keyword merges + into the **default branch** (`main`), so a feature -> `develop` PR merge never fires it. + Reference the issue in the `develop` PR body if useful, but the actual closing keyword belongs on + the promotion PR. Closing by hand is the ordinary route wherever the keyword cannot fire (a + promotion that already merged without it, or completed work with no promotion imminent), not a + repair for a botched promotion, cite the squash SHA and re-read that commit before closing. +- **Neither ruleset requires branches to be up to date before merging**, for different reasons on + each branch (a graph-based check that would fail every release on `main`, a check that stalls + bot auto-merge on `develop`). Detail: `references/branch-protection-and-promotion.md`. +- **Configuring branch protection: import the committed ruleset payloads, don't hand-build them.** + Exactly two rulesets, named `develop` and `main`. Full procedure, including the operational + `develop` payload and the brownfield-repo signing caveat: + `references/branch-protection-and-promotion.md`. +- **Dependabot and codegen target both `main` and `develop` in parallel**, each branch absorbing + its own bot PRs independently so neither falls behind, with the merge-bot dispatching the merge + form (`--squash`/`--merge`) that matches each PR's base ruleset. Codegen output must be + deterministic from its inputs alone, never per-run state, or the two branches' legs conflict on + every promotion. Full mechanics: `references/branch-protection-and-promotion.md`. +- **App-token workflows authenticate with Client ID, not the deprecated App ID.** Use + `client-id: ${{ secrets.CODEGEN_APP_CLIENT_ID }}` at any new App-token call site. + +## Publishing (release model) + +- **The two-phase model is the default: PRs build fast, publishing is batched.** A PR only + smoke-tests (unit tests plus a reduced build of the changed targets), it never pushes anything. + `publish-release.yml` is the sole publisher, and each run builds a **single trigger branch** + (`main` a release, `develop` a prerelease). +- **A human merge never auto-publishes.** Publishing fires on a **`workflow_dispatch`** of + `main`/`develop` (a human-initiated release), a **code-affecting bot push to `main`** (the + codegen App merging a Dependabot/codegen PR, gated on `github.actor` so a human + merge/promotion skips it), or a **weekly `schedule`** (Docker only, to refresh the base image). + A source-only repo publishes on dispatch only. +- **The changes-detection job is a required check that must succeed, not just not fail.** A + paths-filter error must never let a target-changing PR merge with its smoke build silently + skipped. A skipped smoke job (no matching change) passes, `failure`/`cancelled` blocks. +- **Versioning is semantic and maintainer-controlled.** `version.json`'s `major.minor` is the + version floor, edited by the maintainer for functional changes only, in the PR that introduces + the work, never on a fixed cadence or mechanically after a release. NBGV appends the git height + automatically on every commit, so a release always gets a fresh build version with **no + post-release bump** and no develop-ahead requirement. +- **Docs reference the 2-digit `major.minor` line, never a 3-digit build.** `README.md`, + `HISTORY.md`, and release notes name the version as `Version 1.0` (the floor), never the concrete + build height, which is both wrong (the real height differs) and a maintenance trap. + "Correcting" `1.0` to `1.0.0` is a defect. +- **A no-op publish (unchanged NBGV `SemVer2`) re-pushes nothing to any target keyed on the + version string, except Docker, which always re-pushes** to pick up upstream base-image + refreshes. Full guarantee and the `version.json` `pathFilters` boundary: + `references/release-publish-mechanics.md`. +- **Adding, dropping, or wiring a release target** (which leaf task, which artifact-naming + contract, which seam a given output belongs to: a GitHub Release asset, a package-registry push, + an image-registry push, a filesystem deploy, or a source-only repo with no build layer at all), + and tracking an upstream release from a wrapper repo: `references/release-publish-mechanics.md`. + See also `WORKFLOW.md` for the full CI/CD contract this section's rules are load-bearing + excerpts of. + +## Operational repositories (the complete delta) + +Everything above is the `release` model. An `operational` repo (registry `workflowModel: +operational`) tracks a live service's running state rather than shipping versioned units of +delivery, and differs from the `release` model in exactly these ways, everything not listed here +stays the same: + +- **Commit configuration directly to `develop`.** There is no feature branch requirement, the + maintainer commits straight to `develop`, and only *occasionally* opens a `develop -> main` PR to + bless a known-good snapshot. The `develop` ruleset drops the PR and status-check gate, so direct + signed pushes are allowed (force-push, deletion, and unsigned commits are still blocked), and CI + runs on the push as **advisory** feedback that never rejects a commit. +- **A PR into `develop` stays available, and CI runs on it, reported but not required.** Dropping + the requirement permits the direct push, it does not withdraw the pull request, so a change worth + reviewing takes one and both paths into `develop` are legitimate. +- **Take the pull request whenever the change is not one a reader takes in at a glance and + reverts cleanly.** What decides it is the shape of the change, not a line count: restructuring + rather than adjusting a value, touching several files at once, introducing a device, an + integration, or an automation that did not exist before, and anything whose failure shows up on + the live service rather than in a lint run are each the pull request case. So is a change the + author cannot state in one sentence. This stays a judgment call by design, adding a + `pull_request` rule to the operational `develop` ruleset would gate the direct push too and + withdraw the allowance the model exists to give. +- **The `main` promotion gate is unchanged.** The shared `main` ruleset still **enforces** the + required `Check pull request workflow status job` on the `develop -> main` PR. For an operational + repo that check is lint/validation only (editorconfig/EOL plus a domain linter such as a Home + Assistant or ESPHome config validation, never unit tests), so `develop` stays the live surface + and a broken config can never reach `main`. +- **Release only by manual dispatch.** Operational repos carry `releaseTrigger: dispatch-only` and + run no codegen or auto-publish bots, publishing **only** on a manual `workflow_dispatch` (the + same source-only release the publisher already supports: tag, source zip, README, LICENSE, + NBGV-versioned), never automatically. The `develop -> main` promotion just blesses a known-good + snapshot, a release is a separate, deliberate dispatch. +- **Fleet sync still applies.** Dependabot's dual-target sync and the App-signed merge-bot run on + **every** tier, operational included, so both branches stay in sync and a promotion stays a + clean forward merge. +- **Line-ending policy differs too**, following the consuming app's native platform rather than the + fleet LF default, per the registry `lineEndings` field. That rule belongs to + `comment-and-doc-style`, not repeated here. diff --git a/.github/skills/operational-vs-release-workflow/references/branch-protection-and-promotion.md b/.github/skills/operational-vs-release-workflow/references/branch-protection-and-promotion.md new file mode 100644 index 00000000..f30abd0a --- /dev/null +++ b/.github/skills/operational-vs-release-workflow/references/branch-protection-and-promotion.md @@ -0,0 +1,114 @@ +# Branch Protection and Promotion Mechanics + +Full detail for the "Branching" rules in `SKILL.md`. Load this when configuring or reconstructing +branch protection on a fleet repo, executing a `develop -> main` promotion, recovering a lost +`develop`, resolving an EOL-only promotion conflict, or working on the dual-target bot wiring +(Dependabot, codegen, the merge-bot), not for an ordinary feature-branch PR (the SKILL.md summary +covers that case). + +## Configuring branch protection: don't hand-build the rules + +Reconstructing rulesets by hand is error-prone and has gone wrong on past ports. First delete +**all** legacy classic branch-protection rules and any stray rulesets (rulesets are the *only* +mechanism used), then create **exactly two rulesets named `develop` and `main`** by importing the +committed `repo-config/*.json` ruleset payloads via `gh api -X POST "repos/<owner>/<repo>/rulesets"` +(`gh ruleset` is read-only). The names are load-bearing, other governance content and the +workflows reference them. Operational repos import `repo-config/operational/develop.json` as their +`develop` ruleset (the `main` ruleset is shared), and the hub's `repo-config/configure.sh`, run +from a hub checkout against the repository named on its command line, selects the right `develop` +payload from the registry `workflowModel` automatically. **Brownfield repos** (pre-existing +history) need an extra step: `Require signed commits` rejects legacy unsigned commits and the +admin bypass does not cover `git push --force`, so re-signing requires temporarily disabling the +ruleset. See `repo-config/README.md` "Rulesets" for the configured state. + +## Executing a `develop -> main` promotion safely + +Two traps, both learned the hard way: + +- **Never delete `develop`.** A promotion PR's head *is* `develop`, so `gh pr merge --delete-branch` + (and a repo's "Automatically delete head branches" toggle, kept off in `repo-config/settings.json` + for exactly this reason) deletes `develop` itself. Merge a promotion with a plain + `gh pr merge --merge`, no `--delete-branch`. If `develop` is ever lost this way, restore it to + the merged PR's head SHA, which is still reachable as the merge commit's second parent: + `gh api -X POST "repos/<owner>/<repo>/git/refs" -f ref=refs/heads/develop -f sha="$(gh pr view <n> --json headRefOid --jq .headRefOid)"`. +- **Spurious EOL-only conflicts resolve by taking `develop`.** When `develop`'s `.editorconfig` + line-ending default has changed (for example the fleet-wide CRLF-to-LF flip) while `main` hasn't + caught up yet, `develop -> main` conflicts *whole-file* on every renormalized path. + `develop`'s `required_linear_history` plus PR rulesets forbid resolving on `develop` (no merge + commit, no force-push), so resolve on a throwaway branch off `main`: + `git checkout -b promote/develop-to-main origin/main && git merge origin/develop`, take + `develop`'s side for the EOL-conflicted files (`git checkout --theirs <file>`) **after + confirming each is content-identical modulo EOL, or that `develop` is a strict superset** + (`diff <(git show :2:f | tr -d '\r') <(git show :3:f | tr -d '\r')`), then open that branch into + `main`. Verify no genuine `main`-only content is dropped (build/test where the repo supports it). + +## Why both rulesets omit "Require branches to be up to date before merging" + +The flag is off on `main` and on `develop`, for related but distinct reasons. + +- **Main**: the check is graph-based, it asks whether `main`'s tip commit is reachable from + `develop`, not whether the two branches have the same content. After any `develop -> main` + release, `main`'s tip is a brand-new merge commit that `develop`'s history doesn't contain. + Forward-only `develop` never adds it (no back-merge of `main` into `develop`), so the check + would fail on every subsequent release. Other technical workarounds (rebasing `develop` onto + `main`, or rewriting `develop`'s history) exist but contradict the squash-only `develop` ruleset + and the linearity invariant. +- **Develop**: the check stalls bot auto-merge when two bot PRs against `develop` land within the + same window. As soon as the first merges, the second flips to `mergeStateStatus: BEHIND` and + GitHub's auto-merge will not fire while strict is on. The merge-bot only *enables* auto-merge on + `opened`/`reopened` and never auto-updates bot branches, and Dependabot's rebase isn't real-time, + so the second PR sits OPEN with all checks green indefinitely. Squash mechanics still rebase the + diff onto `develop`'s tip on merge, `required_linear_history` still enforces linearity, textual + conflicts still block `mergeable: CONFLICTING`, and the required `Check pull request workflow + status job` still gates merges. The only thing lost is pre-merge detection of + *semantic-but-not-textual* conflicts, which the post-merge `develop` CI run catches anyway. + +## Dual-target bots + +**Dependabot and codegen target both `main` and `develop` in parallel.** +`.github/dependabot.yml` duplicates every ecosystem entry (one per branch) and the codegen +workflow runs as a matrix over both branches with branch names `codegen-main` and +`codegen-develop`. Each branch absorbs its own bot PRs independently, so neither falls behind, and +the forward-only rule still holds, nothing is back-merged from `main` to `develop`, both branches +receive their updates directly. The merge-bot (`.github/workflows/merge-bot-pull-request.yml`) +dispatches `--squash` or `--merge` from each PR's base ref via a `case` statement so the form +matches the ruleset on either base. Dependabot **security** PRs (CVE-driven) always open against +the repo default branch (`main`) regardless of `target-branch`, and the same `case` statement +covers them. The merge-bot auto-merges **every** Dependabot tier including semver-major (no +ecosystem or update-type guard), the required CI checks are the gate, not the bump magnitude, so a +major that breaks the build fails its checks and never merges. + +**Why parallel dual-target rather than develop-only with eventual flow-through:** +push-distribution channels (HACS for Home Assistant integrations, Linux distros that vendor from +`main`, etc.) consume `main` directly. A develop-only model would leave `main` running stale code +during long-running develop features. Codegen content can also be production-critical (live +API-derived data, language lists, build catalogs) rather than just sample/demo content, so both +branches need fresh codegen on their own cadence. + +**Maintainer-pushed commits on a bot PR auto-disable auto-merge.** The merge-bot's +`merge-dependabot` and `merge-codegen` jobs only fire on `opened`/`reopened` events (auto-merge is +enabled exactly once per PR). When a maintainer pushes commits to a bot's branch (a `synchronize` +event with an actor that isn't the same bot), the merge-bot's +`disable-auto-merge-on-maintainer-push` job fires and calls `gh pr merge --disable-auto`. The +maintainer's commits stay in the PR but won't auto-merge with the bot's content. Re-enable +auto-merge manually (`gh pr merge --auto <PR>` or the GitHub UI) when ready. + +## Codegen determinism + +The codegen workflow is a mechanism to refresh files that are checked into the repo: it runs a +matrix over `main` and `develop`, each leg regenerating against its own checkout and opening its +own PR (`codegen-main -> main`, `codegen-develop -> develop`). For the two legs not to conflict on +`develop -> main`, the generated output must depend only on its inputs, never on per-invocation +state (timestamps, GUIDs, build IDs), which would diverge every run and conflict on every release. +**What** a repo regenerates (data files, source, or both) and **how** (download and process an +external source, transform local inputs, whatever) is entirely its own concern. The constraint is +only that the output be input-deterministic, not how it is produced. A repo adopting codegen +supplies its own input-deterministic generator and wires the codegen reference workflow +(`run-codegen-pull-request-task.yml` and its scheduler). + +## App-token workflows use Client ID, not App ID + +`actions/create-github-app-token` deprecated the numeric `app-id` input in v3.0.0. Use +`client-id: ${{ secrets.CODEGEN_APP_CLIENT_ID }}`. When adding new App-token call sites, use the +same form, and do not reintroduce `app-id` / `CODEGEN_APP_ID`. See `repo-config/README.md` +"Secrets" for which secrets each mechanism needs. diff --git a/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md b/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md new file mode 100644 index 00000000..f19b5cf3 --- /dev/null +++ b/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md @@ -0,0 +1,137 @@ +# Release Build and Publish Mechanics + +Full detail for the "Publishing" rules in `SKILL.md`. Load this when adding or removing a release +target, wiring a new leaf build task, deciding where a build output belongs (a GitHub Release +asset, a package-registry push, an image push, a deploy), or setting up a wrapper repo that tracks +an upstream release, not for reading the release model's shape (the SKILL.md summary covers that). + +## Reusable-task parameter contract + +Every `build-*-task.yml` and `build-release-task.yml` takes `ref` (git ref to check out/version), +`branch` (logical branch driving config/tags/prerelease, where `main` => Release/`latest`/ +non-prerelease, else Debug/`develop`/prerelease), and where relevant `smoke`. +**Branch-derived config keys off `inputs.branch`**: each run builds one branch, and the top-level +publisher passes `branch: ${{ github.ref_name }}`, which the tasks forward and read as +`inputs.branch` (not `github.ref_name`) for config/tags/prerelease. `get-version-task.yml` takes a +`ref` so NBGV versions the right branch. + +## Per-target subsetting + +`build-release-task.yml` is a hub-hosted task with per-target `enable_*` inputs, so a repo drops a +target by setting its `enable_<target>: false` at the caller stub rather than deleting a job: the +hub task carries the full job graph for every repo, and the caller stub's `with:` block is where +the target list is expressed. A repo still curates its path-filter entry in +`test-pull-request.yml`, and (for PyPI) the `publish-pypi` job in its own `publish-release.yml`, +since `id-token: write` belongs at that one entry point. CodeGen, versioning, badge, merge-bot, +and Dependabot are target-agnostic. + +## Orchestration vs. build: the override seam + +The pipeline splits into two layers. The **orchestration** layer is generic and is the +standardization baseline: `publish-release.yml` (single-branch publish plan), the `get-version` +task plus `github-release` job inside `build-release-task.yml`, `get-version-task.yml`, and the +aggregator shape of `test-pull-request.yml`. Within +`test-pull-request.yml`, only the `changes -> smoke-build -> check-workflow-status` aggregator +wiring and the ruleset-bound job name are verbatim orchestration, while the `unit-test` job and +the `dorny/paths-filter` entries are owned/per-target. The **build** layer is a hook: a composite +action at `.github/actions/build-<target>` the hub-hosted `build-release-task.yml` reaches. The +hub defaults require explicit project paths. A project needing more than a path override carries +its own hook. + +The contract that keeps the seam clean: **a target contributes files to the GitHub release by +uploading a workflow artifact named `release-asset-<branch>-<target>`.** The `github-release` job +collects every `release-asset-<branch>-*` artifact by pattern, so its `download-artifact` step +uses `pattern:`/`merge-multiple:`, **never an `artifact-ids:` that names a build job's output** +(the producing build jobs still appear in `needs` for sequencing). That makes the tag-the-commit +plus create-the-release plus attach-the-assets logic reusable **as-is** across repos. **This +name-pattern handoff is canonical for every repo, single-target included**: name your one asset +`release-asset-<branch>-<target>` and the verbatim `github-release` globs it. Do not switch a +single-target repo to an `artifact-id` output plus `download-artifact` `artifact-ids:`, which +looks tidier for 1:1 but forks the `github-release` download and breaks its verbatim carry. + +**What a repo still curates** (by design, not a leak): which `enable_<target>` inputs its caller +stub sets, per the per-target subsetting rule above. `build-release-task.yml` is hub-hosted +(`docs/reusable-workflows.md` "Stage 4: The Release Chain and the Docker Core"), so its job graph +and its `github-release` job are the hub's, not a per-repo file a caller edits. A repo adopting the +release chain carries only the caller stub in its own `publish-release.yml` and +`test-pull-request.yml`, naming the hub task by pin and setting the `enable_*`, `docker_image`, +and project-path inputs its targets need. + +## Map your outputs to the right seam + +Pick by where each artifact *goes*, not by language: + +- **Files attached to the GitHub Release** (zips, binaries, packaged libraries): a build-executable + or build-nuget hook per output, each uploading `release-asset-<branch>-<name>`. This is where the + .NET `dotnet publish` or `dotnet build` and package push lives. The hub default takes an explicit + project path, and a project needing different build behavior replaces the hook. A data-only + repo's own output (e.g. a symbol library) is not yet + expressible as a hub hook or an `enable_*` input, so it stays a carried leaf until the hub task + grows one. +- **Package-registry pushes** (NuGet.org, PyPI): the target both builds **and** publishes to its + registry. NuGet pushes from inside the build-nuget hook (OIDC trusted publishing through + `NuGet/login`, no stored API key) *and* also uploads a `release-asset-*` (.7z) for the GitHub + release. PyPI is split: the build-pypi hook only builds and uploads the + `pypilibrary-build-<branch>` artifact, and the separate `publish-pypi` job in the caller's own + `publish-release.yml` does the OIDC Trusted-Publishing upload (`id-token: write` is granted only + at that one entry point), and PyPI contributes **no** `release-asset-*`. +- **Image-registry pushes** (Docker Hub): `build-docker-task.yml`, hub-hosted like + `build-release-task.yml`, pushes multi-arch tags directly and contributes **no** + `release-asset-*`. The image set comes from a docker-prepare hook (the hub default emits the + single vanilla entry an `image` input implies). A multi-image or upstream-pinned repo carries its + own hook, and a shared base layer comes from a required docker-build-base hook with no hub + default. To publish the Docker Hub repository overview, the hub-hosted `publish-docker-readme-task.yml` + pushes a readme via `peter-evans/dockerhub-description` (single-repo by default, matrix per + image for multi-image repos), wired into `publish-release.yml` and gated to `main` both by the + caller's `branch` input and inside the task itself. A `docker-readme-transform` hook sets a + `readme-filepath` step output naming which file to push, defaulting to `Docker/README.md` if + present else `README.md` as-is, so a repo needs a hook only to render the file first or to + override that default. +- **Filesystem on a host the project owns** (a static site, a config tree): a deploy leaf builds + the tree and ships it over the repo's own transport, contributing **no** `release-asset-*`. It + is a **separate `workflow_dispatch`** from the release, so a redeploy of an unchanged commit + mints no tag, and its credentials come from a **per-environment GitHub Environment** rather than + the repository secret store. Its last step asserts what the host actually serves, the release id + and the environment, never that the transport exited zero. Retention at the destination is + bounded by a declared count, and one side is recorded as owning the prune: the deploy where its + credential can observe the destination, the host where that credential is deliberately + write-only. +- **Source-only / no build** (validate + tag + release): this seam does not apply. A source-only + repo carries **no** `build-release-task.yml` (its `appliesTo` excludes it), so there are no leaf + tasks and no `get-version`/`github-release` jobs to curate. Its whole release is + the standalone `publish-release.yml` on `workflow_dispatch`: a `validate` job (the repo's + reusable validation task) gates a publish job that **inlines** NBGV for the tag and + `action-gh-release` for the release (tag, auto source archive, README, LICENSE). + +`get-version-task.yml` installs the .NET SDK only because NBGV needs the runtime to compute the +version/tag, which is heavyweight but expected even for a non-.NET repo, and acceptable as-is. + +## No-op republish guarantee + +A weekly/dispatch publish where NBGV `SemVer2` is **unchanged** (no new commit since the last +publish) re-pushes **nothing** to GitHub Releases (the `github-release` job's `release-exists` +check skips the create step), NuGet (`dotnet nuget push --skip-duplicate`), or PyPI +(`gh-action-pypi-publish` `skip-existing: true`), since all three key on the version string. +**Docker always re-pushes** by design: it picks up upstream base-image refreshes (e.g. +`ubuntu:rolling`) that aren't visible in the repo. Boundary: `version.json` has **no +`pathFilters`**, so *any* commit, including a CI/workflow-only or docs-only change, advances the +NBGV git height and therefore `SemVer2`, and the next publish *does* create a fresh release for it +even when the shipped binary is byte-identical. This is accepted NBGV behavior, and `pathFilters` +are intentionally not added. + +## Wrapper repos that track an upstream release + +A repo wrapping an upstream release uses the hub-hosted `check-upstream-version-task.yml`: a +required `resolve-upstream` hook sets a `versions` step output, a **JSON object of +`name -> version`**, written to a committed state file at the **repo root beside `version.json`** +(default `upstream-version.json`, since it is a build-input version source, not GitHub-platform +config, so it does not belong under `.github/`), and opens a rolling App-signed bump PR per branch +that the merge-bot auto-merges (`merge-upstream-version`). The object carries one key for the +common single-version case (`{"version": "X"}`) or N keys for a wrapper that pins several upstream +components (e.g. an image plus a companion tool), and the build reads each component by key, and +the bump PR's title/body name only the keys that actually moved. Call it from a scheduled +entry-point workflow and matrix only the branches that ship the version (a CI-only version uses +`["develop"]`). A merged bump ships on the **next publish**, not immediately, which is the +two-phase latency tradeoff. A tracker whose bump needs a human decision instead of auto-merge, for +example one that snapshots a package list to review rather than a version to adopt outright, sets +`auto-merge: false`, which prefixes the head so no merge-bot rule matches it. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md new file mode 100644 index 00000000..2880da22 --- /dev/null +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -0,0 +1,144 @@ +--- +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 + 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 are implemented by + scripts/pr_review.py and bootstrapped by .github/copilot-instructions.md. This skill is the + contract those surfaces implement, not a replacement for them. +--- + +# PR Review Conduct + +## Why this exists + +`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 + +**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. For a finding on platform-specific code + (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per + `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent + elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. +2. **Not real, or real but structurally out of scope, so decline in the thread with evidence.** + Disprove a wrong finding with the command and its output, the code path that makes it + impossible, or the rule that governs it. A finding that is factually correct but not this + repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that + sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape + closes the thread on its own evidence. An assertion ("this is fine") does not close a finding, + a decline needs evidence the reviewer itself could check. +3. **Real, fixable here, but deliberately left as is, a value call rather than a scope + boundary, so it is the maintainer's, not the agent's.** Reach for this only once outcome 2 is + ruled out, since a scope boundary declines on its own evidence and never needs this outcome at + all. State the finding and why the fix is unwanted, and get an explicit answer in the same + turn, before moving to other work. A plan to ask later is resolution by silence the moment + attention moves elsewhere. If the maintainer is not reachable right now, leave the thread open + and say so, rather than treating the intention to ask as the asking. +4. **Real and worth doing later, so file the issue first, then reply with its link.** A deferral + noted only in a thread is lost the moment the PR merges. +5. **Keeps recurring, so fix the class, not the instance.** A finding raised repeatedly against + correct code means the code is not communicating something: add the comment, sharpen the name, + narrow the interface, or fix the rule if the rule is wrong. Bouncing the same point across + rounds is the signal to escalate the rule itself, not to keep re-arguing it. + +**A disposition decided on one PR does not carry to the next.** The same finding shape recurring +on a sibling repo or PR, even within one batch or one session, gets its own outcome: its own +evidence-backed decline (outcome 2) or its own explicit maintainer answer (outcome 3). A prior +instance's outcome is context for the new one, never a standing answer to reuse in its place. + +## 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 <SHA>`, +`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. Use `scripts/pr_review.py` from a hub checkout for +the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and +shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching +the finding's own words instead of a line number a fix push can move. The repository's +`.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable +coverage marker. Do not reconstruct the API operations by hand. diff --git a/.github/skills/python-codestyle/SKILL.md b/.github/skills/python-codestyle/SKILL.md new file mode 100644 index 00000000..911ae30c --- /dev/null +++ b/.github/skills/python-codestyle/SKILL.md @@ -0,0 +1,163 @@ +--- +name: python-codestyle +description: >- + Governs Python code style for ptr727/ProjectTemplate fleet repos: the build-versus-lint-only + profile split, the uv/ruff/pyright/mypy/pytest toolchain, src layout, formatting and linting, + comment and docstring conventions, type hints, naming, imports, patterns to avoid, test + conventions, and versioning. Use this whenever writing, reviewing, or editing a .py file, a + pyproject.toml, or a uv.lock, whenever deciding whether a Python subtree is a shippable project + or a lint-only scripts tree, whenever choosing pyright versus mypy for a repo's CI gate, or + whenever writing or reviewing a pytest test. Triggers even when the task looks like a small + local fix ("just add a helper function", "silence this lint warning", "add a dependency"), + because the profile split, the ruff-is-authoritative rule, and the ban on backward-compat + shims or impossible-case error handling are each easy to violate one file at a time. Applies + only to a repo's Python side, a repo with no Python has no use for this Skill. +--- + +# Python Codestyle + +## Why this exists + +This is the Python-specific half of the fleet's code style guide, kept in one place instead of +re-derived per repo or per session. CODESTYLE.md's General section still owns the rules every +language shares (clean-compile verification as a concept, the suppression-scope order, tooling +casing in prose), this Skill is everything specific to a Python project on top of that: the two +profiles, the toolchain, layout, and the language-level conventions. + +## Two profiles + +Read the repo's `pyproject.toml` shape and pick the profile before applying any other rule: + +- **build** (Project): `[project]` + `[build-system]` + committed `uv.lock`. Uses `uv run`, pytest, + pyright strict (or mypy where the repo requires it). +- **lint-only** (Scripts): no `[project]`, no lockfile. Uses `uvx`, unittest, mypy as CI gate. + +For the full profile specification and per-repo adaptation axes (type checker, dependency +declaration, versioning, VS Code config), see `references/profiles.md`. + +## Toolchain + +| Tool | Role | Config | +|---|---|---| +| [uv][uv-link] | env, deps, build, publish (build/publish only where the repo ships a package) | `pyproject.toml` `[dependency-groups]` or `[project.optional-dependencies]`, `uv.lock` | +| [hatchling][latest-link] | build backend (published packages) | `pyproject.toml` `[build-system]` | +| [ruff][ruff-link] | lint + format + import sort | `pyproject.toml` `[tool.ruff]` | +| [pyright][pyright-link] | type checker (the default, a strict baseline) | `pyproject.toml` `[tool.pyright]` | +| [mypy][mypy-link] | additional/alternate type checker (optional, the CI checker in a mypy-in-CI repo, required for Home Assistant) | `pyproject.toml` `[tool.mypy]` (or per home-assistant/core) | +| [pytest][docs-link] | test runner | `pyproject.toml` `[tool.pytest.ini_options]` | + +**Type checking targets strongly typed, deterministic code.** pyright in strict mode is the +default baseline on first-party code (a repo may instead run mypy in CI and keep pyright +editor-only via Pylance, per the next paragraph): `[tool.pyright]` `strict = ["src"]`, or the +integration package for a Home Assistant repo, with tests run in standard mode. pyright is the +anchor because Pylance embeds it, so the editor and the CLI/CI (`uv run pyright`) run the same +engine and never disagree. The standalone `ms-pyright.pyright` extension stays in +`unwantedRecommendations` because Pylance covers it. Relax strictness on third-party code only +when a dependency has no usable types and no alternative (e.g. `pandas`): a targeted, commented +`# pyright: ignore[...]` or a scoped `[tool.pyright]` override, never a blanket relaxation. + +**mypy is allowed, and required where the ecosystem demands it, it is not banned.** Running more +than one checker is normal when each serves a purpose (the .NET side pairs CSharpier and +`dotnet format` the same way), and pyright's inference and mypy's plugin ecosystem (e.g. +`pydantic.mypy`) catch different classes of error. A Home Assistant integration runs +`mypy --strict` because the platinum `strict-typing` quality-scale tier requires it, and a +pydantic-heavy library may opt in for the plugin. When a repo uses mypy it runs in CI and the +editor (the `ms-python.mypy-type-checker` extension) so the two stay consistent, and its mypy +command joins the clean-compile. A repo with no such need stays pyright-only, which is lighter and +inherently consistent. + +## Local development loop + +From inside the Python project directory: + +```sh +uv sync # creates .venv, installs deps + dev group +uv run ruff format # auto-format +uv run ruff check --fix # auto-fix lint +uv run ruff check # verify lint clean +uv run ruff format --check # verify format clean +uv run pyright # verify types +uv run pytest # run tests +uv build # produce wheel + sdist in ./dist (published packages only) +``` + +The Python clean-compile is `uv run ruff format` + `uv run ruff check` + the repo's type checker: +`uv run pyright`, or `uv run mypy src` where mypy is the CI checker, or both where the repo runs +both (see Type checking above). Run it, plus `uv run pytest`, before committing. These are +documented commands, and an optional VS Code tasks mirror (all `type: process`, no `&&` shell +chaining, so it runs the same on any task shell) is in the hub `vscode-tasks-python.json` snippet. +CI runs the same clean-compile commands as the authoritative backstop. Git hooks are opt-in, so +wire `pre-commit` for `ruff` and the type checker yourself if you want local enforcement. + +## Layout + +`src` layout, which keeps the package out of the repo root and prevents accidental imports of +unbuilt code: + +```text +<python-project>/ + pyproject.toml + README.md + uv.lock # committed for reproducible CI + src/ + <package_name>/ + __init__.py + _version.py # published packages; a source-only repo uses a static version instead + <modules>.py + tests/ + __init__.py + test_<module>.py +``` + +## Code style + +Key rules for every Python task: + +- **`ruff format` is authoritative.** Don't argue with the formatter. Configure in `pyproject.toml` + `[tool.ruff]`, not via inline `# fmt:` directives. +- **Run `ruff check --fix` before committing.** The configured rule families are in + `[tool.ruff.lint]` `select`. Add new rule families project-wide, not scattered inline `# noqa`. +- **`# noqa` is a last resort.** Scope it narrowly (`# noqa: E501`) with a comment. Recurring + false positives belong in `[tool.ruff.lint]` `ignore` or `per-file-ignores`. +- **All public APIs are typed.** Use modern syntax (`list[int]`, `X | None`). Don't add + `# type: ignore` without an explaining comment. +- **Don't add backward-compat shims.** Just delete unused code. Git history is the audit trail. +- **Don't add error handling for impossible cases.** Trust internal code. Validate only at boundaries. + +For comments, docstrings, full type-hint rules, naming, imports, and all patterns to avoid, see +`references/code-style.md`. + +## Tests + +`uv run pytest`. One test file per module (`test_<module>.py`), fixtures over setup/teardown, +fakes over mocks. Test the docstring's contract, not implementation details. See +`references/testing.md` for the full conventions. + +## Versioning + +Published packages use `_version.py` with `__version__ = "0.0.0"` as a placeholder. Wire +`hatch-vcs` or equivalent to increment, publish with `skip-existing: true`. Source-only repos use +a static `version` in `[project]` with no `_version.py`. See `references/profiles.md` for details. + +## Linter cleanliness + +Before pushing or opening a PR: + +- VS Code's Problems pane should be quiet for the files you touched. The relevant linters are ruff + (via the `charliermarsh.ruff` extension) and pyright (via the `ms-python.python` extension's + bundled Pylance). +- The CI gate is `uv run ruff check`, `uv run ruff format --check`, the repo's type checker + (`uv run pyright` or `uv run mypy src`), and `uv run pytest`, the same commands as the local + loop above, run from the Python project directory (invoked as separate steps, not `&&`-chained, + so the runner shell is irrelevant). +- Markdown in this directory follows CODESTYLE.md's repo-wide Markdown and Spelling rules, + packaged as the `comment-and-doc-style` Skill. + +<!-- External --> + +[docs-link]: https://docs.pytest.org/ +[latest-link]: https://hatch.pypa.io/latest/ +[mypy-link]: https://mypy-lang.org/ +[pyright-link]: https://microsoft.github.io/pyright/ +[ruff-link]: https://docs.astral.sh/ruff/ +[uv-link]: https://docs.astral.sh/uv/ diff --git a/.github/skills/python-codestyle/references/code-style.md b/.github/skills/python-codestyle/references/code-style.md new file mode 100644 index 00000000..b7e62b00 --- /dev/null +++ b/.github/skills/python-codestyle/references/code-style.md @@ -0,0 +1,80 @@ +# Python Code Style: Full Reference + +## Formatting and linting + +- **`ruff format` is authoritative.** Don't argue with the formatter, and if it reformats your + code, that's the final form. Configure (line length, target version) in `pyproject.toml` + `[tool.ruff]`, not via inline `# fmt:` directives. +- **Run `ruff check --fix` before committing.** Most ruff lint rules have safe autofixes, let the + tool handle them. The configured rule families are listed under `[tool.ruff.lint]` `select`. Add + new rule families project-wide rather than scattering inline `# noqa` markers. +- **`# noqa` is a last resort.** When you must use one, scope it narrowly (`# noqa: E501`, not + bare `# noqa`) and add a short comment on the same line explaining why. False-positive patterns + that recur across the codebase belong in `[tool.ruff.lint]` `ignore` or per-file + `[tool.ruff.lint.per-file-ignores]`, with a comment. Porting an existing codebase is not a + license to add `ignore` / `per-file-ignores` blocks to mute newly surfaced lint. Fix it. + +## Comments + +- **Inline `#` comments**: keep tight and local. One line is preferred, but multi-line is fine + when you need to document a non-obvious implementation constraint, a local trade-off, or + coupling that future edits could easily break. Keep that rationale next to the affected block so + the reviewer/maintainer sees it at edit-time. +- **Don't explain what the code does.** Well-named identifiers handle that. Don't reference the + current task ("added for X", "used by Y"), which belongs in the PR description. + +## Docstrings + +- Follow [PEP 257][pep-0257-link]. Focus docstrings primarily on the behavior contract (what + callers and tests can rely on), public semantics, and edge-case expectations. + Implementation-local rationale belongs in inline `#` comments, not docstrings. +- A short one-liner is fine for trivial functions and tests with self-documenting names. +- For non-trivial behavior (non-obvious test scenarios, contracts a test pins, edge cases callers + must know about, design trade-offs that are load-bearing for future maintainers), write a + one-line summary, blank line, then a details paragraph. Multi-paragraph docstrings are fine when + the contract earns it. +- Design notes belong in the code (docstrings or inline comments). They do NOT belong in + `HISTORY.md`, which is end-user release notes, not a design log. + +## Type hints + +- **All public APIs are typed.** The repo's configured type checker runs on `src/` (pyright strict + via `[tool.pyright]` `strict = ["src"]`, or mypy where that is the CI checker), and tests run in + the checker's looser/standard mode. +- **Use modern syntax**: `list[int]` not `List[int]`, `dict[str, X]` not `Dict[str, X]`, + `X | None` not `Optional[X]`, `from __future__ import annotations` only when needed for forward + references. +- **Don't add `# type: ignore` to silence pyright errors without a comment** explaining the + constraint. If a recurring false positive needs suppression, configure it project-wide in + `[tool.pyright]`. A new port doesn't change this, fix freshly surfaced type errors rather than + muting them. + +## Naming + +- `snake_case` for functions, methods, variables, modules, package directories. +- `PascalCase` for classes, type aliases, type vars, enum members. +- `UPPER_SNAKE_CASE` for module-level constants. +- Single leading underscore for module-private, double leading underscore for name-mangled (rare, + and usually means rethink the design). + +## Imports + +- **Let ruff sort imports.** `[tool.ruff.lint]` `select` includes the `I` rule family + (isort-equivalent). Don't hand-sort. +- Standard library first, then third-party, then first-party (the project itself), each block + separated by a blank line, which ruff enforces automatically. +- Avoid wildcard imports (`from x import *`) outside `__init__.py` re-exports. + +## Patterns to avoid + +- **Don't add backward-compat shims, `# removed` markers, or rename-to-`_` for unused vars**, just + delete. Git history is the audit trail. +- **Don't add error handling for impossible cases.** Trust internal code, and validate only at + boundaries (user input, parsed config, external APIs). +- **Don't use exceptions for expected control flow.** Exceptions are for unexpected states. +- **Don't suppress errors silently** (`except Exception: pass`). Either handle the specific + exception and document why it's safe, or let it propagate. + +<!-- External --> + +[pep-0257-link]: https://peps.python.org/pep-0257/ diff --git a/.github/skills/python-codestyle/references/profiles.md b/.github/skills/python-codestyle/references/profiles.md new file mode 100644 index 00000000..b7eedfb9 --- /dev/null +++ b/.github/skills/python-codestyle/references/profiles.md @@ -0,0 +1,74 @@ +# Python Profile Details + +## Adapt before propagating + +The rules in `SKILL.md` describe the default Python profile: a package that publishes to PyPI, +type-checked by pyright in strict mode, dependencies in `[dependency-groups]`. A derived repo +often differs, and when it does, adapt these fields to match the repo's actual toolchain rather +than copying verbatim (a verbatim copy that misdescribes the repo is inaccurate and gets rejected +in review). The axes that commonly vary per repo: + +- **Type checker in CI**: pyright strict, mypy in CI with pyright editor-only (Pylance), or both. + Whichever runs in CI is the one the clean-compile and the CI gate invoke. +- **Dependency declaration**: `[dependency-groups]`, or PEP 621 `[project.optional-dependencies]` + (dev tools installed with `uv sync --extra <group>`). +- **Versioning / publishing**: a published package (`_version.py` plus a version source, + `uv build`, and a PyPI publish step), or a source-only repo with a static `version` and no + publish step (see Versioning below). +- **Disabled markdownlint rules**: repo-specific, `.markdownlint-cli2.jsonc` at the repo root is + the source of truth, not any example rule named here. +- **VS Code config home**: editor settings/extensions may live in `.vscode/*.json` or the + `<Repo>.code-workspace`, while tasks/launch/debug configs can only be external `.vscode/*.json` + (they cannot live in the workspace file). The repo's own `tasks.json` sits wherever it keeps it, + and the canonical task definitions it is written against are the hub `vscode-tasks-python.json` + snippet, which resolves the same way from every repo. + +## Two profiles: full specification + +A repo's Python is one of two shapes, declared as the `build` or `lint-only` profile and validated +against the `pyproject.toml` shape. Most of the `SKILL.md` rules (uv project, `uv.lock`, `uv run`, +src layout, pytest coverage) describe the Project shape (the `build` profile). The two differ by +whether the Python has third-party runtime dependencies, which shows up structurally in +`pyproject.toml`, so the fleet's audit reads the shape there: + +- **Project** (the `build` profile): the Python has third-party runtime dependencies, or is the + repo's deliverable. It is a PEP 621 uv project: `[project]` with `dependencies` (dev tools in + `[project.optional-dependencies]` or `[dependency-groups]`), a `[build-system]`, and a committed + `uv.lock` (pinned LF, per GOVERNANCE.md's "Line Endings" section). CI runs `uv sync --frozen` + + `uv run <tool>`, so the lockfile pins tool versions. +- **Scripts** (the `lint-only` profile): stdlib-only utility scripts embedded in a non-Python repo + (e.g. a Python tooling subtree of a `csharp` app). Run the tools with `uvx` (no project install, + no lockfile): the `pyproject.toml` carries only tool config (`[tool.ruff]`, `[tool.mypy]`, and + an optional `[tool.pyright]` editor block), with no `[project]`, no `[build-system]`, and no + `uv.lock` (that metadata would misrepresent it as a shippable package). mypy is the type-check + gate (there is no first-party package for pyright strict to anchor on), and a `[tool.pyright]` + block in standard mode keeps Pylance quiet in the editor, the same mypy-gate/pyright-editor + split the build profile uses. There is no lockfile, and a `uvx <tool>@<ver>` pin in a `run:` + step is not something Dependabot tracks, so CI runs `uvx ruff@latest` / `uvx mypy@latest` rather + than a manual pin that would silently go stale. The fleet rule is to pin only what Dependabot + auto-updates (SHA-pinned actions, package deps) and otherwise run latest, so the VS Code tasks, + README, and CI all run the unpinned latest here. `.py` files follow the repo's LF line-ending + default (per GOVERNANCE.md's "Line Endings" section). There is no pytest suite, and `unittest` is + the runner instead. A script that carries a gate still earns tests, written with the standard + library's `unittest` so they run under bare `python3` with nothing installed, as + `test_<script>.py` under a `tests/` directory beside the scripts it exercises + (`<scripts-dir>/tests/`), kept apart so a test never reads as a tool. Within the scripts + directory the name carries the kind: a gate that checks and exits non-zero on a finding takes a + `_lint` or `_gate` suffix, and a utility that does work takes none. Any repo carrying Python + carries the Python tooling in CI, coverage included, this profile too: `uvx ruff@latest check`, + `uvx ruff@latest format --check`, `uvx mypy@latest`, and the unittest suite under + `uvx coverage@latest run -m unittest discover -s <scripts-dir>/tests` with `coverage report`, + informational with no threshold adopted. A co-present `csharp` type still carries `codecov.yml` + for its own tests. + +## Versioning + +**Published packages.** `_version.py` ships with `__version__ = "0.0.0"` as a placeholder. Until +you wire `_version.py` to something that increments (the usual options are `hatch-vcs`, a +version.json bridge, or manual bumps), no new PyPI versions will land, and publishing with +`skip-existing: true` keeps a stuck placeholder version from failing the run. + +**Source-only repos** (no PyPI publish, with a source-release on dispatch or no release at all) do +not need `_version.py`: keep a static `version` in `pyproject.toml` `[project]`, or let the +release pipeline's version source (e.g. NBGV plus `version.json`) own the tag. There is no publish +step to guard, so `skip-existing` does not apply. diff --git a/.github/skills/python-codestyle/references/testing.md b/.github/skills/python-codestyle/references/testing.md new file mode 100644 index 00000000..c19ff9d7 --- /dev/null +++ b/.github/skills/python-codestyle/references/testing.md @@ -0,0 +1,13 @@ +# Python Testing Conventions + +Use `pytest` with configuration in `[tool.pytest.ini_options]`. Default invocation: +`uv run pytest`. + +- One test file per module under test, named `test_<module>.py`. +- Test functions named `test_<scenario>_<expected_behavior>`, descriptive and not numbered. +- Use fixtures (defined in `conftest.py` for shared ones, or per-test for narrowly-scoped) instead + of setup/teardown methods. +- **Avoid mocking when fakes work.** Hand-rolled fakes that implement the protocol you depend on + are usually clearer and break less than `unittest.mock` magic. +- **Test edge cases that the docstring promises**, not implementation details. If the test breaks + when you refactor without changing behavior, the test is asserting on an implementation detail. diff --git a/.github/skills/repo-worktree/SKILL.md b/.github/skills/repo-worktree/SKILL.md new file mode 100644 index 00000000..82bf9d30 --- /dev/null +++ b/.github/skills/repo-worktree/SKILL.md @@ -0,0 +1,134 @@ +--- +name: repo-worktree +description: >- + Mandates and mechanizes task isolation in ptr727/ProjectTemplate fleet repos: every task, + including a continuation of a prior session's task, creates its own git worktree on its own + feature branch before its first file edit, based on the branch work starts on (develop on both + fleet workflow models unless the task is explicitly about main-only content, never whichever + branch a tool defaulted to). Also wraps the mechanics: + creating a worktree with git worktree add, the fleet layout convention, listing what is in + flight, and removing a worktree and its branch after merge. Use this whenever about to create + or edit files in a fleet repo, whenever starting or resuming a task, whenever the task's + branch is already checked out in a shared checkout, and whenever creating, listing, or + removing a worktree. Triggers even when the session was launched in the primary checkout or + the change looks like a one-line fix, because the primary checkout is the maintainer's own + surface and the incident this guards against was two sessions sharing one checkout, each + session's blanket add committing the other's uncommitted files. +--- + +# Repo Worktree + +## Why This Exists + +Two agent sessions once ran concurrently in the same primary checkout, on the same feature +branch, neither knowing the other was in the tree. One session's commits swept in the other +session's uncommitted files, so two commits landed carrying work their subjects never mention, +committed by a task that never saw it. No rule fired at the moment it was violated, which is the +first file edit: the commit-time and review-time skills all run after a sweep has already +happened. This skill is that missing task-start surface. `GOVERNANCE.md` "Repository Boundaries +and Write Safety" keeps the isolation law and wins on any disagreement, and the mechanics below +are this skill's own content. + +## The Mandate + +- **Every task isolates into its own worktree before its first file edit.** All new work begins + by creating a unique worktree (or clone) on its own feature branch. The primary checkout is + the maintainer's own surface, so a session launched there isolates before writing rather than + after noticing contention. +- **A continuation re-isolates.** A session resuming prior work finds its branch already checked + out somewhere and naturally resumes there, and that instinct is the hazard: a branch sitting + checked out in a shared tree is exactly how two sessions end up in one checkout. Create a + fresh worktree for the continuation and check the branch out there. +- **The moment is the first file edit, not the commit.** By commit time another task's + uncommitted work can already be swept into the staging area, so isolating late protects + nothing. Reading anywhere is fine, and the worktree exists before the first write. +- **Someone else's tree stays theirs.** A branch that changes when nothing you did changed it, + or an edit of yours reverted with no conflict, means another task is live in that tree, and + the response is to stop rather than to re-apply the edit, per `GOVERNANCE.md` "Repository + Boundaries and Write Safety". + +## The Base Branch + +Base the worktree on the branch work starts on for the repository's model, not on whichever +branch a tool defaulted to. GitHub's own "default branch" setting reads `main`, but on both +fleet workflow models work starts on `develop`, so a worktree defaulted to "the default branch" +lands on `main` and silently misses everything merged to `develop` but not yet promoted. Branch +from `develop` unless the task is explicitly about `main`-only content, per `GOVERNANCE.md` +"Branching Model". Fetch immediately before creating and base on the remote ref, because a clone +is whatever it last fetched rather than the branch it names. + +## Creating a Worktree + +The fleet layout convention keeps every base clone and every in-flight task visible in one +place: + +```text +~/repos/<Repo> base clone, on its default/working branch +~/repos/worktrees/<Repo>-<task-slug> one worktree per in-flight task, own branch +~/repos/upstream/<owner>-<repo> clone of a repo under another owner, not a fork +``` + +The top level carries no owner segment because everything in it is the fleet owner's own, an +original repo and a fork alike. A fork is named `<upstream-owner>-<upstream-repo>` at fork time, +so a fork of `acme/core` is `acme-core`, and its name identifies the upstream project and stays +unique in the flat namespace without an owner segment of its own. A repository adopted as the +owner's own work rather than kept as a fork is detached from its parent and keeps a plain name, +`widget` rather than `initech-widget`, since it no longer tracks anything upstream. + +A clone of a repository under another owner is neither of those, and flattening one collides +rather than merely reading oddly: `acme/core` joined the way a fork is joined **is** the fork's +name, `acme-core`, while reduced to a bare `core` it names no project and collides with the next +`core` cloned from any other owner. Those clones live one level down under `upstream/`, named by +that same join, so `upstream/acme-core` sits beside the fork it would otherwise land on. The +segment states the relationship rather than the owner, so a reference checkout is told from a +working repo without a `git remote` call, and the names under it never compete with the flat +namespace above. The join is ambiguous in the abstract, since a hyphen in either half means +`acme-labs/core` and `acme/labs-core` produce one name, and it is kept anyway because it is the +fork convention's own join: the ambiguity is inherited from the flat namespace above rather than +introduced here, and it surfaces at clone time as a directory that already exists, where the +second clone takes a hand-picked name. A worktree off one of them keeps the flat worktrees path +under the same name, `~/repos/worktrees/<owner>-<repo>-<task-slug>`. Contributing a change from +such a clone is never a push out of it: fork the upstream first, per the +`upstream-contribution-workflow` skill, and that fork's own clone then belongs in the flat +namespace above, under the name this one already has. + +```sh +git -C ~/repos/<Repo> fetch origin develop +git -C ~/repos/<Repo> worktree add ~/repos/worktrees/<Repo>-<task-slug> -b <task-branch> origin/develop +``` + +A continuation attaches the task's existing branch rather than forking a fresh one: + +```sh +git -C ~/repos/<Repo> fetch origin <task-branch> +git -C ~/repos/<Repo> worktree add ~/repos/worktrees/<Repo>-<task-slug> <task-branch> +``` + +When the base clone holds only the remote-tracking ref, the same command creates the local +branch tracking `origin/<task-branch>` through git's ordinary checkout guessing, so a fresh +clone needs no separate branch setup. Git refuses to attach a branch that is already checked +out somewhere else, and that refusal is the mandate working, since the branch sitting checked +out in a shared tree is the hazard the continuation rule exists for. Return that checkout to +its own working branch first when its tree is clean, and stop when it is not, because a dirty +tree there may be another task's uncommitted work. + +A machine not yet migrated to this layout still isolates exactly the same way, since the mandate +is the isolation rather than the path: create the worktree beside whatever layout the machine +has, and note that the base clone may live elsewhere than `~/repos/<Repo>`. + +Claude Code's own `EnterWorktree` tool acts only on an explicit instruction from the user or the +project instructions, which is why the carried rules state this mandate in so many words. Given +a `name`, it creates the worktree under `.claude/worktrees/` inside the repo and bases it on the +GitHub default branch, which is the wrong path and the wrong base here. Create the worktree with +`git worktree add` as above, then attach with `EnterWorktree` `path:`, not `name:`. + +## Listing and Cleanup + +- `git worktree list`, run in any checkout of a repo, names that repo's base clone and every + worktree with its branch. On the convention layout, one `ls ~/repos/worktrees/` reads what is + in flight across the whole fleet. +- After the task's pull request merges, remove the worktree and its branch from the base clone: + `git worktree remove ~/repos/worktrees/<Repo>-<task-slug>`, then `git branch -d <task-branch>`. +- A worktree that refuses removal is dirty, and force is not the fix: look at what is + uncommitted in it first, since discarding uncommitted work runs only on explicit instruction, + per the `git-commit-conventions` skill. diff --git a/.github/skills/resync-a-repo/SKILL.md b/.github/skills/resync-a-repo/SKILL.md new file mode 100644 index 00000000..308dcbc9 --- /dev/null +++ b/.github/skills/resync-a-repo/SKILL.md @@ -0,0 +1,80 @@ +--- +name: resync-a-repo +description: >- + Drives RESYNC.md's procedure for bringing a ptr727/ProjectTemplate fleet repo that is already + stood up back into line with the current hub, run from a hub checkout against a named target + repo. Use this whenever asked to resync, sync, converge, or bring a specific repo up to date + with the hub, or to run a conformance sweep against a named repo and apply what it finds. Needs + a hub checkout and a named target repo to mean anything, so it does not usefully trigger from + inside a downstream repo's own session with no target named and no hub checkout present, that + case is fleet-conformance-check instead. Triggers even when the request sounds routine, such as + "just copy AGENTS.md over" or "make repo X match the hub," because that phrasing is exactly how + the AGENTS.md-overwrite incident happened. +--- + +# Resync a Repo + +## Why this exists + +RESYNC.md's own apply order already sequences the remedies so the rules land before the files +they govern and a deletion lands before the re-vendor that would otherwise refresh it. The +AGENTS.md-overwrite incident happened inside that same procedure, on the step that looked most +routine. This skill exists so the mandatory check survives contact with a real, time-pressured +resync instead of depending on an agent remembering to run it unprompted. It is a driver over +RESYNC.md, not a replacement for it. Read RESYNC.md itself for the deletion sweep, the +letters-versus-drift routing, the settings and ruleset step, and everything else that does not +change from one resync to the next. + +## Confirm the procedure before starting + +Read RESYNC.md section 0. A repo with no instruction set at all, or a partial one, is not this +skill's job, it is STANDUP.md sections 1A and 2 instead, since an absent carried file is a +baseline that never arrived rather than drift to converge. Run `spec/audit.py <RepoName>`, the +target's `registry/repos.json` `name` field rather than an `owner/repo` slug or a checkout path, +and read whether the findings are letters (absent) or drift (present but stale) before doing +anything else. The finding kind names the procedure the repo is owed. + +## Reach the hub and measure before changing anything + +Fetch a hub checkout of your own immediately before reading it, per RESYNC.md section 1, since a +stale clone answers confidently instead of failing, and verify the host with +`python3 scripts/host_gate.py --repo <path-to-target-checkout>`. Then run the audit end to end, +RESYNC.md section 2, against the target's `main` branch, never `develop`. A finding is a snapshot, +so quote the run stamp in anything derived from it and re-run before acting on a finding read +earlier in the session. + +## Apply, in this order + +1. **The instruction set first.** `AGENTS.md` and `GOVERNANCE.md` verbatim sections, then + `CODESTYLE.md` and `WORKFLOW.md`, including the `AGENTS.md` skill-dependency pointer paragraph + (naming `scripts/skills_install.py` and where the fleet's Skills live) as one more verbatim + unit carried in this same step, not a separate pass. **Before any verbatim re-vendor in this + step, run the `carried-instruction-file-guard` skill's distinctive-phrase probe against the + target file, every time, without exception.** This is not advisory language to weigh against + how routine the diff looks, a diff that looks routine is exactly the shape the + AGENTS.md-overwrite incident took. Do not proceed to the re-vendor until the probe has run and + any local addition it finds has a destination, per that skill's own procedure. +2. **Deletions second, before any re-vendor.** Only a `retire` disposition in + `spec/divergences.json` authorizes removing a file, and the removal is swept tree-wide, per + RESYNC.md section 4, before the deletion counts as done. +3. **Verbatim re-vendors** for everything the probe in step 1 cleared. A finding classified + modified rather than stale gets its diff read before being overwritten, since it may be an + improvement the hub should adopt instead of a mistake to erase. +4. **Interface workflows.** Honor the named contract, required jobs, the ruleset-bound check name, + the artifact-name handoff, rather than copying bytes. +5. **Settings, rulesets, and secrets.** Run + `repo-config/configure.sh check <owner>/<repo> release|operational` against the repo by name, + then `apply` for what it reports, never from a carried copy. +6. **Intent files last, and by hand,** since nothing mechanical judges these. + +Reconcile the registry entry (`status`, `types`, `releaseTrigger`, `workflowModel`, +`driftNotes`) in the same pass, and delete a `driftNote` describing work this pass just finished +rather than leaving it standing. + +## Ship it + +One focused pull request per drift class, branched from the target's `develop`, never a direct +push to a protected branch and never a hand edit outside a pull request. Close the review loop, +per the `pr-review-conduct` skill, before asking the maintainer for merge permission. The +maintainer merges, the agent drives to green and stops. Re-run the audit after the merge and +commit the report, done means measured, not applied. diff --git a/.github/skills/shell-codestyle/SKILL.md b/.github/skills/shell-codestyle/SKILL.md new file mode 100644 index 00000000..50b1f604 --- /dev/null +++ b/.github/skills/shell-codestyle/SKILL.md @@ -0,0 +1,55 @@ +--- +name: shell-codestyle +description: >- + Governs Bash/shell script style for ptr727/ProjectTemplate fleet repos: when a bootstrap or + host-tool script may be shell instead of Python, the mandatory set -Eeuo pipefail header, the + pipefail-versus-early-reader pitfall, self-locating scripts, shellcheck cleanliness, and the + why-not-what comment rule. Use this whenever writing, reviewing, or editing a .sh file, whenever + deciding whether a new script should be Bash or Python, or whenever a pipeline built from + `curl`/`grep`/`jq`-style commands looks like it silently swallowed a failure. Triggers even when + the task looks like a one-line tweak to an existing script, because a missing `-e`/`pipefail`, + or a reader piped straight from a producer that closes the pipe early, are each invisible until + the exact failure mode they guard against actually happens. Fleet-wide: a shell script can + appear in any repo (a bootstrap that installs the interpreter, a host tool that must run before + a toolchain exists), not only a repo whose primary language is shell. +--- + +# Shell Codestyle + +## Why this exists + +This is the shell-specific half of the fleet's code style guide, kept in one place instead of +re-derived per repo or per session. Shell is the fleet's exception language, reached for only +where Python cannot run yet, so its rules exist to keep that narrow surface safe rather than to +cover general scripting style. + +## When shell, not Python + +Bash, and only where a program cannot be Python: a bootstrap that installs the interpreter cannot +be written in it, and a host tool that must run before a development toolchain exists cannot +depend on one. Everything else is Python, with a test under the scripts tree's `tests/` directory. + +## Rules + +- **`set -Eeuo pipefail`, before the first command the script runs.** A header comment sits above + it, as `repo-config/configure.sh` and the `host-setup/` scripts do, since what matters is that + nothing executes unguarded rather than which line number it lands on. Without `-e` a failed + command in the middle of a sequence lets the rest run against a state nobody checked, and + without `pipefail` a pipeline reports the exit of its last stage, so a fetch that failed reads + as an answer when a parser downstream succeeds on an empty input. `-E` carries an `ERR` trap + into functions and command substitutions, so a script that later adds one is not surprised by + where it does not fire. +- **A reader that stops early needs its producer read first.** Under `pipefail`, a producer + writing to a closed pipe exits non-zero, so `curl ... | grep -q` reports a successful fetch as + a failure whenever the match is found early enough. Capture the output, then search it. +- **Self-locating, never dependent on the caller's directory.** A script resolves its own + directory from `BASH_SOURCE` and references its payloads through it, since the working + directory at invocation is not a property of the script. +- **`shellcheck` clean, and a deliberate exception carries its reason inline.** A + `# shellcheck disable=SCxxxx` names why the rule does not apply here, so the next reader can + tell a considered exception from an unread warning. `repo-config/configure.sh` is the worked + example, carrying five `SC2016` disables where a single-quoted `jq` program must stay + unexpanded, each with its reason on the same line. +- **Comments say why, never what.** The code states what it does. A comment restating it goes + stale silently, where a comment carrying a reason fails visibly when the reason stops being + true. diff --git a/.github/skills/skill-lifecycle/SKILL.md b/.github/skills/skill-lifecycle/SKILL.md new file mode 100644 index 00000000..720c59ac --- /dev/null +++ b/.github/skills/skill-lifecycle/SKILL.md @@ -0,0 +1,49 @@ +--- +name: skill-lifecycle +description: >- + Governs the lifecycle of the fleet's own skills in ptr727/ProjectTemplate: creating, changing, splitting, and retiring a skill under .agents/skills/, the source-versus-generated split with .github/skills/ and .claude-plugin/, the regenerate and --check semantics of scripts/build_dist.py, the install and stamp semantics of scripts/skills_install.py, the doc-packaging pattern that keeps a law doc and its skill in agreement, and the trigger-description conventions that make a skill fire. Use this whenever about to create, edit, move, or delete anything under .agents/skills/, .github/skills/, or .claude-plugin/, whenever packaging a doc or a doc section as a skill, and whenever deciding whether a topic deserves a skill at all. Triggers even when the edit looks trivial, such as fixing a typo in one SKILL.md, because the generated distributions desync the moment the source changes without a build_dist.py run, and CI fails the pull request on exactly that. Hub-context only, since .agents/skills/ exists only in the hub. +--- + +# Skill Lifecycle + +## Why This Exists + +The agent most likely to get a skill wrong is the one editing a skill, and before this skill existed nothing watched that moment: the regenerate and install semantics lived in `scripts/` docstrings and scattered prose, so the procedure was rediscovered per session. The two standing hazards are mechanical and silent. A hand-edit to the generated `.claude-plugin/` tree is overwritten by the next regenerate, and a source edit without a regenerate ships a plugin that no longer matches its source, which the CI `--check` gate fails rather than anyone noticing in review. + +## The Pipeline + +- **`.agents/skills/<name>/SKILL.md` is the only hand-authored source**, with optional `references/` and `scripts/` directories beside it. Codex and opencode read this tree directly, project-local, and also read the global `~/.agents/skills/` copy the installer materializes. +- **Generated distributions serve GitHub Copilot and Claude Code.** `scripts/build_dist.py` generates `.github/skills/` for GitHub Copilot and a Claude-plugin-compatible copy at `.claude-plugin/fleet-skills/`, published through `.claude-plugin/marketplace.json`. Neither generated tree is hand-edited, and `build_dist.py --check` exits non-zero when either tree differs from `.agents/skills/`. +- **The skill set is implicit.** Every `.agents/skills/<name>/` directory carrying a `SKILL.md` is a skill, and the generated `plugin.json` derives its list from those directories, so adding or retiring a skill edits no manifest by hand. `marketplace.json` names the plugin, not the skills, and is untouched by ordinary lifecycle work. +- **`scripts/skills_install.py`, run from a hub checkout, installs both forms per machine**: an overlay copy into `~/.agents/skills/` for Codex and opencode, marked per skill so a retired skill is removed on the next run and a foreign skill is never touched, and a user-scope plugin install for Claude Code via the `claude` CLI. Each run stamps the hub commit into `~/.agents/skills-install-stamp.json`, and `--report` reads that stamp against the checkout and exits non-zero when the machine is behind. The install is global per user, and per-repo pinning is a settled non-goal (`docs/fleet-map.md` "Skills Install Model"). + +## Deciding a Topic Deserves a Skill + +A skill surfaces at a trigger moment. A rule that binds every action all the time, or a short reference section a task reads once, gains nothing from being one: the always-on layer is the carried instruction set (`AGENTS.md` and the sections it maps), and packaging it as a skill duplicates it and spends the tokens the delegation rules exist to save. The `AGENTS.md` "Where the Rules Live" map records the disposition either way, a skill annotation on the row or the deliberate absence of one, so a topic with no skill reads as a decision rather than an oversight. + +## Creating a Skill + +1. **Name the directory in kebab-case** and set the frontmatter `name:` to the same string. +2. **Write the `description:` to carry the trigger**, since it is the only part an agent reads before deciding to load the skill: state what the skill governs, then the concrete moments it applies ("Use this whenever..."), then the routine phrasings that precede the failure it guards against ("Triggers even when..."), naming a real incident where one exists. Disambiguate against sibling skills by name, the way `standup-a-repo`, `resync-a-repo`, and `fleet-conformance-check` each state which of the three a session is in. +3. **Author the body per the `comment-and-doc-style` skill**: LF (the repo default), present tense, ASCII tiers, no semicolon in prose. Name hub paths as plain code spans rather than repo-relative links, because an installed copy resolves no repo path, and say "from a hub checkout" for anything the reader must run. +4. **Split bulk into `references/`** when the source doc is large: the SKILL.md carries the summary and the binding rules, and each `references/*.md` carries one topic read on demand, the shape `comment-and-doc-style` uses. +5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. +6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then commit the source and both generated trees in one commit. CI runs `--check` on every pull request and fails a desynced distribution. `python3 scripts/tests/test_build_dist.py` covers the generator itself. +7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. +8. **Refresh the machines after merge**: re-run `python3 scripts/skills_install.py` per machine, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then every machine serves the previous skill set, which `--report` says. + +## Changing or Retiring a Skill + +- **Edit only the source tree.** Any skill-content change under `.github/skills/` or `.claude-plugin/` that did not come from a `build_dist.py` run is a defect, whatever it fixes. +- **Retiring is deleting the source directory and regenerating.** The derived `plugin.json` list shrinks with it, and the installer's per-skill markers remove the retired skill from `~/.agents/skills/` on each machine's next run. +- **A deletion sweeps the prose that references the skill**, in the same change rather than as follow-up: the `AGENTS.md` map row or paragraph naming it, any law-doc packaging pointer to it, and any sibling skill that disambiguates against it. A law-doc section that had moved its full rules into the skill takes them back, or is retired with it, so no rule is silently lost with the skill that carried it. +- **Renaming is a retire plus a create** as far as the installer's markers and the plugin list are concerned, so sweep references the same way. + +## The Doc-Packaging Pattern + +Packaging keeps one topic in one authoritative place while the skill makes it surface automatically. It has two shapes, and each pairing states which it uses: + +- **Moved content.** The law-doc section keeps a summary and the skill holds the full rules (`git-commit-conventions`, `comment-and-doc-style`, `pr-review-conduct`). The section ends with the standard pointer sentence: packaged as the named skill at `.agents/skills/<name>/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 full rules. +- **Kept authority.** The source doc keeps the full rules and the skill is the summary that routes to them (`audit-a-repo` over `AUDIT.md`, `workflow-ci-contract` over `WORKFLOW.md`, `agent-conduct` over its GOVERNANCE sections). The skill states per topic which doc section owns it. + +In both shapes the doc wins on any disagreement, and the skill is what needs fixing. A rule stated fully in both places is the drift this pattern exists to prevent, so an edit to a packaged rule lands in its owning place and the other side's summary is checked against it in the same change. diff --git a/.github/skills/standup-a-repo/SKILL.md b/.github/skills/standup-a-repo/SKILL.md new file mode 100644 index 00000000..56c66a1e --- /dev/null +++ b/.github/skills/standup-a-repo/SKILL.md @@ -0,0 +1,99 @@ +--- +name: standup-a-repo +description: >- + Drives STANDUP.md's procedure for taking a ptr727/ProjectTemplate fleet repo from nothing (or a + partial state) to operational against the fleet ground truth, run from a hub checkout for a + named target repo the maintainer is standing up. Use this whenever asked to stand up, create, + bootstrap, or onboard a new fleet repo, or to onboard a new repo type. Needs a hub checkout and + a target repo, new or partially started, to mean anything, so it does not usefully trigger + inside an already-operational downstream repo's own session with no hub checkout present, that + case is resync-a-repo for drift or fleet-conformance-check for a self-check instead. Triggers + even when the request sounds like "just copy the template over" or "spin up a quick repo," + because skipping the ordered signing, branch, and instruction-set steps below is exactly how a + repo ends up unsigned, unrecoverable, or authored against unknown rules. +--- + +# Stand Up a Repo + +## Why this exists + +STANDUP.md's own section order exists because several of its steps close a window that cannot be +reopened cheaply: commit signing has to be correct before the first commit, the long-lived +branches have to exist before any standup commit lands on one, and the instruction set has to be +carried before anything else is authored against it. This skill exists so that order survives +contact with a real, time-pressured standup instead of depending on an agent remembering to run +each gate unprompted. It is a driver over STANDUP.md, not a replacement for it. Read STANDUP.md +itself for the full text of every step, the onboarding-a-new-repo-type procedure, and the +cold-start self-test. + +## Before starting + +Read STANDUP.md section 0A first. Nothing in this procedure creates the GitHub repository, its +App, or its secrets, each an outward-facing write that needs the maintainer's explicit permission +and inputs, so hand that checklist over before step 1 rather than discovering the gap partway +through. A repo with no remote is not partially stood up, it is not started, and only the +maintainer can supply what section 0A lists. + +## Apply, in order + +1. **Signing, before the first commit.** STANDUP.md section 0: verify, never set, the inherited + `--global` commit identity and signing configuration, and the host tool floors via + `python3 scripts/host_gate.py`. The window closes at the first commit, since a repo committed + under the wrong identity or unsigned cannot be cleanly repaired afterward. + +2. **Branches, before the first standup commit.** STANDUP.md section 0B: create `main` and + `develop` empty, off one signed empty root commit, then run every step below on a feature + branch off `develop`. Never commit standup work directly onto `develop`. `non_fast_forward` on + both branch payloads, or the missing blocking rule on an operational repo's `develop` ruleset, + makes that mistake either unrecoverable or silently unprotected. + +3. **Classify and catalog.** STANDUP.md section 1: resolve the repo's type(s) against `AUDIT.md` + section 2, then write or repair its `registry/repos.json` entry and confirm it with + `spec/validate.py`. + +4. **The instruction set, before authoring anything.** STANDUP.md section 1A: carry `AGENTS.md`, + `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md` and `AUDIT.md`, adapted rather than cloned for + the ones that describe a repo, plus `.markdownlint-cli2.jsonc` and `cspell.json`. Read + `CODESTYLE.md` and the `GOVERNANCE.md` documentation-style rules before writing any repo + content of your own, the same window-closes shape as signing in step 1. + +5. **Capture the source, if one exists.** STANDUP.md section 1B, only when the repo's content + replaces a live external system: capture it and verify the capture against the source before + anything is scaffolded from it, since the source is not under version control and cannot be + re-derived once it stops serving. + +6. **The baseline files.** STANDUP.md section 2: copy every `spec/files.json` entry whose + `appliesTo` matches the repo's selector set, adapted rather than cloned, and choose + `version.json`'s version floor deliberately rather than propagating the template's. Carry + `AGENTS.md`'s skill-dependency pointer paragraph, naming `scripts/skills_install.py` and where + the fleet's Skills live, as one more verbatim unit in this same step, not a separate pass, the + identical requirement `RESYNC.md` places on a repo already stood up. + +7. **The workflows.** STANDUP.md section 3: implement the Actions `WORKFLOW.md` requires for the + repo's type, reusing `catalog/snippets/workflows/` as the reference implementation rather than + inventing a shape. + +8. **Settings, rulesets, and secrets.** STANDUP.md section 4: confirm the remote and the GitHub + repository agree before running anything else here, then apply with + `repo-config/configure.sh apply owner/repo release|operational` and check with the same + command's `check` subcommand, never from a hand-built or carried copy. + +9. **Verify with the audit.** STANDUP.md section 5: run `AUDIT.md` end to end. The repo is stood + up only when it passes for its type, or its residual deltas are tracked in + `reports/<repo>/audit.md` plus an issue. + +## Onboarding a new repo type + +When a repo matches no existing type in `spec/project-types.json`, that is a type to onboard, not +a repo to force into the nearest existing one. STANDUP.md's "Onboarding a New Repo Type" section +covers the manifest additions (`spec/project-types.json`, `spec/files.json`, `spec/secrets.json`, +`spec/scope-model.md`, `spec/type-model.md`, and the `registry/repos.schema.json` target enum for +a new publish destination) and the cold-start self-test that proves the result usable by a +context-free agent, not just by the one that wrote it. + +## Ship it + +One pull request per standup, branched from `develop` per step 2 above, into `develop`, never a +direct push to a protected branch. Close the review loop, per the `pr-review-conduct` skill, +before asking the maintainer for merge permission. The maintainer merges, the agent drives to +green and stops. diff --git a/.github/skills/upstream-contribution-workflow/SKILL.md b/.github/skills/upstream-contribution-workflow/SKILL.md new file mode 100644 index 00000000..3f88062a --- /dev/null +++ b/.github/skills/upstream-contribution-workflow/SKILL.md @@ -0,0 +1,80 @@ +--- +name: upstream-contribution-workflow +description: >- + Governs how the maintainer contributes to a third-party repository he does not control (for + example esphome/esphome), distinct from the fleet's own internal branching model: a dirty work + branch on his own fork for the actual work and review iteration, squashed once clean to a second + branch that carries only the intended minimal history, that clean branch opened as the PR + against the upstream repo, and reviewer feedback applied to the dirty branch first, then + re-squashed into the clean one. Use this whenever about to open a pull request against a + repository outside the ptr727 fleet, whenever forking a third-party project to contribute a fix + or feature, whenever an upstream reviewer requests changes on a PR opened this way, and whenever + deciding which issue or PR template to use for a third-party repository. Triggers regardless of + the target repo's own type or workflow model, since this skill is about the shape of a + contribution to someone else's repo, not the target repo's own internal conventions, which this + skill does not attempt to state and are never assumed to match the fleet's. +--- + +# Upstream Contribution Workflow + +## Why this exists + +The fleet's own branching model (`operational-vs-release-workflow`) governs repos the maintainer +controls end to end: squash-only feature branches, merge-commit promotions, signed commits under +his own identity. None of that applies to someone else's repository. A PR into a third-party +project answers to that project's own maintainers, on their own timeline, with their own review +cycles, and the history that lands there should read as a deliberate, minimal contribution, not as +the maintainer's own iteration log. This skill is that different shape, kept separate from the +fleet's internal model so the two are never conflated. + +## The two-branch shape + +1. **Fork the upstream repo**, if not already forked. +2. **Do the actual work on a dirty work branch**, on the maintainer's own fork. This branch is + allowed to be messy: false starts, fixup commits, back-and-forth in response to review, whatever + the real work looks like while it's happening. Open a PR from this branch into a branch on the + maintainer's **own fork** (not upstream), so all the iteration happens there, visible and + reviewable, without touching the upstream repo at all. +3. **Once the dirty branch is clean and the change is ready, squash it to a second branch** that + carries only the intended, minimal commit history, one commit (or a small, deliberate set) that + states what the change is, not how it was arrived at. +4. **Open the PR against the upstream repo from that second, clean branch.** This is the only + branch upstream ever sees. +5. **If upstream reviewers ask for changes, apply them to the dirty branch first**, iterate there + the same way as step 2, then re-squash the updated dirty branch into the clean branch that + actually reaches upstream. Updating the same upstream PR rather than opening a new one each + round rewrites the clean branch's history, and pushing a rewritten branch that is already + published requires `git push --force-with-lease` (prefer it over a bare `--force`, it refuses + the push if the remote moved since the last fetch). **`git-commit-conventions`'s never-force-push + rule governs this fleet's own repos, where a branch is shared with bots, other branches, and + required-check history a rewrite would orphan, and it stays absolute there, with no exception. + It has no jurisdiction here**: this clean presentation branch lives on the maintainer's own + fork, outside the fleet entirely, and carries nobody's work but this squash. Force-with-lease + is scoped just as tightly regardless: only this one branch, only on the maintainer's own fork, + never the dirty work branch, which is the append-only iteration log this whole workflow exists + to preserve. If force-with-lease is ever refused or unavailable, open a fresh PR from a newly + named clean branch rather than fighting the push. + +**The dirty branch is always the working copy. The clean branch is always the presentation copy.** +Never reverse this: never iterate directly on the branch that's open against upstream, and never +skip the squash step because the dirty branch "looks clean enough." + +## Use the upstream repo's own conventions, not the fleet's + +Always use the upstream repo's own issue and PR templates, its own contribution guidelines, and +its own commit-message and code-style conventions when they differ from this fleet's. The fleet's +`comment-and-doc-style`, `git-commit-conventions`, and `pr-review-conduct` skills describe how +*this fleet* does things, and none of them are the target repository's own rules. Read the target +repo's `CONTRIBUTING.md` (or equivalent) and follow it. Where the target repo states no +convention of its own, matching the surrounding code's existing style in that file is the better +default, not falling back to the fleet's own convention by habit. + +## What stays governed by the fleet's own rules + +Signing commits and using the correct git identity are host configuration, not project +convention, so `git-commit-conventions`'s signing and identity rules still apply on both the dirty +and clean branches. They are properties of the committer, not of the target repository. The +write-safety rules (never write to a repository outside explicit authorization, never fabricate a +GitHub id) also still apply in full. A fork the maintainer owns is within scope to push to, and +the upstream repository itself is written to only through the PR the maintainer explicitly asked +for. diff --git a/.github/skills/workflow-ci-contract/SKILL.md b/.github/skills/workflow-ci-contract/SKILL.md new file mode 100644 index 00000000..b36ec4a1 --- /dev/null +++ b/.github/skills/workflow-ci-contract/SKILL.md @@ -0,0 +1,47 @@ +--- +name: workflow-ci-contract +description: >- + Governs the WORKFLOW.md CI/CD behavioral contract for every ptr727/ProjectTemplate fleet repo: the D1-D9 guarantees stated as the failure mode each prevents, the seam contract for release assets, the artifact lifecycle, NBGV versioning and classification, validate-at-entry, and the 5A/5B/5C test methodology with its per-type walkthroughs. Use this whenever writing or editing anything under .github/workflows/, adding or dropping a release target, auditing a repo's workflows, or reasoning about why a publish did or did not fire. This is the YAML half of the pipeline, and the operational-vs-release-workflow skill keeps the git half (branching, promotion, publish policy), so branch choice questions go there. Triggers even when the edit looks mechanical, such as bumping an action, renaming a job, or adding one upload step, because SHA pinning, the ruleset-bound aggregator name, smoke gating on uploads, and retention-days are each easy to break in a one-line diff that no smoke build exercises, since workflow-only changes are deliberately not smoke-built. WORKFLOW.md keeps authority, and GOVERNANCE.md wins where the two overlap. +--- + +# Workflow CI Contract + +## Why This Exists + +`WORKFLOW.md` in the hub is the largest law doc, a behavioral contract stating required outcomes rather than a required implementation, and it had no skill surface, so agents edited workflow YAML without the contract in view. This skill is the summary plus the binding rules, with the guarantee catalog and the test methodology split into `references/`. `WORKFLOW.md` keeps authority for the contract and methodology, and `GOVERNANCE.md` ("Workflow YAML Conventions", "Release Model") wins where the two overlap. + +## How the Contract Is Read + +- **Outcomes, not bytes.** A workflow is correct when it satisfies the section 4 contract against the expected inputs and outputs, not when it matches a catalog snippet byte for byte. Two repos may implement one guarantee with different YAML. +- **Applicability.** A guarantee governing a construct the repo does not contain is N/A: recorded, excluded from the verdict, never a defect. A source-only pipeline is mostly N/A and that is fine. +- **Operational is binary.** Every applicable guarantee holds, or the workflow is not operational. A single applicable input-output mismatch is a defect regardless of how clean the YAML looks. +- **Reached, not carried.** A standard workflow whose job graph is identical across repos of a type is a `workflow_call` task the hub hosts once, and a repo carries only a caller stub pinned to a hub release commit plus a composite-action hook at `.github/actions/<hook>` for what is its own. A hub task reaches its own actions and sibling tasks through `$/`, which resolves at that pinned commit. The merge-bot is the first, and `docs/reusable-workflows.md` in the hub carries the model, the hook contract, and the phase each workflow migrates in. Until a workflow's phase ships, its copy is graded as below. +- **Two layers.** Orchestration (the PR entry workflow, publisher, version/release/badge jobs) is generic and standard at the job level. Build leaves (`build-<target>-task.yml`) are repo-owned. Inputs like `github`/`nuget`/`dockerhub`/`expect_release_assets` live on the orchestrator, a leaf only receives `ref`/`branch`/`smoke` and a derived `push`, so assert each input in the layer that declares it. What a repo curates is the list of targets, and adding or dropping one edits the whole surface together: the `enable_<target>` input, the `build-<target>` job and its `github-release` `needs:` entry, the `changes` paths-filter entry and output, and the `smoke-build` enable-forward (D6.4). + +## Style Rules That Break in One-Line Diffs + +- **Pin every action to a commit SHA** with a trailing `# vX.Y.Z` comment, first-party included. The one documented no-pin exception is `dotnet/nbgv@master`. Invent no others. +- **Names carry meaning**: `-task.yml` files and "task" names are reusable (`on: workflow_call`), entry points end in what they do and their names end in "action", every job `name:` ends in "job" and every step in "step". A ruleset-bound required check's job `name:` and the ruleset `context:` are one string renamed together, in the live ruleset and `repo-config/{develop,main}.json` in lockstep, or required-check enforcement silently breaks. +- **Concurrency**: top-level workflows use `group: '${{ github.workflow }}-${{ github.ref }}'` with `cancel-in-progress: true`. The publisher is the documented exception: a global ref-independent group with `cancel-in-progress: false`, so publishes serialize and never cancel mid-push. +- **Shells**: every multi-line bash `run:` starts `set -Eeuo pipefail`. Multi-line `if:` uses `>-`, never `|`. +- **Boolean inputs** are declared in both trigger blocks and compared against both forms, `${{ inputs.foo == true || inputs.foo == 'true' }}`, since `workflow_dispatch` delivers strings. +- **Permissions validate before `if:`**, so even a skipped job needs valid `permissions:`, and a callee's extra scope (`actions: write`, `id-token: write`) is granted by the caller at the one entry point that needs it. +- **Chaining across optional jobs** allowlists `success`/`skipped` explicitly, because `!= 'failure'` lets `cancelled` through. +- **Docker layer cache** targets a registry tag (`buildcache-<branch>`), never `type=gha`. +- **Workflow YAML is LF.** Preserve endings on every edit. + +## The Core Behavioral Spine + +- **PRs validate fast and never publish**: a paths-filter smoke-builds only changed targets, a type-appropriate validation job always runs, and one required aggregator gates the merge, treating skipped smoke as pass and blocking on failure or cancelled. Smoke does a full compile/lint/test but pushes nothing and uploads nothing, every `upload-artifact` gated `!smoke`. +- **A human merge never auto-publishes**: a `plan` job decides once and every job gates on it. Publishes come from a code-affecting bot push to `main`, a manual dispatch of `main` or `develop`, or the main-only weekly Docker schedule. Each run builds the one trigger branch, `main` a clean `X.Y.Z`, anything else a prerelease `X.Y.Z-g<sha>`, with NBGV owning the patch from git height. The release tags the built commit's SHA (`GitCommitId`), never a branch name. +- **Validate at entry**: cross-input and input-versus-derived-state invariants are asserted once in a dedicated entry job the downstream jobs `needs:`, failing fast with `::error::` before expensive work. The release gate checks branch-versus-prerelease in both directions, strips `+buildmetadata`, and on smoke skips the check while the job still succeeds. +- **The seam contract**: a target contributes a release file by uploading `release-asset-<branch>-<target>`, and the release job collects by `pattern:` plus `merge-multiple:`, never `artifact-ids:`, canonical even for a single target. A repo with no file target passes `expect_release_assets: false` at the caller. +- **Artifacts are an intra-run handoff**: consume-then-delete at the point of consumption, gated to the consumer's condition, best-effort, `retention-days: 1` on every upload as the backstop, and never a blanket delete of the run's artifact set, which destroys the diagnostics you need when the run fails. +- **No-op republish**: an unchanged version re-pushes nothing, the release-create step skips when the tag exists, registries dedupe server-side (`--skip-duplicate`, `skip-existing: true`), and Docker alone always re-pushes by design. +- **A build failure blocks every publish target**: `github-release` needs every build, and the terminal registry pusher guards with `!failure() && !cancelled()`, so nothing partial ships. + +The full catalog, each guarantee with the failure mode it prevents, is in `references/d-guarantees.md`. Auditing, tracing, and probing a repo's workflows is `references/test-methodology.md`. + +## After Any Workflow Edit + +Workflow-only changes are not smoke-built, so run actionlint locally (the Docker invocation in `GOVERNANCE.md` "Running the Linters Locally", which bundles shellcheck for `run:` blocks) before pushing, and remember a workflow change is only fully exercised by CI, since `secrets: inherit`, `permissions:`, and `needs:` wiring resolve only in a real run. diff --git a/.github/skills/workflow-ci-contract/references/d-guarantees.md b/.github/skills/workflow-ci-contract/references/d-guarantees.md new file mode 100644 index 00000000..b41c99a6 --- /dev/null +++ b/.github/skills/workflow-ci-contract/references/d-guarantees.md @@ -0,0 +1,70 @@ +# The D-Guarantees, Condensed + +Each guarantee is a MUST from `WORKFLOW.md` section 4, stated as input to output plus the failure mode it prevents. This is the condensed catalog for working from, and `WORKFLOW.md` keeps authority, so read the section there when a guarantee's exact wording decides a verdict. + +## D1: PR Fast-Feedback (Smoke) + +- **D1.1** Only changed targets build: each target has a paths-filter entry, unchanged targets skip. Prevents a changed target slipping through unbuilt. +- **D1.2** A validation job always runs on any PR, and a non-.NET repo replaces it (never deletes it), re-pointing every `needs:` on it, the aggregator and `smoke-build` both. Prevents a PR merging with no validation, or a dangling `needs:` failing the workflow to load. +- **D1.3** Smoke never publishes and never uploads: full compile/lint/test, no pushes, every `upload-artifact` gated `!smoke`. Prevents a PR publishing and orphaned artifacts. +- **D1.4** Workflow-file changes are not smoke-built (the filter excludes `.github/workflows/**`), actionlint still validates them. +- **D1.5** One required aggregator gates merge: `needs:` the changes and validation jobs, passes on skipped smoke, blocks on failure or cancelled, and its name is ruleset-bound (job `name:` equals ruleset `context:`, renamed together). +- **D1.6** Coverage reports to Codecov for C# and Python repos with tests, best-effort so an outage never reds the gate, with a `codecov.yml` setting statuses informational and `.gitignore` excluding coverage output. + +## D2: Validation at Entry + +- **D2.1** A dedicated entry job asserts each cross-input invariant before expensive work, downstream jobs `needs:` it. +- **D2.2** The release gate fails loud when the default branch carries a prerelease suffix or a non-default branch carries none, strips `+buildmetadata` first, and on smoke skips the check while the job still succeeds (a job-level `if:` would skip dependents with it). +- **D2.3** A dispatch publish from any ref other than `main` or `develop` fails fast. +- **D2.4** Mutually-exclusive or must-pair inputs are validated, a half-filled combination fails fast. + +## D3: Versioning and Classification + +- **D3.1** One branch per run: `github.ref` names the built branch, NBGV classifies it directly, no `IGNORE_GITHUB_REF`. +- **D3.2** Default branch yields `X.Y.Z`, every other branch `X.Y.Z-g<sha>`, and the default-branch literal in the gate, the `prerelease` expression, and `version.json`'s `publicReleaseRefSpec` all name the repo's real default branch. +- **D3.3** `version.json` sets the major.minor floor, NBGV appends git height as the patch, and both are retained even by a no-compiler repo, since they own the tag. +- **D3.4** Registry versions follow the classification per registry: NuGet.org derives prerelease from the SemVer2 suffix, PyPI builds from `AssemblyFileVersion` with `.dev0` appended on `develop` only, and the develop build stays `--pre`-selectable above the released version. +- **D3.5** A wrapper repo drives its image version from a committed `name -> version` state file, and the leaf must actually read it, since a leaf still tagging off NBGV means the wrapper is not pinned to upstream. + +## D4: Release and Publish + +- **D4.1** Gated single-branch publish: a human merge never auto-publishes, the `plan` job decides once, publishes come from a code-affecting bot push to `main`, a dispatch of `main`/`develop`, or the main-only weekly Docker schedule. +- **D4.2** `target_commitish` is the built commit's SHA (NBGV `GitCommitId`), never a branch name and never `github.sha`. +- **D4.3** Every release is a tag plus source zip, README, and LICENSE, file targets attach `release-asset-*`, and a no-file-target caller passes `expect_release_assets: false` or the release-create step fails on unmatched files. +- **D4.4** No-op republish: an unchanged version re-pushes nothing, the release-create skips when the tag exists (refreshed only on `workflow_dispatch`), registries dedupe server-side, and Docker always re-pushes by design. +- **D4.5** A failed build blocks every publish target: `github-release` needs every build, the terminal registry pusher guards `!failure() && !cancelled()`, so nothing partial ships. +- **D4.6** A deploy check asserts which release and which environment answer, waiting for convergence to a bounded timeout, with an unreachable host reported distinctly from an HTTP status. + +## D5: Resource Cleanup + +- **D5.1** A cross-job transfer artifact is deleted at its point of consumption. An in-run intermediate may rely on the retention backstop. +- **D5.2** The delete runs under the same condition as its consumer, so a no-op re-run skips the release-asset delete while the PyPI build-artifact delete still runs. +- **D5.3** Cleanup is best-effort (`continue-on-error`, tolerate a failed listing, delete all matching ids). +- **D5.4** Every `upload-artifact` sets `retention-days: 1`. +- **D5.5** Never blanket-delete the run's artifacts, which destroys diagnostics and auto-emitted build records. +- **D5.6** A durable deploy destination's retention is bounded by a declared count with one side recorded as owning the prune: the deploy where its credential can observe the destination, the host where the credential is deliberately write-only. + +## D6: Seam Conformance + +- **D6.1** The release job downloads by `pattern:`/`merge-multiple:`, never `artifact-ids:`, canonical for single-target repos too. +- **D6.2** Branch-derived config reads `inputs.branch`, never `github.ref_name`. +- **D6.3** Artifact names are branch-suffixed. +- **D6.4** A target add or drop updates the whole surface together: `enable_<target>` input, `build-<target>` job, `github-release` `needs:` entry, paths-filter entry and output, and the `smoke-build` enable-forward. + +## D7: Concurrency, Permissions, Safety + +- **D7.1** The publisher serializes: global ref-independent concurrency group, `cancel-in-progress: false`. +- **D7.2** Every reusable job declares valid `permissions:` (validated before `if:`), a callee's extra scope granted by the caller. +- **D7.3** Boolean inputs are declared in both trigger blocks and compared against both forms. +- **D7.4** Optional-dependency chaining allowlists `success`/`skipped` explicitly. + +## D8: Bots and Automation + +- **D8.1** The merge-bot enables auto-merge on `opened`/`reopened` for every Dependabot tier, dispatches squash or merge by base ref, disables on a maintainer-pushed `synchronize`, and keys concurrency on the PR number, not `github.ref`. +- **D8.2** Codegen runs a deterministic matrix over both branches, Dependabot targets both branches. +- **D8.3** The upstream tracker writes a committed `name -> version` state file via a rolling per-branch bump PR the merge-bot auto-merges, and its branch prefix must match the merge-bot's head-ref pairs or auto-merge silently never fires. +- **D8.4** An identity allowlist used as a gate emits a `::warning::` on the non-matching branch rather than falling through silently, since a renamed App slug otherwise turns the gate off invisibly. + +## D9: Style and Static + +SHA pins with version comments, the name-suffix rules, `set -Eeuo pipefail`, `if: >-`, registry-tag Docker cache with `cache-to` only the built branch on push and `cache-from` both branches, line endings per `.editorconfig`. diff --git a/.github/skills/workflow-ci-contract/references/test-methodology.md b/.github/skills/workflow-ci-contract/references/test-methodology.md new file mode 100644 index 00000000..ecd09487 --- /dev/null +++ b/.github/skills/workflow-ci-contract/references/test-methodology.md @@ -0,0 +1,27 @@ +# Testing a Repo's Workflows + +The three escalating verification modes from `WORKFLOW.md` section 5, which keeps authority. N/A items (a check or scenario for an absent construct) are recorded and excluded, never failed. + +## 5A: Static Audit + +Read the workflow files plus `version.json` and assert the structural fact behind each applicable D-guarantee, each pass, fail, or N/A with a `file:line` citation, asserting each input in the layer that declares it. The core sweep covers: the paths-filter's target coverage and `.github/workflows/**` exclusion, smoke gating on every upload, the aggregator's `needs:` and skip/fail handling, the entry validation jobs and the two-directional release gate, the single-branch NBGV classification and the three default-branch literals agreeing, `target_commitish` from `GitCommitId`, the consume-then-delete artifact lifecycle with `retention-days: 1` everywhere and no blanket delete, the `pattern:` handoff and `inputs.branch` config, the publisher's serialized concurrency, and the SHA pins. `WORKFLOW.md` 5A lists the per-type addenda (console runtime matrix, NuGet `--skip-duplicate`, the PyPI OIDC environment split, Docker `expect_release_assets` and cache shape, the static-site deploy gates), so apply only the ones the repo's types imply. + +## 5B: Trace Scenarios + +For each applicable scenario, evaluate every job's `if:`/`needs:` against the inputs and compare the predicted run/skip, version, release, and artifact end state to the expected table in `WORKFLOW.md` 5B. The load-bearing ones: + +- **S1** a PR touching a target: that target smoke-builds, nothing uploads, the aggregator succeeds. +- **S5/S6** a bot push to `main`: publishes only when code-affecting, and a human push never does. +- **S7** a publish run builds the one trigger branch with the right classification and leaves no dangling artifacts. +- **S8** a dispatch from a ref other than `main`/`develop` fails fast. +- **S9** a no-op re-run: release-create skipped, registries dedupe, PyPI build artifact still deleted, Docker still re-pushes. +- **S10** branch and version classification disagree: the gate fails loud and everything downstream skips. +- **S12/S13** a deploy dispatch: ref gate first, environment re-asserted, pointer flip separate, live check names the release, and a production deploy from a non-default ref fails before anything is written. + +## 5C: Live Probe + +Only for what a static trace cannot settle: a trivial PR to confirm S1, a smoke push-probe of both branches' version classification, registry queries after a real publish, and the artifact lifecycle read from a real run's logs. The deploy ref gate is verified only by tripping it, and that dispatch is the maintainer's to run: the agent prepares the command and reads back the four evidence items (gate conclusion, its error text, every downstream job skipped, deployment count unchanged), and a harness refusal to fire it is the control working, never something to re-shape. + +## Verdict + +Operational iff every applicable 5A item passes and every applicable 5B scenario matches, with the failing guarantees and their triggering inputs named, and the N/A list recorded. Per-project-type walkthroughs mapping scenarios onto targets, including source-only, static-site, and operational shapes, are `WORKFLOW.md` section 6. diff --git a/AGENTS.md b/AGENTS.md index 9f185b94..a77ca505 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,7 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | Any prose, comment, doc, or line-ending change | `Documentation Style Conventions`, packaged as the `comment-and-doc-style` Skill | | Proving work actually happened | `Verification Discipline`, surfaced at its decision moment by the `agent-conduct` Skill, and the section keeps the full rules | | Requesting, answering, or closing a review | `PR Review Etiquette`, packaged as the `pr-review-conduct` Skill | +| Reviewing a pull request, patch, or change set | `code-review`, which routes to the applicable general, language, documentation, and workflow skills | | Reporting progress or asking the user something | `Communicating with the User`, surfaced at its decision moment by the `agent-conduct` Skill, and the section keeps the full rules | | Editing a workflow YAML file | `Workflow YAML Conventions`, surfaced with the full `WORKFLOW.md` contract by the `workflow-ci-contract` Skill, and this section and `WORKFLOW.md` keep the full rules | | Choosing an OS, runtime, or toolchain target | `Supported Development Platforms` | diff --git a/docs/fleet-map.md b/docs/fleet-map.md index 2408b653..a60fc81a 100644 --- a/docs/fleet-map.md +++ b/docs/fleet-map.md @@ -48,6 +48,7 @@ flowchart TD scripts["scripts/ hub-hosted tooling"] --> audit skills[".agents/skills/ source"] --> build[build_dist.py] build --> plugin[".claude-plugin/ generated"] + build --> githubskills[".github/skills/ generated"] plugin --> install[skills_install.py] skills --> install install --> host["host: ~/.agents/skills + plugin + stamp"] @@ -55,7 +56,7 @@ flowchart TD ci["CI gates + prose-gate action"] --> law ``` -The layers, one line each. The lifecycle docs route and procedure ([`AGENTS.md`][agents] routes, [`STANDUP.md`][standup] creates, [`RESYNC.md`][resync] re-lines, [`AUDIT.md`][audit] measures). The law docs hold the rules ([`GOVERNANCE.md`][governance] cross-cutting, [`CODESTYLE.md`][codestyle] per language, [`WORKFLOW.md`][workflow] the CI/CD contract). The machine ground truth is [`spec/`][files] plus [`registry/repos.json`][repos]. The hub-hosted tooling is [`scripts/`][scripts-readme], reached rather than carried. The skills source of truth is [`.agents/skills/`][skills-readme], and [`scripts/build_dist.py`][build-dist] generates the Claude Code plugin under [`.claude-plugin/`][marketplace] from it. [`scripts/skills_install.py`][skills-install] installs both forms per machine. [`host-setup/`][host-setup-doc] provisions a host from a stock OS. CI enforces the deterministic subset of the rules on every pull request. +The layers, one line each. The lifecycle docs route and procedure ([`AGENTS.md`][agents] routes, [`STANDUP.md`][standup] creates, [`RESYNC.md`][resync] re-lines, [`AUDIT.md`][audit] measures). The law docs hold the rules ([`GOVERNANCE.md`][governance] cross-cutting, [`CODESTYLE.md`][codestyle] per language, [`WORKFLOW.md`][workflow] the CI/CD contract). The machine ground truth is [`spec/`][files] plus [`registry/repos.json`][repos]. The hub-hosted tooling is [`scripts/`][scripts-readme], reached rather than carried. The skills source of truth is [`.agents/skills/`][skills-readme], and [`scripts/build_dist.py`][build-dist] generates the GitHub Copilot tree and Claude Code plugin from it. [`scripts/skills_install.py`][skills-install] installs the host-scoped forms. [`host-setup/`][host-setup-doc] provisions a host from a stock OS. CI enforces the deterministic subset of the rules on every pull request. ## Entry Points @@ -136,7 +137,7 @@ Owned by [`AUDIT.md`][audit] section 10, [`GOVERNANCE.md` "Hub-Hosted Tooling"][ **Resolved: the install is global per user, and the work is closing its gaps, not adding a second model.** A per-repo pinned install was considered and rejected: it would let a repo's skills match its own state, but it forfeits coverage of ad-hoc sessions in no repo at all (which is where the incidents this fleet guards against actually happened), doubles the staleness surface, and adds a version-resolution mechanism the fleet does not need while the whole fleet tracks one hub. -The lifecycle chain as built: a skill is hand-authored under [`.agents/skills/`][skills-readme], [`scripts/build_dist.py`][build-dist] generates the Claude Code plugin under [`.claude-plugin/`][marketplace], and [`scripts/skills_install.py`][skills-install] installs both forms per machine (an overlay copy into `~/.agents/skills/` for Codex and opencode, a user-scope plugin install for Claude Code), stamping the hub commit into `~/.agents/skills-install-stamp.json`. `skills_install.py --report` is the read-only staleness check and exits non-zero when the machine is behind the checkout. +The lifecycle chain as built: a skill is hand-authored under [`.agents/skills/`][skills-readme], [`scripts/build_dist.py`][build-dist] generates `.github/skills/` and the Claude Code plugin, and [`scripts/skills_install.py`][skills-install] installs both host-scoped forms per machine. The installer stamps the hub commit into `~/.agents/skills-install-stamp.json`. `skills_install.py --report` is the read-only staleness check and exits non-zero when the machine is behind the checkout. Four wiring points close the model, and each is in place: @@ -216,8 +217,8 @@ flowchart LR ### G8: Generated Plugin Can Ship Stale (Closed) -- **Gap** - `.claude-plugin/` is generated from `.agents/skills/`, and a merge that edits the source without re-running [`scripts/build_dist.py`][build-dist] would ship a plugin that no longer matches it. -- **Resolution** - [`.github/workflows/validate-task.yml`][validate-task] runs `build_dist.py --check` as its own step in the lint job, on every pull request, and the required aggregator check gates on that job. A PR desyncing the two trees therefore fails the required check, which is this row's closing test. +- **Gap** - `.github/skills/` and `.claude-plugin/` are generated from `.agents/skills/`, and a merge that edits the source without re-running [`scripts/build_dist.py`][build-dist] would ship stale distributions. +- **Resolution** - [`.github/workflows/validate-task.yml`][validate-task] runs `build_dist.py --check` as its own step in the lint job, on every pull request, and the required aggregator check gates on that job. A PR desyncing a generated tree therefore fails the required check, which is this row's closing test. - **Provenance** - The step landed in [#676][pr-676], which predates this register's merge, so this row's original `Checked` claim was stale on arrival. Recording that here rather than silently deleting the row is the maintenance rule doing its job. ### G9: WORKFLOW.md and AUDIT.md Have No Skill (Closed) @@ -355,7 +356,6 @@ Design-doc first: this doc merges, then each unchecked item becomes an issue lin [host-setup-doc]: ./host-setup.md [host-setup-readme]: ../host-setup/README.md [host-tools]: ../spec/host-tools.json -[marketplace]: ../.claude-plugin/marketplace.json [operations]: ../OPERATIONS.md [peer-messaging]: ./peer-messaging.md [pr-review]: ../scripts/pr_review.py diff --git a/scripts/README.md b/scripts/README.md index 51929788..6dd4363f 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -196,9 +196,9 @@ The match is on the block's heading rather than anywhere in the body, and on the ## `build_dist.py` -Regenerates [`.claude-plugin/fleet-skills/`][fleet-skills-dist] from [`.agents/skills/`][agents-skills], the hub's own hand-authored fleet Skills. Codex and opencode read `.agents/skills/` directly, project-local, with no install step. Claude Code scans neither that path nor any repo-root path by default, only `.claude/skills/` or a plugin's own `skills/`, so this script materializes a Claude-plugin-compatible copy for [`.claude-plugin/marketplace.json`][marketplace] to publish, nested under `.claude-plugin/` rather than a top-level `dist/`, since this repo's `.gitignore` already gives `dist/` a different, Python-build-artifact meaning. `.agents/skills/` stays the one place a skill is hand-edited, and `.claude-plugin/fleet-skills/` is generated and never hand-edited, the same discipline this fleet already applies to other derived trees. +Regenerates [`.github/skills/`][github-skills-dist] and [`.claude-plugin/fleet-skills/`][fleet-skills-dist] from [`.agents/skills/`][agents-skills], the hub's own hand-authored fleet Skills. Codex and opencode read `.agents/skills/` directly, GitHub Copilot reads `.github/skills/`, and Claude Code reads the generated plugin published through [`.claude-plugin/marketplace.json`][marketplace]. `.agents/skills/` stays the one place a skill is hand-edited. Both generated trees are never hand-edited. -`--check` is the read-only mode: it exits `1` when the generated plugin was built from different source bytes than `.agents/skills/` currently holds, comparing a digest over every source file rather than a file-count or a timestamp, so a same-size edit still registers. CI runs `--check` rather than trusting a contributor to have run the generator, the same reason `spec/audit.py` exists rather than trusting a hand-carried file. +`--check` is the read-only mode: it exits `1` when either generated tree differs from `.agents/skills/`, comparing a digest over every file rather than a file count or timestamp. CI runs `--check` rather than trusting a contributor to have run the generator, the same reason `spec/audit.py` exists rather than trusting a hand-carried file. ## `skills_install.py` @@ -215,6 +215,7 @@ Installs the fleet's Skills for the current machine, cross-platform and idempote [editorconfig]: ../.editorconfig [files]: ../spec/files.json [fleet-skills-dist]: ../.claude-plugin/fleet-skills/ +[github-skills-dist]: ../.github/skills/ [gitattributes]: ../.gitattributes [governance]: ../GOVERNANCE.md [host-setup]: ../docs/host-setup.md diff --git a/scripts/build_dist.py b/scripts/build_dist.py index 7fe2e239..74cf18ba 100755 --- a/scripts/build_dist.py +++ b/scripts/build_dist.py @@ -1,15 +1,15 @@ #!/usr/bin/env python3 -"""Generate the Claude-plugin-compatible copy of .agents/skills/ at .claude-plugin/fleet-skills/. +"""Generate the GitHub and Claude-compatible copies of .agents/skills/. .agents/skills/ is the one hand-authored source: Codex and opencode read it directly with no install step. Claude Code never scans that path, only .claude/skills/ or a plugin's own skills/ directory, so this script materializes a plugin (.claude-plugin/fleet-skills/) that -.claude-plugin/marketplace.json publishes, keeping .agents/skills/ the single place a skill's -content is ever hand-edited. Nested under .claude-plugin/ rather than a top-level dist/, since -this repo's .gitignore already gives dist/ a different, Python-build-artifact meaning. +.claude-plugin/marketplace.json publishes. GitHub Copilot discovers repository skills under +.github/skills/, so the script also materializes that tree. .agents/skills/ stays the single +place a skill's content is ever hand-edited. -Usage: python3 scripts/build_dist.py regenerate the plugin from .agents/skills/ - python3 scripts/build_dist.py --check read-only: exit 1 if the plugin is stale +Usage: python3 scripts/build_dist.py regenerate distributions from .agents/skills/ + python3 scripts/build_dist.py --check read-only: exit 1 if a distribution is stale """ from __future__ import annotations @@ -27,6 +27,7 @@ DIST_PLUGIN = ROOT / ".claude-plugin" / PLUGIN_NAME PLUGIN_MANIFEST = DIST_PLUGIN / ".claude-plugin" / "plugin.json" DIGEST_STAMP = DIST_PLUGIN / ".source-digest" +GITHUB_SKILLS = ROOT / ".github" / "skills" def skill_names(): @@ -87,6 +88,16 @@ def source_digest(names): return tree_digest(SKILLS_SRC, names) +def has_exact_skill_directories(root, names): + """Whether `root` exists and contains only the expected skill directories.""" + if not root.is_dir() or root.is_symlink(): + return False + entries = list(root.iterdir()) + return all(entry.is_dir() for entry in entries) and {entry.name for entry in entries} == set( + names + ) + + def expected_manifest(names): """The plugin.json content `names` should produce, entirely deterministic. @@ -124,6 +135,13 @@ def regenerate(): for name in names: reject_symlinks(SKILLS_SRC / name) shutil.copytree(SKILLS_SRC / name, dist_skills / name) + if GITHUB_SKILLS.is_symlink() or GITHUB_SKILLS.is_file(): + GITHUB_SKILLS.unlink() + elif GITHUB_SKILLS.is_dir(): + shutil.rmtree(GITHUB_SKILLS) + GITHUB_SKILLS.mkdir(parents=True, exist_ok=True) + for name in names: + shutil.copytree(SKILLS_SRC / name, GITHUB_SKILLS / name) write_plugin_manifest(names) # LF, same as write_plugin_manifest, explicit for the same Windows-platform-default reason. DIGEST_STAMP.write_text(source_digest(names) + "\n", encoding="utf-8", newline="\n") @@ -131,7 +149,7 @@ def regenerate(): def is_stale(): - """Whether the generated plugin needs regenerating: missing, corrupted, or built from + """Whether a generated distribution needs regenerating: missing, corrupted, or built from different source bytes. Checks the manifest's own content and the generated tree's actual bytes, not only the digest stamp, since a stamp surviving a partial deletion, a hand-edited manifest, or an edited-in-place generated file would otherwise report current over a plugin @@ -155,21 +173,24 @@ def is_stale(): # An extra directory under DIST_PLUGIN/skills/ (a retired skill left behind, one added by hand) would never be read and could not affect that comparison. # Checked by name first, deliberately not folded into the digest walk itself. dist_skills = DIST_PLUGIN / "skills" - actual_names = ( - {p.name for p in dist_skills.iterdir() if p.is_dir()} if dist_skills.is_dir() else set() - ) - if actual_names != set(names): + if not has_exact_skill_directories(dist_skills, names): return True current_source_digest = source_digest(names) if DIGEST_STAMP.read_text(encoding="utf-8").strip() != current_source_digest: return True - return current_source_digest != tree_digest(dist_skills, names) + if current_source_digest != tree_digest(dist_skills, names): + return True + if not has_exact_skill_directories(GITHUB_SKILLS, names): + return True + return current_source_digest != tree_digest(GITHUB_SKILLS, names) def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( - "--check", action="store_true", help="read-only: exit 1 if the generated plugin is stale" + "--check", + action="store_true", + help="read-only: exit 1 if a generated skill distribution is stale", ) args = parser.parse_args() @@ -180,9 +201,12 @@ def main(): print(exc, file=sys.stderr) return 1 if stale: - print(f"{DIST_PLUGIN} is stale: run `python3 scripts/build_dist.py`.", file=sys.stderr) + print( + "Generated skill distributions are stale: run `python3 scripts/build_dist.py`.", + file=sys.stderr, + ) return 1 - print(f"{DIST_PLUGIN} is current.") + print("Generated skill distributions are current.") return 0 try: diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 482923bd..9028b28d 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -14,8 +14,9 @@ review findings between them. Read-only. Exit 0 = every reference resolves, 70 = one does not, 71 = there were references and none could be read, so nothing was decided. status One digest line, any unresolved threads, and any suppressed findings. Read-only. - Exit 0 = every shape in the reviewer's output is one this reads, and the round - covering the head read the whole diff or stated nothing about what it read. + Exit 0 = no review covers the head yet, or every output shape is recognized and the + round covering the head read the whole diff. Use `wait` when review presence is the + condition, since `status` reports an absent review without treating it as a failure. 42 = that round read fewer files than the pull request changed, so part of the diff has no review at all. Measured over four pull requests and seven rounds here, a re-request never cleared one and no round ever recovered, so this is a state to @@ -24,6 +25,8 @@ be believed. The remedy is an issue on the repository hosting this script, and the review loop does not close until the reader is fixed. Merging regardless is the maintainer's decision rather than the agent's. + 45 = the review covering the head stated no changed-file coverage. Request another + review only after confirming the head branch carries the current review instructions. reply Answer one thread selected by its text, and resolve it on request. Exists because the hand-run form keeps failing the same way: a node id typed into a mutation, which resolves globally and so writes to a real thread somewhere @@ -46,7 +49,7 @@ 40 reports the shape of that answer and reads nothing of its cause: an answer carrying no commit covers no head, so the wait ends and the reader decides. 41 = the review carrying the head says it did not review, so it covers nothing. - 42 and 43 = the review landed and `status`'s two blocking readings apply to it, + 42, 43, and 45 = the review landed and `status`'s blocking readings apply to it, since a wait ending on a round that covered half the diff, or on output nothing here can read, has ended on something other than a review of this pull request. 44 = the review loop closed, the merge reads BLOCKED, and a check is in a shape no @@ -140,6 +143,12 @@ # That one keeps the text requirement, since the name alone does not say the line states coverage. COVERAGE_BULLET = re.compile(r"\s*[-*]\s*\*\*Files reviewed:", re.IGNORECASE) COVERAGE_SENTENCE = re.compile(r"\s*Copilot\b", re.IGNORECASE) +# Ask the reviewer for a stable coverage shape instead of adapting only to changing prose. +# Keep the prose readers for reviews made before the marker shipped. +FLEET_REVIEW = re.compile( + r"\s*<!--\s*fleet-review:\s*reviewed=(\d+)\s+changed=(\d+)\s+findings=(\d+)\s*-->\s*", + re.IGNORECASE, +) # The count pair itself, in the two spellings the corpus carries. # The comment tail one of them ends on is deliberately not part of the unit. # It says how many comments the round raised, which is not coverage. @@ -639,6 +648,8 @@ def reviewed_head(pr: dict) -> bool: def is_coverage_line(line: str) -> bool: """Whether this line is the reviewer stating its file coverage, rather than prose about it.""" + if FLEET_REVIEW.fullmatch(line): + return True if COVERAGE_BULLET.match(line): return True return bool(COVERAGE_SENTENCE.match(line)) and "changed file" in line.lower() @@ -658,7 +669,11 @@ def read_coverage(line: str) -> tuple[int, int] | None: it is a line this script is parsing wrongly, and reading it as full coverage fails open on exactly the statement that says something is off. """ + marker = FLEET_REVIEW.fullmatch(line) m = COVERAGE_COUNTS.search(line) + if marker: + reviewed, changed = marker.group(1), marker.group(2) + return (int(reviewed), int(changed)) if int(reviewed) <= int(changed) else None if not m: return None reviewed, changed = (m.group(1), m.group(2)) if m.group(1) else (m.group(3), m.group(4)) @@ -668,12 +683,9 @@ def read_coverage(line: str) -> tuple[int, int] | None: def coverage_of(node: dict) -> tuple[str, str]: """This round's coverage reading, with the line it was read from. - A round making no statement at all reads as unstated rather than a pass or a failure. 28 of - the 332 bodies measured carry an overview and a change list and nothing more, that shape - interleaves with the counted one throughout rather than preceding it, and one pull request - carries both across its two rounds. Failing on it would cry wolf on roughly one review in - twelve, and a guard an agent learns to work around is worse than no guard. Passing it as - coverage is the bug this whole reading exists to remove, one shape over. + A round making no statement at all reads as unstated rather than unvetted. 28 of the 332 + bodies measured carry an overview and a change list and nothing more. The shape is recognized, + but it cannot prove full diff coverage and therefore blocks the status gate. A wording that is coverage-shaped and parses to no counts is the failure whose remedy is fixing this script, since a gate that allows whatever it does not recognize stops gating as @@ -910,6 +922,10 @@ def report_verdict(pr: dict) -> int: f"maintainer's decision to take and not this script's, and not the agent's." ) return 43 + # A missing head review has no coverage verdict yet. + # The wait path owns that incomplete state. + if not head_reviews(pr): + return 0 state, line = head_coverage(pr) if state == PARTIAL: # The unread count comes from the line that decided PARTIAL, never from past rounds. @@ -933,6 +949,15 @@ def report_verdict(pr: dict) -> int: f"{table_against_diff(pr, counts)}" ) return 42 + if state == UNSTATED: + print( + "status=COVERAGE_IS_UNSTATED the review covering the head states no changed-file " + "coverage, so the review loop cannot prove that it read the full diff. Confirm the " + "head branch carries the current code-review skill and Copilot instructions, then " + "request another review. Merging without coverage is the maintainer's decision, " + "not the agent's." + ) + return 45 return 0 diff --git a/scripts/tests/test_build_dist.py b/scripts/tests/test_build_dist.py index 9c558a38..82959074 100755 --- a/scripts/tests/test_build_dist.py +++ b/scripts/tests/test_build_dist.py @@ -22,23 +22,27 @@ def setUp(self) -> None: self.tmp = Path(self.enterContext(__import__("tempfile").TemporaryDirectory())) self.skills_src = self.tmp / ".agents" / "skills" self.dist_plugin = self.tmp / ".claude-plugin" / "fleet-skills" + self.github_skills = self.tmp / ".github" / "skills" self.addCleanup( self._restore, build_dist.SKILLS_SRC, build_dist.DIST_PLUGIN, build_dist.PLUGIN_MANIFEST, build_dist.DIGEST_STAMP, + build_dist.GITHUB_SKILLS, ) build_dist.SKILLS_SRC = self.skills_src build_dist.DIST_PLUGIN = self.dist_plugin build_dist.PLUGIN_MANIFEST = self.dist_plugin / ".claude-plugin" / "plugin.json" build_dist.DIGEST_STAMP = self.dist_plugin / ".source-digest" + build_dist.GITHUB_SKILLS = self.github_skills - def _restore(self, src, dist, manifest, stamp) -> None: + def _restore(self, src, dist, manifest, stamp, github_skills) -> None: build_dist.SKILLS_SRC = src build_dist.DIST_PLUGIN = dist build_dist.PLUGIN_MANIFEST = manifest build_dist.DIGEST_STAMP = stamp + build_dist.GITHUB_SKILLS = github_skills def make_skill(self, name: str, body: str = "content") -> None: d = self.skills_src / name @@ -105,8 +109,45 @@ def test_regenerate_copies_skill_content_and_lists_it_in_the_manifest(self) -> N (self.dist_plugin / "skills" / "foo" / "SKILL.md").read_text(encoding="utf-8"), "content", ) + self.assertEqual( + (self.github_skills / "foo" / "SKILL.md").read_text(encoding="utf-8"), + "content", + ) self.assertFalse(build_dist.is_stale()) + def test_a_deleted_github_skill_reports_stale(self) -> None: + self.make_skill("foo") + build_dist.regenerate() + import shutil + + shutil.rmtree(self.github_skills / "foo") + self.assertTrue(build_dist.is_stale()) + + def test_a_github_skill_edited_in_place_reports_stale(self) -> None: + self.make_skill("foo") + build_dist.regenerate() + (self.github_skills / "foo" / "SKILL.md").write_text("tampered", encoding="utf-8") + self.assertTrue(build_dist.is_stale()) + + def test_an_orphaned_github_skill_reports_stale(self) -> None: + self.make_skill("foo") + build_dist.regenerate() + orphan = self.github_skills / "orphan" + orphan.mkdir() + (orphan / "SKILL.md").write_text("stray", encoding="utf-8") + self.assertTrue(build_dist.is_stale()) + + def test_a_stray_file_in_github_skills_reports_stale(self) -> None: + self.make_skill("foo") + build_dist.regenerate() + (self.github_skills / "README.md").write_text("stray", encoding="utf-8") + self.assertTrue(build_dist.is_stale()) + + def test_a_missing_empty_github_skills_tree_reports_stale(self) -> None: + build_dist.regenerate() + self.github_skills.rmdir() + self.assertTrue(build_dist.is_stale()) + def test_a_deleted_manifest_reports_stale_even_with_a_current_digest_stamp(self) -> None: self.make_skill("foo") build_dist.regenerate() @@ -164,6 +205,12 @@ def test_an_orphaned_generated_skill_directory_reports_stale(self) -> None: (self.dist_plugin / "skills" / "orphan" / "SKILL.md").write_text("stray", encoding="utf-8") self.assertTrue(build_dist.is_stale()) + def test_a_stray_file_in_generated_plugin_skills_reports_stale(self) -> None: + self.make_skill("foo") + build_dist.regenerate() + (self.dist_plugin / "skills" / "README.md").write_text("stray", encoding="utf-8") + self.assertTrue(build_dist.is_stale()) + def test_editing_a_skill_after_regenerate_reports_stale(self) -> None: self.make_skill("foo") build_dist.regenerate() diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 80e18e79..ce703f28 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -43,7 +43,10 @@ def review( - login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = OVERVIEW, at: str = EARLY + login: str = pr_review.REVIEWER, + oid: str = HEAD, + body: str = OVERVIEW + "\n<!-- fleet-review: reviewed=1 changed=1 findings=0 -->", + at: str = EARLY, ) -> dict: return { "author": {"login": login}, @@ -901,12 +904,11 @@ def test_a_coverage_line_that_parses_to_nothing_names_this_script(self) -> None: self.assertIn("UNRECOGNIZED REVIEWER OUTPUT (1)", out) self.assertIn(f"coverage line: {line}", out) - def test_a_round_stating_no_coverage_at_all_is_unstated_rather_than_a_verdict(self) -> None: + def test_a_round_stating_no_coverage_at_all_is_unstated(self) -> None: """28 of the 332 bodies are an overview and a change list, and that shape is current. - It interleaves with the counted one throughout rather than preceding it, and one pull - request carries both across its two rounds, so failing on it would cry wolf on about one - review in twelve. Reporting it as coverage is the bug this reading exists to remove. + It is a recognized shape rather than unvetted reviewer output, but it cannot prove that + the reviewer covered the full diff and therefore blocks the status gate. """ body = ( "## Pull request overview\n\nThis PR updates the backlog.\n\n" @@ -917,6 +919,18 @@ def test_a_round_stating_no_coverage_at_all_is_unstated_rather_than_a_verdict(se self.assertNotIn("COVERAGE IS PARTIAL", out) self.assertIn("shapes=ok", out) + def test_machine_readable_marker_reports_full_coverage(self) -> None: + """The instructed marker is stable input while older prose remains supported.""" + marker = "<!-- fleet-review: reviewed=3 changed=3 findings=0 -->" + self.assertEqual((3, 3), pr_review.read_coverage(marker)) + self.assertEqual(pr_review.FULL, pr_review.coverage_of({"body": marker})[0]) + + def test_machine_readable_marker_reports_partial_coverage(self) -> None: + """The marker's measured counts use the same fail-closed comparison as prose.""" + marker = "<!-- fleet-review: reviewed=2 changed=3 findings=1 -->" + self.assertEqual((2, 3), pr_review.read_coverage(marker)) + self.assertEqual(pr_review.PARTIAL, pr_review.coverage_of({"body": marker})[0]) + def test_a_refusal_is_exempt_rather_than_an_unrecognized_shape(self) -> None: """It states no coverage by design, and `head_reviews` has already dropped it. @@ -1126,7 +1140,7 @@ def test_every_round_is_read_rather_than_the_head_s(self) -> None: class TestCoverageExitCodes(GqlCase): - """`status` returned 0 unconditionally, so a partial round reported as a covered head.""" + """Coverage that is partial or unstated blocks a covered-head verdict.""" def setUp(self) -> None: self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) @@ -1142,6 +1156,18 @@ def test_status_exits_forty_two_on_a_round_that_read_part_of_the_diff(self) -> N self.assertEqual(42, pr_review.main(["status", "7", "--repo", "o/r"])) self.assertIn("status=COVERAGE_IS_PARTIAL", self.out.getvalue()) + def test_status_exits_forty_five_when_coverage_is_unstated(self) -> None: + """A current-head review is not proof that the reviewer read the full diff.""" + self.answer(payload([review(body=OVERVIEW)])) + self.assertEqual(45, pr_review.main(["status", "7", "--repo", "o/r"])) + self.assertIn("status=COVERAGE_IS_UNSTATED", self.out.getvalue()) + + def test_status_has_no_coverage_verdict_before_a_review_lands(self) -> None: + """A missing round is incomplete work, not an unstated statement by a reviewer.""" + self.answer(payload([])) + self.assertEqual(0, pr_review.main(["status", "7", "--repo", "o/r"])) + self.assertNotIn("status=COVERAGE_IS_UNSTATED", self.out.getvalue()) + def test_the_partial_message_counts_the_unread_files_rather_than_assuming_one(self) -> None: """Every partial on record skipped exactly one file, which is a measurement of seven rounds rather than a property the state carries, so the line reads the run it prints.""" @@ -1189,9 +1215,10 @@ def test_status_exits_forty_three_on_a_wording_it_does_not_read(self) -> None: self.assertIn("status=UNRECOGNIZED_REVIEWER_OUTPUT", out) self.assertIn("coverage=UNVETTED", out) - def test_status_still_exits_zero_on_a_full_round_and_on_a_silent_one(self) -> None: - """Failing the silent shape would fail roughly one review in twelve on this repository.""" - for body in (OVERVIEW + "\n" + COVERED, nested(), OVERVIEW): + def test_status_still_exits_zero_on_a_full_round(self) -> None: + """Both the legacy coverage prose and the stable marker close the coverage gate.""" + marker = OVERVIEW + "\n<!-- fleet-review: reviewed=1 changed=1 findings=0 -->" + for body in (OVERVIEW + "\n" + COVERED, nested(), marker): with self.subTest(body=body[:40]): self.answer(payload([review(body=body)])) self.assertEqual(0, pr_review.main(["status", "7", "--repo", "o/r"])) @@ -2800,47 +2827,32 @@ def test_gh_being_absent_does_not_raise(self) -> None: class TestContract(unittest.TestCase): - def test_the_reviewer_login_matches_the_runbook_graphql_form(self) -> None: - """GraphQL drops the `[bot]` suffix REST carries, and this script is GraphQL-only. + def test_status_documents_its_no_review_success_case(self) -> None: + self.assertIn("Exit 0 = no review covers the head yet", pr_review.__doc__ or "") - The runbook is the source, so this reads it rather than restating the string. - """ + def test_the_runbook_bootstraps_the_review_skill(self) -> None: + """Copilot reaches the provider-independent review contract from its always-on file.""" text = RUNBOOK.read_text(encoding="utf-8") - self.assertIn(f"`{pr_review.REVIEWER}`, with **no `[bot]` suffix**", text) - self.assertFalse(pr_review.REVIEWER.endswith("[bot]")) + self.assertIn(".github/skills/code-review/SKILL.md", text) - def test_the_suppressed_pattern_is_the_runbook_alternation(self) -> None: - """The heading wording has changed once, so the pattern tracks the runbook, not a memory.""" + def test_the_runbook_forbids_suppressed_findings(self) -> None: + """A finding without a thread cannot participate in the ordinary reply loop.""" text = RUNBOOK.read_text(encoding="utf-8") - self.assertIn(f'test("{pr_review.SUPPRESSED.pattern}")', text) + self.assertIn("Never suppress a finding", text) - def test_the_refusal_pattern_is_the_runbook_alternation(self) -> None: - """A refusal reworded once is a refusal read as coverage, so the pattern is not a memory.""" - text = RUNBOOK.read_text(encoding="utf-8") - self.assertIn(f'test("{pr_review.REFUSAL.pattern}")', text) - # The published filter is single-quoted, which no spelling of the apostrophe survives. - self.assertNotIn("'", pr_review.REFUSAL.pattern) - - def test_the_vetted_coverage_spellings_are_the_ones_the_runbook_publishes(self) -> None: - """The wording drifts, so the vetted list is read out of the runbook rather than recalled. + def test_the_runbook_publishes_the_machine_readable_coverage_marker(self) -> None: + """The instructed shape is stable while the parser retains legacy prose readers.""" + text = (REPO / ".github" / "skills" / "code-review" / "SKILL.md").read_text( + encoding="utf-8" + ) + marker = "<!-- fleet-review: reviewed=N changed=N findings=N -->" + self.assertIn(marker, text) + self.assertIsNotNone(pr_review.read_coverage(marker.replace("N", "1"))) - The two published lines are pulled out with the script's own opener and handed to its own - parser, which holds the pair in step in both directions: a spelling the runbook adds and - this cannot read fails here, and so does one this reads that the runbook never named. - """ - text = RUNBOOK.read_text(encoding="utf-8") - published = [ln.strip() for ln in text.splitlines() if pr_review.is_coverage_line(ln)] - self.assertEqual(2, len(published), published) - for line in published: - with self.subTest(line=line[:44]): - self.assertIsNotNone(pr_review.read_coverage(line)) - # The published filter is single-quoted in the shell, which no apostrophe survives. - self.assertNotIn("'", pr_review.COVERAGE_COUNTS.pattern) - - def test_the_runbook_names_the_partial_round_as_a_state_that_blocks_a_merge(self) -> None: + def test_the_runbook_names_partial_coverage_as_a_state_that_blocks_a_merge(self) -> None: """A verify step reading `commit.oid` alone is what let five partial rounds merge.""" text = RUNBOOK.read_text(encoding="utf-8") - self.assertIn("Coverage of the head is not coverage of the diff", text) + self.assertIn("partial or absent coverage statement", text) def test_the_only_writes_are_the_three_named_here(self) -> None: """Every write this script makes is one of three, and each arrived as a reviewed change. @@ -2885,12 +2897,11 @@ def test_the_only_writes_are_the_three_named_here(self) -> None: self.assertIn("resolveReviewThread", source) self.assertIn("requestReviews", source) - def test_the_mutations_are_the_ones_the_runbook_publishes(self) -> None: - """A helper performing a different write than the documented one is undocumented.""" + def test_the_runbook_routes_mutations_to_the_script(self) -> None: + """Provider mechanics have one executable owner instead of copied query snippets.""" text = RUNBOOK.read_text(encoding="utf-8") - for name in ("addPullRequestReviewThreadReply", "resolveReviewThread", "requestReviews"): - with self.subTest(mutation=name): - self.assertIn(name, text) + self.assertIn("scripts/pr_review.py", text) + self.assertNotIn("mutation(", text) def test_no_argument_accepts_a_thread_id(self) -> None: """The failure is an id typed into a mutation, so the fix is having nowhere to type one.