Skip to content

feat(desktop): remove threads through the protocol, not around it - #258

Merged
oratis merged 2 commits into
mainfrom
feat/desktop-thread-archive
Aug 9, 2026
Merged

feat(desktop): remove threads through the protocol, not around it#258
oratis merged 2 commits into
mainfrom
feat/desktop-thread-archive

Conversation

@oratis

@oratis oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Stacked on #254#252.

Closes the 仍未做 item in docs/THREE_WAY_REVIEW.md §7: "桌面侧栏的 archive / delete 仍走 Tauri … 这是'消除第二个读取者'的后半截。"

It was worse than "a second reader"

The app-server is the single owner and writer of thread storage. A renderer deleting session files behind it can pull the ground out from under an open writer, or leave the index pointing at something that is gone. So this is a correctness problem, not only a tidiness one.

And it was three call sites, not two. window.deepcode.sessions.list() has preferred the protocol since #231 — but Sidebar.tsx bypassed the shim entirely and called listSessions() from tauri-api.ts directly, with its own row shape and its own sort. The list was a second reader the whole time.

thread/delete is new

The protocol served thread/list, thread/fork and thread/archive — but not delete, which is why delete had nowhere to go. It mirrors archive:

  • 404s on a thread that does not exist rather than silently succeeding;
  • a store that cannot delete says so rather than quietly archiving instead. Being helpful about a destructive verb by performing a different one is the worst available answer.

Served under the existing threadManagement capability. The Tauri commands stay as the fallback for a sidecar too old to know the method — the same pattern #231 documented for list.

Delete removes both representations

The protocol snapshot and the canonical session projection share an id and are two views of one thing, and the composite list reads both. Removing one left the row reappearing on the next refresh as an empty session that could not be opened. Tested directly.

SessionManager.delete takes the v1 stream, the legacy stream, the meta sidecar, the writer lock and the per-session directory (snapshots, background-task logs, todos). The listing reads the sidecar, so leaving that behind is not a tidy half-delete — it is a row you cannot get rid of.

Verification

typecheck, lint, format, docs clean; full suite green through the pre-commit hook. 9 new tests:

  • protocol runtime: delete drops from the listing, 404s on a missing thread, a store without the method is rejected;
  • archive keeps the thread readable, delete does not — both drop out of the listing, so the listing alone cannot tell them apart;
  • store: both representations go, and other threads are untouched;
  • desktop agent: archive/delete reach the protocol, and report false rather than throwing when the server cannot manage threads, so the fallback is a fallback and not a swallowed error.

The preview fixture answers thread/delete, so the Playwright journey exercises the new path in CI.

🤖 Generated with Claude Code

@oratis

oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Review — approve, with one hardening pushed

Finding that the list was also bypassing the shim, and saying so rather than quietly fixing three things under a two-thing title, is the right instinct. The thread/delete design choices are all defensible:

  • 404 rather than silently succeeding on a missing thread;
  • a store that cannot delete rejects rather than archiving instead — being helpful about a destructive verb by performing a different one is the worst available answer, and the test pins it;
  • delete? optional on ThreadStore, so no implementation breaks;
  • both representations go, with a test proving the row does not come back. That is the failure that would actually have shipped.

archive keeps the thread readable, delete does not is a good test — the listing alone cannot distinguish them, so reading is the only thing that can.

Pushed: validate the id before the recursive delete

deleteSession ends in rm -rf <root>/<id>, and .. passes the ^[a-zA-Z0-9._-]+$ check that both this path and validThreadId rely on — it is spelled entirely in characters an id may legitimately contain. join(root, '..') is the directory above the sessions root.

Not reachable today: ProtocolRuntime.deleteThread 404s first, and CanonicalThreadStore.delete runs FileThreadStore.delete (which validates) before the session projection. But SessionManager.delete is newly public API on @deepcode/core, this is a recursive delete resolved from an untrusted string, and it is one refactor away from being the only check. AGENTS.md asks for adversarial tests exactly here.

deleteSession now refuses a malformed id before removing anything, and validThreadId stops admitting ./.. — harmless while every path built from an id had a suffix appended, not harmless now that one reaches a directory removal.

Worth noting how it was confirmed: deleting the guard and running the test walked rm -rf up out of the fixture and into $TMPDIR before hitting EPERM. The committed fixture nests its sessions root deep enough that every hostile id still resolves inside the temp directory, so a future contributor who comments out the guard gets a failing test rather than that.

One thing I did not change

In window-shim.ts, delete falls back to the Tauri writer on any thrown error:

try {
  if (await deleteProtocolThread(id)) return;
} catch {
  /* fall through to the local writer */
}
await sessionDelete(id);

The capability check already returns false for a sidecar too old to know the method, and that is the version-skew case the fallback exists for. So the catch only fires when the owner genuinely failed — possibly partway through — and the response is for the renderer to delete files behind it, which is the thing this PR is removing. For archive that is a cheap retry; for delete it is the ground being pulled out from under a writer that was already in trouble.

Left alone because narrowing it is a judgement call about desktop fallback behaviour that I would rather you make than have me make silently. Worth a follow-up.

@oratis
oratis force-pushed the feat/contract-result-filter branch from 3cae1f2 to 4aa9a23 Compare August 9, 2026 15:39
@oratis
oratis changed the base branch from feat/contract-result-filter to main August 9, 2026 15:43
@oratis
oratis force-pushed the feat/desktop-thread-archive branch from c096d86 to 2192b8b Compare August 9, 2026 15:44
oratis and others added 2 commits August 9, 2026 23:48
The sidebar archived and deleted session files through Tauri while the
app-server served the same threads. That is not only a second reader — the
app-server is the single owner and writer of thread storage, so a renderer
deleting files behind it can pull the ground out from under an open writer or
leave the index pointing at something gone.

Three call sites, not the two THREE_WAY_REVIEW recorded. `window.deepcode.
sessions.list()` has preferred the protocol since #231, but `Sidebar.tsx`
bypassed the shim and called `listSessions()` directly — so the list was a
second reader too, with its own row shape and its own sort.

`thread/delete` is new. The protocol could list, fork and archive but not
delete, which is why delete had nowhere to go. It sits under the existing
`threadManagement` capability and mirrors `archive`: 404 on a thread that does
not exist rather than silently succeeding, and a store that cannot delete says
so instead of quietly archiving instead — being helpful about a destructive verb
by doing a different one is the worst available answer.

Deleting removes both representations. The protocol snapshot and the canonical
session projection share an id and are two views of one thing, and the composite
`list` reads both; removing one left the row reappearing on the next refresh as
an empty session that could not be opened. `SessionManager.delete` takes the
stream, the legacy stream, the meta sidecar, the writer lock and the per-session
directory — the listing reads the sidecar, so leaving it behind is not a tidy
half-delete.

The Tauri commands stay as the fallback for a sidecar too old to serve the
methods, matching what #231 established for `list`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`deleteSession` ends in `rm -rf <root>/<id>`, and the id is a string the
caller was handed. `..` passes the `^[a-zA-Z0-9._-]+$` check that both
this path and the thread store rely on — it is spelled entirely in
characters an id may legitimately contain — and `join(root, '..')` is the
directory above the sessions root.

Every in-tree caller validates first: `ProtocolRuntime.deleteThread`
404s on a thread that does not exist, and `CanonicalThreadStore.delete`
runs `FileThreadStore.delete` (which validates) before the session
projection. So this is not reachable today. It is also a recursive
delete resolved from an untrusted string, one refactor away from being
the only check, and AGENTS.md asks for adversarial tests exactly here.

`deleteSession` now rejects a malformed id before removing anything, and
`validThreadId` stops admitting `.` and `..` — harmless while every path
built from an id had a suffix appended, not harmless now that one
reaches a directory removal.

The fixture nests its sessions root deep enough that every hostile id
still resolves inside the temp directory: confirming this bug by
deleting the guard walked `rm -rf` up into $TMPDIR, which is not
something a test should be able to do by accident.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@oratis
oratis force-pushed the feat/desktop-thread-archive branch from 2192b8b to 00a9477 Compare August 9, 2026 15:49
@oratis
oratis merged commit 8eee002 into main Aug 9, 2026
5 checks passed
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.

1 participant