Skip to content

fix(acp): route Zed thoughts to reasoning callbacks#18578

Closed
HenkDz wants to merge 7 commits into
NousResearch:mainfrom
HenkDz:fix/acp-zed-rendering
Closed

fix(acp): route Zed thoughts to reasoning callbacks#18578
HenkDz wants to merge 7 commits into
NousResearch:mainfrom
HenkDz:fix/acp-zed-rendering

Conversation

@HenkDz
Copy link
Copy Markdown
Contributor

@HenkDz HenkDz commented May 1, 2026

Summary

  • route ACP thought panes to provider reasoning deltas instead of Hermes kawaii waiting/status updates
  • stream assistant response chunks through stream_delta_callback and avoid re-sending final_response after streaming
  • update ACP command-list test expectation for advertised /steer and /queue

Test Plan

  • scripts/run_tests.sh tests/acp/test_server.py tests/acp/test_events.py -q
  • python -m py_compile acp_adapter/server.py tests/acp/test_server.py
image

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/acp Agent Communication Protocol adapter labels May 2, 2026
@HenkDz
Copy link
Copy Markdown
Contributor Author

HenkDz commented May 2, 2026

Added:
1. Proper Skill View:

image

2. Proper Skill Patch:

image

3. Proper ToDo rendering:

image

4. Proper Search results rendering:

image

@HenkDz HenkDz force-pushed the fix/acp-zed-rendering branch 2 times, most recently from 3483b95 to 5109465 Compare May 2, 2026 21:06
@teknium1
Copy link
Copy Markdown
Contributor

teknium1 commented May 3, 2026

Merged via #19139 — all 7 of your commits were cherry-picked onto current main with authorship preserved (rebase-merged, so your per-commit history is intact in the tree).

