Repository navigation
fix(env): env-migration follow-ups (HACK-1709, HACK-1711) - #555
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
canMigrateCheckoutLegacyDotenvreturning false; it now requires an actionable refusal. The real source CLI prints that repair, and following it allows migration to deliver the legacy value..hack/.env. Overlay-only environment and env_file conflict regressions cover both messages.materialize:false, butmigrateLegacyProjectEnvis 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.selectLegacyDotenvOverlays.assertLocalBaseIsTheDefaultEnvLayerrefuses the outside-Gitenvironment: localcase. Existing regressions cover empty base, scoped deletion and successful repair toenvironment: defaultintests/env-legacy-service-scope.test.ts.assertBaseDoesNotOutrankTrackedOverlaysto 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 --localis 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, keeping this PR independent. That PR subsequently landed as
d75ab351on next. Merge99ac15549f6d1c4a65f09c567049f0a13ca9ba6aresolves 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.defaultwas measured first; its refusal is also a committed regression. All rows printed no--local --value-stdinrecovery command:.env.default.env.My_Staging.env.foo.bar.env.example.env.bak.env.example.Api"a b"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.exampleand.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 asd75ab351):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 run typecheck: pass (CLI executed, DB cache hit).bun run check: pass (34 pre-existing warnings; CLI executed, DB cache hit).bun run docs:preparethenbun run docs:check: pass. Generated site content stays ignored.bun run buildand./dist/hack --version: pass.ccis absent;bun run build:config-compilerthen passed.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.origin/next..HEAD, pass.d75ab351reproduced 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.3f0e5bd3: shard 3 found an old doctor test still requiring the item-2 dead-end. Test-only commit3d04409c6f7784782141d571a94e9ab75ff466f5now 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.3d04409c: all four test shards and thetestfan-in pass. Some other jobs remain queued/running.computer pr review-status hack-dance/hack 555:CLEAR at 3d04409c6f, 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 at3d04409c6f7784782141d571a94e9ab75ff466f5: 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
fix(env): env-migration follow-ups (HACK-1709, HACK-1711)fixyes; follows the repository's release workflow from the squash title.Semantic Surfaces
tests/env-legacy-service-scope.test.ts,tests/doctor-command.test.ts,tests/project-env-local-inheritance.test.ts.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 addwas 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 at99ac15549f6d1c4a65f09c567049f0a13ca9ba6a: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.