diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index ff49ddf53..27906161d 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -11,7 +11,7 @@ const { const { readInstallState, writeInstallState } = require('../install-state'); const { assertHookConsentReady, - disableOpenCodeHookPluginRegistration, + parseJsoncConfig, getDisabledOpenCodePluginContent, getRecordedHookConsent, getOpenCodeActivationPathKind, @@ -451,9 +451,16 @@ function activationIsInactive(kind, operation, content) { return content.toString('utf8') === getDisabledOpenCodePluginContent(); } const text = content.toString('utf8'); - // Validate first so malformed JSON retains its source context. - disableOpenCodeHookPluginRegistration(text, operation.sourceRelativePath || operation.destinationPath); - const config = JSON.parse(text); + const relativePath = operation.sourceRelativePath || operation.destinationPath; + // JSONC-tolerant parse so a comment in the user's config does not crash + // the install; the ownership guard never writes this file (issue #3546). + const config = parseJsoncConfig(text, relativePath); + // Keep the same shape error the strict parser surfaced via + // disableOpenCodeHookPluginRegistration: a non-array plugin must not + // silently count as inactive. + if (config.plugin !== undefined && !Array.isArray(config.plugin)) { + throw new Error(`Invalid ${relativePath}: plugin must be an array`); + } return !Array.isArray(config.plugin) || !config.plugin.includes('./plugins'); } diff --git a/scripts/lib/install/hook-consent.js b/scripts/lib/install/hook-consent.js index 4876b681e..07cff400e 100644 --- a/scripts/lib/install/hook-consent.js +++ b/scripts/lib/install/hook-consent.js @@ -45,16 +45,101 @@ function normalizeOperationPath(value) { return String(value || '').replace(/\\/g, '/').toLowerCase(); } -function disableOpenCodeHookPluginRegistration(content, sourceRelativePath) { +/** + * Parse an OpenCode config the way OpenCode itself does: comments and the + * trailing commas they hide are legal (JSONC). The installer only ever + * reads these files (the ownership guard never writes user-owned configs), + * so a tolerant read prevents a membership check from crashing the install + * on a config OpenCode accepts. + */ +function parseJsoncConfig(content, sourceRelativePath) { let config; try { - config = JSON.parse(content); + config = JSON.parse(stripJsonc(content)); } catch (error) { throw new Error(`Failed to parse ${sourceRelativePath}: ${error.message}`); } if (!config || typeof config !== 'object' || Array.isArray(config)) { throw new Error(`Invalid ${sourceRelativePath}: expected a JSON object`); } + return config; +} + +function isLineBreak(ch) { + return ch === '\n' || ch === '\r'; +} + +/** + * Advance past whitespace and // or /* *\/ comments; returns the index of + * the next significant character. + */ +function skipJsoncTrivia(input, start) { + let j = start; + for (;;) { + while (j < input.length && /\s/.test(input[j])) j += 1; + if (input[j] === '/' && input[j + 1] === '/') { + while (j < input.length && !isLineBreak(input[j])) j += 1; + continue; + } + if (input[j] === '/' && input[j + 1] === '*') { + j += 2; + while (j < input.length && !(input[j] === '*' && input[j + 1] === '/')) j += 1; + if (j >= input.length) throw new Error('Unterminated block comment'); + j += 2; + continue; + } + return j; + } +} + +/** + * Strip // and /* *\/ comments and the trailing commas they hide, without + * touching string contents (URLs like "https://..." must survive intact). + */ +function stripJsonc(input) { + let out = ''; + let inString = false; + let escaped = false; + for (let i = 0; i < input.length; i += 1) { + const ch = input[i]; + if (inString) { + out += ch; + if (escaped) escaped = false; + else if (ch === '\\') escaped = true; + else if (ch === '"') inString = false; + continue; + } + if (ch === '"') { + inString = true; + out += ch; + continue; + } + if (ch === '/' && input[i + 1] === '/') { + while (i < input.length && !isLineBreak(input[i])) i += 1; + out += '\n'; + continue; + } + if (ch === '/' && input[i + 1] === '*') { + i += 2; + while (i < input.length && !(input[i] === '*' && input[i + 1] === '/')) i += 1; + if (i >= input.length) throw new Error('Unterminated block comment'); + i += 1; // consume the closing '/' + out += ' '; + continue; + } + if (ch === ',') { + const next = skipJsoncTrivia(input, i + 1); + // A comma before '}' or ']' (ignoring trivia) is a trailing comma + // JSON forbids. + if (input[next] === '}' || input[next] === ']') continue; + } + out += ch; + } + return out; +} + +function disableOpenCodeHookPluginRegistration(content, sourceRelativePath) { + const config = parseJsoncConfig(content, sourceRelativePath); if (config.plugin !== undefined && !Array.isArray(config.plugin)) { throw new Error(`Invalid ${sourceRelativePath}: plugin must be an array`); } @@ -328,6 +413,7 @@ module.exports = { assertHookConsentReady, disableUnselectedOpenCodeHooks, disableOpenCodeHookPluginRegistration, + parseJsoncConfig, getDisabledOpenCodePluginContent, getOpenCodeActivationPathKind, getOpenCodeSourceActivationKind, diff --git a/tests/lib/opencode-hook-consent-safety.test.js b/tests/lib/opencode-hook-consent-safety.test.js index 25c113493..a068d2a14 100644 --- a/tests/lib/opencode-hook-consent-safety.test.js +++ b/tests/lib/opencode-hook-consent-safety.test.js @@ -9,7 +9,7 @@ const { createFileSystemLoader } = require('./helpers/load-with-file-system'); const fileSystem = { ...fs }; const load = createFileSystemLoader(fileSystem); const { applyInstallPlan } = load(require.resolve('../../scripts/lib/install/apply')); -const { withHookConsent } = require('../../scripts/lib/install/hook-consent'); +const { parseJsoncConfig, withHookConsent } = require('../../scripts/lib/install/hook-consent'); const { createInstallState, readInstallState, writeInstallState } = require('../../scripts/lib/install-state'); const { buildDoctorReport, repairInstalledStates } = load(require.resolve('../../scripts/lib/install-lifecycle')); @@ -248,6 +248,20 @@ function runTests() { assert.strictEqual(fs.readFileSync(destination, 'utf8'), '{ malformed'); assert.deepStrictEqual(fs.readFileSync(value.installStatePath), before); })); + test(`${mode} reports a non-array plugin with source context`, () => fixture(value => { + const destination = path.join(value.targetRoot, 'opencode.json'); + fs.writeFileSync(destination, '{"plugin":"./plugins"}'); + const before = fs.readFileSync(value.installStatePath); + if (mode === 'apply') assert.throws(() => applyInstallPlan(value.declinePlan), /Invalid .*opencode\.json: plugin must be an array/); + else if (mode === 'repair') assert.match(repair(value).results[0].error, /plugin must be an array/); + else { + const result = buildDoctorReport({ repoRoot: value.sourceRoot, homeDir: value.homeDir, + projectRoot: value.homeDir, targets: ['opencode'] }).results[0]; + assert.match(JSON.stringify(result.issues), /plugin must be an array/); + } + assert.strictEqual(fs.readFileSync(destination, 'utf8'), '{"plugin":"./plugins"}'); + assert.deepStrictEqual(fs.readFileSync(value.installStatePath), before); + })); } test('apply rejects another enabled writer and repair while holding its target lease', () => fixture(value => { let checked = false; @@ -314,6 +328,53 @@ function runTests() { assert.strictEqual(fs.existsSync(`${value.installStatePath}.ecc.lock`), false); })); } + test('parseJsoncConfig tolerates comments and trailing commas without touching strings', () => { + const config = parseJsoncConfig(`{ + // my note + "plugin": [ + "ecc-universal", /* inline */ + ], + "endpoint": "https://example.com/path", // URL survives + "block": "/* not a comment */", +} +`, 'opencode.json'); + assert.deepStrictEqual(config.plugin, ['ecc-universal']); + assert.strictEqual(config.endpoint, 'https://example.com/path'); + assert.strictEqual(config.block, '/* not a comment */'); + }); + test('parseJsoncConfig rejects an unterminated block comment with source context', () => { + assert.throws( + () => parseJsoncConfig('{"plugin":[]} /* unfinished', 'opencode.json'), + /Failed to parse opencode\.json: Unterminated block comment/, + ); + }); + test('parseJsoncConfig stops line comments at CR-only line endings', () => { + const config = parseJsoncConfig('{"plugin":[] // note\r}', 'opencode.json'); + assert.deepStrictEqual(config.plugin, []); + }); + test(`fresh ${consent || 'default'} apply preserves inactive JSONC user config with comments`, () => fixture(value => { + fs.unlinkSync(value.installStatePath); + fs.unlinkSync(path.join(value.targetRoot, 'plugins/ecc-hooks.ts')); + const destination = path.join(value.targetRoot, 'opencode.json'); + // JSONC with a line comment, a block comment, trailing commas and a + // URL string: OpenCode accepts this; the installer must not crash on + // the read-only membership check (issue #3546). + const content = `{ + // my note + "plugin": [ + "ecc-universal", + ], + "endpoint": "https://example.com/path", /* keep */ +} +`; + fs.writeFileSync(destination, content); + const result = applyInstallPlan(withHookConsent(value.basePlan, consent)); + assert.strictEqual(result.applied, true); + assert.strictEqual(fs.readFileSync(destination, 'utf8'), content); + assert.ok(result.warnings.some(warning => warning.includes(`Skipped user-owned file ${destination}`))); + assert.ok(result.skippedOperations.some(operation => operation.destinationPath === destination)); + assert.strictEqual(fs.existsSync(`${value.installStatePath}.ecc.lock`), false); + })); test(`fresh ${consent || 'default'} apply still refuses active unrecorded config`, () => fixture(value => { fs.unlinkSync(value.installStatePath); fs.unlinkSync(path.join(value.targetRoot, 'plugins/ecc-hooks.ts'));