feat(web): add safe persisted session deletion - #396
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
Exact-head review: persisted Session deletion is valuable, but two blockers remain. Focused adapter/host tests passed 42/42. An additional probe using the real PiWebRuntime activation/retention methods, real Pi SessionManager files, and PiWebAdapter reproduced deletion of a still-streaming background Session and loss of its original history. The fake agent lifecycle seam follows existing runtime tests; no provider call or installed UI acceptance is claimed. The destructive endpoint also omits the native reviewed confirmation required by #347. No source changes or merge performed.
| const canonical = resolve(session.path); | ||
| const activePath = this.runtime.sessionManager.getSessionFile(); | ||
| if ( | ||
| session.id === this.runtime.sessionManager.getSessionId() || |
There was a problem hiding this comment.
[P1] Protect every live Session owner before deleting its file
This checks only the foreground runtime.sessionManager. PiWebRuntime deliberately retains a previous runtime while it is streaming after activateCandidate switches to another Session. Reproduced A streaming → switch to B → delete A: deletion succeeds while A remains in retainedRuntimes with isStreaming=true. A subsequent real SessionManager.appendMessage recreates the file with only that new message, without the Session header or original history. Please move deletion admission behind the runtime lifecycle boundary so retained/in-flight owners and transitions cannot race with deletion; cover this supported background-generation path.
| if (!path) | ||
| return this.json(response, 400, { error: "session path is required" }); | ||
| try { | ||
| const deletedPath = await this.adapter.deleteSession(path); |
There was a problem hiding this comment.
[P2] Enforce the required reviewed confirmation before exposing deletion
Issue #347 explicitly requires destructive deletion to show the exact persisted target and use native reviewed confirmation. This authenticated endpoint already calls rm through the adapter with only a path, so a direct API request bypasses that requirement entirely. Deferring confirmation to a future UI does not protect the callable backend mutation. Please enforce confirmation bound to the exact canonical deletion target before committing the operation, with a regression proving a bare authenticated DELETE cannot delete it.
Problem
Related to #347. Web Workbench can archive Sessions but cannot safely remove a persisted non-active Session. Deletion must not be confused with archive metadata removal, and the active Session must never be deleted.
Value
Adds a bounded, auditable persistence-management primitive for Session retention while keeping Pi JSONL files authoritative and preventing accidental active-session loss.
Approach
DELETE /api/sessions?path=...endpoint..jsonlfile inside the configured Web Session directory.409 SESSION_CONFLICTresponse.session_deletedevent for connected clients.Validation
biome format/biome lint --error-on-warnings: passed.tsc --noEmit: passed.bunis not installed in this environment, so the equivalent repository scripts were run with the bundled Node 24 executable and local Biome/Vitest binaries.Impact