From ffe5319fe54e5b50b305ff0e5c0e526ccf224fc9 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Mon, 30 Mar 2026 13:52:51 -0700 Subject: [PATCH] fix(install): handle JSONC (comments) in settings.json without data loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When settings.json contains comments (// or /* */), which many CLI tools allow, JSON.parse() fails and readSettings() silently returned {}. This empty object was then written back by writeSettings(), destroying the user's entire configuration. Changes: - Add stripJsonComments() that handles line comments, block comments, trailing commas, and preserves comments inside string values - readSettings() tries standard JSON first (fast path), falls back to JSONC stripping on parse failure - On truly malformed files (even JSONC stripping fails), return null with a warning instead of silently returning {} — prevents data loss - All callers of readSettings() now guard against null return to skip settings modification rather than overwriting with empty object Closes #1461 Co-Authored-By: Claude Opus 4.6 --- bin/install.js | 87 ++++++++++++++-- tests/settings-jsonc.test.cjs | 186 ++++++++++++++++++++++++++++++++++ 2 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 tests/settings-jsonc.test.cjs diff --git a/bin/install.js b/bin/install.js index df69acc29..85066848d 100755 --- a/bin/install.js +++ b/bin/install.js @@ -364,14 +364,78 @@ function resolveOpencodeConfigPath(configDir) { } /** - * Read and parse settings.json, returning empty object if it doesn't exist + * Strip JSONC comments (// and /* *​/) from a string to produce valid JSON. + * Handles comments inside strings correctly (does not strip them). + */ +function stripJsonComments(text) { + let result = ''; + let i = 0; + let inString = false; + let stringChar = ''; + while (i < text.length) { + // Handle string literals — don't strip comments inside strings + if (inString) { + if (text[i] === '\\') { + result += text[i] + (text[i + 1] || ''); + i += 2; + continue; + } + if (text[i] === stringChar) { + inString = false; + } + result += text[i]; + i++; + continue; + } + // Start of string + if (text[i] === '"' || text[i] === "'") { + inString = true; + stringChar = text[i]; + result += text[i]; + i++; + continue; + } + // Line comment + if (text[i] === '/' && text[i + 1] === '/') { + // Skip to end of line + while (i < text.length && text[i] !== '\n') i++; + continue; + } + // Block comment + if (text[i] === '/' && text[i + 1] === '*') { + i += 2; + while (i < text.length && !(text[i] === '*' && text[i + 1] === '/')) i++; + i += 2; // skip closing */ + continue; + } + result += text[i]; + i++; + } + // Remove trailing commas before } or ] (common in JSONC) + return result.replace(/,\s*([}\]])/g, '$1'); +} + +/** + * Read and parse settings.json, returning empty object if it doesn't exist. + * Supports JSONC (JSON with comments) — many CLI tools allow comments in + * their settings files, so we strip them before parsing to avoid silent + * data loss from JSON.parse failures. */ function readSettings(settingsPath) { if (fs.existsSync(settingsPath)) { try { - return JSON.parse(fs.readFileSync(settingsPath, 'utf8')); + const raw = fs.readFileSync(settingsPath, 'utf8'); + // Try standard JSON first (fast path) + try { + return JSON.parse(raw); + } catch { + // Fall back to JSONC stripping + return JSON.parse(stripJsonComments(raw)); + } } catch (e) { - return {}; + // If even JSONC stripping fails, warn instead of silently returning {} + console.warn(' ' + yellow + '⚠' + reset + ' Warning: Could not parse ' + settingsPath + ' — file may be malformed. Existing settings preserved.'); + return null; } } return {}; @@ -402,11 +466,11 @@ function getCommitAttribution(runtime) { if (runtime === 'opencode') { const config = readSettings(resolveOpencodeConfigPath(getGlobalDir('opencode', null))); - result = config.disable_ai_attribution === true ? null : undefined; + result = (config && config.disable_ai_attribution === true) ? null : undefined; } else if (runtime === 'gemini') { // Gemini: check gemini settings.json for attribution config const settings = readSettings(path.join(getGlobalDir('gemini', explicitConfigDir), 'settings.json')); - if (!settings.attribution || settings.attribution.commit === undefined) { + if (!settings || !settings.attribution || settings.attribution.commit === undefined) { result = undefined; } else if (settings.attribution.commit === '') { result = null; @@ -416,7 +480,7 @@ function getCommitAttribution(runtime) { } else if (runtime === 'claude') { // Claude Code const settings = readSettings(path.join(getGlobalDir('claude', explicitConfigDir), 'settings.json')); - if (!settings.attribution || settings.attribution.commit === undefined) { + if (!settings || !settings.attribution || settings.attribution.commit === undefined) { result = undefined; } else if (settings.attribution.commit === '') { result = null; @@ -3558,6 +3622,10 @@ function uninstall(isGlobal, runtime = 'claude') { const settingsPath = path.join(targetDir, 'settings.json'); if (fs.existsSync(settingsPath)) { let settings = readSettings(settingsPath); + if (settings === null) { + console.log(` ${yellow}i${reset} Skipping settings.json cleanup — file could not be parsed`); + settings = {}; // prevent downstream crashes, but don't write back + } let settingsModified = false; // Remove GSD statusline if it references our hook @@ -4447,7 +4515,12 @@ function install(isGlobal, runtime = 'claude') { // Gemini and Antigravity use AfterTool instead of PostToolUse for post-tool hooks const postToolEvent = (runtime === 'gemini' || runtime === 'antigravity') ? 'AfterTool' : 'PostToolUse'; const settingsPath = path.join(targetDir, 'settings.json'); - const settings = validateHookFields(cleanupOrphanedHooks(readSettings(settingsPath))); + const rawSettings = readSettings(settingsPath); + if (rawSettings === null) { + console.log(' ' + yellow + 'i' + reset + ' Skipping settings.json configuration — file could not be parsed (comments or malformed JSON). Your existing settings are preserved.'); + return; + } + const settings = validateHookFields(cleanupOrphanedHooks(rawSettings)); const statuslineCommand = isGlobal ? buildHookCommand(targetDir, 'gsd-statusline.js') : 'node ' + dirName + '/hooks/gsd-statusline.js'; diff --git a/tests/settings-jsonc.test.cjs b/tests/settings-jsonc.test.cjs new file mode 100644 index 000000000..1ebf1f915 --- /dev/null +++ b/tests/settings-jsonc.test.cjs @@ -0,0 +1,186 @@ +/** + * GSD Tools Tests - settings.json JSONC (JSON with comments) support + * + * Validates that the installer's readSettings() correctly handles + * settings.json files containing comments (line and block) without + * silently overwriting them with empty objects. + * + * Closes: #1461 + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert'); +const fs = require('fs'); +const path = require('path'); + +// ─── inline stripJsonComments (mirrors install.js logic) ───────────────────── + +function stripJsonComments(text) { + let result = ''; + let i = 0; + let inString = false; + let stringChar = ''; + while (i < text.length) { + if (inString) { + if (text[i] === '\\') { + result += text[i] + (text[i + 1] || ''); + i += 2; + continue; + } + if (text[i] === stringChar) { + inString = false; + } + result += text[i]; + i++; + continue; + } + if (text[i] === '"' || text[i] === "'") { + inString = true; + stringChar = text[i]; + result += text[i]; + i++; + continue; + } + if (text[i] === '/' && text[i + 1] === '/') { + while (i < text.length && text[i] !== '\n') i++; + continue; + } + if (text[i] === '/' && text[i + 1] === '*') { + i += 2; + while (i < text.length && !(text[i] === '*' && text[i + 1] === '/')) i++; + i += 2; + continue; + } + result += text[i]; + i++; + } + return result.replace(/,\s*([}\]])/g, '$1'); +} + +// ─── tests ─────────────────────────────────────────────────────────────────── + +describe('stripJsonComments (#1461)', () => { + + test('strips line comments', () => { + const input = `{ + // This is a comment + "key": "value" +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.deepStrictEqual(result, { key: 'value' }); + }); + + test('strips block comments', () => { + const input = `{ + /* Block comment */ + "key": "value" +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.deepStrictEqual(result, { key: 'value' }); + }); + + test('strips multi-line block comments', () => { + const input = `{ + /* + * Multi-line + * block comment + */ + "key": "value" +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.deepStrictEqual(result, { key: 'value' }); + }); + + test('preserves comments inside string values', () => { + const input = `{ + "url": "https://example.com/path", + "description": "Use // for line comments" +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.strictEqual(result.url, 'https://example.com/path'); + assert.strictEqual(result.description, 'Use // for line comments'); + }); + + test('handles trailing commas', () => { + const input = `{ + "a": 1, + "b": 2, +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.deepStrictEqual(result, { a: 1, b: 2 }); + }); + + test('handles inline comments after values', () => { + const input = `{ + "timeout": 5000, // milliseconds + "retries": 3 // max attempts +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.strictEqual(result.timeout, 5000); + assert.strictEqual(result.retries, 3); + }); + + test('handles standard JSON (no comments) unchanged', () => { + const input = '{"key": "value", "num": 42}'; + const result = JSON.parse(stripJsonComments(input)); + assert.deepStrictEqual(result, { key: 'value', num: 42 }); + }); + + test('handles empty object', () => { + const result = JSON.parse(stripJsonComments('{}')); + assert.deepStrictEqual(result, {}); + }); + + test('handles real-world settings.json with comments', () => { + const input = `{ + // My configuration + "hooks": { + "SessionStart": [ + { + "matcher": "", /* match all */ + "hooks": [ + { + "type": "command", + "command": "node ~/.claude/hooks/gsd-statusline.js" + } + ] + } + ] + }, + "statusLine": { + "command": "node ~/.claude/hooks/gsd-statusline.js", + "refreshInterval": 10 + } +}`; + const result = JSON.parse(stripJsonComments(input)); + assert.ok(result.hooks, 'should have hooks'); + assert.ok(result.statusLine, 'should have statusLine'); + assert.strictEqual(result.statusLine.refreshInterval, 10); + }); +}); + +describe('readSettings null return on malformed files (#1461)', () => { + test('install.js contains JSONC stripping in readSettings', () => { + const installPath = path.join(__dirname, '..', 'bin', 'install.js'); + const content = fs.readFileSync(installPath, 'utf8'); + assert.ok(content.includes('stripJsonComments'), + 'install.js should use stripJsonComments in readSettings'); + }); + + test('readSettings returns null on truly malformed files (not empty object)', () => { + const installPath = path.join(__dirname, '..', 'bin', 'install.js'); + const content = fs.readFileSync(installPath, 'utf8'); + assert.ok(content.includes('return null'), + 'readSettings should return null on parse failure, not empty object'); + }); + + test('callers guard against null readSettings return', () => { + const installPath = path.join(__dirname, '..', 'bin', 'install.js'); + const content = fs.readFileSync(installPath, 'utf8'); + // Should have null guards at the settings configuration call sites + assert.ok( + content.includes('=== null') || content.includes('rawSettings === null'), + 'callers should check for null return from readSettings' + ); + }); +});