fix: decode llms.txt entities once, after every tag strip - #1331
Conversation
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff. The ordering fix is right and the removal-not-reordering framing holds: once oneLine stops decoding, every one of its callers still sits upstream of the single decodeEntities(body), so nothing leaks an undecoded entity, and the description sites were the same inversion spelled backwards. The measurements back it up rather than just asserting it, which is what I wanted to see on a change whose whole risk is silent loss.
Two things below. The one that matters is the hole strip: it is a third independent source change riding along with the two decode fixes, and nothing tests it. I checked, and reverting it alone leaves the entire suite green.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, scoped to the fixture commit and its blast radius. The fixture does discriminate and the comment correction is right.
Two observations about what the hole strip leaves in the corpus, both on the same theme: a template hole in PROSE is dropped, so a sentence about a form binding can arrive with the binding gone. Worth writing down even though I do not think either is this PR's to fix, since the corpus is the surface an LLM reads.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, scoped to the hole-preservation commit. Three real problems, all in code that commit added. A fourth (the AGENTS.md entry) was already covered by a later commit than the one under review.
The sentinel one is the sharpest: the two keep passes run in sequence, so a string-literal hole can park text that already carries an escaped hole's sentinel, and a single-pass restore then emits the inner sentinel verbatim into a text/plain response. No docs page authors that shape today, so the corpus was clean and the mechanism was still wrong.
The llms corpus builder decoded HTML entities inside oneLine() and then ran a generic tag strip over the decoded text. A `<` a docs page authored as prose became a bare `<`, and the later strip matched from it to the next `>` anywhere in the document, deleting everything between the two. On /docs/metadata-routes that was a single 935-character match that swallowed 5 of the page's 9 code samples plus the paragraphs among them. Site-wide it deleted an escaped tag out of 253 prose lines, and where the match crossed a newline it merged separate list items into one run-on line. Tags are now stripped at every stage and entities decoded exactly once, at the end. The two extractPage description sites spelled the same inversion the other way round (decode then strip), so both route through a new plainText() helper that applies the safe order. The template-hole strip is brace-aware too: with the ordering fixed, the `"}` debris a nested hole leaves behind would have become reader-visible instead of being swallowed.
The hole strip shipped with no counterfactual: reverting it alone left every corpus walk and all four new fixtures green, because the only observable difference is two corpus lines nothing asserted. Add the fixture, which reds on the old regex. Also correct the plainText doc comment. It named … as motivating the second whitespace collapse, but that decodes to '...', which is not whitespace. Only motivates it.
Restoring the swallowed fragments exposed what the hole strip does to prose about form bindings. On main those clauses were deleted outright, so the corpus had no <form action=\> line at all; with the fragments back it had 12, plus a stranded escape backslash on the client-router @submit line. A prose hole has three shapes and only one is dynamic. \${x} is ESCAPED, so the page renders the literal ${x} and there is no interpolation at all. ${"lit"} interpolates a string literal, so the reader sees that literal. Only ${x} is resolved at render time and has nothing to contribute. The first two are now kept (parked behind a sentinel so the dynamic strip cannot eat them) and the third is still dropped. The corpus is the surface an LLM reads, and root AGENTS.md invariant 12 is about exactly which shapes a bound form may take, so shipping 12 lines that teach <form action=\> would trade one defect for another. It now renders 24 correct <form action=${importedAction}> bindings, matching what a reader of the rendered docs page sees.
The module now holds two invariants, not one. Record the second alongside the strip-then-decode rule so a future edit cannot collapse the three hole shapes back into one strip.
Two defects in the hole-preservation pass, plus the coverage it dropped.
The restore replaced once and did not rescan what it inserted, so a
string-literal hole parking an escaped hole's sentinel emitted that inner
sentinel verbatim. A private-use codepoint would have shipped in a
text/plain response and into the search index. It now repeats until none is
left, bounded by the parked count since a hole can only carry sentinels
parked before it.
The kept text is copied out of page SOURCE, which is a JS template literal,
so a backtick is written escaped there. Keeping it verbatim moved the escape
debris from in front of the hole to inside it: /docs/components and
/docs/lifecycle rendered .fallback=${html\`…\`} where the page shows
.fallback=${html`…`}, and /docs/troubleshooting mangled the very example
teaching invariant 9. Both keep passes now fold the escapes out.
Keeping the string-literal shape also stole the dynamic strip's only
counterfactual, since that fixture no longer reaches it. A dynamic hole
carrying a nested object literal pins its brace-awareness instead.
unescapeJs(lit) shipped with no counterfactual: no docs page carries an
escape inside a string-literal hole, so reverting it left every test green
and the corpus byte-identical. A fixture holds that half of the rule now.
The brace-aware comment also overclaimed. The regex handles ONE level of
nesting, so ${fn({a:{b:1}})} still leaves )} behind. No page authors that,
checked across all 44, and arbitrary depth is not a regex's job, so the
fixture asserts the limit instead of implying it does not exist.
Test-only: the rendered corpus is byte-identical before and after.
128e29e to
3cd0670
Compare
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass, scoped to the nested-restore commit. Two gaps worth closing and one I am leaving alone.
The one I am leaving is the fenced-sample path. It is the same escape-folding question, but the code-block capture is a different code path that this PR does not touch, and I measured it as unchanged in both directions against main (5 fenced form bindings before, 5 after, and the backslash-backtick count identical at 371). Folding escapes there moves roughly 775 more corpus positions and needs its own before-and-after, so it is separate work rather than something to bolt on here. Flagged for a call on filing it.
1 participant
Closes #1261
Summary
The llms corpus builder decoded HTML entities inside
oneLine()and then ran a generic tag strip over the decoded text. A<that a docs page authored as prose became a bare<, and the later strip matched from it to the next>anywhere in the document, deleting everything between the two. On/docs/metadata-routesthat was a single 935-character match that swallowed 5 of the page's 9 code samples plus the paragraphs among them. Site-wide it deleted an escaped tag out of 253 prose lines, and where the match crossed a newline it merged separate list items into one run-on line.Tags are now stripped at every stage and entities decoded exactly once, at the end, so no stage ever sees text an earlier stage decoded. The pipeline becomes correct by removal rather than by reordering.
What changed
website/lib/docs-llms.server.ts:oneLine()no longer decodes entities. It is a tag strip plus a whitespace collapse and a trim. The singledecodeEntities(body)already sits after the generic strip, so it does all prose decoding.New exported
plainText()applies the safe order (strip, then decode). BothextractPagedescription sites calledoneLine(decodeEntities(...)), which is the same inversion spelled the other way round, and both now route through it.Prose template holes now keep what a reader actually sees. A hole has three shapes and only one is dynamic:
\${x}is ESCAPED, so the page renders the literal${x}and there is no interpolation at all;${"lit"}interpolates a string literal the reader sees; only a bare${x}is resolved at render time and has nothing to contribute. The first two are preserved (parked behind a sentinel so the dynamic strip cannot eat them), the third is still dropped.This was not in the original plan and is the one place the PR grew. Fixing the decode ordering RESTORES fragments main had deleted outright, and those fragments are mostly sentences about form bindings, so the corpus went from zero
<form action=\>lines to 12. The corpus is the surface whose only reader is an LLM, and rootAGENTS.mdinvariant 12 is precisely about which shapes a bound form may take, so shipping that would have traded one defect for another. It now renders 24 correct<form action=${importedAction}>bindings and zero broken ones.The stale comment block describing this loss as live is replaced by the invariant the file now holds.
website/test/ssr/docs-llms.test.ts: theKNOWN_TRUNCATEDpin, its explanatory comment, and thethe truncation exemption still describes realitytest are deleted, and the walkevery sample a page authors reaches the corpusnow covers all 44 pages with no exemption. The pin asserted the broken counts in both directions precisely so a correct fix would red it. Ten tests added: one per behaviour the decode fix restores, and one per hole shape plus the two failure modes preserving them introduced.website/AGENTS.md: records both invariants the module now holds, the strip-then-decode ordering and the three hole shapes.Measured, before and after
The issue's absolute figures were taken when the corpus had 43 pages; it now has 44, so the numbers below are re-measured on this branch against current
main. Every relative shape the issue predicted holds exactly.<code-block>count differs from fenced count/docs/metadata-routes/llms-full.txt<form action=${...}>bindings<form action=\>etc.)The diff accounts for itself completely: 278 evenly-paired changed lines, every one of them longer and none shorter, plus four structural hunks (2 lines to 7, 1 to 76, 1 to 3, 2 to 3) for a net +83 lines, which are the merged list items un-merging on
/docs/no-build,/docs/metadata-routes,/docs/editor-setup, and/docs/deployment. No corpus line loses a character.Diagnostics :is its own bullet again and<script type="importmap">is back in the no-build list. The/docs/client-routerindex description now readsintercepts same-origin <a> clicks and <form> submissions.Checks on the output, all clean: no private-use sentinel leak (U+E000 or U+E001), no residual entity (
<>&"all absent, so prose about markup reads as<div>), noaction="}debris, no broken binding (<form action=\>,<form action=>,<button formaction=\>and@submit=\are each 0),. The backslash-backtick count is identical to main (371), so the escape debris that remains is all inside fenced samples, on the code-block path this PR does not touch.Test plan
Unit:
cd website && node --test test/ssr/docs-llms.test.ts, 18 pass.Counterfactuals, proven at
cf0933c4by toggling each source change independently and re-running. Every one of the three hunks has one:oneLinedecode chain reds three tests: bothbodyToMarkdownfixtures and the now-unexempted corpus walk, which reports/docs/metadata-routes: 9 authored, 4 fenced.extractPagedescription sites redsa page description keeps the escaped tags it teaches.a hole whose value is a string literal keeps the valueandan escaped hole is literal text, not an interpolation to drop, whilea genuinely dynamic hole is still droppedholds the other side of that line so the two kept shapes cannot be over-generalised into keeping everything.Reverting the whole file at once proves nothing, so do not use it as the counterfactual: the import of
plainTextthen fails and the suite never loads.App suite + typecheck:
npm testandnpm run typecheckinwebsite, the two the CIappsjob runs.Route-level integration:
node --test test/docs/llms.test.mjs.Conventions:
npx webjs checkandnpx webjs doctoroverwebsite.Layers that do not apply: browser (nothing hydrates, this is a
.server.tsmodule servingtext/plain), e2e (the routes are already covered by the integration test throughcreateRequestHandler, and no navigation or streaming behaviour changes), Bun parity (a pure string transform in an app's ownlib/, no runtime-specific API, nopackages/*/srctouched, so the parity hook does not fire), smoke (covers the example apps, not the marketing site).Docs surfaces
website/AGENTS.md.webjsconfig key, nohtmlhole prefix, no lifecycle hook. So the rootAGENTS.md, the skill at.agents/skills/webjs/, the docs site pages, the marketing copy, the scaffold templates,README.md, the MCP server,CONVENTIONS.md, and the changelogs all stay untouched. No docs page's prose changes either:website/app/docs/metadata-routes/page.ts:52is correct documentation and rewriting it to dodge the bug would only defer it to the next page teaching escaping.Out of scope
#1262(the docs search fence tracking) ownswebsite/app/api/search/route.tsand rebases on this. For its measurements: the phantom heading count stays at 53, unchanged by this fix, because the restoredmetadata-routessamples open no line with#. The totals move, so #1262 refreshes its own expectations as the later merge.A note on the one deferral
The fenced-sample capture copies from page source without folding JS escapes, so a sample still reaches the corpus as
${html\...`}where the page shows `` ${html...} ``. It is the same question this PR answers for prose, but on a code path the PR does not touch, and it is **unchanged in both directions against main** (5 fenced form bindings before and after, backslash-backtick count identical at 371). Fixing it moves roughly 775 more corpus positions and needs a rethink ofa sample that reaches the corpus reaches it whole`, which compares raw source text, so it is separate work rather than a tail on this one. Recorded on the review thread, awaiting a call on filing.One environmental note for whoever runs the suites:
test/bun/listener.test.mjsfails in a linked worktree (clientIpreads_anon_) and passes in a full checkout at the same commit. It is not affected by this branch: reverting all three changed files makes the tree byte-identical tomainand it still fails.