fix(install): reject unterminated comments, keep plugin shape check, handle CR endings

Address review findings on #3590:

- Unterminated block comments now throw instead of silently passing:
  {'plugin':[]} /* no longer parses, so the install keeps its
  source-context error for genuinely malformed configs (CodeRabbit,
  Greptile).
- Line-comment scanners stop at CR as well as LF, so CR-only line
  endings do not consume the rest of the file (CodeRabbit).
- activationIsInactive restores the 'plugin must be an array' shape
  check that the strict parser surfaced, so a non-array plugin no
  longer counts as inactive (Greptile).

Regression tests: unterminated comment with source context, CR-only
endings, and non-array plugin across apply/repair/doctor modes.
This commit is contained in:
Hemang Varshney committed 2026-10-15 17:49:10 +05:30
1 parent ced82ccf9f
commit 8e2cdefba6
3 files changed
+40 -3

No files matched your search

+8 -1
View File
@@ -451,9 +451,16 @@ function activationIsInactive(kind, operation, content) {
return content.toString('utf8') === getDisabledOpenCodePluginContent();
}
const text = content.toString('utf8');
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, operation.sourceRelativePath || operation.destinationPath);
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');
}
+8 -2
View File
@@ -65,6 +65,10 @@ function parseJsoncConfig(content, sourceRelativePath) {
return config;
}
function isLineBreak(ch) {
return ch === '\n' || ch === '\r';
}
/**
* Advance past whitespace and // or /* *\/ comments; returns the index of
* the next significant character.
@@ -74,12 +78,13 @@ function skipJsoncTrivia(input, start) {
for (;;) {
while (j < input.length && /\s/.test(input[j])) j += 1;
if (input[j] === '/' && input[j + 1] === '/') {
while (j < input.length && input[j] !== '\n') 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;
}
@@ -110,13 +115,14 @@ function stripJsonc(input) {
continue;
}
if (ch === '/' && input[i + 1] === '/') {
while (i < input.length && input[i] !== '\n') 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;
@@ -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;
@@ -328,6 +342,16 @@ function runTests() {
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'));