Skip to content

docs(daemon): align adapter spikes with web-first roadmap#4296

Open
chiga0 wants to merge 1 commit into
QwenLM:mainfrom
chiga0:feat/daemon-client-doc-alignment
Open

docs(daemon): align adapter spikes with web-first roadmap#4296
chiga0 wants to merge 1 commit into
QwenLM:mainfrom
chiga0:feat/daemon-client-doc-alignment

Conversation

@chiga0
Copy link
Copy Markdown
Collaborator

@chiga0 chiga0 commented May 18, 2026

Summary

  • What changed: Updated the daemon adapter spike docs for TUI, channel/web, and IDE to match the latest web-first daemon roadmap.
  • Why it changed: The latest architecture keeps native local TUI on the direct runtime path, keeps channel and IDE on ACP by default, and prioritizes daemon-native web chat / web terminal before broader client migration.
  • Reviewer focus: Please verify that these docs correctly describe the merged default-off spikes as historical/future references rather than default migration plans.

Validation

  • Commands run:
    git diff --check
    npx prettier --check docs/developers/daemon-client-adapters/tui.md docs/developers/daemon-client-adapters/channel-web.md docs/developers/daemon-client-adapters/ide.md
  • Prompts / inputs used: N/A, docs-only change.
  • Expected result: Markdown formatting passes and the docs align with the updated daemon roadmap.
  • Observed result: git diff --check passed; Prettier reported all matched files use Prettier code style.
  • Quickest reviewer verification path: Read the three updated adapter docs and compare their stated defaults against the current daemon roadmap discussion.
  • Evidence (output, logs, screenshots, video, JSON, before/after, etc.): Command results above.

Scope / Risk

  • Main risk or tradeoff: Documentation wording may need adjustment if maintainers want a different name for the web-first daemon phase.
  • Not covered / not validated: No runtime behavior, CLI behavior, or tests changed.
  • Breaking changes / migration notes: None.

Testing Matrix

🍏 🪟 🐧
npm run N/A N/A N/A
npx N/A N/A
Docker N/A N/A N/A
Podman N/A N/A N/A
Seatbelt N/A N/A N/A

Testing matrix notes:

  • Docs-only change. Prettier was checked locally on macOS.

Linked Issues / Bugs

Related to #3803 and #4175.

@github-actions
Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR updates three daemon adapter documentation files (TUI, channel-web, IDE) to align with the web-first daemon roadmap established in the 2026-05-19 architecture decision. The changes reframe these adapters from "default migration candidates" to "historical/future reference spikes" while keeping native TUI and ACP-based IDE/channel paths as defaults. The documentation is clear, well-structured, and correctly reflects the current architectural direction.

🔍 General Feedback

  • Excellent documentation hygiene: All three files consistently use "Spike" terminology instead of "Draft", correctly signaling these are experimental references rather than active migration plans.
  • Clear architectural positioning: Each doc now explicitly states the 2026-05-19 decision and what the default path should be (native TUI, ACP for IDE/channels).
  • Consistent structure: All three docs follow the same pattern (Goal → Entry Point → Event Mapping → Non-Goals → Merge Safety → Validation → Follow-Up Direction), making them easy to cross-reference.
  • Risk-aware: Each doc correctly identifies session isolation, permission routing, and identity as prerequisites before any future migration.
  • Positive pattern: The "Current Follow-Up Direction" sections provide actionable next steps (extract shared render core, prioritize web terminal) rather than just listing blockers.

🎯 Specific Feedback

🔵 Low

  • File: tui.md:11 - The phrase "native local TUI is not planned to default-migrate to daemon HTTP/SSE" could benefit from a brief rationale link or parenthetical explaining why (e.g., "avoids extra localhost hop, simpler UX"). Consider adding a sentence or linking to the architecture decision discussion.

  • File: tui.md:63 - "Typed event reducers should stay in the daemon client/protocol layer, not in server internals" — this is a design decision that might benefit from a brief explanation or link to related discussions for future readers.

  • File: channel-web.md:23 - The command example qwen channel start telegram and qwen web-chat-backend are described as "Historical channel backend experiment" but the commands themselves don't exist yet (docs say "not wired yet"). Consider clarifying these are proposed entry points that were never implemented, rather than historical commands that were deprecated.

  • File: ide.md:119 - "Revisit daemon IDE integration later, after web contract, workspace/path identity checks, editor-context routing, auth/status UI, and control-plane parity are stable" — this is a long list without prioritization. Consider grouping into phases (e.g., "Phase 1: web contract + identity; Phase 2: editor-context + auth UI; Phase 3: control-plane parity").

✅ Highlights

  • Clear risk communication: The PR body explicitly calls out the main risk ("Documentation wording may need adjustment if maintainers want a different name for the web-first daemon phase") — excellent transparency.
  • Conservative defaults: All three docs correctly emphasize that existing behaviors (native TUI, ACP subprocess for IDE/channels) remain unchanged and are the recommended paths.
  • Session isolation guidance: The channel-web doc's explicit warning against multiplexing unrelated threads into one daemon session is crucial security/architecture guidance that's easy to find.
  • Merge safety sections: Each doc explicitly states "Default off", "Additive code path", and "No existing behavior changes" — makes review and future maintenance much easier.
  • Validation plans are concrete: Each doc lists specific unit tests and smoke tests, making it clear what "done" looks like for future implementations.

Copy link
Copy Markdown
Collaborator

@wenshao wenshao left a comment

Choose a reason for hiding this comment

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

One additional suggestion could not be anchored inline because the relevant unchanged line is outside GitHub's diff hunk:

