Repository navigation
feat: add remote workspace operations API - #127
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extends the cloud workspace platform with inventory, cleanup, and archive-export capabilities. It introduces workspace status and archive metadata dataclasses, adds five new protocol methods to ChangesCloud Workspace Management Lifecycle
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. 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: 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/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 800-836: Wrap filesystem stat()/access operations to tolerate
concurrent deletions: in _docker_workspace_entry, catch
FileNotFoundError/OSError around workspace_root.stat() (used for mtime checks
and updated_at) and return a sentinel (or raise a custom skip) so callers can
skip the entry; in _list_docker_workspaces, call _docker_workspace_entry inside
a try/except (catching FileNotFoundError/OSError) and continue (skip) when a
workspace disappears between iteration and stat. Ensure you only skip the
problematic workspace and do not let the exception bubble up.
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 1368-1409: The handlers get workspace and cleanup_workspace
currently map KeyError to 404 using str(exc) and let ValueError bubble to 500;
change both to: catch ValueError from get_cloud_workspace_from_config and
cleanup_cloud_workspace_from_config and raise HTTPException(status_code=400,
detail=str(exc)) to indicate malformed client input, replace raising 404 with
HTTPException(status_code=404, detail=_key_error_detail(exc)) so KeyError uses
_key_error_detail instead of str(exc), and ensure these changes are applied in
the async try/except blocks in the get handler (where
get_cloud_workspace_from_config is called) and in cleanup_workspace (where
cleanup_cloud_workspace_from_config is called).
🪄 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: 966e311c-58aa-4357-8807-8549ec0ffc54
📒 Files selected for processing (7)
coding-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/ui/http_server.pycoding-agent/src/coding_agent/ui/schemas.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server_workspace_transfer.py
Comment on lines
+800
to
+836
| def _list_docker_workspaces( | ||
| provider_config: _DockerWorkspaceProviderConfig, | ||
| active_workspace_ids: set[str], | ||
| ) -> list[WorkspaceInventoryEntry]: | ||
| if not provider_config.workspace_root.exists(): | ||
| return [] | ||
| entries: list[WorkspaceInventoryEntry] = [] | ||
| for path in sorted( | ||
| provider_config.workspace_root.iterdir(), key=lambda item: item.name | ||
| ): | ||
| if not path.is_dir() or not _is_provider_workspace_id(path.name): | ||
| continue | ||
| entries.append( | ||
| _docker_workspace_entry(provider_config, path, active_workspace_ids) | ||
| ) | ||
| return entries | ||
|
|
||
|
|
||
| def _docker_workspace_entry( | ||
| provider_config: _DockerWorkspaceProviderConfig, | ||
| workspace_root: Path, | ||
| active_workspace_ids: set[str], | ||
| ) -> WorkspaceInventoryEntry: | ||
| workspace_id = workspace_root.name | ||
| status: WorkspaceStatus = "active" | ||
| if workspace_id not in active_workspace_ids: | ||
| max_age = provider_config.max_workspace_age_seconds | ||
| if ( | ||
| max_age is not None | ||
| and workspace_root.stat().st_mtime < time.time() - max_age | ||
| ): | ||
| status = "stale" | ||
| return WorkspaceInventoryEntry( | ||
| workspace_id=workspace_id, | ||
| status=status, | ||
| updated_at=datetime.fromtimestamp(workspace_root.stat().st_mtime, UTC), | ||
| ) |
Contributor
There was a problem hiding this comment.
Handle concurrent workspace deletion during inventory listing.
At Line 807 and Line 829/835, stat() can fail if another request cleans a workspace concurrently, causing the entire listing to error instead of skipping that workspace.
Proposed fix
def _list_docker_workspaces(
provider_config: _DockerWorkspaceProviderConfig,
active_workspace_ids: set[str],
) -> list[WorkspaceInventoryEntry]:
@@
for path in sorted(
provider_config.workspace_root.iterdir(), key=lambda item: item.name
):
if not path.is_dir() or not _is_provider_workspace_id(path.name):
continue
- entries.append(
- _docker_workspace_entry(provider_config, path, active_workspace_ids)
- )
+ try:
+ entries.append(
+ _docker_workspace_entry(provider_config, path, active_workspace_ids)
+ )
+ except FileNotFoundError:
+ continue
return entries
@@
def _docker_workspace_entry(
@@
) -> WorkspaceInventoryEntry:
workspace_id = workspace_root.name
+ stat_result = workspace_root.stat()
status: WorkspaceStatus = "active"
if workspace_id not in active_workspace_ids:
max_age = provider_config.max_workspace_age_seconds
if (
max_age is not None
- and workspace_root.stat().st_mtime < time.time() - max_age
+ and stat_result.st_mtime < time.time() - max_age
):
status = "stale"
return WorkspaceInventoryEntry(
workspace_id=workspace_id,
status=status,
- updated_at=datetime.fromtimestamp(workspace_root.stat().st_mtime, UTC),
+ updated_at=datetime.fromtimestamp(stat_result.st_mtime, UTC),
)🤖 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 800 - 836, Wrap filesystem stat()/access operations to tolerate
concurrent deletions: in _docker_workspace_entry, catch
FileNotFoundError/OSError around workspace_root.stat() (used for mtime checks
and updated_at) and return a sentinel (or raise a custom skip) so callers can
skip the entry; in _list_docker_workspaces, call _docker_workspace_entry inside
a try/except (catching FileNotFoundError/OSError) and continue (skip) when a
workspace disappears between iteration and stat. Ensure you only skip the
problematic workspace and do not let the exception bubble up.
Comment on lines
+1368
to
+1409
| try: | ||
| entry = await asyncio.to_thread( | ||
| get_cloud_workspace_from_config, | ||
| _load_cloud_workspace_config(), | ||
| workspace_id, | ||
| active_workspace_ids=await _active_cloud_workspace_ids(), | ||
| ) | ||
| except KeyError as exc: | ||
| raise HTTPException(status_code=404, detail=str(exc)) from exc | ||
| return _workspace_summary_response(entry) | ||
|
|
||
|
|
||
| @app.delete("/workspaces/{workspace_id}", response_model=WorkspaceCleanupResponse) | ||
| @limiter.limit(RateLimits.CLOSE_SESSION) | ||
| async def cleanup_workspace( | ||
| request: Request, | ||
| response: Response, | ||
| workspace_id: str, | ||
| auth_context: AuthContext | None = Depends(auth_context_from_headers), | ||
| ) -> WorkspaceCleanupResponse: | ||
| del request | ||
| _require_admin_context(auth_context) | ||
| try: | ||
| entry = await asyncio.to_thread( | ||
| cleanup_cloud_workspace_from_config, | ||
| _load_cloud_workspace_config(), | ||
| workspace_id, | ||
| active_workspace_ids=await _active_cloud_workspace_ids(), | ||
| ) | ||
| except KeyError as exc: | ||
| raise HTTPException(status_code=404, detail=str(exc)) from exc | ||
| except RuntimeError as exc: | ||
| raise HTTPException(status_code=409, detail=str(exc)) from exc | ||
| except Exception as exc: | ||
| logger.exception("Cloud workspace cleanup failed workspace_id=%s", workspace_id) | ||
| response.status_code = 500 | ||
| return WorkspaceCleanupResponse( | ||
| workspace_id=workspace_id, | ||
| status="cleanup_failed", | ||
| error=str(exc), | ||
| ) | ||
| return WorkspaceCleanupResponse(workspace_id=entry.workspace_id, status="cleaned") |
Contributor
There was a problem hiding this comment.
Map invalid workspace_id input to 400 (not 500) on workspace get/cleanup.
At Line 1369 and Line 1391, malformed IDs can raise ValueError from provider validation; today that falls through to 500 paths. This should be a client error (400). Also prefer _key_error_detail(exc) over str(exc) for clean 404 details.
Proposed fix
async def get_workspace(
@@
try:
entry = await asyncio.to_thread(
get_cloud_workspace_from_config,
_load_cloud_workspace_config(),
workspace_id,
active_workspace_ids=await _active_cloud_workspace_ids(),
)
except KeyError as exc:
- raise HTTPException(status_code=404, detail=str(exc)) from exc
+ raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc
+ except (ValueError, TypeError) as exc:
+ raise HTTPException(status_code=400, detail=str(exc)) from exc
return _workspace_summary_response(entry)
@@
async def cleanup_workspace(
@@
try:
entry = await asyncio.to_thread(
cleanup_cloud_workspace_from_config,
_load_cloud_workspace_config(),
workspace_id,
active_workspace_ids=await _active_cloud_workspace_ids(),
)
except KeyError as exc:
- raise HTTPException(status_code=404, detail=str(exc)) from exc
+ raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc
+ except (ValueError, TypeError) as exc:
+ raise HTTPException(status_code=400, detail=str(exc)) from exc
except RuntimeError as exc:
raise HTTPException(status_code=409, detail=str(exc)) from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| entry = await asyncio.to_thread( | |
| get_cloud_workspace_from_config, | |
| _load_cloud_workspace_config(), | |
| workspace_id, | |
| active_workspace_ids=await _active_cloud_workspace_ids(), | |
| ) | |
| except KeyError as exc: | |
| raise HTTPException(status_code=404, detail=str(exc)) from exc | |
| return _workspace_summary_response(entry) | |
| @app.delete("/workspaces/{workspace_id}", response_model=WorkspaceCleanupResponse) | |
| @limiter.limit(RateLimits.CLOSE_SESSION) | |
| async def cleanup_workspace( | |
| request: Request, | |
| response: Response, | |
| workspace_id: str, | |
| auth_context: AuthContext | None = Depends(auth_context_from_headers), | |
| ) -> WorkspaceCleanupResponse: | |
| del request | |
| _require_admin_context(auth_context) | |
| try: | |
| entry = await asyncio.to_thread( | |
| cleanup_cloud_workspace_from_config, | |
| _load_cloud_workspace_config(), | |
| workspace_id, | |
| active_workspace_ids=await _active_cloud_workspace_ids(), | |
| ) | |
| except KeyError as exc: | |
| raise HTTPException(status_code=404, detail=str(exc)) from exc | |
| except RuntimeError as exc: | |
| raise HTTPException(status_code=409, detail=str(exc)) from exc | |
| except Exception as exc: | |
| logger.exception("Cloud workspace cleanup failed workspace_id=%s", workspace_id) | |
| response.status_code = 500 | |
| return WorkspaceCleanupResponse( | |
| workspace_id=workspace_id, | |
| status="cleanup_failed", | |
| error=str(exc), | |
| ) | |
| return WorkspaceCleanupResponse(workspace_id=entry.workspace_id, status="cleaned") | |
| try: | |
| entry = await asyncio.to_thread( | |
| get_cloud_workspace_from_config, | |
| _load_cloud_workspace_config(), | |
| workspace_id, | |
| active_workspace_ids=await _active_cloud_workspace_ids(), | |
| ) | |
| except KeyError as exc: | |
| raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc | |
| except (ValueError, TypeError) as exc: | |
| raise HTTPException(status_code=400, detail=str(exc)) from exc | |
| return _workspace_summary_response(entry) | |
| `@app.delete`("/workspaces/{workspace_id}", response_model=WorkspaceCleanupResponse) | |
| `@limiter.limit`(RateLimits.CLOSE_SESSION) | |
| async def cleanup_workspace( | |
| request: Request, | |
| response: Response, | |
| workspace_id: str, | |
| auth_context: AuthContext | None = Depends(auth_context_from_headers), | |
| ) -> WorkspaceCleanupResponse: | |
| del request | |
| _require_admin_context(auth_context) | |
| try: | |
| entry = await asyncio.to_thread( | |
| cleanup_cloud_workspace_from_config, | |
| _load_cloud_workspace_config(), | |
| workspace_id, | |
| active_workspace_ids=await _active_cloud_workspace_ids(), | |
| ) | |
| except KeyError as exc: | |
| raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc | |
| except (ValueError, TypeError) as exc: | |
| raise HTTPException(status_code=400, detail=str(exc)) from exc | |
| except RuntimeError as exc: | |
| raise HTTPException(status_code=409, detail=str(exc)) from exc | |
| except Exception as exc: | |
| logger.exception("Cloud workspace cleanup failed workspace_id=%s", workspace_id) | |
| response.status_code = 500 | |
| return WorkspaceCleanupResponse( | |
| workspace_id=workspace_id, | |
| status="cleanup_failed", | |
| error=str(exc), | |
| ) | |
| return WorkspaceCleanupResponse(workspace_id=entry.workspace_id, status="cleaned") |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 1386-1386: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 1368 - 1409,
The handlers get workspace and cleanup_workspace currently map KeyError to 404
using str(exc) and let ValueError bubble to 500; change both to: catch
ValueError from get_cloud_workspace_from_config and
cleanup_cloud_workspace_from_config and raise HTTPException(status_code=400,
detail=str(exc)) to indicate malformed client input, replace raising 404 with
HTTPException(status_code=404, detail=_key_error_detail(exc)) so KeyError uses
_key_error_detail instead of str(exc), and ensure these changes are applied in
the async try/except blocks in the get handler (where
get_cloud_workspace_from_config is called) and in cleanup_workspace (where
cleanup_cloud_workspace_from_config is called).
This was referenced May 10, 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
Validation
Summary by CodeRabbit
Release Notes
New Features
Tests