Skip to content

feat(agent): implement /clear command#3546

Open
haacked wants to merge 3 commits into
mainfrom
posthog-code/clear-command
Open

feat(agent): implement /clear command#3546
haacked wants to merge 3 commits into
mainfrom
posthog-code/clear-command

Conversation

@haacked

@haacked haacked commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Problem

Typing /clear in PostHog Code reports "Unsupported slash command". Upstream hid the SDK's own /clear because it is unreliable in this embedding, but users still want a way to drop the conversation and free up context without abandoning the task, including on cloud runs, where the append-only session log is what rehydrates the conversation on resume.

Here's an example session that shows it works.

Screenshot 2026-07-17 at 5 22 23 PM

Refs #670

Changes

  • Claude adapter (packages/agent): prompt() intercepts /clear (never forwarded to the SDK). It retires the current SDK query (reusing the refreshSession teardown, now extracted as retireQuery) and swaps in a brand-new SDK session, fresh id, no resume, under the same ACP session. The ACP session id stays stable; the new Session.sdkSessionId tracks the underlying SDK session, and hasSession accepts either id so cancel/reconnect work from both sides. /clear is advertised in the command list again.
  • Session log / cloud resume: the clear is recorded as a _posthog/conversation_cleared marker plus an updated sdk_session mapping in the append-only log. rebuildConversation (jsonl hydration + ResumeSaga) treats the marker as a boundary, and ResumeSaga.findSessionId picks up the post-clear session id, so cloud sandbox restarts and desktop cold reconnects resume the fresh SDK session with only post-clear turns.
  • UI: the transcript keeps the full history and renders a "Conversation cleared" divider (like the compaction marker); the context indicator resets to zero via a usage_update.
  • Timeout and reconnect hardening: the /clear prompt is only broadcast (and thus logged) once the new session is confirmed live, so a timeout leaves no orphaned entry in the log. A "Clearing…" status indicator mirrors the existing compaction spinner, so a slow or failed clear gives the user feedback instead of going silent, with a clearing_failed row rendering the outcome. On success, the local session file left under the original session id gets deleted so a cold reconnect can't mistake it for current and resume the pre-clear conversation. /clear also now refuses while a turn is queued, not just while one is active.

Codex sessions are unchanged, /clear is Claude-adapter only for now.

How did you test this?

  • New unit tests: adapter /clear behavior (fresh session swap, marker/mapping emission, busy/queued-turn guards, refresh-after-clear, timeout handling, stale jsonl cleanup, resumeSession id matching after a clear), command advertisement, clear-boundary handling in jsonl hydration and ResumeSaga, and the UI divider and status-row rendering (including clearing/clearing_failed).
  • Ran the full @posthog/agent suite, @posthog/ui sessions suite, and pnpm typecheck for agent/core/ui plus Biome on the touched packages.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Why: /clear is a frequently reached-for command that PostHog Code currently rejects; implementing it cloud-first required making the session-log rehydration clear-aware.


Created with PostHog Code

@trunk-io

trunk-io Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit 1c5c6ae.

@haacked
haacked force-pushed the posthog-code/clear-command branch from f58e90b to 30f05dc Compare July 17, 2026 21:12
@haacked
haacked marked this pull request as ready for review July 18, 2026 00:24
haacked and others added 3 commits July 20, 2026 12:04
Intercept /clear in the Claude adapter instead of forwarding it to the SDK: retire the current query and swap in a brand-new SDK session (fresh id, no resume) under the same ACP session. Persist a _posthog/conversation_cleared marker plus an updated sdk_session mapping to the append-only session log, and make the rehydration paths (jsonl hydration, ResumeSaga) treat the marker as a conversation boundary so desktop reconnects and cloud resumes rebuild only the post-clear conversation. The UI renders a "Conversation cleared" divider and resets the context indicator.

