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

docs/env.md

@@ -1151,7 +1151,7 @@ files, when:
   become global layer values that Hack applies after `environment`, so the service would
   switch to the `.env` value. This includes an entry that only refers to its own key
   (`PORT: ${PORT}`), whose result can differ when an overlay or the shell sets the key. The error names the keys and services, never values.
-  Remove the key from the `.hack/.env` file or from that service's `environment`, then retry;
+  Remove the key from the legacy `.env` file or overlay that sets it, or from that service's `environment`, then retry;
 - a service does not list a legacy file with values to migrate in `env_file` (`E_CONFIG_INVALID`).
   Migrated values are global and reach every service, including third-party images, so that
   service would receive values it never had. This includes a file no service lists, whose values
@@ -1189,9 +1189,12 @@ files, when:
   overlay file doesn't set (`E_CONFIG_INVALID`), because a local base outranks tracked and inherited
   overlay layers. Remove the key from `.hack/.env`, or set the value you want in that legacy overlay
   file, creating it if needed; the Compose `env_file` checks above then apply to that file;
+- env layers include a symlink while legacy values remain undelivered (`E_PATH_CONFLICT`).
+  Replace linked layers with regular files containing the intended layer content, then retry
+  `hack doctor --migrate-env-config`. The migration does not write through a linked layer;
 - `.hack/.env` has a state file, env layers exist, and its text isn't what `hack env materialize`
   would write (`E_PATH_CONFLICT`). The file is out of date or was edited by hand, and Hack can't
-  tell which. If it's generated, move any value of your own with `hack env set`, then run
+  tell which. If it's generated, move any value of your own with `hack env set --local`, then run
   `hack env materialize`, which replaces it. If it's your own file, delete `.hack/.env.state.json`
   so it's read as a legacy file, but not for generated output, whose values would then migrate as
   your own. When the text does match, the migration doesn't read the file or offer it for cleanup;
@@ -1219,6 +1222,10 @@ files, when:
   that resolves to a readable file inside `.hack` is migrated like a regular one; cleanup
   removes only the link, never its target.
 
+A refused migration keeps the legacy source files. Earlier layer merges or ignore-rule
+updates may remain, and removing newly created layers after a failure can itself fail.
+Check the layers and ignore rules before retrying; migration compares the values again.
+
 With `hack.env.json`, the migration refuses (`E_CONFIG_INVALID`) when a `plain_env` value comes only
 from your shell, because it would otherwise be saved to a plaintext file you didn't ask for. This applies to commands
 that migrate implicitly, such as `hack env exec`. Simply skipping the value isn't enough, because

src/commands/doctor.ts

