-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5478
cixzhang · merged 553b07f126c1 · view on GitHub
Verdict: approve — no review posted (GitHub forbids self-review); merged at the reviewed head
Two rounds, both read-only on the PR. Round 1 approved with two non-blocking findings and one of them was fixed in round 2; round 2 re-verified the corrected head and lifted the hold. The approval could not be posted, and the PR merged anyway — see the outcome block below, which is the part of this record worth reading twice.
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12. Read at gate 1 of each round and held through both critic passes.
| Reviews posted on the PR | none — the reviews endpoint returns 0, and the only comments are Vercel and the PR Analysis Report |
| Why |
GitHub refuses --approve from the PR's own author. The loop drafts in the maintainer's voice and posts as her account, and she wrote this PR, so the platform rejects the approval as a self-review. This is a mechanical block, not a change of verdict |
| Merged |
553b07f126c1ea7756624ddea3e37bf34ba83ce5, squash, 2026-08-26 14:48 PT — on main
|
| What authorised the merge | the full loop returned approve; every required check was green on the merged head; branch protection requires 0 approving reviews, so no review was owed |
| Merged head vs reviewed head | the same commit — f492a1c6 is what round 2 reviewed and what was squashed |
The distinction this record exists to preserve: the review was completed and unposted, not skipped. A future reader looking at #5478 on GitHub sees a PR that merged with no review on it, and nothing there says one was done. This page is the only place that fact lives.
It is also the shape worth noticing. A self-authored PR cannot receive the loop's approval through GitHub at all, so on this class of PR the loop's verdict is invisible on the artifact and the merge rests on CI plus the verdict recorded here. Where an outside contributor's PR carries its review in public, a maintainer's own carries nothing.
Lane. full. Fast was unavailable: measurement found a user-facing divergence beyond the stated one, a measured cost, and a need for more than one decisive check — each is a promotion trigger on its own.
Frame. Head repo facebook/astryx, so the code is trusted; the diff is one
changeset, three .mjs files and one colocated test, with no package.json,
lockfile, workflow, script or dependency change. Author bucket: the maintainer
herself, also in .github/ENGOWNERS. Base 299077f204c8, merge-base
b0a09a630f5c. No prior review existed, so there was nothing to reconcile or
contradict.
Main had not invalidated it. All three resolver files and both cited test
files were byte-identical between the merge-base and main at the time, and
main had added no new existence probe under the CLI package in the 42 commits
since the base.
A builder on a Mac types astryx component button, gets Button's documentation,
and never learns the component is called Button. Their teammate on Linux runs
the same command and gets an error. Worse, astryx hook mediaquery hands back a
doc path spelled useMediaquery.doc.mjs — a filename that exists on no disk
anywhere. It reads back fine on the Mac that produced it and fails the moment
anything case-sensitive touches it.
Two of the repository's own tests already encode the correct behaviour and fail
on a clean main on macOS, so the second person hurt is the next maintainer, who
sees a red suite on checkout and has no reason to believe it is real.
VERDICT: clear
Asking a Mac "does this file exist?" is asking a question the Mac answers generously: it ignores capitalisation, so it says yes to a name nobody ever used. The fix stops trusting that answer. After the quick yes it reads the actual list of names in each folder along the way and checks the spelling really is there. A folder listing is the same on every operating system, so every machine now agrees on which names exist and what they are really called. The deliberate "I'll forgive your capitalisation" hook lookup still forgives it — it just answers with the file's real name instead of the one the caller typed.
1. name-derived path probes verify each segment against its parent's
real directory listing instead of trusting the filesystem's own answer [the fix]
One decision, tracing to the stated problem. The changeset, the four new helper tests and the JSDoc are how the decision is evidenced, not further decisions.
Unrecorded-change audit: clean. Every change has a stated reason. The body quotes both failing tests, names the mechanism, and carries a measured before/after table for two probes. Nothing here required the reviewer to construct an argument the PR had not made.
Early hypothesis, recorded before the slots: everything I find will be a child of "the fix is scoped to three resolvers, not to the class of probe".
VERDICT: clear
OWNER: the CLI's path layer
TIER 1: none · TIER 2: none
SEAMS: the three name resolvers, and every other caller that builds a path from
a user-typed name and asks the filesystem whether it exists
BEHAVIOR UNIT: pure utility — two strings in, a boolean out, with four colocated
tests driving it directly
The cheaper mechanism was tested and rejected, not assumed away. Resolving the real path in one call recovers the true casing and looks like the better answer:
asked : …/packages/core/src/button/Button.doc.mjs
got : …/packages/core/src/Button/Button.doc.mjs ← the case is recovered
But it also resolves symlinks, and a real pnpm consumer reaches core through
one — apps/sandbox/node_modules/@astryxdesign/core is a link to
packages/core in this repository. Driven through that link, a correctly-cased
path gives:
| probe on a correctly-cased path through the symlink | result |
|---|---|
| real-path comparison | false — a false negative on a valid path |
| the PR's per-segment check | true — correct |
So the per-segment directory read is not the naive choice; it is the correct one. The gap is coverage, not layer.
VERDICT: note — right mechanism, right home; the class of probe is not closed
Everyone running the CLI on macOS or Windows, which is most builders, plus anyone whose checkout is on a case-sensitive volume and getting different answers from their colleagues. On Linux nothing changes at all: the plain existence check already answered case-exactly there, so the added verification can only agree. Nothing renders, so no end user of an app built with Astryx notices anything.
What they get, run against the real component package at the reviewed head:
$ astryx component button
Error: No component named "button"
Button (exact name)
IconButton (keyword "button")
$ astryx component Button # control
# Button …
$ astryx hook mediaquery --json
{ "type": "hook.detail", "data": { "name": "useMediaQuery", … } }
What landing this newly exposes. After this PR two sibling commands disagree about the same typed name:
astryx component button → No component named "button" (fixed here)
astryx swizzle button → [ok] Copied 2 files to <out>/button/
Both are real output at the reviewed head. Before the PR they agreed — both
accepted button. The swizzle owner lookup probes the component directory with
the same unverified existence check, and the PR does not touch that file.
For a person: a builder is told button is not a component, then swizzles it
anyway and gets components/astryx/button/Button.tsx committed into their
repository — a directory named the way they typed it, not the way the component
is named. Their teammate on Linux gets Component "button" not found.
The swizzle half is not new and not this PR's to fix. The disagreement between the two commands is new.
VERDICT: note — two sibling commands answer the same name differently on macOS
No API change. The new helper is internal: the CLI package's exports map
publishes ten entry points and the path module is not among them, only one
foundation/ subpath is exposed at all, and the symbol appears nowhere but the
two resolver modules and its own test — no barrel re-export, no type declaration.
Nothing ossifies, and the human-judgement gate for new capability does not fire: no new prop, export, type, variant, named preset, theme target or opt-in behaviour.
Ossification of the internal addition. The one question worth asking is whether a helper beside a shipped one already does this. The filesystem folder holds a path module and a path-safety module; nothing in either answers "does this exact spelling exist" — the containment guard is containment, not existence. No duplicate spelling, and the new helper sits where the other existence-shaped helpers already live.
VERDICT: clear — no API change; the internal helper duplicates nothing
Structural only. Greps across all five changed files return 0 for every
colour, gradient, shadow, light-dark(), StyleX and inline-style pattern, and
0 for every ARIA, role and translator pattern. No new theme targets; the diff
is Node filesystem code in the CLI and writes no CSS.
VERDICT: clear
There is a consumer who can be broken: the CLI package is not private and is
published, so this is real released surface and the patch changeset is right.
BEHAVIOR: one intended change — a wrong-cased name on a case-insensitive host
stops resolving by accident and takes the normal unknown-name path
API: no — no signature, export or default changed for any consumer
VISUAL: no — nothing renders; the CLI writes text to a terminal
THEME: no — no target, token or override is involved
States walked: exact-cased name (unchanged, both filesystem kinds) · wrong-cased name on a case-insensitive host (the fix) · wrong-cased name on a case-sensitive host (unchanged — the plain check already missed) · the deliberate case-insensitive hook lookup (still resolves, now returns the true casing) · target outside the root and the root itself (both guarded, both newly tested) · empty, loading, error and disabled are not reachable — this is a synchronous path predicate with no state, no default and no prop.
The honest line the three axes have no column for: a CLI answer changed, deliberately, and that is the release note.
VERDICT: clear — one intended behaviour change, correctly described
Effects: zero. No React in this diff. No listener, no observer, no subscription, no timer.
Measured with both arms in one session against the real component package (106 components). Directory reads are the assertion; wall clock is colour only.
Single lookup, mean of 20 runs:
| call | directory reads before | after |
|---|---|---|
| readme, exact name — hit | 0 | 2 |
| source, exact name — hit | 1 | 3 |
| hook doc, exact name — hit | 0 | 1 |
| hook doc, folded name | 0 | 3 |
| readme, wrong case — miss | 0 | 136 |
| source, wrong case — miss | 1 | 132 |
The two miss rows are the cost the PR body already states and states honestly: a miss now falls through to the full-tree scan instead of short-circuiting on a wrong-case hit, which is what every unknown name on Linux has always paid.
The rows the body does not mention are the hit rows, so they were priced at the worst realistic N in the repository. The readme lookup runs once per component on the list and search flows, and the core package holds 106 components. Five order-alternated runs, median:
before 417 directory reads median 117ms (81–224)
after 630 directory reads median 137ms (135–321)
+213 reads (+51%), +20ms
Who pays: the library, not a user — 20ms inside a command whose Node startup alone is several hundred, and the clock spread is wider than the delta, so the count is the honest number. It survives the fix; it is what case-exactness costs. Memoising the directory listing per root for the life of a call would remove most of it, since 630 reads over 106 lookups is a handful of directories read repeatedly. That memo was not prescribed: it was not built, and it carries a cache-invalidation question this review did not answer.
The accept/decline was left with the maintainer rather than spent by the reviewer. The recommendation given was to eat it.
VERDICT: note — +51% directory reads on the list/search flow (+20ms at N=106)
VISUAL CHECK: not applicable
WHY: no rendering path is reachable from this diff. The five changed files are
one markdown changeset and four Node modules in the CLI package — a
filesystem predicate and three name resolvers whose only output is a string
path consumed by terminal formatters. The structural greps return 0 for
every colour, style, string and role pattern. CI agrees by path filter: the
a11y, visual and RTL jobs all SKIPPED on this exact head, and the PR
Analysis Report reports no new or modified components and no component
packages changed. BREAKING · Visual is "no", so the empty-slot kill rule
does not fire.
No frames, and no sensor receipts are owed — nothing was captured because nothing could be. The terminal output quoted under Impact is what stands in for frames.
VERDICT: clear — non-rendering diff, proven by changed paths and three skipped visual jobs
Nothing here is touched, and here is what was checked. The greps return 0 for ARIA attributes, roles, the translator hook and translation keys across all five changed files. No element is added, removed or moved; no focus, keyboard path, live region or announcement exists in a Node path predicate. No user-visible string is added — the only new prose is a JSDoc block and the changeset, neither of which ships to a screen. The English locale catalogue is untouched.
The a11y job SKIPPED on this head, correctly, because no component package changed; the PR Analysis Report reports no accessibility violations, and no baseline entry was added, so no silence was bought. No logical or physical property, no directional glyph, no transform — the RTL job SKIPPED, correctly.
VERDICT: clear — no assistive-technology or user-facing surface in the diff
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | note — class of probe not closed |
| IMPACT | note — two sibling commands disagree on macOS |
| API | clear — no API change |
| THEMING | clear — n/a, structural |
| BREAKING | clear — one intended behaviour change, described |
| PERFORMANCE | note — +51% directory reads, imperceptible |
| VISUAL | clear — non-rendering, proven |
| A11Y & I18N | clear — nothing touched |
Read against the slot bodies, not the labels. Architecture and Impact are one
finding seen twice: Architecture noticed the swizzle probe as coverage, Impact
owns its consequence, and Impact is where the severity is honest. Performance is
independent and imperceptible. Three notes, two findings, and neither reaches a
user who is stuck. The nearest thing to a bug in any slot body is the swizzle
paragraph, and main already does it, the PR does not make it worse, and nobody
is stuck, loses data or cannot get out.
GOAL: partly met. The three named resolvers reach exact parity, measured; the class does not.
The decisive check was a real case-sensitive volume — a case-sensitive APFS disk image, its sensitivity asserted before use, with the ordinary volume confirmed insensitive as the control. An identical fixture tree was built on both and both arms were run against both:
| call | case-sensitive, before | case-sensitive, after | case-insensitive, before | after |
|---|---|---|---|---|
readme, button
|
null | null | src/button/button.doc.mjs |
null |
readme, Button
|
correct | correct | correct | correct |
source, button
|
null | null | src/button/XDSbutton.tsx |
null |
source, Button
|
correct | correct | correct | correct |
hook doc, mediaquery
|
true casing | true casing | useMediaquery.doc.mjs |
true casing |
hook doc, useMediaQuery
|
correct | correct | correct | correct |
hook doc, MediaQuery
|
correct | correct | correct | correct |
readme, card
|
null | null | src/Layout/Container/Card.doc.mjs |
null |
After equals the case-sensitive answer on all eight rows. XDSbutton.tsx is
the clearest single piece of evidence in the run: a path the old code returned
that exists on no filesystem in the world.
And the repository's own tests, both sides, on the real tree:
BEFORE main — 2 failed, exactly the two the body names:
× is case-SENSITIVE: a lowercased name is unknown
× resolves a bare (use-prefix-stripped, case-insensitive) name
AFTER the PR head — 4 files, 65 tests, all passed
partly met rather than met because of the uncovered member: swizzle still
resolves the wrong-cased name on macOS. It can follow separately — one call site,
not a regression, and holding this fix for it would keep two red tests on main.
What could not follow separately was the changeset sentence, which shipped in the
release notes claiming the whole thing.
DISPOSITION.
- The swizzle owner lookup still folds → follow-up, not blocking. Pre-existing, not made worse here, one line to fix with the helper this PR adds. Intermediate state: two commands disagree on a wrong-cased name on macOS — survivable, because the one that now refuses is the one that teaches the right name.
- The changeset claimed "every host resolves the same names to the same real paths" → the ask. True of three lookups, not of the CLI. One clause.
- +51% directory reads → not accepted by the reviewer. Stated, priced, who pays named, remedy named; the decision left with the maintainer.
ADVICE: proven remedy for (1) — the new helper applied to the swizzle probe; it is already imported by two peers and was driven through the pnpm symlink case. The memoisation in (3) was deliberately not prescribed.
AUTHOR CAN PROCEED: yes. The defect is proven and the acceptance criterion is one sentence: swizzle and the component lookup give the same verdict on macOS as on Linux, provable by re-running the case-sensitive-volume probe. No new owner, public API, concept, boundary or accepted cost is required from anyone.
WORST OUTCOME: "a builder is told button is not a component, then swizzles
it anyway and gets components/astryx/button/Button.tsx committed into their
repo … Their teammate on Linux gets Component "button" not found." →
compatible with approve. Nobody is stuck, nothing is lost, no path is
unreachable; the command that changed became louder, not quieter.
JUDGEMENT NEEDED: none — a defect fixed against the existing contract.
Late parent check. The findings share the parent recorded as the early hypothesis — the fix is scoped to three resolvers, not to the class of probe. Architecture and Impact are both children of it; Performance is genuinely independent. The early hypothesis was right, which is the less valuable outcome; the slot that could have refuted it instead closed the new-surface question cleanly.
Second confirmation. Not a blocking finding, so not owed, but done because it was the review's headline: found by reading the swizzle module, confirmed by running the real CLI at the head, and confirmed a third way by the diff not containing that file at all.
1. [not blocking] the swizzle owner lookup probes the component directory
with an unverified existence check
→ `astryx component button` refuses the name and `astryx swizzle button`
copies Button into `button/` — two commands, one typed name, two answers
on macOS
2. [not blocking] the changeset claims the whole CLI
→ whoever reads the release notes concludes CLI name resolution is now
host-independent; swizzle still is not
Pass 1 failed the draft on length and voice, not on substance. The review text came in at 112 words against a 30-word cap for an approve, ended on a closing question — a request-changes device, not an approve one — carried an evidence chain for a finding already accepted, restated the author's own diagnosis back to her, and duplicated the file anchor in both the summary and the inline.
Pass 2 on the rewrite: no violations, and it would have been posted. Summary 23 words, inlines 15 and 10, whole review 48 words against the maintainer's own 52-word benchmark on a much larger PR.
One commit was pushed in reply to round 1: docs: narrow case-exact changeset scope, one markdown file, two insertions and one deletion.
The probes now verify each path segment against its parent's real directory
-listing, so every host resolves the same names to the same real paths. The
+listing, so these component and hook lookups resolve the same names to the
+same real paths on every host. TheThe commit type is a risk claim, and it was checked. docs: asserts nothing
consumer-visible changed. The diff is one sentence of changeset prose. Honest.
The runtime is byte-identical to the head round 1 reviewed — hash-compared rather than eyeballed, across all four code files, with the whole extracted module tree diffed as a second check. So every round-1 measurement is valid at this head by construction. The decisive ones were re-run anyway.
Main had still not invalidated it. Main advanced twice during the review; the three resolvers and the swizzle module remained byte-identical from the merge-base through the newest main, and main touched nothing in the CLI package.
| round-1 ask | status at the corrected head |
|---|---|
| the changeset claims the whole CLI | satisfied — the sentence now scopes to the component and hook lookups named in the body's opening clause. Finding deleted, not softened: a finding the head has fixed does not survive into the next round |
| the swizzle owner lookup still folds | unchanged, and re-proven at this head rather than carried over on trust |
| +51% directory reads | unchanged; the runtime is byte-identical |
Deleting the changeset finding did not empty the review, so the verdict stayed a real approve rather than a thinner one.
- The eight-row case-sensitive-versus-case-insensitive probe: 8 of 8 agree.
- The repository's tests: 4 files, 65 tests, all passed, in 11.6 seconds.
-
changeset status: the CLI package is in the patch bump list, nothing at minor or major. - Both commands run again for real — the component lookup refuses
button, the swizzle command still copies it. The divergence is unchanged.
17 SUCCESS · 4 SKIPPED · 0 failures. Every check required by branch
protection — build, lint, test, docsite-test and the review gate — passed. The
a11y, visual and RTL jobs and one review-chain job skipped by path filter, which
is correct for a diff that changes no component package. Mergeability finished at
MERGEABLE / CLEAN; the earlier BLOCKED reading was two jobs still running,
not a human gate, since branch protection requires zero approving reviews.
Slot verdicts are unchanged from round 1 except Breaking, whose description is now accurate about its reach, and the Impact and Architecture notes, which now describe a gap the changeset acknowledges rather than one it papered over.
GOAL: met — and the reason is worth being precise about, because it is the one
place a re-review can fool itself. The code did not get better; the claim got
true. Round 1's partly met was measured against "every host resolves the same
names to the same real paths", which the CLI as a whole does not do. Against
"these component and hook lookups", it does — on all eight probes, on both
filesystems.
Was the round-1 hold resolved? Its substantive clause — the overclaim — yes. Its second clause was never about the PR: a finding raised on a file this PR does not touch is not tracked by this PR, so it must be filed separately or not raised at all. That obligation belongs to the loop, not to the diff, and it was never a reason to hold a clean merge.
AUTHOR CAN PROCEED: yes. Nothing was required from anyone before this landed.
JUDGEMENT NEEDED: none.
One thing checked and deliberately not asked for. The changeset's headline is still unqualified — "[fix] Component and hook names resolve case-exactly on macOS and Windows, matching Linux" — and strictly, the swizzle command takes a component name and still folds. It was not raised, for three reasons: the body names the three functions in its very first clause, so the rendered entry scopes itself one sentence later, which was confirmed by reflowing the file; a second round notes rather than re-blocks unless the PR's shape changes, and a changelog headline is not shape; and asking for a second wording pass on a sentence that was not flagged in round 1 is moving the goalposts after the author fixed exactly what was asked. Recorded so the silence is not mistaken for not having looked.
1. [not blocking] the swizzle owner lookup probes the component directory
with an unverified existence check
→ two commands, one typed name, two answers on macOS
Round 1's second finding is deleted: the head fixed it.
Pass 1 failed the draft again on length and voice — 46 words against the 30-word cap, a closing question on an approve, and mechanism restated in the summary. It also caught something sharper: the draft ended by offering to carry the follow-up over, which is a commitment the leg could not keep in the same turn. An offer that outlives the session that made it is a promise with nothing behind it, so it came out of the text and became a precondition on posting instead.
Pass 2 on the rewrite: no violations. Summary 23 words, inline 12, whole review 35 words.
Thanks — that reads right now.
swizzle buttonstill resolves on macOS; same folded probe, in a file you didn't touch. Nit, separate PR.[Reviewed by Robohands]
Inline (1):
-
packages/cli/api/swizzle/copy/copy.mjs:140— Same folded probe as the three you fixed. Might wantexistsCaseExacthere.
No Discord clause: the author is internal. No full-review link: the maintainer's own PRs are excluded from that rule.
- Unicode normalisation. The segment comparison is an exact string compare, so a decomposed-versus-composed filename would be a false negative where the plain existence check succeeds. Every name in the core source tree is ASCII, so it is unreachable today — worth knowing, not worth a finding.
- The integration component doc lookup carries the same unverified probe as swizzle. It was not driven, because reaching it needs a loaded integration manifest. It is a third member of the same class and belongs in the same follow-up.
- The changeset's line wrapping is ragged after the edit. It renders correctly — confirmed by reflowing the file — lint passes, and no CI job format-checks changeset prose. Cosmetic in the source only.
- The per-segment lookup is linear in directory entries. At 106 entries that is noise next to the directory read itself; the memoisation is the change worth measuring, not the lookup.
- Windows. Every case-insensitivity claim was measured on macOS APFS against a case-sensitive APFS control. The NTFS claim follows from the same existence semantics and was not driven — no Windows host was available.
- The integration component doc lookup — reasoned from source, not driven.
Total 39 minutes across two rounds — round 1 22m, round 2 17m.
round 1 setup 3 · reading 6 · measuring 6 · writing 7 · waste 2
round 2 setup 1 · reading 3 · measuring 4 · waiting 5 · writing 4 · waste 0
Warm main was reused as the BEFORE arm in both rounds. One re-measure, in round 1: the N=106 timing was retaken order-alternated across five runs because the first pass's clock was cache-order noise and would have reported the fix as faster than the code it replaced. Round 1's waste was extracting four files instead of the whole module tree, so the first probe died on a missing sibling import; round 2 reported none, and the byte-identity check up front is why — it converted "re-run everything" into "prove nothing changed, then re-run the decisive ones".
Round 2's five waiting minutes are reported on their own line because waiting for CI is not work.
Nothing was posted. GitHub rejects an approval from the pull request's own author, so the loop's approve verdict never reached the PR. The maintainer merged it on the strength of green required checks and branch protection that requires no approving review.
So the usual training signal this slot exists to capture — the gap between what the loop drafted and what a human actually sent — does not exist for this review. The drafted text above is the complete and final output, unedited and unsent.
The one thing still owed, and it is the loop's, not the author's. The swizzle finding lives on a file this PR did not touch, so merging #5478 does not carry it anywhere. Unless it is filed as its own issue, the finding evaporates with this page. That is the standing rule about findings raised on someone else's ground, and this record is the reminder that it was not yet discharged when the PR merged.