From e3a86218c653d26e54ef352c45322721df58f801 Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 14:53:26 -0500 Subject: [PATCH 1/6] fix(configure): use ~/.claude/rules/ for claude-cli, not CLAUDE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code auto-loads any *.md file under ~/.claude/rules/ on every session (see https://code.claude.com/docs/en/memory#user-level-rules), so the Snyk rules belong in their own dedicated file rather than injected as a delimited block into the user's global ~/.claude/CLAUDE.md. Why this matters: - ~/.claude/CLAUDE.md is the user's personal preferences file. Tool-managed content there is unwelcome pollution and survives uninstall as a leftover delimited block unless removeGlobalRules runs successfully. - A dedicated file uninstalls cleanly with a single os.Remove instead of a delimiter-based search-and-replace inside a shared file. - Mirrors the pattern already used for Cursor (globalSkillsPath → ~/.cursor/skills/snyk-rules/SKILL.md) — claude-cli was the inconsistent one in the existing codebase. Migration safety: a new legacyGlobalRulesPath field on hostConfig points at the prior ~/.claude/CLAUDE.md location. Both addConfiguration and removeConfiguration now invoke removeGlobalRules against it (idempotent — no-op when the delimited block is absent), so users upgrading from a prior install have the old block automatically stripped from their CLAUDE.md without losing any of their own personal content around it. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/configure/configure.go | 22 +++++++ internal/configure/configure_test.go | 92 +++++++++++++++++++++++----- internal/configure/host_config.go | 25 +++++--- 3 files changed, 115 insertions(+), 24 deletions(-) diff --git a/internal/configure/configure.go b/internal/configure/configure.go index ea7fb47..7df07f8 100644 --- a/internal/configure/configure.go +++ b/internal/configure/configure.go @@ -113,6 +113,17 @@ func removeConfiguration(logger *zerolog.Logger, config configuration.Configurat logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) } } + + // Clean up legacy global rules block left behind by a prior install that + // injected delimited rules into a shared file (e.g. claude-cli installs + // before the move to ~/.claude/rules/). removeGlobalRules is idempotent — + // no-op when the delimited block is absent — so this is safe regardless. + if ideConf.legacyGlobalRulesPath != "" { + err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) + } + } } _ = userInterface.Output("\n🎉 Removal complete!") @@ -245,6 +256,17 @@ func addConfiguration(logger *zerolog.Logger, config configuration.Configuration logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) } } + + // Clean up legacy global rules block when migrating to a dedicated rules + // file (e.g. claude-cli moved from delimited block in ~/.claude/CLAUDE.md + // to ~/.claude/rules/snyk-security.md). removeGlobalRules only strips the + // delimited block, leaving any user content in the legacy file intact. + if ideConf.legacyGlobalRulesPath != "" { + err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) + } + } } _ = userInterface.Output("\n🎉 Configuration complete!") diff --git a/internal/configure/configure_test.go b/internal/configure/configure_test.go index 32a8292..060c19e 100644 --- a/internal/configure/configure_test.go +++ b/internal/configure/configure_test.go @@ -52,15 +52,16 @@ func TestGetHostConfig(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - hostName string - expectError bool - expectedName string - expectMcpGlobalConfig bool - expectLocalRulesPath bool - expectGlobalRulesPath bool - expectGlobalSkillsPath bool - expectLegacyLocalRulesPath bool + name string + hostName string + expectError bool + expectedName string + expectMcpGlobalConfig bool + expectLocalRulesPath bool + expectGlobalRulesPath bool + expectGlobalSkillsPath bool + expectLegacyLocalRulesPath bool + expectLegacyGlobalRulesPath bool }{ { name: "cursor", @@ -113,12 +114,13 @@ func TestGetHostConfig(t *testing.T) { expectGlobalRulesPath: true, }, { - name: "claude-cli", - hostName: "claude-cli", - expectError: false, - expectedName: "claude-cli", - expectMcpGlobalConfig: true, - expectGlobalRulesPath: true, + name: "claude-cli", + hostName: "claude-cli", + expectError: false, + expectedName: "claude-cli", + expectMcpGlobalConfig: true, + expectGlobalSkillsPath: true, + expectLegacyGlobalRulesPath: true, }, { name: "unsupported", @@ -155,10 +157,70 @@ func TestGetHostConfig(t *testing.T) { if tt.expectLegacyLocalRulesPath { assert.NotEmpty(t, config.legacyLocalRulesPath) } + if tt.expectLegacyGlobalRulesPath { + assert.NotEmpty(t, config.legacyGlobalRulesPath) + assert.Contains(t, config.legacyGlobalRulesPath, homeDir) + } }) } } +func TestGetHostConfig_ClaudeCliPaths(t *testing.T) { + homeDir, err := os.UserHomeDir() + require.NoError(t, err) + + config, err := getHostConfig("claude-cli") + require.NoError(t, err) + + // MCP server entry continues to live in ~/.claude.json + assert.Equal(t, filepath.Join(homeDir, ".claude.json"), config.mcpGlobalConfigPath) + + // Rules now live in a dedicated, auto-loaded file under ~/.claude/rules/ + // (per https://code.claude.com/docs/en/memory#user-level-rules) instead of + // being injected into the user's global ~/.claude/CLAUDE.md. + assert.Equal(t, filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), config.globalSkillsPath) + + // CLAUDE.md is preserved as a legacy path so prior installs get cleaned up. + assert.Equal(t, filepath.Join(homeDir, ".claude", "CLAUDE.md"), config.legacyGlobalRulesPath) + + // Delimited globalRulesPath is no longer used for claude-cli — the rules + // file is now Snyk-owned and should be written without delimiters. + assert.Empty(t, config.globalRulesPath) + + // claude-cli has never used local workspace rules. + assert.Empty(t, config.localRulesPath) + assert.Empty(t, config.legacyLocalRulesPath) +} + +func TestRemoveGlobalRulesIsClaudeCliMigrationSafe(t *testing.T) { + // Verifies the migration path: a CLAUDE.md containing both pre-existing user + // content AND a Snyk delimited block (left by an older install) gets the + // Snyk block stripped while user content is preserved verbatim. This is the + // behavior addConfiguration / removeConfiguration rely on for claude-cli. + nopLogger := zerolog.New(io.Discard) + logger := &nopLogger + + tempDir := t.TempDir() + claudeMd := filepath.Join(tempDir, "CLAUDE.md") + + userContent := "# My personal preferences\n\n- Use tabs for indentation\n- Prefer Go over Rust\n" + snykBlock := RuleStart + "\n# Snyk Security At Inception\nDo the snyk thing.\n" + RuleEnd + combined := userContent + "\n" + snykBlock + "\n" + require.NoError(t, os.WriteFile(claudeMd, []byte(combined), 0644)) + + require.NoError(t, removeGlobalRules(claudeMd, logger)) + + result, err := os.ReadFile(claudeMd) + require.NoError(t, err) + resultStr := string(result) + + assert.Contains(t, resultStr, "Use tabs for indentation") + assert.Contains(t, resultStr, "Prefer Go over Rust") + assert.NotContains(t, resultStr, RuleStart) + assert.NotContains(t, resultStr, RuleEnd) + assert.NotContains(t, resultStr, "Snyk Security At Inception") +} + func TestGetHostConfig_VSCodePaths(t *testing.T) { configDir, err := os.UserConfigDir() require.NoError(t, err) diff --git a/internal/configure/host_config.go b/internal/configure/host_config.go index d423368..42002b0 100644 --- a/internal/configure/host_config.go +++ b/internal/configure/host_config.go @@ -8,12 +8,13 @@ import ( ) type hostConfig struct { - name string - mcpGlobalConfigPath string // Path to MCP server configuration JSON - localRulesPath string // Relative path for local workspace rules - globalRulesPath string // Absolute path for global user rules (delimited) - globalSkillsPath string // Absolute path for global user skills (no delimiters) - legacyLocalRulesPath string // Old local rules path to clean up during migration + name string + mcpGlobalConfigPath string // Path to MCP server configuration JSON + localRulesPath string // Relative path for local workspace rules + globalRulesPath string // Absolute path for global user rules (delimited) + globalSkillsPath string // Absolute path for global user skills (no delimiters) + legacyLocalRulesPath string // Old local rules path to clean up during migration + legacyGlobalRulesPath string // Old global rules file to clean up during migration (delimited block removed in place) } // getHostConfig returns MCP-Host-specific configuration based on the host name @@ -69,10 +70,16 @@ func getHostConfig(hostName string) (*hostConfig, error) { globalRulesPath: filepath.Join(homeDir, ".gemini", "GEMINI.md"), }, nil case "claude-cli": + // Claude Code auto-loads any *.md file in ~/.claude/rules/ on every session + // (see https://code.claude.com/docs/en/memory#user-level-rules), so the rules + // belong in their own dedicated file rather than injected into the user's + // global ~/.claude/CLAUDE.md. legacyGlobalRulesPath cleans up the old + // in-place injection from prior installs. return &hostConfig{ - name: hostName, - mcpGlobalConfigPath: filepath.Join(homeDir, ".claude.json"), - globalRulesPath: filepath.Join(homeDir, ".claude", "CLAUDE.md"), + name: hostName, + mcpGlobalConfigPath: filepath.Join(homeDir, ".claude.json"), + globalSkillsPath: filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), + legacyGlobalRulesPath: filepath.Join(homeDir, ".claude", "CLAUDE.md"), }, nil } return nil, fmt.Errorf("unsupported Tool: %s", hostName) From d721616a2c150a5963a2924ad712e266b52857d0 Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 15:41:38 -0500 Subject: [PATCH 2/6] refactor(configure): split globalSkillsPath; embed Claude rules content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new globalDedicatedRulesPath field on hostConfig distinct from globalSkillsPath, and embeds Claude-Code-specific rules content under internal/configure/rules/claude/ that omits the Cursor SKILL.md frontmatter (name:/description:) it doesn't recognize. Before this commit the claude-cli host wrote Cursor-style SKILL frontmatter into ~/.claude/rules/snyk-security.md. Six tribunal agents flagged this independently as a content/format mismatch: Claude Code's user-level-rules schema does not recognize the SKILL keys, and relying on the loader silently ignoring unknown frontmatter is fragile. The split also resolves the leaky-abstraction concern that "globalSkillsPath" was lying for claude-cli — the path pointed at a rules directory, not a skills one. Cursor continues to use globalSkillsPath unchanged. claude-cli now uses globalDedicatedRulesPath, which is wired into the same writeGlobalSkills machinery (it's just "write a whole file, no delimiters" — the function name remains accurate to its semantics, even if it predated this split). Addresses Tribunal finding #F1 (content/format mismatch, score 82) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Addresses Tribunal finding #F7 (naming / leaky abstraction, score 75) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Co-Authored-By: Claude Opus 4.7 (1M context) -- ᴛ-ᴅᴜʙ's Review Tribunal ⚖️ --- internal/configure/configure.go | 44 ++++++++++++++++ internal/configure/configure_test.go | 51 +++++++++++-------- internal/configure/host_config.go | 31 ++++++----- .../configure/rules/claude/always_apply.md | 8 +++ .../configure/rules/claude/smart_apply.md | 9 ++++ 5 files changed, 109 insertions(+), 34 deletions(-) create mode 100644 internal/configure/rules/claude/always_apply.md create mode 100644 internal/configure/rules/claude/smart_apply.md diff --git a/internal/configure/configure.go b/internal/configure/configure.go index 7df07f8..5b603ad 100644 --- a/internal/configure/configure.go +++ b/internal/configure/configure.go @@ -23,6 +23,16 @@ var snykSkillsAlwaysApply string //go:embed skills/sast/smart_apply.md var snykSkillsSmartApply string +// Claude Code's user-level rules loader (~/.claude/rules/*.md) does not use +// the Cursor SKILL.md frontmatter (name/description); these are clean +// markdown bodies tailored for that loader. +// +//go:embed rules/claude/always_apply.md +var snykClaudeRulesAlwaysApply string + +//go:embed rules/claude/smart_apply.md +var snykClaudeRulesSmartApply string + // Configure sets up MCP server and rules for the specified IDE host. func Configure(logger *zerolog.Logger, config configuration.Configuration, userInterface ui.UserInterface, cliPath string) error { hostName := config.GetString(shared.ToolNameParam) @@ -80,6 +90,19 @@ func removeConfiguration(logger *zerolog.Logger, config configuration.Configurat logger.Info().Msgf("Successfully removed global skills for %s from %s", ideConf.name, ideConf.globalSkillsPath) } + // Remove dedicated rules file (e.g. claude-cli) + if ideConf.globalDedicatedRulesPath != "" { + _ = userInterface.Output(fmt.Sprintf("📋 Removing dedicated rules file from: %s", ideConf.globalDedicatedRulesPath)) + + err := removeGlobalSkills(ideConf.globalDedicatedRulesPath, logger) + if err != nil { + return fmt.Errorf("failed to remove dedicated rules file for %s: %w", ideConf.name, err) + } + + _ = userInterface.Output(fmt.Sprintf("✅ Successfully removed dedicated rules file for %s", ideConf.name)) + logger.Info().Msgf("Successfully removed dedicated rules file for %s from %s", ideConf.name, ideConf.globalDedicatedRulesPath) + } + // Remove global rules (e.g. Windsurf, Antigravity, gemini-cli, claude-cli) if rulesScope == shared.RulesGlobalScope && ideConf.globalRulesPath != "" { _ = userInterface.Output(fmt.Sprintf("📋 Removing global rules from: %s", ideConf.globalRulesPath)) @@ -218,6 +241,27 @@ func addConfiguration(logger *zerolog.Logger, config configuration.Configuration logger.Info().Msgf("Successfully wrote global skills for %s at %s", ideConf.name, ideConf.globalSkillsPath) } + // Write dedicated rules file (e.g. claude-cli ~/.claude/rules/snyk-security.md) + if ideConf.globalDedicatedRulesPath != "" { + var dedicatedRulesContent string + switch ruleType { + case shared.RuleTypeAlwaysApply: + dedicatedRulesContent = snykClaudeRulesAlwaysApply + case shared.RuleTypeSmart: + dedicatedRulesContent = snykClaudeRulesSmartApply + } + + _ = userInterface.Output(fmt.Sprintf("📋 Writing dedicated rules file (%s) to: %s", ruleType, ideConf.globalDedicatedRulesPath)) + + err := writeGlobalSkills(ideConf.globalDedicatedRulesPath, dedicatedRulesContent, logger) + if err != nil { + return fmt.Errorf("failed to write dedicated rules file for %s: %w", ideConf.name, err) + } + + _ = userInterface.Output(fmt.Sprintf("✅ Successfully wrote dedicated rules file for %s", ideConf.name)) + logger.Info().Msgf("Successfully wrote dedicated rules file for %s at %s", ideConf.name, ideConf.globalDedicatedRulesPath) + } + // Write global rules with delimiters (e.g. Windsurf, Antigravity, gemini-cli, claude-cli) if rulesScope == shared.RulesGlobalScope && ideConf.globalRulesPath != "" { _ = userInterface.Output(fmt.Sprintf("📋 Writing global rules (%s) to: %s", ruleType, ideConf.globalRulesPath)) diff --git a/internal/configure/configure_test.go b/internal/configure/configure_test.go index 060c19e..7edac53 100644 --- a/internal/configure/configure_test.go +++ b/internal/configure/configure_test.go @@ -52,16 +52,17 @@ func TestGetHostConfig(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - hostName string - expectError bool - expectedName string - expectMcpGlobalConfig bool - expectLocalRulesPath bool - expectGlobalRulesPath bool - expectGlobalSkillsPath bool - expectLegacyLocalRulesPath bool - expectLegacyGlobalRulesPath bool + name string + hostName string + expectError bool + expectedName string + expectMcpGlobalConfig bool + expectLocalRulesPath bool + expectGlobalRulesPath bool + expectGlobalSkillsPath bool + expectGlobalDedicatedRulesPath bool + expectLegacyLocalRulesPath bool + expectLegacyGlobalRulesPath bool }{ { name: "cursor", @@ -114,13 +115,13 @@ func TestGetHostConfig(t *testing.T) { expectGlobalRulesPath: true, }, { - name: "claude-cli", - hostName: "claude-cli", - expectError: false, - expectedName: "claude-cli", - expectMcpGlobalConfig: true, - expectGlobalSkillsPath: true, - expectLegacyGlobalRulesPath: true, + name: "claude-cli", + hostName: "claude-cli", + expectError: false, + expectedName: "claude-cli", + expectMcpGlobalConfig: true, + expectGlobalDedicatedRulesPath: true, + expectLegacyGlobalRulesPath: true, }, { name: "unsupported", @@ -154,6 +155,10 @@ func TestGetHostConfig(t *testing.T) { assert.NotEmpty(t, config.globalSkillsPath) assert.Contains(t, config.globalSkillsPath, homeDir) } + if tt.expectGlobalDedicatedRulesPath { + assert.NotEmpty(t, config.globalDedicatedRulesPath) + assert.Contains(t, config.globalDedicatedRulesPath, homeDir) + } if tt.expectLegacyLocalRulesPath { assert.NotEmpty(t, config.legacyLocalRulesPath) } @@ -175,10 +180,10 @@ func TestGetHostConfig_ClaudeCliPaths(t *testing.T) { // MCP server entry continues to live in ~/.claude.json assert.Equal(t, filepath.Join(homeDir, ".claude.json"), config.mcpGlobalConfigPath) - // Rules now live in a dedicated, auto-loaded file under ~/.claude/rules/ - // (per https://code.claude.com/docs/en/memory#user-level-rules) instead of - // being injected into the user's global ~/.claude/CLAUDE.md. - assert.Equal(t, filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), config.globalSkillsPath) + // Rules live in a dedicated, auto-loaded file under ~/.claude/rules/ + // (per https://code.claude.com/docs/en/memory#user-level-rules) — distinct + // from globalSkillsPath, which is reserved for Cursor's SKILL.md slot. + assert.Equal(t, filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), config.globalDedicatedRulesPath) // CLAUDE.md is preserved as a legacy path so prior installs get cleaned up. assert.Equal(t, filepath.Join(homeDir, ".claude", "CLAUDE.md"), config.legacyGlobalRulesPath) @@ -187,6 +192,10 @@ func TestGetHostConfig_ClaudeCliPaths(t *testing.T) { // file is now Snyk-owned and should be written without delimiters. assert.Empty(t, config.globalRulesPath) + // globalSkillsPath is the Cursor-specific SKILL.md slot; claude-cli must + // not piggyback on it (we use the dedicated rules path instead). + assert.Empty(t, config.globalSkillsPath) + // claude-cli has never used local workspace rules. assert.Empty(t, config.localRulesPath) assert.Empty(t, config.legacyLocalRulesPath) diff --git a/internal/configure/host_config.go b/internal/configure/host_config.go index 42002b0..9a90ad6 100644 --- a/internal/configure/host_config.go +++ b/internal/configure/host_config.go @@ -8,13 +8,14 @@ import ( ) type hostConfig struct { - name string - mcpGlobalConfigPath string // Path to MCP server configuration JSON - localRulesPath string // Relative path for local workspace rules - globalRulesPath string // Absolute path for global user rules (delimited) - globalSkillsPath string // Absolute path for global user skills (no delimiters) - legacyLocalRulesPath string // Old local rules path to clean up during migration - legacyGlobalRulesPath string // Old global rules file to clean up during migration (delimited block removed in place) + name string + mcpGlobalConfigPath string // Path to MCP server configuration JSON + localRulesPath string // Relative path for local workspace rules + globalRulesPath string // Absolute path for global user rules (delimited block in a shared file) + globalSkillsPath string // Absolute path for a host-native skill file (e.g. Cursor SKILL.md, no delimiters) + globalDedicatedRulesPath string // Absolute path for a dedicated host-native rules file (e.g. Claude Code ~/.claude/rules/*.md, no delimiters) + legacyLocalRulesPath string // Old local rules path to clean up during migration (whole file removed) + legacyGlobalRulesPath string // Old global rules file to clean up during migration (delimited block removed in place; surrounding user content preserved) } // getHostConfig returns MCP-Host-specific configuration based on the host name @@ -73,13 +74,17 @@ func getHostConfig(hostName string) (*hostConfig, error) { // Claude Code auto-loads any *.md file in ~/.claude/rules/ on every session // (see https://code.claude.com/docs/en/memory#user-level-rules), so the rules // belong in their own dedicated file rather than injected into the user's - // global ~/.claude/CLAUDE.md. legacyGlobalRulesPath cleans up the old - // in-place injection from prior installs. + // global ~/.claude/CLAUDE.md. globalDedicatedRulesPath is distinct from + // globalSkillsPath (Cursor's SKILL.md slot) so each host gets content + // formatted for its own loader — Claude Code rules need no frontmatter, + // while Cursor's SKILL.md expects name/description keys. + // legacyGlobalRulesPath cleans up the old in-place injection from prior + // installs that wrote a delimited block into ~/.claude/CLAUDE.md. return &hostConfig{ - name: hostName, - mcpGlobalConfigPath: filepath.Join(homeDir, ".claude.json"), - globalSkillsPath: filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), - legacyGlobalRulesPath: filepath.Join(homeDir, ".claude", "CLAUDE.md"), + name: hostName, + mcpGlobalConfigPath: filepath.Join(homeDir, ".claude.json"), + globalDedicatedRulesPath: filepath.Join(homeDir, ".claude", "rules", "snyk-security.md"), + legacyGlobalRulesPath: filepath.Join(homeDir, ".claude", "CLAUDE.md"), }, nil } return nil, fmt.Errorf("unsupported Tool: %s", hostName) diff --git a/internal/configure/rules/claude/always_apply.md b/internal/configure/rules/claude/always_apply.md new file mode 100644 index 0000000..dbbc396 --- /dev/null +++ b/internal/configure/rules/claude/always_apply.md @@ -0,0 +1,8 @@ +# Snyk Security At Inception + +Project security best practices for AI-generated code: + +- Always run snyk_code_scan tool for new first party code that is generated in a Snyk-supported language. +- If any security issues are found based on newly introduced or modified code or dependencies, attempt to fix the issues using the results context from Snyk. +- Rescan the code after fixing the issues to ensure that the issues were fixed and that there are no newly introduced issues. +- Repeat this process until no new issues are found. diff --git a/internal/configure/rules/claude/smart_apply.md b/internal/configure/rules/claude/smart_apply.md new file mode 100644 index 0000000..13b1c66 --- /dev/null +++ b/internal/configure/rules/claude/smart_apply.md @@ -0,0 +1,9 @@ +# Snyk Security At Inception + +Project security best practices for AI-generated code: + +- BEFORE declaring task complete: Run snyk_code_scan tool when a significant change has been made in first party code. +- This should only apply for Snyk-supported coding language +- If any security issues are found based on newly introduced or modified code or dependencies, attempt to fix the issues using the results context from Snyk. +- Rescan the code after fixing the issues to ensure that the issues were fixed and that there are no newly introduced issues. +- Repeat this process until no new issues are found. From ac19d72bc571e1d7c16064d8b278220b887ea967 Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 15:43:07 -0500 Subject: [PATCH 3/6] fix(configure): run legacy global-rules cleanup outside configureRules gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the migration cleanup that strips the old delimited Snyk block from ~/.claude/CLAUDE.md sat inside `if configureRules { ... }` in both addConfiguration and removeConfiguration. A user invoking `studio-mcp configure --tool claude-cli --no-rules` (MCP only) — a perfectly reasonable invocation — would never have their old CLAUDE.md Snyk block cleaned up, even though Snyk has abandoned that location. Migration housekeeping is conceptually independent of rules configuration: once we've decided the legacy block is stale (the prior PR did), it should be cleaned up regardless of whether the user is configuring rules this run. This commit moves only the legacyGlobalRulesPath cleanup out of the gate; legacyLocalRulesPath stays inside (its semantics — "you're migrating from local to global rules" — are inherently tied to rules being configured). Addresses Tribunal finding #F12 (migration completeness — gating, score 60) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Co-Authored-By: Claude Opus 4.7 (1M context) -- ᴛ-ᴅᴜʙ's Review Tribunal ⚖️ --- internal/configure/configure.go | 46 ++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/internal/configure/configure.go b/internal/configure/configure.go index 5b603ad..31603ff 100644 --- a/internal/configure/configure.go +++ b/internal/configure/configure.go @@ -136,16 +136,21 @@ func removeConfiguration(logger *zerolog.Logger, config configuration.Configurat logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) } } + } - // Clean up legacy global rules block left behind by a prior install that - // injected delimited rules into a shared file (e.g. claude-cli installs - // before the move to ~/.claude/rules/). removeGlobalRules is idempotent — - // no-op when the delimited block is absent — so this is safe regardless. - if ideConf.legacyGlobalRulesPath != "" { - err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) - } + // Clean up legacy global rules block left behind by a prior install that + // injected delimited rules into a shared file (e.g. claude-cli installs + // before the move to ~/.claude/rules/). This runs OUTSIDE the + // configureRules gate intentionally: a user invoking remove with + // configureRules=false (e.g. wanting to remove only the MCP entry) still + // expects stale Snyk content in their personal CLAUDE.md to be cleaned up + // — migration housekeeping is independent of whether rules are being + // configured this run. removeGlobalRules is idempotent (no-op when the + // delimited block is absent), so this is safe regardless. + if ideConf.legacyGlobalRulesPath != "" { + err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) } } @@ -300,16 +305,21 @@ func addConfiguration(logger *zerolog.Logger, config configuration.Configuration logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) } } + } - // Clean up legacy global rules block when migrating to a dedicated rules - // file (e.g. claude-cli moved from delimited block in ~/.claude/CLAUDE.md - // to ~/.claude/rules/snyk-security.md). removeGlobalRules only strips the - // delimited block, leaving any user content in the legacy file intact. - if ideConf.legacyGlobalRulesPath != "" { - err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) - } + // Clean up legacy global rules block when migrating to a dedicated rules + // file (e.g. claude-cli moved from delimited block in ~/.claude/CLAUDE.md + // to ~/.claude/rules/snyk-security.md). This runs OUTSIDE the + // configureRules gate intentionally: a user running configure with + // configureRules=false (MCP-only) has still abandoned the legacy + // location, so the stale block in their personal CLAUDE.md should be + // cleaned up regardless of whether rules are being configured this run. + // removeGlobalRules only strips the delimited block, leaving any user + // content in the legacy file intact. + if ideConf.legacyGlobalRulesPath != "" { + err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) } } From 86532f755c2c362f1b02879301dcb0d02e24d419 Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 15:45:53 -0500 Subject: [PATCH 4/6] test(configure): add end-to-end claude-cli configure/remove integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds TestConfigure_ClaudeCliEndToEnd, which drives Configure() — not the write/remove helpers — for the claude-cli host against a temp HOME and asserts the full migration story: 1. add: writes ~/.claude/rules/snyk-security.md with the new Claude-Code-shaped content (no Cursor SKILL frontmatter), strips the delimited Snyk block from a pre-existing CLAUDE.md, and preserves user content above and below the block. 2. remove: deletes the dedicated rules file and strips the legacy block; user content survives. 3. MCP-only configure (configureRules=false): legacy CLAUDE.md cleanup still runs (pinning the F12 behavior contract — migration housekeeping is independent of whether rules are configured). Pre-existing tests pinned the helper-level behavior of removeGlobalRules but never exercised the wiring through addConfiguration / removeConfiguration — so a future refactor that dropped the migration call sites would slip past every other test in this package. This test catches that. Implementation notes: a small noopUserInterface stub satisfies ui.UserInterface without polluting test output. The test is skipped on Windows because t.Setenv("HOME",...) does not redirect os.UserHomeDir() there (it reads USERPROFILE) — the migration logic itself is platform-agnostic; we just rely on Linux/macOS to drive it. Addresses Tribunal finding #F3 (test gap / integration-untested behavior, score 80) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Co-Authored-By: Claude Opus 4.7 (1M context) -- ᴛ-ᴅᴜʙ's Review Tribunal ⚖️ --- internal/configure/configure_test.go | 187 +++++++++++++++++++++++++++ 1 file changed, 187 insertions(+) diff --git a/internal/configure/configure_test.go b/internal/configure/configure_test.go index 7edac53..30f24fc 100644 --- a/internal/configure/configure_test.go +++ b/internal/configure/configure_test.go @@ -5,16 +5,30 @@ import ( "io" "os" "path/filepath" + "runtime" "strings" "testing" "github.com/go-git/go-git/v5" "github.com/rs/zerolog" + "github.com/snyk/go-application-framework/pkg/configuration" + "github.com/snyk/go-application-framework/pkg/ui" "github.com/snyk/studio-mcp/shared" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// noopUserInterface satisfies ui.UserInterface for tests; every method +// discards. configure.go only calls Output(), but the interface contract +// requires the rest, so each method returns a benign zero value. +type noopUserInterface struct{} + +func (noopUserInterface) Output(string) error { return nil } +func (noopUserInterface) OutputError(error, ...ui.Opts) error { return nil } +func (noopUserInterface) NewProgressBar() ui.ProgressBar { return nil } +func (noopUserInterface) Input(string) (string, error) { return "", nil } +func (noopUserInterface) SelectOptions(string, []string) (int, string, error) { return 0, "", nil } + func TestUpsertDelimitedBlock(t *testing.T) { tests := []struct { name string @@ -1236,3 +1250,176 @@ func TestRemoveDelimitedBlock(t *testing.T) { }) } } + +// TestConfigure_ClaudeCliEndToEnd exercises the full add → remove cycle for +// the claude-cli host. It pins three behaviors that helper-level tests do +// NOT cover: that addConfiguration writes the new dedicated rules file, +// that the new file is removed by removeConfiguration, and that the +// migration cleanup wired into configure.go actually strips the legacy +// delimited block from a pre-existing CLAUDE.md while preserving +// surrounding user content. A future refactor that drops the migration +// call sites would slip past every other test in this package — this is +// the one that catches it. +func TestConfigure_ClaudeCliEndToEnd(t *testing.T) { + if runtime.GOOS == "windows" { + // os.UserHomeDir() resolves via USERPROFILE on Windows; t.Setenv("HOME",...) + // would not redirect it. The migration logic is platform-agnostic, so we + // rely on Linux/macOS for this integration test. + t.Skip("integration test relies on $HOME-driven UserHomeDir resolution") + } + + nopLogger := zerolog.New(io.Discard) + logger := &nopLogger + uiStub := noopUserInterface{} + + // Construct a fresh, isolated HOME for each subtest. addConfiguration and + // removeConfiguration both call os.UserHomeDir() via getHostConfig, which + // on Linux/macOS reads $HOME — t.Setenv lets us redirect those reads + // without touching the developer's actual ~/.claude tree. + setupTempHome := func(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".claude"), 0755)) + return home + } + + // makeConfig returns a configuration.Configuration populated with the + // minimum keys Configure() reads. ruleType always-apply is the default + // payload Snyk ships; configureMcp+configureRules both true exercises + // the full add path. + makeConfig := func(removeMode bool) configuration.Configuration { + c := configuration.New() + c.Set(shared.ToolNameParam, "claude-cli") + c.Set(shared.RemoveParam, removeMode) + c.Set(shared.RuleTypeParam, shared.RuleTypeAlwaysApply) + c.Set(shared.RulesScopeParam, shared.RulesGlobalScope) + c.Set(shared.WorkspacePathParam, "") + c.Set(shared.ConfigureMcpParam, true) + c.Set(shared.ConfigureRulesParam, true) + return c + } + + t.Run("add: writes dedicated rules file and strips legacy CLAUDE.md block", func(t *testing.T) { + home := setupTempHome(t) + + // Pre-seed CLAUDE.md with the exact shape a prior install would have + // left behind: user content above and below a delimited Snyk block. + // Migration must remove ONLY the block. + userTop := "# My personal preferences\n\n- Always tabs, never spaces\n" + userBottom := "\n## Personal projects\n\n- Be terse\n" + legacyBlock := RuleStart + "\n# Snyk Security At Inception\nDo the snyk thing.\n" + RuleEnd + claudeMd := filepath.Join(home, ".claude", "CLAUDE.md") + require.NoError(t, os.WriteFile(claudeMd, []byte(userTop+legacyBlock+userBottom), 0644)) + + require.NoError(t, Configure(logger, makeConfig(false), uiStub, "/usr/local/bin/snyk")) + + // New dedicated rules file exists with the embedded Claude-Code rules + // content (no Cursor SKILL frontmatter). + rulesFile := filepath.Join(home, ".claude", "rules", "snyk-security.md") + assert.FileExists(t, rulesFile) + got, err := os.ReadFile(rulesFile) + require.NoError(t, err) + assert.Equal(t, snykClaudeRulesAlwaysApply, string(got), + "new rules file should contain the Claude-Code-shaped content embed") + assert.NotContains(t, string(got), "name: snyk-rules", + "Cursor SKILL frontmatter must not appear in the Claude rules file") + + // Legacy block is gone but user content survives. + md, err := os.ReadFile(claudeMd) + require.NoError(t, err) + mds := string(md) + assert.NotContains(t, mds, RuleStart, "legacy block start marker should be stripped") + assert.NotContains(t, mds, RuleEnd, "legacy block end marker should be stripped") + assert.NotContains(t, mds, "Do the snyk thing", "legacy Snyk content body should be stripped") + assert.Contains(t, mds, "Always tabs, never spaces", "user content above the legacy block must survive") + assert.Contains(t, mds, "Be terse", "user content below the legacy block must survive") + + // MCP server entry was written to ~/.claude.json. + claudeJSON := filepath.Join(home, ".claude.json") + assert.FileExists(t, claudeJSON) + jsonBytes, err := os.ReadFile(claudeJSON) + require.NoError(t, err) + var parsed McpConfig + require.NoError(t, json.Unmarshal(jsonBytes, &parsed)) + assert.Contains(t, parsed.McpServers, shared.ServerNameKey, + "~/.claude.json should contain the Snyk MCP server entry") + }) + + t.Run("remove: deletes dedicated rules file and strips legacy CLAUDE.md block", func(t *testing.T) { + home := setupTempHome(t) + + // Pre-create both the new rules file (as if a prior add had run) AND a + // CLAUDE.md still carrying the legacy block. removeConfiguration must + // take care of both. + rulesFile := filepath.Join(home, ".claude", "rules", "snyk-security.md") + require.NoError(t, os.MkdirAll(filepath.Dir(rulesFile), 0755)) + require.NoError(t, os.WriteFile(rulesFile, []byte(snykClaudeRulesAlwaysApply), 0644)) + + userContent := "# Personal\n\n- A\n- B\n" + legacyBlock := RuleStart + "\n# Old Snyk\nOld.\n" + RuleEnd + "\n" + claudeMd := filepath.Join(home, ".claude", "CLAUDE.md") + require.NoError(t, os.WriteFile(claudeMd, []byte(userContent+legacyBlock), 0644)) + + // Pre-seed an MCP server entry so removeConfiguration has something to + // strip. + claudeJSON := filepath.Join(home, ".claude.json") + seed := []byte(`{"mcpServers":{"Snyk":{"command":"/x","args":["mcp","-t","stdio"],"env":{}}}}`) + require.NoError(t, os.WriteFile(claudeJSON, seed, 0644)) + + require.NoError(t, Configure(logger, makeConfig(true), uiStub, "/usr/local/bin/snyk")) + + // Dedicated rules file is gone. + _, statErr := os.Stat(rulesFile) + assert.True(t, os.IsNotExist(statErr), "dedicated rules file should be removed") + + // Legacy block stripped, user content preserved. + md, err := os.ReadFile(claudeMd) + require.NoError(t, err) + assert.NotContains(t, string(md), RuleStart) + assert.NotContains(t, string(md), "Old Snyk") + assert.Contains(t, string(md), "# Personal") + + // MCP entry removed from ~/.claude.json. + jsonBytes, err := os.ReadFile(claudeJSON) + require.NoError(t, err) + var parsed McpConfig + require.NoError(t, json.Unmarshal(jsonBytes, &parsed)) + assert.NotContains(t, parsed.McpServers, shared.ServerNameKey) + }) + + t.Run("MCP-only configure (configureRules=false) still runs legacy cleanup", func(t *testing.T) { + // Pins F12: migration housekeeping must run regardless of whether the + // user is configuring rules this invocation. A user running + // `--configure-rules=false --configure-mcp=true` who previously + // installed the delimited variant should still get their CLAUDE.md + // cleaned up. + home := setupTempHome(t) + + userContent := "# Personal\n" + legacyBlock := RuleStart + "\n# stale snyk\n" + RuleEnd + "\n" + claudeMd := filepath.Join(home, ".claude", "CLAUDE.md") + require.NoError(t, os.WriteFile(claudeMd, []byte(userContent+legacyBlock), 0644)) + + c := configuration.New() + c.Set(shared.ToolNameParam, "claude-cli") + c.Set(shared.RemoveParam, false) + c.Set(shared.RuleTypeParam, shared.RuleTypeAlwaysApply) + c.Set(shared.RulesScopeParam, shared.RulesGlobalScope) + c.Set(shared.WorkspacePathParam, "") + c.Set(shared.ConfigureMcpParam, true) + c.Set(shared.ConfigureRulesParam, false) // <- the gate the legacy cleanup must NOT respect + require.NoError(t, Configure(logger, c, uiStub, "/usr/local/bin/snyk")) + + // Dedicated rules file should NOT have been written (configureRules=false). + _, statErr := os.Stat(filepath.Join(home, ".claude", "rules", "snyk-security.md")) + assert.True(t, os.IsNotExist(statErr), "rules file must not be written when configureRules=false") + + // But the legacy block in CLAUDE.md MUST still be cleaned up. + md, err := os.ReadFile(claudeMd) + require.NoError(t, err) + assert.NotContains(t, string(md), RuleStart, "legacy cleanup must run even when configureRules=false") + assert.NotContains(t, string(md), "stale snyk") + assert.Contains(t, string(md), "# Personal") + }) +} From 43d6c2c363dbb1558a7da9f1fa6df8067584a68c Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 15:49:48 -0500 Subject: [PATCH 5/6] fix(configure): line-anchor delimited block matching to preserve user content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit removeDelimitedBlock and upsertDelimitedBlock previously located markers via strings.Index (first occurrence). A user's CLAUDE.md that quoted the literal RuleStart token in a sentence above the real Snyk block — e.g. a how-to doc explaining the migration — would have everything between the user's quote and Snyk's real RuleEnd silently deleted by the migration cleanup. The new migration runs unconditionally on every claude-cli install, so the failure mode would compound over time. This commit replaces the index-based search with line-anchored matching: both markers must appear on their own line (no surrounding text), which is exactly what writeGlobalRules / writeGlobalSkills emit. User content that quotes the marker text inline within a sentence no longer matches and is preserved. When multiple complete delimited pairs exist (e.g. user pasted the marker on its own line in a fenced code block AFTER the real block), the new findDelimitedBlockLines helper scans from the end and returns the last START that has a matching END after it — biased toward the real block rather than orphan user-quoted markers. A new TestRemoveDelimitedBlock_LineAnchored_PreservesUserQuotedMarkers pins both pathological cases. The existing TestRemoveDelimitedBlock and TestUpsertDelimitedBlock cases pass unchanged — the line-anchored algorithm produces identical output for any input the prior implementation got right; it differs only in the user-quoted-marker case it was getting wrong. Addresses Tribunal finding #F2 (data destruction / migration safety, score 80) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Co-Authored-By: Claude Opus 4.7 (1M context) -- ᴛ-ᴅᴜʙ's Review Tribunal ⚖️ --- internal/configure/configure_test.go | 58 ++++++++++++++++++++++ internal/configure/rules.go | 72 +++++++++++++++++++++------- 2 files changed, 112 insertions(+), 18 deletions(-) diff --git a/internal/configure/configure_test.go b/internal/configure/configure_test.go index 30f24fc..30c6fa3 100644 --- a/internal/configure/configure_test.go +++ b/internal/configure/configure_test.go @@ -1251,6 +1251,64 @@ func TestRemoveDelimitedBlock(t *testing.T) { } } +// TestRemoveDelimitedBlock_LineAnchored_PreservesUserQuotedMarkers is a +// regression for the security finding that prior first-occurrence matching +// could destroy user content. Pre-fix: a CLAUDE.md that quoted the literal +// marker text inline (e.g. in a how-to doc) before the real Snyk block +// would have everything between the user's quote and Snyk's real end +// marker silently deleted. Post-fix (line-anchored matching): the inline +// quote does not match (markers must be on their own line), so user +// content above the real block survives intact. +func TestRemoveDelimitedBlock_LineAnchored_PreservesUserQuotedMarkers(t *testing.T) { + t.Run("user quotes marker inline in a sentence above real block", func(t *testing.T) { + userPreamble := "# How Snyk migration works\n\n" + + "Snyk wraps its rules with `" + RuleStart + "` and `" + RuleEnd + "` markers.\n" + + "Cleanup of legacy installs strips that delimited block.\n\n" + realBlock := RuleStart + "\n# Snyk Security At Inception\nReal content.\n" + RuleEnd + "\n" + userTrailer := "\n# More notes\nAfter the block.\n" + + source := userPreamble + realBlock + userTrailer + result := removeDelimitedBlock(source, RuleStart, RuleEnd) + + // User prose around the real block survives intact. + assert.Contains(t, result, "# How Snyk migration works") + assert.Contains(t, result, "Cleanup of legacy installs strips") + assert.Contains(t, result, "# More notes") + assert.Contains(t, result, "After the block.") + + // The quoted marker tokens INSIDE the user's sentence are preserved + // (they're user-authored prose, not Snyk's actual delimiters). + assert.Contains(t, result, "Snyk wraps its rules with `"+RuleStart+"`") + + // Real block content is gone. + assert.NotContains(t, result, "Real content.") + // AND no orphan Snyk block markers should remain on their own line. + // (We can't simply NotContains the marker tokens — they survive in + // the user's quoted prose. Verify line-anchored absence instead.) + for _, line := range strings.Split(result, "\n") { + assert.NotEqual(t, RuleStart, line, "no orphan RuleStart line should remain") + assert.NotEqual(t, RuleEnd, line, "no orphan RuleEnd line should remain") + } + }) + + t.Run("user has marker on its own line AFTER real block — last-start-with-matching-end picks the real one", func(t *testing.T) { + realBlock := RuleStart + "\n# Real Snyk\nReal content.\n" + RuleEnd + // User pasted the start marker on its own line below, e.g. in a code + // block they were writing. There's no matching end after it, so the + // real block above is the only complete pair. + userBelow := "\n\n```\n" + RuleStart + "\n```\n\nMy notes.\n" + + source := realBlock + userBelow + result := removeDelimitedBlock(source, RuleStart, RuleEnd) + + // Real block stripped. + assert.NotContains(t, result, "Real content.") + // Orphan user-pasted marker survives (it's inside a fenced code block). + assert.Contains(t, result, "```\n"+RuleStart+"\n```") + assert.Contains(t, result, "My notes.") + }) +} + // TestConfigure_ClaudeCliEndToEnd exercises the full add → remove cycle for // the claude-cli host. It pins three behaviors that helper-level tests do // NOT cover: that addConfiguration writes the new dedicated rules file, diff --git a/internal/configure/rules.go b/internal/configure/rules.go index 56575ea..c729dd3 100644 --- a/internal/configure/rules.go +++ b/internal/configure/rules.go @@ -154,22 +154,54 @@ func removeGlobalRules(targetFile string, logger *zerolog.Logger) error { return nil } -// removeDelimitedBlock removes a delimited block from the content +// findDelimitedBlockLines locates a delimited block in a normalized source +// using line-anchored matching: both markers must appear on their own line +// (no surrounding text on the same line), which matches what +// writeGlobalRules / writeGlobalSkills emit. This avoids destroying user +// content that quotes the marker text inline (e.g. in a how-to doc) and +// is robust against multiple complete blocks: scanning from the end +// returns the LAST start that has a matching end after it. +// +// Returns the (startLine, endLine) zero-based line indices, or (-1, -1) +// when no complete delimited pair is found. +func findDelimitedBlockLines(lines []string, start, end string) (int, int) { + for i := len(lines) - 1; i >= 0; i-- { + if lines[i] != start { + continue + } + for j := i + 1; j < len(lines); j++ { + if lines[j] == end { + return i, j + } + } + } + return -1, -1 +} + +// removeDelimitedBlock removes a delimited block from the content. Matching +// is line-anchored (see findDelimitedBlockLines); a user's CLAUDE.md that +// quotes the literal marker text inline within a sentence is preserved. func removeDelimitedBlock(source, start, end string) string { // Normalize newlines to \n src := strings.ReplaceAll(source, "\r\n", "\n") + lines := strings.Split(src, "\n") - startIdx := strings.Index(src, start) - endIdx := strings.Index(src, end) - - if startIdx == -1 || endIdx == -1 || endIdx <= startIdx { - // No block found + startLine, endLine := findDelimitedBlockLines(lines, start, end) + if startLine == -1 { + // No complete delimited pair found; leave source untouched. This + // returns the ORIGINAL source (not normalized) so a CRLF file with + // no Snyk block survives byte-for-byte. return source } - // Remove from start marker to end marker (inclusive of the end marker) - before := trimTrailingNewlines(src[:startIdx]) - after := trimLeadingNewlines(src[endIdx+len(end):]) + before := strings.Join(lines[:startLine], "\n") + after := "" + if endLine+1 < len(lines) { + after = strings.Join(lines[endLine+1:], "\n") + } + + before = trimTrailingNewlines(before) + after = trimLeadingNewlines(after) separator := "" if before != "" && after != "" { @@ -185,18 +217,22 @@ func removeDelimitedBlock(source, start, end string) string { return result } -// upsertDelimitedBlock replaces or appends a delimited block inside a file content +// upsertDelimitedBlock replaces or appends a delimited block inside a file +// content. Matching is line-anchored and uses the same robust pair-finding +// rule as removeDelimitedBlock — see findDelimitedBlockLines. func upsertDelimitedBlock(source, start, end, fullBlockToInsert string) string { // Normalize newlines to \n src := strings.ReplaceAll(source, "\r\n", "\n") - - startIdx := strings.Index(src, start) - endIdx := strings.Index(src, end) - - if startIdx != -1 && endIdx != -1 && endIdx > startIdx { - // Replace from start marker to end marker (inclusive) - before := src[:startIdx] - after := src[endIdx+len(end):] + lines := strings.Split(src, "\n") + + startLine, endLine := findDelimitedBlockLines(lines, start, end) + if startLine != -1 { + // Replace from startLine to endLine (inclusive) + before := strings.Join(lines[:startLine], "\n") + after := "" + if endLine+1 < len(lines) { + after = strings.Join(lines[endLine+1:], "\n") + } return trimTrailingNewlines(before) + "\n" + strings.TrimSpace(fullBlockToInsert) + "\n" + trimLeadingNewlines(after) } From 37d0d4cd058510d31d89b71e4a99c4244c6a2124 Mon Sep 17 00:00:00 2001 From: "t-dub (Tom Wolf)" Date: Tue, 5 May 2026 15:51:39 -0500 Subject: [PATCH 6/6] refactor(configure): extract legacy-cleanup helpers; surface global cleanup in UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts the four near-identical legacy-cleanup blocks (legacy_local + legacy_global, in both addConfiguration and removeConfiguration) into two small helpers: cleanupLegacyLocalRules (workspace-relative, quiet) and cleanupLegacyGlobalRules (announces in the CLI, surfaces failures with a concrete file path so the user can inspect manually). The two-helper split (rather than one bundled cleanupLegacyPaths) reflects the different gating: cleanupLegacyLocalRules is still called from inside the configureRules block (its local→global migration semantics are tied to rules being configured this run), while cleanupLegacyGlobalRules is called from outside the gate (migration housekeeping is unconditional — see prior commit ac19d72). The user-facing breadcrumb on legacy global cleanup matters because the whole point of moving claude-cli rules out of CLAUDE.md is to stop silently mutating a user-owned file. Announcing the cleanup before it runs and surfacing failures keeps that contract honest: a user who's now told their CLAUDE.md will be edited can see when it actually happens. Addresses Tribunal finding #F4 (duplication / DRY, score 80) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Addresses Tribunal finding #F10 (silent failure / user-visibility, score 65) from review tribunal-2026-05-05T20-11-37Z-pr-35-f6cf7a69.json. Co-Authored-By: Claude Opus 4.7 (1M context) -- ᴛ-ᴅᴜʙ's Review Tribunal ⚖️ --- internal/configure/configure.go | 88 ++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/internal/configure/configure.go b/internal/configure/configure.go index 31603ff..80a6494 100644 --- a/internal/configure/configure.go +++ b/internal/configure/configure.go @@ -129,30 +129,15 @@ func removeConfiguration(logger *zerolog.Logger, config configuration.Configurat logger.Info().Msgf("Successfully removed local rules for %s", ideConf.name) } - // Clean up legacy local rules - if ideConf.legacyLocalRulesPath != "" && workspacePath != "" { - err := removeLocalRules(workspacePath, ideConf.legacyLocalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) - } - } + cleanupLegacyLocalRules(logger, workspacePath, ideConf) } - // Clean up legacy global rules block left behind by a prior install that - // injected delimited rules into a shared file (e.g. claude-cli installs - // before the move to ~/.claude/rules/). This runs OUTSIDE the - // configureRules gate intentionally: a user invoking remove with - // configureRules=false (e.g. wanting to remove only the MCP entry) still - // expects stale Snyk content in their personal CLAUDE.md to be cleaned up - // — migration housekeeping is independent of whether rules are being - // configured this run. removeGlobalRules is idempotent (no-op when the - // delimited block is absent), so this is safe regardless. - if ideConf.legacyGlobalRulesPath != "" { - err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) - } - } + // Migration housekeeping runs OUTSIDE the configureRules gate: a user + // invoking remove with configureRules=false (e.g. wanting to remove only + // the MCP entry) still expects stale Snyk content in their personal + // CLAUDE.md to be cleaned up — migration is independent of whether rules + // are configured this run. + cleanupLegacyGlobalRules(logger, userInterface, ideConf) _ = userInterface.Output("\n🎉 Removal complete!") _ = userInterface.Output("\nNext steps:") @@ -298,30 +283,15 @@ func addConfiguration(logger *zerolog.Logger, config configuration.Configuration logger.Info().Msgf("Successfully wrote local rules for %s", ideConf.name) } - // Clean up legacy local rules when migrating to global rules/skills - if ideConf.legacyLocalRulesPath != "" && workspacePath != "" { - err := removeLocalRules(workspacePath, ideConf.legacyLocalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) - } - } + cleanupLegacyLocalRules(logger, workspacePath, ideConf) } - // Clean up legacy global rules block when migrating to a dedicated rules - // file (e.g. claude-cli moved from delimited block in ~/.claude/CLAUDE.md - // to ~/.claude/rules/snyk-security.md). This runs OUTSIDE the - // configureRules gate intentionally: a user running configure with + // Migration housekeeping runs OUTSIDE the configureRules gate (see + // cleanupLegacyGlobalRules): a user running configure with // configureRules=false (MCP-only) has still abandoned the legacy // location, so the stale block in their personal CLAUDE.md should be // cleaned up regardless of whether rules are being configured this run. - // removeGlobalRules only strips the delimited block, leaving any user - // content in the legacy file intact. - if ideConf.legacyGlobalRulesPath != "" { - err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) - if err != nil { - logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) - } - } + cleanupLegacyGlobalRules(logger, userInterface, ideConf) _ = userInterface.Output("\n🎉 Configuration complete!") _ = userInterface.Output("\nNext steps:") @@ -331,6 +301,42 @@ func addConfiguration(logger *zerolog.Logger, config configuration.Configuration return nil } +// cleanupLegacyLocalRules removes a workspace-relative legacy rules file +// left behind by an older install (e.g. .cursor/rules/snyk_rules.mdc from +// before the move to global skills). Best-effort: errors are logged at warn +// level and execution continues. Quiet by design — local-rules cleanup +// piggybacks on the rules-write call site and doesn't warrant its own user +// breadcrumb. +func cleanupLegacyLocalRules(logger *zerolog.Logger, workspacePath string, ideConf *hostConfig) { + if ideConf.legacyLocalRulesPath == "" || workspacePath == "" { + return + } + err := removeLocalRules(workspacePath, ideConf.legacyLocalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy local rules at %s", ideConf.legacyLocalRulesPath) + } +} + +// cleanupLegacyGlobalRules strips a Snyk delimited block left behind in a +// legacy shared file (e.g. ~/.claude/CLAUDE.md from claude-cli installs +// before the move to ~/.claude/rules/). Mutating a user-owned file is the +// kind of thing the user should see in the CLI output, so this helper +// announces the cleanup before running and surfaces any failure with a +// concrete pointer back to the legacy file. removeGlobalRules is +// idempotent (no-op when the delimited block is absent), so calling this +// on every configure/remove run is safe. +func cleanupLegacyGlobalRules(logger *zerolog.Logger, userInterface ui.UserInterface, ideConf *hostConfig) { + if ideConf.legacyGlobalRulesPath == "" { + return + } + _ = userInterface.Output(fmt.Sprintf("📋 Cleaning up legacy global rules block in: %s", ideConf.legacyGlobalRulesPath)) + err := removeGlobalRules(ideConf.legacyGlobalRulesPath, logger) + if err != nil { + logger.Warn().Err(err).Msgf("Unable to clean up legacy global rules at %s", ideConf.legacyGlobalRulesPath) + _ = userInterface.Output(fmt.Sprintf("⚠️ Failed to clean up legacy global rules at %s — please inspect this file manually for the Snyk delimited block.", ideConf.legacyGlobalRulesPath)) + } +} + // determineCommand returns the command and args based on execution context func determineCommand(cliPath, integrationName string) (string, []string) { if utils.IsRunningFromNpm(integrationName) {