Merged
Conversation
|
Contributor
|
Important Review skippedToo many files! 51 files out of 201 files are above the max files limit of 150. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
📋 OpenAPI Schema AnalysisSummarySchema Components
Overall Status
Detailed Changes📋 Schema Components ChangesModified Schemas (1):
🤖 This comment is automatically updated when you push new commits. |
luxass
force-pushed
the
store-changes
branch
from
9c0e0b8 to
e386e2c
Compare
Updated the UCD store to utilize a lockfile instead of a manifest for version management. This change includes the introduction of lockfile-related functions, adjustments to the internal context, and modifications to various operations to read from and write to the lockfile. The bootstrap, sync, and verification processes have been adapted to ensure compatibility with the new lockfile structure, enhancing the overall organization and maintainability of the codebase.
…test coverage This commit finalizes the refactor of the test structure by implementing reusable test helpers and updating all relevant test files to utilize the new structure. The changes include the completion of various test files, such as bootstrap, sync, and verify tests, ensuring they now leverage the new lockfile management system. Additionally, unused variables in the sync tests were addressed, improving overall test coverage and maintainability.
This commit cleans up the list and tree test files by removing unnecessary blank lines, improving readability and maintaining consistency across the test suite.
… test coverage This commit adds a new read-only filesystem bridge for testing purposes, allowing for better control over file operations during tests. It also updates the test utilities to include this new bridge and enhances the test suite for lockfile management, ensuring comprehensive coverage of various scenarios, including integration tests for store operations and snapshot handling. Additionally, it refactors existing tests to utilize the new bridge, improving maintainability and consistency across the test suite.
This commit introduces a comprehensive redesign of the CLI store commands to align with the new ucd-store-v2 API. Key changes include the addition of new commands (sync, mirror, verify, analyze, status) and the removal of deprecated commands (repair, clean). The CLI now supports an operation-based structure, enhanced lockfile inspection capabilities, and flexible flag options such as --force and --lockfile-only. Additionally, the package dependency has been updated to include @ucdjs/ucd-store-v2, ensuring compatibility with the new API.
…issue This commit addresses two key issues in the CLI: 1. Corrects the property paths accessed from the AnalysisReport in the analyze command to align with the actual structure. 2. Fixes the getLockfilePath function to return a relative path instead of an absolute path, resolving the double directory path issue during initialization. Additionally, updates are made to ensure consistent usage of the new path structure across various commands, including init, mirror, and file operations. This enhances the overall functionality and reliability of the CLI commands.
…ion plan This commit introduces a detailed plan for verifying and fixing the fs-bridge path resolution security. The plan outlines various tasks, including analyzing current implementations, evaluating API improvements, creating comprehensive test suites for different path scenarios, and conducting security audits. It aims to ensure that path traversal is prevented and that both relative and absolute paths are handled securely, enhancing the overall security and maintainability of the fs-bridge.
…ion plan This commit marks the completion of several tasks outlined in the fs-bridge path resolution security verification plan. Key updates include the analysis of the resolveSafePath implementation, the fs-bridge node and http bridges, and the creation of comprehensive test suites for various path scenarios, including edge cases and security tests. Additionally, the plan has been updated to reflect the current status of tasks, ensuring a clear overview of progress and next steps.
This commit introduces a suite of security tests for the HTTP bridge in the fs-bridge package. Key additions include tests for path traversal prevention, URL-encoded traversal attacks, boundary enforcement for shallow and deep pathnames, and handling of excessive and mixed encoding scenarios. The tests ensure that the HTTP bridge correctly enforces security measures against various attack vectors, enhancing the overall robustness of the system. Additionally, the plan has been updated to reflect the completion of these security test implementations.
This commit finalizes the implementation of security tests for the fs-bridge, focusing on preventing path traversal attacks through various vectors, including encoded paths and recursive directory listings. New test cases ensure that both the Node and HTTP bridges effectively handle malicious input and maintain robust security measures. Additionally, the comprehensive plan has been updated to reflect the completion of these critical security enhancements.
This commit removes the outdated FS Bridge documentation and introduces a new structure for the fs-bridge documentation, including an overview, hooks system, and detailed specifications for both Node.js and HTTP bridges. The hooks system allows users to monitor and debug file system operations effectively. Additionally, comprehensive usage examples and best practices are provided to enhance user understanding and implementation of the fs-bridge package.
This commit introduces new documentation for the capabilities system and error handling patterns within the fs-bridge package. The capabilities section explains how bridges detect and assert optional operations, while the error handling section outlines various error types and best practices for managing errors during bridge operations. Additionally, links to these new sections have been added to the main fs-bridge documentation for improved accessibility.
This commit introduces the `assertWriteCapability` function to ensure that the UCD store has the necessary write permissions before performing operations. The function is integrated into the `init`, `mirror`, `sync`, and `status` commands to provide early error handling for operations requiring write access. Additionally, the documentation for the `sync` operation is updated to reflect the new behavior, emphasizing the synchronization of lockfiles with API versions and the mirroring of files. This enhancement improves the robustness and user experience of the CLI commands.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces significant changes to the store management system, transitioning from a manifest-based approach to a lockfile-based approach with snapshots. The changes span across the CLI, shared packages, and core store functionality.
- Replaced manifest system with lockfile and per-version snapshots for improved version tracking
- Migrated from file-tree endpoints to per-version manifest endpoints
- Added comprehensive test utilities for lockfile and snapshot creation
- Enhanced filter support with extractable patterns and default exclusions
Reviewed changes
Copilot reviewed 177 out of 198 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ucd-store-v2/test/operations/analyze.test.ts | Updated tests to use new test context helper and lockfile system |
| packages/ucd-store-v2/test/integration/store-operations.test.ts | New integration tests for bootstrap→mirror→verify workflow |
| packages/ucd-store-v2/test/helpers/test-context.ts | New test helper for creating test contexts with lockfiles |
| packages/ucd-store-v2/test/core/snapshot.test.ts | New tests for snapshot read/write operations |
| packages/ucd-store-v2/test/core/lockfile.test.ts | Updated lockfile tests to use new error types and paths |
| packages/ucd-store-v2/test/core/files.test.ts | Updated to use manifest endpoint instead of file-tree |
| packages/ucd-store-v2/test/core/context.test.ts | Updated property names from manifestPath to lockfilePath |
| packages/ucd-store-v2/test/core/config-discovery.test.ts | New tests for config discovery and version extraction |
| packages/ucd-store-v2/src/types.ts | Updated documentation from manifest to lockfile terminology |
| packages/ucd-store-v2/src/store.ts | Refactored to use lockfile instead of manifest |
| packages/ucd-store-v2/src/setup/verify.ts | Updated to use lockfile instead of manifest |
| packages/ucd-store-v2/src/setup/bootstrap.ts | Updated to create lockfile instead of manifest |
| packages/ucd-store-v2/src/operations/sync.ts | Major refactor to sync lockfile and mirror files |
| packages/ucd-store-v2/src/operations/mirror.ts | Added snapshot creation and lockfile updates |
| packages/ucd-store-v2/src/operations/files/tree.ts | Updated to use wrapTry instead of tryCatch |
| packages/ucd-store-v2/src/operations/files/list.ts | Updated to use wrapTry instead of tryCatch |
| packages/ucd-store-v2/src/operations/files/get.ts | Updated to use wrapTry and tryOr utilities |
| packages/ucd-store-v2/src/operations/analyze.ts | Updated to use wrapTry and filter snapshot.json |
| packages/ucd-store-v2/src/factory.ts | Added path resolution for basePath |
| packages/ucd-store-v2/src/core/files.ts | Updated to use manifest endpoint instead of file-tree |
| packages/ucd-store-v2/src/core/context.ts | Added filter pattern extraction utility |
| packages/ucd-store-v2/package.json | Added lockfile dependency and test-utils imports |
| packages/test-utils/src/mock-store/handlers/well-known.ts | Added per-version manifest endpoint handler |
| packages/test-utils/src/mock-store/handlers/file-tree.ts | Updated to support version-specific file responses |
| packages/test-utils/src/fs-bridges/memory-fs-bridge.ts | Enhanced with explicit directory support and function overrides |
| packages/test-utils/src/fs-bridges/read-only-bridge.ts | New read-only bridge for testing |
| packages/shared/src/fetch/fetch.ts | Added schema validation support |
| packages/shared/src/filter.ts | Exported default exclusions constant |
| packages/shared/src/async/try-catch.ts | Added tryOr utility and renamed tryCatch to wrapTry |
| packages/schemas/src/manifest.ts | New schema for per-version manifests |
| packages/schemas/src/lockfile.ts | Added filters field to lockfile schema |
| packages/lockfile/* | New package for lockfile operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // We can't proceed with the sync operation. | ||
| if (configResult.error || !configResult.data) { | ||
| // TODO: handle this error better. | ||
| throw new Error("Failed to fetch versions from API"); |
There was a problem hiding this comment.
The error message "Failed to fetch versions from API" is generic and doesn't provide details about why the fetch failed. Consider including the underlying error information from configResult.error if available.
Suggested change
| throw new Error("Failed to fetch versions from API"); | |
| const errorDetail = | |
| configResult.error && (configResult.error as any).message | |
| ? `: ${(configResult.error as any).message}` | |
| : configResult.error | |
| ? `: ${String(configResult.error)}` | |
| : ""; | |
| throw new Error(`Failed to fetch versions from API${errorDetail}`); |
| * Gets the default lockfile path for a given base path. | ||
| * The lockfile is always named `.ucd-store.lock` regardless of base path. | ||
| * | ||
| * @param {string} _basePath - Base path (unused, kept for API compatibility) |
There was a problem hiding this comment.
The parameter _basePath is prefixed with underscore to indicate it's unused, but according to the JSDoc it's "kept for API compatibility". Consider either removing this parameter entirely if it's not needed for compatibility, or using a more descriptive name without the underscore prefix if it might be used in the future.
Suggested change
| * @param {string} _basePath - Base path (unused, kept for API compatibility) | |
| * @param {string} _basePath - Base path (currently unused; reserved for API stability) |
Greptile Summary
Important Files Changed
Confidence score: 2/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (7)
-
apps/api/vitest.config.worker.tslogic: Removing worker test configuration will break Cloudflare Workers-specific tests. Check that worker tests are properly configured elsewhere or that this deletion is intentional. Are the worker tests now handled by a different configuration, or should this deletion be reconsidered?
-
packages/ucd-store-v2/package.json, line 16 (link)syntax: Directory path references old 'ucd-store' instead of 'ucd-store-v2' - should be updated for consistency
-
packages/ucd-store-v2/src/types.ts, line 14-15 (link)syntax: Comment still references 'manifest' but should say 'lockfile' for consistency
-
packages/test-utils/src/mock-store/utils.ts, line 103-104 (link)style: Comment still references the old
:wildcard+pattern but describes the correct behavior -
packages/cli/src/cmd/store/root.ts, line 28 (link)logic: Redundant condition -
!isValidSubcommand(subcommand)is checked twice -
packages/client/test/resources/manifest.test.ts, line 58 (link)style: inconsistent approach - this test uses actual version interpolation while others use placeholder pattern
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
packages/ucd-store-v2/src/operations/mirror.ts, line 225-247 (link)logic: The
totalMetricValuevariable is computed usingreportbeforereportis fully assigned, causing it to use the previous iteration's values. This creates incorrect metric calculations.
197 files reviewed, 45 comments
Comment on lines
+141
to
+143
| await expect( | ||
| bridge.read("//etc/passwd"), | ||
| ).rejects.toThrow(); |
There was a problem hiding this comment.
logic: The comment suggests this should not throw a traversal error, but the test expects it to throw - clarify the expected behavior. Should protocol-relative URLs throw PathTraversalError or just file not found errors?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/fs-bridge/test/security/penetration-testing.test.ts
Line: 141:143
Comment:
**logic:** The comment suggests this should not throw a traversal error, but the test expects it to throw - clarify the expected behavior. Should protocol-relative URLs throw PathTraversalError or just file not found errors?
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+285
to
+287
| // Empty string should resolve to basePath | ||
| const exists = await bridge.exists(" "); | ||
| expect(exists).toBe(true); |
There was a problem hiding this comment.
logic: This test assumes whitespace-only paths resolve to basePath directory existence - verify this behavior is intentional. Is it intentional that whitespace-only paths should return true for exists()?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/fs-bridge/test/security/penetration-testing.test.ts
Line: 285:287
Comment:
**logic:** This test assumes whitespace-only paths resolve to basePath directory existence - verify this behavior is intentional. Is it intentional that whitespace-only paths should return true for exists()?
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Yes, because the underlying bridge calls resolveSafePath which trims the string, and run it trough path.normalize which causes it to be a .
- Introduced `toMatchError` matcher for enhanced error assertions. - Updated `vitest-setup.ts` to include the new matcher. - Modified `tsdown.config.ts` and `base.json` to reference new matcher files. - Adjusted `vitest.config.ts` to include the new matcher setup file.
This was referenced Dec 28, 2025
Closed
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.
🔗 Linked issue
📚 Description
This PR is very big one that changes alot of things. From the CLI to the Shared package.
Ideas:
console.info, console.logoutput to stderr, when--jsonis used.files getcommandfiles infoto list info about the files