feat(agent): implement /clear and advertise it as a capability - #76457
feat(agent): implement /clear and advertise it as a capability#76457charlesvien wants to merge 8 commits into
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 |
Prompt To Fix All With AI### Issue 1
products/desktop/packages/agent/src/adapters/claude/claude-agent.ts:1753-1759
**Stale session survives successful clear**
When deleting the original session JSONL fails with a non-`ENOENT` error, `finishClear` only logs the error and reports success. A subsequent cold reconnect can select that surviving file and restore the pre-clear conversation, undoing `/clear` and returning the supposedly freed context.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Reset stale plan state on /clear" | Re-trigger Greptile |
|
@haacked this is your PostHog/code#3546 remade in the monorepo (main there is frozen after the desktop import). Your commits are intact, only paths moved. Can you review and approve if it looks good? |
|
Closing for now: this has to work with cloud first. |
Ok, I'll re-open and fix it. |
|
Reviews (2): Last reviewed commit: "Reset stale plan state on /clear" | Re-trigger Greptile |
8851d8e to
c4ecbe2
Compare
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.33 MiB | 🔺 +1.7 KiB (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.25 MiB · 22 files | no change | ███░░░░░░░ 27.7% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.14 MiB · 3,036 files | 🔺 +601 B (+0.0%) | ████████░░ 83.8% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.4 KiB | src/lib/api.ts |
| 94.7 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.19 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.19 MiB · 17 files | no change | ████░░░░░░ 38.3% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 718.1 KiB | dist/toolbar/toolbar-app-6ZY7LRCK.css |
| 551.5 KiB | dist/toolbar/chunk-chunk-UMQTZ4EG.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-ORHT67OV.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-Z7BTLT7U.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-2LEWJD2B.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-KBKYVHTL.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-7YA5UZUN.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +50.8 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1389.60 MiB · 🔺 +50.8 KiB (+0.0%)
ℹ️ MCP UI apps size — 32 app(s), 17070.3 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 599.6 KB | 187.7 KB |
| action | 457.8 KB | 187.7 KB |
| action-list | 564.4 KB | 187.7 KB |
| cohort | 456.8 KB | 187.7 KB |
| cohort-list | 563.4 KB | 187.7 KB |
| email-template | 456.6 KB | 187.7 KB |
| error-details | 472.4 KB | 187.7 KB |
| error-issue | 457.5 KB | 187.7 KB |
| error-issue-list | 564.3 KB | 187.7 KB |
| experiment | 561.5 KB | 187.7 KB |
| experiment-list | 565.2 KB | 187.7 KB |
| experiment-results | 563.2 KB | 187.7 KB |
| feature-flag | 567.2 KB | 187.7 KB |
| feature-flag-list | 570.9 KB | 187.7 KB |
| feature-flag-testing | 461.0 KB | 187.7 KB |
| insight-actors | 562.2 KB | 187.7 KB |
| invite-email-preview | 456.0 KB | 187.7 KB |
| llm-costs | 559.5 KB | 187.7 KB |
| session-recording | 458.6 KB | 187.7 KB |
| session-summary | 463.9 KB | 187.7 KB |
| survey | 458.4 KB | 187.7 KB |
| survey-global-stats | 562.2 KB | 187.7 KB |
| survey-list | 565.1 KB | 187.7 KB |
| survey-stats | 562.2 KB | 187.7 KB |
| trace-span | 457.2 KB | 187.7 KB |
| trace-span-list | 564.3 KB | 187.7 KB |
| workflow | 457.1 KB | 187.7 KB |
| workflow-list | 563.7 KB | 187.7 KB |
| loops-review | 461.2 KB | 187.7 KB |
| query-results | 747.4 KB | 187.7 KB |
| render-ui | 828.0 KB | 187.7 KB |
| visual-review-snapshots | 461.6 KB | 187.7 KB |
✅ Backend coverage — no measured backend lines changed
🧪 Backend test coverage
No measured backend lines changed in this PR (patch coverage n/a).
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
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 |
applySessionMode updated the running query and session.permissionMode but left session.queryOptions.permissionMode at the value the session was created with. Both query rebuilds (/clear, refreshSession) seed the replacement from queryOptions, so a session that started in bypassPermissions and was later narrowed silently returned to bypassPermissions after a clear, with nothing on screen to say the mode had moved. Sync the mode into queryOptions when it changes, matching how effort and the 1M-context beta already do it, so every rebuild inherits the live value rather than each one re-rooting its own fields. Reported by veria-ai on #76457.
finishClear unlinks the jsonl the SDK wrote under the stable ACP id, so a cold reconnect re-fetches the authoritative log instead of finding that file and resuming the pre-clear conversation. A non-ENOENT failure was logged and the clear still reported success, which is the one case where the file survives: the next reconnect quietly restored the context the user asked to drop. Treat it as a failed clear instead. performClear already terminates the unproven query, closes the session, and resolves the spinner with clearing_failed, so the user sees the outcome rather than a clear that unwinds itself later. Reported by greptile on #76457.
81f8532 to
e9ab7d7
Compare
🦔 ReviewHog reviewed this pull requestFound 1 must fix, 2 should fix, 2 consider. Published 5 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 3 issues
Files (14)
products/desktop/packages/agent/src/acp-extensions.tsproducts/desktop/packages/agent/src/adapters/base-acp-agent.tsproducts/desktop/packages/agent/src/adapters/claude/UPSTREAM.mdproducts/desktop/packages/agent/src/adapters/claude/claude-agent.tsproducts/desktop/packages/agent/src/adapters/claude/session/commands.tsproducts/desktop/packages/agent/src/adapters/claude/session/jsonl-hydration.tsproducts/desktop/packages/agent/src/adapters/claude/types.tsproducts/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.tsproducts/desktop/packages/agent/src/adapters/prompt-blocks.tsproducts/desktop/packages/agent/src/sagas/resume-saga.tsproducts/desktop/packages/agent/src/server/agent-server.tsproducts/desktop/packages/core/src/sessions/acpNotifications.tsproducts/desktop/packages/core/src/sessions/sessionService.tsproducts/desktop/packages/shared/src/sessions.ts
What were the main changes
- Command detection now reads the raw ACP prompt and skips host-injected blocks (
_meta.ui.hiddenresume preamble, local-skill context) via a new sharedadapters/prompt-blocks.tsused by both the Claude and Codex adapters, fixing/clearbeing read as literal model text in cloud - New
clearConversation/performClear/finishClear/retireQueryflow inclaude-agent.ts: retires the live SDK query and swaps in a fresh SDK session in place, guarded against concurrent clears/prompts/cancels viasession.clearing - Session log gets a
_posthog/conversation_clearedmarker recording the rehydration boundary;jsonl-hydration.tsandresume-saga.tstruncate rebuilt conversations at that marker, andresume-saga'sfindSessionIdnow honors a marker naming no session instead of scanning back to a stalerun_started Sessiontype extended withsdkSessionId/clearing;base-acp-agent.ts'shasSessionmade overridable so the Claude adapter can match on the post-clear SDK session id for reconnects/clearadvertised as an available slash command (session/commands.ts) even though the SDK excludes it, and the adapter advertises aconversationClearcapability ininitialize()- Cloud
agent-server.tsextracts and relays theconversationClearcapability on_posthog/run_started;core/sessionService.tsandshared/sessions.tstrack it on the session record - Permission mode is now synced into
session.queryOptionsat both mutation sites (applySessionModeandcreateOnModeChange) so a query rebuilt by/clearorrefreshSessioninherits the live mode instead of the creation-time one - UPSTREAM.md updated to document that PostHog Code now implements
/clearitself rather than forwarding the SDK's
Frontend
Issues: 2 issues
Files (8)
products/desktop/packages/ui/src/features/sessions/components/ConversationView.tsxproducts/desktop/packages/ui/src/features/sessions/components/SessionFooter.tsxproducts/desktop/packages/ui/src/features/sessions/components/buildConversationItems.tsproducts/desktop/packages/ui/src/features/sessions/components/chat-thread/ChatThreadFooter.tsxproducts/desktop/packages/ui/src/features/sessions/components/incrementalConversationItems.tsproducts/desktop/packages/ui/src/features/sessions/components/session-update/ConversationClearedView.tsxproducts/desktop/packages/ui/src/features/sessions/components/session-update/SessionUpdateView.tsxproducts/desktop/packages/ui/src/features/sessions/components/session-update/StatusNotificationView.tsx
What were the main changes
buildConversationItems.tshandles the_posthog/conversation_clearednotification (renders a boundary marker, resets builder state) and the newclearing/clearing_failedstatuses, tracking anisClearingflag alongside the existingisCompactingone- New
ConversationClearedViewcomponent renders the clear boundary as a separator marker in the new chat-thread UI and a bordered row in the legacy view StatusNotificationView'sCompactingStatusViewgeneralized with a configurable label so it can render a 'Clearing conversation…' spinner, plus aclearing_failedrow for timed-out clearsisClearingthreaded throughSessionFooter,ChatThreadFooter,ConversationView, andincrementalConversationItemsso the generic 'Generating…' footer stays hidden while a clear's dedicated spinner is showing
applySessionMode updated the running query and session.permissionMode but left session.queryOptions.permissionMode at the value the session was created with. Both query rebuilds (/clear, refreshSession) seed the replacement from queryOptions, so a session that started in bypassPermissions and was later narrowed silently returned to bypassPermissions after a clear, with nothing on screen to say the mode had moved. Sync the mode into queryOptions when it changes, matching how effort and the 1M-context beta already do it, so every rebuild inherits the live value rather than each one re-rooting its own fields. Reported by veria-ai on #76457.
finishClear unlinks the jsonl the SDK wrote under the stable ACP id, so a cold reconnect re-fetches the authoritative log instead of finding that file and resuming the pre-clear conversation. A non-ENOENT failure was logged and the clear still reported success, which is the one case where the file survives: the next reconnect quietly restored the context the user asked to drop. Treat it as a failed clear instead. performClear already terminates the unproven query, closes the session, and resolves the spinner with clearing_failed, so the user sees the outcome rather than a clear that unwinds itself later. Reported by greptile on #76457.
baf703c to
fdd5e7d
Compare
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. A _posthog/conversation_cleared marker records the boundary in the append-only session log, and the rehydration paths (jsonl hydration, ResumeSaga) treat it 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. 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. The "/clear" prompt is broadcast only once the new session is confirmed live, so a timeout leaves no orphaned entry in the log, and any error terminates the unproven replacement query, closes the session, and resolves the spinner with clearing_failed. The command is read off the ACP prompt rather than the converted SDK message, skipping blocks the host injected rather than the user: promptToClaude prepends detected-PR and local-skill context, and cloud prompts lead with hidden blocks (a resume preamble; on desktop, shell-execute recaps). Matching the first text block of either read host context as the user's command and missed the command entirely, which is why /clear never fired on a resumed cloud run. The adapter advertises conversationClear in its initialize capabilities and the cloud agent-server relays it on _posthog/run_started, so a host can tell whether the agent it is talking to honours the boundary. Hosts that record one without an agent gate on it; an agent that predates the marker ignores it on resume and would rebuild the conversation the boundary was meant to retire. Claude-Session: https://claude.ai/code/session_01HJQHhq27qXnKGj98x7UrXZ
applySessionMode updated the running query and session.permissionMode but left session.queryOptions.permissionMode at the value the session was created with. Both query rebuilds (/clear, refreshSession) seed the replacement from queryOptions, so a session that started in bypassPermissions and was later narrowed silently returned to bypassPermissions after a clear, with nothing on screen to say the mode had moved. Sync the mode into queryOptions when it changes, matching how effort and the 1M-context beta already do it, so every rebuild inherits the live value rather than each one re-rooting its own fields. Reported by veria-ai on #76457.
finishClear unlinks the jsonl the SDK wrote under the stable ACP id, so a cold reconnect re-fetches the authoritative log instead of finding that file and resuming the pre-clear conversation. A non-ENOENT failure was logged and the clear still reported success, which is the one case where the file survives: the next reconnect quietly restored the context the user asked to drop. Treat it as a failed clear instead. performClear already terminates the unproven query, closes the session, and resolves the spinner with clearing_failed, so the user sees the outcome rather than a clear that unwinds itself later. Reported by greptile on #76457.
createOnModeChange updated session.permissionMode but not session.queryOptions.permissionMode, so a /clear while the agent sat in plan mode rebuilt the query under the creation-time mode while the UI still reported plan. Writing the mode at the mutation site keeps both rebuild paths (/clear and refreshSession) on the live mode. Claude-Session: https://claude.ai/code/session_019in9sZiQW9VRhMcg2q3369
RUN_STARTED only ever set session.conversationClear to true and never reset it, so once a newer agent reported the capability, a later resume by an older agent kept the stale true value instead of reflecting that the connected agent doesn't support /clear's marker. Sync the field to the current run's reported value in both directions instead. Generated-By: PostHog Code Task-Id: 154fe1bc-5425-4660-9641-1169a3773d02
…leak retireQuery awaited session.query.interrupt() with no timeout, unlike the bounded oldConsumer drain a few lines below — and cancel() now no-ops during a clear, so a wedged interrupt() had no recovery path. Bound it with the same withTimeout used for oldConsumer. finishClear always unlinked the jsonl keyed by the stable ACP session id, which is only the file the SDK wrote before a session's first /clear. From the second clear onward that unlink was a no-op and the jsonl the SDK actually just finished writing (keyed by the retired sdkSessionId) was orphaned on disk. Capture that id before it's overwritten and unlink it instead. Generated-By: PostHog Code Task-Id: 154fe1bc-5425-4660-9641-1169a3773d02
The conversation_cleared handler never called
markRuntimeStatusComplete("clearing"), unlike the analogous
compact_boundary handler a few lines above. If the paired "clearing"
status-complete event were ever dropped or reordered, the "Clearing…"
spinner would stay stuck forever in the replayed transcript. Mirror
the compaction pattern so the boundary marker is authoritative on its
own.
Generated-By: PostHog Code
Task-Id: 154fe1bc-5425-4660-9641-1169a3773d02
@radix-ui/themes is banned for any file, with no exception for mirroring an existing legacy file. Replace Box/Flex with a plain div and Tailwind, and Text with the quill primitive. Generated-By: PostHog Code Task-Id: 154fe1bc-5425-4660-9641-1169a3773d02
3c17dfb to
d7b5622
Compare
|
React Doctor found 3 issues in 2 files · 3 warnings. 3 warnings
Reviewed by React Doctor for commit |
Problem
/cleardidn't work in cloud, which was blocking the feature. Two independent reasons:The adapter read the leading slash command off the converted SDK message. In cloud that's almost never the user's text.
promptToClaudeprepends detected-PR and local-skill context, and the agent-server wraps a pending message in a hidden resume preamble. Since a finished cloud run turns every send into a new run, that resume shape is the common path, so/clearreached the model as literal text instead of clearing anything.Separately,
/clearwasn't advertised at all, so it never appeared in the composer's command menu.Changes
Command detection now reads the ACP prompt and skips blocks the host injected rather than the user (
_meta.ui.hidden, plus the local-skill chunkpromptToClaudeconsumes). That also fixes the desktop case where shell-execute recaps precede the prompt. The hidden-block predicate already existed in the codex adapter, so it moved to a sharedadapters/prompt-blocks.tsthat both use.The adapter also advertises a
conversationClearcapability in its initialize response, and the cloud agent-server relays it on_posthog/run_started.That capability exists because of how this ships. The agent is published as
@posthog/agentand baked into the sandbox image, on its own release, while the backend that records a clear boundary ships with PostHog. An agent predating the marker ignores it on resume and rebuilds the conversation the boundary was meant to retire. Without something to gate on, a user could be shown a cleared conversation that wasn't. The stacked PR consumes this.This is the bottom of a two-PR stack, and it has to ship first. It is also useful alone:
/clearon a live run works end to end once anagent-v*release lands.How did you test this code?
Automated, all run locally:
packages/agent(adapters + sagas), including 16 inclaude-agent.clear.test.tspackages/coresessionstypecheckclean on agent, core, ui, sharedNew tests, and the regression each catches:
claude-agent.clear.test.ts— a cloud-shaped prompt where/clearsits behind a hidden resume preamble still clears. This is the exact bug above; every existing clear test sent a bare/clearblock, so none of them caught it.resume-saga.test.ts— aconversation_clearedmarker naming no session resumes nothing.findSessionIdused to skip such an entry and keep scanning back to an earlierrun_started, which on a warm sandbox would natively resume the conversation the marker retired.I have not exercised
/clearagainst a live cloud run. That needs the agent published and the sandbox image rolled, neither of which can happen before this merges.Automatic notifications
Docs update
UPSTREAM.mdnotes that PostHog Code now implements/clearitself rather than forwarding the SDK's.