Upgrade glamour to fix hyperlinks (OSC8)

[heaths]

Fixes #188

[heaths]

This is stacked atop #280 so this either needs to be split into a separate PR atop trunk or turned into a stack / that PR merged first. I'm building up my own replacement module with fixes I need for extensions we use in workflows and custom agent instructions.

[heaths]

This is part of a stack but not showing here because stacks aren't enabled for this repo it seems:

Syncing PRs ...
✓ PR #280 (issue279) — Open
✓ PR #281 (issue188) — Open
⚠ Stacked PRs are not enabled for this repository

✓ Branches synced
  Stacked on trunk (450618a)

I can split these later if needs be, but stacks at least make it easier to rebase --onto (automatic vs. manual).

[BagToad]

I would prefer this dependency bump on its own and not stacked with the unrelated change please. If you have the time; proof that it doesn't break tests in cli/cli would also be an asset to decrease the review burden. If it does break tests, some investigation into why is in order as well.

I don't understand the "replacement module" comment, sorry. If that's something you meant for us to action, can you expand on that?

[heaths]

I would prefer this dependency bump on its own and not stacked with the unrelated change please.

@BagToad if you're open to taking it, I can unstack them. I mainly did that so I could simply build against the head of the stack against my fork. Less for the stack and more for the natural HEAD so I can build against all changes.

I'm making a similar change to the cli right now for cli/cli#3718. There were a few expected \e[0m -> \e[m conversions (effectively equivalent) but also a couple other changes with the newer version of lipgloss that you already used in some places (your go.mod for gh uses both the v2 and v0 versions of lipgloss.

But how would this change impact the cli directly? I see you are using x/markdown. I should probably split this PR out and use a replacement module (literally "replace" in the go.mod e.g., https://github.com/heaths/gh-minimize/blob/710a1ab366b560af52e84846379a81df05ed00b5/go.mod#L14) there as well.

[heaths]

The lint issue is that golangci-lint timed out or otherwise couldn't connect to the schema. Odd thing for it to fail on; however, I can't rerun the failed check myself. I imagine it'd pass on a subsequent run. The file exists—I just checked in.

[heaths]

Since I can't rerun the failed test I simply amended the commit and did a force push.

[BagToad]

@heaths sorry for the long delay on this. Definitely do want to fix this in cli/cli.

@BagToad if you're open to taking it, I can unstack them. I mainly did that so I could simply build against the head of the stack against my fork. Less for the stack and more for the natural HEAD so I can build against all changes.

I would like to take this change, yes. If we could unstack the changes I think we can merge it.

[williammartin]

I think that if we take the glamour v2 upgrade here, we need to figure out some way in the markdown renderer to avoid emitting the OSC8 links when rendering markdown in the non-interactive mode. Think without any work they will still be included, which probably isn't what users expect.

[heaths]

If we could unstack the changes I think we can merge it.

@BagToad done.

@williammartin the OSC8 characters aren't there when piping to, say, more:

Or did you just mean markdown in general? That's with my changes i.e., go run ./cmd/gh pr -R cli/cli view 3519 --comments | more, but gh pr -R cli/cli view 3519 using the latest gh version published also shows this:

In fact, I just redirected both commands' output to tmp files and diff'd them. No differences at all.

The new glamour takes into account whether the output is a TTY just like the old one did.

[williammartin]

The new glamour takes into account whether the output is a TTY just like the old one did.

I don't think that's the case. What you're seeing with gh pr -R cli/cli view 3519 | more is that pr view is detecting whether the terminal is a TTY and determining whether to render markdown or not: https://github.com/cli/cli/blob/581392e6aea8df94b1fde390d920a4b0d99c12a2/pkg/cmd/pr/view/view.go#L134-L143

As a comparison, skill preview does not do this (gh built against this branch):

➜  ./bin/gh skill preview anthropics/skills academy-guide | cat -v | grep http
All content lives on ^[]8;id=7132715;https://academy.claude.com^GClaude Academy^[]8;;^G ^[]8;id=7132715;https://academy.claude.com^Ghttps://academy.claude.com^[]8;;^G,
surface: ^[]8;id=2837610102;https://academy.claude.com/claude^GClaude^[]8;;^G ^[]8;id=2837610102;https://academy.claude.com/claude^Ghttps://academy.claude.com/claude^[]8;;^G,

That said, I've probably changed my mind anyway. It just seems that we should fix our call sites to not markdown.Render when there is no terminal attached (at least skills preview, project view and agent-task view --log and agent-task create --follow). Rendering markdown on the machine readable output is probably the incorrect behaviour here.

[heaths]

That seems like a failure of the agent harness, though. Either it emulates a TTY (which is funny, since TTYs we know are emulations) and handles VT sequences e.g., VSCode's integrated terminal, or it attaches output streams that are not a TTY. Glamour et. al. all consistently check TTY the same way.

we should fix our call sites to not markdown.Render when there is no terminal attached

Yeah, seems like a better approach overall. markdown.Render is still going to reformat—reflow if nothing else—and there's no reason to waste cycles.

Do you want me to do that in cli/cli#14148 or treat that as a separate change?

[heaths]

@williammartin FWIW, I was looking into some way to avoid rendering VT sequences—turns out Glamour doesn't care about TTY at all, just renders text given—and realized it's not only hyperlinks that force VT sequences. Some other elements do to, like positioning, bold, etc. Sure enough, even the current release of gh with the old glamour does this too:

So, yes, seems just not calling markdown.Render for non-TTYs might be a good idea.

[williammartin]

Do you want me to do that in cli/cli#14148 or treat that as a separate change?

No particular opinion. Nice if we can cover it in one release, but it doesn't need to be a single PR for me.

This is probably ready for review right? There's nothing else we think is required in go-gh?

[Copilot]

🟡 Changes recommended

It pins versions with a known rendering panic, mishandles relative OSC 8 targets, and lacks regression coverage for wrapped hyperlinks.

3 open findings

• Use patched Glamour and Lip Gloss releases to avoid writer-close panic · New
• BaseURL is not applied to OSC 8 link targets · New
• Add regression test for wrapped links preserving OSC 8 destinations · New

What changed in this PR

Upgrades Glamour to v2 to add OSC 8 hyperlink support.

Changes:

• Migrates Glamour and Lip Gloss to v2 import paths.
• Updates dependencies and rendering expectations.
• Preserves accessibility styling compatibility.

File	Description
go.mod	Updates Go and terminal-rendering dependencies.
go.sum	Refreshes dependency checksums.
pkg/​markdown/​markdown.go	Migrates Markdown rendering to Glamour v2.
pkg/​markdown/​markdown_test.go	Updates ANSI output expectations.
pkg/​text/​text.go	Migrates width calculation to Lip Gloss v2.
pkg/​x/​markdown/​accessibility.go	Migrates accessible styles to Glamour v2.
pkg/​x/​markdown/​accessibility_test.go	Updates accessibility test imports.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[williammartin]

Hmmm, the most painful thing about this PR is that since this is a major version bump of glamour, and glamour package types are exposed on exported function signatures, this is a breaking change for anyone who references the glamour type themselves.

For example, anyone who does something like cli/cli here: https://github.com/cli/cli/blob/ec5b512045db67e5a2a4ff4a1b02660b2fb24390/pkg/markdown/markdown.go

Still, I'm probably inclined to say we should just make it a breaking change and apologise if anyone gets into problems where they integrate with go-gh but really need the old version of glamour through the rest of their codebase.

WDYT @heaths @BagToad

[williammartin]

Re: my previous comment.

Public usage of the package is quite small, so we might just be able to go round opening PRs to fix it for people.

On the other hand, there might be private repos we don't know about. If we're worried then we should consider deprecating the current package and making a new one beside it with types we own that adapts glamour/v2. This would be safer in the long run too. Since there aren't too many integrators there also won't be lots of people that don't realise they need to upgrade. This would probably be my preferred option just to avoid the same thing happening in future.

[heaths]

The fact I didn't have to change any WithOptions in cli/cli seems a testament to not needing a change since Go is duck typed. But I'll compare v1 against v2. If anything, maybe we could add v1-compatible WithOptions as needed.

Maybe it would also be better right now in this PR to change that WithOptions arg to use our own duck type-compatible WithOptions and avoid exposing glamour/v2 directly. Thoughts?

[heaths]

Unfortunately, I found a bug adding a test case: charmbracelet/glamour#637

As long as no one actually uses WithBaseURL, they are no worse off; though, I just checked, and indeed gh does: https://github.com/cli/cli/blob/ec5b512045db67e5a2a4ff4a1b02660b2fb24390/pkg/cmd/repo/view/view.go#L190

Probably should hold off merging this until I can get a fix upstream or at least figure out a way to work around it. The behavior in glamour/v2 definitely seems incorrect because the navigable href is wrong and, well, unnavigable.

[williammartin]

The fact I didn't have to change any WithOptions in cli/cli seems a testament to not needing a change since Go is duck typed. But I'll compare v1 against v2. If anything, maybe we could add v1-compatible WithOptions as needed.

Maybe it would also be better right now in this PR to change that WithOptions arg to use our own duck type-compatible WithOptions and avoid exposing glamour/v2 directly. Thoughts?

Unfortunately I don't think this is a duck typing thing, otherwise your suggestion would work. I think the reason you didn't need to change functions in cli/cli is that the exact same type names exist in glamour 0.10.0 and v2. So when you changed the package import to v2 all the types just changed.

That you needed to change the import is the breaking change.

There's also an issue with indirect dependency compatibility on github.com/charmbracelet/x/cellbuf which I created a repro for here. This would be a problem for anyone still on bubbletea v1 ecosystem, which I think is probably quite a lot. The issue is that lipgloss/v1, bubbletea/v1, huh/v1 or glamour 0.x depend on x/cellbuf which depends on x/ansi, but the version of x/cellbuf requires an incompatible x/ansi than glamour/v2 depends on.

Here's the dep graph showing the issue:

I think fixing it takes one of two paths depending on whether we make a breaking change to glamour/v2 or whether we deprecate our existing markdown package and create a new one side by side. In the first case, we'd need to take a direct dependency on cellbuf in a tools.go (or something like that) just to pin it:

In the second case, we'd still have cellbuf as an indirect dependency, so we can also pin it:

If we don't take either of these options, our consumers can run go get github.com/charmbracelet/x/cellbuf@v0.0.15 to fix it themselves, but it's just another breaking change in a non-major version bump 😬

[heaths]

It also raises this module's go directive to 1.25.8.

That's still semver-compatible with 1.25.0, which is what go-gh declares now. Is that so bad? For a >= 1.x, even a >= 1.25 is semver-compatible but I appreciate not forcing a minor upgrade.

github.com/charmbracelet/x/cellbuf@v0.0.15

We could take an explicit dependency on it. 0.0.15 is semver-compatible with 0.0.13. While we see so many devs afraid to support a 1.x in Rust, I'm sad to see it seemingly increasing in Go as well; however, at least they are only rev'ing the patch version and not the minor because a hypothetical 0.13.0 is not semver-compatible with 0.15.0.

[heaths]

Unfortunately, I found a bug adding a test case: charmbracelet/glamour#637

Once charmbracelet/glamour#639 is merged, it's just a matter of the cellbuf dependency. If we could force a dependency—even though it's transitive—perhaps we could upgrade people automatically.