diff --git a/AGENTS.md b/AGENTS.md index 7d7ed09..9f03b6d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -70,16 +70,16 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | Recording a durable lesson or updating governance | `Durable Knowledge and Self-Improvement` | | Any push, API mutation, comment, label, or merge, or which checkout the work happens in | `Repository Boundaries and Write Safety` | | Quoting data into a comment, commit, test, or doc | `Representative Data in Agent-Authored Text` | -| Committing, signing, rebasing, force-pushing | `Git and Commit Rules` | -| Branch choice, promotion, keeping branches in sync | `Branching Model` | -| Releasing, version bumps, publishing | `Release Model` | -| A live config repo rather than a code repo | `Operational Repositories` | -| Onboarding a repo or running a conformance sweep | `Repository Onboarding and Conformance` (hub only, not carried) | +| Committing, signing, rebasing, force-pushing | `Git and Commit Rules`, packaged as the `git-commit-conventions` Skill | +| Branch choice, promotion, keeping branches in sync | `Branching Model`, packaged as the `operational-vs-release-workflow` Skill | +| Releasing, version bumps, publishing | `Release Model`, packaged as the `operational-vs-release-workflow` Skill | +| A live config repo rather than a code repo | `Operational Repositories`, packaged as the `operational-vs-release-workflow` Skill | +| Onboarding a repo or running a conformance sweep | `Repository Onboarding and Conformance` (hub only, not carried). Standing up a new repo from a hub checkout is packaged as the `standup-a-repo` Skill, and resyncing one already stood up the same way is `resync-a-repo`, both hub-context only | | Running a fleet gate, the review digest, or the config script | `Hub-Hosted Tooling` | -| Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions` | -| Any prose, comment, doc, or line-ending change | `Documentation Style Conventions` | +| Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions`, packaged as the `comment-and-doc-style` Skill | +| 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` | -| Requesting, answering, or closing a review | `PR Review Etiquette` | +| Requesting, answering, or closing a review | `PR Review Etiquette`, packaged as the `pr-review-conduct` Skill | | Reporting progress or asking the user something | `Communicating with the User` | | Editing a workflow YAML file | `Workflow YAML Conventions` | | Choosing an OS, runtime, or toolchain target | `Supported Development Platforms` | @@ -87,3 +87,5 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | Editor settings and tasks | `Editor and Tasks` | | The About panel, description, or repo toggles | `Repository Details` | | Where a file belongs in the tree | `Repository Layout` | + +Some of the rules above are also packaged as Claude Code / opencode / Codex Skills, hand-authored at `.agents/skills/` in the hub (not a repo-relative link here, since that path is hub-local and not carried into every fleet repo), so they surface automatically instead of needing to be re-read every session. `scripts/` is hub-hosted and reached rather than carried, per "Hub-Hosted Tooling", so run the installer from a hub checkout: `python3 scripts/skills_install.py` (or the `.sh`/`.ps1` wrapper) once per machine, from `github.com/ptr727/ProjectTemplate`, installs them for every repo touched from that machine. `python3 scripts/skills_install.py --report`, also from a hub checkout, says whether this machine is current. A rule that keeps needing to be restated is a sign the install is missing or stale, not that the rule does not exist. Keeping a repo's own carried `.github/copilot-instructions.md` in sync with the hub, without losing that repo's own "Disproved Claims" ledger entries in the process, is `copilot-instructions-keeper`, a skill about maintaining that file rather than a rule extracted from it, since the file itself is read directly by the Copilot bot and stays fully intact everywhere it is carried. Checking, from inside this repo's own session with no operator watching, whether this repo and this machine are actually current against the hub is `fleet-conformance-check`, new content rather than a rule extracted from a section, the counterpart to `resync-a-repo` that needs no standing hub checkout or named target beyond the repo the session is already in, even though its own check fetches a hub checkout to reach `scripts/skills_install.py`. Opening a pull request against a repository outside this fleet, one the maintainer does not control, follows a different workflow entirely, new content rather than a rule extracted from a section, packaged as `upstream-contribution-workflow` and independent of the target repo's own type or workflow model. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index e4bf7ee..fc2057d 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -39,75 +39,60 @@ Agent-authored text illustrates with data the agent constructed, never with data ## Git and Commit Rules -- **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless the developer has explicitly authorized the agent to commit for the current ask ("commit this", "open a PR", etc.). Authorization is scope-bound: it covers the commits needed for that specific task, not a blanket commit license for the rest of the session. -- **"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 work that stops 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 (see the Branching Model), and never with `--force`. Holding a commit locally is the narrower case, so it happens when the developer asks for it rather than by default. -- **Check the working tree for the maintainer's own uncommitted edits before committing.** The maintainer hand-edits files live (often `README.md`/`HISTORY.md`, sometimes with the editor's LF->CRLF flip on top). Review `git status` first. If there are changes you did not make, ask whether to include them rather than bundling half-finished work or stranding it in an unrelated commit. -- **All commits must be cryptographically signed (SSH or GPG).** Branch protection enforces this on both branches, and unsigned commits are rejected on push. Signing depends on environment configuration: `git config commit.gpgsign true`, a configured `user.signingkey`, and a working signing agent (loaded `ssh-agent` for SSH, or `gpg-agent` for GPG). If signing is not configured in the environment, **do not commit**. Surface the missing config to the developer and stop at `git add`. Verify before any agent-authored commit (`git config --get commit.gpgsign && ssh-add -L` or the GPG equivalent). **Signing must be live before the *first* commit, not retrofitted.** Turning on `Require signed commits` against a branch that already has 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 (a rebase preserves the `author` field but not the original signatures, and you cannot sign another contributor's commits for them). During new-repo setup, never create commits until signing is verified. -- **Commit under the committing account's own GitHub `noreply` identity, never a private, personal, or invented address.** The `author` and `committer` on every agent-authored commit are the GitHub `noreply` address of the account whose key signs the commit (above). GitHub issues these in a `username@users.noreply.github.com` or `ID+username@users.noreply.github.com` form, and for this single-maintainer fleet it is the owner's `ptr727@users.noreply.github.com`. Do not set `user.name`/`user.email` to a fabricated persona, bot name, or product name, and do not commit under whatever identity the environment happens to carry: verify `git config --get user.email` is that GitHub `noreply` address before committing. **Verify it, do not set it.** The identity is host configuration, set globally once, so a repo-local `user.email` is redundant where the global is right and a wrong identity where it is not, and it silently shadows the global it overrides. A mismatch is a host fault to surface to the maintainer rather than to patch per repo, because a local override hides a broken host that then commits under the wrong identity in every other repo on that machine. A wrong identity is not cosmetic: a private email trips GitHub's email-privacy push protection (GH007), and an unrecognized or invented author pollutes history. Identity is separate from signing: a wrong author does not by itself fail the signature rule, but the ad-hoc identities that produce it are typically also unsigned, which the signing rule above then rejects on push. -- **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. -- **A history rewrite includes only the commits that must change, and re-identifies any commit it rewrites that is not yours.** Filtering history (`git filter-repo` / `filter-branch`, e.g. to strip PII) rewrites the touched commits and you re-sign them with your key, while the tooling preserves each commit's original `author` and `committer` unless told otherwise. GitHub verifies a signature against the commit's `committer` identity, so a signature from your key over a commit still committed by a bot (`dependabot[bot]`, `github-actions[bot]`) or by GitHub's web-flow does not match its committer and is marked `unknown_key`/unverified, which a require-signed-commits ruleset then rejects. Two gates keep committer and signature aligned. **First, scope the rewrite to only the commits that must be modified.** By default those are your own, whose committer is already your identity, and a commit that does not need changing is kept out of the rewrite so its identity and signature are never touched. **Second, if a commit that must change is not yours, set its `committer` to the signing identity before re-signing** (and its `author` too, since a rewrite that alters the content should not keep attributing it to the bot), so the committer GitHub verifies matches your key. The original bot attribution is deliberately given up as the cost of having to rewrite it. Never leave your signature over a commit committed by another identity. Verify after the rewrite that every rewritten commit is signed and committed under your identity (`git log --show-signature`). -- **Never run destructive git commands** (`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`) without explicit developer instruction. +The fleet's mechanical git rules: default to staging rather than committing, commit means commit +and push, every commit is signed and carries the committer's own verified GitHub `noreply` +identity, never force push, a history rewrite re-identifies only the commits it touches that +aren't yours, and destructive git commands run only on explicit instruction. + +This is packaged as the `git-commit-conventions` Skill at +`.agents/skills/git-commit-conventions/SKILL.md` in the hub, not a repo-relative link since that +path is hub-local and not carried into every fleet repo. The summary above sketches the contract. +Read the skill for the full rules. ## Branching Model -- **Two workflow models, set per repo by the registry `workflowModel` field.** Most repos are `release`: they ship versioned units of delivery through the feature -> `develop` -> `main` flow this section describes. **Operational** repos instead track a live service's running state and differ substantially (direct-to-`develop`, advisory CI, dispatch-only release), so see "Operational Repositories". The rest of this section is the `release` model unless noted. The promotion mechanics (never delete `develop`, EOL-conflict resolution) apply to both. -- `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 (see "Release Model" below). 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). 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. -- **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 the repo's "Automatically delete head branches" toggle, which is why that toggle is [kept off](./repo-config/settings.json)) 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///git/refs" -f ref=refs/heads/develop -f sha="$(gh pr view --json headRefOid --jq .headRefOid)"`. - - **Spurious EOL-only conflicts resolve by taking `develop`.** When develop declared workflow YAML as LF while main is still CRLF, `develop -> main` conflicts *whole-file* on those paths. develop's `required_linear_history` + 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 `) **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 -> `main`. Verify no genuine main-only content is dropped (build/test where the repo supports it). -- **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`). A feature -> `develop` PR merges into `develop`, so the keyword never fires there. Reference the issue in the `develop` PR body if useful, but put the actual closing keyword on the promotion PR. If a `develop` PR merges with the keyword on it, the keyword does nothing and the issue stays open, so put it on the promotion PR body instead. **Closing by hand is the ordinary route wherever the keyword cannot fire, not a repair for a botched promotion.** An issue closes when its work is verifiably complete, and the keyword automates that for the common case where completion and promotion coincide rather than adding a second condition to it. Two cases fall outside the keyword and both close by hand: a promotion that merged without it, and work complete on `develop` with no promotion imminent, where waiting holds a finished issue open for however long the next promotion takes. Cite the squash SHA that completed the work, and re-read that commit against the branch before closing rather than trusting the claim that asked for the close, so the closing comment carries evidence a reader can check. -- **Both rulesets intentionally 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` (see below) 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. - - See [`repo-config/README.md`](./repo-config/README.md) "Rulesets" for the configured state. -- **Configuring branch protection on a fleet repo: don't hand-build the rules.** Reconstructing the rules 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///rulesets"` (`gh ruleset` is read-only). The names are load-bearing, since this file 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. -- **Bots (Dependabot and codegen) target both `main` and `develop` in parallel.** [`.github/dependabot.yml`](./.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, and both branches receive their updates directly). The merge-bot ([`.github/workflows/merge-bot-pull-request.yml`](./.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. -- **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 ` or the GitHub UI) when ready. -- **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. -- **Codegen regenerates committed files, and its output must be deterministic from its inputs, never per-run state.** 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, whether code changes or pure data) 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. - - *Reference:* 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`](./repo-config/README.md) "Secrets" for which secrets each mechanism needs. +Two workflow models, set per repo by the registry `workflowModel` field. Most repos are +`release`: squash-only feature branches into `develop`, merge-commit-only `develop -> main` +promotions, forward-only with no back-merges, and two promotion traps worth knowing before the +first one (never delete `develop`, resolve an EOL-only conflict by taking `develop`'s side). +**Operational** repos differ substantially (direct-to-`develop`, advisory CI, dispatch-only +release), covered as a delta rather than a separate model. + +This is packaged as the `operational-vs-release-workflow` Skill at +`.agents/skills/operational-vs-release-workflow/SKILL.md` in the hub, not a repo-relative link +since that path is hub-local and not carried into every fleet repo. The summary above sketches +the contract. Read the skill for the full rules, including branch protection configuration, the +dual-target bot wiring, and the operational-repo delta in full. ## Release Model -The **two-phase model is the default**: PRs build fast, publishing is batched. See [`WORKFLOW.md`](./WORKFLOW.md) for the full rationale. The load-bearing rules follow. The auto-publish paths (bot push, schedule) apply to `release` repos. **Operational** repos differ, with a dispatch-only release and no auto-publish. See "Operational Repositories". - -- **PRs smoke-test only.** [`test-pull-request.yml`](./.github/workflows/test-pull-request.yml) always runs unit tests, then a `dorny/paths-filter` `changes` job gates a **reduced** build of only the changed targets (Docker `linux/amd64` only, executable on a representative runtime subset), never pushing. Build-workflow files are intentionally not in the path filters, because a filter can't tell a logic change from an action-version bump, so a workflow-only change isn't smoke-built. The reusable workflows are exercised by the next run that uses them (a later code PR's smoke build, or the scheduled/publish run). Workflow YAML is still linted in CI by the lint job's `actionlint` step. Also run `actionlint` locally before pushing to catch issues early. -- **A human merge never auto-publishes.** [`publish-release.yml`](./.github/workflows/publish-release.yml) is the sole publisher, and each run builds the **single trigger branch** (`main` a release, `develop` a prerelease). A first `plan` job decides once whether the run publishes and every other job gates on its output. It publishes on a **`workflow_dispatch`** of `main`/`develop` (the human-initiated release), a **code-affecting bot push to `main`** (the codegen App merges every Dependabot/codegen PR, so `github.actor` is the gate, and a human merge/promotion to `main` skips), or a **weekly `schedule`** (Docker only, to refresh the base image). The `push` is main-only and paths-filtered, so a develop bot merge and an Actions-only bump publish nothing. A source-only repo publishes on dispatch only. -- **Required check.** The `changes` job is in the `Check pull request workflow status job` aggregator's `needs` and **must succeed** (not just "not fail"), because a paths-filter error must never let a target-changing PR merge with its smoke build silently skipped. Skipped smoke jobs (no matching change) pass, and `failure`/`cancelled` blocks. -- **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` has per-target `enable_*` gates and self-contained leaf tasks, so a project that drops a target deletes: its `build--task.yml`, the matching job + `github-release` `needs` entry in `build-release-task.yml`, its path-filter entry in `test-pull-request.yml`, and (for PyPI) the `publish-pypi` job in `publish-release.yml`. 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`](./.github/workflows/publish-release.yml) (single-branch publish plan), the `get-version` + `github-release` jobs inside `build-release-task.yml`, `get-version-task.yml`, `build-datebadge-task.yml`, and the aggregator shape of [`test-pull-request.yml`](./.github/workflows/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 (the `build--task.yml` leaf tasks) is what a derived project owns and replaces. The contract that keeps the seam clean: **a target contributes files to the GitHub release by uploading a workflow artifact named `release-asset--`.** The `github-release` job collects every `release-asset--*` 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 it (the tag-the-commit + create-the-release + 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--` 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 (`pattern:`/`merge-multiple:`) and breaks its verbatim carry. - - **What a repo still curates** (this is by design, not a leak): the *list* of leaf jobs in `build-release-task.yml`. Per **Per-target subsetting** above, you delete the target jobs you don't ship and add the one(s) you do. `build-release-task.yml`'s `github-release` job is untouched, but the file is not byte-identical because its `needs`/job list reflects your targets. Making that list itself target-agnostic is a larger "factor build from orchestration" refactor that is intentionally **not** done. - - **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): one leaf task per output, each uploading `release-asset--`. A data-only repo (e.g. a symbol library) has exactly one such task: validate -> `zip` -> upload `release-asset--library`. It deletes the nuget/pypi/executable/docker jobs and the `publish-pypi` job, keeps `github-release` as-is. This is also where the .NET `build-executable-task` lives, and it is *not* a generic file step but specifically `dotnet publish` of the console app, so replace it wholesale, don't adapt it. - - *Package-registry pushes* (NuGet.org, PyPI): the leaf task both builds **and** publishes to its registry. NuGet pushes from inside `build-nugetlibrary-task` (`dotnet nuget push --skip-duplicate`) *and* also uploads a `release-asset-*` (.7z) for the GitHub release. PyPI is split: `build-pypilibrary-task` only builds + uploads the `pypilibrary-build-` artifact, and the separate `publish-pypi` job in `publish-release.yml` does the OIDC Trusted-Publishing upload (so `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` pushes multi-arch tags directly and contributes **no** `release-asset-*`. The image tag is build-layer-owned, so drive it from whatever version source fits (NBGV `SemVer2`, an upstream-release pin, or a per-image matrix). To publish the Docker Hub repository overview, `publish-docker-readme-task.yml` pushes `Docker/README.md` 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`. - - *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`/`date-badge` jobs to curate. Its whole release is the standalone [`publish-release.yml`](./.github/workflows/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. -- **Versioning is semantic and maintainer-controlled.** The `version` (major.minor) in [`version.json`](./version.json) is the version floor. NBGV appends the git height (the SemVer patch position) for the build version. `main` (the public release ref) builds a stable `X.Y.`, and `develop` builds a prerelease `X.Y.-g`. The maintainer edits `version.json`. Dependency bumps, CI/workflow fixes, and doc edits leave it untouched. - - **Bump `version.json` only for functional changes, by maintainer instruction.** Raise the major/minor when the work being introduced warrants a new semantic version (a new feature, a behavior or API change, a breaking change), and do it in the PR that introduces that work (typically on `develop`). Do **not** bump on a fixed cadence or mechanically after a release. NBGV advances the patch (git height) on every commit automatically, so a release always gets a fresh build version without any `version.json` edit. - - **No post-release bump, and no develop-ahead requirement.** NBGV advances the patch (git height) on every commit, so a release always gets a fresh build version with no `version.json` edit and there is no `bump-version-X.Y` PR after a release. A `develop -> main` promotion carries whatever `version.json` is current: a promotion with a functional bump releases that new version on `main`, and a maintenance-only promotion carries the unchanged `version.json` and `main` advances only its NBGV height. -- **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 `version.json` floor). NBGV owns the patch/build position, so a concrete three-part number in a doc is both wrong (the real build height differs) and a maintenance trap. "Correcting" `1.0` to `1.0.0` is a defect, not a fix, and it has blocked a release. -- **Wrapper repos that track an upstream release.** A repo wrapping an upstream release uses `check-upstream-version-task.yml`: a resolver command prints the upstream version(s) as 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. +The **two-phase model is the default**: PRs build fast, publishing is batched, a human merge +never auto-publishes on its own. See [`WORKFLOW.md`](./WORKFLOW.md) for the full CI/CD contract. +Publishing fires on a manual dispatch, a code-affecting bot push to `main`, or (Docker only) a +weekly schedule, and versioning is semantic and maintainer-controlled (NBGV owns the build number, +the maintainer owns the `major.minor` floor). **Operational** repos differ, with a dispatch-only +release and no auto-publish bots. See "Operational Repositories" below. -## Operational Repositories +This is packaged as part of the `operational-vs-release-workflow` Skill at +`.agents/skills/operational-vs-release-workflow/SKILL.md` in the hub, not a repo-relative link +since that path is hub-local and not carried into every fleet repo. The summary above sketches +the contract. Read the skill for the full rules, including the release-target build layer, the +no-op republish guarantee, and wrapper-repo upstream-version tracking. -The registry `workflowModel` field is `release` (the default) or `operational`. This section is the operational delta, and every other rule in this file is the `release` model unless it says otherwise. +## Operational Repositories -**Operational** repos track a live service's running state rather than shipping versioned units of delivery: live-service config such as Home Assistant, ESPHome, Vantage, and home automation. +The registry `workflowModel` field is `release` (the default) or `operational`. **Operational** +repos track a live service's running state rather than shipping versioned units of delivery +(live-service config such as Home Assistant, ESPHome, Vantage, and home automation): commits go +directly to `develop`, CI runs on the push as advisory feedback only, a PR still exists for a +change worth reviewing, the `main` promotion gate is unchanged, and release happens only by manual +dispatch. -- **Commit configuration directly to `develop`.** There is no feature branch, and the maintainer commits straight to `develop` and *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.** 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. The lint workflow's `pull_request` trigger therefore names `develop` alongside `main`, since a trigger set naming `main` alone matches nothing on a `develop` PR and that PR would merge with no validation at all, which is the one output [WORKFLOW.md](./WORKFLOW.md) D1.2 forbids on *any* PR. The result is **reported and not required** there, because a required status check on `develop` would gate the direct push too and dissolve the allowance the model is built on. The asymmetry is the point: on the direct-commit path CI can only be advisory after the fact, while on the PR path the change has not landed yet, so validating it is both pre-merge and actionable. -- **Take the pull request whenever the change is not one a reader takes in at a glance and reverts cleanly.** The grant exists for the one-line config edit whose review round costs more than it protects, and that reason stops applying well before a change gets large, so what decides it is the shape of the change rather than 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, which is the same signal read from the other side. This stays guidance by construction, because adding a `pull_request` rule to the [operational `develop` ruleset](./repo-config/operational/develop.json) would gate the direct push too and withdraw the very allowance the model exists to give, so the judgment sits with whoever is about to commit and nothing mechanical will make it for them. -- **The `main` promotion gate is unchanged.** The [`main` ruleset](./repo-config/main.json) is shared with `release` repos, so the `develop -> main` PR still **enforces** the required `Check pull request workflow status job`. For an operational repo that check is lint/validation only, meaning editorconfig/EOL plus domain linters (a Home Assistant or ESPHome config validation, a firmware build) and 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, so they publish **only** on a manual `workflow_dispatch`, the 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, and 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. +This is packaged as part of the `operational-vs-release-workflow` Skill at +`.agents/skills/operational-vs-release-workflow/SKILL.md` in the hub, not a repo-relative link +since that path is hub-local and not carried into every fleet repo. The summary above sketches +the contract. Read the skill for the full rules, including when a config change still earns a +pull request. Line-ending governance for an operational repo is in [Line Endings](#line-endings), where its `[*]` default follows the consuming app's native platform per the registry `lineEndings` field, not the fleet CRLF default. @@ -119,6 +104,8 @@ The fleet's tooling lives in the hub once and a repository runs it from there ra **Reaching it is a checkout of the hub rather than a copy of one file.** A tool reads the payloads, tables, and sibling modules beside it, so a single file lifted out of the tree runs against whatever the caller happens to have, which is the copy problem again in a shorter loop. Read `main`, the promoted and gated state, and fetch immediately before running, because a clone is whatever it last fetched rather than the branch it names, and a stale clone answers confidently instead of failing. Name the tool by its path in that checkout and name the target explicitly, since a tool that defaults to the current directory or the current repository resolves somewhere either way, and a result computed against the wrong repository is well-formed. Which directory the command runs in is the tool's own contract rather than a rule here, so a gate reading a working tree runs in the repository under test while a tool taking its target as an argument runs anywhere and is given one. What the rule forbids is letting a default decide which repository the answer is about. +**A loader is outside this section rather than exempt from it.** The rule above governs a tool that reads hub content, because a tool reads the payloads, tables, and sibling modules beside it. A loader reads none of them: it obtains a tree and hands control to a tool inside that tree, on a host that cannot yet obtain one. The bound is what it may contain rather than who runs it, and it is one line: a loader references no path inside the tree it fetches except the single entry point it hands control to, and everything else it touches is the machine or the network. A loader that grows a second path into that tree has become a tool and is governed above. + **A report or finding a hub tool produces names the hub commit it ran from.** The tool moves independently of the repository it measures, so a verdict carrying no hub commit cannot be re-run, and two runs that disagree cannot be attributed to the tree or to the tool. The obligation is the runner's rather than the tool's, since a tool reports on the repository it measures rather than on itself, so the commit is read from the hub checkout and written into the report beside the verdict. This is the same requirement "Verification Discipline" places on any claim that gets acted on. **CI reaches the same tooling as a pinned action.** A runner holds no hub checkout, so a workflow consumes the hub's composite action and pins it to a commit SHA, per the action-pinning rule under "Workflow YAML Conventions". The pin is what makes a released repository's gate reproducible, since an unpinned consume lets a later hub commit fail a re-run of a change that already passed. Branch-dependent behavior belongs inside the consumed action, because `uses:` takes no expressions and a per-branch ref therefore cannot be selected in the workflow file. @@ -127,105 +114,27 @@ The fleet's tooling lives in the hub once and a repository runs it from there ra ## Pull Request Title and Commit Message Conventions -### Format - -- Imperative subject summarizing the change, <=72 characters, no trailing period. ("Add 24-hour PM2.5 average sensor", not "Added X" or "Adds X".) -- Optional body, blank-line separated, explaining *why* the change is being made when that's non-obvious. The diff shows *what*. - -### Rules +A PR title and a commit message share one contract: an imperative subject, 72 characters or fewer, no trailing period, no vague titles like `update stuff` or `wip` (Dependabot's `Bump X from Y to Z` is fine as-is), no unsolicited `Co-Authored-By:` lines, and no release-bump magnitude in the title, since Nerdbank.GitVersioning computes the next version from `version.json` and git history. -- Don't write `update stuff`, `wip`, or other vague titles. (Dependabot's default `Bump X from Y to Z` titles are fine, so keep them.) -- Don't add `Co-Authored-By:` lines unless the developer explicitly asks. -- Don't put release-bump magnitude in the title, so no "minor", "patch", "release v0.2.0", etc. Nerdbank.GitVersioning computes the next release version from `version.json` + git history. Dependency versions in dependency-bump titles are fine and expected. -- Use US English spelling and match the existing heading style of the file you're editing: title case with lowercase short bind words (a, an, the, and, but, or, of, in, on, at, to, by, for, from); hyphenated compounds capitalize both parts unless the second is a short preposition (*Built-in*, *EPA-Corrected*, *24-Hour*). - -### Examples - -```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 -``` +This is packaged as the `comment-and-doc-style` Skill at `.agents/skills/comment-and-doc-style/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the full format, rules, and examples. ## Documentation Style Conventions -- **Carried files carry no coordination references.** In the files the fleet carries (`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, the `repo-config/` and `spec/` files, the carried `AUDIT.md`), two things are banned. **Any reference to the template repo**, in prose or in a link: the coordination flow is machinery a consumer of that repo should never have to see, and naming where a file came from is the derived-from framing the present-tense rule below 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 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 is governed normally, so a repository that names the destination in a paragraph it owns is still in breach. 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 maintainer supplies the destination out of band. **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, so where the content came from stays out and 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 to re-point at a local path, which is what left the reference resolving differently in every copy before. In `AGENTS.md` and `GOVERNANCE.md` it 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 reached rather than carried, and how, is in [Hub-Hosted Tooling](#hub-hosted-tooling). And **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. **A contextually relevant link to a related project is not a coordination reference, and is expected.** 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 rule governs carried template content. A repo's own `README.md` and topical docs are its own content, not carried verbatim, and it does not reach them. This pairs with the present-tense rule below: state the current shape, not a history of which repo it came from. - -### Markdown +The fleet's prose and formatting contract: what a carried file may reference, how Markdown links, headings, and tense are structured, the comment philosophy, the ASCII character-set tiers, the line-ending policy, and how a quantitative claim in a doc stays honest. Applies to docs and code/workflow comments alike. -- **Reference-style links in human-facing docs.** Every Markdown file uses reference-style links only, **except** the four agent-instruction files that keep inline links: `AGENTS.md`, `GOVERNANCE.md`, `OPERATIONS.md`, and `.github/copilot-instructions.md`. Those are read one section at a time rather than end to end, so an inline target resolves where it is read while a definition at the bottom of the file is never reached. The exception is that closed list rather than a category to argue from, so every other Markdown file follows the rule whatever its audience. Under the rule: every URI (internal path, anchor, external URL, or shield image) is defined at the **bottom of the file**, split into groups by type under an HTML-comment header (e.g. ``, ``, ``, ``) with each group's definitions alphabetized by reference name. **The sort key is the reference name alone**, the text inside the brackets, never 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 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. **Reference names are contextual and encode the target and its group**: `foo-shield` for a shield image, `foo-link` for an external URL, and a bare `foo` for a local path or anchor (e.g. `[license-shield]`, `[releases-link]`, `[repo-config]`), never numeric (`[1]`) or opaque. No inline `[text](uri)` targets in prose. **A URL inside a fenced code block stays inline**, because reference links do not resolve in code blocks, so do not extract it, 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**, since an orphaned definition fails the no-unused-defs rule. The one exception is the Table of Contents, whose entries stay inline anchor links (see Table of Contents below). -- **Table of Contents.** Generate it with the Markdown All in One extension, which fills and auto-updates the list on save, so leave the `## Table of Contents` heading for the extension to populate and never hand-author or hand-edit the entries. Exclude a heading with an inline `` marker on it (the badge/build header block and the `## Table of Contents` heading itself carry it). The workspace sets which heading levels appear. -- One logical paragraph per line, with no hard-wrap line-length limit. For an intentional hard line break within a block (stacked badges, status, or license lines), end the line with a trailing backslash (`\`). This explicit form is preferred over trailing whitespace and is not treated as a paragraph split. -- Headings follow the title-case-with-short-bind-words rule from the PR-title section. -- **Write in the present tense, describing only the current state.** The reader has no knowledge beyond what they are reading, so state what *is* (what to know, do, follow, or avoid), never a change from a prior state. Write "X does Y", never "X *now* does Y", "X *no longer* does Z", "X *still* does W", or "changed/switched/restored to Y". 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, not in `README.md`, `AGENTS.md`, or other living docs. -- **When you change a behavior, search for prose that asserts the old one.** Updating the guarantee or rule you are consciously editing is not enough: comments, diagram labels, reusable-workflow input descriptions, and audit statements elsewhere may still describe the prior behavior, and each was accurate when written. Grep for the old behavior's distinctive phrasing and fix every instance. No linter catches this, because markdownlint, cspell, actionlint, and editorconfig-checker all pass on a claim that is merely untrue, so the sweep is the only mechanism that will. This is the maintenance counterpart to the present-tense rule above: that one governs how to phrase a doc, this one how to keep it true when the behavior underneath it moves. +This is packaged as the `comment-and-doc-style` Skill at `.agents/skills/comment-and-doc-style/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the full rules. ### 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. -- Write for the human reading *this* project's code now: state only the non-obvious *why*. No cross-project references (do not name other repos), no historic or design narrative, no rule citations, since governance lives in this file, not echoed inline. -- **Keep it short.** One line is the default. A comment earns a second line only by carrying a constraint the code cannot. Most comments are one sentence, and never restate *what* the code does, since a well-named symbol already says it. -- **Structured, not prose: one sentence per line, and never wrap a sentence across lines.** No block paragraphs and no multi-sentence run-ons. A comment that genuinely needs several sentences is several lines, each a single sentence. A sentence too long for one sensible line is too long, so split the thought. -- **A comment line that opens prose starts with a capital.** A line opening in lowercase reads as the continuation of the one above it, so a sentence that genuinely starts there is capitalized. A trailing fragment that annotates the code on its line is a label rather than a sentence, and the version pin the action-pinning rule requires is one, so neither takes a capital. Where the first word is a tool or identifier whose own casing is lowercase, restructure so the sentence does not open on it rather than capitalizing the name against its official spelling. -- **A multi-line comment shows whether it is a continuation or a list.** A continuation of the same topic stays unindented, one sentence per line. Mark a sub-topic with a `-` after the comment marker (`# -`, `// -`), and only for genuine sub-topics: parallel items hanging off a lead line, never a continuation of one thought. -- **No class-, type-, or file-header summary comment blocks.** A type or file gets a comment only for a specific non-obvious point, kept terse, never a block summarizing what the file contains or what the class is for. A summary restates the declaration below it, goes stale as the file grows, and is the file-scope form of the design narrative and verbosity creep this section already bans. A license or provenance header a tool or policy requires is not a summary and is unaffected. -- **Do not grow a comment across edits.** When you touch code near an existing comment, the comment must come out **same length or shorter**, never appending "one more clause" of rationale. If a block comment has crept to multiple sentences of prose, cut it back to its single load-bearing point as part of your change. Verbosity creep is the specific regression to prevent: every iteration that adds a clause is a regression, not an improvement. - -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. -``` +The full comment philosophy, what earns one, structure, capitalization, growth discipline, is in the `comment-and-doc-style` Skill referenced above. ### 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) -> **restructure the sentence**. Two sentences, or a comma. Do not substitute a spaced hyphen. - - right arrow (U+2192) -> `->`, double arrow (U+21D2) -> `=>` - - curly quotes (U+2018/U+2019/U+201C/U+201D) -> straight `'` and `"` - - ellipsis (U+2026) -> `...`, bullet (U+2022) -> `-` - - no-break space (U+00A0) -> a space, non-breaking hyphen (U+2011) -> `-` -- **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 U+2264 directly before `35` in a table of sensor thresholds is the range it describes and stays. The same character between two words, as in a sentence about one check running before another, is prose and takes `<=`. -- **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. Do not approximate it away, and do not spell it out. -- **Unicode the developer deliberately typed** stays regardless of tier, such as emoji used for emphasis or as callout markers, for example the warning markers a maintainer placed in `README.md`. Never strip the developer's own characters. The carve-out governs what an agent may rewrite rather than what the gate reports, so an un-tiered character of this kind is still a `charset-unknown` finding until someone classifies it. It covers developer-authored text, and is not a license for the agent to add emoji. -- **An unrecognized non-ASCII character is reported, not allowed.** Classify it into a tier above before using it. A gate that passes whatever it does not recognize stops gating as the character set grows, which is the silent-narrowing failure named under "Verification Discipline". -- **No spaced hyphen joining or interrupting a sentence.** The em-dash-style clause break ` - `, and the paired aside ` - x - `, are both recast: a comma where the clauses are short and closely linked, two sentences where they are not, or parentheses for a genuine aside. This is the same construction the tier-1 em dash is restructured into, so allowing its ASCII spelling would keep the shape and only change the character. A hyphen inside a compound word, a leading list marker, a range, and the `- **Label** - explanation` separator that opens a governed bullet keep their standard use: the last is structurally a colon, and flagging it would restructure the document format rather than the prose. Existing prose is corrected as each file is next edited, not swept. -- **No semicolon in agent-authored prose.** A mid-sentence semicolon joining clauses is recast as a comma or as two sentences. A semicolon separating items in a list that already contains commas keeps its standard use, and a statement terminator in **code** is untouched. A colon introducing an explanation, elaboration, or list is not a semicolon and is unaffected. Existing prose is corrected as each file is next edited, not swept. -- **In carried verbatim content the hub sweeps a class rather than waiting for the next edit.** Correct-as-you-next-edit assumes someone able to edit the file, and a downstream repo cannot edit a verbatim section, since its copy is byte-matched against the hub's. So a review finding against carried rule text is fixed at the hub, for the whole class at once, and re-vendored. Fixing one instance leaves the class to resurface in a different section on the next downstream review, where it still cannot be answered. This is scoped to verbatim content. A file a repo owns, or one carried at `intent` fidelity, is corrected as it is next edited like any other prose. +The full ASCII tier system (never legitimate, legitimate next to a number, always legitimate, developer-typed Unicode) and the semicolon and spaced-hyphen rules are in the `comment-and-doc-style` Skill referenced above. ### Line Endings -- **[`.editorconfig`](./.editorconfig) sets the line ending:** `[*] end_of_line = crlf` is the **default** (every file type is CRLF unless pinned otherwise), with **LF** pinned for the execution-sensitive exceptions: `*.sh`, Dockerfiles, and any individual `.py` executed directly via its shebang (pinned **by path**, e.g. `spec/validate.py`; vanilla `.py` stays CRLF, since Python's universal newlines accept it and it is commonly edited on Windows). Only the LF exceptions are declared. The redundant per-type CRLF rules are intentionally omitted. `.gitattributes` mirrors it: `* -text` (git stores the exact bytes you commit and will **not** normalize) plus the matching LF pins. -- **Choosing an ending for a new file type:** CRLF is the **default**, since cross-platform editors on Windows produce it and it is harmless on Linux for everything except shell. Use LF only when the type **requires** it or CRLF **breaks how it is consumed**: executable scripts/shebangs (`*.sh`, s6, husky), Dockerfiles (CRLF breaks `RUN` heredocs/continuations), and tool-owned formats with a native LF ending (KiCad). **Non-workflow YAML stays CRLF**, since GitHub Actions' parser tolerates it (a repo that also runs yamllint sets `new-lines: disable` to defer to `.editorconfig`). **Workflow YAML (`.github/workflows/*.{yml,yaml}`) is pinned LF** in `.editorconfig`, because Dependabot and Actions rewrite it with LF, so declaring LF keeps it consistent instead of mixed on every bump. This is an LF class **not** backed by a `.gitattributes` pin: git keeps `* -text` (no normalization), and CI's `editorconfig-checker` (EOL-only) catches a mismatch instead. Distinguish where a file is *consumed* from where it is *edited*: consumption on Linux alone does not force LF. A config or pattern file consumed by a Linux tool stays CRLF when the tool tolerates a trailing CR: `.dockerignore` and `.gitignore` are CRLF (their parsers strip the CR), and only a *Dockerfile*, interpreted and broken by a CR in a `RUN` heredoc or line continuation, is LF. -- **Operational (config) repos: the global default follows the consuming application's native platform, not the fleet CRLF 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 must use the ending the app itself reads and writes, and forcing the fleet CRLF default would fight the app. Set the `[*] end_of_line` default to the app's native ending and record it in the registry `lineEndings` field (`lf` | `crlf`): **LF** for a Linux-native app whose config lives in a Linux container (ESPHome, Home Assistant, a devcontainer-only or HACS config), and **CRLF** for a Windows-native editor, for example Vantage InFusion config edited by Design Center on Windows. The execution-sensitive LF pins (`*.sh`, Dockerfiles, workflow YAML) still apply on top, and `.gitattributes` still mirrors the chosen default. This override is for operational repos only, and `release` repos keep the `[*] end_of_line = crlf` fleet default above. Do **not** re-normalize such a repo to the fleet default, which is exactly the over-normalization these per-repo endings prevent. - - **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 (CRLF) lives in its own repo, **not** as a subtree inside a Linux-`lf` config repo. That keeps each repo's default, CI, and checkout matched to one platform and avoids per-path EOL machinery entirely. **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 (e.g. `[/**] end_of_line = crlf`) matching its consumer, treated like any tool-owned format. The global `* -text` in `.gitattributes` already preserves those bytes, so no extra git pin is needed. -- **Scripts and extensionless executables must be LF, and pinned in `.gitattributes`, not just configured.** A CRLF shebang (`#!/usr/bin/env bash\r`) breaks execution. `.editorconfig` sets `[*.sh] = lf`, but that extension-based rule does not match **extensionless** executables (s6 service scripts `run`/`up`/`finish`, husky/git hook scripts like `.husky/pre-commit`), and `* -text` enforces nothing, so a broad normalization pass or an editor can silently flip them to CRLF (it has). `.gitattributes` is the enforcement layer: it carries `*.sh text eol=lf`, and 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`. This pin is mandatory for any repo that overrides s6 init, uses husky/git hooks, or otherwise ships executable scripts. The same explicit-pin rule extends to **tool-owned file formats the base config doesn't key on**: pin them to whatever ending the tool reads and writes so a normalization sweep can't churn them, for example KiCad project/footprint/3D files (`*.kicad_mod`, `*.kicad_sym`, `*.step`), which KiCad writes LF (`*.kicad_mod text eol=lf`, ...). The principle is general: a file class the `.editorconfig` extension rules and `* -text` don't cover needs an explicit `.gitattributes` pin matching its tool's native ending. -- **Pair each such pin with a matching `.editorconfig` override, since the git pin alone is not enough.** `.gitattributes` governs **git** (checkout, commit, `--renormalize`), while the **editor** follows `.editorconfig`, where the `[*] end_of_line = crlf` default still applies to any file no extension rule covers. So even with the git pin, the editor writes a CRLF shebang into an extensionless hook (breaking it when run from the working tree) or re-ends/trims a byte-sensitive data file. Give every extensionless **executable** an editorconfig LF override beside its `.gitattributes` pin (`[.husky/pre-commit] end_of_line = lf`), and for a **byte-preserve data directory** (downloaded or opaque source whose exact bytes the consumer may depend on) disable *all* editor normalization, not just EOL: `[/*]` 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, so the editor enforces neither the global `charset` nor `end_of_line` on that path). Keep these overrides with the line-ending governance (above any `.NET-only` divider), not in the language-style section. -- **New files:** create them with the `.editorconfig`-mandated ending. -- **Editing an existing file:** **preserve the file's current line endings**, and 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 (`file` is unreliable here, per Auditing below). If a diff balloons to the whole file, you flipped the endings, so 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 (a repo-wide cleanup sometimes does), call it out explicitly in the commit/PR description and verify the content separately with `git diff --ignore-cr-at-eol`. -- **Auditing line endings: don't trust `file` or naive `git ls-files --eol`.** The authoritative check is a **byte scan** that classifies by which endings are present: **CRLF-only** (every `\n` is 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`/`.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 an agent 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. -- **Both `.editorconfig` and `.gitattributes` are required.** [`.editorconfig`](./.editorconfig) **and** [`.gitattributes`](./.gitattributes) together govern line endings. A repo missing either file, or one whose `.editorconfig` sets no global `end_of_line` default (e.g. declares it only under `[*.md]`), will accumulate files mixed between LF and CRLF, the exact failure these two files prevent. The canonical form is a `[*] end_of_line = crlf` default plus the LF exception pins, mirroring `.gitattributes`. Carry both files **whole** (the `[*.cs]` block is inert without `.cs` files), including the `*.sh text eol=lf` pin and any extensionless-script path pins. Adopting `.gitattributes` for the first time requires a one-time normalization pass. - -### Quantitative Claims - -- Any quantitative claim in `README.md` (counts, sizes, version floors, supported platforms) must be verified against current code. If 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. +The full CRLF/LF policy (`.editorconfig` and `.gitattributes` defaults and pins, choosing an ending for a new file type, operational-repo overrides, editing discipline, and auditing) is in the `comment-and-doc-style` Skill referenced above. ## Verification Discipline @@ -249,84 +158,11 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -> This "PR Review Etiquette" section is the provider-agnostic review-loop *contract* every fleet repo follows, alongside the [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook" that implements it. Without both in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to ad-hoc (and known-broken) behavior. - -The repo runs a review loop on every PR: local agent iteration plus remote automated review (GitHub Copilot is the configured reviewer). Treat this as a contract regardless of which local agent authored the changes. - -### Merge Gate (read this first) - -**Do not merge, and do not enable auto-merge, unless ALL of these hold:** - -1. Required status checks are green (`mergeStateStatus: CLEAN`), and where they are not, the reason is **read** rather than inferred, because `BLOCKED` is one word for a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, so the response to it is decided from the check states and never from the word, **and** -2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, and because a review can carry the head SHA and still say it read only part of the diff, which is the same clean pass in everything a SHA match can see, so the body's own count of the files it read is checked against the files the pull request changed, **and** -3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and** -4. Nothing in the reviewer's output was a shape the review tooling could not read, since every reader keys on a marker and a marker that changes spelling is a section the reader stops finding and reports as absent, which is how three separate misreadings each reported a clean pass over a review they had not understood. An unrecognized shape blocks this gate on its own, and the remedy is an issue filed against the repository hosting the reader, naming the shape and quoting the body it came from, rather than a judgment about what the new wording probably meant, **and** -5. The maintainer has given **explicit** permission to merge. - -`mergeStateStatus: CLEAN` reflects **only** required statuses, and never open bot review comments, so `CLEAN` alone is **never** sufficient to merge. A green/`CLEAN` PR with an unresolved Copilot finding fails this gate, so treat it as "not mergeable" no matter what the merge-state field says. The agent never merges on its own (consistent with "default to staging", and merging is maintainer-authorized). - -**`BLOCKED` is not self-explaining either, and the remedy differs by cause.** A failed check is a defect to fix, an unresolved thread is a finding to close, and a required check sitting queued with no runner assigned is neither: it is hosted-runner capacity, which no re-request, rebase, or empty commit clears, and which a reader waiting on it cannot tell from patience. Report a blocked merge by naming the blocking check and its state rather than reporting the word, and where the cause is capacity, say so and stop rather than pushing at it, since the bypass is the maintainer's to run and a starved runner is not a reason to weaken a gate. - -**Merging is not releasing.** A merge to a release branch does **not** by itself publish. Publishing is a separate, explicitly configured step in the repo's release pipeline (e.g. a scheduled run, a manual dispatch, or an opted-in publish-on-merge trigger), not an automatic consequence of merging. Never describe a merge as cutting a release, and never trigger a publish without explicit maintainer instruction. - -### Expected Review Loop +The provider-agnostic review-loop contract every fleet repo follows: request a review on every push, confirm it covers the current head SHA and the full diff rather than only part of it, triage every finding including the low-confidence ones a review body collapses rather than threads, and reply and resolve. Never merge on a green or CLEAN merge state alone, since that field can go clean once checks pass and every known thread is resolved while still saying nothing about whether the review covered the current head SHA, read the full diff, or left a suppressed finding, which opens no thread at all, unanswered. -1. Push changes to the PR branch. -2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (reliable end-to-end, per the runbook). The UI is only a fallback. -3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. A review whose body says it did not review is the one exception, and it is terminal in the other direction: nothing follows it, re-requesting the same head repeats it, and the body names what has to change first. -4. Triage findings. -5. Apply fixes or write a rationale for declines. -6. Reply to each thread and resolve what was addressed. -7. Re-run the loop after every fix push until no actionable findings remain. +This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. -Drive the loop to green, meaning a review confirmed on the latest head SHA and every actionable finding closed, then stop and apply the **Merge Gate** above: all five preconditions must hold, and `mergeStateStatus: CLEAN` alone never satisfies it. - -For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract, and that file owns the mechanics. - -### Every Finding Ends in an Action - -**A finding is closed by one of five outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. - -1. **It is real, so fix it.** Reply with the fixing commit SHA. -2. **It is not real, so disprove it in the thread**, with the command and its output, the code path that makes it impossible, or the rule that governs it. The proof is addressed to the reviewer as much as to the maintainer, since a decline it can read is what stops it raising the same thing next round. An assertion is not a proof and does not close a finding. -3. **It is real and deliberately not being fixed, which is the maintainer's call and not the agent's.** Say what the finding is, why the fix is unwanted, and get an explicit answer. Never suppress one by silence, by resolving the thread, or by an answer that reads as a decline while conceding the point. -4. **It is real and worth doing later, so file the issue first and reply with its link.** A deferral recorded only in a thread is lost the moment the pull request merges, so the issue is what carries it and the link is what proves it exists rather than being intended. This is for work the change did not create: an adjacent defect the reviewer noticed in passing, or a fix too large to ride along. It does not cover a defect in the change under review, because filing an issue about a bug you are about to merge is outcome 3 in other clothes, and that one is the maintainer's to decide. -5. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. - -### Triaging Review Comments - -**A low-confidence finding is not a low-value one.** Copilot collapses the findings it is least sure of into the review body instead of raising a thread, and in this fleet's experience those are right the large majority of the time. Judge each one against the code, never against its confidence label. They are also the easiest to lose, because they appear in no thread, so a loop that polls threads alone reports a clean pass while they stand (see the Merge Gate, condition 3). - -For each comment, classify before responding: - -- **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. -- **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: - - The cited rule matches what the existing codebase already does -> fix the offending code. - - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 5 above), rather than counting rounds until some threshold licenses it. -- **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation, and don't apply it unilaterally. - -### Responding and Resolution Expectations - -Reply inline with either the fixing commit SHA (for accepted issues) or the evidence that disproves it (for declines). **A decline carries proof rather than an assertion**, meaning the command and its output, the code path that makes the concern impossible, or the rule that governs it. "This is fine" is not a reply, and disagreeing without evidence is not addressing a finding, so a thread is not resolved on one. Resolve review threads when addressed, or when declined with that evidence recorded in the thread. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action, so acknowledge with a reply if needed and move on. - -After the final push on a PR, sweep older threads from earlier rounds whose code paths no longer exist, or stale unresolved markers remain in the review UI. - -**Answering a suppressed finding is a different act from replying in a thread, and it carries its own pairing.** A threaded reply sits under the comment it answers and the UI shows whether it is resolved. A suppressed finding has neither, so an answer that does not carry its own context is unverifiable: the maintainer cannot tell that it was seen, which finding it addresses, or whether any were skipped, and has to ask. An answer therefore **quotes the finding** in a blockquote, with its `file:line` anchor and enough of Copilot's own words to identify it, **carries one bold verdict per finding** (`Fixed in `, `Disproven`, or `No change needed`) so the outcomes are scannable without reading prose, **states the `(N)` count** the block heading gives so N answers can be checked against N findings, and **links the review** that raised them, since a PR accumulates rounds and an unlinked answer is ambiguous about which one it closes. One comment per review round keeps the answers together. - -**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. The hub's `scripts/pr_review.py status --repo /` reports every round and marks which are from earlier ones, and it names the repository because a pull request number resolves in every repository and a digest of the wrong one is well-formed. That script is hub-hosted rather than carried, so no repository holds a copy to run and it is invoked from a hub checkout per "Hub-Hosted Tooling" above. - -**The review's own overview cannot be trusted to say whether findings exist.** A body that reads "Copilot reviewed N out of N changed files and generated no new comments" routinely carries a collapsed block of suppressed findings directly beneath that sentence. Read the body for the block rather than the summary line, because the summary line and `reviewDecision` and an empty unresolved-thread list all agree that a review with four outstanding findings is clean. - -### Escalating to the User - -Bring the user in when: - -- **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. -- **A recurring finding** the code keeps attracting, which is the fix-the-class signal. Summarize the pattern and bring the remedy, whether that is the rule change or what the code has to say differently to stop earning it. -- **A finding you judge real but do not want fixed**, which is outcome 3 above and is never the agent's call to make quietly. -- **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation, and never apply it unilaterally. - -Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. +The provider-specific mechanics this contract needs to actually drive GitHub Copilot, how to request a review, poll for it, match the suppressed-findings heading, verify coverage, and reply or resolve a thread without a hand-typed id, live in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook", unaffected by this pointer. That content is read directly by the Copilot reviewer bot, which cannot read a Skill, so it stays fully written out rather than being packaged the same way. ## Communicating with the User @@ -359,7 +195,7 @@ CI runs the full lint set, but run the linters locally before pushing to catch i **Each surface runs the lint with the tool that fits it, all from the same config files** (`.markdownlint-cli2.jsonc`, `cspell.json`, `.editorconfig`): -- **CI (authoritative)** runs **markdownlint-cli2**, **cspell**, and **actionlint** as pinned action wrappers (Dependabot bumps them), plus **editorconfig-checker** via Docker `:latest` (its action only installs the CLI, so the Docker one-liner is what actually runs the check). markdownlint covers all `**/*.md`, and **cspell is scoped to `README.md` + `HISTORY.md`** (see [CODESTYLE.md](./CODESTYLE.md) "Markdown and Spelling" for why), matching the cspell one-liner below. +- **CI (authoritative)** runs **markdownlint-cli2**, **cspell**, and **actionlint** as pinned action wrappers (Dependabot bumps them), plus **editorconfig-checker** and **PSScriptAnalyzer** via Docker `:latest` (editorconfig-checker's action only installs the CLI, and PSScriptAnalyzer has no action, so the Docker one-liner is what actually runs each check). markdownlint covers all `**/*.md`, and **cspell is scoped to `README.md` + `HISTORY.md`** (see [CODESTYLE.md](./CODESTYLE.md) "Markdown and Spelling" for why), matching the cspell one-liner below. - **The `.husky/pre-commit` hook** runs **language formatting** and the **diff-scoped doc gates**, never Docker and never a network call, so it stays fast. The formatting half is whatever the repo's own language needs, CSharpier and `dotnet format` for .NET or ruff for Python, via native tooling. A repo adds each half once its tree passes that half, since a gate that fails on the corpus it guards blocks every commit from the moment it lands, so a hook running one half is a repo mid-convergence rather than a repo out of conformance. The doc half runs each gate at the scope that fits it. The prose gate is scoped to what the commit changes rather than swept over the tree, which is the difference between about 2.2 seconds and about 0.13 and is what makes it affordable in a hook at all. A whole-repo check belongs there too when it is already fast and takes no file list, which the line-ending consistency check is, so scope is a property of the gate rather than a rule the hook applies to all of them. `repo_gate.py --check sha-pin` stays out, since it resolves a same-owner pin against the GitHub API and a hook that needs a network fails offline. A repo enables the hook per clone with `git config core.hooksPath .husky`, and CI remains the authoritative run either way. - **The VS Code Lint tasks** run the full doc-lint set via Docker `:latest` on demand, the local surface for Markdown, spelling, workflow, and EditorConfig checks. @@ -391,6 +227,28 @@ The Docker invocations below are the same ones the VS Code tasks use, for ad-hoc docker run --rm --pull=always -v "$PWD":/workdir --workdir /workdir ghcr.io/streetsidesoftware/cspell:latest --no-progress README.md HISTORY.md ``` +- **PSScriptAnalyzer** (PowerShell, the peer of the shellcheck step, with the excluded rules and their reasons in [`PSScriptAnalyzerSettings.psd1`](./PSScriptAnalyzerSettings.psd1)): + + ```sh + docker run --rm --pull=always -e PS_SCRIPTS="$(git ls-files '*.ps1')" -v "$PWD":/mnt --workdir /mnt mcr.microsoft.com/powershell:latest \ + pwsh -NoProfile -Command ' + Set-PSRepository PSGallery -InstallationPolicy Trusted + Install-Module PSScriptAnalyzer -RequiredVersion 1.23.0 -Force -Scope AllUsers + Import-Module PSScriptAnalyzer + $files = $env:PS_SCRIPTS -split "\s+" | Where-Object { $_ } + if (-not $files) { Write-Host "no PowerShell scripts are tracked"; exit 0 } + $found = @() + foreach ($file in $files) { $found += Invoke-ScriptAnalyzer -Path $file -Settings ./PSScriptAnalyzerSettings.psd1 } + Write-Host "Checked $($files.Count) file(s)" + if ($found) { $found | Format-Table RuleName,Severity,ScriptName,Line,Message -AutoSize | Out-String -Width 200 | Write-Host; exit 1 } + Write-Host "no findings" + ' + ``` + + The module version is pinned beside the image, because the image alone does not fix it and a floating install makes a local run a different check from CI. 1.23.0 rather than the newest, since 1.24.0 needs a newer `System.Management.Automation` than the image carries and fails to import after installing cleanly. The file list comes from `git ls-files` for the same reason the shellcheck step uses it, and the count is printed because a run that read no files reports the same clean as one that read them all. + + **The list splits on whitespace rather than on a newline, and the regex is double-quoted.** A shell joins the file list with newlines and PowerShell joins it with spaces, so a newline-only split hands the analyzer one path holding every file, which it reports as one file it cannot find followed by a clean run over nothing. The double quotes are what let the whole invocation stay inside the single-quoted `-Command` a shell passes, since PowerShell escapes with a backtick and leaves the backslash alone. Run verbatim it reports `Checked 5 file(s)` from either shell. + In a configured editor the davidanson extension is enough. Use the Docker CLI when there's no IDE (agent/headless) or to confirm a clean run before pushing. When pulling a public image fails on a Docker-Desktop/WSL credential-helper error (`docker-credential-desktop.exe: exec format error`), retry with an empty Docker config: `DOCKER_CONFIG=$(mktemp -d) docker run ...` after writing `{}` to `$DOCKER_CONFIG/config.json`.