[Suggestion] In docs/developers/daemon-client-adapters/channel-web.md, the minimal channel flow should prefer the session-scoped permission route for multi-user daemon adapters. The SDK exposes session.respondToSessionPermission(), and the daemon advertises it via caps.features.session_permission_vote; using the session-scoped route keeps permission votes tied to the daemon session that emitted the request and avoids future implementers copying a less isolated pattern into channel/web deployments. Suggested wording: Cast permission votes through session.respondToSessionPermission() when caps.features.session_permission_vote is advertised; use the legacy request-id route only for explicitly single-user or older-daemon fallback.

— gpt-5.5 via Qwen Code /review

Copy link
Copy Markdown
Collaborator

@wenshao wenshao left a comment

Choose a reason for hiding this comment

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

[Suggestion] ide.md:62 — "yet" retained but removed from the other 2 files

The PR edited the "not wired" sentence in all 3 files but applied inconsistent fixes:

  • tui.md:64: removed "yet" entirely
  • channel-web.md:89: replaced "yet" with "by default"
  • ide.md:62: left "yet" unchanged ("It is not wired into the default QwenAgentManager path yet")

"Yet" implies default migration is still the intended endpoint, contradicting the spike framing.

Suggested fix: Change to "It is not wired into the default QwenAgentManager path. Existing VS Code behavior remains ACP subprocess based."

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

existing ACP subprocess behavior by default; daemon channel integration remains
future / behind-flag evaluation.

This draft covers server-side clients only:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] The title was changed from "Draft" to "Spike" but this line still reads "This draft covers server-side clients only". Internal inconsistency within the same file.

Suggested change
This draft covers server-side clients only:
This document covers server-side clients only:

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

## Proposed Entry Points

Channel backend:
Historical channel backend experiment:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] The section header ## Proposed Entry Points (line 21 above) was not renamed to match the spike framing. The other 2 files both use ## Historical Experimental Entry Point. This subsection text was correctly updated to "Historical channel backend experiment:" but the parent heading was missed.

Suggested fix: Rename line 21 to ## Historical Experimental Entry Points (plural, since this file covers two entry points).

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

`DaemonSessionClient` instead of creating an in-process `Config` + agent
runtime.
Document the default-off `DaemonTuiAdapter` spike that was added while
evaluating Mode B client integration.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] "Mode B" is retained here but all other "Mode B" references were removed from ide.md and channel-web.md by this PR. It is now an orphaned term, undefined anywhere in the doc set. A reader encountering this file for the first time has no way to understand what "Mode B" refers to.

Suggested change
evaluating Mode B client integration.
evaluating daemon-backed client integration.

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

daemon setting level. Until per-request `sessionScope` lands, multi-user channel
or web deployments must choose one of these safe shapes:
Multi-user channel or web deployments must explicitly isolate sessions. Use
per-request `sessionScope: 'thread'` when supported; otherwise choose one of
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The old text explicitly stated "Current daemon Stage 1 behavior is effectively sessionScope: single at the daemon setting level". This PR replaced it with "when supported" — a hedge that obscures whether sessionScope: 'thread' actually exists at the daemon server level (code search shows sessionScope type exists in channels but the daemon server core has zero references to it).

Suggested fix: Restore the diagnostic: "As of this spike, the daemon does not support per-request sessionScope. Until it does, multi-user deployments must choose one of these isolation shapes:"

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

Document the default-off `DaemonChannelBridge` spike for server-side channel
and web backends.

As of the 2026-05-19 architecture decision, web chat / web terminal are the
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] All 3 files add "As of the 2026-05-19 architecture decision" but no ADR, design doc, or decision record exists in the repository for this date. Future readers have no way to verify what this decision entailed, who made it, or whether it has since been superseded.

Suggested fix: Add an ADR in .qwen/design/, link to the relevant GitHub discussion, or soften the reference (e.g., "As of a May 2026 architecture direction...").

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

- Smoke-test browser -> BFF -> daemon without exposing daemon token.

## Blockers Before Default Migration
## Current Follow-Up Direction
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The old "Blockers Before Default Migration" sections across all 3 files were explicit checklists of hard prerequisites (session-scoped permission route, daemon-stamped client identity, FileSystemService boundary, session lifecycle close/delete). The new "Current Follow-Up Direction" prose mentions these concepts vaguely, losing the gating semantics. A future implementer scanning the doc will not see a clear stop-sign.

Suggested fix: Add a brief "Security Prerequisites" subsection within this section listing the unresolved security items (client identity, session-scoped permission, filesystem boundary).

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

@chiga0 chiga0 force-pushed the feat/daemon-client-doc-alignment branch 2 times, most recently from b3c56db to 5cd64d5 Compare May 19, 2026 02:58
@chiga0 chiga0 force-pushed the feat/daemon-client-doc-alignment branch from 5cd64d5 to 7430e5d Compare May 19, 2026 03:00
@chiga0
Copy link
Copy Markdown
Collaborator Author

chiga0 commented May 19, 2026

Handled the valid review items and the low-risk documentation polish in the same follow-up:

  • ide.md no longer says the daemon IDE path is not wired “yet”; it now states the default remains ACP subprocess based.
  • channel-web.md now prefers session.respondToSessionPermission() when session_permission_vote is advertised, with legacy request-id route only for older/single-user fallback.
  • tui.md / ide.md now use the same session-scoped permission wording for future daemon paths.
  • Clarified that the channel backend command is a proposed historical experiment, not a currently wired command.
  • Added a short reducer-layer rationale and phased the IDE follow-up list.

Generated by GPT-5 Codex

@chiga0 chiga0 requested a review from wenshao May 19, 2026 03:00
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.

2 participants