Skip to content

Commit 10516d9

Browse files
committed
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.
1 parent a793ff5 commit 10516d9

4 files changed

Lines changed: 71 additions & 2 deletions

File tree

src/config/settings.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -807,6 +807,22 @@ export async function loadLocalSettings(path: string): Promise<LocalSettings | n
807807
return settings;
808808
}
809809

810+
// Resolve the base for a read-modify-write of the local selection file.
811+
// Absent file → empty base (create OK). Partial fail-open → cleaned fields.
812+
// Invalid JSON / unreadable / fully unusable → null so the caller skips the
813+
// write instead of collapsing to {} and wiping the file.
814+
export async function loadLocalSettingsWriteBase(path: string): Promise<LocalSettings | null> {
815+
try {
816+
const result = await loadLocalSettingsResult(path);
817+
if (result.settings !== null) return result.settings;
818+
// Absent (ENOENT) returns null settings with empty diagnostics.
819+
if (result.diagnostics.length === 0) return {};
820+
return null;
821+
} catch {
822+
return null;
823+
}
824+
}
825+
810826
// Resolve the base for a read-modify-write of the global settings file.
811827
// An absent file yields a fresh minimal base; an unreadable or invalid file
812828
// yields null so the caller skips the write — falling back to a minimal base

src/index.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,13 @@ export async function mainWithRunners(
2020
runners: Runners,
2121
): Promise<number> {
2222
const config = await loadConfig(argv, { allowUnconfigured: true });
23+
// Exec has no Ink surface for settings diagnostics — fail-open must still
24+
// tell the operator what was ignored and how to fix it.
25+
if (config.configured && config.command === "exec" && config.settingsDiagnostics !== undefined) {
26+
for (const d of config.settingsDiagnostics) {
27+
process.stderr.write(`settings: ${d.message}\n fix: ${d.fix}\n`);
28+
}
29+
}
2330
// Always the TRUE global settings file, never config.globalSettingsPath —
2431
// that's the --config override file when one was given, and splitting
2532
// telemetry across two files means the installationId lands somewhere the

src/settings.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
isLocalSettings,
88
isSettings,
99
loadLocalSettings,
10+
loadLocalSettingsWriteBase,
1011
loadSettings,
1112
normalizeOpenAICompatibleBaseURL,
1213
resolveProvider,
@@ -361,6 +362,47 @@ describe("loaders", () => {
361362
}
362363
});
363364

365+
test("loadLocalSettings fails open on invalid JSON with diagnostics", async () => {
366+
const dir = await mkdtemp(join(tmpdir(), "ic-settings-"));
367+
try {
368+
const path = join(dir, "settings.json");
369+
await writeFile(path, "{ not json");
370+
expect(await loadLocalSettings(path)).toBeNull();
371+
const { loadLocalSettingsResult } = await import("./config/settings.js");
372+
const result = await loadLocalSettingsResult(path);
373+
expect(result.settings).toBeNull();
374+
expect(result.diagnostics.some((d) => /Invalid JSON/i.test(d.message))).toBe(true);
375+
} finally {
376+
await rm(dir, { recursive: true, force: true });
377+
}
378+
});
379+
380+
test("loadLocalSettingsWriteBase distinguishes absent, cleaned, and unusable", async () => {
381+
const dir = await mkdtemp(join(tmpdir(), "ic-settings-"));
382+
try {
383+
const path = join(dir, "settings.json");
384+
// Absent: empty base is safe to create.
385+
expect(await loadLocalSettingsWriteBase(path)).toEqual({});
386+
387+
// Partial fail-open: cleaned known fields are the base.
388+
await writeFile(
389+
path,
390+
JSON.stringify({ provider: "a", model: "m1", apiKey: "leak", weird: true }),
391+
);
392+
expect(await loadLocalSettingsWriteBase(path)).toEqual({ provider: "a", model: "m1" });
393+
394+
// Invalid JSON: skip write — do not collapse to {}.
395+
await writeFile(path, "{ not json");
396+
expect(await loadLocalSettingsWriteBase(path)).toBeNull();
397+
398+
// Non-object: skip write.
399+
await writeFile(path, JSON.stringify(["not", "object"]));
400+
expect(await loadLocalSettingsWriteBase(path)).toBeNull();
401+
} finally {
402+
await rm(dir, { recursive: true, force: true });
403+
}
404+
});
405+
364406
test("loadSettings preserves tools block through a round trip", async () => {
365407
const dir = await mkdtemp(join(tmpdir(), "ic-settings-"));
366408
try {

src/tui/runner.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { buildCodexSource, buildOpenAISource, buildXaiSource, type Config } from
1717
import {
1818
globalSettingsPath,
1919
loadLocalSettings,
20+
loadLocalSettingsWriteBase,
2021
loadGlobalSettingsWriteBase,
2122
loadSettings,
2223
localSettingsPath,
@@ -1441,8 +1442,11 @@ export async function runTUI(initialConfig: Config): Promise<number> {
14411442
onChangeSessionMode={async (mode, scope) => {
14421443
if (scope === "local") {
14431444
const path = localSettingsPath(config.cwd);
1444-
const existing = (await loadLocalSettings(path).catch(() => null)) ?? {};
1445-
const next: LocalSettings = { ...existing, sessionMode: mode };
1445+
const base = await loadLocalSettingsWriteBase(path);
1446+
// Skip write when the file exists but is unreadable/unusable so we
1447+
// never wipe a broken selection down to only sessionMode.
1448+
if (base === null) return;
1449+
const next: LocalSettings = { ...base, sessionMode: mode };
14461450
await saveLocalSettings(path, next);
14471451
} else {
14481452
const current = await loadSettings(config.globalSettingsPath).catch(() => null);

0 commit comments

Comments
 (0)