feat(tui): add isolated /btw conversations#748
Conversation
Add /btw with inline-question support, isolated non-resumable session forks, background main-run routing, and safe return behavior. Refs #637
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughChangesBTW isolated side conversations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant SessionStore
participant ParentRun
User->>TUI: enter /btw [question]
TUI->>SessionStore: fork active session as side
SessionStore-->>TUI: return side session metadata
TUI->>TUI: start isolated side prompt
ParentRun-->>TUI: emit parent messages
TUI->>TUI: route parent messages to hidden parent
User->>TUI: leave BTW
TUI-->>User: restore parent without merging side events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/btw_test.go (1)
170-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression test:
/btwissued again while active should return to the main session.Coverage here validates leaving via Ctrl+C (
leaveBTW) and blocks/newfrom replacing the side session, but nothing exercisesside.dispatchCommand(parseCommand("/btw"))whilem.btw.active— the second documented way to return (per README/help text and the blocked-command notice itself). Given the dispatch guard atinternal/tui/model.goLines 4129-4135 runs for every command kind includingcommandBTW, a test asserting/btwwhile active returns to the parent session (mirroringTestBTWCreatesIsolatedForkAndReturnsWithoutMerge's assertions) would directly close this gap.As per path instructions, "
**/*_test.go: Keep tests beside their source files, add regression tests for behavior changes, and run affected concurrent code under the race detector."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/btw_test.go` around lines 170 - 181, Add a regression test beside TestBTWBlocksCommandsThatWouldReplaceItsSession for dispatching parseCommand("/btw") while m.btw.active. Assert the command returns to the parent/main session, mirroring the session and BTW-state assertions in TestBTWCreatesIsolatedForkAndReturnsWithoutMerge, and run the affected concurrent test under the race detector.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 1033-1040: Update the Ctrl+C handling in the m.btw.active branch
to gate composer and suggestion clearing on !m.pending, matching the non-BTW
branch’s in-flight-run behavior; when m.pending is true, proceed directly to
m.leaveBTW() regardless of composer text.
---
Nitpick comments:
In `@internal/tui/btw_test.go`:
- Around line 170-181: Add a regression test beside
TestBTWBlocksCommandsThatWouldReplaceItsSession for dispatching
parseCommand("/btw") while m.btw.active. Assert the command returns to the
parent/main session, mirroring the session and BTW-state assertions in
TestBTWCreatesIsolatedForkAndReturnsWithoutMerge, and run the affected
concurrent test under the race detector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30575e1d-744a-465d-9c53-7855e43ce256
📒 Files selected for processing (8)
README.mdinternal/sessions/store.gointernal/sessions/store_test.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/session.go
Preserve drafts when Ctrl+C is pressed during an active BTW run and cover returning to the parent through the documented /btw toggle.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/model.go (1)
1033-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated composer-clear conditions.
The complex condition for clearing the composer is now identical in both the BTW and non-BTW branches. Consolidating them reduces duplication and prevents the conditions from diverging in future updates.
♻️ Proposed refactor
- if m.btw.active { - if !m.pending && m.composerValue() != "" && m.noBlockingModal() && !m.transcriptDetailed && !m.subchat.active { - m.clearComposer() - m.clearSuggestions() - return m, nil - } - return m.leaveBTW() - } if !m.pending && m.composerValue() != "" && m.noBlockingModal() && !m.transcriptDetailed && !m.subchat.active { m.clearComposer() m.clearSuggestions() - m = m.disarmExitConfirmation() + if !m.btw.active { + m = m.disarmExitConfirmation() + } return m, nil } + + if m.btw.active { + return m.leaveBTW() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 1033 - 1046, Consolidate the duplicated composer-clearing condition in the surrounding BTW handling logic: evaluate the shared !m.pending, composerValue, noBlockingModal, transcriptDetailed, and subchat checks once, then retain the branch-specific behavior of calling leaveBTW for active BTW and disarmExitConfirmation for non-BTW before returning. Keep clearComposer and clearSuggestions executed only when the shared condition passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/model.go`:
- Around line 1033-1046: Consolidate the duplicated composer-clearing condition
in the surrounding BTW handling logic: evaluate the shared !m.pending,
composerValue, noBlockingModal, transcriptDetailed, and subchat checks once,
then retain the branch-specific behavior of calling leaveBTW for active BTW and
disarmExitConfirmation for non-BTW before returning. Keep clearComposer and
clearSuggestions executed only when the shared condition passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53f57c0a-29c0-456c-8bb2-be185d35176a
📒 Files selected for processing (2)
internal/tui/btw_test.gointernal/tui/model.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — two minors worth addressing before merge. The isolation (the whole point) is airtight: Store.Fork writes only to the fork, leaveBTW does zero store writes, side prompts persist only to the fork, side→parent message routing can't leak (side runIDs start at parent.runID+btwRunIDGap and only increase; parent routing requires 0 < runID < sideRunIDBase), shallow-copy aliasing is reset to fresh maps/slices, and the edge cases are guarded (nesting blocked, no-session rejected, compaction refused, return-while-pending blocked, side excluded from the resume picker). All 8 new BTW tests + the store fork test pass.
Findings:
- [Minor]
/exitinside a/btwconversation bypasses the parent run's cancel+flush protection — the BTW guard atmodel.go:4129only blocks session-changing kinds, socommandExitreaches the exit path and quits without draining the hidden parent's in-flight run. Add a BTW-aware guard oncommandExit(cancel+drain the parent, or block /exit with a hint) whenm.btw.activeand the parent is pending. - [Minor] The working spinner / elapsed timer freezes after returning from
/btwwhile the main run is still in-flight —spinner.TickMsgcarries no runID sorouteBTWParentMessagedoesn't re-arm it. InleaveBTW, when the restored parent is pending/compacting, batch a spinner tick so the run doesn't look hung. - [Nit] Opening
/btwpermanently stops the main session's active/loop(clearLoopsForSessionSwitchruns before the parent is captured) — snapshot + re-arm on leave, or document it. - [Nit]
btwRunIDGapis a magic1,000,000boundary with no assertion — add a comment/guard thatsideRunIDBasemust stay above any achievable parent runID.
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Really like this. The hard part is the concurrency, and it holds up: capturing the parent model, keeping its in-flight run progressing in the background, and routing its messages back by the runID boundary (side IDs starting a million above the parent's counter). I went through the routing carefully looking for a parent message that btwMessageRunID doesn't cover leaking onto the side surface, and for a side message mis-routed back to the parent, and couldn't find one. The fork/store changes are clean, IsResumableKind correctly hides side sessions from the TUI resume picker, and the "nothing merged into the main session" guarantee genuinely holds for the transcript and the session events.
Two isolation gaps to close before merge, though, and one disk note.
1. Config commands leak out of the isolated conversation (the main one). btwCommandChangesSession only blocks the session-replacing commands (/new /resume /retitle /spec /loop). Everything else runs against the side model and is meant to be discarded on return, but a few of those commands persist to disk and process env, which leaveBTW never unwinds (it restores the in-memory parent snapshot only). Concretely: open a side conversation, run /model some-other-model. That calls SetActiveProviderEnv (os.Setenv ZERO_ACTIVE_PROVIDER), persistSelectedModel (writes the config file), and recordRecentModels. Return with /btw, and the restored parent shows the OLD model in memory while the env var and the on-disk config are the NEW one. So the parent's still-running background turns keep using the old provider client, but any sub-agent or swarm member it spawns inherits ZERO_ACTIVE_PROVIDER = the new one (wrong-provider billing, possible auth failure), and on the next launch or /resume the model chosen in a throwaway side conversation silently wins. /turns and /profile do the same via ZERO_MAX_TURNS, and /theme / /config recaps write their preference to disk too. That directly contradicts the return notice, "Its messages were not added to this session." Simplest fix is to extend btwCommandChangesSession to also block the config-mutating commands (/model, /provider, /turns, /profile, /theme, /config), same as /new. /effort, /style, /selfcorrect are in-memory only and revert cleanly, so those are fine to leave.
2. --resume-latest can resume a discarded side session. The TUI picker filters by IsResumableKind, but store.Latest() doesn't, and the CLI zero exec --resume-latest path uses Latest() directly (exec_session.go, and the preflight twin in cli/exec_sessions.go). So: in the TUI run /btw explain X with an inline question, which runs the side fork and bumps its UpdatedAt to newest, then return. Later from a shell, zero exec --resume-latest "keep going" resolves Latest() to that discarded side session and appends the new turn into the abandoned isolated branch instead of the real main session. It is partly a pre-existing gap (child/spec kinds are unfiltered by Latest() too), but /btw is the first thing that makes a user-created non-resumable session become "latest," so it is now reachable. Routing the resume-latest path through IsResumableKind (or a LatestResumable) closes it.
3. (minor, fine as a follow-up) Each /btw permanently duplicates the session on disk. Fork copies every parent event plus a full physical copy of every checkpoint blob, and side sessions are never deleted (no GC in the sessions package), so a long session with repeated /btw grows disk without bound. Not a blocker, but worth either cleaning up the side session on return or skipping the blob copy for side kinds down the line.
The /loop stop on entry is fine by the way, I checked it, it is the documented session-switch behavior and the "Stopped N loop(s)" line lands in the restored parent transcript, so it is not silent.
Requesting changes for 1 (and ideally 2). The design is solid, these are gaps in the isolation boundary rather than anything structural.
34eb2d4
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/exec_sessions_test.go`:
- Around line 10-20: The existing TestPreflightResumeLatestSkipsSideSessions
covers only resumeLatest; add regression coverage for explicit resume by passing
the side-session ID through execOptions resume handling and asserting
preflightExecSession rejects it. Add corresponding coverage for PrepareExec so
both CLI preflight and execution preparation enforce that side sessions are not
resumable.
In `@internal/sessions/exec_session.go`:
- Line 92: Apply the non-resumable-session validation to the explicit Resume
branch in internal/sessions/exec_session.go at lines 92-92, and mirror the same
check in the CLI preflight in internal/cli/exec_sessions.go at lines 76-76. Add
a regression test for explicitly resuming a side session in
internal/cli/exec_sessions_test.go at lines 10-20, verifying it is rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75b49eb0-5c9c-4411-902c-bde260842fe4
📒 Files selected for processing (7)
internal/cli/exec_sessions.gointernal/cli/exec_sessions_test.gointernal/sessions/exec_session.gointernal/sessions/store_test.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sessions/store_test.go
- internal/tui/btw.go
- internal/tui/model.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed on the new head. Both isolation gaps are closed, and cleanly.
The config-command leak is fixed the way I hoped: /model, /provider, /turns, /profile, /theme, /config are now blocked inside a side conversation, so they can't persist to disk or env behind the reverted parent. And the resume side went further than I asked, both PrepareExec and the CLI preflight now use LatestResumable(), and an explicit --resume <side-id> is rejected with "not resumable" rather than silently resuming the discarded branch. Nice. The two extra fixes you folded in are good catches too: restarting the parent's spinner tick chain on return so it doesn't freeze when the parent is still running, and blocking /exit while the main session is live.
One small thing in the same family, and it's a one-liner: /stt-model (commandSTTModel) also persists to the user config (SetSTTProvider/SetSTTModel via the STT picker, plus SetSTTLocalEngine on the dictation-download path), so it has the identical disk-vs-in-memory divergence that /theme did. It just needs commandSTTModel added to btwCommandChangesSession alongside the others. Not worth another round on its own, so I'm approving; please fold it in before this lands (or as a quick follow-up) so the block list covers every disk-persisting config command.
The per-/btw disk duplication (full history + checkpoint blob copy with no GC) is still there, but as I said before that one is fine as a separate follow-up, not part of this.
Good work turning these around. Approving.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — two blockers, both in the isolation boundary rather than the design.
Re-reviewed at a439a1e. The follow-up commits genuinely closed what I raised last round: /exit now drains the hidden parent (model.go:4129, and the len(flushRunIDs) > 0 disjunct is the right call — checking only pending would have missed the post-cancel state), Ctrl+C now carries the !m.pending guard term-for-term (model.go:1034), and the resume rule landed on all four paths with tests. IsResumableKind + ListResumable/LatestResumable is the right shape. The core routing still holds up: side IDs above sideRunIDBase, parent routing requiring 0 < runID < sideRunIDBase, shallow-copy aliasing reset to fresh maps. Nice work.
Two things block, though.
1. [Blocker] Side run IDs collide across /btw re-entry — a cancelled run hijacks the next one (internal/tui/btw.go:95)
side.runID = parent.runID + btwRunIDGap is recomputed from the restored parent on every entry, and leaveBTW (btw.go:148) restores the snapshot — it never writes side.runID back. So every side conversation in a session where the main run is idle gets the identical run-ID sequence. A run cancelled in side conversation #1 is still unwinding in its goroutine; when its agentResponseMsg lands during side conversation #2, model.go:2122 sees msg.runID == m.activeRunID and takes the run-finished branch: clears pending, calls runCancel() on the second run's context, and renders the first question's answer under the second question.
Reproduced against the branch:
side run #1 activeRunID = 1000001 (sideRunIDBase=1000000)
after cancel: pending=false activeRunID=0 flushRunIDs=map[1000001:...]
restored parent runID = 0
side run #2 activeRunID = 1000001 (sideRunIDBase=1000000)
*** COLLISION CONFIRMED ***
*** HIJACK CONFIRMED: stale run #1 response terminated in-flight run #2 ***
--- side #2 transcript ---
[1] second side question
[2] answer to the FIRST question
The Esc-Esc cancel this depends on is the exact gesture btw.go:142 instructs users to use, so it is not an exotic path. Fix: carry the side counter forward instead of deriving it from the parent each time — keep a monotonic btwRunIDSeq that leaveBTW writes back from side.runID, or make sideRunIDBase strictly greater than any previously issued side ID.
2. [Blocker] /rewind inside a BTW conversation reverts the main session's workspace files (internal/tui/btw.go:172)
commandRewind is not in btwCommandChangesSession, and model.go:4137 is the only gate, so /rewind dispatches normally to model.go:4352. Store.Fork deliberately copies the parent's checkpoint events and physically copies its blobs — with the comment "so a rewind on the fork can restore file content" (store.go:461-467) — so the side session resolves the main session's checkpoints against real blobs. handleRewindCommand then calls ApplyRewind(m.activeSession.SessionID, m.cwd, target) (session_controls.go:643), and side.cwd == parent.cwd. The main run's file edits are reverted on disk, then leaveBTW prints "Its messages were not added to this session."
Worse, handleRewindCommand's own safety guards are scoped to the wrong model: m.pending (:629) and len(m.flushRunIDs) (:635) read the side model's fields, which handleBTWCommand zeroed at btw.go:92,96. So the rewind is permitted while the hidden main run is still in flight writing the very files being restored — the guards that exist specifically to prevent that are blind here.
Add commandRewind (and commandCompact) to btwCommandChangesSession. If /rewind must stay available, it has to consult m.btw.parent.pending / m.btw.parent.flushRunIDs.
Majors
/resume <side-id>in the TUI is unguarded (internal/tui/session.go:276).resolveResumeSessionresolves an explicit id via a baresessionStore.Getwith noIsResumableKindcheck. Thelatestbranch and both pickers filter; the CLI is guarded atexec_sessions.go:75andexec_session.go:108. The TUI — where/btwactually lives — was missed. The id is discoverable (zero sessions listuses unfilteredList()). Resuming it puts the user in a session with no BTW banner and no return affordance, whilesessionPromptkeys the isolation preamble offSessionKind(session.go:254) notm.btw.active— so every prompt is silently prefixed with "Do not modify workspace state…" and the agent hedges on ordinary edits for no visible reason. One line mirrorsexec_sessions.go:75.leaveBTWignores the side model'sflushRunIDs(btw.go:141). It gates onpendingandcompactInFlightonly. An Esc-Esc-cancelled side run leavespending == falsewithflushRunIDsnon-empty; the side model is then discarded, so the lateagentResponseMsgfinds no entry atmodel.go:2131and the run's checkpoint blobs are never referenced by any event. This PR already treats exactly that as a busy signal for the parent atmodel.go:4130— the omission is inconsistent with its own contract./btwbypasses them.exitingguard (btw.go:134).model.go:4100is scoped tocommand.kind == commandPrompt, but/btw <question>iscommandBTWand reachesside.launchPrompt, so it forks a session and starts a paid run during the deferred-quit window after Ctrl+C-Ctrl+C.side.exitingis also never reset among the ~30 fieldshandleBTWCommandclears, so plain follow-up prompts in the side surface are then silently swallowed by that samemodel.go:4100guard, with no transcript message.- No persistent BTW indicator. Entry is announced by one scrollable transcript row;
footerViewandstatusLinecarry no BTW chip, and the(BTW)title never reaches the chrome. After a few exchanges nothing on screen distinguishes the isolated surface from the main session — and work typed there is discarded on return. The status line already treats hidden state this way for loops ("so a running loop is always visible",view.go:236-242); this needs the same. - Terminal resizes during
/btware lost.tea.WindowSizeMsgcarries no runID sobtwMessageRunIDdoesn't route it; the parent snapshot keeps its entry-time geometry andleaveBTWrestores it verbatim. Bubble Tea won't re-emit, so the main session stays wrapped at the old width until the user resizes again.
Minors / nits
- The spinner fix is half-done (
btw.go:160). The!parent.reducedMotionconjunct is wrong — no other arming site gates on it (model.go:4625,session_controls.go:594), and the stockTickMsghandler deliberately keeps the chain alive under reduced motion. UnderZERO_REDUCED_MOTIONor no-TTY the elapsed timer still freezes on return, and there the clock is the only liveness cue by design (model.go:3280-3286). Deleting the conjunct fixes it. - Config blocklist has two gaps and one over-reach.
/sttmodel(stt_model_picker.go:220,244) and/mcp(command_views.go:110) still write user config from inside a side conversation and leave the restored parent stale relative to disk. Conversely, gating bycommandKindnow also blocks the read-only forms — bare/turns,/config,/provider,/profile statusall return "unavailable in a BTW conversation", and asking "what model am I on?" is a reasonable thing to want in a side chat. - Side spend is billed to the main session.
usageTrackeris a*usage.Tracker;parent := mandside := mcopy the same pointer andleaveBTWnever unwinds it, so an expensive side turn shows up in the main session's cost readout and/context. That this is per-conversation state is established bystartNewSessioncallingReset(). Give the side model its own tracker and drop it on return. zero sessions list --kind sideis rejected (internal/cli/sessions.go:219) even though the same command prints[side]rows. Add the kind toparseSessionKindFlagand the--kindhelp text.- A failed
/btwstill kills the user's loops (btw.go:52). The clear happens before the fork is attempted, so both error returns hand back a model whose loops are gone — with "Stopped 1 loop(s) before opening the BTW conversation." directly above "Could not open BTW conversation: …". Move the clear onto thesidecopy after the fork succeeds. (The loop-stop-on-entry behavior itself I'm fine with, as discussed — but the notice is written to the parent transcript and only becomes visible after return, which is the wrong moment to see it.) ?overlay still says Ctrl+C means "cancel the run, then quit" (keybinding_help.go:41), which is false in BTW. README was updated; the in-app overlay wasn't, and nothing gates the overlay onm.btw.active.btwRunIDGapis still a bare1_000_000with no stated invariant. Worth a comment — and note the real invariant is relative ("the parent must not advance by ≥ gap during one side conversation"), not absolute.- Disk growth (
store.go:447-457,checkpoint.go:218-228): every/btwcopies the full event log plus a byte copy of every blob, with no GC anywhere. Fine as a follow-up, but the resume-hiding makes these orphans unreachable through every product surface while still consuming disk. Blobs are content-addressed and immutable, soos.LinkincopyBlobswould make the blob half free.
Things I looked at and cleared
zero exec --fork <side-id> producing a resumable fork is pre-existing --fork semantics applied uniformly to child/spec-draft/spec-impl already — not something this PR broke. Same for /add-dir, "always allow" permission grants, and /stop reaching process-scoped state: all real mechanisms, all pre-existing and out of scope for the promise this PR makes ("nothing from this side conversation will be merged into the main session"), which does hold for transcript and session events. Worth separate tickets at most.
Verification
gofmt clean, go vet clean, go test -race ./internal/tui -run TestBTW -count=2 and -race ./internal/sessions -run TestStoreFork -count=2 both pass. The internal/cli failures I hit locally (TestRunAddDir…, TestRunDoctor…, TestRunSandboxCheck…) are sandbox/network artifacts of my environment, unrelated to this diff — CI is green on all 9 checks.
To reproduce blocker 1, drop this in internal/tui/ and run go test ./internal/tui/ -run TestBTWRunIDReuse -v -count=1:
func TestBTWRunIDReuse(t *testing.T) {
m := newBTWTestModel(t)
m.provider = &fakeProvider{}
m.runID = 0
side1, _ := m.handleBTWCommand("first side question")
first := side1.activeRunID
side1.cancelRun()
back, _ := side1.leaveBTW()
side2, _ := back.handleBTWCommand("second side question")
if first != side2.activeRunID {
t.Fatalf("no collision: %d vs %d", first, side2.activeRunID)
}
updated, _ := side2.updateModel(agentResponseMsg{
runID: first,
rows: []transcriptRow{{kind: rowAssistant, text: "answer to the FIRST question"}},
})
if transcriptContains(updated.(model).transcript, "answer to the FIRST question") {
t.Fatalf("stale run #1 answer rendered under side conversation #2")
}
}Merge is kevin's call per the program gate.
dc94e60
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/btw.go (1)
75-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLong manual field-reset block is a drift risk.
Building
side(and preservingparent) relies on ~35 explicit field resets. Any futuremodelfield added for run/composer/usage state that isn't added here will silently leak between the hidden parent and the isolated side session (or fail to restore onleaveBTW), and no test will catch the omission until it manifests as a real isolation break. Consider centralizing this into amodel.resetForkedRunState()-style helper (or a small struct listing "session-scoped" fields) so new fields have one obvious place to be accounted for.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/btw.go` around lines 75 - 155, The BTW setup in the function containing the side-session initialization should replace the long manual reset block with a centralized helper such as model.resetForkedRunState(), covering all run, composer, streaming, usage, permission, and loop-scoped state while preserving session-specific fields and the existing BTW initialization. Update leaveBTW restoration to use the same centralized state definition so future fields cannot leak between parent and side sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/btw.go`:
- Around line 75-155: The BTW setup in the function containing the side-session
initialization should replace the long manual reset block with a centralized
helper such as model.resetForkedRunState(), covering all run, composer,
streaming, usage, permission, and loop-scoped state while preserving
session-specific fields and the existing BTW initialization. Update leaveBTW
restoration to use the same centralized state definition so future fields cannot
leak between parent and side sessions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1de94e7f-b47f-4349-8662-37a6655a55b4
📒 Files selected for processing (8)
internal/cli/sessions.gointernal/cli/sessions_test.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/session.gointernal/tui/view.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/model.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE. Re-reviewed at dc94e60. Both blockers are genuinely fixed — I re-ran my repros against the new head rather than reading the diff — and 14 of the 15 items from the last round are closed. The one remaining is the disk-growth follow-up we'd all already agreed was out of scope.
Blockers — verified fixed, not just patched
Run-ID collision. The fix is the right shape: a monotonic btwRunIDSeq on the model, side.runID = maxInt(parent.runID+btwRunIDGap, parent.btwRunIDSeq) on entry and parent.btwRunIDSeq = maxInt(parent.btwRunIDSeq, m.runID) on return, so the counter survives the parent snapshot instead of being re-derived from it. Re-ran the exact repro from my last review:
side run #1 activeRunID = 1000001 (base=1000000, seq=0)
after cancel: pending=false flushRunIDs=map[1000001:...]
leave while flushing -> still in btw? true
after late response: flushRunIDs=map[]
restored parent: runID=0 btwRunIDSeq=1000001
side run #2 activeRunID = 1000002 (base=1000001)
OK: no collision
OK: stale run #1 response ignored (pending still true)
Previously this produced second side question / answer to the FIRST question. Now the stale response is correctly ignored and the in-flight run survives.
/rewind in a side conversation. commandRewind and commandCompact are both in the blocklist now and covered by TestBTWBlocksPersistentConfigurationCommands. That closes the path where a side-conversation rewind reverted the main session's workspace files.
The rest
All confirmed present at dc94e60:
resolveResumeSessionnow gates onIsResumableKind(session.go:283) — the TUI finally matches the two CLI paths.leaveBTWrefuses whilelen(m.flushRunIDs) > 0, so a cancelled side run's checkpoint events aren't dropped.handleBTWCommandrefuses whenm.exiting, and resetsside.exiting = false— you caught the second half, which was the part that silently swallowed follow-up prompts.- Persistent amber
BTW ·chip instatusLine, threaded through all the takeover branches (tiny tier, exit-confirm, cancel-confirm, dictation) rather than just the default one. resizeBTWParentmirrors the geometry half of the realWindowSizeMsghandler field-for-field. Skipping theheaderPrinted/flushQueuescrollback write is correct for a hidden model, andleaveBTWalready re-arms the spinner, so nothing is missed by not callingensureSpinnerTick.!parent.reducedMotiondropped from the spinner re-arm, so the elapsed timer no longer freezes underZERO_REDUCED_MOTION/ no-TTY.commandSTTModelandcommandMCPadded, plus independentusage.NewTrackerfor the side model withlastUsage,unpricedRequests/Tokens,compactRequests, and the turn-latency/TTFT counters all reset alongside it — more thorough than what I flagged.--kind sideaccepted, error text and--kindhelp updated.- Loop clearing moved after the fork and
resumeEventssucceed, so a failed/btwno longer destroys the user's loops — and the notice now also lands in the side transcript, which fixes the "you only find out after you return" half I mentioned. ?overlay Ctrl+C text is conditional onm.btw.active.btwRunIDGapnow documents its invariant, and correctly states the relative one.
Two things I liked more than what I proposed:
btwCommandUnavailabletaking the wholeparsedCommandand allowing the read-only forms (/model list,/provider status, bare/turns,/profile status,/theme list, bare/config) is a better answer than my "key off mutating args" hand-wave, and it fixes the over-reach I introduced in the previous round's suggestion.- Deferring the loop clear until after the fork succeeds fixes the error-path leak and the wrong-surface notice in one move.
Checked and clean
/exitwhile the side run is still flushing setsexiting = trueand returns no command — it arms the deferred quit rather than quitting and dropping the flush. So the newflushRunIDsguard doesn't create a trap:/btwand Ctrl+C hold you in the side surface,/exitdefers, and nothing loses the pending checkpoint events.usage.NewTracker(usage.TrackerOptions{Registry: &side.modelCatalog, ...})takes a pointer into a local copy, which I wanted to be sure of — butnewModeldoes exactly the same thing atmodel.go:756, andmodelregistry.Registryis only ever read viaResolve. Harmless and consistent.
One forward-looking note (not blocking)
btwCommandUnavailable now encodes per-command argument allowlists inline (arg != "list", arg != "status", …). That's the right behavior today, but it will rot silently as subcommands are added — a future /theme preview or /config get x gets blocked with no signal that the allowlist is the reason. A short comment naming the rule ("only argument forms that render text and touch no disk/env state are allowed") or a table-driven test enumerating allowed/blocked forms per command would keep it honest.
Still open, by agreement
Unbounded disk growth per /btw (store.go:447-457 full event copy, checkpoint.go:218-228 byte-copied blobs, no GC). Both Vasanth and I flagged this as acceptable as a follow-up. Worth a tracking issue before this merges so it doesn't get lost — os.Link in copyBlobs would make the blob half free, since blobs are content-addressed and immutable.
Verification
gofmt clean, go vet clean. go test -race ./internal/tui -run TestBTW -count=2 and -race ./internal/sessions -run TestStoreFork -count=2 both pass, as do the new internal/cli session-kind tests. Eight new tests landed and map 1:1 onto the findings, including TestBTWCancelledRunFlushesBeforeReturnAndRunIDsStayUnique, TestBTWResizeUpdatesHiddenParent, TestBTWUsesIndependentUsageTracker, and TestBTWFailedForkKeepsMainSessionLoops.
Nice turnaround on this — the run-ID fix in particular is the correct structural answer rather than a patch over the symptom. Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed the hardening pass at dc94e60. My earlier approval was auto-dismissed by the new commit, so re-confirming.
My one open note from last round is handled, and better than I asked: /stt-model is blocked now, but instead of just adding it flat you moved to btwCommandUnavailable(command) and kept the read-only forms usable (/model list, /provider status, bare /turns, /profile status, /theme list, bare /config). Blocking only the disk/env-mutating variants while leaving inspection available is the right call.
I traced the two structural fixes that landed in this pass rather than just reading the summary:
btwRunIDSeqis the correct answer to BTW re-entry, not a patch. Because it lives on the model and survives the parent snapshot (side.runID = maxInt(parent.runID+gap, parent.btwRunIDSeq)on entry,parent.btwRunIDSeq = maxInt(..., m.runID)on return), a straggler from a discarded side run always has a runID below the next side's base, so it routes to the hidden parent rather than being mistaken for a live side message. That closes the collision cleanly.- Deferring the loop clear until after the fork and resumeEvents succeed means a failed
/btwcan no longer destroy the main session's loops, and folding the notice into the side transcript fixes the wrong-surface half in the same move. - The
!reducedMotiondrop on the spinner re-arm is correct, not a regression: the tick handler already freezes the phase increment under reduced motion, so the glyph stays static while the elapsed timer keeps advancing, which is what you want.
The independent usage.NewTracker for the side model plus resetting the latency/TTFT/compact counters is more isolation than strictly required and a good instinct.
Non-blocking, seconding gnanam: the inline per-command argument allowlists in btwCommandUnavailable will rot quietly as subcommands are added (a future /theme preview gets blocked with no signal why). A table-driven test enumerating allowed vs blocked forms per command would keep it honest. And the per-/btw disk duplication is still the agreed follow-up, worth a tracking issue before this lands, os.Link in copyBlobs would make the blob copy free since those blobs are content-addressed and immutable.
CI green, gnanam re-verified against the same head. Approving.
3 participants
Summary
/btw [question]to fork the current context into an isolated side conversation/btwor Ctrl+C without merging side messages into the main session/resumeand guard commands that would replace the side sessionWhy
This lets users validate assumptions and explore related questions without interrupting or expanding the main conversation context.
Closes #637
Verification
make fmt-checkgo vet ./...go test ./...go test -race ./internal/tui -run TestBTW -count=1go test -race ./internal/sessions -run TestStoreFork -count=1go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegovulncheck ./...git diff HEAD --checkSummary by CodeRabbit
/btw [question]for an isolated side conversation that returns to the main session when re-run, with clear BTW status in the UI.--resume/--resumeLatestnow skip non-resumable side sessions and reject resuming non-resumable session kinds./btwdescriptions.