Conversation
Co-authored-by: AlexandreYang <49917914+AlexandreYang@users.noreply.github.com>
|
Bits Dev status: ✅ Done Comment @DataDog to request changes |
|
I can only run on private repositories. |
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz and
thieman
as code owners
Resolve conflicts to keep both execution timeout support and ps/ProcPath support from main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
![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
Line 87 in 5fc1974
Apply timeout while reading CLI stdin
The new --timeout only constrains runCtx passed into execute, but this path still does a blocking io.ReadAll(stdin) first. If stdin never reaches EOF (for example a pipe left open), the process can hang indefinitely and never hit the timeout handling/exit code 124, which defeats the safety goal of bounding a whole CLI run.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…allowlist The execution timeout feature added in the previous commit uses these symbols in api.go but they were missing from the interp allowed symbols list, causing TestInterpAllowedSymbols and TestVerificationInterpCleanPass to fail in CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Branch: dd/add-execution-timeout vs main
Scope: 6 files — interp.MaxExecutionTime API option, --timeout CLI flag, documentation, tests.
Overall assessment: Safe to merge — no sandbox bypass, no new filesystem access, no unsafe imports. The timeout is applied via Go's standard context.WithTimeout and correctly threaded through the call chain. Context propagation, error-code conventions, and resource cleanup are all sound.
Finding Summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
cmd/rshell/main.go:131 |
context.Canceled (parent cancellation) falls through to exit code 1 — undocumented and untested |
| 2 | |
interp/timeout_test.go:62 |
WithinDuration assertion is valid but the why isn't obvious; lacks a comment explaining Go's deadline inheritance |
| 3 |  |
interp/timeout_test.go:35 |
TestMaxExecutionTimeStopsRun only exercises the execHandler path — the interpreter's own stop()/loop ctx.Err() checks are not covered |
| 4 | |
cmd/rshell/main_test.go:248 |
1ns timeout tests an already-expired context, not mid-execution cancellation |
Test Coverage Table
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
MaxExecutionTime negative value rejected |
— | TestMaxExecutionTimeRejectsNegative |
✅ Covered |
MaxExecutionTime stops execHandler-based command |
— | TestMaxExecutionTimeStopsRun |
✅ Covered |
MaxExecutionTime respects earlier parent deadline |
— | TestMaxExecutionTimeRespectsEarlierParentDeadline |
✅ Covered |
MaxExecutionTime fresh deadline per Run call |
— | TestMaxExecutionTimeIsRefreshedPerRun |
✅ Covered |
MaxExecutionTime(0) is a no-op |
— | — | |
Interpreter stop() loop check fires on timeout |
— | — | |
CLI --timeout stops execution |
— | TestTimeoutFlagTimesOutExecution |
✅ Covered (pre-expired ctx) |
CLI --timeout appears in help |
— | TestHelp |
✅ Covered |
context.Canceled from parent → exit code |
— | — |
Positive Observations
exitCodeTimeout = 124correctly follows the POSIXtimeout(1)convention.- Timeout applied per
Run()call (not at construction) is the right design — reused runners get a fresh deadline each time. defer cancel()placement is correct in bothmain.goandapi.go:Run— no context leak.errors.Is(not==) is used forcontext.DeadlineExceeded, correctly handling wrapped errors.timeout > 0guard adds zero overhead when no timeout is configured.- No new filesystem access paths, no unsafe imports, sandbox surface is unchanged.
…tdin reads - Reject negative --timeout values with an error (consistent with interp.MaxExecutionTime which also rejects negatives) - Replace io.ReadAll(stdin) with readAllContext so the timeout also bounds the blocking stdin read phase, not just script execution - Add TestTimeoutFlagRejectsNegative to cover the new validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e09b5771c3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1ns is too small for Windows' ~15ms timer resolution: the echo command completes before the context cancel goroutine fires, so the test got exit code 0 instead of 124. Replace with a blocking-stdin approach: pass an io.Pipe reader with no -c flag so readAllContext blocks until the 50ms context deadline fires. This is reliable on all platforms because 50ms >> Windows timer jitter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee693329a2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Pass context.Background() through main() into run() so callers (tests, future integrations) can inject a context - Use cmd.ExecuteContext(ctx) instead of cmd.Execute() - Add errors.Is(err, context.Canceled) branch → exit code 124 + "error: execution canceled" message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add comment to TestMaxExecutionTimeRespectsEarlierParentDeadline explaining why WithinDuration uses the parent deadline (Go's context.WithTimeout picks the earlier of two deadlines). - Add TestMaxExecutionTimeStopsForLoop to exercise the interpreter's own ctx.Err() check inside the for-loop body, not just the execHandler cooperative-cancellation path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — dd/add-execution-timeout
Overview
This PR adds bounded-execution support at two levels:
interp.MaxExecutionTime— a per-Run-call timeout set viaRunnerOption; the context is derived fresh each call so reusing a runner does not inherit a stale deadline.--timeoutCLI flag — wraps the cobraRunEcontext; also applied to stdin reads viareadAllContextso a hung pipe cannot bypass the time limit.
No security issues found. The sandbox perimeter is unchanged — the new code does not open any new filesystem or execution paths, and the timeout context correctly threads through both the interpreter loop (stop(ctx)) and the exec handler. The implementation is clean and well-tested.
Overall assessment: ✅ safe to merge — P3 findings only.
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
cmd/rshell/main.go:97 |
os.ReadFile in multi-file loop not gated by context |
| 2 | |
cmd/rshell/main_test.go:254 |
CLI timeout test covers only the stdin-blocking path, not --timeout -c execution path |
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
MaxExecutionTime negative rejected |
— | TestMaxExecutionTimeRejectsNegative |
Covered |
MaxExecutionTime fires mid-run (execHandler) |
— | TestMaxExecutionTimeStopsRun |
Covered |
MaxExecutionTime fires mid for-loop |
— | TestMaxExecutionTimeStopsForLoop |
Covered |
| Parent deadline takes precedence | — | TestMaxExecutionTimeRespectsEarlierParentDeadline |
Covered |
Deadline refreshed on each Run call |
— | TestMaxExecutionTimeIsRefreshedPerRun |
Covered |
CLI --timeout fires (stdin path) |
— | TestTimeoutFlagTimesOutExecution |
Covered |
CLI --timeout negative rejected |
— | TestTimeoutFlagRejectsNegative |
Covered |
| CLI parent context canceled → 124 | — | TestCanceledContextExitsWithTimeoutCode |
Covered |
CLI --timeout -c fires mid-execution |
— | — | Gap (P3) |
Positive Observations
context.WithTimeoutwraps the incoming context rather than replacing it — a caller with a tighter deadline keeps their deadline (tested byTestMaxExecutionTimeRespectsEarlierParentDeadline).readAllContextuses a buffered channel of size 1 so the goroutine never blocks on send when the context fires first — the goroutine cleans up immediately after the reader returns.- Negative duration validation is consistent between the API (
MaxExecutionTime) and the CLI (--timeout), giving callers a fast-fail with a clear error. exitCodeTimeout = 124matches the POSIXtimeout(1)convention.
| return fmt.Errorf("reading %s: %w", file, err) | ||
| } | ||
| if err := execute(cmd.Context(), string(data), file, execOpts, bytes.NewReader(stdinData), stdout, stderr); err != nil { | ||
| if err := execute(runCtx, string(data), file, execOpts, bytes.NewReader(stdinData), stdout, stderr); err != nil { |
There was a problem hiding this comment.
os.ReadFile in file-arg loop not gated by context between iterations
When multiple script files are passed and the timeout fires between file N and N+1, the next file is still read from disk before the already-expired context is checked (inside execute() → stmt() → stop()). No incorrect output is produced, but unnecessary I/O occurs.
Consider an explicit check before each read:
| if err := execute(runCtx, string(data), file, execOpts, bytes.NewReader(stdinData), stdout, stderr); err != nil { | |
| if err := runCtx.Err(); err != nil { | |
| return err | |
| } | |
| data, err := os.ReadFile(file) |
| assert.Contains(t, stderr, "unknown flag: --command") | ||
| } | ||
|
|
||
| func TestTimeoutFlagTimesOutExecution(t *testing.T) { |
There was a problem hiding this comment.
readAllContext path, not the -c interpreter path
TestTimeoutFlagTimesOutExecution validates that a blocking stdin read is interrupted by the timeout — good cross-platform approach. But the separate code path (--timeout with -c 'script') skips readAllContext entirely and relies solely on the interpreter's stop() check. This is covered at the interp package level by TestMaxExecutionTimeStopsForLoop, but there is no CLI-level end-to-end test for it.
This is a known limitation: the CLI cannot inject a slow execHandler, and while loops are unsupported, so the only reliable slow construct would require external commands or a forked test helper. Worth a comment in the test file noting this gap.
There was a problem hiding this comment.
💡 Codex Review
Line 97 in f5a45fe
Enforce --timeout during script file reads
The new timeout only wraps execute(...); each script file is loaded first via os.ReadFile, which is not context-aware. If a positional script argument is a blocking source (for example, a FIFO with no writer, or a slow/hung mount), rshell --timeout ... file can block indefinitely instead of exiting with code 124, so the advertised whole-run timeout is not actually enforced for this path.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| runCtx := cmd.Context() | ||
| if timeout > 0 { |
There was a problem hiding this comment.
I'd suggest combining these
| if timeout > 0 { | |
| if timeout > 0 { | |
| var cancel context.CancelFunc | |
| runCtx, cancel = context.WithTimeout(runCtx, timeout) | |
| defer cancel() | |
| } else { | |
| return fmt.Errorf("--timeout must be >= 0") | |
| } |
There was a problem hiding this comment.
I think this suggestion above doesn't account for timeout == 0 case:
⏺ The reviewer's suggestion has a correctness issue: it removes the timeout == 0 case (no timeout set = run indefinitely) by making timeout > 0 required for the function to proceed. With the suggested code, passing
--timeout 0 would hit the else branch and return an error, which contradicts the intended behavior where 0 means "no timeout."
The current code handles three cases correctly:
- < 0 → error
- == 0 → no timeout (run indefinitely)
- > 0 → apply timeout
The suggestion collapses it to two cases, breaking the middle one. Beyond that, two separate if blocks with early returns is standard Go style — putting the error in an else is less idiomatic.
I'd decline the suggestion on both correctness and style grounds.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c73d6f7fe4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if timeout > 0 { | ||
| var cancel context.CancelFunc | ||
| runCtx, cancel = context.WithTimeout(runCtx, timeout) | ||
| defer cancel() |
There was a problem hiding this comment.
The timeout implementation only derives a child context with context.WithTimeout, but it never interrupts a command that is blocked in a stdin Read, so the advertised whole-run deadline is bypassed for common pipelines (for example, cat waits for EOF and ignores ctx.Done() while blocked). Fresh evidence: running (sleep 2) | rshell --allow-all-commands --timeout 100ms -c 'cat' against commit dc4dfb8 still exits 0 after about 3.3 seconds with no timeout message. This means both CLI --timeout and interp.MaxExecutionTime do not reliably bound runtime unless stdin readers are made cancellation-aware (or closed on timeout).
Useful? React with 👍 / 👎.
|
|
||
| for _, file := range args { | ||
| data, err := os.ReadFile(file) | ||
| f, err := os.Open(file) |
There was a problem hiding this comment.
These direct os.Open calls and the io.ReadAll on potentially infinite sources are both concerning, we need to take a look here and make sure there aren't ways to get around our invariants in this part of the flow
There was a problem hiding this comment.
note that this is cmd/rshell/main.go building ./rshell CLI , only for dev/testing
4 participants
What does this PR do?
Add whole-run execution timeout support to rshell in both the public Go API and the CLI.
Motivation
AI agents need a reliable way to bound shell execution so a script cannot run indefinitely.
This change builds on rshell's existing context-aware execution model by exposing a first-class timeout configuration for callers and a
--timeoutflag for CLI usage.Testing
make fmtgo test ./interp/... ./cmd/rshellgo build -o /dev/null ./cmd/rshell && go vet ./cmd/rshellgo build -o /dev/null ./interp && go vet ./interpChecklist
PR by Bits - View session in Datadog
Comment @DataDog to request changes