-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(harness): clarify skill shell path handling #3026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,8 +86,10 @@ public final class SkillPromptBuilder { | |
| 1. After loading a skill, look at its <files-root> in <available_skills> | ||
| 2. List its files: ls <files-root>/ | ||
| 3. Run scripts: python3 <files-root>/scripts/<script-name> | ||
| 4. Always use absolute paths derived from <files-root>; never invent paths | ||
| 5. If a script exists for the task, run it directly — do not rewrite its logic inline | ||
| 4. In the command, always use absolute paths derived from <files-root>; never invent paths | ||
| 5. The working_directory parameter accepts only workspace-relative paths; never pass <files-root> to it | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bullet says |
||
| 6. Omit working_directory unless the command needs a workspace-relative directory | ||
| 7. If a script exists for the task, run it directly — do not rewrite its logic inline | ||
| </code_execution> | ||
| """; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,9 @@ public String execute( | |
| @ToolParam( | ||
| name = "working_directory", | ||
| description = | ||
| "Working directory (relative to workspace root, optional)", | ||
| "Optional working directory relative to the workspace root." | ||
| + " Omit it when invoking an absolute path, such as a skill" | ||
| + " script.", | ||
| required = false) | ||
| String workingDirectory, | ||
| @ToolParam( | ||
|
|
@@ -66,7 +68,8 @@ public String execute( | |
| String wd = workingDirectory.strip(); | ||
| if (wd.startsWith("/") || wd.startsWith("~") || wd.contains("..")) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The message now promises that "absolute paths, '~', and '..' are not allowed", but the guard only catches POSIX-shaped absolute paths. A Windows-shaped one ( |
||
| return "Error: working_directory must be a relative path within the workspace" | ||
| + " (absolute paths, '~', and '..' are not allowed)."; | ||
| + " (absolute paths, '~', and '..' are not allowed). Put absolute paths in" | ||
| + " the command instead, or omit working_directory."; | ||
| } | ||
| effectiveCommand = | ||
| commandWithWorkingDirectory( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,15 +307,15 @@ The agent doesn't see this difference — `load_skill_through_path` always works | |
|
|
||
| ### `<files-root>` and shell execution | ||
|
|
||
| When a skill ships scripts (e.g. `scripts/run-checks.sh`), the agent needs an absolute path to invoke them via `execute_shell_command`. That path comes from the `<files-root>` element on each skill entry. Resolution depends on the filesystem mode: | ||
| When a skill ships scripts (e.g. `scripts/run-checks.sh`), the agent needs an absolute path to invoke them with the `execute` tool. That path comes from the `<files-root>` element on each skill entry. Resolution depends on the filesystem mode: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch renaming the tool here, but the rename is incomplete in the same doc set: |
||
|
|
||
| | FS mode (shell available?) | Workspace skill `<files-root>` | Marketplace skill `<files-root>` | | ||
| |----------------------------|--------------------------------|-----------------------------------| | ||
| | Sandbox | `/workspace/skills/<name>` | `/workspace/.skills-cache/<source>/<name>` | | ||
| | Local-with-shell | `<wsRoot>/skills/<name>` | `<wsRoot>/.skills-cache/<source>/<name>` | | ||
| | Local without shell / Composite | (not rendered — no shell tool registered) | (not rendered) | | ||
|
|
||
| So the agent's shell call is always `execute_shell_command("python3 <files-root>/scripts/foo.py")` — no path guessing, no per-source variations to remember. | ||
| So the agent always puts that path in the command, for example `execute(command="python3 <files-root>/scripts/foo.py")` — no path guessing, no per-source variations to remember. The optional `working_directory` parameter accepts only a path relative to the workspace root. Do not pass `<files-root>` to it; omit it when running skill scripts unless the command needs a workspace-relative working directory. | ||
|
|
||
| ### Where marketplace files actually live | ||
|
|
||
|
|
@@ -380,11 +380,11 @@ In sandbox mode, each skill's `<files-root>` in the `<available_skills>` block i | |
|
|
||
| So the agent simply issues: | ||
|
|
||
| ``` | ||
| execute_shell_command("python3 /workspace/skills/code-reviewer/scripts/run-checks.sh <target>") | ||
| ```text | ||
| execute(command="python3 /workspace/skills/code-reviewer/scripts/run-checks.sh <target>") | ||
| ``` | ||
|
|
||
| That command runs in the container and reads exactly the file that was projected in. The agent doesn't need to know which layer a skill came from — the framework computes the prefix. | ||
| That command runs in the container and reads exactly the file that was projected in. The absolute path stays in `command`; `working_directory` is omitted. The agent doesn't need to know which layer a skill came from — the framework computes the prefix. | ||
|
|
||
| > If a sandbox backend mounts the workspace at a non-default location (e.g. AgentRun uses `/home/agentscope/workspace`), the `<files-root>` prefix changes accordingly, and the agent still gets a correct absolute path. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same policy now lives in four places that must move together: this bullet list, the
working_directorydescription inShellExecuteTool, the runtime error string, anddocs/v2/{en,zh}/docs/harness/skill.md. The two new tests pin the prompt lines and the schema description independently, so any future rewording needs several coordinated edits. Would extracting the rule into one shared constant (consumed by both the prompt and the tool description) work here, or is the duplication deliberate because this prompt text is intentionally frozen?