fix(scripts): govern .mjs in the desktop and web file-size ratchets - #6736
Open
baxen wants to merge 2 commits into
Open
fix(scripts): govern .mjs in the desktop and web file-size ratchets#6736baxen wants to merge 2 commits into
baxen wants to merge 2 commits into
Conversation
AGENTS.md states the 1000-line ceiling is "enforced across Desktop, Web, and Mobile by the repository-level `just file-size-check` gate", but the desktop and web rule tables listed only `.ts`/`.tsx` for their script roots, and `runFileSizeCheck` skips any file whose extension is not in its rule's allowlist. Desktop's suite is `*.test.mjs` by convention and its shared test rigs are plain `.mjs` modules, so 545 files inside roots the ratchet already governs were outside the ceiling -- silently, since an uncovered file makes the check exit 0 just like a clean one. Found when review caught a 1253-line test file on #6720 that every local gate and CI had passed. Refs #6726. The gap and the fix, on a clean main checkout: a new 1501-line `.mjs` under `src/features` exited 0 before this change, and byte-identical content named `.ts` exited 1. Adding the extension needs no splits, because the gate is a ratchet rather than an absolute bound -- `allowedLineCount` returns `baseLines` when the base file already exceeds the max. Verified against all twelve oversize files at once: rewriting each without changing its line count exits 0, and adding a single line to each exits 1 naming all twelve with their inherited limits. New files are still held to 1000 from birth. The rule tables move into `file-size-rules.mjs` so the tests can assert the configuration the runners actually execute rather than a restatement of it. The runners stay unconditional: a module that both exports its rules and self-guards its execution can silently stop gating, which is the same failure class this change exists to close. Three tests, each proven non-vacuous by reverting the thing it covers in isolation: mutation fails desktop table back to .ts/.tsx all 3 web table back to .ts/.tsx the allowlist test alone grandfathering -> always maxLines the 2 inherited-size tests The two behavioural tests drive the real rule table against throwaway repositories in a child process, so a leaked `process.exitCode` cannot mark the suite failed. Node exits 1 for an uncaught exception as well as for a violation, so a failing child must also print the violation report to count as one -- otherwise a broken harness would read as a passing gate, which is the same trap again. That guard is itself covered: a bad rules import fails with "crashed rather than gated" instead of passing. Signed-off-by: ss-dev-01 <11939edb7df583f855dbef923f2358f1184538f88ca452e19e7e35e42ad6d796@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
The child-process guard proved the gate *started* reporting, not that it
finished. Status 1 plus the report heading was accepted as a completed
policy decision, so a child that printed the heading -- or the whole
report -- and then crashed still read as the gate having decided.
The eval script now writes a fixed sentinel to stdout after the awaited
`runFileSizeCheck` returns, and the harness requires it on both the
exit-0 and exit-1 paths. The two questions are separated: the sentinel
proves normal completion, and only then does the exit status carry the
policy result.
Proven against the case the old guard missed. Injecting a throw after the
full violation report is emitted but before the gate returns:
old guard (heading only) 9 passing / 0 failing <- crash read as a pass
new guard (sentinel) 7 passing / 2 failing <- "did not run to
completion ... crashed
rather than gated"
A throw injected immediately after the heading also fails with the same
assertion rather than being misattributed to a missing violation line.
This is the same defect the PR closes, one level up: a check that cannot
distinguish the outcome it reports from a failure to reach it.
Signed-off-by: ss-dev-01 <11939edb7df583f855dbef923f2358f1184538f88ca452e19e7e35e42ad6d796@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
baxen
added a commit
that referenced
this pull request
Aug 25, 2026
…harness `AgentSessionWorkBlock.test.mjs` landed at 1,146 lines. Under today's rule tables that passes, but only because the desktop ratchet's script roots allowlist `.ts`/`.tsx` and silently skip `.mjs` — the gap #6736 (`ce1f1d427`) closes. Measured rather than assumed: with that rule table cherry-picked, this file is the one remaining violation on C, and because `allowedLineCount` grandfathers a base that already exceeds the max, whatever count C lands with becomes the file's permanent ceiling on main. 1,146 is not a ceiling worth inheriting for a file that exists to cover a single component. Split on the seam the file already had, at `// -- Orphaned work --`, where the subject changes from live-vs-finished *policy* to what an individual *row* is: - `AgentSessionWorkBlockTestRig.mjs` (311) — jsdom lifecycle, the item fixtures, `settle()`, `renderBlock()`. - `AgentSessionWorkBlock.test.mjs` (352) — Live, Finished, fold animation, reader choice, rail glyph states. - `AgentSessionWorkBlock.orphaned.test.mjs` (528) — orphaned work, the per-kind rail presentation, streaming re-render cost. One rig, not a copy per file. The two suites run in separate processes (node's runner is one process per file), so a second copy of the jsdom and `matchMedia` setup could not *collide* — it would drift, and a drifted ambient pin fails a fixture for a reason that has nothing to do with the markup under test. That trap already cost this suite family two commits (`2b7b0baf6`, `c91819706`). The split forces one real behavioural change. `prefersReducedMotion` was a module-level `let` the reduced-motion test assigned directly; ESM bindings are read-only in importers, so once it lives in the rig that assignment cannot work. It goes through `setPrefersReducedMotion(value)`, with the rig's `afterEach` still resetting it. Proved non-vacuous by stubbing the setter to a no-op: that fails exactly one test — the reduced-motion one — and restoring it passes. Without that check the test would have been asserting against a flag nothing sets, which is precisely the way a split can silently disarm a test. Behaviour is otherwise preserved, checked rather than assumed: the 30 test titles across the two files are an exact set match with the 30 in the single file, and every body is byte-identical apart from the setter call. Harness prune, in the same revision because it is the same debt: with the four `<details>` disclosure tests gone, `export const domWindow = dom.window` had no consumer anywhere in `desktop/src` — it existed only for `new domWindow.Event("toggle")`. Dropping just the `export` leaves an unused local that Biome flags (`lint/correctness/noUnusedVariables`), so the declaration and its now-false doc comment both go (576 -> 571). `TRIGGER_TITLE` only loses its `export`; it is still used inside the harness. Ratchet with #6736's rules cherry-picked, run in the same shell as `git rev-parse HEAD`: - base `fd2e01799` (the current PR base, which is what CI resolves — `resolveBaseRef` returns `HEAD^1` under `GITHUB_ACTIONS`): exit 0. - base `merge-base(origin/main)` = `db5617dd1`, which is what it resolves to locally and would resolve to if C retargets: exit 0. - negative control at the pre-split tree, same rules, same base: exit 1, `AgentSessionWorkBlock.test.mjs: new -> 1146 lines (allowed 1000)`. The gate is measuring the thing this commit fixes. Both suites green together, 30/30. `conversation` + `conversationChrome` still green. `tsc --noEmit` clean, `pnpm check` findings identical to main's baseline. The split shape, the seam and the reduced-motion proof are dev-01's, handed over as a verified patch (`OUTBOX/C_WORKBLOCK_SPLIT_HARNESS_PRUNE.patch`, sha256 6cfbccc9…) rather than as a commit in this checkout. I read the seam, reconstructed the tree from `fe57b7ac7` + that patch to confirm it is exactly what was proposed, and re-ran the title/body comparison, the setter mutant, both ratchet bases and the negative control here. Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AGENTS.md:608states the 1000-line ceiling is "enforced across Desktop, Web, and Mobile by the repository-leveljust file-size-checkgate (just check, CI, and every pre-push)." For.mjsfiles it is not.The desktop and web rule tables listed only
.ts/.tsxfor their script roots, andrunFileSizeCheckskips any file whose extension is absent from its rule's allowlist:This is not a path oversight — the governed roots are correct and
src/featuresis already covered..mjsis filtered out inside roots the ratchet already walks. Desktop's test suite is*.test.mjsby convention and its shared test rigs (observedUnreadTestHarness.mjsand friends) are plain.mjsmodules, so 545 files under already-governed roots sat outside the ceiling.The failure mode is silent and indistinguishable from success: an uncovered file makes the check exit 0 exactly like a clean one. Found when review caught a 1253-line test file on #6720 that
just file-size-check,just check, the pre-push hook, and CI had all passed. Refs #6726.The gap, demonstrated on a clean
maincheckoutChange
Add
.mjsto the existing extension sets for the roots already governed indesktop/scripts/check-file-sizes.mjsandweb/scripts/check-file-sizes.mjs. No new roots and no change to the limit. Rust (.rs) and CSS roots are untouched.The rule tables move into a
file-size-rules.mjsmodule per project so the tests can assert the configuration the runners actually execute rather than a restatement of it. The runners stay unconditional — a module that both exports its rules and self-guards its own execution can silently stop gating, which is the same failure class this PR closes.No splits required: the gate is a ratchet, not an absolute bound
allowedLineCountreturnsbaseLineswhen the base file already exceeds the max, so an inherited oversize file is grandfathered at its committed size and may only hold or shrink:New files are still held to 1000 from birth. That is exactly the discipline
AGENTS.md:608already claims.Grandfathered debt now visible to the gate
Twelve
.mjsfiles under governed roots exceed the ceiling. Counts arecountLinessemantics — what the gate itself prints, which counts a trailing newline as a final line:desktop/src/features/agents/activeAgentTurnsStore.test.mjsdesktop/src/features/community-members/useCommunityJoinAlerts.test.mjsdesktop/src/features/agents/ui/agentSessionTranscript.test.mjsdesktop/src/features/agents/ui/agentSessionTranscriptGrouping.test.mjsdesktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjsdesktop/src/features/agents/ui/createAgentLocalModeGate.test.mjsdesktop/src/shared/ui/markdown.test.mjsdesktop/src/features/messages/lib/useDrafts.test.mjsdesktop/src/shared/api/relayReconnectReplay.test.mjsdesktop/src/features/agents/ingestArchivedObserverEvents.test.mjsdesktop/src/features/terminal/terminalBannerWave.test.mjsdesktop/src/features/terminal/TerminalSubstrate.test.mjsEach is now pinned at that number. None may grow; all may shrink. Web has no
.mjsunder its governed roots today, so its rule change is purely forward-looking.Verification
The ratchet claim, tested against all twelve at once on a clean checkout — not reasoned from the source:
The original 1501-line
.mjsprobe now exits 1, and the same file at exactly 1000 lines exits 0 — the ceiling moved onto.mjswithout tightening for anyone.Three new tests, each proven non-vacuous by reverting the thing it covers, in isolation:
.ts/.tsxdesktop root src/app.ts/.tsxweb root src/appallowedLineCount→ alwaysmaxLinesThe two behavioural tests drive the real rule table against throwaway git repositories in a child process, so a leaked
process.exitCodecannot mark the suite failed.Two vacuity traps in the harness itself, worth naming because they are the same shape as the bug — a check that cannot distinguish the outcome it reports from a failure to reach it.
First: the guard asserted the exit status was 0 or 1, assuming a crash produced neither. Node exits 1 for an uncaught exception, so a broken harness was indistinguishable from a detected violation.
Second, caught in review: requiring the violation report proved the gate started reporting, not that it finished. A child that printed the report and then crashed before returning still read as a completed policy decision. The eval script now writes a completion sentinel to stdout after the awaited call returns, and the harness requires it on both the exit-0 and exit-1 paths — the sentinel proves normal completion, and only then does the exit status carry the policy result.
Proven against the case the previous guard missed. Injecting a throw after the full violation report is emitted but before the gate returns:
A throw injected immediately after the heading fails with the same assertion rather than being misattributed to a missing violation line, and pointing the child at a nonexistent rules module fails the same way instead of passing.
The allowlist test also asserts it found TypeScript roots at all, so a future rule-table reshape cannot make it pass by iterating an empty list.
Gates at the committed head
b9d77258d, clean tree, same shell:node --test scripts/check-file-sizes-core.test.mjs— 9 passing / 0 failing (6 pre-existing + 3 new)5434 passing / 0 failing— not scoped; unchanged by this PR, since these tests live inscripts/tsc --noEmitclean;biome checkat main's baseline (2 warnings / 2 infos); px-text and pubkey-truncation cleanfile-size-checkamong themFollow-up not in this PR
mobile/scripts/check-file-sizes.mjsgovernslibfor.dartonly. Whether Dart-adjacent tooling files there deserve coverage is a separate judgment for mobile owners, so I left it alone.