Repository navigation
fix(agents): name the permission fix and keep a value-free cause on input access refusals - #452
Merged
Merged
Conversation
…nput access refusals Target inspector follow-ups after #379 (HACK-1390). - Classify input presence faults through an exported seam, projectInputAccessFault(), so EPERM is covered by a test (a mode-000 directory only produces EACCES). - ProjectInputAccessError keeps a value-free class/code diagnostic (for example Error:ENOTDIR) as a non-enumerable property, read with projectInputAccessDiagnostic(). Envelopes, printed errors and JSON stay unchanged and carry no path, OS message or cause. - input_permission_denied now names the permission fix first. The contract doc no longer says an unsearchable .hack or .hack/.internal is adoption_state_unsafe; it is input_permission_denied even on a project that never used adoption. Refs HACK-1390 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
marked this pull request as ready for review
October 24, 2026 02:43
This was referenced Oct 24, 2026
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
Follow-ups to #433 (HACK-1375) and #435 (HACK-1376), both already on `next`. ## Root cause 1. **The unset compare-and-swap was not tested.** `unsetProjectEnvValue` passes `expectedExisting: text` to the layer write. If you delete that line, every lock, hydrate and config test still passes, because only the set path had a paused-writer conflict test. 2. **Some layer locks can never be reclaimed, and the message did not say so.** `withEnvLayerLock` reuses the projects-registry owner protocol. That protocol never removes a malformed receipt, a receipt it does not trust (another uid, or writable by group or others), a receipt whose PID now belongs to a live process, or a dead owner behind a leftover `<lock>.recovery` guard. In each of these cases every `env set/add/unset/hydrate` on the layer waited 10 s and then printed `Another hack env command is writing <file>; … Retry when it finishes.` No retry could ever succeed, and the message did not name the lock file. 3. **A key can become unignored after a later `git init`.** In a non-git directory, the first `--secret` creates the key and writes its root `.gitignore` entry. Outside git, an existing key deliberately triggers no `.gitignore` write. So if `.gitignore` is deleted and the directory then gets `git init`, the key is not ignored, and Hack is not running at that moment. 4. **The re-include refusal named the wrong cause.** The ignore probe that runs after the entry is in place produced the same "Hack could not add it to <.gitignore>" message as a failed write. That is wrong when `.gitignore` already contains `.hack.secret.key` followed by `!.hack.secret.key`. ## Fix - `src/lib/env-layer-lock.ts`: `EnvLayerLockTimeoutError` now has a `reason` (`held` | `recovery_guard`) and the `lockPath`. The message is still one value-free line with no stack: - `held`: `Another hack env command is writing <file>; gave up after waiting 10s for its lock at <lock>. Nothing was written. Retry when it finishes. If no hack env command is running, Hack cannot reclaim that lock itself: remove <lock> and retry.` - `recovery_guard`: when the registry reports a dead owner behind a busy guard, the message keeps the registry's `recovery guard is busy (interrupted recovery requires offline inspection)` detail and names both `<lock>.recovery` and `<lock>` as the files to remove. - The lock helper still never removes anything itself. `src/lib/projects-registry-lock.ts` is unchanged; the classification reads the registry's existing timeout detail. That file belongs to G73 / HACK-1377. - `src/lib/project-env-config.ts`: `projectEnvKeyUnignoredError` now takes a `cause`. `unwritable` keeps the existing message. `reincluded` says the root `.gitignore` lists the key but a later rule in that file re-includes it, and asks the user to remove that rule. Once the entry is present, no deeper ignore file can apply to a key at the project root, so a later rule in that same file is the only possible cause. - `src/commands/doctor.ts`: exports `checkProjectEnvKey` so the real doctor check can be tested end to end. It also moves a misplaced docblock onto that function. The check's behavior is unchanged. - `docs/env.md`: documents unreclaimable locks and the recovery step, the re-include cause, and the `git init` decision. It also adds an `env-key-unignored` example to the doctor table. ### Decision on a later `git init` (item 3) Watching for `git init` is out of scope for env writes, because Hack runs no code when it happens. Outside git, an existing key still needs no entry, and Hack does not recreate a root `.gitignore` the user deleted. That behavior is unchanged and covered by the existing test. The guarantee comes back at the next Hack touchpoint: - `hack doctor`'s env key check reports `env-key-unignored` (warn) with a remedy. This check already existed; the new test proves it catches this exact sequence. - The first `--secret` set or hydrate in the new repository adds the entry before it uses the key, or it refuses (#433 behavior). ## Tests (each proven by mutation) | Test | Mutation that makes it fail | | --- | --- | | `project-env-layer-lock`: *an unset refuses with a conflict when a writer that takes no lock changed the layer after its read*. A `spyOn` pauses `resolvePrimaryLocalProjectDir`, the one awaited call between the local unset's read and its write. The test edits the layer, resumes, and expects `AtomicWriteConflictError` with the edit kept. | delete `expectedExisting: text,` from the unset: 1 fail | | `project-env- layer-lock`: *a layer lock Hack cannot reclaim…* uses three receipts: malformed, group-writable with an exited PID, and PID 1 standing in for a reused PID. Set and unset each refuse with the exact message, the receipt is left untouched, and removing the named lock lets the retry succeed. The existing held-lock test asserts the new message too. | restore the old message: 2 fail | | `project-env-layer-lock`: *a dead owner behind an interrupted recovery guard…* covers a set (300 ms wait) and a hydrate (default 10 s): both give the `recovery_guard` message with both paths, write nothing, and leave the lock and guard in place. After both are removed, the set succeeds. | classify the guard case as `held`: 1 fail | | `project-env-key-ignore`: the re- include test now expects the `reincluded` message, does not contain "could not add it", and checks that `.gitignore` was not rewritten. | send `unwritable` for the post-entry probe: 1 fail | | `project-env-key-ignore`: *a key whose entry was removed before a later git init…* runs the exact reported sequence through the source CLI, then the real `checkProjectEnvKey`: status `warn`, `env-key-unignored`, and no key bytes or absolute root in the output. The next `--secret` set repairs the entry and doctor returns `ok`. | drop `inspectLocalKeyIgnored` from doctor: 1 fail | Every fixture uses a private HOME, HACK_HOME, global config and Docker config, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`, and git without global or system config. The env layer is written before any `--secret`. The in-process doctor call swaps in the same private environment and restores it afterwards. ## Evidence - Manual repro before the fix: (2) dead owner plus guard, and a malformed receipt, gave the old "Retry when it finishes." message. (3) after `git init`, `git check-ignore` exits 1 and `git status` lists `.hack.secret.key`. (4) gave the "could not add it" message. After the fix, the same script prints the messages above, and removing the named lock makes the retry exit 0. - Focused: `tests/project-env-layer-lock.test.ts` 12/12 and `tests/project-env-key- ignore.test.ts` 8/8 pass locally. - `bun run check` (includes `privacy:check`) exit 0. `bun run --cwd packages/cli typecheck` exit 0. gitleaks 8.30.1 (`gitleaks dir` on the `git log -p origin/next..HEAD` export): no leaks. - The CLI surface and consumer agent guidance did not change, so no reference or plugin regeneration was needed. - Full suite: hosted CI `test` at this PR head (see checks). ## Gaps - In a wider local run of 11 related env/doctor files, 13 tests in `project-env-write-refusals`, `project-env-stable-ciphertext` and `project-env-worktree-key` failed with `beforeEach/afterEach hook timed out` while the host load average was 51–60 on 18 cores. None of them asserts a message this PR changes. Hosted CI is the evidence for them. - The refusal is still a `ClosedCliError` with no `E_*` code (as before). Adding a lock-busy code would change the error-code registry and its generated docs, so it is left for a separate decision. - Classifying `recovery_guard` depends on the registry's existing timeout text. If G73 rewords that text, the guard test fails in the merge train rather than silently falling back to `held`. - The `held` message cannot tell a live writer from an unreclaimable lock without parsing the receipt (code G73 is editing), so it covers both cases: retry, or remove the lock when no hack env command is running. - The unset CAS test uses `spyOn` on an ESM namespace export (`worktree-local-config.ts`), in process and restored in `finally`. ## Round 2: merge with current next While this PR was being verified, `next` moved from 7d9c89f to 5208ee5 (#404, #446, #430, #439, #421, #448, #452, #405), and the PR became `CONFLICTING`. - Merge commit 7ac7cbb merges `origin/next` (5208ee5) into the branch. The only conflict was the `checkProjectEnvKey` docblock in `src/commands/doctor.ts`, which #448 also moved. It is resolved to this PR's side: the extended docblock plus `export async function checkProjectEnvKey`. Compared with next, `doctor.ts` now differs only in those lines. - No other code changed in this round. The diff against next is still the same 6 files. - Gates on the merged head: `bun run check` (includes `privacy:check`) exit 0. `bun run --cwd packages/cli typecheck` exit 0. Focused tests `project-env-layer-lock`, `project-env-key- ignore` and `cli-spawn-env-isolation` pass 27/27 (`--timeout 120000`) under #405's new preloads. gitleaks 8.30.1 (`gitleaks dir` on the `git log -p origin/next..HEAD` export): no leaks. - Full suite: hosted CI run 40056707145 at 7ac7cbb (base 5208ee5): `test` passed in 38m45s, plus docker-e2e 1-4/4, Candidate core (ubuntu), Config compiler (ubuntu x64/arm, macos-15-intel), Runtime state models, linux-process-lifetime, runtime-images and secret-scan. The two `macos-latest` jobs were still queued for a runner after about an hour. The earlier green run 39978726682 was at ed877f5, before the base moved, so it does not count for this head. Refs HACK-1386 🤖 Generated with [Claude Code](https://claude.com/claude-code)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…efusals (#462) Follow-ups to #452 (HACK-1390) on the target inspector. These are items 1 to 4 of HACK-1415. Item 5 (the diagnostic is not shown even with `HACK_DEBUG=1`) is information only and is unchanged. ## Root cause 1. **The proof matrix claimed more than the tests did.** The row "Unsearchable `.hack` or `.hack/.internal` on a never-adopted project" in `docs/reference/ax-target-result-contract.md` says a paired fixture covers it. But the only paired CLI/MCP fixture was `.hack` at mode 000. The `.hack/.internal` case and the 0644 cases ran only in the domain test. 2. **Root skipped the message assertions.** The checks on `TARGET_REFUSALS.input_permission_denied.message` do not depend on the user. They sat inside `test.skipIf(getuid() === 0)`, so a root run skipped them. 3. **Nothing tied `inputPresent()` to the classifier.** EPERM was tested only by calling the exported `projectInputAccessFault` seam directly. If `inputPresent()` used its own inline classifier that dropped EPERM, no test would fail. 4. **Adoption marker refusals kept no diagnostic.** `LegacyComposeAdoptionSelectionError`'s `permission` and `input-access` kinds come from the adoption marker's state-root check. They cover an unsearchable or 0644 `.hack` or `.hack/.internal`, and a file where `.hack` belongs. These refusals kept nothing about the fault, while `ProjectInputAccessError` keeps a value-free class/code diagnostic. ## Fix 1. The paired CLI/MCP test "an unsearchable .hack or .hack/.internal is a closed refusal without the path" now runs all four cases on both surfaces: `.hack` and `.hack/.internal`, each at modes 000 and 0644. The `unsearchable` fixture gets a `.hack/.internal` directory and never uses adoption. The proof-matrix row now names exactly these cases. 2. The message assertions now live in their own test with no skip: "the permission-denied refusal names the permission fix, not adoption recovery". The mode-dependent domain loop is still skipped as root. 3. A new test, "input presence checks classify through the projectInputAccessFault seam", uses `spyOn` on `node:fs/promises` `lstat`. It faults the real presence check of one input (`.dev/docker- compose.yml`) with EPERM, EACCES and EIO, calls `inspectProjectInputsAtRoot`, and asserts the `access` kind and the `Error:<code>` diagnostic. Other paths use the real `lstat`, and `mock.restore()` runs in `afterEach`. This follows the existing `spyOn(fs, "open")` pattern in `tests/native-env-values.test.ts`. 4. `LegacyComposeAdoptionSelectionError(kind, diagnostic?)` now attaches the same non-enumerable, identifier-checked class/code diagnostic (for example `Error:ENOTDIR`) to the `permission` and `input-access` kinds. Read it with `legacyComposeAdoptionSelectionDiagnostic()`. The `adoption` kind keeps none. The marker captures the diagnostic where it classifies the state-root `lstat` fault. The helpers move from `project-input-selection.ts` into a new module, `src/lib/value-free-diagnostic.ts`, which both errors use. `ProjectInputAccessError` behaves the same as before. Output stays closed and value-free. Envelopes, printed CLI errors, public codes and messages are unchanged. There is no `cause`, no path and no OS message. The contract doc gains one sentence: the filesystem guard refusals keep the class/errno code only as an internal diagnostic, and it never appears in the envelope. ## Tests and mutation evidence I applied each mutation, ran the focused tests, and restored the file. Each mutation made the listed tests fail: - **Inline classifier in `inputPresent()` that keeps the diagnostic but drops EPERM:** 1 test fails, "input presence checks classify through the projectInputAccessFault seam". - **Marker does not pass the diagnostic:** 3 tests fail: "a file where .hack belongs …" (ENOTDIR), "EPERM on the adoption state root …", and "an unsearchable .hack or .hack/.internal keeps a value-free adoption marker diagnostic". - **Diagnostic made enumerable (shared helper):** 7 tests fail across both errors. They assert that `Object.keys`, `String()` and `JSON.stringify` carry neither the code nor the path. - **EPERM dropped from the marker's `classifyStateRootFailure`:** 1 test fails, "EPERM on the adoption state root …". - **Diagnostic attached to the `adoption` kind too:** 1 test fails, "only filesystem adoption marker refusals carry a diagnostic". - **`input_permission_denied` message reverted:** the unconditional message test fails. - **Marker `permission` kind mapped back to `adoption_state_unsafe` in `target-inspect.ts`:** both the paired CLI/MCP test and the domain test fail. ## Validation - `tests/project-input-selection.test.ts`, `tests/project-target-inspect.test.ts` and `tests/global-state-guard.test.ts`: 67 pass, 0 fail. I ran them locally as a non-root user, with a private HOME, HACK_HOME and DOCKER_CONFIG and `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`. - `bun run --cwd packages/cli typecheck` (covers `src` and `tests`): 0 errors. - Root `bun run check`: `privacy-check: ok`. `@hack/cli:check` shows 34 warnings, which were already there, and no errors. Biome shows no issues in the 5 changed TypeScript files. - `bun run docs:cli-reference` is not needed: the CLI surface did not change. No consumer agent guidance changed. - gitleaks 8.30.1 (`gitleaks dir` on the exported `git log -p origin/next..HEAD`): no leaks found. - Full suite: the hosted CI `test` job at this PR head (see checks). ## Gaps - No mutation tells the four paired cases apart. In production, `.hack` and `.hack/.internal` at 000 and 0644 all produce EACCES from the same state-root `lstat`. The loop asserts each case separately, labelled with its directory and mode, but a mutation could not break one case without the others. - The diagnostic is still not shown in CLI output, even with `HACK_DEBUG=1` (HACK-1415 item 5). It is available only to tests and to code that calls the reader functions. - I did not run the root-only paths as root locally. The unconditional message test is the part that root now also runs. Refs HACK-1415, HACK-1390. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…60 min limit (#442) ## Root cause The hosted `test` job ran every `bun run test` file (568 root `tests/` files plus `packages/db`) in one Turbo task on `blacksmith-6vcpu-macos-15`. That step alone took about 39 min (next run 39873410893: `Tests (Turbo)` 23:42:18 to 00:21:20). Slower runners reached the 60 min job limit and were cancelled. Turbo buffers task output until the end, so a cancelled run did not show which file was running. ## Fix - `scripts/test-shard-plan.ts` finds the same file set that `bun run test` runs: root `tests/` (with `bunfig.toml` `root = "./tests"` and its preload) and `packages/db` (from its own directory). It splits the files into shards, longest first. Durations come from `scripts/test- shard-durations.json`. That table was first built from the per-test times in the hosted log above. A file with no recorded duration is planned at 5 s, and each file adds 0.3 s for process start. The split is deterministic. - `scripts/test-shard.ts run --shard=i/4`: - runs `check:bun-version` and `test:preflight` first, as `bun run test` does; - runs each file with `bun test ./<file>` in its own process, from the file's suite directory, so the bunfig preload and the db path aliases apply; - prints `start` and `PASS`/`FAIL` lines with the file's duration and the shard's elapsed time, and a heartbeat every 2 min while a file runs; - stops a file after 12 min, and stops the shard at 30 min or on SIGINT/SIGTERM (which is what a GitHub cancellation or timeout sends). It then prints the running file, the files not started and the 10 slowest files; - fails when a file fails, times out, or Bun does not report exactly 1 file run; - writes per-file durations, uploaded as the `test-shard-durations-<i>` artifact. `update- durations` merges those artifacts back into the table. - Coverage check, run by every shard before it starts and by `verify --count=4`: the files found on disk must equal the files `git ls-files` lists. The union of all shards must equal that set, with no file in two shards. The set of workspaces with a `test` script, and the bunfig test root, must match the known suites. - `.github/workflows/ci.yml`: - adds the `test-shard` matrix (`test shard i/4`, `fail-fast: false`, `timeout-minutes: 40`). It keeps the existing runner, Bun 1.4.2, frozen install, Rust 1.97.1 and the compiler build on every shard. Typecheck, privacy, lint, the CLI build and the release smoke run on shard 1 only. - `test` is now a fan-in job (`needs: test-shard`, `if: always()`). It passes only when every shard succeeded, so the required check name `test` does not change. No admin change to the ruleset is needed. - `scripts/prerelease-plan.ts` `CI_JOBS` now also requires each `test shard i/4` job, the same way it requires the `docker-e2e` shards. ## Tests `tests/test-shard.test.ts` (37 tests) covers: - shard parsing and test-file name matching; - partition determinism, exact cover, balance, the fallback duration, and zero-second files being spread across shards; - the coverage check, with drop, duplicate and extra-file cases; - fixture git checkouts for drift, ignored files and a missing suite root; - the current checkout's 4-shard plan against `git ls-files`; - the workspace and bunfig checks; - the runner: per-file lines, file timeout, deadline, interrupt, the stop report, and the file- count rule; - real `bun test` child processes: exit status, suite cwd, stopping a hung file, and not waiting for a leftover process that holds stderr open; - the workflow wiring and the prerelease gate. Mutation proofs (each mutation was applied, the focused file was run, then the file was restored). Each one made at least one test fail: | Mutation | Failing tests | | --- | --- | | partition drops the last file | 7 | | no per-file start cost added | 2 | | coverage ignores missing files | 2 | | coverage ignores duplicate files | 1 | | no disk-vs-git drift check | 1 | | no workspace check | 1 | | any file count counts as a pass | 1 | | no file timeout | 2 | | stop report omits the running file | 1 | | `test` fan-in without `always()` | 1 | | prerelease gate drops one shard | 1 | | wait for stderr end of file | 1 | | no SIGTERM on abort | 1 | | wrong suite cwd | 1 | CLI mutation: when the partition drops a file, `bun scripts/test-shard.ts verify --count=4` exits 2 with `not run by any shard (1): tests/zig-aarch64-musl-linker.test.ts`. Restored, it exits 0 with all files in 4 shards (571 files when first written, 573 at `503dc18d`, 576 at `57736870`; planned about 10m25s to 10m32s per shard). ## Evidence - Bun 1.4.2 (`mise x bun@1.4.2`), Rust 1.97.1. - Local (loaded M5 Mac, private HOME/HACK_HOME/DOCKER_CONFIG, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`): - `bun run check` (includes `privacy:check`) passed; - `bun run --cwd packages/cli typecheck` passed; - actionlint 1.7.12 found nothing on the 4 workflows; - focused tests passed: `test-shard`, `prerelease-plan`, `e2e-shard`, `native-plan-cli- wiring`, `ci-bun-crash-census` and `agent-instruction-source` (135 tests); - gitleaks 8.30.1 over `origin/next..HEAD` found no leaks. - Full suite: hosted CI is the full-suite evidence for this unit, so no M3 run is needed. Each hosted run executes every test file across the 4 shards; the fan-in `test` check passes only when all 4 shards pass. - Hosted shard runs: see "Hosted runs" below (3 runs, 2 of them green). ## Hosted runs Times are the `Tests (shard)` step; the whole job adds about 1 to 1.5 min of setup. Every shard job has `timeout-minutes: 40`. | Run | Head | Shard 1/4 | Shard 2/4 | Shard 3/4 | Shard 4/4 | Result | | --- | --- | --- | --- | --- | --- | --- | | [39888691204](https://github.com/hack-dance/hack/actions/runs/39888691204) | `a33511f4` | 11m48s (failed, test bug) | 11m39s | 10m32s | 12m57s | red: one test, see below | | [39977193863](https://github.com/hack-dance/hack/actions/runs/39977193863) | `503dc18d` | 11m49s | 11m30s | 11m44s | 12m16s | green, all 20 checks | | [40057462690](https://github.com/hack-dance/hack/actions/runs/40057462690) | `57736870` | 10m12s | 10m39s | 9m49s | 11m29s | green, all 20 checks | Whole shard jobs took 10m34s to 13m54s across the 3 runs, well under the 40 min timeout. For comparison, `next`'s serial `test` job ([39916341281](https://github.com/hack- dance/hack/actions/runs/39916341281), `7d9c89f8`) took 40m17s, with `Tests (Turbo)` at 39m21s, and run 39783802503 was cancelled at 60m25s. Same file set as the serial job: the shard logs of run 39977193863 list 573 unique PASS files. `next`'s serial log (39916341281) lists 572 files; the extra one is the new `tests/test- shard.test.ts`. Unique skipped tests are the same 157 names on both. Passed tests are 8057 on `next` and 8094 on the PR; the 37 extra are the new `test-shard` tests. Running each file in its own process did not skip anything new. ### Why run 39888691204 was red Shard 1 failed one test in `tests/project-exec-command.test.ts`: "exec warns value-free on stderr only after the service env changed since start". The clack CoreDNS/Caddy warnings went to stdout instead of stderr. This was a test that depended on file order, not a sharding bug and not a known flake. In the old single `bun test` process, an earlier file had already set `HACK_LOGGER=console`. Run on its own, on a host without `gum`, the logger falls back to clack, which writes to stdout. Repro on `origin/next`, running only that file with `gum` removed from `PATH`: 4 pass, 1 fail, the same test. Fix (`503dc18d`, `test: pin the console logger in the exec env-change test`): the test file sets `HACK_LOGGER=console` in `beforeAll` and restores the previous value in `afterAll`. With the fix the file gives 5 pass on its own; with `next`'s version of the file put back it gives 4 pass, 1 fail again. Run 39977193863 at `503dc18d` is green. ## Round 2 (verifier follow-up) - The verifier found that this body still said PENDING for the hosted and full-suite evidence, quoted 571 files instead of the plan's count, and did not explain the red run. This body now lists the hosted runs, the red-run cause and fix, and drops the M3 item. - File counts: the plan had 571 files when the CLI mutation above was first written, 573 at `503dc18d`, and 576 after merging `next` at `5208ee54` (which adds #405, the global-state- guard preload, and #452). - New head `57736870`: merge of `next` `5208ee54` into `503dc18d`. The merge was clean; no PR file changed. - Local gates at `57736870` (Bun 1.4.2, private HOME/HACK_HOME/DOCKER_CONFIG, `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`, a bogus `DOCKER_HOST`): - gitleaks 8.30.1 `dir` over the `git log -p origin/next..HEAD` export: no leaks; - `bun run check` (includes `privacy:check`): 2 of 2 tasks passed; - `bun run --cwd packages/cli typecheck`: exit 0; - focused tests `test-shard`, `prerelease-plan` and `project-exec-command`: 99 pass, 0 fail; - `bun scripts/test-shard.ts verify --count=4`: 576 files in 4 shards, each file exactly once, planned about 10m31s per shard. 7 files from `next` have no recorded duration yet and use the 5 s fallback. - Hosted CI at `57736870`: run [40057462690](https://github.com/hack- dance/hack/actions/runs/40057462690): all 20 checks green, including `test shard 1/4` to `4/4` and the `test` fan-in. The shard logs list 576 unique PASS files and no FAIL line, which matches the local plan. Shard jobs took 10m34s to 12m11s. - Required check: still `test` (ruleset on `next`). The fan-in job keeps that name, so no admin change is needed. ## Round 3 (re-check) - Remote head is still `57736870`, and `next` is still `5208ee54`, which is an ancestor of the head. No merge or new commit was needed, so nothing was pushed this round. - Re-checked this body against GitHub: `gh pr checks 442` shows all 20 checks green at `57736870`. The `Tests (shard)` step times of run 40057462690 match the table (10m12s, 10m39s, 9m49s, 11m29s). Its shard logs list 576 unique PASS files and no FAIL line. - `bun scripts/test-shard.ts verify --count=4` at `57736870`: 576 files in 4 shards, each exactly once, planned 10m31s to 10m32s per shard. - The Tests section above now gives the file count at each head instead of only the first count (571). ## Gaps - Each file now runs in its own process instead of sharing one `bun test` process. Files no longer share module state or global hooks across files. That is stricter isolation, so a test that depended on another file running first in the same process would now fail. The red run 39888691204 was such a case: a test passed in the shared process only because an earlier file had set `HACK_LOGGER=console`. It is fixed in `503dc18d` (see "Why run 39888691204 was red"). The green hosted runs are the evidence for the current tree. - `scripts/ci-bun-crash-census.ts` matches one exact job name, and its default is `test`. For runs after this change the Bun logs are in the `test shard i/4` jobs, so the census needs `--job "test shard i/4"` once per shard. This is documented in the verify skill. The script itself is not changed here. - After a stop, a child that ignores SIGTERM is SIGKILLed after 10 s. Its own descendants are not process-group killed, because changing process groups could change the lifecycle tests. Refs HACK-1381 🤖 Generated with [Claude Code](https://claude.com/claude-code)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…using adopted Compose (HACK-1421) (#477) The docker e2e scenario `native-compose-adoption-job-worktrees` failed once on `next` (run 40055366376, job 120757063141, docker-e2e shard 4/4, commit d803ad6). At `phase=alpha-start-7`, `hack up` exited with `code=E_CONFIG_INVALID ordered-stage=unavailable` after about 2.8 s. Passing runs of the same up take 7 to 11 s. ## Root cause The task suggested that #452 had started mapping input access faults to `E_CONFIG_INVALID`. That is not the case. #452 only added a value-free diagnostic and changed one message. `ProjectInputAccessError` already returned `E_CONFIG_INVALID` before it. The scenario also has no concurrent writer, so I found no half-written `.hack` input to blame. The cause I found is a Docker read race in the retained-resource binding proof (`src/lib/native- compose-adoption-binding.ts`). I found it by reading the source. The CI log does not show it directly (see Gaps). - While an adopted `up` runs its ordered job plan, it keeps calling `assertFresh()`. Each call runs `readInputs()` and then `inspectResources()`. That function reads each owned bridge's `.Containers` keys with `docker network inspect`. - Docker lists an endpoint that is not joined to a sandbox as `ep-<endpoint id>`. Moby's `buildContainerAttachments` (daemon/network.go) does this, and it happens for a moment on every container start (endpoint created, sandbox not yet joined) and exit (sandbox left, endpoint not yet deleted). A container inspect waits for the container lock, but a network inspect does not, so it can see this key. The native Compose owner already handles this key (#272, #362). The adoption binding did not. - `networkRow` and `pluralNetworkRows` accepted only 64-hex container IDs, so the key failed at once with `E_LEGACY_COMPOSE_BINDING_IDENTITY`. The snapshot loop retries only when running state changes, so this refusal was never retried. Inside the job plan the error carries no ordered-refusal tag, so the store returns `E_LEGACY_ADOPTION_STATE` and the command reports `E_CONFIG_INVALID` with no `legacy_adoption_refusal` detail. That is exactly what the CI line shows. - At alpha-start-7 the DB is already healthy, so the seed job starts within the first seconds of the up, during the polling. This fits the early 2.8 s refusal. The window is only milliseconds long, so the failure is rare (once in the last 100 `ci.yml` runs). ## Fix - A new helper, `bridgeMembers()`, splits a bridge's keys into container IDs and a `transient` flag. A key that exactly matches `ep-<64 hex>` is never attributed, admitted or saved. Any other non-ID key, or a duplicate, still refuses identity at once. - If the accepted loop read has a placeholder, the existing snapshot loop throws that observation away and reads again. It keeps the same aggregate probe deadline and output allowance. A placeholder that is still there at the deadline refuses with the probe deadline error. The first bridge read is only an identity read and is always followed by a loop read, so a placeholder there costs no extra scan. - `docs/reference/native-compose-adoption.md` now describes this rule next to the existing note that "delayed endpoint removal can still refuse". - **Diagnostic:** the scenario's `cli-result` line now ends with `refusal=<token>`, for example `refusal=legacy-state`. The token comes from a closed list. It is set only when the envelope message exactly matches one of the fixed product messages: the `LegacyComposeAdoptedGenerationError` codes, the adopted-command refusal and cancel messages, the adoption marker kinds, and the shared input-access guard. Any other message gives `other`. The message text is never printed. `E_PERMISSION` is now also a known code. The task asked to surface #462's errno diagnostic in the scenario output. That is not possible: #462 is still open, and its diagnostic is internal and never reaches the CLI envelope. The new token does tell an input-access or marker fault apart from an adoption-store refusal. ## Tests and mutation evidence New tests in `tests/native-compose-adoption-binding.test.ts`. The fake `docker` adds an `ep-<hex>` key to a chosen network inspect read: - placeholder on the identity read (no extra scan) and on the accepted loop read (exactly one extra scan): the binding equals a clean acquisition. - placeholder on one of two owned bridges: both bridges are read again. - a placeholder on every read uses up the shared deadline and refuses with `E_LEGACY_COMPOSE_BINDING_PROBE`. - a malformed key that looks like a placeholder (`ep-not-an-endpoint`) still refuses identity after one read. Before the fix, the three placeholder cases and the deadline case failed with `E_LEGACY_COMPOSE_BINDING_IDENTITY`. That is the CI symptom. I applied each mutation, ran the tests, and restored the file: | Mutation | Tests that failed | | --- | --- | | Drop `transient ||` from the loop condition | 3: loop-read placeholder, two-bridge, persistent | | Accept only IDs in `bridgeMembers` (the old behavior) | 4: all placeholder cases and the deadline case | | Loosen the pattern to `/^ep-/` | 1: the malformed-key test | | Drop the plural `transient ||=` | 1: the two-bridge test | | Diagnostics: map `legacy-state` to the CHANGED message | 1: the new token test | | Diagnostics: print the raw message instead of a token | 4: every test that checks for leaked text | | Diagnostics: match by prefix instead of exact text | 1: the new token test (suffix case) | ## Validation - `tests/native-compose-adoption-binding.test.ts`: 104 pass, 0 fail. I ran it with `--timeout 60000`, a private HOME, HACK_HOME and DOCKER_CONFIG, and `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`. The run excluded one existing test, "moving job snapshot counterexample". It times out at its own 30 s limit on this Mac (load average 50 to 90), and it fails the same way with the unmodified `origin/next` source. - `tests/native-compose-adoption-job-diagnostics.test.ts`: 8 pass. The adoption execution, jobs, probe and mount-format files also pass. `tests/native-compose-adoption-job-worktrees.test.ts` has 1 or 2 process-timing failures on this loaded Mac ("Synthetic owned child did not become ready" in 2 s). They fail the same way with the original diagnostics file. - `bun run --cwd packages/cli typecheck`: 0 errors. - Root `bun run check`: `privacy-check: ok`. `@hack/cli:check` shows the 34 existing warnings. In the binding module, the same 4 complexity warnings existed before; `inspectResources` goes from 30 to 31. - `docs:cli-reference` and `generate:agent-plugins` are not needed: the CLI surface and the consumer agent guidance did not change. - gitleaks 8.30.1 (`gitleaks dir` on the exported `git log -p origin/next..HEAD`): no leaks found. - I ran no Docker e2e on this Mac. Hosted CI at head `3d26b784` (run 40068851447): all four docker-e2e shards and the aggregate `docker-e2e` gate pass. `native-compose-adoption-job- worktrees` passes in shard 2 (98.3 s), and its `cli-result` lines now end with `refusal=`. The Config compiler, Candidate core (ubuntu), runtime-images, linux-process-lifetime, Runtime state models and secret-scan checks also pass. - Hosted `test` is **red**: shard 3/4 failed one test in `tests/project-env-layer-lock.test.ts`, "concurrent legacy dotenv sets and removals of different keys all apply". A concurrent `upsertDotEnvValue` read the env file during a replacement and refused it as "a symlink or not a regular file". This PR does not touch that code. HACK-1422 already tracks this as an intermittent failure. It is not on this workflow's list of flakes that may be rerun, so I did not rerun it. Shards 1, 2 and 4 pass. ## Gaps - The CI log does not name the cause. The scenario printed only the code and ordered detail, no artifacts were kept, and the CLI gives no finer stage. The `ep-` race is the only path I found that matches all the evidence. But it is inferred from the source and Moby's code, not observed. If the failure comes back, the new `refusal=` token will show whether it is an adoption-store refusal (`legacy-state`) or something else. - `legacy-state` is still shared by many store sites. A recurrence with `legacy-state` after this fix would point to another transient read, such as ID-keyed delayed endpoint removal, which the doc already lists as able to refuse. - I did not reproduce the window against a real daemon, by design (no local Docker e2e). Refs HACK-1421 ## Round 2 (current head c9ce3f9) - Merged origin/next 6e4bcf2 into the branch (no conflicts; the diff against next is unchanged). - Hosted CI run 40102249794 at c9ce3f9: every check passed, including all four `test` shards and the aggregate `test`, and all four docker-e2e shards. The earlier red `test` at 3d26b78 (run 40068851447) was HACK-1422's legacy dotenv concurrency test, which this PR does not touch; its fix is #480. - Independent verification: next's binding module fails 4 of the 5 new placeholder tests (3 with E_LEGACY_COMPOSE_BINDING_IDENTITY, the CI symptom); the PR head passes them. Mutations of the binding and diagnostics code were killed. - Remaining follow-ups (hand-copied diagnostic strings, a load-sensitive two-bridge test, a pre- existing untested duplicate-key check) are tracked separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #379 (HACK-1332) on the target inspector. Item 4 of the ticket (stale
refs/remotes/m3ci/F17fuon the M3) was already done and is not part of this PR.Root cause
inputPresent()insrc/lib/project-input-selection.tsclassifiedEACCESandEPERMaspermission. A fixture can only produceEACCES(a mode-000 directory), so droppingEPERMfailed no test.permissionselection kind toinput_permission_denied. Butdocs/reference/ax-target-result-contract.mdstill said an unsearchable project directory isadoption_state_unsafe("Use explicit adoption recovery"). Theinput_permission_deniedmessage also named no fix..devfile or a mode-000.devfromE_UNEXPECTED(with the raw cause) to a closedE_CONFIG_INVALID, but kept nothing about the fault. That left tests and debugging with no signal.Fix
projectInputAccessFault(error), andinputPresent()calls it. A new test injectsEPERM,EACCES,ENOTDIRandEIO.input_permission_deniedmessage now names the permission fix first: "A project input could not be read: permission denied. Fix the permissions first: make the project directory (.hack or .dev), the directories inside it and its input files readable by your user, and the directories searchable (for example chmod u+rwx .hack), then retry." The contract doc now says an unsearchable.hackor.hack/.internalisinput_permission_deniedeven on a project that never used adoption.adoption_state_unsafecovers only unparseable or interrupted adoption state. The proof matrix has a new row for this case.ProjectInputAccessErrornow keeps a value-free diagnostic: the class name and errno code (for exampleError:ENOTDIR), checked against identifier-shaped patterns. It is a non-enumerable own property, read withprojectInputAccessDiagnostic(). This follows the design of the native env metadata refusal in fix(env): keep a value-free cause on native metadata refusals and trim tracked-local fixture #420 (HACK-1362). That PR is not merged yet, so the helper is local to this module. Envelopes, printed CLI errors andJSON.stringifyare unchanged: no path, no OS message, nocause. The public code and message are unchanged.Tests and mutation evidence
Each new or extended test failed under a mutation, and passed again once the mutation was reverted:
|| code === "EPERM": the new "EPERM and EACCES classify as permission" test fails..devfile, the EPERM seam test and the mode-000.devtest).Object.keys,String()andJSON.stringifycarry neither the code nor the path.input_permission_deniedmessage: the new domain test fails ("an unsearchable .hack or .hack/.internal names the permission fix, not adoption recovery"). That test covers.hackand.hack/.internalat modes 000 and 0644 on a never-adopted project.permissionkind back toadoption_state_unsafe: the same domain test fails.The mode-000/0644 cases are skipped when running as root, like the existing ones.
Validation
tests/project-input-selection.test.tsandtests/project-target-inspect.test.ts: 54 pass, 0 fail.bun run --cwd packages/cli typecheck: 0 errors.bun run check:privacy:checkok.@hack/cli:checkreports 0 errors and 34 existing warnings; Biome reports no issues in the 4 changed source and test files.bun run docs:cli-reference: no diff, because the CLI surface did not change. No consumer agent guidance changed.gitleaks diron the exportedorigin/next..HEADpatch): no leaks.testjob at this PR head (see checks).Gaps
HACK_DEBUG=1. It is available to tests and to code that callsprojectInputAccessDiagnostic(). Showing it would be a separate output decision..hack) also maps toinput_permission_denied. The message names.hackor.devas the place to fix.Refs HACK-1390, HACK-1332.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.