✨ feat(sessions): delete a session from the sidebar

[rayzalzero]

What changed

A session can now be deleted from the sidebar, and every row's actions moved into one overflow menu.

• One ⋯ trigger per row, replacing the side-by-side hover icons (rename / archive). Desktop reveals it on hover/focus; the mobile drawer shows it always, since touch has no hover. Right-clicking a row opens the same menu. The menu holds Rename, Archive/Unarchive, and Delete.
• Delete removes the session and everything keyed to it: the transcript JSONL, any .bak-* copy an earlier rewind left beside it, its sibling subagent-artifacts directory, its side questions (rows, live child, and the private transcript copy under the btw workspace), and the chamber's archived_sessions / session_ui_state / session_stream_state / queued_messages / chat_sessions rows. In mock mode the demo sessions row and its files tree.
• A confirmation dialog names what goes, and renders the server's refusal reason inline. A pending new-… chat is not offered the action.

Two behaviours the server enforces, not the caller:

1. A session whose omp child is mid-turn is refused (409) — a run, a live subagent, or a dialog the agent is blocked on. Killing that would destroy work the user can still see.
2. An idle child is destroyed and awaited before the purge. omp flushes session state on shutdown and would otherwise recreate the transcript just removed.

Deliberately left unchanged — all upstream, all kept through the merge: the desktop subagent chevron, the mobile roster chevron and useExpandedSessions(), and the status slot where awaitingInput outranks the run spinner.

Why

The sidebar had no way to remove a session — only Archive, which keeps the transcript and hides the row. Users who wanted a conversation actually gone had to leave the UI and delete the JSONL under ~/.omp/agent/sessions by hand, which then left the chamber's own rows behind as orphans.

The menu is part of the same ask: three side-by-side hover icons cost roughly a third of the title's width in the 268px sidebar (measured: title column 146px → 204px), and every further action would take another slice.

Surface

Area	Touched
src/server (Bun/Elysia, omp bridge, SQLite)	[x]
src/client (Preact UI)	[x]
src/shared (server and client)	[x]
src/cli (ompchamber serve/update/stop/status/logs)	[ ]
docs, CI, packaging	[ ]

Mode you actually ran	Ran
MOCK=true (runs with no omp install)	[x]
a real omp session	[x]
production build (bun run build, then bun run start or ompchamber serve --prod)	[x]

Validation

bun run lint                                              -> exit 0, no output
bunx tsc --noEmit --noUnusedLocals --noUnusedParameters   -> exit 0, no output
find src -name "*.ts" -o -name "*.tsx" | xargs wc -l | grep -v total | awk '$1>350'
                                                          -> prints nothing
bun run build                                             -> [build] 8 files, 11.06 MB -> dist/client
bun test                                                  -> 1102 pass, 0 fail, 4623 expect() calls (120 files)
grep -rnE "from '\.\.?/" src --include='*.ts' --include='*.tsx' --include='*.js' | grep -v node_modules
                                                          -> prints nothing

src/client/components/mobile/mobile-session-sidebar/SessionRow.test.ts (4 tests, upstream) passes against the merged row.

Evidence

Ran bun run dev and NODE_ENV=production bun run scripts/../src/server/index.ts on Bun 1.4.2, Ubuntu 24.04 (WSL2), headless Chromium 1440×900 and 390×844. Agent dir and DB were pointed at throwaway paths so a real ~/.omp was never touched. Evidence is from HEAD 5a2e8c2.

Real omp session (dev mode). Deleting a row through the menu removed it from the sidebar, and GET /api/sessions/list no longer listed it. On disk the transcript, its .bak-1 copy and the sibling artifacts directory were all gone, while a second session in the same project survived. Repeating with archived_sessions, session_ui_state and queued_messages rows seeded for the id left all three empty afterwards.

Production mode. Same flow against NODE_ENV=production: the row disappeared, ?sessionId= was cleared, and the transcript was gone from disk.

