Skip to content

Conversation

@kaitranntt
Copy link
Collaborator

Summary

Merges accumulated fixes from dev to main, including:

PR #290: Kit-scoped uninstall and HOME directory edge cases

  • getInstalledKits(): Returns ALL matching kits in legacy format
  • HOME directory handling: Auto-selects global at HOME, warns on init/new
  • Deduplication: Prevents duplicate installations when local===global
  • PathResolver: Added isAtHomeDirectory(), isLocalSameAsGlobal() helpers

PR #289: Legacy migration skip directories

  • legacy-migration: Skips node_modules/ and .venv/ during scan
  • skills-manifest: Skips internal directories in manifest generation
  • hash-calculator: Skips internal directories in hash calculation

Files Changed (15 files, +275/-30)

Area Changes
Commands uninstall-command.ts, selection-handler.ts, directory-setup.ts, conflict-handler.ts
Migration metadata-migration.ts, legacy-migration.ts
Skills skills-manifest.ts, hash-calculator.ts
Services installation-detector.ts, claudekit-scanner.ts
Shared path-resolver.ts, prompts.ts
Tests metadata-migration.test.ts, path-resolver.test.ts, legacy-migration.test.ts

Test Plan

  • Unit tests pass for all changes
  • Quality gate: typecheck, lint, build pass

kaitranntt and others added 6 commits January 4, 2026 23:16
Fixes #288 - legacy migration was counting 82K+ files because
scanFiles() didn't apply SKIP_DIRS_ALL exclusions.
Same bug pattern - getAllFiles() in hash-calculator.ts and
skills-manifest.ts only skipped node_modules, not .venv etc.
Now uses BUILD_ARTIFACT_DIRS for consistent exclusion.
- Fix getInstalledKits() to return ALL matching kits in legacy format
  (was returning early after first match, missing merged kits)