Generated-By: PostHog Code
Task-Id: 7f180c25-f99b-4c96-ac84-f39c0e887a37
- Parallelize post-clear notifications instead of sequential awaits
- Use isNotification() helper instead of set-based method checking
- Remove unused variables and redundant state resets
- Improve comments for session id handling
Only broadcast (and thus log) the "/clear" prompt once the new session
is confirmed live, so a timeout leaves no orphaned entry in the log.
Add a "Clearing…" status indicator, mirroring the existing compaction
spinner, so a slow or failed clear gives the user feedback instead of
going silent. Delete the local session file left under the original
session id after a clear so a cold reconnect can't mistake it for
current and resume the pre-clear conversation.
@haacked
haacked force-pushed the posthog-code/clear-command branch from e0d3e15 to 1c5c6ae Compare July 20, 2026 19:44
@haacked
haacked requested review from a team and charlesvien July 20, 2026 19:44
@haacked haacked added reviewhog ($$$) Reviews pull requests before humans do Stamphog This will request an autostamp by stamphog on small changes labels Jul 20, 2026
stamphog[bot]

This comment was marked as off-topic.

@stamphog stamphog Bot removed the Stamphog This will request an autostamp by stamphog on small changes label Jul 21, 2026
@posthog

posthog Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

🦔 ReviewHog reviewed this pull request

Found 0 must fix, 3 should fix, 0 consider.

Published 3 findings (view the review).

@posthog

posthog Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot 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.

ReviewHog Report

Feature

Issues: 2 issues

Files (9)
  • packages/agent/src/acp-extensions.ts
  • packages/core/src/sessions/acpNotifications.ts
  • packages/agent/src/adapters/base-acp-agent.ts
  • packages/agent/src/adapters/claude/types.ts
  • packages/agent/src/adapters/claude/claude-agent.ts
  • packages/agent/src/adapters/claude/session/commands.ts
  • packages/agent/src/adapters/claude/session/jsonl-hydration.ts
  • packages/agent/src/sagas/resume-saga.ts
  • packages/agent/src/adapters/claude/UPSTREAM.md
What were the main changes
  • New _posthog/conversation_cleared notification constant marks the /clear boundary in the append-only session log (agent + core copies)
  • base-acp-agent.hasSession extracted as an overridable hook so cancel/reconnect can match either the ACP session id or a post-clear SDK session id
  • Claude adapter intercepts /clear in prompt(): guards against busy/queued turns, retires the current SDK query via new retireQuery (extracted from refreshSession), and swaps in a fresh SDK session under the same ACP session id, tracked via new Session.sdkSessionId
  • clearConversation broadcasts a 'clearing' status, only persists the /clear prompt once the new session is confirmed live, emits the conversation_cleared marker + sdk_session mapping + zeroed usage_update, deletes the stale local jsonl, and handles timeout with a 'clearing_failed' status and no orphaned log entry
  • /clear re-added to advertised slash commands (adapter-implemented, not forwarded to SDK) via getAvailableSlashCommands
  • rebuildConversation (jsonl hydration) and ResumeSaga.findSessionId/rebuildConversation treat CONVERSATION_CLEARED as a boundary so cloud/desktop resume rehydrate only post-clear turns using the fresh SDK session id
  • UPSTREAM.md updated to document that /clear is now reimplemented locally, superseding the prior 'hide /clear' note

Frontend

Issues: 1 issue

Files (4)
  • packages/ui/src/features/sessions/components/buildConversationItems.ts
  • packages/ui/src/features/sessions/components/session-update/ConversationClearedView.tsx
  • packages/ui/src/features/sessions/components/session-update/SessionUpdateView.tsx
  • packages/ui/src/features/sessions/components/session-update/StatusNotificationView.tsx
What were the main changes
  • buildConversationItems renders a 'conversation_cleared' item on the CONVERSATION_CLEARED notification and handles the new 'clearing'/'clearing_failed' status rows (mirroring compaction)
  • New ConversationClearedView renders a 'Conversation cleared' divider (chat-thread separator or legacy bordered row)
  • SessionUpdateView wires the new 'conversation_cleared' item type to the new view component
  • StatusNotificationView/CompactingStatusView generalized to show a 'Clearing conversation…' spinner and a 'clearing_failed' outcome row, reusing the compaction spinner pattern with a configurable label

Other findings (outside the changed lines)

Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.

Missing isClearing state leaves the generic "Generating…" spinner running alongside the dedicated "Clearing…" row

Priority: should_fix | File: packages/ui/src/features/sessions/components/buildConversationItems.ts:748-781 | Category: bug

Why we think it's a valid issue
  • Checked: handleRuntimeStatus/markRuntimeStatusComplete in buildConversationItems.ts (748-793, 873-894), the isCompacting thread through useConversationItems.ts → ConversationView.tsx:158/468 → SessionFooter.tsx, the SessionFooter gate, StatusNotificationView.tsx clearing render, the adapter's clearConversation (claude-agent.ts:1517-1673) and prompt() dispatch (line 475), and isPromptPending lifecycle in sessionService.ts.
  • Found: b.isCompacting is set only in the compacting branch (buildConversationItems.ts:753) and reset only for "compacting" (line 875); the clearing/clearing_failed branches (765-780) set no analogous flag. BuildResult.isCompacting is the only busy flag threaded to SessionFooter, whose sole gate is if (isPromptPending && !isCompacting) (SessionFooter.tsx:52) — no isClearing prop exists (props at 14-28).
  • Found: /clear runs through the ordinary prompt RPC: prompt() returns clearConversation(params) (claude-agent.ts:475-478), and sendLocalPrompt sets isPromptPending: true (sessionService.ts:2673) flipping it false only after agent.prompt.mutate resolves (2708-2711) — i.e. after the whole clear. Meanwhile clearConversation emits clearing (isComplete omitted → false) at line 1544-1547 before the swap, and StatusNotificationView.tsx:170-179 renders that as a CompactingStatusView label="Clearing conversation…" spinner row.
  • Impact: For the entire duration of every /clear, both the generic footer "Generating…" indicator + spinning SlotMachineLever and the dedicated "Clearing conversation…" row render simultaneously — 100% reproducible, not an edge case. On a slow clear this persists up to SESSION_VALIDATION_TIMEOUT_MS = 30_000 ms (claude-agent.ts:153), the exact scenario the clearing spinner was added for, where the footer misleadingly says the agent is "Generating…". This directly contradicts the PR's stated intent that clearing "mirrors the existing compaction spinner" (compaction suppresses the generic indicator via the !isCompacting gate; clearing does not). The suggested fix (add isClearing, thread it, extend the gate to !isCompacting && !isClearing) is the correct mirror of the compaction path.
Issue description

The new clearing/clearing_failed handling in handleRuntimeStatus mirrors the compacting branches (both push a dedicated status row and complete it via markRuntimeStatusComplete), but it never sets an isClearing-equivalent of b.isCompacting. BuildResult/ItemBuilder expose isCompacting (set true at line 753, reset via markRuntimeStatusComplete(b, "compacting") at line 875) but there is no analogous flag for clearing. Downstream, SessionFooter.tsx:52 uses if (isPromptPending && !isCompacting) to suppress the generic "Generating…" spinner + SlotMachineLever while compaction's dedicated row is shown. Since /clear is dispatched as an ordinary session/prompt RPC, isPromptPending stays true for the whole clear (it only flips false when the RPC resolves after the entire clear completes), while isCompacting stays false throughout (it's never set for clearing). Verified by tracing useConversationItemsConversationView.tsx:468 / ChatThreadFooter.tsx:62, which pass isCompacting straight from BuildResult.isCompacting with nothing else merged in, and confirming SessionFooter has no isClearing/isBusy prop. Consequently, for the entire duration of every /clear, the generic footer spinner/lever and the dedicated "Clearing conversation…" status row (StatusNotificationView.tsx:170-180) render simultaneously — contradicting the PR's stated intent that the clearing indicator "mirrors the existing compaction spinner" (compaction correctly suppresses the generic indicator; clearing does not).

Suggested fix

