Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe TUI now uses proxy-served session titles when harvested titles are unavailable. Harvested titles remain preferred. Served titles do not count as harvested titles for title checks or re-harvest backoff. ChangesSession title fallback
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The served-title fallback remains display-only, preserves harvested-title precedence, and applies the intended sanitization. No merge-blocking issue was established; the change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is limited to session naming for display. Titles are sanitized and capped, matched to the correct session, and kept separate from transcript-harvesting decisions. No introduced security concern was established, but incomplete comparison coverage warrants a cautious low-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abctl's TITLE column came only from harvested Claude Code transcripts, so a session with no transcript tree on the operator's disk rendered blank. PR rossoctl#1167 added a server-derived title to /v1/sessions and nothing read it. Now the display falls back to it. On the laptop this was found on, every blank harvested entry belonged to an agent that has no Claude Code transcript tree — but does route through the proxy, so a served title existed for exactly those rows. The two sources are complementary rather than redundant: harvesting names what the proxy never saw, the proxy names what the filesystem cannot. Harvest wins when both exist. It is the richer of the two (cwd plus prompt text, tiered) and the stable one, since the served title is first-wins per session. The fallback is display-only, which is the load-bearing part. sessionTitle feeds sessionHasTitle and through it every harvest-backoff predicate, so folding the served title in there would make the row read as named, zero untitledMisses and stop the re-harvest permanently — settling for whichever title the proxy derived first. So sessionTitle is unchanged and a new sessionTitleFor applies the precedence for the cell and the three headers. A server-titled row therefore still reads as unnamed to the backoff and keeps being re-harvested; that periodic scan is the intended trade. Two details worth keeping: - the fallback triggers on titleIsBlank, not == "", so a harvested " " (which paints nothing) falls back while a harvested "\t" (which sanitizeLabel turns into a visible glyph) does not — overriding a title already on screen would be the worse bug. - the served string is sanitizeLabel'd like the harvested one. /v1/sessions is unauthenticated and the title is folded from caller-supplied event content, so sessionTitle's CWE-150 reasoning applies to it at least as much. Cached-only rows pass no served title by construction: they exist because the server stopped listing the session, so there is no summary to carry one. Their test says so plainly now — it cannot fail on the "simplification" of looking the title up there, because cachedOnlySessionIDs excludes every listed id, so the lookup can only return "". What it earns is the check that such a row does not disturb a live row's title; it is kept for that and not as a gate. The README described the old behavior in the two places a reader looks first — the Sessions pane column list and --skip-claude-metadata — and both now say what fills the column and which source wins. One transient shape is documented rather than changed: a session that arrives on the event stream before a list refresh gets a stub summary with a zero Title, so its row shows no served title for up to two seconds. It is the one display path where an empty served string does not mean the proxy derived none, and it self-corrects on the next poll. Eight tests, and the one that matters is the backoff guard — it is the only one that fails when the fallback is moved into sessionTitle, because the rendered cell is correct either way. Five mutations checked by hand, each dying against the test written for it: fallback moved into sessionTitle, precedence swapped, trigger weakened to == "", sanitizeLabel dropped, and sessionLabel reverted to harvest-only. Verified: cmd/abctl/tui green with -race, gofmt and go vet clean, go mod tidy -diff clean. The one cmd/abctl failure (TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost) predates this branch — it wants a local CA bundle this machine has not got — and was confirmed failing on the untouched base. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
esnible
force-pushed
the
feat/tui-served-title
branch
from
September 29, 2026 21:43
3189468 to
ed78188
Compare
…ved title Review found eight places where the fallback's arrival left a claim behind that is now false, plus one footgun. All comment and doc changes except the constant; no behavior changes, so the suite passing unmodified is the check that the descriptions now match the code rather than the reverse. The two that actually misled: - sessionHasTitle's doc said it judges titles "as the TITLE cell would judge it" and that the two "can never disagree". The entire design is that they now deliberately do: a served-only row displays a title while this predicate calls it unnamed, which is what keeps the harvest looking. It is now documented as asking whether the HARVEST has named the session, with the distinction and the warning not to widen it. - --skip-claude-metadata's help text still promised bare ids for unharvested rows. The README was fixed for that in the previous commit and the flag string was not, which is the copy a user reads without opening a doc. Overstatement, corrected in all three places it appeared (sessionTitleFor, README, CLAUDE.md): the harvest was justified as "richer — cwd plus prompt text, tiered". It is either/or, not both — the harvester returns from one switch arm, and on the tree measured most sessions fell through to a bare cwd. Since both sides pick titles through their own ranking and both may change, these now say as little as possible about either mechanism: harvest-wins is recorded as a fixed precedence, not a claim about which string is better, with the note that the two rankings do not agree on every session and an explicit /rename is the clearest case where the loser looks better. Accepted for now pending what operators report; it is one line to invert. titleIsBlank's doc enumerated its three callers and described what each asks about. The list was wrong the moment sessionTitleFor was added, so it is gone rather than extended — the callers are one grep away. untitledSettled's clock-skew comment said the harm is a permanently blank TITLE cell. With a fallback it is stuck on whatever the proxy served, or blank if it served nothing. app.go's harvest repaint said the sessions table is the only cell a title lands in and implied the harvest map is the only thing naming sessions. Two inputs do now; only the harvest needs its repaint triggered there, because the list refresh rebuilds on its own path. Verified by checking every write to m.sessions. The footgun, and the one real code change: sessionTitleFor takes the served title as a parameter, so sessionTitleFor(id, "") compiles and silently disables the fallback. The parameter stays — the live row loop already holds the summary, and resolving it internally would put a scan of m.sessions in the per-row render path — so the single legitimate empty argument is now a named noServedTitle constant. Also noted where the value reaches the two display paths differently (the row loop has the summary, the header has only an id), so neither gets "unified" into the other. Two findings from the same review are not addressed here, deliberately. SessionSummary.Title's doc still says "No consumer reads this yet", which is now false, but it lives in core/session and this PR is scoped to abctl. And the suggestion that the served title removes an escape hatch for keeping prompt text off screen is declined: --skip-claude-metadata suppresses the scan, never the display, and the served title is folded from events the same operator is already reading on the same unauthenticated port. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
huang195
requested changes
Sep 29, 2026
huang195
left a comment
huang195
left a comment
Member
There was a problem hiding this comment.
The fallback is correct and cleanly layered. Keeping it out of sessionTitle is the right call, and the headline mutation (folding the fallback into sessionTitle) is killed by exactly the test the body says, and only that test in the whole package.
Two new behaviors have no test that can fail, and both fixes are under 10 lines in this PR's own test file (guards inline, each verified green at HEAD and red under its mutant):
servedTitle's id match: every fixture lists one session.- The gate side of the backoff (
untitledSettled/untitledFresh), which the regression test's header claims to cover.
The rest are comments the second commit set out to correct and missed. One more is outside this diff: core/session/store.go:737 still says "No consumer reads this yet". The commit declines it for scope, which is fair, but it becomes false on merge. Worth a follow-up issue.
Mutation gate: 15 mutants, 11 killed, 4 survived (3 live gaps, 1 unreachable by construction: the cached-only lookup, as the test's own comment says).
| // 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 { |
Member
There was a problem hiding this comment.
must-fix: this lookup has no test that can fail. Mutating if s.ID == id to if s.ID != "" (so every id gets the first listed session's title) leaves the whole tui suite green. Every served-title fixture lists exactly one session, so the lookup is indistinguishable from a constant. It is reachable on any pod with two or more sessions, where session B's header would show A's title.
This guard is green at HEAD and red under that mutant. It could also go into TestSessionsPane_ServedTitleFillsAnUnharvestedCell by adding an s2:
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)", "gone": "gone"} {
if got := m.sessionLabel(id); got != want {
t.Errorf("sessionLabel(%q) = %q, want %q", id, got, want)
}
}| // | ||
| // This is the only test that fails on that mutation: 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) { |
Member
There was a problem hiding this comment.
must-fix: the gate half of the backoff is unasserted. The header says a served title must not satisfy "the harvest-backoff predicates", and the PR body lists four. This test asserts countUntitled and harvestNamedSomething, which are the scoring side. It does not assert the two predicates the gate actually reads (app.go:1457–1462). Repointing untitledSettled's skip (session_metadata.go:246) or untitledFresh's (:307) at !titleIsBlank(m.sessionTitleFor(id, s.Title)) survives the whole suite. The untitledSettled one is exactly the regression this test is named for: a served-only row never opens the gate, so the re-harvest stops for good.
This guard is green at HEAD and red under both mutants:
// The GATE's two predicates too, not only the scoring's: untitledSettled decides whether a
// harvest starts at all.
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 start")
}
if !m.untitledFresh() {
t.Error("untitledFresh is false for an uncounted served-only row")
}|
|
||
| // Harvest wins when both sources name the session. | ||
| // | ||
| // It is the richer of the two — tiered from cwd and prompt text — and the stable one: the served |
Member
There was a problem hiding this comment.
suggestion: c2f2088 retracted "richer — cwd plus prompt text, tiered" as an overstatement, "corrected in all three places it appeared". It is still here, and still in the PR body ("the richer of the two", "the richer harvested one"). The "stable one" reason is also backwards. The harvest is LAST-wins (core/observe/claude/harvest.go:510; every prompt tier is last-wins), while the served title is FIRST-wins. So the harvest lets whichever turn landed last decide, which is exactly what this comment faults the proxy for doing with the first turn. Suggest reusing sessionTitleFor's "fixed precedence, not a judgement" wording.
| // | ||
| // 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 hunting for the better title, at the cost of a periodic |
Member
There was a problem hiding this comment.
nit: "hunting for the better title" sits ten lines below "Deliberately a fixed precedence and not a judgement about which string is better". "the harvested title" or "the title it would prefer" (sessionHasTitle's wording) keeps the two consistent. CLAUDE.md's /v1/sessions row has the same pair ("not a judgement" … "looking for the better one").
| |---|---|---| | ||
| | `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 `<user_query>`, else ordinary user prose, with `<system-reminder>` 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 `<user_query>`, else ordinary user prose, with `<system-reminder>` 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 the better one. | |
Member
There was a problem hiding this comment.
nit: this cell says the precedence is "fixed rather than a judgement about which string is better" and then ends "so a served title does not stop it looking for the better one". "looking for a harvested one" would say the same without the judgement.
| // 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 DOES NOT CALL THIS, and the claim that it does was overstated. sessionTitleCell tests |
Member
There was a problem hiding this comment.
suggestion: this paragraph went stale in this PR. sessionTitleCell → sessionTitleFor → titleIsBlank, so the cell does call this now. And "a " " title … is returned as " "" no longer holds: a probe at HEAD gets "" for a harvested " " with no served title, and the served title when there is one. Line 377's "it renders sessionTitle" should now say sessionTitleFor.
| @@ -136,12 +136,34 @@ func (m *model) sessionLabel(id string) string { | |||
| // THROUGH titleIsBlank, like the other two consumers of "is this named". A raw != "" accepted | |||
Member
There was a problem hiding this comment.
nit: "like the other two consumers" is now three: sessionHasTitle, harvestNamedSomething and sessionTitleFor. This PR removed titleIsBlank's caller list because it "went stale the first time a caller was added", and this count did the same. Dropping the number avoids the next one.
| // words, which is the side this test is named for. | ||
| titleW := sessionsColumnWidth(sessionsColumnsFor(90), "TITLE") | ||
| cut := m.sessionTitleCell("s1", titleW) | ||
| cut := m.sessionTitleCell("s1", "", titleW) |
Member
There was a problem hiding this comment.
nit: noServedTitle exists so that "the one legitimate empty argument says so by name". This call and the five other rewritten ones (647, 787, 856, 943, 1000) pass a bare "" where they mean exactly that.
…lently Review found two new behaviors with no test that could fail. Both are real reachable bugs, not theoretical ones, and each mutant was confirmed surviving the whole tui suite before the guard was written and killing it after. servedTitle's id match had no fixture that could distinguish it from a constant. Every served-title fixture listed exactly one session, so mutating `if s.ID == id` to `if s.ID != ""` — return the first listed session's title for every id — left the suite green. On any pod listing two or more sessions that shows session B's header with A's title. TestSessionMetadata_ServedTitleIsPerSession lists two and also asks about an unlisted id, so it pins the miss path that returns "" as well; the mutant fails both of those assertions. The GATE half of the backoff was unasserted. The regression guard's own header claims a served title must not satisfy "the harvest-backoff predicates", but it only checked the SCORING side — countUntitled and harvestNamedSomething, which record what a harvest achieved. The two the gate actually reads, untitledSettled and untitledFresh, decide whether a harvest starts at all, and repointing either at sessionTitleFor survived the suite. untitledSettled is precisely the regression the test is named for: a served-only row that never opens the gate is never re-harvested, whatever the scoring would have said. Both are asserted now, each killing its own mutant. The headline mutant the PR body claims — sessionTitle learning the fallback — was re-checked against the full suite and is still killed by that guard alone. The rest are comments the previous commit set out to fix and missed: - The harvest-wins test comment still carried the "richer … tiered" overstatement that commit retracted in three other places, and its "stable one" reasoning was backwards. The harvest is LAST-wins (core/observe/claude, every tier) and the served title is FIRST-wins, so the harvest lets the latest turn decide — which is what that comment faulted the proxy for doing with the first. Neither side is the steady one; they disagree about which turn should name a session. Reworded to sessionTitleFor's "fixed precedence, not a judgement". - titleIsBlank's "THE CELL DOES NOT CALL THIS" paragraph went stale in this PR: sessionTitleCell now reaches it through sessionTitleFor. Its worked example was also wrong — a harvested " " is no longer returned as " ", it answers blank and the cell shows the served title or "". Probed both to confirm before rewriting. - "like the other two consumers" had become three, the same stale-count problem this PR fixed by deleting titleIsBlank's caller list. Dropped the number. - Two "the better title" phrasings sat beside "not a judgement about which string is better", in sessions_pane.go and CLAUDE.md. Both now say "harvested". - The six rewritten test call sites passed a bare "" where noServedTitle says exactly that; the constant exists for them too. One review item is still open by design: core/session/store.go's "No consumer reads this yet" becomes false on merge. It is outside this PR's scope and has a follow-up issue. Verified: cmd/abctl/tui green plain and with -race, gofmt and go vet clean, go mod tidy -diff clean, and git diff against the base touches no core/ file. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
Member
Author
|
Both must-fix items are addressed in 93b5ebe, and both of your suggested guards were right about
The gate half of the backoff — folded into the regression guard, with your Worth flagging one process note: my first attempt at re-running the headline mutant was a no-op All the comment items are fixed too, including the one you caught as a factual error rather than On Also done: the stale "other two consumers" count (dropped rather than incremented, same reasoning as Still open by design: Not done, unchanged: no end-to-end run against a live proxy. Assisted-By: Claude (Anthropic AI) noreply@anthropic.com |
…lsified Three review findings, all about the served title reaching code whose comments predate it. THE CAP IS THE ONE BEHAVIOUR CHANGE. sessionTitleFor sanitised the served title but never bounded its length. The harvested title arrives capped at claude.MaxTitleLen and the cross-module cap test asserts that against a harvested fixture -- a path the served title never takes. The proxy's own cap is unexported in another module on purpose, and /v1/sessions is unauthenticated and operator-pointed, so "the producer caps it" was not an assertion this side could make. What that cost was not a malformed cell -- truncLeft/truncRight bound their output either way -- but the quadratic search inside them. Their fast path is disabled by any zero-width rune, and a served title keeps its combining marks, so, on ONE call: 2503 runes 72ms, 5003 287ms, 10003 1.12s, 20003 4.50s, 40003 17.55s -- and 200003 runes ran past a 10-MINUTE test timeout without finishing. A profile of that run names the cost centre: lipgloss.Width -> ansi.stringWidth -> displaywidth.lookup, re-measuring the whole remaining tail on every iteration. On the UI goroutine, per row per rebuild. Capped at the harvester's own constant so both sources share one budget. TWO COMMENTS STATED A PREMISE THAT IS NOW FALSE. truncLeft justified its skip-ahead guard with "a title reaching this file never contains a zero-width rune -- core/observe/claude drops every Mn/Me/Cf/Cc/Sk", and truncRight referred to it. True of a harvested title only: core/session.sanitizeTitle deliberately KEEPS combining marks, and pipeline.IsControlRune covers C0/C1/DEL/BIDI/Cf but not Mn/Me/Sk. So an accent or any ordinary emoji (U+FE0F is Mn, not Cf as first reported) runs the path the comment called unreachable. The guard is structural so behaviour was always correct -- but the false premise is what someone would delete the guard on. AND THE PRODUCER'S OWN DOC CONTRADICTED THIS PR. SessionSummary.Title still said "No consumer reads this yet". This adds that consumer, so it now records that abctl renders the field as a fallback, and which sessions it therefore decides the display for. Verified: the cap guard was confirmed to FAIL with the cap removed (both the accessor and the header assertion) and pass with it restored -- this package's mutation gate. gofmt, go vet and go mod tidy -diff clean on both modules; cmd/abctl/tui green plain and under -race. Still no end-to-end run against a live proxy. One pre-existing failure is unrelated and untouched by this branch: TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost fails identically on the base ref, from a CA bundle path in the local environment. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
esnible
force-pushed
the
feat/tui-served-title
branch
from
September 30, 2026 00:50
d27e6ee to
4df25f9
Compare
…s not asserted Six review items. One is a user-visible wording error, one is a dead test assertion, and the rest name limits the comments overstated. THE README WAS WRONG ABOUT WHEN A CELL IS EMPTY. It said "empty only when neither source names it", which the cached-only row contradicts: it passes noServedTitle unconditionally, so a session named ONLY by the proxy renders its title while listed and goes blank once the server stops listing it -- the rossoctl#870 scenario whose whole point is that the events are preserved. An operator watches the name disappear from a row that still has data. The row's own comment stated this correctly; only the README generalized from it. A TEST ASSERTED A LITERAL, NOT ITS FIXTURE. TestSessionsPane_ServedTitleOnlyFillsWhatRendersBlank built the model with newServedTitleModel and then passed "served name" directly, so it passed whether or not the helper had wired Title onto the summary -- leaving one other test as the only gate on that wiring. It now resolves through m.servedTitle, and a mutant that strips the helper's assignment fails three of its subtests. THE BIDI GAP IS NOW RECORDED. sanitizeLabel covers the BIDI overrides and isolates but NOT the plain marks (U+200E/200F/061C) or ZWJ; measured, all four survive it. core/session.sanitizeTitle folds them, so a served title is safe -- which makes this the second cross-package invariant this fallback leans on, after the length cap. Pinned as characterization that names the reliance, plus the contrasting override case, rather than by exporting sanitizeTitle from another module to test one line of rendering. AND TWO COMMENTS UNDERSOLD THEIR COSTS. "Until it finds one or the backoff caps out" reads transient, but for an agent with no transcript tree the harvest can NEVER succeed: the steady state is a full ~/.claude walk every 3 minutes for the process lifetime, on exactly the rows this feature serves. And noServedTitle's "cannot have one" now says it is a fact about today's structure -- nothing retains a last-seen served title -- rather than reading as settled. Two further items needed no change. The stale "core/observe/claude normalises every one" in truncLeft's ANSI argument was already fixed in 4df25f9, which names both normalisers. The positional-vs-by-id divergence between the row loop and sessionLabel needs duplicate ids, which an id-keyed store cannot produce. One suggestion declined: a sessionTitleForID(id) wrapper would put a scan of m.sessions behind the SHORTER name, and the live row loop calls it per row per rebuild -- making the O(n)-per-row call the default is the regression noServedTitle's doc exists to prevent. Verified: the rossoctl#8 fix confirmed to FAIL with the fixture wiring removed and pass with it restored. gofmt, go vet, go mod tidy -diff clean on both modules; cmd/abctl/tui green plain and under -race. Still no end-to-end run against a live proxy. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
…sified Five stale docs plus one live gap the served-title cap had made asymmetric. THE HARVESTED TITLE WAS CAPPED ONLY BY ITS WRITER. core/observe/claude emits at most MaxTitleLen runes, but LoadSessionMetadata re-reads that file and applies no cap, so a rewritten or hand-edited ~/.cortex/session-metadata.json reached the same quadratic truncation the served cap was just added to prevent. Measured: a 10003-rune path-shaped title with combining marks made ONE rebuildSessionsTable take 1.11s on the UI goroutine. Pre-existing, and out of this PR's path -- but capping one source and not the other left them asymmetric for no reason, and it is the same constant. Capped in sessionTitle, the accessor every consumer reads, so one line covers the cell, the headers and sessionHasTitle. Safe for the backoff by construction: truncation cannot turn a non-blank title blank, so no named/unnamed verdict moves, and the guard asserts that alongside the length. FOUR DOCS CLAIMED THE HARVEST WAS THE ONLY WAY TO NAME A SESSION. - app.go's sessionsData field: "what an agent knows about its own sessions that the proxy does not" and "renders as an empty TITLE column" -- both false now. The same file states it correctly at the harvest-repaint site, which this PR added, so the field doc was the outlier in a file already edited here. - LoadSessionMetadata: a total load failure no longer blanks the column for a proxy-named session. Worth saying, because it makes the deliberate silence on a corrupt file easier to justify rather than harder. - README's --skip-claude-metadata opening: presented the harvest as the only route, contradicting the same section's own fallback description further down. Now names both and scopes the section to the harvest, including that the flag suppresses this route only. Verified: the new cap confirmed to FAIL with it removed and pass with it restored. The full cmd/abctl/tui suite is green after capping a path every title flows through -- the real risk here, since existing title tests read the same accessor. gofmt, go vet, go mod tidy -diff clean on both modules; green plain and under -race. Still no end-to-end run against a live proxy. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
Three defects, and the first was mine: the cap I added last commit came with a false claim and a guard that asserted it with a fixture that could not test it. THE CAP COULD FLIP A ROW FROM NAMED TO UNNAMED. "Truncation cannot turn a non-blank title blank" is false. A harvested title whose first MaxTitleLen runes are whitespace, with real text after them, clips to pure spaces -- which titleIsBlank calls blank, so sessionHasTitle returns false and the row re-harvests ~/.claude every 3 minutes for the life of the process. That is the permanent-rescan cost this package documents as the price of an UNNAMABLE session, charged instead to a session with a perfectly good name. Reproduced before fixing: sessionHasTitle was false for exactly that input. Fixed by trimming after the cut, mirroring clipTitle in core/observe/claude, which does the same thing for the same reason. Trimming cannot introduce the failure it prevents -- it only removes whitespace, so a clip still holding text is untouched and one holding nothing else collapses to "", which is the honest answer and one sessionHasTitle already handles. THE GUARD ASSERTED THAT INVARIANT WITH A LEADING-"/" PATH, where no prefix is whitespace, so it passed for the wrong reason. Now three cases: the over-long performance one, whitespace filling the whole clip window (must read unnamed), and a whitespace prefix with text inside the window (must still read named). It also asserts the result is trimmed, so the verdict is deliberate rather than incidental. A mutant dropping TrimSpace fails two of the three. AND A WHITESPACE-ONLY SERVED TITLE PAINTED SPACES INTO THE CELL. titleIsBlank guarded the harvested title and not the served one, so sessionTitleFor returned " " verbatim while sessionLabel -- which blank-checks what sessionTitleFor returns -- rendered the bare id. One accessor, two callers, two different names for one session. Guarded before the cap. One note on the new test's fixtures: it covers spaces and the exotic spaces (U+00A0, U+3000), NOT tab or newline. Written first with "\t" it failed, and the test was wrong rather than the code -- titleIsBlank sanitises before it trims, so a tab becomes a visible U+FFFD glyph and is a real title. That order is already pinned from the harvested side, and the two tests must not contradict each other. Verified: both fixes independently mutation-checked -- dropping TrimSpace fails the harvested test, removing the blank-served guard fails all four served cases. gofmt, go vet, go mod tidy -diff clean on both modules; cmd/abctl/tui green plain and under -race. Still no end-to-end run against a live proxy. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
…t two overstated claims Ten review items. Four changed behaviour; six were claims the code did not support. Each verified against the source or by probe before being accepted. THE SANITISER WAS RELYING ON THE PRODUCER IT SAID IT DID NOT. sessionTitleFor's doc claimed "this does not rely on [the producer]" while sanitizeLabel replaced only the BIDI overrides and isolates (U+202A-202E, U+2066-2069). The plain marks U+200E/200F/061C and the zero-widths U+200B/200C/200D/2060/FEFF passed through untouched, so the only thing keeping a mark out of a cell was core/session's sanitizeTitle — unexported, in another module, reached over an unauthenticated API. That is the rune class whose whole purpose is to make the rendered order differ from the byte order, and the one this side must not delegate. sanitizeLabel now delegates to pipeline.IsControlRune, the repo's single copy of the rule. Both sources are covered: the metadata file on disk is not a trusted input either. THE CAP COULD SEVER A GRAPHEME CLUSTER. The cap mirrored clipTitle's TrimSpace while silently dropping its precondition — clipTitle's own doc says a plain rune cut is safe for it ONLY BECAUSE normalizeTitle removed every binding character first, and neither string this cap sees has been through that (core/session's sanitizeTitle keeps combining marks "so café survives"). A cut at MaxTitleLen could leave a dangling accent, half a ZWJ emoji, or one regional indicator of a flag. capTitleRunes now walks back off Mn/Me/Mc, ZWJ and regional indicators. Deliberately not Lm: Grapheme_Extend excludes modifier letters, and Lm also holds runes that legitimately start a cluster (U+02BB okina). A fixture built on U+02B0 is what surfaced that — the fixture was wrong, not the code. ALLOCATION-FREE ON THE COMMON PATH. len([]rune(x)) allocated a full rune slice just to compare a length, ~3-4x per row per 2s tick: 160 B/op and 232ns for a 40-rune title against 0 B/op and 155ns with utf8.RuneCountInString. Measured trade, noted in the comment: counting first walks the string twice, so the over-long path is ~45% slower (780us vs 1.13ms at 200k runes). The short path is the one that runs constantly. THE CAP TESTS PASSED FOR A BYTE-BASED CAP. Both asserted <= MaxTitleLen, which a byte cap satisfies while halving the budget of a two-byte-per-rune title. Now ==, and a genuine byte cap fails it. The fixtures also had no guard against NFC normalisation: a precomposed U+00E9 is one Mn-free rune, takes the truncation FAST path, and would leave both tests passing while exercising nothing. Shared combiningMarkRune + assertFixtureIsSlowPath, which the served test guarded inline and the harvested test did not guard at all. Docs corrected, each falsified by this PR or by the one before it: - "Capping the input is what makes the render cost flat" overstated it. The cap removes the quadratic term; sanitizeLabel's b.Grow(len(s)) and the rune count stay linear in the untruncated input, ~1.6MB transient per row per rebuild at 200k runes. Linear is the difference between laggy and unusable, but "flat" was wrong, and the honest bound is what a reader needs when deciding whether to cap earlier at the decode. - sessionTitleFor's two arguments have no coupling, so a future caller can pair one session's id with another's title and render a confident wrong name — the worst failure this column has, since a title is what an operator reads before acting on a row. Both live callers resolve served from the same id; that is now stated as the contract a third must keep. - --skip-claude-metadata's comment said it leaves only "whatever titles the metadata file already held". Served titles still render; the flag declines a filesystem scan, not naming. - Its flag help said a bare id "means neither source named it" — not exact for a cached-only row, whose served title is not retained and goes away with the listing. - README said the worst a missing metadata file costs is the TITLE column, which is the claim LoadSessionMetadata's own doc retracts four lines above. One characterization test deleted rather than inverted, as its doc instructed: it recorded the BIDI-mark gap as a deliberate reliance on core/session, and that reliance is what this commit removes. Sixteen new test functions against the base ref. Four mutations, each confirmed to fail with the fix reverted: sanitizeLabel narrowed to its old set (fails on both sources), a genuine byte cap, the cluster walk removed (all six binder cases plus the degenerate all-marks case), and the fixture constant normalised to U+00E9 (fails both cap tests). Still not run end-to-end against a live proxy: restarting the shared local Cortex cuts every attached session. These commits touch sessionTitle, the accessor every title in the UI flows through, so that gap is worth weighing. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
Two behavioural fixes and five gaps review found in the round-5 commit. capTitleRunes walked back over EVERY regional indicator, with no pairing logic. They pair left-to-right into flags, so in a run only every second one binds, and treating all of them as binders walked to index 0 across the run: 41 consecutive flags capped to "", and "a"*70 + 10 flags lost 11 runes rather than the "rune or two" the function's own doc promised. That is not cosmetic, because sessionHasTitle reads sessionTitle: an emptied title flips a named row to unnamed and restarts the permanent ~3-minute re-harvest -- the same failure f5a0a61 fixed for whitespace, reached by a second route. riBindsAtCut resolves the pairing by counting the run that PRECEDES the cut, so the scan is bounded by that run and the loop still takes at most one step. sessionTitleFor sanitised the untruncated served string twice -- titleIsBlank(served) built and discarded a full copy, then the next line sanitised again for the cap. sanitizeLabel is idempotent, so the second pass was pure waste: ~3.2MB of transient allocation per row per rebuild at 200k runes where the documented bound said ~1.6MB. It now sanitises once into a local and blank-checks that through a new blankSanitized, which is titleIsBlank's trim half split out. Two call sites in session_metadata.go were in the same position and now say so. The sanitise-before-trim ORDER titleIsBlank documents is preserved, not dropped, and blankSanitized's doc says why handing it a raw title reintroduces the hazard that ordering prevents. Tests and docs: - The existing cap test asserted the walk-to-zero behaviour on 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 -- the test encoded the defect. Removed, with a comment on why the assertion was wrong. - Three new tests: the RI parity cases, a flag-only title staying named through sessionHasTitle and countUntitled, and modifier letters (Lm, including U+02BB okina) not binding -- the "DELIBERATELY NOT Lm" exclusion had no gate, so adding Lm and Sk kept the suite green. - A fourth pins the zeroWidthFree (Mn/Me/Cf/Cc/Sk) versus bindsToPrevious (Mn/Me/Mc) divergence, which was unpinned: the two predicates answer different questions and unifying their category sets is wrong in both directions. - The served-title sanitise-before-cap ordering is now gated at a ZWJ cut; it survived the whole suite before. - Two tests built a nil served map and called sessionTitleFor directly, so they contributed nothing to servedTitle's coverage; both now route through m.servedTitle. One header assertion used the at-most form the same test argues against five lines earlier, and one blankness assertion trimmed before comparing -- the exact defect titleIsBlank exists to prevent. - untitledSettled and untitledFresh still claimed the steady state is "every row titled" and costs nothing. sessionHasTitle answers false for served-only rows, and on an agent with no transcript tree the harvest can never succeed, so that state is permanent rather than transient. Both docs now say so. - bindsToPrevious records that its ZWJ arm is unreachable from both production callers today, and that 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, not clusters. - Rewrapped three over-wide lines (one Go doc line, two README). cmd/abctl/tui green plain and with -race; gofmt and GOWORK=off go vet clean on every changed file; go mod tidy -diff clean. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
abctl's TITLE column came only from harvested Claude Code transcripts, so a session with
no transcript tree on the operator's disk rendered blank. #1167 added a server-derived
titleto/v1/sessionsand nothing read it. Now the display falls back to it.On the laptop this was found on, every blank harvested entry belonged to an agent that has
no Claude Code transcript tree — but does route through the proxy, so a served title
existed for exactly those rows. The two sources are complementary rather than redundant:
harvesting names what the proxy never saw, the proxy names what the filesystem cannot.
Harvest wins when both exist — a fixed precedence, not a judgement about which string is
better. Neither side is the "stable" one: the harvest is LAST-wins (every tier in
core/observe/claude) and the served title is FIRST-wins, so they disagree about which turn shouldname a session rather than one being steadier. The two rankings do not agree on every session, and
an explicit
/renamelosing to a bare cwd is the clearest case where the loser looks better;accepted for now pending what operators report, and one line to invert. No provenance indicator —
the cell shows a title, not where it came from.
Scope: abctl, plus two comments in
core/session. An earlier revision carried anunrelated comment fix there and it was dropped. What is in the diff now is not that: review
found
SessionSummary.Title's own doc still said "No consumer reads this yet", which thisPR falsifies, so
core/session/store.gocarries a comment-only change to the field thisfeature consumes. No code outside
cmd/abctlchanges.The load-bearing part: the fallback is display-only
sessionTitlefeedssessionHasTitle, and through it every harvest-backoff predicate(
untitledSettled,untitledFresh,countUntitled,harvestNamedSomething);countUntitledin turn feeds both the harvest gate and its scoring. Folding the servedtitle into
sessionTitlewould make the row read as named, zerountitledMissesandstop the re-harvest permanently — settling for whichever title the proxy derived first
and never looking for the harvested one.
So
sessionTitleandsessionHasTitleare unchanged, and a newsessionTitleForappliesthe precedence for the cell and the three headers. A server-titled row therefore still
reads as unnamed to the backoff and keeps being re-harvested; that periodic transcript
scan is the intended trade, not a leak.
Two details worth keeping:
titleIsBlank, not== "", so a harvested" "(which paintsnothing) falls back while a harvested
"\t"(whichsanitizeLabelturns into a visibleglyph) does not — overriding a title already on screen would be the worse bug.
sanitizeLabel'd like the harvested one./v1/sessionsisunauthenticated and the title is folded from caller-supplied event content, so
sessionTitle's CWE-150 reasoning applies to it at least as much. The producer alreadytrims and caps; this does not rely on that — and review caught that the claim was not
true when first written. See below.
Two things review found that the earlier commits got wrong
The sanitiser was relying on the producer it said it did not.
sanitizeLabelreplacedonly the BIDI overrides and isolates (U+202A-202E, U+2066-2069). The plain marks
U+200E/200F/061C and the zero-widths U+200B/200C/200D/2060/FEFF went through untouched, so
the only thing keeping a mark out of a cell was
core/session'ssanitizeTitle—unexported, in another module, reached over an unauthenticated API. That is precisely the
rune class whose purpose is to make rendered order differ from byte order, and the one this
side must not delegate. It now delegates to
pipeline.IsControlRune, the repo's single copyof the rule, whose own doc records that three byte-identical duplicates once drifted under a
comment asserting they moved together. Both sources are covered: the metadata file on disk is
not a trusted input either.
The cap walked back over every regional indicator. They pair left-to-right into flags, so
in a run only every second one binds — and treating all of them as binders walked to index 0
across the run. Measured: 41 consecutive flags capped to
"", and"a"*70+ 10 flags lost 11runes rather than the "rune or two" the function's own doc promised. Since
sessionHasTitlereads
sessionTitle, an emptied title flips a named row to unnamed and restarts the permanent~3-minute re-harvest — the same failure
f5a0a615fixed for whitespace, reached by a secondroute. Binding is position-dependent, not rune-intrinsic, so a per-rune predicate cannot
answer it:
riBindsAtCutcounts the run that precedes the cut, so the scan is bounded by thatrun and the loop still takes at most one step. The existing cap test had asserted the
walk-to-zero behaviour on a lone RI — one RI after ordinary text starts a flag rather than
completing one, so that cut was already on a boundary and the test encoded the defect.
sanitizeLabelran twice over the untruncated served string.titleIsBlank(served)builtand discarded a full copy, then the next line sanitised again for the cap — ~3.2MB of transient
allocation per row per rebuild at 200k runes where the documented bound said ~1.6MB. Idempotent
made it harmless, not free. It now sanitises once into a local and blank-checks that through a
new
blankSanitized, which istitleIsBlank's trim half split out; twosession_metadata.gocall sites were in the same position and now say so. The sanitise-before-trim order that
titleIsBlankdocuments is preserved rather than dropped, andblankSanitized's doc records whyhanding it a raw title reintroduces the hazard that ordering prevents.
The cap could sever a grapheme cluster. The cap mirrored
clipTitle'sTrimSpacewhiledropping its precondition —
clipTitle's doc says a plain rune cut is safe for it onlybecause
normalizeTitlestripped every binding character first, and neither string this capsees has been through that (
core/session'ssanitizeTitledeliberately keeps combiningmarks "so café survives"). A cut at
MaxTitleLencould leave a dangling accent, half a ZWJemoji, or one regional indicator of a flag.
capTitleRunesnow walks back off Mn/Me/Mc, ZWJand regional indicators — deliberately not
Lm, which Grapheme_Extend excludes and whichholds runes that legitimately start a cluster (U+02BB ʻokina). A fixture built on U+02B0 is
what surfaced that; the fixture was the wrong part, not the code.
Both cap sites are now one helper, which is also where the
len([]rune(x))allocation went:counting with
utf8.RuneCountInStringtakes the common path from 160 B/op and 232ns to 0 B/opand 155ns for a 40-rune title, several times per row per 2s tick. The measured counter-trade,
recorded in the comment: counting first walks the string twice, so the over-long path is ~45%
slower (780µs vs 1.13ms at 200k runes). The short path is the one that runs constantly.
Review also found four claims in the code that no test would have caught being falsified —
the
Lmexclusion, the sanitise-before-cap ordering, the served-title path throughservedTitle, and thezeroWidthFree/bindsToPreviousdivergence. Each now has a gate. Twoassertions were themselves too weak to fail: one header check used the at-most form the same
test argues against five lines earlier, and one blankness check trimmed before comparing, which
is the exact defect
titleIsBlankexists to prevent.untitledSettledanduntitledFreshstill described the steady state as "every row titled",costing nothing.
sessionHasTitleanswers false for served-only rows by design, and on an agentwith no transcript tree the harvest can never succeed — so that state is permanent, not
transient, and the per-row-per-tick allocation is paid for as long as the pane is open (bounded
by the ~3m backoff cap rather than by ever being satisfied). That is the accepted price of not
letting a served title stop the search for the harvested one, and both docs now say so. It is
the same class of stale claim this PR set out to fix.
Four comments were corrected because this PR or the one before it falsified them, including
one of my own overstatements — "capping the input is what makes the render cost flat" claimed
more than the code does. The cap removes the quadratic term;
sanitizeLabel'sb.Grow(len(s))and the rune count stay linear in the untruncated input, ~1.6MB transient per row per
rebuild at 200k runes. Linear is the difference between laggy and unusable, but "flat" was
wrong, and the honest bound is what a reader needs in order to decide whether to cap earlier at
the decode instead.
Docs
The README stated the old behavior in the two places a reader looks first — the Sessions
pane column list and the
--skip-claude-metadatasection — and both now describe whatfills the column and which source wins.
CLAUDE.md's/v1/sessionsrow is updated thesame way.
One transient shape, documented rather than changed
A session that arrives on the event stream before a list refresh gets a stub
SessionSummarywith a zeroTitle, so its row shows no served title for up to twoseconds. It is the one display path where an empty served string does not mean "the proxy
derived none", and it self-corrects on the next poll. Noted in
sessionTitleFor's doc.Cached-only rows are the other empty-by-construction case: they exist because the server
stopped listing the session, so there is no summary to carry a title.
Tests
Twenty-one new test functions against the base ref (76 vs 55), and the one that matters most
is still the backoff guard — it is the only one that fails when the fallback is moved into
sessionTitle, because the rendered cell is correct either way.Sixteen mutations checked by hand, each dying against the test written for it: fallback
moved into
sessionTitle, precedence swapped, trigger weakened to== "",sanitizeLabeldropped,
sessionLabelreverted to harvest-only, the served-title cap removed (failing boththe accessor and the header assertion), the harvested-title cap removed,
newServedTitleModel's fixture wiring stripped — that last one is why a test that had beenasserting a literal instead of its fixture is now a gate at all — plus four from the latest
round:
sanitizeLabelnarrowed back to overrides-and-isolates only (fails ZWJ, WJ, BOM andLRM on both sources), a genuine byte-based cap (fails the
==rune assertion that the old<=let through), the grapheme-cluster walk removed (fails all six binder cases and thedegenerate all-marks case), and the shared fixture constant normalised to a precomposed
U+00E9 (fails both cap tests via
assertFixtureIsSlowPath, which is the point of having it).Three more from the latest round:
sanitizeLabeldropped fromsessionTitleFornow fails 13tests rather than the 2 it did before the fixtures were rewired;
titleIsBlank's orderingreversed to sanitise-after-trim; and
zeroWidthFree's category set unified withbindsToPrevious's — the tempting "these two are near-duplicates" refactor, which nothingcaught until the new pin. The two predicates answer different questions (is a rune count wrong
about this string's width? vs would cutting before this rune orphan it?) and unifying them is
wrong in both directions: it makes a
Mc-terminated title take the slow width path for nothing,and makes the cap walk back off an
Skthat starts nothing.Two fixture guards, because a cap test is easy to write so that it exercises nothing:
assertFixtureIsSlowPathfails loudly if the string it is handed would taketruncLeft'sfast path or is already under the cap. The served test had that check inline; the harvested
test had none.
One test is explicitly not a mutation gate, and now says so in its own comment:
TestSessionsPane_CachedOnlyRowTakesNoServedTitlecannot fail if the cached-only loop ischanged to look the title up itself, because
cachedOnlySessionIDsexcludes every listedid, so the lookup can only return
"". Verified by running that mutation — the suitestays green. It is kept for the cross-talk check and the worked example, not as a gate.
One test was deleted rather than inverted, as its own doc instructed:
TestSessionsPane_ServedTitleBidiMarksRelyOnTheProducerrecorded the BIDI-mark gap as adeliberate reliance on
core/session's sanitiser. Closing that gap is what the latestcommit does, so the characterization no longer describes the code. Its replacement,
TestSessionsPane_ServedTitleStripsEveryControlClass, walks all ten classes (LRM, RLM, ALM,ZWSP, ZWNJ, ZWJ, WJ, BOM, RLO, LRI) and asserts the rune is gone, U+FFFD is present, and the
surrounding text survives — and its harvested twin does the same for the file on disk, which
is not a trusted input either.
cmd/abctl/tuigreen plain and with-race;gofmt -landGOWORK=off go vetclean;go mod tidy -diffclean.Not done
served title) has not been confirmed on screen.
cmd/abctlfailure,TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost,predates this branch — it wants a local CA bundle this machine has not got — and was
confirmed failing on the untouched base.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit