Skip to content

refactor(lint): retire the facade closure test for a budget row and a lint rule - #1990

Merged
thymikee merged 1 commit into
test/1960-eager-closure-budgetsfrom
test/1960-drop-redundant-hub-pin
Aug 24, 2026
Merged

refactor(lint): retire the facade closure test for a budget row and a lint rule#1990
thymikee merged 1 commit into
test/1960-eager-closure-budgetsfrom
test/1960-drop-redundant-hub-pin

Conversation

@thymikee

@thymikee thymikee commented Aug 24, 2026

Copy link
Copy Markdown
Member

Stacked on #1965 — merge that first. (Also blocked on #1965's open tracked-manifest discovery finding.)

Retires contracts-entry-closure.test.ts (added in #1959). It carried two assertions. Neither replacement subsumes it alone — together they do, on two different axes.

1. Its whole-tree scan → a lint rule

It parsed ~490 candidate files to prove nothing value-imports the two wide façades. eslint/no-restricted-imports states exactly that, and allowTypeImports draws the one distinction that made a custom walker look necessary: import type is erased, so it stays legal.

The rule is already in this repo's toolchain — oxlint 1.79, and the same rule already bans node:child_process outside exec.ts. It improves on the deleted test in two ways: it covers every file rather than the four hubs the test named, and it reaches dynamic imports too. It also moves the feedback from a CI run into the editor.

2. Its hub pin → the lint rule, plus #1960's rows on a second axis

The hub pin checked that four named hubs never reach either façade. The lint rule forbids the import edge that was the only way that could happen. #1960's rows then add an axis the old test never had: closure size drift.

Correction from the previous revision of this description, which claimed the budget was "strictly stronger" and caught "any closure change". That is wrong, and thymikee is right to reject it: a cardinality budget cannot see an equal-size graph substitution — swap one module for another of the same closure size and the count is unchanged, so the row passes. Forbidden edges and size drift are different properties. Each gate owns one; the pair covers what the deleted test covered.

The hub list is deliberately not restored — the lint rule already covers those four files along with everything else.

Why this needed per-zone proof

Override semantics are not additive — a same-rule override replaces the parent. Two traps followed:

So the paths are added per zone, and that "off" becomes a façade-only config that keeps the node:child_process exemption it existed for.

Planted red in all three zonessrc/core/capabilities.ts, a src/__tests__ file, and packages/capture-kit/src — each flagged, while a type-only import in the same probe file was not. The first packages probe read as a pass because the sed that built it produced a type-only import; that zone was re-probed with a real value import rather than trusting the green.

Relying on config is safe here because misconfiguration fails loudly: a typo'd rule name makes oxlint exit 1 with Rule 'x' not found in plugin, rather than pass silently the way the rg assertions in #1976 did.

What a linter cannot replace

The eager-closure budgets stay. They express a transitive weight property: a module already imported grows an import, and the cost arrives without any single file's import list changing. Per-file rules cannot see that, and no-restricted-imports can only ban specifiers named in advance — precisely what #1950, #1956 and #1959 could not have named ahead of time.

Verification

pnpm lint clean, pnpm typecheck clean, budget gate green (207 tests). Net −98 lines of test, +59 lines of config. One iOS Smoke Tests job failed on a live-simulator wait timed out for text and passed unchanged on re-run — this branch changes no production code.

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.39 MB 2.39 MB 0 B
JS gzip 801.1 kB 801.1 kB 0 B
npm tarball 923.3 kB 923.3 kB 0 B
npm unpacked 3.20 MB 3.20 MB 0 B

npm unpacked components

Component Base Current Diff
JS / dist source 2.54 MB 2.54 MB 0 B
Apple runner source/project 564.3 kB 564.3 kB 0 B
macOS helper source 54.5 kB 54.5 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 44.5 kB 44.5 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.8 ms 27.6 ms -0.2 ms
CLI --help 79.5 ms 78.9 ms -0.6 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

No changed packed files.

@thymikee

Copy link
Copy Markdown
Member Author

Small correction before approval: deleting the four-hub reachability test is safe, but the new comment/PR wording must not call the exact-size budget strictly stronger or say it fails on any closure change. The budget pins only cardinality (actual === budget), so an equally sized graph substitution can newly reach a wide facade without moving the count. What actually subsumes the deleted case is the combination: exact budgets catch closure-size drift for the named hubs, while the retained whole-tree rule catches forbidden facade value imports anywhere (with its non-vacuity guard). Rewrite the ownership claim accordingly; do not restore the redundant hub list. #1990 is otherwise clean/green, but remains stacked on #1965, which currently has a separate review finding.

@thymikee
thymikee force-pushed the test/1960-drop-redundant-hub-pin branch from 9bd2080 to d03db8d Compare August 24, 2026 09:01
@thymikee thymikee changed the title test(contracts): drop the hub pin the budget ratchet now subsumes refactor(lint): retire the facade closure test for a budget row and a lint rule Aug 24, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Exact head d03db8d is code-clean but not ready to advance. The deletion is sound: the lint rule now owns forbidden direct value imports across src and package sources (including dynamic imports, while preserving type-only imports), and #1965's exact rows own transitive closure-size drift for the four named hubs. However, the PR body and commit message still incorrectly call the cardinality budget strictly stronger and say it catches any closure change; an equal-size graph substitution disproves that. Rewrite the claim to say the lint rule and budgets jointly subsume the old test—do not restore the hub list. The PR also remains blocked on #1965's unresolved tracked-manifest discovery finding, and one iOS Smoke job is still running.

@thymikee

Copy link
Copy Markdown
Member Author

Code delta is clean at c007973: the description now correctly separates the lint rule’s forbidden-edge property from the budgets’ closure-size property, and the rule covers all effective config zones while allowing erased type-only imports.

Branch blocker: this head still contains old #1965 head 7815272, while current #1965 is 2baf7ef; GitHub correctly reports the stack conflicting. The failed Coverage job is the same stale-base symptom (missing current contract rows and exact pins), not a new lint-design failure. Restack/rebase #1990 onto current #1965, rerun CI, and retain only the #1990 lint/deletion delta. Not merge-ready until synchronized.

…udget rows

`contracts-entry-closure.test.ts` (from #1959) carried two assertions. Neither
replacement subsumes it alone; together they do, on two different axes.

Its whole-tree scan parsed ~490 candidate files to prove nothing value-imports
the two wide facades. `eslint/no-restricted-imports` states exactly that, and
`allowTypeImports` draws the one distinction that made a custom walker look
necessary: `import type` is erased, so it stays legal. The rule covers every
file rather than the four hubs the deleted test named, and it reaches dynamic
imports too.

Its hub pin checked that four named hubs never reach either facade. The lint
rule forbids the import edge that was the only way that could happen, and
#1960's rows add an axis the old test never had: closure SIZE drift. That
cardinality check is deliberately not described here as strictly stronger,
because it is not — an equal-size graph substitution leaves the count intact and
passes. Size drift and forbidden edges are different properties, and the two
gates own one each.

The override semantics are not additive, so the rule was verified per zone
rather than assumed. A same-rule override REPLACES the parent, so a top-level
rule would have been silently dropped for `src/**`; and the existing blanket
`"off"` for `exec.ts` and the test tree would have exempted the files that
carried most of the cost #1959 removed. The paths are therefore added per zone,
and that `"off"` becomes a facade-only config that keeps the
`node:child_process` exemption it existed for.

Planted red in all three zones — `src/core/capabilities.ts`, a `src/__tests__`
file, and `packages/capture-kit/src` — each flagged, while a type-only import in
the same probe file was not. The first packages probe read as a pass because the
sed that built it produced a type-only import; that zone was re-probed with a
real value import rather than trusting the green.

Relying on config is safe here because misconfiguration fails loudly: a typo'd
rule name makes oxlint exit 1 with "Rule not found in plugin", rather than pass
silently the way the `rg` assertions in #1976 did.

The budgets stay for what no linter can express: a transitive weight property,
where a module already imported grows an import and the cost arrives without any
single file's import list changing. Per-file rules cannot see that, and
`no-restricted-imports` can only ban specifiers named in advance — exactly what
#1950/#1956/#1959 could not have named.
@thymikee
thymikee force-pushed the test/1960-drop-redundant-hub-pin branch from c007973 to 2db6086 Compare August 24, 2026 09:56
@thymikee
thymikee merged commit c34d27e into main Aug 24, 2026
27 of 28 checks passed
@thymikee
thymikee deleted the test/1960-drop-redundant-hub-pin branch August 24, 2026 10:40
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-24 10:41 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant