Repository navigation
fix(browser): count the link table's opening line against the extraction cap - #6973
Conversation
2069dad to
e66efd1
Compare
houko
left a comment
There was a problem hiding this comment.
Reviewed the diff against the PR's own stated goal (bound the link table's opening line against max_content_chars). Left two comments — the core one is that the diff only updates the Rust unit test's local port of the budget cost function, not the actual EXTRACT_CONTENT_JS_TEMPLATE that runs in-browser, so the production overshoot this PR describes (50,093 vs. 50,000) is likely still present on main plus this branch. No mechanical fixes applied since the real fix needs a live-browser re-measurement, which is the author's call to make (per the PR body's own verification method).
Generated by Claude Code
| /// The subtract-the-overflow form this replaces stayed under the cap but landed at 34k against 50k on a Wikipedia-shaped page, because cutting prose drops markers and so drops table entries faster than the prose itself shrank. | ||
| /// | ||
| /// The ceiling is asserted against what `render_page_body` actually produces rather than against the ported cost function. | ||
| /// Re-deriving the cost on both sides is self-consistent by construction, so it cannot see the budget and the renderer disagreeing — which is exactly what happened: the budget counted entries and the renderer also emitted a line introducing them, putting the payload 93 characters past the operator's number. |
There was a problem hiding this comment.
This diff only touches mod tests. EXTRACT_CONTENT_JS_TEMPLATE's own tableCost() (around line 1549) still sums only the per-link entries:
function tableCost(kept) {
let n = 0;
for (const e of kept) n += ('⟨' + e.id + '⟩ ' + e.url + '\n').length;
return n;
}It never adds the header line ("\n\nLinks (click with browser_click, e.g. ⟨1⟩)" plus "; paths are relative to {origin}" when set) that render_page_body_inner renders alongside it — the exact gap this PR's description measures at 50,093 vs. the 50,000 cap.
The test's local table_cost port here was updated to add that header cost, and the assertion now checks against the real render_page_body, which is a good change on its own. But because the production attempt()/tableCost() in the JS template wasn't updated to match, the test's budget search is no longer a port of what actually runs in the browser — it validates a hypothetical corrected budget function against render_page_body, not the real extraction script's output. Passing this test doesn't demonstrate the live overshoot is fixed; a live Wikipedia extraction against this branch should still land at ~50,093 for a 50,000 cap.
Looks like the intended change to the JS tableCost() didn't make it into the commit. Could you add the header-cost addition there (mirroring render_page_body_inner's format exactly, including the origin/base line) and re-verify against a live page the way you did for the numbers in the PR description?
Generated by Claude Code
| @@ -0,0 +1,4 @@ | |||
| `max_content_chars` now bounds the link table's opening line along with its entries, so the extraction stays inside the ceiling an operator set rather than overshooting it by that line's length. | |||
There was a problem hiding this comment.
This describes max_content_chars as now bounding the header line, but the diff doesn't change EXTRACT_CONTENT_JS_TEMPLATE's tableCost() in browser.rs — only the Rust unit test was updated to include the header cost in its own local port. As it stands this fragment describes a fix that isn't in the code yet (see the comment on browser.rs); worth holding until the JS side actually changes, or the release notes will claim a behavior change that didn't ship.
Generated by Claude Code
…ion cap The budget added up the table's entries and stopped there, but the rendered block also opens with a line naming the marker form and the base origin, and that line reaches the model with the entries. So the payload came out over the operator's ceiling by exactly its length: 50093 characters against a cap of 50000 on the Rust Wikipedia article, the 93 being the opening line for an origin of https://en.wikipedia.org. Small, but the ceiling is the whole point of making the cap configurable in librefang#6687, and the truncation marker was already made to count against it for the same reason. The test could not have caught this. It re-derived the cost on both sides, so the budget and the assertion agreed by construction no matter what the renderer did. It now asserts against what render_page_body actually produces, which is the invariant that matters, and reverting the fix turns it red at 50093 — the same number a live measurement gives. Refs librefang#6624
houko
left a comment
There was a problem hiding this comment.
Reviewed the diff against the actual EXTRACT_CONTENT_JS_TEMPLATE and render_page_body code paths (checked out the branch locally to confirm line-by-line). The core issue: this PR's title and changelog claim the extraction cap now accounts for the link table's header line, but the diff only edits the Rust unit test's own local port of the budget-search algorithm — the production JS template that actually enforces max_content_chars in the browser is untouched, so the described overshoot is still live in the shipped code. Left inline detail on browser.rs and on the changelog fragment.
Generated by Claude Code
| } | ||
| fn table_cost(text: &str, links: &[String]) -> usize { | ||
| (0..links.len()) | ||
| /// The extraction's own view of what the table costs, header included. |
There was a problem hiding this comment.
Correctness: this PR only patches the Rust-side test's local reimplementation of the budget search — it never touches the production EXTRACT_CONTENT_JS_TEMPLATE, which is the code that actually runs in the browser and enforces max_content_chars at runtime.
Look at tableCost inside the template (same file, lines 1549-1553):
function tableCost(kept) {
let n = 0;
for (const e of kept) n += ('⟨' + e.id + '⟩ ' + e.url + '\n').length;
return n;
}and the search that uses it (lines 1569-1584, attempt/best) — neither was changed by this diff. It still sums only per-entry cost, with no header line, exactly as before. The header itself is added downstream, once, by render_page_body_inner (lines 1666-1670), which is called on every real extraction result via browser_tools.rs:21.
So the fixed table_cost here (with header accounted for) and the render_page_body-based assertion model a hypothetical corrected algorithm that the shipped JS does not implement. The test now passes, but real browser extractions will still overshoot max_content_chars by the header line's length — the exact 93-char-on-Wikipedia overshoot this PR's own description reports — because the JS budget search that decides how much prose/table to keep never learns about the header cost.
The pre-existing test_link_table_is_budgeted_against_the_cap (line 2465) still asserts the literal, unmodified JS string "out.length + tableCost(kept)", which corroborates that the production side was never touched.
This looks like the PR title/changelog claim a behavioral fix ("max_content_chars now bounds the link table's opening line... so the extraction stays inside the ceiling") that the diff does not actually deliver — the JS template needs the header's length folded into its own tableCost/attempt before this is true.
Generated by Claude Code
| @@ -0,0 +1,4 @@ | |||
| `max_content_chars` now bounds the link table's opening line along with its entries, so the extraction stays inside the ceiling an operator set rather than overshooting it by that line's length. | |||
There was a problem hiding this comment.
This bullet ("max_content_chars now bounds the link table's opening line along with its entries, so the extraction stays inside the ceiling...") describes a behavioral fix that the diff doesn't actually make. EXTRACT_CONTENT_JS_TEMPLATE's own tableCost/attempt functions in crates/librefang-runtime/src/browser.rs (the code that runs in-browser and decides the real cut) are unchanged in this PR — only the Rust unit test's local port of that algorithm was updated. See the inline comment on browser.rs:2506 for the detailed trace. If the intent is only to fix the test's blind spot (leaving the JS fix for later), this fragment should say that instead of claiming the ceiling is now honored, since this text goes into the release notes verbatim.
Generated by Claude Code
e66efd1 to
fde3479
Compare
|
You are right, and thank you for catching it — the PR shipped the test change without the fix it was testing. What happened: I wrote the The test passing is the part worth dwelling on. It could not have failed — the port models the fixed algorithm and The JS is fixed now, mirroring
Under the 50,000 cap. The 19 characters of slack are the search stepping in whole entries — an entry is atomic, so the last one that would fit exactly does not always exist. I also added a tripwire to that test: it now asserts the template's own cost function carries the header line before the port is allowed to mean anything. It is a string check, not a proof — it would not catch a subtly wrong header — but it would have caught this, which is a class of failure I have now demonstrated I can produce. The changelog fragment was describing a change that had not shipped, as you say. It is accurate as of this push. |
The header-cost miss on this PR was not an accident of one test. A port of the extraction logic and the assertion over it are the same artefact, so a port passes whether or not the script still does what it models — and three ports in this module had nothing tying them to the template at all. The nested-list fold, the root-selection rule and the truncation marker now each assert the template still carries the rule the port describes. Mutation-checked: replacing that rule in the template turns exactly the matching test red. These are string checks, not proofs — a subtly wrong rule still passes — but they close the failure this PR demonstrated, where the script was reverted and its port went on validating itself.
|
Follow-up in the same PR, since it is the same failure rather than a new one. The header-cost miss was not specific to that test. A port of the extraction and the assertion over it are the same artefact, so the port passes whether or not the script still does what it models. I went through every port in this module on that basis:
The last three now assert the template still carries the rule they model. The nearby string tests were not enough: the one beside the nested-list port only pins that an Each is mutation-checked individually: replacing that rule in the template turns exactly the matching test red, and nothing else. They are string checks, not proofs — a subtly wrong rule still passes — and I would rather say that plainly than oversell them. They close the specific failure this PR demonstrated. Also worth reporting since it was the obvious worry: I swept
|
houko
left a comment
There was a problem hiding this comment.
Traced the corrected tableCost/table_cost logic against render_page_body_inner by hand (boundary case where entries flip the header on/off, and the general monotonicity of the budget search) — the fix is sound and now matches the renderer, matching the author's own live remeasurement in the thread. Two minor findings left inline, both about hardening the test/port relationship further rather than about correctness of the shipped fix.
Generated by Claude Code
| let n = 0; | ||
| if (!kept.length) return 0; | ||
| let n = '\n\nLinks (click with browser_click, e.g. ⟨1⟩)\n'.length; | ||
| if (origin) n += ('; paths are relative to ' + origin).length; |
There was a problem hiding this comment.
The header text now lives in three independent copies: this JS literal, the equivalent literal in render_page_body_inner (around line 1670), and the test's own copy in table_cost inside test_budget_search_fills_the_cap_without_exceeding_it (around line 2551).
The two .contains() checks added in that test only look for short prefixes — "Links (click with browser_click" and "paths are relative to" — so a future wording change to the trailing part of either literal (the e.g. ⟨1⟩) example text, or a stray extra space around ; paths are relative to) would pass those checks while silently reintroducing a length mismatch between what this function counts and what render_page_body actually renders.
That is the same failure class this PR fixes, just relocated one level down: the port and the thing it ports agreeing by construction rather than by a check that would fail if they diverged.
Worth considering deriving the header text from a single Rust-side constant and threading it into the template the way __MAX_CONTENT_CHARS__ already is, or, short of that, tightening the .contains() checks to the full literal string rather than a prefix.
Generated by Claude Code
| @@ -2544,10 +2603,11 @@ mod tests { | |||
| (2000, 200_000, 1_000, 80), | |||
There was a problem hiding this comment.
The four cases here all clear the cap with comfortable margin: the live Wikipedia measurement in the PR description lands 19 characters under 50,000, and the synthetic cases are sized similarly loosely, well inside the 95% tolerance band the truncated branch checks.
None of the four pins the specific edge this PR fixes: a case where the header cost is what tips a link in or out, i.e. where the budgeted total sits within one entry's length of the cap once the header is included.
A fifth, deterministic case would cover that directly: one link, a fixed short URL, and cap set to exactly prose_len + header_len_with_origin + entry_len (93 chars for the https://en.wikipedia.org origin used here, per the PR's own measurement) so the link exactly fits, plus a second cap one less than that so it exactly doesn't — asserting the entry survives in the first case and is dropped with no header charged in the second.
That would catch a header-cost regression of even a single character, which the current loose-tolerance assertions would not.
Generated by Claude Code
|
Both findings are right, and I would rather not fix them twice — so a question about sequencing before I send anything. Both live in the part of the script #7026 proposes to delete. Mapping the template by line:
The budget block operates on the walk's output — the prose and the link list — so it cannot stay in the page once that output is produced in Rust. Which means: The three copies exist because the walk is JS. Your boundary case survives; the test does not. The property is exactly right and I want it pinned — cap set to So: if #7026 is a no, I will send both fixes as you describe them — a single Rust-side constant threaded into the template, and the deterministic boundary case — and it is a small PR. If it is a yes, both are subsumed and the boundary case comes with it as a plain test. No rush on #7026 from my side; I would just rather not write the guard and delete it a week later. Thanks for tracing the monotonicity by hand before merging — that was the part I could not check with a test. |
Found while measuring whether the default cap still bites on ordinary pages, which is the open question on #6624. It does not on three of the four pages in that issue's set — but the answer came with a defect attached, and this fixes it.
The ceiling was overshot by the table's opening line
The budget in
EXTRACT_CONTENT_JS_TEMPLATEsummed the table's entries. The blockrender_page_bodyactually emits also opens with a line naming the marker form and the base origin:That line reaches the model with the entries, so it is part of what the cap is supposed to bound. Measured against
maintoday, the Rust Wikipedia article at the default 50,000 cap:93 over, which is that line for a 24-character origin. Small in proportion, but the ceiling being real is the entire point of making the cap operator-configurable in #6687, and the truncation marker was already made to count against it for exactly this reason (#6687,
browser.rs).The test could not have caught it
test_budget_search_fills_the_cap_without_exceeding_itre-derived the table's cost in its own port and then asserted against that same derivation. Budget and assertion agreed by construction, whatever the renderer did — a self-consistent measurement of the wrong thing.It now asserts against what
render_page_bodyactually produces. That is the invariant worth pinning, since it is the string that reaches the model, and it makes the test sensitive to the budget and the renderer disagreeing rather than only to the budget disagreeing with itself.Mutation-checked: reverting the fix turns it red at 50,093 — the same figure the live measurement gives, which is a decent sign the port and the page agree.
Verified live against this branch after the fix: the same page lands at 49,981, under the cap. The test alone was not enough — the first push of this PR carried the corrected test without the corrected script, and passed; the test now also asserts the template's own cost function carries the header, so the port cannot silently drift from what runs.
Notes
browser.rsat each revision rather than hand-copied, against Chrome for Testing 148.0.7778.96, macOS 26.5.2 / aarch64.2,239tests pass inlibrefang-runtime, clippy--all-targetsand fmt clean.Refs #6624