Repository navigation
fix(file-links): detect non-Latin paths and trim prose glued around them - #17827
lazybuttrying wants to merge 5 commits into
Conversation
Space-less languages glue particles onto paths (README.mdへ). Widen
path classes with \p{L} for mid-path CJK (stablyai#13396) and trim non-ASCII
letters after an ASCII extension so the particle is not part of the
link (stablyai#15240 symmetry).
…ary module Move non-ASCII prose boundary from terminal-http-url-extraction into src/shared/non-ascii-terminal-text-boundary.ts and wire file-link trim through the same module (stablyai#15240 mirror).
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
Two gaps remained after the trailing trim.
`파일.md` never linked at all: BARE_FILENAME_PATTERN was ASCII-only, so a
bare filename whose *name* is non-Latin failed looksLikeFilename with or
without a particle. Widen it to \p{L}\p{M}\p{N} and require an ASCII
extension, which keeps the prose rejection the function exists for -
otherwise every sentence ending in `.` reaches the filesystem probe.
Prose also glues onto the left (`문서는README.md`), and the span swallowed
it. Trim a leading non-ASCII run, but only when the path resumes in ASCII:
`참고파일.md` and `한글폴더/파일.md` are plausible real names and must stay
whole. A file genuinely named `파일은README.md` is the accepted cost.
Applied in all five implementations - renderer links, bare detection,
mobile tap, its injected WebView mirror, and mobile markdown - with the
new cases in the shared conformance list, so each asserts twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QDkPfyEHWNUW1JQZRe6tKb
`docs/a.md see docs/b.md` linked neither file. The spaced-path scan walks every extension on the line and extends the span when the added segment contains a separator, guarded by countPathStarts > 1. That guard only recognises `/`, `./`, `~/` and `C:\` starts, so two *relative* paths never tripped it: the span ran to the last extension, produced a string that is not a file, and both real paths went dead. Absolute pairs were unaffected, which is why this stayed hidden. Stop extending once the selection is already a finished path - a separator plus an alphabetic extension. The alphabetic part matters: `/tmp/v1.2` ends in a numeric `.2`, so a spaced directory name still extends correctly into `/tmp/v1.2 reports/result.json`. Early-exiting made several line-ending prefix candidates trim to the same span, so dedupe emitted spans - `/var/log/app.log` was returned four times. Mirrored into the mobile tap matcher and its injected WebView copy, with both halves of `docs/a.md see docs/b.md` in the shared conformance list. Pre-existing on main and not CJK-specific; found while testing this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QDkPfyEHWNUW1JQZRe6tKb
Greptile SummaryThe PR broadens terminal and mobile file-link recognition to Unicode path components, trims non-ASCII prose around detected links, and separates adjacent relative paths.
Confidence Score: 3/5The PR should not merge until valid dotted-directory spaced paths and mixed-script filenames are preserved. The new completion check splits valid spaced paths at alphabetic dotted directories, while the shared trailing trim can redirect valid mixed-script filenames to an earlier extension-like prefix. Files Needing Attention: src/renderer/src/lib/terminal-spaced-path-trailing-prose.ts, src/shared/non-ascii-terminal-text-boundary.ts
|
| Filename | Overview |
|---|---|
| src/shared/non-ascii-terminal-text-boundary.ts | Adds shared Unicode boundary trimming, but trailing trimming can truncate valid mixed-script filenames after an extension-like component. |
| src/renderer/src/lib/terminal-spaced-path-trailing-prose.ts | Extracts spaced-path trimming and separates adjacent relative paths, but misclassifies alphabetic dotted directory segments as completed paths. |
| src/renderer/src/lib/terminal-links.ts | Applies shared trimming and deduplicates identical candidate spans; its behavior inherits the two trimming defects. |
| mobile/src/terminal/terminal-path-tap.ts | Mirrors Unicode matching and spaced-path splitting for native mobile taps, including the dotted-directory truncation behavior. |
| mobile/src/terminal/terminal-path-tap-injected.ts | Keeps injected WebView matching aligned with the native implementation, including the same edge-case regressions. |
| mobile/src/components/markdown-file-path-detection.ts | Broadens Markdown path detection to Unicode and applies shared trimming before openability checks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Terminal or Markdown text] --> B[Candidate regex]
B --> C[Spaced-path trimming]
C --> D[Non-ASCII prose trimming]
D --> E[Parse path and line/column]
E --> F[Resolve and open file]
Reviews (1): Last reviewed commit: "fix(file-links): stop two relative paths..." | Re-trigger Greptile
| if (selected !== null && COMPLETE_PATH.test(selected)) { | ||
| break | ||
| } |
There was a problem hiding this comment.
Dotted directories terminate paths
When a spaced path contains an alphabetic dotted directory such as /tmp/My.Project reports/result.json, COMPLETE_PATH treats /tmp/My.Project as a finished file and stops scanning. This splits the target and resolves reports/result.json relative to the working directory instead of opening the intended absolute path.
| const ASCII_EXT_THEN_NON_ASCII = | ||
| /^(?<path>.*\.[A-Za-z0-9_+-]+(?::\d+)?(?::\d+)?)(?<prose>\P{ASCII}[\s\S]*)$/u | ||
|
|
||
| export function trimFileLinkTrailingNonAsciiProse(text: string): string { | ||
| const match = ASCII_EXT_THEN_NON_ASCII.exec(text) | ||
| return match?.groups?.path ?? text | ||
| } |
There was a problem hiding this comment.
Mixed-script filenames are truncated
When a valid filename contains an ASCII extension-like component followed by non-ASCII characters, such as foo.md日本語.txt, this regex discards everything after .md. The resulting link targets foo.md instead of the existing mixed-script file, causing it to open the wrong file or fail.
There was a problem hiding this comment.
Important
Two correctness bugs in the new prose-trim logic: left-glued prose corrupts the reassembled text in mobile markdown detection, and the two-relative-paths fix truncates spaced paths whose first directory name ends in a dotted alphabetic token.
Reviewed changes
- Widen path classes to
\p{L}\p{M}\p{N}across the renderer, bare detection, mobile tap, injected WebView, and markdown detectors so non-Latin names match at all. - Add
src/shared/non-ascii-terminal-text-boundary.tsowning the #15240 URL terminator (moved verbatim) plus file-link leading/trailing prose trims with range-index adjustment. - Stop two relative paths on one line from merging via a
COMPLETE_PATHbreak and span-level dedupe in the spaced-path pass. - Extend the shared conformance list so each new case is asserted by both the TypeScript tap matcher and the injected WebView matcher.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
|
|
||
| while ((match = CANDIDATE_PATTERN.exec(text))) { | ||
| const candidate = match[0] | ||
| const candidate = trimFileLinkNonAsciiProse(match[0]) |
There was a problem hiding this comment.
trimFileLinkNonAsciiProse trims leading prose too, but match.index and lastIndex below are computed from the untrimmed match, so the reassembled text is corrupted when prose is glued on the left. For 参照docs/文書.md the candidate becomes docs/文書.md while the segment is still emitted at match.index, dropping the leading prose and duplicating the tail. Use the trailing-only trim here, or shift the indices by the trimmed prefix length.
Technical details
# Markdown left-glued prose misalignment
## Affected sites
- mobile/src/components/markdown-file-path-detection.ts:185 — `trimFileLinkNonAsciiProse(match[0])` strips a leading prefix, but `prev` (line 189), `text.slice(lastIndex, match.index)` (line 197), and `lastIndex = match.index + candidate.length` (line 200) still use the untrimmed `match.index`.
## Required outcome
- `detectFilePathSegments('参照docs/文書.md を見て')` must emit a text segment for `参照` plus a file segment `docs/文書.md` whose positions reconstruct the original string exactly.
## Suggested approach
- Compute the leading-trimmed text separately and advance the segment start by the stripped prefix length (then apply the trailing trim), or call `trimFileLinkTrailingNonAsciiProse` instead of `trimFileLinkNonAsciiProse` — the candidate pattern never captures a trailing non-ASCII run, so the trailing trim is a no-op here and only the harmful leading trim fires.
- Add a `detectFilePathSegments` case for left-glued prose so the positions are pinned by test.| } | ||
| // Why: countPathStarts only sees absolute starts, so two relative paths on one | ||
| // line used to merge into a span that resolves to nothing and killed both. | ||
| if (selected !== null && COMPLETE_PATH.test(selected)) { |
There was a problem hiding this comment.
The COMPLETE_PATH break fires whenever the selected span already ends in a separator plus an alphabetic extension, which also truncates a single spaced path whose first directory name ends in a dotted alphabetic token followed by whitespace. ./my.dir with space/file.md now yields ./my.dir and space/file.md instead of one link, whereas before this change the same input merged into the full path. The alphabetic-extension guard covers v1.2-style numeric tails but not directory names like my.dir or api.v2.
Technical details
# COMPLETE_PATH over-breaks dotted directories in spaced paths
## Affected sites
- src/renderer/src/lib/terminal-spaced-path-trailing-prose.ts:38 — the break condition.
- mobile/src/terminal/terminal-path-tap.ts:98 and mobile/src/terminal/terminal-path-tap-injected.ts — mirrored copies of the same rule.
## Required outcome
- `extractTerminalFileLinks('./my.dir with space/file.md')` must return the single full path; only genuinely separate paths (e.g. `docs/a.md see docs/b.md`) should break.
## Suggested approach
- Constrain the break so it only fires when the remainder between the selected span and the next extension begins a new path, or accept the tradeoff and document it explicitly. Either way, add a regression test for a dotted-directory spaced path.
ELI5
In space-less languages such as Korean and Japanese you don't put a space before a grammatical particle, so people write
README.md로or文書.mdへ— the particle is glued straight onto the filename. Orca's terminal saw that whole blob and gave up: it stopped the link at the first non-Latin character, sodocs/파일.mdwas never clickable at all.This teaches file links the same lesson URLs already learned in #15240: letters inside a path are part of the path, but prose glued around it is not. Now the link covers exactly the file and stops where the sentence resumes — on either side of the path.
What Changed
Five commits, each a separate defect:
\p{L}\p{M}\p{N}so non-Latin directory and file names match at all, and trim a trailing particle.src/shared/non-ascii-terminal-text-boundary.tsnow owns both halves: the URL terminator (moved verbatim, no behavior change) and the file-link trim.파일.mdnever linked at all;문서는README.mdswallowed the prose.Applied in all five implementations — renderer links, bare detection, mobile tap, its injected WebView mirror, and mobile markdown — with cases in the shared conformance list so each asserts twice.
Why
#15240fixed this for URLs and left the file-link half undone. Three things bite in sequence.A letters-only trim never fires when the tail isn't a bare letter run, which is exactly the failure identified in the #10573 review:
Worse, widening to
\p{L}removes an accidental safety net: today's ASCII-only class truncates at the first non-Latin byte, which happens to give the right answer for some inputs. Measured againstmain:mainplans/foo.mdを(参照)plans/foo.mdplans/foo.mdを(参照❌plans/foo.mdplans/foo.mdへabcplans/foo.mdplans/foo.mdへabc❌plans/foo.mddocs/日本語.md」を 参照docs/日本語.mdSo the trim carries the boundary itself: once an ASCII extension closes the name, a real path only continues in ASCII (
/,\,:line), so cut at the first\P{ASCII}. That also picks up CJK brackets (」,)), which #15240 already treats as URL boundaries.\P{ASCII}rather than[^\x00-\x7F]because the latter tripsno-control-regex.Bare filenames were still ASCII-only.
BARE_FILENAME_PATTERNcould never match파일.md, so a bare non-Latin name wasn't a link with or without a particle. Widening it requires a guard — an ASCII extension — or every sentence ending in.reaches the filesystem probe.Prose glues onto the left too.
문서는README.mdswallowed the prefix. The left side is genuinely ambiguous, so the trim only fires at a non-ASCII → ASCII boundary:참고파일.mdand한글폴더/파일.mdare plausible real names and stay whole. A file genuinely named파일은README.mdis the accepted cost — it can't be told from prose without a filesystem probe at detection time.Linked Issue
Fixes #13396
Related: #15240 (merged — this is its file-link mirror), #10571, #10573 (closed unmerged), #13399 (closed unmerged; this diff is a superset and additionally addresses the
\p{M}test comment left on it).One pre-existing bug fixed along the way
docs/a.md see docs/b.mdlinked neither file. The spaced-path scan extends a span when the added segment contains a separator, guarded bycountPathStarts > 1— but that guard only recognises/,./,~/,C:\starts, so two relative paths never tripped it. The span ran to the last extension, produced a string that isn't a file, and both real paths went dead. Absolute pairs were unaffected, which is why it stayed hidden.This reproduces on
mainwith plain ASCII and has nothing to do with #13396. It's included because it makes multi-path lines unusable and the fix is small; happy to split it into its own PR if you'd prefer.Visual Proof
Captured by driving the real app through the Playwright/CDP e2e harness (
electron-headless), hovering the same cell on each branch. All fixtures are real files, so link resolution is genuine.main)cjk-demo/파일.md로 열어라cjk-demo/파일.md참고cjk-demo/파일.md 봐라cjk-demo/파일.mdcjk-demo/a.md see cjk-demo/b.mdcjk-demo/a.md+cjk-demo/b.mdcjk-demo/참고파일.md문서는README.md다README.mdTesting
Also the checks
static_analysisruns thatpnpm lintdoesn't cover:And the full
mobile/gate:typecheck0,lint0,format:checkclean (1447 files),test487 files / 3,982 passed.Full-suite failures are pre-existing (
codex-fetcher-pty-settle,shell-startup-feature-channel, twonode-ptyfish tests). Verified not by counting but by comparing failing sets in isolation on both branches: 7 onmain, 7 on this branch, identical — nothing fails here that doesn't fail onmain.Behavior is pinned in
src/shared/terminal-file-link-conformance.ts, the shared case list driving both the TypeScript tap matcher and the injected WebView matcher, so each new case is asserted twice.Platforms exercised: macOS (unit, build, app-level e2e). The change is pure regex with no filesystem, shell, or process interaction, so there is no platform-divergent path;
\P{ASCII}was additionally checked against the Hermes compiler (see Review).AI Disclosure
Claude Opus 5 (via Claude Code) was used to review the original commits, find the regressions and gaps above, write the failing tests first, and implement the fixes. Every finding was reproduced by execution against
mainbefore any change.Review
[\\/]) preserved. Regex-only.\p{ASCII}is a binary Unicode property, unlike the\p{L}/\p{N}general categories already used in mobile code, so it was checked separately. Hermes 0.14.1 (RN 0.83.10) accepts it and rejects a bogus property name (Invalid property name), confirming it validates rather than ignores. Verified at compile time viahermesc, which cannot execute — on-device runtime semantics were not directly exercised. Node/Electron and the WebView engines support it as ES2018.execcalls on an already-bounded candidate. The multi-path fix shortens the walk by exiting early, and adds a span-level dedupe that removes duplicate emissions (/var/log/app.logwas previously returned four times through the line-ending prefix candidates).terminal-links-redos.test.tscovers catastrophic-backtracking inputs (30k-char padding) and still passes.terminal-path-tap.tshad already drifted from the injected mirror; that drift is fixed and now pinned by the shared conformance list.resolveTerminalFileLink/ worktree-boundary checks, untouched./tmp/v1.2 reports/result.json(the numeric.2is why the "finished path" rule requires an alphabetic extension).Agent skill upstream boundary
Notes
pnpm formatreformats several files unrelated to this change (pre-existing drift onmain); reverted to keep the diff scoped.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass