Add /btw command for quick side questions#3733
Conversation
|
Merging to
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 |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 2 consider. Published 4 findings (view the review). |
|
✅ Visual changes approved by @haacked — baseline updated in 4 new. |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/ui/src/features/sessions/sideQuestionStore.ts:16
**Result Outlives Its Session Run**
The store keys answers only by `taskId`, while `/btw` answers are based on one specific session transcript. If that task reconnects or starts a new run before the old request settles, the completion is accepted under the same key and `SessionView` displays an answer from the previous transcript above the new session; completed cards also remain after a run changes. Include `taskRunId` in the entry and settlement check, or clear the entry when the active run changes.
### Issue 2 of 2
packages/workspace-server/src/services/agent/agent.ts:1404-1421
**Side Question Is Idle During Work**
This resets the idle timer only when the request starts and deliberately leaves `promptPending` unchanged. If the fork is still answering when that timer expires, `killIdleSession` sees neither a pending prompt nor an in-flight tool call and cleans up the session underneath this request, so a valid side question can fail and its isolated SDK fork may continue until its own timeout. Track the side question as active work, or refresh/release the idle lifecycle around the awaited extension call.
Reviews (1): Last reviewed commit: "chore(visual): update storybook baseline..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| interface SideQuestionState { | ||
| /** Latest side question per task. Ephemeral: never persisted, never part of session history. */ | ||
| byTaskId: Record<string, SideQuestionEntry>; |
There was a problem hiding this comment.
The store keys answers only by taskId, while /btw answers are based on one specific session transcript. If that task reconnects or starts a new run before the old request settles, the completion is accepted under the same key and SessionView displays an answer from the previous transcript above the new session; completed cards also remain after a run changes. Include taskRunId in the entry and settlement check, or clear the entry when the active run changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/sessions/sideQuestionStore.ts
Line: 16
Comment:
**Result Outlives Its Session Run**
The store keys answers only by `taskId`, while `/btw` answers are based on one specific session transcript. If that task reconnects or starts a new run before the old request settles, the completion is accepted under the same key and `SessionView` displays an answer from the previous transcript above the new session; completed cards also remain after a run changes. Include `taskRunId` in the entry and settlement check, or clear the entry when the active run changes.
How can I resolve this? If you propose a fix, please make it concise.| async sideQuestion( | ||
| sessionId: string, | ||
| question: string, | ||
| ): Promise<SideQuestionOutput> { | ||
| const session = this.sessions.get(sessionId); | ||
| if (!session) { | ||
| throw new Error(`Session not found: ${sessionId}`); | ||
| } | ||
|
|
||
| session.lastActivityAt = Date.now(); | ||
| this.recordActivity(sessionId); | ||
|
|
||
| const result = await session.clientSideConnection.extMethod( | ||
| POSTHOG_METHODS.SIDE_QUESTION, | ||
| { sessionId: getAgentSessionId(session), question }, | ||
| ); | ||
| return sideQuestionOutput.parse(result); | ||
| } |
There was a problem hiding this comment.
This resets the idle timer only when the request starts and deliberately leaves promptPending unchanged. If the fork is still answering when that timer expires, killIdleSession sees neither a pending prompt nor an in-flight tool call and cleans up the session underneath this request, so a valid side question can fail and its isolated SDK fork may continue until its own timeout. Track the side question as active work, or refresh/release the idle lifecycle around the awaited extension call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/workspace-server/src/services/agent/agent.ts
Line: 1404-1421
Comment:
**Side Question Is Idle During Work**
This resets the idle timer only when the request starts and deliberately leaves `promptPending` unchanged. If the fork is still answering when that timer expires, `killIdleSession` sees neither a pending prompt nor an in-flight tool call and cleans up the session underneath this request, so a valid side question can fail and its isolated SDK fork may continue until its own timeout. Track the side question as active work, or refresh/release the idle lifecycle around the awaited extension call.
How can I resolve this? If you propose a fix, please make it concise.
Adds a Claude Code-style /btw command: a single-turn, tool-less query forked off the live session transcript that answers without entering the conversation. The exchange renders as an ephemeral card above the composer and leaves no trace in session history, working both mid-turn and while idle. - Adapter answers via a one-shot query() with resume + forkSession, maxTurns 1, an empty toolset, and an always-deny canUseTool, exposed through a new _posthog/side_question extension method. - The sideQuestion capability is advertised in initialize() and plumbed through workspace-server, host-router, and sessionService, gated in the UI by sessionSupportsSideQuestion (local Claude sessions only). Generated-By: PostHog Code Task-Id: c8fbfbf7-10b8-4d4b-8088-c0e9d76075e9
- Collapse the per-capability extraction helpers into one extractPosthogCapabilities (workspace-server) and readCapabilities (core) so future capabilities extend an object instead of cloning a cast chain. - Make extMethod dispatch symmetric: refresh_session validation moves into handleRefreshSession beside answerSideQuestion. - Move the fork's message-collection loop into collectSideQuestionAnswer in side-question.ts, flattening answerSideQuestion. - Model SideQuestionEntry as a status-discriminated union, use crypto.randomUUID for entry ids, and extract the fire-and-forget saga into fireSideQuestion beside the store, using getErrorMessage so tRPC error shapes surface their real message. - Clear withTimeout's timer once the race settles so a successful side question doesn't leave a 120s timer alive. Generated-By: PostHog Code Task-Id: c8fbfbf7-10b8-4d4b-8088-c0e9d76075e9
- Correct AgentService.sideQuestion's doc comment: it does reset the session's idle-kill timer via recordActivity, contrary to what the comment claimed - Rename the dropped sessionId destructure target from _drop to _sessionId for clarity - Add an aria-live region to SideQuestionCard so the answer is announced to screen readers - Add a title tooltip so a truncated question is still readable on hover - Add test coverage: the in-flight guard clearing after a failed side question, the stale-answer guard in sideQuestionStore, the SideQuestionCard render states and dismiss button, the askSideQuestion gating in useSessionCallbacks, and AgentService.sideQuestion end to end
…uring them Key answers and cards by taskRunId so a completed /btw answer never displays under a session that reconnected or started a new run. Track pending side questions on the managed session so the idle timer does not reap a session while a side question is still awaiting its answer.
bbe345d to
64769b1
Compare
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| )} | ||
| {entry.status === "done" && ( | ||
| <Box className="max-h-64 overflow-y-auto text-[13px] text-gray-12"> | ||
| <MarkdownRenderer content={entry.answer} /> |
There was a problem hiding this comment.
High: Remote images can exfiltrate transcript data
MarkdownRenderer renders Markdown images using the default <img> component. If entry.answer contains a Markdown image whose URL includes conversation or source data, Chromium will request that URL automatically without a click. Block remote images in this renderer.
| <MarkdownRenderer content={entry.answer} /> | |
| <MarkdownRenderer | |
| content={entry.answer} | |
| componentsOverride={{ | |
| img: ({ alt }) => ( | |
| <Text className="text-gray-9 text-[13px]"> | |
| Remote image blocked{alt ? `: ${alt}` : ""} | |
| </Text> | |
| ), | |
| }} | |
| /> |
PR overviewThis pull request adds a There is one open security concern in the side-question answer rendering path. Markdown answers currently allow remote images to load automatically, which could cause browser requests to attacker-controlled URLs and leak conversation-derived data embedded in those URLs. No issues have been addressed yet, so the PR still needs a rendering hardening change before the feature is safe to merge. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 1 issue
Files (5)
packages/agent/src/acp-extensions.tspackages/agent/src/adapters/claude/claude-agent.tspackages/agent/src/adapters/claude/side-question.tspackages/agent/src/index.tspackages/agent/src/utils/common.ts
What were the main changes
- Adds the
_posthog/side_questionACP extension method and exports it from the agent package - Implements ClaudeAcpAgent.answerSideQuestion: forks the live session's transcript into a single-turn, tool-less query (tools: [], allowedTools: [], mcpServers: {}, always-deny canUseTool) with its own session id and AbortController so the main conversation is untouched
- Adds side-question.ts helpers: buildSideQuestionPrompt (system-reminder wrapper) and collectSideQuestionAnswer (drains SDK message stream)
- Advertises the new
sideQuestioncapability alongsidesteeringduring initialize() - Fixes a timer leak in withTimeout (utils/common.ts) by clearing the timeout in a finally block, used by the side-question timeout path
Frontend
Issues: 3 issues
Files (5)
packages/ui/src/features/message-editor/commands.tspackages/ui/src/features/sessions/components/SideQuestionCard.tsxpackages/ui/src/features/sessions/components/SessionView.tsxpackages/ui/src/features/sessions/hooks/useSessionCallbacks.tspackages/ui/src/features/sessions/sideQuestionStore.ts
What were the main changes
- Adds the
/btwcomposer command that fires a side question via ctx.askSideQuestion, with toasts for missing question/unsupported session - New sideQuestionStore (zustand) tracking a per-task pending/done/error entry, with a stale-answer guard (settle only if the entry id still matches) and fireSideQuestion fire-and-forget saga
- New SideQuestionCard component rendered above the composer: dismissible card with truncated-question tooltip, aria-live status region, spinner/markdown/error states
- useSessionCallbacks wires askSideQuestion into the command context, gated by sessionSupportsSideQuestion so it's only offered for local (non-cloud) sessions
- SessionView renders above the message queue dock
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.
SideQuestionCard is wrapped in the isRunning-gated fade box, so a resolved answer can go invisible and undismissable whenever the local session isn't "connected"
Priority: should_fix | File: packages/ui/src/features/sessions/components/SessionView.tsx:668-678 | Category: bug
Why we think it's a valid issue
- Checked:
SessionView.tsx:667-730(the composer wrapperBox),SideQuestionCard.tsx,sideQuestionStore.ts,sessionViewState.ts,sessionService.tsstatus transitions, andsessionSupportsSideQuestioninpackages/shared/src/sessions.ts. - Found (gating chain holds):
sessionSupportsSideQuestionreturns true only for non-cloud Claude sessions (sessions.ts:282—if (session.isCloud) return false), and for non-cloud sessionsderiveSessionViewStatesetsisRunning = session?.status === "connected"(sessionViewState.ts:40). SoisRunninggoes false on any status other than"connected". - Found (the coupling):
SideQuestionCardatSessionView.tsx:675sits inside theBoxopened at:667, whose className appliespointer-events-none translate-y-4 opacity-0when!isRunning(:670-671). The card is therefore hidden and non-interactive whenever the session isn't"connected". - Found (trigger reachable):
sessionService.tsflips local sessions out of"connected"during normal operation — reconnect sets"connecting"(:6237,:6295), disconnect handlers set"disconnected"(:1767,:2459,:5690), and idle-kill/errors set"error". The composer branch still renders in these states (it's theelseafterisInitializing, which is false onceeventsis non-empty), so the card is actively hidden rather than the whole view being replaced. - Found (answer is non-recoverable): the store entry lives only in ephemeral view state (
sideQuestionStore.ts:16, keyed by taskId, never persisted or added to the transcript) andfireSideQuestion(:74) is fire-and-forget, fully decoupled from the session lifecycle — it never re-fires or surfaces elsewhere. QueuedMessagesDock (same wrapper,:676) is safe by contrast because queued prompts resend on reconnect. - Impact: A pending or already-resolved side-question answer becomes invisible and un-dismissible during any connection hiccup. Transient reconnect blips self-heal (the store entry survives the CSS hiding and reappears when status returns to
"connected"), but terminal transitions — idle-kill (status"error",idleKilled=true→hasErrorfalse → composer renders with the card hidden), a non-retryable error, or the user clicking New Session/Retry — strand the answer permanently with no other surface to read it from. Concrete, directly introduced by this PR, and the suggested fix (render as a sibling outside the gatedBox) is sound.
Issue description
deriveSessionViewState (packages/core/src/sessions/sessionViewState.ts) sets isRunning = session?.status === "connected" for non-cloud sessions — the only sessions /btw is offered on per sessionSupportsSideQuestion. SessionView.tsx wraps the whole composer stack, including the newly added {taskId && <SideQuestionCard taskId={taskId} />} (line 675), inside a Box (line 668) whose className applies opacity-0 pointer-events-none whenever isRunning is false, i.e. whenever session.status is "connecting", "disconnected", or "error". fireSideQuestion is a fire-and-forget saga fully decoupled from the main session's connection lifecycle, so its entry in useSideQuestionStore can resolve to "done" or "error" at any point regardless of what the primary session is doing. If the session's status flips away from "connected" (a reconnect blip, an idle-kill, a worktree error) while that entry is pending or already resolved, the card — including an answer the user is still waiting to read — becomes fully invisible and non-interactive (can't even be dismissed) until the session reconnects, if it ever does. This is different from QueuedMessagesDock sharing the same wrapper: queued prompts are safe to hide because they simply resend on reconnect, but a resolved side-question entry never re-fires or surfaces anywhere else, so hiding it can permanently strand the answer from the user (e.g. if they click "New Session"/"Retry" instead of waiting for reconnection).
Suggested fix
Render SideQuestionCard outside the isRunning-gated opacity/pointer-events Box (e.g. as a sibling that always mounts once taskId is known) so a pending/done/error side-question entry stays visible and dismissible independent of the underlying session's connection status.
| /** | ||
| * Answers a "/btw" side question by forking the live session's transcript | ||
| * into a one-shot, tool-less, single-turn query. Strictly non-mutating: | ||
| * the fork gets its own SDK session id and AbortController, and nothing on | ||
| * `this.session` is touched, so the main conversation (including an | ||
| * in-flight turn) never sees the exchange. | ||
| */ | ||
| private async answerSideQuestion( | ||
| question: string, | ||
| ): Promise<{ answer: string }> { | ||
| if (this.sideQuestionInFlight) { | ||
| throw new RequestError(-32002, "A side question is already in progress"); | ||
| } | ||
| this.sideQuestionInFlight = true; | ||
|
|
||
| const abortController = new AbortController(); | ||
| try { | ||
| // Drop `sessionId` (identity comes from `resume`) and `hooks` (they | ||
| // close over live-session caches and task state). | ||
| const { | ||
| sessionId: _sessionId, | ||
| hooks: _hooks, | ||
| ...rest | ||
| } = this.session.queryOptions; | ||
| const options: Options = { | ||
| ...rest, | ||
| resume: this.sessionId, | ||
| forkSession: true, | ||
| maxTurns: 1, | ||
| // Belt and braces: remove the toolset entirely and deny anything | ||
| // that slips through; the prompt wrapper also says "no tools". | ||
| tools: [], | ||
| allowedTools: [], | ||
| canUseTool: async () => ({ | ||
| behavior: "deny", | ||
| message: "Tools are unavailable while answering a side question.", | ||
| interrupt: false, | ||
| }), | ||
| // Never reuse in-process MCP instances ("Already connected to a | ||
| // transport"); the fork has no tools, so it needs no servers. | ||
| mcpServers: {}, | ||
| abortController, | ||
| // `rest.model` is the creation-time value; the user may have | ||
| // switched models since, so answer on the live session model. | ||
| ...(this.session.modelId && { | ||
| model: toSdkModelId(this.session.modelId), | ||
| }), | ||
| }; | ||
|
|
||
| const oneShot = query({ | ||
| prompt: buildSideQuestionPrompt(question), | ||
| options, | ||
| }); | ||
|
|
||
| const answer = await withTimeout( | ||
| collectSideQuestionAnswer(oneShot), | ||
| SIDE_QUESTION_TIMEOUT_MS, | ||
| ); | ||
|
|
||
| if (answer.result === "timeout") { | ||
| throw new RequestError( | ||
| -32603, | ||
| `Side question timed out after ${SIDE_QUESTION_TIMEOUT_MS}ms`, | ||
| ); | ||
| } | ||
| if (!answer.value) { | ||
| throw new RequestError(-32603, "Side question produced no answer"); | ||
| } | ||
| return { answer: answer.value }; | ||
| } catch (error) { | ||
| if (error instanceof RequestError) throw error; | ||
| // A brand-new session has no transcript on disk yet, so `resume` has | ||
| // nothing to fork; surface that case clearly. | ||
| throw new RequestError( | ||
| -32603, | ||
| `Side question failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| } finally { | ||
| abortController.abort(); | ||
| this.sideQuestionInFlight = false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Side-question fork's AbortController is never wired into session cancel/close, so it can outlive the session
Why we think it's a valid issue
- Checked: The teardown path the finding relies on —
BaseAcpAgent.cancel()/closeSession()(base-acp-agent.ts:74-101) andClaudeAcpAgent.interrupt()(claude-agent.ts:1436-1472) — versus the side-question controller's scope (claude-agent.ts:1555, 1617-1619). - Found: Confirmed accurate:
closeSessionaborts onlythis.session.abortControllerand callsinterrupt(), which acts onsession.query/session.cancelController— none of which is the fork's controller.answerSideQuestion'sabortControlleris a method-local const, never stored onthis.session, so a mid-flight/btwfork is not aborted by cancel/close. (TherefreshSessionanalogy is loose — it reassignsprev.abortControllerbecause it replaces the live query, not to register an auxiliary controller — but the reachability gap the finding describes is real.) - Impact: The consequence is bounded and self-healing, not a true leak: the
finally(claude-agent.ts:1617-1619) aborts the fork when the query completes or whenwithTimeouthitsSIDE_QUESTION_TIMEOUT_MS(120s), so the orphan window is ≤120s and typically a few seconds (a one-shot answer).sideQuestionInFlightprevents pile-up, and the feature is local-only (sessionSupportsSideQuestionreturns false for cloud), so the wasted resource is one short query on the user's own machine/API budget after they closed the session. - Priority: Down-ranked should_fix →
consider. The gap is genuine (namable trigger: close/cancel while a fork streams; namable consequence: a subprocess + API call surviving teardown), but bounded ≤120s, auto-cleaned, single short query, narrow window, and local-only — well below should_fix. Real-but-minor, so keep it on record at the lowest severity rather than dismiss.
Issue description
answerSideQuestion creates its own local AbortController (const abortController = new AbortController();) that is only aborted in the method's own finally block. It is never stored on this.session (or any other instance field reachable from cancel()/closeSession()), unlike the sibling refreshSession method a few lines below, which explicitly stores its controller as prev.abortController so BaseAcpAgent.closeSession()/cancel() (this.session.abortController.abort()) can reach and abort it. If a client cancels or closes the session while a /btw side question is in flight, the forked one-shot query (its own Claude CLI subprocess) keeps running untouched — it will only stop when it finishes naturally or when SIDE_QUESTION_TIMEOUT_MS (120s) elapses inside withTimeout, whichever comes first. That is up to two minutes of an orphaned subprocess/API call surviving past session teardown, burning API cost and resources for a session the user already closed.
Suggested fix
Track the side-question AbortController somewhere the session lifecycle can reach, e.g. store it as this.session.sideQuestionAbortController (mirroring prev.abortController in refreshSession) and have cancel()/closeSession() abort it alongside this.session.abortController, or abort it directly from an overridden cancel()/closeSession() in ClaudeAcpAgent. That way closing/cancelling the session promptly tears down any in-flight side question instead of leaving it to time out on its own.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/claude/claude-agent.ts#L1540-1622
<issue_description>
`answerSideQuestion` creates its own local `AbortController` (`const abortController = new AbortController();`) that is only aborted in the method's own `finally` block. It is never stored on `this.session` (or any other instance field reachable from `cancel()`/`closeSession()`), unlike the sibling `refreshSession` method a few lines below, which explicitly stores its controller as `prev.abortController` so `BaseAcpAgent.closeSession()`/`cancel()` (`this.session.abortController.abort()`) can reach and abort it. If a client cancels or closes the session while a `/btw` side question is in flight, the forked one-shot query (its own Claude CLI subprocess) keeps running untouched — it will only stop when it finishes naturally or when `SIDE_QUESTION_TIMEOUT_MS` (120s) elapses inside `withTimeout`, whichever comes first. That is up to two minutes of an orphaned subprocess/API call surviving past session teardown, burning API cost and resources for a session the user already closed.
</issue_description>
<issue_validation>
- **Checked:** The teardown path the finding relies on — `BaseAcpAgent.cancel()`/`closeSession()` (base-acp-agent.ts:74-101) and `ClaudeAcpAgent.interrupt()` (claude-agent.ts:1436-1472) — versus the side-question controller's scope (claude-agent.ts:1555, 1617-1619).
- **Found:** Confirmed accurate: `closeSession` aborts only `this.session.abortController` and calls `interrupt()`, which acts on `session.query`/`session.cancelController` — none of which is the fork's controller. `answerSideQuestion`'s `abortController` is a method-local const, never stored on `this.session`, so a mid-flight `/btw` fork is not aborted by cancel/close. (The `refreshSession` analogy is loose — it reassigns `prev.abortController` because it *replaces the live query*, not to register an auxiliary controller — but the reachability gap the finding describes is real.)
- **Impact:** The consequence is bounded and self-healing, not a true leak: the `finally` (claude-agent.ts:1617-1619) aborts the fork when the query completes or when `withTimeout` hits `SIDE_QUESTION_TIMEOUT_MS` (120s), so the orphan window is ≤120s and typically a few seconds (a one-shot answer). `sideQuestionInFlight` prevents pile-up, and the feature is local-only (`sessionSupportsSideQuestion` returns false for cloud), so the wasted resource is one short query on the user's own machine/API budget after they closed the session.
- **Priority:** Down-ranked should_fix → `consider`. The gap is genuine (namable trigger: close/cancel while a fork streams; namable consequence: a subprocess + API call surviving teardown), but bounded ≤120s, auto-cleaned, single short query, narrow window, and local-only — well below should_fix. Real-but-minor, so keep it on record at the lowest severity rather than dismiss.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track the side-question `AbortController` somewhere the session lifecycle can reach, e.g. store it as `this.session.sideQuestionAbortController` (mirroring `prev.abortController` in `refreshSession`) and have `cancel()`/`closeSession()` abort it alongside `this.session.abortController`, or abort it directly from an overridden `cancel()`/`closeSession()` in `ClaudeAcpAgent`. That way closing/cancelling the session promptly tears down any in-flight side question instead of leaving it to time out on its own.
</potential_solution>
| dismiss: (taskId) => | ||
| set((state) => { | ||
| const { [taskId]: _dismissed, ...rest } = state.byTaskId; | ||
| return { byTaskId: rest }; | ||
| }), |
There was a problem hiding this comment.
No client-side guard against re-firing a side question while one is in flight — collides with the server's single-flight lock for up to 120s
Why we think it's a valid issue
- Checked: the backend lock in
claude-agent.ts(answerSideQuestion), the timeout constant, the client path (sideQuestionStore.tsfireSideQuestion/dismiss/settle),sessionService.askSideQuestion, and whether any cancel path exists for side questions. - Found (single-flight lock is real):
claude-agent.ts:1550-1551rejects a concurrent call withRequestError(-32002, "A side question is already in progress"); the lock (sideQuestionInFlight) is set at :1553 and only released in thefinallyat :1619 after the fork resolves or times out.SIDE_QUESTION_TIMEOUT_MS = 120_000(side-question.ts:9), so worst-case hold is 120s. - Found (no client guard, no cancel):
fireSideQuestion(sideQuestionStore.ts:74-88) callsask()+askSideQuestionunconditionally;dismiss(:61-65) only deletes thebyTaskIdentry.sessionService.askSideQuestion(sessionService.ts:3581-3602) forwards straight to the tRPC mutation with no in-flight dedupe. The only cancel method anywhere is the main-turnagent.cancel; there is no side-question cancel/abort exposed to the UI, and the fork'sabortControlleris local toanswerSideQuestion(:1555). - Found (silent discard confirmed):
settle(sideQuestionStore.ts:34-43) returns state unchanged whenentry.id !== id. So in the double-ask flow the first request's completed answer is dropped when it resolves (id no longer matches), while the second entry displays the -32002 rejection instead. - Impact: Two plausible flows — ask then dismiss-and-re-ask, or ask twice within the answer window — hit the server lock and surface "A side question is already in progress" on the new card; in the double-ask case a completed, cost-incurred answer is silently discarded. It self-heals once the first fork finishes (seconds typically, up to 120s), so no persistent data loss, but it's a real reliability/correctness rough edge directly introduced here. The author already added the
settlestale-answer guard, showing this concurrency dimension is in scope; the missing in-flight guard is a cheap, consistent complement (checkbyTaskId[taskId]?.status === "pending"before firing). Concrete trigger + concrete consequence, so it clears the bar.
Issue description
fireSideQuestion fires a new askSideQuestion request unconditionally, and dismiss only removes the entry from byTaskId — neither checks nor cancels the in-flight request. The backend agent (ClaudeAcpAgent.answerSideQuestion in packages/agent/src/adapters/claude/claude-agent.ts) allows only one side question at a time per session (sideQuestionInFlight) and rejects a concurrent call with RequestError(-32002, "A side question is already in progress"), and the fork it runs is bounded by a 120s timeout (SIDE_QUESTION_TIMEOUT_MS) with no cancellation hook exposed to the UI at all (no cancel/abort tRPC method for side questions — only the unrelated agent.cancel for the main turn, which explicitly never touches the side-question's own AbortController). So the very plausible flow — ask /btw, dismiss the card because you no longer want the answer, then immediately ask a different /btw question — sends a second request that the server rejects for up to 120 seconds, surfacing a confusing "A side question is already in progress" error in the new card with no way to recover except waiting. The same race happens even without dismissing, if the user asks a second /btw before the first resolves: the first request's real answer/cost is silently discarded (by the id-based settle guard) while the newer entry shows the guard's rejection error instead of the intended answer.
Suggested fix
Guard client-side before firing a new request: in fireSideQuestion (or in btwCommand.execute), check useSideQuestionStore.getState().byTaskId[taskId]?.status === "pending" and short-circuit with a clear toast ("A side question is already answering — wait for it or try again in a moment") instead of letting the request race the server's lock and surface a cryptic backend error. If true cancellation is wanted, dismiss needs a way to actually abort the in-flight request (which requires a cancel path threaded down to the fork's AbortController on the agent side) rather than just hiding the card while the fork keeps the single-flight slot occupied.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/sessions/sideQuestionStore.ts#L61-65
@packages/ui/src/features/sessions/sideQuestionStore.ts#L74-87
<issue_description>
`fireSideQuestion` fires a new `askSideQuestion` request unconditionally, and `dismiss` only removes the entry from `byTaskId` — neither checks nor cancels the in-flight request. The backend agent (`ClaudeAcpAgent.answerSideQuestion` in packages/agent/src/adapters/claude/claude-agent.ts) allows only one side question at a time per session (`sideQuestionInFlight`) and rejects a concurrent call with `RequestError(-32002, "A side question is already in progress")`, and the fork it runs is bounded by a 120s timeout (`SIDE_QUESTION_TIMEOUT_MS`) with no cancellation hook exposed to the UI at all (no `cancel`/abort tRPC method for side questions — only the unrelated `agent.cancel` for the main turn, which explicitly never touches the side-question's own AbortController). So the very plausible flow — ask `/btw`, dismiss the card because you no longer want the answer, then immediately ask a different `/btw` question — sends a second request that the server rejects for up to 120 seconds, surfacing a confusing "A side question is already in progress" error in the new card with no way to recover except waiting. The same race happens even without dismissing, if the user asks a second `/btw` before the first resolves: the first request's real answer/cost is silently discarded (by the id-based `settle` guard) while the newer entry shows the guard's rejection error instead of the intended answer.
</issue_description>
<issue_validation>
- **Checked:** the backend lock in `claude-agent.ts` (`answerSideQuestion`), the timeout constant, the client path (`sideQuestionStore.ts` `fireSideQuestion`/`dismiss`/`settle`), `sessionService.askSideQuestion`, and whether any cancel path exists for side questions.
- **Found (single-flight lock is real):** `claude-agent.ts:1550-1551` rejects a concurrent call with `RequestError(-32002, "A side question is already in progress")`; the lock (`sideQuestionInFlight`) is set at :1553 and only released in the `finally` at :1619 after the fork resolves or times out. `SIDE_QUESTION_TIMEOUT_MS = 120_000` (`side-question.ts:9`), so worst-case hold is 120s.
- **Found (no client guard, no cancel):** `fireSideQuestion` (`sideQuestionStore.ts:74-88`) calls `ask()`+`askSideQuestion` unconditionally; `dismiss` (:61-65) only deletes the `byTaskId` entry. `sessionService.askSideQuestion` (`sessionService.ts:3581-3602`) forwards straight to the tRPC mutation with no in-flight dedupe. The only cancel method anywhere is the main-turn `agent.cancel`; there is no side-question cancel/abort exposed to the UI, and the fork's `abortController` is local to `answerSideQuestion` (:1555).
- **Found (silent discard confirmed):** `settle` (`sideQuestionStore.ts:34-43`) returns state unchanged when `entry.id !== id`. So in the double-ask flow the first request's completed answer is dropped when it resolves (id no longer matches), while the second entry displays the -32002 rejection instead.
- **Impact:** Two plausible flows — ask then dismiss-and-re-ask, or ask twice within the answer window — hit the server lock and surface "A side question is already in progress" on the new card; in the double-ask case a completed, cost-incurred answer is silently discarded. It self-heals once the first fork finishes (seconds typically, up to 120s), so no persistent data loss, but it's a real reliability/correctness rough edge directly introduced here. The author already added the `settle` stale-answer guard, showing this concurrency dimension is in scope; the missing in-flight guard is a cheap, consistent complement (check `byTaskId[taskId]?.status === "pending"` before firing). Concrete trigger + concrete consequence, so it clears the bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Guard client-side before firing a new request: in `fireSideQuestion` (or in `btwCommand.execute`), check `useSideQuestionStore.getState().byTaskId[taskId]?.status === "pending"` and short-circuit with a clear toast ("A side question is already answering — wait for it or try again in a moment") instead of letting the request race the server's lock and surface a cryptic backend error. If true cancellation is wanted, `dismiss` needs a way to actually abort the in-flight request (which requires a cancel path threaded down to the fork's AbortController on the agent side) rather than just hiding the card while the fork keeps the single-flight slot occupied.
</potential_solution>
| const btwCommand: CodeCommand = { | ||
| name: "btw", | ||
| description: | ||
| "Ask a quick side question without interrupting the conversation", | ||
| input: { hint: "your question" }, | ||
| execute(args, ctx) { | ||
| const question = args?.trim(); | ||
| if (!question) { | ||
| toast.error("Add a question after /btw"); | ||
| return; | ||
| } | ||
| if (!ctx.askSideQuestion) { | ||
| toast.error("Side questions aren't supported for this session yet."); | ||
| return; | ||
| } | ||
| ctx.askSideQuestion(question); | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
/btw question text is permanently lost (no restore) when the session doesn't support side questions, unlike every other send-failure path in this file
Why we think it's a valid issue
- Checked: the optimistic-clear flow in
useTiptapEditor.submit(:767-810), the sibling restore paths inuseSessionCallbacks.handleSendPrompt, howaskSideQuestionis derived, and thebtwCommand.executeunsupported branch. - Found (optimistic clear confirmed):
submit()firescallbackRefs.current.onSubmit?.(serialized)(:807) then unconditionally callsdoClear()(:810), which runseditor.commands.clearContent()+draft.clearDraft()(:780-784).onSubmit(handleSendPrompt) is not awaited, so the composer is wiped on every submit regardless of the async handler's outcome. - Found (sibling paths restore, this one doesn't):
handleSendPromptrestores typed text on both failure paths — queued-update throw (useSessionCallbacks.ts:132-134) and sendPrompt throw (:159-161) — each callingsetPendingContent+requestFocuswith the comment "The composer clears optimistically on submit" (:156). But a command that returns handled=true exits at :86 with no restore, andbtwCommand.execute's unsupported branch (commands.ts:120-122) only toasts and returns, so the typed question is gone. - Found (reachable):
askSideQuestionisundefinedwhenever!currentSession || !sessionSupportsSideQuestion(currentSession)(useSessionCallbacks.ts:81-84), i.e. cloud sessions, non-Claude/codex adapters, or a local Claude session beforecurrentSessionis populated./btwis offered in autocomplete for all sessions (no suggestion-list filter), so a user can select it, type a question, submit, and hit this branch. - Impact: confirmed loss of user-typed input with no recovery, inconsistent with the deliberate restore pattern in the same flow — a real bug, and the team clearly cares about not losing typed prompts (they wrote explicit restore logic twice).
- Priority: lowering to
consider— the lost content is a short side-question (not a long composed prompt), the trigger is the niche unsupported-session path, and a clear explanatory toast is shown, so impact is modest and sits below the should_fix threshold while still worth recording.
Issue description
useTiptapEditor.submit() calls callbackRefs.current.onSubmit?.(serialized) (which drives handleSendPrompt → tryExecuteCodeCommand → btwCommand.execute) and then synchronously calls doClear() right after, without awaiting the async handler — the composer is wiped unconditionally on every submit, not just on success. useSessionCallbacks.handleSendPrompt already accounts for this: every other failure path (updateQueuedMessage throwing, sendPrompt throwing) explicitly calls setPendingContent(...)/requestFocus(...) to restore the typed text, with comments noting the composer 'clears optimistically on submit' and would otherwise lose the prompt. btwCommand.execute (lines 109-127) was never wired into that pattern: when ctx.askSideQuestion is undefined (line 120) — e.g. a cloud session, a non-Claude adapter, or simply because a user can type /btw ... manually even before the suggestion-list gating fix — it only calls toast.error("Side questions aren't supported for this session yet.") and returns. The user's fully-typed question is gone from the composer with no recovery path, forcing a full retype, which is a regression relative to the established behavior for the sibling send-failure paths in the same file.
Suggested fix
Give btwCommand.execute's unsupported-session (and, for symmetry, the empty-question) branch the same restore behavior as the other failure paths in handleSendPrompt — e.g. thread setPendingContent/requestFocus (or a callback that wraps them) through CommandContext the same way askSideQuestion was added, and call it before returning so the typed question survives the toast instead of being silently discarded.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/message-editor/commands.ts#L109-127
<issue_description>
`useTiptapEditor.submit()` calls `callbackRefs.current.onSubmit?.(serialized)` (which drives `handleSendPrompt` → `tryExecuteCodeCommand` → `btwCommand.execute`) and then synchronously calls `doClear()` right after, without awaiting the async handler — the composer is wiped unconditionally on every submit, not just on success. `useSessionCallbacks.handleSendPrompt` already accounts for this: every other failure path (`updateQueuedMessage` throwing, `sendPrompt` throwing) explicitly calls `setPendingContent(...)`/`requestFocus(...)` to restore the typed text, with comments noting the composer 'clears optimistically on submit' and would otherwise lose the prompt. `btwCommand.execute` (lines 109-127) was never wired into that pattern: when `ctx.askSideQuestion` is undefined (line 120) — e.g. a cloud session, a non-Claude adapter, or simply because a user can type `/btw ...` manually even before the suggestion-list gating fix — it only calls `toast.error("Side questions aren't supported for this session yet.")` and returns. The user's fully-typed question is gone from the composer with no recovery path, forcing a full retype, which is a regression relative to the established behavior for the sibling send-failure paths in the same file.
</issue_description>
<issue_validation>
- **Checked:** the optimistic-clear flow in `useTiptapEditor.submit` (:767-810), the sibling restore paths in `useSessionCallbacks.handleSendPrompt`, how `askSideQuestion` is derived, and the `btwCommand.execute` unsupported branch.
- **Found (optimistic clear confirmed):** `submit()` fires `callbackRefs.current.onSubmit?.(serialized)` (:807) then unconditionally calls `doClear()` (:810), which runs `editor.commands.clearContent()` + `draft.clearDraft()` (:780-784). `onSubmit` (handleSendPrompt) is not awaited, so the composer is wiped on every submit regardless of the async handler's outcome.
- **Found (sibling paths restore, this one doesn't):** `handleSendPrompt` restores typed text on both failure paths — queued-update throw (`useSessionCallbacks.ts:132-134`) and sendPrompt throw (:159-161) — each calling `setPendingContent`+`requestFocus` with the comment "The composer clears optimistically on submit" (:156). But a command that returns handled=true exits at :86 with no restore, and `btwCommand.execute`'s unsupported branch (`commands.ts:120-122`) only toasts and returns, so the typed question is gone.
- **Found (reachable):** `askSideQuestion` is `undefined` whenever `!currentSession || !sessionSupportsSideQuestion(currentSession)` (`useSessionCallbacks.ts:81-84`), i.e. cloud sessions, non-Claude/codex adapters, or a local Claude session before `currentSession` is populated. `/btw` is offered in autocomplete for all sessions (no suggestion-list filter), so a user can select it, type a question, submit, and hit this branch.
- **Impact:** confirmed loss of user-typed input with no recovery, inconsistent with the deliberate restore pattern in the same flow — a real bug, and the team clearly cares about not losing typed prompts (they wrote explicit restore logic twice).
- **Priority:** lowering to `consider` — the lost content is a short side-question (not a long composed prompt), the trigger is the niche unsupported-session path, and a clear explanatory toast is shown, so impact is modest and sits below the should_fix threshold while still worth recording.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Give `btwCommand.execute`'s unsupported-session (and, for symmetry, the empty-question) branch the same restore behavior as the other failure paths in `handleSendPrompt` — e.g. thread `setPendingContent`/`requestFocus` (or a callback that wraps them) through `CommandContext` the same way `askSideQuestion` was added, and call it before returning so the typed question survives the toast instead of being silently discarded.
</potential_solution>
1 participant
Problem
Users mid-conversation with Code often want a quick answer about something in the current session (what changed, what a function does) without derailing the active turn or polluting the conversation history. There was no way to do this without either interrupting the running agent or starting a whole new session.
Changes
Adds a
/btwcommand that answers a one-shot side question by forking the live session's transcript, similar to how Claude Code's own/btwworks.The fork is a single turn with no tools:
tools: [],allowedTools: [],mcpServers: {}, and an always-denycanUseToolindependently block tool use, and it gets its own session id and abort controller so nothing on the live session is touched. The answer renders as a dismissible card above the composer and never enters the session transcript, so it works both mid-turn and while idle.The Claude adapter advertises the capability during
initialize(), the same way it advertisessteering. Workspace-server, the host-router, andSessionServiceall thread the capability through, and the UI gates the command onsessionSupportsSideQuestion, which is true only for local (non-cloud) Claude sessions.The last commit addresses review findings from an internal pass: fixes a doc comment on
AgentService.sideQuestionthat claimed it doesn't touch the session's idle lifecycle when it actually resets the idle-kill timer, adds an aria-live region so the answer is announced to screen readers, adds a hover tooltip for a truncated question, and adds test coverage for the in-flight guard, the stale-answer guard in the store, the card's render states, and the capability gating.Here's a screenshot of it in action:
How did you test this?
Ran the full test suites for the touched packages (
@posthog/agent,@posthog/core,@posthog/workspace-server,@posthog/ui) andpnpm typecheckon each; everything passes. Ran an eight-dimension code review (security, performance, correctness, maintainability, testing, compatibility, architecture, frontend) against the diff and applied the fixes it surfaced.I haven't manually exercised the
/btwcommand in the running Electron app.Automatic notifications