Repository navigation
feat(cloud): harden workspace shell parity - #104
Merged
Merged
Conversation
Align cloud shell behavior with local session semantics by handling cd/export in-session, surfacing deterministic timeout errors, and rejecting explicit cwd escapes before remote execution. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Expose an authoritative cloud workspace id in run trace metadata so runtime context stays useful without leaking workspace URLs or trusting caller-supplied cloud markers. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
📝 WalkthroughWalkthroughThis PR adds a ChangesTrace metadata normalization
Cloud shell tooling, tests, and session wiring
Sequence DiagramsequenceDiagram
participant User
participant App as Agent Creation<br/>app.py
participant Trace as _run_trace_metadata
participant Env as CloudEnvironment<br/>(cloud.py)
participant Shell as Shell Tool<br/>bash_run
participant Client as CloudWorkspaceClient
User->>App: create_agent(trace_metadata)
App->>Trace: _run_trace_metadata(environment, trace_metadata)
Trace->>Env: is environment.kind == "cloud"?
alt cloud
Trace->>Env: environment.tool_config()
Env-->>Trace: workspace_id
Trace-->>App: sanitized trace_metadata (cloud.workspace_id set, cloud.* removed)
else non-cloud
Trace-->>App: trace_metadata (unchanged)
end
User->>Shell: execute "cd /path" or "export KEY=VAL" (with cwd/env)
Shell->>Shell: parse command via shlex
Shell->>Shell: resolve/validate execution_cwd with posixpath (ensure inside workspace)
alt valid and parsed cd/export
Shell-->>User: "Changed directory ..." or "Exported KEY=VALUE"
else validated execution -> run
Shell->>Client: run_command(cmd, cwd=execution_cwd, env=...)
Client-->>Shell: result or TimeoutError
alt TimeoutError
Shell-->>User: "Error: cloud command timed out (timed out after Xs)"
else
Shell-->>User: output
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
coding-agent/tests/coding_agent/environment/test_cloud_environment.py (2)
236-245: ⚡ Quick winTest name implies prefix-collision test but covers an unrelated case; the
/workspacefooprefix attack is untested.
test_cloud_environment_rejects_cd_and_explicit_cwd_with_same_prefixtestscd /etcandcwd="/etc", which are unambiguously outside the workspace. The prefix-collision case the name refers to (a path like/workspacefoosharing the/workspaceprefix but not a child) is handled byworkspace_root.rstrip("/") + "/"in_resolve_workspace_pathbut never exercised.💡 Add the actual prefix-attack case
def test_cloud_environment_rejects_cd_and_explicit_cwd_with_same_prefix() -> None: client = FakeCloudWorkspaceClient() shell_tool = CloudEnvironment(client).build_shell_tool() - cd_result = shell_tool("cd /etc", cwd="/workspace") - cwd_result = shell_tool("pwd", cwd="/etc") + # /workspacefoo shares the /workspace prefix but is NOT a child directory + cd_result = shell_tool("cd /workspacefoo", cwd="/workspace") + cwd_result = shell_tool("pwd", cwd="/workspacefoo") - assert cd_result == "Error: Directory is outside cloud workspace: /etc" - assert cwd_result == "Error: Directory is outside cloud workspace: /etc" + assert cd_result == "Error: Directory is outside cloud workspace: /workspacefoo" + assert cwd_result == "Error: Directory is outside cloud workspace: /workspacefoo" assert client.shell_calls == []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/environment/test_cloud_environment.py` around lines 236 - 245, Update the test test_cloud_environment_rejects_cd_and_explicit_cwd_with_same_prefix to exercise the prefix-collision case by invoking the shell tool with a path that shares the workspace prefix but is not a child (e.g. "/workspacefoo") so _resolve_workspace_path's workspace_root.rstrip("/") + "/" logic is exercised; use FakeCloudWorkspaceClient and CloudEnvironment().build_shell_tool() the same way, call shell_tool("cd /workspacefoo", cwd="/workspacefoo") (and/or shell_tool("pwd", cwd="/workspacefoo")) and assert it returns the same "Error: Directory is outside cloud workspace: /workspacefoo" and that client.shell_calls remains empty.
126-134: 💤 Low valueTwo separate
CloudEnvironment(client)instances may cause confusion; consider sharing one.
shell_session.do_mount(...)receives the firstCloudEnvironment(client), andCoreToolsPlugin(...)receives a second independent instance. SinceCloudEnvironmentis stateless (all state lives inclient), this works correctly today — but ifCloudEnvironmentever gains instance state, the two instances would diverge silently.💡 Suggested simplification
+ env = CloudEnvironment(client) shell_session = ShellSessionPlugin() _ = shell_session.do_mount( - ctx=type("Ctx", (), {"config": {"environment": CloudEnvironment(client)}})() + ctx=type("Ctx", (), {"config": {"environment": env}})() ) plugin = CoreToolsPlugin( - environment=CloudEnvironment(client), + environment=env, shell_session=shell_session, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/environment/test_cloud_environment.py` around lines 126 - 134, The test creates two separate CloudEnvironment(client) instances (one passed into shell_session.do_mount via the fake ctx and a second passed to CoreToolsPlugin), which could diverge if CloudEnvironment later becomes stateful; change the test to instantiate a single CloudEnvironment once (e.g., env = CloudEnvironment(client)) and pass that same env into the ctx passed to ShellSessionPlugin.do_mount and into CoreToolsPlugin to ensure both shell_session.do_mount and CoreToolsPlugin use the identical CloudEnvironment instance; keep references to FakeCloudWorkspaceClient, ShellSessionPlugin.do_mount, CoreToolsPlugin, CloudEnvironment, shell_session and plugin to locate and update the code.coding-agent/tests/coding_agent/test_bootstrap.py (1)
332-396: 💤 Low valueNew trace-metadata tests are thorough; the string-scan assertion is slightly fragile.
Both tests correctly exercise
_run_trace_metadata: the cloud test confirms stalecloud.workspace_idis overwritten andworkspace_urlis excluded; the local test confirms allcloud.*keys are stripped. One minor fragility: thestr(dict(…))secret-scan on lines 364 and 396 would false-positive if any legitimate metadata value happened to contain the substring"secret"or"cloud.". The direct key assertions on lines 363 and 394 already provide the meaningful coverage; consider replacing the string-scan assertions with more precise checks.💡 More precise alternative
- assert "secret" not in str(dict(ctx.run_context.trace_metadata)) + assert not any("secret" in str(v) for v in ctx.run_context.trace_metadata.values())- assert "cloud." not in str(dict(ctx.run_context.trace_metadata)) + assert not any(k.startswith("cloud.") for k in ctx.run_context.trace_metadata)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/test_bootstrap.py` around lines 332 - 396, Replace the fragile string-scans in the two tests (test_create_agent_adds_secret_free_cloud_workspace_trace_metadata and test_create_agent_strips_cloud_trace_metadata_for_local_environment) with precise checks against the trace_metadata dict: assert directly that no key startswith "cloud." (e.g., all(not k.startswith("cloud.") for k in ctx.run_context.trace_metadata.keys())) and assert that no value contains the substring "secret" (e.g., all("secret" not in str(v) for v in ctx.run_context.trace_metadata.values())); keep the existing direct key assertions (like exact trace_metadata equality and absence/presence of workspace_url) and remove the str(dict(...)) assertions.coding-agent/src/coding_agent/environment/cloud.py (1)
153-191: 💤 Low value
bash_runreturn-message format forcd/exportis an implicit contract withShellSessionPlugin.When
_apply_cdor_apply_exportsucceeds,bash_runreturns a plain string ("Changed directory to {path}"/"Exported {key}={value}").ShellSessionPluginpresumably parses these exact strings to update sessioncwd/env_vars. This creates a silent integration contract: changing the message format (e.g., internationalizing it, adding punctuation) would break session state tracking without any compile-time warning.Consider documenting this contract with a constant or structured return, or verify it is explicitly covered by the integration test in
test_cloud_environment_shell_tool_updates_session_cwd_and_env.#!/bin/bash # Verify how ShellSessionPlugin processes bash_run return values for cd/export rg -n "Changed directory\|Exported\|cd\|export\|env_vars\|session_cwd" \ --type py -A3 -B3 \ coding-agent/src/coding_agent/plugins/shell_session.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/environment/cloud.py` around lines 153 - 191, The bash_run function in build_shell_tool currently returns free-form strings for _apply_cd and _apply_export ("Changed directory to ..." / "Exported ..."), creating an implicit contract with ShellSessionPlugin; change bash_run to return a structured, stable indicator (e.g., a constant tag plus payload or a dict-like string) instead of free-form text and update ShellSessionPlugin and the test test_cloud_environment_shell_tool_updates_session_cwd_and_env to parse that structured response; specifically, introduce stable tokens or keys (e.g., "SHELL_SESSION_ACTION:cd:<path>" and "SHELL_SESSION_ACTION:export:<key>=<value>" or JSON with action/path) in build_shell_tool::bash_run, update ShellSessionPlugin to detect and handle those tokens (instead of exact human-readable phrases), and ensure the integration test asserts the new structured format so future formatting changes won’t break session cwd/env updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@coding-agent/src/coding_agent/environment/cloud.py`:
- Around line 153-191: The bash_run function in build_shell_tool currently
returns free-form strings for _apply_cd and _apply_export ("Changed directory to
..." / "Exported ..."), creating an implicit contract with ShellSessionPlugin;
change bash_run to return a structured, stable indicator (e.g., a constant tag
plus payload or a dict-like string) instead of free-form text and update
ShellSessionPlugin and the test
test_cloud_environment_shell_tool_updates_session_cwd_and_env to parse that
structured response; specifically, introduce stable tokens or keys (e.g.,
"SHELL_SESSION_ACTION:cd:<path>" and "SHELL_SESSION_ACTION:export:<key>=<value>"
or JSON with action/path) in build_shell_tool::bash_run, update
ShellSessionPlugin to detect and handle those tokens (instead of exact
human-readable phrases), and ensure the integration test asserts the new
structured format so future formatting changes won’t break session cwd/env
updates.
In `@coding-agent/tests/coding_agent/environment/test_cloud_environment.py`:
- Around line 236-245: Update the test
test_cloud_environment_rejects_cd_and_explicit_cwd_with_same_prefix to exercise
the prefix-collision case by invoking the shell tool with a path that shares the
workspace prefix but is not a child (e.g. "/workspacefoo") so
_resolve_workspace_path's workspace_root.rstrip("/") + "/" logic is exercised;
use FakeCloudWorkspaceClient and CloudEnvironment().build_shell_tool() the same
way, call shell_tool("cd /workspacefoo", cwd="/workspacefoo") (and/or
shell_tool("pwd", cwd="/workspacefoo")) and assert it returns the same "Error:
Directory is outside cloud workspace: /workspacefoo" and that client.shell_calls
remains empty.
- Around line 126-134: The test creates two separate CloudEnvironment(client)
instances (one passed into shell_session.do_mount via the fake ctx and a second
passed to CoreToolsPlugin), which could diverge if CloudEnvironment later
becomes stateful; change the test to instantiate a single CloudEnvironment once
(e.g., env = CloudEnvironment(client)) and pass that same env into the ctx
passed to ShellSessionPlugin.do_mount and into CoreToolsPlugin to ensure both
shell_session.do_mount and CoreToolsPlugin use the identical CloudEnvironment
instance; keep references to FakeCloudWorkspaceClient,
ShellSessionPlugin.do_mount, CoreToolsPlugin, CloudEnvironment, shell_session
and plugin to locate and update the code.
In `@coding-agent/tests/coding_agent/test_bootstrap.py`:
- Around line 332-396: Replace the fragile string-scans in the two tests
(test_create_agent_adds_secret_free_cloud_workspace_trace_metadata and
test_create_agent_strips_cloud_trace_metadata_for_local_environment) with
precise checks against the trace_metadata dict: assert directly that no key
startswith "cloud." (e.g., all(not k.startswith("cloud.") for k in
ctx.run_context.trace_metadata.keys())) and assert that no value contains the
substring "secret" (e.g., all("secret" not in str(v) for v in
ctx.run_context.trace_metadata.values())); keep the existing direct key
assertions (like exact trace_metadata equality and absence/presence of
workspace_url) and remove the str(dict(...)) assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36b5a2e8-b40a-4770-8daf-33ef8e643af3
📒 Files selected for processing (6)
coding-agent/src/coding_agent/app.pycoding-agent/src/coding_agent/environment/cloud.pycoding-agent/src/coding_agent/ui/binding_resolver.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/coding_agent/environment/test_cloud_environment.pycoding-agent/tests/coding_agent/test_bootstrap.py
💤 Files with no reviewable changes (1)
- coding-agent/src/coding_agent/ui/binding_resolver.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cdandexport, surface deterministic timeout failures, and reject explicit cwd escapes before remote executioncloud.workspace_idtrace metadata without leaking workspace URLs or trusting caller-supplied cloud markersVerification
uv run pytest tests/coding_agent/environment/test_cloud_environment.py tests/coding_agent/plugins/test_core_tools.py tests/coding_agent/test_bootstrap.py -k \"cloud or non_local or workspace\" -vuv run pytest tests/agentkit/environment/ tests/coding_agent/environment/ tests/coding_agent/plugins/test_core_tools.py tests/coding_agent/test_bootstrap.py tests/ui/test_execution_binding.py tests/ui/test_session_manager_runtime.py tests/ui/test_http_server.py -k \"environment or cloud or binding or workspace\" -vuv run pytest tests/coding_agent/tools/test_shell.py tests/tools/test_shell.py -k \"cd or export or timeout or non_zero_exit_code\" -vuv run python -m compileall -q src/agentkit src/coding_agent tests/agentkit/environment tests/coding_agent tests/uicd,export, timeout, cwd escape rejection, and trace metadata behaviorNotes
uv run ruff format ...is still unavailable here becauseruffis not installed/spawnable in this environmentSummary by CodeRabbit
New Features
cdandexportwith workspace-aware path validation and clear error/timeout messages.Tests
Refactor