-
Notifications
You must be signed in to change notification settings - Fork 27
feat: prefix content transform and deprecated settings removal #296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Implements #292: When updating kits, settings that were previously installed but are no longer shipped in the source will be automatically removed from the user's destination settings. Changes: - Add hooksRemoved, mcpServersRemoved counters to MergeResult - Add removedHooks, removedMcpServers arrays for detailed tracking - mergeHooks: detect and remove deprecated hooks (in installed but not source) - mergeMcp: detect and remove deprecated MCP servers - Add comprehensive test coverage for removal scenarios Safety: Only removes CK-managed entries (identified via installedSettings). User-added entries (not in installedSettings) are preserved.
feat: Auto-remove deprecated hooks and MCP servers during merge
Previously --prefix only reorganized directory structure (commands/ → commands/ck/) but didn't transform content inside files. This caused slash commands to fail because files still referenced /plan:fast instead of /ck:plan:fast. Changes: - Add content-transformer.ts for command reference transformation - Integrate into prefix-applier.ts after directory restructuring - Transform patterns: /plan:, /fix:, /code:, /review:, /cook:, /brainstorm, etc. - Add comprehensive unit tests (20 test cases) Closes #294
Added 6 missing ClaudeKit commands to COMMAND_ROOTS: - test, debug, preview, kanban, journal, watzup These commands are referenced in engineer repo but weren't being transformed when --prefix was applied. Added 6 new tests to verify transformations.
fix: transform command references in file contents when --prefix applied
PR Review: Prefix Content Transform and Deprecated Settings RemovalSummaryThis is an excellent PR that addresses two important gaps in the ClaudeKit CLI functionality. The implementation is well-structured, thoroughly tested, and follows the project's code standards. The changes are additive and non-breaking, with strong attention to edge cases. ✅ Strengths1. Architecture & Design
2. Test Coverage ⭐
3. Code Quality
🔍 Potential Issues & Suggestions1. Content Transformer - Regex Performance (Minor)Location: The current implementation creates 32 regex patterns (16 commands × 2 patterns each) and applies them sequentially on every file. For large files, this could be slow. Suggestion: Consider combining patterns into a single regex with capture groups: // Instead of 32 separate patterns, use one optimized pattern
const COMMAND_PATTERN = new RegExp(
`(?<![\\w:])(/)(?:${COMMAND_ROOTS.join('|')})(?::|(?=[\\s\`"'\\)\\]}>.,;:!?]|$))`,
'g'
);This would reduce the number of passes over the content from 32 to 1, significantly improving performance for large files. 2. File Extension Detection Edge Case (Minor)Location: function shouldTransformFile(filename: string): boolean {
const ext = filename.toLowerCase().slice(filename.lastIndexOf("."));
return TRANSFORMABLE_EXTENSIONS.has(ext);
}Issue: Files without extensions will return the entire filename as the extension.
Impact: Low - these files won't match the extensions set anyway, but it's semantically incorrect. Suggestion: function shouldTransformFile(filename: string): boolean {
const lastDot = filename.lastIndexOf(".");
if (lastDot === -1 || lastDot === 0) return false; // no extension or dotfile
const ext = filename.slice(lastDot).toLowerCase();
return TRANSFORMABLE_EXTENSIONS.has(ext);
}3. Merge Engine - Potential Memory Issue (Low Priority)Location: The Current: filtered.push({ ...entry, hooks: remainingHooks });Suggestion: Only clone if changes were made: if (remainingHooks.length === entry.hooks.length) {
filtered.push(entry); // No changes, keep original reference
} else if (remainingHooks.length > 0) {
filtered.push({ ...entry, hooks: remainingHooks });
}4. Missing Error Boundary (Enhancement)Location: The content transformation happens after successful directory reorganization but inside the try-catch. If transformation fails catastrophically, it could leave the system in an inconsistent state (directories moved but content not transformed). Suggestion: Add a separate try-catch for content transformation: try {
const transformResult = await transformCommandReferences(claudeDir, {
verbose: logger.isVerbose(),
});
// ... success logging
} catch (transformError) {
logger.warn("Content transformation failed, but directory structure is correct");
logger.debug(`Transform error: ${transformError}`);
// Don't fail the entire operation
}5. Test Gap: Multiple Replacements with Same Command (Minor)The tests cover multiple different commands but not multiple instances of the same command: Suggestion: Add a test case: it("transforms multiple instances of same command", () => {
const input = "Use /plan:fast first, then /plan:hard, then /plan:fast again";
const { transformed, changes } = transformCommandContent(input);
expect(changes).toBe(3);
expect(transformed).toContain("/ck:plan:fast");
expect(transformed).toContain("/ck:plan:hard");
});🔒 Security Considerations✅ No security concerns identified
🎯 Performance Considerations
let cachedPatterns: Array<{ regex: RegExp; replacement: string }> | null = null;
function buildCommandPatterns() {
if (cachedPatterns) return cachedPatterns;
cachedPatterns = [...]; // build patterns
return cachedPatterns;
}
📋 Additional ObservationsPositive Patterns:
Suggestions:
🎓 Code Standards Compliance✅ Passes all quality gates from CLAUDE.md:
✅ Follows project patterns:
📊 Final AssessmentRecommendation: ✅ APPROVE WITH MINOR SUGGESTIONS This PR is production-ready. The identified issues are minor optimizations and edge cases that don't impact core functionality. The code is well-tested, follows best practices, and solves real user problems. Priority of Suggestions:
Merge Decision:✅ Ready to merge as-is, with optional follow-up PR for optimizations if needed. Great work on the thorough testing and clean implementation! 🚀 |
|
🎉 This PR is included in version 3.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
--prefixonly moved directories but didn't update file contents. Now transforms/plan:fast→/ck:plan:fast, etc.Changes
Prefix Content Transform
content-transformer.tsfor slash command reference transformationDeprecated Settings Removal
settings.jsonduring mergeTest Plan