Repository navigation
(sessions): follow /clear to the new transcript instead of listing it apart - #477
Conversation
devsuitup
left a comment
There was a problem hiding this comment.
Review at 401e27e. Thanks for the PR. The idea is right: after /clear the new transcript should belong to the running session. The problems are in how the new transcript is attributed and how the session is re-keyed. The reviewer traced the paths below in the source. I re-read the code behind the first two; I have not run them.
Blocking
- An unrelated
/clearcan be attached to our terminal (session-transitions.js, thesignals.clearedbranch).matchedis true whenowner === 'unknown' && isSoleClaudeIn(folder). On Windows and macOS the ownership of a transcript cannot be established, soownerisunknown. If one managed session runs in a folder and the user runsclaudeoutside the app in the same folder and clears it, that other session's new transcript is taken over by our terminal. - Re-keying removes the id that in-flight triggers use (same block:
activeSessions.delete(sessionId)thenset(newId, ...))./clearis one of the two commands the trigger watcher may send, and a chain resolves its target by session id at each step. After the/clearstep the old id no longer exists, so the following steps abort even though the terminal is alive. - A pending owner has no retry (same block). A file in the
pendingstate is only rechecked when something else triggers a scan, and a partially written header can stay excluded from detection for good. - Quit after
/clear, before the first prompt (public/app.js). The saved new id is not in the index yet, so restore cannot find it.
Non-blocking
test/session-transitions.test.js(the awaiting-fork case) passes even though an older snapshot matcher causes a transition, so it does not pin what it says.CHANGELOG.md: the entry needs the PR number as its suffix,(#477).- #482's archive state and the injected pending row may disagree.
The branch is also two commits behind main, with three overlapping files.
Six existing suites pass against the unchanged sources (81 tests, Node 24). Not verified: a real restart with SQLite, Node 20/22 with c8.
|
Tested on Linux (Ubuntu, GNOME on Wayland), PR head 401e27e. Setup. An isolated instance from the PR head, with a temp HOME, its own
Blocking 1 (an unrelated
Blocking 2 (a trigger chain with
Blocking 3 (pending owner with no retry): not observed. In every live Blocking 4 (quit after
|
|
Blocking 1 of my review (an unrelated Driven in memory with the shipped
So on Windows (and macOS, which shares the null reader), the single-managed-session fallback accepts any external Not run: a real CLI or Electron, Node 20/22 with c8. |
|
Thanks to both of you, and thanks @devsuitup for the Windows reproduction of point 1. Rebased on current main (eaf59ff); the conflicts with #374/#487 in Blocking
Non-blocking
Other changes
Each fix was removed in turn, and each removal fails a test: the unknown fallback, the alias record, the PTY and status resolution, the recheck, the partial-file recheck and its age limit, the snapshot exclusion, the persist exclusion and the persist on becoming real. On the full suite, the only failures are the CLI-state canary, |
devsuitup
left a comment
There was a problem hiding this comment.
Review at 4a670ad, rebased on current main. Thanks: the unknown-owner fallback no longer attaches a stranger's transcript, the missing-id lookup and the awaiting-fork assertion are fixed, the pending-owner retry works, the changelog suffix is there and the docs are updated. 176 tests in 12 related suites pass locally on Node 24, and the reviewer reproduced the findings below with the shipped modules. Four points block; three of them were already reproduced on Linux in the maintainer's test of 401e27e.
Blocking
- A header-only transcript is marked known before
/clearappears, so it never transitions (session-transitions.js~495, the finalknownJsonlFilesupdate). A flush that reads only the initial mode/caveat records seescleared === false; the file is not put inemptyFiles, so it joinsknownJsonlFilesand the/clearrecord appended later never reconsiders it. Keep a new file with a header and no command record eligible (as the pending-owner case does) until it shows a first user turn or goes stale. Test: write the header, flush, append the/clearrecord, flush again, and expect the transition. - Quitting after
/clearand before the first prompt loses the tab on restore (public/app.js~185).persistWorkingSetsaves the new id; the new transcript is bookkeeping-only, so it is never indexed and the restore planner reports "not in the index". Reproduced on Linux: "Not restored: is not in the index". Persist enough to restore the cleared session independently of indexability (or keep the old id as the restorable target until a prompt exists). Test: clear, persist, restart the planner with that set, and expect the session restored. - An awaiting fork accepts another session's snapshot prefix (
session-transitions.js~450). A session waiting for its fork matches any new transcript that has only snapshots and no/clearmarker, before any ownership evidence arrives. Require theforkFromor ownership evidence before matching, or wait for the marker. - The old and the new id take different trigger locks (
trigger-context.js~44,trigger-watcher.js~1843).sessionLocksis keyed by the raw session id, but both ids reach the same PTY after a re-key, so triggers aimed at the old and the new id can interleave their writes. Key the lock by the PTY (or resolve the id throughrekeyedbefore locking). The chain that aborts at step 0 on Linux ("session exited during chain step 0 submit verify") needs the in-flight target followed through the transition as well; please cover it with a test that runs[/clear, reply]through the shipped watcher.
Non-blocking
- Pending-row injection in the renderer bypasses the backend archive filtering (
public/app.js~1171): an archived cleared session can reappear. - An original trigger id stops resolving after 33 clears (
session-transitions.js~34, the bounded map). - Explanatory comments at
session-transitions.js~31 and ~449 go beyond the one-line pointer allowed in code. test/clear-pending-persist.test.js(~43) supplies the pending rows by hand, so the new renderer callback branch is not pinned.
Not run: a live /clear chain, a SQLite restart, macOS, Node 20/22 with c8, lint, fresh CI. CI on this head is not approved yet.
|
Thanks. Rebased on current main (75ab0ad). The conflicts were in Blocking
Non-blocking
Docs:
Each fix was removed in turn, and each removal fails a test. That covers the no-turn hold, its own-file exclusion and age limit, the owner check on snapshot-only files, both recheck conditions, the alias collapse, the PTY lock key and the Full suite on Node 22: 4872 pass, 17 fail. The failures are the CLI-state canary, four Not run: a live |
devsuitup
left a comment
There was a problem hiding this comment.
Review at 6923cd7, rebased on current main (75ab0ad). Thanks for the thorough follow-up. All four blocking points of the last review are addressed and each fix is pinned: removing each one locally (twelve mutations) makes at least one new test fail. The nine touched test files pass (270 tests, 0 failures) and CI on this head is green on Node 20/22 for Linux and Windows. Three points still block, one of them new.
Previous findings
| # | Finding | Status | Where |
|---|---|---|---|
| 1 | Header-only transcript marked known before /clear |
Resolved | session-transitions.js:361 (hasTurn), :470-473 |
| 2 | Quit after /clear before the first prompt |
Resolved; side effects under Minor | public/app.js:185-191, :226-246; public/restore-plan.js:40 |
| 3 | Awaiting fork accepts a stranger's snapshot-only file | Resolved | session-transitions.js:453-458 |
| 4 | Old and new ids take different trigger locks / [/clear, reply] chain |
Resolved | trigger-watcher.js:1927-1928, trigger-context.js:42; test/trigger-clear-chain.test.js |
| – | Archive / injected row | Resolved | public/app.js:1195-1196 |
| – | 33-clear bound | Resolved, see Blocking 1 | session-transitions.js:36-38 |
| – | Comments, renderer callback | Resolved | public/app.js:512 |
The merge with #454 keeps both sides: getLiveDescriptor is wired at main.js:3426, the compaction cursor of getTranscriptTurn is intact at trigger-context.js:86-93.
Blocking
1. A trigger aimed at a cleared conversation's id goes to the wrong terminal once that conversation is resumed (session-transitions.js:30-38, trigger-context.js:38). rekeyed keeps old-id → new-id for the app's life and resolve() applies it even when activeSessions holds a live session under old-id itself. The changelog says the cleared conversation stays in the list as a past session; clicking it opens a new terminal keyed by old-id (main.js:2747, :2897). Every trigger lookup then follows the alias: getPtyForSession, isSessionBusy, getComposerState, getCliStatus, getTranscriptTurn, getLiveDescriptor resolve to new-id, and acquireSessionLock locks the wrong PTY. Reproduced with the shipped session-transitions.js and trigger-context.js: after /clear from A, a second PTY B registered under old-id, a trigger to old-id returned PTY A. The same happens after a fork re-key when the fork's source is reopened. Resolve through the alias only when the raw id has no live entry (activeSessions.has(id) ? id : currentSessionId(id)), drop aliases whose source becomes active again, prune those whose target exited, and add a test for this sequence.
2. The changelog entry is in the released v0.0.90 section (CHANGELOG.md:28). It sits between ## v0.0.90 (line 10) and ## v0.0.89 (line 31); please move it under ## Unreleased. The CI check only looks for the file in the diff.
3. No CI run shows the new tests failing before the fix. For bug fixes we ask for a run that is red on the regression tests alone, then the fix that turns it green. Every commit here (19ea917, b6b9294, 6923cd7) carries tests and fix together, and the only runs (401e27e, 4a670ad, 6923cd7) are green. Could you push the new and changed tests first on top of main, let CI run red, then the fix commits? A force-push of the reordered branch is fine.
Major
4. On Windows and macOS, /clear is never followed, but the changelog says it is (cli-session-state.js:453-459, main.js:2863). Managed sessions start claude through a shell, so pty.pid is the shell's pid, and defaultReadParentPid returns null outside Linux (cli-session-state.js:68); clearOwner therefore returns unknown at depth 0 for every /clear there and nothing is re-keyed. Either scope the entry ("On Linux, …") or establish ancestry on Windows and macOS (ps -o ppid=, a process snapshot), with a test for the non-Linux path.
Minor
public/app.js:233,public/restore-plan.js:40: afreshentry wins over the index at restore, so a session saved as fresh whose transcript exists by restart starts empty instead of resuming.applySessionForked(public/app.js:534) also marks any fork without a pending entry ascleared. Resume whensessionMap.has(item.sessionId), and wait forindexingDonebefore treating a fresh entry as unindexable.public/app.js:236-239(inferred, not reproduced): a pending + session launched withforkFromor a worktree default is saved as fresh and comes back as a plain session in the main checkout. KeepforkFromand the worktree choice in the entry, or do not mark such sessions fresh.trigger-context.js:91:readPathsis keyed by the raw trigger id, so after a mid-chain re-key the old file's cache entry is never forgotten (one small leak per/clearsent by a chain). Key it by file path, or forget the previous path first.
Suggestion
The hold at session-transitions.js:470 also applies, for up to an hour, to header-only files of unrelated CLIs in the same folder (up to 512 KB re-read per flush per session), and an awaited fork logs "NO MATCH" at info level on every flush; logging once per file would keep main.log readable.
Not verified: a live /clear in the app, a real restart, macOS; finding 6 is inferred from the code.
Changes requested: 3 blocking, 1 major, 3 minor, 1 suggestion.
|
Heads-up for coordination, not a review finding: #518 (a backgrounded session restored from its pre-fork transcript) is the same class as this PR — the PTY's conversation id changes while it lives — but it is not caught by the |
6923cd7 to
eceb9aa
Compare
|
Thanks for the review. Rebased on current main (a02f96c, v0.0.92). The branch is now two commits: eceb9aa, the new and changed tests alone, then 6c1fa8c, the fix.
Blocking
Major
The macOS CLI writes no anchor, so there a Tests (with an injected table) passed on
Not run: a live Minor
Suggestion
Mutation checks: each of the 18 changes, removed one at a time, turns at least one test red. This includes the size half of the cache key and both halves of the alias pruning. I also removed an untested sweep in Full suite (Node 22, this sandboxed Linux session): the only failures are the CLI-state canary, the Not run: a live |
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at 6c1fa8c — request changes (1 major, 3 minor, 3 suggestions). The /clear logic holds: re-key, aliases, the [/clear, reply] chain, the Windows owner check and the fresh restore; no conflict with #518/#524 and nothing merged is reverted. Base a02f96c, mergeable.
Previous findings
| # | Status |
|---|---|
| B1 alias sends a trigger to the wrong terminal | closed — session-transitions.js:37-55 currentSessionId keeps the raw id when a live session holds it and drops aliases whose target exited; trigger-context.js:38 uses it everywhere; both tests pass |
| B2 changelog under v0.0.90 | closed — Unreleased / Fixed |
| B3 no red CI before the fix | closed, with a caveat (minor 1): run 38040849047 on eceb9aa red on the 4 test jobs (37 failures, all new/changed tests), run 38041240410 on 6c1fa8c green, none of the new tests skipped |
| Mj4 Windows/macOS | closed for Windows (cli-session-state.js:597-601 uses #521's process table, Windows tests green on windows-2022); macOS named in the changelog |
| m5, m6, m7, suggestion | closed (restore-plan.js:40, app.js:227/:583/:187, trigger-context.js:91-92, signals cache, once-per-file NO MATCH log) |
Major — the "New session" row after /clear inherits the cleared conversation's identity (public/app.js:586)
The pending row is { ...entry.session, sessionId: newId, summary: 'New session', firstPrompt: '', name: null, messageCount: 0 }, so it keeps aiTitle, starred, modified, created and bridgeSessionId. Sidebar (sidebar.js:1664), grid and terminal header display name || aiTitle || summary, so the new row shows the old title (jsdom probe of the shipped function: "Refactor the payment flow", not "New session"), two rows carry the same title, and the new one is pinned and tied to the old claude.ai session. Build the row like launchNewSession does, and add a test on the displayed name.
Minor
test/clear-pending-persist.test.js:63:setup()loadsapplySessionForkedin every test; main has no function of that name (its listener is anonymous), so all 10 restore tests went red on "must define a top-level function applySessionForked" — 7 never call it. Same for 2 tool-bar tests and 1 session-activity test. The red therefore shows a missing symbol, not the restore bug. Replayed locally against main with only the anonymous callback renamed, 8 of 10 fail on assertions, so the tests do catch it. Load only what each test needs and capture the callback through awindow.api.onSessionForkedstub.trigger-watcher.js:1926-1929(plausible): the trigger lock is now keyed by the PTY object; if the PTY is replaced under the same id mid-chain, the running chain writes to the new PTY while a new trigger takes a different lock and does not wait (on main both waited on the id). Key it by the resolved id..ai/contexts/trigger-watcher.md:995-996: the raw carriage-return byte between backticks became a line feed, splitting the line and naming the wrong character (od -con both versions).
Suggestions
session-transitions.js:489,:499: two info logs fire every second for up to 60 s per recheck.- Plausible:
/resetand/newmay not be recognised as /clear. - Plausible, unsettled: if /clear also writes a
continued-inrecord into the cleared transcript, #518's restore would redirect a click on the cleared conversation to the new one. - The PR description still describes the 401e27e design.
Local (Node 22): touched suites plus the #518/#524 ones, 330/330. The one trigger-watcher.test.js failure (i488-fix2 … 650 ms) is an existing timing flake — 2 of 8 on both head and main. Not run: a live /clear, a real restart with SQLite, macOS, a /clear in an attached background session.
6c1fa8c to
037de9e
Compare
|
Addressed at a468b5b, tests first: 037de9e is red in Test run 38069061167 (all four test jobs, 37 failures, all of them new tests) and a468b5b is green in Test run 38069643247. Major, the new row inherits the cleared identity. The open entry's session is now replaced with a new session built as Minor 1, missing-symbol red. Minor 2, lock keyed by PTY. The lock is keyed by id again. A trigger now waits for every queued id that resolves ( Minor 3, CR byte. Restored in Suggestions
Local (Node 22): full suite 5120/5137. The 17 failures are the canary and sandbox/test-pr tests that fail in this sandboxed session, the same set as on main. |
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at a468b5b — approve (2 nits). All eight points of the 6c1fa8c review are closed.
| Previous point | Status |
|---|---|
| Major: the row after /clear keeps the old title/star/bridge | closed — public/app.js:586-588 builds a fresh session like launchNewSession (summary 'New session', starred: 0, new dates, no aiTitle/bridgeSessionId), shared by the pending row and sessionMap; test the row after /clear is a new session, not a copy of the cleared conversation checks name‖aiTitle‖summary on the pending row, sessionMap and openSessions |
Red from a missing applySessionForked |
closed — listener anonymous again; tool-bar / session-activity tests slice the shipped listener |
| Trigger lock keyed by the PTY object | closed — trigger-watcher.js:1925-1938 keys by id and waits on every queued id ctx.resolveSessionId maps to the same target; new test a trigger waits for a chain whose session got a new PTY under the same id |
CR byte turned into LF in trigger-watcher.md |
closed (od -c, identical to main) |
| Per-second info logs | closed — logOwnerOnce, with a test |
/reset, /new |
closed — CLEAR_COMMANDS, three tests, changelog updated |
continued-in interplay with #518 |
documented (CLI 2.1.296 writes none on /clear); not re-checked here |
| PR description | updated |
Red-first: run 38069061167 on 037de9e red on the 4 test jobs, 37 failures all in tests #477 adds or changes — 31 assertions, 1 TypeError that is the behaviour under test (re-keyed id not resolved), 5 missing symbols that are the new public APIs (clearOwner, currentSessionId). Run 38069643247 on a468b5b green, builds green. Locally (Node 22) touched + bridge/continuation/restore suites: 426/428, 2 skipped; the i488-fix2 … 650 ms trigger-watcher failure is the known flake also seen on main.
No change to session-cache.js, db.js or session-continuations.js; the new row no longer carries bridgeSessionId, so it cannot join the old conversation's #524 bridge group.
Nits
session-transitions.js:36: the/** The id a session is keyed under now… */JSDoc now sits aboveCLEAR_COMMANDS/logOwnerOnceinstead ofcurrentSessionId.app.js:587(plausible): the new row dropsremoteAliasandtype, read atapp.js:182/187andterminal-manager.js:324-390. Safe today sinceclearOwnernever returnsminefor a remote session; worth keeping in mind if remote /clear following is added.
Not run: a live /clear, a real restart with SQLite, macOS, a remote or bg-attached session.
…stead of listing it apart)
…by id, and owner checks Tests for the last review round of devsuitup#477, which devsuitup#548 did not merge: the row after /clear is a new session rather than a copy of the cleared one; /reset, /new and /clear with a name are followed; a trigger waits for a chain whose session got a new PTY under the same id; owner logs fire on change only; clearOwner reads the process table on Windows and never claims on macOS; held header-only files and fresh entries wait for indexing.
a468b5b to
4568996
Compare
…by id, and owner checks Tests for the last review round of devsuitup#477, which devsuitup#548 did not merge: the row after /clear is a new session rather than a copy of the cleared one; /reset, /new and /clear with a name are followed; a trigger waits for a chain whose session got a new PTY under the same id; owner logs fire on change only; clearOwner reads the process table on Windows and never claims on macOS; held header-only files and fresh entries wait for indexing.
…ne trigger queue per session After /clear the open entry gets a new session built as launchNewSession builds one, instead of inheriting the cleared conversation's title, star, bridge id and dates. /reset, /new and /clear with a name are recognised. The trigger lock is keyed by the resolved session id again, so a chain and a new trigger share one queue across a PTY replacement or a /clear. Owner logs fire only when the owner changes, the fork listener is anonymous again (tests capture it through the onSessionForked stub), and the changelog entry moves to Unreleased.
3288ba3 to
6daa029
Compare
|
Notes from reviewing the earlier pair 4568996/3288ba3, ahead of the rebased fix:
|
|
#548 merged 6923cd7, an older head of this PR, not the approved a468b5b. The branch is now main plus that missing last round only: new-session row after /clear, /reset and /new, trigger lock by resolved id, owner checks, anonymous fork listener, changelog moved to Unreleased. Tests 6daa029 red on assertions (run 38144139365), fix a88f03c green (run 38144648704). The body lists it. |
|
Review of a88f03c: approved.
Merging into v0.0.93 at a88f03c. |
… open sessions when the app closes, last review round) Merged onto main after devsuitup#477: the only conflict was the CHANGELOG, where both PRs moved their stale v0.0.90 entry to Unreleased; both lines are kept under Unreleased and neither under v0.0.90.
Now: the last review round only
#548 merged an older head of this PR (6923cd7), not the approved a468b5b. This branch is now main plus what 6923cd7 lacked:
launchNewSessionbuilds one, instead of inheriting the cleared conversation's title, star, bridge id and dates./reset,/newand/clear <name>are followed like/clear.clearOwnerreads the process table on Windows and never claims on macOS; held header-only files and fresh entries wait for indexing; owner and NO MATCH logs fire on change only.applySessionForkedis anonymous again; renderer tests capture the listener through awindow.api.onSessionForkedstub..ai/contexts/trigger-watcher.mdgets its carriage-return byte back.Tests first: 6daa029 is red on all four test jobs, on assertions only (run 38144139365; 21 tests, two of them only on Windows). The test that captures the fork listener accepts main's named form so the red is an assertion, not a missing symbol; the fix commit drops that branch. The fix a88f03c is green on every job (run 38144648704).
Problem
/clear(and its aliases/reset,/new) makes the CLI open a new jsonl under a new session id. Nothing in it names the session it replaced (noforkedFrom; every record carries the new id), sodetectSessionTransitions()never matched it: the open terminal stayed on the old row, and the new conversation appeared as a separate, unattached sidebar entry once its first prompt made it indexable.Fix
~/.claude/sessions/<pid>.jsonwith the newsessionIdon/clear.cliSessionState.clearOwner(newId, ptyPid)answersmine/other/pending/unknown. On Linux it walks the/procparent chain of the pid that namesnewIdup to the PTY. On Windows it uses (bg-agents): on Windows, Switchboard's own tabs show as External and count as live elsewhere #521's process table (anchor pid, creation times). On macOS the CLI writes no anchor, so the answer isotherand the conversation is listed apart, as before.minere-keys the session like a fork (session-forked, kind'clear').pendingis rechecked once a second for up to 60 s.unknownis never matched.readNewSessionSignals()takes the first user record that is not bookkeeping./clear,/resetand/newall count, with or without a name argument. A header-only file stays eligible until it shows its first turn.launchNewSessionbuilds one. It keeps none of the cleared conversation's title, star, bridge id or dates. That row is pending until its first prompt makes it indexable. The cleared conversation stays in the list as a past, resumable session.sessionTransitions.currentSessionId()resolves an old id to the current one, so a chain such as[/clear, reply]keeps reaching its terminal. An id held by a live session is never redirected. The per-session trigger lock queues every id that resolves to the same session.freshentry. On restore it is resumed if indexing found its transcript; otherwise it starts anew in that project, with the project defaults and no new worktree./clearwrites nocontinued-inrecord. On CLI 2.1.296 the only writer is the background-session handoff, and none of 41 local/cleartranscripts is the target of one. So #518's continuation restore does not redirect a click on the cleared conversation.Docs:
.ai/contexts/cli-session-state.md("Owner of a /clear transcript"),.ai/contexts/trigger-watcher.md("Re-keyed sessions", "Session serialization"),docs/session-restore.md("A session with no transcript yet").Tests
Tests-first:
clearOwner/currentSessionIdAPIs being absent on main.The renderer tests capture the shipped
onSessionForkedlistener through awindow.apistub, with no named function extracted.New tests:
session-transitions: owner verdicts; aliases and the named/clear; rechecks; aliasing; once-per-change logging.cli-session-stateandcli-session-process-table: Linux, Windows and macOSclearOwner.trigger-clear-chain: a[/clear, reply]chain; a new-id trigger waits for the old-id chain; a PTY replaced under the same id keeps the queue.trigger-context: re-keyed id resolution.clear-pending-persist: the fresh restore, and the new row's identity.🤖 Generated with Claude Code