Fail open on invalid local settings with in-app diagnostics

[TheGreatAxios]

Summary

• Invalid/unknown keys in .corbits/settings.json no longer crash startup
• Known fields still apply; credentials and bad values are ignored
• Actionable diagnostics surface on the main TUI with fix guidance

Test plan

• Put unknown keys + credentials in local settings.json — app starts
• Known provider/model still applied
• Main screen shows settings diagnostics with fix recommendations
• bun test src/settings.test.ts passes

Closes CL-5348

[linear-code]

CL-5348

[TheGreatAxios]

Code review (parent — multi-agent fleet unavailable)

Verdict: approve with nits — core fail-open path looks solid.

Correctness

• loadLocalSettingsResult + coerceLocalSettings fail open on unknown keys, credentials, invalid JSON, and bad field types.
• Known keys still applied; credentials never enter settings.
• Diagnostics threaded via Config.settingsDiagnostics into TUI (app.tsx / runner.tsx).
• Regression test updated: credentials/unknown keys no longer throw.

Nits

1. Confirm global settings path still fail-closed or has equivalent UX if invalid (issue screenshot was local path).
2. Ensure diagnostics render is non-blocking and dismissible / not flooding the transcript every remount.
3. loadLocalSettings wrapper drops diagnostics — good for callers that only need settings; document that in a short JSDoc (already partly there).

Tests

• Good fail-open regression. Consider also asserting loadLocalSettingsResult diagnostics messages when time allows.

[TheGreatAxios]

Review — request changes

Fail-open + credential stripping is directionally right (allowlist into LocalSettings only; diagnostics name keys, not values). A few correctness/UX issues should land before merge.

Blocking / should-fix

1. Esc dismiss is not wired (src/tui/app.tsx)

   • UI says “Press Esc to dismiss settings warnings” and the prop docs call the notice dismissible, but setSettingsNotice is only used in the useState initializer — nothing ever clears it.
   • Esc still goes through double-Esc / exit paths in use-keymap. Please wire dismiss (and prefer a dedicated key if Esc is overloaded).

2. Fail-open reasoningEffort allowlist is incomplete (coerceLocalSettings)

   • Coercion only accepts "low" | "medium" | "high", but REASONING_EFFORTS also includes "none", "minimal", "xhigh", etc.
   • Example: { "reasoningEffort": "none", "weird": true } drops a valid effort.
   • Use isReasoningEffort() (or the shared enum) and update the diagnostic fix text.

Non-blocking

3. Chrome geometry — multi-line settingsNotice is not counted in extraChromeRowCount, so tall diagnostics can crowd the prompt/scrollback budget.
4. Test gaps — no coverage for invalid JSON / non-object fail-open; no assertion that valid non-low/med/high efforts survive when unknown keys force the coerce path; no TUI dismiss test.
5. Invalid sessionMode — dropped without a specific diagnostic when other diagnostics already exist (generic path only when diagnostics.length === 0).

Security (credentials)

• OK: returned settings are built only from LOCAL_ALLOWED_KEYS / typed fields — unknown keys including apiKey/token/etc. never enter LocalSettings.
• OK: diagnostic messages include key names only, not secret values.
• OK: saveLocalSettings still requires isLocalSettings before write, so a later session-mode save rewrites a cleaned selection file.
• Credential-key heuristics (/key|token|…/) are messaging-only; stripping is allowlist-based (good).

Merge

• Mergeable vs main; no overlap with Guide first-run onboarding with numbered step copy #321 files.

[TheGreatAxios]

Addressed multi-agent review:

• Esc dismisses the settings diagnostics banner via keymap (settingsNoticePresent + dismissSettingsNotice).
• Fail-open coerce uses shared isReasoningEffort() so all effort levels (not just low/medium/high) survive.
• Chrome budget reserves 2 rows for the banner + Esc hint.

[TheGreatAxios]

Multi-agent review (blended: cto / greybeard / neckbeard / critique / bruckheimer)

Verdict: Approve (after earlier post-review fixes)

Fixed earlier

• Esc dismisses settings diagnostics banner.
• Fail-open coerce uses shared isReasoningEffort().
• Chrome budget reserves rows for the banner.

Ready for human review/merge.

[TheGreatAxios]

Hard verdict: DENY

