feat(bash): add yield_time_ms and explicit accesses#1140
Conversation
🦋 Changeset detectedLatest commit: 8b06072 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a56e2ed10f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const yieldMs = args.yield_time_ms ?? 10_000; | ||
| const yieldDeadline = sleep(yieldMs); | ||
| const completionOrDetach = this.backgroundManager.waitForForegroundRelease(taskId); | ||
| const release = await Promise.race([completionOrDetach, yieldDeadline.then(() => 'yielded' as const)]); |
There was a problem hiding this comment.
In profiles where Bash is available but TaskOutput/TaskStop are not active, such as the coder and explore subagent profiles, allowBackground is false, yet every foreground command that runs longer than the new default 10s yield now returns a task_id and tells the model to poll or stop it. Because those tools are not in the subagent's tool list and the task is not detached for automatic notification, the caller loses the command result for ordinary long commands like tests or builds. Gate this yield path on allowBackground, or use the existing detach/background result path only when task tools are actually available.
Useful? React with 👍 / 👎.
| sandbox_permissions: z | ||
| .enum(['use_default', 'with_additional_permissions', 'require_escalated']) | ||
| .optional() | ||
| .describe( | ||
| 'Per-command sandbox override. Defaults to `use_default`; use `with_additional_permissions` with `additional_permissions`, or `require_escalated` for unsandboxed execution.', |
There was a problem hiding this comment.
These new arguments are accepted by the Bash schema, but the execution path never reads them; repo-wide search for sandbox_permissions only finds this schema and the prompt text. A call with sandbox_permissions: "require_escalated" therefore follows the same approval and spawn path as any other Bash command: justification is not required, no extra approval is forced, and additional_permissions is never applied. Please either connect these fields to the permission/execution layer or keep them out of the schema and prompt until they are supported.
Useful? React with 👍 / 👎.
starSumi
changed the title
starSumi
force-pushed
the
feat/tool-enhancements
branch
from
1cab83f to
ae0f660
Compare
|
Follow-up pushed in
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae0f660a14
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const release = this.allowBackground | ||
| ? await Promise.race([ | ||
| completionOrDetach, | ||
| sleep(args.yield_time_ms ?? 10_000).then(() => 'yielded' as const), |
There was a problem hiding this comment.
When this BashTool is used by ToolManager.runShellCommand for the TUI ! path, that caller resolves Bash with only { command, timeout: SHELL_FOREGROUND_TIMEOUT_S } and only treats results starting with task_id: as backgrounded. With this new 10s default, any manual shell command that runs longer than 10 seconds returns through the yielded branch, but its handle is embedded in result.output as a <system>...task_id... message while runShellCommand reports only the streamed stdout/stderr and drops the handle, leaving the process running without a visible way for the user to poll or cancel it. Please make yielding opt-in for this path or teach the shell-command caller to handle yielded results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e275e1: manual ! shell commands now call Bash through an internal no-auto-yield execution path, so they wait for completion, timeout, cancellation, or explicit ctrl+b detach instead of returning a hidden yielded task_id. Added a regression test in packages/agent-core/test/agent/context.test.ts.
starSumi
force-pushed
the
feat/tool-enhancements
branch
from
ae0f660 to
7e275e1
Compare
|
Follow-up pushed in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e275e1aa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (release === 'yielded') { | ||
| // Command is still running after yield_time_ms. Return partial output | ||
| // with wall_time_seconds so the caller knows how long we waited. | ||
| collectForegroundOutput = false; |
There was a problem hiding this comment.
When a foreground Bash call reaches yield_time_ms, this branch returns a task_id but does not detach the task or start output persistence, and then collectForegroundOutput is disabled. For a yielded command that emits more than TaskOutput's 32 KiB preview but less than the manager's 1 MiB spill threshold after the yield (for example, a long test ending with a 100 KiB log), TaskOutput has no output_path and only exposes the tail, so the head of the output is inaccessible to the model even though the result tells it to poll TaskOutput. Start persisting the task log when yielding.
Useful? React with 👍 / 👎.
starSumi
force-pushed
the
feat/tool-enhancements
branch
from
7e275e1 to
22aca6b
Compare
|
Follow-up pushed in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22aca6b425
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (release === 'yielded') { | ||
| // Command is still running after yield_time_ms. Return partial output | ||
| // with wall_time_seconds so the caller knows how long we waited. | ||
| this.backgroundManager.persistOutput(taskId); |
There was a problem hiding this comment.
When a foreground Bash call auto-yields, this only starts output.log persistence; unlike the detach path, it does not write the task JSON metadata before returning the task_id. In a persistent session, if the CLI/session restarts after this return but before the process reaches finalizeTask, BackgroundManager.loadFromDisk() has no task record to reconcile, so the returned task disappears and TaskOutput cannot recover or mark it lost. Please persist the live task metadata as part of the yield path before exposing the task id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0aad1f1: persistOutput(task_id) now also persists the live task metadata before exposing the yielded task_id, and the regression test checks that tasks/<task_id>.json exists before the process finishes.
starSumi
force-pushed
the
feat/tool-enhancements
branch
from
22aca6b to
0aad1f1
Compare
|
Follow-up pushed in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aad1f16ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const entry = this.tasks.get(taskId); | ||
| if (entry === undefined) return; | ||
| this.startOutputPersist(entry); | ||
| void this.persistLive(entry); |
There was a problem hiding this comment.
When a foreground Bash call yields, this fire-and-forget write can still be pending after the tool result has returned the task_id. If the CLI exits or restarts immediately after that yield, tasks/<task_id>.json may never reach disk, so the returned id is not recoverable via loadFromDisk()/TaskOutput even though the command was handed back as a background-like task. Make this persistence path awaitable and await it before returning the yielded result.
Useful? React with 👍 / 👎.
starSumi
force-pushed
the
feat/tool-enhancements
branch
from
0aad1f1 to
8b06072
Compare
|
Follow-up pushed in
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b06072b1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return { | ||
| isError: false, |
There was a problem hiding this comment.
When this return path resolves, the shell process is still running, but the loop releases the call's ToolAccesses.all() lock as soon as the tool result promise settles (tool-call.ts schedules access only around that promise). In a provider batch such as Bash("sleep 1; echo x > a", yield_time_ms=250) followed by a Read/Edit of a, the later tool can start while the yielded command is still mutating the workspace, so it can observe stale state or race with the shell despite Bash declaring exclusive access. Please stop/skip the remaining batch when yielding, or otherwise keep the exclusive scheduling boundary until the process is actually detached/completed.
Useful? React with 👍 / 👎.
1 participant
Related Issue
No related issue. This PR addresses the Bash tool behavior gaps described below.
Problem
Foreground Bash commands can yield partial output, but that only works when the agent has background task tools available. In profiles that enable Bash without
TaskOutputorTaskStop, returning atask_idwould strand the running command because the model cannot poll or stop it.The branch also exposed sandbox override parameters before the execution layer could enforce them, which made the tool schema and prompt promise behavior that was not wired.
What changed
yield_time_msfor normal Bash tool profiles, but only return a yieldedtask_idwhen background task tools are available.!shell commands from auto-yielding after the Bash yield window; they continue waiting for completion, timeout, cancellation, or explicit detach.TaskOutputcan point at a complete log file instead of only exposing the tail preview.task_id, so resumed sessions can reconcile the running task even if the CLI exits immediately after yield.Validation
pnpm exec vitest run packages/agent-core/test/tools/bash.test.ts --reporter=verbosepnpm exec vitest run packages/agent-core/test/tools/bash.test.ts packages/agent-core/test/tools/grep.test.ts packages/agent-core/test/loop/tool-scheduler.test.ts --reporter=dotpnpm exec vitest run packages/agent-core/test/agent/context.test.ts packages/agent-core/test/tools/bash.test.ts packages/agent-core/test/tools/grep.test.ts packages/agent-core/test/loop/tool-scheduler.test.ts --reporter=dotpnpm exec vitest run packages/agent-core/test/tools/bash.test.ts packages/agent-core/test/agent/background/foreground-persistence.test.ts --reporter=dotpnpm exec vitest run packages/agent-core/test/agent/context.test.ts packages/agent-core/test/tools/bash.test.ts packages/agent-core/test/tools/grep.test.ts packages/agent-core/test/loop/tool-scheduler.test.ts packages/agent-core/test/agent/background/foreground-persistence.test.ts --reporter=dotpnpm --filter @moonshot-ai/agent-core typecheckpnpm exec oxlint packages/agent-core/src/tools/builtin/shell/bash.ts packages/agent-core/src/agent/tool/index.ts packages/agent-core/src/agent/background/index.ts packages/agent-core/test/agent/context.test.ts packages/agent-core/test/tools/bash.test.tspnpm changeset status --since da63403207691856b9fe762953413b7205d5c903git diff --checkChecklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.