fix(acp): resolve preset args by command match when no runtime id is set - #3905
fix(acp): resolve preset args by command match when no runtime id is set#3905Kamal-Nayan-Kumar wants to merge 3 commits into
Conversation
Managed agents pinned to a preset's command via agent_command_override (e.g. "opencode") but without an explicit runtime id silently lost the preset's default args. resolve_effective_harness_descriptor only looked up the harness definition via record.runtime / persona.runtime, so a command-override-only agent found no definition and fell back to its own (empty) agent_args. For OpenCode this meant Buzz spawned the bare interactive TUI instead of `opencode acp`, and every pooled worker timed out at the ACP initialize handshake. Add lookup_loaded_harness_by_command as a fallback in both harness_def resolution sites in readiness.rs, matching the effective command against the already-loaded preset/custom registry when the id-based lookup misses. Adds a regression test reproducing the exact scenario (agent_command_override = "opencode", no runtime id, empty agent_args) and asserting the resolved descriptor carries args = ["acp"]. Signed-off-by: Kamal Nayan Kumar <kamalnayanofficialwork@gmail.com>
|
clean preset-arg fallback. one ask: if both runtime id and command match are set and disagree, which wins — and is that spelled out in a test? |
Per review on block#3905: when record.runtime and the effective command (from agent_command_override) name different presets, the explicit runtime id wins — lookup_loaded_harness_by_id short-circuits the new lookup_loaded_harness_by_command fallback via or_else, so the fallback only ever fires when the id-based lookup misses. This precedence predates this PR; this test just documents it now that a command-match path exists to disagree with the id at all. Signed-off-by: Kamal Nayan Kumar <kamalnayanofficialwork@gmail.com>
|
Good question — traced it through:
That precedence isn't new — it's inherited from the original Pushed as a follow-up commit. Full |
Chessing234
left a comment
There was a problem hiding this comment.
this is the right seam for #3824 — command-override without runtime id was silently dropping preset args. the opencode acp regression test is exactly what i'd want before merge. one nit: when runtime id and command disagree, id-wins leaving opencode with amp's empty args is still a footgun; maybe a warn log on that mismatch path so desktop shows why acp never started.
Per review on block#3905: when record.runtime resolves to a harness whose command differs from the agent's effective command (from agent_command_override), that mismatched definition's args/env still apply — the id always wins over a disagreeing command match, per runtime_id_wins_over_disagreeing_command_match. This can silently strip args the effective command actually needs (e.g. an ACP subcommand), and previously surfaced only as a bare ACP initialize timeout with no clue why. Log a tracing::warn! in resolve_effective_harness_descriptor when this mismatch occurs, naming the agent pubkey, the runtime id, the harness's own command, and the effective command, so the agent's log explains the mismatch instead of just timing out. Scoped to resolve_effective_harness_descriptor only (spawn/hash/model probe path) — not the sibling resolve_effective_agent_env, which is also called from several UI preview paths (e.g. live env-diff previews as a user edits settings) where firing on every keystroke would be noise rather than signal. Signed-off-by: Kamal Nayan Kumar <kamalnayanofficialwork@gmail.com>
|
Good nit — pushed a follow-up: a Scoped it to 911 tests still pass, fmt/clippy clean. |
Summary
Managed agents pinned to a preset's command via
agent_command_override(e.g."opencode") but with no explicitruntimeid silently lost the preset's defaultargs. This is the exact bug I hit locally: Buzz spawned bareopencode(its interactive TUI) instead ofopencode acp, so every one of the 10 pooled workers timed out at the ACPinitializehandshake, and the agent never came online.Root cause
resolve_effective_harness_descriptor(readiness.rs) resolves the harness definition (for defaultargs/env) only viarecord.runtime→persona.runtime→lookup_loaded_harness_by_id. An agent whose command was set directly to a preset's command string — without also recording that preset'sruntimeid — satisfies neither, so the lookup misses and the agent falls back to its own (empty)agent_args, even though the command exactly matches a known preset (opencode→ args["acp"]).Fix
Add
lookup_loaded_harness_by_commandincustom_harnesses.rs, scanning the same already-loaded registry (preset + custom harnesses) bycommandinstead ofid. Wire it as an.or_elsefallback in both harness_def resolution sites inreadiness.rs(resolve_effective_harness_descriptorandresolve_effective_agent_env), so a command match still finds the definition when no runtime id is set.Verification
command_override_without_runtime_id_still_gets_preset_argsindiscovery/tests.rs— builds a record shaped exactly like the reported bug (agent_command_override: "opencode", noruntime, emptyagent_args) and asserts the resolved descriptor carriesargs = ["acp"].managed_agents::lib suite: 910 passed, 0 failed.cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean.agent_pool_ready, relay connection, and presence-online all completed successfully.Related
This same root cause is already tracked in #3824, #3729, #3457, #3660, and is also addressed by #3804 (already open, using a similar command-match fallback sourced from
PRESET_HARNESSESdirectly rather than the loaded registry). Posting this as an independent, fully-tested fix per the repo's own diagnosis path in case the alternate approach here (matching against the already-loaded registry, which already carries every preset + custom harness) is useful — happy to close in favor of #3804 if a maintainer prefers consolidating on that one.