Fail-open for invalid local settings is the right product direction, but this PR ships silent fail-open for every non-TUI caller and leaves a write-base clobber path that can destroy the on-disk file. That is not mergeable as-is.

Blockers (must fix)

1. Silent fail-open outside TUI — src/config/settings.ts, src/index.ts, src/exec/runner.ts

loadLocalSettings now swallows invalid JSON / schema / unknown keys and returns partial or null with no signal:

export async function loadLocalSettings(path: string): Promise<LocalSettings | null> {
  // Fail open: never throw for schema/unknown-key problems. Callers that need
  // diagnostics should use loadLocalSettingsResult.
  const { settings } = await loadLocalSettingsResult(path);
  return settings;
}

loadConfig attaches settingsDiagnostics, but only the TUI path surfaces them (tui/runner.tsx → App). mainWithRunners routes exec straight to runExec with zero diagnostics emission. Secondary reloads also discard them:

• src/exec/runner.ts:246 — loadLocalSettings(...).catch(() => null)
• src/tui/runner.tsx:634 — same pattern for session mode

Fix: At process boundary (e.g. src/index.ts after loadConfig, or inside runExec), if config.settingsDiagnostics?.length, write each diagnostic to stderr (message + fix). Prefer also logging via the existing logger. Do not rely on Ink for the only signal — CI/corbits exec must see it.

2. Write-base null collapse / clobber — src/tui/runner.tsx:1441-1446

Global settings already refuse to RMW over a broken file:

// An absent file yields a fresh minimal base; an unreadable or invalid file
// yields null so the caller skips the write — falling back to a minimal base
// there would overwrite the whole file to flip one key.
export async function loadGlobalSettingsWriteBase(path: string): Promise<Settings | null> {
  try {
    return (await loadSettings(path)) ?? { providers: {} };
  } catch {
    return null;
  }
}

Local session-mode RMW does the opposite:

      onChangeSessionMode={async (mode, scope) => {
        if (scope === "local") {
          const path = localSettingsPath(config.cwd);
          const existing = (await loadLocalSettings(path).catch(() => null)) ?? {};
          const next: LocalSettings = { ...existing, sessionMode: mode };
          await saveLocalSettings(path, next);

After this PR, invalid JSON → loadLocalSettings returns null (no throw) → ?? {} → saveLocalSettings writes { sessionMode } and wipes the broken file. Same for "all known fields invalid → null". That is exactly the clobber the global write-base comment forbids.

Fix: Add loadLocalSettingsWriteBase (or use loadLocalSettingsResult) with explicit states:

Disk state	Write base
ENOENT / absent	{} (create OK)
Valid / fail-open partial with known fields	merge into cleaned LocalSettings
Invalid JSON / non-object / unreadable	skip write (return null); surface error to UI

Never treat "load failed with diagnostics and settings === null while file exists" as empty base.

Non-blocking but real

• Chrome budget undercount (chrome-geometry.ts:91): hard-codes 2 rows, but settingsNotice is diagnostics.map(...message + fix).join("\n") plus Esc hint — N diagnostics can be 2N+1 lines. Budget from actual line count or cap rendered lines.
• Tests thin (settings.test.ts): no coverage for invalid JSON, non-object, empty-after-coerce, write-base skip, or loadConfig → diagnostics wiring; no exec/stderr assertion.
• Esc dismiss + isReasoningEffort coerce + pickLocalFields share look correct; keep those.

What is fine

• TUI banner + Esc dismiss (keymap test present)
• isReasoningEffort on coerce path
• pickLocalFields strict/coerce split (no drift risk on known keys)
• Credential keys stripped from in-memory load

REQUEST_CHANGES until (1) non-TUI diagnostics emission and (2) local write-base no-clobber are fixed.

[TheGreatAxios]

Hard review (greybeard + CTO)

Verdict: APPROVE (after follow-up commits)

Greybeard

Fail-open local load + diagnostics on the main screen is correct. Write-base helpers for both global and local selection files prevent RMW clobber of broken JSON. Exec path emits diagnostics to stderr so Ink is not the only signal. Shared pickLocalFields keeps coerce/strict paths honest.

CTO

This was the most important reliability fix in the set. Tests cover invalid JSON, credentials strip, write-base absent/cleaned/unusable, and global write-base. Ship.

No remaining blockers.