Add Devin CLI provider - #27
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesDevin CLI Provider Integration
CI npm caching
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Thank you for the thorough review! I've addressed all the critical issues identified: Fixed Issues1. Logic Error in ProviderManager ✅
2. Command Injection Vulnerability in tmux.py ✅
3. Null MCP Environment Variables ✅
4. Cross-Platform Encoding Issues ✅
5. Response Extraction Bug ✅
6. Duplicate E2E Test Method ✅
7. Hardcoded Timeouts in Playwright Tests ✅
8. CI Reliability - package-lock.json ✅
Testing
All critical security and functionality issues from the review have been addressed. The code is now ready for CI verification. |
|
@Amazon-Q-Developer Thank you for the review! I've fixed both critical issues:
All tests pass and CI is green. |
|
@gemini-code-assist Thank you for the thorough review! I've fixed all the critical issues you identified:
All tests pass and CI is green. |
|
Thank you for the update and for confirming that all the identified issues have been addressed. It's great to see the Devin CLI provider implementation, the security enhancements in |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summaries (23 snapshots, latest commit 2a9c8d3)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2a9c8d3)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Incremental review since commit Previous review (commit 0390876)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit 2cb9006)Status: No Issues Found | Recommendation: Merge Incremental Review NotesThe changes since the previous review (
No bugs, security issues, runtime errors, or breaking changes detected in the changed lines. Files Reviewed (9 files)
Previous review (commit db16031)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Incremental Review NotesThe changes since the previous review (
No bugs, security issues, runtime errors, or breaking changes detected in the changed lines. Previous review (commit 49a3fcc)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Incremental Review NotesThe changes since the previous review (
No bugs, security issues, runtime errors, or breaking changes detected in the changed lines. Previous review (commit b942dea)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Incremental Review NotesThe changes since the previous review (
No bugs, security issues, runtime errors, or breaking changes detected in the changed lines. Previous review (commit 8ab170c)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 47b09c3)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 1c9f2e1)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit 729b598)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Incremental Review NotesThe incremental diff (since Previous review (commit 5d963b7)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Incremental Review NotesThe incremental diff (since Previous review (commit 20bf62f)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental Review NotesThe incremental diff (since Previous review (commit 5c89e12)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental Review NotesThe incremental diff (since Previous review (commit a7f4ada)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental Review NotesThe incremental diff (since Previous review (commit f346261)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 427eaf8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 2591929)Status: No Issues Found | Recommendation: Merge Files Reviewed (incremental, previously-reviewed set)
NotesThe incremental diff (
Previous review (commit 624a2f3)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 67598f1)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (3 files)
Previous review (commit 543b1ea)Status: No Issues Found | Recommendation: Merge All 3 prior findings were re-verified against current HEAD and are resolved:
Files Reviewed (this incremental pass)
Out of scope (not in PR diff, already in base via merge from main): Previous review (commit 84f1b77)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (2 files changed since previous review)
Prior findings still open (unchanged files)
Previous review (commit a2ce5d1)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (2 files changed since previous review)
Prior findings still open (unchanged files)
Previous review (commit 9dea899)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
S[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by step-3.7-flash · Input: 46.7K · Output: 6.3K · Cached: 337K |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
CodeAnt AI is reviewing your PR. |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…xing Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…fore spec open() sinks Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…tartswith guard Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…tswith guard Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…esolved realpath Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…y findings Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…mplexity findings Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…d switch memory repair logs to logger.exception Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
… complexity Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…n render Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…cumented reasons Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
| if [[ "${DEMO_FLEET}" = "1" ]]; then | ||
| cao shutdown --session "cao-${FLEET_SESSION}" >/dev/null 2>&1 || true | ||
| fi | ||
| [[ -n "${SERVER_PID}" ]] && kill "${SERVER_PID}" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
Mock fleet session leak
cleanup() depends on cao shutdown --session "cao-${FLEET_SESSION}", but that only reaches delete_session() over HTTP, so if cao-server is already down _delete_session() fails, || true hides it, and the later kill "${SERVER_PID}" leaves the tmux session and demo fleet behind — should we add a local fallback teardown or stop swallowing the shutdown failure?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
examples/agui-dashboard/run.sh around lines 37-40 (the `if [[ "${DEMO_FLEET}" = "1" ]]`
cleanup path that runs `cao shutdown --session "cao-${FLEET_SESSION}"` and then `kill
"${SERVER_PID}"`), stop swallowing shutdown failures with `|| true` because it hides the
real cause and allows the tmux session/FIFOs to leak when the server is already dead.
Refactor the cleanup logic so that if `cao shutdown` fails (non-zero), you run a local
fallback teardown that mirrors shutdown.py’s `delete_session()` behavior: kill the
tmux session for `cao-${FLEET_SESSION}`, remove the per-session FIFOs, and clear any
per-session state; then proceed to kill `${SERVER_PID}` if set. Also, emit a clear log
message when the HTTP shutdown fails so the leak is observable in CI/dev logs.
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
There was a problem hiding this comment.
5 issues found across 38 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli_agent_orchestrator/graph/providers/memory.py">
<violation number="1" location="src/cli_agent_orchestrator/graph/providers/memory.py:61">
P3: The new cache key breaks the existing `CacheKey` type contract, so mypy now reports an argument-type error for this provider. Updating `CacheKey` and the cache's key annotations/tests to the four-part `(base_dir, provider, scope, scope_id)` shape would preserve the isolation change without leaving the type checker inconsistent.</violation>
</file>
<file name="src/cli_agent_orchestrator/providers/antigravity_cli.py">
<violation number="1" location="src/cli_agent_orchestrator/providers/antigravity_cli.py:403">
P3: The suppression rationale says `if/elif`, but this method uses independent `if`/`continue` branches; that mismatch can mislead future maintenance of the startup-dialog loop. A shorter rationale that describes the actual dismissal loop would keep the `NOSONAR` justification accurate.</violation>
</file>
<file name="src/cli_agent_orchestrator/providers/claude_code.py">
<violation number="1" location="src/cli_agent_orchestrator/providers/claude_code.py:200">
P2: The Sonar suppression is attached to the closing return-annotation line rather than the `def` line, so `_build_claude_command`'s cognitive-complexity finding remains unsuppressed and can keep the quality gate failing. Moving the comment to `def _build_claude_command(` would make the suppression effective.</violation>
</file>
<file name="web/src/graph/buildGraph.ts">
<violation number="1" location="web/src/graph/buildGraph.ts:34">
P2: Contradictions are hidden whenever the same topic pair already has a `relates_to` edge: the provider emits the related edge first, and this guard skips the later contradiction without updating the existing edge color. Preserving the single edge while changing its color to `CONTRADICTION_COLOR` when the duplicate is a contradiction would keep the graph's contradiction styling accurate.</violation>
</file>
<file name="test/services/test_script_runner.py">
<violation number="1" location="test/services/test_script_runner.py:719">
P3: Missing `# NOSONAR` on the dict line containing the hardcoded path `/tmp/wf.py`. SonarCloud's NOSONAR comment suppresses issues only on the line where it appears, so the hardcoded file-path rule would still fire on this line — the NOSONAR on the closing `),` line is not enough. The other three similar occurrences in this file correctly place the NOSONAR comment on both the dict line and the closing line; this one should match.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| def _build_claude_command(self, profile: Optional["AgentProfile"] = _UNSET) -> str: | ||
| def _build_claude_command( | ||
| self, profile: Optional["AgentProfile"] = _UNSET | ||
| ) -> str: # NOSONAR -- command routing is intentionally branched |
There was a problem hiding this comment.
P2: The Sonar suppression is attached to the closing return-annotation line rather than the def line, so _build_claude_command's cognitive-complexity finding remains unsuppressed and can keep the quality gate failing. Moving the comment to def _build_claude_command( would make the suppression effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli_agent_orchestrator/providers/claude_code.py, line 200:
<comment>The Sonar suppression is attached to the closing return-annotation line rather than the `def` line, so `_build_claude_command`'s cognitive-complexity finding remains unsuppressed and can keep the quality gate failing. Moving the comment to `def _build_claude_command(` would make the suppression effective.</comment>
<file context>
@@ -195,7 +195,9 @@ def _load_profile(self) -> Optional["AgentProfile"]:
- def _build_claude_command(self, profile: Optional["AgentProfile"] = _UNSET) -> str:
+ def _build_claude_command(
+ self, profile: Optional["AgentProfile"] = _UNSET
+ ) -> str: # NOSONAR -- command routing is intentionally branched
"""Build Claude Code command with agent profile if provided.
</file context>
|
|
||
| for (const edge of view.edges) { | ||
| if (!graph.hasNode(edge.source) || !graph.hasNode(edge.target)) continue; | ||
| if (graph.hasEdge(edge.source, edge.target)) continue; |
There was a problem hiding this comment.
P2: Contradictions are hidden whenever the same topic pair already has a relates_to edge: the provider emits the related edge first, and this guard skips the later contradiction without updating the existing edge color. Preserving the single edge while changing its color to CONTRADICTION_COLOR when the duplicate is a contradiction would keep the graph's contradiction styling accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/graph/buildGraph.ts, line 34:
<comment>Contradictions are hidden whenever the same topic pair already has a `relates_to` edge: the provider emits the related edge first, and this guard skips the later contradiction without updating the existing edge color. Preserving the single edge while changing its color to `CONTRADICTION_COLOR` when the duplicate is a contradiction would keep the graph's contradiction styling accurate.</comment>
<file context>
@@ -0,0 +1,45 @@
+
+ for (const edge of view.edges) {
+ if (!graph.hasNode(edge.source) || !graph.hasNode(edge.target)) continue;
+ if (graph.hasEdge(edge.source, edge.target)) continue;
+ graph.addEdge(edge.source, edge.target, {
+ color:
</file context>
| scope_id: Optional[str] = None if raw_scope_id is None else str(raw_scope_id) | ||
|
|
||
| key = ("memory", scope, scope_id) | ||
| key = (str(self._svc.base_dir), "memory", scope, scope_id) |
There was a problem hiding this comment.
P3: The new cache key breaks the existing CacheKey type contract, so mypy now reports an argument-type error for this provider. Updating CacheKey and the cache's key annotations/tests to the four-part (base_dir, provider, scope, scope_id) shape would preserve the isolation change without leaving the type checker inconsistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli_agent_orchestrator/graph/providers/memory.py, line 61:
<comment>The new cache key breaks the existing `CacheKey` type contract, so mypy now reports an argument-type error for this provider. Updating `CacheKey` and the cache's key annotations/tests to the four-part `(base_dir, provider, scope, scope_id)` shape would preserve the isolation change without leaving the type checker inconsistent.</comment>
<file context>
@@ -57,7 +58,7 @@ async def project(self, **filters: Any) -> GraphView:
scope_id: Optional[str] = None if raw_scope_id is None else str(raw_scope_id)
- key = ("memory", scope, scope_id)
+ key = (str(self._svc.base_dir), "memory", scope, scope_id)
view, cached, as_of = await _CACHE.get_or_build(key, lambda: self._build(scope, scope_id))
# Re-wrap with fresh cache provenance without mutating the cached
</file context>
| self._mcp_server_names = [] | ||
|
|
||
| def _handle_startup_dialog( | ||
| def _handle_startup_dialog( # NOSONAR -- startup dialog dismissal loop; sequential if/elif branches handle trust, survey, and ready footer. |
There was a problem hiding this comment.
P3: The suppression rationale says if/elif, but this method uses independent if/continue branches; that mismatch can mislead future maintenance of the startup-dialog loop. A shorter rationale that describes the actual dismissal loop would keep the NOSONAR justification accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli_agent_orchestrator/providers/antigravity_cli.py, line 403:
<comment>The suppression rationale says `if/elif`, but this method uses independent `if`/`continue` branches; that mismatch can mislead future maintenance of the startup-dialog loop. A shorter rationale that describes the actual dismissal loop would keep the `NOSONAR` justification accurate.</comment>
<file context>
@@ -400,7 +400,7 @@ def _unregister_mcp_servers(self) -> None:
self._mcp_server_names = []
- def _handle_startup_dialog(
+ def _handle_startup_dialog( # NOSONAR -- startup dialog dismissal loop; sequential if/elif branches handle trust, survey, and ready footer.
self, idle_gap: Optional[float] = None, outer_timeout: Optional[float] = None
) -> None:
</file context>
| def _handle_startup_dialog( # NOSONAR -- startup dialog dismissal loop; sequential if/elif branches handle trust, survey, and ready footer. | |
| def _handle_startup_dialog( # NOSONAR -- startup dialog dismissal loop. |
| workflow_name="wf", | ||
| spec_snapshot=json.dumps({"source": source, "path": "/tmp/wf.py"}), | ||
| spec_snapshot=json.dumps( | ||
| {"source": source, "path": "/tmp/wf.py"} |
There was a problem hiding this comment.
P3: Missing # NOSONAR on the dict line containing the hardcoded path /tmp/wf.py. SonarCloud's NOSONAR comment suppresses issues only on the line where it appears, so the hardcoded file-path rule would still fire on this line — the NOSONAR on the closing ), line is not enough. The other three similar occurrences in this file correctly place the NOSONAR comment on both the dict line and the closing line; this one should match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/services/test_script_runner.py, line 719:
<comment>Missing `# NOSONAR` on the dict line containing the hardcoded path `/tmp/wf.py`. SonarCloud's NOSONAR comment suppresses issues only on the line where it appears, so the hardcoded file-path rule would still fire on this line — the NOSONAR on the closing `),` line is not enough. The other three similar occurrences in this file correctly place the NOSONAR comment on both the dict line and the closing line; this one should match.</comment>
<file context>
@@ -715,7 +715,9 @@ async def test_resume_happy_materializes_and_deletes_temp(monkeypatch: pytest.Mo
workflow_name="wf",
- spec_snapshot=json.dumps({"source": source, "path": "/tmp/wf.py"}),
+ spec_snapshot=json.dumps(
+ {"source": source, "path": "/tmp/wf.py"}
+ ), # NOSONAR -- test fixture path
inputs_json="{}",
</file context>
| {"source": source, "path": "/tmp/wf.py"} | |
| {"source": source, "path": "/tmp/wf.py"} # NOSONAR -- test fixture path |
|
Closing in favor of awslabs#465. Original branch state is preserved in origin/fix/devin-cli-provider-backup. |
User description
User description
**
**
Upstream PR: awslabs#336
Add Devin CLI provider implementation with unit tests and registration in all required locations.
What Changed
DevinCliProviderwith prompt/status parsing and/exithandling--config, launchingcao-mcp-serverand passingCAO_TERMINAL_IDagent_profilesystem prompts and softallowed_toolsenforcement via a prepended security promptdevin_cliacross the app:ProviderType, provider manager factory,api/main.pyproviders list,launchworkspace-access list, settings agent dirs, agent profile listing, andtool_mapping(Bash/Read/Write)devin_cliTrue End-to-End Testing with Devcontainer
The Problem
WSL has tmux limitations (
[Errno 95] Operation not supported) that prevent real Devin CLI spawns. Unit and API tests pass, but actual agent spawning cannot be tested in WSL.The Solution: Devcontainer on Host Machine
A
.devcontainer/configuration that:~/.config/devin/How to Use for True E2E Testing
Prerequisites:
~/.config/devin/Steps:
Why This Works:
Alternative: CI Testing
If you don't have Docker on your host machine, the CI pipeline (which runs on real Linux) will perform the true end-to-end testing of Devin CLI spawns.
Web UI E2E Tests - Why They Matter
What They Test
These E2E tests verify the complete user journey through the CAO web interface:
How They Help vs Existing Tests
Existing Unit Tests:
Existing API Tests:
NEW E2E Tests:
Example Issues E2E Tests Catch
Trade-offs
Overall, E2E tests complement unit and API tests by providing confidence that the complete user experience works correctly, not just individual components in isolation.
Bug Fixes
#) as input prompts; terminate on horizontal rules/status bar onlySummary by CodeRabbit
devin_cli) provider support end-to-end, including orchestration, agent profiles, tool restrictions, and workspace confirmation.Generated description
Below is a concise technical summary of the changes proposed in this PR:
Add
DevinCliProviderand wire it into provider selection, launch/session handling, terminal input delivery, status parsing, and tool restrictions throughProviderManager,TerminalService, andMcpApp. Extend the web UI and test harness with Devin provider listings, graph/session updates, and end-to-end coverage for the new CLI flow.Modified files (9)
Latest Contributors(2)
Modified files (37)
Latest Contributors(2)
DevinCliProviderand wire it into provider selection, launch/session handling, terminal input delivery, status parsing, and tool restrictions.Modified files (29)
Latest Contributors(2)