refactor: modify fs-bridge to work better #439
Conversation
|
📝 WalkthroughWalkthroughAdds Node listdir API; exports an errors entry; tags HTTP bridges with a runtime symbol and adds an isBuiltinHttpBridge guard; strengthens HTTP bridge URL/schema, logging, and schema checks; refactors BridgeGenericError to accept ErrorOptions; adds playground scripts and test/fixture updates; updates build/config files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPBridge as HTTP Bridge
participant RemoteAPI as Remote API
participant Zod as Schema Validator
rect rgba(99,102,241,0.06)
Client->>HTTPBridge: listdir(path, recursive?)
end
HTTPBridge->>RemoteAPI: GET /list?path=... (fetch)
RemoteAPI-->>HTTPBridge: 200 JSON / 404 / 500
alt 200
HTTPBridge->>Zod: validate(listResponse)
Zod-->>HTTPBridge: ok / error
alt ok
HTTPBridge-->>Client: FSEntry[] (normalized paths, dirs with trailing '/')
else error
HTTPBridge-->>Client: throw BridgeGenericError("Invalid response schema")
end
else 404/403/500
HTTPBridge-->>Client: map status -> empty/list or throw (logged)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
github-actions
bot
added
the
pkg: fs-bridge
📋 OpenAPI Schema Analysis✅ No changes detected - The OpenAPI schema is identical to the main branch.🤖 This comment is automatically updated when you push new commits. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
bot
added
pkg: ucd-store
🌏 Preview DeploymentsNote No deployable apps affected by changes in this PR. Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
Introduced the `isBuiltinHttpBridge` function to check if a file system bridge is the built-in HTTP File System Bridge. Updated the exports in `guards.ts` and `index.ts` to include this new function. Additionally, refactored the `FileSystemBridgeFactory` type definition for improved clarity.
luxass
force-pushed
the
bridge-improvements
branch
from
5866646 to
21748cc
Compare
Greptile Summary
Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant HTTPBridge as "HTTP Bridge"
participant PathUtils as "Path Utils"
participant RemoteAPI as "Remote API"
participant Schema as "Zod Schema"
User->>HTTPBridge: "HTTPFileSystemBridge({ baseUrl })"
HTTPBridge->>HTTPBridge: "Validate baseUrl with Zod"
HTTPBridge-->>User: "Bridge instance"
User->>HTTPBridge: "read(path)"
HTTPBridge->>HTTPBridge: "Validate path (reject trailing /)"
HTTPBridge->>PathUtils: "resolveSafePath(baseUrl.pathname, path)"
PathUtils-->>HTTPBridge: "Safe resolved path"
HTTPBridge->>RemoteAPI: "GET request to resolved URL"
RemoteAPI-->>HTTPBridge: "Response with file content"
HTTPBridge->>HTTPBridge: "response.text()"
HTTPBridge-->>User: "File content string"
User->>HTTPBridge: "exists(path)"
HTTPBridge->>PathUtils: "resolveSafePath(baseUrl.pathname, path)"
PathUtils-->>HTTPBridge: "Safe resolved path"
HTTPBridge->>RemoteAPI: "HEAD request to resolved URL"
RemoteAPI-->>HTTPBridge: "Response status"
HTTPBridge-->>User: "Boolean exists result"
User->>HTTPBridge: "listdir(path, recursive)"
HTTPBridge->>PathUtils: "resolveSafePath(baseUrl.pathname, path)"
PathUtils-->>HTTPBridge: "Safe resolved path"
HTTPBridge->>RemoteAPI: "GET request with Accept: application/json"
RemoteAPI-->>HTTPBridge: "JSON directory listing"
HTTPBridge->>Schema: "Validate with FileEntrySchema"
Schema-->>HTTPBridge: "Validated entries array"
alt recursive = true
loop For each directory entry
HTTPBridge->>HTTPBridge: "Recursive listdir call for subdirectories"
end
end
HTTPBridge-->>User: "FSEntry[] with file/directory structure"
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the fs-bridge package to improve error handling, add playgrounds for manual testing, and enhance schema validation. The changes support work from PR #440 (playgrounds) and help close PR #420 (comprehensive refactoring).
Key changes:
- Updated
@luxass/msw-utilsfrom 0.5.0 to 0.6.0 for better MSW error detection - Added Node.js and HTTP playgrounds for manual testing and verification
- Improved error handling with MSW error detection in the error flow
- Enhanced HTTP bridge with Zod schema validation for file entries
- Updated test expectations for path format consistency (directory paths now require leading/trailing slashes)
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Bumped @luxass/msw-utils from 0.5.0 to 0.6.0 |
| pnpm-lock.yaml | Updated lockfile to reflect dependency version changes |
| packages/ucd-store/test/maintenance/analyze.test.ts | Disabled remote store test pending ucd-store-v2 replacement |
| packages/fs-bridge/tsdown.config.ts | Added bundling config for @luxass/msw-utils and exposed errors module |
| packages/fs-bridge/test/security/node/*.test.ts | Enhanced security tests with better error assertions and recursive listdir coverage |
| packages/fs-bridge/test/security/http/*.test.ts | Updated path format expectations and improved error message assertions |
| packages/fs-bridge/test/bridges/node/node.test.ts | Added comprehensive shape consistency and error surface tests |
| packages/fs-bridge/test/bridges/http/http.test.ts | Added Zod schema validation and fetch error handling tests |
| packages/fs-bridge/test/bridges/http/basepath.*.test.ts | Updated mock responses to use correct path format (leading slashes) |
| packages/fs-bridge/test/bridge-methods.test.ts | Removed redundant comments |
| packages/fs-bridge/src/utils.ts | Integrated isMSWError utility, updated error handling flow, refactored error constructor calls |
| packages/fs-bridge/src/types.ts | Extracted FileSystemBridgeArgs as reusable type |
| packages/fs-bridge/src/index.ts | Added exports for isBuiltinHttpBridge and FileSystemBridgeArgs |
| packages/fs-bridge/src/guards.ts | Added isBuiltinHttpBridge utility function |
| packages/fs-bridge/src/errors.ts | Simplified BridgeGenericError constructor to use ErrorOptions |
| packages/fs-bridge/src/bridges/node.ts | Added comprehensive documentation explaining parity differences with HTTP bridge |
| packages/fs-bridge/src/bridges/http.ts | Replaced deprecated z.httpUrl with z.url config, added debug logging throughout, normalized listdir paths |
| packages/fs-bridge/playgrounds/node-playground.ts | New playground for manual Node bridge testing |
| packages/fs-bridge/playgrounds/http-playground.ts | New playground for manual HTTP bridge testing against real API |
| packages/fs-bridge/package.json | Added playground scripts and errors export, included @luxass/msw-utils in devDependencies |
| packages/fs-bridge/eslint.config.js | Added playgrounds directory to eslint ignore list |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaced instances of `BridgeGenericError` with `BridgeBaseError` in error handling logic to ensure consistent error classification across the file system bridge operations.
- Introduced `kHttpBridgeSymbol` to uniquely identify the HTTP File System Bridge. - Updated `defineFileSystemBridge` to accept an optional `symbol` property. - Enhanced `isBuiltinHttpBridge` to check for the bridge type using the symbol.
github-actions
bot
removed
apps: api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fs-bridge/test/security/http/encoded-attacks.test.ts (1)
146-161: Use consistent version format matching other tests in the file.The "excessive encoding" test (line 146-147) uses
/api/v1/files/16.0.0(without "v" prefix), but earlier tests in the same file use/api/v1/files/v16.0.0(with "v" prefix) on lines 54-55 and 108-109. For consistency, change line 146-147 to use the "v" prefix:Suggested change
- describe("deep pathname (/api/v1/files/16.0.0)", () => { - const basePath = "/api/v1/files/16.0.0"; + describe("deep pathname (/api/v1/files/v16.0.0)", () => { + const basePath = "/api/v1/files/v16.0.0";
🤖 Fix all issues with AI agents
In @packages/fs-bridge/test/security/node/excessive-encoding.test.ts:
- Around line 37-40: Tests use inconsistent error assertion styles: change the
second assertion that expects a thrown string "Failed to decode path" to assert
the specific error class FailedToDecodePathError instead. Update the expect call
around bridge.read(`${encodedTraversal}etc/passwd`) to use
.rejects.toThrow(FailedToDecodePathError) so both tests consistently assert the
same error type.
In @packages/fs-bridge/tsdown.config.ts:
- Around line 13-15: The current tsdown.config.ts forces bundling of
@luxass/msw-utils (listed in noExternal) which pulls test-only helpers like
isMSWError into production; remove @luxass/msw-utils from noExternal and either
(A) extract the isMSWError implementation into the @ucdjs-internal/shared
package and update src/utils.ts to import it from shared, or (B) change the
import in src/utils.ts to a conditional dynamic import (e.g., only load
@luxass/msw-utils when NODE_ENV !== 'production' or when running tests) and make
noExternal conditional on NODE_ENV so production builds do not bundle the
package; update all imports of isMSWError to the new location or to the guarded
dynamic import accordingly.
🧹 Nitpick comments (4)
packages/fs-bridge/test/bridges/http/basepath.http.absolute.test.ts (1)
142-159: Consider adding assertions for path values.The mock data was updated to include leading slashes in the path property (
"/file1.txt","/file2.txt"), but the test only asserts on the length of entries without validating the actual path values. Consider adding assertions to verify the path format matches the expected behavior:💡 Suggested enhancement
const entries = await bridge.listdir(""); expect(entries).toHaveLength(2); +expect(entries[0]?.path).toBe("/file1.txt"); +expect(entries[1]?.path).toBe("/file2.txt");packages/fs-bridge/src/bridges/node.ts (1)
47-47: Refactor the duplicated path validation logic.The TODO correctly identifies code duplication between
readandwritemethods. Both validate trailing slashes identically (lines 48-53 vs 142-147). Consider extracting this validation into a reusable helper function.♻️ Proposed refactor
Add a helper function at the module level:
+function validateFilePathFormat(path: string): void { + const trimmedPath = path.trim(); + if (trimmedPath.endsWith("/") && trimmedPath !== "/" && trimmedPath !== "./" && trimmedPath !== "../") { + throw new Error("Cannot access file: path ends with '/'"); + } +}Then use it in both methods:
async read(path) { - // TODO: duplicate code with write - refactor - // Reject file paths ending with / - files don't have trailing slashes - // Allow /, ./, and ../ as they are special directory references - const trimmedPath = path.trim(); - if (trimmedPath.endsWith("/") && trimmedPath !== "/" && trimmedPath !== "./" && trimmedPath !== "../") { - throw new Error("Cannot read file: path ends with '/'"); - } + validateFilePathFormat(path); const resolvedPath = resolveSafePath(basePath, path); return fsp.readFile(resolvedPath, "utf-8"); }, async write(path, data, encoding = "utf-8") { - // Reject file paths ending with / - files don't have trailing slashes - // Allow /, ./, and ../ as they are special directory references - const trimmedPath = path.trim(); - if (trimmedPath.endsWith("/") && trimmedPath !== "/" && trimmedPath !== "./" && trimmedPath !== "../") { - throw new Error("Cannot write file: path ends with '/'"); - } + validateFilePathFormat(path); const resolvedPath = resolveSafePath(basePath, path); // ... rest of write logicpackages/fs-bridge/playgrounds/node-playground.ts (1)
283-284: Exit handling is acceptable but could be more graceful.Using
process.exit(1)immediately terminates the process. While cleanup has already completed at this point, usingprocess.exitCode = 1would be more graceful and consistent with best practices for allowing any remaining async operations to complete naturally.packages/fs-bridge/src/bridges/http.ts (1)
65-68: Simplify listdir path handling to match the pattern used in read method.The
listdirmethod explicitly prepends a/to the path (\/${path}`), while thereadmethod and other methods use the path directly. SinceresolveSafePathinternally normalizes both cases to the same result, this redundant prefix adds unnecessary inconsistency. Alignlistdirwith the pattern used elsewhere by removing the explicit/` prefix.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
packages/fs-bridge/eslint.config.jspackages/fs-bridge/package.jsonpackages/fs-bridge/playgrounds/http-playground.tspackages/fs-bridge/playgrounds/node-playground.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/define.tspackages/fs-bridge/src/errors.tspackages/fs-bridge/src/guards.tspackages/fs-bridge/src/index.tspackages/fs-bridge/src/types.tspackages/fs-bridge/src/utils.tspackages/fs-bridge/test/bridge-methods.test.tspackages/fs-bridge/test/bridges/http/basepath.http.absolute.test.tspackages/fs-bridge/test/bridges/http/basepath.http.relative.test.tspackages/fs-bridge/test/bridges/http/http.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/test/security/http/encoded-attacks.test.tspackages/fs-bridge/test/security/http/excessive-encoding.test.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/test/security/node/encoded-attacks.test.tspackages/fs-bridge/test/security/node/excessive-encoding.test.tspackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/tsdown.config.tspackages/ucd-store/test/maintenance/analyze.test.tspnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- packages/fs-bridge/test/bridge-methods.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Import test utilities from #test-utils/* path aliases instead of @ucdjs/test-utils in test files to avoid build step requirements and prevent cyclic dependencies
Use testdir() from vitest-testdirs to create temporary test directories in filesystem tests
Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API in test files
Files:
packages/fs-bridge/test/security/http/excessive-encoding.test.tspackages/fs-bridge/test/security/node/excessive-encoding.test.tspackages/fs-bridge/test/bridges/http/basepath.http.absolute.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/test/bridges/http/basepath.http.relative.test.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/fs-bridge/test/bridges/http/http.test.tspackages/fs-bridge/test/security/http/encoded-attacks.test.tspackages/fs-bridge/test/security/node/encoded-attacks.test.ts
packages/*/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for dependencies between workspace packages in package.json
Files:
packages/fs-bridge/package.json
🧠 Learnings (24)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
📚 Learning: 2025-07-29T06:02:32.422Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 175
File: packages/fs-bridge/test/bridges/node.test.ts:0-0
Timestamp: 2025-07-29T06:02:32.422Z
Learning: In the NodeFileSystemBridge security validation, path traversal detection occurs before critical system paths checks, which means absolute paths to critical system directories will always throw "Path traversal detected" rather than "Critical system path access denied", making it impossible to meaningfully test the critical system paths validation logic.
Applied to files:
packages/fs-bridge/test/security/http/excessive-encoding.test.tspackages/fs-bridge/test/security/node/excessive-encoding.test.tspackages/fs-bridge/src/bridges/node.tspackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/test/bridges/http/http.test.tspackages/fs-bridge/test/security/http/encoded-attacks.test.tspackages/fs-bridge/test/security/node/encoded-attacks.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use File System Bridge Pattern (ucdjs/fs-bridge) abstraction for file system operations to support multiple backends (Node.js, HTTP, in-memory)
Applied to files:
packages/fs-bridge/src/guards.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/src/index.tspackages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/playgrounds/node-playground.tspackages/fs-bridge/playgrounds/http-playground.tspackages/fs-bridge/src/utils.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/src/define.tspackages/fs-bridge/test/bridges/http/http.test.tspackages/fs-bridge/src/types.tspackages/fs-bridge/test/security/http/encoded-attacks.test.ts
📚 Learning: 2025-05-04T11:52:22.858Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Applied to files:
packages/fs-bridge/test/bridges/http/basepath.http.absolute.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/tsdown.config.tspackages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/playgrounds/node-playground.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/test/bridges/http/http.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Import test utilities from #test-utils/* path aliases instead of ucdjs/test-utils in test files to avoid build step requirements and prevent cyclic dependencies
Applied to files:
packages/fs-bridge/test/bridges/http/basepath.http.absolute.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/tsdown.config.tspackages/fs-bridge/package.jsonpackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/eslint.config.jspackages/fs-bridge/test/security/http/recursive-listdir-security.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to apps/api/test/**/*.{test,spec}.?(c|m)[jt]s?(x) : Place API app tests in apps/api/test/routes/** for endpoint tests and apps/api/test/unit/** for unit tests with mocks
Applied to files:
packages/fs-bridge/test/bridges/http/basepath.http.absolute.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API in test files
Applied to files:
packages/fs-bridge/test/bridges/http/basepath.http.absolute.test.tspackages/fs-bridge/tsdown.config.tspackages/fs-bridge/test/bridges/http/basepath.http.relative.test.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/ucd-store/test/maintenance/analyze.test.ts
📚 Learning: 2025-06-29T11:20:13.668Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 87
File: packages/worker-shared/tsconfig.build.json:1-4
Timestamp: 2025-06-29T11:20:13.668Z
Learning: In the ucdjs/ucd project, the packages use tsdown instead of tsc for building libraries. The tsconfig.build.json files are primarily for IDE experience and type checking, not for direct compilation, so including "test" directories in these configs doesn't affect build output.
Applied to files:
packages/fs-bridge/tsdown.config.tspackages/fs-bridge/package.json
📚 Learning: 2025-07-20T05:37:40.565Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 131
File: tooling/eslint-plugin/package.json:0-0
Timestamp: 2025-07-20T05:37:40.565Z
Learning: In the ucdjs/ucd project, internal tooling packages (private packages in the tooling/ directory) export TypeScript files directly without requiring a build step, unlike published packages which use tsdown to build and export from ./dist/. Examples include ucdjs/tsdown-config and ucdjs/eslint-plugin.
Applied to files:
packages/fs-bridge/tsdown.config.tspackages/fs-bridge/package.json
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use tsdown for building TypeScript packages with shared tooling configuration from ucdjs-tooling/tsdown-config
Applied to files:
packages/fs-bridge/tsdown.config.tspackages/fs-bridge/package.json
📚 Learning: 2025-10-14T07:15:35.199Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 326
File: packages/shared/src/ucd-config.ts:34-58
Timestamp: 2025-10-14T07:15:35.199Z
Learning: In packages/shared/src/ucd-config.ts, the `getDefaultUCDEndpointConfig` function uses a build-time define `__UCD_ENDPOINT_DEFAULT_CONFIG__` that is replaced at build time by the bundler. The `??` operator is evaluated at build time, so the final code contains no runtime branching. Therefore, runtime unit tests for this function are not valuable—the build process itself handles the injection and fallback logic.
Applied to files:
packages/fs-bridge/tsdown.config.tspackages/ucd-store/test/maintenance/analyze.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to packages/**/.eslintrc.* : Use luxass/eslint-config for linting configuration in new packages
Applied to files:
packages/fs-bridge/package.json
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to packages/*/tsconfig.json : Use ucdjs-tooling/tsconfig for TypeScript configuration in new packages
Applied to files:
packages/fs-bridge/package.json
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use pnpm catalogs for centralized dependency version management organized by purpose (monorepo, testing, linting, prod, dev, workers, types, web, vscode, docs)
Applied to files:
packages/fs-bridge/package.jsonpnpm-workspace.yaml
📚 Learning: 2025-06-14T05:20:20.149Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/validate.ts:115-130
Timestamp: 2025-06-14T05:20:20.149Z
Learning: Node.js 22 supports the `recursive` option in `fs.readdir()` (introduced in Node.js 18.17.0), which returns a flattened string array of all descendant files and directories when `recursive: true` is passed.
Applied to files:
packages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/test/security/node/path-traversal.test.tspackages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/test/bridges/http/http.test.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: The `recursive` option for `fs.readdir()` was introduced in Node.js v20.1.0 and is available in Node.js 22. When `recursive: true` is passed, it will recursively traverse directories and return all file paths as a flat string array.
Applied to files:
packages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/test/security/node/path-traversal.test.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: In Node.js 22, the `recursive` option for `fs.readdir()` is supported and will recursively find all files and return them in a string array when set to true.
Applied to files:
packages/fs-bridge/src/bridges/node.tspackages/fs-bridge/src/bridges/http.tspackages/fs-bridge/test/security/node/path-traversal.test.ts
📚 Learning: 2025-07-09T15:05:57.763Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 97
File: packages/utils/src/fs-bridge/node.ts:40-50
Timestamp: 2025-07-09T15:05:57.763Z
Learning: The `parentPath` property on `fs.Dirent` was added in Node.js v20.12, contrary to previous assumptions about it not being part of the Node.js API.
Applied to files:
packages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-09-02T02:01:40.288Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 231
File: packages/fs-bridge/src/utils.ts:4-8
Timestamp: 2025-09-02T02:01:40.288Z
Learning: The ucdjs/ucd project uses ESLint for linting, not Biome. Don't suggest Biome-specific ignore comments for this codebase.
Applied to files:
packages/fs-bridge/eslint.config.js
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use pnpm workspaces with Turbo for build orchestration and Vitest for testing in this monorepo project
Applied to files:
packages/fs-bridge/eslint.config.js
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Require Node.js >= 22.18 and pnpm 10.26.1 for development in this project
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-08-23T05:20:36.940Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.940Z
Learning: In the ucdjs/ucd codebase using modern Vitest, the assert export from "vitest" works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that need compile-time type safety. The existing usage pattern assert(condition, message) throughout the test suite is working correctly.
Applied to files:
packages/fs-bridge/test/security/http/recursive-listdir-security.test.tspackages/fs-bridge/test/security/http/encoded-attacks.test.ts
📚 Learning: 2025-08-17T10:10:19.096Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.096Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/test/maintenance/analyze.test.ts
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
packages/ucd-store/test/maintenance/analyze.test.ts
🧬 Code graph analysis (8)
packages/fs-bridge/src/guards.ts (2)
packages/fs-bridge/src/types.ts (1)
FileSystemBridge(155-170)packages/fs-bridge/src/bridges/http.ts (1)
kHttpBridgeSymbol(11-11)
packages/fs-bridge/test/bridges/node/node.test.ts (1)
tooling/moonbeam/src/esm-loader.mjs (1)
entries(44-44)
packages/fs-bridge/src/bridges/http.ts (1)
packages/path-utils/src/security.ts (1)
resolveSafePath(134-283)
packages/fs-bridge/src/utils.ts (3)
packages/fs-bridge/src/index.ts (3)
FileSystemBridgeOperations(20-20)BridgeBaseError(4-4)BridgeGenericError(7-7)packages/path-utils/src/index.ts (1)
PathUtilsBaseError(6-6)packages/fs-bridge/src/errors.ts (1)
BridgeGenericError(10-15)
packages/fs-bridge/test/security/http/recursive-listdir-security.test.ts (1)
packages/test-utils/src/msw.ts (2)
mockFetch(16-19)HttpResponse(11-11)
packages/fs-bridge/src/define.ts (1)
packages/fs-bridge/src/types.ts (2)
FileSystemBridgeObject(107-153)FileSystemBridge(155-170)
packages/fs-bridge/src/types.ts (1)
packages/fs-bridge/src/index.ts (2)
FileSystemBridgeArgs(16-16)FileSystemBridgeFactory(17-17)
packages/fs-bridge/test/security/http/encoded-attacks.test.ts (1)
packages/ucd-store-v2/src/core/context.ts (1)
basePath(78-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
🔇 Additional comments (38)
packages/fs-bridge/test/security/node/encoded-attacks.test.ts (1)
7-8: LGTM! Title casing corrected.The title now correctly capitalizes "URL" as a proper noun, with appropriate ESLint suppression.
packages/fs-bridge/eslint.config.js (1)
8-12: LGTM! Playground scripts appropriately excluded from linting rules.The addition of
"playgrounds/**"to the ignore patterns aligns with the new playground validation scripts introduced in this PR and allows them to use Node.js built-ins without triggering theno-restricted-importsrule.packages/fs-bridge/test/security/http/recursive-listdir-security.test.ts (5)
1-2: LGTM! Type reference enables custom matcher support.The reference directive provides type definitions for custom matchers like
toMatchErrorused later in the test.
6-6: LGTM! Import aligns with updated error handling.The
BridgeGenericErrorimport is required for the updated error expectation at line 69, where schema validation failures are now expected to throw this error type.
24-24: Path format updated to match HTTP bridge normalization.The leading slash format aligns with the HTTP bridge's path schema, which expects directory paths to start with
/and end with/.
42-71: LGTM! Defense-in-depth error handling implemented.The test now validates that schema validation acts as the first line of defense, catching malformed paths before they reach traversal checks. The encoded path
%2f%2e%2e%2f%2e%2e%2fetc%2fis rejected by the schema because it doesn't conform to the expected format (missing proper leading/trailing slashes), resulting in aBridgeGenericErrorwith "Invalid response schema" message rather than aPathTraversalError.
84-104: Path assertions updated consistently.Both the mock response and the assertion now use the normalized directory path format (
/subdir/), maintaining consistency with the HTTP bridge's path schema.packages/fs-bridge/test/security/http/encoded-attacks.test.ts (2)
8-9: LGTM! Title casing corrected.Consistent with the Node test file, "URL" is now properly capitalized as a proper noun with appropriate ESLint suppression.
128-142: LGTM! Variable introduction improves test maintainability.Extracting
basePathas a variable and using it in both the URL construction and error message assertion reduces duplication and makes the test more maintainable. The dynamic reference in the error message (line 142) ensures the assertion remains accurate if the base path changes.packages/fs-bridge/src/utils.ts (4)
14-14: LGTM! Import supports new MSW error handling.The
isMSWErrorutility is used at line 201 to detect and properly handle MSW-specific errors in tests.
184-198: LGTM! Error normalization uses native Error cause chain.The refactor improves clarity by:
- Renaming to
normalizedError(more descriptive thanknownError)- Using native
ErrorOptionswith{ cause: err }for error chaining, aligning with the updatedBridgeGenericErrorconstructor signature
200-208: LGTM! MSW error handling improves test debugging.Detecting and rethrowing MSW errors without wrapping ensures that mock-related failures in tests are surfaced with their original error messages, making test debugging significantly easier.
217-235: LGTM! Error handling correctly preserves Bridge error types.The
instanceof BridgeBaseErrorcheck at line 217 correctly detects all Bridge error subclasses (includingBridgeGenericError,BridgeFileNotFound, etc.) and rethrows them without wrapping. Only truly unexpected errors are wrapped inBridgeGenericErrorat lines 232-235, with the original error preserved as thecause.packages/fs-bridge/package.json (2)
29-29: LGTM! New errors export aligns with module structure.The new
./errorsexport correctly maps to the built errors module and aligns with the new entry intsdown.config.tsand the refactorederrors.tsfile.
47-48: LGTM! Playground scripts enable local testing.The playground scripts provide a convenient way to test the Node and HTTP bridge implementations during development.
packages/fs-bridge/tsdown.config.ts (1)
8-9: LGTM! Errors module entry aligns with public export.The new
./src/errors.tsentry correctly exposes the errors module, matching the public export inpackage.json.packages/fs-bridge/src/types.ts (1)
172-183: LGTM! Type extraction improves reusability and clarity.Extracting
FileSystemBridgeArgsas a separate exported type alias improves:
- Reusability: The type can now be imported and used by consumers
- Clarity: Separates the argument-shaping logic from the factory signature
- Maintainability: Single source of truth for argument type computation
The refactor preserves the existing conditional logic while making it more accessible.
packages/fs-bridge/src/errors.ts (1)
11-14: No breaking change - BridgeGenericError never exposed anoriginalErrorproperty.The refactor to native
ErrorOptionsis correct and safe. BridgeGenericError was never designed to store the error in a custom property; it only passed the cause through options. All existing instantiations (lines 122, 194, 232 in utils.ts) already use the{ cause: ... }pattern.The inconsistency with BridgeSetupError is intentional: BridgeSetupError maintains the
originalErrorproperty for backward compatibility while also using the standardcausefield (see lines 18, 20, 25). Test utilities already handle both patterns (error-matchers.ts lines 82–83).pnpm-workspace.yaml (1)
65-65: Verify compatibility with@luxass/[email protected].The version bump should be backward compatible per semver, but publicly available release notes for this version are unavailable. Since this package is bundled in the fs-bridge production bundle via
noExternal, confirm there are no breaking changes in the API exports used:createMockFetchandisMSWError/runtime-guards.packages/fs-bridge/test/security/http/excessive-encoding.test.ts (1)
34-37: Verify error expectation consistency across test cases.Similar to the node bridge tests, these tests expect the error message string
"Failed to decode path"for traversal attempts (lines 36, 68), while other tests in the same file expectFailedToDecodePathError(lines 22, 54). This pattern is consistent with the node bridge tests, but should be verified to ensure it reflects the actual error handling behavior.Also applies to: 66-69
packages/fs-bridge/src/guards.ts (1)
38-48: Excellent improvement to bridge identification.The symbol-based detection approach effectively addresses the previous concern about fragile string matching. Using
kHttpBridgeSymbolensures the check is robust against name changes and cannot be easily spoofed by custom bridges. The defensive double-check pattern (in fs&&=== true) is sound.packages/fs-bridge/test/bridges/node/node.test.ts (2)
425-535: LGTM! Comprehensive shape validation tests.The listdir shape consistency tests thoroughly validate the output contract:
- File entries have exactly the expected properties (type, name, path)
- Directory entries consistently include a children array
- Recursive mode properly populates nested structures
- Shape consistency is maintained across multiple entries
The defensive checks using
Object.keys()and type guards ensure the tests catch any deviations from the expected schema.
537-585: LGTM! Well-structured error handling tests.The error surface tests consistently verify that:
- Errors are properly wrapped in
BridgeGenericError- Original Node.js error codes (ENOENT, ENOTDIR) are preserved in the
causeproperty- The error wrapping pattern is uniform across different operations
The try-catch pattern with
expect.fail()clearly documents the expected behavior and provides helpful failure messages.packages/fs-bridge/src/define.ts (2)
2-2: LGTM! Symbol parameter enables runtime bridge identification.The addition of the optional
symbolproperty to thefsBridgeparameter aligns with the new symbol-based bridge identification pattern introduced in this PR (e.g.,kHttpBridgeSymbolandisBuiltinHttpBridge).Also applies to: 23-23
63-63: Verify type safety of symbol attachment pattern.The symbol attachment uses a double type assertion
(bridge as unknown as Record<symbol, boolean>)to bypass TypeScript's type checking. While this enables the runtime feature, it creates a gap between the type system and runtime behavior:
- The
FileSystemBridgetype doesn't declare symbol index signatures- Symbol properties won't be visible to TypeScript consumers
- Type safety is lost for this feature
Consider whether this is the intended design or if an alternative approach would be more type-safe:
- Option 1 (Current): Accept runtime-only symbols for internal identification
- Option 2: Extend
FileSystemBridgewith an index signature:[key: symbol]: boolean | undefined- Option 3: Use a WeakMap or separate registry for symbol-to-bridge mapping
Is the current runtime-only symbol pattern intentional, or should the
FileSystemBridgetype be extended to support symbol properties for better type safety?Also applies to: 79-82
packages/fs-bridge/src/index.ts (1)
12-12: LGTM! Public API expansion is clear and consistent.The new exports
isBuiltinHttpBridgeandFileSystemBridgeArgsenhance the public API:
isBuiltinHttpBridgeenables consumers to detect built-in HTTP bridgesFileSystemBridgeArgsprovides type-safe access to bridge factory parametersBoth additions follow existing naming conventions and integrate well with the PR's symbol-based identification pattern.
Also applies to: 16-16
packages/fs-bridge/test/bridges/http/http.test.ts (2)
135-135: LGTM! Consistent directory path convention.The trailing slash convention for directory paths (
"/subdir/"instead of"/subdir") standardizes the path representation across all fixtures. This change is applied consistently throughout the test expectations.Also applies to: 168-168, 207-207, 269-269, 294-294
528-775: LGTM! Excellent test coverage for schema validation and error handling.The new test suites add comprehensive coverage for critical scenarios:
- Schema validation (Zod): Tests enforce strict validation of listdir responses, catching missing/invalid fields early
- Fetch error handling: Covers network errors, server errors, and forbidden responses with appropriate assertions
- Content handling: Validates Content-Type agnosticism, edge cases (empty, large), and binary-like content
This significantly improves the robustness of the HTTP bridge implementation.
packages/fs-bridge/test/bridges/http/basepath.http.relative.test.ts (1)
177-178: LGTM! Fixture update aligns with path convention.The mock data paths have been updated to use absolute paths with leading slashes (
"/file1.txt"instead of"file1.txt"), consistent with the path format standardization across the HTTP bridge test suite.packages/fs-bridge/test/security/node/path-traversal.test.ts (1)
112-178: LGTM! Comprehensive security coverage for recursive listdir.The new "recursive listdir security" test suite adds critical security validation:
- Malicious directory names: Prevents traversal via
"../"paths in listdir operations- Encoded path components: Catches URL-encoded traversal attempts (
"%2e%2e/") that might bypass naive checks- Legitimate nested structures: Validates correct recursive behavior for safe paths
These tests complement the existing path-traversal prevention and strengthen the security posture of the Node bridge's listdir feature.
packages/fs-bridge/src/bridges/node.ts (2)
61-74: Excellent documentation of design decisions.The docblock clearly explains why the Node bridge uses a different validation approach than the HTTP bridge, documents the FSEntry contract, and references verification tests. This is helpful context for maintainers.
107-139: Recursive directory listing implementation is sound.The Map-based hierarchy building approach is efficient and correct. The use of
entry.parentPathis appropriate given Node.js version requirements, and the path normalization handles cross-platform differences properly.packages/fs-bridge/playgrounds/node-playground.ts (1)
46-255: Comprehensive test coverage for Node bridge operations.The test suite covers all bridge methods with appropriate edge cases including path prefixes, trailing slashes, recursive options, and error scenarios. This provides good validation of the bridge implementation.
packages/fs-bridge/playgrounds/http-playground.ts (1)
31-236: Excellent HTTP bridge test coverage.The test suite properly validates the read-only nature of the HTTP bridge, tests all supported operations, ensures mutating operations throw appropriately, and includes a realistic workflow test. The use of
assertCapabilityis a good pattern for validating optional capabilities.packages/fs-bridge/src/bridges/http.ts (4)
36-60: Debug logging enhancements improve observability.The added debug logs at key points (read attempts, rejections, fetch operations, errors, and successes) will help troubleshoot issues and understand bridge behavior in production. The log messages include appropriate context.
63-158: Comprehensive debug logging for listdir operations.The debug logs cover all major code paths including error conditions (404, 403, 500), schema validation failures, and recursive processing. This will significantly help with debugging and monitoring.
160-185: Exists method enhancements are well-implemented.The debug logging and MSW error handling (for testing purposes) improve both observability and testability. Using HEAD requests for existence checks is the appropriate HTTP method.
13-20: [rewritten review comment]
[classification tag]
… tests This is fine for now, since ucd-store-v2 will replace it very soon
Updated the `createFSEntry` function to ensure all paths have a leading slash and directories have a trailing slash. This change improves path consistency across the file system bridge.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ucd-store/test/maintenance/mirror.test.ts:
- Line 12: Multiple tests are being disabled with describe.todo() across the
suite (including the instance in describe.todo("store mirror") and 20+
occurrences in other test files), so annotate each disabled describe.todo() with
an inline tracking comment referencing the migration (e.g., add "// TODO:
Re-enable after ucd-store-v2 migration") immediately above or on the same line
as the describe.todo() call to make scope and intent explicit; ensure you update
each describe.todo() occurrence (including nested ones in init.test,
errors.test, factory.test, and this mirror.test's describe.todo) rather than
only a few files, and leave existing correct uses of mockStoreApi() and testdir
unchanged.
🧹 Nitpick comments (1)
packages/fs-bridge/src/bridges/node.ts (1)
47-53: Consider extracting shared trailing slash validation.The TODO is valid—the trailing slash validation logic (lines 48-53 and 153-158) is duplicated. A small helper like
assertNotTrailingSlash(path)would reduce duplication and centralize the logic.♻️ Optional: Extract shared validation
+function assertValidFilePath(path: string): void { + const trimmedPath = path.trim(); + if (trimmedPath.endsWith("/") && trimmedPath !== "/" && trimmedPath !== "./" && trimmedPath !== "../") { + throw new Error("Path ends with '/' - files don't have trailing slashes"); + } +} + async read(path) { - // TODO: duplicate code with write - refactor - // Reject file paths ending with / - files don't have trailing slashes - // Allow /, ./, and ../ as they are special directory references - const trimmedPath = path.trim(); - if (trimmedPath.endsWith("/") && trimmedPath !== "/" && trimmedPath !== "./" && trimmedPath !== "../") { - throw new Error("Cannot read file: path ends with '/'"); - } + assertValidFilePath(path);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/fs-bridge/src/bridges/node.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.tspackages/ucd-store/test/core/init.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/internal/files.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/maintenance/mirror.test.tspackages/ucd-store/test/maintenance/repair.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ucd-store/test/internal/files.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ucd-store/test/maintenance/analyze.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Import test utilities from #test-utils/* path aliases instead of @ucdjs/test-utils in test files to avoid build step requirements and prevent cyclic dependencies
Use testdir() from vitest-testdirs to create temporary test directories in filesystem tests
Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API in test files
Files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/ucd-store/test/maintenance/repair.test.tspackages/ucd-store/test/maintenance/mirror.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/core/init.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use File System Bridge Pattern (ucdjs/fs-bridge) abstraction for file system operations to support multiple backends (Node.js, HTTP, in-memory)
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API in test files
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/repair.test.tspackages/ucd-store/test/maintenance/mirror.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/core/init.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.ts
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/ucd-store/test/maintenance/repair.test.tspackages/ucd-store/test/maintenance/mirror.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/core/init.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.ts
📚 Learning: 2025-08-17T10:10:19.096Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.096Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/test/maintenance/clean.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Import test utilities from #test-utils/* path aliases instead of ucdjs/test-utils in test files to avoid build step requirements and prevent cyclic dependencies
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/fs-bridge/test/bridges/node/node.test.tspackages/ucd-store/test/maintenance/repair.test.tspackages/ucd-store/test/maintenance/mirror.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/core/init.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.ts
📚 Learning: 2025-08-23T05:20:36.940Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.940Z
Learning: In the ucdjs/ucd codebase using modern Vitest, the assert export from "vitest" works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that need compile-time type safety. The existing usage pattern assert(condition, message) throughout the test suite is working correctly.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/maintenance/repair.test.tspackages/ucd-store/test/maintenance/mirror.test.ts
📚 Learning: 2025-08-23T05:20:36.940Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.940Z
Learning: In the ucdjs/ucd codebase, Vitest's assert import works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that also need compile-time type safety.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/maintenance/repair.test.ts
📚 Learning: 2025-08-23T05:16:26.866Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:16:26.866Z
Learning: In the ucdjs/ucd codebase, the team prefers using `assert(condition, message)` over `expect(condition).toBeTruthy()` because assert provides TypeScript type narrowing while expect doesn't. This is important for maintaining type safety after runtime validation checks.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/capability-requirements.test.ts
📚 Learning: 2025-10-14T07:15:35.199Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 326
File: packages/shared/src/ucd-config.ts:34-58
Timestamp: 2025-10-14T07:15:35.199Z
Learning: In packages/shared/src/ucd-config.ts, the `getDefaultUCDEndpointConfig` function uses a build-time define `__UCD_ENDPOINT_DEFAULT_CONFIG__` that is replaced at build time by the bundler. The `??` operator is evaluated at build time, so the final code contains no runtime branching. Therefore, runtime unit tests for this function are not valuable—the build process itself handles the injection and fallback logic.
Applied to files:
packages/ucd-store/test/file-operations/capability-requirements.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store/test/core/errors.test.tspackages/ucd-store/test/core/factory.test.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use testdir() from vitest-testdirs to create temporary test directories in filesystem tests
Applied to files:
packages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/test/file-operations/file-paths.test.ts
📚 Learning: 2025-05-04T11:52:22.858Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Applied to files:
packages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-12-27T11:08:54.630Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T11:08:54.630Z
Learning: Use File System Bridge Pattern (ucdjs/fs-bridge) abstraction for file system operations to support multiple backends (Node.js, HTTP, in-memory)
Applied to files:
packages/fs-bridge/test/bridges/node/node.test.tspackages/ucd-store/test/core/factory.test.tspackages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-07-09T15:05:57.763Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 97
File: packages/utils/src/fs-bridge/node.ts:40-50
Timestamp: 2025-07-09T15:05:57.763Z
Learning: The `parentPath` property on `fs.Dirent` was added in Node.js v20.12, contrary to previous assumptions about it not being part of the Node.js API.
Applied to files:
packages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-07-29T06:02:32.422Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 175
File: packages/fs-bridge/test/bridges/node.test.ts:0-0
Timestamp: 2025-07-29T06:02:32.422Z
Learning: In the NodeFileSystemBridge security validation, path traversal detection occurs before critical system paths checks, which means absolute paths to critical system directories will always throw "Path traversal detected" rather than "Critical system path access denied", making it impossible to meaningfully test the critical system paths validation logic.
Applied to files:
packages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-06-14T05:20:20.149Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/validate.ts:115-130
Timestamp: 2025-06-14T05:20:20.149Z
Learning: Node.js 22 supports the `recursive` option in `fs.readdir()` (introduced in Node.js 18.17.0), which returns a flattened string array of all descendant files and directories when `recursive: true` is passed.
Applied to files:
packages/fs-bridge/test/bridges/node/node.test.tspackages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-06-29T11:20:13.668Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 87
File: packages/worker-shared/tsconfig.build.json:1-4
Timestamp: 2025-06-29T11:20:13.668Z
Learning: In the ucdjs/ucd project, the packages use tsdown instead of tsc for building libraries. The tsconfig.build.json files are primarily for IDE experience and type checking, not for direct compilation, so including "test" directories in these configs doesn't affect build output.
Applied to files:
packages/ucd-store/test/file-operations/file-tree.test.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: The `recursive` option for `fs.readdir()` was introduced in Node.js v20.1.0 and is available in Node.js 22. When `recursive: true` is passed, it will recursively traverse directories and return all file paths as a flat string array.
Applied to files:
packages/fs-bridge/src/bridges/node.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: In Node.js 22, the `recursive` option for `fs.readdir()` is supported and will recursively find all files and return them in a string array when set to true.
Applied to files:
packages/fs-bridge/src/bridges/node.ts
🧬 Code graph analysis (1)
packages/fs-bridge/src/bridges/node.ts (2)
packages/path-utils/src/security.ts (1)
resolveSafePath(134-283)packages/fs-bridge/src/types.ts (1)
FSEntry(16-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: typecheck
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
🔇 Additional comments (16)
packages/ucd-store/test/maintenance/clean.test.ts (1)
12-12: Test suite marked as pending for ucd-store-v2 migration.The conversion to
describe.todointentionally defers execution of the clean operation tests while preserving their structure for future updates. This is consistent with the broader migration pattern across the ucd-store test suite.packages/ucd-store/test/core/init.test.ts (1)
21-21: Initialization test suites marked as pending.The conversion of these nested describe blocks to
describe.tododefers the store initialization, loading, and version diff tests while preserving their definitions for the v2 migration.Also applies to: 42-42, 156-156, 257-257
packages/ucd-store/test/file-operations/capability-requirements.test.ts (1)
8-8: Capability requirements tests marked as pending.The test suite validating filesystem bridge capabilities is deferred during the migration. This preserves validation logic for re-enablement after ucd-store-v2 updates.
packages/ucd-store/test/core/factory.test.ts (1)
9-9: Factory function tests marked as pending.The test suites for
createUCDStore,createNodeUCDStore, andcreateHTTPUCDStoreare deferred during the v2 migration, preserving validation of the public factory APIs for future re-enablement.Also applies to: 15-15, 84-84, 154-154
packages/ucd-store/test/core/errors.test.ts (1)
13-14: Error class tests marked as pending.The test suites validating all custom error classes (
UCDStoreGenericError,UCDStoreFileNotFoundError, etc.) are deferred during the migration, preserving comprehensive error handling validation for future re-enablement.Also applies to: 43-43, 71-71, 85-85, 147-147, 180-180
packages/fs-bridge/test/bridges/node/node.test.ts (6)
124-128: LGTM! Path format expectations align with new formatting conventions.The updated expectations correctly reflect the new path formatting contract: files with leading slashes (
/file1.txt), directories with leading and trailing slashes (/subdir/), and directories include an emptychildrenarray in non-recursive mode.
150-154: LGTM! Recursive listing expectations updated correctly.The flattened path expectations now include leading slashes, consistent with the new path formatting contract.
425-535: Well-structured shape consistency tests.These tests provide valuable coverage ensuring that
FSEntryobjects conform to the expected contract. The validation of property keys (lines 522-523, 528-529) is a good defensive measure against schema drift.
537-585: Good error surface coverage ensuring cause preservation.These tests verify that
BridgeGenericErrorproperly wraps underlying Node.js errors while preserving the original error code in thecauseproperty. This is valuable for consumers who need to handle specific error types.
608-613: LGTM! Complex workflow expectations updated correctly.The project workspace test now expects paths with leading slashes and trailing slashes for directories, consistent with the updated
FSEntrycontract.
309-314: No changes needed: path format is correct for non-recursive listdir.The expected path
/nested.txtis the correct behavior. For non-recursivelistdiroperations, paths are returned as just the entry name with a leading slash (line 89 of the implementation usesentry.nameas the path base). This differs intentionally from recursive mode, which returns full relative paths. The test is correctly aligned with the implementation.packages/fs-bridge/src/bridges/node.ts (5)
61-74: Excellent documentation explaining the validation parity decision.This JSDoc clearly explains why the Node bridge doesn't use Zod schema validation for
listdiroutput (trusted local data vs. untrusted remote JSON), while documenting the shape contract that must be maintained. This addresses the concern from a previous review about bridge inconsistency.
78-86: Clean path formatting utility.The
formatEntryPathfunction centralizes the path formatting logic, ensuring consistent leading/trailing slash handling across all FSEntry objects. This is a good single-responsibility helper.
88-103: LGTM! Clear FSEntry construction with explicit path control.The
createFSEntryfunction properly handles both recursive (explicitrelativePath) and non-recursive (fallback toentry.name) modes while ensuring consistent path formatting.
118-124: Good backward compatibility handling forentry.parentPath.The fallback to
entry.pathensures compatibility with Node.js versions before v20.12 whereparentPathwasn't available. Based on learnings,parentPathwas added in Node.js v20.12.
129-133: The map key consistency is correct. FSEntry entries are stored withnormalizedPathas keys (e.g.,"parent/child.txt"), and parent lookups usepath.dirname(entryPath)which returns the matching format (e.g.,"parent"). Root entries are properly skipped via theparentPath !== "."condition. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/cli/test/cmd/store/analyze.test.ts:
- Line 198: Add a short comment immediately above the it.todo("should show
complete status for store with all files", ...) test explaining why this test
was disabled and what must change to re-enable it (e.g., flaky behavior,
dependency missing, environment requirement, or a tracking issue ID like #1234);
include the expected conditions or a link to a ticket and an optional TODO with
an owner or ETA so future maintainers know when to restore the original it(...)
test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/test/cmd/store/analyze.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Import test utilities from #test-utils/* path aliases instead of @ucdjs/test-utils in test files to avoid build step requirements and prevent cyclic dependencies
Use testdir() from vitest-testdirs to create temporary test directories in filesystem tests
Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API in test files
Files:
packages/cli/test/cmd/store/analyze.test.ts
🧠 Learnings (1)
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
packages/cli/test/cmd/store/analyze.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: typecheck
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const url = joinURL( | ||
| baseUrl.origin, | ||
| resolveSafePath(baseUrl.pathname, path), | ||
| resolveSafePath(baseUrl.pathname, `/${path}`), |
There was a problem hiding this comment.
Prepending / to path may cause double slashes if path already starts with /
| resolveSafePath(baseUrl.pathname, `/${path}`), | |
| resolveSafePath(baseUrl.pathname, path), |
|
|
||
| return { | ||
| async read(path) { | ||
| // TODO: duplicate code with write - refactor |
There was a problem hiding this comment.
Duplicate validation logic in read and write
Per TODO, refactor trailing slash validation into shared helper function
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!
There was a problem hiding this comment.
Additional Comments (1)
-
packages/fs-bridge/src/errors.ts, line 17-27 (link)style: Inconsistent error handling patterns -
BridgeSetupErrormaintains bothoriginalErrorproperty andcause, whileBridgeGenericErroronly usescause. Should all error classes use the same pattern for consistency?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!
38 files reviewed, 2 comments
| type: "directory", | ||
| name: "malicious", | ||
| path: "%2e%2e%2f%2e%2e%2fetc", // Encoded traversal | ||
| path: "%2f%2e%2e%2f%2e%2e%2fetc%2f", // Encoded traversal - rejected by schema |
There was a problem hiding this comment.
logic: The encoded path starts and ends with %2f (/) which actually conforms to the schema - this may not trigger the expected schema validation error. Should this encoded path be designed to fail schema validation, or should it test traversal detection after decoding?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/fs-bridge/test/security/http/recursive-listdir-security.test.ts
Line: 54:54
Comment:
**logic:** The encoded path starts and ends with %2f (/) which actually conforms to the schema - this may not trigger the expected schema validation error. Should this encoded path be designed to fail schema validation, or should it test traversal detection after decoding?
How can I resolve this? If you propose a fix, please make it concise.
🔗 Linked issue
📚 Description
This PR is a combination of changes related to fs-bridge, but also playgrounds from #440.
This will help close #420.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.