fix(cli): only a pushed frame may say a turn has started

CORRECTION FIRST. 8014af7's message asserts that catch-up history reaches
the client through Merge and never Apply. That is false, and it was false
when it was written. Four production callsites feed a read response
straight into Apply: internal/cli/command.go:353 (desync Read),
command.go:401 and stream.go:260 (the two deliberate pager doors), and
stream.go:295 (readHistoryIntoPager). Only livelog_bridge.go and
transcript.go use Merge. Aria 250ebd07 was asked to falsify the claim and
did; the message is left standing rather than amended, so the record shows
what was believed and what turned out to be true.

THE RACE. Enter the pager while a turn is running and the catch-up
ReadBefore snapshots that turn UNSEALED. The turn ends, turn.done lands
and settles the status, and the read response arrives afterwards still
describing the turn as live. Read as a lifecycle edge it says "a turn is
now running" when none is, and nothing will ever say it stopped: the
spinner runs until the session closes. claimsOpen cannot refuse it,
because the snapshot was taken before the seal.

THE CONTENT-BEARING HALF OF THIS PREDATES THE BRANCH. OnLive called
beginTurn on any frame carrying assistant output, read or push, and has
since it was written. A stale catch-up read carrying nodes could relight a
finished turn on main today, with no part of this branch involved.
8014af7 widened the hole from content-bearing pages to inquiry-only ones;
it did not open it. Both halves are fixed here, because a known silent bug
deferred is a bug nobody fixes.

PROVENANCE, NOT A STALENESS CHECK. Apply is the push door and ApplyRead is
the read door: same fold, same order, withholding only the turn-open edge.
A staleness check was considered and rejected. rpc.DoneEntry is {reason,
idle} and carries no turn id, so such a check would have to infer which
turn ended, which is guessing about precisely the ordering that is already
broken, and a heuristic that is usually right is the worst available shape
for a failure that is silent. The caller is holding either a response or a
notification and always knows which; this makes it say so. Routing a read
into the push door now takes typing the wrong word on purpose.

ONE OWNER, NOT TWO GUARDS. Guarding OnTurnOpen alone fixed only the newer
half, which the table-driven test caught. All three routes into the open
slot (the question arriving first, a part carrying nodes, a live delta)
now go through one claim helper that takes the slot AND reports the edge,
so the two cannot drift apart and the push-only gate lives in one place.
OnLive no longer touches the status.

Deleting OnLive's arming needed its own test, because it did cover a case
the inquiry path does not: a client attaching mid-turn receives a live
delta as its first frame, carrying no question, because the question was
broadcast before it was listening. claim() now reports that from the
nodes and live branches. TestJoiningATurnInFlightMarksItRunning pins it,
and replaces a companion test that asserted the duplicate mechanism this
commit removes.

The stale-read test is table-driven over both shapes and fails on the tip:
inquiry-only fails against 8014af7, content-bearing fails against main.

Also narrows a comment from the first commit. "so /nix/store detection is
robust" became "works in every install layout", which traded a banned word
for an absolute claim; it now says what EvalSymlinks actually does.

Author: gluck

internal/cli/command.go

@@ -350,7 +350,7 @@ func (in *interactiveInput) desyncHandler(gen uint64) func(int) {
 				return
 			}
 			in.mu.Lock()
-			in.lt.apply(r)
+			in.lt.applyRead(r)
 			in.mu.Unlock()
 		}()
 	}
