feat(managed-agents): add Oh My Pi as a native ACP runtime#2573
Conversation
b7c5b98 to
c8aa75d
Compare
bradhallett
left a comment
There was a problem hiding this comment.
Nice work on this — the OMP capability mapping is accurate. I verified every claim against live omp (v17.1.3) and the ACP surface, and it all holds up (details below). The one thing blocking is a rebase: #2680 landed on main after this branch and restructured the install-metadata fields — that's the dirty/conflicting state, not a logic issue.
Blocker: rebase onto post-#2680 main
#2680 renamed install_instructions_url → cli_install_instructions_url and split out a new adapter_install_instructions_url field. The OMP literal still uses the pre-#2680 single-field name, so it won't compile against current main. After rebasing, update the OMP KnownAcpRuntime entry:
// pre-#2680 (current branch)
install_instructions_url: "https://github.com/can1357/oh-my-pi",
// post-#2680
cli_install_instructions_url: "https://github.com/can1357/oh-my-pi",
adapter_install_instructions_url: "","" is the right adapter URL here — OMP speaks ACP natively, so there's no separate adapter to install.
Capability mapping — verified correct
I ran the ACP initialize handshake against omp acp and probed the CLI; each flag matches what OMP actually exposes:
initializeadvertises nomodelCapabilities→supports_acp_model_switching: false✓initializeadvertises no config capability →supports_acp_native_config: false✓authMethods: [{ id: "agent", "Use existing local credentials…under ~/.omp" }]→ thesetupVersionprobe +omp setuplogin hint match (omp config get setupVersionreturns a positive int, exit 0 when set up) ✓- OMP has no env vars for thinking / maxTokens / contextLimit / primary model / provider → all the
Nonebindings are correct (thinking is the--thinkingflag +model.defaultThinkingLevelconfig key) ✓ config_file_path: ~/.omp/agent/config.yml✓;skill_dir: .omp/skills✓ (matches the.goose/.claude/.codexproject-local pattern; OMP also discovers~/.omp/agent/skillsglobally)- Windows
%LOCALAPPDATA%\omp✓ —install.ps1sets$InstallDir = "$env:LOCALAPPDATA\omp"and writesomp.exe omp.sh/installandomp.sh/install.ps1both resolve (302 →can1357/oh-my-pi/main/scripts/install.{sh,ps1})
The probe_auth_status change (capturing stdout + the positive-integer gate) is sound — it runs only on the non-timeout path after exit_status.success(), and ""/"0" both correctly yield LoggedOut.
Minor
- PR title: CONTRIBUTING.md requires Conventional Commits —
feat(managed-agents): add Oh My Pi as a native ACP runtime. - Onboarding order:
ONBOARDING_RUNTIME_ORDER = ["omp", "claude", "codex"]puts OMP first, ahead of the first-party options. Worth a one-line justification in the body (or a maintainer's explicit call). - Related issue: the PR template has a "Related issue" line — please add it ("none found" is fine) per CONTRIBUTING.
- Probe test:
auth_probe_requires_positive_integer_stdoutis#[cfg(unix)]-only, so the new path gets no Windows coverage. Low risk, but a platform-neutral variant would close the gap.
Happy to help with the rebase if useful.
Signed-off-by: Dakota Secula-Rosell <286529425+dkta0@users.noreply.github.com>
c8aa75d to
a2c86d5
Compare
|
Rebased onto current
Local checks:
|
Summary
omp acpinvocation%LOCALAPPDATA%\omppathRelated issue
None found.
Testing
cargo fmt --manifest-path desktop/src-tauri/Cargo.toml -- --checkcargo test --manifest-path desktop/src-tauri/Cargo.toml managed_agents::discovery::tests:: --lib(78 passed)node --test --experimental-strip-types desktop/src/features/onboarding/ui/onboardingRuntimeSelection.test.mjs(4 passed)