From 61359b31413096c2e0a106450e72895419cc0864 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 12:20:00 -0700 Subject: [PATCH 1/5] Push the NuGet Package From This Repository Rather Than the Hub Task The first real release after this repository adopted the hub release chain failed at the NuGet.org token exchange with HTTP 401, because trusted publishing validates the OIDC token's job_workflow_ref claim against the repository owning the package and the claim named the hub's build-release-task.yml. The hub's fix, ProjectTemplate#1126, moves the push into the calling repository, and this is this repository's half of it. The pin moves from 2.0.526 to 2.0.536 across all six references in three workflows. That is one change with the stub edit rather than two, because the hub removed the nuget input and the NUGET_USERNAME secret from the task, so a pin bump on its own startup-fails against names the task no longer declares. Verified mechanically rather than by reading: every input and secret each of the four callers passes is declared on that task at 1fe2537. The publish job sheds the NUGET_USERNAME mapping, the id-token grant and the nuget input, keeping enable_nuget so the task still builds and uploads the package. A new publish-nuget job downloads nuget-build-, trades the GitHub OIDC token for a short-lived NuGet key, and pushes, so the claim names this repository. It carries id-token at that one entry point, which is what D7.2 asks for. The smoke path drops nuget: false with them, since the input is gone there too. WORKFLOW.md described the superseded model, that NuGet pushes from the leaf, on three of the lines this change contradicts. It is carried at intent fidelity, so it is carried forward from the hub at 2.0.536 rather than hand-edited, with one exception. This repository holds a local correction to D1.6 saying CODECOV_TOKEN is mapped explicitly under the calling job's own secrets block. The guard's exact-phrase probe finds that sentence in neither hub revision, so it is a local addition, and it is the accurate one here: both workflows map the token explicitly and neither uses secrets: inherit. It is preserved, and the carried file now differs from the hub by exactly that one line. The release notes record the fix, since it is what lets the package ship again. --- .github/workflows/merge-bot-pull-request.yml | 2 +- .github/workflows/publish-release.yml | 67 +++++++++++++++++--- .github/workflows/test-pull-request.yml | 5 +- HISTORY.md | 1 + WORKFLOW.md | 51 ++++++++------- 5 files changed, 91 insertions(+), 35 deletions(-) diff --git a/.github/workflows/merge-bot-pull-request.yml b/.github/workflows/merge-bot-pull-request.yml index 6b5c59f..4fdc380 100644 --- a/.github/workflows/merge-bot-pull-request.yml +++ b/.github/workflows/merge-bot-pull-request.yml @@ -19,7 +19,7 @@ jobs: merge-bot: name: Merge bot pull request job - uses: ptr727/ProjectTemplate/.github/workflows/merge-bot-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 + uses: ptr727/ProjectTemplate/.github/workflows/merge-bot-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 secrets: CODEGEN_APP_CLIENT_ID: ${{ secrets.CODEGEN_APP_CLIENT_ID }} CODEGEN_APP_PRIVATE_KEY: ${{ secrets.CODEGEN_APP_PRIVATE_KEY }} diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 7950574..aa8a12b 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -31,7 +31,7 @@ jobs: # Single source of the release-gate decision, publish or not and stable or not, reused by every job below. plan: name: Plan release job - uses: ptr727/ProjectTemplate/.github/workflows/publish-plan-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 + uses: ptr727/ProjectTemplate/.github/workflows/publish-plan-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 with: event_name: ${{ github.event_name }} actor: ${{ github.actor }} @@ -42,7 +42,7 @@ jobs: name: Validate job needs: [plan] if: ${{ needs.plan.outputs.publish == 'true' }} - uses: ptr727/ProjectTemplate/.github/workflows/validate-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 + uses: ptr727/ProjectTemplate/.github/workflows/validate-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} permissions: @@ -51,26 +51,77 @@ jobs: # Build, version, push, and release the triggering branch, main a stable release and develop a prerelease by dispatch. # This repository ships one target, the NuGet package, so every other target is disabled explicitly rather than left at its default. # The grants are what the enabled paths write with. - # Contents and actions cover the release upload and its artifact cleanup, and id-token covers the keyless NuGet push through OIDC trusted publishing. + # Contents and actions cover the release upload and its artifact cleanup. + # The NuGet push is not here, so neither is id-token, see the publish-nuget job below. publish: name: Publish project release job needs: [plan, validate] if: ${{ needs.plan.outputs.publish == 'true' && needs.validate.result == 'success' }} - uses: ptr727/ProjectTemplate/.github/workflows/build-release-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 - secrets: - NUGET_USERNAME: ${{ secrets.NUGET_USERNAME }} + uses: ptr727/ProjectTemplate/.github/workflows/build-release-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 permissions: contents: write - id-token: write actions: write with: ref: ${{ github.sha }} branch: ${{ github.ref_name }} smoke: false github: true - nuget: true enable_nuget: true enable_docker: false enable_pypi: false enable_dotnet_publish: false nuget_project: ./Utilities/Utilities.csproj + + # The push lives here rather than in the hub task so the OIDC token's job_workflow_ref claim names this repository. + # NuGet.org validates that claim against the repository owning the package, and a job running from the hub task carries the hub's workflow path instead (ptr727/ProjectTemplate#1126). + # A skipped publish job skips this one too, so no separate release gate is needed. + publish-nuget: + name: Publish NuGet library job + needs: [validate, publish] + runs-on: ubuntu-latest + permissions: + id-token: write + contents: read + actions: write + steps: + - name: Download build artifacts step + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: nuget-build-${{ github.ref_name }} + path: ./nuget + - name: Setup .NET SDK step + uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6.0.0 + with: + dotnet-version: 10.x + # Trades the GitHub OIDC token for a short-lived NuGet key, so there is no stored API key. + - name: NuGet login step + id: nuget-login + uses: NuGet/login@8d196754b4036150537f80ac539e15c2f1028841 # v1.2.0 + with: + user: ${{ secrets.NUGET_USERNAME }} + # Pushing the .nupkg also pushes the co-located .snupkg to nuget.org's symbol server, since no --no-symbols flag is set. + - name: Push to NuGet.org step + env: + NUGET_API_KEY: ${{ steps.nuget-login.outputs.NUGET_API_KEY }} + run: | + set -Eeuo pipefail + dotnet nuget push ./nuget/*.nupkg \ + --source https://api.nuget.org/v3/index.json \ + --api-key "$NUGET_API_KEY" \ + --skip-duplicate + - name: Delete consumed NuGet build artifact step + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + run: | + set -Eeuo pipefail + if ! ids=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/${{ github.run_id }}/artifacts" --paginate \ + --jq ".artifacts[] | select(.name == \"nuget-build-${{ github.ref_name }}\") | .id"); then + echo "::warning::Could not list NuGet build artifacts. The retention-days backstop will reap them." + ids="" + fi + for id in $ids; do + if ! gh api --method DELETE "repos/$GITHUB_REPOSITORY/actions/artifacts/$id"; then + echo "::warning::Failed to delete artifact $id. The retention-days backstop will reap it." + fi + done diff --git a/.github/workflows/test-pull-request.yml b/.github/workflows/test-pull-request.yml index 4d8049a..ce383d5 100644 --- a/.github/workflows/test-pull-request.yml +++ b/.github/workflows/test-pull-request.yml @@ -55,7 +55,7 @@ jobs: validate: name: Validate job - uses: ptr727/ProjectTemplate/.github/workflows/validate-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 + uses: ptr727/ProjectTemplate/.github/workflows/validate-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 permissions: contents: read secrets: @@ -71,13 +71,12 @@ jobs: name: Smoke build job needs: [changes] if: needs.changes.outputs.release == 'true' || github.event_name == 'workflow_dispatch' - uses: ptr727/ProjectTemplate/.github/workflows/build-release-task.yml@f3b4cc98654878e63ebfe21b68b8516b7bb46469 # 2.0.526 + uses: ptr727/ProjectTemplate/.github/workflows/build-release-task.yml@1fe25376c68bccf6b4581db1568bf325a854dba0 # 2.0.536 permissions: contents: read with: smoke: true github: false - nuget: false enable_nuget: true enable_docker: false enable_pypi: false diff --git a/HISTORY.md b/HISTORY.md index 525c22e..57aa14e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -6,6 +6,7 @@ Some useful and not so useful C# .NET utility classes. - v4.1: - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination file: both opened it with `File.OpenWrite()`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now truncate the destination explicitly and rewrite it in place, which keeps its permissions, ownership, and any links to it. The truncation happens once the response headers are accepted rather than once the body has arrived, so a download that fails partway now leaves a short file where it previously left the original bytes behind the new ones. + - Moved the NuGet.org push out of the shared release task and into this repository's own publisher, restoring publishing. Trusted publishing validates the OIDC token's `job_workflow_ref` claim against the repository owning the package, and a push made from a workflow another repository hosts is rejected at the token exchange, which is what failed the first release attempt after the release chain was adopted. - Added `StringHistory.SetLimits()`, which applies both limits in one re-partition. Assigning `MaxFirstLines` and `MaxLastLines` one after the other re-partitions twice, so the first assignment measures against the other limit's previous value and can discard lines the final pair would have retained. Which order avoids that, where either does, depends on the values and on what is stored, so no fixed ordering is safe. - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later `AppendLine()`, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. `AppendLine()` changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - v4.0: diff --git a/WORKFLOW.md b/WORKFLOW.md index ee101bd..20da0fb 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -20,7 +20,7 @@ The guarantees are distilled from failures observed in practice and stated as th - **Applicability.** A guarantee (or a 5A check, or a 5B scenario) is **applicable** only if the repo contains the construct it governs: a given target, a transfer artifact, a registry push, a wrapper-version source. An item that governs an absent construct is **N/A**: record it as N/A and **exclude it from the verdict**. N/A is never a defect. Section 6 names which items go N/A per project type. A near-empty pipeline (source-only) is mostly N/A and that is fine. - **Operational is binary.** A workflow is operational only if every *applicable* guarantee holds. A single applicable input/output mismatch is a defect and makes the workflow non-operational, regardless of how clean the YAML looks. - **Default branch.** Guarantees say "default branch" portably. It is implemented as the literal `main` in several places (the validate gate, the `prerelease` expression, and `version.json`'s `publicReleaseRefSpec`). These MUST all reference the repo's *actual* default branch. A divergence is a defect (section 5A). -- **Two layers when auditing.** The pipeline splits into an **orchestrator** layer (the PR entry workflow, the publisher, and the version/release/badge jobs) and a **build-leaf** layer (`build--task.yml`). Inputs like `github`/`nuget`/`dockerhub`/`expect_release_assets` live on the orchestrator. A leaf only ever receives `ref`/`branch`/`smoke` (and a derived `push`). When a check names an input, assert it in the layer that declares it. +- **Two layers when auditing.** The pipeline splits into an **orchestrator** layer (the PR entry workflow, the publisher, and the version and release jobs) and a **build-leaf** layer (the `build-` tasks, whether separate files or jobs inside the release task). Inputs like `github`/`dockerhub`/`expect_release_assets` live on the orchestrator. A leaf receives `ref`/`branch`/`smoke` and whatever else its own target needs, a derived `push` among them where that leaf pushes. A package target declares no push input on either layer, because section 3's `Output Seam by Destination` puts its push in a `publish-` job in the publisher, gated by `needs:` rather than by a flag. When a check names an input, assert it in the layer that declares it. - **The three verbs.** Audit (static), Test (trace + probe), Assess (verdict). Section 5 gives the exact procedure. ## 2. Workflow Style Conventions @@ -69,8 +69,8 @@ Their CI is lint/validation only (editorconfig/EOL plus domain linters such as H ### Two Layers: Orchestration vs Build - **Orchestration** is generic and forms the standardization baseline **at the job level**: the single-branch publisher, the `get-version`, `validate-release`, and `github-release` jobs, and the `changes -> smoke-build -> aggregator` shape of the PR workflow. These job *bodies* should not need per-repo edits. -- **Build** is repo-owned: the `build--task.yml` leaf tasks. -- **What the repo curates** (by design, not a leak): the *list* of targets. This is **not** a byte-for-byte file carry. Adding or dropping a target edits the orchestrator's surface: the `enable_` inputs and the `build-` job + its `github-release` `needs:` entry in the release task, **and** the `changes` paths-filter entry + output + the `smoke-build` enable-forward in the PR workflow. "Verbatim" applies to the `github-release` job and the version/publish-plan logic, not to the release task's job list or the paths-filter. Subsetting is symmetric: the same surface you trim to drop a target you extend to add a new one (e.g. a `release-asset--library` producer needs a new `enable_library` input, a `build-library` job, a `needs:` entry, and a `library` paths-filter). +- **Build** is repo-owned in shape: the `build-` leaf tasks, whether this repo hosts them itself or reaches hub-hosted ones by pin. +- **What the repo curates** (by design, not a leak): the *list* of targets. This is **not** a byte-for-byte file carry. Adding or dropping a target edits the orchestrator's surface: the `enable_` inputs and the `build-` job + its `github-release` **and** `build-docker` `needs:` entries in the release task, **and** the `changes` paths-filter entry + output + the `smoke-build` enable-forward in the PR workflow, plus the separate `publish-` job for a package target. "Verbatim" applies to the `github-release` job and the version/publish-plan logic, not to the release task's job list or the paths-filter. Subsetting is symmetric: the same surface you trim to drop a target you extend to add a new one (e.g. a `release-asset--library` producer needs a new `enable_library` input, a `build-library` job, its two `needs:` entries, and a `library` paths-filter entry, output, and `smoke-build` enable-forward). ### The Seam Contract @@ -78,12 +78,17 @@ A target contributes a file to the GitHub release by uploading a workflow artifa ```mermaid flowchart LR - dotnet[dotnet-publish] -->|release-asset--dotnet-publish| store[(run artifacts)] - nuget[build-nuget] -->|release-asset--nuget| store + dotnet[dotnet-publish] -->|release-asset-BRANCH-dotnet-publish| store[(run artifacts)] + nuget[build-nuget] -->|release-asset-BRANCH-nuget| store store -->|pattern + merge-multiple| rel["github-release job (D6)"] - reg[registry leaf: nuget / pypi / docker] -->|push, no asset| registries[(registries)] + nuget -->|nuget-build-BRANCH| pub["publish-TARGET job in the repo's own publisher"] + pypi[build-pypi] -->|pypi-build-BRANCH| pub + pub -->|push| registries[(registries)] + docker[build-docker] -->|push| registries ``` +The diagram writes `BRANCH` and `TARGET` where the prose writes `` and ``, because a mermaid label is sanitized as HTML at render and an angle-bracket placeholder is dropped as an unknown tag. This reaches node labels as well as edge labels, which is why the Release Model diagram below writes `X.Y.Z-g-sha` rather than bracketing its own placeholder. + ### Reusable-Task Parameter Contract Every leaf and the release task take `ref`, `branch` (the **logical** branch that drives config/tags/prerelease), and where relevant `smoke`. Branch-derived config keys off `inputs.branch` (the logical branch the caller passes). Artifact names are branch-suffixed. @@ -117,7 +122,7 @@ flowchart TD ### Release Model -Each publish builds a **single branch**, the trigger ref (`main` a release, `develop` a prerelease), so there is no branch matrix and `github.ref` always names the built branch. A **human merge never auto-publishes**: a first `plan` job (`publish-plan-task.yml`) decides once and every job gates on it. A run publishes on a **code-affecting bot push to `main`** (the App merges every Dependabot/codegen PR, so `github.actor` gates it, and a shared paths filter also drops a non-substantive change like an Actions bump), a **manual dispatch** of `main`/`develop`, or a **main-only weekly schedule** (Docker, to refresh the base image). The `push` is main-only, so a develop bot merge publishes nothing (its prerelease comes via dispatch). A **source-only** repo publishes on **dispatch only**. Every release is a tag on the built commit plus a source archive, README, and LICENSE. Targets amend it with `release-asset-*` files or push to their own registry. An unchanged version re-pushes nothing (no-op republish). Docker re-pushes by design. +Each publish builds a **single branch**, the trigger ref (`main` a release, `develop` a prerelease), so there is no branch matrix and `github.ref` always names the built branch. A **human merge never auto-publishes**: a first `plan` job (`publish-plan-task.yml`) decides once and every job gates on it. A run publishes on a **code-affecting bot push to `main`** (the App merges every Dependabot/codegen PR, so `github.actor` gates it, and a shared paths filter also drops a non-substantive change like an Actions bump), a **manual dispatch** of `main`/`develop`, or a **main-only weekly schedule** (Docker, to refresh the base image). The `push` is main-only, so a develop bot merge publishes nothing (its prerelease comes via dispatch). A **source-only** repo publishes on **dispatch only**. Every release is a tag on the built commit plus a source archive, README, and LICENSE. Targets amend it with `release-asset-*` files, and a registry push contributes none, made by the Docker leaf for an image and by the separate `publish-` job for a package. An unchanged version re-pushes nothing (no-op republish). Docker re-pushes by design. ```mermaid flowchart TD @@ -133,7 +138,7 @@ flowchart TD Pick each output's path by **where the artifact goes**: - **File on the GitHub release** (zip, binary, packaged library): one leaf per output uploading `release-asset--`. The repo keeps `expect_release_assets: true` (its default). -- **Package-registry push** (NuGet, PyPI): the leaf builds and publishes to its registry. NuGet pushes from the leaf *and* uploads a `release-asset-*`. PyPI is **split**: the leaf only builds + uploads its build artifact, a separate publish job does the OIDC upload (so `id-token: write` is granted at one entry point, behind an environment gate) and contributes **no** `release-asset-*`. +- **Package-registry push** (NuGet, PyPI): the leaf builds and uploads a build artifact (`nuget-build-` / `pypi-build-`), and a separate `publish-` job in the **publishing repository's own** publisher consumes it and pushes. Both registries publish through OIDC Trusted Publishing, never a stored API key, and two things put that push outside the leaf. Trusted publishing validates the OIDC token's `job_workflow_ref` claim, which names the workflow the job actually ran from, so a push made from a reusable workflow a *different* repository hosts is rejected at the token exchange, NuGet.org answering `HTTP 401` with `does not start with //.github/workflows/`. That alone rules out a leaf another repository hosts. A leaf this repository hosts clears the claim, and the split still applies to it, because a called job declaring no `permissions:` runs under the calling job's whole grant, so a push anywhere inside the release task would put `id-token: write` on every job in it rather than at the one entry point D7.2 requires. The registered trusted-publishing policy therefore names the publisher, `publish-release.yml`. PyPI additionally gates its publish job behind an environment. NuGet.org binds its policy to the workflow file rather than to an environment and needs none. NuGet's leaf also uploads a `release-asset-*` carrying the package, and PyPI contributes none. - **Image-registry push** (Docker): the leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only (arm64 emulation is reserved for the released image), and contributes no `release-asset-*`. - **Filesystem on a host the project owns** (a static site, a config tree): the leaf builds the tree, ships it to the host, and contributes no `release-asset-*`. The transport is the repo's own. What the contract fixes is that the deploy is a **separate `workflow_dispatch`** from the release, so a redeploy of an unchanged commit mints no tag and a host rebuild, a rollback, or proving a branch on a non-production environment costs nothing; that its credentials come from a **per-environment GitHub Environment** rather than the repository secret store; and that the deploy ends by asserting **what the host serves** rather than the transport's exit status (D4.6). Retention at the destination is bounded by a declared count with one side recorded as owning the prune, which is the deploy where its credential can observe the destination and the host where that credential is deliberately write-only (D5.6). - **No file target via the release task** (Docker-only, PyPI-only, source-only): the release is tag + source zip + README + LICENSE. The caller **MUST pass `expect_release_assets: false`** to the release task. A publisher with file targets retains the default `true`. This setting is caller-specific. The default `true` fails on `fail_on_unmatched_files` when no assets exist. A **source-only** repo also passes every `enable_*` input as false because it has no build leaf (see Section 6). @@ -172,13 +177,13 @@ The required behaviors, organized by domain. Each is a **MUST**, stated as input - **D4.2 Tag the built commit.** Output: the release `target_commitish` is the built commit's SHA (NBGV's `GitCommitId`), never a branch name or a separately re-resolved ref. *Prevents: the tag landing on the default branch instead of the built tree.* - **D4.3 Release contents.** Output: every release contains a tag on the built commit plus the auto source zip, README, and LICENSE. File targets attach `release-asset-*`. The `prerelease` value equals `branch != default`. A no-file-target caller sets `expect_release_assets: false` to reach the tag-only shape. This applies to Docker-only, PyPI-only, and source-only repos. The setting relaxes `fail_on_unmatched_files` and skips the asset download. The release-create step fails when no assets exist and the setting retains its default `true`. A source-only caller also sets every `enable_*` input false. - **D4.4 No-op republish.** Input: a re-run whose version is unchanged. Output: nothing is re-pushed, because the release-create step is skipped when the tag exists (refreshed only on `workflow_dispatch`), and the paired asset-delete is skipped with it. Registry pushes are no-ops. The NuGet/PyPI publish steps are **not** statically gated on existence. They run and the **server** dedupes (`dotnet nuget push --skip-duplicate` turns a 409 into success, and PyPI does the same under `skip-existing: true`). **Docker always re-pushes** the image (base-image refresh), independently of the release-create skip, within the same run. *Prevents: duplicate releases and wasted pushes.* -- **D4.5 A build failure blocks every publish target.** Input: a real publish where one enabled build fails. Output: nothing publishes. `github-release` needs every build, so a failed build skips it (no tag, no release), and the terminal registry pusher (Docker) needs every other build and guards its `if` with `!failure() && !cancelled()`, so a failed build skips docker too (no image push) while a disabled or unchanged target (skipped, not failed) still lets docker push. *Prevents: a partial publish, e.g. a Docker image pushed while .NET publish failed and no release was cut.* A repo pushing two registry targets at once would need a build/publish split behind an all-builds gate, which none does today. +- **D4.5 A build failure blocks every publish target.** Input: a real publish where one enabled build fails. Output: nothing publishes. `github-release` needs every build, so a failed build skips it (no tag, no release), and the terminal registry pusher (Docker) needs every other build and guards its `if` with `!failure() && !cancelled()`, so a failed build skips docker too (no image push) while a disabled or unchanged target (skipped, not failed) still lets docker push. *Prevents: a partial publish, e.g. a Docker image pushed while .NET publish failed and no release was cut.* A package target's separate publish job needs its own gate for the same reason, since it sits outside the `github-release` and Docker `needs:` chains: it `needs:` the release-task call, so a failed build skips it with the rest. - **D4.6 Deploy verification names the release.** Input: a deploy to a filesystem on a host the project owns that completes without error. Output: a check against the running host asserts **which release is answering**, not merely that it answers. The artifact stamps its own version into the configuration it ships, and the check compares that against the version just installed, **waiting for convergence to a bounded timeout** rather than sampling once, because content goes live the instant a pointer moves while server rules wait on an asynchronous reload. The same check asserts **which environment** answered, since several environments serve a byte-identical artifact and a proxy rule aimed at the wrong one answers healthily under the right hostname. An unreachable host is reported distinctly from an HTTP status. *Prevents: a green deploy over a host still serving the previous release's configuration, a URL contract checked against the wrong environment, and a dead config watcher read as a routing fault.* ### D5 - Resource Cleanup - **D5.1 Delete at the point of consumption.** Output: the job that downloads a **cross-job** transfer artifact deletes it (by exact name/pattern) right after consuming it. An intermediate consumed only within the same run MAY instead rely on the `retention-days: 1` backstop. *Prevents: transfer artifacts accumulating against the storage quota.* -- **D5.2 Gate the delete to the consumer's condition.** Output: the delete runs under the **same** condition as its consuming step. Where the consumer is conditional (the GitHub release create), the delete is conditional too. Where the consumer always runs when its job runs (the PyPI publish step), the delete always runs, so on a no-op re-run the `release-asset-*` delete is **skipped** while the PyPI build-artifact delete still **runs** (its publish ran). *Prevents: deleting freshly built assets on a no-op re-run.* +- **D5.2 Gate the delete to the consumer's condition.** Output: the delete runs under the **same** condition as its consuming step. Where the consumer is conditional (the GitHub release create), the delete is conditional too. Where the consumer always runs when its job runs (a package publish job's push), the delete always runs, so on a no-op re-run the `release-asset-*` delete is **skipped** while the `nuget-build-*` and `pypi-build-*` deletes still **run** (their publish ran). *Prevents: deleting freshly built assets on a no-op re-run.* - **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.* @@ -189,12 +194,12 @@ The required behaviors, organized by domain. Each is a **MUST**, stated as input - **D6.1 Pattern handoff.** Output: the release job downloads by `pattern:`/`merge-multiple:`, not `artifact-ids:`. Targets upload `release-asset--`. Canonical for single-target. - **D6.2 Branch drives config.** Output: branch-derived config reads `inputs.branch`, never `github.ref_name`. - **D6.3 Branch-suffixed artifacts.** Output: artifact names are branch-suffixed so a branch's artifacts do not collide with another branch's. -- **D6.4 Target add/drop is consistent.** Output: adding or dropping a target updates **all** of: the `enable_` input, the `build-` job and its `github-release` `needs:` entry, the `changes` paths-filter entry + output, and the `smoke-build` enable-forward (and, for PyPI, the separate `publish-pypi` job). The `github-release` job body stays verbatim. *Prevents: a partial subset that startup-fails on a missing leaf or never smoke-builds a target.* +- **D6.4 Target add/drop is consistent.** Output: adding or dropping a target updates **all** of: the `enable_` input, the `build-` job and its `github-release` and `build-docker` `needs:` entries, the `changes` paths-filter entry + output, and the `smoke-build` enable-forward (and, for a package target, the separate `publish-` job). The `github-release` job body stays verbatim. *Prevents: a partial subset that startup-fails on a missing leaf or never smoke-builds a target.* ### D7 - Concurrency, Permissions, Safety - **D7.1 Publisher serializes.** Output: the publisher uses a **global, ref-independent** concurrency group with `cancel-in-progress: false`. *Prevents: a schedule and a dispatch double-pushing, or a cancelled publish leaving a partial release.* -- **D7.2 Skipped jobs still need valid permissions.** Output: every reusable job declares valid `permissions:`. A callee's extra scope (e.g. `actions: write` for cleanup, or `id-token: write` for OIDC) is granted by the caller and appears at exactly the one entry point that needs it. +- **D7.2 A called job's permissions block is validated before its `if:`.** Output: a reusable job declares `permissions:` only where **every** caller grants that scope at startup, and otherwise declares none and runs under whatever the calling job granted. A callee's extra scope (e.g. `actions: write` for cleanup, or `id-token: write` for OIDC) is granted by the caller and appears at exactly the one entry point that needs it. *Prevents: a `startup_failure` on every caller that does not grant a scope only one target needs, including a smoke build under a read-only pull request token.* - **D7.3 Boolean inputs both forms.** Output: declared in both trigger blocks, compared against `true` and `'true'`. - **D7.4 Optional-dependency chaining.** Output: cross-job conditions allowlist `success`/`skipped` explicitly. @@ -223,19 +228,19 @@ Read the workflow files plus `version.json` and assert the structural fact behin **Core (every repo):** -- **D1:** a `changes` paths-filter job exists, covers each of the repo's targets, and **excludes** `.github/workflows/**`; the PR entry workflow's smoke call sets `github/nuget/dockerhub: false` on the release task; the leaf receives `smoke: true` and a derived `push` (false on smoke); every build-task `upload-artifact` (and any aggregation job) is gated `!smoke`; the aggregator `needs:` the `changes` and validation jobs, blocks on `failure`/`cancelled`, passes on `skipped`; a validation job runs unconditionally. +- **D1:** a `changes` paths-filter job exists, covers each of the repo's targets, and **excludes** `.github/workflows/**`; the PR entry workflow's smoke call sets every publish flag its release task declares to false (`github`/`dockerhub`, and a package-push flag there is itself a finding, per section 1); a pushing leaf receives `smoke: true` and a derived `push` (false on smoke), and a build-only leaf receives `smoke: true` with no `push` to derive; every build-task `upload-artifact` (and any aggregation job) is gated `!smoke`; the aggregator `needs:` the `changes` and validation jobs, blocks on `failure`/`cancelled`, passes on `skipped`; a validation job runs unconditionally. - **D2:** an entry validation job/step exists per complex-input workflow; the release gate checks both directions, strips `+buildmetadata`, and skips on smoke; the publisher rejects a dispatch from a ref other than `main` or `develop`. - **D3:** each run builds one branch, so NBGV classifies `github.ref` directly (no `IGNORE_GITHUB_REF`), and the default-branch literal in the gate (`== 'main'`), the `prerelease` expression (`!= 'main'`), and `version.json`'s `publicReleaseRefSpec` all name the repo's actual default branch. - **D4:** `target_commitish` is the NBGV commit id; `prerelease` equals `branch != default`; the release-create step is gated `exists == 'false' || github.event_name == 'workflow_dispatch'` (the step output is the string `'false'`, not a boolean); the asset-delete step is gated identically. A dispatch-only publisher (`releaseTrigger: dispatch-only`) may omit the gate and the exists-check entirely: every run is a dispatch, so the skip leg can never fire and create-or-refresh is unconditional. Record the gate N/A there, not missing. - **D5:** each cross-job transfer artifact has a delete step at its consumer, gated to the consumer's condition, `continue-on-error: true`, looping all ids; **every** upload sets `retention-days: 1`; **no** `.artifacts[].id` blanket delete exists anywhere. -- **D6:** the release download uses `pattern:`/`merge-multiple:` (no `artifact-ids:`). Branch-derived config reads `inputs.branch` (a `github.ref_name` in such config is a finding). Artifact names are branch-suffixed. The target set is consistent across the release task and the paths-filter. -- **D7:** the publisher concurrency group is ref-independent with `cancel-in-progress: false`. Reusable jobs declare permissions. Boolean `if:` uses both forms. +- **D6:** the release download uses `pattern:`/`merge-multiple:` (no `artifact-ids:`). Branch-derived config reads `inputs.branch` (a `github.ref_name` in such config is a finding). Artifact names are branch-suffixed. The target set is consistent across the release task (both the `github-release` and `build-docker` `needs:` lists), the paths-filter, the `smoke-build` enable-forward, and any separate `publish-` job the package-registry seam requires. The `inputs.branch` rule above binds a called leaf, while a `publish-` job is in the publisher and reads `github.ref_name` correctly. +- **D7:** the publisher concurrency group is ref-independent with `cancel-in-progress: false`. A reusable job declares `permissions:` only where every caller grants that scope at startup, per D7.2. Boolean `if:` uses both forms. - **D8/D9:** merge-bot concurrency keys on PR number. The upstream tracker's branch prefix matches a merge-bot rule (wrapper repos). Actions are SHA-pinned. Names/shells/conditionals follow section 2. **Per-type addenda (apply only the ones present):** - **.NET publish:** the smoke runtime set is a strict non-empty subset of the full runtime set. The selected set runs sequentially inside one composite-action job. A non-smoke run uploads one `release-asset--dotnet-publish` artifact, while a smoke run skips the archive and upload steps. -- **NuGet:** the publish step is gated `if: inputs.push` only (not on an existence check) and uses `--skip-duplicate`. `*.nupkg` push also carries the paired `.snupkg` to the symbol server where symbols are enabled. The `release-asset` zip carries the package(s). +- **NuGet:** `publish-nuget` is a job in the repo's own publisher, never inside the release task and never in a reusable workflow a different repository hosts. `id-token: write` appears on that job only, absent from the build and PR paths, beside `actions: write` for the artifact cleanup. The push uses `--skip-duplicate` and is gated by that job's `needs:` on the release-task call, never on an existence check, so a PR never reaches it. The job consume-then-deletes `nuget-build-` per D5.1. `*.nupkg` push also carries the paired `.snupkg` to the symbol server where symbols are enabled. The `release-asset` `.7z` carries the package(s). - **PyPI:** `publish-pypi` declares `environment: { name: pypi }`. `id-token: write` appears only on that job (absent from the build/PR path). `skip-existing: true` is set on the publish action. The build artifact is deleted after publish. The `pypi` environment has a deployment-branch rule. - **Docker:** a Docker-only repo's caller passes `expect_release_assets: false`. The leaf reads the external state file for the tag instead of `SemVer2` (wrapper repos only, since a plain Docker repo correctly tags off `SemVer2` and records this N/A). The readme job is gated main-only, both by the caller's branch input and inside the hub-hosted `publish-docker-readme-task.yml` itself. The docker-readme task validates `repositories` XOR `manifest`+`manifest-jq`. The buildcache follows D9.4. - **Static site deployed to a host:** the generator is pinned by version **and** by a checksum verified before install, declared once across the workflows that install it. The deploy is a dispatch carrying an environment choice, with concurrency keyed on the **environment** and `cancel-in-progress: false`, and production gated to the default branch while any ref may reach a non-production environment. The reusable callee re-asserts the environment name in a job of its own. The upload targets a per-release directory and carries no delete flag at the environment root, and the pointer flip is a separate step. The terminal check asserts the golden-list length floors first, then the environment, then the release id, then the URL contract. Retention is bounded by a declared count and one side is recorded as owning the prune: the deploy asserts it where the credential can observe the destination, and the host owns it where the credential is confined write-only (D5.6). @@ -252,9 +257,9 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | S4 | PR base = default branch, carrying a build target | smoke versions as prerelease, validate-release **skipped (smoke)** so the default-branch arm does **not** fire, aggregator **success**, promotion not blocked | D1.3, D2.2 | | S5 | bot push to `main` not touching a release path (e.g. an Actions bump) | the paths filter excludes it, so nothing publishes | D4.1 | | S6 | code-affecting **bot** push to `main` (a human push/promotion, or any develop push, does not) | the `plan` job gates it to the App/Dependabot actor, and `main` publishes a release | D3, D4 | -| S7 | publish run (schedule, a bot push to main, or a dispatch) | builds the **one** trigger branch: `main` -> `X.Y.Z`, `prerelease=false`, registry stable, badge/readme run; `develop` -> `X.Y.Z-g`, `prerelease=true`, registry prerelease; `release-asset-*` consumed-then-deleted; PyPI build-artifact deleted after its publish; **no dangling artifacts** | D3, D4, D5, D6, D7 | +| S7 | publish run (schedule, a bot push to main, or a dispatch) | builds the **one** trigger branch: `main` -> `X.Y.Z`, `prerelease=false`, registry stable, readme run; `develop` -> `X.Y.Z-g`, `prerelease=true`, registry prerelease; `release-asset-*` consumed-then-deleted; each package build-artifact (`nuget-build-*`, `pypi-build-*`) deleted after its publish; **no dangling artifacts** | D3, D4, D5, D6, D7 | | S8 | dispatch from a ref other than `main` or `develop` | **fails fast** | D2.3 | -| S9 | re-run publish, version unchanged | release-create **skipped**, `release-asset-*` delete **skipped**; NuGet/PyPI pushes no-op (server dedupe); **PyPI build-artifact still deleted** (its publish ran); **Docker still re-pushes** the image; no duplicate release | D4.4, D5.2 | +| S9 | re-run publish, version unchanged | release-create **skipped**, `release-asset-*` delete **skipped**; NuGet/PyPI pushes no-op (server dedupe); **package build-artifacts still deleted** (their publish ran); **Docker still re-pushes** the image; no duplicate release | D4.4, D5.2 | | S10 | branch/version classification disagree | validate-release **fails loud**, build/publish skip | D2.2 | | S11 | scheduled upstream-version bump (wrapper) | resolver detects a change -> commits the state file -> opens a `-` PR -> merge-bot auto-merges -> 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, D7.1 | @@ -263,7 +268,7 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the ### 5C. Live Probe (Where Warranted) - Open a trivial-change PR touching one target and confirm S1. -- Drive a `smoke: true` push-probe of the build task for **both** the default and a non-default branch and assert the version classification (clean vs prerelease) and that the gate passes, **without publishing**. *Caveat: the Docker leg logs in to the registry even on smoke and reads the buildcache, so it needs `DOCKER_HUB_*` secrets and cannot run on a fork PR (same-repo only).* +- Drive a `smoke: true` push-probe of the build task for **both** the default and a non-default branch and assert the version classification (clean vs prerelease) from the `get-version` job's outputs, **without publishing**. The release gate is not evidence here, since D2.2 has it exit early and report success on smoke. *Caveat: the Docker leg logs in to the registry even on smoke and reads the buildcache, so it needs `DOCKER_HUB_*` secrets and cannot run on a fork PR (same-repo only).* - Per registry: after a real publish, query NuGet.org for the expected version + prerelease classification (and the `.snupkg` on the symbol server), and confirm a re-run added no duplicate. For PyPI inspect the `Compute PyPI version step` log and the built `dist/*` filenames for `.dev0` off `develop` vs a plain version on the default branch. - Inspect the latest real publish's logs for `PublicRelease`/`SemVer2` per leg and confirm the artifact lifecycle (uploaded, consumed, deleted, with none left behind). - **The deploy ref gate (S13) is verified only by tripping it, and the dispatch that trips it is the maintainer's to run.** Dispatch the production environment from a non-default ref and expect the run to fail at the gate. The evidence is four things, and each of them matters: the gate job's conclusion, its error text naming the expected and the received ref, every downstream job recorded as **skipped** rather than passed, and the deployment count against the production environment unchanged. Capture all four, because a gate that fails open and a gate nobody tripped produce the same empty run history, so "we have never seen it fail" is not evidence about the one control standing between a mis-dispatch and the live site. **The agent prepares the command and reads all four back afterwards. It does not fire it.** An agent harness may refuse to dispatch a production deploy, which is the harness working as intended, and the refusal is neither re-shaped into a raw API call nor talked around (GOVERNANCE.md "Repository Boundaries and Write Safety"). The same split applies to any probe that acts on the deploy host directly, an outbound SSH exercising a forced command among them. @@ -281,13 +286,13 @@ The workflow is **operational** iff every *applicable* 5A item passes and every Each type maps the *applicable* S-scenarios onto its targets. The differences are which leaf tasks exist and what each produces, which 5A addenda apply, and which scenarios are N/A. Walking these is the self-check that the contract holds for each shape. -- **.NET publish.** The target runs a sequential `dotnet publish` runtime loop inside one composite-action job. Configuration is Release on `main` and Debug otherwise. A non-smoke run builds the full runtime set, zips the combined output, and uploads `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 two-runtime subset and skips the zip and upload steps, so it uploads nothing. S1 smoke-builds that subset after a .NET project change. 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 both pushes (`dotnet nuget push *.nupkg --skip-duplicate`, gated `if: push` only) and uploads `release-asset--nuget`. 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 asset zip 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. +- **.NET 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 two-runtime subset and skips the archive and upload steps, so it uploads nothing. S1 smoke-builds that subset after a .NET project change. 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. Section 3's package-registry seam 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, **unconditionally on consume**, so on S9 it is deleted even though the `release-asset-*` delete is skipped. The version is `AssemblyFileVersion` with `.dev0` appended on `develop` only, and must stay `--pre`-selectable and sorted above the default release. 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 image.** The leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only, with a per-branch registry buildcache (`buildcache-`; a multi-image repo adds a per-image tag) (`cache-to` only the built branch and only on push, `cache-from` both branches); 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`; the docker-readme task validates `repositories` XOR `manifest`+`manifest-jq` 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). A **wrapper** repo tracks an upstream release: the upstream tracker writes a `name -> version` state file and the merge-bot auto-merges the bump PR (S11), and the leaf MUST read that file for the immutable tag instead of `SemVer2` (the tracker ships without this consumer wiring). Test: S7 default leg pushes `latest` + the version tag and updates the readme. Non-default pushes the develop tag (amd64 only). S9 still re-pushes. S11 ships the bumped upstream version next publish. 5C Docker probe needs `DOCKER_HUB_*` secrets and same-repo (not fork) runs. -- **Data / asset library.** A single new leaf: validate -> zip -> upload `release-asset--library` (`retention-days: 1`, upload gated `!smoke`, mirroring the NuGet leaf's shape). Because no such leaf ships, you **add a target** (D6.4): a new `enable_library` input + `build-library` job + `github-release` `needs:` entry in the release task, and a `library` paths-filter entry + `changes` output + `smoke-build` enable-forward in the PR workflow (without it, D1.1 never smoke-builds the library). Keep `expect_release_assets: true` (it has a file target, unlike Docker). The .NET `unit-test` job is replaced by a type-appropriate validator with the aggregator **and** `smoke-build` both re-pointed to it (D1.2/D1.5). `version.json` + the NBGV `get-version` step 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 *scheduled* re-run release-create + asset-delete skip (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run **refreshes** the release and re-runs the asset-delete (the asset is re-uploaded then re-deleted). N/A: the NuGet, PyPI, Docker, and .NET publish 5A addenda and their scenario clauses. +- **Data / asset library.** A single new leaf: validate -> zip -> upload `release-asset--library` (`retention-days: 1` per D5.4, upload gated `!smoke` per D1.3). Because no such leaf ships, you **add a target** (D6.4): a new `enable_library` input + `build-library` job + its `github-release` and `build-docker` `needs:` entries in the release task, and a `library` paths-filter entry + `changes` output + `smoke-build` enable-forward in the PR workflow (without it, D1.1 never smoke-builds the library). Keep `expect_release_assets: true` (it has a file target, unlike Docker). The .NET `unit-test` job is replaced by a type-appropriate validator, with the aggregator re-pointed to it (D1.2). `smoke-build` keeps `needs: [changes]`, as 5A's D1 line and the canonical stub both have it. `version.json` + the NBGV `get-version` step 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 *scheduled* re-run release-create + asset-delete skip (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run **refreshes** the release and re-runs the asset-delete (the asset is re-uploaded then re-deleted). N/A: the NuGet, PyPI, Docker, and .NET publish 5A addenda and their scenario clauses. - **Source-only / no build.** There is no package/image build leaf. A repo may own the reusable release task or call its hub-hosted copy. The dispatch-only `publish-release.yml` reaches the reusable plan, validation, and release tasks. Its publish job passes `github: true`, every `enable_*` input as false, and `expect_release_assets: false`. This produces tag + source zip + README + LICENSE with no asset download. With no target, the paths-filter matches nothing. A retained `smoke-build` job is therefore **structurally always skipped**. The repo may instead drop that never-running job. Validation remains the replaced, non-.NET validation job. The aggregator and any retained `smoke-build` job must depend on it (D1.2). NBGV and `version.json` own the tag. The publish job depends on the same reusable validation task that the PR workflow runs. This prevents a dispatch from releasing a ref that fails validation. Applicable scenarios are S1 (validation only), S7, S8, S9, and S10. S7 covers the tag-only release, S8 the dispatch guard, S9 no-op republish, and S10 the classification gate. S2-S6, D5/D6 artifact items, and all per-type 5A addenda are N/A. The artifact-lifecycle and registry clauses of S7/S9 are also N/A, not failed. -- **Static site deployed to a host the project owns.** Two independent surfaces, and keeping them apart is the point. The **release** is the source-only shape above, unchanged: a dispatch-only `publish-release.yml` where NBGV and `version.json` own the tag, producing tag + source zip + README + LICENSE. The **deploy** is its own `workflow_dispatch` carrying an `environment` choice input, so redeploying an unchanged commit mints no tag, which matters because redeploying is routine. It runs a ref gate **first**, before anything is installed or written (production from the default branch only, while any ref may reach a non-production environment, since proving a branch before it merges is what that environment is for), then the **same** reusable validation task the PR gate runs, so a dispatch cannot deploy a ref that fails validation, then calls the hub-hosted `deploy-site-task.yml`, binding the same `environment:` on the caller's own job so the one crossing secret, `DEPLOY_SSH_PRIVATE_KEY`, resolves from the GitHub Environment store and can be mapped explicitly rather than through `secrets: inherit`, which a cross-repository reusable workflow cannot use. Concurrency is keyed on the environment with `cancel-in-progress: false`, because a cancelled deploy leaves a release uploaded and unflipped. The task re-asserts the environment name in a job of its own, because the `environment:` binding resolves before any step runs and a `workflow_call` caller is not bound by the dispatch choice list a human sees. Its environment-bound job then: checks out full history (a shallow clone silently changes page metadata), derives the release id **once** and exports it (deriving it twice yields ids seconds apart, and the live check then asserts a version nothing installed), runs a required deploy hook that builds the tree with whatever generator and precompression the site owns, installs the deploy credential from the environment, uploads into a per-release directory hard-linked against the current release and carrying **no** delete flag (at an environment root a delete removes the rollback targets), flips the pointer as a separate atomic step so a failed transfer cannot half-publish, then runs the same hook again to prune old releases and to check the running host (D4.6). Retention (D5.6) is bounded by a declared count with one side recorded as owning it: a deploy whose credential can observe the destination prunes and asserts the count here, while a credential confined **write-only** can neither delete nor read back, so there the prune is a host-side timer and the repo's runbook records that ownership. Widening the credential to bring the prune in-pipeline would trade a real confinement boundary for a check, and is the wrong trade. What the guarantee rejects is neither side owning it. One thing the pipeline cannot assert and the server config must: a non-public environment serving a byte-identical copy must not be indexed, and that default belongs on the side that is harmless in production, since a non-public container missing the value is still behind its gate while a production container inheriting it deindexes the site silently. Applicable scenarios: S1 (validation), the source-only release set S7/S8/S9/S10, and S12/S13 (the deploy dispatch). N/A: S2-S4, every registry scenario, and D5.1-D5.4 (the pipeline uploads no workflow artifact at all, so D5.6 is what applies in their place), all recorded N/A, not failed. +- **Static site deployed to a host the project owns.** Two independent surfaces, and keeping them apart is the point. The **release** is the source-only shape above, unchanged: a dispatch-only `publish-release.yml` where NBGV and `version.json` own the tag, producing tag + source zip + README + LICENSE. The **deploy** is its own `workflow_dispatch` carrying an `environment` choice input, so redeploying an unchanged commit mints no tag, which matters because redeploying is routine. It runs a ref gate **first**, before anything is installed or written (production from the default branch only, while any ref may reach a non-production environment, since proving a branch before it merges is what that environment is for), then the **same** reusable validation task the PR gate runs, so a dispatch cannot deploy a ref that fails validation, then calls the hub-hosted `deploy-site-task.yml`, with the `environment:` declared inside that task rather than on the calling job, since GitHub rejects a job carrying both `uses:` and `environment:`. The one crossing secret, `DEPLOY_SSH_PRIVATE_KEY`, is therefore mapped explicitly under the call's `secrets:`, because `secrets: inherit` does not carry an environment-scoped secret across a cross-repository call. Concurrency is keyed on the environment with `cancel-in-progress: false`, because a cancelled deploy leaves a release uploaded and unflipped. The task re-asserts the environment name in a job of its own, because the `environment:` binding resolves before any step runs and a `workflow_call` caller is not bound by the dispatch choice list a human sees. Its environment-bound job then: checks out full history (a shallow clone silently changes page metadata), derives the release id **once** and exports it (deriving it twice yields ids seconds apart, and the live check then asserts a version nothing installed), runs a required deploy hook that builds the tree with whatever generator and precompression the site owns, installs the deploy credential from the environment, uploads into a per-release directory hard-linked against the current release and carrying **no** delete flag (at an environment root a delete removes the rollback targets), flips the pointer as a separate atomic step so a failed transfer cannot half-publish, then runs the same hook again to prune old releases and to check the running host (D4.6). Retention (D5.6) is bounded by a declared count with one side recorded as owning it: a deploy whose credential can observe the destination prunes and asserts the count here, while a credential confined **write-only** can neither delete nor read back, so there the prune is a host-side timer and the repo's runbook records that ownership. Widening the credential to bring the prune in-pipeline would trade a real confinement boundary for a check, and is the wrong trade. What the guarantee rejects is neither side owning it. One thing the pipeline cannot assert and the server config must: a non-public environment serving a byte-identical copy must not be indexed, and that default belongs on the side that is harmless in production, since a non-public container missing the value is still behind its gate while a production container inheriting it deindexes the site silently. Applicable scenarios: S1 (validation), the source-only release set S7/S8/S9/S10, and S12/S13 (the deploy dispatch). N/A: S2-S4, every registry scenario, and D5.1-D5.4 (the pipeline uploads no workflow artifact at all, so D5.6 is what applies in their place), all recorded N/A, not failed. - **Operational (workflow model, not a build target).** A `workflowModel: operational` repo layers direct commits to `develop` onto the **source-only** release shape above. It has two workflows. The first is a **lint/validation** PR workflow that feeds the required `Check pull request workflow status job`. It uses the generic linters (editorconfig/EOL, markdownlint, cspell, actionlint) plus a domain validator, with **no unit tests**. Examples include Home Assistant `hass --script check_config`, `esphome config`, or a firmware build. Its triggers differ from the `release` model. It runs on pushes to `develop`, pull requests to `[ main, develop ]`, and `workflow_dispatch`. Push validation is advisory. Pull request validation is enforced on `main` and reported but not required on `develop`. The second workflow is the standard **source-only publisher** with `releaseTrigger: dispatch-only`. NBGV and `version.json` own the tag. The reusable release task creates tag + source zip + README + LICENSE. **The PR trigger names both branches, and naming `main` alone is a defect.** Omitting `develop` starts no validation when a PR opens against `develop`. The aggregator then never reports, and the PR appears clean with an empty check list. D1.2 forbids that output. Naming both causes a duplicate run after a PR merge. The change validates on the PR and again on the resulting push, regardless of merge method. The operational `develop` ruleset prescribes no merge method. The concurrency group uses the workflow name plus `${{ github.ref }}` (Section 2). A pull request uses `refs/pull//merge`, while its push uses `refs/heads/develop`. The runs occupy different groups and neither cancels the other. Pay that cost. The lint-only gate costs only a few runner-minutes. Suppressing the push requires distinguishing a merge commit from a direct commit, which restores the ambiguity the trigger set removes. S1 applies to every PR, including promotion and `develop` PRs. The source-only S7, S8, S9, and S10 scenarios also apply. Bot-push and schedule paths in S5/S6 are N/A, as are every build and registry scenario. See the branch-model note in Section 3 and [GOVERNANCE.md "Branching Model"][governance-branching-model]. From aca9dfe8e5771cc3f9344bc82a8e2ac413c0ff50 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 12:32:04 -0700 Subject: [PATCH 2/5] Carry the Skills That Still Teach the Superseded Publish Model A local pass found that carrying WORKFLOW.md forward left the carried Skills behind, and those are what an agent actually loads at the moment this matters. release-publish-mechanics.md said "NuGet pushes from inside the build-nuget hook" and named that hook as where the package push lives, which is the model this change removes. workflow-ci-contract listed nuget as an orchestrator input the task no longer declares, and its D-guarantees omitted the package publish job's own gate, the build-docker needs entry, and the current D7.2 wording. An agent bumping the pin or restoring a target would have read the pre-fix model and put the push back inside the leaf, reproducing the HTTP 401 this change exists to fix, and an agent auditing the repository would have reported the corrected workflows as drift. No pull request check compares a carried Skill against WORKFLOW.md, and a smoke build never reaches a push, so nothing would have caught it before the next real release. Seven files carried from the hub at 2.0.536, verified byte-identical to it afterward. They were a byte-clean carry of 2.0.526 beforehand, checked file by file, so nothing local was at risk and no probe was owed. That is the opposite of WORKFLOW.md in the previous commit, which did hold a local line. The publish-nuget comment also records an ordering consequence the carried contract does not cover. D4.5 gates the publish job behind a failed build, but the release is cut inside the publish job and this one runs after it, so a failed push leaves a release for a version that never reached NuGet.org. Re-running the publisher is the remedy, both halves being idempotent. That shape comes from the hub's documented stub rather than from anything chosen here. --- .github/skills/agent-conduct/SKILL.md | 2 +- .github/skills/drive-pr/SKILL.md | 38 +++++--- .github/skills/local-strict-review/SKILL.md | 95 ++++++++++++++++++- .../references/release-publish-mechanics.md | 38 +++++--- .github/skills/pr-review-conduct/SKILL.md | 30 +++--- .github/skills/workflow-ci-contract/SKILL.md | 8 +- .../references/d-guarantees.md | 8 +- .github/workflows/publish-release.yml | 5 +- 8 files changed, 172 insertions(+), 52 deletions(-) diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index 060267c..9f20255 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -24,7 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. -- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. +- **PR-bound work runs `local-strict-review` before the claim, and records the pass.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap, and recording that pass with a hub checkout's `scripts/local_review.py`, run with this repository as the working directory since the engine records into whichever repository the cwd sits in, per that skill's own commands. In the repository that authors canonical content others carry, a change moving one of its units owes a second pass over that unit's whole text, recorded with `scripts/canonical_review.py` before the commit, since its ledger is tracked. Where a capture point exists it then checks what applies. Every push toward a pull request owes one, the fix pushes answering review findings included, which is the round it is most often skipped on. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index 1e3fb1f..2bfba18 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -54,28 +54,40 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop -1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Run `local-strict-review` against the branch's current diff, then push the branch and open - the feature -> develop PR if it does not exist yet. +1. Isolate into a worktree per repo-worktree, based on the branch that skill's base rule names, develop unless the task is explicitly about main-only content, before the first edit. +2. Commit the work, then run `local-strict-review` and record its pass in the order that skill + gives, its diff receipt following the commit, and its carried-content record instead preceding + the commit where the change moves a carried canonical unit in the repository that authors one, + because that ledger is tracked. Then push the branch and open the feature -> develop PR if it + does not exist yet. A push refused by a `.husky/pre-push` hook, which the hub carries and a + repository has only if it adds one, is that gate working rather than an + obstacle to route around, and that + skill's refusal table says what each refusal means and what clears it. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. -4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq +4. Capture the branch's own tip before merging, `gh pr view --repo / --json headRefOid --jq .headRefOid`, needed for the verify-then-delete step below since `gh pr merge` itself reports the resulting squash commit on `develop`, not the PR's `headRefOid`. Merge the feature PR into - develop, `gh pr merge [number] --squash --repo owner/repo`. Never `--delete-branch` on this + develop, `gh pr merge --squash --repo /`. Never `--delete-branch` on this call, it is run from inside the task's own worktree per step 1, where the feature branch is checked out, and `gh pr merge --delete-branch` needs to switch that worktree to the base branch to delete it, which fails when `develop` is already checked out somewhere else, the ordinary - case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone: remove - the worktree, delete the now-merged local task branch, then verify before deleting the remote - one, `git ls-remote --heads --exit-code -- origin "refs/heads/"` matches the - `headRefOid` captured above, `--` before `origin` and the fully-qualified ref. `--heads origin + case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove + the worktree and delete the now-merged local task branch, then verify before deleting the remote + one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + so they hold only where the pull request's head branch lives in this repository, which step 1 + guarantees by branching here. A pull request opened from a fork follows + `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would + name the base repository and exit `2` would mean the branch was never there rather than already + deleted. The object id in `git ls-remote --heads --exit-code -- origin "refs/heads/"`, + which prints `\t` so the id is its first field, matches the `headRefOid` captured above, `--` before `origin` and the fully-qualified ref. `--heads origin ""` alone still tail-matches a differently-prefixed branch sharing the same suffix, and `--` placed after `origin` instead of before it is not equivalent either, verified empirically against a `refs/heads/other/--` ref: after-origin also matched it, before-origin matched only the one intended. `--exit-code` distinguishes exit `2`, branch genuinely gone, from any other non-zero exit, a failed query, an unreachable remote and a gone branch both print nothing to - stdout otherwise. Stop and report either a mismatch or a failed query rather than deleting, + stdout otherwise. Exit `2` means the remote branch is already gone, so the delete is done and + the step is complete. Stop and report either a mismatch or a failed query rather than deleting, someone could have pushed to the branch after the merge, or the name could have been reused. `` is the real value, substituted as its own quoted argument (a shell variable expansion such as `"$branch"`, or an argv element), never handed to `eval` or `sh -c` for a @@ -102,8 +114,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply - with its commit SHA (outcome 1). +- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA + (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and + the branch was already reviewed once, and a fix push carries content no pass has read exactly as + the first push did. - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index cad9a06..cecb485 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -1,7 +1,7 @@ --- name: local-strict-review description: >- - Runs one read-only, adversarial review pass against this branch's current diff against its + Runs a read-only, adversarial review pass against this branch's current diff against its target branch, full file context included, on the strongest model tier the session can reach, before a unit of work is pushed toward a pull request or claimed done. Use this whenever staged, committed, or untracked work is about to be pushed on a PR-bound branch, and whenever @@ -11,7 +11,11 @@ description: >- the exact gap this skill exists to close before a PR-hosted reviewer closes it instead. Reuses `code-review`'s "Review the Change" criteria rather than restating them, and owns only this local, pre-PR moment. Once a pull request exists, `pr-review-conduct` and `drive-pr` own - triaging and disposing of what a PR-hosted reviewer finds. + triaging and disposing of what a PR-hosted reviewer finds. Also triggers whenever a change + edits rule text, a Skill, or any other canonical content this repository authors and other + repositories carry, because that content reaches a reviewer whole only when a repository + carries it for the first time, and a second pass reading each changed unit's whole text is + what moves that read into the repository that can act on what it finds. --- # Local Strict Review @@ -22,7 +26,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, at any of the moments this skill is invoked from. An empty diff is not the same as nothing to review, and it is never the signal to stop: it reports no untracked file at all, and it reports nothing for content a commit carries that the working tree has since put back. The untracked-file list below covers the first of those. The second is why the diff pass commits before reviewing, the carried-content pass below running against uncommitted content instead, since a removal or a restore that is committed leaves no net content to miss, and why the engine reads HEAD rather than this diff, its change set coming from the merge base against HEAD, the index and the working tree, so the two answer different questions. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. `git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. @@ -53,15 +57,96 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of **Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. +## Recording the Pass + +`scripts/local_review.py` is what makes this rule checkable rather than something each session has to remember. For the pass above, the engine only records that it happened, keyed on the content the reviewer actually saw, and its `run --backend ` subcommand is the separate case where a headless backend performs the review and records its own count. That receipt is what a capture point reads, the hub's own `.husky/pre-push` hook being the only one today, and a repository having none unless it adds one, since no manifest entry carries it. + +Commit first, then read the digest, then dispatch the subagent, then hand that same value back. Nothing may change the tree between the read and the record. Staging a modified tracked file is such a change, moving the digest although the content did not, and a commit can move it too, since HEAD decides which paths are in the change set at all. Reading after the commit is what leaves neither of them between the read and the record. + +```sh +engine="/scripts/local_review.py" # in the hub itself, scripts/local_review.py +python3 "$engine" status --target # JSON, take contentDigest +# run the pass above, then: +python3 "$engine" record --reviewer agent-skill --target --expect-digest [--findings N] +``` + +Every subcommand here, `run --backend ` included, runs with the repository under review as the working directory, whichever repository that is. The engine takes no `--repo` and reads whichever repository it is run in, so the path names where the script lives and the working directory names what it measures. + +`` is the same branch "What It Does" resolved for the review, passed to both commands. Leaving it off defaults them to `develop`, and on a `main`-based branch that computes the digest against a merge base the reviewer never read, so the receipt would attest to a change set nobody looked at. A receipt is only valid against the target it names, so the two have to agree. + +`--expect-digest` is required rather than optional, and binding it to the earlier read is the whole point. A format-on-save or a hook autofix between the review and the record would otherwise be stamped as reviewed by a pass that never saw it. A refusal there is the content having moved, so the answer is another pass over the current content rather than another read of the digest. + +Record the pass whatever it found, including nothing. The key covers the net content the branch introduces against its target rather than the commit series, so an interactive rebase that leaves the tree alone keeps the receipt valid, and changing one byte invalidates it. + +**Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, and the hook's tree check refuses a push whose tracked content differs from HEAD, so the record has to describe what HEAD holds. A commit that leaves the tree alone usually does not move the receipt's key, so diligence done before it still describes the same content, and a commit putting a path back to its base state drops it from the change set and does move it. Two reasons make the order matter anyway: staging a modified tracked file moves the key even though its content did not change, and a commit made after the record can carry content the pass never read. Reviewing earlier than this is still worth doing as ordinary diligence, and it does not substitute for the recorded pass: the digest read and the record bracket a window in which the tree holds still, and a commit inside that window ends it. + +The engine is hub-hosted per `GOVERNANCE.md` "Hub-Hosted Tooling", so a downstream repository reaches a hub checkout's copy rather than carrying one, which is what the path above is for. + +## The Carried-Content Pass + +A second pass under the same rule, run in the repository that authors canonical content other repositories carry, which in this fleet is the hub. `GOVERNANCE.md` "Verification Discipline" states the rule and why the ordering it corrects is a defect, and is not restated here. What it requires of a run is below. + +**The unit is what a reviewer reads whole**, and `spec/files.json` rather than the document decides which, down to which files carry units at all. `canonical_review.py list` names the whole set and is the authority on it, so the rules are not paraphrased here, where a paraphrase can only drift from them. In the ordinary case a unit is one level-two section of a carried Markdown canonical, and `check` names each one it wants exactly as `record` takes it. The pass reads that unit's whole current text rather than the diff that moved it, because reproducing the carrier's read is the entire point, and a diff with surrounding context is a different read the pass above has already done. + +Run it at the same model tier and in the same delegation shape as the pass above. The brief, the engine, its flags, and the point in the sequence where the record is written each differ, and all four are below. + +```text +Task: adversarial review of one canonical unit, read as a repository carrying it for the first + time reads it, whole, knowing nothing about what this branch changed in it. +Paths: , read in full out of the file that key names. + Read the whole unit, never a diff of it. +Rules that bind this task: , and judge the text + as a reader who has only this unit: a claim it makes about a tool, a path, a command, or + another rule is a defect wherever that claim is false, stale, or unverifiable from the unit + itself, and an instruction it gives is a defect wherever following it literally fails. +Return: one finding per line, the sentence quoted, and what is wrong with it. No severity theater. +Bounds: read-only. Report a rule that looks incomplete rather than guessing at what it meant. + +``` + +```sh +git fetch origin # stop and report a failed fetch rather than measuring past it +python3 scripts/canonical_review.py check --target # each uncovered unit, with its digest +# run the pass above over each unit it named, then, per unit: +python3 scripts/canonical_review.py record --reviewer agent-skill --unit '=' [--findings N] +``` + +These run in the authoring repository itself, which is the only repository this pass ever runs in, so the engine path is the plain one and there is no downstream side needing the `/` form the pass above shows for its own reach. Point an engine in one checkout at another checkout's tree and the second is measured with the first's unit model, while `record` stamps the ledger with a commit read from the second. + +`` is the branch this work targets, resolved once as the pass above resolves it and passed to `check` explicitly. Left off it defaults to `develop`, so a branch based on `main` is measured from a fork point nobody read. The fetch matters for the same reason it does above: the engine resolves `origin/` if it already exists and never fetches it, so a stale remote-tracking ref moves the fork point without saying so. Lagging, which is the ordinary way to be stale, moves it back and gates units this change never touched, and the reverse case, where the branch restores text the target has since changed, drops one it did move. Neither is announced, so the fetch is what keeps the fork point meaning what the reviewer read against. `check` names each uncovered unit with the digest to hand back, so nothing has to be looked up separately, and `list` is there for reading the whole set rather than for this loop. + +The digest is bound to the read for the same reason `--expect-digest` is above: recording a unit by name alone would stamp whatever the file holds at record time, so an edit between the review and the record would be attested to by a reviewer who never saw it. Record each unit whatever the pass found, including nothing. Fixing a finding is itself such an edit, so `record` then refuses the digest you were holding: that refusal is the content having moved rather than a fault in the record, and the answer is a read of the unit's new text, which is what a carrier will actually receive, recorded at its new digest. + +**This pass records before the commit, where the pass above records after it**, and the two orders are opposite because the two records live in different places. A receipt sits in the worktree's git directory and can never be committed, so it is written once the commit has fixed what a push will deliver. This ledger and its burn-down are tracked files the commit has to carry, so writing them after that commit leaves the tree differing from HEAD, which is a state the pre-push hook refuses before either gate runs. The shortest order meeting both, and the one the refusal table below assumes, is: run this pass and record each unit, commit that together with the change, then read the digest, run the diff pass, record its receipt, and push. Committing the change first and the ledger in a second commit satisfies the same constraint and costs a commit. + +**A unit nothing has read here yet is not this branch's debt.** `check` refuses the units this change moved, meaning the ones whose text it edited and the ones it newly carried, since widening the manifest hands a carrier content for the first time exactly as writing it would. Everything else is a burn-down entry in the hub's `reports/canonical-review.md` rather than a block on unrelated work. Working one of those off is worthwhile, and it is its own change rather than a tax on an unrelated one. + ## Disposing of Findings -Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." +Every finding maps to one of `pr-review-conduct`'s five outcomes, at whichever moment this pass ran: fixed (1), evidence-disproven (2), escalated to the maintainer for an explicit call (3), filed as a deferred issue (4), or, if it keeps recurring, taken as a signal to fix the class (5). Outcome 2 is the agent's own on its own evidence, covering a finding that is not real and one that is structurally out of scope. A finding judged real and left unfixed is never the agent's alone, so outcome 3 needs the maintainer's explicit answer in the same turn, reached only once outcome 2 is ruled out, and outcomes 4 and 5 reach the maintainer too, for the deferral and for the rule itself. Running this pass is required before every push toward a pull request, per `agent-conduct`. Two claims sit next to each other here and they point opposite ways, so they are stated apart rather than in one sentence. **The pass is mandatory**, and where a capture point enforces it, a push carrying content no recorded pass covers is refused. That refusal is the gate working rather than a fault to route around. **The findings stay advisory**, and the count a pass raises gates nothing at all, since a pass records that a review ran and never that the content is clean. The disposition above is what closes each finding, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." ## When to Run It - Before the first push toward a pull request (`drive-pr`'s Drive Loop step 2, `pr-review-conduct`'s Expected review loop step 1). - Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. +- Before pushing a change that edits canonical content other repositories carry, or that newly carries some by widening the manifest, over each unit `check` names, per "The Carried-Content Pass" above. + +In the hub, `.husky/pre-push` checks the receipt, and the canonical-unit coverage beside it, at the push itself, so the moments above are where each pass is run rather than the only places it is noticed. A blocked push usually means one of those passes was skipped. Both capture points, that hook and the pull request one named below, are the hub's own, and a repository carrying this Skill has neither until one is carried to it, which is what makes the moments above the layer that actually binds everywhere. The hook is a backstop under this skill and not a replacement for it: it fires only in a clone that enabled `core.hooksPath`, it says nothing about a repository that carries no such hook, and it is bypassable by design, `--no-verify` being the documented route for a genuine pickle rather than for a diff nobody read. That route is not open in every seat. A Claude Code session running the fleet's agent-safety hook has the flag denied unconditionally, so where the rows below say a bypass is the answer, the answer in that seat is to report the state and hand the push to the maintainer rather than to force it. The hub's own `.github/actions/validate` composite action runs the canonical-unit half again as a step on every pull request into `main` or `develop`, which is what its workflow triggers on. That one needs no hooks path, runs whether or not any clone enabled one, and `--no-verify` does not reach it, which is what makes it the capture point a push cannot bypass where it applies. + +**Read the refusal itself, which names its own case.** Some of the rows below are cleared by running a pass and some are cleared by nothing of the kind, and each row says which, so no count of either is kept here to go stale against the table. Some the hook decides before either engine runs, so there is no engine message under them, and the rows say where each one's detail comes from. + +| The refusal says | What it means | What clears it | +| --- | --- | --- | +| No local review covers this branch's current content | The ordinary missing pass: no recorded receipt covers what this push delivers, either because none was recorded or because the content moved after one was | One pass over the branch's whole diff, recorded per "Recording the Pass" above | +| Tracked content differs from HEAD | A push delivers HEAD while a receipt covers the index and working tree, so the receipt does not describe this push. The hook prints the same headline for an unresolved merge and for a `git update-index --refresh` that exited above 1, naming each on its own line | Commit what is being pushed, then the pass, then the record. Where the change also moved a canonical unit, follow "The Carried-Content Pass" order instead, since committing first strands that ledger after the commit and each fix then lands on another row. Resolve the merge first where the hook names one, and run `git status` first where it names the refresh, since the content may not differ at all | +| The commit is not this worktree's HEAD | Any pushed branch ref carrying an object id that is neither this worktree's HEAD nor the all-zero id of a delete, which a push from a checkout sitting elsewhere reaches and so does a multi-ref push such as `git push --all` | Push one branch, the one this worktree holds. Where another branch is the one wanted, check it out in its own worktree first, per `repo-worktree` | +| Any wording saying the gate did not or could not run | An execution boundary rather than a verdict, which blocks because a gate that waves a push through when it could not run has stopped gating. The cause is named in that same message or in the engine error printed above it, and it is a missing Python interpreter, an unresolvable target, an unreadable receipt, a git command that failed, a manifest or ledger the engine could not read, or any unexpected failure | Whatever the message names, most often installing an interpreter per `docs/host-setup.md` or fetching the target branch. Never another pass | +| This branch changes N carried canonical unit(s) that no recorded pass covers | The carried-content pass was skipped for a unit this change moved or newly carried, and the refusal names each one with the digest to hand back | One carried-content pass per named unit, then `canonical_review.py record` for each, in the order "The Carried-Content Pass" above gives. The ledger that writes is tracked content, so the commit has to carry it and the diff pass comes after | +| A canonical refusal naming units this branch never touched | The fork point is not where the reader thinks it is. Either `origin/` does not hold the commit this branch forked from, since neither engine ever fetches it, or the branch is based on something other than `develop` and the hook, which passes no `--target`, measured it against `develop` regardless. Unlike the row below it still prints a record command, and taking that one records passes over units nobody read | `git fetch origin `, then `canonical_review.py check --target ` by hand for the real set, then pass and record what that names and commit the ledger with the change, per the carried-unit row above. Where the branch targets something the hook does not measure, no pass clears it, so the gate cannot judge that branch at all and the bypass is its answer, as in the row below | +| The recorded pass was run against X and this check measured Y, printed under the missing-pass headline | The hook reads `develop` and nothing else, so a branch based elsewhere is measured against `develop` whatever the pass targeted, and the engine deliberately prints no record command, since the one it would print records a pass over a diff nobody read | One more pass against the branch this work actually targets, where it does target the measured one. Where it does not, the gate cannot judge the branch at all and the bypass is its answer | + +This table is the fleet's one enumeration of these, and every other surface states the principle and routes here rather than listing the shapes. That is deliberate: every review round that added a shape also left a restatement of it somewhere else, and keeping one table is what stops the next round doing the same. ## Mechanics Live Elsewhere @@ -69,3 +154,5 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". - Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. - Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. +- The receipt's key, its backends, and the three-valued exit contract a capture point folds: `scripts/README.md` "`local_review.py`". +- The unit model, the coverage ledger, and the burn-down report: `scripts/README.md` "`canonical_review.py`". diff --git a/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md b/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md index ad7253f..a95b734 100644 --- a/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md +++ b/.github/skills/operational-vs-release-workflow/references/release-publish-mechanics.md @@ -20,10 +20,12 @@ publisher passes `branch: ${{ github.ref_name }}`, which the tasks forward and r `build-release-task.yml` is a hub-hosted task with per-target `enable_*` inputs, so a repo drops a target by setting its `enable_: false` at the caller stub rather than deleting a job: the hub task carries the full job graph for every repo, and the caller stub's `with:` block is where -the target list is expressed. A repo still curates its path-filter entry in -`test-pull-request.yml`, and (for PyPI) the `publish-pypi` job in its own `publish-release.yml`, -since `id-token: write` belongs at that one entry point. CodeGen, versioning, badge, merge-bot, -and Dependabot are target-agnostic. +the target list is expressed. A repo still curates, in `test-pull-request.yml`, its +path-filter entry, that filter's output, and the `smoke-build` enable-forward, all three together +per D6.4, since an entry nothing consumes never smoke-builds the target. And, for a package +target, the `publish-nuget` or `publish-pypi` job in its own `publish-release.yml`, since +`id-token: write` belongs at that one entry point. CodeGen, versioning, merge-bot, and Dependabot +are target-agnostic. ## Orchestration vs. build: the override seam @@ -62,22 +64,30 @@ and project-path inputs its targets need. Pick by where each artifact *goes*, not by language: - **Files attached to the GitHub Release** (zips, binaries, packaged libraries): a dotnet-publish - hook or a build-nuget hook per output, each uploading `release-asset--`. This is where the - .NET `dotnet publish` or `dotnet build` and package push lives. The hub default takes an explicit + hook or a build-nuget hook per output, each uploading `release-asset--`. This is where the + .NET `dotnet publish` or `dotnet build` lives, though a package push does not. The hub default takes an explicit project path, and a project needing different build behavior replaces the hook. A data-only repo's own output (e.g. a symbol library) is not yet expressible as a hub hook or an `enable_*` input, so it stays a carried leaf until the hub task grows one. -- **Package-registry pushes** (NuGet.org, PyPI): the target both builds **and** publishes to its - registry. NuGet pushes from inside the build-nuget hook (OIDC trusted publishing through - `NuGet/login`, no stored API key) *and* also uploads a `release-asset-*` (.7z) for the GitHub - release. PyPI is split: the build-pypi hook only builds and uploads the +- **Package-registry pushes** (NuGet.org, PyPI): both are split, and the push never sits in the + hook. OIDC trusted publishing validates the token's `job_workflow_ref` claim, which names the + workflow the job actually ran from, so a push from a hub-hosted task is rejected at the token + exchange, NuGet.org answering `HTTP 401` and PyPI under its own code. And because D7.2 has a callee + declare `permissions:` only where every caller grants that scope at startup, the release task's + jobs declare none and run under the calling job's whole grant, so a push anywhere inside that + task would put `id-token: write` on every job in it. The + build-nuget hook uploads a `nuget-build-` artifact for a separate `publish-nuget` job in + the caller's own `publish-release.yml`, which authenticates through `NuGet/login` and therefore + carries `id-token: write` (plus `actions: write` to delete the artifact it consumed), *and* also + uploads a `release-asset-*` (.7z) for the GitHub release. PyPI is the same shape: the build-pypi hook only builds and uploads the `pypi-build-` artifact, and the separate `publish-pypi` job in the caller's own - `publish-release.yml` does the OIDC Trusted-Publishing upload (`id-token: write` is granted only - at that one entry point), and PyPI contributes **no** `release-asset-*`. + `publish-release.yml` does the OIDC Trusted-Publishing upload, behind an `environment: pypi` + gate and with `skip-existing: true` (`id-token: write` is granted only at that one entry + point), and PyPI contributes **no** `release-asset-*`. - **Image-registry pushes** (Docker Hub): `build-docker-task.yml`, hub-hosted like - `build-release-task.yml`, pushes multi-arch tags directly and contributes **no** - `release-asset-*`. The image set comes from a docker-prepare hook (the hub default emits the + `build-release-task.yml`, pushes the default branch multi-arch (amd64+arm64) and any other + branch `amd64`-only, and contributes **no** `release-asset-*`. The image set comes from a docker-prepare hook (the hub default emits the single vanilla entry an `image` input implies). A multi-image or upstream-pinned repo carries its own hook, and a shared base layer comes from a required docker-build-base hook with no hub default. To publish the Docker Hub repository overview, the hub-hosted `publish-docker-readme-task.yml` diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 810955e..836fdb3 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -78,13 +78,16 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. -Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. Follow that skill's own ordering and record each pass, which is what a capture point reads, the hub's own `pre-push` hook being one and a repository having none until such a hook is carried to it. A push that hook refuses, where one is present, is the gate working rather than an obstacle to route around, and that skill's refusal table says what each refusal means and what clears it. 1. Push changes to the PR branch and open the pull request when it does not exist. -2. Run `scripts/pr_review.py status` once in the foreground and read its output. +2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. 3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly (mechanics in the Copilot runbook). The UI is a fallback only. -4. Run a bounded `scripts/pr_review.py wait` in a background process and read its terminal output. + explicitly (mechanics in the Copilot runbook, `.github/copilot-instructions.md`), which step 4's + `wait` also does on its own, though it skips the request where a review already covers the head, + where the answer came outside a formal review, and where it detects drift. The UI is a fallback + only. +4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one exception, and it is terminal the other way. Nothing follows it, and re-requesting the same @@ -94,7 +97,7 @@ Run `local-strict-review` against the branch's current diff before step 1's push 7. Reply to each thread and resolve what was addressed. 8. Re-run the loop after every fix push until the checks are green and no finding remains open. -The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `Lite`, `Balanced`, or `Max` when the completed review exposes that metadata, and distinguishes an inherited `Default ()` from an explicit choice. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. +The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping @@ -105,8 +108,10 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing - the fix, then reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push + went, then reply with the fixing commit SHA. A branch already reviewed once + has not been reviewed for the fix, which is the round this gets dropped on and the churn + `local-strict-review` exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. @@ -156,7 +161,7 @@ reviewer's own words to identify it), give one bold verdict per finding (`Fixed `Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be checked against findings, and link the review round. **Read every round, not only the head.** A suppressed finding does not retire when a later push supersedes it, it just stops showing up in a -head-scoped query while still unanswered. Post the answer with `scripts/pr_review.py comment` +head-scoped query while still unanswered. Post the answer with `scripts/pr_review.py comment --repo / --body ` from a hub checkout. Do not use a provider connector or reconstruct the GitHub mutation. ## Escalate to the maintainer when @@ -170,9 +175,12 @@ from a hub checkout. Do not use a provider connector or reconstruct the GitHub m ## Mechanics Live Elsewhere This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for -the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and +the GitHub-specific API operations, each taking ` --repo /`. `claims` checks the pull +request description against the branch it describes, catching a commit or `uses:` ref the head no +longer carries. `status` reports coverage, threads, body-only findings, and shapes in one call. `wait` requests and polls in-process. `comment` posts a PR-conversation -answer after it reads the PR node ID. `reply` resolves a thread by matching the finding's own -words instead of a line number a fix push can move. The repository's +answer after it reads the PR node ID. `reply` answers a thread by matching the finding's own +words instead of a line number a fix push can move, and resolves it only when `--resolve` is +given. The repository's `.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable coverage marker. Do not reconstruct the API operations by hand. diff --git a/.github/skills/workflow-ci-contract/SKILL.md b/.github/skills/workflow-ci-contract/SKILL.md index e02fdfb..54d97c1 100644 --- a/.github/skills/workflow-ci-contract/SKILL.md +++ b/.github/skills/workflow-ci-contract/SKILL.md @@ -16,16 +16,16 @@ description: >- - **Applicability.** A guarantee governing a construct the repo does not contain is N/A: recorded, excluded from the verdict, never a defect. A source-only pipeline is mostly N/A and that is fine. - **Operational is binary.** Every applicable guarantee holds, or the workflow is not operational. A single applicable input-output mismatch is a defect regardless of how clean the YAML looks. - **Reached, not carried.** A standard workflow whose job graph is identical across repos of a type is a `workflow_call` task the hub hosts once, and a repo carries only a caller stub pinned to a hub release commit plus a composite-action hook at `.github/actions/` for what is its own. A hub task reaches its own actions and sibling tasks through `$/`, which resolves at that pinned commit. The merge-bot is the first, and `docs/reusable-workflows.md` in the hub carries the model, the hook contract, and the phase each workflow migrates in. Until a workflow's phase ships, its copy is graded as below. -- **Two layers.** Orchestration (the PR entry workflow, publisher, version/release/badge jobs) is generic and standard at the job level. Build leaves (`build--task.yml`) are repo-owned. Inputs like `github`/`nuget`/`dockerhub`/`expect_release_assets` live on the orchestrator, a leaf only receives `ref`/`branch`/`smoke` and a derived `push`, so assert each input in the layer that declares it. What a repo curates is the list of targets, and adding or dropping one edits the whole surface together: the `enable_` input, the `build-` job and its `github-release` `needs:` entry, the `changes` paths-filter entry and output, and the `smoke-build` enable-forward (D6.4). +- **Two layers.** Orchestration (the PR entry workflow, publisher, version and release jobs) is generic and standard at the job level. Build leaves (the `build-` tasks) are repo-owned. Inputs like `github`/`dockerhub`/`expect_release_assets` live on the orchestrator, a leaf receives `ref`/`branch`/`smoke` and whatever else its target needs, a derived `push` among them where that leaf pushes, so assert each input in the layer that declares it. A package target declares no push input on either layer, its push living in a separate `publish-` job in the repo's own publisher. What a repo curates is the list of targets, and adding or dropping one edits the whole surface together: the `enable_` input, the `build-` job and its `github-release` and `build-docker` `needs:` entries, the `changes` paths-filter entry and output, the `smoke-build` enable-forward, and a package target's `publish-` job (D6.4). ## Style Rules That Break in One-Line Diffs - **Pin every action to a commit SHA** with a trailing `# vX.Y.Z` comment, first-party included. The one documented no-pin exception is `dotnet/nbgv@master`. Invent no others. - **Names carry meaning**: `-task.yml` files and "task" names are reusable (`on: workflow_call`), entry points end in what they do and their names end in "action", every job `name:` ends in "job" and every step in "step". A ruleset-bound required check's job `name:` and the ruleset `context:` are one string renamed together, in the live ruleset and the hub's `repo-config/` payloads in lockstep, or required-check enforcement silently breaks. -- **Concurrency**: top-level workflows use `group: '${{ github.workflow }}-${{ github.ref }}'` with `cancel-in-progress: true`. The publisher is the documented exception: a global ref-independent group with `cancel-in-progress: false`, so publishes serialize and never cancel mid-push. +- **Concurrency**: top-level workflows use `group: '${{ github.workflow }}-${{ github.ref }}'` with `cancel-in-progress: true`. Two are documented exceptions. The publisher takes a global ref-independent group with `cancel-in-progress: false`, so publishes serialize and never cancel mid-push. The merge-bot takes `cancel-in-progress: false` and keys on the PR number rather than `github.ref`, per D8.1, so each PR queues independently and every event runs to completion. - **Shells**: every multi-line bash `run:` starts `set -Eeuo pipefail`. Multi-line `if:` uses `>-`, never `|`. - **Boolean inputs** are declared in both trigger blocks and compared against both forms, `${{ inputs.foo == true || inputs.foo == 'true' }}`, since `workflow_dispatch` delivers strings. -- **Permissions validate before `if:`**, so even a skipped job needs valid `permissions:`, and a callee's extra scope (`actions: write`, `id-token: write`) is granted by the caller at the one entry point that needs it. +- **Permissions validate before `if:`**, so a callee declares `permissions:` only where every caller grants that scope at startup and otherwise declares none, running under the calling job's grant. A callee's extra scope (`actions: write` for cleanup) is granted by the caller at the one entry point that needs it. - **Chaining across optional jobs** allowlists `success`/`skipped` explicitly, because `!= 'failure'` lets `cancelled` through. - **Docker layer cache** targets a registry tag (`buildcache-`), never `type=gha`. - **Workflow YAML is LF.** Preserve endings on every edit. @@ -38,7 +38,7 @@ description: >- - **The seam contract**: a target contributes a release file by uploading `release-asset--`, and the release job collects by `pattern:` plus `merge-multiple:`, never `artifact-ids:`, canonical even for a single target. A repo with no file target passes `expect_release_assets: false` at the caller. - **Artifacts are an intra-run handoff**: consume-then-delete at the point of consumption, gated to the consumer's condition, best-effort, `retention-days: 1` on every upload as the backstop, and never a blanket delete of the run's artifact set, which destroys the diagnostics you need when the run fails. - **No-op republish**: an unchanged version re-pushes nothing, the release-create step skips when the tag exists, registries dedupe server-side (`--skip-duplicate`, `skip-existing: true`), and Docker alone always re-pushes by design. -- **A build failure blocks every publish target**: `github-release` needs every build, and the terminal registry pusher guards with `!failure() && !cancelled()`, so nothing partial ships. +- **A build failure blocks every publish target**: `github-release` needs every build, the terminal registry pusher guards with `!failure() && !cancelled()`, and a package target's separate `publish-` job `needs:` the release-task call, so nothing partial ships. The full catalog, each guarantee with the failure mode it prevents, is in `references/d-guarantees.md`. Auditing, tracing, and probing a repo's workflows is `references/test-methodology.md`. diff --git a/.github/skills/workflow-ci-contract/references/d-guarantees.md b/.github/skills/workflow-ci-contract/references/d-guarantees.md index b41c99a..b32348b 100644 --- a/.github/skills/workflow-ci-contract/references/d-guarantees.md +++ b/.github/skills/workflow-ci-contract/references/d-guarantees.md @@ -32,13 +32,13 @@ Each guarantee is a MUST from `WORKFLOW.md` section 4, stated as input to output - **D4.2** `target_commitish` is the built commit's SHA (NBGV `GitCommitId`), never a branch name and never `github.sha`. - **D4.3** Every release is a tag plus source zip, README, and LICENSE, file targets attach `release-asset-*`, and a no-file-target caller passes `expect_release_assets: false` or the release-create step fails on unmatched files. - **D4.4** No-op republish: an unchanged version re-pushes nothing, the release-create skips when the tag exists (refreshed only on `workflow_dispatch`), registries dedupe server-side, and Docker always re-pushes by design. -- **D4.5** A failed build blocks every publish target: `github-release` needs every build, the terminal registry pusher guards `!failure() && !cancelled()`, so nothing partial ships. +- **D4.5** A failed build blocks every publish target: `github-release` needs every build, the terminal registry pusher guards `!failure() && !cancelled()`, and a package target's separate `publish-` job `needs:` the release-task call, so nothing partial ships. - **D4.6** A deploy check asserts which release and which environment answer, waiting for convergence to a bounded timeout, with an unreachable host reported distinctly from an HTTP status. ## D5: Resource Cleanup - **D5.1** A cross-job transfer artifact is deleted at its point of consumption. An in-run intermediate may rely on the retention backstop. -- **D5.2** The delete runs under the same condition as its consumer, so a no-op re-run skips the release-asset delete while the PyPI build-artifact delete still runs. +- **D5.2** The delete runs under the same condition as its consumer, so a no-op re-run skips the release-asset delete while the `nuget-build-*` and `pypi-build-*` deletes still run. - **D5.3** Cleanup is best-effort (`continue-on-error`, tolerate a failed listing, delete all matching ids). - **D5.4** Every `upload-artifact` sets `retention-days: 1`. - **D5.5** Never blanket-delete the run's artifacts, which destroys diagnostics and auto-emitted build records. @@ -49,12 +49,12 @@ Each guarantee is a MUST from `WORKFLOW.md` section 4, stated as input to output - **D6.1** The release job downloads by `pattern:`/`merge-multiple:`, never `artifact-ids:`, canonical for single-target repos too. - **D6.2** Branch-derived config reads `inputs.branch`, never `github.ref_name`. - **D6.3** Artifact names are branch-suffixed. -- **D6.4** A target add or drop updates the whole surface together: `enable_` input, `build-` job, `github-release` `needs:` entry, paths-filter entry and output, and the `smoke-build` enable-forward. +- **D6.4** A target add or drop updates the whole surface together: `enable_` input, `build-` job, its `github-release` and `build-docker` `needs:` entries, paths-filter entry and output, the `smoke-build` enable-forward, and a package target's separate `publish-` job. ## D7: Concurrency, Permissions, Safety - **D7.1** The publisher serializes: global ref-independent concurrency group, `cancel-in-progress: false`. -- **D7.2** Every reusable job declares valid `permissions:` (validated before `if:`), a callee's extra scope granted by the caller. +- **D7.2** A reusable job declares `permissions:` only where every caller grants that scope at startup (the block is validated before `if:`), and otherwise declares none and runs under the calling job's grant, a callee's extra scope granted by the caller at the one entry point needing it. - **D7.3** Boolean inputs are declared in both trigger blocks and compared against both forms. - **D7.4** Optional-dependency chaining allowlists `success`/`skipped` explicitly. diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index aa8a12b..f1bef4c 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -72,9 +72,10 @@ jobs: enable_dotnet_publish: false nuget_project: ./Utilities/Utilities.csproj - # The push lives here rather than in the hub task so the OIDC token's job_workflow_ref claim names this repository. - # NuGet.org validates that claim against the repository owning the package, and a job running from the hub task carries the hub's workflow path instead (ptr727/ProjectTemplate#1126). + # The push lives here rather than in the hub task so the OIDC token's job_workflow_ref claim names this repository, which NuGet.org validates against the package owner (ptr727/ProjectTemplate#1126). # A skipped publish job skips this one too, so no separate release gate is needed. + # The release is cut before this job runs, so a failed push leaves a release for a version that never reached NuGet.org. + # Re-running the publisher is the remedy, since the release-exists gate and --skip-duplicate are both idempotent. publish-nuget: name: Publish NuGet library job needs: [validate, publish] From 3a54d0df216322f2175d72e29625430e48e6e9b1 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 13:41:55 -0700 Subject: [PATCH 3/5] Repair the Garbled Clause in the SetLimits Remark The sentence read "so no fixed ordering is safe and this applies both before re-partitioning at all", which was meant as "this method applies both limits before re-partitioning at all" and parses as neither. It is the remark a consumer sees in IntelliSense on a published package, and it was explaining the one thing the method exists for, so an unreadable clause there is worth its own fix. Raised by a reviewer on the promotion pull request, #456, whose head is develop and cannot carry a fix. It rides here rather than in a third pull request for one sentence, which costs this one a review round and saves a whole cycle. --- Utilities/StringHistory.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index d654589..d5acb76 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -119,8 +119,9 @@ public override string ToString() => /// Assigning and one after the other /// re-partitions twice, so the first assignment measures against the other limit's previous /// value and can discard lines the final pair would have retained. Which order avoids that, - /// where either does, depends on the values and on what is stored, so no fixed ordering is safe - /// and this applies both before re-partitioning at all. + /// where either does, depends on the values and on what is stored, so no fixed ordering is + /// safe. This method assigns both limits before re-partitioning at all, which is what removes + /// the dependence on order. /// Both limits at zero is the one unrestricted mode, so passing zero twice retains every stored /// line and every later one rather than discarding them. /// From 8892da3dbd5061b20de9bd28e4ef2f5ea529ffb9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 13:53:09 -0700 Subject: [PATCH 4/5] Correct the StringHistory Documentation the Repair Pass Exposed Repairing one clause put the surrounding blocks under review, and several were inaccurate about the code. All of them ship to consumers as XML documentation, so each claim below was confirmed by running the built library rather than by reading. The re-partition claim was unconditional and is not. The class remarks, both property remarks and ARCHITECTURE.md all said assigning a limit re-partitions the stored lines and discards what the new limits exclude. Where the other limit is already zero, the assignment leaves both at zero, Repartition returns early, and nothing is discarded. Probed: new StringHistory(3, 0) holding [a,b,c], then MaxFirstLines = 0, keeps [a,b,c] and every later line, so a caller setting it to zero to stop retaining anything instead removes the bound, on the one class whose purpose is bounding memory. Each site now states the exception. SetLimits said it applies both limits at once without saying it never recovers a dropped line, which invited the inference that widening refills the head. Probed: (2,2) holding [1,2,5,6], SetLimits(5, 2) leaves it unchanged. AppendLine documented neither of the two ways it does not simply store the value. Probed: (2,0) after a,b,c holds [a,b], the line discarded outright, and (2,2) after 1 to 5 holds [1,2,4,5], the oldest retained tail line evicted. ToString was documented as the stored lines with line breaks, which reads as a join. It appends a trailing newline and answers empty for an empty history, both probed, so a caller comparing against string.Join gets a mismatch. StringList read as a snapshot and is a live view, the same instance every time. Probed: a reference captured at [p] reads back [p,r] after two appends, and enumerating it while appending throws InvalidOperationException as any list enumeration does. The type carries no thread-safety guarantee, which was written down nowhere. The parameterless constructor said "with no limits", the phrasing HISTORY.md records this release as having corrected away, and it reads as limits not yet configured rather than the unrestricted mode. HISTORY.md listed where a negative is rejected and omitted SetLimits, which validates both arguments before assigning either. Probed: SetLimits(1, -1) throws on maxLastLines and leaves both limits unchanged. --- ARCHITECTURE.md | 2 +- HISTORY.md | 2 +- Utilities/StringHistory.cs | 40 +++++++++++++++++++++++++++++++------- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5657554..e34519d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -21,6 +21,6 @@ These are behavioral contracts rather than formatting rules, which is why they l - **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo()` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString()` buffers the whole response and a large body belongs in `DownloadFile()` instead. A download to a file truncates and rewrites the destination in place, so its permissions, ownership, and any links to it survive. The destination is truncated once the response headers are accepted rather than once the body has arrived, so a request that fails before that leaves it untouched, while one that fails partway through the body leaves a short file. - **`FileEx`** wraps its I/O in retry logic configured through the static `FileEx.Options`, a `FileExOptions`, and honors cancellation from both `FileEx.Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate and takes a configurable compression level. It works in strings rather than streams, so a caller hands it no stream to own. -- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Zero on a single side retains no lines on that side, and zero on both is the one unrestricted mode. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. `SetLimits()` applies both limits in one re-partition, which assigning the two properties in turn cannot do. +- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Zero on a single side retains no lines on that side, and zero on both is the one unrestricted mode. Either limit rejects a negative value, as does `SetLimits()`, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow, except that an assignment leaving both limits at zero enters the unrestricted mode and discards nothing. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. `SetLimits()` applies both limits in one re-partition, which assigning the two properties in turn cannot do. - **`CompressExtensions`** uses the C# `extension` block form inside a static class for its string helpers, and the internal `LogExtensions` does the same for the logger helpers. - **Logging is a seam, never a dependency.** The library depends on `Microsoft.Extensions.Logging.Abstractions` and takes an `ILoggerFactory` through `LogOptions`. It references no logging framework or sink, so a consumer chooses its own. `Serilog` appears only in `Sandbox` and the tests, where an application legitimately picks one. diff --git a/HISTORY.md b/HISTORY.md index 57aa14e..4040387 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,7 +8,7 @@ Some useful and not so useful C# .NET utility classes. - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination file: both opened it with `File.OpenWrite()`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now truncate the destination explicitly and rewrite it in place, which keeps its permissions, ownership, and any links to it. The truncation happens once the response headers are accepted rather than once the body has arrived, so a download that fails partway now leaves a short file where it previously left the original bytes behind the new ones. - Moved the NuGet.org push out of the shared release task and into this repository's own publisher, restoring publishing. Trusted publishing validates the OIDC token's `job_workflow_ref` claim against the repository owning the package, and a push made from a workflow another repository hosts is rejected at the token exchange, which is what failed the first release attempt after the release chain was adopted. - Added `StringHistory.SetLimits()`, which applies both limits in one re-partition. Assigning `MaxFirstLines` and `MaxLastLines` one after the other re-partitions twice, so the first assignment measures against the other limit's previous value and can discard lines the final pair would have retained. Which order avoids that, where either does, depends on the values and on what is stored, so no fixed ordering is safe. - - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later `AppendLine()`, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. `AppendLine()` changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. + - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor, at the property and in `SetLimits()` rather than at a later `AppendLine()`, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. `AppendLine()` changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - v4.0: - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`. It exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. - Added `AssemblyInfo`, an AOT-safe assembly and application identity helper whose `For()` substitutes for `Assembly.GetExecutingAssembly()` (unreliable under Native AOT), and which supplies the consuming application name, version, and a default User-Agent. diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index d5acb76..2377126 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -8,14 +8,16 @@ namespace ptr727.Utilities; /// while discarding intermediate content when limits are exceeded. Zero on a single side retains no /// lines on that side, and zero on both is the one unrestricted mode, where every appended line is /// retained. A limit assigned after lines have been appended re-partitions what is already -/// stored, so the history never holds more than the limits then in force allow. Re-partitioning -/// only ever discards: once a line has been dropped the head is closed, and a later, larger -/// raises the ceiling without recovering or repopulating it. +/// stored, so the history never holds more than the limits then in force allow, except that an +/// assignment leaving both limits at zero enters the unrestricted mode and discards nothing. +/// Re-partitioning only ever discards: once a line has been dropped the head is closed, and a +/// later, larger raises the ceiling without refilling the head. /// public class StringHistory { /// - /// Initializes a new instance of the class with no limits. + /// Initializes a new instance of the class in the unrestricted + /// mode, both limits zero, where every appended line is retained. /// public StringHistory() => StringList = _stringList.AsReadOnly(); @@ -43,6 +45,11 @@ public StringHistory(int maxFirstLines, int maxLastLines) /// /// Appends a line to the history, respecting the configured limits. /// + /// + /// The line is not always stored. Where the head is full and is + /// zero, the line is discarded rather than retained, and where both sides are full the oldest + /// retained tail line is evicted to make room for it. + /// /// The string value to append. /// Thrown when is null. public void AppendLine(string value) @@ -100,9 +107,13 @@ public void AppendLine(string value) } /// - /// Returns all stored lines as a single string with line breaks. + /// Returns all stored lines as a single string, each line followed by + /// . /// - /// A string containing all stored lines. + /// + /// The stored lines, with a trailing newline after the last, or an empty string where nothing + /// is stored. The trailing newline means this is not a plain join of the lines. + /// public override string ToString() => string.Join(Environment.NewLine, _stringList) + (_stringList.Count > 0 ? Environment.NewLine : string.Empty); @@ -123,7 +134,9 @@ public override string ToString() => /// safe. This method assigns both limits before re-partitioning at all, which is what removes /// the dependence on order. /// Both limits at zero is the one unrestricted mode, so passing zero twice retains every stored - /// line and every later one rather than discarding them. + /// line and every later one rather than discarding them. This applies both limits at once and + /// never recovers a line already dropped, so widening a limit raises the ceiling without + /// refilling the head from what is still stored. /// public void SetLimits(int maxFirstLines, int maxLastLines) { @@ -145,6 +158,9 @@ public void SetLimits(int maxFirstLines, int maxLastLines) /// /// Assigning this re-partitions the lines already stored against the limits then in force, /// which discards whatever the new limits exclude and never recovers a line already dropped. + /// An assignment leaving both limits at zero is the exception, entering the unrestricted mode + /// and discarding nothing, so setting this to zero while the other limit is already zero + /// removes the bound rather than emptying the history. /// Setting both limits therefore applies them one at a time, and the first assignment can /// discard lines the second would have retained. Use to apply both at /// once, or the two-argument constructor when both limits are known up front. @@ -168,6 +184,9 @@ public int MaxFirstLines /// /// Assigning this re-partitions the lines already stored against the limits then in force, /// which discards whatever the new limits exclude and never recovers a line already dropped. + /// An assignment leaving both limits at zero is the exception, entering the unrestricted mode + /// and discarding nothing, so setting this to zero while the other limit is already zero + /// removes the bound rather than emptying the history. /// Setting both limits therefore applies them one at a time, and the first assignment can /// discard lines the second would have retained. Use to apply both at /// once, or the two-argument constructor when both limits are known up front. @@ -187,6 +206,13 @@ public int MaxLastLines /// /// Gets the read-only collection of stored strings. /// + /// + /// This is a live view over the history rather than a snapshot, and the same instance is + /// returned every time, so what a caller holds changes as lines are appended or discarded. + /// Copy it before enumerating alongside an , which would otherwise + /// throw as any list enumeration does when the list + /// changes under it. This type carries no thread-safety guarantee. + /// public ReadOnlyCollection StringList { get; } /// From d8bb1eb800953fec70e1b3d223c85c77f5bd7f28 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 14:01:39 -0700 Subject: [PATCH 5/5] Say Which Condition Evicts a Tail Line, Not That Both Sides Are Full The AppendLine remark added in the previous commit said the oldest retained tail line is evicted "where both sides are full". A reviewer pointed out that does not describe the reachable case, and reproducing it confirms that: a history at (2,2) holding [1,2,4,5], given SetLimits(5, 2), has a head limit of 5 against 2 stored head lines, so the head is plainly not full, and appending still evicts, giving [1,2,5,6]. The head is closed rather than full. Once anything has been discarded a later, larger MaxFirstLines raises the ceiling without reopening it, which the class remarks already say and which this block contradicted. The condition that actually evicts is the tail being at MaxLastLines, independent of what MaxFirstLines allows, and the remark now says that. --- Utilities/StringHistory.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index 2377126..5388b3d 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -46,9 +46,11 @@ public StringHistory(int maxFirstLines, int maxLastLines) /// Appends a line to the history, respecting the configured limits. /// /// - /// The line is not always stored. Where the head is full and is - /// zero, the line is discarded rather than retained, and where both sides are full the oldest - /// retained tail line is evicted to make room for it. + /// The line is not always stored. Where is zero and the head is not + /// taking it, the line is discarded rather than retained. Where the tail already holds + /// lines, the oldest retained tail line is evicted to make room, + /// and that happens whether or not still has room, since the head + /// is closed once anything has been discarded and a later, larger limit does not reopen it. /// /// The string value to append. /// Thrown when is null.