From 864ca29139a1b11445cc7fb910d8c7720d09b263 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 01:58:59 -0400 Subject: [PATCH 1/6] fix(agents): refuse Git rebase--interactive --cmd and scope subcommand flags after the subcommand Git 2.20-2.33 ship a `rebase--interactive` builtin whose `--cmd ` adds an exec line after each pick and runs it. The task catalog's inline-command denylist did not model it, so `git rebase--interactive --cmd id`, its `--cmd=id` form and the dashed `git-rebase--interactive` were accepted. `--cmd` is now a Git inline flag under every subcommand (no Git option shares the `--cm` prefix, and a repository alias can rename the helper), and `--interactive` is read as its porcelain like `--helper`, so its `--add-exec-commands` mode meets the `rebase`-scoped flag. Subcommand-scoped flags (`clone -u`, `rebase --add-exec-commands`) now count only after an operand naming the subcommand: Git reads a subcommand's options after its name and rejects an unknown option before it, so `git push -u origin clone`, `git config --add alias.r rebase` and `git remote set-branches --add origin rebase` pass, while an unknown global option's value still cannot hide the subcommand. The remaining collateral (`git commit -m rebase --add`, `git log -S clone -u`) is documented and tested. The tracked-pin test now uses CASE_TIMEOUT_MS. Refs HACK-1423 Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-task-catalog.md | 42 ++++--- src/lib/task-catalog-recipe.ts | 85 +++++++++----- tests/project-task-catalog.test.ts | 179 ++++++++++++++++++++--------- 3 files changed, 208 insertions(+), 98 deletions(-) diff --git a/docs/reference/ax-task-catalog.md b/docs/reference/ax-task-catalog.md index 43b1c4160..3ae7bac0d 100644 --- a/docs/reference/ax-task-catalog.md +++ b/docs/reference/ax-task-catalog.md @@ -74,7 +74,7 @@ is not implemented here. wrappers (`npx -c`, `npm exec -c`, `pnpm -c`, `yarn exec`, `bun exec`, `nodemon --exec`, `tsx -e`, `ts-node -e`, `julia -e`, `sudo -s`, `doas -s`, `git -c`/`--config`/`--exec`/`--upload-pack`/ - `--receive-pack`/`clone -u`/`submodule foreach`/`bisect`/ + `--receive-pack`/`--cmd`/`clone -u`/`submodule foreach`/`bisect`/ `filter-branch`, a Git `!` alias value such as `git config alias.x '!cmd'`, `make --eval`, `nix eval`). A dashed Git builtin, by basename or absolute path (`git-clone`, @@ -97,9 +97,12 @@ is not implemented here. (`psql -1c`, `mysql -Be`). For `psql`, `mysql`, `mariadb` and Python, a cluster is read only up to the first option that takes a value, so `psql -hlocalhost`, `psql -Ucustomer`, `mysql -uroot -psecret` and - `python -mcompileall` pass. `clone -u` counts when any operand is - `clone`, so a Git global option that takes a value + `python -mcompileall` pass. `clone -u` counts after any operand that + is `clone`, so a Git global option that takes a value (`git --attr-source HEAD clone -u cmd`) cannot hide the subcommand. + It does not count before that operand, because Git reads a + subcommand's options only after its name, so + `git push -u origin clone` (a branch named `clone`) passes. Accepted forms include `psql -f script.sql`, `mysql --batch db`, `sqlite3 -csv app.db`, `duckdb -f script.sql data.duckdb`, @@ -358,6 +361,8 @@ until its recipes are re-reviewed. | `psql -o`/`--output` piped to a command (`-o '\|cmd'`, `-o\|cmd`, `-qo\|cmd`, `--output=\|cmd`, `--out=\|cmd`) | `inline_command`; `psql -o out.txt`, `psql -F'\|'`, `psql -L '\|log'` (a file name) still pass | domain test | | Dashed Git builtins (`/usr/lib/git-core/git-clone -u cmd`, `git-config alias.x '!cmd'`, `git-ls-remote --upload-pack=cmd`, `git-fetch-pack --exec=cmd`) | `inline_command`; `git-clone url dir`, `git-push -u origin main`, `git-config alias.st status` still pass | domain test | | Git plumbing helpers read as their porcelain (`git-bisect--helper --bisect-run cmd`, `/usr/lib/git-core/git-bisect--helper.exe run cmd`, `git bisect--helper --bisect-run cmd`, `git-submodule--helper foreach cmd`, `git-config alias.x bisect--helper`, older Git's `git rebase--helper --add-exec-commands cmd` in any letter case) | `inline_command`; `git-submodule--helper status`, `git-difftool--helper a b`, `git add bisect--helper.txt`, `git config --add alias.st status` still pass | domain test | +| Git 2.20-2.33's `rebase--interactive --cmd cmd` (`git rebase--interactive --cmd=cmd`, `git-rebase--interactive --cmd cmd`, `--cm cmd`, a repository alias such as `git ri --cmd cmd`) and its `--add-exec-commands` mode | `inline_command`; `git rebase--interactive --continue`, `git-rebase--interactive --edit-todo` still pass | domain test | +| A subcommand-scoped Git flag before the subcommand's name (`git push -u origin clone`, `git remote set-branches --add origin rebase`, `git config --add alias.r rebase`) | accepted; the flag after the name (`git clone -u cmd url`, `git --unknown v clone -u cmd url`, `git rebase--helper --add cmd`) is still refused | domain test | | Detached linked worktree with no named instance, on CLI `task list`/`task inspect` and MCP `hack.project.task.list`/`inspect` | `detached_checkout`; naming the instance (`--branch`, MCP `branch`) passes | paired fixture, domain test | | `duckdb -storage-version VER db` | accepted; a SQL operand after the database is still refused | domain test | | `git clone -u` (also after `--attr-source ` or an unknown global option), abbreviated or attached `--upload-pack`/`--receive-pack`/`--exec`, `clone --config`, and `!` alias values (`git config alias.x '!cmd'`, `git -c alias.x=!cmd`) | `inline_command`; `git clone url dir`, `git push -u origin main`, `git add -u`, `git config alias.st status` still pass | domain test | @@ -401,7 +406,7 @@ until its recipes are re-reviewed. reviewer must check argv, and WU26 execution must not rely on this check as a sandbox. For a known program it is fail-closed, so it can also refuse an ordinary argument that looks like an inline flag (`script.sh -c`, - `git clean -x`, `git -C dir status`, `git push -u origin clone`, + `git clean -x`, `git -C dir status`, `git log -S clone -u`, `make -e`, `python -I`, `mysql -E`, a Git argument such as `--grep=!x` that starts a value with `!`, or a script argument named like a REPL module after a Python trampoline, as in @@ -417,16 +422,25 @@ until its recipes are re-reviewed. string in argv is. Code a program loads from a file is not inspected either: for example, a reviewed script that loads a data file with `pickle` or `marshal`, or reads a config file that it evaluates. A Git - `--helper` plumbing command (`git-bisect--helper`, - `git submodule--helper`) is read as `git `, so it meets the rules for - that subcommand. A helper's own options are modeled only where they run a - command: older Git's `rebase--helper --add-exec-commands` is refused - through a flag scoped to `rebase`. Any whole-word `--helper` - argument is read as `` (fail-closed), so - `git push -u origin clone--helper` (a branch of that name) and - `git add bisect--helper` (a file of that name) are refused. The `rebase`-scoped flag counts when any - operand is `rebase`, and Git abbreviates, so - `git config --add alias.r rebase` is refused too. Git config writes other than `!` aliases (for example + `--helper` or `--interactive` plumbing command + (`git-bisect--helper`, `git submodule--helper`, Git 2.20-2.33's + `git rebase--interactive`) is read as `git `, so it meets the rules + for that subcommand. A helper's own options are modeled only where they + run a command: Git 2.13-2.19's `rebase--helper --add-exec-commands` is + refused through a flag scoped to `rebase`, and `rebase--interactive + --cmd` through `--cmd`, which is refused under every subcommand because + a repository alias can rename the helper. No Git option shares its + `--cm` prefix, so only an argument spelled like it, such as + `git grep -e --cmd`, is refused with it. Any whole-word `--helper` or `--interactive` + argument is read as `` (fail-closed), so `git add bisect--helper` + (a file of that name) is refused. A subcommand-scoped flag (`clone -u`, + `rebase --add-exec-commands`) counts after any operand that names the + subcommand, and Git abbreviates, so an earlier word `clone` or `rebase` + scopes it even when that word is another option's value: + `git commit -m rebase --add` and `git log -S clone -u` (`-u` is + `--patch`) are refused. The same flag before the word passes + (`git config --add alias.r rebase`, + `git remote set-branches --add origin rebase`). Git config writes other than `!` aliases (for example `git config core.pager cmd` or `include.path`) are not detected. Nor are `mysql`/`mariadb` option files named in argv (`--defaults-extra-file`, which can set `init-command`) or `--pager`, which runs a command only in diff --git a/src/lib/task-catalog-recipe.ts b/src/lib/task-catalog-recipe.ts index 4aa589b23..78a9fde1e 100644 --- a/src/lib/task-catalog-recipe.ts +++ b/src/lib/task-catalog-recipe.ts @@ -369,10 +369,13 @@ type Evaluator = { /** * Inline flags that count only under one subcommand, because the same * letter is harmless elsewhere: `git clone -u ` runs a command, while - * `git push -u` sets an upstream. They count when any operand names the - * subcommand, not only the first, so a global option missing from + * `git push -u` sets an upstream. They count after any operand that names + * the subcommand, not only the first, so a global option missing from * `valueOptions` (whose value would be taken as the subcommand) cannot - * hide it (fail-closed). + * hide it (fail-closed). They do not count before it: Git reads a + * subcommand's options only after the subcommand's name and rejects an + * unknown option before it, so `git push -u origin clone` (a branch named + * `clone`) passes. */ readonly subcommandInline: Readonly>; /** @@ -664,10 +667,14 @@ const EVALUATORS: Readonly> = { // Git accepts abbreviated long options (`--upl=cmd`) and a value attached // to a short option (`-u/path/cmd`). A dashed builtin (`git-clone`, // `/usr/lib/git-core/git-config`) is read as `git `, and a plumbing - // helper (`git-bisect--helper`, `git submodule--helper`) as its porcelain, - // so older Git's `rebase--helper --add-exec-commands cmd` meets a - // `rebase`-scoped flag. That flag is scoped because, as a `git` inline - // flag, it would also match `git config --add` (Git abbreviates options). + // helper (`git-bisect--helper`, `git submodule--helper`, Git 2.20-2.33's + // `git rebase--interactive`) as its porcelain, so Git 2.13-2.19's + // `rebase--helper --add-exec-commands cmd` meets a `rebase`-scoped flag. + // That flag is scoped because, as a `git` inline flag, it would also + // match `git config --add` (Git abbreviates options). The + // `rebase--interactive --cmd cmd` option is not scoped: no other Git + // option starts with `--cm`, and a repository alias for the helper + // (`git ri --cmd cmd`) would hide the subcommand. git: evaluator({ inline: [ "-c", @@ -678,6 +685,7 @@ const EVALUATORS: Readonly> = { "--extcmd", "--upload-pack", "--receive-pack", + "--cmd", ], words: ["foreach", "bisect", "filter-branch"], // Global options before the subcommand that consume the next argument. @@ -856,15 +864,17 @@ function programName(arg: string): string { /** A dashed Git builtin (`git-clone`) and its subcommand (`clone`). */ const DASHED_GIT = /^git-(.+)$/; /** - * A Git plumbing helper subcommand (`bisect--helper`, `submodule--helper`) - * and the porcelain subcommand it implements (`bisect`, `submodule`). + * A Git plumbing helper subcommand (`bisect--helper`, `submodule--helper`, + * Git 2.20-2.33's `rebase--interactive`) and the porcelain subcommand it + * implements (`bisect`, `submodule`, `rebase`). */ -const GIT_HELPER = /^([a-z0-9][a-z0-9-]*?)--helper$/i; +const GIT_HELPER = /^([a-z0-9][a-z0-9-]*?)--(?:helper|interactive)$/i; /** - * A Git argument with a `--helper` subcommand read as its porcelain - * ``, so `git bisect--helper --bisect-run cmd` meets the `bisect` rule - * and `submodule--helper foreach` the `foreach` rule. Any other argument is + * A Git argument with a `--helper` or `--interactive` subcommand + * read as its porcelain ``, so `git bisect--helper --bisect-run cmd` + * meets the `bisect` rule, `submodule--helper foreach` the `foreach` rule + * and `rebase--interactive --add-exec-commands` the `rebase` rule. Any other argument is * unchanged. A matching argument starts with a letter or digit and holds no * `=` or `!`, so no inline flag, word or `!` value is lost by the rewrite. * The porcelain is lowercased, like a program name, so a mixed-case helper @@ -1211,23 +1221,28 @@ function afterModuleFlag(opts: { return index === -1 ? [] : opts.rest.slice(index + 1); } +/** An operand and its index in the program's arguments. */ +type Operand = { readonly word: string; readonly index: number }; + /** * Every argument that is neither an option nor a known option's value, - * in order. Everything after `--` is an operand. + * in order, with its index. Everything after `--` is an operand. */ function operands(opts: { readonly rest: readonly string[]; readonly spec: Evaluator; -}): string[] { - const out: string[] = []; +}): Operand[] { + const out: Operand[] = []; for (let index = 0; index < opts.rest.length; index++) { const arg = opts.rest[index] ?? ""; if (arg === "--") { - out.push(...opts.rest.slice(index + 1)); + for (let after = index + 1; after < opts.rest.length; after++) { + out.push({ word: opts.rest[after] ?? "", index: after }); + } break; } if (arg === "-" || !(arg.startsWith("-") || arg.startsWith("+"))) { - out.push(arg); + out.push({ word: arg, index }); } else if ( opts.spec.valueOptions.includes(normalizeOption(arg, opts.spec)) ) { @@ -1237,6 +1252,23 @@ function operands(opts: { return out; } +/** + * The subcommand-scoped inline flags in force at argument `at`: those of + * every subcommand named by an operand before it (`git clone -u`, not + * `git push -u origin clone`). + */ +function scopedFlags(opts: { + readonly positional: readonly Operand[]; + readonly at: number; + readonly spec: Evaluator; +}): readonly string[] { + return opts.positional.flatMap(({ word, index }) => + index < opts.at && Object.hasOwn(opts.spec.subcommandInline, word) + ? (opts.spec.subcommandInline[word] ?? []) + : [] + ); +} + /** A value that is a shell command to Git: `!cmd` or `key=!cmd`. */ const BANG_COMMAND = /(?:^|=)\s*!/; @@ -1261,17 +1293,16 @@ function evaluatesCode(argv: readonly string[], index: number): boolean { return rest.length > 0; } const positional = operands({ rest, spec }); - const operand = positional[0] ?? null; - const scoped = positional.flatMap((word) => - Object.hasOwn(spec.subcommandInline, word) - ? (spec.subcommandInline[word] ?? []) - : [] - ); - const flags = [...spec.inline, ...spec.stdin, ...scoped]; + const operand = positional[0]?.word ?? null; + const flags = [...spec.inline, ...spec.stdin]; if ( rest.some( - (arg) => - matchesFlag({ arg, flags, spec }) || + (arg, at) => + matchesFlag({ + arg, + flags: [...flags, ...scopedFlags({ positional, at, spec })], + spec, + }) || spec.words.includes(arg.toLowerCase()) || (spec.dataUrlModules && DATA_URL.test(arg)) || (spec.bangCommands && BANG_COMMAND.test(arg)) diff --git a/tests/project-task-catalog.test.ts b/tests/project-task-catalog.test.ts index 6d886fdf4..7668217d9 100644 --- a/tests/project-task-catalog.test.ts +++ b/tests/project-task-catalog.test.ts @@ -1006,51 +1006,58 @@ describe("catalog domain", () => { }); }); - test("pins must be Git-tracked files; untracked content is never hashed", async () => { - const { dir, registryPath } = await project("tracking"); - const inspect = (taskId: string) => - inspectTask({ cwd: dir, selector: {}, registryPath, taskId }); - // Gitignored and untracked: refused without any digest. - const untracked = await inspect("untracked-secret"); - expect(untracked).toMatchObject({ - ok: false, - error: { detail: { reason: "input_path_invalid" } }, - }); - expect(JSON.stringify(untracked)).not.toContain( - sha256(canonical("fixture-value")) - ); - // Positive control: once the file is tracked the same pin is checked, - // so the tracked-file rule was the only reason for the refusal. - await git(dir, ["add", "--force", "config/secrets.json"]); - expect(await inspect("untracked-secret")).toMatchObject({ - ok: false, - error: { - detail: { - reason: "stale_recipe", - stale: { - inputs: [ - { index: 1, observed_sha256: sha256(canonical("fixture-value")) }, - ], + test( + "pins must be Git-tracked files; untracked content is never hashed", + async () => { + const { dir, registryPath } = await project("tracking"); + const inspect = (taskId: string) => + inspectTask({ cwd: dir, selector: {}, registryPath, taskId }); + // Gitignored and untracked: refused without any digest. + const untracked = await inspect("untracked-secret"); + expect(untracked).toMatchObject({ + ok: false, + error: { detail: { reason: "input_path_invalid" } }, + }); + expect(JSON.stringify(untracked)).not.toContain( + sha256(canonical("fixture-value")) + ); + // Positive control: once the file is tracked the same pin is checked, + // so the tracked-file rule was the only reason for the refusal. + await git(dir, ["add", "--force", "config/secrets.json"]); + expect(await inspect("untracked-secret")).toMatchObject({ + ok: false, + error: { + detail: { + reason: "stale_recipe", + stale: { + inputs: [ + { + index: 1, + observed_sha256: sha256(canonical("fixture-value")), + }, + ], + }, }, }, - }, - }); - // A reviewed recipe whose script leaves the index is refused. - expect((await inspect("lint")).ok).toBe(true); - await git(dir, ["rm", "--cached", "--quiet", "scripts/run-tool.sh"]); - expect(await inspect("lint")).toMatchObject({ - ok: false, - error: { detail: { reason: "input_path_invalid" } }, - }); - // Without Git nothing is tracked, so every pinned recipe is refused. - await git(dir, ["add", "scripts/run-tool.sh"]); - expect((await inspect("lint")).ok).toBe(true); - await rm(join(dir, ".git"), { recursive: true, force: true }); - expect(await inspect("lint")).toMatchObject({ - ok: false, - error: { detail: { reason: "input_path_invalid" } }, - }); - }); + }); + // A reviewed recipe whose script leaves the index is refused. + expect((await inspect("lint")).ok).toBe(true); + await git(dir, ["rm", "--cached", "--quiet", "scripts/run-tool.sh"]); + expect(await inspect("lint")).toMatchObject({ + ok: false, + error: { detail: { reason: "input_path_invalid" } }, + }); + // Without Git nothing is tracked, so every pinned recipe is refused. + await git(dir, ["add", "scripts/run-tool.sh"]); + expect((await inspect("lint")).ok).toBe(true); + await rm(join(dir, ".git"), { recursive: true, force: true }); + expect(await inspect("lint")).toMatchObject({ + ok: false, + error: { detail: { reason: "input_path_invalid" } }, + }); + }, + CASE_TIMEOUT_MS + ); test( "HACK-1331: the tracked-pin query ignores the caller's GIT_* env", @@ -1698,6 +1705,62 @@ describe("recipe grammar", () => { ["git", "rebase--helper", "--continue"], ], }, + { + // Git 2.20-2.33 ship `rebase--interactive`, whose `--cmd ` adds + // an `exec ` line after each pick and runs it. + issue: "HACK-1423", + rule: "Git rebase--interactive --cmd", + refused: [ + ["git", "rebase--interactive", "--cmd", "id"], + ["git", "rebase--interactive", "--cmd=id"], + ["git-rebase--interactive", "--cmd", "id"], + ["/usr/lib/git-core/git-rebase--interactive", "--cmd=id"], + ["git", "Rebase--Interactive", "--cm", "id"], + ["git", "--git-dir", "x", "rebase--interactive", "--cmd", "id"], + // A repository alias for the helper hides the subcommand, so + // `--cmd` is not scoped to `rebase`. + ["git", "ri", "--cmd", "id"], + // `rebase--interactive` is read as `rebase`, so its + // `--add-exec-commands` mode meets the `rebase`-scoped flag. + ["git", "rebase--interactive", "--add-exec-commands"], + ["git-rebase--interactive", "--add-exec"], + ], + accepted: [ + ["git", "rebase--interactive", "--continue"], + ["git-rebase--interactive", "--edit-todo"], + ["git", "add", "rebase--interactive.txt"], + ], + }, + { + // A subcommand-scoped flag counts only after the operand that names + // the subcommand: Git reads a subcommand's options after its name + // and rejects an unknown option before it. + issue: "HACK-1423", + rule: "subcommand-scoped Git flags count after the subcommand", + refused: [ + ["git", "clone", "-u", "id", "url"], + ["git-clone", "-u", "id", "url"], + ["git", "--attr-source", "HEAD", "clone", "-u", "id", "url"], + // An unknown global option's value is read as an operand, so the + // subcommand after it still scopes the flag (fail-closed). + ["git", "--unknown", "v", "clone", "-u", "id", "url"], + [ + "git", + "--unknown", + "v", + "rebase--helper", + "--add-exec-commands", + "id", + ], + ["git", "--namespace", "n", "rebase--helper", "--add", "id"], + ], + accepted: [ + ["git", "push", "-u", "origin", "clone"], + ["git", "remote", "set-branches", "--add", "origin", "rebase"], + ["git", "config", "--add", "alias.r", "rebase"], + ["git", "push", "-u", "origin", "clone--helper"], + ], + }, ]; for (const gap of gapCases) { @@ -1721,11 +1784,6 @@ describe("recipe grammar", () => { // `-C` matches `-c` (case-folded), so `git -C dir status` is refused; // a recipe must set the Compose workdir instead. expect(isInlineCommandArgv(["git", "-C", "dir", "status"])).toBe(true); - // Any operand named `clone` scopes `-u` to it, so a branch or remote - // named `clone` makes `git push -u` refused too. - expect(isInlineCommandArgv(["git", "push", "-u", "origin", "clone"])).toBe( - true - ); // Git's value-taking short options depend on the subcommand // (`rebase -mx cmd`), so an attached value containing `c`, `x` or `u` // is refused; the value as its own argument passes. @@ -1745,18 +1803,25 @@ describe("recipe grammar", () => { }); test("HACK-1411: documented Git helper fail-closed collateral stays refused", () => { - // Any whole-word `--helper` argument is read as ``, so a - // branch named `clone--helper` scopes `-u` to `clone` and a file named - // `bisect--helper` meets the `bisect` rule. - expect( - isInlineCommandArgv(["git", "push", "-u", "origin", "clone--helper"]) - ).toBe(true); + // Any whole-word `--helper` or `--interactive` argument is + // read as ``, so a file named `bisect--helper` meets the `bisect` + // rule. expect(isInlineCommandArgv(["git", "add", "bisect--helper"])).toBe(true); - // Any operand named `rebase` scopes `--add-exec-commands` to it, and - // Git abbreviates, so `--add` before an alias value `rebase` is refused. + // HACK-1423: an operand named `clone` or `rebase` scopes its flags to + // everything after it, even when it is another option's value (Git's + // option values are not modeled), and Git abbreviates, so `--add` + // after a message `rebase` and `-u` (`--patch`) after a pickaxe string + // `clone` are refused. The same flag before the word passes. expect( - isInlineCommandArgv(["git", "config", "--add", "alias.r", "rebase"]) + isInlineCommandArgv(["git", "commit", "-m", "rebase", "--add"]) ).toBe(true); + expect(isInlineCommandArgv(["git", "log", "-S", "clone", "-u"])).toBe(true); + expect(isInlineCommandArgv(["git", "log", "-u", "-S", "clone"])).toBe( + false + ); + // HACK-1423: `--cmd` is refused under every subcommand, so a Git + // argument spelled `--cmd` or `--cm` is refused too. + expect(isInlineCommandArgv(["git", "grep", "-e", "--cmd"])).toBe(true); }); test("effects must be covered by required capabilities", () => { From e3814e87d88a2df1ecd1ca31a4c179a1e9884e3b Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 02:02:37 -0400 Subject: [PATCH 2/6] fix(ci): read the test shard jobs in the Bun crash census and fix stale CI test job docs Since #442 the `test` job is only the required fan-in of the four `test shard i/4` jobs. It runs no Bun, but `ci:bun-crash-census` still read it by default and reported zero crashes without saying so, and `workflowJobBunVersion(ci.yml, "test")` found the fan-in block, which has no setup-bun. - With no --job, the census reads every `test shard i/n` job of a run, or the `test` job of a run that has no shard job (a run from before the split, whose `test` job ran the suite). - --job accepts `/` and a `*` pattern, so the documented `--job "test shard 1/4"` and `--job "test shard *"` work; before, both were refused as an invalid argument. - The workflow lookup matches a job by its `name:` template, so `test shard 2/4` finds the `test-shard` matrix job's Bun pin. - The report prints a closed `warning:` line (also on stderr, and in --json) when no job matched or a selected job shows no Bun setup in its log or workflow block. - docs/security/env-secrets-threat-model.md and docs/reference/shared-host-lifecycle.md now say the tests run in a `test shard i/4` job and that `test` is the fan-in. Refs HACK-1424 Co-Authored-By: Claude Opus 5.5 --- .ai/skills/hack-repo-verify/SKILL.md | 2 +- docs/reference/shared-host-lifecycle.md | 7 +- docs/security/env-secrets-threat-model.md | 2 +- scripts/ci-bun-crash-census.ts | 204 ++++++++++++++--- tests/ci-bun-crash-census.test.ts | 253 +++++++++++++++++++++- 5 files changed, 432 insertions(+), 36 deletions(-) diff --git a/.ai/skills/hack-repo-verify/SKILL.md b/.ai/skills/hack-repo-verify/SKILL.md index ed6106405..0faa9c9ff 100644 --- a/.ai/skills/hack-repo-verify/SKILL.md +++ b/.ai/skills/hack-repo-verify/SKILL.md @@ -98,7 +98,7 @@ untracked candidate additions. Keep private artifacts excluded from the index. | Native authored command slices | `tests/native-authored-command.test.ts` (unsupported matrix), `tests/native-authored-command-dispatch.test.ts`, `tests/native-authored-project-{start,run,observe,logs,exec,recovery}.test.ts` and the [command contract](../../../docs/reference/native-authored-commands.md); the dispatch "source CLI" cases need the prepared compiler (`bun run build:config-compiler` or absolute `HACK_CONFIG_COMPILER_BINARY`) and fail without it; `native-authored-project-start` and `native-authored-project-exec` are nondeterministic on a local macOS host, so a single local pass or failure is not proof; live acceptance runs host-serialized on one prepared macOS provider slot | | Native VM/routing/reclamation | [candidate guide](../../../docs/guides/native-candidate.md), relevant acceptance gate and isolated host fixture; Linux checks cannot qualify macOS effects | | Resource/performance claims | [performance skill](../hack-repo-performance/SKILL.md), matched workload and measured boundaries | -| Bun runtime crashes | Read-only history: `bun run ci:bun-crash-census -- --since --until ` (failed `test` job logs; `--scan-passing` for all; since the suite moved to `test shard i/4` jobs, pass `--job "test shard i/4"` once per shard for newer runs). Matched reproduction: the gated [experiment](../../../.github/workflows/bun-crash-experiment.yml), which spends hosted macOS minutes and needs an explicit go-ahead; never loop on a shared host. Its default `focused` mode loops `tests/native-https-owner.test.ts` (where both original segfaults happened); `full` needs a budget of at least 40 minutes because one full suite takes 30-35. A run never starts unless its estimate fits the remaining budget, so read `summary.json` for the verdict and per-mode crash, failure and skipped-for-budget counts. Both share [the classifier](../../../scripts/lib/bun-crash-classify.ts) | +| Bun runtime crashes | Read-only history: `bun run ci:bun-crash-census -- --since --until ` (failed test-suite job logs: every `test shard i/n` job, or the `test` job of a run from before the split, since `test` is now the logless fan-in; `--scan-passing` for all; `--job NAME` selects by exact name or a `*` pattern such as `"test shard *"`; a `warning:` line means no job matched or a selected job ran no Bun, so its zero is not evidence). Matched reproduction: the gated [experiment](../../../.github/workflows/bun-crash-experiment.yml), which spends hosted macOS minutes and needs an explicit go-ahead; never loop on a shared host. Its default `focused` mode loops `tests/native-https-owner.test.ts` (where both original segfaults happened); `full` needs a budget of at least 40 minutes because one full suite takes 30-35. A run never starts unless its estimate fits the remaining budget, so read `summary.json` for the verdict and per-mode crash, failure and skipped-for-budget counts. Both share [the classifier](../../../scripts/lib/bun-crash-classify.ts) | Read [CI](../../../.github/workflows/ci.yml) for current hosted gates. A local pass does not mean CI ran. diff --git a/docs/reference/shared-host-lifecycle.md b/docs/reference/shared-host-lifecycle.md index 0255d8138..587a8a172 100644 --- a/docs/reference/shared-host-lifecycle.md +++ b/docs/reference/shared-host-lifecycle.md @@ -226,9 +226,10 @@ Progress events are also closed: `lock-waiting`, `lock-acquired`, `claim-stale`, `bun test tests/shared-host-lifecycle.test.ts` runs portable local fixtures. Each fixture is a Bun process on a loopback port with a private HOME. No Docker, real -HOME, keychain or privileged effect is involved. The file runs in the hosted macOS -`test` job and is registered in the hosted Linux `linux-process-lifetime` job. A -Linux pass is a receipt only once that hosted job has run it. +HOME, keychain or privileged effect is involved. The file runs in one of the hosted +macOS `test shard i/4` jobs (the required `test` check passes only when every shard +passes) and is registered in the hosted Linux `linux-process-lifetime` job. A Linux +pass is a receipt only once that hosted job has run it. | Scenario | Asserted behavior | | --- | --- | diff --git a/docs/security/env-secrets-threat-model.md b/docs/security/env-secrets-threat-model.md index 7cff5ae79..a077f7de1 100644 --- a/docs/security/env-secrets-threat-model.md +++ b/docs/security/env-secrets-threat-model.md @@ -96,7 +96,7 @@ refer to `hack-dance/hack`. | A key copy ships in build output or a static site. | `hack doctor` reports `env-key-public-path` for copies under `public/`, `static/`, `dist/`, `build/`, `out/`, `www/`, `_site/`, `.next/` and `.output/` (#309). | | A weak, short or repetitive key. | `env-key-wrong-length` and `env-key-low-entropy` (#309). Generated keys are 32 random bytes in base64url. | | Materialized `.hack/.env` or local overrides are committed. | Hack owns a committed `.hack/.gitignore` block, and `hack doctor` flags tracked generated files (see [ignore rules](../env.md#ignore-rules-committed-hackgitignore)). | -| A secret leaks into a public artifact through a path nobody listed. | `tests/env-secret-sentinel-scan.test.ts` runs in the CI `test` job. It gives a primary checkout and a linked worktree random synthetic secrets through `env add --secret --value-stdin` (shared and `--local`), runs materialize, exec, up (Compose generation), env apply, logs, list, explain and doctor, then commits everything with `git add -A`. It requires zero occurrences of any value or the project key, raw or hex/base64/base64url encoded, in CLI output, recorded child argv, tracked files, every Git object, HACK_HOME, the private HOME and its caches, and generated Compose and runtime files. Plaintext may exist only in `.hack/.env`, and key text only in key files; each must be git-ignored, untracked and mode 0600. `.hack/.internal/compose.env.override.yml` must hold `${HACK_ENV_...}` references and no value, and must also be private and ignored. Values may reach a child's environment only for the exec'd command and `docker compose`. Positive controls prove delivery and that the scanner finds planted encodings in files and Git objects. Docker is a recording shim; no engine, keychain or installed Hack is reached. | +| A secret leaks into a public artifact through a path nobody listed. | `tests/env-secret-sentinel-scan.test.ts` runs in one of the hosted CI `test shard i/4` jobs; the required `test` check passes only when every shard passes. It gives a primary checkout and a linked worktree random synthetic secrets through `env add --secret --value-stdin` (shared and `--local`), runs materialize, exec, up (Compose generation), env apply, logs, list, explain and doctor, then commits everything with `git add -A`. It requires zero occurrences of any value or the project key, raw or hex/base64/base64url encoded, in CLI output, recorded child argv, tracked files, every Git object, HACK_HOME, the private HOME and its caches, and generated Compose and runtime files. Plaintext may exist only in `.hack/.env`, and key text only in key files; each must be git-ignored, untracked and mode 0600. `.hack/.internal/compose.env.override.yml` must hold `${HACK_ENV_...}` references and no value, and must also be private and ignored. Values may reach a child's environment only for the exec'd command and `docker compose`. Positive controls prove delivery and that the scanner finds planted encodings in files and Git objects. Docker is a recording shim; no engine, keychain or installed Hack is reached. | | A dependency vulnerability ships in Hack. | `bun install --frozen-lockfile`, and the read-only `scripts/dependabot-inventory.ts` lists open alerts (#309). Hack does not sandbox project dependencies that run under `hack host exec`; they receive the injected env by design. | ### Agent and MCP exfiltration diff --git a/scripts/ci-bun-crash-census.ts b/scripts/ci-bun-crash-census.ts index c6b853687..f04cce77c 100644 --- a/scripts/ci-bun-crash-census.ts +++ b/scripts/ci-bun-crash-census.ts @@ -3,10 +3,19 @@ * Read-only census of Bun crashes in hosted CI history. * * For a `--since`/`--until` window it lists every run of the CI workflow, reads - * the `test` job of every run attempt, downloads only failed-job logs (all + * the test-suite jobs of every run attempt, downloads only failed-job logs (all * completed logs with `--scan-passing`) into an owned temporary directory, and * classifies each with the shared classifier used by `scripts/bun-crash-loop.ts`. * + * By default the test-suite jobs are every `test shard i/n` job of a run. Since + * HACK-1381 the `test` job is only the required fan-in: it runs no Bun, so its + * log holds nothing to classify. A run with no shard job predates the split, so + * its `test` job ran the suite and is read instead. `--job NAME` selects jobs by + * exact name, or by a pattern where `*` matches any text + * (`--job "test shard *"`). The report warns when no job matched, or when a + * selected job shows no Bun setup in its log or workflow block, because a zero + * crash count from such a job is not evidence. + * * It uses the caller's existing `gh` authentication and never handles a token. * It is strictly read-only: no reruns, comments, labels or uploads. It prints * counts, closed tokens, versions and numeric run/job ids only, never log bodies. @@ -62,6 +71,18 @@ export interface JobRecord { readonly reason: BunRunReason | null; } +/** + * A closed warning. Each one means the census could not see the Bun output a + * zero crash count needs, so the totals are not evidence by themselves. + */ +export type CensusWarning = + | { readonly kind: "no_jobs_matched" } + | { + readonly kind: "no_bun_setup"; + readonly jobs: number; + readonly selected: number; + }; + export interface CrashingJob { readonly runId: number; readonly attempt: number; @@ -85,7 +106,15 @@ export interface VersionTotals { const ISO_INSTANT = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/; const REPOSITORY = /^[A-Za-z0-9_.-]{1,100}\/[A-Za-z0-9_.-]{1,100}$/; const WORKFLOW_FILE = /^[A-Za-z0-9_.-]{1,100}\.ya?ml$/; -const JOB_NAME = /^[A-Za-z0-9_. -]{1,100}$/; +const JOB_NAME = /^[A-Za-z0-9_. /*-]{1,100}$/; +/** A shard job of the hosted test suite (`.github/workflows/ci.yml`, HACK-1381). */ +const TEST_SHARD_JOB = /^test shard \d+\/\d+$/; +/** Before the suite was sharded this job ran it; now it is the logless fan-in. */ +const LEGACY_TEST_JOB = "test"; +const WORKFLOW_JOB_KEY = /^ {2}([A-Za-z0-9_-]+):\s*$/; +const WORKFLOW_JOB_NAME = /^ {4}name:\s*(.+?)\s*$/; +const WORKFLOW_EXPRESSION = /\$\{\{.*?\}\}/; +const WORKFLOW_BUN_VERSION = /^\s+bun-version:\s*["']?([^"'\s]+)["']?\s*$/; const SHA = /^[0-9a-f]{40}$/; /** The runs endpoint returns at most 1000 results for one `created` query. */ const MAX_WINDOW_RUNS = 1000; @@ -107,24 +136,76 @@ const positiveInteger = (value: unknown): number | null => ? value : null; +const escapeRegExp = (text: string): string => + text.replace(/[.*+?^${}()|[\]\\/]/g, "\\$&"); + +/** Whether a job display name matches a `--job` value (`*` matches any text). */ +export function jobNameMatches(pattern: string, name: string): boolean { + return new RegExp( + `^${pattern.split("*").map(escapeRegExp).join(".*")}$` + ).test(name); +} + /** - * The `bun-version` given to the named job in a workflow file, or null. The - * job block runs from ` :` to the next two-space-indented key. + * Whether a workflow job with key `key` and an optional `name:` template has + * the display name `name`. A `${{ ... }}` expression in the template matches + * any non-empty text, so `test shard ${{ matrix.shard }}/4` names + * `test shard 2/4`. + */ +function workflowJobNamed(opts: { + readonly key: string; + readonly template: string | null; + readonly name: string; +}): boolean { + if (opts.template === null) { + return opts.key === opts.name; + } + const template = opts.template.replace(/^(["'])(.*)\1$/, "$2"); + return new RegExp( + `^${template.split(WORKFLOW_EXPRESSION).map(escapeRegExp).join(".+")}$` + ).test(opts.name); +} + +/** + * The `bun-version` given to the job with display name `job` in a workflow + * file, or null. A job block runs from its two-space-indented key under + * `jobs:` to the next one. Its display name is its `name:` template, or its + * key when it has none, so `test shard 2/4` finds the `test-shard` matrix job. + * A job with no `bun-version`, such as the `test` fan-in, returns null. */ export function workflowJobBunVersion( workflow: string, job: string ): string | null { const lines = workflow.split(/\r?\n/); - const start = lines.findIndex((line) => line.trimEnd() === ` ${job}:`); - if (start === -1) { + const jobsAt = lines.findIndex((line) => line.trimEnd() === "jobs:"); + if (jobsAt === -1) { return null; } - for (const line of lines.slice(start + 1)) { - if (/^ {2}\S/.test(line)) { - return null; + const blocks: { readonly key: string; readonly lines: string[] }[] = []; + for (const line of lines.slice(jobsAt + 1)) { + if (/^\S/.test(line)) { + break; } - const match = /^\s+bun-version:\s*["']?([^"'\s]+)["']?\s*$/.exec(line); + const key = WORKFLOW_JOB_KEY.exec(line)?.[1]; + if (key) { + blocks.push({ key, lines: [] }); + } else { + blocks.at(-1)?.lines.push(line); + } + } + const block = blocks.find((candidate) => + workflowJobNamed({ + key: candidate.key, + template: + candidate.lines + .map((line) => WORKFLOW_JOB_NAME.exec(line)?.[1]) + .find((value) => value !== undefined) ?? null, + name: job, + }) + ); + for (const line of block?.lines ?? []) { + const match = WORKFLOW_BUN_VERSION.exec(line); if (match) { return closedBunVersion(match[1]); } @@ -132,6 +213,38 @@ export function workflowJobBunVersion( return null; } +/** + * Which job names of one run the census reads. With no `--job`, the run's + * `test shard i/n` jobs, or its `test` job when it has no shard job (a run + * from before the split, whose `test` job ran the suite). + */ +export function suiteJobSelector(opts: { + readonly job: string | null; + readonly runJobNames: readonly string[]; +}): (name: string) => boolean { + const { job } = opts; + if (job !== null) { + return (name) => jobNameMatches(job, name); + } + if (opts.runJobNames.some((name) => TEST_SHARD_JOB.test(name))) { + return (name) => TEST_SHARD_JOB.test(name); + } + return (name) => name === LEGACY_TEST_JOB; +} + +/** The warnings for a finished census; see {@link CensusWarning}. */ +export function censusWarnings(records: readonly JobRecord[]): CensusWarning[] { + if (records.length === 0) { + return [{ kind: "no_jobs_matched" }]; + } + const withoutBun = records.filter( + (entry) => entry.versionSource === "unknown" + ).length; + return withoutBun === 0 + ? [] + : [{ kind: "no_bun_setup", jobs: withoutBun, selected: records.length }]; +} + export function emptyTotals(bunVersion: string): VersionTotals { return { bunVersion, @@ -200,7 +313,8 @@ export function aggregateCensus( export interface CensusOptions { readonly repository: string; readonly workflow: string; - readonly job: string; + /** Null reads the test-suite jobs; see {@link suiteJobSelector}. */ + readonly job: string | null; readonly since: string; readonly until: string; readonly scanPassing: boolean; @@ -211,6 +325,7 @@ export interface CensusResult { readonly runs: number; readonly records: readonly JobRecord[]; readonly totals: readonly VersionTotals[]; + readonly warnings: readonly CensusWarning[]; } async function mapConcurrent( @@ -335,11 +450,15 @@ export async function runCensus( } const workflowVersions = new Map>(); - const workflowVersion = (sha: string | null): Promise => { + const workflowVersion = ( + sha: string | null, + job: string + ): Promise => { if (sha === null) { return Promise.resolve(null); } - const cached = workflowVersions.get(sha); + const cacheKey = `${sha}\n${job}`; + const cached = workflowVersions.get(cacheKey); if (cached) { return cached; } @@ -358,11 +477,11 @@ export async function runCensus( } return workflowJobBunVersion( Buffer.from(value.content, "base64").toString("utf8"), - options.job + job ); }) .catch(() => null); - workflowVersions.set(sha, pending); + workflowVersions.set(cacheKey, pending); return pending; }; @@ -372,16 +491,24 @@ export async function runCensus( async (run) => { const seen = new Set(); const records: JobRecord[] = []; - const lines = await gh.lines( - `repos/${repo}/actions/runs/${run.id}/jobs?filter=all&per_page=100`, - ".jobs[] | {id, name, run_attempt, status, conclusion}" - ); - for (const line of lines) { - const job = parseLine(line); + const jobs = ( + await gh.lines( + `repos/${repo}/actions/runs/${run.id}/jobs?filter=all&per_page=100`, + ".jobs[] | {id, name, run_attempt, status, conclusion}" + ) + ).map(parseLine); + const selected = suiteJobSelector({ + job: options.job, + runJobNames: jobs.flatMap((job) => + typeof job.name === "string" ? [job.name] : [] + ), + }); + for (const job of jobs) { const jobId = positiveInteger(job.id); const attempt = positiveInteger(job.run_attempt); if ( - job.name !== options.job || + typeof job.name !== "string" || + !selected(job.name) || job.status !== "completed" || job.conclusion === "skipped" || jobId === null || @@ -399,7 +526,7 @@ export async function runCensus( : null; const fromWorkflow = scanned?.bunVersion ? null - : await workflowVersion(run.headSha); + : await workflowVersion(run.headSha, job.name); records.push({ runId: run.id, attempt, @@ -414,9 +541,26 @@ export async function runCensus( } ); const records = perRun.flat(); - return { runs: runs.length, records, totals: aggregateCensus(records) }; + return { + runs: runs.length, + records, + totals: aggregateCensus(records), + warnings: censusWarnings(records), + }; +} + +/** One closed, value-free line for a warning. */ +export function renderCensusWarning(warning: CensusWarning): string { + if (warning.kind === "no_jobs_matched") { + return "warning: no completed job matched the job selector, so zero crashes is not evidence; check --job against the workflow's job names"; + } + return `warning: ${warning.jobs} of ${warning.selected} selected jobs show no Bun setup in their log or workflow, so their crash count is not evidence; the test job is a fan-in that runs no Bun since HACK-1381, so omit --job to read the test shard jobs`; } +/** The report's `job=` value: the quoted selector, or `default`. */ +const jobLabel = (job: string | null): string => + job === null ? "default" : JSON.stringify(job); + /** Closed, value-free text report. */ export function renderCensus(opts: { readonly options: CensusOptions; @@ -424,7 +568,7 @@ export function renderCensus(opts: { }): string { const { options, result } = opts; const lines = [ - `Bun crash census repo=${options.repository} workflow=${options.workflow} job=${options.job} since=${options.since} until=${options.until} runs=${result.runs} jobs=${result.records.length} scan_passing=${options.scanPassing}`, + `Bun crash census repo=${options.repository} workflow=${options.workflow} job=${jobLabel(options.job)} since=${options.since} until=${options.until} runs=${result.runs} jobs=${result.records.length} scan_passing=${options.scanPassing}`, ]; for (const totals of result.totals) { const sources = Object.entries(totals.versionSources) @@ -439,6 +583,7 @@ export function renderCensus(opts: { ); } } + lines.push(...result.warnings.map(renderCensusWarning)); return `${lines.join("\n")}\n`; } @@ -495,12 +640,12 @@ export function parseCensusArguments(argv: readonly string[]): CensusOptions & { } const repository = values.get("repo") ?? "hack-dance/hack"; const workflow = values.get("workflow") ?? "ci.yml"; - const job = values.get("job") ?? "test"; + const job = values.get("job") ?? null; if ( !( REPOSITORY.test(repository) && WORKFLOW_FILE.test(workflow) && - JOB_NAME.test(job) + (job === null || JOB_NAME.test(job)) ) ) { throw new CensusError("Invalid argument"); @@ -583,9 +728,12 @@ async function main(): Promise { const result = await runCensus(options, ghCliReader, workDir); process.stdout.write( json - ? `${JSON.stringify({ options, runs: result.runs, totals: result.totals }, null, 2)}\n` + ? `${JSON.stringify({ options, runs: result.runs, totals: result.totals, warnings: result.warnings }, null, 2)}\n` : renderCensus({ options, result }) ); + for (const warning of result.warnings) { + process.stderr.write(`${renderCensusWarning(warning)}\n`); + } } finally { process.off("SIGINT", interrupt); process.off("SIGTERM", interrupt); diff --git a/tests/ci-bun-crash-census.test.ts b/tests/ci-bun-crash-census.test.ts index ca037363d..acf751556 100644 --- a/tests/ci-bun-crash-census.test.ts +++ b/tests/ci-bun-crash-census.test.ts @@ -1,15 +1,17 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdtemp, readdir, rm } from "node:fs/promises"; +import { mkdtemp, readdir, readFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, resolve } from "node:path"; import { aggregateCensus, type CensusOptions, type GhReader, type JobRecord, + jobNameMatches, parseCensusArguments, renderCensus, runCensus, + suiteJobSelector, workflowJobBunVersion, } from "../scripts/ci-bun-crash-census.ts"; import { @@ -407,12 +409,26 @@ test("census arguments are validated and closed", () => { ).toEqual({ repository: "hack-dance/hack", workflow: "ci.yml", - job: "test", + job: null, since: "2026-10-01T17:35:00Z", until: "2026-10-11T23:43:30Z", scanPassing: true, json: false, }); + const window = [ + "--since", + "2026-10-01T17:35:00Z", + "--until", + "2026-10-11T23:43:30Z", + ]; + // The verify skill documents `--job "test shard i/4"`; a `/` or `*` used to be refused. + expect(parseCensusArguments([...window, "--job", "test shard 1/4"]).job).toBe( + "test shard 1/4" + ); + expect(parseCensusArguments([...window, "--job=test shard *"]).job).toBe( + "test shard *" + ); + expect(parseCensusArguments([...window, "--job", "test"]).job).toBe("test"); for (const argv of [ [], ["--since", "2026-10-01", "--until", "2026-10-11T23:43:30Z"], @@ -440,7 +456,238 @@ test("census arguments are validated and closed", () => { "--json", "--json", ], + [...window, "--job", "test;shard"], + [...window, "--job", "t".repeat(101)], ]) { expect(() => parseCensusArguments(argv)).toThrow(); } }); + +describe("sharded test suite (HACK-1424)", () => { + const SHARDED_WORKFLOW = [ + "name: CI", + "on:", + " push:", + " pull_request:", + "jobs:", + " test-shard:", + " name: test shard ${{ matrix.shard }}/4", + " steps:", + " - uses: oven-sh/setup-bun@v1", + " with:", + ' bun-version: "1.4.2"', + " test:", + " needs: test-shard", + " if: ${{ always() }}", + " steps:", + " - name: Require every test shard to succeed", + " run: exit 0", + " quoted:", + ' name: "quoted ${{ matrix.os }}"', + " steps:", + " - with:", + " bun-version: 1.3.9", + ].join("\n"); + + test("a shard job's Bun version comes from its name template; the fan-in has none", () => { + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "test shard 3/4")).toBe( + "1.4.2" + ); + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "test")).toBeNull(); + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "test shard /4")).toBeNull(); + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "test-shard")).toBeNull(); + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "quoted macos")).toBe( + "1.3.9" + ); + // `on:` keys are not jobs. + expect(workflowJobBunVersion(SHARDED_WORKFLOW, "push")).toBeNull(); + }); + + test("the real CI workflow gives every shard the pinned Bun and the test fan-in none", async () => { + const workflow = await readFile( + resolve(import.meta.dir, "../.github/workflows/ci.yml"), + "utf8" + ); + const manifest: unknown = JSON.parse( + await readFile(resolve(import.meta.dir, "../package.json"), "utf8") + ); + const pin = + typeof manifest === "object" && + manifest !== null && + "packageManager" in manifest && + typeof manifest.packageManager === "string" + ? manifest.packageManager.replace(/^bun@/, "") + : null; + expect(pin).toMatch(/^\d+\.\d+\.\d+$/); + for (const shard of [1, 2, 3, 4]) { + expect(workflowJobBunVersion(workflow, `test shard ${shard}/4`)).toBe( + pin + ); + } + expect(workflowJobBunVersion(workflow, "test")).toBeNull(); + }); + + test("job selectors: default reads shards, or the legacy test job when a run has none", () => { + const sharded = suiteJobSelector({ + job: null, + runJobNames: ["test shard 1/4", "test shard 2/4", "test", "docker-e2e"], + }); + expect( + ["test shard 1/4", "test shard 2/4", "test", "docker-e2e shard 1/4"].map( + sharded + ) + ).toEqual([true, true, false, false]); + const legacy = suiteJobSelector({ + job: null, + runJobNames: ["test", "runtime-images"], + }); + expect(["test", "runtime-images"].map(legacy)).toEqual([true, false]); + expect(jobNameMatches("test shard *", "test shard 4/4")).toBe(true); + expect(jobNameMatches("test shard *", "test")).toBe(false); + expect(jobNameMatches("test shard 1/4", "test shard 1/4")).toBe(true); + expect(jobNameMatches("test shard 1/4", "test shard 1/40")).toBe(false); + expect(jobNameMatches("test.shard", "testXshard")).toBe(false); + }); + + const FAN_IN_FAILURE_LOG = [ + stamp('##[group]Run if [ "$SHARD_RESULT" != "success" ]; then'), + stamp("test shards did not all succeed (result: failure)"), + stamp("##[error]Process completed with exit code 1."), + ].join("\n"); + const SHA_SHARDED = "c".repeat(40); + + function shardedGh() { + const downloads: string[] = []; + const logs: Record = { + 31: CRASH_LOG, + 33: FAN_IN_FAILURE_LOG, + 41: TEST_FAILURE_LOG, + }; + const job = (opts: { + readonly id: number; + readonly name: string; + readonly conclusion: string; + readonly attempt?: number; + }): string => + JSON.stringify({ + id: opts.id, + name: opts.name, + run_attempt: opts.attempt ?? 1, + status: "completed", + conclusion: opts.conclusion, + }); + const reader: GhReader = { + json: async (path) => { + if (path.includes("/runs?created=")) { + return { total_count: 2 }; + } + if (path.endsWith(`?ref=${SHA_SHARDED}`)) { + return { + content: Buffer.from(SHARDED_WORKFLOW).toString("base64"), + }; + } + if (path.endsWith(`?ref=${SHA_OLD}`)) { + return { content: workflowFor("1.3.9") }; + } + throw new Error("unexpected path"); + }, + lines: async (path) => { + if (path.includes("/runs?created=")) { + return [ + JSON.stringify({ id: 300, head_sha: SHA_SHARDED }), + JSON.stringify({ id: 400, head_sha: SHA_OLD }), + ]; + } + if (path.includes("/runs/300/jobs?filter=all")) { + return [ + job({ id: 31, name: "test shard 1/4", conclusion: "failure" }), + job({ id: 32, name: "test shard 2/4", conclusion: "success" }), + job({ id: 33, name: "test", conclusion: "failure" }), + job({ + id: 34, + name: "docker-e2e shard 1/4", + conclusion: "failure", + }), + ]; + } + if (path.includes("/runs/400/jobs?filter=all")) { + return [ + job({ id: 41, name: "test", conclusion: "failure" }), + job({ id: 42, name: "test", conclusion: "success", attempt: 2 }), + ]; + } + throw new Error("unexpected path"); + }, + download: async (path, destination) => { + const jobId = Number(/\/jobs\/(\d+)\/logs$/.exec(path)?.[1]); + downloads.push(path); + await Bun.write(destination, logs[jobId] ?? ""); + }, + }; + return { reader, downloads }; + } + + test("with no --job the census reads shard jobs, skips the fan-in, and still reads pre-split runs", async () => { + const dir = await workDir(); + const gh = shardedGh(); + const defaults: CensusOptions = { ...options, job: null }; + const result = await runCensus(defaults, gh.reader, dir); + expect(gh.downloads).toEqual([ + "repos/hack-dance/hack/actions/jobs/31/logs", + "repos/hack-dance/hack/actions/jobs/41/logs", + ]); + expect(await readdir(dir)).toEqual([]); + expect( + result.records.map((entry) => [ + entry.jobId, + entry.bunVersion, + entry.versionSource, + ]) + ).toEqual([ + [31, "1.3.9", "log"], + [32, "1.4.2", "workflow"], + [41, "1.4.2", "log"], + [42, "1.3.9", "workflow"], + ]); + expect(result.warnings).toEqual([]); + const text = renderCensus({ options: defaults, result }); + expect(text).toContain(" job=default "); + expect(text).toContain(" crash run=300 attempt=1 job=31 reason=banner"); + expect(text).not.toContain("warning:"); + }); + + test("an explicit --job test on sharded runs warns that the fan-in has no Bun output", async () => { + const dir = await workDir(); + const gh = shardedGh(); + const fanIn: CensusOptions = { ...options, job: "test" }; + const result = await runCensus(fanIn, gh.reader, dir); + expect(result.records.map((entry) => entry.jobId)).toEqual([33, 41, 42]); + expect(result.records[0]).toMatchObject({ + bunVersion: "unknown", + versionSource: "unknown", + scan: "failure", + }); + expect(result.warnings).toEqual([ + { kind: "no_bun_setup", jobs: 1, selected: 3 }, + ]); + const text = renderCensus({ options: fanIn, result }); + expect(text).toContain(' job="test" '); + expect(text).toContain( + "warning: 1 of 3 selected jobs show no Bun setup in their log or workflow" + ); + expect(text).not.toContain("SHARD_RESULT"); + }); + + test("a selector that matches no job warns instead of reporting a silent zero", async () => { + const dir = await workDir(); + const gh = shardedGh(); + const none: CensusOptions = { ...options, job: "test shard 9/9" }; + const result = await runCensus(none, gh.reader, dir); + expect(result.records).toEqual([]); + expect(gh.downloads).toEqual([]); + expect(result.warnings).toEqual([{ kind: "no_jobs_matched" }]); + expect(renderCensus({ options: none, result })).toContain( + "warning: no completed job matched the job selector" + ); + }); +}); From 1f30cbe59cec75dafb1783dda31685e0848f3199 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 02:05:29 -0400 Subject: [PATCH 3/6] fix(logs): name Alloy read positions in logs-reset; pin the exit drain bound and Loki hint wiring - Pin NATIVE_EXIT_DRAIN_BOUND_MS at its documented 5 s floor and above the late-writer delay, and assert the held-descendant cases (runtime client and foreground graph owner) keep reading for the whole production bound, so a smaller constant or default no longer passes. - Add late-closed-code cases for the one-off and recovery (selection and cleanup) bound drains. - Add CLI cases that carry --pretty, --query and --until into the empty-Loki hint through src/commands/project.ts. - `hack global logs-reset` now says it also deletes the Alloy read positions volume in its prompt and summary; regenerate docs/reference/cli.md. Refs HACK-1425 Co-Authored-By: Claude Opus 5.5 --- docs/reference/cli.md | 4 +- src/backends/native-runtime-client.ts | 4 +- src/commands/global.ts | 5 +- tests/logs-loki-empty-fallback-cli.test.ts | 38 ++++++++++++ tests/native-project-process.test.ts | 10 ++++ tests/native-runtime-client.test.ts | 70 ++++++++++++++++++++++ 6 files changed, 126 insertions(+), 5 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index bf4683833..2038984cf 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1037,7 +1037,7 @@ hack global [options] | `hack global ca` | Export Caddy Local CA cert (print path or PEM) | | `hack global cert ` | Generate local TLS certs via mkcert (for non-Caddy services) | | `hack global trust` | Trust Caddy Local CA (macOS) so https://*.hack is trusted | -| `hack global logs-reset` | Wipe Loki/Grafana volumes (fresh logs + dashboards) | +| `hack global logs-reset` | Wipe Loki, Grafana and Alloy volumes (fresh logs, dashboards and Alloy read positions) | | `hack global authorize` | Authorize passwordless DNS recovery commands on macOS | ### Options @@ -1242,7 +1242,7 @@ hack global trust [options] ## `hack global logs-reset` -Wipe Loki/Grafana volumes (fresh logs + dashboards) +Wipe Loki, Grafana and Alloy volumes (fresh logs, dashboards and Alloy read positions) ### Usage diff --git a/src/backends/native-runtime-client.ts b/src/backends/native-runtime-client.ts index 30626dfa4..1a64ca6e0 100644 --- a/src/backends/native-runtime-client.ts +++ b/src/backends/native-runtime-client.ts @@ -103,7 +103,9 @@ const MAX_PRIVATE_INPUT_BYTES = 256 * 1024; * heavily oversubscribed host still schedules the pipe readers before it * expires. A 250 ms bound lost the closed code under load for the foreground * graph owner (HACK-1350) and for bounded one-shot requests (HACK-1387). - * Shared by {@link invokeNativeRuntime} and the foreground graph owner. + * Shared by {@link invokeNativeRuntime} and the foreground graph owner. The + * documented floor is 5 s; `tests/native-runtime-client.test.ts` fails if the + * bound drops below it (HACK-1425). */ export const NATIVE_EXIT_DRAIN_BOUND_MS = 5000; const REQUEST_BUDGET_REFUSAL = diff --git a/src/commands/global.ts b/src/commands/global.ts index 086602dd3..0cd9cbd2f 100644 --- a/src/commands/global.ts +++ b/src/commands/global.ts @@ -375,7 +375,8 @@ const globalTrustSpec = defineCommand({ const globalLogsResetSpec = defineCommand({ name: "logs-reset", - summary: "Wipe Loki/Grafana volumes (fresh logs + dashboards)", + summary: + "Wipe Loki, Grafana and Alloy volumes (fresh logs, dashboards and Alloy read positions)", group: "Global", options: [], positionals: [], @@ -1202,7 +1203,7 @@ async function globalLogsReset(): Promise { const ok = await confirmSafe({ message: - "This will stop the logging stack and delete ALL Loki logs and Grafana state (volumes).\nContinue?", + "This will stop the logging stack and delete its volumes: ALL Loki logs, Grafana state and Alloy read positions.\nContinue?", initialValue: false, nonInteractive: "accept-default", }); diff --git a/tests/logs-loki-empty-fallback-cli.test.ts b/tests/logs-loki-empty-fallback-cli.test.ts index 8cad3aa3e..e0e79c008 100644 --- a/tests/logs-loki-empty-fallback-cli.test.ts +++ b/tests/logs-loki-empty-fallback-cli.test.ts @@ -278,6 +278,44 @@ test("explicit --loki snapshot hint turns a single --services value into the pos expect(await composeLogsCalls()).toEqual([]); }, 60_000); +// HACK-1425: `--pretty` reaches the hint through the CLI wiring, not only +// through the renderer's unit tests. +test("explicit --loki snapshot hint repeats --pretty", async () => { + const result = await runLogs([ + "--loki", + "--no-follow", + "--pretty", + "migrate", + ]); + + expect(result.exitCode).toBe(0); + expect(lokiQueries).toEqual(['{project="oneshot",service="migrate"}']); + expect(result.stderr).toContain( + "try `hack logs --compose --no-follow --pretty migrate`." + ); + expect(await composeLogsCalls()).toEqual([]); +}, 60_000); + +// HACK-1425: a custom `--query` selects the Loki stream and, with `--until`, +// is named in the hint as a selector `--compose` cannot apply. +test("explicit --query snapshot hint names --query and --until", async () => { + const query = '{project="oneshot",service="migrate"} |= "applied"'; + const result = await runLogs([ + "--no-follow", + "--query", + query, + "--until", + "1m", + ]); + + expect(result.exitCode).toBe(0); + expect(lokiQueries).toEqual([query]); + expect(result.stderr).toContain( + `try \`hack logs --compose --no-follow\`. The compose backend has no equivalent for --query '${query}' and --until 1m.` + ); + expect(await composeLogsCalls()).toEqual([]); +}, 60_000); + test("Loki follow backend notices an empty tail on stderr and keeps following", async () => { await writeFile( join(projectRoot, ".hack", "hack.config.json"), diff --git a/tests/native-project-process.test.ts b/tests/native-project-process.test.ts index 5d007846f..6eefb7c95 100644 --- a/tests/native-project-process.test.ts +++ b/tests/native-project-process.test.ts @@ -230,6 +230,11 @@ test("readiness rejection retains identity while observing safe native cleanup c /** Slack for host load between the stand-in's recorded exit and settlement. */ const DRAIN_SETTLE_MARGIN_MS = 5000; +/** + * How much earlier than its delay a timer may appear to fire when measured + * with whole-millisecond wall clocks in two processes. + */ +const TIMER_CLOCK_TOLERANCE_MS = 50; /** * Fixed delay before the descendant writes the closed code, measured from the * test observing the stand-in reaped: well above the former 250 ms window. @@ -414,6 +419,11 @@ test.each([ expect(settledAt - exitedAt).toBeLessThan( NATIVE_EXIT_DRAIN_BOUND_MS + DRAIN_SETTLE_MARGIN_MS ); + // No seam is passed, so the production bound is the one in effect: + // the drain kept reading the held pipes for all of it (HACK-1425). + expect(settledAt - exitedAt).toBeGreaterThanOrEqual( + NATIVE_EXIT_DRAIN_BOUND_MS - TIMER_CLOCK_TOLERANCE_MS + ); if (ready) { expect(caught).toBeUndefined(); expect(result).toBe(0); diff --git a/tests/native-runtime-client.test.ts b/tests/native-runtime-client.test.ts index 2710cb20d..3a416915f 100644 --- a/tests/native-runtime-client.test.ts +++ b/tests/native-runtime-client.test.ts @@ -56,6 +56,17 @@ const LATE_CODE_DELAY_MS = 1000; * bound and prove that it still ends a drain EOF cannot. */ const LATE_WRITER_DRAIN_BOUND_MS = 25_000; +/** + * The production exit drain bound's documented floor (the doc comment on + * NATIVE_EXIT_DRAIN_BOUND_MS). Every other case reads the constant itself, so + * without this pin a smaller production bound would pass them all (HACK-1425). + */ +const DOCUMENTED_EXIT_DRAIN_BOUND_MS = 5000; +/** + * How much earlier than its delay a timer may appear to fire when measured + * with whole-millisecond wall clocks in two processes. + */ +const TIMER_CLOCK_TOLERANCE_MS = 50; /** * Gap between creating a stand-in's pid file and writing its digits. It makes * the empty-file window that a loaded host opens by chance happen on every @@ -391,6 +402,17 @@ test("native recovery draining requires exact independent selectors and refuses } }); +test("the production exit drain bound keeps its documented floor and outlasts a late writer", () => { + expect(NATIVE_EXIT_DRAIN_BOUND_MS).toBeGreaterThanOrEqual( + DOCUMENTED_EXIT_DRAIN_BOUND_MS + ); + // A closed code written this long after the exit must still be read under + // the production bound, not only under the late-writer cases' wide seam. + expect(NATIVE_EXIT_DRAIN_BOUND_MS).toBeGreaterThan( + LATE_CODE_DELAY_MS + TIMER_CLOCK_TOLERANCE_MS + ); +}); + bunTest.each([ ["read", "exit"], ["read", "abort"], @@ -481,6 +503,13 @@ ${mode === "abort" ? "await Bun.sleep(60_000);" : `(await import('node:fs')).wri expect((settledAt ?? Number.POSITIVE_INFINITY) - endedAt).toBeLessThan( NATIVE_EXIT_DRAIN_BOUND_MS + DRAIN_SETTLE_MARGIN_MS ); + if (mode === "exit") { + // No seam is passed, so the production bound is the one in effect: + // the drain kept reading the held pipes for all of it (HACK-1425). + expect((settledAt ?? 0) - endedAt).toBeGreaterThanOrEqual( + NATIVE_EXIT_DRAIN_BOUND_MS - TIMER_CLOCK_TOLERANCE_MS + ); + } expect(await Bun.file(finished).exists()).toBe(false); const keeperPid = await waitForPid(holding); expect(() => process.kill(keeperPid, 0)).not.toThrow(); @@ -561,6 +590,47 @@ const LATE_WRITER_SELECTIONS = { ], flags: { boundNativeAuthoredExecDrain: true }, }, + "one-off": { + args: [ + "graph", + "native", + "one-off", + "--run-id", + RUN_ID, + "--service", + "web", + "--json", + "--", + "true", + ], + flags: { boundNativeAuthoredOneOffDrain: true }, + }, + selection: { + args: [ + "graph", + "native", + "recovery-selection", + "--run-id", + RUN_ID, + "--json", + ], + flags: { boundNativeAuthoredRecoveryDrain: "selection" }, + }, + cleanup: { + args: [ + "graph", + "native", + "recover-live-owner", + "--run-id", + RUN_ID, + "--expect-receipt", + "b".repeat(64), + "--expect-owner", + "c".repeat(64), + "--json", + ], + flags: { boundNativeAuthoredRecoveryDrain: "cleanup" }, + }, } as const; bunTest.each( From d43027e660d07bde82e5839bcbd5ce3858a19edb Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 02:09:30 -0400 Subject: [PATCH 4/6] fix(cli): close HACK-1426 follow-ups (up E_PROJECT_NOT_FOUND, git-accurate gitignore entry, no-TTY Tailscale test) - `hack up` without a project now prints the closed `E_PROJECT_NOT_FOUND: ...` line like down, restart, ps, logs and open. - ensureGitignoreEntry reads each line as git does (trailing CR and spaces dropped, leading whitespace kept), so an indented `.hack.secret.key` no longer counts as the entry and is no longer refused as "re-included"; the exact entry is appended instead. - tests: no-TTY guard test for "Turn on Tailscale?", a named warm-up for the cold transpiler cache in that file, the ACL test's setup moved inside its try, and the paired mode-000 `.hack` fixture without `.hack/.internal` restored. Refs HACK-1426 Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-target-result-contract.md | 2 +- src/commands/project.ts | 22 +++-- src/lib/fs.ts | 24 ++++- tests/git-info-exclude.test.ts | 18 ++-- tests/missing-project-error-code.test.ts | 37 +++++++- tests/no-tty-prompt-sites-cli.test.ts | 99 +++++++++++++++++++-- tests/project-env-key-ignore.test.ts | 43 +++++++++ tests/project-target-inspect.test.ts | 30 +++++-- 8 files changed, 236 insertions(+), 39 deletions(-) diff --git a/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index bce008873..c4b2c5aeb 100644 --- a/docs/reference/ax-target-result-contract.md +++ b/docs/reference/ax-target-result-contract.md @@ -228,7 +228,7 @@ WU21). The inspector itself only produces snapshots. | Malformed Hack config or Compose file, oversized input | `config_parse_failed`, `compose_parse_failed`, `generation_input_too_large` | same | paired fixture | | Unreadable input (directory in place of a file, no read permission) | `input_unreadable`, `input_permission_denied` | same | paired fixture (permission case skipped as root) | | Input guard refusals (native/legacy conflict, unsafe adoption state, non-directory project dir) | `project_inputs_unsupported`, `adoption_state_unsafe`, `input_unreadable` | same | paired fixture | -| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test, each at mode `000` and `0644` on both directories (skipped as root; the message is checked unconditionally) | +| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test, each at mode `000` and `0644` on both directories, plus a paired `.hack` at `000` with no `.hack/.internal` (skipped as root; the message is checked unconditionally) | | Uncheckable input presence (`.dev` file or unsearchable `.dev` beside a valid `.hack`) | `input_unreadable`, `input_permission_denied` | same | paired fixture and domain test (unsearchable case skipped as root; `EPERM` through the classifier seam and an injected presence-check fault) | | Detached linked worktree, no project, missing path | closed refusals | same | paired fixture | | Unknown service | `service_not_found` | same | paired fixture | diff --git a/src/commands/project.ts b/src/commands/project.ts index b801775b6..8c37f4a73 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -5335,18 +5335,16 @@ async function runUpCommand({ projectOpt: args.options.project, }); } catch (error: unknown) { - if (error instanceof MissingProjectContextError) { - if (json) { - return emitLifecycleResult({ - result: errorResult({ - code: "E_PROJECT_NOT_FOUND", - message: error.message, - }), - exitCode: 1, - }); - } - logger.error({ message: error.message }); - return 1; + // Without --json the error reaches the CLI's closed-error renderer, like + // `down` and `restart`: one `E_PROJECT_NOT_FOUND: ...` line, no stack. + if (json && error instanceof MissingProjectContextError) { + return emitLifecycleResult({ + result: errorResult({ + code: "E_PROJECT_NOT_FOUND", + message: error.message, + }), + exitCode: 1, + }); } throw error; } diff --git a/src/lib/fs.ts b/src/lib/fs.ts index fd17e01ca..96bf4114b 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -701,8 +701,28 @@ export function mergeManagedGitignoreBlock(opts: { return `${base}${separator}${block}\n`; } +/** A line ending's carriage return, which git drops from an ignore line. */ +const GITIGNORE_LINE_CR = /\r$/; +/** Trailing spaces, which git drops from an ignore line (only spaces). */ +const GITIGNORE_TRAILING_SPACES = / +$/; + +/** + * The pattern git reads from one `.gitignore` line: a trailing carriage return + * and trailing spaces are dropped, while leading whitespace is part of the + * pattern (` .hack.secret.key` names a file that starts with two spaces). + * Escaped trailing spaces (`\ `) are not unescaped: no Hack entry ends in one. + */ +function gitignoreLinePattern(line: string): string { + return line + .replace(GITIGNORE_LINE_CR, "") + .replace(GITIGNORE_TRAILING_SPACES, ""); +} + /** * Ensures an entry exists in .gitignore. Creates the file if it doesn't exist. + * A line counts as the entry only when git reads it as the same pattern (see + * {@link gitignoreLinePattern}), so an indented copy, which git does not apply + * to the entry's path, does not stop the entry from being added. * @returns Whether the file was modified */ export async function ensureGitignoreEntry(opts: { @@ -715,7 +735,9 @@ export async function ensureGitignoreEntry(opts: { if (existing !== null) { const lines = existing.split("\n"); - const hasEntry = lines.some((line) => line.trim() === entryLine); + const hasEntry = lines.some( + (line) => gitignoreLinePattern(line) === entryLine + ); if (hasEntry) { return { changed: false }; } diff --git a/tests/git-info-exclude.test.ts b/tests/git-info-exclude.test.ts index 67688c785..1de846017 100644 --- a/tests/git-info-exclude.test.ts +++ b/tests/git-info-exclude.test.ts @@ -161,15 +161,17 @@ test.skipIf(process.platform !== "darwin")( async () => { const exclude = join(repo, ".git", "info", "exclude"); await writeFile(exclude, "# user rule\n"); - await chmod(exclude, 0o444); - const acl = Bun.spawnSync( - ["/bin/chmod", "+a", `${userInfo().username} allow write`, exclude], - { stdin: "ignore", stdout: "ignore", stderr: "ignore" } - ); - expect(acl.exitCode).toBe(0); - // The ACL makes access(W_OK) succeed; only the mode bits refuse it. - await access(exclude, constants.W_OK); + // Every step after the mode change runs inside the try, so a failed ACL + // grant or probe still restores mode 0644 for the cleanup. try { + await chmod(exclude, 0o444); + const acl = Bun.spawnSync( + ["/bin/chmod", "+a", `${userInfo().username} allow write`, exclude], + { stdin: "ignore", stdout: "ignore", stderr: "ignore" } + ); + expect(acl.exitCode).toBe(0); + // The ACL makes access(W_OK) succeed; only the mode bits refuse it. + await access(exclude, constants.W_OK); await ensureHackGitInfoExclude({ projectDir: join(repo, ".hack") }); expect(await readFile(exclude, "utf8")).toBe("# user rule\n"); expect((await lstat(exclude)).mode & 0o777).toBe(0o444); diff --git a/tests/missing-project-error-code.test.ts b/tests/missing-project-error-code.test.ts index 604c171ff..b5284c8f5 100644 --- a/tests/missing-project-error-code.test.ts +++ b/tests/missing-project-error-code.test.ts @@ -245,11 +245,13 @@ async function emptyProject() { async function cli(opts: { readonly root: string; readonly args: readonly string[]; + /** Working directory; defaults to the fixture root. */ + readonly cwd?: string; }) { const child = Bun.spawn( [process.execPath, resolve("index.ts"), ...opts.args], { - cwd: opts.root, + cwd: opts.cwd ?? opts.root, env: privateCliEnv({ root: opts.root, env: { HACK_NO_INTERACTIVE: "1", HACK_LOGGER: "console" }, @@ -324,11 +326,40 @@ for (const [name, expectation] of Object.entries(COMMANDS)) { ); } +/** + * The commands pinned above in their `--json` form. Without `--json` each + * prints the same closed line (HACK-1426: `up` used to drop the code). + */ +const ENVELOPE_COMMANDS_WITHOUT_JSON = [ + "up", + "down", + "restart", + "ps", + "logs", + "open", +] as const; + +for (const command of ENVELOPE_COMMANDS_WITHOUT_JSON) { + test( + `${command} -p without --json prints one closed line and no stack`, + async () => { + const { root, dir } = await emptyProject(); + const result = await cli({ root, args: [command, "-p", dir] }); + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe( + `ERROR: E_PROJECT_NOT_FOUND: ${MISSING_PROJECT_MESSAGE}\n` + ); + }, + SPAWN_TIMEOUT_MS + ); +} + test( - "ps -p without --json prints one closed line and no stack", + "up from a working directory without a project prints the closed E_PROJECT_NOT_FOUND line", async () => { const { root, dir } = await emptyProject(); - const result = await cli({ root, args: ["ps", "-p", dir] }); + const result = await cli({ root, cwd: dir, args: ["up"] }); expect(result.code).toBe(1); expect(result.stdout).toBe(""); expect(result.stderr).toBe( diff --git a/tests/no-tty-prompt-sites-cli.test.ts b/tests/no-tty-prompt-sites-cli.test.ts index 8e3df4261..48742691b 100644 --- a/tests/no-tty-prompt-sites-cli.test.ts +++ b/tests/no-tty-prompt-sites-cli.test.ts @@ -33,7 +33,9 @@ import { privateCliEnv } from "./helpers/private-cli-env.ts"; * and `HACK_SESSIONS_MUX=none` (no tmux or zellij is run). `secrets` children * preload a `Bun.secrets` stub, so no case reaches the OS keychain even when a * guard is mutated away. `update` children preload a fetch stub, so nothing - * reaches the network. No Docker socket, launchd or keychain is touched. + * reaches the network. The `ssh --tailscale` child finds a logging + * `tailscale` stand-in first on PATH, never the real CLI. No Docker socket, + * launchd or keychain is touched. */ const REPO_ROOT = resolve(import.meta.dir, ".."); @@ -51,8 +53,17 @@ const ATTACHED_PROMPT_WORKER = join( * shared Bun cache is warm; the regressed prompt loop never exits. */ const KILL_AFTER_MS = 10_000; -/** Overall bound for each test, covering a cold first start. */ +/** Overall bound for each test. */ const TEST_TIMEOUT_MS = 30_000; +/** + * Bound for the one warm-up child in `beforeAll`. Each file run starts with a + * fresh private HOME, so Bun's runtime transpiler cache (kept under HOME) is + * cold, and a cold start of the CLI took 6.3 s on a loaded host (HACK-1426). + * The warm-up pays that cost once, outside every case's {@link KILL_AFTER_MS}. + */ +const WARM_UP_KILL_AFTER_MS = 90_000; +/** `beforeAll` bound: the warm-up child plus margin. */ +const WARM_UP_HOOK_TIMEOUT_MS = WARM_UP_KILL_AFTER_MS + 10_000; /** Clack's active-prompt marker: no case may render a live prompt. */ const ACTIVE_PROMPT_MARKER = "◆"; @@ -67,10 +78,47 @@ type RunResult = { let fileRoot: string | null = null; let caseRoot: string | null = null; -beforeAll(async () => { - // One HOME per file keeps Bun's transpiler cache warm across cases. - fileRoot = await mkdtemp(join(tmpdir(), "hack-no-tty-prompt-sites-")); -}); +beforeAll( + async () => { + // One HOME per file keeps Bun's transpiler cache warm across cases. + fileRoot = await mkdtemp(join(tmpdir(), "hack-no-tty-prompt-sites-")); + await warmTranspilerCache(fileRoot); + }, + { timeout: WARM_UP_HOOK_TIMEOUT_MS } +); + +/** + * Loads the CLI's module graph once under the file's private HOME, so no case + * pays the cold transpile inside its {@link KILL_AFTER_MS} bound. `--help` + * needs no project, prompt, network or keychain. Only a kill fails the file: + * the cases themselves pin every exit code and message. + */ +async function warmTranspilerCache(root: string): Promise { + const proc = Bun.spawn([process.execPath, ENTRYPOINT, "--help"], { + cwd: root, + env: privateCliEnv({ + root, + env: { + HACK_HOME: join(root, "warm-up-hack-home"), + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_SESSIONS_MUX: "none", + HACK_NO_INTERACTIVE: "1", + NO_COLOR: "1", + }, + }), + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + timeout: WARM_UP_KILL_AFTER_MS, + killSignal: "SIGKILL", + }); + await proc.exited; + if (proc.signalCode !== null) { + throw new Error( + `CLI warm-up was killed (${proc.signalCode}) after ${WARM_UP_KILL_AFTER_MS} ms` + ); + } +} afterAll(async () => { if (fileRoot) { @@ -369,6 +417,45 @@ test( { timeout: TEST_TIMEOUT_MS } ); +test( + "ssh --tailscale does not offer to turn on a stopped Tailscale without a TTY", + async () => { + // A `tailscale` stand-in on PATH reports a stopped backend and logs each + // call, so the test sees whether `tailscale up` would have run. + const bin = join(requireCaseRoot(), "bin"); + await mkdir(bin, { recursive: true }); + const callLog = join(requireCaseRoot(), "tailscale.log"); + const stub = join(bin, "tailscale"); + await writeFile( + stub, + [ + "#!/bin/sh", + `printf '%s\\n' "$*" >> '${callLog}'`, + 'if [ "$1" = status ]; then', + ` printf '%s\\n' '{"BackendState":"Stopped"}'`, + "fi", + "exit 0", + "", + ].join("\n") + ); + await chmod(stub, 0o755); + const result = await runPiped({ + args: ["ssh", "--tailscale"], + env: { PATH: `${bin}:/usr/bin:/bin` }, + }); + expectSettled(result); + expect(result.exitCode).toBe(1); + const output = `${result.stdout}${result.stderr}`; + expect(output).toContain("Tailscale is stopped"); + expect(output).toContain("Run `tailscale up`, then re-run `hack ssh`."); + expect(output).not.toContain("Turn on Tailscale?"); + const calls = (await readFile(callLog, "utf8")).split("\n"); + expect(calls).toContain("status --json"); + expect(calls).not.toContain("up"); + }, + { timeout: TEST_TIMEOUT_MS } +); + test( "ssh --direct host prompt refuses without a TTY and names --host", async () => { diff --git a/tests/project-env-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts index 252807bfe..3c2845a5f 100644 --- a/tests/project-env-key-ignore.test.ts +++ b/tests/project-env-key-ignore.test.ts @@ -17,6 +17,7 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; +import { ensureGitignoreEntry } from "../src/lib/fs.ts"; import { resolveProjectEnvConfigPath } from "../src/lib/project-env-config.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -376,6 +377,48 @@ test("a rule that re-includes the key is refused both before creating a key and expect(await readFile(repo.layer, "utf8")).toBe(layerBefore); }, 60_000); +test("an indented entry, which git does not apply, is completed by an exact entry instead of being refused as re-included", async () => { + // HACK-1426: git keeps leading whitespace in a pattern, so this line does + // not ignore the key; Hack must not count it as the entry. + const repo = await createRepo({ git: true }); + const indented = `${BASE_GITIGNORE} ${PROJECT_ENV_KEY_FILENAME}\n`; + await writeFile(repo.gitignore, indented); + + const run = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); + + expect(run.exitCode).toBe(0); + expect(`${run.stdout}${run.stderr}`).not.toContain("re-includes the key"); + expect(await keyExists(repo)).toBe(true); + expectNoKeyMaterial(run, (await readFile(repo.keyPath, "utf8")).trim()); + expect(await readFile(repo.gitignore, "utf8")).toBe( + `${indented}\n# project env key\n${PROJECT_ENV_KEY_FILENAME}\n` + ); + expect(await keyIsCommittable(repo)).toBe(false); +}, 60_000); + +test("ensureGitignoreEntry reads each line as git does: trailing spaces and CR dropped, leading whitespace kept", async () => { + const root = await mkdtemp(join(tmpdir(), "hack-gitignore-entry-")); + tempDirs.add(root); + const path = join(root, ".gitignore"); + const entry = PROJECT_ENV_KEY_FILENAME; + + for (const present of [`${entry} \n`, `${entry}\r\n`, `a\n${entry} \r\n`]) { + await writeFile(path, present); + expect(await ensureGitignoreEntry({ gitignorePath: path, entry })).toEqual({ + changed: false, + }); + expect(await readFile(path, "utf8")).toBe(present); + } + + for (const absent of [` ${entry}\n`, `\t${entry}\n`, `${entry}\t\n`]) { + await writeFile(path, absent); + expect(await ensureGitignoreEntry({ gitignorePath: path, entry })).toEqual({ + changed: true, + }); + expect(await readFile(path, "utf8")).toBe(`${absent}\n${entry}\n`); + } +}); + test("outside git an existing key is used without writing a root .gitignore", async () => { const repo = await createRepo({ git: false }); const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE); diff --git a/tests/project-target-inspect.test.ts b/tests/project-target-inspect.test.ts index 953bde0bb..7de241f73 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -258,6 +258,12 @@ beforeAll(async () => { }); // Never adopted: .hack/.internal exists only so its mode can be dropped. await mkdir(join(root, "unsearchable", ".hack", ".internal")); + // Never adopted and without .hack/.internal: the most common real layout. + await writeProject({ + repo: join(root, "unsearchable-no-internal"), + name: "unsearchable-no-internal", + services: ["api"], + }); // Native and legacy inputs side by side (a conflicting project family). const nativeConflict = join(root, "native-conflict"); await writeProject({ @@ -709,21 +715,29 @@ describe("paired CLI/MCP target fixtures", () => { test.skipIf(process.getuid?.() === 0)( "an unsearchable .hack or .hack/.internal is a closed refusal without the path", async () => { - const path = join(fixture.root, "unsearchable"); // HACK-1415: every case the proof matrix names runs on both surfaces, - // on a project that never used adoption. - for (const [dir, mode] of [ - [".hack", 0o000], - [".hack", 0o644], - [".hack/.internal", 0o000], - [".hack/.internal", 0o644], + // on a project that never used adoption. HACK-1426: the first case is + // a .hack at mode 000 with no .hack/.internal at all. + for (const [project, dir, mode] of [ + ["unsearchable-no-internal", ".hack", 0o000], + ["unsearchable", ".hack", 0o000], + ["unsearchable", ".hack", 0o644], + ["unsearchable", ".hack/.internal", 0o000], + ["unsearchable", ".hack/.internal", 0o644], ] as const) { + const path = join(fixture.root, project); const locked = join(path, dir); await chmod(locked, mode); try { // An unsearchable project directory is a permission fault (#402's // `permission` selection kind), not unsafe adoption state. - expect({ dir, mode, result: await inspectBoth({ path }) }).toEqual({ + expect({ + project, + dir, + mode, + result: await inspectBoth({ path }), + }).toEqual({ + project, dir, mode, result: targetRefusalEnvelope({ From 8ff17289f10a98b327e67cb6e92f06c28ec6a7dc Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 02:16:27 -0400 Subject: [PATCH 5/6] fix(env): retry a private env read that races an atomic replace instead of refusing it as a symlink An unlocked read of a `.hack/.env*` file (the preflight in `upsertDotEnvValue`/`removeDotEnvKey`, and every resolver) compared the inode seen by `lstat` with the inode it then opened. A concurrent writer's atomic rename landing between the two made the reader exit 1 with the "symlink or not a regular file" refusal, although the path only ever held regular files. `readReplaceablePrivateFile` now starts the read again when both entries are regular files (at most `REPLACEABLE_READ_ATTEMPTS`), and still refuses a symlink or non-regular entry at once and a path replaced on every attempt. This was the cause of the intermittent `project-env-layer-lock` legacy dotenv failure; #435's layer lock was already in place, so no update was lost. Also fix two test-only flakes from HACK-1422: - native-runtime-client: wait for the keeper's completion text, not for the file's existence; the keeper now opens the empty-file window every run. - xattr-owner: give the in-process durable store cases a named 30 s budget instead of Bun's 5 s default; they wait on no timer. Refs HACK-1422 Co-Authored-By: Claude Opus 5.5 --- src/lib/private-file.ts | 45 +- ...ompose-storage-witness-xattr-owner.test.ts | 407 ++++++++++-------- tests/native-runtime-client.test.ts | 33 +- tests/private-env-file.test.ts | 130 +++++- 4 files changed, 415 insertions(+), 200 deletions(-) diff --git a/src/lib/private-file.ts b/src/lib/private-file.ts index 48e7dac08..1533f43cb 100644 --- a/src/lib/private-file.ts +++ b/src/lib/private-file.ts @@ -272,13 +272,29 @@ async function observeReplaceableTarget(opts: { return observed; } +/** + * How many times {@link readReplaceablePrivateFile} re-observes a path whose + * regular file was atomically replaced between its `lstat` and its `open` + * before it refuses closed. One concurrent writer's rename costs one retry; + * only a path replaced on every attempt exhausts the budget. + */ +export const REPLACEABLE_READ_ATTEMPTS = 8; + /** * Reads a private file that is later replaced with * {@link replacePrivateFileAtomic}. Returns null when the entry is absent. A * symlink at the path, a non-regular entry, a symlinked or replaced directory - * from `directories`, or a file that changes identity during the read throws + * from `directories`, or a file that keeps changing identity throws * `refusal()`; other filesystem errors (for example EACCES) propagate, so an * unreadable file is never mistaken for an empty one and overwritten. + * + * Concurrency: readers need no lock. When the regular file seen by `lstat` + * and the regular file opened (no-follow) differ, a writer renamed a complete + * sibling over the path in between, which is the replacement this module's + * writer promises readers may observe; the read starts again (at most + * {@link REPLACEABLE_READ_ATTEMPTS} times) and returns the previous or the new + * contents, never a false symlink refusal (HACK-1422). A symlink or + * non-regular entry opened at any attempt is still refused at once. */ export async function readReplaceablePrivateFile( opts: ReplaceablePrivateFileOptions & { @@ -287,9 +303,28 @@ export async function readReplaceablePrivateFile( } ): Promise { const directories = await captureRealDirectories(opts); + for (let attempt = 1; ; attempt += 1) { + const read = await readReplaceableTargetOnce(opts); + if (read === "replaced") { + if (attempt < REPLACEABLE_READ_ATTEMPTS) { + continue; + } + throw opts.refusal(); + } + await recheckRealDirectories({ directories, refusal: opts.refusal }); + return read; + } +} + +/** + * One no-follow observe-open-read of `opts.path`: null when absent, + * `"replaced"` when the opened regular file is not the one `lstat` observed. + */ +async function readReplaceableTargetOnce( + opts: ReplaceablePrivateFileOptions & { readonly maxBytes?: number } +): Promise { const observed = await observeReplaceableTarget(opts); if (!observed) { - await recheckRealDirectories({ directories, refusal: opts.refusal }); return null; } let file: FileHandle; @@ -308,12 +343,13 @@ export async function readReplaceablePrivateFile( const opened = await file.stat(); if ( !opened.isFile() || - opened.dev !== observed.dev || - opened.ino !== observed.ino || (opts.maxBytes !== undefined && opened.size > opts.maxBytes) ) { throw opts.refusal(); } + if (opened.dev !== observed.dev || opened.ino !== observed.ino) { + return "replaced"; + } const text = await file.readFile({ encoding: "utf8" }); if ( opts.maxBytes !== undefined && @@ -322,7 +358,6 @@ export async function readReplaceablePrivateFile( // The file grew in place after the size check. throw opts.refusal(); } - await recheckRealDirectories({ directories, refusal: opts.refusal }); return { text, mode: opened.mode & 0o7777 }; } finally { await file.close(); diff --git a/tests/native-compose-storage-witness-xattr-owner.test.ts b/tests/native-compose-storage-witness-xattr-owner.test.ts index 2073dd0ea..19f431400 100644 --- a/tests/native-compose-storage-witness-xattr-owner.test.ts +++ b/tests/native-compose-storage-witness-xattr-owner.test.ts @@ -78,6 +78,14 @@ const artifact: NativeComposeStorageXattrArtifact = { helperHash: "e".repeat(64), kernelAbi: 1, }; +/** + * Bun timeout for store cases that drive a full durable generation lifecycle + * (publish, enroll, verify, retire) in process. They wait on no timer: the + * fake carrier answers at once and its deadline is 60 s away. Their cost is + * durable store file work (mostly system time), which took 6-8 s per case on + * a loaded host and timed out under Bun's 5 s default (HACK-1422). + */ +const STORE_CASE_BUDGET_MS = 30_000; const roots: string[] = []; const stores: NativeComposeGenerationStore[] = []; let commandReaderCases = 0, @@ -664,50 +672,55 @@ test("completion publication refusal retains its first boundary after complete h test.each([ ["unset", undefined, 90_000], ["configured", "240000", 240_000], -] as const)("command storage carriers use the %s named storage budget, never a short start budget", async (_label, configured, expected) => { - const { store, transport } = await rememberedActive(); - const command = commandPorts(transport); - const deadlines: { readonly deadline: number; readonly from: number }[] = []; - const carrier = command.ports.carrier; - const ports = { - ...command.ports, - carrier: async (opts: { - readonly signal: AbortSignal; - readonly deadline: number; - }) => { - deadlines.push({ deadline: opts.deadline, from: Date.now() }); - return await carrier(opts); - }, - }; - const oldStartup = process.env.HACK_COMPOSE_STARTUP_TIMEOUT_MS; - const oldStorage = process.env.HACK_NATIVE_STORAGE_TIMEOUT_MS; - restoreEnv("HACK_COMPOSE_STARTUP_TIMEOUT_MS", "15000"); - restoreEnv("HACK_NATIVE_STORAGE_TIMEOUT_MS", configured); - try { - await store.withMutation(async (mutation) => { - const before = Date.now(); - const storage = await prepareNativeComposeCommandStorage({ - store, - mutation, - operation: "up", - selected: [selection], - signal: transport.controller.signal, - ports, +] as const)( + "command storage carriers use the %s named storage budget, never a short start budget", + async (_label, configured, expected) => { + const { store, transport } = await rememberedActive(); + const command = commandPorts(transport); + const deadlines: { readonly deadline: number; readonly from: number }[] = + []; + const carrier = command.ports.carrier; + const ports = { + ...command.ports, + carrier: async (opts: { + readonly signal: AbortSignal; + readonly deadline: number; + }) => { + deadlines.push({ deadline: opts.deadline, from: Date.now() }); + return await carrier(opts); + }, + }; + const oldStartup = process.env.HACK_COMPOSE_STARTUP_TIMEOUT_MS; + const oldStorage = process.env.HACK_NATIVE_STORAGE_TIMEOUT_MS; + restoreEnv("HACK_COMPOSE_STARTUP_TIMEOUT_MS", "15000"); + restoreEnv("HACK_NATIVE_STORAGE_TIMEOUT_MS", configured); + try { + await store.withMutation(async (mutation) => { + const before = Date.now(); + const storage = await prepareNativeComposeCommandStorage({ + store, + mutation, + operation: "up", + selected: [selection], + signal: transport.controller.signal, + ports, + }); + expect(storage).not.toBeNull(); + expect(deadlines.length).toBeGreaterThanOrEqual(2); + for (const { deadline, from } of deadlines) { + // Each carrier deadline is taken when it is created, from the storage + // budget alone: never the 15 s start budget the command was given. + expect(deadline).toBeGreaterThanOrEqual(before + expected); + expect(deadline).toBeLessThanOrEqual(from + expected); + } }); - expect(storage).not.toBeNull(); - expect(deadlines.length).toBeGreaterThanOrEqual(2); - for (const { deadline, from } of deadlines) { - // Each carrier deadline is taken when it is created, from the storage - // budget alone: never the 15 s start budget the command was given. - expect(deadline).toBeGreaterThanOrEqual(before + expected); - expect(deadline).toBeLessThanOrEqual(from + expected); - } - }); - } finally { - restoreEnv("HACK_COMPOSE_STARTUP_TIMEOUT_MS", oldStartup); - restoreEnv("HACK_NATIVE_STORAGE_TIMEOUT_MS", oldStorage); - } -}); + } finally { + restoreEnv("HACK_COMPOSE_STARTUP_TIMEOUT_MS", oldStartup); + restoreEnv("HACK_NATIVE_STORAGE_TIMEOUT_MS", oldStorage); + } + }, + STORE_CASE_BUDGET_MS +); test.each([ "up", @@ -2143,76 +2156,84 @@ test.each( ); }); -test("post-workload marker loss refuses completion, retaining exact pending generation", async () => { - const { store, transport, generation } = await active(); - transport.calls.length = 0; - await store.withMutation(async (mutation) => { - await expect( - mutation.runEffect({ - generation, - operation: "up", - assertOwned: async () => {}, - assertFresh: async () => {}, - storageWitnesses: { - kind: "directory-xattr", - engineId, - carrier: transport.carrier, - }, - effect: async () => { - transport.calls.push("workload"); - transport.attributes.clear(); - return { value: 0, outcome: "complete" }; - }, - }) - ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); - }); - expect(transport.calls).toContain("workload"); - expect((await store.loadCurrent()).pending?.generationId).toBe( - generation.generationId - ); - expect((await store.loadCurrent()).storageWitnesses?.[0]?.state).toBe( - "enrolled" - ); - expect(transport.calls).not.toContain("seed"); -}); +test( + "post-workload marker loss refuses completion, retaining exact pending generation", + async () => { + const { store, transport, generation } = await active(); + transport.calls.length = 0; + await store.withMutation(async (mutation) => { + await expect( + mutation.runEffect({ + generation, + operation: "up", + assertOwned: async () => {}, + assertFresh: async () => {}, + storageWitnesses: { + kind: "directory-xattr", + engineId, + carrier: transport.carrier, + }, + effect: async () => { + transport.calls.push("workload"); + transport.attributes.clear(); + return { value: 0, outcome: "complete" }; + }, + }) + ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); + }); + expect(transport.calls).toContain("workload"); + expect((await store.loadCurrent()).pending?.generationId).toBe( + generation.generationId + ); + expect((await store.loadCurrent()).storageWitnesses?.[0]?.state).toBe( + "enrolled" + ); + expect(transport.calls).not.toContain("seed"); + }, + STORE_CASE_BUDGET_MS +); -test("same-birth root replacement while awaiting the final reader cannot publish ready from its pinned old directory", async () => { - const { store, transport, generation } = await active(); - let exchanged = false; - await store.withMutation(async (mutation) => { - await expect( - mutation.runEffect({ - generation, - operation: "up", - assertOwned: async () => {}, - assertFresh: async () => {}, - storageWitnesses: { - kind: "directory-xattr", - engineId, - carrier: transport.carrier, - }, - beforeComplete: async () => { - transport.state.afterInvoke = (input) => { - if (!exchanged && input.request.operation === "verify") { - exchanged = true; - transport.state.root = { ...transport.state.root, inode: "43" }; - transport.attributes.clear(); - } - }; - }, - effect: async () => ({ value: 0, outcome: "complete" }), - }) - ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); - }); - expect(exchanged).toBe(true); - expect(transport.state.volume).toEqual(volume); - expect((await store.loadCurrent()).pending?.generationId).toBe( - generation.generationId - ); - expect((await store.loadCurrent()).storageWitnesses?.[0]?.state).toBe( - "enrolled" - ); -}); +test( + "same-birth root replacement while awaiting the final reader cannot publish ready from its pinned old directory", + async () => { + const { store, transport, generation } = await active(); + let exchanged = false; + await store.withMutation(async (mutation) => { + await expect( + mutation.runEffect({ + generation, + operation: "up", + assertOwned: async () => {}, + assertFresh: async () => {}, + storageWitnesses: { + kind: "directory-xattr", + engineId, + carrier: transport.carrier, + }, + beforeComplete: async () => { + transport.state.afterInvoke = (input) => { + if (!exchanged && input.request.operation === "verify") { + exchanged = true; + transport.state.root = { ...transport.state.root, inode: "43" }; + transport.attributes.clear(); + } + }; + }, + effect: async () => ({ value: 0, outcome: "complete" }), + }) + ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); + }); + expect(exchanged).toBe(true); + expect(transport.state.volume).toEqual(volume); + expect((await store.loadCurrent()).pending?.generationId).toBe( + generation.generationId + ); + expect((await store.loadCurrent()).storageWitnesses?.[0]?.state).toBe( + "enrolled" + ); + }, + STORE_CASE_BUDGET_MS +); test("known finite refused marker read clears only its proven absent helper intent and never repairs the witness", async () => { const { store, transport, generation, reference } = await active(); @@ -2286,96 +2307,100 @@ test.each([ "changed-created", "created-uncertain", "cleanup", -] as const)("unknown enrolled helper %s blocks replay and saved retirement without changing the main material receipt", async (failure) => { - const { store, transport, generation, reference } = await active(); - transport.state.failure = failure; - transport.calls.length = 0; - let workload = 0; - await store.withMutation(async (mutation) => { - await expect( - mutation.runEffect({ - generation, - operation: "run", - assertFresh: async () => {}, - assertOwned: async () => {}, - storageWitnesses: { - kind: "directory-xattr", - engineId, - carrier: transport.carrier, - }, - effect: async () => { - workload++; - return { value: 0, outcome: "complete" }; - }, - }) - ).rejects.toThrow("values omitted"); - }); - expect(workload).toBe(0); - const unknown = await store.loadCurrent(); - expect(unknown.generation?.generationId).toBe(generation.generationId); - expect(unknown.pending).toBeNull(); - expect(unknown.storageWitnessesPending).toBe(true); - expect(unknown.storageWitnesses?.[0]).toMatchObject({ - state: "enrolled", - reference, - }); - const observed = [...transport.calls]; - await store.withMutation(async (mutation) => { - if (failure === "no-created") { - const foreignGeneration = await publish(mutation); - await expect( - mutation.runEffect({ - generation: foreignGeneration, - operation: "down", - recoverPending: true, - assertOwned: async () => {}, - effect: async () => { - workload++; - return { value: 0, outcome: "complete" }; - }, - }) - ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_STATE" }); - } - for (const operation of ["up", "down"] as const) { +] as const)( + "unknown enrolled helper %s blocks replay and saved retirement without changing the main material receipt", + async (failure) => { + const { store, transport, generation, reference } = await active(); + transport.state.failure = failure; + transport.calls.length = 0; + let workload = 0; + await store.withMutation(async (mutation) => { await expect( mutation.runEffect({ generation, - operation, + operation: "run", assertFresh: async () => {}, assertOwned: async () => {}, + storageWitnesses: { + kind: "directory-xattr", + engineId, + carrier: transport.carrier, + }, effect: async () => { workload++; return { value: 0, outcome: "complete" }; }, }) - ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); - } - const stopped = await mutation.runEffect({ - generation, - operation: "down", - recoverPending: true, - assertFresh: async () => {}, - assertOwned: async () => {}, - effect: async () => { - workload++; - return { value: 0, outcome: "complete" }; - }, - beforeComplete: async () => { - throw new Error("Unknown helper cannot retire claims"); - }, + ).rejects.toThrow("values omitted"); }); - expect(stopped).toEqual({ value: 0, outcome: "uncertain" }); - }); - expect(workload).toBe(1); - expect(transport.calls).toEqual(observed); - const retained = await store.loadCurrent(); - expect(retained.pending?.generationId).toBe(generation.generationId); - expect(retained.storageWitnessesPending).toBe(true); - expect(retained.storageWitnesses?.[0]).toMatchObject({ - state: "enrolled", - reference, - }); -}); + expect(workload).toBe(0); + const unknown = await store.loadCurrent(); + expect(unknown.generation?.generationId).toBe(generation.generationId); + expect(unknown.pending).toBeNull(); + expect(unknown.storageWitnessesPending).toBe(true); + expect(unknown.storageWitnesses?.[0]).toMatchObject({ + state: "enrolled", + reference, + }); + const observed = [...transport.calls]; + await store.withMutation(async (mutation) => { + if (failure === "no-created") { + const foreignGeneration = await publish(mutation); + await expect( + mutation.runEffect({ + generation: foreignGeneration, + operation: "down", + recoverPending: true, + assertOwned: async () => {}, + effect: async () => { + workload++; + return { value: 0, outcome: "complete" }; + }, + }) + ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_STATE" }); + } + for (const operation of ["up", "down"] as const) { + await expect( + mutation.runEffect({ + generation, + operation, + assertFresh: async () => {}, + assertOwned: async () => {}, + effect: async () => { + workload++; + return { value: 0, outcome: "complete" }; + }, + }) + ).rejects.toMatchObject({ code: "E_NATIVE_COMPOSE_UNCERTAIN" }); + } + const stopped = await mutation.runEffect({ + generation, + operation: "down", + recoverPending: true, + assertFresh: async () => {}, + assertOwned: async () => {}, + effect: async () => { + workload++; + return { value: 0, outcome: "complete" }; + }, + beforeComplete: async () => { + throw new Error("Unknown helper cannot retire claims"); + }, + }); + expect(stopped).toEqual({ value: 0, outcome: "uncertain" }); + }); + expect(workload).toBe(1); + expect(transport.calls).toEqual(observed); + const retained = await store.loadCurrent(); + expect(retained.pending?.generationId).toBe(generation.generationId); + expect(retained.storageWitnessesPending).toBe(true); + expect(retained.storageWitnesses?.[0]).toMatchObject({ + state: "enrolled", + reference, + }); + }, + STORE_CASE_BUDGET_MS +); test("retirement rereads the original receipt after its final carrier-journal await", async () => { const { store, generation } = await active(); diff --git a/tests/native-runtime-client.test.ts b/tests/native-runtime-client.test.ts index 2710cb20d..fcd155150 100644 --- a/tests/native-runtime-client.test.ts +++ b/tests/native-runtime-client.test.ts @@ -88,6 +88,31 @@ async function waitForFile( } } +/** + * Wait until a fixture's completion file holds exactly `expected`. A + * fixture's writeFileSync creates the file before it writes the bytes, so an + * existence wait can return while the file is still "" (HACK-1422). + */ +async function waitForFileText( + path: string, + expected: string, + budgetMs = HANDSHAKE_BUDGET_MS +): Promise { + const deadline = performance.now() + budgetMs; + for (;;) { + const file = Bun.file(path); + if ((await file.exists()) && (await file.text()) === expected) { + return; + } + if (performance.now() >= deadline) { + throw new Error( + `Fixture completion did not write its text within ${budgetMs} ms.` + ); + } + await Bun.sleep(10); + } +} + /** * Wait until a fixture's pid handshake holds a complete pid. A fixture's * writeFileSync creates the file before it writes the digits, so a read that @@ -409,11 +434,15 @@ bunTest.each([ // The keeper holds the request's stdout/stderr until the test releases it, // so the request can only finish by bounding its drain. The bound is // measured from the stand-in's own exit (or the abort), never from spawn, - // so slow process startup under load cannot decide the outcome. + // so slow process startup under load cannot decide the outcome. Its + // completion write opens the empty-file window on every run, so the + // completion wait below must wait for the text, not for existence. const keeper = `import {existsSync,writeFileSync} from 'node:fs'; writeFileSync(${JSON.stringify(holding)},String(process.pid)); const cap=performance.now()+${KEEPER_RELEASE_CAP_MS}; while(!existsSync(${JSON.stringify(release)})&&performance.now() undefined); await Bun.write(release, "release"); - completed = await waitForFile(finished).then( + completed = await waitForFileText(finished, "finished").then( () => true, () => false ); diff --git a/tests/private-env-file.test.ts b/tests/private-env-file.test.ts index dabf01520..a1063d2bd 100644 --- a/tests/private-env-file.test.ts +++ b/tests/private-env-file.test.ts @@ -1,4 +1,6 @@ -import { afterEach, beforeEach, expect, test } from "bun:test"; +import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import * as privateFiles from "node:fs/promises"; import { chmod, link, @@ -22,7 +24,10 @@ import { } from "../src/constants.ts"; import { removeDotEnvKey, upsertDotEnvValue } from "../src/lib/hack-env.ts"; import { PRIVATE_ENV_FILE_REFUSAL } from "../src/lib/private-env-file.ts"; -import { replacePrivateFileAtomic } from "../src/lib/private-file.ts"; +import { + REPLACEABLE_READ_ATTEMPTS, + replacePrivateFileAtomic, +} from "../src/lib/private-file.ts"; import { materializeProjectEnv, setProjectEnvValue, @@ -399,3 +404,124 @@ test("negative control: replacePrivateFileAtomic itself refuses a symlinked targ await expect(lstat(danglingTarget)).rejects.toThrow(); expect(await stagingLeftovers(project.projectDir)).toEqual([]); }); + +/** + * HACK-1422: env files are read without the layer lock (the preflight of + * `upsertDotEnvValue`/`removeDotEnvKey`, and every resolver). A concurrent + * writer's atomic rename landing between the reader's `lstat` and its + * no-follow `open` used to fail the reader with the symlink refusal. `open` is + * intercepted so the replacement lands exactly in that window. + */ +function replaceOnOpen(opts: { + readonly envFile: string; + readonly replace: (call: number) => Promise; +}): { readonly calls: () => number; readonly restore: () => void } { + const originalOpen = privateFiles.open; + let calls = 0; + const intercept = spyOn(privateFiles, "open").mockImplementation( + async (...args) => { + if (args[0] === opts.envFile) { + calls += 1; + await opts.replace(calls); + } + return await Reflect.apply(originalOpen, privateFiles, args); + } + ); + return { calls: () => calls, restore: () => intercept.mockRestore() }; +} + +/** Renames a complete sibling over `path`, as `replacePrivateFileAtomic` does. */ +async function renameOver(path: string, text: string): Promise { + const sibling = `${path}.${randomUUID()}.racing`; + await writeFile(sibling, text, { mode: 0o600 }); + await privateFiles.rename(sibling, path); +} + +test("a set whose unlocked read races another writer's atomic replace keeps both keys (HACK-1422)", async () => { + const project = await createProject(); + await writeFile(project.envFile, "PRESET=1\n", { mode: 0o600 }); + const racing = replaceOnOpen({ + envFile: project.envFile, + replace: async (call) => { + if (call === 1) { + await renameOver(project.envFile, "OTHER=2\nPRESET=1\n"); + } + }, + }); + try { + const result = await upsertDotEnvValue({ + envFile: project.envFile, + key: SENTINEL_KEY, + value: SENTINEL_VALUE, + }); + expect(result.changed).toBe(true); + } finally { + racing.restore(); + } + expect(racing.calls()).toBeGreaterThan(1); + expect(await readFile(project.envFile, "utf8")).toBe( + `OTHER=2\nPRESET=1\n${SENTINEL_KEY}=${SENTINEL_VALUE}\n` + ); + expect((await stat(project.envFile)).mode & 0o777).toBe(0o600); +}); + +test("negative control: an env file replaced on every read attempt is refused closed after the bounded retries", async () => { + const project = await createProject(); + await writeFile(project.envFile, `${SENTINEL_KEY}=${SENTINEL_VALUE}\n`, { + mode: 0o600, + }); + const racing = replaceOnOpen({ + envFile: project.envFile, + replace: async (call) => { + await renameOver( + project.envFile, + `${SENTINEL_KEY}=${SENTINEL_VALUE}\nCHURN=${call}\n` + ); + }, + }); + let error: unknown; + try { + error = await caught(() => + removeDotEnvKey({ envFile: project.envFile, key: SENTINEL_KEY }) + ); + } finally { + racing.restore(); + } + expectClosedRefusal(error, [project.envFile]); + expect(racing.calls()).toBe(REPLACEABLE_READ_ATTEMPTS); + expect(await readFile(project.envFile, "utf8")).toBe( + `${SENTINEL_KEY}=${SENTINEL_VALUE}\nCHURN=${REPLACEABLE_READ_ATTEMPTS}\n` + ); + expect(await readdir(project.projectDir)).not.toContain(".internal"); +}); + +test("negative control: a symlink swapped in between the read's lstat and open is still refused at once", async () => { + const project = await createProject(); + await writeFile(project.envFile, "PRESET=1\n", { mode: 0o600 }); + const racing = replaceOnOpen({ + envFile: project.envFile, + replace: async (call) => { + if (call === 1) { + const swapped = `${project.envFile}.swapped`; + await symlink(project.outsideFile, swapped); + await privateFiles.rename(swapped, project.envFile); + } + }, + }); + let error: unknown; + try { + error = await caught(() => + upsertDotEnvValue({ + envFile: project.envFile, + key: SENTINEL_KEY, + value: SENTINEL_VALUE, + }) + ); + } finally { + racing.restore(); + } + expectClosedRefusal(error, [project.envFile, project.outsideFile]); + expect(racing.calls()).toBe(1); + expect(await readFile(project.outsideFile, "utf8")).toBe(OUTSIDE_TEXT); + expect((await lstat(project.envFile)).isSymbolicLink()).toBe(true); +}); From 34e6af28bdd7cdee0cab8e02ca4df178f43be5da Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 11:57:04 -0400 Subject: [PATCH 6/6] test(native): keep the xattr store case budget ahead of the fake carrier deadline The same-birth root replacement case still timed out at 30 s on a host at load average 45-95 on 18 CPUs. Its cost is a fixed amount of store file work (about 19k lstat, 13k realpath and 4.5k open calls in its second `up`, the same on every run), so its wall time follows host load. A larger budget alone was not enough: the fake carrier's fixed 60 s deadline then expired first and the case failed with the storage witness refusal instead of the asserted E_NATIVE_COMPOSE_UNCERTAIN. The fake carrier's lifetime is now twice the store case budget, so a slow host can only fail a case as a clean Bun timeout. The budget is 180 s, a hang guard rather than a speed assertion. Refs HACK-1422 Co-Authored-By: Claude Opus 5.5 --- ...ompose-storage-witness-xattr-owner.test.ts | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/native-compose-storage-witness-xattr-owner.test.ts b/tests/native-compose-storage-witness-xattr-owner.test.ts index 19f431400..838413200 100644 --- a/tests/native-compose-storage-witness-xattr-owner.test.ts +++ b/tests/native-compose-storage-witness-xattr-owner.test.ts @@ -81,11 +81,22 @@ const artifact: NativeComposeStorageXattrArtifact = { /** * Bun timeout for store cases that drive a full durable generation lifecycle * (publish, enroll, verify, retire) in process. They wait on no timer: the - * fake carrier answers at once and its deadline is 60 s away. Their cost is - * durable store file work (mostly system time), which took 6-8 s per case on - * a loaded host and timed out under Bun's 5 s default (HACK-1422). + * fake carrier answers at once. Their cost is a fixed amount of durable store + * file work, mostly system time: the second `up` of the same-birth case alone + * makes about 19k lstat, 13k realpath and 4.5k open calls, the same count on + * every run. That takes about 5-7 s on hosted CI, but 5-60 s or more on a + * host whose load is several times its CPU count, so this budget is a hang + * guard, not a speed assertion (HACK-1422). */ -const STORE_CASE_BUDGET_MS = 30_000; +const STORE_CASE_BUDGET_MS = 180_000; +/** + * Wall-clock lifetime of the fake carrier. Every carrier is captured inside a + * case, so its deadline is always later than that case's Bun budget: a slow + * host fails the case as a clean timeout, never by exhausting the carrier + * deadline, which refuses with the storage witness error and would change the + * asserted outcome (HACK-1422). + */ +const FAKE_CARRIER_LIFETIME_MS = 2 * STORE_CASE_BUDGET_MS; const roots: string[] = []; const stores: NativeComposeGenerationStore[] = []; let commandReaderCases = 0, @@ -306,7 +317,7 @@ function fake(store: NativeComposeGenerationStore) { artifact, ports, signal: controller.signal, - deadline: Date.now() + 60_000, + deadline: Date.now() + FAKE_CARRIER_LIFETIME_MS, }); return { state, ports, carrier, calls, attributes, controller }; }