diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 9c54ce96..6b1e1c0e 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -46,6 +46,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv - **`Bun.Glob.scan({ dot: false })` silently drops dot-prefixed segments — even on explicit paths** — `dot: false` (the default) skips matches whose path contains a `.`-prefixed segment, including patterns that explicitly name the dir (e.g. `.github/workflows/release.yml`). Behavior also varies across platforms — Windows reliably drops the match while Linux can match the same pattern, so a rule appears to "work in CI" but no-ops locally. For code repos where `.github/`, `.husky/`, `.vscode/` are first-class source dirs, ALWAYS pass `dot: true` to `Bun.Glob.scan()`. Applied in `src/engine/runner.ts` (`ctx.glob`, `ctx.grepFiles`) and `src/engine/git-files.ts` (`resolveScopedFiles`). See archgate/cli#222. - **PowerShell 5.1 reads BOM-less `.ps1` files as ANSI — never use non-ASCII chars in `install.ps1`** — `install.ps1` has no UTF-8 BOM (first bytes are `# A...`). On Windows PowerShell 5.1, this means the file is decoded as the system codepage (Windows-1252), so multi-byte UTF-8 characters like em-dash (`—`, `\xE2\x80\x94`) get split into garbage bytes that break later string parsing — the parser reports cryptic errors like "string is missing the terminator" on lines that look fine. ALWAYS stick to ASCII (`-`, `--`, straight quotes) in comments and string literals in `install.ps1`. To verify after editing: `[System.Management.Automation.Language.Parser]::ParseFile((Resolve-Path .\install.ps1).Path, [ref]$null, [ref]$errs)`. Same caveat applies to any unsigned `.ps1` file the project distributes. - **SLSA reusable workflow MUST be tag-pinned, not SHA-pinned** — `slsa-framework/slsa-github-generator/.github/workflows/*` looks like it should follow CI-001 (SHA pin), but the SLSA generator's `generate-builder.sh` reads the workflow ref to download the prebuilt builder from a GitHub release and rejects non-tag refs (`Invalid ref: ... Expected ref of the form refs/tags/vX.Y.Z`, exit 2). Pinning by SHA broke the v0.31.0 release ([run 25107195589](https://github.com/archgate/cli/actions/runs/25107195589)). The CI-001 rule allowlists this path so it does NOT block a SHA repin — meaning a future agent could "fix" the tag pin and the rule would be silent until the next release fails. ALWAYS keep `@v2.x.y` for `release-binaries.yml:165` and read the inline comment + CI-001 "Carved-out exceptions" before changing. Upstream issue: [slsa-framework/slsa-github-generator#150](https://github.com/slsa-framework/slsa-github-generator/issues/150). +- **Windows binary upgrade: never use detached child processes for `.old` cleanup** — On Windows, `replaceBinary()` renames the running exe to `.old` because the OS file-locks it. Cleaning up the `.old` via a detached `cmd /c ping -n 2 ... & del` process is unreliable (process may not spawn, `del` may fail silently, timing races). Instead, `cleanupStaleBinary()` runs at the next CLI startup as a fire-and-forget `unlink()` — the file is guaranteed unlocked by then. The cleanup is platform-agnostic (uses `getArtifactInfo()` to resolve the binary name), so it works on any supported platform even though only Windows currently creates `.old` files. The sync `unlinkSync` in `replaceBinary()` is kept as defense-in-depth for leftover `.old` files from previous upgrades. Do NOT reintroduce detached cleanup processes. - **`GITHUB_TOKEN`-authored pushes do NOT trigger downstream workflows — release.yml MUST use the GH App token** — When an Actions workflow pushes commits or opens PRs using `${{ github.token }}` / `secrets.GITHUB_TOKEN`, GitHub intentionally suppresses the resulting `push` / `pull_request` events to prevent recursion. Symptom on release PRs: the head SHA has no `pull_request`-event check runs, so `Validate Code` / `Lint, Test & Check` / `DCO Sign-off Check` are missing from the PR rollup and branch protection treats the PR as missing required checks. PR [#131](https://github.com/archgate/cli/pull/131) papered over this by manually `gh workflow run` + posting commit statuses, but `workflow_dispatch` runs land on `head_branch: release` with `pull_requests: []` — they are not associated with the PR ref, so `Lint, Test & Check` stayed orphaned and the bug recurred on PR [#251](https://github.com/archgate/cli/pull/251). Root-cause fix: in `release.yml` the `pull-request` job MUST generate a GitHub App installation token via `actions/create-github-app-token` (using `secrets.GH_APP_APP_ID` / `secrets.GH_APP_PRIVATE_KEY`) and pass it to BOTH `actions/checkout` and `simple-release-action`. App-token-authored pushes DO trigger `pull_request` events naturally. Apply the same pattern to any future workflow that pushes to a branch whose downstream CI must run. ## Validation Pipeline diff --git a/src/cli.ts b/src/cli.ts index d0a053de..1c85429b 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -14,6 +14,7 @@ import { registerReviewContextCommand } from "./commands/review-context"; import { registerSessionContextCommand } from "./commands/session-context/index"; import { registerTelemetryCommand } from "./commands/telemetry"; import { registerUpgradeCommand } from "./commands/upgrade"; +import { cleanupStaleBinary } from "./helpers/binary-upgrade"; import { beginCommand, exitWith, finalizeCommand } from "./helpers/exit"; import { installGit } from "./helpers/git"; import { type LogLevel, logError, setLogLevel } from "./helpers/log"; @@ -59,6 +60,10 @@ if (!isSupportedPlatform()) { createPathIfNotExists(paths.cacheFolder); +// Fire-and-forget: remove leftover .old binary from a previous Windows upgrade. +// No await — this must never block startup or affect the user's command. +cleanupStaleBinary(); + async function main() { await installGit(); diff --git a/src/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index 7d63a292..3fc70c2d 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -1,9 +1,11 @@ import { createHash } from "node:crypto"; import { chmodSync, mkdtempSync, renameSync, unlinkSync } from "node:fs"; +import { unlink } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { logDebug } from "./log"; +import { internalPath } from "./paths"; import { isWindows } from "./platform"; // --------------------------------------------------------------------------- @@ -213,8 +215,9 @@ export async function downloadReleaseBinary( * Replace the running binary with the new one. * * Unix: directly renames the new binary over the old one (OS handles inode unlinking). - * Windows: renames the running exe to .old (allowed by the OS), moves the new one - * into place, and spawns a detached cleanup process for the old file. + * Windows: renames the running exe to .old (allowed by the OS), then moves the + * new one into place. The .old file is cleaned up on the next CLI startup via + * {@link cleanupStaleBinary}. */ export function replaceBinary( currentPath: string, @@ -234,17 +237,46 @@ export function replaceBinary( renameSync(currentPath, oldPath); renameSync(newBinaryPath, currentPath); - // Spawn detached cleanup — waits for this process to exit, then deletes the old file - Bun.spawn(["cmd", "/c", `ping -n 2 127.0.0.1 >nul & del "${oldPath}"`], { - stdout: "ignore", - stderr: "ignore", - }); + // The .old file is still locked by the running process so it cannot be + // deleted right now. cleanupStaleBinary() will remove it on the next + // CLI invocation when the file is guaranteed to be unlocked. } else { renameSync(newBinaryPath, currentPath); chmodSync(currentPath, 0o755); } } +// --------------------------------------------------------------------------- +// Stale binary cleanup +// --------------------------------------------------------------------------- + +/** + * Attempt to delete the leftover `.old` binary from a previous upgrade. + * + * On Windows, `replaceBinary()` renames the running exe to `.old` because the + * OS file-locks the running binary. The `.old` file cannot be deleted during + * that same process — but it is guaranteed to be unlocked by the time the + * *next* CLI invocation starts. + * + * The cleanup is platform-agnostic: it resolves the correct binary name for + * the current platform and attempts to remove `.old` from the install + * directory. On Unix the `.old` file is unlikely to exist (rename is atomic), + * but running the check everywhere keeps the logic unified. + * + * Call this once at CLI startup (fire-and-forget, no `await`). Errors are + * silently swallowed — cleanup is best-effort and must never affect the + * user's command. + */ +export function cleanupStaleBinary(): Promise { + const artifact = getArtifactInfo(); + if (!artifact) return Promise.resolve(); + + const oldPath = internalPath("bin", `${artifact.binaryName}.old`); + return unlink(oldPath).catch(() => { + // File absent or still locked — nothing to do. + }); +} + /** * Returns the manual install hint for the current platform. */ diff --git a/tests/helpers/binary-upgrade.test.ts b/tests/helpers/binary-upgrade.test.ts index af0acead..8c541217 100644 --- a/tests/helpers/binary-upgrade.test.ts +++ b/tests/helpers/binary-upgrade.test.ts @@ -1,9 +1,16 @@ -import { describe, expect, test, mock, afterEach } from "bun:test"; -import { existsSync, mkdtempSync, statSync, writeFileSync } from "node:fs"; +import { describe, expect, test, mock, beforeEach, afterEach } from "bun:test"; +import { + existsSync, + mkdirSync, + mkdtempSync, + statSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { + cleanupStaleBinary, getArtifactInfo, getManualInstallHint, fetchLatestGitHubVersion, @@ -142,4 +149,62 @@ describe("replaceBinary", () => { const mode = statSync(currentPath).mode & 0o777; expect(mode).toBe(0o755); }); + + test("creates .old file on Windows", () => { + if (process.platform !== "win32") return; + + const tmpDir = mkdtempSync(join(tmpdir(), "archgate-replace-test-")); + const currentPath = join(tmpDir, "archgate.exe"); + const newBinaryPath = join(tmpDir, "archgate.exe.new"); + + writeFileSync(currentPath, "old binary content"); + writeFileSync(newBinaryPath, "new binary content"); + + replaceBinary(currentPath, newBinaryPath); + + expect(existsSync(currentPath)).toBe(true); + expect(existsSync(currentPath + ".old")).toBe(true); + expect(existsSync(newBinaryPath)).toBe(false); + }); +}); + +describe("cleanupStaleBinary", () => { + let savedHome: string | undefined; + + beforeEach(() => { + savedHome = Bun.env.HOME; + }); + + afterEach(() => { + Bun.env.HOME = savedHome; + }); + + test("deletes the .old binary when present", async () => { + const artifact = getArtifactInfo(); + if (!artifact) return; // unsupported platform + + const tmpDir = mkdtempSync(join(tmpdir(), "archgate-cleanup-test-")); + Bun.env.HOME = tmpDir; + + // Recreate the ~/.archgate/bin/ structure + const binDir = join(tmpDir, ".archgate", "bin"); + mkdirSync(binDir, { recursive: true }); + const oldPath = join(binDir, `${artifact.binaryName}.old`); + writeFileSync(oldPath, "stale binary"); + + await cleanupStaleBinary(); + + expect(existsSync(oldPath)).toBe(false); + }); + + test("resolves silently when no .old file exists", async () => { + const artifact = getArtifactInfo(); + if (!artifact) return; // unsupported platform + + const tmpDir = mkdtempSync(join(tmpdir(), "archgate-cleanup-test-")); + Bun.env.HOME = tmpDir; + + // No .old file — should not throw + await expect(cleanupStaleBinary()).resolves.toBeUndefined(); + }); });