Mock mode. MOCK=true: 15 demo rows → 14, and the sessions row was gone from SQLite.

Ordering. The await-before-purge rule was exercised with a stub wrapper whose destroyAndWait() writes a valid transcript back, as omp does on shutdown: the file was still gone after the call. A busy stub was refused with 409 and its transcript left alone. This is a stub, not a real omp child — see the caveat below.

Refusals. new-… → 400 session_pending; an id containing .. → 400; an unknown id → 404; GET /api/sessions/:id (the workspace listing) still answers, i.e. the DELETE binding did not shadow it.

Menu. Rename from the menu opens the inline editor without changing the URL; Archive persists and the label flips to Unarchive; Delete opens the dialog and Cancel is a no-op; clicking a row body still selects the session. The menu flips above its trigger when it would overflow the viewport bottom (300px viewport: trigger 185–207, menu rendered 78–181).

Padding. The ⋯ trigger's ink sits 14.38px from the pill's left edge and 14.33px from its right, measured from the glyph getBBox() and confirmed against the rendered pixels (14.4 / 13.6). Before the last commit those were 14.38 / 12.33.

Risk

Destructive and not reversible from the UI. The transcript is gone; there is no trash bin. Mitigation is the confirmation dialog naming what disappears, and Archive remaining the adjacent, reversible action.

State touched: the session JSONL under ~/.omp/agent/sessions/<project>/, its sibling artifacts directory, <db dir>/btw/<sessionId>/, and the SQLite tables archived_sessions, session_ui_state, session_stream_state, queued_messages, chat_sessions (plus sessions/files in mock mode). A live PTY, omp_chamber_settings, and a running daemon are untouched.

Partial failure. The purge is ordered so the worst crash leaves an invisible orphan rather than a broken row: if the process dies after the file is removed but before the DB rows, the session is already gone from the sidebar and only unreferenced rows remain. The reverse order would leave a session that reappears with none of its state. Two known gaps, both small and left as-is: purgeChamberRows runs its seven DELETEs without a withTransaction wrapper, and its table order matters only because files must precede sessions (FK is on) — the order is correct but nothing enforces it.

Rollback: revert the merge commit; the API is additive (one new DELETE route), so nothing else depends on it.

Not verified

• The live-child path was exercised with a stub, not a real omp process. This sandbox has no provider credential, so omp cannot spawn ("No models available"). The ordering contract itself is documented on AgentSessionWrapper.destroyAndWait().
• No screenshots: this environment has no vision-capable model, so the evidence above is DOM measurements and API/disk assertions rather than images. A reviewer should eyeball the mobile drawer and the menu's position at the viewport edges.

---

Review response (review 1 — BLOCKED)

All four findings are addressed in e4fd65e. The branch was rebuilt on main
as a single commit with an identical tree, so the emoji-prefixed subjects are
gone without changing any file content.

1. blocker — . deleted the whole btw root: FIXED, and reproduced first.

path.join(btwRoot, '.') is btwRoot, and purgeBtwForSession removed that
joined path unconditionally, so a . id wiped every session's side questions
and the route still answered 404. Reproduced live before the fix, against a
throwaway btw root holding two unrelated sessions' topic transcripts:

DELETE /api/sessions/.      -> 404   (curl needs --path-as-is; it strips the dot)
btw files before: 2         after: 0

The guard is now one shared predicate (src/server/lib/fs/path-segment.ts)
applied at all three entry points — the route, purgeSessionData, and
purgeBtwForSession — because the latter two are reachable on their own and
their failure mode is other sessions' data. It asserts normalize(id) === id,
the same operation the callers' join performs, so it proves a property of the
path that gets built rather than enumerating suspicious spellings.

After the fix, against the same fixture:

DELETE /api/sessions/.      -> 400
DELETE /api/sessions/%2E    -> 400
DELETE /api/sessions/..     -> 400
DELETE /api/sessions/a%2Fb  -> 400
btw files before: 2         after: 2