Add an isClearing field to ItemBuilder/BuildResult (set true when status.status === "clearing" and not complete, reset to false in markRuntimeStatusComplete when completing a "clearing" row, same as the existing isCompacting handling), thread it through useConversationItems, ConversationView.tsx, and ChatThreadFooter.tsx, and extend SessionFooter.tsx's gate to if (isPromptPending && !isCompacting && !isClearing) so the generic spinner is suppressed during a clear just like it is during compaction.

Comment on lines +1544 to +1547
await this.client.extNotification(POSTHOG_NOTIFICATIONS.STATUS, {
sessionId: params.sessionId,
status: "clearing",
});

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.

clearConversation() has no error/rollback path other than the timeout branch, so any other failure leaves the client spinner stuck and the session silently broken

should_fix bug

Why we think it's a valid issue
  • Checked: clearConversation (claude-agent.ts:1544-1697), retireQuery (1480-1505), withTimeout (utils/common.ts:8-20), the consumer wiring (ensureConsumer 535-548), and the UI spinner lifecycle (buildConversationItems.ts:745-781, StatusNotificationView.tsx).
  • Found: withTimeout only converts the timeout race outcome — operationPromise = operation.then(...) (common.ts:15-19), so a genuine rejection of the wrapped promise propagates. Both awaited withTimeout calls in the clear path can therefore reject non-timeout: retireQuery's await withTimeout(oldConsumer, 5000) (1503) — and the consumer genuinely can reject (the codebase attaches a .catch logging "Consumer terminated unexpectedly" at 542) — and await withTimeout(newQuery.initializationResult(), …) (1584) on SDK subprocess crash / a concurrent abort of the live newAbortController (assigned to the session at 1582).
  • Found: Only the result.result === "timeout" branch (1588-1604) recovers (terminateQuery + queryClosed = true + clearing_failed). There is no surrounding try/catch, and session.query/input/queryOptions/abortController are overwritten at 1579-1582 before the init await at 1584 — so a non-timeout rejection leaves the session pointing at a half-initialized query with queryClosed unset (so the guards at 1521/485 don't fire), and the "clearing" STATUS emitted at 1544-1547 is never resolved.
  • Found: The UI confirms the stuck-spinner harm: the in-progress clearing row is flipped complete only by a clearing status with isComplete (buildConversationItems.ts:765-769) or a clearing_failed (771-780); nothing else (turn-end, error) clears it, so a non-timeout failure leaves "Clearing…" spinning permanently until a reload.
  • Impact: A reachable failure mode (SDK init reject / consumer reject) leaves a permanently stuck spinner plus a half-swapped, non-queryClosed session that accepts further prompts into a dead query — directly defeating the PR's stated "failed clear gives the user feedback" hardening goal. Not defensive paranoia: these rejections are real (the code's own .catch acknowledges consumer rejection), and the author already handles the sibling timeout case identically, so extending it is completing intended behavior.
  • Priority: Lowered must_fix → should_fix: the defect only triggers on an abnormal non-timeout failure during the clear swap (the success and timeout paths are both handled), so it degrades an already-failing operation rather than breaking the happy path; real and worth fixing, but not happy-path/data-loss severity.
Issue description

