Skip to content

fix(env): env-migration follow-ups (HACK-1709, HACK-1711) - #555

Merged
roodboi merged 3 commits into
nextfrom
kit/hack-1709-1711-env-followups
Nov 3, 2026
Merged

roodboi merged 3 commits into
nextfrom
kit/hack-1709-1711-env-followups

Conversation

@roodboi

@roodboi roodboi commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

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 fix(env): refuse a contract migration that would commit a shell value #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, 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):

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 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 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.

@linear-code

linear-code Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

HACK-1709

HACK-1711

@roodboi
roodboi merged commit 31b497c into next Nov 3, 2026
23 checks passed
@roodboi
roodboi deleted the kit/hack-1709-1711-env-followups branch November 3, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant