Feat: Show the served session title in abctl when harvesting names nothing - #1192
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 (9)
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 a live proxy title when transcript harvesting has no title, while preferring harvested titles when available. It sanitizes and caps displayed titles. Documentation describes title precedence, fallback behavior, and the effect of skipping or failing transcript harvesting. ChangesSession title display
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change shows the proxy-served session title when no harvested title exists, and it sanitizes and caps displayed titles. No merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The fallback sanitizes and limits displayed titles while retaining session IDs as the authoritative identity. Served titles do not satisfy harvested-title checks. No material security regression was established in these paths, but response-size containment and deployment access restrictions are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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 |
04bc000 to
bc55d2e
Compare
…thing 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) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
bc55d2e to
cd04e69
Compare
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Adds a fallback so abctl's TITLE column and the Enter header use the /v1/sessions title when no transcript title was harvested. The backoff stays harvest-only. Tests pass locally, and the new tests fail on 9 of 10 targeted mutations; the one that survives removes a branch that can never change the result (see the inline comment on if blankSanitized(clean)).
The main suggestion: rivo/uniseg is already in the module graph and could replace the hand-written grapheme-boundary walk. The rest are comment cleanups and test nits, none blocking.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go, Docs
Agent/IDE config (.claude/.vscode): none
Commits: 1, signed off: yes
CI status: passing
| // 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, |
There was a problem hiding this comment.
suggestion: The comment says abctl has no grapheme-segmentation library. It does: github.com/rivo/uniseg v0.4.7 is already in cmd/abctl/go.mod, pulled in indirectly by go-runewidth and x/ansi. Using uniseg.FirstGraphemeClusterInString (or a Graphemes iterator) up to MaxTitleLen could replace clusterStart, riBindsAtCut and bindsToPrevious, plus most of the comments around them. It would also cover cases the Mn/Me/Mc approximation misses: Hangul jamo, emoji skin-tone modifiers, and SpacingMark characters outside Mc. Making it a direct dependency is a one-line change to go.mod.
| // | ||
| // 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 |
There was a problem hiding this comment.
suggestion: f5a0a615 isn't on main (compare against main reports diverged), so it's an earlier version of this PR's branch and will point nowhere once this is squashed. The test file cites it too. More broadly, a lot of the new comments tell the review history ("wrong twice over", "Review caught…", "an earlier version of this comment claimed…"). That history belongs in the commit message; in the code it goes stale. Could the comments state the current rule and why, and move the history into the commit message?
| // 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) { |
There was a problem hiding this comment.
nit: This branch never changes the result. capTitleRunes already calls TrimSpace first, so a whitespace-only clean comes back as "" without this check. Deleting the branch (replacing the condition with false) leaves every test green. Either remove it, or cut the comment above it to one line saying it's a fast path.
| // 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)) { |
There was a problem hiding this comment.
nit: When the harvested title isn't blank, it gets sanitized twice: once here, then again inside m.sessionTitle(id). The "TWO IS THE FLOOR" comment says a second sanitize must never come back, but it's here on the harvested side. The cost is small because harvested titles are normally 80 runes or fewer. A hand-edited metadata file, which sessionTitle's comment mentions, would pay it on the full string.
| if unicode.In(r, unicode.Mn, unicode.Me, unicode.Mc) { | ||
| return true | ||
| } | ||
| return r == '' || (r >= 0x1F1E6 && r <= 0x1F1FF) |
There was a problem hiding this comment.
nit: '' is a literal zero-width joiner, invisible in most editors and diffs. Use '\u200D'. The test file has a few raw invisible literals too, and its own comment on combiningMarkRune warns about exactly this.
| 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 |
There was a problem hiding this comment.
nit: Go comments don't process escapes, so \u2014 here (and a few lines below) shows up as the literal text \u2014. Use a real — like the rest of the file.
| // "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 |
There was a problem hiding this comment.
nit: There's no blank // line between paragraphs here: "…does not grow with the title. On every shape…" runs straight into "A LEADING DEGENERATE CLUSTER…". The same happens before "served IS CAPPED HERE" in sessionTitleFor's comment.
| for _, tc := range []struct { | ||
| name string | ||
| title string | ||
| wantNamed bool |
There was a problem hiding this comment.
nit: wantNamed is true in every row, so the != tc.wantNamed check never tests the "unnamed" answer. Add a row that should come back unnamed, or drop the field.
| if len(gr) == 0 { | ||
| t.Fatalf("capTitleRunes returned empty for %q", title) | ||
| } | ||
| if last := gr[len(gr)-1]; last == []rune(tc.binder)[0] { |
There was a problem hiding this comment.
nit: This last == binder check can never fail: r[:cut] never includes r[cut], so the result can't end in the binder. The HasSuffix(got, "e") check below is the one doing the work.
2 participants
What
abctl's TITLE column is filled by harvesting Claude Code transcripts off the operator's
disk. A session with no transcript there renders blank — on the laptop this was found on,
26 of 324 rows, all belonging to an agent that has no transcript tree but does route
through the proxy.
/v1/sessionsalready publishes atitlethe proxy derives from the session's own events,and nothing read it. Now the TITLE column falls back to it, as does the header shown on
Enter.
Harvest wins when both sources have a title.
The one design constraint
The fallback is display-only.
sessionTitlefeedssessionHasTitle, which drives theharvest-backoff predicates, so folding the served title in there would make the row read as
named and permanently stop re-harvesting — settling for whichever title the proxy derived
first. So
sessionTitleandsessionHasTitleare untouched and a newsessionTitleForapplies the precedence for display only. A served-titled row still reads as unnamed to the
backoff and keeps being re-harvested; that is the intended trade.
Two smaller details: the fallback triggers on
titleIsBlankrather than== "", so aharvested
" "(which paints nothing) falls back while a harvested"\t"(which paints aglyph) does not; and the served string is sanitised and length-capped like the harvested
one, since
/v1/sessionsis unauthenticated and the title is folded from caller-suppliedcontent.
The length cap is the same constraint reached again, and most of the care in this diff is
there. It must never return
""for a title that renders something, because an emptiedtitle reads as unnamed and restarts the re-harvest. Two things that took:
the rune budget — otherwise 80 leading spaces push the real text past the cut and the trim
then empties what is left.
a plain cut could sever a cluster; the cap walks back to that cluster's start. The one
shape with no earlier boundary (a title opening with combining marks, or an odd flag half)
is cut bluntly rather than emptied.
Precedence is decided on the raw harvested title rather than the capped one, so a display
bound cannot silently hand a named row to the other source.
Verification
cmd/abctl/tuigreen plain and with-race;gofmt -landGOWORK=off go vetclean;go mod tidy -diffclean.Confirmed end-to-end against a live proxy with a base-ref control: a session bucketed by
X-Task-Id(no transcript to harvest) rendersWhy did the flag emoji title cap to an empty string?on this branch and blank on the base ref, from the same proxy. An A2A-onlysession in the same run stays blank on both, so the fallback is per-row rather than a
blanket substitution.
One pre-existing failure is unrelated:
TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLostwants a local CA bundle this machine has not got, and fails identically on the untouched
base.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com