diff --git a/docs/env.md b/docs/env.md index 411252423..ce271e187 100644 --- a/docs/env.md +++ b/docs/env.md @@ -1046,6 +1046,17 @@ That migrates the repo into: - `.hack/hack.env..yaml` - `.hack.secret.key` when needed +Doctor offers legacy dotenv files for cleanup only when every parsed entry was +delivered to an env layer. Files containing undeclared or undelivered entries stay +on disk; the cleanup report lists their key names and omits their values. This +also applies to untracked overlays such as `.env.bak` and `.env.example`. +Migration refuses a legacy `.env.default` overlay before writing env layers, +because it would overwrite the base `hack.env.default.yaml` layer. Rename that +legacy overlay and update its project references before retrying. +If a legacy dotenv file cannot be read, migration refuses cleanup and leaves +legacy artifacts in place. Env layers may already have been written; check read +access and preserve the remaining entries before cleaning up manually. + `hack doctor` also warns when a repo still depends on the old format. For modern env repos, `hack doctor` also warns when materialized compatibility output in `.hack/.env` or `.hack/.env.state.json` is stale and points to `hack env materialize`. diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 55e0fce7b..ed380174a 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -3546,12 +3546,18 @@ async function runDoctorFix(opts: { await maybeInstallMutagenForDoctorFix(); await maybeUntrackGeneratedFiles({ startDir: opts.startDir }); - await maybeEnsureHackDirGitignore({ startDir: opts.startDir }); + // Let env migration refuse before creating an unrelated missing gitignore. + if (!opts.migrateEnvConfig) { + await maybeEnsureHackDirGitignore({ startDir: opts.startDir }); + } await maybeRemovePlaintextComposeEnvOverride({ startDir: opts.startDir }); await maybeRepairHackd(); const dockerOk = await dockerInfoOk(); if (!dockerOk) { + if (opts.migrateEnvConfig) { + await maybeEnsureHackDirGitignore({ startDir: opts.startDir }); + } note( "Docker is not reachable; daemon repairs were still applied, but Docker-backed fixes were skipped.", "doctor" @@ -3587,6 +3593,7 @@ async function runDoctorFix(opts: { await maybeMigrateDnsmasq(); if (opts.migrateEnvConfig) { await maybeMigrateProjectEnvConfig({ startDir: opts.startDir }); + await maybeEnsureHackDirGitignore({ startDir: opts.startDir }); } renderSkippedDoctorFixSteps(); @@ -3728,6 +3735,13 @@ async function maybeMigrateProjectEnvConfig(opts: { } note(migrationNotes.join("\n"), "doctor"); + for (const retained of migrated.retainedEnvFiles) { + note( + `Kept ${retained.path}: keys not preserved in env layers: ${retained.keys.join(", ")} (values omitted).`, + "env cleanup" + ); + } + let blockedCleanupCandidates = [...migrated.blockedCleanupCandidates]; if (migrated.composeEnvFileReferences.length > 0) { note( diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 7d44a6310..efe53b23f 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -39,6 +39,7 @@ import { mergeManagedGitignoreBlock, pathExists, readAtomicWriteTargetText, + readOptionalTextFile, readTextFile, writeTextFile, writeTextFileAtomicIfChanged, @@ -4898,6 +4899,10 @@ export async function migrateLegacyProjectEnv(opts: { readonly cleanupCandidates: readonly string[]; readonly blockedCleanupCandidates: readonly string[]; readonly composeEnvFileReferences: readonly LegacyComposeEnvFileReference[]; + readonly retainedEnvFiles: readonly { + readonly path: string; + readonly keys: readonly string[]; + }[]; }> { await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); @@ -4911,6 +4916,7 @@ export async function migrateLegacyProjectEnv(opts: { cleanupCandidates: [], blockedCleanupCandidates: [], composeEnvFileReferences: [], + retainedEnvFiles: [], }; } @@ -4923,12 +4929,24 @@ export async function migrateLegacyProjectEnv(opts: { const overlayNames = await discoverLegacyOverlayNames({ projectDir: opts.projectDir, }); + if (overlayNames.includes("default")) { + throw new ClosedCliError( + "Refusing legacy env migration: .env.default would overwrite the base hack.env.default.yaml layer. Rename the legacy default overlay and update its project references before retrying. No env migration files were written." + ); + } const keyInfo = baseResolved.values.some( (value) => value.source === "keychain" ) ? await ensureProjectEnvSecretKey({ projectRoot: opts.projectRoot }) : null; + // An overlay may inherit an equal base value without writing its own entry. + // Keep the full resolved delivery per source file, including inherited values. + const deliveredValues = new Map>(); + deliveredValues.set( + resolve(opts.projectDir, PROJECT_ENV_FILENAME), + new Map(baseResolved.values.map((entry) => [entry.key, entry.value])) + ); const wroteFiles: string[] = []; const defaultConfig = buildMigratedProjectEnvConfig({ environment: "default", @@ -4958,6 +4976,10 @@ export async function migrateLegacyProjectEnv(opts: { projectName: opts.projectName, envName: overlayName, }); + deliveredValues.set( + resolve(opts.projectDir, `${PROJECT_ENV_FILENAME}.${overlayName}`), + new Map(overlayResolved.values.map((entry) => [entry.key, entry.value])) + ); const overlayConfig = buildMigratedProjectEnvConfig({ environment: overlayName, values: buildMigratedValues({ @@ -5013,6 +5035,7 @@ export async function migrateLegacyProjectEnv(opts: { overlayNames, materialize: opts.materialize, configCleanupCandidates: configCleanup.cleanupCandidates, + deliveredValues, composeEnvFileReferences, }); @@ -5023,6 +5046,7 @@ export async function migrateLegacyProjectEnv(opts: { updatedProjectConfig: configCleanup.changed, cleanupCandidates: cleanupCandidates.allowed, blockedCleanupCandidates: cleanupCandidates.blocked, + retainedEnvFiles: cleanupCandidates.retainedEnvFiles, composeEnvFileReferences, }; } @@ -5138,16 +5162,37 @@ function buildMigratedValues(opts: { return values; } +/** Cleanup requires readable contents, even when legacy resolution treated a read failure as absence. */ +async function readLegacyDotenvCleanupEntries(opts: { + readonly path: string; +}): Promise>> { + const source = await readOptionalTextFile(opts.path); + if (source.kind !== "text") { + throw new ClosedCliError( + `Refusing legacy env cleanup: could not read ${opts.path}. No legacy files were removed; env layers may already have been written. Check read access before cleaning up manually (values omitted).` + ); + } + return parseDotEnv(source.text); +} + async function collectLegacyProjectEnvCleanupCandidates(opts: { readonly projectRoot: string; readonly projectDir: string; readonly overlayNames: readonly string[]; readonly materialize: boolean; + readonly deliveredValues: ReadonlyMap< + string, + ReadonlyMap + >; readonly configCleanupCandidates: readonly string[]; readonly composeEnvFileReferences: readonly LegacyComposeEnvFileReference[]; }): Promise<{ readonly allowed: readonly string[]; readonly blocked: readonly string[]; + readonly retainedEnvFiles: readonly { + readonly path: string; + readonly keys: readonly string[]; + }[]; }> { const candidates = new Set(); candidates.add(resolve(opts.projectDir, PROJECT_ENV_CONTRACT_FILENAME)); @@ -5166,10 +5211,25 @@ async function collectLegacyProjectEnvCleanupCandidates(opts: { const blockedPaths = new Set( opts.composeEnvFileReferences.map((reference) => reference.resolvedPath) ); + const retainedEnvFiles: { path: string; keys: string[] }[] = []; const allowed: string[] = []; const blocked: string[] = []; for (const path of candidates) { if (await pathExists(path)) { + // Value retention is independent of Compose references: repairing Compose + // must never promote a file holding undelivered values into cleanup. + const delivered = opts.deliveredValues.get(path); + if (delivered) { + const entries = await readLegacyDotenvCleanupEntries({ path }); + const keys = Object.entries(entries) + .filter(([key, value]) => delivered.get(key) !== value) + .map(([key]) => key) + .sort(); + if (keys.length > 0) { + retainedEnvFiles.push({ path, keys }); + continue; + } + } if (blockedPaths.has(path)) { blocked.push(path); } else { @@ -5178,6 +5238,7 @@ async function collectLegacyProjectEnvCleanupCandidates(opts: { } } return { + retainedEnvFiles, allowed: allowed.sort((left, right) => left.localeCompare(right)), blocked: blocked.sort((left, right) => left.localeCompare(right)), }; diff --git a/tests/doctor-fix-noninteractive.test.ts b/tests/doctor-fix-noninteractive.test.ts index 958e5eaec..668fe08e9 100644 --- a/tests/doctor-fix-noninteractive.test.ts +++ b/tests/doctor-fix-noninteractive.test.ts @@ -615,3 +615,194 @@ test("doctor --fix refuses an outside .hack and preserves its override (HACK-131 expect(notes).not.toContain(canary); expect(notes).not.toContain(outside); }, 30_000); + +test("doctor contract migration keeps base and untracked overlays with undelivered entries (HACK-1695)", async () => { + const projectRoot = await createProjectWithStaleDaemonPid(); + const projectDir = resolve(projectRoot, ".hack"); + await writeFile( + resolve(projectDir, "hack.env.json"), + JSON.stringify({ + version: 1, + vars: [{ key: "A", source: "plain_env" }], + }) + ); + const files = { + ".env": "A=synthetic-delivered\nUNDECLARED=synthetic-base-only\n", + ".env.bak": "A=synthetic-backup\nBACKUP_ONLY=synthetic-backup-only\n", + ".env.example": + "A=synthetic-example\nEXAMPLE_ONLY=synthetic-example-only\n", + ".env.qa": "A=synthetic-qa\n", + ".env.same": "A=synthetic-delivered\n", + ".env.empty": "A=\n", + }; + for (const [name, text] of Object.entries(files)) { + await writeFile(resolve(projectDir, name), text); + } + await writeFile( + resolve(projectDir, "docker-compose.yml"), + "services:\n api:\n image: alpine:3.19\n env_file: [./.env, ./.env.bak]\n" + ); + const { runCli } = await import("../src/cli/run.ts"); + expect( + await runCli([ + "doctor", + "--migrate-env-config", + "--no-interactive", + "--path", + projectRoot, + ]) + ).toBe(0); + const notes = noteCalls.join("\n"); + for (const name of [ + ".env", + ".env.bak", + ".env.example", + ".env.empty", + ] as const) { + expect(await readFile(resolve(projectDir, name), "utf8")).toBe(files[name]); + } + for (const key of ["UNDECLARED", "BACKUP_ONLY", "EXAMPLE_ONLY"]) { + expect(notes).toContain( + `keys not preserved in env layers: ${key} (values omitted)` + ); + } + for (const text of Object.values(files)) { + for (const line of text.trim().split("\n")) { + const value = line.split("=")[1]; + if (value) { + expect(notes).not.toContain(value); + } + } + } + expect( + await lstat(resolve(projectDir, ".env.qa")).catch(() => null) + ).toBeNull(); + expect( + await lstat(resolve(projectDir, ".env.same")).catch(() => null) + ).toBeNull(); + expect( + await readFile(resolve(projectDir, "hack.env.default.yaml"), "utf8") + ).toContain("synthetic-delivered"); + expect( + await readFile(resolve(projectDir, "hack.env.bak.yaml"), "utf8") + ).toContain("synthetic-backup"); + expect( + await readFile(resolve(projectDir, "hack.env.example.yaml"), "utf8") + ).toContain("synthetic-example"); +}); + +test("doctor migration refusal leaves a missing .hack/.gitignore unwritten (HACK-1695)", async () => { + const projectRoot = await createProjectWithStaleDaemonPid(); + const projectDir = resolve(projectRoot, ".hack"); + await writeFile( + resolve(projectDir, "hack.env.json"), + JSON.stringify({ + version: 1, + vars: [{ key: "TOKEN", source: "keychain" }], + }) + ); + await mkdir(resolve(projectRoot, ".hack.secret.key")); + const { runCli } = await import("../src/cli/run.ts"); + expect( + await runCli([ + "doctor", + "--migrate-env-config", + "--no-interactive", + "--path", + projectRoot, + ]) + ).toBe(1); + expect( + await lstat(resolve(projectDir, ".gitignore")).catch(() => null) + ).toBeNull(); + expect( + await lstat(resolve(projectDir, "hack.env.default.yaml")).catch(() => null) + ).toBeNull(); +}); + +test("doctor refuses a default overlay that would overwrite the migrated base (HACK-1695)", async () => { + const projectRoot = await createProjectWithStaleDaemonPid(); + const projectDir = resolve(projectRoot, ".hack"); + await writeFile( + resolve(projectDir, "hack.env.json"), + JSON.stringify({ + version: 1, + vars: [{ key: "A", source: "plain_env" }], + }) + ); + const base = "A=synthetic-collision-base\n"; + const overlay = "A=synthetic-collision-overlay\n"; + await writeFile(resolve(projectDir, ".env"), base); + await writeFile(resolve(projectDir, ".env.default"), overlay); + const { runCli } = await import("../src/cli/run.ts"); + expect( + await runCli([ + "doctor", + "--migrate-env-config", + "--no-interactive", + "--path", + projectRoot, + ]) + ).toBe(1); + expect(await readFile(resolve(projectDir, ".env"), "utf8")).toBe(base); + expect(await readFile(resolve(projectDir, ".env.default"), "utf8")).toBe( + overlay + ); + expect( + await lstat(resolve(projectDir, "hack.env.default.yaml")).catch(() => null) + ).toBeNull(); + expect( + await lstat(resolve(projectDir, ".gitignore")).catch(() => null) + ).toBeNull(); + expect( + await lstat(resolve(projectDir, "hack.env.json")).catch(() => null) + ).not.toBeNull(); + expect(noteCalls.join("\n")).not.toContain("synthetic-collision-base"); + expect(noteCalls.join("\n")).not.toContain("synthetic-collision-overlay"); +}); + +for (const filename of [".env", ".env.bak"]) { + test.skipIf(process.getuid?.() === 0)( + `doctor refuses cleanup when ${filename} cannot be read (HACK-1695)`, + async () => { + const projectRoot = await createProjectWithStaleDaemonPid(); + const projectDir = resolve(projectRoot, ".hack"); + await writeFile( + resolve(projectDir, "hack.env.json"), + JSON.stringify({ + version: 1, + vars: [{ key: "A", source: "plain_env" }], + }) + ); + const file = resolve(projectDir, filename); + const text = + "A=synthetic-unreadable-declared\nUNREADABLE_ONLY=synthetic-unreadable-only\n"; + await writeFile(file, text); + await chmod(file, 0o000); + try { + const { runCli } = await import("../src/cli/run.ts"); + expect( + await runCli([ + "doctor", + "--migrate-env-config", + "--no-interactive", + "--path", + projectRoot, + ]) + ).toBe(1); + expect(await lstat(file).catch(() => null)).not.toBeNull(); + expect( + await lstat(resolve(projectDir, "hack.env.json")).catch(() => null) + ).not.toBeNull(); + expect(noteCalls.join("\n")).not.toContain("can now be removed"); + expect(noteCalls.join("\n")).not.toContain( + "synthetic-unreadable-declared" + ); + expect(noteCalls.join("\n")).not.toContain("synthetic-unreadable-only"); + } finally { + await chmod(file, 0o600); + } + expect(await readFile(file, "utf8")).toBe(text); + } + ); +}