diff --git a/.agents/skills/backlog-burndown/SKILL.md b/.agents/skills/backlog-burndown/SKILL.md index 6af2eca2d..f56dc4e76 100644 --- a/.agents/skills/backlog-burndown/SKILL.md +++ b/.agents/skills/backlog-burndown/SKILL.md @@ -170,9 +170,11 @@ for, and it binds harder than any throughput target. invisible to the round that has to respect it, and recording it on the issue is what makes the next bullet a read of durable state rather than of the orchestrator's memory. - **Verify the prediction before dispatching**, against everything in flight, which is wider than - this round: the files changed by every open **feature** pull request on this repository (`gh pr - diff --name-only` per open pull request), and the claim comments of every group still - holding a branch, parked groups from earlier rounds included. `git worktree list` reports the + this round: the files changed by every open **feature** pull request on this repository, and the + claim comments of every group still holding a branch, parked groups from earlier rounds included. + `gh pr list --state open --limit 1000 --json number,baseRefName,headRefName,isCrossRepository --jq '.[] | select(.headRefName != "develop" or .baseRefName != "main" or .isCrossRepository) | .number'` + lists the open pull requests, its filter applying the promotion exclusion below. + `gh pr diff --name-only` then reads each one's files. `git worktree list` reports the registered worktrees and the branch checked out in each, which is not the same as every branch that exists, so pair it with `git branch -r`, after `git fetch --prune origin`, for one that was pushed and whose worktree is already gone, and with `git branch` for one that was never pushed diff --git a/.agents/skills/check-this-repo/SKILL.md b/.agents/skills/check-this-repo/SKILL.md index d34f3bb97..5e1762e47 100644 --- a/.agents/skills/check-this-repo/SKILL.md +++ b/.agents/skills/check-this-repo/SKILL.md @@ -35,7 +35,9 @@ repo. content in the first place, and no amount of re-reading `GOVERNANCE.md` fixes that. For a Claude Code session, read `live` as well, since that channel loads the registered directory in place rather than the copy. A registered directory that is missing is an answer there whatever - the exit code says. + the exit code says. So is a `live.plugin` reading `installed: false` or `enabled: false`, which + describes the user-scope install the installer makes, as Claude Code reports it for the + directory the report runs in, and a project-level install does not count. A null there means the plugin listing could not be read. - Where `live` carries `vcs: archive`, the channel serves the tree the hub's `host-setup/bootstrap.sh` or `bootstrap.ps1` keeps, which has no git, so `branch: null` there is not a detached checkout. A `live.commit` behind the hub's `main` is the answer, and so is @@ -69,13 +71,13 @@ per the hub's `RESYNC.md` by this repo's own session or by `resync-a-repo` from ## Refresh cadence -Re-run the installer from a hub checkout on a freshly fetched `main` when `--report` exits -non-zero, and after any promotion to `main` that touches `.agents/skills/`. A copy taken from -`develop` reads not current by design, since the snapshot is judged against the promoted -revision. Session entry runs no automatic check, by design: the trigger is suspicion, -and the restated-rule symptom below is the loudest form of it. `docs/host-setup.md` -"Fleet Skills Install" in the hub states the same cadence for the host side, and an automated -refresh stays out of scope until the fleet has evidence the manual cadence fails. +This skill refreshes only when `--report` exits non-zero, and only with the `--snapshot-only` run +above, from its own freshly fetched `main` checkout. A copy taken from `develop` reads not current +by design, since the snapshot is judged against the promoted revision. Session entry runs no +automatic check, by design. The trigger is suspicion, and the restated-rule symptom this skill +triggers on is the loudest form of it. The refresh from the maintainer's own long-lived hub +checkout is a different run, and `docs/host-setup.md` "Fleet Skills Install" in the hub states its +cadence. ## What it escalates instead of touching diff --git a/.agents/skills/comment-and-doc-style/SKILL.md b/.agents/skills/comment-and-doc-style/SKILL.md index 6c597e70e..5f29c390e 100644 --- a/.agents/skills/comment-and-doc-style/SKILL.md +++ b/.agents/skills/comment-and-doc-style/SKILL.md @@ -322,7 +322,8 @@ silent pass, a letter of a recorded name excepted per the bullet below. before using it. A letter of a recorded name is the one exception, per the bullet above. - **No semicolon in agent-authored prose.** Recast a mid-sentence semicolon as a comma or as two sentences. A semicolon separating items in a list that already contains commas, or a statement - terminator in code, is unaffected. + terminator in code, is unaffected. A single semicolon separates such a list only between two + labeled items, as in `Inputs: a, b; outputs: c`, and a list item's bold opening label is not one. - **No spaced hyphen joining or interrupting a sentence** (` - `, or the paired aside ` - x - `). Recast as a comma, two sentences, or parentheses. A hyphen inside a compound word, a leading list marker, a range, and the `- **Label** - explanation` bullet separator are unaffected. diff --git a/.agents/skills/comment-and-doc-style/references/line-endings.md b/.agents/skills/comment-and-doc-style/references/line-endings.md index 7726adede..ec32e16a0 100644 --- a/.agents/skills/comment-and-doc-style/references/line-endings.md +++ b/.agents/skills/comment-and-doc-style/references/line-endings.md @@ -113,8 +113,10 @@ JSONC (it has `//` comments), so strip them before JSON-parsing it. Editing CRLF files programmatically with a regex has a sharper trap: `.` matches `\r`, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode -rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits -(`splitlines(keepends=True)`) or literal replacement over regex reassembly. In Python the -text-mode failure is the default: `Path.read_text()` decodes through universal newlines and -`write_text()` writes `\n` back, so a read-edit-write round trip flattens the whole file while the -edit itself looks correct. Pass `newline=''` to both, or work in bytes. +rewrite has the mirror failure, silently rewriting every line ending to the host's own. Prefer +line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. In +Python the text-mode failure is the default. `Path.read_text()` decodes through universal +newlines, and `write_text()` translates each `\n` to `os.linesep`. A read-edit-write round trip +therefore rewrites every ending in the file while the edit itself looks correct. Work in bytes, or +use `open()` with `newline=''` on both the read and the write. `Path.read_text()` is no substitute, +since it accepts `newline` only on Python 3.13 and newer and raises `TypeError` below it. diff --git a/.agents/skills/python-codestyle/SKILL.md b/.agents/skills/python-codestyle/SKILL.md index 00ab2f7c7..3d13e248a 100644 --- a/.agents/skills/python-codestyle/SKILL.md +++ b/.agents/skills/python-codestyle/SKILL.md @@ -33,7 +33,8 @@ Then read the `pyproject.toml` shape and pick the profile before running Python - **build** (Project): third-party runtime dependencies, or the repo's deliverable. Either a uv project (`[project]` + `[build-system]` + committed `uv.lock`, run with `uv run`) or a `pyproject.toml` beside a `requirements*.txt` and no `uv.lock`, installed with pip. Uses pytest, - and pyright strict, mypy with its strict flags, or both as the CI type checker. + and pyright strict or mypy with its strict flags as the CI type checker. A repo enforcing + pyright beside mypy runs pyright itself, per Toolchain below. - **lint-only** (Scripts): no `[project]`, no `[build-system]`, no lockfile, no `requirements*.txt` (the hub validator runs pytest wherever one sits). Uses `uvx` for third-party tools, unittest for tests, and mypy as the CI gate. Do not run pytest or diagnose its absence as an environment @@ -76,7 +77,10 @@ than one checker is normal when each serves a purpose (the .NET side pairs CShar `mypy --strict` because the platinum `strict-typing` quality-scale tier requires it, and a pydantic-heavy library may opt in for the plugin. When a repo uses mypy it runs in CI and the editor (the `ms-python.mypy-type-checker` extension) so the two stay consistent, and its mypy -command joins the clean-compile. mypy may also be a build repo's only CI checker, run with its +command joins the clean-compile. The hub validator runs at most one checker in each Python +directory, mypy where both are configured. A repo enforcing pyright beside mypy runs pyright from +its own `.github/actions/validate/action.yml` hook. That hook starts from a bare checkout, so it +sets up its own Python environment. mypy may also be a build repo's only CI checker, run with its strict flags, and Pylance's pyright diagnostics are then advisory, since CI never runs them. A pyright-only repo is the lightest and is inherently consistent, since the editor and CI run one engine. @@ -101,9 +105,13 @@ uv build # produce wheel + sdist in ./dist (published pa ``` The **build**-profile Python clean-compile, in its uv form, is `uv run ruff format` + -`uv run ruff check` + the repo's type checker: `uv run pyright`, or `uv run mypy src` where mypy is -the CI checker, or both where the repo runs both (see Type checking above). Run it, plus -`uv run pytest`, before committing. +`uv run ruff check` + the repo's type checker. That checker is `uv run pyright`, or `uv run mypy` +where mypy is the CI checker, or both where the repo runs both (see Type checking above). Where the +config sits in the project directory, CI passes the checker no path, so the config's own target +settings decide what is checked. mypy left with no target that way exits with an error. A declared +subdirectory with no config of its own uses the repository root's instead. CI then runs the checker +from the root with the directory as its path, `uv run --project `, so run it +the same way. Run the clean-compile, plus `uv run pytest`, before committing. A **build**-profile directory in its pip form builds its environment the way CI does, through uv's pip interface, which writes no `uv.lock`. It installs every `requirements*.txt` in one command, since @@ -125,8 +133,12 @@ uvx ruff@latest format --check # verify format clean Its type checker runs against that environment. mypy runs as `.venv/bin/python -m mypy` where the environment installs it, and otherwise as `uvx mypy@latest --python-executable .venv/bin/python`, adding `--python-version` with the environment's version unless the mypy config pins one. pyright -runs as `uvx pyright@latest --pythonpath .venv/bin/python`. On Windows the environment's interpreter -is `.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's +runs as `uvx pyright@latest --pythonpath .venv/bin/python`. Where the config sits in the project +directory, CI runs the checker there with no path, as in the uv form. A declared subdirectory with +no config of its own uses the repository root's instead. CI then runs the checker from the root +with the directory as its path and the interpreter as `/.venv/bin/python`. Run it the same +way, as in `/.venv/bin/python -m mypy `. On Windows each interpreter path ends in +`.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's clean-compile, run with pytest before committing. A **lint-only** profile's clean-compile substitutes its `uvx` and `unittest` equivalents, per Two @@ -225,10 +237,10 @@ Before pushing or opening a PR: - VS Code's Problems pane should be quiet for the files you touched. The relevant linters are ruff (via the `charliermarsh.ruff` extension) and pyright (via the `ms-python.python` extension's bundled Pylance). -- The **build**-profile CI gate, in its uv form, is `uv run ruff check`, - `uv run ruff format --check`, the repo's type checker (`uv run pyright` or `uv run mypy src`), and - `uv run pytest`, the same commands as the local loop above, run from the Python project directory - (invoked as separate steps, not `&&`-chained, so the runner shell is irrelevant). The pip form's +- The **build**-profile CI gate, in its uv form, runs the local loop's commands above, each from + where that loop runs it. Those are `uv run ruff check`, `uv run ruff format --check`, the repo's + type checker, and `uv run pytest`. CI invokes them as separate steps, not `&&`-chained, so the + runner shell is irrelevant. The pip form's CI gate is its commands in the local loop above, and a **lint-only** profile's is its `uvx` equivalents plus its `unittest` suite, each per `references/profiles.md`. The local loop names what CI relaxes for an undeclared root. diff --git a/.agents/skills/python-codestyle/references/profiles.md b/.agents/skills/python-codestyle/references/profiles.md index 092a9bd25..4151386b9 100644 --- a/.agents/skills/python-codestyle/references/profiles.md +++ b/.agents/skills/python-codestyle/references/profiles.md @@ -9,7 +9,8 @@ than copying verbatim (a verbatim copy that misdescribes the repo is inaccurate in review). The axes that commonly vary per repo: - **Type checker in CI**: pyright strict, mypy with its strict flags (run in CI and the editor, with - pyright kept editor-only through Pylance), or both. The clean-compile runs every checker CI runs. + pyright kept editor-only through Pylance), or both. A repo running both runs pyright itself, + per `SKILL.md` "Toolchain". The clean-compile runs every checker CI runs. - **Dependency declaration**: the uv form declares the dev tools CI runs in the `dev` group of `[dependency-groups]`, the one group a plain local `uv sync` or `uv run` installs, and which CI's `uv sync --all-groups --frozen` installs too, since it takes every group and no extra. PEP 621 diff --git a/.agents/skills/shell-codestyle/SKILL.md b/.agents/skills/shell-codestyle/SKILL.md index 5f5894a19..1708f1dfc 100644 --- a/.agents/skills/shell-codestyle/SKILL.md +++ b/.agents/skills/shell-codestyle/SKILL.md @@ -70,8 +70,8 @@ depend on Python either. Everything else is Python, with a test under its own sc - **`shellcheck` clean, and a deliberate exception carries its reason inline.** A `# shellcheck disable=SCxxxx` names why the rule does not apply here, so the next reader can tell a considered exception from an unread warning. The hub's own `repo-config/configure.sh` is - the worked example, carrying `SC2016` disables where a single-quoted `jq` program must stay - unexpanded, each with its reason on the same line. + the worked example. Its `SC2016` disables sit only where shellcheck flags a single-quoted `jq` + program or GraphQL query that must stay unexpanded. Each carries its reason on the same line. - **Comments say why, never what.** The code states what it does. A comment restating it goes stale silently, where a comment carrying a reason fails visibly when the reason stops being true. diff --git a/.agents/skills/skill-lifecycle/SKILL.md b/.agents/skills/skill-lifecycle/SKILL.md index 79f5c590b..3a43e700c 100644 --- a/.agents/skills/skill-lifecycle/SKILL.md +++ b/.agents/skills/skill-lifecycle/SKILL.md @@ -32,7 +32,7 @@ A skill surfaces at a trigger moment. A rule that binds every action all the tim 5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. 6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then, once authorized, commit the source and both generated trees in one commit, per `git-commit-conventions`. CI runs `--check` on every pull request and fails a desynced distribution. `python3 tests/test_build_dist.py` covers the generator itself. 7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. -8. **Refresh the machines after promotion**: re-run `python3 scripts/skills_install.py --snapshot-only` per machine, from a freshly fetched `main`, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. +8. **Refresh the machines after promotion**, per machine, as `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. ## Changing or Retiring a Skill diff --git a/.agents/skills/workflow-ci-contract/references/architecture.md b/.agents/skills/workflow-ci-contract/references/architecture.md index 384dca25e..a33ab8239 100644 --- a/.agents/skills/workflow-ci-contract/references/architecture.md +++ b/.agents/skills/workflow-ci-contract/references/architecture.md @@ -28,7 +28,7 @@ flowchart LR The direct commit is an **allowance, not a substitute for review**. The ruleset drops the pull-request *requirement*, which permits a direct push without withdrawing the pull request, so a change worth reviewing still takes one and both paths reach `develop` legally. Which changes those are is stated as a shape rather than a line count in `GOVERNANCE.md` "Operational Repositories", which owns the test and is the one place it is written, since nothing in a ruleset can apply it. What differs is when validation lands. On the direct-commit path the commit is already on the branch, so CI can only be advisory after the fact, and that is the accepted cost of the model. On the pull-request path the change has not landed, so validation is pre-merge and actionable, which is the moment it is worth the most, and the lint workflow's `pull_request` trigger therefore names `develop` alongside `main` (`WORKFLOW.md` section 6). That is what makes **D1.2** hold here, since its input is *any* PR and the operational model is no exception. The check is reported on a `develop` PR rather than required, because a required status check on `develop` binds the direct push too and would dissolve the allowance the model is built on. -Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`; tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. +Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`, tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. ### Two Layers: Orchestration vs Build diff --git a/.agents/skills/workflow-ci-contract/references/d-guarantees.md b/.agents/skills/workflow-ci-contract/references/d-guarantees.md index 30a96a29f..d4c935b9b 100644 --- a/.agents/skills/workflow-ci-contract/references/d-guarantees.md +++ b/.agents/skills/workflow-ci-contract/references/d-guarantees.md @@ -50,7 +50,7 @@ The required behaviors, organized by domain. Each is a **MUST**, and its `Output - **D5.3 Best-effort.** Output: cleanup is `continue-on-error`, tolerates a failed listing, and deletes **all** matching ids. *Prevents: a cleanup hiccup reddening a job whose publish succeeded.* - **D5.4 Retention backstop.** Output: **every** `upload-artifact` sets `retention-days: 1`. - **D5.5 Never blanket-delete.** Output: cleanup MUST NOT enumerate and delete the run's whole artifact set. *Prevents: destroying diagnostic/log artifacts and auto-emitted build-records.* -- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy; and the split-ownership version of the same, where each side assumes the other prunes.* +- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy. It also prevents the split-ownership version of the same, where each side assumes the other prunes.* ### D6 - Seam / Architecture Conformance diff --git a/.agents/skills/workflow-ci-contract/references/test-methodology.md b/.agents/skills/workflow-ci-contract/references/test-methodology.md index dcbe64cc7..c9c86eca8 100644 --- a/.agents/skills/workflow-ci-contract/references/test-methodology.md +++ b/.agents/skills/workflow-ci-contract/references/test-methodology.md @@ -35,7 +35,7 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | S11 | scheduled upstream-version bump (wrapper) | resolver detects a change -> commits the state file -> opens a per-branch bump PR -> the merge-bot auto-merges it, or leaves it for the maintainer where the tracker sets `auto-merge: false` (D8.3) -> the `main` pin publishes via the gate (a develop pin does not auto-publish, shipping instead via a develop dispatch or promotion) | D8.3, D3.5 | | S12 | deploy dispatch naming an environment | the ref gate runs **first** (production from the default branch only, any ref to a non-production environment); validation runs; the callee re-asserts the environment name; a release installs under its own id; the pointer flips as a separate step; retention is bounded by whichever of the two D5.6 shapes the repo uses, so a deploy whose credential can observe the destination asserts the count converged and one confined write-only leaves it to the host; the live check asserts the environment and the release id, waiting out the reload, then the URL contract; **no tag and no release are created** | D2.1, D4.6, D5.6 | | S13 | deploy dispatch of a production environment from a non-default ref | **fails fast**, before anything is installed or written | D2.1 | -| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch; any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | +| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch. Any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | ### 5C. Live Probe (Where Warranted) diff --git a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown index 57e5b1905..22e93b420 100644 --- a/.claude-plugin/fleet-skills/.source-digests/backlog-burndown +++ b/.claude-plugin/fleet-skills/.source-digests/backlog-burndown @@ -1 +1 @@ -f9045d01bca0c44a +37058389d54e899a diff --git a/.claude-plugin/fleet-skills/.source-digests/check-this-repo b/.claude-plugin/fleet-skills/.source-digests/check-this-repo index f0a3d22de..fb8682363 100644 --- a/.claude-plugin/fleet-skills/.source-digests/check-this-repo +++ b/.claude-plugin/fleet-skills/.source-digests/check-this-repo @@ -1 +1 @@ -da0a6b105966e75b +74d2afd5c47b7d7c diff --git a/.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style b/.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style index 6e093d6ee..93806e89d 100644 --- a/.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style +++ b/.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style @@ -1 +1 @@ -db8fb1a840099988 +bf9626afca871416 diff --git a/.claude-plugin/fleet-skills/.source-digests/python-codestyle b/.claude-plugin/fleet-skills/.source-digests/python-codestyle index 0fc961d55..66bd085bd 100644 --- a/.claude-plugin/fleet-skills/.source-digests/python-codestyle +++ b/.claude-plugin/fleet-skills/.source-digests/python-codestyle @@ -1 +1 @@ -29f1d243d1548ec9 +4a55b75c694378d2 diff --git a/.claude-plugin/fleet-skills/.source-digests/shell-codestyle b/.claude-plugin/fleet-skills/.source-digests/shell-codestyle index a0e38df3b..188cb067b 100644 --- a/.claude-plugin/fleet-skills/.source-digests/shell-codestyle +++ b/.claude-plugin/fleet-skills/.source-digests/shell-codestyle @@ -1 +1 @@ -89729bb8dbd2a730 +52454c88dd2aedc0 diff --git a/.claude-plugin/fleet-skills/.source-digests/skill-lifecycle b/.claude-plugin/fleet-skills/.source-digests/skill-lifecycle index 1150444f9..f63d3e74e 100644 --- a/.claude-plugin/fleet-skills/.source-digests/skill-lifecycle +++ b/.claude-plugin/fleet-skills/.source-digests/skill-lifecycle @@ -1 +1 @@ -22bb56c8d9fe6fec +cd59a02058f84185 diff --git a/.claude-plugin/fleet-skills/.source-digests/workflow-ci-contract b/.claude-plugin/fleet-skills/.source-digests/workflow-ci-contract index fd494cebf..b5cdf548f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/workflow-ci-contract +++ b/.claude-plugin/fleet-skills/.source-digests/workflow-ci-contract @@ -1 +1 @@ -d66ade3710520c4f +f7b85460ab9fa398 diff --git a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md index 6af2eca2d..f56dc4e76 100644 --- a/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/backlog-burndown/SKILL.md @@ -170,9 +170,11 @@ for, and it binds harder than any throughput target. invisible to the round that has to respect it, and recording it on the issue is what makes the next bullet a read of durable state rather than of the orchestrator's memory. - **Verify the prediction before dispatching**, against everything in flight, which is wider than - this round: the files changed by every open **feature** pull request on this repository (`gh pr - diff --name-only` per open pull request), and the claim comments of every group still - holding a branch, parked groups from earlier rounds included. `git worktree list` reports the + this round: the files changed by every open **feature** pull request on this repository, and the + claim comments of every group still holding a branch, parked groups from earlier rounds included. + `gh pr list --state open --limit 1000 --json number,baseRefName,headRefName,isCrossRepository --jq '.[] | select(.headRefName != "develop" or .baseRefName != "main" or .isCrossRepository) | .number'` + lists the open pull requests, its filter applying the promotion exclusion below. + `gh pr diff --name-only` then reads each one's files. `git worktree list` reports the registered worktrees and the branch checked out in each, which is not the same as every branch that exists, so pair it with `git branch -r`, after `git fetch --prune origin`, for one that was pushed and whose worktree is already gone, and with `git branch` for one that was never pushed diff --git a/.claude-plugin/fleet-skills/skills/check-this-repo/SKILL.md b/.claude-plugin/fleet-skills/skills/check-this-repo/SKILL.md index d34f3bb97..5e1762e47 100644 --- a/.claude-plugin/fleet-skills/skills/check-this-repo/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/check-this-repo/SKILL.md @@ -35,7 +35,9 @@ repo. content in the first place, and no amount of re-reading `GOVERNANCE.md` fixes that. For a Claude Code session, read `live` as well, since that channel loads the registered directory in place rather than the copy. A registered directory that is missing is an answer there whatever - the exit code says. + the exit code says. So is a `live.plugin` reading `installed: false` or `enabled: false`, which + describes the user-scope install the installer makes, as Claude Code reports it for the + directory the report runs in, and a project-level install does not count. A null there means the plugin listing could not be read. - Where `live` carries `vcs: archive`, the channel serves the tree the hub's `host-setup/bootstrap.sh` or `bootstrap.ps1` keeps, which has no git, so `branch: null` there is not a detached checkout. A `live.commit` behind the hub's `main` is the answer, and so is @@ -69,13 +71,13 @@ per the hub's `RESYNC.md` by this repo's own session or by `resync-a-repo` from ## Refresh cadence -Re-run the installer from a hub checkout on a freshly fetched `main` when `--report` exits -non-zero, and after any promotion to `main` that touches `.agents/skills/`. A copy taken from -`develop` reads not current by design, since the snapshot is judged against the promoted -revision. Session entry runs no automatic check, by design: the trigger is suspicion, -and the restated-rule symptom below is the loudest form of it. `docs/host-setup.md` -"Fleet Skills Install" in the hub states the same cadence for the host side, and an automated -refresh stays out of scope until the fleet has evidence the manual cadence fails. +This skill refreshes only when `--report` exits non-zero, and only with the `--snapshot-only` run +above, from its own freshly fetched `main` checkout. A copy taken from `develop` reads not current +by design, since the snapshot is judged against the promoted revision. Session entry runs no +automatic check, by design. The trigger is suspicion, and the restated-rule symptom this skill +triggers on is the loudest form of it. The refresh from the maintainer's own long-lived hub +checkout is a different run, and `docs/host-setup.md` "Fleet Skills Install" in the hub states its +cadence. ## What it escalates instead of touching diff --git a/.claude-plugin/fleet-skills/skills/comment-and-doc-style/SKILL.md b/.claude-plugin/fleet-skills/skills/comment-and-doc-style/SKILL.md index 6c597e70e..5f29c390e 100644 --- a/.claude-plugin/fleet-skills/skills/comment-and-doc-style/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/comment-and-doc-style/SKILL.md @@ -322,7 +322,8 @@ silent pass, a letter of a recorded name excepted per the bullet below. before using it. A letter of a recorded name is the one exception, per the bullet above. - **No semicolon in agent-authored prose.** Recast a mid-sentence semicolon as a comma or as two sentences. A semicolon separating items in a list that already contains commas, or a statement - terminator in code, is unaffected. + terminator in code, is unaffected. A single semicolon separates such a list only between two + labeled items, as in `Inputs: a, b; outputs: c`, and a list item's bold opening label is not one. - **No spaced hyphen joining or interrupting a sentence** (` - `, or the paired aside ` - x - `). Recast as a comma, two sentences, or parentheses. A hyphen inside a compound word, a leading list marker, a range, and the `- **Label** - explanation` bullet separator are unaffected. diff --git a/.claude-plugin/fleet-skills/skills/comment-and-doc-style/references/line-endings.md b/.claude-plugin/fleet-skills/skills/comment-and-doc-style/references/line-endings.md index 7726adede..ec32e16a0 100644 --- a/.claude-plugin/fleet-skills/skills/comment-and-doc-style/references/line-endings.md +++ b/.claude-plugin/fleet-skills/skills/comment-and-doc-style/references/line-endings.md @@ -113,8 +113,10 @@ JSONC (it has `//` comments), so strip them before JSON-parsing it. Editing CRLF files programmatically with a regex has a sharper trap: `.` matches `\r`, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode -rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits -(`splitlines(keepends=True)`) or literal replacement over regex reassembly. In Python the -text-mode failure is the default: `Path.read_text()` decodes through universal newlines and -`write_text()` writes `\n` back, so a read-edit-write round trip flattens the whole file while the -edit itself looks correct. Pass `newline=''` to both, or work in bytes. +rewrite has the mirror failure, silently rewriting every line ending to the host's own. Prefer +line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. In +Python the text-mode failure is the default. `Path.read_text()` decodes through universal +newlines, and `write_text()` translates each `\n` to `os.linesep`. A read-edit-write round trip +therefore rewrites every ending in the file while the edit itself looks correct. Work in bytes, or +use `open()` with `newline=''` on both the read and the write. `Path.read_text()` is no substitute, +since it accepts `newline` only on Python 3.13 and newer and raises `TypeError` below it. diff --git a/.claude-plugin/fleet-skills/skills/python-codestyle/SKILL.md b/.claude-plugin/fleet-skills/skills/python-codestyle/SKILL.md index 00ab2f7c7..3d13e248a 100644 --- a/.claude-plugin/fleet-skills/skills/python-codestyle/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/python-codestyle/SKILL.md @@ -33,7 +33,8 @@ Then read the `pyproject.toml` shape and pick the profile before running Python - **build** (Project): third-party runtime dependencies, or the repo's deliverable. Either a uv project (`[project]` + `[build-system]` + committed `uv.lock`, run with `uv run`) or a `pyproject.toml` beside a `requirements*.txt` and no `uv.lock`, installed with pip. Uses pytest, - and pyright strict, mypy with its strict flags, or both as the CI type checker. + and pyright strict or mypy with its strict flags as the CI type checker. A repo enforcing + pyright beside mypy runs pyright itself, per Toolchain below. - **lint-only** (Scripts): no `[project]`, no `[build-system]`, no lockfile, no `requirements*.txt` (the hub validator runs pytest wherever one sits). Uses `uvx` for third-party tools, unittest for tests, and mypy as the CI gate. Do not run pytest or diagnose its absence as an environment @@ -76,7 +77,10 @@ than one checker is normal when each serves a purpose (the .NET side pairs CShar `mypy --strict` because the platinum `strict-typing` quality-scale tier requires it, and a pydantic-heavy library may opt in for the plugin. When a repo uses mypy it runs in CI and the editor (the `ms-python.mypy-type-checker` extension) so the two stay consistent, and its mypy -command joins the clean-compile. mypy may also be a build repo's only CI checker, run with its +command joins the clean-compile. The hub validator runs at most one checker in each Python +directory, mypy where both are configured. A repo enforcing pyright beside mypy runs pyright from +its own `.github/actions/validate/action.yml` hook. That hook starts from a bare checkout, so it +sets up its own Python environment. mypy may also be a build repo's only CI checker, run with its strict flags, and Pylance's pyright diagnostics are then advisory, since CI never runs them. A pyright-only repo is the lightest and is inherently consistent, since the editor and CI run one engine. @@ -101,9 +105,13 @@ uv build # produce wheel + sdist in ./dist (published pa ``` The **build**-profile Python clean-compile, in its uv form, is `uv run ruff format` + -`uv run ruff check` + the repo's type checker: `uv run pyright`, or `uv run mypy src` where mypy is -the CI checker, or both where the repo runs both (see Type checking above). Run it, plus -`uv run pytest`, before committing. +`uv run ruff check` + the repo's type checker. That checker is `uv run pyright`, or `uv run mypy` +where mypy is the CI checker, or both where the repo runs both (see Type checking above). Where the +config sits in the project directory, CI passes the checker no path, so the config's own target +settings decide what is checked. mypy left with no target that way exits with an error. A declared +subdirectory with no config of its own uses the repository root's instead. CI then runs the checker +from the root with the directory as its path, `uv run --project `, so run it +the same way. Run the clean-compile, plus `uv run pytest`, before committing. A **build**-profile directory in its pip form builds its environment the way CI does, through uv's pip interface, which writes no `uv.lock`. It installs every `requirements*.txt` in one command, since @@ -125,8 +133,12 @@ uvx ruff@latest format --check # verify format clean Its type checker runs against that environment. mypy runs as `.venv/bin/python -m mypy` where the environment installs it, and otherwise as `uvx mypy@latest --python-executable .venv/bin/python`, adding `--python-version` with the environment's version unless the mypy config pins one. pyright -runs as `uvx pyright@latest --pythonpath .venv/bin/python`. On Windows the environment's interpreter -is `.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's +runs as `uvx pyright@latest --pythonpath .venv/bin/python`. Where the config sits in the project +directory, CI runs the checker there with no path, as in the uv form. A declared subdirectory with +no config of its own uses the repository root's instead. CI then runs the checker from the root +with the directory as its path and the interpreter as `/.venv/bin/python`. Run it the same +way, as in `/.venv/bin/python -m mypy `. On Windows each interpreter path ends in +`.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's clean-compile, run with pytest before committing. A **lint-only** profile's clean-compile substitutes its `uvx` and `unittest` equivalents, per Two @@ -225,10 +237,10 @@ Before pushing or opening a PR: - VS Code's Problems pane should be quiet for the files you touched. The relevant linters are ruff (via the `charliermarsh.ruff` extension) and pyright (via the `ms-python.python` extension's bundled Pylance). -- The **build**-profile CI gate, in its uv form, is `uv run ruff check`, - `uv run ruff format --check`, the repo's type checker (`uv run pyright` or `uv run mypy src`), and - `uv run pytest`, the same commands as the local loop above, run from the Python project directory - (invoked as separate steps, not `&&`-chained, so the runner shell is irrelevant). The pip form's +- The **build**-profile CI gate, in its uv form, runs the local loop's commands above, each from + where that loop runs it. Those are `uv run ruff check`, `uv run ruff format --check`, the repo's + type checker, and `uv run pytest`. CI invokes them as separate steps, not `&&`-chained, so the + runner shell is irrelevant. The pip form's CI gate is its commands in the local loop above, and a **lint-only** profile's is its `uvx` equivalents plus its `unittest` suite, each per `references/profiles.md`. The local loop names what CI relaxes for an undeclared root. diff --git a/.claude-plugin/fleet-skills/skills/python-codestyle/references/profiles.md b/.claude-plugin/fleet-skills/skills/python-codestyle/references/profiles.md index 092a9bd25..4151386b9 100644 --- a/.claude-plugin/fleet-skills/skills/python-codestyle/references/profiles.md +++ b/.claude-plugin/fleet-skills/skills/python-codestyle/references/profiles.md @@ -9,7 +9,8 @@ than copying verbatim (a verbatim copy that misdescribes the repo is inaccurate in review). The axes that commonly vary per repo: - **Type checker in CI**: pyright strict, mypy with its strict flags (run in CI and the editor, with - pyright kept editor-only through Pylance), or both. The clean-compile runs every checker CI runs. + pyright kept editor-only through Pylance), or both. A repo running both runs pyright itself, + per `SKILL.md` "Toolchain". The clean-compile runs every checker CI runs. - **Dependency declaration**: the uv form declares the dev tools CI runs in the `dev` group of `[dependency-groups]`, the one group a plain local `uv sync` or `uv run` installs, and which CI's `uv sync --all-groups --frozen` installs too, since it takes every group and no extra. PEP 621 diff --git a/.claude-plugin/fleet-skills/skills/shell-codestyle/SKILL.md b/.claude-plugin/fleet-skills/skills/shell-codestyle/SKILL.md index 5f5894a19..1708f1dfc 100644 --- a/.claude-plugin/fleet-skills/skills/shell-codestyle/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/shell-codestyle/SKILL.md @@ -70,8 +70,8 @@ depend on Python either. Everything else is Python, with a test under its own sc - **`shellcheck` clean, and a deliberate exception carries its reason inline.** A `# shellcheck disable=SCxxxx` names why the rule does not apply here, so the next reader can tell a considered exception from an unread warning. The hub's own `repo-config/configure.sh` is - the worked example, carrying `SC2016` disables where a single-quoted `jq` program must stay - unexpanded, each with its reason on the same line. + the worked example. Its `SC2016` disables sit only where shellcheck flags a single-quoted `jq` + program or GraphQL query that must stay unexpanded. Each carries its reason on the same line. - **Comments say why, never what.** The code states what it does. A comment restating it goes stale silently, where a comment carrying a reason fails visibly when the reason stops being true. diff --git a/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md b/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md index 79f5c590b..3a43e700c 100644 --- a/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/skill-lifecycle/SKILL.md @@ -32,7 +32,7 @@ A skill surfaces at a trigger moment. A rule that binds every action all the tim 5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. 6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then, once authorized, commit the source and both generated trees in one commit, per `git-commit-conventions`. CI runs `--check` on every pull request and fails a desynced distribution. `python3 tests/test_build_dist.py` covers the generator itself. 7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. -8. **Refresh the machines after promotion**: re-run `python3 scripts/skills_install.py --snapshot-only` per machine, from a freshly fetched `main`, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. +8. **Refresh the machines after promotion**, per machine, as `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. ## Changing or Retiring a Skill diff --git a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/architecture.md b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/architecture.md index 384dca25e..a33ab8239 100644 --- a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/architecture.md +++ b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/architecture.md @@ -28,7 +28,7 @@ flowchart LR The direct commit is an **allowance, not a substitute for review**. The ruleset drops the pull-request *requirement*, which permits a direct push without withdrawing the pull request, so a change worth reviewing still takes one and both paths reach `develop` legally. Which changes those are is stated as a shape rather than a line count in `GOVERNANCE.md` "Operational Repositories", which owns the test and is the one place it is written, since nothing in a ruleset can apply it. What differs is when validation lands. On the direct-commit path the commit is already on the branch, so CI can only be advisory after the fact, and that is the accepted cost of the model. On the pull-request path the change has not landed, so validation is pre-merge and actionable, which is the moment it is worth the most, and the lint workflow's `pull_request` trigger therefore names `develop` alongside `main` (`WORKFLOW.md` section 6). That is what makes **D1.2** hold here, since its input is *any* PR and the operational model is no exception. The check is reported on a `develop` PR rather than required, because a required status check on `develop` binds the direct push too and would dissolve the allowance the model is built on. -Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`; tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. +Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`, tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. ### Two Layers: Orchestration vs Build diff --git a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/d-guarantees.md b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/d-guarantees.md index 30a96a29f..d4c935b9b 100644 --- a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/d-guarantees.md +++ b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/d-guarantees.md @@ -50,7 +50,7 @@ The required behaviors, organized by domain. Each is a **MUST**, and its `Output - **D5.3 Best-effort.** Output: cleanup is `continue-on-error`, tolerates a failed listing, and deletes **all** matching ids. *Prevents: a cleanup hiccup reddening a job whose publish succeeded.* - **D5.4 Retention backstop.** Output: **every** `upload-artifact` sets `retention-days: 1`. - **D5.5 Never blanket-delete.** Output: cleanup MUST NOT enumerate and delete the run's whole artifact set. *Prevents: destroying diagnostic/log artifacts and auto-emitted build-records.* -- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy; and the split-ownership version of the same, where each side assumes the other prunes.* +- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy. It also prevents the split-ownership version of the same, where each side assumes the other prunes.* ### D6 - Seam / Architecture Conformance diff --git a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/test-methodology.md b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/test-methodology.md index dcbe64cc7..c9c86eca8 100644 --- a/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/test-methodology.md +++ b/.claude-plugin/fleet-skills/skills/workflow-ci-contract/references/test-methodology.md @@ -35,7 +35,7 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | S11 | scheduled upstream-version bump (wrapper) | resolver detects a change -> commits the state file -> opens a per-branch bump PR -> the merge-bot auto-merges it, or leaves it for the maintainer where the tracker sets `auto-merge: false` (D8.3) -> the `main` pin publishes via the gate (a develop pin does not auto-publish, shipping instead via a develop dispatch or promotion) | D8.3, D3.5 | | S12 | deploy dispatch naming an environment | the ref gate runs **first** (production from the default branch only, any ref to a non-production environment); validation runs; the callee re-asserts the environment name; a release installs under its own id; the pointer flips as a separate step; retention is bounded by whichever of the two D5.6 shapes the repo uses, so a deploy whose credential can observe the destination asserts the count converged and one confined write-only leaves it to the host; the live check asserts the environment and the release id, waiting out the reload, then the URL contract; **no tag and no release are created** | D2.1, D4.6, D5.6 | | S13 | deploy dispatch of a production environment from a non-default ref | **fails fast**, before anything is installed or written | D2.1 | -| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch; any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | +| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch. Any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | ### 5C. Live Probe (Where Warranted) diff --git a/.github/actions/prose-gate/prose_lint.py b/.github/actions/prose-gate/prose_lint.py index 96b387a0c..9067ca47a 100755 --- a/.github/actions/prose-gate/prose_lint.py +++ b/.github/actions/prose-gate/prose_lint.py @@ -5,7 +5,7 @@ these rules, so nothing enforced them before this script. Rules implemented: charset Non-ASCII judged against the three tiers the charset rule defines. charset-unknown Non-ASCII in no tier, bar a Latin letter and its U+0300-U+036F diacritics. - semicolon No semicolon in prose, outside a list that already carries commas. + semicolon No semicolon in prose, outside a series or labeled pair carrying a comma. dash No spaced hyphen joining or interrupting a sentence. comment-wrap One sentence per comment line, never wrapped and never two on a line. comment-case A comment sentence starts with a capital, not a lowercase word. @@ -48,7 +48,7 @@ RULES = { "charset": "a non-ASCII character its tier does not permit here", "charset-unknown": "non-ASCII in no tier, bar a Latin letter and its U+0300-U+036F diacritics", - "semicolon": "a semicolon in prose, outside a list that already carries commas", + "semicolon": "a semicolon in prose, outside a series or labeled pair carrying a comma", "dash": "a spaced hyphen joining or interrupting a sentence", "comment-wrap": "a comment sentence wrapped across lines, or two on one line", "comment-case": "a comment sentence opening in lowercase", @@ -1046,6 +1046,11 @@ def discover( # A pronoun-keyed pattern found 170 of 493 and missed every imperative splice. SEMICOLON = re.compile(r";") +# A lone semicolon separates a list only between labeled items, as in `Inputs: a, b; outputs: c`. +# A colon that explains rather than labels otherwise exempted the splice after it. +LABEL_WORD = r"[\w`]+(?:[./-][\w`]+)*" +LABELED_ITEM = re.compile(rf"^\s*[*_]*{LABEL_WORD}(?:\s+{LABEL_WORD}){{0,2}}[*_]*:[*_]*(?:\s|$)") + # A spaced hyphen, the em-dash-style clause break and the paired aside alike. # A compound word carries no spaces, and a range is digit-bounded. # A list marker has only whitespace before it, once QUOTE_PREFIX blanks a blockquote's `>`. @@ -2618,11 +2623,15 @@ def check_file( # The sentence is the unit, since the list an exemption protects lives in one. # Judged over a whole bullet, one colon exempted every semicolon after it. for sentence in sentences(span): - # A list keeps its semicolons, announced by a colon or a second separator. + # A list keeps its semicolons, announced by a second separator or a label. # The comma qualifies the list rather than one separator's position. # An enumeration whose commas fall in a later item keeps every semicolon. # Read positionally, it split one series and flagged that series' openers. - listish = sentence.count(";") > 1 or ":" in sentence.split(";")[0] + head, _, tail = sentence.partition(";") + listish = sentence.count(";") > 1 or ( + LABELED_ITEM.match(LIST_MARKER.sub("", head, count=1)) is not None + and LABELED_ITEM.match(tail) is not None + ) if listish and "," in sentence: continue for _ in SEMICOLON.finditer(sentence): diff --git a/.github/skills/backlog-burndown/SKILL.md b/.github/skills/backlog-burndown/SKILL.md index 6af2eca2d..f56dc4e76 100644 --- a/.github/skills/backlog-burndown/SKILL.md +++ b/.github/skills/backlog-burndown/SKILL.md @@ -170,9 +170,11 @@ for, and it binds harder than any throughput target. invisible to the round that has to respect it, and recording it on the issue is what makes the next bullet a read of durable state rather than of the orchestrator's memory. - **Verify the prediction before dispatching**, against everything in flight, which is wider than - this round: the files changed by every open **feature** pull request on this repository (`gh pr - diff --name-only` per open pull request), and the claim comments of every group still - holding a branch, parked groups from earlier rounds included. `git worktree list` reports the + this round: the files changed by every open **feature** pull request on this repository, and the + claim comments of every group still holding a branch, parked groups from earlier rounds included. + `gh pr list --state open --limit 1000 --json number,baseRefName,headRefName,isCrossRepository --jq '.[] | select(.headRefName != "develop" or .baseRefName != "main" or .isCrossRepository) | .number'` + lists the open pull requests, its filter applying the promotion exclusion below. + `gh pr diff --name-only` then reads each one's files. `git worktree list` reports the registered worktrees and the branch checked out in each, which is not the same as every branch that exists, so pair it with `git branch -r`, after `git fetch --prune origin`, for one that was pushed and whose worktree is already gone, and with `git branch` for one that was never pushed diff --git a/.github/skills/check-this-repo/SKILL.md b/.github/skills/check-this-repo/SKILL.md index d34f3bb97..5e1762e47 100644 --- a/.github/skills/check-this-repo/SKILL.md +++ b/.github/skills/check-this-repo/SKILL.md @@ -35,7 +35,9 @@ repo. content in the first place, and no amount of re-reading `GOVERNANCE.md` fixes that. For a Claude Code session, read `live` as well, since that channel loads the registered directory in place rather than the copy. A registered directory that is missing is an answer there whatever - the exit code says. + the exit code says. So is a `live.plugin` reading `installed: false` or `enabled: false`, which + describes the user-scope install the installer makes, as Claude Code reports it for the + directory the report runs in, and a project-level install does not count. A null there means the plugin listing could not be read. - Where `live` carries `vcs: archive`, the channel serves the tree the hub's `host-setup/bootstrap.sh` or `bootstrap.ps1` keeps, which has no git, so `branch: null` there is not a detached checkout. A `live.commit` behind the hub's `main` is the answer, and so is @@ -69,13 +71,13 @@ per the hub's `RESYNC.md` by this repo's own session or by `resync-a-repo` from ## Refresh cadence -Re-run the installer from a hub checkout on a freshly fetched `main` when `--report` exits -non-zero, and after any promotion to `main` that touches `.agents/skills/`. A copy taken from -`develop` reads not current by design, since the snapshot is judged against the promoted -revision. Session entry runs no automatic check, by design: the trigger is suspicion, -and the restated-rule symptom below is the loudest form of it. `docs/host-setup.md` -"Fleet Skills Install" in the hub states the same cadence for the host side, and an automated -refresh stays out of scope until the fleet has evidence the manual cadence fails. +This skill refreshes only when `--report` exits non-zero, and only with the `--snapshot-only` run +above, from its own freshly fetched `main` checkout. A copy taken from `develop` reads not current +by design, since the snapshot is judged against the promoted revision. Session entry runs no +automatic check, by design. The trigger is suspicion, and the restated-rule symptom this skill +triggers on is the loudest form of it. The refresh from the maintainer's own long-lived hub +checkout is a different run, and `docs/host-setup.md` "Fleet Skills Install" in the hub states its +cadence. ## What it escalates instead of touching diff --git a/.github/skills/comment-and-doc-style/SKILL.md b/.github/skills/comment-and-doc-style/SKILL.md index 6c597e70e..5f29c390e 100644 --- a/.github/skills/comment-and-doc-style/SKILL.md +++ b/.github/skills/comment-and-doc-style/SKILL.md @@ -322,7 +322,8 @@ silent pass, a letter of a recorded name excepted per the bullet below. before using it. A letter of a recorded name is the one exception, per the bullet above. - **No semicolon in agent-authored prose.** Recast a mid-sentence semicolon as a comma or as two sentences. A semicolon separating items in a list that already contains commas, or a statement - terminator in code, is unaffected. + terminator in code, is unaffected. A single semicolon separates such a list only between two + labeled items, as in `Inputs: a, b; outputs: c`, and a list item's bold opening label is not one. - **No spaced hyphen joining or interrupting a sentence** (` - `, or the paired aside ` - x - `). Recast as a comma, two sentences, or parentheses. A hyphen inside a compound word, a leading list marker, a range, and the `- **Label** - explanation` bullet separator are unaffected. diff --git a/.github/skills/comment-and-doc-style/references/line-endings.md b/.github/skills/comment-and-doc-style/references/line-endings.md index 7726adede..ec32e16a0 100644 --- a/.github/skills/comment-and-doc-style/references/line-endings.md +++ b/.github/skills/comment-and-doc-style/references/line-endings.md @@ -113,8 +113,10 @@ JSONC (it has `//` comments), so strip them before JSON-parsing it. Editing CRLF files programmatically with a regex has a sharper trap: `.` matches `\r`, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode -rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits -(`splitlines(keepends=True)`) or literal replacement over regex reassembly. In Python the -text-mode failure is the default: `Path.read_text()` decodes through universal newlines and -`write_text()` writes `\n` back, so a read-edit-write round trip flattens the whole file while the -edit itself looks correct. Pass `newline=''` to both, or work in bytes. +rewrite has the mirror failure, silently rewriting every line ending to the host's own. Prefer +line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. In +Python the text-mode failure is the default. `Path.read_text()` decodes through universal +newlines, and `write_text()` translates each `\n` to `os.linesep`. A read-edit-write round trip +therefore rewrites every ending in the file while the edit itself looks correct. Work in bytes, or +use `open()` with `newline=''` on both the read and the write. `Path.read_text()` is no substitute, +since it accepts `newline` only on Python 3.13 and newer and raises `TypeError` below it. diff --git a/.github/skills/python-codestyle/SKILL.md b/.github/skills/python-codestyle/SKILL.md index 00ab2f7c7..3d13e248a 100644 --- a/.github/skills/python-codestyle/SKILL.md +++ b/.github/skills/python-codestyle/SKILL.md @@ -33,7 +33,8 @@ Then read the `pyproject.toml` shape and pick the profile before running Python - **build** (Project): third-party runtime dependencies, or the repo's deliverable. Either a uv project (`[project]` + `[build-system]` + committed `uv.lock`, run with `uv run`) or a `pyproject.toml` beside a `requirements*.txt` and no `uv.lock`, installed with pip. Uses pytest, - and pyright strict, mypy with its strict flags, or both as the CI type checker. + and pyright strict or mypy with its strict flags as the CI type checker. A repo enforcing + pyright beside mypy runs pyright itself, per Toolchain below. - **lint-only** (Scripts): no `[project]`, no `[build-system]`, no lockfile, no `requirements*.txt` (the hub validator runs pytest wherever one sits). Uses `uvx` for third-party tools, unittest for tests, and mypy as the CI gate. Do not run pytest or diagnose its absence as an environment @@ -76,7 +77,10 @@ than one checker is normal when each serves a purpose (the .NET side pairs CShar `mypy --strict` because the platinum `strict-typing` quality-scale tier requires it, and a pydantic-heavy library may opt in for the plugin. When a repo uses mypy it runs in CI and the editor (the `ms-python.mypy-type-checker` extension) so the two stay consistent, and its mypy -command joins the clean-compile. mypy may also be a build repo's only CI checker, run with its +command joins the clean-compile. The hub validator runs at most one checker in each Python +directory, mypy where both are configured. A repo enforcing pyright beside mypy runs pyright from +its own `.github/actions/validate/action.yml` hook. That hook starts from a bare checkout, so it +sets up its own Python environment. mypy may also be a build repo's only CI checker, run with its strict flags, and Pylance's pyright diagnostics are then advisory, since CI never runs them. A pyright-only repo is the lightest and is inherently consistent, since the editor and CI run one engine. @@ -101,9 +105,13 @@ uv build # produce wheel + sdist in ./dist (published pa ``` The **build**-profile Python clean-compile, in its uv form, is `uv run ruff format` + -`uv run ruff check` + the repo's type checker: `uv run pyright`, or `uv run mypy src` where mypy is -the CI checker, or both where the repo runs both (see Type checking above). Run it, plus -`uv run pytest`, before committing. +`uv run ruff check` + the repo's type checker. That checker is `uv run pyright`, or `uv run mypy` +where mypy is the CI checker, or both where the repo runs both (see Type checking above). Where the +config sits in the project directory, CI passes the checker no path, so the config's own target +settings decide what is checked. mypy left with no target that way exits with an error. A declared +subdirectory with no config of its own uses the repository root's instead. CI then runs the checker +from the root with the directory as its path, `uv run --project `, so run it +the same way. Run the clean-compile, plus `uv run pytest`, before committing. A **build**-profile directory in its pip form builds its environment the way CI does, through uv's pip interface, which writes no `uv.lock`. It installs every `requirements*.txt` in one command, since @@ -125,8 +133,12 @@ uvx ruff@latest format --check # verify format clean Its type checker runs against that environment. mypy runs as `.venv/bin/python -m mypy` where the environment installs it, and otherwise as `uvx mypy@latest --python-executable .venv/bin/python`, adding `--python-version` with the environment's version unless the mypy config pins one. pyright -runs as `uvx pyright@latest --pythonpath .venv/bin/python`. On Windows the environment's interpreter -is `.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's +runs as `uvx pyright@latest --pythonpath .venv/bin/python`. Where the config sits in the project +directory, CI runs the checker there with no path, as in the uv form. A declared subdirectory with +no config of its own uses the repository root's instead. CI then runs the checker from the root +with the directory as its path and the interpreter as `/.venv/bin/python`. Run it the same +way, as in `/.venv/bin/python -m mypy `. On Windows each interpreter path ends in +`.venv\Scripts\python.exe` instead. Those ruff commands and that type checker are the pip form's clean-compile, run with pytest before committing. A **lint-only** profile's clean-compile substitutes its `uvx` and `unittest` equivalents, per Two @@ -225,10 +237,10 @@ Before pushing or opening a PR: - VS Code's Problems pane should be quiet for the files you touched. The relevant linters are ruff (via the `charliermarsh.ruff` extension) and pyright (via the `ms-python.python` extension's bundled Pylance). -- The **build**-profile CI gate, in its uv form, is `uv run ruff check`, - `uv run ruff format --check`, the repo's type checker (`uv run pyright` or `uv run mypy src`), and - `uv run pytest`, the same commands as the local loop above, run from the Python project directory - (invoked as separate steps, not `&&`-chained, so the runner shell is irrelevant). The pip form's +- The **build**-profile CI gate, in its uv form, runs the local loop's commands above, each from + where that loop runs it. Those are `uv run ruff check`, `uv run ruff format --check`, the repo's + type checker, and `uv run pytest`. CI invokes them as separate steps, not `&&`-chained, so the + runner shell is irrelevant. The pip form's CI gate is its commands in the local loop above, and a **lint-only** profile's is its `uvx` equivalents plus its `unittest` suite, each per `references/profiles.md`. The local loop names what CI relaxes for an undeclared root. diff --git a/.github/skills/python-codestyle/references/profiles.md b/.github/skills/python-codestyle/references/profiles.md index 092a9bd25..4151386b9 100644 --- a/.github/skills/python-codestyle/references/profiles.md +++ b/.github/skills/python-codestyle/references/profiles.md @@ -9,7 +9,8 @@ than copying verbatim (a verbatim copy that misdescribes the repo is inaccurate in review). The axes that commonly vary per repo: - **Type checker in CI**: pyright strict, mypy with its strict flags (run in CI and the editor, with - pyright kept editor-only through Pylance), or both. The clean-compile runs every checker CI runs. + pyright kept editor-only through Pylance), or both. A repo running both runs pyright itself, + per `SKILL.md` "Toolchain". The clean-compile runs every checker CI runs. - **Dependency declaration**: the uv form declares the dev tools CI runs in the `dev` group of `[dependency-groups]`, the one group a plain local `uv sync` or `uv run` installs, and which CI's `uv sync --all-groups --frozen` installs too, since it takes every group and no extra. PEP 621 diff --git a/.github/skills/shell-codestyle/SKILL.md b/.github/skills/shell-codestyle/SKILL.md index 5f5894a19..1708f1dfc 100644 --- a/.github/skills/shell-codestyle/SKILL.md +++ b/.github/skills/shell-codestyle/SKILL.md @@ -70,8 +70,8 @@ depend on Python either. Everything else is Python, with a test under its own sc - **`shellcheck` clean, and a deliberate exception carries its reason inline.** A `# shellcheck disable=SCxxxx` names why the rule does not apply here, so the next reader can tell a considered exception from an unread warning. The hub's own `repo-config/configure.sh` is - the worked example, carrying `SC2016` disables where a single-quoted `jq` program must stay - unexpanded, each with its reason on the same line. + the worked example. Its `SC2016` disables sit only where shellcheck flags a single-quoted `jq` + program or GraphQL query that must stay unexpanded. Each carries its reason on the same line. - **Comments say why, never what.** The code states what it does. A comment restating it goes stale silently, where a comment carrying a reason fails visibly when the reason stops being true. diff --git a/.github/skills/skill-lifecycle/SKILL.md b/.github/skills/skill-lifecycle/SKILL.md index 79f5c590b..3a43e700c 100644 --- a/.github/skills/skill-lifecycle/SKILL.md +++ b/.github/skills/skill-lifecycle/SKILL.md @@ -32,7 +32,7 @@ A skill surfaces at a trigger moment. A rule that binds every action all the tim 5. **Apply the doc-packaging pattern below in the same change** when the skill packages a law doc or one of its sections. 6. **Regenerate and commit all trees together**: `python3 scripts/build_dist.py`, then, once authorized, commit the source and both generated trees in one commit, per `git-commit-conventions`. CI runs `--check` on every pull request and fails a desynced distribution. `python3 tests/test_build_dist.py` covers the generator itself. 7. **Record the surfacing**: annotate the `AGENTS.md` "Where the Rules Live" row when the skill packages a GOVERNANCE section, or its closing paragraph when the skill is new content, so the map stays the one place coverage is read from. -8. **Refresh the machines after promotion**: re-run `python3 scripts/skills_install.py --snapshot-only` per machine, from a freshly fetched `main`, the cadence `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. +8. **Refresh the machines after promotion**, per machine, as `docs/host-setup.md` "Fleet Skills Install" states. Until then each machine's Codex and opencode copy holds the previous skill set, which `--report` says, while Claude Code serves whatever the hub checkout holds. ## Changing or Retiring a Skill diff --git a/.github/skills/workflow-ci-contract/references/architecture.md b/.github/skills/workflow-ci-contract/references/architecture.md index 384dca25e..a33ab8239 100644 --- a/.github/skills/workflow-ci-contract/references/architecture.md +++ b/.github/skills/workflow-ci-contract/references/architecture.md @@ -28,7 +28,7 @@ flowchart LR The direct commit is an **allowance, not a substitute for review**. The ruleset drops the pull-request *requirement*, which permits a direct push without withdrawing the pull request, so a change worth reviewing still takes one and both paths reach `develop` legally. Which changes those are is stated as a shape rather than a line count in `GOVERNANCE.md` "Operational Repositories", which owns the test and is the one place it is written, since nothing in a ruleset can apply it. What differs is when validation lands. On the direct-commit path the commit is already on the branch, so CI can only be advisory after the fact, and that is the accepted cost of the model. On the pull-request path the change has not landed, so validation is pre-merge and actionable, which is the moment it is worth the most, and the lint workflow's `pull_request` trigger therefore names `develop` alongside `main` (`WORKFLOW.md` section 6). That is what makes **D1.2** hold here, since its input is *any* PR and the operational model is no exception. The check is reported on a `develop` PR rather than required, because a required status check on `develop` binds the direct push too and would dissolve the allowance the model is built on. -Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`; tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. +Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`, tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. ### Two Layers: Orchestration vs Build diff --git a/.github/skills/workflow-ci-contract/references/d-guarantees.md b/.github/skills/workflow-ci-contract/references/d-guarantees.md index 30a96a29f..d4c935b9b 100644 --- a/.github/skills/workflow-ci-contract/references/d-guarantees.md +++ b/.github/skills/workflow-ci-contract/references/d-guarantees.md @@ -50,7 +50,7 @@ The required behaviors, organized by domain. Each is a **MUST**, and its `Output - **D5.3 Best-effort.** Output: cleanup is `continue-on-error`, tolerates a failed listing, and deletes **all** matching ids. *Prevents: a cleanup hiccup reddening a job whose publish succeeded.* - **D5.4 Retention backstop.** Output: **every** `upload-artifact` sets `retention-days: 1`. - **D5.5 Never blanket-delete.** Output: cleanup MUST NOT enumerate and delete the run's whole artifact set. *Prevents: destroying diagnostic/log artifacts and auto-emitted build-records.* -- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy; and the split-ownership version of the same, where each side assumes the other prunes.* +- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy. It also prevents the split-ownership version of the same, where each side assumes the other prunes.* ### D6 - Seam / Architecture Conformance diff --git a/.github/skills/workflow-ci-contract/references/test-methodology.md b/.github/skills/workflow-ci-contract/references/test-methodology.md index dcbe64cc7..c9c86eca8 100644 --- a/.github/skills/workflow-ci-contract/references/test-methodology.md +++ b/.github/skills/workflow-ci-contract/references/test-methodology.md @@ -35,7 +35,7 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | S11 | scheduled upstream-version bump (wrapper) | resolver detects a change -> commits the state file -> opens a per-branch bump PR -> the merge-bot auto-merges it, or leaves it for the maintainer where the tracker sets `auto-merge: false` (D8.3) -> the `main` pin publishes via the gate (a develop pin does not auto-publish, shipping instead via a develop dispatch or promotion) | D8.3, D3.5 | | S12 | deploy dispatch naming an environment | the ref gate runs **first** (production from the default branch only, any ref to a non-production environment); validation runs; the callee re-asserts the environment name; a release installs under its own id; the pointer flips as a separate step; retention is bounded by whichever of the two D5.6 shapes the repo uses, so a deploy whose credential can observe the destination asserts the count converged and one confined write-only leaves it to the host; the live check asserts the environment and the release id, waiting out the reload, then the URL contract; **no tag and no release are created** | D2.1, D4.6, D5.6 | | S13 | deploy dispatch of a production environment from a non-default ref | **fails fast**, before anything is installed or written | D2.1 | -| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch; any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | +| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch. Any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | ### 5C. Live Probe (Where Warranted) diff --git a/AUDIT.md b/AUDIT.md index ff3328847..673dac1ae 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -198,7 +198,7 @@ A downstream repo uses its context where it is worth most. It **files findings a **Findings are a point-in-time snapshot. Stamp them and re-verify before acting.** [`spec/audit.py`][audit-runner] prints a run stamp (`audit run | hub `) and, per repo, the exact commit it read (`@ @`). Anything derived from a run (a report, and especially an **onboarding or conformance issue**) quotes that stamp, so a reader can tell whether it still applies. A convergence issue is generated from the audit, never composed by hand: `spec/audit.py --issue ` emits a ready-to-file title and body from that repo's live findings (grouped into must-fix, converge, and could-not-verify), so the issue content cannot drift from what the audit actually found and regenerates as the repo changes. -**Verify a convergence before it is promoted with `--branch`.** `spec/audit.py --branch ` reads that ref instead of the repo's registry `groundTruthBranch`, so a repo can audit its own `develop` while the work is still in flight rather than discovering the gaps after `main` has moved. The registry is not edited, the run is still read-only, and the run stamp names the override so a finding cannot be mistaken for one against ground truth. A ref that does not resolve is a single error naming it, never a baseline's worth of file-absent letters. +**Verify a convergence before it is promoted with `--branch`.** `spec/audit.py --branch ` reads that ref instead of the repo's registry `groundTruthBranch`, so a repo can audit its own `develop` while the work is still in flight rather than discovering the gaps after `main` has moved. The registry is not edited, the run is still read-only, and the run stamp names the override so a finding cannot be mistaken for one against ground truth. A ref that does not resolve is a single error naming it, never a baseline's worth of file-absent letters. A ref off the grammar a registry `groundTruthBranch` is held to, `GROUND_TRUTH_BRANCH_PATTERN` in [`spec/validate.py`][validate], is refused before anything is read. The run then exits `2` with one line on stderr naming the ref. **Re-running the audit needs a full hub clone with git history.** The verbatim stale-vs-modified classification and the intent staleness advisory walk the canonical's history (`git log` / `git show` from the hub root). A shallow hub clone cannot answer "matches a past hub revision". In one, `spec/audit.py` reports a single ERROR per repo and exits non-zero. `spec/audit.py --issue` exits 2 with the error on stderr. `spec/fidelity_honesty.py` stops before writing a report. Each error says to run `git fetch --unshallow origin` in that checkout. A downstream agent verifying one finding without the full history can instead compare against the current hub canonical on `main` (the whole file for a file-level unit, or the named `## heading` block for a verbatim section), which decides current-match but not stale-vs-modified. An agent picking up such an issue **re-runs the audit first and acts on the live result, not the pasted findings**: a repo moves between filing and pickup, so a stale block leads an agent to "fix" what is already fixed (re-requesting secrets that exist, attempting a no-op forward-sync). State the findings as evidence for *why* the issue was filed, never as the current state. diff --git a/WORKFLOW.md b/WORKFLOW.md index ecf530ea9..6a4126474 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -51,7 +51,7 @@ flowchart LR The direct commit is an **allowance, not a substitute for review**. The ruleset drops the pull-request *requirement*, which permits a direct push without withdrawing the pull request, so a change worth reviewing still takes one and both paths reach `develop` legally. Which changes those are is stated as a shape rather than a line count in `GOVERNANCE.md` "Operational Repositories", which owns the test and is the one place it is written, since nothing in a ruleset can apply it. What differs is when validation lands. On the direct-commit path the commit is already on the branch, so CI can only be advisory after the fact, and that is the accepted cost of the model. On the pull-request path the change has not landed, so validation is pre-merge and actionable, which is the moment it is worth the most, and the lint workflow's `pull_request` trigger therefore names `develop` alongside `main` (`WORKFLOW.md` section 6). That is what makes **D1.2** hold here, since its input is *any* PR and the operational model is no exception. The check is reported on a `develop` PR rather than required, because a required status check on `develop` binds the direct push too and would dissolve the allowance the model is built on. -Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`; tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. +Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees in `WORKFLOW.md` section 4 that assume a build/test pipeline are **N/A** exactly as for `source-only` (`WORKFLOW.md` section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`, tag + source zip). Branch-model rulesets are specified in `GOVERNANCE.md` "Branching Model" rather than in `WORKFLOW.md`. ### Two Layers: Orchestration vs Build @@ -178,7 +178,7 @@ The required behaviors, organized by domain. Each is a **MUST**, and its `Output - **D5.3 Best-effort.** Output: cleanup is `continue-on-error`, tolerates a failed listing, and deletes **all** matching ids. *Prevents: a cleanup hiccup reddening a job whose publish succeeded.* - **D5.4 Retention backstop.** Output: **every** `upload-artifact` sets `retention-days: 1`. - **D5.5 Never blanket-delete.** Output: cleanup MUST NOT enumerate and delete the run's whole artifact set. *Prevents: destroying diagnostic/log artifacts and auto-emitted build-records.* -- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy; and the split-ownership version of the same, where each side assumes the other prunes.* +- **D5.6 A durable destination's retention is bounded and owned.** Input: a deploy that installs a release beside the retained ones on a host the project owns. Output: retention is bounded by a **declared count**, and the side owning the prune is **written down**. Where the deploy credential can observe the destination, the deploy asserts the count converged and fails when it does not. Where the credential is deliberately write-only, so it can neither delete nor read back, the prune belongs to the **host** and that ownership is recorded there: widening the credential to reach the destination would trade a real confinement boundary for a check, which is the wrong trade. The release the live pointer resolves to is never a prune candidate, whatever the sort order says. A prune that runs against a local scratch tree, or that is best-effort, or that no side is recorded as owning, satisfies none of this. Unlike D5.1 through D5.4, this destination is durable rather than a run-scoped artifact, so no retention backstop expires it. *Prevents: a destination growing without bound until the disk fills, which surfaces as a site outage rather than as a failed deploy. It also prevents the split-ownership version of the same, where each side assumes the other prunes.* ### D6 - Seam / Architecture Conformance @@ -244,7 +244,7 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | S11 | scheduled upstream-version bump (wrapper) | resolver detects a change -> commits the state file -> opens a per-branch bump PR -> the merge-bot auto-merges it, or leaves it for the maintainer where the tracker sets `auto-merge: false` (D8.3) -> the `main` pin publishes via the gate (a develop pin does not auto-publish, shipping instead via a develop dispatch or promotion) | D8.3, D3.5 | | S12 | deploy dispatch naming an environment | the ref gate runs **first** (production from the default branch only, any ref to a non-production environment); validation runs; the callee re-asserts the environment name; a release installs under its own id; the pointer flips as a separate step; retention is bounded by whichever of the two D5.6 shapes the repo uses, so a deploy whose credential can observe the destination asserts the count converged and one confined write-only leaves it to the host; the live check asserts the environment and the release id, waiting out the reload, then the URL contract; **no tag and no release are created** | D2.1, D4.6, D5.6 | | S13 | deploy dispatch of a production environment from a non-default ref | **fails fast**, before anything is installed or written | D2.1 | -| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch; any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | +| S14 | publish whose branch head gained different `.github/workflows` content after the build began | every update since a release bot's push or merge: release-create **skipped**, the publisher is dispatched on the branch, this run ends **cancelled** with its package publish jobs skipped (a Docker image already pushed), and the dispatched run releases the head, except that a push landing after the branch is confirmed unmoved and before the dispatch is released as if it were one of them (an accepted, bounded limit per D4.7), and the dispatched run queues behind this run only where the head's publisher evaluates this run's group for a dispatch. Any other update: the step **fails**, nothing is released or dispatched | D4.1, D4.2, D4.5, D4.7 | ### 5C. Live Probe (Where Warranted) @@ -291,7 +291,7 @@ The rows below say which constructs a type brings with it. Read the repository f What each type adds beyond that, and how its leaf behaves, is below. - **`dotnet-publish`.** The target runs a sequential `dotnet publish` runtime loop inside one composite-action job. Configuration is Release on the default branch and Debug otherwise. A non-smoke run builds the full runtime set, archives the combined output as a `.7z`, and uploads it as `release-asset--dotnet-publish`. The archive is named from the project file stem unless `dotnet_publish_asset_name` overrides it. A smoke run builds a **strict, non-empty subset** of that runtime set and skips the archive and upload steps, so it uploads nothing. S1 smoke-builds that subset after a .NET project change. Where the repo has a publisher, S7 attaches the 7z from a non-smoke run. The non-default leg sets `prerelease=true`, and the default leg sets `prerelease=false`. GitHub marks the stable default release "Latest" automatically. -- **`nuget`.** The leaf uploads both `release-asset--nuget` and `nuget-build-` on a non-smoke run and pushes nothing, and a separate `publish-nuget` job in the repo's own publisher consumes the second and runs `dotnet nuget push *.nupkg --skip-duplicate`, then deletes it under the download step's own success (D5.2). Section 3's `Output Seam by Destination` says why the push sits there rather than in the leaf. Configuration is Release on the default branch, Debug otherwise. Where symbols are enabled (`snupkg`), the push auto-carries the paired `.snupkg` to NuGet.org's symbol server and the release-asset `.7z` also contains it, a triple surface. NuGet.org derives `isPrerelease` from the SemVer2 `-g` suffix (the workflow sets no such flag). Test: S7 non-default leg publishes a prerelease package + asset, default a stable; S9 re-run is a server-side `--skip-duplicate` no-op. 5C: query NuGet.org for both versions and the symbol package. +- **`nuget`.** The leaf uploads both `release-asset--nuget` and `nuget-build-` on a non-smoke run and pushes nothing, and a separate `publish-nuget` job in the repo's own publisher consumes the second and runs `dotnet nuget push *.nupkg --skip-duplicate`, then deletes it under the download step's own success (D5.2). Section 3's `Output Seam by Destination` says why the push sits there rather than in the leaf. Configuration is Release on the default branch, Debug otherwise. Where symbols are enabled (`snupkg`), the push auto-carries the paired `.snupkg` to NuGet.org's symbol server and the release-asset `.7z` also contains it, a triple surface. NuGet.org derives `isPrerelease` from the SemVer2 `-g` suffix (the workflow sets no such flag). Test: S7 non-default leg publishes a prerelease package + asset, default a stable. S9 re-run is a server-side `--skip-duplicate` no-op. 5C: query NuGet.org for both versions and the symbol package. - **`pypi`.** The leaf builds and uploads `pypi-build-`. A **separate** `publish-pypi` job (with `environment: pypi`, `id-token: write`, `actions: write`) does the OIDC Trusted-Publishing upload with `skip-existing: true`, then **consume-then-deletes** the build artifact under the download step's own success (D5.2), so on S9 it is deleted even though the `release-asset-*` delete is skipped. The version is the `X.Y.Z` core of `SemVer2` with `.dev0` appended on `develop` only, per D3.4. PyPI contributes no `release-asset-*`. A PyPI-only repo sets `expect_release_assets: false` at the caller. Test: S7 default leg publishes a release, non-default a `.dev0`; S9 is a `skip-existing` no-op; 5C inspects the `dist/*` filenames and the compute-version log. - **`docker`.** The leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only, with a per-branch registry buildcache. A single-image repo caches to `:buildcache-`, and a multi-image repo varies the cache **repository** rather than the tag, `:buildcache-` for each image, since the tag alone cannot distinguish two images (`cache-to` writes only the built branch and only on push, `cache-from` reads both branches). It contributes no `release-asset-*`, so a Docker-only repo's caller passes `expect_release_assets: false`. The readme job (`peter-evans/dockerhub-description`, `DOCKER_HUB_ACCESS_TOKEN`) runs **only** when the default branch publishes, whether called directly or reached through the hub-hosted `publish-docker-readme-task.yml`. Where the Docker Hub overview differs from the project README, the repo publishes a `Docker/README.md` through that task, the Hub description being size-limited. The docker-readme task validates its two mutually-exclusive input sources, `repositories` against `manifest`+`manifest-jq`, and defaults to the calling repository where neither is supplied, and a multi-image repo derives its publish matrix from the manifest. Docker **always re-pushes** the image, independently of a skipped release-create (S9). Test, under the default tag matrix, which a caller-supplied `matrix` or `docker-prepare` hook replaces: S7 default leg pushes `latest` + the version tag and updates the readme. Non-default pushes `develop` + the version tag (amd64 only). S9 still re-pushes. 5C Docker probe needs `DOCKER_HUB_*` secrets and same-repo (not fork) runs. - **`library` (a worked example, not a fleet type).** No such leaf ships, so this row walks D6.4's add-a-target procedure rather than an existing shape: a single new leaf that validates, zips, and uploads `release-asset--library` (`retention-days: 1` per D5.4, upload gated on smoke being false per D1.3). Adding it means a new `enable_library` input, a `build-library` job and its `github-release` and `build-docker` `needs:` entries in the release task, a `library` paths-filter entry, `changes` output, and `smoke-build` enable-forward in the PR workflow (without that last one, D1.1 never smoke-builds the library), and in the publisher the `enable_library` value it passes and the library's paths in its `on.push.paths` where it carries a `push` trigger. **Only a repo that owns its release task can make the release-task half of that edit.** The hub-hosted release task declares a closed input set and a call passing an input it does not declare fails, so a repo calling it sets `enable_release_asset` instead and carries the leaf as its `build-release-asset` hook. The hook writes the zip, and the task uploads it under `release-asset--build-release-asset`, skipping the upload on a smoke run. Keep `expect_release_assets: true` (it has a file target, unlike Docker). The caller's own validation job is replaced by a type-appropriate validator only where the reusable one cannot express this repo's validation, with the aggregator re-pointed to the replacement (D1.2). `smoke-build` keeps `needs: [changes]`, as D1.2 has it. `version.json` and the NBGV `get-version` **job** are retained (they own the tag). Test: S1 smoke runs validate+zip and uploads nothing; S7 attaches the zip, prerelease on the non-default leg; S9 on a push re-run skips release-create and the asset-delete (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run is D4.4's refresh case rather than S9's, re-uploading then re-deleting the asset. diff --git a/catalog/snippets/vscode/README.md b/catalog/snippets/vscode/README.md index 1cbfc9815..b132d6bf2 100644 --- a/catalog/snippets/vscode/README.md +++ b/catalog/snippets/vscode/README.md @@ -14,8 +14,8 @@ The standard set holds only extensions that work without a separately installed ## Language and Target Additions -- **.NET / C#** (`dotnet.jsonc`): `ms-dotnettools.csdevkit`, `csharpier.csharpier-vscode`; format-on-save for `[csharp]` via CSharpier. -- **Python** (`python.jsonc`): `ms-python.python`, `ms-python.vscode-pylance`, `charliermarsh.ruff`, `ms-python.mypy-type-checker`; format-on-save and import organization for `[python]` via Ruff. +- **.NET / C#** (`dotnet.jsonc`): `ms-dotnettools.csdevkit` and `csharpier.csharpier-vscode`. Format-on-save for `[csharp]` runs via CSharpier. +- **Python** (`python.jsonc`): `ms-python.python`, `ms-python.vscode-pylance`, `charliermarsh.ruff`, and `ms-python.mypy-type-checker`. Format-on-save and import organization for `[python]` run via Ruff. - **Docker** (`docker.jsonc`): `ms-azuretools.vscode-docker`. ## Settings diff --git a/docs/fleet-map.md b/docs/fleet-map.md index 01adf1616..b2c739627 100644 --- a/docs/fleet-map.md +++ b/docs/fleet-map.md @@ -145,7 +145,7 @@ Four wiring points close the model, and each is in place: 1. **Bootstrap** (G1, closed): [`host-setup/bootstrap.sh`][bootstrap] and [`bootstrap.ps1`][bootstrap-ps1] end their host mode with a skills step, driven by the `install-skills` pair in the platform directories, degrading gracefully when the `claude` CLI is absent (the overlay half still lands, and the stamp records the partial install). Each loader hands the commit it resolved to the installer, so a stamp written from the tarball tree stays checkable. 2. **Host contract** (G1, closed): [`docs/host-setup.md`][host-setup-doc] states the install and the verify command in its "Fleet Skills Install" section, and the [`README.md`][readme] "Using This Repo" section names the skills install among its four deployed things. -3. **Session entry** (G6, closed): the tail of [`AGENTS.md`][agents] says a rule that keeps needing restating signals a stale install, and the `check-this-repo` skill runs the report and states the cadence, so the symptom routes to the check without new tooling. +3. **Session entry** (G6, closed): the tail of [`AGENTS.md`][agents] says a rule that keeps needing restating signals a stale install. The `check-this-repo` skill runs the report and points to [`docs/host-setup.md`][host-setup-doc] for the cadence, so the symptom routes to the check without new tooling. 4. **Refresh cadence** (G6, closed): [`docs/host-setup.md`][host-setup-doc] "Fleet Skills Install" states it: re-run the installer from a freshly fetched `main` when `--report` exits non-zero, and after any promotion to `main` that touches `.agents/skills/`. The maintainer runs it by hand, and an automated refresh is deliberately out of scope until the fleet has evidence the manual cadence fails. ## Gap Register @@ -210,7 +210,7 @@ flowchart LR ### G6: Session Entry Never Checks Skill Staleness (Closed) - **Gap** - A machine with stale or missing skills behaves like a machine that never installed them, and nothing at session entry said so. The symptom is a rule that keeps needing to be restated. -- **Resolution** - The cadence is stated in both places the row asked for. [`docs/host-setup.md`][host-setup-doc] "Fleet Skills Install" directs a re-run of the installer from a freshly fetched `main` when `--report` exits non-zero and after any promotion to `main` touching `.agents/skills/`, and the `check-this-repo` skill carries the same cadence in its own "Refresh cadence" section, routing the restated-rule symptom to the report it already runs. No new tooling, by design: the trigger is suspicion, and session entry stays uninstrumented until the fleet has evidence the manual cadence fails. +- **Resolution** - The cadence has one home, and the other place the row asked for points to it. [`docs/host-setup.md`][host-setup-doc] "Fleet Skills Install" directs a re-run of the installer from a freshly fetched `main` when `--report` exits non-zero and after any promotion to `main` touching `.agents/skills/`. The `check-this-repo` skill points there from its own "Refresh cadence" section and routes the restated-rule symptom to the report it already runs. Its own `--snapshot-only` refresh covers only the checkout it fetches. No new tooling, by design: the trigger is suspicion, and session entry stays uninstrumented until the fleet has evidence the manual cadence fails. ### G7: Operational Develop PR-Only Is Prose-Enforced (Closed) @@ -226,7 +226,7 @@ flowchart LR ### G9: WORKFLOW.md and AUDIT.md Have No Skill (Closed) -- **Gap** - The largest law doc ([`WORKFLOW.md`][workflow], the D1-D9 contract) and the measurement procedure ([`AUDIT.md`][audit]) had no skill surface, while every other procedure and language did. Thirteen [`GOVERNANCE.md`][governance] sections were likewise doc-only. +- **Gap** - The CI/CD law doc ([`WORKFLOW.md`][workflow], the D1-D9 contract) and the measurement procedure ([`AUDIT.md`][audit]) had no skill surface, while every other procedure and language did. Thirteen [`GOVERNANCE.md`][governance] sections were likewise doc-only. - **Resolution** - `audit-a-repo` packages `AUDIT.md` in the kept-authority shape, the doc keeping the full rules and the skill routing into it. `workflow-ci-contract` packages `WORKFLOW.md` sections 3, 4, and 5 as generated includes and the rest of that document in the same kept-authority shape. The [`AGENTS.md`][agents] rule map carries a disposition per section: `Workflow YAML Conventions` and the three conduct sections are annotated with their surfacing skill, and a paragraph after the table states why each remaining unannotated section is doc-only by decision, so absence reads as a choice rather than an oversight. Both closing tests hold: the skills ship, and the map carries the dispositions. - **Provenance** - All four phase-2 skills shipped in one pull request at the maintainer's direction, superseding the one-pull-request-per-skill note this doc carried, with `skill-lifecycle` authored first inside it so the others follow its procedure. @@ -265,7 +265,7 @@ Four skills close G9, G10, and G12, shipped through the [`.agents/skills/`][skil ### workflow-ci-contract - **Scope** - The [`WORKFLOW.md`][workflow] behavioral contract: the D-guarantees, the seam contract, artifact lifecycle, NBGV versioning, and validate-at-entry, with the architecture, the guarantee catalog, and the test methodology carried as references. -- **Trigger** - Writing or editing workflow YAML, adding or dropping a release target, or reasoning about why a publish did or did not fire. +- **Trigger** - Writing or editing anything under `.github/workflows/`, a composite action under `.github/actions/`, or `version.json`, adding or dropping a release target, auditing a repository's workflows, or tracing which job, input, or condition made a publish run or skip. - **Packages** - The YAML half of the pipeline. `branching-and-release-model` keeps the git half (branching, promotion, publish policy), and the two descriptions state the split. - **Overlap** - The source doc is large, so the skill is a summary with `references/` splits, the shape `comment-and-doc-style` already uses. Sections 3, 4, and 5 are each carried whole as a generated include. diff --git a/registry/repos.json b/registry/repos.json index 52dc02d0b..55e67e568 100644 --- a/registry/repos.json +++ b/registry/repos.json @@ -33,8 +33,7 @@ "publish": [{ "target": "nuget", "mechanism": "oidc" }], "requiredSecrets": ["NUGET_USERNAME", "CODECOV_TOKEN"], "consumerModel": "pull", - "releaseTrigger": "two-phase", - "driftNotes": ["No get-version-task, relying on validate-task instead."] + "releaseTrigger": "two-phase" }, { "name": "LanguageTags", diff --git a/repo-config/configure.sh b/repo-config/configure.sh index b924a2cba..9fbbf8553 100755 --- a/repo-config/configure.sh +++ b/repo-config/configure.sh @@ -367,7 +367,6 @@ apply_project() { # project-node-id number="$(jqr '.number' "$project_file")" title="$(jqr '.title' "$project_file")" live="$(repo_projects)" - # shellcheck disable=SC2016 # $p is a jq --arg variable, not a shell expansion if jq -e --arg p "$pid" '.projects | index($p) != null' <<<"$live" >/dev/null 2>&1; then echo "Already linked to project '$title' ($owner, number $number)" return @@ -555,9 +554,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol ptypes="$(jqr '[.rules[] | select(has("parameters")) | .type] | .[]' "$file")" while IFS= read -r t; do [ -z "$t" ] && continue - # shellcheck disable=SC2016 # $t is a jq --arg variable, not a shell expansion want="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" "$file")" - # shellcheck disable=SC2016 # $t is a jq --arg variable, not a shell expansion got="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" <<<"$live")" assert "'$rname' rule '$t' parameters match the payload" test "$got" = "$want" done <<<"$ptypes" @@ -711,7 +708,6 @@ check_environments() { return fi # The declared value is emitted verbatim, invalid shapes included, so each one reaches the test that judges it rather than being defaulted away here. - # shellcheck disable=SC2016 # $n is a jq --arg variable, not a shell expansion if ! entries="$(jq -c --arg n "$name" '[.repos[] | select(.name == $n)][0] | if has("environments") then .environments else [] end' "$registry")"; then fail "could not read the declared deployment environments from $registry" return @@ -746,7 +742,6 @@ check_environments() { fi ename="$(jqr '.name' <<<"$row")" policy="$(jqr '.branchPolicy' <<<"$row")" - # shellcheck disable=SC2016 # $n is a jq --arg variable, not a shell expansion env_live="$(jq -c --arg n "$ename" '[.[] | select(.name == $n)] | first // empty' <<<"$live_envs")" if [ -z "$env_live" ]; then fail "environment '$ename' missing" diff --git a/reports/_template.md b/reports/_template.md index 621080ce3..ed8e5e03b 100644 --- a/reports/_template.md +++ b/reports/_template.md @@ -17,16 +17,20 @@ | nuget | | | | | | pypi | | | | | | python | | | | | -| console | | | | | +| dotnet-publish | | | | | +| hugo | | | | | | docker | | | | | | branch-model | | | | | +| carried-scope | | | | | | repo-setup | | | | | +| runtime-secrets | | | | | +| verbatim-tree | | | | | | linter-parity | | | | | | recurring-violations | | | | | | readme-structure | | | | | | workflow (WORKFLOW.md 5A/5B) | | | | | -Verdict values: pass | drift | defect | N/A. Remove rows that are N/A for the repo's types, or mark them N/A. +Verdict values: operational | not-operational | N/A. Letter and Intent values: pass | miss | N/A, where a letter miss with intent pass is a drift finding and both missing is a defect. Mark a row N/A, in all three columns, only when its checks govern a construct the repo does not contain, and never delete a row, so the report shows the dimension was judged. ## Defects (most severe first) diff --git a/scripts/README.md b/scripts/README.md index 831756147..41b9638f7 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -77,11 +77,11 @@ The `spelling` rule covers the US English convention where cspell does not reach A double-quoted span in Markdown is treated as a quotation and not scanned for prose rules, so a rule that states its own counter-example does not report the document that documents it. Outside Markdown a double quote is structural, so the prose inside it still counts. -The `semicolon` and `dash` rules ban a construction rather than a detectable subset of it, so each flags by default and the exceptions are the ones the rule names: a semicolon inside a list that already carries commas, and for the dash a compound word, a leading list marker, a range, and the `- **Label** - explanation` separator that opens a governed bullet. +The `semicolon` and `dash` rules ban a construction rather than a detectable subset of it, so each flags by default and the exceptions are the ones the rule names: a semicolon inside a list that already carries commas, and for the dash a compound word, a leading list marker, a range, and the `- **Label** - explanation` separator that opens a governed bullet. Where such a list is joined by a single semicolon, each of its two items carries a label. **The semicolon rule reads the list where it lives.** The comma qualifies the list as a whole rather than one separator's position, so an enumeration whose commas fall in a later item keeps every semicolon it carries. Reading it positionally split one series in two, flagging the openers of the same list it then exempted the tail of, which would have restructured the enumerated guarantees the exemption exists to protect. A Markdown table row is judged one cell at a time, since a row is a record of fields and a comma in one column cannot excuse a semicolon in another, and a bullet's `**Label**:` is dropped before the line is read, because it opens the bullet rather than announcing a list, the same construct the label dash is exempted for. The colon is written inside the emphasis as often as outside it, so `**Label:**` is dropped on the same grounds, matching only one spelling having left the other announcing a list it never announced. -**The sentence is the unit the exemption is judged on, because that is where a list lives.** The whole bullet decided it once, so a colon anywhere before the first semicolon marked the bullet a list and exempted every semicolon after it, however plainly one joined two independent clauses, and the two did not have to be near each other or related at all. Measured over this repo when it was fixed, the exemption was covering 62 spans holding 120 semicolons across 9 files while the rule reported none of them, so the gate read as clean over the docs it exists to check. Scoping it to the sentence reported 43 further semicolons and silenced none, with a 44th from dropping the other spelling of the label colon, and the sentence boundary is the run-on rule's, so an initial or an abbreviation ends nothing and a terminator closing inside emphasis or a bracket (`.**`, `.)`) still ends a sentence. The colon arm was measured before being kept rather than dropped: dropping it flagged 14 further lines, and those were genuine colon-introduced lists whose items carry commas, which is the standard use the rule names. +**The sentence is the unit the exemption is judged on, because that is where a list lives.** The whole bullet decided it once, so a colon anywhere before the first semicolon marked the bullet a list and exempted every semicolon after it, however plainly one joined two independent clauses, and the two did not have to be near each other or related at all. Measured over this repo when it was fixed, the exemption was covering 62 spans holding 120 semicolons across 9 files while the rule reported none of them, so the gate read as clean over the docs it exists to check. Scoping it to the sentence reported 43 further semicolons and silenced none, with a 44th from dropping the other spelling of the label colon, and the sentence boundary is the run-on rule's, so an initial or an abbreviation ends nothing and a terminator closing inside emphasis or a bracket (`.**`, `.)`) still ends a sentence. A lone semicolon separates a list only where the items on both sides of it open with a label, as in `Inputs: a, b; outputs: c`. No structure tells an explanatory colon from a list colon. A colon and a comma alone once exempted the one semicolon joining two clauses after them. A sentence with two or more semicolons and a comma stays a series whatever colon it holds. **Both are Markdown-only for now.** A shell script carries 78 statement separators that are not prose at all, so telling a comment from code is a precondition for reaching source files. Until then a semicolon or dash in a code comment is missed, which reading the diff by eye still catches. @@ -176,13 +176,13 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` -**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``, and only after confirming the checkout is the pushed head, holds no other change, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. +**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``, and only after confirming the checkout is the pushed head, holds no other change, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head. Where it is attested, `wait` polls the head's checks instead. The poll runs only while the checks can still move the merge, as the `30` paragraph below lists. Once they settle, or the pull request closes, the head ends as covered. Where it is not attested, `wait` exits `49`, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. `--repo` is required and carries no default. A default names one repository, and a run from anywhere else resolves its number there instead: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent. -`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. +`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. `wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers. @@ -207,11 +207,11 @@ A partial round is reported and handed over rather than retried into. Measured o The vetted inventory is measured rather than imagined, and it is small because the output is regular. Across those 332 bodies, with fenced blocks dropped and text reduced to ASCII, the whole corpus is nine headings, six `` texts and three metadata labels. Counts are normalized to `(N)` and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section having changed, and dropping the emoji is also what keeps this repository's charset rule satisfied. Letter case is folded at the comparison rather than in the reduction, so a marker keeps the case the reviewer wrote while a drift of case alone stops blocking a loop, which it did once, on a heading already in the inventory. A body carrying **no** heading at all is itself unrecognized, which is what catches a rewrite that changes every marker at once, and a **refusal is exempt** because it is a bare paragraph by design and `REFUSAL` is its vetted spelling. That exemption is the pattern rather than a carve-out, so a refusal reworded stops being exempt and blocks, which is the refusal check's own failure mode caught one rewording later. The last reading is the quietest: a reviewer **login** that reads as this reviewer without being the spelling every query filters on, since a rename leaves every filter matching nothing and the digest then reports a review that landed as no review at all. A case runs the whole inventory over the measured corpus, where it raises nothing. `wait` exits `44` when the review loop has closed but a required check sits in a shape no wait clears. The digest carries `checks=N/M` beside the merge word and names the shape as `stuck=`, because `mergeStateStatus` reports one word, `BLOCKED`, for a red check, a check nothing is running, an unresolved thread, and a missing approval alike, and a run in this repository spent twenty-five minutes polling that word on a pull request whose only unfinished check was an aggregator job no runner ever took. The cause came from the maintainer rather than from any field here, which is the whole defect: the digest named the state it could not explain and stopped there. Four shapes are told apart because each wants a different response. `NOT_POSTED` is a required status whose poster has not spoken, which is a `StatusContext`'s `EXPECTED` and only ever that. It is deliberately not folded in with the starved shape, since no runner is owed a status nothing has posted, so re-running a workflow clears nothing and the starved wording would send a reader at the runner pool over a missing poster. `NOT_PICKED_UP` is a check GitHub dispatched and assigned no runner, read from the queued state rather than from a runner name GraphQL does not carry, and the state suffices because a job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so there is no dependency-blocked queue to mistake for a starved one. Nothing agent-side starts it, since the pool is GitHub-hosted, so the remedy is a re-run or that capacity. `RUNNING_LONG` is deliberately the weaker reading and its wording says so, because duration alone cannot separate a hung job from a slow one: this repository's lint job legitimately runs nine to eleven minutes while its aggregator is a single shell conditional, so the threshold is generous, the elapsed time prints for the reader to judge against what the job costs, and nothing asserts a fault. `FAILED` is a verdict rather than a stuck check, reported so no reader deduces a red check from `BLOCKED`. -A check merely still running normally is **not** any of these and exits `0`. That boundary is the whole design, because `wait` returns the moment coverage lands and on almost every pull request the checks are still going at that instant, so taking `44` for a pending check would make `44` the ordinary outcome and a code that fires always carries nothing. `44` additionally requires `mergeStateStatus: BLOCKED`, which the module docstring's own list of shapes has to name rather than only implying, because a reader who sees `stuck=FAILED` and exit `0` on a merge that is `UNSTABLE` should find the condition written down rather than infer the field is unreliable. It is because a rollup carries checks the ruleset does not require and four of the six on a green pull request here are exactly that, so the code borrows GitHub's own reading of which checks gate a merge rather than reading the ruleset's contexts over another call. `CLEAN` proves no required gate is outstanding whatever else the rollup is doing, and without that condition a stuck check nothing requires returns `44` on a mergeable pull request. The digest names the check either way, so the narrower code costs the reader nothing. Both of those came out of this change's own review. +A check merely still running normally is **not** any of these and exits `0`. That boundary is the whole design, because `wait` returns the moment coverage lands and on almost every pull request the checks are still going at that instant, so taking `44` for a pending check would make `44` the ordinary outcome and a code that fires always carries nothing. An attested held head is the exception, its coverage landing before CI concludes, so `wait` polls its checks as the `attest` paragraph states. `44` additionally requires that the stuck check be required and that the merge read `mergeStateStatus: BLOCKED`. The module docstring's own list of shapes names both conditions rather than only implying them. A reader who sees `stuck=FAILED` and exit `0` should find the condition written down rather than infer the field is unreliable. A rollup carries checks the ruleset does not require, four of the six on a green pull request here. So the code reads each rollup node's `isRequired`, which the same query already fetches, and counts only a check it marks required. Without that, a merge `BLOCKED` by an unresolved thread or a missing approval and carrying a failed check nothing requires returns `44`. `CLEAN` proves no required gate is outstanding whatever else the rollup is doing, so `BLOCKED` stays required as well. The digest names every stuck check either way, so the narrower code costs the reader nothing. A rollup member that is neither a `CheckRun` nor a `StatusContext` is skipped rather than forced into one of those shapes, since forcing reads a label under a key the node does not use and a state that is not there, so an unknown member would arrive as an anonymous failure. Skipping it **quietly** would be the other half of the same mistake, because a check absent from the tally and the stuck reading renders as a clean pass over something never seen, which is this script's own core failure and not a case its newest field gets an exception from. So the member is carried as a marker, counted by neither reader, and named on a `CHECKS PARTIALLY UNREAD` line. `CHECKS_WINDOW` is substituted into `Q_FULL` rather than sitting beside a hard-coded `100`, since a constant that does not drive the query only documents the literal, and the case asserting the two agree holds only where someone runs it. The substitution is a `.replace` rather than an f-string because GraphQL is braces from end to end and interpolation would need every one of them doubled. The rendering path reads every field with `.get` and clamps a negative age to zero, for the reason `age` catches two exceptions: a caller handing the digest an odd node shape, or a clock behind GitHub's, should cost a field rather than the one call whose job is to report the state. Through the CLI the negative age is unreachable, since a negative age cannot exceed a non-negative threshold and the parser refuses a negative one, so that clamp is hardening on the library path rather than a live defect. -The contexts connection is guarded the way the review and comment windows are, since it has the same failure. A rollup past a hundred contexts would drop the rest silently, so a required check among them would be absent from the tally and the stuck reading alike and the digest would render a clean pass over a check it never saw, which a fleet repository with a large matrix build reaches long before this one does. `CHECKS TRUNCATED` says so instead. The rollup is normalized once per digest and the list handed to both readers, since the parse is what this script exists to spend once, and the two readers calling it separately was invisible in the output, which is what let it pass. The `44` message is worded as a coincidence rather than a cause, because nothing here proves the stuck check is the blocker: `BLOCKED` is also worn by an open thread or a missing approval, so naming the check as the blocker would assert a link this cannot read. The normalized stamp key is `since` rather than `started`, since it holds a `CheckRun`'s `startedAt` for one shape and a `StatusContext`'s `createdAt` for the other, and one name over two different meanings reads like a comparison of equivalents. +The contexts connection is guarded the way the review and comment windows are, since it has the same failure. A rollup past a hundred contexts would drop the rest silently, so a required check among them would be absent from the tally and the stuck reading alike and the digest would render a clean pass over a check it never saw, which a fleet repository with a large matrix build reaches long before this one does. `CHECKS TRUNCATED` says so instead. The rollup is normalized once per digest and the list handed to both readers, since the parse is what this script exists to spend once, and the two readers calling it separately was invisible in the output, which is what let it pass. The `44` message names each required stuck check, so a reader can tell it from an optional one the block above also prints. It calls them one cause of the block rather than the only one, since an open thread or a missing approval reads `BLOCKED` too. The normalized stamp key is `since` rather than `started`, since it holds a `CheckRun`'s `startedAt` for one shape and a `StatusContext`'s `createdAt` for the other, and one name over two different meanings reads like a comparison of equivalents. The rollup is selected by matching `headRefOid` rather than taken as the connection's first node, and where no commit matches, the digest says `CHECKS UNREADABLE` instead of reading another commit's rollup or letting `checks=0/0` pass for a fact about the head. A fallback to the newest node is the stale reading reached by a different route, and a silent `0/0` is the narrowing this whole script is built against, which its own newest field does not get an exception from. The case that was supposed to hold this asserted the *fixture's* commit equalled the head, which tests the payload rather than the code, and the code was reading position regardless. The reading of an age is guarded the same way: `age` catches `TypeError` as well as `ValueError`, because a stamp carrying no zone *parses* and yields a naive datetime that will not subtract from an aware `now`, so catching one and not the other lets a crash out of the call whose whole job is to report the state. `--check-grace` must be less than `--check-stall`, since asserting the two constants are ordered while leaving the flags free to invert them is the gap between a rule and its check one level down. All four came out of this change's own review, as low-confidence findings carrying no thread. @@ -357,7 +357,7 @@ The target is required to be clean, with no owned root exempted from the unrelat Installs the fleet's Skills for the current machine, cross-platform and idempotent, mirroring [`host-setup/agent-safety/claude/install.py`][agent-safety-install]'s shape: `skills_install.sh` and `skills_install.ps1` are thin wrappers that locate a Python 3 interpreter and hand off, so every OS runs one tested code path. Two independent things happen on a run, since the three tools this fleet targets discover skills differently: `.agents/skills/` is materialized (not symlinked) to `$HOME/.agents/skills/`, so Codex and opencode's global scan covers every repo on the machine rather than only the one that happens to be open, and this repo's marketplace is registered with the `claude` CLI (`claude plugin marketplace add`, `claude plugin install`) so Claude Code loads the same content the other two read directly. The marketplace/plugin registration goes through the `claude` CLI's own commands rather than writing its internal `known_marketplaces.json` by hand, because that file's shape is the CLI's state, not a documented contract, and a hand-written copy risks drifting from what the CLI expects on its next release. `claude plugin marketplace add` re-points an existing registration without failing, so the installer reads the CLI's listing first and moves a registration only where it is a directory source whose path is no longer a directory, saying so. Any other existing registration is left in place, and the run prints the command that moves it. Where the CLI gives no listing, the run registers nothing and exits non-zero. `--snapshot-only` refreshes the Codex and opencode copy and its stamp without touching the registration at all. -The two channels hold different things: the Codex and opencode copy keeps the revision it was taken from, while the Claude Code marketplace is a directory source that loads the hub checkout in place and serves whatever it holds at read time. `--report` reads the stamp a prior run wrote (`$HOME/.agents/skills-install-stamp.json`, naming the hub commit installed) and answers each channel by name, without installing anything. The copy is judged against the promoted `main` as last fetched, or `--intended ` in a git checkout, or on a bootstrapped tarball tree the commit its loader resolved, where `--intended` is refused. The live channel reports the branch, commit, and dirty state of the checkout the marketplace registers. The exit code is the copy's verdict alone. Where no intended revision resolves, the copy is not asserted current and the snapshot's `reason` says it cannot be judged, which a re-install does not fix. A repository whose `AGENTS.md` keeps needing a rule restated is usually this: the machine was never installed, or was installed from an older commit. +The two channels hold different things: the Codex and opencode copy keeps the revision it was taken from, while the Claude Code marketplace is a directory source that loads the hub checkout in place and serves whatever it holds at read time. `--report` reads the stamp a prior run wrote (`$HOME/.agents/skills-install-stamp.json`, naming the hub commit installed) and answers each channel by name, without installing anything. The copy is judged against the promoted `main` as last fetched, or `--intended ` in a git checkout, or on a bootstrapped tarball tree the commit its loader resolved, where `--intended` is refused. The live channel reports the branch, commit, and dirty state of the checkout the marketplace registers. Where the marketplace is registered it also carries `plugin`, `installed` and `enabled` for the user-scope install the installer makes, read from `claude plugin list --json` (each null where that listing cannot be read). A project- or local-scope install does not count, and `enabled` is as Claude Code reports it for the directory the report runs in. The exit code is the copy's verdict alone. Where no intended revision resolves, the copy is not asserted current and the snapshot's `reason` says it cannot be judged, which a re-install does not fix. A repository whose `AGENTS.md` keeps needing a rule restated is usually this: the machine was never installed, or was installed from an older commit. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 904932672..5911ec02c 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -95,11 +95,12 @@ file-count refusal is spent by coverage of the same head. `wait` is where that state gets its own exit codes, 46 and 47 below, because only `wait` is the command a caller might otherwise poll out a timeout on. - `unresolved` counts every tracked reviewer's own open thread, not only Copilot's: - CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are - tracked at the identity and thread-resolution level, since an open thread blocks a - ruleset-gated merge whoever opened it and `unresolved=0` once hid one of theirs that - still did. + `unresolved` counts every open thread whoever opened it, since an open thread blocks + a ruleset-gated merge whatever its author, and a filter on known reviewer logins + once read `unresolved=0` over a qodo thread posted under a login it did not know. + The known logins, Copilot, CodeRabbit (`coderabbitai`), and qodo + (`qodo-free-for-open-source-projects`), only attribute a thread in the breakdown + beside the count, where any other author reads as `other`. Both `threads=` and `unresolved=` are read from a single 100-thread page with no further pagination, so a pull request carrying more than that undercounts silently past that point: both fields print a trailing `+` and a `THREADS TRUNCATED` block @@ -226,8 +227,10 @@ double-requests. It is also skipped under 46's and 47's quota readings below, since a request into a reached limit spends quota and returns the same refusal. It is skipped too on a pull request into a branch other than the default once Copilot has reviewed it at - all, since a fix push there is covered by an attested local pass: an attested head ends - the wait as covered, and one with no attestation exits 49 naming the `attest` step. + all, since a fix push there is covered by an attested local pass: an attested head is + covered at once, so the wait polls its checks instead only while they can still move + its merge, as 30 below lists, and one with no attestation exits 49 naming the `attest` + step. --request asks for a round anyway. A pull request into the default branch, a promotion among them, a pull request Copilot has not reviewed yet, and one with a partial on record or a review history past the window are requested as before. The comment also carries the @@ -239,8 +242,14 @@ polling only) where both windows come up empty, since a repository with no Copilot review in either has nothing to read the id from and a fabricated one is never an option. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. - Exit 0 = review present, or on a held head an attested local pass, 30 = still pending at - timeout (pending is not failure), + Exit 0 = review present, or on a held head an attested local pass with its required + checks settled as far as the rollup window reads them, with the merge reading CLEAN, + UNSTABLE, or HAS_HOOKS, or with the pull request closed, 30 = still pending at timeout + (pending is not failure), on a held head a required check still settling, any check while + no required one has posted, none posted on a BLOCKED, BEHIND, or DRAFT merge, or the + merge still UNKNOWN, printed as `status=CHECKS_PENDING`, or a round requested while the + held poll ran, printed as `status=PENDING` unless 40's outside answer or 46's or 47's + quota reading outranks it, 40 = Copilot answered outside a formal review, so read the printed body. 40 reports the shape of that answer and reads nothing of its cause: an answer carrying no commit covers no head, so the wait ends and the reader decides. @@ -252,12 +261,14 @@ head carrying a 50 a stuck check shows only in the digest. A 42 can be decided by an earlier round whose partial coverage carries to a head stating none, in which case the round the wait ended on is not the round the code came from. - 44 = the review loop closed, the merge reads BLOCKED, and a check is in a shape no - wait clears: queued with nothing acting on it, expected and never posted, running - far past what the job costs, or failed. A check merely still running normally is - not this and exits 0, and neither is a stuck check on a merge that is not BLOCKED, - since the rollup carries checks no ruleset requires. The digest reports the check - in both cases, so a shape outside 44 is still named rather than lost. + 44 = the review loop closed, the merge reads BLOCKED, and a required check is in a shape + no wait clears: queued with nothing acting on it, expected and never posted, running far + past what the job costs, or failed, though on a held head a required check running long + is still polled, as 30 states. A check merely still running normally is not this and + exits 0, or on a held head is polled while it can still move the merge. Neither is a + stuck check its rollup node reads as not required, since the rollup carries checks no + ruleset requires, nor any stuck check on a merge that is not BLOCKED. The digest reports + the check in every case, so a shape outside 44 is still named rather than lost. 46 = the newest Copilot review on the pull request, on this head or an earlier one, is a refusal naming the account quota, or one saying only that it encountered an error, printed above under COPILOT REFUSED THIS ROUND. The weekly rate limit posts that error @@ -278,7 +289,8 @@ a request already pending is still polled for. Pass --ignore-quota-signal to request and poll --timeout anyway once the quota is believed to have reset. 46 is read from this pull request's own reviews and always takes priority over 47, so a genuine 0/40/41/42/43/45 on - this pull request outranks 47 whenever both would otherwise apply, and so does a 50. + this pull request outranks 47 whenever both would otherwise apply, and so does a 50, and + on a held head covered by an attested local pass a 30 or a 44. A pending request remains pending until a review, an answer, or the timeout. GitHub's effort-labeled review lifecycle does not always emit `copilot_work_started`, so that event is not evidence that distinguishes queued work from abandoned work. @@ -332,8 +344,6 @@ # Other review bots this repository has trialed alongside Copilot. # Tracked at the identity level only, login and commit oid, never body prose, except where a reader below names one explicitly. # Each format read here is its own reader, and doing that well is a separate task per bot. -# What generalizes without reading any of their prose is thread resolution. -# An open thread blocks a ruleset-gated merge whoever opened it, and `status`'s `unresolved=0` once silently hid a CodeRabbit/qodo thread that did block one. CODERABBIT_LOGIN = "coderabbitai" QODO_LOGIN = "qodo-free-for-open-source-projects" # Named rather than inlined at each of their own readers below, so a login rename updates one spelling instead of silently leaving a hardcoded copy matching nothing. @@ -842,30 +852,35 @@ def strip_fences( requestReviews(input:{pullRequestId:$pr, botIds:[$bot], union:true}){ pullRequest{ id __REQUEST_STATE__ } }} """.replace("__REQUEST_STATE__", REQUEST_STATE) -# Full query: run once on transition, not per poll. # The rollup rides this query rather than a REST call, so reading the checks costs no round-trip. # It is asked of the last commit because a rollup hangs off a commit object. # A case holds that commit equal to `headRefOid`, since a rollup a push ago still renders whole. -Q_FULL = """ -query($o:String!,$r:String!,$n:Int!){ - repository(owner:$o,name:$r){ pullRequest(number:$n){ - headRefOid baseRefName mergeable mergeStateStatus +HELD_FIELDS = """ + headRefOid baseRefName state mergeable mergeStateStatus baseRepository{ defaultBranchRef{ name } } reviews(last:100){ nodes{ id author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } - reviewThreads(first:100){ nodes{ id isResolved - comments(first:1){ nodes{ author{login} path line body fullDatabaseId pullRequestReview{ id } } } } pageInfo{ hasNextPage } } comments(last:100){ nodes{ author{login} authorAssociation createdAt body } pageInfo{ hasPreviousPage } } reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } - files(first:__FILES_WINDOW__){ pageInfo{ hasNextPage } nodes{ path } } commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state contexts(first:__CHECKS_WINDOW__){ pageInfo{ hasNextPage } nodes{ __typename - ... on CheckRun{ name status conclusion startedAt + ... on CheckRun{ name status conclusion startedAt isRequired(pullRequestNumber:$n) checkSuite{ databaseId app{ slug } workflowRun{ workflow{ databaseId } } } } - ... on StatusContext{ context state createdAt } + ... on StatusContext{ context state createdAt isRequired(pullRequestNumber:$n) } }}}}}} +""".replace("__CHECKS_WINDOW__", str(CHECKS_WINDOW)) +Q_HELD = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ __HELD_FIELDS__ }}} +""".replace("__HELD_FIELDS__", HELD_FIELDS) +Q_FULL = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ __HELD_FIELDS__ + reviewThreads(first:100){ nodes{ id isResolved + comments(first:1){ nodes{ author{login} path line body fullDatabaseId pullRequestReview{ id } } } } pageInfo{ hasNextPage } } + files(first:__FILES_WINDOW__){ pageInfo{ hasNextPage } nodes{ path } } }}} -""".replace("__CHECKS_WINDOW__", str(CHECKS_WINDOW)).replace("__FILES_WINDOW__", str(FILES_WINDOW)) +""".replace("__HELD_FIELDS__", HELD_FIELDS).replace("__FILES_WINDOW__", str(FILES_WINDOW)) # Substituted rather than interpolated, because GraphQL is braces from end to end. # An f-string would need every one of them doubled, which is unreadable against the schema. @@ -1304,7 +1319,7 @@ def threads_truncated(pr: dict) -> bool: open. That inversion is why this is its own guard rather than a second use of `window_blind`: that one settles the question from what is already in view, and there is no such settling available here, only the fact that something was cut, undercounted ever since `unresolved` - widened from Copilot's own threads to every tracked reviewer's. + widened from Copilot's own threads to every open thread. """ return bool(((pr.get("reviewThreads") or {}).get("pageInfo") or {}).get("hasNextPage")) @@ -2338,14 +2353,18 @@ def uncounted_verdict(pr: dict) -> tuple[str, str] | None: return None if suppressed_blocks(body) or previously_missed_blocks(body): return None - if open_reviewer_threads((pr.get("reviewThreads") or {}).get("nodes") or []): + if open_threads((pr.get("reviewThreads") or {}).get("nodes") or []): return None return verdict, headline -def open_reviewer_threads(threads: list[dict]) -> list[dict]: - """The unresolved threads a known reviewer opened, which block a ruleset-gated merge.""" - return [t for t in threads if not t.get("isResolved") and thread_author(t) in KNOWN_REVIEWERS] +def open_threads(threads: list[dict]) -> list[dict]: + """Every unresolved thread, whoever opened it, since each one blocks a ruleset-gated merge. + + The author decides only how `status` attributes a thread, never whether it counts, because a + login set read off past history misses the next spelling a reviewer posts under. + """ + return [t for t in threads if not t.get("isResolved")] def unlisted_findings(manifest: tuple[int | None, int] | None) -> int: @@ -3190,7 +3209,11 @@ def head_commit(pr: dict) -> dict: def check_nodes(pr: dict) -> list[dict]: - """The head commit's checks, each as {name, state, conclusion, since}. + """The head commit's checks, each a dict with `name`, `state`, `conclusion`, and `since`. + + A readable node also carries `required`, true unless the rollup marks it `isRequired: false`, + which `held_checks_open` and `wait`'s exit `44` decide on. A node of an unrecognized union + member carries empty values for the four and an `unreadable` key naming its type instead. A rollup carries two node shapes and they spell every field differently: a CheckRun has a `name`, a `status` and a `conclusion`, while a StatusContext has a `context` and a single @@ -3236,6 +3259,7 @@ def check_nodes(pr: dict) -> list[dict]: "state": n.get("status") or "", "conclusion": n.get("conclusion") or "", "since": n.get("startedAt") or "", + "required": n.get("isRequired") is not False, } ) elif n.get("__typename") == "StatusContext": @@ -3252,6 +3276,7 @@ def check_nodes(pr: dict) -> list[dict]: "state": "IN_PROGRESS" if state == "PENDING" else state, "conclusion": state, "since": n.get("createdAt") or "", + "required": n.get("isRequired") is not False, } ) else: @@ -3342,6 +3367,60 @@ def checks_stuck( ] +def checks_settling(nodes: list[dict], now: datetime, grace: float, stall: float) -> list[dict]: + """Every check still on its way to a conclusion that waiting can reach. + + A stuck check is left out, since its shape is one no wait clears and polling it only runs the + timeout out. RUNNING_LONG is the exception, since duration alone cannot tell a stalled job + from a slow one. Of the rest, a check is settling where it has not passed, which leaves the + state taxonomy to `check_shape` alone. + """ + return [ + n + for n in nodes + if not n.get("unreadable") + and check_shape(n, now, grace, stall) in ("", "RUNNING_LONG") + and (n.get("conclusion") or "") not in CHECK_OK + ] + + +def held_checks_open( + pr: dict, nodes: list[dict], now: datetime, grace: float, stall: float +) -> bool: + """True where a covered head's checks can still move its merge, so a held wait polls on. + + A merge reading CLEAN, UNSTABLE, or HAS_HOOKS has no required check outstanding, which is + GitHub's own reading, so on such a merge a check nothing requires does not hold the wait. A + BLOCKED merge carrying a stuck required check, other than one running long, has its exit, 44, + decided already, so nothing holds it. Otherwise a required check still settling holds it. So + does any check still settling while no required check has posted, since a required aggregator + behind `needs:` enters the rollup only once its dependencies finish. A rollup carrying no check + at all holds it while the merge reads BLOCKED, BEHIND, or DRAFT, since that is a push whose + check suites have not registered, where a conflicted, DIRTY one runs no workflow at all. A merge + reading UNKNOWN holds it whatever the rollup carries, since GitHub has not yet decided the word + the exit code is chosen by. A pull request no longer open has no merge left to move, and GitHub + reads UNKNOWN on it for good. + """ + if pr.get("state", "OPEN") != "OPEN": + return False + merge = pr.get("mergeStateStatus") + if merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS"): + return False + if merge == "UNKNOWN": + return True + if merge == "BLOCKED" and any( + n.get("required") and shape != "RUNNING_LONG" + for n, shape in checks_stuck(nodes, now, grace, stall) + ): + return False + settling = checks_settling(nodes, now, grace, stall) + if any(n.get("required") for n in settling): + return True + if settling and not any(n.get("required") for n in nodes): + return True + return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) + + def checks_truncated(pr: dict) -> bool: """True where the head's rollup carries more contexts than the query asked for. @@ -3385,10 +3464,24 @@ def checks_tally(nodes: list[dict]) -> tuple[int, int]: return sum(1 for n in read if (n.get("conclusion") or "") in CHECK_OK), len(read) -def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: - """Return (head_sha, copilot_reviewed_current_head, copilot_answer_outside_a_review).""" - pr = gql(Q_LIVE, owner, repo, num) - return pr["headRefOid"], reviewed_head(pr), answered_outside_review(pr) +def liveness(pr: dict, min_rounds: int = 0) -> tuple[bool, dict | None, list[str]]: + """Return (head_review_done, answered_outside_review, reviewer_login_drift) off one payload. + + These are the three readings `wait`'s Copilot poll stops on. Its first evaluation and every + re-read both come here, so a reading added to one cannot be missing from the other. A + drifted login matches no filter, so `head_review_done` stays false however long the wait + runs, and waiting it out reports a review that landed as one that never did. `Q_LIVE` + carries the authors, so the drift costs the poll no extra call. A `min_rounds` of 0 reads + as `reviewed_head`. + """ + return head_review_done(pr, min_rounds), answered_outside_review(pr), reviewer_login_drift(pr) + + +def live_state( + owner: str, repo: str, num: int, min_rounds: int = 0 +) -> tuple[bool, dict | None, list[str]]: + """Return `liveness` off one fresh `Q_LIVE`, since a push during the wait moves the head.""" + return liveness(gql(Q_LIVE, owner, repo, num), min_rounds) def heading_of(block: str) -> str: @@ -3794,20 +3887,15 @@ def digest( truncated = threads_truncated(pr) # Same reasoning, the `reviews` connection rather than `reviewThreads`, feeding `suppressed=` and `cr_outside_diff=` below. revs_truncated = reviews_truncated(pr) - # Any known reviewer's own thread, not only Copilot's. - # An open thread blocks a ruleset-gated merge whoever opened it, and counting Copilot's alone hid a CodeRabbit/qodo thread that did block one. - # `thread_author` carries the deleted-account default this needs. - unresolved = open_reviewer_threads(threads) - # A breakdown beside the raw count, but only where more than one reviewer contributes to it. - # A single reviewer's own count is what `unresolved=N` already meant before this generalized. - # Printing one name beside its own total says nothing the number did not already say. + unresolved = open_threads(threads) by_login = { login: sum(1 for t in unresolved if thread_author(t) == login) for login in KNOWN_REVIEWERS } + by_login["other"] = sum(1 for t in unresolved if thread_author(t) not in KNOWN_REVIEWERS) contributors = {login: n for login, n in by_login.items() if n} breakdown = ( " (" + " ".join(f"{login}={n}" for login, n in contributors.items()) + ")" - if len(contributors) > 1 + if len(contributors) > 1 or "other" in contributors else "" ) @@ -4379,7 +4467,7 @@ def unresolved_threads(owner: str, repo: str, num: int) -> list[dict]: conn = gh_graphql(Q_THREADS, o=owner, r=repo, n=num, **extra)["repository"]["pullRequest"][ "reviewThreads" ] - out += [t for t in conn["nodes"] if not t["isResolved"]] + out += open_threads(conn["nodes"]) page = conn.get("pageInfo") or {} if not page.get("hasNextPage"): return out @@ -4708,6 +4796,34 @@ def local_cover(pr: dict) -> bool: ) +POLL_DELAYS = (15, 20, 30, 45, 60, 120) + + +def backoff[Polled]( + value: Polled, + read: Callable[[], Polled], + keep: Callable[[Polled], bool], + start: float, + timeout: float, +) -> Polled: + """Re-read `value` on the `POLL_DELAYS` schedule while `keep` holds and `timeout` allows. + + `start` is a `time.monotonic` reading and `timeout` is in seconds. The bound is checked + before each sleep, so the poll can run past it by up to one delay. Returns the last value + read, or `value` itself where the poll never sleeps. + + One loop for both of `wait`'s polls, since a fix to the bound or the schedule applied to one + inline copy and not the other is a wait that times out differently depending on its arm. + The backoff runs in-process, so the whole wait costs one agent turn. + """ + i = 0 + while keep(value) and time.monotonic() - start <= timeout: + time.sleep(POLL_DELAYS[min(i, len(POLL_DELAYS) - 1)]) + i += 1 + value = read() + return value + + def reply_to_thread( owner: str, repo: str, @@ -5209,20 +5325,14 @@ def main(argv: list[str] | None = None) -> int: print(f"status=OUT_OF_SCOPE nothing was written: {why}") return 64 - # In-process backoff, so the whole wait costs one agent turn. - delays = [15, 20, 30, 45, 60, 120] start = time.monotonic() pr = gql(Q_LIVE, owner, repo, a.number) - done, answer = head_review_done(pr, a.min_rounds), answered_outside_review(pr) + done, answer, drift = liveness(pr, a.min_rounds) if done and a.ignore_quota_signal: full = gql(Q_FULL, owner, repo, a.number) if refusing_review(full) and not reviewed_head(full): a.min_rounds = max(a.min_rounds, len(reviewer_nodes(full, "reviews"))) done = False - # A drifted login matches no filter here, so `done` stays false however long this runs. - # Waiting it out reports a review that landed as one that never did, at the timeout. - # The liveness query carries the authors, so this costs the loop no extra call. - drift = reviewer_login_drift(pr) # Read whenever nothing has landed on this pull request yet, whether or not a request is already outstanding. # An already-pending request drawing no answer at all is exactly the shape a repo-wide quota exhaustion leaves, measured over the six consecutive pull requests that followed a refusal and carried no Copilot activity at all. # The bot id for a fresh request comes from this same traversal, so a caller needing either pays for one call rather than two. @@ -5258,7 +5368,7 @@ def main(argv: list[str] | None = None) -> int: and not reviewer_requested(pr) ): line, recorded = request_copilot_review( - owner, repo, a.number, pr["id"], copilot_bot_id(history), delays[0] + owner, repo, a.number, pr["id"], copilot_bot_id(history), POLL_DELAYS[0] ) if recorded is False: final = gql(Q_FULL, owner, repo, a.number) @@ -5278,8 +5388,36 @@ def main(argv: list[str] | None = None) -> int: "note: this pull request merges into a branch other than the default and Copilot " "has reviewed it already, so a fix push is covered by an attested local pass rather " "than another Copilot round, and this wait requests nothing. Pass --request to ask " - "for a round anyway." + "for a round anyway. An attested head has its checks polled instead, while they can " + "still move its merge." ) + + def still_held(polled: dict) -> bool: + return ( + holds(polled) + and local_cover(polled) + and not unrecognized_shapes(polled) + and held_checks_open( + polled, + check_nodes(polled), + datetime.now(UTC), + a.check_grace, + a.check_stall, + ) + ) + + assert snapshot is not None + last_full = [snapshot] + + def held_read() -> dict: + polled = gql(Q_HELD, owner, repo, a.number) + if still_held(polled): + return polled + last_full[0] = gql(Q_FULL, owner, repo, a.number) + return last_full[0] + + polled = backoff(last_full[0], held_read, still_held, start, a.timeout) + final = polled if polled is last_full[0] else None elif stopped and not reviewer_requested(pr): print( "note: this pull request's newest Copilot review is a refusal naming the account " @@ -5300,17 +5438,13 @@ def main(argv: list[str] | None = None) -> int: "--ignore-quota-signal to poll anyway, once the quota is believed to have reset." ) else: - i = 0 - while not done and not answer and not drift: - elapsed = time.monotonic() - start - if elapsed > a.timeout: - break - time.sleep(delays[min(i, len(delays) - 1)]) - i += 1 - # Re-read head each iteration: a push during the wait moves it. - pr = gql(Q_LIVE, owner, repo, a.number) - done, answer = head_review_done(pr, a.min_rounds), answered_outside_review(pr) - drift = reviewer_login_drift(pr) + backoff( + (done, answer, drift), + lambda: live_state(owner, repo, a.number, a.min_rounds), + lambda s: not any(s), + start, + a.timeout, + ) # One payload decides the digest and the exit code together. # Read separately, a review landing between them prints coverage and returns a timeout code. @@ -5358,30 +5492,35 @@ def main(argv: list[str] | None = None) -> int: # Only once the review itself is sound does a stuck required check decide the code. if verdict: return verdict + if covered and held_checks_open(final, checks, now, a.check_grace, a.check_stall): + print( + "status=CHECKS_PENDING an attested local pass covers this head, and by the " + "timeout a check had not concluded, none had posted, or the merge was still " + "UNKNOWN, so the merge is not ready yet: wait again, or read the checks above" + ) + return 30 # The review loop closing is not the merge gate, and 0 alone was saying it was. # A wait ends the moment coverage lands, which leaves the checks mid-flight nearly always. # So a merely pending check is not this code, or the code would be the usual outcome. # Only a shape no waiting clears earns it, which is what the stuck field already prints. # It is read from the same payload the digest was, so the two can never disagree. # A rollup carries checks the ruleset does not require, four of six on a green run here. - # So `BLOCKED` is required of the code as well, borrowing GitHub's own reading. - # That is cheaper than reading the ruleset's contexts over another call. - # Without it, a stuck check nothing requires returns 44 on a mergeable pull request. - # `CLEAN` proves no required gate is outstanding, whatever else the rollup is doing. - # The digest reports the check either way, so the narrower code costs the reader nothing. - if stuck and final.get("mergeStateStatus") == "BLOCKED": - # Worded as a coincidence rather than a cause. - # Nothing here proves the stuck check is what blocks the merge. + # So only a stuck check whose rollup node reads `isRequired` counts toward the code. + # Without that, a stuck optional check on a merge a thread blocks returns 44. + # `BLOCKED` is required as well, since `CLEAN` proves no required gate is outstanding. + # The digest reports every stuck check either way, so the narrower code costs nothing. + gates = [n.get("name") or "unnamed" for n, _ in stuck if n.get("required")] + if final.get("mergeStateStatus") == "BLOCKED" and gates: + # The block above prints optional stuck checks too, so the required ones are named. + # They need not be all that blocks it. # `BLOCKED` is also worn by an open thread or a missing approval. - # The rollup also carries checks no ruleset requires. - # So naming the check as the blocker would assert a link this cannot read. - # Both facts are true, and both are printed. print( "status=CHECKS_NOT_MERGEABLE the review loop is closed, the merge reads " - "BLOCKED, and a check is in a shape waiting does not clear: read the block " - "above, since a starved check wants a re-run, an unposted one its poster, a " - "long one a judgment, and a failed one a fix. Which of them gates the merge is " - "not read here, because BLOCKED is also worn by a thread or a missing approval" + "BLOCKED, and a required check is in a shape waiting does not clear, " + f"required stuck {', '.join(repr(g) for g in gates)}: read their lines in the " + "block above, since a starved check wants a re-run, an unposted one its poster, " + "a long one a judgment, and a failed one a fix. It need not be all that blocks " + "the merge, because BLOCKED is also worn by a thread or a missing approval" ) return 44 return 0 diff --git a/scripts/skills_install.py b/scripts/skills_install.py index 701ab942b..c4c8040ac 100755 --- a/scripts/skills_install.py +++ b/scripts/skills_install.py @@ -203,11 +203,11 @@ def claude_available(): return shutil.which("claude") is not None -def marketplace_entry(): - """This marketplace's entry in the CLI's listing, {} when it is not registered, None when there is no listing.""" +def claude_json(*args): + """The JSON list `claude ` prints, or None where it cannot be run or read as one.""" try: listing = subprocess.run( - ["claude", "plugin", "marketplace", "list", "--json"], + ["claude", *args], capture_output=True, text=True, encoding="utf-8", @@ -216,14 +216,44 @@ def marketplace_entry(): ) entries = json.loads(listing.stdout) if listing.returncode == 0 else None except (OSError, subprocess.TimeoutExpired, json.JSONDecodeError): - entries = None - if not isinstance(entries, list): + return None + return entries if isinstance(entries, list) else None + + +def marketplace_entry(): + """This marketplace's entry in the CLI's listing, {} when it is not registered, None when there is no listing.""" + entries = claude_json("plugin", "marketplace", "list", "--json") + if entries is None: return None return next( (e for e in entries if isinstance(e, dict) and e.get("name") == MARKETPLACE_NAME), {} ) +def plugin_state(): + """Whether the fleet plugin is installed and enabled at user scope, each None where unreadable. + + A registered marketplace serves nothing until its plugin is installed, and a plugin the user + later disabled or uninstalled leaves the marketplace registered, so the listing alone proves neither. + The installer installs at user scope, so a project or local install of the same id says nothing here. + """ + entries = claude_json("plugin", "list", "--json") + if entries is None: + return {"installed": None, "enabled": None} + plugin_id = f"{PLUGIN_NAME}@{MARKETPLACE_NAME}" + entry = next( + ( + e + for e in entries + if isinstance(e, dict) and e.get("id") == plugin_id and e.get("scope") == "user" + ), + None, + ) + if entry is None: + return {"installed": False, "enabled": False} + return {"installed": True, "enabled": entry.get("enabled") is True} + + def entry_location(entry): """The directory a listing entry names, or None where it names none.""" location = entry.get("installLocation") or entry.get("path") @@ -383,7 +413,18 @@ def intended_commit(rev=None): def live_channel(): - """What the Claude Code channel serves now, read from the checkout the marketplace names. + """What the Claude Code channel serves now, with whether its plugin is installed and enabled. + + A registered marketplace carries a `plugin` field, since the channel serves nothing otherwise. + """ + live = marketplace_channel() + if live.get("registered") is True: + live["plugin"] = plugin_state() + return live + + +def marketplace_channel(): + """What the registered marketplace serves now, read from the checkout the marketplace names. The checkout running this report is not necessarily the one registered, a worktree or a fresh clone being the ordinary cases, so the registered path is read back from the CLI. diff --git a/tests/test_local_review.py b/tests/test_local_review.py index a850e2723..b754a84bd 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -1249,8 +1249,8 @@ def fake_cli( script = bin_dir / "coderabbit" script.write_text(body, encoding="utf-8") script.chmod(0o755) - prev = os.environ.get("PATH", "") - os.environ["PATH"] = f"{bin_dir}{os.pathsep}{prev}" + prev = os.environ.get("PATH") + os.environ["PATH"] = f"{bin_dir}{os.pathsep}{prev if prev is not None else os.defpath}" self.addCleanup(self.restore_env, "PATH", prev) def test_a_completed_run_counts_findings(self) -> None: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index bbcb6ab3c..9cc09d890 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -49,6 +49,15 @@ def flowed_text(text: str) -> str: return " ".join(text.split()) +def comment_flowed_text(text: str) -> str: + """Strip each line's leading `#` marker, then collapse as `flowed_text` does. + + A `#` comment that rewraps gains a marker on its new line as well as a newline, so + collapsing whitespace alone leaves the marker between two words of a pinned phrase. + """ + return flowed_text(" ".join(line.lstrip().removeprefix("#") for line in text.splitlines())) + + HEAD = "a" * 40 OLD = "b" * 40 @@ -543,12 +552,22 @@ def test_the_review_must_be_the_reviewer_and_on_the_current_head(self) -> None: ): with self.subTest(case=label): self.answer(payload(reviews)) - self.assertEqual((HEAD, want, None), pr_review.live_state("o", "r", 1)) + self.assertEqual((want, None, []), pr_review.live_state("o", "r", 1)) def test_a_null_author_or_commit_does_not_raise(self) -> None: """GraphQL returns null for a deleted account, and a crash there stalls the whole wait.""" self.answer(payload([{"author": None, "state": "COMMENTED", "commit": None}])) - self.assertEqual((HEAD, False, None), pr_review.live_state("o", "r", 1)) + self.assertEqual((False, None, []), pr_review.live_state("o", "r", 1)) + + def test_a_poll_reads_the_same_three_readings_its_payload_gives(self) -> None: + for label, pr in ( + ("on head", payload([review()])), + ("stale", payload([review(oid=OLD)])), + ("answered outside", payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])), + ): + with self.subTest(case=label): + self.answer(pr) + self.assertEqual(pr_review.liveness(pr), pr_review.live_state("o", "r", 1)) class TestAnsweredOutsideReview(unittest.TestCase): @@ -736,21 +755,26 @@ def test_digest_reports_effective_effort_without_changing_the_verdict(self) -> N self.assertIn("coverage=full", out) def test_a_thread_from_a_deleted_account_does_not_crash_the_digest(self) -> None: - """GraphQL sends `author` present and null, which a defaulted lookup returns as None.""" + """GraphQL sends `author` present and null, which a defaulted lookup returns as None. + + The thread still blocks the merge, so it counts, attributed as `other`. + """ orphan = thread("T1") orphan["comments"]["nodes"][0]["author"] = None self.answer(payload([review()], [orphan, thread("T2")])) out, unresolved = pr_review.digest("o", "r", 7) - self.assertEqual(1, unresolved) + self.assertEqual(2, unresolved) + self.assertIn(f"unresolved=2 ({pr_review.REVIEWER}=1 other=1)", out) + self.assertIn("T1", out) self.assertIn("T2", out) - def test_only_the_reviewer_s_own_unresolved_threads_are_listed(self) -> None: - """A maintainer's own open thread is not a review finding to answer.""" + def test_every_open_thread_is_listed_whoever_opened_it(self) -> None: + """An open thread blocks a ruleset-gated merge whatever its author.""" self.answer(payload([review()], [thread("T1", login="ptr727"), thread("T2")])) out, unresolved = pr_review.digest("o", "r", 7) - self.assertEqual(1, unresolved) + self.assertEqual(2, unresolved) + self.assertIn("T1", out) self.assertIn("T2", out) - self.assertNotIn("T1", out) def test_a_seen_set_marks_each_thread_new_exactly_once(self) -> None: """The seen set is what tells a second round's findings from the ones already answered.""" @@ -784,10 +808,11 @@ class TestOtherReviewers(GqlCase): in `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below, not here. `status`'s `unresolved=0` used to hide a CodeRabbit/qodo thread that still blocked a - ruleset-gated merge, since only Copilot's own threads - counted. Coverage and refusal reading stay Copilot-only: `review_on_head` above names - Copilot's own coverage specifically, the reviewer this script requests and waits for, not - "no review of any kind covers this head". + ruleset-gated merge, first since only Copilot's own threads counted and then since only known + reviewer logins did, so every open thread now counts and a login only attributes it. Coverage + and refusal reading stay Copilot-only: `review_on_head` above names Copilot's own coverage + specifically, the reviewer this script requests and waits for, not "no review of any kind + covers this head". """ def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict: @@ -818,14 +843,34 @@ def test_a_coderabbit_and_a_qodo_thread_both_count_toward_unresolved(self) -> No self.assertIn("T2", out) self.assertNotIn("T3", out) - def test_a_human_thread_still_does_not_count(self) -> None: - """Generalizing past Copilot means the other tracked bots, not every account.""" - self.answer(payload([review()], [thread("T1", login="ptr727")])) + def test_a_thread_from_an_unknown_login_counts_and_is_named_other(self) -> None: + """A reviewer posting under a login missing from the known set once read `unresolved=0`. + + The known logins attribute a thread and never decide whether it counts, and `other` is + named even where it is the only contributor, since it is the count a reader cannot assume. + """ + self.answer(payload([review()], [thread("T1", login="example-review-bot")])) out, unresolved = pr_review.digest("o", "r", 7) - self.assertEqual(0, unresolved) - self.assertNotIn("T1", out) + self.assertEqual(1, unresolved) + self.assertIn("unresolved=1 (other=1)", out) + self.assertIn("T1", out) + + def test_status_and_wait_print_an_unknown_login_s_thread_in_the_digest(self) -> None: + """`status` and `wait` print the digest a Merge Gate decision is read from.""" + unknown = thread("T1", login="example-review-bot") + for command in (["status", "7"], ["wait", "7"]): + with self.subTest(command=command[0]): + self.answer(payload([review()], [unknown])) + out = io.StringIO() + with ( + contextlib.redirect_stdout(out), + mock.patch.object(pr_review.time, "sleep"), + ): + pr_review.main([*command, "--repo", "o/r"]) + self.assertIn("unresolved=1 (other=1)", out.getvalue()) + self.assertIn("T1", out.getvalue()) - def test_the_breakdown_appears_only_once_more_than_one_reviewer_contributes(self) -> None: + def test_one_known_reviewer_alone_prints_no_breakdown_and_two_print_one(self) -> None: self.answer(payload([review()], [thread("T1"), thread("T2")])) out, unresolved = pr_review.digest("o", "r", 7) self.assertEqual(2, unresolved) @@ -4258,9 +4303,19 @@ def test_every_emphasis_tag_the_stripper_names_is_dropped(self) -> None: self.assertEqual("Open (N)", pr_review.normal(f"<{tag}>Open (2)")) self.assertEqual("Open (N)", pr_review.normal("Open (2)")) + def test_a_phrase_rewrapped_across_comment_lines_reads_as_one_sentence(self) -> None: + """Collapsing whitespace alone would leave the second line's marker inside the phrase.""" + wrapped = ( + "x = 1\n # the output is regular: 10 headings, 10\n # summaries and 4 labels.\n" + ) + self.assertIn( + "the output is regular: 10 headings, 10 summaries and 4 labels.", + comment_flowed_text(wrapped), + ) + def test_the_vetted_lists_hold_what_the_comment_beside_them_counts(self) -> None: """The comment states the sizes, and adding an entry without it is how it goes stale.""" - source = Path(pr_review.__file__).read_text(encoding="utf-8") + source = comment_flowed_text(Path(pr_review.__file__).read_text(encoding="utf-8")) stated = re.search( r"the output is regular: (\d+) headings, (\d+) summaries and (\d+) labels", source ) @@ -4678,6 +4733,8 @@ def test_an_open_thread_defers_the_reading_until_it_is_resolved(self) -> None: """The headline may name that thread, which the loop already waits on.""" body = self.headline() self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=body)], [thread("T1")]))) + unknown = thread("T1", login="example-review-bot") + self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=body)], [unknown]))) resolved = payload([review(body=body)], [thread("T1", resolved=True)]) self.assertIsNotNone(pr_review.uncounted_verdict(resolved)) @@ -6035,6 +6092,18 @@ def test_the_rollup_query_asks_for_the_suite_and_workflow_identity(self) -> None "checkSuite{ databaseId app{ slug } workflowRun{ workflow{ databaseId } } }", query ) + def test_the_full_query_asks_which_checks_are_required_and_whether_the_pr_is_open(self) -> None: + """A dropped field reads as required or open, which holds a held wait to its timeout.""" + query = " ".join(pr_review.Q_FULL.split()) + self.assertIn("headRefOid baseRefName state mergeable", query) + self.assertIn("conclusion startedAt isRequired(pullRequestNumber:$n)", query) + self.assertIn("context state createdAt isRequired(pullRequestNumber:$n)", query) + + def test_a_status_context_reads_its_own_required_flag(self) -> None: + optional = {**status_context("ci/external", state="PENDING"), "isRequired": False} + nodes = pr_review.check_nodes(payload([review()], checks=[optional, check()])) + self.assertEqual([False, True], [n["required"] for n in nodes]) + def test_a_check_run_with_no_suite_does_not_crash(self) -> None: """A node missing its suite reads as suite zero under no workflow and no app.""" bare = check(name="lint") @@ -6367,13 +6436,12 @@ def test_wait_exits_forty_four_where_the_review_closed_but_a_check_is_starved(se self.assertIn("status=CHECKS_NOT_MERGEABLE", out) self.assertIn("CHECK NOT PICKED UP", out) - def test_a_stuck_check_nothing_requires_does_not_take_forty_four(self) -> None: - """A rollup carries checks the ruleset does not require, four of six on a green run here. + def test_a_stuck_check_on_a_clean_merge_does_not_take_forty_four(self) -> None: + """`CLEAN` proves no required gate is outstanding, whatever else the rollup is doing. - So the code borrows GitHub's own reading of which checks gate a merge, and `CLEAN` proves - no required gate is outstanding whatever else the rollup is doing. Without that, a stuck - check nothing requires returns 44 on a mergeable pull request. Raised in review on this - change. The digest still names the check, so the narrower code costs the reader nothing. + The node carries no `isRequired`, which reads as required, so this pins the `BLOCKED` + half of the condition. The digest still names the check, so the narrower code costs the + reader nothing. """ self.answer( payload( @@ -6392,6 +6460,42 @@ def test_a_stuck_check_nothing_requires_does_not_take_forty_four(self) -> None: self.assertIn("stuck=NOT_PICKED_UP", out) self.assertNotIn("status=CHECKS_NOT_MERGEABLE", out) + def test_only_a_required_stuck_check_on_a_blocked_merge_takes_forty_four(self) -> None: + """`BLOCKED` is also worn by an open thread, so it cannot say a stuck check is required. + + The rollup's `isRequired` can, so a failed check it marks optional exits 0 on a merge + blocked for another reason, and the same check marked required still exits 44. + """ + for required, code in ((False, 0), (True, 44)): + with self.subTest(required=required): + self.out.seek(0) + self.out.truncate() + failed = {**check(name="scan", conclusion="FAILURE"), "isRequired": required} + self.answer(payload([review()], merge="BLOCKED", checks=[failed])) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(code, self.cli(["wait", "7"])) + out = self.out.getvalue() + self.assertIn("stuck=FAILED", out) + self.assertEqual(code == 44, "status=CHECKS_NOT_MERGEABLE" in out) + + def test_the_forty_four_line_names_only_the_required_stuck_check(self) -> None: + """The block above prints every stuck check, so the 44 line says which one is required. + + Otherwise a reader fixes the optional failure the block lists first and meets 44 again. + """ + optional = {**check(name="scan", conclusion="FAILURE"), "isRequired": False} + gate = {**check(name="gate", conclusion="FAILURE", suite=2), "isRequired": True} + self.answer(payload([review()], merge="BLOCKED", checks=[optional, gate])) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(44, self.cli(["wait", "7"])) + status = next( + line + for line in self.out.getvalue().splitlines() + if line.startswith("status=CHECKS_NOT_MERGEABLE") + ) + self.assertIn("required stuck 'gate':", status) + self.assertNotIn("'scan'", status) + def test_wait_exits_zero_where_a_check_is_merely_still_running(self) -> None: """A code that fires on every pull request mid-CI carries nothing, so this must be 0. @@ -7055,6 +7159,19 @@ def bodyless(self, oid: str) -> dict: """A liveness payload, whose reviews carry no body, as `Q_LIVE` asks for none.""" return payload([{k: v for k, v in review(oid=oid).items() if k != "body"}]) + def test_the_first_stop_reading_and_every_poll_share_one_helper(self) -> None: + """A stop reading added to one site and not the other splits the first decision off.""" + self.answer( + payload([review(oid=OLD)], pending=True), + payload([review(oid=OLD)], pending=True), + payload([review(oid=OLD), review(rid="PRR_new")]), + ) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + spy = self.enterContext(mock.patch.object(pr_review, "liveness", wraps=pr_review.liveness)) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + self.assertEqual(slept.call_count + 1, spy.call_count) + def test_a_pending_request_past_an_error_round_is_still_polled_for(self) -> None: """The stop withholds a request, and a review already on its way still lands.""" self.answer( @@ -7146,6 +7263,376 @@ def test_an_attested_fix_push_closes_the_wait(self) -> None: self.assertIn("review_on_head=local", out) self.assertIn("coverage=local", out) + def test_an_attested_head_with_a_check_running_at_the_timeout_is_pending(self) -> None: + """Covered at once, the wait ended while CI ran and exit 0 read as a mergeable head.""" + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True) + self.answer(pr) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("coverage=local", out) + self.assertIn("status=CHECKS_PENDING", out) + + def test_an_attested_head_polls_its_checks_until_they_settle(self) -> None: + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + pending = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True + ) + green = self.into(payload([review(oid=OLD)], checks=[check()]), attest=True) + self.answer(pending, pending, green) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_called_once() + self.assertNotIn("CHECKS_PENDING", self.out.getvalue()) + + def test_a_stuck_check_on_an_attested_head_ends_the_poll(self) -> None: + """A shape no wait clears is 44 at once, rather than the timeout polled out against it.""" + starved = check(status="QUEUED", conclusion="", started=real_ago(900)) + pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[starved]), attest=True) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(44, self.cli(["wait", "7"])) + slept.assert_not_called() + + def test_a_failed_check_ends_the_poll_while_another_still_runs(self) -> None: + """A BLOCKED merge carrying a stuck check is the 44 the review path returns too.""" + running = check(name="test", status="IN_PROGRESS", conclusion="", started=real_ago(60)) + failed = check(name="lint", conclusion="FAILURE", started=real_ago(60)) + pr = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[failed, running]), attest=True + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(44, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_an_optional_failure_does_not_end_the_poll_on_a_required_check(self) -> None: + optional = {**check(name="lint", conclusion="FAILURE"), "isRequired": False} + gate = check(name="gate", status="IN_PROGRESS", conclusion="", started=real_ago(60)) + blocked = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[optional, gate]), attest=True + ) + passed = self.into( + payload([review(oid=OLD)], merge="UNSTABLE", checks=[optional, check(name="gate")]), + attest=True, + ) + self.answer(blocked, blocked, passed) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_called_once() + + def test_a_required_check_running_at_the_timeout_outranks_an_optional_failure(self) -> None: + """Waiting can still clear the merge, so the code is 30 rather than 44.""" + optional = {**check(name="coverage", conclusion="FAILURE"), "isRequired": False} + gate = check(name="gate", status="IN_PROGRESS", conclusion="", started=real_ago(60)) + pr = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[optional, gate]), attest=True + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: + """A conflicted pull request runs no `pull_request` workflow, so none is coming.""" + for merge, code in ( + ("BLOCKED", 30), + ("BEHIND", 30), + ("DRAFT", 30), + ("UNKNOWN", 30), + ("DIRTY", 0), + ): + with self.subTest(merge=merge): + pr = self.into(payload([review(oid=OLD)], merge=merge, checks=[]), attest=True) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(code, self.cli(["wait", "7", "--timeout", "0"])) + + def test_an_optional_check_does_not_hold_once_the_required_gate_concluded(self) -> None: + """A merge BLOCKED on a thread is not waiting on a check nothing requires.""" + optional = { + **check(name="coverage", status="IN_PROGRESS", conclusion="", started=real_ago(60)), + "isRequired": False, + } + pr = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[check(name="gate"), optional]), + attest=True, + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_optional_checks_hold_until_a_required_one_posts(self) -> None: + """A required aggregator behind `needs:` is absent until the jobs it waits on finish.""" + optional = { + **check(name="lint", status="IN_PROGRESS", conclusion="", started=real_ago(60)), + "isRequired": False, + } + pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[optional]), attest=True) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_an_unattested_held_head_is_not_polled_for_its_checks(self) -> None: + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + self.answer(self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]))) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(49, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_a_check_nothing_requires_does_not_hold_an_attested_head(self) -> None: + """A merge reading CLEAN, UNSTABLE, or HAS_HOOKS has no required check outstanding.""" + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + for merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS"): + with self.subTest(merge=merge): + pr = self.into( + payload([review(oid=OLD)], merge=merge, checks=[running]), attest=True + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + slept.assert_not_called() + + def test_the_held_poll_reads_the_narrow_query_and_the_verdict_the_full_one(self) -> None: + """The digest needs the threads and files the poll leaves out, so it reads `Q_FULL`. + + A poll that never sleeps grades its first read, so the narrow query costs it no call. + """ + empty = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[]), attest=True) + green = self.into(payload([review(oid=OLD)], checks=[check()]), attest=True) + self.answer(empty, empty, green) + gql = self.enterContext(mock.patch.object(pr_review, "gql", side_effect=pr_review.gql)) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7"])) + queries = [c.args[0] for c in gql.call_args_list] + self.assertEqual( + [pr_review.Q_LIVE, pr_review.Q_FULL, pr_review.Q_HELD, pr_review.Q_FULL], + queries, + ) + self.out.seek(0) + self.out.truncate() + self.answer(green) + gql = self.enterContext(mock.patch.object(pr_review, "gql", side_effect=pr_review.gql)) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_not_called() + self.assertEqual( + [pr_review.Q_LIVE, pr_review.Q_FULL], [c.args[0] for c in gql.call_args_list] + ) + + def test_a_held_poll_stops_only_on_a_full_read_that_agrees(self) -> None: + """A narrow read ending the poll is confirmed by the full read the verdict grades. + + A merge reading UNKNOWN on that full read keeps the poll going, rather than returning 30 + with time left on a payload the poll never judged. + """ + empty = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[]), attest=True) + green = self.into(payload([review(oid=OLD)], checks=[check()]), attest=True) + unknown = self.into( + payload([review(oid=OLD)], merge="UNKNOWN", checks=[check()]), attest=True + ) + self.answer(empty, empty, green, unknown, green) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + self.assertEqual(2, slept.call_count) + + def test_a_held_poll_timing_out_on_a_narrow_read_grades_a_full_one(self) -> None: + """`Q_HELD` carries no threads or files, so the verdict at the timeout reads `Q_FULL`.""" + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + held = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True) + self.answer(held) + served = pr_review.gql + + def narrow(query: str, owner: str, repo: str, num: int) -> dict: + pr = served(query, owner, repo, num) + if query is pr_review.Q_HELD: + return {k: v for k, v in pr.items() if k not in ("reviewThreads", "files")} + return pr + + self.enterContext(mock.patch.object(pr_review, "gql", side_effect=narrow)) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + clock = iter([0.0, 0.0]) + with ( + mock.patch.object(pr_review.time, "monotonic", side_effect=lambda: next(clock, 1e9)), + mock.patch.object(pr_review.time, "sleep") as slept, + ): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "60"])) + slept.assert_called_once() + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_an_attested_head_with_no_checks_registered_yet_is_polled(self) -> None: + """A wait run right after the push reads an empty rollup, which is CI not started.""" + empty = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[]), attest=True) + green = self.into(payload([review(oid=OLD)], checks=[check()]), attest=True) + self.answer(empty, empty, green) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_called_once() + self.out.seek(0) + self.out.truncate() + self.answer(empty) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_a_review_requested_during_a_held_wait_ends_the_poll(self) -> None: + """A requested round is no longer held, so its checks are not what the wait is for.""" + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + held = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True) + requested = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[running], pending=True), + attest=True, + ) + self.answer(held, held, requested) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_called_once() + self.assertNotIn("CHECKS_PENDING", self.out.getvalue()) + self.assertIn("status=PENDING", self.out.getvalue()) + + def test_a_quota_reading_outranks_pending_after_a_mid_poll_request(self) -> None: + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + held = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True) + requested = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[running], pending=True), + attest=True, + ) + self.answer(held, held, requested) + self.wire_history([hist_review(962, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(47, self.cli(["wait", "7", "--timeout", "1"])) + self.assertNotIn("status=PENDING", self.out.getvalue()) + + def test_an_unrecognized_shape_ends_the_held_poll_at_once(self) -> None: + """The 43 outranks every check reading, so polling the checks cannot change it.""" + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + odd = review(oid=OLD, body=OVERVIEW + "\n### Confidence assessment\n") + pr = self.into(payload([odd], merge="BLOCKED", checks=[running]), attest=True) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(43, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_a_required_check_running_long_holds_the_poll_rather_than_ending_it(self) -> None: + """Duration alone cannot tell a stalled job from a slow one, so waiting may clear it.""" + slow = check(name="gate", status="IN_PROGRESS", conclusion="", started=real_ago(2000)) + pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[slow]), attest=True) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_an_unreadable_rollup_is_not_held_as_one_not_yet_registered(self) -> None: + """A rollup read off another commit is empty here, and waiting does not clear that.""" + pr = payload([review(oid=OLD)], merge="BLOCKED", checks=[check()], rollup_oid="d" * 40) + self.answer(self.into(pr, attest=True)) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_an_outside_answer_outranks_pending_after_a_mid_poll_request(self) -> None: + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + rounds = payload( + [review(oid=OLD, at=EARLY)], + merge="BLOCKED", + checks=[running], + comments=[comment(at=LATE)], + ) + held = self.into(rounds, attest=True) + held["comments"]["nodes"].append(comment(at=LATE)) + requested = {**held, "reviewRequests": payload([], pending=True)["reviewRequests"]} + self.answer(held, held, requested) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(40, self.cli(["wait", "7", "--timeout", "1"])) + self.assertNotIn("status=PENDING", self.out.getvalue()) + + def test_a_merged_pull_request_is_not_held_on_its_unknown_merge(self) -> None: + """GitHub reads UNKNOWN on a merged pull request for good, so holding it never ends.""" + pr = self.into(payload([review(oid=OLD)], merge="UNKNOWN", checks=[check()]), attest=True) + self.answer({**pr, "state": "MERGED"}) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + + def test_a_push_during_the_held_poll_ends_it_on_the_unattested_head(self) -> None: + running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) + held = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True) + moved = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[running]), attest=True + ) + moved["headRefOid"] = "c" * 40 + self.answer(held, held, moved) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(49, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_called_once() + + def test_an_unknown_merge_holds_the_poll_over_a_failed_required_check(self) -> None: + """GitHub recomputes the word after a check concludes, and the code is chosen by it.""" + failed = check(name="gate", conclusion="FAILURE", started=real_ago(60)) + unknown = self.into( + payload([review(oid=OLD)], merge="UNKNOWN", checks=[failed]), attest=True + ) + blocked = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[failed]), attest=True + ) + self.answer(unknown, unknown, blocked) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(44, self.cli(["wait", "7"])) + slept.assert_called_once() + + def test_settling_is_a_check_waiting_can_still_conclude(self) -> None: + nodes = pr_review.check_nodes( + payload( + [review()], + checks=[ + check(name="running", status="IN_PROGRESS", conclusion=""), + check(name="queued", status="QUEUED", conclusion=""), + check(name="concluding", status="COMPLETED", conclusion=""), + check(name="starved", status="QUEUED", conclusion="", started=ago(900)), + check(name="long", status="IN_PROGRESS", conclusion="", started=ago(2000)), + check(name="passed"), + check(name="failed", conclusion="FAILURE"), + status_context("ci/building", state="PENDING"), + status_context("ci/expected", state="EXPECTED"), + status_context("ci/done"), + {"__typename": "UnknownContext"}, + ], + ) + ) + settling = pr_review.checks_settling(nodes, NOW, 300, 1800) + self.assertEqual( + ["running", "queued", "concluding", "long", "ci/building", "ci/expected"], + [n["name"] for n in settling], + ) + def test_request_asks_for_a_round_on_a_fix_push(self) -> None: self.answer(self.into(payload([review(oid=OLD)]))) calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) @@ -7202,12 +7689,15 @@ def test_a_pending_request_on_a_fix_push_is_polled_for(self) -> None: self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) - def test_a_push_during_a_held_wait_grades_the_new_head(self) -> None: - """An attestation of the head read first does not cover the head the verdict reads.""" + def test_a_hold_decided_on_a_pushed_unattested_head_grades_that_head(self) -> None: + """A push before the snapshot read moves the head the hold is decided for. + + No local pass attests the moved head, so the verdict is 49 and nothing is requested. + """ first = self.into(payload([review(oid=OLD)]), attest=True) moved = self.into(payload([review(oid=OLD)]), attest=True) moved["headRefOid"] = "c" * 40 - self.answer(first, first, moved) + self.answer(first, moved) calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(49, self.cli(["wait", "7", "--timeout", "0"])) @@ -8350,7 +8840,7 @@ def test_an_archive_that_is_not_a_readable_tarball_is_undecided(self) -> None: def test_a_ref_is_matched_as_bytes_so_an_undecodable_file_is_still_searched(self) -> None: """Skipping a file this cannot decode is how a present ref reads as absent.""" source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8") - self.assertIn("n in blob", source) + self.assertIn("n in blob", flowed_text(source)) def test_an_absent_status_is_absence_and_a_rate_limit_is_not(self) -> None: """Reading a 403 as a missing commit reports a correct description as contradicting itself.""" @@ -8618,14 +9108,53 @@ def test_an_inverted_pair_of_check_thresholds_is_rejected_at_the_flags_too(self) def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later.""" - source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8") - delays = [ - int(n) for n in source.split("delays = [")[1].split("]")[0].replace(" ", "").split(",") - ] + delays = list(pr_review.POLL_DELAYS) self.assertGreaterEqual(len(delays), 3) self.assertTrue(all(d > 0 for d in delays)) self.assertEqual(delays, sorted(delays)) + def test_the_backoff_sleeps_the_schedule_until_the_timeout(self) -> None: + """The last delay repeats, and the bound is read before each sleep rather than after. + + Six sleeps reach 290 seconds and a seventh 410, so with a 400-second bound the eighth is + never taken. + """ + clock = [0.0] + slept: list[int] = [] + + def sleep(seconds: int) -> None: + slept.append(seconds) + clock[0] += seconds + + reads = iter(range(1, 100)) + with ( + mock.patch.object(pr_review.time, "monotonic", side_effect=lambda: clock[0]), + mock.patch.object(pr_review.time, "sleep", side_effect=sleep), + ): + last = pr_review.backoff(0, lambda: next(reads), lambda _: True, 0.0, 400) + self.assertEqual([15, 20, 30, 45, 60, 120, 120], slept) + self.assertEqual(7, last) + + def test_the_backoff_returns_the_first_value_it_does_not_keep(self) -> None: + """A met condition ends the poll without a sleep, and a later one ends it on that read.""" + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual("done", pr_review.backoff("done", str, lambda v: v != "done", 0.0, 9)) + slept.assert_not_called() + reads = iter(["more", "done", "never"]) + with mock.patch.object(pr_review.time, "sleep") as slept: + last = pr_review.backoff("start", lambda: next(reads), lambda v: v != "done", 0.0, 1e9) + self.assertEqual("done", last) + self.assertEqual([mock.call(15), mock.call(20)], slept.call_args_list) + + def test_the_held_poll_reads_neither_threads_nor_files(self) -> None: + """The poll re-reads every iteration, and only the final digest read needs the two.""" + poll = " ".join(pr_review.Q_HELD.split()) + self.assertNotIn("reviewThreads", poll) + self.assertNotIn("files(", poll) + self.assertIn("headRefOid baseRefName state mergeable mergeStateStatus", poll) + self.assertIn("conclusion startedAt isRequired(pullRequestNumber:$n)", poll) + self.assertIn("authorAssociation createdAt body", poll) + class TestScopeRefusalNamesTheDirectoryItProbed(unittest.TestCase): """These refusals name the directory probed, the reader's own being a different one.""" diff --git a/tests/test_prose_lint.py b/tests/test_prose_lint.py index 5af72664c..a9bcfcb49 100755 --- a/tests/test_prose_lint.py +++ b/tests/test_prose_lint.py @@ -604,13 +604,17 @@ def test_a_series_in_one_sentence_does_not_exempt_the_next(self) -> None: ), ) - def test_a_colon_introduced_list_whose_items_carry_commas_keeps_its_semicolon(self) -> None: - """The colon arm earns its place: dropping it flagged this, the use the rule names. + def test_a_colon_that_explains_does_not_exempt_a_lone_semicolon(self) -> None: + """A colon and a comma once exempted any one semicolon after them, joining two clauses. - Measured over the tree, dropping it reported 14 further lines, and the shapes below are - what they were, so the arm is scoped rather than removed. + No structure tells an explanatory colon from a list colon, so a lone semicolon needs a + labeled item on each side of it. The colon arm let all four through. """ for text in ( + ( + "The gate runs once, at merge: it reads the head, the base, and the label; " + "the result stays on the pull request.\n" + ), ( "Match the heading style: title case with short bind words (a, an, the, of); " "hyphenated compounds capitalize both parts.\n" @@ -619,10 +623,47 @@ def test_a_colon_introduced_list_whose_items_carry_commas_keeps_its_semicolon(se "- **Python** (the script profile): lint, format, and type check; " "format-on-save and import organization via the formatter.\n" ), + ( + "The gate runs once, at merge: it reads, checks, and records the label; " + "outputs: d and e.\n" + ), ): with self.subTest(text=text.split(":")[0]): + self.assertEqual(["semicolon"], self.kinds(text, {"semicolon"})) + + def test_a_lone_semicolon_between_labeled_items_keeps_its_place(self) -> None: + """A label marks a two-item list, in emphasis or code, or as a dotted or slashed name.""" + for text in ( + "Inputs: a, b, and c; **Outputs**: d and e.\n", + "Inputs: a, b, and c; **Outputs:** d and e.\n", + "Inputs: a, b, and c; `out` paths: d and e.\n", + "Guarantees: D1.1 holds, always; D1.2: the gate runs.\n", + "Inputs: a, b, and c; read/write paths: d and e.\n", + ): + with self.subTest(text=text.strip()): self.assertEqual([], self.kinds(text, {"semicolon"})) + def test_a_list_marker_is_not_part_of_the_first_label(self) -> None: + """A plain label opening a list item labels it, whichever marker opens the item.""" + for marker in ("-", "*", "+", "1.", "1)", "> -"): + with self.subTest(marker=marker): + self.assertEqual( + [], self.kinds(f"{marker} Inputs: a, b; outputs: c and d.\n", {"semicolon"}) + ) + + def test_a_lone_semicolon_needs_a_label_on_each_side(self) -> None: + """A labeled pair still needs a colon and a comma, and these shapes are not labels.""" + for text in ( + "It runs on push, always; outputs: d and e.\n", + "Inputs: a; outputs: b.\n", + "Inputs: a, b, and c; the four word label: d and e.\n", + 'Inputs: a, b; it says "stop": the run halts.\n', + "Inputs: a, b; e.g. this one: the run halts.\n", + "Inputs: a, b; see /tmp: the run halts.\n", + ): + with self.subTest(text=text.strip()): + self.assertEqual(["semicolon"], self.kinds(text, {"semicolon"})) + def test_a_bullet_label_colon_inside_the_emphasis_is_the_same_opener(self) -> None: """`- **D3:**` and `- **D3**:` are one construct, and only one spelling was stripped.""" self.assertEqual( @@ -669,7 +710,7 @@ def test_an_abbreviation_does_not_end_a_sentence(self) -> None: self.assertEqual( [], self.kinds( - "Pinned by path: a script, a hook (e.g. a shebang); vanilla files stay as they are.\n", + "Pinned by path: a script; a hook (e.g. a shebang); a config, as written.\n", {"semicolon"}, ), ) diff --git a/tests/test_repo_gate.py b/tests/test_repo_gate.py index 3421e9bf8..c3547f869 100755 --- a/tests/test_repo_gate.py +++ b/tests/test_repo_gate.py @@ -66,10 +66,12 @@ def test_the_table_holds_every_check_the_cli_offers(self) -> None: self.assertGreaterEqual(len(repo_gate.CHECKS), 3) def test_the_patterns_compile_and_carry_no_invisible_characters(self) -> None: - """A shell heredoc turns a backslash escape into a control character no diff shows.""" + """printf, echo -e, and $'...' turn an escape into a control character no diff shows.""" for name in ("USES", "PIN", "WORKFLOW", "HTTP_STATUS"): with self.subTest(pattern=name): - self.assertTrue(re.compile(getattr(repo_gate, name).pattern)) + pattern = getattr(repo_gate, name).pattern + self.assertTrue(re.compile(pattern)) + self.assertTrue(pattern.isascii() and pattern.isprintable()) for action in repo_gate.SHA_EXCEPTIONS: with self.subTest(exception=action): self.assertTrue(action.isascii() and action.isprintable()) diff --git a/tests/test_skills_install.py b/tests/test_skills_install.py index 03e8bbdbc..7381a5fe0 100755 --- a/tests/test_skills_install.py +++ b/tests/test_skills_install.py @@ -484,6 +484,10 @@ class LiveChannelCase(unittest.TestCase): def setUp(self) -> None: self.addCleanup(mock.patch.stopall) mock.patch("skills_install.claude_available", return_value=True).start() + mock.patch( + "skills_install.plugin_state", + return_value={"installed": True, "enabled": True}, + ).start() def listing(self, stdout: str, returncode: int = 0) -> None: result = mock.Mock(returncode=returncode, stdout=stdout) @@ -607,6 +611,7 @@ def fake(root, *args): "branch": "develop", "commit": "dev", "dirty": False, + "plugin": {"installed": True, "enabled": True}, }, ) self.assertEqual(set(roots), {Path("/hub/checkout")}) @@ -668,6 +673,7 @@ def test_a_tree_the_bootstrap_keeps_reads_its_commit_marker_rather_than_git(self "branch": None, "commit": "cafe1234", "dirty": None, + "plugin": {"installed": True, "enabled": True}, }, ) @@ -689,6 +695,66 @@ def test_a_kept_tree_with_no_commit_marker_names_no_commit(self) -> None: self.assertEqual(live["vcs"], "archive") +class PluginStateCase(unittest.TestCase): + """The marketplace listing proves the directory is registered, not that Claude Code loads + its plugin, so the plugin's install and enabled state is read from `claude plugin list`.""" + + def setUp(self) -> None: + self.addCleanup(mock.patch.stopall) + + def plugin_list(self, stdout: str, returncode: int = 0) -> None: + result = mock.Mock(returncode=returncode, stdout=stdout) + self.runner = mock.patch("subprocess.run", return_value=result).start() + + def plugin_id(self) -> str: + return f"{skills_install.PLUGIN_NAME}@{skills_install.MARKETPLACE_NAME}" + + def test_an_installed_and_enabled_plugin_reads_as_both(self) -> None: + self.plugin_list(json.dumps([{"id": self.plugin_id(), "scope": "user", "enabled": True}])) + self.assertEqual(skills_install.plugin_state(), {"installed": True, "enabled": True}) + self.assertEqual(self.runner.call_args.args[0], ["claude", "plugin", "list", "--json"]) + self.assertEqual(self.runner.call_args.kwargs["timeout"], skills_install.SUBPROCESS_TIMEOUT) + + def test_only_the_user_scope_install_counts(self) -> None: + """A project install of the same id is not what the installer put in place.""" + self.plugin_list( + json.dumps([{"id": self.plugin_id(), "scope": "project", "enabled": True}]) + ) + self.assertEqual(skills_install.plugin_state(), {"installed": False, "enabled": False}) + + def test_an_installed_but_disabled_plugin_reads_as_not_enabled(self) -> None: + self.plugin_list(json.dumps([{"id": self.plugin_id(), "scope": "user", "enabled": False}])) + self.assertEqual(skills_install.plugin_state(), {"installed": True, "enabled": False}) + + def test_a_plugin_absent_from_the_listing_reads_as_not_installed(self) -> None: + self.plugin_list(json.dumps([{"id": "other@elsewhere", "scope": "user", "enabled": True}])) + self.assertEqual(skills_install.plugin_state(), {"installed": False, "enabled": False}) + + def test_an_unreadable_listing_is_unknown_rather_than_not_installed(self) -> None: + unknown = {"installed": None, "enabled": None} + self.plugin_list("", returncode=1) + self.assertEqual(skills_install.plugin_state(), unknown) + self.plugin_list("not json") + self.assertEqual(skills_install.plugin_state(), unknown) + self.plugin_list("{}") + self.assertEqual(skills_install.plugin_state(), unknown) + mock.patch("subprocess.run", side_effect=OSError).start() + self.assertEqual(skills_install.plugin_state(), unknown) + mock.patch( + "subprocess.run", side_effect=subprocess.TimeoutExpired(cmd="claude", timeout=1) + ).start() + self.assertEqual(skills_install.plugin_state(), {"installed": None, "enabled": None}) + + def test_the_live_channel_carries_the_state_only_where_registered(self) -> None: + mock.patch("skills_install.claude_available", return_value=True).start() + state = {"installed": True, "enabled": False} + mock.patch("skills_install.plugin_state", return_value=state).start() + mock.patch("skills_install.marketplace_channel", return_value={"registered": True}).start() + self.assertEqual(skills_install.live_channel()["plugin"], state) + mock.patch("skills_install.marketplace_channel", return_value={"registered": False}).start() + self.assertNotIn("plugin", skills_install.live_channel()) + + class MainExitCodeCase(unittest.TestCase): """A caller scripting this installer needs the exit code to distinguish a real failure (claude present but registration failed) from an expected partial install (no claude on @@ -927,7 +993,7 @@ class LinuxWrapperSudoGuardCase(unittest.TestCase): def run_wrapper(self, root: bool, sudo_user: str | None) -> subprocess.CompletedProcess[str]: argv = ["unshare", "-r"] if root else [] - env = {"PATH": os.environ.get("PATH", ""), "HOME": os.environ.get("HOME", "")} + env = {"PATH": os.environ.get("PATH", os.defpath), "HOME": os.environ.get("HOME", "")} if sudo_user is not None: env["SUDO_USER"] = sudo_user return subprocess.run( @@ -976,7 +1042,7 @@ def test_help_still_answers_under_sudo(self) -> None: encoding="utf-8", timeout=60, check=False, - env={"PATH": os.environ.get("PATH", ""), "SUDO_USER": "someone"}, + env={"PATH": os.environ.get("PATH", os.defpath), "SUDO_USER": "someone"}, ) self.assertEqual(r.returncode, 0) self.assertIn("never under sudo", r.stdout)