test(exports): check every implementation key and cross-sibling condition order - #753
Merged
sroussey merged 1 commit intoAug 13, 2026
Conversation
sroussey
force-pushed
the
claude/wonderful-turing-rjtcnx-ai-types
branch
from
August 13, 2026 03:55
d86050a to
f8a9718
Compare
Two coverage gaps in the exports guard. Both are latent — no workspace manifest
violates either rule today (168 branches across 38 manifests, 0 new violations),
which is exactly why they went unnoticed.
Only the first implementation key per object was examined.
`IMPLEMENTATION_KEYS.find(...)` stopped at `import`, and the recursion skipped
string-valued implementation keys, so a flat dual-package object
`{types, import: "./a.js", require: "./a.cjs"}` never had its `require` paired
against anything — a `.cjs` declared by a `.d.ts` sailed through. The file's own
`.cjs` fixtures use the nested `require: {types, default}` form, which is what
hid it. Now every string-valued implementation key yields a branch.
That makes one object produce several branches, which would collide in `label()`
— the key for ALLOWED_MISMATCHES and the staleness check — so the
implementation key joins the branch identity and the label reads
`[condition > key]`. The shorthand form keeps its bare `[condition]`: its value
IS the implementation, so there is no key to name. ALLOWED_MISMATCHES ships
empty, so no allowlist migration is needed; only failure text changes.
Condition ORDER across sibling keys was never checked. `typesBeforeImplementation`
compares indices within one object, so a map whose branches are each internally
well formed but ordered `{types, browser: {…}, import}` passes every existing
check while TypeScript matches the outer `types` and never looks at `browser` —
the browser-typed-as-node bug this file exists to prevent, expressed through
ordering rather than through a wrong target. `{import, browser: {…}}` is the
runtime equivalent. Adds `orderViolations`, covering object and string-shorthand
condition keys alike, and asserts it over every workspace manifest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
sroussey
force-pushed
the
claude/exports-guard-order-and-multi-impl
branch
from
August 13, 2026 03:55
f04ccc5 to
422fe8c
Compare
sroussey
merged commit Aug 13, 2026
c32a28f
into
claude/wonderful-turing-rjtcnx-ai-types
10 of 11 checks passed
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.
Based on #717 (retarget to
mainonce that merges). Both branches rebased ontoorigin/main(67bed681). Deliberately not combined with the manifest changes already on #717 — those are verified correct and this is purely about the guard that protects them.Rebase note
#717 was 94 commits behind. It rebased onto current
mainwith zero conflicts (4 replayed commits), confirming the earlier merge-preview prediction — including thatmainhad independently landed the byte-identical./workertypes change, which git resolved silently. This branch was then rebased onto the updated #717. The guard suite was re-run against the post-rebase manifest set, which matters becausemainmoved 94 commits: 20 passed, with the new workspace-wide order check finding 0 violations across today's real manifests.What
Two coverage gaps in
ExportTypesPairing.test.ts. Both are latent: no workspace manifest violates either rule today, which is precisely why neither was noticed.1. Only the first implementation key per object was examined.
IMPLEMENTATION_KEYS.find(...)stopped at the first match, and the recursioncontinued past string-valued implementation keys. So a flat dual-package object —{ "types": "./dist/a.d.ts", "import": "./dist/a.js", "require": "./dist/a.cjs" }— had
requirechecked by nothing at all. A.cjsimplementation declared by a.d.ts(which TypeScript does not honour:.cjswants.d.cts) passes silently. The file's own.cjsfixtures use the nestedrequire: {types, default}form, which is exactly what kept the flat shape from ever being exercised.2. Condition order across sibling keys was never checked.
typesBeforeImplementationcompares indices within one object. A map whose branches are each internally well formed but ordered wrongly passes everything:{ "types": "./dist/node.d.ts", "browser": { "types": "./dist/browser.d.ts", "import": "./dist/browser.js" }, "import": "./dist/node.js" }Every
typeshere names the right target beside the right implementation, soviolationsis[]andlateis[]— while TypeScript matches the outertypesfirst and never looks atbrowser. That is the browser-typed-as-node bug this file exists to prevent, expressed through ordering instead of a wrong target.{import, browser: {…}}is the runtime equivalent: resolution stops atimport, so the browser build never loads.Why this fix
MEDIUM-2 landed first within the PR because it changes
label(), which MEDIUM-1's fixtures quote.Collecting every string-valued implementation key means one object can now yield several branches, which would collide in
label()— the key for bothALLOWED_MISMATCHESand the staleness check. SoimplementationKeyjoins the branch identity and the label reads[condition > key]. The string-shorthand form keeps its bare[condition]: there the condition's value is the implementation, so there is no key to name, and typing itstring | undefinedsays that rather than inventing one.ALLOWED_MISMATCHESships empty, so no allowlist migration — only failure-message text changes.orderViolationswalks each subpath object recursively and reports any key that is neithertypesnor a string-valued implementation key when atypesor an implementation string precedes it. The string-shorthand form is included (browser: "./dist/x.js"aftertypes): it is just as dead as the object form, and the file's existing fixtures prove this repo writes that shape.Tests
20 pass (was 15, then 13 on the rebased base). New fixtures:
{types, import, require}→ one message namingrequire,./dist/a.cjs, expected./dist/a.d.cts[]){types, import, default}where both agree →[](extra branches must not become noise)types-before-nested-browser→ reported, andfindViolationsasserted[]explicitly{import, browser:{…}}→ reportedtypes→ reported (both reasons){browser:{…}, types, import}→[]Plus a new workspace-wide assertion: every manifest must declare each condition before the siblings that would shadow it.
The
findViolations(...) === []assertion inside the order fixture is deliberate. It documents why a second check is needed and is the permanent, inline demonstration that the existing rule is blind to this shape.Actually executed (this is the part the plan could only reason about):
browser,bun,react-native..slice(0, 1)): 1 failed / 19 passed — the flat dual-package fixture.orderViolationsto return[]: 3 failed / 17 passed — exactly the three positive order fixtures.[browser > import],[require > default]); the> 50branch-count floor still holds, since branch count grows only where a second implementation key exists — nowhere today.prettier --checkclean.Risk / blast radius
Test-only. Nothing ships to consumers. No manifest is modified by this PR.
Two things a reviewer must accept:
[browser]becomes[browser > import]. Harmless today (ALLOWED_MISMATCHESis empty) but it would invalidate allowlist entries if any existed, so future entries must use the new form.types, fails CI where it previously passed. That is the intent.Unverified
use-sourcemode. This file only readspackage.jsonfiles off disk, so neither should matter.> 50vacuity floor cannot detect a whole workspace GROUP disappearing (workspaceManifestscontinues on a missing group dir;workspaceRootdrops anyworkspacesglob still containing*after the/**strip). It is real, but it is not a rider on either fix here. The one-line follow-up is to assert a non-zero count per group.