Why salvage instead of merging this directly: the branch had drifted far enough behind main that a naive GitHub merge would have silently reverted two recent fixes (fix(model): avoid bedrock credential probe in provider picker and fix(tools): reconfigure enabled unconfigured toolsets) — they showed up as deletions in the raw diff even though none of your commits touched hermes_cli/*. Cherry-pick sidestepped that completely.

#19139

Thanks for the rendering overhaul — the Zed context indicator and per-tool polish are a huge quality-of-life win.

@teknium1
Copy link
Copy Markdown
Contributor

teknium1 commented May 3, 2026

Hey @HenkDz, thanks for the work here. Before we pull this in we'd like to clarify a few things about the scope:

1. Scope / title mismatch. The PR title is fix(acp): route Zed thoughts to reasoning callbacks but the diff is really seven related ACP changes totaling +1488 / -63 across 5 files:

  • commit 1 — the actual thought-callback routing (~15 LOC on server.py)
  • commit 2 — UsageUpdate wiring + tool-history replay on session/load + session/resume + richer /context + the first batch of per-tool formatters
  • commit 3 — extends _POLISHED_TOOLS to ~50 entries and adds per-tool start/completion formatters for most of our tool catalog (memory, session_search, delegate_task, process, browser_, cronjob, feishu_, kanban_, ha_, yb_*, discord, etc.)
  • commits 4–7 — rendering tweaks on top (compact read_file / web_extract starts, fenced code output, scheduled history replay)

Could you either split this into smaller focused PRs or update the description so each commit's intent, the specific Zed symptom it addresses, and a before/after is called out? Right now the title undersells the change and the Test Plan is just run tests + py_compile — we'd like to see the actual Zed rendering problems each commit solves.

2. _POLISHED_TOOLS generic fallback. The set now includes a lot of platform-integration tools (discord, yb_*, feishu_*, kanban_*, ha_*, mixture_of_agents, etc.) that don't have explicit formatters — they fall through to _format_generic_structured_result, which is a priority-key dump. That's a lot of new surface where output shape matters.

  • Did you actually exercise these tools through Zed, or were they added speculatively because they return JSON?
  • What happens in Zed today (before the PR) when these tools run? Is there a concrete rendering problem you're solving for each, or is the goal to pre-emptively cover "anything that returns JSON"?
  • If it's the latter, we'd prefer to only polish tools you've actually verified and let the rest keep the existing rendering until someone has a concrete complaint.

3. raw_output=None for polished tools. build_tool_complete now drops raw_output for every polished tool. This intentionally trades raw-JSON visibility in Zed's UI for noise reduction. Did you confirm that clients (Zed specifically, but also anything using the raw_output field programmatically) don't rely on it? If you have a Zed screenshot showing the before/after noise difference that motivated this, can you attach it?

4. Per-commit screenshots. You included one screenshot in the PR body but it's not clear which of the seven changes it demonstrates. Ideally we'd see a Zed before/after per major change — at minimum: (a) thought pane showing real reasoning vs kawaii strings, (b) context indicator populated, (c) history replay showing tool calls, (d) one of the polished tool completions.

We like the direction and commit 1 is a clean fix on its own. We just want to merge this understanding the full scope. I've closed our salvage PR (#19143) in the meantime — we'll pick this back up once the above is clarified.

Thanks!

@HenkDz
Copy link
Copy Markdown
Contributor Author

HenkDz commented May 3, 2026

Thanks for the detailed review — that’s fair feedback.

First, agreed on the scope/title mismatch. The original title was accurate for commit 1, but the PR grew into a broader ACP/Zed rendering polish pass. In hindsight I should have either split it or renamed/described it as:

fix(acp): route Zed reasoning and polish tool/context rendering

Scope / intent by commit

1. fix(acp): route Zed thoughts to reasoning callbacks

Symptom: Zed’s thought/reasoning pane was getting Hermes UI spinner/thinking strings instead of actual provider reasoning.

Intent: disable the generic thinking_callback path for ACP and wire real provider reasoning through reasoning_callback, while normal assistant text goes through stream_delta_callback.

This is the small standalone correctness fix.

2. fix(acp): polish Zed context and tool rendering

Symptoms:

  • Zed context indicator was not populated/useful.
  • /context output was less actionable than the CLI view.
  • tool starts/completions in Zed were noisy/raw for common tools.
  • session load/resume did not replay useful historical tool-call structure.

Intent:

  • wire UsageUpdate;
  • improve /context text with threshold/compression guidance;
  • replay historical tool calls on load/resume;
  • add the first batch of targeted renderers for high-frequency tools.

3. fix(acp): polish common tool rendering

Symptom: many common Hermes tools return structured JSON that was technically correct but bad UX in Zed — large raw blobs, noisy key dumps, and unreadable completions.

Intent: extend the rendering polish so Zed shows human-readable starts/completions for the tools most likely to appear in normal agent sessions.

That said, your concern is valid: some entries in _POLISHED_TOOLS were added because they return JSON-shaped output and would benefit from the generic formatter, not because I manually exercised every single platform integration in Zed.

The tools I directly targeted/tested were the core Zed/dev-loop tools: todo, read_file, write_file, patch, search_files, execute_code, process, delegate_task, session_search, memory, skill_view, skill_manage, web_search, web_extract, browser tools, and vision/image-style outputs.

For the platform-specific integrations (discord, yb_*, feishu_*, kanban_*, ha_*, etc.), the intent was proactive noise reduction for structured JSON, not a claim that each one had a separately observed Zed bug. If you prefer, I’m completely fine narrowing _POLISHED_TOOLS to the explicitly verified/common tools and letting the long-tail integrations keep existing raw rendering until there is concrete feedback.

4–7. Compact starts, fenced output, scheduled replay

Symptoms:

  • read_file / web_extract starts were too verbose for Zed’s tool timeline.
  • file contents could render badly in Zed markdown, especially pipe-heavy content being interpreted as tables.
  • immediate history replay could attach awkwardly relative to the active response.

Intent:

  • keep start messages short;
  • fence file/text output so Zed renders it as content, not markdown tables;
  • schedule replay so historical tool calls attach more predictably after load/resume.

On _POLISHED_TOOLS and generic fallback

Your read is correct: the generic fallback is broader than the manually verified surface.

The intended design was:

  • explicit formatter where we know the tool shape;
  • generic structured summary for JSON results that would otherwise be raw/noisy;
  • fallback to existing behavior for anything not in _POLISHED_TOOLS.

But I agree that putting many integrations into _POLISHED_TOOLS expands the behavioral surface. If maintainers want the lower-risk version, I’d support a follow-up that removes speculative platform integrations from _POLISHED_TOOLS and keeps only the core/common tools that were verified in Zed.

On raw_output=None

This was motivated by Zed UI noise: for polished tools, showing both the human-readable content and the raw JSON made the output feel duplicated and much harder to scan.

I verified the intended behavior from the Zed UI side, but I’m not claiming that no ACP client could ever depend on raw_output programmatically. That’s a fair protocol-surface concern.

Safer options I’d be fine with:

  1. only drop raw_output for explicitly formatted/core tools;
  2. keep raw_output for generic-fallback tools;
  3. make dropping raw output conditional/client-specific if the ACP layer can reliably identify Zed;
  4. restore raw_output everywhere and rely only on the polished display content.

My preference for Zed UX is still to suppress raw output for explicitly polished tools, but I agree the broader generic case should be conservative.

Screenshots / before-after

The screenshots I attached were mostly examples of the polished output after the formatter work, not a clean per-commit before/after matrix. That was incomplete.

The specific before/after cases I was targeting were:

  • thought pane: Hermes spinner/kawaii thinking strings → actual model/provider reasoning;
  • context indicator: empty/unhelpful → populated via UsageUpdate;
  • session load/resume: missing/flat historical tool output → replayed structured tool calls;
  • common tool completions: raw JSON blobs → readable summaries;
  • file output: markdown/table misrendering → fenced code/text blocks.

I can gather/attach a clearer Zed screenshot set if useful.

Bottom line

I’m aligned with the direction you suggested:

  • commit 1 is the clean standalone fix;
  • the rendering work is useful but should be documented as a broader ACP/Zed UX polish;
  • the speculative _POLISHED_TOOLS entries and raw_output=None behavior are the parts most worth narrowing if you want reduced risk.

If you want, I can open a follow-up cleanup PR that:

  • renames/documents the scope more clearly;
  • narrows _POLISHED_TOOLS to verified/common tools;
  • keeps raw_output for generic-fallback/platform integrations;
  • adds before/after screenshots and a clearer test plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants