Skip to content

[WRONG BRANCH] fix: back up non-object config values instead of repairing #207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −0
Draft

[WRONG BRANCH] fix: back up non-object config values instead of repairing #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1816,6 +1816,12 @@ export function loadConfig(): OcxConfig {
warnDegradedUpstreamHostCircuitThreshold(parsed);
return normalizeClaudeSubagentEffort(normalizeNativeSubagentSync(config, parsed), parsed);
}
// Only object-shaped configs are repairable. Spreading another JSON value into
// defaults can manufacture a valid config and bypass the invalid-file backup.
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
warnAndBackupInvalidConfig(configPath, result.error);
return getDefaultConfig();
Comment on lines +1821 to +1823

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject arrays in the shared diagnostics path

When config.json contains a top-level array and a diagnostics-based operation runs before loadConfig()—for example, ocx config set or mutatePersistedConfig()—configDiagnosticsFromRaw() still passes the array to mergeConfigDefaults(), which spreads it over defaults and returns source: "file" with no error. The operation can then overwrite the malformed original with defaults without creating the backup this guard promises, and config admission can incorrectly authorize writes from it. Apply the same non-null, non-array object check in the shared diagnostics/merge path and add coverage through readConfigDiagnostics().

Useful? React with 👍 / 👎.

}
// Schema validation failed — merge defaults into the raw object instead of
// discarding it entirely, so pool accounts and providers survive a missing
// field like defaultProvider.
Expand Down
21 changes: 21 additions & 0 deletions tests/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,27 @@ describe("opencodex config defaults", () => {
}
});

test.each([
["number", "123"],
["boolean", "true"],
["string", JSON.stringify("not-an-object")],
["array", "[]"],
["null", "null"],
])("backs up a top-level %s instead of repairing it", (_kind, raw) => {
writeConfig(raw);
const errorSpy = spyOn(console, "error").mockImplementation(() => {});

try {
expect(loadConfig()).toEqual(getDefaultConfig());
const backups = backupNames();
expect(backups).toHaveLength(1);
expect(readFileSync(join(testDir, backups[0]), "utf-8")).toBe(raw);
expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Could not load opencodex config"));
} finally {
errorSpy.mockRestore();
}
});

test("backs up config when defaultProvider is absent from providers", () => {
writeConfig({
port: 10100,
Expand Down
Loading