@@ -173,6 +173,7 @@ import {
   inspectLegacyComposeEnvFileReferences,
   inspectProjectEnvMaterialization,
   type LegacyComposeEnvFileReference,
+  LegacyDotenvMigrationRefusalError,
   type LegacyProjectEnvMigration,
   migrateLegacyProjectEnv,
   ProjectEnvLayerPathConflictError,
@@ -4094,14 +4095,26 @@ async function buildEnvMigrationPlanStep(opts: {
     return "Skip env migration because no project was detected from this path.";
   }
 
-  // The same format decision as the migration itself. A layer entry it refuses is
-  // reported as a plan; a path leaving the project still stops the run (HACK-1685).
+  // Match the execution gate, including checkout values not yet in the layers.
+  // A layer conflict is reported as a plan; escaping paths still stop the run.
   let modernExists: boolean;
   try {
     modernExists = await projectEnvConfigExistsForMutation({
       projectDir: opts.project.projectDir,
     });
+    if (modernExists) {
+      modernExists = !(await canMigrateCheckoutLegacyDotenv({
+        projectRoot: opts.project.projectRoot,
+        projectDir: opts.project.projectDir,
+        serviceNames: await discoverComposeServiceNames({
+          composeFile: opts.project.composeFile,
+        }),
+      }));
+    }
   } catch (error: unknown) {
+    if (error instanceof LegacyDotenvMigrationRefusalError) {
+      return error.message;
+    }
     if (error instanceof ProjectEnvLayerPathConflictError) {
       return `Refuse env migration (E_PATH_CONFLICT) because ${error.path} is not a regular env layer file.`;
     }

src/lib/project-env-config.ts

@@ -5433,11 +5433,12 @@ export async function migrateLegacyProjectEnv(opts: {
 }
 
 /**
- * Closed, value-free refusal of the explicit migration of a project without a
- * `.hack/hack.env.json` contract ({@link migrateUndeclaredLegacyDotenv}). It is
- * raised before any file is written, so the legacy `.hack/.env*` files and any
- * existing entries stay as they were. The message names paths, legacy file
- * names and counts only, never a key or a value.
+ * Closed, value-free refusal of a legacy project env migration, including the
+ * explicit path without a contract ({@link migrateUndeclaredLegacyDotenv}). It
+ * may also be raised after ignore rules or earlier layers have been written.
+ * The legacy `.hack/.env*` files are kept; a retry compares their values again.
+ * The message names paths, valid keys and scopes, or counts of malformed entries,
+ * never values.
  */
 export class LegacyDotenvMigrationRefusalError extends HackCliError {
   constructor(opts: {
@@ -5448,7 +5449,7 @@ export class LegacyDotenvMigrationRefusalError extends HackCliError {
   }) {
     super({
       code: opts.code,
-      message: `Refusing to migrate the legacy .hack/.env files: ${opts.reason} ${opts.closing ?? "Nothing was written, and the legacy files are kept."}`,
+      message: `Refusing to migrate the legacy .hack/.env files: ${opts.reason} ${opts.closing ?? "The legacy files are kept. Earlier migration writes may remain; check the layers and ignore rules before retrying."}`,
     });
     this.name = "LegacyDotenvMigrationRefusalError";
   }
@@ -5545,11 +5546,6 @@ async function assertUntrackedMigrationLocalLayer(opts: {
   }
 }
 
-/**
- * Legacy `.hack/.env.<name>` overlays the `.env`-only migration converts.
- * Refuses, before anything is written, names that are not overlay names or
- * that would land on the default or a machine-local layer.
- */
 /**
  * Names legacy overlay files for a refusal. File names are not values, but
  * each is JSON-quoted so a control character cannot break the one-line message.
@@ -5991,13 +5987,13 @@ async function assertComposeEnvironmentKeepsPrecedence(opts: {
   if (conflicts.length > 0) {
     throw new LegacyDotenvMigrationRefusalError({
       code: "E_CONFIG_INVALID",
-      reason: `Compose environment sets ${conflicts.sort().join(", ")}, which the legacy .env files also set; after the migration the .env value would override the Compose value. Remove the key from the .hack/.env file or from that service's environment, then retry.`,
+      reason: `Compose environment sets ${conflicts.sort().join(", ")}, which the legacy .env files also set; after the migration the .env value would override the Compose value. Remove the key from the legacy .env file or overlay that sets it, or from that service's environment, then retry.`,
     });
   }
   if (envFileConflicts.length > 0) {
     throw new LegacyDotenvMigrationRefusalError({
       code: "E_CONFIG_INVALID",
-      reason: `Compose env_file sets ${envFileConflicts.sort().join(", ")}, which the legacy .env files also set; after the migration the .env value would override that file's value. Remove the key from the .hack/.env file or from that env_file, then retry.`,
+      reason: `Compose env_file sets ${envFileConflicts.sort().join(", ")}, which the legacy .env files also set; after the migration the .env value would override that file's value. Remove the key from the legacy .env file or overlay that sets it, or from that env_file, then retry.`,
     });
   }
   const defaultOverlay = await readProjectDefaultEnvConfig({
@@ -6212,6 +6208,10 @@ function legacyOverlayChangedValues(opts: {
   );
 }
 
+/**
+ * Legacy `.hack/.env.<name>` overlays the `.env`-only migration converts.
+ * Refuses names that are not overlay names or would land on a default or local layer.
+ */
 async function selectLegacyDotenvOverlays(opts: {
   readonly projectDir: string;
 }): Promise<readonly string[]> {
@@ -6320,7 +6320,8 @@ async function deliveredLayerValuesForMigration(opts: {
  * `.hack/.env` with a valid state file is treated as generated output, so it is
  * skipped. A migration that runs for another reason checks its text
  * ({@link legacyBaseDotenvIsGenerated}); with nothing else to migrate, nothing does.
- * Overlays are not generated by materialize, so they are always compared.
+ * Overlays are not generated by materialize. Only valid, non-reserved overlay
+ * names are compared; state files, templates and backups are excluded too.
  */
 export async function findUnmigratedLegacyDotenv(opts: {
   readonly projectRoot: string;
@@ -6416,12 +6417,15 @@ export function describeUnmigratedLegacyDotenv(
   return parts.join("; ");
 }
 
-/** Whether `doctor --migrate-env-config` can move these values: regular layer files only. */
+/** Whether legacy values remain to migrate; refuses non-regular layers with a repair. */
 export async function canMigrateCheckoutLegacyDotenv(opts: {
   readonly projectRoot: string;
   readonly projectDir: string;
   readonly serviceNames: readonly string[];
 }): Promise<boolean> {
+  if (!hasUnmigratedLegacyDotenv(await findUnmigratedLegacyDotenv(opts))) {
+    return false;
+  }
   const entries = await readdir(opts.projectDir, { withFileTypes: true });
   const allRegular = entries
     .filter(
@@ -6430,10 +6434,14 @@ export async function canMigrateCheckoutLegacyDotenv(opts: {
         entry.name.endsWith(PROJECT_ENV_CONFIG_FILENAME_SUFFIX)
     )
     .every((entry) => entry.isFile());
-  return (
-    allRegular &&
-    hasUnmigratedLegacyDotenv(await findUnmigratedLegacyDotenv(opts))
-  );
+  if (!allRegular) {
+    throw new LegacyDotenvMigrationRefusalError({
+      code: "E_PATH_CONFLICT",
+      reason:
+        "env layers include a symlink or non-regular entry, and legacy values remain undelivered. Replace symlinked env layers with regular files containing the intended layer content, repair any other non-regular entries, then retry `hack doctor --migrate-env-config`.",
+    });
+  }
+  return true;
 }
 
 /**
@@ -7383,8 +7391,8 @@ async function planLocalLayerMerges(opts: {
 }
 
 /**
- * Publishes machine-local layers, then tracked ones. If a write fails, removes
- * what this call created, so the checkout keeps using the legacy files.
+ * Publishes machine-local layers, then tracked ones. If a write fails, attempts
+ * to remove what this call created. A cleanup failure can leave a created layer.
  */
 async function publishMigratedLegacyLayers(opts: {
   readonly projectRoot: string;
@@ -7426,8 +7434,8 @@ async function publishMigratedLegacyLayers(opts: {
       }
     }
   } catch (error: unknown) {
-    // Every target was absent before this call, so removing what it created
-    // leaves the project on the legacy files.
+    // Best-effort cleanup of targets this call created; the refusal must not
+    // promise rollback, because an unlink can fail too.
     for (const path of [...wroteFiles].reverse()) {
       await unlink(path).catch(() => undefined);
     }
@@ -7505,9 +7513,9 @@ async function deliveredValuesPerLegacyFile(opts: {
  *   `hack.env.default.yaml` (and `hack.env.<overlay>.yaml` for each migrated
  *   overlay) is written with no values, so the project switches to env layers.
  *   With no legacy file at all only the empty default layer is written.
- * - Refused before anything is written ({@link LegacyDotenvMigrationRefusalError},
+ * - Preflight refuses ({@link LegacyDotenvMigrationRefusalError},
  *   or {@link ProjectEnvLayerPathConflictError} for an unusable layer entry):
- *   any existing env layer entry, a legacy file that is a symlink out of
+ *   an unusable env layer entry or conflicting local value, a legacy file that is a symlink out of
  *   `.hack`, a legacy overlay named like the default or a
  *   machine-local layer, a Compose `env_file` on a file the migration leaves
  *   out or on an overlay a launch without `--env` would not select, a Compose
@@ -7519,9 +7527,10 @@ async function deliveredValuesPerLegacyFile(opts: {
  *   from Hack, a key that does not match `^[A-Z_][A-Z0-9_]*$`
  *   (counted per file, never shown), and a machine-local target that git
  *   tracks or, inside a git checkout, cannot report on.
- * - Layers are created only where nothing exists (compare-and-swap against an
- *   absent file); if a later write fails, the layers this call created are
- *   removed again, so the project keeps using the legacy files.
+ * - Missing layers are created with an absence check. Existing local layers
+ *   receive missing keys after a content comparison. Publication failure attempts
+ *   to remove newly created layers; earlier merges and ignore-rule updates are
+ *   not rolled back, and cleanup can fail. Legacy sources are kept for retries.
  * - Without a contract nothing names secret-backend values, so none are read
  *   or migrated, and `hack.config.json` (including any secret backend it
  *   configures, and that backend's store) is left unchanged and never offered
@@ -7824,7 +7833,7 @@ async function legacyBaseDotenvIsGenerated(opts: {
   if (generated !== read.text) {
     throw new LegacyDotenvMigrationRefusalError({
       code: "E_PATH_CONFLICT",
-      reason: `.hack/.env has a state file, but its text isn't what \`hack env materialize\` would write, so it may hold values of your own. If it's generated output, move any value of your own with \`hack env set\`, then run \`hack env materialize\`, which replaces the file. If it's your own file, delete .hack/.env.state.json so it's read as a legacy file. Don't do that for generated output, because its values would then migrate as your own.`,
+      reason: `.hack/.env has a state file, but its text isn't what \`hack env materialize\` would write, so it may hold values of your own. If it's generated output, move any value of your own with \`hack env set --local\`, then run \`hack env materialize\`, which replaces the file. If it's your own file, delete .hack/.env.state.json so it's read as a legacy file. Don't do that for generated output, because its values would then migrate as your own.`,
     });
   }
   return true;

tests/doctor-command.test.ts

@@ -232,6 +232,21 @@ test("doctor remediation plan skips env migration when modern env files already
   }
 });
 
+test("doctor plans migration when existing layers leave a legacy key undelivered", async () => {
+  const root = await createDoctorTestProject({ modern: true });
+  try {
+    await writeFile(join(root, ".hack", ".env"), "EXTRA=synthetic\n");
+    const lines = await buildDoctorRemediationPlanLines({
+      startDir: root,
+      migrateEnvConfig: true,
+    });
+    expect(lines.join("\n")).toContain("Prompt to migrate legacy env values");
+    expect(lines.join("\n")).not.toContain("already uses");
+  } finally {
+    await rm(root, { recursive: true, force: true });
+  }
+});
+
 test("doctor guidance keeps CoreDNS restartable outages out of doctor --fix", () => {
   const guidance = buildDoctorRecoveryGuidance({
     results: [

tests/doctor-fix-noninteractive.test.ts

@@ -665,7 +665,7 @@ for (const docker of [
   }, 30_000);
 }
 
-test("doctor --migrate-env-config treats a symlinked overlay as env layers and never writes through it (HACK-1370)", async () => {
+test("doctor --migrate-env-config refuses a symlinked overlay with undelivered values and never writes through it (HACK-1370)", async () => {
   const projectRoot = await createProjectWithStaleDaemonPid();
   const projectDir = resolve(projectRoot, ".hack");
   const owned = ["synthetic", "owned", "77d1"].join("-");
@@ -686,8 +686,8 @@ test("doctor --migrate-env-config treats a symlinked overlay as env layers and n
     "--path",
     projectRoot,
   ]);
-  expect(code).toBe(0);
-  expect(noteCalls.join("\n")).toContain(
+  expect(code).not.toBe(0);
+  expect(noteCalls.join("\n")).not.toContain(
     "Project already uses the new env config files."
   );
   // The link and its target are untouched; nothing was migrated or cleaned.