ci: detect a dead EXPORT, and make the 84 existing ones honest (#9852) - #9858
Merged
Conversation
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 51a4852 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 03:08 PM |
Bundle ReportBundle size has no change ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9858 +/- ##
==========================================
- Coverage 91.65% 91.65% -0.01%
==========================================
Files 916 916
Lines 112790 112789 -1
Branches 27097 27097
==========================================
- Hits 103375 103374 -1
Misses 8126 8126
Partials 1289 1289
Flags with carried forward coverage won't be shown. Click here to find out more.
|
`check-dead-source-files.ts` catches a dead FILE and its own header names what it does not catch: "no detection of a file that's imported but whose EXPORTS are all unused". That gap was populated -- 84 `src/**` symbols were exported and referenced nowhere outside the file declaring them, not by another module, not by a script, not even by their own test. Each was one of three things and nothing told them apart: a missing wire-up (#9492's class one level down), a safety net with nothing behind it (#9851 exactly -- two route-spec tables exported "for the meta-test", test never written), or dead surface that still has to be read and maintained. Coverage cannot catch any of them, because the declaring file's own tests exercise the symbol directly and report green while it contributes nothing to the running system. The fix is almost never deletion. 82 of the 84 are used INSIDE their file, so only the `export` keyword was untrue -- dropping it makes the surface honest, and tsc proves the safety of every one (if any had really been reachable from elsewhere, the build would say so; it is clean). The two exceptions, each decided by reading rather than by rule: - `DEFAULT_REVIEW_CONFIDENCE` is deleted. Its doc claims it is "shared by the parser and the combiners so the fallback is identical everywhere" -- nothing references it, and the constant declared directly below it (#8833) replaced the fallback it describes. Stale, and its comment actively misleads. - `impactMapQueryCache` is allowlisted with the reason: check-schema-drift.ts PARSES this file's sqliteTable declarations rather than importing the symbol, and the table's reads are raw SQL, so deleting the declaration would blind that check to the table. The check is wired into test:ci and mirrors its sibling's conventions exactly: pure over injectable seams, and an exception must state WHY rather than being a bare name on a list.
JSONbored
force-pushed
the
ci/dead-export-check-9852
branch
from
July 29, 2026 15:05
f8b3ef1 to
51a4852
Compare
This was referenced Jul 29, 2026
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
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.
Closes #9852.
The gap
check-dead-source-files.tscatches a dead file, and its own header names what it does not catch:That gap was populated: 84
src/**symbols were exported and referenced nowhere outside the file declaring them — not by another module, not by a script, not even by their own test.Each was one of three things, and nothing told them apart:
Coverage cannot catch any of them: the declaring file's own tests exercise the symbol directly and report green while it contributes nothing to the running system.
The fix is almost never deletion
82 of the 84 are used inside their own file — only the
exportkeyword was untrue. Dropping it makes the surface honest, andtscproves each one: if any had really been reachable from elsewhere, the build would say so. It is clean.The two exceptions were decided by reading, not by rule:
DEFAULT_REVIEW_CONFIDENCEis deleted. Its doc claims it is "shared by the parser and the combiners so the fallback is identical everywhere" — nothing references it, and the constant declared immediately below it (gate: move every objectively-checkable criterion from LLM judgment to deterministic checks #8833) replaced the fallback it describes. Stale, and the comment actively misleads a reader.impactMapQueryCacheis allowlisted, with the reason.check-schema-drift.tsparses this file'ssqliteTabledeclarations rather than importing the symbol, and the table's reads are raw SQL — deleting the declaration would blind that check to the table.The check
Mirrors its sibling's conventions deliberately: pure over injectable seams, wired into
test:ci, and an exception must state why rather than being a bare name on a list (STAGED_AHEAD_OF_CONSUMERSis the precedent — "remove the entry once the seam gains its first real consumer").Types are out of scope on purpose: an unused type costs nothing at runtime and
noUnusedLocalsalready covers the local case. It is textual like its sibling, so a symbol reached only through a namespace import or a dynamic string is a false positive — which is what the allowlist is for.Validation
npx vitest run --changed=origin/main: 25332 passed. (One ui-kit pack test fails on an unbuilt workspace and passes afternpm run build --workspace @loopover/ui-kit— its own failure message says so; unrelated.)internalUsesreporting, references from any root, the allowlist, types ignored, and that a substring is not mistaken for a reference.tscclean across all 52 changed files.