Repository navigation
fix(env): refuse a contract migration that would commit a shell value - #538
Merged
Merged
Conversation
In a legacy .hack/.env project, `hack env set` and `hack env unset` ignored --service and changed the global value. They now refuse an invalid scope with the same closed message as env-layer projects, and refuse any other non-global scope with a usage error that names the remedy (migrate to env layers, or omit --service). The env-format decision of set/unset no longer treats a layer entry that is not a usable file (a dangling symlink, a link to a directory, a directory) as absent: it refuses with E_PATH_CONFLICT before stdin is read or anything is written, instead of falling back to .hack/.env. A symlink to a regular layer counts as an env layer. Reads of an invalid layer (env list, explain, materialize) now raise a closed one-line error instead of a plain Error with a stack, and an invalid scope name in a layer is no longer echoed into that line. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cy-env-service-scope
…projects `hack env set`/`unset --service` in a legacy project points to `hack doctor --migrate-env-config`, but that command did nothing for a project with only `.hack/.env` (no `.hack/hack.env.json`), so the user looped between the refusal and doctor. - `migrateLegacyProjectEnv` takes `migrateUndeclaredDotenv`: without a contract, each `.hack/.env` entry becomes a plaintext global value in `hack.env.default.yaml` and each differing overlay entry a global value in `hack.env.<overlay>.yaml` (an empty default layer when no legacy file exists). Secret-backend values are not migrated, and hack.config.json and its secret store are left unchanged and never offered for cleanup. Only doctor sets it; implicit migrations by other commands still need the contract. - `hack doctor --migrate-env-config` uses it, and runs the file-only env migration before the Docker gate so an unreachable Docker no longer skips it. - docs/env.md documents the .env-only migration and the remedy. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…acy-env-service-scope
…ly migration hack doctor --migrate-env-config, for a project with no hack.env.json, now writes the .hack/.env and overlay values to the gitignored machine-local layers (hack.env.local.yaml, hack.env.<overlay>.local.yaml) and the tracked hack.env.default.yaml/overlay layers with no values, so secret-shaped plaintext never lands in a tracked file. Before writing anything it refuses closed, keeping the legacy files: keys that do not match ^[A-Z_][A-Z0-9_]*$ (E_CONFIG_INVALID, counted per file, never shown), legacy .env.default/.env.local/.env.<name>.local overlays, existing or unusable layer entries (E_PATH_CONFLICT), and a machine-local target git tracks or cannot report on. Layers are created only where nothing exists, and created layers are removed if a later write fails. Doctor now decides the env format with projectEnvConfigExistsForMutation, so a symlinked layer is never written through and a dangling layer refuses with E_PATH_CONFLICT instead of being replaced. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…of falling back to .hack/.env env list, env explain and runtime env resolution read a selected layer with readTextFile, which returns null for a dangling symlink, a directory or an unreadable file. The read then looked like "no env layers" and fell back to the legacy .hack/.env format with exit 0. Only ENOENT is now an absent layer; a non-file entry refuses with E_PATH_CONFLICT and an unreadable one with a closed one-line error. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… checks, and keep tracked or compose-referenced legacy files out of cleanup Review round 1 (HACK-1370): - the contract migration decides the format with the mutation check, so a dangling default layer refuses (E_PATH_CONFLICT) instead of being replaced; its layer writes are create-only - doctor's env checks and project metadata report an unusable layer entry instead of the legacy format - the .env-only migration refuses .env.<name> files whose name is not an overlay name, skips template and backup copies, and never offers a git-tracked legacy file for cleanup - doctor offers compose-referenced legacy files for cleanup only after the compose repair removed those references - a test for an unreadable default layer Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e .env-only migration Codex review on #436: - machine-local layers are published create-only as 0600 files - in a git checkout, each machine-local layer must be ignored after the .hack/.gitignore step; a read-only .gitignore without the managed block refuses instead of only warning - an unreadable legacy .env or overlay refuses (E_PERMISSION) instead of migrating as empty and then being offered for deletion Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
discoverLegacyOverlayNames skipped symlinks, so a symlinked .hack/.env.<overlay> was left out of the migration and its values were lost once the project switched formats. A link to a readable file now migrates like a regular overlay, and cleanup removes only the link. A dangling link or a link to a directory refuses with E_PERMISSION before anything is written. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n one line Review round 2 (optional P3s): the non-overlay-name refusal lists the files instead of counting them and says to select a renamed overlay with --env. Both name refusals JSON-quote each file name, so a control character cannot break the one-line message. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ate files A Compose env_file that uses a template or backup copy such as .env.sample reached no migrated layer, and the Compose repair then removed the reference, so the next launch lost those values. The .env-only migration now refuses first. Docs: overriding a migrated key for one service needs --local, because the migrated global value in hack.env.local.yaml wins over a tracked service value. A test pins both halves of that behaviour. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compose loads env_file: .env.staging on every launch, but the .env-only migration put those values only in the staging layers and the Compose repair removed the reference. With no default overlay, the next launch without --env lost them although the migration reported success. It now refuses first, unless the overlay is env.defaultOverlay or adds nothing beyond the base. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t Compose environment value Compose ranks a service's own environment above env_file, but the migration turns every .hack/.env entry into a global layer value, and Hack's generated Compose override applies those after environment. A service with env_file .env and environment PORT "4000" would switch to the .env PORT. Preserving the Compose value is not sound (global values reach every service, and Compose values may be interpolations or passthrough entries), so the migration now refuses, naming the keys and services (JSON-quoted), never values. An environment entry it cannot read refuses too. Codex review on #436 (PRRT_kwDOQwrGd86yWkPh). Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing values A repository could link .hack/.env.<name> to a file outside .hack, such as the root .env, and the contract migration would write its plain_env values to a tracked layer. That path now reads regular overlay files only, as it did before this PR, and the .env-only migration refuses a legacy link that leaves .hack. The .env-only migration also refuses when another Compose env_file sets a migrated key: that file stays in Compose, but the migrated value would override it. The refusal names keys, files and services, never values. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rvices
Compose gives a legacy file listed in env_file only to the services that list
it, but migrated values are global and reach every service. The .env-only
migration now refuses when some services list a file with values to migrate and
others do not, naming files and services, never values. A file no service lists
has no per-service boundary to keep, so it still migrates, and docs/env.md says
its values then reach every service.
An environment entry that only refers to its own key (${KEY}, $KEY,
${KEY:-default}) is no longer a precedence conflict: Compose resolves it from
the same values before and after.
Based on the uncommitted draft from the Hetzner session at ff06193. That draft
refused unless every service listed every file, which also refused a project
where no service lists .env, so the rule here is narrower.
Refs HACK-1370
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ation Compose drops an unquoted inline comment and expands $ outside single quotes, but parseDotEnv keeps both literally. A migrated PORT=3000 # dev would reach the service as that whole string through Hack's generated override. The .env-only migration now refuses such lines, counting them per file and never showing them. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ure gaps
Review of the exact head showed the ${KEY} exception changed values: an overlay
or the shell can set the key, and the generated override then wins. It is gone,
so those entries refuse again; the remedy is to drop the entry from environment.
The service check now compares what each service had: a service that would gain
a base key from a file only another service lists is refused, including through
an overlay identical to the base. Top-level include, service extends and variable
env_file paths refuse because the check cannot see what they add. When no service
lists a legacy file the migration still runs, since its values may only feed
Compose interpolation, and doctor names the keys every service then receives.
The contract migration now refuses .hack/.env or a default overlay linked out of
.hack before reading it, so a repository cannot pull an outside dotenv into a
tracked layer. That closes the route doctor exposed by migrating before the
Docker gate.
Refs HACK-1370, HACK-1686
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compose decodes backslash escapes in double quotes, expands $ across quoted lines, and accepts KEY: value and a bare KEY, so a PEM key written with \n would have migrated corrupted and a colon entry would have been lost. The migration now scans entries, tracking quoted values across lines, and refuses those forms. A default overlay reaches every launch, so one a service does not list now refuses when any service lists a legacy file. A key the Compose file interpolates that the default overlay changes refuses too: Compose takes it from .hack/.env today and the overlay would apply afterwards. Keys from an unlisted non-default overlay are named by doctor, whose note now warns against service-scoping a key Compose interpolates. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… values Compose reads text after a closing quote as another entry, so A='x' B=2 lost B, and it reads backslashes in single quotes differently, so A='a\\b' changed. A backslash before a single quote also desynced the scanner and hid a later line. The migration now refuses both, keeping a trailing comment. An interpolated name that only starts with an overlay key no longer refuses. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compose lets the last listed env_file win, but env layers always let the default overlay win over the base. env_file: [.env.staging, .env] with differing values would change the service's value and the Compose repair would remove the listing. The .env-only migration now compares each service's ordered legacy values with the selected layers and refuses a difference, naming keys and services only. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The contract migration writes plain_env values to tracked layers. A committed .hack/.env -> .env.local stayed inside .hack, passed the containment check, and would have published a machine-local credential. This path now refuses a symlinked .hack/.env or default overlay wherever it points. The .env-only migration keeps allowing links inside .hack, because it writes only to gitignored machine-local layers. Refs HACK-1370, HACK-1686 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ration Compose v5.5.0 drops an exact repeat in env_file, so [.env.staging, .env, .env.staging] passed the order check while the value changed. format: raw makes Compose keep text as written, but the migration parses it as dotenv, so quotes and trailing spaces changed. The .env-only migration now refuses both as inputs it cannot check. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on migration Compose expands ~ in env_file, but the check resolved it inside .hack, found no file, and missed a conflict with a migrated key. The .env-only migration now refuses a ~ path and a !reset or !override tag, and treats a top-level secret or config reading a key the default overlay changes like interpolation. A parent directory env_file no longer counts as a repeat of .env. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compose reads a key without a $ in places like a bare build.args entry, so matching interpolation syntax missed it and images would build with a different value. The .env-only migration now refuses when the key's name appears anywhere in the Compose file, or it is a COMPOSE_* or DOCKER_* setting, such as COMPOSE_PROFILES turning on a gated service. This replaces the per-form checks. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gration Compose merges several YAML documents, but the parser returned no map and every check was skipped, so explicit values changed. The .env-only migration now refuses a Compose file it cannot read as one document. It also searches other env_file text for keys the default overlay changes, since Compose expands variables there, and refuses DOCKER_*, BUILDKIT_*, BUILDX_*, proxy settings, SOURCE_DATE_EPOCH, HOME and PATH in any legacy file. Migrated values enter Compose's process environment, so an unchanged DOCKER_HOST could select another daemon. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Once one developer commits the .env-only migration, another checkout's .hack/.env stopped reaching services with no warning, doctor said the project already used env layers, and hack env materialize then erased the file, its only copy. doctor --migrate-env-config now moves such a checkout's values to its own local layers, leaving the tracked layers alone. hack up warns and names the keys, doctor reports them, and hack env materialize refuses to replace the file. This applies when the checkout has regular layer files, no machine-local layer, no contract and no materialization state. A Compose file with include and no services now reaches the checks too, and doctor's note says host lifecycle hooks receive migrated values. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Protection depended on which files existed, so an unrelated local layer, a stray state file or an ordinary env set --local turned it off and hack env materialize could erase a checkout's only copy. It now compares each legacy file's values with what the layers deliver, including overlays, and keeps protecting them in those states. Migrating adds to an existing local layer, refuses a key it sets differently, and no longer copies values the layers already deliver. Diagnostics printed raw parsed names, so a malformed line could echo a secret. They now name only valid keys, count the rest and name unreadable files, and malformed or unreadable entries still block materialize. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… migration A local layer that failed to parse gave an empty default, and adding migrated keys then rewrote the file, losing its entries. It now refuses and leaves the file alone. A local overlay that disagreed with its legacy overlay could pass unnoticed when the entry equalled the base, leaving a key reported as undelivered that no further run could clear. It now refuses too. Adding to an existing local layer now makes it private, because the values were in a private .env and a layer from hack env set --local may be readable by others. Refs HACK-1370 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
next's .env.default refusal now applies whatever the file holds, so this branch's test and comment that said otherwise are updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cleanup assumed every selected legacy file's entries had reached a layer. A legacy local layer breaks that, so compare with what each env now receives and keep the file, naming the keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…al value Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
added a commit
that referenced
this pull request
Nov 2, 2026
…ACK-1712, HACK-1713) (#542) Contract migration previously copied `plain_env` values from `.hack/.env` into tracked `hack.env.default.yaml`. It now writes them to the existing gitignored machine-local YAML layer, including overlay and service-scoped values. Doctor reports each moved key and destination path without values. Secret-backed values still migrate as ciphertext, and legacy files with undelivered entries remain in place. The writer reuses local-layer hydration's tracking, validation and locking checks, and requires effective Git ignore protection before plaintext writes. It refuses an outdated read-only ignore file. A preflight checks every tracked and effective local destination for aliases before migration writes, including compatibility paths and case-only aliases. Empty tracked overlay metadata preserves discovery without storing plaintext. #537's refusal is obsolete once migration preserves locality, so this removes it and keeps legacy `set --local` writing dotenv until migration. Modern `--local` writes are unchanged. #436 owns the separate no-contract migration; this change aligns contract migration's destination with it. #538's shell-only refusal remains separate: this patch does not modify either related branch. Current contract process fallback also stays machine-local. #539's undelivered-entry cleanup protection is retained. Validation: - Doctor CLI regressions cover existing-contract dotenv and HACK-1713's no-contract `set --local` followed by a new contract. They scan stdout/stderr and every staged file after `git add`, check the ignored destination, and require key/path reporting. - Built unfixed `next` at ae032e7 fails both regressions specifically at the tracked-default-value exclusion assertion; a separate built-CLI fixture reproduces that tracked write. The fixed built CLI delivers `LOCAL_KEY` locally, has no value in output or tracked files, and passes `git check-ignore`. - Focused coverage includes overlays, service targeting, linked-worktree locality, effective ignore protection, tracked/local/compatibility destination collisions, case-only aliases and preservation of undelivered legacy files. The new ignore/alias regressions fail on fd64d3e and pass with the fix. - Repository typecheck and check passed. Changed test files pass lint. Contract CLI regressions pass; project-env tests: 60 passed; focused Doctor retention test: passed. The rebuilt final CLI passes all five targeted Doctor cases (69 assertions), including HACK-1712/HACK-1713 and the new ignore/alias cases. Full local suite was interrupted under host memory pressure; broad verification is delegated to CI. Fixes HACK-1712 and HACK-1713. Release intent: `fix`; no merge or release publication performed. — Kit · bot --------- Co-authored-by: Dimitri Kennedy <344842+roodboi@users.noreply.github.com>
Keeps PR 542's machine-local contract migration and destination preflights, removes the --local refusal it made obsolete, and keeps PR 436's --service refusal and no-contract migration. Doctor's note now lists moved keys. With a compatibility local base, the migration's env-layer check now returns before the alias preflight. It writes nothing and leaves every file in place, so that test now pins this result. The CLI alias tests still cover the refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in PR 542's machine-local contract migration. This PR's refusal of a shell-only value stays. Overlay values are resolved once. An overlay's own value now moves to its machine-local layer and outranks a local base, so the recovery test and docs step say so. Reserved overlay names refuse earlier with this PR's own message, with the same result: nothing written and every source kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Outside git, a hack.env.local.yaml with `environment: local` is the `local` overlay. Base values written there reach `--env local` and not the default env, past the scoped-value check, so the migration now refuses and writes nothing. Also corrects the symlink refusal's reason after the machine-local contract migration, and makes the cleanup test assert delivery and its candidate lists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Inside git, tracking decides what hack.env.local.yaml is, so a tracked layer got a refusal whose remedy didn't work. It now gets the tracked-layer refusal again. Adds tests for that case, for the remedy, and for cleanup keeping a file whose values its env doesn't receive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also corrects what PR 542 made untrue: the shell-only refusal's reason, its JSDoc, docs and threat model, and the printed overlay recovery. Documents and tests that `.env.local` refuses even with no value of its own, and renames two tests that claimed more than their assertions show. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The branch for an unavailable backend key still claimed a tracked layer. Restores JSDoc for quoted names and that refusal, and names three tests for what they assert. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
added a commit
that referenced
this pull request
Nov 2, 2026
#436) ## Current state at `2bb4271237ee676e32e992d72c703fe024da86a6` Merges `next` at `ae032e79` (#539) and resolves the conflict with it. Changes since `eaa083dc`: - **Scoped deletion with no global value:** a migrated value no longer restores a key that a service or host scope explicitly deleted when no global value exists. This is the should-fix from the `eaa083dc` review. The two new table cases failed before the fix. - **Retention on the no-contract route:** cleanup now compares what each selected legacy file's env actually receives, in place of an empty map. A legacy file holding a value its env still wouldn't receive is kept, and only key names are reported. - **Unlistable primary:** the real permission test stays. CI gets a bounded Linux step for it, and an injected filesystem-failure test covers the branch on other platforms. - **Ordering:** `.hack/.gitignore` is created right after the migration, before the Docker gate. **Verified locally:** typecheck, and 405 focused tests pass at this head. Gitleaks passes over `eaa083dc..2bb4271`. **Not verified yet:** hosted CI at this head was still running when this was written, including whether the Linux step really runs its test. The full suite wasn't run locally. **Not reviewed yet:** the retention and scoped-deletion changes have had no independent review. Sections below describe earlier heads. --- ## Current state at `eaa083dc47f7ff03893bbf1a5e205fb53502f64b` **Held; don't merge.** Besides HACK-1694 below, this is held for the preservation risks in HACK-1695: this PR extends that deletion to Docker down. Don't release it without #538 either: on this PR alone, doctor with a malformed contract still writes an empty layer and removes `.env` and the contract, as it does on `next`. Merged with `next` at `f938d629`, which includes HACK-1710 (#537), HACK-1685 (#535), #533, #534 and the fixture daemon prevention (#536). The only conflict with #537 was additive, and both refusals are kept. The sections below this one describe the PR's earlier history; where they differ, this section is current. **Review coverage, one independent reviewer, reading only:** source through `d0376173`, docs and tests through `f980705d`, the merge `c07cc4ae`, and the `env-arguments.ts` and `env.ts` resolution in `b9ec8778`. Not read: the docs commit `11d5e8ff`, or the rest of the later merges. Automatic Codex review stopped on a quota notice at `91db1781` and wasn't retried; it isn't a gate. This is not an approval. **Added since `b9ec8778`: a migrated value no longer replaces a service or host value.** A later env layer beats an earlier one whatever their scopes, so a migrated machine-local global value silently replaced a service or host value from an earlier layer. Measured at `b9ec8778` with real files and synthetic values: `api` moved from its tracked value to the legacy one, and likewise a host value and a tracked overlay's service value. On `next` the same inputs don't migrate, so this was a regression in this PR. The migration now resolves each target with the planned layers, for the base, every overlay and any overlay a linked worktree inherits, and refuses before writing, naming keys, targets and envs only. Each new test failed on the source before its repair; the first control for the inherited case was invalid (its fixture hit an unrelated refusal), so it was fixed and rerun. Coverage added after that: a service and a host value together, a Compose service named `host`, a scoped secret (replaced, equal, and in the same local layer), and an unknown scope. Disabling the check fails 4 of those tests and removing the secret guard fails 1; the rest guard against false refusals. A layer with an invalid scope refuses earlier, as unreadable, and has its own test. A Linux-only test covers a primary directory that can't be listed, and CI is its first real run: check that it ran, not just that the suite passed. At this head: 395 pass, 0 fail across env-legacy-service-scope, project-env-config, symlink-boundary, doctor-fix-noninteractive, doctor-command, local-inheritance, project-env-metadata and write-refusal-gaps. Biome, typecheck and gitleaks pass. No local full suite. **Review of this fix, the same independent reviewer, reading only:** `b9ec8778..a62921a`, no blockers. The last commit `eaa083dc` (test-only: its two accuracy points) wasn't read. On macOS a primary directory that can't be listed turns inheritance off, for migration and runtime alike, so the listing refusal is reached only on Linux. Still open: HACK-1709 items 9 and 10. This is not an approval. **Added since `ca5ef35f`:** - A generated `.hack/.env` isn't migrated as legacy values, and Hack checks its text to tell. A hand-edited one refuses. - These now refuse and write nothing: - a state entry that exists but can't be read, including a dangling link or a link into a directory without search permission; - an env layer that can't be read or resolved, including a missing project env key on any overlay; - a Compose file that can't be read; - another `env_file` that sets a migrated key as `export K=`, `K:`, a bare `K` or after a closing quote. - The project boundary from HACK-1685 now also applies when a layer is a symlink. Without that, `hack doctor` rewrote a Compose file outside the project through a linked `.hack`. **Local verification at `91db1781`** (the merge to `b9ec8778` had its own focused run and scan; Bun 1.4.2, one file at a time, empty environment, stubs first on PATH, private HOME and TMPDIR): - `env-legacy-service-scope` 193, `project-env-metadata` 9, `project-env-write-refusal-gaps` 28 with the ExFAT case skipped, `doctor-fix-noninteractive` 22, `doctor-fix-adopted-project` 4, `doctor-command` 28, `project-env-config` 58, `daemon-autostart-fixture-guard` 7, `project-env-symlink-boundary` 28, `project-env-local-inheritance` 9, `project-env-stable-ciphertext` 18, `cli-spawn-env-isolation` 7, `launchd-isolation` 14, `e2e-launchd-guard` 5. - Compose merge through `docker compose config` with a socket that doesn't exist, Compose v5.5.0: 38 inputs, 29 refuse, 9 migrate with the same rendered values, none differ. - Controls: the dangling-link case fails on `ce5c770c` source, and the no-search state link migrated generated values on that source. - Typecheck, Biome on changed files and gitleaks on `ca5ef35f..91db178` pass. **Not verified locally:** the full suite, and `scripts/docs/check.ts`, which needs generated `site/content/docs` and fails the same way on `next`. CI at the exact head is the evidence for both. All 20 checks passed at `91db1781`; read `b9ec8778` from its checks. **Open decisions, not waived:** - **HACK-1694:** with a symlinked layer, `hack doctor` can remove `env_file: .env`, and a service loses a value only that file set. Measured on this head and on `next` with Docker up; this PR also makes it happen with Docker down. Documented, not fixed. - A missing project env key on any overlay blocks the whole migration. It stays fail-closed until decided. - `hack env set` refuses an in-project dangling layer link, where `next` replaces it. `hack env add` still replaces it. Documented until decided. **Release signal:** squash title `fix(env): refuse --service and unusable layers in legacy env set/unset`. Non-blocking follow-ups are in HACK-1709. --- Refs HACK-1370. Unit G60. ## Root cause 1. **`--service` dropped in legacy mode.** `handleEnvSet` (`src/commands/env.ts`) sends a project with env layers to `env add`, which reads `--service`. Its legacy `.hack/.env` branch never read `args.options.service`, so `hack env set NEW --service api --value-stdin` wrote a global `NEW` to `.hack/.env` and exited 0. An invalid scope such as `'Bad Scope!'` was accepted the same way. `handleEnvUnset` had the same gap in its legacy branch, so `unset KEY --service api` removed the **global** value. 2. **A dangling layer switched formats.** `projectEnvConfigExists` checks the default layer with `pathExists`, which follows symlinks, and checks overlays with `Dirent.isFile()`. A dangling `.hack/hack.env.default.yaml`, or any symlinked overlay, therefore read as "no env layers". `env set` then wrote `.hack/.env` and exited 0. 3. **Read paths printed a stack.** `readProjectEnvLayers` threw a plain `Error("Failed to parse …")` for an invalid layer. The top-level handler treats a plain `Error` as unexpected, so `hack env list` / `explain` / `materialize` printed the full stack. ## Fix - `env set` validates `--service` up front with `assertValidProjectEnvScopeName`, in both formats, before the project, stdin or any file is touched. This is the same closed one-line refusal that `env add` gives. `env unset` does the same in its legacy branch. - In legacy mode, a non-global scope is refused with a `CliUsageError` (`LEGACY_ENV_SERVICE_SCOPE_MESSAGE` in `src/cli/env-arguments.ts`). The message names the remedy: migrate with `hack doctor --migrate-env-config`, or omit `--service`. The legacy `.hack/.env` holds one value per key, so the scope can't be honoured. Omitting the scope, or passing `--service global`, still works. The refusal happens before stdin is read or a secret store is resolved. - New `projectEnvConfigExistsForMutation` (`src/lib/project-env-config.ts`) is used by `env set` / `env unset` to choose the env format. Every `.hack/hack.env.*.yaml` entry must be a regular file or a symlink that resolves to one, and either kind counts as an env layer. Anything else (a dangling link, a link to a directory, a directory) is refused with the new `ProjectEnvLayerPathConflictError` (`E_PATH_CONFLICT`, one `CODE: message` line naming the path), and nothing is written. - `readProjectEnvLayers` now throws `ClosedCliError` with the same text as the write path's parse refusal. The layer parser no longer echoes an invalid scope name: it is file content and could contain a value or a line break, which would break the one-line output. - `docs/env.md` ("Legacy repos and migration") documents the legacy `--service` refusal and the layer-entry rule. **Decision for review:** a symlink that resolves to a regular layer file is **accepted** and counts as an env layer. It is not refused. The atomic writer already writes through such links by design, and refusing them would break projects that use them today. The only change here is that a symlinked overlay no longer silently selects legacy mode. If you want every symlinked layer refused, that is a one-line change in `projectEnvConfigExistsForMutation`. ## Tests `tests/env-legacy-service-scope.test.ts` (new, 14 tests). Every CLI child gets a private HOME, HACK_HOME, global config and Docker client config, plus an `encrypted_file` secret backend inside the temp dir with an env-provided key and `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`. That keeps the keychain unreachable even when a guard is mutated away. Values are synthetic and assembled at runtime, and every test asserts that no value appears in the output. - legacy `set --service api` (with and without `--secret`) refuses, and nothing is written to `.hack/` or the secret store - legacy `set --service '<invalid>'` gives the closed scope message, without echoing the argument - positive control: legacy `set` without `--service`, and with `--service global`, still writes `.hack/.env` and creates no layer - legacy `unset --service api` / invalid scope refuses, and `.hack/.env` is unchanged - `set` and `unset` refuse a dangling default layer, a dangling overlay, a local layer linked to a directory, and a directory default layer with `E_PATH_CONFLICT` on one line; the entry and `.hack/` are unchanged - a symlinked overlay that resolves to a regular file selects the env layers (writes `hack.env.default.yaml`, no `.hack/.env`) - a unit test of `projectEnvConfigExistsForMutation` and the error class and code - `env list` / `env explain` on invalid YAML, and on a layer with a line-break scope name, print one line with no stack under `HACK_LOGGER=console|clack|gum` - an invalid layer read is a `ClosedCliError` whose reason does not echo the scope **Mutation proof** (each mutation applied alone, then reverted; each was caught): | Mutation | Caught by | |---|---| | drop `assertLegacyEnvScope` in set | legacy set scope refusal | | drop it in unset | legacy unset scope refusal | | no scope validation in set | invalid-scope refusal | | no scope validation in unset | legacy unset refusal | | set uses old `projectEnvConfigExists` | 4 layer-entry tests + symlinked-overlay test | | unset uses old `projectEnvConfigExists` | 4 layer-entry tests | | linked layer not counted | symlinked-overlay test | | read path back to plain `Error` | 3 logger tests + closed-error unit test | | scope name echoed again | 3 logger tests + closed-error unit test | | conflict code `E_UNEXPECTED` | 4 layer-entry tests + unit test | ## Evidence - Bun 1.4.2 (`mise x bun@1.4.2`), `bun install --frozen-lockfile`, no `bun.lock` change. - Local (macOS, this checkout): - `bun run check` passes, including `privacy:check: ok` (warnings only, all pre-existing) - `bun run --cwd packages/cli typecheck` and root `bun run typecheck` pass with 0 errors - focused tests pass: the new file, `project-env-write-refusals`, `env-value-stdin-cli`, `env-key-argument-redaction-cli`, plus 13 other env/project-env files - `env-selection-parity` needs the config compiler, which is not built locally; it runs in the M3 suite below - `gitleaks` 8.30.1 `dir` scan of the `git log -p origin/next..HEAD` export: no leaks. - `bun run docs:cli-reference`: no change (the CLI surface is unchanged). - Full suite on the M3 at the exact head: pending (queued on the shared M3 lock behind other units; this PR will be updated with the result) ## Gaps - Read paths still fall back to legacy for a dangling or unreadable default layer: `env list` shows "base (.hack/.env only)" and exits 0. Only the `set`/`unset` format decision refuses here. Reported below as a follow-up finding. - `hack doctor --migrate-env-config` migrates only projects that have `.hack/hack.env.json`. A legacy project with only `.hack/.env` has no automated migration, and the refusal message points to the docs section for that case. **(Fixed in round 2, below.)** ## Further findings (not fixed here) 1. **Read paths silently pick legacy for a dangling or unreadable default layer** (Medium). - Repro: `.hack/hack.env.default.yaml` is a dangling symlink, or mode 000; run `hack env list --path <p>`. - Expected: a closed refusal, as `set`/`unset` now give. - Actual: exit 0 with "env_selection base (.hack/.env only)". `readProjectEnvLayers`' non-strict read treats both as absent, so runtime injection probably falls back the same way (not verified). 2. **Legacy `env set --local` is ignored** (Low). - Repro: in a legacy project, `printf v | hack env set NEW --local --value-stdin`. - Expected: a refusal, or a documented no-op. - Actual: "Updated .hack/.env", exit 0. `.hack/.env` is machine-local, so the effect matches, but the flag is silently dropped. ## Round 2: a working remedy for `.env`-only legacy projects **Verifier defect (Medium, blocking).** The legacy `--service` refusal points to `hack doctor --migrate-env-config`, but that command did nothing for a project with `.hack/docker-compose.yml`, `hack.config.json` and `.hack/.env` but no `.hack/hack.env.json`. `maybeMigrateProjectEnvConfig` returned silently without the contract, and `migrateLegacyProjectEnv` returned `legacyDetected: false`. The user looped: refused, doctor did nothing, refused again. A second cause made the loop worse: doctor ran the env migration only after the Docker gate, so with Docker unreachable it skipped the migration even for a project with a contract. **Fix** (head `456bdad5`; `next` 5208ee5 merged in first, with no conflicts): - `migrateLegacyProjectEnv` takes a new `migrateUndeclaredDotenv` option. Without a contract it now migrates `.hack/.env` and its overlays: - each `.hack/.env` entry becomes a plaintext global value in `hack.env.default.yaml` - each `.hack/.env.<overlay>` entry that differs from the base becomes a global value in `hack.env.<overlay>.yaml` - a project with no legacy env file gets an empty default layer, so the refusal's remedy works for every legacy shape - Without a contract, nothing names secret-backend values, so none are read or migrated. `hack.config.json`, and any secret backend it configures, is left unchanged, and the backend's store is never offered for cleanup. Values go only to the layer files; they are never returned or printed. - Only doctor sets the option. The implicit migrations that other commands run (for example `env add`, `env hydrate`, `env materialize`, and the project and session flows) still need the contract, so no command starts migrating `.env`-only projects on its own. - `hack doctor --migrate-env-config` passes the option. It now runs the env migration, which changes project files only, before the Docker gate. Its plan line names the `.env`-only case. - `docs/env.md` ("Legacy repos and migration") documents the `.env`-only migration: secrets are not migrated and must be set again, the config is unchanged, the migration runs without Docker, and the cleanup prompt follows. The `--service` paragraph now names the remedy and says it also covers `.env`-only projects. The refusal text (`LEGACY_ENV_SERVICE_SCOPE_MESSAGE`) is unchanged, because the command it names now works. **Tests added** - `tests/env-legacy-service-scope.test.ts`, 3 new tests (17 in total): - `.env` plus `.env.staging`, no contract: `set --service api` is refused; the explicit migration writes the expected default and overlay layers (the overlay keeps only the changed key); the config file is byte-identical; the cleanup candidates are exactly the two dotenv files; then the same `set --service api` exits 0 and writes the `api` scope; no secret store is created and no value appears in the output. - a project with no legacy env file: the migration writes an empty default layer, and `set --service api` then works. - without the option, a contract-less project is not migrated and `.hack/` is unchanged. - `tests/doctor-fix-noninteractive.test.ts`, 2 new cases: `doctor --migrate-env-config --no-interactive` on a `.env`-only project writes `hack.env.default.yaml` with the legacy value, once with Docker reachable and once with it unreachable. No value appears in the notes. The mocked `docker info` exit code is now set per test. **Mutation proof** (each mutation applied alone by `scratchpad/G60/mutate.sh`, then reverted; the source was confirmed restored): | Mutation | Failing tests | |---|---| | doctor does not pass `migrateUndeclaredDotenv` | doctor 2 | | doctor requires the contract again | doctor 2 | | env migration moved back after the Docker gate | doctor 1 (the unreachable case) | | migrator ignores the option | doctor 2, env 2 | | migrator migrates without the option | env 1 | | no overlay-vs-base filter | env 1 | | contract-less path rewrites `hack.config.json` | env 1 | **End-to-end repro** (`scratchpad/G60/remedy-loop.sh`, private HOME, HACK_HOME and DOCKER_CONFIG, fake Docker host, stub `security`, `launchctl` and `docker` on PATH, keychain fallback and hackd autostart disabled): 1. `env set NEW_SETTING --service api --value-stdin` is refused (exit 1). 2. `hack doctor --migrate-env-config --no-interactive` writes `hack.env.default.yaml`, removes `.hack/.env`, then reports Docker unreachable. Exit 1 comes from the failed Docker check; no value is printed. 3. The same `env set` exits 0 with `scope=api`. The layer holds `global.KEPT_SETTING` and `api.NEW_SETTING`, and no secret store is created. **Gates at `456bdad5`** - `bun run --cwd packages/cli typecheck`: 0 errors. - Root `bun run check`: exit 0, `privacy-check: ok`. No diagnostics in the changed files. - `ultracite check` on the changed source and test files: clean. - gitleaks 8.30.1 `dir` over the `git log -p origin/next..HEAD` export: no leaks. (`gitleaks git` is refused by the worktree guard.) - Focused tests, private env: `tests/doctor-*`, `tests/env-*`, `tests/project-env-*` and `tests/compose-env-*` gave 527 pass and 2 fail across 42 files. The two failures do not involve this PR: - `env-selection-parity` needs the config-compiler binary, which is not built in this checkout. - `project-env-layer-lock`'s "concurrent legacy dotenv sets and removals" fails intermittently on `next` 5208ee5 as well (2 of 8 runs on a clean `next` worktree, 1 of 5 here). This is reported below. - `doctor-command`'s native-input test needed `--timeout 60000` locally; it spawns 4 CLI children in a 5 s default. With that timeout it passes 28/28. - The CLI surface and consumer agent guidance did not change, so `docs:cli-reference` and `generate:agent-plugins` were not needed. - Hosted CI at `456bdad5`: every check passes on run 40060725798 (head SHA `456bdad5`, conclusion success). The `test` job (one job, not sharded) reports 8112 pass, 0 fail, 8269 tests across 575 files, and both new test files ran, including the 5 new tests. All four docker-e2e shards, Candidate core (macOS and Ubuntu), Config compiler (4 platforms), Runtime state models, linux-process-lifetime, runtime-images and secret-scan pass. **Round 1 gap now closed:** the second bullet under "Gaps" above (doctor migrated only projects with `.hack/hack.env.json`) is fixed by this round. The earlier "Full suite on the M3: pending" line is replaced by hosted CI's `test` job: round 1 at `f87ee54c` passed (7913 pass, 0 fail), and round 2 is reported above. **Set aside, not in this PR:** a local draft that refused *every* symlinked layer, including one that resolves to a regular file, was not included. It would reverse the documented "Decision for review" above and the atomic writer's documented symlink behaviour, and no verifier asked for it. **Further finding (not fixed here)** 3. **`project-env-layer-lock` legacy dotenv concurrency test is flaky on `next`** (Low; test or race). - Repro: run `bun test tests/project-env-layer-lock.test.ts` repeatedly on a clean `next` 5208ee5. - Expected: 9/9 pass every run. - Actual: about 1 run in 4 fails "concurrent legacy dotenv sets and removals of different keys all apply". One worker exits 1 with the `readReplaceablePrivateFile` refusal "the env file … is a symlink or not a regular file", which means the inode changed between `lstat` and `open`. The legacy `.hack/.env` read in `upsertDotEnvValue` seems to race with another worker's rename, outside the serialized section. The failing worker's stderr also carries a stack, not one closed line. ## Round 3: a safe `.env`-only migration (verifier defects A, B and C) Head `78b72762`. It merges `origin/next` `7dac4a03` (no conflicts) and adds one commit. This section replaces the round 2 behaviour described above. Round 2 copied `.hack/.env` values into the tracked `hack.env.default.yaml`. The round 2 test descriptions and repro lines that name that file now describe the old behaviour. **What changed** - **C (blocking): values stay out of tracked files.** `.hack/.env` is gitignored and can hold passwords or API keys, so the migration no longer copies its values into a tracked layer. Each `.hack/.env` entry becomes a plaintext global value in the gitignored machine-local layer `hack.env.local.yaml`. Each overlay entry that differs from `.hack/.env` goes to `hack.env.<overlay>.local.yaml`. The tracked `hack.env.default.yaml`, and `hack.env.<overlay>.yaml` for each migrated overlay, are written with no values, which switches the project to env layers. The `.hack/.gitignore` managed block is ensured first, as `hack env set --local` does. A machine-local target that git tracks is refused. So is one inside a git checkout where git cannot report its status (fail closed, as `hack env hydrate` does). The resolved env still holds every value. - **A: invalid keys refuse closed.** Before anything is written, every key in `.hack/.env` and its overlays must match `^[A-Z_][A-Z0-9_]*$`. Otherwise the migration exits with `E_CONFIG_INVALID` on one line, for example "2 keys in .env, 1 key in .env.staging do not". The keys themselves are not shown, because a malformed line (`export X=…`, base64 padding) may hold a value; this matches `ENV_KEY_INVALID_MESSAGE`. Nothing is written, the legacy files are kept, and no cleanup is offered. - **B: doctor uses the mutation format check.** `maybeMigrateProjectEnvConfig`, and the `--plan` step, now decide the format with `projectEnvConfigExistsForMutation`. A symlinked layer counts as an env layer: doctor says the project already uses env layers and writes nothing through the link. A dangling or non-file layer refuses with `E_PATH_CONFLICT`, and the entry is left in place. The migration also runs the same check itself, so a direct caller cannot write through or replace such an entry either. - **Related guards in the same path.** A legacy `.env.default`, `.env.local` or `.env.<name>.local` would land on the default layer or a machine-local layer rather than an overlay, so it is refused with `E_CONFIG_INVALID`. `.env.state.json` is Hack's own state file and is no longer read as an overlay. Every layer is created with a compare-and-swap against an absent file. If a later write fails, the layers this call created are removed, so the project stays on the legacy files. - `docs/env.md` "Legacy repos and migration" documents where the values go, that teammates do not get them through git, and each refusal. **Tests** (`tests/env-legacy-service-scope.test.ts`, `tests/doctor-fix-noninteractive.test.ts`) - The `.env`-only migration test now asserts that values are in `hack.env.local.yaml` and `hack.env.staging.local.yaml`, that the tracked layers hold `global: {}`, and that `resolveProjectEnvConfig` still returns every value. A service-scoped `env set` still works after migration. - New, C: in a `git init` repo with `DATABASE_PASSWORD` and an `sk_test_…`-shaped `STRIPE_SECRET_KEY` (synthetic), no file that `git ls-files --cached --others --exclude-standard` lists contains either value, and the local layers are ignored. - New, A: `GOOD_KEY`, `node_env` and `export OTHER` (plus `lower_case` in `.env.staging`) give a closed `E_CONFIG_INVALID` refusal with per-file counts. No key or value appears, and the `.hack` entries and legacy file bytes are unchanged. - New: `.env.default`, `.env.local` and `.env.staging.local` are each refused, and nothing is written. - New: a machine-local target that is in the git index (but not on disk) is refused with `E_PATH_CONFLICT`. - New, B (library): a symlinked `hack.env.staging.yaml` is refused, and its outside target keeps its content. A dangling `hack.env.default.yaml` gives `ProjectEnvLayerPathConflictError` and is still a symlink afterwards. - New, B (doctor): `doctor --migrate-env-config --no-interactive` with a symlinked overlay exits 0, reports "already uses the new env config files", and leaves the link, its target, `.env` and `.env.staging` unchanged. With a dangling default layer it exits 1 (`E_PATH_CONFLICT`), and the link and `.env` are kept. - The round 2 doctor migration test now expects the value in `hack.env.local.yaml` and an empty tracked default layer. **Mutation proof** (each change made alone, then the named test run, then the source restored): | Mutation | Killed by | | --- | --- | | invalid-key refusal disabled (A) | invalid-keys test | | doctor back to `projectEnvConfigExists` (B) | doctor symlinked-overlay test | | migration's own format guard removed (B) | library existing-entries test | | values written to the tracked default layer (C) | secret-shaped git test | | reserved overlay check disabled | all 3 reserved-name tests | | git-tracked check disabled | git-tracks test | **Evidence** - `bun run --cwd packages/cli typecheck`: 0 errors. Root `bun run check` (includes `privacy-check: ok`): passes, with warnings only from existing files. - gitleaks 8.30.1 `dir` over the `git log -p origin/next..HEAD` export: no leaks. - Focused tests, run locally with load average around 83: `env-legacy-service-scope` passes 24/24 and `doctor-fix-noninteractive` passes 19/19 (each file run on its own). - The CLI surface and consumer agent guidance did not change, so `docs:cli-reference` and `generate:agent-plugins` were not needed. - Hosted CI at `78b72762`: every check passes on run 40129035737 (head SHA `78b72762`, conclusion success). That includes `test` shards 1-4 (the full suite, now sharded), docker-e2e shards 1-4, secret-scan, Candidate core (macOS and Ubuntu), Config compiler (4 platforms), Runtime state models, linux-process-lifetime and runtime-images. **Gaps** - The rollback of created layers after a failed write has no test, because a write failure after the preflight is hard to inject. Its logic is short and only removes paths that did not exist before the call. - The doctor dangling-layer test survives when only the doctor check is reverted, because the migration's own guard still refuses. It fails only when both guards are reverted; each guard is covered by its own test. - Behaviour change for review: migrated values now stay on the machine that ran the migration. Teammates need `hack env set` or `hack env set --secret` for the values they share. This is documented. **Further findings (not fixed here)** 4. **Legacy `.hack/.env.<overlay>` files are outside the managed `.hack/.gitignore` block** (Medium; pre-existing). - Repro: in a git repo with the managed block, create `.hack/.env.staging`, then run `git ls-files --others --exclude-standard`. - Expected: ignored, like `.hack/.env`. - Actual: listed as committable. `HACK_DIR_GITIGNORE_ENTRIES` covers `.env` and `.env.state.json` only, so legacy overlay values can be committed. This PR routes overlay values to ignored local layers and does not touch the input files. 5. **The contract migration (`hack.env.json` present) still writes contract-declared plain values to tracked layers** (Low; by design, but worth confirming). - Repro: a contract that marks `DB_PASSWORD` as non-secret, with the value in `.hack/.env`; run `doctor --migrate-env-config`. - Expected: confirm whether plain values should also go to the local layer. - Actual: the value lands in tracked `hack.env.default.yaml` (`buildMigratedValues`). Only keychain-sourced values are encrypted. Refs HACK-1370 ## Round 4 (2026-10-31): reads refuse too, review findings, Codex P1s This round merges `origin/next` (`e42bcf08`, no conflicts) and adds these commits: - `23c8e7ea`: **reads refuse a broken layer** (finding 1 above, now fixed). - Only ENOENT counts as an absent layer. - A selected layer entry that is not a regular file (dangling link, directory, link to a directory) refuses with `E_PATH_CONFLICT`. An unreadable file refuses with one closed line. - This covers `env list`, `env explain` and runtime env resolution, which previously exited 0 and showed "base (.hack/.env only)". - Two tests that pinned the old null fallback were updated: `project-env-metadata` and the #468 directory case. - `69fb8ea0`: reviewer round 1 (one P2, four P3s). - The contract migration decides the format with the mutation check and writes create-only, so it never replaces a dangling layer. - Doctor `env mode` and project metadata report a broken layer instead of "legacy". - The `.env`-only migration refuses non-overlay `.env.<name>` names, skips template and backup copies, and never offers a git-tracked file for cleanup. - Doctor offers files Compose references for cleanup only after the Compose repair removed those references. - Adds a test for an unreadable layer. - `0e732928`: Codex P1 threads. - Machine-local layers are created 0600 and create-only. - Inside a git checkout, each local layer must be git-ignored, so a read-only `.gitignore` refuses instead of only warning. - An unreadable legacy file refuses with `E_PERMISSION` instead of migrating as empty. - `69def4f0` (another session): symlinked legacy overlays are migrated, and cleanup removes only the link. - `ede4cb9b`: refusals name the files, JSON-quoted, and say to select a renamed overlay with `--env`. - `1b1806d1` (another session): refuse a migration whose Compose `env_file` points at a template or backup copy that the migration leaves out. **Mutation proof:** each new guard was disabled on its own and its test failed, then the source was restored and confirmed. That covers the read-path refusal (4 tests), the contract guard, the doctor and metadata classifiers, the unreadable branch, the name refusal, the template skip, the tracked-cleanup filter, the Compose-repair cleanup helper, the 0600 publish, the ignore check, and the unreadable legacy file. **Real-target repro:** `./dist/hack` built from this branch, with a private HOME and synthetic values. Exit codes: | Command | Exit | Result | |---|---|---| | legacy `env set NEW --service api --value-stdin` | 1 | refuses and names the remedy; `.hack/.env` unchanged | | legacy `env set NEW --service 'Bad Scope!'` | 1 | closed scope message | | legacy `env unset KEPT --service api` | 1 | `.hack/.env` keeps `KEPT` | | legacy `env set` with no scope (control) | 0 | still works | | dangling `hack.env.default.yaml`, `env set` | 1 | one-line `E_PATH_CONFLICT` | | dangling `hack.env.default.yaml`, `env list` | 1 | one-line `E_PATH_CONFLICT` (exited 0 with legacy fallback before this round) | | invalid YAML layer, `env list` / `explain` | 1 | one line, no stack | No value appeared in any output. **Review:** one persistent Claude Opus 5.5 reviewer. Round 1 at `23c8e7ea`: CHANGES. Round 2 at `69fb8ea0`: CLEAN. Round 3 at `ede4cb9b`: CLEAN. Codex's four P1 threads are answered and resolved. **Gates:** `bun run typecheck` and `bun run check` pass on Bun 1.4.2. The focused env, doctor, project-env, compose-env and project-meta suites pass, except `doctor-fix-adopted-project`, which fails locally with or without this PR (native adoption state), and `env-selection-parity`, which needs the config compiler. Hosted CI covers both. **Follow-up (not fixed here; already true before this PR):** the contract branch of `migrateLegacyProjectEnv` does not apply the `.env`-only overlay-name rules. With `.hack/hack.env.json`, a `.hack/.env.local`, `.env.example` or `.env.state.json` file is treated as an overlay, so its values can land in the wrong layer. No value is lost. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/hack-dance/codesmith/hack/pr/436?autoLogin=true&ref=codesmith_pr_footer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3-3x.png" width="162" height="28"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1795309313&installation_model_id=17053&pr_number=436&ref=codesmith_pr_footer&repository=hack-dance%2Fhack&return_to=https%3A%2F%2Fgithub.com%2Fhack-dance%2Fhack%2Fpull%2F436&signature=d4db006104551d52ac8f82bada2e90e9bce1aa20066501a5bca05ca148ac30ce"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark-3x.png" width="190" height="28"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> ## Migration refusals added through review (head 49cf488) The `.env`-only migration now refuses, writing nothing, when: - only some Compose services list a legacy file, a service would gain base keys, or a default overlay is not listed while another legacy file is - Compose uses top-level `include`, service `extends` or a variable `env_file` path - a Compose `environment` entry (including `${KEY}`) or another `env_file` sets a migrated key - the Compose file interpolates a key the default overlay changes - an entry is read differently by Compose: unquoted ` #`, `$` outside single quotes, `\` in a quoted value, text after a closing quote, or a line without `=` - `.hack/.env` or an overlay links outside `.hack` The contract migration reads regular overlay files only and refuses `.hack/.env` or a default overlay linked outside `.hack`. Known behaviour: if no service lists a legacy file, the migration proceeds and every service receives its keys once that env is selected, because those keys may only feed interpolation. Doctor names them. Validation at 49cf488: focused env, gitignore, doctor and pre-commit suites pass (env-legacy-service-scope 90, 0 fail); mutation runs fail as expected; independent exact-head review reports no blocking defects, using `docker compose config` on Compose v5.5.0 and not running containers. No native, timing or TLC tests were run. Hosted CI is the check for the full suite. Follow-ups: HACK-1682, HACK-1685, HACK-1688, HACK-1689. ## Further changes through review (head b3f8f00) These supersede the 49cf488 validation above. In addition to the refusals listed there, the `.env`-only migration now refuses: - an `env_file` order that gives a key a different value from the layers - a repeated `env_file` entry, a `format`, a `~` path, a `!reset` or `!override` tag, or `include` with no `services` - a Compose file that isn't one YAML document - any key the default overlay changes whose name appears in the Compose file or another `env_file`, or a `COMPOSE_*` setting - `DOCKER_*`, `BUILDKIT_*`, `BUILDX_*`, proxy settings, `SOURCE_DATE_EPOCH`, `HOME` or `PATH` in a legacy file The contract migration refuses any symlinked `.hack/.env` or default overlay, wherever it points. **Other checkouts.** Once a migration is committed, a teammate's own `.hack/.env` and overlays may stop reaching services. Hack compares those files with what the layers deliver. `hack up` warns, doctor reports it, `hack env materialize` refuses, and `hack doctor --migrate-env-config` moves the values to their local layers. Messages name valid keys only, count malformed entries, and name unreadable files. **Behaviour changes to note:** - `hack env materialize` can now refuse where it used to overwrite. - `hack up` can print a new warning. - `replacePrivateFileAtomic` has a new optional `beforeRename` hook. **Validation at b3f8f00:** - Focused suites: 0 fail (env-legacy-service-scope 141; hack-gitignore, project-env-metadata, project-env-write-refusal-gaps, doctor-fix-noninteractive, doctor-fix-adopted-project, doctor-command, pre-commit-hook, project-env-config, private-env-file, project-env-write-refusals, missing-project-error-code). - Same-reviewer review of this exact head: no blocking defects, using `docker compose config` on Compose v5.5.0 and direct calls to the migration functions. It didn't use running containers, a real `git pull` between checkouts, or the interactive prompts. - Concurrency evidence is deterministic steps through a test seam or a git shim. No timed race was run. As with the shared atomic writer, the last check narrows the window and can't close it. - No native, timing or TLC tests were run. **Hooks:** commits 581da64 through ef9147f were made with `HUSKY=0`, so the repository hooks didn't run on them. commitlint on each message and Biome on the final tree passed separately, which isn't the same as the hooks having run. 190c66b onward used the normal hooks. Added follow-ups: HACK-1686, HACK-1690, HACK-1691, HACK-1692, HACK-1693. ## Final behaviour change (head ca5ef35) This supersedes the "Known behaviour" and b3f8f00 validation above. **Global delivery now needs consent.** Migrated values are global and reach every service, including third-party images. The `.env`-only migration refuses unless every service lists each legacy file with values to migrate. This includes a file no service lists, whose values today only feed Compose interpolation, and a non-default overlay, which must become `env.defaultOverlay` or have its values moved. Listing the file is how a developer accepts this. A value Compose interpolates can't be set per service: keep it in the shell, or list the file. **Also refused:** a migrated base value that would replace a value a tracked overlay sets, when the legacy overlay file doesn't set that key. **Remaining exposure, documented and noted by doctor but not refused:** a service added after migrating receives the global values, and host lifecycle hooks gain them. Tracked in HACK-1692. **Validation at ca5ef35:** - Focused suites: 0 fail (env-legacy-service-scope 150; the other 11 suites as listed above). - `bun run typecheck` and `bun run check`: exit 0. - Same-reviewer review of this exact head: no blocking defects, using `docker compose config` on Compose v5.5.0 and the real `hack doctor` and `hack env` CLI. It didn't use containers, a real `git pull` between checkouts, or interactive prompts. Nothing is a timed race. - No native, timing or TLC tests were run. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Dimitri Kennedy <344842+roodboi@users.noreply.github.com>
PR 436 landed as a squash, so this uses its last head, 3c1bf86, as the merge base. This PR's own changes are unchanged by the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keeps PR 543's keychain error helper. Its generic case now returns SecretStoreKeyUnavailableError, which this PR's shell-value check depends on. A keychain timeout stays its own error and stops the migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
marked this pull request as ready for review
November 3, 2026 00:19
roodboi
added a commit
that referenced
this pull request
Nov 3, 2026
## Summary Doctor now gives a repair instead of saying a checkout is already migrated when linked env layers leave legacy keys undelivered. Its migration plan also recognizes undelivered checkout values. Migration refusals no longer promise that nothing was written: earlier local-layer merges, ignore-rule updates, or failed cleanup can leave writes behind. Legacy sources remain available for retry. Correct stale comments and overlay-only recovery wording, and make the edited-generated-dotenv remedy use `hack env set --local`. ## Findings and decisions HACK-1709: 1. Fixed the unconditional “Nothing was written” claim. The ignore-rule regression failed before the fix; an injected concurrent edit also proves that the first local-layer merge can remain when the second refuses. Rollback remains best effort: this PR corrects the promise rather than introducing a transaction or swallowing a failed migration. 2. Fixed the linked-layer doctor dead-end. A regression first observed `canMigrateCheckoutLegacyDotenv` returning false; it now requires an actionable refusal. The real source CLI prints that repair, and following it allows migration to deliver the legacy value. 3. Updated the migration contract comment for existing layers, merges, and incomplete rollback. 4. Narrowed the “always compared” comment to the names actually inspected. Behavior remains unchanged: reserved, invalid, state, template and backup names are excluded from this diagnostic. 5. Fixed both Compose-conflict remedies to name the legacy file **or overlay that sets the key**, rather than assuming `.hack/.env`. Overlay-only environment and env_file conflict regressions cover both messages. 6. **Keep as defense.** Retain the migration's own format gate, post-write Compose inspection/cleanup calculation, and optional materialization path. Doctor currently passes `materialize:false`, but `migrateLegacyProjectEnv` is an exported API and a future/direct caller must still preserve its preconditions and truthful cleanup accounting. No dead-code removal or behavior expansion here. 7. Moved the misplaced selection comment onto `selectLegacyDotenvOverlays`. 8. Fixed the regular-layer plan mismatch by asking the same checkout migration predicate as execution. Its regression failed before the fix. The unreadable/dangling-contract reader correction originally left with #538 has now landed on next and is preserved in the merge; the plan retains its explicit unreadable-contract refusal. 9. **Already fixed on next; no duplicate change.** `assertLocalBaseIsTheDefaultEnvLayer` refuses the outside-Git `environment: local` case. Existing regressions cover empty base, scoped deletion and successful repair to `environment: default` in `tests/env-legacy-service-scope.test.ts`. 10. **Accept conservative refusal.** A checkout's own local overlay can still cause `assertBaseDoesNotOutrankTrackedOverlays` to refuse even though that overlay would remain above the local base. A regression records the refusal and preservation of both sources. This is a false positive, not value loss; precise post-write precedence modeling is deferred rather than mixed into these recovery fixes. Additional edited-state remedy: `hack env set --local` is now explicit in code and docs. The existing real-CLI recovery regression asserts it and verifies the manual and overlay values survive. ### HACK-1711 — initially deferred to #538; now inherited from next Per Kit's explicit direction, these rows were left with [#538](#538), keeping this PR independent. That PR subsequently landed as `d75ab351` on next. Merge `99ac15549f6d1c4a65f09c567049f0a13ca9ba6a` resolves the resulting conflicts, preserves its contract-specific refusal closings and recovery filters, and uses this PR's truthful fallback closing for other migration refusals. No N1/N2 implementation is duplicated in this PR's diff against next. Historical measurement at the original next base `42357454`: isolated source-CLI fixtures, synthetic values, a private encrypted backend and no keychain fallback. `.env.default` was measured first; its refusal is also a committed regression. All rows printed **no** `--local --value-stdin` recovery command: | Row | Current next behavior | Decision / exact #538 evidence | | --- | --- | --- | | `.env.default` | exit 1, explicit base-collision refusal, no layers written | Defer recovery handling to #538. Its filter rejects selectors that normalize to the base. | | `.env.My_Staging` | exit 0, creates base and named overlay/local layers | Defer to #538; no recovery emitter exists on next. #538 filters the unusable selector and JSON-quotes the filename. | | `.env.foo.bar` | exit 0, creates base and named overlay/local layers | Same deferral; #538 filters and JSON-quotes the filename. | | `.env.example` | exit 0, creates base and example overlay/local layers | Defer template/placeholder recovery semantics to #538. It is a valid unchanged selector, so the proposed parser check alone does not exclude it. #538 now describes the overlay command as an optional later change, not a mandatory recovery step. | | `.env.bak` | exit 0, creates base and bak overlay/local layers | Same backup-file deferral and parser limitation as `.env.example`. | | service `Api` | exit 1, no layers written | Defer recovery handling to #538; its service-name validator suppresses commands for this scope. | | service `"a b"` | exit 1, no layers written | Same deferral; #538 JSON-quotes the unusable service. | The invalid overlay/service rows are now covered by #538's inherited regressions, run again in the merged 329-test suite. **Accepted decision for `.env.example` and `.env.bak`:** these are unchanged valid selectors. Keep the inherited optional “To change it later” command; it is not a required step that pins a placeholder or backup value. Do not add an exclusion the proposed parser rule does not imply. Exact source evidence at inspected #538 head `7afe02ba312672cc6e8c014ab640ddd09e326e1b` (subsequently merged as `d75ab351`): - [Overlay/service filters and JSON-quoted names](https://github.com/hack-dance/hack/blob/7afe02ba312672cc6e8c014ab640ddd09e326e1b/src/lib/project-env-config.ts#L6505-L6547). - [Optional later overlay-change wording](https://github.com/hack-dance/hack/blob/7afe02ba312672cc6e8c014ab640ddd09e326e1b/src/lib/project-env-config.ts#L6556-L6559). - [Invalid overlay and service regressions](https://github.com/hack-dance/hack/blob/7afe02ba312672cc6e8c014ab640ddd09e326e1b/tests/env-contract-shell-values.test.ts#L702-L731). - Current next base-collision guard: [4235745 source](https://github.com/hack-dance/hack/blob/42357454ffe2b10c43874748b2ec41a7d3cb84c1/src/lib/project-env-config.ts#L5197-L5200). **HACK-1709 update for Kit to copy (Linear unavailable in this session):** N1/N2 were deliberately left with #538 rather than making this unit wait on it. #538 subsequently landed on next as d75ab35; merge 99ac155 preserves its recovery filters and custom refusal closings. The merged focused suite, including its contract recovery and no-key-provisioning tests, passes 329 tests. Historical measurements and exact source links are above. Template/backup commands remain optional later changes for valid selectors. — Kit · bot ## Verification - Bun 1.4.2, native Linux. Frozen dependency installation. - Focused env/doctor/worktree suite: 268 pass, 0 fail (three files). Includes pre-fix failures for items 1, 2 and 8. - `bun run typecheck`: pass (CLI executed, DB cache hit). - `bun run check`: pass (34 pre-existing warnings; CLI executed, DB cache hit). - Changed tests/source: Ultracite pass. - `bun run docs:prepare` then `bun run docs:check`: pass. Generated site content stays ignored. - `bun run build` and `./dist/hack --version`: pass. - Prepared pinned Rust config compiler using pinned Zig as linker because host `cc` is absent; `bun run build:config-compiler` then passed. - Independent Codex review (codex-dio, gpt-6-astra), round 1 at `3f0e5bd3d4cde9d7485c31f971668f8c6b9a3166`: no P0–P3 findings, `VERDICT: CLEAN`. Reviewer thread: `thread:delegated-task:command%3Amcp%3A4fb63140-a990-42d9-a4a8-d8753a9af77f%3Adelegate-task%3A49487a98-88b7-421d-9388-67ba6d9aa11b`; completed and settled. No review fixes were required. - Gitleaks: one-commit range `origin/next..HEAD`, pass. - Local full-suite limitation: the first Linux run (umask 002) failed 852 tests. A second run under umask 022 encountered baseline MCP timeouts and inode-control failures, then ended without a final status available from its command session. It does not qualify the merged head. An untouched archive of next `d75ab351` reproduced all six targeted failures: three inode-control failures and three MCP timeouts (50 pass, 6 fail). Putting the pinned Bun 1.4.2 binary first on PATH fixes all three MCP tests (3/3 pass); the inherited machine-heavy Bun shim was unsuitable for these private child environments. No native test assertions were weakened or changed here. - CI at `3f0e5bd3`: shard 3 found an old doctor test still requiring the item-2 dead-end. Test-only commit `3d04409c6f7784782141d571a94e9ab75ff466f5` now requires refusal and preserves every link, target, legacy-file and missing-layer assertion. `bun test tests/doctor-fix-noninteractive.test.ts`: 27 pass, 0 fail. Root check, privacy check, changed-test lint and two-commit Gitleaks scan pass. No implementation changed after round 1. - Hosted CI at `3d04409c`: all four test shards and the `test` fan-in pass. Some other jobs remain queued/running. `computer pr review-status hack-dance/hack 555`: `CLEAR at 3d04409`, no unresolved bot threads or change requests; only the Linear linkback comment exists and was read. Final whole-CI status and local full-suite result remain pending. Round 2 at `3d04409c6f7784782141d571a94e9ab75ff466f5`: no P0–P3 findings, `VERDICT: CLEAN`; all seven files and surrounding gates reviewed. Thread: `thread:delegated-task:command%3Amcp%3A4fb63140-a990-42d9-a4a8-d8753a9af77f%3Adelegate-task%3A2d3a5a3d-3d05-4805-9eb6-ab6626c31591`, completed and settled. T3 requires a new delegate task for each recheck, so round 2 carried the original review context rather than resuming the original child. ## Release Signal - Commit / squash title: `fix(env): env-migration follow-ups (HACK-1709, HACK-1711)` - Release intent: `fix` - Should this PR trigger a release signal? `yes`; follows the repository's release workflow from the squash title. ## Semantic Surfaces - Env migration/doctor guidance changes: yes. No new flags or env-resolution ordering changes. - Targeted tests: `tests/env-legacy-service-scope.test.ts`, `tests/doctor-command.test.ts`, `tests/project-env-local-inheritance.test.ts`. - Docs: `docs/env.md`. ## Risks / Follow-up This PR intentionally retains item 10's conservative refusal and does not add transactional rollback. N1/N2 and the unreadable-contract correction are inherited from the now-merged #538. No merge or release is authorized; Kit merges after required gates. Pending project writeback: the inherited umask setup finding targets `.ai/skills/hack-repo-verify/SKILL.md`. `fclt ai writeback add` was attempted but refused the existing project asset as absent from its graph, even with an explicit project root. Nothing was recorded or globally promoted. Merge verification at `99ac1554`: merged env/contract/doctor/worktree suite 329 pass, 0 fail; typecheck, root check, docs prepare/check, compiled CLI build/--version, privacy check and Gitleaks pass. The doctor-fix file also passes 27/27 on the merged head. Round 3 at `99ac15549f6d1c4a65f09c567049f0a13ca9ba6a`: `VERDICT: CLEAN`, no P0–P3 findings. Reviewer confirmed custom closings, both doctor gates and both docs changes survive. Thread: `thread:delegated-task:command%3Amcp%3A4fb63140-a990-42d9-a4a8-d8753a9af77f%3Adelegate-task%3Aa42901d2-e2f9-492e-a8a4-8d4d350c579b`, completed and settled. Fresh CI is pending; bot status is CLEAR at 99ac155 with no unresolved bot threads or change requests. --------- Co-authored-by: Kit <344842+roodboi@users.noreply.github.com>
roodboi
added a commit
that referenced
this pull request
Nov 3, 2026
) **Update at 7bd4fa0:** merged `next` with #538 (conflicts in `doctor.ts`, `project-env-config.ts`, `secret-store.ts` and #538's shell-value test). The keychain-disabled refusal is now a `SecretStoreKeyUnavailableError`, which #538's check depends on. One behaviour change: with the keychain disabled, no key and no store file, a run counts as first use and creates a key file, as on `main` (#423). On `next` before this merge it stopped with a refusal, so I removed the one step of #538's test that asserted that stop. The #538 test also restores a preloaded `HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK` instead of deleting it. Focused env, secret-store and doctor tests pass on the pinned Bun 1.4.4. Details on HACK-1758. **Update at ede96a8:** merged `next` with #543 (conflicts in `secrets.ts` and `secret-store.ts`); the shared keychain now gates first and then uses #543's deadline-bounded calls, with a new test for that order. **Update at 03a0c75:** merged `next` with #436 (one conflict, in doctor's env materialization check, kept both sides); focused env, doctor and MCP tests show no new failures on Bun 1.4.4. ## Update at b2ca3cd: review findings at 30f2b1b This note replaces the older text below where they disagree. `next` had not moved, so there was nothing new to merge. - **Relative overrides in MCP sessions:** a session now refuses only a relative `HACK_HOME`. A relative `HACK_GLOBAL_CONFIG_PATH` resolves against the captured cwd again, which matches child commands because they run in that cwd. The previous note's refusal of that setting was a mistake and is reverted: it made the server refuse what the CLI and its children accept. - **Guard seam:** the fixture case now says it uses the seam. The seam answers only the next read, so it can't stay set for later files, and a new nested test checks that a later file gets the real reader back. The outer test now says it shows detection and reporting, not that the repair (`disableNativeProxies`) works. **Verified here** (private HOME and HACK_HOME, `HACK_DAEMON_AUTOSTART_DISABLE=1`): `bun run typecheck`, `bun run check`, `bun run docs:check`, gitleaks, and focused tests: 264 pass, 61 skip, 0 fail across 31 files, all `*mcp*` files included. | Mutation | Result | | --- | --- | | Refuse a relative `HACK_GLOBAL_CONFIG_PATH` again | 2 tests fail | | Resolve a relative `HACK_GLOBAL_CONFIG_PATH` against the process cwd | 2 tests fail | | Resolve a relative `HACK_HOME` against the session cwd | 3 tests fail | | Seam stays set | The guard test fails | **Not rerun:** the whole `bun run test` suite on this head; hosted CI is the check. --- ## Update at 30f2b1b: review findings at 676bf9b This note replaces the older text below where they disagree. Merged current `next` (`f94b17ff`, including #547) with a merge, not a rebase. - **`ffc49b33` Relative `HACK_HOME`:** `captureMcpSessionContext` now refuses a relative `HACK_HOME` or `HACK_GLOBAL_CONFIG_PATH` with the CLI's error wording. The earlier "MCP sessions still resolve it" note no longer applies. A socket client with such a context loses only its own session. The CLI itself still doesn't check a relative `HACK_GLOBAL_CONFIG_PATH`; only MCP sessions refuse it. - **`71975bb6`:** dropped `logs` from the commands that record branch usage, in `docs/cli.md` and `src/lib/branch-usage.ts`. - **`30f2b1bf` Guard positive case:** this is a seam, not a real leak. Under Bun 1.4.4, these routes left no native change that `process.env` hides: set and delete, `loadEnvFile`, a Worker, libc `setenv`, and a replaced env object. So the fixture hands the guard's native reader a changed environment. It tests the boundary check and its report, not a real Bun leak. - **`30f2b1bf` SDK comment:** reworded in `tests/mcp-tool-result-size.test.ts` for the pinned SDK 1.32. - **`RELEASE_PAT`:** stays on `actions/checkout` in `release.yml` and `release-prepare.yml` (`676bf9b7`). The earlier note about `useblacksmith/checkout` receiving it and the 9-step count no longer apply. **Verified here** (private HOME and HACK_HOME, `HACK_DAEMON_AUTOSTART_DISABLE=1`): - `bun run typecheck`, `bun run check`, `bun run docs:check` and gitleaks over `origin/next..HEAD` pass. - Focused tests: 325 pass, 61 skip, 0 fail across 35 files, all `*mcp*` files included. `tests/mcp-tool-result-size.test.ts` passes (13). - A real `hack mcp serve` start with either relative override exits 1 with the error. - Mutation, session rule restored to cwd-relative: 4 tests fail. Mutation, guard's native comparison disabled: the guard test fails. **Read, not run:** `checkByteCutoff` in `tests/e2e/scenarios/mcp-logs-compose.ts` should still hold. Events are parsed from the capture, so they total about 8 MiB and fit under the 9.5 MiB result limit. No event is dropped, so the reported cutoff stays `{bytes, 8 MiB}`, and #547 only trims `stdout` on a code point boundary. Docker wasn't used; hosted CI runs this scenario. **Not rerun:** the whole `bun run test` suite on this head; hosted CI is the check. --- **Current head: `676bf9b7`.** It merges `next` at `ddc73a0d` and keeps `RELEASE_PAT` on `actions/checkout@v4` in `release.yml` and `release-prepare.yml`. `main` passes that credential to `useblacksmith/checkout@v1`, a third-party action on a floating tag. Doing the same on `next` is a security decision of its own, so it isn't part of this port. All other checkout steps ported here stay on Blacksmith. This changes the "`RELEASE_PAT` now goes through `useblacksmith/checkout`" point below. --- Refs HACK-1758. Forwards the security pins and fixes from `main` to `next`, one topic per commit. `main` is not merged. Each commit was checked by reading `next` and, where `main` has a regression suite, by running that suite against unmodified `next`. ## Classification | `main` commit | What it does | Result | Evidence on this branch | | --- | --- | --- | --- | | `69ed70e9` #457 | Bun 1.4.4 and Node 24.22.1 pins | **Ported**, with the exceptions below | `2c84aac3`: all workflows including `prerelease.yml`, `.tool-versions`, `mise.toml`, `packageManager`, Dockerfiles, candidate build guards, e2e and pattern fixture tags | | `b5d3212a` HACK-1404 | Delete unset env vars instead of assigning `undefined` | **Already present** | `tests/helpers/env.ts:12` (`restoreEnv`, used by 109 test files). No `process.env.X = undefined` remains, and every restore of a value that may be undefined is guarded. Bun 1.4.4 did change proxy deletes, handled in `2852393c` | | `395351bc` #454 | Dependabot `bun` ecosystem | **Ported** | `f97ca462`: `.github/dependabot.yml`, same as `main`. GitHub reads this file from the default branch, so it has no effect until `next` lands | | `739c4e57` #456 | next, MCP SDK, drizzle-orm bumps | **Ported** | `dd92fae0`: MCP SDK 1.26.0 to 1.32.0, drizzle-orm ^0.44.5 to 0.45.2, next 16.2.6 to 16.4.1 in `examples/next-app`; `site/` already has next 16.4.3 | | `75225e00` #513 | zod 4.6.7 (GHSA-mrhx-c7xc-m3w5) | **Ported** | `dd92fae0`: zod 4.3.5 to 4.6.7; `site/` already has it | | `4c194a12` #423 | Hermetic keychain tests | **Ported** | `80abd5fc`: `src/lib/keychain.ts`, `tests/preload.ts`, `tests/keychain-guard.test.ts`. The fake is not frozen, because `next`'s tests spy on it | | `3312658f` #526 | LogQL escaping and empty Loki follow hint | **Already present** | `src/lib/logs.ts:62`, `:91` and `src/ui/loki-logs.ts:442`, from #417 and #478. `main`'s `tests/loki-tail.test.ts` passes unchanged on unmodified `next` | | `98ab4e70` #527 | No global state under the cwd for an empty or relative HOME/HACK_HOME | **Ported** and extended | `ac12697c`: `src/lib/home-dir.ts`, plus the same bug in `next`-only code: MCP session capture, launchd access, native AWS environment, setup migrate | | `65e4d97b` #528 | Fail and sweep on leaked e2e Docker networks | **Ported** | `8a79897a`, `32cd2edf`: `tests/e2e/docker-ownership.ts`, harness and `run.ts` | | `736544ef` #529 | Inherit primary local env overlays in linked worktrees | **Partly present; the gaps are ported** | `next` already inherited (`resolvePrimaryLocalProjectDir`). 24 of the 36 cases in `main`'s test failed on unmodified `next`. `40d0df6f`, `e4837dca` add worktree-family verification, the env opt-out, fail-closed reads, YAML routing and doctor's obsolete report | | `83d71ac7` #530 | Keep branch usage out of the tracked tree | **Ported** | `eb30178f`, `d925c5ef`: `src/lib/branch-usage.ts`; `touchBranchUsage` is gone | | `541c611b` #520 | `useblacksmith/checkout` on Blacksmith Ubuntu runners | **Ported** | `f97ca462`: 9 steps across `ci`, `release`, `release-prepare`, `release-node-runtime-image`. Hosted and macOS jobs keep `actions/checkout`, as on `main` | | `1c1cc983` #521 | Resize three runners | **Left out: needs a decision** | The numbers come from `main`'s single `docker-e2e` job, and `next` shards that job. The original PR itself marks the sizes as unbenchmarked, and it adds cost | | `08e0412a`, `2e8dc61d`, `91b14bf0`, `60f3c0b9`, `010880c7`, `5e03d47e` | 4.2.x releases | **Not applicable** | Excluded by the issue; `next` has its own release line | | `2dd3853b` #512 | Remove private names from a sample digest | **Ported, not on the issue's list** | `25dfed3e`: `next` is public and still carried these names. Drop this commit if you want it handled separately | ## Pins that stay on the old version - **Native file and storage helper, `oven/bun:1.4.2-slim`** (`src/lib/native-compose-vm-file-protocol.ts:6`, `src/lib/native-compose-storage-witness-docker-artifact.ts:19`, `:66`). The catalog pins exact image, Bun and libc bytes per platform, and `scripts/generate-native-storage-witness-helper.ts` says changed artifacts need separate runtime qualification. Moving it is a product decision. Evidence: the helper bundle reproduces byte for byte under Bun 1.4.4, and its source has no Brotli, zlib or fetch use. Bun 1.4.2 is still in that image. CI now pulls both images, and `docs/reference/native-config-files.md` still names 1.4.2-slim correctly. - **Frozen benchmark workload**: `scripts/application-benchmark-protocol.json` and `scripts/check-application-benchmark-protocol.py` freeze this pin as part of the protocol, and `scripts/benchmark-native-compose.py` defaults to the same image. No Bun from these ships. - **Records of past runs**: the observed client version in `src/agents/client-capabilities.ts:501`, the `docs/versions/` snapshots, `scripts/test-shard-durations.json`, and comments describing 1.4.2 behaviour. The capability record still says MCP SDK 1.26.0 and needs a new observation. - **Test fixtures** that use `1.4.2` only as sample data. ## Behaviour to review - A relative `HACK_HOME` is now an error in the CLI. MCP sessions still resolve it against the captured checkout, because `tests/mcp-session-context.test.ts` pins that as `next`'s contract. - `HACK_WORKTREE_INHERIT_LOCAL=0` applies to the Compose path. The graph runner follows only `worktree.inherit_local`, because its adoption proofs record that setting. `docs/cli.md` says so. - A `.git` link that cannot be verified, such as a bare-repo worktree, now reads no primary overlays instead of failing the command. - Two expectations in `main`'s `project-env-worktree-local` test keep `next`'s behaviour: the layer guard's refusal for a symlink leaving the project, and the tombstone from `env unset --local`, which hides an inherited value. - `tests/global-state-guard.test.ts` changed because Bun 1.4.4 now carries a `process.env` delete into its native environment. That test was written to fail on this upgrade. It now asserts the new behaviour and fails if the old one returns. The guard's native check has no reachable leak under 1.4.4 and stays as a regression check. - `useblacksmith/checkout` now receives `RELEASE_PAT` in `release-prepare.yml` and `release.yml`, as on `main`. No permissions or secrets changed. - The private names removed by `25dfed3e` remain in git history. This PR does not rewrite history. ## Verified Local, on macOS arm64 with Bun 1.4.4 from mise. All tests ran with a private temporary HOME and HACK_HOME, `HACK_DAEMON_AUTOSTART_DISABLE=1`, and a `DOCKER_HOST` pointing at a socket that does not exist. - `bun install --frozen-lockfile`: pass. - `bun run typecheck`, `bun run check`, `bun run docs:check`: pass on the head. - gitleaks 8.30.1 over `origin/next..HEAD`: no leaks. - `.hack/tasks.json`: the method first reproduced the old `mise.toml` pin and all three `package.json` script pins. Only `mise.toml` needed re-pinning, and `tests/project-task-catalog.test.ts` passes. `tests/integration-ownership.test.ts` passes with no fingerprint change. - Focused tests on the head: 446 pass and 5 fail across 24 files. All 5 are in `tests/doctor-fix-noninteractive.test.ts` and are described below. `bun run test` ran once, before the last three fix commits and before rebasing onto #542: **8505 pass, 127 skip, 27 fail** across 598 files. | Failures | Cause | Also on unmodified `next`? | Now | | --- | --- | --- | --- | | 20 in `native-compose-ingress`, `native-compose-proxy-probe` | My private `DOCKER_HOST` | Yes, on Bun 1.4.2 and 1.4.4 | All 25 tests in those files pass without that variable | | 3 in `global-state-guard` | Bun 1.4.4 proxy delete change | No: they fail on `next` with 1.4.4, pass with 1.4.2 | Fixed in `2852393c`, passing | | 1 each in `project-env-local-inheritance`, `cli-spawn-env-isolation`, `e2e-docker-ownership` | This PR's ports | No | Fixed in `e4837dca`, `32cd2edf`, passing | | 1 in `native-compose-route-claims` (`Unexpected end of JSON input`) | Not established. The host was under heavy load and this PR does not touch that code | Not reproduced there | Passed 5 of 5 focused reruns on this branch | The 5 `doctor-fix-noninteractive` failures appear only when that file runs alone under plain `bun test` with this isolation. Unmodified `next` fails the same 5 the same way, and they passed in the full run. ## Not verified - Hosted CI, including the Linux jobs and the sharded `docker-e2e`. No Docker scenario ran locally, so the 1.4.4 and 24.22.1 image tags, `docker-ownership` against a real engine, and `worktree-parallel-up` are unproven until CI runs. - Docker image builds, the release workflows and Blacksmith checkout behaviour. - Whether CVE-2026-111497 is reachable in Hack's use. - The whole suite on the final head; CI is the check for that. - The issue's other done criteria: a green merged `next`, a published prerelease, and the published build reporting the new pin. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- codesmith:footer --> --- <a href="https://backend.blacksmith.sh/track/run-proof?expires=1796240682&installation_model_id=17053&pr_number=545&ref=codesmith_pr_footer&repository=hack-dance%2Fhack&return_to=https%3A%2F%2Fgithub.com%2Fhack-dance%2Fhack%2Fpull%2F545&variant=run_proof_on_this_pr&signature=cfadffa3b85fba4f3f8041d26088dbdafce9792afdc357fbc9c8ba4cec915168"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-light.svg"><img alt="Run Proof on this PR" src="https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-dark-3x.png" width="168" height="28"></picture></a> <sup><a href="https://app.blacksmith.sh/hack-dance/codesmith/hack/pr/545?autoLogin=true&ref=codesmith_pr_footer">Open in [code]smith</a> · Need help on this PR? Tag <code>@codesmith-bot</code> with what you need.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> --------- Co-authored-by: Dimitri Kennedy <344842+roodboi@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
![https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source](https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img](https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-dark.svg"><source](https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-dark.svg"><source){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-light.svg"><img](https://pr-comments-assets.blacksmith.sh/codesmith/proof-run-proof-on-this-pr-light.svg"><img){kind=link}
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.
Head:
93460bcbf30f6b7c46796114991d0d735b521e37. Merged with #436 at2bb42712, which includesnextatae032e79(#539).At this head:
.env.defaultrefuses even when it adds no value of its own, kept fromnext.deliveredValuescomes from this PR'soverlaysResolvedvalues.Verified locally: typecheck, and 501 focused tests pass on this combination. Gitleaks passes over
10e55e2f..93460bcb.Not verified yet: hosted CI at this head was still running when this was written.
Not reviewed yet: the merge forward and the #436 changes it brings in have had no independent review.
Text below describes earlier heads.
Draft, stacked on #436, and held for preservation risks. Don't merge before #436; retarget to
nextand rebuild once #436 lands.Head:
10e55e2f4fad0002b62a9358871f8787c242d12b. Merged with #436 ateaa083dc, which includesnextatf938d629(#537).Summary
plain_envvalue that came only from your shell into a tracked layer. It now refuses, naming the key and never the value, before Hack provisions any secret-store key, and prints scope-preserving recovery commands.SecretStoreKeyUnavailableErrorlets that case be told apart from other backend failures, which still stop the migration.hack.env.jsonit can't open or parse; an overlay nameddefault,localor<name>.localin any case that sets its own value; and a service that isn't a valid env scope when a value would be written for it. Onnext, these overwrote the base value, wrote the machine-local path, wrote an unparseable layer, or, for a malformed contract, wrote an empty layer and let doctor delete.envand the contract.--env baseor--env defaultwould select no overlay and replace the base value, so those names, and others--envor--servicecan't take, are reported instead.Verification
Local, Bun 1.4.2, one file at a time, empty environment, stubs first on PATH, private HOME and TMPDIR.
Bun.secretsis stubbed where the keychain branch runs; no real keychain or credential by design. All values are synthetic.15ecf62d:env-contract-no-key-provisioning4,env-contract-shell-values21,secret-store32,env-backend-command19,env-legacy-service-scope193,cli-spawn-env-isolation7,project-env-symlink-boundary28,doctor-fix-noninteractive22. At5ebbf6b2, which only changes a comment: the first two again.Mutation control. Each mutant is one source line in its own checkout, with exports kept, so tests load normally:
Errorat the no-provisioning siteErrorat the disable-flag siteThe existing tests set
HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK, which stops before any provisioning code, so they couldn't show this. That's why the new tests exist.At
9dfd095e:env-contract-shell-valuesandenv-contract-no-key-provisioning54 together,secret-store32,env-backend-command19,env-legacy-service-scope193,project-env-config58,project-env-symlink-boundary28,doctor-fix-noninteractive22,project-env-write-refusal-gaps28 with 1 skip,cli-spawn-env-isolation7,daemon-autostart-fixture-guard7. Each new set of refusal tests was first run against the source before its repair and failed. The recovery controls passed before any repair, so that behavior was already correct and only untested.At
2b2352fa: tests for a contract that can't be opened, through the real CLI for all three forms, with controls. They nameenv addonly;exec,hydrate, session andsetwithout--localrest on a scratch harness, not a named test.HACK-1716,
2b2352fa..99ee84f4: for a contract that can't be read or parsed, doctor now says it can't be read, the plan step andenv modecheck say the migration refuses, and doctor goes straight to the refusal without asking. Behaviour change: an interactive user can no longer decline at that prompt and carry on to the later fix steps. Non-interactive runs already refused. Each new test failed on the source before its repair. Atad6f0590: 333 pass, 0 fail across shell-values, no-provisioning, env-backend-command, daemon-env, doctor-fix-noninteractive, doctor-command and env-legacy-service-scope. At99ee84f4: shell-values and doctor-command, 81 pass, 0 fail. Biome, typecheck and gitleaks pass.Combination with #436's scoped-value refusal: git auto-merged with no conflicts. Typecheck passed before each merge commit. Exact combination
10e55e2f: 462 pass, 0 fail across env-set-local-contract, env-exec-command, project-env-metadata, write-refusal-gaps, write-refusals, shell-values, no-provisioning, env-backend-command, doctor-command, env-legacy-service-scope and local-inheritance. The previous merge31ded153: 480 pass, 0 fail across its focused list. An unopenable contract refuses before the no-contract route, so the two sides' checks don't meet.Independent source review (one reviewer, reading only) covered through
ad6f0590and the merge31ded153, with no blocking finding. The later merge10e55e2fand99ee84f4weren't read; the first only brings in #436's test-onlyeaa083dc.99ee84f4only renames a test, drops an assertion that could no longer fire and removes a comment, and it wasn't read. This is not an approval. Not run locally: the full suite. CI at this head is the evidence for it. CI at earlier heads:2b2352farun 41365434498 succeeded.99ee84f4failed once onCandidate core (macos-latest)(HACK-1718, open, cause not established), and its retry succeeded per root.Release Signal
fix(env): refuse a contract migration that would commit a shell valuefixyesyes, include the required changeset or equivalent release artifact in this PR. Follows the repository's release workflow from the squash title.Semantic Surfaces
hack run,hack exec, env resolution, runtime-state reconciliation, or lifecycle shell/process behavior?yes. Commands that migrate implicitly, such ashack env exec, now refuse with a shell-only value.tests/env-contract-shell-values.test.ts,tests/env-contract-no-key-provisioning.test.ts.docs/env.md,docs/security/env-secrets-threat-model.md.Risks / Follow-up
Why this is a draft, open items:
hack doctor --migrate-env-config --no-interactivedeletes.envand untracked.env.bakand.env.exampleholding values no layer received. It reproduces onnextwith Docker up (synthetic values, not evidence of real loss). fix(env): refuse --service and unusable layers in legacy env set/unset #436 extends it to Docker down. Owner: Kit; not fixed here. Handoff posted on the ticket, not yet accepted.Product contracts, undecided and unchanged here:
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.