fix: workspace skill prompt injection and guidance for skill access t…#1051
Conversation
🦋 Changeset detectedLatest commit: 6ae8490 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughReplaces full SKILL.md injection with metadata-only (name, id, description), instructs agents to use workspace skill tools in system prompts, infers skill file allowlists from instruction links, avoids per-skill async loads for activated lists, adds workspaceSkillsPrompt agent option, and normalizes/propagates sandbox command context. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant Workspace as Workspace
participant Hook as WorkspaceSkillsHook
participant AI as Model
participant Tools as WorkspaceSkillTools
Agent->>Workspace: initialize (workspaceSkillsPrompt option)
Workspace->>Hook: build skills prompt (metadata + inferred allowlists)
Hook->>Agent: onPrepareMessages adds <workspace_skills> system message
Agent->>AI: send system + user messages
AI->>Agent: response (may request tools)
Agent->>Tools: call workspace_read_skill / workspace_read_skill_script / other tools
Tools-->>Agent: tool outputs (files, scripts, assets)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying voltagent with
|
| Latest commit: |
6ae8490
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://66176d3b.voltagent.pages.dev |
| Branch Preview URL: | https://fix-workspace-skill.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.
2 issues found across 29 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/sandbox-daytona/src/index.ts">
<violation number="1" location="packages/sandbox-daytona/src/index.ts:64">
P2: tokenizeCommandLine drops empty quoted arguments, so commands like `cmd ""` lose the empty argument and change shell semantics. Preserve empty tokens when quotes are used.</violation>
</file>
<file name="packages/sandbox-e2b/src/index.ts">
<violation number="1" location="packages/sandbox-e2b/src/index.ts:81">
P2: Empty quoted arguments (e.g., `""` or `''`) are silently dropped by the tokenizer. `pushCurrent` skips tokens where `current.length === 0`, but an empty quoted string is a valid argument. For instance, `grep "" file.txt` would lose the empty pattern and become `["grep", "file.txt"]`, changing command semantics.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let quote: "'" | '"' | null = null; | ||
| let escapeNext = false; | ||
|
|
||
| const pushCurrent = () => { |
There was a problem hiding this comment.
P2: Empty quoted arguments (e.g., "" or '') are silently dropped by the tokenizer. pushCurrent skips tokens where current.length === 0, but an empty quoted string is a valid argument. For instance, grep "" file.txt would lose the empty pattern and become ["grep", "file.txt"], changing command semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sandbox-e2b/src/index.ts, line 81:
<comment>Empty quoted arguments (e.g., `""` or `''`) are silently dropped by the tokenizer. `pushCurrent` skips tokens where `current.length === 0`, but an empty quoted string is a valid argument. For instance, `grep "" file.txt` would lose the empty pattern and become `["grep", "file.txt"]`, changing command semantics.</comment>
<file context>
@@ -72,6 +72,105 @@ const DEFAULT_TIMEOUT_MS = 60000;
+ let quote: "'" | '"' | null = null;
+ let escapeNext = false;
+
+ const pushCurrent = () => {
+ if (current.length > 0) {
+ tokens.push(current);
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@examples/with-workspace/workspace/skills/playwright-cli/references/request-mocking.md`:
- Around line 30-35: The fenced code block in request-mocking.md that contains
the patterns (e.g., "**/api/users - Exact path match", "**/api/*/details",
"**/*.{png,jpg,jpeg}", "**/search?q=*") is missing a language specifier; update
the opening fence from ``` to something like ```text (or ```plaintext) so the
block has a language identifier and the MD040 lint warning is resolved.
In
`@examples/with-workspace/workspace/skills/playwright-cli/references/storage-state.md`:
- Around line 243-266: Update the example filenames to match the .gitignore
pattern '*.auth-state.json': replace occurrences of "auth.json" with
"auth.auth-state.json" (or "auth-state.auth-state.json") and "my-session.json"
with "my-session.auth-state.json" in the code blocks (e.g., the commands using
playwright-cli state-save/state-load and the roundtrip example). Also update the
other instances noted (around the 271-273 example) so all sample state files end
with ".auth-state.json" to align with the security guidance.
In `@packages/sandbox-daytona/src/index.ts`:
- Around line 57-154: Export normalizeCommandAndArgs (and its helper
tokenizeCommandLine if needed) from `@voltagent/core` by adding them to the
sandbox package's public exports (e.g., export from
packages/core/src/workspace/sandbox/index.ts and re-export via the package's
main entry), promote `@voltagent/core` from devDependency to a runtime dependency,
then remove the duplicate implementations in
packages/sandbox-daytona/src/index.ts and import normalizeCommandAndArgs from
'@voltagent/core' (use the symbol normalizeCommandAndArgs to locate call sites
and tokenizeCommandLine if referenced).
🧹 Nitpick comments (6)
examples/with-workspace/workspace/skills/playwright-cli/references/tracing.md (2)
7-18: Consider using more descriptive selector examples.The basic usage example uses generic selectors like
e1ande2without context. While these may represent Playwright CLI's selector notation, adding brief comments or using more descriptive examples would improve clarity for users unfamiliar with the tool.📝 Suggestion to improve example clarity
```bash # Start trace recording playwright-cli tracing-start # Perform actions playwright-cli open https://example.com -playwright-cli click e1 -playwright-cli fill e2 "test" +playwright-cli click e1 # Click the login button +playwright-cli fill e2 "test" # Fill the username field # Stop trace recording playwright-cli tracing-stop</details> --- `134-135`: **Consider a safer approach for the cleanup command.** The `find -delete` command operates immediately without confirmation, which could be dangerous if the path is mistyped or if users adapt the command incorrectly. Consider adding a warning or showing a two-step approach (preview first, then delete). <details> <summary>🛡️ Safer alternative for cleanup command</summary> ```diff -# Remove traces older than 7 days -find .playwright-cli/traces -mtime +7 -delete +# Preview traces older than 7 days before deleting +find .playwright-cli/traces -mtime +7 -type f + +# After verifying, remove them +find .playwright-cli/traces -mtime +7 -type f -deleteOr add a safety note:
# Remove traces older than 7 days +# ⚠️ Verify the path before running - this deletes files immediately find .playwright-cli/traces -mtime +7 -deletepackages/core/src/workspace/sandbox/local.ts (1)
503-504: Minor: redundant.trim()call.
normalizeCommandAndArgsalready trims the command internally (line 81 in command-normalization.ts:command.trim()). The.trim()on line 504 is redundant. Not a bug, just a nit.packages/sandbox-e2b/src/index.ts (2)
432-433: Redundant.trim()—normalizeCommandAndArgsalready trims the command.
normalizeCommandAndArgstrims the command internally (line 150), so the subsequent.trim()on line 433 is a no-op. Not a bug, but unnecessary.🧹 Suggested simplification
const normalized = normalizeCommandAndArgs(options.command ?? "", options.args); - const command = normalized.command.trim(); + const command = normalized.command;
75-172: Duplicated command normalization logic — consider consolidating or exporting from@voltagent/core.
tokenizeCommandLineandnormalizeCommandAndArgsare exact copies of the implementations inpackages/core/src/workspace/sandbox/command-normalization.ts. These functions are not currently exported from the core package's public API, but consolidating them would eliminate maintenance burden. Options:
- Export
normalizeCommandAndArgsandNormalizedCommandtype from@voltagent/core, then import here.- Create a shared internal utility if these should remain private to core.
Additionally, line 433's
normalized.command.trim()is redundant—the command is already trimmed withinnormalizeCommandAndArgs.packages/core/src/workspace/sandbox/toolkit.ts (1)
158-175: Type assertion toWorkspaceWithPathContextis safe but brittle.The cast on line 165 assumes the
workspaceobject may have agetPathContextmethod not declared inWorkspaceIdentity. This works because the optional chaining + try/catch (lines 166-174) make it safe at runtime, but it relies on duck-typing rather than the type system.Consider extending
WorkspaceIdentity(or using a union type inWorkspaceSandboxToolkitContext) to express thatworkspacemay carrygetPathContext, which would remove the need for theascast.
| const tokenizeCommandLine = (value: string): string[] | null => { | ||
| const tokens: string[] = []; | ||
| let current = ""; | ||
| let quote: "'" | '"' | null = null; | ||
| let escapeNext = false; | ||
|
|
||
| const pushCurrent = () => { | ||
| if (current.length > 0) { | ||
| tokens.push(current); | ||
| current = ""; | ||
| } | ||
| }; | ||
|
|
||
| for (const char of value) { | ||
| if (escapeNext) { | ||
| current += char; | ||
| escapeNext = false; | ||
| continue; | ||
| } | ||
|
|
||
| if (quote === null) { | ||
| if (char === "\\") { | ||
| escapeNext = true; | ||
| continue; | ||
| } | ||
| if (char === "'" || char === '"') { | ||
| quote = char; | ||
| continue; | ||
| } | ||
| if (/\s/.test(char)) { | ||
| pushCurrent(); | ||
| continue; | ||
| } | ||
| current += char; | ||
| continue; | ||
| } | ||
|
|
||
| if (quote === "'") { | ||
| if (char === "'") { | ||
| quote = null; | ||
| } else { | ||
| current += char; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '"') { | ||
| quote = null; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === "\\") { | ||
| escapeNext = true; | ||
| continue; | ||
| } | ||
|
|
||
| current += char; | ||
| } | ||
|
|
||
| if (escapeNext) { | ||
| current += "\\"; | ||
| } | ||
|
|
||
| if (quote !== null) { | ||
| return null; | ||
| } | ||
|
|
||
| pushCurrent(); | ||
| return tokens.length > 0 ? tokens : null; | ||
| }; | ||
|
|
||
| const normalizeCommandAndArgs = ( | ||
| command: string, | ||
| args?: string[], | ||
| ): { command: string; args?: string[] } => { | ||
| const trimmedCommand = command.trim(); | ||
| const normalizedArgs = args && args.length > 0 ? args : undefined; | ||
|
|
||
| if (!trimmedCommand) { | ||
| return { command: trimmedCommand, args: normalizedArgs }; | ||
| } | ||
|
|
||
| if (!/\s/.test(trimmedCommand)) { | ||
| return { command: trimmedCommand, args: normalizedArgs }; | ||
| } | ||
|
|
||
| const parsed = tokenizeCommandLine(trimmedCommand); | ||
| if (!parsed || parsed.length === 0) { | ||
| return { command: trimmedCommand, args: normalizedArgs }; | ||
| } | ||
|
|
||
| const [normalizedCommand, ...parsedArgs] = parsed; | ||
| const mergedArgs = [...parsedArgs, ...(normalizedArgs ?? [])]; | ||
| return { | ||
| command: normalizedCommand, | ||
| args: mergedArgs.length > 0 ? mergedArgs : undefined, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the command-normalization.ts file in core
find . -path '*/node_modules' -prune -o -name 'command-normalization.ts' -type f -printRepository: VoltAgent/voltagent
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check the structure of packages/core to understand exports
ls -la packages/core/src/ 2>/dev/null | head -20Repository: VoltAgent/voltagent
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Look at packages/core's index or public entry points
find packages/core/src -name 'index.ts' -type f | head -5Repository: VoltAgent/voltagent
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Search for normalizeCommandAndArgs exports in core
rg -n 'export.*normalizeCommandAndArgs' --type=tsRepository: VoltAgent/voltagent
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Read the command-normalization.ts from core to compare
cat -n packages/core/src/workspace/sandbox/command-normalization.tsRepository: VoltAgent/voltagent
Length of output: 2875
🏁 Script executed:
#!/bin/bash
# Read lines 57-154 from sandbox-daytona/src/index.ts
sed -n '57,154p' packages/sandbox-daytona/src/index.tsRepository: VoltAgent/voltagent
Length of output: 2086
🏁 Script executed:
#!/bin/bash
# Check if command-normalization exports are in core's public index.ts
grep -n 'command-normalization' packages/core/src/index.tsRepository: VoltAgent/voltagent
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check sandbox-daytona's package.json dependencies
cat packages/sandbox-daytona/package.json | grep -A 20 '"dependencies"'Repository: VoltAgent/voltagent
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Check the workspace/sandbox directory structure in core
find packages/core/src/workspace/sandbox -type f -name '*.ts' | head -20Repository: VoltAgent/voltagent
Length of output: 407
🏁 Script executed:
#!/bin/bash
# Check what's exported from workspace/sandbox/index if it exists
cat packages/core/src/workspace/sandbox/index.ts 2>/dev/null || echo "No index.ts found"Repository: VoltAgent/voltagent
Length of output: 604
🏁 Script executed:
#!/bin/bash
# Look at how other packages import from `@voltagent/core`
rg 'from.*@voltagent/core' --type=ts | grep -v node_modules | head -15Repository: VoltAgent/voltagent
Length of output: 1510
🏁 Script executed:
#!/bin/bash
# Verify that `@voltagent/core` is indeed only in devDependencies for sandbox-daytona
cat packages/sandbox-daytona/package.json | jq '.dependencies, .devDependencies'Repository: VoltAgent/voltagent
Length of output: 205
Remove duplication by exporting normalizeCommandAndArgs from @voltagent/core's public API, then import it in sandbox-daytona.
The tokenizeCommandLine and normalizeCommandAndArgs functions in lines 57-154 are identical to those in packages/core/src/workspace/sandbox/command-normalization.ts. Currently, these functions are not re-exported from @voltagent/core's public API, making it impossible to import them directly. To enable this refactor, they must first be added to the exports in packages/core/src/workspace/sandbox/index.ts and made available through the package's public entry point. Once exported, update sandbox-daytona to import from @voltagent/core instead of maintaining a duplicate copy. Note: @voltagent/core is currently only a devDependency; it will need to be promoted to a runtime dependency.
🤖 Prompt for AI Agents
In `@packages/sandbox-daytona/src/index.ts` around lines 57 - 154, Export
normalizeCommandAndArgs (and its helper tokenizeCommandLine if needed) from
`@voltagent/core` by adding them to the sandbox package's public exports (e.g.,
export from packages/core/src/workspace/sandbox/index.ts and re-export via the
package's main entry), promote `@voltagent/core` from devDependency to a runtime
dependency, then remove the duplicate implementations in
packages/sandbox-daytona/src/index.ts and import normalizeCommandAndArgs from
'@voltagent/core' (use the symbol normalizeCommandAndArgs to locate call sites
and tokenizeCommandLine if referenced).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/agent/agent.ts`:
- Around line 369-388: The resolveWorkspaceSkillsPromptHook function skips
injecting the default workspace skills prompt whenever
options.hooks?.onPrepareMessages exists; change it so the default prompt is only
disabled when options.workspaceSkillsPrompt === false. Remove or modify the
early return that checks options.hooks?.onPrepareMessages and instead return a
hook (from resolveWorkspaceSkillsPromptHook) that composes with an existing
onPrepareMessages: call the existing hook if present and then inject or merge
the workspace skills prompt (respecting promptConfig and
isWorkspaceSkillsToolkitEnabled) so custom hooks and the default skills prompt
both run unless workspaceSkillsPrompt is explicitly false.
🧹 Nitpick comments (2)
packages/core/src/workspace/sandbox/toolkit.ts (2)
158-175: Theascast is guarded but could benefit from a narrowing type guard.Line 165 uses
as WorkspaceWithPathContextwhich bypasses type checking. While the runtime guard on line 166 (?.getPathContext) makes this safe in practice, a type predicate would be more idiomatic TypeScript and catch regressions at compile time.♻️ Optional: extract a type guard
+const hasPathContext = ( + workspace: WorkspaceIdentity | undefined, +): workspace is WorkspaceWithPathContext => + typeof (workspace as WorkspaceWithPathContext)?.getPathContext === "function"; + const resolvePathContext = ( context: WorkspaceSandboxToolkitContext, ): WorkspacePathContext | null => { if (context.pathContext) { return context.pathContext; } - const workspaceWithPathContext = context.workspace as WorkspaceWithPathContext | undefined; - if (!workspaceWithPathContext?.getPathContext) { + if (!hasPathContext(context.workspace)) { return null; } try { - return workspaceWithPathContext.getPathContext() ?? null; + return context.workspace.getPathContext() ?? null; } catch { return null; } };
265-271: Null-handling semantics differ betweensystemPromptandcustomToolDescription.
systemPromptuses strict=== undefined(line 271), lettingnullpass through to explicitly disable the prompt.customToolDescriptionuses||(line 303), which collapses bothnulland""to the default. This appears intentional (disabling the tool description has no practical use), but documenting this distinction inWorkspaceSandboxToolkitOptionswould prevent future confusion.Also applies to: 301-303
| const resolveWorkspaceSkillsPromptHook = ( | ||
| workspace: Workspace | undefined, | ||
| options: AgentOptions, | ||
| ): AgentHooks["onPrepareMessages"] | undefined => { | ||
| if (!workspace?.skills) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const promptConfig = options.workspaceSkillsPrompt; | ||
| if (promptConfig === false) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const hasExplicitPromptConfig = promptConfig !== undefined; | ||
| if (!hasExplicitPromptConfig && options.hooks?.onPrepareMessages) { | ||
| return undefined; | ||
| } | ||
| if (!hasExplicitPromptConfig && !isWorkspaceSkillsToolkitEnabled(options.workspaceToolkits)) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Default workspace skills prompt is skipped when a custom onPrepareMessages hook is present.
Line 383-385 short-circuits default injection whenever options.hooks?.onPrepareMessages is set. That contradicts the “auto‑inject by default” objective and means agents with custom hooks won’t get skills guidance unless they explicitly opt in. Consider letting the composition handle ordering and only disable via workspaceSkillsPrompt === false.
💡 Suggested fix
- if (!hasExplicitPromptConfig && options.hooks?.onPrepareMessages) {
- return undefined;
- }📝 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.
| const resolveWorkspaceSkillsPromptHook = ( | |
| workspace: Workspace | undefined, | |
| options: AgentOptions, | |
| ): AgentHooks["onPrepareMessages"] | undefined => { | |
| if (!workspace?.skills) { | |
| return undefined; | |
| } | |
| const promptConfig = options.workspaceSkillsPrompt; | |
| if (promptConfig === false) { | |
| return undefined; | |
| } | |
| const hasExplicitPromptConfig = promptConfig !== undefined; | |
| if (!hasExplicitPromptConfig && options.hooks?.onPrepareMessages) { | |
| return undefined; | |
| } | |
| if (!hasExplicitPromptConfig && !isWorkspaceSkillsToolkitEnabled(options.workspaceToolkits)) { | |
| return undefined; | |
| } | |
| const resolveWorkspaceSkillsPromptHook = ( | |
| workspace: Workspace | undefined, | |
| options: AgentOptions, | |
| ): AgentHooks["onPrepareMessages"] | undefined => { | |
| if (!workspace?.skills) { | |
| return undefined; | |
| } | |
| const promptConfig = options.workspaceSkillsPrompt; | |
| if (promptConfig === false) { | |
| return undefined; | |
| } | |
| const hasExplicitPromptConfig = promptConfig !== undefined; | |
| if (!hasExplicitPromptConfig && !isWorkspaceSkillsToolkitEnabled(options.workspaceToolkits)) { | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/agent/agent.ts` around lines 369 - 388, The
resolveWorkspaceSkillsPromptHook function skips injecting the default workspace
skills prompt whenever options.hooks?.onPrepareMessages exists; change it so the
default prompt is only disabled when options.workspaceSkillsPrompt === false.
Remove or modify the early return that checks options.hooks?.onPrepareMessages
and instead return a hook (from resolveWorkspaceSkillsPromptHook) that composes
with an existing onPrepareMessages: call the existing hook if present and then
inject or merge the workspace skills prompt (respecting promptConfig and
isWorkspaceSkillsToolkitEnabled) so custom hooks and the default skills prompt
both run unless workspaceSkillsPrompt is explicitly false.
1 participant
…ools
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
Auto-inject a concise workspace skills prompt that lists metadata only and clearly guides agents to use skill tools; improve sandbox and filesystem tooling with better command normalization, quoted-arg handling, path context, structured outputs, and tool‑arg coercion. Infer allowed files from links in third‑party SKILL.md and update examples/docs. Fixes #1045.
New Features
Bug Fixes
Written for commit 6ae8490. Summary will update on new commits.
Summary by CodeRabbit
Improvements
New Features
Documentation
Tests