diff --git a/src/lib/generate.test.ts b/src/lib/generate.test.ts index 94ec411ab..b4d0f6c59 100644 --- a/src/lib/generate.test.ts +++ b/src/lib/generate.test.ts @@ -12,7 +12,7 @@ import { SkillsProcessor } from "../features/skills/skills-processor.js"; import { SubagentsProcessor } from "../features/subagents/subagents-processor.js"; import { createMockLogger } from "../test-utils/mock-logger.js"; import { fileExists, readFileContentOrNull } from "../utils/file.js"; -import { checkRulesyncDirExists, generate } from "./generate.js"; +import { checkRulesyncDirExists, generate, resolveExecutionOrder } from "./generate.js"; const logger = createMockLogger(); @@ -1062,3 +1062,89 @@ describe("generate", () => { }); }); }); + +const executionStep = ( + id: string, + opts: { writesSharedFile?: string[]; dependsOn?: string[] } = {}, +) => + ({ + id, + ...opts, + run: async () => ({ count: 0, paths: [], hasDiff: false }), + }) as never; + +describe("resolveExecutionOrder", () => { + const step = executionStep; + + it("orders a dependent step after its dependency regardless of array order", () => { + const ordered = resolveExecutionOrder([ + step("rules", { writesSharedFile: ["f"], dependsOn: ["mcp"] }), + step("mcp", { writesSharedFile: ["f"] }), + ]); + expect(ordered.map((s) => s.id)).toEqual(["mcp", "rules"]); + }); + + it("orders a value-only dependency (no shared file) before its dependent", () => { + const ordered = resolveExecutionOrder([ + step("rules", { dependsOn: ["skills"] }), + step("skills"), + ]); + expect(ordered.map((s) => s.id)).toEqual(["skills", "rules"]); + }); + + it("throws when two steps write the same shared file without a declared order", () => { + expect(() => + resolveExecutionOrder([ + step("ignore", { writesSharedFile: ["claude-settings"] }), + step("permissions", { writesSharedFile: ["claude-settings"] }), + ]), + ).toThrow(/both write the shared file 'claude-settings'/); + }); + + it("does not throw when shared-file writers are ordered by dependsOn", () => { + expect(() => + resolveExecutionOrder([ + step("ignore", { writesSharedFile: ["claude-settings"] }), + step("permissions", { + writesSharedFile: ["claude-settings"], + dependsOn: ["ignore"], + }), + ]), + ).not.toThrow(); + }); + + it("throws when one of three writers is not ordered against the others", () => { + expect(() => + resolveExecutionOrder([ + step("ignore", { writesSharedFile: ["claude-settings"] }), + step("hooks", { writesSharedFile: ["claude-settings"], dependsOn: ["ignore"] }), + step("permissions", { writesSharedFile: ["claude-settings"], dependsOn: ["ignore"] }), + ]), + ).toThrow(/both write the shared file 'claude-settings'/); + }); + + it("does not throw when all three writers are totally ordered", () => { + expect(() => + resolveExecutionOrder([ + step("ignore", { writesSharedFile: ["claude-settings"] }), + step("hooks", { writesSharedFile: ["claude-settings"], dependsOn: ["ignore"] }), + step("permissions", { + writesSharedFile: ["claude-settings"], + dependsOn: ["ignore", "hooks"], + }), + ]), + ).not.toThrow(); + }); + + it("throws on an unknown dependency", () => { + expect(() => resolveExecutionOrder([step("rules", { dependsOn: ["mcp"] })])).toThrow( + /unknown step 'mcp'/, + ); + }); + + it("throws on a cyclic dependency", () => { + expect(() => + resolveExecutionOrder([step("a", { dependsOn: ["b"] }), step("b", { dependsOn: ["a"] })]), + ).toThrow(/cyclic/); + }); +}); diff --git a/src/lib/generate.ts b/src/lib/generate.ts index 49ae59633..aca2d019f 100644 --- a/src/lib/generate.ts +++ b/src/lib/generate.ts @@ -188,6 +188,118 @@ export async function checkRulesyncDirExists(params: { inputRoot: string }): Pro return fileExists(join(params.inputRoot, RULESYNC_RELATIVE_DIR_PATH)); } +type GenerationStepId = + | "ignore" + | "mcp" + | "commands" + | "subagents" + | "skills" + | "hooks" + | "permissions" + | "rules"; + +type GenerationStep = { + id: GenerationStepId; + /** Tokens for on-disk files this step read-modify-writes and shares with other steps. */ + writesSharedFile?: string[]; + /** Step ids that must run before this one (they write a shared file this step then reads). */ + dependsOn?: GenerationStepId[]; + run: () => Promise; +}; + +function dependsOnReachable( + byId: Map, + from: GenerationStepId, + target: GenerationStepId, +): boolean { + const seen = new Set(); + const stack = [from]; + while (stack.length > 0) { + const current = stack.pop(); + if (current === undefined || seen.has(current)) continue; + seen.add(current); + if (current === target) return true; + for (const dep of byId.get(current)?.dependsOn ?? []) { + stack.push(dep); + } + } + return false; +} + +function assertSharedFilesOrdered( + steps: GenerationStep[], + byId: Map, +): void { + const writersByFile = new Map(); + for (const step of steps) { + for (const file of step.writesSharedFile ?? []) { + writersByFile.set(file, [...(writersByFile.get(file) ?? []), step.id]); + } + } + for (const [file, writers] of writersByFile) { + for (let i = 0; i < writers.length; i++) { + for (let j = i + 1; j < writers.length; j++) { + const a = writers[i]!; + const b = writers[j]!; + if (!dependsOnReachable(byId, a, b) && !dependsOnReachable(byId, b, a)) { + throw new Error( + `Generation steps '${a}' and '${b}' both write the shared file '${file}' ` + + `but neither declares a 'dependsOn' the other. Add a 'dependsOn' so the ` + + `read-modify-write order is fixed; otherwise one step silently drops the ` + + `other's keys.`, + ); + } + } + } + } +} + +/** + * Topologically sort generation steps and reject ordering hazards: a shared file + * with two writers not ordered by `dependsOn` (a silent data-loss trap), an + * unknown dependency, or a cycle. Reordering `steps` stays safe as a result. + * + * @throws Error if a shared file has unordered writers, a dependency is unknown, + * or the dependency graph contains a cycle. + */ +export function resolveExecutionOrder(steps: GenerationStep[]): GenerationStep[] { + const byId = new Map(steps.map((step) => [step.id, step])); + + assertSharedFilesOrdered(steps, byId); + + const unresolvedDeps = new Map(steps.map((step) => [step.id, 0])); + const dependents = new Map(); + for (const step of steps) { + for (const dep of step.dependsOn ?? []) { + if (!byId.has(dep)) { + throw new Error(`Generation step '${step.id}' depends on unknown step '${dep}'.`); + } + unresolvedDeps.set(step.id, (unresolvedDeps.get(step.id) ?? 0) + 1); + dependents.set(dep, [...(dependents.get(dep) ?? []), step.id]); + } + } + + const ready = steps + .filter((step) => (unresolvedDeps.get(step.id) ?? 0) === 0) + .map((step) => step.id); + const ordered: GenerationStep[] = []; + while (ready.length > 0) { + const id = ready.shift()!; + ordered.push(byId.get(id)!); + for (const dependent of dependents.get(id) ?? []) { + const next = (unresolvedDeps.get(dependent) ?? 0) - 1; + unresolvedDeps.set(dependent, next); + if (next === 0) ready.push(dependent); + } + } + + if (ordered.length !== steps.length) { + throw new Error("Generation steps contain a cyclic 'dependsOn' dependency."); + } + + return ordered; +} + /** * Generate configuration files for AI tools. * @throws Error if generation fails @@ -198,53 +310,118 @@ export async function generate(params: { }): Promise { const { config, logger } = params; - const ignoreResult = await generateIgnoreCore({ config, logger }); - // NOTE: For Kilo, MCP MUST run before Rules. Both features merge into the - // shared `kilo.jsonc` (mcp writes the `mcp`/`tools` keys, rules writes the - // `instructions` key). Each reads the existing file from disk and preserves - // the other's keys, so running mcp first lets the rules write see the freshly - // written `mcp` block. Reordering or parallelizing these calls would drop one - // of the two keys. - const mcpResult = await generateMcpCore({ config, logger }); - - const commandsResult = await generateCommandsCore({ config, logger }); - const subagentsResult = await generateSubagentsCore({ config, logger }); - const skillsResult = await generateSkillsCore({ config, logger }); - const hooksResult = await generateHooksCore({ config, logger }); - // NOTE: Permissions MUST run after ignore. Both features write to `.claude/settings.json` - // (ignore writes Read deny entries, permissions merges all permission arrays). - // Permissions reads the file written by ignore and preserves non-managed entries. - // Changing this order or parallelizing these calls will cause data loss. - const permissionsResult = await generatePermissionsCore({ config, logger }); - const rulesResult = await generateRulesCore({ config, logger, skills: skillsResult.skills }); - - const hasDiff = - ignoreResult.hasDiff || - mcpResult.hasDiff || - commandsResult.hasDiff || - subagentsResult.hasDiff || - skillsResult.hasDiff || - hooksResult.hasDiff || - permissionsResult.hasDiff || - rulesResult.hasDiff; + // Captured by the skills step so the rules step can read the generated skills. + let skillsResult: Awaited> | undefined; + + const steps: GenerationStep[] = [ + { + id: "ignore", + writesSharedFile: ["claude-settings", "zed-settings"], + run: () => generateIgnoreCore({ config, logger }), + }, + { + id: "mcp", + writesSharedFile: [ + "kilo-opencode-config", + "zed-settings", + "qwencode-settings", + "augmentcode-settings", + "hermesagent-config", + "amp-settings", + "codexcli-config", + "grokcli-config", + "vibe-config", + ], + dependsOn: ["ignore"], + run: () => generateMcpCore({ config, logger }), + }, + { id: "commands", run: () => generateCommandsCore({ config, logger }) }, + { id: "subagents", run: () => generateSubagentsCore({ config, logger }) }, + { + id: "skills", + run: async () => { + skillsResult = await generateSkillsCore({ config, logger }); + return skillsResult; + }, + }, + { + id: "hooks", + writesSharedFile: [ + "claude-settings", + "qwencode-settings", + "augmentcode-settings", + "hermesagent-config", + "kiro-agent-config", + "codexcli-config", + "vibe-config", + ], + dependsOn: ["ignore", "mcp"], + run: () => generateHooksCore({ config, logger }), + }, + { + id: "permissions", + writesSharedFile: [ + "claude-settings", + "kilo-opencode-config", + "zed-settings", + "qwencode-settings", + "augmentcode-settings", + "hermesagent-config", + "kiro-agent-config", + "amp-settings", + "codexcli-config", + "grokcli-config", + "vibe-config", + ], + dependsOn: ["ignore", "hooks", "mcp"], + run: () => generatePermissionsCore({ config, logger }), + }, + { + id: "rules", + writesSharedFile: ["kilo-opencode-config"], + dependsOn: ["mcp", "skills", "permissions"], + run: () => generateRulesCore({ config, logger, skills: skillsResult?.skills }), + }, + ]; + + const orderedSteps = resolveExecutionOrder(steps); + + const resultsById = new Map(); + for (const step of orderedSteps) { + resultsById.set(step.id, await step.run()); + } + + if (!skillsResult) { + throw new Error("Skills generation step did not run."); + } + + const get = (id: GenerationStepId): FeatureGenerateResult => { + const result = resultsById.get(id); + if (!result) { + throw new Error(`Missing generation result for step '${id}'.`); + } + return result; + }; + + const hasDiff = orderedSteps.some((step) => get(step.id).hasDiff); return { - rulesCount: rulesResult.count, - rulesPaths: rulesResult.paths, - ignoreCount: ignoreResult.count, - ignorePaths: ignoreResult.paths, - mcpCount: mcpResult.count, - mcpPaths: mcpResult.paths, - commandsCount: commandsResult.count, - commandsPaths: commandsResult.paths, - subagentsCount: subagentsResult.count, - subagentsPaths: subagentsResult.paths, + rulesCount: get("rules").count, + rulesPaths: get("rules").paths, + ignoreCount: get("ignore").count, + ignorePaths: get("ignore").paths, + mcpCount: get("mcp").count, + mcpPaths: get("mcp").paths, + commandsCount: get("commands").count, + commandsPaths: get("commands").paths, + subagentsCount: get("subagents").count, + subagentsPaths: get("subagents").paths, skillsCount: skillsResult.count, skillsPaths: skillsResult.paths, - hooksCount: hooksResult.count, - hooksPaths: hooksResult.paths, - permissionsCount: permissionsResult.count, - permissionsPaths: permissionsResult.paths, + hooksCount: get("hooks").count, + hooksPaths: get("hooks").paths, + permissionsCount: get("permissions").count, + permissionsPaths: get("permissions").paths, skills: skillsResult.skills, hasDiff, };