Repository navigation
fix(bestax-mcp): name every component that declares a CSS variable, its owner first - #975
Conversation
…ts owner first The index kept one component per CSS variable, whichever the generator read last. Several components read the same partials: DateTimeInput renders DateInput's calendar and TimeInput's wheel, and the form controls all read Bulma's input variables. So the calendar's variables came back as declared by DateTimeInput, the subtitle's by Title and the input's by TextArea, and an agent theming DateInput was pointed at another component. Each variable now lists every component that declares it, since an override reaches each. The one it is named after comes first, the longest name the variable continues after --bulma-, with or without hyphens between words, and the rest follow in code-point order. A variable named after none of them, such as the popover the three pickers share, lists them all in code-point order. search_bestax and get_css_variables show the list, and the next call goes to the first. The index's shape changed, so its schema version moves to 2 with the server's.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://e224e5a7.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | An owner-less variable still routes next to one arbitrary co-declarer (the 8 --bulma-picker-popover-* entries always point at DateInput) |
bestax-mcp/src/search.ts:173 |
Overall: Sound, and the fix is the right shape — turning the index entry into a sorted list removes the "last read wins" nondeterminism entirely rather than papering over one symptom, and the new generator test holds the rule against the real index (every variable lists exactly its declarers, owner first, rest in code-point order) instead of a fixture. The riskiest part is the owner heuristic in orderDeclarers, so that is where to look first: it scores by the length of the matched prefix inside the variable name, which is a correct specificity ordering because two matching keys are both prefixes of the same string and therefore comparable — I checked all 88 multi-declarer entries by hand and every one picks the component a human would. The schema bump is self-consistent (index and server ship in one tarball, assertSchema fails loudly on a mismatch) and cssVarIndex has no public type surface — bestax-mcp is bin-only with no exports — so fix is the right release type.
Residual risk:
- Owner-less shared variables — real and present today (the 8
picker-popoverentries); posted as finding 1. Mitigated becausedetailnames all three declarers on the row and the arbitrary first entry does declare the variable. - A shared variable spelled so neither key matches its owner (mixed hyphenation, e.g. a hypothetical
--bulma-datetime-input-*) would silently fall into the owner-less path, and the generator test asserts the rule conditionally, underif best > 0, rather than asserting that a multi-declarer variable has an owner — so nothing would flag it. Named here as open; no such variable exists in the index today, verified by enumerating all 10 owner-less entries. - A variable named after a component that does not declare it — refuted.
--bulma-loading-colorresolves toButtonand--bulma-block-spacingtoField; I read both component records, and these are declarations made by Bulma itself in those partials (Loading.jsondeclares--bulma-loading-icon-spin-color, not--bulma-loading-color), so the index points at the only place an override can go. - Ties in the owner rule — refuted. Two declarers can only tie at a nonzero score if their keys are the identical prefix of the variable; I collided every catalog component name across both key forms and found no collisions, so the first-wins reduce is never load-bearing.
- A
nextnaming a component the server cannot resolve — refuted. All 64 distinct declarers in the index are catalog components, anddeclarersis never empty or non-array (0 bad shapes), so the first element can never render as the string "undefined". - Response bloat on the two changed surfaces — refuted as material. Worst case is a
get_css_variablesquery of "input", 3,071 → 5,066 characters;search_bestaxcaps at 15 hits.catalog.json, read eagerly at startup, grows 83.1 KB → 89.6 KB.
What I ran
pnpm --filter bestax-mcp run test 250 passed, incl. the new 964 server test
pnpm --filter bestax-mcp run test:coverage 97.04 / 91.25 / 98.41 / 98.63 — above the 95/78 gate
pnpm --filter bestax-mcp run typecheck clean
pnpm --filter bestax-mcp run lint clean
pnpm run gen:mcp:check index not stale
node --test scripts/gen-mcp-index.test.mjs 18 passed
with coverage on gen-mcp-index.mjs 91.25 / 89.44 / 92.11 vs floors 86 / 85 / 88
pnpm run check:conformance only version-regression, a shallow-clone artefact of this checkout
pnpm run test:scripts also reports pre-existing bulma-ui export map failures in this environment because bulma-ui/dist is not built; unrelated to the diff.
Evidence for the bug itself: git show HEAD~1:bestax-mcp/data/catalog.json maps --bulma-dateinput-cell-color to DateTimeInput, matching the report in #964 verbatim.
🏄 Clean little set wave, bro — one gnarly "whoever I read last wins" rip current got traded for a proper sorted lineup, with tests paddling out over the real index instead of a kiddie pool. Only the shared popover still has three dudes claiming the same break, and the PR is already cool with that. Send it.
…an owner The comment on a CSS variable's next call said the first declarer is the one the variable is named after. For the picker popover's variables none is, and the first is first only in code-point order, so the clause is deleted.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 4f1d102. |
Preview DeploymentPreview URL: https://9effb519.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| # | Thread | Outcome | Location |
|---|---|---|---|
| 1 | 🔵 Advisory · Correctness — an owner-less variable still routes next to one arbitrary co-declarer |
✅ Verified fixed — the false second clause of the comment is deleted in 4f1d1020 |
bestax-mcp/src/search.ts:168 |
Overall: This pass settled threads and reviewed no commits. The one open thread — the 🔵 Advisory on the css-var hit comment claiming the next call goes to "the component the variable is named after", which is untrue for the eight owner-less --bulma-picker-popover-* variables — is fixed: the clause is gone, so the comment now claims only that the next call goes to the first declarer. The routing itself is deliberately unchanged, which the thread had already put on the record as acceptable (DateInput does declare those variables, and detail names all three pickers). pnpm --filter bestax-mcp run test is green at 250/250.
Residual risk: Not re-assessed — a verify pass settles open threads and raises no new findings. For a review of the code as it stands at 4f1d1020, ask for a fresh pass with a deep-review: fresh steer.
🏄 Mellow set, this one — paddled back out, shaved off the one clause that was overclaiming, and the thread closed out clean. Nothing left bobbing in the lineup. Good to go.
github-actions
Bot
added
the
review-converged
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
…owner # Conflicts: # bestax-mcp/data/catalog.json
…l warning #952 taught the date and time pickers to name their default left icon when an icon size or column set on them inside a Control does nothing, so the advice rebuilds what their own Control drew. DateRangeInput landed in the meantime without it, so its advice left out iconLeftName="calendar" and the shared Control-level tests failed on main. It now passes its default icon for the advice, as the other pickers do.
…list DateRangeInput now reads the calendar's variables too, so the tests that pinned `DateInput, DateTimeInput` failed while the index was right. The generator test now pins only each shared variable's owner, which is the rule, and leaves the rest of the list to the check that every declarer is listed. The server test reads the list from the index and pins DateInput leading it.
github-actions
Bot
removed
the
review-converged
Preview DeploymentPreview URL: https://ab2bf0e9.bestax.pages.dev |
|
deep-review: fresh The last run on a52b3cf was cancelled at the runner limit after it posted two threads (on |
# Conflicts: # scripts/gen-mcp-index.test.mjs
…p a picker count The declarer test computed its expected owner with a copy of the generator's own rule, and skipped the check whenever that copy found none, so a rule that stopped finding an owner left the variable in code-point order and still passed. Every variable several components declare must now have an owner, the picker popover's excepted by name, and the hand-written owners add --bulma-icon-text-spacing, the one real variable decided by a name's hyphenated form. The generator's comment no longer counts the pickers that share the popover, a number that moved when DateRangeInput arrived.
|
deep-review: fresh Main is merged in to clear a conflict, so this reviews 68f96c2. Two threads from a cancelled run on a52b3cf (on |
Preview DeploymentPreview URL: https://095710ab.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 1 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | Two orderDeclarers unit cases expect the code-point answer, so neither can fail if the mid-word guard or the exact-match branch is loosened |
scripts/gen-mcp-index.test.mjs:294-302 |
| 2 | 🔵 Advisory | Correctness | Carried over from the cancelled run — the generator comment’s picker count is fixed, but the PR description still says "the three pickers share" / "all three use" where the index lists four. Still open; the PR body is the maintainer’s to edit | scripts/gen-mcp-index.mjs:323 |
| 3 | 🔵 Advisory | Coverage | Carried over from the cancelled run — the per-variable owner assertion could not fail on a wrong owner. Verified fixed at 68f96c20 and resolved by this pass |
scripts/gen-mcp-index.test.mjs:238 |
| 4 | 🔵 Advisory | Performance | The eight-name declarer list is re-rendered on each of up to 60 rows, so a get_css_variables query for "input" costs ~700 extra tokens |
bestax-mcp/src/server.ts:685 |
Overall: The change is sound and it fixes #964 at the root: --bulma-dateinput-cell-color now lists DateInput, DateRangeInput and DateTimeInput, in that order, instead of whichever picker the generator happened to read last, and pnpm run gen:mcp:check is clean, so the committed index is the generated one. The riskiest part is still the naming heuristic in orderDeclarers, so look there first — I hand-checked all 88 multi-declarer entries and every owner is the component a human would name: Icon over IconText, Title over SubTitle, Input over the seven controls that read Bulma’s input partial. The schema bump is self-consistent: the generator writes SCHEMA_VERSION into both catalog.json and bulma-classes.json, data.ts holds both to the same constant, and bestax-mcp is bin-only with no exports, so no consumer sees cssVarIndex as a type and fix is the right release type. The one thing worth a human minute is finding 1 — two new unit cases that document the match predicate’s boundaries but pass either way.
Residual risk:
- Owner-less shared variables still route
nextto an alphabetical first — real and present: the eight--bulma-picker-popover-*entries always point atDateInput. Already on the record in a thread resolved on an accepted reason —DateInputdoes declare them and the row names all four pickers, so the follow-up call answers correctly. Not re-raised. - A shared variable spelled so it matches no declarer’s name would fall into that same path silently — refuted for the future as well as the present. The generator test now asserts a nonzero owner score unconditionally for every multi-declarer variable, with
OWNERLESSthe only escape hatch, so a new owner-less family fails the suite until someone adds it on purpose. The index has exactly ten owner-less entries today: eightpicker-popover, two single-declarer. - The match predicate’s two boundaries, mid-word rejection and whole-name equality, have no biting test — open, posted as finding 1. The index-wide test cannot cover for them: loosening either branch changes no owner in the current index, so only the unit cases could catch it, and both are satisfied by code-point order anyway.
- A
nextnaming a component the server cannot resolve — refuted. Every declarer in the index is a catalog component, and acssVarIndexentry is only ever created by pushing a name, so the first element is never undefined. - Ties in the owner rule — refuted. The
reducecompares strictly, so a tie keeps the code-point-first name; two declarers can only tie at a nonzero score if their keys are the identical prefix of the variable, and no two catalog component names collide across either key form. - A stale generated artefact — refuted empirically.
pnpm run gen:mcp:checkregeneratedbestax-mcp/dataand reported no diff,tsc --noEmitpasses for the package, andpnpm --filter bestax-mcp run testis green at 314/314. The rootscriptssuite is green on both new generator tests; its other failures are the documented "needsbulma-ui/dist" shape, anddist/is absent in this checkout.
🏄 Clean little set, this one — paddled out and turned one-name-wins into the whole lineup, owner right on the nose. Only ding is a couple of test waves that look like they are breaking but never close out, plus a PR description still counting three pickers after a fourth showed up. Patch those and it rides all the way to the beach.
…rts first Three of the cases expected an owner that code-point order would have put first anyway, so they passed whether the rule credited the owner or not, and loosening the hyphen guard or dropping the whole-name match went unnoticed. Each now pairs the owner with a declarer that sorts before it, so the two orders differ.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 9f155c5. |
Preview DeploymentPreview URL: https://b0850ae4.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 3 resolved · 0 open
| # | Thread | Outcome | Location |
|---|---|---|---|
| 1 | "the three pickers" is four as of this head commit — 🔵 Advisory · Correctness | ✅ Verified fixed | scripts/gen-mcp-index.mjs:323 |
| 2 | Two orderDeclarers cases cannot fail for the boundary they name — 🟡 Minor · Coverage |
✅ Verified fixed | scripts/gen-mcp-index.test.mjs:296 |
| 3 | The declarer list repeats per row, uncapped, in the 60-row search table — 🔵 Advisory · Performance | ✅ Accepted as a deliberate trade-off | bestax-mcp/src/server.ts:685 |
What I checked
- (1) The PR description now reads "such as the popover the pickers share" and "a variable they all use" — no count remains, in either copy. The generator JSDoc carries the matching deletion: "named after none of its declarers (
--bulma-picker-popover-*, which the pickers share)". - (2) Both unit cases now pair the candidate owner with a co-declarer that sorts before it, so owner-first and code-point order give different lists and the assertion can fail. Traced against
orderDeclarers(scripts/gen-mcp-index.mjs:331-347): loosening the hyphen guard torest.startsWith(k)makes--bulma-dateinputs-xcreditDateInputand return['DateInput', 'Control'], and dropping therest === kbranch leaves--bulma-boxowner-less and returns['Block', 'Box']— each a failure where the old pair passed either way.--bulma-icon-dimensionsgot a first-sorting co-declarer too, so the longest-match branch is pinned as well. - (3) A reasoned keep-as-is from the PR author. Listing every declarer is the change itself; collapsing the cell to the owner plus a count reintroduces #964 for the non-owner declarers. The original finding already said no change was required.
Tests run: pnpm --filter bestax-mcp run test — 314/314 green. pnpm run test:scripts — the two generator assertions this PR owns both pass (every CSS variable lists every declarer, the one it is named after first (#964), a variable goes to the declarer whose name accounts for most of it). The remaining failures in that suite are the known "build bulma-ui/dist first" shape (export-map, tree-shaking, scaffold-compile tests), unrelated to this diff.
Overall: This pass settled threads and reviewed no commits. All three of my open threads are closed — two verified fixed in 68f96c20/9f155c58, one accepted as a deliberate trade-off — leaving nothing of mine open on 9f155c58.
Residual risk: Not re-assessed on this pass; a verify pass reviews no commits. The residual risk recorded by the prior review stands as written, less the coverage gap thread 2 closed: the match predicate's mid-word and whole-name branches now have discriminating unit cases, so an edit to the heart of this change no longer lands silently.
🏄 Three threads paddled in and all three caught the beach clean — two real fixes and one honest "nah, that's the point, brah." Nothing left bobbing out there; this one's good to go.
|
deep-review: fresh Every earlier thread is settled, and this asks for a fresh review of 9f155c5 as it stands, so a finding gets its own thread on this commit. Start with the declarer ordering in |
There was a problem hiding this comment.
Deep review — 0 blocking · 0 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| — | — | — | No blocking defects found. | — |
Overall: Sound, and verified end to end rather than read. I reproduced #964's exact shape at the base commit (--bulma-dateinput-cell-color → "DateTimeInput", --bulma-subtitle-color → "Title", --bulma-input-border-color → "TextArea"), confirmed each is now a list led by the right owner, and re-ran the generator over the working tree — git status came back clean, so the committed data/ is byte-identical to what pnpm gen:mcp produces and gen:mcp:check will pass. Turning the entry into a sorted list also removes the read-order dependence that caused the bug outright: orderDeclarers dedupes and sorts before it picks, so the output no longer depends on which page the generator reached last. The riskiest part is the longest-hyphen-prefix ownership heuristic, and it is the part now best covered — 9f155c58's point was to make the five orderDeclarers cases discriminate, and they do.
What I ran
| Check | Result |
|---|---|
node --test scripts/gen-mcp-index.test.mjs |
21/21 pass, including both new tests |
pnpm --filter bestax-mcp test |
314/314 pass |
pnpm --filter bestax-mcp test:coverage |
97.54 stmts / 91.71 branches — above the 95/78 floor |
pnpm --filter bestax-mcp typecheck |
clean (string → string[] reaches both call sites) |
node scripts/gen-mcp-index.mjs then git status |
clean — committed index matches the generator |
pnpm check:conformance |
all checks pass |
prettier --check on the six changed files |
clean |
Mutation check on the ownership rule, against the five orderDeclarers cases as written at 9f155c58 — every weakening is caught by at least one case, which is what the last commit claimed:
| Mutation | Caught by |
|---|---|
drop the whole-name match (rest === k) |
--bulma-box |
loosen the hyphen guard (startsWith(k)) |
--bulma-dateinputs-x |
| drop the hyphenated key | --bulma-icon-text-spacing |
> → >= in the owner reduce |
--bulma-picker-popover-shadow, --bulma-dateinputs-x |
The root scripts/ coverage-floor run could not be judged here: bulma-ui/dist is absent in this checkout, so the export-map and colour-tuple suites fail on their own "build first" messages and coverage-floors.mjs refuses to score a red run, exactly as documented. scripts/gen-mcp-index.test.mjs passes on its own and the PR adds no uncovered code to that file.
Residual risk:
- A wrong owner, rather than a missing one, is caught only by the six hand-written pins. The catalog-level loop's
namedLengthoracle (gen-mcp-index.test.mjs:209-216) is still a copy of the generator's ownnamed, so it cannot disagree with it. Refuted as a practical gap: I mutated the rule to pick the shortest match, and to pick the first match in code-point order, and--bulma-icon-text-spacing → IconTextfails under both. Thebest > 0assertion at238-240covers the complementary case — a rule that stops finding owners. - The heuristic mis-ranking a future shared family. Refuted for the current library: only two component names are hyphen-prefixes of another (
Icon < IconText,Link < LinkButton), both resolve correctly in the generated index (--bulma-icon-dimensions*→Icon, IconText;--bulma-icon-text-spacing→IconText, Icon;--bulma-link-button-*→LinkButton), and no variable has two declarers tied at the maximum matched length, so the strict>tie-break is never load-bearing. A new family whose variable prefix matches none of its declarers fails${css} has no owner among ${listed}at generation time rather than silently falling back to code-point order. - Read-order nondeterminism, the mechanism behind #964. Refuted structurally, not only by test:
orderDeclarersappliesnew Set(...).sort(byCodePoint)before selecting, so the result is independent of the order pages were read, andoutput is deterministic and code-point sortedholds the key order. - A server left reading a v1 index. Refuted:
SCHEMA_VERSIONandSUPPORTED_SCHEMA_VERSIONboth move to 2,assertSchemathrows on a mismatch, anddata/ships in the same tarball asdist/. Both committed files that carry the field (catalog.json,bulma-classes.json) read 2;skills.jsonandcomponents/*.jsoncarry none and no loader checks them. - Open, not a defect in this diff:
search_bestax'snextstill routes todeclarers[0], which for the eight owner-less--bulma-picker-popover-*variables is whichever picker sorts first. The author kept this deliberately — the row names all four, and truncating or re-pointing it is the #964 shape for the other three — and that thread is settled; recording it here rather than re-raising it. - Noted, outside the diff:
data.ts:5's "the catalog (~76 KB)" is now 92 KB. It was already 84 KB before this PR, so the drift is pre-existing and nothing a reader acts on.
🏄 Clean little set, dude — the index stopped crediting the wrong component, the generator sorts before it picks so the whole "last one read wins" wipeout is gone, and that last commit actually taught the tests to fail. Paddle it out.
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
The MCP index kept one component per CSS variable, whichever the generator read last. Several components read the same stylesheets: DateTimeInput renders DateInput's calendar and TimeInput's wheel, Title and SubTitle read the same Bulma partial, and the form controls all read Bulma's input variables. So
search_bestaxandget_css_variablescalled the calendar's variables "declared by DateTimeInput", the subtitle's "declared by Title" and the input's "declared by TextArea", and pointed an agent theming DateInput at another component.Each variable now lists every component that declares it, since an override reaches each of them. The one it is named after comes first: the longest component name the variable continues after
--bulma-, matched with or without hyphens between words, so--bulma-subtitle-*is SubTitle's and--bulma-input-*is Input's. The rest follow in code-point order. A variable named after none of its declarers, such as the popover the pickers share, lists them all in code-point order rather than crediting one picker with a variable they all use. Both tools show the list, and the next call goes to the first entry.Naming only an owner was the other option. It reads shorter, but it has no honest answer for the shared popover, where any single name repeats the original bug for the other pickers.
The index's CSS-variable map changes from one name to a list, so its schema version moves to 2 along with the server's. The index and the server ship in the same package, so no installed server reads an index of the other shape.
The generator's tests hold the rule for every variable in the index: each lists exactly its declarers, the one it is named after first when there is one, and the rest in code-point order.
pnpm allpasses locally.Fixes #964