Skip to content

fix(opencode): route skill commands through EventBus-only path - #341

Open
Leoyzen wants to merge 11 commits into
mainfrom
fix/skill-command-routing
Open

fix(opencode): route skill commands through EventBus-only path#341
Leoyzen wants to merge 11 commits into
mainfrom
fix/skill-command-routing

Conversation

@Leoyzen

@Leoyzen Leoyzen commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Problem

Skill slash commands (e.g. /lodestone) in the OpenCode protocol server were dispatched through _execute_slashed_command, a path designed for simple commands (like /help). This caused three user-visible problems — see issue #339 for full analysis:

  1. User message swallowed: _execute_slashed_command only created an assistant_message — never a role="user" message. The user's /lodestone <args> input never appeared in session history or the TUI.
  2. "Loading skill" shown as AI reply: skill_bridge's ctx.print("Loading skill: ...") was captured into the assistant message's TextPart, so the TUI rendered it as if the AI said it.
  3. Double prompt injection: skill_bridge injected the full <skill-instruction> into staged_content, AND _execute_slashed_command assembled a second agent_prompt ("用户执行了命令 '...' 并说: ...") passed to run_stream. The model received both, with the second being hardcoded Chinese template text.

Fix

When command.category == "skill" (or state.skill_bridge.get_command() matches), _execute_slashed_command now:

  1. Creates a USER message (with role="user" + TextPart containing the user's arguments) and broadcasts it — visible in the TUI, matching the normal prompt path.
  2. Routes via session_pool_integration.route_message()send_message_consume_run (EventBus-only, exactly-once events) — the same path as normal prompts.
  3. Does NOT call run_stream() — no double prompt injection. Skill instructions remain in staged_content (injected by skill_bridge's command.execute), consumed automatically by turn.py which prepends staged_content to the prompt.

Non-skill commands (e.g. /help) keep the legacy run_stream() behavior unchanged.

Architecture Alignment

This aligns the OpenCode skill command path with the ACP protocol's skill handling (acp_server/handler.py: inject into staged_contentsend_message → EventBus-only), and with the normal OpenCode prompt path (message_routes.py: create user message → route_message_consume_run).

Tests

  • test_skill_command_routes_via_route_message_not_run_stream: verifies skill command broadcasts a user message, calls route_message, does NOT call run_stream.
  • test_plain_commands_keeps_run_stream_path: verifies non-skill commands still use run_stream.
  • All 36 existing command execution tests pass.

Related

Leoyzen added 11 commits July 31, 2026 23:48
Skill slash commands (e.g. /lodestone) were dispatched through
_execute_slashed_command, which:
1. Never created a user message — the user's input was swallowed (TUI
   showed 'Loading skill: ...' as the AI reply, user message invisible)
2. Called run_stream(agent_prompt) which double-injected the prompt
   (skill_bridge injected staged_content + agent_prompt was passed
   separately, both reaching the model)
3. Used a different path from normal prompts (run_stream vs
   route_message → _consume_run)

Fix: when command.category == 'skill', create a proper USER message
(visible in TUI) and route via session_pool_integration.route_message()
→ send_message → _consume_run (EventBus-only, exactly-once events).
Skill instructions remain in staged_content (injected by skill_bridge's
command.execute), consumed automatically by turn.py. Non-skill commands
keep the legacy run_stream() behavior.

Regression tests verify:
- skill command broadcasts a user message + calls route_message
- skill command does NOT call run_stream
- non-skill commands still use run_stream

See issue #339 for full problem analysis.
@opencode-agent

Copy link
Copy Markdown

All three specialist reviews are complete and my spot-checks confirm the load-bearing claims. Posting the synthesized review.

Review: fix(opencode): route skill commands through EventBus-only path

Verdict: Request changes (blocking). The PR body describes a focused routing fix, but the diff is dominated by a re-introduction of the agent-db feature that was already reverted on main, and the routing fix itself has critical correctness flaws. Full details below.


1. BLOCKING — Scope: PR re-introduces reverted, broken agent-db code

11 of 12 changed files (+~5,300 lines) are the agent_db capability that main already reverted via PR #340 (revert: remove incomplete agent_db commits (942a9e70b..b8d10ca2f)). The diff vs. the base branch re-adds all of it. The PR body mentions #340 as "unblocks CI" but treats the agent-db code as out of scope — yet it is in this diff. Either rebase to drop it, or explicitly document intent to re-land it.

The re-added code cannot be imported:

  • All 5 source files import agentpool.capabilities.agent_db.helpers, .visibility, and AgentDBCapabilitynone of which exist in the tree (git ls-tree HEAD src/agentpool/capabilities/agent_db/ shows only the 5 .py files; no __init__.py).
  • All 4 test files use fixtures (mock_client, mock_viking, agent_db_cap) defined nowhere, and import the missing AgentDBCapability — the suite breaks at collection.
  • tools.py::build_tools() is dead code — nothing wires it into a capability, registry, or build_tools entry point.
  • ruff.toml adds suppression blocks for 6 files that don't exist (agent_db/__init__.py, helpers.py, test_proxy_tools.py, test_scaffolding.py, test_helpers.py, tests/.../agent_db/__init__.py).
  • No OpenSpec change, no docs, no AGENTS.md in the new directory (all required by the repo workflow).
  • tools.py:209 calls client.grep(pattern, uri=...) but the viking SDK signature is client.grep(uri, pattern, ...) — the keyword collision is caught by blanket except, so agentdb_grep always returns "grep error".
  • No logfire instrumentation anywhere in ~2,900 lines (capabilities must be instrumented per src/agentpool/capabilities/AGENTS.md).
  • Docstrings are actually Google-style and clean (the D417 suppressions are justified); the tests are well-written behavioral tests — but they're all unrunnable. The commit history confirms this is the "half-committed state" that was correctly reverted.

2. BLOCKING — The skill fix double-publishes the user message

session_routes.py:318-320 directly appends + broadcasts the user message (PartUpdatedEvent + MessageUpdatedEvent), and then calls route_message() (line 325), which publishes a UserMessageInsertedEvent that the OpenCode EventProcessor also turns into a broadcast user message (event_processor.py:1052-1151). There is no message-ID dedup on the OpenCode side — the only displayed_message_ids set lives in the ACP converter. The codebase explicitly consolidated to a single publication path (tests/servers/opencode_server/test_message_routes_dedup.py:5 — "The dedup set has been removed — there is only one publication path now"). The comment at session_routes.py:322-323 claiming "dedup with this emission" is false; the TUI receives a second text part with a fresh part ID. The normal path avoids this by passing meta=OpenCodeUserMessageMeta(parts=...) (message_routes.py:700-701) and letting the EventProcessor be the sole emitter. Drop the direct broadcast and pass meta instead.

3. BLOCKING — Skill instructions never reach the model

The cmd_ctx is built from state.agent (the shared server agent, session_routes.py:247-250), so skill_bridge.execute_skill injects <skill-instruction> into state.agent.staged_content (skill_bridge.py:140). But the run started by route_messagesend_message executes on the per-session agent (session_pool_messaging.py:243-245get_or_create_session_agent), whose staged_content is empty and consumed by run.py:419/turn.py. There is no copy path between them. The ACP reference resolves the per-session agent for exactly this reason (acp_server/handler.py:597-618: "Use per-session agent context so expanded prompts land in the correct staged_content"). The PR's claim at session_routes.py:277-279 is factually wrong — the model receives only the raw arguments. (This predates the PR, but the PR asserts this mechanism as its fix and doesn't test it.)