clearConversation() broadcasts a "clearing" STATUS notification (1544-1547), then does a sequence of operations that can each fail for reasons other than a timeout: retireQuery(session) (1549) awaits withTimeout(oldConsumer, 5000) which does NOT catch a genuine rejection of oldConsumer (only converts the timeout race outcome, per withTimeout's implementation in packages/agent/src/utils/common.ts — a rejection of the wrapped promise still propagates as a rejection); session.buildInProcessMcpServers() (1561) and query({...}) (1577) can throw synchronously (e.g. createLocalToolsMcpServer/resolveGithubToken failures); and newQuery.initializationResult() (1584-1587) can reject for reasons other than timing out (e.g. the SDK subprocess crashing, or the newAbortController being aborted by a concurrent cancel()/interrupt() — see the related concurrency finding). Only the explicit result.result === "timeout" branch (1588-1604) cleans up: it calls terminateQuery, sets session.queryClosed = true, and emits a clearing_failed STATUS notification. Any other exception in this method propagates uncaught: the client's "Clearing…" spinner (from the notification at 1544-1547) is never resolved with either isComplete: true or clearing_failed, so it hangs forever from the user's perspective, and session.query/session.input/session.abortController have already been overwritten with the broken/half-initialized new Query (1579-1582) without session.queryClosed being set, so the session is left in a corrupted state that isn't gated against further prompts the way the timeout path (and closeQueryStream) intends.

Suggested fix

Wrap the risky portion of clearConversation (from retireQuery through the notifications) in a try/catch (or try/finally) that, on ANY error — not just the timeout case — mirrors the timeout branch: call terminateQuery on the new query, set session.queryClosed = true (or otherwise restore a known-good state), and emit a clearing_failed STATUS notification before rethrowing. This guarantees the client never sees an unresolved "clearing" spinner and the session is never left half-swapped.

Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/claude/claude-agent.ts#L1544-1547
@packages/agent/src/adapters/claude/claude-agent.ts#L1576-1587
@packages/agent/src/adapters/claude/claude-agent.ts#L1588-1604

<issue_description>
clearConversation() broadcasts a `"clearing"` STATUS notification (1544-1547), then does a sequence of operations that can each fail for reasons other than a timeout: `retireQuery(session)` (1549) awaits `withTimeout(oldConsumer, 5000)` which does NOT catch a genuine rejection of `oldConsumer` (only converts the *timeout* race outcome, per `withTimeout`'s implementation in `packages/agent/src/utils/common.ts` — a rejection of the wrapped promise still propagates as a rejection); `session.buildInProcessMcpServers()` (1561) and `query({...})` (1577) can throw synchronously (e.g. `createLocalToolsMcpServer`/`resolveGithubToken` failures); and `newQuery.initializationResult()` (1584-1587) can reject for reasons other than timing out (e.g. the SDK subprocess crashing, or the `newAbortController` being aborted by a concurrent `cancel()`/`interrupt()` — see the related concurrency finding). Only the explicit `result.result === "timeout"` branch (1588-1604) cleans up: it calls `terminateQuery`, sets `session.queryClosed = true`, and emits a `clearing_failed` STATUS notification. Any other exception in this method propagates uncaught: the client's "Clearing…" spinner (from the notification at 1544-1547) is never resolved with either `isComplete: true` or `clearing_failed`, so it hangs forever from the user's perspective, and `session.query`/`session.input`/`session.abortController` have already been overwritten with the broken/half-initialized new Query (1579-1582) without `session.queryClosed` being set, so the session is left in a corrupted state that isn't gated against further prompts the way the timeout path (and `closeQueryStream`) intends.
</issue_description>

<issue_validation>
- **Checked:** `clearConversation` (claude-agent.ts:1544-1697), `retireQuery` (1480-1505), `withTimeout` (utils/common.ts:8-20), the consumer wiring (`ensureConsumer` 535-548), and the UI spinner lifecycle (buildConversationItems.ts:745-781, StatusNotificationView.tsx).
- **Found:** `withTimeout` only converts the *timeout* race outcome — `operationPromise = operation.then(...)` (common.ts:15-19), so a genuine rejection of the wrapped promise propagates. Both awaited `withTimeout` calls in the clear path can therefore reject non-timeout: `retireQuery`'s `await withTimeout(oldConsumer, 5000)` (1503) — and the consumer genuinely can reject (the codebase attaches a `.catch` logging "Consumer terminated unexpectedly" at 542) — and `await withTimeout(newQuery.initializationResult(), …)` (1584) on SDK subprocess crash / a concurrent abort of the live `newAbortController` (assigned to the session at 1582).
- **Found:** Only the `result.result === "timeout"` branch (1588-1604) recovers (terminateQuery + `queryClosed = true` + `clearing_failed`). There is no surrounding try/catch, and `session.query`/`input`/`queryOptions`/`abortController` are overwritten at 1579-1582 *before* the init await at 1584 — so a non-timeout rejection leaves the session pointing at a half-initialized query with `queryClosed` unset (so the guards at 1521/485 don't fire), and the `"clearing"` STATUS emitted at 1544-1547 is never resolved.
- **Found:** The UI confirms the stuck-spinner harm: the in-progress `clearing` row is flipped complete only by a `clearing` status with `isComplete` (buildConversationItems.ts:765-769) or a `clearing_failed` (771-780); nothing else (turn-end, error) clears it, so a non-timeout failure leaves "Clearing…" spinning permanently until a reload.
- **Impact:** A reachable failure mode (SDK init reject / consumer reject) leaves a permanently stuck spinner plus a half-swapped, non-`queryClosed` session that accepts further prompts into a dead query — directly defeating the PR's stated "failed clear gives the user feedback" hardening goal. Not defensive paranoia: these rejections are real (the code's own `.catch` acknowledges consumer rejection), and the author already handles the sibling timeout case identically, so extending it is completing intended behavior.
- **Priority:** Lowered must_fix → should_fix: the defect only triggers on an abnormal non-timeout failure during the clear swap (the success and timeout paths are both handled), so it degrades an already-failing operation rather than breaking the happy path; real and worth fixing, but not happy-path/data-loss severity.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Wrap the risky portion of clearConversation (from `retireQuery` through the notifications) in a try/catch (or try/finally) that, on ANY error — not just the timeout case — mirrors the timeout branch: call `terminateQuery` on the new query, set `session.queryClosed = true` (or otherwise restore a known-good state), and emit a `clearing_failed` STATUS notification before rethrowing. This guarantees the client never sees an unresolved "clearing" spinner and the session is never left half-swapped.
</potential_solution>

Comment on lines +475 to +479
if (commandMatch?.[1] === "/clear") {
// Handled by the adapter, never forwarded to the SDK (whose own /clear
// is unreliable in this embedding — see UPSTREAM.md "Hide /clear").
return this.clearConversation(params);
}

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.

/clear bypasses the turn-queue concurrency guard entirely, letting a concurrent /clear, prompt, or cancel race the in-flight session swap

should_fix bug

Why we think it's a valid issue
  • Checked: the /clear short-circuit in prompt() (claude-agent.ts:475-479) vs the normal turn-registration path (515-532), clearConversation's guard (1520-1549), the Session type (types.ts — fields turnQueue/activeTurn/queryClosed, no clearing), interrupt() (1387-1423), and the ACP transport's dispatch loop in @agentclientprotocol/sdk.
  • Found: The transport does not serialize per-session handlers — jsonrpc.js:521-537 receiveMessage fires void this.processIncomingMessage(...) and the receive() read-loop (487-505) immediately reads the next message without awaiting the handler, so two prompt/cancel handlers run concurrently, interleaving at await points. This is the same concurrency the codebase's turnQueue/activeTurn/forceCancelTimer machinery exists to tame.
  • Found: /clear returns this.clearConversation(params) at 478 before the turnQueue.push(turn) at 529, so a clear registers nothing. clearConversation's sole guard (1524) checks activeTurn !== null || turnQueue.length > 0 and there is no clearing flag on Session — so a second concurrent /clear sees a clean state and also passes, and queryClosed isn't set on the success path either (only at timeout 1592). Two clearConversation bodies then race the same session, both reassigning session.query/input/abortController/sdkSessionId (1579-1582); the first clear's freshly-created Query is orphaned and never terminated (only the timeout branch at 1588-1604 calls terminateQuery), leaking that SDK subprocess/connection.
  • Found: A concurrent cancel()interrupt() (1422 operates on the live session.query) or the abort of the just-installed newAbortController (1582) can reject the in-flight init, and a /clear+prompt overlap can push a user message to the old, already-end()-ed input from retireQuery (1500), silently dropping it.
  • Impact: Confirmed race on shared mutable session state with a leaked SDK subprocess and possible silent message loss / corrupted swap — squarely the "race conditions that corrupt shared state" + "leaked connections" keep categories, and reachable via a plausible double-fire (double-click, client retry, or /clear immediately followed by another action) within the sub-second swap window; not a never-gonna-happen edge case given this codebase's concurrent dispatch model. The trivial fix (a synchronously-set session.clearing flag, or registering /clear as a pseudo-turn) is well-scoped.
  • Priority: Lowered must_fix → should_fix: the common single-/clear flow is correctly guarded against in-flight normal turns, so the residual gap is the narrower concurrent clear/prompt/cancel timing window with restart-recoverable impact — real and worth fixing, but not happy-path/unrecoverable severity that must block merge.
Issue description

prompt() short-circuits /clear straight into clearConversation(params) (475-479) before any turnQueue/activeTurn registration happens — unlike every other prompt, which gets queued via session.turnQueue.push(turn) (see the normal path later in prompt()). clearConversation's only guard against concurrency is a one-time synchronous check of session.activeTurn !== null || session.turnQueue.length > 0 (1524), which protects against a normal turn being in flight but does nothing to prevent a second, overlapping clearConversation call: nothing on Session (checked in packages/agent/src/adapters/claude/types.ts) marks "a clear is currently in progress." The ACP transport (@agentclientprotocol/sdk's Connection.receive()/processIncomingMessage) dispatches every incoming JSON-RPC message via void this.processIncomingMessage(...) without awaiting the handler or serializing per-session — this codebase's own turnQueue/activeTurn/forceCancelTimer machinery exists specifically because concurrent prompt()+cancel() (and by extension a double /clear) is an expected, designed-for scenario, not a theoretical one. Concretely: a user double-sending /clear (double click, client retry), or sending /clear immediately followed by another prompt or a session/cancel, can run two overlapping clearConversation bodies (or a clearConversation racing interrupt()) against the same shared session object. Both mutate session.query, session.input, session.abortController, session.sdkSessionId, session.taskState, this.toolUseStreamCache, and this.emittedToolCalls without coordination — whichever call finishes last silently wins, and the "losing" call's freshly-created Query (and its underlying SDK subprocess/connection) is never terminated (only the timeout branch calls terminateQuery), leaking that resource. A concurrent cancel() mid-clear can also abort the very newAbortController that clearConversation just installed, feeding directly into the failure mode described in the previous finding.

Suggested fix

Add a session-level guard (e.g. a session.clearing: boolean, checked and set synchronously alongside the existing activeTurn/turnQueue check before the first await) so a second /clear — or a cancel() — arriving while one is already in flight is rejected or deferred instead of racing the swap. Alternatively, register /clear as a pseudo-Turn (push it onto turnQueue/activeTurn) so the existing turn-queue and forceCancelTimer machinery naturally serializes it the same way it already serializes normal prompts and cancels.

Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/claude/claude-agent.ts#L475-479
@packages/agent/src/adapters/claude/claude-agent.ts#L1520-1549

<issue_description>
`prompt()` short-circuits `/clear` straight into `clearConversation(params)` (475-479) before any `turnQueue`/`activeTurn` registration happens — unlike every other prompt, which gets queued via `session.turnQueue.push(turn)` (see the normal path later in `prompt()`). `clearConversation`'s only guard against concurrency is a one-time synchronous check of `session.activeTurn !== null || session.turnQueue.length > 0` (1524), which protects against a normal turn being in flight but does nothing to prevent a second, overlapping `clearConversation` call: nothing on `Session` (checked in `packages/agent/src/adapters/claude/types.ts`) marks "a clear is currently in progress." The ACP transport (`@agentclientprotocol/sdk`'s `Connection.receive()`/`processIncomingMessage`) dispatches every incoming JSON-RPC message via `void this.processIncomingMessage(...)` without awaiting the handler or serializing per-session — this codebase's own `turnQueue`/`activeTurn`/`forceCancelTimer` machinery exists specifically because concurrent `prompt()`+`cancel()` (and by extension a double `/clear`) is an expected, designed-for scenario, not a theoretical one. Concretely: a user double-sending `/clear` (double click, client retry), or sending `/clear` immediately followed by another prompt or a `session/cancel`, can run two overlapping `clearConversation` bodies (or a `clearConversation` racing `interrupt()`) against the *same* shared `session` object. Both mutate `session.query`, `session.input`, `session.abortController`, `session.sdkSessionId`, `session.taskState`, `this.toolUseStreamCache`, and `this.emittedToolCalls` without coordination — whichever call finishes last silently wins, and the "losing" call's freshly-created `Query` (and its underlying SDK subprocess/connection) is never terminated (only the timeout branch calls `terminateQuery`), leaking that resource. A concurrent `cancel()` mid-clear can also abort the very `newAbortController` that `clearConversation` just installed, feeding directly into the failure mode described in the previous finding.
</issue_description>

