Skip to content

fix(terminal): terminate HTTP links on CJK brackets and whitespace - #10573

Closed
innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/terminal-url-cjk-terminators
Closed

innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/terminal-url-cjk-terminators

Conversation

@innocarpe

@innocarpe innocarpe commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Stop terminal HTTP links at CJK brackets, punctuation, and Unicode whitespace instead of swallowing adjacent prose.
  • Reject non-ASCII characters while scanning the URL authority so missed boundaries cannot be folded into an IDNA hostname.
  • Keep non-ASCII document paths and queries valid after the authority boundary.

Fixes #10571.

Why

Terminal URL extraction previously recognized only ASCII boundaries. Text such as a Japanese annotation directly after a GitHub URL could be included in the link, producing a percent-encoded or IDNA-transformed destination instead of the URL the user intended.

Scope and design

  • The existing URL extraction and terminal hit-testing flow on main is preserved.
  • Boundary predicates are isolated in terminal-http-url-boundary.ts so the scanner remains reviewable and below the module line budget.
  • Unicode White_Space, punctuation, and symbol properties cover new scripts without an open-ended glyph list.
  • Authority scanning stops at slash, query, or fragment; after that point, Unicode path/query text remains allowed.
  • No network, filesystem, or URL-routing behavior changes beyond identifying the correct URL range.

Test plan

  • CJK bracket, punctuation, and ideographic-space cases
  • Unicode White_Space and property-class terminators
  • Authority guard against bare CJK host text
  • Non-ASCII path preservation
  • Related terminal-link tests: 140 passed
  • Typecheck
  • Oxlint, React Doctor lint, Oxfmt, and diff checks
  • Manual: paste the issue repro lines and verify only the URL is underlined/clickable

Screenshots

No visual chrome changes.

Security audit

The authority guard prevents missed Unicode boundaries from being silently IDNA-folded into a different hostname. The change adds no external requests, persistence, or new routing destination.

Platform review

The scanner uses standard JavaScript Unicode property escapes and URL parsing already supported by the desktop terminal path. The behavior is platform-independent across macOS, Linux, and Windows.

ELI5

If a terminal prints a URL followed immediately by Japanese or other Unicode notes, Orca now links only the real URL. Unicode text inside the URL path still works.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds shared URL boundary predicates for ASCII whitespace, delimiters, trailing punctuation, and CJK/fullwidth punctuation. Terminal URL hit testing now imports these predicates instead of local implementations. Tests cover URL termination around fullwidth parentheses, CJK corner brackets, ideographic spaces, and half-width separators, including extracted URL spans.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes directly implement #10571 by adding CJK whitespace, bracket, and punctuation boundaries plus regression tests.
Out of Scope Changes check ✅ Passed The diff stays focused on terminal URL boundary logic and tests, with no unrelated feature or refactor changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: terminating terminal HTTP links at CJK brackets and whitespace.
Description check ✅ Passed The description covers the user-visible change, scope, tests, screenshots, security, and platform behavior, but omits separate AI Review Report and Notes sections.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.test.ts (1)

63-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for trailing CJK punctuation.

These tests cover bracket and whitespace boundaries, but not isCjkUrlTrailingPunctuation for characters such as 。, 、, !, or ?. Add a case asserting both the extracted URL and endIndex exclude trailing punctuation.

Based on the PR objective to cover trailing CJK punctuation, this path needs a direct regression case.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7e9cbe17-e47c-4d28-b873-7247211b83d2

📥 Commits

Reviewing files that changed from the base of the PR and between 505967e and 2a4bb28a2a514c571dc2350ebb856a09ffc9bfcd.

📒 Files selected for processing (3)
  • src/renderer/src/components/terminal-pane/terminal-http-url-boundary.ts
  • src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.test.ts
  • src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts

Comment thread src/renderer/src/components/terminal-pane/terminal-http-url-boundary.ts Outdated
@AmethystLiang
AmethystLiang self-requested a review July 25, 2026 23:10
@AmethystLiang AmethystLiang self-assigned this Jul 25, 2026
@AmethystLiang

Copy link
Copy Markdown
Contributor

Thanks for turning this around so fast — the diagnosis in #10571 is spot on, the ASCII-only predicates are exactly the bug.

I dug in before approving and think the fix needs a different shape. Running this PR's predicates against a wider case set, the punctuation-only half of isCjkUrlTrailingPunctuation — 、。・…,.!?:;, everything it adds beyond isCjkUrlBracketOrQuote — never actually fires:

line this PR resolves to
https://example.com、それから https://example.xn--com-6y3b8ordxnwa/
https://example.com。次の行 https://example.com.xn--u9j508ocks/
https://example.com・次 https://example.xn--com-rs4b4090a/

Trailing-trim only strips from the end of an already-terminated candidate. 、 isn't in the body-terminator set and CJK letters don't terminate either, so findHttpUrlCandidateEnd runs to end of line; the last character is ら, which isn't trailing punctuation, so the trim loop strips nothing. The three lines in the issue pass because ( and 「 are real body terminators — the ・ in (作成済み・マージ待ち) never gets exercised.

Worth flagging what that output actually is: with no path, the WHATWG parser folds the CJK into the authority and IDNA-encodes it, so the link silently retargets to a different, registrable domain rather than 404ing. I confirmed it against the live module in a running renderer. That makes this a link-spoofing surface, which is why I want to be careful here.

CodeRabbit's one nitpick on this PR was "add coverage for trailing CJK punctuation (。, 、, !, ?)" — that test is exactly what would have surfaced it.

Two changes I'd make:

  1. Move CJK punctuation into the body-terminator set, and go property-based — \p{White_Space}, \p{P} and \p{Sm}, scoped to code points above U+007F — instead of enumerating. This is the third boundary bug in this scanner after PR auto-link fails for Claude Code PRs: ANSI reset code glued to URL breaks terminal scanner #8126 and [Bug]: Cmd-click on a URL opens it with the next line's text glued on #8832, and each enumeration only covers what someone thought of: 〖〗 and ⦅⦆ are missing from the bracket list here, and 〜 U+301C is Pd while ~ U+FF5E is Sm, so even "all the CJK punctuation" wouldn't have caught them. I got that wrong myself on the first pass and had to check the categories rather than assume them.
  2. Add a hostname guard so the authority case can't happen at all, independent of which terminators are in the set.

There's also a second detector in play: pane-dom-creation.ts constructs WebLinksAddon with no urlRegex, so the hover underline and tooltip come from xterm's default regex while the click re-resolves through extractTerminalHttpLinks. The two disagree in both directions today, which is why the underline stays wrong even with this patch applied.

That adds up to a good deal more than this PR signed up for, so I'm going to pick it up in a follow-up with you as co-author rather than ask you to build the whole thing. Hope that's alright — the report and repro were genuinely useful and made this much faster to work through.

@innocarpe

Copy link
Copy Markdown
Contributor Author

Sync update (7d9e85301)

Address Amethyst review: property-based non-ASCII terminators + hostname guard against IDNA spoof; broader Unicode whitespace.

@nwparker nwparker added the bug Something isn't working label Jul 27, 2026
URL scanning only treated ASCII whitespace/punctuation as boundaries, so
Japanese/Chinese annotations glued after a URL (fullwidth parentheses,
corner brackets, ideographic space) were swallowed into the link and
opened as percent-encoded garbage (fixes stablyai#10571).
Address Amethyst review: non-ASCII body terminators via Unicode
White_Space/P/Sm properties; authority rejects non-ASCII so CJK cannot
IDNA-fold into the host; cover 、。・ and broader Unicode whitespace.
@innocarpe
innocarpe force-pushed the fix/terminal-url-cjk-terminators branch from 7d9e853 to 3c294e5 Compare August 4, 2026 13:07
@innocarpe

Copy link
Copy Markdown
Contributor Author

Closing in favor of merged #15240, which already closes #10571.

Current main now terminates HTTP URLs at CJK punctuation / ideographic space in terminal-http-url-extraction.ts (NON_ASCII_URL_TERMINATOR). That is the same user-facing gap this PR targeted. Thank you @nwparker.

@innocarpe innocarpe closed this Aug 20, 2026
lazybuttrying pushed a commit to lazybuttrying/orca that referenced this pull request Aug 31, 2026
The stablyai#10573 review showed a trailing trim only fires when the tail is
exactly what it strips. The letters-only form had the same hole: a
particle followed by punctuation or ASCII left the whole span linked.

  plans/foo.mdを(参照)    ->  plans/foo.mdを(参照
  plans/foo.mdへabc       ->  plans/foo.mdへabc
  docs/日本語.md」を 参照  ->  docs/日本語.md」を 参照

The first two were regressions against main, where the ASCII-only class
truncated at the first non-Latin byte by accident. Widening to \p{L}
removes that accident, so the trim has to carry the boundary itself.

Once an ASCII extension closes the name a real path only continues with
ASCII (`/`, `\`, `:line`), so cut at the first \P{ASCII}. Same lookahead
in all three copies of the spaced-path scan: renderer, mobile tap, and
the injected webview mirror that carries its own regexes.

Also pin \p{M} in the NFD test: the fixture decomposes to a combining
mark, so dropping \p{M} now fails (flagged by coderabbit on stablyai#13399).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QDkPfyEHWNUW1JQZRe6tKb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Terminal URL detection swallows CJK text — non-ASCII characters are never treated as URL terminators

3 participants