fix(harness): clarify skill shell path handling - #3026
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ba4d0fd to
9792348
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Clarifying the working_directory contract in three places (tool description, prompt bullets, runtime error) plus a rename of the stale tool name in the harness skill docs — the direction is right, and the added tests pin both the schema description and the prompt wording, which is what makes this kind of guidance change safe to land. Two things worth a second look: the rejection guard still only recognises POSIX-shaped absolute paths (so a Windows path passes and the new message over-promises), and the execute_shell_command → execute rename stops at skill.md while the sibling workspace.md harness pages still name the old tool. Non-blocking, so leaving as comments.
Automated review by github-manager-bot
| @@ -66,7 +68,8 @@ public String execute( | |||
| String wd = workingDirectory.strip(); | |||
| if (wd.startsWith("/") || wd.startsWith("~") || wd.contains("..")) { | |||
There was a problem hiding this comment.
The message now promises that "absolute paths, '~', and '..' are not allowed", but the guard only catches POSIX-shaped absolute paths. A Windows-shaped one (C:\workspace\skills\alpha, \\server\share\x, or /d/workspace/x) passes this check and reaches commandWithWorkingDirectory, which emits cd /d "C:\workspace\skills\alpha" && ... resolved against the sandbox process cwd — exactly the confusing failure this PR is removing, and the repo does build on windows-latest and already has a Windows branch here. Could the check also reject a drive-letter / leading-\ form? Then the new rejection test would have a Windows counterpart.
| 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 |
There was a problem hiding this comment.
This bullet says working_directory accepts "only workspace-relative paths", but the tool also rejects a leading ~ and any path containing ... A model that reads only the prompt will still emit working_directory="skills/../shared" and burn a tool call on the error. Since the stated goal of the change is to stop wasted calls, could this line state the same three rejections as the tool message (or just defer to it), so prompt and @ToolParam description express one rule instead of two partial ones?
| 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 |
There was a problem hiding this comment.
The same policy now lives in four places that must move together: this bullet list, the working_directory description in ShellExecuteTool, the runtime error string, and docs/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?
| ### `<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: |
There was a problem hiding this comment.
Good catch renaming the tool here, but the rename is incomplete in the same doc set: docs/v2/en/docs/harness/workspace.md:391 and docs/v2/zh/docs/harness/workspace.md:386 still say the agent invokes skill scripts "via execute_shell_command". On the harness path the registered shell tool is execute (ShellExecuteTool.NAME), so a reader following those harness docs ends up calling a tool that the harness does not register — the same class of mistake this PR exists to prevent. (execute_shell_command is still legitimate for users who register core ShellCommandTool themselves, and AgentSkillPromptProvider keeps that wording for the SkillBox path, so a scope note would be enough if that split is intentional.) Could you update the two workspace.md lines here, or file a follow-up so the harness docs name one tool consistently?
2 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Fixes #2788.
Skill execution guidance previously referred to a non-existent
execute_shell_commandtool and could lead agents to pass an absolute<files-root>path asworking_directory. However,working_directoryintentionally accepts only workspace-relative paths, so this caused skill script execution to be rejected.This PR:
executetool name in the generated skill prompt.command, whileworking_directorymust remain workspace-relative or be omitted.How to test:
Local results: 34 targeted tests passed. The clean Harness verification passed 989 tests with 6 environment-dependent tests skipped on Windows (5 Windows-disabled shell integration tests and 1 POSIX-permissions assumption).
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)