2. evidence-gap — screenshots: NOT CLOSED, and I cannot close it here.

This environment has no vision-capable model, so I can capture images but not
read them, and I will not post a screenshot I have not looked at. What I have
instead is DOM measurement and API/disk assertion, including the numbers the
finding asks about: the trigger's ink sits 14.4px / 13.6px from the pill's edges
(measured from getBBox() and confirmed against rendered pixels), the menu
flips above its trigger when it would overflow (300px viewport: trigger
185-207, menu rendered 78-181), and the mobile trigger measures right-aligned
to its row at 390px. A reviewer still needs to eyeball readability and the
error strip — that part of the finding stands.

3. non-blocker — tests: ADDED (10).

• src/server/lib/fs/path-segment.test.ts (5) — the guard's accept/reject
  table, plus the property test that whenever the guard says yes,
  path.join(root, id) stays directly inside root and dirname is root.
  Two of them call purgeBtwForSession('.') against a throwaway btw root and
  assert both unrelated transcripts survive.
• src/server/routes/sessions/delete.test.ts (5) — the refusal branches:
  400 session_pending, 400 invalid id (. and four traversal shapes), 404 for
  a well-formed unknown id (kept distinct from 400), and 405 on a wrong verb.

The regression is real, not nominal: disabling the guard turns 2 of the 5
path-segment tests red. The route test points PI_CODING_AGENT_DIR at an empty
temp tree so the 404 branch does not walk a real ~/.omp (35ms, down from
5.46s).

4. non-blocker — emoji subjects: CONFIRMED, and fixed.

Verified with the repo's own parser rather than by reading the grammar:

new CommitParser({}).parse('✨ feat(sessions): delete a session from the sidebar')
  -> { type: undefined }
new CommitParser({}).parse('feat(sessions): delete a session from the sidebar')
  -> { type: 'feat', scope: 'sessions' }

You were right that release/release-rules.js pins only noteKeywords /
notesPattern and leaves headerPattern at the default. Rebuilt as a single
feat(sessions): … commit; the tree is byte-identical to the reviewed one
(git rev-parse HEAD^{tree} compared), so every measurement below still holds.

---

Screenshots (finding 2)

Captured at HEAD e4fd65e on a real omp session (not MOCK), Chromium, desktop
1440x900 and mobile 390x844. Kept on a separate branch (pr-assets/4-screenshots)
so they do not enter this PR's diff.

Caveat, stated plainly: this environment has no vision model, so I captured
these but did not look at them. They are verified mechanically only — each
image decodes, its ink-to-background ratio is plausible (1.9%-13.7%), and each
before/after pair differs where it should (dialog → +error changes 2.4% inside
the dialog's own bounding box; mobile row → menu changes 0.8% near the row).
A human still has to confirm readability, which is exactly what the finding asks
for.

Desktop — the row and its menu

Row at rest (⋯ revealed)	Menu open

Desktop — the delete dialog, and a server refusal

Confirmation	Refused ("Session not found")

The refusal shot is a genuine round trip: the transcript was removed from disk
behind the already-rendered row, so the route answered 404 and the dialog
rendered it inline rather than closing.

Mobile (390px) — trigger always visible, no hover

Drawer at rest	Menu open	Confirmation

[ompchamber-bot]

Code Review Summary

For the maintainer: don't merge yet — DELETE /api/sessions/. passes the id validation and wipes the entire btw workspace root (every session's side-question data), and the UI redesign ships without the required screenshots.

The PR adds DELETE /api/sessions/:sessionId (src/server/routes/sessions/delete.ts) that refuses a busy omp child with 409, awaits an idle child's exit, purges side questions, the transcript with its .bak-* copies, the subagent artifacts directory, and seven chamber tables, then collapses the sidebar's per-row hover icons into one overflow menu (session-actions-menu/) shared by desktop and mobile, with a confirmation modal that renders the server's refusal inline. The purge ordering, cache invalidation (invalidateOmpSidebarData, clearSessionFileCaches) and client-side forgetSession are correct as written; the route binding on the shared :sessionId path does not shadow the folder GET. First review of this HEAD — no prior findings exist.

