feat(desktop): remove threads through the protocol, not around it - #258
Conversation
Review — approve, with one hardening pushedFinding 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
Pushed: validate the id before the recursive delete
Not reachable today:
Worth noting how it was confirmed: deleting the guard and running the test walked One thing I did not changeIn try {
if (await deleteProtocolThread(id)) return;
} catch {
/* fall through to the local writer */
}
await sessionDelete(id);The capability check already returns 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. |
3cae1f2 to
4aa9a23
Compare
c096d86 to
2192b8b
Compare
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>
2192b8b to
00a9477
Compare
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 — butSidebar.tsxbypassed the shim entirely and calledlistSessions()fromtauri-api.tsdirectly, with its own row shape and its own sort. The list was a second reader the whole time.thread/deleteis newThe protocol served
thread/list,thread/forkandthread/archive— but not delete, which is why delete had nowhere to go. It mirrorsarchive:Served under the existing
threadManagementcapability. The Tauri commands stay as the fallback for a sidecar too old to know the method — the same pattern #231 documented forlist.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
listreads both. Removing one left the row reappearing on the next refresh as an empty session that could not be opened. Tested directly.SessionManager.deletetakes 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:
falserather 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