- Add PathResolver.isAtHomeDirectory(), getLocalClaudeDir(), isLocalSameAsGlobal()
- Uninstall: skip scope prompt at HOME (auto-select global), block --local at HOME
- Init: warn when installing at HOME, prompt for global confirmation
- New: warn when creating project at HOME, prompt for global confirmation
- Add tests for HOME directory detection and multi-kit legacy detection
fix: skip node_modules/.venv in legacy migration scan
- installation-detector: skip local when local===global (avoid duplicates)
- claudekit-scanner: skip project population when same as global path
- conflict-handler: use PathResolver.isLocalSameAsGlobal() for consistency
fix: kit-scoped uninstall and HOME directory edge cases (#287)
@claude
Copy link

claude bot commented Jan 5, 2026

Pull Request Review

This PR successfully merges accumulated fixes from dev to main, including kit-scoped uninstall and HOME directory handling improvements. The code quality is excellent and all quality gate checks pass.

✅ Strengths

1. Well-Structured Changes

  • Clear separation of concerns across domains (migration, installation, UI)
  • New helper methods in PathResolver are well-documented with JSDoc
  • Consistent error handling patterns throughout

2. Excellent Test Coverage

  • Comprehensive unit tests for new PathResolver methods (isAtHomeDirectory, isLocalSameAsGlobal)
  • Edge case coverage for HOME directory scenarios in uninstall tests
  • Legacy migration tests properly validate skip directories functionality
  • All 322 tests pass with full quality gate compliance

3. Security & Robustness

  • Proper validation to prevent accidental local installation at HOME directory
  • Clear user warnings and interactive prompts when attempting problematic operations
  • Graceful degradation in non-interactive mode with helpful error messages

4. User Experience Improvements

  • Intelligent auto-selection of global scope when at HOME directory
  • Clear, actionable error messages with guidance on next steps
  • New selectScope() prompt provides user-friendly options

5. Performance Optimization

  • Centralized skip directories in shared/skip-directories.ts prevents redundant definitions
  • Efficient deduplication logic in installation-detector.ts avoids duplicate work

📋 Code Quality Observations

1. PathResolver Enhancements (src/shared/path-resolver.ts:345-381)
The new helper methods are well-designed:

isAtHomeDirectory(cwd?: string): boolean
getLocalClaudeDir(baseDir?: string): string
isLocalSameAsGlobal(cwd?: string): boolean
  • Clear, single-responsibility methods
  • Proper path normalization to handle cross-platform differences
  • Respect for test environment (CK_TEST_HOME)

2. Legacy Migration Improvements (src/domains/migration/metadata-migration.ts:232-259)
The getInstalledKits() refactor properly handles bundle kits:

// Legacy format - detect ALL kits from name using word boundaries
const kits: KitType[] = [];
if (/\bengineer\b/i.test(nameToCheck)) kits.push("engineer");
if (/\bmarketing\b/i.test(nameToCheck)) kits.push("marketing");
  • Uses word boundaries to avoid false matches
  • Returns ALL matching kits (not just first match)
  • Properly tested with "Engineer + Marketing Bundle" case

3. Skip Directories Consolidation (src/shared/skip-directories.ts)
Excellent consolidation of skip patterns:

  • BUILD_ARTIFACT_DIRS: node_modules, .venv, dist, etc.
  • CLAUDE_CODE_INTERNAL_DIRS: debug, projects, shell-snapshots, etc.
  • SKIP_DIRS_ALL: Combined list for general file operations

This prevents permission issues and unnecessary scanning.

🔍 Minor Observations

1. Deduplication Logic (src/commands/uninstall/installation-detector.ts:33)
The deduplication is handled correctly:

if (setup.project.path && setup.project.metadata && !isLocalSameAsGlobal) {

Consider adding a debug log when skipping due to deduplication to help with troubleshooting.

2. Comment Numbering (src/commands/uninstall/uninstall-command.ts)
The sequential comment numbering (5, 6, 7... 14) is helpful for tracking flow, but steps 1-4 are missing from the visible code. This is fine, but ensure consistency if adding more steps.

3. Error Messages (src/commands/new/phases/directory-setup.ts:132-134)
The error messages are clear and actionable. Minor suggestion: consider adding the exact HOME path in the warning:

logger.warning(`You're creating a project at HOME directory (${home}).`);

🎯 Best Practices Adherence

Quality Gate: ✅ All checks pass

✓ typecheck - No errors
✓ lint:fix  - No issues  
✓ test      - 322 pass, 0 fail
✓ build     - Success (1.44 MB bundle)

Code Standards: ✅ Follows project conventions

  • Domain-driven structure maintained
  • Proper use of path aliases (@/)
  • Consistent logger usage
  • Zod schema validation where appropriate

Documentation: ✅ Well-documented

  • JSDoc comments for new methods
  • Clear commit messages following convention
  • Comprehensive PR description with file breakdown

🚀 Performance Considerations

No performance concerns identified:

  • Minimal overhead from new path checks (simple string comparison)
  • Skip directories optimization improves scan performance
  • Deduplication reduces unnecessary file operations

🔒 Security Review

No security concerns:

  • No command injection risks
  • Proper path validation and normalization
  • Safe handling of user input
  • No hardcoded credentials or secrets

📊 Overall Assessment

Rating: Excellent ⭐⭐⭐⭐⭐

This is a well-crafted PR that addresses important edge cases while maintaining code quality and test coverage. The HOME directory handling is particularly thoughtful - it prevents user confusion and potential data loss while providing clear guidance.

Recommendation: APPROVE & MERGE

The changes are production-ready and improve both the robustness and user experience of the CLI tool.

🎉 Highlights

  1. Zero breaking changes - Purely additive improvements
  2. Comprehensive testing - All new functionality is tested
  3. User-centric design - Clear warnings and helpful error messages
  4. Code organization - Excellent refactoring of skip directories

Great work on this PR! The attention to detail and thorough testing demonstrates strong engineering practices. 🚀

@kaitranntt kaitranntt merged commit afe0fdf into main Jan 5, 2026
5 checks passed
@github-actions
Copy link

github-actions bot commented Jan 5, 2026

🎉 This PR is included in version 3.22.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants