fix(env): verify cleanup receipts and deletion outcomes (HACK-2171)

docs/env.md

@@ -933,6 +933,22 @@ copies remain until migration cleanup or explicit removal; encrypting a YAML ent
 does not erase those copies. `doctor --migrate-env-config` prints the remaining
 plaintext key names and a `hack env protect` command after migration.
 
+Migration cleanup keeps content and file-identity receipts from its source
+inventory. After confirmation, it takes the source writer locks, checks those
+receipts and rechecks current YAML delivery and Compose references before
+removing anything. A legacy store and its key use their separate owner locks,
+including the key lock shared by every store using that path.
+Changed sources or delivery refuse cleanup with reconciliation
+advice; migration writes already completed remain. Confirmed removals before a
+failure are named. Implicit cleanup warns and lets the original command continue
+even if a lock times out, a key is unavailable or a file is unreadable.
+Once YAML layers exist, rerunning migration may not import changed legacy
+values. Keep the copies and keys, reconcile chosen values with `env set KEY
+--value-stdin` (use `--local` for machine-local values, add `--secret` for secrets
+and preserve scope/overlay), check
+delivery, then remove only copies confirmed obsolete. Editors and older writers
+that do not take Hack's locks can still race a filesystem operation.
+
 ### Concurrent writes to one env file
 
 Hack env commands that change an env file never silently lose another command's change.