@@ -398,7 +398,7 @@ func (in *interactiveInput) seedSubject() {
 	defer in.mu.Unlock()
 	in.caughtUp = rerr == nil
 	if rerr == nil {
-		in.lt.apply(r)
+		in.lt.applyRead(r)
 		in.lt.setMoreBefore(r.More.Before)
 	}
 	in.wireHooks()

internal/cli/livelog_bridge.go

@@ -68,7 +68,7 @@ type livelogTurn struct {
 	// Holding is a few milliseconds at the very start of a session and nothing
 	// is dropped: openInline applies every held page in arrival order.
 	hold bool
-	held []aria.Page
+	held []heldPage
 
 	// seeded is the catch-up page fetched when we joined a turn we did NOT open.
 	// The inline view prints a bounded slice of it (seedContext); the PAGER gets
@@ -199,7 +199,12 @@ func (t *livelogTurn) wireClient() {
 				t.finished = false
 			}
 			t.thinkingOpen = false // adopted by real content
-			t.status.beginTurn()
+			// The status is NOT armed here. OnTurnOpen owns that edge now,
+			// and it fires for pushes only. This line used to arm on any
+			// content frame, which meant a catch-up read that snapshotted a
+			// live turn could relight a turn that had already ended, with no
+			// later event to put it out. That predates OnTurnOpen; it is the
+			// same race, on the older half.
 		}
 		// A turn taller than the viewport can't render inline without scrolling
 		// its own live region off-screen; move it to the scrollable pager
@@ -346,12 +351,31 @@ func (t *livelogTurn) armThinking() {
 // conversation.
 func (t *livelogTurn) setMoreBefore(more bool) { t.client.SetMoreBefore(more) }
 
-func (t *livelogTurn) apply(r aria.Page) {
+// apply folds a PUSHED frame. applyRead folds a READ RESPONSE, which is the
+// same fold minus the turn-open edge: see aria.Client.ApplyRead for the race
+// that separates them. The two doors exist so a callsite has to say which it
+// is holding, because from the page alone nothing can tell.
+func (t *livelogTurn) apply(r aria.Page) { t.applyPage(r, true) }
+
+func (t *livelogTurn) applyRead(r aria.Page) { t.applyPage(r, false) }
+
+func (t *livelogTurn) applyPage(r aria.Page, live bool) {
 	if t.hold {
-		t.held = append(t.held, r)
+		t.held = append(t.held, heldPage{page: r, live: live})
+		return
+	}
+	if live {
+		t.client.Apply(r)
 		return
 	}
-	t.client.Apply(r)
+	t.client.ApplyRead(r)
+}
+
+// heldPage is a buffered page and where it came from. Provenance survives the
+// hold because it is not recoverable from the page: see applyPage.
+type heldPage struct {
+	page aria.Page
+	live bool
 }
 
 // holdFrames buffers applied pages until openInline. Armed before the
@@ -366,12 +390,12 @@ func (t *livelogTurn) openInline(fetched historyPage) {
 	// store when the pager opens: see enterPager).
 	t.seeded, t.seedExtents, t.seedMore = fetched.msgs, fetched.extents, fetched.more
 	t.seedContext(fetched.msgs) // no-op when there is nothing to orient with
-	t.armThinking()             // no-op in the pager, and no-op if already pinned
+	t.armThinking()             // the incipit half is a no-op in the pager
 	t.hold = false
 	held := t.held
 	t.held = nil
-	for _, r := range held {
-		t.client.Apply(r)
+	for _, h := range held {
+		t.applyPage(h.page, h.live)
 	}
 	// The pager draws from the shared model rather than from the event, so a
 	// page that finalized nothing would leave it showing the pre-release state.

internal/cli/stream.go

@@ -257,7 +257,7 @@ func (in *interactiveInput) enterPager(history bool) {
 	in.lt.setQueuedFetch(in.refreshQueued)
 	in.lt.setHistoryFetcher(in.historyFetcher())
 	if rerr == nil {
-		in.lt.apply(r)
+		in.lt.applyRead(r)
 		// The wire's own answer to "is there anything before this page", which
 		// is the only honest source for it: the pager reads it back as "can I
 		// still page older history" instead of latching a bit of its own.
@@ -292,7 +292,7 @@ func (in *interactiveInput) readHistoryIntoPager() {
 	// inline branch of OnClosed (which freezes to native scrollback) is not
 	// reachable while tr.active, which is the trap this whole design is built
 	// around.
-	in.lt.apply(r)
+	in.lt.applyRead(r)
 	in.lt.setMoreBefore(r.More.Before)
 	// The window was derived from the tail before this page existed; re-derive it
 	// so the rows that just arrived are reachable by a scroll.

internal/cli/thinking_armed_test.go

@@ -65,28 +65,26 @@ func TestInquiryFrameMarksTheTurnRunning(t *testing.T) {
 	}
 }
 
-// The companion that proves the two above can fail: content-bearing frames
-// arm the status by a different route (OnLive), so a test that fed one would
-// pass whether or not the inquiry path works at all.
-func TestInquiryArmingIsNotJustOnLive(t *testing.T) {
+// JOINING A TURN ALREADY IN FLIGHT. A client that attaches mid-turn, or
+// reconnects, gets a live delta as its FIRST frame: the question was
+// broadcast before it was listening, so no inquiry rides on it. That case
+// used to be covered by OnLive arming on any content frame, which is the
+// half that also caused the stale-read bug. It must survive the removal.
+func TestJoiningATurnInFlightMarksItRunning(t *testing.T) {
 	var out bytes.Buffer
 	status := newSessionStatus("aria1234", time.Now())
 	lt := newLivelogTurn(&out, 80, 20, &renderSettings{}, "aria1234", time.Now(), status, nil, nil)
 	lt.enterTranscript()
 
-	lt.apply(inquiryPage(7, "WHAT IS THE GAP?"))
-	armedByInquiry := status.turnRunning()
-
-	status.setTurn(turnStatusIdle)
+	// No inquiry: we were not listening when it was asked.
 	lt.apply(aria.Page{Parts: []aria.TurnPart{{Turn: aria.Turn{
-		ID: 7, Live: &aria.Live{From: 0, Nodes: []aria.NodeDelta{{
-			ID: 0, Set: map[string]any{"type": "prose", "markdown": "the answer"},
+		ID: 57, Live: &aria.Live{From: 0, Nodes: []aria.NodeDelta{{
+			ID: 0, Set: map[string]any{"type": "prose", "markdown": "mid sentence"},
 		}}},
 	}}}})
-	armedByContent := status.turnRunning()
 
-	if !armedByInquiry || !armedByContent {
-		t.Fatalf("inquiry=%v content=%v, want both true", armedByInquiry, armedByContent)
+	if !status.turnRunning() {
+		t.Fatal("joined a running turn and showed idle")
 	}
 }
 
@@ -109,3 +107,75 @@ func TestSealedFrameDoesNotMarkTheTurnRunning(t *testing.T) {
 		t.Fatalf("a sealed turn lit the spinner:\n%s", strings.TrimSpace(out.String()))
 	}
 }
+
+// THE ORDERING RACE A SEALED-PART GUARD DOES NOT COVER.
+//
+// Enter the pager while a turn is running and the catch-up ReadBefore
+// snapshots that turn UNSEALED. The turn then ends, turn.done lands and
+// settles the status, and the read response arrives afterwards still
+// describing the turn as live. claimsOpen cannot refuse it: the snapshot was
+// taken before the seal, so it is not a sealed part.
+//
+// Treated as a lifecycle edge it says "a turn is now running" when none is,
+// and nothing will ever say it stopped. The spinner runs until the session
+// closes. So a read does not carry that edge at all, which is the whole
+// difference between apply and applyRead.
+func TestStaleCatchUpReadDoesNotRelightTheSpinner(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		page aria.Page
+	}{
+		{"inquiry only", inquiryPage(9, "A QUESTION")},
+		{"with content", aria.Page{Parts: []aria.TurnPart{{Turn: aria.Turn{
+			ID: 9, Inquiry: "A QUESTION", Live: &aria.Live{From: 0, Nodes: []aria.NodeDelta{{
+				ID: 0, Set: map[string]any{"type": "prose", "markdown": "the answer"},
+			}}},
+		}}}}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			var out bytes.Buffer
+			status := newSessionStatus("aria1234", time.Now())
+			lt := newLivelogTurn(&out, 80, 20, &renderSettings{}, "aria1234", time.Now(), status, nil, nil)
+			lt.enterTranscript()
+
+			lt.apply(inquiryPage(9, "A QUESTION")) // the turn ran
+			lt.finishTurn("stop")                  // and ended
+			if status.turnRunning() {
+				t.Fatal("still running after turn.done")
+			}
+
+			lt.applyRead(tc.page) // the read that was already in flight
+
+			if status.turnRunning() {
+				t.Fatal("STUCK SPINNER: a stale catch-up read relit a finished turn, " +
+					"and no later event will put it out")
+			}
+		})
+	}
+}
+
+// The companion: the SAME page through the push door must still arm, or the
+// test above would pass just as well with the whole feature deleted.
+func TestTheReadDoorIsWhatWithholdsTheEdgeNotThePage(t *testing.T) {
+	page := inquiryPage(11, "A QUESTION")
+
+	armed := func(push bool) bool {
+		var out bytes.Buffer
+		status := newSessionStatus("aria1234", time.Now())
+		lt := newLivelogTurn(&out, 80, 20, &renderSettings{}, "aria1234", time.Now(), status, nil, nil)
+		lt.enterTranscript()
+		if push {
+			lt.apply(page)
+		} else {
+			lt.applyRead(page)
+		}
+		return status.turnRunning()
+	}
+
+	if !armed(true) {
+		t.Fatal("the push door stopped arming; the feature is gone, not fixed")
+	}
+	if armed(false) {
+		t.Fatal("the read door armed")
+	}
+}

internal/livelog/aria/client.go

@@ -277,14 +277,45 @@ func (c *Client) OpenAnimating() bool {
 	return c.store.OpenAnimating()
 }
 
-// Apply folds one page.
-// Apply folds a Page into the local view.
-func (c *Client) Apply(p Page) {
+// Apply folds one PUSHED frame: a figaro.aria notification, which describes
+// the aria as it is right now. Only a push may report lifecycle edges, which
+// is the whole difference between this and ApplyRead.
+func (c *Client) Apply(p Page) { c.apply(p, true) }
+
+// ApplyRead folds a READ RESPONSE: the answer to figaro.read or a backward
+// page. Same fold, same order, one thing withheld: a read describes the aria
+// AS IT WAS WHEN THE DAEMON SERVED IT, which may be several events ago, so it
+// may not report a turn as starting.
+//
+// The race that forces the distinction: enter the pager while a turn is
+// running and the catch-up ReadBefore snapshots the turn unsealed. The turn
+// then ends, turn.done lands and settles the status, and the read response
+// arrives after it, still describing the turn as live. Fired as a lifecycle
+// edge that says "a turn is now running", with no later event to say it
+// stopped, and the spinner runs until the session is closed. The sealed-part
+// guard in claimsOpen does not cover it, because the snapshot was taken
+// before the seal.
+func (c *Client) ApplyRead(p Page) { c.apply(p, false) }
+
+// pushed says the page came from a notification. See Apply and ApplyRead.
+func (c *Client) apply(p Page, pushed bool) {
 	c.mu.Lock()
 	var finalized []Message
 	desync := -1
 	// opened is the turn that took the open slot on this page, 0 for none.
 	opened := 0
+	// claim takes the open slot AND reports the lifecycle edge, so the two
+	// cannot drift apart. Every route into the slot goes through here: the
+	// question arriving first (the common case), and a client that joined a
+	// turn already in flight, whose first frame is a live delta carrying no
+	// question at all. Reads claim the slot but report nothing (ApplyRead).
+	claim := func(id int) {
+		c.store.ClaimOpen(id)
+		if pushed && id > c.openedTurn {
+			c.openedTurn = id
+			opened = id
+		}
+	}
 	metrics := p.Metrics
 
 	for _, part := range p.Parts {
@@ -311,21 +342,16 @@ func (c *Client) Apply(p Page) {
 			// describes a turn we hold only the tail of, and claiming the open
 			// slot for it would destroy the live turn on its way past.
 			if staged && !part.ClippedHead {
-				c.store.ClaimOpen(id)
-				// This is the earliest a client can know a turn is running:
-				// the question is committed and the provider has not been
-				// called yet. Reported outside the lock with everything else.
-				if id > c.openedTurn {
-					c.openedTurn = id
-					opened = id
-				}
+				// The earliest a client can know a turn is running: the
+				// question is committed and no provider has been called yet.
+				claim(id)
 			}
 		}
 
 		// Adopt any nodes the part carries. From is the positional id of
 		// Nodes[0], so a clipped part slots into place rather than replacing.
 		if len(part.Nodes) > 0 && staged {
-			c.store.ClaimOpen(id)
+			claim(id)
 			// A part CLIPPED off the head of a turn is ALL we hold of it: the
 			// nodes below From were never delivered, so they are not ours to
 			// release. Floor the emit cursor at From, or absorb's padding slots
@@ -340,7 +366,7 @@ func (c *Client) Apply(p Page) {
 		}
 
 		if part.Live != nil && staged {
-			c.store.ClaimOpen(id)
+			claim(id)
 			c.store.SetLiveFrom(part.Live.From)
 			for _, nd := range part.Live.Nodes {
 				c.store.FoldAt(nd)

internal/update/update.go

@@ -62,7 +62,7 @@ func DetectChannel() Channel {
 // without a binary in every install layout.
 func channelFor(exe string) Channel {
 	// Resolve one level of symlink (Nix wrappers, ~/.nix-profile, …)
-	// so /nix/store detection works in every install layout.
+	// so /nix/store detection sees the real path.
 	if resolved, err := filepath.EvalSymlinks(exe); err == nil {
 		exe = resolved
 	}