From 8847da47ae23fd15496d009f5fbf9b1b65b33bf5 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 11:50:06 -0400 Subject: [PATCH 01/14] 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 02/14] 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 03/14] 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 06f262309db3111e65c8a23324456ef73ac57e83 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:48:25 -0400 Subject: [PATCH 04/14] feat(env): idempotent batch hydration from stdin or file Add `hack env hydrate`, which reads KEY=value entries (or one JSON object with --format json) from piped stdin or --file and writes them to the machine-local env layer in one rename-committed write. - Only the effective local layer is written; a git-tracked one is refused. - An entry that already holds its value is kept (secret ciphertext and scalar YAML type), and a no-op import does not rewrite the file. A changed value gets new ciphertext with a fresh IV. - Local entries outside the input, other scopes and removal markers for other keys are kept. - A secret in the layer that does not open under the reader key, a key-file mint that would shadow HACK_ENV_SECRET_KEY, a held or stale hydration lock, and a layer changed by another writer (compare-and-swap recheck before the rename) all refuse without changing the layer. - Input is bounded (1 MiB), strict and all-or-nothing; refusals are closed and value-free and name at most a line number. - Unexpected positional arguments are now reported by count, not echoed, so `hack env hydrate KEY=value` cannot print the value. Refs HACK-1158. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 69 +- docs/reference/cli.md | 29 + .../skills/hack-cli/references/patterns.md | 12 +- src/agents/pattern-catalogue.ts | 10 +- src/cli/command.ts | 7 +- src/commands/env.ts | 119 +++ src/lib/env-hydrate-input.ts | 417 +++++++++++ src/lib/fs.ts | 28 +- src/lib/project-env-config.ts | 510 ++++++++++++- tests/cli-command.test.ts | 9 + tests/env-hydrate-input.test.ts | 233 ++++++ tests/fixtures/project-env-hydrate-worker.ts | 84 +++ tests/project-env-hydrate.test.ts | 692 ++++++++++++++++++ 13 files changed, 2203 insertions(+), 16 deletions(-) create mode 100644 src/lib/env-hydrate-input.ts create mode 100644 tests/env-hydrate-input.test.ts create mode 100644 tests/fixtures/project-env-hydrate-worker.ts create mode 100644 tests/project-env-hydrate.test.ts diff --git a/docs/env.md b/docs/env.md index d3263b015..7f17db0d5 100644 --- a/docs/env.md +++ b/docs/env.md @@ -255,6 +255,7 @@ into a host command, and `--target` on env and host commands chooses the `host` | `hack env apply` | `--env`, `--service` | Runs `hack restart ` with the same `--env`, recreating one service. | This is the refresh path for one service. | `--service` is required. | | `hack env add` | `--env`, `--service` | Selects the file written (`hack.env..yaml`, or the base file for `base`/`default`/`none`; `--local` writes the local variant). | Running workloads keep old values until `hack env apply` or `hack restart`. | Mutates only env files. | | `hack env set` | `--env`, `--service` | Alias for `hack env add`. | Same as `hack env add`. | Mutates only env files. | +| `hack env hydrate` | `--env`, `--service` | Selects the local file written, as for `hack env add --local`; imports every entry in one commit. | Same as `hack env add`. | Mutates only the machine-local env file; refuses a git-tracked one. | | `hack env unset` | `--env`, `--service` | Selects the file the key is removed from, as for `hack env add`. | Same as `hack env add`. | Mutates only env files. | | `hack env materialize` | `--env`, `--service` | Writes `.hack/.env` once, at invocation. | Rerun `hack env materialize`. | Compatibility output for external tools; runtime injection does not depend on it. | | `hack env exec` | `--env`, `--service`, `--target` | Per invocation, into one host command (host view by default). | Each invocation resolves again. | Runs on the host, not in a container. | @@ -419,7 +420,8 @@ A hand-edited file (comments, other formatting, or no `global: {}`) is normalize 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. +is not atomic across them. To write many machine-local values in one commit, use +`hack env hydrate` (below). 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. @@ -448,6 +450,71 @@ hack env unset SERVICE_TOKEN --service api --env qa hack env unset API_BASE_URL --local ``` +### Hydrate machine-local values in one step + +`hack env hydrate` imports many values into the machine-local layer at once, for example +when a new checkout or machine pulls its secrets from a secret manager. Tracked defaults +stay in `hack.env.default.yaml`; hydration writes only the gitignored local layer +(`.hack/hack.env.local.yaml`, or `.hack/hack.env..local.yaml` with `--env`). + +```bash +# Pipe KEY=value lines from a secret manager; values never appear in argv. + | hack env hydrate --secret + +# Or read a file, or a pipe such as <(secret-source). +hack env hydrate --secret --file .secrets.env +hack env hydrate --secret --file secrets.json --format json --env qa --service api +``` + +Input comes only from piped stdin or `--file`, never from arguments, and is at most +1 MiB. The default `env` format is strict, one entry per line: + +- blank lines and lines starting with `#` are skipped, and an `export ` prefix is allowed +- `KEY=value`, where the key matches `^[A-Z_][A-Z0-9_]*$`; an unquoted value is the + rest of the line with surrounding spaces removed, and `#` inside it is part of the value +- `'...'` is literal; `"..."` accepts `\n`, `\r`, `\t`, `\"` and `\\`; a quoted value + must close on the same line, followed only by spaces or a `# comment` +- empty values, repeated keys and `${VAR}` expansion are not supported + +`--format json` takes one object whose values are non-empty strings. One malformed +entry refuses the whole input. The message gives only the line number, never the +line, and nothing is written. + +`--secret` encrypts every imported value; without it the values are stored as plain +entries in the local layer. `--service` picks one scope for all entries (default +`global`). The output and `--json` result list the added, updated and unchanged keys, +never values. + +Hydration is idempotent and all-or-nothing: + +- An entry that already holds its value is left alone: a secret keeps its ciphertext + and a plain number keeps its YAML type. When nothing changes, the file is not + rewritten, so comments and formatting survive and Git shows no change. +- A changed value gets new ciphertext with a fresh IV. +- Local entries that are not in the input, other scopes, and removal markers for other + keys are kept. A key in the input replaces that key's local entry, including a + value you set by hand with `--local`. +- The whole new layer is written to a staging file and published by one rename, so an + interrupted hydration leaves the previous layer, with every key at its old value. + +Hydration refuses, without changing the layer, when: + +- git tracks the target local layer +- a secret already in the local layer does not decrypt under the project env key, or no + key is available +- `--secret` is used, no `.hack.secret.key` exists, and `HACK_ENV_SECRET_KEY` is set + (a new key file would replace that key for every layer) +- another `hack env hydrate` for the same project is running +- the local layer changed between hydration's read and its rename, for example from a + concurrent `hack env set --local` + +Concurrent hydrations are serialized by `.hack/.internal/env-hydrate.lock`, which holds +the owner's process ID. A hydration that is killed (for example with SIGKILL) cannot +remove it; the next hydration then reports the stale lock and its path instead of taking +it over. Remove the file once no hydration is running. `hack env set --local` does not +take this lock. Hydration checks the layer again just before its rename, which narrows +the window in which such a write could be overwritten but cannot close it. + Materialize a compatibility `.hack/.env`: ```bash diff --git a/docs/reference/cli.md b/docs/reference/cli.md index dd1f96c8c..011efee21 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2205,6 +2205,7 @@ hack env [options] | `hack env apply` | Recreate one service with its resolved env | | `hack env add [key] [value]` | Add or update an env value | | `hack env set [key] [value]` | Alias for env add | +| `hack env hydrate` | Import many values into the machine-local env layer at once | | `hack env materialize` | Write a compatibility .env file from the selected overlay | | `hack env exec [command...]` | Run a host command with project env injected | | `hack env shell` | Open a host shell with project env injected | @@ -2394,6 +2395,34 @@ Same value rules as `hack env add`: pipe secrets with --value-stdin or use the h | `--help, -h` | Show help | | `--version, -v` | Show version | +## `hack env hydrate` + +Import many values into the machine-local env layer at once + +### Usage + +```bash +hack env hydrate [options] +``` + +Reads KEY=value entries from piped stdin or --file (never from arguments) and writes them, in one atomic commit, to the worktree-local env file (`hack.env.local.yaml`, or `hack.env..local.yaml` with --env). Tracked env files are never written. Repeating an import with the same values changes nothing; other local entries are kept. Values are never printed. + +### Options + +| Option | Description | +| --- | --- | +| `--path, -p ` | Run a project command against a repo path (overrides cwd search) | +| `--project ` | Target a registered project by name (from ~/.hack/projects.json) | +| `--env ` | Select an env overlay by name; omit for the configured default overlay, or use 'base' or 'default' for base layers only | +| `--json` | Output JSON (machine-readable) | +| `--secret` | Store value as an encrypted secure entry | +| `--service ` | Target scope (global or a discovered service name) | +| `--file ` | Read entries from this file (or a pipe such as <(producer)) instead of stdin | +| `--format ` | Input format: KEY=value lines (env, default) or one JSON object of string values (json) | +| `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | +| `--help, -h` | Show help | +| `--version, -v` | Show version | + ## `hack env materialize` Write a compatibility .env file from the selected overlay diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index bdc5406d0..ef39aec19 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -135,13 +135,13 @@ 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: 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. +- Disposition: Partial. Missing: Hydration replaces an imported key's local entry; it cannot tell an operator's deliberate local override of that key from a stale hydrated value. `hack env set --local` does not take the hydration lock, so only a recheck just before the rename guards against it. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: `hack env set --local --value-stdin --secret`, `hack env explain --json` +- Commands: `hack env hydrate --file --secret`, `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 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. +- Env ownership: Defaults are tracked; `hack env hydrate --secret` reads `KEY=value` entries from piped stdin or `--file` and writes only the gitignored machine-local layer, so values stay out of argv and out of tracked files. Local entries outside the input are kept. +- Readiness: A repeat import with the same values does not rewrite the layer; a changed value gets new ciphertext with a fresh IV. One rename commits all keys, so an interrupted import leaves every key at its previous value. A wrong key, a tracked local layer or a concurrent hydration refuses without changing the layer. +- Cleanup: Unchanged values cause no rewrite. A killed import can leave a staging file and a stale `.hack/.internal/env-hydrate.lock`; the next import reports the lock instead of taking it over. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `27bff041d416`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `d2a336816ac2`. 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 789138ce4..e6ae909cd 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -255,21 +255,23 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ "A repository tracks env defaults and each machine adds local values on top.", disposition: "partial", supportedCommands: [ + "hack env hydrate --file --secret", "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 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.", + "Defaults are tracked; `hack env hydrate --secret` reads `KEY=value` entries from piped stdin or `--file` and writes only the gitignored machine-local layer, so values stay out of argv and out of tracked files. Local entries outside the input are kept.", 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.", + "A repeat import with the same values does not rewrite the layer; a changed value gets new ciphertext with a fresh IV. One rename commits all keys, so an interrupted import leaves every key at its previous value. A wrong key, a tracked local layer or a concurrent hydration refuses without changing the layer.", + cleanup: + "Unchanged values cause no rewrite. A killed import can leave a staging file and a stale `.hack/.internal/env-hydrate.lock`; the next import reports the lock instead of taking it over.", 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: - "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.", + "Hydration replaces an imported key's local entry; it cannot tell an operator's deliberate local override of that key from a stale hydrated value. `hack env set --local` does not take the hydration lock, so only a recheck just before the rename guards against it.", provenance: { consumers: 1, source: "HACK-P08 research register" }, example: null, }, diff --git a/src/cli/command.ts b/src/cli/command.ts index d12b833f1..7bf585f2e 100644 --- a/src/cli/command.ts +++ b/src/cli/command.ts @@ -448,8 +448,11 @@ export function parsePositionalsForCommand< } if (idx < remaining.length) { - const extra = remaining.slice(idx).join(" "); - throw new CliUsageError(`Unexpected arguments: ${extra}`); + // Count only: an extra argument is often a pasted `KEY=value` or secret. + const extra = remaining.length - idx; + throw new CliUsageError( + `Unexpected arguments: ${extra} more than this command accepts. They are not shown because they may contain values.` + ); } return out as PositionalsValues; diff --git a/src/commands/env.ts b/src/commands/env.ts index bcef1651d..eac7edf24 100644 --- a/src/commands/env.ts +++ b/src/commands/env.ts @@ -22,6 +22,10 @@ import { resolveRegisteredProjectOption } from "../cli/project-selector.ts"; import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { updateGlobalConfig } from "../lib/config.ts"; import { resolveGlobalConfigPath } from "../lib/config-paths.ts"; +import { + parseEnvHydrateInputFormat, + readEnvHydrateInput, +} from "../lib/env-hydrate-input.ts"; import { assertEnvValueStdinUsage, readEnvValueFromStdin, @@ -69,6 +73,7 @@ import { import { discoverComposeServiceNames, ENV_KEY_INVALID_MESSAGE, + hydrateProjectEnvLocalLayer, materializeProjectEnv, migrateLegacyProjectEnv, parseProjectEnvTarget, @@ -289,6 +294,44 @@ const setSpec = defineCommand({ subcommands: [], } as const); +const optHydrateFile = defineOption({ + name: "file", + type: "string", + long: "--file", + valueHint: "", + description: + "Read entries from this file (or a pipe such as <(producer)) instead of stdin", +} as const); + +const optHydrateFormat = defineOption({ + name: "format", + type: "string", + long: "--format", + valueHint: "", + description: + "Input format: KEY=value lines (env, default) or one JSON object of string values (json)", +} as const); + +const hydrateSpec = defineCommand({ + name: "hydrate", + summary: "Import many values into the machine-local env layer at once", + description: + "Reads KEY=value entries from piped stdin or --file (never from arguments) and writes them, in one atomic commit, to the worktree-local env file (`hack.env.local.yaml`, or `hack.env..local.yaml` with --env). Tracked env files are never written. Repeating an import with the same values changes nothing; other local entries are kept. Values are never printed.", + group: "Project", + options: [ + optPath, + optProject, + optEnv, + optJson, + optSecret, + optService, + optHydrateFile, + optHydrateFormat, + ], + positionals: [], + subcommands: [], +} as const); + const materializeSpec = defineCommand({ name: "materialize", summary: "Write a compatibility .env file from the selected overlay", @@ -1919,6 +1962,81 @@ const handleEnvSet: CommandHandlerFor = async ({ return 0; }; +const handleEnvHydrate: CommandHandlerFor = async ({ + ctx, + args, +}): Promise => { + // Refuse bad options, select the project, and read and parse the whole + // input before anything is migrated or written, so a refusal stores nothing. + const format = parseEnvHydrateInputFormat(args.options.format); + const scope = parseEnvScopeOption({ scopeName: args.options.service }); + const envName = resolveRequestedEnvName({ envOption: args.options.env }); + const project = await resolveProjectForEnv({ + ctx, + pathOpt: args.options.path, + projectOpt: args.options.project, + }); + const entries = await readEnvHydrateInput({ + format, + filePath: + args.options.file === undefined + ? null + : resolve(ctx.cwd, args.options.file), + }); + + const projectName = await resolveProjectName(project); + await maybeMigrateLegacyProjectEnv({ + project, + projectName, + reason: "mutation", + }); + + const result = await hydrateProjectEnvLocalLayer({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: envName ?? null, + scope, + entries, + secret: args.options.secret === true, + }); + + if (args.options.json === true) { + process.stdout.write( + `${JSON.stringify( + { + file: result.filePath, + local: true, + scope: result.scope, + secret: args.options.secret === true, + changed: result.changed, + created_key: result.createdKey, + added: result.added, + updated: result.updated, + unchanged: result.unchanged, + }, + null, + 2 + )}\n` + ); + return 0; + } + + logger.success({ + message: [ + result.changed + ? `Hydrated ${result.filePath}` + : `No changes needed in ${result.filePath}`, + "worktree-local override", + `scope=${result.scope}`, + `${result.added.length} added, ${result.updated.length} updated, ${result.unchanged.length} unchanged`, + result.createdKey ? "generated project env key" : null, + ] + .filter((value): value is string => typeof value === "string") + .join(" • "), + }); + return 0; +}; + const handleEnvMaterialize: CommandHandlerFor = async ({ ctx, args, @@ -2512,6 +2630,7 @@ export const envCommand = defineCommand({ withHandler(applySpec, handleEnvApply), withHandler(addSpec, handleEnvAdd), withHandler(setSpec, handleEnvSet), + withHandler(hydrateSpec, handleEnvHydrate), withHandler(materializeSpec, handleEnvMaterialize), withHandler(execSpec, handleEnvExec), withHandler(shellSpec, handleEnvShell), diff --git a/src/lib/env-hydrate-input.ts b/src/lib/env-hydrate-input.ts new file mode 100644 index 000000000..8d6516f41 --- /dev/null +++ b/src/lib/env-hydrate-input.ts @@ -0,0 +1,417 @@ +import { constants, type FileHandle, open } from "node:fs/promises"; +import { ClosedCliError } from "./cli-result.ts"; +import type { EnvValueStdinSource } from "./env-value-stdin.ts"; + +/** + * Batch input for `hack env hydrate`: many `KEY=value` entries read from a + * non-TTY stdin or from a file, never from argv. + * + * Contract: + * - Input is bounded by {@link ENV_HYDRATE_INPUT_MAX_BYTES}, the size bound of + * one env layer. Reading stops as soon as the bound is exceeded. + * - A TTY stdin is refused before any byte is read. + * - Parsing is strict and all-or-nothing: one malformed line refuses the + * whole input, so a source that lost a value is never half-imported. + * - Every refusal is a closed, value-free {@link EnvHydrateInputError}. The + * message may carry a 1-based line number; it never carries input bytes, + * keys, values, the file path or an underlying error. + * + * `env` format, one entry per line (`\n` or `\r\n`): + * - Blank lines and lines whose first non-blank character is `#` are skipped. + * - An optional `export ` prefix is accepted. + * - `KEY=value`, where `KEY` matches `^[A-Z_][A-Z0-9_]*$` (spaces around the + * key are ignored). An unquoted value is the rest of the line with + * surrounding blanks removed; `#` inside it is literal. + * - `'...'` is literal. `"..."` accepts the escapes `\n`, `\r`, `\t`, `\"` and + * `\\`. A quoted value must close on the same line; only blanks or a + * `# comment` may follow it. + * - Empty values and repeated keys are refused. No `${VAR}` expansion. + * + * `json` format: one object whose keys match the key pattern and whose values + * are non-empty strings. + */ +export const ENV_HYDRATE_INPUT_MAX_BYTES = 1024 * 1024; + +export const ENV_HYDRATE_INPUT_FORMATS = ["env", "json"] as const; +export type EnvHydrateInputFormat = (typeof ENV_HYDRATE_INPUT_FORMATS)[number]; + +export const ENV_HYDRATE_INPUT_REFUSAL_REASONS = [ + "interactive_tty", + "invalid_format", + "oversize", + "read_failed", + "invalid_utf8", + "nul_byte", + "empty", + "invalid_line", + "invalid_key", + "duplicate_key", + "empty_value", + "unterminated_quote", + "invalid_escape", + "trailing_text", + "invalid_json", + "json_not_object", + "json_value_not_string", +] as const; + +export type EnvHydrateInputRefusalReason = + (typeof ENV_HYDRATE_INPUT_REFUSAL_REASONS)[number]; + +const NOTHING_WRITTEN = "Nothing was written."; +const NOT_SHOWN = "Input is not shown because it may contain values."; + +const REFUSAL_MESSAGES: Readonly> = + { + interactive_tty: + "hack env hydrate reads entries from piped stdin or --file, but stdin is an interactive terminal. Pipe the entries in or pass --file.", + invalid_format: 'Invalid --format value. Expected "env" or "json".', + oversize: `Hydration input exceeds the ${ENV_HYDRATE_INPUT_MAX_BYTES}-byte limit.`, + read_failed: + "Could not read the hydration input. The path is not shown because it may contain a value.", + invalid_utf8: "Hydration input is not valid UTF-8.", + nul_byte: + "Hydration input contains a NUL byte, which env values cannot carry.", + empty: "Hydration input contains no entries.", + invalid_line: "expected KEY=value.", + invalid_key: "keys must match ^[A-Z_][A-Z0-9_]*$.", + duplicate_key: "the key repeats an earlier entry.", + empty_value: "the value is empty.", + unterminated_quote: "the quoted value does not close on this line.", + invalid_escape: + 'unsupported escape in a double-quoted value (use \\n, \\r, \\t, \\" or \\\\).', + trailing_text: "unexpected text after the closing quote.", + invalid_json: "Hydration input is not valid JSON.", + json_not_object: + "JSON hydration input must be one object mapping keys to values.", + json_value_not_string: "JSON hydration values must be strings.", + }; + +/** Closed refusal for hydration input; the message never carries input. */ +export class EnvHydrateInputError extends ClosedCliError { + readonly reason: EnvHydrateInputRefusalReason; + /** 1-based line of an `env`-format refusal, or null. */ + readonly line: number | null; + + constructor( + reason: EnvHydrateInputRefusalReason, + line: number | null = null + ) { + const detail = REFUSAL_MESSAGES[reason]; + super( + line === null + ? `${detail} ${NOT_SHOWN} ${NOTHING_WRITTEN}` + : `Hydration input line ${line}: ${detail} ${NOT_SHOWN} ${NOTHING_WRITTEN}` + ); + this.name = "EnvHydrateInputError"; + this.reason = reason; + this.line = line; + } +} + +/** Fixed refusal for `--format`; the argument is never echoed. */ +export function parseEnvHydrateInputFormat( + format: string | undefined +): EnvHydrateInputFormat { + const requested = format?.trim() || "env"; + for (const candidate of ENV_HYDRATE_INPUT_FORMATS) { + if (candidate === requested) { + return candidate; + } + } + throw new EnvHydrateInputError("invalid_format"); +} + +const ENV_KEY_PATTERN = /^[A-Z_][A-Z0-9_]*$/; + +function processStdinSource(): EnvValueStdinSource { + return { + isTTY: process.stdin.isTTY === true, + open: () => Bun.stdin.stream(), + }; +} + +/** + * Read and parse one hydration input. + * + * @param input.filePath - Absolute path to read instead of stdin. Pipes and + * FIFOs (for example `--file <(producer)`) are accepted; directories are not. + * @param input.stdin - Stdin source; defaults to the process stdin. + * @returns Validated keys to values, in input order. + * @throws EnvHydrateInputError for every refusal (closed, value-free). + */ +export async function readEnvHydrateInput(input: { + readonly format: EnvHydrateInputFormat; + readonly filePath?: string | null; + readonly stdin?: EnvValueStdinSource; + readonly maxBytes?: number; +}): Promise> { + const maxBytes = input.maxBytes ?? ENV_HYDRATE_INPUT_MAX_BYTES; + const bytes = + typeof input.filePath === "string" + ? await readBoundedFile({ path: input.filePath, maxBytes }) + : await readBoundedStdin({ + source: input.stdin ?? processStdinSource(), + maxBytes, + }); + let text: string; + try { + text = new TextDecoder("utf-8", { fatal: true }).decode(bytes); + } catch { + throw new EnvHydrateInputError("invalid_utf8"); + } finally { + bytes.fill(0); + } + if (text.includes("\0")) { + throw new EnvHydrateInputError("nul_byte"); + } + return parseEnvHydrateInput({ text, format: input.format }); +} + +/** Parse decoded hydration input under the module contract. */ +export function parseEnvHydrateInput(input: { + readonly text: string; + readonly format: EnvHydrateInputFormat; +}): ReadonlyMap { + const entries = + input.format === "json" + ? parseJsonEntries(input.text) + : parseEnvEntries(input.text); + if (entries.size === 0) { + throw new EnvHydrateInputError("empty"); + } + return entries; +} + +function parseEnvEntries(text: string): Map { + const entries = new Map(); + const lines = text.split("\n"); + for (let index = 0; index < lines.length; index += 1) { + const lineNumber = index + 1; + const raw = lines[index] ?? ""; + const line = raw.endsWith("\r") ? raw.slice(0, -1) : raw; + const trimmed = line.trim(); + if (trimmed.length === 0 || trimmed.startsWith("#")) { + continue; + } + const body = trimmed.startsWith("export ") + ? trimmed.slice("export ".length).trimStart() + : trimmed; + const equals = body.indexOf("="); + if (equals <= 0) { + throw new EnvHydrateInputError("invalid_line", lineNumber); + } + const key = body.slice(0, equals).trim(); + if (!ENV_KEY_PATTERN.test(key)) { + throw new EnvHydrateInputError("invalid_key", lineNumber); + } + if (entries.has(key)) { + throw new EnvHydrateInputError("duplicate_key", lineNumber); + } + const value = parseEnvValue({ + raw: body.slice(equals + 1).trim(), + lineNumber, + }); + if (value.length === 0) { + throw new EnvHydrateInputError("empty_value", lineNumber); + } + entries.set(key, value); + } + return entries; +} + +function parseEnvValue(input: { + readonly raw: string; + readonly lineNumber: number; +}): string { + const quote = input.raw[0]; + if (quote === "'") { + const close = input.raw.indexOf("'", 1); + if (close < 0) { + throw new EnvHydrateInputError("unterminated_quote", input.lineNumber); + } + assertOnlyCommentAfter({ + rest: input.raw.slice(close + 1), + lineNumber: input.lineNumber, + }); + return input.raw.slice(1, close); + } + if (quote === '"') { + return parseDoubleQuoted(input); + } + return input.raw; +} + +const DOUBLE_QUOTE_ESCAPES: Readonly> = { + n: "\n", + r: "\r", + t: "\t", + '"': '"', + "\\": "\\", +}; + +function parseDoubleQuoted(input: { + readonly raw: string; + readonly lineNumber: number; +}): string { + let value = ""; + for (let index = 1; index < input.raw.length; index += 1) { + const char = input.raw[index]; + if (char === '"') { + assertOnlyCommentAfter({ + rest: input.raw.slice(index + 1), + lineNumber: input.lineNumber, + }); + return value; + } + if (char === "\\") { + const escaped = DOUBLE_QUOTE_ESCAPES[input.raw[index + 1] ?? ""]; + if (escaped === undefined) { + throw new EnvHydrateInputError("invalid_escape", input.lineNumber); + } + value += escaped; + index += 1; + continue; + } + value += char; + } + throw new EnvHydrateInputError("unterminated_quote", input.lineNumber); +} + +function assertOnlyCommentAfter(input: { + readonly rest: string; + readonly lineNumber: number; +}): void { + const rest = input.rest.trim(); + if (rest.length > 0 && !rest.startsWith("#")) { + throw new EnvHydrateInputError("trailing_text", input.lineNumber); + } +} + +function parseJsonEntries(text: string): Map { + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new EnvHydrateInputError("invalid_json"); + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new EnvHydrateInputError("json_not_object"); + } + const entries = new Map(); + for (const [key, value] of Object.entries(parsed)) { + if (!ENV_KEY_PATTERN.test(key)) { + throw new EnvHydrateInputError("invalid_key"); + } + if (typeof value !== "string") { + throw new EnvHydrateInputError("json_value_not_string"); + } + if (value.length === 0) { + throw new EnvHydrateInputError("empty_value"); + } + entries.set(key, value); + } + return entries; +} + +async function readBoundedStdin(input: { + readonly source: EnvValueStdinSource; + readonly maxBytes: number; +}): Promise { + if (input.source.isTTY) { + throw new EnvHydrateInputError("interactive_tty"); + } + const chunks: Uint8Array[] = []; + let total = 0; + let reader: ReadableStreamDefaultReader; + try { + reader = input.source.open().getReader(); + } catch { + throw new EnvHydrateInputError("read_failed"); + } + try { + while (true) { + let result: Awaited>; + try { + result = await reader.read(); + } catch { + wipe(chunks); + throw new EnvHydrateInputError("read_failed"); + } + if (result.done) { + break; + } + total += result.value.byteLength; + chunks.push(result.value); + if (total > input.maxBytes) { + wipe(chunks); + await reader.cancel().catch(() => undefined); + throw new EnvHydrateInputError("oversize"); + } + } + } finally { + reader.releaseLock(); + } + return concat(chunks, total); +} + +async function readBoundedFile(input: { + readonly path: string; + readonly maxBytes: number; +}): Promise { + let file: FileHandle; + try { + file = await open(input.path, constants.O_RDONLY); + } catch { + throw new EnvHydrateInputError("read_failed"); + } + const chunks: Uint8Array[] = []; + let total = 0; + try { + const stats = await file.stat().catch(() => null); + if (stats === null || stats.isDirectory()) { + throw new EnvHydrateInputError("read_failed"); + } + while (true) { + const chunk = new Uint8Array(64 * 1024); + let bytesRead: number; + try { + ({ bytesRead } = await file.read(chunk, 0, chunk.byteLength, null)); + } catch { + chunk.fill(0); + wipe(chunks); + throw new EnvHydrateInputError("read_failed"); + } + if (bytesRead === 0) { + break; + } + total += bytesRead; + chunks.push(chunk.subarray(0, bytesRead)); + if (total > input.maxBytes) { + chunk.fill(0); + wipe(chunks); + throw new EnvHydrateInputError("oversize"); + } + } + } finally { + await file.close().catch(() => undefined); + } + return concat(chunks, total); +} + +function concat(chunks: readonly Uint8Array[], total: number): Uint8Array { + const out = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + out.set(chunk, offset); + offset += chunk.byteLength; + } + wipe(chunks); + return out; +} + +function wipe(chunks: readonly Uint8Array[]): void { + for (const chunk of chunks) { + chunk.fill(0); + } +} diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 96558b275..4224aec98 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -101,6 +101,14 @@ export async function writeTextFileIfChanged( return { changed: true }; } +/** A compare-and-swap write found the file changed by another writer. */ +export class AtomicWriteConflictError extends Error { + constructor() { + super("The file changed while it was being replaced; nothing was written."); + this.name = "AtomicWriteConflictError"; + } +} + /** * Replaces `absolutePath` with `content` only when the text differs, publishing * the new contents atomically. @@ -119,12 +127,24 @@ export async function writeTextFileIfChanged( * 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. + * + * With `expectedExisting`, the write is a compare-and-swap: the current text + * (null when absent) must equal it both before staging and again after the + * staging file is fsynced, immediately before the rename; otherwise the + * staging file is removed and {@link AtomicWriteConflictError} is thrown. The + * second check narrows, but cannot close, the window in which a writer that + * does not use this check can still be overwritten. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, - content: string + content: string, + options: { readonly expectedExisting?: string | null } = {} ): Promise<{ readonly changed: boolean }> { const existing = await readTextFile(absolutePath); + const expectsExisting = options.expectedExisting !== undefined; + if (expectsExisting && existing !== options.expectedExisting) { + throw new AtomicWriteConflictError(); + } if (existing === content) { return { changed: false }; } @@ -150,6 +170,12 @@ export async function writeTextFileAtomicIfChanged( } await file.writeFile(content, "utf8"); await file.sync(); + if ( + expectsExisting && + (await readTextFile(absolutePath)) !== options.expectedExisting + ) { + throw new AtomicWriteConflictError(); + } await rename(staging, target); committed = true; } finally { diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 2f533b849..566272aa7 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -4,7 +4,7 @@ import { createHash, randomBytes, } from "node:crypto"; -import { lstat, readdir, rm, stat } from "node:fs/promises"; +import { lstat, readdir, rm, stat, unlink } from "node:fs/promises"; import { basename, dirname, relative, resolve } from "node:path"; import { YAML } from "bun"; import { @@ -25,8 +25,10 @@ import { HACK_DIR_GITIGNORE_END_MARKER, HACK_DIR_GITIGNORE_ENTRIES, } from "../templates.ts"; +import { ClosedCliError } from "./cli-result.ts"; import { parseDotEnv, serializeDotEnv } from "./env.ts"; import { + AtomicWriteConflictError, ensureDir, ensureGitignoreEntry, ensureManagedGitignoreBlock, @@ -649,6 +651,19 @@ async function readProjectEnvConfigFile(opts: { readonly strictRead?: boolean; }): Promise { const text = await readProjectEnvLayerText(opts); + return parseProjectEnvConfigText({ + path: opts.path, + environment: opts.environment, + text, + }); +} + +function parseProjectEnvConfigText(opts: { + readonly path: string; + readonly environment: string; + readonly text: string | null; +}): ProjectEnvConfigReadResult { + const text = opts.text; if (text === null) { return { path: opts.path, @@ -2470,15 +2485,506 @@ export async function unsetProjectEnvValue(opts: { return { changed, filePath }; } +/** Closed reasons `hydrateProjectEnvLocalLayer` refuses with; none writes a layer. */ +export const PROJECT_ENV_HYDRATION_REFUSAL_REASONS = [ + "tracked_layer", + "locked", + "stale_lock", + "unreadable_layer", + "wrong_key", + "key_env_only", + "concurrent_write", +] as const; + +export type ProjectEnvHydrationRefusalReason = + (typeof PROJECT_ENV_HYDRATION_REFUSAL_REASONS)[number]; + +const PROJECT_ENV_HYDRATION_LOCK_FILENAME = "env-hydrate.lock"; +const PROJECT_ENV_HYDRATION_NOTHING_WRITTEN = "No env layer was changed."; + +function projectEnvHydrationRefusalMessage(opts: { + readonly reason: ProjectEnvHydrationRefusalReason; + readonly lockPath: string; +}): string { + switch (opts.reason) { + case "tracked_layer": + return "Refusing to hydrate: git tracks the machine-local env layer, so hydrated values would be committed. Untrack it with `git rm --cached` and retry."; + case "locked": + return "Refusing to hydrate: another `hack env hydrate` is writing this project's machine-local env layer. Retry when it finishes."; + case "stale_lock": + return `Refusing to hydrate: an interrupted \`hack env hydrate\` left its lock at ${opts.lockPath}, and the process that held it has exited. Remove that file and retry.`; + case "unreadable_layer": + return "Refusing to hydrate: the machine-local env layer is not a readable, valid env file. Fix or remove it and retry."; + case "wrong_key": + return `Refusing to hydrate: a secret already in the machine-local env layer does not decrypt under the project env key, or no key is available. Restore the matching ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "key_env_only": + return `Refusing to hydrate secrets: ${PROJECT_ENV_SECRET_KEY_ENV} is set but no ${PROJECT_ENV_KEY_FILENAME} exists, and creating one would replace that key for every layer. Store the same key in ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "concurrent_write": + return "Refusing to hydrate: the machine-local env layer changed while hydration was preparing it. Re-run hydration."; + default: + return "Refusing to hydrate."; + } +} + +/** + * Closed, value-free refusal from `hydrateProjectEnvLocalLayer`. The message + * names no key, value or input line; it may name Hack's own lock path. + */ +export class ProjectEnvHydrationRefusalError extends ClosedCliError { + readonly reason: ProjectEnvHydrationRefusalReason; + + constructor(opts: { + readonly reason: ProjectEnvHydrationRefusalReason; + readonly lockPath: string; + }) { + super( + `${projectEnvHydrationRefusalMessage(opts)} ${PROJECT_ENV_HYDRATION_NOTHING_WRITTEN}` + ); + this.name = "ProjectEnvHydrationRefusalError"; + this.reason = opts.reason; + } +} + +export type ProjectEnvHydrationResult = { + readonly filePath: string; + readonly scope: string; + readonly changed: boolean; + readonly createdKey: boolean; + /** Imported keys that the layer's scope did not hold. Sorted. */ + readonly added: readonly string[]; + /** Imported keys whose stored entry was replaced. Sorted. */ + readonly updated: readonly string[]; + /** Imported keys whose stored entry already held the value. Sorted. */ + readonly unchanged: readonly string[]; +}; + +/** + * Writes several values into the machine-local env layer in one commit. + * + * Contract: + * - Only the effective machine-local layer for `envName` is written + * (`hack.env.local.yaml` or `hack.env..local.yaml`); a layer that + * git tracks is refused. Tracked defaults are never read for writing. + * - Keys outside `entries`, other scopes and removal markers for other keys + * are kept as they are. An imported key replaces that key's local entry. + * - Idempotent: an entry that already holds its value is kept unchanged (a + * secret keeps its ciphertext, a scalar keeps its YAML type). When no entry + * changes, the file is not rewritten at all. A changed or newly secret value + * gets new ciphertext with a fresh IV. + * - All-or-nothing: the whole new layer is published by one rename. Every + * refusal ({@link ProjectEnvHydrationRefusalError}) happens before that + * rename, so the previous layer stays in place. + * - Concurrency: hydrations of one project are serialized by an exclusive + * lock file under `.internal/`; a second hydration refuses instead of + * waiting. The write is also a compare-and-swap against the bytes read, so a + * different writer that changed the layer meanwhile makes hydration refuse + * rather than overwrite it. A lock left by a killed process is reported, not + * removed. + * - Wrong key: every secret already in the layer must decrypt under the key + * readers would use, or hydration refuses. Secret mode encrypts with the key + * file `hack env add --secret` uses and refuses to create one while only + * `HACK_ENV_SECRET_KEY` provides a key. + * + * Values never appear in errors or results; results name only keys. + */ +export async function hydrateProjectEnvLocalLayer(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + /** Validated keys to plaintext values. */ + readonly entries: ReadonlyMap; + readonly secret: boolean; +}): Promise { + await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); + await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); + for (const key of opts.entries.keys()) { + if (!PROJECT_ENV_KEY_PATTERN.test(key)) { + throw new Error(ENV_KEY_INVALID_MESSAGE); + } + } + const scope = assertValidProjectEnvScopeName({ scopeName: opts.scope }); + const filePath = await resolveProjectEnvEffectiveLocalConfigPath({ + projectRoot: opts.projectRoot, + projectDir: opts.projectDir, + envName: opts.envName, + }); + const lockPath = resolve( + opts.projectDir, + ".internal", + PROJECT_ENV_HYDRATION_LOCK_FILENAME + ); + const refuse = (reason: ProjectEnvHydrationRefusalReason) => + new ProjectEnvHydrationRefusalError({ reason, lockPath }); + + const tracked = await isGitTrackedProjectEnvPath({ + projectRoot: opts.projectRoot, + path: filePath, + }); + if (tracked === true) { + throw refuse("tracked_layer"); + } + + const release = await acquireProjectEnvHydrationLock({ + projectDir: opts.projectDir, + lockPath, + refuse, + }); + try { + return await hydrateLockedProjectEnvLayer({ + ...opts, + scope, + filePath, + refuse, + }); + } finally { + await release(); + } +} + +type ProjectEnvHydrationRefuse = ( + reason: ProjectEnvHydrationRefusalReason +) => ProjectEnvHydrationRefusalError; + +async function hydrateLockedProjectEnvLayer(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + readonly entries: ReadonlyMap; + readonly secret: boolean; + readonly filePath: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise { + const environment = opts.envName ?? "default"; + const layer = await readProjectEnvHydrationLayer({ + filePath: opts.filePath, + environment, + refuse: opts.refuse, + }); + const key = await resolveProjectEnvHydrationKey({ + projectRoot: opts.projectRoot, + config: layer.config, + secret: opts.secret, + refuse: opts.refuse, + }); + const plan = planProjectEnvHydration({ + previousScope: layer.config.values[opts.scope] ?? {}, + entries: opts.entries, + keyText: key.keyText, + }); + const result = { + filePath: opts.filePath, + scope: opts.scope, + createdKey: key.createdKey, + added: plan.added, + updated: plan.updated, + unchanged: plan.unchanged, + }; + if (plan.added.length === 0 && plan.updated.length === 0) { + // Nothing to change: leave the layer's bytes, including hand edits, alone. + return { ...result, changed: false }; + } + + const nextValues: ProjectEnvValuesByScope = { + ...layer.config.values, + [opts.scope]: plan.nextScope, + }; + if (!("global" in nextValues)) { + nextValues.global = {}; + } + await ensureHackDirGitignore({ projectDir: opts.projectDir }); + try { + await writeProjectEnvConfigFile({ + path: opts.filePath, + config: { + version: PROJECT_ENV_CONFIG_VERSION, + environment, + secretsprovider: PROJECT_ENV_SECRETS_PROVIDER, + values: nextValues, + }, + expectedExisting: layer.text, + }); + } catch (error: unknown) { + if (error instanceof AtomicWriteConflictError) { + throw opts.refuse("concurrent_write"); + } + throw error; + } + return { ...result, changed: true }; +} + +/** The layer's exact bytes (null when absent) and its parsed config. */ +async function readProjectEnvHydrationLayer(opts: { + readonly filePath: string; + readonly environment: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<{ + readonly text: string | null; + readonly config: ProjectEnvConfig; +}> { + let text: string | null; + try { + text = await readProjectEnvLayerText({ + path: opts.filePath, + strictRead: true, + }); + } catch { + throw opts.refuse("unreadable_layer"); + } + const read = parseProjectEnvConfigText({ + path: opts.filePath, + environment: opts.environment, + text, + }); + if (read.parseError !== undefined) { + throw opts.refuse("unreadable_layer"); + } + return { text, config: read.config }; +} + +/** + * Checks every secret already in the layer against the key readers use and, + * in secret mode, selects (or first creates) the encryption key. + */ +async function resolveProjectEnvHydrationKey(opts: { + readonly projectRoot: string; + readonly config: ProjectEnvConfig; + readonly secret: boolean; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<{ readonly keyText: string | null; readonly createdKey: boolean }> { + const fileKey = await readExistingProjectEnvKeyFileText({ + projectRoot: opts.projectRoot, + }); + const environmentKey = + process.env[PROJECT_ENV_SECRET_KEY_ENV]?.trim() || null; + // The same priority `resolveProjectEnvKey` gives readers of this layer. + const readerKey = fileKey ?? environmentKey; + const existingSecrets = Object.values(opts.config.values).flatMap( + (scopeValues) => + Object.values(scopeValues).filter( + (value): value is ProjectEnvSecretValue => + value !== null && isProjectEnvSecretValue(value) + ) + ); + const opensAll = + readerKey !== null && + existingSecrets.every((secret) => + canDecryptProjectEnvValue({ + ciphertext: secret.secure, + keyText: readerKey, + }) + ); + if (existingSecrets.length > 0 && !opensAll) { + throw opts.refuse("wrong_key"); + } + if (!opts.secret) { + return { keyText: null, createdKey: false }; + } + if (fileKey === null && readerKey !== null) { + throw opts.refuse("key_env_only"); + } + const ensured = await ensureProjectEnvSecretKey({ + projectRoot: opts.projectRoot, + }); + if (readerKey !== null && ensured.keyText !== readerKey) { + throw opts.refuse("wrong_key"); + } + return { keyText: ensured.keyText, createdKey: ensured.created }; +} + +/** Pure merge of imported entries into one scope; keys are reported sorted. */ +function planProjectEnvHydration(opts: { + readonly previousScope: Readonly>; + readonly entries: ReadonlyMap; + /** Encrypts every imported value when set; stores plain values when null. */ + readonly keyText: string | null; +}) { + const nextScope: Record = { + ...opts.previousScope, + }; + const added: string[] = []; + const updated: string[] = []; + const unchanged: string[] = []; + const keys = [...opts.entries.keys()].sort((left, right) => + left.localeCompare(right) + ); + for (const key of keys) { + const plaintext = opts.entries.get(key) ?? ""; + const previous = Object.hasOwn(opts.previousScope, key) + ? opts.previousScope[key] + : undefined; + const stored: ProjectEnvStoredValue = + opts.keyText === null + ? stableProjectEnvPlainValue({ previous, plaintext }) + : stableProjectEnvSecretValue({ + previous, + plaintext, + keyText: opts.keyText, + }); + if (sameProjectEnvStoredValue(previous, stored)) { + unchanged.push(key); + continue; + } + (previous === undefined ? added : updated).push(key); + nextScope[key] = stored; + } + return { nextScope, added, updated, unchanged }; +} + +/** A plain scalar that already prints as `plaintext` keeps its YAML type. */ +function stableProjectEnvPlainValue(opts: { + readonly previous: ProjectEnvStoredValue | undefined; + readonly plaintext: string; +}): ProjectEnvStoredValue { + const previous = opts.previous; + if ( + (typeof previous === "string" || + typeof previous === "number" || + typeof previous === "boolean") && + String(previous) === opts.plaintext + ) { + return previous; + } + return opts.plaintext; +} + +function sameProjectEnvStoredValue( + previous: ProjectEnvStoredValue | undefined, + next: ProjectEnvStoredValue +): boolean { + if (previous === undefined || previous === null || next === null) { + return previous === next; + } + if (isProjectEnvSecretValue(previous) || isProjectEnvSecretValue(next)) { + return ( + isProjectEnvSecretValue(previous) && + isProjectEnvSecretValue(next) && + previous.secure === next.secure + ); + } + return previous === next; +} + +function canDecryptProjectEnvValue(opts: { + readonly ciphertext: string; + readonly keyText: string; +}): boolean { + try { + decryptProjectEnvValue(opts); + return true; + } catch { + return false; + } +} + +/** + * The key file `ensureProjectEnvSecretKey` would select (checkout-local, + * existing shared, then the primary checkout's), read without creating, + * copying or adopting anything. + */ +async function readExistingProjectEnvKeyFileText(opts: { + readonly projectRoot: string; +}): Promise { + const localKey = await readLegacyProjectEnvKeyFile({ + path: resolveProjectEnvKeyPath({ projectRoot: opts.projectRoot }), + }); + if (localKey !== null) { + return localKey; + } + const sharedKeyPath = await resolveProjectEnvSharedKeyPath({ + projectRoot: opts.projectRoot, + }); + const sharedKey = + sharedKeyPath === null + ? null + : await readLegacyProjectEnvKeyFile({ path: sharedKeyPath }); + if (sharedKey !== null) { + return sharedKey; + } + const inherited = await resolveInheritedProjectEnvKey({ + projectRoot: opts.projectRoot, + }); + return inherited?.keyText ?? null; +} + +/** + * Takes the project's hydration lock: `.internal/env-hydrate.lock`, created + * exclusively (no symlink follow, mode 0600) and holding the owner's PID. + * + * An existing lock is never removed or taken over, because a second process + * cannot prove the first one is gone without racing a third. It refuses as + * `locked` while the recorded PID is alive (or unreadable, which includes a + * lock still being written) and as `stale_lock` once that process has exited. + * The returned release removes the lock only while the path still names the + * file this call created. + */ +async function acquireProjectEnvHydrationLock(opts: { + readonly projectDir: string; + readonly lockPath: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<() => Promise> { + await ensureDir(dirname(opts.lockPath)); + await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); + try { + await createPrivateFileExclusive({ + path: opts.lockPath, + text: `${process.pid}\n`, + }); + } catch (error: unknown) { + if (!(isRecord(error) && error.code === "EEXIST")) { + throw error; + } + const holder = await readTextFile(opts.lockPath); + const pid = Number.parseInt(holder?.trim() ?? "", 10); + throw opts.refuse( + Number.isSafeInteger(pid) && pid > 0 && !isProcessAlive(pid) + ? "stale_lock" + : "locked" + ); + } + const created = await lstat(opts.lockPath); + return async () => { + const current = await lstat(opts.lockPath).catch(() => null); + if ( + current?.isFile() && + current.dev === created.dev && + current.ino === created.ino + ) { + await unlink(opts.lockPath).catch(() => undefined); + } + }; +} + +/** Signal 0 probes existence only; EPERM means a live process of another user. */ +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error: unknown) { + return isRecord(error) && error.code === "EPERM"; + } +} + async function writeProjectEnvConfigFile(opts: { readonly path: string; readonly config: ProjectEnvConfig; + /** Compare-and-swap guard; see `writeTextFileAtomicIfChanged`. */ + readonly expectedExisting?: string | null; }): Promise { const yaml = YAML.stringify(opts.config, null, 2); const text = yaml.endsWith("\n") ? yaml : `${yaml}\n`; await assertLegacyProjectDirectory({ projectDir: dirname(opts.path) }); // Rename-committed: an interrupted mutation leaves the previous layer intact. - return (await writeTextFileAtomicIfChanged(opts.path, text)).changed; + return ( + await writeTextFileAtomicIfChanged( + opts.path, + text, + opts.expectedExisting === undefined + ? {} + : { expectedExisting: opts.expectedExisting } + ) + ).changed; } export async function materializeProjectEnv(opts: { diff --git a/tests/cli-command.test.ts b/tests/cli-command.test.ts index e63a10ae4..32ef8c017 100644 --- a/tests/cli-command.test.ts +++ b/tests/cli-command.test.ts @@ -48,6 +48,15 @@ test("parsePositionalsForCommand throws on extra args", () => { ).toThrow("Unexpected arguments"); }); +test("parsePositionalsForCommand never echoes the extra arguments", () => { + // An extra argument is often a pasted `KEY=value`; only the count is shown. + expect(() => + parsePositionalsForCommand([], ["API_TOKEN=synthetic-extra-4c2d", "more"]) + ).toThrow( + "Unexpected arguments: 2 more than this command accepts. They are not shown because they may contain values." + ); +}); + test("parseOptionsForCommand converts number options", () => { const opts = [ { diff --git a/tests/env-hydrate-input.test.ts b/tests/env-hydrate-input.test.ts new file mode 100644 index 000000000..2eee54328 --- /dev/null +++ b/tests/env-hydrate-input.test.ts @@ -0,0 +1,233 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + EnvHydrateInputError, + type EnvHydrateInputRefusalReason, + parseEnvHydrateInput, + parseEnvHydrateInputFormat, + readEnvHydrateInput, +} from "../src/lib/env-hydrate-input.ts"; + +/** + * HACK-1158: hydration input is parsed strictly and all-or-nothing, and every + * refusal is value-free. CANARY is a synthetic string that must never reach + * an error message. + */ + +const CANARY = "synthetic-canary-7d41"; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const dir of tempDirs) { + await rm(dir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +function entriesOf(text: string, format: "env" | "json" = "env") { + return Object.fromEntries(parseEnvHydrateInput({ text, format })); +} + +function refusal(run: () => unknown): { + reason: EnvHydrateInputRefusalReason; + line: number | null; +} { + try { + run(); + } catch (error: unknown) { + if (!(error instanceof EnvHydrateInputError)) { + throw error; + } + expect(error.message).not.toContain(CANARY); + expect(error.message).toContain("Nothing was written."); + return { reason: error.reason, line: error.line }; + } + throw new Error("Expected a hydration input refusal"); +} + +async function asyncRefusal( + run: () => Promise +): Promise { + try { + await run(); + } catch (error: unknown) { + if (!(error instanceof EnvHydrateInputError)) { + throw error; + } + expect(error.message).not.toContain(CANARY); + return error.reason; + } + throw new Error("Expected a hydration input refusal"); +} + +function stdinOf(bytes: Uint8Array, isTTY = false) { + return { + isTTY, + open: () => + new ReadableStream({ + start(controller) { + controller.enqueue(bytes); + controller.close(); + }, + }), + }; +} + +test("env format accepts comments, export, quotes and literal #", () => { + expect( + entriesOf( + [ + "# source: synthetic fixture", + "", + "API_TOKEN=plain-value", + " export DB_PASS = 'single # quoted' # trailing comment", + 'MULTI="line\\none\\t\\"q\\" \\\\"', + "HASH=abc#def", + "SPACED= inner value ", + "CRLF=value\r", + ].join("\n") + ) + ).toEqual({ + API_TOKEN: "plain-value", + DB_PASS: "single # quoted", + MULTI: 'line\none\t"q" \\', + HASH: "abc#def", + SPACED: "inner value", + CRLF: "value", + }); +}); + +test("json format accepts one object of string values", () => { + expect( + entriesOf(JSON.stringify({ API_TOKEN: CANARY, DB_PASS: "b" }), "json") + ).toEqual({ API_TOKEN: CANARY, DB_PASS: "b" }); +}); + +test("each malformed env line refuses the whole input with its line number", () => { + const cases: readonly [string, EnvHydrateInputRefusalReason, number][] = [ + [`OK=1\n${CANARY}`, "invalid_line", 2], + [`OK=1\n=${CANARY}`, "invalid_line", 2], + [`lower_${CANARY}=x`, "invalid_key", 1], + [`API_TOKEN=${CANARY}\nAPI_TOKEN=${CANARY}`, "duplicate_key", 2], + ["API_TOKEN=", "empty_value", 1], + ["API_TOKEN=''", "empty_value", 1], + [`API_TOKEN='${CANARY}`, "unterminated_quote", 1], + [`API_TOKEN="${CANARY}\nrest"`, "unterminated_quote", 1], + [`API_TOKEN="${CANARY}\\x"`, "invalid_escape", 1], + [`API_TOKEN='${CANARY}' ${CANARY}`, "trailing_text", 1], + ]; + for (const [text, reason, line] of cases) { + expect(refusal(() => entriesOf(text))).toEqual({ reason, line }); + } +}); + +test("empty input and malformed json are refused without echo", () => { + expect(refusal(() => entriesOf("# only comments\n\n")).reason).toBe("empty"); + const jsonCases: readonly [string, EnvHydrateInputRefusalReason][] = [ + [`{"API_TOKEN": ${CANARY}}`, "invalid_json"], + [`["${CANARY}"]`, "json_not_object"], + [`"${CANARY}"`, "json_not_object"], + [`{"bad-${CANARY}": "x"}`, "invalid_key"], + ['{"API_TOKEN": 7}', "json_value_not_string"], + ['{"API_TOKEN": ""}', "empty_value"], + ["{}", "empty"], + ]; + for (const [text, reason] of jsonCases) { + expect(refusal(() => entriesOf(text, "json")).reason).toBe(reason); + } +}); + +test("--format accepts only env and json and never echoes the argument", () => { + expect(parseEnvHydrateInputFormat(undefined)).toBe("env"); + expect(parseEnvHydrateInputFormat("json")).toBe("json"); + expect(refusal(() => parseEnvHydrateInputFormat(CANARY)).reason).toBe( + "invalid_format" + ); +}); + +test("stdin input is bounded and refuses TTY, NUL and invalid UTF-8", async () => { + const encoder = new TextEncoder(); + expect( + Object.fromEntries( + await readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}\n`)), + }) + ) + ).toEqual({ API_TOKEN: CANARY }); + let opened = false; + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: { + isTTY: true, + open: () => { + opened = true; + throw new Error("must not read a TTY"); + }, + }, + }) + ) + ).toBe("interactive_tty"); + expect(opened).toBe(false); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}`)), + maxBytes: 8, + }) + ) + ).toBe("oversize"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}\0`)), + }) + ) + ).toBe("nul_byte"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(new Uint8Array([0x41, 0x3d, 0xff, 0xfe])), + }) + ) + ).toBe("invalid_utf8"); +}); + +test("file input reads regular files and refuses directories and missing paths", async () => { + const dir = await mkdtemp(join(tmpdir(), "hack-hydrate-input-")); + tempDirs.add(dir); + const file = join(dir, "entries.env"); + await writeFile(file, `API_TOKEN=${CANARY}\n`); + expect( + Object.fromEntries( + await readEnvHydrateInput({ format: "env", filePath: file }) + ) + ).toEqual({ API_TOKEN: CANARY }); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ format: "env", filePath: dir }) + ) + ).toBe("read_failed"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + filePath: join(dir, `missing-${CANARY}`), + }) + ) + ).toBe("read_failed"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ format: "env", filePath: file, maxBytes: 4 }) + ) + ).toBe("oversize"); +}); diff --git a/tests/fixtures/project-env-hydrate-worker.ts b/tests/fixtures/project-env-hydrate-worker.ts new file mode 100644 index 000000000..a8cfd35b2 --- /dev/null +++ b/tests/fixtures/project-env-hydrate-worker.ts @@ -0,0 +1,84 @@ +import { mock } from "bun:test"; +import { existsSync } from "node:fs"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `hydrateProjectEnvLocalLayer` secret import in a disposable child + * and pauses it at a chosen point of the layer write. Everything before the + * pause runs unmocked. + * + * - `rename`: pause at the commit rename, after the staging file is written + * and fsynced. The parent SIGKILLs the child to test a real interruption. + * - `staging`: pause when the staging file is opened, before the + * compare-and-swap recheck, until `/go` exists. The parent + * changes the layer meanwhile to test a concurrent writer. + * + * Entries arrive on stdin as `KEY=value` lines of synthetic test strings, + * never in argv. The child prints `prepared`, then `committed` or + * `refused:`. + */ +const [projectRoot, projectDir, pauseAt] = process.argv.slice(2); +if ( + !( + projectRoot && + projectDir && + (pauseAt === "rename" || pauseAt === "staging") + ) +) { + throw new Error("Missing project env hydrate worker arguments"); +} + +const rename = fs.rename; +const open = fs.open; +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async (from: string, to: string) => { + if (pauseAt === "rename" && from.endsWith(".tmp")) { + 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); + }, + open: async (...args: Parameters) => { + const [path] = args; + if ( + pauseAt === "staging" && + typeof path === "string" && + path.endsWith(".tmp") + ) { + process.stdout.write("prepared\n"); + while (!existsSync(`${projectDir}/go`)) { + await Bun.sleep(10); + } + } + return await open(...args); + }, +})); + +const entries = new Map(); +for (const line of (await Bun.stdin.text()).split("\n")) { + const equals = line.indexOf("="); + if (equals > 0) { + entries.set(line.slice(0, equals), line.slice(equals + 1)); + } +} + +const { hydrateProjectEnvLocalLayer, ProjectEnvHydrationRefusalError } = + await import("../../src/lib/project-env-config.ts"); +try { + await hydrateProjectEnvLocalLayer({ + projectRoot, + projectDir, + envName: null, + scope: "global", + entries, + secret: true, + }); + process.stdout.write("committed\n"); +} catch (error: unknown) { + if (!(error instanceof ProjectEnvHydrationRefusalError)) { + throw error; + } + process.stdout.write(`refused:${error.reason}\n`); +} diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts new file mode 100644 index 000000000..34a390952 --- /dev/null +++ b/tests/project-env-hydrate.test.ts @@ -0,0 +1,692 @@ +import { afterEach, expect, test } from "bun:test"; +import { + mkdir, + mkdtemp, + readdir, + readFile, + rm, + 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 { + AtomicWriteConflictError, + writeTextFileAtomicIfChanged, +} from "../src/lib/fs.ts"; +import { + ensureHackDirGitignore, + hydrateProjectEnvLocalLayer, + ProjectEnvHydrationRefusalError, + type ProjectEnvHydrationRefusalReason, + resolveProjectEnvConfig, + resolveProjectEnvConfigPath, + resolveProjectEnvLocalConfigPath, + setProjectEnvValue, +} from "../src/lib/project-env-config.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1158: `hack env hydrate` writes many values into the machine-local env + * layer in one commit. Repeats are byte-identical, changed values rotate with + * a fresh IV, other local entries survive, and a wrong key, an interrupted + * write or a concurrent writer leaves the previous layer in place. 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-hydrate-worker.ts" +); +const TOKEN = "synthetic-token-3b9e"; +const PASSWORD = "synthetic-password-c51a"; +const CHANGED = "synthetic-changed-80f2"; +const CANARIES = [TOKEN, PASSWORD, CHANGED] as const; + +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 trackedFile: string; + readonly localFile: string; +}; + +async function createRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-hydrate-")); + 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-hydrate", dev_host: "env-hydrate.hack" }, null, 2)}\n` + ); + await writeFile( + resolve(projectRoot, ".gitignore"), + `${PROJECT_ENV_KEY_FILENAME}\n` + ); + return { + root, + projectRoot, + projectDir, + trackedFile: resolveProjectEnvConfigPath({ projectDir, envName: null }), + localFile: resolveProjectEnvLocalConfigPath({ projectDir, envName: null }), + }; +} + +async function hydrate( + repo: Repo, + entries: Record, + secret = true, + envName: string | null = null +) { + return await hydrateProjectEnvLocalLayer({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName, + scope: "global", + entries: new Map(Object.entries(entries)), + secret, + }); +} + +async function refusalOf( + run: () => Promise +): Promise { + try { + await run(); + } catch (error: unknown) { + if (!(error instanceof ProjectEnvHydrationRefusalError)) { + throw error; + } + for (const canary of CANARIES) { + expect(error.message).not.toContain(canary); + } + expect(error.message).toContain("No env layer was changed."); + return error.reason; + } + throw new Error("Expected a hydration refusal"); +} + +async function resolvedGlobal(repo: Repo): Promise> { + const resolved = await resolveProjectEnvConfig({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + serviceNames: ["api"], + }); + return { ...(resolved?.globalEnv ?? {}) }; +} + +type LayerValues = Record>; + +function layerValues(text: string): LayerValues { + const parsed: unknown = YAML.parse(text); + return (parsed as { values: LayerValues }).values; +} + +function ciphertextOf(text: string, key: string): string { + const entry = layerValues(text).global?.[key] as + | { secure?: unknown } + | undefined; + if (typeof entry?.secure !== "string") { + throw new Error("Expected a stored secure value"); + } + return entry.secure; +} + +/** v1 values are `v1:::`. */ +function ivSegment(ciphertext: string): string { + return ciphertext.split(":")[1] ?? ""; +} + +async function stagingFiles(repo: Repo): Promise { + return (await readdir(repo.projectDir)).filter((name) => + name.endsWith(".tmp") + ); +} + +function lockPath(repo: Repo): string { + return resolve(repo.projectDir, ".internal", "env-hydrate.lock"); +} + +async function lockExists(repo: Repo): Promise { + return await Bun.file(lockPath(repo)).exists(); +} + +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", + ]); +} + +/** A git repo with a committed tracked default and the managed ignore file. */ +async function createTrackedRepo(): Promise { + const repo = await createRepo(); + await git(repo, ["init", "-q", "-b", "main"]); + await ensureHackDirGitignore({ projectDir: repo.projectDir }); + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "TRACKED_DEFAULT", + value: "tracked-default", + secret: false, + }); + await commitAll(repo); + return repo; +} + +test("a repeated import leaves the local layer and the tracked tree byte-identical", async () => { + const repo = await createTrackedRepo(); + const tracked = await readFile(repo.trackedFile); + + const first = await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + expect(first).toMatchObject({ + changed: true, + createdKey: true, + added: ["API_TOKEN", "DB_PASS"], + updated: [], + unchanged: [], + }); + const hydrated = await readFile(repo.localFile); + expect(hydrated.toString("utf8")).not.toContain(TOKEN); + + for (let attempt = 0; attempt < 3; attempt += 1) { + const repeat = await hydrate(repo, { DB_PASS: PASSWORD, API_TOKEN: TOKEN }); + expect(repeat).toMatchObject({ + changed: false, + createdKey: false, + added: [], + updated: [], + unchanged: ["API_TOKEN", "DB_PASS"], + }); + } + expect((await readFile(repo.localFile)).equals(hydrated)).toBe(true); + expect((await readFile(repo.trackedFile)).equals(tracked)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + expect(await resolvedGlobal(repo)).toMatchObject({ + TRACKED_DEFAULT: "tracked-default", + API_TOKEN: TOKEN, + DB_PASS: PASSWORD, + }); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("a changed value gets a fresh IV while unchanged entries keep their ciphertext", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + const before = await readFile(repo.localFile, "utf8"); + + const result = await hydrate(repo, { API_TOKEN: CHANGED, DB_PASS: PASSWORD }); + expect(result).toMatchObject({ + changed: true, + added: [], + updated: ["API_TOKEN"], + unchanged: ["DB_PASS"], + }); + const after = await readFile(repo.localFile, "utf8"); + expect(ciphertextOf(after, "API_TOKEN")).not.toBe( + ciphertextOf(before, "API_TOKEN") + ); + expect(ivSegment(ciphertextOf(after, "API_TOKEN"))).not.toBe( + ivSegment(ciphertextOf(before, "API_TOKEN")) + ); + expect(ciphertextOf(after, "DB_PASS")).toBe(ciphertextOf(before, "DB_PASS")); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); +}); + +test("other local entries, scopes, removal markers and scalar types survive", async () => { + const repo = await createTrackedRepo(); + const tracked = await readFile(repo.trackedFile); + await writeFile( + repo.localFile, + [ + "version: 1", + "environment: default", + "secretsprovider: project_key", + "values:", + " global:", + " # operator note: hand-edited", + " OPERATOR_OVERRIDE: operator-value", + " PORT: 3000", + " TRACKED_DEFAULT: null", + " api:", + " SCOPED: scoped-value", + "", + ].join("\n") + ); + const handEdited = await readFile(repo.localFile); + + // An import that changes nothing does not rewrite (or reformat) the layer. + const noop = await hydrate( + repo, + { OPERATOR_OVERRIDE: "operator-value", PORT: "3000" }, + false + ); + expect(noop).toMatchObject({ + changed: false, + unchanged: ["OPERATOR_OVERRIDE", "PORT"], + }); + expect((await readFile(repo.localFile)).equals(handEdited)).toBe(true); + + const first = await hydrate( + repo, + { PORT: "3000", API_URL: "http://synthetic.test" }, + false + ); + expect(first).toMatchObject({ + changed: true, + added: ["API_URL"], + unchanged: ["PORT"], + }); + const text = await readFile(repo.localFile, "utf8"); + expect(layerValues(text)).toEqual({ + global: { + OPERATOR_OVERRIDE: "operator-value", + PORT: 3000, + TRACKED_DEFAULT: null, + API_URL: "http://synthetic.test", + }, + api: { SCOPED: "scoped-value" }, + }); + expect((await readFile(repo.trackedFile)).equals(tracked)).toBe(true); + + const repeat = await hydrate( + repo, + { PORT: "3000", API_URL: "http://synthetic.test" }, + false + ); + expect(repeat.changed).toBe(false); + expect(await readFile(repo.localFile, "utf8")).toBe(text); + const resolved = await resolvedGlobal(repo); + expect(resolved.OPERATOR_OVERRIDE).toBe("operator-value"); + expect(resolved.TRACKED_DEFAULT).toBeUndefined(); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); +}, 30_000); + +test("a secret that does not open under the current key refuses the whole import", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + const keyFile = resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME); + // Synthetic replacement key: the stored entry cannot open under it. + await writeFile(keyFile, "synthetic-replacement-key-material-0123456789\n"); + + expect(await refusalOf(() => hydrate(repo, { DB_PASS: PASSWORD }))).toBe( + "wrong_key" + ); + expect(await refusalOf(() => hydrate(repo, { PLAIN: "plain" }, false))).toBe( + "wrong_key" + ); + await rm(keyFile); + expect(await refusalOf(() => hydrate(repo, { DB_PASS: PASSWORD }))).toBe( + "wrong_key" + ); + // No replacement key was minted, nothing was staged and the lock is free. + expect(await Bun.file(keyFile).exists()).toBe(false); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}); + +test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { + const repo = await createRepo(); + const previous = process.env.HACK_ENV_SECRET_KEY; + process.env.HACK_ENV_SECRET_KEY = "synthetic-environment-key-material-42"; + try { + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( + "key_env_only" + ); + } finally { + if (previous === undefined) { + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); + } else { + process.env.HACK_ENV_SECRET_KEY = previous; + } + } + expect( + await Bun.file(resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME)).exists() + ).toBe(false); + expect(await Bun.file(repo.localFile).exists()).toBe(false); +}); + +test("a git-tracked local layer is refused", async () => { + const repo = await createTrackedRepo(); + const overlayLocal = resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: "dev", + }); + await writeFile( + overlayLocal, + "version: 1\nenvironment: dev\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await git(repo, ["add", "-f", overlayLocal]); + const original = await readFile(overlayLocal); + + expect( + await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }, true, "dev")) + ).toBe("tracked_layer"); + expect((await readFile(overlayLocal)).equals(original)).toBe(true); + // Positive control: the untracked base local layer still hydrates. + expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); +}, 30_000); + +test("a held lock refuses, and a lock from an exited process is reported as stale", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + + await writeFile(lockPath(repo), `${process.pid}\n`); + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "locked" + ); + // The refusal never removes a lock it does not own. + expect(await lockExists(repo)).toBe(true); + + const exited = Bun.spawn(["true"], { + env: privateCliEnv({ root: repo.root }), + }); + await exited.exited; + await writeFile(lockPath(repo), `${exited.pid}\n`); + let message = ""; + try { + await hydrate(repo, { API_TOKEN: CHANGED }); + } catch (error: unknown) { + message = error instanceof Error ? error.message : ""; + } + expect(message).toContain(lockPath(repo)); + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "stale_lock" + ); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + + await rm(lockPath(repo)); + expect((await hydrate(repo, { API_TOKEN: CHANGED })).changed).toBe(true); +}); + +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(); + } + return seen; +} + +function spawnWorker(repo: Repo, pauseAt: "rename" | "staging") { + const worker = Bun.spawn( + [process.execPath, WORKER, repo.projectRoot, repo.projectDir, pauseAt], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write(`API_TOKEN=${CHANGED}\nDB_PASS=${CHANGED}\n`); + worker.stdin.end(); + return worker; +} + +test("an import killed before its commit leaves every key at its previous value", async () => { + const repo = await createTrackedRepo(); + await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + const original = await readFile(repo.localFile); + + const worker = spawnWorker(repo, "rename"); + await readUntil(worker.stdout, "prepared\n"); + // The whole new layer is staged and fsynced, but not committed. + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + // A second hydration while the first holds the lock refuses. + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "locked" + ); + worker.kill("SIGKILL"); + await worker.exited; + + // No key changed: the previous layer is intact and still resolves. + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: TOKEN, + DB_PASS: PASSWORD, + }); + // The staging orphan and the stale lock are ignored, so the tree is clean. + expect(await stagingFiles(repo)).toHaveLength(1); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + // The killed process cannot release its lock; it is reported, not taken over. + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "stale_lock" + ); + await rm(lockPath(repo)); + const retried = await hydrate(repo, { + API_TOKEN: CHANGED, + DB_PASS: CHANGED, + }); + expect(retried.updated).toEqual(["API_TOKEN", "DB_PASS"]); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: CHANGED, + }); +}, 30_000); + +test("a layer changed by another writer during an import is not overwritten", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + + const worker = spawnWorker(repo, "staging"); + await readUntil(worker.stdout, "prepared\n"); + // A concurrent `hack env set --local` commits its own change meanwhile. + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "OPERATOR_OVERRIDE", + value: "operator-value", + secret: false, + local: true, + }); + const concurrent = await readFile(repo.localFile); + await writeFile(resolve(repo.projectDir, "go"), ""); + + const [stdout, exitCode] = await Promise.all([ + readUntil(worker.stdout, "\n"), + worker.exited, + ]); + expect(exitCode).toBe(0); + expect(stdout).toBe("refused:concurrent_write\n"); + // The other writer's layer stands; no imported key was applied. + expect((await readFile(repo.localFile)).equals(concurrent)).toBe(true); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: TOKEN, + OPERATOR_OVERRIDE: "operator-value", + }); + expect((await resolvedGlobal(repo)).DB_PASS).toBeUndefined(); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("a compare-and-swap write refuses when the file no longer matches", async () => { + const repo = await createRepo(); + const target = resolve(repo.projectDir, "cas.yaml"); + await writeFile(target, "current: 1\n"); + await expect( + writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: "stale: 0\n", + }) + ).rejects.toBeInstanceOf(AtomicWriteConflictError); + await expect( + writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: null, + }) + ).rejects.toBeInstanceOf(AtomicWriteConflictError); + expect(await readFile(target, "utf8")).toBe("current: 1\n"); + expect(await stagingFiles(repo)).toEqual([]); + expect( + await writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: "current: 1\n", + }) + ).toEqual({ changed: true }); +}); + +async function runCli( + repo: Repo, + args: readonly string[], + stdin: string +): Promise<{ exitCode: number; stdout: string; stderr: string }> { + 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, + ]); + for (const canary of CANARIES) { + expect(stdout).not.toContain(canary); + expect(stderr).not.toContain(canary); + } + return { exitCode, stdout, stderr }; +} + +test("`hack env hydrate` imports from stdin or a file and never prints values", async () => { + const repo = await createTrackedRepo(); + const base = ["env", "hydrate", "--secret", "--path", repo.projectRoot]; + const input = `# synthetic\nAPI_TOKEN=${TOKEN}\nexport DB_PASS="${PASSWORD}"\n`; + + const first = await runCli(repo, [...base, "--json"], input); + expect(first.exitCode).toBe(0); + expect(JSON.parse(first.stdout)).toMatchObject({ + file: repo.localFile, + local: true, + scope: "global", + changed: true, + added: ["API_TOKEN", "DB_PASS"], + }); + const hydrated = await readFile(repo.localFile); + + const repeat = await runCli(repo, [...base, "--json"], input); + expect(JSON.parse(repeat.stdout)).toMatchObject({ + changed: false, + unchanged: ["API_TOKEN", "DB_PASS"], + }); + expect((await readFile(repo.localFile)).equals(hydrated)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + + const file = join(repo.root, "entries.json"); + await writeFile(file, JSON.stringify({ API_TOKEN: CHANGED })); + const fromFile = await runCli( + repo, + [...base, "--file", file, "--format", "json"], + "" + ); + expect(fromFile.exitCode).toBe(0); + expect(fromFile.stdout + fromFile.stderr).toContain( + "0 added, 1 updated, 0 unchanged" + ); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); +}, 60_000); + +test("`hack env hydrate` refuses values in argv and malformed input without echo", async () => { + const repo = await createRepo(); + const base = ["env", "hydrate", "--path", repo.projectRoot]; + + const argv = await runCli(repo, [...base, `API_TOKEN=${TOKEN}`], ""); + expect(argv.exitCode).not.toBe(0); + expect(argv.stdout + argv.stderr).toContain("Unexpected arguments"); + + const malformed = await runCli( + repo, + base, + `API_TOKEN=${TOKEN}\n${PASSWORD}\n` + ); + expect(malformed.exitCode).not.toBe(0); + expect(malformed.stdout + malformed.stderr).toContain( + "Hydration input line 2" + ); + expect(await Bun.file(repo.localFile).exists()).toBe(false); +}, 60_000); From 52f0ffc9b43f883eef7e4fd54234ea8f9660e43a Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:54:21 -0400 Subject: [PATCH 05/14] 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 57a82c2b350b071f0bd132d323b55e6eb8895abb Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 13:03:34 -0400 Subject: [PATCH 06/14] test(env): assemble the shadowing secret key fixture at runtime gitleaks flagged the literal HACK_ENV_SECRET_KEY fixture as a generic API key. Build it at runtime like the repo's other key-shaped fixtures and record the fingerprint of the branch commit that still holds the literal. Co-Authored-By: Claude Opus 5.5 --- .gitleaksignore | 1 + tests/project-env-hydrate.test.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.gitleaksignore b/.gitleaksignore index c753a2d99..426810123 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -2,3 +2,4 @@ cbad51795170d25e1ab0926c1ac5a74d1e001ef9:.factory/validation/misc-admin-followups-1/user-testing/flows/management-token-email-verified.json:generic-api-key:8 e069b8546c112e5b5c8ab0ec82fd24a4f19ef16b:.hack/tickets/events/events-2026-03.jsonl:generic-api-key:58 939c2b2cb680744707b1d085db0bc0ce8fac32e4:tests/env.test.ts:private-key:7 +06f262309db3111e65c8a23324456ef73ac57e83:tests/project-env-hydrate.test.ts:generic-api-key:389 diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index 34a390952..b07cf0e15 100644 --- a/tests/project-env-hydrate.test.ts +++ b/tests/project-env-hydrate.test.ts @@ -386,7 +386,14 @@ test("a secret that does not open under the current key refuses the whole import test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { const repo = await createRepo(); const previous = process.env.HACK_ENV_SECRET_KEY; - process.env.HACK_ENV_SECRET_KEY = "synthetic-environment-key-material-42"; + // Assembled at runtime so no key-shaped literal is committed. + process.env.HACK_ENV_SECRET_KEY = [ + "synthetic", + "environment", + "key", + "material", + "42", + ].join("-"); try { expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( "key_env_only" From fba3e87cf9488fa71142e01b6f7a3038dc4cdc44 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 13:31:39 -0400 Subject: [PATCH 07/14] fix(env): fail closed on hydration edge cases - Refuse hydration as tracking_unknown when the project is inside a git checkout but git cannot report whether it tracks the local layer (exit 128 from a safe.directory refusal or damaged index, or git missing). Outside any checkout, hydration still proceeds. - Refuse a plain import (no --secret) of a key the target scope stores as a secret (secret_downgrade), instead of silently replacing ciphertext with plaintext. - Refuse repeated top-level keys in --format json input (duplicate_key), matching the env format, instead of keeping the last value. - Pin the unreadable_layer refusal (invalid YAML, wrong shape, wrong version) and the key-file-before-HACK_ENV_SECRET_KEY reader priority with regression tests; share one runtime-built synthetic key helper. - docs/env.md lists every hydration refusal. Refs: HACK-1158 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 22 ++++- src/lib/env-hydrate-input.ts | 62 +++++++++++- src/lib/project-env-config.ts | 66 ++++++++++++- tests/env-hydrate-input.test.ts | 34 +++++++ tests/project-env-hydrate.test.ts | 158 ++++++++++++++++++++++++++---- 5 files changed, 317 insertions(+), 25 deletions(-) diff --git a/docs/env.md b/docs/env.md index 7f17db0d5..0d88924fb 100644 --- a/docs/env.md +++ b/docs/env.md @@ -476,12 +476,15 @@ Input comes only from piped stdin or `--file`, never from arguments, and is at m must close on the same line, followed only by spaces or a `# comment` - empty values, repeated keys and `${VAR}` expansion are not supported -`--format json` takes one object whose values are non-empty strings. One malformed -entry refuses the whole input. The message gives only the line number, never the +`--format json` takes one object whose values are non-empty strings. A key that +appears twice is refused, as in the `env` format; it is not resolved by keeping the +last value. One malformed entry refuses the whole input. The message gives only the line number, never the line, and nothing is written. `--secret` encrypts every imported value; without it the values are stored as plain -entries in the local layer. `--service` picks one scope for all entries (default +entries in the local layer. A plain import refuses to replace a key that the local +layer already stores as a secret, so repeat an import with the same flags it was first +run with. `--service` picks one scope for all entries (default `global`). The output and `--json` result list the added, updated and unchanged keys, never values. @@ -500,11 +503,20 @@ Hydration is idempotent and all-or-nothing: Hydration refuses, without changing the layer, when: - git tracks the target local layer +- the project is in a git checkout but git cannot say whether it tracks the local layer, + for example after a `safe.directory` refusal, with a damaged index, or with git not + installed (outside any git checkout, hydration proceeds) +- the local layer exists but is not a valid env file, for example invalid YAML, the + wrong `version`, or a scope that is not a mapping; fix or remove it first - a secret already in the local layer does not decrypt under the project env key, or no - key is available + key is available (`.hack.secret.key` is used before `HACK_ENV_SECRET_KEY`, as when + values are read) - `--secret` is used, no `.hack.secret.key` exists, and `HACK_ENV_SECRET_KEY` is set (a new key file would replace that key for every layer) -- another `hack env hydrate` for the same project is running +- `--secret` is not used and an imported key is stored as a secret in the target scope; + re-run with `--secret`, or remove the key with `hack env unset --local` first +- another `hack env hydrate` for the same project is running, or an interrupted one left + a stale lock (see below) - the local layer changed between hydration's read and its rename, for example from a concurrent `hack env set --local` diff --git a/src/lib/env-hydrate-input.ts b/src/lib/env-hydrate-input.ts index 8d6516f41..d1672a7d9 100644 --- a/src/lib/env-hydrate-input.ts +++ b/src/lib/env-hydrate-input.ts @@ -28,7 +28,7 @@ import type { EnvValueStdinSource } from "./env-value-stdin.ts"; * - Empty values and repeated keys are refused. No `${VAR}` expansion. * * `json` format: one object whose keys match the key pattern and whose values - * are non-empty strings. + * are non-empty strings. Repeated keys are refused, not resolved last-wins. */ export const ENV_HYDRATE_INPUT_MAX_BYTES = 1024 * 1024; @@ -298,6 +298,11 @@ function parseJsonEntries(text: string): Map { if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { throw new EnvHydrateInputError("json_not_object"); } + // JSON.parse silently keeps the last of repeated keys; refuse them instead, + // as the env format does. + if (hasDuplicateTopLevelJsonKey(text)) { + throw new EnvHydrateInputError("duplicate_key"); + } const entries = new Map(); for (const [key, value] of Object.entries(parsed)) { if (!ENV_KEY_PATTERN.test(key)) { @@ -314,6 +319,61 @@ function parseJsonEntries(text: string): Map { return entries; } +/** + * Whether the top-level object in `text` names one key twice. `text` must + * already be valid JSON whose root is an object: the scan only tracks string + * boundaries and nesting depth, and decodes each depth-1 key with JSON.parse + * so escaped spellings of one key compare equal. + */ +function hasDuplicateTopLevelJsonKey(text: string): boolean { + const seen = new Set(); + let depth = 0; + let expectKey = false; + for (let index = 0; index < text.length; index += 1) { + const char = text[index]; + if (char === '"') { + const end = closingJsonQuoteIndex(text, index); + // expectKey is set only at depth 1, after `{` or `,`, and in valid JSON + // the next string there is the key. + if (expectKey) { + const key = String(JSON.parse(text.slice(index, end + 1))); + if (seen.has(key)) { + return true; + } + seen.add(key); + expectKey = false; + } + index = end; + continue; + } + depth += jsonDepthChange(char); + // Only the root opens depth 1, and the caller checked it is an object. + if (depth === 1 && (char === "{" || char === ",")) { + expectKey = true; + } + } + return false; +} + +/** Index of the quote closing the JSON string that opens at `start`. */ +function closingJsonQuoteIndex(text: string, start: number): number { + let index = start + 1; + while (index < text.length && text[index] !== '"') { + index += text[index] === "\\" ? 2 : 1; + } + return index; +} + +function jsonDepthChange(char: string | undefined): number { + if (char === "{" || char === "[") { + return 1; + } + if (char === "}" || char === "]") { + return -1; + } + return 0; +} + async function readBoundedStdin(input: { readonly source: EnvValueStdinSource; readonly maxBytes: number; diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 566272aa7..dcf433373 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -479,6 +479,31 @@ async function isGitTrackedProjectEnvPath(opts: { return null; } +/** + * Whether `startDir` or an ancestor holds a `.git` entry (directory, linked + * worktree file or symlink), or `GIT_DIR` points git elsewhere. It does not ask + * git, so it still answers when git itself refuses to run in the checkout. + */ +async function isInsideGitCheckoutMarker(opts: { + readonly startDir: string; +}): Promise { + if ((process.env.GIT_DIR ?? "").trim().length > 0) { + return true; + } + let current = resolve(opts.startDir); + while (true) { + const marker = await lstat(resolve(current, ".git")).catch(() => null); + if (marker !== null) { + return true; + } + const parent = dirname(current); + if (parent === current) { + return false; + } + current = parent; + } +} + async function hasLegacyLocalEnvironment(opts: { readonly path: string; }): Promise { @@ -2488,11 +2513,13 @@ export async function unsetProjectEnvValue(opts: { /** Closed reasons `hydrateProjectEnvLocalLayer` refuses with; none writes a layer. */ export const PROJECT_ENV_HYDRATION_REFUSAL_REASONS = [ "tracked_layer", + "tracking_unknown", "locked", "stale_lock", "unreadable_layer", "wrong_key", "key_env_only", + "secret_downgrade", "concurrent_write", ] as const; @@ -2509,6 +2536,8 @@ function projectEnvHydrationRefusalMessage(opts: { switch (opts.reason) { case "tracked_layer": return "Refusing to hydrate: git tracks the machine-local env layer, so hydrated values would be committed. Untrack it with `git rm --cached` and retry."; + case "tracking_unknown": + return "Refusing to hydrate: this checkout is a git repository, but git could not report whether it tracks the machine-local env layer (for example after a safe.directory refusal, with a damaged index, or without git installed), so hydrated values could be committed. Make `git status` work in this checkout and retry."; case "locked": return "Refusing to hydrate: another `hack env hydrate` is writing this project's machine-local env layer. Retry when it finishes."; case "stale_lock": @@ -2519,6 +2548,8 @@ function projectEnvHydrationRefusalMessage(opts: { return `Refusing to hydrate: a secret already in the machine-local env layer does not decrypt under the project env key, or no key is available. Restore the matching ${PROJECT_ENV_KEY_FILENAME} and retry.`; case "key_env_only": return `Refusing to hydrate secrets: ${PROJECT_ENV_SECRET_KEY_ENV} is set but no ${PROJECT_ENV_KEY_FILENAME} exists, and creating one would replace that key for every layer. Store the same key in ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "secret_downgrade": + return "Refusing to hydrate without --secret: an imported key is stored as a secret in the machine-local env layer, and a plain import would replace its ciphertext with plaintext. Re-run with --secret, or first remove that key with `hack env unset --local` if it should be stored as plain text."; case "concurrent_write": return "Refusing to hydrate: the machine-local env layer changed while hydration was preparing it. Re-run hydration."; default: @@ -2564,7 +2595,13 @@ export type ProjectEnvHydrationResult = { * Contract: * - Only the effective machine-local layer for `envName` is written * (`hack.env.local.yaml` or `hack.env..local.yaml`); a layer that - * git tracks is refused. Tracked defaults are never read for writing. + * git tracks is refused, and so is any layer inside a git checkout where + * git cannot answer (fail closed). Tracked defaults are never read for + * writing. + * - A layer that exists but is not a valid env file is refused, never + * replaced, so hand-written content is not lost. + * - Without `secret`, an imported key that the target scope stores as a + * secret is refused rather than downgraded to plaintext. * - Keys outside `entries`, other scopes and removal markers for other keys * are kept as they are. An imported key replaces that key's local entry. * - Idempotent: an entry that already holds its value is kept unchanged (a @@ -2624,6 +2661,14 @@ export async function hydrateProjectEnvLocalLayer(opts: { if (tracked === true) { throw refuse("tracked_layer"); } + // `null` covers both "not a repository" and "git failed" (a safe.directory + // refusal, a damaged index, git missing). Only the first is safe to write. + if ( + tracked === null && + (await isInsideGitCheckoutMarker({ startDir: opts.projectRoot })) + ) { + throw refuse("tracking_unknown"); + } const release = await acquireProjectEnvHydrationLock({ projectDir: opts.projectDir, @@ -2668,8 +2713,25 @@ async function hydrateLockedProjectEnvLayer(opts: { secret: opts.secret, refuse: opts.refuse, }); + const previousScope = layer.config.values[opts.scope] ?? {}; + if ( + !opts.secret && + [...opts.entries.keys()].some((entryKey) => { + const previous = Object.hasOwn(previousScope, entryKey) + ? previousScope[entryKey] + : undefined; + return ( + previous !== undefined && + previous !== null && + isProjectEnvSecretValue(previous) + ); + }) + ) { + // A plain import would silently drop at-rest encryption for that key. + throw opts.refuse("secret_downgrade"); + } const plan = planProjectEnvHydration({ - previousScope: layer.config.values[opts.scope] ?? {}, + previousScope, entries: opts.entries, keyText: key.keyText, }); diff --git a/tests/env-hydrate-input.test.ts b/tests/env-hydrate-input.test.ts index 2eee54328..67099a32e 100644 --- a/tests/env-hydrate-input.test.ts +++ b/tests/env-hydrate-input.test.ts @@ -141,6 +141,40 @@ test("empty input and malformed json are refused without echo", () => { } }); +test("json format refuses a repeated key instead of keeping the last value", () => { + const duplicates = [ + `{"API_TOKEN": "${CANARY}", "API_TOKEN": "x"}`, + `{"API_TOKEN": "x", "DB_PASS": "y", "API_TOKEN": "${CANARY}"}`, + // An escaped spelling names the same key. + `{"API_TOKEN": "${CANARY}", "\\u0041PI_TOKEN": "x"}`, + // Brackets inside a value do not hide a later repeat. + `{"API_TOKEN": "{[${CANARY}", "API_TOKEN": "x"}`, + ]; + for (const text of duplicates) { + expect(refusal(() => entriesOf(text, "json"))).toEqual({ + reason: "duplicate_key", + line: null, + }); + } + // Quotes, commas and braces inside values, and nested keys, are not keys. + expect( + entriesOf( + `{"API_TOKEN": "a\\",\\"API_TOKEN\\": {[", "DB_PASS": "}${CANARY}"}`, + "json" + ) + ).toEqual({ API_TOKEN: 'a","API_TOKEN": {[', DB_PASS: `}${CANARY}` }); + for (const nested of [ + '{"API_TOKEN": {"NESTED": "a", "NESTED": "b"}}', + '{"API_TOKEN": {"API_TOKEN": "a"}}', + '{"API_TOKEN": "a", "DB_PASS": {"X": 1, "API_TOKEN": 2}}', + '{"API_TOKEN": ["a", "API_TOKEN"]}', + ]) { + expect(refusal(() => entriesOf(nested, "json")).reason).toBe( + "json_value_not_string" + ); + } +}); + test("--format accepts only env and json and never echoes the argument", () => { expect(parseEnvHydrateInputFormat(undefined)).toBe("env"); expect(parseEnvHydrateInputFormat("json")).toBe("json"); diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index b07cf0e15..d781d3e30 100644 --- a/tests/project-env-hydrate.test.ts +++ b/tests/project-env-hydrate.test.ts @@ -51,6 +51,32 @@ const PASSWORD = "synthetic-password-c51a"; const CHANGED = "synthetic-changed-80f2"; const CANARIES = [TOKEN, PASSWORD, CHANGED] as const; +/** + * Synthetic key material, assembled at runtime so that no key-shaped literal + * sits in source for secret scanners to flag. + */ +function syntheticKeyMaterial(label: string): string { + return ["synthetic", label, "key", "material", "0123456789"].join("-"); +} + +/** Runs `run` with `HACK_ENV_SECRET_KEY` set, restoring the previous value. */ +async function withEnvironmentKey( + keyText: string, + run: () => Promise +): Promise { + const previous = process.env.HACK_ENV_SECRET_KEY; + process.env.HACK_ENV_SECRET_KEY = keyText; + try { + return await run(); + } finally { + if (previous === undefined) { + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); + } else { + process.env.HACK_ENV_SECRET_KEY = previous; + } + } +} + const tempDirs = new Set(); afterEach(async () => { @@ -385,26 +411,11 @@ test("a secret that does not open under the current key refuses the whole import test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { const repo = await createRepo(); - const previous = process.env.HACK_ENV_SECRET_KEY; - // Assembled at runtime so no key-shaped literal is committed. - process.env.HACK_ENV_SECRET_KEY = [ - "synthetic", - "environment", - "key", - "material", - "42", - ].join("-"); - try { + await withEnvironmentKey(syntheticKeyMaterial("environment"), async () => { expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( "key_env_only" ); - } finally { - if (previous === undefined) { - Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); - } else { - process.env.HACK_ENV_SECRET_KEY = previous; - } - } + }); expect( await Bun.file(resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME)).exists() ).toBe(false); @@ -432,6 +443,119 @@ test("a git-tracked local layer is refused", async () => { expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); }, 30_000); +test("a checkout where git cannot report tracking refuses instead of writing", async () => { + const repo = await createTrackedRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + // A damaged index makes `git ls-files` exit 128, as a safe.directory + // refusal does; that is not the same as "not a repository". + await writeFile(resolve(repo.projectRoot, ".git", "index"), "not an index\n"); + + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "tracking_unknown" + ); + expect( + await refusalOf(() => hydrate(repo, { PLAIN: "plain" }, false, "dev")) + ).toBe("tracking_unknown"); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect( + await Bun.file( + resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: "dev", + }) + ).exists() + ).toBe(false); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("an existing local layer that is not a valid env file is refused, never replaced", async () => { + const repo = await createRepo(); + const invalidLayers = [ + // Not YAML at all. + `values: [unclosed\n API_TOKEN: ${TOKEN}\n`, + // Valid YAML, wrong shape: a scope that is not a mapping. + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: operator notes\n", + // Valid YAML, wrong version. + "version: 99\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n", + ]; + for (const layer of invalidLayers) { + await writeFile(repo.localFile, layer); + for (const secret of [true, false]) { + expect( + await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }, secret)) + ).toBe("unreadable_layer"); + expect(await readFile(repo.localFile, "utf8")).toBe(layer); + } + } + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); + + // Positive control: once the layer is removed, the same import succeeds. + await rm(repo.localFile); + expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); +}); + +test("existing secrets are checked with the key file before HACK_ENV_SECRET_KEY, as readers do", async () => { + const repo = await createRepo(); + // Creates the key file; the stored secret is encrypted under it. + await hydrate(repo, { API_TOKEN: TOKEN }); + + await withEnvironmentKey(syntheticKeyMaterial("environment"), async () => { + const plain = await hydrate(repo, { PLAIN: "plain" }, false); + expect(plain).toMatchObject({ changed: true, added: ["PLAIN"] }); + const secret = await hydrate(repo, { + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); + expect(secret).toMatchObject({ + changed: true, + createdKey: false, + added: ["DB_PASS"], + updated: ["API_TOKEN"], + }); + // Readers resolve with the same priority and open every entry. + expect(await resolvedGlobal(repo)).toMatchObject({ + PLAIN: "plain", + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); + }); +}); + +test("a plain import never downgrades a stored secret to plaintext", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + + // Same value or a new one, alone or with other keys: all refused. + const attempts: readonly Record[] = [ + { API_TOKEN: TOKEN }, + { API_TOKEN: CHANGED }, + { OTHER: "plain", API_TOKEN: TOKEN }, + ]; + for (const entries of attempts) { + expect(await refusalOf(() => hydrate(repo, entries, false))).toBe( + "secret_downgrade" + ); + } + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); + + // Positive controls: other plain keys, and a secret repeat, still work. + expect((await hydrate(repo, { OTHER: "plain" }, false)).added).toEqual([ + "OTHER", + ]); + expect((await hydrate(repo, { API_TOKEN: TOKEN })).unchanged).toEqual([ + "API_TOKEN", + ]); + expect( + ciphertextOf(await readFile(repo.localFile, "utf8"), "API_TOKEN") + ).toBe(ciphertextOf(original.toString("utf8"), "API_TOKEN")); +}); + test("a held lock refuses, and a lock from an exited process is reported as stale", async () => { const repo = await createRepo(); await hydrate(repo, { API_TOKEN: TOKEN }); From 5d58e9b063e0c81a2ed73d40580be4097d8a6da3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 14:25:38 -0400 Subject: [PATCH 08/14] 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 09/14] 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); From 3a3bf6edd480fe6f6de9413813c4af7656d87cd1 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 14:02:53 -0400 Subject: [PATCH 10/14] fix(native): let run reuse the running env and profile selection `hack run --profile X` on the native runtime refused every --env/--profile, even when X was exactly what `hack up --profile X` started (HACK-1276). Native run now compares a given --env or --profile with the selection the saved run mapping recorded at startup: profiles as a normalized set, env by its resolved name. An equal selection runs the one-off; a different one, or a legacy mapping without recorded selectors, refuses with a one-line E_USAGE before any runtime request. Omitted options still inherit the running selection, and run never restarts the project. The P02 README and the native candidate guide document the native flow. Refs HACK-1276 Co-Authored-By: Claude Opus 5.5 --- docs/guides/native-candidate.md | 5 + .../patterns/p02-portable-toolchain/README.md | 17 ++ src/backends/native-project-one-off.ts | 49 +++- src/commands/project.ts | 31 ++- tests/native-project-one-off.test.ts | 91 +++++- tests/native-run-selection-cli.test.ts | 261 ++++++++++++++++++ tests/native-runtime-not-running-cli.test.ts | 9 +- 7 files changed, 445 insertions(+), 18 deletions(-) create mode 100644 tests/native-run-selection-cli.test.ts diff --git a/docs/guides/native-candidate.md b/docs/guides/native-candidate.md index 50a6c6fb4..89c77bb0b 100644 --- a/docs/guides/native-candidate.md +++ b/docs/guides/native-candidate.md @@ -799,6 +799,11 @@ It does not publish service ports or aliases. Commands are noninteractive and bounded to 300 seconds; omitted arguments retain the service's default command. The CLI returns command output and exit status only after owned job cleanup is confirmed. There is no automatic command replay or fallback to Compose. +`run` uses the running project's environment and profiles. `--env` and `--profile` +are optional; when given they must equal the selection recorded at startup +(profiles compare as a set). A different selection, or a legacy mapping without +recorded selectors, refuses with `E_USAGE` before any runtime request. `run` never +restarts the project to change its selection. Services with no managed environment values also support `run`; they keep the admitted image's environment without creating a private environment mount or launcher. The one-off request still carries an explicit empty, identity-bound diff --git a/examples/patterns/p02-portable-toolchain/README.md b/examples/patterns/p02-portable-toolchain/README.md index f3785f2f6..2be5dcef5 100644 --- a/examples/patterns/p02-portable-toolchain/README.md +++ b/examples/patterns/p02-portable-toolchain/README.md @@ -35,6 +35,23 @@ output through a filter that can hide a failed check. The service is one-shot, so there is nothing to keep running. `hack down` keeps the named cache volumes. +### Native runtime candidate + +With the native runtime candidate (`HACK_RUNTIME_BACKEND=native`), the run +commands above work only against a started project. Start it with the same +profile first. Native `up` stays in the foreground, so run it in its own terminal: + +```sh +hack up --profile toolchain +``` + +Then run the commands above from another terminal. A native run uses the +environment and profiles that the running project started with. You can omit +`--profile` and `--env`. If you pass them, they must match the running selection +exactly; the order and repeats of profile names do not matter. A different +selection is refused with `E_USAGE` before any command runs. A run never +restarts the project to change the selection. + ## What this example does not prove - Cache reuse across worktrees with bounded eviction. diff --git a/src/backends/native-project-one-off.ts b/src/backends/native-project-one-off.ts index 8373b094a..7116b50ed 100644 --- a/src/backends/native-project-one-off.ts +++ b/src/backends/native-project-one-off.ts @@ -1,10 +1,12 @@ -import { ClosedCliError } from "../lib/cli-result.ts"; +import { ClosedCliError, HackCliError } from "../lib/cli-result.ts"; import { isRecord } from "../lib/guards.ts"; import { prepareNativeExecEnvironment } from "./native-exec-environment.ts"; import { refreshNativeProjectDependencies } from "./native-project-dependency-refresh.ts"; import { loadNativeProjectRun, + type NativeProjectRun, type NativeProjectRunScope, + normalizeNativeProfiles, } from "./native-project-run.ts"; import { invokeNativeRuntime, @@ -29,6 +31,48 @@ function decode(value: unknown): Buffer { } return bytes; } +/** The closed `E_USAGE` refusal of a run whose selection differs from the running one. */ +export const NATIVE_RUN_SELECTION_REFUSAL = + "Native `hack run` uses the running project's environment and profiles, and the requested --env or --profile does not match them. Omit --env and --profile, or pass exactly the selection `hack up` started with. No command ran."; + +/** + * The selection a native one-off asks for. An omitted field inherits the + * running selection; a given field must equal it exactly. Profiles compare as + * a set (order and duplicates do not matter), and an explicit base env is + * `null`. A run never restarts the project to change its selection. + */ +export type NativeRunSelection = { + readonly envName?: string | null; + readonly profiles?: readonly string[]; +}; + +/** + * Refuse, before any runtime request, a requested selection that differs from + * the running one or that a legacy mapping without recorded selectors cannot + * confirm. + */ +export function requireNativeRunSelection(opts: { + readonly run: NativeProjectRun; + readonly selection?: NativeRunSelection; +}): void { + const { run, selection } = opts; + const envDiffers = + selection?.envName !== undefined && + (run.effectiveEnvName === undefined || + selection.envName !== run.effectiveEnvName); + const profilesDiffer = + selection?.profiles !== undefined && + (run.profiles === undefined || + JSON.stringify(normalizeNativeProfiles(selection.profiles)) !== + JSON.stringify(run.profiles)); + if (envDiffers || profilesDiffer) { + throw new HackCliError({ + code: "E_USAGE", + message: NATIVE_RUN_SELECTION_REFUSAL, + }); + } +} + /** A separate service container; completion requires confirmed job cleanup. */ export async function nativeProjectOneOff(opts: { readonly runtime: NativeRuntimeSelection; @@ -37,6 +81,8 @@ export async function nativeProjectOneOff(opts: { readonly argv: readonly string[]; readonly workdir?: string; readonly composeFile: string; + /** Optional `--env`/`--profile` request; it must match the running selection. */ + readonly selection?: NativeRunSelection; readonly invoke?: typeof invokeNativeRuntime; readonly environment?: typeof prepareNativeExecEnvironment; }) { @@ -63,6 +109,7 @@ export async function nativeProjectOneOff(opts: { "Native run currently requires a started project; run up first." ); } + requireNativeRunSelection({ run: mapping, selection: opts.selection }); const refreshed = await refreshNativeProjectDependencies(opts); if (JSON.stringify(refreshed.mapping) !== JSON.stringify(mapping)) { throw refused(); diff --git a/src/commands/project.ts b/src/commands/project.ts index 9ac23931c..cdc6edc61 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -51,7 +51,10 @@ import { nativeProjectPs, requireComposeOperationAvailable, } from "../backends/native-project-observe.ts"; -import { nativeProjectOneOff } from "../backends/native-project-one-off.ts"; +import { + type NativeRunSelection, + nativeProjectOneOff, +} from "../backends/native-project-one-off.ts"; import { retireNativeRecoveredPublisher, verifyNativeFrontendRecovery, @@ -7415,16 +7418,27 @@ async function handlePs({ return 0; } -function validateRunBackendOptions(native: boolean, args: RunArgs): void { +/** + * Compose runs need the Compose operation; a native one-off instead carries its + * `--env`/`--profile` request. Omitted options inherit the running selection; + * given options must match it exactly, which the native backend checks before + * any runtime request (no implicit restart). + */ +function runBackendSelection( + native: boolean, + args: RunArgs +): NativeRunSelection | undefined { if (!native) { requireComposeOperationAvailable("run"); return; } - if (args.options.env || args.options.profile) { - throw new CliUsageError( - "Native run uses the started project's environment and profiles; restart with the requested selections first." - ); - } + return { + envName: resolveRequestedEnvName({ envOption: args.options.env }), + profiles: + args.options.profile === undefined + ? undefined + : parseCsvList(args.options.profile), + }; } async function handleRun({ @@ -7450,7 +7464,7 @@ async function handleRun({ return authored; } const native = resolveNativeRuntimeSelection(); - validateRunBackendOptions(Boolean(native), args); + const selection = runBackendSelection(Boolean(native), args); const project = await resolveProjectForArgs({ ctx, pathOpt: args.options.path, @@ -7477,6 +7491,7 @@ async function handleRun({ argv: args.positionals.cmd, workdir: args.options.workdir, composeFile: project.composeFile, + selection, }); process.stdout.write(result.stdout); process.stderr.write(result.stderr); diff --git a/tests/native-project-one-off.test.ts b/tests/native-project-one-off.test.ts index f792f40ef..3a25c07bd 100644 --- a/tests/native-project-one-off.test.ts +++ b/tests/native-project-one-off.test.ts @@ -2,12 +2,18 @@ import { afterEach, expect, test } from "bun:test"; import { mkdir, mkdtemp, realpath, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { nativeProjectOneOff } from "../src/backends/native-project-one-off.ts"; +import { + NATIVE_RUN_SELECTION_REFUSAL, + nativeProjectOneOff, +} from "../src/backends/native-project-one-off.ts"; import { removeNativeProjectRun, saveNativeProjectRun, } from "../src/backends/native-project-run.ts"; -import { expectClosedRefusal } from "./helpers/closed-refusal.ts"; +import { + expectClosedRefusal, + expectCodedRefusal, +} from "./helpers/closed-refusal.ts"; const roots: string[] = []; afterEach(async () => { @@ -45,7 +51,7 @@ function refreshed() { changed_slots: [], }; } -async function fixture(saved = true) { +async function fixture(saved = true, recorded: Record = {}) { const projectRoot = await realpath( await mkdtemp(join(tmpdir(), "native-one-off-")) ); @@ -56,7 +62,7 @@ async function fixture(saved = true) { await mkdir(nativeHome); const scope = { projectRoot, projectDir, nativeHome, branch: null }; if (saved) { - await saveNativeProjectRun({ ...scope, run }); + await saveNativeProjectRun({ ...scope, run: { ...run, ...recorded } }); } return { scope, @@ -298,3 +304,80 @@ test("failed dependency refresh refuses before one-off selection or private prep expect(prepared).toBe(0); } }); + +const startedSelection = { + profiles: ["toolchain", "tools"], + effectiveEnvName: "staging", + aws: null, +}; + +test("a run selection equal to the running one proceeds to the job", async () => { + for (const requested of [ + {}, + { profiles: ["tools", "toolchain", "tools"] }, + { envName: "staging", profiles: ["toolchain", "tools"] }, + ]) { + const opts = await fixture(true, startedSelection); + const calls: string[] = []; + const result = await nativeProjectOneOff({ + ...opts, + argv: ["bun", "--version"], + selection: requested, + environment: async () => ({}), + invoke: async (request) => { + calls.push(String(request.args[1])); + if (request.args[1] === "refresh-dependencies") { + return refreshed(); + } + if (request.args[1] === "run-selection") { + return selection; + } + return { ...completion, exit_code: 0 }; + }, + }); + expect(calls).toEqual([ + "refresh-dependencies", + "run-selection", + "run-service", + ]); + expect(result.exitCode).toBe(0); + } +}); + +test("a run selection that differs from the running one refuses before any request", async () => { + const cases: { + readonly recorded: Record; + readonly selection: { envName?: string | null; profiles?: string[] }; + }[] = [ + { recorded: startedSelection, selection: { profiles: ["web"] } }, + { recorded: startedSelection, selection: { profiles: ["toolchain"] } }, + { + recorded: startedSelection, + selection: { profiles: ["toolchain", "tools", "web"] }, + }, + { recorded: startedSelection, selection: { profiles: [] } }, + { recorded: startedSelection, selection: { envName: null } }, + { recorded: startedSelection, selection: { envName: "preview" } }, + // A legacy mapping without recorded selectors cannot confirm a match. + { recorded: {}, selection: { profiles: [] } }, + { recorded: {}, selection: { envName: null } }, + ]; + for (const { recorded, selection: requested } of cases) { + const opts = await fixture(true, recorded); + await expectCodedRefusal( + nativeProjectOneOff({ + ...opts, + argv: ["bun", "--version"], + selection: requested, + environment: async () => { + throw new Error("must not prepare"); + }, + invoke: async () => { + throw new Error("must not invoke"); + }, + }), + "E_USAGE", + NATIVE_RUN_SELECTION_REFUSAL + ); + } +}); diff --git a/tests/native-run-selection-cli.test.ts b/tests/native-run-selection-cli.test.ts new file mode 100644 index 000000000..7c7716692 --- /dev/null +++ b/tests/native-run-selection-cli.test.ts @@ -0,0 +1,261 @@ +import { afterEach, expect, test } from "bun:test"; +import { + mkdir, + mkdtemp, + readFile, + realpath, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { NATIVE_RUN_SELECTION_REFUSAL } from "../src/backends/native-project-one-off.ts"; +import { saveNativeProjectRun } from "../src/backends/native-project-run.ts"; +import { privateCliEnv, privateCliHome } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1276: native `hack run --profile X` after `hack up --profile X`. The + * stub runtime answers the one-off protocol (dependency refresh, run + * selection, run-service completion) for the saved run, so a run whose + * selection matches the running one completes, and one that differs refuses + * before any runtime request. + */ + +const roots: string[] = []; +const entrypoint = resolve("index.ts"); +const ids = { + run: "a".repeat(32), + owner: "b".repeat(32), + namespace: "c".repeat(64), + plan: "d".repeat(64), + generation: "e".repeat(64), + job: "f".repeat(32), +}; +/** A JavaScript stack frame as Bun prints it. */ +const STACK_FRAME = /^\s+at /m; +const CHILD_STDOUT = "1.4.2\n"; + +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +async function fixture(opts: { + readonly profiles?: readonly string[]; + readonly effectiveEnvName?: string | null; +}) { + const root = await realpath( + await mkdtemp(join(tmpdir(), "native-run-selection-cli-")) + ); + roots.push(root); + const project = join(root, "project"); + const projectDir = join(project, ".hack"); + const nativeHome = join(root, "native-home"); + await mkdir(projectDir, { recursive: true }); + await mkdir(nativeHome, { mode: 0o700 }); + await writeFile( + join(projectDir, "docker-compose.yml"), + JSON.stringify({ + services: { + toolchain: { + image: "docker.io/library/busybox:1.37", + profiles: ["toolchain"], + }, + }, + }) + ); + await writeFile( + join(projectDir, "hack.config.json"), + JSON.stringify({ + name: "run-selection-fixture", + dev_host: "run-selection-fixture.hack.local", + }) + ); + const refresh = JSON.stringify({ + ok: true, + run: ids.run, + owner: ids.owner, + namespace: ids.namespace, + plan: ids.plan, + generation: ids.generation, + changed_slots: [], + }); + const selection = JSON.stringify({ + run: ids.run, + owner: ids.owner, + namespace: ids.namespace, + plan: ids.plan, + service: "toolchain", + generation: ids.generation, + boot: "boot", + }); + const completion = JSON.stringify({ + job: ids.job, + cleanup_confirmed: true, + exit_code: 0, + stdout_base64: Buffer.from(CHILD_STDOUT).toString("base64"), + stderr_base64: "", + truncated: false, + }); + const binary = join(root, "native-runtime"); + // argv: --candidate-root HOME graph ... + await writeFile( + binary, + `#!/bin/sh +printf '%s\\n' "$*" >> "$HOME/native-calls" +case "$4" in + refresh-dependencies) printf '%s' '${refresh}' ;; + run-selection) printf '%s' '${selection}' ;; + run-service) cat > /dev/null; printf '%s' '${completion}' ;; + *) printf '%s' '{"code":"runtime_not_running"}' >&2; exit 1 ;; +esac +`, + { mode: 0o700 } + ); + await saveNativeProjectRun({ + projectRoot: project, + projectDir, + nativeHome, + branch: "alpha", + run: { + run: ids.run, + owner: ids.owner, + namespace: ids.namespace, + planId: ids.plan, + profiles: [...(opts.profiles ?? [])], + effectiveEnvName: opts.effectiveEnvName ?? null, + aws: null, + }, + }); + const env = privateCliEnv({ + root, + env: { + HACK_RUNTIME_BACKEND: "native", + HACK_NATIVE_BINARY: binary, + HACK_NATIVE_HOME: nativeHome, + HACK_NO_INTERACTIVE: "1", + HACK_LOGGER: "console", + }, + }); + return { root, project, env }; +} + +async function cli(opts: { + readonly fixture: Awaited>; + readonly args: readonly string[]; +}) { + // Project selection goes first: after the child command every token + // belongs to the child. + const child = Bun.spawn( + [ + process.execPath, + entrypoint, + "--path", + opts.fixture.project, + "--branch", + "alpha", + ...opts.args, + ], + { + cwd: opts.fixture.root, + env: opts.fixture.env, + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + timeout: 20_000, + } + ); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { stdout, stderr, code }; +} + +async function nativeCalls(root: string): Promise { + const text = await readFile( + join(privateCliHome(root), "native-calls"), + "utf8" + ).catch(() => ""); + return text.split("\n").filter(Boolean); +} + +function operations(calls: readonly string[]): string[] { + return calls.map((call) => call.split(" ")[3] ?? ""); +} + +test("run --profile with the running profile set runs the child command", async () => { + const f = await fixture({ profiles: ["toolchain"] }); + const result = await cli({ + fixture: f, + args: [ + "run", + "--profile", + "toolchain", + "toolchain", + "--", + "bun", + "--version", + ], + }); + expect(result.stderr).not.toMatch(STACK_FRAME); + expect(result.code).toBe(0); + expect(result.stdout).toBe(CHILD_STDOUT); + const calls = await nativeCalls(f.root); + expect(operations(calls)).toEqual([ + "refresh-dependencies", + "run-selection", + "run-service", + ]); + // The child argv reaches the runtime unchanged, after its separator. + expect(calls[2]?.endsWith(" -- bun --version")).toBe(true); +}, 30_000); + +test("a matching --env and an unordered, repeated profile list also run", async () => { + const f = await fixture({ + profiles: ["toolchain", "tools"], + effectiveEnvName: null, + }); + const result = await cli({ + fixture: f, + args: [ + "run", + "--env", + "base", + "--profile", + "toolchain,tools,toolchain", + "toolchain", + "bun", + "--version", + ], + }); + expect(result.code).toBe(0); + expect(result.stdout).toBe(CHILD_STDOUT); + expect(operations(await nativeCalls(f.root))).toContain("run-service"); +}, 30_000); + +for (const request of [ + { name: "a different profile", args: ["--profile", "web"] }, + { name: "a subset of the running profiles", args: ["--profile", "tools"] }, + { name: "an extra profile", args: ["--profile", "toolchain,tools,web"] }, + { name: "a different env", args: ["--env", "staging"] }, +]) { + test(`run with ${request.name} refuses with one closed line and no runtime request`, async () => { + const f = await fixture({ profiles: ["toolchain", "tools"] }); + const result = await cli({ + fixture: f, + args: ["run", ...request.args, "toolchain", "bun", "--version"], + }); + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr.trim()).toBe( + `ERROR: E_USAGE: ${NATIVE_RUN_SELECTION_REFUSAL}` + ); + expect(result.stderr).not.toContain("hack [options]"); + expect(result.stderr).not.toContain("--help"); + expect(result.stderr).not.toMatch(STACK_FRAME); + expect(await nativeCalls(f.root)).toEqual([]); + }, 30_000); +} diff --git a/tests/native-runtime-not-running-cli.test.ts b/tests/native-runtime-not-running-cli.test.ts index 161024896..b85781e09 100644 --- a/tests/native-runtime-not-running-cli.test.ts +++ b/tests/native-runtime-not-running-cli.test.ts @@ -9,6 +9,7 @@ import { } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; +import { NATIVE_RUN_SELECTION_REFUSAL } from "../src/backends/native-project-one-off.ts"; import { saveNativeProjectRun } from "../src/backends/native-project-run.ts"; import { privateCliEnv, privateCliHome } from "./helpers/private-cli-env.ts"; @@ -219,7 +220,7 @@ test("HACK_DEBUG=1 adds the stack to a closed native refusal", async () => { expect(result.stdout + result.stderr).toMatch(STACK_FRAME); }); -test("a refused native run prints the refusal and one hint, not the help or Hack's version", async () => { +test("a native run with a differing profile prints one closed refusal, not the help or Hack's version", async () => { const f = await fixture({ nativeCode: "runtime_not_running" }); const result = await cli({ fixture: f, @@ -227,10 +228,8 @@ test("a refused native run prints the refusal and one hint, not the help or Hack }); const output = result.stdout + result.stderr; expect(result.code).toBe(1); - expect(output).toContain( - "Native run uses the started project's environment and profiles; restart with the requested selections first." - ); - expect(output).toContain("See `hack run --help` for usage."); + // The saved run started with no profiles; `toolchain` differs (HACK-1276). + expect(output.trim()).toBe(`ERROR: E_USAGE: ${NATIVE_RUN_SELECTION_REFUSAL}`); expect(output).not.toContain("hack [options]"); expect(result.stdout).not.toMatch(/^hack v\d/m); expect(output).not.toMatch(STACK_FRAME); From 2774d1d49081a8fd06d4c04390cbc73dac70904c Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 14:07:47 -0400 Subject: [PATCH 11/14] fix(cli): parse options per resolved command so -p means --path again argv was parsed once against the union of every command's options keyed by long name, so the last declaration of a short flag won everywhere: `-p` became `ssh --port` for every command that declares `--path -p`, and run.ts refused `hack up -p ` with `Option(s) not valid for "up": --port`. Parse in two passes instead. The first resolves the command path (and the child-command boundary) reading each option with the options of the command resolved so far; the second parses with that command's options plus the global and built-in ones. Options only other commands declare are still parsed by long name so they keep their "not valid for " refusal; their short flags never leak. The per-scope guard test replaces the cross-tree type guard. Refs HACK-1293 Co-Authored-By: Claude Opus 5.5 --- src/cli/command.ts | 319 +++++++++++++++++++------- tests/cli-command.test.ts | 48 ++-- tests/cli-per-command-options.test.ts | 257 +++++++++++++++++++++ 3 files changed, 526 insertions(+), 98 deletions(-) create mode 100644 tests/cli-per-command-options.test.ts diff --git a/src/cli/command.ts b/src/cli/command.ts index 1dc48c438..d70270a2c 100644 --- a/src/cli/command.ts +++ b/src/cli/command.ts @@ -229,28 +229,40 @@ export interface ParsedCliInvocation { readonly positionals: readonly string[]; } +/** + * Parses argv in two passes. The first resolves the command path from the + * leading positionals ({@link scanInvocation}); the second parses with that + * command's own options plus the global and built-in ones, so each command + * keeps its own short flags and option types: `-p` is `--path` for project + * commands and `--port` for `ssh`, and one long name may be a flag in one + * command and take a value in another. + * + * An option only other commands declare is still parsed by its long name + * (never by its short flag), so the caller can refuse it as not valid for the + * resolved command rather than as unknown. + */ export function parseCliArgv( cli: CliSpec, argv: readonly string[], opts?: { readonly allowUnknownOptions?: boolean } ): ParsedCliInvocation { - const childStart = findPassthroughStart(cli, argv); + const { command, childStart } = scanInvocation(cli, argv); + const table = optionTableFor(cli, command); const normalizedArgv = childStart === null - ? normalizeArgvForDefaults(cli, argv) + ? normalizeArgvForDefaults(table, argv) : [ - ...normalizeArgvForDefaults(cli, argv.slice(0, childStart)), + ...normalizeArgvForDefaults(table, argv.slice(0, childStart)), // End Hack's option parsing: the child argv is passed on verbatim. "--", ...argv.slice(childStart), ]; - const options = buildUnionParseOptions(cli); let parsed: unknown; try { parsed = bunParseArgs({ args: [...normalizedArgv], - options, + options: parseArgsOptions(table), strict: opts?.allowUnknownOptions !== true, allowPositionals: true, }); @@ -276,16 +288,9 @@ export function parseCliArgv( } function normalizeArgvForDefaults( - cli: CliSpec, + table: OptionTable, argv: readonly string[] ): string[] { - const defaultable = new Map(); - for (const opt of [...builtinOptions(), ...collectAllOptions(cli)]) { - if (isDefaultableOption(opt)) { - defaultable.set(opt.long, opt); - } - } - const out: string[] = []; for (let i = 0; i < argv.length; i += 1) { const token = argv[i] ?? ""; @@ -300,8 +305,8 @@ function normalizeArgvForDefaults( continue; } - const opt = defaultable.get(token); - if (!opt) { + const opt = table.byLong.get(token); + if (!(opt && isDefaultableOption(opt))) { out.push(token); continue; } @@ -322,51 +327,106 @@ function normalizeArgvForDefaults( * marked {@link PositionalSpec.passthrough}, or null when there is none or an * explicit `--` already ends Hack's options before it. * - * The scan mirrors the parser: it resolves the command path from leading - * positionals and skips the value token of each value-taking option, including - * the omitted-value form of options with a default. Without this boundary, - * `hack run web bun --version` would let Hack's own `--version` (or `--help`, - * `--json`, `--path`...) consume a flag meant for the child. + * Without this boundary, `hack run web bun --version` would let Hack's own + * `--version` (or `--help`, `--json`, `--path`...) consume a flag meant for + * the child. See {@link scanInvocation} for how option values are skipped. */ export function findPassthroughStart( cli: CliSpec, argv: readonly string[] ): number | null { - const valueOptions = collectValueOptions(cli); - let subcommands: readonly AnyCommandSpec[] = cli.commands; - let command: AnyCommandSpec | null = null; - let resolving = true; - let positionalIndex = 0; + return scanInvocation(cli, argv).childStart; +} + +/** The command an argv resolves to, and where its child argv starts. */ +interface ScannedInvocation { + readonly command: AnyCommandSpec | null; + readonly childStart: number | null; +} + +/** + * First parse pass: resolves the command path from the leading positionals + * and finds the child-command boundary, as the second pass will read them. + * + * Each option token is read with the options of the command resolved so far + * (plus the global and built-in ones), and its value token is skipped, + * including the omitted-value form of options with a default. An option that + * command does not declare, such as one placed before the command name, is + * read as the other declarations of that spelling read it; when they disagree + * on taking a value, it is read as a flag. After an explicit `--` every token + * is a positional and there is no child boundary. + */ +function scanInvocation( + cli: CliSpec, + argv: readonly string[] +): ScannedInvocation { + const state: ScanState = { + command: null, + subcommands: cli.commands, + separated: false, + positionalIndex: 0, + }; for (let i = 0; i < argv.length; i += 1) { const token = argv[i] ?? ""; - if (token === "--") { - return null; - } - if (token.startsWith("-") && token !== "-") { - if (consumesNextToken({ token, nextToken: argv[i + 1], valueOptions })) { - i += 1; + if (state.separated || token === "-" || !token.startsWith("-")) { + const step = scanPositional(state, token); + if (step !== "next") { + return { + command: state.command, + childStart: step === "child" ? i : null, + }; } - continue; - } - const next = resolving - ? subcommands.find((c) => c.name === token) - : undefined; - if (next) { - command = next; - subcommands = next.subcommands; - continue; - } - resolving = false; - const start = passthroughPositionalIndex(command); - if (start === null) { - return null; - } - if (positionalIndex === start) { - return i; + } else if (token === "--") { + state.separated = true; + } else if ( + consumesNextToken({ + opt: scanOption({ cli, command: state.command, token }), + token, + nextToken: argv[i + 1], + }) + ) { + i += 1; } - positionalIndex += 1; } - return null; + return { command: state.command, childStart: null }; +} + +interface ScanState { + command: AnyCommandSpec | null; + subcommands: readonly AnyCommandSpec[]; + /** An explicit `--` was seen: every later token is a positional. */ + separated: boolean; + /** Index among the resolved command's own positionals. */ + positionalIndex: number; +} + +/** + * Reads one positional token: a subcommand name extends the path; otherwise + * it is the command's own positional, which may be where the child argv + * starts ("child") or show there is no child boundary ("done"). + */ +function scanPositional( + state: ScanState, + token: string +): "next" | "child" | "done" { + const next = state.subcommands.find((c) => c.name === token); + if (next) { + state.command = next; + state.subcommands = next.subcommands; + return "next"; + } + const start = state.separated + ? null + : passthroughPositionalIndex(state.command); + if (start === null) { + return "done"; + } + if (state.positionalIndex === start) { + return "child"; + } + state.subcommands = []; + state.positionalIndex += 1; + return "next"; } /** @@ -388,32 +448,35 @@ export function hackOwnedArgv( return argv.slice(0, end); } -/** Every value-taking option, by long and short spelling. */ -function collectValueOptions(cli: CliSpec): ReadonlyMap { - const valueOptions = new Map(); - for (const opt of [...builtinOptions(), ...collectAllOptions(cli)]) { - if (opt.type === "boolean") { - continue; - } - valueOptions.set(opt.long, opt); - if (opt.short) { - valueOptions.set(opt.short, opt); - } +/** The option an option token names while `command` is resolved so far. */ +function scanOption(opts: { + readonly cli: CliSpec; + readonly command: AnyCommandSpec | null; + readonly token: string; +}): OptionSpec | undefined { + if (opts.token.includes("=")) { + return; } - return valueOptions; + const table = optionTableFor(opts.cli, opts.command); + if (opts.token.startsWith("--")) { + return table.byLong.get(opts.token); + } + return ( + table.byShort.get(opts.token) ?? + declarationsBySpelling(opts.cli).byShort.get(opts.token) + ); } /** True when an option token takes the following token as its value. */ function consumesNextToken(opts: { + readonly opt: OptionSpec | undefined; readonly token: string; readonly nextToken: string | undefined; - readonly valueOptions: ReadonlyMap; }): boolean { - const opt = opts.token.includes("=") - ? undefined - : opts.valueOptions.get(opts.token); + const { opt } = opts; return ( opt !== undefined && + opt.type !== "boolean" && !omitsOptionValue({ token: opts.token, opt, nextToken: opts.nextToken }) ); } @@ -459,20 +522,122 @@ function shouldApplyDefault(opts: { return false; } -function buildUnionParseOptions( - cli: CliSpec +/** The options one command parses with, by long and by short spelling. */ +interface OptionTable { + readonly byLong: ReadonlyMap; + readonly byShort: ReadonlyMap; +} + +const commandOptionTables = new WeakMap< + CliSpec, + Map +>(); + +/** + * The parse table for `command` (null before a command resolves): its own + * options plus the global and built-in ones, by long and short spelling, then + * each option only other commands declare, by long spelling alone. A short + * flag never carries over from another command. + */ +function optionTableFor( + cli: CliSpec, + command: AnyCommandSpec | null +): OptionTable { + const cached = commandOptionTables.get(cli)?.get(command); + if (cached) { + return cached; + } + const byLong = new Map(); + const byShort = new Map(); + const scope = [ + ...builtinOptions(), + ...cli.globalOptions, + ...(command?.options ?? []), + ]; + for (const opt of scope) { + byLong.set(opt.long, opt); + if (opt.short) { + byShort.set(opt.short, opt); + } + } + for (const [long, opt] of declarationsBySpelling(cli).byLong) { + if (!byLong.has(long)) { + byLong.set(long, opt); + } + } + const table: OptionTable = { byLong, byShort }; + const tables = + commandOptionTables.get(cli) ?? + new Map(); + tables.set(command, table); + commandOptionTables.set(cli, tables); + return table; +} + +const declarationTables = new WeakMap(); + +/** + * One representative per option spelling across the whole command tree, for + * reading an option outside a scope that declares it. When the declarations + * disagree on taking a value, the representative is a flag, so it never + * consumes a token that may be a command name or a positional. + */ +function declarationsBySpelling(cli: CliSpec): OptionTable { + const cached = declarationTables.get(cli); + if (cached) { + return cached; + } + const longs = new Map(); + const shorts = new Map(); + for (const opt of [...builtinOptions(), ...collectAllOptions(cli)]) { + longs.set(opt.long, [...(longs.get(opt.long) ?? []), opt]); + if (opt.short) { + shorts.set(opt.short, [...(shorts.get(opt.short) ?? []), opt]); + } + } + const table: OptionTable = { + byLong: representativeDeclarations(longs), + byShort: representativeDeclarations(shorts), + }; + declarationTables.set(cli, table); + return table; +} + +function representativeDeclarations( + declarations: ReadonlyMap +): ReadonlyMap { + const out = new Map(); + for (const [spelling, opts] of declarations) { + const last = opts.at(-1); + if (!last) { + continue; + } + const kinds = new Set(opts.map((opt) => opt.type === "boolean")); + out.set( + spelling, + kinds.size === 1 + ? last + : { ...last, type: "boolean", defaultValue: undefined } + ); + } + return out; +} + +function parseArgsOptions( + table: OptionTable ): Record { const out: Record = {}; - for (const opt of [...builtinOptions(), ...collectAllOptions(cli)]) { - const longName = opt.long.replace(LONG_OPTION_PREFIX_REGEX, ""); - const short = opt.short - ? opt.short.replace(SHORT_OPTION_PREFIX_REGEX, "") - : null; - out[longName] = - short === null - ? { type: opt.type === "boolean" ? "boolean" : "string" } - : { type: opt.type === "boolean" ? "boolean" : "string", short }; + for (const [long, opt] of table.byLong) { + out[long.replace(LONG_OPTION_PREFIX_REGEX, "")] = { + type: opt.type === "boolean" ? "boolean" : "string", + }; + } + for (const [short, opt] of table.byShort) { + const entry = out[opt.long.replace(LONG_OPTION_PREFIX_REGEX, "")]; + if (entry) { + entry.short = short.replace(SHORT_OPTION_PREFIX_REGEX, ""); + } } return out; } diff --git a/tests/cli-command.test.ts b/tests/cli-command.test.ts index d3eb7c4c4..e6a99405c 100644 --- a/tests/cli-command.test.ts +++ b/tests/cli-command.test.ts @@ -91,45 +91,51 @@ test("resolveCommand exposes host exec path", () => { }); /** - * argv is parsed once against the union of every command's options, keyed by - * long name, so one long name declared as a boolean in one command and as a - * value in another changes how every command parses it. A value-typed - * `setup migrate --rollback` once made `config adopt --rollback --json` consume - * `--json` as its value and fail with a usage error. + * argv is parsed with the resolved command's options plus the global and + * built-in ones, so a spelling may mean different options in different + * commands (`-p` is `--path` for project commands and `--port` for `ssh`). + * Within one command's scope, each long and short spelling must name exactly + * one option, or the parser would silently pick one of them. */ -test("each long option keeps one value type across the command tree", () => { - const declarations = new Map>(); - const record = (opts: { +test("each command scope declares every option spelling once", () => { + const conflicts: string[] = []; + const check = (opts: { readonly owner: string; readonly options: readonly OptionSpec[]; }) => { + const seen = new Map(); for (const option of opts.options) { - const kind = option.type === "boolean" ? "boolean" : "value"; - const owners = declarations.get(option.long) ?? new Set(); - owners.add(`${kind} (${opts.owner})`); - declarations.set(option.long, owners); + for (const spelling of [option.long, option.short]) { + if (spelling === undefined) { + continue; + } + const previous = seen.get(spelling); + if (previous !== undefined && previous !== option) { + conflicts.push( + `${opts.owner}: ${spelling} names ${previous.long} and ${option.long}` + ); + } + seen.set(spelling, option); + } } }; - record({ owner: "builtin", options: builtinOptions() }); - record({ owner: "global", options: CLI_SPEC.globalOptions }); + const shared = [...builtinOptions(), ...CLI_SPEC.globalOptions]; + check({ owner: "hack", options: shared }); const walk = ( commands: readonly AnyCommandSpec[], parent: readonly string[] ) => { for (const command of commands) { const path = [...parent, command.name]; - record({ owner: path.join(" "), options: command.options }); + check({ + owner: ["hack", ...path].join(" "), + options: [...shared, ...command.options], + }); walk(command.subcommands, path); } }; walk(CLI_SPEC.commands, []); - const conflicts = [...declarations] - .filter( - ([, owners]) => - new Set([...owners].map((owner) => owner.split(" ")[0])).size > 1 - ) - .map(([long, owners]) => `${long}: ${[...owners].sort().join(", ")}`); expect(conflicts).toEqual([]); }); diff --git a/tests/cli-per-command-options.test.ts b/tests/cli-per-command-options.test.ts new file mode 100644 index 000000000..008944a79 --- /dev/null +++ b/tests/cli-per-command-options.test.ts @@ -0,0 +1,257 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdtemp, realpath, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { + type AnyCommandSpec, + defineCli, + defineCommand, + defineOption, + findPassthroughStart, + parseCliArgv, + resolveCommand, +} from "../src/cli/command.ts"; +import { CLI_SPEC } from "../src/cli/spec.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * argv is parsed against the resolved command's own options plus the global + * and built-in ones, so a short flag or an option type declared by one command + * never changes how another command parses. `-p` is `--path` for project + * commands and `--port` for `ssh`. + */ + +function parse(argv: readonly string[]) { + const parsed = parseCliArgv(CLI_SPEC, argv); + const resolved = resolveCommand(CLI_SPEC, parsed.positionals); + return { + values: parsed.values, + path: resolved.path.map((command) => command.name), + rest: resolved.remainingPositionals, + }; +} + +/** Every command path whose own options include `--path` with `-p`. */ +function commandsWithShortPath(): readonly (readonly string[])[] { + const out: string[][] = []; + const walk = ( + commands: readonly AnyCommandSpec[], + parent: readonly string[] + ) => { + for (const command of commands) { + const path = [...parent, command.name]; + if ( + command.options.some( + (option) => option.long === "--path" && option.short === "-p" + ) + ) { + out.push(path); + } + walk(command.subcommands, path); + } + }; + walk(CLI_SPEC.commands, []); + return out; +} + +test("-p is --path for every command that declares it", () => { + const commands = commandsWithShortPath(); + for (const expected of [ + ["init"], + ["up"], + ["down"], + ["ps"], + ["logs"], + ["env", "list"], + ["config", "validate"], + ["setup", "migrate"], + ]) { + expect(commands).toContainEqual(expected); + } + for (const command of commands) { + const parsed = parse([...command, "-p", "/repo"]); + expect({ command, path: parsed.path, values: parsed.values }).toEqual({ + command, + path: [...command], + values: { path: "/repo" }, + }); + } +}); + +test("ssh -p sets --port", () => { + const parsed = parse(["ssh", "-p", "2222"]); + expect(parsed.path).toEqual(["ssh"]); + expect(parsed.values.port).toBe("2222"); + expect(parsed.values.path).toBeUndefined(); +}); + +test("a short flag before the command name still follows that command", () => { + expect(parse(["-p", "/repo", "up"]).values).toEqual({ path: "/repo" }); + expect(parse(["-p", "2222", "ssh"]).values).toEqual({ port: "2222" }); +}); + +test("an option another command declares is refused as not valid here", () => { + // Parsed by long name so the refusal can name it; never by its short flag. + expect(parse(["up", "--port", "2222"]).values).toEqual({ port: "2222" }); + expect(() => parse(["up", "-x"])).toThrow("Unknown option '-x'"); +}); + +test("config adopt --rollback stays boolean; setup migrate --rollback-manifest takes a value", () => { + const adopt = parse(["config", "adopt", "--rollback", "--json"]); + expect(adopt.path).toEqual(["config", "adopt"]); + expect(adopt.values).toEqual({ rollback: true, json: true }); + expect(adopt.rest).toEqual([]); + + const migrate = parse([ + "setup", + "migrate", + "--rollback-manifest", + "manifest.json", + "--json", + ]); + expect(migrate.path).toEqual(["setup", "migrate"]); + expect(migrate.values).toEqual({ + "rollback-manifest": "manifest.json", + json: true, + }); +}); + +const optFlag = defineOption({ + name: "rollback", + type: "boolean", + long: "--rollback", + short: "-r", + description: "Boolean in alpha", +} as const); + +const optValue = defineOption({ + name: "rollback", + type: "string", + long: "--rollback", + short: "-r", + valueHint: "", + description: "Value in beta", +} as const); + +const optTarget = defineOption({ + name: "target", + type: "string", + long: "--target", + short: "-t", + description: "Value in gamma, which also takes alpha's boolean --rollback", +} as const); + +const COLLIDING_CLI = defineCli({ + name: "demo", + version: "0.0.0", + summary: "Commands that reuse one spelling with different types", + globalOptions: [], + commands: [ + defineCommand({ + name: "alpha", + summary: "boolean --rollback", + group: "Project", + options: [optFlag], + positionals: [{ name: "rest", multiple: true }], + subcommands: [], + } as const), + defineCommand({ + name: "gamma", + summary: "child command", + group: "Project", + options: [optFlag, optTarget], + positionals: [{ name: "command", multiple: true, passthrough: true }], + subcommands: [], + } as const), + // Declared last, so a last-declaration-wins union would read `-r` and + // `--rollback` as taking a value everywhere. + defineCommand({ + name: "beta", + summary: "value --rollback", + group: "Project", + options: [optValue], + positionals: [{ name: "rest", multiple: true }], + subcommands: [], + } as const), + ], +} as const); + +test("one spelling may have a different type in each command", () => { + for (const flag of ["--rollback", "-r"]) { + const alpha = parseCliArgv(COLLIDING_CLI, ["alpha", flag, "keep"]); + expect(alpha.values).toEqual({ rollback: true }); + expect(alpha.positionals).toEqual(["alpha", "keep"]); + + const beta = parseCliArgv(COLLIDING_CLI, ["beta", flag, "file.json"]); + expect(beta.values).toEqual({ rollback: "file.json" }); + expect(beta.positionals).toEqual(["beta"]); + } +}); + +test("a spelling with disagreeing types before the command name never eats it", () => { + // Before `alpha` resolves, `-r` is a flag in alpha and takes a value in + // beta, so it is read as a flag and `alpha` stays the command name. + const alpha = parseCliArgv(COLLIDING_CLI, ["-r", "alpha", "keep"]); + expect(alpha.values).toEqual({ rollback: true }); + expect(alpha.positionals).toEqual(["alpha", "keep"]); +}); + +test("the child-command boundary uses the resolved command's option types", () => { + // In gamma `-r` is boolean, although beta declares it with a value: the + // child command starts right after it, and `-t` still takes `x`. + expect( + findPassthroughStart(COLLIDING_CLI, ["gamma", "-r", "-t", "x", "ls", "-r"]) + ).toBe(4); + const parsed = parseCliArgv(COLLIDING_CLI, [ + "gamma", + "-r", + "-t", + "x", + "ls", + "-r", + ]); + expect(parsed.values).toEqual({ rollback: true, target: "x" }); + expect(parsed.positionals).toEqual(["gamma", "ls", "-r"]); + expect(findPassthroughStart(COLLIDING_CLI, ["gamma", "-r", "ls"])).toBe(2); +}); + +const roots: string[] = []; +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +/** A source CLI spawn can take several seconds on a loaded runner. */ +const SPAWN_TIMEOUT_MS = 30_000; + +test( + "source CLI accepts -p as --path for a project command", + async () => { + const root = await realpath(await mkdtemp(join(tmpdir(), "hack-short-"))); + roots.push(root); + const child = Bun.spawn( + [process.execPath, resolve("index.ts"), "ps", "-p", root, "--json"], + { + cwd: root, + env: privateCliEnv({ + root, + env: { HACK_NO_INTERACTIVE: "1", HACK_LOGGER: "console" }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + timeout: 20_000, + } + ); + const [stdout, stderr] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + // No project in the fixture: `ps` itself refuses, but it got the path. + expect(stdout + stderr).not.toContain("--port"); + expect(stdout + stderr).not.toContain("not valid for"); + }, + SPAWN_TIMEOUT_MS +); From 9ee2beca2411106ec50da783081f3699e41c0646 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 14:09:56 -0400 Subject: [PATCH 12/14] fix(native): keep Compose-only projects on the Compose backend with closed refusals Native image-project `up` and `restart` now refuse an external Compose network, a reserved ownership label (com.docker.compose.*, io.hack-local.*, io.hack.native-config.*), a build outside the import preview's basic form, or a non-arm64 service platform before branch, env, lifecycle, storage or runtime work. Each refusal is one value-free E_NATIVE_PROJECT_UNSUPPORTED line that names the feature and the HACK_RUNTIME_BACKEND=compose selection. `hack doctor` reports the same features in a `compose backend features` check, and the native candidate guide lists which Compose features keep a project on the Compose backend. Refs HACK-1291 Co-Authored-By: Claude Opus 5.5 --- docs/guides/native-candidate.md | 50 ++- docs/reference/native-config-import.md | 7 + src/commands/doctor.ts | 48 +++ src/commands/project.ts | 7 + src/lib/doctor-compose-backend.ts | 58 +++ src/lib/native-compose-backend-features.ts | 253 ++++++++++++ ...ative-compose-backend-features-cli.test.ts | 376 ++++++++++++++++++ tests/native-compose-backend-features.test.ts | 243 +++++++++++ 8 files changed, 1041 insertions(+), 1 deletion(-) create mode 100644 src/lib/doctor-compose-backend.ts create mode 100644 src/lib/native-compose-backend-features.ts create mode 100644 tests/native-compose-backend-features-cli.test.ts create mode 100644 tests/native-compose-backend-features.test.ts diff --git a/docs/guides/native-candidate.md b/docs/guides/native-candidate.md index 50a6c6fb4..db51ad4dd 100644 --- a/docs/guides/native-candidate.md +++ b/docs/guides/native-candidate.md @@ -170,6 +170,52 @@ finalization checks still apply. Fresh and already branch-scoped graphs keep their normal branch namespace. This compatibility path does not authorize adopting an unrelated branch or project. +## Compose projects that stay on the Compose backend + +In v5, an unchanged Compose project (`.hack/hack.config.json` with +`.hack/docker-compose.yml`) that uses any of the following features runs on the +Compose backend. Native support for them is planned after v5. + +| Compose feature | Kept on the Compose backend when | +| --- | --- | +| External network | A top-level network sets `external: true` or `external: {name: ...}`. A reviewed `x-hack-isolated: true` replacement is not external: it creates an owned internal bridge. | +| Reserved ownership label | A service, network or volume label key starts with `com.docker.compose.`, `io.hack-local.` or `io.hack.native-config.`. Compose, the native runtime and native-config Compose use these labels to prove which resources they own. | +| Build outside the basic form | A service `build` is not the basic form that [`hack config import --dry-run`](../reference/native-config-import.md#pure-basic-build-preview) accepts (a relative context and optional relative `dockerfile` and `target`), or the service also sets `image`. Build arguments, secrets, SSH, cache options, platforms and other build options are outside the basic form. | +| Non-native platform | A service sets a `platform` other than `linux/arm64` or `linux/arm64/v8`, for example `linux/amd64`. The native VM runs Linux arm64 only. | + +Native `hack up` and `hack restart` check for these features after selecting the +project and before branch, environment, lifecycle, storage or runtime work. They +refuse with `E_NATIVE_PROJECT_UNSUPPORTED` and one line that names each feature +found, without network names, label keys, paths or platform values. Nothing is +started, stopped or written. Every service is checked, including services outside +the selected profiles. + +To run such a project, select the Compose backend: + +```sh +HACK_RUNTIME_BACKEND=compose hack up +``` + +The bundle's `hack-v5` entrypoint and the `hack-next` launcher accept the same +selection; see [Test both backends](https://github.com/hack-dance/hack/blob/next/docs/guides/candidate-install.md#test-both-backends). + +Other labels keep their native behavior. Routing (`caddy*`) and dependency cache +(`hack.dependencies.*`) labels are reviewed as before, and other ordinary labels +are refused with the general native configuration refusal. A basic build is also +refused by native image-project `up`, with the general refusal: it needs the +authored native format. + +`hack doctor` reports the same features for the current project in its +`compose backend features` check. With `--json`, the check's `composeBackend.features` +lists the closed names `external_network`, `reserved_label`, `unsupported_build` +and `foreign_platform`. On the native backend a found feature is a warning; on +the Compose backend it is informational. + +Native `up` and `restart` also stay in the foreground in every project: +`--detach` refuses with one `E_USAGE` line. Run `hack up` in its own terminal and +keep it open, use `hack ps` and `hack logs` from another terminal, and press +Ctrl-C to stop it. For detached startup, use the Compose backend. + ## Manual candidate upgrade and rollback Manual candidate bundles are selected by their full path. The separate @@ -667,7 +713,9 @@ Without that grant, `up` and `restart` refuse with `E_PERMISSION` before any sto runtime request; set it and run the command again. This initial foreground path does not support detached/JSON startup, selected -services or external networks. `--detach`, `--json`, `--target` and service names +services, or the Compose features listed in +[Compose projects that stay on the Compose backend](#compose-projects-that-stay-on-the-compose-backend), +such as external networks. `--detach`, `--json`, `--target` and service names each refuse with a one-line `E_USAGE` message that names the option and the supported form: run `hack up` in the foreground in its own terminal and use `hack ps` and `hack logs` from another. Routed services and host dependencies require the diff --git a/docs/reference/native-config-import.md b/docs/reference/native-config-import.md index 0bc970d14..d4e220a66 100644 --- a/docs/reference/native-config-import.md +++ b/docs/reference/native-config-import.md @@ -15,6 +15,13 @@ projects, read managed environment values or keys, or inspect/change engine resources. Any refusal discards the private candidate; no partial conversion is reported as successful. +An unchanged project that uses an external network, a reserved ownership label, +a build outside the basic form below, or a non-arm64 platform stays on the Compose +backend in v5. Native `up` refuses those features before any work, and +`hack doctor` reports them; see +[Compose projects that stay on the Compose backend](../guides/native-candidate.md#compose-projects-that-stay-on-the-compose-backend). +That check uses the same basic build mapping as this preview. + ## Supported subset | Legacy input | Native mapping | diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index a0c884842..cb55ad580 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -66,6 +66,11 @@ import { checkBrowserLocalNetwork, parseBrowserNetworkOptions, } from "../lib/doctor-browser-network.ts"; +import { + COMPOSE_BACKEND_CHECK, + type ComposeBackendReport, + composeBackendCheckResult, +} from "../lib/doctor-compose-backend.ts"; import { describeEnvKeyInspection, type EnvKeyInspection, @@ -117,6 +122,7 @@ import { type NativeCaDoctorDependencies, repairNativeCaTrust, } from "../lib/native-ca-doctor.ts"; +import { readComposeBackendFeatures } from "../lib/native-compose-backend-features.ts"; import { describeHackLocalRoute, HACK_LOCAL_ROLLBACK_COMMAND, @@ -199,6 +205,8 @@ interface CheckResult { readonly hackLocalRoute?: HackLocalRouteReport; /** Closed, value-free env key findings; project-relative subjects only. */ readonly envKey?: EnvKeyInspection; + /** Closed Compose-backend feature names only; never authored values. */ + readonly composeBackend?: ComposeBackendReport; } const HACK_LOCAL_ROUTE_CHECK = "native hack.local route"; @@ -855,6 +863,11 @@ const handleDoctor: CommandHandlerFor = async ({ checkComposeNetworkHygiene({ startDir }) ) ); + results.push( + await runCheck(s, COMPOSE_BACKEND_CHECK, () => + checkComposeBackendFeatures({ startDir, backend: "compose" }) + ) + ); results.push( await runCheck(s, "DEV_HOST", () => checkDevHost({ startDir })) ); @@ -1176,6 +1189,14 @@ async function handleNativeDoctor(opts: { { timeoutMs: 5000 } ) ); + results.push( + await runCheck(progress, COMPOSE_BACKEND_CHECK, () => + checkComposeBackendFeatures({ + startDir: opts.startDir, + backend: "native", + }) + ) + ); } progress.stop(`Native doctor checks complete (${results.length} checks)`); const hasError = results.some((result) => result.status === "error"); @@ -1340,6 +1361,8 @@ export type DoctorJsonCheck = { readonly hackLocalRoute?: HackLocalRouteReport; /** Present only on the env key check: closed codes, no key bytes or absolute paths. */ readonly envKey?: EnvKeyInspection; + /** Present only on the Compose-backend feature check: closed feature names. */ + readonly composeBackend?: ComposeBackendReport; }; export type DoctorJsonData = { @@ -1375,6 +1398,9 @@ export function buildDoctorJsonData(opts: { ? { hackLocalRoute: result.hackLocalRoute } : {}), ...(result.envKey ? { envKey: result.envKey } : {}), + ...(result.composeBackend + ? { composeBackend: result.composeBackend } + : {}), })), summary: buildDoctorSummaryLines({ results: opts.results }), counts, @@ -2971,6 +2997,28 @@ 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; value-free. + */ +async function checkComposeBackendFeatures(opts: { + readonly startDir: string; + readonly backend: "native" | "compose"; +}): Promise { + const ctx = await findProjectContext(opts.startDir); + if (!ctx) { + return { + name: COMPOSE_BACKEND_CHECK, + status: "warn", + message: `Skipped (no ${HACK_PROJECT_DIR_PRIMARY}/ found)`, + }; + } + return composeBackendCheckResult({ + backend: opts.backend, + features: await readComposeBackendFeatures(ctx.composeFile), + }); +} + async function checkProjectEnvKey({ startDir, }: { diff --git a/src/commands/project.ts b/src/commands/project.ts index 9ac23931c..fd8677404 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -216,6 +216,7 @@ import { resolveShouldTryLoki, resolveUseLoki, } from "../lib/logs.ts"; +import { refuseNativeComposeBackendFeatures } from "../lib/native-compose-backend-features.ts"; import { tryNativeComposeCommand } from "../lib/native-compose-command.ts"; import { tryNativeComposeOpen } from "../lib/native-compose-open.ts"; import { @@ -4933,6 +4934,12 @@ async function handleNativeUp({ projectOpt: args.options.project, touchRegistration: false, }); + // Before branch, config, env, lifecycle, storage or runtime work. `down` + // stays available for any graph a project already has. + await refuseNativeComposeBackendFeatures({ + composeFile: project.composeFile, + restart, + }); const branch = await resolveEffectiveBranchForCommand({ project, branchOption: args.options.branch, diff --git a/src/lib/doctor-compose-backend.ts b/src/lib/doctor-compose-backend.ts new file mode 100644 index 000000000..af31eea6a --- /dev/null +++ b/src/lib/doctor-compose-backend.ts @@ -0,0 +1,58 @@ +import { + COMPOSE_BACKEND_SELECTION, + type ComposeBackendFeature, + describeComposeBackendFeatures, +} from "./native-compose-backend-features.ts"; + +/** Doctor check id for the Compose-backend feature report. */ +export const COMPOSE_BACKEND_CHECK = "compose backend features"; + +/** Value-free JSON payload: closed feature names only. */ +export type ComposeBackendReport = { + readonly features: readonly ComposeBackendFeature[]; +}; + +/** + * Doctor result for the current project's Compose-backend features + * (HACK-1291). Read-only and value-free: it names closed features, never a + * network, label, path or platform value. + * + * On the native backend a blocking feature is a warning, because native `up` + * refuses the project; on the Compose backend it is informational, because + * the project already runs where those features are supported. + */ +export function composeBackendCheckResult(opts: { + readonly backend: "native" | "compose"; + readonly features: readonly ComposeBackendFeature[]; +}): { + readonly name: string; + readonly status: "ok" | "warn"; + readonly message: string; + readonly composeBackend: ComposeBackendReport; +} { + const composeBackend = { features: [...opts.features] }; + if (opts.features.length === 0) { + return { + name: COMPOSE_BACKEND_CHECK, + status: "ok", + message: + "No Compose-only features found; native up still reviews the rest of the configuration when it starts.", + composeBackend, + }; + } + const uses = `This project uses ${describeComposeBackendFeatures(opts.features)}`; + if (opts.backend === "native") { + return { + name: COMPOSE_BACKEND_CHECK, + status: "warn", + message: `${uses}, so native up refuses it and the project runs on the Compose backend. ${COMPOSE_BACKEND_SELECTION} to run it there.`, + composeBackend, + }; + } + return { + name: COMPOSE_BACKEND_CHECK, + status: "ok", + message: `${uses}, so it stays on the Compose backend; native up refuses these features.`, + composeBackend, + }; +} diff --git a/src/lib/native-compose-backend-features.ts b/src/lib/native-compose-backend-features.ts new file mode 100644 index 000000000..98c32db1f --- /dev/null +++ b/src/lib/native-compose-backend-features.ts @@ -0,0 +1,253 @@ +import { constants } from "node:fs"; +import { open } from "node:fs/promises"; +import { HackCliError } from "./cli-result.ts"; +import { isRecord } from "./guards.ts"; +import { mapLegacyComposeBuild } from "./native-config-import-build.ts"; + +/** + * Compose features that keep an unchanged Compose project on the Compose + * backend in v5 (HACK-1291). Native support for them is post-v5 work + * (HACK-1270). Each one is refused by native image-project `up` and `restart` + * before any runtime request, storage preparation, lifecycle hook or stop. + * + * The set is closed and ordered; it is part of the refusal and the doctor + * report contract. Messages and reports name only these features, never a + * network, label, path, platform or other authored value. + */ +export const COMPOSE_BACKEND_FEATURES = [ + "external_network", + "reserved_label", + "unsupported_build", + "foreign_platform", +] as const; + +/** One Compose feature from {@link COMPOSE_BACKEND_FEATURES}. */ +export type ComposeBackendFeature = (typeof COMPOSE_BACKEND_FEATURES)[number]; + +/** + * Label namespaces that prove resource ownership. A project must not declare + * them, because their consumers trust them as ownership evidence: + * + * - `com.docker.compose.*`: Docker Compose's own project, service and one-off + * labels, which the Compose backend reads to find a project's containers. + * - `io.hack-local.*`: native runtime graph ownership (`owner`, `graph`, + * `namespace`, `plan`, `kind`, `resource`, `job` and cache labels), which the + * runtime-core provider writes and verifies before cleanup. + * - `io.hack.native-config.*`: native-config Compose ownership (`version`, + * `instance`, `owner`, `generation`, `workload`, `storage`), which the + * native-config renderer writes and its generation inventory, route owner + * and storage witness verify. + * + * Other labels, including Hack's routing (`caddy*`), dependency cache + * (`hack.dependencies.*`) and one-shot markers, are not reserved: native `up` + * reviews them as it did before. + */ +export const RESERVED_OWNERSHIP_LABEL_PREFIXES = [ + "com.docker.compose.", + "io.hack-local.", + "io.hack.native-config.", +] as const; + +/** The native VM runs Linux arm64; registry admission selects only this platform. */ +const NATIVE_PLATFORMS = new Set(["linux/arm64", "linux/arm64/v8"]); + +/** The same bound native image-project input applies to the Compose file. */ +const COMPOSE_LIMIT = 256 * 1024; + +const FEATURE_PHRASES: Readonly> = { + external_network: "an external Compose network", + reserved_label: + "a label in a reserved ownership namespace (com.docker.compose.*, io.hack-local.* or io.hack.native-config.*)", + unsupported_build: + "a Compose build outside the basic context, dockerfile and target form", + foreign_platform: "a service platform other than linux/arm64", +}; + +function labelKeys(labels: unknown): readonly string[] { + if (isRecord(labels)) { + return Object.keys(labels); + } + if (Array.isArray(labels)) { + return labels + .filter((label): label is string => typeof label === "string") + .map((label) => label.split("=", 1)[0] ?? ""); + } + return []; +} + +function hasReservedLabel(labels: unknown): boolean { + return labelKeys(labels).some((key) => + RESERVED_OWNERSHIP_LABEL_PREFIXES.some((prefix) => key.startsWith(prefix)) + ); +} + +/** + * Compose treats `external: true` and the legacy `external: {name: ...}` form + * as a network the project does not own. The runtime's reviewed + * `x-hack-isolated: true` replacement is supported natively: it creates an + * owned internal bridge instead of joining the external one. + */ +function isExternalNetwork(network: unknown): boolean { + if (!isRecord(network)) { + return false; + } + const external = network.external; + const declared = external === true || isRecord(external); + return declared && network["x-hack-isolated"] !== true; +} + +/** + * A build is inside the supported path only when the import preview's basic + * build mapper accepts it and the service names no image, exactly as + * `hack config import --dry-run` reports `invalid_or_unsupported_build` and + * `image_build_exclusive`. + */ +function isUnsupportedBuild(service: Record): boolean { + if (!Object.hasOwn(service, "build")) { + return false; + } + return ( + Object.hasOwn(service, "image") || + mapLegacyComposeBuild(service.build) === undefined + ); +} + +function isForeignPlatform(service: Record): boolean { + return ( + Object.hasOwn(service, "platform") && + !( + typeof service.platform === "string" && + NATIVE_PLATFORMS.has(service.platform) + ) + ); +} + +function topLevelEntries( + compose: Record, + key: string +): unknown[] { + const value = compose[key]; + return isRecord(value) ? Object.values(value) : []; +} + +/** + * Classifies a parsed Compose document. Returns the blocking features in + * {@link COMPOSE_BACKEND_FEATURES} order, each at most once. Every service is + * inspected, including services outside the selected profiles, because the + * native path reviews all declarations. Shapes this classifier does not + * recognize are left to native review, which refuses them as before. + */ +export function findComposeBackendFeatures( + compose: unknown +): ComposeBackendFeature[] { + if (!isRecord(compose)) { + return []; + } + const found = new Set(); + const services = topLevelEntries(compose, "services").filter(isRecord); + if (topLevelEntries(compose, "networks").some(isExternalNetwork)) { + found.add("external_network"); + } + const resources = [ + ...topLevelEntries(compose, "networks"), + ...topLevelEntries(compose, "volumes"), + ].filter(isRecord); + if ( + services.some((service) => hasReservedLabel(service.labels)) || + resources.some((resource) => hasReservedLabel(resource.labels)) + ) { + found.add("reserved_label"); + } + if (services.some(isUnsupportedBuild)) { + found.add("unsupported_build"); + } + if (services.some(isForeignPlatform)) { + found.add("foreign_platform"); + } + return COMPOSE_BACKEND_FEATURES.filter((feature) => found.has(feature)); +} + +/** + * Reads and classifies one Compose file with the bounds native image-project + * input uses. Read-only: no lock, no write, no runtime request. A file that is + * missing, oversized, a symlink or not parseable returns no features, so the + * existing native input refusal still reports it. + */ +export async function readComposeBackendFeatures( + composeFile: string +): Promise { + let text: string; + try { + const file = await open( + composeFile, + constants.O_RDONLY | constants.O_NONBLOCK | constants.O_NOFOLLOW + ); + try { + // One byte past the bound detects an oversized or growing file. + const buffer = Buffer.alloc(COMPOSE_LIMIT + 1); + const { bytesRead } = await file.read(buffer, 0, buffer.length, 0); + if (bytesRead > COMPOSE_LIMIT) { + return []; + } + text = new TextDecoder("utf-8", { fatal: true }).decode( + buffer.subarray(0, bytesRead) + ); + } finally { + await file.close(); + } + } catch { + return []; + } + try { + return findComposeBackendFeatures(Bun.YAML.parse(text)); + } catch { + return []; + } +} + +/** Value-free phrase list, such as "a and b" or "a, b and c". */ +export function describeComposeBackendFeatures( + features: readonly ComposeBackendFeature[] +): string { + const phrases = features.map((feature) => FEATURE_PHRASES[feature]); + if (phrases.length <= 1) { + return phrases[0] ?? ""; + } + return `${phrases.slice(0, -1).join(", ")} and ${phrases.at(-1)}`; +} + +/** The selection that runs this project on the Compose backend. */ +export const COMPOSE_BACKEND_SELECTION = + "Set HACK_RUNTIME_BACKEND=compose (hack-next and hack-v5 accept it too)"; + +/** + * The closed refusal for a project that needs the Compose backend: stable code + * `E_NATIVE_PROJECT_UNSUPPORTED`, one line, and a `detail` that lists only the + * closed feature names. + */ +export function composeBackendRefusal(opts: { + readonly features: readonly ComposeBackendFeature[]; + readonly restart: boolean; +}): HackCliError { + const command = opts.restart ? "hack restart" : "hack up"; + return new HackCliError({ + code: "E_NATIVE_PROJECT_UNSUPPORTED", + message: `Native \`${command}\` does not support ${describeComposeBackendFeatures(opts.features)}; this project runs on the Compose backend. ${COMPOSE_BACKEND_SELECTION} and run the command again. Nothing was started or stopped.`, + detail: { backend: "compose", features: [...opts.features] }, + }); +} + +/** + * Refuses native image-project `up` or `restart` for a Compose project that + * needs the Compose backend. Call it after project selection and before any + * runtime request, storage preparation, lifecycle hook or stop. + */ +export async function refuseNativeComposeBackendFeatures(opts: { + readonly composeFile: string; + readonly restart: boolean; +}): Promise { + const features = await readComposeBackendFeatures(opts.composeFile); + if (features.length > 0) { + throw composeBackendRefusal({ features, restart: opts.restart }); + } +} diff --git a/tests/native-compose-backend-features-cli.test.ts b/tests/native-compose-backend-features-cli.test.ts new file mode 100644 index 000000000..c80384bc9 --- /dev/null +++ b/tests/native-compose-backend-features-cli.test.ts @@ -0,0 +1,376 @@ +import { afterEach, expect, test } from "bun:test"; +import { + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, relative, resolve } from "node:path"; +import { NATIVE_CONFIGURATION_REFUSAL } from "../src/backends/native-project-start.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1291: a real Compose project that uses an external network, a reserved + * ownership label, an advanced build or a non-arm64 platform stays on the + * Compose backend. Native `up` and `restart` refuse each one through the real + * CLI with one closed `E_NATIVE_PROJECT_UNSUPPORTED` line that names the + * feature and the Compose selection, before any runtime request or write. + * + * The stub runtime records every request and answers only the scripted + * `runtime check-project-share` reply, so a refusal that came after a runtime + * request or a write would show up in `calls` or in the file tree. Nothing + * here starts a real runtime, Docker or a VM. + */ + +const roots: string[] = []; +const entrypoint = resolve("index.ts"); +const STACK_FRAME = /^\s+at /m; +const SPAWN_TIMEOUT_MS = 60_000; +const IMAGE = `sha256:${"e".repeat(64)}`; +const SELECTION = + "this project runs on the Compose backend. Set HACK_RUNTIME_BACKEND=compose (hack-next and hack-v5 accept it too) and run the command again. Nothing was started or stopped."; + +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +async function fixture(compose: Record) { + const root = await realpath( + await mkdtemp(join(tmpdir(), "native-compose-backend-cli-")) + ); + roots.push(root); + const project = join(root, "project"); + const projectDir = join(project, ".hack"); + const nativeHome = join(root, "native-home"); + const runtimeDir = join(root, "runtime"); + const stub = join(root, "stub"); + await mkdir(projectDir, { recursive: true }); + await mkdir(nativeHome, { mode: 0o700 }); + await mkdir(runtimeDir); + await mkdir(stub); + await writeFile( + join(projectDir, "docker-compose.yml"), + JSON.stringify(compose) + ); + await writeFile( + join(projectDir, "hack.config.json"), + JSON.stringify({ name: "backend-fixture" }) + ); + await writeFile(join(runtimeDir, "hack-relay-guest"), "synthetic relay"); + const binary = join(runtimeDir, "native-runtime"); + await writeFile( + binary, + `#!/bin/sh +printf '%s\\n' "$*" >> '${stub}/calls' +if [ -f '${stub}/'"$3-$4"'.out' ]; then cat '${stub}/'"$3-$4"'.out'; exit 0; fi +printf '%s' '{"code":"runtime_not_running"}' >&2 +exit 1 +`, + { mode: 0o700 } + ); + await writeFile( + join(stub, "runtime-check-project-share.out"), + JSON.stringify({ source_admitted: true, pool_initialized: true }) + ); + const calls = async () => + (await readFile(join(stub, "calls"), "utf8").catch(() => "")) + .split("\n") + .filter(Boolean) + .map((line) => line.split(" ").slice(2, 4).join(" ")); + const env = (extra: Record) => + privateCliEnv({ + root, + env: { + HACK_RUNTIME_BACKEND: "native", + HACK_NATIVE_BINARY: binary, + HACK_NATIVE_HOME: nativeHome, + HACK_NO_INTERACTIVE: "1", + HACK_LOGGER: "console", + ...extra, + }, + }); + return { root, project, calls, env }; +} + +type Fixture = Awaited>; + +async function cli( + f: Fixture, + args: readonly string[], + env: Record = { HACK_NATIVE_SHARED_SOURCE: "1" } +) { + const child = Bun.spawn( + [process.execPath, entrypoint, "--path", f.project, ...args], + { + cwd: f.root, + env: f.env(env), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + timeout: SPAWN_TIMEOUT_MS - 5000, + } + ); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { code, output: stdout + stderr }; +} + +/** + * Every file under the fixture except the stub's own request log and Bun's + * transpiler cache, which the spawned source CLI writes under its HOME. + */ +async function tree(root: string): Promise { + const entries = await readdir(root, { recursive: true }); + return entries + .map((entry) => relative(root, join(root, entry))) + .filter( + (entry) => + !( + entry.startsWith("stub") || + entry.startsWith(join("user-home", "Library")) + ) + ) + .sort(); +} + +function expectOneClosedLine( + result: Awaited>, + line: string +): void { + expect(result.code).toBe(1); + const errors = result.output + .split("\n") + .filter((text) => text.startsWith("ERROR:")); + expect(errors).toEqual([`ERROR: ${line}`]); + expect(result.output).not.toMatch(STACK_FRAME); + expect(result.output).not.toContain("Error: "); + expect(result.output).not.toContain("hack [options]"); +} + +/** Refused with the Compose-backend line, and nothing was requested or written. */ +async function expectComposeBackendRefusal(opts: { + readonly compose: Record; + readonly phrase: string; + readonly args?: readonly string[]; + readonly env?: Record; + readonly command?: string; + /** Authored values that must not appear in the output. */ + readonly secrets?: readonly string[]; +}): Promise { + const f = await fixture(opts.compose); + // Building the child env creates the private HOME and Docker client config. + f.env({}); + const before = await tree(f.root); + const result = await cli(f, opts.args ?? ["up"], opts.env); + expectOneClosedLine( + result, + `E_NATIVE_PROJECT_UNSUPPORTED: Native \`${opts.command ?? "hack up"}\` does not support ${opts.phrase}; ${SELECTION}` + ); + for (const secret of opts.secrets ?? []) { + expect(result.output).not.toContain(secret); + } + expect(await f.calls()).toEqual([]); + expect(await tree(f.root)).toEqual(before); +} + +test( + "an external Compose network refuses native up before any request or write", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { web: { image: IMAGE, networks: ["shared-dev-net"] } }, + networks: { "shared-dev-net": { external: true } }, + }, + phrase: "an external Compose network", + secrets: ["shared-dev-net"], + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "a reserved ownership label refuses native up before any request or write", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { + web: { + image: IMAGE, + labels: { "io.hack-local.owner": "borrowed-owner-token" }, + }, + }, + }, + phrase: + "a label in a reserved ownership namespace (com.docker.compose.*, io.hack-local.* or io.hack.native-config.*)", + secrets: ["borrowed-owner-token", "io.hack-local.owner"], + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "a build outside the basic form refuses native up before any request or write", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { + web: { build: { context: "..", args: { TOKEN_NAME: "build-arg" } } }, + }, + }, + phrase: + "a Compose build outside the basic context, dockerfile and target form", + secrets: ["TOKEN_NAME", "build-arg"], + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "a non-arm64 service platform refuses native up before any request or write", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { web: { image: IMAGE, platform: "linux/amd64" } }, + }, + phrase: "a service platform other than linux/arm64", + secrets: ["linux/amd64"], + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "all features are named in one line, ahead of the shared-source grant", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { + web: { + image: IMAGE, + platform: "linux/amd64", + labels: ["com.docker.compose.project=other"], + }, + api: { image: IMAGE, build: ".." }, + }, + networks: { edge: { external: { name: "edge" } } }, + }, + env: {}, + phrase: + "an external Compose network, a label in a reserved ownership namespace (com.docker.compose.*, io.hack-local.* or io.hack.native-config.*), a Compose build outside the basic context, dockerfile and target form and a service platform other than linux/arm64", + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "native restart refuses the same features before any stop", + async () => { + await expectComposeBackendRefusal({ + compose: { + services: { web: { image: IMAGE, networks: ["edge"] } }, + networks: { edge: { external: true } }, + }, + args: ["restart"], + command: "hack restart", + phrase: "an external Compose network", + }); + }, + SPAWN_TIMEOUT_MS +); + +test( + "control: ordinary labels and a basic build keep today's native refusal", + async () => { + for (const service of [ + { image: IMAGE, labels: { "com.example.team": "web" } }, + { build: ".." }, + ]) { + const f = await fixture({ services: { web: service } }); + const result = await cli(f, ["up"]); + expectOneClosedLine( + result, + `E_NATIVE_PROJECT_UNSUPPORTED: ${NATIVE_CONFIGURATION_REFUSAL}` + ); + expect(result.output).not.toContain("HACK_RUNTIME_BACKEND=compose"); + // The check passed: native up went on to its own source preflight. + expect(await f.calls()).toEqual(["runtime check-project-share"]); + } + }, + SPAWN_TIMEOUT_MS +); + +test( + "control: a reviewed isolated network replacement and arm64 platform pass the check", + async () => { + const f = await fixture({ + services: { + web: { + image: IMAGE, + platform: "linux/arm64", + labels: { "com.example.team": "web" }, + networks: ["edge"], + }, + }, + networks: { edge: { external: true, "x-hack-isolated": true } }, + }); + const result = await cli(f, ["up"]); + expect(result.output).not.toContain("HACK_RUNTIME_BACKEND=compose"); + expect(await f.calls()).toEqual(["runtime check-project-share"]); + }, + SPAWN_TIMEOUT_MS +); + +test( + "native doctor reports the same features for the current project, value-free", + async () => { + const f = await fixture({ + services: { web: { image: IMAGE, platform: "linux/amd64" } }, + networks: { "shared-dev-net": { external: true } }, + }); + const result = await cli(f, ["doctor", "--json"], {}); + const envelope: unknown = JSON.parse( + result.output.slice(result.output.indexOf("{")) + ); + expect(envelope).toMatchObject({ + ok: true, + data: { + checks: expect.arrayContaining([ + expect.objectContaining({ + id: "compose backend features", + status: "warn", + composeBackend: { + features: ["external_network", "foreign_platform"], + }, + }), + ]), + }, + }); + expect(result.output).toContain("HACK_RUNTIME_BACKEND=compose"); + expect(result.output).not.toContain("shared-dev-net"); + expect(result.output).not.toContain("linux/amd64"); + }, + SPAWN_TIMEOUT_MS +); + +test( + "native up --detach keeps its one-line foreground-only refusal", + async () => { + const f = await fixture({ services: { web: { image: IMAGE } } }); + const result = await cli(f, ["up", "--detach"]); + expectOneClosedLine( + result, + "E_USAGE: Native `hack up` runs in the foreground only, so --detach is not supported. Run `hack up` without --detach in its own terminal and keep it open; use `hack ps` and `hack logs` from another terminal, and press Ctrl-C to stop it." + ); + expect(await f.calls()).toEqual([]); + }, + SPAWN_TIMEOUT_MS +); diff --git a/tests/native-compose-backend-features.test.ts b/tests/native-compose-backend-features.test.ts new file mode 100644 index 000000000..0b1bb4d8d --- /dev/null +++ b/tests/native-compose-backend-features.test.ts @@ -0,0 +1,243 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + COMPOSE_BACKEND_CHECK, + composeBackendCheckResult, +} from "../src/lib/doctor-compose-backend.ts"; +import { + composeBackendRefusal, + findComposeBackendFeatures, + RESERVED_OWNERSHIP_LABEL_PREFIXES, + readComposeBackendFeatures, + refuseNativeComposeBackendFeatures, +} from "../src/lib/native-compose-backend-features.ts"; +import { expectCodedRefusal } from "./helpers/closed-refusal.ts"; + +const IMAGE = "registry.example/app:1"; +const roots: string[] = []; + +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +function services(service: Record) { + return { services: { web: service } }; +} + +test("an image-only project has no Compose-backend features", () => { + expect(findComposeBackendFeatures(services({ image: IMAGE }))).toEqual([]); + expect(findComposeBackendFeatures(null)).toEqual([]); + expect(findComposeBackendFeatures({ services: [] })).toEqual([]); +}); + +test("external networks are blocking; owned and isolated replacements are not", () => { + const network = (definition: unknown) => ({ + ...services({ image: IMAGE }), + networks: { edge: definition }, + }); + expect(findComposeBackendFeatures(network({ external: true }))).toEqual([ + "external_network", + ]); + expect( + findComposeBackendFeatures(network({ external: { name: "edge" } })) + ).toEqual(["external_network"]); + for (const owned of [ + null, + {}, + { external: false }, + { driver: "bridge", internal: true }, + { external: true, "x-hack-isolated": true }, + ]) { + expect(findComposeBackendFeatures(network(owned))).toEqual([]); + } +}); + +test("each reserved ownership namespace is blocking in map and list labels", () => { + expect(RESERVED_OWNERSHIP_LABEL_PREFIXES).toEqual([ + "com.docker.compose.", + "io.hack-local.", + "io.hack.native-config.", + ]); + for (const key of [ + "com.docker.compose.project", + "io.hack-local.owner", + "io.hack.native-config.instance", + ]) { + expect( + findComposeBackendFeatures( + services({ image: IMAGE, labels: { [key]: "x" } }) + ) + ).toEqual(["reserved_label"]); + expect( + findComposeBackendFeatures( + services({ image: IMAGE, labels: [`${key}=x`] }) + ) + ).toEqual(["reserved_label"]); + expect( + findComposeBackendFeatures({ + ...services({ image: IMAGE }), + volumes: { data: { labels: { [key]: "x" } } }, + }) + ).toEqual(["reserved_label"]); + } +}); + +test("ordinary, routing, cache and one-shot labels are not reserved", () => { + for (const labels of [ + { "com.example.team": "web" }, + ["com.docker.composer=x", "io.hack-localx=1", "io.hack.other=1"], + { caddy: "web.hack", "caddy.reverse_proxy": "{{upstreams 3000}}" }, + { "hack.dependencies.bootstrap": "install" }, + { "hack.service.one-shot": "true" }, + ]) { + expect( + findComposeBackendFeatures(services({ image: IMAGE, labels })) + ).toEqual([]); + } +}); + +test("builds follow the import preview's basic build path", () => { + for (const build of [ + "..", + ".", + { context: "../app" }, + { context: "..", dockerfile: "docker/Dockerfile", target: "dev" }, + {}, + ]) { + expect(findComposeBackendFeatures(services({ build }))).toEqual([]); + } + for (const service of [ + { build: { context: "..", args: { A: "1" } } }, + { build: { context: "..", platforms: ["linux/amd64"] } }, + { build: "/absolute/context" }, + { build: "../../outside" }, + { build: { context: "..", dockerfile: "../../Dockerfile" } }, + { build: "..", image: IMAGE }, + ]) { + expect(findComposeBackendFeatures(services(service))).toEqual([ + "unsupported_build", + ]); + } +}); + +test("only the native linux/arm64 platform passes", () => { + for (const platform of ["linux/arm64", "linux/arm64/v8"]) { + expect( + findComposeBackendFeatures(services({ image: IMAGE, platform })) + ).toEqual([]); + } + for (const platform of [ + "linux/amd64", + "linux/arm/v7", + "${PLATFORM}", + 42, + null, + ]) { + expect( + findComposeBackendFeatures(services({ image: IMAGE, platform })) + ).toEqual(["foreign_platform"]); + } +}); + +test("features come back once each in closed order across services", () => { + expect( + findComposeBackendFeatures({ + services: { + a: { image: IMAGE, platform: "linux/amd64" }, + b: { build: { context: "..", ssh: ["default"] } }, + c: { image: IMAGE, labels: ["io.hack-local.kind=x"] }, + d: { image: IMAGE, platform: "linux/amd64", profiles: ["off"] }, + }, + networks: { a: { external: true }, b: { external: true } }, + }) + ).toEqual([ + "external_network", + "reserved_label", + "unsupported_build", + "foreign_platform", + ]); +}); + +test("the refusal is one coded line with closed detail and no values", async () => { + const root = await mkdtemp(join(tmpdir(), "compose-backend-")); + roots.push(root); + const file = join(root, "docker-compose.yml"); + await writeFile( + file, + [ + "services:", + " web:", + " image: private.registry/app:secret-tag", + " platform: linux/amd64", + " networks: [private-net]", + "networks:", + " private-net:", + " external: true", + "", + ].join("\n") + ); + const error = await expectCodedRefusal( + refuseNativeComposeBackendFeatures({ composeFile: file, restart: false }), + "E_NATIVE_PROJECT_UNSUPPORTED", + "Native `hack up` does not support an external Compose network and a service platform other than linux/arm64; this project runs on the Compose backend. Set HACK_RUNTIME_BACKEND=compose" + ); + expect(error.message).not.toContain("\n"); + for (const value of ["private-net", "secret-tag", "linux/amd64", root]) { + expect(error.message).not.toContain(value); + } + expect(error.detail).toEqual({ + backend: "compose", + features: ["external_network", "foreign_platform"], + }); + expect( + composeBackendRefusal({ features: ["reserved_label"], restart: true }) + .message + ).toStartWith("Native `hack restart` does not support a label"); +}); + +test("unreadable, oversized, symlinked or invalid Compose input is left to native review", async () => { + const root = await mkdtemp(join(tmpdir(), "compose-backend-")); + roots.push(root); + const blocking = "networks:\n edge:\n external: true\n"; + const real = join(root, "real.yml"); + await writeFile(real, blocking); + expect(await readComposeBackendFeatures(real)).toEqual(["external_network"]); + const link = join(root, "link.yml"); + await symlink(real, link); + const big = join(root, "big.yml"); + await writeFile(big, `${blocking}#${"x".repeat(256 * 1024)}\n`); + const invalid = join(root, "invalid.yml"); + await writeFile(invalid, "services: [unclosed\n"); + for (const path of [join(root, "missing.yml"), link, big, invalid]) { + expect(await readComposeBackendFeatures(path)).toEqual([]); + await refuseNativeComposeBackendFeatures({ + composeFile: path, + restart: false, + }); + } +}); + +test("doctor reports the same features value-free for each backend", () => { + const features = ["external_network"] as const; + const native = composeBackendCheckResult({ backend: "native", features }); + expect(native).toMatchObject({ + name: COMPOSE_BACKEND_CHECK, + status: "warn", + composeBackend: { features: ["external_network"] }, + }); + expect(native.message).toContain("an external Compose network"); + expect(native.message).toContain("HACK_RUNTIME_BACKEND=compose"); + expect( + composeBackendCheckResult({ backend: "compose", features }) + ).toMatchObject({ status: "ok", composeBackend: { features } }); + for (const backend of ["native", "compose"] as const) { + expect(composeBackendCheckResult({ backend, features: [] })).toMatchObject({ + status: "ok", + composeBackend: { features: [] }, + }); + } +}); From 76003b54ad8bfd1c6e859758c6f22a5179ccd901 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 16:51:16 -0400 Subject: [PATCH 13/14] fix(native): classify Compose-backend features after the native adaptation The HACK-1291 Compose-backend check read only the original Compose file, so an external network that a valid HACK_NATIVE_ADAPTATION selection isolates (the documented `isolatedNetworks` path) was refused before native start applied the adaptation. Native up, restart, restart --recover and native doctor now apply the selection's isolatedNetworks first. The selection is read only when the file has an external network; an invalid selection refuses with the same closed E_CONFIG_INVALID line native start gives, before any runtime request. Co-Authored-By: Claude Opus 5.5 --- docs/guides/native-candidate.md | 18 ++- docs/reference/native-config-import.md | 7 +- src/commands/doctor.ts | 10 +- src/commands/project.ts | 3 + src/lib/native-compose-backend-features.ts | 124 +++++++++++++-- ...ative-compose-backend-features-cli.test.ts | 140 +++++++++++++++++ tests/native-compose-backend-features.test.ts | 141 ++++++++++++++++++ 7 files changed, 419 insertions(+), 24 deletions(-) diff --git a/docs/guides/native-candidate.md b/docs/guides/native-candidate.md index db51ad4dd..4ab9653b1 100644 --- a/docs/guides/native-candidate.md +++ b/docs/guides/native-candidate.md @@ -178,7 +178,7 @@ Compose backend. Native support for them is planned after v5. | Compose feature | Kept on the Compose backend when | | --- | --- | -| External network | A top-level network sets `external: true` or `external: {name: ...}`. A reviewed `x-hack-isolated: true` replacement is not external: it creates an owned internal bridge. | +| External network | A top-level network sets `external: true` or `external: {name: ...}`. A reviewed `x-hack-isolated: true` replacement is not external: it creates an owned internal bridge. A network that the `HACK_NATIVE_ADAPTATION` selection (see [Frontend integration status](#frontend-integration-status)) lists in `isolatedNetworks` gets the same replacement, so it is not external either. | | Reserved ownership label | A service, network or volume label key starts with `com.docker.compose.`, `io.hack-local.` or `io.hack.native-config.`. Compose, the native runtime and native-config Compose use these labels to prove which resources they own. | | Build outside the basic form | A service `build` is not the basic form that [`hack config import --dry-run`](../reference/native-config-import.md#pure-basic-build-preview) accepts (a relative context and optional relative `dockerfile` and `target`), or the service also sets `image`. Build arguments, secrets, SSH, cache options, platforms and other build options are outside the basic form. | | Non-native platform | A service sets a `platform` other than `linux/arm64` or `linux/arm64/v8`, for example `linux/amd64`. The native VM runs Linux arm64 only. | @@ -190,6 +190,15 @@ found, without network names, label keys, paths or platform values. Nothing is started, stopped or written. Every service is checked, including services outside the selected profiles. +The check applies the explicit `HACK_NATIVE_ADAPTATION` selection first, as native +start does. An unchanged Compose file whose external network is listed in +`isolatedNetworks` therefore stays on native, and native start replaces that +network with an owned isolated one. The selection is read only when the Compose +file has an external network. If it cannot be read, is not version 1, or its +`isolatedNetworks` is not a list of unique names, the command refuses with the +same one-line `E_CONFIG_INVALID` message native start gives, before any runtime +request. + To run such a project, select the Compose backend: ```sh @@ -209,7 +218,8 @@ authored native format. `compose backend features` check. With `--json`, the check's `composeBackend.features` lists the closed names `external_network`, `reserved_label`, `unsupported_build` and `foreign_platform`. On the native backend a found feature is a warning; on -the Compose backend it is informational. +the Compose backend it is informational. Native doctor applies the same +`HACK_NATIVE_ADAPTATION` selection before it reports. Native `up` and `restart` also stay in the foreground in every project: `--detach` refuses with one `E_USAGE` line. Run `hack up` in its own terminal and @@ -1256,7 +1266,9 @@ adaptations are accepted: ``` Networks must already be declared external; selection explicitly replaces their -connectivity with the owned isolated network. Probes apply only when no existing +connectivity with the owned isolated network. Native `up`, `restart` and doctor +then treat those networks as owned, not as external Compose networks (see +[Compose projects that stay on the Compose backend](#compose-projects-that-stay-on-the-compose-backend)). Probes apply only when no existing healthcheck is present. Additional hostnames append to existing Caddy labels. Additional host aliases append explicit `host-gateway` declarations and still need separate pinned dependency selections; they do not enable general host access or diff --git a/docs/reference/native-config-import.md b/docs/reference/native-config-import.md index d4e220a66..e94366406 100644 --- a/docs/reference/native-config-import.md +++ b/docs/reference/native-config-import.md @@ -15,9 +15,10 @@ projects, read managed environment values or keys, or inspect/change engine resources. Any refusal discards the private candidate; no partial conversion is reported as successful. -An unchanged project that uses an external network, a reserved ownership label, -a build outside the basic form below, or a non-arm64 platform stays on the Compose -backend in v5. Native `up` refuses those features before any work, and +An unchanged project that uses an external network (one that the +`HACK_NATIVE_ADAPTATION` selection does not list in `isolatedNetworks`), a reserved +ownership label, a build outside the basic form below, or a non-arm64 platform +stays on the Compose backend in v5. Native `up` refuses those features before any work, and `hack doctor` reports them; see [Compose projects that stay on the Compose backend](../guides/native-candidate.md#compose-projects-that-stay-on-the-compose-backend). That check uses the same basic build mapping as this preview. diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 9445afc4b..7a545316f 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1207,6 +1207,7 @@ async function handleNativeDoctor(opts: { checkComposeBackendFeatures({ startDir: opts.startDir, backend: "native", + adaptationFile: process.env.HACK_NATIVE_ADAPTATION, }) ) ); @@ -3012,11 +3013,14 @@ export function envKeyCheckResult(inspection: EnvKeyInspection): CheckResult { */ /** * Reports which Compose features keep the current project on the Compose - * backend (HACK-1291). Reads only the project's Compose file; value-free. + * backend (HACK-1291). Reads only the project's Compose file and, on the native + * backend, the `HACK_NATIVE_ADAPTATION` selection native up applies. An + * invalid selection is the same value-free error native up gives. */ async function checkComposeBackendFeatures(opts: { readonly startDir: string; readonly backend: "native" | "compose"; + readonly adaptationFile?: string; }): Promise { const ctx = await findProjectContext(opts.startDir); if (!ctx) { @@ -3028,7 +3032,9 @@ async function checkComposeBackendFeatures(opts: { } return composeBackendCheckResult({ backend: opts.backend, - features: await readComposeBackendFeatures(ctx.composeFile), + features: await readComposeBackendFeatures(ctx.composeFile, { + adaptationFile: opts.adaptationFile, + }), }); } diff --git a/src/commands/project.ts b/src/commands/project.ts index ae732a30f..69b54fe11 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -4973,8 +4973,11 @@ async function handleNativeUp({ }); // Before branch, config, env, lifecycle, storage or runtime work. `down` // stays available for any graph a project already has. + // The explicit adaptation applies first: a network it isolates is not + // external here, exactly as native start normalizes it. await refuseNativeComposeBackendFeatures({ composeFile: project.composeFile, + adaptationFile: process.env.HACK_NATIVE_ADAPTATION, restart, }); const branch = await resolveEffectiveBranchForCommand({ diff --git a/src/lib/native-compose-backend-features.ts b/src/lib/native-compose-backend-features.ts index 98c32db1f..f3d7063c1 100644 --- a/src/lib/native-compose-backend-features.ts +++ b/src/lib/native-compose-backend-features.ts @@ -1,5 +1,6 @@ import { constants } from "node:fs"; import { open } from "node:fs/promises"; +import { readNativeSelection } from "../backends/native-project-selection.ts"; import { HackCliError } from "./cli-result.ts"; import { isRecord } from "./guards.ts"; import { mapLegacyComposeBuild } from "./native-config-import-build.ts"; @@ -85,7 +86,9 @@ function hasReservedLabel(labels: unknown): boolean { * Compose treats `external: true` and the legacy `external: {name: ...}` form * as a network the project does not own. The runtime's reviewed * `x-hack-isolated: true` replacement is supported natively: it creates an - * owned internal bridge instead of joining the external one. + * owned internal bridge instead of joining the external one. A network that + * the `HACK_NATIVE_ADAPTATION` selection lists in `isolatedNetworks` receives + * that replacement in native normalization, so it is not external either. */ function isExternalNetwork(network: unknown): boolean { if (!isRecord(network)) { @@ -135,17 +138,34 @@ function topLevelEntries( * {@link COMPOSE_BACKEND_FEATURES} order, each at most once. Every service is * inspected, including services outside the selected profiles, because the * native path reviews all declarations. Shapes this classifier does not - * recognize are left to native review, which refuses them as before. + * recognize are left to native review, which refuses them as before. A + * network named in `isolatedNetworks` is not external, because the native + * adaptation replaces its connectivity with an owned isolated network. */ export function findComposeBackendFeatures( - compose: unknown + compose: unknown, + opts?: { + /** + * Network names a valid native adaptation isolates; see + * {@link readAdaptedIsolatedNetworks}. + */ + readonly isolatedNetworks?: ReadonlySet; + } ): ComposeBackendFeature[] { if (!isRecord(compose)) { return []; } const found = new Set(); const services = topLevelEntries(compose, "services").filter(isRecord); - if (topLevelEntries(compose, "networks").some(isExternalNetwork)) { + const isolated = opts?.isolatedNetworks ?? new Set(); + const networks = isRecord(compose.networks) + ? Object.entries(compose.networks) + : []; + if ( + networks.some( + ([name, network]) => !isolated.has(name) && isExternalNetwork(network) + ) + ) { found.add("external_network"); } const resources = [ @@ -168,14 +188,12 @@ export function findComposeBackendFeatures( } /** - * Reads and classifies one Compose file with the bounds native image-project - * input uses. Read-only: no lock, no write, no runtime request. A file that is - * missing, oversized, a symlink or not parseable returns no features, so the - * existing native input refusal still reports it. + * Reads one Compose file with the bounds native image-project input uses. + * Read-only: no lock, no write, no runtime request. A file that is missing, + * oversized, a symlink or not parseable returns `undefined`, so the existing + * native input refusal still reports it. */ -export async function readComposeBackendFeatures( - composeFile: string -): Promise { +async function readComposeDocument(composeFile: string): Promise { let text: string; try { const file = await open( @@ -187,7 +205,7 @@ export async function readComposeBackendFeatures( const buffer = Buffer.alloc(COMPOSE_LIMIT + 1); const { bytesRead } = await file.read(buffer, 0, buffer.length, 0); if (bytesRead > COMPOSE_LIMIT) { - return []; + return; } text = new TextDecoder("utf-8", { fatal: true }).decode( buffer.subarray(0, bytesRead) @@ -196,15 +214,85 @@ export async function readComposeBackendFeatures( await file.close(); } } catch { - return []; + return; } try { - return findComposeBackendFeatures(Bun.YAML.parse(text)); + return Bun.YAML.parse(text); } catch { - return []; + return; } } +/** The same closed refusal native start gives for an invalid adaptation. */ +function adaptationRefused(): HackCliError { + return new HackCliError({ + code: "E_CONFIG_INVALID", + message: + "Native project adaptation is invalid or conflicts with declared configuration; values omitted.", + }); +} + +/** + * Reads the network names a `HACK_NATIVE_ADAPTATION` selection isolates. The + * file is read with the same bounds as native start. An unreadable file, a + * selection that is not version 1, or an `isolatedNetworks` value that is not + * a bounded list of unique names refuses with `E_CONFIG_INVALID`, as native + * start would. Native start still validates the whole selection against the + * normalized Compose file, including that each name is an external network. + */ +export async function readAdaptedIsolatedNetworks( + adaptationFile: string +): Promise> { + const selection = await readNativeSelection(adaptationFile); + if (!isRecord(selection) || selection.version !== 1) { + throw adaptationRefused(); + } + const names = selection.isolatedNetworks; + if (names === undefined) { + return new Set(); + } + if ( + !( + Array.isArray(names) && + names.length <= 32 && + names.every((name) => typeof name === "string") && + new Set(names).size === names.length + ) + ) { + throw adaptationRefused(); + } + return new Set(names); +} + +/** + * Reads and classifies one Compose file after the explicit native adaptation. + * Read-only: no lock, no write, no runtime request. A Compose file that is + * missing, oversized, a symlink or not parseable returns no features, so the + * existing native input refusal still reports it. + * + * `adaptationFile` is the `HACK_NATIVE_ADAPTATION` path native start applies. + * It is read only when the unadapted file has an external network, so a + * project without one keeps its existing order of refusals. An invalid + * adaptation refuses with `E_CONFIG_INVALID` (see + * {@link readAdaptedIsolatedNetworks}). + */ +export async function readComposeBackendFeatures( + composeFile: string, + opts?: { readonly adaptationFile?: string } +): Promise { + const compose = await readComposeDocument(composeFile); + const features = findComposeBackendFeatures(compose); + if ( + opts?.adaptationFile === undefined || + !features.includes("external_network") + ) { + return features; + } + return findComposeBackendFeatures(compose, { + isolatedNetworks: await readAdaptedIsolatedNetworks(opts.adaptationFile), + }); +} + /** Value-free phrase list, such as "a and b" or "a, b and c". */ export function describeComposeBackendFeatures( features: readonly ComposeBackendFeature[] @@ -244,9 +332,13 @@ export function composeBackendRefusal(opts: { */ export async function refuseNativeComposeBackendFeatures(opts: { readonly composeFile: string; + /** The `HACK_NATIVE_ADAPTATION` path native start applies, if any. */ + readonly adaptationFile?: string; readonly restart: boolean; }): Promise { - const features = await readComposeBackendFeatures(opts.composeFile); + const features = await readComposeBackendFeatures(opts.composeFile, { + adaptationFile: opts.adaptationFile, + }); if (features.length > 0) { throw composeBackendRefusal({ features, restart: opts.restart }); } diff --git a/tests/native-compose-backend-features-cli.test.ts b/tests/native-compose-backend-features-cli.test.ts index c80384bc9..44ad9d0f6 100644 --- a/tests/native-compose-backend-features-cli.test.ts +++ b/tests/native-compose-backend-features-cli.test.ts @@ -329,6 +329,146 @@ test( SPAWN_TIMEOUT_MS ); +/** The documented `isolatedNetworks` adaptation for an external `hack-dev`. */ +const HACK_DEV_COMPOSE = { + services: { web: { image: IMAGE, networks: ["hack-dev"] } }, + networks: { "hack-dev": { external: true } }, +}; + +async function adaptation(f: Fixture, selection: unknown): Promise { + const path = join(f.root, "native-adaptation.json"); + await writeFile(path, JSON.stringify(selection)); + return path; +} + +/** A runtime request that the stub refused or that timed out on a loaded host. */ +const RUNTIME_REQUEST_LINE = + /^ERROR: Native runtime request (failed \(runtime_not_running\)|timed out); inspect owned state before retrying\.$/m; + +test( + "the documented isolatedNetworks adaptation keeps an external network on native up", + async () => { + const f = await fixture(HACK_DEV_COMPOSE); + const path = await adaptation(f, { + version: 1, + isolatedNetworks: ["hack-dev"], + }); + const result = await cli(f, ["up"], { + HACK_NATIVE_SHARED_SOURCE: "1", + HACK_NATIVE_ADAPTATION: path, + }); + expect(result.output).not.toContain("E_NATIVE_PROJECT_UNSUPPORTED"); + expect(result.output).not.toContain("HACK_RUNTIME_BACKEND=compose"); + // The check passed: native start went on to its source preflight and, + // after applying the adaptation in native review, to `runtime up`. + expect(result.output).toMatch(RUNTIME_REQUEST_LINE); + const calls = await f.calls(); + expect(calls).toEqual( + ["runtime check-project-share", "runtime up"].slice(0, calls.length) + ); + }, + SPAWN_TIMEOUT_MS +); + +test( + "the same adaptation lets native restart reach its own preflight", + async () => { + const f = await fixture(HACK_DEV_COMPOSE); + const path = await adaptation(f, { + version: 1, + isolatedNetworks: ["hack-dev"], + }); + const result = await cli(f, ["restart"], { + HACK_NATIVE_SHARED_SOURCE: "1", + HACK_NATIVE_ADAPTATION: path, + }); + // Restart preflight refuses a project with no run, after the check. + expectOneClosedLine( + result, + "Native restart has no existing run; use hack up for initial startup." + ); + expect(await f.calls()).toEqual([]); + }, + SPAWN_TIMEOUT_MS +); + +test( + "an adaptation that isolates a different network still refuses the external one", + async () => { + const f = await fixture(HACK_DEV_COMPOSE); + f.env({}); + const path = await adaptation(f, { + version: 1, + isolatedNetworks: ["other-net"], + }); + const before = await tree(f.root); + const result = await cli(f, ["up"], { + HACK_NATIVE_SHARED_SOURCE: "1", + HACK_NATIVE_ADAPTATION: path, + }); + expectOneClosedLine( + result, + `E_NATIVE_PROJECT_UNSUPPORTED: Native \`hack up\` does not support an external Compose network; ${SELECTION}` + ); + expect(await f.calls()).toEqual([]); + expect(await tree(f.root)).toEqual(before); + }, + SPAWN_TIMEOUT_MS +); + +test( + "an invalid adaptation refuses with native start's closed line before any request", + async () => { + const f = await fixture(HACK_DEV_COMPOSE); + f.env({}); + const path = await adaptation(f, { + version: 2, + isolatedNetworks: ["hack-dev"], + }); + const before = await tree(f.root); + const result = await cli(f, ["up"], { + HACK_NATIVE_SHARED_SOURCE: "1", + HACK_NATIVE_ADAPTATION: path, + }); + expectOneClosedLine( + result, + "E_CONFIG_INVALID: Native project adaptation is invalid or conflicts with declared configuration; values omitted." + ); + expect(await f.calls()).toEqual([]); + expect(await tree(f.root)).toEqual(before); + }, + SPAWN_TIMEOUT_MS +); + +test( + "native doctor applies the same adaptation before reporting", + async () => { + const f = await fixture(HACK_DEV_COMPOSE); + const path = await adaptation(f, { + version: 1, + isolatedNetworks: ["hack-dev"], + }); + const result = await cli(f, ["doctor", "--json"], { + HACK_NATIVE_ADAPTATION: path, + }); + const envelope: unknown = JSON.parse( + result.output.slice(result.output.indexOf("{")) + ); + expect(envelope).toMatchObject({ + data: { + checks: expect.arrayContaining([ + expect.objectContaining({ + id: "compose backend features", + status: "ok", + composeBackend: { features: [] }, + }), + ]), + }, + }); + }, + SPAWN_TIMEOUT_MS +); + test( "native doctor reports the same features for the current project, value-free", async () => { diff --git a/tests/native-compose-backend-features.test.ts b/tests/native-compose-backend-features.test.ts index 0b1bb4d8d..327228e48 100644 --- a/tests/native-compose-backend-features.test.ts +++ b/tests/native-compose-backend-features.test.ts @@ -2,6 +2,8 @@ import { afterEach, expect, test } from "bun:test"; import { mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { adaptNativeProject } from "../src/backends/native-project-adaptation.ts"; +import type { NativeProjectInput } from "../src/backends/native-project-input.ts"; import { COMPOSE_BACKEND_CHECK, composeBackendCheckResult, @@ -241,3 +243,142 @@ test("doctor reports the same features value-free for each backend", () => { }); } }); + +const ADAPTATION_REFUSAL = + "Native project adaptation is invalid or conflicts with declared configuration; values omitted."; + +function adaptationInput(compose: unknown): NativeProjectInput { + return { + originalSha256: "0".repeat(64), + normalizedComposeJson: JSON.stringify(compose), + managedEnvironment: {}, + lifecycleHostEnvironment: {}, + effectiveEnvName: null, + environmentFiles: [], + serviceNames: ["web"], + }; +} + +test("a network the native adaptation isolates is not external, as after native normalization", () => { + const compose = { + services: { web: { image: IMAGE, networks: ["hack-dev", "edge"] } }, + networks: { "hack-dev": { external: true }, edge: { external: true } }, + }; + const both = new Set(["hack-dev", "edge"]); + expect( + findComposeBackendFeatures(compose, { isolatedNetworks: both }) + ).toEqual([]); + // Parity with the adaptation itself: the adapted normalized copy has no + // external network left for the same selection. + const adapted = adaptNativeProject({ + input: adaptationInput(compose), + selection: { version: 1, isolatedNetworks: [...both] }, + }); + expect( + findComposeBackendFeatures(JSON.parse(adapted.normalizedComposeJson)) + ).toEqual([]); + // A network the adaptation does not list is still external. + expect( + findComposeBackendFeatures(compose, { + isolatedNetworks: new Set(["hack-dev"]), + }) + ).toEqual(["external_network"]); + // Isolation hides only the network feature, never another one. + expect( + findComposeBackendFeatures( + { + ...compose, + services: { web: { image: IMAGE, platform: "linux/amd64" } }, + }, + { isolatedNetworks: both } + ) + ).toEqual(["foreign_platform"]); +}); + +test("the Compose file is classified after a valid HACK_NATIVE_ADAPTATION selection", async () => { + const root = await mkdtemp(join(tmpdir(), "compose-backend-")); + roots.push(root); + const composeFile = join(root, "docker-compose.yml"); + await writeFile( + composeFile, + "services:\n web:\n image: app\n networks: [hack-dev]\nnetworks:\n hack-dev:\n external: true\n" + ); + const selection = async (name: string, value: unknown) => { + const path = join(root, name); + await writeFile(path, JSON.stringify(value)); + return path; + }; + const isolating = await selection("isolating.json", { + version: 1, + isolatedNetworks: ["hack-dev"], + }); + expect( + await readComposeBackendFeatures(composeFile, { adaptationFile: isolating }) + ).toEqual([]); + await refuseNativeComposeBackendFeatures({ + composeFile, + adaptationFile: isolating, + restart: true, + }); + for (const [index, other] of [ + { version: 1, isolatedNetworks: ["other-net"] }, + { version: 1, httpProbes: { web: { port: 3000, path: "/health" } } }, + ].entries()) { + expect( + await readComposeBackendFeatures(composeFile, { + adaptationFile: await selection(`other-${index}.json`, other), + }) + ).toEqual(["external_network"]); + } +}); + +test("an invalid adaptation refuses as native start does, and is read only for an external network", async () => { + const root = await mkdtemp(join(tmpdir(), "compose-backend-")); + roots.push(root); + const external = join(root, "external.yml"); + await writeFile(external, "networks:\n hack-dev:\n external: true\n"); + const owned = join(root, "owned.yml"); + await writeFile(owned, "services:\n web:\n image: app\n"); + const invalid: unknown[] = [ + { version: 2, isolatedNetworks: ["hack-dev"] }, + { version: 1, isolatedNetworks: "hack-dev" }, + { version: 1, isolatedNetworks: ["hack-dev", "hack-dev"] }, + { version: 1, isolatedNetworks: [7] }, + ["hack-dev"], + ]; + for (const [index, value] of invalid.entries()) { + const path = join(root, `invalid-${index}.json`); + await writeFile(path, JSON.stringify(value)); + await expectCodedRefusal( + refuseNativeComposeBackendFeatures({ + composeFile: external, + adaptationFile: path, + restart: false, + }), + "E_CONFIG_INVALID", + ADAPTATION_REFUSAL + ); + // The adaptation native start applies refuses the same selection. + expect(() => + adaptNativeProject({ + input: adaptationInput({ + services: { web: { image: IMAGE } }, + networks: { "hack-dev": { external: true } }, + }), + selection: value, + }) + ).toThrow(ADAPTATION_REFUSAL); + // Without an external network the selection is not read here; native + // start reports it in its usual order. + expect( + await readComposeBackendFeatures(owned, { adaptationFile: path }) + ).toEqual([]); + } + for (const unreadable of [join(root, "missing.json"), "relative.json"]) { + await expectCodedRefusal( + readComposeBackendFeatures(external, { adaptationFile: unreadable }), + "E_CONFIG_INVALID", + "values omitted" + ); + } +}); From 94e12a99d175021fd492732dee8fca04051d83b3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 18:34:34 -0400 Subject: [PATCH 14/14] test(native): accept every post-check runtime reply in the adaptation CLI case Under heavy host load the stub runtime's reply can time out or arrive unreadable. Each of those lines is only reachable after the Compose-backend check passed, so the adaptation case accepts all three. Co-Authored-By: Claude Opus 5.5 --- tests/native-compose-backend-features-cli.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/native-compose-backend-features-cli.test.ts b/tests/native-compose-backend-features-cli.test.ts index 44ad9d0f6..962159072 100644 --- a/tests/native-compose-backend-features-cli.test.ts +++ b/tests/native-compose-backend-features-cli.test.ts @@ -341,9 +341,13 @@ async function adaptation(f: Fixture, selection: unknown): Promise { return path; } -/** A runtime request that the stub refused or that timed out on a loaded host. */ +/** + * A runtime request that the stub refused, or, on a heavily loaded host, one + * that timed out or whose reply the client could not read. Each is only + * reachable after the Compose-backend check passed. + */ const RUNTIME_REQUEST_LINE = - /^ERROR: Native runtime request (failed \(runtime_not_running\)|timed out); inspect owned state before retrying\.$/m; + /^ERROR: Native runtime (request failed \(runtime_not_running\)|request timed out); inspect owned state before retrying\.$|^ERROR: Native runtime returned an invalid control response\.$/m; test( "the documented isolatedNetworks adaptation keeps an external network on native up",