diff --git a/.oxlintrc.json b/.oxlintrc.json index 8775d750b..5fa7d359a 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -11,6 +11,7 @@ }, "rules": { + "complexity": ["error", { "max": 20 }], "eqeqeq": "error", "import/no-cycle": "error", "no-console": "error", diff --git a/src/cli/commands/generate.ts b/src/cli/commands/generate.ts index 82f82d793..1a39ca434 100644 --- a/src/cli/commands/generate.ts +++ b/src/cli/commands/generate.ts @@ -1,5 +1,5 @@ import { ConfigResolver, type ConfigResolverResolveParams } from "../../config/config-resolver.js"; -import { checkRulesyncDirExists, generate } from "../../lib/generate.js"; +import { checkRulesyncDirExists, generate, type GenerateResult } from "../../lib/generate.js"; import { CLIError, ErrorCodes } from "../../types/json-output.js"; import type { Logger } from "../../utils/logger.js"; import { calculateTotalCount } from "../../utils/result.js"; @@ -55,9 +55,20 @@ function logFeatureResult( } } -export async function generateCommand(logger: Logger, options: GenerateOptions): Promise { - const { baseDir, outputRoots, ...rest } = options; - +/** + * Resolve the effective output roots from the canonical `--output-roots` and + * the deprecated `--base-dir` alias, emitting the relevant deprecation / + * override warnings as a side effect. Returns the resolved output-root list. + */ +function resolveOutputRoots({ + logger, + baseDir, + outputRoots, +}: { + logger: Logger; + baseDir: string[] | undefined; + outputRoots: string[] | undefined; +}): string[] | undefined { // The deprecated `--base-dir` CLI flag is accepted as an alias of // `--output-roots`. Emit a deprecation warning whenever it is used so the // user sees a clear migration prompt at the call site. When both are @@ -90,6 +101,68 @@ export async function generateCommand(logger: Logger, options: GenerateOptions): ); } + return outputRootsResolved; +} + +const FEATURE_DEBUG_MESSAGES: Record = { + ignore: "Generating ignore files...", + mcp: "Generating MCP files...", + commands: "Generating command files...", + subagents: "Generating subagent files...", + skills: "Generating skill files...", + hooks: "Generating hooks...", + rules: "Generating rule files...", +}; + +// Order in which per-feature debug messages are emitted; matches the original +// sequential `if (features.includes(...))` ladder. +const FEATURE_DEBUG_ORDER = [ + "ignore", + "mcp", + "commands", + "subagents", + "skills", + "hooks", + "rules", +] as const; + +function logFeatureDebugMessages(logger: Logger, features: readonly string[]): void { + for (const feature of FEATURE_DEBUG_ORDER) { + if (features.includes(feature)) { + logger.debug(FEATURE_DEBUG_MESSAGES[feature] ?? ""); + } + } +} + +/** + * Build the human-readable per-feature summary fragments (e.g. "3 rules") for + * features that produced at least one file. Order matches the original + * sequential `if (count > 0) parts.push(...)` ladder. + */ +function buildSummaryParts(result: GenerateResult): string[] { + const summarySpecs: { count: number; label: string }[] = [ + { count: result.rulesCount, label: "rules" }, + { count: result.ignoreCount, label: "ignore files" }, + { count: result.mcpCount, label: "MCP files" }, + { count: result.commandsCount, label: "commands" }, + { count: result.subagentsCount, label: "subagents" }, + { count: result.skillsCount, label: "skills" }, + { count: result.hooksCount, label: "hooks" }, + { count: result.permissionsCount, label: "permissions" }, + ]; + + const parts: string[] = []; + for (const { count, label } of summarySpecs) { + if (count > 0) parts.push(`${count} ${label}`); + } + return parts; +} + +export async function generateCommand(logger: Logger, options: GenerateOptions): Promise { + const { baseDir, outputRoots, ...rest } = options; + + const outputRootsResolved = resolveOutputRoots({ logger, baseDir, outputRoots }); + const config = await ConfigResolver.resolve( { ...rest, outputRoots: outputRootsResolved }, { logger }, @@ -113,27 +186,7 @@ export async function generateCommand(logger: Logger, options: GenerateOptions): const features = config.getFeatures(); - if (features.includes("ignore")) { - logger.debug("Generating ignore files..."); - } - if (features.includes("mcp")) { - logger.debug("Generating MCP files..."); - } - if (features.includes("commands")) { - logger.debug("Generating command files..."); - } - if (features.includes("subagents")) { - logger.debug("Generating subagent files..."); - } - if (features.includes("skills")) { - logger.debug("Generating skill files..."); - } - if (features.includes("hooks")) { - logger.debug("Generating hooks..."); - } - if (features.includes("rules")) { - logger.debug("Generating rule files..."); - } + logFeatureDebugMessages(logger, features); const result = await generate({ config, logger }); @@ -200,15 +253,7 @@ export async function generateCommand(logger: Logger, options: GenerateOptions): return; } - const parts = []; - if (result.rulesCount > 0) parts.push(`${result.rulesCount} rules`); - if (result.ignoreCount > 0) parts.push(`${result.ignoreCount} ignore files`); - if (result.mcpCount > 0) parts.push(`${result.mcpCount} MCP files`); - if (result.commandsCount > 0) parts.push(`${result.commandsCount} commands`); - if (result.subagentsCount > 0) parts.push(`${result.subagentsCount} subagents`); - if (result.skillsCount > 0) parts.push(`${result.skillsCount} skills`); - if (result.hooksCount > 0) parts.push(`${result.hooksCount} hooks`); - if (result.permissionsCount > 0) parts.push(`${result.permissionsCount} permissions`); + const parts = buildSummaryParts(result); if (isPreview) { logger.info(`${modePrefix} Would write ${totalGenerated} file(s) total (${parts.join(" + ")})`); diff --git a/src/config/config-resolver.ts b/src/config/config-resolver.ts index 299296d7c..6e5abf2ac 100644 --- a/src/config/config-resolver.ts +++ b/src/config/config-resolver.ts @@ -143,6 +143,140 @@ const mergeConfigs = ( }; }; +/** + * Resolve a single config value honouring precedence: + * CLI option > config-file value > default. The first defined value wins. + */ +function pick({ + cli, + file, + fallback, +}: { + cli: T | undefined; + file: T | undefined; + fallback: T; +}): T { + return cli ?? file ?? fallback; +} + +/** + * Map the deprecated `baseDirs` alias onto `outputRoots`. If both are supplied, + * `outputRoots` wins; either way emit a one-shot deprecation warning so callers + * know to migrate. Returns the effective `outputRoots`. + */ +function applyDeprecatedBaseDirs({ + outputRoots, + deprecatedBaseDirs, +}: { + outputRoots: string[] | undefined; + deprecatedBaseDirs: string[] | undefined; +}): string[] | undefined { + if (deprecatedBaseDirs === undefined) { + return outputRoots; + } + emitBaseDirsConfigFieldDeprecationWarning(); + return outputRoots ?? deprecatedBaseDirs; +} + +/** + * Re-validate `targets`/`features` mutual-exclusivity after the base and local + * config files have been merged. A base file and local file can each be valid + * in isolation yet merge into an invalid `{ targets: object, features: array }` + * state, so this throws with a message naming both files. + */ +function assertMergedTargetsFeaturesExclusive({ + configByFile, + validatedConfigPath, + localConfigPath, +}: { + configByFile: PartialConfigParams; + validatedConfigPath: string; + localConfigPath: string; +}): void { + try { + assertTargetsFeaturesExclusive({ + targets: configByFile.targets, + features: configByFile.features, + }); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error( + `${detail} (detected after merging '${validatedConfigPath}' with '${localConfigPath}' — the two files combined produce the invalid combination; remove the conflicting field from one of them).`, + { cause: error }, + ); + } +} + +/** + * Resolve the effective `global` flag. When an `inputRoot` is in play the user + * is decoupling source from output, so a config-file `global: true` is dropped + * (unless the caller also explicitly passes `global`); a warning is emitted in + * that case. Returns the resolved boolean `global`. + */ +function resolveGlobal({ + logger, + resolvedInputRoot, + global, + configByFile, + validatedConfigPath, +}: { + logger: Logger | undefined; + resolvedInputRoot: string | undefined; + global: boolean | undefined; + configByFile: PartialConfigParams; + validatedConfigPath: string; +}): boolean { + if (resolvedInputRoot !== undefined && global === undefined && configByFile.global === true) { + warnWithFallback( + logger, + `Ignoring "global: true" from ${JSON.stringify(validatedConfigPath)} because ` + + `an inputRoot was configured; pass global=true (CLI: --global) to keep ` + + `user-scope output. Output will be project-scope (global=false).`, + ); + } + const configGlobal = resolvedInputRoot !== undefined ? false : configByFile.global; + return pick({ cli: global, file: configGlobal, fallback: getDefaults().global }); +} + +/** + * Resolve `features`/`targets` while honouring the strict mutual-exclusivity + * rule enforced by `assertTargetsFeaturesExclusive`: + * + * - When the user provides `targets` in object form, `features` must stay + * undefined (the per-target feature config lives inside the `targets` + * object); skip the `features` default. + * - When the user provides `features` in object form without `targets`, leave + * `targets` undefined so `Config.getTargets` can derive the target list from + * the `features` object keys; skip the `targets` default. + * - Otherwise fall through to the array-form defaults. + */ +function resolveFeaturesAndTargets({ + features, + targets, + configByFile, +}: { + features: ConfigResolverResolveParams["features"]; + targets: ConfigResolverResolveParams["targets"]; + configByFile: PartialConfigParams; +}): { + resolvedFeatures: ConfigParams["features"]; + resolvedTargets: ConfigParams["targets"]; +} { + const userProvidedFeatures = features ?? configByFile.features; + const userProvidedTargets = targets ?? configByFile.targets; + const targetsIsObject = userProvidedTargets !== undefined && !Array.isArray(userProvidedTargets); + const featuresIsObject = + userProvidedFeatures !== undefined && !Array.isArray(userProvidedFeatures); + if (featuresIsObject) { + emitFeaturesObjectFormDeprecationWarning(); + } + const resolvedFeatures = + userProvidedFeatures ?? (targetsIsObject ? undefined : getDefaults().features); + const resolvedTargets = + userProvidedTargets ?? (featuresIsObject ? undefined : getDefaults().targets); + return { resolvedFeatures, resolvedTargets }; +} + // oxlint-disable-next-line no-extraneous-class export class ConfigResolver { public static async resolve( @@ -170,12 +304,7 @@ export class ConfigResolver { // Map the deprecated programmatic `baseDirs` alias to `outputRoots`. // If both are supplied, `outputRoots` wins; either way emit a one-shot // deprecation warning so callers know to migrate. - if (deprecatedBaseDirs !== undefined) { - emitBaseDirsConfigFieldDeprecationWarning(); - if (outputRoots === undefined) { - outputRoots = deprecatedBaseDirs; - } - } + outputRoots = applyDeprecatedBaseDirs({ outputRoots, deprecatedBaseDirs }); // Capture cwd once at the entry point so the resolved config is // deterministic and independent of any later `process.chdir()` calls. const cwd = resolve(process.cwd()); @@ -219,18 +348,7 @@ export class ConfigResolver { // merge into an invalid `{ targets: object, features: array }` state. // Re-check after the merge and throw with a message that names both files // so the user knows where to look. - try { - assertTargetsFeaturesExclusive({ - targets: configByFile.targets, - features: configByFile.features, - }); - } catch (error) { - const detail = error instanceof Error ? error.message : String(error); - throw new Error( - `${detail} (detected after merging '${validatedConfigPath}' with '${localConfigPath}' — the two files combined produce the invalid combination; remove the conflicting field from one of them).`, - { cause: error }, - ); - } + assertMergedTargetsFeaturesExclusive({ configByFile, validatedConfigPath, localConfigPath }); // When `inputRoot` is set (from CLI, programmatic args, or a config file) // the user is decoupling source from output, so "global: true" from the @@ -243,74 +361,62 @@ export class ConfigResolver { // `configByFile.inputRoot`, the symmetric warning still fires so a user // moving from CLI flag to config-file form sees consistent behavior. const resolvedInputRoot = inputRoot ?? configByFile.inputRoot; - if (resolvedInputRoot !== undefined && global === undefined && configByFile.global === true) { - warnWithFallback( - logger, - `Ignoring "global: true" from ${JSON.stringify(validatedConfigPath)} because ` + - `an inputRoot was configured; pass global=true (CLI: --global) to keep ` + - `user-scope output. Output will be project-scope (global=false).`, - ); - } - const configGlobal = resolvedInputRoot !== undefined ? false : configByFile.global; - const resolvedGlobal = global ?? configGlobal ?? getDefaults().global; - const resolvedSimulateCommands = - simulateCommands ?? configByFile.simulateCommands ?? getDefaults().simulateCommands; - const resolvedSimulateSubagents = - simulateSubagents ?? configByFile.simulateSubagents ?? getDefaults().simulateSubagents; - - const resolvedSimulateSkills = - simulateSkills ?? configByFile.simulateSkills ?? getDefaults().simulateSkills; - const resolvedGitignoreTargetsOnly = - gitignoreTargetsOnly ?? - configByFile.gitignoreTargetsOnly ?? - getDefaults().gitignoreTargetsOnly; + const resolvedGlobal = resolveGlobal({ + logger, + resolvedInputRoot, + global, + configByFile, + validatedConfigPath, + }); - // Resolve features/targets while honouring the strict mutual-exclusivity - // rule enforced by `assertTargetsFeaturesExclusive`: - // - // - When the user provides `targets` in object form, `features` must - // stay undefined (the per-target feature config lives inside the - // `targets` object itself); skip the `features` default. - // - When the user provides `features` in object form without `targets`, - // leave `targets` undefined so `Config.getTargets` can derive the - // target list from the `features` object keys; skip the `targets` - // default. - // - Otherwise fall through to the array-form defaults. - const userProvidedFeatures = features ?? configByFile.features; - const userProvidedTargets = targets ?? configByFile.targets; - const targetsIsObject = - userProvidedTargets !== undefined && !Array.isArray(userProvidedTargets); - const featuresIsObject = - userProvidedFeatures !== undefined && !Array.isArray(userProvidedFeatures); - if (featuresIsObject) { - emitFeaturesObjectFormDeprecationWarning(); - } - const resolvedFeatures = - userProvidedFeatures ?? (targetsIsObject ? undefined : getDefaults().features); - const resolvedTargets = - userProvidedTargets ?? (featuresIsObject ? undefined : getDefaults().targets); + const { resolvedFeatures, resolvedTargets } = resolveFeaturesAndTargets({ + features, + targets, + configByFile, + }); const configParams = { targets: resolvedTargets, features: resolvedFeatures, - verbose: verbose ?? configByFile.verbose ?? getDefaults().verbose, - delete: isDelete ?? configByFile.delete ?? getDefaults().delete, + verbose: pick({ cli: verbose, file: configByFile.verbose, fallback: getDefaults().verbose }), + delete: pick({ cli: isDelete, file: configByFile.delete, fallback: getDefaults().delete }), outputRoots: getOutputRootsInLightOfGlobal({ - outputRoots: outputRoots ?? configByFile.outputRoots ?? getDefaults().outputRoots, + outputRoots: pick({ + cli: outputRoots, + file: configByFile.outputRoots, + fallback: getDefaults().outputRoots, + }), global: resolvedGlobal, }), global: resolvedGlobal, - silent: silent ?? configByFile.silent ?? getDefaults().silent, - simulateCommands: resolvedSimulateCommands, - simulateSubagents: resolvedSimulateSubagents, - simulateSkills: resolvedSimulateSkills, - gitignoreTargetsOnly: resolvedGitignoreTargetsOnly, - gitignoreDestination: - gitignoreDestination ?? - configByFile.gitignoreDestination ?? - getDefaults().gitignoreDestination, - dryRun: dryRun ?? configByFile.dryRun ?? getDefaults().dryRun, - check: check ?? configByFile.check ?? getDefaults().check, + silent: pick({ cli: silent, file: configByFile.silent, fallback: getDefaults().silent }), + simulateCommands: pick({ + cli: simulateCommands, + file: configByFile.simulateCommands, + fallback: getDefaults().simulateCommands, + }), + simulateSubagents: pick({ + cli: simulateSubagents, + file: configByFile.simulateSubagents, + fallback: getDefaults().simulateSubagents, + }), + simulateSkills: pick({ + cli: simulateSkills, + file: configByFile.simulateSkills, + fallback: getDefaults().simulateSkills, + }), + gitignoreTargetsOnly: pick({ + cli: gitignoreTargetsOnly, + file: configByFile.gitignoreTargetsOnly, + fallback: getDefaults().gitignoreTargetsOnly, + }), + gitignoreDestination: pick({ + cli: gitignoreDestination, + file: configByFile.gitignoreDestination, + fallback: getDefaults().gitignoreDestination, + }), + dryRun: pick({ cli: dryRun, file: configByFile.dryRun, fallback: getDefaults().dryRun }), + check: pick({ cli: check, file: configByFile.check, fallback: getDefaults().check }), // Pass the fully-resolved absolute inputRoot so `Config.getInputRoot()` // is pure and never re-reads `process.cwd()` after construction. When // neither CLI nor config file supplied an inputRoot, fall back to the diff --git a/src/features/hooks/copilotcli-hooks.ts b/src/features/hooks/copilotcli-hooks.ts index 7aef47e82..f28e73ce0 100644 --- a/src/features/hooks/copilotcli-hooks.ts +++ b/src/features/hooks/copilotcli-hooks.ts @@ -108,6 +108,114 @@ const CopilotCliHookEntrySchema = z.looseObject({ type CopilotCliHookEntry = z.infer; +/** Filter the shared config hooks down to events the Copilot CLI supports. */ +function filterSupportedCopilotCliHooks(hooks: HooksConfig["hooks"]): HooksConfig["hooks"] { + const supported: Set = new Set(COPILOTCLI_HOOK_EVENTS); + const sharedConfigHooks: HooksConfig["hooks"] = {}; + for (const [event, defs] of Object.entries(hooks)) { + if (supported.has(event)) { + sharedConfigHooks[event] = defs; + } + } + return sharedConfigHooks; +} + +/** + * Resolve the `matcher` part for an exported entry. Copilot CLI honors `matcher` + * only on preToolUse/postToolUse entries; on any other event a matcher would be + * silently dropped by the CLI, so we drop it here with a warning rather than + * emitting a dead field. + */ +function resolveExportMatcherPart({ + matcher, + matcherSupported, + eventName, + logger, +}: { + matcher: string | null | undefined; + matcherSupported: boolean; + eventName: string; + logger?: Logger; +}): { matcher?: string } { + if (matcher === undefined || matcher === null || matcher === "") { + return {}; + } + if (matcherSupported) { + return { matcher }; + } + logger?.warn( + `Copilot CLI hook matchers are only honored on preToolUse/postToolUse; dropping matcher "${matcher}" on '${eventName}'.`, + ); + return {}; +} + +/** + * Build the exported entries for a single canonical event. Returns an empty + * array when no entries are emitted (e.g. all prompt hooks were skipped). + */ +function buildCopilotCliEntriesForEvent({ + eventName, + definitions, + canonicalSchemaKeys, + commandField, + logger, +}: { + eventName: string; + definitions: HooksConfig["hooks"][string]; + canonicalSchemaKeys: string[]; + commandField: "bash" | "powershell"; + logger?: Logger; +}): CopilotCliHookEntry[] { + const matcherSupported = COPILOTCLI_MATCHER_EVENTS.has(eventName); + const entries: CopilotCliHookEntry[] = []; + for (const def of definitions) { + const hookType = def.type ?? "command"; + const timeout = def.timeout; + const timeoutPart = timeout !== undefined && timeout !== null ? { timeoutSec: timeout } : {}; + const matcherPart = resolveExportMatcherPart({ + matcher: def.matcher, + matcherSupported, + eventName, + logger, + }); + // Non-canonical fields (cwd, env, url, headers, allowedEnvVars, ...) pass + // through verbatim. + const rest = Object.fromEntries( + Object.entries(def).filter(([k]) => !canonicalSchemaKeys.includes(k)), + ); + + if (hookType === "prompt") { + // Copilot CLI only honors prompt hooks on sessionStart. + if (eventName !== "sessionStart") { + logger?.warn( + `Copilot CLI prompt hooks are only supported on sessionStart; skipping a prompt hook on '${eventName}'.`, + ); + continue; + } + if (def.prompt === undefined || def.prompt === null) continue; + entries.push({ type: "prompt", prompt: def.prompt, ...rest }); + } else if (hookType === "http") { + entries.push({ + type: "http", + ...matcherPart, + ...(def.url !== undefined && def.url !== null && { url: def.url }), + ...timeoutPart, + ...rest, + }); + } else { + const command = def.command; + entries.push({ + type: "command", + ...matcherPart, + ...(command !== undefined && command !== null && { [commandField]: command }), + ...timeoutPart, + ...rest, + }); + } + } + return entries; +} + function canonicalToCopilotCliHooks( config: HooksConfig, logger?: Logger, @@ -115,78 +223,24 @@ function canonicalToCopilotCliHooks( const canonicalSchemaKeys = Object.keys(HookDefinitionSchema.shape); const isWindows = process.platform === "win32"; const commandField = isWindows ? "powershell" : "bash"; - const supported: Set = new Set(COPILOTCLI_HOOK_EVENTS); - const sharedConfigHooks: HooksConfig["hooks"] = {}; - for (const [event, defs] of Object.entries(config.hooks)) { - if (supported.has(event)) { - sharedConfigHooks[event] = defs; - } - } // `copilotcli` falls back to the shared `copilot.hooks` override key when no // `copilotcli.hooks` block is present, then lets `copilotcli.hooks` win on // conflicts. const effectiveHooks: HooksConfig["hooks"] = { - ...sharedConfigHooks, + ...filterSupportedCopilotCliHooks(config.hooks), ...config.copilot?.hooks, ...config.copilotcli?.hooks, }; const out: Record = {}; for (const [eventName, definitions] of Object.entries(effectiveHooks)) { const copilotEventName = CANONICAL_TO_COPILOTCLI_EVENT_NAMES[eventName] ?? eventName; - const matcherSupported = COPILOTCLI_MATCHER_EVENTS.has(eventName); - const entries: CopilotCliHookEntry[] = []; - for (const def of definitions) { - const hookType = def.type ?? "command"; - const timeout = def.timeout; - const timeoutPart = timeout !== undefined && timeout !== null ? { timeoutSec: timeout } : {}; - // Copilot CLI honors `matcher` only on preToolUse/postToolUse entries; on - // any other event a matcher would be silently dropped by the CLI, so we - // drop it here with a warning rather than emitting a dead field. - let matcherPart: { matcher?: string } = {}; - if (def.matcher !== undefined && def.matcher !== null && def.matcher !== "") { - if (matcherSupported) { - matcherPart = { matcher: def.matcher }; - } else { - logger?.warn( - `Copilot CLI hook matchers are only honored on preToolUse/postToolUse; dropping matcher "${def.matcher}" on '${eventName}'.`, - ); - } - } - // Non-canonical fields (cwd, env, url, headers, allowedEnvVars, ...) pass - // through verbatim. - const rest = Object.fromEntries( - Object.entries(def).filter(([k]) => !canonicalSchemaKeys.includes(k)), - ); - - if (hookType === "prompt") { - // Copilot CLI only honors prompt hooks on sessionStart. - if (eventName !== "sessionStart") { - logger?.warn( - `Copilot CLI prompt hooks are only supported on sessionStart; skipping a prompt hook on '${eventName}'.`, - ); - continue; - } - if (def.prompt === undefined || def.prompt === null) continue; - entries.push({ type: "prompt", prompt: def.prompt, ...rest }); - } else if (hookType === "http") { - entries.push({ - type: "http", - ...matcherPart, - ...(def.url !== undefined && def.url !== null && { url: def.url }), - ...timeoutPart, - ...rest, - }); - } else { - const command = def.command; - entries.push({ - type: "command", - ...matcherPart, - ...(command !== undefined && command !== null && { [commandField]: command }), - ...timeoutPart, - ...rest, - }); - } - } + const entries = buildCopilotCliEntriesForEvent({ + eventName, + definitions, + canonicalSchemaKeys, + commandField, + logger, + }); if (entries.length > 0) { out[copilotEventName] = entries; } diff --git a/src/features/hooks/geminicli-hooks.ts b/src/features/hooks/geminicli-hooks.ts index 0dc86116a..c81465d14 100644 --- a/src/features/hooks/geminicli-hooks.ts +++ b/src/features/hooks/geminicli-hooks.ts @@ -102,6 +102,35 @@ const GeminiMatcherEntrySchema = z.looseObject({ hooks: z.optional(z.array(GeminiHookEntrySchema)), }); +/** + * Convert a single parsed Gemini CLI matcher group into canonical hook definitions. + */ +function geminiMatcherEntryToCanonical( + entry: z.infer, +): HooksConfig["hooks"][string] { + const defs: HooksConfig["hooks"][string] = []; + const hooks = entry.hooks ?? []; + for (const h of hooks) { + const cmd = h.command; + const command = + typeof cmd === "string" && cmd.startsWith("$GEMINI_PROJECT_DIR/") + ? cmd.replace(/^\$GEMINI_PROJECT_DIR\/?/, "./") + : cmd; + const hookType = h.type === "command" || h.type === "prompt" ? h.type : "command"; + defs.push({ + type: hookType, + ...(command !== undefined && command !== null && { command }), + ...(h.timeout !== undefined && h.timeout !== null && { timeout: h.timeout }), + ...(h.name !== undefined && h.name !== null && { name: h.name }), + ...(h.description !== undefined && h.description !== null && { description: h.description }), + ...(entry.matcher !== undefined && + entry.matcher !== null && + entry.matcher !== "" && { matcher: entry.matcher }), + }); + } + return defs; +} + /** * Extract hooks from Gemini CLI settings.json into canonical format. */ @@ -117,27 +146,7 @@ function geminiHooksToCanonical(geminiHooks: unknown): HooksConfig["hooks"] { for (const rawEntry of matcherEntries) { const parseResult = GeminiMatcherEntrySchema.safeParse(rawEntry); if (!parseResult.success) continue; - const entry = parseResult.data; - const hooks = entry.hooks ?? []; - for (const h of hooks) { - const cmd = h.command; - const command = - typeof cmd === "string" && cmd.startsWith("$GEMINI_PROJECT_DIR/") - ? cmd.replace(/^\$GEMINI_PROJECT_DIR\/?/, "./") - : cmd; - const hookType = h.type === "command" || h.type === "prompt" ? h.type : "command"; - defs.push({ - type: hookType, - ...(command !== undefined && command !== null && { command }), - ...(h.timeout !== undefined && h.timeout !== null && { timeout: h.timeout }), - ...(h.name !== undefined && h.name !== null && { name: h.name }), - ...(h.description !== undefined && - h.description !== null && { description: h.description }), - ...(entry.matcher !== undefined && - entry.matcher !== null && - entry.matcher !== "" && { matcher: entry.matcher }), - }); - } + defs.push(...geminiMatcherEntryToCanonical(parseResult.data)); } if (defs.length > 0) { canonical[eventName] = defs; diff --git a/src/features/hooks/kiro-hooks.ts b/src/features/hooks/kiro-hooks.ts index bde021b46..2f9b4a55e 100644 --- a/src/features/hooks/kiro-hooks.ts +++ b/src/features/hooks/kiro-hooks.ts @@ -28,6 +28,27 @@ import { * Filters shared hooks to KIRO_HOOK_EVENTS, merges config.kiro?.hooks, * then maps event names and emits Kiro CLI hook arrays. */ +/** Build the Kiro CLI hook entries for a single canonical event's definitions. */ +function buildKiroEntriesForEvent(definitions: HooksConfig["hooks"][string]): unknown[] { + const entries: unknown[] = []; + for (const def of definitions) { + if ((def.type ?? "command") !== "command") continue; + entries.push({ + command: def.command, + ...(def.matcher !== undefined && + def.matcher !== null && + def.matcher !== "" && { matcher: def.matcher }), + ...(def.timeout !== undefined && + def.timeout !== null && + def.timeout > 0 && { timeout_ms: def.timeout }), + ...(def.name !== undefined && def.name !== null && { name: def.name }), + ...(def.description !== undefined && + def.description !== null && { description: def.description }), + }); + } + return entries; +} + function canonicalToKiroHooks( config: HooksConfig, overrideKey: "kiro" | "kiro-cli" = "kiro", @@ -51,22 +72,7 @@ function canonicalToKiroHooks( const kiro: Record = {}; for (const [eventName, definitions] of Object.entries(effectiveHooks)) { const kiroEventName = CANONICAL_TO_KIRO_EVENT_NAMES[eventName] ?? eventName; - const entries: unknown[] = []; - for (const def of definitions) { - if ((def.type ?? "command") !== "command") continue; - entries.push({ - command: def.command, - ...(def.matcher !== undefined && - def.matcher !== null && - def.matcher !== "" && { matcher: def.matcher }), - ...(def.timeout !== undefined && - def.timeout !== null && - def.timeout > 0 && { timeout_ms: def.timeout }), - ...(def.name !== undefined && def.name !== null && { name: def.name }), - ...(def.description !== undefined && - def.description !== null && { description: def.description }), - }); - } + const entries = buildKiroEntriesForEvent(definitions); if (entries.length > 0) { if (kiro[kiroEventName]) { kiro[kiroEventName].push(...entries); diff --git a/src/features/hooks/opencode-style-generator.ts b/src/features/hooks/opencode-style-generator.ts index e3f0c84ed..14b307b99 100644 --- a/src/features/hooks/opencode-style-generator.ts +++ b/src/features/hooks/opencode-style-generator.ts @@ -25,31 +25,21 @@ function validateAndSanitizeMatcher(matcher: string): string { type Handler = { command: string; matcher?: string }; type HandlerGroup = Record; -export function generateOpencodeStylePluginCode( - config: HooksConfig, - supportedEvents: readonly string[], - toolConfigKey: "kilo" | "opencode", - eventMap: Record, - // Export shape of the generated plugin module: - // - "named" (default): `export const RulesyncHooksPlugin = async ({ $ }) => {...}` - // — the OpenCode convention. - // - "default": `export default { id: "rulesync-hooks", server: async ({ $ }) => {...} }` - // — Kilo's canonical `{ id, server }` module descriptor. Kilo marks named - // exports as legacy, so the Kilo target emits this form. - // https://kilo.ai/docs/automate/extending/plugins - exportStyle: "named" | "default" = "named", -): string { - const supported: Set = new Set(supportedEvents); - const configHooks = { ...config.hooks, ...config[toolConfigKey]?.hooks }; - const effectiveHooks: HooksConfig["hooks"] = {}; - - for (const [event, defs] of Object.entries(configHooks)) { - if (supported.has(event)) effectiveHooks[event] = defs; - } - - const namedEventHandlers: HandlerGroup = {}; - const genericEventHandlers: HandlerGroup = {}; - +/** + * Group the effective hooks into named (tool.execute.*) and generic event + * handler groups, keyed by tool event name. Mutates the supplied groups. + */ +function collectOpencodeStyleHandlers({ + effectiveHooks, + eventMap, + namedEventHandlers, + genericEventHandlers, +}: { + effectiveHooks: HooksConfig["hooks"]; + eventMap: Record; + namedEventHandlers: HandlerGroup; + genericEventHandlers: HandlerGroup; +}): void { for (const [canonicalEvent, definitions] of Object.entries(effectiveHooks)) { const toolEvent = eventMap[canonicalEvent]; if (!toolEvent) continue; @@ -74,28 +64,32 @@ export function generateOpencodeStylePluginCode( } } } +} - // Build the handler entries (the contents of the returned object) once with a - // base indentation, then wrap them in the requested export shape. The default - // (Kilo) export nests the function one level deeper, so its body is re-indented - // by an extra two spaces relative to the named (OpenCode) export. +/** Emit the `event: async ({ event }) => {...}` block for generic handlers. */ +function buildGenericEventBodyLines(genericEventHandlers: HandlerGroup): string[] { const bodyLines: string[] = []; - - if (Object.keys(genericEventHandlers).length > 0) { - bodyLines.push(" event: async ({ event }) => {"); - let isFirst = true; - for (const [eventName, handlers] of Object.entries(genericEventHandlers)) { - bodyLines.push(` ${isFirst ? "if" : "else if"} (event.type === "${eventName}") {`); - isFirst = false; - for (const handler of handlers) { - const escapedCommand = escapeForTemplateLiteral(handler.command); - bodyLines.push(` await $\`${escapedCommand}\`;`); - } - bodyLines.push(" }"); + if (Object.keys(genericEventHandlers).length === 0) { + return bodyLines; + } + bodyLines.push(" event: async ({ event }) => {"); + let isFirst = true; + for (const [eventName, handlers] of Object.entries(genericEventHandlers)) { + bodyLines.push(` ${isFirst ? "if" : "else if"} (event.type === "${eventName}") {`); + isFirst = false; + for (const handler of handlers) { + const escapedCommand = escapeForTemplateLiteral(handler.command); + bodyLines.push(` await $\`${escapedCommand}\`;`); } - bodyLines.push(" },"); + bodyLines.push(" }"); } + bodyLines.push(" },"); + return bodyLines; +} +/** Emit the named (`tool.execute.*`) handler blocks. */ +function buildNamedEventBodyLines(namedEventHandlers: HandlerGroup): string[] { + const bodyLines: string[] = []; for (const [eventName, handlers] of Object.entries(namedEventHandlers)) { bodyLines.push(` "${eventName}": async (input) => {`); for (const handler of handlers) { @@ -114,7 +108,17 @@ export function generateOpencodeStylePluginCode( } bodyLines.push(" },"); } + return bodyLines; +} +/** Wrap the handler body lines in the requested export shape. */ +function wrapInExportShape({ + bodyLines, + exportStyle, +}: { + bodyLines: string[]; + exportStyle: "named" | "default"; +}): string[] { const lines: string[] = []; if (exportStyle === "default") { lines.push("export default {"); @@ -137,6 +141,51 @@ export function generateOpencodeStylePluginCode( lines.push("};"); } lines.push(""); + return lines; +} + +export function generateOpencodeStylePluginCode( + config: HooksConfig, + supportedEvents: readonly string[], + toolConfigKey: "kilo" | "opencode", + eventMap: Record, + // Export shape of the generated plugin module: + // - "named" (default): `export const RulesyncHooksPlugin = async ({ $ }) => {...}` + // — the OpenCode convention. + // - "default": `export default { id: "rulesync-hooks", server: async ({ $ }) => {...} }` + // — Kilo's canonical `{ id, server }` module descriptor. Kilo marks named + // exports as legacy, so the Kilo target emits this form. + // https://kilo.ai/docs/automate/extending/plugins + exportStyle: "named" | "default" = "named", +): string { + const supported: Set = new Set(supportedEvents); + const configHooks = { ...config.hooks, ...config[toolConfigKey]?.hooks }; + const effectiveHooks: HooksConfig["hooks"] = {}; + + for (const [event, defs] of Object.entries(configHooks)) { + if (supported.has(event)) effectiveHooks[event] = defs; + } + + const namedEventHandlers: HandlerGroup = {}; + const genericEventHandlers: HandlerGroup = {}; + + collectOpencodeStyleHandlers({ + effectiveHooks, + eventMap, + namedEventHandlers, + genericEventHandlers, + }); + + // Build the handler entries (the contents of the returned object) once with a + // base indentation, then wrap them in the requested export shape. The default + // (Kilo) export nests the function one level deeper, so its body is re-indented + // by an extra two spaces relative to the named (OpenCode) export. + const bodyLines: string[] = [ + ...buildGenericEventBodyLines(genericEventHandlers), + ...buildNamedEventBodyLines(namedEventHandlers), + ]; + + const lines = wrapInExportShape({ bodyLines, exportStyle }); return lines.join("\n"); } diff --git a/src/features/hooks/qwencode-hooks.ts b/src/features/hooks/qwencode-hooks.ts index 02081c806..46dc34040 100644 --- a/src/features/hooks/qwencode-hooks.ts +++ b/src/features/hooks/qwencode-hooks.ts @@ -107,6 +107,37 @@ const QwencodeMatcherEntrySchema = z.looseObject({ sequential: z.optional(z.boolean()), }); +/** + * Convert a single parsed Qwen Code matcher group into canonical hook definitions. + */ +function qwencodeMatcherEntryToCanonical( + entry: z.infer, +): HooksConfig["hooks"][string] { + const defs: HooksConfig["hooks"][string] = []; + const hooks = entry.hooks ?? []; + const sequential = entry.sequential === true; + for (const h of hooks) { + const command = h.command; + // Preserve the `http` transport (and its target URL) instead of + // collapsing every non-prompt hook to `command`. + const hookType = + h.type === "command" || h.type === "prompt" || h.type === "http" ? h.type : "command"; + defs.push({ + type: hookType, + ...(command !== undefined && command !== null && { command }), + ...(h.url !== undefined && h.url !== null && { url: h.url }), + ...(h.timeout !== undefined && h.timeout !== null && { timeout: h.timeout }), + ...(h.name !== undefined && h.name !== null && { name: h.name }), + ...(h.description !== undefined && h.description !== null && { description: h.description }), + ...(sequential && { sequential: true }), + ...(entry.matcher !== undefined && + entry.matcher !== null && + entry.matcher !== "" && { matcher: entry.matcher }), + }); + } + return defs; +} + /** * Extract hooks from Qwen Code settings.json into canonical format. */ @@ -122,29 +153,7 @@ function qwencodeHooksToCanonical(qwencodeHooks: unknown): HooksConfig["hooks"] for (const rawEntry of matcherEntries) { const parseResult = QwencodeMatcherEntrySchema.safeParse(rawEntry); if (!parseResult.success) continue; - const entry = parseResult.data; - const hooks = entry.hooks ?? []; - const sequential = entry.sequential === true; - for (const h of hooks) { - const command = h.command; - // Preserve the `http` transport (and its target URL) instead of - // collapsing every non-prompt hook to `command`. - const hookType = - h.type === "command" || h.type === "prompt" || h.type === "http" ? h.type : "command"; - defs.push({ - type: hookType, - ...(command !== undefined && command !== null && { command }), - ...(h.url !== undefined && h.url !== null && { url: h.url }), - ...(h.timeout !== undefined && h.timeout !== null && { timeout: h.timeout }), - ...(h.name !== undefined && h.name !== null && { name: h.name }), - ...(h.description !== undefined && - h.description !== null && { description: h.description }), - ...(sequential && { sequential: true }), - ...(entry.matcher !== undefined && - entry.matcher !== null && - entry.matcher !== "" && { matcher: entry.matcher }), - }); - } + defs.push(...qwencodeMatcherEntryToCanonical(parseResult.data)); } if (defs.length > 0) { canonical[eventName] = defs; diff --git a/src/features/hooks/tool-hooks-converter.ts b/src/features/hooks/tool-hooks-converter.ts index 543aa94a3..616fe8e26 100644 --- a/src/features/hooks/tool-hooks-converter.ts +++ b/src/features/hooks/tool-hooks-converter.ts @@ -38,6 +38,105 @@ export type ToolHooksConverterConfig = { noMatcherEvents?: ReadonlySet; }; +/** + * Filter the shared canonical hooks to the supported events and merge tool overrides on top. + */ +function buildEffectiveHooks({ + config, + toolOverrideHooks, + supportedEvents, +}: { + config: HooksConfig; + toolOverrideHooks: HooksConfig["hooks"] | undefined; + supportedEvents: readonly HookEvent[]; +}): HooksConfig["hooks"] { + const supported: Set = new Set(supportedEvents); + const sharedHooks: HooksConfig["hooks"] = {}; + for (const [event, defs] of Object.entries(config.hooks)) { + if (supported.has(event)) { + sharedHooks[event] = defs; + } + } + return { + ...sharedHooks, + ...toolOverrideHooks, + }; +} + +/** + * Group a list of hook definitions by their `matcher` (empty string when absent), + * preserving insertion order of both keys and grouped definitions. + */ +function groupDefinitionsByMatcher( + definitions: HooksConfig["hooks"][string], +): Map { + const byMatcher = new Map(); + for (const def of definitions) { + const key = def.matcher ?? ""; + const list = byMatcher.get(key); + if (list) list.push(def); + else byMatcher.set(key, [def]); + } + return byMatcher; +} + +/** + * Apply the optional project directory variable prefix to a command string. + */ +function applyCommandPrefix({ + def, + converterConfig, +}: { + def: HooksConfig["hooks"][string][number]; + converterConfig: ToolHooksConverterConfig; +}): unknown { + const commandText = def.command; + const trimmedCommand = typeof commandText === "string" ? commandText.trimStart() : undefined; + const shouldPrefix = + converterConfig.projectDirVar !== "" && + typeof trimmedCommand === "string" && + !trimmedCommand.startsWith("$") && + (!converterConfig.prefixDotRelativeCommandsOnly || trimmedCommand.startsWith(".")); + + return shouldPrefix && typeof trimmedCommand === "string" + ? `${converterConfig.projectDirVar}/${trimmedCommand.replace(/^\.\//, "")}` + : def.command; +} + +/** + * Convert the definitions of a single matcher group into tool hook entries, + * honoring supported hook types and passthrough fields. + */ +function buildToolHooks({ + defs, + converterConfig, +}: { + defs: HooksConfig["hooks"][string]; + converterConfig: ToolHooksConverterConfig; +}): Array> { + const hooks: Array> = []; + for (const def of defs) { + const hookType = def.type ?? "command"; + if (converterConfig.supportedHookTypes && !converterConfig.supportedHookTypes.has(hookType)) { + continue; + } + const command = applyCommandPrefix({ def, converterConfig }); + hooks.push({ + type: hookType, + ...(command !== undefined && command !== null && { command }), + ...(def.timeout !== undefined && def.timeout !== null && { timeout: def.timeout }), + ...(def.prompt !== undefined && def.prompt !== null && { prompt: def.prompt }), + ...(converterConfig.passthroughFields?.includes("name") && + def.name !== undefined && + def.name !== null && { name: def.name }), + ...(converterConfig.passthroughFields?.includes("description") && + def.description !== undefined && + def.description !== null && { description: def.description }), + }); + } + return hooks; +} + /** * Convert canonical hooks config to tool-specific format (shared by Claude and Factory Droid). * Uses explicit event name mapping tables rather than algorithmic case conversion, @@ -55,27 +154,15 @@ export function canonicalToToolHooks({ converterConfig: ToolHooksConverterConfig; logger?: Logger; }): Record { - const supported: Set = new Set(converterConfig.supportedEvents); - const sharedHooks: HooksConfig["hooks"] = {}; - for (const [event, defs] of Object.entries(config.hooks)) { - if (supported.has(event)) { - sharedHooks[event] = defs; - } - } - const effectiveHooks: HooksConfig["hooks"] = { - ...sharedHooks, - ...toolOverrideHooks, - }; + const effectiveHooks = buildEffectiveHooks({ + config, + toolOverrideHooks, + supportedEvents: converterConfig.supportedEvents, + }); const result: Record = {}; for (const [eventName, definitions] of Object.entries(effectiveHooks)) { const toolEventName = converterConfig.canonicalToToolEventNames[eventName] ?? eventName; - const byMatcher = new Map(); - for (const def of definitions) { - const key = def.matcher ?? ""; - const list = byMatcher.get(key); - if (list) list.push(def); - else byMatcher.set(key, [def]); - } + const byMatcher = groupDefinitionsByMatcher(definitions); const entries: unknown[] = []; const isNoMatcherEvent = converterConfig.noMatcherEvents?.has(eventName) ?? false; for (const [matcherKey, defs] of byMatcher) { @@ -84,41 +171,7 @@ export function canonicalToToolHooks({ `matcher "${matcherKey}" on "${eventName}" hook will be ignored — this event does not support matchers`, ); } - const hooks: Array> = []; - for (const def of defs) { - const hookType = def.type ?? "command"; - if ( - converterConfig.supportedHookTypes && - !converterConfig.supportedHookTypes.has(hookType) - ) { - continue; - } - const commandText = def.command; - const trimmedCommand = - typeof commandText === "string" ? commandText.trimStart() : undefined; - const shouldPrefix = - converterConfig.projectDirVar !== "" && - typeof trimmedCommand === "string" && - !trimmedCommand.startsWith("$") && - (!converterConfig.prefixDotRelativeCommandsOnly || trimmedCommand.startsWith(".")); - - const command = - shouldPrefix && typeof trimmedCommand === "string" - ? `${converterConfig.projectDirVar}/${trimmedCommand.replace(/^\.\//, "")}` - : def.command; - hooks.push({ - type: hookType, - ...(command !== undefined && command !== null && { command }), - ...(def.timeout !== undefined && def.timeout !== null && { timeout: def.timeout }), - ...(def.prompt !== undefined && def.prompt !== null && { prompt: def.prompt }), - ...(converterConfig.passthroughFields?.includes("name") && - def.name !== undefined && - def.name !== null && { name: def.name }), - ...(converterConfig.passthroughFields?.includes("description") && - def.description !== undefined && - def.description !== null && { description: def.description }), - }); - } + const hooks = buildToolHooks({ defs, converterConfig }); if (hooks.length === 0) { continue; } @@ -141,6 +194,76 @@ export function canonicalToToolHooks({ * includes a matcher on such an event, it will be preserved in canonical format but dropped * on the next export (with a warning). */ +/** + * Strip the project directory variable prefix from a tool command string, + * converting it back to a `./`-relative command. + */ +function stripCommandPrefix({ + command, + converterConfig, +}: { + command: unknown; + converterConfig: ToolHooksConverterConfig; +}): string | undefined { + const cmd = typeof command === "string" ? command : undefined; + if ( + converterConfig.projectDirVar !== "" && + typeof cmd === "string" && + cmd.includes(`${converterConfig.projectDirVar}/`) + ) { + return cmd.replace( + new RegExp(`^${converterConfig.projectDirVar.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\/?`), + "./", + ); + } + return cmd; +} + +/** + * Convert a single tool hook record into a canonical hook definition. + */ +function toolHookToCanonical({ + h, + rawEntry, + converterConfig, +}: { + h: Record; + rawEntry: ToolMatcherEntry; + converterConfig: ToolHooksConverterConfig; +}): HooksConfig["hooks"][string][number] { + const command = stripCommandPrefix({ command: h.command, converterConfig }); + const hookType = h.type === "command" || h.type === "prompt" ? h.type : "command"; + const timeout = typeof h.timeout === "number" ? h.timeout : undefined; + const prompt = typeof h.prompt === "string" ? h.prompt : undefined; + return { + type: hookType, + ...(command !== undefined && command !== null && { command }), + ...(timeout !== undefined && timeout !== null && { timeout }), + ...(prompt !== undefined && prompt !== null && { prompt }), + ...(converterConfig.passthroughFields?.includes("name") && + typeof h.name === "string" && { name: h.name }), + ...(converterConfig.passthroughFields?.includes("description") && + typeof h.description === "string" && { description: h.description }), + ...(rawEntry.matcher !== undefined && + rawEntry.matcher !== null && + rawEntry.matcher !== "" && { matcher: rawEntry.matcher }), + }; +} + +/** + * Convert a single tool matcher entry into canonical hook definitions. + */ +function toolMatcherEntryToCanonical({ + rawEntry, + converterConfig, +}: { + rawEntry: ToolMatcherEntry; + converterConfig: ToolHooksConverterConfig; +}): HooksConfig["hooks"][string] { + const hookDefs = rawEntry.hooks ?? []; + return hookDefs.map((h) => toolHookToCanonical({ h, rawEntry, converterConfig })); +} + export function toolHooksToCanonical({ hooks, converterConfig, @@ -158,37 +281,7 @@ export function toolHooksToCanonical({ const defs: HooksConfig["hooks"][string] = []; for (const rawEntry of matcherEntries) { if (!isToolMatcherEntry(rawEntry)) continue; - const hookDefs = rawEntry.hooks ?? []; - for (const h of hookDefs) { - const cmd = typeof h.command === "string" ? h.command : undefined; - const command = - converterConfig.projectDirVar !== "" && - typeof cmd === "string" && - cmd.includes(`${converterConfig.projectDirVar}/`) - ? cmd.replace( - new RegExp( - `^${converterConfig.projectDirVar.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\/?`, - ), - "./", - ) - : cmd; - const hookType = h.type === "command" || h.type === "prompt" ? h.type : "command"; - const timeout = typeof h.timeout === "number" ? h.timeout : undefined; - const prompt = typeof h.prompt === "string" ? h.prompt : undefined; - defs.push({ - type: hookType, - ...(command !== undefined && command !== null && { command }), - ...(timeout !== undefined && timeout !== null && { timeout }), - ...(prompt !== undefined && prompt !== null && { prompt }), - ...(converterConfig.passthroughFields?.includes("name") && - typeof h.name === "string" && { name: h.name }), - ...(converterConfig.passthroughFields?.includes("description") && - typeof h.description === "string" && { description: h.description }), - ...(rawEntry.matcher !== undefined && - rawEntry.matcher !== null && - rawEntry.matcher !== "" && { matcher: rawEntry.matcher }), - }); - } + defs.push(...toolMatcherEntryToCanonical({ rawEntry, converterConfig })); } if (defs.length > 0) { canonical[eventName] = defs; diff --git a/src/features/hooks/vibe-hooks.ts b/src/features/hooks/vibe-hooks.ts index 5e83b5ca2..e648d9907 100644 --- a/src/features/hooks/vibe-hooks.ts +++ b/src/features/hooks/vibe-hooks.ts @@ -121,6 +121,41 @@ function canonicalToVibeHooks( * Reverse {@link canonicalToVibeHooks}: parse the flat `[[hooks]]` array back * into a canonical event → definition[] record. */ +/** Convert one raw `[[hooks]]` entry to a canonical definition, or null to skip. */ +function vibeEntryToCanonicalDef( + raw: unknown, +): { canonicalEvent: string; def: HookDefinition } | null { + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { + return null; + } + const entry = raw as Record; + const vibeEvent = typeof entry.type === "string" ? entry.type : undefined; + if (vibeEvent === undefined) { + return null; + } + const canonicalEvent = VIBE_TO_CANONICAL_EVENT_NAMES[vibeEvent] ?? vibeEvent; + const def: HookDefinition = { type: "command" }; + if (typeof entry.command === "string") { + def.command = entry.command; + } + if (typeof entry.match === "string" && entry.match !== "" && entry.match !== "*") { + def.matcher = entry.match; + } + if (typeof entry.timeout === "number") { + def.timeout = entry.timeout; + } + if (typeof entry.name === "string") { + def.name = entry.name; + } + if (typeof entry.description === "string") { + def.description = entry.description; + } + if (typeof entry.strict === "boolean") { + (def as Record).strict = entry.strict; + } + return { canonicalEvent, def }; +} + function vibeHooksToCanonical(parsed: unknown): HooksConfig["hooks"] { const canonical: HooksConfig["hooks"] = {}; if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { @@ -131,37 +166,13 @@ function vibeHooksToCanonical(parsed: unknown): HooksConfig["hooks"] { return canonical; } for (const raw of rawHooks) { - if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { - continue; - } - const entry = raw as Record; - const vibeEvent = typeof entry.type === "string" ? entry.type : undefined; - if (vibeEvent === undefined) { + const result = vibeEntryToCanonicalDef(raw); + if (result === null) { continue; } - const canonicalEvent = VIBE_TO_CANONICAL_EVENT_NAMES[vibeEvent] ?? vibeEvent; - const def: HookDefinition = { type: "command" }; - if (typeof entry.command === "string") { - def.command = entry.command; - } - if (typeof entry.match === "string" && entry.match !== "" && entry.match !== "*") { - def.matcher = entry.match; - } - if (typeof entry.timeout === "number") { - def.timeout = entry.timeout; - } - if (typeof entry.name === "string") { - def.name = entry.name; - } - if (typeof entry.description === "string") { - def.description = entry.description; - } - if (typeof entry.strict === "boolean") { - (def as Record).strict = entry.strict; - } - const list = canonical[canonicalEvent] ?? []; - list.push(def); - canonical[canonicalEvent] = list; + const list = canonical[result.canonicalEvent] ?? []; + list.push(result.def); + canonical[result.canonicalEvent] = list; } return canonical; } diff --git a/src/features/mcp/goose-mcp.ts b/src/features/mcp/goose-mcp.ts index 113cb1958..2854e7253 100644 --- a/src/features/mcp/goose-mcp.ts +++ b/src/features/mcp/goose-mcp.ts @@ -55,6 +55,87 @@ function canonicalTransport(config: Record): string | undefined return undefined; } +/** + * Resolves the canonical remote URL for a server (`url` or the `httpUrl` alias). + */ +function resolveGooseUrl(config: Record): string | undefined { + return ( + (typeof config.url === "string" ? config.url : undefined) ?? + (typeof config.httpUrl === "string" ? config.httpUrl : undefined) + ); +} + +/** + * Determines the Goose `type` for a server based on its command/url/transport. + */ +function resolveGooseType(config: Record, url: string | undefined): string { + if (config.command !== undefined) { + return "stdio"; + } + if (url !== undefined) { + return canonicalTransport(config) === "sse" ? "sse" : "streamable_http"; + } + return canonicalTransport(config) === "builtin" ? "builtin" : "stdio"; +} + +/** + * Resolves the canonical timeout for a server (`timeout` or `networkTimeout`). + */ +function resolveGooseTimeout(config: Record): number | undefined { + if (typeof config.timeout === "number") return config.timeout; + if (typeof config.networkTimeout === "number") return config.networkTimeout; + return undefined; +} + +/** + * Populates the stdio-specific fields (`cmd`, `args`, `envs`) on a Goose ext. + */ +function applyGooseStdioFields( + ext: Record, + config: Record, +): void { + const command = config.command; + // `command` may be a string or an array; Goose `cmd` is a single + // executable, so an array's tail is folded into `args`. + if (Array.isArray(command)) { + if (typeof command[0] === "string") ext.cmd = command[0]; + const rest = command.slice(1).filter((c): c is string => typeof c === "string"); + const args = isStringArray(config.args) ? config.args : []; + if (rest.length > 0 || args.length > 0) ext.args = [...rest, ...args]; + } else if (typeof command === "string") { + ext.cmd = command; + if (isStringArray(config.args)) ext.args = config.args; + } + if (isRecord(config.env)) ext.envs = config.env; +} + +/** + * Converts a single rulesync canonical MCP server into a Goose `extensions:` entry. + */ +function convertServerToGooseExtension( + name: string, + config: Record, +): Record { + const url = resolveGooseUrl(config); + const gooseType = resolveGooseType(config, url); + + const ext: Record = { name, type: gooseType }; + + if (gooseType === "stdio") { + applyGooseStdioFields(ext, config); + } else if (gooseType === "sse" || gooseType === "streamable_http") { + if (url !== undefined) ext.uri = url; + if (isRecord(config.headers)) ext.headers = config.headers; + } + + ext.enabled = config.disabled !== true; + + const timeout = resolveGooseTimeout(config); + if (timeout !== undefined) ext.timeout = timeout; + + return ext; +} + /** * Converts rulesync canonical MCP servers into Goose `extensions:` entries. * @@ -67,53 +148,7 @@ function convertToGooseFormat(mcpServers: McpServers): Record = { name, type: gooseType }; - - if (gooseType === "stdio") { - // `command` may be a string or an array; Goose `cmd` is a single - // executable, so an array's tail is folded into `args`. - if (Array.isArray(command)) { - if (typeof command[0] === "string") ext.cmd = command[0]; - const rest = command.slice(1).filter((c): c is string => typeof c === "string"); - const args = isStringArray(config.args) ? config.args : []; - if (rest.length > 0 || args.length > 0) ext.args = [...rest, ...args]; - } else if (typeof command === "string") { - ext.cmd = command; - if (isStringArray(config.args)) ext.args = config.args; - } - if (isRecord(config.env)) ext.envs = config.env; - } else if (gooseType === "sse" || gooseType === "streamable_http") { - if (url !== undefined) ext.uri = url; - if (isRecord(config.headers)) ext.headers = config.headers; - } - - ext.enabled = config.disabled !== true; - - const timeout = - typeof config.timeout === "number" - ? config.timeout - : typeof config.networkTimeout === "number" - ? config.networkTimeout - : undefined; - if (timeout !== undefined) ext.timeout = timeout; - - extensions[name] = ext; + extensions[name] = convertServerToGooseExtension(name, config); } return extensions; diff --git a/src/features/mcp/kilo-mcp.ts b/src/features/mcp/kilo-mcp.ts index 0881ac81d..cf2f30530 100644 --- a/src/features/mcp/kilo-mcp.ts +++ b/src/features/mcp/kilo-mcp.ts @@ -166,6 +166,73 @@ function convertFromKiloFormat( * - disabled -> enabled (inverted) * - enabledTools/disabledTools -> top-level tools map (with server name prefix) */ +type McpServerConfig = McpServers[string]; + +/** + * Collect a server's enabledTools/disabledTools into the shared top-level tools + * map, prefixing each tool name with the server name. Mutates `tools` in place. + */ +function collectKiloServerTools( + tools: Record, + serverName: string, + serverConfig: McpServerConfig, +): void { + if (serverConfig.enabledTools) { + for (const tool of serverConfig.enabledTools) { + tools[`${serverName}_${tool}`] = true; + } + } + if (serverConfig.disabledTools) { + for (const tool of serverConfig.disabledTools) { + tools[`${serverName}_${tool}`] = false; + } + } +} + +/** + * Convert a single rulesync MCP server into its Kilo native form (local or remote). + */ +function convertServerToKiloFormat(serverConfig: McpServerConfig): KiloMcpServer { + const isRemote = serverConfig.type === "sse" || serverConfig.type === "http" || serverConfig.url; + + if (isRemote) { + // `oauth` is Kilo-specific (object | false) and carried through via the + // rulesync MCP server's looseObject passthrough; it is not a declared + // field on McpServerSchema. + const oauth = (serverConfig as { oauth?: unknown }).oauth; + return { + type: "remote", + url: serverConfig.url ?? serverConfig.httpUrl ?? "", + enabled: serverConfig.disabled !== undefined ? !serverConfig.disabled : true, + ...(serverConfig.headers && { headers: serverConfig.headers }), + ...(serverConfig.timeout !== undefined && { timeout: serverConfig.timeout }), + ...(oauth !== undefined && { oauth: oauth as z.infer }), + }; + } + + // Build command array: merge command and args + const commandArray: string[] = []; + if (serverConfig.command) { + if (Array.isArray(serverConfig.command)) { + commandArray.push(...serverConfig.command); + } else { + commandArray.push(serverConfig.command); + } + } + if (serverConfig.args) { + commandArray.push(...serverConfig.args); + } + + return { + type: "local", + command: commandArray, + enabled: serverConfig.disabled !== undefined ? !serverConfig.disabled : true, + ...(serverConfig.env && { environment: serverConfig.env }), + ...(serverConfig.cwd && { cwd: serverConfig.cwd }), + ...(serverConfig.timeout !== undefined && { timeout: serverConfig.timeout }), + }; +} + function convertToKiloFormat(mcpServers: McpServers): { mcp: Record; tools: Record; @@ -174,59 +241,8 @@ function convertToKiloFormat(mcpServers: McpServers): { const mcp = Object.fromEntries( Object.entries(mcpServers).map(([serverName, serverConfig]) => { - const isRemote = - serverConfig.type === "sse" || serverConfig.type === "http" || serverConfig.url; - - // Collect enabledTools/disabledTools into the top-level tools map - if (serverConfig.enabledTools) { - for (const tool of serverConfig.enabledTools) { - tools[`${serverName}_${tool}`] = true; - } - } - if (serverConfig.disabledTools) { - for (const tool of serverConfig.disabledTools) { - tools[`${serverName}_${tool}`] = false; - } - } - - if (isRemote) { - // `oauth` is Kilo-specific (object | false) and carried through via the - // rulesync MCP server's looseObject passthrough; it is not a declared - // field on McpServerSchema. - const oauth = (serverConfig as { oauth?: unknown }).oauth; - const remoteServer: KiloMcpServer = { - type: "remote", - url: serverConfig.url ?? serverConfig.httpUrl ?? "", - enabled: serverConfig.disabled !== undefined ? !serverConfig.disabled : true, - ...(serverConfig.headers && { headers: serverConfig.headers }), - ...(serverConfig.timeout !== undefined && { timeout: serverConfig.timeout }), - ...(oauth !== undefined && { oauth: oauth as z.infer }), - }; - return [serverName, remoteServer]; - } - - // Build command array: merge command and args - const commandArray: string[] = []; - if (serverConfig.command) { - if (Array.isArray(serverConfig.command)) { - commandArray.push(...serverConfig.command); - } else { - commandArray.push(serverConfig.command); - } - } - if (serverConfig.args) { - commandArray.push(...serverConfig.args); - } - - const localServer: KiloMcpServer = { - type: "local", - command: commandArray, - enabled: serverConfig.disabled !== undefined ? !serverConfig.disabled : true, - ...(serverConfig.env && { environment: serverConfig.env }), - ...(serverConfig.cwd && { cwd: serverConfig.cwd }), - ...(serverConfig.timeout !== undefined && { timeout: serverConfig.timeout }), - }; - return [serverName, localServer]; + collectKiloServerTools(tools, serverName, serverConfig); + return [serverName, convertServerToKiloFormat(serverConfig)]; }), ); diff --git a/src/features/permissions/cline-permissions.ts b/src/features/permissions/cline-permissions.ts index 239cdfcb6..421a93ca9 100644 --- a/src/features/permissions/cline-permissions.ts +++ b/src/features/permissions/cline-permissions.ts @@ -37,6 +37,84 @@ const ClineCommandPermissionsSchema = z.looseObject({ type ClineCommandPermissions = z.infer; +type ClineTranslationResult = { + allow: string[]; + deny: string[]; + droppedCategories: string[]; + translatedAskPatterns: string[]; +}; + +/** + * Translate rulesync permission categories into Cline allow/deny command lists. + * Non-bash categories and `ask` rules are tracked separately so a single + * translation notice can be surfaced by the caller. + */ +function translateClinePermissions( + permission: PermissionsConfig["permission"], +): ClineTranslationResult { + const allow: string[] = []; + const deny: string[] = []; + const droppedCategories: string[] = []; + const translatedAskPatterns: string[] = []; + + for (const [category, rules] of Object.entries(permission)) { + if (category !== "bash") { + droppedCategories.push(category); + continue; + } + for (const [pattern, action] of Object.entries(rules)) { + if (action === "ask") { + // Cline has no `ask` semantics. Translate to `deny` for fail-closed safety so the + // protective intent of the rule is preserved instead of being silently dropped. + translatedAskPatterns.push(pattern); + deny.push(pattern); + continue; + } + if (action === "allow") { + allow.push(pattern); + } else if (action === "deny") { + deny.push(pattern); + } + } + } + + return { allow, deny, droppedCategories, translatedAskPatterns }; +} + +/** + * Surface a single aggregated translation notice via `logger.warn` so that + * (a) CI gates that treat `error` lines as failures don't fail spuriously, matching the + * project convention used by every other permissions translator, and + * (b) the user still sees one prominent "WARNING" message describing the translation. + */ +function warnClineTranslationNotices({ + droppedCategories, + translatedAskPatterns, + logger, +}: { + droppedCategories: string[]; + translatedAskPatterns: string[]; + logger?: ToolPermissionsFromRulesyncPermissionsParams["logger"]; +}): void { + if (droppedCategories.length === 0 && translatedAskPatterns.length === 0) { + return; + } + const parts: string[] = []; + if (droppedCategories.length > 0) { + parts.push( + `non-bash categories [${droppedCategories.join(", ")}] (Cline only enforces shell ` + + `commands; use the rulesync ignore feature for read/write restrictions)`, + ); + } + if (translatedAskPatterns.length > 0) { + parts.push( + `'ask' rules for bash patterns [${translatedAskPatterns.join(", ")}] translated to ` + + `'deny' for fail-closed safety, since Cline lacks 'ask'`, + ); + } + logger?.warn(`WARNING: Cline command permissions translation notice: ${parts.join("; ")}.`); +} + export class ClinePermissions extends ToolPermissions { constructor(params: AiFileParams) { super({ @@ -110,53 +188,11 @@ export class ClinePermissions extends ToolPermissions { } const config = rulesyncPermissions.getJson(); - const allow: string[] = []; - const deny: string[] = []; - - // Translation notices are aggregated and surfaced via a single `logger.warn` per call so that - // (a) CI gates that treat `error` lines as failures don't fail spuriously, matching the - // project convention used by every other permissions translator, and - // (b) the user still sees one prominent "WARNING" message describing the translation. - const droppedCategories: string[] = []; - const translatedAskPatterns: string[] = []; - - for (const [category, rules] of Object.entries(config.permission)) { - if (category !== "bash") { - droppedCategories.push(category); - continue; - } - for (const [pattern, action] of Object.entries(rules)) { - if (action === "ask") { - // Cline has no `ask` semantics. Translate to `deny` for fail-closed safety so the - // protective intent of the rule is preserved instead of being silently dropped. - translatedAskPatterns.push(pattern); - deny.push(pattern); - continue; - } - if (action === "allow") { - allow.push(pattern); - } else if (action === "deny") { - deny.push(pattern); - } - } - } + const { allow, deny, droppedCategories, translatedAskPatterns } = translateClinePermissions( + config.permission, + ); - if (droppedCategories.length > 0 || translatedAskPatterns.length > 0) { - const parts: string[] = []; - if (droppedCategories.length > 0) { - parts.push( - `non-bash categories [${droppedCategories.join(", ")}] (Cline only enforces shell ` + - `commands; use the rulesync ignore feature for read/write restrictions)`, - ); - } - if (translatedAskPatterns.length > 0) { - parts.push( - `'ask' rules for bash patterns [${translatedAskPatterns.join(", ")}] translated to ` + - `'deny' for fail-closed safety, since Cline lacks 'ask'`, - ); - } - logger?.warn(`WARNING: Cline command permissions translation notice: ${parts.join("; ")}.`); - } + warnClineTranslationNotices({ droppedCategories, translatedAskPatterns, logger }); const dedupedAllow = uniq(allow.toSorted()); const dedupedDeny = uniq(deny.toSorted()); diff --git a/src/features/permissions/codexcli-permissions.ts b/src/features/permissions/codexcli-permissions.ts index b0c8c7d59..cd7596648 100644 --- a/src/features/permissions/codexcli-permissions.ts +++ b/src/features/permissions/codexcli-permissions.ts @@ -222,6 +222,32 @@ export function createCodexcliBashRulesFile({ }); } +function addCodexWebfetchRules({ + rules, + domains, + logger, +}: { + rules: Record; + domains: Record; + logger?: ToolPermissionsFromRulesyncPermissionsParams["logger"]; +}): void { + for (const [pattern, action] of Object.entries(rules)) { + if (action === "ask") { + logger?.warn( + `Codex CLI does not support "ask" for network domain permissions. Skipping webfetch rule: ${pattern}`, + ); + continue; + } + if (pattern === GLOBAL_WILDCARD_DOMAIN && action === "deny") { + logger?.warn( + `Codex CLI rejects the global wildcard "${pattern}" in denied network domains at config load time. Skipping webfetch rule; unlisted domains are denied by default.`, + ); + continue; + } + domains[pattern] = action; + } +} + function convertRulesyncToCodexProfile({ config, logger, @@ -236,26 +262,14 @@ function convertRulesyncToCodexProfile({ const domains: Record = {}; for (const [toolName, rules] of Object.entries(config.permission)) { - if (toolName === "read") { + if (toolName === "read" || toolName === "edit" || toolName === "write") { + const mapAction = toolName === "read" ? mapReadAction : mapWriteAction; for (const [pattern, action] of Object.entries(rules)) { addFilesystemRule({ filesystem, workspaceRootFilesystem, pattern, - access: mapReadAction(action), - logger, - }); - } - continue; - } - - if (toolName === "edit" || toolName === "write") { - for (const [pattern, action] of Object.entries(rules)) { - addFilesystemRule({ - filesystem, - workspaceRootFilesystem, - pattern, - access: mapWriteAction(action), + access: mapAction(action), logger, }); } @@ -263,21 +277,7 @@ function convertRulesyncToCodexProfile({ } if (toolName === "webfetch") { - for (const [pattern, action] of Object.entries(rules)) { - if (action === "ask") { - logger?.warn( - `Codex CLI does not support "ask" for network domain permissions. Skipping webfetch rule: ${pattern}`, - ); - continue; - } - if (pattern === GLOBAL_WILDCARD_DOMAIN && action === "deny") { - logger?.warn( - `Codex CLI rejects the global wildcard "${pattern}" in denied network domains at config load time. Skipping webfetch rule; unlisted domains are denied by default.`, - ); - continue; - } - domains[pattern] = action; - } + addCodexWebfetchRules({ rules, domains, logger }); continue; } diff --git a/src/features/rules/rules-processor.ts b/src/features/rules/rules-processor.ts index ec1a62cee..09bdffa11 100644 --- a/src/features/rules/rules-processor.ts +++ b/src/features/rules/rules-processor.ts @@ -831,74 +831,129 @@ export class RulesProcessor extends FeatureProcessor { this.foldNonRootRulesIntoRootRule(toolRules); } - const includeLocalRoot = resolveIncludeLocalRoot(this.featureOptions); + this.applyLocalRootRules({ toolRules, localRootRules, factory }); - // Handle localRoot rules (only in non-global mode and when enabled) - if (localRootRules.length > 0 && !this.global && includeLocalRoot) { - const localRootRule = localRootRules[0]; - if (localRootRule && factory.class.isTargetedByRulesyncRule(localRootRule)) { - this.handleLocalRootRule(toolRules, localRootRule, factory); - } + this.appendSeparateConventionsRule({ toolRules, factory }); + + const extraFiles = await this.buildMcpInstructionFiles({ toolRules, meta }); + + this.applyRootRuleSections({ toolRules, meta }); + + return [...toolRules, ...extraFiles]; + } + + /** + * Handle localRoot rules (only in non-global mode and when enabled). Mutates + * `toolRules` in place. + */ + private applyLocalRootRules({ + toolRules, + localRootRules, + factory, + }: { + toolRules: ToolRule[]; + localRootRules: RulesyncRule[]; + factory: ToolRuleFactory; + }): void { + const includeLocalRoot = resolveIncludeLocalRoot(this.featureOptions); + if (localRootRules.length === 0 || this.global || !includeLocalRoot) { + return; } + const localRootRule = localRootRules[0]; + if (localRootRule && factory.class.isTargetedByRulesyncRule(localRootRule)) { + this.handleLocalRootRule(toolRules, localRootRule, factory); + } + } + /** + * For tools that create a separate conventions rule file (e.g., cursor, roo), + * push that rule onto `toolRules`. Mutates `toolRules` in place. + */ + private appendSeparateConventionsRule({ + toolRules, + factory, + }: { + toolRules: ToolRule[]; + factory: ToolRuleFactory; + }): void { + const { meta } = factory; const isSimulated = this.simulateCommands || this.simulateSubagents || this.simulateSkills; - - // For tools that create a separate conventions rule file (e.g., cursor, roo) - if (isSimulated && meta.createsSeparateConventionsRule && meta.additionalConventions) { - const conventionsContent = this.generateAdditionalConventionsSectionFromMeta(meta); - const settablePaths = factory.class.getSettablePaths(); - const nonRootPath = "nonRoot" in settablePaths ? settablePaths.nonRoot : null; - if (nonRootPath) { - // Use .md extension - CursorRule.fromRulesyncRule will convert to .mdc - toolRules.push( - factory.class.fromRulesyncRule({ - outputRoot: this.outputRoot, - rulesyncRule: new RulesyncRule({ - outputRoot: this.outputRoot, - relativeDirPath: nonRootPath.relativeDirPath, - relativeFilePath: "additional-conventions.md", - frontmatter: { - root: false, - targets: [this.toolTarget], - }, - body: conventionsContent, - }), - validate: true, - global: this.global, - }), - ); - } + if (!isSimulated || !meta.createsSeparateConventionsRule || !meta.additionalConventions) { + return; } - // Non-root rules of some tools are not auto-loaded; the tool's MCP feature - // registers them in its shared config's `instructions` key. The root rule is - // auto-loaded and never registered. Project scope only. - const extraFiles: ToolFile[] = []; - if (meta.mcpInstructionsRegistrar && !this.global) { - const instructionPaths = toolRules - .filter((rule) => !rule.isRoot()) - .map((rule) => toPosixPath(join(rule.getRelativeDirPath(), rule.getRelativeFilePath()))); - if (instructionPaths.length > 0) { - extraFiles.push( - await meta.mcpInstructionsRegistrar.fromInstructions({ - outputRoot: this.outputRoot, - instructions: instructionPaths, - validate: true, - global: this.global, - }), - ); - } + const conventionsContent = this.generateAdditionalConventionsSectionFromMeta(meta); + const settablePaths = factory.class.getSettablePaths(); + const nonRootPath = "nonRoot" in settablePaths ? settablePaths.nonRoot : null; + if (!nonRootPath) { + return; } + // Use .md extension - CursorRule.fromRulesyncRule will convert to .mdc + toolRules.push( + factory.class.fromRulesyncRule({ + outputRoot: this.outputRoot, + rulesyncRule: new RulesyncRule({ + outputRoot: this.outputRoot, + relativeDirPath: nonRootPath.relativeDirPath, + relativeFilePath: "additional-conventions.md", + frontmatter: { + root: false, + targets: [this.toolTarget], + }, + body: conventionsContent, + }), + validate: true, + global: this.global, + }), + ); + } - const rootRuleIndex = toolRules.findIndex((rule) => rule.isRoot()); - if (rootRuleIndex === -1) { - return [...toolRules, ...extraFiles]; + /** + * Non-root rules of some tools are not auto-loaded; the tool's MCP feature + * registers them in its shared config's `instructions` key. The root rule is + * auto-loaded and never registered. Project scope only. + */ + private async buildMcpInstructionFiles({ + toolRules, + meta, + }: { + toolRules: ToolRule[]; + meta: ToolRuleFactory["meta"]; + }): Promise { + if (!meta.mcpInstructionsRegistrar || this.global) { + return []; } + const instructionPaths = toolRules + .filter((rule) => !rule.isRoot()) + .map((rule) => toPosixPath(join(rule.getRelativeDirPath(), rule.getRelativeFilePath()))); + if (instructionPaths.length === 0) { + return []; + } + return [ + await meta.mcpInstructionsRegistrar.fromInstructions({ + outputRoot: this.outputRoot, + instructions: instructionPaths, + validate: true, + global: this.global, + }), + ]; + } - // For tools that don't create a separate conventions rule, prepend to the root rule - const rootRule = toolRules[rootRuleIndex]; + /** + * For tools that don't create a separate conventions rule, prepend the + * reference and conventions sections to the root rule content. Mutates the + * root rule in place. + */ + private applyRootRuleSections({ + toolRules, + meta, + }: { + toolRules: ToolRule[]; + meta: ToolRuleFactory["meta"]; + }): void { + const rootRule = toolRules.find((rule) => rule.isRoot()); if (!rootRule) { - return [...toolRules, ...extraFiles]; + return; } // Generate reference section based on meta configuration @@ -917,8 +972,6 @@ export class RulesProcessor extends FeatureProcessor { if (meta.mirrorsRootToAgentsMd && !this.global) { this.mirrorRootRuleToAgentsMd({ toolRules, rootRule, content: newContent }); } - - return [...toolRules, ...extraFiles]; } /** diff --git a/src/features/skills/vibe-skill.ts b/src/features/skills/vibe-skill.ts index 07ff0df19..2bd6537ea 100644 --- a/src/features/skills/vibe-skill.ts +++ b/src/features/skills/vibe-skill.ts @@ -6,7 +6,12 @@ import { SKILL_FILE_NAME } from "../../constants/general.js"; import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; import { ValidationResult } from "../../types/ai-dir.js"; import { formatError } from "../../utils/error.js"; -import { RulesyncSkill, RulesyncSkillFrontmatterInput, SkillFile } from "./rulesync-skill.js"; +import { + RulesyncSkill, + RulesyncSkillFrontmatter, + RulesyncSkillFrontmatterInput, + SkillFile, +} from "./rulesync-skill.js"; import { resolveUserInvocable } from "./skills-utils.js"; import { ToolSkill, @@ -39,6 +44,70 @@ export type VibeSkillParams = { global?: boolean; }; +/** Resolve the top-level `license` field, if present as a string. */ +function resolveTopLevelLicense(looseTopLevel: Record): string | undefined { + return typeof looseTopLevel.license === "string" ? looseTopLevel.license : undefined; +} + +/** Resolve the top-level `compatibility` field (string or object), if present. */ +function resolveTopLevelCompatibility( + looseTopLevel: Record, +): string | Record | undefined { + const value = looseTopLevel.compatibility; + if (typeof value === "string" || (typeof value === "object" && value !== null)) { + return value as string | Record; + } + return undefined; +} + +/** Resolve the top-level `metadata` field (object), if present. */ +function resolveTopLevelMetadata( + looseTopLevel: Record, +): Record | undefined { + const value = looseTopLevel.metadata; + return typeof value === "object" && value !== null + ? (value as Record) + : undefined; +} + +/** + * Build the Vibe frontmatter from a rulesync skill frontmatter, preferring the + * dedicated `vibe` section over any loosely-typed top-level fields. + */ +function buildVibeFrontmatter(rulesyncFrontmatter: RulesyncSkillFrontmatter): VibeSkillFrontmatter { + const vibeSection = rulesyncFrontmatter.vibe; + + const looseTopLevel = rulesyncFrontmatter as Record; + const topLevelLicense = resolveTopLevelLicense(looseTopLevel); + const topLevelCompatibility = resolveTopLevelCompatibility(looseTopLevel); + const topLevelMetadata = resolveTopLevelMetadata(looseTopLevel); + + const resolvedUserInvocable = resolveUserInvocable({ + rootFrontmatter: rulesyncFrontmatter, + section: vibeSection, + }); + + return { + name: rulesyncFrontmatter.name, + description: rulesyncFrontmatter.description, + ...(vibeSection?.license !== undefined || topLevelLicense !== undefined + ? { license: vibeSection?.license ?? topLevelLicense } + : {}), + ...(vibeSection?.compatibility !== undefined || topLevelCompatibility !== undefined + ? { compatibility: vibeSection?.compatibility ?? topLevelCompatibility } + : {}), + ...(vibeSection?.metadata !== undefined || topLevelMetadata !== undefined + ? { metadata: vibeSection?.metadata ?? topLevelMetadata } + : {}), + ...(resolvedUserInvocable !== undefined && { + "user-invocable": resolvedUserInvocable, + }), + ...(vibeSection?.["allowed-tools"] !== undefined && { + "allowed-tools": vibeSection["allowed-tools"], + }), + }; +} + export class VibeSkill extends ToolSkill { constructor({ outputRoot = process.cwd(), @@ -147,45 +216,8 @@ export class VibeSkill extends ToolSkill { }: ToolSkillFromRulesyncSkillParams): VibeSkill { const settablePaths = VibeSkill.getSettablePaths({ global }); const rulesyncFrontmatter = rulesyncSkill.getFrontmatter(); - const vibeSection = rulesyncFrontmatter.vibe; - - const looseTopLevel = rulesyncFrontmatter as Record; - const topLevelLicense = - typeof looseTopLevel.license === "string" ? looseTopLevel.license : undefined; - const topLevelCompatibility = - typeof looseTopLevel.compatibility === "string" || - (typeof looseTopLevel.compatibility === "object" && looseTopLevel.compatibility !== null) - ? (looseTopLevel.compatibility as string | Record) - : undefined; - const topLevelMetadata = - typeof looseTopLevel.metadata === "object" && looseTopLevel.metadata !== null - ? (looseTopLevel.metadata as Record) - : undefined; - - const resolvedUserInvocable = resolveUserInvocable({ - rootFrontmatter: rulesyncFrontmatter, - section: vibeSection, - }); - const vibeFrontmatter: VibeSkillFrontmatter = { - name: rulesyncFrontmatter.name, - description: rulesyncFrontmatter.description, - ...(vibeSection?.license !== undefined || topLevelLicense !== undefined - ? { license: vibeSection?.license ?? topLevelLicense } - : {}), - ...(vibeSection?.compatibility !== undefined || topLevelCompatibility !== undefined - ? { compatibility: vibeSection?.compatibility ?? topLevelCompatibility } - : {}), - ...(vibeSection?.metadata !== undefined || topLevelMetadata !== undefined - ? { metadata: vibeSection?.metadata ?? topLevelMetadata } - : {}), - ...(resolvedUserInvocable !== undefined && { - "user-invocable": resolvedUserInvocable, - }), - ...(vibeSection?.["allowed-tools"] !== undefined && { - "allowed-tools": vibeSection["allowed-tools"], - }), - }; + const vibeFrontmatter = buildVibeFrontmatter(rulesyncFrontmatter); return new VibeSkill({ outputRoot, diff --git a/src/lib/apm/apm-install.ts b/src/lib/apm/apm-install.ts index 862e1b7b0..e9033a2ec 100644 --- a/src/lib/apm/apm-install.ts +++ b/src/lib/apm/apm-install.ts @@ -79,39 +79,7 @@ export async function installApm(params: { const existingLock = await readApmLock(projectRoot); if (options.frozen) { - if (!existingLock) { - throw new Error( - "Frozen install failed: rulesync-apm.lock.yaml is missing. Run 'rulesync install --mode apm' to create it.", - ); - } - const missing = manifest.dependencies.filter( - (dep) => !findApmLockDependency(existingLock, canonicalRepoUrl(dep)), - ); - if (missing.length > 0) { - const names = missing.map((d) => d.gitUrl).join(", "); - throw new Error( - `Frozen install failed: rulesync-apm.lock.yaml is missing entries for: ${names}. Run 'rulesync install --mode apm' to update the lockfile.`, - ); - } - // Detect manifest drift: when the user edited `ref` in apm.yml without - // re-running install, the locked ref no longer matches the declared one. - // In frozen mode we refuse rather than silently install the locked SHA. - const drifted = manifest.dependencies.filter((dep) => { - if (dep.ref === undefined) return false; - const locked = findApmLockDependency(existingLock, canonicalRepoUrl(dep)); - return locked?.resolved_ref !== undefined && locked.resolved_ref !== dep.ref; - }); - if (drifted.length > 0) { - const names = drifted - .map((d) => { - const locked = findApmLockDependency(existingLock, canonicalRepoUrl(d)); - return `${d.gitUrl} (manifest=${d.ref}, lock=${locked?.resolved_ref})`; - }) - .join(", "); - throw new Error( - `Frozen install failed: manifest ref does not match rulesync-apm.lock.yaml for: ${names}. Run 'rulesync install --mode apm' to update the lockfile.`, - ); - } + assertFrozenLockCoversManifest({ existingLock, dependencies: manifest.dependencies }); } const token = GitHubClient.resolveToken(options.token); @@ -208,32 +176,7 @@ export async function installApm(params: { // path. Offending entries are skipped with a warn log rather than fatal so // that a single bad row cannot brick the install. if (existingLock) { - const newDeployedFiles = new Set(newLock.dependencies.flatMap((d) => d.deployed_files)); - const toDelete: string[] = []; - for (const prev of existingLock.dependencies) { - for (const deployed of prev.deployed_files) { - if (!newDeployedFiles.has(deployed)) { - toDelete.push(deployed); - } - } - } - for (const relativePath of toDelete) { - if (posix.isAbsolute(relativePath) || relativePath.split(/[/\\]/).includes("..")) { - logger.warn(`Refusing to remove stale apm file with suspicious path: "${relativePath}".`); - continue; - } - try { - checkPathTraversal({ relativePath, intendedRootDir: projectRoot }); - } catch { - logger.warn(`Refusing to remove stale apm file outside projectRoot: "${relativePath}".`); - continue; - } - const absolute = join(projectRoot, relativePath); - // `removeFile` is best-effort and swallows ENOENT, so missing files are - // a no-op. This keeps a corrupted partial-install from blowing up here. - await removeFile(absolute); - logger.debug(`Removed stale apm file: ${relativePath}`); - } + await removeStaleApmFiles({ existingLock, newLock, projectRoot, logger }); } // Always rewrite the lockfile (except under --frozen, which is a verify-only @@ -259,6 +202,93 @@ export async function installApm(params: { }; } +/** + * Frozen-mode validation: the lockfile must exist, cover every manifest + * dependency, and not have drifted from any declared `ref`. Throws with + * remediation guidance on the first failing check (preserving the original + * order: missing-lock, missing-entries, then ref drift). + */ +function assertFrozenLockCoversManifest(params: { + existingLock: ApmLock | null; + dependencies: ApmDependency[]; +}): asserts params is { existingLock: ApmLock; dependencies: ApmDependency[] } { + const { existingLock, dependencies } = params; + if (!existingLock) { + throw new Error( + "Frozen install failed: rulesync-apm.lock.yaml is missing. Run 'rulesync install --mode apm' to create it.", + ); + } + const missing = dependencies.filter( + (dep) => !findApmLockDependency(existingLock, canonicalRepoUrl(dep)), + ); + if (missing.length > 0) { + const names = missing.map((d) => d.gitUrl).join(", "); + throw new Error( + `Frozen install failed: rulesync-apm.lock.yaml is missing entries for: ${names}. Run 'rulesync install --mode apm' to update the lockfile.`, + ); + } + // Detect manifest drift: when the user edited `ref` in apm.yml without + // re-running install, the locked ref no longer matches the declared one. + // In frozen mode we refuse rather than silently install the locked SHA. + const drifted = dependencies.filter((dep) => { + if (dep.ref === undefined) return false; + const locked = findApmLockDependency(existingLock, canonicalRepoUrl(dep)); + return locked?.resolved_ref !== undefined && locked.resolved_ref !== dep.ref; + }); + if (drifted.length > 0) { + const names = drifted + .map((d) => { + const locked = findApmLockDependency(existingLock, canonicalRepoUrl(d)); + return `${d.gitUrl} (manifest=${d.ref}, lock=${locked?.resolved_ref})`; + }) + .join(", "); + throw new Error( + `Frozen install failed: manifest ref does not match rulesync-apm.lock.yaml for: ${names}. Run 'rulesync install --mode apm' to update the lockfile.`, + ); + } +} + +/** + * Remove files that a previous install deployed but that are no longer part of + * any current dependency's `deployed_files`. Each entry is path-traversal + * hardened (shape check + `checkPathTraversal`) and offending rows are skipped + * with a warn log rather than fatal. + */ +async function removeStaleApmFiles(params: { + existingLock: ApmLock; + newLock: ApmLock; + projectRoot: string; + logger: Logger; +}): Promise { + const { existingLock, newLock, projectRoot, logger } = params; + const newDeployedFiles = new Set(newLock.dependencies.flatMap((d) => d.deployed_files)); + const toDelete: string[] = []; + for (const prev of existingLock.dependencies) { + for (const deployed of prev.deployed_files) { + if (!newDeployedFiles.has(deployed)) { + toDelete.push(deployed); + } + } + } + for (const relativePath of toDelete) { + if (posix.isAbsolute(relativePath) || relativePath.split(/[/\\]/).includes("..")) { + logger.warn(`Refusing to remove stale apm file with suspicious path: "${relativePath}".`); + continue; + } + try { + checkPathTraversal({ relativePath, intendedRootDir: projectRoot }); + } catch { + logger.warn(`Refusing to remove stale apm file outside projectRoot: "${relativePath}".`); + continue; + } + const absolute = join(projectRoot, relativePath); + // `removeFile` is best-effort and swallows ENOENT, so missing files are + // a no-op. This keeps a corrupted partial-install from blowing up here. + await removeFile(absolute); + logger.debug(`Removed stale apm file: ${relativePath}`); + } +} + async function installDependency(params: { dep: ApmDependency; client: GitHubClient; @@ -305,73 +335,27 @@ async function installDependency(params: { }); if (files.length === 0) continue; - for (const file of files) { - if (file.size > MAX_FILE_SIZE) { - logger.warn( - `Skipping "${file.path}" from ${repoUrl}: ${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - ); - continue; - } - const relativeToBase = posix.relative(remoteBase, toPosixPath(file.path)); - if (!relativeToBase || relativeToBase.startsWith("..") || posix.isAbsolute(relativeToBase)) { - logger.warn( - `Skipping "${file.path}" from ${repoUrl}: resolved outside of "${remoteBase}".`, - ); - continue; - } - const deployRelative = toPosixPath(join(primitive.deployDir, relativeToBase)); - checkPathTraversal({ - relativePath: deployRelative, - intendedRootDir: projectRoot, - }); - const content = await withSemaphore(semaphore, () => - client.getFileContent(dep.owner, dep.repo, file.path, resolvedSha), - ); - // The tree-listing size can lie (LFS pointers, filter-driver output), - // so enforce the cap on the fetched bytes as well. - const byteLength = Buffer.byteLength(content, "utf8"); - if (byteLength > MAX_FILE_SIZE) { - logger.warn( - `Skipping "${file.path}" from ${repoUrl}: fetched ${(byteLength / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - ); - continue; - } - deployed.push({ path: deployRelative, content }); - if (!frozen) { - await writeFileContent(join(projectRoot, deployRelative), content); - } - } + await collectPrimitiveDeployments({ + dep, + client, + semaphore, + projectRoot, + primitive, + remoteBase, + files, + resolvedSha, + repoUrl, + frozen, + deployed, + logger, + }); } deployed.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0)); const deployedFiles = deployed.map((d) => d.path); const contentHash = computeContentHash(deployed); - // Verify integrity against the lockfile when running frozen and the prior - // lock recorded a hash rulesync itself wrote. A mismatch means either the - // upstream content moved under the same SHA (unlikely with git) or someone - // tampered with the lockfile / deployed files. We do this *before* writing - // anything to disk under --frozen so that tampered bytes never hit the - // filesystem. - // - // If the recorded hash does not match the rulesync format (e.g. the - // lockfile was produced by the upstream `apm` CLI which writes a different - // shape), we skip the integrity check rather than fail — the commit SHA - // pin is still enforced, and this preserves interop for users migrating - // from `apm` to `rulesync install --mode apm`. - if (frozen && locked?.content_hash) { - if (RULESYNC_CONTENT_HASH_REGEX.test(locked.content_hash)) { - if (locked.content_hash !== contentHash) { - throw new Error( - `content_hash mismatch for ${repoUrl}: lock=${locked.content_hash} computed=${contentHash}. Refuse to trust the deployment under --frozen.`, - ); - } - } else { - logger.debug( - `Skipping content_hash integrity check for ${repoUrl}: recorded hash "${locked.content_hash}" was not written by rulesync.`, - ); - } - } + assertFrozenContentHashMatches({ frozen, locked, contentHash, repoUrl, logger }); // Under --frozen we deferred all writes until after the hash check passed. if (frozen) { @@ -398,6 +382,114 @@ async function installDependency(params: { return { lockEntry, deployedFiles }; } +/** + * Fetch and validate the files for a single primitive directory, appending the + * deployable (path, content) pairs to `deployed`. Oversized or out-of-bounds + * files are skipped with a warn log; under non-frozen mode bytes are written to + * disk as they are collected. + */ +async function collectPrimitiveDeployments(params: { + dep: ApmDependency; + client: GitHubClient; + semaphore: Semaphore; + projectRoot: string; + primitive: (typeof APM_PRIMITIVES)[number]; + remoteBase: string; + files: GitHubFileEntry[]; + resolvedSha: string; + repoUrl: string; + frozen: boolean; + deployed: Array<{ path: string; content: string }>; + logger: Logger; +}): Promise { + const { + dep, + client, + semaphore, + projectRoot, + primitive, + remoteBase, + files, + resolvedSha, + repoUrl, + frozen, + deployed, + logger, + } = params; + + for (const file of files) { + if (file.size > MAX_FILE_SIZE) { + logger.warn( + `Skipping "${file.path}" from ${repoUrl}: ${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, + ); + continue; + } + const relativeToBase = posix.relative(remoteBase, toPosixPath(file.path)); + if (!relativeToBase || relativeToBase.startsWith("..") || posix.isAbsolute(relativeToBase)) { + logger.warn(`Skipping "${file.path}" from ${repoUrl}: resolved outside of "${remoteBase}".`); + continue; + } + const deployRelative = toPosixPath(join(primitive.deployDir, relativeToBase)); + checkPathTraversal({ + relativePath: deployRelative, + intendedRootDir: projectRoot, + }); + const content = await withSemaphore(semaphore, () => + client.getFileContent(dep.owner, dep.repo, file.path, resolvedSha), + ); + // The tree-listing size can lie (LFS pointers, filter-driver output), + // so enforce the cap on the fetched bytes as well. + const byteLength = Buffer.byteLength(content, "utf8"); + if (byteLength > MAX_FILE_SIZE) { + logger.warn( + `Skipping "${file.path}" from ${repoUrl}: fetched ${(byteLength / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, + ); + continue; + } + deployed.push({ path: deployRelative, content }); + if (!frozen) { + await writeFileContent(join(projectRoot, deployRelative), content); + } + } +} + +/** + * Verify integrity against the lockfile when running frozen and the prior + * lock recorded a hash rulesync itself wrote. A mismatch means either the + * upstream content moved under the same SHA (unlikely with git) or someone + * tampered with the lockfile / deployed files. We do this *before* writing + * anything to disk under --frozen so that tampered bytes never hit the + * filesystem. + * + * If the recorded hash does not match the rulesync format (e.g. the + * lockfile was produced by the upstream `apm` CLI which writes a different + * shape), we skip the integrity check rather than fail — the commit SHA + * pin is still enforced, and this preserves interop for users migrating + * from `apm` to `rulesync install --mode apm`. + */ +function assertFrozenContentHashMatches(params: { + frozen: boolean; + locked: ApmLockDependency | undefined; + contentHash: string; + repoUrl: string; + logger: Logger; +}): void { + const { frozen, locked, contentHash, repoUrl, logger } = params; + if (frozen && locked?.content_hash) { + if (RULESYNC_CONTENT_HASH_REGEX.test(locked.content_hash)) { + if (locked.content_hash !== contentHash) { + throw new Error( + `content_hash mismatch for ${repoUrl}: lock=${locked.content_hash} computed=${contentHash}. Refuse to trust the deployment under --frozen.`, + ); + } + } else { + logger.debug( + `Skipping content_hash integrity check for ${repoUrl}: recorded hash "${locked.content_hash}" was not written by rulesync.`, + ); + } + } +} + /** * SHA-256 over a canonical, order-independent representation of the deployed * files. Written into `content_hash` so that `--frozen` installs can refuse diff --git a/src/lib/gh/gh-install.ts b/src/lib/gh/gh-install.ts index da8a31b03..9ac9789d4 100644 --- a/src/lib/gh/gh-install.ts +++ b/src/lib/gh/gh-install.ts @@ -69,6 +69,10 @@ type SkillInstallation = { deployed: DeployedFile[]; }; +type SourceResult = + | { status: "ok"; installations: SkillInstallation[] } + | { status: "failed"; preserved: GhLockInstallation[] }; + /** * Entry point for `rulesync install --mode gh`. Reads `sources` from * `rulesync.jsonc`, resolves each one against the GitHub API, and deploys @@ -91,43 +95,7 @@ export async function installGh(params: { // Pre-resolve every source's owner/repo + agent/scope defaults so the // frozen-mode coverage check below has a stable view of what installations // are required. We do not contact the API yet — that happens per-source. - const resolvedSources: ResolvedSource[] = sources.map((entry) => { - const parsed = parseSource(entry.source); - if (parsed.provider !== "github") { - throw new Error( - `--mode gh only supports GitHub sources. "${entry.source}" resolves to provider "${parsed.provider}".`, - ); - } - // gh mode does not honor `transport` or `path` from the SourceEntry — - // both are rulesync-mode-only concepts. Silently dropping them would - // surprise users migrating from --mode rulesync, so reject up-front - // with a message that names the offending field. - if (entry.transport !== undefined && entry.transport !== "github") { - throw new Error( - `--mode gh: field "transport" is not supported (got "${entry.transport}" for source "${entry.source}"). Drop the field or switch to --mode rulesync.`, - ); - } - if (entry.path !== undefined) { - throw new Error( - `--mode gh: field "path" is not supported for source "${entry.source}". The remote layout is fixed to "skills//SKILL.md".`, - ); - } - const agent = entry.agent ?? "github-copilot"; - if (!GH_AGENTS.includes(agent)) { - throw new Error( - `--mode gh: unknown agent "${agent}" for source "${entry.source}". Valid agents: ${GH_AGENTS.join(", ")}.`, - ); - } - const scope: GhScope = entry.scope ?? "project"; - return { - entry, - owner: parsed.owner, - repo: parsed.repo, - ref: entry.ref ?? parsed.ref, - agent, - scope, - }; - }); + const resolvedSources: ResolvedSource[] = sources.map(resolveGhSource); const existingLock = await readGhLock(projectRoot); const frozen = options.frozen ?? false; @@ -139,53 +107,8 @@ export async function installGh(params: { ); } - // Frozen mode: per-source coverage check. A brand-new source (no - // installations at all in the lock) must fail before we contact the - // GitHub API — both to save quota and to prevent in-flight Promise.all - // siblings from writing files when another source is going to throw. - // Per-skill coverage (when entry.skills lists names that ARE absent from - // the lock) is still enforced lazily inside installSource, since that - // requires API discovery to know which skills exist remotely. if (frozen && existingLock) { - const uncovered: string[] = []; - for (const rs of resolvedSources) { - const hasAny = existingLock.installations.some( - (i) => - i.source.toLowerCase() === rs.entry.source.toLowerCase() && - i.agent === rs.agent && - i.scope === rs.scope, - ); - if (!hasAny) { - uncovered.push(`${rs.entry.source} (agent=${rs.agent}, scope=${rs.scope})`); - } - } - if (uncovered.length > 0) { - throw new Error( - `Frozen install failed: rulesync-gh.lock.yaml is missing entries for: ${uncovered.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, - ); - } - - // Detect manifest drift on `ref`: when the user edited `ref` in - // rulesync.jsonc without re-running install, refuse rather than - // silently install the locked SHA against a different declared ref. - const drifted: string[] = []; - for (const rs of resolvedSources) { - if (!rs.ref) continue; - const matches = existingLock.installations.filter( - (i) => i.source.toLowerCase() === rs.entry.source.toLowerCase(), - ); - for (const m of matches) { - if (m.requested_ref !== undefined && m.requested_ref !== rs.ref) { - drifted.push(`${rs.entry.source} (manifest=${rs.ref}, lock=${m.requested_ref})`); - break; - } - } - } - if (drifted.length > 0) { - throw new Error( - `Frozen install failed: manifest ref does not match rulesync-gh.lock.yaml for: ${drifted.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, - ); - } + assertFrozenLockCoversSources({ existingLock, resolvedSources }); } const token = GitHubClient.resolveToken(options.token); @@ -194,10 +117,6 @@ export async function installGh(params: { const newLock: GhLock = createEmptyGhLock({ existingLock }); - type SourceResult = - | { status: "ok"; installations: SkillInstallation[] } - | { status: "failed"; preserved: GhLockInstallation[] }; - const runOne = async (rs: ResolvedSource): Promise => { const installations = await installSource({ rs, @@ -235,61 +154,15 @@ export async function installGh(params: { }), ); - // Frozen-mode deferred writes. `installSource` never touches the disk - // under --frozen — every write lands here, only after Promise.all has - // resolved successfully for every source. Without this gate, source A - // could finish writing its bytes before source B's coverage / integrity - // check throws, leaving the working tree in a partially-frozen state - // despite the install reporting failure. if (frozen) { - for (const result of results) { - if (result.status !== "ok") continue; - for (const inst of result.installations) { - for (const d of inst.deployed) { - await writeFileContent(d.absolutePath, d.content); - } - } - } + await writeDeferredFrozenFiles(results); } - let totalInstalled = 0; - let failedCount = 0; - for (const result of results) { - if (result.status === "ok") { - for (const inst of result.installations) { - newLock.installations.push(inst.installation); - } - totalInstalled += result.installations.length; - } else { - failedCount += 1; - for (const preserved of result.preserved) { - newLock.installations.push(preserved); - } - } - } + const { totalInstalled, failedCount } = aggregateSourceResults({ results, newLock }); // Stale-file cleanup. Same hardening shape as apm-install. if (existingLock) { - const newDeployed = new Set(); - for (const inst of newLock.installations) { - for (const file of inst.deployed_files) { - // Key by (scope, path) so a file in `/.claude/skills/foo` and a - // file at `/.claude/skills/foo` are not conflated. - newDeployed.add(`${inst.scope}::${file}`); - } - } - for (const prev of existingLock.installations) { - for (const deployed of prev.deployed_files) { - const key = `${prev.scope}::${deployed}`; - if (newDeployed.has(key)) continue; - await removeStaleFile({ - relativePath: deployed, - scope: prev.scope === "user" ? "user" : "project", - projectRoot, - logger, - }); - } - } + await removeStaleGhFiles({ existingLock, newLock, projectRoot, logger }); } if (!frozen) { @@ -311,6 +184,183 @@ export async function installGh(params: { }; } +/** + * Validate and normalize a single declared source into a ResolvedSource without + * contacting the API. Rejects non-GitHub providers and the gh-unsupported + * `transport`/`path` fields, and applies the agent/scope defaults. + */ +function resolveGhSource(entry: SourceEntry): ResolvedSource { + const parsed = parseSource(entry.source); + if (parsed.provider !== "github") { + throw new Error( + `--mode gh only supports GitHub sources. "${entry.source}" resolves to provider "${parsed.provider}".`, + ); + } + // gh mode does not honor `transport` or `path` from the SourceEntry — + // both are rulesync-mode-only concepts. Silently dropping them would + // surprise users migrating from --mode rulesync, so reject up-front + // with a message that names the offending field. + if (entry.transport !== undefined && entry.transport !== "github") { + throw new Error( + `--mode gh: field "transport" is not supported (got "${entry.transport}" for source "${entry.source}"). Drop the field or switch to --mode rulesync.`, + ); + } + if (entry.path !== undefined) { + throw new Error( + `--mode gh: field "path" is not supported for source "${entry.source}". The remote layout is fixed to "skills//SKILL.md".`, + ); + } + const agent = entry.agent ?? "github-copilot"; + if (!GH_AGENTS.includes(agent)) { + throw new Error( + `--mode gh: unknown agent "${agent}" for source "${entry.source}". Valid agents: ${GH_AGENTS.join(", ")}.`, + ); + } + const scope: GhScope = entry.scope ?? "project"; + return { + entry, + owner: parsed.owner, + repo: parsed.repo, + ref: entry.ref ?? parsed.ref, + agent, + scope, + }; +} + +/** + * Frozen mode: per-source coverage check plus `ref` drift detection. A + * brand-new source (no installations at all in the lock) must fail before we + * contact the GitHub API — both to save quota and to prevent in-flight + * Promise.all siblings from writing files when another source is going to + * throw. Per-skill coverage is enforced lazily inside installSource, since that + * requires API discovery to know which skills exist remotely. + */ +function assertFrozenLockCoversSources(params: { + existingLock: GhLock; + resolvedSources: ResolvedSource[]; +}): void { + const { existingLock, resolvedSources } = params; + const uncovered: string[] = []; + for (const rs of resolvedSources) { + const hasAny = existingLock.installations.some( + (i) => + i.source.toLowerCase() === rs.entry.source.toLowerCase() && + i.agent === rs.agent && + i.scope === rs.scope, + ); + if (!hasAny) { + uncovered.push(`${rs.entry.source} (agent=${rs.agent}, scope=${rs.scope})`); + } + } + if (uncovered.length > 0) { + throw new Error( + `Frozen install failed: rulesync-gh.lock.yaml is missing entries for: ${uncovered.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, + ); + } + + // Detect manifest drift on `ref`: when the user edited `ref` in + // rulesync.jsonc without re-running install, refuse rather than + // silently install the locked SHA against a different declared ref. + const drifted: string[] = []; + for (const rs of resolvedSources) { + if (!rs.ref) continue; + const matches = existingLock.installations.filter( + (i) => i.source.toLowerCase() === rs.entry.source.toLowerCase(), + ); + for (const m of matches) { + if (m.requested_ref !== undefined && m.requested_ref !== rs.ref) { + drifted.push(`${rs.entry.source} (manifest=${rs.ref}, lock=${m.requested_ref})`); + break; + } + } + } + if (drifted.length > 0) { + throw new Error( + `Frozen install failed: manifest ref does not match rulesync-gh.lock.yaml for: ${drifted.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, + ); + } +} + +/** + * Frozen-mode deferred writes. `installSource` never touches the disk under + * --frozen — every write lands here, only after Promise.all has resolved + * successfully for every source. Without this gate, source A could finish + * writing its bytes before source B's coverage / integrity check throws, + * leaving the working tree in a partially-frozen state despite the install + * reporting failure. + */ +async function writeDeferredFrozenFiles(results: SourceResult[]): Promise { + for (const result of results) { + if (result.status !== "ok") continue; + for (const inst of result.installations) { + for (const d of inst.deployed) { + await writeFileContent(d.absolutePath, d.content); + } + } + } +} + +/** + * Push each source result's installations (or preserved prior entries on + * failure) into the new lock, returning the installed and failed counts. + */ +function aggregateSourceResults(params: { results: SourceResult[]; newLock: GhLock }): { + totalInstalled: number; + failedCount: number; +} { + const { results, newLock } = params; + let totalInstalled = 0; + let failedCount = 0; + for (const result of results) { + if (result.status === "ok") { + for (const inst of result.installations) { + newLock.installations.push(inst.installation); + } + totalInstalled += result.installations.length; + } else { + failedCount += 1; + for (const preserved of result.preserved) { + newLock.installations.push(preserved); + } + } + } + return { totalInstalled, failedCount }; +} + +/** + * Remove files deployed by a previous install that are no longer part of any + * current installation, keyed by (scope, path) so identically-named files under + * different scope roots are not conflated. + */ +async function removeStaleGhFiles(params: { + existingLock: GhLock; + newLock: GhLock; + projectRoot: string; + logger: Logger; +}): Promise { + const { existingLock, newLock, projectRoot, logger } = params; + const newDeployed = new Set(); + for (const inst of newLock.installations) { + for (const file of inst.deployed_files) { + // Key by (scope, path) so a file in `/.claude/skills/foo` and a + // file at `/.claude/skills/foo` are not conflated. + newDeployed.add(`${inst.scope}::${file}`); + } + } + for (const prev of existingLock.installations) { + for (const deployed of prev.deployed_files) { + const key = `${prev.scope}::${deployed}`; + if (newDeployed.has(key)) continue; + await removeStaleFile({ + relativePath: deployed, + scope: prev.scope === "user" ? "user" : "project", + projectRoot, + logger, + }); + } + } +} + async function installSource(params: { rs: ResolvedSource; client: GitHubClient; @@ -325,96 +375,36 @@ async function installSource(params: { const { entry, owner, repo, agent, scope } = rs; const sourceKey = entry.source; - // Resolve the ref. Order: - // 1. entry.ref (explicit pin) - // 2. latest release's tag_name - // 3. default branch (when the repo has no releases) - let resolvedRef: string; - let usedTag = false; - if (rs.ref) { - resolvedRef = rs.ref; - } else { - try { - const release = await client.getLatestRelease(owner, repo); - resolvedRef = release.tag_name; - usedTag = true; - } catch (error) { - // gh's behavior: when a repo has no releases, getLatestRelease returns - // 404. We treat any 404 (real GitHubClientError or any thrown value - // carrying statusCode 404) as "no releases" and fall back to the - // default branch. Other errors propagate. - if (is404(error)) { - resolvedRef = await client.getDefaultBranch(owner, repo); - } else { - throw error; - } - } - } - const resolvedSha = await client.resolveRefToSha(owner, repo, resolvedRef); - logger.debug(`Resolved ${sourceKey} -> ref=${resolvedRef} sha=${resolvedSha}`); + const { resolvedRef, resolvedSha, usedTag } = await resolveGhRef({ + rs, + client, + owner, + repo, + sourceKey, + logger, + }); // Discover skills under `skills/`. - let topLevel: Awaited>; - try { - topLevel = await client.listDirectory(owner, repo, SKILLS_REMOTE_DIR, resolvedSha); - } catch (error) { - if (is404(error)) { - logger.warn(`No skills/ directory found in ${sourceKey}. Skipping.`); - return []; - } - throw error; - } - - const skillDirs = topLevel - .filter((e) => e.type === "dir") - .map((e) => ({ name: e.name, path: e.path })); - - // Discover which subdirectories are actual skills (contain a SKILL.md). - // Resolved sequentially to avoid hammering the API for large monorepos - // beyond the FETCH_CONCURRENCY_LIMIT. - const validatedSkills: Array<{ name: string; path: string }> = []; - for (const sk of skillDirs) { - const info = await withSemaphore(semaphore, () => - client.getFileInfo(owner, repo, posix.join(sk.path, SKILL_FILE_NAME), resolvedSha), - ); - if (info) { - validatedSkills.push(sk); - } + const validatedSkills = await discoverValidatedSkills({ + client, + semaphore, + owner, + repo, + resolvedSha, + sourceKey, + logger, + }); + if (validatedSkills === null) { + return []; } // Apply the explicit skill filter when provided. - let selected = validatedSkills; - if (entry.skills && entry.skills.length > 0) { - const requested = new Set(entry.skills); - selected = validatedSkills.filter((s) => requested.has(s.name)); - const presentNames = new Set(validatedSkills.map((s) => s.name)); - for (const want of entry.skills) { - if (!presentNames.has(want)) { - logger.warn(`Requested skill "${want}" not found in ${sourceKey} under skills/. Skipping.`); - } - } - } + const selected = selectSkills({ validatedSkills, entry, sourceKey, logger }); // Frozen-mode coverage check (per-skill). Only enforceable now that we know // the requested skill set. if (frozen && existingLock) { - const missing: string[] = []; - for (const sk of selected) { - const locked = findGhLockInstallation(existingLock, { - source: sourceKey, - agent, - scope, - skill: sk.name, - }); - if (!locked) { - missing.push(sk.name); - } - } - if (missing.length > 0) { - throw new Error( - `Frozen install failed: rulesync-gh.lock.yaml is missing entries for ${sourceKey} (agent=${agent}, scope=${scope}) skills: ${missing.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, - ); - } + assertFrozenSkillCoverage({ selected, existingLock, sourceKey, agent, scope }); } const results: SkillInstallation[] = []; @@ -451,74 +441,23 @@ async function installSource(params: { semaphore, }); - const deployed: DeployedFile[] = []; - for (const file of allFiles) { - if (file.size > MAX_FILE_SIZE) { - logger.warn( - `Skipping "${file.path}" from ${sourceKey}: ${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - ); - continue; - } - // Path of the file relative to the skill directory root upstream. - const relativeToSkill = posix.relative(sk.path, toPosixPath(file.path)); - if ( - !relativeToSkill || - relativeToSkill.startsWith("..") || - posix.isAbsolute(relativeToSkill) - ) { - logger.warn(`Skipping "${file.path}" from ${sourceKey}: resolved outside of "${sk.path}".`); - continue; - } - - // Path under the scope root (relative). This is the value persisted to - // the lockfile. - const deployRelative = toPosixPath(join(installRelDir, sk.name, relativeToSkill)); - // Path-traversal hardening rooted at the scope root, then a tighter - // check rooted at the per-(agent,scope) install dir to refuse anything - // that escapes the agent-specific deployment directory. - checkPathTraversal({ relativePath: deployRelative, intendedRootDir: scopeRoot }); - const installAbs = join(scopeRoot, installRelDir); - const withinInstallDir = toPosixPath(join(sk.name, relativeToSkill)); - checkPathTraversal({ relativePath: withinInstallDir, intendedRootDir: installAbs }); - - let content = await withSemaphore(semaphore, () => - client.getFileContent(owner, repo, file.path, resolvedSha), - ); - const byteLength = Buffer.byteLength(content, "utf8"); - if (byteLength > MAX_FILE_SIZE) { - logger.warn( - `Skipping "${file.path}" from ${sourceKey}: fetched ${(byteLength / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, - ); - continue; - } - - // Inject provenance frontmatter into SKILL.md files. Other files - // (e.g. supporting markdown, scripts) pass through unchanged. - if (basename(file.path) === SKILL_FILE_NAME) { - try { - content = injectSourceMetadata({ - content, - source: sourceUrl, - repository, - ref: provenanceRef, - }); - } catch { - // Frontmatter exists but is not parseable. Fall back to a fresh - // prepend so we still record provenance — but warn the user. - logger.warn( - `Frontmatter in ${file.path} (${sourceKey}) is invalid. Prepending a fresh provenance block.`, - ); - content = `---\nsource: ${sourceUrl}\nrepository: ${repository}\nref: ${provenanceRef}\n---\n${content}`; - } - } - - const absolutePath = join(scopeRoot, deployRelative); - deployed.push({ relativeToScopeRoot: deployRelative, absolutePath, content }); - - if (!frozen) { - await writeFileContent(absolutePath, content); - } - } + const deployed = await buildSkillDeployment({ + sk, + allFiles, + client, + semaphore, + owner, + repo, + resolvedSha, + installRelDir, + scopeRoot, + sourceUrl, + repository, + provenanceRef, + sourceKey, + frozen, + logger, + }); deployed.sort((a, b) => a.relativeToScopeRoot < b.relativeToScopeRoot @@ -530,21 +469,16 @@ async function installSource(params: { const deployedFiles = deployed.map((d) => d.relativeToScopeRoot); const contentHash = computeContentHash(deployed); - // Frozen integrity check: refuse to overwrite known-good bytes with - // tampered ones when the prior content_hash matches the rulesync format. - if (frozen && locked?.content_hash) { - if (RULESYNC_CONTENT_HASH_REGEX.test(locked.content_hash)) { - if (locked.content_hash !== contentHash) { - throw new Error( - `content_hash mismatch for ${sourceKey} skill "${sk.name}" (agent=${agent}, scope=${scope}): lock=${locked.content_hash} computed=${contentHash}. Refuse to trust the deployment under --frozen.`, - ); - } - } else { - logger.debug( - `Skipping content_hash integrity check for ${sourceKey} skill "${sk.name}": recorded hash "${locked.content_hash}" was not written by rulesync.`, - ); - } - } + assertFrozenSkillIntegrity({ + frozen, + locked, + contentHash, + sourceKey, + skillName: sk.name, + agent, + scope, + logger, + }); // Under --frozen we deliberately do NOT write here even after the // integrity check passes. Writes are deferred to the top-level installGh @@ -576,6 +510,284 @@ async function installSource(params: { return results; } +/** + * Resolve the ref for a gh source. Order: explicit `entry.ref`, then the latest + * release's tag, then the default branch (when the repo has no releases). + * Returns the resolved ref, its commit SHA, and whether a release tag was used. + */ +async function resolveGhRef(params: { + rs: ResolvedSource; + client: GitHubClient; + owner: string; + repo: string; + sourceKey: string; + logger: Logger; +}): Promise<{ resolvedRef: string; resolvedSha: string; usedTag: boolean }> { + const { rs, client, owner, repo, sourceKey, logger } = params; + let resolvedRef: string; + let usedTag = false; + if (rs.ref) { + resolvedRef = rs.ref; + } else { + try { + const release = await client.getLatestRelease(owner, repo); + resolvedRef = release.tag_name; + usedTag = true; + } catch (error) { + // gh's behavior: when a repo has no releases, getLatestRelease returns + // 404. We treat any 404 (real GitHubClientError or any thrown value + // carrying statusCode 404) as "no releases" and fall back to the + // default branch. Other errors propagate. + if (is404(error)) { + resolvedRef = await client.getDefaultBranch(owner, repo); + } else { + throw error; + } + } + } + const resolvedSha = await client.resolveRefToSha(owner, repo, resolvedRef); + logger.debug(`Resolved ${sourceKey} -> ref=${resolvedRef} sha=${resolvedSha}`); + return { resolvedRef, resolvedSha, usedTag }; +} + +/** + * List `skills/` and validate which subdirectories are actual skills (contain a + * SKILL.md). Returns null (with a warn log) when the `skills/` directory 404s so + * the caller can skip the source. Validation is sequential to avoid hammering + * the API for large monorepos beyond FETCH_CONCURRENCY_LIMIT. + */ +async function discoverValidatedSkills(params: { + client: GitHubClient; + semaphore: Semaphore; + owner: string; + repo: string; + resolvedSha: string; + sourceKey: string; + logger: Logger; +}): Promise | null> { + const { client, semaphore, owner, repo, resolvedSha, sourceKey, logger } = params; + let topLevel: Awaited>; + try { + topLevel = await client.listDirectory(owner, repo, SKILLS_REMOTE_DIR, resolvedSha); + } catch (error) { + if (is404(error)) { + logger.warn(`No skills/ directory found in ${sourceKey}. Skipping.`); + return null; + } + throw error; + } + + const skillDirs = topLevel + .filter((e) => e.type === "dir") + .map((e) => ({ name: e.name, path: e.path })); + + const validatedSkills: Array<{ name: string; path: string }> = []; + for (const sk of skillDirs) { + const info = await withSemaphore(semaphore, () => + client.getFileInfo(owner, repo, posix.join(sk.path, SKILL_FILE_NAME), resolvedSha), + ); + if (info) { + validatedSkills.push(sk); + } + } + return validatedSkills; +} + +/** + * Apply the explicit `entry.skills` filter to the validated skills, warning for + * each requested name that is absent upstream. Returns all validated skills when + * no filter is provided. + */ +function selectSkills(params: { + validatedSkills: Array<{ name: string; path: string }>; + entry: SourceEntry; + sourceKey: string; + logger: Logger; +}): Array<{ name: string; path: string }> { + const { validatedSkills, entry, sourceKey, logger } = params; + if (!entry.skills || entry.skills.length === 0) { + return validatedSkills; + } + const requested = new Set(entry.skills); + const selected = validatedSkills.filter((s) => requested.has(s.name)); + const presentNames = new Set(validatedSkills.map((s) => s.name)); + for (const want of entry.skills) { + if (!presentNames.has(want)) { + logger.warn(`Requested skill "${want}" not found in ${sourceKey} under skills/. Skipping.`); + } + } + return selected; +} + +/** + * Frozen-mode per-skill coverage check. Throws when any selected skill has no + * matching lock installation for the (source, agent, scope) tuple. + */ +function assertFrozenSkillCoverage(params: { + selected: Array<{ name: string; path: string }>; + existingLock: GhLock; + sourceKey: string; + agent: GhAgent; + scope: GhScope; +}): void { + const { selected, existingLock, sourceKey, agent, scope } = params; + const missing: string[] = []; + for (const sk of selected) { + const locked = findGhLockInstallation(existingLock, { + source: sourceKey, + agent, + scope, + skill: sk.name, + }); + if (!locked) { + missing.push(sk.name); + } + } + if (missing.length > 0) { + throw new Error( + `Frozen install failed: rulesync-gh.lock.yaml is missing entries for ${sourceKey} (agent=${agent}, scope=${scope}) skills: ${missing.join(", ")}. Run 'rulesync install --mode gh' to update the lockfile.`, + ); + } +} + +/** + * Fetch, validate, and (under non-frozen) write a single skill's file tree, + * returning the deployable files. Oversized or out-of-bounds files are skipped + * with a warn log; SKILL.md files have provenance frontmatter injected. + */ +async function buildSkillDeployment(params: { + sk: { name: string; path: string }; + allFiles: Awaited>; + client: GitHubClient; + semaphore: Semaphore; + owner: string; + repo: string; + resolvedSha: string; + installRelDir: string; + scopeRoot: string; + sourceUrl: string; + repository: string; + provenanceRef: string; + sourceKey: string; + frozen: boolean; + logger: Logger; +}): Promise { + const { + sk, + allFiles, + client, + semaphore, + owner, + repo, + resolvedSha, + installRelDir, + scopeRoot, + sourceUrl, + repository, + provenanceRef, + sourceKey, + frozen, + logger, + } = params; + + const deployed: DeployedFile[] = []; + for (const file of allFiles) { + if (file.size > MAX_FILE_SIZE) { + logger.warn( + `Skipping "${file.path}" from ${sourceKey}: ${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, + ); + continue; + } + // Path of the file relative to the skill directory root upstream. + const relativeToSkill = posix.relative(sk.path, toPosixPath(file.path)); + if (!relativeToSkill || relativeToSkill.startsWith("..") || posix.isAbsolute(relativeToSkill)) { + logger.warn(`Skipping "${file.path}" from ${sourceKey}: resolved outside of "${sk.path}".`); + continue; + } + + // Path under the scope root (relative). This is the value persisted to + // the lockfile. + const deployRelative = toPosixPath(join(installRelDir, sk.name, relativeToSkill)); + // Path-traversal hardening rooted at the scope root, then a tighter + // check rooted at the per-(agent,scope) install dir to refuse anything + // that escapes the agent-specific deployment directory. + checkPathTraversal({ relativePath: deployRelative, intendedRootDir: scopeRoot }); + const installAbs = join(scopeRoot, installRelDir); + const withinInstallDir = toPosixPath(join(sk.name, relativeToSkill)); + checkPathTraversal({ relativePath: withinInstallDir, intendedRootDir: installAbs }); + + let content = await withSemaphore(semaphore, () => + client.getFileContent(owner, repo, file.path, resolvedSha), + ); + const byteLength = Buffer.byteLength(content, "utf8"); + if (byteLength > MAX_FILE_SIZE) { + logger.warn( + `Skipping "${file.path}" from ${sourceKey}: fetched ${(byteLength / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit.`, + ); + continue; + } + + // Inject provenance frontmatter into SKILL.md files. Other files + // (e.g. supporting markdown, scripts) pass through unchanged. + if (basename(file.path) === SKILL_FILE_NAME) { + try { + content = injectSourceMetadata({ + content, + source: sourceUrl, + repository, + ref: provenanceRef, + }); + } catch { + // Frontmatter exists but is not parseable. Fall back to a fresh + // prepend so we still record provenance — but warn the user. + logger.warn( + `Frontmatter in ${file.path} (${sourceKey}) is invalid. Prepending a fresh provenance block.`, + ); + content = `---\nsource: ${sourceUrl}\nrepository: ${repository}\nref: ${provenanceRef}\n---\n${content}`; + } + } + + const absolutePath = join(scopeRoot, deployRelative); + deployed.push({ relativeToScopeRoot: deployRelative, absolutePath, content }); + + if (!frozen) { + await writeFileContent(absolutePath, content); + } + } + return deployed; +} + +/** + * Frozen integrity check: refuse to overwrite known-good bytes with tampered + * ones when the prior content_hash matches the rulesync format. Hashes not + * written by rulesync are skipped (debug-logged), preserving the commit-SHA pin. + */ +function assertFrozenSkillIntegrity(params: { + frozen: boolean; + locked: GhLockInstallation | undefined; + contentHash: string; + sourceKey: string; + skillName: string; + agent: GhAgent; + scope: GhScope; + logger: Logger; +}): void { + const { frozen, locked, contentHash, sourceKey, skillName, agent, scope, logger } = params; + if (frozen && locked?.content_hash) { + if (RULESYNC_CONTENT_HASH_REGEX.test(locked.content_hash)) { + if (locked.content_hash !== contentHash) { + throw new Error( + `content_hash mismatch for ${sourceKey} skill "${skillName}" (agent=${agent}, scope=${scope}): lock=${locked.content_hash} computed=${contentHash}. Refuse to trust the deployment under --frozen.`, + ); + } + } else { + logger.debug( + `Skipping content_hash integrity check for ${sourceKey} skill "${skillName}": recorded hash "${locked.content_hash}" was not written by rulesync.`, + ); + } + } +} + async function removeStaleFile(params: { relativePath: string; scope: GhScope; diff --git a/src/lib/sources.ts b/src/lib/sources.ts index 80e3dc128..9033b1e58 100644 --- a/src/lib/sources.ts +++ b/src/lib/sources.ts @@ -9,6 +9,7 @@ import { RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATH, } from "../constants/rulesync-paths.js"; import { getLocalSkillDirNames } from "../features/skills/skills-utils.js"; +import type { GitHubFileEntry, ParsedSource } from "../types/fetch.js"; import { formatError } from "../utils/error.js"; import { checkPathTraversal, @@ -91,19 +92,7 @@ export async function resolveAndFetchSources(params: { // Frozen mode: validate lockfile covers all declared sources. // Missing curated skills are fetched using locked refs. if (options.frozen) { - const missingKeys: string[] = []; - - for (const source of sources) { - const locked = getLockedSource(lock, source.source); - if (!locked) { - missingKeys.push(source.source); - } - } - if (missingKeys.length > 0) { - throw new Error( - `Frozen install failed: lockfile is missing entries for: ${missingKeys.join(", ")}. Run 'rulesync install' to update the lockfile.`, - ); - } + assertFrozenLockCoversSources({ lock, sources }); } const originalLockJson = JSON.stringify(lock); @@ -120,31 +109,17 @@ export async function resolveAndFetchSources(params: { for (const sourceEntry of sources) { try { - const transport = sourceEntry.transport ?? "github"; - let result: { skillCount: number; fetchedSkillNames: string[]; updatedLock: SourcesLock }; - if (transport === "git") { - result = await fetchSourceViaGit({ - sourceEntry, - projectRoot, - lock, - localSkillNames, - alreadyFetchedSkillNames: allFetchedSkillNames, - updateSources: options.updateSources ?? false, - frozen: options.frozen ?? false, - logger, - }); - } else { - result = await fetchSource({ - sourceEntry, - client, - projectRoot, - lock, - localSkillNames, - alreadyFetchedSkillNames: allFetchedSkillNames, - updateSources: options.updateSources ?? false, - logger, - }); - } + const result = await fetchSourceByTransport({ + sourceEntry, + client, + projectRoot, + lock, + localSkillNames, + alreadyFetchedSkillNames: allFetchedSkillNames, + updateSources: options.updateSources ?? false, + frozen: options.frozen ?? false, + logger, + }); const { skillCount, fetchedSkillNames, updatedLock } = result; lock = updatedLock; @@ -162,17 +137,7 @@ export async function resolveAndFetchSources(params: { } } - // Prune stale lockfile entries whose keys are not in the current sources (immutable) - const sourceKeys = new Set(sources.map((s) => normalizeSourceKey(s.source))); - const prunedSources: typeof lock.sources = {}; - for (const [key, value] of Object.entries(lock.sources)) { - if (sourceKeys.has(normalizeSourceKey(key))) { - prunedSources[key] = value; - } else { - logger.debug(`Pruned stale lockfile entry: ${key}`); - } - } - lock = { lockfileVersion: lock.lockfileVersion, sources: prunedSources }; + lock = pruneStaleLockEntries({ lock, sources, logger }); // Only write lockfile if it has changed (and not in frozen mode) if (!options.frozen && JSON.stringify(lock) !== originalLockJson) { @@ -196,6 +161,103 @@ function logGitClientHints(params: { error: GitClientError; logger: Logger }): v } } +/** + * Frozen mode: validate the lockfile covers every declared source. Throws with + * remediation guidance listing any uncovered source keys. + */ +function assertFrozenLockCoversSources(params: { + lock: SourcesLock; + sources: SourceEntry[]; +}): void { + const { lock, sources } = params; + const missingKeys: string[] = []; + + for (const source of sources) { + const locked = getLockedSource(lock, source.source); + if (!locked) { + missingKeys.push(source.source); + } + } + if (missingKeys.length > 0) { + throw new Error( + `Frozen install failed: lockfile is missing entries for: ${missingKeys.join(", ")}. Run 'rulesync install' to update the lockfile.`, + ); + } +} + +/** + * Dispatch a single source to the transport-specific fetcher (git CLI vs. + * GitHub REST API), preserving the original default of "github". + */ +async function fetchSourceByTransport(params: { + sourceEntry: SourceEntry; + client: GitHubClient; + projectRoot: string; + lock: SourcesLock; + localSkillNames: Set; + alreadyFetchedSkillNames: Set; + updateSources: boolean; + frozen: boolean; + logger: Logger; +}): Promise<{ skillCount: number; fetchedSkillNames: string[]; updatedLock: SourcesLock }> { + const { + sourceEntry, + client, + projectRoot, + lock, + localSkillNames, + alreadyFetchedSkillNames, + updateSources, + frozen, + logger, + } = params; + const transport = sourceEntry.transport ?? "github"; + if (transport === "git") { + return fetchSourceViaGit({ + sourceEntry, + projectRoot, + lock, + localSkillNames, + alreadyFetchedSkillNames, + updateSources, + frozen, + logger, + }); + } + return fetchSource({ + sourceEntry, + client, + projectRoot, + lock, + localSkillNames, + alreadyFetchedSkillNames, + updateSources, + logger, + }); +} + +/** + * Prune stale lockfile entries whose keys are not in the current sources + * (immutable — returns a fresh lock object). + */ +function pruneStaleLockEntries(params: { + lock: SourcesLock; + sources: SourceEntry[]; + logger: Logger; +}): SourcesLock { + const { lock, sources, logger } = params; + const sourceKeys = new Set(sources.map((s) => normalizeSourceKey(s.source))); + const prunedSources: typeof lock.sources = {}; + for (const [key, value] of Object.entries(lock.sources)) { + if (sourceKeys.has(normalizeSourceKey(key))) { + prunedSources[key] = value; + } else { + logger.debug(`Pruned stale lockfile entry: ${key}`); + } + } + return { lockfileVersion: lock.lockfileVersion, sources: prunedSources }; +} + /** * Check if all locked skills exist on disk in the curated directory. */ @@ -412,6 +474,290 @@ function groupRemoteFilesBySkillRoot(params: { // Transport-specific fetch functions // --------------------------------------------------------------------------- +/** + * Resolve a GitHub source's ref to a commit SHA, preferring the locked SHA for + * deterministic fetches and otherwise resolving the declared ref or default + * branch. Returns the on-disk `ref` (SHA when freshly resolved, else locked + * ref), the resolved SHA, and the requested ref. + */ +async function resolveGithubFetchRef(params: { + parsed: ParsedSource; + locked: LockedSource | undefined; + updateSources: boolean; + sourceKey: string; + client: GitHubClient; + logger: Logger; +}): Promise<{ ref: string; resolvedSha: string; requestedRef: string | undefined }> { + const { parsed, locked, updateSources, sourceKey, client, logger } = params; + if (locked && !updateSources) { + // Use the locked SHA for deterministic fetching + logger.debug(`Using locked ref for ${sourceKey}: ${locked.resolvedRef}`); + return { + ref: locked.resolvedRef, + resolvedSha: locked.resolvedRef, + requestedRef: locked.requestedRef, + }; + } + // Resolve the ref (or default branch) to a SHA + const requestedRef = parsed.ref ?? (await client.getDefaultBranch(parsed.owner, parsed.repo)); + const resolvedSha = await client.resolveRefToSha(parsed.owner, parsed.repo, requestedRef); + logger.debug(`Resolved ${sourceKey} ref "${requestedRef}" to SHA: ${resolvedSha}`); + return { ref: resolvedSha, resolvedSha, requestedRef }; +} + +/** + * Fallback path used when the skills directory has no subdirectories but does + * contain a single flat skill (root-level files). Fetches and writes that skill + * into `fetchedSkills`. Returns whether the fallback fired and the resulting + * remote skill names. + */ +async function fetchRootLevelFallbackSkill(params: { + entries: GitHubFileEntry[]; + parsed: ParsedSource; + ref: string; + resolvedSha: string; + skillFilter: string[]; + isWildcard: boolean; + curatedDir: string; + locked: LockedSource | undefined; + sourceKey: string; + localSkillNames: Set; + alreadyFetchedSkillNames: Set; + client: GitHubClient; + semaphore: Semaphore; + fetchedSkills: Record; + logger: Logger; +}): Promise<{ handled: boolean; remoteSkillNames: string[] }> { + const { + entries, + parsed, + ref, + resolvedSha, + skillFilter, + isWildcard, + curatedDir, + locked, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + client, + semaphore, + fetchedSkills, + logger, + } = params; + + const rootFiles = entries.filter((entry) => entry.type === "file"); + const rootSkillFiles: RemoteSkillFile[] = []; + + for (const file of rootFiles) { + if (file.size > MAX_FILE_SIZE) { + logger.warn( + `Skipping file "${file.path}" (${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit).`, + ); + continue; + } + const content = await withSemaphore(semaphore, () => + client.getFileContent(parsed.owner, parsed.repo, file.path, ref), + ); + rootSkillFiles.push({ relativePath: file.name, content }); + } + + const groupedRootFiles = groupRemoteFilesBySkillRoot({ + remoteFiles: rootSkillFiles, + skillFilter, + isWildcard, + }); + const [fallbackSkillName] = groupedRootFiles.keys(); + if (fallbackSkillName === undefined) { + return { handled: false, remoteSkillNames: [] }; + } + + if ( + !shouldSkipSkill({ + skillName: fallbackSkillName, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + logger, + }) + ) { + fetchedSkills[fallbackSkillName] = await writeSkillAndComputeIntegrity({ + skillName: fallbackSkillName, + files: groupedRootFiles.get(fallbackSkillName) ?? [], + curatedDir, + locked, + resolvedSha, + sourceKey, + logger, + }); + logger.debug(`Fetched skill "${fallbackSkillName}" from ${sourceKey}`); + } + + return { handled: true, remoteSkillNames: [fallbackSkillName] }; +} + +/** + * Recursively fetch and write a single skill directory's files via the GitHub + * REST API, returning its computed LockedSkill entry. + */ +async function fetchGithubSkillDir(params: { + skillDir: { name: string; path: string }; + parsed: ParsedSource; + ref: string; + resolvedSha: string; + curatedDir: string; + locked: LockedSource | undefined; + sourceKey: string; + client: GitHubClient; + semaphore: Semaphore; + logger: Logger; +}): Promise { + const { + skillDir, + parsed, + ref, + resolvedSha, + curatedDir, + locked, + sourceKey, + client, + semaphore, + logger, + } = params; + + // Recursively fetch all files in this skill directory + const allFiles = await listDirectoryRecursive({ + client, + owner: parsed.owner, + repo: parsed.repo, + path: skillDir.path, + ref, + semaphore, + }); + + // Filter out files exceeding MAX_FILE_SIZE + const files = allFiles.filter((file) => { + if (file.size > MAX_FILE_SIZE) { + logger.warn( + `Skipping file "${file.path}" (${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit).`, + ); + return false; + } + return true; + }); + + // Fetch all file contents + const skillFiles: Array<{ relativePath: string; content: string }> = []; + for (const file of files) { + const relativeToSkill = file.path.substring(skillDir.path.length + 1); + const content = await withSemaphore(semaphore, () => + client.getFileContent(parsed.owner, parsed.repo, file.path, ref), + ); + skillFiles.push({ relativePath: relativeToSkill, content }); + } + + return writeSkillAndComputeIntegrity({ + skillName: skillDir.name, + files: skillFiles, + curatedDir, + locked, + resolvedSha, + sourceKey, + logger, + }); +} + +/** + * List the remote skills directory and apply the root-level fallback. Returns a + * `notFound` sentinel when the directory 404s (so the caller can skip the + * source), otherwise the discovered skill subdirectories plus any fallback skill + * names already written into `fetchedSkills`. + */ +async function discoverGithubSkillDirs(params: { + parsed: ParsedSource; + ref: string; + resolvedSha: string; + skillFilter: string[]; + isWildcard: boolean; + curatedDir: string; + locked: LockedSource | undefined; + sourceKey: string; + localSkillNames: Set; + alreadyFetchedSkillNames: Set; + client: GitHubClient; + semaphore: Semaphore; + fetchedSkills: Record; + logger: Logger; +}): Promise< + | { status: "notFound" } + | { + status: "ok"; + remoteSkillDirs: Array<{ name: string; path: string }>; + fallbackHandled: boolean; + remoteSkillNames: string[]; + } +> { + const { + parsed, + ref, + resolvedSha, + skillFilter, + isWildcard, + curatedDir, + locked, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + client, + semaphore, + fetchedSkills, + logger, + } = params; + + const skillsBasePath = parsed.path ?? "skills"; + try { + const entries = await client.listDirectory(parsed.owner, parsed.repo, skillsBasePath, ref); + const remoteSkillDirs = entries + .filter((e) => e.type === "dir") + .map((e) => ({ name: e.name, path: e.path })); + + if (remoteSkillDirs.length === 0 && !isWildcard && skillFilter.length === 1) { + const fallback = await fetchRootLevelFallbackSkill({ + entries, + parsed, + ref, + resolvedSha, + skillFilter, + isWildcard, + curatedDir, + locked, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + client, + semaphore, + fetchedSkills, + logger, + }); + if (fallback.handled) { + return { + status: "ok", + remoteSkillDirs, + fallbackHandled: true, + remoteSkillNames: fallback.remoteSkillNames, + }; + } + } + + return { status: "ok", remoteSkillDirs, fallbackHandled: false, remoteSkillNames: [] }; + } catch (error) { + if (error instanceof GitHubClientError && error.statusCode === 404) { + return { status: "notFound" }; + } + throw error; + } +} + /** * Fetch skills from a single source entry via the GitHub REST API. */ @@ -452,23 +798,14 @@ async function fetchSource(params: { const lockedSkillNames = locked ? getLockedSkillNames(locked) : []; // Resolve the ref to a commit SHA - let ref: string; - let resolvedSha: string; - let requestedRef: string | undefined; - - if (locked && !updateSources) { - // Use the locked SHA for deterministic fetching - ref = locked.resolvedRef; - resolvedSha = locked.resolvedRef; - requestedRef = locked.requestedRef; - logger.debug(`Using locked ref for ${sourceKey}: ${resolvedSha}`); - } else { - // Resolve the ref (or default branch) to a SHA - requestedRef = parsed.ref ?? (await client.getDefaultBranch(parsed.owner, parsed.repo)); - resolvedSha = await client.resolveRefToSha(parsed.owner, parsed.repo, requestedRef); - ref = resolvedSha; - logger.debug(`Resolved ${sourceKey} ref "${requestedRef}" to SHA: ${resolvedSha}`); - } + const { ref, resolvedSha, requestedRef } = await resolveGithubFetchRef({ + parsed, + locked, + updateSources, + sourceKey, + client, + logger, + }); const curatedDir = join(projectRoot, RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATH); @@ -494,81 +831,33 @@ async function fetchSource(params: { // List the skills/ directory in the remote repo. // If a path is given in the source URL, it points directly to the skills directory. // Otherwise, look for "skills/" at the repo root. - const skillsBasePath = parsed.path ?? "skills"; - let remoteSkillDirs: Array<{ name: string; path: string }>; - let remoteSkillNames: string[] = []; - let fallbackHandled = false; - - try { - const entries = await client.listDirectory(parsed.owner, parsed.repo, skillsBasePath, ref); - remoteSkillDirs = entries - .filter((e) => e.type === "dir") - .map((e) => ({ name: e.name, path: e.path })); - - if (remoteSkillDirs.length === 0 && !isWildcard && skillFilter.length === 1) { - const rootFiles = entries.filter((entry) => entry.type === "file"); - const rootSkillFiles: RemoteSkillFile[] = []; - - for (const file of rootFiles) { - if (file.size > MAX_FILE_SIZE) { - logger.warn( - `Skipping file "${file.path}" (${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit).`, - ); - continue; - } - const content = await withSemaphore(semaphore, () => - client.getFileContent(parsed.owner, parsed.repo, file.path, ref), - ); - rootSkillFiles.push({ relativePath: file.name, content }); - } - - const groupedRootFiles = groupRemoteFilesBySkillRoot({ - remoteFiles: rootSkillFiles, - skillFilter, - isWildcard, - }); - const [fallbackSkillName] = groupedRootFiles.keys(); - if (fallbackSkillName !== undefined) { - fallbackHandled = true; - remoteSkillNames = [fallbackSkillName]; - - if ( - !shouldSkipSkill({ - skillName: fallbackSkillName, - sourceKey, - localSkillNames, - alreadyFetchedSkillNames, - logger, - }) - ) { - fetchedSkills[fallbackSkillName] = await writeSkillAndComputeIntegrity({ - skillName: fallbackSkillName, - files: groupedRootFiles.get(fallbackSkillName) ?? [], - curatedDir, - locked, - resolvedSha, - sourceKey, - logger, - }); - logger.debug(`Fetched skill "${fallbackSkillName}" from ${sourceKey}`); - } - } - } - } catch (error) { - if (error instanceof GitHubClientError && error.statusCode === 404) { - logger.warn(`No skills/ directory found in ${sourceKey}. Skipping.`); - return { skillCount: 0, fetchedSkillNames: [], updatedLock: lock }; - } - throw error; + const discovery = await discoverGithubSkillDirs({ + parsed, + ref, + resolvedSha, + skillFilter, + isWildcard, + curatedDir, + locked, + sourceKey, + localSkillNames, + alreadyFetchedSkillNames, + client, + semaphore, + fetchedSkills, + logger, + }); + if (discovery.status === "notFound") { + logger.warn(`No skills/ directory found in ${sourceKey}. Skipping.`); + return { skillCount: 0, fetchedSkillNames: [], updatedLock: lock }; } + const { remoteSkillDirs, fallbackHandled, remoteSkillNames: fallbackSkillNames } = discovery; // Filter skills by name const filteredDirs = isWildcard ? remoteSkillDirs : remoteSkillDirs.filter((d) => skillFilter.includes(d.name)); - if (!fallbackHandled) { - remoteSkillNames = filteredDirs.map((d) => d.name); - } + const remoteSkillNames = fallbackHandled ? fallbackSkillNames : filteredDirs.map((d) => d.name); if (locked) { await cleanPreviousCuratedSkills({ curatedDir, lockedSkillNames, logger }); @@ -587,44 +876,16 @@ async function fetchSource(params: { continue; } - // Recursively fetch all files in this skill directory - const allFiles = await listDirectoryRecursive({ - client, - owner: parsed.owner, - repo: parsed.repo, - path: skillDir.path, + fetchedSkills[skillDir.name] = await fetchGithubSkillDir({ + skillDir, + parsed, ref, - semaphore, - }); - - // Filter out files exceeding MAX_FILE_SIZE - const files = allFiles.filter((file) => { - if (file.size > MAX_FILE_SIZE) { - logger.warn( - `Skipping file "${file.path}" (${(file.size / 1024 / 1024).toFixed(2)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit).`, - ); - return false; - } - return true; - }); - - // Fetch all file contents - const skillFiles: Array<{ relativePath: string; content: string }> = []; - for (const file of files) { - const relativeToSkill = file.path.substring(skillDir.path.length + 1); - const content = await withSemaphore(semaphore, () => - client.getFileContent(parsed.owner, parsed.repo, file.path, ref), - ); - skillFiles.push({ relativePath: relativeToSkill, content }); - } - - fetchedSkills[skillDir.name] = await writeSkillAndComputeIntegrity({ - skillName: skillDir.name, - files: skillFiles, + resolvedSha, curatedDir, locked, - resolvedSha, sourceKey, + client, + semaphore, logger, }); logger.debug(`Fetched skill "${skillDir.name}" from ${sourceKey}`); diff --git a/src/lib/update.ts b/src/lib/update.ts index de83c0f36..5a2ee9d5f 100644 --- a/src/lib/update.ts +++ b/src/lib/update.ts @@ -297,21 +297,14 @@ export type UpdateOptions = { }; /** - * Perform the binary update + * Resolve the platform binary asset and the mandatory SHA256SUMS asset from a + * release, throwing with manual-download guidance when either is unavailable. */ -export async function performBinaryUpdate( - currentVersion: string, - options: UpdateOptions = {}, -): Promise { - const { force = false, token } = options; - - // Check for updates - const updateCheck = await checkForUpdate(currentVersion, token); - - if (!updateCheck.hasUpdate && !force) { - return `Already at the latest version (${currentVersion})`; - } - +function resolveUpdateAssets(release: GitHubRelease): { + assetName: string; + binaryAsset: GitHubReleaseAsset; + checksumAsset: GitHubReleaseAsset; +} { // Get platform-specific asset name const assetName = getPlatformAssetName(); if (!assetName) { @@ -321,7 +314,7 @@ export async function performBinaryUpdate( } // Find the binary asset - const binaryAsset = findAsset(updateCheck.release, assetName); + const binaryAsset = findAsset(release, assetName); if (!binaryAsset) { throw new Error( `Binary for ${assetName} not found in release. Please download manually from ${RELEASES_URL}`, @@ -329,109 +322,211 @@ export async function performBinaryUpdate( } // Find the SHA256SUMS asset for verification (mandatory) - const checksumAsset = findAsset(updateCheck.release, "SHA256SUMS"); + const checksumAsset = findAsset(release, "SHA256SUMS"); if (!checksumAsset) { throw new Error( `SHA256SUMS not found in release. Cannot verify download integrity. Please download manually from ${RELEASES_URL}`, ); } - // Create temporary directory for download - const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "rulesync-update-")); - let restoreFailed = false; + return { assetName, binaryAsset, checksumAsset }; +} +/** + * Download the binary and SHA256SUMS into the temp directory, then verify the + * binary's checksum. Throws when the checksum entry is missing or mismatched. + */ +async function downloadAndVerifyBinary(params: { + tempDir: string; + assetName: string; + binaryAsset: GitHubReleaseAsset; + checksumAsset: GitHubReleaseAsset; +}): Promise { + const { tempDir, assetName, binaryAsset, checksumAsset } = params; + const tempBinaryPath = path.join(tempDir, assetName); + + // Download the binary + await downloadFile(binaryAsset.browser_download_url, tempBinaryPath); + + // Verify checksum (mandatory) + const checksumsPath = path.join(tempDir, "SHA256SUMS"); + await downloadFile(checksumAsset.browser_download_url, checksumsPath); + + const checksumsContent = await fs.promises.readFile(checksumsPath, "utf-8"); + const checksums = parseSha256Sums(checksumsContent); + const expectedChecksum = checksums.get(assetName); + + if (!expectedChecksum) { + throw new Error( + `Checksum entry for "${assetName}" not found in SHA256SUMS. Cannot verify download integrity.`, + ); + } + + const actualChecksum = await calculateSha256(tempBinaryPath); + if (actualChecksum !== expectedChecksum) { + throw new Error( + `Checksum verification failed. Expected: ${expectedChecksum}, Got: ${actualChecksum}. The download may be corrupted.`, + ); + } + + return tempBinaryPath; +} + +/** + * Replace the running executable at `currentExePath` with the verified binary, + * preferring an atomic rename and falling back to a direct cross-filesystem copy. + */ +async function replaceCurrentBinary(params: { + tempBinaryPath: string; + currentExePath: string; + currentDir: string; +}): Promise { + const { tempBinaryPath, currentExePath, currentDir } = params; + // Attempt atomic replacement via rename (works when on the same filesystem) + const tempInPlace = path.join(currentDir, `.rulesync-update-${crypto.randomUUID()}`); try { - // Set restrictive permissions on temp directory (Unix only) + await fs.promises.copyFile(tempBinaryPath, tempInPlace); if (os.platform() !== "win32") { - await fs.promises.chmod(tempDir, 0o700); + await fs.promises.chmod(tempInPlace, 0o755); } + await fs.promises.rename(tempInPlace, currentExePath); + } catch { + // Cleanup temp-in-place file on failure, then fall back to direct copy + try { + await fs.promises.unlink(tempInPlace); + } catch { + // Ignore cleanup errors + } + // Fallback: direct copy (non-atomic but works across filesystems) + await fs.promises.copyFile(tempBinaryPath, currentExePath); + if (os.platform() !== "win32") { + await fs.promises.chmod(currentExePath, 0o755); + } + } +} - const tempBinaryPath = path.join(tempDir, assetName); - - // Download the binary - await downloadFile(binaryAsset.browser_download_url, tempBinaryPath); - - // Verify checksum (mandatory) - const checksumsPath = path.join(tempDir, "SHA256SUMS"); - await downloadFile(checksumAsset.browser_download_url, checksumsPath); +/** + * Install the verified binary over the current executable, backing it up first + * and restoring from backup on failure. Returns whether the restore failed so + * the caller can preserve the temp directory for manual recovery. + */ +async function installVerifiedBinary(params: { + tempDir: string; + tempBinaryPath: string; + currentVersion: string; + latestVersion: string; +}): Promise<{ message: string; restoreFailed: boolean }> { + const { tempDir, tempBinaryPath, currentVersion, latestVersion } = params; - const checksumsContent = await fs.promises.readFile(checksumsPath, "utf-8"); - const checksums = parseSha256Sums(checksumsContent); - const expectedChecksum = checksums.get(assetName); + // Resolve symlinks to get the real executable path + const currentExePath = await fs.promises.realpath(process.execPath); + const currentDir = path.dirname(currentExePath); - if (!expectedChecksum) { - throw new Error( - `Checksum entry for "${assetName}" not found in SHA256SUMS. Cannot verify download integrity.`, + // Backup current binary to temp directory (not predictable path) + const backupPath = path.join(tempDir, "rulesync.backup"); + try { + await fs.promises.copyFile(currentExePath, backupPath); + } catch (error) { + if (isPermissionError(error)) { + throw new UpdatePermissionError( + `Permission denied: Cannot read ${currentExePath}. Try running with sudo.`, ); } + throw error; + } - const actualChecksum = await calculateSha256(tempBinaryPath); - if (actualChecksum !== expectedChecksum) { - throw new Error( - `Checksum verification failed. Expected: ${expectedChecksum}, Got: ${actualChecksum}. The download may be corrupted.`, + try { + await replaceCurrentBinary({ tempBinaryPath, currentExePath, currentDir }); + return { + message: `Successfully updated from ${currentVersion} to ${latestVersion}`, + restoreFailed: false, + }; + } catch (error) { + // Restore from backup on failure + try { + await fs.promises.copyFile(backupPath, currentExePath); + } catch { + throw new RestoreFailedError( + new Error( + `Failed to replace binary and restore failed. Backup is preserved at: ${backupPath} (in ${tempDir}). ` + + `Please manually copy it to ${currentExePath}. Original error: ${error instanceof Error ? error.message : String(error)}`, + { cause: error }, + ), ); } + if (isPermissionError(error)) { + throw new UpdatePermissionError( + `Permission denied: Cannot write to ${path.dirname(currentExePath)}. Try running with sudo.`, + ); + } + throw error; + } +} - // Resolve symlinks to get the real executable path - const currentExePath = await fs.promises.realpath(process.execPath); - const currentDir = path.dirname(currentExePath); +/** + * Internal marker wrapping the error thrown when both the binary replacement + * and the backup restore fail. The caller unwraps it so the temp directory is + * preserved for manual recovery (mirrors the original inline `restoreFailed` + * flag behavior). + */ +class RestoreFailedError extends Error { + override readonly cause: Error; + constructor(cause: Error) { + super(cause.message); + this.name = "RestoreFailedError"; + this.cause = cause; + } +} - // Backup current binary to temp directory (not predictable path) - const backupPath = path.join(tempDir, "rulesync.backup"); - try { - await fs.promises.copyFile(currentExePath, backupPath); - } catch (error) { - if (isPermissionError(error)) { - throw new UpdatePermissionError( - `Permission denied: Cannot read ${currentExePath}. Try running with sudo.`, - ); - } - throw error; - } +/** + * Perform the binary update + */ +export async function performBinaryUpdate( + currentVersion: string, + options: UpdateOptions = {}, +): Promise { + const { force = false, token } = options; - try { - // Attempt atomic replacement via rename (works when on the same filesystem) - const tempInPlace = path.join(currentDir, `.rulesync-update-${crypto.randomUUID()}`); - try { - await fs.promises.copyFile(tempBinaryPath, tempInPlace); - if (os.platform() !== "win32") { - await fs.promises.chmod(tempInPlace, 0o755); - } - await fs.promises.rename(tempInPlace, currentExePath); - } catch { - // Cleanup temp-in-place file on failure, then fall back to direct copy - try { - await fs.promises.unlink(tempInPlace); - } catch { - // Ignore cleanup errors - } - // Fallback: direct copy (non-atomic but works across filesystems) - await fs.promises.copyFile(tempBinaryPath, currentExePath); - if (os.platform() !== "win32") { - await fs.promises.chmod(currentExePath, 0o755); - } - } + // Check for updates + const updateCheck = await checkForUpdate(currentVersion, token); - return `Successfully updated from ${currentVersion} to ${updateCheck.latestVersion}`; - } catch (error) { - // Restore from backup on failure - try { - await fs.promises.copyFile(backupPath, currentExePath); - } catch { - restoreFailed = true; - throw new Error( - `Failed to replace binary and restore failed. Backup is preserved at: ${backupPath} (in ${tempDir}). ` + - `Please manually copy it to ${currentExePath}. Original error: ${error instanceof Error ? error.message : String(error)}`, - { cause: error }, - ); - } - if (isPermissionError(error)) { - throw new UpdatePermissionError( - `Permission denied: Cannot write to ${path.dirname(currentExePath)}. Try running with sudo.`, - ); - } - throw error; + if (!updateCheck.hasUpdate && !force) { + return `Already at the latest version (${currentVersion})`; + } + + const { assetName, binaryAsset, checksumAsset } = resolveUpdateAssets(updateCheck.release); + + // Create temporary directory for download + const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "rulesync-update-")); + let restoreFailed = false; + + try { + // Set restrictive permissions on temp directory (Unix only) + if (os.platform() !== "win32") { + await fs.promises.chmod(tempDir, 0o700); + } + + const tempBinaryPath = await downloadAndVerifyBinary({ + tempDir, + assetName, + binaryAsset, + checksumAsset, + }); + + const installed = await installVerifiedBinary({ + tempDir, + tempBinaryPath, + currentVersion, + latestVersion: updateCheck.latestVersion, + }); + restoreFailed = installed.restoreFailed; + return installed.message; + } catch (error) { + if (error instanceof RestoreFailedError) { + restoreFailed = true; + throw error.cause; } + throw error; } finally { // Skip cleanup if restore failed, so the backup is preserved for manual recovery if (!restoreFailed) { diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 4a7e0fb36..91279c2e8 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -182,6 +182,214 @@ function ensureBody({ body, feature, operation }: RulesyncToolArgs): string { return body; } +function requireContent({ + content, + feature, +}: { + content: string | undefined; + feature: string; +}): string { + if (!content) { + throw new Error(`content is required for ${feature} put operation`); + } + + return content; +} + +function executeRule(parsed: RulesyncToolArgs) { + if (parsed.operation === "list") { + return ruleTools.listRules.execute(); + } + + if (parsed.operation === "get") { + return ruleTools.getRule.execute({ relativePathFromCwd: requireTargetPath(parsed) }); + } + + if (parsed.operation === "put") { + return ruleTools.putRule.execute({ + relativePathFromCwd: requireTargetPath(parsed), + frontmatter: parseFrontmatter({ + feature: "rule", + frontmatter: parsed.frontmatter ?? {}, + }), + body: ensureBody(parsed), + }); + } + + return ruleTools.deleteRule.execute({ relativePathFromCwd: requireTargetPath(parsed) }); +} + +function executeCommand(parsed: RulesyncToolArgs) { + if (parsed.operation === "list") { + return commandTools.listCommands.execute(); + } + + if (parsed.operation === "get") { + return commandTools.getCommand.execute({ + relativePathFromCwd: requireTargetPath(parsed), + }); + } + + if (parsed.operation === "put") { + return commandTools.putCommand.execute({ + relativePathFromCwd: requireTargetPath(parsed), + frontmatter: parseFrontmatter({ + feature: "command", + frontmatter: parsed.frontmatter ?? {}, + }), + body: ensureBody(parsed), + }); + } + + return commandTools.deleteCommand.execute({ + relativePathFromCwd: requireTargetPath(parsed), + }); +} + +function executeSubagent(parsed: RulesyncToolArgs) { + if (parsed.operation === "list") { + return subagentTools.listSubagents.execute(); + } + + if (parsed.operation === "get") { + return subagentTools.getSubagent.execute({ + relativePathFromCwd: requireTargetPath(parsed), + }); + } + + if (parsed.operation === "put") { + return subagentTools.putSubagent.execute({ + relativePathFromCwd: requireTargetPath(parsed), + frontmatter: parseFrontmatter({ + feature: "subagent", + frontmatter: parsed.frontmatter ?? {}, + }), + body: ensureBody(parsed), + }); + } + + return subagentTools.deleteSubagent.execute({ + relativePathFromCwd: requireTargetPath(parsed), + }); +} + +function executeSkill(parsed: RulesyncToolArgs) { + if (parsed.operation === "list") { + return skillTools.listSkills.execute(); + } + + if (parsed.operation === "get") { + return skillTools.getSkill.execute({ relativeDirPathFromCwd: requireTargetPath(parsed) }); + } + + if (parsed.operation === "put") { + return skillTools.putSkill.execute({ + relativeDirPathFromCwd: requireTargetPath(parsed), + frontmatter: parseFrontmatter({ + feature: "skill", + frontmatter: parsed.frontmatter ?? {}, + }), + body: ensureBody(parsed), + otherFiles: parsed.otherFiles ?? [], + }); + } + + return skillTools.deleteSkill.execute({ + relativeDirPathFromCwd: requireTargetPath(parsed), + }); +} + +function executeIgnore(parsed: RulesyncToolArgs) { + if (parsed.operation === "get") { + return ignoreTools.getIgnoreFile.execute(); + } + + if (parsed.operation === "put") { + return ignoreTools.putIgnoreFile.execute({ + content: requireContent({ content: parsed.content, feature: "ignore" }), + }); + } + + return ignoreTools.deleteIgnoreFile.execute(); +} + +function executeMcp(parsed: RulesyncToolArgs) { + if (parsed.operation === "get") { + return mcpTools.getMcpFile.execute(); + } + + if (parsed.operation === "put") { + return mcpTools.putMcpFile.execute({ + content: requireContent({ content: parsed.content, feature: "mcp" }), + }); + } + + return mcpTools.deleteMcpFile.execute(); +} + +function executePermissions(parsed: RulesyncToolArgs) { + if (parsed.operation === "get") { + return permissionsTools.getPermissionsFile.execute(); + } + + if (parsed.operation === "put") { + return permissionsTools.putPermissionsFile.execute({ + content: requireContent({ content: parsed.content, feature: "permissions" }), + }); + } + + return permissionsTools.deletePermissionsFile.execute(); +} + +function executeHooks(parsed: RulesyncToolArgs) { + if (parsed.operation === "get") { + return hooksTools.getHooksFile.execute(); + } + + if (parsed.operation === "put") { + return hooksTools.putHooksFile.execute({ + content: requireContent({ content: parsed.content, feature: "hooks" }), + }); + } + + return hooksTools.deleteHooksFile.execute(); +} + +function executeGenerate(parsed: RulesyncToolArgs) { + // Only "run" operation is supported for generate feature + return generateTools.executeGenerate.execute(parsed.generateOptions ?? {}); +} + +function executeImport(parsed: RulesyncToolArgs) { + // Only "run" operation is supported for import feature + if (!parsed.importOptions) { + throw new Error("importOptions is required for import feature"); + } + return importTools.executeImport.execute(parsed.importOptions); +} + +function executeConvert(parsed: RulesyncToolArgs) { + // Only "run" operation is supported for convert feature + if (!parsed.convertOptions) { + throw new Error("convertOptions is required for convert feature"); + } + return convertTools.executeConvert.execute(parsed.convertOptions); +} + +const featureExecutors: Record Promise> = { + rule: executeRule, + command: executeCommand, + subagent: executeSubagent, + skill: executeSkill, + ignore: executeIgnore, + mcp: executeMcp, + permissions: executePermissions, + hooks: executeHooks, + generate: executeGenerate, + import: executeImport, + convert: executeConvert, +}; + export const rulesyncTool = { name: "rulesyncTool", description: @@ -192,187 +400,11 @@ export const rulesyncTool = { assertSupported({ feature: parsed.feature, operation: parsed.operation }); - switch (parsed.feature) { - case "rule": { - if (parsed.operation === "list") { - return ruleTools.listRules.execute(); - } - - if (parsed.operation === "get") { - return ruleTools.getRule.execute({ relativePathFromCwd: requireTargetPath(parsed) }); - } - - if (parsed.operation === "put") { - return ruleTools.putRule.execute({ - relativePathFromCwd: requireTargetPath(parsed), - frontmatter: parseFrontmatter({ - feature: "rule", - frontmatter: parsed.frontmatter ?? {}, - }), - body: ensureBody(parsed), - }); - } - - return ruleTools.deleteRule.execute({ relativePathFromCwd: requireTargetPath(parsed) }); - } - case "command": { - if (parsed.operation === "list") { - return commandTools.listCommands.execute(); - } - - if (parsed.operation === "get") { - return commandTools.getCommand.execute({ - relativePathFromCwd: requireTargetPath(parsed), - }); - } - - if (parsed.operation === "put") { - return commandTools.putCommand.execute({ - relativePathFromCwd: requireTargetPath(parsed), - frontmatter: parseFrontmatter({ - feature: "command", - frontmatter: parsed.frontmatter ?? {}, - }), - body: ensureBody(parsed), - }); - } - - return commandTools.deleteCommand.execute({ - relativePathFromCwd: requireTargetPath(parsed), - }); - } - case "subagent": { - if (parsed.operation === "list") { - return subagentTools.listSubagents.execute(); - } - - if (parsed.operation === "get") { - return subagentTools.getSubagent.execute({ - relativePathFromCwd: requireTargetPath(parsed), - }); - } - - if (parsed.operation === "put") { - return subagentTools.putSubagent.execute({ - relativePathFromCwd: requireTargetPath(parsed), - frontmatter: parseFrontmatter({ - feature: "subagent", - frontmatter: parsed.frontmatter ?? {}, - }), - body: ensureBody(parsed), - }); - } - - return subagentTools.deleteSubagent.execute({ - relativePathFromCwd: requireTargetPath(parsed), - }); - } - case "skill": { - if (parsed.operation === "list") { - return skillTools.listSkills.execute(); - } - - if (parsed.operation === "get") { - return skillTools.getSkill.execute({ relativeDirPathFromCwd: requireTargetPath(parsed) }); - } - - if (parsed.operation === "put") { - return skillTools.putSkill.execute({ - relativeDirPathFromCwd: requireTargetPath(parsed), - frontmatter: parseFrontmatter({ - feature: "skill", - frontmatter: parsed.frontmatter ?? {}, - }), - body: ensureBody(parsed), - otherFiles: parsed.otherFiles ?? [], - }); - } - - return skillTools.deleteSkill.execute({ - relativeDirPathFromCwd: requireTargetPath(parsed), - }); - } - case "ignore": { - if (parsed.operation === "get") { - return ignoreTools.getIgnoreFile.execute(); - } - - if (parsed.operation === "put") { - if (!parsed.content) { - throw new Error("content is required for ignore put operation"); - } - - return ignoreTools.putIgnoreFile.execute({ content: parsed.content }); - } - - return ignoreTools.deleteIgnoreFile.execute(); - } - case "mcp": { - if (parsed.operation === "get") { - return mcpTools.getMcpFile.execute(); - } - - if (parsed.operation === "put") { - if (!parsed.content) { - throw new Error("content is required for mcp put operation"); - } - - return mcpTools.putMcpFile.execute({ content: parsed.content }); - } - - return mcpTools.deleteMcpFile.execute(); - } - case "permissions": { - if (parsed.operation === "get") { - return permissionsTools.getPermissionsFile.execute(); - } - - if (parsed.operation === "put") { - if (!parsed.content) { - throw new Error("content is required for permissions put operation"); - } - - return permissionsTools.putPermissionsFile.execute({ content: parsed.content }); - } - - return permissionsTools.deletePermissionsFile.execute(); - } - case "hooks": { - if (parsed.operation === "get") { - return hooksTools.getHooksFile.execute(); - } - - if (parsed.operation === "put") { - if (!parsed.content) { - throw new Error("content is required for hooks put operation"); - } - - return hooksTools.putHooksFile.execute({ content: parsed.content }); - } - - return hooksTools.deleteHooksFile.execute(); - } - case "generate": { - // Only "run" operation is supported for generate feature - return generateTools.executeGenerate.execute(parsed.generateOptions ?? {}); - } - case "import": { - // Only "run" operation is supported for import feature - if (!parsed.importOptions) { - throw new Error("importOptions is required for import feature"); - } - return importTools.executeImport.execute(parsed.importOptions); - } - case "convert": { - // Only "run" operation is supported for convert feature - if (!parsed.convertOptions) { - throw new Error("convertOptions is required for convert feature"); - } - return convertTools.executeConvert.execute(parsed.convertOptions); - } - default: { - throw new Error(`Unknown feature: ${parsed.feature}`); - } + const executor = featureExecutors[parsed.feature]; + if (!executor) { + throw new Error(`Unknown feature: ${parsed.feature}`); } + + return executor(parsed); }, } as const;