diff --git a/CLAUDE.md b/CLAUDE.md index f127d5b4d..45e1aa5d8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -478,7 +478,7 @@ When `session.enabled` is true (default) and `listener.session_api_addr` is non- | Method & Path | Format | Purpose | |---|---|---| | `GET /` | text | One-line-per-endpoint index. Answers "is this the session API, and on the right port?" — the reason a 404 here was worth replacing. | -| `GET /v1/sessions` | `application/json` | List active sessions: `{sessions: [{id, createdAt, updatedAt, eventCount, title, totalTokens, costMicros, avoidedMicros, saturated, active, promptContext}]}`. `id`, `createdAt`, `updatedAt`, `eventCount` and `active` are always present; every other field is `omitempty` — absent rather than zero, on the standing rule that an unknown value must not render as a real one. (Do not read that off the position of `active`: it sits second-to-last, between two `omitempty` fields.) **`title` is a suggestion, not an identifier:** the proxy derives it from the session's own events (a `/rename`, else a ``, else ordinary user prose, with `` blocks excised), so it is a display convenience and nothing addresses a session by it. Absent when nothing in the events named it. **Folded at append time and FIRST-WINS, except that a `/rename` always overrides** — so ordinary conversation does not re-title a session on every turn, and a `/rename` survives eviction of the event that carried it. abctl does not read this field yet — its TITLE column still comes from harvested Claude Code transcripts, and reconciling the two is outstanding. | +| `GET /v1/sessions` | `application/json` | List active sessions: `{sessions: [{id, createdAt, updatedAt, eventCount, title, totalTokens, costMicros, avoidedMicros, saturated, active, promptContext}]}`. `id`, `createdAt`, `updatedAt`, `eventCount` and `active` are always present; every other field is `omitempty` — absent rather than zero, on the standing rule that an unknown value must not render as a real one. (Do not read that off the position of `active`: it sits second-to-last, between two `omitempty` fields.) **`title` is a suggestion, not an identifier:** the proxy derives it from the session's own events (a `/rename`, else a ``, else ordinary user prose, with `` blocks excised), so it is a display convenience and nothing addresses a session by it. Absent when nothing in the events named it. **Folded at append time and FIRST-WINS, except that a `/rename` always overrides** — so ordinary conversation does not re-title a session on every turn, and a `/rename` survives eviction of the event that carried it. abctl reads this field as a FALLBACK: its TITLE column prefers a harvested Claude Code transcript title and uses the served title only for a session the harvest cannot name. That precedence is fixed rather than a judgement about which string is better — both sides rank candidates their own way and do not agree on every session. That is the case worth having: an agent with no transcript tree on the operator's disk still routes through the proxy, so a row that used to render blank now has a name. abctl deliberately still treats such a row as unnamed for its own re-harvest backoff, so a served title does not stop it looking for a harvested one. | | `GET /v1/sessions/{id}` | `application/json` | The session's most recent events. `?limit=N` (default 500, max 2000) sets the window; `?before=` returns the page ending just before that event, so the whole session is reachable by paging backward from the tail. `totalEvents` is the session's true length and `oldestSeq` the oldest event the store still holds — both present only when this response is not the whole session, so a client can tell "this is the beginning" from "there is more behind me" without a second request. 404 if unknown/expired. **One response is still not a full snapshot:** with `session.max_events` unset a session can hold thousands of events, and one real session's whole history encoded to 1.1GB — 17s to write, against clients that time out in 10. That cap is why `before` exists — until it did, a session past 2000 events had a beginning no request could reach at any limit, while still costing memory. The response is written one event at a time rather than encoded whole, so serving it costs the proxy heap proportional to one event; see the chatty-traffic gotcha below. | | `GET /v1/events` | `text/event-stream` | SSE stream of new events. Optional `?session=` filters to one session. Heartbeat every 30s. | | `GET /v1/pipeline` | `application/json` | Active pipeline composition: `{inbound: [...], outbound: [...]}`. Each plugin entry carries `name`, `direction`, `position`, `readsBody`, plus the static metadata (`requires`, `requiresAny`, `description`) and runtime `config` when present. abctl renders this as the Pipeline pane. | diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index df948a8ef..07546c734 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -124,6 +124,13 @@ reads those transcripts and writes what it finds to `~/.cortex/session-metadata.json`, so the sessions table can show a `TITLE` column instead of a bare id. +This is **one of two** routes to a name, and the one this section is about. The +proxy also derives a title from a session's own events and serves it on +`/v1/sessions`; `TITLE` prefers the harvested name and falls back to that one, +so the column can be populated for a session with no transcript here at all. +Everything below concerns the harvest only — including `--skip-claude-metadata`, +which suppresses this route and not the served fallback. + The scan runs in the **background**, while the viewer is already up: `abctl observe` paints immediately and the titles appear when the scan finishes — usually before you have picked a pod. The viewer opens with whatever titles the last run recorded, so a scan @@ -150,15 +157,20 @@ full scan, and milliseconds once the file exists. Pass `--skip-claude-metadata` when the scan is unwanted, or when `~/.claude` should simply not be touched. It suppresses only the *scan*: the viewer still reads `~/.cortex/session-metadata.json`, so titles recorded by earlier runs keep rendering and -only sessions new or renamed since the last scan show as bare ids. There is no flag that -hides titles already on disk — delete the file for that. - -A harvest that cannot run is never fatal — the worst a missing or unreadable file -costs is the `TITLE` column, and the viewer still opens. A file that does not parse is -rebuilt from the transcripts rather than costing anything; the entries a rebuild cannot -recover are sessions whose transcripts Claude Code has already pruned. The failures that -need a human, such as an unreadable metadata file, print one line to stderr with the -repair before the viewer starts; success says nothing. +only sessions new or renamed since the last scan go unharvested — and those still show the +title the proxy serves, if it derived one, rather than a bare id. There is no flag that +hides titles already on disk — delete the file for that, and note that it does not suppress +the served title either, which arrives over the API and not from any file. + +A harvest that cannot run is never fatal, and it costs less than it used to: the viewer +still opens, and a session the proxy has named still shows that name, because the served +title arrives over the API and not from this file. What a missing or unreadable file +costs is therefore the `TITLE` column only for sessions the proxy has not named — which, +on a machine whose agents all route through the proxy, may be none of them. A file that +does not parse is rebuilt from the transcripts rather than costing anything; the entries +a rebuild cannot recover are sessions whose transcripts Claude Code has already pruned. +The failures that need a human, such as an unreadable metadata file, print one line to +stderr with the repair before the viewer starts; success says nothing. The config directory is `CLAUDE_CONFIG_DIR` when set, and `~/.claude` otherwise. To read a different directory, or to force a full re-read of every @@ -577,15 +589,20 @@ abctl is for, and the other three are surfaces you visit and leave. - **Sessions** (default): table of active sessions in the store, most recently updated first. Columns: session (truncated), title, updated - (relative), event count, tokens, cost, saved, context. `TITLE` is populated - from Claude Code's transcripts — see + (relative), event count, tokens, cost, saved, context. `TITLE` comes from + Claude Code's transcripts — see [`--skip-claude-metadata`](#naming-sessions-from-claude-code---skip-claude-metadata) - — and is empty for a session nothing has harvested. The proxy now also derives - a title of its own from the session's events and reports it as `title` on - `/v1/sessions`; **this pane does not read that field yet**, so a harvested - title is still the only thing that fills this column. Reconciling the two is - outstanding work. Numerics are right-aligned - so the digits line up between rows. + — and **falls back to the title the proxy serves** on `/v1/sessions`, which it + derives from the session's own events. So a session with no transcript on this + machine can still be named, and the cell is empty when neither source names it + — or when the proxy has stopped listing the session, since a row kept alive by + its cached events alone has no summary to carry a served title. A session named + only by the proxy therefore loses its name at that point while its events + remain, which is the one case where a title visibly disappears. The harvested + title wins when both exist — a fixed precedence, not a claim that it is always + the better string; the two sides rank candidates differently and may not agree + on a given session. The column does not say which source it used. Numerics are + right-aligned so the digits line up between rows. `CONTEXT(1M)` is a gauge, not a figure: how full the **conversation's** context was on its latest turn, against a fixed one-million-token window. The diff --git a/cmd/abctl/main.go b/cmd/abctl/main.go index 876182019..5b8605322 100644 --- a/cmd/abctl/main.go +++ b/cmd/abctl/main.go @@ -199,7 +199,11 @@ func wantsInfoFlagOnly(args []string) bool { // which is why this exists at all. func observeHarvester(f observeFlags, warn io.Writer) tui.HarvestFunc { // Nil under --skip-claude-metadata, which is what turns the harvest off: the viewer then - // shows whatever titles the metadata file already held, from the last run. + // shows whatever titles the metadata file already held, from the last run — AND the titles + // /v1/sessions serves, which this flag does not touch. It declines a filesystem scan, not + // naming: a session the proxy has named still shows that name with the harvest off entirely. + // The flag help one line below says the same; both are here because "harvest off" reads as + // "no titles" and has stopped meaning that. if *f.skipClaudeMetadata { return nil } @@ -405,7 +409,7 @@ func registerObserveFlags(fs *flag.FlagSet) observeFlags { // flag to decline is narrower, and it is the reason to keep it: a machine where // ~/.claude should simply not be touched. skipClaudeMetadata: fs.Bool("skip-claude-metadata", false, - "do not harvest session titles from Claude Code's transcripts. By default abctl observe scans CLAUDE_CONFIG_DIR / ~/.claude in the background once the viewer is up and records titles in ~/.cortex/session-metadata.json, so sessions show a name instead of a bare UUID. This skips the scan; titles already recorded by earlier runs are still shown, so only sessions new or renamed since the last scan appear as bare ids."), + "do not harvest session titles from Claude Code's transcripts. By default abctl observe scans CLAUDE_CONFIG_DIR / ~/.claude in the background once the viewer is up and records titles in ~/.cortex/session-metadata.json, so sessions show a name instead of a bare UUID. This skips the scan; titles already recorded by earlier runs are still shown, and a session the harvest has not named falls back to the title the proxy serves, so a bare id usually means neither source named it — except for a session the proxy has stopped listing, whose served title is not retained and so goes away with the listing."), } } diff --git a/cmd/abctl/tui/app.go b/cmd/abctl/tui/app.go index 484d5c8d4..8ac9dac28 100644 --- a/cmd/abctl/tui/app.go +++ b/cmd/abctl/tui/app.go @@ -428,10 +428,16 @@ type model struct { // events stop carrying the evidence (view=summary strips it) while the answer stays true. // Which turn wins is not a question of size — see pipeline.PromptContextFold. contextRun map[string]pipeline.PromptContextFold - // sessionsData is what an agent knows about its own sessions that the proxy does - // not — a title, mostly. Read once at startup from ~/.cortex/session-metadata.json, - // which `abctl experimental read-claude-sessions` writes; empty when that has never - // run, which renders as an empty TITLE column rather than as a failure. + // sessionsData is what the HARVEST knows about an agent's sessions — a title, mostly. + // Read once at startup from ~/.cortex/session-metadata.json, which + // `abctl experimental read-claude-sessions` writes; empty when that has never run, + // which is not a failure. + // + // NO LONGER THE ONLY THING THAT NAMES A SESSION, and this doc claimed both halves of + // that. It is not "what the proxy does not know": /v1/sessions serves a title derived + // from the session's own events, and sessionTitleFor falls back to it. So an empty map + // renders an empty TITLE column only for sessions the proxy has not named either — the + // two sources overlap rather than partition. This map still WINS where both have one. // // Keyed by the same session id the proxy buckets on, so a lookup is direct. Nil-safe // by construction: a read on a nil map yields the zero SessionMetadata, so an @@ -1299,6 +1305,13 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Repaint what names sessions. The sessions table is the only place a title is // rendered into a cell; every other user of sessionLabel builds its text on each // View, so those pick the new names up on the next frame with nothing to do here. + // + // Two inputs name a session now, not one: this map and the served title on + // m.sessions (see sessionTitleFor). Only the harvest needs a rebuild triggered + // here, because m.sessions is replaced by the two-second list refresh, which + // rebuilds the table on its own path. So the dependency set is wider than this + // call site suggests — a future input that names sessions and does NOT already + // rebuild needs its own repaint. m.rebuildSessionsTable() } return m, nil diff --git a/cmd/abctl/tui/session_metadata.go b/cmd/abctl/tui/session_metadata.go index c27013379..bb4c8e27a 100644 --- a/cmd/abctl/tui/session_metadata.go +++ b/cmd/abctl/tui/session_metadata.go @@ -36,8 +36,14 @@ func SessionMetadataPath() (string, error) { return claude.SessionMetadataPath() // It never returns an error, for the reason loadUserConfig does not: this file is a // convenience cache another command produces, and a missing or corrupt one must not // keep the viewer from opening — the viewer being the tool you reach for when -// everything else is broken. Every failure yields an empty map, which renders as an -// empty TITLE column: the pane still works, it just cannot name anything. +// everything else is broken. Every failure yields an empty map: the pane still works, it +// just cannot name anything FROM HERE. +// +// AND THE COLUMN NO LONGER GOES BLANK WITH IT. This used to say a failure "renders as an +// empty TITLE column", which stopped being true when sessionTitleFor gained the fallback +// to the title /v1/sessions serves: a session the proxy has named still renders one +// through a total load failure. Degrading to none is therefore less visible than it was, +// which is the right direction and worth stating so the silence stays justified. // // Absent is not a failure at all. Nobody has this file until they run // `abctl experimental read-claude-sessions`, so a first run must be silent rather than @@ -133,15 +139,39 @@ func loadSessionMetadataForModel() map[string]SessionMetadata { // unique — two sessions in the same directory get the same harvested title, so a title // alone would make them indistinguishable in a header. func (m *model) sessionLabel(id string) string { - // THROUGH titleIsBlank, like the other two consumers of "is this named". A raw != "" accepted + // THROUGH titleIsBlank, like every other consumer of "is this named". A raw != "" accepted // a whitespace-only title and rendered " (id)" — a header padded by a title that shows // nothing, which is worse than the bare id it would otherwise print. - if title := m.sessionTitle(id); !titleIsBlank(title) { + // + // sessionTitleFor, not sessionTitle, so a header names a session on whichever source can — the + // same precedence the TITLE column applies. A row an operator selected BY its served title must + // not lose it on Enter; that inconsistency is exactly what this helper's doc above rules out. + // blankSanitized, not titleIsBlank: sessionTitleFor returns a sanitised string on every path, so + // re-sanitising it here would allocate a second copy of it to reach the same answer. + if title := m.sessionTitleFor(id, m.servedTitle(id)); !blankSanitized(title) { return title + " (" + id + ")" } return id } +// servedTitle returns the title /v1/sessions published for this session, or "" if it listed none. +// +// A LINEAR WALK, deliberately, as several others in this package already are: m.sessions is one +// pod's live sessions — a handful in practice — and the three headers this feeds each render ONE +// selected session per frame. An id-keyed map would be a second structure to keep in step with the +// slice, and the slice is rebuilt wholesale on every poll, so the sync is the cost, not the lookup. +// +// "" for a session the server does not list is the honest answer and the one sessionTitleFor wants: +// it means nothing served a title, which is indistinguishable here from serving an empty one. +func (m *model) servedTitle(id string) string { + for _, s := range m.sessions { + if s.ID == id { + return s.Title + } + } + return "" +} + // HarvestFunc reads an agent's transcripts and returns what it learned, keyed by session id. // // A function on RunOptions rather than a direct call into core/observe/claude, so this @@ -202,8 +232,18 @@ func harvestCmd(h HarvestFunc) tea.Cmd { // this cheap: without it every event on a still-unnamed session would trigger a scan, and // with it a busy session is harvested once, after it pauses. // -// Only sessions the metadata does NOT name are considered, so the steady state — every row titled -// — triggers nothing at all. +// Only sessions the metadata does NOT name are considered, so a fully-harvested list triggers +// nothing at all. +// +// "FULLY HARVESTED" IS NOT "EVERY ROW NAMED ON SCREEN", and this comment used to conflate the two. +// sessionHasTitle asks about the HARVEST only — sessionTitleFor's served-title fallback is +// deliberately invisible to it (see sessionTitle's doc for why) — so a row showing a proxy-derived +// title still counts as untitled here and keeps triggering settled harvests. For an agent with no +// Claude Code transcript tree on this disk those harvests can never succeed, so that is not a +// transient state on the way to quiet: it is the permanent one, and the allocation noted below is +// paid for as long as the pane is open. Bounded by untitledBackoffCap (~3m between attempts) rather +// than by ever being satisfied. That periodic scan is the accepted price of not letting a served +// title stop the search for the richer harvested one; it is not a leak, but it is not free either. // // THE QUIET TEST SPANS TWO CLOCKS and tolerates them disagreeing in the safe direction; the // reasoning is at the comparison itself. @@ -239,9 +279,10 @@ func (m *model) untitledSettled(now time.Time) bool { // way this gets large. // // A FUTURE UpdatedAt IS A BROKEN CLOCK, NOT A SETTLED SESSION. Pod ahead of client gives - // a negative delta, which can never reach untitledSettleDelay, so that row's title never - // arrives — no error, no log, just a permanently blank TITLE cell, and the skew has to - // exceed only 5s to do it. Treating it as settled instead is the safe direction: the + // a negative delta, which can never reach untitledSettleDelay, so the harvested title for + // that row never arrives — no error, no log, and the skew has to exceed only 5s to do it. + // The row is left on whatever the proxy served, or blank if it served nothing; either way + // it is stuck there. Treating it as settled instead is the safe direction: the // cost of harvesting early is one wasted tree walk that the backoff then widens, against // a title that otherwise never comes at all. // @@ -269,9 +310,12 @@ func (m *model) untitledSettled(now time.Time) bool { // it had no part in earning, and the reset only took effect afterwards — for the next new row. // Reading the set here means the arrival is priced on the tick it arrives. // -// COSTS ONE MAP LOOKUP PER UNNAMED ROW PER TICK, and only while the sessions pane is open. The -// steady state — every row titled — exits on sessionHasTitle without touching the set at all, -// and the loop is over one pod's live sessions. It is the same walk untitledSettled does and the +// COSTS ONE MAP LOOKUP PER UNNAMED ROW PER TICK, and only while the sessions pane is open. A +// harvest-named row exits on sessionHasTitle without touching the set at all, and the loop is over +// one pod's live sessions. Note that "unnamed" here means UNHARVESTED, not blank on screen: a row +// wearing a served title from sessionTitleFor reaches the lookup, and on an agent whose transcripts +// this machine does not have it reaches it on every tick indefinitely — see untitledSettled, +// where the same asymmetry is spelled out. It is the same walk untitledSettled does and the // same walk the scoring does; see countUntitled, which the scoring shares with this. // // DOES NOT MUTATE THE SET. The gate asks a question; the harvest's scoring is what records the @@ -313,12 +357,17 @@ func (m *model) countUntitled() (counted map[string]bool, fresh bool) { return counted, fresh } -// sessionHasTitle reports whether this session renders a title, as the TITLE cell would judge it. +// sessionHasTitle reports whether the HARVEST has named this session. +// +// NOT "does the row render a title" — it deliberately says less than that. A row the harvest has +// not named can still display the title the proxy served (see sessionTitleFor), and this predicate +// answers false for it on purpose, so the harvest keeps looking for the title it would prefer. +// Every backoff predicate in this file is built on that distinction; do not widen this to mean +// "something is on screen". // -// THROUGH sessionTitle, not the raw map, so this predicate and the cell can never disagree about -// what "unnamed" means: the cell sanitises (sessionTitle does), and a predicate reading -// m.sessionsData[id].Title directly would be asserting about a different string than the one on -// screen. sanitizeLabel replaces rather than strips, so it cannot change emptiness today — the +// THROUGH sessionTitle, not the raw map, so this asks about the same sanitised string the harvest +// path renders: a predicate reading m.sessionsData[id].Title directly would judge a different +// string. sanitizeLabel replaces rather than strips, so it cannot change emptiness today — the // point is that this does not depend on that remaining true. // // WHITESPACE COUNTS AS UNNAMED, which the raw comparison got wrong. A title of " " is non-empty @@ -327,27 +376,28 @@ func (m *model) countUntitled() (counted map[string]bool, fresh bool) { // value, so this is defence at the consumer rather than a live upstream bug — but this file // renders whatever is in that map, including what an older harvester or a hand-edited file left. func (m *model) sessionHasTitle(id string) bool { - return !titleIsBlank(m.sessionTitle(id)) + // blankSanitized: sessionTitle sanitises, so titleIsBlank would do it again on a string already + // through it — per row per tick on the render path. + return !blankSanitized(m.sessionTitle(id)) } // titleIsBlank reports whether a title string would render as an empty TITLE cell. // -// THE ONE DEFINITION OF "UNNAMED" AMONG THE PREDICATES, extracted because three callers ask that -// question about different strings — sessionHasTitle about what the model already holds, -// sessionLabel about the same for a header, and harvestNamedSomething about what a harvest just -// returned, which is not in the model yet and so cannot be reached through sessionTitle. An -// inline copy in any of them is the drift sessionHasTitle's comment exists to prevent. -// -// THE CELL DOES NOT CALL THIS, and the claim that it does was overstated. sessionTitleCell tests -// a raw title == "" as a fast path to skip truncating an empty string; it does not judge -// blankness, and a " " title falls through it and is returned as " ". So the two AGREE in -// behaviour on every input — verified across "", " ", " ", "\t" and ordinary prose — but by -// construction rather than by sharing this function. If that fast path ever becomes a real -// blankness test, it should route through here. -// -// SANITISES BEFORE TRIMMING, in that order, because that is the order the cell applies them: it -// renders sessionTitle, which is sanitizeLabel'd, and nothing trims afterwards. sanitizeLabel -// REPLACES control and BIDI runes with U+FFFD rather than stripping them, so a title of "\t" or +// THE ONE DEFINITION OF "UNNAMED", extracted because its callers ask that question about strings +// reached different ways — what the model already holds, what a harvest just returned and is not in +// the model yet, what the proxy served — and an inline copy in any of them is the drift +// sessionHasTitle's comment exists to prevent. Deliberately not enumerated here: the list went +// stale the first time a caller was added, and the callers are one grep away. +// +// THE CELL REACHES THIS NOW, through sessionTitleFor, which is where the fallback decides whether +// the harvested title is worth keeping. So a harvested " " no longer survives to the screen: it +// answers blank here and the cell shows the served title instead, or "" when there is none. What +// remains NOT a blankness test is sessionTitleCell's own `title == ""` fast path, which only skips +// truncating an empty string and is reached after this predicate has already had its say. +// +// SANITISES BEFORE TRIMMING, in that order, because that is the order the display applies them: it +// renders sessionTitleFor, which is sanitizeLabel'd on both paths, and nothing trims afterwards. +// sanitizeLabel REPLACES control and BIDI runes with U+FFFD rather than stripping them, so a "\t" or // "\n" is NOT blank here — the cell shows "�", a visible glyph, and a predicate calling that row // unnamed would re-harvest forever for a row that is already displaying something. // @@ -361,8 +411,30 @@ func (m *model) sessionHasTitle(id string) bool { // Sanitising a string sessionTitle already sanitised is a no-op, not a second pass with different // meaning: sanitizeLabel is idempotent — U+FFFD matches none of its cases and falls through — so // the caller does not have to know which of the two paths got there first. +// +// IDEMPOTENT IS NOT FREE, THOUGH, which is why blankSanitized exists beside this. sanitizeLabel +// builds a new string with b.Grow(len(s)) on the UNTRUNCATED input, so a caller already holding one +// and calling this anyway allocates a second full copy to reach the same answer. On the render path +// that is per row per rebuild, at whatever length the producer sent. Callers holding a sanitised +// string should say so rather than pay for the round trip. func titleIsBlank(title string) bool { - return strings.TrimSpace(sanitizeLabel(title)) == "" + return blankSanitized(sanitizeLabel(title)) +} + +// blankSanitized is titleIsBlank for a string that sanitizeLabel has ALREADY been applied to. +// +// The trim half of the predicate, split out so the sanitise half is not paid twice. Every caller of +// titleIsBlank that passes the output of sessionTitle or sessionTitleFor is in that position, and +// so is sessionTitleFor itself, once it holds the sanitised served string it is about to return. +// +// WHY THE SPLIT IS SAFE HERE AND NOT A GENERAL LICENCE: the two halves are not interchangeable. +// titleIsBlank's doc above spends a paragraph on why the ORDER matters — sanitise before trim, so +// a "\t" answers not-blank because the cell paints a glyph for it. This helper is the second half +// only, so handing it a RAW title reintroduces the exact disagreement-with-the-screen that the +// ordering prevents: a raw "\t" would trim to "" and read blank, and the row would re-harvest +// forever while displaying a glyph. Call it only where the sanitising demonstrably already ran. +func blankSanitized(sanitized string) bool { + return strings.TrimSpace(sanitized) == "" } // harvestNamedSomething reports whether a finished harvest named a session that is ON SCREEN and diff --git a/cmd/abctl/tui/sessions_pane.go b/cmd/abctl/tui/sessions_pane.go index ce8a9b725..dcd2e7152 100644 --- a/cmd/abctl/tui/sessions_pane.go +++ b/cmd/abctl/tui/sessions_pane.go @@ -7,10 +7,12 @@ import ( "strings" "time" "unicode" + "unicode/utf8" "github.com/charmbracelet/bubbles/table" "github.com/charmbracelet/lipgloss" + "github.com/rossoctl/cortex/core/observe/claude" "github.com/rossoctl/cortex/core/pipeline" ) @@ -257,7 +259,12 @@ func (m *model) rebuildSessionsTable() { trunc(s.ID, idW), } if showTitle { - row = append(row, m.sessionTitleCell(s.ID, titleW)) + // s.Title straight off the summary, where sessionLabel reaches the same value by + // id through m.servedTitle. Two routes, equal only because they read the same + // slice — probed across many inputs without finding a divergence. Do not "unify" + // one into the other: this loop has the summary in hand and should not pay a + // lookup, and the header path has only an id and cannot avoid one. + row = append(row, m.sessionTitleCell(s.ID, s.Title, titleW)) } row = append(row, relTime(now, s.UpdatedAt), @@ -297,7 +304,11 @@ func (m *model) rebuildSessionsTable() { trunc(id, idW), } if showTitle { - row = append(row, m.sessionTitleCell(id, titleW)) + // These rows exist precisely because the server no longer lists the session, so + // there is no summary to carry a served title. Harvested metadata outlives the + // listing, so the cell can still fill from that. Named constant rather than a bare + // "" so the absence reads as a fact about this row, not a forgotten argument. + row = append(row, m.sessionTitleCell(id, noServedTitle, titleW)) } row = append(row, // "cached" sits in UPDATED now, where an em dash used to, because ACTIVE is gone @@ -381,6 +392,11 @@ func (m *model) cachedOnlySessionIDs() []string { // SessionMetadata, so a session nobody harvested, an id not in the file, and the file being // absent altogether all render the same empty cell. That is the right answer for all three — // none of them is a fact about the session, only about whether anyone has run the harvester. +// +// HARVEST-ONLY, AND THAT IS LOAD-BEARING — do not fold the server's /v1/sessions title in here. +// sessionHasTitle reads this, and through it every backoff predicate in session_metadata.go, so a +// server title arriving would make the row read as named and STOP the re-harvest for good. The +// fallback lives in sessionTitleFor instead, which only display paths call. See its doc. func (m *model) sessionTitle(id string) string { // SANITISED AT THE ACCESSOR, so every consumer is covered by one line. The title is // LLM-generated transcript text — sessions_metadata.go says so — and it reaches a table @@ -391,10 +407,405 @@ func (m *model) sessionTitle(id string) string { // sanitizeLabel is the package's existing answer for exactly this, introduced with a // CWE-150 citation. Severity is bounded — the file lives under the operator's own home // directory — which is why this is a one-line routing rather than a redesign. - return sanitizeLabel(m.sessionsData[id].Title) + // + // AND CAPPED HERE TOO, for the reason sessionTitleFor caps the served title: the harvester + // emits at most claude.MaxTitleLen runes, but NOTHING RE-CHECKS THAT ON LOAD. + // LoadSessionMetadata parses the JSON and applies no cap, so a hand-edited or rewritten + // ~/.cortex/session-metadata.json reaches the quadratic path in truncLeft/truncRight exactly + // as an uncapped served title would. Measured before this line existed: a 10003-rune + // path-shaped title with combining marks made ONE rebuildSessionsTable take 1.11s. + // + // Pre-existing rather than introduced by the fallback — but capping only the served side left + // the two sources asymmetric for no reason, and the fix is the same constant. Re-capping an + // already-capped harvested title costs a length check. + // + // TRIMMED BEFORE THE CUT, and that is a CORRECTNESS requirement rather than tidiness — but note + // the history, because this comment asserted the OPPOSITE order for the same reason and was + // wrong twice over. + // + // The cap was first written claiming "truncation cannot turn a non-blank title blank". False. It + // was then rewritten to trim AFTER the cut, with a comment claiming that trimming cannot + // introduce the failure it prevents "because it only ever removes whitespace". Also false, and + // the measurements are the refutation: a title whose first MaxTitleLen runes are whitespace with + // real text after them loses the text to the CUT, and is then emptied by the TRIM. Measured at + // MaxTitleLen = 80: 40 leading spaces keeps 50 runes, 79 keeps just "m", and 80 or more returns + // "" — for U+0020, U+00A0 and U+3000 alike. sessionHasTitle reads sessionTitle, so that flips a + // named row to unnamed and re-harvests ~/.claude every ~3 minutes for the life of the process: + // the permanent-rescan cost this file documents as the price of an UNNAMABLE session, charged + // instead to a session with a perfectly good name. + // + // Trimming first dissolves the problem rather than guarding against it. Leading whitespace is not + // part of the name, so it should never have consumed the rune budget; once it does not, no amount + // of it can push real text past the cut. capTitleRunes trims both ends of its input before + // measuring, and trims again after cutting to drop whitespace the cut newly exposed. + // + // clipTitle (core/observe/claude/harvest.go) trims after its own cut and is safe doing so for a + // reason that does not transfer: normalizeTitle has already collapsed every whitespace run ahead + // of it, so it never sees a leading run long enough to matter. Neither string here has been + // through that — which is the same precondition the cluster walk below exists because this side + // lacks. + return capTitleRunes(sanitizeLabel(m.sessionsData[id].Title)) } -// sessionTitleCell is sessionTitle fitted to the TITLE column, truncated from the LEFT. +// capTitleRunes cuts s to at most claude.MaxTitleLen runes, on a grapheme-cluster boundary, and +// trims the result. +// +// ONE HELPER BECAUSE THERE ARE TWO SOURCES. sessionTitle caps the harvested title and +// sessionTitleFor caps the served one; both need the same bound, and the two open-coded copies that +// preceded this differed only in which string they read. The cost of the cap is documented at both +// call sites — briefly, truncLeft/truncRight bound their OUTPUT but search quadratically over their +// INPUT whenever a zero-width rune disables the fast path, so an uncapped title is seconds per row +// per rebuild on the UI goroutine. +// +// utf8.RuneCountInString RATHER THAN len([]rune(s)), because the common case is a title already +// under the cap and that case must allocate nothing. The rune slice was 160 B/op and 232ns for a +// 40-rune title against 0 B/op and 155ns here, on a path reached ~3-4x per row per 2s tick. The +// slice is still built when a cut is actually needed, where its cost is the point rather than +// overhead. Note the trade: counting first walks the string twice, so on the over-long path this is +// ~45% slower than slicing immediately (780µs vs 1.13ms at 200k runes). That is the right way round +// — the short path is the one that runs constantly, and the long path is a cut this is preventing +// the expensive consequences of, not an operation to optimise. +// +// THE CUT LANDS ON A CLUSTER BOUNDARY, and that is not a nicety. clipTitle upstream says a plain +// rune cut is safe for it ONLY BECAUSE normalizeTitle has already removed every character that +// binds to its neighbour — combining marks, modifiers, joiners, regional indicators. NEITHER string +// here has been through that: the harvested title is re-read from a file that may have been +// rewritten, and the served title comes from core/session's sanitizeTitle, whose own doc says it +// KEEPS combining marks "so café survives". So a blind cut at MaxTitleLen can sever a cluster, +// leaving a dangling accent bound to whatever precedes it, half an emoji ZWJ sequence, or one +// regional indicator of a flag — a title that renders as something nobody wrote. Walking back off +// the binders costs a few rune tests on the only path that ever cuts. +// +// normalizeTitle is unexported and staying that way, so this is a boundary walk rather than a reuse. +// It is deliberately narrower than normalising: the goal is only that the cut not land mid-cluster, +// not that clusters be removed. +func capTitleRunes(s string) string { + // TRIMMED FIRST, so surrounding whitespace never consumes the rune budget. See sessionTitle's + // note: trimming after the cut let 80 leading spaces empty a genuinely-named title, because the + // cut kept only the spaces and the trim then removed them. Whitespace is not part of a name, so + // the fix is for it not to count rather than to detect the damage afterwards. + // + // This also makes the early return below exact. Trimming after it would mean a string of 80 + // real runes plus one trailing space took the cut path to produce a result the early return + // could have returned untouched. + s = strings.TrimSpace(s) + if utf8.RuneCountInString(s) <= claude.MaxTitleLen { + return s + } + r := []rune(s) + cut := claude.MaxTitleLen + // Walk back off a cut that would orphan r[cut] from what precedes it. + // + // ONE CLUSTER IS THE WHOLE BUDGET, and saying so is the entire reason this is a loop with a + // floor rather than a while-it-binds walk. Two earlier versions each walked until the rune at + // the cut stopped binding, and each emptied a non-blank title on a long enough run: + // + // - Regional indicators pair into flags, so in a run only every second one binds. Treating + // all of them as binders walked to index 0: 41 consecutive flags capped to "". + // - Combining marks STACK — "a mark cannot follow a mark" is false, and this comment used to + // assert it as the loop's bound. "a" + 100 U+0301 capped to "", and "a"*60 + 40 marks lost + // 21 runes against a documented bound of one step. + // + // Both are the same bug reached by different routes, and the shared root cause is that the + // binding predicate answers "is this rune part of a cluster?" while the loop needed "where does + // this cluster START?". An unbounded walk answers the first question repeatedly and can consume + // the whole title; a degenerate cluster is not a reason to return nothing. + // + // Emptying the title is the failure that matters, not the shortening: sessionHasTitle reads + // sessionTitle, so a named row that caps to "" reads as unnamed and restarts the permanent + // ~3-minute re-harvest. That is the same failure f5a0a615 fixed for whitespace, which is why + // this walk now has a floor that cannot reach 0 while any non-binder precedes the cut. + // + // So: find the start of the cluster the cut lands inside, then take all of it or none of it. + // clusterStart stops at the first non-binder, so it reads at most the ONE cluster straddling the + // cut — and the ONLY way to lose the whole title is a string that is a single degenerate cluster + // from index 0, a title with no base character at all, which no longer costs anything because + // the fallback below keeps MaxTitleLen runes of it rather than returning "". + // + // "One cluster" is the right bound but NOT a small number, and it is worth being exact because a + // reviewer read the earlier wording as promising one step. A degenerate cluster can be as long as + // the cut, so the scan is min(cluster length, MaxTitleLen) — 80 steps for "a" + 100 marks, and + // still 80 for "a" + 2000, which is the part that matters: it does not grow with the title. On + // every shape where ordinary text precedes the cut it is 1 step, because r[cut] is a base + // character and the loop returns immediately. + // A LEADING DEGENERATE CLUSTER IS THE ONE SHAPE WITH NO BOUNDARY TO CUT ON, and it is decided + // STRUCTURALLY — by clusterStart reporting 0 — rather than by noticing afterwards that the + // result came out blank. That distinction is the whole reason this reads the way it does. A + // blanket "if the cap emptied a non-blank title, cut bluntly instead" guard was written here + // first and rejected: it rescues the output of ANY walk, including the unbounded one this + // replaces, so every mutation test of the walk passed under it. A fallback that makes the + // broken and the fixed implementation indistinguishable is not a safety net, it is a mask. + // + // clusterStart == 0 means r[0] itself binds to what precedes it, i.e. the title opens with + // marks or an odd flag half and there is no earlier boundary in the string. Cutting bluntly at + // MaxTitleLen splits that cluster, which is the lesser evil by a wide margin: a dangling accent + // renders as one odd glyph, whereas "" flips sessionHasTitle to unnamed and restarts the + // permanent ~3-minute re-harvest. + if start := clusterStart(r, cut); start > 0 { + cut = start + } + return strings.TrimSpace(string(r[:cut])) +} + +// clusterStart returns the index of the first rune of the grapheme cluster that r[cut] belongs to, +// or cut itself when r[cut] starts its own cluster and the cut is already on a boundary. +// +// BOUNDED BY THE ONE CLUSTER STRADDLING THE CUT, not by the title. The scan stops at the first rune +// that does not bind to what precedes it. That bound is the fix for two defects that shipped in this +// file: capTitleRunes' doc above has the measurements. +// +// NOT bounded by a constant, and the difference has been misread: a degenerate cluster can run the +// whole way back, so the worst case is min(cluster length, cut) steps — 80 for "a" + 100 combining +// marks, and still 80 for "a" + 2000, which is the property that matters. Where ordinary text +// precedes the cut it returns on the first iteration. +// +// cut MAY EQUAL len(r), meaning "the cut is past the last rune". r[cut] does not exist there, so +// there is nothing to orphan and the answer is cut itself. Handled explicitly rather than left to +// the caller: capTitleRunes only ever passes a cut strictly inside r (its early return guarantees +// len(r) > MaxTitleLen), so this arm is unreachable from the one live caller today — but the +// alternative was an index-out-of-range panic on a plausible direct call, load-bearing on a coupling +// two functions apart that nothing stated and no test pinned. bindsToPrevious' doc invites other +// callers in this package; this makes the invitation safe. A cut ABOVE len(r) is a caller bug and +// still panics, deliberately: clamping it would invent an answer for a question the caller got wrong. +// +// Regional indicators are resolved by parity rather than by the per-rune predicate, because their +// binding depends on POSITION — only the second of a pair binds. riBindsAtCut counts the preceding +// run, so an even run means r[i] opens a fresh pair and i is already a boundary. +func clusterStart(r []rune, cut int) int { + if cut >= len(r) { + return cut + } + for i := cut; i > 0; i-- { + if r[i] >= 0x1F1E6 && r[i] <= 0x1F1FF { + if !riBindsAtCut(r, i) { + return i + } + } else if !bindsToPrevious(r[i]) { + return i + } + } + return 0 +} + +// riBindsAtCut reports whether the regional indicator at r[cut] is the SECOND half of a flag, so +// cutting before it would leave a bare letter where a flag was. +// +// Regional indicators are the one binder whose binding depends on POSITION rather than on the rune: +// they pair left to right, so in "🇺🇸🇬🇧" the first and third bind to nothing while the second and +// fourth complete a flag. Counting the unbroken run of them that precedes the cut gives the parity — +// an even-length run means r[cut] starts a fresh pair and the cut is already on a boundary. +// +// The scan is bounded by the run, not by the title: it stops at the first non-RI rune. A title that +// is nothing but flags is the worst case, and it is exactly the case a global walk got wrong. +func riBindsAtCut(r []rune, cut int) bool { + run := 0 + for i := cut - 1; i >= 0 && r[i] >= 0x1F1E6 && r[i] <= 0x1F1FF; i-- { + run++ + } + return run%2 == 1 +} + +// bindsToPrevious reports that r renders as part of the cluster started by the rune before it, so a +// cut immediately before r would split that cluster. +// +// Mn/Me/Mc ARE THE MARK CATEGORIES, and together they approximate Unicode's Grapheme_Extend: Mn +// non-spacing (a combining accent, a variation selector), Me enclosing, Mc spacing-combining (a +// Devanagari vowel sign, which occupies a column but still belongs to the letter before it). Two +// more bind without any category saying so: U+200D ZWJ is what joins the codepoints of a +// multi-part emoji, and Regional_Indicator runes pair up into flags, so a cut between two leaves a +// bare letter where a flag was. +// +// REGIONAL INDICATORS ANSWER TRUE HERE BUT ARE NOT DECIDED HERE. Their binding depends on position, +// not on the rune — only the second of a pair binds — and this function sees one rune with no +// context. capTitleRunes routes them through riBindsAtCut instead; this arm remains so the +// predicate's answer to "can this rune ever bind?" stays honest for any other caller. +// +// THE ZWJ ARM IS UNREACHABLE FROM BOTH PRODUCTION CALLERS TODAY, and deliberately kept. Since +// sanitizeLabel began delegating to pipeline.IsControlRune, a ZWJ is replaced by U+FFFD before either +// cap site sees it, and U+FFFD binds to nothing. So the hazard the paragraph above describes cannot +// currently occur on either path. It is kept because the unreachability is incidental — it depends on +// a sanitiser in another file continuing to treat ZWJ as a control rune, which is a rule about +// terminal safety and not about grapheme clusters. A future caller that caps an unsanitised string, +// or a narrowing of IsControlRune, restores the hazard silently. A dead rune test is cheaper than +// that coupling. +// +// DELIBERATELY NOT Lm. Modifier LETTERS (U+02B0 ʰ and the like) read as though they belong here and +// Grapheme_Extend excludes them — and the category also holds runes that legitimately START a +// cluster, U+02BB ʻokina being a letter in Hawaiian orthography. Including Lm would walk the cut +// back off an ordinary word character. Sk (modifier SYMBOLS, U+02C7 ˇ) is out for the same reason. +// A test fixture built on U+02B0 is what surfaced this; it was the fixture that was wrong. +// +// A SMALL EXPLICIT SET rather than a grapheme-segmentation library: abctl has no such dependency, +// this is one cut on one display path, and being slightly conservative only moves the cut earlier by +// a rune or two. The failure it prevents is a severed cluster; the cost of over-walking is a shorter +// title. +func bindsToPrevious(r rune) bool { + if unicode.In(r, unicode.Mn, unicode.Me, unicode.Mc) { + return true + } + return r == '‍' || (r >= 0x1F1E6 && r <= 0x1F1FF) +} + +// noServedTitle is the served-title argument for a row that cannot have one. Only the cached-only +// rows qualify, and only because the server has stopped listing those sessions entirely. +// +// It exists because sessionTitleFor takes the served title as a parameter, so passing "" silently +// disables the fallback and compiles. The parameter stays — the live row loop already holds the +// summary, and resolving it inside would put a scan of m.sessions in the per-row render path — so +// the one legitimate empty argument says so by name instead. +// +// "CANNOT HAVE ONE" IS ABOUT TODAY'S STRUCTURE, not a claim that nothing better is possible. A +// session named ONLY by the proxy keeps its name while listed and loses it the moment it becomes +// cached-only, so an operator watches a title vanish from a row whose events are deliberately +// preserved (the #870 scenario). Remembering the last-seen served title would fix that, and nothing +// here does: neither this constant nor cachedOnlySessionIDs retains anything from the summary that +// went away. Deferred rather than overlooked — it means holding title state across list refreshes, +// which is a store question and not a rendering one. +const noServedTitle = "" + +// sessionTitleFor names a session for DISPLAY, falling back to the title the proxy served. +// +// Two independent sources, and each covers what the other cannot. The harvest reads Claude Code's +// transcripts on this machine; /v1/sessions carries a title the proxy derived from the session's +// own events. So a session the harvester has no transcript for can still be named, and that is not +// a rare shape: on a laptop where every blank harvested entry was checked, all of them belonged to +// an agent with no Claude Code transcript tree — one that does route through the proxy, so a served +// title existed for exactly those. A blank cell was never "this session has no name", only "no name +// where abctl was looking". +// +// HARVEST WINS when both exist. Deliberately a fixed precedence and not a judgement about which +// string is better: both sides pick a title through their own ranking, both may change, and this +// says as little as possible about either mechanism so that it does not go stale when they do. +// What it costs is worth knowing — the two rankings do not agree on every session, so a row can +// show a harvested title while the proxy held one an operator would have preferred (an explicit +// /rename is the clearest case). That is accepted for now, pending what operators report; the +// precedence is one line to invert if it turns out wrong. +// +// THE FALLBACK IS ONLY HERE, not in sessionTitle. Every backoff predicate in session_metadata.go +// judges "unnamed" through sessionTitle, so this deliberately leaves a server-titled row reading as +// unnamed to them: the harvest keeps looking for the harvested title, at the cost of a periodic +// transcript scan. That cost is the trade, not a leak — but it is a PERMANENT steady state on +// exactly the rows this fallback serves, not a transient one. An agent with no Claude Code +// transcript tree is the case that motivated the feature, and for it the harvest can never +// succeed, so the backoff pins at untitledBackoffCap and re-walks ~/.claude every 3 minutes for +// the life of the process (~0.7s for a first full scan, per the README). Accepted because the +// alternative is the re-harvest stopping on a served title and never picking up a transcript that +// appears later; worth revisiting if that walk ever becomes expensive enough to matter. +// +// THROUGH titleIsBlank rather than == "", because a harvested " " renders as nothing and filling +// nothing is the whole point. titleIsBlank's own doc anticipates this seam. +// +// served IS SANITISED TOO, and not on the assumption that it arrives clean. The proxy does trim and +// cap it, but /v1/sessions is unauthenticated and the title is folded from caller-supplied event +// content — sessionTitle's CWE-150 reasoning applies to this string at least as much as to the +// harvested one. +// +// THAT INDEPENDENCE IS NOW ACTUALLY TRUE, and it was not when the fallback first landed. +// sanitizeLabel then replaced the BIDI overrides and isolates but not the BIDI MARKS (U+200E/200F, +// U+061C) or the zero-widths (U+200B/200C/200D/2060/FEFF), all of which pipeline.IsControlRune names +// and core/session's sanitizeTitle does strip. So the only thing keeping a mark out of the cell was +// the producer this comment claimed not to rely on — a claim the code contradicted, for exactly the +// class of rune whose whole purpose is to make the rendered order differ from the byte order. +// sanitizeLabel delegates to IsControlRune now; see its doc. +// +// AN EMPTY served DOES NOT ALWAYS MEAN "the proxy derived none". A session that arrives on the +// event stream before it appears in a list refresh gets a stub SessionSummary with a zero Title +// (app.go's streamed-event path), so its row shows no served title until the next poll fills the +// summary in — under two seconds, and it self-corrects with no help from here. Worth knowing only +// because it makes a blank cell briefly ambiguous; nothing downstream needs to tell the two apart. +// served IS CAPPED HERE, and this is the only thing that bounds it. The harvested title arrives +// already capped at claude.MaxTitleLen runes, and the existing cross-module cap test asserts that +// against a harvested fixture — a path the served title never takes. The proxy does cap at its own +// maxTitleLen, but that is 80 in ANOTHER MODULE, unexported on purpose (its doc: "deliberately NOT +// that constant"), so nothing here can assert it and no client should assume it. /v1/sessions is +// unauthenticated and operator-pointed, so a title of any length is a thing abctl can be handed. +// +// What that costs without a cap is not a wide cell — truncLeft/truncRight bound the OUTPUT — it is +// the SEARCH inside them. Their fast path is disabled by any zero-width rune, and a served title +// keeps its combining marks, so a long one runs a quadratic scan: measured, 20003 runes takes 4.50s +// on ONE call, and 200003 did not finish in two minutes. That is the UI goroutine, once per row per +// rebuild. Capping the input is what REMOVES THE QUADRATIC TERM — not what makes the whole path +// flat, which an earlier version of this comment claimed and the code does not do. Two passes still +// run over the UNTRUNCATED string before the cap can apply: sanitizeLabel builds a new string with +// b.Grow(len(s)), and the cap's own rune count walks it. Both are linear, so a 200k-rune title still +// costs ~1.6MB of transient allocation and a couple of walks per row per rebuild. +// +// TWO IS THE FLOOR, AND IT WAS THREE. Review caught sessionTitleFor calling titleIsBlank(served) and +// then sanitizeLabel(served), which sanitised the untruncated string TWICE — ~3.2MB, not ~1.6MB, for +// the same answer, because sanitizeLabel is idempotent and the second copy was pure waste. It now +// sanitises into a local and blank-checks that (see blankSanitized). Anything that reintroduces a +// second sanitise, or a second []rune conversion, doubles the number in this paragraph; there is no +// benchmark that would notice, so read the call sites. Linear is the +// difference between a laggy column and an unusable one — the measured 4.50s at 20003 runes was the +// quadratic search, not these — but "flat" was wrong, and the honest bound is what a future reader +// needs when deciding whether to cap EARLIER, at the decode in apiclient, where it would be. +// +// AT THE HARVESTER'S CAP, reusing claude.MaxTitleLen rather than a new number: it is what the other +// source is already capped to, so the two titles get the same budget and the column keeps one rule. +// Runes, not columns, matching what that constant counts — the width re-measure downstream is what +// turns either into a fitted cell. +// +// THE TWO ARGUMENTS ARE UNCHECKED AGAINST EACH OTHER, and nothing here can detect a mismatch. served +// is meant to be THIS id's Title, but it is passed in rather than looked up, so pairing one session's +// id with another's title compiles and renders a confident wrong name — the worst failure shape this +// column has, because a title is what an operator uses to pick a row before acting on it. The two +// live callers are safe by construction (the row loop reads both from one SessionSummary; sessionLabel +// resolves served from the same id it passes), and that is the invariant a third caller must preserve: +// RESOLVE served FROM id, never from an index, a neighbouring row, or a previous frame's summary. +// servedTitle(id) exists for exactly that and is the right thing to reach for. The parameter stays +// because resolving inside would put a scan of m.sessions in the per-row render path — see +// noServedTitle — so this is a documented contract rather than an enforced one. +func (m *model) sessionTitleFor(id, served string) string { + // PRECEDENCE IS DECIDED ON THE RAW HARVESTED TITLE, not on what the cap left of it, and that + // separation is the fix for a real inversion. This read m.sessionTitle(id) — sanitised AND capped + // — and asked whether THAT was blank. So any route by which the cap could blank a non-blank + // harvested title also silently handed the row to the served one: sessionTitleFor(85 spaces + + // "real", "served-name") returned "served-name", showing the proxy's name to an operator who has + // been told in two docs and a core/session comment that HARVEST WINS. Worse than the missing + // title it replaced, because a wrong name is acted on and a blank one is not. + // + // The whitespace defect behind that specific case is fixed in capTitleRunes, so the two + // formulations now agree on every input known to differ. This one is still the right question to + // ask: "did the harvest name this session?" is about the harvest, and routing it through a + // length cap makes a display bound into a precedence rule. Whatever the cap does to a long + // title, it cannot move the row to the other source. + // + // sanitizeLabel is still applied — blankness must be judged on what RENDERS, which is + // titleIsBlank's whole reason for sanitising before trimming ("\t" paints a visible glyph, so it + // is not blank). Only the cap is out of the decision. + if raw := m.sessionsData[id].Title; !titleIsBlank(sanitizeLabel(raw)) { + return m.sessionTitle(id) + } + // SANITISE ONCE, INTO A LOCAL, then use it for both the blank check and the cap. + // + // SANITISE BEFORE CAPPING, matching sessionTitle, so the cluster walk inside the cap sees the + // string that will actually render. sanitizeLabel is rune-for-rune, so the two orders agree on + // WHERE the cut lands — but only one of them agrees on WHAT is at the cut: sanitising afterwards + // would walk back off a combining mark that sanitizeLabel then replaces with U+FFFD, a standalone + // glyph that never needed the walk. Ordering it this way keeps one rule for both sources. + // + // ONE PASS, NOT TWO: this used to call titleIsBlank(served) and then sanitizeLabel(served) on the + // next line. Both sanitise, and sanitizeLabel is O(len) with a b.Grow(len(s)) on the UNTRUNCATED + // input, so the pair built two full copies of a served title to reach one answer — doubling the + // transient allocation this function's own cost note bounds, per row per rebuild. Idempotent made + // it harmless but not free. + clean := sanitizeLabel(served) + // BLANK-CHECKED ON THIS SIDE TOO, because titleIsBlank was applied to the harvested title and + // not to this one — so a whitespace-only served title painted spaces into the cell while + // sessionLabel, which blank-checks what this returns, showed the bare id. The same session named + // two different ways by two callers of one accessor. Returning "" makes the cell agree with the + // header, and "" is what both already do when nothing names a session. + // + // blankSanitized rather than titleIsBlank because clean has already been through sanitizeLabel — + // and the ORDER that check documents is preserved, not dropped: sanitising happened above, and the + // trim happens here, which is the same sanitise-then-trim titleIsBlank performs internally. + if blankSanitized(clean) { + return "" + } + return capTitleRunes(clean) +} + +// sessionTitleCell is sessionTitleFor fitted to the TITLE column, truncated from the LEFT. // // Truncating from the left because the titles are mostly paths. bubbles truncates every cell // from the right, which on "/Users/person/src/cortex/.worktrees/claudesessions" keeps @@ -407,8 +818,8 @@ func (m *model) sessionTitle(id string) string { // why layout() must also rebuild these rows — see its call to rebuildSessionsTable. // // Left alone when it is not a path: a title is prose, and prose reads from the left. -func (m *model) sessionTitleCell(id string, titleW int) string { - title := m.sessionTitle(id) +func (m *model) sessionTitleCell(id, served string, titleW int) string { + title := m.sessionTitleFor(id, served) if title == "" { return title } @@ -545,6 +956,12 @@ func truncRight(s string, n int) string { // lower bound. This had the identical quadratic shape — 2500 runes 39ms, 20000 2.30s on one call // — and is reachable the same way, since looksLikePath needs a leading "/" so a RELATIVE cwd // routes down this branch while just as uncapped. + // + // AND THE GUARD'S SLOW PATH RUNS HERE TOO, for the reason truncLeft now spells out: a title + // served by /v1/sessions keeps its combining marks, so an accent or an emoji turns the skip off. + // Prose is the COMMON shape for a served title — it is folded from a user's own message — so this + // branch is the likelier of the two to meet one. Correctness is unaffected; the cost is bounded + // by sessionTitleFor's cap, not by anything here. r := []rune(s) if len(r) > n && zeroWidthFree(s) { r = r[:n] @@ -592,7 +1009,8 @@ func truncLeft(s string, n int) string { // AND IT IS NOT THE LAST MEASUREMENT THE CELL MEETS. bubbles v1.0.0 runs runewidth.Truncate over // every cell before styling, and runewidth does not skip ANSI. Measuring here in display columns // is therefore necessary but not sufficient: it holds only while the cell is PLAIN, which for a - // title is guaranteed upstream (core/observe/claude normalises every one) and asserted below. + // title is guaranteed upstream (core/observe/claude normalises a harvested one; sessionTitleFor + // runs sanitizeLabel over a served one) and asserted below. // Styling a title would put escape bytes inside that second budget and collapse a narrow cell to // a lone ellipsis. if lipgloss.Width(s) <= n { @@ -619,11 +1037,21 @@ func truncLeft(s string, n int) string { // different bytes. // // So skipping is conditional on the premise: zeroWidthFree reports whether s contains any - // zero-width rune, and only then is the prefix provably untestable. A title reaching this file - // never contains one — core/observe/claude drops every Mn/Me/Cf/Cc/Sk, asserted there across - // the whole Unicode range — so the fast path is what actually runs. The fallback exists because - // these are general helpers with callers that make no such promise, and a wrong answer is worse - // than a slow one. + // zero-width rune, and only then is the prefix provably untestable. + // + // THE SLOW PATH IS REACHABLE, and the comment here used to deny it. It read that a title never + // contains a zero-width rune because core/observe/claude drops every Mn/Me/Cf/Cc/Sk — true of a + // HARVESTED title, and no longer the only kind. A title served by /v1/sessions goes through + // core/session.sanitizeTitle instead, which deliberately KEEPS combining marks (its own doc: "so + // café survives"), and pipeline.IsControlRune covers C0/C1/DEL/BIDI/Cf but not Mn/Me/Sk. So an + // accented word or any ordinary emoji — U+FE0F VARIATION SELECTOR-16 is Mn — makes zeroWidthFree + // false and runs the quadratic search this comment claimed never executes. Measured on one call + // with a combining mark every other rune: 2503 runes 72ms, 5003 287ms, 10003 1.12s, 20003 4.50s, + // which is worse than the ASCII figures above because each measurement now folds a mark too. + // + // The guard is STRUCTURAL, so the answer stays correct either way — this is about the premise, + // not a bug. It is called out because the premise is what someone would delete the guard on, and + // because the cost is only bounded by sessionTitleFor capping its input; see the cap there. r := []rune(s) start := 0 if len(r) > n && zeroWidthFree(s) { diff --git a/cmd/abctl/tui/sessions_title_test.go b/cmd/abctl/tui/sessions_title_test.go index ef2461218..68ab3a887 100644 --- a/cmd/abctl/tui/sessions_title_test.go +++ b/cmd/abctl/tui/sessions_title_test.go @@ -6,9 +6,11 @@ import ( "math/rand" "os" "path/filepath" + "strconv" "strings" "testing" "time" + "unicode/utf8" "github.com/charmbracelet/bubbles/table" tea "github.com/charmbracelet/bubbletea" @@ -148,6 +150,21 @@ func newTitleModel(t *testing.T, meta map[string]SessionMetadata, ids ...string) } } +// newServedTitleModel is newTitleModel with titles on the SERVER summaries too. +// +// A sibling rather than a wider newTitleModel: the 50-odd existing callers are all about harvested +// titles, and threading an empty map through every one of them would say nothing. served is keyed +// by session id; an id absent from it lists with no title, which is what a proxy that derived none +// sends. +func newServedTitleModel(t *testing.T, meta map[string]SessionMetadata, served map[string]string, ids ...string) *model { + t.Helper() + m := newTitleModel(t, meta, ids...) + for i := range m.sessions { + m.sessions[i].Title = served[m.sessions[i].ID] + } + return m +} + // titleRow finds the row for one session id. Keyed on cell 0, which is the id by contract — // sessionsColumns' comment and selectedSessionID both depend on that. func titleRow(t *testing.T, m *model, id string) table.Row { @@ -211,7 +228,7 @@ func TestSessionsPane_ProseTitleTruncatesFromTheRight(t *testing.T) { // And at a width where it does NOT fit, the cut takes the tail and keeps the opening // words, which is the side this test is named for. titleW := sessionsColumnWidth(sessionsColumnsFor(90), "TITLE") - cut := m.sessionTitleCell("s1", titleW) + cut := m.sessionTitleCell("s1", noServedTitle, titleW) if lipgloss.Width(cut) > titleW { t.Errorf("TITLE is %d columns against a %d-column cell: %q", lipgloss.Width(cut), titleW, cut) } @@ -629,7 +646,7 @@ func TestSessionTitleCell_BoundsCJKProse(t *testing.T) { id: {Title: "日本語のセッションタイトルです日本語のセッション"}, }, id) for _, w := range []int{11, 14, 20} { - got := m.sessionTitleCell(id, w) + got := m.sessionTitleCell(id, noServedTitle, w) if cw := lipgloss.Width(got); cw > w { t.Errorf("sessionTitleCell(%d) is %d display columns: %q", w, cw, got) } @@ -769,7 +786,7 @@ func TestSessionTitleCell_NonPositiveWidthYieldsNothing(t *testing.T) { for _, id := range []string{prose, path} { for _, w := range []int{0, -1} { - if got := m.sessionTitleCell(id, w); got != "" { + if got := m.sessionTitleCell(id, noServedTitle, w); got != "" { t.Errorf("sessionTitleCell(%q, %d) = %q, want \"\"", id, w, got) } } @@ -838,7 +855,7 @@ func TestSessionTitleCell_CarriesNoANSI(t *testing.T) { // The WIDTH half, on the helper. This one is real here: sessionTitleCell does the truncation, // so a budget it fails to honour is its own bug. for _, w := range []int{11, 20, 40} { - got := m.sessionTitleCell(id, w) + got := m.sessionTitleCell(id, noServedTitle, w) if lipgloss.Width(got) > w { t.Errorf("title cell is %d columns against a %d-column budget: %q", lipgloss.Width(got), w, got) @@ -925,7 +942,7 @@ func TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures(t *testing.T) { const id = "s1" m := newTitleModel(t, map[string]SessionMetadata{id: {Title: tc.title}}, id) for _, w := range []int{11, 14, 20, 40} { - got := m.sessionTitleCell(id, w) + got := m.sessionTitleCell(id, noServedTitle, w) if cw := lipgloss.Width(got); cw > w { t.Errorf("%s: a %d-rune title rendered %d columns into a %d-column cell: %q — the "+ "harvester's cap is a RUNE count, so this package must re-truncate by width", @@ -982,7 +999,7 @@ func TestSessionTitleCell_SlashCommandIsNotAPath(t *testing.T) { t.Run(tc.name, func(t *testing.T) { m := newTitleModel(t, map[string]SessionMetadata{id: {Title: tc.title}}, id) const w = 20 - got := m.sessionTitleCell(id, w) + got := m.sessionTitleCell(id, noServedTitle, w) if lipgloss.Width(got) > w { t.Fatalf("cell is %d columns against a %d-column budget: %q", lipgloss.Width(got), w, got) } @@ -2168,3 +2185,1133 @@ func TestLoadSessionMetadata_AtTheReadCap(t *testing.T) { t.Errorf("loaded %d entries from a %d byte file, want the 1 real entry", len(got), len(under)) } } + +// A served title fills a cell the harvest could not name. +// +// The motivating case, and the reason the fallback exists at all: on the laptop this was found on, +// every blank harvested entry belonged to an agent with no Claude Code transcript tree to read — but +// one that routes through the proxy, so /v1/sessions had derived a title for exactly those sessions. +// A blank TITLE was never "this session has no name", only "no name where abctl was looking". +func TestSessionsPane_ServedTitleFillsAnUnharvestedCell(t *testing.T) { + m := newServedTitleModel(t, + map[string]SessionMetadata{}, + map[string]string{"s1": "Investigate the flaky reloader test"}, + "s1") + m.rebuildSessionsTable() + + if got := sessionsCell(t, m, titleRow(t, m, "s1"), "TITLE"); got != "Investigate the flaky reloader test" { + t.Errorf("TITLE = %q, want the served title", got) + } + // And through the header, which an operator reaches by pressing Enter on that same row. A row + // selected BY its served title must not lose it one keystroke later. + if got := m.sessionLabel("s1"); got != "Investigate the flaky reloader test (s1)" { + t.Errorf("sessionLabel = %q, want the served title and the id", got) + } +} + +// Each session gets ITS OWN served title, which needs two of them to say anything at all. +// +// servedTitle walks m.sessions comparing ids, and against a one-session fixture that walk is +// indistinguishable from returning the first entry unconditionally — `if s.ID == id` mutated to +// `if s.ID != ""` survived every other test in this package. The bug that hides there is not +// exotic: on any pod listing two sessions, session B's header would show A's title. So the +// fixture lists two, and the unlisted id pins the miss path that returns "". +func TestSessionMetadata_ServedTitleIsPerSession(t *testing.T) { + m := newServedTitleModel(t, map[string]SessionMetadata{}, + map[string]string{"s1": "first", "s2": "second"}, "s1", "s2") + + for id, want := range map[string]string{ + "s1": "first (s1)", + "s2": "second (s2)", + // Listed by nobody: servedTitle finds no match and the bare id is all a header can say. + "gone": "gone", + } { + if got := m.sessionLabel(id); got != want { + t.Errorf("sessionLabel(%q) = %q, want %q", id, got, want) + } + } +} + +// The ROW LOOP pairs each session with ITS OWN served title, which only two rows can show. +// +// The sibling above pins the same property for the HEADER, and that is why this one is separate +// rather than an extra assertion there: the two paths reach the served title differently. The header +// calls servedTitle(id), a lookup by id. The row loop never looks anything up — it renders from the +// summary it is already iterating and passes s.Title straight into sessionTitleCell. A lookup can be +// wrong about WHICH id it matched; a loop can be wrong about WHICH ITERATION it read from, and no +// test of the lookup can see that. +// +// Review found the gap by mutating the loop to pass m.sessions[0].Title instead of s.Title — every +// row rendering the FIRST session's title — and the whole package stayed green, because every +// fixture that put a served title in the table had exactly one row. On a pod listing two sessions +// that mutant labels B's row with A's name, which is indistinguishable from the bug this feature +// exists to fix except that it is confidently wrong rather than blank. +// +// Distinct titles in BOTH directions, so neither "always the first" nor "always the last" passes. +func TestSessionsPane_ServedTitleCellIsPerRow(t *testing.T) { + m := newServedTitleModel(t, map[string]SessionMetadata{}, + map[string]string{"s1": "first session", "s2": "second session"}, "s1", "s2") + m.rebuildSessionsTable() + + for id, want := range map[string]string{ + "s1": "first session", + "s2": "second session", + } { + if got := sessionsCell(t, m, titleRow(t, m, id), "TITLE"); got != want { + t.Errorf("TITLE for %s = %q, want %q", id, got, want) + } + } +} + +// capTitleRunes MUST NOT EMPTY A TITLE THAT RENDERS SOMETHING, whatever the cluster structure. +// +// This is the property, stated once, over every degenerate shape found so far. It matters because +// sessionHasTitle reads sessionTitle: a named row whose title caps to "" reads as UNNAMED, which +// zeroes untitledMisses and restarts the permanent ~3-minute re-harvest. f5a0a615 fixed that for +// whitespace; two more routes to the same failure have shipped since, both in the cap's walk: +// +// - 41 consecutive flags capped to "", because regional indicators pair and a walk that treated +// every one as a binder ran to index 0. +// - "a" + 100 combining marks capped to "", because the walk's documented bound — "a mark cannot +// follow a mark" — is false. Marks stack, so the run walked to the base and past it. +// +// Both were found by review rather than by a test, which is the argument for asserting the INVARIANT +// over a table of shapes instead of the arithmetic of any one of them. A new binder class added to +// bindsToPrevious gets this check for free; an expected-length assertion would not. +// +// A THIRD ROUTE WAS FOUND WITH THIS TEST ALREADY GREEN, and it is why the table now carries +// whitespace shapes. Every fixture here was binder-class, so none of them exercised the cap's TRIM \u2014 +// and the trim ran AFTER the cut, so a title whose first MaxTitleLen runes were whitespace lost its +// real text to the cut and was then emptied by the trim. 80 leading spaces plus "my-project" +// returned "". The property was right and the table could not reach the defect: the lesson is that +// an invariant test is only as broad as its shapes, so a fixture belongs here for every code path +// inside the function, not just for every bug that has been found in one of them. +func TestSessionsPane_CapNeverEmptiesANonBlankTitle(t *testing.T) { + const acute = "\u0301" // Mn, stacks without limit + flag := "\U0001F1FA\U0001F1F8" + // Over the cap on their own, so the cut fires before any trim can. + const pad = claude.MaxTitleLen + 5 + + for _, tc := range []struct{ name, in string }{ + // THE TRIM ROUTE. Whitespace is not binder-class, so none of the cluster fixtures below + // reach it: these cover the cut-then-trim interaction instead. Three whitespace classes, + // because TrimSpace is Unicode-aware and a fix that only counted U+0020 would pass on one. + {"long space prefix then text", strings.Repeat(" ", pad) + "my-project"}, + {"long NBSP prefix then text", strings.Repeat("\u00a0", pad) + "my-project"}, + {"long ideographic-space prefix then text", strings.Repeat("\u3000", pad) + "my-project"}, + // Exactly at the boundary, where the old code first returned "" rather than one rune. + {"space prefix exactly at the cap", strings.Repeat(" ", claude.MaxTitleLen) + "my-project"}, + // Interior whitespace must not be mistaken for the same thing: this one has real text + // inside the first MaxTitleLen runes and must keep some of it either way. + {"text then long space run then text", "a" + strings.Repeat(" ", pad) + "b"}, + // The two shipped defects, at the sizes they were measured at. + {"base plus a long mark run", "a" + strings.Repeat(acute, 100)}, + {"consecutive flags", strings.Repeat(flag, 41)}, + {"text then flags", strings.Repeat("a", 70) + strings.Repeat(flag, 10)}, + // No base character at all: every rune binds, so there is no boundary to cut on and the + // cluster-start walk correctly reports 0. The blunt cut is the deliberate fallback — a + // dangling mark renders oddly, "" restarts the re-harvest. + {"nothing but marks", strings.Repeat(acute, 100)}, + {"nothing but one flag half", strings.Repeat("\U0001F1FA", 100)}, + // Mixed, because a run of one binder class is not the only degenerate shape. + {"marks and flags interleaved", strings.Repeat(acute+flag, 40)}, + } { + got := capTitleRunes(tc.in) + if titleIsBlank(tc.in) { + t.Fatalf("%s: fixture is blank before capping, so it proves nothing", tc.name) + } + if titleIsBlank(got) { + t.Errorf("%s: capTitleRunes(%d runes) = %q, which is blank — this flips sessionHasTitle and restarts the re-harvest", + tc.name, utf8.RuneCountInString(tc.in), got) + } + if n := utf8.RuneCountInString(got); n > claude.MaxTitleLen { + t.Errorf("%s: capTitleRunes returned %d runes, over the %d cap", tc.name, n, claude.MaxTitleLen) + } + } +} + +// A WHITESPACE-PREFIXED HARVESTED TITLE MUST STILL READ AS NAMED, end to end. +// +// This is the reported symptom rather than the mechanism, and it is asserted through +// sessionHasTitle on purpose: the cap tests above check the string, and this one checks the +// CONSEQUENCE. sessionHasTitle gates all four backoff predicates (untitledSettled, untitledFresh, +// countUntitled, harvestNamedSomething), so a title emptied by the cap does not merely render blank +// — it zeroes untitledMisses and re-harvests ~/.claude every ~3 minutes for the life of the process, +// on a session the harvest has ALREADY successfully named. That is the cost this file documents as +// the price of an unnamable session, billed to a named one. +// +// The lengths bracket the old failure curve: at 40 leading spaces the old code kept 50 runes, at 79 +// it kept exactly "m", and at 80 and beyond it returned "". All of them must now read as named. +func TestSessionsPane_WhitespacePrefixedHarvestStillReadsAsNamed(t *testing.T) { + for _, ws := range []struct{ name, r string }{ + {"space", " "}, + {"no-break space", " "}, + {"ideographic space", "　"}, + } { + for _, n := range []int{40, claude.MaxTitleLen - 1, claude.MaxTitleLen, claude.MaxTitleLen + 5, 500} { + m := newTitleModel(t, map[string]SessionMetadata{ + "s1": {Title: strings.Repeat(ws.r, n) + "my-project"}, + }, "s1") + + if got := m.sessionTitle("s1"); titleIsBlank(got) { + t.Errorf("%s x%d: sessionTitle = %q, blank — the harvest named this session", ws.name, n, got) + } + if !m.sessionHasTitle("s1") { + t.Errorf("%s x%d: sessionHasTitle = false for a named session; this restarts the permanent re-harvest", ws.name, n) + } + // The name itself must survive, not just some non-blank remnant. The old code's + // 79-space case kept "m", which is non-blank and useless. + if got := m.sessionTitle("s1"); got != "my-project" { + t.Errorf("%s x%d: sessionTitle = %q, want the name with its padding trimmed", ws.name, n, got) + } + } + } +} + +// THE WALK IS BOUNDED BY ONE CLUSTER, not by the title — the claim the old comment got wrong. +// +// The superseded bound was "at most one step for marks and ZWJ, because a mark cannot follow a +// mark". Marks do follow marks, so the real bound has to come from somewhere else: clusterStart +// stops at the first rune that does not bind, so the work and the loss are both proportional to the +// ONE cluster straddling the cut. +// +// Growing that cluster from 5 marks to 2000 must not move the cut, and marks parked far past the cut +// must not move it either. An expected-length assertion is the point here rather than an invariant: +// the defect this replaces was measurable only as "how much did it lose", and 2000 marks losing the +// same 1 rune as 5 is what says the walk terminates at the base instead of running through it. +func TestSessionsPane_CapWalkIsBoundedByOneCluster(t *testing.T) { + const acute = "\u0301" + + // A cluster straddling the cut: 79 plain runes, then a base at 79 carrying every mark. The only + // boundary at or before the cut is 79, so that is where it must land, for any run length. + for _, marks := range []int{5, 40, 200, 2000} { + in := strings.Repeat("a", 79) + "e" + strings.Repeat(acute, marks) + "tail" + got := capTitleRunes(in) + if want := strings.Repeat("a", 79); got != want { + t.Errorf("straddling cluster with %d marks: got %d runes, want the 79 before the base", + marks, utf8.RuneCountInString(got)) + } + } + // Marks far PAST the cut are not the cut's business at all: index 80 sits inside a run of plain + // letters, which is already a boundary, so nothing should be walked back. + for _, marks := range []int{2, 200, 2000} { + in := strings.Repeat("a", 100) + strings.Repeat(acute, marks) + if got := utf8.RuneCountInString(capTitleRunes(in)); got != claude.MaxTitleLen { + t.Errorf("trailing %d marks: got %d runes, want the full %d — the cut is on a boundary already", + marks, got, claude.MaxTitleLen) + } + } + // THE WORST CASE IS THE CUT, NOT THE TITLE, and this is the case a reviewer read the doc as + // denying. A cluster degenerate from index 0 has no boundary anywhere, so the scan runs the whole + // way back — but that is MaxTitleLen steps and not len(title) steps, so growing the title 20x + // past the cut must not cost anything. Asserted through clusterStart directly, because + // capTitleRunes' blunt-cut fallback returns the same answer either way and would hide it. + for _, marks := range []int{100, 2000} { + r := []rune("a" + strings.Repeat(acute, marks)) + if got := clusterStart(r, claude.MaxTitleLen); got != 0 { + t.Errorf("degenerate cluster with %d marks: clusterStart = %d, want 0 — every rune binds", marks, got) + } + } +} + +// clusterStart MUST NOT PANIC WHEN THE CUT IS PAST THE LAST RUNE. +// +// clusterStart([]rune("abc"), 3) indexed r[3] and panicked with "index out of range [3] with length +// 3". It was safe in production only because capTitleRunes' early return guarantees len(r) > +// MaxTitleLen before it ever calls — a coupling two functions apart that no comment stated and no +// test pinned, while bindsToPrevious' own doc invites other callers in this package. The smallest +// fixture anywhere in this file is 89 runes, so nothing came close to it. +// +// cut == len(r) means "the cut is past the last rune": r[cut] does not exist, nothing can be +// orphaned, and the answer is cut itself. Short strings are checked at their own length rather than +// at MaxTitleLen so the boundary is the SUBJECT of the test and not incidental to it. +func TestSessionsPane_ClusterStartHandlesACutAtTheEnd(t *testing.T) { + const acute = "́" + for _, in := range []string{"abc", "", "a", strings.Repeat("a", claude.MaxTitleLen), "e" + acute} { + r := []rune(in) + got := clusterStart(r, len(r)) + if got != len(r) { + t.Errorf("clusterStart(%q, %d) = %d, want %d — a cut past the last rune orphans nothing", + in, len(r), got, len(r)) + } + } + // The one length that used to be load-bearing: exactly MaxTitleLen + 1 runes is the shortest + // string capTitleRunes will cut, so it is the shortest slice clusterStart has ever been handed. + // A test at 89 runes cannot tell whether the bound is len(r) or something larger. + r := []rune(strings.Repeat("a", claude.MaxTitleLen+1)) + if got := clusterStart(r, claude.MaxTitleLen); got != claude.MaxTitleLen { + t.Errorf("clusterStart at the shortest cuttable length = %d, want %d", got, claude.MaxTitleLen) + } +} + +// Harvest wins when both sources name the session. +// +// A fixed precedence, not a judgement about which string is better — see sessionTitleFor. Neither +// side is the "stable" one: the harvest is LAST-wins (core/observe/claude, every tier) and the +// served title is FIRST-wins, so they disagree about which turn should name a session rather than +// one of them being steadier. Asserted at the cell AND the header, because they reach the fallback +// by different routes (the row loop carries the summary; the header looks it up). +func TestSessionsPane_HarvestBeatsServedTitle(t *testing.T) { + m := newServedTitleModel(t, + map[string]SessionMetadata{"s1": {Title: "harvested name"}}, + map[string]string{"s1": "served name"}, + "s1") + m.rebuildSessionsTable() + + if got := sessionsCell(t, m, titleRow(t, m, "s1"), "TITLE"); got != "harvested name" { + t.Errorf("TITLE = %q, want the harvested title to win", got) + } + if got := m.sessionLabel("s1"); got != "harvested name (s1)" { + t.Errorf("sessionLabel = %q, want the harvested title to win", got) + } +} + +// HARVEST WINS EVEN WHEN THE CAP MANGLES THE HARVESTED TITLE, because precedence is a question about +// the harvest and not about the display bound. +// +// sessionTitleFor used to ask whether m.sessionTitle(id) — sanitised AND CAPPED — was blank. So every +// route by which the cap could blank a non-blank harvested title ALSO silently inverted the +// documented precedence: with the cap trimming after its cut, sessionTitleFor(85 spaces + "real", +// "served-name") returned "served-name". That is worse than the blank cell it replaced. An operator +// picks a row by its name before acting on it, and CLAUDE.md, cmd/abctl/README.md and +// core/session/store.go all tell them the harvest is what they are looking at. +// +// NOT A MUTATION GATE, and saying so is the honest version of this test. Reverting the guard to +// !titleIsBlank(m.sessionTitle(id)) leaves the whole suite green — verified by running it. It has to: +// the guard only behaves differently when the cap can blank a non-blank title, and that is exactly +// what the whitespace fix and the blunt-cut fallback between them removed. A search over the +// degenerate shapes this file knows — combining marks, ZWJ, BOM, word joiner, NBSP, ideographic +// space, lone regional indicators, Thai vowel signs, at five lengths around the cap, with and +// without a real suffix — found ZERO inputs that sanitise non-blank and cap to blank. With no such +// input there is no observable difference, so no black-box test can pin the structure. +// +// It is kept anyway, for the two things it does do: it pins the OUTCOME (the measured inversion +// string now resolves to the harvested title, at the cell and the header), and it is where the next +// person who reintroduces a cap-blanking route will find out what else breaks. The structural form +// of the guard is defence for when that search stops being exhaustive — a display bound should not +// be able to reach into a precedence decision at all — and its justification is the argument in +// sessionTitleFor's doc, not a failing assertion here. +func TestSessionsPane_HarvestWinsEvenWhenTheCapShortensIt(t *testing.T) { + const acute = "́" + for _, tc := range []struct { + name string + harvested string + }{ + // The measured inversion: leading whitespace long enough to consume the whole budget. + {"long space prefix", strings.Repeat(" ", claude.MaxTitleLen+5) + "real"}, + // A degenerate cluster from index 0, where the cap has no boundary to cut on and falls + // back to a blunt cut. If that fallback is ever removed, the served title takes over a + // named row rather than the cell merely going blank — this pins both consequences at once. + {"nothing but combining marks", strings.Repeat(acute, claude.MaxTitleLen+20)}, + } { + m := newServedTitleModel(t, + map[string]SessionMetadata{"s1": {Title: tc.harvested}}, + map[string]string{"s1": "served-name"}, + "s1") + m.rebuildSessionsTable() + + // Asserted at the accessor AND both display paths, because the served title reaches them by + // different routes — the row loop carries the summary, sessionLabel looks it up — and the + // inversion showed the proxy's name in all three. + if got := m.sessionTitleFor("s1", "served-name"); got == "served-name" { + t.Errorf("%s: sessionTitleFor returned the SERVED title, inverting the documented harvest-wins precedence", tc.name) + } else if titleIsBlank(got) { + t.Errorf("%s: sessionTitleFor = %q, blank — the harvested title named this session", tc.name, got) + } + if got := sessionsCell(t, m, titleRow(t, m, "s1"), "TITLE"); strings.Contains(got, "served-name") { + t.Errorf("%s: TITLE cell = %q, want the harvested name", tc.name, got) + } + if got := m.sessionLabel("s1"); strings.Contains(got, "served-name") { + t.Errorf("%s: sessionLabel = %q, want the harvested name", tc.name, got) + } + } +} + +// A BLANK harvested title falls back; a title that RENDERS SOMETHING does not. +// +// The distinction is titleIsBlank's, not == "" — a harvested " " paints nothing, so falling back +// fills nothing rather than overriding something. "\t" is the opposite case and the one a +// simplification gets wrong: sanitizeLabel REPLACES it with U+FFFD rather than stripping it, so the +// cell shows a visible glyph and the row is named. Falling back there would override a title that +// is on screen. +func TestSessionsPane_ServedTitleOnlyFillsWhatRendersBlank(t *testing.T) { + for _, tc := range []struct { + name string + harvested string + want string + }{ + {"empty", "", "served name"}, + {"one space", " ", "served name"}, + {"several spaces", " ", "served name"}, + {"tab renders a glyph", "\t", "�"}, + {"newline renders a glyph", "\n", "�"}, + } { + t.Run(tc.name, func(t *testing.T) { + m := newServedTitleModel(t, + map[string]SessionMetadata{"s1": {Title: tc.harvested}}, + map[string]string{"s1": "served name"}, + "s1") + // THROUGH THE FIXTURE, not a literal. Passing "served name" here instead read the + // same string the served map holds, so the assertion passed whether or not + // newServedTitleModel had wired Title onto the summary at all — leaving one test as + // the only gate on that wiring. sessionLabel resolves the served title by id, so + // going through it asserts the fixture and the accessor together. + if got := m.sessionTitleFor("s1", m.servedTitle("s1")); got != tc.want { + t.Errorf("sessionTitleFor(%q harvested) = %q, want %q", tc.harvested, got, tc.want) + } + }) + } +} + +// Neither source names it: the cell stays empty and the header stays a bare id. +// +// The pre-existing behaviour, pinned against the fallback having quietly introduced a placeholder. +// "" is still the honest answer for a session nothing has named. +func TestSessionsPane_NoTitleAnywhereRendersAsBefore(t *testing.T) { + m := newServedTitleModel(t, + map[string]SessionMetadata{"s1": {Title: " "}}, + map[string]string{"s1": " "}, + "s1") + m.rebuildSessionsTable() + + // ASSERTED EXACTLY, not through TrimSpace: trimming here would accept a cell of spaces, which is + // the precise defect titleIsBlank exists to prevent — a whitespace-only title reaching the screen + // while sessionLabel calls the same row unnamed. + if got := sessionsCell(t, m, titleRow(t, m, "s1"), "TITLE"); got != "" { + t.Errorf("TITLE = %q, want an empty cell when neither source names the session", got) + } + if got := m.sessionLabel("s1"); got != "s1" { + t.Errorf("sessionLabel = %q, want the bare id", got) + } +} + +// A served title is SANITISED, exactly as a harvested one is. +// +// /v1/sessions is unauthenticated and the title is folded from caller-supplied event content, so the +// CWE-150 reasoning behind sanitizeLabel applies to this string at least as much as to the file the +// harvester wrote. An ESC recolours the pane; a newline splits the frame. +func TestSessionsPane_ServedTitleIsSanitized(t *testing.T) { + // THROUGH servedTitle, not with the string handed in directly. Passing the hostile title as a + // literal tests sanitizeLabel and nothing else; resolving it from the summary the way the header + // does means this also fails if the lookup stops finding it. + const hostile = "before\x1b[31mafter\nnext" + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": hostile}, "s1") + + if served := m.servedTitle("s1"); served != hostile { + t.Fatalf("servedTitle = %q, want the fixture's title — the rest of this test is vacuous "+ + "without it", served) + } + got := m.sessionTitleFor("s1", m.servedTitle("s1")) + for _, bad := range []string{"\x1b", "\n"} { + if strings.Contains(got, bad) { + t.Errorf("sessionTitleFor = %q, still carries %q", got, bad) + } + } + if !strings.Contains(got, "before") || !strings.Contains(got, "after") { + t.Errorf("sessionTitleFor = %q, want the text kept and only the control runes replaced", got) + } +} + +// EVERY CONTROL CLASS IS REPLACED, including the ones abctl used to leave to the producer. +// +// THIS TEST REPLACES A CHARACTERIZATION. sanitizeLabel covered the BIDI overrides and isolates +// (U+202A-202E, U+2066-2069) and stopped there, so the plain MARKS — U+200E LRM, U+200F RLM, +// U+061C ALM — and the zero-widths passed through untouched. The test that used to sit here recorded +// that as a deliberate reliance on core/session.sanitizeTitle stripping them upstream, and said in +// its own doc that closing the gap should DELETE it rather than invert it. This is that deletion. +// +// WHY THE RELIANCE WAS WRONG. The comment it justified claimed the served path "does not rely on the +// producer" — while the only thing keeping a mark out of the cell WAS the producer. /v1/sessions is +// unauthenticated and operator-pointed, the proxy's normaliser is unexported in another module, and +// this is the rune class whose entire function is to make the rendered order differ from the byte +// order: "report\u202Egnp.exe" reads as something else on screen. A cross-module invariant is a poor +// place to keep that, and pipeline.IsControlRune already named the full set. +// +// THE MARKS AND THE ZERO-WIDTHS ARE DIFFERENT ATTACKS, asserted together because one predicate now +// covers both. A mark REORDERS what follows it and needs no matching pop, so one is enough. A +// zero-width makes two distinct titles render identically, so a row can wear another session's name +// while nothing addresses it by that name. +// +// U+FFFD RATHER THAN DROPPED, per sanitizeLabel's standing rule: tampering must be visible instead of +// silently producing a plausible label. +func TestSessionsPane_ServedTitleStripsEveryControlClass(t *testing.T) { + for _, tc := range []struct{ name, bad string }{ + {"LRM U+200E", "\u200e"}, + {"RLM U+200F", "\u200f"}, + {"ALM U+061C", "\u061c"}, + {"ZWSP U+200B", "\u200b"}, + {"ZWNJ U+200C", "\u200c"}, + {"ZWJ U+200D", "\u200d"}, + {"WJ U+2060", "\u2060"}, + {"BOM U+FEFF", "\ufeff"}, + {"RLO U+202E", "\u202e"}, + {"LRI U+2066", "\u2066"}, + } { + t.Run(tc.name, func(t *testing.T) { + // BUILT PER CASE AND RESOLVED THROUGH servedTitle, so this gates the lookup as well as + // the sanitiser. Handing sessionTitleFor a literal leaves it passing with servedTitle + // stubbed to "" — the header path would then be silently untested here. + title := "report" + tc.bad + "exe" + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": title}, "s1") + if served := m.servedTitle("s1"); served != title { + t.Fatalf("servedTitle = %q, want the fixture's title", served) + } + + got := m.sessionTitleFor("s1", m.servedTitle("s1")) + if strings.Contains(got, tc.bad) { + t.Errorf("sessionTitleFor = %q still carries %s — an unauthenticated API is not a "+ + "place to rely on another module having stripped it", got, tc.name) + } + if !strings.Contains(got, "\ufffd") { + t.Errorf("sessionTitleFor = %q, want %s replaced by U+FFFD so tampering is visible, "+ + "not dropped into a plausible-looking label", got, tc.name) + } + if !strings.Contains(got, "report") || !strings.Contains(got, "exe") { + t.Errorf("sessionTitleFor = %q, want the surrounding text kept", got) + } + }) + } +} + +// AND THE HARVESTED TITLE TOO, since both sources share one sanitiser. +// +// The widening was motivated by the served path, but sessionTitle reads a file on disk that anything +// may have rewritten — LoadSessionMetadata applies no sanitisation of its own — so the same runes +// arrive by that route. Asserting both is what keeps a future narrowing of sanitizeLabel from being +// justified as "only the served path needed it". +func TestSessionsPane_HarvestedTitleStripsBidiMarks(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{ + "s1": {Title: "report\u200eexe"}, + }, "s1") + + if got := m.sessionTitle("s1"); strings.Contains(got, "\u200e") { + t.Errorf("sessionTitle = %q still carries U+200E LRM — the metadata file is not a trusted "+ + "input either", got) + } +} + +// A served title that looks like a path is truncated from the LEFT, like any other. +// +// The fallback feeds the existing cell, so it inherits looksLikePath and both truncation sides +// rather than bypassing them. Cheap to assert and it pins that the fallback did not become a +// second, unbounded rendering path. +func TestSessionsPane_ServedPathTitleTruncatesFromTheLeft(t *testing.T) { + const long = "/Users/someone/src/cortex/.worktrees/servedtitle/authbridge" + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": long}, "s1") + m.width = tableWidth(sessionsColumns()) + m.rebuildSessionsTable() + + got := sessionsCell(t, m, titleRow(t, m, "s1"), "TITLE") + if !strings.HasPrefix(got, "…") || !strings.HasSuffix(got, "authbridge") { + t.Errorf("TITLE = %q, want the cut front and the kept tail", got) + } + if n := lipgloss.Width(got); n > sessionsTitleWidth { + t.Errorf("TITLE is %d columns against a %d-column cell: %q", n, sessionsTitleWidth, got) + } +} + +// A cached-only row shows no served title, and a live row beside it still shows its own. +// +// HONEST ABOUT ITS OWN REACH: this cannot fail on the "simplification" of having the cached-only +// loop call m.servedTitle(id) itself — checked, the whole suite stays green. cachedOnlySessionIDs +// skips every id in m.sessions, so servedTitle can only ever return "" there; the behavior is +// guaranteed by that exclusion, not by this assertion. What the test does earn is the second +// check — that a cached-only row in the table does not disturb the live row's title — plus a +// worked example of the two row kinds side by side. Kept for that, not as a mutation gate. +func TestSessionsPane_CachedOnlyRowTakesNoServedTitle(t *testing.T) { + m := newServedTitleModel(t, + map[string]SessionMetadata{}, + map[string]string{"live": "a live session's name"}, + "live") + m.events["gone"] = []pipeline.SessionEvent{{Host: "example.test"}} + m.rebuildSessionsTable() + + if got := sessionsCell(t, m, titleRow(t, m, "gone"), "TITLE"); got != "" { + t.Errorf("cached-only TITLE = %q, want \"\" — the server lists no summary for it", got) + } + if got := sessionsCell(t, m, titleRow(t, m, "live"), "TITLE"); got != "a live session's name" { + t.Errorf("live TITLE = %q, want its own served title", got) + } +} + +// THE REGRESSION GUARD: a served title must NOT satisfy the harvest-backoff predicates. +// +// Every predicate in session_metadata.go judges "unnamed" through sessionTitle, and that is +// deliberate — a server-titled row has to keep reading as unnamed so the harvest keeps looking for +// the harvested title. Fold the fallback into sessionTitle and the row reads as named, +// untitledMisses zeroes, and the re-harvest stops for good. +// +// BOTH SIDES OF THE BACKOFF, because they fail differently and only one used to be asserted here. +// The SCORING side (countUntitled, harvestNamedSomething) records what a harvest achieved; the GATE +// side (untitledSettled, untitledFresh) decides whether one starts at all. Repointing either gate +// predicate at sessionTitleFor survives every other test in this package — and untitledSettled is +// the exact regression this test is named for, since a served-only row that never opens the gate +// never gets re-harvested no matter what the scoring would have said. +// +// This is the only test that fails on any of those mutations: every display test above still +// passes, because the cell is correct either way. That is exactly why it is here. +func TestSessionMetadata_ServedTitleDoesNotSatisfyTheHarvestBackoff(t *testing.T) { + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": "served name"}, "s1") + + if m.sessionHasTitle("s1") { + t.Error("sessionHasTitle is true for a row only the SERVER named: the harvest will stop") + } + if n, _ := m.countUntitled(); len(n) != 1 { + t.Errorf("countUntitled counted %d untitled rows, want 1 — a served title is not a harvested one", len(n)) + } + // And the harvest can still report progress on it, which is what keeps the backoff from + // capping out on a row it could in fact name. + if !m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: "harvested at last"}}) { + t.Error("harvestNamedSomething is false for a served-only row the harvest just named") + } + // The gate, which is the half that decides whether a harvest runs. Aged past the settle delay + // so the only thing left that can hold the gate shut is the title question. + now := time.Now() + m.sessions[0].UpdatedAt = now.Add(-untitledSettleDelay) + if !m.untitledSettled(now) { + t.Error("untitledSettled is false for a settled served-only row: no harvest will ever start") + } + if !m.untitledFresh() { + t.Error("untitledFresh is false for an uncounted served-only row") + } + // The display, meanwhile, was never blank — which is the point of the whole change. + if got := m.sessionTitleFor("s1", "served name"); got != "served name" { + t.Errorf("sessionTitleFor = %q, want the served title on screen throughout", got) + } +} + +// combiningMarkRune is "e" followed by U+0301 COMBINING ACUTE ACCENT — a DECOMPOSED "e-acute", +// two runes that render as one glyph. +// +// A NAMED CONSTANT BECAUSE THE BYTES ARE THE POINT, and a literal in the test body does not survive +// its own file being edited. Both cap tests need a rune that makes zeroWidthFree false, which is +// what selects the quadratic truncation path an uncapped title would take; a precomposed U+00E9 +// "é" is a single Mn-free rune and takes the FAST path, so a fixture that gets normalised — by an +// editor, a formatter, a copy through a tool that applies NFC — silently stops testing anything +// while still passing. That is the failure mode assertFixtureIsSlowPath exists to catch. +const combiningMarkRune = "e\u0301" + +// assertFixtureIsSlowPath fails when a fixture no longer exercises the cost a cap prevents. +// +// SHARED BY BOTH CAP TESTS, because the hazard is identical on both sides and only one of them used +// to check: the served test guarded zeroWidthFree inline and the harvested one guarded nothing, so +// an NFC normalisation of the harvested fixture would have gone unnoticed. Asserting the rune count +// too catches the other half — a "é" that normalised to one rune also halves the fixture's length, +// which could bring it under the cap and make the test vacuous rather than merely fast. +func assertFixtureIsSlowPath(t *testing.T, s string) { + t.Helper() + if zeroWidthFree(s) { + t.Fatalf("fixture takes the truncation FAST path — it no longer exercises the cost a cap "+ + "prevents. Most likely %q was normalised to a precomposed form; it must stay decomposed "+ + "(a base letter plus a combining mark)", combiningMarkRune) + } + if n := len([]rune(s)); n <= claude.MaxTitleLen { + t.Fatalf("fixture is %d runes, at or under the %d-rune cap — it cannot show that a cap "+ + "applies", n, claude.MaxTitleLen) + } +} + +// THE SERVED TITLE IS CAPPED CLIENT-SIDE, and nothing upstream of abctl is what guarantees it. +// +// The pairing this closes: TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures covers the HARVESTED +// title's cap against claude.MaxTitleLen, and the served title never touches that path. The proxy has +// its own cap, but it is unexported in another module on purpose, /v1/sessions is unauthenticated, and +// abctl is pointed at whatever host an operator names — so "the producer caps it" is not an assertion +// this side can make. +// +// WHY A LENGTH AND NOT A DEADLINE. What an uncapped title costs is not a malformed cell — truncLeft +// and truncRight bound their output regardless — but the quadratic search inside them, whose fast path +// any combining mark disables and which a served title keeps its marks through. Measured on one call, +// 20003 runes took 4.50s and 200003 did not finish in two minutes, on the UI goroutine, per row per +// rebuild. A timing assertion would encode a machine's speed and flake in CI, so this pins the INPUT +// bound that makes the cost flat instead. +// +// THE FIXTURE CARRIES COMBINING MARKS, which is what makes it the real shape rather than a long +// string: an ASCII title of the same length takes the fast path and would pass a weaker cap. +func TestSessionsPane_ServedTitleIsCappedBeforeTruncation(t *testing.T) { + // Every other rune is U+0301, so zeroWidthFree is false and the slow path is what a missing cap + // would hand this to. + served := strings.Repeat(combiningMarkRune, 5000) + assertFixtureIsSlowPath(t, served) + + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": served}, "s1") + + got := m.sessionTitleFor("s1", served) + // EXACTLY THE CAP, NOT "AT MOST" IT. A `>` assertion passed for a BYTE-based cap too: this + // fixture is two bytes per rune, so cutting at 80 BYTES leaves 40 runes, which satisfies any + // at-most bound while silently halving the budget a multi-byte title gets. The cap is specified + // in runes and this is what pins that. combiningMarkRune cuts on a clean boundary at index 80 — + // every even index is the base letter — so the cluster walk does not move the cut here; the + // dedicated test below covers the case where it does. + if n := len([]rune(got)); n != claude.MaxTitleLen { + t.Errorf("sessionTitleFor returned %d runes, want exactly %d — a byte-based cap satisfies "+ + "an at-most bound while halving a multi-byte title", n, claude.MaxTitleLen) + } + + // AND THROUGH THE HEADER TOO, which reaches the same accessor by id alone. Without this, a cap + // applied only in the cell's own call path would pass. + // EXACTLY, not at-most, for the reason argued five lines above: an at-most bound is satisfied by a + // byte cap that halves a multi-byte title's rune budget. + if n, want := len([]rune(m.sessionLabel("s1"))), claude.MaxTitleLen+len(" (s1)"); n != want { + t.Errorf("sessionLabel is %d runes, want exactly %d — the served title capped before it is "+ + "labelled", n, want) + } +} + +// THE HARVESTED TITLE IS CAPPED AT LOAD TOO, not only by the harvester that wrote it. +// +// The asymmetry this closes: the served title is capped in sessionTitleFor, and the harvested one +// was capped only upstream in core/observe/claude. LoadSessionMetadata re-reads that file and +// applies no cap, so a rewritten or hand-edited ~/.cortex/session-metadata.json bypassed the +// guarantee entirely and reached the same quadratic truncation the served cap exists to prevent. +// Measured before the cap: one rebuildSessionsTable took 1.11s on a 10003-rune title. +// +// THROUGH sessionTitle, which is where every consumer reads it — the cell, the headers, and +// sessionHasTitle. Capping at the accessor rather than at load is what makes that one line cover +// all of them. +// +// AND THE BACKOFF VERDICT MUST NOT MOVE, which is where the first version of this cap went wrong. +// sessionHasTitle reads the same accessor, so a clip that blanks a title flips a row from named to +// unnamed and restarts the ~3-minute ~/.claude re-harvest permanently — the cost this package +// documents as the price of an UNNAMABLE session, charged to one that has a name. +// +// THE WHITESPACE-PREFIX CASE IS THE WHOLE POINT, AND THIS TEST GOT IT BACKWARDS TWICE. +// +// It first asserted "truncation cannot turn a non-blank title blank" using a leading-"/" path +// fixture, where no prefix is whitespace — so it could not exercise the claim it made and passed for +// the wrong reason. The whitespace fixture was then added, and the expectation written down as +// wantNamed: FALSE for a title reading 80 spaces + "real name" — labelled "the correctness case" +// while the assertion five lines below it called an unnamed verdict the thing that "restarts the +// ~3-minute re-harvest forever". The test encoded the defect and then explained why the defect was +// bad. +// +// It is wantNamed: true now, because "real name" is a name and nothing about padding changes that. +// capTitleRunes trims BEFORE measuring, so leading whitespace no longer spends the rune budget and +// there is no window for it to fill. See sessionTitle's doc for the measured curve this replaces. +func TestSessionsPane_HarvestedTitleIsCappedAtLoad(t *testing.T) { + for _, tc := range []struct { + name string + title string + wantNamed bool + // slowPath marks the over-long fixture whose combining marks are what select the quadratic + // truncation path. Only that case can degrade silently under NFC normalisation, so only it + // is guarded — the whitespace cases are short by design and would fail the guard's own + // length check. + slowPath bool + }{ + { + // The performance case: over-long, with a combining mark so the truncation fast path + // is off. + name: "over-long path with combining marks", + title: "/a/" + strings.Repeat(combiningMarkRune, 5000), + wantNamed: true, + slowPath: true, + }, + { + // The correctness case: real text begins after where a naive cut would land, so a cap + // that measured before trimming kept only spaces and then trimmed them to "". + name: "whitespace fills the whole clip window", + title: strings.Repeat(" ", claude.MaxTitleLen) + "real name", + wantNamed: true, + }, + { + // Far past it, so no "off by a few runes" fix can pass this by accident. + name: "whitespace far past the clip window", + title: strings.Repeat(" ", 500) + "real name", + wantNamed: true, + }, + { + // And the one that must STILL read as named: whitespace prefix, text inside the window. + name: "whitespace prefix but text within the window", + title: strings.Repeat(" ", claude.MaxTitleLen-4) + "real name", + wantNamed: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.slowPath { + assertFixtureIsSlowPath(t, tc.title) + } + m := newTitleModel(t, map[string]SessionMetadata{"s1": {Title: tc.title}}, "s1") + + got := m.sessionTitle("s1") + if n := len([]rune(got)); n > claude.MaxTitleLen { + t.Errorf("sessionTitle returned %d runes, want at most %d — an uncapped harvested "+ + "title reaches the quadratic truncation path on the UI goroutine", + n, claude.MaxTitleLen) + } + // NO LEADING OR TRAILING WHITESPACE SURVIVES THE CAP, which is what makes the verdict + // below deliberate rather than incidental. The cap trims on both sides of the cut — + // before, so padding does not spend the budget, and after, to drop whitespace the cut + // newly exposed. + if got != strings.TrimSpace(got) { + t.Errorf("sessionTitle = %q, want it trimmed — untrimmed whitespace "+ + "is what flips a named row to unnamed", got) + } + if named := m.sessionHasTitle("s1"); named != tc.wantNamed { + t.Errorf("sessionHasTitle = %v, want %v — a wrong verdict here either restarts "+ + "the ~3-minute re-harvest forever or stops it on a session with no name", + named, tc.wantNamed) + } + }) + } +} + +// THE CAP CUTS ON A GRAPHEME BOUNDARY, never inside a cluster. +// +// WHY THIS IS NOT PEDANTRY. clipTitle upstream (core/observe/claude) does a plain rune cut and says +// in its own doc that this is safe ONLY BECAUSE normalizeTitle ran first and removed every character +// that binds to its neighbour. Neither string capTitleRunes sees has been through that: the harvested +// title is re-read from a file that may have been rewritten, and the served title comes from +// core/session's sanitizeTitle, whose doc says it KEEPS combining marks "so café survives". The first +// version of this cap mirrored clipTitle's TrimSpace while silently dropping its precondition. +// +// WHAT A BLIND CUT PRODUCES is a title nobody wrote: an accent rebound to whatever letter happens to +// land last, half of a ZWJ emoji sequence, or one regional indicator of a two-letter flag — which +// renders as a bare letter. On a column an operator reads to pick a row before acting on it. +// +// EACH CASE PUTS THE BINDER EXACTLY AT THE CUT, which is the only index where the walk-back matters; +// a fixture with clusters merely present would pass with no walk at all. +func TestSessionsPane_CapCutsOnAClusterBoundary(t *testing.T) { + for _, tc := range []struct { + name string + // binder is placed AT index claude.MaxTitleLen, so a cut that does not walk back would + // orphan it from the base rune at MaxTitleLen-1. + binder string + }{ + {"combining acute U+0301 (Mn)", "\u0301"}, + {"variation selector U+FE0F (Mn)", "\ufe0f"}, + {"enclosing circle U+20DD (Me)", "\u20dd"}, + {"Devanagari vowel sign U+093E (Mc)", "\u093e"}, + {"zero-width joiner U+200D", "\u200d"}, + // NOT A LONE REGIONAL INDICATOR. One RI preceded by ordinary text STARTS a flag rather than + // completing one, so the cut before it is already on a boundary and must NOT walk back — + // asserting otherwise is what pinned the walk-to-zero defect in place. RI pairing gets its + // own test below, where the run length is what decides. + // + // NOT U+02B0 AND FRIENDS. Lm modifier LETTERS look like they belong in this list and do + // not: Unicode's Grapheme_Extend property excludes them, and Lm also contains runes that + // legitimately START a cluster (U+02BB ʻokina is a letter in Hawaiian orthography). Treating + // Lm as a binder would walk the cut back off an ordinary word character. The categories that + // extend a cluster are Mn/Me/Mc, and the two sequence-builders below are the special cases + // that carry no category marking them. + } { + t.Run(tc.name, func(t *testing.T) { + // "a" * (MaxTitleLen-1) + a base rune + the binder + filler past the cap. + title := strings.Repeat("a", claude.MaxTitleLen-1) + "e" + tc.binder + + strings.Repeat("z", 50) + r := []rune(title) + if len(r) <= claude.MaxTitleLen { + t.Fatalf("fixture is %d runes, must exceed the %d-rune cap", len(r), claude.MaxTitleLen) + } + if r[claude.MaxTitleLen] != []rune(tc.binder)[0] { + t.Fatalf("fixture misaligned: rune at the cut is %U, want the binder %U", + r[claude.MaxTitleLen], []rune(tc.binder)[0]) + } + + got := capTitleRunes(title) + + // THE BINDER MUST NOT LEAD THE RESULT'S TAIL. Concretely: the cut must not have landed + // between the base rune and its binder, which is what leaves the binder as the first + // rune of nothing. + gr := []rune(got) + if len(gr) == 0 { + t.Fatalf("capTitleRunes returned empty for %q", title) + } + if last := gr[len(gr)-1]; last == []rune(tc.binder)[0] { + t.Errorf("capTitleRunes = ...%U, want the cut walked BACK off the binder so the "+ + "cluster is dropped whole rather than severed", last) + } + // AND THE BASE RUNE GOES WITH IT. Keeping "e" while dropping its accent is the same + // defect from the other side: the glyph changes into one the title never contained. + if strings.HasSuffix(got, "e") { + t.Errorf("capTitleRunes = %q, want the base rune dropped alongside its binder — "+ + "keeping it silently changes the final glyph", got) + } + if n := len(gr); n > claude.MaxTitleLen { + t.Errorf("capTitleRunes returned %d runes, want at most %d", n, claude.MaxTitleLen) + } + }) + } +} + +// REGIONAL INDICATORS PAIR, so the walk must count the run rather than treat each one as a binder. +// +// THE DEFECT THIS PINS: bindsToPrevious answers true for every RI, and the walk used to act on that +// answer directly, so a run of them had no even-parity rune to stop at and the cut slid to index 0. +// 41 consecutive flags capped to "" and "a"*70 + 10 flags lost 11 runes, against a doc promising the +// over-walk costs "a rune or two". +// +// WHY AN EMPTIED TITLE IS NOT COSMETIC, and why this test sits with the backoff tests in spirit: +// sessionHasTitle reads sessionTitle, which caps. A harvested flag title that caps to "" reads as +// UNNAMED, zeroing untitledMisses and restarting the permanent ~3-minute re-harvest for a row the +// harvest had already named. f5a0a615 fixed that exact failure for whitespace-only titles; this is +// the same failure reached through the cap instead of through the trim. +// +// THE PARITY IS WHAT IS ASSERTED, not a single index: an even-length run before the cut means the +// rune at the cut starts a fresh flag and the cut is already on a boundary, while an odd-length run +// means it completes one and the cut must step back exactly once. +func TestSessionsPane_CapWalksBackOverAtMostOneRegionalIndicator(t *testing.T) { + const flag = "\U0001F1FA\U0001F1F8" // two RIs + + t.Run("a title of nothing but flags keeps its cap", func(t *testing.T) { + // 41 flags = 82 RIs, every one of them a binder by category. A walk that does not pair + // returns "" here. + title := strings.Repeat(flag, 41) + got := capTitleRunes(title) + n := utf8.RuneCountInString(got) + if n == 0 { + t.Fatalf("capTitleRunes emptied a %d-rune flag title — the walk ran to index 0 instead "+ + "of stopping at a pair boundary, which flips the row to unnamed and restarts the "+ + "re-harvest", utf8.RuneCountInString(title)) + } + // The cap is even and flags are two runes wide, so the whole budget is usable here. + if n != claude.MaxTitleLen { + t.Errorf("capTitleRunes returned %d runes, want exactly %d", n, claude.MaxTitleLen) + } + // AND NO HALF FLAG AT THE TAIL: an odd count would mean a severed pair. + if n%2 != 0 { + t.Errorf("capTitleRunes returned an odd %d runes, so the tail is half a flag", n) + } + }) + + t.Run("text then flags loses at most one rune to the walk", func(t *testing.T) { + // The cut at index 80 lands on the FIRST RI of the sixth flag: the run before it is 10 runes + // (five whole flags), which is even, so it starts a pair and the cut needs no walk at all. + title := strings.Repeat("a", 70) + strings.Repeat(flag, 10) + got := capTitleRunes(title) + if n := utf8.RuneCountInString(got); n != claude.MaxTitleLen { + t.Errorf("capTitleRunes returned %d runes, want %d — the walk crossed a pair boundary "+ + "it had no reason to cross", n, claude.MaxTitleLen) + } + }) + + t.Run("an odd run steps back exactly once", func(t *testing.T) { + // 71 letters shifts the parity: the cut now lands on the SECOND RI of a flag, so the walk + // must step back one rune and no further. + title := strings.Repeat("a", 71) + strings.Repeat(flag, 10) + got := capTitleRunes(title) + n := utf8.RuneCountInString(got) + if n != claude.MaxTitleLen-1 { + t.Errorf("capTitleRunes returned %d runes, want %d — exactly one step back off the "+ + "second half of a flag", n, claude.MaxTitleLen-1) + } + // DELIBERATELY NO "the tail is not an RI" ASSERTION. It is the tempting one and it is wrong: + // after stepping back, the tail here IS a regional indicator — the FIRST of a pair, which is + // a legal boundary. A title may end where a flag was about to begin. Only an odd-parity RI + // at the cut is a severed pair, and the length assertion above is what pins that. + if n%2 == 0 { + t.Errorf("capTitleRunes returned %d runes; this fixture's cut has odd parity, so an even "+ + "result means the walk moved further than the one step the pairing calls for", n) + } + }) +} + +// A HARVESTED FLAG TITLE STILL READS AS NAMED, which is the consequence the cap must not break. +// +// The display tests above assert what the cell shows; this asserts what the BACKOFF concludes, and +// they are different questions with different failure modes. sessionHasTitle goes through +// sessionTitle, which caps — so any cap bug that empties a title silently converts "named" into +// "unnamed", zeroes untitledMisses and restarts a ~3-minute re-harvest that can never succeed. The +// row keeps rendering whatever the fallback finds, so nothing on screen says anything is wrong. +// +// THIS IS THE SECOND ROUTE TO ONE FAILURE. f5a0a615 closed the first: a whitespace-prefixed title +// that TrimSpace emptied. The cap is the other, and a test that only checks the rendered cell cannot +// tell them apart. +func TestSessionsPane_FlagOnlyHarvestedTitleStaysNamed(t *testing.T) { + flags := strings.Repeat("\U0001F1FA\U0001F1F8", 41) + m := newTitleModel(t, map[string]SessionMetadata{"s1": {Title: flags}}, "s1") + + if got := m.sessionTitle("s1"); got == "" { + t.Fatalf("sessionTitle emptied a flag-only harvested title") + } + if !m.sessionHasTitle("s1") { + t.Errorf("sessionHasTitle = false for a session the harvest DID name, so the row will "+ + "re-harvest forever; sessionTitle = %q", m.sessionTitle("s1")) + } + if counted, _ := m.countUntitled(); len(counted) != 0 { + t.Errorf("countUntitled counted %d rows, want 0 — a named row must not be a miss", len(counted)) + } +} + +// THE Lm/Sk EXCLUSION IS A GATE, not just a paragraph in bindsToPrevious's doc. +// +// Adding unicode.Lm and unicode.Sk to that predicate is the single most plausible "improvement" a +// future reader can make to it — they are modifier categories, they look like the mark categories, +// and until this test existed the whole suite stayed green while every title ending in one of them +// silently lost a character. U+02BB ʻokina is a LETTER in Hawaiian orthography, so walking back off +// it truncates an ordinary word. +// +// ASSERTED AT THE PREDICATE, deliberately, rather than only through capTitleRunes: a cap-level test +// would need a fixture placing the rune exactly at index MaxTitleLen, and the point is the category +// judgement itself. +func TestSessionsPane_ModifierLettersDoNotBind(t *testing.T) { + for _, tc := range []struct { + name string + r rune + }{ + {"U+02BB okina (Lm) — starts a cluster in Hawaiian", '\u02bb'}, + {"U+02B0 modifier small h (Lm)", '\u02b0'}, + {"U+02C7 caron (Sk) — a standalone symbol", '\u02c7'}, + {"U+02D0 triangular colon (Lm)", '\u02d0'}, + } { + t.Run(tc.name, func(t *testing.T) { + if bindsToPrevious(tc.r) { + t.Errorf("bindsToPrevious(%U) = true, want false: Grapheme_Extend excludes modifier "+ + "letters and symbols, and Lm holds runes that legitimately START a cluster", + tc.r) + } + }) + } + + // AND AT THE CAP, for the one that is a real word character: a title ending in ʻokina keeps it. + title := strings.Repeat("a", claude.MaxTitleLen-1) + "\u02bb" + strings.Repeat("z", 20) + got := capTitleRunes(title) + if n := utf8.RuneCountInString(got); n != claude.MaxTitleLen { + t.Errorf("capTitleRunes returned %d runes, want %d — the walk crossed a modifier letter", + n, claude.MaxTitleLen) + } + if !strings.HasSuffix(got, "\u02bb") { + t.Errorf("capTitleRunes = %q, want the trailing okina kept", got) + } +} + +// SANITISING HAPPENS BEFORE CAPPING, and the order is observable rather than a matter of taste. +// +// sessionTitleFor's doc argues the ordering; this makes it fail if reversed. The two orders agree on +// WHERE the cut falls — sanitizeLabel is rune-for-rune — but not on WHAT sits at it. A ZWJ exactly at +// the cut is the discriminating case: sanitising first replaces it with U+FFFD, a standalone glyph +// that binds to nothing, so the cut stands and 80 runes survive. Capping first sees the live ZWJ, +// walks back off it, and returns 79 — one rune shorter for a title whose rendered form contains no +// joiner at all, because the joiner was going to be replaced regardless. +// +// ASSERTED THROUGH sessionTitleFor, the real caller, so this pins the composition and not just two +// helpers in isolation. +func TestSessionsPane_ServedTitleIsSanitizedBeforeCapping(t *testing.T) { + // A base rune then a ZWJ exactly at index MaxTitleLen, then filler past the cap. + title := strings.Repeat("a", claude.MaxTitleLen-1) + "e\u200d" + strings.Repeat("z", 20) + if r := []rune(title); r[claude.MaxTitleLen] != '\u200d' { + t.Fatalf("fixture misaligned: rune at the cut is %U, want U+200D", r[claude.MaxTitleLen]) + } + + m := newServedTitleModel(t, map[string]SessionMetadata{}, map[string]string{"s1": title}, "s1") + got := m.sessionTitleFor("s1", m.servedTitle("s1")) + + if n := utf8.RuneCountInString(got); n != claude.MaxTitleLen { + t.Errorf("sessionTitleFor returned %d runes, want %d — capping before sanitising walks back "+ + "off a joiner that sanitizeLabel was about to replace with a standalone glyph", n, + claude.MaxTitleLen) + } + // THE TAIL IS THE BASE RUNE, NOT THE JOINER, and that is the point rather than an oversight: the + // cut is exclusive, so r[:MaxTitleLen] never contains the rune AT the cut. What the ordering + // decides is whether the walk moves that boundary, and the rune count above is the only thing + // that can see it. Asserting a U+FFFD tail here would be asserting the fixture, not the order. + if !strings.HasSuffix(got, "e") { + t.Errorf("sessionTitleFor = %q, want the base rune at the boundary kept", got) + } +} + +// A title of nothing but combining marks KEEPS ITS CAP-LENGTH PREFIX rather than capping to "". +// +// THIS TEST ASSERTED THE OPPOSITE AND WAS WRONG, in the same way the lone-regional-indicator test +// was wrong an earlier round: it characterised what the walk did instead of what the cap owes its +// callers. Its old reasoning was that every rune binds, so the walk runs to index 0 and "there is no +// cluster to keep" — and that "\"\" is what sessionHasTitle already handles". That last clause is +// the defect, stated as the justification. sessionHasTitle does not "handle" "": it reads it as +// UNNAMED, which zeroes untitledMisses and restarts the permanent ~3-minute re-harvest for a session +// that had a perfectly good name. Returning "" is the expensive answer, not the honest one. +// +// The degenerate end of the walk is still worth pinning, so this keeps the fixture and inverts the +// expectation. With no base character anywhere there IS no boundary to cut on, so the cap takes its +// blunt prefix and accepts a split cluster. That trade is deliberate and one-directional: a dangling +// mark renders as an odd glyph on one row, while "" silently costs a transcript scan every three +// minutes for as long as the pane is open. +func TestSessionsPane_CapOfOnlyCombiningMarksKeepsAPrefix(t *testing.T) { + title := strings.Repeat("\u0301", claude.MaxTitleLen+20) + + got := capTitleRunes(title) + if titleIsBlank(got) { + t.Errorf("capTitleRunes = %q, which is blank — a named session would read as unnamed and re-harvest forever", got) + } + if n := utf8.RuneCountInString(got); n != claude.MaxTitleLen { + t.Errorf("capTitleRunes returned %d runes, want the blunt %d-rune prefix", n, claude.MaxTitleLen) + } +} + +// A WHITESPACE-ONLY SERVED TITLE MUST NOT PAINT SPACES INTO THE CELL. +// +// titleIsBlank guarded the harvested title and not the served one, so sessionTitleFor returned +// " " verbatim: the cell rendered blanks while sessionLabel — which blank-checks what +// sessionTitleFor returns — rendered the bare id. One accessor, two callers, two different names +// for the same session. Asserted on both ends so they cannot drift apart again. +// +// SPACES ONLY, deliberately. A tab or a newline is NOT blank by this package's definition: +// titleIsBlank sanitises before it trims, so "\t" becomes a visible U+FFFD glyph and is a real — +// if ugly — title. Writing this test with "\t" in the fixture failed, and the test was wrong +// rather than the code; TestSessionsPane_ServedTitleOnlyFillsWhatRendersBlank already pins that +// sanitise-before-trim order from the harvested side, and the two must not contradict each other. +func TestSessionsPane_BlankServedTitleRendersAsUnnamed(t *testing.T) { + for _, served := range []string{" ", " ", " ", " 　 "} { + t.Run(strconv.Quote(served), func(t *testing.T) { + m := newServedTitleModel(t, map[string]SessionMetadata{}, + map[string]string{"s1": served}, "s1") + + if got := m.sessionTitleFor("s1", m.servedTitle("s1")); got != "" { + t.Errorf("sessionTitleFor = %q, want an empty string — a blank served title must "+ + "not paint whitespace into the cell", got) + } + if got := m.sessionLabel("s1"); got != "s1" { + t.Errorf("sessionLabel = %q, want the bare id", got) + } + }) + } +} + +// TestSessionsPane_ZeroWidthFreeAndBindsToPreviousDisagreeOnPurpose pins the rune classes on which +// the file's two Unicode-category predicates deliberately differ. +// +// They look like near-duplicates and are not. zeroWidthFree asks "could a rune count of this string +// be wrong about its DISPLAY WIDTH?" and so covers Cf and Cc (invisible) and Sk (modifier symbols, +// which combine in emoji sequences) alongside Mn/Me. bindsToPrevious asks "would cutting BEFORE this +// rune orphan it from its cluster?" and so covers Mc — a spacing combining mark, which has width and +// therefore does not concern zeroWidthFree at all — while excluding Cf and Cc because sanitizeLabel +// has already replaced those with U+FFFD before either predicate sees a served title. +// +// WITHOUT THIS TEST the divergence is unpinned, and the tempting refactor — one shared category set, +// or one predicate calling the other — passes the rest of the suite while being wrong in both +// directions: it would make a Mc-terminated title take zeroWidthFree's slow path for no reason, and +// make bindsToPrevious walk back off an Sk that starts nothing. +func TestSessionsPane_ZeroWidthFreeAndBindsToPreviousDisagreeOnPurpose(t *testing.T) { + cases := []struct { + name string + r rune + zeroWidthFree bool // false == "this rune defeats the fast path" + binds bool + }{ + // Mc: spacing combining mark. Binds (cutting before it orphans it), but it HAS a column, so + // a rune count is not wrong about it and the width fast path may keep running. + {"Mc DEVANAGARI SIGN VISARGA U+0903", 'ः', true, true}, + // Sk: modifier symbol. The emoji skin-tone modifiers are here, and they DO combine — so a + // rune count misjudges the width and zeroWidthFree must claim them. bindsToPrevious does + // not, because Grapheme_Extend excludes Sk: U+1F3FB is Emoji_Modifier, which the grapheme + // rules handle as part of an emoji sequence rather than as an extender. This is the + // sharpest of the four rows — the only one where a reader might think bindsToPrevious is + // the one with the bug. It is not: over-claiming here costs a rune off a title for + // nothing, and the cut is exclusive, so a cut BEFORE U+1F3FB leaves the base emoji whole. + {"Sk EMOJI MODIFIER FITZPATRICK U+1F3FB", 0x1F3FB, false, false}, + // Cf: format. Invisible, so the width count is wrong — and sanitizeLabel has already turned + // it into U+FFFD by the time the cap runs, which is why bindsToPrevious need not claim it. + {"Cf ZWSP-adjacent U+2060 WORD JOINER", '⁠', false, false}, + // Mn: the one class both claim, for their two different reasons. + {"Mn COMBINING ACUTE U+0301", '́', false, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := zeroWidthFree(string(tc.r)); got != tc.zeroWidthFree { + t.Errorf("zeroWidthFree(%U) = %v, want %v", tc.r, got, tc.zeroWidthFree) + } + if got := bindsToPrevious(tc.r); got != tc.binds { + t.Errorf("bindsToPrevious(%U) = %v, want %v", tc.r, got, tc.binds) + } + }) + } +} diff --git a/cmd/abctl/tui/usage_render.go b/cmd/abctl/tui/usage_render.go index 04d7b4688..5006c16f5 100644 --- a/cmd/abctl/tui/usage_render.go +++ b/cmd/abctl/tui/usage_render.go @@ -7,6 +7,7 @@ import ( "time" "github.com/rossoctl/cortex/core/cost/usage" + "github.com/rossoctl/cortex/core/pipeline" ) // Bar geometry. Four columns wide with a one-column gap, so ten bars occupy 49 @@ -628,6 +629,18 @@ func provenanceNote(by map[string]int64) string { // // Control characters and DEL become U+FFFD rather than being dropped, so tampering is // visible instead of silently producing a plausible-looking label. +// +// ALSO USED FOR SESSION TITLES, from two sources — the harvested transcript title and the one +// /v1/sessions serves — which is what widened it past the inference-model labels it was written +// for. Both of those are content this side did not produce and cannot bound: the harvested title is +// re-read from a file on disk that anything may have rewritten, and the served one is folded from +// caller-supplied event content and arrives over an unauthenticated API. Nothing here assumes +// either producer sanitised anything. +// +// THE FULL CONTROL SET, not a subset of it: C0/C1/DEL, the BIDI overrides and isolates, AND the +// BIDI marks and zero-widths that pipeline.IsControlRune names. The last group was missing while +// this function only saw model labels, and the gap became visible the moment a served title could +// reach a cell — see the switch arms for what each class does on a terminal. func sanitizeLabel(s string) string { var b strings.Builder b.Grow(len(s)) @@ -647,7 +660,26 @@ func sanitizeLabel(s string) string { // display in an order that is not the order of its bytes — "report\u202Egnp.exe" reads as // something else entirely — and these titles are LLM-generated transcript text from an // unauthenticated-by-nature file, so their content is not ours to trust. - case r >= 0x202a && r <= 0x202e, r >= 0x2066 && r <= 0x2069: + // BIDI MARKS AND ZERO-WIDTHS: U+200E/200F/061C, and U+200B/200C/200D/2060/FEFF. Delegated to + // pipeline.IsControlRune rather than re-listed, because that predicate is the repo's single + // copy of this rule \u2014 its own doc records that three byte-identical duplicates once drifted + // under a comment asserting they moved together, and only one had coverage for the marks. + // + // THE MARKS ARE THE SAME CLASS AS THE OVERRIDES ABOVE, and strictly easier to abuse: an + // override needs a matching pop, while one LRM reorders the neutral characters after it on + // its own. Leaving them out while replacing U+202A-E was an inconsistency, not a judgement. + // + // THE ZERO-WIDTHS ARE A DIFFERENT ATTACK, and the reason to take them in the same pass: they + // make two distinct labels render identically, so a session can wear another's name on + // screen while nothing addresses it by that name. They also disable zeroWidthFree, which is + // the truncation fast path \u2014 replacing them with a visible glyph both shows the tampering + // and keeps the cheap path available. + // + // WHY THE C0/C1/DEL CASES STAY OPEN-CODED above rather than folding into the same call: they + // must remain readable as the CWE-150 answer this function was introduced for, and the + // switch documents each class where it applies. IsControlRune covers those ranges too, so + // this arm is reached only for what the earlier arms did not claim. + case r >= 0x202a && r <= 0x202e, r >= 0x2066 && r <= 0x2069, pipeline.IsControlRune(r): b.WriteRune('\uFFFD') default: b.WriteRune(r) diff --git a/core/session/store.go b/core/session/store.go index d329d9201..46496718f 100644 --- a/core/session/store.go +++ b/core/session/store.go @@ -734,9 +734,15 @@ type SessionSummary struct { // lost when the event carrying it is evicted. // // omitempty on the standing rule CostMicros states below: an unknown value must not - // render as a real one. No consumer reads this yet — abctl's TITLE column still comes - // from harvested Claude Code transcripts — so an absent key is what a client that starts - // reading it should expect for a session the events never named. + // render as a real one, so an absent key is what a client should expect for a session the + // events never named. + // + // ABSENT IS A DISPLAYED STATE NOW, not just an unread one. abctl renders this in its TITLE + // column as a FALLBACK: it prefers a title harvested from Claude Code's transcripts and + // reaches for this one only when that harvest named nothing. So the sessions where this + // field decides what an operator sees are exactly those with no transcript on the machine + // running abctl — an agent that routes through the proxy without writing Claude Code + // transcripts is the case that motivated it. Do not assume a change here is invisible. Title string `json:"title,omitempty"` TotalTokens int `json:"totalTokens,omitempty"` // sum of Inference.TotalTokens across response events // CostMicros is what this session's events cost, in millionths of a dollar, summed from