<issue_validation>
- **Checked:** the `/clear` short-circuit in `prompt()` (claude-agent.ts:475-479) vs the normal turn-registration path (515-532), `clearConversation`'s guard (1520-1549), the `Session` type (types.ts — fields `turnQueue`/`activeTurn`/`queryClosed`, no `clearing`), `interrupt()` (1387-1423), and the ACP transport's dispatch loop in `@agentclientprotocol/sdk`.
- **Found:** The transport does not serialize per-session handlers — `jsonrpc.js:521-537` `receiveMessage` fires `void this.processIncomingMessage(...)` and the `receive()` read-loop (487-505) immediately reads the next message without awaiting the handler, so two `prompt`/`cancel` handlers run concurrently, interleaving at `await` points. This is the same concurrency the codebase's `turnQueue`/`activeTurn`/`forceCancelTimer` machinery exists to tame.
- **Found:** `/clear` returns `this.clearConversation(params)` at 478 before the `turnQueue.push(turn)` at 529, so a clear registers nothing. `clearConversation`'s sole guard (1524) checks `activeTurn !== null || turnQueue.length > 0` and there is no `clearing` flag on `Session` — so a second concurrent `/clear` sees a clean state and also passes, and `queryClosed` isn't set on the success path either (only at timeout 1592). Two `clearConversation` bodies then race the same `session`, both reassigning `session.query`/`input`/`abortController`/`sdkSessionId` (1579-1582); the first clear's freshly-created `Query` is orphaned and never terminated (only the timeout branch at 1588-1604 calls `terminateQuery`), leaking that SDK subprocess/connection.
- **Found:** A concurrent `cancel()`→`interrupt()` (1422 operates on the live `session.query`) or the abort of the just-installed `newAbortController` (1582) can reject the in-flight init, and a `/clear`+prompt overlap can push a user message to the old, already-`end()`-ed input from `retireQuery` (1500), silently dropping it.
- **Impact:** Confirmed race on shared mutable session state with a leaked SDK subprocess and possible silent message loss / corrupted swap — squarely the "race conditions that corrupt shared state" + "leaked connections" keep categories, and reachable via a plausible double-fire (double-click, client retry, or /clear immediately followed by another action) within the sub-second swap window; not a never-gonna-happen edge case given this codebase's concurrent dispatch model. The trivial fix (a synchronously-set `session.clearing` flag, or registering /clear as a pseudo-turn) is well-scoped.
- **Priority:** Lowered must_fix → should_fix: the common single-/clear flow is correctly guarded against in-flight normal turns, so the residual gap is the narrower concurrent clear/prompt/cancel timing window with restart-recoverable impact — real and worth fixing, but not happy-path/unrecoverable severity that must block merge.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add a session-level guard (e.g. a `session.clearing: boolean`, checked and set synchronously alongside the existing `activeTurn`/`turnQueue` check before the first `await`) so a second `/clear` — or a `cancel()` — arriving while one is already in flight is rejected or deferred instead of racing the swap. Alternatively, register `/clear` as a pseudo-`Turn` (push it onto `turnQueue`/`activeTurn`) so the existing turn-queue and `forceCancelTimer` machinery naturally serializes it the same way it already serializes normal prompts and cancels.
</potential_solution>

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

Labels

reviewhog ($$$) Reviews pull requests before humans do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant