From 78b996ac75cb4a3ad1cf4d938e384b1d9a247c35 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Thu, 6 Aug 2026 23:47:32 -0400 Subject: [PATCH 01/11] fix: stash bare URLs before glamour render so wrap can't split them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Glamour hard-splits a bare-URL line mid-word once the URL is longer than the wrap width — the rendered Link/autolink ANSI run interacts with reflow's word-wrap differently from plain text and forces a break instead of pushing the whole word to the next line. In a normal pane width, that's most URLs, every time. renderMarkdown now stashes any line that's nothing but a bare URL (the board convention) behind a short placeholder before handing raw markdown to glamour, then restores the real URL afterward, styled to match glamour's own Link style. The line is free to overflow the pane width now — intentional, and the one exception TestNeverWiderThanWidth allows — but the URL itself is never split, so terminal link detection still picks up the whole thing. TestBareURLIntact now renders at width 40 instead of 78, which is what let this ship unnoticed — none of the fixture URLs exceeded 78 chars, so the split path never fired. Fixes #15. Co-Authored-By: Claude Sonnet 5 --- render_test.go | 13 ++++++++--- style.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/render_test.go b/render_test.go index 794ca68..83c5cd8 100644 --- a/render_test.go +++ b/render_test.go @@ -38,11 +38,16 @@ func TestCompactSpacing(t *testing.T) { } // NEVER render wider than the requested width — padded/overwide lines wrap -// in the pane and fake double-spacing. +// in the pane and fake double-spacing. Bare-URL-only lines are the one +// deliberate exception: they're kept intact on one line even past the +// width rather than split mid-URL (see TestBareURLIntact and issue #15). func TestNeverWiderThanWidth(t *testing.T) { for _, width := range []int{40, 60, 78} { out := renderFixture(t, width) for i, line := range strings.Split(out, "\n") { + if strings.Contains(line, "https://") || strings.Contains(line, "http://") { + continue + } if w := visibleWidth(line); w > width { t.Errorf("width %d, line %d: visible width %d: %q", width, i, w, stripANSI(line)) @@ -62,9 +67,11 @@ func TestNoTrailingSpacePadding(t *testing.T) { } // Bare URLs must survive intact on a single line so Ghostty's link -// detection can make them clickable. +// detection can make them clickable — even at pane widths narrower than +// the URL itself, which is the common case and is what used to hard-split +// mid-URL (issue #15). func TestBareURLIntact(t *testing.T) { - out := stripANSI(renderFixture(t, 78)) + out := stripANSI(renderFixture(t, 40)) for _, url := range []string{ "https://github.com/example/app/pull/412", "https://qa.example.dev/checkout-race", diff --git a/style.go b/style.go index e5db43d..ec08bba 100644 --- a/style.go +++ b/style.go @@ -1,6 +1,8 @@ package main import ( + "fmt" + "regexp" "strings" "github.com/charmbracelet/glamour" @@ -117,6 +119,61 @@ func styleConfig() ansi.StyleConfig { } } +// bareURLLine matches a markdown line that is nothing but a bare URL — the +// board convention for links (see CLAUDE.md: "bare URLs, each on its own +// line") — whether it's an indented continuation line under a bullet or a +// top-level list item in its own right. +var bareURLLine = regexp.MustCompile(`(?m)^([ \t]*(?:[-*+]\s+)?)(https?://\S+)([ \t]*)$`) + +// urlPlaceholder is a short, markdown-inert stand-in for a stashed URL. It +// uses the ASCII unit separator as a delimiter so it can never collide with +// real board text, and stays well under any realistic wrap width. +func urlPlaceholder(i int) string { + return fmt.Sprintf("\x1fsidecarurl%d\x1f", i) +} + +// stashBareURLs replaces every bare-URL-only line with a short placeholder, +// returning the stashed URLs in order. Glamour's word-wrap hard-splits a +// long link token mid-URL once it exceeds the wrap width (it treats the +// autolink's rendered ANSI run differently from plain text and force-breaks +// it instead of pushing the whole word to the next line) — the placeholder +// keeps such lines out of that path entirely. restoreBareURLs puts the real, +// styled URL back after rendering. +func stashBareURLs(raw string) (string, []string) { + var urls []string + out := bareURLLine.ReplaceAllStringFunc(raw, func(line string) string { + m := bareURLLine.FindStringSubmatch(line) + urls = append(urls, m[2]) + return m[1] + urlPlaceholder(len(urls)-1) + }) + return out, urls +} + +// restoreBareURLs swaps each placeholder back for its real URL, styled the +// same as glamour would style a Link (colorLink, underlined) — so a +// bare-URL-only line always survives on one physical line, clickable, no +// matter how far it overflows the pane width. Everything after the +// placeholder is dropped rather than kept: it's block-margin padding sized +// for the short placeholder, not the real URL, and would just trail stale +// spaces past the restored line. +func restoreBareURLs(rendered string, urls []string) string { + if len(urls) == 0 { + return rendered + } + lines := strings.Split(rendered, "\n") + for i, url := range urls { + styled := termenv.String(url).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() + ph := urlPlaceholder(i) + for li, line := range lines { + if idx := strings.Index(line, ph); idx >= 0 { + lines[li] = line[:idx] + styled + break + } + } + } + return strings.Join(lines, "\n") +} + // renderMarkdown renders raw markdown at the given width (already reduced // from the pane width by the caller). Output is post-processed to guarantee // the hard requirements: no trailing-space padding, at most one blank line @@ -125,6 +182,7 @@ func renderMarkdown(raw string, width int) (string, error) { if width < 10 { width = 10 } + raw, urls := stashBareURLs(raw) r, err := glamour.NewTermRenderer( glamour.WithStyles(styleConfig()), glamour.WithWordWrap(width), @@ -139,6 +197,7 @@ func renderMarkdown(raw string, width int) (string, error) { if err != nil { return "", err } + out = restoreBareURLs(out, urls) return tidy(out), nil } From 504e115aa825f8ac9ddb2e7f691c816d22d0c244 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 00:02:45 -0400 Subject: [PATCH 02/11] fix: OSC 8 hyperlink bare URLs instead of letting them overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit on this branch (stash/restore to keep glamour from force-splitting a bare URL mid-word) fixed the split but not the real problem: bubbles viewport applies lipgloss's MaxWidth to every frame, which silently truncated any over-width line — losing the tail of the URL outright, worse than the split it replaced. Checked the actual gate before building further: bubbletea's flush() and lipgloss's MaxWidth both truncate via charmbracelet/x/ansi, which parses OSC sequences and always preserves escape bytes regardless of where the visible-cell cutoff falls (confirmed by reading truncate.go). So OSC 8 hyperlinks survive the real render pipeline intact — the one question PR #18's last review round flagged as unverified and could not check from that environment. This reimplements #15 as PR #18 was heading, using its four rounds of review as the spec: - Each bare URL keeps its own index-keyed placeholder end to end, so two URLs that elide to identical visible text can never search-and cross-link (PR #18's confirmed collision bug) — there's nothing to search for. - Fenced code blocks are skipped during stashing (line-by-line fence tracking), so a URL a user typed verbatim in a code block is never truncated or hyperlinked. - The elision budget is measured from the actual rendered line prefix at restore time, not guessed — fixes PR #18's nested-bullet-indent bug structurally rather than special-casing it. - The OSC 8 target is percent-encoded for control/non-ASCII bytes instead of rejected, so a pathological or non-ASCII URL still gets a complete, safe hyperlink instead of silently losing it. - visibleWidth switched from muesli/reflow (CSI-only, no OSC 8 awareness) to charmbracelet/x/ansi.StringWidth, so width/padding math and the 'never wider than the pane' invariant hold for hyperlinked lines too — no exemption needed. - changedLines needed no change: its existing SGR-only stripANSI never touched OSC 8 to begin with, so the diff key already retains the link target — a target-only edit still flashes/marks correctly. New tests in render_test.go cover each of the above, plus the one gate PR #18 could never check: TestHyperlinkSurvivesDownstreamTruncation pins x/ansi.Truncate against a real hyperlinked line. Still needs a manual Ghostty eyeball at ~40 columns to confirm the link is actually clickable — that's the one check no unit test here can make and the one both this fix and PR #18 skipped before. Refs #15, informed by #18. Co-Authored-By: Claude Sonnet 5 --- go.mod | 6 +- render_test.go | 156 +++++++++++++++++++++++++++++++++++++++++++----- style.go | 159 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 272 insertions(+), 49 deletions(-) diff --git a/go.mod b/go.mod index 0147d99..1a2a01d 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,9 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/glamour v1.0.0 github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 + github.com/charmbracelet/x/ansi v0.11.6 github.com/fsnotify/fsnotify v1.10.1 - github.com/muesli/reflow v0.3.0 + github.com/mattn/go-runewidth v0.0.19 github.com/muesli/termenv v0.16.0 golang.org/x/term v0.43.0 ) @@ -19,7 +20,6 @@ require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/aymerick/douceur v0.2.0 // indirect github.com/charmbracelet/colorprofile v0.4.1 // indirect - github.com/charmbracelet/x/ansi v0.11.6 // indirect github.com/charmbracelet/x/cellbuf v0.0.15 // indirect github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf // indirect github.com/charmbracelet/x/term v0.2.2 // indirect @@ -32,10 +32,10 @@ require ( github.com/lucasb-eyer/go-colorful v1.3.0 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-localereader v0.0.1 // indirect - github.com/mattn/go-runewidth v0.0.19 // indirect github.com/microcosm-cc/bluemonday v1.0.27 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/cancelreader v0.2.2 // indirect + github.com/muesli/reflow v0.3.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yuin/goldmark v1.7.13 // indirect diff --git a/render_test.go b/render_test.go index 83c5cd8..d075f4c 100644 --- a/render_test.go +++ b/render_test.go @@ -5,6 +5,8 @@ import ( "regexp" "strings" "testing" + + xansi "github.com/charmbracelet/x/ansi" ) func renderFixture(t *testing.T, width int) string { @@ -38,16 +40,13 @@ func TestCompactSpacing(t *testing.T) { } // NEVER render wider than the requested width — padded/overwide lines wrap -// in the pane and fake double-spacing. Bare-URL-only lines are the one -// deliberate exception: they're kept intact on one line even past the -// width rather than split mid-URL (see TestBareURLIntact and issue #15). +// in the pane and fake double-spacing. This holds even for bare-URL lines +// wider than the width: their OSC 8 target carries the full URL, but the +// visible display text is elided to fit (see TestBareURLIntact, issue #15). func TestNeverWiderThanWidth(t *testing.T) { for _, width := range []int{40, 60, 78} { out := renderFixture(t, width) for i, line := range strings.Split(out, "\n") { - if strings.Contains(line, "https://") || strings.Contains(line, "http://") { - continue - } if w := visibleWidth(line); w > width { t.Errorf("width %d, line %d: visible width %d: %q", width, i, w, stripANSI(line)) @@ -66,31 +65,158 @@ func TestNoTrailingSpacePadding(t *testing.T) { } } -// Bare URLs must survive intact on a single line so Ghostty's link -// detection can make them clickable — even at pane widths narrower than -// the URL itself, which is the common case and is what used to hard-split -// mid-URL (issue #15). +// oscLinkRE finds an OSC 8 hyperlink's target and display text. The display +// text carries its own SGR styling, so it isn't ANSI-escape-free — match +// non-greedily up to the closing OSC 8 rather than excluding ESC outright. +var oscLinkRE = regexp.MustCompile(`(?s)\x1b]8;;([^\x07\n]*)\x07(.*?)\x1b]8;;\x07`) + +// Bare URLs must be reachable via a single, complete OSC 8 hyperlink on one +// line — even at pane widths narrower than the URL itself, which is the +// common case and is what used to hard-split mid-URL (issue #15), and later +// what a naive fix let the viewport silently truncate instead. func TestBareURLIntact(t *testing.T) { - out := stripANSI(renderFixture(t, 40)) + out := renderFixture(t, 40) for _, url := range []string{ "https://github.com/example/app/pull/412", "https://qa.example.dev/checkout-race", } { found := false for _, line := range strings.Split(out, "\n") { - if n := strings.Count(line, url); n > 0 { + for _, m := range oscLinkRE.FindAllStringSubmatch(line, -1) { + if m[1] != url { + continue + } found = true - if strings.Count(line, "http") > 1 { - t.Errorf("URL duplicated on line: %q", line) + if n := strings.Count(line, "\x1b]8;;"+url+"\x07"); n > 1 { + t.Errorf("target %s duplicated on line: %q", url, line) } } } if !found { - t.Errorf("URL %s not intact on a single line:\n%s", url, out) + t.Errorf("URL %s has no complete OSC 8 hyperlink on one line:\n%s", url, stripANSI(out)) } } } +// Two bare URLs that elide to identical visible text must still each get +// their own correct hyperlink target — no cross-linking (PR #18's confirmed +// collision bug: a global text search re-found the first occurrence). +func TestBareURLCollisionSafe(t *testing.T) { + raw := "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n" + + "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 2 { + t.Fatalf("want 2 hyperlinks, got %d:\n%s", len(links), stripANSI(out)) + } + if links[0][1] != "https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1" { + t.Errorf("first link target wrong: %q", links[0][1]) + } + if links[1][1] != "https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2" { + t.Errorf("second link target wrong: %q", links[1][1]) + } +} + +// A bare URL inside a fenced code block is content the user typed verbatim +// — stashBareURLs must leave it alone entirely (no OSC 8 hyperlink, no +// leaked placeholder). Whatever glamour itself does with fenced content +// (it word-wraps long hyphenated lines same as any other text — pre-existing, +// unrelated to bare-URL handling) is out of scope here. +func TestBareURLInFenceUntouched(t *testing.T) { + raw := "```\nhttps://example.test/verbatim-in-a-fence-that-is-long-enough-to-elide\n```\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("URL inside a fence got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into fenced output:\n%s", stripANSI(out)) + } +} + +// A control byte or non-ASCII byte in a URL must not break out of the OSC 8 +// escape (injection) or get silently dropped (lossy percent-encoding). +func TestBareURLUnsafeBytesEncoded(t *testing.T) { + raw := "- https://example.test/caf\u00e9-and-a-bell-\x07-in-the-middle\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatalf("no hyperlink found:\n%s", stripANSI(out)) + } + if strings.ContainsAny(m[1], "\x07\x1b") { + t.Fatalf("target still contains a raw control byte: %q", m[1]) + } + if !strings.Contains(m[1], "%C3%A9") || !strings.Contains(m[1], "%07") { + t.Errorf("target wasn't percent-encoded correctly: %q", m[1]) + } +} + +// A nested list item's budget must account for its actual indent, not a +// guessed constant (PR #18's confirmed bug: a hard-coded top-level-bullet +// reserve overshot on nested items and re-broke the line it was meant to +// fix). +func TestBareURLNestedIndentBudget(t *testing.T) { + raw := "- Parent\n - https://example.test/nested-item-url-thats-long-enough-to-need-eliding\n" + out, err := renderMarkdown(raw, 30) + if err != nil { + t.Fatal(err) + } + for i, line := range strings.Split(out, "\n") { + if w := visibleWidth(line); w > 30 { + t.Errorf("line %d: visible width %d > 30: %q", i, w, stripANSI(line)) + } + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/nested-item-url-thats-long-enough-to-need-eliding" { + t.Errorf("nested URL lost its hyperlink target: %v", m) + } +} + +// changedLines must still detect a change when only a URL's target differs +// but its elided display text happens to be identical — the diff key has to +// retain the OSC 8 target, not just the visible text. +func TestChangedLinesSeesURLTargetChange(t *testing.T) { + before, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n", 40) + if err != nil { + t.Fatal(err) + } + after, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n", 40) + if err != nil { + t.Fatal(err) + } + changed := changedLines(strings.Split(before, "\n"), strings.Split(after, "\n")) + if len(changed) == 0 { + t.Errorf("target-only URL change went undetected") + } +} + +// The gate PR #18 could never verify: bubbletea's renderer and lipgloss's +// MaxWidth both truncate the final frame to the pane width using +// charmbracelet/x/ansi, which understands OSC 8 and only cuts visible +// cells. If that ever regresses to an OSC-blind truncator, a hyperlinked +// line gets cut inside the escape and the display text (which comes after +// it) is dropped outright — this pins the assumption directly. +func TestHyperlinkSurvivesDownstreamTruncation(t *testing.T) { + target := "https://example.test/downstream-truncation-gate" + line := "prefix " + hyperlink(target, "short") + got := xansi.Truncate(line, 20, "") + m := oscLinkRE.FindStringSubmatch(got) + if m == nil || m[1] != target { + t.Fatalf("x/ansi.Truncate dropped or corrupted the hyperlink: %q", got) + } + if !strings.Contains(stripANSI(got), "short") { + t.Errorf("x/ansi.Truncate dropped the display text: %q", got) + } +} + // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40) diff --git a/style.go b/style.go index ec08bba..164dbab 100644 --- a/style.go +++ b/style.go @@ -7,7 +7,8 @@ import ( "github.com/charmbracelet/glamour" "github.com/charmbracelet/glamour/ansi" - reflowansi "github.com/muesli/reflow/ansi" + xansi "github.com/charmbracelet/x/ansi" + "github.com/mattn/go-runewidth" "github.com/muesli/termenv" ) @@ -123,52 +124,144 @@ func styleConfig() ansi.StyleConfig { // board convention for links (see CLAUDE.md: "bare URLs, each on its own // line") — whether it's an indented continuation line under a bullet or a // top-level list item in its own right. -var bareURLLine = regexp.MustCompile(`(?m)^([ \t]*(?:[-*+]\s+)?)(https?://\S+)([ \t]*)$`) +var bareURLLine = regexp.MustCompile(`^([ \t]*(?:[-*+]\s+)?)(https?://\S+)([ \t]*)$`) + +// fenceLine matches a fenced-code-block delimiter (``` or ~~~, 3+ of the +// same character). Bare URLs inside a fence are content the user typed +// verbatim and are never word-wrapped by glamour in the first place, so +// stashBareURLs leaves them alone. +var fenceLine = regexp.MustCompile("^[ \t]*(```+|~~~+)") // urlPlaceholder is a short, markdown-inert stand-in for a stashed URL. It // uses the ASCII unit separator as a delimiter so it can never collide with -// real board text, and stays well under any realistic wrap width. +// real board text, and stays well under any realistic wrap width — short +// enough that glamour's word-wrap never has a reason to touch it. func urlPlaceholder(i int) string { - return fmt.Sprintf("\x1fsidecarurl%d\x1f", i) + return fmt.Sprintf("\x1fU%d\x1f", i) } // stashBareURLs replaces every bare-URL-only line with a short placeholder, -// returning the stashed URLs in order. Glamour's word-wrap hard-splits a -// long link token mid-URL once it exceeds the wrap width (it treats the -// autolink's rendered ANSI run differently from plain text and force-breaks -// it instead of pushing the whole word to the next line) — the placeholder -// keeps such lines out of that path entirely. restoreBareURLs puts the real, -// styled URL back after rendering. +// returning the stashed URLs in order (skipping lines inside fenced code +// blocks). Glamour's word-wrap hard-splits a long link token mid-URL once it +// exceeds the wrap width — it treats the autolink's rendered ANSI run +// differently from plain text and force-breaks it instead of pushing the +// whole word to the next line — so the placeholder keeps such lines out of +// that path entirely. restoreBareURLs puts the real, styled, hyperlinked URL +// back after rendering. func stashBareURLs(raw string) (string, []string) { + lines := strings.Split(raw, "\n") var urls []string - out := bareURLLine.ReplaceAllStringFunc(raw, func(line string) string { - m := bareURLLine.FindStringSubmatch(line) - urls = append(urls, m[2]) - return m[1] + urlPlaceholder(len(urls)-1) - }) - return out, urls + var fenceChar byte + for i, line := range lines { + if m := fenceLine.FindStringSubmatch(line); m != nil { + c := m[1][0] + switch fenceChar { + case 0: + fenceChar = c + case c: + fenceChar = 0 + } + continue + } + if fenceChar != 0 { + continue + } + if m := bareURLLine.FindStringSubmatch(line); m != nil { + urls = append(urls, m[2]) + lines[i] = m[1] + urlPlaceholder(len(urls)-1) + } + } + return strings.Join(lines, "\n"), urls +} + +// oscOpen/oscClose delimit an OSC 8 hyperlink. BEL-terminated rather than +// ST (ESC \\): this codebase's other ANSI helpers only recognize a bare +// ESC[0m-style reset, and BEL is a single unambiguous byte to bound a +// terminator scan on. +const oscOpen = "\x1b]8;;" +const oscBEL = "\x07" +const oscClose = oscOpen + oscBEL + +// oscTarget encodes url for safe use as an OSC 8 target: bytes that could +// terminate the escape early (control bytes, DEL) or aren't valid in a URI +// (anything non-ASCII) are percent-encoded rather than rejected, so a +// pathological or non-ASCII URL still gets a working, complete link instead +// of silently losing its hyperlink. +func oscTarget(url string) string { + var b strings.Builder + for i := 0; i < len(url); i++ { + c := url[i] + if c < 0x20 || c == 0x7f || c >= 0x80 { + fmt.Fprintf(&b, "%%%02X", c) + continue + } + b.WriteByte(c) + } + return b.String() +} + +// hyperlink wraps display in an OSC 8 hyperlink pointing at target. display +// carries its own SGR styling; the OSC 8 escapes only add the link. +func hyperlink(target, display string) string { + return oscOpen + oscTarget(target) + oscBEL + display + oscClose +} + +// elideURL returns url unchanged if it fits within budget cells, otherwise +// cuts it (cell-width aware, not byte- or rune-count aware) to make room for +// a trailing ellipsis whose own width is measured rather than assumed to be +// one cell. budget <= 0 is a pathological deeply-nested-bullet-in-a-tiny-pane +// case; it still returns a single ellipsis rather than nothing, since an +// empty display text would be an invisible — but still clickable — link. +func elideURL(url string, budget int) string { + if runewidth.StringWidth(url) <= budget { + return url + } + ellipsisWidth := runewidth.RuneWidth('…') + keep := budget - ellipsisWidth + if keep <= 0 { + return "…" + } + var b strings.Builder + w := 0 + for _, r := range url { + rw := runewidth.RuneWidth(r) + if w+rw > keep { + break + } + b.WriteRune(r) + w += rw + } + b.WriteRune('…') + return b.String() } -// restoreBareURLs swaps each placeholder back for its real URL, styled the -// same as glamour would style a Link (colorLink, underlined) — so a -// bare-URL-only line always survives on one physical line, clickable, no -// matter how far it overflows the pane width. Everything after the -// placeholder is dropped rather than kept: it's block-margin padding sized -// for the short placeholder, not the real URL, and would just trail stale -// spaces past the restored line. -func restoreBareURLs(rendered string, urls []string) string { +// restoreBareURLs swaps each placeholder back for its real URL, wrapped in +// an OSC 8 hyperlink and styled like glamour's own Link (colorLink, +// underlined). The visible text is elided to fit whatever width remains on +// its line — measured from the actual rendered prefix (bullet, indent, +// nesting), not guessed — but the hyperlink target always carries the full, +// untruncated URL, so the link opens the right place regardless of how much +// of it is shown. Each URL has its own index-keyed placeholder, so two URLs +// that happen to elide to identical visible text can never cross-link — +// there's nothing to search for and collide on. +func restoreBareURLs(rendered string, urls []string, width int) string { if len(urls) == 0 { return rendered } lines := strings.Split(rendered, "\n") for i, url := range urls { - styled := termenv.String(url).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() ph := urlPlaceholder(i) for li, line := range lines { - if idx := strings.Index(line, ph); idx >= 0 { - lines[li] = line[:idx] + styled - break + idx := strings.Index(line, ph) + if idx < 0 { + continue } + prefix := line[:idx] + budget := width - visibleWidth(prefix) + display := elideURL(url, budget) + styled := termenv.String(display).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() + lines[li] = prefix + hyperlink(url, styled) + break } } return strings.Join(lines, "\n") @@ -197,7 +290,7 @@ func renderMarkdown(raw string, width int) (string, error) { if err != nil { return "", err } - out = restoreBareURLs(out, urls) + out = restoreBareURLs(out, urls, width) return tidy(out), nil } @@ -230,7 +323,11 @@ func tidy(s string) string { return strings.Join(out, "\n") } -// visibleWidth is the printable cell width of a line, ignoring ANSI codes. +// visibleWidth is the printable cell width of a line, ignoring ANSI codes — +// SGR and OSC 8 hyperlinks alike. x/ansi's parser understands OSC; the +// muesli/reflow width counter this used to call does not (it only +// recognizes CSI's `[0-9;]*[A-Za-z]` terminator), so an OSC 8-wrapped line +// would otherwise measure with the link target counted as visible text. func visibleWidth(line string) int { - return reflowansi.PrintableRuneWidth(line) + return xansi.StringWidth(line) } From 4cc99a10cd2f5fdf9b696f8da3a68ef3fb05129e Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 10:45:53 -0400 Subject: [PATCH 03/11] fix review: preserve trailing content, drop-not-overflow, CRLF, indented code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings on PR #21, one of them real: a. restoreBareURLs discarded everything after the placeholder on its line. Safe for the board's own convention (a URL as its own list item), not safe for a bare-URL line that's a soft-wrapped continuation inside a multi-line paragraph — real content after the URL was silently deleted, and the elision budget didn't reserve room for it either. Now keeps and measures both the prefix and the rest of the line. b. elideURL's budget<=0 fallback (a bare ellipsis) could still push a deeply-nested, tiny-pane line past the width — the one invariant every other line in this renderer holds. restoreBareURLs now drops the URL entirely rather than draw anything when there's no room. c. bareURLLine's trailing-whitespace class didn't include \r, so a CRLF board silently kept the pre-fix wrap-split bug on every bare URL. tidy() already treats CRLF as supported input; the regex now does too. d. A 4-space indented code block (CommonMark verbatim content, same as a fence) matched bareURLLine and got truncated/hyperlinked. Skipped now on the same signal as the indented-code heuristic allows: no list marker plus a 4+ space or tab indent. Trade-off noted in the comment — this can also false-skip a deeply-nested bullet continuation, which is the safer failure. Plus the two smaller notes: a residual-placeholder sweep in renderMarkdown so an unanticipated reflow edge case fails invisibly rather than leaving raw \x1f bytes on screen, and README/--static doc updates that describe what actually happens now (OSC 8 hyperlink, possibly-elided display, full target) instead of the pre-OSC8 claim. Five new tests, one per finding plus the doc-adjacent behavior. Full suite + go vet clean. Co-Authored-By: Claude Sonnet 5 --- README.md | 2 +- main.go | 6 +++- render_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ style.go | 78 +++++++++++++++++++++++++++++++++++--------------- 4 files changed, 136 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index c1e284e..c5be2f5 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ The glamour style is built into the binary (`style.go`) and tuned for a narrow p - Compact, with at most one blank line between blocks, no margins, and no trailing-space padding. - Hex (truecolor) colors throughout, never 256-palette indexes, which Ghostty remaps. -- Teal, underlined links. Bare URLs stay on their own line, so Ghostty can detect them and make them clickable. +- Teal, underlined links. Bare URLs (on their own line, per convention) render as real OSC 8 hyperlinks — clickable in any terminal that supports them, Ghostty included — with the visible text shortened to fit the pane when the URL itself is longer, though the link always opens the full, untruncated address. In a terminal without OSC 8 support, the shortened text is just plain text. - An H1 badge in black on lavender, H2 headings in muted amber with a `▍ ` prefix, and bold text in the default foreground. To adjust the colors, edit the `color…` constants at the top of `style.go` — links use `colorLink` — and rebuild. diff --git a/main.go b/main.go index a7059d1..bad28b6 100644 --- a/main.go +++ b/main.go @@ -97,7 +97,11 @@ func main() { // runStatic renders the file once to stdout and exits — no watching, no // alt-screen. Handy for piping, CI, and quick inline checks. Width is the -// terminal width (minus 2) when stdout is a TTY, else 80. +// terminal width (minus 2) when stdout is a TTY, else 80. A bare URL longer +// than that width is still elided in the visible text (the full URL lives +// only in its OSC 8 target) — fine for a terminal, but grep, an editor, or +// a CI log viewer reading the piped bytes plain won't see the untruncated +// URL. func runStatic(args []string) int { path := defaultBoardPath() if len(args) > 0 { diff --git a/render_test.go b/render_test.go index d075f4c..6e51934 100644 --- a/render_test.go +++ b/render_test.go @@ -217,6 +217,81 @@ func TestHyperlinkSurvivesDownstreamTruncation(t *testing.T) { } } +// A bare-URL line that's a soft-wrapped continuation inside a multi-line +// paragraph can have real content after the URL on the same rendered line +// (e.g. "See docs at\nhttps://…\nfor more." reflows to one paragraph). +// That trailing content must survive, and the elision budget must leave it +// room — restoreBareURLs used to discard everything after the placeholder. +func TestBareURLKeepsTrailingContent(t *testing.T) { + raw := "See the docs at\nhttps://example.test/reasonably-long-path-name\nfor more information, seriously.\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + plain := stripANSI(out) + if !strings.Contains(plain, "for more") { + t.Errorf("trailing paragraph content after the URL was dropped:\n%s", plain) + } + for i, line := range strings.Split(out, "\n") { + if w := visibleWidth(line); w > 40 { + t.Errorf("line %d: visible width %d > 40: %q", i, w, stripANSI(line)) + } + } +} + +// budget < 1 (indent alone fills the pane) must drop the URL rather than +// render a line wider than the pane — the one invariant every other line +// in this renderer holds. Exercised directly against restoreBareURLs: a +// markdown fixture that reliably produces a zero-budget nested indent +// through the real glamour pipeline is brittle to construct, and this is +// the exact boundary the fix claims to hold. +func TestBareURLNoRoomDropsRatherThanOverflows(t *testing.T) { + prefix := strings.Repeat("x", 10) // consumes the entire width on its own + rendered := prefix + urlPlaceholder(0) + out := restoreBareURLs(rendered, []string{"https://example.test/no-room-left"}, 10) + if w := visibleWidth(out); w > 10 { + t.Errorf("visible width %d > 10: %q", w, out) + } + if strings.Contains(out, "\x1f") { + t.Errorf("placeholder leaked into output: %q", out) + } + if strings.Contains(out, "http") { + t.Errorf("URL text should have been dropped, not shown partially: %q", out) + } +} + +// A CRLF board must still get the fix — bareURLLine's trailing-whitespace +// class has to include \r or a CRLF board silently keeps the pre-fix +// wrap-split bug. +func TestBareURLCRLFStillStashed(t *testing.T) { + raw := "- https://example.test/crlf-board-long-enough-to-need-eliding\r\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/crlf-board-long-enough-to-need-eliding" { + t.Errorf("CRLF board line wasn't hyperlinked: %v\n%s", m, stripANSI(out)) + } +} + +// A URL inside a 4-space indented code block is CommonMark verbatim +// content, same as a fenced block — it must not be truncated or +// hyperlinked. +func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { + raw := "Some paragraph.\n\n https://example.test/indented-code-block-verbatim-text\n" + out, err := renderMarkdown(raw, 40) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("URL inside an indented code block got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into indented-code-block output:\n%s", stripANSI(out)) + } +} + // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40) diff --git a/style.go b/style.go index 164dbab..f8b31e8 100644 --- a/style.go +++ b/style.go @@ -123,8 +123,10 @@ func styleConfig() ansi.StyleConfig { // bareURLLine matches a markdown line that is nothing but a bare URL — the // board convention for links (see CLAUDE.md: "bare URLs, each on its own // line") — whether it's an indented continuation line under a bullet or a -// top-level list item in its own right. -var bareURLLine = regexp.MustCompile(`^([ \t]*(?:[-*+]\s+)?)(https?://\S+)([ \t]*)$`) +// top-level list item in its own right. Groups: 1 = leading indent, 2 = +// optional list marker (with its trailing space), 3 = the URL, 4 = trailing +// whitespace, \r included so a CRLF board doesn't silently skip the fix. +var bareURLLine = regexp.MustCompile(`^([ \t]*)((?:[-*+]\s+)?)(https?://\S+)([ \t\r]*)$`) // fenceLine matches a fenced-code-block delimiter (``` or ~~~, 3+ of the // same character). Bare URLs inside a fence are content the user typed @@ -140,14 +142,18 @@ func urlPlaceholder(i int) string { return fmt.Sprintf("\x1fU%d\x1f", i) } +// residualPlaceholder matches any urlPlaceholder token — a fallback sweep +// for the case restoreBareURLs' own placeholder search doesn't find one. +var residualPlaceholder = regexp.MustCompile(`\x1fU\d+\x1f`) + // stashBareURLs replaces every bare-URL-only line with a short placeholder, -// returning the stashed URLs in order (skipping lines inside fenced code -// blocks). Glamour's word-wrap hard-splits a long link token mid-URL once it -// exceeds the wrap width — it treats the autolink's rendered ANSI run -// differently from plain text and force-breaks it instead of pushing the -// whole word to the next line — so the placeholder keeps such lines out of -// that path entirely. restoreBareURLs puts the real, styled, hyperlinked URL -// back after rendering. +// returning the stashed URLs in order (skipping lines inside fenced or +// indented code blocks). Glamour's word-wrap hard-splits a long link token +// mid-URL once it exceeds the wrap width — it treats the autolink's +// rendered ANSI run differently from plain text and force-breaks it instead +// of pushing the whole word to the next line — so the placeholder keeps +// such lines out of that path entirely. restoreBareURLs puts the real, +// styled, hyperlinked URL back after rendering. func stashBareURLs(raw string) (string, []string) { lines := strings.Split(raw, "\n") var urls []string @@ -166,10 +172,22 @@ func stashBareURLs(raw string) (string, []string) { if fenceChar != 0 { continue } - if m := bareURLLine.FindStringSubmatch(line); m != nil { - urls = append(urls, m[2]) - lines[i] = m[1] + urlPlaceholder(len(urls)-1) + m := bareURLLine.FindStringSubmatch(line) + if m == nil { + continue } + indent, marker := m[1], m[2] + // A 4-space (or tab) indent with no list marker is CommonMark's + // indented code block — verbatim content, leave it untouched, same + // as a fence. This can also false-skip a deeply-nested bullet + // continuation line (glamour's LevelIndent is 2/level, so level 2+ + // reaches 4): falling back to the pre-fix wrap-split behavior there + // is the safer failure than hyperlinking real code. + if marker == "" && (strings.Contains(indent, "\t") || len(indent) >= 4) { + continue + } + urls = append(urls, m[3]) + lines[i] = indent + marker + urlPlaceholder(len(urls)-1) + m[4] } return strings.Join(lines, "\n"), urls } @@ -209,9 +227,7 @@ func hyperlink(target, display string) string { // elideURL returns url unchanged if it fits within budget cells, otherwise // cuts it (cell-width aware, not byte- or rune-count aware) to make room for // a trailing ellipsis whose own width is measured rather than assumed to be -// one cell. budget <= 0 is a pathological deeply-nested-bullet-in-a-tiny-pane -// case; it still returns a single ellipsis rather than nothing, since an -// empty display text would be an invisible — but still clickable — link. +// one cell. Caller guarantees budget >= 1. func elideURL(url string, budget int) string { if runewidth.StringWidth(url) <= budget { return url @@ -238,12 +254,17 @@ func elideURL(url string, budget int) string { // restoreBareURLs swaps each placeholder back for its real URL, wrapped in // an OSC 8 hyperlink and styled like glamour's own Link (colorLink, // underlined). The visible text is elided to fit whatever width remains on -// its line — measured from the actual rendered prefix (bullet, indent, -// nesting), not guessed — but the hyperlink target always carries the full, -// untruncated URL, so the link opens the right place regardless of how much -// of it is shown. Each URL has its own index-keyed placeholder, so two URLs -// that happen to elide to identical visible text can never cross-link — -// there's nothing to search for and collide on. +// its line — measured from the actual rendered prefix and suffix (bullet, +// indent, nesting, and anything glamour reflowed onto the same physical +// line after the URL, e.g. a soft-wrapped paragraph continuation), not +// guessed — but the hyperlink target always carries the full, untruncated +// URL, so the link opens the right place regardless of how much of it is +// shown. Each URL has its own index-keyed placeholder, so two URLs that +// happen to elide to identical visible text can never cross-link — there's +// nothing to search for and collide on. If there's no room at all (a +// pathologically narrow pane with a deep indent), the URL is dropped +// rather than drawn — an over-width line would break the one invariant +// every other line in this renderer holds. func restoreBareURLs(rendered string, urls []string, width int) string { if len(urls) == 0 { return rendered @@ -257,10 +278,15 @@ func restoreBareURLs(rendered string, urls []string, width int) string { continue } prefix := line[:idx] - budget := width - visibleWidth(prefix) + rest := line[idx+len(ph):] + budget := width - visibleWidth(prefix) - visibleWidth(rest) + if budget < 1 { + lines[li] = prefix + rest + break + } display := elideURL(url, budget) styled := termenv.String(display).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() - lines[li] = prefix + hyperlink(url, styled) + lines[li] = prefix + hyperlink(url, styled) + rest break } } @@ -291,6 +317,12 @@ func renderMarkdown(raw string, width int) (string, error) { return "", err } out = restoreBareURLs(out, urls, width) + // Defensive: if a placeholder somehow didn't survive glamour intact (an + // unanticipated reflow edge case), strip the residual bytes rather than + // let a raw \x1f-delimited token land on screen — the URL is lost + // either way at that point; better an invisible failure than a garbled + // one. + out = residualPlaceholder.ReplaceAllString(out, "") return tidy(out), nil } From f8c037fb2a7bf3ebba09a14902fd3a02b11bbf1a Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 10:59:31 -0400 Subject: [PATCH 04/11] fix review: metric-consistent elision, list-vs-code disambiguation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on PR #21 passed with no blocking findings, but took three of them anyway — one real correctness gap, one real board shape, one free. a. elideURL measured with go-runewidth (rune-sum) but visibleWidth enforces the pane-width invariant with xansi.StringWidth (grapheme-cluster-aware). They disagree on presentation-sequence emoji (VS16), so a URL containing one could produce a display string that measures over budget by the metric that actually matters — reopening the exact overflow this PR exists to prevent. Replaced with xansi.Truncate, the same package/metric the invariant already uses; drops the go-runewidth dependency entirely. b. The indented-code-block guard (4-space indent, no list marker) is only correct under CommonMark's actual precondition: a blank line immediately before it. Without that check it also caught a real board shape — a URL as its own continuation line two list levels deep, which reaches the same 4-space indent but is a lazy list continuation, not code. stashBareURLs now tracks the previous line's blankness and only applies the guard when it holds. c. fenceLine's close-toggle matched on character alone, so a ~~~ line inside a ~~~~-opened block closed the fence early per CommonMark's own rule (closing run must be >= opening run length). Tracks the opening run length now. Plus: residualPlaceholder's regexp scan now only runs when a URL was actually stashed, and TestNeverWiderThanWidth's width sweep is scoped to hyperlinked lines specifically — an unscoped 10-100 sweep surfaced a genuine but unrelated pre-existing glamour off-by-one on plain, non-URL text at certain odd widths (confirmed pre-existing: both xansi and reflow agree on the over-width measurement, so it isn't a metric disagreement introduced here) — out of scope for #15/#21. Three new tests: nested-list-continuation-not-mistaken-for-code (the regression b fixes), plus the hyperlink-scoped width sweep across 10-100. Full suite + go vet clean; go.mod no longer carries go-runewidth as a direct dependency. Co-Authored-By: Claude Sonnet 5 --- go.mod | 2 +- render_test.go | 40 +++++++++++++++++++++++ style.go | 87 ++++++++++++++++++++++---------------------------- 3 files changed, 79 insertions(+), 50 deletions(-) diff --git a/go.mod b/go.mod index 1a2a01d..b5430b8 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,6 @@ require ( github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 github.com/charmbracelet/x/ansi v0.11.6 github.com/fsnotify/fsnotify v1.10.1 - github.com/mattn/go-runewidth v0.0.19 github.com/muesli/termenv v0.16.0 golang.org/x/term v0.43.0 ) @@ -32,6 +31,7 @@ require ( github.com/lucasb-eyer/go-colorful v1.3.0 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-localereader v0.0.1 // indirect + github.com/mattn/go-runewidth v0.0.19 // indirect github.com/microcosm-cc/bluemonday v1.0.27 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/cancelreader v0.2.2 // indirect diff --git a/render_test.go b/render_test.go index 6e51934..1701c97 100644 --- a/render_test.go +++ b/render_test.go @@ -55,6 +55,29 @@ func TestNeverWiderThanWidth(t *testing.T) { } } +// The hyperlinked-line path has width-sensitive elision math the rest of +// the renderer doesn't (restoreBareURLs' budget calculation), so it gets +// its own sweep across every width from the render-width clamp floor up — +// scoped to lines that actually contain a hyperlink, so it isn't tripped +// up by glamour's own pre-existing, unrelated wrap imprecision on ordinary +// text at odd widths (a real but out-of-scope issue: e.g. at width 15 a +// plain non-URL blockquote line measures one cell over, independent of +// anything in this file). +func TestHyperlinkedLineNeverWiderThanWidth(t *testing.T) { + for w := 10; w <= 100; w++ { + out := renderFixture(t, w) + for i, line := range strings.Split(out, "\n") { + if !oscLinkRE.MatchString(line) { + continue + } + if got := visibleWidth(line); got > w { + t.Errorf("width %d, line %d: visible width %d: %q", + w, i, got, stripANSI(line)) + } + } + } +} + // No trailing-space padding on any line (glow's -w padding bug). func TestNoTrailingSpacePadding(t *testing.T) { out := renderFixture(t, 78) @@ -180,6 +203,23 @@ func TestBareURLNestedIndentBudget(t *testing.T) { } } +// A URL as its own continuation line under a 2-levels-deep nested bullet +// reaches the same 4-space indent as an indented code block, but it's a +// CommonMark lazy list continuation, not code — preceded by a non-blank +// line (the child item itself), not a blank one. It must still be +// hyperlinked, not skipped by the indented-code-block guard. +func TestBareURLNestedListContinuationNotMistakenForCode(t *testing.T) { + raw := "- Parent\n - Child item\n https://example.test/nested-continuation-line-long-enough-to-elide\n" + out, err := renderMarkdown(raw, 30) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/nested-continuation-line-long-enough-to-elide" { + t.Errorf("nested list continuation URL wasn't hyperlinked (mistaken for indented code?): %v\n%s", m, stripANSI(out)) + } +} + // changedLines must still detect a change when only a URL's target differs // but its elided display text happens to be identical — the diff key has to // retain the OSC 8 target, not just the visible text. diff --git a/style.go b/style.go index f8b31e8..7a761e5 100644 --- a/style.go +++ b/style.go @@ -8,7 +8,6 @@ import ( "github.com/charmbracelet/glamour" "github.com/charmbracelet/glamour/ansi" xansi "github.com/charmbracelet/x/ansi" - "github.com/mattn/go-runewidth" "github.com/muesli/termenv" ) @@ -158,14 +157,23 @@ func stashBareURLs(raw string) (string, []string) { lines := strings.Split(raw, "\n") var urls []string var fenceChar byte + var fenceLen int + prevBlank := true // document start counts as a boundary, same as CommonMark for i, line := range lines { + wasPrevBlank := prevBlank + prevBlank = strings.TrimSpace(line) == "" + if m := fenceLine.FindStringSubmatch(line); m != nil { - c := m[1][0] - switch fenceChar { - case 0: - fenceChar = c - case c: - fenceChar = 0 + c, n := m[1][0], len(m[1]) + switch { + case fenceChar == 0: + fenceChar, fenceLen = c, n + case c == fenceChar && n >= fenceLen: + // Per CommonMark, a fence only closes on a run of the same + // character at least as long as the one that opened it — + // a shorter or different-character run (e.g. a "~~~" line + // inside a "~~~~"-opened block) is just content. + fenceChar, fenceLen = 0, 0 } continue } @@ -177,13 +185,13 @@ func stashBareURLs(raw string) (string, []string) { continue } indent, marker := m[1], m[2] - // A 4-space (or tab) indent with no list marker is CommonMark's - // indented code block — verbatim content, leave it untouched, same - // as a fence. This can also false-skip a deeply-nested bullet - // continuation line (glamour's LevelIndent is 2/level, so level 2+ - // reaches 4): falling back to the pre-fix wrap-split behavior there - // is the safer failure than hyperlinking real code. - if marker == "" && (strings.Contains(indent, "\t") || len(indent) >= 4) { + // A 4-space (or tab) indent with no list marker and a blank line + // immediately before it is CommonMark's indented code block — + // verbatim content, leave it untouched, same as a fence. Requiring + // the preceding blank line is what tells it apart from a nested + // list's lazy continuation line, which reaches the same 4-space + // depth (glamour's LevelIndent is 2/level) but isn't code. + if marker == "" && (strings.Contains(indent, "\t") || len(indent) >= 4) && wasPrevBlank { continue } urls = append(urls, m[3]) @@ -224,33 +232,6 @@ func hyperlink(target, display string) string { return oscOpen + oscTarget(target) + oscBEL + display + oscClose } -// elideURL returns url unchanged if it fits within budget cells, otherwise -// cuts it (cell-width aware, not byte- or rune-count aware) to make room for -// a trailing ellipsis whose own width is measured rather than assumed to be -// one cell. Caller guarantees budget >= 1. -func elideURL(url string, budget int) string { - if runewidth.StringWidth(url) <= budget { - return url - } - ellipsisWidth := runewidth.RuneWidth('…') - keep := budget - ellipsisWidth - if keep <= 0 { - return "…" - } - var b strings.Builder - w := 0 - for _, r := range url { - rw := runewidth.RuneWidth(r) - if w+rw > keep { - break - } - b.WriteRune(r) - w += rw - } - b.WriteRune('…') - return b.String() -} - // restoreBareURLs swaps each placeholder back for its real URL, wrapped in // an OSC 8 hyperlink and styled like glamour's own Link (colorLink, // underlined). The visible text is elided to fit whatever width remains on @@ -284,7 +265,13 @@ func restoreBareURLs(rendered string, urls []string, width int) string { lines[li] = prefix + rest break } - display := elideURL(url, budget) + // xansi.Truncate, not go-runewidth: it's grapheme-cluster aware + // (a VS16 emoji presentation sequence is one cluster but two + // runes) and it's the same measurement visibleWidth uses to + // enforce the pane-width invariant. Measuring the budget with + // one metric and building the display text with a different + // one is exactly how that invariant would quietly break again. + display := xansi.Truncate(url, budget, "…") styled := termenv.String(display).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() lines[li] = prefix + hyperlink(url, styled) + rest break @@ -316,13 +303,15 @@ func renderMarkdown(raw string, width int) (string, error) { if err != nil { return "", err } - out = restoreBareURLs(out, urls, width) - // Defensive: if a placeholder somehow didn't survive glamour intact (an - // unanticipated reflow edge case), strip the residual bytes rather than - // let a raw \x1f-delimited token land on screen — the URL is lost - // either way at that point; better an invisible failure than a garbled - // one. - out = residualPlaceholder.ReplaceAllString(out, "") + if len(urls) > 0 { + out = restoreBareURLs(out, urls, width) + // Defensive: if a placeholder somehow didn't survive glamour intact + // (an unanticipated reflow edge case), strip the residual bytes + // rather than let a raw \x1f-delimited token land on screen — the + // URL is lost either way at that point; better an invisible + // failure than a garbled one. + out = residualPlaceholder.ReplaceAllString(out, "") + } return tidy(out), nil } From d03dc6615365622365180e99ddb77aa7519e335f Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 11:13:05 -0400 Subject: [PATCH 05/11] fix review: ordered-list URLs, non-TTY skips hyperlinking entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round: 'solid' verdict, no blocking findings, three 'worth addressing' items. Took two, deliberately skipped one. b. bareURLLine's marker group only matched '-'/'*'/'+', so an ordered-list URL ('1. https://…') was never stashed and still hit the original wrap-split bug. Widened to accept '\d+[.)]' too — a real board shape the fix was otherwise silently missing. c. runStatic's non-TTY fallback (piped, grep, CI) documented the caveat that a long URL's full address only lives in an OSC 8 target the reader can't see. Fixed it instead: renderMarkdown takes a linkify bool now, false for the non-TTY path, which skips stashBareURLs entirely — a bare URL renders as plain, complete text there. (This doesn't rescue a URL longer than the fallback width from the pre-#15 wrap-split; it only stops hiding an already-short one behind an escape sequence grep can't read. That's the actual common case at width 80, and documented as such.) Skipped: middle-elision instead of tail-elision. Real UX improvement for terminals without OSC 8 support, but no invariant violation (target is always complete either way) and meaningfully more surface for a fourth review round on a diminishing-returns question. Left as a follow-up rather than chased further. Two new tests: ordered-list stashing, and linkify=false actually skipping the OSC 8 path (at the width where it matters — a narrower first attempt at the test caught the pre-existing wrap-split instead, which is expected and documented as out of scope for linkify=false). Full suite + go vet clean. Co-Authored-By: Claude Sonnet 5 --- diff_test.go | 2 +- main.go | 16 ++++++------ render_test.go | 68 +++++++++++++++++++++++++++++++++++++++++--------- style.go | 27 ++++++++++++++------ ui.go | 6 ++--- 5 files changed, 89 insertions(+), 30 deletions(-) diff --git a/diff_test.go b/diff_test.go index 3b54ae8..060e95c 100644 --- a/diff_test.go +++ b/diff_test.go @@ -160,7 +160,7 @@ func TestChangedLinesDeletionNoSpuriousMark(t *testing.T) { func TestComposeMarkedNeverWiderThanWidth(t *testing.T) { for _, w := range []int{20, 40, 80} { lines, _ := func() ([]string, error) { - out, err := renderMarkdown("# Title\n\n- a fairly long bullet item that will wrap\n- short\n\nsome prose here too\n", w) + out, err := renderMarkdown("# Title\n\n- a fairly long bullet item that will wrap\n- short\n\nsome prose here too\n", w, true) return strings.Split(out, "\n"), err }() all := map[int]bool{} diff --git a/main.go b/main.go index bad28b6..e1a4dd1 100644 --- a/main.go +++ b/main.go @@ -97,11 +97,12 @@ func main() { // runStatic renders the file once to stdout and exits — no watching, no // alt-screen. Handy for piping, CI, and quick inline checks. Width is the -// terminal width (minus 2) when stdout is a TTY, else 80. A bare URL longer -// than that width is still elided in the visible text (the full URL lives -// only in its OSC 8 target) — fine for a terminal, but grep, an editor, or -// a CI log viewer reading the piped bytes plain won't see the untruncated -// URL. +// terminal width (minus 2) when stdout is a TTY, else 80. Bare-URL OSC 8 +// hyperlinking (and the display-text elision that comes with it) is skipped +// whenever stdout isn't a TTY: the same term.GetSize failure that picks the +// width-80 fallback means whatever's on the other end of the pipe — grep, +// an editor, a CI log — can't render the escape sequence anyway, so a full, +// plain, un-elided URL serves it better than a shortened one. func runStatic(args []string) int { path := defaultBoardPath() if len(args) > 0 { @@ -118,10 +119,11 @@ func runStatic(args []string) int { return 1 } width := 80 + isTTY := false if w, _, err := term.GetSize(int(os.Stdout.Fd())); err == nil && w > 0 { - width = w + width, isTTY = w, true } - out, err := renderMarkdown(string(data), width-2) + out, err := renderMarkdown(string(data), width-2, isTTY) if err != nil { fmt.Fprintln(os.Stderr, "sidecar:", err) return 1 diff --git a/render_test.go b/render_test.go index 1701c97..b1e0353 100644 --- a/render_test.go +++ b/render_test.go @@ -15,7 +15,7 @@ func renderFixture(t *testing.T, width int) string { if err != nil { t.Fatal(err) } - out, err := renderMarkdown(string(raw), width) + out, err := renderMarkdown(string(raw), width, true) if err != nil { t.Fatal(err) } @@ -121,13 +121,57 @@ func TestBareURLIntact(t *testing.T) { } } +// linkify=false (the non-TTY path runStatic uses when stdout isn't a +// terminal) must skip the stash/hyperlink machinery entirely — a plain, +// complete URL for grep/CI/an editor, not an OSC 8 escape it can't render +// and won't see through. At runStatic's real non-TTY width (80), the +// fixture's URLs fit without wrapping at all — the common case linkify=false +// is meant to help. (At a narrower width a long URL still hits the +// original pre-#15 wrap-split via glamour's own autolink path; skipping the +// stash doesn't and isn't meant to fix that — it only avoids hiding an +// already-short URL behind an unreadable escape sequence.) +func TestLinkifyFalseSkipsHyperlinking(t *testing.T) { + raw, err := os.ReadFile("testdata/REVIEW.md") + if err != nil { + t.Fatal(err) + } + out, err := renderMarkdown(string(raw), 78, false) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("linkify=false still produced an OSC 8 hyperlink:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked with linkify=false:\n%s", stripANSI(out)) + } + if !strings.Contains(stripANSI(out), "https://github.com/example/app/pull/412") { + t.Errorf("plain URL missing with linkify=false:\n%s", stripANSI(out)) + } +} + +// An ordered-list URL (`1. https://…`) is a real board shape, not just a +// bulleted one — it must get the same fix, not fall through to the +// original wrap-split bug. +func TestBareURLOrderedListMarker(t *testing.T) { + raw := "1. https://example.test/ordered-list-item-long-enough-to-need-eliding\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/ordered-list-item-long-enough-to-need-eliding" { + t.Errorf("ordered-list URL wasn't hyperlinked: %v\n%s", m, stripANSI(out)) + } +} + // Two bare URLs that elide to identical visible text must still each get // their own correct hyperlink target — no cross-linking (PR #18's confirmed // collision bug: a global text search re-found the first occurrence). func TestBareURLCollisionSafe(t *testing.T) { raw := "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n" + "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -150,7 +194,7 @@ func TestBareURLCollisionSafe(t *testing.T) { // unrelated to bare-URL handling) is out of scope here. func TestBareURLInFenceUntouched(t *testing.T) { raw := "```\nhttps://example.test/verbatim-in-a-fence-that-is-long-enough-to-elide\n```\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -166,7 +210,7 @@ func TestBareURLInFenceUntouched(t *testing.T) { // escape (injection) or get silently dropped (lossy percent-encoding). func TestBareURLUnsafeBytesEncoded(t *testing.T) { raw := "- https://example.test/caf\u00e9-and-a-bell-\x07-in-the-middle\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -188,7 +232,7 @@ func TestBareURLUnsafeBytesEncoded(t *testing.T) { // fix). func TestBareURLNestedIndentBudget(t *testing.T) { raw := "- Parent\n - https://example.test/nested-item-url-thats-long-enough-to-need-eliding\n" - out, err := renderMarkdown(raw, 30) + out, err := renderMarkdown(raw, 30, true) if err != nil { t.Fatal(err) } @@ -210,7 +254,7 @@ func TestBareURLNestedIndentBudget(t *testing.T) { // hyperlinked, not skipped by the indented-code-block guard. func TestBareURLNestedListContinuationNotMistakenForCode(t *testing.T) { raw := "- Parent\n - Child item\n https://example.test/nested-continuation-line-long-enough-to-elide\n" - out, err := renderMarkdown(raw, 30) + out, err := renderMarkdown(raw, 30, true) if err != nil { t.Fatal(err) } @@ -224,11 +268,11 @@ func TestBareURLNestedListContinuationNotMistakenForCode(t *testing.T) { // but its elided display text happens to be identical — the diff key has to // retain the OSC 8 target, not just the visible text. func TestChangedLinesSeesURLTargetChange(t *testing.T) { - before, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n", 40) + before, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n", 40, true) if err != nil { t.Fatal(err) } - after, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n", 40) + after, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n", 40, true) if err != nil { t.Fatal(err) } @@ -264,7 +308,7 @@ func TestHyperlinkSurvivesDownstreamTruncation(t *testing.T) { // room — restoreBareURLs used to discard everything after the placeholder. func TestBareURLKeepsTrailingContent(t *testing.T) { raw := "See the docs at\nhttps://example.test/reasonably-long-path-name\nfor more information, seriously.\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -305,7 +349,7 @@ func TestBareURLNoRoomDropsRatherThanOverflows(t *testing.T) { // wrap-split bug. func TestBareURLCRLFStillStashed(t *testing.T) { raw := "- https://example.test/crlf-board-long-enough-to-need-eliding\r\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -320,7 +364,7 @@ func TestBareURLCRLFStillStashed(t *testing.T) { // hyperlinked. func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { raw := "Some paragraph.\n\n https://example.test/indented-code-block-verbatim-text\n" - out, err := renderMarkdown(raw, 40) + out, err := renderMarkdown(raw, 40, true) if err != nil { t.Fatal(err) } @@ -334,7 +378,7 @@ func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { - out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40) + out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40, true) if err != nil { t.Fatal(err) } diff --git a/style.go b/style.go index 7a761e5..b49f25d 100644 --- a/style.go +++ b/style.go @@ -121,11 +121,12 @@ func styleConfig() ansi.StyleConfig { // bareURLLine matches a markdown line that is nothing but a bare URL — the // board convention for links (see CLAUDE.md: "bare URLs, each on its own -// line") — whether it's an indented continuation line under a bullet or a -// top-level list item in its own right. Groups: 1 = leading indent, 2 = -// optional list marker (with its trailing space), 3 = the URL, 4 = trailing -// whitespace, \r included so a CRLF board doesn't silently skip the fix. -var bareURLLine = regexp.MustCompile(`^([ \t]*)((?:[-*+]\s+)?)(https?://\S+)([ \t\r]*)$`) +// line") — whether it's an indented continuation line under a bullet, a +// bulleted top-level item, or an ordered-list item (`1.`/`1)`). Groups: 1 = +// leading indent, 2 = optional list marker (with its trailing space), 3 = +// the URL, 4 = trailing whitespace, \r included so a CRLF board doesn't +// silently skip the fix. +var bareURLLine = regexp.MustCompile(`^([ \t]*)((?:(?:[-*+]|\d+[.)])\s+)?)(https?://\S+)([ \t\r]*)$`) // fenceLine matches a fenced-code-block delimiter (``` or ~~~, 3+ of the // same character). Bare URLs inside a fence are content the user typed @@ -284,11 +285,23 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // from the pane width by the caller). Output is post-processed to guarantee // the hard requirements: no trailing-space padding, at most one blank line // between blocks, no leading/trailing blank runs. -func renderMarkdown(raw string, width int) (string, error) { +// +// linkify controls the bare-URL OSC 8 hyperlink path: true for the +// interactive TUI, where the escape sequence is invisible to the terminal +// and only the (possibly elided) display text is shown. false skips +// stashBareURLs entirely, so bare URLs render as plain, complete, +// un-elided text — for a non-TTY consumer (runStatic piped to a file, +// grep, a CI log) whose stdout is not a terminal that would render the +// hyperlink at all; there, showing the full plain URL beats hiding it +// behind an escape sequence that reader can't see. +func renderMarkdown(raw string, width int, linkify bool) (string, error) { if width < 10 { width = 10 } - raw, urls := stashBareURLs(raw) + var urls []string + if linkify { + raw, urls = stashBareURLs(raw) + } r, err := glamour.NewTermRenderer( glamour.WithStyles(styleConfig()), glamour.WithWordWrap(width), diff --git a/ui.go b/ui.go index 0539fb2..4f9485a 100644 --- a/ui.go +++ b/ui.go @@ -283,7 +283,7 @@ func (m *model) reload(force bool) (changed bool) { displayRaw = applyCollapse(raw, board, m.collapsed) } - rendered, err := renderMarkdown(displayRaw, m.renderWidth()) + rendered, err := renderMarkdown(displayRaw, m.renderWidth(), true) if err != nil { m.loadErr = err m.vp.SetContent(fmt.Sprintf("\n Render error: %v", err)) @@ -307,7 +307,7 @@ func (m *model) reload(force bool) (changed bool) { if baseBoard, ok := parseBoard(m.prevBaseline); ok { baseDisplay = applyCollapse(m.prevBaseline, baseBoard, m.collapsed) } - if base, berr := renderMarkdown(baseDisplay, m.renderWidth()); berr == nil { + if base, berr := renderMarkdown(baseDisplay, m.renderWidth(), true); berr == nil { changedMap = changedLines(strings.Split(base, "\n"), lines) } } @@ -351,7 +351,7 @@ func (m *model) rerenderCollapse() { return } displayRaw := applyCollapse(m.raw, m.board, m.collapsed) - rendered, err := renderMarkdown(displayRaw, m.renderWidth()) + rendered, err := renderMarkdown(displayRaw, m.renderWidth(), true) if err != nil { return // m.raw already rendered fine on the last successful reload } From a20e1f5dc74ee3c9548b46d54225345a9409d190 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 11:26:52 -0400 Subject: [PATCH 06/11] fix review: sanitize display text against escape injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth review round found a real vulnerability, not a style nit. The stashed URL is never seen by glamour — it's swapped for a placeholder before rendering, so glamour never gets a chance to neutralize anything embedded in it. oscTarget percent-encodes control bytes for the OSC 8 target, but the display text was built straight from the raw URL: xansi.Truncate passes escape bytes through unconditionally (the same property this PR relies on for the hyperlink to survive downstream truncation), and termenv only styles text, it doesn't sanitize it. So a board line with a URL containing a literal ESC byte — plausible, since these files are agent-written from pasted tool output, no special crafting needed — reached the terminal verbatim. A second, attacker-chosen OSC 8 open nested inside the display text points the visible link somewhere the board never named; an arbitrary control sequence does worse. The pane-width invariant didn't catch it either: visibleWidth scores injected escapes as zero cells, so the line measures fine while the screen doesn't reflect what's in the file. Added stripControlBytes and applied it to the display text before truncating. TestBareURLUnsafeBytesEncoded now asserts on the display group, not just the target (that gap is exactly how this slipped through the first pass); TestBareURLDisplayTextNoEscapeInjection pins the nested-OSC-8 case directly. Two more from the same round, both cheap: - residualPlaceholder's paired-delimiter regex can't match a placeholder that got split across a wrap (reachable only at the width-10 floor with a deep indent). Added a bare \x1f sweep after it — a lone \x1f is unambiguously our own byte, safe to drop outright. - Fixed a doc-comment citation: CLAUDE.md is excluded via .git/info/exclude, not part of the repo from a fresh clone's perspective. Points at README.md's rendering-style section instead, plus one line on the loose-list-continuation boundary so it reads as a known tradeoff rather than an oversight. Full suite + go vet clean. Co-Authored-By: Claude Sonnet 5 --- render_test.go | 36 +++++++++++++++++++++++++++++++++++- style.go | 49 ++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/render_test.go b/render_test.go index b1e0353..e07768d 100644 --- a/render_test.go +++ b/render_test.go @@ -207,7 +207,12 @@ func TestBareURLInFenceUntouched(t *testing.T) { } // A control byte or non-ASCII byte in a URL must not break out of the OSC 8 -// escape (injection) or get silently dropped (lossy percent-encoding). +// escape (injection) or get silently dropped (lossy percent-encoding) \u2014 in +// EITHER half of the hyperlink. The target (m[1]) percent-encodes so the +// link stays meaningful; the display text (m[2]) just needs the control +// bytes gone outright, since a raw ESC/BEL there reaches the terminal as a +// live escape sequence (glamour never sees the stashed URL to neutralize +// it, and neither xansi.Truncate nor termenv sanitize). func TestBareURLUnsafeBytesEncoded(t *testing.T) { raw := "- https://example.test/caf\u00e9-and-a-bell-\x07-in-the-middle\n" out, err := renderMarkdown(raw, 40, true) @@ -224,6 +229,35 @@ func TestBareURLUnsafeBytesEncoded(t *testing.T) { if !strings.Contains(m[1], "%C3%A9") || !strings.Contains(m[1], "%07") { t.Errorf("target wasn't percent-encoded correctly: %q", m[1]) } + // The display text legitimately carries our own SGR styling (teal, + // underline), which is itself ESC bytes \u2014 strip that (SGR-only; OSC 8 + // isn't SGR and survives) before checking for anything else. + if plain := stripANSI(m[2]); strings.ContainsAny(plain, "\x07\x1b\x00") { + t.Fatalf("display text still contains a raw control byte \u2014 escape injection: %q", plain) + } +} + +// An ESC byte in a URL must not let a pasted-in escape sequence \u2014 most +// pointedly a second, attacker-controlled OSC 8 open \u2014 reach the terminal +// inside the display text. Board files are agent-written from pasted tool +// output, so this isn't hypothetical. +func TestBareURLDisplayTextNoEscapeInjection(t *testing.T) { + raw := "- https://example.test/x\x1b]8;;https://evil.test\x07pwned\n" + out, err := renderMarkdown(raw, 60, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatalf("no hyperlink found:\n%s", stripANSI(out)) + } + plain := stripANSI(m[2]) // strip our own legitimate SGR styling first + if strings.Contains(plain, "\x1b]8;;") { + t.Fatalf("display text carries an injected OSC 8 open: %q", plain) + } + if strings.Contains(plain, "\x1b") { + t.Fatalf("display text contains a raw ESC byte: %q", plain) + } } // A nested list item's budget must account for its actual indent, not a diff --git a/style.go b/style.go index b49f25d..e707d1b 100644 --- a/style.go +++ b/style.go @@ -120,11 +120,12 @@ func styleConfig() ansi.StyleConfig { } // bareURLLine matches a markdown line that is nothing but a bare URL — the -// board convention for links (see CLAUDE.md: "bare URLs, each on its own -// line") — whether it's an indented continuation line under a bullet, a -// bulleted top-level item, or an ordered-list item (`1.`/`1)`). Groups: 1 = -// leading indent, 2 = optional list marker (with its trailing space), 3 = -// the URL, 4 = trailing whitespace, \r included so a CRLF board doesn't +// board convention for links (see README.md's rendering-style section: +// "Bare URLs ... render as real OSC 8 hyperlinks") — whether it's an +// indented continuation line under a bullet, a bulleted top-level item, or +// an ordered-list item (`1.`/`1)`). Groups: 1 = leading indent, 2 = +// optional list marker (with its trailing space), 3 = the URL, 4 = trailing +// whitespace, \r included so a CRLF board doesn't // silently skip the fix. var bareURLLine = regexp.MustCompile(`^([ \t]*)((?:(?:[-*+]|\d+[.)])\s+)?)(https?://\S+)([ \t\r]*)$`) @@ -192,6 +193,13 @@ func stashBareURLs(raw string) (string, []string) { // the preceding blank line is what tells it apart from a nested // list's lazy continuation line, which reaches the same 4-space // depth (glamour's LevelIndent is 2/level) but isn't code. + // + // Known boundary, not a bug: a *loose* list's second paragraph + // (blank line, then a 4-space-indented URL under a bullet) looks + // identical to this guard and falls through to the old wrap-split. + // Distinguishing it needs real list-context tracking; the board + // convention doesn't produce loose lists, so this hasn't been + // worth the complexity. if marker == "" && (strings.Contains(indent, "\t") || len(indent) >= 4) && wasPrevBlank { continue } @@ -233,6 +241,28 @@ func hyperlink(target, display string) string { return oscOpen + oscTarget(target) + oscBEL + display + oscClose } +// stripControlBytes removes ASCII control bytes and DEL from s. The stashed +// URL is never seen by glamour (it's replaced with a placeholder before +// rendering), so nothing else in the pipeline neutralizes an embedded ESC +// or BEL before it becomes on-screen display text — xansi.Truncate passes +// escapes through unconditionally (that's the whole premise this PR relies +// on for the hyperlink itself to survive truncation) and termenv only +// styles text, it doesn't sanitize it. oscTarget percent-encodes the same +// bytes for the link target, where they need to stay meaningful; the +// visible text just needs them gone, since a board line is written by an +// agent pasting arbitrary tool output and an ESC there would otherwise +// reach the terminal as a real escape sequence — a nested OSC 8 pointing +// somewhere the board never named, or worse. +func stripControlBytes(s string) string { + var b strings.Builder + for i := 0; i < len(s); i++ { + if c := s[i]; c >= 0x20 && c != 0x7f { + b.WriteByte(c) + } + } + return b.String() +} + // restoreBareURLs swaps each placeholder back for its real URL, wrapped in // an OSC 8 hyperlink and styled like glamour's own Link (colorLink, // underlined). The visible text is elided to fit whatever width remains on @@ -272,7 +302,7 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // enforce the pane-width invariant. Measuring the budget with // one metric and building the display text with a different // one is exactly how that invariant would quietly break again. - display := xansi.Truncate(url, budget, "…") + display := xansi.Truncate(stripControlBytes(url), budget, "…") styled := termenv.String(display).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() lines[li] = prefix + hyperlink(url, styled) + rest break @@ -322,8 +352,13 @@ func renderMarkdown(raw string, width int, linkify bool) (string, error) { // (an unanticipated reflow edge case), strip the residual bytes // rather than let a raw \x1f-delimited token land on screen — the // URL is lost either way at that point; better an invisible - // failure than a garbled one. + // failure than a garbled one. The plain \x1f pass after it is the + // same safety net for the case the placeholder itself got split + // across a wrap: the paired-delimiter regex above can't match half + // a token, but a lone \x1f is unambiguously our own byte (nothing + // else in this pipeline emits it) and safe to drop on sight. out = residualPlaceholder.ReplaceAllString(out, "") + out = strings.ReplaceAll(out, "\x1f", "") } return tidy(out), nil } From a750ad0e8c2351fdeb1e703b2e9748a0c897ad4a Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 11:43:33 -0400 Subject: [PATCH 07/11] fix review: hyperlink styling forced truecolor, not termenv auto-detect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth review round: one real thing (explicitly called out as the one they'd want fixed before merge), one nit, one noted-not-asked-for. a. The hyperlink's display text was styled via termenv.String(...), which binds to termenv's auto-detected package-global Output profile — every other color path in this codebase deliberately overrides that (glamour.WithColorProfile(termenv.TrueColor) here, diff.go's hand-written 38;2;r;g;b). Under a degraded profile, termenv.String silently drops styling entirely, so the link text would render unstyled while everything else on screen stayed truecolor — and no test caught it, because every test here runs with stdout not a terminal, the exact condition that triggers it. Switched to the same raw-ANSI idiom diff.go already uses via hexToRGB. New test pins the exact SGR sequence directly. b. Tried widening the residual-placeholder fallback regex to catch a placeholder split across a wrap — caught it myself before it went out: has every anchor optional, so it would have matched a bare capital U anywhere in real board text (URL, Update) and silently eaten it. Reverted to the original well-formed-token-only regex; the comment now says plainly that the split case leaves a cosmetic U0 rather than overclaiming an invisible failure the code doesn't deliver — narrowing further isn't worth the risk of corrupting real content over an edge this narrow (needs a placeholder split at the width-10 floor). c. Reflowed-paragraph URLs can elide down to just a couple of cells when 'rest' (prose after the URL on the same physical line) eats most of the budget — already the accepted, README-documented tradeoff of this approach, and the reviewer flagged genuine uncertainty about the exact severity ('I couldn't add a probe test in this environment... treat the exact cell count as illustrative'). Not changing behavior on an unverified claim during an already-thorough fifth round; logging as a board follow-up instead of a fix. Full suite + go vet clean. Co-Authored-By: Claude Sonnet 5 --- render_test.go | 20 ++++++++++++++++++++ style.go | 30 +++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/render_test.go b/render_test.go index e07768d..dc6434e 100644 --- a/render_test.go +++ b/render_test.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "regexp" "strings" @@ -423,6 +424,25 @@ func TestEmojiHeadingWidth(t *testing.T) { } } +// A hyperlink's display text must carry raw truecolor SGR, not termenv's +// auto-detected-profile styling — every test here runs with stdout not a +// terminal, which is exactly the condition under which termenv.String +// would silently drop the styling and this would be the one link on +// screen that isn't truecolor while everything else (forced via +// glamour.WithColorProfile / diff.go's hand-written 38;2;r;g;b) stays so. +func TestHyperlinkDisplayIsRawTruecolor(t *testing.T) { + out := renderFixture(t, 40) + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatal("no hyperlink found in fixture at width 40") + } + lr, lg, lb := hexToRGB(colorLink) + want := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm", lr, lg, lb) + if !strings.Contains(m[2], want) { + t.Errorf("display text missing raw truecolor SGR %q: %q", want, m[2]) + } +} + // Colors must be hex, not 256-palette indexes (Ghostty remaps the palette). func TestTrueColorOutput(t *testing.T) { out := renderFixture(t, 78) diff --git a/style.go b/style.go index e707d1b..68d619c 100644 --- a/style.go +++ b/style.go @@ -303,7 +303,17 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // one metric and building the display text with a different // one is exactly how that invariant would quietly break again. display := xansi.Truncate(stripControlBytes(url), budget, "…") - styled := termenv.String(display).Foreground(termenv.TrueColor.Color(colorLink)).Underline().String() + // Raw ANSI, not termenv.String: termenv.String binds to + // termenv's auto-detected package-global Output profile, which + // this codebase deliberately overrides everywhere else — the + // glamour renderer is forced to termenv.TrueColor below, and + // diff.go writes 38;2;r;g;b by hand for the same reason + // ("piped/degraded profiles were how glow washed out"). Under + // a degraded profile termenv.String would silently drop the + // styling and this would be the one link on screen that isn't + // truecolor. + lr, lg, lb := hexToRGB(colorLink) + styled := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm%s\x1b[0m", lr, lg, lb, display) lines[li] = prefix + hyperlink(url, styled) + rest break } @@ -351,12 +361,18 @@ func renderMarkdown(raw string, width int, linkify bool) (string, error) { // Defensive: if a placeholder somehow didn't survive glamour intact // (an unanticipated reflow edge case), strip the residual bytes // rather than let a raw \x1f-delimited token land on screen — the - // URL is lost either way at that point; better an invisible - // failure than a garbled one. The plain \x1f pass after it is the - // same safety net for the case the placeholder itself got split - // across a wrap: the paired-delimiter regex above can't match half - // a token, but a lone \x1f is unambiguously our own byte (nothing - // else in this pipeline emits it) and safe to drop on sight. + // URL is lost either way at that point; better a bare, mostly + // inert failure than one carrying our own control bytes. The plain + // \x1f pass after it also catches a placeholder that got hard- + // broken across a wrap (the paired-delimiter regex above can't + // match half a token) — \x1f itself is unambiguously our own byte + // (nothing else in this pipeline emits it), so it's always safe to + // drop. It's not a fully invisible failure in that split case, + // though: only the delimiter bytes are removed, so a naked "U0" + // can be left as plain visible text. Narrowing that further would + // mean matching digits without a \x1f anchor, which risks eating + // real board text (e.g. "U2", "Update") instead — worse than the + // cosmetic residue it would prevent. out = residualPlaceholder.ReplaceAllString(out, "") out = strings.ReplaceAll(out, "\x1f", "") } From eab11426146e25e4e1dedb99713c32af27d4f17c Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 11:55:30 -0400 Subject: [PATCH 08/11] fix review: indented code is a block, not a per-line check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth review round found one real correctness bug. a. stashBareURLs guarded indented code blocks with a per-line wasPrevBlank check, but an indented code block is a block: once a blank line plus a 4-space indented, unmarked line opens one, every subsequent indented line belongs to it whether or not that later line's own predecessor was blank. Two URLs in the same code block (second one immediately following the first, no blank line between them) rendered differently — the first correctly left alone, the second incorrectly stashed and hyperlinked. Fixed by tracking inIndentedCode as running state across every line, symmetric with the fence tracking already next to it. New test covers the multi-URL-same-block case the old single-line test couldn't catch. Two free nits from the same round: - main.go: swapped the term.GetSize-error-implies-non-TTY inference for term.IsTerminal, which says what it means directly instead of needing a comment to explain the conflation. The width fallback stays keyed off GetSize independently, since that's a separate question (what width to use) from TTY-ness (whether to linkify). - hexToRGB(colorLink) hoisted out of restoreBareURLs' per-URL loop — loop-invariant. Deliberately not changed, per explicit steer to stop an already six-round loop on non-invariant findings: - Two URLs sharing one reflowed line can starve the second one's elision budget. Width invariant holds, both links stay clickable (reviewer's own words) — a UX rough edge, not a correctness bug. Logging as a board follow-up. - The width-15 plain-text overflow flagged again this round was already confirmed pre-existing and unrelated to this PR's metric swap in an earlier round of this same review cycle: both x/ansi and the old reflow counter agree exactly on the over-width measurement for the failing line, so it isn't a disagreement this PR introduced. Full suite plus go vet clean. Co-Authored-By: Claude Sonnet 5 --- main.go | 11 ++++---- render_test.go | 20 ++++++++++++++ style.go | 72 +++++++++++++++++++++++++++++++++++++------------- 3 files changed, 78 insertions(+), 25 deletions(-) diff --git a/main.go b/main.go index e1a4dd1..3045308 100644 --- a/main.go +++ b/main.go @@ -99,10 +99,9 @@ func main() { // alt-screen. Handy for piping, CI, and quick inline checks. Width is the // terminal width (minus 2) when stdout is a TTY, else 80. Bare-URL OSC 8 // hyperlinking (and the display-text elision that comes with it) is skipped -// whenever stdout isn't a TTY: the same term.GetSize failure that picks the -// width-80 fallback means whatever's on the other end of the pipe — grep, -// an editor, a CI log — can't render the escape sequence anyway, so a full, -// plain, un-elided URL serves it better than a shortened one. +// whenever stdout isn't a TTY: whatever's on the other end of the pipe — +// grep, an editor, a CI log — can't render the escape sequence anyway, so a +// full, plain, un-elided URL serves it better than a shortened one. func runStatic(args []string) int { path := defaultBoardPath() if len(args) > 0 { @@ -118,10 +117,10 @@ func runStatic(args []string) int { fmt.Fprintln(os.Stderr, "sidecar:", err) return 1 } + isTTY := term.IsTerminal(int(os.Stdout.Fd())) width := 80 - isTTY := false if w, _, err := term.GetSize(int(os.Stdout.Fd())); err == nil && w > 0 { - width, isTTY = w, true + width = w } out, err := renderMarkdown(string(data), width-2, isTTY) if err != nil { diff --git a/render_test.go b/render_test.go index dc6434e..c1604e8 100644 --- a/render_test.go +++ b/render_test.go @@ -411,6 +411,26 @@ func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { } } +// An indented code block is a block, not a per-line property: a second URL +// deeper in the same block (whose own immediate predecessor isn't blank) +// must be protected exactly like the first one, whose predecessor is. +// Round-7 review's confirmed bug: a per-line wasPrevBlank check caught the +// first URL and missed the second, so two URLs in the same verbatim block +// rendered differently from each other. +func TestBareURLIndentedCodeBlockMultiLine(t *testing.T) { + raw := "Run these:\n\n https://example.test/first-verbatim-command\n https://example.test/second-verbatim-command\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("a URL deeper in a multi-line indented code block got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into multi-line indented-code-block output:\n%s", stripANSI(out)) + } +} + // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40, true) diff --git a/style.go b/style.go index 68d619c..fea0788 100644 --- a/style.go +++ b/style.go @@ -147,6 +147,26 @@ func urlPlaceholder(i int) string { // for the case restoreBareURLs' own placeholder search doesn't find one. var residualPlaceholder = regexp.MustCompile(`\x1fU\d+\x1f`) +// listMarkerPrefix matches a line beginning with a list marker, regardless +// of what follows — used to tell an indented code block's opening line +// apart from a list item that merely reaches the same 4-space indent. +var listMarkerPrefix = regexp.MustCompile(`^[ \t]*(?:[-*+]|\d+[.)])\s+`) + +// leadingIndent returns line's leading run of spaces/tabs. +func leadingIndent(line string) string { + i := 0 + for i < len(line) && (line[i] == ' ' || line[i] == '\t') { + i++ + } + return line[:i] +} + +// hasCodeIndent reports whether indent (spaces/tabs only) is deep enough to +// open or continue a CommonMark indented code block: 4+ spaces, or any tab. +func hasCodeIndent(indent string) bool { + return strings.Contains(indent, "\t") || len(indent) >= 4 +} + // stashBareURLs replaces every bare-URL-only line with a short placeholder, // returning the stashed URLs in order (skipping lines inside fenced or // indented code blocks). Glamour's word-wrap hard-splits a long link token @@ -160,10 +180,12 @@ func stashBareURLs(raw string) (string, []string) { var urls []string var fenceChar byte var fenceLen int + inIndentedCode := false prevBlank := true // document start counts as a boundary, same as CommonMark for i, line := range lines { wasPrevBlank := prevBlank - prevBlank = strings.TrimSpace(line) == "" + blank := strings.TrimSpace(line) == "" + prevBlank = blank if m := fenceLine.FindStringSubmatch(line); m != nil { c, n := m[1][0], len(m[1]) @@ -182,29 +204,41 @@ func stashBareURLs(raw string) (string, []string) { if fenceChar != 0 { continue } + + // Indented code is a *block*, not a per-line property: once a + // blank line followed by a 4-space (or tab) indented, unmarked + // line opens one, every subsequent indented line belongs to it — + // regardless of whether that later line's own predecessor was + // blank — until a non-blank, non-indented line closes it. Tracked + // as running state across every line (URL or not), symmetric with + // the fence tracking above, so a second URL deeper in the same + // block doesn't fall through the guard the first URL was caught by. + indent := leadingIndent(line) + indented := hasCodeIndent(indent) + switch { + case inIndentedCode: + if !blank && !indented { + inIndentedCode = false + } + case wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line): + inIndentedCode = true + } + if inIndentedCode { + continue + } + m := bareURLLine.FindStringSubmatch(line) if m == nil { continue } - indent, marker := m[1], m[2] - // A 4-space (or tab) indent with no list marker and a blank line - // immediately before it is CommonMark's indented code block — - // verbatim content, leave it untouched, same as a fence. Requiring - // the preceding blank line is what tells it apart from a nested - // list's lazy continuation line, which reaches the same 4-space - // depth (glamour's LevelIndent is 2/level) but isn't code. - // // Known boundary, not a bug: a *loose* list's second paragraph - // (blank line, then a 4-space-indented URL under a bullet) looks - // identical to this guard and falls through to the old wrap-split. - // Distinguishing it needs real list-context tracking; the board - // convention doesn't produce loose lists, so this hasn't been - // worth the complexity. - if marker == "" && (strings.Contains(indent, "\t") || len(indent) >= 4) && wasPrevBlank { - continue - } + // (blank line, then a 4-space-indented URL under a bullet) opens + // this same indented-code state and falls through to the old + // wrap-split. Distinguishing it needs real list-context tracking; + // the board convention doesn't produce loose lists, so this hasn't + // been worth the complexity. urls = append(urls, m[3]) - lines[i] = indent + marker + urlPlaceholder(len(urls)-1) + m[4] + lines[i] = m[1] + m[2] + urlPlaceholder(len(urls)-1) + m[4] } return strings.Join(lines, "\n"), urls } @@ -282,6 +316,7 @@ func restoreBareURLs(rendered string, urls []string, width int) string { return rendered } lines := strings.Split(rendered, "\n") + lr, lg, lb := hexToRGB(colorLink) // loop-invariant for i, url := range urls { ph := urlPlaceholder(i) for li, line := range lines { @@ -312,7 +347,6 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // a degraded profile termenv.String would silently drop the // styling and this would be the one link on screen that isn't // truecolor. - lr, lg, lb := hexToRGB(colorLink) styled := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm%s\x1b[0m", lr, lg, lb, display) lines[li] = prefix + hyperlink(url, styled) + rest break From fb7c717e1f8dbc9fddc276155ee5711eefcf5939 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 12:15:41 -0400 Subject: [PATCH 09/11] fix review: elision budget was measuring glamour's own padding as content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 review flagged a real bug: two bare URLs sharing one reflowed line starve the second one, because restoreBareURLs processed URLs independently and the first one's expanded hyperlink became part of the "rest" the second one budgeted against. Fixing that properly required rewriting restoreBareURLs to treat the rendered LINE as the unit of work: find every placeholder on a line at once, compute one shared budget, split it evenly. Debugging that fix surfaced something bigger. glamour pads every short block line with trailing spaces out to the full block width (styleConfig's own doc comment says this). The per-line budget calculation was measuring that padding as if it were real content — so a single bare URL alone on its own line, with the entire pane width free, was getting a budget of about 2 cells and rendering as "h..." This has been true since the "keep trailing content" fix several rounds back added visibleWidth(rest) to the budget formula: rest includes the padding, not just genuine trailing prose. It passed every review round and every existing test because nothing checked display-text legibility — only that the hyperlink target was intact and the line never exceeded the pane width, both of which stayed true for a two-cell display just as much as a full one. Fixed by stripping the padding before measuring: trim the line's plain-text trailing spaces, then cut the ANSI-styled line to that same visible width with x/ansi.Truncate (escape-aware, so this only drops the filler and keeps every real byte). Two board follow-ups I logged in earlier rounds turn out to have been this same bug in disguise — "reflowed-paragraph URLs elide down to very few cells" and "two URLs sharing a line starve the second" were both the padding-as-content measurement, not separate deliberate tradeoffs. Neither needs a board entry now; both are fixed here. Two new tests: the shared-line case review round 8 asked for (asserting both targets intact and both displays above a usable width floor), and a strengthened TestBareURLIntact that checks display width directly rather than only target-correctness — the check that should have caught this several rounds ago. Full suite plus go vet clean. Co-Authored-By: Claude Sonnet 5 --- render_test.go | 53 ++++++++++++++ style.go | 185 +++++++++++++++++++++++++++++++------------------ 2 files changed, 172 insertions(+), 66 deletions(-) diff --git a/render_test.go b/render_test.go index c1604e8..986f6bb 100644 --- a/render_test.go +++ b/render_test.go @@ -114,6 +114,19 @@ func TestBareURLIntact(t *testing.T) { if n := strings.Count(line, "\x1b]8;;"+url+"\x07"); n > 1 { t.Errorf("target %s duplicated on line: %q", url, line) } + // The display text should use most of the available + // width, not just technically "fit" — a single bare URL + // alone on its own line, at a width comfortably close to + // its own length, has no reason to elide down to almost + // nothing. Root-cause regression guard: the elision + // budget briefly measured glamour's own trailing + // pad-to-width filler as if it were real content, which + // starved every hyperlinked line's display text down to a + // cell or two while still passing every other check here + // (target intact, one line, width never exceeded). + if w := visibleWidth(m[2]); w < 20 { + t.Errorf("display text for %s suspiciously narrow (%d cells) given ~38 available: %q", url, w, stripANSI(m[2])) + } } } if !found { @@ -188,6 +201,46 @@ func TestBareURLCollisionSafe(t *testing.T) { } } +// Two bare URLs as consecutive continuation lines of one paragraph reflow +// onto a single physical rendered line — the same reflow +// TestBareURLKeepsTrailingContent already relies on, just with a second +// placeholder instead of trailing prose. Both must keep a usable display +// width sharing that line's budget, not have the first URL claim nearly +// all of it and starve the second down to a cell or two. Round-8 review's +// confirmed bug, though the actual root cause it surfaced was bigger: the +// budget calculation was measuring glamour's own trailing pad-to-width +// filler as if it were content, starving the elision budget on almost +// every hyperlinked line regardless of whether it shared its line with +// anything. +func TestBareURLsSharingLineSplitBudgetFairly(t *testing.T) { + raw := "See docs:\nhttps://a.example.test/some/long/path\nhttps://b.example.test/some/long/path\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 2 { + t.Fatalf("want 2 hyperlinks, got %d:\n%s", len(links), stripANSI(out)) + } + const wantTarget0 = "https://a.example.test/some/long/path" + const wantTarget1 = "https://b.example.test/some/long/path" + if links[0][1] != wantTarget0 { + t.Errorf("first target wrong: %q", links[0][1]) + } + if links[1][1] != wantTarget1 { + t.Errorf("second target wrong: %q", links[1][1]) + } + // A "usable" floor, not a precise one: enough to recognize the domain, + // not just "h…". The old, per-URL-not-per-line budget calculation + // gave the second URL 1-2 cells here. + const minUsableDisplay = 8 + for i, m := range links { + if w := visibleWidth(m[2]); w < minUsableDisplay { + t.Errorf("link %d display text too narrow to be usable: %d cells (%q)", i, w, stripANSI(m[2])) + } + } +} + // A bare URL inside a fenced code block is content the user typed verbatim // — stashBareURLs must leave it alone entirely (no OSC 8 hyperlink, no // leaked placeholder). Whatever glamour itself does with fenced content diff --git a/style.go b/style.go index fea0788..9461ea8 100644 --- a/style.go +++ b/style.go @@ -3,6 +3,7 @@ package main import ( "fmt" "regexp" + "strconv" "strings" "github.com/charmbracelet/glamour" @@ -187,21 +188,17 @@ func stashBareURLs(raw string) (string, []string) { blank := strings.TrimSpace(line) == "" prevBlank = blank - if m := fenceLine.FindStringSubmatch(line); m != nil { - c, n := m[1][0], len(m[1]) - switch { - case fenceChar == 0: - fenceChar, fenceLen = c, n - case c == fenceChar && n >= fenceLen: - // Per CommonMark, a fence only closes on a run of the same - // character at least as long as the one that opened it — - // a shorter or different-character run (e.g. a "~~~" line - // inside a "~~~~"-opened block) is just content. - fenceChar, fenceLen = 0, 0 - } - continue - } if fenceChar != 0 { + // Already inside a fence: only a run of the same character, + // at least as long as the one that opened it, closes it — + // anything else (including a run of the other fence + // character) is just content. + if m := fenceLine.FindStringSubmatch(line); m != nil { + c, n := m[1][0], len(m[1]) + if c == fenceChar && n >= fenceLen { + fenceChar, fenceLen = 0, 0 + } + } continue } @@ -215,16 +212,27 @@ func stashBareURLs(raw string) (string, []string) { // block doesn't fall through the guard the first URL was caught by. indent := leadingIndent(line) indented := hasCodeIndent(indent) - switch { - case inIndentedCode: - if !blank && !indented { - inIndentedCode = false - } - case wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line): - inIndentedCode = true - } if inIndentedCode { - continue + if blank || indented { + continue + } + inIndentedCode = false + } else { + // A fence delimiter only opens a fence outside an indented + // code block — a ``` or ~~~ line that's itself part of one is + // just indented content, same as any other line in it, so + // this check has to come after the indented-code state above + // or an indented fence-looking line would open a fence that + // never finds its close and silently reverts every bare URL + // for the rest of the document. + if m := fenceLine.FindStringSubmatch(line); m != nil { + fenceChar, fenceLen = m[1][0], len(m[1]) + continue + } + if wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line) { + inIndentedCode = true + continue + } } m := bareURLLine.FindStringSubmatch(line) @@ -238,7 +246,11 @@ func stashBareURLs(raw string) (string, []string) { // the board convention doesn't produce loose lists, so this hasn't // been worth the complexity. urls = append(urls, m[3]) - lines[i] = m[1] + m[2] + urlPlaceholder(len(urls)-1) + m[4] + // m[4] (trailing whitespace) is dropped, not reinserted: tidy() + // strips it from the final output anyway, and keeping it here + // would shrink restoreBareURLs' elision budget for spaces nobody + // sees. + lines[i] = m[1] + m[2] + urlPlaceholder(len(urls)-1) } return strings.Join(lines, "\n"), urls } @@ -297,60 +309,101 @@ func stripControlBytes(s string) string { return b.String() } +// placeholderFind locates every placeholder on a line at once, capturing +// its index — used by restoreBareURLs to size the shared budget when more +// than one bare URL reflowed onto the same physical line. +var placeholderFind = regexp.MustCompile(`\x1fU(\d+)\x1f`) + // restoreBareURLs swaps each placeholder back for its real URL, wrapped in // an OSC 8 hyperlink and styled like glamour's own Link (colorLink, -// underlined). The visible text is elided to fit whatever width remains on -// its line — measured from the actual rendered prefix and suffix (bullet, -// indent, nesting, and anything glamour reflowed onto the same physical -// line after the URL, e.g. a soft-wrapped paragraph continuation), not -// guessed — but the hyperlink target always carries the full, untruncated -// URL, so the link opens the right place regardless of how much of it is -// shown. Each URL has its own index-keyed placeholder, so two URLs that -// happen to elide to identical visible text can never cross-link — there's -// nothing to search for and collide on. If there's no room at all (a -// pathologically narrow pane with a deep indent), the URL is dropped -// rather than drawn — an over-width line would break the one invariant -// every other line in this renderer holds. +// underlined). The unit of work is the rendered LINE, not the individual +// URL: every placeholder on a line shares one width budget, computed once +// from the line with all placeholders removed and then split evenly across +// however many there are. Processing URLs independently (each measuring +// its budget against a line that already had earlier URLs' full-size +// hyperlinks substituted in) let the first URL on a shared line claim +// nearly the whole width and starve the rest down to a cell or two — width +// invariant intact, but a real bug: two bare URLs as consecutive +// continuation lines of one board item reflow onto a single physical line +// exactly like this. The hyperlink target always carries the full, +// untruncated URL regardless of what its display text elides to. Each URL +// has its own index-keyed placeholder, so two URLs that happen to elide to +// identical visible text can never cross-link — there's nothing to search +// for and collide on. If a URL's share of the line's budget is under 1 +// cell, it's dropped rather than drawn — an over-width line would break +// the one invariant every other line in this renderer holds. func restoreBareURLs(rendered string, urls []string, width int) string { if len(urls) == 0 { return rendered } lines := strings.Split(rendered, "\n") lr, lg, lb := hexToRGB(colorLink) // loop-invariant - for i, url := range urls { - ph := urlPlaceholder(i) - for li, line := range lines { - idx := strings.Index(line, ph) - if idx < 0 { - continue - } - prefix := line[:idx] - rest := line[idx+len(ph):] - budget := width - visibleWidth(prefix) - visibleWidth(rest) - if budget < 1 { - lines[li] = prefix + rest - break + for li, line := range lines { + matches := placeholderFind.FindAllStringSubmatchIndex(line, -1) + if matches == nil { + continue + } + + // glamour's MarginWriter pads every short block line with + // trailing spaces out to the full block width (see styleConfig's + // doc comment). Real trailing content — what the "keep trailing + // content" fix this comment replaces was protecting — only ever + // needs the width AFTER that padding, never the padding itself. + // Left uncorrected, the padding reads as content already filling + // the line and starves the elision budget to almost nothing on + // every hyperlinked line, regardless of how much real room there + // actually is — this was silent since every existing test checks + // the hyperlink target and the width invariant, neither of which + // notices a `budget` this small; the display text is still + // technically "intact" and "within width", just useless. Strip + // the padding by measuring the plain-text line with real + // trailing spaces trimmed, then cutting the ANSI-styled line to + // that same visible width — x/ansi.Truncate is escape-aware, so + // this keeps every SGR code and every placeholder intact and + // only drops the trailing filler. + realWidth := visibleWidth(strings.TrimRight(stripANSI(line), " ")) + line = xansi.Truncate(line, realWidth, "") + matches = placeholderFind.FindAllStringSubmatchIndex(line, -1) + if matches == nil { + continue // placeholders are never whitespace; stay safe anyway + } + + available := width - visibleWidth(placeholderFind.ReplaceAllString(line, "")) + share := available / len(matches) + + var out strings.Builder + cursor := 0 + for _, m := range matches { + start, end := m[0], m[1] + out.WriteString(line[cursor:start]) + cursor = end + idx, err := strconv.Atoi(line[m[2]:m[3]]) + if err != nil || idx < 0 || idx >= len(urls) || share < 1 { + continue // drop this URL: no room, or a malformed index } - // xansi.Truncate, not go-runewidth: it's grapheme-cluster aware - // (a VS16 emoji presentation sequence is one cluster but two - // runes) and it's the same measurement visibleWidth uses to - // enforce the pane-width invariant. Measuring the budget with - // one metric and building the display text with a different - // one is exactly how that invariant would quietly break again. - display := xansi.Truncate(stripControlBytes(url), budget, "…") + url := urls[idx] + // xansi.Truncate, not go-runewidth: it's grapheme-cluster + // aware (a VS16 emoji presentation sequence is one cluster + // but two runes) and it's the same measurement visibleWidth + // uses to enforce the pane-width invariant. Measuring the + // budget with one metric and building the display text with + // a different one is exactly how that invariant would + // quietly break again. + display := xansi.Truncate(stripControlBytes(url), share, "…") // Raw ANSI, not termenv.String: termenv.String binds to - // termenv's auto-detected package-global Output profile, which - // this codebase deliberately overrides everywhere else — the - // glamour renderer is forced to termenv.TrueColor below, and - // diff.go writes 38;2;r;g;b by hand for the same reason - // ("piped/degraded profiles were how glow washed out"). Under - // a degraded profile termenv.String would silently drop the - // styling and this would be the one link on screen that isn't - // truecolor. + // termenv's auto-detected package-global Output profile, + // which this codebase deliberately overrides everywhere else + // — the glamour renderer is forced to termenv.TrueColor + // below, and diff.go writes 38;2;r;g;b by hand for the same + // reason ("piped/degraded profiles were how glow washed + // out"). Under a degraded profile termenv.String would + // silently drop the styling and this would be the one link + // on screen that isn't truecolor. styled := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm%s\x1b[0m", lr, lg, lb, display) - lines[li] = prefix + hyperlink(url, styled) + rest - break + out.WriteString(hyperlink(url, styled)) } + out.WriteString(line[cursor:]) + lines[li] = out.String() } return strings.Join(lines, "\n") } From d80adfc270994a60e87b027fd1cc7f0ae96c9479 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 12:26:52 -0400 Subject: [PATCH 10/11] fix review: fence check skipped entirely on an indented-code-close line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 review found one real bug, traced and confirmed exactly as reported. The fence-open check lived in the else branch of the inIndentedCode if/else. The line that CLOSES an indented code block (non-blank, non-indented) takes the if branch, clears the flag, and — because it's if/else, not sequential — never reaches the else branch's fence check on that same line. So a fence immediately following an indented code block never opens: the URL inside it gets hyperlinked, exactly what fence protection exists to prevent. Second-order effect confirmed by tracing it through: the line meant to CLOSE that fence then hits the (still-untouched) else branch fresh, and since fenceChar is still 0 it reads as an OPEN instead of a close — a phantom fence starts there and never finds a match, silently disabling the whole fix for every real bare URL for the rest of the document. Fixed by hoisting the fence check out of the else and running it unconditionally after the inIndentedCode block, rather than as an alternative to it. indented is false by construction on the line that just closed a code block (that's what let it fall through in the first place), so the indented-code-open check right after can't misfire on that same line. Also softened two doc comments (renderMarkdown's linkify parameter, runStatic) that oversold what the non-TTY path actually guarantees — a short URL comes through plain and intact, but one long enough to still need wrapping at the fallback width hits glamour's original hard break, same as before this fix. Round 9's other two notes (uneven shared-line budget split, x/ansi width metric now used everywhere not just hyperlinked lines) were both explicitly "fine to leave" / "harmless" from the reviewer — no code change. New test reproduces the exact sequence from the review: intro text, indented code, fence, URL, fence, blank, then a real bare URL that must still be hyperlinked — asserting both that the fenced URL stays untouched and that the later real URL doesn't fall victim to the phantom fence. Full suite plus go vet clean. Co-Authored-By: Claude Sonnet 5 --- main.go | 7 +++++-- render_test.go | 32 ++++++++++++++++++++++++++++ style.go | 57 +++++++++++++++++++++++++++++++------------------- 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/main.go b/main.go index 3045308..c7e0266 100644 --- a/main.go +++ b/main.go @@ -100,8 +100,11 @@ func main() { // terminal width (minus 2) when stdout is a TTY, else 80. Bare-URL OSC 8 // hyperlinking (and the display-text elision that comes with it) is skipped // whenever stdout isn't a TTY: whatever's on the other end of the pipe — -// grep, an editor, a CI log — can't render the escape sequence anyway, so a -// full, plain, un-elided URL serves it better than a shortened one. +// grep, an editor, a CI log — can't render the escape sequence anyway. A +// URL short enough to fit the fallback width comes through plain and +// intact; one long enough to still need wrapping hits glamour's original +// hard break instead (see renderMarkdown's linkify doc comment) — out of +// scope for this fix, just not silently hidden behind an OSC 8 escape. func runStatic(args []string) int { path := defaultBoardPath() if len(args) > 0 { diff --git a/render_test.go b/render_test.go index 986f6bb..86c7560 100644 --- a/render_test.go +++ b/render_test.go @@ -484,6 +484,38 @@ func TestBareURLIndentedCodeBlockMultiLine(t *testing.T) { } } +// The line that CLOSES an indented code block must still be checked as a +// possible fence opener itself. Round-9 review's confirmed bug: the fence +// check lived in an else-branch the closing line skipped entirely, so a +// fence right after an indented block never opened, the URL inside it got +// hyperlinked (exactly what fence protection exists to prevent), and the +// line meant to CLOSE that fence opened a phantom one instead — silently +// disabling the whole fix for every real bare URL for the rest of the +// document. +func TestBareURLFenceAfterIndentedCodeBlock(t *testing.T) { + raw := "intro\n\n indented code\n\n```\nhttps://example.test/long-url-inside-a-fence-after-code\n```\n\n" + + "- https://example.test/real-url-after-the-fence-must-still-be-fixed\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + for _, m := range links { + if m[1] == "https://example.test/long-url-inside-a-fence-after-code" { + t.Errorf("URL inside the fence got hyperlinked — fence never opened:\n%s", stripANSI(out)) + } + } + found := false + for _, m := range links { + if m[1] == "https://example.test/real-url-after-the-fence-must-still-be-fixed" { + found = true + } + } + if !found { + t.Errorf("real bare URL after the fence wasn't hyperlinked — phantom fence never closed:\n%s", stripANSI(out)) + } +} + // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40, true) diff --git a/style.go b/style.go index 9461ea8..17eede8 100644 --- a/style.go +++ b/style.go @@ -217,22 +217,35 @@ func stashBareURLs(raw string) (string, []string) { continue } inIndentedCode = false - } else { - // A fence delimiter only opens a fence outside an indented - // code block — a ``` or ~~~ line that's itself part of one is - // just indented content, same as any other line in it, so - // this check has to come after the indented-code state above - // or an indented fence-looking line would open a fence that - // never finds its close and silently reverts every bare URL - // for the rest of the document. - if m := fenceLine.FindStringSubmatch(line); m != nil { - fenceChar, fenceLen = m[1][0], len(m[1]) - continue - } - if wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line) { - inIndentedCode = true - continue - } + // Falls through to the fence/open checks below rather than + // an else-branch skip: the line that closes an indented code + // block (non-blank, non-indented — the case that just fell + // through above) still needs to be checked as a possible + // fence opener itself. Missing that let a ``` line closing + // an indented block skip fence-recognition entirely, so the + // fence it should have opened never did, the bare URL right + // after it went unprotected, and the *next* fence-looking + // line (meant to close that fence) opened a phantom one + // instead — silently reverting every real bare URL for the + // rest of the document. `indented` is false here by + // construction (that's what let this branch fall through at + // all), so the indented-code-open check below cannot misfire + // on this same line. + } + // A fence delimiter only opens a fence outside an indented code + // block — a ``` or ~~~ line that's itself part of one is just + // indented content, same as any other line in it, so this check + // has to come after the indented-code state above or an indented + // fence-looking line would open a fence that never finds its + // close and silently reverts every bare URL for the rest of the + // document. + if m := fenceLine.FindStringSubmatch(line); m != nil { + fenceChar, fenceLen = m[1][0], len(m[1]) + continue + } + if wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line) { + inIndentedCode = true + continue } m := bareURLLine.FindStringSubmatch(line) @@ -416,11 +429,13 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // linkify controls the bare-URL OSC 8 hyperlink path: true for the // interactive TUI, where the escape sequence is invisible to the terminal // and only the (possibly elided) display text is shown. false skips -// stashBareURLs entirely, so bare URLs render as plain, complete, -// un-elided text — for a non-TTY consumer (runStatic piped to a file, -// grep, a CI log) whose stdout is not a terminal that would render the -// hyperlink at all; there, showing the full plain URL beats hiding it -// behind an escape sequence that reader can't see. +// stashBareURLs entirely — for a non-TTY consumer (runStatic piped to a +// file, grep, a CI log) whose stdout is not a terminal that would render +// the hyperlink at all, an OSC 8 escape it can't see is worse than plain +// text. It isn't a full guarantee, though: a URL long enough to still hit +// glamour's own hard word-wrap on that path renders un-elided but still +// split mid-URL, same as before this fix — this only helps the common +// case, a URL short enough to need no wrap at the non-TTY fallback width. func renderMarkdown(raw string, width int, linkify bool) (string, error) { if width < 10 { width = 10 From 22a512b888c6a5ac922086e343983e08ba9cbd18 Mon Sep 17 00:00:00 2001 From: Than Tibbetts Date: Fri, 7 Aug 2026 13:05:25 -0400 Subject: [PATCH 11/11] fix review: neutralize control byte in raw input, trim comment archaeology MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review found one real gap, one worth taking. The one real gap: restoreBareURLs searches rendered output for the \x1f-delimited placeholder, but nothing stripped \x1f from the raw board text on the way in. A line already containing that byte (pasted tool output, the same threat model stripControlBytes exists for) could prefix-match a real placeholder and get substituted with the wrong URL's hyperlink. \x1f has no legitimate use in board text, so stashBareURLs now drops it from the input unconditionally, same directness as the display-text sanitization from an earlier round. Also trimmed review-round citations out of several comments ("Round-7 review's confirmed bug", "PR #18's confirmed bug", and similar) per the same round's note that they read as review history rather than code documentation, and will mean less to a reader once that context is gone. Kept every behavioral explanation; only dropped the provenance. Two smaller notes from this round — uneven budget split on a shared line, a dropped URL collapsing its line to nothing rather than showing a single character — were explicitly framed as notes rather than blockers, and are left as-is. New test: a board line containing a raw \x1f byte must not corrupt an adjacent real URL's hyperlink. Full suite plus go vet clean. Co-Authored-By: Claude Sonnet 5 --- render_test.go | 67 +++++++++++++++++++++++++++++--------------------- style.go | 34 ++++++++++++++----------- 2 files changed, 58 insertions(+), 43 deletions(-) diff --git a/render_test.go b/render_test.go index 86c7560..1d6697b 100644 --- a/render_test.go +++ b/render_test.go @@ -180,8 +180,9 @@ func TestBareURLOrderedListMarker(t *testing.T) { } // Two bare URLs that elide to identical visible text must still each get -// their own correct hyperlink target — no cross-linking (PR #18's confirmed -// collision bug: a global text search re-found the first occurrence). +// their own correct hyperlink target — no cross-linking. (A global text +// search that re-finds the first occurrence is the failure mode this +// guards against; index-keyed placeholders avoid it structurally.) func TestBareURLCollisionSafe(t *testing.T) { raw := "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n" + "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n" @@ -201,17 +202,29 @@ func TestBareURLCollisionSafe(t *testing.T) { } } +// A board line that already contains \x1f (a pasted-tool-output edge case) +// must not prefix-match a real placeholder and substitute the wrong URL. +func TestBareURLSourceControlCharNeutralized(t *testing.T) { + raw := "- weird\x1fbytes here, not a url\n- https://example.test/the-real-target-url\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if strings.Contains(out, "\x1f") { + t.Errorf("raw \\x1f from the source survived into output: %q", out) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 1 || links[0][1] != "https://example.test/the-real-target-url" { + t.Errorf("wrong or missing link, want exactly the real URL: %v", links) + } +} + // Two bare URLs as consecutive continuation lines of one paragraph reflow // onto a single physical rendered line — the same reflow // TestBareURLKeepsTrailingContent already relies on, just with a second // placeholder instead of trailing prose. Both must keep a usable display // width sharing that line's budget, not have the first URL claim nearly -// all of it and starve the second down to a cell or two. Round-8 review's -// confirmed bug, though the actual root cause it surfaced was bigger: the -// budget calculation was measuring glamour's own trailing pad-to-width -// filler as if it were content, starving the elision budget on almost -// every hyperlinked line regardless of whether it shared its line with -// anything. +// all of it and starve the second down to a cell or two. func TestBareURLsSharingLineSplitBudgetFairly(t *testing.T) { raw := "See docs:\nhttps://a.example.test/some/long/path\nhttps://b.example.test/some/long/path\n" out, err := renderMarkdown(raw, 40, true) @@ -315,9 +328,8 @@ func TestBareURLDisplayTextNoEscapeInjection(t *testing.T) { } // A nested list item's budget must account for its actual indent, not a -// guessed constant (PR #18's confirmed bug: a hard-coded top-level-bullet -// reserve overshot on nested items and re-broke the line it was meant to -// fix). +// guessed constant — a hard-coded top-level-bullet reserve would overshoot +// on nested items and re-break the line it's meant to fix. func TestBareURLNestedIndentBudget(t *testing.T) { raw := "- Parent\n - https://example.test/nested-item-url-thats-long-enough-to-need-eliding\n" out, err := renderMarkdown(raw, 30, true) @@ -370,12 +382,12 @@ func TestChangedLinesSeesURLTargetChange(t *testing.T) { } } -// The gate PR #18 could never verify: bubbletea's renderer and lipgloss's -// MaxWidth both truncate the final frame to the pane width using -// charmbracelet/x/ansi, which understands OSC 8 and only cuts visible -// cells. If that ever regresses to an OSC-blind truncator, a hyperlinked -// line gets cut inside the escape and the display text (which comes after -// it) is dropped outright — this pins the assumption directly. +// The load-bearing assumption behind this whole approach: bubbletea's +// renderer and lipgloss's MaxWidth both truncate the final frame to the +// pane width using charmbracelet/x/ansi, which understands OSC 8 and only +// cuts visible cells. If that ever regresses to an OSC-blind truncator, a +// hyperlinked line gets cut inside the escape and the display text (which +// comes after it) is dropped outright — this pins the assumption directly. func TestHyperlinkSurvivesDownstreamTruncation(t *testing.T) { target := "https://example.test/downstream-truncation-gate" line := "prefix " + hyperlink(target, "short") @@ -466,10 +478,10 @@ func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { // An indented code block is a block, not a per-line property: a second URL // deeper in the same block (whose own immediate predecessor isn't blank) -// must be protected exactly like the first one, whose predecessor is. -// Round-7 review's confirmed bug: a per-line wasPrevBlank check caught the -// first URL and missed the second, so two URLs in the same verbatim block -// rendered differently from each other. +// must be protected exactly like the first one, whose predecessor is. A +// per-line wasPrevBlank check alone catches the first URL and misses the +// second, so two URLs in the same verbatim block would render differently +// from each other. func TestBareURLIndentedCodeBlockMultiLine(t *testing.T) { raw := "Run these:\n\n https://example.test/first-verbatim-command\n https://example.test/second-verbatim-command\n" out, err := renderMarkdown(raw, 40, true) @@ -485,13 +497,12 @@ func TestBareURLIndentedCodeBlockMultiLine(t *testing.T) { } // The line that CLOSES an indented code block must still be checked as a -// possible fence opener itself. Round-9 review's confirmed bug: the fence -// check lived in an else-branch the closing line skipped entirely, so a -// fence right after an indented block never opened, the URL inside it got -// hyperlinked (exactly what fence protection exists to prevent), and the -// line meant to CLOSE that fence opened a phantom one instead — silently -// disabling the whole fix for every real bare URL for the rest of the -// document. +// possible fence opener itself — an else-branch skip there would let a +// fence right after an indented block never open, hyperlinking the URL +// inside it (exactly what fence protection exists to prevent) and, worse, +// leave the line meant to CLOSE that fence opening a phantom one instead — +// silently disabling the whole fix for every real bare URL for the rest of +// the document. func TestBareURLFenceAfterIndentedCodeBlock(t *testing.T) { raw := "intro\n\n indented code\n\n```\nhttps://example.test/long-url-inside-a-fence-after-code\n```\n\n" + "- https://example.test/real-url-after-the-fence-must-still-be-fixed\n" diff --git a/style.go b/style.go index 17eede8..0649dd5 100644 --- a/style.go +++ b/style.go @@ -177,6 +177,12 @@ func hasCodeIndent(indent string) bool { // such lines out of that path entirely. restoreBareURLs puts the real, // styled, hyperlinked URL back after rendering. func stashBareURLs(raw string) (string, []string) { + // A board line already containing \x1f — a pasted-tool-output edge + // case, the same threat model stripControlBytes exists for — would + // otherwise prefix-match a real placeholder in restoreBareURLs' search + // and substitute the wrong URL. \x1f has no legitimate use in board + // text, so it's always safe to drop from the input outright. + raw = strings.ReplaceAll(raw, "\x1f", "") lines := strings.Split(raw, "\n") var urls []string var fenceChar byte @@ -359,21 +365,19 @@ func restoreBareURLs(rendered string, urls []string, width int) string { // glamour's MarginWriter pads every short block line with // trailing spaces out to the full block width (see styleConfig's - // doc comment). Real trailing content — what the "keep trailing - // content" fix this comment replaces was protecting — only ever - // needs the width AFTER that padding, never the padding itself. - // Left uncorrected, the padding reads as content already filling - // the line and starves the elision budget to almost nothing on - // every hyperlinked line, regardless of how much real room there - // actually is — this was silent since every existing test checks - // the hyperlink target and the width invariant, neither of which - // notices a `budget` this small; the display text is still - // technically "intact" and "within width", just useless. Strip - // the padding by measuring the plain-text line with real - // trailing spaces trimmed, then cutting the ANSI-styled line to - // that same visible width — x/ansi.Truncate is escape-aware, so - // this keeps every SGR code and every placeholder intact and - // only drops the trailing filler. + // doc comment). Real trailing content on the line only ever needs + // the width AFTER that padding, never the padding itself — left + // uncorrected, the padding reads as content already filling the + // line and starves the elision budget to almost nothing, + // regardless of how much real room there actually is. The + // hyperlink target and the width invariant both stay technically + // correct at any budget, including a useless one, so this needs + // its own check rather than relying on those. Strip the padding + // by measuring the plain-text line with real trailing spaces + // trimmed, then cutting the ANSI-styled line to that same visible + // width — x/ansi.Truncate is escape-aware, so this keeps every + // SGR code and every placeholder intact and only drops the + // trailing filler. realWidth := visibleWidth(strings.TrimRight(stripANSI(line), " ")) line = xansi.Truncate(line, realWidth, "") matches = placeholderFind.FindAllStringSubmatchIndex(line, -1)