Skip to content

Conversation

mshsheikh
Copy link
Contributor

Previously, when timeout_ms was explicitly set to 0, the condition if args.timeout_ms evaluated to False (since 0 is falsy), causing the timeout to be treated as None (no timeout). This violated the intended behavior where 0 should mean "time out immediately."

The fix changes the check to if args.timeout_ms is not None, ensuring that:

  • timeout_ms=0 correctly results in a 0.0 second timeout
  • timeout_ms=None (or unset) still results in no timeout
  • All other positive values work as before

This aligns the implementation with standard subprocess.run() timeout semantics and prevents unintended indefinite command execution.

Previously, when `timeout_ms` was explicitly set to `0`, the condition `if args.timeout_ms` evaluated to `False` (since `0` is falsy), causing the timeout to be treated as `None` (no timeout).
This violated the intended behavior where `0` should mean "time out immediately."

The fix changes the check to `if args.timeout_ms is not None`, ensuring that:
- `timeout_ms=0` correctly results in a `0.0` second timeout
- `timeout_ms=None` (or unset) still results in no timeout
- All other positive values work as before

This aligns the implementation with standard `subprocess.run()` timeout semantics and prevents unintended indefinite command execution.
@ihower
Copy link
Contributor

ihower commented Oct 15, 2025

@mshsheikh interesting issue, this timeout_ms value actually comes from the model’s response. Have you seen a real response where it returned 0, except when the user prompt (unreasonably) forces it to 0?

If the timeout is 0, the command wouldn’t really execute at all since it would immediately time out, so it’s useless in practice.

FYI, this example code was directly copied from the official OpenAI documentation:
https://platform.openai.com/docs/guides/tools-local-shell

@mshsheikh
Copy link
Contributor Author

@mshsheikh interesting issue, this timeout_ms value actually comes from the model’s response. Have you seen a real response where it returned 0, except when the user prompt (unreasonably) forces it to 0?

If the timeout is 0, the command wouldn’t really execute at all since it would immediately time out, so it’s useless in practice.

FYI, this example code was directly copied from the official OpenAI documentation: https://platform.openai.com/docs/guides/tools-local-shell

Hi @ihower, thanks for the thoughtful feedback.
You are correct that a value of timeout_ms equal to 0 will terminate immediately, and that is exactly how Python subprocess.run defines timeout where 0.0 seconds expires at once, see the official docs at:
https://docs.python.org/3/library/subprocess.html#subprocess.run

Even if this value is rare, treating 0 as None discards an explicit instruction from the model or the user and reduces an important safety control.
If the API accepts an integer including zero, the implementation should honor it for correctness and predictability. The Local shell guide shows a truthy check, but SDK code should be more robust than an example, especially for edge cases that influence reliability and safety, see the guide at:
https://platform.openai.com/docs/guides/tools-local-shell

This one line change keeps None as no timeout, honors 0 as immediate timeout, preserves positive values, and aligns with subprocess.run semantics. It also prevents unintended unbounded execution if a model ever emits 0 for this field.

I am happy to submit a small documentation update so the guide and the sample stay consistent with this behavior.

@ihower
Copy link
Contributor

ihower commented Oct 15, 2025

@mshsheikh I think the key point is not to match the behavior of subprocess.run, but to understand what the model means when it returns timeout_ms: 0.

In many systems that run commands, the meaning of 0 can vary depending on the execution mechanism, and the SDK may use different ways to execute tools (not only subprocess).

So what really matters is staying consistent with the model’s intent — maybe the model uses timeout_ms=0 to mean “no limit,” not “immediate stop.” I’m not sure, but maybe we should check with the codex-mini-latest team to confirm what timeout_ms=0 is supposed to mean.

@seratch seratch added the documentation Improvements or additions to documentation label Oct 16, 2025
@seratch
Copy link
Member

seratch commented Oct 16, 2025

The current code aligns with the example on the document page, so I'd like to hold off making this change: https://platform.openai.com/docs/guides/tools-local-shell

@seratch seratch closed this Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants