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.