4. High — Message-ID collision when the client sends message_id

Both assistant_msg_id (line 209) and user_msg_id (line 297) use identifier.ascending("message", request.message_id), which returns the given ID verbatim (identifiers.py:46-67). When the OpenCode client supplies a message_id (the D14 pattern, as the normal path does at message_routes.py:306), user and assistant messages share one ID — the user message is deduped out of state.messages (opencode_message_bridge.py:202). The normal path deliberately generates a fresh assistant ID (message_routes.py:512). Not covered by the tests (which never pass message_id).

5. Medium — Red-line violation, ordering, silent failures, edge cases

  • getattr(command, "category", "") (line 285) violates the AGENTS.md red line ("No getattr/hasattr"). slashed BaseCommand declares category: str; command.category == "skill" is fully typed.
  • Ordering inverted: the command-output assistant message is appended/broadcast first (lines 226-240), the user message after (line 318) — user input renders below the assistant message. Additionally, the run's assistant message (same ID, from _pending_message_ids) is deduped out of state.messages, and step_finish (line 383) is broadcast before the run produces output.
  • Silent error swallowing: the whole routing block is except Exception: pass (lines 372-374) with no logging — a failed route_message returns HTTP 200 with no error surfaced, and the new path is unspanned (no logfire instrumentation).
  • Empty arguments: request.arguments or "" produces an empty user bubble; the legacy path had a fallback prompt.
  • Tests: test_skill_command_routing.py verifies only the routing decision (route_message called / run_stream not called), so it passes despite issues feat(tools): implement extended tool definitions with native Pydantic… #2feat: add configurable skills loading paths with YAML configuration #4. Per tests/AGENTS.md, a bug fix requires a reproducing test of the actual TUI-visible defect, and protocol event-emission changes warrant VCR/E2E. The tests also use getattr (line 116) and # type: ignore (lines 86, 98, 129, 143).

6. Docs / OpenSpec

  • No OpenSpec change for either the agent-db feature or the routing fix (both required by AGENTS.md).
  • agent-db is undocumented: no AgentDBCapability in docs/explanation/capabilities.md, no row in src/agentpool/capabilities/AGENTS.md, no agent_db/AGENTS.md.
  • Pre-existing (not this PR): broken link docs/explanation/team-mode.md in the root AGENTS.md context table.

Recommended path forward

  1. Rebase onto current main to drop the agent-db commits (or, if re-landing intentionally, land the missing __init__.py/helpers.py/visibility.py/AgentDBCapability/conftest alongside and file an OpenSpec change).
  2. Fix the user-message path: drop the direct broadcast, pass meta=OpenCodeUserMessageMeta(parts=...) to route_message — single publication path.
  3. Resolve the per-session agent (as ACP does) for the command context so staged_content lands on the run's agent.
  4. Fix the user_msg_id/assistant_msg_id collision; use command.category directly; log the swallowed exceptions.
  5. Add an E2E/VCR test asserting the actual TUI-visible behavior (user message once, response rendered, skill instructions present).

New%20session%20-%202026-07-31T17%3A25%3A12.192Z
opencode session  |  github run

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