mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 20:42:10 +00:00
fix: render local-command-stdout messages instead of dropping them (#649)
AI disclaimer: I identified this bug after noticing that custom skills produced no output within the Zed Thread/Claude Agent UI. I used Claude to investigate the issue and develop the fix. I have manually verified the equivalent fix against an installed `@zed-industries/claude-code-acp@0.16.1` (the deprecated package Zed currently ships): custom slash commands that previously produced empty threads now render the model's response correctly after Zed restart. ## Summary The live `case "user" / "assistant"` handler in `src/acp-agent.ts` drops every SDK message whose string content contains `<local-command-stdout>`. The branch was added to tolerate `/compact`'s malformed output, but its side-effect is silencing every successful slash command invocation — custom user-defined skills (whose expanded bodies arrive wrapped in `<command-*>` + `<local-command-stdout>` markers) and built-in commands that emit textual output through these tags. ## Symptom in ACP clients User types `/some-command` (custom skill, or one of the affected built-ins). Thread renders the user's `/some-command` bubble at the top, then nothing — no expanded skill body, no model response. Under the hood the model has actually received the expansion and produced a turn, but the user message carrying the expansion never reaches the UI, and the cascade of subsequent assistant content gets visually decoupled. In CLI Claude Code the same expansion is printed to the terminal, so the asymmetry is purely client-side. ## Fix This repo already ships `stripLocalCommandMetadata` (and its underlying `stripMarkerTags` helper) — they return `null` for marker-only payloads and the stripped prose otherwise. They are already wired into session replay/load (`acp-agent.ts:1385`) but not into the live handler. The change wires the same helper into the live handler: strip the markers and route what remains through the existing `toAcpNotifications` path, preserving the message's role. Pure-marker payloads (e.g. `/compact`'s) still no-op via the `null` branch, so the existing `/compact works` integration test in `src/tests/acp-agent.test.ts` continues to pass. ## Why the previous behaviour was conservative-but-wrong The inline comment notes the filter exists for `/compact`'s SDK-side malformed output. That tradeoff swallows a much larger class of legitimate output (anything that uses `<local-command-stdout>` to wrap real prose) to avoid one specific command's noise. With `stripLocalCommandMetadata` available, the conservative behaviour can target exactly the empty-after-strip case (still `null` → no-op) without affecting messages that have real prose alongside markers — which is exactly the regression case the helper's tests were written to cover (see `src/tests/acp-agent.test.ts:1145-1163`, *"in the original bug report the entire /model preamble and the user's real 'hi' prompt were concatenated into a single message. We want to strip the marker tags and preserve the real prose, not drop the whole message."*). The fix applies that same principle to the live handler, where the helper is most needed. ## Refs - `zed-industries/claude-code-acp#624` — "'Skill' Session Updates missing when invoked via slash command". The reporter correctly diagnoses the mechanism: *"Claude just attaches the skill directly to context when the /command is used, which means the skill loader isn't used."* The skill body arrives wrapped in markers and gets dropped by this filter. - `zed-industries/claude-code-acp#642` — built-in slash commands (`/usage`, `/status`, `/model`, `/memory`, `/permissions`, `/agents`, `/mcp`) emit no output when invoked. Same root cause: their output arrives in messages tagged with `<local-command-stdout>` and is filtered out. Both upstream issues remain open against the old repo `zed-industries/claude-code-acp` and have no comments / linked PRs yet. Cross-linking here so this PR can be the closing reference for both. ## Test plan - [x] `npm run check` (lint + format) — passes - [x] `npm run build` (tsc) — passes - [x] `npx vitest run src/tests/acp-agent.test.ts -t "stripLocalCommandMetadata"` — 8 passed - [ ] Existing `/compact works` integration test — needs a CLI environment to run; relies on the `null` branch preserving the no-op behaviour for marker-only payloads, which the helper tests already cover 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This commit is contained in:
committed by
GitHub
parent
eb1259cf88
commit
3b9b7d5a56
@@ -1118,11 +1118,37 @@ export class ClaudeAcpAgent implements Agent {
|
||||
|
||||
// Slash commands like /compact can generate invalid output... doesn't match
|
||||
// their own docs: https://docs.anthropic.com/en/docs/claude-code/sdk/sdk-slash-commands#%2Fcompact-compact-conversation-history
|
||||
//
|
||||
// Strip local-command marker tags from the content and render whatever
|
||||
// real prose remains, so that custom slash commands / user-defined
|
||||
// skills (whose bodies arrive wrapped in <command-*> / <local-command-stdout>
|
||||
// markers) and built-in commands that emit textual output reach the UI.
|
||||
// Previously this branch dropped every message containing a stdout
|
||||
// marker, which silently swallowed every successful skill invocation
|
||||
// and any built-in command that emits output through these tags.
|
||||
// strip-and-render is safe because stripLocalCommandMetadata returns
|
||||
// null when nothing renderable remains (e.g. pure-marker /compact
|
||||
// payloads), so the no-render no-op path is preserved for those cases.
|
||||
//
|
||||
// Refs zed-industries/claude-code-acp#624, #642.
|
||||
if (
|
||||
typeof message.message.content === "string" &&
|
||||
message.message.content.includes("<local-command-stdout>")
|
||||
) {
|
||||
this.logger.log(message.message.content);
|
||||
const stripped = stripLocalCommandMetadata(message.message.content);
|
||||
if (typeof stripped === "string") {
|
||||
for (const notification of toAcpNotifications(
|
||||
stripped,
|
||||
message.message.role,
|
||||
params.sessionId,
|
||||
this.toolUseCache,
|
||||
this.client,
|
||||
this.logger,
|
||||
)) {
|
||||
await this.client.sessionUpdate(notification);
|
||||
}
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user