Repository navigation
feat: add workspace transfer round-trip - #117
Merged
Merged
Conversation
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>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces workspace archive transfer capability, enabling local workspace snapshots to be uploaded to and downloaded from remote coding agent sessions. Changes span archive utilities, a provider interface extension, Docker implementation, HTTP server endpoints, CLI commands, and comprehensive test coverage. ChangesWorkspace Archive Transfer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/src/coding_agent/__main__.py (1)
464-468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale help text for
--repo.The option help still claims
--repois "Reserved for future workspace upload; currently unsupported", but this PR is exactly what makes it supported —--reponow uploads a snapshot and rounds it back. Update the help string so--helpoutput matches the new behavior.Proposed fix
`@click.option`( "--repo", default=None, - help="Reserved for future workspace upload; currently unsupported.", + help="Local repository path to upload as the remote workspace; remote changes are written back here on completion.", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/__main__.py` around lines 464 - 468, The help string for the click option "--repo" is outdated; update the click.option declaration for "--repo" in coding_agent.__main__.py so the help text reflects its new behavior (it now uploads a snapshot and rounds it back) instead of saying it's unsupported; locate the click.option block for "--repo" and replace the help message with a concise description of the new supported behavior (mentioning snapshot upload/roundtrip) so the --help output matches the implemented functionality.
🧹 Nitpick comments (10)
coding-agent/src/coding_agent/environment/workspace_provider.py (1)
35-46: 💤 Low valueProtocol expansion is a breaking change for third-party providers.
WorkspaceProvideris aProtocol, but adding required methods is still a breaking change for any external implementation registered viaregister_workspace_provider. If you intend to leave room for providers that don't support archive transfer, consider giving these methods defaultraise NotImplementedErrorimplementations on a base class (or having the_from_confighelpers raise a clear "provider X does not support workspace archive transfer" message) so non-Docker providers can be added without immediately breaking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/environment/workspace_provider.py` around lines 35 - 46, The new required Protocol methods import_workspace_archive and export_workspace_archive break third-party providers; make them optional by providing default implementations that raise NotImplementedError on a concrete base class (e.g., create or update a BaseWorkspaceProvider with import_workspace_archive and export_workspace_archive that raise NotImplementedError) and update register_workspace_provider and any _from_config helper to instantiate/return that base class (or explicitly catch missing support and raise a clear "provider X does not support workspace archive transfer" error) so existing non-archive providers do not break.coding-agent/src/coding_agent/ui/http_server.py (2)
140-146: ⚡ Quick winSnapshot import errors during provisioning may leak the cloud binding.
If
import_workspace_archive_from_configraises (bad base64, traversal, IO error), control flows up to theexcept Exceptionat L771–L783 which calls_cleanup_provisioned_cloud_binding. Good. But the_populate_provisioned_cloud_bindingcall doesn't itself surface a structured error type —ValueErrorfrom the archive layer becomes400via the generic handler, which is fine, but a tarfile-level error (e.g.,tarfile.ReadError) is not in the explicit(ValueError, TypeError)whitelist and will be reported as500even though the failure is purely a malformed client-supplied payload. Consider catching tar/IO errors close to the call site and re-raising asValueErrorso the response code matches the cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 140 - 146, The call to import_workspace_archive_from_config inside _populate_provisioned_cloud_binding can raise tarfile/IO errors which currently surface as 500; catch tarfile.TarError (including ReadError) and relevant IO errors (OSError/IOError) inside _populate_provisioned_cloud_binding and re-raise a ValueError with a clear message so the generic handler treats malformed/malicious archives as client errors; ensure you only transform those archive/IO-related exceptions and let other exceptions propagate so _cleanup_provisioned_cloud_binding and the existing except blocks still run as before.
1008-1038: ⚡ Quick winUse a more restrictive rate limit for workspace export.
The endpoint exports a full workspace tarball and returns it base64-encoded in JSON, making it substantially heavier than typical
GET /sessions/{id}operations. UsingRateLimits.GET_SESSION(60/minute) exposes the service to resource exhaustion; considerRateLimits.CAPTURE_CHECKPOINT(20/minute) or a dedicated tighter limit to match the computational cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 1008 - 1038, The get_workspace_archive endpoint is using the generic, permissive limiter.limit(RateLimits.GET_SESSION) but exports a heavy base64 tarball; change the rate limit on the `@limiter.limit` decorator for get_workspace_archive to a more restrictive value such as RateLimits.CAPTURE_CHECKPOINT (or create and reference a new dedicated constant like RateLimits.WORKSPACE_EXPORT) to reduce allowed requests (e.g., from 60/min to 20/min) and update any related rate limit enum/definition if adding a new constant.coding-agent/src/coding_agent/remote/client.py (1)
134-146: ⚡ Quick winTight 60s timeout for archive upload/download.
create_remote_sessionanddownload_workspace_archiveboth usetimeout=60.0(single shared timeout). For non-trivial--reposnapshots, the upload POST and the download GET will be doing a base64-laden body transfer plus server-side tar packing, which can easily exceed 60s on large repos and surface as aRequestError("Failed to create remote session: ..."). Consider decoupling connect vs. read (similar tostream_prompt'shttpx.Timeout(connect=10.0, read=None, ...)), or making the read timeout configurable so users transferring sizeable workspaces aren't artificially capped.Also applies to: 162-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/remote/client.py` around lines 134 - 146, The shared hardcoded timeout=60.0 in the httpx.Client used by create_remote_session (and likewise in download_workspace_archive) causes large archive uploads/downloads to fail; replace the single float timeout with an httpx.Timeout instance that separates connect and read timeouts (e.g., connect short, read None or a larger value) or wire in a configurable read timeout option; update the httpx.Client(...) timeout argument in create_remote_session and download_workspace_archive accordingly (mirroring stream_prompt's httpx.Timeout(connect=..., read=...)) so long transfers aren’t capped at 60s.coding-agent/src/coding_agent/environment/docker_workspace_provider.py (1)
389-412: ⚡ Quick winImport is additive; consider documenting or guarding repeated imports.
import_workspace_archiveextracts directly into the workspace root that the docker container already mounts. Calling it more than once on the same binding (or after the container has written files) produces a merged tree, not a replacement — files only present on disk but absent from the new archive will linger. For the currentPOST /sessionsflow it's only invoked once on a freshly provisioned binding, but the public provider API allows repeat calls. At minimum, document that this method is intended for a freshly provisioned, empty workspace; ideally, assert the workspace is empty (or refuse) and surface an explicit error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py` around lines 389 - 412, import_workspace_archive currently extracts into the mounted workspace root and will merge with existing files; change it to first verify the target workspace is empty (using the directory returned by _workspace_root_for_id) and refuse with a clear error (e.g., raise ValueError or a WorkspaceNotEmptyError) if any files or non-hidden entries exist, instead of blindly calling extract_workspace_archive_base64; reference import_workspace_archive, _docker_workspace_provider_config, _workspace_root_for_id, and extract_workspace_archive_base64 when adding the pre-check and error path, or alternatively add a docstring comment stating the method requires a freshly provisioned empty workspace if you prefer documentation rather than enforcement.coding-agent/src/coding_agent/ui/schemas.py (1)
22-24: ⚡ Quick winConsider bounding
snapshot_archive_base64size.The new field has
min_length=1but nomax_length, while the rest of the schemas in this module bound their string fields. Since this payload is base64-decoded and unpacked through tarfile in_populate_provisioned_cloud_binding, an unbounded value lets a single request inflate into a very large in-memory tar plus on-disk extraction. A pragmatic upper bound (matched against your operational tar size limit) here would shed obviously-bad uploads at the FastAPI validation layer before they hit the worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/ui/schemas.py` around lines 22 - 24, The DockerWorkspaceSourceRequest schema currently allows unbounded snapshot_archive_base64 which can lead to huge in-memory and on-disk tar extraction in _populate_provisioned_cloud_binding; add a max_length to snapshot_archive_base64 (choose a value that corresponds to your operational tar size limit converted to base64 size, e.g. ~1.34x the raw byte limit) so FastAPI validation rejects excessively large uploads before they reach _populate_provisioned_cloud_binding and ensure the error message or Field description reflects the intended raw tar size limit.coding-agent/src/coding_agent/workspace_archive.py (3)
14-28: 🏗️ Heavy liftWhole-archive in memory may be a problem for non-trivial repos.
The implementation reads each file with
path.read_bytes(), accumulates the full gzip tar into aBytesIO, then base64-encodes the result. Peak memory is roughly(uncompressed tar) + (compressed tar) + (base64 of compressed tar), all on the heap simultaneously, on both the client and server. For a moderately sized repo (e.g., a few hundred MB after exclusions) this can easily push >1GB resident, and the HTTP layer then JSON-encodes that base64 string again. For the initial round-trip MVP this may be acceptable, but worth tracking for a follow-up: streamaddfilefrom open file objects, and consider a streaming binary endpoint (multipart orapplication/octet-stream) instead of base64 in JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/workspace_archive.py` around lines 14 - 28, The current archive creation keeps the entire gzip tar and base64 string in memory by using a BytesIO buffer, path.read_bytes(), archive.addfile with in-memory BytesIO, and then base64.b64encode(buffer.getvalue()); change this to stream files into the tar/gzip writer and avoid building the whole archive in RAM: open each file with path.open("rb") and pass the file object as the fileobj argument to tarfile.addfile (setting TarInfo.size from file.stat().st_size) or use tarfile.add with the file path to let tarfile stream from disk, and write the tar-gz output to a streaming sink (tempfile or direct HTTP stream) instead of collecting into buffer and base64-encoding; also update the transport to use a binary multipart or application/octet-stream endpoint rather than embedding base64 in JSON.
16-19: 🏗️ Heavy lift
.gitexclusion only matches top-level; consider broader skip rules.The check
relative.parts[0] == ".git"only filters the workspace's own top-level.gitdirectory. Submodule.gitfiles/directories,node_modules,.venv,__pycache__,dist,build, secrets like.env, etc. all flow through the archive. For a workspace transfer feature this means the upload payload silently includes vendored deps and credentials, and the round-trip overwrites them on extract. Consider a configurable ignore list (or honoring.gitignore) and at least excluding common heavy/sensitive directories by default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/workspace_archive.py` around lines 16 - 19, The current archive loop only skips a top-level ".git" because it checks relative.parts[0] == ".git"; change this to use a configurable ignore list (and optionally honor .gitignore) and test against any path segment or glob pattern instead of only the first part. Update the iteration around root.rglob("*") / relative = path.relative_to(root) to skip entries when any segment in relative.parts matches common defaults (e.g., ".git", "node_modules", ".venv", "__pycache__", "dist", "build") or when the relative path matches configured glob patterns or loaded .gitignore rules; expose the ignore list as a parameter or setting so callers can override, and ensure secrets like ".env" are excluded by default. Ensure the check covers nested occurrences (not just relative.parts[0]) and document the new ignore option in the function/class docstring.
38-41: 💤 Low value
UnicodeEncodeErrorescapes the validation wrapper.
archive_base64.encode("ascii")raisesUnicodeEncodeError(a subclass ofValueErrorin CPython, actually — butb64decode(validate=True)only checks alphabet on the bytes, not the original str). If a caller passes a string containing non-ASCII characters, the encode step still surfaces as aUnicodeEncodeError, which technically subclassesValueErrorso it is caught — but the wrapped message becomes "workspace archive must be valid base64" with the originalUnicodeEncodeErrorchained, which is confusing. Consider explicitly normalizing input or assertingarchive_base64.isascii()first to produce a clearer error before decoding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/workspace_archive.py` around lines 38 - 41, Before calling base64.b64decode on archive_base64, explicitly validate that the input is an ASCII-only string (e.g. check isinstance(archive_base64, str) and archive_base64.isascii()); if that check fails raise a clear ValueError like "workspace archive must be an ASCII base64 string" so a UnicodeEncodeError cannot leak into the chained exception, then proceed to encode with .encode("ascii") and call base64.b64decode(..., validate=True) inside the existing try/except (keeping the existing ValueError handling for decoding errors) — reference the archive_base64 variable and the base64.b64decode call in the current try/except block.coding-agent/tests/coding_agent/environment/test_docker_workspace_provider_transfer.py (1)
32-63: ⚡ Quick winMake cleanup unconditional for better test isolation.
cleanup_cloud_binding_from_configat Line 62 is skipped if any earlier assertion fails. Wrap the test body intry/finallyso cleanup always runs.Suggested change
binding = provision_cloud_binding_from_config(config, {"kind": "docker"}) - - source = tmp_path / "source" - (source / "nested").mkdir(parents=True) - (source / "README.md").write_text("uploaded\n", encoding="utf-8") - (source / "nested" / "data.txt").write_text("from client\n", encoding="utf-8") - - import_workspace_archive_from_config( - config, - binding, - create_workspace_archive_base64(source), - ) - - workspace_path = workspace_root / binding.workspace_id - assert (workspace_path / "README.md").read_text(encoding="utf-8") == "uploaded\n" - assert ( - workspace_path / "nested" / "data.txt" - ).read_text(encoding="utf-8") == "from client\n" - - export_target = tmp_path / "export" - extract_workspace_archive_base64( - export_target, - export_workspace_archive_from_config(config, binding), - ) - - assert (export_target / "README.md").read_text(encoding="utf-8") == "uploaded\n" - assert ( - export_target / "nested" / "data.txt" - ).read_text(encoding="utf-8") == "from client\n" - - cleanup_cloud_binding_from_config(config, binding) + try: + source = tmp_path / "source" + (source / "nested").mkdir(parents=True) + (source / "README.md").write_text("uploaded\n", encoding="utf-8") + (source / "nested" / "data.txt").write_text("from client\n", encoding="utf-8") + + import_workspace_archive_from_config( + config, + binding, + create_workspace_archive_base64(source), + ) + + workspace_path = workspace_root / binding.workspace_id + assert (workspace_path / "README.md").read_text(encoding="utf-8") == "uploaded\n" + assert ( + workspace_path / "nested" / "data.txt" + ).read_text(encoding="utf-8") == "from client\n" + + export_target = tmp_path / "export" + extract_workspace_archive_base64( + export_target, + export_workspace_archive_from_config(config, binding), + ) + + assert (export_target / "README.md").read_text(encoding="utf-8") == "uploaded\n" + assert ( + export_target / "nested" / "data.txt" + ).read_text(encoding="utf-8") == "from client\n" + finally: + cleanup_cloud_binding_from_config(config, binding)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/tests/coding_agent/environment/test_docker_workspace_provider_transfer.py` around lines 32 - 63, The test currently calls cleanup_cloud_binding_from_config(binding) at the end but will skip it if an assertion raises; modify the test to ensure cleanup always runs by wrapping the main test body (the provisioning via provision_cloud_binding_from_config, import_workspace_archive_from_config, assertions, export and extract steps) in a try/finally and call cleanup_cloud_binding_from_config(config, binding) from the finally block; locate references to binding, provision_cloud_binding_from_config, import_workspace_archive_from_config, export_workspace_archive_from_config, extract_workspace_archive_base64 and cleanup_cloud_binding_from_config in the test to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/__main__.py`:
- Around line 511-532: Wrap the remote session lifecycle in a try/finally: after
calling create_remote_session(...) and before raising SystemExit(status) ensure
you always DELETE the session on the server (call the existing session-delete
endpoint for session_id) in the finally block; also, move the
download_workspace_archive(...) and extract_workspace_archive_base64(...) into
that finally (best-effort) so that even if stream_prompt(...) raises or is
interrupted you attempt to fetch and extract the workspace before re-raising the
original error or exiting; preserve original exceptions (re-raise after the
finally) so errors are not swallowed while still guaranteeing session cleanup
and attempted download.
In `@coding-agent/src/coding_agent/workspace_archive.py`:
- Around line 31-57: extract_workspace_archive_base64 currently overlays files
and therefore resurrects remote-deleted files; fix by removing non-preserved
entries in workspace_root before extracting so the extracted tree exactly
reflects the archive. Implement in extract_workspace_archive_base64: compute a
preservation set (at minimum ".git" and any other paths skipped by
create_workspace_archive_base64), iterate workspace_root children and remove
everything not in that set (use Path.rmdir/unlink or shutil.rmtree as
appropriate), then proceed with the existing safe extraction logic (keeping use
of _safe_archive_target and the validate/decode steps). Ensure the preservation
rules match create_workspace_archive_base64's skip rules so .git and other
intended entries are not deleted.
- Around line 9-28: The archive writer create_workspace_archive_base64 currently
sets only TarInfo.size and leaves mode/mtime at zero; update it to stat the
source file (path.stat()) and populate TarInfo.mode and TarInfo.mtime (and
optionally uid/gid) from that stat before archive.addfile so executability and
timestamps are preserved; also ensure the extractor applies member.mode and
member.mtime after writing files (e.g., call chmod(member.mode & 0o777) and
os.utime(path, (member.mtime, member.mtime))) so the preserved metadata takes
effect.
In `@coding-agent/tests/ui/test_http_server_workspace_transfer.py`:
- Around line 50-56: The global rate limiter state is only reset before each
test but not after, so add a call to limiter.reset() in the test teardown path
to avoid state leakage; update the teardown sequence (where
session_manager.configure_owner_leases(...) and session_manager.clear_sessions()
run) to call limiter.reset() after clearing sessions so the global limiter is
returned to a clean state for subsequent tests.
---
Outside diff comments:
In `@coding-agent/src/coding_agent/__main__.py`:
- Around line 464-468: The help string for the click option "--repo" is
outdated; update the click.option declaration for "--repo" in
coding_agent.__main__.py so the help text reflects its new behavior (it now
uploads a snapshot and rounds it back) instead of saying it's unsupported;
locate the click.option block for "--repo" and replace the help message with a
concise description of the new supported behavior (mentioning snapshot
upload/roundtrip) so the --help output matches the implemented functionality.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 389-412: import_workspace_archive currently extracts into the
mounted workspace root and will merge with existing files; change it to first
verify the target workspace is empty (using the directory returned by
_workspace_root_for_id) and refuse with a clear error (e.g., raise ValueError or
a WorkspaceNotEmptyError) if any files or non-hidden entries exist, instead of
blindly calling extract_workspace_archive_base64; reference
import_workspace_archive, _docker_workspace_provider_config,
_workspace_root_for_id, and extract_workspace_archive_base64 when adding the
pre-check and error path, or alternatively add a docstring comment stating the
method requires a freshly provisioned empty workspace if you prefer
documentation rather than enforcement.
In `@coding-agent/src/coding_agent/environment/workspace_provider.py`:
- Around line 35-46: The new required Protocol methods import_workspace_archive
and export_workspace_archive break third-party providers; make them optional by
providing default implementations that raise NotImplementedError on a concrete
base class (e.g., create or update a BaseWorkspaceProvider with
import_workspace_archive and export_workspace_archive that raise
NotImplementedError) and update register_workspace_provider and any _from_config
helper to instantiate/return that base class (or explicitly catch missing
support and raise a clear "provider X does not support workspace archive
transfer" error) so existing non-archive providers do not break.
In `@coding-agent/src/coding_agent/remote/client.py`:
- Around line 134-146: The shared hardcoded timeout=60.0 in the httpx.Client
used by create_remote_session (and likewise in download_workspace_archive)
causes large archive uploads/downloads to fail; replace the single float timeout
with an httpx.Timeout instance that separates connect and read timeouts (e.g.,
connect short, read None or a larger value) or wire in a configurable read
timeout option; update the httpx.Client(...) timeout argument in
create_remote_session and download_workspace_archive accordingly (mirroring
stream_prompt's httpx.Timeout(connect=..., read=...)) so long transfers aren’t
capped at 60s.
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 140-146: The call to import_workspace_archive_from_config inside
_populate_provisioned_cloud_binding can raise tarfile/IO errors which currently
surface as 500; catch tarfile.TarError (including ReadError) and relevant IO
errors (OSError/IOError) inside _populate_provisioned_cloud_binding and re-raise
a ValueError with a clear message so the generic handler treats
malformed/malicious archives as client errors; ensure you only transform those
archive/IO-related exceptions and let other exceptions propagate so
_cleanup_provisioned_cloud_binding and the existing except blocks still run as
before.
- Around line 1008-1038: The get_workspace_archive endpoint is using the
generic, permissive limiter.limit(RateLimits.GET_SESSION) but exports a heavy
base64 tarball; change the rate limit on the `@limiter.limit` decorator for
get_workspace_archive to a more restrictive value such as
RateLimits.CAPTURE_CHECKPOINT (or create and reference a new dedicated constant
like RateLimits.WORKSPACE_EXPORT) to reduce allowed requests (e.g., from 60/min
to 20/min) and update any related rate limit enum/definition if adding a new
constant.
In `@coding-agent/src/coding_agent/ui/schemas.py`:
- Around line 22-24: The DockerWorkspaceSourceRequest schema currently allows
unbounded snapshot_archive_base64 which can lead to huge in-memory and on-disk
tar extraction in _populate_provisioned_cloud_binding; add a max_length to
snapshot_archive_base64 (choose a value that corresponds to your operational tar
size limit converted to base64 size, e.g. ~1.34x the raw byte limit) so FastAPI
validation rejects excessively large uploads before they reach
_populate_provisioned_cloud_binding and ensure the error message or Field
description reflects the intended raw tar size limit.
In `@coding-agent/src/coding_agent/workspace_archive.py`:
- Around line 14-28: The current archive creation keeps the entire gzip tar and
base64 string in memory by using a BytesIO buffer, path.read_bytes(),
archive.addfile with in-memory BytesIO, and then
base64.b64encode(buffer.getvalue()); change this to stream files into the
tar/gzip writer and avoid building the whole archive in RAM: open each file with
path.open("rb") and pass the file object as the fileobj argument to
tarfile.addfile (setting TarInfo.size from file.stat().st_size) or use
tarfile.add with the file path to let tarfile stream from disk, and write the
tar-gz output to a streaming sink (tempfile or direct HTTP stream) instead of
collecting into buffer and base64-encoding; also update the transport to use a
binary multipart or application/octet-stream endpoint rather than embedding
base64 in JSON.
- Around line 16-19: The current archive loop only skips a top-level ".git"
because it checks relative.parts[0] == ".git"; change this to use a configurable
ignore list (and optionally honor .gitignore) and test against any path segment
or glob pattern instead of only the first part. Update the iteration around
root.rglob("*") / relative = path.relative_to(root) to skip entries when any
segment in relative.parts matches common defaults (e.g., ".git", "node_modules",
".venv", "__pycache__", "dist", "build") or when the relative path matches
configured glob patterns or loaded .gitignore rules; expose the ignore list as a
parameter or setting so callers can override, and ensure secrets like ".env" are
excluded by default. Ensure the check covers nested occurrences (not just
relative.parts[0]) and document the new ignore option in the function/class
docstring.
- Around line 38-41: Before calling base64.b64decode on archive_base64,
explicitly validate that the input is an ASCII-only string (e.g. check
isinstance(archive_base64, str) and archive_base64.isascii()); if that check
fails raise a clear ValueError like "workspace archive must be an ASCII base64
string" so a UnicodeEncodeError cannot leak into the chained exception, then
proceed to encode with .encode("ascii") and call base64.b64decode(...,
validate=True) inside the existing try/except (keeping the existing ValueError
handling for decoding errors) — reference the archive_base64 variable and the
base64.b64decode call in the current try/except block.
In
`@coding-agent/tests/coding_agent/environment/test_docker_workspace_provider_transfer.py`:
- Around line 32-63: The test currently calls
cleanup_cloud_binding_from_config(binding) at the end but will skip it if an
assertion raises; modify the test to ensure cleanup always runs by wrapping the
main test body (the provisioning via provision_cloud_binding_from_config,
import_workspace_archive_from_config, assertions, export and extract steps) in a
try/finally and call cleanup_cloud_binding_from_config(config, binding) from the
finally block; locate references to binding,
provision_cloud_binding_from_config, import_workspace_archive_from_config,
export_workspace_archive_from_config, extract_workspace_archive_base64 and
cleanup_cloud_binding_from_config in the test to apply the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7b7c20d-88de-4067-8270-56d5d574425a
📒 Files selected for processing (12)
coding-agent/src/coding_agent/__main__.pycoding-agent/src/coding_agent/environment/__init__.pycoding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/src/coding_agent/environment/workspace_provider.pycoding-agent/src/coding_agent/remote/client.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/schemas.pycoding-agent/src/coding_agent/workspace_archive.pycoding-agent/tests/cli/test_remote_client.pycoding-agent/tests/coding_agent/environment/test_docker_workspace_provider_transfer.pycoding-agent/tests/coding_agent/environment/test_workspace_archive.pycoding-agent/tests/ui/test_http_server_workspace_transfer.py
Preserve file metadata and clear stale files before extraction so downloaded workspaces round-trip cleanly into existing repos. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep workspace download and remote session deletion on the remote repl teardown path so streamed runs don't leak sessions or lose final workspace state. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Align the async workspace transfer fixtures with the shared rate limiter and async client typing so the focused transfer tests stay isolated. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
coding-agent/src/coding_agent/__main__.py (1)
526-527: ⚡ Quick winRuff BLE001: blind
except BaseExceptionshould be narrowed toexcept Exception.
KeyboardInterrupt,SystemExit, andGeneratorExitareBaseExceptionsubclasses that should not be caught here. Thefinallyblock already executes onKeyboardInterruptregardless of whether it is caught, so narrowing theexceptclause does not change the cleanup behaviour (workspace download and session deletion still run). Usingexcept Exceptionalso satisfies the Ruff BLE001 warning.♻️ Proposed change
- except BaseException as exc: + except Exception as exc: stream_error = exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/__main__.py` around lines 526 - 527, The current except block uses "except BaseException as exc" which improperly catches SystemExit/KeyboardInterrupt; update the handler to "except Exception as exc" so only regular exceptions are caught, assign the caught exception to stream_error as before, and leave the existing finally block (workspace download and session deletion) unchanged so cleanup still runs; target the except block that assigns to stream_error in __main__.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/__main__.py`:
- Around line 530-541: The inner try/except around
download_workspace_archive/extract_workspace_archive_base64 is too narrow and
lets tarfile.ReadError/OSError/shutil.Error escape and skip
delete_remote_session and clobber stream_error; change the inner except from
"except (click.ClickException, ValueError)" to "except Exception as exc" so all
extraction/download errors are caught, when caught add the original exception
message to stream_error via stream_error.add_note(f"Workspace download also
failed: {exc}") if stream_error is not None, otherwise re-raise as
click.ClickException(str(exc)) from exc, ensuring the surrounding finally still
runs and delete_remote_session is always invoked; keep references to
download_workspace_archive, extract_workspace_archive_base64,
delete_remote_session and stream_error so reviewers can find the change.
In `@coding-agent/src/coding_agent/workspace_archive.py`:
- Around line 46-55: The workspace is being cleared by
_clear_extract_target(root) before the tar.gz bytes are validated, which can
cause data loss if tarfile.open raises tarfile.ReadError; change the flow in the
function that decodes archive_base64 so you first validate the archive by
opening tarfile.open(fileobj=io.BytesIO(archive_bytes), mode="r:gz") (catching
tarfile.ReadError and re-raising a clear error) and fully iterate/getmembers to
ensure it's a valid tar stream, then close/rewind the BytesIO and only then call
_clear_extract_target(root) and perform the actual extraction using the
validated BytesIO; keep references to _safe_archive_target(member.name) for path
checks and ensure exceptions from tarfile are handled and propagated without
clearing the workspace prematurely.
- Around line 67-69: The code currently applies member.mode blindly which yields
chmod(0) for TarInfo entries with no stored mode; update the extraction logic
around target_path.chmod to compute mode = member.mode & 0o777, and if that is
zero substitute a sensible default (e.g., 0o755 for directories or entries with
any executable bit, otherwise 0o644 for regular files); also avoid calling chmod
for symlinks (member.issym()) and then call target_path.chmod(mode). Use the
existing symbols member and target_path in the workspace_archive extraction
function to locate and patch this behavior.
In `@coding-agent/tests/ui/test_http_server_workspace_transfer.py`:
- Around line 18-26: The helper _build_workspace_archive builds TarInfo entries
with default mode 0 which results in extracted files getting 0o000; update
_build_workspace_archive to set a sensible info.mode on each TarInfo (e.g.,
default 0o644 for regular files and 0o755 for paths that look executable like
names in bin/ or ending with .sh) before archive.addfile; reference TarInfo
instances created in _build_workspace_archive and ensure the mode is set so
extract_workspace_archive_base64 doesn't chmod files to 0.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/__main__.py`:
- Around line 526-527: The current except block uses "except BaseException as
exc" which improperly catches SystemExit/KeyboardInterrupt; update the handler
to "except Exception as exc" so only regular exceptions are caught, assign the
caught exception to stream_error as before, and leave the existing finally block
(workspace download and session deletion) unchanged so cleanup still runs;
target the except block that assigns to stream_error in __main__.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bcd54fc5-15c1-48aa-a855-64a3870725ad
📒 Files selected for processing (6)
coding-agent/src/coding_agent/__main__.pycoding-agent/src/coding_agent/remote/client.pycoding-agent/src/coding_agent/workspace_archive.pycoding-agent/tests/cli/test_remote_client.pycoding-agent/tests/coding_agent/environment/test_workspace_archive.pycoding-agent/tests/ui/test_http_server_workspace_transfer.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/src/coding_agent/remote/client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/coding_agent/environment/test_workspace_archive.py
Validate tar.gz payloads before clearing the target and default missing archive modes to readable permissions so round-trips do not destroy existing workspaces or chmod files to zero. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Defer archive extraction failures until after remote session deletion and stop catching BaseException so CLI cleanup still runs without swallowing interrupts or exits. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/__main__.py`:
- Around line 543-553: The except handler for delete_remote_session is too
narrow and can clobber the original deferred_error; change the cleanup handling
so you catch any Exception from delete_remote_session (not only
click.ClickException), attach the cleanup exception as a note to stream_error if
stream_error is set, otherwise if deferred_error is set attach the cleanup
exception as a note to deferred_error and re-raise deferred_error, and only if
neither stream_error nor deferred_error exist re-raise the cleanup exception;
update the block around delete_remote_session, stream_error, and deferred_error
to preserve and re-raise the original error while still recording the cleanup
failure (reference delete_remote_session, stream_error, deferred_error and
_raise_remote_http_error to locate relevant logic).
In `@coding-agent/src/coding_agent/workspace_archive.py`:
- Around line 52-57: The except clause currently only catches tarfile.ReadError
so tarfile.HeaderError (and other tar-level errors) can escape; update the
exception handler in the archive extraction block that opens archive_buffer (the
try around tarfile.open(...) and archive.getmembers()) to catch tarfile.TarError
instead of tarfile.ReadError so all tarfile-related errors (including
HeaderError) are converted to the same ValueError("workspace archive is not a
valid tar.gz") by re-raising from the caught exception; leave the call to
_safe_archive_target and the raise ValueError(... ) from exc unchanged.
In `@coding-agent/tests/coding_agent/environment/test_workspace_archive.py`:
- Around line 62-66: Update the pytest.raises pattern in the test for
extract_workspace_archive_base64 to escape the dot so the regex matches the
literal ".gz" (use a raw string like r"valid tar\.gz" or equivalent), update the
match argument in the test function in
coding_agent/tests/coding_agent/environment/test_workspace_archive.py, and
reformat/lint the file (uv run ruff format src/) to address the RUF043 warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b24f8b46-8d80-46ef-bef7-a78d2591f72f
📒 Files selected for processing (5)
coding-agent/src/coding_agent/__main__.pycoding-agent/src/coding_agent/workspace_archive.pycoding-agent/tests/cli/test_remote_client.pycoding-agent/tests/coding_agent/environment/test_workspace_archive.pycoding-agent/tests/ui/test_http_server_workspace_transfer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/ui/test_http_server_workspace_transfer.py
Normalize tar header failures to the same invalid tar.gz error so archive validation stays stable across more corrupt tar payloads without clearing the workspace. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep remote repl reporting the primary extract failure while still recording teardown failures so cleanup noise cannot replace the user-visible root cause. 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/tests/cli/test_remote_client.py`:
- Around line 337-382: The test currently stubs download_workspace_archive to
always return "result-archive" without recording its call, so update the
monkeypatch for coding_agent.remote.client.download_workspace_archive to record
its input parameters (base_url, session_id, headers) into the existing
calls/applied list before returning "result-archive"; keep the existing
monkeypatch for coding_agent.workspace_archive.extract_workspace_archive_base64
that records (repo_path_arg, archive_base64) and ensure the recorded download
call appears in the expected sequence (before extract and before the delete
call) so the assertions validate the actual download inputs and ordering.
In `@coding-agent/tests/coding_agent/environment/test_workspace_archive.py`:
- Around line 78-99: The test currently flips archive_bytes[header_offset] using
members[0].offset which is 0 for the first entry (corrupting the gzip wrapper
instead of the tar header); change the mutation to corrupt the tar header block
by computing header_offset = members[0].offset_data - 512 (use
members[0].offset_data to locate the file's data and subtract one 512-byte
header block to get the header start) and flip a byte at that header_offset
before calling extract_workspace_archive_base64 so the tar-header failure path
is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98f36cfb-71b7-46e9-8735-6ca996817258
📒 Files selected for processing (4)
coding-agent/src/coding_agent/__main__.pycoding-agent/src/coding_agent/workspace_archive.pycoding-agent/tests/cli/test_remote_client.pycoding-agent/tests/coding_agent/environment/test_workspace_archive.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/src/coding_agent/workspace_archive.py
This was referenced May 8, 2026
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
remote repl --repoupload a local snapshot, run remotely, then download the resulting workspace back locally.Verification
uv run pytest tests/coding_agent/environment/test_workspace_archive.py tests/coding_agent/environment/test_docker_workspace_provider_transfer.py tests/cli/test_remote_client.py tests/ui/test_http_server.py tests/ui/test_http_server_workspace_transfer.py -k "workspace or remote_repl" -vuv run python -m compileall -q src/coding_agentremote repl --repo ... --goal hello, streamed output, and wrote remote changes back to the local repo.Commits
084b5c5feat: add workspace archive helpers8c197f1feat: wire docker workspace transfer1ec0d57feat: expose workspace transfer http api11d649dfeat: add remote workspace round-tripSummary by CodeRabbit
New Features
Bug Fixes
Tests