From 8402787ae47c98bf2b3de6cb696271fa34ded9e1 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 4 Aug 2026 19:48:30 -0700 Subject: [PATCH 1/5] Fail open on invalid local settings with in-app diagnostics Invalid or unknown keys in .corbits/settings.json no longer crash startup. Known fields still apply; credentials and bad values are ignored with actionable diagnostics surfaced on the main TUI. Closes CL-5348 --- src/config/index.ts | 13 +++- src/config/settings.ts | 156 ++++++++++++++++++++++++++++++++++++----- src/settings.test.ts | 28 ++++++-- src/tui/app.tsx | 24 +++++++ src/tui/runner.tsx | 3 + 5 files changed, 199 insertions(+), 25 deletions(-) diff --git a/src/config/index.ts b/src/config/index.ts index 7b009e498..4d80d24fe 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -34,6 +34,8 @@ import { xaiUserIdFromAccessToken } from "../auth/xai/session.js"; import { globalSettingsPath, loadLocalSettings, + loadLocalSettingsResult, + type SettingsLoadDiagnostic, loadSettings, localSettingsPath, normalizeOpenAICompatibleBaseURL, @@ -245,6 +247,11 @@ export type Config = { * providerCatalogToSettings (or re-read disk) before any persist. */ settings?: Settings; + /** + * Fail-open diagnostics from local settings load (unknown keys, invalid JSON, + * stripped credentials). Shown on the main TUI so startup never hard-crashes. + */ + settingsDiagnostics?: SettingsLoadDiagnostic[]; }; // Returned by loadConfig when no provider is configured and allowUnconfigured is @@ -403,7 +410,10 @@ export async function loadConfig( // The per-repo selection file still applies on top of a --config source: that // file supplies provider definitions, while .corbits/settings.json supplies // the provider/model selection. CLI --provider/--model override both. - const local = await loadLocalSettings(localSettingsPath(cwd)); + // Fail open on unknown/invalid local keys — never crash startup. + const localResult = await loadLocalSettingsResult(localSettingsPath(cwd)); + const local = localResult.settings; + const settingsDiagnostics = localResult.diagnostics; const profile = await resolveProfile(cwd, profileFlag); @@ -499,6 +509,7 @@ export async function loadConfig( // Codex/xAI providers that are never written to settings.json. Not safe // to persist as-is — use providerCatalogToSettings or re-read disk. ...(settingsForResolution !== null ? { settings: settingsForResolution } : {}), + ...(settingsDiagnostics.length > 0 ? { settingsDiagnostics } : {}), }; } diff --git a/src/config/settings.ts b/src/config/settings.ts index e448e7d84..fddf126ce 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -650,36 +650,154 @@ export async function loadSettings(path: string): Promise { }; } -export async function loadLocalSettings(path: string): Promise { +/** Diagnostic produced when settings fail open instead of crashing startup. */ +export type SettingsLoadDiagnostic = { + path: string; + message: string; + /** Actionable recommendation for the user. */ + fix: string; +}; + +export type LocalSettingsLoadResult = { + settings: LocalSettings | null; + diagnostics: SettingsLoadDiagnostic[]; +}; + +const LOCAL_ALLOWED_KEYS = new Set(LOCAL_SETTINGS_OPTIONAL_KEYS); +const LOCAL_CREDENTIAL_KEYS = new Set([ + "apiKey", + "api_key", + "token", + "secret", + "password", + "authorization", +]); + +function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadResult { + const diagnostics: SettingsLoadDiagnostic[] = []; + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + return { + settings: null, + diagnostics: [ + { + path, + message: `Local settings in ${path} is not a JSON object.`, + fix: `Edit ${path} to a JSON object with only: provider, model, reasoningEffort, mcpServers, sessionMode, env.`, + }, + ], + }; + } + const s = parsed as Record; + // Valid strict path still returns cleanly with no diagnostics. + if (isLocalSettings(parsed)) { + const optional: OptionalLocalSettingsFields = { + provider: s.provider as string | undefined, + model: s.model as string | undefined, + reasoningEffort: s.reasoningEffort as ReasoningEffort | undefined, + mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, + sessionMode: + s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, + env: s.env as Record | undefined, + }; + return { settings: pickDefined(optional), diagnostics: [] }; + } + + const unknownKeys = Object.keys(s).filter((k) => !LOCAL_ALLOWED_KEYS.has(k)); + const credentialKeys = unknownKeys.filter( + (k) => LOCAL_CREDENTIAL_KEYS.has(k) || /key|token|secret|password/i.test(k), + ); + const otherUnknown = unknownKeys.filter((k) => !credentialKeys.includes(k)); + if (credentialKeys.length > 0) { + diagnostics.push({ + path, + message: `Ignored credential field(s) in local settings (${credentialKeys.join(", ")}).`, + fix: "Keep credentials out of local .corbits/settings.json — store API keys via provider settings / keychain, not local selection files.", + }); + } + if (otherUnknown.length > 0) { + diagnostics.push({ + path, + message: `Ignored unknown local settings key(s): ${otherUnknown.join(", ")}.`, + fix: `Remove unknown keys from ${path}. Allowed keys: ${[...LOCAL_ALLOWED_KEYS].join(", ")}.`, + }); + } + + const optional: OptionalLocalSettingsFields = { + provider: typeof s.provider === "string" ? s.provider : undefined, + model: typeof s.model === "string" ? s.model : undefined, + reasoningEffort: + s.reasoningEffort === "low" || s.reasoningEffort === "medium" || s.reasoningEffort === "high" + ? s.reasoningEffort + : undefined, + mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, + sessionMode: + s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, + env: + s.env !== undefined && typeof s.env === "object" && s.env !== null && !Array.isArray(s.env) + ? Object.fromEntries( + Object.entries(s.env as Record).filter( + (e): e is [string, string] => typeof e[1] === "string", + ), + ) + : undefined, + }; + if (s.mcpServers !== undefined && optional.mcpServers === undefined) { + diagnostics.push({ + path, + message: `mcpServers in ${path} was invalid and was ignored.`, + fix: "Use an object map of MCP server entries (command/args or url).", + }); + } + if (s.reasoningEffort !== undefined && optional.reasoningEffort === undefined) { + diagnostics.push({ + path, + message: `reasoningEffort in ${path} was invalid and was ignored.`, + fix: 'Use "low", "medium", or "high".', + }); + } + if (diagnostics.length === 0) { + // Shape failed isLocalSettings for another reason (e.g. wrong types). + diagnostics.push({ + path, + message: `Local settings in ${path} had invalid values and were partially ignored.`, + fix: `Edit ${path}: only "provider", "model", "reasoningEffort", "mcpServers", "sessionMode", and "env" are allowed (no credentials).`, + }); + } + const settings = pickDefined(optional); + return { settings: Object.keys(settings).length > 0 ? settings : null, diagnostics }; +} + +export async function loadLocalSettingsResult(path: string): Promise { let raw: string; try { raw = await readFile(path, "utf8"); } catch (err) { - if (isENOENT(err)) return null; + if (isENOENT(err)) return { settings: null, diagnostics: [] }; throw err; } let parsed: unknown; try { parsed = JSON.parse(raw); } catch { - throw new Error(`Invalid JSON in local settings file: ${path}`); - } - if (!isLocalSettings(parsed)) { - throw new Error( - `Invalid local settings in ${path}: only "provider", "model", "reasoningEffort", "mcpServers", "sessionMode", and "env" are allowed (no credentials).`, - ); + return { + settings: null, + diagnostics: [ + { + path, + message: `Invalid JSON in local settings file: ${path}`, + fix: `Fix JSON syntax in ${path}, or delete the file to fall back to global settings only.`, + }, + ], + }; } - const s = parsed as Record; - const optional: OptionalLocalSettingsFields = { - provider: s.provider as string | undefined, - model: s.model as string | undefined, - reasoningEffort: s.reasoningEffort as ReasoningEffort | undefined, - mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, - sessionMode: - s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, - env: s.env as Record | undefined, - }; - return pickDefined(optional); + return coerceLocalSettings(path, parsed); +} + +export async function loadLocalSettings(path: string): Promise { + // Fail open: never throw for schema/unknown-key problems. Callers that need + // diagnostics should use loadLocalSettingsResult. + const { settings } = await loadLocalSettingsResult(path); + return settings; } // Resolve the base for a read-modify-write of the global settings file. diff --git a/src/settings.test.ts b/src/settings.test.ts index 64f315f9a..f1ea9aaa9 100644 --- a/src/settings.test.ts +++ b/src/settings.test.ts @@ -336,13 +336,26 @@ describe("loaders", () => { } }); - test("loadLocalSettings rejects credentials", async () => { + test("loadLocalSettings fails open on credentials and unknown keys", async () => { const dir = await mkdtemp(join(tmpdir(), "ic-settings-")); try { await mkdir(join(dir, ".corbits"), { recursive: true }); const path = join(dir, ".corbits", "settings.json"); - await writeFile(path, JSON.stringify({ provider: "a", apiKey: "leak" })); - await expect(loadLocalSettings(path)).rejects.toThrow(/no credentials/); + await writeFile( + path, + JSON.stringify({ provider: "a", model: "m1", apiKey: "leak", providers: {}, weird: true }), + ); + // Must not throw — app starts with known keys applied. + const loaded = await loadLocalSettings(path); + expect(loaded).toEqual({ provider: "a", model: "m1" }); + // Credentials never load. + expect(loaded).not.toHaveProperty("apiKey"); + const { loadLocalSettingsResult } = await import("./config/settings.js"); + const result = await loadLocalSettingsResult(path); + expect(result.settings).toEqual({ provider: "a", model: "m1" }); + expect(result.diagnostics.length).toBeGreaterThan(0); + expect(result.diagnostics.some((d) => /credential|apiKey|unknown/i.test(d.message))).toBe(true); + expect(result.diagnostics.every((d) => d.fix.length > 0)).toBe(true); } finally { await rm(dir, { recursive: true, force: true }); } @@ -641,13 +654,18 @@ describe("saveLocalSettings", () => { } }); - test("loadLocalSettings rejects an invalid reasoningEffort", async () => { + test("loadLocalSettings fails open on invalid reasoningEffort", async () => { const dir = await mkdtemp(join(tmpdir(), "ic-settings-")); try { await mkdir(join(dir, ".corbits"), { recursive: true }); const path = join(dir, ".corbits", "settings.json"); await writeFile(path, JSON.stringify({ model: "m", reasoningEffort: "legendary" })); - await expect(loadLocalSettings(path)).rejects.toThrow(/reasoningEffort/); + // Fail open: keep model, drop invalid effort, surface diagnostic. + expect(await loadLocalSettings(path)).toEqual({ model: "m" }); + const { loadLocalSettingsResult } = await import("./config/settings.js"); + const result = await loadLocalSettingsResult(path); + expect(result.settings).toEqual({ model: "m" }); + expect(result.diagnostics.some((d) => /reasoningEffort/i.test(d.message))).toBe(true); } finally { await rm(dir, { recursive: true, force: true }); } diff --git a/src/tui/app.tsx b/src/tui/app.tsx index 27a869d05..90775de63 100644 --- a/src/tui/app.tsx +++ b/src/tui/app.tsx @@ -196,6 +196,15 @@ export type AppProps = { /** One-line passive notice shown once in the top-of-scrollback banner on * the first run telemetry is active. Undefined/empty renders nothing. */ telemetryNotice?: string; + /** + * Fail-open settings diagnostics (unknown keys, invalid JSON, stripped + * credentials). Shown as a dismissible notice on the main screen. + */ + settingsDiagnostics?: readonly { + path: string; + message: string; + fix: string; + }[]; /** Markdown release notes shown once after upgrade in the session banner. */ whatsNewMarkdown?: string; /** Whether anonymous telemetry is currently enabled, for the settings toggle. */ @@ -266,6 +275,7 @@ export function App({ subAgentSessions, goalApi, telemetryNotice, + settingsDiagnostics, whatsNewMarkdown, telemetryEnabled = false, onChangeTelemetryEnabled, @@ -334,6 +344,14 @@ export function App({ const [pluginsOpen, setPluginsOpen] = useState(false); const [permissionEntries, setPermissionEntries] = useState([]); const [commandMessage, setCommandMessage] = useState(null); + // Seed from fail-open settings load so unknown/invalid keys never crash + // startup — the operator sees the problem and fix on the main screen. + const [settingsNotice, setSettingsNotice] = useState(() => { + if (settingsDiagnostics === undefined || settingsDiagnostics.length === 0) return null; + return settingsDiagnostics + .map((d) => `Settings warning: ${d.message}\n Fix: ${d.fix}`) + .join("\n"); + }); // Tracks the live auto-mode flag so SHIFT+TAB can toggle (not only enable). // Seeded from config.auto / --no-auto; gate is updated via onToggleAuto. const [autoEnabled, setAutoEnabled] = useState(initialAuto); @@ -1204,6 +1222,12 @@ export function App({ removeCodexProfileEverywhere={removeCodexProfileEverywhere} /> {mcpStatus.needsAuth.length > 0 && } + {settingsNotice !== null && ( + + {settingsNotice} + Press Esc to dismiss settings warnings + + )} {commandMessage !== null && ( {commandMessage.split("\n").map((line, i) => ( diff --git a/src/tui/runner.tsx b/src/tui/runner.tsx index face8712d..27633bac7 100644 --- a/src/tui/runner.tsx +++ b/src/tui/runner.tsx @@ -1363,6 +1363,9 @@ export async function runTUI(initialConfig: Config): Promise { globalOnboardingPath={trueGlobalSettingsPath} globallyOnboarded={globallyOnboarded} {...(telemetryNotice !== undefined ? { telemetryNotice } : {})} + {...(config.settingsDiagnostics !== undefined && config.settingsDiagnostics.length > 0 + ? { settingsDiagnostics: config.settingsDiagnostics } + : {})} {...(whatsNewMarkdown !== undefined ? { whatsNewMarkdown } : {})} telemetryEnabled={liveTelemetryIntent} onChangeTelemetryEnabled={(enabled) => { From d95a2a3370d26827b5deafedf7a384c4dc60c944 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 4 Aug 2026 20:11:44 -0700 Subject: [PATCH 2/5] Dismiss settings diagnostics with Esc; coerce effort via isReasoningEffort Wire settingsNotice into the global keymap so Esc clears the fail-open banner. Use the shared isReasoningEffort helper so all effort levels survive coerce. Budget two chrome rows for the diagnostics banner. --- src/config/settings.ts | 9 +++------ src/tui/app.tsx | 3 +++ src/tui/chrome-geometry.ts | 3 +++ src/tui/hooks/use-keymap.test.ts | 24 ++++++++++++++++++++++++ src/tui/hooks/use-keymap.ts | 7 +++++++ 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/config/settings.ts b/src/config/settings.ts index fddf126ce..a0a7d3def 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -6,7 +6,7 @@ import { dirname, join } from "node:path"; import { type } from "arktype"; import { SETTINGS_DIR_NAME } from "../branding.js"; -import { REASONING_EFFORTS, type ReasoningEffort } from "../provider/reasoning-effort.js"; +import { REASONING_EFFORTS, isReasoningEffort, type ReasoningEffort } from "../provider/reasoning-effort.js"; import { isSessionMode, type SessionMode } from "./session-mode.js"; // A configured inference provider. `apiKey` is secret and lives only in the @@ -725,10 +725,7 @@ function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadRe const optional: OptionalLocalSettingsFields = { provider: typeof s.provider === "string" ? s.provider : undefined, model: typeof s.model === "string" ? s.model : undefined, - reasoningEffort: - s.reasoningEffort === "low" || s.reasoningEffort === "medium" || s.reasoningEffort === "high" - ? s.reasoningEffort - : undefined, + reasoningEffort: isReasoningEffort(s.reasoningEffort) ? s.reasoningEffort : undefined, mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, sessionMode: s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, @@ -752,7 +749,7 @@ function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadRe diagnostics.push({ path, message: `reasoningEffort in ${path} was invalid and was ignored.`, - fix: 'Use "low", "medium", or "high".', + fix: `Use one of: ${REASONING_EFFORTS.join(", ")}.`, }); } if (diagnostics.length === 0) { diff --git a/src/tui/app.tsx b/src/tui/app.tsx index 90775de63..862374a54 100644 --- a/src/tui/app.tsx +++ b/src/tui/app.tsx @@ -567,6 +567,7 @@ export function App({ mcpNeedsAuthCount: mcpStatus.needsAuth.length, commandMessageRows: commandMessage === null ? 0 : Math.max(1, commandMessage.split("\n").length), + settingsNoticePresent: settingsNotice !== null, goalChromeRows, taskChromeRows, pluginChromeRows, @@ -800,6 +801,7 @@ export function App({ copyModeOpen, agentsNavOpen, enteredSession: enteredSession !== undefined, + settingsNoticePresent: settingsNotice !== null, commandPaletteOpen: inputValue.startsWith("/") && ( !inputValue.includes(" ") || listCommands().some( @@ -827,6 +829,7 @@ export function App({ } }, closeHookPanel: () => setHookPanelOpen(false), + dismissSettingsNotice: () => setSettingsNotice(null), scrollUp: () => activeScroll.scrollUp(visibleRows), scrollDown: () => activeScroll.scrollDown(visibleRows), scrollToBottom: () => activeScroll.scrollToBottom(), diff --git a/src/tui/chrome-geometry.ts b/src/tui/chrome-geometry.ts index 02166dea8..7d6e82890 100644 --- a/src/tui/chrome-geometry.ts +++ b/src/tui/chrome-geometry.ts @@ -72,6 +72,7 @@ export function extraChromeRowCount(args: { mcpNeedsAuthCount: number; /** Rows reserved for the command feedback banner (0 when absent). */ commandMessageRows: number; + settingsNoticePresent?: boolean; goalChromeRows: number; taskChromeRows: number; pluginChromeRows: number; @@ -87,6 +88,8 @@ export function extraChromeRowCount(args: { return ( (args.mcpNeedsAuthCount > 0 ? 1 : 0) + args.commandMessageRows + + // Settings diagnostics banner is multi-line; budget 2 rows (message + Esc hint). + (args.settingsNoticePresent === true ? 2 : 0) + args.goalChromeRows + args.taskChromeRows + args.pluginChromeRows + diff --git a/src/tui/hooks/use-keymap.test.ts b/src/tui/hooks/use-keymap.test.ts index ed25442da..67f9408f9 100644 --- a/src/tui/hooks/use-keymap.test.ts +++ b/src/tui/hooks/use-keymap.test.ts @@ -37,6 +37,7 @@ const noopActions: KeymapActions = { agentsNavCancel: () => undefined, agentsNavKill: () => undefined, exitEnteredSession: () => undefined, + dismissSettingsNotice: () => undefined, }; const baseContext: KeymapContext = { @@ -53,6 +54,7 @@ const baseContext: KeymapContext = { copyModeOpen: false, agentsNavOpen: false, enteredSession: false, + settingsNoticePresent: false, }; describe("handleKey quota cancel", () => { @@ -198,4 +200,26 @@ describe("agents nav and enter-session", () => { handleKey("x", key(), ctx, actions, 0, 0); expect(calls).toEqual(["kill"]); }); + + test("Esc dismisses settings notice before double-Esc stop logic", () => { + let dismissed = false; + let stopped = false; + const actions: KeymapActions = { + ...noopActions, + dismissSettingsNotice: () => { + dismissed = true; + }, + requestStop: () => { + stopped = true; + }, + }; + const ctx: KeymapContext = { + ...baseContext, + settingsNoticePresent: true, + isRunning: true, + }; + handleKey("", key({ escape: true }), ctx, actions, 0, 0); + expect(dismissed).toBe(true); + expect(stopped).toBe(false); + }); }); \ No newline at end of file diff --git a/src/tui/hooks/use-keymap.ts b/src/tui/hooks/use-keymap.ts index caa7625a7..599a12d57 100644 --- a/src/tui/hooks/use-keymap.ts +++ b/src/tui/hooks/use-keymap.ts @@ -21,6 +21,8 @@ export type KeymapContext = { agentsNavOpen: boolean; // Observing a sub-agent session (read-only enter). enteredSession: boolean; + // Fail-open settings diagnostics banner on the main screen. + settingsNoticePresent: boolean; }; export type KeymapActions = { @@ -55,6 +57,7 @@ export type KeymapActions = { // Abort the selected (nav) or focused (entered) running sub-agent. agentsNavKill: () => void; exitEnteredSession: () => void; + dismissSettingsNotice: () => void; }; // Pure dispatch function — separated from the hook so it can be unit tested @@ -171,6 +174,10 @@ export function handleKey( actions.closeHookPanel(); return 0; } + if (context.settingsNoticePresent) { + actions.dismissSettingsNotice(); + return 0; + } if (now - lastEscMs <= DOUBLE_ESC_MS) { if (context.hasInput) { actions.clearInput(); From 060027ba5d85d313a53bb2fa9ec8465c1d62e826 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 4 Aug 2026 21:15:34 -0700 Subject: [PATCH 3/5] Share pickLocalFields for strict and coerce paths One helper builds optional local-settings fields for both the clean strict load and fail-open coerce paths so the two maps cannot drift. --- src/config/settings.ts | 62 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/config/settings.ts b/src/config/settings.ts index a0a7d3def..f6250c8a6 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -673,6 +673,40 @@ const LOCAL_CREDENTIAL_KEYS = new Set([ "authorization", ]); +/** Pick known local-settings fields from a raw object (strict or fail-open). */ +function pickLocalFields( + s: Record, + mode: "strict" | "coerce", +): OptionalLocalSettingsFields { + if (mode === "strict") { + return { + provider: s.provider as string | undefined, + model: s.model as string | undefined, + reasoningEffort: s.reasoningEffort as ReasoningEffort | undefined, + mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, + sessionMode: + s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, + env: s.env as Record | undefined, + }; + } + return { + provider: typeof s.provider === "string" ? s.provider : undefined, + model: typeof s.model === "string" ? s.model : undefined, + reasoningEffort: isReasoningEffort(s.reasoningEffort) ? s.reasoningEffort : undefined, + mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, + sessionMode: + s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, + env: + s.env !== undefined && typeof s.env === "object" && s.env !== null && !Array.isArray(s.env) + ? Object.fromEntries( + Object.entries(s.env as Record).filter( + (e): e is [string, string] => typeof e[1] === "string", + ), + ) + : undefined, + }; +} + function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadResult { const diagnostics: SettingsLoadDiagnostic[] = []; if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { @@ -690,16 +724,7 @@ function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadRe const s = parsed as Record; // Valid strict path still returns cleanly with no diagnostics. if (isLocalSettings(parsed)) { - const optional: OptionalLocalSettingsFields = { - provider: s.provider as string | undefined, - model: s.model as string | undefined, - reasoningEffort: s.reasoningEffort as ReasoningEffort | undefined, - mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, - sessionMode: - s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, - env: s.env as Record | undefined, - }; - return { settings: pickDefined(optional), diagnostics: [] }; + return { settings: pickDefined(pickLocalFields(s, "strict")), diagnostics: [] }; } const unknownKeys = Object.keys(s).filter((k) => !LOCAL_ALLOWED_KEYS.has(k)); @@ -722,22 +747,7 @@ function coerceLocalSettings(path: string, parsed: unknown): LocalSettingsLoadRe }); } - const optional: OptionalLocalSettingsFields = { - provider: typeof s.provider === "string" ? s.provider : undefined, - model: typeof s.model === "string" ? s.model : undefined, - reasoningEffort: isReasoningEffort(s.reasoningEffort) ? s.reasoningEffort : undefined, - mcpServers: s.mcpServers !== undefined ? normalizeMcpServers(s.mcpServers) : undefined, - sessionMode: - s.sessionMode === "single" || s.sessionMode === "orchestrator" ? s.sessionMode : undefined, - env: - s.env !== undefined && typeof s.env === "object" && s.env !== null && !Array.isArray(s.env) - ? Object.fromEntries( - Object.entries(s.env as Record).filter( - (e): e is [string, string] => typeof e[1] === "string", - ), - ) - : undefined, - }; + const optional = pickLocalFields(s, "coerce"); if (s.mcpServers !== undefined && optional.mcpServers === undefined) { diagnostics.push({ path, From d0a480fa12ace01fdf9b9f0b395b2ed972311e1d Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 4 Aug 2026 22:03:09 -0700 Subject: [PATCH 4/5] Prevent local settings clobber and surface exec diagnostics Add loadLocalSettingsWriteBase so session-mode RMW skips unusable files instead of writing {} over broken JSON. Emit settings diagnostics to stderr on exec so fail-open is never silent outside the TUI. --- src/config/settings.ts | 16 ++++++++++++++++ src/index.ts | 7 +++++++ src/settings.test.ts | 42 ++++++++++++++++++++++++++++++++++++++++++ src/tui/runner.tsx | 8 ++++++-- 4 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/config/settings.ts b/src/config/settings.ts index f6250c8a6..d70d69f07 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -807,6 +807,22 @@ export async function loadLocalSettings(path: string): Promise { + try { + const result = await loadLocalSettingsResult(path); + if (result.settings !== null) return result.settings; + // Absent (ENOENT) returns null settings with empty diagnostics. + if (result.diagnostics.length === 0) return {}; + return null; + } catch { + return null; + } +} + // Resolve the base for a read-modify-write of the global settings 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 diff --git a/src/index.ts b/src/index.ts index ce67e7cbb..23aa76c92 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,6 +20,13 @@ export async function mainWithRunners( runners: Runners, ): Promise { const config = await loadConfig(argv, { allowUnconfigured: true }); + // Exec has no Ink surface for settings diagnostics — fail-open must still + // tell the operator what was ignored and how to fix it. + if (config.configured && config.command === "exec" && config.settingsDiagnostics !== undefined) { + for (const d of config.settingsDiagnostics) { + process.stderr.write(`settings: ${d.message}\n fix: ${d.fix}\n`); + } + } // Always the TRUE global settings file, never config.globalSettingsPath — // that's the --config override file when one was given, and splitting // telemetry across two files means the installationId lands somewhere the diff --git a/src/settings.test.ts b/src/settings.test.ts index f1ea9aaa9..f059bfce7 100644 --- a/src/settings.test.ts +++ b/src/settings.test.ts @@ -7,6 +7,7 @@ import { isLocalSettings, isSettings, loadLocalSettings, + loadLocalSettingsWriteBase, loadSettings, normalizeOpenAICompatibleBaseURL, resolveProvider, @@ -361,6 +362,47 @@ describe("loaders", () => { } }); + test("loadLocalSettings fails open on invalid JSON with diagnostics", async () => { + const dir = await mkdtemp(join(tmpdir(), "ic-settings-")); + try { + const path = join(dir, "settings.json"); + await writeFile(path, "{ not json"); + expect(await loadLocalSettings(path)).toBeNull(); + const { loadLocalSettingsResult } = await import("./config/settings.js"); + const result = await loadLocalSettingsResult(path); + expect(result.settings).toBeNull(); + expect(result.diagnostics.some((d) => /Invalid JSON/i.test(d.message))).toBe(true); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("loadLocalSettingsWriteBase distinguishes absent, cleaned, and unusable", async () => { + const dir = await mkdtemp(join(tmpdir(), "ic-settings-")); + try { + const path = join(dir, "settings.json"); + // Absent: empty base is safe to create. + expect(await loadLocalSettingsWriteBase(path)).toEqual({}); + + // Partial fail-open: cleaned known fields are the base. + await writeFile( + path, + JSON.stringify({ provider: "a", model: "m1", apiKey: "leak", weird: true }), + ); + expect(await loadLocalSettingsWriteBase(path)).toEqual({ provider: "a", model: "m1" }); + + // Invalid JSON: skip write — do not collapse to {}. + await writeFile(path, "{ not json"); + expect(await loadLocalSettingsWriteBase(path)).toBeNull(); + + // Non-object: skip write. + await writeFile(path, JSON.stringify(["not", "object"])); + expect(await loadLocalSettingsWriteBase(path)).toBeNull(); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + test("loadSettings preserves tools block through a round trip", async () => { const dir = await mkdtemp(join(tmpdir(), "ic-settings-")); try { diff --git a/src/tui/runner.tsx b/src/tui/runner.tsx index 27633bac7..bce9a33ab 100644 --- a/src/tui/runner.tsx +++ b/src/tui/runner.tsx @@ -17,6 +17,7 @@ import { buildCodexSource, buildOpenAISource, buildXaiSource, type Config } from import { globalSettingsPath, loadLocalSettings, + loadLocalSettingsWriteBase, loadGlobalSettingsWriteBase, loadSettings, localSettingsPath, @@ -1441,8 +1442,11 @@ export async function runTUI(initialConfig: Config): Promise { 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 }; + const base = await loadLocalSettingsWriteBase(path); + // Skip write when the file exists but is unreadable/unusable so we + // never wipe a broken selection down to only sessionMode. + if (base === null) return; + const next: LocalSettings = { ...base, sessionMode: mode }; await saveLocalSettings(path, next); } else { const current = await loadSettings(config.globalSettingsPath).catch(() => null); From 37f5c262180245bb086d5917870b3ec15f60946d Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 4 Aug 2026 22:40:22 -0700 Subject: [PATCH 5/5] Budget multi-line settings banner and keep diagnostics on unconfigured path --- src/config.test.ts | 23 +++++++++++++++++++++++ src/config/index.ts | 9 +++++++++ src/index.ts | 9 ++++++--- src/tui/app.tsx | 7 ++++++- src/tui/chrome-geometry.ts | 16 +++++++++++++--- 5 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/config.test.ts b/src/config.test.ts index 0cf2928e1..ba28c69c5 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -94,6 +94,29 @@ describe("loadConfig", () => { } }); + test("threads local settings diagnostics on unconfigured early return", async () => { + const cwd = await emptyCwd(); + try { + await mkdir(join(cwd, ".corbits"), { recursive: true }); + await writeFile( + join(cwd, ".corbits", "settings.json"), + JSON.stringify({ unknownKey: true, anotherJunk: 1 }), + ); + const result = await loadConfig(["--cwd", cwd, "do it"], { + globalSettingsPath: NO_SETTINGS, + allowUnconfigured: true, + }); + expect(result.configured).toBe(false); + if (result.configured === false) { + expect(result.settingsDiagnostics).toBeDefined(); + expect(result.settingsDiagnostics!.length).toBeGreaterThan(0); + expect(result.settingsDiagnostics!.some((d) => /unknown/i.test(d.message))).toBe(true); + } + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + test("UnconfiguredConfig.globalSettingsPath reflects --config path, not the global default", async () => { const cwd = await emptyCwd(); try { diff --git a/src/config/index.ts b/src/config/index.ts index 4d80d24fe..cbeda3427 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -269,6 +269,12 @@ export type UnconfiguredConfig = { globalSettingsPath: string; // The original error message, used for non-TUI (exec) error output. providerError: string; + /** + * Fail-open diagnostics from local settings load. Still threaded on the + * unconfigured path so junk local files surface via stderr/banner rather + * than disappearing when provider setup fails early. + */ + settingsDiagnostics?: SettingsLoadDiagnostic[]; }; export type LoadConfigOptions = { @@ -458,6 +464,9 @@ export async function loadConfig( command, globalSettingsPath: effectiveSettingsPath, providerError: err instanceof Error ? err.message : String(err), + // Keep diagnostics even when provider setup fails early so junk local + // files still reach stderr (exec) / banner (TUI after onboarding). + ...(settingsDiagnostics.length > 0 ? { settingsDiagnostics } : {}), }; } diff --git a/src/index.ts b/src/index.ts index 23aa76c92..e32f83a57 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,9 +20,12 @@ export async function mainWithRunners( runners: Runners, ): Promise { const config = await loadConfig(argv, { allowUnconfigured: true }); - // Exec has no Ink surface for settings diagnostics — fail-open must still - // tell the operator what was ignored and how to fix it. - if (config.configured && config.command === "exec" && config.settingsDiagnostics !== undefined) { + // Exec has no Ink banner; unconfigured TUI goes to onboarding without the + // main-screen notice. Surface fail-open diagnostics on stderr for those + // paths so junk local files are never silent. + const surfaceDiagnosticsOnStderr = + config.command === "exec" || !config.configured; + if (surfaceDiagnosticsOnStderr && config.settingsDiagnostics !== undefined) { for (const d of config.settingsDiagnostics) { process.stderr.write(`settings: ${d.message}\n fix: ${d.fix}\n`); } diff --git a/src/tui/app.tsx b/src/tui/app.tsx index 862374a54..810d5ba9d 100644 --- a/src/tui/app.tsx +++ b/src/tui/app.tsx @@ -84,6 +84,7 @@ import { taskChromeRowCount, pluginChromeRowCount, extraChromeRowCount, + settingsNoticeRowCount, } from "./chrome-geometry.js"; import { progressChromeRowCount } from "./chrome-zones.js"; import { @@ -567,7 +568,11 @@ export function App({ mcpNeedsAuthCount: mcpStatus.needsAuth.length, commandMessageRows: commandMessage === null ? 0 : Math.max(1, commandMessage.split("\n").length), - settingsNoticePresent: settingsNotice !== null, + // Multi-line banner: 2 rows per diagnostic + Esc hint; 0 when dismissed. + settingsNoticeRows: + settingsNotice === null + ? 0 + : settingsNoticeRowCount(settingsDiagnostics?.length ?? 0), goalChromeRows, taskChromeRows, pluginChromeRows, diff --git a/src/tui/chrome-geometry.ts b/src/tui/chrome-geometry.ts index 7d6e82890..9dc4357e7 100644 --- a/src/tui/chrome-geometry.ts +++ b/src/tui/chrome-geometry.ts @@ -68,11 +68,22 @@ export function pluginChromeRowCount(args: { return 6 + list.length + widestCreds + 2; } +/** + * Rows for the settings diagnostics banner: each diagnostic is two lines + * (`Settings warning: …` + ` Fix: …`) plus one Esc-dismiss hint. Pass 0 when + * the banner is absent or dismissed. + */ +export function settingsNoticeRowCount(diagnosticCount: number): number { + if (diagnosticCount <= 0) return 0; + return diagnosticCount * 2 + 1; +} + export function extraChromeRowCount(args: { mcpNeedsAuthCount: number; /** Rows reserved for the command feedback banner (0 when absent). */ commandMessageRows: number; - settingsNoticePresent?: boolean; + /** Multi-line settings banner rows (0 when dismissed). Prefer settingsNoticeRowCount. */ + settingsNoticeRows?: number; goalChromeRows: number; taskChromeRows: number; pluginChromeRows: number; @@ -88,8 +99,7 @@ export function extraChromeRowCount(args: { return ( (args.mcpNeedsAuthCount > 0 ? 1 : 0) + args.commandMessageRows + - // Settings diagnostics banner is multi-line; budget 2 rows (message + Esc hint). - (args.settingsNoticePresent === true ? 2 : 0) + + (args.settingsNoticeRows ?? 0) + args.goalChromeRows + args.taskChromeRows + args.pluginChromeRows +