From 151f978fc60b49b801292f36a8270d3bf5ca9c19 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Thu, 20 Aug 2026 11:14:20 +0000 Subject: [PATCH] chore: adopt the standard CI shape, CodeQL and Dependabot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts NextIteration.Standards section 3 and the section 4 workflow clauses. Closes 3.0, 3.1, 3.5, 3.6, 3.7, 4.4 and 4.6 for this repo. ci.yml is the canonical template, with two repo-specific parts. The `release` job that cut the GitHub release from CHANGELOG.md is kept and moved downstream of `publish`, so a release is only ever cut for bytes that reached nuget.org; STANDARD.md 3.1 names four jobs and does not forbid a fifth. The test matrix now runs all three platforms per 3.1.1 with no exception claimed — this is a filesystem library, so Windows and macOS exercise real differences rather than the same code path twice. Adding the Windows leg exposed a defect in AtomicFile that has shipped since 0.1.0. File.Move(overwrite: true) is rename(2) on POSIX, which replaces a destination another handle holds open and serialises concurrent renames; on Windows it is MoveFileEx, which raises a sharing violation in both cases. WriteAllTextAsync now routes Windows through File.Replace (ReplaceFile) with a short retry for the window between testing for the destination and replacing it. Fixed here rather than follow-up so the branch is green on every leg it now runs. Coverage was referenced but never collected: `-- --coverage` now invokes the Microsoft.Testing.Platform extension and the .coverage files are uploaded per platform. dependabot.yml's ignore list carries only the one package this repo floors per TFM. Copying Auth's three verbatim would assert two dependencies that are not in the tree. Build is clean at zero warnings and all 64 tests pass on net8.0 and net10.0 locally; the Windows and macOS legs run for the first time in this PR. Co-Authored-By: Claude Opus 5 (1M context) --- .github/dependabot.yml | 80 +++++++++ .github/workflows/ci.yml | 158 ++++++++++++++---- .github/workflows/codeql.yml | 62 +++++++ .github/workflows/dependabot-auto-merge.yml | 82 +++++++++ CHANGELOG.md | 27 +++ .../Persistence/AtomicFile.cs | 66 +++++++- 6 files changed, 436 insertions(+), 39 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/codeql.yml create mode 100644 .github/workflows/dependabot-auto-merge.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..ccfe019 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,80 @@ +version: 2 + +updates: + # --------------------------------------------------------------------------- + # NuGet packages (src + tests) + # + # Minor and patch bumps are grouped into a single PR so the auto-merge + # workflow has one unambiguous update-type to act on. Major bumps are + # deliberately left OUT of the group, so each arrives as its own PR and + # stays open for manual review. + # --------------------------------------------------------------------------- + - package-ecosystem: nuget + directory: "/" + schedule: + interval: weekly + day: monday + time: "06:00" + timezone: Etc/UTC + open-pull-requests-limit: 10 + commit-message: + prefix: "chore(deps)" + labels: + - dependencies + - nuget + # ------------------------------------------------------------------------- + # REPO-SPECIFIC. STANDARD.md 4.10 defines this list as exactly the packages + # *this* repo floors per target framework, and those differ per repo — an + # ignore entry for a package the tree does not reference asserts a dependency + # that is not there. This repo floors exactly one, in Directory.Packages.props. + # + # It carries a deliberate per-TFM floor: a net8.0 consumer must stay on its own + # 8.0.x servicing line, so an 8.x -> 10.x major PR is never mergeable here and + # would just be weekly noise. + # + # Two caveats, both inherent to Dependabot rather than to this repo: + # 1. `ignore` matches by dependency NAME and cannot be scoped to a single + # target framework. The package is referenced under BOTH the net8.0 and + # net10.0 ItemGroups, so this also suppresses a future net10 major + # (10.x -> 11.x). Bump it by hand when a new .NET major lands. + # 2. `ignore` conditions filter SECURITY updates as well as version + # updates, so a major-version security fix would also be suppressed. Low + # risk in practice (a CVE fix for 8.0.x ships as 8.0.y, a patch), but + # worth knowing. + # ------------------------------------------------------------------------- + ignore: + - dependency-name: Microsoft.Extensions.DependencyInjection.Abstractions + update-types: + - version-update:semver-major + groups: + nuget-minor-patch: + patterns: + - "*" + update-types: + - minor + - patch + + # --------------------------------------------------------------------------- + # GitHub Actions used by ci.yml (checkout, setup-dotnet, upload/download + # artifact, NuGet/login). Same grouping rule as NuGet. + # --------------------------------------------------------------------------- + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: weekly + day: monday + time: "06:00" + timezone: Etc/UTC + open-pull-requests-limit: 10 + commit-message: + prefix: "chore(actions)" + labels: + - dependencies + - github-actions + groups: + actions-minor-patch: + patterns: + - "*" + update-types: + - minor + - patch diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 42e7f31..4b55761 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,39 +1,64 @@ +# CI for NextIteration.SpectreConsole.Settings. +# Canonical shape defined in NextIteration.Standards STANDARD.md section 3 — change it +# there first, then here. +# +# The single required status check is `ci`, the aggregating gate below. `build` and `test` +# must NOT be required directly: `test` is a matrix, so its check names carry the matrix +# values and change whenever the matrix does. The gate's name is stable. +# +# The test matrix runs all three platforms (STANDARD.md 3.1.1). This library has no +# OS-native backend, but it is a filesystem library: `AtomicFile` needs a different +# replace primitive on Windows (`ReplaceFile`) than on POSIX (`rename(2)`), and path +# handling, file locking and case sensitivity all differ. No EXCEPTIONS.md entry applies +# to this repo. +# +# `release` is a fifth, tag-gated job beyond STANDARD.md 3.1's four. It cuts the GitHub +# release from CHANGELOG.md after `publish`, and cannot run on a pull request. name: CI on: push: branches: [ main ] - tags: - - 'v*' + tags: [ 'v*' ] pull_request: branches: [ main ] +# Superseded pushes are cancelled. Tag builds are never cancelled — a half-cancelled +# release can leave an incomplete package set on nuget.org. +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: ${{ !startsWith(github.ref, 'refs/tags/') }} + +permissions: + contents: read + jobs: build: runs-on: ubuntu-latest - + timeout-minutes: 15 steps: - - name: Checkout - uses: actions/checkout@v7 + - uses: actions/checkout@v7 - # The tests multi-target net8.0 and net10.0, so both runtimes must be - # present; the build itself always uses the latest SDK installed here. - - name: Setup .NET - uses: actions/setup-dotnet@v6 + # Both SDKs: shipping projects target net8.0 and net10.0 and the tests run + # against BOTH (STANDARD.md 2.3), which needs the 8.0 runtime present. + - uses: actions/setup-dotnet@v6 with: dotnet-version: | 8.0.x 10.0.x + - uses: actions/cache@v6 + with: + path: ~/.nuget/packages + key: nuget-${{ runner.os }}-${{ hashFiles('**/Directory.Packages.props', '**/*.csproj') }} + restore-keys: nuget-${{ runner.os }}- + - name: Restore run: dotnet restore - name: Build run: dotnet build --configuration Release --no-restore - - name: Test - run: dotnet test --configuration Release --no-build --verbosity normal - - name: Pack run: dotnet pack --configuration Release --no-build --output ./artifacts @@ -41,53 +66,120 @@ jobs: uses: actions/upload-artifact@v7 with: name: nuget-package - # Capture both .nupkg and .snupkg so the publish job's - # `dotnet nuget push *.nupkg` can also push the matching - # symbol package next to it. + # Both .nupkg and .snupkg, so the publish job's glob also pushes symbols. path: ./artifacts/*nupkg + test: + strategy: + fail-fast: false # one platform failing must not hide another's result + matrix: + os: [ ubuntu-latest, windows-latest, macos-latest ] + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-dotnet@v6 + with: + dotnet-version: | + 8.0.x + 10.0.x + + - uses: actions/cache@v6 + with: + path: ~/.nuget/packages + key: nuget-${{ runner.os }}-${{ hashFiles('**/Directory.Packages.props', '**/*.csproj') }} + restore-keys: nuget-${{ runner.os }}- + + # Tests run across every shipped TFM (STANDARD.md 2.3). No --no-build: this job + # does not share a filesystem with `build`, and rebuilding is cheaper and less + # fragile than shipping obj/ between jobs. + # `-- --coverage` passes through to Microsoft.Testing.Platform's coverage + # extension (STANDARD.md 2.6). Referencing a collector without invoking it is + # worse than none: it reads as coverage in the dependency list while producing + # no data. + - name: Test + run: dotnet test --configuration Release --verbosity normal -- --coverage + + - name: Upload coverage + if: always() + uses: actions/upload-artifact@v7 + with: + name: coverage-${{ matrix.os }} + path: '**/TestResults/*.coverage' + if-no-files-found: warn + + # THE required status check. Aggregates everything above so the ruleset never has to + # know the matrix shape. `if: always()` is essential — without it the gate is skipped + # when a dependency fails, and a skipped check reads as success to branch protection. + ci: + needs: [ build, test ] + if: always() + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Verify every required job succeeded + env: + RESULTS: ${{ join(needs.*.result, ',') }} + run: | + echo "upstream results: $RESULTS" + case "$RESULTS" in + *failure*|*cancelled*|*skipped*) + echo "::error title=CI gate::an upstream job did not succeed ($RESULTS)" + exit 1 ;; + esac + echo "all upstream jobs succeeded" + publish: - needs: build + needs: ci runs-on: ubuntu-latest - if: startsWith(github.ref, 'refs/tags/v') + timeout-minutes: 15 + if: startsWith(github.ref, 'refs/tags/') + permissions: - id-token: write # enable GitHub OIDC token issuance for NuGet Trusted Publishing + id-token: write # GitHub OIDC token issuance for NuGet trusted publishing + contents: read steps: - - name: Setup .NET 10 - uses: actions/setup-dotnet@v6 + - uses: actions/setup-dotnet@v6 with: dotnet-version: '10.0.x' - - name: Download artifact - uses: actions/download-artifact@v8 + - uses: actions/download-artifact@v8 with: name: nuget-package path: ./artifacts - # Exchange the GitHub OIDC token for a short-lived (1-hour) nuget.org API - # key. Requires a matching Trusted Publishing policy on nuget.org. Run this - # immediately before the push so the temporary key doesn't expire. - - name: NuGet login (OIDC -> temp API key) + # Exchanges the OIDC token for a short-lived (1h) nuget.org key. Requires a + # Trusted Publishing policy on nuget.org bound to this repo + workflow file. + # NUGET_USER is the nuget.org account name, not an email. + - name: NuGet login (OIDC to temporary API key) uses: NuGet/login@v1 id: login with: - # nuget.org username (profile name), NOT an email address. user: ${{ secrets.NUGET_USER }} - name: Publish to NuGet - run: dotnet nuget push "./artifacts/*.nupkg" --api-key "${{ steps.login.outputs.NUGET_API_KEY }}" --source https://api.nuget.org/v3/index.json --skip-duplicate - + run: > + dotnet nuget push "./artifacts/*.nupkg" + --api-key "${{ steps.login.outputs.NUGET_API_KEY }}" + --source https://api.nuget.org/v3/index.json + --skip-duplicate + + # Beyond STANDARD.md 3.1's canonical four, and downstream of `publish`, so a GitHub + # release is only ever cut for bytes that actually reached nuget.org. Tag-gated, so it + # can never run on a pull request. release: needs: publish runs-on: ubuntu-latest - if: startsWith(github.ref, 'refs/tags/v') + timeout-minutes: 10 + if: startsWith(github.ref, 'refs/tags/') + permissions: - contents: write + contents: write # creating the GitHub release steps: - - name: Checkout - uses: actions/checkout@v7 + - uses: actions/checkout@v7 # Pull the section for this tag's version out of CHANGELOG.md # (e.g. tag v0.1.0 -> the "## [0.1.0] — …" block) for the release body. diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..f7cad91 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,62 @@ +# CodeQL code scanning. See STANDARD.md section 4.4. +name: CodeQL + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + schedule: + # Weekly, so a newly published query pack finds existing code even when + # nothing has been pushed. Offset off the hour to avoid the scheduling spike. + - cron: '37 4 * * 1' + +concurrency: + group: codeql-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + analyze: + name: analyze + runs-on: ubuntu-latest + timeout-minutes: 30 + + permissions: + security-events: write # required to upload results + contents: read + + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Setup .NET + uses: actions/setup-dotnet@v6 + with: + dotnet-version: | + 8.0.x + 10.0.x + + - name: Initialize CodeQL + uses: github/codeql-action/init@v4 + with: + languages: csharp + # security-and-quality is broader than the default security-extended; + # these are small libraries, so the extra findings are affordable. + queries: security-and-quality + + # Explicit build rather than autobuild: these repos multi-target, and + # autobuild has picked a single TFM in the past, silently analysing half + # the code. Restore is separate so a restore failure is legible. + - name: Restore + run: dotnet restore + + - name: Build + run: dotnet build --configuration Release --no-restore + + - name: Perform CodeQL analysis + uses: github/codeql-action/analyze@v4 + with: + category: "/language:csharp" diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml new file mode 100644 index 0000000..ce7ab4e --- /dev/null +++ b/.github/workflows/dependabot-auto-merge.yml @@ -0,0 +1,82 @@ +name: Dependabot auto-merge + +# Auto-merges Dependabot minor and patch bumps once CI passes. Major bumps are +# left untouched so they stay open for manual review. +# +# `on: pull_request` (not pull_request_target) is deliberate: pull_request_target +# would run with a write token in the base-repo context, which is the classic +# privilege-escalation footgun. On Dependabot pull_request events the GITHUB_TOKEN +# is read-only by default, and the `permissions:` block below grants the write +# scopes back. This is GitHub's documented recipe. +# +# --------------------------------------------------------------------------- +# REQUIRED SETUP: the `AUTO_MERGE_PAT` secret must be stored as a +# **Dependabot secret**, NOT an Actions secret: +# +# Settings -> Secrets and variables -> Dependabot -> New repository secret +# gh secret set AUTO_MERGE_PAT --app dependabot +# +# Workflows triggered by Dependabot events only receive Dependabot secrets; +# Actions secrets resolve to an empty string. The guard step below fails loudly +# if that happens rather than letting the approval silently no-op. +# +# The PAT must belong to a CODEOWNER (the main ruleset sets +# require_code_owner_review: true, and a GITHUB_TOKEN/bot approval cannot +# satisfy a code-owner review). Scope: fine-grained with +# "Pull requests: read and write" on this repo, or classic `repo`. +# --------------------------------------------------------------------------- +on: pull_request + +permissions: + contents: read + pull-requests: read + +jobs: + auto-merge: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login == 'dependabot[bot]' + + steps: + - name: Verify AUTO_MERGE_PAT is present + env: + AUTO_MERGE_PAT: ${{ secrets.AUTO_MERGE_PAT }} + run: | + if [ -z "$AUTO_MERGE_PAT" ]; then + echo "::error title=Missing AUTO_MERGE_PAT::Store it as a *Dependabot* secret (gh secret set AUTO_MERGE_PAT --app dependabot). Actions secrets are not available to Dependabot-triggered workflows." + exit 1 + fi + + - name: Fetch Dependabot metadata + id: meta + uses: dependabot/fetch-metadata@v3 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + + # `--auto` does NOT merge immediately: it queues the merge behind the branch + # ruleset, so the required check must go green first. That check is `ci` and only + # `ci` (STANDARD.md 3.1) — an aggregating gate over `build` and `test`, so the + # matrix can be reshaped without touching the ruleset or this comment. + # If CI fails, the PR just stays open. + # + # The approval uses the PAT so it counts as a code-owner review. Because the + # ruleset also sets dismiss_stale_reviews_on_push and require_last_push_approval, + # a follow-up Dependabot force-push re-triggers this workflow (pull_request + # includes `synchronize`) and the PR is re-approved. + - name: Approve and enable auto-merge (minor + patch) + if: | + steps.meta.outputs.update-type == 'version-update:semver-minor' || + steps.meta.outputs.update-type == 'version-update:semver-patch' + env: + PR_URL: ${{ github.event.pull_request.html_url }} + GH_TOKEN: ${{ secrets.AUTO_MERGE_PAT }} + run: | + gh pr review --approve "$PR_URL" + gh pr merge --auto --squash "$PR_URL" + + # No approval, no auto-merge — the PR stays open for a human. + - name: Leave major bumps open + if: steps.meta.outputs.update-type == 'version-update:semver-major' + env: + DEPS: ${{ steps.meta.outputs.dependency-names }} + run: | + echo "::notice title=Major version bump::${DEPS} is a major bump; leaving this PR open for manual review." diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d818da..81bf2bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,9 +59,36 @@ floor. `setup-dotnet` v6, `upload-artifact` v7, `download-artifact` v8) and dropped the now-redundant `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24` override — those majors already run on Node 24. +- Adopted the canonical CI shape from + [NextIteration.Standards](https://github.com/StuartMeeks/NextIteration.Standards) + (`STANDARD.md` section 3). `build` and `test` are now separate jobs, `test` + runs a three-platform matrix (Linux, Windows, macOS) rather than Linux alone, + and an aggregating `ci` gate is the single required status check — so the + matrix can be reshaped without touching branch protection. Coverage is now + actually collected in CI (`dotnet test -- --coverage`); the collector was + referenced but never invoked. Workflows gained `concurrency`, + `timeout-minutes`, a least-privilege `permissions` block and a NuGet cache. + +### Added + +- CodeQL code scanning (`security-and-quality` query pack), weekly plus on every + push and pull request. +- Dependabot for NuGet and GitHub Actions, with minor and patch updates grouped + and auto-merged behind CI, and majors left open for review. Major updates to + `Microsoft.Extensions.DependencyInjection.Abstractions` are suppressed, because + its floor is deliberately per-target-framework and an 8.x -> 10.x bump is never + mergeable here. ### Fixed +- `AtomicFile` raised a sharing violation on Windows when two writers replaced + the same settings file concurrently, or when the destination was held open by + another handle. `File.Move(overwrite: true)` is `rename(2)` on POSIX, which + tolerates both, but `MoveFileEx` on Windows, which does not. Windows now uses + `File.Replace` (`ReplaceFile`) with a short retry. Present since 0.1.0 and + never caught, because the concurrent-writer test had only ever run on Linux; + identified while adding the Windows leg to the matrix and fixed in the same + change, so the branch stays green. - README claimed the package targets `net10.0` only; it has shipped `net8.0` and `net10.0` assemblies since 0.2.0. diff --git a/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs b/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs index c992349..14c9118 100644 --- a/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs +++ b/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs @@ -2,11 +2,20 @@ namespace NextIteration.SpectreConsole.Settings.Persistence { /// /// Crash-safe text file writer. Writes to a uniquely-named temp file in the - /// same directory as the final path, then performs an atomic rename. - /// is atomic on NTFS and - /// backed by rename(2) on POSIX, so a reader observes either the old - /// content or the new content — never a partial write, even if the process - /// is killed mid-call. + /// same directory as the final path, then atomically replaces the final path, + /// so a reader observes either the old content or the new content — never a + /// partial write, even if the process is killed mid-call. + /// + /// The replace primitive differs by platform. On POSIX, + /// is rename(2), which + /// replaces the destination even while another handle holds it open, and + /// serialises concurrent renames. On Windows the same call is + /// MoveFileEx with MOVEFILE_REPLACE_EXISTING, which does + /// not tolerate that: it raises a sharing violation when the + /// destination is open or when two replacements race. Windows therefore uses + /// (ReplaceFile), + /// which is built for exactly that case, with a short retry for the window + /// between testing for the destination and replacing it. /// /// /// This does not serialise concurrent writers. Two writers each producing a @@ -24,7 +33,7 @@ internal static async Task WriteAllTextAsync( try { await File.WriteAllTextAsync(tempPath, contents, cancellationToken).ConfigureAwait(false); - File.Move(tempPath, path, overwrite: true); + await ReplaceAtomicallyAsync(tempPath, path).ConfigureAwait(false); } catch { @@ -33,6 +42,51 @@ internal static async Task WriteAllTextAsync( } } + /// + /// Moves onto , + /// replacing it if present. See the type remarks for why Windows cannot + /// use the POSIX path. + /// + private static async Task ReplaceAtomicallyAsync(string tempPath, string path) + { + if (!OperatingSystem.IsWindows()) + { + File.Move(tempPath, path, overwrite: true); + return; + } + + const int maxAttempts = 5; + for (var attempt = 1; ; attempt++) + { + try + { + if (File.Exists(path)) + { + // ReplaceFile semantics: tolerates an open destination and + // deletes the source on success. No backup file wanted. + File.Replace(tempPath, path, destinationBackupFileName: null); + } + else + { + // Destination absent, so a plain move is the whole job. + // Deliberately not overwrite:true — if a racing writer + // created it in the meantime we want the throw, and the + // retry below routes us to File.Replace instead. + File.Move(tempPath, path); + } + + return; + } + catch (Exception ex) when (ex is UnauthorizedAccessException or IOException && attempt < maxAttempts) + { + // A concurrent writer is mid-replace, or created the + // destination between our File.Exists test and the call. + // Both are transient; back off and re-evaluate. + await Task.Delay(10 * attempt).ConfigureAwait(false); + } + } + } + private static string BuildTempPath(string finalPath) => // Unique per call so concurrent writers don't collide on a shared // "{path}.tmp" name.