Merge reviewed OpenCode JSONC fix from #3590

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');
 }
 

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,

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'));