Skip to content

fix(tools): honor exec timeout and boolean run options - #3319

Open
MrTreasure wants to merge 1 commit into
sipeed:mainfrom
MrTreasure:fix/exec-run-options
Open

fix(tools): honor exec timeout and boolean run options#3319
MrTreasure wants to merge 1 commit into
sipeed:mainfrom
MrTreasure:fix/exec-run-options

Conversation

@MrTreasure

Copy link
Copy Markdown

📝 Description

The exec tool advertises a per-run timeout argument, but synchronous execution always used the configured global timeout and silently ignored the supplied value. The tool schema also declared background and pty as strings even though they are boolean options and the executor accepts boolean values.

This PR:

  • honors an explicitly supplied per-run timeout, including values longer or shorter than the configured default;
  • rejects negative or overflowing timeout values;
  • declares background and pty as JSON booleans;
  • tells the model to use background sessions with poll/read instead of synchronous sleeps after a timeout;
  • adds regression tests for the schema and both timeout override directions.

This is a focused follow-up to #2161, limited to the exec option contract. It also relates to the fixed global-timeout report in #1025.

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

🧪 Test Environment

  • Hardware: x86_64 NAS / PC
  • OS: Linux
  • Model/Provider: DeepSeek V4 Flash through an OpenAI-compatible provider
  • Channels: Weixin and CLI

Tests run:

  • go test ./pkg/tools -count=1
  • go vet ./pkg/tools
  • deployed the patched binary and verified the gateway health endpoint

make test was also run. It reached an unrelated existing failure in TestMCPAddRejectsNonExecutableLocalCommand because the NAS temporary filesystem does not preserve the test's non-executable mode; this branch does not modify MCP code.

📸 Evidence (Optional)

Behavior verified
  • A per-run timeout shorter than the configured default terminates at the supplied deadline.
  • A per-run timeout longer than the configured default allows the command to complete.
  • Boolean background and pty parameters pass schema validation.

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the tool description and regression tests accordingly.

@github-actions

Copy link
Copy Markdown

This PR has had no activity for 7 days and has been marked as stale. If you are still working on it, please push an update or leave a comment; otherwise it will be closed automatically in 7 days.

@github-actions github-actions Bot added the stale label Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant