Repository navigation
fix(session): let every surface find a transcript filed under another group - #455
Conversation
… group The cross-group search in session_path was gated on project_slug, so only a surface that groups by launch directory ran it. The gateway groups by channel and never did -- yet list_sessions scans every group, so the served page offered a conversation started in a terminal and then could not open it: session.resume missed the file, fell through to its fresh-mint branch, and the reader got an empty transcript under a new id. The write verbs resolve through the same function, so the miss was not only a failed read. A rename or a turn from the page filed a second transcript under the channel group and split one conversation across two files. Ungating the search alone is wrong: the glob matches on the chat_id, which names no channel, so a tui:<id> save adopted the cli:<id> file. A candidate is now taken only once its own metadata key says it is this session; a keyless transcript predates that field and is still adopted on the stem. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed github/main...HEAD and found the fallback now works for the gateway while the stored-key check prevents an ordinary same-chat-id collision from crossing channels. The direct-path behavior and new-session grouping remain unchanged, and keyless transcript adoption preserves the intended legacy fallback.
Coverage included the repository rules in AGENTS.md/CLAUDE.md and the canonical Session terms in CONTEXT-MAP.md/CONTEXT.md; the complete diff; every session_path caller and the RPC read/write paths; storage-format and grouping history; backward compatibility; the additive tests (no weakened coverage); and the Runtime/TUI architecture boundary.
Verification: uv run pytest tests/test_session_project_grouping.py tests/test_session_manager.py tests/test_cli_session_commands.py tests/test_rpc_session.py -q passed (288 tests). Ruff check, Ruff format check, and ty check raven/session/manager.py also passed. The repository-wide type check currently reports one diagnostic at untouched raven/agent/tools/web.py:516; that is pre-existing and outside this diff.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision only merges current main; the effective PR diff is unchanged, and intervening main commits did not touch the session manager, its RPC callers, or the focused session tests. I rechecked the prior review areas against the merge: repository rules and canonical terms, the full diff, callers/history, backward compatibility, additive test coverage, and architecture boundaries. No earlier conclusion is invalidated.
Verification on this head: the focused session/project-grouping/CLI/RPC suite passed (288 tests), and Ruff check, Ruff format check, and the session-manager type check all passed.
The diff-coverage gate reported 73.68% against a 90% threshold. All five uncovered lines were _stored_key's error paths, which its docstring already promises: a file whose head is missing, unreadable, or keyless answers None. Adds three tests for those three ways to answer None -- a dangling symlink for the OSError path, a truncated first line for the JSONDecodeError path, and a head that parses but is not a metadata record. Each asserts the transcript is still adopted across groups rather than shadowed by a fresh empty one. Diff coverage is 100.00% (19/19) with this change. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision adds only three additive tests for the documented _stored_key fallback branches; production code and callers are unchanged, and no existing assertion was weakened. I rechecked the full effective diff against the repository rules and canonical terms, caller/history and compatibility assumptions, and the existing Runtime architecture boundary. The new fixtures match the helper's current contract and do not invalidate the prior review.
Verification on this head: the focused session/project-grouping/CLI/RPC suite passed (291 tests), and Ruff check, Ruff format check, and the session-manager type check all passed.
|
**Not a blocker -- I found nothing wrong with the fix, and the mutation pass killed seven of ten. One survivor is worth a line: the
for existing in sorted(self.sessions_dir.glob(f"*/{chat_id}.jsonl")):
stored = _stored_key(existing)
if stored is None or stored == key:
return existingMeasured on the merged tree, one mutation at a time, tree restored (
The control is what makes the survivor mean something: the suite does reach this block. And the survivor is not equivalent. Six group directories holding the same The order only decides anything when more than one candidate passes the filter -- and this PR's own keyless escape hatch ( A test that fixes the answer -- three keyless transcripts under groups that sort differently from creation order, asserting which path comes back -- closes it in one function. The code as written is correct; this is about keeping it that way. Two more survivors I am NOT filing, and why
What held up
One correction to my own brief, which the pass caught: I called |
… group (#455) ## Summary A conversation started in the TUI was listed by the served page and then could not be opened: clicking it produced an empty transcript. `SessionManager.session_path` looks for a transcript in this process's own group, and falls back to searching the other groups when it is not there. That fallback was gated on `project_slug`, so only a surface that groups by launch directory ever ran it. The gateway groups by channel and never did -- yet `list_sessions` scans every group, so the page offered a conversation filed under a launch directory's slug and then looked for it under `sessions/tui/`. `session.resume` missed the file, fell through to its fresh-mint branch, and handed the reader an empty transcript under a new id. The write verbs resolve through the same function, so the miss was not only a failed read. A rename, a pin or a turn from the page filed a second transcript under the channel group, splitting one conversation across two files: ``` sessions/-srv-alpha/abc.jsonl -> 2 messages sessions/tui/abc.jsonl -> 1 messages (written by the gateway) ``` Ungating the search is necessary but not sufficient, and the second half is the part worth reviewing. The glob matches on the chat_id, and a chat_id names no channel: `cli:<id>` and `tui:<id>` are two conversations, so an ungated search let a `tui:` save adopt the `cli:` file and append to it. A candidate is now taken only once its own metadata key says it is this session. A transcript carrying no key predates that field and is still adopted on the stem alone, which is the case the search was originally added for. Matching on the stored key rather than on the directory name is also what makes this correct on Windows. `key_from_path` tells a slug from a channel by a leading `-`, which holds only for POSIX paths; a Windows launch directory slugs to `C--Windows-system32`, with no leading separator to read. Root cause was confirmed against a control rather than by inspection: two `tui:` sessions, same handler, same manager, differing only in which group holds the file. The one under `sessions/tui/` resumed with its 26 messages; the one under a slug minted a fresh empty session. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification ``` uv run pytest tests/test_session_project_grouping.py -q 2 failed, 32 passed uv run pytest tests/test_cli_session_commands.py -q 43 passed uv run ruff check raven/session/manager.py tests/test_session_project_grouping.py All checks passed! uv run ruff format --check <same two files> 2 files already formatted uv run ty check raven/session/manager.py All checks passed! ``` The 2 failures in `test_session_project_grouping.py` are present on unmodified `main` in this Windows checkout and are not touched by this change: `test_project_dir_is_not_rewritten_by_a_later_run_elsewhere` and `test_slug_collides_for_a_separator_and_an_underscore` both build fixtures from POSIX paths such as `/srv/alpha`, which `Path` renders as `\srv\alpha` on Windows. Regression scope was measured rather than asserted. Running `pytest tests/ -k "session or transcript or workdir or resume"` (about 1085 tests) with and without the change gives **372 failures before and 372 after, with identical failure sets** -- no test changes state in either direction. The same comparison over the session-adjacent files alone gives 156 before and 156 after. Five tests were added to `tests/test_session_project_grouping.py`, extending the existing file rather than adding a new one. Two of them fail without the fix and pass with it: - `test_the_gateway_opens_a_session_filed_under_a_project_slug` -- the reported bug; - `test_the_gateway_appends_to_the_transcript_it_opened` -- the split-file half; - `test_two_channels_sharing_a_chat_id_keep_their_own_transcripts` -- the guard against over-correcting, and the case that caught the first attempt at this fix; - `test_a_keyless_transcript_is_still_adopted_across_groups` -- pre-key transcripts stay reachable; - `test_the_gateway_still_files_its_own_sessions_by_channel` -- reaching across groups is a fallback for an existing transcript, not a change of where this process writes. The first attempt, which ungated the search without the key check, broke `test_cross_channel_ambiguous_same_id_two_channels` in `tests/test_cli_session_commands.py`. That failure is what produced the key check. Fix verified against the real workspace that produced the report: the affected session now opens with both of its messages, resolving to its actual file under the launch directory's slug. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No docs change: this corrects the behaviour the surrounding docstrings already described, and coins no domain term. ## Risk User-visible change: a surface can now open a transcript filed under another group. On the gateway this closes a gap rather than widening reach -- `list_sessions` already scans every group, so those sessions were already listed and offered; only opening them was broken. For a slug-grouped surface the search is unchanged apart from the new key check, which makes it strictly stricter: a candidate that would previously have been adopted on its stem alone is now refused when its stored key names a different session. Transcripts written before the metadata `key` field are unaffected -- they answer no key and are still adopted on the stem. `session_path` now reads the first line of each candidate when the direct path is absent. The read is one `readline` on a file the caller is deciding whether to open, and the candidate set for an exact chat_id is normally empty or a single file. Rollback is a revert of the single commit; the change is confined to one function and one new module-private helper, and writes nothing to disk that a revert would strand. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com> Co-authored-by: KT <74288668+0xKT@users.noreply.github.com>
Summary
A conversation started in the TUI was listed by the served page and then could not be opened: clicking it produced an empty transcript.
SessionManager.session_pathlooks for a transcript in this process's own group, and falls back to searching the other groups when it is not there. That fallback was gated onproject_slug, so only a surface that groups by launch directory ever ran it. The gateway groups by channel and never did -- yetlist_sessionsscans every group, so the page offered a conversation filed under a launch directory's slug and then looked for it undersessions/tui/.session.resumemissed the file, fell through to its fresh-mint branch, and handed the reader an empty transcript under a new id.The write verbs resolve through the same function, so the miss was not only a failed read. A rename, a pin or a turn from the page filed a second transcript under the channel group, splitting one conversation across two files:
Ungating the search is necessary but not sufficient, and the second half is the part worth reviewing. The glob matches on the chat_id, and a chat_id names no channel:
cli:<id>andtui:<id>are two conversations, so an ungated search let atui:save adopt thecli:file and append to it. A candidate is now taken only once its own metadata key says it is this session. A transcript carrying no key predates that field and is still adopted on the stem alone, which is the case the search was originally added for.Matching on the stored key rather than on the directory name is also what makes this correct on Windows.
key_from_pathtells a slug from a channel by a leading-, which holds only for POSIX paths; a Windows launch directory slugs toC--Windows-system32, with no leading separator to read.Root cause was confirmed against a control rather than by inspection: two
tui:sessions, same handler, same manager, differing only in which group holds the file. The one undersessions/tui/resumed with its 26 messages; the one under a slug minted a fresh empty session.Type
Verification
The 2 failures in
test_session_project_grouping.pyare present on unmodifiedmainin this Windows checkout and are not touched by this change:test_project_dir_is_not_rewritten_by_a_later_run_elsewhereandtest_slug_collides_for_a_separator_and_an_underscoreboth build fixtures from POSIX paths such as/srv/alpha, whichPathrenders as\srv\alphaon Windows.Regression scope was measured rather than asserted. Running
pytest tests/ -k "session or transcript or workdir or resume"(about 1085 tests) with and without the change gives 372 failures before and 372 after, with identical failure sets -- no test changes state in either direction. The same comparison over the session-adjacent files alone gives 156 before and 156 after.Five tests were added to
tests/test_session_project_grouping.py, extending the existing file rather than adding a new one. Two of them fail without the fix and pass with it:test_the_gateway_opens_a_session_filed_under_a_project_slug-- the reported bug;test_the_gateway_appends_to_the_transcript_it_opened-- the split-file half;test_two_channels_sharing_a_chat_id_keep_their_own_transcripts-- the guard against over-correcting, and the case that caught the first attempt at this fix;test_a_keyless_transcript_is_still_adopted_across_groups-- pre-key transcripts stay reachable;test_the_gateway_still_files_its_own_sessions_by_channel-- reaching across groups is a fallback for an existing transcript, not a change of where this process writes.The first attempt, which ungated the search without the key check, broke
test_cross_channel_ambiguous_same_id_two_channelsintests/test_cli_session_commands.py. That failure is what produced the key check.Fix verified against the real workspace that produced the report: the affected session now opens with both of its messages, resolving to its actual file under the launch directory's slug.
No docs change: this corrects the behaviour the surrounding docstrings already described, and coins no domain term.
Risk
User-visible change: a surface can now open a transcript filed under another group. On the gateway this closes a gap rather than widening reach --
list_sessionsalready scans every group, so those sessions were already listed and offered; only opening them was broken. For a slug-grouped surface the search is unchanged apart from the new key check, which makes it strictly stricter: a candidate that would previously have been adopted on its stem alone is now refused when its stored key names a different session.Transcripts written before the metadata
keyfield are unaffected -- they answer no key and are still adopted on the stem.session_pathnow reads the first line of each candidate when the direct path is absent. The read is onereadlineon a file the caller is deciding whether to open, and the candidate set for an exact chat_id is normally empty or a single file.Rollback is a revert of the single commit; the change is confined to one function and one new module-private helper, and writes nothing to disk that a revert would strand.
Related Issues
N/A