feat(markdown): seam for autolinking bare organisational tokens - #7270
Conversation
46d4e9e to
eafaae3
Compare
eafaae3 to
679ef66
Compare
UX Review (Fable 5, fork) — ✅ PASSUX-level review of The evidence is complete. The registry ships empty so the stock dashboard renders byte-identically (test-pinned); when an edition registers a rule, the generated UX-Verdict: PASS Zero stock-build delta; edition-registered tokens become ordinary links through the existing anchor path, with quoted code and pasted URLs left untouched. [UX-REVIEWED] 4f43347 |
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS A real gap closed at the established seam layer: registrar convention followed, vocabulary kept downstream, and the untrusted-content risk confined by registration-time validation. [DESIGN-REVIEWED] 4f43347 |
First Principles Review (Fable 5, fork) — ✅ PASSPremise-level review of All claims verified against the base. Final review: First-Principles-Verdict: PASS A markdown-autolink seam no existing seam can express, built on the repo's recorded edition-seam model, with every hardening step traced to a measured failure. What this change shipsIntent: let a downstream edition make bare organisational tokens (ticket ids) in chat prose clickable. ADDITION.
Counts run: [FIRST-PRINCIPLES-REVIEWED] 4f43347 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
679ef66 to
c868b44
Compare
5f48e78 to
595e52c
Compare
042cc97 to
9abbf92
Compare
9abbf92 to
83efa75
Compare
83efa75 to
3df078d
Compare
3df078d to
aba5132
Compare
aba5132 to
e11fa48
Compare
Adds registerAutolinkRules() plus an applyAutolinkRules() pass beside the two existing source-rewrite passes. The core registers none, so stock is unchanged.
bolichen97
left a comment
There was a problem hiding this comment.
Approving on the strength of a full readiness audit of every open PR against main, not a
line-by-line reading of this diff — recording that plainly so the next reader knows what this
stamp does and does not cover.
Verified against this exact head SHA:
readiness: passedpresent, andPR Readiness— the one required status context onmain
(rulesetprotected-branches) — issuccesson this head.- No check run on this head is
failure,cancelled,timed_outor still in flight. Skipped
jobs are path-filtered conditionals, none of them required. mergeable: true, and the head is not far enough behindmainfor its green CI to describe a
base that no longer exists.- No surviving reviewer
CHANGES_REQUESTED: any such review is on an older commit and therefore
already dismissed bydismiss_stale_reviews_on_push. - Every issue comment, inline review comment and review thread was read and classified. Nothing
left is an unresolved human change request — the remainder is bot review-lane output, resolved
or outdated threads, explicitly non-blocking suggestions, and author status notes.
Auto-merge (squash) is armed, so this lands once every other ruleset requirement is met.
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation
GFM autolinks anything carrying a scheme (
https://…) or an email shape. What itcannot know is that in a given organisation a bare token is an address — a
ticket key, a change-review id, an incident number. Those tokens get written
constantly in agent prose and arrive as dead text, so every reader hand-copies them
into a URL bar.
There is no way for a downstream edition to fix this today. The thirteen existing
extension seams are all additive contributions of surfaces (routes, icons, themes,
widgets, panels) and none touches markdown rendering, so the vocabulary cannot be
supplied from outside
MarkdownRenderer.tsx.MD_COMPONENTSis module-level andnot exported, so the trick that lets an edition rewrite a core i18n string —
mutating an exported map before the core reads it — does not apply either.
Why it matters
The mechanism is general; the vocabulary is not. A token scheme usually names
infrastructure specific to one deployment, which has no business being hard-coded
in a shared renderer. Splitting them is what makes this contributable at all: the
core gets a seam it registers nothing into, and each edition supplies its own rules.
Cost to the stock build is zero, not merely small. The plugin returns before
walking when the registry is empty, so a build that registers nothing renders
identically to one without the seam. That is asserted, not asserted-by-hope: the
first test in the new suite pins it, and the "before" frames below are what a stock
build keeps rendering.
What changed
New registry —
website/src/utils/autolinkRules.ts.registerAutolinkRules([{ id, pattern, href }])paired with
getAutolinkRules(), following the existing registrar convention(
reportSeamCollisionon a duplicate id, first registration wins, readonly reader).Three validations run at registration, so a bad rule fails once and loudly
rather than on one unlucky message:
lastIndex, so a scanfinds at most the token at offset 0 and the rule appears to work on exactly one
input;
guarantee: a pattern can still yield a zero-width match against real content
(
/(?=x)/), which registration cannot see. The scan handles that case byABANDONING the rule — measured, the alternative of nudging
lastIndexpast azero-width match is an infinite loop under
u, because the nudge landsmid-surrogate and the engine re-matches the same position forever (2,000,001
iterations without terminating on
/(?=💩)/gu, against an ASCII control thatterminates in 2);
gis added rather than refused — a papercut, not a mistake.hrefis a URL template, not a function:'https://tickets.example.com/{match}'.{match}is substituted percent-encoded, so a token cannot introduce a scheme,userinfo, host or separator — it is confined to one path, query or fragment segment.
That is what lets the destination be validated once, at registration, with no
per-match re-check and no guard against a rule that throws: there is no per-match
outcome that could differ. The check is
safeHttpUrl(
website/src/lib/safeUrl.ts), reused rather than reimplemented — it also rejectsBasic-auth userinfo (
https://user:pw@host), which a destination assembled frommessage text should not carry — plus a two-canary origin comparison: the template
is expanded twice with different values and both must yield the same origin. That
rejects a placeholder sitting in the authority, where a token could otherwise steer
the host, and leaves only the positions percent-encoding cannot escape. Measured
against nine adversarial matches (
a/b,a@b,a:b,../..,evil.com, …), theorigin never moves. The normalized
new URL(...).hrefis what gets linked.New plugin —
website/src/utils/remarkAutolinkRules.ts, a remark plugin overmdast
textnodes. It is not a source rewrite: a match becomes alinknode,so nothing is re-parsed and no markdown is spliced into a line.
That shape is what keeps the safety argument short, because the constructs a source
rewrite has to enumerate cannot arise in the first place:
linknode by the time the plugin runs — it is ordered last, afterremark-gfmfootnoteReferencelabel]in the matched text!immediately before the token)in the destinationTwo checks the tree cannot express structurally do remain, and each is covered by a
test that fails without it:
<code>…</code>is asibling of those
htmlnodes rather than their child, so the walk pairs the tagsand skips what they enclose. The enclosure depth descends with the walk, so a token
nested inside emphasis (
<a href="…">**TOKEN**</a>) stays literal too rather thanproducing an anchor inside an anchor.
stay literal. CommonMark escapes only ASCII punctuation, so
\TOKEN-1reaches thetree with its backslash intact.
Positions are never shifted, so unlike the two sibling source-rewrite passes
(
fixCjkAutolinkBoundaries,fixUnencodedLinkDestinations) this also runs underdata-sourceposand needs no gate.The renderer touch is correspondingly small:
MarkdownRenderer.tsxgains one importand one entry at the end of
REMARK_PLUGINS.REMARK_PLUGINS_WITH_BREAKSis aspread of that array, so it picks the plugin up with no second edit, and no existing
function in the file is modified.
Overlaps resolve in registration order FIRST, and only the accepted spans are
then sorted by position. The other order — sort by start, emit greedily — hands a
contested span to whichever rule happens to start earlier, so a later-registered rule
silently steals it from an earlier one.
Docs —
website/docs/extension-seams.mdgains an inventory row and a section, andits four "thirteen" counts become "fourteen".
src/extensions.tslists the newregistrar in its header.
Screenshot Evidence
Captured with
website/scripts/capture-token-autolink.mjs(committed), which runsthe real built SPA behind the shared static server with
/api/**answered fromfixtures, so the bubble goes through the actual remark/rehype pipeline.
Two builds, because the seam is inert on its own — the core registers no rules, so
the delta only appears once an edition registers one:
distbuilt withKIROCREW_EDITION_DIRunset (stock, no rules). Thisis also the evidence for the no-op claim above.
distbuilt with a throwaway demo edition registering one\bTICKET-\d{3,}\brule. The demo edition is not part of this PR.Each frame answers three questions at once: does a bare token in prose become a
link, does a token inside a code span stay literal, and does a pasted URL that
already contains the token survive intact.
Dark — before / after
Light — before / after
Line 1 is the feature. Line 2 and line 3 are the ones worth checking: the code span
renders identically in both frames, and the pasted URL's href comes out unchanged.
Tests
website/src/test/autolinkRules.test.ts— 39 tests, weighted on the exclusionsrather than the happy path, since the plugin is trivially right on a bare token in a
sentence and every real defect is a token somewhere it must not be linked.
Each exclusion has a paired positive assertion, so a test that passes because the
rule never fired at all shows up as a failure.
Covered: the empty registry adds no link; a bare token links and the surrounding
prose survives; every occurrence links; code spans, fences, existing link labels,
pasted bare URLs, footnote labels, raw-HTML-enclosed text and escaped tokens are all
left alone; a token nested in emphasis, italics or strikethrough between paired raw
<a>tags stays literal (paired with a positive for the same emphasis with noenclosing tag); an anchored pattern does not match inside a longer word; a
non-
http(s)template, adata:URL, a relative destination, Basic-auth userinfo,a placeholder in the authority, a placeholder in the port and a template with no
{match}at all are each refused at registration; a match containing/or@ispercent-encoded and cannot escape its path segment; an overlapping
span goes to the earlier-registered rule EVEN WHEN the later rule starts earlier
(paired with a test that a non-overlapping later-rule match still links, so blanket
over-rejection cannot satisfy it); the normalized destination is what gets linked; a
zero-width rule is abandoned rather than hanging (with a sibling-rule test proving
the abandon is scoped to that one rule); and the registration validations behave as
described.
Three cases assert the linking a source rewrite had to refuse — a bracket-bearing
match, a token preceded by
!, and a destination containing)— since a link nodemakes all three inert.
website/src/test/extensionSeams.test.tsxgains the seam's three shared checks(ships empty, a registered rule is retrievable and normalised, a duplicate id is
fail-loud and preserves the first registration), so the doc's claim that the suite
exercises every seam stays true.
Negative controls. Every load-bearing check was verified able to fail, each
restoring byte-identical afterwards: neutering the raw-HTML pairing fails exactly
three tests, removing the backslash guard fails exactly one, dropping the two-canary
origin comparison fails exactly one, and substituting the match raw instead of
percent-encoded fails exactly one. The nested-anchor cases were written first and
observed failing before the depth fix.
Local runs:
tsc -p tsconfig.app.json --noEmitclean — that is the config thatactually contains these files; the root
tsconfig.jsonis project-references onlyand checks zero files, so a bare
tsc --noEmitat the root is vacuous here.eslint src/ --max-warnings 603passes with 0 errors, against the ratchet maintightened from 659 in #7558.
vitest186 passed across thenew suite, the seam suite and the two neighbouring autolink suites.
Copy/paste detection. The first revision of the capture script duplicated 25
lines of the sibling CJK harness's
main(); that is fixed by rewriting it onto theshared
openTranscriptHarnesshelper, which exists for exactly this reason.npx jscpd .reports 0 clones.