Skip to content

Fix Optional Timeout Parameter in ShellExecuteTool#2108

Closed
hanydd wants to merge 1 commit into
agentscope-ai:mainfrom
hanydd:codex/fix-shell-execute-optional-params
Closed

Fix Optional Timeout Parameter in ShellExecuteTool#2108
hanydd wants to merge 1 commit into
agentscope-ai:mainfrom
hanydd:codex/fix-shell-execute-optional-params

Conversation

@hanydd

@hanydd hanydd commented Jul 10, 2026

Copy link
Copy Markdown

问题

  • working_directorytimeout 这两个参数在描述和实际代码中,都是有默认值的,但是实际注册schema的时候没有传入 @ToolParam(required=false)`,导致实际上这两个参数会被当成必填项进行校验,造成Agent调用频繁出错,造成效率损失。
  • timeout 参数是int类型,如果大模型没有返回这个值,那么反射处理参数的地方会传入null,传给primitive int会直接报错。

Summary

  • mark working_directory and timeout as optional in the generated schema
  • accept a nullable timeout and default it to 30 seconds when omitted or non-positive
  • add regression coverage for the schema, command-only reflective invocation, and explicit timeouts

Root cause

@ToolParam.required() defaults to true, but ShellExecuteTool did not override it for its optional parameters. In addition, the primitive int timeout could not receive the null produced for an omitted argument.

Testing

mvn -pl agentscope-harness -am "-Dtest=ShellExecuteToolTest" "-Dsurefire.failIfNoSpecifiedTests=false" "-Dspotless.check.skip=true" test

Result: 3 tests passed, 0 failures.

更新(2026-07-18)

更新原因

本 PR rebase 到最新 main 后,提交 0e67ac1be 已将 working_directory 标记为可选,并优化了 shell 工具的使用提示。为避免重复修改,本 PR 保留该提交的语义,调整为只补充 timeout 的可选参数支持。

当前调整

  • timeout 仍可由调用方显式修改,正数值会原样传递给 sandbox。
  • 省略 timeout 时允许反射调用器传入 null,并默认使用 30 秒。
  • timeout 为非正数时同样回退到 30 秒。
  • 生成的 Schema 只要求 command

最新验证

ShellExecuteToolTest 共 4 个测试全部通过,覆盖 Schema、仅传 command、显式 timeout 和非正数 timeout 的行为;timeout 新增判断的分支覆盖为 4/4。

@hanydd hanydd changed the title Fix optional parameters in ShellExecuteTool Fix Parameter Schema in ShellExecuteTool Jul 10, 2026
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@CLAassistant

CLAassistant commented Jul 10, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@hanydd
hanydd force-pushed the codex/fix-shell-execute-optional-params branch from fc730a0 to 95a05a8 Compare July 11, 2026 01:03
@hanydd
hanydd marked this pull request as ready for review July 11, 2026 01:16
@hanydd
hanydd requested a review from a team July 11, 2026 01:16
@hanydd
hanydd force-pushed the codex/fix-shell-execute-optional-params branch from 95a05a8 to f75cecf Compare July 11, 2026 01:41

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR modifies 2 file(s) (+111 -4).

Reviewed the changes and they look reasonable overall. No critical issues found.


Automated review by github-manager-bot

@AgentScopeJavaBot AgentScopeJavaBot added bug Something isn't working area/harness agentscope-harness (test/runtime support) labels Jul 11, 2026

@AgentScopeJavaBot AgentScopeJavaBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review

Fixes ShellExecuteTool schema: marks working_directory and timeout as optional, changes timeout to Integer for null safety. 4 tests. Clean fix.

@hanydd
hanydd force-pushed the codex/fix-shell-execute-optional-params branch from f75cecf to d4e5466 Compare July 18, 2026 05:27
@hanydd hanydd changed the title Fix Parameter Schema in ShellExecuteTool Fix Optional Timeout Parameter in ShellExecuteTool Jul 18, 2026
@hanydd hanydd closed this Jul 18, 2026
@hanydd
hanydd deleted the codex/fix-shell-execute-optional-params branch July 18, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/harness agentscope-harness (test/runtime support) bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants