Skip to content

fix(acp): implement session/delete so Zed Trash removes whale sessions - #355

Merged
shayne-snap merged 2 commits into
usewhale:mainfrom
reneleonhardt:fix/acp-session-delete
Aug 6, 2026
Merged

fix(acp): implement session/delete so Zed Trash removes whale sessions#355
shayne-snap merged 2 commits into
usewhale:mainfrom
reneleonhardt:fix/acp-session-delete

Conversation

@reneleonhardt

Copy link
Copy Markdown
Contributor

Summary

Zed gates thread deletion on agentCapabilities.sessionCapabilities.delete + session/delete; whale-acp advertised neither → agent-panel Trash was a cosmetic no-op (zed-industries/zed#51912).

Changes

  • Capability: initialize advertises sessionCapabilities {list:{}, delete:{}}.
  • session/delete: params required; sessionId validated (isSafeSessionID) before any file access. Live session: refuses in-flight prompts (every .jsonl writer is turn-scoped → file resurrection unreachable), else drops from map + closes runtime off-path. Unknown id → idempotent success.
  • File removal: primary <id>.jsonl fatal on failure; sidecars best-effort (meta.json, .approval_events.jsonl, .tool_input_events.jsonl, .approvals.json, stale .jsonl.tmp).
  • Hardening: prompt registration re-resolves the session under lock — closes the lookup→register gap where delete/eviction could leave a prompt on a closed runtime, resurrecting the .jsonl.

Validation

  • go build ./..., go vet, gofmt clean; full suite green except internal/tools (pre-existing TestRunShellBackgroundDoesNotPanic 10m timeout).
  • -race on internal/acp + internal/session + internal/store.
  • Tests: advertise; happy path (artifacts gone, list excludes); unknown id; 9 invalid ids/params; live idle (Close fires); in-flight refusal; post-delete load replays nothing; fatal .jsonl failure; dir-as-file; _meta ignored; -race delete-vs-list concurrency.

Review guidance

  • handleSessionDelete + removeSessionFiles (handler.go): in-flight guard, off-path close, fatal-vs-best-effort split.
  • adapter.go registration re-check: closes the delete/eviction gap.

User-visible impact

  • Trash on archived whale threads now deletes them.
  • On success, Zed re-lists its session list itself (SessionListUpdate::Refresh after delete_session succeeds), so the thread disappears from the archive.
  • Agent pushes no list-change notification — ACP has none; the client owns list refresh.
  • Server-side delete is durable + idempotent, so a non-refreshing client only shows a stale thread until its next session/list.

Breaking changes

None. New method session/delete; capability addition only affects clients that check it.

Reproduction

  1. initializesessionCapabilities.delete advertised.
  2. session/new + session/prompt; archive; session/delete{}; session/list → session gone, artifacts removed on disk.

Developed with carefully directed, manually reviewed AI assistance.

Comment thread internal/acp/handler.go
core.ApprovalEventsSuffix,
core.ToolInputEventsSuffix,
".approvals.json",
".jsonl.tmp",

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.

Blocking: This hard-delete path leaves several session-scoped sidecars on disk: .state.json, .todo.json, .user_input.json, and .goal.json (see the corresponding state helpers under internal/session). Those files can retain the session mode, todos, pending user-input questions, and goal/objective after Zed reports the thread deleted.

Please include them in this cleanup list and extend the deletion test to create and assert removal of each.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful review — confirmed. session/delete (removeSessionFiles) removed the primary .jsonl plus the meta/telemetry/approvals sidecars, but left the session-state files persisted by the helpers under internal/session. I cross-checked every session-scoped path in the repo; the full missing set is exactly the four you listed:

  • .state.json (mode) — internal/session/mode_state.go
  • .todo.json (todos) — internal/session/todo_state.go
  • .user_input.json (pending user-input questions) — internal/session/user_input_state.go
  • .goal.json (goal/objective) — internal/session/goal_state.go

(internal/app/commands_doctor.go independently lists meta/state/todo/user_input; .goal.json is confirmed by goal_state.go.)

The fix adds all four to removeSessionFiles using the same SanitizeSessionID(id)+suffix path construction as the helpers. TestSessionDeleteRemovesPersistedSession now creates each of the four sidecars and asserts they are gone after delete, and assertSessionArtifactsGone covers them as well. The full internal/acp suite passes.

For completeness, the sweep also verified the remaining per-session artifacts are out of scope for this path: promoted_tools.json is written only by the main app into a per-session subdirectory (sessionsDir/<id>/promoted_tools.json), and tool-result payloads live under ~/.whale/tool-results/<id>/ — the ACP handler creates neither; it keeps flat files in its sessions dir.

@reneleonhardt
reneleonhardt force-pushed the fix/acp-session-delete branch 2 times, most recently from 453240f to e9c83b5 Compare August 6, 2026 12:13
reneleonhardt and others added 2 commits August 6, 2026 14:23
Zed's agent panel only deletes archived threads when the agent advertises
agentCapabilities.sessionCapabilities.delete and implements session/delete;
without it the Trash button is a cosmetic no-op (zed-industries/zed#51912).

- initialize advertises delete: {} (SessionDeleteCapabilities).
- Dispatch session/delete -> handleSessionDelete: refuse in-flight prompts
  (every .jsonl writer is turn-scoped, so deleting mid-turn would let the
  prompt resurrect the file); otherwise drop the live session and close its
  runtime off-path; idempotent success for unknown sessions.
- removeSessionFiles: primary <id>.jsonl removal is fatal on failure;
  sidecars (meta.json, approval/tool-input events, approvals.json, stale
  .jsonl.tmp) are best-effort.
- Tests: advertise; happy path; unknown id; invalid ids/params; live idle
  session (Close fires); in-flight refusal; post-delete load replays
  nothing; fatal .jsonl removal failure; -race delete-vs-list concurrency.

Co-authored-by: GPT-5.6 Sol <codex@openai.com>
handlePrompt looked up the session, released the lock, then registered the
prompt under a second lock acquire. session/delete or LRU eviction could
remove the session in that window, leaving the prompt to run on a closed
runtime and — for delete — recreate the .jsonl the delete just removed.

Register under the lock with a fresh map lookup; if the session is gone,
respond session-not-found instead of running on a closed runtime.

Co-authored-by: GPT-5.6 Sol <codex@openai.com>
@reneleonhardt
reneleonhardt force-pushed the fix/acp-session-delete branch from e9c83b5 to 03cab99 Compare August 6, 2026 12:23
@reneleonhardt

Copy link
Copy Markdown
Contributor Author

Commits are only separated for reviewability, you can Squash and merge.
https://docs.github.com/en/pull-requests/reference/pull-request-merges#squash-and-merge-your-commits

@shayne-snap
shayne-snap merged commit 6de6b6b into usewhale:main Aug 6, 2026
2 checks passed
@reneleonhardt
reneleonhardt deleted the fix/acp-session-delete branch August 6, 2026 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants