feat: Complete UMS CLI separation - migrate to pure functions#70
feat: Complete UMS CLI separation - migrate to pure functions#70
Conversation
- Add constants validation tests with 42 test cases covering all UMS v1.0 constants - Add error formatting utility tests with 42 test cases for CLI error handling - Add UMS error utility tests with 52 test cases for library error types - All tests pass with comprehensive edge case coverage - Improves test coverage for utility functions and type validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add extensive test suite for modules.config.yml conflict resolution - Test all three conflict strategies: error, replace, and warn - Test nested conflict resolution across multiple config levels - Test performance with large numbers of conflicts - Test edge cases: empty configs, malformed strategies, circular replacements - Document current implementation gap where error strategy converts to warnings - 23 comprehensive test cases with 100% coverage of conflict resolution scenarios - All tests pass with proper mocking of file system operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ations - Add CLI file operations utilities (file-operations.ts) with comprehensive error handling - Add CLI configuration loader (config-loader.ts) for modules.config.yml management - Update UMS library to support content-based parsing alongside file-based loading - Add parseModule() and parsePersona() functions for content-only operations - Make filePath optional in UMSModule interface for backward compatibility - Update build command to use new CLI file operations with utf-8 encoding - Add comprehensive test coverage for all new utilities (100% coverage) - Maintain full backward compatibility with existing UMS library API This completes Phase 1 of the CLI/library separation plan, moving all file I/O operations to CLI helpers while preserving UMS library parsing capabilities. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit implements Phase 2 of the UMS CLI separation migration, moving module discovery and configuration management from UMS library to CLI layer, and fixes all tests to work with the new architecture. **Phase 2 Architecture Changes:** - Create CLI-based module discovery utilities with conflict resolution - Refactor ModuleRegistry to accept pre-loaded modules via constructor - Update BuildEngine to require ModuleRegistry through dependency injection - Remove file system dependencies from UMS library core - Move configuration management entirely to CLI layer **New CLI Components:** - `module-discovery.ts`: Utilities for discovering standard and local modules - `module-discovery.test.ts`: Comprehensive tests for discovery logic - Updated CLI commands to handle module discovery and registry creation **UMS Library Changes:** - ModuleRegistry constructor now accepts `(modules, warnings)` parameters - BuildEngine constructor now requires `registry` parameter - Removed file discovery logic from library core - Updated tests to work with dependency injection pattern **Test Infrastructure Updates:** - Fixed build command tests for new ModuleRegistry/BuildEngine constructors - Fixed search command tests to use direct module resolution - Updated all mocks to match new architecture patterns - Removed deprecated `loadModule` references from tests - Fixed module-discovery test to handle multiple module instances correctly **Lint Fixes:** - Removed unnecessary `async` keywords from `loadModulesFromRegistry` functions - Fixed `@typescript-eslint/require-await` errors in list.ts and search.ts **Test Results:** - CLI Package: 171 tests passing (8 test files) - UMS Library: 96 tests passing (4 test files) - Total: 267 tests passing with no errors **Benefits:** - Clear separation of concerns: CLI handles I/O, UMS lib handles logic - Improved testability through dependency injection - Centralized conflict resolution in CLI layer - All tests pass with new architecture - No lint errors remaining 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…re functions - Create pure function modules for complete separation of concerns: * resolver.ts: Module resolution and conflict handling * renderer.ts: Markdown rendering per UMS v1.0 spec * report-generator.ts: Build report generation with SHA-256 hashing - Deprecate BuildEngine and ModuleRegistry classes in favor of functional composition - Update CLI build command to use pure function pipeline: parsePersona → resolvePersonaModules → renderMarkdown → generateBuildReport - Add comprehensive test coverage for all new pure functions (121 tests passing) - Maintain backward compatibility during transition period - Export new functions from UMS library index for public API Phase 3 achieves complete architectural separation between CLI orchestration and UMS library logic through dependency injection and pure functions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Complete the migration from BuildEngine class-based architecture to pure function-based UMS v1.0 implementation. This finalizes the separation of concerns between CLI I/O operations and UMS library business logic. Key changes: - Update all CLI commands to use pure functions from UMS library - Remove BuildEngine class and related test files from ums-lib - Migrate tests to use pure function mocking patterns - Add test setup files for both CLI and UMS packages - Update package dependencies and exports in ums-lib - Ensure clean architectural boundaries between packages This completes the UMS CLI separation project, establishing a clean separation where the CLI handles all I/O operations and the UMS library provides pure data transformation functions for parsing, validation, resolution, rendering, and reporting. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @synthable, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural overhaul, establishing a clean separation of concerns between the core Unified Module System (UMS) library and its Command Line Interface (CLI). By migrating the UMS library to a pure function architecture, it becomes more testable, predictable, and decoupled from I/O operations. The CLI now explicitly manages all file system interactions and module discovery, ensuring a robust and maintainable foundation for future development. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR completes the migration to a pure function architecture by removing all file system dependencies from the UMS library core and establishing clean separation between CLI I/O operations and UMS business logic. The changes enable better testability, modularity, and future reusability of the UMS library in different environments.
Key changes:
- Pure Function Architecture: Converted module and persona loaders from file-based to content-string-based pure functions
- CLI Separation: Moved all file I/O operations to CLI package with dedicated utility modules
- Clean Dependencies: Removed file system dependencies from UMS library, maintaining strict separation of concerns
Reviewed Changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ums-lib/src/core/module-loader.ts | Converted loadModule to pure parseModule function accepting content strings |
| packages/ums-lib/src/core/persona-loader.ts | Converted loadPersona to pure parsePersona function for content parsing |
| packages/ums-lib/src/core/build-engine.ts | Removed BuildEngine class entirely, replaced with pure functions |
| packages/copilot-instructions-cli/src/commands/build.ts | Migrated to use pure UMS functions with CLI-based I/O operations |
| packages/copilot-instructions-cli/src/utils/module-discovery.ts | New CLI utility for module discovery with conflict resolution |
| packages/ums-lib/src/types/index.ts | Updated types to make filePath optional and remove file dependencies |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Process persona and modules | ||
| const result = await processPersonaAndModules(buildEnvironment, progress); | ||
| const result = processPersonaAndModules(buildEnvironment, progress); |
There was a problem hiding this comment.
[nitpick] The function processPersonaAndModules is now synchronous but the variable name result suggests it might have been async before. Consider renaming the function to generateBuildResult to better reflect its pure function nature.
packages/copilot-instructions-cli/src/utils/module-discovery.ts
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed architectural refactoring that successfully migrates the UMS build process from a class-based BuildEngine to a pure functional approach. The separation of concerns is much cleaner, with the ums-lib now free of file system dependencies and the CLI handling all I/O. The new utility files for module discovery, configuration, and file operations are a great addition. The tests have been diligently updated to reflect these changes. I have one major concern regarding the build report generation, where module digests are no longer being calculated, which impacts the integrity and auditability of the build. Other than that, this is an excellent improvement to the codebase.
Add comprehensive JSDoc documentation with parameter descriptions, return type, and exception handling information. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Add comprehensive JSDoc documentation with YAML structure example, parameter descriptions, return type, and exception handling. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Improve comment to clearly specify when filePath is present vs absent, making the optional behavior more explicit for API consumers. Co-Authored-By: Claude <noreply@anthropic.com>
Add default parameter to discoverStandardModules function to improve testability and flexibility while maintaining backward compatibility. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // For now, return modules as-is | ||
| return modules; |
There was a problem hiding this comment.
This function is a placeholder that doesn't use its second parameter. The unused parameter should be removed or the function should be marked as intentionally incomplete with a more descriptive implementation plan.
| // For now, return modules as-is | |
| return modules; | |
| // Placeholder: Implementation resolution is not yet implemented. | |
| // The 'registry' parameter will be used in the future to resolve implementations. | |
| throw new Error("resolveImplementations is not yet implemented. This function will use the 'registry' parameter to resolve module implementations when the 'implement' field is added to ModuleBody."); |
Replace incorrect use of map() with side effects with a proper for-of loop to prevent potential array length mismatches and improve code clarity. Co-Authored-By: Claude <noreply@anthropic.com>
Replace complex conditional separator logic with cleaner join() approach that builds module blocks first then joins with separators, improving code readability and maintainability. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Key Changes
Test Coverage
🤖 Generated with Claude Code