Repository navigation
fix: wire cloud workspace config - #112
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>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughAdds a workspace-provider registry and factory resolver, exposes those symbols from the environment package, wires cloud-workspace config into the HTTP server's binding resolver construction, and adds tests and test doubles covering provider registration, resolution, and validation. ChangesWorkspace Provider Registry and HTTP Server Integration
Sequence DiagramsequenceDiagram
participant Server as HTTP Server
participant Config as agent.toml Config
participant Registry as Workspace Registry
participant Provider as Workspace Provider
participant Factory as CloudClientFactory
participant Client as Cloud Workspace Client
Server->>Config: load cloud_workspace settings
Config-->>Server: returns config (enabled, provider, ...)
Server->>Registry: cloud_client_factory_from_config(config)
Registry->>Registry: validate provider name
Registry->>Provider: lookup registered provider
Provider->>Factory: build_cloud_client_factory(config)
Factory-->>Registry: return CloudClientFactory
Registry-->>Server: return CloudClientFactory
Server->>Server: inject factory into SessionManager
Note over Server,Factory: At execution time
Server->>Factory: invoke with CloudWorkspaceBinding
Factory->>Client: construct Cloud Workspace Client
Client-->>Server: client ready for operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🧹 Nitpick comments (3)
coding-agent/src/coding_agent/ui/http_server.py (2)
165-180: ⚡ Quick win
_build_session_managertests don't patch_build_binding_resolver, creating an environment-dependent test.The three
test_build_session_manager_*tests mock_load_storage_configbut not_build_binding_resolver. If a developer's localagent.tomlhascloud_workspace.enabled = truewith an unregistered provider,_build_session_manager()will raise aValueErrorfromcloud_client_factory_from_config, causing those tests to fail despite being unrelated to cloud binding.Add a
monkeypatch.setattrfor_build_binding_resolver(or_load_cloud_workspace_config) in all three tests:monkeypatch.setattr( "coding_agent.ui.http_server._build_binding_resolver", lambda: DefaultBindingResolver(), )🤖 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 165 - 180, The tests for _build_session_manager are leaking local environment because they mock _load_storage_config but not _build_binding_resolver; update each test_build_session_manager_* to monkeypatch _build_binding_resolver to return a simple resolver (e.g., DefaultBindingResolver()) so cloud provider discovery isn't executed during tests; use monkeypatch.setattr("coding_agent.ui.http_server._build_binding_resolver", lambda: DefaultBindingResolver()) (or alternatively patch _load_cloud_workspace_config) in all three tests to isolate them from local agent.toml settings.
96-121: ⚡ Quick win
_load_cloud_workspace_configis a verbatim copy of_load_storage_config, causing a doubleagent.tomlread.The two functions differ only in the config-section key (
"cloud_workspace"vs"storage") and the log message. Extracting a shared helper removes the duplication and halves the file-system reads on startup.♻️ Proposed refactor
-def _load_storage_config() -> dict[str, Any]: - config_path = Path(__file__).resolve().parent.parent / "agent.toml" - try: - return cast( - dict[str, Any], load_agent_toml(config_path).extra.get("storage", {}) - ) - except (ConfigError, OSError) as exc: - if isinstance(exc, ConfigError): - detail = exc.args[0] if exc.args and isinstance(exc.args[0], str) else "" - if not detail.startswith("config file not found:"): - raise - logger.warning( - "Unable to load storage config from %s; using defaults", - config_path, - exc_info=True, - ) - return {} - - -def _load_cloud_workspace_config() -> dict[str, Any]: - config_path = Path(__file__).resolve().parent.parent / "agent.toml" - try: - return cast( - dict[str, Any], load_agent_toml(config_path).extra.get("cloud_workspace", {}) - ) - except (ConfigError, OSError) as exc: - if isinstance(exc, ConfigError): - detail = exc.args[0] if exc.args and isinstance(exc.args[0], str) else "" - if not detail.startswith("config file not found:"): - raise - logger.warning( - "Unable to load cloud workspace config from %s; using defaults", - config_path, - exc_info=True, - ) - return {} +def _load_config_section(section: str) -> dict[str, Any]: + config_path = Path(__file__).resolve().parent.parent / "agent.toml" + try: + return cast( + dict[str, Any], load_agent_toml(config_path).extra.get(section, {}) + ) + except (ConfigError, OSError) as exc: + if isinstance(exc, ConfigError): + detail = exc.args[0] if exc.args and isinstance(exc.args[0], str) else "" + if not detail.startswith("config file not found:"): + raise + logger.warning( + "Unable to load [%s] config from %s; using defaults", + section, + config_path, + exc_info=True, + ) + return {} + + +def _load_storage_config() -> dict[str, Any]: + return _load_config_section("storage") + + +def _load_cloud_workspace_config() -> dict[str, Any]: + return _load_config_section("cloud_workspace")🤖 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 96 - 121, The two functions _load_cloud_workspace_config and _load_storage_config duplicate logic and cause agent.toml to be read twice; extract a small helper (eg. _load_agent_toml_section(section: str) -> dict[str, Any]) that takes the section key ("cloud_workspace" or "storage") and handles load_agent_toml(config_path).extra.get(...), ConfigError/OSError handling and logging, then make both _load_cloud_workspace_config and _load_storage_config call that helper; keep _build_binding_resolver behavior unchanged (it should call _load_cloud_workspace_config) and ensure cloud_client_factory_from_config is still passed the returned dict.coding-agent/tests/ui/test_http_server.py (1)
131-166: 💤 Low value
FakeCloudClientis defined but not referenced by any test in this file.It duplicates the
FakeCloudClientintest_execution_binding.py(with a differentworkspace_id). If it is needed for upcoming cloud-wiring tests in this file, consider sharing a single fixture viaconftest.pyinstead.🤖 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/ui/test_http_server.py` around lines 131 - 166, The file defines a duplicate FakeCloudClient class (FakeCloudClient with workspace_id) that isn't used; remove this duplicate and reuse the existing shared fixture instead: either import or move the canonical FakeCloudClient into a central test fixture (conftest.py) and have tests in test_http_server.py reference that fixture (or import the class) rather than defining another FakeCloudClient; ensure the shared fixture provides the same attributes/methods (workspace_id, workspace_url, default_cwd, read_file, write_file, replace_file, glob_files, grep_search, apply_patch, run_command) so tests continue to work.
🤖 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/ui/test_execution_binding.py`:
- Around line 237-241: The test's pytest.raises(..., match=...) uses unescaped
dots so "cloud_workspace.provider" and "cloud_workspace.enabled" are treated as
regex wildcards; update the assertion in test_execution_binding.py to use a
literal match by escaping the dots (e.g., "cloud_workspace\\.provider" and
"cloud_workspace\\.enabled") or build the pattern via re.escape, keeping the
call to cloud_client_factory_from_config({"enabled": True}) and the same
ValueError expectation.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 165-180: The tests for _build_session_manager are leaking local
environment because they mock _load_storage_config but not
_build_binding_resolver; update each test_build_session_manager_* to monkeypatch
_build_binding_resolver to return a simple resolver (e.g.,
DefaultBindingResolver()) so cloud provider discovery isn't executed during
tests; use
monkeypatch.setattr("coding_agent.ui.http_server._build_binding_resolver",
lambda: DefaultBindingResolver()) (or alternatively patch
_load_cloud_workspace_config) in all three tests to isolate them from local
agent.toml settings.
- Around line 96-121: The two functions _load_cloud_workspace_config and
_load_storage_config duplicate logic and cause agent.toml to be read twice;
extract a small helper (eg. _load_agent_toml_section(section: str) -> dict[str,
Any]) that takes the section key ("cloud_workspace" or "storage") and handles
load_agent_toml(config_path).extra.get(...), ConfigError/OSError handling and
logging, then make both _load_cloud_workspace_config and _load_storage_config
call that helper; keep _build_binding_resolver behavior unchanged (it should
call _load_cloud_workspace_config) and ensure cloud_client_factory_from_config
is still passed the returned dict.
In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 131-166: The file defines a duplicate FakeCloudClient class
(FakeCloudClient with workspace_id) that isn't used; remove this duplicate and
reuse the existing shared fixture instead: either import or move the canonical
FakeCloudClient into a central test fixture (conftest.py) and have tests in
test_http_server.py reference that fixture (or import the class) rather than
defining another FakeCloudClient; ensure the shared fixture provides the same
attributes/methods (workspace_id, workspace_url, default_cwd, read_file,
write_file, replace_file, glob_files, grep_search, apply_patch, run_command) so
tests continue to work.
🪄 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: 014dcba5-67a8-4d1f-ba74-5ed47744c3fe
📒 Files selected for processing (5)
coding-agent/src/coding_agent/environment/__init__.pycoding-agent/src/coding_agent/environment/workspace_provider.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/tests/ui/test_execution_binding.pycoding-agent/tests/ui/test_http_server.py
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.
🧹 Nitpick comments (1)
coding-agent/tests/ui/test_execution_binding.py (1)
196-208: 💤 Low valueMissing return type annotation on
build_cloud_client_factory.Line 200 omits the return type for
build_cloud_client_factory, leaving it as implicitlyNonein tools that rely on annotations (mypy, pyright). The method clearly returnsCallable[[CloudWorkspaceBinding], FakeCloudClient].🔧 Proposed fix
+from collections.abc import Callable + class FakeWorkspaceProvider: def __init__(self) -> None: self.seen_configs: list[dict[str, object]] = [] - def build_cloud_client_factory(self, config: dict[str, object]): + def build_cloud_client_factory( + self, config: dict[str, object] + ) -> Callable[[CloudWorkspaceBinding], FakeCloudClient]:🤖 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/ui/test_execution_binding.py` around lines 196 - 208, The method build_cloud_client_factory is missing a return type annotation; update its signature to return a callable of type Callable[[CloudWorkspaceBinding], FakeCloudClient] (i.e., add -> Callable[[CloudWorkspaceBinding], FakeCloudClient]) and ensure typing.Callable is imported if not already present; reference the build_cloud_client_factory function and the CloudWorkspaceBinding and FakeCloudClient types when making 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.
Nitpick comments:
In `@coding-agent/tests/ui/test_execution_binding.py`:
- Around line 196-208: The method build_cloud_client_factory is missing a return
type annotation; update its signature to return a callable of type
Callable[[CloudWorkspaceBinding], FakeCloudClient] (i.e., add ->
Callable[[CloudWorkspaceBinding], FakeCloudClient]) and ensure typing.Callable
is imported if not already present; reference the build_cloud_client_factory
function and the CloudWorkspaceBinding and FakeCloudClient types when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e093ed0-e7d7-473f-9eaa-e9bb6d94cab8
📒 Files selected for processing (1)
coding-agent/tests/ui/test_execution_binding.py
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This was referenced May 5, 2026
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
Verification
Summary by CodeRabbit
New Features
Tests