From 8847da47ae23fd15496d009f5fbf9b1b65b33bf5 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 11:50:06 -0400 Subject: [PATCH 1/6] fix(env): keep stored ciphertext when a secret value is unchanged setProjectEnvValue now keeps an entry's existing ciphertext when it already decrypts to the requested value under the current key, so repeating `hack env set --secret --value-stdin` with the same value leaves the tracked env file byte-identical and git status clean. A different value, a plain entry becoming secret, or an entry that does not open under the current key is re-encrypted with a fresh IV. Env layer writes now commit by rename from an exclusive, fsynced sibling, so a write interrupted before the commit leaves the previous layer intact and resolvable. Existing permission bits and symlink targets are preserved. HACK-P08 moves from deferred to partial: per-key no-op detection and atomic layer writes exist; a multi-key hydration import remains missing. Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 9 + .../skills/hack-cli/references/patterns.md | 12 +- src/agents/pattern-catalogue.ts | 10 +- src/lib/fs.ts | 83 +++- src/lib/project-env-config.ts | 68 +++- tests/fixtures/project-env-set-worker.ts | 41 ++ tests/pattern-catalogue.test.ts | 2 +- tests/project-env-stable-ciphertext.test.ts | 369 ++++++++++++++++++ 8 files changed, 574 insertions(+), 20 deletions(-) create mode 100644 tests/fixtures/project-env-set-worker.ts create mode 100644 tests/project-env-stable-ciphertext.test.ts diff --git a/docs/env.md b/docs/env.md index 3452d237e..07e5d57d5 100644 --- a/docs/env.md +++ b/docs/env.md @@ -411,6 +411,15 @@ input and without writing anything: `.hack/hack.env..local.yaml`) instead of the shared repo env file — use it for per-checkout values you don't want to commit. +Repeating `hack env add` or `hack env set` with the value already stored is a no-op: when +the existing secret decrypts to the same value under the current key, its ciphertext is +kept, so the file stays byte-identical and Git shows no change. A different value, a +plain entry becoming secret, or an entry that does not decrypt under the current key gets +new ciphertext with a fresh IV. Each env file write goes to a temporary file in the same +directory and replaces the file by rename, so an interrupted write leaves the previous +file in place. Each command writes one key; a script that sets several keys is not atomic +across them. + If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the command applies documented defaults or fails fast with `E_INTERACTIVE_REQUIRED` instead of diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index b191f8189..e9b9fb51b 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -14,7 +14,7 @@ missing part with ad hoc scripts. | HACK-P05 | Branch-owned external routing claim | Deferred | WU29 | | HACK-P06 | Opt-in public tunnel with end-to-end readiness | Deferred | WU29 | | HACK-P07 | Laptop and headless-runner capability overlays | Partial | WU27/WU30 | -| HACK-P08 | Tracked defaults plus idempotent machine-local hydration | Deferred | WU27/WU14 | +| HACK-P08 | Tracked defaults plus idempotent machine-local hydration | Partial | WU27/WU14 | | HACK-P09 | Disposable run workspaces with reference-aware retention | Partial | WU12/WU30 | | HACK-P10 | Pinned tool image plus independent dependency caches | Partial | WU30 | | HACK-P11 | Stable agent guidance and typed operation results | Partial | WU19/WU21 | @@ -135,12 +135,12 @@ missing part with ad hoc scripts. ## HACK-P08: Tracked defaults plus idempotent machine-local hydration - Use when: A repository tracks env defaults and each machine adds local values on top. -- Disposition: Deferred. Missing: Hack has no semantic no-op detection or atomic machine-local import contract for env hydration. +- Disposition: Partial. Missing: No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: none yet; do not substitute a hand-written recipe. +- Commands: `hack env explain --json` - Prerequisites: Tracked defaults, a machine-local layer and preserved operator overrides. -- Env ownership: Defaults are tracked; hydration writes only the local layer and keeps values out of argv. -- Readiness: Repeat hydration leaves the tracked tree clean; an interrupted import still leaves a resolvable layer. +- Env ownership: Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv. +- Readiness: Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable. - Cleanup: Unchanged values cause no rewrite. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `091b4e80a9b6`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `85e011bccf62`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/pattern-catalogue.ts b/src/agents/pattern-catalogue.ts index 2aa0b783a..c9259d324 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -253,20 +253,20 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ title: "Tracked defaults plus idempotent machine-local hydration", useWhen: "A repository tracks env defaults and each machine adds local values on top.", - disposition: "deferred", - supportedCommands: [], + disposition: "partial", + supportedCommands: ["hack env explain --json"], prerequisites: "Tracked defaults, a machine-local layer and preserved operator overrides.", envOwnership: - "Defaults are tracked; hydration writes only the local layer and keeps values out of argv.", + "Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv.", readiness: - "Repeat hydration leaves the tracked tree clean; an interrupted import still leaves a resolvable layer.", + "Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable.", cleanup: "Unchanged values cause no rewrite.", negativeControl: "Re-encrypting unchanged entries causes diff and stash churn, and values passed in command arguments are not a secure import mechanism.", domainOwner: "WU27/WU14", missingContract: - "Hack has no semantic no-op detection or atomic machine-local import contract for env hydration.", + "No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not.", provenance: { consumers: 1, source: "HACK-P08 research register" }, example: null, }, diff --git a/src/lib/fs.ts b/src/lib/fs.ts index f289e353a..706ea6229 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -1,4 +1,16 @@ -import { lstat, mkdir, stat } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { + constants, + type FileHandle, + lstat, + mkdir, + open, + realpath, + rename, + stat, + unlink, +} from "node:fs/promises"; +import { basename, dirname, join } from "node:path"; export async function pathExists(absolutePath: string): Promise { try { @@ -89,6 +101,75 @@ export async function writeTextFileIfChanged( return { changed: true }; } +/** + * Replaces `absolutePath` with `content` only when the text differs, publishing + * the new contents atomically. + * + * Commit point: a uniquely named sibling is created exclusively (no symlink + * follow), written and fsynced, then renamed over the target. A process that + * dies before the rename leaves the previous complete file in place and at most + * an orphaned `...tmp` sibling that no reader selects; the staging + * file is removed on every failure this process observes. Directory fsync is + * best effort. + * + * An existing target keeps its permission bits, and an existing symlink keeps + * pointing where it did: the replacement lands at the link's resolved path, as + * a plain write through the link would. A new file gets the umask default. + */ +export async function writeTextFileAtomicIfChanged( + absolutePath: string, + content: string +): Promise<{ readonly changed: boolean }> { + const existing = await readTextFile(absolutePath); + if (existing === content) { + return { changed: false }; + } + const target = + existing === null ? absolutePath : await realpath(absolutePath); + const mode = existing === null ? null : (await stat(target)).mode & 0o7777; + const staging = join( + dirname(target), + `.${basename(target)}.${randomUUID()}.tmp` + ); + const file = await open( + staging, + constants.O_CREAT | + constants.O_EXCL | + constants.O_NOFOLLOW | + constants.O_WRONLY, + 0o666 + ); + let committed = false; + try { + if (mode !== null) { + await file.chmod(mode); + } + await file.writeFile(content, "utf8"); + await file.sync(); + await rename(staging, target); + committed = true; + } finally { + await file.close(); + if (!committed) { + await unlink(staging).catch(() => undefined); + } + } + await syncDirectoryBestEffort(dirname(target)); + return { changed: true }; +} + +async function syncDirectoryBestEffort(directory: string): Promise { + let handle: FileHandle | null = null; + try { + handle = await open(directory, constants.O_RDONLY); + await handle.sync(); + } catch { + // Some filesystems refuse directory fsync; the file itself is synced. + } finally { + await handle?.close().catch(() => undefined); + } +} + /** * Ensures a marker-delimited managed block exists in a gitignore-style file. * diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 3a18af37c..2f533b849 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -33,6 +33,7 @@ import { pathExists, readTextFile, writeTextFile, + writeTextFileAtomicIfChanged, writeTextFileIfChanged, } from "./fs.ts"; import { @@ -2259,6 +2260,59 @@ function decryptProjectEnvStoredValue(opts: { }); } +/** + * Secret value to store for `plaintext`, keeping the entry's current ciphertext + * when it already opens to exactly `plaintext` under `keyText`. + * + * Fresh-IV encryption makes every write differ, so re-encrypting an unchanged + * value would churn a tracked file on each repeat. Reuse is semantic, not + * textual: an absent, plain, removed, malformed, wrong-key or different entry + * gets a new ciphertext with a fresh IV. Decryption failures are not reported; + * they only select rotation. + */ +function stableProjectEnvSecretValue(opts: { + readonly previous: ProjectEnvStoredValue | undefined; + readonly plaintext: string; + readonly keyText: string; +}): ProjectEnvSecretValue { + const previous = opts.previous; + if ( + previous !== undefined && + previous !== null && + isProjectEnvSecretValue(previous) && + opensToPlaintext({ + ciphertext: previous.secure, + plaintext: opts.plaintext, + keyText: opts.keyText, + }) + ) { + return { secure: previous.secure }; + } + return { + secure: encryptProjectEnvValue({ + plaintext: opts.plaintext, + keyText: opts.keyText, + }), + }; +} + +function opensToPlaintext(opts: { + readonly ciphertext: string; + readonly plaintext: string; + readonly keyText: string; +}): boolean { + try { + return ( + decryptProjectEnvValue({ + ciphertext: opts.ciphertext, + keyText: opts.keyText, + }) === opts.plaintext + ); + } catch { + return false; + } +} + export async function setProjectEnvValue(opts: { readonly projectRoot: string; readonly projectDir: string; @@ -2311,12 +2365,11 @@ export async function setProjectEnvValue(opts: { projectRoot: opts.projectRoot, }); createdKey = ensuredKey.created; - storedValue = { - secure: encryptProjectEnvValue({ - plaintext: opts.value, - keyText: ensuredKey.keyText, - }), - }; + storedValue = stableProjectEnvSecretValue({ + previous: read.config.values[scope]?.[opts.key], + plaintext: opts.value, + keyText: ensuredKey.keyText, + }); } if (opts.local === true) { await ensureHackDirGitignore({ @@ -2424,7 +2477,8 @@ async function writeProjectEnvConfigFile(opts: { const yaml = YAML.stringify(opts.config, null, 2); const text = yaml.endsWith("\n") ? yaml : `${yaml}\n`; await assertLegacyProjectDirectory({ projectDir: dirname(opts.path) }); - return (await writeTextFileIfChanged(opts.path, text)).changed; + // Rename-committed: an interrupted mutation leaves the previous layer intact. + return (await writeTextFileAtomicIfChanged(opts.path, text)).changed; } export async function materializeProjectEnv(opts: { diff --git a/tests/fixtures/project-env-set-worker.ts b/tests/fixtures/project-env-set-worker.ts new file mode 100644 index 000000000..a77f0ee6a --- /dev/null +++ b/tests/fixtures/project-env-set-worker.ts @@ -0,0 +1,41 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `setProjectEnvValue` secret write in a disposable child and pauses + * it at the commit rename. Everything before that boundary (reading the layer, + * encrypting, creating, writing and fsyncing the staging file) runs unmocked; + * the parent then SIGKILLs the child to test a real pre-commit interruption. + * Values are synthetic test strings passed on stdin, never in argv. + */ +const [projectRoot, projectDir] = process.argv.slice(2); +if (!(projectRoot && projectDir)) { + throw new Error("Missing project env worker arguments"); +} + +const rename = fs.rename; +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async (from: string, to: string) => { + process.stdout.write("prepared\n"); + // Hold the staged write open until the parent kills this process. + await Bun.sleep(120_000); + return await rename(from, to); + }, +})); + +const value = await Bun.stdin.text(); + +const { setProjectEnvValue } = await import( + "../../src/lib/project-env-config.ts" +); +await setProjectEnvValue({ + projectRoot, + projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value, + secret: true, +}); +process.stdout.write("committed\n"); diff --git a/tests/pattern-catalogue.test.ts b/tests/pattern-catalogue.test.ts index 4aca9e395..a836df669 100644 --- a/tests/pattern-catalogue.test.ts +++ b/tests/pattern-catalogue.test.ts @@ -68,7 +68,7 @@ test("starting dispositions match the recorded decisions", () => { "HACK-P05": "deferred", "HACK-P06": "deferred", "HACK-P07": "partial", - "HACK-P08": "deferred", + "HACK-P08": "partial", "HACK-P09": "partial", "HACK-P10": "partial", "HACK-P11": "partial", diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts new file mode 100644 index 000000000..c6ba1ffb6 --- /dev/null +++ b/tests/project-env-stable-ciphertext.test.ts @@ -0,0 +1,369 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { YAML } from "bun"; + +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + PROJECT_ENV_KEY_FILENAME, +} from "../src/constants.ts"; +import { + resolveProjectEnvConfig, + resolveProjectEnvConfigPath, + setProjectEnvValue, +} from "../src/lib/project-env-config.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1152 (pattern HACK-P08): repeating an unchanged secret write keeps the + * stored ciphertext, a changed value rotates it with a fresh IV, and an + * interrupted write leaves the previous layer resolvable. Every value here is a + * synthetic test string; every child gets a private HOME. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const WORKER = resolve(import.meta.dir, "fixtures/project-env-set-worker.ts"); +const FIRST = "synthetic-first-6f1c"; +const SECOND = "synthetic-second-92ad"; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const tempDir of tempDirs) { + await rm(tempDir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +type Repo = { + readonly root: string; + readonly projectRoot: string; + readonly projectDir: string; + readonly envFile: string; +}; + +async function createRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-stable-")); + tempDirs.add(root); + const projectRoot = resolve(root, "repo"); + const projectDir = resolve(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + resolve(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + resolve(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: "env-stable", dev_host: "env-stable.hack" }, null, 2)}\n` + ); + await writeFile( + resolve(projectRoot, ".gitignore"), + `${PROJECT_ENV_KEY_FILENAME}\n` + ); + return { + root, + projectRoot, + projectDir, + envFile: resolveProjectEnvConfigPath({ projectDir, envName: null }), + }; +} + +async function setSecret(repo: Repo, value: string) { + return await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value, + secret: true, + }); +} + +async function resolvedToken(repo: Repo): Promise { + const resolved = await resolveProjectEnvConfig({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + serviceNames: ["api"], + }); + return resolved?.globalEnv.API_TOKEN; +} + +function storedCiphertext(text: string): string { + const parsed: unknown = YAML.parse(text); + const secure = ( + parsed as { values?: { global?: { API_TOKEN?: { secure?: unknown } } } } + ).values?.global?.API_TOKEN?.secure; + if (typeof secure !== "string") { + throw new Error("Expected a stored secure value"); + } + return secure; +} + +/** v1 values are `v1:::`. */ +function ivSegment(ciphertext: string): string { + return ciphertext.split(":")[1] ?? ""; +} + +async function git(repo: Repo, args: readonly string[]): Promise { + const proc = Bun.spawn(["git", ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null" }, + }), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + if (exitCode !== 0) { + throw new Error(`git ${args[0] ?? ""} failed`); + } + return stdout; +} + +async function commitAll(repo: Repo): Promise { + await git(repo, ["add", "-A"]); + await git(repo, [ + "-c", + "user.name=Hack Test", + "-c", + "user.email=hack@example.com", + "commit", + "-q", + "-m", + "fixture", + ]); +} + +test("repeating an unchanged secret keeps the tracked file byte-identical", async () => { + const repo = await createRepo(); + await git(repo, ["init", "-q", "-b", "main"]); + const first = await setSecret(repo, FIRST); + expect(first.changed).toBe(true); + expect(first.createdKey).toBe(true); + await commitAll(repo); + const committed = await readFile(repo.envFile); + + for (let attempt = 0; attempt < 3; attempt += 1) { + const repeat = await setSecret(repo, FIRST); + expect(repeat.changed).toBe(false); + expect(repeat.createdKey).toBe(false); + } + expect((await readFile(repo.envFile)).equals(committed)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("a changed secret rotates ciphertext with a fresh IV and resolves", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const before = storedCiphertext(await readFile(repo.envFile, "utf8")); + + const changed = await setSecret(repo, SECOND); + expect(changed.changed).toBe(true); + const after = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(after).not.toBe(before); + expect(ivSegment(after)).not.toBe(ivSegment(before)); + expect(await resolvedToken(repo)).toBe(SECOND); + + // Returning to an earlier value is a change, not a reuse of old ciphertext. + await setSecret(repo, FIRST); + const reverted = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(reverted).not.toBe(before); + expect(reverted).not.toBe(after); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("an entry that no longer opens under the current key is re-encrypted", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const sealedUnderOldKey = storedCiphertext( + await readFile(repo.envFile, "utf8") + ); + // Synthetic replacement key: the stored entry cannot open under it. + await writeFile( + resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME), + "synthetic-replacement-key-material-0123456789\n" + ); + + const result = await setSecret(repo, FIRST); + expect(result.changed).toBe(true); + const rotated = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(rotated).not.toBe(sealedUnderOldKey); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("plain and secret forms of the same value are never treated as unchanged", async () => { + const repo = await createRepo(); + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value: FIRST, + secret: false, + }); + const upgraded = await setSecret(repo, FIRST); + expect(upgraded.changed).toBe(true); + const text = await readFile(repo.envFile, "utf8"); + expect(text).not.toContain(FIRST); + storedCiphertext(text); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("a rewrite keeps the layer's permission bits", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + await chmod(repo.envFile, 0o640); + await setSecret(repo, SECOND); + expect((await stat(repo.envFile)).mode & 0o777).toBe(0o640); +}); + +async function readUntil( + stream: ReadableStream, + marker: string +): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let seen = ""; + try { + while (!seen.includes(marker)) { + const chunk = await reader.read(); + if (chunk.done) { + throw new Error(`Worker exited before ${marker.trim()}`); + } + seen += decoder.decode(chunk.value, { stream: true }); + } + } finally { + reader.releaseLock(); + } +} + +test("a write killed before its commit rename leaves the previous layer resolvable", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const original = await readFile(repo.envFile); + + const worker = Bun.spawn( + [process.execPath, WORKER, repo.projectRoot, repo.projectDir], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write(SECOND); + await worker.stdin.end(); + await readUntil(worker.stdout, "prepared\n"); + // The replacement is fully staged and fsynced, but not yet committed. + expect((await readFile(repo.envFile)).equals(original)).toBe(true); + worker.kill("SIGKILL"); + await worker.exited; + + const abandoned = (await readdir(repo.projectDir)).filter((name) => + name.endsWith(".tmp") + ); + expect(abandoned).toHaveLength(1); + const staged = await readFile( + join(repo.projectDir, abandoned[0] ?? ""), + "utf8" + ); + expect(storedCiphertext(staged)).not.toBe( + storedCiphertext(original.toString("utf8")) + ); + + // The old layer is intact and still resolves; the orphan is not a layer. + expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect(await resolvedToken(repo)).toBe(FIRST); + + // The next writer commits normally without adopting the orphan. + const retried = await setSecret(repo, SECOND); + expect(retried.changed).toBe(true); + expect(await resolvedToken(repo)).toBe(SECOND); + expect(storedCiphertext(await readFile(repo.envFile, "utf8"))).not.toBe( + storedCiphertext(staged) + ); +}, 30_000); + +async function runCli( + repo: Repo, + args: readonly string[], + stdin: string +): Promise { + const proc = Bun.spawn([process.execPath, ENTRYPOINT, ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { + HACK_GLOBAL_CONFIG_PATH: join(repo.root, "hack-config.json"), + HACK_LOGGER: "console", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + proc.stdin.write(stdin); + await proc.stdin.end(); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + expect(stdout).not.toContain(FIRST); + expect(stderr).not.toContain(FIRST); + return exitCode; +} + +test("repeat `env set --secret --value-stdin` leaves git status clean", async () => { + const repo = await createRepo(); + await writeFile( + repo.envFile, + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await git(repo, ["init", "-q", "-b", "main"]); + const setArgs = [ + "env", + "set", + "API_TOKEN", + "--secret", + "--value-stdin", + "--path", + repo.projectRoot, + ]; + expect(await runCli(repo, setArgs, `${FIRST}\n`)).toBe(0); + await commitAll(repo); + const committed = await readFile(repo.envFile); + + expect(await runCli(repo, setArgs, `${FIRST}\n`)).toBe(0); + expect((await readFile(repo.envFile)).equals(committed)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + + // Positive control: a changed value does reach the tracked file. + expect(await runCli(repo, setArgs, `${SECOND}\n`)).toBe(0); + expect(await git(repo, ["status", "--porcelain"])).toBe( + " M .hack/hack.env.default.yaml\n" + ); +}, 60_000); From 77c599ac3ed09f64e67239c99cf19477183b7772 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:05:10 -0400 Subject: [PATCH 2/6] fix(env): ignore orphaned env staging files and pin atomic write behavior A `--local` env write killed before its commit rename left a `.hack.env*..tmp` staging file that git did not ignore, so a later `git add -A` could commit machine-local values, and a tracked-layer orphan left the tree dirty. The managed `.hack/.gitignore` now ignores `.hack.env*.tmp`, and `hack doctor` flags a tracked staging file. Tests now cover symlink preservation, staging cleanup after a failed rename, and clean git status after a killed tracked or local write. The argv-env-value recipe skips flags, so the P08 card can name `hack env set --local --value-stdin --secret`. Docs describe the first-repeat normalization of hand-edited files and the rename edge cases. Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 25 +++-- .../skills/hack-cli/references/patterns.md | 6 +- src/agents/pattern-catalogue.ts | 7 +- src/lib/doctor-generated-files.ts | 1 + src/lib/fs.ts | 8 +- src/templates.ts | 5 + tests/doctor-generated-files.test.ts | 11 ++- tests/fixtures/project-env-set-worker.ts | 10 +- tests/hack-gitignore.test.ts | 2 + tests/helpers/pattern-recipes.ts | 18 +++- tests/pattern-catalogue.test.ts | 20 ++++ tests/project-env-stable-ciphertext.test.ts | 92 +++++++++++++++++-- 12 files changed, 177 insertions(+), 28 deletions(-) diff --git a/docs/env.md b/docs/env.md index 07e5d57d5..d3263b015 100644 --- a/docs/env.md +++ b/docs/env.md @@ -51,6 +51,7 @@ worktrees inherit the rules with zero setup. It covers (patterns relative to - `.env.state.json` - `hack.env.local.yaml` - `hack.env.*.local.yaml` +- `.hack.env*.tmp` (staging files an interrupted env write can leave behind) - `tickets/` (retained only to contain retired machine-local ticket caches during upgrades) How it is maintained: @@ -413,12 +414,24 @@ per-checkout values you don't want to commit. Repeating `hack env add` or `hack env set` with the value already stored is a no-op: when the existing secret decrypts to the same value under the current key, its ciphertext is -kept, so the file stays byte-identical and Git shows no change. A different value, a -plain entry becoming secret, or an entry that does not decrypt under the current key gets -new ciphertext with a fresh IV. Each env file write goes to a temporary file in the same -directory and replaces the file by rename, so an interrupted write leaves the previous -file in place. Each command writes one key; a script that sets several keys is not atomic -across them. +kept, so once Hack has written the file it stays byte-identical and Git shows no change. +A hand-edited file (comments, other formatting, or no `global: {}`) is normalized on the +first repeat and stays byte-identical after that. A different value, a plain entry +becoming secret, or an entry that does not decrypt under the current key gets new +ciphertext with a fresh IV. Each command writes one key; a script that sets several keys +is not atomic across them. + +Each env file write goes to a hidden `...tmp` file in the same directory and +replaces the file by rename, so an interrupted write leaves the previous file in place. +A write killed before the rename can leave that staging file behind. It holds the whole +new layer, so `.hack/.gitignore` ignores `.hack.env*.tmp`, and you can delete it. Hack +does not remove it on the next write, because another write may still be using it. The +rename has these effects: + +- a symlinked env file keeps its link; the new contents land at the file the link points + to, and the staging file is created next to that file +- a symlink whose target does not exist is replaced by a regular file +- the directory must be writable, even when the env file itself is writable If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index e9b9fb51b..bdc5406d0 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -137,9 +137,9 @@ missing part with ad hoc scripts. - Use when: A repository tracks env defaults and each machine adds local values on top. - Disposition: Partial. Missing: No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: `hack env explain --json` +- Commands: `hack env set --local --value-stdin --secret`, `hack env explain --json` - Prerequisites: Tracked defaults, a machine-local layer and preserved operator overrides. -- Env ownership: Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv. +- Env ownership: Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default. - Readiness: Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable. - Cleanup: Unchanged values cause no rewrite. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `85e011bccf62`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `27bff041d416`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/pattern-catalogue.ts b/src/agents/pattern-catalogue.ts index c9259d324..789138ce4 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -254,11 +254,14 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ useWhen: "A repository tracks env defaults and each machine adds local values on top.", disposition: "partial", - supportedCommands: ["hack env explain --json"], + supportedCommands: [ + "hack env set --local --value-stdin --secret", + "hack env explain --json", + ], prerequisites: "Tracked defaults, a machine-local layer and preserved operator overrides.", envOwnership: - "Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv.", + "Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default.", readiness: "Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable.", cleanup: "Unchanged values cause no rewrite.", diff --git a/src/lib/doctor-generated-files.ts b/src/lib/doctor-generated-files.ts index 0728c784d..e1eb7fbe0 100644 --- a/src/lib/doctor-generated-files.ts +++ b/src/lib/doctor-generated-files.ts @@ -28,6 +28,7 @@ export function buildGeneratedFilePathspecs(opts: { `${dir}/.env.state.json`, `${dir}/tickets`, `${dir}/hack.env.*.local.yaml`, + `${dir}/.hack.env*.tmp`, PROJECT_ENV_KEY_FILENAME, ]; } diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 706ea6229..96558b275 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -109,12 +109,16 @@ export async function writeTextFileIfChanged( * follow), written and fsynced, then renamed over the target. A process that * dies before the rename leaves the previous complete file in place and at most * an orphaned `...tmp` sibling that no reader selects; the staging - * file is removed on every failure this process observes. Directory fsync is - * best effort. + * file is removed on every failure this process observes. Orphans are never + * swept here, because a concurrent writer may still own one; callers that write + * under `.hack/` rely on its managed `.gitignore` to keep them unstaged. + * Directory fsync is best effort. * * An existing target keeps its permission bits, and an existing symlink keeps * pointing where it did: the replacement lands at the link's resolved path, as * a plain write through the link would. A new file gets the umask default. + * Unlike an in-place write, a dangling symlink is replaced by a regular file, + * and the containing directory must be writable. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, diff --git a/src/templates.ts b/src/templates.ts index a7b665d2b..0c7b447be 100644 --- a/src/templates.ts +++ b/src/templates.ts @@ -331,6 +331,10 @@ export const HACK_DIR_GITIGNORE_END_MARKER = "# end managed by hack" as const; * the `.hack/` directory. Root-level `.hack.secret.key` stays in the root * `.gitignore` (see `ensureProjectEnvSecretKey`). The retired `tickets/` * entry remains upgrade-safe so existing machine-local caches cannot be staged. + * `.hack.env*.tmp` covers the `...tmp` staging sibling that + * `writeTextFileAtomicIfChanged` leaves behind when an env layer write is + * killed before its commit rename; it holds a whole layer, including + * machine-local values, so it must never be staged. */ export const HACK_DIR_GITIGNORE_ENTRIES = [ ".internal/", @@ -339,6 +343,7 @@ export const HACK_DIR_GITIGNORE_ENTRIES = [ ".env.state.json", "hack.env.local.yaml", "hack.env.*.local.yaml", + ".hack.env*.tmp", "tickets/", ] as const; diff --git a/tests/doctor-generated-files.test.ts b/tests/doctor-generated-files.test.ts index 2f130e436..337706f5b 100644 --- a/tests/doctor-generated-files.test.ts +++ b/tests/doctor-generated-files.test.ts @@ -39,6 +39,8 @@ async function runGit(args: readonly string[], cwd: string): Promise { return stdout.trim(); } +const STAGING_ORPHAN = ".hack.env.local.yaml.0b88c1d2.tmp"; + /** * Creates a git repo where machine-local generated files were force-added and * committed — the leak scenario the doctor check exists to catch. @@ -69,6 +71,11 @@ async function createLeakedRepo(): Promise { resolve(repoRoot, ".hack", "tickets", "legacy-cache.json"), "{}\n" ); + // Staging file left by an env layer write killed before its commit rename. + await writeFile( + resolve(repoRoot, ".hack", STAGING_ORPHAN), + "version: 1\nvalues:\n global: {}\n" + ); await writeFile( resolve(repoRoot, PROJECT_ENV_KEY_FILENAME), "super-secret-key\n" @@ -90,6 +97,7 @@ test("inspectTrackedGeneratedFiles lists tracked generated files and flags the s PROJECT_ENV_KEY_FILENAME, ".hack/.branch/compose.x.override.yml", ".hack/.env.state.json", + `.hack/${STAGING_ORPHAN}`, ".hack/tickets/legacy-cache.json", ]); expect(inspection?.secretKeyTracked).toBe(true); @@ -138,7 +146,7 @@ test("untrackGeneratedFiles removes offenders from the index, keeps files on dis projectRoot: repoRoot, projectDirName: ".hack", }); - expect(inspection?.trackedPaths.length).toBe(4); + expect(inspection?.trackedPaths.length).toBe(5); // Same flow as `hack doctor --fix`: untrack, then ensure the nested ignore. const untracked = await untrackGeneratedFiles({ @@ -152,6 +160,7 @@ test("untrackGeneratedFiles removes offenders from the index, keeps files on dis for (const path of [ ".hack/.branch/compose.x.override.yml", ".hack/.env.state.json", + `.hack/${STAGING_ORPHAN}`, ".hack/tickets/legacy-cache.json", PROJECT_ENV_KEY_FILENAME, ]) { diff --git a/tests/fixtures/project-env-set-worker.ts b/tests/fixtures/project-env-set-worker.ts index a77f0ee6a..154a3393e 100644 --- a/tests/fixtures/project-env-set-worker.ts +++ b/tests/fixtures/project-env-set-worker.ts @@ -6,10 +6,13 @@ import * as fs from "node:fs/promises"; * it at the commit rename. Everything before that boundary (reading the layer, * encrypting, creating, writing and fsyncing the staging file) runs unmocked; * the parent then SIGKILLs the child to test a real pre-commit interruption. - * Values are synthetic test strings passed on stdin, never in argv. + * Values are synthetic test strings passed on stdin, never in argv. The optional + * third argument, `tracked` (default) or `local`, selects the layer written. */ -const [projectRoot, projectDir] = process.argv.slice(2); -if (!(projectRoot && projectDir)) { +const [projectRoot, projectDir, layer = "tracked"] = process.argv.slice(2); +if ( + !(projectRoot && projectDir && (layer === "tracked" || layer === "local")) +) { throw new Error("Missing project env worker arguments"); } @@ -37,5 +40,6 @@ await setProjectEnvValue({ key: "API_TOKEN", value, secret: true, + local: layer === "local", }); process.stdout.write("committed\n"); diff --git a/tests/hack-gitignore.test.ts b/tests/hack-gitignore.test.ts index 92663caa4..3a017d063 100644 --- a/tests/hack-gitignore.test.ts +++ b/tests/hack-gitignore.test.ts @@ -186,6 +186,8 @@ test("nested .hack/.gitignore makes git ignore generated files, including in lin ".hack/.env.state.json", ".hack/hack.env.local.yaml", ".hack/hack.env.qa.local.yaml", + ".hack/.hack.env.local.yaml.0b88c1d2.tmp", + ".hack/.hack.env.default.yaml.0b88c1d2.tmp", ".hack/tickets/state/projection.sqlite", ]) { expect( diff --git a/tests/helpers/pattern-recipes.ts b/tests/helpers/pattern-recipes.ts index fe447a022..4a71ef854 100644 --- a/tests/helpers/pattern-recipes.ts +++ b/tests/helpers/pattern-recipes.ts @@ -8,6 +8,22 @@ import { * Findings are closed codes; the scanned text is never echoed. */ +// One shell word on the same line. A backtick ends a word, because it closes +// an inline code span; a whole backtick command substitution is one operand. +const WORD = "[^\\s`]*"; +const FLAG = `[ \\t]+-${WORD}`; +const OPERAND = `[ \\t]+(?:\`[^\`\\n]*\`|[^\\s\`-]${WORD})`; + +/** + * `hack env set|add` with two operands, so a key and a value in argv. Flags + * between them are skipped, which keeps `hack env set --value-stdin` + * allowed. A flag that takes a value (`--env dev`) leaves that value as an + * operand, so such a form is still flagged. + */ +const ARGV_ENV_VALUE = new RegExp( + `\\bhack[ \\t]+env[ \\t]+(?:set|add)(?:${FLAG})*${OPERAND}(?:${FLAG})*${OPERAND}` +); + /** Recipes that must never appear outside a negative-example block. */ export const BANNED_RECIPES: readonly (readonly [string, RegExp])[] = [ ["broad-process-kill", /\b(?:pkill|killall)\b/], @@ -22,7 +38,7 @@ export const BANNED_RECIPES: readonly (readonly [string, RegExp])[] = [ "argv-secret-assignment", /\b[A-Z][A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|API_KEY)=\S/, ], - ["argv-env-value", /\bhack\s+env\s+(?:set|add)\s+\S+\s+\S+/], + ["argv-env-value", ARGV_ENV_VALUE], ]; function stripNegativeExamples(text: string): string { diff --git a/tests/pattern-catalogue.test.ts b/tests/pattern-catalogue.test.ts index a836df669..d4bb37be1 100644 --- a/tests/pattern-catalogue.test.ts +++ b/tests/pattern-catalogue.test.ts @@ -161,6 +161,26 @@ test("negative control: the banned-recipe scan flags each recipe outside a negat } }); +test("the argv-env-value scan skips flags but still flags a value operand", () => { + for (const text of [ + "hack env set API_TOKEN --local --value-stdin --secret", + "`hack env set --value-stdin` (pipe the value)", + "run `hack env set API_TOKEN` and pipe the value", + "hack env set API_TOKEN\nsecond line", + ]) { + expect(bannedRecipeFindings(text), text).toEqual([]); + } + for (const text of [ + "hack env set --local API_TOKEN synthetic-value", + "hack env add API_TOKEN --local synthetic-value", + "`hack env set API_TOKEN synthetic-value`", + "hack env set API_TOKEN `cat synthetic-file`", + "hack env set API_TOKEN --env dev", + ]) { + expect(bannedRecipeFindings(text), text).toEqual(["argv-env-value"]); + } +}); + test("the packaged hack-cli reference is the rendered catalogue", async () => { const packaged = await Bun.file( join( diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index c6ba1ffb6..8a0b69d31 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -1,12 +1,14 @@ import { afterEach, expect, test } from "bun:test"; import { chmod, + lstat, mkdir, mkdtemp, readdir, readFile, rm, stat, + symlink, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -18,9 +20,12 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; +import { writeTextFileAtomicIfChanged } from "../src/lib/fs.ts"; import { + ensureHackDirGitignore, resolveProjectEnvConfig, resolveProjectEnvConfigPath, + resolveProjectEnvLocalConfigPath, setProjectEnvValue, } from "../src/lib/project-env-config.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -80,7 +85,18 @@ async function createRepo(): Promise { }; } -async function setSecret(repo: Repo, value: string) { +type Layer = "tracked" | "local"; + +function layerPath(repo: Repo, layer: Layer): string { + return layer === "local" + ? resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: null, + }) + : repo.envFile; +} + +async function setSecret(repo: Repo, value: string, layer: Layer = "tracked") { return await setProjectEnvValue({ projectRoot: repo.projectRoot, projectDir: repo.projectDir, @@ -89,6 +105,7 @@ async function setSecret(repo: Repo, value: string) { key: "API_TOKEN", value, secret: true, + local: layer === "local", }); } @@ -257,13 +274,63 @@ async function readUntil( } } -test("a write killed before its commit rename leaves the previous layer resolvable", async () => { +test("a symlinked layer keeps its link and the write lands at the link target", async () => { const repo = await createRepo(); - await setSecret(repo, FIRST); - const original = await readFile(repo.envFile); + const shared = resolve(repo.root, "shared"); + await mkdir(shared); + const linkTarget = resolve(shared, "hack.env.default.yaml"); + await writeFile( + linkTarget, + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await symlink(linkTarget, repo.envFile); + + expect((await setSecret(repo, FIRST)).changed).toBe(true); + expect((await lstat(repo.envFile)).isSymbolicLink()).toBe(true); + storedCiphertext(await readFile(linkTarget, "utf8")); + expect(await resolvedToken(repo)).toBe(FIRST); + // Neither directory keeps a staging file after a committed write. + for (const dir of [repo.projectDir, shared]) { + expect( + (await readdir(dir)).filter((name) => name.endsWith(".tmp")) + ).toEqual([]); + } +}); + +test("a write that fails after staging removes its staging file", async () => { + const repo = await createRepo(); + // A directory at the target path: staging succeeds, the commit rename fails. + const target = resolve(repo.projectDir, "occupied.yaml"); + await mkdir(target); + + await expect( + writeTextFileAtomicIfChanged(target, "synthetic: content\n") + ).rejects.toThrow(); + expect((await stat(target)).isDirectory()).toBe(true); + expect( + (await readdir(repo.projectDir)).filter((name) => name.endsWith(".tmp")) + ).toEqual([]); +}); + +for (const layer of ["tracked", "local"] as const) { + test(`a ${layer} write killed before its commit rename leaves the layer resolvable and git clean`, async () => { + await assertKilledWriteIsRecoverable(layer); + }, 30_000); +} + +async function assertKilledWriteIsRecoverable(layer: Layer): Promise { + const repo = await createRepo(); + const layerFile = layerPath(repo, layer); + await git(repo, ["init", "-q", "-b", "main"]); + // `hack init` writes the managed `.hack/.gitignore`; `--local` writes heal it. + await ensureHackDirGitignore({ projectDir: repo.projectDir }); + await setSecret(repo, FIRST, layer); + await commitAll(repo); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + const original = await readFile(layerFile); const worker = Bun.spawn( - [process.execPath, WORKER, repo.projectRoot, repo.projectDir], + [process.execPath, WORKER, repo.projectRoot, repo.projectDir, layer], { cwd: repo.projectRoot, env: privateCliEnv({ root: repo.root }), @@ -276,7 +343,7 @@ test("a write killed before its commit rename leaves the previous layer resolvab await worker.stdin.end(); await readUntil(worker.stdout, "prepared\n"); // The replacement is fully staged and fsynced, but not yet committed. - expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect((await readFile(layerFile)).equals(original)).toBe(true); worker.kill("SIGKILL"); await worker.exited; @@ -293,17 +360,22 @@ test("a write killed before its commit rename leaves the previous layer resolvab ); // The old layer is intact and still resolves; the orphan is not a layer. - expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect((await readFile(layerFile)).equals(original)).toBe(true); expect(await resolvedToken(repo)).toBe(FIRST); + // The orphan holds the whole layer, so it must be ignored: `git add -A` + // would otherwise commit machine-local values, and the tree would be dirty. + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + await git(repo, ["add", "-A"]); + expect(await git(repo, ["diff", "--cached", "--name-only"])).toBe(""); // The next writer commits normally without adopting the orphan. - const retried = await setSecret(repo, SECOND); + const retried = await setSecret(repo, SECOND, layer); expect(retried.changed).toBe(true); expect(await resolvedToken(repo)).toBe(SECOND); - expect(storedCiphertext(await readFile(repo.envFile, "utf8"))).not.toBe( + expect(storedCiphertext(await readFile(layerFile, "utf8"))).not.toBe( storedCiphertext(staged) ); -}, 30_000); +} async function runCli( repo: Repo, From ad65a7ea2dd0682c2709f0168c5e57c1b6a808ca Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:09:41 -0400 Subject: [PATCH 3/6] chore(hack): refresh the managed .hack/.gitignore block Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- .hack/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.hack/.gitignore b/.hack/.gitignore index d6aeb28b5..1dcac9688 100644 --- a/.hack/.gitignore +++ b/.hack/.gitignore @@ -5,5 +5,6 @@ .env.state.json hack.env.local.yaml hack.env.*.local.yaml +.hack.env*.tmp tickets/ # end managed by hack From 52f0ffc9b43f883eef7e4fd54234ea8f9660e43a Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:54:21 -0400 Subject: [PATCH 4/6] test(env): encrypt ciphertext-only fixtures in a fresh store With stable ciphertext, setting an unchanged secret keeps the stored ciphertext, so the shared cipher fixture returned identical values and the ciphertext-only change tests no longer produced a change. Encrypt each fixture value in its own store so every call is a fresh encryption. Co-Authored-By: Claude Opus 5.5 --- tests/native-compose-inputs.test.ts | 6 +++++- tests/native-env-values.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/native-compose-inputs.test.ts b/tests/native-compose-inputs.test.ts index fc3fd1d4b..344ee4128 100644 --- a/tests/native-compose-inputs.test.ts +++ b/tests/native-compose-inputs.test.ts @@ -210,8 +210,12 @@ test.each([ }); /** Produce synthetic ciphertext through the existing owner outside the native fixture. */ +// Each call encrypts in a fresh store: an existing store keeps the stored +// ciphertext for an unchanged value, so reusing one would not re-encrypt. +let cipherFixtures = 0; async function encrypted(): Promise { - const fixture = join(root, "cipher-fixture"); + cipherFixtures += 1; + const fixture = join(root, `cipher-fixture-${cipherFixtures}`); await mkdir(join(fixture, ".hack"), { recursive: true }); await writeFile(join(fixture, PROJECT_ENV_KEY_FILENAME), KEY); await setProjectEnvValue({ diff --git a/tests/native-env-values.test.ts b/tests/native-env-values.test.ts index 4497f14d9..86e2c3012 100644 --- a/tests/native-env-values.test.ts +++ b/tests/native-env-values.test.ts @@ -133,8 +133,12 @@ async function linked() { } /** Generate synthetic ciphertext using the existing mutation owner, outside the native fixture. */ +// Each call encrypts in a fresh store: an existing store keeps the stored +// ciphertext for an unchanged value, so reusing one would not re-encrypt. +let cipherFixtures = 0; async function encrypted(plaintext = SENTINEL): Promise { - const fixture = join(root, "cipher-fixture"); + cipherFixtures += 1; + const fixture = join(root, `cipher-fixture-${cipherFixtures}`); await fs.mkdir(join(fixture, ".hack"), { recursive: true }); await fs.writeFile(join(fixture, PROJECT_ENV_KEY_FILENAME), KEY); await setProjectEnvValue({ From 5d58e9b063e0c81a2ed73d40580be4097d8a6da3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 14:25:38 -0400 Subject: [PATCH 5/6] fix(env): refuse to replace an env layer that cannot be read or written The rename-committed env write treated any read failure as an absent file, so `hack env set` on a layer it could not read (mode 000, or root-owned 0600 from an earlier `sudo hack`) built a layer holding only the new key and renamed it over the old one, dropping every other value. Rename also skips the write-permission check an in-place write got, so a read-only or foreign layer was silently replaced with a new owner. writeTextFileAtomicIfChanged now classifies the target before staging: only a missing path or a dangling symlink is absent. An existing target that cannot be read, is not a regular file, is not writable, or whose owner and group the staging file cannot take is refused with a closed, value-free AtomicWriteRefusedError, and the file is left as it was. A rewrite keeps the file's owner and group as well as its mode. Document the refusals and that rename detaches hard links, and list the staging pathspec in the doctor check docstring. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 10 ++ src/commands/doctor.ts | 3 +- src/lib/fs.ts | 156 +++++++++++++++-- .../atomic-write-rename-failure-worker.ts | 32 ++++ tests/project-env-stable-ciphertext.test.ts | 161 +++++++++++++++++- 5 files changed, 343 insertions(+), 19 deletions(-) create mode 100644 tests/fixtures/atomic-write-rename-failure-worker.ts diff --git a/docs/env.md b/docs/env.md index d3263b015..e445232e7 100644 --- a/docs/env.md +++ b/docs/env.md @@ -431,8 +431,18 @@ rename has these effects: - a symlinked env file keeps its link; the new contents land at the file the link points to, and the staging file is created next to that file - a symlink whose target does not exist is replaced by a regular file +- a hard link is broken: the env file gets a new inode, and any other name for the old + file keeps the previous contents - the directory must be writable, even when the env file itself is writable +A rename does not need write access to the file it replaces, so Hack checks that access +itself before it stages anything. It refuses to replace an existing env file that it +cannot read, that is not a regular file, that is not writable, or whose owner and group +it cannot keep (for example a root-owned file left by an earlier `sudo hack`). The error +names the file and the reason, never a value, and the file is left as it was. Fix the +file's permissions or ownership and run the command again. A rewrite keeps the file's +permission bits, owner and group. + If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the command applies documented defaults or fails fast with `E_INTERACTIVE_REQUIRED` instead of diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index a0c884842..5e53eabbf 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -2999,7 +2999,8 @@ async function checkProjectEnvKey({ /** * Warns when hack-generated machine-local files (`.hack/.internal/`, * `.hack/.branch/`, `.hack/.env`, `.hack/.env.state.json`, - * `.hack/hack.env.*.local.yaml`, `.hack.secret.key`) are tracked in git. + * `.hack/hack.env.*.local.yaml`, `.hack/tickets`, `.hack/.hack.env*.tmp` + * staging files, `.hack.secret.key`) are tracked in git. * A tracked secret key is reported as an error because it is a committed * secret. `hack doctor --fix` untracks the offenders (files stay on disk) and * ensures the committed `.hack/.gitignore`. diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 96558b275..8d4186465 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import { + access, constants, type FileHandle, lstat, @@ -114,23 +115,43 @@ export async function writeTextFileIfChanged( * under `.hack/` rely on its managed `.gitignore` to keep them unstaged. * Directory fsync is best effort. * - * An existing target keeps its permission bits, and an existing symlink keeps - * pointing where it did: the replacement lands at the link's resolved path, as - * a plain write through the link would. A new file gets the umask default. - * Unlike an in-place write, a dangling symlink is replaced by a regular file, - * and the containing directory must be writable. + * Rename needs no write permission on the file it replaces, so the guards an + * in-place write got from the kernel are checked explicitly, before any staging + * file exists, and fail with {@link AtomicWriteRefusedError}: an existing target + * that cannot be read, is not a regular file or is not writable is refused, and + * so is one whose owner and group the staging file cannot take (for example a + * root-owned file left by `sudo hack`). A caller that merged a change into what + * it read therefore never replaces contents it could not see. These checks are + * not an atomic fence against a concurrent permission change. + * + * An existing target keeps its permission bits, owner and group, and an + * existing symlink keeps pointing where it did: the replacement lands at the + * link's resolved path, as a plain write through the link would. A new file + * gets the umask default. Unlike an in-place write, a dangling symlink is + * replaced by a regular file, a hard link to the target is detached (its other + * names keep the previous contents), and the containing directory must be + * writable. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, content: string ): Promise<{ readonly changed: boolean }> { - const existing = await readTextFile(absolutePath); - if (existing === content) { + const existing = await inspectAtomicWriteTarget(absolutePath); + if (existing.kind === "existing" && existing.text === content) { return { changed: false }; } const target = - existing === null ? absolutePath : await realpath(absolutePath); - const mode = existing === null ? null : (await stat(target)).mode & 0o7777; + existing.kind === "absent" ? absolutePath : await realpath(absolutePath); + if (existing.kind === "existing") { + try { + await access(target, constants.W_OK); + } catch { + throw new AtomicWriteRefusedError({ + path: absolutePath, + reason: "not_writable", + }); + } + } const staging = join( dirname(target), `.${basename(target)}.${randomUUID()}.tmp` @@ -145,8 +166,10 @@ export async function writeTextFileAtomicIfChanged( ); let committed = false; try { - if (mode !== null) { - await file.chmod(mode); + if (existing.kind === "existing") { + // chown can clear set-id bits, so the mode is applied after it. + await preserveOwnership({ file, existing, path: absolutePath }); + await file.chmod(existing.mode); } await file.writeFile(content, "utf8"); await file.sync(); @@ -162,6 +185,117 @@ export async function writeTextFileAtomicIfChanged( return { changed: true }; } +/** Why {@link writeTextFileAtomicIfChanged} refused to replace a file. */ +export type AtomicWriteRefusal = + | "unreadable" + | "not_regular_file" + | "not_writable" + | "owner_not_preserved"; + +const ATOMIC_WRITE_REFUSAL_MESSAGES: Readonly< + Record +> = { + unreadable: "it exists but cannot be read, so its contents cannot be kept", + not_regular_file: "it is not a regular file", + not_writable: "it is not writable", + owner_not_preserved: "its owner and group cannot be preserved", +}; + +/** + * Closed refusal from {@link writeTextFileAtomicIfChanged}. The message names + * the path and a fixed reason only; it never carries file contents or a cause. + */ +export class AtomicWriteRefusedError extends Error { + readonly path: string; + readonly reason: AtomicWriteRefusal; + + constructor(opts: { + readonly path: string; + readonly reason: AtomicWriteRefusal; + }) { + super( + `Refusing to replace ${opts.path}: ${ATOMIC_WRITE_REFUSAL_MESSAGES[opts.reason]}.` + ); + this.name = "AtomicWriteRefusedError"; + this.path = opts.path; + this.reason = opts.reason; + } +} + +type AtomicWriteTarget = + | { readonly kind: "absent" } + | { + readonly kind: "existing"; + readonly text: string; + readonly mode: number; + readonly uid: number; + readonly gid: number; + }; + +/** + * Classifies the current target without collapsing "exists but unreadable" + * into "absent". Only a missing path, or a symlink whose target is missing, is + * `absent`; any other lstat, stat or read failure is a refusal. + */ +async function inspectAtomicWriteTarget( + absolutePath: string +): Promise { + const refuse = (reason: AtomicWriteRefusal) => + new AtomicWriteRefusedError({ path: absolutePath, reason }); + try { + await lstat(absolutePath); + } catch (error: unknown) { + if (isMissingPathError(error)) { + return { kind: "absent" }; + } + throw refuse("unreadable"); + } + let info: Awaited>; + try { + info = await stat(absolutePath); + } catch (error: unknown) { + // lstat found an entry, so a missing stat target is a dangling symlink. + if (isMissingPathError(error)) { + return { kind: "absent" }; + } + throw refuse("unreadable"); + } + if (!info.isFile()) { + throw refuse("not_regular_file"); + } + const text = await readTextFile(absolutePath); + if (text === null) { + throw refuse("unreadable"); + } + return { + kind: "existing", + text, + mode: Number(info.mode) & 0o7777, + uid: Number(info.uid), + gid: Number(info.gid), + }; +} + +/** Gives the staging file the target's owner and group, or refuses. */ +async function preserveOwnership(opts: { + readonly file: FileHandle; + readonly existing: { readonly uid: number; readonly gid: number }; + readonly path: string; +}): Promise { + const staged = await opts.file.stat(); + if (staged.uid === opts.existing.uid && staged.gid === opts.existing.gid) { + return; + } + try { + await opts.file.chown(opts.existing.uid, opts.existing.gid); + } catch { + throw new AtomicWriteRefusedError({ + path: opts.path, + reason: "owner_not_preserved", + }); + } +} + async function syncDirectoryBestEffort(directory: string): Promise { let handle: FileHandle | null = null; try { diff --git a/tests/fixtures/atomic-write-rename-failure-worker.ts b/tests/fixtures/atomic-write-rename-failure-worker.ts new file mode 100644 index 000000000..e80397224 --- /dev/null +++ b/tests/fixtures/atomic-write-rename-failure-worker.ts @@ -0,0 +1,32 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `writeTextFileAtomicIfChanged` in a disposable child whose commit + * rename fails after the staging file is created, written and fsynced. Every + * step before the rename runs unmocked. Prints `failed` when the write rejects + * and `committed` if it unexpectedly succeeds. The content is a synthetic test + * string passed on stdin, never in argv. + */ +const [target] = process.argv.slice(2); +if (!target) { + throw new Error("Missing atomic write worker target"); +} + +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async () => { + throw Object.assign(new Error("synthetic rename failure"), { + code: "EIO", + }); + }, +})); + +const content = await Bun.stdin.text(); +const { writeTextFileAtomicIfChanged } = await import("../../src/lib/fs.ts"); +try { + await writeTextFileAtomicIfChanged(target, content); + process.stdout.write("committed\n"); +} catch { + process.stdout.write("failed\n"); +} diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index 8a0b69d31..131919b2a 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -1,6 +1,7 @@ import { afterEach, expect, test } from "bun:test"; import { chmod, + chown, lstat, mkdir, mkdtemp, @@ -20,7 +21,10 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; -import { writeTextFileAtomicIfChanged } from "../src/lib/fs.ts"; +import { + AtomicWriteRefusedError, + writeTextFileAtomicIfChanged, +} from "../src/lib/fs.ts"; import { ensureHackDirGitignore, resolveProjectEnvConfig, @@ -40,6 +44,10 @@ import { privateCliEnv } from "./helpers/private-cli-env.ts"; const REPO_ROOT = resolve(import.meta.dir, ".."); const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); const WORKER = resolve(import.meta.dir, "fixtures/project-env-set-worker.ts"); +const RENAME_FAILURE_WORKER = resolve( + import.meta.dir, + "fixtures/atomic-write-rename-failure-worker.ts" +); const FIRST = "synthetic-first-6f1c"; const SECOND = "synthetic-second-92ad"; @@ -299,19 +307,127 @@ test("a symlinked layer keeps its link and the write lands at the link target", test("a write that fails after staging removes its staging file", async () => { const repo = await createRepo(); - // A directory at the target path: staging succeeds, the commit rename fails. + const target = resolve(repo.projectDir, "occupied.yaml"); + await writeFile(target, "synthetic: before\n"); + + const worker = Bun.spawn([process.execPath, RENAME_FAILURE_WORKER, target], { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + worker.stdin.write("synthetic: after\n"); + await worker.stdin.end(); + const [stdout, exitCode] = await Promise.all([ + new Response(worker.stdout).text(), + worker.exited, + ]); + expect({ stdout, exitCode }).toEqual({ stdout: "failed\n", exitCode: 0 }); + expect(await readFile(target, "utf8")).toBe("synthetic: before\n"); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); +}, 30_000); + +test("a directory at the target is refused before anything is staged", async () => { + const repo = await createRepo(); const target = resolve(repo.projectDir, "occupied.yaml"); await mkdir(target); - await expect( + const refusal = await refusalOf( writeTextFileAtomicIfChanged(target, "synthetic: content\n") - ).rejects.toThrow(); + ); + expect(refusal.reason).toBe("not_regular_file"); expect((await stat(target)).isDirectory()).toBe(true); - expect( - (await readdir(repo.projectDir)).filter((name) => name.endsWith(".tmp")) - ).toEqual([]); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); +}); + +const RUNS_AS_ROOT = + typeof process.getuid === "function" && process.getuid() === 0; + +/** Synthetic layer holding a key the failed write must not drop. */ +const OTHER_LAYER = + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global:\n OTHER_SETTING: synthetic-other-31b7\n"; + +// Root bypasses permission bits, so these refusals only apply to other users. +test.skipIf(RUNS_AS_ROOT)( + "an existing layer that cannot be read is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o000); + try { + const refusal = await refusalOf(setSecret(repo, FIRST)); + expect(refusal.reason).toBe("unreadable"); + expect(refusal.message).not.toContain(FIRST); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + } finally { + await chmod(repo.envFile, 0o600); + } + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + } +); + +test.skipIf(RUNS_AS_ROOT)( + "a readable layer that is not writable is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o444); + const before = await stat(repo.envFile); + + const refusal = await refusalOf(setSecret(repo, FIRST)); + expect(refusal.reason).toBe("not_writable"); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + const after = await stat(repo.envFile); + expect(after.ino).toBe(before.ino); + expect(after.mode & 0o777).toBe(0o444); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + } +); + +test("a rewrite keeps the layer's group", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const current = (await stat(repo.envFile)).gid; + // Any other group this process belongs to can be assigned without root. + const other = (process.getgroups?.() ?? []).find((gid) => gid !== current); + if (other === undefined) { + return; + } + await chown(repo.envFile, (await stat(repo.envFile)).uid, other); + + expect((await setSecret(repo, SECOND)).changed).toBe(true); + expect((await stat(repo.envFile)).gid).toBe(other); + expect(await resolvedToken(repo)).toBe(SECOND); +}); + +test("a dangling symlink layer is replaced by a regular file", async () => { + const repo = await createRepo(); + await symlink(resolve(repo.root, "missing.yaml"), repo.envFile); + + expect((await setSecret(repo, FIRST)).changed).toBe(true); + expect((await lstat(repo.envFile)).isFile()).toBe(true); + expect(await resolvedToken(repo)).toBe(FIRST); }); +async function refusalOf( + pending: Promise +): Promise { + try { + await pending; + } catch (error: unknown) { + if (error instanceof AtomicWriteRefusedError) { + return error; + } + throw error; + } + throw new Error("Expected the write to be refused"); +} + +async function tmpSiblings(directory: string): Promise { + return (await readdir(directory)).filter((name) => name.endsWith(".tmp")); +} + for (const layer of ["tracked", "local"] as const) { test(`a ${layer} write killed before its commit rename leaves the layer resolvable and git clean`, async () => { await assertKilledWriteIsRecoverable(layer); @@ -439,3 +555,34 @@ test("repeat `env set --secret --value-stdin` leaves git status clean", async () " M .hack/hack.env.default.yaml\n" ); }, 60_000); + +test.skipIf(RUNS_AS_ROOT)( + "`env set` on an unreadable layer fails and keeps the layer", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o000); + let exitCode: number; + try { + exitCode = await runCli( + repo, + [ + "env", + "set", + "API_TOKEN", + "--secret", + "--value-stdin", + "--path", + repo.projectRoot, + ], + `${FIRST}\n` + ); + } finally { + await chmod(repo.envFile, 0o600); + } + expect(exitCode).not.toBe(0); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + }, + 60_000 +); From e99f68c64a97b7991954f2291e1b356a93ffdff3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 20 Oct 2026 09:14:37 -0400 Subject: [PATCH 6/6] test(env): cover the refusal when a layer's owner cannot be kept The owner_not_preserved refusal in writeTextFileAtomicIfChanged had no test, because a non-root user cannot create a file owned by someone else. A disposable child now reports the target as owned by another uid, so the staging file's fchown really fails with EPERM. The test expects the closed refusal, the old bytes and inode kept, and no staging file left. Co-Authored-By: Claude Opus 5.5 --- .../atomic-write-foreign-owner-worker.ts | 48 +++++++++++++++++++ tests/project-env-stable-ciphertext.test.ts | 39 +++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 tests/fixtures/atomic-write-foreign-owner-worker.ts diff --git a/tests/fixtures/atomic-write-foreign-owner-worker.ts b/tests/fixtures/atomic-write-foreign-owner-worker.ts new file mode 100644 index 000000000..7b7f907c7 --- /dev/null +++ b/tests/fixtures/atomic-write-foreign-owner-worker.ts @@ -0,0 +1,48 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `writeTextFileAtomicIfChanged` in a disposable child where the + * target appears to belong to another user: `stat` of the target reports the + * caller's uid plus one, so the staging file must be `fchown`ed to a user the + * caller is not. Run as a non-root user, that `fchown` really fails with EPERM; + * nothing else is mocked. Prints `refused:` for an + * `AtomicWriteRefusedError`, `failed` for any other rejection and `committed` + * if the write succeeds. The content is a synthetic test string passed on + * stdin, never in argv. + */ +const [target] = process.argv.slice(2); +if (!target) { + throw new Error("Missing atomic write worker target"); +} + +// Captured before mocking: the namespace binding then points at the mock. +const realStat = fs.stat; + +mock.module("node:fs/promises", () => ({ + ...fs, + stat: async (...args: Parameters) => { + const info = await realStat(...args); + if (args[0] !== target) { + return info; + } + return Object.assign(Object.create(Object.getPrototypeOf(info)), info, { + uid: Number(info.uid) + 1, + }); + }, +})); + +const content = await Bun.stdin.text(); +const { AtomicWriteRefusedError, writeTextFileAtomicIfChanged } = await import( + "../../src/lib/fs.ts" +); +try { + await writeTextFileAtomicIfChanged(target, content); + process.stdout.write("committed\n"); +} catch (error: unknown) { + process.stdout.write( + error instanceof AtomicWriteRefusedError + ? `refused:${error.reason}\n` + : "failed\n" + ); +} diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index 131919b2a..46b5442ee 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -48,6 +48,10 @@ const RENAME_FAILURE_WORKER = resolve( import.meta.dir, "fixtures/atomic-write-rename-failure-worker.ts" ); +const FOREIGN_OWNER_WORKER = resolve( + import.meta.dir, + "fixtures/atomic-write-foreign-owner-worker.ts" +); const FIRST = "synthetic-first-6f1c"; const SECOND = "synthetic-second-92ad"; @@ -401,6 +405,41 @@ test("a rewrite keeps the layer's group", async () => { expect(await resolvedToken(repo)).toBe(SECOND); }); +// Root can chown to any user, so the refusal only applies to other users. +test.skipIf(RUNS_AS_ROOT)( + "a layer whose owner cannot be kept is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + const before = await stat(repo.envFile); + + const worker = Bun.spawn( + [process.execPath, FOREIGN_OWNER_WORKER, repo.envFile], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write("synthetic: after\n"); + await worker.stdin.end(); + const [stdout, exitCode] = await Promise.all([ + new Response(worker.stdout).text(), + worker.exited, + ]); + expect({ stdout, exitCode }).toEqual({ + stdout: "refused:owner_not_preserved\n", + exitCode: 0, + }); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + expect((await stat(repo.envFile)).ino).toBe(before.ino); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + }, + 30_000 +); + test("a dangling symlink layer is replaced by a regular file", async () => { const repo = await createRepo(); await symlink(resolve(repo.root, "missing.yaml"), repo.envFile);