Repository navigation
fix(agents): keep a value-free diagnostic on adoption marker access refusals - #462
Merged
Merged
Conversation
…efusals Follow-ups to #452 (HACK-1390) on the target inspector. - LegacyComposeAdoptionSelectionError's `permission` and `input-access` kinds now keep the same non-enumerable class/code diagnostic as ProjectInputAccessError (read with legacyComposeAdoptionSelectionDiagnostic). The helpers move to src/lib/value-free-diagnostic.ts so both errors share them. - Pin that inputPresent() classifies through projectInputAccessFault by faulting the real presence check with EPERM, EACCES and EIO. - Paired CLI/MCP fixtures now cover .hack and .hack/.internal at modes 000 and 0644, matching the proof-matrix row. - The input_permission_denied message assertions run unconditionally, also as root. Refs HACK-1415 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
marked this pull request as ready for review
October 24, 2026 05:49
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)
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…urate gitignore entry, no-TTY Tailscale test) (#479) The six small CLI follow-ups in HACK-1426, from the verifiers of #466, #441, #453, #460 and #462. ## Root cause 1. **Tailscale guard had no test.** The `canPrompt()` guard in front of "Turn on Tailscale?" in `setupTailscale` (`src/commands/ssh.ts`) had no case in `tests/no-tty-prompt-sites-cli.test.ts`. If you changed it to `if (false)`, every ssh case still passed. 2. **Cold cache in the first case.** Each run of that file uses a fresh private HOME. Bun keeps its runtime transpiler cache under HOME (`~/Library/Caches/bun/@t@` on macOS), so the first spawn started cold inside its 10 s kill bound. On a loaded host that first case took 6.3 s and was SIGKILLed. 3. **`hack up` dropped the error code.** For a missing project, `runUpCommand` caught `MissingProjectContextError` and, without `--json`, logged only its message. `down`, `restart`, `ps`, `logs` and `open` let the error reach the CLI's closed-error renderer, which prints `E_PROJECT_NOT_FOUND: ...`. 4. **Indented `.gitignore` entry counted as present.** `ensureGitignoreEntry` (`src/lib/fs.ts`) compared trimmed lines. Git keeps leading whitespace as part of the pattern, so ` .hack.secret.key` does not ignore the key. Hack counted that line as the entry, wrote nothing, then found the key not ignored. It then refused with the "a later rule re-includes the key" message, which names the wrong cause. 5. **ACL test could leave mode 0444.** In `tests/git-info-exclude.test.ts`, the macOS ACL test ran `chmod 0444`, the ACL grant and `access(W_OK)` before the `try` whose `finally` restores mode 0644. If any of those steps failed, the file kept mode 0444. 6. **Paired fixture lost a case.** #462 gave the paired `unsearchable` fixture a `.hack/.internal` directory. After that, nothing covered a `.hack` at mode 000 with no `.hack/.internal`, which is the most common real layout. ## Fix 1. New paired no-TTY case: `ssh --tailscale` runs against a logging `tailscale` stand-in placed first on PATH. The stand-in reports `{"BackendState":"Stopped"}`. The case expects exit 1, the `Run \`tailscale up\`, then re-run \`hack ssh\`.` hint, no prompt, and no `tailscale up` call. 2. `beforeAll` now runs one warm-up child (`index.ts --help`) under the file's private HOME. It has its own named budget: `WARM_UP_KILL_AFTER_MS` = 90 s, and the hook bound is that plus 10 s. A kill fails the file with a clear message. The cases keep the 10 s bound. 3. `runUpCommand` now handles `MissingProjectContextError` itself only under `--json`. Without `--json` it rethrows, so `hack up` prints `ERROR: E_PROJECT_NOT_FOUND: No .hack/ (or legacy .dev/) found. Run: hack init` (console logger), the same as the other lifecycle commands. `docs/cli.md` already promised this. 4. `ensureGitignoreEntry` now reads each line the way git does: it drops a trailing CR and trailing spaces, and keeps leading whitespace. An indented line no longer counts as the entry, so Hack appends the exact entry, git ignores the key, and the secret write goes ahead. The "re- included" refusal now appears only when the exact entry is present and git still does not ignore the key, which is what its message says. Tabs are not trimmed, which matches git's `trim_trailing_spaces`. 5. The ACL test's mode change, ACL grant and `access` probe now run inside the `try`. 6. New `unsearchable-no-internal` fixture (never adopted, no `.hack/.internal`). Its `.hack` at mode 000 is the first case in the paired CLI/MCP loop. The proof-matrix row in `docs/reference/ax-target-result-contract.md` now names this case. ## Tests and mutation evidence For each mutation I applied it, ran the focused tests, and restored the file. - **`if (!canPrompt())` changed to `if (false)` in `setupTailscale`:** the new case fails. The child was SIGKILLed at the 10 s bound with the prompt open. The other 3 ssh cases pass. The stand-in recorded no `up` call. - **`up`'s non-JSON branch reverted:** 2 tests fail, `up -p <empty dir> without --json …` and `up from a working directory without a project …`. The same new loop over `down`, `restart`, `ps`, `logs` and `open` still passes. - **`line.trim() === entryLine` restored in `ensureGitignoreEntry`:** 2 tests fail, the CLI case "an indented entry … is completed by an exact entry instead of being refused as re-included" and the unit case "ensureGitignoreEntry reads each line as git does …". - **Exact comparison, `line === entryLine`:** the unit case fails on the trailing-space and CRLF inputs. - **EACCES/EPERM no longer mapped to `permission` in the adoption marker's `classifyStateRootFailure`:** the paired loop fails on its first case, `unsearchable-no- internal` with `.hack` at 000. It gets `adoption_state_unsafe` / `E_CONFIG_INVALID` where `input_permission_denied` / `E_PERMISSION` is expected. - **Warm-up removed (timing, idle host):** the first case took 1.10 s without the warm-up and 0.65 s with it. The later cases are unchanged. ## Validation - Focused tests: 6 files (`no-tty-prompt-sites-cli`, `missing-project-error-code`, `project-env- key-ignore`, `project-target-inspect`, `git-info-exclude`, `doctor-generated-files`), 121 pass and 0 fail. `project-up-command` also passes. Runs used a private HOME, HACK_HOME and DOCKER_CONFIG, and `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`, as a non-root user on macOS. - `bun run --cwd packages/cli typecheck`: 0 errors. - Root `bun run check`: `privacy-check: ok`. `@hack/cli:check` shows the 34 warnings that were already there. Biome finds no issues in the changed files. - `bun run docs:cli-reference`: no diff. Only an error line changed, not the CLI surface. 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 (all shards) at this PR head. ## Gaps - I did not reproduce item 2 under load. The evidence is that the warm-up moves the cold start out of the first case: on an idle host, 1.10 s became 0.65 s. The reported failure was 6.3 s at load about 60. - Item 5 is a structural move with unchanged assertions. No mutation tells the old and new placement apart without failing the ACL setup itself. - As in #462, no mutation tells the no-`.internal` case apart from the `.hack` 000 case with `.internal`. Both get EACCES from the same state-root `lstat`. The new case is live (it fails first under the classifier mutation) but not independent. - Locally, `tests/native-compose-adoption-search-depth.test.ts` "a pending adoption more than 64 directories above …" fails in its setup with `E_LEGACY_ADOPTION_DEADLINE`. It fails the same way with `origin/next`'s `src/`, so this change did not cause it. Hosted CI decides. - Not run as root. The mode-dependent cases are skipped as root, as before. Refs HACK-1426 🤖 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 #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
.hackor.hack/.internalon a never-adopted project" indocs/reference/ax-target-result-contract.mdsays a paired fixture covers it. But the only paired CLI/MCP fixture was.hackat mode 000. The.hack/.internalcase and the 0644 cases ran only in the domain test.TARGET_REFUSALS.input_permission_denied.messagedo not depend on the user. They sat insidetest.skipIf(getuid() === 0), so a root run skipped them.inputPresent()to the classifier. EPERM was tested only by calling the exportedprojectInputAccessFaultseam directly. IfinputPresent()used its own inline classifier that dropped EPERM, no test would fail.LegacyComposeAdoptionSelectionError'spermissionandinput-accesskinds come from the adoption marker's state-root check. They cover an unsearchable or 0644.hackor.hack/.internal, and a file where.hackbelongs. These refusals kept nothing about the fault, whileProjectInputAccessErrorkeeps a value-free class/code diagnostic.Fix
.hackand.hack/.internal, each at modes 000 and 0644. Theunsearchablefixture gets a.hack/.internaldirectory and never uses adoption. The proof-matrix row now names exactly these cases.spyOnonnode:fs/promiseslstat. It faults the real presence check of one input (.dev/docker-compose.yml) with EPERM, EACCES and EIO, callsinspectProjectInputsAtRoot, and asserts theaccesskind and theError:<code>diagnostic. Other paths use the reallstat, andmock.restore()runs inafterEach. This follows the existingspyOn(fs, "open")pattern intests/native-env-values.test.ts.LegacyComposeAdoptionSelectionError(kind, diagnostic?)now attaches the same non-enumerable, identifier-checked class/code diagnostic (for exampleError:ENOTDIR) to thepermissionandinput-accesskinds. Read it withlegacyComposeAdoptionSelectionDiagnostic(). Theadoptionkind keeps none. The marker captures the diagnostic where it classifies the state-rootlstatfault. The helpers move fromproject-input-selection.tsinto a new module,src/lib/value-free-diagnostic.ts, which both errors use.ProjectInputAccessErrorbehaves 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:
inputPresent()that keeps the diagnostic but drops EPERM: 1 test fails, "input presence checks classify through the projectInputAccessFault seam".Object.keys,String()andJSON.stringifycarry neither the code nor the path.classifyStateRootFailure: 1 test fails, "EPERM on the adoption state root …".adoptionkind too: 1 test fails, "only filesystem adoption marker refusals carry a diagnostic".input_permission_deniedmessage reverted: the unconditional message test fails.permissionkind mapped back toadoption_state_unsafeintarget-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.tsandtests/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 andHACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1.bun run --cwd packages/cli typecheck(coverssrcandtests): 0 errors.bun run check:privacy-check: ok.@hack/cli:checkshows 34 warnings, which were already there, and no errors. Biome shows no issues in the 5 changed TypeScript files.bun run docs:cli-referenceis not needed: the CLI surface did not change. No consumer agent guidance changed.gitleaks diron the exportedgit log -p origin/next..HEAD): no leaks found.testjob at this PR head (see checks).Gaps
.hackand.hack/.internalat 000 and 0644 all produce EACCES from the same state-rootlstat. The loop asserts each case separately, labelled with its directory and mode, but a mutation could not break one case without the others.HACK_DEBUG=1(HACK-1415 item 5). It is available only to tests and to code that calls the reader functions.Refs HACK-1415, HACK-1390.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.