refactor(storage): merge-first-validate-once for S3 config

Env vars are only used as fallback when no explicit value is provided.
A malformed env var no longer breaks the build if the caller overrides
that field. Removes readEnv/parseSource/stripAbsent in favor of a
single loop + one Zod parse on the merged result.

Removes misleading eslint-disable comment on S3Client constructor.

Author: UpperM

packages/core/src/storage/s3.ts

@@ -51,48 +51,41 @@ const s3ConfigSchema = z.object({
 	publicUrl: z.string().optional(),
 });
 
-function isRecord(value: unknown): value is Record<string, unknown> {
-	return typeof value === "object" && value !== null;
-}
-
 function isConfigKey(key: unknown): key is keyof S3StorageConfig {
 	return typeof key === "string" && key in ENV_KEYS;
 }
 
-/** Treat empty strings and undefined as absent so `S3_X=""` / `s3({ x: "" })` never mask config, and absent keys don't clobber env during merge. */
-function stripAbsent(raw: unknown): Record<string, unknown> {
-	if (!isRecord(raw)) return {};
-	const out: Record<string, unknown> = {};
-	for (const [key, value] of Object.entries(raw)) {
-		if (value === "" || value === undefined) continue;
-		out[key] = value;
-	}
-	return out;
-}
-
-function parseSource(raw: unknown, source: "env" | "explicit"): Partial<S3StorageConfig> {
-	const result = s3ConfigSchema.safeParse(stripAbsent(raw));
-	if (result.success) return result.data;
-	const issue = result.error.issues[0];
-	const pathKey = issue?.path[0];
-	if (!issue || !isConfigKey(pathKey)) fail("S3 config validation failed");
-	const label = source === "env" ? ENV_KEYS[pathKey] : `s3({ ${pathKey} })`;
-	fail(`${label} ${issue.message}`);
-}
-
-function readEnv(): Partial<S3StorageConfig> {
-	if (typeof process === "undefined" || !process.env) return {};
+/**
+ * Build the merged config: for each field, use the explicit value if present,
+ * otherwise fall back to the corresponding S3_* env var.  Validate once on the
+ * final merged result so a malformed env var never breaks the build when the
+ * caller provides that field explicitly.
+ */
+export function resolveS3Config(partial: Record<string, unknown>): S3StorageConfig {
 	const raw: Record<string, unknown> = {};
 	for (const [field, envKey] of Object.entries(ENV_KEYS)) {
-		raw[field] = process.env[envKey];
+		const explicit = partial[field];
+		if (explicit !== undefined && explicit !== "") {
+			raw[field] = explicit;
+			continue;
+		}
+		const envVal =
+			typeof process !== "undefined" && process.env ? process.env[envKey] : undefined;
+		if (envVal !== undefined && envVal !== "") {
+			raw[field] = envVal;
+		}
 	}
-	return parseSource(raw, "env");
-}
 
-export function resolveS3Config(partial: Record<string, unknown>): S3StorageConfig {
-	const env = readEnv();
-	const explicit = parseSource(partial, "explicit");
-	const merged = { ...env, ...explicit };
+	const result = s3ConfigSchema.safeParse(raw);
+	if (!result.success) {
+		const issue = result.error.issues[0];
+		const pathKey = issue?.path[0];
+		if (!issue || !isConfigKey(pathKey)) fail("S3 config validation failed");
+		const fromExplicit = partial[pathKey] !== undefined && partial[pathKey] !== "";
+		const label = fromExplicit ? `s3({ ${pathKey} })` : ENV_KEYS[pathKey];
+		fail(`${label} ${issue.message}`);
+	}
+	const merged = result.data;
 
 	const endpoint = merged.endpoint;
 	const bucket = merged.bucket;
@@ -139,7 +132,6 @@ export class S3Storage implements Storage {
 		this.publicUrl = config.publicUrl;
 		this.endpoint = config.endpoint;
 
-		// eslint-disable-next-line typescript-eslint(no-unsafe-type-assertion) -- SDK type declares credentials as required but accepts omission at runtime when the caller provides credentials through an alternative mechanism
 		this.client = new S3Client({
 			endpoint: config.endpoint,
 			region: config.region || "auto",

packages/core/tests/unit/storage/s3.test.ts

@@ -1,10 +1,4 @@
-/**
- * Tests for resolveS3Config (packages/core/src/storage/s3.ts)
- *
- * TDD: written before the implementation file exists.
- */
-
-import { describe, it, expect, vi, afterEach } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 
 // The AWS SDK is a "bring-your-own" dependency of emdash core — it is NOT
 // installed in CI. Stub it here so loading s3.ts (which statically imports
@@ -32,7 +26,7 @@ vi.mock("@aws-sdk/s3-request-presigner", () => ({
 	getSignedUrl: () => Promise.resolve("https://signed.example.com/fake"),
 }));
 
-import { resolveS3Config, createStorage } from "../../../src/storage/s3.js";
+import { createStorage, resolveS3Config } from "../../../src/storage/s3.js";
 import { EmDashStorageError } from "../../../src/storage/types.js";
 
 const FULL_ENV = {
@@ -192,13 +186,10 @@ describe("resolveS3Config", () => {
 			expect(() => resolveS3Config({ endpoint: "not-a-url" })).toThrow("s3({ endpoint })");
 		});
 
-		it("invalid S3_ENDPOINT throws even when explicit endpoint is also provided", () => {
-			// Decision (a): invalid env values are surfaced eagerly. Removing the
-			// explicit override later must not silently activate a broken env value.
+		it("malformed S3_ENDPOINT is ignored when explicit endpoint is provided", () => {
 			setEnv({ ...FULL_ENV, S3_ENDPOINT: "not-a-url" });
-			expect(() => resolveS3Config({ endpoint: "https://explicit.example.com" })).toThrow(
-				"S3_ENDPOINT",
-			);
+			const r = resolveS3Config({ endpoint: "https://explicit.example.com" });
+			expect(r.endpoint).toBe("https://explicit.example.com");
 		});
 	});