Sync fork with upstream/main (Effect services, dep bump)#42
Merged
aaditagrawal merged 6 commits intomainfrom Mar 31, 2026
Merged
Sync fork with upstream/main (Effect services, dep bump)#42aaditagrawal merged 6 commits intomainfrom
aaditagrawal merged 6 commits intomainfrom
Conversation
…lling Integrates 3 upstream commits: - Extract project/workspace functionality to Effect services (pingdotgg#1524) - Replace wait-on with internal desktop resource polling (pingdotgg#1600) - Bump Effect catalog dependencies (pingdotgg#1594) Conflict resolution: deleted fork's workspaceEntries.ts (extracted upstream), accepted upstream's gitCoreLayer extraction in serverLayers, preserved fork's SessionTextGenerationLive for GitManager, moved runPromise definition earlier for favicon route handler.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates workspace indexing, path handling, and workspace file writes into Effect-based services; removes the old imperative workspaceEntries module and tests; adds WorkspacePaths/WorkspaceEntries/WorkspaceFileSystem/ProjectFaviconResolver layers and tests; enhances GitCore with stdin/truncation-aware outputs; replaces wait-on with waitForResources; and rewires server layer composition to provide the new services. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkspaceEntries
participant Cache
participant GitCore
participant FileSystem
Client->>WorkspaceEntries: search({cwd, query, limit})
WorkspaceEntries->>Cache: lookup(cwd)
alt cache hit
Cache-->>WorkspaceEntries: index
else cache miss
WorkspaceEntries->>GitCore: isInsideWorkTree(cwd)
alt inside git
GitCore-->>WorkspaceEntries: true
WorkspaceEntries->>GitCore: listWorkspaceFiles(cwd)
GitCore-->>WorkspaceEntries: paths (+ truncated)
else not git or git missing
WorkspaceEntries->>FileSystem: traverse workspace (concurrent)
FileSystem-->>WorkspaceEntries: paths (+ truncated)
end
WorkspaceEntries->>Cache: store index
end
WorkspaceEntries-->>Client: ranked { entries, truncated }
sequenceDiagram
participant Client
participant WorkspaceFileSystem
participant FileSystem
participant WorkspaceEntries
Client->>WorkspaceFileSystem: writeFile({cwd, relativePath, contents})
WorkspaceFileSystem->>FileSystem: makeDirectory(..., recursive)
WorkspaceFileSystem->>FileSystem: writeFileString(target, contents)
FileSystem-->>WorkspaceFileSystem: ok
WorkspaceFileSystem->>WorkspaceEntries: invalidate(cwd)
WorkspaceFileSystem-->>Client: { relativePath }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/server/src/workspace/Layers/WorkspaceEntries.ts (1)
463-465: Return plainProjectEntrys from the public result.
candidate.entryis aSearchableWorkspaceEntry, sonormalizedPathandnormalizedNameescape the layer here. Keeping those fields internal avoids widening theProjectSearchEntriesResultpayload with cache-only data.♻️ Suggested change
return { - entries: rankedEntries.map((candidate) => candidate.entry), + entries: rankedEntries.map(({ entry }) => ({ + path: entry.path, + kind: entry.kind, + ...(entry.parentPath ? { parentPath: entry.parentPath } : {}), + })), truncated: index.truncated || matchedEntryCount > limit, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/workspace/Layers/WorkspaceEntries.ts` around lines 463 - 465, The public result currently returns SearchableWorkspaceEntry objects (via rankedEntries.map(candidate => candidate.entry)), leaking internal fields like normalizedPath and normalizedName; instead, transform each candidate.entry into a plain ProjectEntry before returning so ProjectSearchEntriesResult.entries contains only the public fields. Update the return to map rankedEntries to a new object (selecting the ProjectEntry properties) and keep truncated logic unchanged; target the code around rankedEntries and the function in WorkspaceEntries that builds the ProjectSearchEntriesResult to perform this projection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/scripts/wait-for-resources.mjs`:
- Around line 83-85: The current check in waitForResources only ensures tcpPort
is a positive integer but allows values >65535; update the validation in the
waitForResources function to require Number.isInteger(tcpPort) && tcpPort >= 1
&& tcpPort <= 65535 and throw a clear TypeError (e.g., "waitForResources
requires a tcpPort in range 1–65535") when it fails so invalid ports are
rejected before net.createConnection is called.
In `@apps/server/src/project/Layers/ProjectFaviconResolver.ts`:
- Around line 66-83: The current findExistingFile accepts symlinked candidates
because fileSystem.stat follows symlinks; update the logic to detect and reject
or canonicalize symlinks before accepting a candidate: use fileSystem.lstat (or
equivalent) to detect if a candidate is a symlink (check the lstat result for
symbolic link type), and if it is either reject it outright or call
fileSystem.realpath and then verify the resolved path is within the project
using isPathWithinProject; only return the candidate when it is not a symlink
(or its resolved realpath passes the in-project check). Update the code paths
around findExistingFile and fileSystem.stat to use lstat/realpath checks, and
add a regression test that creates a symlinked favicon pointing outside the repo
to ensure it's rejected.
In `@apps/server/src/workspace/Layers/WorkspaceEntries.ts`:
- Around line 324-327: The current Effect.catchIf around Effect.catchIf(() =>
relativeDir.length > 0, () => Effect.succeed({ relativeDir, dirents: null })) is
swallowing all nested readdir errors (EACCES, EIO, etc.) and treating them as a
clean "not found" miss; change the handler so it only falls back to {
relativeDir, dirents: null } for not-found/not-directory errors (e.g.,
ENOENT/ENOTDIR) by inspecting the thrown error/code inside the catch predicate,
and for any other error propagate or convert it into a partial/truncated result
(set truncated: true on the final index result) so failures like EACCES and EIO
are not hidden. Ensure you update the code paths that compute truncated to
reflect this new branch and reference Effect.catchIf, relativeDir, dirents, and
the truncated flag when making the change.
- Around line 199-203: The helper isPathInIgnoredDirectory only checks the first
path segment so nested ignored dirs like .next or dist slip through for
Git-backed entries; update isPathInIgnoredDirectory to split the path into all
segments (e.g., relativePath.split("/").filter(Boolean)) and return true if any
segment is present in IGNORED_DIRECTORY_NAMES, ensuring it normalizes/ignores
empty segments (and considers posix separators) so both filesystem and
Git-backed path checks (the callers around the Git-backed indexing logic) behave
consistently.
In `@apps/server/src/workspace/Layers/WorkspacePaths.ts`:
- Around line 62-80: The current containment check in WorkspacePaths (using
toPosixRelativePath and string compares) can be bypassed by symlinked parent
directories; after computing absolutePath, canonicalize both the workspace root
and the target path (or at minimum resolve the existing parent chain) using
fs.realpath or iteratively lstat the parent chain to detect/resolve symlinks,
then compare the canonical realpath of the target against the canonical
workspaceRoot; if the resolved target is outside the workspaceRoot, yield
WorkspacePathOutsideRootError (same payload), otherwise return the resolved
absolutePath and relativePath. Also ensure this prevents
WorkspaceFileSystem.writeFile following symlinks outside the root by validating
parents before allowing writes.
- Around line 30-49: normalizeWorkspaceRoot currently trims and then resolves
the input which makes a whitespace-only workspaceRoot become "" and
path.resolve("") resolves to the process CWD; add an explicit early check in the
Effect.fn body to reject blank inputs: after obtaining const raw = workspaceRoot
and before calling expandHomePath/path.resolve, if raw.trim() === "" return
yield* new WorkspaceRootNotExistsError({ workspaceRoot: raw,
normalizedWorkspaceRoot: raw }); this ensures normalizeWorkspaceRoot rejects
whitespace-only roots instead of resolving them to the server CWD; keep existing
WorkspaceRootNotDirectoryError/exists checks unchanged.
In `@apps/server/src/wsServer.ts`:
- Around line 748-753: The error path wrapping workspaceFileSystem.writeFile
currently uses cause.message which is undefined for WorkspaceFileSystemError;
update the mapping inside Effect.mapError to detect WorkspaceFileSystemError
(the error from workspaceFileSystem.writeFile) and include its detail field in
the RouteRequestError message (e.g., use cause.detail when the error is
WorkspaceFileSystemError, otherwise fall back to cause.message or a generic
message) so clients receive the real failure detail.
---
Nitpick comments:
In `@apps/server/src/workspace/Layers/WorkspaceEntries.ts`:
- Around line 463-465: The public result currently returns
SearchableWorkspaceEntry objects (via rankedEntries.map(candidate =>
candidate.entry)), leaking internal fields like normalizedPath and
normalizedName; instead, transform each candidate.entry into a plain
ProjectEntry before returning so ProjectSearchEntriesResult.entries contains
only the public fields. Update the return to map rankedEntries to a new object
(selecting the ProjectEntry properties) and keep truncated logic unchanged;
target the code around rankedEntries and the function in WorkspaceEntries that
builds the ProjectSearchEntriesResult to perform this projection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b093ae3d-1583-48aa-9374-f8b9b8fb3de9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
apps/desktop/package.jsonapps/desktop/scripts/dev-electron.mjsapps/desktop/scripts/wait-for-resources.mjsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Services/GitCore.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/project/Layers/ProjectFaviconResolver.test.tsapps/server/src/project/Layers/ProjectFaviconResolver.tsapps/server/src/project/Services/ProjectFaviconResolver.tsapps/server/src/projectFaviconRoute.test.tsapps/server/src/projectFaviconRoute.tsapps/server/src/serverLayers.tsapps/server/src/workspace/Layers/WorkspaceEntries.test.tsapps/server/src/workspace/Layers/WorkspaceEntries.tsapps/server/src/workspace/Layers/WorkspaceFileSystem.test.tsapps/server/src/workspace/Layers/WorkspaceFileSystem.tsapps/server/src/workspace/Layers/WorkspacePaths.test.tsapps/server/src/workspace/Layers/WorkspacePaths.tsapps/server/src/workspace/Services/WorkspaceEntries.tsapps/server/src/workspace/Services/WorkspaceFileSystem.tsapps/server/src/workspace/Services/WorkspacePaths.tsapps/server/src/workspaceEntries.chunking.test.tsapps/server/src/workspaceEntries.test.tsapps/server/src/workspaceEntries.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tspackage.json
💤 Files with no reviewable changes (3)
- apps/server/src/workspaceEntries.chunking.test.ts
- apps/server/src/workspaceEntries.test.ts
- apps/server/src/workspaceEntries.ts
Comment on lines
+83
to
+85
| if (!Number.isInteger(tcpPort) || tcpPort <= 0) { | ||
| throw new TypeError("waitForResources requires a positive integer tcpPort"); | ||
| } |
There was a problem hiding this comment.
Validate full TCP port range (1–65535).
Line 83 currently allows values above 65535. That can fail later in createConnection with a low-level socket error instead of a clear argument error.
🔧 Proposed fix
- if (!Number.isInteger(tcpPort) || tcpPort <= 0) {
+ if (!Number.isInteger(tcpPort) || tcpPort <= 0 || tcpPort > 65_535) {
throw new TypeError("waitForResources requires a positive integer tcpPort");
}What is the valid TCP port range accepted by Node.js net.createConnection()?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/scripts/wait-for-resources.mjs` around lines 83 - 85, The
current check in waitForResources only ensures tcpPort is a positive integer but
allows values >65535; update the validation in the waitForResources function to
require Number.isInteger(tcpPort) && tcpPort >= 1 && tcpPort <= 65535 and throw
a clear TypeError (e.g., "waitForResources requires a tcpPort in range 1–65535")
when it fails so invalid ports are rejected before net.createConnection is
called.
Comment on lines
+324
to
+327
| Effect.catchIf( | ||
| () => relativeDir.length > 0, | ||
| () => Effect.succeed({ relativeDir, dirents: null }), | ||
| ), |
There was a problem hiding this comment.
Don't swallow every nested readdir failure as a clean miss.
This catches EACCES, EIO, and any other non-root readdir error the same way as a disappeared directory, then the final index still reports truncated: false. Please restrict the fallback to not-found/not-directory races, or mark the index as partial when a nested read fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/workspace/Layers/WorkspaceEntries.ts` around lines 324 - 327,
The current Effect.catchIf around Effect.catchIf(() => relativeDir.length > 0,
() => Effect.succeed({ relativeDir, dirents: null })) is swallowing all nested
readdir errors (EACCES, EIO, etc.) and treating them as a clean "not found"
miss; change the handler so it only falls back to { relativeDir, dirents: null }
for not-found/not-directory errors (e.g., ENOENT/ENOTDIR) by inspecting the
thrown error/code inside the catch predicate, and for any other error propagate
or convert it into a partial/truncated result (set truncated: true on the final
index result) so failures like EACCES and EIO are not hidden. Ensure you update
the code paths that compute truncated to reflect this new branch and reference
Effect.catchIf, relativeDir, dirents, and the truncated flag when making the
change.
Security: - Reject symlinked favicon candidates via realPath resolution - Resolve real paths before workspace containment checks to prevent symlink-based directory traversal - Reject blank workspace roots before path.resolve falls back to CWD Correctness: - Check all path segments against IGNORED_DIRECTORY_NAMES, not just first - Only swallow ENOENT/ENOTDIR in nested readdir, propagate EACCES/EIO - Validate TCP port range 1-65535 in desktop resource polling - Use cause.detail for WorkspaceFileSystemError in writeFile error path
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/workspace/Layers/WorkspaceEntries.ts`:
- Around line 199-203: The current isPathInIgnoredDirectory(relativePath) treats
the basename like an ancestor and filters files named e.g. "build" or "dist";
change it to only check ancestor directory segments by splitting on "/",
filtering empty segments, removing the last segment (pop) before testing
IGNORED_DIRECTORY_NAMES, and returning whether any remaining segment matches;
leave the existing directory-only guards as-is. Also update the other duplicated
segment-checks in this file that use the same pattern so they also exclude the
final path segment before matching.
- Around line 76-80: normalizeQuery currently strips all leading dots because
replace(/^[@./]+/, "") removes a lone leading '.' (breaking dotfile queries like
".env"); update the replace call inside normalizeQuery so it only removes
path-like prefixes (e.g., "./", "../", or leading "/" and optionally a leading
"@") while preserving a single leading dot that is part of the filename—i.e.,
change the regex used in the replace in normalizeQuery to target "./" or "../"
sequences and leading slashes/@ specifically instead of any leading dots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458370f5-2854-4564-83e2-d26814c3ce1d
📒 Files selected for processing (5)
apps/desktop/scripts/wait-for-resources.mjsapps/server/src/project/Layers/ProjectFaviconResolver.tsapps/server/src/workspace/Layers/WorkspaceEntries.tsapps/server/src/workspace/Layers/WorkspacePaths.tsapps/server/src/wsServer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/scripts/wait-for-resources.mjs
- apps/server/src/project/Layers/ProjectFaviconResolver.ts
- apps/server/src/workspace/Layers/WorkspacePaths.ts
- apps/server/src/wsServer.ts
- normalizeQuery: preserve leading dot for dotfile queries (.env) instead of stripping it with the path-prefix regex - isPathInIgnoredDirectory: only check ancestor segments, not the file basename (files named build/dist are legitimate) - Git index budget: cap directory entries to half the budget so derived directories can't starve file entries
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
2 participants
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.
Summary
Upstream Changes
workspaceEntries.tsdeleted (functionality moved to services).wait-ondependency.effectand related packages.Conflict Resolution (4 files)
workspaceEntries.ts— deleted (extracted to Effect services upstream)serverLayers.ts— accepted upstream'sgitCoreLayerextraction, kept fork'sSessionTextGenerationLivefor GitManagerGitCore.test.ts— accepted upstream's test helper additions andrunPromisepatternbun.lock— regenerated from upstreamwsServer.ts— movedrunPromisedefinition earlier so favicon route handler has service contextTest Plan
bun typecheck— 0 errorsbun fmt/bun lint— cleanSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores