feat(providers): add Devin CLI provider - #465
Conversation
… implementation, registration, and tests.\nIncludes devin-specific status detection, paste-buffer control support,\nand E2E supervisor orchestration scaffolding. Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…vice Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…r, clean temp files, resolve native status, tighten auth regex Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…ONAR) Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
21b0f62 to
370bf6d
Compare
|
Thanks for the review feedback on the earlier iteration (ThePlenkov#27). This clean, rebased branch incorporates the main points raised there:
This PR is rebased on the latest |
- tmux: log keys length, not payload content, in send-keys path - tmux: use '--' separator before send-keys literal payload - devin_cli: resolve init timeout from profile/server settings - devin_cli: translate temp prompt/config paths for container profiles - provider manager: restore allowed_tools when recreating a provider - tool_mapping: align devin_cli native tool names with Devin docs - docs: add required profile install steps for Devin E2E tests - tests: update tmux send-keys assertion and add devin binary field check Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- devin_cli: normalize profile MCP server entries to Devin's CLI config-file schema - devin_cli: keep temp file paths when deletion fails so cleanup can retry - devin_cli: always clean up temp files in initialize() finally block - devin_cli: evaluate error patterns before the welcome banner in get_status() - devin_cli: scope processing detection to the last viewport lines/start-of-line - devin_cli: only treat a horizontal rule as response terminator when followed by the # prompt - e2e tests: add timeout to requests.post in supervisor orchestration test - README: add Devin CLI to the supported provider tables Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- tool_mapping: remove exec from devin_cli fs_* wildcard so file-system scope no longer grants shell - database: preserve empty allowed_tools list instead of collapsing it to NULL on insert - devin_cli: cache agent profile load and reuse it for command building and init timeout - devin_cli: use tempfile.mkstemp/os.fdopen for prompt/config temp files and always remove on write failure Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- manager: type-annotate **kwargs with Any for all provider factories - e2e test: wait for Devin CLI to leave the initial ready state after input before polling for completion Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…s registry Use inspect.signature to dispatch only the constructor arguments each provider accepts, eliminating 11 near-identical _create_*_provider methods. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- _normalize_mcp_server_for_devin: keep oauthClientId, oauthClientSecret, and oauthResource (including empty string) for remote MCP servers - _write_prompt_file/_write_config_file: clear fd immediately inside the os.fdopen context so a write failure does not double-close the fd and mask the original exception Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ction CAO_HOME_DIR is hardcoded under the real home directory, so suggesting CAO_HOME could give a false sense of isolation. Advise backing up the agent-store instead. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- get_status: evaluate _is_error() immediately after building lines, before spinner/prompt/completed checks, so crash output is not masked - e2e _wait_for_status_change: require either a PROCESSING status or a visible output change from the pre-task baseline before declaring that Devin CLI started work Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- get_status: keep processing detection before the # prompt (matches unit tests and the Devin TUI where a spinner can appear above the prompt), then error detection, so completed replies that quote errors are not misreported as ERROR - add shared _write_temp_file helper used by _write_config_file and _write_prompt_file - e2e _wait_for_status_change: compare output change before the status gate so fast turns are not missed between polls Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Replace _has_input_prompt with _find_last_input_prompt so get_status can check whether an explicit error/crash appears after the prompt. This prevents a stale prompt from masking a crash while still treating completed responses that merely mention an error as COMPLETED. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
mkstemp is already restrictive on POSIX, but enforce 0o600 explicitly for portability and to address the review concern about temp file permissions. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
call-me-ram
left a comment
There was a problem hiding this comment.
First review — apologies this sat since 07-19. The provider scaffolding follows the cao-provider checklist closely, the temp-file handling is careful (explicit chmod 0o600 in the latest commit is the right instinct), and the e2e TestDevinCliSupervisorOrchestration with a require_devin fixture tells me this was calibrated against the real binary rather than guessed. Three blocking items, all of which I reproduced.
[must-fix] The new use_paste_buffer=False literal-keys path hard-fails above ~16 KB. tmux.py:294 passes the whole message as one argv element to tmux send-keys -l -- <keys> with check=True. tmux caps a single command message. Measured on this host (tmux 3.4):
8000 bytes: OK
16000 bytes: OK
16500 bytes: FAIL -> command too long
20000 bytes: FAIL -> command too long
50000 bytes: FAIL -> command too long
For scale: the repo's own test/clients/test_tmux_send_keys.py:163 exercises a 50,000-byte message through the paste-buffer path, so messages well past this ceiling are an expected size here. A handoff prompt or a pasted diff will blow through 16 KB routinely, and check=True turns it into a CalledProcessError rather than a degraded send. Needs chunking, or a fallback to the buffer path above a threshold.
[must-fix] The branch reverts two merged tmux fixes. clients/tmux.py here is the pre-#430 implementation — it unconditionally hand-crafts \x1b[200~ … \x1b[201~ (tmux.py:328). On main that was replaced by a tmux-version gate (base-main tmux.py:251-260, "tmux >= 3.7 passes pasted buffer content through vis(3) sanitization"), because on tmux >= 3.7 the hand-crafted markers render as literal ^[[200~ garbage in the pane. #500 touched the same function. The branch is 31 commits behind main and DIRTY with conflicts in 4 files, so this will surface as a merge conflict — please rebase and keep main's version-gated send_keys rather than resolving in favour of this branch's copy.
[must-fix] devin_cli is in TOOL_MAPPING but missing from SOFT_ENFORCEMENT_PROVIDERS. tool_mapping.py:54 registers a devin entry, but terminal_service.py:140-144 lists only KIMI_CLI, CODEX, ANTIGRAVITY_CLI. Devin's restrictions here are advisory only — _build_security_constraint() writes prompt text and _build_command() never emits a native deny flag — so without membership in that set, the provider in SOFT_ENFORCEMENT_PROVIDERS branch at terminal_service.py:287 never fires and cao launch presents prompt-level restrictions as if they were enforced. That is a security-affordance mismatch, and it is a one-line fix.
Withdrawing a finding I had drafted, because it does not survive scrutiny. I was going to flag that DevinCliProvider._clean() (devin_cli.py:124) normalizes \r but not cursor motion (CUP/CHA/CUU), and that it should use the shared utils.text.strip_terminal_escapes like other providers. On checking, copilot_cli._clean() on upstream main is the same implementation — \r\n/\r → \n, then OSC, ANSI, control chars, no cursor normalization — and it ships against a similarly bottom-anchored TUI (status bar below the prompt). So this mirrors accepted upstream practice rather than deviating from it, and I have no real Devin capture showing the failure. Reducing to an optional suggestion: consider strip_terminal_escapes for robustness. But docs/devin-cli.md should be corrected regardless — it says _clean() strips things "so redraws and cursor-motion don't glue the prompt onto a previous line", and cursor motion is precisely what it does not handle.
Should-fix, none blocking on their own:
submit_delayis silently ignored on the new path. Theif not use_paste_buffer:branch returns beforetime.sleep(submit_delay)(tmux.py:347), so the valueterminal_service.send_inputresolves fromprovider.paste_submit_delayhas no effect for Devin. Given #517 just landed for exactly this class of bug on antigravity, worth wiring up.- Container path translation is a no-op for the provider's own temp files.
_translate_path()is applied to the--prompt-file/--configarguments, but_write_temp_filecreates them viatempfile.mkstemp()in the host temp dir, which is not covered bycontainer.path_maps— a containerized Devin profile gets an unreadable path. - Restricted profiles launch in Devin's default permission mode.
--permission-mode dangerous --respect-workspace-trust falseis added only whenallowed_toolscontains*(devin_cli.py:281-289), and there is noWAITING_USER_ANSWERdetection path, so a restricted profile can sit on an unanswered permission prompt with nothing to notice it. - Please split the unrelated refactors out. Commit
cc636c3rewritesProviderManagerto reflection-based construction (inspect.signature) for all 11 providers, and81403cfchangesclients/database.py:544insert semantics. Both are plausible changes, but they put every existing provider at risk inside a PR whose stated scope is "add a new provider" — and they are the reason this PR needs a much more careful merge than a new-provider PR normally would. - The test suite pins very little of what you iterated on. Mutation testing suggests 4 of 5 targeted behaviour changes survive with the suite green — including the horizontal-rule guard in
_find_last_input_prompt, whose own docstring explains the false positive it exists to prevent. Worth adding assertions for the specific cases you hit during those 21 commits.
Nits: docs/devin-cli.md contradicts the implementation in three places; herdr log statements lost their session/window/pane identifiers; README.md loses its trailing newline; CHANGELOG.md not updated.
The provider itself looks well-built — the blocking items are mostly about the delivery-path changes it carries alongside. Rebase onto current main (keeping main's send_keys), fix the size ceiling and the enforcement-set entry, and I think this moves quickly.
Summary
Add a dedicated
devin_cliprovider so the orchestrator can drive the Devin CLI inside a tmux pane, alongside the existing Kiro / Claude Code / Codex providers.This is a clean, focused replacement for #336. It contains only the Devin-provider-related changes and is rebased on the latest
main.What changed
src/cli_agent_orchestrator/providers/devin_cli.py— newDevinCliProviderimplementingBaseProvider:IDLE,PROCESSING,COMPLETED,WAITING_USER_ANSWER,ERROR,UNKNOWN) from ANSI-stripped pane output using Devin-specific regex patterns.>user-input line and the trailing horizontal rule / status bar.devinlaunch command with optional--agent-profile,--allowed-tools, model override, and an injected security prompt.CAO_TERMINAL_IDforwarding.src/cli_agent_orchestrator/models/provider.py— addDEVIN_CLItoProviderType.src/cli_agent_orchestrator/providers/manager.py— registerDevinCliProviderin the provider factory.src/cli_agent_orchestrator/utils/tool_mapping.py— register Devin's tool vocabulary (Bash,Read,Write,list,grep).src/cli_agent_orchestrator/services/settings_service.py— add default agent directory fordevin_cli.src/cli_agent_orchestrator/utils/agent_profiles.py— adddevin_cli -> devinsource label.src/cli_agent_orchestrator/cli/commands/launch.py— adddevin_clito providers requiring workspace access.src/cli_agent_orchestrator/api/main.py— exposedevinbinary inGET /agents/providers.src/cli_agent_orchestrator/clients/tmux.py— adduse_paste_buffer=Falsefallback tosend-keys -landshlex.quotethepipe-panefile path.docs/devin-cli.md— provider usage documentation.test/providers/test_devin_cli_unit.py+ fixtures — unit tests for status detection, response extraction, tool restrictions, and registration.test/clients/test_tmux_send_keys.py— add test foruse_paste_buffer=False.test/api/test_api_endpoints.py— update provider count and assertdevin_cliis listed.test/e2e/conftest.py/test/e2e/test_supervisor_orchestration.py— addrequire_devinfixture and E2E supervisor orchestration scaffolding for Devin.Why
Devin uses a fixed
#prompt, a>user-input prefix, and aMode: ... Model: ...status bar. The existing generic regex patterns do not match these markers, so a dedicated provider is needed for reliable status detection and response extraction.The paste-buffer fallback is required because Devin's input prompt does not accept tmux's bracketed-paste sequence; sending keys literally via
send-keys -lis the supported path.