@@ -1529,6 +1545,8 @@ only, so it runs even when Docker is not reachable; the other `--migrate-env-con
 repairs still need Docker. As with any migration, `hack doctor` then offers to remove the
 legacy `.hack/.env*` files that Compose no longer references (a non-interactive run
 accepts for untracked files, and for git-tracked ones only with `--accept-tracked-env-changes`).
+An untracked legacy store key joins its tracked store's confirmation batch, so
+declining tracked cleanup keeps both files and the store remains readable.
 Files Compose still names as `env_file` are offered only when the Compose repair
 removed those references. Without `hack.env.json`, a legacy file that git tracks, or one
 whose status git cannot report in a git checkout, is never offered, because the migrated
@@ -1667,6 +1685,7 @@ Some older surfaces are still compatibility-oriented:
   bound to a path, project or variable name beyond the scoped key inside the
   encrypted store plaintext
 - the daemon env GET response identifies the format. For YAML projects it returns names, scopes, encryption flags and availability without decrypting or presenting legacy state. Legacy projects keep the dotenv-shaped metadata response. Legacy set/unset mutations refuse YAML projects until those endpoints use canonical YAML mutation helpers; use the CLI instead.
+- legacy daemon unset reports HTTP 500 with `secret_delete_unconfirmed` and `secretDeleted: null` if backend deletion throws, including an unknown outcome. It preserves `dotenvChanged` for the completed dotenv update; this operation is not atomic across both stores. A failure does not confirm that a backend value is absent.
 
 If you are writing new docs or new project setup flows, document the YAML overlay model first and treat `.hack/hack.env.json` as legacy migration context.
 

docs/security/env-secrets-threat-model.md

@@ -290,34 +290,27 @@ planned branch or a passing documentation check is not a mitigation.
     session is followed until Hack's next record; ancestors of `.hack/.internal/`
     are not checked for other writers; staged multiplexer payloads are removed by
     the next staging, not by an independent reaper.
-17. **Open: secret-store and key integrity (HACK-2169).** In
-    `src/lib/secret-store.ts`, an unreadable encrypted store can be treated as empty
-    by `readEncryptedEntries`; a later mutation can replace it with only the new
-    entry. The process-global key cache is not bound to a store/key path, so a
-    cached key can be reused for another store whose key is absent. Explicit key
-    provisioning can replace an existing key without authenticating and re-encrypting
-    the store under the new one. Separately, project env add/set can create a file
-    key despite a working environment-only project key; file precedence then hides
-    that environment key from reads (`src/lib/project-env-config.ts`,
-    `src/commands/env.ts`). These are integrity and recoverability gaps, not safe
-    rotation paths. Keep them open until the corresponding fixes merge.
-18. **Open: local-layer symlinks, ignores and modes (HACK-2165).** Hydration checks
-    whether the logical local path is tracked before resolving an allowed
-    in-project symlink; the writer can then replace its tracked shared target.
-    Ordinary local add/set and hydrate can warn about failed ignore setup and
-    still write, and named local layers can already be tracked
-    (`src/lib/project-env-config.ts`, `src/lib/project-env-layer-path.ts`,
-    `src/lib/fs.ts`). Existing local-layer modes are preserved. Legacy dotenv
-    inspection also follows symlinks through `readTextFile`, including links
-    outside the project (`src/lib/hack-env.ts`); mutation checks do not protect
-    that read path. Keep these gaps open until the hygiene fixes merge; an
-    in-project target or local filename is not proof of private storage.
-19. **Open: migration cleanup recheck (HACK-2171).** Cleanup removes the current
-    path after an earlier migration/consent decision without rechecking the
-    selected file's current bytes and identity under a lock
-    (`removeLegacyProjectEnvArtifacts` in `src/lib/project-env-config.ts`,
-    cleanup callers in `src/commands/doctor.ts`). Edits made between that decision
-    and removal can be lost. Keep this open until cleanup revalidation merges.
+17. **Narrowed: secret-store and key integrity (HACK-2169, #766).** Unreadable or
+    malformed stores refuse mutation instead of becoming an empty store. Configured
+    file-key lookup does not borrow a cached key from another path; provisioning
+    refuses to replace a different existing key. An environment-only project key
+    is retained without creating a shadowing file key. Shared key-writer locks
+    serialize provisioning and cleanup. These are preservation checks, not a
+    rotation or recovery mechanism for keys already lost.
+18. **Narrowed: local-layer symlinks and Git privacy (HACK-2165, #762).**
+    Machine-local writes refuse linked or tracked destinations, unknown tracking
+    inside a checkout and ineffective ignore rules. Legacy dotenv inspection uses
+    no-follow reads. Local naming and ignores still do not prove private historical
+    permissions or erase earlier copies; inspect Doctor's findings and retain the
+    remaining filesystem-race limits described above.
+19. **Mitigated: stale migration cleanup decisions (HACK-2171).** Cleanup carries
+    content and file-identity receipts from before migration, then rechecks them
+    and current scoped delivery under writer locks after consent. Each unlink
+    rechecks its target. Refusals preserve remaining files and report confirmed
+    removals; automatic cleanup warns and continues the original command. Writers
+    outside Hack's cooperative locks can still race the final unlink. The daemon
+    also reports unconfirmed secret deletion as HTTP 500 `partial_failure`,
+    retaining the completed dotenv result instead of claiming success.
 
 ### Plaintext on disk outside materialization
 

src/commands/doctor.ts

@@ -4529,6 +4529,7 @@ async function maybeMigrateProjectEnvConfig(opts: {
     projectDir: project.projectDir,
     projectName,
     paths: cleanupCandidates,
+    context: migrated.cleanupContext,
     acceptTrackedChanges: opts.acceptTrackedChanges,
   });
   await printMigrationProtectionHint(project);
@@ -4575,6 +4576,7 @@ async function offerDoctorEnvCleanup(opts: {
   readonly projectDir: string;
   readonly projectName: string;
   readonly paths: readonly string[];
+  readonly context?: LegacyProjectEnvMigration["cleanupContext"];
   readonly acceptTrackedChanges: boolean;
 }): Promise<void> {
   const untracked: string[] = [];
@@ -4589,6 +4591,17 @@ async function offerDoctorEnvCleanup(opts: {
     ).push(path);
   }
 
+  // A key cannot be removed while its tracked store still exists. Include an
+  // untracked key in that store's later consent batch, keeping the dependency
+  // intact whether tracked cleanup is accepted, declined or refused.
+  const store = opts.context?.store;
+  if (store && tracked.includes(store.path)) {
+    const keyIndex = untracked.indexOf(store.keyPath);
+    if (keyIndex !== -1) {
+      tracked.push(...untracked.splice(keyIndex, 1));
+    }
+  }
+
   if (untracked.length > 0) {
     note(
       [
@@ -4609,14 +4622,15 @@ async function offerDoctorEnvCleanup(opts: {
         projectDir: opts.projectDir,
         projectName: opts.projectName,
         paths: untracked,
+        context: opts.context,
       });
     }
   }
 
   if (tracked.length > 0) {
     note(
       [
-        "These legacy env files are tracked by git and can now be removed. Teammates share them, so commit the removal together with the new env layers:",
+        "These legacy env files are tracked by git, or are keys kept with a tracked store, and can now be removed. Teammates share the tracked files, so commit their removal together with the new env layers:",
         ...tracked.map((path) => `- ${path}`),
       ].join("\n"),
       "env cleanup"
@@ -4632,6 +4646,7 @@ async function offerDoctorEnvCleanup(opts: {
         projectDir: opts.projectDir,
         projectName: opts.projectName,
         paths: tracked,
+        context: opts.context,
       });
     } else {
       note(
@@ -4652,10 +4667,12 @@ async function removeDoctorEnvCleanup(opts: {
   readonly projectDir: string;
   readonly projectName: string;
   readonly paths: readonly string[];
+  readonly context?: LegacyProjectEnvMigration["cleanupContext"];
 }): Promise<void> {
   const removed = await removeLegacyProjectEnvArtifacts({
     paths: opts.paths,
     projectRoot: opts.projectRoot,
+    context: opts.context,
   });
   if (removed.length > 0) {
     note(`Removed ${removed.join(", ")}`, "env cleanup");

src/daemon/routes/env.ts

@@ -525,7 +525,19 @@ async function handleUnsetEnv(opts: {
       }),
     });
   } catch {
-    secretDeleted = null;
+    return jsonResponse(
+      {
+        status: "partial_failure",
+        error: "secret_delete_unconfirmed",
+        message:
+          "The dotenv update completed, but secret deletion could not be confirmed. Inspect backend state and retry; values omitted.",
+        dotenvChanged: dotenvResult.changed,
+        secretDeleted: null,
+        backend: secretStore.descriptor.backend,
+        location: secretStore.descriptor.location,
+      },
+      500
+    );
   }
 
   return jsonResponse(

src/lib/legacy-env-cleanup-receipt.ts

@@ -0,0 +1,64 @@
+import { createHash } from "node:crypto";
+import { constants } from "node:fs";
+import { lstat, open } from "node:fs/promises";
+
+/** In-memory deletion authority tied to file identity and contents; never source text. */
+export type LegacyEnvCleanupReceipt = {
+  readonly path: string;
+  readonly identity: string;
+  readonly digest: string;
+};
+
+/** Missing, linked, changing or unreadable files have no deletion authority. */
+export async function legacyEnvCleanupReceipt(
+  path: string
+): Promise<LegacyEnvCleanupReceipt | null> {
+  try {
+    const file = await open(
+      path,
+      constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK
+    );
+    try {
+      const before = await file.stat({ bigint: true });
+      if (!before.isFile() || before.size > 32n * 1024n * 1024n) {
+        return null;
+      }
+      const bytes = Buffer.alloc(Number(before.size) + 1);
+      const { bytesRead } = await file.read(bytes, 0, bytes.length, 0);
+      if (BigInt(bytesRead) !== before.size) {
+        return null;
+      }
+      const digest = createHash("sha256")
+        .update(bytes.subarray(0, bytesRead))
+        .digest("hex");
+      const after = await file.stat({ bigint: true });
+      const named = await lstat(path, { bigint: true });
+      const identity = (entry: typeof before) =>
+        [entry.dev, entry.ino, entry.size, entry.mtimeNs, entry.ctimeNs].join(
+          ":"
+        );
+      if (
+        named.isSymbolicLink() ||
+        identity(before) !== identity(after) ||
+        identity(after) !== identity(named)
+      ) {
+        return null;
+      }
+      return { path, identity: identity(after), digest };
+    } finally {
+      await file.close();
+    }
+  } catch {
+    return null;
+  }
+}
+
+export async function legacyEnvCleanupReceiptMatches(
+  expected: LegacyEnvCleanupReceipt
+): Promise<boolean> {
+  const current = await legacyEnvCleanupReceipt(expected.path);
+  return (
+    current?.identity === expected.identity &&
+    current.digest === expected.digest
+  );
+}