From cd04e698e811ef467bb7348943ab0ff7b0ddad95 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 30 Sep 2026 07:07:43 -0400 Subject: [PATCH] Feat: Show the served session title in abctl when harvesting names nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit abctl's TITLE column came only from harvested Claude Code transcripts, so a session with no transcript on the operator's disk rendered blank. /v1/sessions publishes a server-derived title; use it when the harvest has none. The fallback is display-only. sessionTitle and sessionHasTitle are unchanged, so a served-titled row still reads as unnamed to the harvest backoff and keeps being re-harvested. Harvest wins when both titles exist, decided on the raw harvested title so a display bound cannot invert the precedence. Both titles are sanitised and capped at MaxTitleLen. The cap trims before it measures, so leading whitespace cannot spend the rune budget and push the real name past the cut, and it walks back to the start of any grapheme cluster straddling the cut so it cannot leave a dangling mark or half a flag. It never returns "" for a title that renders something — an emptied title would read as unnamed and restart the permanent ~3-minute re-harvest. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- CLAUDE.md | 2 +- cmd/abctl/README.md | 51 +- cmd/abctl/main.go | 8 +- cmd/abctl/tui/app.go | 21 +- cmd/abctl/tui/session_metadata.go | 142 +++- cmd/abctl/tui/sessions_pane.go | 452 +++++++++- cmd/abctl/tui/sessions_title_test.go | 1159 +++++++++++++++++++++++++- cmd/abctl/tui/usage_render.go | 34 +- core/session/store.go | 12 +- 9 files changed, 1800 insertions(+), 81 deletions(-) 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