From aed5f4fbd9ad9b63eafbd158cacdfe6a5fe3c6d5 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 10:33:35 -0400 Subject: [PATCH 1/2] fix(env): name unreclaimable layer locks and the re-include cause - An env layer lock that Hack can never reclaim (a malformed or untrusted receipt, a reused PID, a dead owner behind a leftover `.recovery` guard) made every set/add/unset/hydrate refuse with "Retry when it finishes." forever. The timeout now names the lock path and the recovery step, and keeps the registry's "recovery guard is busy" detail with both paths. Closed, one line, value-free. - The unignored-key refusal no longer says Hack "could not add" the entry when the root .gitignore already lists the key and a later rule re-includes it; it names that rule as the cause. - A later `git init` is out of scope for env writes (Hack runs no code then). `hack doctor`'s env key check is the backstop: it flags the unignored key, and the next --secret write repairs the entry. Documented in docs/env.md and covered end to end. - Tests: paused-unset compare-and-swap conflict, unreclaimable lock receipts, recovery guard (set and hydrate), re-include message, and the git-init doctor scenario. Each was proven by mutation. Refs HACK-1386 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 30 +++- src/commands/doctor.ts | 11 +- src/lib/env-layer-lock.ts | 67 +++++++-- src/lib/project-env-config.ts | 26 +++- tests/project-env-key-ignore.test.ts | 101 ++++++++++++- tests/project-env-layer-lock.test.ts | 214 ++++++++++++++++++++++++++- 6 files changed, 419 insertions(+), 30 deletions(-) diff --git a/docs/env.md b/docs/env.md index 32dc18f3c..49323e021 100644 --- a/docs/env.md +++ b/docs/env.md @@ -103,7 +103,17 @@ How it is maintained: key is created. Before a `--secret` set or hydrate uses an existing checkout-local key that git does not ignore, Hack adds the entry; if it cannot, or a later rule re-includes the key, the command is refused and - nothing is encrypted with that key + nothing is encrypted with that key. The refusal names the cause: either the + entry could not be written, or the file already lists the key and a later + rule (such as `!.hack.secret.key`) re-includes it, which you need to remove +- outside git there is nothing to commit, so an existing key does not need the + entry and Hack does not recreate a deleted root `.gitignore` for it. Hack + does not run when you later turn the directory into a repository with + `git init`, so it cannot add the entry at that moment. If the entry is + missing then, `hack doctor` reports `env-key-unignored` (see + [Key strength and exposure](#secret-key-handling)), and the first `--secret` set or + hydrate in the new repository adds the entry before it uses the key, or is + refused - older repos that added `.hack/.internal/` to the root `.gitignore` keep working; the entries are equivalent and nothing is removed @@ -663,9 +673,19 @@ file, change it and replace it while holding that file's write lock, The same applies to the legacy `.hack/.env` files. So when several commands change different keys of the same file at once, every change is kept. A command that finds the lock held waits up to 10 seconds, then stops with a one-line message that -names the file and says nothing was written; run it again. The lock is released when the -command ends. If a command is killed while it holds the lock, the next command takes the -lock over once it confirms that process has exited. +names the file and its lock and says nothing was written; run it again. The lock is +released when the command ends. If a command is killed while it holds the lock, the next +command takes the lock over once it confirms that process has exited. + +Hack never removes a lock it cannot prove is abandoned. That includes a lock file that +is not a complete Hack receipt, one owned by another user or writable by others, and one +that names a process ID now used by an unrelated live process. In those cases every +retry would wait and fail the same way, so the message also says what to do: if no +`hack env` command is running, remove the lock file it names and retry. If a command was +killed while it was taking over an abandoned lock, it leaves `.recovery` behind, +and the message says the recovery guard is busy and names both paths; when no `hack env` +command is running, remove both and retry. `hack env hydrate` reports the same message +when it cannot get the env file's lock. `.hack/.internal/` is ignored by the managed `.hack/.gitignore`, so lock files never show in `git status`. If `.hack/.internal` is a symlink or not a directory, these commands @@ -920,7 +940,7 @@ it cannot spell portably as `/.hack.secret.key`. | `env-key-wrong-owner` | warn | the file is not owned by the current user | | `env-key-wrong-length` | warn | shorter than the 43-character base64url form of a 256-bit key, or empty | | `env-key-low-entropy` | warn | long enough but repetitive (estimated under 128 bits) | -| `env-key-unignored` | warn | the checkout-local key is not ignored by git, so `git status` offers it for commit; suggests adding it to the root `.gitignore` | +| `env-key-unignored` | warn | the checkout-local key is not ignored by git, so `git status` offers it for commit (for example after `git init` in a directory whose root `.gitignore` entry was removed); suggests adding it to the root `.gitignore` | | `env-key-symlink` | error | the key path is a symlink; Hack refuses symlinked keys | | `env-key-not-regular` | error | the key path is a directory or another non-file entry | | `env-key-unreadable` | error | unreadable, oversized or not UTF-8 | diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 52ea034d3..2453c879c 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -2932,10 +2932,6 @@ export function envKeyCheckResult(inspection: EnvKeyInspection): CheckResult { }; } -/** - * Read-only key-strength and exposure audit (HACK-1127). `--fix` never acts on - * it; each finding suggests the exact command instead. - */ /** * Reports which Compose features keep the current project on the Compose * backend (HACK-1291). Reads only the project's Compose file and, on the native @@ -2963,7 +2959,12 @@ async function checkComposeBackendFeatures(opts: { }); } -async function checkProjectEnvKey({ +/** + * Read-only key-strength and exposure audit (HACK-1127). `--fix` never acts on + * it; each finding suggests the exact command instead. It also catches a key + * that a later `git init` left unignored (`env-key-unignored`, HACK-1386). + */ +export async function checkProjectEnvKey({ startDir, }: { readonly startDir: string; diff --git a/src/lib/env-layer-lock.ts b/src/lib/env-layer-lock.ts index 0daa22397..760bc6662 100644 --- a/src/lib/env-layer-lock.ts +++ b/src/lib/env-layer-lock.ts @@ -13,18 +13,55 @@ import { export const ENV_LAYER_LOCK_WAIT_MS = 10_000; /** - * Another `hack env` writer held the layer's lock for the whole bounded wait - * (or an interrupted lock recovery needs offline inspection). Closed and - * retryable: nothing was read or written, and the message names only the - * layer path, never a key or value. + * Why {@link EnvLayerLockTimeoutError} gave up: + * - `held`: the lock stayed occupied for the whole wait. Either a live Hack + * writer holds it, or Hack can never reclaim it on its own (a malformed + * receipt, one owned by another user, or a PID now used by a live process). + * - `recovery_guard`: the owner has exited, but the `.recovery` guard + * left by an interrupted reclaimer blocks reclamation until it is removed. + */ +export type EnvLayerLockTimeoutReason = "held" | "recovery_guard"; + +/** The projects-registry lock's detail for a dead owner behind a busy guard. */ +const RECOVERY_GUARD_BUSY_DETAIL = + "recovery guard is busy (interrupted recovery requires offline inspection)"; + +/** + * The layer's lock stayed held for the whole bounded wait. Closed and + * retryable: nothing was read or written. The one-line message names the + * layer, its lock path and the recovery step, never a key or value, so a lock + * Hack cannot reclaim does not leave the user retrying a command that can + * never succeed. */ export class EnvLayerLockTimeoutError extends ClosedCliError { - constructor(opts: { readonly path: string; readonly waitMs: number }) { - super( - `Another hack env command is writing ${opts.path}; gave up after waiting ${formatWait(opts.waitMs)} for it. Nothing was written. Retry when it finishes.` - ); + readonly reason: EnvLayerLockTimeoutReason; + readonly lockPath: string; + + constructor(opts: { + readonly path: string; + readonly lockPath: string; + readonly waitMs: number; + readonly reason: EnvLayerLockTimeoutReason; + }) { + super(envLayerLockTimeoutMessage(opts)); this.name = "EnvLayerLockTimeoutError"; + this.reason = opts.reason; + this.lockPath = opts.lockPath; + } +} + +function envLayerLockTimeoutMessage(opts: { + readonly path: string; + readonly lockPath: string; + readonly waitMs: number; + readonly reason: EnvLayerLockTimeoutReason; +}): string { + const wait = formatWait(opts.waitMs); + if (opts.reason === "recovery_guard") { + const guard = `${opts.lockPath}.recovery`; + return `Refused to update ${opts.path}: its lock at ${opts.lockPath} was left by a process that has exited, and the ${RECOVERY_GUARD_BUSY_DETAIL} at ${guard}; gave up after waiting ${wait}. Nothing was written. If no hack env command is running, remove ${guard} and ${opts.lockPath}, then retry.`; } + return `Another hack env command is writing ${opts.path}; gave up after waiting ${wait} for its lock at ${opts.lockPath}. Nothing was written. Retry when it finishes. If no hack env command is running, Hack cannot reclaim that lock itself: remove ${opts.lockPath} and retry.`; } /** @@ -69,7 +106,10 @@ export function resolveEnvLayerLockPath(opts: { * reclaimed only after `kill(pid, 0)` reports the owner gone (ESRCH). * - The wait is bounded (`timeoutMs`, default {@link ENV_LAYER_LOCK_WAIT_MS}) * with jittered polling. When it runs out, {@link EnvLayerLockTimeoutError} - * is thrown and `run` never starts, so nothing is written. + * is thrown and `run` never starts, so nothing is written. Its message + * names the lock path and how to clear a lock Hack cannot reclaim itself + * (a malformed or foreign receipt, a reused PID, an interrupted recovery's + * guard); this helper never removes such a lock on its own. * - A symlinked or non-directory `.internal` beside the layer is refused with * {@link EnvLayerLockRefusedError}; a filesystem failure while taking the * lock (permission, full disk) is refused as the layer it blocked, never as @@ -109,7 +149,14 @@ export async function withEnvLayerLock(opts: { throw error; } if (error instanceof ProjectsRegistryLockTimeoutError) { - throw new EnvLayerLockTimeoutError({ path: opts.layerPath, waitMs }); + throw new EnvLayerLockTimeoutError({ + path: opts.layerPath, + lockPath, + waitMs, + reason: error.message.includes(RECOVERY_GUARD_BUSY_DETAIL) + ? "recovery_guard" + : "held", + }); } if (!(await directoryWritable(dirname(opts.layerPath)))) { // No Hack writer can replace a layer whose directory it cannot write diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index fa7684f57..0c03f9cbd 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -4180,12 +4180,20 @@ async function writeProjectEnvKeyWithFallback(opts: { return { keyPath: opts.localKeyPath, keyText, warnings }; } -/** Fixed, value-free refusal for a checkout-local key git would commit. */ +/** + * Fixed, value-free refusal for a checkout-local key git would commit. + * `unwritable`: Hack could not add the entry to the root `.gitignore`. + * `reincluded`: the entry is there, but a later rule in that file (such as + * `!.hack.secret.key`) re-includes the key, so adding it again cannot help. + */ function projectEnvKeyUnignoredError(opts: { readonly rootGitignore: string; + readonly cause: "unwritable" | "reincluded"; }): ProjectEnvKeyRefusalError { return new ProjectEnvKeyRefusalError( - `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${opts.rootGitignore}. Add a '${PROJECT_ENV_KEY_FILENAME}' line to that file (making it writable if needed) and remove any rule that re-includes the key, then retry; key contents are never included in this message.` + opts.cause === "reincluded" + ? `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed. ${opts.rootGitignore} lists '${PROJECT_ENV_KEY_FILENAME}', but a later rule in that file re-includes the key (for example '!${PROJECT_ENV_KEY_FILENAME}'). Remove that rule, then retry; key contents are never included in this message.` + : `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${opts.rootGitignore}. Add a '${PROJECT_ENV_KEY_FILENAME}' line to that file (making it writable if needed) and remove any rule that re-includes the key, then retry; key contents are never included in this message.` ); } @@ -4203,7 +4211,12 @@ function projectEnvKeyUnignoredError(opts: { * key is not used. * * Either way, a key git still does not ignore after the entry (for example a - * later `!.hack.secret.key` rule) is refused. + * later `!.hack.secret.key` rule) is refused, naming that cause. + * + * A checkout outside git that later becomes one (`git init`) is not watched: + * Hack runs no code at that point. If the entry went missing meanwhile, the + * first `--secret` set or hydrate in the new checkout repairs it or refuses + * here, and `hack doctor`'s env key check reports `env-key-unignored`. */ async function ensureProjectEnvKeyIgnored(opts: { readonly projectRoot: string; @@ -4224,10 +4237,13 @@ async function ensureProjectEnvKeyIgnored(opts: { } catch (error: unknown) { throw opts.creating ? closedFileWriteError({ error, path: rootGitignore, operation: "write" }) - : projectEnvKeyUnignoredError({ rootGitignore }); + : projectEnvKeyUnignoredError({ rootGitignore, cause: "unwritable" }); } + // The entry is in the root `.gitignore` now (written or already there), and + // no deeper ignore file applies to a key at the project root, so only a + // later rule in that same file can re-include it. if ((await isGitIgnoredProjectEnvPath(probe)) === false) { - throw projectEnvKeyUnignoredError({ rootGitignore }); + throw projectEnvKeyUnignoredError({ rootGitignore, cause: "reincluded" }); } } diff --git a/tests/project-env-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts index d8f2de92c..9bc051fd7 100644 --- a/tests/project-env-key-ignore.test.ts +++ b/tests/project-env-key-ignore.test.ts @@ -11,7 +11,7 @@ import { } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; - +import { checkProjectEnvKey } from "../src/commands/doctor.ts"; import { PROJECT_COMPOSE_FILENAME, PROJECT_CONFIG_FILENAME, @@ -233,6 +233,11 @@ function unignoredRefusal(repo: Repo): string { return `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${repo.gitignore}.`; } +/** HACK-1386: the entry is present, so the refusal names the re-including rule instead. */ +function reincludedRefusal(repo: Repo): string { + return `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed. ${repo.gitignore} lists '${PROJECT_ENV_KEY_FILENAME}', but a later rule in that file re-includes the key (for example '!${PROJECT_ENV_KEY_FILENAME}'). Remove that rule, then retry; key contents are never included in this message.`; +} + /** Creates the key with a first secret, then drops its ignore entry (the pre-fix state). */ async function createUnignoredKey(repo: Repo): Promise { const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE); @@ -352,9 +357,12 @@ test("a rule that re-includes the key is refused both before creating a key and await writeFile(repo.gitignore, reincluding); const first = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); - expectOneLineRefusal(first, unignoredRefusal(repo)); + expectOneLineRefusal(first, reincludedRefusal(repo)); + expect(`${first.stdout}${first.stderr}`).not.toContain("could not add it"); expect(await keyExists(repo)).toBe(false); expect(await readFile(repo.layer, "utf8")).toBe(LAYER); + // The file already listed the key, so it was not rewritten. + expect(await readFile(repo.gitignore, "utf8")).toBe(reincluding); // An existing key under the same rule is refused before it encrypts. await writeFile(repo.gitignore, BASE_GITIGNORE); @@ -362,7 +370,7 @@ test("a rule that re-includes the key is refused both before creating a key and await writeFile(repo.gitignore, reincluding); const layerBefore = await readFile(repo.layer, "utf8"); const next = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); - expectOneLineRefusal(next, unignoredRefusal(repo)); + expectOneLineRefusal(next, reincludedRefusal(repo)); expectNoKeyMaterial(next, keyText); expect(await readFile(repo.layer, "utf8")).toBe(layerBefore); }, 60_000); @@ -382,3 +390,90 @@ test("outside git an existing key is used without writing a root .gitignore", as expect(run.exitCode).toBe(0); expect(await lstat(repo.gitignore).catch(() => null)).toBeNull(); }, 60_000); + +/** + * Runs `run` in this process with the fixture's private HOME, HACK_HOME and + * global config, and git's global, system and XDG configuration (so any + * personal excludes file) out of reach; restores the previous environment. + */ +async function withPrivateEnvironment( + repo: Repo, + run: () => Promise +): Promise { + const home = join(repo.root, "user-home"); + const overrides: Record = { + HOME: home, + XDG_CONFIG_HOME: join(home, ".config"), + HACK_HOME: join(repo.root, "hack-home"), + HACK_GLOBAL_CONFIG_PATH: join(repo.root, "hack-config.json"), + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }; + const previous = Object.fromEntries( + Object.keys(overrides).map((key) => [key, process.env[key]]) + ); + Object.assign(process.env, overrides); + try { + return await run(); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) { + Reflect.deleteProperty(process.env, key); + } else { + process.env[key] = value; + } + } + } +} + +/** The real `hack doctor` env key check for the fixture, in this process. */ +async function doctorEnvKey(repo: Repo): Promise<{ + readonly status: string; + readonly codes: readonly string[]; + readonly message: string; +}> { + const result = await withPrivateEnvironment( + repo, + async () => await checkProjectEnvKey({ startDir: repo.projectRoot }) + ); + return { + status: result.status, + codes: (result.envKey?.findings ?? []).map((finding) => finding.code), + message: result.message, + }; +} + +test("a key whose entry was removed before a later git init is flagged by doctor and repaired by the next secret set", async () => { + // HACK-1386: outside git an existing key needs no entry, and Hack does not + // observe a later `git init`. Doctor's env key check is the backstop, and + // the first secret write in the new checkout restores the entry. + const repo = await createRepo({ git: false }); + const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE); + expect(first.exitCode).toBe(0); + const keyText = (await readFile(repo.keyPath, "utf8")).trim(); + await rm(repo.gitignore); + const second = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + expect(second.exitCode).toBe(0); + expect(await lstat(repo.gitignore).catch(() => null)).toBeNull(); + + expect((await git(repo, ["init", "-q"])).exitCode).toBe(0); + expect(await keyIsCommittable(repo)).toBe(true); + + const flagged = await doctorEnvKey(repo); + expect(flagged.status).toBe("warn"); + expect(flagged.codes).toContain("env-key-unignored"); + expect(flagged.message).toContain( + `env-key-unignored: ${PROJECT_ENV_KEY_FILENAME} is not ignored by git` + ); + expect(flagged.message).not.toContain(keyText); + expect(flagged.message).not.toContain(repo.root); + + const repair = await setSecret(repo, "THIRD_SECRET", FIRST_VALUE); + expect(repair.exitCode).toBe(0); + expectNoKeyMaterial(repair, keyText); + expect(await keyIsCommittable(repo)).toBe(false); + const repaired = await doctorEnvKey(repo); + expect(repaired.codes).not.toContain("env-key-unignored"); + expect(repaired.status).toBe("ok"); +}, 60_000); diff --git a/tests/project-env-layer-lock.test.ts b/tests/project-env-layer-lock.test.ts index c98af390c..7ece97a6e 100644 --- a/tests/project-env-layer-lock.test.ts +++ b/tests/project-env-layer-lock.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, expect, test } from "bun:test"; +import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; import { randomUUID } from "node:crypto"; import { chmod, @@ -31,6 +31,7 @@ import { } from "../src/lib/fs.ts"; import { removeDotEnvKey, upsertDotEnvValue } from "../src/lib/hack-env.ts"; import { + hydrateProjectEnvLocalLayer, resolveProjectEnvConfig, resolveProjectEnvConfigPath, resolveProjectEnvLocalConfigPath, @@ -38,6 +39,7 @@ import { setProjectEnvValue, unsetProjectEnvValue, } from "../src/lib/project-env-config.ts"; +import * as worktreeLocalConfig from "../src/lib/worktree-local-config.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv, privateCliHome } from "./helpers/private-cli-env.ts"; import { createSecretWorktreeFixture } from "./helpers/secret-fixture.ts"; @@ -470,6 +472,14 @@ test("concurrent legacy dotenv sets and removals of different keys all apply", a } }, 60_000); +/** HACK-1386: the timeout names the lock and how to clear one Hack cannot reclaim. */ +function heldLockRefusal(opts: { + readonly layer: string; + readonly lockPath: string; +}): string { + return `Another hack env command is writing ${opts.layer}; gave up after waiting 300ms for its lock at ${opts.lockPath}. Nothing was written. Retry when it finishes. If no hack env command is running, Hack cannot reclaim that lock itself: remove ${opts.lockPath} and retry.`; +} + /** Publishes a live owner receipt (this test process) at the layer's lock. */ async function holdLayerLock(layerPath: string): Promise { const lockPath = resolveEnvLayerLockPath({ layerPath }); @@ -537,9 +547,10 @@ test("a writer that cannot get the layer lock in time refuses closed and writes ]; for (const [index, refusal] of refusals.entries()) { const layer = index < 2 ? file : envFile; + const layerLock = index < 2 ? lockPath : dotEnvLockPath; expect(refusal).toBeInstanceOf(EnvLayerLockTimeoutError); expect(refusal.message).toBe( - `Another hack env command is writing ${layer}; gave up after waiting 300ms for it. Nothing was written. Retry when it finishes.` + heldLockRefusal({ layer, lockPath: layerLock }) ); expect(refusal.message).not.toContain(synthetic("BLOCKED")); expect(refusal.message).not.toContain(synthetic("PRESET")); @@ -720,3 +731,202 @@ test("a set refuses with a conflict when a writer that takes no lock changed the // The other writer's layer stands; the set wrote nothing. expect(await readFile(file, "utf8")).toBe(edited); }, 30_000); + +test("an unset refuses with a conflict when a writer that takes no lock changed the layer after its read", async () => { + // HACK-1386: only the set's compare-and-swap was covered. A local unset + // asks whether a primary checkout's local layer is inherited after it read + // the layer and before it writes; pausing that one call opens the window + // in which an editor (or Hack v4) rewrites the layer. + const project = await createProject(); + const file = resolveProjectEnvLocalConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + await writeFile( + file, + layerText({ + PRESET: synthetic("PRESET"), + DOOMED: synthetic("DOOMED"), + }) + ); + const { promise: reached, resolve: markReached } = + Promise.withResolvers(); + const { promise: release, resolve: resume } = Promise.withResolvers(); + const paused = spyOn( + worktreeLocalConfig, + "resolvePrimaryLocalProjectDir" + ).mockImplementation(async () => { + markReached(); + await release; + return null; + }); + try { + const pending = unsetProjectEnvValue({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + key: "DOOMED", + local: true, + }).then( + () => null, + (error: unknown) => error + ); + await reached; + const edited = layerText({ + PRESET: synthetic("PRESET"), + DOOMED: synthetic("DOOMED"), + HAND_EDITED: synthetic("HAND_EDITED"), + }); + await writeFile(file, edited); + resume(); + + const error = await pending; + expect(paused).toHaveBeenCalledTimes(1); + expect(error).toBeInstanceOf(AtomicWriteConflictError); + // The other writer's layer stands, HAND_EDITED included; nothing was removed. + expect(await readFile(file, "utf8")).toBe(edited); + } finally { + resume(); + paused.mockRestore(); + } +}); + +/** A PID that has exited: a short child that this test waited for. */ +function exitedPid(): number { + const child = Bun.spawnSync({ cmd: ["true"], stdout: "ignore" }); + expect(child.exitCode).toBe(0); + return child.pid; +} + +test("a layer lock Hack cannot reclaim makes set and unset refuse with its path and the recovery step", async () => { + // HACK-1386: a malformed receipt, a receipt that is not private, and a + // live PID that holds no Hack lock (a reused PID) are never reclaimed, so + // before the fix every retry failed with only "Retry when it finishes." + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const original = layerText({ PRESET: synthetic("PRESET") }); + await writeFile(file, original); + const lockPath = resolveEnvLayerLockPath({ layerPath: file }); + await mkdir(dirname(lockPath), { recursive: true }); + const base = { + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + layerLockTimeoutMs: 300, + }; + // A group-writable receipt is not trusted, like one owned by another user, + // even though its owner has exited. PID 1 is alive and holds no Hack lock, + // standing in for a reused PID. + const receipts: readonly { readonly text: string; readonly mode: number }[] = + [ + { text: "not a receipt\n", mode: 0o600 }, + { text: `${exitedPid()}\n`, mode: 0o620 }, + { text: "1\n", mode: 0o600 }, + ]; + for (const receipt of receipts) { + await writeFile(lockPath, receipt.text); + await chmod(lockPath, receipt.mode); + for (const attempt of [ + () => + setProjectEnvValue({ + ...base, + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + }), + () => unsetProjectEnvValue({ ...base, key: "PRESET" }), + ]) { + const refusal = await closedRefusalOf(attempt()); + expect(refusal).toBeInstanceOf(EnvLayerLockTimeoutError); + expect((refusal as EnvLayerLockTimeoutError).reason).toBe("held"); + expect(refusal.message).toBe(heldLockRefusal({ layer: file, lockPath })); + expect(refusal.message).not.toContain("\n"); + expect(refusal.message).not.toContain(synthetic("BLOCKED")); + expect(refusal.message).not.toContain(synthetic("PRESET")); + } + expect(await readFile(file, "utf8")).toBe(original); + // Nothing reclaimed it: the receipt is exactly as it was left. + expect(await readFile(lockPath, "utf8")).toBe(receipt.text); + } + + // The recovery step the message names works. + await rm(lockPath); + const done = await unsetProjectEnvValue({ ...base, key: "PRESET" }); + expect(done.changed).toBe(true); + expect(await readdir(dirname(lockPath))).toEqual([]); +}); + +test("a dead owner behind an interrupted recovery guard is refused with both paths, also by hydrate", async () => { + // HACK-1386: a reclaimer that crashed leaves `.recovery`; the guard + // is never age-reclaimed, so the owner can never be replaced on its own. + const project = await createProject(); + const file = resolveProjectEnvLocalConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const original = layerText({ PRESET: synthetic("PRESET") }); + await writeFile(file, original); + const lockPath = resolveEnvLayerLockPath({ layerPath: file }); + const guard = `${lockPath}.recovery`; + await mkdir(guard, { recursive: true, mode: 0o700 }); + await writeFile(lockPath, `${exitedPid()}\n${randomUUID()}\n`, { + mode: 0o600, + }); + const expected = `Refused to update ${file}: its lock at ${lockPath} was left by a process that has exited, and the recovery guard is busy (interrupted recovery requires offline inspection) at ${guard}; gave up after waiting 300ms. Nothing was written. If no hack env command is running, remove ${guard} and ${lockPath}, then retry.`; + + const refusal = await closedRefusalOf( + setProjectEnvValue({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + local: true, + layerLockTimeoutMs: 300, + }) + ); + expect(refusal).toBeInstanceOf(EnvLayerLockTimeoutError); + expect((refusal as EnvLayerLockTimeoutError).reason).toBe("recovery_guard"); + expect(refusal.message).toBe(expected); + + // Hydration waits the default 10 s for the same lock and refuses alike. + const hydrated = await closedRefusalOf( + hydrateProjectEnvLocalLayer({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + entries: new Map([["HYDRATED", synthetic("HYDRATED")]]), + secret: false, + }) + ); + expect(hydrated).toBeInstanceOf(EnvLayerLockTimeoutError); + expect(hydrated.message).toBe(expected.replace("300ms", "10s")); + expect(hydrated.message).not.toContain(synthetic("HYDRATED")); + expect(await readFile(file, "utf8")).toBe(original); + // Hydration released its own lock; the layer lock and guard are untouched. + expect((await readdir(dirname(lockPath))).sort()).toEqual( + [basename(lockPath), basename(guard)].sort() + ); + + await rm(guard, { recursive: true }); + await rm(lockPath); + const done = await setProjectEnvValue({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + local: true, + }); + expect(done.changed).toBe(true); +}, 30_000); From ed877f5eb22ecd64af658ad197a02db7ee79c592 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 11:29:26 -0400 Subject: [PATCH 2/2] test(env): set the doctor fixture env per key so the spawn-env guard passes The spawn-env isolation guard counts any whole-object use of process.env as a copy of the runner's environment, so withPrivateEnvironment's Object.assign(process.env, overrides) failed it in hosted CI. Set each override by key and restore it with the shared restoreEnv helper; the values were already private. Refs HACK-1386 Co-Authored-By: Claude Opus 5.5 --- tests/project-env-key-ignore.test.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/project-env-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts index 9bc051fd7..252807bfe 100644 --- a/tests/project-env-key-ignore.test.ts +++ b/tests/project-env-key-ignore.test.ts @@ -18,6 +18,7 @@ import { PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; import { resolveProjectEnvConfigPath } from "../src/lib/project-env-config.ts"; +import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; /** @@ -413,16 +414,14 @@ async function withPrivateEnvironment( const previous = Object.fromEntries( Object.keys(overrides).map((key) => [key, process.env[key]]) ); - Object.assign(process.env, overrides); + for (const [key, value] of Object.entries(overrides)) { + process.env[key] = value; + } try { return await run(); } finally { for (const [key, value] of Object.entries(previous)) { - if (value === undefined) { - Reflect.deleteProperty(process.env, key); - } else { - process.env[key] = value; - } + restoreEnv(key, value); } } }