fix(config): isolate broken JSONC layers and treat comment-only as empty

Address review findings on the #2923 fail-closed parse fix:

- Comment-only files produce ValueExpected with no JSON value; treat that
  as empty config instead of INVALID_JSONC. Partial object trees still throw.
- readConfigLayers no longer lets one unparseable layer abort valid sibling
  layers. Mutations still fail closed on the custom/user write target.

Co-authored-by: serkraser <serkraser@gmail.com>
This commit is contained in:
Cursor Agent
2026-08-15 05:13:32 +00:00
co-authored by serkraser
parent 4cc090130c
commit 35563dd78d
6 changed files with 210 additions and 34 deletions
+1 -1
View File
@@ -58,7 +58,7 @@ The webview CSP permits `blob:` only for `worker-src` so shared UI parsers can r
- `bridge-config-runtime.ts`
- Config and skills message handlers (`api:config/*`).
- Includes OpenCode resolution diagnostics parity handler used by shared UI (`/api/config/opencode-resolution`).
- OpenCode JSONC reads in `opencodeConfig.ts` fail closed on any `jsonc-parser` error or non-object result (`INVALID_JSONC`) so mutations cannot rewrite a partial `$schema`-only stub over an existing config.
- OpenCode JSONC reads in `opencodeConfig.ts` fail closed on a partial or non-object `jsonc-parser` tree (`INVALID_JSONC`) so mutations cannot rewrite a `$schema`-only stub over an existing config. Comment-only files read as empty. A broken layer is omitted from the merge and recorded on `layerErrors`; valid sibling layers still load, and writes still refuse to overwrite the broken file.
- `bridge-settings-runtime.ts`
- Settings read/write and OpenCode skills discovery via API for bridge consumers.
@@ -93,4 +93,22 @@ describe('opencodeConfig JSONC parse safety (issue #2923)', () => {
assert.equal(rewritten.mcp.openproject.enabled, false);
assert.equal(fs.readFileSync(`${configPath}.openchamber.backup`, 'utf8'), VALID_CONFIG);
});
test('keeps a valid custom layer writable when a project layer is unparseable', () => {
const customPath = path.join(tempDir, 'custom.jsonc');
const projectDir = path.join(tempDir, 'project');
const projectFile = path.join(projectDir, '.opencode', 'opencode.jsonc');
fs.writeFileSync(customPath, VALID_CONFIG, 'utf8');
fs.mkdirSync(path.dirname(projectFile), { recursive: true });
fs.writeFileSync(projectFile, PARTIAL_PARSE_CONFIG, 'utf8');
process.env.OPENCODE_CONFIG = customPath;
updateMcpConfig('openproject', { enabled: false }, projectDir);
const rewritten = JSON.parse(fs.readFileSync(customPath, 'utf8'));
assert.deepEqual(rewritten.plugin, ['opencode-see-image']);
assert.equal(rewritten.mcp.openproject.enabled, false);
assert.equal(fs.readFileSync(projectFile, 'utf8'), PARTIAL_PARSE_CONFIG);
assert.equal(fs.existsSync(`${projectFile}.openchamber.backup`), false);
});
});
+78 -14
View File
@@ -564,9 +564,17 @@ const formatJsoncParseError = (filePath: string, errors: ParseError[]): string =
return `OpenCode configuration at ${filePath} contains invalid JSONC and cannot be loaded safely${location}`;
};
const isInvalidJsoncError = (error: unknown): error is Error & { code: string } =>
Boolean(error && typeof error === 'object' && 'code' in error && error.code === INVALID_JSONC);
const parseConfigObject = (content: string, filePath: string): Record<string, unknown> => {
const errors: ParseError[] = [];
const parsed = parseJsonc(content, errors, { allowTrailingComma: true });
// Comment-only / no JSON value: jsonc-parser returns undefined plus ValueExpected.
// That is empty config, not a partial tree. The data-loss bug is errors + object.
if (parsed === undefined) {
return {};
}
if (errors.length > 0 || !parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
throw codedError(formatJsoncParseError(filePath, errors), INVALID_JSONC);
}
@@ -603,20 +611,50 @@ const mergeConfigs = (base: Record<string, unknown>, override: Record<string, un
return result;
};
const readConfigLayer = (filePath?: string | null): {
config: Record<string, unknown>;
error: (Error & { code: string }) | null;
} => {
try {
return { config: readConfigFile(filePath), error: null };
} catch (error) {
if (isInvalidJsoncError(error)) {
console.error(error.message);
return { config: {}, error };
}
throw error;
}
};
const readConfigLayers = (workingDirectory?: string) => {
const { userPaths, projectPath, customPath } = getConfigPaths(workingDirectory);
const userPath = getPrimaryUserConfigPath(userPaths);
const userConfig = readConfigFile(userPath);
const projectConfig = readConfigFile(projectPath);
const customConfig = readConfigFile(customPath);
const mergedConfig = mergeConfigs(mergeConfigs(userConfig, projectConfig), customConfig);
const userLayer = readConfigLayer(userPath);
const projectLayer = readConfigLayer(projectPath);
const customLayer = readConfigLayer(customPath);
const mergedConfig = mergeConfigs(
mergeConfigs(userLayer.config, projectLayer.config),
customLayer.config,
);
const layerErrors: Array<{ path: string; code: string; message: string }> = [];
if (userLayer.error) {
layerErrors.push({ path: userPath, code: userLayer.error.code, message: userLayer.error.message });
}
if (projectLayer.error && projectPath) {
layerErrors.push({ path: projectPath, code: projectLayer.error.code, message: projectLayer.error.message });
}
if (customLayer.error && customPath) {
layerErrors.push({ path: customPath, code: customLayer.error.code, message: customLayer.error.message });
}
return {
userConfig,
projectConfig,
customConfig,
userConfig: userLayer.config,
projectConfig: projectLayer.config,
customConfig: customLayer.config,
mergedConfig,
paths: { userPath, projectPath, customPath }
paths: { userPath, projectPath, customPath },
layerErrors,
};
};
@@ -1402,22 +1440,45 @@ export const deleteMcpConfig = (name: string, workingDirectory?: string): void =
writeConfig(config, targetPath);
};
const getLayerError = (
layers: ReturnType<typeof readConfigLayers>,
filePath?: string | null,
) => {
if (!filePath) return null;
return layers.layerErrors.find((entry) => entry.path === filePath) || null;
};
const throwIfLayerError = (
layers: ReturnType<typeof readConfigLayers>,
filePath?: string | null,
) => {
const failed = getLayerError(layers, filePath);
if (!failed) return;
throw codedError(failed.message, failed.code);
};
const getJsonEntrySource = (
layers: ReturnType<typeof readConfigLayers>,
sectionKey: 'agent' | 'command' | 'mcp',
entryName: string
) => {
const { userConfig, projectConfig, customConfig, paths } = layers;
const customSection = (customConfig as Record<string, unknown>)?.[sectionKey] as Record<string, unknown> | undefined;
if (customSection?.[entryName] !== undefined) {
return { section: customSection[entryName], config: customConfig, path: paths.customPath, exists: true };
if (paths.customPath) {
throwIfLayerError(layers, paths.customPath);
const customSection = (customConfig as Record<string, unknown>)?.[sectionKey] as Record<string, unknown> | undefined;
if (customSection?.[entryName] !== undefined) {
return { section: customSection[entryName], config: customConfig, path: paths.customPath, exists: true };
}
}
const projectSection = (projectConfig as Record<string, unknown>)?.[sectionKey] as Record<string, unknown> | undefined;
if (projectSection?.[entryName] !== undefined) {
return { section: projectSection[entryName], config: projectConfig, path: paths.projectPath, exists: true };
if (paths.projectPath && !getLayerError(layers, paths.projectPath)) {
const projectSection = (projectConfig as Record<string, unknown>)?.[sectionKey] as Record<string, unknown> | undefined;
if (projectSection?.[entryName] !== undefined) {
return { section: projectSection[entryName], config: projectConfig, path: paths.projectPath, exists: true };
}
}
throwIfLayerError(layers, paths.userPath);
const userSection = (userConfig as Record<string, unknown>)?.[sectionKey] as Record<string, unknown> | undefined;
if (userSection?.[entryName] !== undefined) {
return { section: userSection[entryName], config: userConfig, path: paths.userPath, exists: true };
@@ -1432,11 +1493,14 @@ const getJsonWriteTarget = (
) => {
const { userConfig, projectConfig, customConfig, paths } = layers;
if (paths.customPath) {
throwIfLayerError(layers, paths.customPath);
return { config: customConfig, path: paths.customPath };
}
if (preferredScope === AGENT_SCOPE.PROJECT && paths.projectPath) {
throwIfLayerError(layers, paths.projectPath);
return { config: projectConfig, path: paths.projectPath };
}
throwIfLayerError(layers, paths.userPath);
return { config: userConfig, path: paths.userPath };
};
@@ -70,11 +70,11 @@ This module provides OpenCode server integration utilities for the web server ru
- `AGENT_SCOPE`, `COMMAND_SCOPE`, `SKILL_SCOPE`: Scope constants with USER and PROJECT values.
- `ensureDirs()`: Creates required OpenCode directories.
- `parseMdFile(filePath)`, `writeMdFile(filePath, frontmatter, body)`: Markdown file operations with YAML frontmatter.
- `getConfigPaths(workingDirectory)`, `readConfigLayers(workingDirectory)`, `readConfig(workingDirectory)`: Config file operations with layer merging (user, project, custom).
- `readConfigFile(filePath)`: Reads one config file. Empty/missing files return `{}`. Any `jsonc-parser` error or non-object result throws `INVALID_JSONC` — partial parse trees must never be treated as authoritative (avoids rewriting a `$schema`-only stub over a full config).
- `getConfigPaths(workingDirectory)`, `readConfigLayers(workingDirectory)`, `readConfig(workingDirectory)`: Config file operations with layer merging (user, project, custom). `readConfigLayers` isolates `INVALID_JSONC` per layer: a broken file is omitted from the merge (`{}` for that layer only), recorded on `layerErrors`, and does not block valid sibling layers. Writes still refuse to overwrite the broken file.
- `readConfigFile(filePath)`: Reads one config file. Missing, whitespace-only, and comment-only files (no JSON value) return `{}`. A `jsonc-parser` error that produces a partial or non-object tree throws `INVALID_JSONC` — partial parse trees must never be treated as authoritative (avoids rewriting a `$schema`-only stub over a full config).
- `writeConfig(config, filePath)`: Writes config with automatic backup. Refuses to overwrite an existing non-empty file that fails the same JSONC parse check.
- `getJsonEntrySource(layers, sectionKey, entryName)`: Resolves which config layer provides an entry.
- `getJsonWriteTarget(layers, preferredScope)`: Determines write target for config updates.
- `getJsonEntrySource(layers, sectionKey, entryName)`: Resolves which config layer provides an entry. A failed custom or user layer throws `INVALID_JSONC` instead of treating that file as empty. A failed project layer is skipped so a valid user/custom entry can still be found.
- `getJsonWriteTarget(layers, preferredScope)`: Determines write target for config updates. Throws `INVALID_JSONC` when the chosen target file is the unparseable layer.
- `getAncestors(startDir, stopDir)`, `findWorktreeRoot(startDir)`: Git worktree helpers.
- `isPromptFileReference(value)`, `resolvePromptFilePath(reference)`, `writePromptFile(filePath, content)`: Prompt file reference handling.
- `walkSkillMdFiles(rootDir)`: Recursively finds all SKILL.md files.
+72 -14
View File
@@ -185,6 +185,11 @@ function formatJsoncParseError(filePath, errors) {
function parseConfigObject(content, filePath) {
const errors = [];
const parsed = parseJsonc(content, errors, { allowTrailingComma: true });
// Comment-only / no JSON value: jsonc-parser returns undefined plus ValueExpected.
// That is empty config, not a partial tree. The data-loss bug is errors + object.
if (parsed === undefined) {
return {};
}
if (errors.length > 0 || !parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
const error = new Error(formatJsoncParseError(filePath, errors));
error.code = INVALID_JSONC;
@@ -239,20 +244,47 @@ function mergeConfigs(base, override) {
return result;
}
function readConfigLayer(filePath) {
try {
return { config: readConfigFile(filePath), error: null };
} catch (error) {
if (isInvalidJsoncError(error)) {
console.error(error.message);
return { config: {}, error };
}
throw error;
}
}
function readConfigLayers(workingDirectory) {
const { userPaths, projectPath, customPath } = getConfigPaths(workingDirectory);
const userPath = getPrimaryUserConfigPath(userPaths);
const userConfig = readConfigFile(userPath);
const projectConfig = readConfigFile(projectPath);
const customConfig = readConfigFile(customPath);
const mergedConfig = mergeConfigs(mergeConfigs(userConfig, projectConfig), customConfig);
const userLayer = readConfigLayer(userPath);
const projectLayer = readConfigLayer(projectPath);
const customLayer = readConfigLayer(customPath);
const mergedConfig = mergeConfigs(
mergeConfigs(userLayer.config, projectLayer.config),
customLayer.config,
);
const layerErrors = [];
if (userLayer.error) {
layerErrors.push({ path: userPath, code: userLayer.error.code, message: userLayer.error.message });
}
if (projectLayer.error && projectPath) {
layerErrors.push({ path: projectPath, code: projectLayer.error.code, message: projectLayer.error.message });
}
if (customLayer.error && customPath) {
layerErrors.push({ path: customPath, code: customLayer.error.code, message: customLayer.error.message });
}
return {
userConfig,
projectConfig,
customConfig,
userConfig: userLayer.config,
projectConfig: projectLayer.config,
customConfig: customLayer.config,
mergedConfig,
paths: { userPath, projectPath, customPath }
paths: { userPath, projectPath, customPath },
layerErrors,
};
}
@@ -299,18 +331,41 @@ function writeConfig(config, filePath = CONFIG_FILE) {
}
}
function getLayerError(layers, filePath) {
if (!filePath || !Array.isArray(layers?.layerErrors)) {
return null;
}
return layers.layerErrors.find((entry) => entry.path === filePath) || null;
}
function throwIfLayerError(layers, filePath) {
const failed = getLayerError(layers, filePath);
if (!failed) {
return;
}
const error = new Error(failed.message);
error.code = failed.code;
throw error;
}
function getJsonEntrySource(layers, sectionKey, entryName) {
const { userConfig, projectConfig, customConfig, paths } = layers;
const customSection = customConfig?.[sectionKey]?.[entryName];
if (customSection !== undefined) {
return { section: customSection, config: customConfig, path: paths.customPath, exists: true };
if (paths.customPath) {
throwIfLayerError(layers, paths.customPath);
const customSection = customConfig?.[sectionKey]?.[entryName];
if (customSection !== undefined) {
return { section: customSection, config: customConfig, path: paths.customPath, exists: true };
}
}
const projectSection = projectConfig?.[sectionKey]?.[entryName];
if (projectSection !== undefined) {
return { section: projectSection, config: projectConfig, path: paths.projectPath, exists: true };
if (paths.projectPath && !getLayerError(layers, paths.projectPath)) {
const projectSection = projectConfig?.[sectionKey]?.[entryName];
if (projectSection !== undefined) {
return { section: projectSection, config: projectConfig, path: paths.projectPath, exists: true };
}
}
throwIfLayerError(layers, paths.userPath);
const userSection = userConfig?.[sectionKey]?.[entryName];
if (userSection !== undefined) {
return { section: userSection, config: userConfig, path: paths.userPath, exists: true };
@@ -322,11 +377,14 @@ function getJsonEntrySource(layers, sectionKey, entryName) {
function getJsonWriteTarget(layers, preferredScope) {
const { userConfig, projectConfig, customConfig, paths } = layers;
if (paths.customPath) {
throwIfLayerError(layers, paths.customPath);
return { config: customConfig, path: paths.customPath };
}
if (preferredScope === AGENT_SCOPE.PROJECT && paths.projectPath) {
throwIfLayerError(layers, paths.projectPath);
return { config: projectConfig, path: paths.projectPath };
}
throwIfLayerError(layers, paths.userPath);
return { config: userConfig, path: paths.userPath };
}
@@ -3,7 +3,7 @@ import fs from 'fs';
import os from 'os';
import path from 'path';
import { parseMdFile, writeMdFile, readConfigFile, writeConfig } from './shared.js';
import { parseMdFile, writeMdFile, readConfigFile, readConfigLayers, writeConfig } from './shared.js';
import { updateAgent } from './agents.js';
import { updateMcpConfig } from './mcp.js';
@@ -337,4 +337,40 @@ describe('readConfigFile / writeConfig JSONC safety (issue #2923)', () => {
else process.env.OPENCODE_CONFIG = previousOpenCodeConfig;
}
});
it('returns an empty object for a comment-only config file', () => {
const file = writeFixture('comments.jsonc', '// placeholder\n/* still empty */\n');
expect(readConfigFile(file)).toEqual({});
});
it('keeps a valid custom layer readable when a project layer is unparseable', () => {
const custom = writeFixture('custom.jsonc', VALID_CONFIG);
const projectDir = path.join(FIXTURE_DIR, 'project');
const projectFile = writeFixture(path.join('project', '.opencode', 'opencode.jsonc'), PARTIAL_PARSE_CONFIG);
const previousOpenCodeConfig = process.env.OPENCODE_CONFIG;
try {
process.env.OPENCODE_CONFIG = custom;
const layers = readConfigLayers(projectDir);
expect(layers.customConfig.plugin).toEqual(['opencode-see-image']);
expect(layers.projectConfig).toEqual({});
expect(layers.mergedConfig.plugin).toEqual(['opencode-see-image']);
expect(layers.layerErrors).toEqual([
expect.objectContaining({
path: projectFile,
code: 'INVALID_JSONC',
}),
]);
updateMcpConfig('openproject', { enabled: false }, projectDir);
const rewritten = JSON.parse(fs.readFileSync(custom, 'utf8'));
expect(rewritten.plugin).toEqual(['opencode-see-image']);
expect(rewritten.mcp.openproject.enabled).toBe(false);
expect(fs.readFileSync(projectFile, 'utf8')).toBe(PARTIAL_PARSE_CONFIG);
expect(fs.existsSync(`${projectFile}.openchamber.backup`)).toBe(false);
} finally {
if (previousOpenCodeConfig === undefined) delete process.env.OPENCODE_CONFIG;
else process.env.OPENCODE_CONFIG = previousOpenCodeConfig;
}
});
});