Verdict: BLOCKED
Handoff: complete

Reviewed HEAD: 5a2e8c2abd106d04c60d115fb932440a0a295db6
Previous reviewed HEAD: none

Findings

1. blocker: a . session id deletes the whole btw root
   File: src/server/routes/sessions/delete.ts:38 (validation) and src/server/lib/btw/purge.server.ts:41
   Problem: SAFE_SESSION_ID (/^[A-Za-z0-9._-]+$/) accepts . and the explicit check only rejects .., so DELETE /api/sessions/. reaches purgeBtwForSession('.'). listBtwTopics('.') returns no rows, and the unconditional removeBtwWorkspace(path.join(await getBtwRoot(), '.')) normalizes to the btw root itself — fs.rm(root, { recursive: true }) deletes every session's side-question workspaces and topic transcripts on disk, then the route answers 404. Any user (or a crafted request) can destroy data far beyond the targeted session.
   Suggested fix: refuse ids that do not name a segment under the root — e.g. add sessionId.startsWith('.') to the 400 branch (omp UUIDs and numeric mock ids never start with a dot), or assert path.normalize(sessionId) === sessionId && sessionId !== '.' before any purge.

2. evidence-gap: no screenshots for a redesigned, destructive UI surface
   File: src/client/components/common/session-actions-menu/index.tsx, src/client/components/common/session-delete-modal/index.tsx
   Problem: the PR redesigns every session row on both layouts and adds the irreversible-delete dialog; CONTRIBUTING requires before/after screenshots plus the mobile view when shared layout is affected. The DOM measurements show nothing about the menu's readability, the error strip rendering, or the mobile drawer's always-visible trigger at real widths.
   Suggested fix: attach desktop + mobile (390px) before/after screenshots of a row with the menu open and of the delete dialog, including the refusal path.

3. non-blocker: no targeted tests for the new destructive server path
   File: src/server/routes/sessions/delete.ts, src/server/lib/omp/session/delete.server.ts
   Problem: the only tests reported are the upstream SessionRow ones. The validation branches (400 session_pending, 400 invalid id, 404, 409 busy), the . traversal above (which a test would have caught), and the purge order are exactly the risky logic this repository expects covered.
   Suggested fix: add a route-level test hitting the refusal branches and a purge test against a throwaway agent dir/DB (the author's own stub harness would do).

4. non-blocker: commit subjects may not parse as Conventional Commits
   Problem: the subjects are emoji-prefixed (✨ feat(sessions): …). The release parser gets no headerPattern override (release/release-rules.js only pins noteKeywords/notesPattern), and a leading emoji before the type is outside the default grammar — if so, this feat cuts no release entry and no changelog bullet. Uncertain on my side; the author should confirm with a local release rehearsal and prefers plain feat(sessions): … either way.

Evidence and Residual Risk

• Review evidence: the validation gates are green and the live-run statement is detailed (real session, production, mock, refusals, ordering via a stub) and dated to this HEAD. Missing: screenshots (finding 2) and the live-child ordering run, which the author honestly flags as stub-only because no provider credential was available — the destroyAndWait-before-purge contract itself is documented on AgentSessionWrapper and matches the route's usage, so I rate that residual risk, not a gap.
• Residual risk: I could not execute anything (review-only policy), so the . finding is derived from path.join semantics (join(root, '.') === root) and the unconditional final removeBtwWorkspace, not from a live request — the code path is short and unambiguous. Multi-chamber-process deletes (a live child in another chamber instance is invisible to getRpcSession) remain an accepted architecture-level edge the PR does not change.

[rayzalzero]

Addressed in e4fd65e — details in the PR description under Review response.

• . blocker: reproduced first (--path-as-is -X DELETE /api/sessions/. took the btw root from 2 files to 0), then fixed with one shared fs/path-segment guard applied at all three entry points. %2E, .. and a%2Fb now all answer 400 and the two unrelated transcripts survive.
• tests: 10 added — 5 on the guard (incl. the join-property test and two purgeBtwForSession('.') regressions) and 5 on the route's refusal branches. Disabling the guard turns 2 red.
• emoji subjects: confirmed with the repo's parser (type: undefined). Branch rebuilt as a single feat(sessions): … commit; tree byte-identical to the reviewed one.
• screenshots: still open, and I cannot close it — this environment has no vision model, so I can capture images but not read them. DOM measurements are in the PR body; a reviewer still needs to eyeball readability and the error strip.

[ompchamber-bot]

Code Review Summary

For the maintainer: the . root-wipe blocker is fixed with a shared guard and real regression tests, but the PR still ships without the screenshots its redesigned destructive UI requires — merge after eyeballing the menu and delete dialog (or attaching them).

Delta since 5a2e8c2: the branch was rebuilt as one conventional feat(sessions): ... commit and adds the isSinglePathSegment guard (src/server/lib/fs/path-segment.ts) applied at all three entry points — the route 400, purgeSessionData, and purgeBtwForSession — plus 10 targeted tests (5 on the guard incl. a path.join-property test and two purgeBtwForSession(".") regressions, 5 on the route refusal branches). I re-read the guard, the route, both purge paths and the DELETE-only binding at HEAD: ./../separators/NUL are refused before any join, and the folder GET loader on the shared path is not shadowed. The validation claim (join(root, ".") === root) is correct and now fenced at every reachable entry point.

Verdict: NEEDS_EVIDENCE
Handoff: complete

Reviewed HEAD: e4fd65e27d3bab4a7b8ed4ede1154d7a1dadde41
Previous reviewed HEAD: 5a2e8c2abd106d04c60d115fb932440a0a295db6

Findings

1. evidence-gap: desktop + mobile before/after screenshots still missing — carried over from review 1, unchanged: the PR redesigns every session row on both layouts and adds an irreversible-delete dialog, and CONTRIBUTING requires screenshots for that surface. The DOM measurements and API/disk assertions are detailed and honestly reported, but a reviewer still needs to see the menu readability and the inline error strip. The author states this environment cannot close it; a maintainer eyeball or an attached image set closes it.
   File: src/client/components/common/session-actions-menu/index.tsx, src/client/components/common/session-delete-modal/index.tsx

Closed since review 1: the . blocker is fixed (guard at src/server/lib/fs/path-segment.ts:44, applied in routes/sessions/delete.ts:41, omp/session/delete.server.ts:74, btw/purge.server.ts:39; verified in the code and pinned by path-segment.test.ts); the missing targeted tests are added and the disabled-guard regression claim matches what they pin; the commit subjects now parse (feat(sessions): ...).

No nits worth recording.

Evidence and Residual Risk

• Review evidence: validation gates green and reported to this HEAD; live-run statement covers real omp, production and mock modes plus the refusal matrix. Still missing: the screenshots (finding 1) and a live-child ordering run — the stub-only caveat from review 1 stands, though the destroy-before-purge contract matches the documented AgentSessionWrapper.destroyAndWait() behavior.
• Residual risk: I could not execute anything (review-only policy); the guard correctness is derived from path.join/normalize semantics and its tests, not a live request. The multi-chamber-process delete edge (a live child invisible to getRpcSession) remains an accepted architecture-level limitation the PR does not change.

[rayzalzero]

Screenshots attached to the PR description (finding 2).

Still worth a human eye: I captured them in an environment with no vision model, so I did not look at them myself. Verified only mechanically — they decode, ink ratios are plausible (1.9%-13.7%), and each pair differs in the right region. Readability and the error strip are what the finding asks for, and only a person can settle those.

They live on pr-assets/4-screenshots, not on this branch, so they stay out of the diff.

[github-actions]

🎉 This PR is included in version 3.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