Repository navigation
revert(cli): revert headless agent session control (#5784) - #5837
Conversation
This reverts commit 8386961.
📝 WalkthroughWalkthroughThe PR removes CLI terminal-agent session controls and E2E harnesses, simplifies host workspace discovery and agent launching, removes PTY snapshot protocol support, restores protocol v2, updates CLI/deep-link documentation, and revises desktop CDP authentication guidance. ChangesPTY protocol and snapshot removal
Host-service agent simplification
CLI and documentation reduction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/docs/content/docs/cli/cli-reference.mdx (1)
634-649: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
agents createconsistently here.supersetonly definesagents createandagents list;agents run/#superset-agents-rundon’t exist. Revert the deep-link section toagents create/#superset-agents-create, and change the skill doc references toagents createas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/docs/content/docs/cli/cli-reference.mdx` around lines 634 - 649, The deep-link documentation must consistently use the existing agents create command and anchor. Update apps/docs/content/docs/cli/cli-reference.mdx lines 634-649 and 719-726 to replace agents run and `#superset-agents-run` references with agents create and `#superset-agents-create`, and update skills/superset/SKILL.md lines 63 and 74-76 to reference agents create as well. Preserve the existing deep-link guidance and session fields.packages/cli-framework/src/parser.ts (1)
147-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
"-"working forfromFilepackages/cli/src/commands/automations/prompt/set/command.tsstill documentsUse '-' to read from stdinand branches onoptions.fromFile === "-"; the newstartsWith("-")check will reject that path before stdin can be read. Preserve the"-"exception here, or change the command contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli-framework/src/parser.ts` around lines 147 - 156, Adjust the missing-value check in the parser’s String/number option handling to allow the exact argument "-" through as a valid value, while continuing to reject other arguments beginning with "-". Preserve the existing fromFile stdin behavior consumed by the command’s options.fromFile handling.
🧹 Nitpick comments (2)
packages/pty-daemon/test/helpers/client.ts (1)
228-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CURRENT_PROTOCOL_VERSIONin these handshake tests. Replace the hardcoded2/[2, 99]literals inpackages/pty-daemon/test/helpers/client.ts,packages/pty-daemon/test/control-plane.test.ts, andpackages/pty-daemon/test/integration.test.tsso the suite stays aligned with the exported protocol constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/test/helpers/client.ts` at line 228, Replace hardcoded handshake protocol values with the exported CURRENT_PROTOCOL_VERSION constant in packages/pty-daemon/test/helpers/client.ts:228, packages/pty-daemon/test/control-plane.test.ts:109-117 and :999, and packages/pty-daemon/test/integration.test.ts:29-33; preserve any additional protocol values such as 99 where they test unsupported versions.packages/host-service/src/trpc/router/agents/agents.ts (1)
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inline type import instead of top-level import.
import("../../../events").EventBusworks but is inconsistent with the top-levelimport type { HostDb }pattern already used in this file. Consider hoisting it to a top-level type import for readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/agents/agents.ts` at line 231, Hoist EventBus into a top-level type import alongside HostDb, then update the ctx type in the affected router definition to reference EventBus directly instead of import("../../../events").EventBus.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/host-workspaces/listHostWorkspaces.ts`:
- Around line 63-84: Update the host-selection logic in listHostWorkspaces so
that when discovery returns hosts but the online-host Set is empty after the
local manifest check, it adds a warning explaining that no discovered hosts are
reachable and falls back to localHostId. Preserve the existing targetHostIds
behavior when reachable hosts are present and retain the discovery-error
fallback in the sibling branch.
---
Outside diff comments:
In `@apps/docs/content/docs/cli/cli-reference.mdx`:
- Around line 634-649: The deep-link documentation must consistently use the
existing agents create command and anchor. Update
apps/docs/content/docs/cli/cli-reference.mdx lines 634-649 and 719-726 to
replace agents run and `#superset-agents-run` references with agents create and
`#superset-agents-create`, and update skills/superset/SKILL.md lines 63 and 74-76
to reference agents create as well. Preserve the existing deep-link guidance and
session fields.
In `@packages/cli-framework/src/parser.ts`:
- Around line 147-156: Adjust the missing-value check in the parser’s
String/number option handling to allow the exact argument "-" through as a valid
value, while continuing to reject other arguments beginning with "-". Preserve
the existing fromFile stdin behavior consumed by the command’s options.fromFile
handling.
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/agents/agents.ts`:
- Line 231: Hoist EventBus into a top-level type import alongside HostDb, then
update the ctx type in the affected router definition to reference EventBus
directly instead of import("../../../events").EventBus.
In `@packages/pty-daemon/test/helpers/client.ts`:
- Line 228: Replace hardcoded handshake protocol values with the exported
CURRENT_PROTOCOL_VERSION constant in
packages/pty-daemon/test/helpers/client.ts:228,
packages/pty-daemon/test/control-plane.test.ts:109-117 and :999, and
packages/pty-daemon/test/integration.test.ts:29-33; preserve any additional
protocol values such as 99 where they test unsupported versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ac99e3b-485c-49ba-899c-235230c1097d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
AGENTS.mdapps/desktop/AGENTS.mdapps/desktop/docs/CLI_CDP_E2E.mdapps/docs/content/docs/cli/cli-reference.mdxpackage.jsonpackages/cli-framework/src/parser.test.tspackages/cli-framework/src/parser.tspackages/cli/package.jsonpackages/cli/src/commands/agents/sessions/list/command.tspackages/cli/src/commands/agents/sessions/meta.tspackages/cli/src/commands/agents/sessions/read/command.tspackages/cli/src/commands/agents/sessions/send/command.test.tspackages/cli/src/commands/agents/sessions/send/command.tspackages/cli/src/commands/agents/sessions/shared.test.tspackages/cli/src/commands/agents/sessions/shared.tspackages/cli/src/commands/agents/sessions/wait/command.tspackages/cli/src/commands/workspaces/create/agent-results.test.tspackages/cli/src/commands/workspaces/create/agent-results.tspackages/cli/src/commands/workspaces/create/command.tspackages/cli/src/lib/host-agent-sessions.tspackages/cli/src/lib/host-target/index.tspackages/cli/src/lib/host-target/queryHostTargets.test.tspackages/cli/src/lib/host-target/queryHostTargets.tspackages/cli/src/lib/host-workspaces/listHostWorkspaces.tspackages/cli/test/e2e/agent-sessions.e2e.tspackages/cli/test/e2e/harness.tspackages/cli/test/fixtures/fake-terminal-agent.mjspackages/cli/tsconfig.jsonpackages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.tspackages/host-service/src/terminal-agents/index.tspackages/host-service/src/terminal-agents/status.test.tspackages/host-service/src/terminal-agents/status.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.node-test.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.tspackages/host-service/src/terminal/DaemonClient/index.tspackages/host-service/src/terminal/terminal-snapshot.test.tspackages/host-service/src/terminal/terminal-snapshot.tspackages/host-service/src/terminal/terminal.adoption.node-test.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/agents/agent-launch.test.tspackages/host-service/src/trpc/router/agents/agent-launch.tspackages/host-service/src/trpc/router/agents/agents.test.tspackages/host-service/src/trpc/router/agents/agents.tspackages/host-service/src/trpc/router/agents/attachment-prompt.tspackages/host-service/src/trpc/router/terminal-agents/terminal-agents.test.tspackages/host-service/src/trpc/router/terminal-agents/terminal-agents.tspackages/host-service/test/fixtures/agent-sessions-cli-host.tspackages/pty-daemon/package.jsonpackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/SessionStore/SessionStore.test.tspackages/pty-daemon/src/SessionStore/SessionStore.tspackages/pty-daemon/src/SessionStore/snapshot.test.tspackages/pty-daemon/src/SessionStore/snapshot.tspackages/pty-daemon/src/handlers/handlers.test.tspackages/pty-daemon/src/handlers/handlers.tspackages/pty-daemon/src/handlers/index.tspackages/pty-daemon/src/protocol/index.tspackages/pty-daemon/src/protocol/messages.tspackages/pty-daemon/src/protocol/version.tspackages/pty-daemon/src/protocol/wire-shape.test.tspackages/pty-daemon/test/control-plane.test.tspackages/pty-daemon/test/helpers/client.tspackages/pty-daemon/test/integration.test.tsskills/superset/SKILL.md
💤 Files with no reviewable changes (44)
- packages/host-service/src/trpc/router/agents/attachment-prompt.ts
- packages/cli/src/commands/agents/sessions/meta.ts
- packages/host-service/src/terminal-agents/status.ts
- packages/host-service/src/terminal-agents/status.test.ts
- packages/pty-daemon/src/handlers/index.ts
- apps/desktop/docs/CLI_CDP_E2E.md
- packages/cli/src/commands/agents/sessions/wait/command.ts
- packages/cli/src/commands/agents/sessions/read/command.ts
- packages/cli/src/commands/workspaces/create/agent-results.ts
- packages/cli/src/commands/agents/sessions/send/command.test.ts
- packages/host-service/src/trpc/router/terminal-agents/terminal-agents.test.ts
- packages/cli/test/fixtures/fake-terminal-agent.mjs
- packages/cli/src/commands/workspaces/create/agent-results.test.ts
- packages/host-service/src/trpc/router/agents/agent-launch.test.ts
- packages/cli/src/commands/agents/sessions/shared.test.ts
- packages/host-service/src/terminal/DaemonClient/index.ts
- packages/pty-daemon/src/SessionStore/snapshot.test.ts
- packages/cli/src/lib/host-target/queryHostTargets.ts
- packages/host-service/src/trpc/router/agents/agent-launch.ts
- packages/host-service/src/terminal/terminal-snapshot.test.ts
- packages/host-service/test/fixtures/agent-sessions-cli-host.ts
- packages/cli/src/lib/host-agent-sessions.ts
- packages/cli/test/e2e/agent-sessions.e2e.ts
- packages/host-service/src/terminal/terminal-snapshot.ts
- packages/cli-framework/src/parser.test.ts
- packages/cli/src/commands/agents/sessions/send/command.ts
- packages/cli/src/commands/agents/sessions/shared.ts
- packages/pty-daemon/src/protocol/messages.ts
- packages/cli/src/lib/host-target/queryHostTargets.test.ts
- packages/cli/package.json
- packages/cli/src/commands/agents/sessions/list/command.ts
- package.json
- packages/cli/src/commands/workspaces/create/command.ts
- packages/pty-daemon/src/SessionStore/SessionStore.test.ts
- packages/pty-daemon/src/handlers/handlers.ts
- packages/host-service/src/terminal-agents/index.ts
- AGENTS.md
- packages/pty-daemon/src/handlers/handlers.test.ts
- packages/cli/src/lib/host-target/index.ts
- packages/pty-daemon/src/SessionStore/snapshot.ts
- packages/pty-daemon/src/Server/Server.ts
- packages/pty-daemon/src/protocol/index.ts
- packages/host-service/src/terminal/terminal.adoption.node-test.ts
- packages/cli/test/e2e/harness.ts
| let targetHostIds: string[]; | ||
| if (options.hostId) { | ||
| targetHostIds = [options.hostId]; | ||
| } else if (hosts.length > 0) { | ||
| const ids = new Set( | ||
| hosts.filter((host) => host.online).map((host) => host.id), | ||
| ); | ||
| // Cloud presence can lag: always include this machine's host when its | ||
| // host-service manifest points at a live process. | ||
| const manifest = readManifest(options.organizationId); | ||
| if (manifest && isProcessAlive(manifest.pid)) { | ||
| ids.add(localHostId); | ||
| } | ||
| targetHostIds = [...ids]; | ||
| } else { | ||
| if (discoveryError !== undefined) { | ||
| warnings.push( | ||
| `Cloud host discovery failed (${describeError(discoveryError)}); listing this machine's host only`, | ||
| ); | ||
| } | ||
| targetHostIds = [localHostId]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent empty result when discovered hosts exist but none are online/reachable.
If options.api.host.list.query succeeds and returns hosts, but none are online and the local manifest is missing/stale, ids (and thus targetHostIds) ends up empty. Promise.allSettled([]) then produces no results, so the function returns { workspaces: [], warnings: [] } with no explanation — the caller can't tell "no workspaces exist" apart from "no hosts are reachable." The sibling else branch (cloud discovery failure) already handles this correctly by warning and falling back to localHostId; this branch should do the same.
🛠️ Proposed fix
const manifest = readManifest(options.organizationId);
if (manifest && isProcessAlive(manifest.pid)) {
ids.add(localHostId);
}
targetHostIds = [...ids];
+ if (targetHostIds.length === 0) {
+ warnings.push(
+ "No hosts are currently online; listing this machine's host only",
+ );
+ targetHostIds = [localHostId];
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let targetHostIds: string[]; | |
| if (options.hostId) { | |
| targetHostIds = [options.hostId]; | |
| } else if (hosts.length > 0) { | |
| const ids = new Set( | |
| hosts.filter((host) => host.online).map((host) => host.id), | |
| ); | |
| // Cloud presence can lag: always include this machine's host when its | |
| // host-service manifest points at a live process. | |
| const manifest = readManifest(options.organizationId); | |
| if (manifest && isProcessAlive(manifest.pid)) { | |
| ids.add(localHostId); | |
| } | |
| targetHostIds = [...ids]; | |
| } else { | |
| if (discoveryError !== undefined) { | |
| warnings.push( | |
| `Cloud host discovery failed (${describeError(discoveryError)}); listing this machine's host only`, | |
| ); | |
| } | |
| targetHostIds = [localHostId]; | |
| } | |
| let targetHostIds: string[]; | |
| if (options.hostId) { | |
| targetHostIds = [options.hostId]; | |
| } else if (hosts.length > 0) { | |
| const ids = new Set( | |
| hosts.filter((host) => host.online).map((host) => host.id), | |
| ); | |
| // Cloud presence can lag: always include this machine's host when its | |
| // host-service manifest points at a live process. | |
| const manifest = readManifest(options.organizationId); | |
| if (manifest && isProcessAlive(manifest.pid)) { | |
| ids.add(localHostId); | |
| } | |
| targetHostIds = [...ids]; | |
| if (targetHostIds.length === 0) { | |
| warnings.push( | |
| "No hosts are currently online; listing this machine's host only", | |
| ); | |
| targetHostIds = [localHostId]; | |
| } | |
| } else { | |
| if (discoveryError !== undefined) { | |
| warnings.push( | |
| `Cloud host discovery failed (${describeError(discoveryError)}); listing this machine's host only`, | |
| ); | |
| } | |
| targetHostIds = [localHostId]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/lib/host-workspaces/listHostWorkspaces.ts` around lines 63 -
84, Update the host-selection logic in listHostWorkspaces so that when discovery
returns hosts but the online-host Set is empty after the local manifest check,
it adds a warning explaining that no discovered hosts are reachable and falls
back to localHostId. Preserve the existing targetHostIds behavior when reachable
hosts are present and retain the discovery-error fallback in the sibling branch.
Adds a public API to message an already-running terminal agent (claude/codex) instead of re-spawning a session per message via agents.create — which spiked terminal-session counts and started a fresh CLI each time rather than continuing the conversation. Host (host-service): - ModeTracker.isBracketedPasteActive() + snapshot() off the existing per-session headless xterm; scrollback 1 -> 1000 so snapshot serves line-mode history. - writeFramedInputToSession (bracketed-paste framing + Enter) and snapshotSession. Both adopt daemon-owned sessions on an in-memory miss (same adoptOnly path pane auto-adoption uses), so background agents stay reachable across host-service restarts. - terminal.send mutation + terminal.snapshot query. Public surfaces — the terminals resource gains list/send/read/close alongside create, following the explicit-host-targeting convention (#5885): - MCP v2: terminals_list/_send/_read/_close with tool annotations - SDK: client.terminals.list/send/read/close - CLI: superset terminals list/send/read/close (--host defaults to this machine) close reuses the existing terminal.killSession; list reuses terminal.listSessions (daemon+DB truth, #5823). No pty-daemon or protocol changes — this is the surgical subset of the capability #5784 attempted before its revert (#5837). Recommended flow: agents.create once to start, then terminals.send for follow-ups (0 new sessions) and terminals.read to observe the reply. Claude-Session: https://claude.ai/code/session_01C4cXgxgH3A7k3qHSLEyFNJ
#5256) * feat(terminals): send/read/close follow-ups to running terminal agents Adds a public API to message an already-running terminal agent (claude/codex) instead of re-spawning a session per message via agents.create — which spiked terminal-session counts and started a fresh CLI each time rather than continuing the conversation. Host (host-service): - ModeTracker.isBracketedPasteActive() + snapshot() off the existing per-session headless xterm; scrollback 1 -> 1000 so snapshot serves line-mode history. - writeFramedInputToSession (bracketed-paste framing + Enter) and snapshotSession. Both adopt daemon-owned sessions on an in-memory miss (same adoptOnly path pane auto-adoption uses), so background agents stay reachable across host-service restarts. - terminal.send mutation + terminal.snapshot query. Public surfaces — the terminals resource gains list/send/read/close alongside create, following the explicit-host-targeting convention (#5885): - MCP v2: terminals_list/_send/_read/_close with tool annotations - SDK: client.terminals.list/send/read/close - CLI: superset terminals list/send/read/close (--host defaults to this machine) close reuses the existing terminal.killSession; list reuses terminal.listSessions (daemon+DB truth, #5823). No pty-daemon or protocol changes — this is the surgical subset of the capability #5784 attempted before its revert (#5837). Recommended flow: agents.create once to start, then terminals.send for follow-ups (0 new sessions) and terminals.read to observe the reply. Claude-Session: https://claude.ai/code/session_01C4cXgxgH3A7k3qHSLEyFNJ * test(host-service): e2e coverage for terminal send/snapshot; deterministic adoption replay wait Real-daemon node-test suite (same harness as terminal.adoption.node-test) covering the headless follow-up surface: send delivers text + Enter, submit: false stages without executing, snapshot serves normal and alt-screen buffers plus maxLines, bracketed-paste framing verified byte-for-byte via `cat > f`, restart-adoption for both verbs, and the not-found / exited / cross-workspace error paths. 10 consecutive runs green. Replaces the fixed 300ms post-adoption sleep with a wait for the replayed ring-buffer bytes to quiesce (they accumulate in session.buffer because no renderer is attached right after adoption), capped at 500ms. Typical adoption now settles in ~50ms instead of a blind 300ms, and slow replays no longer race the tracker rebuild. eventBus is optional on the new functions, matching createTerminalSessionInternal. Claude-Session: https://claude.ai/code/session_01C4cXgxgH3A7k3qHSLEyFNJ * test(host-service): exercise terminal.send/snapshot through the tRPC wire layer Same real-daemon harness, now also through appRouter.createCaller — the layer the public MCP/SDK/CLI verbs actually hit. Verifies the zod schemas, the submit-defaults-to-true behavior, and the error mapping: NOT_FOUND for missing terminals, FORBIDDEN for cross-workspace targeting, BAD_REQUEST for empty text, UNAUTHORIZED for unauthenticated callers. The suite now runs with the tsx loader (the router tree uses extensionless imports that --experimental-strip-types cannot resolve). Claude-Session: https://claude.ai/code/session_01C4cXgxgH3A7k3qHSLEyFNJ
Summary
Reverts #5784 (squash commit
8386961b5) to isolate a deterministic regression: existing terminals stop accepting input, and automation-launched terminals break the same way, reliably on every run. Prod (desktop-v1.16.0, daemon 0.2.5) is unaffected; the regression exists only in post-1.16.0 builds carrying daemon 0.2.6.#5784 is the prime suspect because it is the only recent merge on every input path shared by both broken surfaces:
host-serviceDaemonClient(carries all writes: desktop keystrokes and automation prompt delivery)Changes
git revertof8386961b5— daemon back to protocol v2-only, CLIagents sessions list/read/send/waitremoved, launch path restoredisVersionUpdatePendingonly updates daemons running< EXPECTED_DAEMON_VERSION, so restoring 0.2.5 would leave already-upgraded 0.2.6 daemons running the suspect binary forever. 0.2.7 makes them hand off onto the reverted build. fix(pty-daemon): release macOS PTY master descriptors #5795 (fd disposal) merged before feat(cli): add headless agent session control #5784 and is intentionally not reverted.Validation
bun run typecheck— 35/35 tasks pass (confirms nothing merged after feat(cli): add headless agent session control #5784 references the reverted code)bun run lint— cleanbun install --frozen-lockfile— lockfile consistentIf the automation repro passes on this branch, #5784 is confirmed as the cause and can be re-landed piecewise (DaemonClient rewrite, protocol v3, prompt launcher) to find the specific culprit.
Summary by cubic
Reverts headless agent session control and protocol v3 snapshots to fix a regression where terminals stopped accepting input in post-1.16 builds. Restores the prior agent launch path and v2-only daemon protocol; bumps
@superset/pty-daemonto 0.2.7 so hosts on 0.2.6 converge to the reverted build.snapshotmessages and terminal snapshot rendering.superset agents sessions list/read/send/wait), tests, and CDP runbook; docs updated.@superset/pty-daemonbumped to 0.2.7 to replace existing 0.2.6 daemons with the reverted binary.@superset/cli-frameworkarg parser no longer treats-as a stdin sentinel for string options.Written for commit efa6d3c. Summary will update on new commits.
Summary by CodeRabbit
Changes
Documentation
Maintenance