diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 1e51612..b550b17 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,26 +1,14 @@ -# Weekly dependency updates for NuGet packages and the GitHub Actions pinned in -# .github/workflows. Minor/patch bumps are grouped into a single PR to keep the -# noise down; major bumps still arrive as individual PRs for closer review. +# Weekly dependency updates for the GitHub Actions pinned in .github/workflows. +# +# NuGet packages are deliberately NOT handled here. Dependabot's NuGet updater +# cannot evaluate the WinUI project's Windows-versioned target framework +# (net10.0-windows10.0.19041.0; dependabot/dependabot-core#13923) and declares +# every newer package "not compatible", which also blocks the same packages in the +# other projects. The NuGet updates workflow (.github/workflows/nuget-updates.yml) +# does that job through `dotnet list package --outdated` instead. Dependabot +# security alerts for NuGet are unaffected: they come from the dependency graph. version: 2 updates: - - package-ecosystem: nuget - directory: "/" - # Updates land on dev like all other work and reach main via the gated PR flow. - target-branch: dev - schedule: - interval: weekly - day: monday - time: "06:00" - timezone: America/New_York - groups: - nuget-minor-patch: - applies-to: version-updates - patterns: - - "*" - update-types: - - minor - - patch - - package-ecosystem: github-actions directory: "/" target-branch: dev diff --git a/.github/scripts/Update-NuGetPackages.ps1 b/.github/scripts/Update-NuGetPackages.ps1 new file mode 100644 index 0000000..1f80391 --- /dev/null +++ b/.github/scripts/Update-NuGetPackages.ps1 @@ -0,0 +1,165 @@ +<# +.SYNOPSIS + Bumps every top-level PackageReference in the solution to its latest stable + version and writes a Markdown report of what changed and what was held back. + +.DESCRIPTION + Replaces Dependabot's NuGet ecosystem for this repository. Dependabot's NuGet + updater mis-evaluates Windows-versioned target frameworks such as + net10.0-windows10.0.19041.0 (dependabot/dependabot-core#13923): it declares every + newer package "not compatible" with the WinUI project and, because it updates a + package consistently across all projects, that blocks the test projects too. + `dotnet list package --outdated` uses the real NuGet/MSBuild evaluation and has no + such problem, so this script drives the weekly update PR from it instead. + + Policy, mirroring the Dependabot config it replaces: + - minor and patch bumps are applied; + - major bumps are reported but NOT applied unless -IncludeMajor is given, so + they get a deliberate review; + - prerelease versions are never applied to a package that is on a stable one. + + Versions are edited in place in each .csproj (the exact Include/Version pair), + preserving the file's encoding and line endings. + +.PARAMETER Solution + Solution to evaluate. Default: gclo.slnx. + +.PARAMETER ReportPath + Where to write the Markdown report. Default: nuget-update-report.md. + +.PARAMETER IncludeMajor + Also apply major-version bumps. + +.OUTPUTS + Writes `changed=true|false` and `report=` to $env:GITHUB_OUTPUT when set. + Exit code 0 on success (whether or not anything changed), non-zero on error. +#> +[CmdletBinding()] +param( + [string]$Solution = 'gclo.slnx', + [string]$ReportPath = 'nuget-update-report.md', + [switch]$IncludeMajor +) + +$ErrorActionPreference = 'Stop' + +# [System.IO.File] resolves relative paths against the process working directory, +# which is not necessarily PowerShell's current location; anchor it explicitly. +$ReportPath = [System.IO.Path]::GetFullPath($ReportPath, (Get-Location).Path) + +function Get-NumericVersion([string]$version) { + # NuGet versions may carry a prerelease suffix (1.2.3-beta.1) and may have four + # numeric parts (10.0.28000.2705); [version] handles the latter but not the former. + $core = ($version -split '[-+]', 2)[0] + return [version]$core +} + +function Test-Prerelease([string]$version) { + return $version.Contains('-') +} + +Write-Host "Restoring $Solution" +dotnet restore $Solution | Out-Null +if ($LASTEXITCODE -ne 0) { throw "dotnet restore failed with exit code $LASTEXITCODE." } + +Write-Host "Querying outdated packages" +$raw = dotnet list $Solution package --outdated --format json +if ($LASTEXITCODE -ne 0) { throw "dotnet list package --outdated failed with exit code $LASTEXITCODE." } +$listing = ($raw -join "`n") | ConvertFrom-Json + +# One row per (project, package): the same package can be listed under several +# target frameworks of one project, and we edit the csproj once. +$rows = @{} +# Projects with nothing outdated have no 'frameworks' member at all, and a framework +# with nothing outdated has no 'topLevelPackages'; @($null) still yields one $null +# iteration, hence the Where-Object filters. +foreach ($project in @($listing.projects | Where-Object { $_ })) { + foreach ($framework in @($project.frameworks | Where-Object { $_ })) { + foreach ($package in @($framework.topLevelPackages | Where-Object { $_ })) { + $key = "$($project.path)|$($package.id)" + if (-not $rows.ContainsKey($key)) { + $rows[$key] = [pscustomobject]@{ + Project = $project.path + Id = $package.id + Requested = $package.requestedVersion + Latest = $package.latestVersion + } + } + } + } +} + +$applied = New-Object System.Collections.Generic.List[object] +$held = New-Object System.Collections.Generic.List[object] + +foreach ($row in ($rows.Values | Sort-Object Project, Id)) { + $current = Get-NumericVersion $row.Requested + $latest = Get-NumericVersion $row.Latest + $projectName = [System.IO.Path]::GetFileName($row.Project) + + $reason = $null + if ((Test-Prerelease $row.Latest) -and -not (Test-Prerelease $row.Requested)) { + $reason = 'latest is a prerelease' + } + elseif ($latest.Major -ne $current.Major -and -not $IncludeMajor) { + $reason = 'major version bump: review manually (or run with -IncludeMajor)' + } + elseif ($latest -le $current) { + $reason = 'not newer than the requested version' + } + + if ($reason) { + $held.Add([pscustomobject]@{ Project = $projectName; Id = $row.Id; From = $row.Requested; To = $row.Latest; Reason = $reason }) + continue + } + + # Exact textual edit of the one PackageReference, so nothing else in the file moves. + $bytes = [System.IO.File]::ReadAllBytes($row.Project) + $hasBom = $bytes.Length -ge 3 -and $bytes[0] -eq 0xEF -and $bytes[1] -eq 0xBB -and $bytes[2] -eq 0xBF + $text = [System.Text.Encoding]::UTF8.GetString($bytes) + if ($hasBom) { $text = $text.Substring(1) } + + $pattern = '( $($row.Latest)" +} + +# ---------------------------------------------------------------- report +$lines = New-Object System.Collections.Generic.List[string] +$lines.Add('## NuGet package updates') +$lines.Add('') +if ($applied.Count -eq 0) { + $lines.Add('No minor or patch updates were available.') +} +else { + $lines.Add('| Project | Package | From | To |') + $lines.Add('| --- | --- | --- | --- |') + foreach ($a in $applied) { $lines.Add("| $($a.Project) | $($a.Id) | $($a.From) | $($a.To) |") } +} +if ($held.Count -gt 0) { + $lines.Add('') + $lines.Add('### Held back (not applied)') + $lines.Add('') + $lines.Add('| Project | Package | Current | Latest | Why |') + $lines.Add('| --- | --- | --- | --- | --- |') + foreach ($h in $held) { $lines.Add("| $($h.Project) | $($h.Id) | $($h.From) | $($h.To) | $($h.Reason) |") } +} +$lines.Add('') +$lines.Add("_Generated by ``.github/scripts/Update-NuGetPackages.ps1`` from ``dotnet list package --outdated`` on $(Get-Date -Format 'yyyy-MM-dd')._") +[System.IO.File]::WriteAllText($ReportPath, ($lines -join "`n") + "`n", (New-Object System.Text.UTF8Encoding($false))) + +Write-Host '' +Write-Host (Get-Content $ReportPath -Raw) + +$changed = $applied.Count -gt 0 +if ($env:GITHUB_OUTPUT) { + Add-Content -Path $env:GITHUB_OUTPUT -Value "changed=$($changed.ToString().ToLowerInvariant())" + Add-Content -Path $env:GITHUB_OUTPUT -Value "report=$ReportPath" +} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f512340..ab221d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,6 +9,19 @@ on: push: branches: [main, dev] pull_request: + # The Release workflow calls this one so a tag is gated by exactly the checks + # a pull request is: build, coverage, and formatting, with the UI smoke suite + # advisory (see ui-tests-advisory) just as it is not a required check on main. + workflow_call: + inputs: + ui-tests-advisory: + description: >- + When true, a failing UI end-to-end job does not fail this workflow. The + hosted-runner UIA suite is not yet a required check on main, so a release + must not be held to a stricter bar than a merge; a failure is still + visible in the job list for follow-up. + type: boolean + default: false permissions: contents: read @@ -27,7 +40,7 @@ jobs: uses: actions/checkout@v7 - name: Set up .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x @@ -92,16 +105,20 @@ jobs: ui-tests: # End-to-end FlaUI/UIA tests against the real gclo.exe. Not in the branch # ruleset's required checks yet: hosted-runner UIA is new here, so this job - # earns required status by proving itself stable first. + # earns required status by proving itself stable first. Until then a caller + # (the Release workflow) can make it advisory so a flaky UI run cannot block + # an immutable tag that main itself would have accepted. On push/PR events the + # input is unset, so the job fails the workflow exactly as before. name: UI end-to-end tests (x64) runs-on: windows-latest timeout-minutes: 30 + continue-on-error: ${{ inputs.ui-tests-advisory == true }} steps: - name: Checkout uses: actions/checkout@v7 - name: Set up .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x @@ -122,7 +139,7 @@ jobs: uses: actions/checkout@v7 - name: Set up .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 8e8b90f..277a67d 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -6,11 +6,13 @@ # security analysis, and it should still scan code that has a pending warning). name: CodeQL +# Runs on dev as well as main: feature PRs target dev (CONTRIBUTING.md), so +# scanning only main would let code reach the integration branch unanalyzed. on: push: - branches: [main] + branches: [main, dev] pull_request: - branches: [main] + branches: [main, dev] schedule: # Weekly scan (Mondays 06:30 UTC) so new CodeQL queries run even when the # code has not changed. @@ -36,7 +38,7 @@ jobs: uses: actions/checkout@v7 - name: Set up .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: dotnet-version: 10.0.x diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 741bc5e..d488288 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -48,3 +48,16 @@ jobs: GPL-2.0-only WITH Classpath-exception-2.0, GPL-2.0-or-later WITH Classpath-exception-2.0, GPL-2.0-only WITH GCC-exception-2.0 + # Packages whose nuspec declares the license as a FILE rather than an + # SPDX expression, which the dependency graph reports as "unknown" / + # NOASSERTION and would otherwise fail every version bump. All three have + # shipped in gclo since the first commit, so this is not a new license + # decision - it records what they actually are: + # LibGit2Sharp MIT (App_Readme/LICENSE.md) + # Microsoft.WindowsAppSDK Microsoft Software License Terms (license.txt) + # Microsoft.Windows.SDK.BuildTools Windows SDK license (aka.ms/WinSDKLicenseURL) + # Vulnerability scanning still applies to them; only the license check is skipped. + allow-dependencies-licenses: >- + pkg:nuget/LibGit2Sharp, + pkg:nuget/Microsoft.WindowsAppSDK, + pkg:nuget/Microsoft.Windows.SDK.BuildTools diff --git a/.github/workflows/nuget-updates.yml b/.github/workflows/nuget-updates.yml new file mode 100644 index 0000000..65a2e77 --- /dev/null +++ b/.github/workflows/nuget-updates.yml @@ -0,0 +1,142 @@ +# Weekly NuGet dependency updates, as a pull request against dev. +# +# This replaces Dependabot's NuGet ecosystem for this repository. Dependabot's NuGet +# updater cannot evaluate Windows-versioned target frameworks (the WinUI project's +# net10.0-windows10.0.19041.0; dependabot/dependabot-core#13923): it reports every +# newer package as "not compatible", and because it updates a package consistently +# across projects, that silently blocked updates for the test projects as well. The +# only updates it ever delivered were the ones it later closed as "updatable in +# another way". `dotnet list package --outdated` evaluates through real MSBuild and +# NuGet, so the script below drives the bump from that instead. +# +# Flow: bump minor/patch versions in the .csproj files -> run the same gates CI +# runs (zero-warning x64 build, both unit suites, format) -> open or update a single +# PR on branch deps/nuget-weekly with the report and the gate results in its body. +# Major bumps are listed in the report but not applied; run the workflow manually +# with include-major to take them. +# +# Token note: with the default GITHUB_TOKEN, GitHub does not start other workflows +# for the PR this job creates, so the PR shows no CI checks of its own (the gates +# were run here, and dev -> main still goes through the required checks). To get +# checks on the PR itself, add a fine-grained PAT with contents + pull-requests +# write as the DEPS_PAT repository secret; the step below prefers it when present. +name: NuGet updates + +on: + schedule: + # Mondays 06:00 America/New_York (11:00 UTC in winter, 10:00 UTC in summer). + - cron: '0 11 * * 1' + workflow_dispatch: + inputs: + include-major: + description: Also apply major-version bumps + type: boolean + default: false + +permissions: + contents: read + +concurrency: + group: nuget-updates + cancel-in-progress: false + +jobs: + update: + name: Bump packages and open PR + runs-on: windows-latest + timeout-minutes: 45 + permissions: + contents: write + pull-requests: write + steps: + - name: Checkout dev + uses: actions/checkout@v7 + with: + ref: dev + + - name: Set up .NET SDK + uses: actions/setup-dotnet@v6 + with: + dotnet-version: 10.0.x + + - name: Bump package versions + id: bump + shell: pwsh + run: | + $params = @{ Solution = 'gclo.slnx'; ReportPath = 'nuget-update-report.md' } + if ('${{ inputs.include-major }}' -eq 'true') { $params.IncludeMajor = $true } + ./.github/scripts/Update-NuGetPackages.ps1 @params + + # The gates CI applies, run against the bumped tree. They are allowed to fail + # so the PR still opens and shows what broke; the outcome lands in the body. + - name: Gate - build (x64, warnings as errors) + if: steps.bump.outputs.changed == 'true' + id: build + continue-on-error: true + run: dotnet build gclo.slnx -p:Platform=x64 -warnaserror + + - name: Gate - engine and view-model tests + if: steps.bump.outputs.changed == 'true' && steps.build.outcome == 'success' + id: engine-tests + continue-on-error: true + run: dotnet test gclo.Engine.Tests + + - name: Gate - CLI tests + if: steps.bump.outputs.changed == 'true' && steps.build.outcome == 'success' + id: cli-tests + continue-on-error: true + run: dotnet test gclo.Cli.Tests + + - name: Gate - format + if: steps.bump.outputs.changed == 'true' + id: format + continue-on-error: true + run: dotnet format gclo.slnx --verify-no-changes --severity error + + - name: Compose PR body + if: steps.bump.outputs.changed == 'true' + shell: pwsh + run: | + function Mark($outcome) { + switch ($outcome) { + 'success' { ':white_check_mark: passed' } + 'failure' { ':x: FAILED - see the workflow run' } + default { ':heavy_minus_sign: skipped' } + } + } + $gates = @( + '### Gates (run in this workflow against the bumped tree)', + '', + "- Build (x64, warnings as errors): $(Mark '${{ steps.build.outcome }}')", + "- Engine + ViewModels tests: $(Mark '${{ steps.engine-tests.outcome }}')", + "- CLI tests: $(Mark '${{ steps.cli-tests.outcome }}')", + "- Format: $(Mark '${{ steps.format.outcome }}')", + '', + 'The UI smoke suite and the 100% coverage gate run when dev is merged to main.', + "Workflow run: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}", + '' + ) + $report = Get-Content nuget-update-report.md -Raw + Set-Content -Path pr-body.md -Value (($gates -join "`n") + "`n" + $report) -NoNewline + Remove-Item nuget-update-report.md + + - name: Open or update the pull request + if: steps.bump.outputs.changed == 'true' + uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8.1.1 + with: + token: ${{ secrets.DEPS_PAT || github.token }} + base: dev + branch: deps/nuget-weekly + delete-branch: true + commit-message: 'Deps: weekly NuGet package updates' + title: 'Deps: weekly NuGet package updates' + body-path: pr-body.md + labels: | + dependencies + .NET + add-paths: | + **/*.csproj + + - name: Nothing to update + if: steps.bump.outputs.changed != 'true' + run: echo "All packages are current. No PR opened." diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8d0e1ce..421786f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -12,30 +12,30 @@ on: tags: - 'v*' -# contents: write is required so vpk / gh can create the GitHub Release and -# upload assets to it. The default GITHUB_TOKEN is sufficient for both. +# Three stages, each gated on the previous: +# verify - the tag must be strict semver AND its commit must be on main +# gates - the full CI workflow (build, 100% coverage, format; UI smoke advisory) +# release - package and publish; only runs when both stages succeeded +# +# Read-only by default; only the release job gets contents: write, which vpk +# and gh need to create the GitHub Release and upload assets to it. permissions: - contents: write + contents: read jobs: - release: - name: Build, package, and publish - runs-on: windows-latest - timeout-minutes: 90 - env: - # Optional secrets. Empty when not configured; the steps that need them - # check for that in their `if:` and are skipped cleanly. - NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }} - WINGET_TOKEN: ${{ secrets.WINGET_TOKEN }} - REPO_URL: https://github.com/KofTwentyTwo/gclo + verify: + name: Verify tag + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + version: ${{ steps.version.outputs.version }} + prerelease: ${{ steps.version.outputs.prerelease }} + channel: ${{ steps.version.outputs.channel }} steps: - - name: Checkout + - name: Checkout (full history, so main's ancestry can be checked) uses: actions/checkout@v7 - - - name: Set up .NET SDK - uses: actions/setup-dotnet@v5 with: - dotnet-version: 10.0.x + fetch-depth: 0 - name: Derive version from tag id: version @@ -58,18 +58,64 @@ jobs: Add-Content -Path $env:GITHUB_OUTPUT -Value "prerelease=$($prerelease.ToString().ToLower())" Add-Content -Path $env:GITHUB_OUTPUT -Value "channel=$channel" - # Same quality gates as CI: zero-warning build and green tests. - - name: Build solution (x64, warnings as errors) - run: dotnet build gclo.slnx -p:Platform=x64 -warnaserror + # Releases are cut from main (docs/RELEASING.md). A tag anywhere else - + # a feature branch, an unmerged dev commit, a detached experiment - would + # otherwise publish an immutable release that never passed main's gates. + # To exercise the failure path, push a 'v*' tag on a commit that is not + # on main: this step fails and nothing downstream runs. + - name: Require the tagged commit to be on main + shell: bash + run: | + set -euo pipefail + tagged="$(git rev-parse "$GITHUB_SHA")" + if git merge-base --is-ancestor "$tagged" origin/main; then + echo "OK: $GITHUB_REF_NAME ($tagged) is contained in main." + else + echo "::error::Tag $GITHUB_REF_NAME points at $tagged, which is not contained in origin/main. Releases must be tagged from main (docs/RELEASING.md)." + exit 1 + fi - - name: Run tests - run: dotnet test gclo.Engine.Tests + # The same gates a pull request must pass: zero-warning x64 build, 100% line + # coverage on Engine/ViewModels/Cli, and formatting. If any of those fails, this + # job fails and release is skipped. The FlaUI UI smoke suite runs too but is + # advisory here, exactly as it is not a required check on main: hosted-runner UI + # automation is not yet proven stable enough to hold an immutable tag hostage. + # Documented exception per #44; flip ui-tests-advisory to false (and add the job + # to main's required checks) once the suite has earned it. + gates: + name: Gates + needs: verify + uses: ./.github/workflows/ci.yml + with: + ui-tests-advisory: true + + release: + name: Build, package, and publish + needs: [verify, gates] + runs-on: windows-latest + timeout-minutes: 90 + permissions: + contents: write + env: + # Optional secrets. Empty when not configured; the steps that need them + # check for that in their `if:` and are skipped cleanly. + NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }} + WINGET_TOKEN: ${{ secrets.WINGET_TOKEN }} + REPO_URL: https://github.com/KofTwentyTwo/gclo + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Set up .NET SDK + uses: actions/setup-dotnet@v6 + with: + dotnet-version: 10.0.x # ---------------------------------------------------------------------- # (a) CLI: self-contained single-file publish, zipped as a release asset. # ---------------------------------------------------------------------- - name: Publish CLI (win-x64, single file) - run: dotnet publish gclo.Cli -c Release -r win-x64 --self-contained -p:Version=${{ steps.version.outputs.version }} -p:PublishSingleFile=true -o publish/cli + run: dotnet publish gclo.Cli -c Release -r win-x64 --self-contained -p:Version=${{ needs.verify.outputs.version }} -p:PublishSingleFile=true -o publish/cli - name: Zip CLI shell: pwsh @@ -83,7 +129,7 @@ jobs: # and attaches the Setup, portable, and delta assets to it. # ---------------------------------------------------------------------- - name: Publish app for Velopack (unpackaged win-x64) - run: dotnet publish gclo/gclo.csproj -c Release -p:Platform=x64 -r win-x64 --self-contained -p:Version=${{ steps.version.outputs.version }} -p:WindowsPackageType=None -p:WindowsAppSDKSelfContained=true -p:PublishTrimmed=false -o publish/app + run: dotnet publish gclo/gclo.csproj -c Release -p:Platform=x64 -r win-x64 --self-contained -p:Version=${{ needs.verify.outputs.version }} -p:WindowsPackageType=None -p:WindowsAppSDKSelfContained=true -p:PublishTrimmed=false -o publish/app - name: Install Velopack CLI run: dotnet tool install -g vpk @@ -98,11 +144,11 @@ jobs: GITHUB_TOKEN: ${{ github.token }} run: | $preFlag = @() - if ('${{ steps.version.outputs.prerelease }}' -eq 'true') { $preFlag = @('--pre') } - vpk download github --repoUrl $env:REPO_URL --token $env:GITHUB_TOKEN --channel ${{ steps.version.outputs.channel }} @preFlag + if ('${{ needs.verify.outputs.prerelease }}' -eq 'true') { $preFlag = @('--pre') } + vpk download github --repoUrl $env:REPO_URL --token $env:GITHUB_TOKEN --channel ${{ needs.verify.outputs.channel }} @preFlag - name: Velopack pack - run: vpk pack --packId gclo --packVersion ${{ steps.version.outputs.version }} --packDir publish/app --mainExe gclo.exe --channel ${{ steps.version.outputs.channel }} --icon gclo/Assets/Brand/gclo.ico + run: vpk pack --packId gclo --packVersion ${{ needs.verify.outputs.version }} --packDir publish/app --mainExe gclo.exe --channel ${{ needs.verify.outputs.channel }} --icon gclo/Assets/Brand/gclo.ico # This repo has immutable releases: assets freeze the moment a release is # PUBLISHED. So vpk creates a DRAFT (no --publish), every asset and the @@ -113,8 +159,8 @@ jobs: GITHUB_TOKEN: ${{ github.token }} run: | $preFlag = @() - if ('${{ steps.version.outputs.prerelease }}' -eq 'true') { $preFlag = @('--pre') } - vpk upload github --repoUrl $env:REPO_URL --token $env:GITHUB_TOKEN --channel ${{ steps.version.outputs.channel }} --releaseName "gclo ${{ steps.version.outputs.version }}" --tag $env:GITHUB_REF_NAME @preFlag + if ('${{ needs.verify.outputs.prerelease }}' -eq 'true') { $preFlag = @('--pre') } + vpk upload github --repoUrl $env:REPO_URL --token $env:GITHUB_TOKEN --channel ${{ needs.verify.outputs.channel }} --releaseName "gclo ${{ needs.verify.outputs.version }}" --tag $env:GITHUB_REF_NAME @preFlag - name: Attach CLI zip to the draft release env: @@ -139,7 +185,7 @@ jobs: # NUGET_API_KEY secret is configured on the repository. # ---------------------------------------------------------------------- - name: Pack gclo.Engine - run: dotnet pack gclo.Engine -c Release -p:Version=${{ steps.version.outputs.version }} -o artifacts/nuget + run: dotnet pack gclo.Engine -c Release -p:Version=${{ needs.verify.outputs.version }} -o artifacts/nuget - name: Push gclo.Engine to nuget.org if: env.NUGET_API_KEY != '' @@ -155,10 +201,10 @@ jobs: # docs/RELEASING.md), then this step keeps it current. # ---------------------------------------------------------------------- - name: Submit winget manifest update - if: steps.version.outputs.prerelease == 'false' && env.WINGET_TOKEN != '' + if: needs.verify.outputs.prerelease == 'false' && env.WINGET_TOKEN != '' shell: pwsh run: | Invoke-WebRequest https://aka.ms/wingetcreate/latest -OutFile wingetcreate.exe # Velopack names the installer '--Setup.exe'. $setupUrl = "$env:REPO_URL/releases/download/$env:GITHUB_REF_NAME/gclo-stable-Setup.exe" - .\wingetcreate.exe update KofTwentyTwo.gclo --urls $setupUrl --version ${{ steps.version.outputs.version }} --submit --token $env:WINGET_TOKEN + .\wingetcreate.exe update KofTwentyTwo.gclo --urls $setupUrl --version ${{ needs.verify.outputs.version }} --submit --token $env:WINGET_TOKEN diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e02f83..fbd0681 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,7 +76,7 @@ Every pull request must pass: | **UI end-to-end tests (x64)** | FlaUI/UIA smoke tests drive the real `gclo.exe` (`gclo.UiTests`) | | **Format (style gate)** | `dotnet format gclo.slnx --verify-no-changes --severity error` | | **Dependency review** | New/changed dependencies must have no known vulnerabilities (any severity fails) and a license on the repo's allowlist | -| **CodeQL** | Static security analysis (runs on PRs targeting `main` and weekly) | +| **CodeQL** | Static security analysis (runs on pushes and PRs to `dev` and `main`, plus a weekly scheduled scan) | Running the build, test, and format commands above locally before pushing will catch almost everything CI would. diff --git a/README.md b/README.md index 41f9325..9f20067 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ On launch, gclo shows a brief branded splash, then the **Quick Sync** connect ca 8. **Cancel** at any time: in-flight repos stop cleanly, unstarted repos are marked Canceled, and the summary still appears. After a run, **Retry failed** re-runs exactly the failed rows, and **Open folder** opens the target folder in Explorer. -**When something goes wrong:** failures are isolated per repository — one broken repo never stops the rest, and a failed row shows its error message inline. Most failed clones are cleaned up, so a partial checkout is never mistaken for a valid repo on the next run. Repositories with Windows-impossible paths are the deliberate exception: gclo checks every path in the incoming tree against Windows file-system rules *before* checkout — invalid characters, reserved device names like `CON`, trailing spaces or dots, names that differ only by case — and when it finds any, the repo is marked Failed with a per-path list of reasons, but the fully fetched `.git` is **kept** (nothing was ever checked out). Recovery happens in place, with no re-download: click the row's **Resolve…** link to rename each offending path (safe names are pre-suggested) or skip it, and gclo materializes the working tree right there; the CLI does the same automatically with [`--sanitize-paths`](docs/CLI.md#windows-invalid-paths-and---sanitize-paths). Long paths beyond the classic 260-character limit are handled automatically (`core.longpaths`). +**When something goes wrong:** failures are isolated per repository — one broken repo never stops the rest, and a failed row shows its error message inline. Failed clones are cleaned up, and a clone whose checkout never finished (a crash, a kill, a cleanup that could not delete the folder) is flagged inside its `.git`, so a partial working tree is never mistaken for an up-to-date repo on the next run — gclo completes the checkout instead. Repositories with Windows-impossible paths are the deliberate exception: gclo checks every path in the incoming tree against Windows file-system rules *before* checkout — invalid characters, reserved device names like `CON`, trailing spaces or dots, names that differ only by case — and when it finds any, the repo is marked Failed with a per-path list of reasons, but the fully fetched `.git` is **kept** (nothing was ever checked out). Recovery happens in place, with no re-download: click the row's **Resolve…** link to rename each offending path (safe names are pre-suggested) or skip it, and gclo materializes the working tree right there; the CLI does the same automatically with [`--sanitize-paths`](docs/CLI.md#windows-invalid-paths-and---sanitize-paths). Long paths beyond the classic 260-character limit are handled automatically (`core.longpaths`). **Settings** (File > Settings…) let you set a default GitHub token (stored in the Windows Credential Manager), the default target folder (with a folder picker), the default parallelism, and the theme (System, Light, or Dark); an Advanced section toggles the startup splash and its duration. diff --git a/docs/RELEASING.md b/docs/RELEASING.md index 45baf79..12e250d 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -81,9 +81,9 @@ via `wingetcreate update KofTwentyTwo.gclo ... --submit`. ## Cutting a release, start to finish 1. **Make sure `main` is green.** The CI, CodeQL, and (for PRs) dependency - review workflows must all pass. The release workflow re-runs the build with - `-warnaserror` and the test suite, so a red `main` will fail the release - anyway — just later and more annoyingly. + review workflows must all pass. The release workflow re-runs the entire CI + workflow against the tag, so a red `main` will fail the release anyway — + just later and more annoyingly. 2. **Pick the version.** Follow semver: breaking change → major, new feature → minor, fix → patch. Add a prerelease suffix (`-beta.1`) if this should go to @@ -99,17 +99,28 @@ via `wingetcreate update KofTwentyTwo.gclo ... --submit`. ``` 4. **Watch the workflow.** The `Release` workflow appears under the Actions - tab. It is a single job that will, in order: - - validate the tag and derive version/channel, - - build the solution (x64, warnings as errors) and run the tests, - - publish and zip the CLI, - - publish the WinUI app unpackaged and pack it with Velopack, - - create a **draft** GitHub Release (marked prerelease for `dev`), upload - all assets and the generated changelog to it, then **publish** it — this - repository uses immutable releases, so everything must land while the - release is still a draft, - - push `gclo.Engine` to nuget.org (if `NUGET_API_KEY` is set), - - submit the winget manifest update (stable only, if `WINGET_TOKEN` is set). + tab. It runs three jobs, each gated on the previous one: + - **Verify tag** — validate the tag, derive version/channel, and check + that the tagged commit is contained in `main`. A tag on any other commit + fails here and nothing is built or published. + - **Gates** — the full `CI` workflow, exactly as a pull request runs it: + x64 build with warnings as errors, 100% line coverage on + `gclo.Engine` / `gclo.ViewModels` / `gclo`, and the formatting gate. + The FlaUI UI smoke suite runs as well but is **advisory** for a release, + matching its status on `main` (not a required check yet, because UI + automation on hosted runners is still proving itself). A red UI job + shows in the run but does not stop publishing; look at it afterwards. + If a *blocking* gate fails, fix the cause on `main` and cut a new patch + version — never reuse the tag (see "If a release goes wrong"). + - **Build, package, and publish**, which will, in order: + - publish and zip the CLI, + - publish the WinUI app unpackaged and pack it with Velopack, + - create a **draft** GitHub Release (marked prerelease for `dev`), upload + all assets and the generated changelog to it, then **publish** it — this + repository uses immutable releases, so everything must land while the + release is still a draft, + - push `gclo.Engine` to nuget.org (if `NUGET_API_KEY` is set), + - submit the winget manifest update (stable only, if `WINGET_TOKEN` is set). 5. **Verify.** Check the new release on the [releases page](https://github.com/KofTwentyTwo/gclo/releases): the Setup diff --git a/gclo.Cli.Tests/gclo.Cli.Tests.csproj b/gclo.Cli.Tests/gclo.Cli.Tests.csproj index 62e69ff..c82c882 100644 --- a/gclo.Cli.Tests/gclo.Cli.Tests.csproj +++ b/gclo.Cli.Tests/gclo.Cli.Tests.csproj @@ -8,10 +8,10 @@ - - + + - + diff --git a/gclo.Engine.Tests/AccountsStoreConsistencyTests.cs b/gclo.Engine.Tests/AccountsStoreConsistencyTests.cs new file mode 100644 index 0000000..2445d86 --- /dev/null +++ b/gclo.Engine.Tests/AccountsStoreConsistencyTests.cs @@ -0,0 +1,391 @@ +using gclo.Engine; +using gclo.ViewModels; + +namespace gclo.Engine.Tests; + +/// +/// Contract tests for the two-store consistency rules of +/// (see its class remarks): the order of vault and metadata writes, the compensating +/// step at every partial-failure point, what is left behind when compensation fails +/// too, that a plain retry reconciles every such state, and that no path ever logs +/// or surfaces a token. +/// +public sealed class AccountsStoreConsistencyTests : IDisposable +{ + private const string OldToken = "ghp_OLD_TOKEN_VALUE"; + private const string NewToken = "ghp_NEW_TOKEN_VALUE"; + + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + private readonly FaultableVault _vault = new(); + private readonly RecordingLog _log = new(); + + public void Dispose() + { + UnblockMetadataWrites(); + GitTestHelpers.TryDeleteDirectory(_root); + } + + private AccountsStore NewStore() => new(_vault, _root, _log); + + private static Account MakeAccount(string name) => new() + { + Id = Guid.NewGuid(), + Name = name, + Organization = "acme", + TargetRoot = @"C:\repos", + }; + + /// + /// Makes the next metadata write fail before it changes anything: the store writes + /// to accounts.json.tmp first, and a directory squatting on that name makes the + /// FileStream creation throw UnauthorizedAccessException. + /// + private void BlockMetadataWrites() + => Directory.CreateDirectory(Path.Combine(_root, "accounts.json.tmp")); + + private void UnblockMetadataWrites() + { + string squatter = Path.Combine(_root, "accounts.json.tmp"); + if (Directory.Exists(squatter)) + { + Directory.Delete(squatter); + } + } + + private IReadOnlyList OnDisk() => new AccountsStore(new InMemoryVault(), _root).GetAll(); + + // ---------------------------------------------------------------- save: vault write fails + + [Fact] + public void Save_NewAccount_VaultWriteFails_ChangesNothing() + { + var store = NewStore(); + var account = MakeAccount("Work"); + _vault.StoreFailure = new InvalidOperationException("credential manager locked"); + + var ex = Assert.Throws(() => store.Save(account, NewToken)); + + Assert.Equal("credential manager locked", ex.Message); + Assert.Empty(store.GetAll()); + Assert.False(File.Exists(Path.Combine(_root, "accounts.json")), "metadata must not be written before the vault"); + Assert.Null(_vault.TryRetrieve(account.Id)); + } + + [Fact] + public void Save_UpdateWithNewToken_VaultWriteFails_KeepsPriorMetadataAndPriorToken() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + _vault.StoreFailure = new InvalidOperationException("credential manager locked"); + + Assert.Throws( + () => store.Save(account with { Description = "edited" }, NewToken)); + + // The replacement failed, so the account keeps the credential that worked. + Assert.Equal(OldToken, _vault.TryRetrieve(account.Id)); + Assert.Equal("", Assert.Single(store.GetAll()).Description); + Assert.Equal("", Assert.Single(OnDisk()).Description); + } + + // ---------------------------------------------------------------- save: metadata write fails + + [Fact] + public void Save_NewAccount_MetadataWriteFails_RemovesTheVaultEntryAgain() + { + var store = NewStore(); + var account = MakeAccount("Work"); + BlockMetadataWrites(); + + Assert.Throws(() => store.Save(account, NewToken)); + + // No vault entry may point at an account that was never saved. + Assert.Null(_vault.TryRetrieve(account.Id)); + Assert.Empty(store.GetAll()); + Assert.Equal(1, _vault.DeleteCalls); + } + + [Fact] + public void Save_UpdateWithNewToken_MetadataWriteFails_RestoresThePreviousToken() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + + Assert.Throws(() => store.Save(account with { Description = "edited" }, NewToken)); + + Assert.Equal(OldToken, _vault.TryRetrieve(account.Id)); + Assert.Equal("", Assert.Single(store.GetAll()).Description); + Assert.Equal("", Assert.Single(OnDisk()).Description); + } + + [Fact] + public void Save_WithoutToken_MetadataWriteFails_NeverTouchesTheVault() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + _vault.ResetCounters(); + BlockMetadataWrites(); + + Assert.Throws(() => store.Save(account with { Description = "edited" }, token: null)); + + Assert.Equal(0, _vault.StoreCalls + _vault.DeleteCalls + _vault.RetrieveCalls); + Assert.Equal(OldToken, _vault.TryRetrieve(account.Id)); + } + + // ---------------------------------------------------------------- save: compensation fails + + [Fact] + public void Save_NewAccount_MetadataAndCompensationFail_SurfacesBothWithoutTheToken() + { + var store = NewStore(); + var account = MakeAccount("Work"); + BlockMetadataWrites(); + _vault.DeleteFailure = new InvalidOperationException("delete refused"); + + var ex = Assert.Throws(() => store.Save(account, NewToken)); + + Assert.IsType(ex.InnerException); + Assert.Equal("delete refused", ex.CompensationException.Message); + Assert.Contains("not in accounts.json", ex.Message); + Assert.Contains(account.Id.ToString("N"), ex.Message); + AssertNoTokenLeaked(ex.Message); + Assert.Equal(NewToken, _vault.TryRetrieve(account.Id)); // the documented leftover + Assert.Empty(store.GetAll()); + + var logged = Assert.Single(_log.Errors); + Assert.Contains("could not be restored", logged.Message); + AssertNoTokenLeaked(logged.Message); + } + + [Fact] + public void Save_Update_MetadataAndCompensationFail_SurfacesBothWithoutTheToken() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + _vault.ResetCounters(); + _vault.StoreFailureFromCall = 2; // the replacement write succeeds, the restore does not + _vault.StoreFailure = new InvalidOperationException("restore refused"); + + var ex = Assert.Throws( + () => store.Save(account with { Description = "edited" }, NewToken)); + + Assert.Equal("restore refused", ex.CompensationException.Message); + Assert.Contains("previous metadata", ex.Message); + AssertNoTokenLeaked(ex.Message); + Assert.Equal(NewToken, _vault.TryRetrieve(account.Id)); // the documented leftover + Assert.Equal("", Assert.Single(store.GetAll()).Description); + AssertNoTokenLeaked(Assert.Single(_log.Errors).Message); + } + + // ---------------------------------------------------------------- delete + + [Fact] + public void Delete_VaultDeleteFails_ChangesNothing() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + _vault.DeleteFailure = new InvalidOperationException("delete refused"); + + Assert.Throws(() => store.Delete(account.Id)); + + Assert.Equal(OldToken, _vault.TryRetrieve(account.Id)); + Assert.Single(store.GetAll()); + Assert.Single(OnDisk()); + } + + [Fact] + public void Delete_MetadataWriteFails_RestoresTheTokenAndKeepsTheAccount() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + + Assert.Throws(() => store.Delete(account.Id)); + + // Not orphaned, not lost: the account is still listed AND still has its token. + Assert.Equal(OldToken, _vault.TryRetrieve(account.Id)); + Assert.Single(store.GetAll()); + Assert.Single(OnDisk()); + } + + [Fact] + public void Delete_AccountWithoutToken_MetadataWriteFails_HasNothingToRestore() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, token: null); + _vault.ResetCounters(); + BlockMetadataWrites(); + + Assert.Throws(() => store.Delete(account.Id)); + + Assert.Equal(0, _vault.StoreCalls); + Assert.Single(store.GetAll()); + } + + [Fact] + public void Delete_MetadataAndCompensationFail_SurfacesBothWithoutTheToken() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + _vault.StoreFailure = new InvalidOperationException("restore refused"); + + var ex = Assert.Throws(() => store.Delete(account.Id)); + + Assert.IsType(ex.InnerException); + Assert.Equal("restore refused", ex.CompensationException.Message); + Assert.Contains("still listed", ex.Message); + AssertNoTokenLeaked(ex.Message); + Assert.Null(_vault.TryRetrieve(account.Id)); // the documented leftover + Assert.Single(store.GetAll()); + AssertNoTokenLeaked(Assert.Single(_log.Errors).Message); + } + + // ---------------------------------------------------------------- retry reconciles every leftover + + [Fact] + public void Save_RetryAfterMetadataFailure_Succeeds() + { + var store = NewStore(); + var account = MakeAccount("Work"); + BlockMetadataWrites(); + Assert.Throws(() => store.Save(account, NewToken)); + UnblockMetadataWrites(); + + store.Save(account, NewToken); + + Assert.Equal(NewToken, _vault.TryRetrieve(account.Id)); + Assert.Equal(account, Assert.Single(OnDisk())); + } + + [Fact] + public void Save_RetryAfterOrphanedVaultEntry_Reconciles() + { + var store = NewStore(); + var account = MakeAccount("Work"); + BlockMetadataWrites(); + _vault.DeleteFailure = new InvalidOperationException("delete refused"); + Assert.Throws(() => store.Save(account, NewToken)); + UnblockMetadataWrites(); + _vault.DeleteFailure = null; + + store.Save(account, NewToken); + + Assert.Equal(NewToken, _vault.TryRetrieve(account.Id)); + Assert.Equal(account, Assert.Single(store.GetAll())); + } + + [Fact] + public void Delete_RetryAfterTokenRestoreFailed_CompletesTheRemoval() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + _vault.StoreFailure = new InvalidOperationException("restore refused"); + Assert.Throws(() => store.Delete(account.Id)); + UnblockMetadataWrites(); + _vault.StoreFailure = null; + + store.Delete(account.Id); // the vault entry is already gone; that must be fine + + Assert.Empty(store.GetAll()); + Assert.Empty(OnDisk()); + Assert.Null(_vault.TryRetrieve(account.Id)); + } + + [Fact] + public void Delete_RetryAfterMetadataFailure_Succeeds() + { + var store = NewStore(); + var account = MakeAccount("Work"); + store.Save(account, OldToken); + BlockMetadataWrites(); + Assert.Throws(() => store.Delete(account.Id)); + UnblockMetadataWrites(); + + store.Delete(account.Id); + + Assert.Empty(store.GetAll()); + Assert.Null(_vault.TryRetrieve(account.Id)); + } + + // ---------------------------------------------------------------- helpers + + private static void AssertNoTokenLeaked(string text) + { + Assert.DoesNotContain(OldToken, text); + Assert.DoesNotContain(NewToken, text); + } + + /// + /// An over whose Store and + /// Delete can be made to throw (from a given call number on) without changing + /// state — the contract a real secret store keeps on failure. + /// + private sealed class FaultableVault : ITokenVault + { + private readonly InMemoryVault _inner = new(); + + public Exception? StoreFailure { get; set; } + public int StoreFailureFromCall { get; set; } = 1; + public Exception? DeleteFailure { get; set; } + + public int StoreCalls { get; private set; } + public int DeleteCalls { get; private set; } + public int RetrieveCalls { get; private set; } + + public void ResetCounters() => StoreCalls = DeleteCalls = RetrieveCalls = 0; + + public void Store(Guid accountId, string token) + { + StoreCalls++; + if (StoreFailure is not null && StoreCalls >= StoreFailureFromCall) + { + throw StoreFailure; + } + _inner.Store(accountId, token); + } + + public string? TryRetrieve(Guid accountId) + { + RetrieveCalls++; + return _inner.TryRetrieve(accountId); + } + + public void Delete(Guid accountId) + { + DeleteCalls++; + if (DeleteFailure is not null) + { + throw DeleteFailure; + } + _inner.Delete(accountId); + } + } + + private sealed class RecordingLog : IActivityLog + { + public List<(string Message, Exception? Exception)> Errors { get; } = []; + + public void Info(string message) + { + } + + public void Error(string message, Exception? exception = null) => Errors.Add((message, exception)); + + public string LogDirectory => ""; + + public string CurrentLogFilePath => ""; + } +} diff --git a/gclo.Engine.Tests/LibGit2GitClientTests.cs b/gclo.Engine.Tests/LibGit2GitClientTests.cs index eeb488f..f696ced 100644 --- a/gclo.Engine.Tests/LibGit2GitClientTests.cs +++ b/gclo.Engine.Tests/LibGit2GitClientTests.cs @@ -44,6 +44,33 @@ public sealed class LibGit2GitClientTests : IDisposable Assert.All(progressValues, v => Assert.InRange(v, 0.0, 1.0)); } + [Fact] + public async Task Clone_Completed_LeavesCheckoutPendingMarkerCleared() + { + string source = CreateSourceRepo(); + string target = NewPath("clone-target"); + + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + + // The marker is set when the fetch lands and cleared only after checkout, so a + // finished clone must read as "not pending" — the next pull trusts its tree. + Assert.False(IsCheckoutPending(target)); + } + + [Fact] + public async Task Clone_EmptyOrigin_LeavesCheckoutPendingMarkerCleared() + { + string origin = NewPath("bare-origin"); + Repository.Init(origin, isBare: true); + string target = NewPath("unborn-clone"); + + await _client.CloneAsync(origin, target, Token, null, CancellationToken.None); + + // Nothing to check out means nothing is pending; the unborn-HEAD pull path + // (not the pending path) must handle this repo once the remote gets commits. + Assert.False(IsCheckoutPending(target)); + } + [Fact] public async Task Clone_SourceMissing_ThrowsAndLeavesNoTargetDirectory() { @@ -161,6 +188,71 @@ public sealed class LibGit2GitClientTests : IDisposable Assert.Equal(tipBefore, HeadSha(target)); } + [Fact] + public async Task FetchAndPull_IncompleteCloneLeftBehind_CompletesCheckoutInsteadOfReportingUpToDate() + { + // Simulates a clone whose checkout failed (or was killed) and whose cleanup + // could not delete the directory: .git is complete and valid, the marker is + // still set, and the working tree is partial. Commit tips are EQUAL, which is + // exactly the case that used to pass as "already up to date". + string source = CreateSourceRepo(); + string target = NewPath("half-clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + string tipBefore = HeadSha(target); + File.Delete(Path.Combine(target, "readme.txt")); + SetCheckoutPending(target, true); + Assert.True(_client.IsValidRepository(target)); + + await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + + Assert.Equal("hello from source", File.ReadAllText(Path.Combine(target, "readme.txt"))); + Assert.Equal(tipBefore, HeadSha(target)); + Assert.False(IsCheckoutPending(target)); + } + + [Fact] + public async Task FetchAndPull_IncompleteCloneAndRemoteAhead_MaterializesTheNewTip() + { + string source = CreateSourceRepo(); + string target = NewPath("half-clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + File.Delete(Path.Combine(target, "readme.txt")); + SetCheckoutPending(target, true); + string newSha = CommitFile(source, "update.txt", "new content", "second commit"); + + await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + + Assert.Equal(newSha, HeadSha(target)); + Assert.Equal("hello from source", File.ReadAllText(Path.Combine(target, "readme.txt"))); + Assert.Equal("new content", File.ReadAllText(Path.Combine(target, "update.txt"))); + Assert.False(IsCheckoutPending(target)); + } + + [Fact] + public async Task FetchAndPull_IncompleteCloneWhoseTipIsNowInvalidOnWindows_FailsTypedNotUpToDate() + { + // The pending path re-validates before checking out: an incomplete clone whose + // tree gained a Windows-invalid path must surface the structured failure, not + // a silent "Done" with a partial tree. (Forged entries cannot be created on a + // Windows working tree, which is why this uses the object-database fixture.) + string source = CreateSourceRepo(); + string target = NewPath("half-clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + SetCheckoutPending(target, true); + AppendForgedCommit(source, ("bad:name.txt", "unwritable on Windows")); + + if (!OperatingSystem.IsWindows()) + { + return; // the validator only runs on Windows + } + + var ex = await Assert.ThrowsAsync( + () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + + Assert.Equal("bad:name.txt", Assert.Single(ex.Paths).RepoPath); + Assert.True(IsCheckoutPending(target), "still incomplete; the marker must survive the failure"); + } + [Fact] public async Task FetchAndPull_DivergedHistories_ThrowsNonFastForward() { @@ -276,6 +368,18 @@ public sealed class LibGit2GitClientTests : IDisposable private string NewPath(string name) => Path.Combine(_root, name); + private static bool IsCheckoutPending(string workdir) + { + using var repo = new Repository(workdir); + return repo.Config.Get("gclo.checkoutpending")?.Value == true; + } + + private static void SetCheckoutPending(string workdir, bool value) + { + using var repo = new Repository(workdir); + repo.Config.Set("gclo.checkoutpending", value, ConfigurationLevel.Local); + } + /// Initializes a non-bare repository under the test root with one commit. private string CreateSourceRepo(string name = "source") { diff --git a/gclo.Engine.Tests/OrgSyncEngineTests.cs b/gclo.Engine.Tests/OrgSyncEngineTests.cs index 35baf16..244b3ae 100644 --- a/gclo.Engine.Tests/OrgSyncEngineTests.cs +++ b/gclo.Engine.Tests/OrgSyncEngineTests.cs @@ -287,6 +287,62 @@ public sealed class OrgSyncEngineTests : IDisposable Assert.Empty(_lister.Calls); } + [Theory] + [InlineData("../escape")] + [InlineData("..\\escape")] + [InlineData("nested/child")] + [InlineData("nested\\child")] + [InlineData("C:\\Windows")] + [InlineData("/etc")] + [InlineData("\\\\server\\share")] + [InlineData("..")] + [InlineData(".")] + [InlineData("")] + public async Task Sync_RepoNameThatIsNotOneSafeSegment_FailsBeforeTouchingDisk(string badName) + { + // The list overload is a public API: a descriptor whose Name would land + // outside TargetRoot (or deeper than one level) must fail the whole call + // before the root is created, anything is reported, or git is invoked. + RepoDescriptor[] repositories = [Repo("alpha"), Repo(badName), Repo("bravo")]; + + var ex = await Assert.ThrowsAsync( + () => CreateEngine().SyncAsync(CreateRequest(), repositories, _progress)); + + Assert.Contains(badName, ex.Message); + Assert.False(Directory.Exists(_targetRoot), "nothing may be created when a name is rejected"); + Assert.Empty(_progress.Reports); + Assert.Empty(_git.ValidityChecks); + Assert.Empty(_git.CloneCalls); + Assert.Empty(_git.PullCalls); + } + + [Fact] + public async Task Sync_RepoNameFromLister_IsRejectedTheSameWay() + { + _lister.Repositories = [Repo("alpha"), Repo("../escape")]; + + await Assert.ThrowsAsync(() => CreateEngine().SyncAsync(CreateRequest(), _progress)); + + Assert.False(Directory.Exists(_targetRoot)); + Assert.Empty(_git.CloneCalls); + } + + [Fact] + public async Task Sync_PassesTheNormalizedAbsoluteFolder_ToTheGitClient() + { + // A relative TargetRoot is resolved against the current directory once, so + // the folder handed to git is absolute and sits exactly one level below it. + _lister.Repositories = Repos("alpha"); + string relativeRoot = Path.GetRelativePath(Environment.CurrentDirectory, _targetRoot); + var request = new SyncRequest("acme", "test-token", relativeRoot); + + await CreateEngine().SyncAsync(request, _progress); + + var call = Assert.Single(_git.CloneCalls); + Assert.True(Path.IsPathRooted(call.LocalPath)); + Assert.Equal(Path.GetFullPath(Path.Combine(_targetRoot, "alpha")), call.LocalPath); + } + [Fact] public async Task Sync_ListerFailure_PropagatesAndMakesNoGitCalls() { diff --git a/gclo.Engine.Tests/RepositoryPathResolverTests.cs b/gclo.Engine.Tests/RepositoryPathResolverTests.cs new file mode 100644 index 0000000..e16ec78 --- /dev/null +++ b/gclo.Engine.Tests/RepositoryPathResolverTests.cs @@ -0,0 +1,164 @@ +using gclo.Engine; + +namespace gclo.Engine.Tests; + +/// +/// is the trust boundary between caller-supplied +/// repository names and the file system: every name must be exactly one safe folder +/// segment, and the resolved folder must sit directly under the target root. +/// +public sealed class RepositoryPathResolverTests +{ + private static readonly string Root = Path.Combine(Path.GetTempPath(), "gclo-tests", "resolver-root"); + + // ---------------------------------------------------------------- valid names + + [Theory] + [InlineData("gclo")] + [InlineData("my-repo")] + [InlineData("my_repo")] + [InlineData("my.repo")] + [InlineData("Repo.Name-With_Everything.v2")] + [InlineData("123")] + [InlineData(".github")] + [InlineData("...dots")] + [InlineData("name with spaces")] + public void Resolve_ValidGitHubStyleName_ReturnsFolderDirectlyUnderRoot(string name) + { + string resolved = RepositoryPathResolver.Resolve(Root, name); + + Assert.Equal(Path.Combine(Path.GetFullPath(Root), name), resolved); + Assert.Equal(Path.GetFullPath(Root), Path.GetDirectoryName(resolved)); + Assert.True(Path.IsPathRooted(resolved)); + } + + [Fact] + public void Resolve_RootWithTrailingSeparator_ResolvesIdentically() + { + string withSeparator = Root + Path.DirectorySeparatorChar; + + Assert.Equal( + RepositoryPathResolver.Resolve(Root, "alpha"), + RepositoryPathResolver.Resolve(withSeparator, "alpha")); + } + + [Fact] + public void Resolve_RelativeRoot_ResolvesAgainstCurrentDirectory() + { + string resolved = RepositoryPathResolver.Resolve("relative-root", "alpha"); + + Assert.Equal(Path.Combine(Path.GetFullPath("relative-root"), "alpha"), resolved); + } + + [Fact] + public void Resolve_RootContainingDotSegments_IsNormalizedBeforeContainmentCheck() + { + string messyRoot = Path.Combine(Root, "sub", ".."); + + string resolved = RepositoryPathResolver.Resolve(messyRoot, "alpha"); + + Assert.Equal(Path.Combine(Path.GetFullPath(Root), "alpha"), resolved); + } + + [Theory] + [InlineData("gclo")] + [InlineData("my-repo.v2")] + public void TryValidateName_ValidName_ReturnsTrueWithNoReason(string name) + { + Assert.True(RepositoryPathResolver.TryValidateName(name, out string? reason)); + Assert.Null(reason); + } + + // ---------------------------------------------------------------- rejected names + + [Theory] + [InlineData(null, "empty")] + [InlineData("", "empty")] + [InlineData(" ", "empty")] + [InlineData(".", "current or parent")] + [InlineData("..", "current or parent")] + [InlineData("../escape", "separator")] + [InlineData("..\\escape", "separator")] + [InlineData("nested/child", "separator")] + [InlineData("nested\\child", "separator")] + [InlineData("trailing/", "separator")] + [InlineData("/etc", "separator")] + [InlineData("\\\\server\\share", "separator")] + [InlineData("C:\\Windows", "separator")] + [InlineData("C:", ":")] + [InlineData("C:repo", ":")] + [InlineData("name:stream", ":")] + [InlineData("tab\there", "not allowed")] + [InlineData("nul\0byte", "not allowed")] + public void TryValidateName_UnsafeName_ReturnsFalseWithReason(string? name, string expectedReasonFragment) + { + Assert.False(RepositoryPathResolver.TryValidateName(name, out string? reason)); + Assert.NotNull(reason); + Assert.Contains(expectedReasonFragment, reason); + } + + [Theory] + [InlineData("../escape")] + [InlineData("..\\escape")] + [InlineData("nested/child")] + [InlineData("C:\\Windows")] + [InlineData("/etc")] + [InlineData("\\\\server\\share")] + [InlineData("..")] + [InlineData("")] + public void Resolve_UnsafeName_ThrowsArgumentExceptionNamingTheRepo(string name) + { + var ex = Assert.Throws(() => RepositoryPathResolver.Resolve(Root, name)); + + Assert.Equal("repoName", ex.ParamName); + Assert.Contains($"'{name}'", ex.Message); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Resolve_BlankRoot_ThrowsArgumentException(string root) + { + Assert.ThrowsAny(() => RepositoryPathResolver.Resolve(root, "alpha")); + } + + [Fact] + public void Resolve_NullRoot_ThrowsArgumentNullException() + { + Assert.Throws(() => RepositoryPathResolver.Resolve(null!, "alpha")); + } + + // ---------------------------------------------------------------- containment (the backstop) + + [Fact] + public void Resolve_NameThatNormalizesOntoTheRootItself_IsRejected() + { + // "..." passes the textual checks (it is not "." or ".." and has no separator), + // but Windows path normalization trims trailing dots so it would resolve to the + // root itself. The containment check must catch it on Windows; elsewhere "..." + // is a legitimate folder name. + if (!OperatingSystem.IsWindows()) + { + Assert.Equal(Path.Combine(Path.GetFullPath(Root), "..."), RepositoryPathResolver.Resolve(Root, "...")); + return; + } + + var ex = Assert.Throws(() => RepositoryPathResolver.Resolve(Root, "...")); + + Assert.Contains("not inside the target root", ex.Message); + } + + [Fact] + public void Resolve_IsCaseInsensitiveOnWindows_ForTheRootPrefix() + { + if (!OperatingSystem.IsWindows()) + { + return; + } + + // The root's own casing must not matter for containment on Windows. + string resolved = RepositoryPathResolver.Resolve(Root.ToUpperInvariant(), "alpha"); + + Assert.EndsWith(Path.DirectorySeparatorChar + "alpha", resolved); + } +} diff --git a/gclo.Engine.Tests/gclo.Engine.Tests.csproj b/gclo.Engine.Tests/gclo.Engine.Tests.csproj index bb452e5..fa2e525 100644 --- a/gclo.Engine.Tests/gclo.Engine.Tests.csproj +++ b/gclo.Engine.Tests/gclo.Engine.Tests.csproj @@ -8,10 +8,10 @@ - - + + - + diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index a2907f6..62c611b 100644 --- a/gclo.Engine/LibGit2GitClient.cs +++ b/gclo.Engine/LibGit2GitClient.cs @@ -28,10 +28,15 @@ namespace gclo.Engine; public sealed class LibGit2GitClient : IGitClient { /// - /// Local git config flag set when a clone fetched successfully but its tree failed - /// Windows path validation, so nothing was ever checked out. Cleared once a working - /// tree is materialized (normal checkout after upstream fixed the paths, or - /// ). + /// Local git config flag meaning "the objects are here, but the working tree was + /// never completely checked out". Set the moment a clone's fetch completes and + /// cleared only after checkout finishes, so it is durable evidence of an + /// incomplete clone whatever interrupted it: Windows path validation (the repo is + /// deliberately kept for recovery), a checkout error whose best-effort cleanup + /// could not delete the directory, or the process dying mid-checkout. The next + /// pull sees the flag and completes the checkout instead of comparing commit tips + /// and calling an incomplete tree "up to date". Cleared by a normal checkout or by + /// . /// private const string CheckoutPendingKey = "gclo.checkoutpending"; @@ -99,6 +104,13 @@ public sealed class LibGit2GitClient : IGitClient Repository.Clone(url, path, options); using var repo = new Repository(path); + + // From here until checkout completes the directory is a valid repository + // with no (or a partial) working tree. Record that durably first: if the + // cleanup in the catch below cannot remove the directory, or the process + // dies, the next run must not mistake this for a finished clone. + repo.Config.Set(CheckoutPendingKey, true, ConfigurationLevel.Local); + if (OperatingSystem.IsWindows()) { // Lifts the 260-character path limit before anything touches the @@ -109,7 +121,9 @@ public sealed class LibGit2GitClient : IGitClient var tip = repo.Head.Tip; if (tip is null) { - return; // empty repository — nothing to check out + // Empty repository — nothing to check out, so nothing is pending. + repo.Config.Set(CheckoutPendingKey, false, ConfigurationLevel.Local); + return; } if (OperatingSystem.IsWindows()) @@ -119,9 +133,8 @@ public sealed class LibGit2GitClient : IGitClient { // Keep the fetched repo: all objects are already downloaded, so // ApplyRecoveryAsync can materialize a sanitized working tree without - // touching the network. The marker tells FetchAndPull that this - // repository was never checked out. - repo.Config.Set(CheckoutPendingKey, true, ConfigurationLevel.Local); + // touching the network. The pending marker (already set) tells + // FetchAndPull that this repository was never checked out. throw new InvalidRepositoryPathsException(invalidPaths); } } @@ -131,13 +144,19 @@ public sealed class LibGit2GitClient : IGitClient { CheckoutModifiers = CheckoutModifiers.Force, // materialize the fresh working tree }); + + // Only now is the clone complete. Config writes are atomic on disk, so an + // observer sees either "pending" or "done", never a torn state. + repo.Config.Set(CheckoutPendingKey, false, ConfigurationLevel.Local); } catch (Exception ex) { // Don't leave a half-cloned directory behind: the next run would treat a // partial checkout as "exists locally" and try to pull it. Invalid-path // failures are the deliberate exception — the fetched repo is kept (with - // the pending marker set above) so recovery needs no re-download. + // the pending marker set above) so recovery needs no re-download. If this + // delete fails (open handle, antivirus, permissions) the marker is still + // set, and the next pull completes the checkout instead of trusting the tree. if (!existedBefore && ex is not InvalidRepositoryPathsException) { TryDeleteDirectory(path); @@ -182,11 +201,12 @@ public sealed class LibGit2GitClient : IGitClient ct.ThrowIfCancellationRequested(); - // Repositories whose clone hit Windows-invalid paths never had a checkout; - // they carry a pending marker (and, once the user chose renames/skips, a - // persisted recovery). Both take a dedicated path — their working tree is - // materialized manually, never by a merge checkout. Every other repository - // takes the normal fetch + fast-forward pull below. + // Repositories whose clone never finished its checkout — Windows-invalid + // paths, a checkout error whose cleanup failed, a killed process — carry the + // pending marker (and, once the user chose renames/skips, a persisted + // recovery). Both take a dedicated path — their working tree is materialized + // from the tip, never by a merge checkout that trusts the existing tree. + // Every other repository takes the normal fetch + fast-forward pull below. string recoveryFile = GetRecoveryFilePath(repo); if (File.Exists(recoveryFile)) { @@ -458,10 +478,11 @@ public sealed class LibGit2GitClient : IGitClient } /// - /// Finishes a clone that was interrupted by invalid paths but has no stored - /// recovery: if upstream has fixed the paths, check out normally and clear the - /// marker; otherwise rethrow the same typed failure — the repo must not silently - /// report success with an empty working tree. + /// Finishes a clone whose checkout never completed and that has no stored + /// recovery: moves to the upstream tip, re-validates its paths on Windows, and + /// force-checks-out the whole tree (whatever partial files exist are overwritten + /// to match). If the paths are still invalid, rethrows the same typed failure — + /// the repo must not silently report success with an empty or partial tree. /// private static void CompletePendingCheckout(Repository repo, CancellationToken ct) { diff --git a/gclo.Engine/OrgSyncEngine.cs b/gclo.Engine/OrgSyncEngine.cs index 8122f1e..398578f 100644 --- a/gclo.Engine/OrgSyncEngine.cs +++ b/gclo.Engine/OrgSyncEngine.cs @@ -64,6 +64,14 @@ public sealed class OrgSyncEngine .DistinctBy(r => r.Name, StringComparer.OrdinalIgnoreCase) .ToList(); + // Resolve every target folder before anything is created or reported: this + // overload accepts caller-supplied names, and a name that is not one safe + // segment under TargetRoot (rooted, traversal, separators) fails the whole + // call here rather than cloning somewhere the caller never asked for. + var targets = repos + .Select(r => (Repo: r, Path: RepositoryPathResolver.Resolve(request.TargetRoot, r.Name))) + .ToList(); + // Create the root before anything is reported as Queued: if the path is // invalid, the caller gets one exception instead of repos stranded mid-state. Directory.CreateDirectory(request.TargetRoot); @@ -92,9 +100,9 @@ public sealed class OrgSyncEngine { // Parallel.ForEachAsync stops scheduling as soon as any body throws, so the // body must swallow every per-repo failure; only cancellation may escape. - await Parallel.ForEachAsync(repos, parallelOptions, async (repo, token) => + await Parallel.ForEachAsync(targets, parallelOptions, async (target, token) => { - var path = Path.Combine(request.TargetRoot, repo.Name); + var (repo, path) = target; try { token.ThrowIfCancellationRequested(); diff --git a/gclo.Engine/RepositoryPathResolver.cs b/gclo.Engine/RepositoryPathResolver.cs new file mode 100644 index 0000000..7d45d68 --- /dev/null +++ b/gclo.Engine/RepositoryPathResolver.cs @@ -0,0 +1,104 @@ +namespace gclo.Engine; + +/// +/// Maps a repository name onto its folder under a sync target root, and refuses any +/// name that is not exactly one safe path segment. runs +/// every name through this before it creates a directory or starts a git operation, +/// because the public sync overload accepts caller-supplied descriptors: a name such +/// as ..\other or C:\Windows must fail the call, not clone outside the +/// root the caller asked for. +/// +public static class RepositoryPathResolver +{ + private static readonly char[] SeparatorChars = ['/', '\\']; + + private static readonly char[] InvalidNameChars = Path.GetInvalidFileNameChars(); + + // Windows paths compare case-insensitively; everywhere else the file system is + // (by default) case-sensitive and so is the containment check. + private static readonly StringComparison PathComparison = + OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + + /// + /// Returns the normalized absolute folder for under + /// , after proving the name is a single safe segment + /// and the resolved folder is strictly inside the root. + /// + /// + /// is blank, or is not a + /// valid leaf folder name, or the resolved path escapes the root. + /// + public static string Resolve(string targetRoot, string repoName) + { + ArgumentException.ThrowIfNullOrWhiteSpace(targetRoot); + + if (!TryValidateName(repoName, out string? reason)) + { + throw new ArgumentException( + $"Repository name '{repoName}' cannot be used as a folder name: {reason}.", nameof(repoName)); + } + + // Normalize both sides the same way, then require the candidate to sit one + // level below the root. GetFullPath collapses '.'/'..' segments and, on + // Windows, trims trailing dots and spaces — so a name that survived the + // textual checks but still lands on or outside the root is caught here. + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(targetRoot)) + Path.DirectorySeparatorChar; + string candidate = Path.GetFullPath(Path.Combine(root, repoName)); + + if (!candidate.StartsWith(root, PathComparison) || candidate.Length <= root.Length) + { + throw new ArgumentException( + $"Repository name '{repoName}' resolves to '{candidate}', which is not inside the target root '{root}'.", + nameof(repoName)); + } + + return candidate; + } + + /// + /// True when is a single, safe folder-name segment: + /// not blank, not . or .., no directory separators (so never rooted + /// or UNC), no drive or stream colon, and no characters a file system rejects in a name. + /// + /// The candidate name. + /// Why the name was rejected; null when it is valid. + public static bool TryValidateName(string? repoName, out string? reason) + { + if (string.IsNullOrWhiteSpace(repoName)) + { + reason = "it is empty"; + return false; + } + + if (repoName is "." or "..") + { + reason = "it refers to the current or parent directory"; + return false; + } + + if (repoName.IndexOfAny(SeparatorChars) >= 0) + { + reason = "it contains a directory separator"; + return false; + } + + // ':' is a drive or alternate-data-stream delimiter on Windows ('C:' and + // 'C:repo' are drive-relative paths). Reject it on every platform so a name's + // validity does not depend on where it is checked. With separators and the + // colon gone, no remaining string is rooted on any supported platform. + if (repoName.Contains(':')) + { + reason = "it contains ':'"; + return false; + } + + if (repoName.IndexOfAny(InvalidNameChars) >= 0 || repoName.Any(char.IsControl)) + { + reason = "it contains characters that are not allowed in a folder name"; + return false; + } + + reason = null; + return true; + } +} diff --git a/gclo.Engine/gclo.Engine.csproj b/gclo.Engine/gclo.Engine.csproj index db2732e..5e016a7 100644 --- a/gclo.Engine/gclo.Engine.csproj +++ b/gclo.Engine/gclo.Engine.csproj @@ -22,7 +22,7 @@ - + diff --git a/gclo.UiTests/gclo.UiTests.csproj b/gclo.UiTests/gclo.UiTests.csproj index 0b3ddf2..9855dd8 100644 --- a/gclo.UiTests/gclo.UiTests.csproj +++ b/gclo.UiTests/gclo.UiTests.csproj @@ -15,9 +15,9 @@ - + - + diff --git a/gclo.ViewModels/AccountConsistencyException.cs b/gclo.ViewModels/AccountConsistencyException.cs new file mode 100644 index 0000000..e15a04d --- /dev/null +++ b/gclo.ViewModels/AccountConsistencyException.cs @@ -0,0 +1,22 @@ +namespace gclo.ViewModels; + +/// +/// Thrown by when an operation failed AND the +/// compensating step that would have restored consistency between accounts.json and +/// the token vault failed too. The message describes the exact state left behind +/// and how the next operation reconciles it; it never contains a token. +/// is the original failure and +/// the one that prevented recovery. +/// +public sealed class AccountConsistencyException : Exception +{ + /// The failure that occurred while undoing the partial operation. + public Exception CompensationException { get; } + + /// Creates the exception from the original and compensating failures. + public AccountConsistencyException(string message, Exception original, Exception compensation) + : base(message, original) + { + CompensationException = compensation; + } +} diff --git a/gclo.ViewModels/AccountsStore.cs b/gclo.ViewModels/AccountsStore.cs index efdf04d..7a81a0b 100644 --- a/gclo.ViewModels/AccountsStore.cs +++ b/gclo.ViewModels/AccountsStore.cs @@ -13,6 +13,32 @@ namespace gclo.ViewModels; /// propagate instead of being swallowed; only loading /// is tolerant (a missing or corrupt file yields an empty list). /// +/// +/// +/// Metadata and the vault are two stores with no shared transaction, so every +/// operation that touches both has a fixed order and a compensating step: +/// +/// +/// +/// Save with a token: vault write first, then metadata. A vault failure +/// changes nothing (the vault's either replaces the +/// entry or leaves the previous one intact, so an update keeps its working +/// credential). A metadata failure is compensated by restoring the previous token +/// (or deleting the entry for a brand-new account). +/// +/// +/// Delete: vault delete first, then metadata. A vault failure changes nothing. +/// A metadata failure is compensated by re-storing the token that was removed. +/// +/// +/// Compensation failure surfaces as +/// whose message states the exact leftover state. Every such state is reconciled by +/// simply retrying the same operation: Save overwrites the vault entry and rewrites +/// the file; Delete tolerates an absent vault entry. +/// +/// +/// In-memory state changes only after both stores succeeded. Nothing here logs a token. +/// public sealed class AccountsStore { private readonly ITokenVault _vault; @@ -60,9 +86,11 @@ public sealed class AccountsStore /// Inserts or updates (matching by ) and persists /// immediately. Throws when another account /// already uses the name (case-insensitive); nothing is persisted in that case. - /// A non-null goes into the vault only after the - /// metadata write succeeds, so a vault entry never points at an unsaved account; - /// a null token leaves any existing vault entry untouched. + /// A non-null is written to the vault BEFORE the + /// metadata; if the metadata write then fails, the vault is put back the way it + /// was (previous token restored, or the new entry removed) and the failure + /// propagates. A null token leaves any existing vault entry untouched. + /// See the class remarks for the full consistency contract. /// public void Save(Account account, string? token) { @@ -89,19 +117,63 @@ public sealed class AccountsStore updated.Add(account); } - Persist(updated); // IO failures propagate; _accounts stays unchanged then. - _accounts = updated; - - if (token is not null) + if (token is null) { - _vault.Store(account.Id, token); + Persist(updated); // IO failures propagate; _accounts stays unchanged then. + _accounts = updated; + return; } + + // Vault first: a failing Store leaves the vault exactly as it was (the + // previous token, if any, is still there), and nothing else has changed. + string? previousToken = _vault.TryRetrieve(account.Id); + _vault.Store(account.Id, token); + + try + { + Persist(updated); + } + catch (Exception persistFailure) + { + // Undo the vault write so no entry points at metadata that was never saved. + try + { + if (previousToken is null) + { + _vault.Delete(account.Id); + } + else + { + _vault.Store(account.Id, previousToken); + } + } + catch (Exception compensationFailure) + { + string leftover = previousToken is null + ? $"the vault holds a token for account {account.Id:N} ('{account.Name}') that is not in accounts.json; " + + "saving the account again overwrites it, deleting the account removes it" + : $"the vault holds the NEW token for account {account.Id:N} ('{account.Name}') while accounts.json " + + "still has the previous metadata; saving the account again reconciles both"; + _log.Error($"Account save failed and the vault could not be restored: {leftover}.", compensationFailure); + throw new AccountConsistencyException( + $"Saving account '{account.Name}' failed ({persistFailure.Message}), and restoring the token vault " + + $"failed too ({compensationFailure.Message}). Current state: {leftover}.", + persistFailure, compensationFailure); + } + + throw; + } + + _accounts = updated; } } /// - /// Removes the account's metadata and vault token; an unknown id is a no-op. - /// The vault entry is deleted only after the metadata write succeeds. + /// Removes the account's vault token and then its metadata; an unknown id is a + /// no-op. A vault failure changes nothing. If the metadata write fails after the + /// token was removed, the token is stored again and the failure propagates, so a + /// listed account never silently loses its credential and a token is never left + /// behind without its account. See the class remarks for the full contract. /// public void Delete(Guid id) { @@ -113,9 +185,41 @@ public sealed class AccountsStore return; } - Persist(remaining); - _accounts = remaining; + // Vault first: a failing Delete leaves both stores untouched. Keep the + // token in memory only long enough to put it back if the file write fails. + string? removedToken = _vault.TryRetrieve(id); _vault.Delete(id); + + try + { + Persist(remaining); + } + catch (Exception persistFailure) + { + if (removedToken is null) + { + throw; // there was no token to restore; the account is simply still listed + } + + try + { + _vault.Store(id, removedToken); + } + catch (Exception compensationFailure) + { + string leftover = $"account {id:N} is still listed in accounts.json but its token is gone from the vault; " + + "deleting it again completes the removal, or edit it and enter the token again to keep it"; + _log.Error($"Account delete failed and the token could not be restored: {leftover}.", compensationFailure); + throw new AccountConsistencyException( + $"Deleting the account failed ({persistFailure.Message}), and restoring its token failed too " + + $"({compensationFailure.Message}). Current state: {leftover}.", + persistFailure, compensationFailure); + } + + throw; + } + + _accounts = remaining; } } diff --git a/gclo.ViewModels/ITokenVault.cs b/gclo.ViewModels/ITokenVault.cs index 93dedc6..000976b 100644 --- a/gclo.ViewModels/ITokenVault.cs +++ b/gclo.ViewModels/ITokenVault.cs @@ -11,7 +11,9 @@ public interface ITokenVault /// /// Stores the token for an account, overwriting any existing entry. Throws on /// failure — silently losing a token would leave the account unusable without - /// any warning to the user. + /// any warning to the user. A failed call must leave the vault as it was (the + /// previous entry, if any, still readable): relies on + /// that to keep an account's working credential when a replacement write fails. /// void Store(Guid accountId, string token); diff --git a/gclo/gclo.csproj b/gclo/gclo.csproj index 2c9a5f4..5cefa7e 100644 --- a/gclo/gclo.csproj +++ b/gclo/gclo.csproj @@ -44,9 +44,9 @@ - - - + + +