Skip to content

feat: add provider_cli agent type and related functionality - #459

Merged
pikann merged 4 commits into
masterfrom
feature/add-provider-cli-agent-type
Sep 4, 2026
Merged

feat: add provider_cli agent type and related functionality#459
pikann merged 4 commits into
masterfrom
feature/add-provider-cli-agent-type

Conversation

@pikann

@pikann pikann commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a new provider_cli agent type that runs Claude Code, Codex, Cursor Agent, or Gemini CLI directly (via Goose's own CLI/ACP provider integrations) inside a Paca-hosted static environment, instead of Paca calling a model API itself.

Core feature

  • New agent_type = 'provider_cli', with cli_provider/cli_model/cli_auth_mode/cli_api_key_secret/cli_login_verified_at fields on agents (migration 000049_add_provider_cli_agents.sql).
  • internal/executor/providercli: one adapter per CLI (Claude Code, Codex, Cursor Agent, Gemini CLI) that syncs an agent's configured MCP servers (and, for Claude Code, its skills) into that CLI's own on-disk config, and exposes each CLI's real non-interactive auth-status check (claude auth status, codex login status, cursor-agent status --format json).
  • VerifyCLIAuth/VerifyCLILogin: agent-scoped and environment-scoped HTTP endpoints to check whether a provider_cli agent's CLI is currently logged in — the environment-scoped one lets the create-agent dialog verify login before the agent even exists.
  • provider_cli agents require a default static environment (never fall back to an ephemeral sandbox), since the CLI's own login state must persist across conversations.

Reliability fixes for provider_cli conversations

  • ProviderCLIEnvClients: keeps a provider_cli conversation's ACP connection alive across turns (mirroring the existing chat-sandbox pattern), fixing conversation memory loss between messages that a fresh reconnect-and-session/load every turn was causing.
  • IS_SANDBOX=1: works around Claude Code CLI refusing --dangerously-skip-permissions when running as root (this image runs as root deliberately).
  • Migrated Claude Code and Codex specifically off Goose's deprecated raw "CLI providers" mode onto their real ACP-provider equivalents (claude-acp/codex-acp) — fixes the remaining half of the session-continuity issue and, separately, a bug where tool-call events were never surfaced for these two providers (confirmed in Goose's own source: the raw CLI-provider drivers only ever forward the wrapped CLI's final text, silently dropping its tool calls). Cursor Agent and Gemini CLI stay on the raw provider path — Goose has no ACP equivalent for either yet.

Frontend

  • New "Provider CLI" option in the create-agent dialog's agent-type picker (next to LLM/ACP).
  • Shared DefaultEnvironmentSelect/DefaultFolderSelect components (with inline "create new" support), used consistently by the create-agent dialog, the agent detail page, and other environment/folder pickers.
  • provider_cli auth is terminal-login only (no API key option in this dialog — that stays with the plain LLM agent type): a copyable login command for the selected CLI, plus a live "Verify login" button, in both the create dialog and the agent detail page.
  • Locks the environment picker (not the folder picker) when starting a new conversation with a provider_cli agent, since the CLI's login state is tied to that specific environment.
  • New strings translated into all 9 supported locales.

- Introduced a new agent type `provider_cli` that allows agents to run via local CLI providers instead of directly calling model APIs.
- Added fields to the `agents` table to support `cli_provider`, `cli_model`, `cli_auth_mode`, `cli_api_key_secret`, and `cli_login_verified_at`.
- Implemented `VerifyCLIAuth` method in the environment service to check CLI authentication status.
- Updated agent creation and update handlers to require CLI-specific fields when the agent type is `provider_cli`.
- Added new HTTP endpoints for verifying CLI login for both agents and environments.
- Enhanced agent response DTOs to include CLI-related fields.
- Updated tests to cover new functionality and ensure proper handling of CLI agents.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Well-built PR — extensive regression tests tied to real incidents, careful merge-not-clobber logic, and honest self-documentation of confidence levels. One design gap needs a decision before merge: the CLI credential/config namespace is per-environment + provider, not per-agent, so two provider_cli agents on the same static environment silently share a login and clobber each other's MCP config.

Reviewed changes

  • Agent domain + API — Adds provider_cli agent type with cli_provider/cli_model/cli_auth_mode/cli_api_key_secret/cli_login_verified_at (migration 000049), service-layer validation (default env required at create/update and conversation start; rejected for global agents), api_key/login auth modes, and agent- and environment-scoped verify-cli-login endpoints.
  • Provider adapters — One providercli.Adapter per CLI (Claude Code, Codex, Cursor Agent, Gemini CLI) syncing MCP servers (and Claude Code skills) into each CLI's native config; per-CLI auth-status probes.
  • Agent-runner executionbuildProviderCLIContainerEnv; migration of claude-code/codex onto claude-acp/codex-acp; ProviderCLIEnvClients keep-alive registry + idle reaper; syncProviderCLIConfig symlink bootstrap onto a persistent volume; generalized buildFileTar.
  • Image + frontend — Installs the four CLIs (+ acp npm packages) in the agent image; provider_cli create dialog + agent detail (login command, Verify button), shared environment/folder selectors, provider logos, 9 locales.

⚠️ CLI credential/config namespace is per-environment, not per-agent

syncProviderCLIConfig bootstraps $HOME/.<provider> onto a single persistent path /home/paca/workspaces/.cli-home/<cli_provider> and writes merged MCP config back to that shared $HOME for every provider_cli agent. Two provider_cli agents pointing at the same static environment and the same provider — nothing in create/update prevents that, since default_environment_id is freely shared — will share one CLI login (credential on disk, so one expiry/logout affects both) and, on each cold start, overwrite each other's MCP/skill config in those shared files via last-writer-wins. Per-turn re-sync makes it self-healing but not isolated: whichever agent cold-starts last is what ~/.<provider> reflects, and a user who wants two separate provider accounts on the same environment cannot have them.

Technical details
# Per-agent CLI state isolation

## Affected sites
- services/agent-runner/internal/executor/executor.go:896 (providerCLIHomeRoot const) — path is keyed by cfg.CLIProvider only, not agent ID
- services/agent-runner/internal/executor/executor.go:921-931 — `$HOME/.<HomeDirName>` symlink targets the shared `.cli-home/<provider>` subtree
- services/agent-runner/internal/executor/providercli/* — MergeableFiles()/SyncFiles() all write to the shared $HOME-relative paths

## Required outcome
- Decide (and document) whether two provider_cli agents may share one environment+provider. If isolation is wanted, key the persistent subtree per agent (e.g. `.cli-home/<provider>/<agentID>` or `.cli-home/<agentID>`) so login state and MCP/skill config are not cross-contaminated — accepting the tradeoff that each agent then needs its own `login` + Verify step.

## Open questions for the human
- Is multi-agent-per-single-environment a supported topology for these agents? If the env picker is locked per agent default, is it acceptable to also (or instead) reject a provider_cli agent whose default_environment_id already hosts another provider_cli agent of the same provider?

ℹ️ Nitpicks

  • Switching cli_auth_mode back to login never clears the stored cli_api_key_secret (agent_service.go UpdateAgent): the encrypted key stays in the DB and has_cli_api_key stays true even though it's no longer injected. Harmless to execution (injection is gated on api_key mode) but leaves stale secret data and a misleading UI flag — consider nulling it on a login-mode update.
  • Migration 000049 runs ALTER TABLE agents ALTER COLUMN ... SET NOT NULL on every startup under this repo's re-run-on-boot migration model. It is idempotent, but each boot takes an ACCESS EXCLUSIVE lock on agents; on a large/live table that is a periodic lock hit worth being aware of.
  • GOOSE_MODE=auto is unique to the provider_cli path — the LLM branch (buildAgentContainerEnv) never sets it, so a provider_cli agent runs its CLI as root with full permission auto-approval. This matches the sandbox threat model and is thoroughly documented; worth an explicit conscious sign-off given it is strictly more permissive than the existing llm execution mode.
  • Create-dialog copy (cliLoginModeHint: "API key auth isn't available for this agent type yet") is slightly misleading: the backend fully supports api_key for claude-code/codex/gemini-cli — it's the dialog that doesn't offer it. Consider wording it as a UI-scope statement rather than a platform capability one.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

pikann and others added 2 commits September 4, 2026 09:52
CI's pinned biome (2.4.12, via bun install) flagged two lines that
needed reformatting — a multi-line disabled expr and a wrapped
ternary. Fixes the "Check, test, and build web app" CI job on PR #459.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the changes since the last review. Two commits landed: a mermaid/annotationCard fix for the MCP list_tasks converter, and a biome formatting tweak. The previous review's ⚠️ (per-environment CLI credential/config namespace shared between provider_cli agents on the same environment) is untouched by these commits and still needs a decision before merge.

Reviewed changes

  • Fixed a crash in the MCP list_tasks tool: blocknoteToMarkdown now degrades unknown client-only mermaid/annotationCard block types (with hasMentions updated to detect them) instead of letting BlockNote throw on the missing propSchema. Tied to a live incident, with exact-value regression tests (fenced ```mermaid block, [Comment reference (id: …)], mixed-content case).
  • Reformatted create-agent-dialog.tsx/acp-bridge-setup.tsx per the pinned biome to unblock the web CI lint job (non-substantive).

ℹ️ Out-of-scope fix bundled into a provider_cli PR

The converters.ts mermaid/annotationCard change fixes the MCP list_tasks tool — it has nothing to do with the provider_cli feature this PR is otherwise about. The fix itself is solid, but it muddies the PR's review surface and changelog. Either split it into its own PR or update the PR description to document the extra fix so reviewers and history record it accurately.

Note on the prior review: the ⚠️ per-environment-namespace decision (.cli-home/<provider> shared, not keyed per agent) is still open — executor.go and the providercli sync path are unchanged by these commits. Merge remains gated on that decision.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…ersal

- Introduced ErrSkillNameInvalid to handle invalid skill names that could escape the intended directory structure.
- Implemented validateSkillName function to enforce skill name rules, rejecting names that are empty, contain ".", "..", or path separators.
- Added unit tests to ensure invalid skill names are correctly rejected and valid names are accepted.
- Updated HTTP response handling to return appropriate error codes for skill name validation errors.
- Enhanced agent creation and update logic to include checks for CLI provider requirements and environment ID validations.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the changes since the last review — the skill-name traversal fix is solid and well-tested. The prior review's ⚠️ (per-environment CLI credential/config namespace shared between provider_cli agents on the same environment) is untouched by this commit and remains the only open item before merge.

Reviewed changes

  • Skill-name directory-traversal validationvalidateSkillName (agent_service.go:1165) now rejects empty, ., .., and any name containing / or \. AddSkill is the only skill-name entry point (UpdateSkill cannot rename — no SkillName in UpdateSkillInput), and both on-disk writers build the SKILL.md path from the raw name (executor/skills.go:120 and providercli/claude_code.go:82) without sanitizing, so the check is correctly placed to close the traversal for both the LLM and provider_cli paths.
  • Surface as a clean 4xxErrSkillNameInvalid maps to 400 AGENT_SKILL_NAME_INVALID in response.go (both statusAndCodeFor and the httpStatusForCode 400 list), so a rejected name is a client error rather than a scrubbed 500.
  • Regression tests — exact-value, failure-capable coverage: an invalid-name table spanning "../../../etc/cron.d/x", "foo/bar", "foo\\bar", /etc/passwd, ., and .., a positive dotted-name case, and a statusAndCodeFor guard.

Note: this commit also carries a large block of provider_cli / VerifyCLILogin test scaffolding and CLI error-code mappings that prior reviews described as already present — the branch appears to have been rewritten since the last review, so those lines re-surface here. No new issues found in them.

The standing item from the prior review — the .cli-home/<provider> namespace being shared (not keyed per agent), so two provider_cli agents on the same static environment share a login and last-writer-wins their MCP/skill config — is not addressed by this commit (executor.go / providercli sync untouched) and still needs a topology decision before merge.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit dd1c185 into master Sep 4, 2026
11 checks passed
@pikann
pikann deleted the feature/add-provider-cli-agent-type branch September 4, 2026 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant