feat(agent): implement /clear command#3546
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 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
f58e90b to
30f05dc
Compare
e0d3e15 to
1c5c6ae
Compare
haacked
added
reviewhog
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
stamphog
Bot
removed
the
Stamphog
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 3 should fix, 0 consider. Published 3 findings (view the review). |
|
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: 2 issues
Files (9)
packages/agent/src/acp-extensions.tspackages/core/src/sessions/acpNotifications.tspackages/agent/src/adapters/base-acp-agent.tspackages/agent/src/adapters/claude/types.tspackages/agent/src/adapters/claude/claude-agent.tspackages/agent/src/adapters/claude/session/commands.tspackages/agent/src/adapters/claude/session/jsonl-hydration.tspackages/agent/src/sagas/resume-saga.tspackages/agent/src/adapters/claude/UPSTREAM.md
What were the main changes
- New
_posthog/conversation_clearednotification constant marks the /clear boundary in the append-only session log (agent + core copies) base-acp-agent.hasSessionextracted as an overridable hook so cancel/reconnect can match either the ACP session id or a post-clear SDK session id- Claude adapter intercepts
/clearinprompt(): guards against busy/queued turns, retires the current SDK query via newretireQuery(extracted fromrefreshSession), and swaps in a fresh SDK session under the same ACP session id, tracked via newSession.sdkSessionId clearConversationbroadcasts 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/clearre-added to advertised slash commands (adapter-implemented, not forwarded to SDK) viagetAvailableSlashCommandsrebuildConversation(jsonl hydration) andResumeSaga.findSessionId/rebuildConversationtreat 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.tspackages/ui/src/features/sessions/components/session-update/ConversationClearedView.tsxpackages/ui/src/features/sessions/components/session-update/SessionUpdateView.tsxpackages/ui/src/features/sessions/components/session-update/StatusNotificationView.tsx
What were the main changes
buildConversationItemsrenders a 'conversation_cleared' item on the CONVERSATION_CLEARED notification and handles the new 'clearing'/'clearing_failed' status rows (mirroring compaction)- New
ConversationClearedViewrenders a 'Conversation cleared' divider (chat-thread separator or legacy bordered row) SessionUpdateViewwires the new 'conversation_cleared' item type to the new view componentStatusNotificationView/CompactingStatusViewgeneralized 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/markRuntimeStatusCompletein buildConversationItems.ts (748-793, 873-894), theisCompactingthread 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.isCompactingis set only in thecompactingbranch (buildConversationItems.ts:753) and reset only for"compacting"(line 875); theclearing/clearing_failedbranches (765-780) set no analogous flag.BuildResult.isCompactingis the only busy flag threaded toSessionFooter, whose sole gate isif (isPromptPending && !isCompacting)(SessionFooter.tsx:52) — noisClearingprop exists (props at 14-28). - Found:
/clearruns through the ordinary prompt RPC: prompt() returnsclearConversation(params)(claude-agent.ts:475-478), andsendLocalPromptsetsisPromptPending: true(sessionService.ts:2673) flipping it false only afteragent.prompt.mutateresolves (2708-2711) — i.e. after the whole clear. MeanwhileclearConversationemitsclearing(isComplete omitted → false) at line 1544-1547 before the swap, and StatusNotificationView.tsx:170-179 renders that as aCompactingStatusView label="Clearing conversation…"spinner row. - Impact: For the entire duration of every
/clear, both the generic footer "Generating…" indicator + spinningSlotMachineLeverand the dedicated "Clearing conversation…" row render simultaneously — 100% reproducible, not an edge case. On a slow clear this persists up toSESSION_VALIDATION_TIMEOUT_MS = 30_000ms (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!isCompactinggate; clearing does not). The suggested fix (addisClearing, 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 useConversationItems → ConversationView.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.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
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.
…pinner Fixes the three ReviewHog findings on the /clear implementation: - Session.clearing (a promise, claimed synchronously) serializes the swap: a second /clear is refused, cancel/interrupt is ignored mid-clear, refreshSession refuses, and a racing prompt waits for the clear to settle instead of pushing into the retired input stream. - clearConversation now has a catch-all failure path mirroring the old timeout-only branch: any error terminates the unproven replacement query, closes the session, and resolves the "Clearing…" spinner with clearing_failed instead of leaving it spinning over a half-swapped session. - BuildResult.isClearing threads to SessionFooter's gate so the generic "Generating…" indicator is suppressed while the dedicated "Clearing…" row shows, matching the compaction behavior. Generated-By: PostHog Code Task-Id: 7f180c25-f99b-4c96-ac84-f39c0e887a37
Cleanup pass over the previous commit, no behavior change: - Fold the two near-identical /clear refusal blocks into one send, using the existing text() content helper. - Replace the hand-rolled settleClearing deferred with a promise derived from performClear (waiters only need settlement). - Pass finishClear plain parameters instead of a construct-then- destructure swap object. - Extract a deferInit() test helper for the repeated deferred-init scaffolding; the init-crash test now rejects after the swap starts, dropping the pre-rejected-promise silencer. Generated-By: PostHog Code Task-Id: 7f180c25-f99b-4c96-ac84-f39c0e887a37
ExitPlanMode falls back to lastPlanContent, fileContentCache, and notificationHistory when its tool input omits an explicit plan. Reset them alongside the existing task/tool-cache clears so a plan written before /clear (possibly carrying repo-injected content) can't resurface in the fresh session after approval. Also mark the clearing_failed row's icon aria-hidden, matching the decorative-icon convention used elsewhere in this file. Claude-Session: https://claude.ai/code/session_01UREmJ1dSj1un57eSj7RhdB
227fa95 to
1b71c23
Compare
1 participant
Problem
Typing
/clearin PostHog Code reports "Unsupported slash command". Upstream hid the SDK's own/clearbecause 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.
Refs #670
Changes
packages/agent):prompt()intercepts/clear(never forwarded to the SDK). It retires the current SDK query (reusing therefreshSessionteardown, now extracted asretireQuery) and swaps in a brand-new SDK session, fresh id, no resume, under the same ACP session. The ACP session id stays stable; the newSession.sdkSessionIdtracks the underlying SDK session, andhasSessionaccepts either id so cancel/reconnect work from both sides./clearis advertised in the command list again._posthog/conversation_clearedmarker plus an updatedsdk_sessionmapping in the append-only log.rebuildConversation(jsonl hydration +ResumeSaga) treats the marker as a boundary, andResumeSaga.findSessionIdpicks up the post-clear session id, so cloud sandbox restarts and desktop cold reconnects resume the fresh SDK session with only post-clear turns.usage_update./clearprompt 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 aclearing_failedrow 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./clearalso now refuses while a turn is queued, not just while one is active.Codex sessions are unchanged,
/clearis Claude-adapter only for now.How did you test this?
/clearbehavior (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 andResumeSaga, and the UI divider and status-row rendering (includingclearing/clearing_failed).@posthog/agentsuite,@posthog/uisessions suite, andpnpm typecheckfor agent/core/ui plus Biome on the touched packages.Automatic notifications
Why:
/clearis 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