[codex] Address config quality issues

[furukama]

What changed

This PR addresses a set of code-quality issues in the config and runtime surface.

• replaced silent catch blocks in the agent and TUI clipboard paths with low-noise debug logging
• removed unsafe JSON.parse(... as ...) patterns from src/config/config.ts
• replaced JSON stringify/parse clone shortcuts with structuredClone
• extracted shared runtime path and Codex constants into leaf modules to remove the runtime-config and codex-auth circular dependency
• updated constant-only consumers to import from the new leaf modules instead of pulling in runtime-config
• added regression coverage for cloned exported config objects

Why it changed

These paths were relying on swallowed errors, unchecked JSON parsing, and a documented circular-dependency workaround. The goal here is to make failures observable, keep config handling type-safe, and restore a clean module boundary.

Impact

• runtime behavior remains best-effort where it was previously best-effort
• clipboard and prompt-dump failures are now visible in debug logs
• config exports still preserve deep-clone semantics without unsafe parse/cast shortcuts
• the Codex auth and runtime config modules no longer depend on each other

Root cause

A few low-level shared values were defined in higher-level modules, which forced consumers to import heavier modules than necessary. That created a circular import and encouraged workaround patterns like inlined constants and parse-based cloning.

Validation

• ./node_modules/.bin/vitest run tests/tui-clipboard.test.ts
• ./node_modules/.bin/vitest run tests/configured-models.test.ts
• ./node_modules/.bin/vitest run tests/codex-auth.test.ts tests/configured-models.test.ts tests/runtime-home-layout.test.ts
• npm run typecheck
• npm run lint

[furukama]

@claude Do a thorough review of this PR in terms of: KISS, YAGNI, DRY, Fail Fast, Secure by Default and performance/speed

[claude]

Claude finished @furukama's task in 3m 11s —— View job

---

PR Review: KISS, YAGNI, DRY, Fail Fast, Secure by Default, Performance

• Gather context and read changed files
• Analyze against all six principles
• Post review findings

---

The PR is well-motivated and makes genuine improvements. The circular dependency removal, type-safe JSON parsing, and structuredClone migration are all solid changes. A few patterns introduced here cut against the principles requested though.

---

YAGNI — two exported symbols with no external consumers

resolveDefaultRuntimeHomeDir() is exported but never called outside its own file.

src/config/runtime-paths.ts:4 exports resolveDefaultRuntimeHomeDir() as a public function, but a search across the codebase shows it is only invoked once — to initialize DEFAULT_RUNTIME_HOME_DIR on line 14 of that same file. No other module imports or calls it. Exporting it creates API surface that has no purpose yet.

If the intent is to allow tests to re-evaluate the constant (e.g. after vi.stubEnv), the standard pattern is vi.stubEnv + a dynamic re-import, not exposing an internal factory. The function should either be unexported or removed and inlined as the IIFE it replaced.

// Before (original runtime-config.ts) — IIFE, no exported symbol
export const DEFAULT_RUNTIME_HOME_DIR = (() => { ... })();

// After — exports a function with zero consumers
export function resolveDefaultRuntimeHomeDir(): string { ... }
export const DEFAULT_RUNTIME_HOME_DIR = resolveDefaultRuntimeHomeDir();

Fix this →

---

DEFAULT_RUNTIME_HOME_DIR is re-exported from runtime-config.ts after all consumers were migrated away.

src/config/runtime-config.ts:33-35 has this pattern:

export { DEFAULT_RUNTIME_HOME_DIR } from './runtime-paths.js';  // re-export shim

import { DEFAULT_RUNTIME_HOME_DIR } from './runtime-paths.js';  // local binding

The PR itself migrates every single consumer to runtime-paths.ts directly. The re-export on line 33 exists purely as a backward-compat shim, but the PR's own changes make it obsolete. More importantly, the PR description states the goal is to "restore a clean module boundary" — keeping a re-export in runtime-config.ts keeps that blurred boundary alive. This is classic YAGNI: a shim for consumers that no longer exist.

The two-line import+re-export from the same source is also the most confusing part of runtime-config.ts to read.

Fix this →

---

KISS — cloneValue wraps a standard built-in with no added meaning

src/config/config.ts:40-42:

function cloneValue<T>(value: T): T {
  return structuredClone(value);
}

This is a single-statement wrapper around a well-known, universally understood standard API. It adds one extra level of indirection without clarifying anything — a reader encountering cloneValue(config.discord.guilds) must look up what cloneValue does to confirm it's just a deep clone, while structuredClone(config.discord.guilds) is immediately unambiguous. The six call sites are not complex enough to justify abstracting over a stdlib function.

The helper would earn its keep if it added error handling, logging, or a fallback — but it doesn't. KISS favors direct structuredClone(...) calls here.

Fix this →

---

DRY — codex-auth.ts imports and re-exports CODEX_DEFAULT_BASE_URL from the same source

src/auth/codex-auth.ts:9 and :16:

import { CODEX_DEFAULT_BASE_URL } from '../providers/codex-constants.js';   // line 9
// ...
export { CODEX_DEFAULT_BASE_URL } from '../providers/codex-constants.js';   // line 16

The import on line 9 is needed because the constant is used in the function body at line 766. The re-export on line 16 keeps backward compatibility for consumers of codex-auth.ts. Both are technically necessary, but this is worth flagging: if tests or consumers import CODEX_DEFAULT_BASE_URL from codex-auth.ts (see tests/cli.test.ts:615), those callers now have a two-hop import path through a module with significant side effects (auth I/O). Longer term, direct imports from codex-constants.ts are preferable. Not a blocker, but the re-export in codex-auth.ts will likely be forgotten and not removed.

---

Fail Fast — debug-only logging may hide real failures

The silent catch blocks are now logged at logger.debug(...). This is appropriate for best-effort paths (clipboard I/O, WSL detection, prompt dumps). However:

• isProbablyWsl() at src/tui-clipboard.ts:228 logs /proc/version read failure at debug and returns false. On a non-Linux platform (macOS, Windows) this will emit a spurious debug log on every call since /proc/version doesn't exist there. The function should short-circuit on non-Linux platforms before attempting the read, or the error should only be logged when process.platform === 'linux'.

• readVersionFromPackageJson at src/config/config.ts:55 still has a silent catch {}. This is consistent with the best-effort version detection pattern, but if all four package.json probe paths fail, the function returns null with no signal. Since version resolution is called at startup, a single warn-level log if all probes fail would make startup misconfiguration visible without being noisy.

---

Secure by Default — improvements noted, one minor observation

Genuine wins:

• structuredClone over JSON.parse(JSON.stringify()) removes a class of silent data corruption: dates would silently serialize to strings, undefined values would drop, Map/Set would serialize to {}. structuredClone raises actual errors on non-cloneable values.
• isRecord type guard before accessing .version eliminates the as cast after JSON.parse — this is the right pattern.
• runtime-secrets.ts importing from runtime-paths.ts instead of the heavier runtime-config.ts reduces the blast radius of a hypothetical dependency-chain vulnerability in runtime-config.

Minor observation: src/providers/codex-constants.ts is a single-constant file containing a hardcoded external URL. That's appropriate here (it's a vendor constant), but it's worth noting this URL appears in three import locations now — if it ever needs to be environment-overridable at the constant layer rather than at the call site, that would need to be plumbed into runtime-paths.ts-style resolution. Not an issue now, just a future consideration.

---

Performance/Speed — net positive

• structuredClone avoids the JSON serialization round-trip (serialize to string → parse from string). For the objects being cloned here (Discord config, MCP server maps), this is a genuine micro-optimization with no behavioral cost.
• Module splitting (runtime-paths.ts, codex-constants.ts) allows lightweight modules (runtime-secrets.ts, security/instruction-integrity.ts, channels/whatsapp/auth.ts) to avoid loading all of runtime-config.ts at startup. This reduces module graph weight for these paths.
• logClipboardFallback uses { ...details, err } which allocates a new object on every error call. Error paths only — not a concern.

---

Summary

Principle	Status	Notes
KISS	⚠️	cloneValue wrapper is a needless alias over structuredClone
YAGNI	⚠️	resolveDefaultRuntimeHomeDir() exported but unused; backward-compat re-export in runtime-config.ts obsoleted by this very PR
DRY	✅	Good; the double import+re-export in codex-auth.ts is necessary
Fail Fast	✅	Big improvement; WSL log on non-Linux is a minor nit
Secure by Default	✅	structuredClone, type-guarded parsing, reduced import surface
Performance	✅	structuredClone + lighter module graph at startup

The three fixes worth making before merge: remove the cloneValue indirection, drop the export { DEFAULT_RUNTIME_HOME_DIR } shim from runtime-config.ts, and unexport resolveDefaultRuntimeHomeDir.