fix: use loose equality for null checks in secure_env_collect (#2997) (#3231)

When ctx.ui.custom() returns undefined instead of null, strict equality
checks (=== null / !== null) let the undefined value pass through to
writeEnvKey, which crashes on .replace(). Switch to loose equality
(== null / != null) in both the manifest orchestrator and extension
execute paths so both null and undefined are treated as "skipped".
Also add a type guard in writeEnvKey for defense in depth.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

src/resources/extensions/get-secrets-from-user.ts

@@ -54,6 +54,9 @@ function hydrateProcessEnv(key: string, value: string): void {
 }
 
 async function writeEnvKey(filePath: string, key: string, value: string): Promise<void> {
+	if (typeof value !== "string") {
+		throw new TypeError(`writeEnvKey expects a string value for key "${key}", got ${typeof value}`);
+	}
 	let content = "";
 	try {
 		content = await readFile(filePath, "utf8");
@@ -419,7 +422,7 @@ export async function collectSecretsFromManifest(
 	for (const { key, value } of collected) {
 		const entry = manifest.entries.find((e) => e.key === key);
 		if (entry) {
-			entry.status = value !== null ? "collected" : "skipped";
+			entry.status = value != null ? "collected" : "skipped";
 		}
 	}
 
@@ -427,14 +430,14 @@ export async function collectSecretsFromManifest(
 	await writeFile(manifestPath, formatSecretsManifest(manifest), "utf8");
 
 	// (j) Apply collected values to destination
-	const provided = collected.filter((c) => c.value !== null) as Array<{ key: string; value: string }>;
+	const provided = collected.filter((c) => c.value != null) as Array<{ key: string; value: string }>;
 	const { applied } = await applySecrets(provided, destination, {
 		envFilePath: resolve(ctx.cwd, ".env"),
 	});
 
 	const skipped = [
 		...alreadySkipped,
-		...collected.filter((c) => c.value === null).map((c) => c.key),
+		...collected.filter((c) => c.value == null).map((c) => c.key),
 	];
 
 	return { applied, skipped, existingSkipped };
@@ -505,8 +508,8 @@ export default function secureEnv(pi: ExtensionAPI) {
 				collected.push({ key: item.key, value });
 			}
 
-			const provided = collected.filter((c) => c.value !== null) as Array<{ key: string; value: string }>;
-			const skipped = collected.filter((c) => c.value === null).map((c) => c.key);
+			const provided = collected.filter((c) => c.value != null) as Array<{ key: string; value: string }>;
+			const skipped = collected.filter((c) => c.value == null).map((c) => c.key);
 
 			// Apply to destination via shared helper
 			const { applied, errors } = await applySecrets(provided, destination, {

src/resources/extensions/gsd/tests/secure-env-collect.test.ts

@@ -183,3 +183,137 @@ test("secure_env_collect: detectDestination — convex file (not dir) does not t
 		rmSync(tmp, { recursive: true, force: true });
 	}
 });
+
+// ─── Bug #2997: undefined vs null handling ──────────────────────────────────
+
+/**
+ * When ctx.ui.custom() returns undefined (e.g. noOpUIContext, component
+ * disposal, abort), the strict null checks (=== null / !== null) let
+ * undefined slip through as a "provided" value, crashing writeEnvKey
+ * which calls .replace() on it.
+ *
+ * These tests verify the fix: loose equality (== null / != null) so that
+ * both null AND undefined are treated as "skipped".
+ */
+
+// Helper to dynamically load the orchestrator
+async function loadOrchestrator(): Promise<{
+	collectSecretsFromManifest: Function;
+}> {
+	const mod = await import("../../get-secrets-from-user.ts");
+	return { collectSecretsFromManifest: mod.collectSecretsFromManifest };
+}
+
+// Helper to dynamically load files.ts functions
+async function loadFilesExports(): Promise<{
+	formatSecretsManifest: (m: any) => string;
+}> {
+	const mod = await import("../files.ts");
+	return { formatSecretsManifest: mod.formatSecretsManifest };
+}
+
+function makeManifest(entries: Array<{ key: string; status?: string; formatHint?: string; guidance?: string[] }>): any {
+	return {
+		milestone: "M001",
+		generatedAt: "2026-03-12T00:00:00Z",
+		entries: entries.map((e) => ({
+			key: e.key,
+			service: "TestService",
+			dashboardUrl: "",
+			guidance: e.guidance ?? [],
+			formatHint: e.formatHint ?? "",
+			status: e.status ?? "pending",
+			destination: "dotenv",
+		})),
+	};
+}
+
+async function writeManifestFile(dir: string, manifest: any): Promise<string> {
+	const { formatSecretsManifest } = await loadFilesExports();
+	const milestoneDir = join(dir, ".gsd", "milestones", "M001");
+	mkdirSync(milestoneDir, { recursive: true });
+	const filePath = join(milestoneDir, "M001-SECRETS.md");
+	writeFileSync(filePath, formatSecretsManifest(manifest));
+	return filePath;
+}
+
+test("secure_env_collect #2997: undefined from ctx.ui.custom() is treated as skipped, not provided", async (t) => {
+	const { collectSecretsFromManifest } = await loadOrchestrator();
+
+	const tmp = makeTempDir("sec-undefined-test");
+	t.after(() => {
+		rmSync(tmp, { recursive: true, force: true });
+	});
+
+	const manifest = makeManifest([
+		{ key: "SECRET_THAT_RETURNS_UNDEFINED", status: "pending" },
+	]);
+	await writeManifestFile(tmp, manifest);
+
+	let callIndex = 0;
+	const mockCtx = {
+		cwd: tmp,
+		hasUI: true,
+		ui: {
+			// First call is summary screen, second is collect — return undefined
+			// to simulate noOpUIContext or component disposal
+			custom: async (_factory: any) => {
+				callIndex++;
+				if (callIndex <= 1) return null; // summary screen dismiss
+				return undefined; // BUG TRIGGER: should be treated as skipped
+			},
+		},
+	};
+
+	// Before the fix, this crashes with:
+	// "Cannot read properties of undefined (reading 'replace')"
+	const result = await collectSecretsFromManifest(tmp, "M001", mockCtx as any);
+
+	// The undefined-returning key must appear in skipped, not in applied
+	assert.ok(
+		result.skipped.includes("SECRET_THAT_RETURNS_UNDEFINED"),
+		"Key returning undefined should be in skipped list",
+	);
+	assert.ok(
+		!result.applied.includes("SECRET_THAT_RETURNS_UNDEFINED"),
+		"Key returning undefined must NOT be in applied list",
+	);
+});
+
+test("secure_env_collect #2997: null from ctx.ui.custom() is still treated as skipped (regression guard)", async (t) => {
+	const { collectSecretsFromManifest } = await loadOrchestrator();
+
+	const tmp = makeTempDir("sec-null-test");
+	t.after(() => {
+		rmSync(tmp, { recursive: true, force: true });
+	});
+
+	const manifest = makeManifest([
+		{ key: "SECRET_THAT_RETURNS_NULL", status: "pending" },
+	]);
+	await writeManifestFile(tmp, manifest);
+
+	let callIndex = 0;
+	const mockCtx = {
+		cwd: tmp,
+		hasUI: true,
+		ui: {
+			custom: async (_factory: any) => {
+				callIndex++;
+				if (callIndex <= 1) return null; // summary screen dismiss
+				return null; // explicit null skip
+			},
+		},
+	};
+
+	const result = await collectSecretsFromManifest(tmp, "M001", mockCtx as any);
+
+	assert.ok(
+		result.skipped.includes("SECRET_THAT_RETURNS_NULL"),
+		"Key returning null should be in skipped list",
+	);
+	assert.ok(
+		!result.applied.includes("SECRET_THAT_RETURNS_NULL"),
+		"Key returning null must NOT be in applied list",
+	);
+});