diff --git a/.changeset/steady-lemurs-run.md b/.changeset/steady-lemurs-run.md new file mode 100644 index 000000000..08367a2ae --- /dev/null +++ b/.changeset/steady-lemurs-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2302 +--- +**Claude Code installs now pre-approve `.planning/` and `STATE.md` writes** — the installer wrote `Write(.planning/*)`/`Write(STATE.md)` permission rules, but Claude Code has no standalone `Write` gate (file edits are gated via `Edit(pattern)`), so those rules never matched and every fresh install still hit first-run approval prompts (and a session-start warning). The installer now writes `Edit(...)` rules and migrates the stale `Write(...)` entries away on the next run. (#2278) diff --git a/bin/install.js b/bin/install.js index 7b6c7e385..cb5b267ec 100755 --- a/bin/install.js +++ b/bin/install.js @@ -168,15 +168,28 @@ const DEFAULT_RUNTIME = 'claude'; const GSD_CLAUDE_ALLOW_PERMISSIONS = Object.freeze([ 'Bash(npx gsd-core *)', 'Read(.planning/*)', - 'Write(.planning/*)', + 'Edit(.planning/*)', 'Read(STATE.md)', - 'Write(STATE.md)', + 'Edit(STATE.md)', ]); const GSD_CLAUDE_DENY_PERMISSIONS = Object.freeze([ 'Read(.env)', 'Read(.env.*)', 'Read(.secrets)', ]); +// #2278 — Stale allow-rule forms from before the fix. Claude Code has no +// standalone `Write` permission gate: file-editing tools (Write/Edit/ +// NotebookEdit) are gated collectively via `Edit(pattern)`. The original +// `Write(.planning/*)` / `Write(STATE.md)` entries were therefore silently +// unmatched (never granted anything) and Claude Code additionally surfaces a +// session-start warning about unmatched permission rules. This list lets +// mergeClaudePermissions and uninstall cleanup retire those stale entries on +// existing installs while the current GSD_CLAUDE_ALLOW_PERMISSIONS above +// carries the working `Edit(...)` forms. +const GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS = Object.freeze([ + 'Write(.planning/*)', + 'Write(STATE.md)', +]); /** * Merge GSD-owned permission entries into a Claude Code settings object. @@ -185,6 +198,12 @@ const GSD_CLAUDE_DENY_PERMISSIONS = Object.freeze([ * entries are appended only if not already present. No other permission sub-keys * (ask, disableBypassPermissionsMode, etc.) are touched. * + * Migration (#2278): before adding the current GSD_CLAUDE_ALLOW_PERMISSIONS, + * any stale GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS entry (e.g. the unmatched + * `Write(...)` forms from before the fix) is removed from permissions.allow, + * so existing installs end up with the working `Edit(...)` forms instead of + * both the dead legacy entry and its replacement sitting side by side. + * * Defensive: if settings is not a plain object, returns immediately without * throwing. If permissions.allow / permissions.deny exist but are not arrays * (malformed settings), they are replaced with valid arrays. @@ -205,6 +224,10 @@ function mergeClaudePermissions(settings) { settings.permissions.deny = []; } + settings.permissions.allow = settings.permissions.allow.filter( + (e) => !GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS.includes(e) + ); + for (const entry of GSD_CLAUDE_ALLOW_PERMISSIONS) { if (!settings.permissions.allow.includes(entry)) { settings.permissions.allow.push(entry); @@ -7591,8 +7614,11 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { let permissionsModified = false; if (Array.isArray(settings.permissions.allow)) { const before = settings.permissions.allow.length; + // #2278 — filter against the union of the current allow-rule forms + // AND the retired legacy forms, so uninstall still cleans up + // pre-fix installs that still carry the stale `Write(...)` entries. settings.permissions.allow = settings.permissions.allow.filter( - (e) => !GSD_CLAUDE_ALLOW_PERMISSIONS.includes(e) + (e) => !GSD_CLAUDE_ALLOW_PERMISSIONS.includes(e) && !GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS.includes(e) ); if (settings.permissions.allow.length !== before) { permissionsModified = true; @@ -12157,6 +12183,7 @@ module.exports = { // #768 — Claude Code permissions pre-population mergeClaudePermissions, GSD_CLAUDE_ALLOW_PERMISSIONS, + GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS, GSD_CODEX_MARKER, CODEX_AGENT_SANDBOX, diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index 5538b31d7..5da3aaa31 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -886,9 +886,9 @@ Since v1.3.1, the installer pre-populates `~/.claude/settings.json` (or "allow": [ "Bash(npx gsd-core *)", "Read(.planning/*)", - "Write(.planning/*)", + "Edit(.planning/*)", "Read(STATE.md)", - "Write(STATE.md)" + "Edit(STATE.md)" ], "deny": [ "Read(.env)", diff --git a/tests/install-regressions.test.cjs b/tests/install-regressions.test.cjs index 99d628cc7..40768a33c 100644 --- a/tests/install-regressions.test.cjs +++ b/tests/install-regressions.test.cjs @@ -37,7 +37,7 @@ try { else process.env.GSD_TEST_MODE = savedTestMode; } -const { install, mergeClaudePermissions, GSD_CLAUDE_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS, rewriteLegacyManagedNodeHookCommands, resolveNodeRunner } = installExports || {}; +const { install, mergeClaudePermissions, GSD_CLAUDE_ALLOW_PERMISSIONS, GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS, rewriteLegacyManagedNodeHookCommands, resolveNodeRunner } = installExports || {}; const { installRuntimeArtifacts, @@ -394,22 +394,26 @@ describe('mergeClaudePermissions (#768): fresh settings object', () => { 'permissions.allow must contain Bash(npx gsd-core *)'); }); - test('includes planning path entries in allow', () => { + test('includes planning path entries in allow (#2278: Edit, not Write)', () => { const settings = {}; mergeClaudePermissions(settings); assert.ok(settings.permissions.allow.includes('Read(.planning/*)'), 'permissions.allow must contain Read(.planning/*)'); - assert.ok(settings.permissions.allow.includes('Write(.planning/*)'), - 'permissions.allow must contain Write(.planning/*)'); + assert.ok(settings.permissions.allow.includes('Edit(.planning/*)'), + 'permissions.allow must contain Edit(.planning/*)'); + assert.ok(!settings.permissions.allow.includes('Write(.planning/*)'), + 'permissions.allow must NOT contain the unmatched Write(.planning/*) form (#2278)'); }); - test('includes STATE.md entries in allow', () => { + test('includes STATE.md entries in allow (#2278: Edit, not Write)', () => { const settings = {}; mergeClaudePermissions(settings); assert.ok(settings.permissions.allow.includes('Read(STATE.md)'), 'permissions.allow must contain Read(STATE.md)'); - assert.ok(settings.permissions.allow.includes('Write(STATE.md)'), - 'permissions.allow must contain Write(STATE.md)'); + assert.ok(settings.permissions.allow.includes('Edit(STATE.md)'), + 'permissions.allow must contain Edit(STATE.md)'); + assert.ok(!settings.permissions.allow.includes('Write(STATE.md)'), + 'permissions.allow must NOT contain the unmatched Write(STATE.md) form (#2278)'); }); test('includes .env denial entries in deny', () => { @@ -496,6 +500,115 @@ describe('mergeClaudePermissions (#768): non-destructive merge', () => { }); }); +// ─── #2278 — Claude Code has no standalone `Write` permission gate; the +// pre-populated allow-rules must use `Edit(pattern)`, and a merge against an +// existing install must retire the stale unmatched `Write(...)` forms. +describe('mergeClaudePermissions (#2278): legacy Write(...) → Edit(...) migration', () => { + test('GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS is exported and lists the stale Write(...) forms', () => { + assert.ok(Array.isArray(GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS), + 'GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS must be an array'); + assert.deepStrictEqual( + [...GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS].sort(), + ['Write(.planning/*)', 'Write(STATE.md)'].sort(), + 'GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS must contain exactly the retired Write(...) forms' + ); + }); + + test('fresh/empty settings: allow ends with Edit(...) forms, never Write(...)', () => { + const settings = {}; + mergeClaudePermissions(settings); + assert.ok(settings.permissions.allow.includes('Edit(.planning/*)'), + 'fresh merge must add Edit(.planning/*)'); + assert.ok(settings.permissions.allow.includes('Edit(STATE.md)'), + 'fresh merge must add Edit(STATE.md)'); + assert.ok(!settings.permissions.allow.includes('Write(.planning/*)'), + 'fresh merge must never add Write(.planning/*)'); + assert.ok(!settings.permissions.allow.includes('Write(STATE.md)'), + 'fresh merge must never add Write(STATE.md)'); + }); + + test('existing install with legacy Write(...) entries: migrated to Edit(...), user entries untouched', () => { + const settings = { + permissions: { + allow: ['Write(.planning/*)', 'Write(STATE.md)', 'Bash(git *)'], + deny: ['WebSearch'], + }, + }; + mergeClaudePermissions(settings); + + // Stale legacy forms must be gone. + assert.ok(!settings.permissions.allow.includes('Write(.planning/*)'), + 'legacy Write(.planning/*) must be removed by merge'); + assert.ok(!settings.permissions.allow.includes('Write(STATE.md)'), + 'legacy Write(STATE.md) must be removed by merge'); + + // Replaced by the working Edit(...) forms. + assert.ok(settings.permissions.allow.includes('Edit(.planning/*)'), + 'Edit(.planning/*) must be present after migration'); + assert.ok(settings.permissions.allow.includes('Edit(STATE.md)'), + 'Edit(STATE.md) must be present after migration'); + + // Unrelated user-added entries must survive untouched. + assert.ok(settings.permissions.allow.includes('Bash(git *)'), + 'unrelated user allow entry must survive migration'); + assert.ok(settings.permissions.deny.includes('WebSearch'), + 'unrelated user deny entry must survive migration'); + }); + + test('mixed state: settings.allow containing BOTH legacy and current forms simultaneously collapses to exactly one Edit(...) each', () => { + const settings = { + permissions: { + allow: ['Write(.planning/*)', 'Edit(.planning/*)', 'Write(STATE.md)', 'Edit(STATE.md)', 'Bash(git *)'], + deny: [], + }, + }; + mergeClaudePermissions(settings); + + // Legacy forms must be gone. + assert.ok(!settings.permissions.allow.includes('Write(.planning/*)'), + 'legacy Write(.planning/*) must be removed even when Edit(.planning/*) was already present'); + assert.ok(!settings.permissions.allow.includes('Write(STATE.md)'), + 'legacy Write(STATE.md) must be removed even when Edit(STATE.md) was already present'); + + // Current forms must appear exactly once (no duplicate from the pre-existing entry). + assert.strictEqual( + settings.permissions.allow.filter((e) => e === 'Edit(.planning/*)').length, + 1, + 'Edit(.planning/*) must appear exactly once, not duplicated' + ); + assert.strictEqual( + settings.permissions.allow.filter((e) => e === 'Edit(STATE.md)').length, + 1, + 'Edit(STATE.md) must appear exactly once, not duplicated' + ); + + // Unrelated user entry must survive. + assert.ok(settings.permissions.allow.includes('Bash(git *)'), + 'unrelated user allow entry must survive the mixed-state migration'); + }); + + test('idempotent: repeated merge produces no dupes and never re-adds legacy entries', () => { + const settings = { + permissions: { + allow: ['Write(.planning/*)', 'Write(STATE.md)'], + deny: [], + }, + }; + mergeClaudePermissions(settings); + mergeClaudePermissions(settings); + mergeClaudePermissions(settings); + + for (const entry of GSD_CLAUDE_ALLOW_PERMISSIONS) { + const count = settings.permissions.allow.filter((e) => e === entry).length; + assert.strictEqual(count, 1, `allow entry "${entry}" must appear exactly once after repeated merges`); + } + for (const legacy of GSD_CLAUDE_LEGACY_ALLOW_PERMISSIONS) { + assert.ok(!settings.permissions.allow.includes(legacy), + `legacy entry "${legacy}" must never reappear after repeated merges`); + } + }); +}); + describe('mergeClaudePermissions (#768): end-to-end install writes permissions to settings.json', () => { test('--claude --global install writes GSD allow/deny entries to settings.json', (t) => { const root = createTempDir('gsd-claude-perm-install-'); @@ -635,6 +748,56 @@ describe('mergeClaudePermissions (#768): end-to-end install writes permissions t assert.ok(deny.includes('WebSearch'), 'user WebSearch deny entry must survive uninstall'); }); + + test('#2278: uninstall removes GSD entries in both legacy Write(...) and current Edit(...) form', (t) => { + const root = createTempDir('gsd-claude-perm-uninstall-legacy-'); + t.after(() => cleanup(root)); + + const spawnOpts = { + encoding: 'utf8', + env: { ...process.env, HOME: root, USERPROFILE: root }, + }; + + // Install first (writes the current Edit(...) forms). + const r1 = spawnSync( + process.execPath, + [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', root], + spawnOpts, + ); + assert.strictEqual(r1.status, 0, `install failed: ${r1.stderr}`); + + // Simulate a pre-fix install that still carries the stale Write(...) + // forms alongside the current Edit(...) forms and a user entry. + const settingsPath = path.join(root, 'settings.json'); + const settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); + settings.permissions.allow.push('Write(.planning/*)', 'Write(STATE.md)', 'Bash(git *)'); + fs.writeFileSync(settingsPath, JSON.stringify(settings, null, 2) + '\n'); + + // Uninstall + const r2 = spawnSync( + process.execPath, + [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', root, '--uninstall'], + spawnOpts, + ); + assert.strictEqual(r2.status, 0, `uninstall failed: ${r2.stderr}`); + + const afterUninstall = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); + const allow = afterUninstall.permissions?.allow ?? []; + + // Both legacy and current GSD-owned forms must be removed. + assert.ok(!allow.includes('Write(.planning/*)'), + 'legacy Write(.planning/*) entry must be removed by uninstall'); + assert.ok(!allow.includes('Write(STATE.md)'), + 'legacy Write(STATE.md) entry must be removed by uninstall'); + assert.ok(!allow.includes('Edit(.planning/*)'), + 'current Edit(.planning/*) entry must be removed by uninstall'); + assert.ok(!allow.includes('Edit(STATE.md)'), + 'current Edit(STATE.md) entry must be removed by uninstall'); + + // User entry must survive. + assert.ok(allow.includes('Bash(git *)'), + 'user Bash(git *) allow entry must survive uninstall'); + }); }); // ─── #976 — args-form hook presence detection ─────────────────────────────────