feat: introduce experimental Workspace support with filesystem, sandb…#1025
feat: introduce experimental Workspace support with filesystem, sandb…#1025
Conversation
🦋 Changeset detectedLatest commit: 0d28dc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds an experimental Workspace subsystem: filesystem backends (in-memory, Node, composite), sandbox providers (Local, E2B, Daytona), BM25/vector search, skills discovery and prompt hooks, toolkit factories, Agent/VoltAgent workspace wiring, server endpoints, example app, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Agent as Agent
participant Workspace as Workspace
participant FS as Filesystem
participant SB as Sandbox
participant Search as Search
participant Skills as Skills
User->>Agent: init(agentOptions { workspace })
Agent->>Workspace: init()
Workspace->>FS: init()
Workspace->>SB: init()
Workspace->>Search: init()
Workspace->>Skills: init()
User->>Agent: request (tool call)
Agent->>Workspace: buildWorkspaceToolkits()
Workspace-->>Agent: toolkits
alt filesystem tool
Agent->>FS: read/write/list/grep(...)
FS-->>Agent: result
else sandbox tool
Agent->>SB: execute_command(...)
SB-->>Agent: stdout/stderr/exit
else search tool
Agent->>Search: workspace_search(query)
Search-->>Agent: results
else skills tool
Agent->>Skills: workspace_list_skills()
Skills-->>Agent: skills list
end
Agent->>User: respond(with tool outputs)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
This comment has been minimized.
This comment has been minimized.
Deploying voltagent with
|
| Latest commit: |
0d28dc7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b9ea5ce7.voltagent.pages.dev |
| Branch Preview URL: | https://feat-add-workspace.voltagent.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 80 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/workspace/filesystem/backends/composite.ts">
<violation number="1" location="packages/core/src/workspace/filesystem/backends/composite.ts:31">
P1: Potential double-slash in path when route prefix doesn't end with `/`. If `prefix` is `/uploads` and `key` is `/uploads/file.txt`, `suffix` becomes `/file.txt` and `strippedKey` becomes `//file.txt`. Consider checking if suffix already starts with `/` before prepending one.</violation>
</file>
<file name="packages/core/src/workspace/sandbox/local.ts">
<violation number="1" location="packages/core/src/workspace/sandbox/local.ts:360">
P1: Inconsistent network isolation defaults between sandbox providers. `sandbox-exec` blocks network by default (when `allowNetwork` is undefined), but `bwrap` allows network by default. For consistent and secure behavior, the condition should block network unless explicitly allowed.</violation>
</file>
<file name="packages/server-elysia/src/routes/agent.routes.ts">
<violation number="1" location="packages/server-elysia/src/routes/agent.routes.ts:280">
P2: The workspace skills route can return a 400 (invalid refresh), but the response schema omits 400. Add a 400 response mapping to avoid schema/OpenAPI mismatches.</violation>
</file>
<file name="examples/with-workspace/workspace/notes/omer.txt">
<violation number="1" location="examples/with-workspace/workspace/notes/omer.txt:14">
P3: The notes file appears to contain corrupted/duplicated text (merged sentences and stray characters). This looks accidental and makes the example workspace data hard to read or use.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for (const [prefix, backend] of this.sortedRoutes) { | ||
| if (key.startsWith(prefix)) { | ||
| const suffix = key.substring(prefix.length); | ||
| const strippedKey = suffix ? `/${suffix}` : "/"; |
There was a problem hiding this comment.
P1: Potential double-slash in path when route prefix doesn't end with /. If prefix is /uploads and key is /uploads/file.txt, suffix becomes /file.txt and strippedKey becomes //file.txt. Consider checking if suffix already starts with / before prepending one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/workspace/filesystem/backends/composite.ts, line 31:
<comment>Potential double-slash in path when route prefix doesn't end with `/`. If `prefix` is `/uploads` and `key` is `/uploads/file.txt`, `suffix` becomes `/file.txt` and `strippedKey` becomes `//file.txt`. Consider checking if suffix already starts with `/` before prepending one.</comment>
<file context>
@@ -0,0 +1,291 @@
+ for (const [prefix, backend] of this.sortedRoutes) {
+ if (key.startsWith(prefix)) {
+ const suffix = key.substring(prefix.length);
+ const strippedKey = suffix ? `/${suffix}` : "/";
+ return [backend, strippedKey];
+ }
</file context>
| success: t.Literal(true), | ||
| data: WorkspaceInfoSchema, | ||
| }), | ||
| 404: ErrorSchema, |
There was a problem hiding this comment.
P2: The workspace skills route can return a 400 (invalid refresh), but the response schema omits 400. Add a 400 response mapping to avoid schema/OpenAPI mismatches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-elysia/src/routes/agent.routes.ts, line 280:
<comment>The workspace skills route can return a 400 (invalid refresh), but the response schema omits 400. Add a 400 response mapping to avoid schema/OpenAPI mismatches.</comment>
<file context>
@@ -229,4 +258,169 @@ export function registerAgentRoutes(app: Elysia, deps: ServerProviderDeps, logge
+ success: t.Literal(true),
+ data: WorkspaceInfoSchema,
+ }),
+ 404: ErrorSchema,
+ 500: ErrorSchema,
+ },
</file context>
| 404: ErrorSchema, | |
| 400: ErrorSchema, | |
| 404: ErrorSchema, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@examples/with-workspace/workspace/notes/omer.txt`:
- Around line 1-2163: The file omer.txt contains accidental repeated debug/test
output (the "Segment Bazında Toplam MRR..." block repeated with garbled suffixes
like "300eSegment"/"300gSegment") adding ~2000 corrupted lines; remove the noisy
repetitions and replace the file with a single clean block (or delete it
entirely) so only the intended note content remains, then amend the commit.
Locate the repeated block in omer.txt, collapse it to one correct instance
(matching the format used in customers_insight.txt / meeting-notes.md) or remove
the file, update the commit (git add/commit --amend or create a cleanup commit)
and ensure similar debug outputs are not reintroduced.
In `@packages/core/src/planagent/filesystem/index.ts`:
- Around line 95-109: The code in setWorkspaceSpanAttributes uses an unsafe cast
(value as never) when calling toolSpan.setAttribute, bypassing TypeScript's type
checking; change this to use the proper OpenTelemetry AttributeValue type:
import AttributeValue from '@opentelemetry/api' (or the correct export) and cast
value to AttributeValue (e.g., toolSpan.setAttribute(key, value as
AttributeValue)); additionally guard against non-primitive values by either
skipping objects/functions or converting them to strings (JSON.stringify) before
casting so only string|number|boolean|Array<string|number|boolean> are passed to
toolSpan.setAttribute.
In `@packages/core/src/voltagent.ts`:
- Around line 68-74: The shutdown() method must call Workspace.destroy() to
clean up sandbox/search/skills/filesystem resources created when a workspace is
set (see registry.setGlobalWorkspace in the constructor). Modify shutdown() to
retrieve the global workspace via this.registry.getGlobalWorkspace(), and if
present await workspace.destroy() before the final log/exit; ensure you await
the async destroy() call so cleanup completes before shutdown finishes.
In `@packages/core/src/workspace/filesystem/backends/composite.ts`:
- Around line 215-236: The sub-backend returns filesUpdate keys using the
stripped backend paths, but write/edit/delete currently return those un-remapped
keys to callers; update each of write, edit, and delete to remap
result.filesUpdate keys back to the composite (mounted) paths before returning.
After calling backend.write/edit/delete (in functions write, edit, delete), if
the returned result has a filesUpdate object, transform its keys by prefixing
the original composite mount path (reconstruct from the incoming filePath and
the strippedKey returned by getBackendAndKey, or capture the mount prefix from
getBackendAndKey if available) so keys become the original composite paths (use
posix path joining/normalization to preserve leading slashes), and return the
modified result. Ensure you keep all other result fields unchanged and apply the
same remapping pattern used in lsInfo/stat/grepRaw/globInfo.
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 592-601: The grepRaw and fallbackSearch functions currently call
new RegExp(pattern) on untrusted input which risks ReDoS when fallbackSearch
executes the regex in-process; to fix, add a pre-validation step in grepRaw and
fallbackSearch that rejects or sanitizes overly complex patterns (e.g., enforce
max pattern length, disallow nested quantifiers/evil constructs or run a
safe-regex check), and if a pattern is deemed unsafe, either reject with an
informative error or fall back to a non-regex search (e.g., treat pattern as a
literal and use indexOf) before executing file-wide line matching; ensure you
wrap RegExp creation/execution in try/catch, avoid unbounded backtracking by
limiting target line length and total matches processed, and apply the same
protection to the fallbackSearch code path referenced in the diff.
- Around line 483-497: In edit(), the SUPPORTS_NOFOLLOW branch uses
fs.stat(resolvedPath) which follows symlinks and can lead to an ELOOP when
opening with O_NOFOLLOW; change this to use fs.lstat(resolvedPath) (same as the
else branch) and ensure the subsequent check mirrors the else-branch behavior:
if lstat indicates a symlink or not a file, return the same "Error: Symlinks are
not allowed" / "Error: File '... ' not found" semantics for
resolvedPath/filePath so symlinks are detected consistently before attempting to
open with O_NOFOLLOW.
In `@packages/core/src/workspace/filesystem/utils.ts`:
- Around line 207-218: The grepMatchesFromFiles function is vulnerable to ReDoS
because it compiles a user-supplied pattern directly; before calling new
RegExp(pattern) validate the pattern with a safety check (e.g., use
safe-regex2.isSafe(pattern)) and/or enforce a maximum pattern length (e.g.,
reject patterns over a defined threshold) and return a clear error string
instead of compiling; update the RegExp creation block (references:
grepMatchesFromFiles, pattern, regex) to first run the safety/length checks and
only proceed to new RegExp if the pattern is deemed safe.
In `@packages/core/src/workspace/index.ts`:
- Around line 161-197: The destroy() race: add logic to await any in-flight
initPromise at the start of destroy() (e.g., if (this.initPromise) await
this.initPromise.catch(() => {})) so destroy won't race with the init IIFE, then
proceed to tear down components; additionally, make init()'s IIFE resilient by
checking this.status !== "destroyed" before calling subsystem methods and before
setting this.status = "ready" (or bail out if destroyed) to prevent init from
starting or overwriting status after a destroy; reference methods/fields:
init(), destroy(), initPromise, status, filesystem, sandbox, searchService,
skills.
In `@packages/core/src/workspace/sandbox/local.ts`:
- Around line 256-279: The bwrap profile generator in
packages/core/src/workspace/sandbox/local.ts uses an opposite default network
policy to sandbox-exec; find the bwrap branch where profile lines are assembled
(look for the code that pushes "(allow network*)" or handles
options.allowNetwork) and change it to deny-by-default: remove unconditional
"(allow network*)" and instead add "(allow network*)" only when
options.allowNetwork is true, and if necessary add an explicit deny like "(deny
network*)" when options.allowNetwork is false to mirror sandbox-exec behavior;
update any comments/tests referring to default network allowance accordingly.
In `@packages/core/src/workspace/sandbox/toolkit.ts`:
- Line 194: Replace Zod v4 coercion syntax with Zod v3 coercion: change any
schema fields using z.number({ coerce: true }) to z.coerce.number();
specifically update the timeout_ms schema line that currently reads z.number({
coerce: true }).optional().describe("Timeout in milliseconds") to
z.coerce.number().optional().describe("Timeout in milliseconds"), and do the
same for the other occurrences of z.number({ coerce: true }) found in the
search-related schemas (the search request/params schemas referenced in this
diff).
In `@packages/sandbox-daytona/src/index.ts`:
- Around line 108-115: The cached promise daytonaModulePromise can hold a
rejected import and prevent retries; modify loadDaytonaModule so that it does
not permanently cache a failing import: when calling import("@daytonaio/sdk")
(inside loadDaytonaModule) wrap the dynamic import in a try/catch or attach a
.catch handler that clears daytonaModulePromise (set it back to undefined)
before rethrowing the error, or assign the import to a local/temp promise and
only store it into daytonaModulePromise after it resolves successfully;
reference daytonaModulePromise and loadDaytonaModule to implement this
retry-safe behavior.
- Around line 167-178: getSandbox currently caches a rejected Promise from
createSandbox via this.sandboxPromise, causing permanent failures; update
getSandbox so that when createSandbox() rejects it clears this.sandboxPromise
(e.g., in a .catch handler) before rethrowing the error, ensuring subsequent
calls will retry creation; keep existing behavior of setting this.sandbox on
successful resolution in the .then handler and only clear the cached promise on
rejection.
🟡 Minor comments (14)
packages/sandbox-e2b/src/index.ts-161-166 (1)
161-166:⚠️ Potential issue | 🟡 Minor
buildCommandLineescapes the command itself whenargsis non-empty.When
argsare provided, the command is also passed throughescapeShellArg(line 165). If a user passes a compound command like"ls -la"with separateargs, it would be quoted as'ls -la', causing the shell to look for a literal binary namedls -larather than runninglswith flag-la.Consider escaping only the arguments, not the command:
Proposed fix
const buildCommandLine = (command: string, args?: string[]): string => { if (!args || args.length === 0) { return command; } - return [command, ...args].map(escapeShellArg).join(" "); + return [command, ...args.map(escapeShellArg)].join(" "); };examples/with-workspace/.env.example-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd a trailing newline.
Static analysis flags a missing blank line at end of file — a common POSIX convention that avoids issues with some tools.
packages/core/src/workspace/sandbox/toolkit.ts-114-127 (1)
114-127:⚠️ Potential issue | 🟡 MinorMulti-byte character truncation at byte boundary.
buf.subarray(0, targetBytes).toString("utf-8")can split a multi-byte UTF-8 character, producing a U+FFFD replacement character at the truncation point. For sandbox output this is likely acceptable, but if precision matters you could walk back to a valid character boundary.packages/core/src/workspace/sandbox/local.ts-123-123 (1)
123-123:⚠️ Potential issue | 🟡 MinorCommand allowlist/blocklist checks only the basename, but the full command is executed.
normalizeCommandextractspath.basename(command), so a user passing/some/path/nodeis checked asnode. This is fine for simple cases, but note that if the sandbox is exposed to untrusted input, an adversary could place a malicious binary at a path whose basename matches an allowed command. The isolation layer (sandbox-exec/bwrap) mitigates this, but without isolation (provider: "none"), this is a weak boundary.Consider documenting this limitation or, when isolation is
"none", resolving the command to its absolute path before checking.Also applies to: 507-513
packages/core/src/workspace/sandbox/local.spec.ts-112-118 (1)
112-118:⚠️ Potential issue | 🟡 MinorSame
process.envdeletion bug for PATH.Same issue as above — use
delete process.env.PATHinstead of assigningundefined.🐛 Proposed fix
afterEach(() => { if (originalPath === undefined) { - process.env.PATH = undefined; + delete process.env.PATH; } else { process.env.PATH = originalPath; } });packages/core/src/workspace/sandbox/local.spec.ts-39-45 (1)
39-45:⚠️ Potential issue | 🟡 MinorBug:
process.env.X = undefinedsets the variable to the string"undefined"in Node.js.In Node.js, assigning
undefinedto aprocess.envproperty coerces it to the string"undefined"rather than deleting it. Usedeleteto properly remove the variable.🐛 Proposed fix
afterEach(() => { if (originalSecret === undefined) { - process.env.VOLTAGENT_TEST_SECRET = undefined; + delete process.env.VOLTAGENT_TEST_SECRET; } else { process.env.VOLTAGENT_TEST_SECRET = originalSecret; } });packages/core/src/workspace/sandbox/local.ts-225-225 (1)
225-225:⚠️ Potential issue | 🟡 MinorSeatbelt profile value escaping is incomplete and could allow profile injection.
The SBPL (seatbelt profile) syntax uses Scheme string escaping rules, which require both backslashes and double quotes to be escaped. Paths containing backslashes—such as Windows UNC paths, certain network paths, or escape sequences—will break the profile syntax. The escape must be applied in the correct order: backslashes first, then quotes.
-const escapeSandboxProfileValue = (value: string): string => value.replace(/"/g, '\\"'); +const escapeSandboxProfileValue = (value: string): string => + value.replace(/\\/g, "\\\\").replace(/"/g, '\\"');packages/core/src/workspace/filesystem/node-filesystem-backend.spec.ts-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorTest assumes Unix-like filesystem layout.
/etc/hostswon't exist on Windows. If CI runs on Windows, this assertion may behave differently. Consider using a platform-agnostic path or guarding with a platform check if Windows CI is in scope.packages/core/src/workspace/sandbox/toolkit.spec.ts-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorMissing
awaitonsandbox.destroy().
destroy()is async per theWorkspaceSandboxinterface. Withoutawait, the cleanupfs.rmon line 53 may race with an incomplete destroy, potentially causing intermittent test failures or resource leaks.Fix
- sandbox.destroy(); + await sandbox.destroy();packages/server-elysia/src/routes/agent.routes.ts-379-396 (1)
379-396:⚠️ Potential issue | 🟡 MinorOpenAPI response schema missing
400status for the skills list route.The handler
handleListAgentWorkspaceSkillscan returnhttpStatus: 400when therefreshquery parameter is invalid, but the route's response schema only declares200,404, and500. The workspace read route (Line 346) correctly includes400— this one should match.Proposed fix
response: { success: t.Literal(true), data: WorkspaceSkillListSchema, }), + 400: ErrorSchema, },website/docs/agents/workspace.md-26-28 (1)
26-28:⚠️ Potential issue | 🟡 MinorFix embedding provider format in workspace.md documentation.
The documentation uses incorrect colon-separated format
"openai:text-embedding-3-small"at lines 28 and 411, but the correct format used throughout the codebase is slash-separated:"openai/text-embedding-3-small". This format is used in all example code (examples/with-workspace/src/index.ts), nearly all other documentation files, generated type definitions, and the CHANGELOG. Update both occurrences in workspace.md to use the slash format for consistency.website/docs/agents/workspace-skills.md-17-27 (1)
17-27:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
The static analysis tool flagged this block for missing a language specifier. Since this shows a directory tree, use a generic identifier like
textorplaintext.Proposed fix
-``` +```text /skills /data-analysis SKILL.mdpackages/core/src/workspace/filesystem/backends/composite.ts-135-181 (1)
135-181:⚠️ Potential issue | 🟡 Minor
grepRawdiscards accumulated matches if any sub-backend returns an error string.At line 169, if a route backend returns a string error, all previously collected matches in
allMatches(including valid results from the default backend) are discarded and the error string is returned. Consider collecting errors separately and only returning them if no matches were found, or skipping the errored backend.♻️ Suggested: skip errored backends instead of aborting
const raw = await backend.grepRaw(pattern, "/", glob); if (typeof raw === "string") { - return raw; + continue; // skip backends that error } allMatches.push(packages/core/src/workspace/search/index.ts-232-234 (1)
232-234:⚠️ Potential issue | 🟡 MinorAuto-indexing fires in the constructor before
init()is called.
indexPathsreads from the filesystem (globInfo,readRaw). If the filesystem backend isn't initialized yet (e.g.,WorkspaceFilesystem.init()hasn't been called), these calls may fail silently — errors are captured in the summary which is then discarded via.then(() => undefined).Consider deferring auto-indexing to
init()orensureAutoIndex()only, rather than eagerly starting in the constructor.
🧹 Nitpick comments (21)
packages/sandbox-daytona/tsconfig.json (1)
4-4:"dom"lib is unnecessary for a server-side Node.js package.Including
"dom"and"dom.iterable"in a sandbox package that runs in Node.js can silently allow DOM API usage (e.g.,document,window) that would fail at runtime. Consider removing them.- "lib": ["dom", "dom.iterable", "esnext"], + "lib": ["esnext"],packages/sandbox-e2b/tsconfig.json (2)
4-4:"dom"and"dom.iterable"libs are unnecessary for a Node.js sandbox package.This package wraps the E2B SDK for server-side sandbox execution. Including DOM libs can mask accidental usage of browser-only APIs (e.g.,
document,window) that would fail at runtime. Consider restricting to just"esnext":- "lib": ["dom", "dom.iterable", "esnext"], + "lib": ["esnext"],
11-11:rootDirset to"./"while onlysrc/**/*.tsis included.With
rootDir: "./", iftscis ever invoked directly (e.g., for type-checking or declaration emit outside of tsup), output will nest underdist/src/rather thandist/. Since tsup generates declarations itself this is low impact, but aligningrootDirto"./src"prevents surprises if the build pipeline changes.packages/sandbox-e2b/src/index.ts (2)
282-293: Rejected sandbox creation promise is cached permanently.If
createSandbox()rejects (e.g., network blip, temporary auth failure),this.sandboxPromiseholds the rejected promise forever. Every subsequentexecute()call on this instance will immediately fail with the cached rejection — no recovery is possible without creating a newE2BSandboxinstance.Consider clearing the cached promise on rejection:
Proposed fix
private async getSandbox(): Promise<E2BSdkSandbox> { if (this.sandbox) { return this.sandbox; } if (!this.sandboxPromise) { - this.sandboxPromise = this.createSandbox().then((sandbox) => { - this.sandbox = sandbox; - return sandbox; - }); + this.sandboxPromise = this.createSandbox().then( + (sandbox) => { + this.sandbox = sandbox; + return sandbox; + }, + (error) => { + this.sandboxPromise = undefined; + throw error; + }, + ); } return this.sandboxPromise; }
369-386: Fire-and-forgetrequestKillsilently swallows errors.Both the abort listener (line 376) and the timeout handler (line 384) call
void requestKill(), discarding any rejection. IfkillCommandfails (e.g., sandbox connection dropped), the process continues running in the sandbox without any signal to the caller —aborted/timedOutflags will be set but the process may still be alive and consuming resources.At minimum, consider logging the error for debuggability:
Proposed fix
const requestKill = async (): Promise<void> => { - await this.killCommand(sandbox, commandHandle); + try { + await this.killCommand(sandbox, commandHandle); + } catch { + // Kill best-effort — sandbox process may still be running + } };This makes the intent explicit. If the project has a logger available, emitting a warning here would aid operational troubleshooting.
packages/core/src/workspace/timeout.spec.ts (1)
27-34: Consider adding a test for mid-execution abort.The current abort test only covers the already-aborted signal case. A test where the signal is aborted during task execution would exercise the
addEventListener("abort", ...)path in the implementation.packages/core/src/workspace/sandbox/local.ts (1)
553-561: Duplicated SIGTERM→SIGKILL escalation logic.The abort handler (lines 553-561) and timeout handler (lines 572-580) contain identical kill-escalation logic. Extract a small helper to reduce duplication.
♻️ Example
+ const killProcess = () => { + proc.kill("SIGTERM"); + setTimeout(() => { + if (!proc.killed) { + proc.kill("SIGKILL"); + } + }, 1000); + }; + const onAbort = () => { aborted = true; - proc.kill("SIGTERM"); - setTimeout(() => { - if (!proc.killed) { - proc.kill("SIGKILL"); - } - }, 1000); + killProcess(); }; ... if (timeoutMs > 0) { timeoutId = setTimeout(() => { timedOut = true; - proc.kill("SIGTERM"); - setTimeout(() => { - if (!proc.killed) { - proc.kill("SIGKILL"); - } - }, 1000); + killProcess(); }, timeoutMs); }Also applies to: 572-580
packages/core/src/workspace/filesystem/toolkit.spec.ts (1)
38-43: Optional chaining silently swallows missing tools, producing misleading test failures.If a tool name is mistyped or removed,
getTool(...)returnsundefined, the?.execute?.()short-circuits toundefined, andString(undefined)becomes"undefined". ThetoContainassertion still fails, but the error message will be confusing ("expected 'undefined' to contain '- summary.md'") rather than clearly indicating the tool is missing.Consider adding an assertion that the tool exists before invoking it, similar to the guard in
toolkit.spec.tsfor the sandbox (line 37–39 ofsandbox/toolkit.spec.ts).Suggested pattern
- const listTree = await getTool("list_tree")?.execute?.( - { path: "/notes", max_depth: 3 }, - executeOptions, - ); + const listTreeTool = getTool("list_tree"); + expect(listTreeTool?.execute).toBeDefined(); + const listTree = await listTreeTool!.execute!( + { path: "/notes", max_depth: 3 }, + executeOptions, + );website/docs/agents/workspace.md (1)
35-42: Undefinedmodelvariable in quick start example.The
modelvariable is used on Line 37 but never declared in this code block. While brief examples often omit boilerplate, adding a comment or a placeholder (e.g.,const model = "openai/gpt-4o-mini";) would help new users follow along without confusion. The same omission appears in other code snippets throughout this page (Lines 49, 71, 198, 250, 369).packages/core/src/planagent/plan-agent.ts (1)
1025-1030: Sanitization works but could be cleaner with destructuring.Setting properties to
undefinedvia type assertion is functional but fragile — the keys still exist on the object (just withundefinedvalues). If the baseAgentconstructor ever checks"workspace" in options, it would still find it. Consider destructuring instead:♻️ Suggested refactor
- const sanitizedAgentOptions = { ...(agentOptions as AgentOptions) }; - (sanitizedAgentOptions as Record<string, unknown>).workspace = undefined; - (sanitizedAgentOptions as Record<string, unknown>).workspaceToolkits = undefined; + const { workspace: _ws, workspaceToolkits: _wt, ...sanitizedAgentOptions } = + agentOptions as AgentOptions & { workspace?: unknown; workspaceToolkits?: unknown };website/docs/agents/workspace-skills.md (1)
113-113: Missing trailing newline at end of file.Most editors and linters expect a final newline.
packages/core/src/workspace/search/bm25.ts (1)
17-22: Tokenizer only handles ASCII[a-z0-9], dropping all Unicode content.The regex
/[^a-z0-9]+/gstrips accented characters (é, ñ), CJK, Cyrillic, etc. Documents or queries containing non-ASCII text will produce empty token lists and be effectively unsearchable. If workspace files may contain internationalized content, consider using a Unicode-aware pattern such as/[^\p{L}\p{N}]+/gu.Proposed fix
const tokenize = (text: string): string[] => { return text .toLowerCase() - .split(/[^a-z0-9]+/g) + .split(/[^\p{L}\p{N}]+/gu) .filter((token) => token.length > 0); };packages/core/src/workspace/search/types.ts (1)
41-52: Redundant score fields inWorkspaceSearchResult.
scoreDetails(line 47) already carriesbm25andvectorsub-scores, whilebm25ScoreandvectorScore(lines 48-49) duplicate the same data at the top level. Consider removing one representation to avoid consumers getting out-of-sync values.♻️ Suggested: consolidate into `scoreDetails` only
export type WorkspaceSearchResult = { id: string; path: string; score: number; content: string; lineRange?: { start: number; end: number }; scoreDetails?: { bm25?: number; vector?: number }; - bm25Score?: number; - vectorScore?: number; snippet?: string; metadata?: Record<string, unknown>; };packages/server-core/src/handlers/agent-additional.handlers.ts (1)
144-191:handleGetAgentWorkspaceInfohardcodesfilesystem: true.Line 176 always reports filesystem as available. If a future Workspace variant doesn't include a filesystem, this would be misleading. Consider deriving it the same way as other capabilities (
Boolean(workspace.filesystem)).packages/core/src/workspace/filesystem/index.ts (2)
426-440:as nevercast bypasses OpenTelemetry'sAttributeValuetype safety.
setAttributeexpectsAttributeValue(string | number | boolean | array thereof). Passing arbitraryunknownviaas neversilently accepts objects,null, etc., which OTel will drop or stringify unexpectedly.A narrower guard would preserve type safety:
♻️ Suggested improvement
- for (const [key, value] of Object.entries(attributes)) { - if (value !== undefined) { - toolSpan.setAttribute(key, value as never); - } + for (const [key, value] of Object.entries(attributes)) { + if ( + value !== undefined && + value !== null && + (typeof value === "string" || typeof value === "number" || typeof value === "boolean") + ) { + toolSpan.setAttribute(key, value); + } }
1144-1176:isToolPolicyGroupis duplicated across modules.This exact helper (checking for
"tools"/"defaults"own properties) also appears inpackages/core/src/workspace/index.ts(lines 84-88) andpackages/core/src/workspace/search/index.ts(lines 692-696). Consider extracting it into the sharedtool-policymodule to keep a single source of truth.packages/core/src/workspace/index.ts (1)
105-124:as TPolicycast inmergeToolPoliciesmay mask missing required fields.The spread of
defaultsproduces a plain object that may not satisfy all ofTPolicy's required properties. In practiceTPolicyis always a partial-like policy interface, so this is likely safe today, but it weakens type guarantees if the policy types gain required fields later.packages/core/src/workspace/filesystem/utils.ts (2)
63-83: Deadtypeofbranch increateFileDataandupdateFileData.
contentis typedstring, sotypeof content === "string"is alwaystrue. The ternary else branch is unreachable and would cause a type error if reached (assigningstringtostring[]). Likely a leftover from a previous signature that also acceptedstring[].♻️ Simplified
export function createFileData(content: string, createdAt?: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString(); ... export function updateFileData(fileData: FileData, content: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n");
11-13: SimplifysanitizeToolCallIdwith a single regex.♻️ Minor simplification
export function sanitizeToolCallId(toolCallId: string): string { - return toolCallId.replace(/\./g, "_").replace(/\//g, "_").replace(/\\/g, "_"); + return toolCallId.replace(/[.\\/]/g, "_"); }packages/core/src/workspace/search/index.ts (2)
656-683: Search results may include full file contents without truncation, risking oversized LLM context.When
include_contentdefaults totrue, theworkspace_searchtool returns the entire file content for each result (up totop_k, default 5).formatSearchResultsemits all of it in thetoModelOutputcallback. For large indexed files this could generate very large tool outputs that exceed model context limits.Consider applying
truncateIfTooLong(already available in../filesystem/utils) to the formatted output, or capping per-result content length.Also applies to: 808-903
625-645:setWorkspaceSpanAttributesandbuildWorkspaceAttributesare duplicated fromfilesystem/index.ts.These exact helpers are copy-pasted across at least two modules. Consider extracting them into a shared workspace utility (e.g., alongside
tool-policy).
| let daytonaModulePromise: Promise<DaytonaModule> | undefined; | ||
|
|
||
| const loadDaytonaModule = async (): Promise<DaytonaModule> => { | ||
| if (!daytonaModulePromise) { | ||
| daytonaModulePromise = import("@daytonaio/sdk").then((mod) => mod as unknown as DaytonaModule); | ||
| } | ||
| return daytonaModulePromise; | ||
| }; |
There was a problem hiding this comment.
Rejected promise is cached forever — failed dynamic import is not retryable.
If import("@daytonaio/sdk") rejects (e.g., module not installed, transient error), the rejected promise is stored in daytonaModulePromise and every subsequent call to loadDaytonaModule() will return the same rejection without retrying.
Proposed fix
const loadDaytonaModule = async (): Promise<DaytonaModule> => {
if (!daytonaModulePromise) {
- daytonaModulePromise = import("@daytonaio/sdk").then((mod) => mod as unknown as DaytonaModule);
+ daytonaModulePromise = import("@daytonaio/sdk").then(
+ (mod) => mod as unknown as DaytonaModule,
+ (err) => {
+ daytonaModulePromise = undefined;
+ throw err;
+ },
+ );
}
return daytonaModulePromise;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let daytonaModulePromise: Promise<DaytonaModule> | undefined; | |
| const loadDaytonaModule = async (): Promise<DaytonaModule> => { | |
| if (!daytonaModulePromise) { | |
| daytonaModulePromise = import("@daytonaio/sdk").then((mod) => mod as unknown as DaytonaModule); | |
| } | |
| return daytonaModulePromise; | |
| }; | |
| let daytonaModulePromise: Promise<DaytonaModule> | undefined; | |
| const loadDaytonaModule = async (): Promise<DaytonaModule> => { | |
| if (!daytonaModulePromise) { | |
| daytonaModulePromise = import("@daytonaio/sdk").then( | |
| (mod) => mod as unknown as DaytonaModule, | |
| (err) => { | |
| daytonaModulePromise = undefined; | |
| throw err; | |
| }, | |
| ); | |
| } | |
| return daytonaModulePromise; | |
| }; |
🤖 Prompt for AI Agents
In `@packages/sandbox-daytona/src/index.ts` around lines 108 - 115, The cached
promise daytonaModulePromise can hold a rejected import and prevent retries;
modify loadDaytonaModule so that it does not permanently cache a failing import:
when calling import("@daytonaio/sdk") (inside loadDaytonaModule) wrap the
dynamic import in a try/catch or attach a .catch handler that clears
daytonaModulePromise (set it back to undefined) before rethrowing the error, or
assign the import to a local/temp promise and only store it into
daytonaModulePromise after it resolves successfully; reference
daytonaModulePromise and loadDaytonaModule to implement this retry-safe
behavior.
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="website/docs/workspaces/overview.md">
<violation number="1" location="website/docs/workspaces/overview.md:27">
P2: The embedding model ID uses `openai:text-embedding-3-small`, but the documented provider-qualified format is `openai/text-embedding-3-small`. The colon variant won't resolve in the model registry; update this snippet to the slash-separated ID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| sandbox: new LocalSandbox(), | ||
| search: { | ||
| autoIndexPaths: ["/"], | ||
| embedding: "openai:text-embedding-3-small", |
There was a problem hiding this comment.
P2: The embedding model ID uses openai:text-embedding-3-small, but the documented provider-qualified format is openai/text-embedding-3-small. The colon variant won't resolve in the model registry; update this snippet to the slash-separated ID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/docs/workspaces/overview.md, line 27:
<comment>The embedding model ID uses `openai:text-embedding-3-small`, but the documented provider-qualified format is `openai/text-embedding-3-small`. The colon variant won't resolve in the model registry; update this snippet to the slash-separated ID.</comment>
<file context>
@@ -0,0 +1,106 @@
+ sandbox: new LocalSandbox(),
+ search: {
+ autoIndexPaths: ["/"],
+ embedding: "openai:text-embedding-3-small",
+ vector: new InMemoryVectorAdapter(),
+ },
</file context>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/workspace/skills/index.ts">
<violation number="1" location="packages/core/src/workspace/skills/index.ts:918">
P2: Path normalization inconsistency: `relativePath` is only trimmed but not path-normalized before being compared against the `normalizedAllowed` list, which IS path-normalized. This causes paths with backslashes (e.g., `docs\file.md`) to silently fail to match their forward-slash equivalents in the allowed list, breaking cross-platform compatibility.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| relativePath: string, | ||
| allowed: string[] | undefined, | ||
| ): string | null { | ||
| const normalized = relativePath.trim(); |
There was a problem hiding this comment.
P2: Path normalization inconsistency: relativePath is only trimmed but not path-normalized before being compared against the normalizedAllowed list, which IS path-normalized. This causes paths with backslashes (e.g., docs\file.md) to silently fail to match their forward-slash equivalents in the allowed list, breaking cross-platform compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/workspace/skills/index.ts, line 918:
<comment>Path normalization inconsistency: `relativePath` is only trimmed but not path-normalized before being compared against the `normalizedAllowed` list, which IS path-normalized. This causes paths with backslashes (e.g., `docs\file.md`) to silently fail to match their forward-slash equivalents in the allowed list, breaking cross-platform compatibility.</comment>
<file context>
@@ -0,0 +1,1335 @@
+ relativePath: string,
+ allowed: string[] | undefined,
+ ): string | null {
+ const normalized = relativePath.trim();
+ if (!normalized) {
+ return null;
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/workspace/skills/index.ts`:
- Around line 375-383: The constructor is fire-and-forgetting
discoverSkills()/indexSkills() into this.autoDiscoverPromise and
this.autoIndexPromise which can produce unhandled rejections; suppress that by
appending a catch to the stored fire-and-forget promises (e.g.
this.autoDiscoverPromise = this.discoverSkills().then(() => undefined).catch(()
=> undefined) and similarly for this.autoIndexPromise) while keeping
ensureDiscovered()/ensureIndexed() implemented to await the original work (call
discoverSkills()/indexSkills() or another stored "real" promise if you choose to
keep one) so that errors still surface when those methods are awaited.
- Around line 689-700: The call to this.embedding.embedBatch may return fewer
embeddings than docs, leading to undefined vectors being stored; after awaiting
this.embedding.embedBatch(docs.map((doc) => doc.content)) in the method that
builds VectorItem entries, validate that embeddings is an array and
embeddings.length === docs.length, and if not either throw or filter out
mismatched entries before creating VectorItem objects; implement this by zipping
docs with embeddings, skipping or logging entries where embeddings[idx] is
undefined (and use a clear error/log message), then pass only fully-populated
VectorItem objects to this.vector.storeBatch to avoid storing undefined vectors.
🧹 Nitpick comments (4)
packages/core/src/workspace/skills/index.ts (4)
130-136: Unreachable fallback inresolveEmbeddingAdapter.Given
EmbeddingAdapterInput = EmbeddingAdapter | EmbeddingModelReference | EmbeddingAdapterConfig, the branches at lines 122 (isEmbeddingAdapter), 126 (string), and 130 (isEmbeddingAdapterConfig) are exhaustive. Line 135 is dead code. If the union type is ever extended, this fallback could silently produce an incorrect adapter.Consider replacing it with an explicit exhaustiveness check.
♻️ Suggested change
if (isEmbeddingAdapterConfig(embedding)) { const { model, ...options } = embedding; return new AiSdkEmbeddingAdapter(model, options); } - return new AiSdkEmbeddingAdapter(embedding); + // Exhaustive check: all branches of EmbeddingAdapterInput should be handled above. + const _exhaustive: never = embedding; + return _exhaustive;
229-236:as nevercast bypasses OTelAttributeValuetype safety.
toolSpan.setAttribute(key, value as never)silences the type checker entirely. ThesetAttributeAPI expectsAttributeValue(string, number, boolean, or arrays thereof), but the callers passRecord<string, unknown>values. If a caller ever passes a non-primitive (e.g., an object orundefined), this will produce invalid span attributes silently.Consider narrowing to the expected types.
♻️ Suggested change
+import type { AttributeValue } from "@opentelemetry/api"; + +const isAttributeValue = (v: unknown): v is AttributeValue => + typeof v === "string" || typeof v === "number" || typeof v === "boolean" || + (Array.isArray(v) && v.every((i) => typeof i === "string" || typeof i === "number" || typeof i === "boolean")); + const setWorkspaceSpanAttributes = ( operationContext: OperationContext, attributes: Record<string, unknown>, ): void => { const toolSpan = operationContext.systemContext.get("parentToolSpan") as Span | undefined; if (!toolSpan) { return; } for (const [key, value] of Object.entries(attributes)) { - if (value !== undefined) { - toolSpan.setAttribute(key, value as never); + if (value !== undefined && isAttributeValue(value)) { + toolSpan.setAttribute(key, value); } } };
913-946: Path traversal check is minimal — consider also blocking null bytes and normalizing before checking.
normalized.includes("..")catches the most common traversal, but the input is not normalized (backslashes not converted) before the..check. An input likescripts\\..\\..\secretwould have backslashes thatnormalizePath(called later at line 927 on the allowed list but not onrelativePathitself at line 918) wouldn't have cleaned yet when the..check runs. SincenormalizePathis only applied to the allowed entries, a craftedrelativePathwith backslash-separated..segments wouldn't match the allowed list either (so it fails safely), but normalizingrelativePathfirst would make the intent clearer and more robust.🛡️ Suggested hardening
resolveSkillFilePath( skill: WorkspaceSkillMetadata, relativePath: string, allowed: string[] | undefined, ): string | null { - const normalized = relativePath.trim(); + const normalized = normalizePath(relativePath.trim()); if (!normalized) { return null; } if (normalized.includes("..")) { return null; } + + if (normalized.includes("\0")) { + return null; + }
636-640: Catch clause useserror: any— preferunknownfor type safety.Multiple catch clauses throughout the file (lines 636, 671, 701, 1087) use
catch (error: any). TypeScript best practice iscatch (error: unknown)with explicit narrowing, which aligns with the project's type-safety guideline. Theerror?.message ? String(error.message) : "unknown error"pattern would work the same with a small adjustment.♻️ Example
- } catch (error: any) { - summary.errors.push( - `Vector cleanup failed: ${error?.message ? String(error.message) : "unknown error"}`, - ); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : "unknown error"; + summary.errors.push(`Vector cleanup failed: ${message}`); }Based on learnings: "Maintain type safety in TypeScript-first codebase."
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="website/docs/workspaces/filesystem.md">
<violation number="1" location="website/docs/workspaces/filesystem.md:98">
P2: The link now points to `../security`, but the `Workspace Security` page is in the same `workspaces` directory. This change will break the docs link. Use `./security` (or `security`) instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@website/docs/workspaces/filesystem.md`:
- Line 98: Update the relative link target for the "Workspace Security"
reference: replace the incorrect "../security" link with "./security" in the
markdown line that reads "For broader recommendations, see [Workspace
Security](../security)." so it points to the security.md in the same directory.
🧹 Nitpick comments (6)
website/docs/workspaces/filesystem.md (6)
29-29: Clarify the workspace-relative path requirement.The statement "must start with
/" might confuse readers since workspace-relative paths could be interpreted as not needing a leading slash. Consider clarifying that/represents the workspace root in the virtual filesystem.📝 Suggested clarification
-All filesystem tool paths are workspace-relative and must start with `/`. +All filesystem tool paths are workspace-relative and must start with `/` (where `/` represents the workspace root).Or alternatively:
-All filesystem tool paths are workspace-relative and must start with `/`. +All filesystem paths are relative to the workspace root. Paths must start with `/`, where `/` represents the workspace root in the virtual filesystem.
42-42: Clarify the relationship betweenlist_filesandlist_tree.If
list_filesis merely an alias, consider clarifying whether both tools are exposed to agents or if this is an implementation detail. Users may be confused about which tool to use.📝 Suggested clarification
-- `list_files`: alias for `list_tree` +- `list_files`: convenience alias for `list_tree` (both tools are available)
63-63: Add context forworkspace_indexandworkspace_index_contenttools.These tools are mentioned without prior introduction in this document. Readers may be confused about what they are. Consider adding a brief explanation or referencing the search documentation.
📝 Suggested clarification
-When read-only, `workspace_index` and `workspace_index_content` are also disabled. +When read-only, workspace search indexing tools (`workspace_index` and `workspace_index_content`) are also disabled. See [Workspace Search](./search) for details.
90-90: Clarify the "prompts a re-read" behavior.The phrase "prompts a re-read if the file changes" is ambiguous. Does the system throw an error requiring the agent to retry? Does it automatically trigger a re-read? Does it send a notification to the agent? Please clarify the exact mechanism.
📝 Suggested clarification
-`requireReadBeforeWrite` forces a `read_file` call before edits/deletes and prompts a re-read if the file changes. +`requireReadBeforeWrite` forces a `read_file` call before edits/deletes. If the file has changed since it was read, the operation will fail with an error, requiring the agent to re-read the file before retrying.(Adjust based on actual behavior)
92-96: Consider adding more specific security guidance.The security notes are helpful but could be more actionable. Consider adding specific details about:
- How the system handles absolute paths (are they rejected?)
- Path traversal prevention (e.g., blocking
../sequences)- Symlink handling within the workspace
These additions would help developers understand the built-in protections and what they need to implement themselves.
81-82: Consider clarifying the scope ofrequireReadBeforeWritepolicy name.The policy applies to operations beyond writes—specifically
delete_fileand implicitlymkdir. While the documentation mentions "edits/deletes" in the explanation, the policy namerequireReadBeforeWritedoesn't clearly convey this broader scope. Note thatmkdirdoesn't actually trigger the policy (directories can't be read before creation), so applying it there is benign but possibly misleading in the configuration example.
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/workspace/filesystem/backends/composite.ts">
<violation number="1" location="packages/core/src/workspace/filesystem/backends/composite.ts:57">
P2: posixPath.normalize can collapse `..` segments and escape the mount prefix, so filesUpdate entries like `/../secret` would be remapped outside the mounted route. Guard against normalized paths that do not remain under the mount prefix before adding them to the result.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 68-74: The constructor registers the Workspace instance with
this.registry via registry.setGlobalWorkspace but never initializes it, so call
and await workspace.init() immediately after registering the workspace to ensure
filesystem/sandbox/search/skills are ready before any toolkit creation; locate
the VoltAgent constructor around the block that creates workspaceInstance
(symbol: Workspace and variable workspaceInstance) and add await
workspaceInstance.init() after
this.registry.setGlobalWorkspace(workspaceInstance) so that subsequent methods
like buildWorkspaceToolkits (referenced later) get a fully initialized
workspace.
In `@packages/core/src/workspace/filesystem/backends/composite.ts`:
- Around line 304-329: The rmdir implementation returns sub-backend RmdirResult
without remapping filesUpdate keys into the composite namespace; update the
rmdir method (which uses getBackendAndKey, sortedRoutes and defaultBackend) to
run the returned result through remapFilesUpdateResult when a matchedPrefix
exists and backend !== this.defaultBackend, then return that remapped result
(preserving existing path remapping logic for result.path as done currently).
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 752-757: The fallbackSearch implementation currently applies
micromatch.isMatch to path.basename(fp) (see the files loop using includeGlob),
which only matches filenames and diverges from ripgrep's glob behavior; update
the check to match the file path relative to the workspace root (e.g., compute
path.relative(root, fp) and pass that to micromatch.isMatch) so globs like
"src/**/*.ts" behave identically to the rg path-based match in the other branch;
ensure you still use includeGlob and preserve the existing include/exclude logic
in the fallbackSearch function.
In `@packages/core/src/workspace/search/index.ts`:
- Around line 656-683: formatSearchResults currently appends result.content
verbatim (when include_content is true) which can produce huge outputs; update
formatSearchResults to cap per-result content length before adding it (e.g.,
prefer result.snippet, or if using result.content call the shared
truncateIfTooLong helper from the filesystem toolkit to limit to a safe max like
1k–2k chars), and also run truncateIfTooLong on the final joined string before
returning so the overall tool response cannot blow past context limits; touch
the logic around result.content/result.snippet in formatSearchResults to prefer
snippet, then truncated content, and import/reuse truncateIfTooLong.
In `@packages/core/src/workspace/skills/index.ts`:
- Around line 222-236: The setWorkspaceSpanAttributes function currently
bypasses type safety by casting value as never for toolSpan.setAttribute;
replace that unsafe cast by normalizing attribute values using the same
normalizeAttributeValue + safeStringify approach used in
planagent/filesystem/index.ts (or extract a shared helper and import it) and
pass the properly narrowed/serialized result to toolSpan.setAttribute; ensure
you do not use JSON.stringify and that the helper exports are reused so
setWorkspaceSpanAttributes calls normalizeAttributeValue(value) before
setAttribute.
In `@packages/sandbox-e2b/src/index.ts`:
- Around line 130-136: The truncation cuts raw bytes at maxBytes and can split
multi-byte UTF-8 sequences (the Buffer.from(...).subarray(0,
maxBytes).toString("utf-8") usage), producing replacement characters; fix by
trimming back to the previous UTF-8 character boundary before converting to
string: create a Buffer from value, if buf.length > maxBytes decrement an end
index while buf[end] is a UTF-8 continuation byte (byte & 0xC0 === 0x80), then
use buf.subarray(0, end) .toString("utf-8") and return truncated=true. Apply the
same boundary-aware logic to both occurrences of the current naive truncation
expression.
- Around line 361-378: The abort and timeout handlers currently call void
requestKill(), which discards the promise from requestKill (which calls
killCommand) and can lead to unhandled promise rejections; update both
abortListener and the setTimeout callback to call requestKill() and explicitly
handle failures (e.g., requestKill().catch(err => { /* log/ignore safely */ }))
so any error from killCommand is caught and handled; locate the requestKill
function and the places where abortListener and the timeoutId/setTimeout are
defined and replace the void calls with promise handling that at minimum catches
errors (preferably logging them).
- Around line 413-426: The code allows sendStdin and response.wait() to run
after the command was killed when aborted || timedOut is true; update the
isCommandHandle branch in the function to short-circuit after calling
this.killCommand(sandbox, response) by returning an appropriate E2BCommandResult
(or the killed handle) immediately, so neither this.sendStdin(sandbox, response,
options.stdin) nor response.wait() are invoked on a killed process; touch the
isCommandHandle handling around commandHandle, killCommand, options.stdin, and
response.wait to implement this early return/skip logic.
- Around line 274-285: The cached rejected promise in getSandbox will
permanently break the instance because this.sandboxPromise is never cleared on
createSandbox() rejection; modify getSandbox so when creating the promise
(this.sandboxPromise = this.createSandbox().then(...)) you attach a .catch
handler that clears this.sandboxPromise (e.g., this.sandboxPromise = undefined)
and rethrows the error so transient failures can be retried, while keeping the
existing success path that sets this.sandbox.
🧹 Nitpick comments (11)
packages/sandbox-e2b/src/index.ts (1)
380-427: Overall command orchestration is well-structured — background mode selection, stdin handling, and output buffering with fallbacks show careful design.One minor note: when
timeoutMsis exactly0, line 396 still passes it to the SDK'srunOptions.timeoutMswhile line 373 skips the localsetTimeout. Verify the E2B SDK's interpretation oftimeoutMs: 0(immediate timeout vs. no timeout) matches your intent.packages/core/src/workspace/sandbox/local.ts (3)
445-450: Fire-and-forget cleanup indestroy()may silently leave artifacts.
fs.rmis async but not awaited; the method returnsvoid. If a caller awaitssandbox.destroy()expecting the root directory to be gone (e.g., in tests or shutdown sequences), the directory may still exist. TheWorkspaceSandboxinterface allowsdestroyto returnPromise<void>, so you could make this async and await the cleanup.This is minor since the
.catch(() => {})prevents unhandled rejections, but worth noting for test reliability.
554-562: Duplicated SIGTERM → SIGKILL escalation pattern.The kill-escalation logic (SIGTERM, wait 1 s, SIGKILL) is copy-pasted between
onAbortand the timeout handler. Consider extracting a small helper to reduce duplication and ensure both paths stay in sync.♻️ Optional extraction
+ const killProcess = (proc: ReturnType<typeof spawn>) => { + proc.kill("SIGTERM"); + setTimeout(() => { + if (!proc.killed) { + proc.kill("SIGKILL"); + } + }, 1000); + }; + const onAbort = () => { aborted = true; - proc.kill("SIGTERM"); - setTimeout(() => { - if (!proc.killed) { - proc.kill("SIGKILL"); - } - }, 1000); + killProcess(proc); }; ... if (timeoutMs > 0) { timeoutId = setTimeout(() => { timedOut = true; - proc.kill("SIGTERM"); - setTimeout(() => { - if (!proc.killed) { - proc.kill("SIGKILL"); - } - }, 1000); + killProcess(proc); }, timeoutMs); }Also applies to: 572-582
493-499: Silent auto-promotion fromidletoreadyonexecute.When
statusis"idle",executesilently sets it to"ready"(line 498). This makes thestop()→idlestate effectively meaningless since any subsequentexecutecall ignores it. This may be intentional for convenience, but it meansstop()doesn't actually prevent execution—onlydestroy()does. If that's the intent, consider documenting it.packages/core/src/workspace/sandbox/toolkit.ts (1)
46-60: DuplicatedsetWorkspaceSpanAttributesandbuildWorkspaceAttributeshelpers across toolkit files.These exact helper functions (
setWorkspaceSpanAttributes,buildWorkspaceAttributes) appear identically insandbox/toolkit.ts,search/index.ts, andfilesystem/index.ts. Consider extracting them into a shared workspace utility module to reduce duplication.packages/core/src/workspace/search/index.ts (2)
361-391:indexedcount includes documents that failed vector indexing.Line 389 unconditionally adds
docs.lengthtosummary.indexedeven if the vector embedding/storage on lines 372-386 failed. A caller seeingindexed: 10, errors: ["Vector indexing failed: ..."]might expect fewer than 10 were fully indexed.This is arguably correct since BM25 indexing did succeed, but it could be confusing. Consider adding a note in the error message (e.g., "Vector indexing failed; BM25 index was updated") or adjusting the count.
96-106: FallbackresolveEmbeddingAdapterbranch may receive unexpected input.Line 105 is a catch-all that passes the raw
embeddingvalue toAiSdkEmbeddingAdapter. At this point, the value has already failed theisEmbeddingAdapter, string, andisEmbeddingAdapterConfigchecks. Depending on theEmbeddingAdapterInputunion, this branch may never be reached or may produce a runtime error. If it's truly unreachable, consider throwing an explicit error instead of silently proceeding.packages/core/src/workspace/filesystem/backends/filesystem.ts (1)
259-263: SUPPORTS_NOFOLLOWread()branch gives misleading error for symlinks.When a symlink is encountered,
lstat().isFile()returnsfalse, so the user sees "File not found" instead of the explicit "Symlinks are not allowed" message from theelsebranch (line 276). Adding a symlink check for consistency would improve debuggability.♻️ Suggested fix
if (SUPPORTS_NOFOLLOW) { const stat = await fs.lstat(resolvedPath); + if (stat.isSymbolicLink()) { + return `Error: Symlinks are not allowed: ${filePath}`; + } if (!stat.isFile()) { return `Error: File '${filePath}' not found`; }packages/core/src/workspace/filesystem/utils.ts (2)
84-104: Dead code branches due to type mismatch increateFileDataandupdateFileData.Both functions accept
content: string, but lines 85 and 96 checktypeof content === "string"with an else branch that treatscontentas already-split. The else branch is unreachable code. If these functions were intended to acceptstring | string[], the parameter type should reflect that. Otherwise, remove the unnecessary check.♻️ Suggested cleanup
export function createFileData(content: string, createdAt?: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString(); ... export function updateFileData(fileData: FileData, content: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString();Based on learnings: "Maintain type safety in TypeScript-first codebase".
162-175:validatePathalways appends a trailing slash — may confuse callers expecting file paths.This function normalizes paths to always end with
/, which is correct for directory-based search scopes (grep, glob base paths) but would be surprising if called with a file path like/foo.txt→/foo.txt/. The function name is generic (validatePath) but the behavior is directory-specific. Consider renaming tonormalizeDirectoryPathornormalizeSearchBasePathfor clarity.packages/core/src/workspace/filesystem/index.ts (1)
1027-1038:list_treeandlist_filesare identical tools with different names.Both call
createListTreeToolwith the same implementation and nearly identical descriptions (lines 75-78). If the intent is to provide an alias, the LLM may invoke both in the same conversation, producing duplicate results. Consider either keeping only one, or having the descriptions clearly differentiate them.Also applies to: 1217-1236
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/voltagent.ts">
<violation number="1" location="packages/core/src/voltagent.ts:199">
P2: Deferring `finalizeInit()` behind `workspace.init()` makes `serverlessProvider` unavailable until the async init completes, so immediate `voltAgent.serverless()` calls (common in serverless entrypoints) will now throw when workspace is enabled. Consider exposing a readiness promise or initializing the serverless provider before the async wait so existing entrypoints don’t break.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 197-206: The current combined async IIFE conflates errors from
workspaceInitPromise and finalizeInit(); change it so workspaceInitPromise is
awaited with its own error handling (e.g., await workspaceInitPromise.catch(err
=> this.logger.error("Workspace initialization failed:", err)) or rethrow if
fatal) and then always call finalizeInit() inside a separate try/catch that logs
failures as e.g. this.logger.error("finalizeInit failed:", err); make sure
workspaceInitPromise and finalizeInit have distinct error messages and that
finalizeInit still runs in degraded mode unless you decide to treat workspace
init as fatal.
In `@packages/core/src/workspace/filesystem/backends/composite.ts`:
- Around line 171-217: In grepRaw in composite.ts (method grepRaw, iterating
this.sortedRoutes and this.routes), fix the false-prefix logic by normalizing
routePrefix once (e.g., const prefix = routePrefix.replace(/\/$/, "")) and use
prefix for both the path.startsWith check and to compute searchPath via
path.substring(prefix.length) || "/"; also change the sub-backend error handling
so a string error from a backend does not abort aggregation: collect such error
strings in an errors array and skip that backend (continue), and after
processing all backends return the aggregated matches if any exist, otherwise
return a representative error string (e.g., the first collected error).
- Around line 75-108: lsInfo is incorrectly stripping the trailing slash from
routePrefix before doing path.startsWith, causing false matches (e.g., "/data/"
matching "/database/..."); update lsInfo to check startsWith using the full
routePrefix (including its trailing slash) and compute suffix as
path.substring(routePrefix.length) (or handle the root suffix correctly) so the
correct backend from this.sortedRoutes is chosen—follow the same pattern used in
getBackendAndKey and stat; apply the same fix to grepRaw and globInfo to ensure
they also use the full routePrefix when matching.
- Around line 277-302: The mkdir implementation currently duplicates the
prefix-finding loop; replace that manual loop with a call to
getMountPrefix(path) to obtain matchedPrefix, keep the existing call to
this.getBackendAndKey(path) and the early unsupported-backend check, and when
remapping the returned path only adjust it when matchedPrefix exists and backend
!== this.defaultBackend (do NOT call remapFilesUpdateResult because MkdirResult
has no filesUpdate field unlike RmdirResult); ensure returned object still
spreads result and updates path via matchedPrefix.slice(0, -1) + result.path
when needed.
- Around line 251-275: write, edit, and delete return the sub-backend's stripped
path because remapFilesUpdateResult only remaps filesUpdate; mirror the
mkdir/rmdir fix by computing the mount prefix (via
this.getMountPrefix(filePath)) before calling remapFilesUpdateResult and, if the
returned result.path exists and a matched prefix is present, prepend
matchedPrefix.slice(0, -1) to result.path so callers receive the composite path;
apply this change in the write, edit, and delete methods (functions referenced:
write, edit, delete, getBackendAndKey, getMountPrefix, remapFilesUpdateResult,
as done in mkdir/rmdir).
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 311-313: When SUPPORTS_NOFOLLOW is true the code uses fs.lstat and
then checks stat.isFile() but does not first check for stat.isSymbolicLink(),
causing symlinks to produce a generic "not found" error; modify the block that
calls fs.lstat (the code using SUPPORTS_NOFOLLOW, fs.lstat, and stat.isFile())
to first test stat.isSymbolicLink() and throw the explicit "Symlinks are not
allowed" error if true, then proceed to the existing isFile() check and the
current error path.
- Around line 259-263: In the read() SUPPORTS_NOFOLLOW branch, lstat is only
checked with stat.isFile(), causing symlinks to fall through to a misleading
"File not found" message; update the read() logic (the SUPPORTS_NOFOLLOW branch
that calls fs.lstat(resolvedPath)) to first check stat.isSymbolicLink() and
return the same explicit symlink error used in the else branch (mirror the check
in edit() that inspects isSymbolicLink() before isFile()), then keep the
existing isFile() check and error behavior.
- Around line 741-750: The fallbackSearch implementation currently uses
fast-glob with its default followSymbolicLinks true and calls fs.stat, allowing
symlinks under root to escape containment; change the fast-glob invocation in
fallbackSearch (the fg(...) call) to explicitly set followSymbolicLinks: false
and then replace the fs.stat(...) call that determines file type with
fs.lstat(...) to avoid following symlinks, and before reading any matched file
ensure you call assertPathContained(...) on the resolved path and skip entries
where lstat indicates a symbolic link so symlink targets outside the workspace
are not read.
In `@packages/core/src/workspace/filesystem/utils.ts`:
- Around line 260-266: The current glob filtering uses basename(fp) which strips
directories and breaks patterns like "src/*.ts" or "**/*.spec.ts"; change the
match to use the path relative to normalizedPath (not basename) so
micromatch.isMatch(relativePath, glob, { dot: true, nobrace: false }) is used;
locate the block that builds filtered (variable filtered, loop key fp) in
utils.ts and mirror how globSearchFiles computes the relative path to
normalizedPath, then pass that relative path into micromatch.isMatch instead of
basename(fp).
In `@packages/core/src/workspace/search/index.ts`:
- Around line 716-725: The isToolEnabled function incorrectly disables
"workspace_index" and "workspace_index_content" when context.filesystem.readOnly
is true; update isToolEnabled (and any related logic using
WorkspaceSearchToolName and resolveToolPolicy) to remove the read-only guard for
these two tools so they respect their policy.enabled value instead of being
forced false — i.e., delete the conditional that checks (name ===
"workspace_index" || name === "workspace_index_content") &&
context.filesystem?.readOnly and let resolveToolPolicy(name)?.enabled ?? true
determine enablement.
- Around line 233-235: The fire-and-forget calls to
this.indexPaths(this.autoIndexPaths) (and the similar call at the later site)
are missing rejection handlers and can cause unhandled promise rejections;
attach a .catch to the promise chain so errors are logged and the promise
resolves to undefined (e.g., this.autoIndexPromise =
this.indexPaths(...).then(() => undefined).catch(err => { /* log err */; return
undefined; })), using the existing logger if available (or console.error) and
apply the same fix for the second occurrence mentioned around line 272;
reference symbols: autoIndexPaths, autoIndexPromise, indexPaths.
In `@packages/sandbox-e2b/src/index.ts`:
- Around line 389-394: The runCommand function currently builds runOptions with
onStdout/onStderr that only call appendOutput (writing to
stdoutBuffer/stderrBuffer) and never invoke the user-provided streaming
callbacks from WorkspaceSandboxExecuteOptions; update the runOptions handlers in
runCommand to first append to the internal buffer (using appendOutput) and then
invoke options.onStdout/options.onStderr if they exist (safely, e.g., guarded
with if (options.onStdout) and try/catch to avoid throwing), so that both
internal buffering and caller streaming receive the output; reference the
runCommand function, the runOptions object, appendOutput, and
options.onStdout/options.onStderr when making the change.
🧹 Nitpick comments (11)
packages/core/src/workspace/filesystem/utils.ts (4)
32-34: Minor: chain of.replace()calls can be simplified.A single regex replacement is equivalent and marginally clearer.
♻️ Optional simplification
export function sanitizeToolCallId(toolCallId: string): string { - return toolCallId.replace(/\./g, "_").replace(/\//g, "_").replace(/\\/g, "_"); + return toolCallId.replace(/[.\\/]/g, "_"); }
84-96: Dead-code ternary:contentis already typed asstring.In both
createFileData(Line 85) andupdateFileData(Line 96),typeof content === "string"is always true per the function signature, making the else branch unreachable. This looks like a copy-paste artifact and is misleading.♻️ Simplify
export function createFileData(content: string, createdAt?: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString(); ... } export function updateFileData(fileData: FileData, content: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString(); ... }
106-115: Unnecessary join-then-split:fileData.contentis alreadystring[].
fileDataToStringjoins the array with"\n", then Line 113 immediately splits it back. You can usefileData.contentdirectly, which also avoids the intermediate string allocation for large files.♻️ Avoid the round-trip
export function formatReadResponse(fileData: FileData, offset: number, limit: number): string { - const content = fileDataToString(fileData); - const emptyMsg = checkEmptyContent(content); + const lines = fileData.content; + const emptyMsg = lines.length === 0 || lines.every(l => l === "") ? EMPTY_CONTENT_WARNING : null; if (emptyMsg) { return emptyMsg; } - const lines = content.split("\n"); const startIdx = offset; const endIdx = Math.min(startIdx + limit, lines.length);
165-178: Thethrowon empty path is unreachable for most inputs.Line 166 coalesces all falsy values (
null,undefined,"") to"/", so the!pathStrguard on Line 167 can never be true. The only input that reaches the trim check is a whitespace-only string (e.g." "). If the intent is to reject empty/blank strings, the validation should happen before the default:♻️ Validate before defaulting
export function validatePath(path: string | null | undefined): string { + if (path !== null && path !== undefined && path.trim() === "") { + throw new Error("Path cannot be empty"); + } - const pathStr = path || "/"; - if (!pathStr || pathStr.trim() === "") { - throw new Error("Path cannot be empty"); - } + const pathStr = path || "/"; let normalized = pathStr.startsWith("/") ? pathStr : `/${pathStr}`;packages/sandbox-e2b/src/index.ts (2)
166-171: Inconsistent escaping:commandis shell-escaped only whenargsis non-empty.When
argsis provided,commandis run throughescapeShellArg(line 170) which quotes it if it contains special characters. Whenargsis absent (line 168),commandis passed verbatim, allowing shell metacharacters. This is likely intentional (barecommand= full shell line,command+args= argv-style), but it's a subtle contract worth documenting so callers don't accidentally pass untrusted input ascommandwithoutargs.
439-454: Error-swallowing on abort/timeout is correct but could benefit from logging.When
aborted || timedOutis true and the command throws, the error is silently swallowed (lines 443–446). The result will correctly reporttimedOut/abortedflags, but any unexpected errors (e.g., SDK bugs) during these paths are invisible. Consider emitting a debug-level log for operational visibility.packages/core/src/workspace/search/index.ts (1)
635-639: Unsafeas nevercast bypasses OpenTelemetry'sAttributeValuetype.
setAttributeexpectsstring | number | boolean | Array<string|number|boolean>. Castingvalue as neversilently allows objects or other unsupported types through, which OTel will drop or coerce incorrectly. Consider filtering or coercing to valid attribute types.packages/core/src/workspace/skills/index.ts (1)
1235-1275: Missing error handling inreadSkillFile— inconsistent with other tool handlers.
readSkillFiledoesn't wraploadSkill/readFileContentcalls in a try-catch, unlikesearchTool(line 1099). IfreadFileContentthrows (e.g., file deleted between resolve and read), the raw error propagates to the agent. Consider wrapping in try-catch and returning a user-friendly message, consistent with howsearchToolhandles errors.Proposed fix
const readSkillFile = async ( skillId: string, filePath: string, kind: "reference" | "script" | "asset", executeOptions: unknown, ): Promise<string> => { const operationContext = executeOptions as OperationContext; setWorkspaceSpanAttributes(operationContext, { ...buildWorkspaceAttributes(context.workspace), "workspace.operation": `skills.read_${kind}`, "workspace.skills.name": skillId, }); if (!context.skills) { return "Workspace skills are not configured."; } + try { const skill = await context.skills.loadSkill(skillId); if (!skill) { return `Skill not found: ${skillId}`; } const list = kind === "reference" ? skill.references : kind === "script" ? skill.scripts : skill.assets; const resolvedPath = context.skills.resolveSkillFilePath(skill, filePath, list); if (!resolvedPath) { const available = list && list.length > 0 ? list.join(", ") : "none"; return `File not allowed for ${kind}. Available: ${available}`; } const content = await context.skills.readFileContent(resolvedPath); setWorkspaceSpanAttributes(operationContext, { "workspace.skills.name": skill.name, "workspace.skills.source": resolvedPath, "workspace.fs.path": resolvedPath, }); return content || "(empty)"; + } catch (error: unknown) { + const message = error instanceof Error ? error.message : "Unknown error"; + return `Failed to read ${kind} file: ${message}`; + } };packages/core/src/workspace/filesystem/backends/filesystem.ts (2)
211-238: DRY: Inline virtual-path logic duplicatestoVirtualPathhelper.Lines 211–238 manually compute the virtual path, but
toVirtualPath(fullPath, isDir)(line 148) already encapsulates this logic. The same duplication exists inglobInfo(lines 870–889). Using the helper would reduce maintenance surface.♻️ Suggested refactor for the virtual-mode branch
} else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { + if (isFile) { + const virtPath = this.toVirtualPath(fullPath, false); results.push({ path: virtPath, is_dir: false, size: entryStat.size, modified_at: entryStat.mtime.toISOString(), }); } else if (isDir) { + const virtPath = this.toVirtualPath(fullPath, true); results.push({ - path: `${virtPath}/`, + path: virtPath, is_dir: true, size: 0, modified_at: entryStat.mtime.toISOString(), }); } }
845-852: Consider settingfollowSymbolicLinks: falseinfast-globfor defense-in-depth.Unlike
fallbackSearch,globInfoproperly guards each matched path withassertPathContainedandlstat(lines 856–858). However, explicitly disabling symlink following in the glob options would prevent unnecessary traversal and align with the backend's overall symlink-rejection policy.const matches = await fg(effectivePattern, { cwd: resolvedSearchPath, absolute: true, onlyFiles: true, dot: true, + followSymbolicLinks: false, });packages/core/src/workspace/filesystem/backends/composite.ts (1)
304-334:rmdircorrectly remapsfilesUpdatenow — minor DRY nit with prefix lookup.The
rmdirlogic is sound. The manual prefix-finding loop (lines 305-311) could be replaced withthis.getMountPrefix(path)for consistency withwrite/edit/delete.
| async grepRaw( | ||
| pattern: string, | ||
| path = "/", | ||
| glob: string | null = null, | ||
| ): Promise<GrepMatch[] | string> { | ||
| for (const [routePrefix, backend] of this.sortedRoutes) { | ||
| if (path.startsWith(routePrefix.replace(/\/$/, ""))) { | ||
| const searchPath = path.substring(routePrefix.length - 1); | ||
| const raw = await backend.grepRaw(pattern, searchPath || "/", glob); | ||
|
|
||
| if (typeof raw === "string") { | ||
| return raw; | ||
| } | ||
|
|
||
| return raw.map((match) => ({ | ||
| ...match, | ||
| path: routePrefix.slice(0, -1) + match.path, | ||
| })); | ||
| } | ||
| } | ||
|
|
||
| const allMatches: GrepMatch[] = []; | ||
| const rawDefault = await this.defaultBackend.grepRaw(pattern, path, glob); | ||
|
|
||
| if (typeof rawDefault === "string") { | ||
| return rawDefault; | ||
| } | ||
|
|
||
| allMatches.push(...rawDefault); | ||
|
|
||
| for (const [routePrefix, backend] of Object.entries(this.routes)) { | ||
| const raw = await backend.grepRaw(pattern, "/", glob); | ||
|
|
||
| if (typeof raw === "string") { | ||
| return raw; | ||
| } | ||
|
|
||
| allMatches.push( | ||
| ...raw.map((match) => ({ | ||
| ...match, | ||
| path: routePrefix.slice(0, -1) + match.path, | ||
| })), | ||
| ); | ||
| } | ||
|
|
||
| return allMatches; | ||
| } |
There was a problem hiding this comment.
Same false prefix match at line 177, plus partial-result loss on sub-backend error.
-
Line 177 has the identical
routePrefix.replace(/\/$/, "")bug described inlsInfo— apply the same fix here. -
Lines 204-206: if any single sub-backend's
grepRawreturns a string error, the entire aggregated search bails out and returns that error, silently discarding all matches collected so far from the default backend and previously iterated sub-backends. Consider collecting errors separately and still returning partial matches, or at least logging the sub-backend failure.
🐛 Proposed fix for both issues
async grepRaw(
pattern: string,
path = "/",
glob: string | null = null,
): Promise<GrepMatch[] | string> {
for (const [routePrefix, backend] of this.sortedRoutes) {
- if (path.startsWith(routePrefix.replace(/\/$/, ""))) {
+ const prefixWithoutSlash = routePrefix.replace(/\/$/, "");
+ if (path === prefixWithoutSlash || path.startsWith(routePrefix)) {
const searchPath = path.substring(routePrefix.length - 1);For the partial-result-loss, consider skipping errored sub-backends:
const raw = await backend.grepRaw(pattern, "/", glob);
if (typeof raw === "string") {
- return raw;
+ continue; // skip errored sub-backend instead of discarding all results
}🤖 Prompt for AI Agents
In `@packages/core/src/workspace/filesystem/backends/composite.ts` around lines
171 - 217, In grepRaw in composite.ts (method grepRaw, iterating
this.sortedRoutes and this.routes), fix the false-prefix logic by normalizing
routePrefix once (e.g., const prefix = routePrefix.replace(/\/$/, "")) and use
prefix for both the path.startsWith check and to compute searchPath via
path.substring(prefix.length) || "/"; also change the sub-backend error handling
so a string error from a backend does not abort aggregation: collect such error
strings in an errors array and skip that backend (continue), and after
processing all backends return the aggregated matches if any exist, otherwise
return a representative error string (e.g., the first collected error).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 198-210: The constructor currently detaches finalizeInit behind an
async IIFE when workspaceInitPromise exists, causing callers to observe
uninitialized state (e.g., getAgents, serverless, getServerInstance) immediately
after new VoltAgent(...); expose and populate a public readiness promise (e.g.,
this.ready) that resolves only after workspaceInitPromise and finalizeInit
complete, or alternatively await workspaceInitPromise then call finalizeInit
synchronously in the constructor; update the code paths that launch the async
IIFE (referencing workspaceInitPromise and finalizeInit) to assign and resolve
this.ready so callers can await voltAgent.ready to guarantee
agents/server/provider are initialized.
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 856-863: globInfo currently calls the fast-glob function (via
loadFastGlob) without the followSymbolicLinks option, allowing symlink
traversal; update the call inside globInfo to pass followSymbolicLinks: false
(like fallbackSearch does) when invoking the fast-glob function so enumeration
won't traverse symlinked directories outside the workspace—locate the fast-glob
invocation in globInfo (the code using loadFastGlob, effectivePattern and
resolvedSearchPath) and add the followSymbolicLinks: false option alongside
absolute/onlyFiles/dot so the subsequent assertPathContained/lstat filtering
remains a defense-in-depth measure.
In `@packages/core/src/workspace/search/index.ts`:
- Around line 372-401: The code currently increments summary.indexed by
docs.length after adding to BM25 even if vector indexing
(this.embedding.embedBatch / this.vector.storeBatch) fails; change this so
summary distinguishes BM25 vs vector success: keep summary.indexed to count BM25
inserts (increment after bm25.addDocument loop) and add a new field like
summary.vectorIndexed (or decrement summary.indexed if you prefer a single
metric) that you increment only after successful embedding + storeBatch; in the
catch block for embedBatch/storeBatch, ensure you record the failure in summary
(e.g., summary.vectorIndexed = 0 or summary.vectorErrors += docs.length) and
include the error message as already done.
In `@packages/sandbox-e2b/src/index.ts`:
- Around line 349-356: The current execute flow calls this.getSandbox() before
arming the timeout, so sandbox provisioning can hang outside
timeoutMs/defaultTimeoutMs; wrap the entire execute logic (including the call to
this.getSandbox()) in a timeout-aware Promise (or move the timeout setup to
occur before calling getSandbox) so that the timeoutMs/defaultTimeoutMs is
enforced for sandbox creation as well; ensure you reference execute,
this.getSandbox, timeoutMs, defaultTimeoutMs and clear/cleanup the timer or
cancel the race when the real execution completes to avoid leaks and return a
proper timeout error when the timer fires.
🧹 Nitpick comments (11)
packages/core/src/workspace/filesystem/utils.ts (3)
84-93: Dead branch:typeof contentcheck is always true.
contentis typed asstring, so the ternary on line 85 always takes the truthy branch. Either widen the parameter type tostring | string[](if the function should accept both) or drop the guard.Suggested fix (if only string is intended)
export function createFileData(content: string, createdAt?: string): FileData { - const lines = typeof content === "string" ? content.split("\n") : content; + const lines = content.split("\n"); const now = new Date().toISOString();Same issue in
updateFileDataon line 96.
165-178: Unreachable throw: the fallback on line 166 prevents the empty-path check from ever triggering.
path || "/"guaranteespathStris at least"/", so the condition on line 167 is always false and thethrowon line 168 is dead code.If you want to reject empty/missing paths, move the check before the fallback:
Suggested fix
export function validatePath(path: string | null | undefined): string { - const pathStr = path || "/"; - if (!pathStr || pathStr.trim() === "") { + if (!path || path.trim() === "") { throw new Error("Path cannot be empty"); } - - let normalized = pathStr.startsWith("/") ? pathStr : `/${pathStr}`; + let normalized = path.startsWith("/") ? path : `/${path}`;Or, if defaulting to
"/"is the desired behaviour, remove the dead guard.
106-123: Minor: join-then-split round-trip onfileData.content.
fileDataToStringjoins the content array with"\n"(line 107), then line 113 immediately splits it back. You could work directly withfileData.contentand skip the round-trip.packages/core/src/workspace/search/index.ts (2)
86-107: Unreachable fallback inresolveEmbeddingAdapter.Line 106 is reached only if
embeddingis truthy, not anEmbeddingAdapter, not astring, and not anEmbeddingAdapterConfig. Given the type checks above, this branch appears unreachable. Passing an unexpected value silently toAiSdkEmbeddingAdaptercould mask a bug.Suggested fix
- return new AiSdkEmbeddingAdapter(embedding); + throw new Error("Unsupported embedding adapter input type.");
645-649:value as neverbypasses type safety.
setAttributehas strict overloads, and casting toneversilences the compiler entirely. This could allow non-serializable values (objects, arrays) to slip through, which OpenTelemetry will silently drop or mishandle. As per coding guidelines, "Maintain type safety in TypeScript-first codebase".Suggested fix — narrow to accepted attribute types
for (const [key, value] of Object.entries(attributes)) { - if (value !== undefined) { - toolSpan.setAttribute(key, value as never); + if (value !== undefined && value !== null) { + if ( + typeof value === "string" || + typeof value === "number" || + typeof value === "boolean" + ) { + toolSpan.setAttribute(key, value); + } } }packages/core/src/workspace/filesystem/backends/composite.ts (1)
21-26: Route keys are never normalized to end with/, but many methods assume they do.Multiple methods (e.g.,
lsInfoline 86,grepRawline 192,globInfoline 245/258,statline 145) callroutePrefix.slice(0, -1)assuming a trailing/. Other methods likegetBackendAndKeyusekey.startsWith(prefix)with the raw prefix. If a consumer passes a route key without a trailing slash (e.g.,"/data"instead of"/data/"),getBackendAndKeywill false-match longer paths (e.g.,"/database/x"), andslice(0, -1)will corrupt the prefix.Consider normalizing in the constructor:
Proposed fix
constructor(defaultBackend: FilesystemBackend, routes: Record<string, FilesystemBackend>) { this.defaultBackend = defaultBackend; - this.routes = routes; - - this.sortedRoutes = Object.entries(routes).sort((a, b) => b[0].length - a[0].length); + // Normalize all route keys to have a trailing slash + const normalized: Record<string, FilesystemBackend> = {}; + for (const [key, backend] of Object.entries(routes)) { + const normalizedKey = key.endsWith("/") ? key : `${key}/`; + normalized[normalizedKey] = backend; + } + this.routes = normalized; + this.sortedRoutes = Object.entries(normalized).sort((a, b) => b[0].length - a[0].length); }This would eliminate the need for per-method normalization guards and fix
getBackendAndKey's false-match risk.#!/bin/bash # Check how routes are constructed by callers to understand if keys always have trailing slashes rg -n 'CompositeFilesystemBackend' --type=ts -C5packages/sandbox-e2b/src/index.ts (2)
166-171:buildCommandLineescapes the command name itself — intentional?
escapeShellArgis applied to every element includingcommand. This is safe for simple binaries and paths-with-spaces, but it will break compound shell expressions passed as the command (e.g.,echo hello | wc -l). If such usage is intentionally unsupported, a brief doc comment clarifying thatcommandmust be a single executable (not a shell expression) would help consumers.
382-387: Dual timeout: localsetTimeoutand E2B SDKtimeoutMsmay interact.
timeoutMsis passed to the SDK'srunOptions(line 424) and used for a localsetTimeout(line 383). Both can independently try to kill/abort the command. This isn't incorrect — the local kill is best-effort and safe — but it means the E2B SDK may throw its own timeout error before or after the local timer fires, producing slightly inconsistent error surfaces (E2B exception vs. thetimedOutflag).If the intent is for the local timeout to be authoritative, consider omitting
timeoutMsfromrunOptions(or setting it to a slightly larger grace value) so only one timeout path fires.Also applies to: 423-425
packages/core/src/workspace/filesystem/backends/filesystem.ts (3)
211-238: Virtual path computation duplicatestoVirtualPath.Lines 212–222 reimplement the same relative-path-to-virtual-path logic already encapsulated in
toVirtualPath(lines 148–166). Consider reusingtoVirtualPath(fullPath, isDir)here to keep the mapping logic in one place.♻️ Suggested simplification
} else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { + if (isFile) { + const virtPath = this.toVirtualPath(fullPath, false); results.push({ path: virtPath, is_dir: false, size: entryStat.size, modified_at: entryStat.mtime.toISOString(), }); } else if (isDir) { + const virtPath = this.toVirtualPath(fullPath, true); results.push({ - path: `${virtPath}/`, + path: virtPath, is_dir: true, size: 0, modified_at: entryStat.mtime.toISOString(), }); } }
647-726:ripgrepSearchhas no match-count limit, risking unbounded memory usage.
fallbackSearchenforcesMAX_GREP_MATCHES, butripgrepSearchbuffers all rg--jsonoutput into a single string (line 665–668) with no cap. On a large repo with a common pattern, this can produce hundreds of megabytes of JSON in memory.Pass
--max-countto rg to stay consistent with the fallback path:♻️ Proposed fix
const args = ["--json"]; if (literal) { args.push("-F"); } if (includeGlob) { args.push("--glob", includeGlob); } + args.push("--max-count", String(MAX_GREP_MATCHES)); args.push("--", pattern, baseFull);Note:
--max-countis per-file in rg, so you may also want to post-process and break out of parsing once total matches reachMAX_GREP_MATCHES.
881-901: Same virtual-path duplication aslsInfo— could reusetoVirtualPath.Same pattern as noted in
lsInfo: the relative-to-virtual path assembly here is a copy oftoVirtualPath. Consolidating would reduce the surface for inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 199-212: The current async IIFE assigned to this.ready swallows
errors from workspaceInitPromise and finalizeInit (logged via logger.error) so
await voltAgent.ready never rejects; change it so fatal finalizeInit errors are
re-thrown after logging (so this.ready rejects) while keeping
workspaceInitPromise failures non-fatal by logging and recording a visible state
(e.g., set this.initError or this.degraded flag) that callers can inspect;
specifically, inside the catch for workspaceInitPromise keep the logger.error
but also assign the error to this.initError or this.degraded, and inside the
catch for finalizeInit call logger.error(...) and then throw the error to allow
the outer async IIFE (this.ready) to reject.
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 647-726: ripgrepSearch currently accumulates unlimited matches
into the output buffer and results map (unlike fallbackSearch which enforces
MAX_GREP_MATCHES), so add a guard to limit total matches to MAX_GREP_MATCHES:
either pass a safe --max-count value to the rg spawn args (in ripgrepSearch
where args is built) or track a match counter while parsing proc.stdout
output/JSON lines and stop parsing/early resolve once count reaches
MAX_GREP_MATCHES; ensure you stop adding to results (the results object) and
stop/respect the child process lifecycle (proc) when the limit is hit.
In `@packages/core/src/workspace/search/index.ts`:
- Around line 128-139: The matchesPathFilter function leaves a leading slash in
the relative path when basePath is present, causing micromatch to fail for globs
like "*.ts"; modify the computation of relative (in matchesPathFilter) so that
after slicing with docPath.slice(basePath.length) you strip any leading slashes
(e.g. .replace(/^\/+/, "")) before calling micromatch.isMatch, ensuring the
basePath and null-basePath branches produce the same normalized relative path
for glob matching.
In `@packages/sandbox-e2b/src/index.ts`:
- Around line 488-495: The code unconditionally sets runOptions.timeoutMs
because effectiveTimeoutMs was computed with Math.max(0, ...), so change the
assignment guard in the block that sets runOptions.timeoutMs to only apply when
effectiveTimeoutMs > 0 (not >= 0) so that a caller passing timeoutMs: 0 will
omit the timeout key and let the E2B SDK use its default; update the condition
around the runOptions.timeoutMs assignment (the block referencing
effectiveTimeoutMs and runOptions) accordingly while leaving
runOptions.requestTimeoutMs logic unchanged.
🧹 Nitpick comments (3)
packages/core/src/voltagent.ts (1)
119-197:finalizeInitmixes synchronous registration with fire-and-forget async work —readyresolves before the server is actually listening.
startServer()(line 190) andcheckDependencies()(line 182) are launched but not awaited insidefinalizeInit. This meansawait voltAgent.readyresolves before the HTTP server is bound to a port. If downstream code relies onreadyto mean "server is accepting requests," it will race.If this is intentional (server start is best-effort background work), a brief doc comment on
readyclarifying what it guarantees would help prevent misuse.packages/core/src/workspace/filesystem/backends/filesystem.ts (2)
211-238: Virtual path computation is duplicated acrosslsInfo,globInfo, andtoVirtualPath.
toVirtualPath(Line 148) already encapsulates this cwd-stripping + normalization logic, butlsInfoandglobInfo(Line 882) reimplement it inline. This creates a maintenance risk where changes to path formatting need to be applied in three places.♻️ Example: Replace inline logic with toVirtualPath
For
lsInfo, the block at lines 211–238 could be simplified:} else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { - results.push({ - path: virtPath, - is_dir: false, - size: entryStat.size, - modified_at: entryStat.mtime.toISOString(), - }); - } else if (isDir) { - results.push({ - path: `${virtPath}/`, - is_dir: true, - size: 0, - modified_at: entryStat.mtime.toISOString(), - }); - } + if (isFile) { + results.push({ + path: this.toVirtualPath(fullPath, false), + is_dir: false, + size: entryStat.size, + modified_at: entryStat.mtime.toISOString(), + }); + } else if (isDir) { + results.push({ + path: this.toVirtualPath(fullPath, true), + is_dir: true, + size: 0, + modified_at: entryStat.mtime.toISOString(), + }); + } }Same refactor applies to
globInfo(lines 882–901).
746-748:fs.statonbaseFullfollows symlinks — usefs.lstatfor consistency with the rest of the class.While
assertPathContained(called ingrepRawat Line 622) already validated the real path, every other stat call in this class usesfs.lstat. IfbaseFullis a symlink,fs.statwould resolve it silently;fs.lstatwould let you detect and skip it.♻️ Proposed fix
- const stat = await fs.stat(baseFull); + const stat = await fs.lstat(baseFull); + if (stat.isSymbolicLink()) { + return {}; + } const root = stat.isDirectory() ? baseFull : path.dirname(baseFull);
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/workspace/filesystem/backends/composite.ts">
<violation number="1" location="packages/core/src/workspace/filesystem/backends/composite.ts:180">
P2: The new prefix matching in grepRaw will also match sibling paths like "/foobar" for a mount at "/foo", which can route to the wrong backend and produce a searchPath missing the leading slash. Restore boundary checking so only the mount root or subpaths match.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ): Promise<GrepMatch[] | string> { | ||
| for (const [routePrefix, backend] of this.sortedRoutes) { | ||
| const prefix = routePrefix.replace(/\/$/, ""); | ||
| if (path.startsWith(prefix)) { |
There was a problem hiding this comment.
P2: The new prefix matching in grepRaw will also match sibling paths like "/foobar" for a mount at "/foo", which can route to the wrong backend and produce a searchPath missing the leading slash. Restore boundary checking so only the mount root or subpaths match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/workspace/filesystem/backends/composite.ts, line 180:
<comment>The new prefix matching in grepRaw will also match sibling paths like "/foobar" for a mount at "/foo", which can route to the wrong backend and produce a searchPath missing the leading slash. Restore boundary checking so only the mount root or subpaths match.</comment>
<file context>
@@ -176,11 +176,9 @@ export class CompositeFilesystemBackend implements FilesystemBackend {
- const suffix = path === prefixRoot ? "" : path.substring(normalizedPrefix.length);
- const searchPath = suffix ? `/${suffix}` : "/";
+ const prefix = routePrefix.replace(/\/$/, "");
+ if (path.startsWith(prefix)) {
+ const searchPath = path.substring(prefix.length) || "/";
const raw = await backend.grepRaw(pattern, searchPath, glob);
</file context>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 201-217: this.ready can reject (e.g., if finalizeInit() throws)
and if nobody awaits it that rejection becomes unhandled; fix by appending a
safety catch to the promise assigned to this.ready that logs the error and sets
the same state the constructor would (e.g., set this.initError and
this.degraded) to avoid an unhandled rejection: update the IIFE that assigns
this.ready (the code referencing workspaceInitPromise, finalizeInit,
this.initError, this.degraded) to include .catch(error => { logger.error("Agent
initialization failed:", { error }); this.initError = error; this.degraded =
true; }) so errors are surfaced/logged even when callers do not await
this.ready.
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 793-794: The call uses fs.stat which follows symlinks,
inconsistent with the rest of filesystem.ts and weakening defense-in-depth;
update the code in the grepRaw flow to use fs.lstat instead of fs.stat when
inspecting baseFull (the variable passed into assertPathContained earlier) so
that isDirectory() reflects the symlink itself rather than its target—locate the
block around grepRaw where baseFull is lstat/stat'd and replace the fs.stat
usage with fs.lstat to match other calls in this class.
In `@packages/sandbox-e2b/src/index.ts`:
- Around line 166-171: The function buildCommandLine inconsistently skips
escaping the command when args is empty, allowing shell metacharacters to pass
through; always pass the command through escapeShellArg so it is escaped in both
branches (i.e., call escapeShellArg(command) before returning when args is
undefined/empty) and keep the existing behavior of escaping each arg via
escapeShellArg for the args path; update any docstring/comment to state that
buildCommandLine escapes the command and arguments so callers should supply a
single program name in command.
🧹 Nitpick comments (1)
packages/core/src/workspace/filesystem/backends/filesystem.ts (1)
211-238: DRY: inline virtual-path logic duplicatestoVirtualPath.The virtual path computation here (and similarly in
globInfoat Lines 928–947) duplicates the logic already intoVirtualPath. Consider reusing that helper to reduce maintenance burden and risk of divergence.♻️ Example refactor for lsInfo
- } else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { - results.push({ - path: virtPath, - is_dir: false, - size: entryStat.size, - modified_at: entryStat.mtime.toISOString(), - }); - } else if (isDir) { - results.push({ - path: `${virtPath}/`, - is_dir: true, - size: 0, - modified_at: entryStat.mtime.toISOString(), - }); - } - } + } else { + const virtPath = this.toVirtualPath(fullPath, isDir); + if (isFile || isDir) { + results.push({ + path: virtPath, + is_dir: isDir, + size: isFile ? entryStat.size : 0, + modified_at: entryStat.mtime.toISOString(), + }); + } + }
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/voltagent.ts">
<violation number="1" location="packages/core/src/voltagent.ts:217">
P2: `ready` no longer rejects on initialization failure because the new catch handler swallows the error. Callers awaiting `ready` will not be notified of failures, which can mask startup errors. Re-throw the error after setting flags so the promise still rejects.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/voltagent.ts`:
- Around line 201-221: The current initialization flow can overwrite an earlier
workspace init error when finalizeInit also throws; capture the workspace init
failure into a local variable (e.g., workspaceError) instead of only writing
this.initError immediately, then when finalizeInit throws combine both failures
rather than overwriting: if only one error occurred set this.initError to that
error, if both occurred set this.initError to an AggregateError (or an object
with { workspaceError, finalizeError }) and log both errors; ensure
this.degraded remains true and logs include both error details (referencing
this.ready, workspaceInitPromise, finalizeInit, this.initError, this.degraded).
In `@packages/core/src/workspace/filesystem/backends/filesystem.ts`:
- Around line 390-440: The write method has a TOCTOU race in the
non-SUPPORTS_NOFOLLOW branch: add a second verification immediately before
performing the actual write to ensure the target is still a regular file (or
absent) and not a symlink; specifically, inside the else branch that calls
fs.writeFile, perform a fresh fs.lstat(resolvedPath) (or handle ENOENT) and
replicate the same checks done earlier (stat.isSymbolicLink(), stat.isFile(),
and overwrite handling) so you refuse to write if it became a symlink or an
unexpected type, then proceed to fs.writeFile only after that re-check; keep the
same error messages and preserve behavior of write, resolvePath, and
assertPathContained.
🧹 Nitpick comments (3)
packages/core/src/workspace/filesystem/backends/filesystem.ts (2)
211-238: DRY: virtual-path mapping is inlined instead of reusingtoVirtualPath.
lsInfo(lines 211–238) andglobInfo(lines 928–947) both manually compute virtual paths with the samecwdStrprefix-stripping and separator-normalizing logic thattoVirtualPathalready encapsulates. This creates three copies of the same mapping logic.Consider reusing
toVirtualPathin both methods:♻️ Example for lsInfo (similar change applies to globInfo)
} else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { + if (isFile) { + const virtPath = this.toVirtualPath(fullPath, false); results.push({ path: virtPath, is_dir: false, size: entryStat.size, modified_at: entryStat.mtime.toISOString(), }); } else if (isDir) { + const virtPath = this.toVirtualPath(fullPath, true); results.push({ - path: `${virtPath}/`, + path: virtPath, is_dir: true, size: 0, modified_at: entryStat.mtime.toISOString(), }); }Also applies to: 928-947
302-303: Consider usingunknowninstead ofanyfor caught errors.Multiple catch blocks use
catch (e: any)and directly accesse.message. Withtsconfigstrict mode, caught values should beunknown. A small helper or inline guard likee instanceof Error ? e.message : String(e)would be more type-safe.As per coding guidelines, "Maintain type safety in TypeScript-first codebase".
Also applies to: 437-438, 544-545, 573-574, 599-600
packages/sandbox-e2b/src/index.ts (1)
54-56: Consider exportingE2BSdkSandboxfor consumer ergonomics.
E2BSandboxOptions.sandbox(line 20) is typed asE2BSdkSandbox, but this type is not exported. Consumers who want to pass a pre-created sandbox instance can't reference the type for their own declarations. Structural typing works at runtime, but exporting the type would improve IDE support and discoverability.Proposed fix
-type E2BSdkSandbox = { +export type E2BSdkSandbox = { commands: E2BCommandsClient; };You may also want to export
E2BCommandResultandE2BCommandHandleif consumers need to interact with raw results.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/workspace/skills/index.ts`:
- Around line 1258-1298: readSkillFile currently calls
context.skills.readFileContent(resolvedPath) without error handling, so IO
errors will propagate inconsistently compared to other handlers like searchTool;
wrap the call to context.skills.readFileContent inside a try-catch in
readSkillFile, catch any error, log or attach minimal context to the operation
(using operationContext and setWorkspaceSpanAttributes if appropriate), and
return a friendly error string (e.g., "Error reading skill file: <brief
message>") for all three kinds ("reference" | "script" | "asset") instead of
throwing; keep existing behavior of returning "(empty)" when content is falsy.
- Around line 450-460: The current ensureDiscovered implementation hides
failures by setting this.autoDiscoverPromise to a promise that swallows errors,
preventing retries because this.discovered remains false and the promise is
never reset; update ensureDiscovered so that when discoverSkills() rejects it
resets this.autoDiscoverPromise to undefined before rethrowing or returning a
rejected state (so callers can retry), and apply the same pattern to
ensureIndexed (reset this.autoIndexPromise on failure); also update the class
constructor/init logic that seeds these promises (the fields
autoDiscoverPromise/autoIndexPromise and discovered/indexed flags) so they are
only left set on success, ensuring subsequent calls to
ensureDiscovered/ensureIndexed will retry failed discovery/indexing attempts.
🧹 Nitpick comments (5)
packages/core/src/voltagent.ts (2)
201-262: The.catchdeduplication logic is overly defensive and hard to follow.The error-merging logic in the
.catchhandler (lines 241–262) re-checksinstanceof AggregateError, inspects.errorswithincludes(), and conditionally wraps/re-wraps — but the only path that reaches.catchis whenfinalizeErroris thrown (line 239), andthis.initErroris already set correctly before that throw. Every reachable branch is a no-op (the error is already present). This dead-but-complex code will confuse future readers.Consider simplifying to a minimal safety-net catch:
Suggested simplification
})().catch((error) => { - this.degraded = true; - if (this.initError) { - if (this.initError instanceof AggregateError) { - const aggregated = (this.initError as AggregateError).errors; - if (!aggregated.includes(error)) { - this.initError = new AggregateError( - [...aggregated, error], - "Agent initialization failed", - ); - } - } else if (this.initError !== error) { - this.initError = new AggregateError( - [this.initError, error], - "Agent initialization failed", - ); - } - } else { - this.initError = error; - } - logger.error("Agent initialization failed:", { error }); + // initError and degraded are already set in the try/catch blocks above. + // This catch only prevents an unhandled rejection. + if (!this.initError) { + this.initError = error; + this.degraded = true; + } });
121-199:finalizeInitcaptures the outer scope cleanly, butstartServerfailure exits the process.Lines 192–198: if
startServerfails,process.exit(1)is called. SincefinalizeInitnow runs inside an async IIFE tied tothis.ready, this fire-and-forgetstartServercall means its rejection is not surfaced throughthis.readyorthis.initError— the process just dies. This is likely intentional for DX but worth noting: callers whoawait voltAgent.readywon't get a chance to handle server-start failures gracefully.No change required if that's the desired behavior, but if the intent is to let callers handle it, consider propagating the error through
this.readyinstead of callingprocess.exit.packages/core/src/workspace/filesystem/backends/filesystem.ts (3)
28-28:SUPPORTS_NOFOLLOWcheck should also guard against a zero value.
fsSync.constants.O_NOFOLLOWcould theoretically be0on a platform that defines the constant but doesn't support the behavior. The current!== undefinedcheck would incorrectly treat that as supported, and theO_NOFOLLOWflag would silently have no effect throughout the file.Proposed fix
-const SUPPORTS_NOFOLLOW = fsSync.constants.O_NOFOLLOW !== undefined; +const SUPPORTS_NOFOLLOW = (fsSync.constants.O_NOFOLLOW ?? 0) !== 0;
211-238: DRY: virtual path computation duplicatestoVirtualPath.Lines 211–238 manually recompute the virtual path, but
toVirtualPath(fullPath, isDir)(line 148) already encapsulates this logic. The same duplication exists inglobInfo(lines 948–967).Proposed refactor for lsInfo
} else { - let relativePath: string; - if (fullPath.startsWith(cwdStr)) { - relativePath = fullPath.substring(cwdStr.length); - } else if (fullPath.startsWith(this.cwd)) { - relativePath = fullPath.substring(this.cwd.length).replace(/^[/\\]/, ""); - } else { - relativePath = fullPath; - } - - relativePath = relativePath.split(path.sep).join("/"); - const virtPath = `/${relativePath}`; - - if (isFile) { - results.push({ - path: virtPath, - is_dir: false, - size: entryStat.size, - modified_at: entryStat.mtime.toISOString(), - }); - } else if (isDir) { - results.push({ - path: `${virtPath}/`, - is_dir: true, - size: 0, - modified_at: entryStat.mtime.toISOString(), - }); - } + const virtPath = this.toVirtualPath(fullPath, isDir); + if (isFile || isDir) { + results.push({ + path: virtPath, + is_dir: isDir, + size: isFile ? entryStat.size : 0, + modified_at: entryStat.mtime.toISOString(), + }); + } }
302-303: Considerunknowninstead ofanyin catch clauses for type safety.Multiple catch blocks use
(e: any)(lines 302, 457, 493, 564, 593, 619). A small helper would maintain type safety per the project guidelines:const errorMessage = (e: unknown): string => e instanceof Error ? e.message : String(e);Then:
catch (e: unknown) { return \Error reading file '${filePath}': ${errorMessage(e)}`; }`As per coding guidelines, "Maintain type safety in TypeScript-first codebase".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/workspace/skills/index.ts`:
- Around line 482-505: The ensureRootPaths method currently caches a rejected
promise in this.rootResolvePromise when this.rootResolver throws; modify the
code so the async resolution assigned to this.rootResolvePromise uses a
try/catch that on error resets this.rootResolvePromise to undefined (so future
calls can retry) and rethrows the error; specifically update the IIFE assigned
to this.rootResolvePromise in ensureRootPaths to wrap the await of
this.rootResolver(...) and the subsequent setting of
this.rootPaths/this.rootResolved in a try block and in the catch block set
this.rootResolvePromise = undefined before throwing the error, keeping
normalizeRootPath, DEFAULT_SKILL_ROOTS, this.rootResolver, this.rootResolved,
and this.rootPaths logic intact.
🧹 Nitpick comments (1)
packages/core/src/workspace/skills/index.ts (1)
1005-1009: Path traversal check withincludes("..")is overly broad.This rejects legitimate filenames like
file..txtor directories namedsomething..else. Consider using a segment-based check instead.♻️ Suggested refinement
- if (normalized.includes("..")) { + if (normalized.split("/").some((segment) => segment === "..")) { return null; }
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/workspace/filesystem/backends/composite.ts">
<violation number="1" location="packages/core/src/workspace/filesystem/backends/composite.ts:186">
P2: When a mounted backend matches the path but returns an error string, the new behavior continues and searches other backends. This can return results from unrelated mounts or the default backend for a path that should be scoped to the matched route, instead of returning the mount error. Consider returning the error immediately for the matched route.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| errors.push(raw); | ||
| continue; |
There was a problem hiding this comment.
P2: When a mounted backend matches the path but returns an error string, the new behavior continues and searches other backends. This can return results from unrelated mounts or the default backend for a path that should be scoped to the matched route, instead of returning the mount error. Consider returning the error immediately for the matched route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/workspace/filesystem/backends/composite.ts, line 186:
<comment>When a mounted backend matches the path but returns an error string, the new behavior continues and searches other backends. This can return results from unrelated mounts or the default backend for a path that should be scoped to the matched route, instead of returning the mount error. Consider returning the error immediately for the matched route.</comment>
<file context>
@@ -175,14 +175,16 @@ export class CompositeFilesystemBackend implements FilesystemBackend {
if (typeof raw === "string") {
- return raw;
+ errors.push(raw);
+ continue;
}
</file context>
| errors.push(raw); | |
| continue; | |
| return raw; |
1 participant
…ox execution
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
What is the new behavior?
fixes (issue)
Notes for reviewers
Summary by cubic
Introduces an experimental Workspace for agents with filesystem, sandboxed command execution, search, and skills, plus global defaults, server APIs, docs, and an example. Adds optional E2B and Daytona sandbox providers, new observability attributes, and VoltAgent readiness. Addresses #1008.
New Features
Migration
Written for commit 0d28dc7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Examples
Documentation
Tests
Server
Public API