Skip to content

refactor(sdks): refactor run in session#641

Merged
Pangjiping merged 1 commit intoalibaba:mainfrom
ninan-nn:hotfix/fix_run_in_session
Apr 7, 2026
Merged

refactor(sdks): refactor run in session#641
Pangjiping merged 1 commit intoalibaba:mainfrom
ninan-nn:hotfix/fix_run_in_session

Conversation

@ninan-nn
Copy link
Copy Markdown
Collaborator

@ninan-nn ninan-nn commented Apr 3, 2026

Summary

Refactor sandbox SDK runInSession / run_in_session timeout parameters to match each language's existing public SDK timeout style instead of leaking execd wire-format milliseconds.

The main reason for this change is consistency and ambiguity reduction:

  • Other timeout-related SDK APIs already use language-native duration types or explicit second-based fields.
  • runInSession was an outlier that exposed raw millisecond semantics directly.
  • Keeping that shape would continue to create unit confusion and semantic drift across SDKs.

This change keeps the server wire format unchanged (timeout is still sent in milliseconds), but updates the handwritten SDK public interfaces to use clearer SDK-facing timeout shapes:

  • JavaScript: timeoutSeconds
  • Python: timedelta
  • Kotlin: Duration
  • C#: TimeoutSeconds

Also add an SDK guideline in sdks/AGENTS.md: public SDK duration parameters should use language-native duration types where available, or explicit second-based fields such as timeoutSeconds.

Testing

  • Unit tests
    • Kotlin: ./gradlew test
    • JavaScript: /Users/ninan/.nvm/versions/node/v22.21.1/bin/node --test tests/commands.run.test.mjs
    • Python: ./.venv/bin/python -m pytest tests/test_command_service_adapter_streaming.py tests/test_sync_command_service_adapter_streaming.py -q
  • Integration tests
  • e2e / manual verification

Notes:

  • C# targeted unit tests were blocked in this environment by vstest socket permission errors.
  • Python tests were run via the local virtualenv instead of uv because uv was unstable in this environment.

Breaking Changes

  • None
  • Yes

This PR intentionally introduces SDK breaking changes so old timeout call sites fail fast instead of silently keeping ambiguous millisecond semantics.

Migration examples:

  • JavaScript

    • Old: runInSession(id, cmd, { timeout: 5000 })
    • New: runInSession(id, cmd, { timeoutSeconds: 5 })
  • Python

    • Old: run_in_session(id, cmd, timeout=5000)
    • New: run_in_session(id, cmd, timeout=timedelta(seconds=5))
  • Kotlin

    • Old: runInSession(id, cmd, timeout = 5000)
    • New: runInSession(id, cmd, timeout = 5.seconds) / Duration.ofSeconds(5)
  • C#

    • Old: new RunInSessionOptions { Timeout = 5000 }
    • New: new RunInSessionOptions { TimeoutSeconds = 5 }

Checklist

  • Motivation clearly described
  • Docs updated
  • Tests updated
  • Security impact considered
  • Backward compatibility considered

@ninan-nn ninan-nn marked this pull request as ready for review April 3, 2026 09:11
@ninan-nn
Copy link
Copy Markdown
Collaborator Author

ninan-nn commented Apr 3, 2026

Added a warning in the latest release to reduce potential impact.
image

@hittyt
Copy link
Copy Markdown
Collaborator

hittyt commented Apr 4, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3c3ec9ef6

ℹ️ 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".

AlexandrePh added a commit to AlexandrePh/OpenSandbox that referenced this pull request Apr 4, 2026
Align Go SDK with PR alibaba#641's cross-SDK convention: public timeout
parameters use language-native duration types instead of leaking
raw millisecond wire-format values. Custom MarshalJSON preserves
the millisecond wire encoding.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@AlexandrePh
Copy link
Copy Markdown

Hey @ninan-nn — nice cleanup! We're contributing a Go SDK on our fork (feat/go-sdk branch) and just pushed a commit to align with this convention:

  • RunCommandRequest.Timeout and RunInSessionRequest.Timeout now use time.Duration (idiomatic Go)
  • Custom MarshalJSON converts to milliseconds for the wire format
  • Zero value is omitted via omitempty, matching server expectations

Commit: f3055bbrefactor(sdk): use time.Duration for command timeout fields

Usage looks like:

sb.RunInSession(ctx, sessID, opensandbox.RunInSessionRequest{
    Command: "sleep 10",
    Timeout: 5 * time.Second,
})

Happy to add the Go entry to sdks/AGENTS.md in our SDK PR when we open it upstream. Let us know if you'd prefer we do it here instead.

@ninan-nn ninan-nn force-pushed the hotfix/fix_run_in_session branch from a3c3ec9 to 19899d2 Compare April 7, 2026 01:48
Copy link
Copy Markdown
Collaborator

@Pangjiping Pangjiping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Pangjiping Pangjiping merged commit 1aa4ee8 into alibaba:main Apr 7, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants