fix(opencode): route skill commands through send_message → EventBus-only

[Leoyzen]

Fix for #339

Problem

OpenCode skill slash commands (e.g. /lodestone) were dispatched through _execute_slashed_command — designed for simple print commands like /help — instead of the full prompt lifecycle. This caused three user-visible bugs:

1. ctx.print('Loading skill: ...') rendered as AI reply — captured into an assistant TextPart via _CommandOutputCapture
2. User message swallowed — only an AssistantMessage was created (with parent_id=""), no UserMessage was ever created
3. Double prompt injection — staged_content (skill instructions + args) PLUS a second hardcoded Chinese prompt built by _execute_slashed_command

The correct _execute_skill_command function existed but was dead code (skill_cmd = None always raised 404).

Solution

Rewrite _execute_skill_command to route through the normal prompt lifecycle (send_message → EventBus-only, fire-and-forget), matching the ACP protocol's correct skill handling pattern (handler.py:558-586).

Key Changes

session_routes.py:

• _DiscardOutputWriter — discards ctx.print output (mirrors ACP's output_writer=lambda msg: logger.debug(...)). Prevents "Loading skill: ..." from being rendered as an assistant TextPart.
• Skill command detection in execute_command dispatcher — commands with category=='skill' route to _execute_skill_command inside the existing session lock; all other commands continue to _execute_slashed_command unchanged.
• Per-session agent — uses get_or_create_session_agent() for staged_content injection (not the shared state.agent), matching ACP's pattern.
• Empty content to send_message — skill_bridge.py already injects both instructions AND args into staged_content. Passing args again as content would double-inject. Empty content means the model gets everything from staged_content.
• meta=OpenCodeUserMessageMeta(parts=[TextPart(raw_command)]) — send_message(content="") causes the EventProcessor to create an empty user message (no parts). Passing meta with serialized TextPart parts allows reconstruction from meta.parts, displaying /lodestone analyze this in the TUI.
• Prefer integration.route_message — propagates assistant_msg_id through _pending_message_ids so the HTTP response ID matches the SSE-delivered ID.
• All within execute_command's existing lock — no race condition (Oracle review finding).

test_skill_command_path.py (new, 10 tests):

• Exercises the real execute_command → skill execution → send_message chain
• Tests all three symptoms: no "Loading skill" in TextPart, UserMessage created with raw command, no double prompt
• Tests parent_id linkage, meta propagation, assistant_msg_id propagation, race condition safety, non-skill command regression, _DiscardOutputWriter, CommandExecutedEvent broadcast

What We Skip (non-essential for skill commands)

• Title generation, model selection, revert commit — these _process_message features are skipped. Can be added later if needed.

Verification

• pytest tests/servers/opencode_server/ — 1001 passed, 0 failed
• ruff check + ruff format — clean
• mypy — clean, no type suppressions

Closes #339

[opencode-agent]

Review complete. I've read AGENTS.md, analyzed the diff, and delegated code and docs reviews to specialist subagents, verifying key findings against the source myself.

Review: fix(opencode): route skill commands through send_message → EventBus-only (#342)

Verdict: Approve with minor changes

The fix is correct in its core design and genuinely resolves all three #339 symptoms. The routing decision (category == "skill" → _execute_skill_command → send_message with content="" + meta parts) matches the ACP reference pattern (handler.py:558-586) and is consistent with how RunHandle.start handles empty prompts with populated staged_content. The assistant_msg_id propagation through integration.route_message → _pending_message_ids is sound, and the removed TimeCreated/UserMessage imports are confirmed unused. Below are the findings worth addressing.

Major

1. Test docstring overstates "no over-mocking"; symptom #2 is tested against a mock simulation, not the real chain
tests/servers/opencode_server/test_skill_command_path.py:12-14 claims these tests "do NOT mock command.execute, send_message, or run_stream." In reality, route_message is an AsyncMock whose side-effect re-implements EventProcessor._process_user_message_inserted (conftest.py:634-719), and send_message, get_or_create_session_agent, and command_store are all mocks. server_state.messages is therefore populated by the mock's simulation, so test_skill_command_creates_user_message would pass even if the real EventBus→EventProcessor link broke. The real regressions (symptom #1 _DiscardOutputWriter, symptom #3 double-injection) are genuinely covered by real SlashedCommand.execute. Tune the claim down and add a @pytest.mark.vcr test driving the real OpenCodeSessionPoolIntegration + EventProcessor, per tests/AGENTS.md ("changes that touch protocol event emission warrant L3 VCR tests").

Minor

2. getattr(cmd, "category", None) violates an explicit AGENTS.md red line — session_routes.py:2156. AGENTS.md forbids getattr/hasattr ("provide full type safety"). command_store.get_command() returns a typed BaseCommand | None with a declared category: str field. Use cmd.category == "skill".

3. Fallback else branch silently drops assistant_msg_id — session_routes.py:492-502. session_pool.send_message(...) doesn't pass assistant_msg_id, so _pending_message_ids never gets populated and the SSE-delivered assistant message ID would diverge from the HTTP placeholder — exactly the D14 mismatch this PR claims to fix. It's effectively dead in production (server.py:133-141 sets session_pool_integration whenever session_pool is not None, and this function 500s when it's None), but as written it's a trap. Delete the branch or add a comment explaining why it's unreachable.

4. request.arguments.split() loses quoting semantics — session_routes.py:417. foo "a b" becomes ['foo', '"a', 'b"']. The ACP path parses the same skill commands with the shlex-based slashed parser, so behavior is inconsistent across protocols. Use shlex.split().

5. Missing busy/idle handling — the skill path no longer calls set_session_status(busy)/mark_session_idle (the slashed path and MCP fallback both do). No stuck-busy risk (the event bridge sets idle on StreamCompleteEvent), but there's a window where the session shows idle between the HTTP 200 and RunStartedEvent. Consistent with the normal /message flow, but confirm against the TUI.

6. command.execute failures surface as a raw 500 — session_routes.py:423-426. The old code surfaced errors as an assistant TextPart and always broadcast CommandExecutedEvent. A fire-and-forget design may be better served by logging the error, broadcasting, and returning the placeholder.

7. Test file lacks pytestmark — sibling test_command_execution.py uses pytestmark = [pytest.mark.asyncio, pytest.mark.integration]. This file has none; it should carry integration for consistent CI filtering.

8. test_discard_output_writer_discards_messages uses hasattr — test_skill_command_path.py:581, which tests/AGENTS.md forbids for tests.

Nits

• CommandExecutedEvent semantics changed — now broadcast at dispatch, not completion (session_routes.py:504-512). The model docstring still says "emitted after a slash command runs" (models/events.py:888). Update it, and verify the TUI doesn't treat command.executed as "finished."
• Redundant get_command lookup — session_routes.py:2142 then again at 397. Harmless under the lock; could pass cmd through.
• Fallback branch imports DeliveryMode inside a function — inconsistent with the module-level import style used by the rest of the file (lines 439-441 do the same for OpenCodeUserMessageMeta). Minor.

Docs

• src/agentpool_server/opencode_server/ENDPOINTS.md:112 — POST /session/{id}/command described as "Execute slash command (MCP prompts)"; now also executes skill commands. Update.
• docs/rfcs/draft/RFC-0017-opencode-command-skill-support.md — flow description (skill commands return output synchronously via _execute_slashed_command) diverges from the new placeholder + SSE behavior. Draft/non-normative, so non-blocking; add a note marking it superseded.
• src/agentpool_server/AGENTS.md Notes — worth a one-liner recording that skill commands route through send_message → EventBus-only with _DiscardOutputWriter, so the dead run_stream path isn't reintroduced.
• Pre-existing (not this PR): AGENTS.md:78 context table references docs/explanation/team-mode.md, which doesn't exist.
• OpenSpec: a small bugfix restoring intended behavior (dead-code path) — skipping the OpenSpec workflow is acceptable and not blocking.

Positives

• Per-session agent wiring via get_or_create_session_agent verified correct — send_message resolves the same instance, so staged_content lands on the agent that runs.
• content="" + OpenCodeUserMessageMeta(parts=[...]) is exactly what EventProcessor._process_user_message_inserted (event_processor.py:1106-1110) requires for empty-content reconstruction.
• _DiscardOutputWriter correctly implements the slashed OutputWriter protocol; command execution runs inside execute_command's existing session lock, so the Oracle race finding is addressed.
• 10 focused regression tests covering all three symptoms, with real command execution and real output-writer behavior.

opencode session  |  github run