From 1bec188fdbd74b07a27129b697fa6085d37fca23 Mon Sep 17 00:00:00 2001 From: saitota Date: Fri, 3 Jul 2026 19:09:10 +0900 Subject: [PATCH] refactor(claudecode): centralize .claude/settings.json permissions ownership in a gateway The rule for who owns which entries inside .claude/settings.json permissions was duplicated across the ignore and permissions features: both knew the Read(...) deny entry format, and permissions guessed about the ignore feature by name in a warning string. Extract that shared contract into a single gateway so each feature only states its intent. No change to generated output. --- .../claudecode-settings-gateway.test.ts | 138 ++++++++++++++++++ src/features/claudecode-settings-gateway.ts | 109 ++++++++++++++ src/features/ignore/claudecode-ignore.ts | 32 ++-- .../permissions/claudecode-permissions.ts | 65 ++------- 4 files changed, 272 insertions(+), 72 deletions(-) create mode 100644 src/features/claudecode-settings-gateway.test.ts create mode 100644 src/features/claudecode-settings-gateway.ts diff --git a/src/features/claudecode-settings-gateway.test.ts b/src/features/claudecode-settings-gateway.test.ts new file mode 100644 index 000000000..c4ca22ec9 --- /dev/null +++ b/src/features/claudecode-settings-gateway.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, it } from "vitest"; + +import { createMockLogger } from "../test-utils/mock-logger.js"; +import type { ClaudeSettingsJson } from "../types/claude-settings.js"; +import { + applyIgnoreReadDenies, + applyPermissions, + buildReadDenyEntry, + isReadDenyEntry, +} from "./claudecode-settings-gateway.js"; + +// The permissions feature parses "Bash(npm *)" into its tool name; the gateway +// is agnostic to the format and takes this extractor as a parameter. +const toolNameOf = (entry: string): string => { + const parenIndex = entry.indexOf("("); + return parenIndex === -1 ? entry : entry.slice(0, parenIndex); +}; + +describe("isReadDenyEntry", () => { + it("recognizes Read(...) entries", () => { + expect(isReadDenyEntry("Read(.env)")).toBe(true); + expect(isReadDenyEntry("Read(*.log)")).toBe(true); + }); + + it("rejects non-Read and malformed entries", () => { + expect(isReadDenyEntry("Write(secret.txt)")).toBe(false); + expect(isReadDenyEntry("Read(unterminated")).toBe(false); + expect(isReadDenyEntry("Bash")).toBe(false); + }); +}); + +describe("buildReadDenyEntry", () => { + it("wraps a pattern into a Read deny entry", () => { + expect(buildReadDenyEntry("*.log")).toBe("Read(*.log)"); + }); +}); + +describe("applyIgnoreReadDenies", () => { + it("preserves non-Read deny entries while replacing the Read set", () => { + const settings: ClaudeSettingsJson = { + permissions: { deny: ["Write(secret.txt)", "Read(old.log)"] }, + }; + + const result = applyIgnoreReadDenies({ + settings, + readDenies: ["Read(*.log)", "Read(node_modules/**)"], + }); + + expect(result.permissions?.deny).toEqual([ + "Read(*.log)", + "Read(node_modules/**)", + "Write(secret.txt)", + ]); + }); + + it("leaves allow/ask untouched and other top-level keys intact", () => { + const settings: ClaudeSettingsJson = { + permissions: { allow: ["Bash(ls)"], ask: ["Bash(rm *)"], deny: ["Read(a)"] }, + hooks: { PreToolUse: [] }, + }; + + const result = applyIgnoreReadDenies({ settings, readDenies: ["Read(b)"] }); + + expect(result.permissions?.allow).toEqual(["Bash(ls)"]); + expect(result.permissions?.ask).toEqual(["Bash(rm *)"]); + expect(result.permissions?.deny).toEqual(["Read(b)"]); + expect(result.hooks).toEqual({ PreToolUse: [] }); + }); + + it("deduplicates and sorts", () => { + const result = applyIgnoreReadDenies({ + settings: { permissions: { deny: ["Read(z)"] } }, + readDenies: ["Read(b)", "Read(a)", "Read(b)"], + }); + + expect(result.permissions?.deny).toEqual(["Read(a)", "Read(b)"]); + }); +}); + +describe("applyPermissions", () => { + it("keeps entries for unmanaged tools and replaces managed ones", () => { + const settings: ClaudeSettingsJson = { + permissions: { deny: ["Read(.env)", "Bash(dangerous *)"] }, + }; + + const result = applyPermissions({ + settings, + managedToolNames: new Set(["Bash"]), + toolNameOf, + allow: [], + ask: [], + deny: ["Bash(rm *)"], + }); + + // Read (unmanaged) preserved; old Bash replaced by the new Bash rule. + expect(result.permissions?.deny).toEqual(["Bash(rm *)", "Read(.env)"]); + }); + + it("overwrites ignore-derived Read denies when Read is managed and warns", () => { + const logger = createMockLogger(); + const settings: ClaudeSettingsJson = { + permissions: { deny: ["Read(.env)", "Read(*.secret)"] }, + }; + + const result = applyPermissions({ + settings, + managedToolNames: new Set(["Read"]), + toolNameOf, + allow: ["Read(src/**)"], + ask: [], + deny: [], + logger, + }); + + expect(result.permissions?.deny).toBeUndefined(); + expect(result.permissions?.allow).toEqual(["Read(src/**)"]); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining("manages 'Read' tool")); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining("2 existing Read deny")); + // The warning no longer speculates about the ignore feature by name. + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringContaining("ignore")); + }); + + it("does not warn when Read is not managed", () => { + const logger = createMockLogger(); + + applyPermissions({ + settings: { permissions: { deny: ["Read(.env)"] } }, + managedToolNames: new Set(["Bash"]), + toolNameOf, + allow: [], + ask: [], + deny: ["Bash(rm *)"], + logger, + }); + + expect(logger.warn).not.toHaveBeenCalled(); + }); +}); diff --git a/src/features/claudecode-settings-gateway.ts b/src/features/claudecode-settings-gateway.ts new file mode 100644 index 000000000..296f8cced --- /dev/null +++ b/src/features/claudecode-settings-gateway.ts @@ -0,0 +1,109 @@ +import { uniq } from "es-toolkit"; + +import type { ClaudeSettingsJson } from "../types/claude-settings.js"; +import type { Logger } from "../utils/logger.js"; + +/** + * Single owner of the `.claude/settings.json` `permissions` block, which both + * `ignore` (writes `Read(...)` into `permissions.deny`) and `permissions` + * (writes the whole `allow`/`ask`/`deny`) read-modify-write. The entry format, + * the merge, and the cross-feature ownership rule (permissions' explicit `Read` + * rules win over ignore-derived `Read` denies) used to be duplicated across both + * feature files; they live here once so each feature just states its intent and + * never reasons about the other's existence. + */ + +const READ_TOOL_NAME = "Read"; + +export const isReadDenyEntry = (entry: string): boolean => + entry.startsWith(`${READ_TOOL_NAME}(`) && entry.endsWith(")"); + +export const buildReadDenyEntry = (pattern: string): string => `${READ_TOOL_NAME}(${pattern})`; + +const parsePermissionsBlock = ( + settings: ClaudeSettingsJson, +): { allow: string[]; ask: string[]; deny: string[] } => { + const permissions = settings.permissions ?? {}; + return { + allow: permissions.allow ?? [], + ask: permissions.ask ?? [], + deny: permissions.deny ?? [], + }; +}; + +// Empty arrays are omitted so the file never carries an empty allow/ask/deny key. +// Other top-level keys (e.g. `hooks`) and other keys under `permissions` are kept. +const withPermissions = ( + settings: ClaudeSettingsJson, + next: { allow: string[]; ask: string[]; deny: string[] }, +): ClaudeSettingsJson => { + const permissions: Record = { ...settings.permissions }; + const assign = (key: "allow" | "ask" | "deny", values: string[]): void => { + if (values.length > 0) { + permissions[key] = values; + } else { + delete permissions[key]; + } + }; + assign("allow", next.allow); + assign("ask", next.ask); + assign("deny", next.deny); + return { ...settings, permissions }; +}; + +// Non-`Read` deny entries belong to the permissions feature and are preserved; +// `Read(...)` denies are replaced wholesale since the ignore source owns them. +export const applyIgnoreReadDenies = (params: { + settings: ClaudeSettingsJson; + readDenies: string[]; +}): ClaudeSettingsJson => { + const { settings, readDenies } = params; + const current = parsePermissionsBlock(settings); + const preservedDeny = current.deny.filter( + (entry) => !isReadDenyEntry(entry) || readDenies.includes(entry), + ); + return withPermissions(settings, { + allow: current.allow, + ask: current.ask, + deny: uniq([...preservedDeny, ...readDenies].toSorted()), + }); +}; + +// Entries for managed tools are replaced; entries for unmanaged tools are kept. +// When `Read` is managed, permissions' rules win over ignore-derived `Read(...)` +// denies — those are overwritten, and the overwrite is warned about if a logger +// is given. +export const applyPermissions = (params: { + settings: ClaudeSettingsJson; + managedToolNames: ReadonlySet; + toolNameOf: (entry: string) => string; + allow: string[]; + ask: string[]; + deny: string[]; + logger?: Logger | undefined; +}): ClaudeSettingsJson => { + const { settings, managedToolNames, toolNameOf, allow, ask, deny, logger } = params; + const current = parsePermissionsBlock(settings); + + const keepUnmanaged = (entries: string[]): string[] => + entries.filter((entry) => !managedToolNames.has(toolNameOf(entry))); + + if (logger && managedToolNames.has(READ_TOOL_NAME)) { + const overwrittenReadDenies = current.deny.filter( + (entry) => toolNameOf(entry) === READ_TOOL_NAME, + ); + if (overwrittenReadDenies.length > 0) { + logger.warn( + `Permissions feature manages '${READ_TOOL_NAME}' tool and will overwrite ` + + `${overwrittenReadDenies.length} existing ${READ_TOOL_NAME} deny entries. ` + + `Permissions take precedence.`, + ); + } + } + + return withPermissions(settings, { + allow: uniq([...keepUnmanaged(current.allow), ...allow].toSorted()), + ask: uniq([...keepUnmanaged(current.ask), ...ask].toSorted()), + deny: uniq([...keepUnmanaged(current.deny), ...deny].toSorted()), + }); +}; diff --git a/src/features/ignore/claudecode-ignore.ts b/src/features/ignore/claudecode-ignore.ts index 0b719e14b..f4d29d685 100644 --- a/src/features/ignore/claudecode-ignore.ts +++ b/src/features/ignore/claudecode-ignore.ts @@ -1,6 +1,5 @@ import { join } from "node:path"; -import { uniq } from "es-toolkit"; import { z } from "zod/mini"; import { @@ -11,6 +10,11 @@ import { import type { ClaudeSettingsJson } from "../../types/claude-settings.js"; import { FeatureOptions } from "../../types/features.js"; import { fileExists, readFileContent } from "../../utils/file.js"; +import { + applyIgnoreReadDenies, + buildReadDenyEntry, + isReadDenyEntry, +} from "../claudecode-settings-gateway.js"; import { RulesyncIgnore } from "./rulesync-ignore.js"; import { ToolIgnore, @@ -104,7 +108,7 @@ export class ClaudecodeIgnore extends ToolIgnore { const rulesyncPatterns = this.patterns .map((pattern) => { // Remove "Read(" prefix and ")" suffix if present - if (pattern.startsWith("Read(") && pattern.endsWith(")")) { + if (isReadDenyEntry(pattern)) { return pattern.slice(5, -1); } return pattern; @@ -133,30 +137,20 @@ export class ClaudecodeIgnore extends ToolIgnore { .split(/\r?\n|\r/) .map((line: string) => line.trim()) .filter((line) => line.length > 0 && !line.startsWith("#")); - const deniedValues = patterns.map((pattern) => `Read(${pattern})`); + const deniedValues = patterns.map((pattern) => buildReadDenyEntry(pattern)); const paths = this.getSettablePaths({ options }); const filePath = join(outputRoot, paths.relativeDirPath, paths.relativeFilePath); const exists = await fileExists(filePath); const existingFileContent = exists ? await readFileContent(filePath) : "{}"; const existingJsonValue: ClaudeSettingsJson = JSON.parse(existingFileContent); - const existingDenies = existingJsonValue.permissions?.deny ?? []; - const preservedDenies = existingDenies.filter((deny) => { - const isReadPattern = deny.startsWith("Read(") && deny.endsWith(")"); - if (isReadPattern) { - return deniedValues.includes(deny); - } - - return true; - }); - const jsonValue: ClaudeSettingsJson = { - ...existingJsonValue, - permissions: { - ...existingJsonValue.permissions, - deny: uniq([...preservedDenies, ...deniedValues].toSorted()), - }, - }; + // The gateway owns the `permissions.deny` merge shared with the permissions + // feature; here we only state the intent (deny these Read patterns). + const jsonValue = applyIgnoreReadDenies({ + settings: existingJsonValue, + readDenies: deniedValues, + }); return new ClaudecodeIgnore({ outputRoot, diff --git a/src/features/permissions/claudecode-permissions.ts b/src/features/permissions/claudecode-permissions.ts index a579a2929..b0f7202f9 100644 --- a/src/features/permissions/claudecode-permissions.ts +++ b/src/features/permissions/claudecode-permissions.ts @@ -1,13 +1,12 @@ import { join } from "node:path"; -import { uniq } from "es-toolkit"; - import { CLAUDECODE_DIR, CLAUDECODE_SETTINGS_FILE_NAME } from "../../constants/claudecode-paths.js"; import type { AiFileParams, ValidationResult } from "../../types/ai-file.js"; import type { ClaudeSettingsJson } from "../../types/claude-settings.js"; import type { PermissionAction, PermissionsConfig } from "../../types/permissions.js"; import { formatError } from "../../utils/error.js"; import { readFileContentOrNull, readOrInitializeFileContent } from "../../utils/file.js"; +import { applyPermissions } from "../claudecode-settings-gateway.js"; import { RulesyncPermissions } from "./rulesync-permissions.js"; import { ToolPermissions, @@ -137,61 +136,21 @@ export class ClaudecodePermissions extends ToolPermissions { const config = rulesyncPermissions.getJson(); const { allow, ask, deny } = convertRulesyncToClaudePermissions(config); - // Determine which tool names are managed by the permissions config const managedToolNames = new Set( Object.keys(config.permission).map((category) => toClaudeToolName(category)), ); - // Read existing permission arrays and preserve entries for tools NOT in the permissions config - const existingPermissions = settings.permissions ?? {}; - const preservedAllow = (existingPermissions.allow ?? []).filter( - (entry) => !managedToolNames.has(parseClaudePermissionEntry(entry).toolName), - ); - const preservedAsk = (existingPermissions.ask ?? []).filter( - (entry) => !managedToolNames.has(parseClaudePermissionEntry(entry).toolName), - ); - const preservedDeny = (existingPermissions.deny ?? []).filter( - (entry) => !managedToolNames.has(parseClaudePermissionEntry(entry).toolName), - ); - - // Warn when permissions feature overwrites ignore-generated Read(...) deny entries - if (logger && managedToolNames.has("Read")) { - const droppedReadDenyEntries = (existingPermissions.deny ?? []).filter((entry) => { - const { toolName } = parseClaudePermissionEntry(entry); - return toolName === "Read"; - }); - if (droppedReadDenyEntries.length > 0) { - logger.warn( - `Permissions feature manages 'Read' tool and will overwrite ${droppedReadDenyEntries.length} existing Read deny entries (possibly from ignore feature). Permissions take precedence.`, - ); - } - } - - const mergedPermissions: Record = { - ...existingPermissions, - }; - - const mergedAllow = uniq([...preservedAllow, ...allow].toSorted()); - const mergedAsk = uniq([...preservedAsk, ...ask].toSorted()); - const mergedDeny = uniq([...preservedDeny, ...deny].toSorted()); - - if (mergedAllow.length > 0) { - mergedPermissions.allow = mergedAllow; - } else { - delete mergedPermissions.allow; - } - if (mergedAsk.length > 0) { - mergedPermissions.ask = mergedAsk; - } else { - delete mergedPermissions.ask; - } - if (mergedDeny.length > 0) { - mergedPermissions.deny = mergedDeny; - } else { - delete mergedPermissions.deny; - } - - const merged = { ...settings, permissions: mergedPermissions }; + // The gateway owns the shared `permissions` merge and the cross-feature + // ownership rule; here we only state the intent (managed tools + arrays). + const merged = applyPermissions({ + settings, + managedToolNames, + toolNameOf: (entry) => parseClaudePermissionEntry(entry).toolName, + allow, + ask, + deny, + logger, + }); const fileContent = JSON.stringify(merged, null, 2); return new ClaudecodePermissions({