Repository navigation
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The deadline does what it says. A temporary test (a tool that never finishes, test clock moved forward 6 minutes) stayed pending on the base branch and came back as a tool.execution timeout error with this PR. tool-registry.test.ts (43/43) and the packages/core typecheck pass.
The blocker is scope: the 5-minute default applies to every tool except shell and question, and several of the others can legitimately run longer:
- Subagents: a foreground
subagentcall blocks on the child session's job, and itsonInterruptinterrupts and cancels the child. Any subagent task longer than 5 minutes is now killed. - MCP tools: they already have a per-server
timeout.executionsetting (default 12 hours inmcp/client.ts). This PR silently caps it at 5 minutes, and users can't raise it. - Code Mode: it has no default interpreter timeout (
timeoutMsis absent unless set), so this isn't a "backstop". Everyexecutescript is capped at 5 minutes in total, nested calls included. - Permission prompts: as the description notes, any edit/write/webfetch/etc. that waits longer than 5 minutes for approval now fails.
Please narrow it: opt subagent, MCP tools and Code Mode execute out (or respect their existing settings), and keep permission waits outside the deadline. Alternatively, put the deadline only on the leaf tools that can actually hang (read in #53433) instead of a global default.
Public API: adds an optional timeoutMs?: number | false to the plugin Tool.Options in @opencode/schema/tool. The new field itself is backwards compatible. The behaviour change is not: any existing plugin or MCP tool that runs longer than 5 minutes now fails unless it sets timeoutMs: false.
Related: #54220
There was a problem hiding this comment.
The new commit only merges dev. The PR's own changes are identical to the version I reviewed before, so none of the earlier points has been addressed, even though those threads are closed. After the merge, tool-registry.test.ts passes (44/44) and tsc -b for packages/core is clean.
Still open from the previous review:
- Subagents: a foreground
subagentrun still has no opt-out, so it is killed after 5 minutes. - MCP tools: the per-server
timeout.executionsetting (default 12 hours) is still capped at 5 minutes. - Code Mode: it has no default timeout of its own, so every
executescript is still capped at 5 minutes in total. - Permission prompts: waits for approval are still counted against the deadline.
- Tests: the test that only checks the constant's value can still be dropped.
Public API: unchanged since the last review. It adds an optional timeoutMs?: number | false to the plugin Tool.Options. The field itself is backwards compatible, but existing tools that run longer than 5 minutes now fail unless they set timeoutMs: false.
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Fixes #53433
Issue for this PR
Closes #53433
Type of change
What does this PR do?
A tool execution that never settles blocks the session step forever: the runner awaits tool fibers via FiberSet join with no deadline (\packages/core/src/session/runner/step.ts), and nothing bounded leaf execution.
This is the V2 implementation of the fix (follow-up to #54297, which targeted V1/\dev\ and does not apply here ΓÇö V2 has no \ ool/registry.ts). Instead of porting the V1 patch, the deadline is enforced at V2's actual shared boundary: \executeTool\ in \packages/core/src/tool.ts, the one path every tool execution crosses, including nested Code Mode calls. On expiry the leaf fiber is interrupted via \Effect.timeoutOption\ (default 5 min, \DEFAULT_TOOL_TIMEOUT_MS) and fails with an ordinary \Tool.Error, so it flows through the existing failure lifecycle and a late result can never overwrite it. Per-tool override via \Tool.Options.timeoutMs: \shell\ opts out (owns its timeout policy, including background waits) and \question\ opts out (waits on the user). MCP tools inherit the policy through the same boundary; Code Mode keeps its own interpreter timeout, with the shared default as backstop for the whole \execute\ call.
Related: #54220
How did you verify your code works?
Note: permission-approval waits inside non-\question\ tools are now also bounded by the 5 min default.
Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #54220
Public API: adds optional
timeoutMs?: number | falseto plugin Tool.Options; existing tools without it are now cut off after 5 minutes