feat(btw): persist side conversations as linked sub-sessions - #10
feat(btw): persist side conversations as linked sub-sessions#10junhoyeo wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0493b30179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // unrecoverable while leaving the retry's transcript missing the turn | ||
| // the user actually saw. | ||
| if (response) { | ||
| dependencies?.recorder?.recordTurn(question, response); |
There was a problem hiding this comment.
When a large side conversation triggers compactSideContext, its one or more generateSummary completions spend tokens but return only strings, and an overflow response is overwritten by the retry at line 537. Because this records only the final response, the persisted transcript still omits all summarization usage and the failed overflow attempt, substantially undercounting the most expensive /btw runs. Persist an assistant usage record for every model completion involved in the run, not only the final answer.
Useful? React with 👍 / 👎.
| sideQuestionId: id, | ||
| question, | ||
| previousTurns, | ||
| paneId, |
There was a problem hiding this comment.
When this client connects to a schema-17 daemon, it sends paneId unconditionally even though DAEMON_COMMAND_COMPATIBILITY.start_side_question remains the legacy { minProtocol: 7 } entry and the existing side_question_transcript capability predates pane IDs. The old daemon therefore accepts the command but ignores the identity, silently losing the promised reconnect grouping. Introduce a negotiated pane-storage capability (or conditional revision compatibility), check it before sending this field, and cover both compatibility directions as required.
AGENTS.md reference: AGENTS.md:L37-L40
Useful? React with 👍 / 👎.
| // entry that is not on disk. Every later main-session append would then | ||
| // hang off a parentId that no reader can resolve, truncating the session on | ||
| // reopen. Side-question storage must never be able to damage the parent. | ||
| parent.appendCustomEntryWithRollback(SIDE_QUESTION_POINTER_TYPE, pointer); |
There was a problem hiding this comment.
If /btw completes while a brand-new parent session still has no assistant response, this append only updates memory: SessionManager._persist returns without writing non-lifecycle entries when hasAssistant is false (session-manager.ts lines 1459-1464). The side transcript itself is flushed by its assistant message, so an abrupt exit before the main response or another lifecycle write leaves an orphan transcript and loses the only pointer and accounting link. Force this pointer durable after the rollback-safe append.
Useful? React with 👍 / 👎.
397a3a3 to
ffc3bd0
Compare
ffc3bd0 to
32a2ccf
Compare
`/btw` ran a real model turn, spent real tokens, and then threw the whole thing away: the transcript lived only in the client's in-memory pane buffer and the assistant message's usage was dropped on the floor. Nothing reached disk, so a side answer could not be revisited and its spend was invisible to every usage ledger, including tokscale. Record each pane as a normal session file under the parent's artifact directory, the same way RLM children are already stored: sessions/<parent>.jsonl one `custom` pointer entry session-artifacts/<parent>/btw-<id>/<side>.jsonl the transcript `custom` entries are ignored by buildSessionContext, so a side question still never enters the main conversation; only the on-disk record changes. Only the new turns are written. The parent context a side question replays is never copied forward, which would duplicate the entire main conversation and its usage once per side question. The pointer is appended with rollback. A plain append indexes the entry before persisting it, so a failed write would leave the parent's leaf pointing at an entry that is not on disk and truncate the session on reopen. Storage failure disables recording for the pane and never surfaces to the answer being read. Turns are grouped by a client-supplied pane id, carried on start_side_question (schema revision 18). Keying on the socket would split one visible pane across two transcripts after a reconnect; older clients fall back to treating a question with no prior turns as a new pane. A failed turn is recorded too, since providers can bill for a turn they then fail. Transcripts set rlmDepth 0 explicitly: a side question is not a subagent run, and lineage is carried by parentSession.
…on wire start_side_question stays a legacy command, so a schema-17 daemon accepts the new paneId and silently drops it. The client would then believe a pane is grouped while the daemon still guesses from previousTurns, splitting one visible conversation across transcripts on reconnect. Gate the field on a negotiated side_question_pane_id capability. Unlike previousTurns this degrades rather than throws: losing pane grouping does not make the answer wrong, and optional daemon metadata must not stop the agent from working. side_question_transcript predates pane ids and does not imply them, so it cannot serve as the gate. Constraint: start_side_question must remain callable against protocol-7 daemons Rejected: bump the command's minProtocol | breaks first questions on older daemons for optional metadata Rejected: reuse side_question_transcript as the gate | it only covers previousTurns, so a transcript-capable daemon would still drop pane ids Confidence: high Scope-risk: narrow
Two ways a /btw run lost accounting. The pointer append is not durable on a brand-new parent: _persist drops non-lifecycle entries until an assistant message exists. The side transcript is flushed by its own assistant message, so an exit before the parent's first response left an orphan transcript with nothing linking back to it. flushNow bypasses that guard, and it exists for exactly this case. The transcript also recorded only the final answer. Compaction summaries come back as plain strings and a context-overflow answer is overwritten by its retry, so the tokens both spent were invisible - and those are the runs that spend the most. generateSummary gains an opt-in onCompletion hook, reported before its error check because a failed summarization still costs tokens, and the run hands every extra completion to recordTurn. Constraint: generateSummary is shared with main compaction, which must not change behaviour Rejected: return usage from generateSummary | changes every caller for one caller's need Rejected: fold the extra usage into the final assistant message | hides that several calls were made Confidence: high Scope-risk: narrow Not-tested: a real provider overflow round-trip; covered only through the faux provider
32a2ccf to
6567e22
Compare
1 participant
Problem
/btwruns a real model turn against the full session context, spends real tokens, and then throws the whole thing away. The transcript lives only inInteractiveMode.sideQuestionTurns[](wiped byclearSideQuestion), andstartSideQuestion's event carries{id, question, answer, status}— the assistant message'susageis dropped on the floor.Nothing reaches disk, so a side answer can never be revisited, and its spend is invisible to every usage ledger. On a 100k-token session a single
/btwre-sends ~100k tokens (plus summarisation calls when it has to compact) and leaves no record anywhere.Prior art
/btwb"branch to chat" → new session file, Q&A as plainmessageentriesagent-aside_question-<hex>.jsonlsidechain beside the main transcriptClaude Code's model is the one adopted here. Its mistake is not: a measured aside file was 567/569 uuids copied from the parent — 52.2M tokens of copy-forward versus 244k genuinely new.
What this does
Records each pane as a normal session file under the parent's artifact directory, exactly how RLM children are already stored:
customentries are ignored bybuildSessionContext, so/btwstill never enters the conversation — only the on-disk record changes. The existing4509suite still passes unmodified in that respect.tokscale clientsalready lists~/.prime/agent/session-artifactsas a scanned root, and the parser countstype:"message"assistant entries with usage.rlmDepth: 0is explicit. A side question is not a subagent run; lineage is carried byparentSession. Deriving depth would misclassify transcripts for consumers keyed on child depth.paneIdonstart_side_question(schema revision 18). Keying on the socket would split one visible pane across two transcripts after a reconnect. Older clients fall back to "a question with no prior turns opens a new pane".appendCustomEntryWithRollback: a plain append indexes the entry before persisting it, so a failed write would leave the parent's leaf pointing at an entry that is not on disk and truncate the session on reopen. Any failure disables recording for that pane and never surfaces to the answer being read.Panes are released on session replacement and close, and the registry is capped (dismissing a pane is client-side and never observed by the daemon), so recorders cannot accumulate in a long-lived daemon.
Produced artifact
{"type":"session","version":3,"id":"01a009dd-…","cwd":"…","parentSession":"…/sessions/01a009dd-….jsonl","rlmDepth":0} {"type":"model_change","provider":"faux","modelId":"faux-1", …} {"type":"message","message":{"role":"user","content":[{"type":"text","text":"codename?"}], …}} {"type":"message","message":{"role":"assistant","content":[…],"usage":{"input":4939,"output":6,"totalTokens":4945, …}}}Review
An adversarial review pass found 1 blocker and 4 majors; all are fixed in this branch and each has a regression test:
paneId.rlmDepthfalsely labelled transcripts as RLM children → now explicit0.Verification
tsgo --noEmit(repo-wide, via pre-commit): 0 errorsbiome check: clean4509-side-questions(52) and the newbtw-side-session-storage(11):4509-side-questions,btw-side-session-storage,agent-connection-daemon,agent-connection-in-process,daemon-mode,daemon-protocol,daemon-client,session-manager,interactive-mode-status,slash-commandsbiome --error-on-warnings+ repotsgo+ installer render + browser smoke): passedThe full
npm run test:cisuite was not run to completion locally — it exceeded 40 minutes on this machine and was killed. CI should be the judge of the untargeted remainder.Follow-ups (not in this PR)
agent-sessions-style listing ofbtw-*transcripts.