diff --git a/packages/vscode/src/DOCUMENTATION.md b/packages/vscode/src/DOCUMENTATION.md index e7d6e610..fe638f97 100644 --- a/packages/vscode/src/DOCUMENTATION.md +++ b/packages/vscode/src/DOCUMENTATION.md @@ -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. diff --git a/packages/vscode/src/opencodeConfig.config-parse.test.ts b/packages/vscode/src/opencodeConfig.config-parse.test.ts index bd6f08b2..c12572e8 100644 --- a/packages/vscode/src/opencodeConfig.config-parse.test.ts +++ b/packages/vscode/src/opencodeConfig.config-parse.test.ts @@ -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); + }); }); diff --git a/packages/vscode/src/opencodeConfig.ts b/packages/vscode/src/opencodeConfig.ts index 199f348e..50a40082 100644 --- a/packages/vscode/src/opencodeConfig.ts +++ b/packages/vscode/src/opencodeConfig.ts @@ -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 => { 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, override: Record; + 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, + filePath?: string | null, +) => { + if (!filePath) return null; + return layers.layerErrors.find((entry) => entry.path === filePath) || null; +}; + +const throwIfLayerError = ( + layers: ReturnType, + filePath?: string | null, +) => { + const failed = getLayerError(layers, filePath); + if (!failed) return; + throw codedError(failed.message, failed.code); +}; + const getJsonEntrySource = ( layers: ReturnType, sectionKey: 'agent' | 'command' | 'mcp', entryName: string ) => { const { userConfig, projectConfig, customConfig, paths } = layers; - const customSection = (customConfig as Record)?.[sectionKey] as Record | 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)?.[sectionKey] as Record | undefined; + if (customSection?.[entryName] !== undefined) { + return { section: customSection[entryName], config: customConfig, path: paths.customPath, exists: true }; + } } - const projectSection = (projectConfig as Record)?.[sectionKey] as Record | 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)?.[sectionKey] as Record | 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)?.[sectionKey] as Record | 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 }; }; diff --git a/packages/web/server/lib/opencode/DOCUMENTATION.md b/packages/web/server/lib/opencode/DOCUMENTATION.md index cbe9f6c8..cb35ed11 100644 --- a/packages/web/server/lib/opencode/DOCUMENTATION.md +++ b/packages/web/server/lib/opencode/DOCUMENTATION.md @@ -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. diff --git a/packages/web/server/lib/opencode/shared.js b/packages/web/server/lib/opencode/shared.js index f99360d8..87a105d4 100644 --- a/packages/web/server/lib/opencode/shared.js +++ b/packages/web/server/lib/opencode/shared.js @@ -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 }; } diff --git a/packages/web/server/lib/opencode/shared.test.js b/packages/web/server/lib/opencode/shared.test.js index a3e646b7..88384ae9 100644 --- a/packages/web/server/lib/opencode/shared.test.js +++ b/packages/web/server/lib/opencode/shared.test.js @@ -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; + } + }); });