Skip to content

fix(vt): store OSC 8 hyperlink params and uri in correct fields#868

Open
paultyng wants to merge 2 commits into
charmbracelet:mainfrom
paultyng:osc-fix
Open

fix(vt): store OSC 8 hyperlink params and uri in correct fields#868
paultyng wants to merge 2 commits into
charmbracelet:mainfrom
paultyng:osc-fix

Conversation

@paultyng

Copy link
Copy Markdown

Summary

handleHyperlink stores the OSC 8 params slot into Link.URL and the URI slot into Link.Params. The two fields are swapped.

Problem

OSC 8 wire format:

ESC ] 8 ; <params> ; <uri> ST

After bytes.Split(data, ';') on valid 8;<params>;<uri> input, parts[1] is <params> and parts[2] is <uri>. The handler does the opposite.

Impact

Any consumer that round-trips cells back to ANSI via ansi.SetHyperlink(cell.Link.URL, cell.Link.Params) re-emits with the slots swapped. The common case ESC]8;;<uri>ST becomes ESC]8;<uri>;ST — URI in the params slot, URI slot empty. xterm.js's parser rejects it via the empty-URI branch; the styled text renders but is not clickable.

In-repo: cellbuf follows this exact pattern at
screen.go:734,
wrap.go:90,
pen.go:66,
writer.go:126,
so any cellbuf consumer whose cells were populated by vt inherits the swap.

Common emitters of OSC 8 whose output would round-trip incorrectly:
GNU coreutils ls --hyperlink,
ripgrep,
fd.

Changes

vt/osc.go: swap the two assignments.
vt/osc_test.go (new): two subtests via Emulator.CellAt(0,0).Link,
covering empty-params and populated-params forms. Both fail on main
with field-value assertions surfacing the swap; pass with the fix.

Testing

go test -race ./vt/... passes locally; CI covers Linux/macOS/Windows.

@paultyng paultyng marked this pull request as ready for review May 22, 2026 12:57
@paultyng paultyng closed this May 26, 2026
@paultyng paultyng reopened this May 27, 2026
@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.90%. Comparing base (798e623) to head (a33b01e).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
- Coverage   38.14%   36.90%   -1.25%     
==========================================
  Files         196       21     -175     
  Lines       16750     1837   -14913     
==========================================
- Hits         6390      678    -5712     
+ Misses       9959     1070    -8889     
+ Partials      401       89     -312     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paultyng added 2 commits June 18, 2026 08:35
handleHyperlink stored parts[1] (the params slot) into Link.URL
and parts[2] (the uri slot) into Link.Params. The OSC 8 wire format
per egmontkov's de facto spec
(https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda)
is `ESC ] 8 ; <params> ; <uri> ST` — params first, uri second — so
the assignments were swapped.

Symptom downstream: every OSC 8 hyperlink that round-trips through
Buffer.Render() (e.g. a vscreen-style snapshot/replay) emits bytes
with the two fields back in the OPPOSITE slots from their original
input. When xterm.js parses those re-emitted bytes its setHyperlink
falls through to the empty-uri rejection branch and never registers
the link, so the styled text renders but the hover/click handlers
never fire. Same defect affects any consumer that re-renders cells
back to ANSI.

Adds osc_test.go with two subtests:
  - empty params, populated uri (the common case — gh, claude,
    coreutils, testagent's /link all emit this form)
  - populated params and uri (id=...:tag=... form)

Both subtests assert via Emulator.CellAt(0,0).Link that URL and
Params land in their spec slots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant