feat(sandbox): add Aliyun AgentRun sandbox client#3732
Conversation
![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: a8509928d2
ℹ️ 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 self.state.env: | ||
| await self._inject_user_env(self.state.env) |
There was a problem hiding this comment.
When callers provide environment values through Manifest(environment=...), the Aliyun backend never resolves or injects them because this branch only writes state.env from client options. Other sandbox clients merge manifest.environment.resolve() into the runtime environment, so Aliyun agents and shell commands silently miss manifest-provided env vars/secrets. Please merge the resolved manifest environment with state.env before calling _inject_user_env.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 23c95aec. _prepare_backend_workspace now resolves self.state.manifest.environment.resolve() and merges it with the option-provided state.env (manifest wins on key collisions, None values dropped) before _inject_user_env, matching the other backends. Added test test_prepare_backend_workspace_merges_manifest_env.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
a850992 to
23c95ae
Compare
|
@codex review Addressed both P2 findings in
New tests added and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23c95aecda
ℹ️ 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".
| options: AliyunSandboxClientOptions, | ||
| ) -> SandboxSession: | ||
| resolved_manifest = _resolve_manifest_root(manifest) | ||
| region = options.region or self._region or DEFAULT_ALIYUN_REGION |
There was a problem hiding this comment.
When callers configure AliyunSandboxClient(region="cn-shanghai") and pass default AliyunSandboxClientOptions(), this expression always selects the options default (cn-hangzhou) before the client value, unlike the credential fallback logic below. That creates the sandbox and serialized state in the wrong region unless every caller redundantly sets options.region; treat the options default as unset or make the field optional so the client-level default can take effect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab23951. AliyunSandboxClientOptions.region is now str | None = None, and create resolves it via _resolve_credential(options.region, self._region) or DEFAULT_ALIYUN_REGION — same is not None precedence as the credential fallback — so a client-level region now takes effect when options leaves it unset. Added test_client_create_honors_client_region_default and test_client_create_options_region_overrides_client.
| env_lines = "\n".join( | ||
| f"export {k}={shlex.quote(v)}" for k, v in env.items() if v is not None | ||
| ) |
There was a problem hiding this comment.
For manifests or options.env whose keys are not fully trusted, the values are quoted but the variable names are emitted raw into a file that is later sourced by every sh -lc command. A key such as X; curl ... # turns the generated export line into arbitrary shell commands inside the sandbox, potentially alongside manifest-provided secrets; reject non-shell-identifier names or write the environment without sourcing unescaped keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab23951. _inject_user_env now validates each key against ^[A-Za-z_][A-Za-z0-9_]*$ before writing the export line; non-identifier names (e.g. X; curl ... #) are skipped with a warning rather than emitted raw into the sourced file. Added test_inject_user_env_skips_unsafe_names and test_inject_user_env_all_names_unsafe_is_noop. (This one was exposed by the manifest-env fix above — good catch.)
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
23c95ae to
dab2395
Compare
|
@codex review Addressed both P2 findings from the last pass in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab2395153
ℹ️ 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".
| f"cat > {_AGENTS_ENV_FILE} <<'OPENAI_AGENTS_ENV_EOF'\n" | ||
| f"{env_lines}\n" | ||
| "OPENAI_AGENTS_ENV_EOF\n" |
There was a problem hiding this comment.
When manifest/options env contains secrets and the session uses the default root /home/user, this writes $HOME/.openai_agents_env inside the persisted workspace. persist_workspace() tars . from that root, so stop()/snapshotting includes the export file and leaks secret values into snapshot storage and resumed workspaces. Put this file outside the persisted root or register it as a persistence skip path before snapshotting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dc63eaa5. _inject_user_env now registers the secret-bearing .openai_agents_env file (which lives at $HOME, i.e. the default workspace root) as a persist-workspace skip path via register_persist_workspace_skip_path, so persist_workspace() adds --exclude=./.openai_agents_env and the secrets never land in snapshots or resumed workspaces. .profile only gets a non-secret source line, so it may persist. Added test_inject_user_env_excluded_from_snapshot.
| self.state.sandbox_id, | ||
| effective_cwd, | ||
| timeout_int, | ||
| shell_cmd[:500], |
There was a problem hiding this comment.
When _prepare_backend_workspace() injects manifest/options env, _inject_user_env() passes a shell script containing export KEY=<value> through _run_command(). Logging shell_cmd at INFO records those env values in normal application logs, including secrets from Manifest.environment; redact the command for env injection or avoid logging command payloads that can contain secrets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dc63eaa5. _run_command gained a redact_command flag; _inject_user_env calls it with redact_command=True so the INFO log records cmd='<redacted>' instead of the export KEY=<secret> payload. The command still executes normally against the sandbox. Added test_inject_user_env_does_not_log_secret.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
dab2395 to
dc63eaa
Compare
|
@codex review Addressed both P1 secret-leak findings in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc63eaa51d
ℹ️ 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".
| # snapshots so secrets never land in persisted/resumed workspaces. (`.profile` | ||
| # only gets a `source` line + marker, no secrets, so it may persist.) | ||
| try: | ||
| self.register_persist_workspace_skip_path(_AGENTS_ENV_REL_PATH) |
There was a problem hiding this comment.
When callers override manifest.root to a parent of $HOME (for example /home or /), the env file written at $HOME/.openai_agents_env is still inside the persisted workspace, but this registers only ./.openai_agents_env as the tar exclusion. persist_workspace() tars from manifest.root, so snapshots in that configuration will include user/.openai_agents_env (or home/user/.openai_agents_env) with the exported secret values; compute the skip path relative to the actual workspace root or place the secret file outside it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e64cf03. The skip path is now computed relative to the actual manifest.root: _inject_user_env takes the env file's absolute path ($HOME/.openai_agents_env, where $HOME is AgentRun's /home/user) and registers env_file_abs.relative_to(manifest.root) — so root /home excludes user/.openai_agents_env, root / excludes home/user/.openai_agents_env, and a root that doesn't contain $HOME registers nothing (no leak, no error). Added test_inject_user_env_skip_path_relative_to_ancestor_root and test_inject_user_env_skip_noop_when_env_outside_root.
| sandbox, | ||
| normalized[0], | ||
| normalized[1:], | ||
| cwd=self.state.manifest.root, |
There was a problem hiding this comment.
For sessions created with AliyunSandboxClientOptions(default_cwd=...), user exec calls never use it because _exec_internal always passes manifest.root into _run_command; the fallback to state.default_cwd in _run_command only runs when cwd is None. As a result, exec('pwd') and tool shell commands run in the workspace root instead of the requested default cwd; omit cwd here or pass the configured value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e64cf03. _exec_internal now passes cwd=self.state.default_cwd or self.state.manifest.root, so user execs honor a configured default_cwd and fall back to the workspace root only when it's unset. Added test_exec_uses_default_cwd and test_exec_defaults_to_manifest_root_without_default_cwd.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
dc63eaa to
7e64cf0
Compare
|
@codex review Addressed both findings from the last pass in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e64cf0309
ℹ️ 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".
| sandbox, | ||
| normalized[0], | ||
| normalized[1:], | ||
| cwd=self.state.default_cwd or self.state.manifest.root, |
There was a problem hiding this comment.
When default_cwd is set to a subdirectory, this cwd is now applied to every exec, including SDK-internal calls. For example, persist_workspace() runs tar cf ... . through self.exec(..., shell=False), so with default_cwd=/home/user/project the snapshot archives only that subdirectory instead of the manifest root, dropping sibling workspace files on resume; the same issue can make runtime-helper installation fail before a manifest-created default cwd exists. Internal commands should either force manifest.root// or the default cwd should only apply to user execs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11b8cbae. The prior pass applied default_cwd inside _exec_internal, but every internal operation (runtime-helper install, workspace setup, persist_workspace/hydrate_workspace, filesystem ops) funnels through that same path — so it broke far more than persist. Since the base session offers no user-vs-internal exec distinction, and default_cwd was already vestigial (never applied to execs originally, unused in examples, no other backend has it), I removed the default_cwd option entirely: all execs now run from the workspace root. This resolves this finding and the previous 'honor default_cwd' one together without a half-working option. Updated the exec test to test_exec_runs_in_manifest_root.
| # CodeInterpreterSandbox context manager polls health for this reason. | ||
| # We bypass that context manager, so wait explicitly before the first | ||
| # exec (e.g. the workspace mkdir) can hit a not-yet-ready sandbox. | ||
| await self._wait_until_healthy(sandbox) |
There was a problem hiding this comment.
If the health wait below times out or raises, the remote sandbox has already been created and stored on self._sandbox, but AliyunSandboxClient.create() propagates the exception without returning a session. Callers therefore have no handle to delete, so each boot/health timeout leaks an AgentRun sandbox until the idle timeout; wrap the health wait with best-effort deletion or defer publishing the handle until after it is healthy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 11b8cbae. _ensure_sandbox now wraps the health wait: if _wait_until_healthy times out or raises, it calls _stop_attached_sandbox() (best-effort delete_sandbox_async) before re-raising, so a sandbox that never becomes healthy is deleted instead of leaking until its idle timeout. Extended test_client_create_waits_for_health_timeout to assert the sandbox is deleted.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
7e64cf0 to
11b8cba
Compare
|
@codex review Addressed both findings from the last pass in
New/updated tests; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11b8cbae39
ℹ️ 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".
| sandbox = CodeInterpreterSandbox.model_validate(base_sandbox.model_dump(by_alias=False)) | ||
| sandbox._config = config | ||
| logger.info("AliyunSandboxClient reattached sandbox: sandbox_id=%s", sandbox_id) | ||
| return sandbox, client |
There was a problem hiding this comment.
When get_sandbox_async() returns a sandbox record for an expired/stopped instance rather than raising 404, resume marks it as preserved and _ensure_sandbox() will keep reusing this handle; the next start() then fails during the workspace probe or mkdir instead of provisioning a fresh sandbox and restoring the snapshot. Please health-check the reattached CodeInterpreterSandbox here and return (None, None) or raise on not-ready states so the existing fresh-sandbox fallback can run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc22ab9b. _reattach_sandbox now health-checks the reattached sandbox: after get_sandbox_async, it calls check_health_async and returns (None, None) if the instance isn't ready (or the probe fails), so resume falls through to provisioning a fresh sandbox + snapshot restore instead of reusing an expired/stopped handle. Added test_client_resume_reattach_unhealthy_falls_back.
| ) | ||
| tar_command = ("tar", "xf", archive_path.as_posix(), "-C", root.as_posix()) | ||
| try: | ||
| self._validate_tar_bytes(raw_bytes) |
There was a problem hiding this comment.
For snapshot/archive data containing a symlink whose target is outside the workspace, _validate_tar_bytes() currently uses the default allow_external_symlink_targets=True and then hands the archive to remote tar xf, so the restored workspace can contain links to paths outside manifest.root that user shell commands can follow. Other hosted backends disable external symlink targets before remote extraction; please pass allow_external_symlink_targets=False here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc22ab9b. _validate_tar_bytes now passes allow_external_symlink_targets=False to validate_tarfile, matching the other hosted backends, so an archive with a symlink pointing outside manifest.root is rejected before the remote tar xf. Added test_validate_tar_bytes_external_symlink_rejected (verified it passes with the old default True and is rejected with False).
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
11b8cba to
cc22ab9
Compare
|
@codex review Addressed both findings from the last pass in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc22ab9b95
ℹ️ 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".
| f"cat > {_AGENTS_ENV_FILE} <<'OPENAI_AGENTS_ENV_EOF'\n" | ||
| f"{env_lines}\n" | ||
| "OPENAI_AGENTS_ENV_EOF\n" |
There was a problem hiding this comment.
When an env value contains a newline with OPENAI_AGENTS_ENV_EOF on its own line, this fixed here-doc is terminated early and the remaining bytes are executed by the setup sh -c during start(). Since _prepare_backend_workspace merges values from AliyunSandboxClientOptions.env and manifest.environment, any caller that passes user-controlled data as an env value can turn that data into arbitrary sandbox commands, potentially before other secret env lines are protected; write the env file via the file API/base64 or otherwise avoid a delimiter that can appear verbatim in the payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5eb654e5. _inject_user_env no longer uses a here-doc: the env file is now transported as printf %s '<base64>' | base64 -d > $FILE. base64 output contains no shell-significant characters (and no newlines), so an env value can't smuggle a here-doc terminator or any shell command into the setup script — the whole payload stays opaque until it's decoded into the (chmod 600) file. Keys are still validated, values are still shlex.quoted for when the file is later sourced, and the command remains redact_command=True. Added test_inject_user_env_value_with_heredoc_delimiter_is_safe (asserts the injected OPENAI_AGENTS_ENV_EOF / touch /pwned never appear as literal shell, but are preserved verbatim in the decoded file).
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
cc22ab9 to
5eb654e
Compare
|
@codex review Addressed the here-doc injection finding in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb654e5b4
ℹ️ 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 timeout_s is None: | ||
| timeout_int = ALIYUN_PROCESS_CMD_HARD_CAP_S | ||
| else: | ||
| timeout_int = max(1, min(int(timeout_s), ALIYUN_PROCESS_CMD_HARD_CAP_S)) |
There was a problem hiding this comment.
When callers pass exec(..., timeout=60) or any value above the 30s /processes/cmd cap, this clamps the provider request to 30 while the outer wait_for still waits for the original timeout. A command that is valid for the caller's timeout but runs longer than 30s will be killed/reported as timed out early; route those calls through the long-running execution path or reject them explicitly instead of silently shortening the timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 76c1d207. Confirmed against the SDK: ProcessOperations.cmd_async(..., timeout=30) just forwards the timeout (default 30, no enforced ceiling) — the 30s cap was a deployment-specific assumption. _run_command no longer clamps: it forwards the caller's timeout as-is (defaulting to 30 when unset), matching the outer wait_for, so a command the caller allowed more time for isn't killed early. If a specific gateway enforces a shorter limit it surfaces as a provider error rather than a silent cap. Renamed the misleading ALIYUN_PROCESS_CMD_HARD_CAP_S → DEFAULT_ALIYUN_EXEC_TIMEOUT_S; updated tests (test_run_command_forwards_caller_timeout, test_run_command_defaults_timeout_when_unset).
| export OPENAI_API_KEY=... | ||
| export ALIBABA_CLOUD_ACCESS_KEY_ID=... | ||
| export ALIBABA_CLOUD_ACCESS_KEY_SECRET=... |
There was a problem hiding this comment.
Following the documented setup with only ALIBABA_CLOUD_ACCESS_KEY_ID and ALIBABA_CLOUD_ACCESS_KEY_SECRET passes the runner's credential check, but the AgentRun SDK still needs an account id (or an explicit data endpoint) to construct the data-plane endpoint before sandbox creation. Users who copy these required env vars will fail later with a configuration error; include AGENTRUN_ACCOUNT_ID/equivalent here and enforce it in the runner unless --api-key/data-endpoint auth is being used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 76c1d207. Confirmed the SDK's Config.get_data_endpoint() builds https://{account_id}.agentrun-data.{region}.aliyuncs.com and get_account_id() raises account id is not set when it's missing and no data endpoint is set. Updated the README setup to export AGENTRUN_ACCOUNT_ID (or FC_ACCOUNT_ID / AGENTRUN_DATA_ENDPOINT, or --account-id), and _require_aliyun_credentials in the runner now enforces an account id (or data endpoint) up front with a clear message instead of letting users fail later at sandbox creation.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
5eb654e to
76c1d20
Compare
|
@codex review Addressed both findings from the last pass in
New/updated tests; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76c1d20715
ℹ️ 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".
| config = _build_config( | ||
| access_key_id=self._access_key_id, | ||
| access_key_secret=self._access_key_secret, | ||
| account_id=self._account_id, | ||
| api_key=self._api_key, |
There was a problem hiding this comment.
create() accepts access_key_id, access_key_secret, account_id, and api_key on AliyunSandboxClientOptions, but those values are only used to build the initial Config and are omitted from serialized state. When a caller creates a session with option-only credentials and then calls resume() on the same client/state, this block rebuilds Config from the empty client fields, so reattach/fresh provisioning loses the required account/keys and fails. Cache the resolved option credentials on the client or require them at client construction instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in edadaa86. create() now caches the resolved option-over-client credentials back onto the client (_access_key_id etc.), so a create()-then-resume() flow on the same client keeps working when credentials were supplied only via AliyunSandboxClientOptions. Serialized state still never carries secrets — resume rebuilds Config from the client fields, which are now populated. Added test_client_resume_uses_option_credentials_from_create.
| if timeout_s is None: | ||
| timeout_int = DEFAULT_ALIYUN_EXEC_TIMEOUT_S | ||
| else: | ||
| timeout_int = max(1, int(timeout_s)) |
There was a problem hiding this comment.
exec(timeout=...) accepts floats and the outer asyncio.wait_for uses the original value, but this floors the provider timeout before sending it to AgentRun. For a caller budget like 1.9, the remote command receives 1 second and can be killed before the outer timeout, which silently shortens the requested budget. Forward the float unchanged or round up if the SDK requires an int.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in edadaa86. _run_command now uses math.ceil(timeout_s) instead of int(...), so a fractional budget like 1.9 forwards as 2 (not 1) and the provider can't kill the command before the outer wait_for. Added test_run_command_rounds_up_fractional_timeout.
| has_keys = bool(access_key_id and access_key_secret) or bool( | ||
| env.get("ALIBABA_CLOUD_ACCESS_KEY_ID") and env.get("ALIBABA_CLOUD_ACCESS_KEY_SECRET") | ||
| ) | ||
| if not (has_keys or (account_id and api_key)): |
There was a problem hiding this comment.
The README added for this runner says agentrun-sdk can resolve credentials from Alibaba config files or RAM roles, but this preflight only accepts CLI access keys, ALIBABA_CLOUD_ACCESS_KEY_*, or account_id+api_key. In environments that rely on those supported SDK credential providers with AGENTRUN_ACCOUNT_ID set, the example exits before the SDK can resolve them; limit the preflight to the account-id requirement or let Config validate credentials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in edadaa86. The runner preflight now only enforces the account-id requirement (the one hard prerequisite for the data-plane endpoint) and leaves the access credentials to the SDK, which resolves them from CLI flags, env vars, an Alibaba Cloud config file, or a RAM role — matching what the README describes. It no longer exits early when a supported SDK credential provider is in use with AGENTRUN_ACCOUNT_ID set.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
76c1d20 to
edadaa8
Compare
|
@codex review Addressed all three findings from the last pass in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edadaa86ff
ℹ️ 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".
| self._access_key_id = self._resolve_credential(options.access_key_id, self._access_key_id) | ||
| self._access_key_secret = self._resolve_credential( | ||
| options.access_key_secret, self._access_key_secret | ||
| ) | ||
| self._account_id = self._resolve_credential(options.account_id, self._account_id) |
There was a problem hiding this comment.
When the same AliyunSandboxClient is reused for more than one tenant/account and credentials are supplied through per-call AliyunSandboxClientOptions, each create() mutates the client-wide credentials. A later resume() of an earlier session rebuilds its Config from whatever credentials the most recent create stored, so it can reattach or provision the resumed workspace under the wrong account (and can also mix an old AK/SK with a newer account id). Keep option credentials scoped to the session instead of overwriting the shared client state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c9329ad — and this reverts the client-caching from my previous reply, which you correctly flagged as unsafe. create() no longer writes resolved credentials back onto the client; the option-over-client resolution is used only to build THIS session's Config (scoped to the session). So a client reused across tenants via per-call options no longer has its shared credentials mutated, and a later resume() of an earlier session can't run under the wrong account. Trade-off: resume() relies on client-level credentials (or the SDK's env/credential providers), since per-call option secrets can't be safely persisted (state carries no secrets by design). Added test_create_does_not_mutate_client_credentials.
| result = await sandbox.process.cmd_async( | ||
| command=shell_cmd, | ||
| cwd=effective_cwd, | ||
| timeout=timeout_int, |
There was a problem hiding this comment.
When a manifest/options env is configured, _inject_user_env only makes it available via ~/.profile, but this path sends shell=False commands directly to AgentRun as the raw command string. Calls such as session.exec("python", ..., shell=False) and SDK helpers that use shell=False therefore never source the profile and run without the configured environment, unlike the other sandbox backends. Please source the env file or otherwise inject the env for the direct-exec path too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c9329ad. Once the env file has been written, _run_command prepends . $HOME/.openai_agents_env 2>/dev/null; to the command it sends to AgentRun (whose cmd API has no env parameter). That gives shell=False direct execs — and SDK helpers that use shell=False — the configured environment too, not just the sh -lc login-shell path that reads .profile. Sessions without configured env are unaffected (no prefix). Added test_exec_shell_false_sources_env_after_injection and test_exec_without_env_has_no_source_prefix.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
edadaa8 to
2c9329a
Compare
|
@codex review Addressed both findings from the last pass in
New tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c9329ad3e
ℹ️ 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 env: | ||
| await self._inject_user_env(env) |
There was a problem hiding this comment.
When a fresh resume restores a snapshot under the default /home/user root, BaseSandboxSession.start() calls _prepare_backend_workspace() before _start_workspace(), so this env injection runs before restore_snapshot_into_workspace_on_resume() clears the workspace. Because _inject_user_env() deliberately excludes .openai_agents_env from snapshots, the restore removes the just-written env file and later execs only source a missing file, so resumed sandboxes lose options.env/manifest environment variables. This breaks resumed jobs that rely on configured secrets or other env after snapshot hydration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in $(git rev-parse --short HEAD). Env injection moved from _prepare_backend_workspace() to a new _after_start() override (extracted into _apply_configured_env()). BaseSandboxSession.start() runs _after_start() after _start_workspace(), i.e. after any snapshot restore — so the env file (which is excluded from snapshots) is written after the restore instead of being wiped by it. This covers both fresh create and resume. Tests renamed to test_after_start_injects_env / test_after_start_merges_manifest_env.
There was a problem hiding this comment.
(correction: the commit is e1aad243 — my previous reply mis-rendered the SHA.)
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
2c9329a to
e1aad24
Compare
|
@codex review Addressed the snapshot/env-ordering finding in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1aad2434d
ℹ️ 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".
| # deliberately excluded from snapshots, so injecting it before the restore | ||
| # would let the restore wipe it and leave resumed execs sourcing a missing | ||
| # file. Running after start covers both fresh create and resume. | ||
| await self._apply_configured_env() |
There was a problem hiding this comment.
Because this hook runs only after _start_workspace() has finished, the Aliyun session does not source options.env or manifest.environment while the initial manifest is being applied. Manifest entries that execute commands during start() and rely on those env vars—for example generic S3/Rclone or mount-s3 mounts using env-based credentials—will fail on Aliyun even though the same manifest works on other hosted backends that inject env at sandbox creation or per exec. Consider making the env available before manifest/ephemeral entry application as well as reinjecting it after snapshot restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d2de6c01 by replacing the env mechanism entirely, per maintainer guidance to follow how the other backends handle env. Instead of writing a ~/.openai_agents_env file and sourcing it from .profile, the resolved environment (options + manifest, manifest wins, unsafe names dropped, values shlex.quoted) is now inlined as a KEY=val ... prefix on every command — the same per-exec model as e2b/daytona/runloop, adapted to AgentRun (whose cmd API has no env parameter). Because env travels inline with each command and touches no workspace file, this resolves this finding and the whole class it belonged to at once: it's available during manifest/mount application, survives snapshot restore, needs no login shell (works for shell=False), and never lands in snapshots or logs. Net: the here-doc/base64/.profile/snapshot-skip machinery (and its edge cases) is gone. New env tests cover the prefix, manifest/option merge, unsafe-name skipping, metachar quoting, shell=False, and log redaction.
iridescentWen
force-pushed
the
feat/aliyun-sandbox
branch
from
e1aad24 to
d2de6c0
Compare
|
@codex review Reworked env handling in |
|
Closing in favor of a fresh PR to reset the review thread (this one accumulated many review rounds). The same, finalized commit will be re-submitted as a new PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2de6c01ee
ℹ️ 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 _is_retryable_gateway(exc) and attempt < max_attempts: | ||
| backoff_s = ALIYUN_GATEWAY_502_BACKOFF_BASE_S * (2 ** (attempt - 1)) | ||
| logger.warning( |
There was a problem hiding this comment.
When AgentRun returns a transient 502 after it has accepted /processes/cmd, this branch retries the exact same command for every exec, including user/tool commands. For non-idempotent commands such as appending to files, package installs, or external API calls, a lost response can make the SDK run the side effect multiple times and corrupt the workspace or external state; keep automatic retries to known-idempotent internal probes or let callers retry explicitly.
Useful? React with 👍 / 👎.
| logger.info( | ||
| "AliyunSandboxSession._run_command sandbox_id=%s cwd=%s timeout=%ss cmd=%r", | ||
| self.state.sandbox_id, | ||
| effective_cwd, | ||
| timeout_int, | ||
| shell_cmd[:500], | ||
| ) |
There was a problem hiding this comment.
This logs every command string at INFO, so production applications that pass secrets in argv or shell snippets (for example curl -H 'Authorization: Bearer ...' or signed URLs) will write those values to normal application logs even though the env prefix is redacted. Consider dropping command payloads to DEBUG or redacting/truncating them behind an explicit opt-in.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds an Aliyun (AgentRun) sandbox backend for the Agents SDK sandbox extension,
built on the async
agentrun-sdk(agentrun-sdk==0.0.35, new optionalaliyunextra). It mirrors the existing Vercel backend lifecycle (
create/resume/delete/SandboxSessioncontext manager) so callers can swap backends withoutcode changes.
Highlights:
create_sandbox_async,cmd_async,download_async/upload_async,check_health_async,delete_async,get_sandbox_async) — no blockingto_threadwrappers./processes/cmdhard cap, and aresolve-workspace-pathempty-stdout retry.running()uses the cheap health probe; addsappend(), a 100MB upload guard,and OSS / NAS / PolarFs mount configs passed straight through to
create_sandbox_async.resume()reattaches an existing sandbox viaget_sandbox_asyncand falls backto a fresh sandbox + snapshot rehydration when it is gone.
AliyunSandboxClientOptions(or
AliyunSandboxClientdefaults) and resolved into an agentrunConfig;serialized session state never carries secrets.
agents.extensions.sandboxbehind the optionalaliyunextra(lazy import guarded), with an example runner and reference docs.
Test plan
tests/extensions/sandbox/test_aliyun.py(72 cases) mocks theagentrun-sdkviasys.modulesinjection, so it needs no real package andnever touches Alibaba Cloud. Covers user-arg rejection, exec normalization,
502/timeout handling, file IO,
append, tar validation, health-basedrunning(), shutdown/delete, workspacepersist/hydrate, credentialthreading, resume reattach + fallback, and env injection. Backend module
coverage is 92%.
make format,make lint,make typecheck(mypy + pyright, 0 errors), andmake tests(4920 passed, 1 skipped) all pass locally.Issue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR