Repository navigation
v0.11.2 — adopter-type gate checks each page member against its own source - #2
Merged
Merged
Conversation
platform-parity compared Android's copy of a method against iOS's, which gave it two blind spots: a declaration wrong the same way on both platforms had nothing to disagree with, and the 13 of 32 page members that exist on one platform only were never compared at all. Two rules that read src/ instead of the built .d.ts close both, because the evidence lives in the body a .d.ts drops: - forwarded-default: a parameter handed straight to another of our functions at a position that function defaults, but restated without a default here. - optional-in-fact: a required parameter whose every use sits on the left of `||`/`??` — the body already handles its absence. Both give an absolute notion of wrong, so neither needs a counterpart. Validated against real defects, not fixtures built to pass: - c4d60ce (pre-v0.11.1) -> forwarded-default fires on page.swipe(), from source alone. - HEAD with the swipe default removed on BOTH pages -> rules 1+2 report "OK"; forwarded-default still fires. That is blind spot one, demonstrated. - HEAD as shipped -> optional-in-fact fired on iOS-only page.unlock(pin), which no parity check could ever see. That is blind spot two, and it was a live defect: `unlock(pin: any)` forced adopters to pass a passcode connect() already held. Fixed here; a consumer calling page.unlock() got TS2554 before and compiles clean after, verified against a packed types tree. - Excluding forwards into builtins (`Number(ref)`) is what took forwarded-default from 1 false positive to 0 on the real tree. Residual gap, stated rather than papered over: this is absolute for parameter optionality — the class that shipped twice — but a return type wrong on both platforms is still only caught where tsc catches it. A lie laundered through a cast, as screenshot() once was, is not detected. 321 tests pass, typecheck clean, check:types green (35 of 51 page members carry parameters and are now checked against their own source). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RYCF6KZZp594eU7Rj8w3i
Code review found both new rules identify a parameter by raw identifier text, so any nested shadow of that name corrupts the analysis. Reproduced all four before fixing, with a negative control alongside so the fixes could not pass by simply neutering the rules: - forwarded-default false positive: a page method `f(duration)` whose inner closure shadows the name (`list.forEach(duration => interact.swipe(...)))` blamed the OUTER parameter, which is never forwarded — CI would have demanded a nonsensical default. - optional-in-fact false negative: a genuinely fallback-only `pin` was masked when an inner arrow reused the name non-fallback, so the rule missed the exact defect class it exists to catch. - forwarded-default false positive via spread: the syntactic argument index was used as the callee's parameter index, so `interact.swipe(0,0,0,0, ...pts, x)` compared `x` against `opts`. A spread contributes an unknown number of arguments; nothing can be concluded, so it is now skipped. - ownership filter: `startsWith(srcDir)` has no path boundary, so a sibling `src-legacy/` counted as ours. Built one, watched it produce a false positive, added `sep`, watched it stop. Both rules now key parameters by symbol via the checker, which resolves scope properly. Rule 4 also carries parent pointers down its own walk rather than reading `node.parent`, which existed only as a side effect of having built the type checker. Probes after the fix: the two shadowing probes and the spread probe are silent, and the plain real defect still fires. Regression, all re-run against the changed checker: both-platform swipe still caught; reverted unlock fix still caught; real c4d60ce still 2 findings; reverted screenshot type still caught by platform-parity; c006cac still exactly 3 bare-object; lost anchor still exit 2. 321 tests pass, typecheck clean, check:types green at 35 of 51 members. Declined one review finding, recorded in the header rather than silently: rule 4 would misread a throwing fallback (`pin || required()`) as a default. No such code exists in either page, and the failure mode is a visible false positive a human dismisses, not a wrong type shipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RYCF6KZZp594eU7Rj8w3i
Found while running the release gates, not by the review. Switching rule 4
to symbol identity introduced a false positive the old text match did not
have: in `post({ pin })` the identifier resolves to the PROPERTY symbol,
not the parameter, so a genuinely-required parameter that is forwarded as
a shorthand property read as fallback-only.
Probed it first — a page method with `const c = pin || passcode` plus
`wdaPost('/probe', { pin })` was wrongly reported optional-in-fact — then
resolved shorthand uses through getShorthandAssignmentValueSymbol and
watched the probe go quiet.
The four earlier probes still behave (both shadowing probes and the spread
probe silent, the plain defect still caught), and the six regression
scenarios are unchanged: both-platform swipe, reverted unlock, real
c4d60ce, reverted screenshot type, c006cac at exactly 3 bare-object, lost
anchor exit 2. 321 tests pass, typecheck clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RYCF6KZZp594eU7Rj8w3i
…wn source CHANGELOG entry for the two new rules and for the iOS `page.unlock()` declaration fix they surfaced, plus two corrections to the existing Unreleased entry, which this branch falsified: it claimed the bare-object rule fires 4x on c006cac (measured: 3, both before and after this branch), and it stated the platform-parity blind spot as a standing known gap, which rules 3 and 4 now close for parameter optionality. Patch, not minor: the only adopter-visible change is a declaration widening (`unlock(pin: any)` -> `unlock(pin?: string | null)`), which is the same shape as v0.11.1. Everything else is CI-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RYCF6KZZp594eU7Rj8w3i
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.
What
platform-paritycould only find disagreement between the Android and iOS pages, which left two blind spots: a declaration wrong the same way on both platforms had nothing to contradict it, and the 13 of 32 page members that exist on one platform only were never compared at all (19 of 32 covered).Two rules now read
src/instead of the built.d.ts, because the evidence lives in the function body a.d.tsdrops:forwarded-default— a parameter handed straight to another of our functions at a position that function defaults, but restated without a default here.optional-in-fact— a required parameter whose every use sits on the left of||/??, where the body already handles its absence.Both judge a member against its own implementation, so they give a notion of wrong rather than only these two disagree, and single-platform members are covered.
Validated on real defects, not fixtures
forwarded-defaultfires onc4d60ce'spage.swipe()from source alone, no cross-platform comparison in play.forwarded-defaultstill fires — the first blind spot, demonstrated.optional-in-factfired on shippedHEADagainst iOS-onlypage.unlock()— a live defect no parity check could see.unlock(pin: any)forced adopters to pass a passcodeconnect()already held; a consumer callingpage.unlock()gotTS2554before and compiles clean after, verified against a packed tarball. Fixed here.Review findings, all reproduced before fixing
Parameters are resolved by symbol rather than name text (a nested closure shadowing a name was both misattributing findings and masking real ones), shorthand property uses (
post({ pin })) count as real uses, an argument after a spread is skipped since its callee position is unknowable, and the ownership filter respects a path boundary. A negative-control probe fires throughout, so none of these fixes passed by neutering a rule.One finding declined and recorded in the script header rather than silently:
optional-in-factwould misread a fallback that itself throws. No such code exists in either page, and the failure mode is a visible false positive, not a wrong type shipped.Verification
321 tests pass,
typecheckclean,check:typesgreen at 35 of 51 page members. Six regression scenarios re-run against every change: both-platform swipe, reverted unlock fix, realc4d60ce, reverted screenshot type,c006cacat exactly 3bare-object, lost anchor exit 2.Patch, not minor: the only adopter-visible change is a declaration widening. Everything else is CI-only.
🤖 Generated with Claude Code