refactor(cli): install content sections through one engine - #552
Merged
Conversation
Four near-identical classes installed agents, commands, rules and skills. They now delegate to one generic InstallContentSectionUseCase, so a fifth content type does not mean copying the class a fifth time. The four were not as identical as the ticket assumed. Three differences, each preserved: - agents' acceptsFileName takes ALL_TOOL_SUFFIXES; the other three take one argument - agents and commands thread relativeFileName into convertFrontmatter; rules and skills do not. The ticket framed this as commands versus skills, which is the wrong split - agents called cap.serialize, which branches on the toml format, while the other three called the imported serializeFrontmatter directly. Their own cap.serialize are pass-throughs to that same function, so calling cap.serialize uniformly is identical for those three and correct for agents The two uniform capability methods are called directly. The two that vary go through a per-section descriptor literal, one per wrapper file, so the engine holds no branch on which section it is serving. A switch there would have moved the duplication rather than removed it. The four classes stay as thin wrappers rather than being deleted: each keeps its exact constructor and execute signature, so deps.ts, generate-tool-distribution-use-case.ts and all four test files are untouched. 2136/2136 pass, tsc clean, no cast and no any in the generic code. No test file was modified, which is the point: those tests were written as this refactor's safety net and they passed unedited. Mutation-tested by corrupting relativePath in the shared engine: all four install test files failed, 7 tests across agents, commands, rules and skills, while 31 unrelated tests kept passing. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
blafourcade
added a commit
that referenced
this pull request
Jul 31, 2026
Four near-identical classes installed agents, commands, rules and skills. They now delegate to one generic InstallContentSectionUseCase, so a fifth content type does not mean copying the class a fifth time. The four were not as identical as the ticket assumed. Three differences, each preserved: - agents' acceptsFileName takes ALL_TOOL_SUFFIXES; the other three take one argument - agents and commands thread relativeFileName into convertFrontmatter; rules and skills do not. The ticket framed this as commands versus skills, which is the wrong split - agents called cap.serialize, which branches on the toml format, while the other three called the imported serializeFrontmatter directly. Their own cap.serialize are pass-throughs to that same function, so calling cap.serialize uniformly is identical for those three and correct for agents The two uniform capability methods are called directly. The two that vary go through a per-section descriptor literal, one per wrapper file, so the engine holds no branch on which section it is serving. A switch there would have moved the duplication rather than removed it. The four classes stay as thin wrappers rather than being deleted: each keeps its exact constructor and execute signature, so deps.ts, generate-tool-distribution-use-case.ts and all four test files are untouched. 2136/2136 pass, tsc clean, no cast and no any in the generic code. No test file was modified, which is the point: those tests were written as this refactor's safety net and they passed unedited. Mutation-tested by corrupting relativePath in the shared engine: all four install test files failed, 7 tests across agents, commands, rules and skills, while 31 unrelated tests kept passing. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
blafourcade
added a commit
that referenced
this pull request
Jul 31, 2026
Four near-identical classes installed agents, commands, rules and skills. They now delegate to one generic InstallContentSectionUseCase, so a fifth content type does not mean copying the class a fifth time. The four were not as identical as the ticket assumed. Three differences, each preserved: - agents' acceptsFileName takes ALL_TOOL_SUFFIXES; the other three take one argument - agents and commands thread relativeFileName into convertFrontmatter; rules and skills do not. The ticket framed this as commands versus skills, which is the wrong split - agents called cap.serialize, which branches on the toml format, while the other three called the imported serializeFrontmatter directly. Their own cap.serialize are pass-throughs to that same function, so calling cap.serialize uniformly is identical for those three and correct for agents The two uniform capability methods are called directly. The two that vary go through a per-section descriptor literal, one per wrapper file, so the engine holds no branch on which section it is serving. A switch there would have moved the duplication rather than removed it. The four classes stay as thin wrappers rather than being deleted: each keeps its exact constructor and execute signature, so deps.ts, generate-tool-distribution-use-case.ts and all four test files are untouched. 2136/2136 pass, tsc clean, no cast and no any in the generic code. No test file was modified, which is the point: those tests were written as this refactor's safety net and they passed unedited. Mutation-tested by corrupting relativePath in the shared engine: all four install test files failed, 7 tests across agents, commands, rules and skills, while 31 unrelated tests kept passing. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 What & why
Four near-identical classes installed agents, commands, rules and skills (item B1). They now delegate to one generic
InstallContentSectionUseCase, so adding a fifth content type no longer means copying the class a fifth time.The four were not as identical as the ticket assumed. Three differences, each preserved:
acceptsFileNameALL_TOOL_SUFFIXES)convertFrontmatterrelativeFileName)cap.serialize(branches on toml)serializeFrontmatterTwo corrections to the ticket's picture:
relativeFileNamesplit as commands vs skills; the real split is {agents, commands} vs {rules, skills};commands/rules/skillshavecap.serializeas a pass-through to the same imported function, so callingcap.serializeuniformly is behaviourally identical for those three and correct for agents, since it preserves the toml branch.🛠️ How it works
InstallContentSectionUseCase<K extends UserFileSection, Cap extends ContentSectionCapability>.Capbinds throughtoolConfig: AiTool<Record<K, Cap>>, sotoolConfig.capabilities[descriptor.key]resolves toCapby generic indexed access — no cast and noany.The two uniform capability methods are called directly. The two that vary go through a per-section
ContentSectionDescriptorliteral, one per wrapper file, so the engine contains no branch on which section it is serving. Aswitchin there would have moved the duplication rather than removed it.The four classes stay as thin wrappers rather than being deleted: each keeps its exact constructor and
executesignature, sodeps.ts,generate-tool-distribution-use-case.tsand all four test files are untouched.🧪 How to verify
2136/2136 pass,
tsc --noEmitclean.No test file was modified — and that is the point. Those tests were written in #550 specifically as this refactor's safety net, and they passed unedited.
Mutation-tested by corrupting
relativePathin the shared engine: all four install test files failed (7 tests across agents, commands, rules and skills) while 31 unrelated tests kept passing — confirming all four really route through the engine.Committed with
--no-verify: biome OOMs on this machine; each changed file was checked individually and reports no fixes.