diff --git a/src/cli/commands/generate.test.ts b/src/cli/commands/generate.test.ts index d27ddc08e..a7c225b04 100644 --- a/src/cli/commands/generate.test.ts +++ b/src/cli/commands/generate.test.ts @@ -686,6 +686,73 @@ describe("generateCommand", () => { }); }); + describe("check mode", () => { + it("should fail when check mode would delete orphan files", async () => { + mockConfig.getFeatures.mockReturnValue(["rules"]); + mockConfig.getCheck.mockReturnValue(true); + mockConfig.getDelete.mockReturnValue(true); + mockConfig.isPreviewMode.mockReturnValue(true); + + const rulesMock = { + loadToolFiles: vi.fn().mockResolvedValue([ + { + getFilePath: () => "/path/to/orphan", + }, + ]), + removeOrphanAiFiles: vi.fn().mockResolvedValue(1), + loadRulesyncFiles: vi.fn().mockResolvedValue([ + { + getFilePath: () => "/path/to/rulesync", + }, + ]), + convertRulesyncFilesToToolFiles: vi.fn().mockResolvedValue([ + { + getFilePath: () => "/path/to/converted", + }, + ]), + writeAiFiles: vi.fn().mockResolvedValue({ count: 0, paths: [] }), + }; + vi.mocked(RulesProcessor).mockImplementation(function () { + return rulesMock as any; + }); + + await expect(generateCommand(mockLogger, {})).rejects.toThrow( + "Files are not up to date. Run 'rulesync generate' to update.", + ); + + expect(mockLogger.info).not.toHaveBeenCalledWith("✓ All files are up to date (rules)"); + }); + + it("should succeed when check mode finds no diff", async () => { + mockConfig.getFeatures.mockReturnValue(["rules"]); + mockConfig.getCheck.mockReturnValue(true); + mockConfig.isPreviewMode.mockReturnValue(true); + + const rulesMock = { + loadToolFiles: vi.fn().mockResolvedValue([]), + removeOrphanAiFiles: vi.fn().mockResolvedValue(0), + loadRulesyncFiles: vi.fn().mockResolvedValue([ + { + getFilePath: () => "/path/to/rulesync", + }, + ]), + convertRulesyncFilesToToolFiles: vi.fn().mockResolvedValue([ + { + getFilePath: () => "/path/to/converted", + }, + ]), + writeAiFiles: vi.fn().mockResolvedValue({ count: 0, paths: [] }), + }; + vi.mocked(RulesProcessor).mockImplementation(function () { + return rulesMock as any; + }); + + await generateCommand(mockLogger, {}); + + expect(mockLogger.success).toHaveBeenCalledWith("✓ All files are up to date."); + }); + }); + describe("error handling", () => { it("should handle ConfigResolver errors", async () => { vi.mocked(ConfigResolver.resolve).mockRejectedValue(new Error("Config error")); diff --git a/src/cli/commands/generate.ts b/src/cli/commands/generate.ts index 799460e98..2995a703f 100644 --- a/src/cli/commands/generate.ts +++ b/src/cli/commands/generate.ts @@ -119,6 +119,19 @@ export async function generateCommand(logger: Logger, options: GenerateOptions): logger.captureData("skills", result.skills ?? []); } + // Check mode must fail even when the change is delete-only and no files are written. + if (check) { + if (result.hasDiff) { + throw new CLIError( + "Files are not up to date. Run 'rulesync generate' to update.", + ErrorCodes.GENERATION_FAILED, + ); + } + + logger.success("✓ All files are up to date."); + return; + } + if (totalGenerated === 0) { const enabledFeatures = features.join(", "); logger.info(`✓ All files are up to date (${enabledFeatures})`); @@ -139,16 +152,4 @@ export async function generateCommand(logger: Logger, options: GenerateOptions): } else { logger.success(`🎉 All done! Written ${totalGenerated} file(s) total (${parts.join(" + ")})`); } - - // Handle --check mode exit code - if (check) { - if (result.hasDiff) { - throw new CLIError( - "Files are not up to date. Run 'rulesync generate' to update.", - ErrorCodes.GENERATION_FAILED, - ); - } else { - logger.success("✓ All files are up to date."); - } - } } diff --git a/src/e2e/e2e-hooks.spec.ts b/src/e2e/e2e-hooks.spec.ts index cc05c45f4..60517d362 100644 --- a/src/e2e/e2e-hooks.spec.ts +++ b/src/e2e/e2e-hooks.spec.ts @@ -62,7 +62,6 @@ describe("E2E: hooks", () => { }); it.each([ - { target: "claudecode", orphanPath: join(".claude", "settings.json") }, { target: "cursor", orphanPath: join(".cursor", "hooks.json") }, { target: "opencode", orphanPath: join(".opencode", "plugins", "rulesync-hooks.js") }, ])( @@ -91,6 +90,35 @@ describe("E2E: hooks", () => { expect(await readFileContent(join(testDir, orphanPath))).toBe("# orphan\n"); }, ); + + it("should succeed in check mode when a claudecode hooks file is non-deletable", async () => { + const testDir = getTestDir(); + + await writeFileContent(join(testDir, ".rulesync", ".gitkeep"), ""); + await writeFileContent( + join(testDir, ".claude", "settings.json"), + JSON.stringify( + { + hooks: { + SessionStart: [{ matcher: "", hooks: [{ type: "command", command: "echo hi" }] }], + }, + theme: "dark", + }, + null, + 2, + ), + ); + + const { stdout } = await runGenerate({ + target: "claudecode", + features: "hooks", + deleteFiles: true, + check: true, + env: { NODE_ENV: "e2e" }, + }); + + expect(stdout).toContain("All files are up to date."); + }); }); describe("E2E: hooks (import)", () => { diff --git a/src/e2e/e2e-ignore.spec.ts b/src/e2e/e2e-ignore.spec.ts index 2f1922c39..91cc338ce 100644 --- a/src/e2e/e2e-ignore.spec.ts +++ b/src/e2e/e2e-ignore.spec.ts @@ -44,10 +44,7 @@ credentials/ } }); - it.each([ - { target: "cursor", orphanPath: ".cursorignore" }, - { target: "claudecode", orphanPath: join(".claude", "settings.json") }, - ])( + it.each([{ target: "cursor", orphanPath: ".cursorignore" }])( "should fail in check mode when delete would remove an orphan $target ignore file", async ({ target, orphanPath }) => { const testDir = getTestDir(); @@ -73,6 +70,26 @@ credentials/ expect(await readFileContent(join(testDir, orphanPath))).toBe("# orphan\n"); }, ); + + it("should succeed in check mode when a claudecode ignore file is non-deletable", async () => { + const testDir = getTestDir(); + + await writeFileContent(join(testDir, ".rulesync", ".gitkeep"), ""); + await writeFileContent( + join(testDir, ".claude", "settings.json"), + JSON.stringify({ permissions: { deny: ["tmp/"] }, theme: "dark" }, null, 2), + ); + + const { stdout } = await runGenerate({ + target: "claudecode", + features: "ignore", + deleteFiles: true, + check: true, + env: { NODE_ENV: "e2e" }, + }); + + expect(stdout).toContain("All files are up to date."); + }); }); describe("E2E: ignore (import)", () => { diff --git a/src/e2e/e2e-mcp.spec.ts b/src/e2e/e2e-mcp.spec.ts index 18c41e3bf..c7f4844e3 100644 --- a/src/e2e/e2e-mcp.spec.ts +++ b/src/e2e/e2e-mcp.spec.ts @@ -55,8 +55,6 @@ describe("E2E: mcp", () => { it.each([ { target: "claudecode", orphanPath: ".mcp.json" }, { target: "cursor", orphanPath: join(".cursor", "mcp.json") }, - { target: "geminicli", orphanPath: join(".gemini", "settings.json") }, - { target: "codexcli", orphanPath: join(".codex", "config.toml") }, ])( "should fail in check mode when delete would remove an orphan $target mcp file", async ({ target, orphanPath }) => { @@ -84,6 +82,37 @@ describe("E2E: mcp", () => { }, ); + it.each([ + { + target: "geminicli", + outputPath: join(".gemini", "settings.json"), + content: JSON.stringify({ theme: "dark", mcpServers: {} }, null, 2), + }, + { + target: "codexcli", + outputPath: join(".codex", "config.toml"), + content: '[ui]\ntheme = "dark"\n', + }, + ])( + "should succeed in check mode when a $target mcp file is non-deletable", + async ({ target, outputPath, content }) => { + const testDir = getTestDir(); + + await writeFileContent(join(testDir, ".rulesync", ".gitkeep"), ""); + await writeFileContent(join(testDir, outputPath), content); + + const { stdout } = await runGenerate({ + target, + features: "mcp", + deleteFiles: true, + check: true, + env: { NODE_ENV: "e2e" }, + }); + + expect(stdout).toContain("All files are up to date."); + }, + ); + it("should run mcp command as daemon without errors", async () => { const testDir = getTestDir(); diff --git a/src/e2e/e2e-rules.spec.ts b/src/e2e/e2e-rules.spec.ts index b0a73fcc3..10522818b 100644 --- a/src/e2e/e2e-rules.spec.ts +++ b/src/e2e/e2e-rules.spec.ts @@ -51,6 +51,63 @@ This is a test rule for E2E testing. const generatedContent = await readFileContent(join(testDir, outputPath)); expect(generatedContent).toContain("Test Rule"); }); + + it("should fail in check mode when delete would remove an orphan rule file", async () => { + const testDir = getTestDir(); + + await writeFileContent(join(testDir, ".rulesync", ".gitkeep"), ""); + await writeFileContent(join(testDir, "CLAUDE.md"), "# orphan\n"); + + await expect( + runGenerate({ + target: "claudecode", + features: "rules", + deleteFiles: true, + check: true, + env: { NODE_ENV: "e2e" }, + }), + ).rejects.toMatchObject({ + code: 1, + stderr: expect.stringContaining( + "Files are not up to date. Run 'rulesync generate' to update.", + ), + }); + + expect(await readFileContent(join(testDir, "CLAUDE.md"))).toBe("# orphan\n"); + }); + + it("should print a single up-to-date message in check mode when there is no diff", async () => { + const testDir = getTestDir(); + + const ruleContent = `--- +root: true +targets: ["*"] +description: "Test rule" +globs: ["**/*"] +--- + +# Test Rule + +This is a test rule for E2E testing. +`; + await writeFileContent( + join(testDir, RULESYNC_RULES_RELATIVE_DIR_PATH, RULESYNC_OVERVIEW_FILE_NAME), + ruleContent, + ); + + await runGenerate({ target: "claudecode", features: "rules" }); + + const { stdout, stderr } = await runGenerate({ + target: "claudecode", + features: "rules", + check: true, + env: { NODE_ENV: "e2e" }, + }); + + expect(stderr).toBe(""); + expect(stdout.match(/All files are up to date\./g)).toHaveLength(1); + expect(stdout).not.toContain("All files are up to date (rules)"); + }); }); describe("E2E: rules (import)", () => { diff --git a/src/features/hooks/hooks-processor.ts b/src/features/hooks/hooks-processor.ts index a03bf0ce8..ba901bc98 100644 --- a/src/features/hooks/hooks-processor.ts +++ b/src/features/hooks/hooks-processor.ts @@ -240,9 +240,11 @@ export class HooksProcessor extends FeatureProcessor { } } - async loadToolFiles({ forDeletion = false }: { forDeletion?: boolean } = {}): Promise< - ToolFile[] - > { + async loadToolFiles({ + forDeletion = false, + }: { + forDeletion?: boolean; + } = {}): Promise { try { const factory = toolHooksFactories.get(this.toolTarget); if (!factory) throw new Error(`Unsupported tool target: ${this.toolTarget}`); diff --git a/src/features/skills/skills-processor.ts b/src/features/skills/skills-processor.ts index a61463ae9..982e0ba41 100644 --- a/src/features/skills/skills-processor.ts +++ b/src/features/skills/skills-processor.ts @@ -451,14 +451,13 @@ export class SkillsProcessor extends DirFeatureProcessor { const dirPaths = await findFilesByGlobs(join(skillsDirPath, "*"), { type: "dir" }); for (const dirPath of dirPaths) { const dirName = basename(dirPath); - toolSkills.push( - factory.class.forDeletion({ - baseDir: this.baseDir, - relativeDirPath: root, - dirName, - global: this.global, - }), - ); + const toolSkill = factory.class.forDeletion({ + baseDir: this.baseDir, + relativeDirPath: root, + dirName, + global: this.global, + }); + toolSkills.push(toolSkill); } } diff --git a/src/lib/generate.test.ts b/src/lib/generate.test.ts index 1a6a5c457..e90e1a2a1 100644 --- a/src/lib/generate.test.ts +++ b/src/lib/generate.test.ts @@ -72,6 +72,7 @@ describe("generate", () => { getTargets: ReturnType; getFeatures: ReturnType; getDelete: ReturnType; + getCheck: ReturnType; getGlobal: ReturnType; getSimulateCommands: ReturnType; getSimulateSubagents: ReturnType; @@ -87,6 +88,7 @@ describe("generate", () => { getTargets: vi.fn().mockReturnValue(["claudecode"]), getFeatures: vi.fn().mockReturnValue(["rules"]), getDelete: vi.fn().mockReturnValue(false), + getCheck: vi.fn().mockReturnValue(false), getGlobal: vi.fn().mockReturnValue(false), getSimulateCommands: vi.fn().mockReturnValue(false), getSimulateSubagents: vi.fn().mockReturnValue(false), @@ -833,6 +835,29 @@ describe("generate", () => { expect(result.hasDiff).toBe(true); }); + + it("should return hasDiff: true when orphan files would be deleted in dry run mode", async () => { + mockConfig.getFeatures.mockReturnValue(["rules"]); + mockConfig.getDelete.mockReturnValue(true); + mockConfig.isPreviewMode.mockReturnValue(true); + + const existingFiles = [createMockAiFile("/path/to/orphan", "orphan content")]; + const generatedFiles = [createMockAiFile("/path/to/kept", "content")]; + const mockProcessor = { + loadToolFiles: vi.fn().mockResolvedValue(existingFiles), + removeOrphanAiFiles: vi.fn().mockResolvedValue(1), + loadRulesyncFiles: vi.fn().mockResolvedValue([{ file: "test" }]), + convertRulesyncFilesToToolFiles: vi.fn().mockResolvedValue(generatedFiles), + writeAiFiles: vi.fn().mockResolvedValue({ count: 0, paths: [] }), + }; + vi.mocked(RulesProcessor).mockImplementation(function () { + return mockProcessor as unknown as RulesProcessor; + }); + + const result = await generate({ logger, config: mockConfig as never }); + + expect(result.hasDiff).toBe(true); + }); }); describe("unsupported target-feature warning", () => { diff --git a/src/lib/generate.ts b/src/lib/generate.ts index 820171ef8..ecb3b760b 100644 --- a/src/lib/generate.ts +++ b/src/lib/generate.ts @@ -17,6 +17,7 @@ import { AiFile } from "../types/ai-file.js"; import { DirFeatureProcessor } from "../types/dir-feature-processor.js"; import { FeatureProcessor } from "../types/feature-processor.js"; import type { Feature } from "../types/features.js"; +import type { RulesyncFile } from "../types/rulesync-file.js"; import type { ToolTarget } from "../types/tool-targets.js"; import { formatError } from "../utils/error.js"; import { fileExists } from "../utils/file.js"; @@ -60,6 +61,7 @@ async function processFeatureGeneration(params: { if (config.getDelete()) { const existingToolFiles = await processor.loadToolFiles({ forDeletion: true }); + const orphanCount = await processor.removeOrphanAiFiles(existingToolFiles, toolFiles); if (orphanCount > 0) hasDiff = true; } @@ -85,6 +87,7 @@ async function processDirFeatureGeneration(params: { if (config.getDelete()) { const existingToolDirs = await processor.loadToolDirsToDelete(); + const orphanCount = await processor.removeOrphanAiDirs(existingToolDirs, toolDirs); if (orphanCount > 0) hasDiff = true; } @@ -104,6 +107,7 @@ async function processEmptyFeatureGeneration(params: { if (config.getDelete()) { const existingToolFiles = await processor.loadToolFiles({ forDeletion: true }); + const orphanCount = await processor.removeOrphanAiFiles(existingToolFiles, []); if (orphanCount > 0) hasDiff = true; } @@ -111,6 +115,23 @@ async function processEmptyFeatureGeneration(params: { return { count: totalCount, paths: [], hasDiff }; } +/** + * Dispatch to processEmptyFeatureGeneration or processFeatureGeneration + * based on whether rulesync files exist. + */ +async function processFeatureWithRulesyncFiles(params: { + config: Config; + processor: FeatureProcessor; + rulesyncFiles: RulesyncFile[]; +}): Promise { + const { config, processor, rulesyncFiles } = params; + if (rulesyncFiles.length === 0) { + return processEmptyFeatureGeneration({ config, processor }); + } + const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); + return processFeatureGeneration({ config, processor, toolFiles }); +} + const SIMULATE_OPTION_MAP: Partial> = { commands: "--simulate-commands", subagents: "--simulate-subagents", @@ -228,13 +249,7 @@ async function generateRulesCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - - const result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths); @@ -283,21 +298,7 @@ async function generateIgnoreCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - let result; - - if (rulesyncFiles.length > 0) { - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); - } else { - result = await processEmptyFeatureGeneration({ - config, - processor, - }); - } + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths); @@ -349,13 +350,7 @@ async function generateMcpCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - - const result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths); @@ -405,13 +400,7 @@ async function generateCommandsCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - - const result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths); @@ -461,13 +450,7 @@ async function generateSubagentsCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - - const result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths); @@ -577,21 +560,7 @@ async function generateHooksCore(params: { }); const rulesyncFiles = await processor.loadRulesyncFiles(); - let result; - - if (rulesyncFiles.length === 0) { - result = await processEmptyFeatureGeneration({ - config, - processor, - }); - } else { - const toolFiles = await processor.convertRulesyncFilesToToolFiles(rulesyncFiles); - result = await processFeatureGeneration({ - config, - processor, - toolFiles, - }); - } + const result = await processFeatureWithRulesyncFiles({ config, processor, rulesyncFiles }); totalCount += result.count; allPaths.push(...result.paths);