Repository navigation
fix(env): contract migration keeps .hack/.env values machine-local (HACK-1712, HACK-1713) - #542
Merged
Merged
Conversation
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
The Codex cloud review could not run because its code-review usage limit was reached; it produced no inline findings. The separately authorized persistent codex-dio GPT-6-Astra reviewer reviewed fd64d3e and found two P1 issues. Commit 637bb8d fixes both: effective ignore protection is now required before plaintext writes, and every tracked/local destination is checked for aliases before any migration writes, including compatibility paths and case aliases. Both new Doctor regressions fail against fd64d3e and pass with the fix. The compatibility-path regression also fails on that head and passes now. Source project-env tests pass (60); the rebuilt CLI passes all five targeted Doctor cases, including HACK-1712/HACK-1713 and the new ignore/alias failures. Typecheck, repository check and changed-file lint pass. Exact-head re-review is in progress in the same reviewer. Hosted CI owns the broad suite under the host memory-pressure constraint. — Kit · bot |
Contributor
Author
|
Independent codex-dio GPT-6-Astra re-review is VERDICT: CLEAN at exact PR head The same persistent reviewer confirmed both previous P1 findings are fixed and independently reran the original probes: the read-only outdated ignore file and overlay destination collision now produce value-free refusals. No remaining P0–P3 findings. The reviewer also checked case aliases, compatibility destinations, plaintext routing, secret encryption, #539 retention and legacy The cloud Codex usage-limit notice remains an unavailable cloud review, rather than an approval; the separately authorized independent review above supplies the requested code review. Hosted CI is still running. No merge performed. — Kit · bot |
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/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.
Contract migration previously copied
plain_envvalues from.hack/.envinto trackedhack.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 --localwriting dotenv until migration. Modern--localwrites 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:
set --localfollowed by a new contract. They scan stdout/stderr and every staged file aftergit add, check the ignored destination, and require key/path reporting.nextat 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 deliversLOCAL_KEYlocally, has no value in output or tracked files, and passesgit check-ignore.Fixes HACK-1712 and HACK-1713. Release intent:
fix; no merge or release publication performed.— Kit · bot