Skip to content

fix(desktop): keep clarify prompts answerable - #47544

Closed
lidises wants to merge 2 commits into
NousResearch:mainfrom
lidises:fix/desktop-clarify-request-lifecycle
Closed

fix(desktop): keep clarify prompts answerable#47544
lidises wants to merge 2 commits into
NousResearch:mainfrom
lidises:fix/desktop-clarify-request-lifecycle

Conversation

@lidises

@lidises lidises commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep clarify tool lifecycle events visible to Desktop even when tool progress is set to off, so the interactive prompt can still render and resolve.
  • Reconstruct a pending clarify tool row from clarify.request when the earlier tool.start row was missed during reconnect/hydration.

Why

Desktop can otherwise show that a session needs input while providing no answerable clarify prompt, leaving the backend blocked on clarify.respond.

Test plan

  • ./.venv/bin/python -m pytest tests/test_tui_gateway_server.py::test_tui_clarify_lifecycle_events_emit_when_tool_progress_off tests/test_tui_gateway_server.py::test_tui_non_interactive_tool_lifecycle_stays_hidden_when_tool_progress_off -o 'addopts=' -q
  • cd apps/desktop && npm run typecheck

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 17, 2026
@alt-glitch alt-glitch added comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state and removed comp/gateway Gateway runner, session dispatch, delivery labels Jun 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying a real blocked-input path. Current main still suppresses tool.start and tool.complete when tool progress is off (tui_gateway/server.py:3606, 3664), while Desktop needs a tool-call part to render ClarifyTool (apps/desktop/src/components/assistant-ui/thread/message-parts.tsx:35-37).

Problems

  • The Desktop file changed by this PR was moved to apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts by 51a710e57e; the change needs to be ported there.
  • The synthetic row is keyed with requestId, but _block() creates that ID independently (tui_gateway/server.py:2034-2040) from the lifecycle callback’s tool_call_id (tui_gateway/server.py:3594, 3621). It cannot merge with the later real completion as the comment claims, so the synthetic pending row can remain alongside a separate settled row.

Suggested changes

  • Preserve the narrow gateway lifecycle exception, but rework missed-start recovery to use an ID shared with the later completion (or carry that tool-call ID in clarify.request), and cover that sequence in Desktop tests.

Automated hermes-sweeper review.

// while the inline UI normally mounts from the earlier `tool.start`
// row. If that row was missed (stream reconnect / hydration race) the
// sidebar still shows "needs input" but there is nowhere to render
// choices. Upsert a stable pending clarify tool row from the request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requestId is generated independently by _block() (tui_gateway/server.py:2034-2040), while the actual tool.start/tool.complete use the agent's tool_call_id. These IDs cannot merge in upsertToolPart; after completion this synthetic pending row can remain beside a separate completed row. Use a shared lifecycle ID or carry the tool-call ID in clarify.request.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
OutThisLife added a commit that referenced this pull request Jul 23, 2026
Attribution mapping for the salvaged #47544 commit.
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #69795 — salvaged with your commits (the Python half cherry-picked verbatim; the TS half re-authored onto the post-split use-message-stream/gateway-event.ts since the file you patched was refactored on main). Authorship preserved via cherry-pick + Co-authored-by, and added a contributor mapping for your email. Thanks @lidises! Closing in favor of the rebased version.

OutThisLife added a commit that referenced this pull request Jul 23, 2026
…ol-progress off (#69795)

* fix(desktop): keep clarify lifecycle when tool progress is off

* fix(desktop): render clarify prompt from the request event

Re-authored onto the current use-message-stream/gateway-event.ts (the
original patched the pre-split use-message-stream.ts). When the tool.start
row that normally mounts the inline clarify UI is missed (stream reconnect
/ hydration race), upsert a stable pending clarify tool row from
clarify.request itself so the prompt stays answerable; a real
tool.start/complete with the same request id merges rather than duplicates.

Co-authored-by: 정수환 <centerid@naver.com>

* chore(contributors): map centerid@naver.com -> lidises

Attribution mapping for the salvaged #47544 commit.

* fix(desktop): correlate clarify rows by question so hydration can't duplicate

The hydrated row (from clarify.request's request_id) and the real tool.start
row (the model's tool_call_id) have different ids, so id-only matching appended
a second clarify card in the normal path (caught by the BLOCKING_CLARIFY e2e:
'question' resolved to 2 elements). Add 'question' to the tool match-value keys
so a clarify upsert merges into the existing pending clarify row regardless of
id (same request<->args correlation ClarifyToolPending already uses); when no
row exists yet (reconnect/hydration) it still creates one.

---------

Co-authored-by: 정수환 <centerid@naver.com>
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…ol-progress off (NousResearch#69795)

* fix(desktop): keep clarify lifecycle when tool progress is off

* fix(desktop): render clarify prompt from the request event

Re-authored onto the current use-message-stream/gateway-event.ts (the
original patched the pre-split use-message-stream.ts). When the tool.start
row that normally mounts the inline clarify UI is missed (stream reconnect
/ hydration race), upsert a stable pending clarify tool row from
clarify.request itself so the prompt stays answerable; a real
tool.start/complete with the same request id merges rather than duplicates.

Co-authored-by: 정수환 <centerid@naver.com>

* chore(contributors): map centerid@naver.com -> lidises

Attribution mapping for the salvaged NousResearch#47544 commit.

* fix(desktop): correlate clarify rows by question so hydration can't duplicate

The hydrated row (from clarify.request's request_id) and the real tool.start
row (the model's tool_call_id) have different ids, so id-only matching appended
a second clarify card in the normal path (caught by the BLOCKING_CLARIFY e2e:
'question' resolved to 2 elements). Add 'question' to the tool match-value keys
so a clarify upsert merges into the existing pending clarify row regardless of
id (same request<->args correlation ClarifyToolPending already uses); when no
row exists yet (reconnect/hydration) it still creates one.

---------

Co-authored-by: 정수환 <centerid@naver.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants