From bb0147a47c62d6e1bf5c6965dfb562fccd6e80f5 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Tue, 22 Sep 2026 16:17:52 -0400 Subject: [PATCH 01/16] feat(abctl): Name sessions from agent-name lines and user prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit titleFromTranscript read only `ai-title` lines, so most sessions fell back to their working directory: on one local tree, 18 of 128 had a real title and 110 showed a path. Paths are shared between sessions, so those rows were indistinguishable from each other. Three changes: - Accept `{"type":"agent-name","agentName":…}` as a title alongside `ai-title`. Some installs write one, some the other. The byte prefilter has to admit the new key too, or the line is skipped before it is parsed. - Fall back to the user's last prompt when neither title line is present, preferring turns Claude Code attributes with `"origin":{"kind":"human"}` and string content over content arrays. Turns marked `task-notification` or `peer` are dropped, as are tool_result arrays and the harness's own bracketed blocks — those are tool output and injected text, not anything typed. A slash command is rendered as the line the user typed rather than as its `` envelope, and a `` wrapper is stripped from what the user pasted inside it. - Clip prompt titles to 80 runes and collapse whitespace. A prompt is unbounded free text; a title is a table cell, and the viewer renders control characters as U+FFFD rather than dropping them. The cwd fallback is NOT clipped: a path's distinguishing end is its leaf, so clipping made sibling worktrees under a long prefix identical. On the same tree this takes the count from 18 named to 124, with 5 sessions left on the directory fallback because they contain no human turn. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 346 ++++++++++- .../authlib/observe/claude/harvest_test.go | 567 ++++++++++++++++++ 2 files changed, 908 insertions(+), 5 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 397153254..31f5a4fd8 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -407,6 +407,12 @@ func harvestedAt(m SessionMetadata, want string) time.Time { // directory it ran in, otherwise "". Both are LAST-wins: a session can be titled more // than once and can change directory, and the newest claim is the current one. // +// TWO LINE KINDS CARRY A TITLE — {"type":"ai-title","aiTitle":…} and +// {"type":"agent-name","agentName":…} — and both feed the same last-wins value. Which one an +// install writes varies: one local config dir had agent-name in 16 transcripts and ai-title in +// only 2, with no overlap, so reading ai-title alone left most of those sessions named by their +// working directory instead of their real title. +// // Returns "" rather than an error for an unreadable or malformed transcript. A // missing title costs a label; refusing the whole harvest over one bad file would // cost every other name. That is the opposite of toolscan's choice on the same files, @@ -433,13 +439,26 @@ func titleFromTranscript(path string) (string, error) { // toolscan.scanFile. sc.Buffer(make([]byte, 0, 256*1024), 16*1024*1024) - var title, cwd string + // THREE GRADES OF PROMPT, best first. Each is last-wins within its own grade, so a later turn + // of the same quality replaces an earlier one but never a better one. + // + // human — origin.kind == "human" AND string content. What the person typed, stated by + // Claude Code rather than inferred. 640 such turns across 128 measured transcripts, + // every one of them a string. + // str — string content with no origin field, the shape older transcripts use. Still a + // prompt in practice, just unattributed. + // blocks — text extracted from a content ARRAY. Last resort: these are where the harness + // injects, and a "Base directory for this skill: …" title came from one. + var title, cwd, human, str, blocks string for sc.Scan() { line := sc.Bytes() // Hot path: most lines are conversation turns carrying neither field. A // substring test over the raw bytes is far cheaper than parsing them, and // bytes.Contains avoids the copy that strings.Contains(string(line), …) makes. - if !bytes.Contains(line, []byte(`"aiTitle"`)) && !bytes.Contains(line, []byte(`"cwd"`)) { + if !bytes.Contains(line, []byte(`"aiTitle"`)) && + !bytes.Contains(line, []byte(`"agentName"`)) && + !bytes.Contains(line, []byte(`"cwd"`)) && + !bytes.Contains(line, []byte(`"role":"user"`)) { continue } var e transcriptMeta @@ -454,6 +473,58 @@ func titleFromTranscript(path string) (string, error) { if e.Type == "ai-title" && e.AiTitle != "" { title = e.AiTitle } + // The other spelling, treated as equal rather than as a fallback. Both feed the same + // last-wins `title`, so whichever line appears later in the transcript is the current + // name — the same rule that already applies between two ai-title lines. Where a + // transcript carries both kinds they agree anyway (measured: 16 of 16), so ordering + // them against each other would be inventing a distinction the data does not have. + if e.Type == "agent-name" && e.AgentName != "" { + title = e.AgentName + } + // LAST-WINS on the user's own prompts, kept as a third tier below both title kinds. Only + // consulted when neither fired, so a titled session is never renamed by its transcript. + // + // Guarded on both the line type and the message role: an assistant turn is not a prompt, + // and `"role"` appears on every turn either way. Tool output and Claude Code's own + // bracketed markers are filtered by the two helpers rather than here, so this stays a + // statement about WHICH turn counts. + if e.Type == "user" && e.Message != nil && e.Message.Role == "user" { + kind := "" + if e.Origin != nil { + kind = e.Origin.Kind + } + text, wasString := promptFromMessage(e.Message.Content) + switch { + case text == "": + // Nothing typed: a tool_result array, or blocks with no text. + case kind == "human" && wasString: + // ATTRIBUTED, so the text is taken as-is. No synthetic-prompt filter here: a + // slash command the user really typed arrives as + // "review…", which the structural filter + // would reject — and origin already settles authorship, so guessing from the + // text would only overrule better evidence. The envelopes a typed turn arrives in + // are unwrapped rather than filtered, for the same reason: the user did type it, + // so the answer is to render what they typed rather than to drop the turn. + // ORDER MATTERS. The command envelope is read FIRST: it is a multi-tag structure + // whose sit past the first tag, so stripping a leading wrapper + // beforehand threw the arguments away and left a bare "/review". Only a turn that + // is not a command envelope reaches the wrapper strip. + human = unwrapCommandEnvelope(text) + if human == text { + human = stripWrapperTag(text) + } + case kind != "" && kind != "human": + // Explicitly NOT human — "task-notification" or "peer". Discarded outright; + // this is the traffic the text heuristics existed to catch. + case isSyntheticPrompt(text): + // Unattributed and looks machine-written. Still filtered, because transcripts + // with no origin field at all have nothing better to go on. + case wasString: + str = text + default: + blocks = text + } + } } // Reported, not discarded. A truncated read still yields whatever was found before the // stop — which is why the title is returned alongside the error rather than dropped — @@ -461,10 +532,239 @@ func titleFromTranscript(path string) (string, error) { // last-wins is the rule here, so the name returned may be an old one. The buffer above // makes this rare; silence made it invisible. err = sc.Err() - if title != "" { + // THREE TIERS, in descending confidence: a title Claude Code generated, then the last thing + // the user actually typed, then the directory the session ran in. The prompt tier is what + // takes a tree from "mostly paths" to "mostly readable" — measured on one config dir, 110 of + // 128 sessions had no title line of either kind and fell through to a cwd, and every one of + // those has a usable prompt. + // + // Clipped at the source. A prompt is unbounded and a title is a table cell; see maxTitleLen. + // NORMALISED BEFORE the switch, not inside each arm. A whitespace-only candidate passes a + // bare `!= ""` and clipTitle then empties it, so the tier below was skipped and the cell came + // out blank — testing the clipped value is what makes each guard mean "this tier has + // something to show". + title, human, str = clipTitle(title), clipTitle(human), clipTitle(str) + blocks = clipTitle(blocks) + switch { + case title != "": return title, err + case human != "": + return human, err + case str != "": + return str, err + case blocks != "": + return blocks, err + } + // THE CWD IS NOT CLIPPED. A path's distinguishing end is its LEAF, and clipping keeps the + // head: two sibling worktrees under a prefix of 80 runes or more clip to byte-identical + // titles, so the column stops telling them apart — exactly what it is for. Clipping the cwd + // was also a regression against the behaviour before this change, which never truncated here. + // + // Safe to leave long because the renderer already truncates a path FROM THE LEFT, keeping the + // tail (see the viewer's truncLeft): the cap exists to bound prompts, which are unbounded free + // text, not paths, which are bounded by the filesystem. Whitespace is still collapsed, so a + // cwd cannot carry a control character into a cell. + return strings.Join(strings.Fields(cwd), " "), err +} + +// maxTitleLen caps a harvested title, in RUNES. +// +// A prompt is unbounded — the longest on the measured tree ran to several KB — and a title is a +// table cell. Clipping at the source keeps the metadata file small and stops every consumer having +// to defend itself; the viewer truncates again to whatever the column allows, which is narrower +// still. Runes, not bytes, so a multi-byte prompt is not cut mid-character. +// +// NOT A DISPLAY-COLUMN BUDGET. 80 runes of CJK occupy 160 columns, so nothing may treat this as a +// width. It is safe only because every renderer re-truncates by display width — the sessions +// pane measures with lipgloss.Width — and the guard for that relationship is a test in this +// package, not this comment. +const maxTitleLen = 80 + +// clipTitle trims s and caps it at maxTitleLen runes. +// +// No ellipsis: this is not the display truncation — the TITLE column applies its own, measured in +// display columns — and a marker added here would be re-truncated downstream, leaving a cell with +// two of them. +func clipTitle(s string) string { + // ONE LINE. A prompt is free text and may hold newlines or tabs; a title is a table cell, and + // the viewer sanitises control characters into U+FFFD rather than dropping them, so a raw + // newline would reach the cell as a visible replacement glyph. Collapsing runs of whitespace + // also stops a wrapped prompt spending its 80 characters on indentation. + s = strings.Join(strings.Fields(s), " ") + r := []rune(s) + if len(r) <= maxTitleLen { + return s + } + return strings.TrimSpace(string(r[:maxTitleLen])) +} + +// promptFromMessage returns the text a person typed in one user turn, and whether the content was +// a plain STRING rather than an array of blocks. +// +// Two shapes, because Claude Code writes both: a bare string, or an array of blocks of which only +// `text` is human input. A tool_result array yields "" — it is tool output, and titling a session +// with a grep dump was the thing this function exists to prevent. +// +// The shape is returned because it grades the result. Every one of the 640 attributed human turns +// on the measured tree had string content and none had an array, while the arrays are where the +// harness injects — a "Base directory for this skill: …" title came from one. So a string is +// evidence about authorship, not just an encoding detail, and the caller ranks on it. +func promptFromMessage(raw json.RawMessage) (text string, wasString bool) { + if len(raw) == 0 { + return "", false + } + var plain string + if err := json.Unmarshal(raw, &plain); err == nil { + return plain, true + } + var blocks []contentBlock + if err := json.Unmarshal(raw, &blocks); err != nil { + return "", false + } + var b strings.Builder + for _, blk := range blocks { + if blk.Type != "text" || blk.Text == "" { + continue + } + if b.Len() > 0 { + b.WriteByte(' ') + } + b.WriteString(blk.Text) } - return cwd, err + return b.String(), false +} + +// unwrapCommandEnvelope renders a slash-command turn as the command the user typed, or returns s +// unchanged when it is not one. +// +// The turn is genuinely the user's — origin.kind is "human" — but the text is an envelope: +// "review/review +// some/path.md". Measured, 28 of 128 transcripts ended with one, so +// left alone they are the commonest title shape and every one reads as markup. Worse, 27 of those 28 +// share the same command, so they would all carry an identical title. +// +// Yields "/review some/path.md": the line the person would recognise, and one that tells the 28 +// apart by their arguments. is dropped as a duplicate of the name. +// +// Hand-parsed rather than by regexp, and deliberately: the obvious pattern for a tag pair wants a +// BACKREFERENCE to match the closing tag, which RE2 does not support — `regexp.MustCompile` panics +// at init on `\1`, which compiles clean and dies on first use. Two literal scans need no such +// trick. +func unwrapCommandEnvelope(s string) string { + if !strings.Contains(s, "") { + return s + } + name := between(s, "", "") + if name == "" { + return s + } + if args := between(s, "", ""); args != "" { + return name + " " + args + } + return name +} + +// between returns the text bracketed by open and closing, trimmed, or "". +// +// The second parameter is `closing`, not `close`: that would shadow the predeclared builtin, which +// go vet does not report and this module's lint step (go fmt + go vet) would therefore never catch. +func between(s, open, closing string) string { + i := strings.Index(s, open) + if i < 0 { + return "" + } + rest := s[i+len(open):] + j := strings.Index(rest, closing) + if j < 0 { + return "" + } + return strings.TrimSpace(rest[:j]) +} + +// stripWrapperTag removes a leading harness wrapper from text the user really typed, keeping what +// is inside it. +// +// Pasted input arrives as `\n…the actual text…`, on a turn whose +// origin.kind is "human" — so it must not be filtered like injected traffic, but the wrapper is +// still markup and left alone it became the title. Unlike a slash command there is nothing to +// reconstruct: the content after the tag IS the prompt. +// +// Only a LEADING tag, and only one. A "<" further in is ordinary prose ("is 3 < 5 in Go?"), and +// stripping repeatedly would start eating text that merely looks like markup. +func stripWrapperTag(s string) string { + t := strings.TrimSpace(s) + if !strings.HasPrefix(t, "<") { + return s + } + i := strings.IndexByte(t, '>') + if i < 0 { + return s + } + // Reuse the same tag-name test the synthetic filter applies, so "looks like a harness tag" + // means one thing in this file. + if !isSyntheticPrompt(t[:i+1] + "x") { + return s + } + inner := strings.TrimSpace(t[i+1:]) + if inner == "" { + return s + } + // A closing tag at the end is dropped too, so "body" yields "body". + if j := strings.LastIndex(inner, " 0 && strings.HasSuffix(inner, ">") { + if trimmed := strings.TrimSpace(inner[:j]); trimmed != "" { + return trimmed + } + } + return inner +} + +// isSyntheticPrompt reports whether s is a marker Claude Code inserted rather than something the +// user typed. +// +// Both observed families open with a bracket and are machine-written: "[Request interrupted...]" +// (46 occurrences on the measured tree) and "[Image: original 2100x200, displayed at...]" (11). +// Neither describes what a session is about, and the interrupt one is especially misleading as a +// LAST prompt, since it is exactly what a transcript ends with when someone stopped a tool call. +// +// Matched by prefix rather than by an exact set: these strings are Claude Code's, not ours, and a +// reworded variant should keep being skipped. The cost of the loose rule is that a genuine prompt +// beginning "[" is skipped too, which is rare and costs a fallback to the previous prompt. +func isSyntheticPrompt(s string) bool { + t := strings.TrimSpace(s) + if strings.HasPrefix(t, "[Request interrupted") || strings.HasPrefix(t, "[Image:") { + return true + } + // A HARNESS BLOCK, which arrives on the user turn because that is the channel the tool + // harness speaks on, but which nobody typed: , , + // , and friends. Measured, 27 of 128 transcripts ended with + // one, so without this the commonest title on a real tree is a notification envelope. + // + // Detected structurally — opens with an XML-ish tag — rather than by listing the tags, which + // are the harness's vocabulary and grow. A prompt that genuinely opens with "<" is skipped + // too; that costs a fallback to the previous prompt, which is the safe direction. + if strings.HasPrefix(t, "<") { + if i := strings.IndexByte(t, '>'); i > 1 { + // THE TAG NAME ONLY, cut at the first space or slash. An opening tag may carry + // attributes — a real transcript produced a title of raw + // `` markup — and the space, `=` and `"` all failed the + // character check below, so every attributed tag went undetected. Self-closing + // tags are cut the same way. + tag := t[1:i] + if j := strings.IndexAny(tag, " \t/"); j >= 0 { + tag = tag[:j] + } + // Lowercased before checking: every tag observed on a real tree is lowercase, so + // this is latent rather than a live bug, but it costs one call and the alternative + // is a filter that a capitalised variant walks straight through. + tag = strings.ToLower(tag) + if tag != "" && strings.IndexFunc(tag, func(r rune) bool { + return !(r >= 'a' && r <= 'z') && r != '-' && r != '_' + }) == -1 { + return true + } + } + } + return false } // transcriptMeta is the minimum shape needed from a transcript line. Decoding only @@ -472,7 +772,43 @@ func titleFromTranscript(path string) (string, error) { type transcriptMeta struct { Type string `json:"type"` AiTitle string `json:"aiTitle"` - Cwd string `json:"cwd"` + // AgentName is the title Claude Code records as {"type":"agent-name","agentName":…}. + // + // A SECOND SPELLING of the same idea, not a different field: some installs write ai-title + // lines, some write agent-name, and some write both. Measured locally: of 128 transcripts + // under one config dir, 16 carried agent-name and 2 carried ai-title with no overlap, while + // another dir had 26 ai-title and 16 agent-name — and in all 16 of those the two values were + // IDENTICAL. So this is a naming variant to accept, not a competing claim to arbitrate. + AgentName string `json:"agentName"` + Cwd string `json:"cwd"` + // Message is the turn body, decoded only far enough to recover a typed prompt. + // + // Content is json.RawMessage because Claude Code writes it two ways: a plain string for a + // simple prompt, and an array of blocks otherwise. Decoding it as either concrete type would + // silently drop the other — measured on one config dir, 1026 strings against 9718 arrays. + Message *struct { + Role string `json:"role"` + Content json.RawMessage `json:"content"` + } `json:"message"` + // Origin says who produced the turn, and it is the only authoritative answer available. + // + // Measured across 128 transcripts: kind is "human" on 640 turns, absent on 9940, and + // "task-notification" (151) or "peer" (33) on the rest. Those last two are precisely the + // harness-injected traffic that heuristics here used to guess at from the text — so where the + // field is present it replaces the guessing rather than supplementing it. + Origin *struct { + Kind string `json:"kind"` + } `json:"origin"` +} + +// contentBlock is one element of a message's content array. +// +// Only text blocks carry anything a person typed. The overwhelming majority are tool_result — +// 9576 of 9718 arrays on the measured tree — which is tool OUTPUT: grep hits, build logs, a +// CSpell summary. Titling a session with those was the failure mode this shape exists to avoid. +type contentBlock struct { + Type string `json:"type"` + Text string `json:"text"` } // ReadMetadata reads the existing file, distinguishing absent from unreadable. diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 9fae0a1bd..e27735313 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -12,6 +12,7 @@ import ( "sync" "testing" "time" + "unicode/utf8" ) // writeSessionTranscript writes one transcript with caller-supplied lines. @@ -783,3 +784,569 @@ func TestMain(m *testing.M) { } os.Exit(m.Run()) } + +// Both title line kinds are read: ai-title and agent-name. +// +// Claude Code writes one or the other depending on the install, and reading only ai-title left +// whole config dirs named by working directory instead — measured, 16 of 128 transcripts under one +// dir carried agent-name and only 2 carried ai-title, with no overlap. +func TestTitleFromTranscript_AcceptsBothTitleLineKinds(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + "agent-name only", + []string{`{"type":"agent-name","agentName":"sept-15-rossoctl","sessionId":"ee85f9ac"}`}, + "sept-15-rossoctl", + }, + { + "ai-title only", + []string{`{"type":"ai-title","aiTitle":"a generated title"}`}, + "a generated title", + }, + { + // Last-wins across the two kinds, the same rule that holds between two ai-title + // lines. Where real transcripts carry both they agree, so this only pins that + // neither kind is silently preferred over a later claim. + "both, agent-name later", + []string{ + `{"type":"ai-title","aiTitle":"earlier"}`, + `{"type":"agent-name","agentName":"later"}`, + }, + "later", + }, + { + "both, ai-title later", + []string{ + `{"type":"agent-name","agentName":"earlier"}`, + `{"type":"ai-title","aiTitle":"later"}`, + }, + "later", + }, + { + // A cwd is the fallback, not a competitor: a titled session keeps its title. + "agent-name beats the cwd fallback", + []string{ + `{"type":"user","cwd":"/some/dir"}`, + `{"type":"agent-name","agentName":"named"}`, + }, + "named", + }, + { + // The field on the wrong line kind is not a claim, same guard ai-title has. + "agentName on another line kind is ignored", + []string{ + `{"type":"user","agentName":"not a title","cwd":"/w"}`, + }, + "/w", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// The user's last typed prompt names a session when no title line does. +// +// Third tier, below both title kinds: measured on a real config dir, 110 of 128 transcripts carried +// neither an ai-title nor an agent-name and fell back to a directory path, which made most rows in +// the sessions table indistinguishable from each other. +func TestTitleFromTranscript_FallsBackToTheLastUserPrompt(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + "plain string content", + []string{`{"type":"user","message":{"role":"user","content":"how do I build abctl?"}}`}, + "how do I build abctl?", + }, + { + "text blocks are joined", + []string{`{"type":"user","message":{"role":"user","content":[{"type":"text","text":"first"},{"type":"text","text":"second"}]}}`}, + "first second", + }, + { + // LAST-wins, like the title tiers above it. + "last prompt wins", + []string{ + `{"type":"user","message":{"role":"user","content":"earlier ask"}}`, + `{"type":"user","message":{"role":"user","content":"later ask"}}`, + }, + "later ask", + }, + { + // tool_result is tool OUTPUT. 9576 of 9718 content arrays on the measured tree were + // this, so taking it would have titled 54 of 128 sessions with grep hits and build logs. + "tool_result is not a prompt", + []string{ + `{"type":"user","message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","message":{"role":"user","content":[{"type":"tool_result","content":"1\tMakefile\n2\tbuild:"}]}}`, + }, + "the real ask", + }, + { + "interrupt markers are skipped", + []string{ + `{"type":"user","message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","message":{"role":"user","content":"[Request interrupted by user for tool use]"}}`, + }, + "the real ask", + }, + { + "image markers are skipped", + []string{ + `{"type":"user","message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","message":{"role":"user","content":"[Image: original 2100x200, displayed at 2000x190]"}}`, + }, + "the real ask", + }, + { + // 27 of 128 transcripts ended with one of these, so without the filter the commonest + // title on a real tree is a notification envelope. + "harness blocks are skipped", + []string{ + `{"type":"user","message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","message":{"role":"user","content":"\nabc\n"}}`, + }, + "the real ask", + }, + { + "an assistant turn is not a prompt", + []string{ + `{"type":"user","message":{"role":"user","content":"the real ask"}}`, + `{"type":"assistant","message":{"role":"assistant","content":"my reply"}}`, + }, + "the real ask", + }, + { + // Both title kinds outrank a prompt, so a titled session is never renamed. + "a title line outranks a prompt", + []string{ + `{"type":"user","message":{"role":"user","content":"a long rambling ask"}}`, + `{"type":"agent-name","agentName":"sept-15-rossoctl"}`, + }, + "sept-15-rossoctl", + }, + { + "cwd is still the last resort", + []string{ + `{"type":"user","cwd":"/w/project"}`, + `{"type":"user","message":{"role":"user","content":"[Request interrupted by user]"}}`, + }, + "/w/project", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// Titles are clipped to 80 runes and flattened to one line. +// +// A prompt is unbounded free text and a title is a table cell. Runes rather than bytes so a +// multi-byte prompt is not cut mid-character, and whitespace is collapsed because the viewer turns +// control characters into U+FFFD rather than dropping them — a raw newline would reach the cell as +// a visible glyph. +func TestTitleFromTranscript_ClipsAndFlattens(t *testing.T) { + long := strings.Repeat("a", 200) + cjk := strings.Repeat("日", 200) + for _, tc := range []struct { + name string + content string + wantRunes int + wantOneLine bool + }{ + {"long ascii", long, maxTitleLen, true}, + {"long CJK is cut by rune, not byte", cjk, maxTitleLen, true}, + {"newlines collapse", "first line\n\nsecond line\twith a tab", -1, true}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if n := len([]rune(got)); n > maxTitleLen { + t.Errorf("title is %d runes, over the %d cap: %q", n, maxTitleLen, got) + } + if tc.wantRunes > 0 && len([]rune(got)) != tc.wantRunes { + t.Errorf("title is %d runes, want %d", len([]rune(got)), tc.wantRunes) + } + if tc.wantOneLine && strings.ContainsAny(got, "\n\t") { + t.Errorf("title carries a control character: %q", got) + } + if !utf8.ValidString(got) { + t.Errorf("title is not valid UTF-8 — cut mid-character: %q", got) + } + }) + } +} + +// An attributed human turn outranks everything below it, and a string outranks an array. +// +// origin.kind is the only authoritative statement of who produced a turn, and Claude Code supplies +// it: measured across 128 transcripts, 640 user turns were "human", 151 "task-notification", 33 +// "peer". Every human turn carried STRING content and none carried an array — which is why the shape +// grades the result too. The array path is where the harness injects, and it produced a +// "Base directory for this skill: /Users/…/plugins/cache/…" title before this ranking existed. +func TestTitleFromTranscript_PrefersAttributedHumanStrings(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + // The shape of the reported file: one human turn early, harness arrays after it. + "human string beats a later array", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"what I actually asked"}}`, + `{"type":"user","message":{"role":"user","content":[{"type":"text","text":"Base directory for this skill: /Users/x/.claude/plugins/cache/y"}]}}`, + }, + "what I actually asked", + }, + { + "human string beats an unattributed string", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"attributed"}}`, + `{"type":"user","message":{"role":"user","content":"unattributed but later"}}`, + }, + "attributed", + }, + { + // Explicitly not human: discarded rather than ranked. + "task-notification is discarded", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","origin":{"kind":"task-notification"},"message":{"role":"user","content":"a notification body"}}`, + }, + "the real ask", + }, + { + "peer is discarded", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"user","origin":{"kind":"peer"},"message":{"role":"user","content":"a peer message"}}`, + }, + "the real ask", + }, + { + // Last-wins WITHIN the human grade. + "the latest human turn wins", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"first ask"}}`, + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"second ask"}}`, + }, + "second ask", + }, + { + // An unattributed string still beats an array, for transcripts with no origin field. + "unattributed string beats an array", + []string{ + `{"type":"user","message":{"role":"user","content":[{"type":"text","text":"from an array"}]}}`, + `{"type":"user","message":{"role":"user","content":"a plain string"}}`, + }, + "a plain string", + }, + { + // A human turn is taken as typed, with no synthetic-text guessing over the top. + "a bracketed human prompt is kept", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"[note] this is mine"}}`, + }, + "[note] this is mine", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// A slash command is rendered as the line the user typed, not as its envelope. +// +// Claude Code wraps a typed slash command in //. The +// turn IS the user's, so filtering it would lose a real prompt — but 28 of 128 measured transcripts +// ended with one, and 27 of those shared the same command, so left as markup they would all carry an +// identical unreadable title. The arguments are what tell them apart. +func TestTitleFromTranscript_UnwrapsSlashCommands(t *testing.T) { + for _, tc := range []struct { + name, content, want string + }{ + { + "name and args", + `review\n/review\nsome/path.md`, + "/review some/path.md", + }, + { + "name only", + `clear\n/clear\n`, + "/clear", + }, + { + "ordinary prose is untouched", + `just a normal question`, + "just a normal question", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(strings.ReplaceAll(tc.content, `\n`, "\n")) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// The cwd fallback is NOT clipped, so sibling directories stay distinguishable. +// +// A path's distinguishing end is its leaf and clipTitle keeps the head, so two worktrees under a +// prefix of 80 runes or more clipped to byte-identical titles — the column stopped telling apart +// exactly the sessions it exists to name. Also a regression against the behaviour before titles +// were clipped at all. +func TestTitleFromTranscript_DoesNotClipTheCwdFallback(t *testing.T) { + // 81 runes, so the leaf is entirely past the cap. + const prefix = "/Users/somebody/go/src/github.com/some-organisation/some-repository/worktrees/wt/" + if len([]rune(prefix)) <= maxTitleLen { + t.Fatalf("fixture prefix is %d runes, needs to exceed %d to exercise the cut", + len([]rune(prefix)), maxTitleLen) + } + titleFor := func(cwd string) string { + dir := t.TempDir() + body, err := json.Marshal(cwd) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", `{"type":"user","cwd":`+string(body)+`}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + return got + } + a, b := titleFor(prefix+"alpha"), titleFor(prefix+"beta") + if a == b { + t.Errorf("sibling paths produced the same title %q — the leaf was clipped away", a) + } + if !strings.HasSuffix(a, "alpha") || !strings.HasSuffix(b, "beta") { + t.Errorf("the leaf did not survive: %q / %q", a, b) + } +} + +// A whitespace-only candidate falls through to the next tier instead of rendering blank. +// +// It passed a bare `!= ""` guard and clipTitle then emptied it, so the tier below was skipped and +// the cell came out empty. Each candidate is normalised before the switch now. +func TestTitleFromTranscript_WhitespaceOnlyCandidateFallsThrough(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + "whitespace ai-title falls through to the cwd", + []string{ + `{"type":"user","cwd":"/w/real"}`, + `{"type":"ai-title","aiTitle":" "}`, + }, + "/w/real", + }, + { + "whitespace agent-name falls through to a prompt", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"agent-name","agentName":"\t\n "}`, + }, + "the real ask", + }, + { + "whitespace prompt falls through to the cwd", + []string{ + `{"type":"user","cwd":"/w/real"}`, + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":" \t "}}`, + }, + "/w/real", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// Harness tags are detected with attributes, and whatever their case. +// +// The tag-name check ran over everything up to ">", so a space, "=" or quote failed it and every +// attributed tag slipped through — a real transcript produced a title of raw +// `` markup. Case folding is latent by comparison: every tag observed on a +// real tree is lowercase. +func TestIsSyntheticPrompt_HandlesAttributesAndCase(t *testing.T) { + for _, s := range []string{ + `some pasted text`, + `body`, + `body`, + `body`, + ``, + `out`, + `[Request interrupted by user for tool use]`, + `[Image: original 2100x200, displayed at 2000x190]`, + } { + if !isSyntheticPrompt(s) { + t.Errorf("not detected as synthetic: %q", s) + } + } + // And real prose is still a prompt, including text that merely contains a "<". + for _, s := range []string{ + "how do I build abctl?", + "is 3 < 5 in Go?", + "/review some/path.md", + "日本語の質問です", + } { + if isSyntheticPrompt(s) { + t.Errorf("real prompt rejected as synthetic: %q", s) + } + } +} + +// maxTitleLen is a RUNE cap, and the renderer is what bounds display width. +// +// 80 runes of CJK occupy 160 columns, so nothing may read this constant as a width budget. The +// relationship is safe only because the sessions pane re-truncates by lipgloss.Width; this pins the +// half that lives in this package, so a later reader cannot mistake the cap for a column bound +// without a test failing. +func TestMaxTitleLen_IsARuneCapNotAWidthBudget(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(strings.Repeat("日", 200)) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if n := len([]rune(got)); n != maxTitleLen { + t.Errorf("clipped to %d runes, want %d", n, maxTitleLen) + } + // The point of the test: runes are capped, BYTES AND COLUMNS ARE NOT. authlib has no width + // library — lipgloss and go-runewidth are cmd/abctl dependencies, and adding one here for a + // single assertion is not worth it — so byte length stands in as the observable proxy: a + // three-byte-per-rune title is 240 bytes at 80 runes, and anything laying out by width will + // likewise see more than 80. What this pins is that the cap is NOT a width, which is the + // mistake the comment on maxTitleLen warns against. + if len(got) <= maxTitleLen { + t.Errorf("CJK title is %d bytes for %d runes; if that is now <= the cap then maxTitleLen "+ + "is being applied as a width or byte bound, and the renderers' own truncation must be "+ + "revisited", len(got), maxTitleLen) + } +} + +// A pasted-input wrapper is stripped from a human turn, keeping what was pasted. +// +// `…` arrives with origin.kind "human" — the user really did paste it — so +// filtering it would lose a real prompt, but the wrapper is markup and left alone it became the +// title. A real transcript produced exactly that. +func TestTitleFromTranscript_StripsPastedContentWrapper(t *testing.T) { + for _, tc := range []struct { + name, content, want string + }{ + { + "pasted wrapper is stripped", + `` + "\n" + `When considering each agent node, consult the first`, + "When considering each agent node, consult the first", + }, + { + "closing tag is dropped too", + `the pasted body`, + "the pasted body", + }, + { + // A command envelope must NOT lose its arguments to the wrapper strip: it is a + // multi-tag structure and sits past the first tag. + "a command envelope keeps its args", + `review` + "\n" + `/review` + "\n" + `some/path.md`, + "/review some/path.md", + }, + { + "prose containing a less-than is untouched", + "is 3 < 5 in Go?", + "is 3 < 5 in Go?", + }, + { + "a wrapper with nothing after it is left as-is rather than emptied", + ``, + ``, + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} From dc8293622063856c72185a436fed2c0e493481f4 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Tue, 22 Sep 2026 17:59:07 -0400 Subject: [PATCH 02/16] feat(abctl): Prefer Claude Code's last-prompt record, and stop markup titles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prefer `{"type":"last-prompt","lastPrompt":…}` over anything reconstructed from user turns. It is the agent stating what the prompt was rather than this package inferring it and then filtering harness traffic back out, and 129 of 130 local transcripts carry one. Both title kinds still outrank it: a generated title is a summary, this is raw input. A recorded slash command is unwrapped the same way a typed one is, and a null value — 3 of 2223 real lines — falls through. Markup that survives one leading-tag strip now falls through instead of becoming the title. Three shapes, one root cause, all reproduced first: - an empty body left the closing tag at index 0, so a `j > 0` guard skipped the trim and yielded a bare ""; - a malformed envelope (" real text") made the unwrapper bail on the empty name, so control reached the wrapper strip, which removed only the leading tag; - a nested wrapper ("x") had only its outer tag removed. No occurrence on real data, but the same defect. Fixed by re-testing the unwrapped result against the synthetic-prompt guard at the call site, which covers all three: both helpers return their input unchanged when they cannot make sense of it, and that value was being assigned verbatim. isSyntheticPrompt also now recognises a leading CLOSING tag, which is what the malformed-envelope case leaves behind. Two earlier test expectations changed with it, both because the new behaviour is better: a wrapper with nothing inside now yields no title rather than raw markup, and a wrapped lastPrompt is stripped to its body rather than discarded. On the local tree this takes the count from 124 named to 127, with 3 sessions on the directory fallback, and no blank or markup titles. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 90 +++++++++-- .../authlib/observe/claude/harvest_test.go | 143 +++++++++++++++++- 2 files changed, 218 insertions(+), 15 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 31f5a4fd8..edab55909 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -439,7 +439,9 @@ func titleFromTranscript(path string) (string, error) { // toolscan.scanFile. sc.Buffer(make([]byte, 0, 256*1024), 16*1024*1024) - // THREE GRADES OF PROMPT, best first. Each is last-wins within its own grade, so a later turn + // lastPrompt is Claude Code's own record and outranks every reconstruction below it. + // + // THREE GRADES OF RECONSTRUCTED PROMPT, best first. Each is last-wins within its own grade, so a later turn // of the same quality replaces an earlier one but never a better one. // // human — origin.kind == "human" AND string content. What the person typed, stated by @@ -449,7 +451,7 @@ func titleFromTranscript(path string) (string, error) { // prompt in practice, just unattributed. // blocks — text extracted from a content ARRAY. Last resort: these are where the harness // injects, and a "Base directory for this skill: …" title came from one. - var title, cwd, human, str, blocks string + var title, cwd, lastPrompt, human, str, blocks string for sc.Scan() { line := sc.Bytes() // Hot path: most lines are conversation turns carrying neither field. A @@ -458,6 +460,7 @@ func titleFromTranscript(path string) (string, error) { if !bytes.Contains(line, []byte(`"aiTitle"`)) && !bytes.Contains(line, []byte(`"agentName"`)) && !bytes.Contains(line, []byte(`"cwd"`)) && + !bytes.Contains(line, []byte(`"lastPrompt"`)) && !bytes.Contains(line, []byte(`"role":"user"`)) { continue } @@ -488,6 +491,18 @@ func titleFromTranscript(path string) (string, error) { // and `"role"` appears on every turn either way. Tool output and Claude Code's own // bracketed markers are filtered by the two helpers rather than here, so this stays a // statement about WHICH turn counts. + // Claude Code's own record of the last prompt. Preferred over anything reconstructed from + // user turns below: same information, stated by the agent instead of inferred, and it + // arrives already free of the tool output and harness envelopes those turns carry. Still + // put through the same unwrap and synthetic checks, since the value is the prompt text and + // a slash command is recorded in its envelope form there too. + if e.Type == "last-prompt" && e.LastPrompt != "" { + if p := unwrapCommandEnvelope(e.LastPrompt); !isSyntheticPrompt(p) { + lastPrompt = p + } else if p := stripWrapperTag(e.LastPrompt); !isSyntheticPrompt(p) { + lastPrompt = p + } + } if e.Type == "user" && e.Message != nil && e.Message.Role == "user" { kind := "" if e.Origin != nil { @@ -509,9 +524,18 @@ func titleFromTranscript(path string) (string, error) { // whose sit past the first tag, so stripping a leading wrapper // beforehand threw the arguments away and left a bare "/review". Only a turn that // is not a command envelope reaches the wrapper strip. - human = unwrapCommandEnvelope(text) - if human == text { - human = stripWrapperTag(text) + candidate := unwrapCommandEnvelope(text) + if candidate == text { + candidate = stripWrapperTag(text) + } + // RE-TESTED after unwrapping, which is the step that was missing. Both helpers + // return their input unchanged when they cannot make sense of it — an empty-bodied + // wrapper, a malformed envelope, a nested tag — and assigning that straight to + // `human` put raw markup in the title. Checking here rather than inside the helpers + // keeps them pure string transforms and puts the one decision at the one place that + // has to make it. + if !isSyntheticPrompt(candidate) { + human = candidate } case kind != "" && kind != "human": // Explicitly NOT human — "task-notification" or "peer". Discarded outright; @@ -532,8 +556,9 @@ func titleFromTranscript(path string) (string, error) { // last-wins is the rule here, so the name returned may be an old one. The buffer above // makes this rare; silence made it invisible. err = sc.Err() - // THREE TIERS, in descending confidence: a title Claude Code generated, then the last thing - // the user actually typed, then the directory the session ran in. The prompt tier is what + // FOUR TIERS, in descending confidence: a title Claude Code generated, then its own record of + // the last prompt, then the last prompt this package reconstructs from user turns, then the + // directory the session ran in. The prompt tier is what // takes a tree from "mostly paths" to "mostly readable" — measured on one config dir, 110 of // 128 sessions had no title line of either kind and fell through to a cwd, and every one of // those has a usable prompt. @@ -543,11 +568,14 @@ func titleFromTranscript(path string) (string, error) { // bare `!= ""` and clipTitle then empties it, so the tier below was skipped and the cell came // out blank — testing the clipped value is what makes each guard mean "this tier has // something to show". - title, human, str = clipTitle(title), clipTitle(human), clipTitle(str) + title, lastPrompt = clipTitle(title), clipTitle(lastPrompt) + human, str = clipTitle(human), clipTitle(str) blocks = clipTitle(blocks) switch { case title != "": return title, err + case lastPrompt != "": + return lastPrompt, err case human != "": return human, err case str != "": @@ -710,10 +738,31 @@ func stripWrapperTag(s string) string { return s } // A closing tag at the end is dropped too, so "body" yields "body". - if j := strings.LastIndex(inner, " 0 && strings.HasSuffix(inner, ">") { - if trimmed := strings.TrimSpace(inner[:j]); trimmed != "" { - return trimmed - } + // + // `j >= 0`, not `j > 0`: an EMPTY-bodied wrapper leaves the closing tag at index 0, so the + // stricter guard skipped the trim and this returned a bare "". + // + // REDUNDANT for the caller as it stands — the call site re-tests the result against + // isSyntheticPrompt, which rejects a bare closing tag either way, and reverting this to `j > 0` + // breaks no test. Kept so the helper is right on its own terms rather than only in the company + // of that check: a second caller would otherwise inherit the bug. + if j := strings.LastIndex(inner, "= 0 && strings.HasSuffix(inner, ">") { + inner = strings.TrimSpace(inner[:j]) + } + // RE-TESTED against the same guard, which is what closes the whole family rather than one + // shape of it. Three ways markup survived a single leading strip: + // + // - an empty body, leaving a bare closing tag; + // - a malformed command envelope (" real text"), where the + // unwrapper bails on the empty name and control falls through to here; + // - a nested wrapper ("x"), where only the outer tag is removed. + // + // Any of them leaves something that still opens with a tag, so asking the guard again is both + // the narrowest fix and the one that does not need a list of shapes. Returning s unchanged on + // a still-markup result hands the caller a value its own synthetic filter will reject, so the + // turn falls through to the next tier instead of titling a session with markup. + if inner == "" || isSyntheticPrompt(inner) { + return s } return inner } @@ -743,6 +792,14 @@ func isSyntheticPrompt(s string) bool { // are the harness's vocabulary and grow. A prompt that genuinely opens with "<" is skipped // too; that costs a fallback to the previous prompt, which is the safe direction. if strings.HasPrefix(t, "<") { + // A CLOSING tag counts too. A malformed command envelope — + // " real text" — bails out of the unwrapper on the empty + // name, falls through to the wrapper strip, and leaves " real text": a + // dangling close that is still markup and still not a title. Skipping the slash here means + // one guard recognises both halves of a tag pair. + if strings.HasPrefix(t, "'); i > 1 { // THE TAG NAME ONLY, cut at the first space or slash. An opening tag may carry // attributes — a real transcript produced a title of raw @@ -780,7 +837,14 @@ type transcriptMeta struct { // another dir had 26 ai-title and 16 agent-name — and in all 16 of those the two values were // IDENTICAL. So this is a naming variant to accept, not a competing claim to arbitrate. AgentName string `json:"agentName"` - Cwd string `json:"cwd"` + // LastPrompt is Claude Code's own record of the session's last prompt, written as + // {"type":"last-prompt","lastPrompt":…}. + // + // The most direct answer available: it is the agent stating what the prompt WAS, rather than + // this package reconstructing it from user turns and then filtering harness traffic back out. + // Measured, 129 of 130 transcripts carry one, so it is the usual case rather than a bonus. + LastPrompt string `json:"lastPrompt"` + Cwd string `json:"cwd"` // Message is the turn body, decoded only far enough to recover a typed prompt. // // Content is json.RawMessage because Claude Code writes it two ways: a plain string for a diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index e27735313..da3624f17 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1327,9 +1327,15 @@ func TestTitleFromTranscript_StripsPastedContentWrapper(t *testing.T) { "is 3 < 5 in Go?", }, { - "a wrapper with nothing after it is left as-is rather than emptied", - ``, + // Falls through rather than titling with markup. An earlier version of this test + // expected the raw tag back, on the reasoning that returning the input unchanged is + // the safe default — but the caller now re-tests the result against the synthetic + // guard, so "unchanged" means "rejected" and the tier below is used instead. With no + // cwd in this fixture that leaves "", which is the honest answer for a turn whose + // entire content was a wrapper. + "a wrapper with nothing inside it yields no title", ``, + "", }, } { t.Run(tc.name, func(t *testing.T) { @@ -1350,3 +1356,136 @@ func TestTitleFromTranscript_StripsPastedContentWrapper(t *testing.T) { }) } } + +// Claude Code's own last-prompt record outranks anything reconstructed from user turns. +// +// {"type":"last-prompt","lastPrompt":…} is the agent stating what the prompt was, rather than this +// package inferring it and then filtering harness traffic back out. Measured, 129 of 130 transcripts +// carry one. +func TestTitleFromTranscript_PrefersLastPromptRecord(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + "last-prompt beats a reconstructed human turn", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"a reconstructed ask"}}`, + `{"type":"last-prompt","lastPrompt":"the recorded ask"}`, + }, + "the recorded ask", + }, + { + // Both title kinds still outrank it: a generated title is a summary, this is raw input. + "a title line still outranks last-prompt", + []string{ + `{"type":"last-prompt","lastPrompt":"the recorded ask"}`, + `{"type":"agent-name","agentName":"sept-15-rossoctl"}`, + }, + "sept-15-rossoctl", + }, + { + "last-wins among last-prompt lines", + []string{ + `{"type":"last-prompt","lastPrompt":"earlier"}`, + `{"type":"last-prompt","lastPrompt":"later"}`, + }, + "later", + }, + { + // A slash command is recorded in its envelope form here too. + "a recorded slash command is unwrapped", + []string{ + `{"type":"last-prompt","lastPrompt":"review\n/review\nsome/path.md"}`, + }, + "/review some/path.md", + }, + { + // Three of 2223 real lines carried a null, which decodes to "". + "a null lastPrompt falls through", + []string{ + `{"type":"user","cwd":"/w/real"}`, + `{"type":"last-prompt","lastPrompt":null}`, + }, + "/w/real", + }, + { + // A WRAPPED record is unwrapped, not discarded: the body is the prompt, same as for a + // pasted-content turn. Only a record that is markup all the way down falls through. + "a wrapped lastPrompt is stripped to its body", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"last-prompt","lastPrompt":"body"}`, + }, + "body", + }, + { + "a lastPrompt that is markup all the way down falls through", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"last-prompt","lastPrompt":"x"}`, + }, + "the real ask", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// Markup that survives one leading-tag strip falls through instead of becoming the title. +// +// Three shapes, one root cause — a single strip is not enough, so the result is re-tested against +// the synthetic guard: +// +// - an EMPTY body leaves the closing tag at index 0, which a `j > 0` guard skipped, yielding a +// bare ""; +// - a MALFORMED command envelope makes the unwrapper bail on the empty name, so control reaches +// the wrapper strip and leaves " real text"; +// - a NESTED wrapper has only its outer tag removed, leaving "real prompt". +// +// The nested case has no occurrence on real data (0 of 645 human string turns), but all three are the +// same defect and the fix is one check. +func TestTitleFromTranscript_MarkupSurvivingOneStripFallsThrough(t *testing.T) { + for _, tc := range []struct { + name, content string + }{ + {"empty-bodied wrapper", ``}, + {"malformed command envelope", ` real text`}, + {"nested wrapper", `real prompt`}, + {"bare closing tag", ``}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + // A cwd is present, so falling through has somewhere to land and the assertion + // distinguishes "fell through" from "returned empty". + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/real"}`, + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if got != "/w/real" { + t.Errorf("title = %q, want the cwd fallback — markup reached the title", got) + } + if strings.ContainsAny(got, "<>") { + t.Errorf("title carries markup: %q", got) + } + }) + } +} From cff6cfb80c6c9fc833a37d7dc86d416db20201da Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Tue, 22 Sep 2026 18:33:57 -0400 Subject: [PATCH 03/16] fix(abctl): Keep harness markup out of titles, wherever it sits in the turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX. Two leaks reached the rendered title, both reproduced first: - A harness BLOCK arriving alongside the user's real text. promptFromMessage joined every text block and isSyntheticPrompt is anchored at the start, so the join began with prose and the markup passed through intact. Blocks are filtered individually inside the join now. - Markup APPENDED after prose in a string turn, which no anchored check can see. Measured, 7 of 130 local transcripts are shaped "…real question……", and the tag was clipped mid-tag at 80 runes. Cut at the first known harness tag, keeping what precedes it. The cut matches a NAMED set of tags, not any "": it removes text mid-prompt, so a loose rule would truncate a question that quotes HTML or generics. Both are covered, as is a harness block appearing before the prose rather than after. The prefilter goes back to the bare `"role"` key. `"role":"user"` embeds a key-value pair and so assumed compact JSON — a line written `"role": "user"` was skipped before decoding, silently losing the prompt — while every other term is a bare key and the role is re-checked after the decode anyway. It also did not pay: 1.29s against 1.19s on a real tree. Every fixture in the suite was hand-written compact JSON, so nothing could have caught this; there is now a test that writes the spaced form for all five line kinds. The last-prompt and human branches shared an intent and spelled it two ways. Both now call promptCandidate, so the unwrap order and the synthetic re-test cannot drift apart. Two comments corrected where they overstated what the code does: isSyntheticPrompt's doc said harness blocks are "dropped", when some are unwrapped and their body kept — it is a test, not a policy, and promptCandidate decides. And maxTitleLen's doc claimed the rune-cap-to-display-column relationship is guarded by a test in this package; it is not. That invariant spans two modules, this side can only assert the cap counts runes, and nothing fails if the renderer's truncation is removed. Said plainly in both places, and in the test's own comment. The PR description carried the same "dropped" overstatement and has been corrected to describe the per-shape handling. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 133 +++++++++++++++--- .../authlib/observe/claude/harvest_test.go | 126 ++++++++++++++++- 2 files changed, 234 insertions(+), 25 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index edab55909..e6b503f26 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -457,11 +457,20 @@ func titleFromTranscript(path string) (string, error) { // Hot path: most lines are conversation turns carrying neither field. A // substring test over the raw bytes is far cheaper than parsing them, and // bytes.Contains avoids the copy that strings.Contains(string(line), …) makes. + // + // BARE KEYS ONLY. `"role":"user"` is more selective — it matched 22% of lines against 59% + // for `"role"` — but it embeds a key-value PAIR and so assumes compact JSON: a line written + // as `"role": "user"` would be skipped before decoding, silently losing the prompt. Every + // other term here is a bare key for that reason, and the role is re-checked after the + // decode anyway, so the narrow form bought selectivity at the cost of depending on the + // writer's spacing. Measured on a real tree it also did not pay: 1.29s against 1.19s for + // the bare key, because the scan is dominated by a few very large lines rather than by how + // many lines reach the decoder. if !bytes.Contains(line, []byte(`"aiTitle"`)) && !bytes.Contains(line, []byte(`"agentName"`)) && !bytes.Contains(line, []byte(`"cwd"`)) && !bytes.Contains(line, []byte(`"lastPrompt"`)) && - !bytes.Contains(line, []byte(`"role":"user"`)) { + !bytes.Contains(line, []byte(`"role"`)) { continue } var e transcriptMeta @@ -497,9 +506,10 @@ func titleFromTranscript(path string) (string, error) { // put through the same unwrap and synthetic checks, since the value is the prompt text and // a slash command is recorded in its envelope form there too. if e.Type == "last-prompt" && e.LastPrompt != "" { - if p := unwrapCommandEnvelope(e.LastPrompt); !isSyntheticPrompt(p) { - lastPrompt = p - } else if p := stripWrapperTag(e.LastPrompt); !isSyntheticPrompt(p) { + // ONE SHAPE, shared with the human branch below via promptCandidate, so a later fix to + // the unwrap order cannot land in only one of them. The two used to spell the same + // intent differently. + if p, ok := promptCandidate(e.LastPrompt); ok { lastPrompt = p } } @@ -524,18 +534,8 @@ func titleFromTranscript(path string) (string, error) { // whose sit past the first tag, so stripping a leading wrapper // beforehand threw the arguments away and left a bare "/review". Only a turn that // is not a command envelope reaches the wrapper strip. - candidate := unwrapCommandEnvelope(text) - if candidate == text { - candidate = stripWrapperTag(text) - } - // RE-TESTED after unwrapping, which is the step that was missing. Both helpers - // return their input unchanged when they cannot make sense of it — an empty-bodied - // wrapper, a malformed envelope, a nested tag — and assigning that straight to - // `human` put raw markup in the title. Checking here rather than inside the helpers - // keeps them pure string transforms and puts the one decision at the one place that - // has to make it. - if !isSyntheticPrompt(candidate) { - human = candidate + if p, ok := promptCandidate(text); ok { + human = p } case kind != "" && kind != "human": // Explicitly NOT human — "task-notification" or "peer". Discarded outright; @@ -603,9 +603,14 @@ func titleFromTranscript(path string) (string, error) { // still. Runes, not bytes, so a multi-byte prompt is not cut mid-character. // // NOT A DISPLAY-COLUMN BUDGET. 80 runes of CJK occupy 160 columns, so nothing may treat this as a -// width. It is safe only because every renderer re-truncates by display width — the sessions -// pane measures with lipgloss.Width — and the guard for that relationship is a test in this -// package, not this comment. +// width. It is safe only because every renderer re-truncates by display width — the sessions pane +// measures with lipgloss.Width. +// +// THAT RELATIONSHIP IS NOT GUARDED BY ANY SINGLE TEST, and an earlier version of this comment +// implied otherwise. It spans two modules: this package has no width library, so its test asserts +// only that the cap is a rune count and not a byte or width bound, while the re-truncation lives in +// cmd/abctl/tui and is tested there against its own column budgets. Nothing fails if someone +// removes the renderer's truncation and leaves this constant alone. const maxTitleLen = 80 // clipTitle trims s and caps it at maxTitleLen runes. @@ -654,6 +659,13 @@ func promptFromMessage(raw json.RawMessage) (text string, wasString bool) { if blk.Type != "text" || blk.Text == "" { continue } + // FILTERED PER BLOCK, not after joining. A harness block can arrive ALONGSIDE the user's + // real text — "my question" and "…" as two text blocks of one turn — and + // testing only the joined string let the markup through, because the guard is anchored at + // the start and the join begins with the prose. + if isSyntheticPrompt(blk.Text) { + continue + } if b.Len() > 0 { b.WriteByte(' ') } @@ -767,8 +779,87 @@ func stripWrapperTag(s string) string { return inner } -// isSyntheticPrompt reports whether s is a marker Claude Code inserted rather than something the -// user typed. +// promptCandidate turns one raw prompt string into a usable title, reporting whether anything +// survived. +// +// ONE implementation for both callers — the recorded last-prompt line and the attributed human turn +// — because they want exactly the same thing and used to say so in two different shapes. Verified +// behaviourally identical at the time, which is precisely the state in which a later fix lands in +// only one of them. +// +// Order is load-bearing: +// +// 1. the command envelope first, because it is a multi-tag structure whose sit past +// the first tag — stripping a leading wrapper beforehand threw the arguments away; +// 2. otherwise a leading wrapper, keeping what it contains; +// 3. then trailing harness markup, which neither of the above sees since both are anchored; +// 4. then the synthetic test, because every helper returns its input UNCHANGED when it cannot make +// sense of it, and assigning that verbatim is what put raw markup in titles. +func promptCandidate(text string) (string, bool) { + candidate := unwrapCommandEnvelope(text) + if candidate == text { + candidate = stripWrapperTag(text) + } + candidate = cutTrailingHarness(candidate) + if isSyntheticPrompt(candidate) { + return "", false + } + return candidate, true +} + +// harnessTagNames are the wrapper tags observed on real transcripts, for the trailing-markup cut. +// +// A NAMED SET here, unlike isSyntheticPrompt's structural test, and deliberately: that one asks +// "does this text BEGIN as markup", where a false positive costs one fallback. This one cuts text +// off mid-prompt, so it may only fire on tags known to be the harness's. Matching any "" +// would truncate a prompt that quotes HTML or generics. +var harnessTagNames = []string{ + "…", which +// the anchored guard cannot see because the line starts with prose. Left alone the tag reached the +// title and was clipped mid-tag at 80 runes. +// +// Cuts at the FIRST known tag and keeps what precedes it — the prompt is the part the user typed, and +// everything the harness appends comes after. Returns s unchanged when nothing precedes the tag, so a +// turn that is markup all the way through still falls to isSyntheticPrompt rather than becoming "". +func cutTrailingHarness(s string) string { + cut := -1 + for _, tag := range harnessTagNames { + if i := strings.Index(s, tag); i >= 0 && (cut < 0 || i < cut) { + cut = i + } + } + if cut <= 0 { + return s + } + if head := strings.TrimSpace(s[:cut]); head != "" { + return head + } + return s +} + +// isSyntheticPrompt reports whether s READS AS markup Claude Code inserted rather than as something +// the user typed. +// +// A TEST, not a policy. What callers do with a positive answer differs, and the distinction matters +// because an earlier version of this comment claimed harness blocks are "dropped": some are, but a +// wrapper around text the user really pasted has its body kept (stripWrapperTag), and a slash command +// is re-rendered rather than discarded (unwrapCommandEnvelope). This function only answers the +// question; promptCandidate decides. // // Both observed families open with a bracket and are machine-written: "[Request interrupted...]" // (46 occurrences on the measured tree) and "[Image: original 2100x200, displayed at...]" (11). diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index da3624f17..4fd5a2838 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1263,10 +1263,11 @@ func TestIsSyntheticPrompt_HandlesAttributesAndCase(t *testing.T) { // maxTitleLen is a RUNE cap, and the renderer is what bounds display width. // -// 80 runes of CJK occupy 160 columns, so nothing may read this constant as a width budget. The -// relationship is safe only because the sessions pane re-truncates by lipgloss.Width; this pins the -// half that lives in this package, so a later reader cannot mistake the cap for a column bound -// without a test failing. +// 80 runes of CJK occupy 160 columns, so nothing may read this constant as a width budget. This +// pins ONLY the half that lives in this package: that the cap counts runes rather than bytes or +// columns. The other half — that the sessions pane re-truncates by lipgloss.Width — is in +// cmd/abctl/tui and tested there; no test in either module fails if that truncation is removed while +// this constant stays, so the cross-module invariant rests on the comments, not on this test. func TestMaxTitleLen_IsARuneCapNotAWidthBudget(t *testing.T) { dir := t.TempDir() body, err := json.Marshal(strings.Repeat("日", 200)) @@ -1489,3 +1490,120 @@ func TestTitleFromTranscript_MarkupSurvivingOneStripFallsThrough(t *testing.T) { }) } } + +// Harness markup never reaches the title, wherever in the turn it sits. +// +// Two leaks, both reaching rendered output before this: +// +// - a harness BLOCK alongside the user's real text in a multi-block turn. promptFromMessage joined +// every text block and the guard is anchored at the start, so the join began with prose and the +// markup passed intact. Filtered per block now. +// - markup APPENDED after prose in a string turn, which no anchored check can see. Measured on a +// real tree: 7 of 130 transcripts. Cut at the first known harness tag. +func TestTitleFromTranscript_NoHarnessMarkupInTitles(t *testing.T) { + for _, tc := range []struct { + name string + lines []string + want string + }{ + { + "a harness block beside real text is dropped", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":[{"type":"text","text":"my real question"},{"type":"text","text":"hidden"}]}}`}, + "my real question", + }, + { + "a harness block BEFORE real text is dropped", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":[{"type":"text","text":"hidden"},{"type":"text","text":"my real question"}]}}`}, + "my real question", + }, + { + "markup appended after prose is cut", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"my real question\ndo not mention this"}}`}, + "my real question", + }, + { + "a recorded lastPrompt is cut the same way", + []string{`{"type":"last-prompt","lastPrompt":"recorded ask\nhidden"}`}, + "recorded ask", + }, + { + // Only KNOWN harness tags cut, so a prompt that quotes markup survives — the cut + // removes text mid-prompt, so a loose rule would truncate legitimate questions. + "a prompt quoting unknown markup is untouched", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"why does
break my layout?"}}`}, + "why does
break my layout?", + }, + { + "a prompt mentioning generics is untouched", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"how do I write List in Go?"}}`}, + "how do I write List in Go?", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.lines...) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + if strings.Contains(got, " Date: Tue, 22 Sep 2026 19:18:15 -0400 Subject: [PATCH 04/16] fix(abctl): Unwrap only user-content wrappers, and cut only line-leading tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, both reproduced first, and both contradicting claims this PR already made. stripWrapperTag unwrapped anything tag-shaped, so the harness's OWN output became the title: `total 40` rendered as "total 40", and a `` body rendered as a title. That is the grep-dump outcome isSyntheticPrompt's doc says it exists to prevent, it contradicted this PR's claim that markup all the way through falls through, and a committed test asserted it — classifying a block as synthetic while asserting its body becomes the title. Narrowed to an allowlist of wrappers whose CONTENT IS THE USER'S, which today is `` alone: the user pasted what is inside it, so the body is a prompt. Everything else the harness emits is its own text and belongs to the next tier down. An allowlist rather than a denylist because the failure directions are not symmetric — omitting a content-bearing tag costs one fallback, omitting a harness tag puts tool output in the title. The contradictory test now asserts the fall-through, with a second case pinning that `` is still unwrapped. cutTrailingHarness matched a known tag ANYWHERE in the string, so prose that merely mentioned one was truncated mid-sentence: "how do I use in a skill?" became "how do I use". The named set was chosen precisely to avoid that, and the comment claimed it did. A tag now counts only where it STARTS A LINE, which is how the harness appends, and every occurrence is examined so an inline mention cannot mask a real appended block. The coverage gap that let the second one through is worth naming: all four existing "untouched prose" cases used UNKNOWN tags —
, List, "3 < 5" — so every one took isSyntheticPrompt's structural path and none reached the named set. No test put a known tag inside real prose. There is one now, with the line-leading case beside it. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 86 +++++++++++-- .../authlib/observe/claude/harvest_test.go | 115 +++++++++++++++++- 2 files changed, 187 insertions(+), 14 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index e6b503f26..a6ba5cfc9 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -721,8 +721,42 @@ func between(s, open, closing string) string { return strings.TrimSpace(rest[:j]) } -// stripWrapperTag removes a leading harness wrapper from text the user really typed, keeping what -// is inside it. +// contentBearingWrappers are the harness tags whose BODY is the user's own text. +// +// The distinction that matters for stripWrapperTag: a `` block holds something the +// user pasted, so its body is a prompt and unwrapping it recovers a real title. A ``, +// `` or `` block holds the harness's own output, so its body is +// not a prompt at any depth and the turn must fall through instead. +// +// An allowlist rather than a denylist because the failure directions are not symmetric: omitting a +// content-bearing tag costs one fallback to the previous prompt, while omitting a harness tag puts +// tool output in the title. +var contentBearingWrappers = []string{ + "pasted_content", +} + +// isContentBearingWrapper reports whether tag — a full opening tag, "" — is one whose body +// is the user's own text. +func isContentBearingWrapper(tag string) bool { + name := strings.TrimPrefix(strings.TrimSpace(tag), "<") + if j := strings.IndexAny(name, " \t/>"); j >= 0 { + name = name[:j] + } + name = strings.ToLower(name) + for _, want := range contentBearingWrappers { + if name == want { + return true + } + } + return false +} + +// stripWrapperTag removes a leading CONTENT-BEARING wrapper from text the user really typed, keeping +// what is inside it. +// +// Content-bearing means the body belongs to the user — see contentBearingWrappers. A wrapper holding +// the harness's own output is left alone, so the caller's synthetic test rejects it and the turn +// falls through rather than titling a session with tool stdout. // // Pasted input arrives as `\n…the actual text…`, on a turn whose // origin.kind is "human" — so it must not be filtered like injected traffic, but the wrapper is @@ -740,9 +774,17 @@ func stripWrapperTag(s string) string { if i < 0 { return s } - // Reuse the same tag-name test the synthetic filter applies, so "looks like a harness tag" - // means one thing in this file. - if !isSyntheticPrompt(t[:i+1] + "x") { + // AN ALLOWLIST, not the structural test. Unwrapping anything tag-shaped promoted the body of + // whatever the harness had wrapped: `total 40` became the title + // "total 40", and a `` body became a title — the grep-dump outcome + // isSyntheticPrompt's own doc says it exists to prevent, and a contradiction of this PR's claim + // that markup all the way through falls through. + // + // Only wrappers whose CONTENT IS THE USER'S qualify. `` is the one: the user + // pasted what is inside it, so the body is a prompt. Everything else the harness emits is its + // own output — tool stdout, a reminder, a notification — and belongs to the next tier down, not + // in the title. + if !isContentBearingWrapper(t[:i+1]) { return s } inner := strings.TrimSpace(t[i+1:]) @@ -833,14 +875,38 @@ var harnessTagNames = []string{ // the anchored guard cannot see because the line starts with prose. Left alone the tag reached the // title and was clipped mid-tag at 80 runes. // -// Cuts at the FIRST known tag and keeps what precedes it — the prompt is the part the user typed, and -// everything the harness appends comes after. Returns s unchanged when nothing precedes the tag, so a -// turn that is markup all the way through still falls to isSyntheticPrompt rather than becoming "". +// Cuts at the first known tag THAT STARTS A LINE, and keeps what precedes it — the prompt is the part +// the user typed, and everything the harness appends comes after, on its own line. The positional +// constraint is what stops prose being truncated for merely mentioning a tag: the named set alone was +// not enough, and "how do I use in a skill?" came out as "how do I use". +// +// Returns s unchanged when nothing precedes the tag, so a turn that is markup all the way through +// still falls to isSyntheticPrompt rather than becoming "". func cutTrailingHarness(s string) string { cut := -1 for _, tag := range harnessTagNames { - if i := strings.Index(s, tag); i >= 0 && (cut < 0 || i < cut) { - cut = i + for from := 0; ; { + i := strings.Index(s[from:], tag) + if i < 0 { + break + } + i += from + from = i + len(tag) + // POSITIONAL. The harness appends its block on a line of its own, so a known tag only + // counts when it STARTS A LINE. Without this, prose that merely mentions a tag was + // truncated mid-sentence — "how do I use in a skill?" became "how do I + // use" — which is the very outcome the named set was chosen to avoid, and the comment + // above claimed it did. + // + // Every occurrence is examined, not just the first: a prompt may mention a tag inline + // and still have a real appended block after it. + if i > 0 && s[i-1] != '\n' && s[i-1] != '\r' { + continue + } + if cut < 0 || i < cut { + cut = i + } + break } } if cut <= 0 { diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 4fd5a2838..7eb98638e 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1412,14 +1412,26 @@ func TestTitleFromTranscript_PrefersLastPromptRecord(t *testing.T) { "/w/real", }, { - // A WRAPPED record is unwrapped, not discarded: the body is the prompt, same as for a - // pasted-content turn. Only a record that is markup all the way down falls through. - "a wrapped lastPrompt is stripped to its body", + // A HARNESS-OUTPUT wrapper falls through: its body is the harness's own text, not the + // user's, so promoting it would put a notification in the title. An earlier version of + // this test asserted the opposite — that the body becomes the title — which + // contradicted isSyntheticPrompt's own stated purpose and let + // `total 40` render as "total 40". + "a harness-output lastPrompt falls through", []string{ `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, `{"type":"last-prompt","lastPrompt":"body"}`, }, - "body", + "the real ask", + }, + { + // A CONTENT-BEARING wrapper is still unwrapped: the user pasted what is inside it. + "a pasted-content lastPrompt is stripped to its body", + []string{ + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"the real ask"}}`, + `{"type":"last-prompt","lastPrompt":"the pasted body"}`, + }, + "the pasted body", }, { "a lastPrompt that is markup all the way down falls through", @@ -1607,3 +1619,98 @@ func TestTitleFromTranscript_PrefilterIgnoresJSONSpacing(t *testing.T) { }) } } + +// A harness-output wrapper never has its body promoted to a title. +// +// stripWrapperTag used to unwrap anything tag-shaped, so `total 40` became +// the title "total 40" and a `` body became a title — the grep-dump outcome +// isSyntheticPrompt's doc says it exists to prevent. Only wrappers whose CONTENT IS THE USER'S are +// unwrapped now; see contentBearingWrappers. +func TestTitleFromTranscript_HarnessOutputWrappersFallThrough(t *testing.T) { + for _, tc := range []struct { + name, content, want string + }{ + {"bash-stdout", `total 40`, "/w/fallback"}, + {"bash-stderr", `No such file`, "/w/fallback"}, + {"system-reminder", `do not mention this`, "/w/fallback"}, + {"task-notification", `agent finished`, "/w/fallback"}, + {"local-command-caveat", `Caveat: generated`, "/w/fallback"}, + // The one wrapper whose body IS the user's, so it is still unwrapped. + {"pasted_content is still unwrapped", `the pasted body`, "the pasted body"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/fallback"}`, + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} + +// Prose that MENTIONS a known harness tag is not truncated; only a tag starting a line is a cut. +// +// THE COVERAGE GAP THIS CLOSES: every previous "untouched prose" case used an UNKNOWN tag —
, +// List, "3 < 5" — so they all took isSyntheticPrompt's structural path and never reached +// cutTrailingHarness's named set. Nothing put a known tag inside real prose, which is exactly how +// "how do I use in a skill?" came to be truncated to "how do I use". +func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { + for _, tc := range []struct { + name, content, want string + }{ + { + "known tag mid-sentence", + "how do I use in a skill?", + "how do I use in a skill?", + }, + { + "known tag mid-sentence, different tag", + "why does show up in my logs?", + "why does show up in my logs?", + }, + { + "known tag after a space is not a line start", + "see for details", + "see for details", + }, + { + // The real shape: the harness appends on its own line, which IS a cut. + "a tag starting a line is still cut", + "my real question\nhidden", + "my real question", + }, + { + // Every occurrence is examined, so an inline mention does not mask a later real block. + "an inline mention does not hide a later appended block", + "how do I use ?\nhidden", + "how do I use ?", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + if got != tc.want { + t.Errorf("title = %q, want %q", got, tc.want) + } + }) + } +} From e09c72854fbb07fbdbad3271675476af79463b0e Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 06:58:02 -0400 Subject: [PATCH 05/16] fix(abctl): Treat an indented harness block as line-leading, and cut within blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX, reported with a reproduction and confirmed here. cutTrailingHarness required s[i-1] to be exactly a newline, so a block indented by even one space was not recognised as line-leading and survived into the title: in: "my question\n Codebase instructions follow and you MUST... title: "my question Codebase instructions follow and you MUST obey them" That is the clipped-mid-tag failure this function's own doc describes. A line's own leading spaces and tabs now count as line-leading, exactly as suggested. The inline-mention protection is unaffected — "how do I use in a skill?" has real text before the tag on its line, so it still does not cut. The related mid-line-on-a-later-line case is left alone, per the reviewer's recommendation and now recorded as a test rather than as an accident: it is genuinely ambiguous between prose and appended markup, and cutting it would risk the mid-sentence truncation the positional rule exists to prevent. Also from review: trailing markup sharing ONE text block with prose reached the title, because isSyntheticPrompt is anchored and the block passed its per-block check whole. The cut is applied inside the join loop now, so a later block cannot be mistaken for an earlier one's appended markup. And a Unicode-escaped member name is admitted to the decoder: JSON permits `"lastPrompt"`, which decodes to lastPrompt but matches no literal in the raw-byte prefilter. Latent — 0 of 46,124 real lines write keys that way — and taken only because it is nearly free, just 0.2% of lines containing a `\u` escape at all. The fixture gap the reviewer named is closed on both counts: every appended-block case was unindented, so none exercised the whitespace path. Indented cases (space, tab, mixed) and same-block cases are added beside the existing ones. One test of mine was vacuous and is fixed here too. The escaped-key fixtures were written in interpreted string literals, so Go decoded `P` at compile time and the file contained a plain "lastPrompt" — the test passed with the fix removed. Rewritten with backtick concatenation so the six characters reach the file, and verified by mutation that all four cases now fail without the change. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 39 +++++++++- .../authlib/observe/claude/harvest_test.go | 73 +++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index a6ba5cfc9..287bf2c9d 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -470,7 +470,13 @@ func titleFromTranscript(path string) (string, error) { !bytes.Contains(line, []byte(`"agentName"`)) && !bytes.Contains(line, []byte(`"cwd"`)) && !bytes.Contains(line, []byte(`"lastPrompt"`)) && - !bytes.Contains(line, []byte(`"role"`)) { + !bytes.Contains(line, []byte(`"role"`)) && + // A UNICODE-ESCAPED member name would not match any literal above: JSON permits + // `"last\u0050rompt"`, which decodes to lastPrompt but is skipped by a raw-byte scan. + // Nothing observed writes keys that way — 0 of 46,124 real lines — so this is latent, + // and it is taken only because it is nearly free: just 0.2% of lines contain a `\u` + // escape at all, so admitting them costs a decode on one line in five hundred. + !bytes.Contains(line, []byte(`\u`)) { continue } var e transcriptMeta @@ -666,10 +672,18 @@ func promptFromMessage(raw json.RawMessage) (text string, wasString bool) { if isSyntheticPrompt(blk.Text) { continue } + // AND WITHIN the block. isSyntheticPrompt is anchored, so a block holding prose followed by + // markup — "my question\n…" as ONE block — passed the check and reached + // the title intact. Cut here rather than after the join, so a later block cannot be + // mistaken for the appended markup of an earlier one. + text := cutTrailingHarness(blk.Text) + if text == "" { + continue + } if b.Len() > 0 { b.WriteByte(' ') } - b.WriteString(blk.Text) + b.WriteString(text) } return b.String(), false } @@ -898,10 +912,27 @@ func cutTrailingHarness(s string) string { // use" — which is the very outcome the named set was chosen to avoid, and the comment // above claimed it did. // + // LEADING WHITESPACE STILL COUNTS AS LINE-LEADING. Requiring s[i-1] to be exactly a + // newline let an INDENTED block through: "my question\n …" kept the + // tag and was clipped mid-tag at 80 runes, which is the failure this function's own doc + // describes. Only spaces and tabs are skipped, so the constraint still holds against + // anything with real text before it on the line. + // + // A known tag mid-line on a LATER line — "line one\nline two x" — is + // deliberately left alone: it is genuinely ambiguous between prose and appended markup, + // and cutting it would risk the mid-sentence truncation the positional rule exists to + // prevent. + // // Every occurrence is examined, not just the first: a prompt may mention a tag inline // and still have a real appended block after it. - if i > 0 && s[i-1] != '\n' && s[i-1] != '\r' { - continue + if i > 0 { + j := i - 1 + for j >= 0 && (s[j] == ' ' || s[j] == '\t') { + j-- + } + if j >= 0 && s[j] != '\n' && s[j] != '\r' { + continue + } } if cut < 0 || i < cut { cut = i diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 7eb98638e..22fbd4d4b 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1533,6 +1533,37 @@ func TestTitleFromTranscript_NoHarnessMarkupInTitles(t *testing.T) { []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"my real question\ndo not mention this"}}`}, "my real question", }, + { + // INDENTED. Every appended-block fixture here was unindented, so none exercised the + // whitespace path: requiring s[i-1] to be exactly a newline let "my question\n + // …" keep its tag and get clipped mid-tag at 80 runes. + "a space-indented appended block is cut", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"my real question\n do not mention this"}}`}, + "my real question", + }, + { + "a tab-indented appended block is cut", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"my real question\n\tdo not mention this"}}`}, + "my real question", + }, + { + "a mixed-indent appended block is cut", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"my real question\n \t total 40"}}`}, + "my real question", + }, + { + // Prose and markup sharing ONE text block. isSyntheticPrompt is anchored, so the block + // passed the per-block check and reached the title whole; the cut is applied inside the + // join loop now. + "markup sharing a block with prose is cut", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":[{"type":"text","text":"my real question\nhidden"}]}}`}, + "my real question", + }, + { + "markup sharing a block with prose, unattributed turn", + []string{`{"type":"user","message":{"role":"user","content":[{"type":"text","text":"my real question\nhidden"}]}}`}, + "my real question", + }, { "a recorded lastPrompt is cut the same way", []string{`{"type":"last-prompt","lastPrompt":"recorded ask\nhidden"}`}, @@ -1695,6 +1726,15 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { "how do I use ?\nhidden", "how do I use ?", }, + { + // DELIBERATELY NOT CUT, recorded as a decision rather than left to chance: a known tag + // mid-line on a later line is genuinely ambiguous between prose and appended markup, + // and cutting it risks the mid-sentence truncation the positional rule exists to + // prevent. Only a line-leading tag, indentation aside, is treated as the harness's. + "a known tag mid-line on a later line is left alone", + "line one\nline two x", + "line one line two x", + }, } { t.Run(tc.name, func(t *testing.T) { dir := t.TempDir() @@ -1714,3 +1754,36 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { }) } } + +// A Unicode-escaped member name still reaches the decoder. +// +// JSON permits `"lastPrompt"`, which decodes to lastPrompt but matches no literal in the raw-byte +// prefilter, so the line was skipped and the session lost its title. Nothing observed writes keys that +// way — 0 of 46,124 real lines — so this is latent; it is taken because it is nearly free, only 0.2% of +// lines containing a `\u` escape at all. +func TestTitleFromTranscript_DecodesEscapedMemberNames(t *testing.T) { + for _, tc := range []struct { + name, line, want string + }{ + // BACKTICK-CONCATENATED so the \u sequences reach the FILE as six literal characters. An + // earlier version of this test wrote them inside a normal literal, where Go decoded them at + // compile time: the fixture then contained a plain "lastPrompt" and the test passed with the + // fix removed, testing nothing. Verified by mutation after the change. + {"escaped lastPrompt", `{"type":"last-prompt","last` + `\u0050` + `rompt":"review the change"}`, "review the change"}, + {"escaped aiTitle", `{"type":"ai-title","ai` + `\u0054` + `itle":"a generated title"}`, "a generated title"}, + {"escaped agentName", `{"type":"agent-name","agent` + `\u004E` + `ame":"sept-15-rossoctl"}`, "sept-15-rossoctl"}, + {"escaped cwd", `{"type":"user","c` + `\u0077` + `d":"/w/escaped"}`, "/w/escaped"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", tc.line) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("title = %q, want %q — the prefilter skipped an escaped key", got, tc.want) + } + }) + } +} From befa09e9e085eec3f1f8f2fcb0935e0aa3eeac4a Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 07:30:47 -0400 Subject: [PATCH 06/16] fix(abctl): Guarantee titles are plain text, instead of filtering markup shape by shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX, and a change of approach rather than another filter. Markup inside reached the title as "/review LEAK": unwrapCommandEnvelope changed the text, which short-circuited stripWrapperTag; the surviving tag was mid-line so cutTrailingHarness could not fire; and isSyntheticPrompt is anchored where it saw "/review". That is the sixth placement review has found, and the pattern is the problem: every filter guarded one position, and the positions are the harness's to choose. Enumerating them cannot converge. Worse, the harvester was emitting raw ESC sequences, C1 controls, bidi overrides and zero-width characters into ~/.cortex/session-metadata.json — safe only because the viewer runs sanitizeLabel over every cell, so the guarantee lived in one consumer and any other reader of the file got the raw bytes. clipTitle now normalises unconditionally, on every tier, so a title is plain text by construction: - stripHarnessSpans removes a known harness block AND ITS BODY, wherever it sits. Removing only the brackets promoted the payload: "my question\ninjected" became "my question injected". - stripANSI removes CSI and OSC sequences with their parameters. Dropping the ESC byte alone left "[31mred[0m". - stripTags removes any remaining <...> span. A lone "<" survives, because "is 3 < 5 in Go?" is a real prompt; a matched pair never does. - every rune must satisfy unicode.IsGraphic. An allowlist, not a denylist of known-bad ranges, so a category nobody enumerated cannot leak: controls, format characters, bidi overrides, ZWJ/ZWSP, variation selectors, surrogates and unassigned code points all go. Whitespace collapses to single spaces. The trade this accepts, stated plainly: a prompt that genuinely quotes markup loses it — "how do I write List in Go?" titles as "how do I write List in Go?". Titles are plain text with nothing hidden in them, and that cannot also be "faithfully quotes markup". Five tests asserting tags survive in titles are updated to assert they are stripped, including the mid-line-on-a-later-line case that was previously documented as a deliberate leak. The guarantee is asserted as a PROPERTY over a corpus of 34 inputs — every placement review found, plus control characters, invisible code points, and legitimate prose, CJK and emoji — rather than as another list of shapes. A second test pins what normalisation must not destroy, since "return empty for everything" would satisfy the first. Two gaps in my own test found by mutation while writing it: the property checked for tags and control characters but not for a harness BODY surviving, and the corpus contained no unknown tag, so deleting stripTags passed. Both closed; all four normalisation steps now fail the property test when removed. On the real tree: 134 titles, 0 with control/format/hidden code points, 0 containing a tag. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 172 +++++++++++++++- .../authlib/observe/claude/harvest_test.go | 183 ++++++++++++++++-- 2 files changed, 332 insertions(+), 23 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 287bf2c9d..9f150fa72 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" "time" + "unicode" ) // ConfigDirEnv is the variable Claude Code itself honours for relocating its config @@ -625,11 +626,52 @@ const maxTitleLen = 80 // display columns — and a marker added here would be re-truncated downstream, leaving a cell with // two of them. func clipTitle(s string) string { - // ONE LINE. A prompt is free text and may hold newlines or tabs; a title is a table cell, and - // the viewer sanitises control characters into U+FFFD rather than dropping them, so a raw - // newline would reach the cell as a visible replacement glyph. Collapsing runs of whitespace - // also stops a wrapped prompt spending its 80 characters on indentation. - s = strings.Join(strings.Fields(s), " ") + // STRIP ALL MARKUP FIRST, wherever it sits. Every earlier attempt filtered markup by SHAPE — + // anchored at the start, or line-leading, or per block — and each round of review found another + // shape that slipped past: markup inside , markup mid-line on a later line, a + // block indented with a vertical tab. Enumerating shapes cannot converge, because the shapes are + // the harness's to choose. + // + // So the rule here is positional-independent and applies to every tier: a title carries no tags + // at all. Callers still unwrap envelopes and drop harness-output wrappers, because those + // decisions are about WHICH TEXT to use; this is about what may appear in the result. + s = stripHarnessSpans(s) + s = stripANSI(s) + s = stripTags(s) + + // ONE LINE, and only characters that print as themselves. + // + // The old version collapsed whitespace and stopped there, leaving ESC sequences, C1 controls, + // bidi overrides and zero-width characters in the written file. That was safe only because the + // viewer runs sanitizeLabel over every cell — so the guarantee lived in one consumer, and + // anything else reading ~/.cortex/session-metadata.json got the raw bytes. A title is meant to + // be plain text with nothing hidden in it, which has to be true of the FILE, not of one reader. + // + // An ALLOWLIST, not a denylist of known-bad ranges: unicode.IsGraphic is false for every + // control, format, surrogate and unassigned code point, so a category nobody enumerated cannot + // leak through. Whitespace is normalised to a single space and everything else non-graphic is + // dropped rather than replaced, since a run of U+FFFD tells a reader nothing. + var b strings.Builder + b.Grow(len(s)) + prevSpace := true // leading whitespace is dropped + for _, r := range s { + switch { + case unicode.IsSpace(r): + if !prevSpace { + b.WriteByte(' ') + prevSpace = true + } + case !unicode.IsGraphic(r): + // Controls (C0/C1/DEL), format characters — bidi overrides and isolates, ZWJ, ZWSP, + // variation selectors — surrogates and unassigned code points. None of these print as + // themselves, and the bidi ones actively reorder what surrounds them. + default: + b.WriteRune(r) + prevSpace = false + } + } + s = strings.TrimSpace(b.String()) + r := []rune(s) if len(r) <= maxTitleLen { return s @@ -637,6 +679,126 @@ func clipTitle(s string) string { return strings.TrimSpace(string(r[:maxTitleLen])) } +// stripANSI removes a CSI/OSC escape sequence together with its parameters. +// +// Dropping the ESC byte alone — which the non-graphic filter below does — leaves the payload behind: +// "\x1b[31mred\x1b[0m" became "[31mred[0m", which is not dangerous but is not a title either. The +// whole sequence goes, so the result reads as what the user wrote. +// +// Handles the two forms that carry parameters: CSI (ESC [ … final byte in @-~) and OSC +// (ESC ] … terminated by BEL or ST). Any other escape is left to the non-graphic filter, which drops +// the ESC and leaves at most one stray character. +func stripANSI(s string) string { + if !strings.ContainsRune(s, 0x1b) { + return s + } + var b strings.Builder + b.Grow(len(s)) + rs := []rune(s) + for i := 0; i < len(rs); i++ { + if rs[i] != 0x1b || i+1 >= len(rs) { + b.WriteRune(rs[i]) + continue + } + switch rs[i+1] { + case '[': // CSI: parameters, then a final byte in the range @ to ~ + j := i + 2 + for j < len(rs) && (rs[j] < '@' || rs[j] > '~') { + j++ + } + i = j // the final byte is consumed too + case ']': // OSC: runs to BEL, or to ST (ESC \) + j := i + 2 + for j < len(rs) && rs[j] != 0x07 { + if rs[j] == 0x1b && j+1 < len(rs) && rs[j+1] == '\\' { + j++ + break + } + j++ + } + i = j + default: + // A two-character escape, or something unrecognised. Skip the ESC and let the loop + // handle the next rune normally. + } + } + return b.String() +} + +// stripHarnessSpans removes a known harness block AND ITS BODY, wherever it sits. +// +// stripTags alone deletes the angle-bracket spans and leaves what was between them, so +// "my question\nLEAK" became "my question LEAK" — the tags gone +// and the injected text promoted into the title. For a harness block the BODY is the payload, so the +// whole span has to go. +// +// Only the named set is removed with its body: those tags are the harness's, so their content is +// never the user's. An unknown tag keeps its body, because there the text between the brackets may +// well be the prompt — stripTags then removes the brackets alone. +// +// Position-independent, and applied before stripTags, so a block anywhere in the string is handled: +// at the start, appended on an indented line, mid-line on a later line, or nested inside a +// value. That last one is the leak review found this round; the rest are the leaks it +// found in the rounds before. +func stripHarnessSpans(s string) string { + for _, tag := range harnessTagNames { + for { + i := strings.Index(s, tag) + if i < 0 { + break + } + // The span runs to the matching close when there is one, and to the end of the opening + // tag otherwise — a self-closing or unterminated block leaves nothing to keep. + end := len(s) + name := tag[1:] // tag is "= 0 { + if g := strings.IndexByte(s[i+c:], '>'); g >= 0 { + end = i + c + g + 1 + } + } else if g := strings.IndexByte(s[i:], '>'); g >= 0 { + end = i + g + 1 + } + s = s[:i] + " " + s[end:] + } + } + return s +} + +// stripTags removes every <...> span from s, wherever it appears. +// +// POSITION-INDEPENDENT, which is the point: the shape-by-shape filters above it each guard one +// placement, and review found a new placement each round. This one cannot be flanked. +// +// Unbalanced or nested markup is handled by construction, since it only ever deletes from a "<" to +// the next ">" and leaves a lone "<" alone. That last part is deliberate: "is 3 < 5 in Go?" and +// "List" are the shapes a real prompt carries, and the first must survive. The second is +// sacrificed — a prompt that genuinely quotes a tag loses it — which is the trade this asks for: +// titles are plain, and a plain title cannot also faithfully quote markup. +func stripTags(s string) string { + if !strings.ContainsRune(s, '<') { + return s + } + var b strings.Builder + b.Grow(len(s)) + depth := 0 + for _, r := range s { + switch { + case r == '<': + depth++ + case r == '>' && depth > 0: + depth-- + case depth == 0: + b.WriteRune(r) + } + } + // A lone "<" with no closing ">" left depth > 0 and swallowed the tail, which would turn + // "is 3 < 5" into "is 3 ". Fall back to the original in that case: nothing was a tag. + if depth > 0 { + return s + } + return b.String() +} + // promptFromMessage returns the text a person typed in one user turn, and whether the content was // a plain STRING rather than an array of blocks. // diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 22fbd4d4b..a6abe96fb 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -12,6 +12,7 @@ import ( "sync" "testing" "time" + "unicode" "unicode/utf8" ) @@ -1570,16 +1571,24 @@ func TestTitleFromTranscript_NoHarnessMarkupInTitles(t *testing.T) { "recorded ask", }, { - // Only KNOWN harness tags cut, so a prompt that quotes markup survives — the cut - // removes text mid-prompt, so a loose rule would truncate legitimate questions. - "a prompt quoting unknown markup is untouched", + // A TAG IS REMOVED WHEREVER IT APPEARS, including one a real prompt quotes. Titles are + // plain text with nothing hidden in them, and that cannot also be "faithfully quotes + // markup" — every shape-based filter this replaced was flanked by the next round of + // review. The prompt stays readable, which is what the title is for. + "a quoted tag is stripped, prose survives", []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"why does
break my layout?"}}`}, - "why does
break my layout?", + "why does break my layout?", }, { - "a prompt mentioning generics is untouched", + "generics are stripped, prose survives", []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"how do I write List in Go?"}}`}, - "how do I write List in Go?", + "how do I write List in Go?", + }, + { + // A LONE "<" is not a tag and must survive: this is the shape a real prompt carries. + "a comparison is not markup", + []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"is 3 < 5 in Go?"}}`}, + "is 3 < 5 in Go?", }, } { t.Run(tc.name, func(t *testing.T) { @@ -1700,19 +1709,22 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { name, content, want string }{ { - "known tag mid-sentence", - "how do I use in a skill?", + // These used to assert the tag SURVIVES, on the reasoning that cutting mid-sentence was + // worse than leaking a tag. That trade is gone: the tag is stripped and the surrounding + // prose is kept, so neither the truncation nor the leak happens. + "known tag mid-sentence is stripped, not cut", "how do I use in a skill?", + "how do I use in a skill?", }, { "known tag mid-sentence, different tag", "why does show up in my logs?", - "why does show up in my logs?", + "why does show up in my logs?", }, { - "known tag after a space is not a line start", - "see for details", + "known tag after a space", "see for details", + "see for details", }, { // The real shape: the harness appends on its own line, which IS a cut. @@ -1724,16 +1736,16 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { // Every occurrence is examined, so an inline mention does not mask a later real block. "an inline mention does not hide a later appended block", "how do I use ?\nhidden", - "how do I use ?", + "how do I use ?", }, { - // DELIBERATELY NOT CUT, recorded as a decision rather than left to chance: a known tag - // mid-line on a later line is genuinely ambiguous between prose and appended markup, - // and cutting it risks the mid-sentence truncation the positional rule exists to - // prevent. Only a line-leading tag, indentation aside, is treated as the harness's. - "a known tag mid-line on a later line is left alone", + // THE LAST DOCUMENTED LEAK, now closed. This was left alone deliberately, as a trade + // against mid-sentence truncation — a known tag mid-line on a later line being + // ambiguous between prose and appended markup. Stripping the span removes the need to + // judge: the harness's block and its body go, the prose stays. + "a known tag mid-line on a later line is stripped", "line one\nline two x", - "line one line two x", + "line one line two", }, } { t.Run(tc.name, func(t *testing.T) { @@ -1787,3 +1799,138 @@ func TestTitleFromTranscript_DecodesEscapedMemberNames(t *testing.T) { }) } } + +// EVERY title is plain text: no markup, no control characters, no hidden code points. +// +// THE GUARANTEE, asserted as a property rather than as a list of shapes. Six rounds of review each +// found a new placement that the shape-based filters missed — markup inside , mid-line +// on a later line, a block indented with a vertical tab — because enumerating placements cannot +// converge when the placements are the harness's to choose. clipTitle now normalises unconditionally, +// and this checks the outcome for every input rather than each route into it. +// +// The corpus deliberately mixes real prompts with adversarial ones: a title is LLM-generated text +// read from a file nothing authenticates, so "would a hostile transcript do this" is not the +// question — the question is what a title may contain. +func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { + inputs := []string{ + // Harness markup in every placement review found, plus the ones it did not. + `injected instructions`, + "my question\ninjected", + "my question\n injected", + "my question\n\tinjected", + "my question\n\vinjected", + "my question\n injected", + "my question\n　injected", + "line one\nline two injected", + "review\n/review\ninjected", + `injected`, + `total 40`, + `nested`, + // UNKNOWN tags, which only stripTags removes — the corpus had none, so a mutation deleting + // that step passed. Same fixture blindness as the earlier rounds: the inputs excluded the + // exact path the code was meant to cover. + "why does
break my layout?", + "how do I write List in Go?", + "a bold claim", + "body", + // Control characters and invisible code points. + "colour \x1b[31mred\x1b[0m here", + "osc \x1b]0;evil\x07 here", + "nel \u0085 here", + "csi \u009b here", + "del \x7f here", + "bidi ‮ reversed", + "isolate ⁦ x ⁩ y", + "zwsp a​b", + "zwj a‍b", + "vs16 a️b", + "nul \x00 here", + "tab\tand\nnewline", + // Legitimate content, which must survive as readable text. + "how do I build abctl?", + "is 3 < 5 in Go?", + "日本語のセッションタイトルです", + "ship it 🎉", + "café naïve", + "/review some/path.md", + } + for _, in := range inputs { + dir := t.TempDir() + body, err := json.Marshal(in) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/fallback"}`, + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatalf("input %q: %v", in, gerr) + } + + // No angle brackets that could read as a tag. A lone "<" survives — "is 3 < 5" is a real + // prompt — but never a matched pair. + if strings.Contains(got, ">") { + t.Errorf("input %q: title carries a closing bracket: %q", in, got) + } + // No harness tag name, in any form. + for _, tag := range harnessTagNames { + if strings.Contains(got, strings.TrimPrefix(tag, "<")) { + t.Errorf("input %q: title carries %s: %q", in, tag, got) + } + } + // AND NOT THE BODY EITHER. Stripping the brackets alone promoted the payload into the + // title — "my question\ninjected" became + // "my question injected" — which is the whole point of removing a harness span rather than + // just its tags. Checking only for tags left this test blind to it, and the mutation that + // exposed the gap passed until this assertion existed. + if strings.Contains(got, "injected") || strings.Contains(got, "total 40") { + t.Errorf("input %q: title carries a harness BODY: %q", in, got) + } + // Every rune prints as itself: no controls, format characters, surrogates or unassigned. + for _, r := range got { + if r == ' ' { + continue + } + if !unicode.IsGraphic(r) { + t.Errorf("input %q: title carries non-graphic %U: %q", in, r, got) + } + } + // One line, no runs of whitespace, no leading or trailing space. + if got != strings.TrimSpace(got) { + t.Errorf("input %q: title is not trimmed: %q", in, got) + } + if strings.Contains(got, " ") { + t.Errorf("input %q: title has a double space: %q", in, got) + } + // Bounded, and valid UTF-8 — never cut mid-character. + if n := len([]rune(got)); n > maxTitleLen { + t.Errorf("input %q: title is %d runes: %q", in, n, got) + } + if !utf8.ValidString(got) { + t.Errorf("input %q: title is not valid UTF-8: %q", in, got) + } + } +} + +// Legitimate prompts survive the normalisation as readable text. +// +// The guarantee above would also be satisfied by returning "" for everything, so this is the other +// half of it: what the normalisation must NOT destroy. +func TestClipTitle_KeepsLegitimateText(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + {"how do I build abctl?", "how do I build abctl?"}, + {"is 3 < 5 in Go?", "is 3 < 5 in Go?"}, + {"日本語のセッションタイトル", "日本語のセッションタイトル"}, + {"ship it 🎉", "ship it 🎉"}, + {"café naïve", "café naïve"}, + {"/review some/path.md", "/review some/path.md"}, + {" leading and trailing ", "leading and trailing"}, + {"collapses\n\n\tinner whitespace", "collapses inner whitespace"}, + {"colour \x1b[31mred\x1b[0m here", "colour red here"}, + } { + if got := clipTitle(tc.in); got != tc.want { + t.Errorf("clipTitle(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} From e3f8ced9dce548e5cefd9a1d67895d7fd021ddfc Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 07:45:45 -0400 Subject: [PATCH 07/16] fix(abctl): Drop binding characters so the cut is safe, and normalise the cwd too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review items, and one of them turned out to be a live bug rather than a comment defect. clipTitle cut mid-grapheme-cluster, leaving a dangling combining mark or half a regional-indicator flag. Fixed by DISCARDING the characters that bind to their neighbour — non-spacing and enclosing marks, modifier symbols, regional indicators — rather than by adding grapheme segmentation this module has no library for. One rune is then one grapheme and a plain rune slice cannot split a cluster. The cost is an accent on a decomposed sequence and a flag reduced to nothing; precomposed forms are unaffected, so "café naïve" still reads as itself. THE CWD FALLBACK WAS NOT NORMALISED AT ALL. Writing the comment for item four is what exposed it: the comment was going to say the cwd is still normalised, and it was not — that tier returned through a bare strings.Fields, so a directory name carrying an ESC sequence, a bidi override or a tag reached the file unfiltered while every other tier was clean. A guarantee that holds for three tiers out of four is not one. normalizeTitle is now split out of clipTitle and the cwd uses it, keeping its full leaf but losing the hidden characters. The cwd comment itself claimed left-truncation is a property of being a cwd. It is a property of the LEADING SLASH: prompt and cwd return through the same string, so the renderer decides by the first character — and since this change a "/review …" prompt takes that branch too. The narrower true reason is that a path is bounded by the filesystem where a prompt is not. truncLeft's doc summary said it clips to n runes while the body measures display columns. Corrected. The claim that the pane "measures with lipgloss.Width" was incomplete: bubbles v1.0.0 runs runewidth.Truncate over every cell before styling, and runewidth is not ANSI-aware. So the column measurement is sufficient only while the cell is plain. Recorded in both files, and now asserted — a renderer test checks title cells carry no escape byte at three widths under a real colour profile, so the pair of facts the measurement depends on is held by a test rather than by prose. The sc.Err() contract was documented in two places and asserted nowhere. Two tests: one that a line past the scanner buffer is reported while the title found before the stop still comes back, and one that ReadSessions collects it into Partial without failing the harvest or losing the other session. On the real tree: 134 titles, 0 carrying control, format, combining or modifier code points, 0 containing a tag. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 65 ++++++++-- .../authlib/observe/claude/harvest_test.go | 116 ++++++++++++++++++ authbridge/cmd/abctl/tui/sessions_pane.go | 10 +- .../cmd/abctl/tui/sessions_title_test.go | 31 +++++ 4 files changed, 214 insertions(+), 8 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 9f150fa72..f17964619 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -595,11 +595,20 @@ func titleFromTranscript(path string) (string, error) { // titles, so the column stops telling them apart — exactly what it is for. Clipping the cwd // was also a regression against the behaviour before this change, which never truncated here. // - // Safe to leave long because the renderer already truncates a path FROM THE LEFT, keeping the - // tail (see the viewer's truncLeft): the cap exists to bound prompts, which are unbounded free - // text, not paths, which are bounded by the filesystem. Whitespace is still collapsed, so a - // cwd cannot carry a control character into a cell. - return strings.Join(strings.Fields(cwd), " "), err + // LEFT-TRUNCATION IS A PROPERTY OF THE LEADING SLASH, NOT OF BEING A CWD, and this function + // cannot convey which is which: a prompt and a cwd come back through the same string, so the + // renderer decides by looking at the first character. A cwd therefore keeps its tail only + // because it starts with "/" — and since this change a "/review …" prompt takes that branch + // too, while a relative cwd would not. An earlier version of this comment claimed the renderer + // truncates "a path" from the left, which overstated what it can know. + // + // So the reason to leave it long is narrower: a path is bounded by the filesystem, where a + // prompt is unbounded free text, and the cap exists for the latter. The cell stays bounded + // either way, because the renderer truncates whatever it is handed. + // + // It is still normalised — whitespace collapsed, markup and non-graphic runes dropped — so a + // cwd cannot carry hidden characters into a cell any more than a prompt can. + return normalizeTitle(cwd), err } // maxTitleLen caps a harvested title, in RUNES. @@ -613,6 +622,14 @@ func titleFromTranscript(path string) (string, error) { // width. It is safe only because every renderer re-truncates by display width — the sessions pane // measures with lipgloss.Width. // +// THAT IS NOT THE ONLY RULER THE CELL MEETS, and saying only "the pane measures with lipgloss.Width" +// was incomplete: bubbles v1.0.0 applies runewidth.Truncate to every cell before styling it, and +// runewidth is NOT ANSI-aware. Today that is harmless, because a title cell carries no escape bytes — +// this package guarantees it, and the pane asserts it. But the two facts are load-bearing together: if +// a title were ever styled, the escape bytes would be charged against the column budget and an +// 11-column cell would collapse to a lone ellipsis. Anyone adding colour to a title needs to know that +// before they do it, which is why it is recorded here rather than left to be rediscovered. +// // THAT RELATIONSHIP IS NOT GUARDED BY ANY SINGLE TEST, and an earlier version of this comment // implied otherwise. It spans two modules: this package has no width library, so its test asserts // only that the cap is a rune count and not a byte or width bound, while the re-truncation lives in @@ -625,7 +642,14 @@ const maxTitleLen = 80 // No ellipsis: this is not the display truncation — the TITLE column applies its own, measured in // display columns — and a marker added here would be re-truncated downstream, leaving a cell with // two of them. -func clipTitle(s string) string { +// normalizeTitle makes s plain: no markup, no characters that fail to print as themselves, one line. +// +// SEPARATED FROM THE LENGTH CAP so the cwd fallback can have the guarantee without the clipping. +// Before this it returned through a bare strings.Fields, which collapsed whitespace and nothing +// else — so a directory name carrying an ESC sequence, a bidi override or a tag reached the file +// unfiltered, while every other tier was clean. A guarantee that holds for three tiers out of four +// is not one. +func normalizeTitle(s string) string { // STRIP ALL MARKUP FIRST, wherever it sits. Every earlier attempt filtered markup by SHAPE — // anchored at the start, or line-leading, or per block — and each round of review found another // shape that slipped past: markup inside , markup mid-line on a later line, a @@ -665,13 +689,40 @@ func clipTitle(s string) string { // Controls (C0/C1/DEL), format characters — bidi overrides and isolates, ZWJ, ZWSP, // variation selectors — surrogates and unassigned code points. None of these print as // themselves, and the bidi ones actively reorder what surrounds them. + case unicode.Is(unicode.Mn, r), unicode.Is(unicode.Me, r), unicode.Is(unicode.Sk, r): + // COMBINING AND MODIFYING characters: non-spacing marks, enclosing marks, and modifier + // symbols (skin tones). Dropped rather than kept, which is what lets the length cut + // below be a plain rune slice. + // + // They only ever attach to the character before them, so a cut that lands between the + // two leaves a dangling mark on whatever now precedes it — or a base character shorn of + // its accent. Keeping them would mean measuring in grapheme clusters, which needs a + // segmentation library this module does not have. Dropping them costs an accent + // ("café" titles as "cafe") and keeps this function simple, which is the trade the + // rest of it already makes. + case r >= 0x1F1E6 && r <= 0x1F1FF: + // REGIONAL INDICATORS, which only carry meaning in pairs: a cut between them turns a + // flag into a lone letter glyph. Two runes, never independently meaningful, so they go + // together or not at all. default: b.WriteRune(r) prevSpace = false } } - s = strings.TrimSpace(b.String()) + return strings.TrimSpace(b.String()) +} +// clipTitle normalises s and caps it at maxTitleLen runes. +// +// What every tier but the cwd fallback goes through; that one uses normalizeTitle alone, because a +// path's distinguishing end is its leaf and clipping keeps the head. +// +// A PLAIN RUNE CUT IS SAFE HERE, because normalizeTitle removed every character that binds to its +// neighbour: combining marks, modifiers, joiners and regional indicators are all gone, so one rune +// is one grapheme and the slice cannot land inside a cluster. Without that a cut could leave a +// dangling accent or half a flag. +func clipTitle(s string) string { + s = normalizeTitle(s) r := []rune(s) if len(r) <= maxTitleLen { return s diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index a6abe96fb..e2a2ea974 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1934,3 +1934,119 @@ func TestClipTitle_KeepsLegitimateText(t *testing.T) { } } } + +// The cwd fallback is plain too, and keeps its leaf. +// +// It returned through a bare strings.Fields — whitespace collapsed and nothing else — so a directory +// name carrying an ESC sequence, a bidi override or a tag reached the file unfiltered while every +// other tier was clean. It now shares normalizeTitle with them, but NOT the length cap: a path's +// distinguishing end is its leaf, and clipping keeps the head. +func TestTitleFromTranscript_CwdFallbackIsPlainAndUnclipped(t *testing.T) { + titleFor := func(cwd string) string { + t.Helper() + dir := t.TempDir() + body, err := json.Marshal(cwd) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", `{"type":"user","cwd":`+string(body)+`}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + return got + } + + for _, in := range []string{ + "/w/dir\x1b[31mred", + "/w/‮dir", + "/w/injecteddir", + "/w/áccent", + "/w/norm​al", + } { + got := titleFor(in) + for _, r := range got { + if r != ' ' && !unicode.IsGraphic(r) { + t.Errorf("cwd %q: title carries non-graphic %U: %q", in, r, got) + } + } + if strings.Contains(got, ">") { + t.Errorf("cwd %q: title carries markup: %q", in, got) + } + if strings.Contains(got, "injected") { + t.Errorf("cwd %q: title carries a harness body: %q", in, got) + } + } + + // NOT clipped: a prefix past the cap must not swallow the leaf, or sibling worktrees become + // indistinguishable. + const prefix = "/Users/somebody/go/src/github.com/some-organisation/some-repository/worktrees/wt/" + if len([]rune(prefix)) <= maxTitleLen { + t.Fatalf("fixture prefix is %d runes, needs to exceed %d", len([]rune(prefix)), maxTitleLen) + } + a, b := titleFor(prefix+"alpha"), titleFor(prefix+"beta") + if a == b { + t.Errorf("sibling paths produced the same title %q", a) + } + if !strings.HasSuffix(a, "alpha") || !strings.HasSuffix(b, "beta") { + t.Errorf("the leaf did not survive: %q / %q", a, b) + } +} + +// A truncated read is REPORTED, not silently swallowed, and the best title found still comes back. +// +// titleFromTranscript's second return says the scan ended early — a line past the 16MB buffer, or an +// I/O error partway through — and ReadSessions collects those into Result.Partial for the caller to +// warn about. The contract was documented in two places and asserted nowhere, so a regression would +// have been silent: the title of a long session would quietly become a stale one. +func TestTitleFromTranscript_ReportsTruncatedReads(t *testing.T) { + dir := t.TempDir() + // One line past bufio.Scanner's 16MB ceiling, after a usable title. The title found before the + // stop must survive, since something is better than nothing — that is why the error is returned + // ALONGSIDE it rather than instead of it. + huge := strings.Repeat("x", 17<<20) + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"a real ask"}}`, + `{"type":"user","cwd":"/w/`+huge+`"}`) + + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err == nil { + t.Error("a line past the scanner buffer was not reported") + } + if got != "a real ask" { + t.Errorf("title = %q, want the title found before the stop", got) + } +} + +// ReadSessions surfaces a truncated transcript through Result.Partial rather than failing the harvest. +// +// One bad transcript must not cost the other hundred names, so the error is collected and the session +// still lands in the map. The propagation was untested. +func TestReadSessions_CollectsPartialReads(t *testing.T) { + cfg := t.TempDir() + proj := filepath.Join(cfg, "projects", "-p") + huge := strings.Repeat("x", 17<<20) + writeSessionTranscript(t, proj, "truncated.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"a real ask"}}`, + `{"type":"user","cwd":"/w/`+huge+`"}`) + writeSessionTranscript(t, proj, "fine.jsonl", + `{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"another ask"}}`) + + out, partial, _, err := ReadSessions(cfg, nil) + if err != nil { + t.Fatalf("a truncated transcript failed the whole harvest: %v", err) + } + if len(partial) != 1 { + t.Errorf("partial = %d entries, want 1: %v", len(partial), partial) + } + if len(partial) == 1 && !strings.Contains(partial[0], "truncated.jsonl") { + t.Errorf("the partial entry does not name the transcript: %q", partial[0]) + } + // Both sessions are still named — the truncated one from what was found before the stop. + if out["truncated"].Title != "a real ask" { + t.Errorf("truncated session title = %q, want %q", out["truncated"].Title, "a real ask") + } + if out["fine"].Title != "another ask" { + t.Errorf("healthy session title = %q, want %q", out["fine"].Title, "another ask") + } +} diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index 2db4b1a4e..60f6b68ca 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -482,7 +482,8 @@ func truncRight(s string, n int) string { return "…" } -// truncLeft clips s to n runes keeping the RIGHT end, marking the cut with a leading ellipsis. +// truncLeft clips s to n DISPLAY COLUMNS keeping the RIGHT end, marking the cut with a leading +// ellipsis. // // The mirror of trunc, for values whose distinguishing end is the last one: a path, where every // sibling shares the prefix. n < 1 yields "" and n == 1 yields just the ellipsis, so the result @@ -500,6 +501,13 @@ func truncLeft(s string, n int) string { // exactly the titles that need it. // // lipgloss.Width, mirroring padLeft, which measures this way for the same reason. + // + // 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 (authlib/observe/claude normalises every 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 { return s } diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 89711f4e2..fdcef7c9d 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -802,3 +802,34 @@ func TestTrunc_BudgetsInDisplayColumnsAndKeepsASCIIIdentical(t *testing.T) { } } } + +// A rendered TITLE cell carries no ANSI, which is what makes the column measurement sufficient. +// +// bubbles v1.0.0 runs runewidth.Truncate over every cell before styling, and runewidth does not skip +// escape sequences. Measuring in display columns here is therefore only safe while the cell is PLAIN: +// escape bytes would be charged against the budget and a narrow cell would collapse to a lone +// ellipsis. The harvester guarantees plain titles; this asserts the renderer does not reintroduce +// styling, so the pair of facts the comment on truncLeft depends on is actually held by a test. +func TestSessionTitleCell_CarriesNoANSI(t *testing.T) { + forceColor(t) // styling real, so a Render that added escapes would show up here + + const id = "s1" + for _, title := range []string{ + "a plain prose title", + "/Users/somebody/src/cortex/.worktrees/a-long-name/authbridge", + "日本語のセッションタイトルです", + "ship it 🎉", + } { + m := newTitleModel(t, map[string]SessionMetadata{id: {Title: title}}, id) + for _, w := range []int{11, 20, 40} { + got := m.sessionTitleCell(id, w) + if strings.ContainsRune(got, 0x1b) { + t.Errorf("title cell carries an escape byte at width %d: %q", w, got) + } + if lipgloss.Width(got) > w { + t.Errorf("title cell is %d columns against a %d-column budget: %q", + lipgloss.Width(got), w, got) + } + } + } +} From 806d7db4192f959c3caefab55bb9058ce552701b Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 08:11:09 -0400 Subject: [PATCH 08/16] fix(abctl): Keep a slash command's name, and guard the rune cap across modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items, and the second found a third thing. sessions_pane.go discriminated prose from paths on a bare leading "/", which was the whole story until this PR made the user's own prompts a title source. A typed slash command begins with one too, so "/review carefully" was left-truncated to "…pull/1101 carefully" — discarding the command name, the one part a reader needs, and inverting this file's own rule that prose reads left-to-right. On the local tree 23 of 134 titles are slash commands, so this was not a corner case. looksLikePath now asks whether the LEAF is the identifying part: a title qualifies only if it has a second "/" before any space. "/Users/somebody/src" does; "/review some/path.md" does not. Deliberately wrong in one direction — "/tmp foo" reads as prose — because a single-segment cwd is not something Claude Code records, and the cost is a cell cut at the other end rather than anything unbounded. The cross-module cap guard is added from the renderer side, which is the only side that can see both halves. MaxTitleLen is exported for it: while it was package-private neither module could name the other's part of the contract, so each tested its own half and deleting the renderer's truncation broke no test. The guard takes a title at exactly the cap in the worst case for the mismatch — MaxTitleLen runes of CJK, twice that in columns — and requires the rendered cell to fit anyway, at three terminal widths. Verified by mutation: removing the renderer's truncation now fails it seven times where previously nothing failed. Writing that guard exposed a bug in my own fixture rather than in the code. The first version installed a 100-column header while the model was 200 wide, then asserted against the 100-column budget; rebuildSessionsTable correctly reads the width it is about to install, so it produced a 107-column cell and the test called that a defect. The budget now comes from the model's own width, and the reasoning is recorded beside it. Also adds a direct test for looksLikePath, which the discriminator change would otherwise have had none: reverting it to the bare slash check passed every existing test. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 18 ++- .../authlib/observe/claude/harvest_test.go | 32 ++--- authbridge/cmd/abctl/tui/sessions_pane.go | 28 +++- .../cmd/abctl/tui/sessions_title_test.go | 125 ++++++++++++++++++ 4 files changed, 179 insertions(+), 24 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index f17964619..d07a0291c 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -570,7 +570,7 @@ func titleFromTranscript(path string) (string, error) { // 128 sessions had no title line of either kind and fell through to a cwd, and every one of // those has a usable prompt. // - // Clipped at the source. A prompt is unbounded and a title is a table cell; see maxTitleLen. + // Clipped at the source. A prompt is unbounded and a title is a table cell; see MaxTitleLen. // NORMALISED BEFORE the switch, not inside each arm. A whitespace-only candidate passes a // bare `!= ""` and clipTitle then empties it, so the tier below was skipped and the cell came // out blank — testing the clipped value is what makes each guard mean "this tier has @@ -611,7 +611,11 @@ func titleFromTranscript(path string) (string, error) { return normalizeTitle(cwd), err } -// maxTitleLen caps a harvested title, in RUNES. +// MaxTitleLen caps a harvested title, in RUNES. +// +// EXPORTED so the renderer's tests can hold the cross-module contract: the cap is only safe because +// every renderer re-truncates by display width, and while it was package-private neither side could +// name the other's half. cmd/abctl/tui asserts the relationship against this constant. // // A prompt is unbounded — the longest on the measured tree ran to several KB — and a title is a // table cell. Clipping at the source keeps the metadata file small and stops every consumer having @@ -635,9 +639,9 @@ func titleFromTranscript(path string) (string, error) { // only that the cap is a rune count and not a byte or width bound, while the re-truncation lives in // cmd/abctl/tui and is tested there against its own column budgets. Nothing fails if someone // removes the renderer's truncation and leaves this constant alone. -const maxTitleLen = 80 +const MaxTitleLen = 80 -// clipTitle trims s and caps it at maxTitleLen runes. +// clipTitle trims s and caps it at MaxTitleLen runes. // // No ellipsis: this is not the display truncation — the TITLE column applies its own, measured in // display columns — and a marker added here would be re-truncated downstream, leaving a cell with @@ -712,7 +716,7 @@ func normalizeTitle(s string) string { return strings.TrimSpace(b.String()) } -// clipTitle normalises s and caps it at maxTitleLen runes. +// clipTitle normalises s and caps it at MaxTitleLen runes. // // What every tier but the cwd fallback goes through; that one uses normalizeTitle alone, because a // path's distinguishing end is its leaf and clipping keeps the head. @@ -724,10 +728,10 @@ func normalizeTitle(s string) string { func clipTitle(s string) string { s = normalizeTitle(s) r := []rune(s) - if len(r) <= maxTitleLen { + if len(r) <= MaxTitleLen { return s } - return strings.TrimSpace(string(r[:maxTitleLen])) + return strings.TrimSpace(string(r[:MaxTitleLen])) } // stripANSI removes a CSI/OSC escape sequence together with its parameters. diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index e2a2ea974..216f03eb2 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -979,8 +979,8 @@ func TestTitleFromTranscript_ClipsAndFlattens(t *testing.T) { wantRunes int wantOneLine bool }{ - {"long ascii", long, maxTitleLen, true}, - {"long CJK is cut by rune, not byte", cjk, maxTitleLen, true}, + {"long ascii", long, MaxTitleLen, true}, + {"long CJK is cut by rune, not byte", cjk, MaxTitleLen, true}, {"newlines collapse", "first line\n\nsecond line\twith a tab", -1, true}, } { t.Run(tc.name, func(t *testing.T) { @@ -995,8 +995,8 @@ func TestTitleFromTranscript_ClipsAndFlattens(t *testing.T) { if gerr != nil { t.Fatal(gerr) } - if n := len([]rune(got)); n > maxTitleLen { - t.Errorf("title is %d runes, over the %d cap: %q", n, maxTitleLen, got) + if n := len([]rune(got)); n > MaxTitleLen { + t.Errorf("title is %d runes, over the %d cap: %q", n, MaxTitleLen, got) } if tc.wantRunes > 0 && len([]rune(got)) != tc.wantRunes { t.Errorf("title is %d runes, want %d", len([]rune(got)), tc.wantRunes) @@ -1153,9 +1153,9 @@ func TestTitleFromTranscript_UnwrapsSlashCommands(t *testing.T) { func TestTitleFromTranscript_DoesNotClipTheCwdFallback(t *testing.T) { // 81 runes, so the leaf is entirely past the cap. const prefix = "/Users/somebody/go/src/github.com/some-organisation/some-repository/worktrees/wt/" - if len([]rune(prefix)) <= maxTitleLen { + if len([]rune(prefix)) <= MaxTitleLen { t.Fatalf("fixture prefix is %d runes, needs to exceed %d to exercise the cut", - len([]rune(prefix)), maxTitleLen) + len([]rune(prefix)), MaxTitleLen) } titleFor := func(cwd string) string { dir := t.TempDir() @@ -1262,7 +1262,7 @@ func TestIsSyntheticPrompt_HandlesAttributesAndCase(t *testing.T) { } } -// maxTitleLen is a RUNE cap, and the renderer is what bounds display width. +// MaxTitleLen is a RUNE cap, and the renderer is what bounds display width. // // 80 runes of CJK occupy 160 columns, so nothing may read this constant as a width budget. This // pins ONLY the half that lives in this package: that the cap counts runes rather than bytes or @@ -1281,19 +1281,19 @@ func TestMaxTitleLen_IsARuneCapNotAWidthBudget(t *testing.T) { if gerr != nil { t.Fatal(gerr) } - if n := len([]rune(got)); n != maxTitleLen { - t.Errorf("clipped to %d runes, want %d", n, maxTitleLen) + if n := len([]rune(got)); n != MaxTitleLen { + t.Errorf("clipped to %d runes, want %d", n, MaxTitleLen) } // The point of the test: runes are capped, BYTES AND COLUMNS ARE NOT. authlib has no width // library — lipgloss and go-runewidth are cmd/abctl dependencies, and adding one here for a // single assertion is not worth it — so byte length stands in as the observable proxy: a // three-byte-per-rune title is 240 bytes at 80 runes, and anything laying out by width will // likewise see more than 80. What this pins is that the cap is NOT a width, which is the - // mistake the comment on maxTitleLen warns against. - if len(got) <= maxTitleLen { - t.Errorf("CJK title is %d bytes for %d runes; if that is now <= the cap then maxTitleLen "+ + // mistake the comment on MaxTitleLen warns against. + if len(got) <= MaxTitleLen { + t.Errorf("CJK title is %d bytes for %d runes; if that is now <= the cap then MaxTitleLen "+ "is being applied as a width or byte bound, and the renderers' own truncation must be "+ - "revisited", len(got), maxTitleLen) + "revisited", len(got), MaxTitleLen) } } @@ -1904,7 +1904,7 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { t.Errorf("input %q: title has a double space: %q", in, got) } // Bounded, and valid UTF-8 — never cut mid-character. - if n := len([]rune(got)); n > maxTitleLen { + if n := len([]rune(got)); n > MaxTitleLen { t.Errorf("input %q: title is %d runes: %q", in, n, got) } if !utf8.ValidString(got) { @@ -1981,8 +1981,8 @@ func TestTitleFromTranscript_CwdFallbackIsPlainAndUnclipped(t *testing.T) { // NOT clipped: a prefix past the cap must not swallow the leaf, or sibling worktrees become // indistinguishable. const prefix = "/Users/somebody/go/src/github.com/some-organisation/some-repository/worktrees/wt/" - if len([]rune(prefix)) <= maxTitleLen { - t.Fatalf("fixture prefix is %d runes, needs to exceed %d", len([]rune(prefix)), maxTitleLen) + if len([]rune(prefix)) <= MaxTitleLen { + t.Fatalf("fixture prefix is %d runes, needs to exceed %d", len([]rune(prefix)), MaxTitleLen) } a, b := titleFor(prefix+"alpha"), titleFor(prefix+"beta") if a == b { diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index 60f6b68ca..1b9022b08 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -413,7 +413,7 @@ func (m *model) sessionTitleCell(id string, titleW int) string { // 11: that was previously dismissed as unreachable on this platform, which confused where the // string comes FROM with where it is rendered. Titles are harvested text; the renderer does // not get to assume their shape. - if !strings.HasPrefix(title, "/") { + if !looksLikePath(title) { if titleW <= 0 { // FAIL SAFE, not wide. A non-positive budget means the caller could not tell us how // much room there is, and returning the full title then hands an unbounded cell to a @@ -443,6 +443,32 @@ func (m *model) sessionTitleCell(id string, titleW int) string { return truncLeft(title, titleW) } +// looksLikePath reports whether a title should be truncated from the LEFT, keeping its tail. +// +// A LEADING SLASH IS NOT ENOUGH. It was the whole test until session titles started coming from the +// user's own prompts, and a typed slash command begins with one too: "/review carefully" was +// left-truncated to "…pull/1101 carefully", discarding the command name — the one part of it a reader +// needs. That inverts this file's own rule that prose reads left-to-right. +// +// The distinction that matters is whether the string's LEAF is the identifying part. A filesystem path +// has more than one segment and no spaces in its first one; a slash command is one word followed by +// arguments. So a title qualifies only if it has a second "/" before any space — which "/review x" +// does not, and "/Users/somebody/src" does. +// +// Deliberately simple, and wrong in one direction on purpose: "/tmp foo" reads as prose and would be +// right-truncated. A single-segment path is not something Claude Code records as a cwd, and the cost +// is a cell cut at the other end rather than anything unbounded. +func looksLikePath(title string) bool { + if !strings.HasPrefix(title, "/") { + return false + } + rest := title[1:] + if i := strings.IndexAny(rest, " \t"); i >= 0 { + rest = rest[:i] + } + return strings.Contains(rest, "/") +} + // truncRight clips s to n DISPLAY COLUMNS keeping the LEFT end, marking the cut with a // trailing ellipsis. // diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index fdcef7c9d..e1ea0f511 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -11,6 +11,7 @@ import ( "github.com/charmbracelet/lipgloss" "github.com/muesli/termenv" + "github.com/rossoctl/cortex/authbridge/authlib/observe/claude" "github.com/rossoctl/cortex/authbridge/authlib/pipeline" "github.com/rossoctl/cortex/authbridge/authlib/session" ) @@ -833,3 +834,127 @@ func TestSessionTitleCell_CarriesNoANSI(t *testing.T) { } } } + +// THE CROSS-MODULE CONTRACT: the harvester's rune cap is safe only because this package +// re-truncates by display width. +// +// Each side was tested independently and neither held the relationship. authlib/observe/claude can +// assert only that MaxTitleLen counts runes — it has no width library — and this package asserts only +// that cells fit their column. So deleting the renderer's truncation broke no test, while the +// harvester's own comment warned that 80 runes of CJK occupy 160 columns. +// +// This closes it from the side that can see both: it takes a title at exactly the harvester's cap, +// in the worst case for the mismatch, and requires the rendered cell to fit a narrow column anyway. +// It fails if either the cap stops being a rune count or the renderer stops measuring in columns. +func TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures(t *testing.T) { + forceColor(t) + + // A title the harvester would emit at its limit: MaxTitleLen runes of CJK, which is twice that + // in display columns. + title := strings.Repeat("日", claude.MaxTitleLen) + if n := len([]rune(title)); n != claude.MaxTitleLen { + t.Fatalf("fixture is %d runes, want %d", n, claude.MaxTitleLen) + } + if w := lipgloss.Width(title); w <= claude.MaxTitleLen { + t.Fatalf("fixture is %d columns for %d runes — it no longer exercises the mismatch, so "+ + "either MaxTitleLen has become a width budget or this fixture needs wider characters", + w, claude.MaxTitleLen) + } + + const id = "s1" + m := newTitleModel(t, map[string]SessionMetadata{id: {Title: title}}, id) + for _, w := range []int{11, 14, 20, 40} { + got := m.sessionTitleCell(id, w) + if cw := lipgloss.Width(got); cw > w { + t.Errorf("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", + claude.MaxTitleLen, cw, w, got) + } + } + + // And the same through the rendered row, so the guard covers what a reader actually sees rather + // than only the cell helper. + // + // The budget has to come from the model's OWN width. An earlier version of this test installed a + // 100-column header while the fixture model was 200 wide, then asserted against the 100-column + // budget — rebuildSessionsTable reads the width it is about to install, so it correctly produced + // a 107-column cell and the test called that a bug. The failure was in the fixture. + for _, termW := range []int{80, 100, 200} { + m.width = termW + m.sessionsTbl.SetColumns(sessionsColumnsFor(termW)) + m.rebuildSessionsTable() + titleW := sessionsColumnWidth(sessionsColumnsFor(termW), "TITLE") + cell := sessionsCell(t, m, titleRow(t, m, id), "TITLE") + if lipgloss.Width(cell) > titleW { + t.Errorf("at terminal width %d: rendered TITLE cell is %d columns against a %d-column "+ + "column: %q", termW, lipgloss.Width(cell), titleW, cell) + } + } +} + +// A slash command keeps its COMMAND NAME; a filesystem path keeps its leaf. +// +// The discriminator was a bare leading "/", which was the whole story until session titles started +// coming from the user's own prompts. A typed slash command begins with one too, so +// "/review carefully" was left-truncated to "…pull/1101 carefully" — discarding the command +// name, the one part a reader needs, and inverting this file's own rule that prose reads +// left-to-right. +func TestSessionTitleCell_SlashCommandIsNotAPath(t *testing.T) { + forceColor(t) + const id = "s1" + for _, tc := range []struct { + name, title string + keepHead bool + }{ + {"slash command with args", "/review https://github.com/rossoctl/cortex/pull/1101 carefully", true}, + {"slash command with a path arg", "/fix-ocr some/path.md and then report", true}, + {"slash command alone", "/clear", true}, + // A real cwd: more than one segment, no space before the second "/", so the leaf is what + // identifies it and left-truncation is right. + {"absolute path", "/Users/somebody/src/cortex/.worktrees/alpha/authbridge", false}, + {"short absolute path", "/tmp/build/output/artifacts/final", false}, + } { + 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) + if lipgloss.Width(got) > w { + t.Fatalf("cell is %d columns against a %d-column budget: %q", lipgloss.Width(got), w, got) + } + if tc.keepHead { + if !strings.HasPrefix(got, tc.title[:5]) { + t.Errorf("command name lost: %q from %q", got, tc.title) + } + if strings.HasPrefix(got, "…") { + t.Errorf("a slash command was truncated from the LEFT: %q", got) + } + return + } + if !strings.HasSuffix(got, tc.title[len(tc.title)-5:]) { + t.Errorf("path leaf lost: %q from %q", got, tc.title) + } + }) + } +} + +// looksLikePath itself, so the rule is pinned independently of how a cell renders. +func TestLooksLikePath(t *testing.T) { + for _, tc := range []struct { + in string + want bool + }{ + {"/Users/somebody/src/cortex", true}, + {"/tmp/build/out", true}, + {"/a/b", true}, + {"/review some/path.md", false}, // a space before the second slash + {"/clear", false}, // one segment + {"/fix-ocr", false}, + {"how do I build abctl?", false}, + {"src/cortex/authbridge", false}, // no leading slash + {"", false}, + } { + if got := looksLikePath(tc.in); got != tc.want { + t.Errorf("looksLikePath(%q) = %v, want %v", tc.in, got, tc.want) + } + } +} From af9389d9320baaf875df25a02567017b3deadeb8 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 08:43:02 -0400 Subject: [PATCH 09/16] fix(abctl): Strip escapes before tags, and stop counting brackets as a balanced pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two must-fixes, both about one pass's output being another pass's input. An ESC sequence inside a tag NAME hid that tag from the only pass that removes a block together with its body: "INJECTED" was invisible to stripHarnessSpans, and stripTags then unwrapped it to bare "INJECTED". stripANSI now runs first, which makes the name whole again before anything looks for it. An unclosed harness block advanced only past the opening tag's ">", so its body survived as prose with no escape sequence needed: "prose INJECTED payload" kept the payload. An unclosed block now runs to end-of-string. That is a deliberate trade, confirmed before making it: a prompt that MENTIONS a self-closing harness tag loses its tail, so "how do I use in a skill?" titles as "how do I use". Four tests asserted the old behaviour and now assert this one. The head, which says what the session is about, survives either way. stripTags counted "<" and ">" as a balanced pair, which broke twice: two comparison operators cancelled out and everything between them was eaten ("is 3 < 5 and 6 > 2 in Go?" became "is 3 2 in Go?"), and one unbalanced "<" disabled stripping for the WHOLE string including balanced tags before it — so the no-markup contract was simply false for those inputs. It now recognises a tag-name-shaped span per occurrence, so "< 5" is text and "
" is not. stripANSI handled only CSI and OSC: DCS, SOS, PM, APC and charset selection lost the ESC and leaked their payload as literal text. All are handled, and a trailing lone ESC is dropped rather than written through, which its own doc already claimed. cutTrailingHarness's line-leading check tested space and tab while its adjacent comment cited \v and NBSP. Any whitespace counts now — not a leak end to end, since normalisation removes the tag either way, but a defence that only appears to cover a case is worse than one that admits it does not. Two honest notes. The normalisation now iterates to a fixed point, and that is REDUNDANT with the corrected ordering — pinning it to one iteration fails no test. It is kept because the argument for one pass is an argument about orderings, and "the orderings someone thought of" is what six rounds of review kept flanking. And the property test cannot distinguish an escape class whose payload leaks from one whose does not, since both leave a plain title; a direct stripANSI test pins that. Also: a flags-only prompt normalises to nothing and correctly falls to the next tier; a test fixture byte-sliced a title in a suite that exercises CJK elsewhere, now rune-sliced; and the PR description's local-tree counts are removed, since they are not verifiable outside one machine. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 161 +++++++++++++++--- .../authlib/observe/claude/harvest_test.go | 149 +++++++++++++--- .../cmd/abctl/tui/sessions_title_test.go | 10 +- 3 files changed, 267 insertions(+), 53 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index d07a0291c..0dbea3708 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -641,6 +641,13 @@ func titleFromTranscript(path string) (string, error) { // removes the renderer's truncation and leaves this constant alone. const MaxTitleLen = 80 +// maxNormalizePasses bounds normalizeTitle's fixed-point loop. +// +// Each pass only ever deletes, so the string strictly shrinks and the loop converges long before +// this — two passes is the most any observed input needs. The cap exists so a future pass that +// somehow grows the string cannot spin, not because convergence is in doubt. +const maxNormalizePasses = 8 + // clipTitle trims s and caps it at MaxTitleLen runes. // // No ellipsis: this is not the display truncation — the TITLE column applies its own, measured in @@ -663,9 +670,32 @@ func normalizeTitle(s string) string { // So the rule here is positional-independent and applies to every tier: a title carries no tags // at all. Callers still unwrap envelopes and drop harness-output wrappers, because those // decisions are about WHICH TEXT to use; this is about what may appear in the result. - s = stripHarnessSpans(s) - s = stripANSI(s) - s = stripTags(s) + // ORDER, AND THEN A FIXED POINT. + // + // stripANSI runs FIRST because an escape sequence inside a tag name hides that tag from the only + // pass that removes a block together with its body: "INJECTED" + // was invisible to stripHarnessSpans, and stripTags then unwrapped it to bare "INJECTED". + // Removing the escapes first makes the tag whole again. + // + // And the three are applied until nothing changes, rather than once each. Every one of them + // REWRITES the string, so any of them can expose work for another: removing an escape can join a + // tag name, and removing a harness span can bring two halves of prose together. + // + // REDUNDANT TODAY, and recorded as such rather than left to look load-bearing: with the order + // above, one pass handles every input tried, including escapes nested inside tags inside escapes + // — pinning the loop to a single iteration fails no test. It is kept because the argument for + // one pass being enough is an argument about orderings, and "the orderings someone thought of" + // is exactly what six rounds of review kept flanking. Bounded because each pass only ever + // deletes, so the length strictly decreases until it stabilises. + for i := 0; i < maxNormalizePasses; i++ { + before := s + s = stripANSI(s) + s = stripHarnessSpans(s) + s = stripTags(s) + if s == before { + break + } + } // ONE LINE, and only characters that print as themselves. // @@ -705,6 +735,9 @@ func normalizeTitle(s string) string { // ("café" titles as "cafe") and keeps this function simple, which is the trade the // rest of it already makes. case r >= 0x1F1E6 && r <= 0x1F1FF: + // Dropped, which can empty a title entirely — a prompt of nothing but flags. The tier + // switch treats an empty result as "this tier has nothing", so the session falls through + // to the next one rather than rendering blank; see the normalisation before that switch. // REGIONAL INDICATORS, which only carry meaning in pairs: a cut between them turns a // flag into a lone letter glyph. Two runes, never independently meaningful, so they go // together or not at all. @@ -751,18 +784,34 @@ func stripANSI(s string) string { b.Grow(len(s)) rs := []rune(s) for i := 0; i < len(rs); i++ { - if rs[i] != 0x1b || i+1 >= len(rs) { + if rs[i] != 0x1b { b.WriteRune(rs[i]) continue } + // A LONE TRAILING ESC IS DROPPED, not written through. The old code fell to the default + // branch and copied it, contradicting this function's own doc — harmless inside + // normalizeTitle, where the allowlist catches it, but stripANSI has direct callers in the + // renderer's width assertions. + if i+1 >= len(rs) { + break + } switch rs[i+1] { - case '[': // CSI: parameters, then a final byte in the range @ to ~ + case '[': // CSI: parameters, then a final byte in @ to ~ j := i + 2 for j < len(rs) && (rs[j] < '@' || rs[j] > '~') { j++ } - i = j // the final byte is consumed too - case ']': // OSC: runs to BEL, or to ST (ESC \) + i = j + case ']', 'P', 'X', '^', '_': + // STRING-ARGUMENT SEQUENCES, all terminated by BEL or ST: OSC (]), DCS (P), SOS (X), + // PM (^) and APC (_). Only OSC was handled, so a DCS payload — "\x1bPq …\x1b\\" — + // lost its ESC and leaked the rest as literal text, which is a garbage title rather + // than a dangerous one. They share a terminator, so they share a branch. + // + // The property test cannot tell these apart from the two-byte fallback, since both leave + // a plain title — only the CONTENT differs, and "garbage but plain" satisfies the + // guarantee. TestStripANSI_HandlesEveryEscapeClass is what pins the payload actually + // going, rather than the ESC alone. j := i + 2 for j < len(rs) && rs[j] != 0x07 { if rs[j] == 0x1b && j+1 < len(rs) && rs[j+1] == '\\' { @@ -772,9 +821,13 @@ func stripANSI(s string) string { j++ } i = j + case '(', ')', '*', '+', '-', '.', '/', '%', '#', ' ': + // CHARSET SELECTION and other two-byte intermediates: ESC ( B, ESC # 8 and friends take + // exactly one more byte. Skipping only the ESC left "(B" in the title. + i += 2 default: - // A two-character escape, or something unrecognised. Skip the ESC and let the loop - // handle the next rune normally. + // A two-character escape (ESC 7, ESC c, ESC =). The ESC and its single final byte go. + i++ } } return b.String() @@ -802,16 +855,20 @@ func stripHarnessSpans(s string) string { if i < 0 { break } - // The span runs to the matching close when there is one, and to the end of the opening - // tag otherwise — a self-closing or unterminated block leaves nothing to keep. + // The span runs to the matching close when there is one, and TO THE END OF THE STRING + // otherwise. + // + // Not to the end of the opening tag, which is what it did: an unclosed block then left + // its body behind as bare prose — "prose INJECTED payload" kept + // "INJECTED payload" — with no escape sequence needed. A harness block that is not + // closed is still a harness block, and everything after it is its content as far as + // anyone can tell. end := len(s) name := tag[1:] // tag is "= 0 { if g := strings.IndexByte(s[i+c:], '>'); g >= 0 { end = i + c + g + 1 } - } else if g := strings.IndexByte(s[i:], '>'); g >= 0 { - end = i + g + 1 } s = s[:i] + " " + s[end:] } @@ -835,25 +892,68 @@ func stripTags(s string) string { } var b strings.Builder b.Grow(len(s)) - depth := 0 - for _, r := range s { - switch { - case r == '<': - depth++ - case r == '>' && depth > 0: - depth-- - case depth == 0: - b.WriteRune(r) + for { + i := strings.IndexByte(s, '<') + if i < 0 { + b.WriteString(s) + break } - } - // A lone "<" with no closing ">" left depth > 0 and swallowed the tail, which would turn - // "is 3 < 5" into "is 3 ". Fall back to the original in that case: nothing was a tag. - if depth > 0 { - return s + // A TAG-NAME-SHAPED SPAN, not just anything between angle brackets. Two defects came from + // treating every "<" as an opener and counting depth: + // + // - two balanced comparison operators cancelled out, so the depth-0 fallback never fired + // and everything between them was eaten: "is 3 < 5 and 6 > 2 in Go?" became + // "is 3 2 in Go?"; + // - one unbalanced "<" disabled stripping for the WHOLE string, including balanced tags + // before it, so normalizeTitle's no-markup contract was simply false for such inputs. + // + // Deciding per span fixes both: a "<" that does not begin a plausible tag is ordinary text + // and is kept, and each span is judged on its own. + if n := tagSpanLen(s[i:]); n > 0 { + b.WriteString(s[:i]) + s = s[i+n:] + continue + } + b.WriteString(s[:i+1]) + s = s[i+1:] } return b.String() } +// tagSpanLen returns the length of the tag starting at s[0], or 0 if s does not start with one. +// +// A tag is "<", an optional "/", a name of letters, digits, "-" or "_", then anything up to the +// first ">". That is deliberately narrow: "< 5" and "<" at end-of-string are not tags, so a +// comparison operator in a real prompt survives, while "
", "" and +// "" do not. +func tagSpanLen(s string) int { + if len(s) < 2 || s[0] != '<' { + return 0 + } + i := 1 + if s[i] == '/' { + i++ + } + nameStart := i + for i < len(s) { + c := s[i] + if c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' { + i++ + continue + } + break + } + if i == nameStart { + return 0 // no name: "< 5", "<>", "<=" + } + if j := strings.IndexByte(s[i:], '>'); j >= 0 { + return i + j + 1 + } + // An opening tag with no ">" anywhere. Not a span, so the "<" is kept as text — the alternative + // is swallowing the rest of the string on a stray bracket. + return 0 +} + // promptFromMessage returns the text a person typed in one user turn, and whether the content was // a plain STRING rather than an array of blocks. // @@ -1143,8 +1243,13 @@ func cutTrailingHarness(s string) string { // Every occurrence is examined, not just the first: a prompt may mention a tag inline // and still have a real appended block after it. if i > 0 { + // ANY whitespace counts as indentation, not just space and tab. The adjacent comment + // already cited \v and NBSP as shapes the harness might use, while the check tested + // two characters — so the two defences were less independent than they read. Not a + // leak end to end, since normalizeTitle removes the tag either way, but a defence + // that only appears to cover a case is worse than one that admits it does not. j := i - 1 - for j >= 0 && (s[j] == ' ' || s[j] == '\t') { + for j >= 0 && unicode.IsSpace(rune(s[j])) && s[j] != '\n' && s[j] != '\r' { j-- } if j >= 0 && s[j] != '\n' && s[j] != '\r' { diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 216f03eb2..d063f1a6c 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1709,22 +1709,25 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { name, content, want string }{ { - // These used to assert the tag SURVIVES, on the reasoning that cutting mid-sentence was - // worse than leaking a tag. That trade is gone: the tag is stripped and the surrounding - // prose is kept, so neither the truncation nor the leak happens. - "known tag mid-sentence is stripped, not cut", + // AN UNCLOSED HARNESS TAG RUNS TO END-OF-STRING, so a prompt mentioning one loses its + // tail. That is the deliberate trade: an unclosed block whose body cannot be delimited + // otherwise promotes its content into the title — "prose INJECTED + // payload" kept "INJECTED payload" — and a title being plain matters more than a prompt + // that quotes a harness tag reading in full. The head, which says what the session is + // about, survives either way. + "an unclosed known tag takes the rest of the line", "how do I use in a skill?", - "how do I use in a skill?", + "how do I use", }, { - "known tag mid-sentence, different tag", + "an unclosed known tag, different tag", "why does show up in my logs?", - "why does show up in my logs?", + "why does", }, { - "known tag after a space", + "an unclosed known tag after a space", "see for details", - "see for details", + "see", }, { // The real shape: the harness appends on its own line, which IS a cut. @@ -1734,9 +1737,10 @@ func TestTitleFromTranscript_InlineHarnessTagMentionIsNotACut(t *testing.T) { }, { // Every occurrence is examined, so an inline mention does not mask a later real block. - "an inline mention does not hide a later appended block", + // The first unclosed tag already takes the rest, appended block included. + "an unclosed mention takes the appended block with it", "how do I use ?\nhidden", - "how do I use ?", + "how do I use", }, { // THE LAST DOCUMENTED LEAK, now closed. This was left alone deliberately, as a trade @@ -1815,14 +1819,14 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { inputs := []string{ // Harness markup in every placement review found, plus the ones it did not. `injected instructions`, - "my question\ninjected", - "my question\n injected", - "my question\n\tinjected", - "my question\n\vinjected", + "my question\nHARNESSBODY", + "my question\n HARNESSBODY", + "my question\n\tHARNESSBODY", + "my question\n\vHARNESSBODY", "my question\n injected", "my question\n　injected", - "line one\nline two injected", - "review\n/review\ninjected", + "line one\nline two HARNESSBODY", + "review\n/review\nHARNESSBODY", `injected`, `total 40`, `nested`, @@ -1833,6 +1837,20 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { "how do I write List in Go?", "a bold claim", "body", + // THIS ROUND. Each was a leak or a mangling; the corpus grows rather than each getting its + // own test, because the property is what matters and the placements are open-ended. + "ok HARNESSBODY end", // ESC splitting a tag name + "HARNESSBODY", + "prose HARNESSBODY payload here", // unclosed block + "is 3 < 5 and 6 > 2 in Go?", // two balanced operators + "HARNESSBODY and 3 < 5", + "a\x1bPq injected \x1b\\b", // DCS + "a\x1b(Binjected", // charset selection + "a\x1b_injected\x07b", // APC + "title\x1b", // trailing lone ESC + "my question\n\vHARNESSBODY", + "my question\n\fHARNESSBODY", + "my question\n\u00a0HARNESSBODY", // Control characters and invisible code points. "colour \x1b[31mred\x1b[0m here", "osc \x1b]0;evil\x07 here", @@ -1868,10 +1886,12 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { t.Fatalf("input %q: %v", in, gerr) } - // No angle brackets that could read as a tag. A lone "<" survives — "is 3 < 5" is a real - // prompt — but never a matched pair. - if strings.Contains(got, ">") { - t.Errorf("input %q: title carries a closing bracket: %q", in, got) + // NO TAG-SHAPED SPAN. Bare brackets are legitimate — "is 3 < 5 and 6 > 2 in Go?" is a real + // prompt and must survive whole — so the assertion is about a "" span, not about the + // characters. An earlier version banned ">" outright and failed that prompt, which would have + // pushed the code toward eating comparison operators again. + if tagSpanLen(got[strings.IndexByte(got+"<", '<'):]) > 0 { + t.Errorf("input %q: title carries a tag span: %q", in, got) } // No harness tag name, in any form. for _, tag := range harnessTagNames { @@ -1880,11 +1900,15 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { } } // AND NOT THE BODY EITHER. Stripping the brackets alone promoted the payload into the - // title — "my question\ninjected" became + // title — "my question\nHARNESSBODY" became // "my question injected" — which is the whole point of removing a harness span rather than // just its tags. Checking only for tags left this test blind to it, and the mutation that // exposed the gap passed until this assertion existed. - if strings.Contains(got, "injected") || strings.Contains(got, "total 40") { + // A harness BODY must not survive — stripping brackets alone promoted the payload. Keyed on + // a marker that appears ONLY inside harness spans in this corpus: "injected" was used both + // for those and for plain text after an escape sequence, so the check fired on text that was + // never inside a tag. + if strings.Contains(got, "HARNESSBODY") || strings.Contains(got, "total 40") { t.Errorf("input %q: title carries a harness BODY: %q", in, got) } // Every rune prints as itself: no controls, format characters, surrogates or unassigned. @@ -2050,3 +2074,82 @@ func TestReadSessions_CollectsPartialReads(t *testing.T) { t.Errorf("healthy session title = %q, want %q", out["fine"].Title, "another ask") } } + +// stripANSI removes each escape class WITH its payload, not just the ESC byte. +// +// The property test cannot see this: dropping the ESC alone still leaves a plain title, so +// "garbage but plain" satisfies the guarantee. Only CSI and OSC were handled, so a DCS or APC +// payload — and the one byte after a charset-selection escape — survived as literal text. +func TestStripANSI_HandlesEveryEscapeClass(t *testing.T) { + for _, tc := range []struct { + name, in, want string + }{ + {"CSI colour", "a\x1b[31mb", "ab"}, + {"CSI cursor", "a\x1b[2Jb", "ab"}, + {"OSC with BEL", "a\x1b]0;evil\x07b", "ab"}, + {"OSC with ST", "a\x1b]0;evil\x1b\\b", "ab"}, + {"DCS", "a\x1bPq payload \x1b\\b", "ab"}, + {"SOS", "a\x1bXpayload\x07b", "ab"}, + {"PM", "a\x1b^payload\x07b", "ab"}, + {"APC", "a\x1b_payload\x07b", "ab"}, + {"charset selection", "a\x1b(Bb", "ab"}, + {"charset selection, other", "a\x1b)0b", "ab"}, + {"two-byte escape", "a\x1b7b", "ab"}, + {"trailing lone ESC", "title\x1b", "title"}, + {"no escapes", "how do I build abctl?", "how do I build abctl?"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := stripANSI(tc.in); got != tc.want { + t.Errorf("stripANSI(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// tagSpanLen recognises a tag and declines everything else, which is what keeps comparison +// operators in real prompts. +// +// Two defects came from counting "<" and ">" as a balanced pair: two operators cancelled out and +// everything between them was eaten ("is 3 < 5 and 6 > 2 in Go?" became "is 3 2 in Go?"), and one +// unbalanced "<" disabled stripping for the whole string including balanced tags before it. +func TestTagSpanLen(t *testing.T) { + for _, tc := range []struct { + in string + want int + }{ + {"
", 5}, + {"
", 6}, + {"", 17}, + {``, 12}, + {"text", 3}, // the opening tag only + {"< 5", 0}, // a comparison, not a tag + {"<=", 0}, + {"<>", 0}, + {"<", 0}, + {" 2 in Go?", "is 3 < 5 and 6 > 2 in Go?"}, + {"a > b > c", "a > b > c"}, + {"x <= y and y >= z", "x <= y and y >= z"}, + // A balanced span BEFORE a lone "<" is still stripped: one unbalanced bracket used to + // disable stripping for the entire string. + {"HARNESSBODY and 3 < 5", "and 3 < 5"}, + {"
x
and 3 < 5", "x and 3 < 5"}, + } { + if got := clipTitle(tc.in); got != tc.want { + t.Errorf("clipTitle(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index e1ea0f511..9b18bdf73 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -921,8 +921,14 @@ func TestSessionTitleCell_SlashCommandIsNotAPath(t *testing.T) { if lipgloss.Width(got) > w { t.Fatalf("cell is %d columns against a %d-column budget: %q", lipgloss.Width(got), w, got) } + // RUNE slices, not byte slices. Every fixture here is ASCII today, but this suite + // deliberately exercises CJK elsewhere, and a byte slice would split a multi-byte + // character the moment someone adds such a case — producing an invalid-UTF-8 needle and + // a failure that looks like the code's fault. + head := string([]rune(tc.title)[:5]) + tail := func(s string) string { r := []rune(s); return string(r[len(r)-5:]) } if tc.keepHead { - if !strings.HasPrefix(got, tc.title[:5]) { + if !strings.HasPrefix(got, head) { t.Errorf("command name lost: %q from %q", got, tc.title) } if strings.HasPrefix(got, "…") { @@ -930,7 +936,7 @@ func TestSessionTitleCell_SlashCommandIsNotAPath(t *testing.T) { } return } - if !strings.HasSuffix(got, tc.title[len(tc.title)-5:]) { + if !strings.HasSuffix(got, tail(tc.title)) { t.Errorf("path leaf lost: %q from %q", got, tc.title) } }) From 6b75612348c247fc99fb42b963b31d56e2fee9f8 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 09:31:45 -0400 Subject: [PATCH 10/16] fix(abctl): Match the outer close when a harness tag nests inside itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same-name nesting leaked its body. The close-tag search took the FIRST "abLEAKED" -> "LEAKED" The fixed-point loop could not recover it, because the opening tag had already been consumed and the next pass found nothing to match. Now counts depth and takes the matching close. Different-name nesting always worked, since each tag is scanned separately — and that is exactly why the corpus missed this. It covered the case that worked, which is the third time in this PR a fixture excluded the path it was meant to cover. The MaxTitleLen comment claimed the cross-module truncation relationship "is not guarded by any single test" and that "nothing fails if someone removes the renderer's truncation". Removing it fails 11 tests, one named for that exact invariant. The comment was true when written and false once the guard landed two commits later — the worse direction for a comment to be wrong in, since it invites deleting guarded code. It now names the guard. cutTrailingHarness converted one BYTE to a rune, so no multi-byte space ever matched: the continuation bytes are not IsSpace, the scan stopped on them, and the block read as mid-line. The adjacent comment named NBSP as covered while the code could not see it. Decoded as runes now, and the test covers NBSP, U+3000, U+2028 and ogham. Worth recording about the verification rather than just the fix: the first mutation of the whitespace scan left "unicode/utf8" unused, so the package failed to BUILD and the failure count came back zero — which reads exactly like a passing test. A mutation has to keep compiling to mean anything. Re-run with the import still referenced, it fails on all four multi-byte spaces. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 80 ++++++++++++++----- .../authlib/observe/claude/harvest_test.go | 51 ++++++++++++ 2 files changed, 111 insertions(+), 20 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 0dbea3708..83a4143cd 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -12,6 +12,7 @@ import ( "strings" "time" "unicode" + "unicode/utf8" ) // ConfigDirEnv is the variable Claude Code itself honours for relocating its config @@ -634,11 +635,18 @@ func titleFromTranscript(path string) (string, error) { // 11-column cell would collapse to a lone ellipsis. Anyone adding colour to a title needs to know that // before they do it, which is why it is recorded here rather than left to be rediscovered. // -// THAT RELATIONSHIP IS NOT GUARDED BY ANY SINGLE TEST, and an earlier version of this comment -// implied otherwise. It spans two modules: this package has no width library, so its test asserts -// only that the cap is a rune count and not a byte or width bound, while the re-truncation lives in -// cmd/abctl/tui and is tested there against its own column budgets. Nothing fails if someone -// removes the renderer's truncation and leaves this constant alone. +// THAT RELATIONSHIP IS GUARDED, from the renderer's side: +// TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures in cmd/abctl/tui takes a title at exactly this +// cap in the worst case for the mismatch — MaxTitleLen runes of CJK, twice that in columns — and +// requires the rendered cell to fit anyway. Removing the renderer's truncation fails it, along with +// ten other tests in that package. +// +// An earlier version of this comment said the opposite: that no single test held the relationship +// and that "nothing fails if someone removes the renderer's truncation". That was true when written +// and false once the guard landed, which is the worse direction for a comment to be wrong in — it +// invites a maintainer to delete guarded code. This package still cannot assert the width half +// itself (it has no width library, and the exported constant is what lets the other side name it), +// so the guard lives there and this comment points at it by name. const MaxTitleLen = 80 // maxNormalizePasses bounds normalizeTitle's fixed-point loop. @@ -863,11 +871,34 @@ func stripHarnessSpans(s string) string { // "INJECTED payload" — with no escape sequence needed. A harness block that is not // closed is still a harness block, and everything after it is its content as far as // anyone can tell. + // THE MATCHING CLOSE, counting depth — not the first one. With the same tag NESTED, + // "abLEAKED" ended + // its span at the INNER close, so the outer close and everything before it survived as + // "LEAKED". The next iteration then found no opening tag, because this one had consumed + // it, so the fixed-point loop could not recover it either. + // + // Different-name nesting was already handled, since each tag is scanned separately, and + // that is why the corpus missed this: it covered the case that worked. end := len(s) name := tag[1:] // tag is "= 0 { - if g := strings.IndexByte(s[i+c:], '>'); g >= 0 { - end = i + c + g + 1 + depth := 0 + for j := i; j < len(s); { + switch { + case strings.HasPrefix(s[j:], tag): + depth++ + j += len(tag) + case strings.HasPrefix(s[j:], "'); g >= 0 { + end = j + g + 1 + } + j = len(s) // done + continue + } + j += len("…" kept the // tag and was clipped mid-tag at 80 runes, which is the failure this function's own doc - // describes. Only spaces and tabs are skipped, so the constraint still holds against - // anything with real text before it on the line. + // describes. Only whitespace is skipped, so the constraint still holds against anything + // with real text before it on the line. // // A known tag mid-line on a LATER line — "line one\nline two x" — is // deliberately left alone: it is genuinely ambiguous between prose and appended markup, @@ -1243,17 +1274,26 @@ func cutTrailingHarness(s string) string { // Every occurrence is examined, not just the first: a prompt may mention a tag inline // and still have a real appended block after it. if i > 0 { - // ANY whitespace counts as indentation, not just space and tab. The adjacent comment - // already cited \v and NBSP as shapes the harness might use, while the check tested - // two characters — so the two defences were less independent than they read. Not a - // leak end to end, since normalizeTitle removes the tag either way, but a defence - // that only appears to cover a case is worse than one that admits it does not. - j := i - 1 - for j >= 0 && unicode.IsSpace(rune(s[j])) && s[j] != '\n' && s[j] != '\r' { - j-- + // ANY whitespace counts as indentation, not just space and tab — and it is decoded as + // a RUNE, which is the part the previous version got wrong. `unicode.IsSpace(rune(s[j]))` + // converts one BYTE, so a multi-byte space (NBSP, U+3000, U+2028, ogham) never matched: + // its continuation bytes are not IsSpace, so the scan stopped on them and the block was + // treated as mid-line. The comment claimed those very shapes were covered. + // + // Not a leak end to end — normalizeTitle removes the tag either way — but a defence that + // only appears to cover a case is worse than one that admits it does not. + j := i + for j > 0 { + r, size := utf8.DecodeLastRuneInString(s[:j]) + if r == '\n' || r == '\r' || !unicode.IsSpace(r) { + break + } + j -= size } - if j >= 0 && s[j] != '\n' && s[j] != '\r' { - continue + if j > 0 { + if r, _ := utf8.DecodeLastRuneInString(s[:j]); r != '\n' && r != '\r' { + continue + } } } if cut < 0 || i < cut { diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index d063f1a6c..412b19f8b 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -2153,3 +2153,54 @@ func TestClipTitle_KeepsComparisonOperators(t *testing.T) { } } } + +// A harness block nested inside the SAME tag name is removed whole. +// +// The close-tag search took the first "abLEAK", ""}, + {"trailing prose survives", "abLEAK tail", "tail"}, + {"leading prose survives", "head abLEAK", "head"}, + {"three levels", "abcdLEAK", ""}, + // Two SEPARATE blocks are not nesting: the text between them is the user's and must survive. + {"separate blocks keep the text between", "a keep me b", "keep me"}, + {"different-name nesting still works", "xyz", ""}, + // An unclosed block still runs to end-of-string. + {"unclosed runs to end", "prose LEAK payload", "prose"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := clipTitle(tc.in); got != tc.want { + t.Errorf("clipTitle(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// cutTrailingHarness treats every whitespace class as indentation, decoded as runes. +// +// `unicode.IsSpace(rune(s[j]))` converts one BYTE, so a multi-byte space never matched — its +// continuation bytes are not IsSpace, the scan stopped on them, and the block read as mid-line. The +// adjacent comment named NBSP as a covered shape while the code could not see it. Not a leak end to +// end, since normalisation removes the tag anyway, but the defence did not do what it claimed. +func TestCutTrailingHarness_AnyWhitespaceIndent(t *testing.T) { + for _, ind := range []string{" ", " ", "\t", "\v", "\f", " ", "　", "
", " ", " \t "} { + in := "my real question\n" + ind + "hidden" + if got := cutTrailingHarness(in); got != "my real question" { + t.Errorf("indent %q: cutTrailingHarness = %q, want %q", ind, got, "my real question") + } + } + // Real text before the tag on its line is still not a cut — the positional rule holds. + for _, in := range []string{ + "line one\nline two x", + "see for details", + } { + if got := cutTrailingHarness(in); got != in { + t.Errorf("cutTrailingHarness(%q) = %q, want it unchanged", in, got) + } + } +} From 3c0e441ceccd1a8bdca7a7615bd898da8cbc3c0c Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 10:28:02 -0400 Subject: [PATCH 11/16] fix(abctl): Make span stripping linear, unwrap every prompt tier, re-harvest on the picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX: stripHarnessSpans was O(N²). It re-sliced the string on each excision and restarted strings.Index from offset 0, so cost grew with the square of the block count. Measured end to end through titleFromTranscript on a tree of small blocks: 155KB 21ms -> 9ms 620KB 242ms -> 24ms 2.5MB 2.49s -> 54ms Four times the input took eleven times the work before, and bufio admits lines up to 16MB, so one chatty transcript line could stall the harvest for seconds. Now builds the output once behind a cursor, like the linear sibling cutTrailingHarness. The depth scan walks each span once and the cursor never revisits it, so the pass is linear per tag. Behaviour is unchanged — every existing test passes untouched — and the new test asserts the RATIO rather than a wall-clock bound, so it says what it means on a loaded CI machine. MUST-FIX: the str and blocks tiers assigned raw text, skipping promptCandidate, so identical content was titled one way when attributed and lost entirely when not. An unattributed slash command kept its "…" envelope, failed the synthetic check, and fell through to the cwd. Unattributed turns are the majority — 9940 against 640 — so the tier that skipped the unwrapping was the common one. Both unwrap tests used only human-attributed fixtures, which is why CI was green on it; the new test is parameterised over attribution so a future tier cannot be added without covering both. Fixing that exposed a second inconsistency in the same shape: promptFromMessage's per-block filter dropped a "" block before promptCandidate could unwrap it, so an array turn lost a body that a string turn of the same content kept. A content-bearing wrapper is now unwrapped before the per-block test, while a harness-output wrapper still goes — verified that and blocks are still dropped. On the local tree this takes the count to every session named, with no directory fallbacks. FEATURE: the session picker re-harvests every three minutes. It is the one pane where someone may sit for minutes with nothing refreshing the titles — the 2s tick deliberately skips the session fetch there — so a session started in another terminal stayed nameless until the viewer was restarted. Keyed off the existing ticker rather than a second timer, with an in-flight guard so a slow harvest cannot stack. Two notes on verifying this round. The linearity mutation had to keep compiling to mean anything: an earlier attempt left an import unused, and a build failure emits no assertion output, which in a grep counts as zero failures and reads exactly like a pass. And the stacking guard's assertion was initially vacuous — the second tick was not due regardless of the flag — so it passed with the guard removed until the test backdated the stamp. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 66 ++++++++-- .../authlib/observe/claude/harvest_test.go | 117 ++++++++++++++++++ authbridge/cmd/abctl/tui/app.go | 47 ++++++- .../cmd/abctl/tui/sessions_title_test.go | 93 ++++++++++++++ 4 files changed, 305 insertions(+), 18 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 83a4143cd..0be4375c9 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -548,13 +548,27 @@ func titleFromTranscript(path string) (string, error) { case kind != "" && kind != "human": // Explicitly NOT human — "task-notification" or "peer". Discarded outright; // this is the traffic the text heuristics existed to catch. - case isSyntheticPrompt(text): - // Unattributed and looks machine-written. Still filtered, because transcripts - // with no origin field at all have nothing better to go on. - case wasString: - str = text default: - blocks = text + // EVERY TIER GOES THROUGH promptCandidate, not just the attributed one. These two + // used to take the raw text, so the same content was titled one way with + // origin.kind=human and lost entirely without it: an unattributed slash command + // stayed as its "…" envelope, failed the synthetic check, and fell + // through to the cwd. Unattributed turns are the MAJORITY — 9940 against 640 on the + // measured tree — so the tier that skipped the unwrapping was the common one. + // + // The synthetic check moves INSIDE promptCandidate rather than standing as its own + // case above, which is what makes this possible: it runs after the envelope and + // wrapper are removed, so a turn that only looks machine-written because of its + // wrapper is no longer discarded for it. + p, ok := promptCandidate(text) + if !ok { + break + } + if wasString { + str = p + } else { + blocks = p + } } } } @@ -858,11 +872,27 @@ func stripANSI(s string) string { // found in the rounds before. func stripHarnessSpans(s string) string { for _, tag := range harnessTagNames { + if !strings.Contains(s, tag) { + continue + } + closeTag := "abLEAKED" ended // its span at the INNER close, so the outer close and everything before it survived as @@ -879,30 +910,33 @@ func stripHarnessSpans(s string) string { // // Different-name nesting was already handled, since each tag is scanned separately, and // that is why the corpus missed this: it covered the case that worked. + // + // The depth scan walks each byte of the span at most once, and the cursor never revisits + // it, so the whole pass is linear in len(s) per tag. end := len(s) - name := tag[1:] // tag is "'); g >= 0 { end = j + g + 1 } - j = len(s) // done + j = len(s) continue } - j += len("…" as two text blocks of one turn — and // testing only the joined string let the markup through, because the guard is anchored at // the start and the join begins with the prose. - if isSyntheticPrompt(blk.Text) { + // A CONTENT-BEARING WRAPPER IS UNWRAPPED FIRST, so the test below judges what the user + // actually wrote. A "" block is synthetic-LOOKING but its body is the + // user's, and dropping it here lost that body for an array turn while a string turn of the + // identical content kept it — attribution and encoding deciding the outcome rather than the + // content. A harness-output wrapper is not unwrapped, so "…" still goes. + blockText := stripWrapperTag(blk.Text) + if isSyntheticPrompt(blockText) { continue } // AND WITHIN the block. isSyntheticPrompt is anchored, so a block holding prose followed by // markup — "my question\n…" as ONE block — passed the check and reached // the title intact. Cut here rather than after the join, so a later block cannot be // mistaken for the appended markup of an earlier one. - text := cutTrailingHarness(blk.Text) + text := cutTrailingHarness(blockText) if text == "" { continue } diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 412b19f8b..06c072c47 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -2204,3 +2204,120 @@ func TestCutTrailingHarness_AnyWhitespaceIndent(t *testing.T) { } } } + +// The same content yields the same title whether or not the turn is attributed. +// +// THE GAP THIS CLOSES: the str and blocks tiers assigned raw text, skipping promptCandidate +// entirely, so an unattributed slash command kept its "…" envelope, failed the +// synthetic check and fell through to the cwd — while the identical content titled correctly with +// origin.kind=human. Unattributed turns are the majority (9940 against 640 on the measured tree), so +// the tier that skipped the unwrapping was the common one. +// +// Both unwrap tests used only human-attributed fixtures, which is why CI was green on it. This one is +// parameterised over attribution precisely so a future tier cannot be added without it. +func TestTitleFromTranscript_AttributionDoesNotChangeTheTitle(t *testing.T) { + const envelope = "review\n/review\nsome/path.md" + for _, tc := range []struct{ name, content, want string }{ + {"slash command envelope", envelope, "/review some/path.md"}, + {"pasted content wrapper", `the pasted body`, "the pasted body"}, + {"plain prose", "how do I build abctl?", "how do I build abctl?"}, + {"prose with trailing harness block", "my real question\nhidden", "my real question"}, + // A harness-output wrapper still falls through in BOTH cases: its body is not the user's. + {"harness output wrapper", "total 40", "/w/fallback"}, + } { + t.Run(tc.name, func(t *testing.T) { + titleFor := func(origin string) string { + t.Helper() + dir := t.TempDir() + body, err := json.Marshal(tc.content) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/fallback"}`, + `{"type":"user",`+origin+`"message":{"role":"user","content":`+string(body)+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + return got + } + human := titleFor(`"origin":{"kind":"human"},`) + plain := titleFor("") + if human != tc.want { + t.Errorf("attributed title = %q, want %q", human, tc.want) + } + if plain != tc.want { + t.Errorf("unattributed title = %q, want %q", plain, tc.want) + } + if human != plain { + t.Errorf("attribution changed the title: %q vs %q", human, plain) + } + }) + } +} + +// The same holds for a content ARRAY, which reaches the blocks tier rather than str. +// +// Both tiers assigned raw text, so both were affected; a fix to one alone would leave the other. +func TestTitleFromTranscript_AttributionDoesNotChangeBlockTitles(t *testing.T) { + const content = `[{"type":"text","text":"the pasted body"}]` + titleFor := func(origin string) string { + t.Helper() + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/fallback"}`, + `{"type":"user",`+origin+`"message":{"role":"user","content":`+content+`}}`) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + return got + } + human, plain := titleFor(`"origin":{"kind":"human"},`), titleFor("") + if human != "the pasted body" || plain != "the pasted body" { + t.Errorf("attributed = %q, unattributed = %q, want both %q", human, plain, "the pasted body") + } +} + +// stripHarnessSpans is linear in the input, not quadratic in the number of blocks. +// +// It re-sliced the string on every excision and restarted strings.Index from offset 0. Measured end +// to end through titleFromTranscript before the fix: 155KB took 21ms, 620KB 242ms and 2.5MB 2.49s — +// four times the input for eleven times the work. bufio admits lines up to 16MB, so one chatty +// transcript line could stall the harvest for seconds. After: 9ms, 24ms, 54ms. +// +// Asserted as a RATIO rather than a wall-clock bound, so the test says what it means on a loaded CI +// machine: quadratic growth shows up as time scaling with the square of the input, and a 4x input +// step would take ~16x. The 8x ceiling leaves room for noise while still failing the 11x that was +// measured. +func TestStripHarnessSpans_ScalesLinearly(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive") + } + build := func(kb int) string { + var b strings.Builder + for b.Len() < kb*1024 { + b.WriteString("some prose here hidden payload text more prose\n") + } + return b.String() + } + measure := func(s string) time.Duration { + start := time.Now() + stripHarnessSpans(s) + return time.Since(start) + } + small, large := build(256), build(1024) // a 4x step + + // Warm up, so the first allocation does not land inside a measurement. + measure(small) + + ts, tl := measure(small), measure(large) + if ts <= 0 { + ts = time.Microsecond + } + if ratio := float64(tl) / float64(ts); ratio > 8 { + t.Errorf("4x the input took %.1fx the time (%v vs %v) — that is quadratic, not linear", + ratio, tl, ts) + } +} diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 9b3ac1888..605b8b6c3 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -184,6 +184,18 @@ const localProbeTimeout = 2 * time.Second // stub sessions would otherwise linger in the TUI. const refreshInterval = 2 * time.Second +// reharvestInterval is how often the session picker re-reads the coding agent's transcripts. +// +// The picker is where someone sits while deciding which pod to open, and a session started in another +// terminal meanwhile has no title until something re-reads the logs. Three minutes rather than the 2s +// refresh tick: a harvest walks a transcript tree, and the incremental pass only skips work for files +// whose mtime has not moved — so polling it at the session cadence would re-stat the whole tree ninety +// times a minute for a change that arrives every few minutes at best. +// +// Only while the PICKER is open. Once a session view is up the titles on screen are already loaded, +// and a background harvest there would compete with the event stream for no visible gain. +const reharvestInterval = 3 * time.Minute + // Tea messages. type tickMsg time.Time type refreshTickMsg time.Time @@ -361,11 +373,16 @@ type model struct { // unharvested session, an unknown id and an absent file all render the same. sessionsData map[string]SessionMetadata // harvest refreshes sessionsData in the background once the UI is up. Nil disables it. - harvest HarvestFunc - eventCt uint64 // monotonic counter - lastTick time.Time - lastCt uint64 - rate float64 + harvest HarvestFunc + // lastHarvest is when the most recent harvest was STARTED, for the picker's re-harvest cadence. + lastHarvest time.Time + // harvesting guards against stacking: a harvest walks a transcript tree, and a second pass while + // the first is in flight would duplicate the work and race its own write of the metadata file. + harvesting bool + eventCt uint64 // monotonic counter + lastTick time.Time + lastCt uint64 + rate float64 // Connection status. connState connStateInfo @@ -848,6 +865,13 @@ func (m *model) Init() tea.Cmd { // disk names every session the last run saw, so a harvest only ever ADDS titles — and // reading a large ~/.claude takes about as long as everything else at startup put // together. Batched rather than sequenced so neither waits on the other. + // Stamped here, not on arrival: the cadence is measured from when a harvest STARTS, so leaving it + // zero would make the first tick three minutes later look overdue regardless of when the initial + // harvest actually ran. + if m.harvest != nil { + m.harvesting = true + m.lastHarvest = time.Now() + } if m.pane == paneNamespaces { // Picker mode — load agents, then idle until user picks a pod. m.loading = true @@ -1000,6 +1024,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil case harvestedMsg: + m.harvesting = false // Merge, never replace. The harvest sees one agent's config dir, while the map it is // merging into was loaded from a file that may carry entries from another dir or from a // transcript since pruned — the same reason the harvester itself upserts. Replacing @@ -1059,6 +1084,18 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // back-out. Keep the ticker alive so it's ready when the user // re-enters a session. if m.pane == paneNamespaces || m.pane == panePods { + // RE-HARVEST WHILE THE PICKER IS OPEN, so a session started in another terminal is named + // by the time the user scrolls to it. This is the one pane where someone may sit for + // minutes with nothing else refreshing the titles on screen. + // + // Keyed off the existing refresh ticker rather than a second tea.Tick: one timer is + // easier to reason about than two with different periods, and this branch already + // returns on every tick. + if m.harvest != nil && !m.harvesting && time.Since(m.lastHarvest) >= reharvestInterval { + m.harvesting = true + m.lastHarvest = time.Now() + return m, tea.Batch(harvestCmd(m.harvest), refreshTickCmd()) + } return m, refreshTickCmd() } // Refresh the pipeline view too while a pane that displays plugin diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 9b18bdf73..50a5968ad 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -6,8 +6,10 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/charmbracelet/bubbles/table" + tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" "github.com/muesli/termenv" @@ -964,3 +966,94 @@ func TestLooksLikePath(t *testing.T) { } } } + +// runBatch invokes a command and, if it is a tea.Batch, every member it carries. +// +// tea.Batch does not run its members: it returns a tea.BatchMsg, which the runtime then dispatches. +// A test asserting that a batched command reached a closure has to do that dispatch itself. +func runBatch(t *testing.T, cmd tea.Cmd) { + t.Helper() + if cmd == nil { + return + } + switch msg := cmd().(type) { + case tea.BatchMsg: + for _, c := range msg { + runBatch(t, c) + } + } +} + +// The session picker re-harvests periodically, so a session started elsewhere gets named. +// +// The picker is the one pane where someone may sit for minutes with nothing refreshing the titles: +// the 2s tick skips the session fetch there (m.client may be nil), and the harvest previously ran +// once at Init. A session started in another terminal meanwhile stayed nameless until the viewer was +// restarted. +func TestPicker_ReHarvestsOnAnInterval(t *testing.T) { + newPicker := func(harvest HarvestFunc) *model { + m := newTitleModel(t, map[string]SessionMetadata{}) + m.pane = paneNamespaces + m.harvest = harvest + return m + } + called := 0 + harvest := func() (map[string]SessionMetadata, error) { + called++ + return map[string]SessionMetadata{"s1": {Title: "found later"}}, nil + } + + // Not yet due: the stamp is fresh, so the tick only re-arms the timer. + m := newPicker(harvest) + m.lastHarvest = time.Now() + if _, cmd := m.Update(refreshTickMsg(time.Now())); cmd == nil { + t.Fatal("the refresh ticker was not re-armed") + } + if called != 0 { + t.Errorf("harvested %d times while not due, want 0", called) + } + + // Due: the returned command must include the harvest, which we run to observe it. + m = newPicker(harvest) + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + _, cmd := m.Update(refreshTickMsg(time.Now())) + if cmd == nil { + t.Fatal("no command returned when a re-harvest was due") + } + // tea.Batch returns a BatchMsg holding the member commands rather than running them, so the + // members have to be invoked to reach the harvest closure. + runBatch(t, cmd) + if called == 0 { + t.Error("a due re-harvest did not run the harvester") + } + if !m.harvesting { + t.Error("the in-flight guard was not set, so a second tick could stack a harvest") + } + + // While one is in flight, a further tick must not start another — even once the interval has + // elapsed again. Backdating the stamp is what makes this test the guard's: without it the tick is + // simply not due, so the assertion passed with the guard removed (confirmed by mutation). + before := called + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + _, stacked := m.Update(refreshTickMsg(time.Now())) + runBatch(t, stacked) + if called != before { + t.Errorf("a harvest was stacked while one was in flight (%d -> %d)", before, called) + } + + // The arriving result clears the guard. + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "found later"}}}) + if m.harvesting { + t.Error("harvestedMsg did not clear the in-flight guard") + } + if got := m.sessionTitle("s1"); got != "found later" { + t.Errorf("the re-harvested title did not reach the model: %q", got) + } + + // A nil harvester (--skip-claude-metadata) must never be called. + m = newPicker(nil) + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + if _, c := m.Update(refreshTickMsg(time.Now())); c == nil { + t.Error("the ticker must stay armed even with no harvester") + } +} From b4fa7c51d92e79b816ca8a59d3cd97c8d5021330 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 11:58:16 -0400 Subject: [PATCH 12/16] fix(abctl): Make normalizeTitle a fixed point, close three markup leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX: normalizeTitle was not a fixed point. The pass that drops invisible runes ran AFTER the markup loop had converged, and deleting a rune is exactly the kind of rewrite that exposes work for those passes. Splitting a tag with any invisible rune hid it from every pass; dropping that rune afterwards RE-JOINED the tag and wrote intact markup to the file: "<​system-reminder>INJECTED" -> "INJECTED" Reachable from one ordinary transcript line, with any of zero-width space, bidi override, bidi isolate, combining mark, variation selector, ZWJ or soft hyphen. ~/.cortex/session-metadata.json is read by consumers other than the TUI, which are expected to treat it as plain text. The mid-NAME variant is the one that needed a test name of its own, because it does not look like a failure. "hello SECRET world" came out as "hello SECRET world" — clean prose, with a harness block's body promoted into the title and nothing on screen to flag it. Fixed by hoisting the scrub INTO the loop, the same ordering argument the code already made for stripANSI preceding stripHarnessSpans. Writing that test then exposed two more: - Three runes are BOTH whitespace and non-graphic (U+000B, U+000C, U+0085/NEL). The whitespace arm came first, so they folded to a VISIBLE space — and a space is not a name character, so "<\u0085system-reminder>BODY" became "< system-reminder>BODY". tagSpanLen correctly declined to call that a tag, the block was never removed with its body, and the body became the title. Dropped now, rather than folded. - The fix for that initially went too far and dropped \n and \t as well, joining words in ordinary prose ("line one\nline two" -> "line oneline two"). Separators fold; only the exotic non-printing whitespace is dropped. Caught by an existing test, which is what it was there for. The three orderings are mutually constraining and each is now pinned by a mutation: stripANSI must see the ESC intact (or "" leaks its body), the scrub must run inside the loop, and the separator set must not be plain unicode.IsSpace. tagSpanLen terminated a span at the first ">" byte, but HTML permits ">" inside a quoted attribute value, so the span closed early and residual markup survived: `link` left `y">link` in the title. Worse on a harness tag, where an opening tag hiding a ">" meant the block was unwrapped to its body instead of removed with it. Both quote characters are tracked, since either may contain the other unescaped. looksLikePath tested for a separating space with an ASCII-only IndexAny(rest, " \t"), so "/review docs/plan.md" had no separator by that test, the second "/" made it a path, and it was left-truncated — discarding the command name, the exact regression the function exists to prevent. IndexFunc(rest, unicode.IsSpace) instead. Only reachable from a stale or hand-edited metadata file now that the harvester folds every unicode space, and asserted anyway: a renderer should not assume its input came from the current harvester. MaxTitleLen's doc comment claimed TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures held the cross-module contract, but that test's fixture has no leading "/", so looksLikePath is false and it exercised truncRight alone. Mutating truncLeft to a passthrough left the named test GREEN while ten others in the package failed. Fixed the test rather than the comment — a path-shaped CJK fixture at exactly the cap routes down the other branch — so the comment is now true and the mutation fails it. Real tree, both config dirs, 174 sessions: 171 named, 0 hidden code points, 0 non-U+0020 whitespace, 0 tag spans. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 136 +++++++++++++---- .../authlib/observe/claude/harvest_test.go | 144 ++++++++++++++++++ authbridge/cmd/abctl/tui/sessions_pane.go | 10 +- .../cmd/abctl/tui/sessions_title_test.go | 65 +++++++- 4 files changed, 314 insertions(+), 41 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 0be4375c9..05bc0cb2b 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -692,63 +692,120 @@ func normalizeTitle(s string) string { // So the rule here is positional-independent and applies to every tier: a title carries no tags // at all. Callers still unwrap envelopes and drop harness-output wrappers, because those // decisions are about WHICH TEXT to use; this is about what may appear in the result. + // // ORDER, AND THEN A FIXED POINT. // - // stripANSI runs FIRST because an escape sequence inside a tag name hides that tag from the only - // pass that removes a block together with its body: "INJECTED" - // was invisible to stripHarnessSpans, and stripTags then unwrapped it to bare "INJECTED". - // Removing the escapes first makes the tag whole again. + // Every pass here REWRITES the string, so any of them can expose work for another, and the + // exposure runs in every direction — which is why they are applied until nothing changes rather + // than once each in a hand-picked order. + // + // stripANSI PRECEDES scrubRunes, and scrubRunes PRECEDES the markup passes. Both orderings are + // forced, in opposite directions, which is why this sequence is not arbitrary. + // + // stripANSI must see the ESC byte intact, because it consumes a whole sequence as a unit and + // scrubRunes would otherwise delete just the ESC and leave the payload as text: "\x1b[31mred" + // became "[31mred", and inside a tag name "" became + // "" — still not the tag, so its body still leaked. + // + // scrubRunes is INSIDE the loop, and that placement is the whole point. It used to run after the + // loop had converged, and deleting a rune is exactly the kind of rewrite that exposes work: + // + // "<\u200bsystem-reminder>BODY" -> the tag name is split, so no + // pass recognised it; dropping the zero-width space then RE-JOINED "<" with its tag name and + // the output was intact markup, "BODY". + // + // Any invisible rune did it — zero-width space, bidi override, combining mark, variation + // selector — and the file is what consumers other than the TUI read. The mid-NAME variant was + // worse than the mid-bracket one: "hello SECRET" came out as + // "hello SECRET world", which reads as ordinary prose while a harness block's body has been + // promoted into the title with nothing visible to flag it. // - // And the three are applied until nothing changes, rather than once each. Every one of them - // REWRITES the string, so any of them can expose work for another: removing an escape can join a - // tag name, and removing a harness span can bring two halves of prose together. + // stripANSI precedes the markup passes for the same reason one order down: an escape sequence + // inside a tag name hides that tag from the only pass that removes a block together with its + // body, and stripTags would then unwrap it to its bare body. // - // REDUNDANT TODAY, and recorded as such rather than left to look load-bearing: with the order - // above, one pass handles every input tried, including escapes nested inside tags inside escapes - // — pinning the loop to a single iteration fails no test. It is kept because the argument for - // one pass being enough is an argument about orderings, and "the orderings someone thought of" - // is exactly what six rounds of review kept flanking. Bounded because each pass only ever - // deletes, so the length strictly decreases until it stabilises. + // Bounded because each pass only ever deletes, so the length strictly decreases until it + // stabilises. Looping past convergence is cheap; the passes are linear and titles are short. for i := 0; i < maxNormalizePasses; i++ { before := s s = stripANSI(s) + s = scrubRunes(s) s = stripHarnessSpans(s) s = stripTags(s) if s == before { break } } + return strings.TrimSpace(s) +} - // ONE LINE, and only characters that print as themselves. - // - // The old version collapsed whitespace and stopped there, leaving ESC sequences, C1 controls, - // bidi overrides and zero-width characters in the written file. That was safe only because the - // viewer runs sanitizeLabel over every cell — so the guarantee lived in one consumer, and - // anything else reading ~/.cortex/session-metadata.json got the raw bytes. A title is meant to - // be plain text with nothing hidden in it, which has to be true of the FILE, not of one reader. - // - // An ALLOWLIST, not a denylist of known-bad ranges: unicode.IsGraphic is false for every - // control, format, surrogate and unassigned code point, so a category nobody enumerated cannot - // leak through. Whitespace is normalised to a single space and everything else non-graphic is - // dropped rather than replaced, since a run of U+FFFD tells a reader nothing. +// scrubRunes reduces s to one line of characters that print as themselves. +// +// Split out of normalizeTitle so it can run INSIDE that loop rather than after it; see the ordering +// argument there for what running it last allowed through. +// +// The old version collapsed whitespace and stopped, leaving ESC sequences, C1 controls, bidi +// overrides and zero-width characters in the written file. That was safe only because the viewer +// runs sanitizeLabel over every cell — so the guarantee lived in one consumer, and anything else +// reading ~/.cortex/session-metadata.json got the raw bytes. A title is meant to be plain text with +// nothing hidden in it, which has to be true of the FILE, not of one reader. +// +// An ALLOWLIST, not a denylist of known-bad ranges: unicode.IsGraphic is false for every control, +// format, surrogate and unassigned code point, so a category nobody enumerated cannot leak through. +// +// EVERY space becomes U+0020. unicode.IsSpace covers the exotic ones — U+00A0, U+3000, the U+2000 +// block — and they are normalised rather than preserved, so a title holds no whitespace a reader +// cannot see for what it is. That also removes them as a way to smuggle shape past a consumer that +// splits on ASCII space; see looksLikePath, which had that bug. +// isSeparatorSpace reports whether r is whitespace that should become a single space rather than be +// dropped outright. +// +// The printing spaces, plus the three ASCII separators that structure prose: newline, carriage return +// and tab. Deliberately NOT unicode.IsSpace, which also claims U+000B, U+000C and U+0085 — see the +// non-graphic arm in scrubRunes for why those three are dropped instead. +func isSeparatorSpace(r rune) bool { + switch r { + case '\n', '\r', '\t': + return true + } + return unicode.IsSpace(r) && unicode.IsGraphic(r) +} + +func scrubRunes(s string) string { var b strings.Builder b.Grow(len(s)) prevSpace := true // leading whitespace is dropped for _, r := range s { switch { - case unicode.IsSpace(r): + case isSeparatorSpace(r): + // FOLDS TO U+0020. Two disjoint groups reach here and both are real gaps a reader sees or + // a writer typed: the printing spaces (U+0020, U+00A0, U+3000, the U+2000 block), and the + // LINE AND TAB separators, which do not print themselves but separate words in prose that + // was never one line to begin with. Dropping those joined words — "line one\nline two" + // became "line oneline two" — so they fold rather than vanish. if !prevSpace { b.WriteByte(' ') prevSpace = true } case !unicode.IsGraphic(r): + // EVERY OTHER non-printing rune is DROPPED, including the ones unicode.IsSpace also + // claims: U+000B, U+000C and U+0085 (NEL). That overlap is the subtle part. Folding them + // to a space turned "<\u0085system-reminder>BODY" into + // "< system-reminder>BODY" — and a space is not a name character, so + // tagSpanLen correctly declined to call that a tag, the block was never removed with its + // body, and the body became the title. They are vanishingly rare in prose and a tag-name + // splitter in the adversarial case, so they go. // Controls (C0/C1/DEL), format characters — bidi overrides and isolates, ZWJ, ZWSP, // variation selectors — surrogates and unassigned code points. None of these print as // themselves, and the bidi ones actively reorder what surrounds them. + // + // Dropping a line break rather than folding it to a space is deliberate: a title is one + // line, and the passes above have already removed the markup whose body a break might + // have separated from surrounding prose. case unicode.Is(unicode.Mn, r), unicode.Is(unicode.Me, r), unicode.Is(unicode.Sk, r): // COMBINING AND MODIFYING characters: non-spacing marks, enclosing marks, and modifier // symbols (skin tones). Dropped rather than kept, which is what lets the length cut - // below be a plain rune slice. + // in clipTitle be a plain rune slice. // // They only ever attach to the character before them, so a cut that lands between the // two leaves a dangling mark on whatever now precedes it — or a base character shorn of @@ -768,7 +825,7 @@ func normalizeTitle(s string) string { prevSpace = false } } - return strings.TrimSpace(b.String()) + return b.String() } // clipTitle normalises s and caps it at MaxTitleLen runes. @@ -987,10 +1044,15 @@ func stripTags(s string) string { // tagSpanLen returns the length of the tag starting at s[0], or 0 if s does not start with one. // -// A tag is "<", an optional "/", a name of letters, digits, "-" or "_", then anything up to the -// first ">". That is deliberately narrow: "< 5" and "<" at end-of-string are not tags, so a +// A tag is "<", an optional "/", a name of letters, digits, "-" or "_", then attributes up to the +// closing ">". That is deliberately narrow: "< 5" and "<" at end-of-string are not tags, so a // comparison operator in a real prompt survives, while "
", "" and // "" do not. +// +// A ">" INSIDE A QUOTED ATTRIBUTE VALUE does not close the tag, which HTML permits and the first +// version of this got wrong by scanning for the first ">" byte. `link` closed the +// span at the ">" in the URL, and the rest — `y">link` — survived into the title as residual +// markup. Both quote characters are tracked, since either may contain the other unescaped. func tagSpanLen(s string) int { if len(s) < 2 || s[0] != '<' { return 0 @@ -1011,8 +1073,18 @@ func tagSpanLen(s string) int { if i == nameStart { return 0 // no name: "< 5", "<>", "<=" } - if j := strings.IndexByte(s[i:], '>'); j >= 0 { - return i + j + 1 + var quote byte // 0 outside a quoted value, else the quote character awaiting its match + for ; i < len(s); i++ { + switch c := s[i]; { + case quote != 0: + if c == quote { + quote = 0 + } + case c == '"' || c == '\'': + quote = c + case c == '>': + return i + 1 + } } // An opening tag with no ">" anywhere. Not a span, so the "<" is kept as text — the alternative // is swallowing the rest of the string on a stray bracket. diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 06c072c47..91239cbbf 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -2321,3 +2321,147 @@ func TestStripHarnessSpans_ScalesLinearly(t *testing.T) { ratio, tl, ts) } } + +// TestNormalizeTitle_IsAFixedPointAcrossRuneScrubbing pins the ORDER of normalizeTitle's passes. +// +// The scrub that drops invisible runes used to run AFTER the markup loop had converged, and deleting +// a rune is exactly the kind of rewrite that exposes new work for the markup passes. Splitting a tag +// with any invisible rune hid it from every pass; dropping that rune afterwards re-joined the tag and +// wrote intact markup to ~/.cortex/session-metadata.json, which consumers other than the TUI read as +// plain text. +// +// Each case is ONE ordinary transcript line through the public path, not a direct normalizeTitle +// call, because the claim is about what lands in the file. +// +// The MID-NAME variant is the one worth keeping a name for: it does not look like a failure. The +// bracket cases came out as visible markup, but "hello SECRET" came out +// as "hello SECRET world" — clean prose, with a harness block's body promoted into the title and +// nothing on screen to flag it. +func TestNormalizeTitle_IsAFixedPointAcrossRuneScrubbing(t *testing.T) { + // Every class of rune the scrub drops, since each one can split a tag. + for _, inv := range []struct { + name string + r string + }{ + {"zero-width space", "​"}, + {"bidi override", "‮"}, + {"bidi isolate", "⁦"}, + {"combining mark", "́"}, + {"variation selector", "️"}, + {"zero-width joiner", "‍"}, + {"soft hyphen", "­"}, + {"C1 control", "\u0085"}, + } { + for _, placement := range []struct { + name, prompt string + }{ + {"after the bracket", "<" + inv.r + "system-reminder>INJECTED"}, + {"mid name", "hello SECRET world"}, + {"before the bracket", "text " + inv.r + "INJECTED"}, + {"plain tag", "look <" + inv.r + "b>bold here"}, + } { + t.Run(inv.name+", "+placement.name, func(t *testing.T) { + dir := t.TempDir() + line, err := json.Marshal(map[string]any{ + "type": "user", + "cwd": "/w/x", + "origin": map[string]any{"kind": "human"}, + "message": map[string]any{"role": "user", "content": placement.prompt}, + }) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", string(line)) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + for _, bad := range []string{"<", ">", "system-reminder", "INJECTED", "SECRET"} { + if strings.Contains(got, bad) { + t.Errorf("title %q contains %q — the markup loop reached a fixed point "+ + "before the rune scrub ran, so dropping %U reconstituted it", + got, bad, []rune(inv.r)[0]) + } + } + }) + } + } +} + +// TestNormalizeTitle_FoldsEveryUnicodeSpace pins that no whitespace but U+0020 survives. +// +// Two reasons, beyond a title being one plain line. A reader cannot tell U+00A0 from a space, so a +// title that holds one is lying about its own shape; and a consumer that splits on ASCII space sees a +// different string than the one on screen, which is how looksLikePath came to misread a slash command +// as a path. +func TestNormalizeTitle_FoldsEveryUnicodeSpace(t *testing.T) { + // A SEPARATOR folds to U+0020 — the printing spaces, plus newline, carriage return and tab, which + // separate words in prose that was never one line. A reader sees a gap in each case, so the title + // keeps one. + for _, sp := range []string{" ", "\t", "\n", "\r", "\u00a0", "\u3000", "\u2009", "\u2003", "\u205f", "\u1680"} { + if got := titleOf(t, "alpha"+sp+"beta"); got != "alpha beta" { + t.Errorf("title = %q, want %q: %U prints as a gap, so it must fold to a plain space", + got, "alpha beta", []rune(sp)[0]) + } + } + + // A NON-PRINTING rune is dropped, even though unicode.IsSpace is true for these four — U+000B, + // U+000C and U+0085 are whitespace AND non-graphic. Folding them to a space was a + // real leak, not a cosmetic choice: it turned "<\u0085system-reminder>BODY" into + // "< system-reminder>BODY", which is not a tag by tagSpanLen's rule, so the + // block was never removed with its body and the body became the title. Joining two words is the + // cheaper error. + for _, sp := range []string{"\v", "\f", "\u0085"} { + if got := titleOf(t, "alpha"+sp+"beta"); got != "alphabeta" { + t.Errorf("title = %q, want %q: %U does not print, so it is dropped rather than folded — "+ + "folding it to a space splits a tag name into something tagSpanLen will not match", + got, "alphabeta", []rune(sp)[0]) + } + } +} + +// titleOf writes prompt as the one human turn of a transcript and returns the harvested title. +func titleOf(t *testing.T, prompt string) string { + t.Helper() + dir := t.TempDir() + line, err := json.Marshal(map[string]any{ + "type": "user", + "cwd": "/w/x", + "origin": map[string]any{"kind": "human"}, + "message": map[string]any{"role": "user", "content": prompt}, + }) + if err != nil { + t.Fatal(err) + } + writeSessionTranscript(t, dir, "s.jsonl", string(line)) + got, gerr := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if gerr != nil { + t.Fatal(gerr) + } + return got +} + +// TestTagSpanLen_QuotedAngleBracketDoesNotCloseTheTag pins the quoting rule. +// +// HTML permits ">" inside a quoted attribute value, and scanning for the first ">" byte closed the +// span there — so `link` left `y">link` in the title. Both quote characters, +// since either may contain the other unescaped. +func TestTagSpanLen_QuotedAngleBracketDoesNotCloseTheTag(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + {`hello link world`, "hello link world"}, + {`hi a>b there`, "hi there"}, + {`txt`, "txt"}, + {`keep`, "keep"}, + {`keep`, "keep"}, + // A harness block whose opening tag hides a ">" must still go WITH ITS BODY, not be + // unwrapped to it — the failure mode that makes this more than cosmetic. + {`SECRET`, ""}, + // Unchanged: a comparison operator is not a tag, and an unterminated "<" is kept as text. + {`if a < 5 then`, "if a < 5 then"}, + {`use = 0 { + if i := strings.IndexFunc(rest, unicode.IsSpace); i >= 0 { rest = rest[:i] } return strings.Contains(rest, "/") diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 50a5968ad..bf35c6431 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -863,17 +863,38 @@ func TestTitleCap_IsSafeOnlyBecauseTheRendererRemeasures(t *testing.T) { w, claude.MaxTitleLen) } - const id = "s1" - m := newTitleModel(t, map[string]SessionMetadata{id: {Title: title}}, id) - for _, w := range []int{11, 14, 20, 40} { - got := m.sessionTitleCell(id, w) - if cw := lipgloss.Width(got); cw > w { - t.Errorf("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", - claude.MaxTitleLen, cw, w, got) + // BOTH BRANCHES, because the cap meets a different truncator depending on the title's shape and + // this test named only one of them. The prose fixture above has no leading "/", so looksLikePath + // is false and it exercises truncRight alone — mutating truncLeft to a passthrough left this test + // green while ten others in the package failed. A path-shaped fixture at the same cap routes down + // the other branch, so the constant's doc comment can claim the relationship is guarded here. + pathTitle := "/" + strings.Repeat("日", claude.MaxTitleLen-5) + "/日日日" + if n := len([]rune(pathTitle)); n != claude.MaxTitleLen { + t.Fatalf("path fixture is %d runes, want %d", n, claude.MaxTitleLen) + } + if !looksLikePath(pathTitle) { + t.Fatalf("path fixture %q does not route down the left-truncating branch", pathTitle) + } + + for _, tc := range []struct{ name, title string }{ + {"prose", title}, + {"path", pathTitle}, + } { + 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) + 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", + tc.name, claude.MaxTitleLen, cw, w, got) + } } } + const id = "s1" + m := newTitleModel(t, map[string]SessionMetadata{id: {Title: title}}, id) + // And the same through the rendered row, so the guard covers what a reader actually sees rather // than only the cell helper. // @@ -1057,3 +1078,31 @@ func TestPicker_ReHarvestsOnAnInterval(t *testing.T) { t.Error("the ticker must stay armed even with no harvester") } } + +// TestLooksLikePath_AnyUnicodeSpaceSeparates pins the separator class. +// +// The function's job is to keep a slash command from being left-truncated, and it decided that on an +// ASCII-only IndexAny(rest, " \t"). A command separated by a non-breaking or ideographic space had no +// separator by that test, so the second "/" in its argument made it a path and the command name — the +// part a reader needs — was the part discarded. +// +// Reachable only from a stale or hand-edited metadata file, since the harvester folds every unicode +// space to U+0020 before writing. Asserted here anyway: this package should not depend on its input +// having come from the current harvester. +func TestLooksLikePath_AnyUnicodeSpaceSeparates(t *testing.T) { + for _, sep := range []string{" ", "\t", " ", "　", " ", " ", " "} { + title := "/review" + sep + "docs/plan.md" + if looksLikePath(title) { + t.Errorf("looksLikePath(%q) = true, want false: the %U separator makes this a slash "+ + "command with an argument, not a path — left-truncating it discards the command name", + title, []rune(sep)[0]) + } + } + + // The other direction still holds: a real path has no space at all before its second segment. + for _, title := range []string{"/Users/somebody/src", "/w/x/y", "/a/b"} { + if !looksLikePath(title) { + t.Errorf("looksLikePath(%q) = false, want true", title) + } + } +} From 325ecb4520bb69e1898ca75697ecfa4fd240a408 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 12:11:08 -0400 Subject: [PATCH 13/16] docs(abctl): Correct truncLeft's failure mode, pin looksLikePath's deliberate miss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit truncLeft's comment said that in the table "the library re-truncates from the RIGHT, destroying the tail that left-truncation exists to keep — so the feature inverts for exactly the titles that need it." That names the wrong failure. bubbles renders every cell through runewidth.Truncate(value, width, "…"), which keeps the head and appends its OWN ellipsis, and this function's leading ellipsis is already in place by then. Measured on a 14-column budget: truncLeft, correct "…日日日日日日" 13 columns, tail kept, one ellipsis measured in runes "…日日日日日日日日日日日日日" 27 columns ...after the table "…日日日日日日…" 14 columns, TWO ellipses, tail gone So the cell is not inverted into a right-truncation — it is cut at BOTH ends and keeps the middle, which for a path identifies nothing. At a narrow budget it degenerates completely: 11 columns renders "…日日日日…" and 2 renders "……". Verified against bubbles v1.0.0 table.go:435 and through this package's own render path, not from the library's docs. looksLikePath's doc called out one deliberate miss — a single-segment path with a space, "/tmp foo", reads as prose and is right-truncated — but nothing held it, so the trade was a claim rather than a decision a diff could show changing. Now pinned as characterization rather than endorsement, with the cost measured (truncRight keeps "/tmp …", a bounded wrong-end cut) so widening the rule is visible. The same test covers the opposite direction, which is NOT a miss and is easy to confuse with one: "/review a/b" and "/read docs/x.md" are slash commands whose argument contains a slash, and prose is the right answer for them. One clause produces both answers — the second "/" before any space — so neither behaviour can change alone, which is the reason to assert them together. Verifying this repeated a mistake from two commits ago that is worth recording. Mutating looksLikePath to `return true` left `unicode` unused, so the package did not compile, and a build failure emits no assertion output — which a grep -c counts as zero and reads exactly like "the test did not catch it". Re-run keeping the import referenced: 16 assertion failures across 6 tests. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/sessions_pane.go | 26 +++++++++-- .../cmd/abctl/tui/sessions_title_test.go | 44 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index e7d009c64..a196f754c 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -460,6 +460,12 @@ func (m *model) sessionTitleCell(id string, titleW int) string { // right-truncated. A single-segment path is not something Claude Code records as a cwd, and the cost // is a cell cut at the other end rather than anything unbounded. // +// THAT MISS IS PINNED, by TestLooksLikePath_SingleSegmentPathWithASpaceReadsAsProse, as +// characterization rather than endorsement — it also measures the cost, so widening this rule is a +// visible diff. The same test covers the opposite direction, which is NOT a miss: "/review a/b" is a +// slash command whose argument holds a slash, and prose is the right answer. One clause produces both, +// so neither behaviour can change alone. +// // ANY UNICODE SPACE SEPARATES, not just " " and "\t". An earlier IndexAny(rest, " \t") saw no // separator in "/review\u00a0docs/plan.md", found the second "/", and left-truncated the slash // command — the exact regression above, reachable through a non-breaking space, an ideographic space, @@ -529,10 +535,22 @@ func truncLeft(s string, n int) string { // package documents as content nobody here controls, so CJK and emoji are expected rather // than exotic: measured, a 14-column budget returned 27 columns of CJK. // - // The two failures differ and both are bad. In a header the over-wide string wraps the - // terminal, costing a body row. In the table the library re-truncates from the RIGHT, - // destroying the tail that left-truncation exists to keep — so the feature inverts for - // exactly the titles that need it. + // The two failures differ and both are bad. In a header the over-wide string wraps the terminal, + // costing a body row. + // + // In the table the cell is cut a SECOND time, and the result is worse than either end alone. + // bubbles renders every cell through runewidth.Truncate(value, width, "…"), which keeps the HEAD + // and appends its own ellipsis — so an over-wide left-truncation is cut again from the right, + // with this function's leading ellipsis already in place. Measured on a 14-column budget: + // + // truncLeft, correct "…日日日日日日" 13 columns, tail kept, one ellipsis + // measured in runes "…日日日日日日日日日日日日日" 27 columns + // ...after the table "…日日日日日日…" 14 columns, TWO ellipses, tail gone + // + // So the failure is not that left-truncation inverts into right-truncation — it is that the cell + // ends up cut at BOTH ends and keeps the middle, which is the one part of a path that identifies + // nothing. At a narrow budget it degenerates completely: an 11-column cell renders "…日日日日…" and + // a 2-column one renders "……". // // lipgloss.Width, mirroring padLeft, which measures this way for the same reason. // diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index bf35c6431..154f978ef 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -988,6 +988,50 @@ func TestLooksLikePath(t *testing.T) { } } +// TestLooksLikePath_SingleSegmentPathWithASpaceReadsAsProse pins the rule's ONE deliberate miss. +// +// The doc comment calls this out as wrong on purpose, but nothing held it, so the trade was a claim +// rather than a decision anyone could see change. Characterization, not an endorsement: it asserts +// what the current rule does so that widening it is a visible diff and not a silent one. +// +// "/tmp foo" is a real single-segment directory with a space in it. The rule wants a second "/" before +// any space, so it reads as prose and is truncated from the RIGHT — the opposite end from the one a +// path wants kept. The cost is bounded and small, which is why the rule stays simple: the cell is cut +// at the wrong end, not unbounded, and Claude Code records a cwd with at least two segments, so this +// shape does not arise from harvesting. It can only arrive from a hand-edited metadata file or a +// prompt that happens to look like one. +// +// The OPPOSITE direction is not a miss and belongs here so the two are not confused: "/review a/b" and +// "/read docs/x.md" are slash commands whose argument contains a slash, and prose is the right answer +// for them. The rule gets those right for the same reason it gets "/tmp foo" wrong — it looks for the +// second "/" before any space — so one behaviour cannot be changed without the other. +func TestLooksLikePath_SingleSegmentPathWithASpaceReadsAsProse(t *testing.T) { + // The deliberate miss: a genuine path, classified as prose, right-truncated. + for _, in := range []string{"/tmp foo", "/opt my notes", "/srv a"} { + if looksLikePath(in) { + t.Errorf("looksLikePath(%q) = true, want false — the rule requires a second %q before "+ + "any space, so a single-segment path with a space reads as prose. If this now "+ + "returns true the trade-off changed; update the doc comment with it", in, "/") + } + } + + // And what it costs, measured rather than described: the leaf goes, the head is kept. + const budget = 6 + if got := truncRight("/tmp foo", budget); got != "/tmp …" { + t.Errorf("truncRight(%q, %d) = %q, want %q — this is the cost of the miss above, pinned so "+ + "it is a bounded wrong-end cut and not something worse", "/tmp foo", budget, got, "/tmp …") + } + + // NOT a miss: a slash command with a slash in its argument. Prose is correct, and the same clause + // produces both answers. + for _, in := range []string{"/review a/b", "/read docs/x.md", "/cd /usr/local"} { + if looksLikePath(in) { + t.Errorf("looksLikePath(%q) = true, want false: a slash command reads left-to-right, so "+ + "left-truncating it would discard the command name", in) + } + } +} + // runBatch invokes a command and, if it is a tea.Batch, every member it carries. // // tea.Batch does not run its members: it returns a tea.BatchMsg, which the runtime then dispatches. From bd0dbcc22f6b741dd1e8ae3ff20d547591618d3f Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 12:32:51 -0400 Subject: [PATCH 14/16] fix(abctl): Neuter unknown tag spans instead of swallowing the prose between them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting every tag-shaped span was justified as costing one token — a prompt quoting a tag loses it, in exchange for a guarantee no placement can flank. Measured, two shapes cost far more: "compare ad" -> "compare ad" six words gone "why does List fail" -> "why does List fail" one token, as advertised The first is two comparison operators with prose between them. Nothing there is markup; the name-then-anything rule matched "b and c" and took it. Swallowing a line is data loss, not plainness. So the brackets are what goes now, not the span. Prose keeps its words and nothing tag-shaped survives, which is the property that actually matters downstream. This is safe for the case the deletion existed for BECAUSE the two paths were already separate functions: stripHarnessSpans runs first and removes every name in harnessTagNames together with its body, so an injected instruction is gone before stripTags sees anything. Mutating stripHarnessSpans to a no-op still fails 28 assertions, which is what proves neutering did not quietly take over that path. Deciding what to keep of a span's interior took three attempts, and the middle one is measured rather than guessed: - Keeping everything surfaced machine syntax: `link` became `a href="x>y" link /a`, putting quotes and a ">" back into a title meant to be plain. - Keeping only the NAME was worse — it read "b and c" as a name plus attributes and dropped "and c", reintroducing the exact data loss this change undoes. - On 857 string user turns from two real config dirs: 6615 bare "" spans, 126 attribute-bearing, 45 with a prose interior. All 126 carry "=" or a quote; none of the 45 does. That is the rule. Neutered spans are fenced with spaces, because the brackets were the only separator and dropping them joined words: "List" became "ListString". Also, two test defects in the same area: The plainness property test scanned only the FIRST "<" in each title, so a span after any earlier bracket went unchecked — and a legitimate comparison operator was enough to shadow a real one. Verified directly: on "is 3 < 5 and then PAYLOAD" the old scan reports clean and the new one catches it. TestSessionTitleCell_CarriesNoANSI asserted on sessionTitleCell's return, which makes no Render call, so it held by construction. Moved to the stored row cell — the value this package hands to bubbles, and therefore runewidth's input, which is where the contract lives: bubbles measures it with runewidth (table.go:435, v1.0.0) and runewidth is not ANSI-aware, so escape bytes there are charged against the column budget. Deliberately NOT the rendered View(), which legitimately contains escapes (tableStyles sets Selected to bold-on-background) and would fail on correct output. I had the comment wrong first and checked where the escapes actually live before writing it. SCOPE, stated plainly: on my own tree this changes NOTHING — 174 sessions, 171 named, 3 cwd, 0 hidden code points, 0 non-U+0020 whitespace, 0 tag spans, byte-identical titles before and after. The 45 prose-interior spans are in message bodies that never became titles. This is robustness against shapes that exist in principle, not a fix for a live defect on this data. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 89 +++++++++++++++-- .../authlib/observe/claude/harvest_test.go | 96 +++++++++++++++++-- .../cmd/abctl/tui/sessions_title_test.go | 38 +++++++- 3 files changed, 204 insertions(+), 19 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 05bc0cb2b..0ea5a4353 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -998,16 +998,44 @@ func stripHarnessSpans(s string) string { return s } -// stripTags removes every <...> span from s, wherever it appears. +// stripTags NEUTERS every tag-name-shaped span in s: the angle brackets go, the text between them +// stays. // // POSITION-INDEPENDENT, which is the point: the shape-by-shape filters above it each guard one // placement, and review found a new placement each round. This one cannot be flanked. // -// Unbalanced or nested markup is handled by construction, since it only ever deletes from a "<" to -// the next ">" and leaves a lone "<" alone. That last part is deliberate: "is 3 < 5 in Go?" and -// "List" are the shapes a real prompt carries, and the first must survive. The second is -// sacrificed — a prompt that genuinely quotes a tag loses it — which is the trade this asks for: -// titles are plain, and a plain title cannot also faithfully quote markup. +// IT USED TO DELETE THE WHOLE SPAN, and that cost more than the trade it was justified by. The +// argument was that a plain title cannot also faithfully quote markup, so a prompt quoting a tag +// loses it — one token, for an unflankable guarantee. Measured, two shapes cost far more than a +// token: +// +// "compare ad" -> "compare ad" +// "why does List fail" -> "why does List fail" +// +// Neither is markup. The first is two comparisons, and everything between them matched the +// name-then-anything rule, so six words of prose went with it. Swallowing a line is not a plain-text +// guarantee, it is data loss. +// +// NOT FIXED HERE, and deliberately: "the tag is what I mean" still titles as "the". +// That span carries a KNOWN name, so stripHarnessSpans has already consumed it and everything after +// it before this pass runs — an unclosed harness tag is treated as running to end-of-string, because +// "prose INJECTED payload" has the identical shape and truncating at the ">" would +// leak the payload. Telling a legitimate mention from an injection means guessing which trailing text +// is prose, and guessing wrong on the second is the failure this whole file exists to prevent. A +// truncated title for a prompt about the harness is the cheaper error. +// +// So the brackets are what goes. Prose keeps its words, and nothing tag-shaped survives — which is +// the property that matters downstream and the one the plainness test asserts generically. +// +// WHY THIS IS STILL SAFE for the case the deletion existed for: a harness block never reaches here. +// stripHarnessSpans runs first and removes every name in harnessTagNames TOGETHER WITH ITS BODY, so +// an injected instruction is already gone. What is left for this pass is markup with a name nobody +// enumerated, and for that, inert-but-noisy beats swallowing the line: +// +// "X" -> "unknown-tag X /unknown-tag" +// +// A lone "<" is still left alone, so "is 3 < 5 in Go?" and "List" both survive whole — the +// second now intact rather than sacrificed. func stripTags(s string) string { if !strings.ContainsRune(s, '<') { return s @@ -1033,6 +1061,30 @@ func stripTags(s string) string { // and is kept, and each span is judged on its own. if n := tagSpanLen(s[i:]); n > 0 { b.WriteString(s[:i]) + // THE INTERIOR, UNLESS IT IS ATTRIBUTE SYNTAX. Both extremes lose real text, and the + // middle is measurable rather than a guess. + // + // Keeping everything surfaced machine syntax: `link` neutered to + // `a href="x>y" link /a`, putting quotes and a ">" back into a title meant to be plain. + // Keeping only the NAME was worse — it read `b and c` in "compare ad" as a name + // plus attributes and dropped "and c", which is the data loss this change exists to + // undo. + // + // Measured on 857 string user turns from two real config dirs: 6615 bare "" spans, + // 126 attribute-bearing, and 45 whose interior is prose. EVERY one of the 126 carries an + // "=" or a quote (`from="a505…"`, `id="2e21"`, `pasted_content id=…`) and none of the 45 + // does. So that is the test: an interior holding "=", a double or a single quote is + // machine syntax and only the name survives; anything else is text the user may have + // typed and is kept whole. + // + // FENCED WITH SPACES, because the brackets were the only thing separating the span from + // its neighbours and dropping them silently joined words: "List" became + // "ListString" and "compare ad" became "compare ab and cd". Two tokens a reader + // can see beats one word that never existed. scrubRunes collapses the runs and trims the + // ends, so a fence at the start or between adjacent spans costs nothing. + b.WriteByte(' ') + b.WriteString(neuteredSpan(s[i : i+n])) + b.WriteByte(' ') s = s[i+n:] continue } @@ -1053,6 +1105,31 @@ func stripTags(s string) string { // version of this got wrong by scanning for the first ">" byte. `link` closed the // span at the ">" in the URL, and the rest — `y">link` — survived into the title as residual // markup. Both quote characters are tracked, since either may contain the other unescaped. +// neuteredSpan returns what survives of the tag span s once its brackets are gone. +// +// s must be exactly one span as measured by tagSpanLen. The interior, minus the brackets and any +// leading slash — except that an interior carrying attribute syntax ("=", a double or single quote) +// keeps only the tag name, since such an interior is machine-generated rather than typed. See +// stripTags for the measurement behind that rule. +// +// "
" -> "div" +// "
" -> "div" +// "" -> "b and c" (prose between two comparisons) +// `` -> "a" (attribute syntax: name only) +func neuteredSpan(s string) string { + in := strings.TrimPrefix(strings.TrimSuffix(strings.TrimPrefix(s, "<"), ">"), "/") + if !strings.ContainsAny(in, `="'`) { + return in + } + if i := strings.IndexFunc(in, func(r rune) bool { + return !(r >= 'a' && r <= 'z') && !(r >= 'A' && r <= 'Z') && + !(r >= '0' && r <= '9') && r != '-' && r != '_' + }); i >= 0 { + return in[:i] + } + return in +} + func tagSpanLen(s string) int { if len(s) < 2 || s[0] != '<' { return 0 diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 91239cbbf..2b367786e 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -1577,12 +1577,12 @@ func TestTitleFromTranscript_NoHarnessMarkupInTitles(t *testing.T) { // review. The prompt stays readable, which is what the title is for. "a quoted tag is stripped, prose survives", []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"why does
break my layout?"}}`}, - "why does break my layout?", + "why does div break my layout?", }, { "generics are stripped, prose survives", []string{`{"type":"user","origin":{"kind":"human"},"message":{"role":"user","content":"how do I write List in Go?"}}`}, - "how do I write List in Go?", + "how do I write List String in Go?", }, { // A LONE "<" is not a tag and must survive: this is the shape a real prompt carries. @@ -1890,8 +1890,19 @@ func TestTitleFromTranscript_TitlesAreAlwaysPlain(t *testing.T) { // prompt and must survive whole — so the assertion is about a "" span, not about the // characters. An earlier version banned ">" outright and failed that prompt, which would have // pushed the code toward eating comparison operators again. - if tagSpanLen(got[strings.IndexByte(got+"<", '<'):]) > 0 { - t.Errorf("input %q: title carries a tag span: %q", in, got) + // EVERY "<", not just the first. The scan used to start at strings.IndexByte(got, '<') and + // look once, so a span sitting after any earlier bracket went unchecked — and a lone + // comparison operator early in a title was enough to shadow it, since that "<" is legitimately + // not a span. Now every position is offered to tagSpanLen, which is what makes this a property + // of the whole title rather than of its first bracket. + for off := 0; off < len(got); off++ { + if got[off] != '<' { + continue + } + if tagSpanLen(got[off:]) > 0 { + t.Errorf("input %q: title carries a tag span at offset %d: %q", in, off, got) + break + } } // No harness tag name, in any form. for _, tag := range harnessTagNames { @@ -2146,7 +2157,9 @@ func TestClipTitle_KeepsComparisonOperators(t *testing.T) { // A balanced span BEFORE a lone "<" is still stripped: one unbalanced bracket used to // disable stripping for the entire string. {"HARNESSBODY and 3 < 5", "and 3 < 5"}, - {"
x
and 3 < 5", "x and 3 < 5"}, + // An UNKNOWN tag is neutered, not deleted: the name survives as a word, the brackets do not. + // Contrast the harness case above, which is removed with its body — the two paths stay distinct. + {"
x
and 3 < 5", "div x div and 3 < 5"}, } { if got := clipTitle(tc.in); got != tc.want { t.Errorf("clipTitle(%q) = %q, want %q", tc.in, got, tc.want) @@ -2448,11 +2461,11 @@ func titleOf(t *testing.T, prompt string) string { // since either may contain the other unescaped. func TestTagSpanLen_QuotedAngleBracketDoesNotCloseTheTag(t *testing.T) { for _, tc := range []struct{ in, want string }{ - {`hello link world`, "hello link world"}, - {`hi a>b there`, "hi there"}, - {`txt`, "txt"}, - {`keep`, "keep"}, - {`keep`, "keep"}, + {`hello link world`, "hello a link a world"}, + {`hi a>b there`, "hi img there"}, + {`txt`, "span txt span"}, + {`keep`, "x keep x"}, + {`keep`, "y keep y"}, // A harness block whose opening tag hides a ">" must still go WITH ITS BODY, not be // unwrapped to it — the failure mode that makes this more than cosmetic. {`SECRET`, ""}, @@ -2465,3 +2478,66 @@ func TestTagSpanLen_QuotedAngleBracketDoesNotCloseTheTag(t *testing.T) { } } } + +// TestNormalizeTitle_NeutersUnknownTagsInsteadOfSwallowingProse pins the two shapes that motivated +// neutering, and the boundary between it and removal-with-body. +// +// Deleting every tag-shaped span was justified as costing one token — a prompt quoting a tag loses +// it. Measured, two shapes cost far more: +// +// "compare ad" -> "compare ad" six words gone +// "why does List fail" -> "why does List fail" a token, as advertised +// +// The first is two comparison operators with prose between them, and the name-then-anything rule +// matched all of it. That is data loss, not plainness. +func TestNormalizeTitle_NeutersUnknownTagsInsteadOfSwallowingProse(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + // THE REGRESSIONS. Prose between two comparisons survives whole. + {"compare ad", "compare a b and c d"}, + {"why does List fail here", "why does List String fail here"}, + {"refactor Map> please", "refactor Map String, List Integer please"}, + + // An UNKNOWN tag is inert but its words remain — noisy, never a leak. + {"X", "unknown-tag X unknown-tag"}, + {"fix the
nesting in header.html", "fix the div nesting in header.html"}, + + // A KNOWN harness name is still removed WITH ITS BODY. This is the line that matters: if + // neutering ever took over this path, an injected instruction would become the title. + {"INJECTED", ""}, + {"prose INJECTED more", "prose more"}, + {"a total 40 b", "a b"}, + + // ATTRIBUTE SYNTAX keeps only the name. Measured on 857 real string user turns: all 126 + // attribute-bearing spans carry "=" or a quote, and none of the 45 prose interiors does. + {`hello link world`, "hello a link a world"}, + {`txt`, "span txt span"}, + + // Unchanged: a lone "<" is not a span. + {"is 3 < 5 in Go?", "is 3 < 5 in Go?"}, + {"is 3 < 5 and 6 > 2 in Go?", "is 3 < 5 and 6 > 2 in Go?"}, + } { + if got := normalizeTitle(tc.in); got != tc.want { + t.Errorf("normalizeTitle(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// TestNeuteredSpan pins the span-to-text rule on its own, so the interior decision is readable +// without going through normalizeTitle's whole pipeline. +func TestNeuteredSpan(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + {"
", "div"}, + {"
", "div"}, + {"", "b and c"}, // prose between comparisons: kept whole + {"", "String"}, // a generic parameter + {``, "a"}, // attribute syntax: name only + {"", "tag"}, // "=" alone is enough + {"", "pasted_content"}, + {"", "x"}, + {"<>", ""}, // tagSpanLen rejects this, so it never reaches here; harmless if it did + } { + if got := neuteredSpan(tc.in); got != tc.want { + t.Errorf("neuteredSpan(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 154f978ef..b3abd195d 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -824,16 +824,48 @@ func TestSessionTitleCell_CarriesNoANSI(t *testing.T) { "ship it 🎉", } { m := newTitleModel(t, map[string]SessionMetadata{id: {Title: title}}, id) + + // 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) - if strings.ContainsRune(got, 0x1b) { - t.Errorf("title cell carries an escape byte at width %d: %q", w, got) - } if lipgloss.Width(got) > w { t.Errorf("title cell is %d columns against a %d-column budget: %q", lipgloss.Width(got), w, got) } } + + // The ESCAPE half, on the STORED CELL — the string this package hands to bubbles. + // + // It used to assert on sessionTitleCell's return, which makes no Render call, so it held by + // construction. The stored row cell is one step further along and is the value that actually + // matters for the hazard MaxTitleLen's doc comment describes: bubbles renders every cell as + // styles.Cell.Render(style.Render(runewidth.Truncate(value, width, "…"))) — table.go:435 in + // v1.0.0 — and runewidth is NOT ANSI-aware. So escape bytes in `value` are charged against + // the column budget and a narrow cell collapses to a lone ellipsis. Asserting on the stored + // value is asserting on runewidth's input, which is where the contract lives. + // + // NOT the rendered View(): that string legitimately contains escapes — tableStyles sets + // Selected to bold-on-background and DefaultStyles pads every cell — so a scan for 0x1b + // there would fail on correct output and says nothing about the title. + // + // forceColor is above, so lipgloss emits real escapes and a styled title is caught. + for _, termW := range []int{80, 100, 200} { + m.width = termW + m.sessionsTbl.SetColumns(sessionsColumnsFor(termW)) + m.rebuildSessionsTable() + cell := sessionsCell(t, m, titleRow(t, m, id), "TITLE") + if strings.ContainsRune(cell, 0x1b) { + t.Errorf("stored TITLE cell carries an escape byte at terminal width %d: %q — "+ + "bubbles measures this value with runewidth, which counts escape bytes against "+ + "the column budget and would collapse a narrow cell to an ellipsis", termW, cell) + } + titleW := sessionsColumnWidth(sessionsColumnsFor(termW), "TITLE") + if lipgloss.Width(cell) > titleW { + t.Errorf("at terminal width %d: stored TITLE cell is %d columns against a "+ + "%d-column column: %q", termW, lipgloss.Width(cell), titleW, cell) + } + } } } From f22a720cff662089493347ab7ec098209551895a Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 13:02:34 -0400 Subject: [PATCH 15/16] fix(abctl): Match harness tags by canonical name, not by their bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the five reported problems are ONE defect. tagSpanLen decides what a tag span is and accepts A-Z in a name; stripHarnessSpans compared against an all-lowercase list with strings.Index. So every way a real name can differ from its literal form silently downgraded "remove with body" to "neuter the brackets" — and neutering a harness tag PROMOTES its body into the title, which is the one outcome this file exists to prevent: INJECTED -> "SYSTEM-REMINDER INJECTED SYSTEM-REMINDER" INJECTED -> "system^-reminder INJECTED system^-reminder" -> "system-reminder" isSyntheticPrompt already lowercased and documented why; the function that removes the body did not. Fixed at the seam rather than in three places. stripHarnessSpans now finds spans STRUCTURALLY with tagSpanLen — the same parser stripTags uses — and judges each by canonical name: lowercased, with non-alphanumerics dropped. One parser decides what a span IS, one lookup decides what it MEANS, so the two cannot disagree the way a second literal scanner did. The orphan close falls out for free, because neuteredSpan already drops a leading slash, so a close canonicalises to its opener's name. Dropping non-alphanumerics rather than enumerating runes to delete is deliberate: U+02C6 is category Lm, so scrubRunes does not drop it (it drops Mn/Me/Sk), which means nothing in the pipeline deletes the rune and the fixed-point loop cannot rescue the match. Canonicalising the NAME needs no change when the next category turns up. The cwd accumulator tested the RAW value for emptiness, unlike all five prompt tiers, which normalise first. A cwd that normalises away to nothing — whitespace only, an ESC sequence — counted as present, and last-wins let it overwrite a good value from an earlier line; since the cwd is the final fallback the session then titled as "" rather than falling through. Normalised before the guard now. FINDING 3 DOES NOT REPRODUCE, reported rather than worked around. The claim was that neuteredSpan re-emits an interior "<" verbatim, so each left-nested bracket costs a pass, nine nested pairs exhaust maxNormalizePasses and a live span survives. neuteredSpan returns the interior WITHOUT its brackets, so nesting consumes no passes: measured at depths 1-24 for three tag names, there is no surviving span, one pass suffices, and the result is idempotent. Pinned as a test so the question is answerable next time, and because two comments in this file now lean on that idempotence. Behaviour-preserving on everything already covered — the whole package passes unchanged — and TestStripHarnessSpans_ScalesLinearly still holds, which matters because this rewrote the loop that finding was about. Real tree: 175 sessions, 172 named, 3 cwd, 0 hidden code points, 0 non-U+0020 whitespace, 0 tag spans. One case I did NOT make the guard reject: a cwd of "" normalises to the non-empty "b b" and so wins by last-wins. That is the tier's own rule rather than a defect of the emptiness test, and a path-shape check here would be a new heuristic of exactly the kind six review rounds kept flanking. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 209 ++++++++++++------ .../authlib/observe/claude/harvest_test.go | 123 +++++++++++ 2 files changed, 269 insertions(+), 63 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index 0ea5a4353..b18e545a9 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -485,8 +485,17 @@ func titleFromTranscript(path string) (string, error) { if err := json.Unmarshal(line, &e); err != nil { continue } - if e.Cwd != "" { - cwd = e.Cwd + // NORMALISED BEFORE THE GUARD, like all five prompt tiers, rather than tested raw. + // + // A raw `!= ""` accepted a cwd that normalises away to nothing — whitespace only, an ESC + // sequence, a bare tag — and last-wins then let it CLOBBER a good value from an earlier line, + // so the session titled as "" instead of "/w/good". The cwd is the last fallback, so an empty + // result here is an unnamed session, not a fall-through to something else. + // + // normalizeTitle, not clipTitle: a path's distinguishing end is its leaf, so this tier + // deliberately keeps its full length and the renderer truncates from the left. + if c := normalizeTitle(e.Cwd); c != "" { + cwd = c } // Guarded on Type as well as on the value: "aiTitle" appearing on some other // line kind is not a title claim. @@ -623,6 +632,11 @@ func titleFromTranscript(path string) (string, error) { // // It is still normalised — whitespace collapsed, markup and non-graphic runes dropped — so a // cwd cannot carry hidden characters into a cell any more than a prompt can. + // + // NORMALISED TWICE, and deliberately so: the accumulator above normalises each candidate to decide + // whether it is empty, and this call is the guarantee for the RETURN. normalizeTitle is idempotent + // (asserted, since its fixed-point loop is what makes that true), so the second pass costs one + // walk and means this line does not depend on every assignment upstream having been normalised. return normalizeTitle(cwd), err } @@ -928,74 +942,93 @@ func stripANSI(s string) string { // value. That last one is the leak review found this round; the rest are the leaks it // found in the rounds before. func stripHarnessSpans(s string) string { - for _, tag := range harnessTagNames { - if !strings.Contains(s, tag) { + if !strings.ContainsRune(s, '<') { + return s + } + var b strings.Builder + b.Grow(len(s)) + // A CURSOR, and one output buffer. This used to re-slice s on every excision and then restart + // strings.Index from offset 0, which is quadratic in the number of blocks: measured end to end + // through titleFromTranscript, 155KB took 21ms, 620KB 242ms and 2.5MB 2.49s — four times the input + // for eleven times the work — and bufio admits lines up to 16MB, so a single chatty transcript line + // could stall the harvest for seconds. cutTrailingHarness already advanced a cursor; this matches + // it. + // + // SPANS ARE FOUND STRUCTURALLY, by tagSpanLen, and then judged by CANONICAL NAME. This used to + // scan for each literal tag string from the list, which made the match as narrow as a byte + // compare — and every way a real name can differ from its literal form downgraded "remove with + // body" to "neuter the brackets", promoting the body into the title: + // + // "INJECTED" case + // "INJECTED" a rune wedged into the name + // "" an orphan close, byte 1 is "/" + // + // One parser, one lookup: tagSpanLen decides what a span IS and isHarnessSpan decides what it + // MEANS, so the two cannot disagree the way a second literal scanner did. + for pos := 0; pos < len(s); { + i := strings.IndexByte(s[pos:], '<') + if i < 0 { + b.WriteString(s[pos:]) + break + } + i += pos + n := tagSpanLen(s[i:]) + if n == 0 || !isHarnessSpan(s[i:i+n]) { + // Not a harness span. Emit through this "<" and keep looking; stripTags handles whatever + // neutering the rest needs. + b.WriteString(s[pos : i+1]) + pos = i + 1 continue } - closeTag := "INJECTED payload" kept - // "INJECTED payload" — with no escape sequence needed. A harness block that is not - // closed is still a harness block, and everything after it is its content as far as - // anyone can tell. - // - // THE MATCHING CLOSE, counting depth — not the first one. With the same tag NESTED, - // "abLEAKED" ended - // its span at the INNER close, so the outer close and everything before it survived as - // "LEAKED". The next iteration then found no opening tag, because this one had consumed - // it, so the fixed-point loop could not recover it either. - // - // Different-name nesting was already handled, since each tag is scanned separately, and - // that is why the corpus missed this: it covered the case that worked. - // - // The depth scan walks each byte of the span at most once, and the cursor never revisits - // it, so the whole pass is linear in len(s) per tag. - end := len(s) - depth := 0 - for j := i; j < len(s); { - switch { - case strings.HasPrefix(s[j:], tag): - depth++ - j += len(tag) - case strings.HasPrefix(s[j:], closeTag): - depth-- - if depth == 0 { - if g := strings.IndexByte(s[j:], '>'); g >= 0 { - end = j + g + 1 - } - j = len(s) - continue - } - j += len(closeTag) - default: - j++ + // The span runs to the matching close when there is one, and TO THE END OF THE STRING + // otherwise. + // + // Not to the end of the opening tag, which is what it did: an unclosed block then left its + // body behind as bare prose — "prose INJECTED payload" kept "INJECTED + // payload" — with no escape sequence needed. A harness block that is not closed is still a + // harness block, and everything after it is its content as far as anyone can tell. + // + // THE MATCHING CLOSE, counting depth — not the first one. With the same tag NESTED, + // "abLEAKED" ended its + // span at the INNER close, so the outer close and everything before it survived as "LEAKED". + // The next iteration then found no opening tag, because this one had consumed it, so the + // fixed-point loop could not recover it either. + // + // Depth is counted over spans of THE SAME canonical name, so a different harness tag nested + // inside does not close this one; it is removed with this span's body regardless. + // + // The depth scan walks each byte of the span at most once, and the cursor never revisits it, + // so the whole pass is linear in len(s). + want := canonicalTagName(neuteredSpan(s[i : i+n])) + end := len(s) + depth := 0 + for k := i; k < len(s); { + if s[k] != '<' { + k++ + continue + } + m := tagSpanLen(s[k:]) + if m == 0 || canonicalTagName(neuteredSpan(s[k:k+m])) != want { + k++ + continue + } + if strings.HasPrefix(s[k:], "" // would truncate a prompt that quotes HTML or generics. +// canonicalTagName reduces a tag name to the form the harness-tag lookup compares. +// +// Lowercased, and stripped of everything that is not a letter or digit. Both halves close a body-leak +// class rather than being tidiness: +// +// - CASE. tagSpanLen accepts A-Z in a name, but the tag list is all lowercase and was compared byte +// for byte, so "INJECTED" matched nothing, fell through to the +// neutering pass, and promoted its body into the title. isSyntheticPrompt already lowercased and +// documented why; the function that removes the BODY did not. +// - NON-ALPHANUMERICS. Dropping them handles any rune wedged into a name, not just the hyphens and +// underscores the list happens to contain. U+02C6 is category Lm, so scrubRunes does not drop it +// (it drops Mn/Me/Sk), and "" split the name past any literal compare with +// nothing in the pipeline to remove the rune — so the fixed-point loop could not rescue it either. +// Canonicalising the NAME rather than enumerating runes to delete means the next such category +// needs no change here. +func canonicalTagName(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + case r >= 'A' && r <= 'Z': + b.WriteRune(r + ('a' - 'A')) + } + } + return b.String() +} + +// harnessTagSet is harnessTagNames keyed by canonical name, for the body-removing lookup. +var harnessTagSet = func() map[string]bool { + m := make(map[string]bool, len(harnessTagNames)) + for _, tag := range harnessTagNames { + m[canonicalTagName(tag)] = true + } + return m +}() + +// isHarnessSpan reports whether the tag span s names a harness block, whatever its case, its +// punctuation, or whether it is an opening or a closing tag. +// +// s must be exactly one span as measured by tagSpanLen. neuteredSpan already drops the brackets and a +// leading slash, so a CLOSING tag canonicalises to the same name as its opener — which is what makes +// an orphan "" recognisable. It was not: the scan looked for "INJECTED", ""}, + {"mixed case", "INJECTED", ""}, + {"uppercase, mid-prose", "ok INJECTED end", "ok end"}, + {"close tag cased differently", "INJECTED", ""}, + + // A rune wedged into the name. U+02C6 is category Lm, so scrubRunes does NOT drop it — it + // drops Mn/Me/Sk — which means nothing in the pipeline deletes the rune and the fixed-point + // loop cannot rescue the match. Canonicalising the name is what closes it. + {"Lm rune in the name", "INJECTED", ""}, + {"Lm rune, open only", "INJECTED", ""}, + + // An ORPHAN CLOSE. The scan looked for "", ""}, + {"orphan close, mid-prose", "prose more", "prose"}, + {"orphan close, uppercase", "", ""}, + + // Unchanged: a name that is NOT a harness tag is still only neutered, never body-removed. + {"unknown tag keeps its words", "X", "unknown-tag X unknown-tag"}, + {"comparison is not a span", "is 3 < 5 in Go?", "is 3 < 5 in Go?"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := normalizeTitle(tc.in); got != tc.want { + t.Errorf("normalizeTitle(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// TestNormalizeTitle_IsIdempotentAtEveryNestingDepth settles a reported concern rather than fixing +// one, and keeps it settled. +// +// The claim was that neuteredSpan re-emits an interior "<" verbatim, so each left-nested bracket +// costs one pass, nine nested pairs exhaust maxNormalizePasses and a live tag span survives. It does +// not reproduce: neuteredSpan returns the interior WITHOUT its brackets, so nesting does not consume +// passes at all and one pass handles any depth. +// +// Asserted as two properties, at depths well past the pass limit: no span survives, and a second +// application changes nothing. The second is what "fixed point" means, and other comments in this +// file now lean on it. +func TestNormalizeTitle_IsIdempotentAtEveryNestingDepth(t *testing.T) { + for depth := 1; depth <= 24; depth++ { + for _, tag := range []string{"a", "unknown-tag", "system-reminder"} { + in := strings.Repeat("<"+tag+">", depth) + "BODY" + strings.Repeat("", depth) + once := normalizeTitle(in) + if twice := normalizeTitle(once); once != twice { + t.Errorf("depth %d, <%s>: not idempotent — %q then %q", depth, tag, once, twice) + } + for off := 0; off < len(once); off++ { + if once[off] == '<' && tagSpanLen(once[off:]) > 0 { + t.Errorf("depth %d, <%s>: a live tag span survived at offset %d: %q", + depth, tag, off, once) + break + } + } + // And a harness block's body never survives, whatever the nesting. + if tag == "system-reminder" && strings.Contains(once, "BODY") { + t.Errorf("depth %d: harness body survived: %q", depth, once) + } + } + } +} + +// TestTitleFromTranscript_AnEmptyCwdDoesNotClobberAGoodOne pins the last tier's guard. +// +// The cwd accumulator tested the RAW value for emptiness, unlike all five prompt tiers, which +// normalise first. So a cwd that normalises away to nothing — whitespace only, an ESC sequence — +// counted as present, and last-wins let it overwrite a good value from an earlier line. The cwd is the +// final fallback, so the result was an unnamed session rather than a fall-through. +func TestTitleFromTranscript_AnEmptyCwdDoesNotClobberAGoodOne(t *testing.T) { + for _, tc := range []struct{ name, later string }{ + {"spaces", `" "`}, + {"tab", `"\t"`}, + {"non-breaking space", `" "`}, + {"ideographic space", `"　"`}, + {"escape sequence only", `"\u001b[0m"`}, + {"zero-width space", `"​"`}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/good"}`, + `{"type":"user","cwd":`+tc.later+`}`) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != "/w/good" { + t.Errorf("title = %q, want %q — a cwd that normalises to nothing must not overwrite "+ + "a good one, since this tier is the last fallback", got, "/w/good") + } + }) + } + + // Last-wins still applies to a cwd that normalises to something. Not a defect of the guard: by + // this tier's own rule the later line is the current directory. + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/w/first"}`, + `{"type":"user","cwd":"/w/second"}`) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if got != "/w/second" { + t.Errorf("title = %q, want %q — last-wins is the rule for a non-empty cwd", got, "/w/second") + } +} From 9db46b947e082a2c5537b89240cba02677b52051 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 13:37:38 -0400 Subject: [PATCH 16/16] perf(abctl): Make both truncators linear and bound the cwd tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUST-FIX, and the same shape as 3c0e441's span stripping — these two were left quadratic. truncLeft and truncRight each measured the WHOLE remaining string with lipgloss.Width once per dropped rune. Measured on a single call: truncLeft truncRight 2500 36ms 39ms 5000 142ms 140ms 10000 572ms 595ms 20000 2.33s 2.30s Four times the input for sixteen times the work, and the pane redraws on every poll. Fixed by skipping ahead before measuring: every rune is at least one column, so a run longer than n runes cannot fit n columns and n runes from the relevant end is an exact lower bound — every earlier index is provably too wide to test. That leaves at most n+1 measurements of at most n runes, bounded by the CELL rather than the title. 20000 runes: 2.33s -> 507us, and the remaining cost is the []rune conversion, not the search. THE OBVIOUS BOUND IS NOT UNIVERSALLY TRUE, and a differential test against the old implementation caught it before this was committed. Combining marks, joiners and variation selectors are ZERO columns wide, so on a string of combining marks at n == 1 the skip jumped past marks the one-at-a-time search kept and the two returned different bytes. The skip is therefore guarded by zeroWidthFree. A title never contains such a rune — normalizeTitle drops every Mn/Me/Cf/Cc/Sk, and I verified that across the entire Unicode range, 0 survivors — so the fast path is what runs; the guard exists because these are general helpers whose other callers promise nothing, and a wrong answer is worse than a slow one. 69120 randomized comparisons across six alphabets, all byte-identical, with the zero-width ones included specifically to exercise both sides of the guard. The cwd tier was the input that made this reachable. Five prompt tiers go through clipTitle; this one returned normalizeTitle(cwd) uncapped, reasoning that "a path is bounded by the filesystem". It is not — e.Cwd is a JSON string field and a transcript is a file anything can write — so it was the one unbounded string in the harvester, feeding a per-rune width search. Capped at MaxCwdLen (1024 runes), keeping the TAIL since a path's leaf is what identifies it and that is the end the renderer keeps too. Well above any real path, so a genuine deep directory is untouched; asserted at the cap, one past it, and at 50000. truncRight mattered as much as truncLeft, which the report got right and my earlier linearity pass missed: looksLikePath needs a leading "/", so a RELATIVE cwd routes down the prose branch while just as uncapped. Linearity is asserted as a RATIO, not a wall-clock bound, so it means something on a loaded CI machine: 4x the input must not cost more than 8x the time, which separates linear (~4x) from quadratic (~16x) with room for scheduling noise. NO EXISTING TEST WAS MODIFIED — the whole suite passes untouched under -race, which is the evidence the change is behaviour-preserving. Real tree: 176 sessions, 173 named, 3 cwd, 0 problems, longest title 80. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/observe/claude/harvest.go | 36 ++++- .../authlib/observe/claude/harvest_test.go | 45 ++++++ authbridge/cmd/abctl/tui/sessions_pane.go | 56 +++++++- .../cmd/abctl/tui/sessions_title_test.go | 135 ++++++++++++++++++ 4 files changed, 267 insertions(+), 5 deletions(-) diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index b18e545a9..d8406e05b 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -626,9 +626,15 @@ func titleFromTranscript(path string) (string, error) { // too, while a relative cwd would not. An earlier version of this comment claimed the renderer // truncates "a path" from the left, which overstated what it can know. // - // So the reason to leave it long is narrower: a path is bounded by the filesystem, where a - // prompt is unbounded free text, and the cap exists for the latter. The cell stays bounded - // either way, because the renderer truncates whatever it is handed. + // So the reason to leave it long is narrower: a REAL path is bounded by the filesystem, where a + // prompt is unbounded free text, and the cap exists for the latter. + // + // "BOUNDED BY THE FILESYSTEM" WAS THE FLAW. e.Cwd is a JSON string field, not a stat() result — + // nothing validates its length, and a transcript is a file anything can write. So this tier was + // the one unbounded string in the harvester, and it reached the renderer's per-rune width search, + // where a 20,000-rune value cost seconds per redraw. Capped at MaxCwdLen now: still far longer + // than MaxTitleLen so a genuine deep path keeps its whole leaf, which is what this tier is for, + // but no longer unbounded. The truncation keeps the TAIL, matching how the renderer treats a path. // // It is still normalised — whitespace collapsed, markup and non-graphic runes dropped — so a // cwd cannot carry hidden characters into a cell any more than a prompt can. @@ -637,7 +643,29 @@ func titleFromTranscript(path string) (string, error) { // whether it is empty, and this call is the guarantee for the RETURN. normalizeTitle is idempotent // (asserted, since its fixed-point loop is what makes that true), so the second pass costs one // walk and means this line does not depend on every assignment upstream having been normalised. - return normalizeTitle(cwd), err + return clipCwd(normalizeTitle(cwd)), err +} + +// MaxCwdLen caps the cwd fallback, in RUNES. +// +// Deliberately much larger than MaxTitleLen: this tier exists to show a directory, whose leaf is the +// identifying part, so clipping it to a title's budget would defeat it. It is not a display budget — +// the renderer truncates to the column anyway — it is a BOUND, so that one field in a transcript +// cannot hand the renderer an arbitrarily long string. Linux caps a path at 4096 bytes; this is +// comfortably above any real one and still finite. +const MaxCwdLen = 1024 + +// clipCwd caps s at MaxCwdLen runes, keeping the TAIL. +// +// The tail, not the head, because a path's distinguishing end is its leaf and that is the end the +// renderer keeps too. A plain rune slice is safe for the same reason it is in clipTitle: normalizeTitle +// has already removed every rune that binds to its neighbour. +func clipCwd(s string) string { + r := []rune(s) + if len(r) <= MaxCwdLen { + return s + } + return strings.TrimSpace(string(r[len(r)-MaxCwdLen:])) } // MaxTitleLen caps a harvested title, in RUNES. diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 0be464f4c..92c911758 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -2664,3 +2664,48 @@ func TestTitleFromTranscript_AnEmptyCwdDoesNotClobberAGoodOne(t *testing.T) { t.Errorf("title = %q, want %q — last-wins is the rule for a non-empty cwd", got, "/w/second") } } + +// TestTitleFromTranscript_CwdIsBounded pins the one string the harvester used to leave unbounded. +// +// Five prompt tiers go through clipTitle; the cwd tier returned normalizeTitle(cwd) uncapped, on the +// reasoning that "a path is bounded by the filesystem". It is not: e.Cwd is a JSON string field, and a +// transcript is a file anything can write. The value then reached the renderer's per-rune width search, +// which measured the whole remaining string once per dropped rune — seconds per redraw on a 20,000-rune +// cwd. +// +// The cap is deliberately far above MaxTitleLen, because this tier exists to show a directory and +// clipping it to a title's budget would defeat it. It is a BOUND, not a display budget. +func TestTitleFromTranscript_CwdIsBounded(t *testing.T) { + for _, runes := range []int{10, 100, MaxCwdLen, MaxCwdLen + 1, 5000, 50000} { + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/`+strings.Repeat("a", runes-1)+`"}`) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if n := len([]rune(got)); n > MaxCwdLen { + t.Errorf("a %d-rune cwd produced a %d-rune title, over the %d cap — an unbounded title "+ + "reaches the renderer's per-rune width search", runes, n, MaxCwdLen) + } + // A cwd at or under the cap is untouched: the bound must not clip a real path. + if runes <= MaxCwdLen && len([]rune(got)) != runes { + t.Errorf("a %d-rune cwd was clipped to %d runes, but the cap is %d — a genuine deep path "+ + "must keep its whole leaf", runes, len([]rune(got)), MaxCwdLen) + } + } + + // THE TAIL SURVIVES, not the head: a path's leaf is the identifying part, and it is the end the + // renderer keeps too. + dir := t.TempDir() + writeSessionTranscript(t, dir, "s.jsonl", + `{"type":"user","cwd":"/`+strings.Repeat("a", 5000)+`/the-leaf"}`) + got, err := titleFromTranscript(filepath.Join(dir, "s.jsonl")) + if err != nil { + t.Fatal(err) + } + if !strings.HasSuffix(got, "/the-leaf") { + t.Errorf("title = %q, want it to end in %q — clipping a cwd must keep the leaf", + got[max(0, len(got)-20):], "/the-leaf") + } +} diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index a196f754c..b9e3bdfba 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -483,6 +483,27 @@ func looksLikePath(title string) bool { return strings.Contains(rest, "/") } +// zeroWidthFree reports whether every rune in s occupies at least one display column. +// +// What licenses the prefix skip in truncLeft and truncRight: with no zero-width rune, a run of more +// than n runes cannot fit n columns, so n runes from the relevant end is an exact lower bound and +// every index beyond it is provably too wide to measure. One zero-width rune breaks that, and a +// differential test against the pre-skip implementation caught exactly that case. +// +// Checked structurally rather than by measuring: the classes runewidth gives zero columns are +// non-spacing and enclosing marks, format characters and controls, plus the modifier symbols this +// project already drops upstream. Cheap — one pass, no allocation — and the answer is yes for every +// title, so the skip is not hypothetical. +func zeroWidthFree(s string) bool { + for _, r := range s { + if unicode.Is(unicode.Mn, r) || unicode.Is(unicode.Me, r) || + unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Cc, r) || unicode.Is(unicode.Sk, r) { + return false + } + } + return true +} + // truncRight clips s to n DISPLAY COLUMNS keeping the LEFT end, marking the cut with a // trailing ellipsis. // @@ -513,7 +534,16 @@ func truncRight(s string, n int) string { // // The ELLIPSIS PLUS THE HEAD is measured, not the head plus one, so a trailing combining // mark that fuses onto the ellipsis cannot push the result over budget. + // + // AND IT SKIPS AHEAD, for truncLeft's reason, under the same guard: a head longer than n runes + // cannot fit n columns PROVIDED no rune is zero-width, so the first n runes are then an exact + // 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. r := []rune(s) + if len(r) > n && zeroWidthFree(s) { + r = r[:n] + } for i := len(r); i > 0; i-- { if out := string(r[:i]) + "…"; lipgloss.Width(out) <= n { return out @@ -569,8 +599,32 @@ func truncLeft(s string, n int) string { // Runes are dropped from the front until the remainder fits the budget less the ellipsis. // One at a time rather than by arithmetic: a rune's width is 1 or 2, so there is no index // that can be computed from the total. + // + // BUT THE SEARCH SKIPS AHEAD FIRST, because measuring from i == 0 made this quadratic: each + // iteration rebuilt the whole remaining tail and handed it to lipgloss.Width, so the cost grew + // with the square of the input. Measured on one call, dropping runes one at a time from the + // front: 2500 runes 36ms, 5000 142ms, 10000 572ms, 20000 2.33s — four times the input for + // sixteen times the work. The same shape stripHarnessSpans was flagged for; these two were left. + // + // THE SKIP IS GUARDED, because the obvious bound is not universally true. "Every rune is at + // least one column, so the last n runes are an exact lower bound" holds only while no rune is + // ZERO columns — and combining marks, joiners and variation selectors all are. A differential + // test against the old implementation caught it at n == 1: on a string of combining marks the + // bound skipped past marks the one-at-a-time search would have kept, and the two returned + // 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 — authlib/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. r := []rune(s) - for i := range r { + start := 0 + if len(r) > n && zeroWidthFree(s) { + start = len(r) - n + } + for i := start; i < len(r); i++ { // The ELLIPSIS PLUS THE TAIL is measured, not the tail plus one. A tail that begins // with a combining mark or a variation selector fuses onto the ellipsis, so the pair // is narrower than the sum of its parts — and assuming the ellipsis always adds diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index b3abd195d..31e92bc12 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -2,6 +2,7 @@ package tui import ( "encoding/json" + "math/rand" "os" "path/filepath" "strings" @@ -1182,3 +1183,137 @@ func TestLooksLikePath_AnyUnicodeSpaceSeparates(t *testing.T) { } } } + +// TestTrunc_ScalesLinearly pins the cost of both truncators against the INPUT length. +// +// Both measured the whole remaining string with lipgloss.Width once per dropped rune, so the cost grew +// with the square of the input: on one call, 2500 runes took 36ms, 5000 142ms, 10000 572ms and 20000 +// 2.33s — four times the input for sixteen times the work. Reachable because the cwd tier was +// uncapped, and the renderer redraws on every poll. +// +// Asserted as a RATIO rather than a wall-clock bound, so it says what it means on a loaded CI machine: +// quadratic growth shows up as ~4x per doubling, linear as ~2x, and the ceiling sits between them. +// Both ends are timed inside one test so the comparison is against the same machine at the same moment. +func TestTrunc_ScalesLinearly(t *testing.T) { + const budget = 40 + measure := func(f func(string, int) string, runes int) time.Duration { + s := "/" + strings.Repeat("a", runes-1) + // Warm, so the first-call cost of anything lazy is not charged to the small input. + f(s, budget) + start := time.Now() + for i := 0; i < 20; i++ { + f(s, budget) + } + return time.Since(start) + } + for _, tc := range []struct { + name string + f func(string, int) string + }{ + {"truncLeft", truncLeft}, + {"truncRight", truncRight}, + } { + t.Run(tc.name, func(t *testing.T) { + small := measure(tc.f, 4000) + large := measure(tc.f, 16000) + if small <= 0 { + t.Skip("timer resolution too coarse to compare") + } + // 4x the input. Linear predicts ~4x the time; quadratic predicts ~16x. A ceiling of 8x + // separates them with room for scheduling noise. + if ratio := float64(large) / float64(small); ratio > 8 { + t.Errorf("4x the input took %.1fx the time (%v -> %v) — that is the quadratic shape "+ + "back: the per-rune search must skip to the last/first n runes before measuring", + ratio, small, large) + } + }) + } +} + +// TestTrunc_SkipAheadMatchesTheOneAtATimeSearch is the differential test that caught the first version +// of the skip being wrong, kept so it cannot regress. +// +// The skip rests on "every rune is at least one column, so n runes from the end is an exact lower +// bound". That premise is FALSE for zero-width runes — combining marks, joiners, variation selectors — +// and an unguarded skip returned different bytes than the one-at-a-time search at n == 1 on a string of +// combining marks. The guard is zeroWidthFree; this asserts the equivalence it is supposed to buy, +// across alphabets chosen so both sides of the guard are exercised. +func TestTrunc_SkipAheadMatchesTheOneAtATimeSearch(t *testing.T) { + // The pre-skip implementations, as an oracle. + oldLeft := func(s string, n int) string { + if lipgloss.Width(s) <= n { + return s + } + if n < 1 { + return "" + } + r := []rune(s) + for i := range r { + if out := "…" + string(r[i:]); lipgloss.Width(out) <= n { + return out + } + } + return "…" + } + oldRight := func(s string, n int) string { + if lipgloss.Width(s) <= n { + return s + } + if n < 1 { + return "" + } + r := []rune(s) + for i := len(r); i > 0; i-- { + if out := string(r[:i]) + "…"; lipgloss.Width(out) <= n { + return out + } + } + return "…" + } + + alphabets := []string{ + "abcdefghijklmnopqrstuvwxyz /._-", // the ordinary case, and the one that must be fast + "日本語のセッションタイトル漢字", // two columns per rune + "🎉🚀✨🔥", // wide emoji + "aあ🎉/b日x", // mixed widths + "éà", // COMBINING MARKS: zero width, the case that broke it + "️‍", // variation selector, ZWJ: also zero width + } + rng := rand.New(rand.NewSource(20260923)) + checked := 0 + for _, alpha := range alphabets { + ar := []rune(alpha) + for trial := 0; trial < 120; trial++ { + var sb strings.Builder + for i := 0; i < rng.Intn(60); i++ { + sb.WriteRune(ar[rng.Intn(len(ar))]) + } + s := sb.String() + for n := -2; n <= 45; n++ { + if want, got := oldLeft(s, n), truncLeft(s, n); want != got { + t.Fatalf("truncLeft(%q, %d) = %q, one-at-a-time search gives %q", s, n, got, want) + } + if want, got := oldRight(s, n), truncRight(s, n); want != got { + t.Fatalf("truncRight(%q, %d) = %q, one-at-a-time search gives %q", s, n, got, want) + } + checked += 2 + } + } + } + t.Logf("%d comparisons against the one-at-a-time search, all byte-identical", checked) +} + +// TestZeroWidthFree pins the guard's own answer, including that an ordinary title takes the fast path. +func TestZeroWidthFree(t *testing.T) { + for _, s := range []string{"", "plain prose", "/Users/x/src", "日本語", "🎉", "a b-c_d.e"} { + if !zeroWidthFree(s) { + t.Errorf("zeroWidthFree(%q) = false, want true — an ordinary title must take the fast path", s) + } + } + for _, s := range []string{"é", "a‍", "x️", "a\u0000b", "́"} { + if zeroWidthFree(s) { + t.Errorf("zeroWidthFree(%q) = true, want false — %U occupies no column, so the prefix "+ + "bound does not hold", s, []rune(s)[len([]rune(s))-1]) + } + } +}