-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5525
bhamodi · open, comment drafted · reviewed at 68f02280f97 · view on GitHub
Verdict: comment — the mechanism question is Cindy's call, not the author's
The review is a comment rather than an approve or a request for changes: there is no defect in the diff and nothing for the author to change, but the same author has two mechanisms in flight for one problem and only a maintainer can pick. It did not merge because that choice — does the CLI get one execute-project-code boundary, or a grammar per field — binds the next PR immediately, and neither PR can see the other.
This and #5531 are a set on the same ground, and the decision is one decision, not two. #5531 puts a gate at the CLI's shared module loader; this narrows one field's grammar. They do not meet: with the gate's environment variable set, resolveTheme still loads and executes a checkout-named package, because this resolver has its own createRequire and never touches the loader. Deciding one settles the other.
Someone inspecting a checkout they have not vetted — a contributor's PR, a triage box, an agent run — types astryx component Button to read what the library exposes, and a file that checkout chose executes on their machine. resolveTheme reads astryx.theme out of the cwd's package.json and, if the value starts with . or /, require()s it; astryx component's text path calls it on every invocation (clients/cli/commands/component/index.mjs:111). The person wanted to read a component list and instead ran the repo's code.
Demonstrated rather than asserted: measured on origin/main, astryx.theme: "./theme.cjs" executes the file in both install layouts.
(1 runtime decision · ~8 runtime statements of 41 added lines in the resolver, of 120 total)
-
astryx.themefrompackage.jsonmust match an npm package-name grammar; anything else warns and degrades tonull. The environment variable keeps path support and ships with it, not separably. - A docblock fix — the stale field name, plus the rationale.
- A prettier reflow of four pre-existing warn calls and one
if.
Two things can name a theme, and they are trusted differently. A variable the operator types may point anywhere, files included, because the operator meant it. A field sitting in a file inside the checkout is data the checkout wrote about itself, so it is only allowed to name a published package — never a location on disk. A value shaped like a path is refused with a notice, and the CLI carries on with no theme, exactly as it already does for a theme name it cannot find. Only decision 1 is a judgement about runtime behaviour, and it traces to the stated problem.
The implementation lives with its owner — resolve-theme.mjs is the only reader of the field — and it does not survive the seam question, which is the review. The CLI has one other place that turns checkout-controlled data into executed code, and it is the general one: foundation/fs/module-loader.mjs, through which config, integrations, doc modules and codemods all flow. This resolver has its own createRequire(import.meta.url) at :30 and does not go through it, so astryx.theme is a fifth instance of that class with its own loader. The same failure has a next home, and it is not hypothetical: the same author, the same night, filed #5531, which puts a gate at exactly that seam and documents it in the CLI README as "the CLI runs on built-in data only." Two mechanisms for one problem, from one author, in flight at once, that do not meet — one unmade decision rather than two PRs.
Builders running the CLI over a checkout they do not control — the reason the PR exists. Before, a file the checkout ships beside its package.json runs when you list components; after, it does not. That is a real improvement and it is the whole win.
Everyone else: nothing. No in-repo package.json sets astryx.theme at all — the only "theme" hits are keyword-array entries in the seven theme packages and the CLI. Nobody's build output changes; resolveTheme feeds one thing, the variant list printed by astryx component's text view.
Anyone with a file-path theme in package.json loses their theme's variant list in astryx component, and sees a notice naming the environment variable as the way back. Loud, one command, recoverable in one line.
What landing it newly exposes: nothing becomes visibly wrong, but it makes the CLI's answer to "may a checkout name code we execute" inconsistent rather than merely absent. After this, one field says no and four other paths still say yes — and a builder who reads the new docblock, "a JSON field should never be able to name an arbitrary file that then executes", will reasonably read that as a property of the CLI when it is a property of this one field.
No API change. resolveTheme(cwd) keeps its signature and its return shape; nothing is added to or removed from any barrel; no new export, type or default.
What does change is the accepted value space of an existing published config field, which is surface enough to enumerate: pkg.astryx.theme: string narrows from any specifier to /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/. It is public — read from a consumer's package.json by a CLI published at a stable 0.5.0 — and it is the only one of the five pkg.astryx.* fields to narrow. The one README line documenting it already shows a package name.
One drift note rather than a finding: the hand-written mirror of npm's name grammar at :37 says in its own docblock that it matches "closely enough", and it rejects legacy uppercase names, which are still installable. No theme package in the ecosystem has one, and the canonical validator is not already a dependency, so adding one to fix a case nobody has is the worse trade.
n/a — no theme targets, no tokens, no styling. This is a Node resolver, and the style grep returns 0 across all three files. The word "theme" here means an npm package the CLI requires, not a theme target; no astryx-* class is rendered, added or removed.
Nothing new is frozen — the change un-freezes, by removing a capability. The permanence question runs the other way: re-adding path support later is easy and harmless, so being wrong here is cheap. That is a genuine point in the PR's favour and it is why nothing blocks. The call site is a JSON field, and the only thing a builder can now get wrong is writing a path and getting a warning that tells them exactly what to do instead — the good version of a narrowing.
There is a consumer who can be broken: packages/cli has no private field and the package is published at a stable 0.5.0, so this is released surface.
- API — no. No signature, export or type changes.
- Visual — no. Nothing renders; the only perceptible difference is one extra warning line in a terminal.
- Theme — no. Existing themes resolve by package name exactly as before.
-
Behaviour — yes, one row. A path-shaped value from
package.jsonwas loaded and applied, and now warns and returnsnull.
The rest of the walk is unchanged: no field and no environment variable returns null before the new guard; a non-string or empty value hits the pre-existing guard at :130 first; a well-formed package name reaches resolution, and the PR's own test asserts it fails on existence rather than on shape; an unresolvable name gets the same "could not resolve" warning; a path-shaped value from the environment variable is exempt at :111, with a test asserting the identical file still loads; and the boundary cases — ., .., some/../path.cjs, a scoped escape, a Windows path — are all rejected by shape, the last three covered by the PR's own test.
Is patch the right tier? Defensible. The removed capability is undocumented, reaches one command's text output, fails loudly and names its own workaround. A minor would be more honest about the removal, but arguing tier on an unadvertised path is churn.
Zero effects — not React. No component, hook, render pass, listener, observer or layout read, and the grep for effects and observers returns 0 across all three files. No bundle change and no dependency added: the regex is a literal, deliberately not the npm validator package. The work added is one regex test against a short string, once per astryx component invocation, on a path that has already read and parsed a file from disk and is about to require() a package. Not measured, and deliberately so — a regex test next to a synchronous require() is below the resolution of any harness worth trusting, and nothing grows with N because there is no N.
No frames, and the reason is the acceptable one: not renderable. This is a Node module in a CLI — no geometry, no colour, no DOM, no component. What was checked rather than assumed: 0 style-family matches across all three files, 0 JSX, no packages/core file touched, and CI's visual, a11y and RTL jobs all reporting skipped on this head, which is the pipeline agreeing that nothing renders.
The evidence that matters here is executable, so it is a table. Each cell is "did the theme module actually run", detected by the module writing a marker file, with each case in its own process so a cached require cannot leak a hit into the next row. CLI outside means the CLI is global or npx-cached, so the checkout's node_modules is not on its resolution chain; CLI inside means the CLI is a dependency of the checkout, the normal local install, where createRequire(import.meta.url) at :30 walks up into the checkout's own node_modules.
| case | before — executed | after — executed | after, with the #5531 gate set |
|---|---|---|---|
"./theme.cjs", CLI outside |
yes | no (warns) | no (warns) |
"./theme.cjs", CLI inside |
yes | no (warns) | no (warns) |
"probe-theme-pkg", CLI outside |
no | no | no |
"probe-theme-pkg", CLI inside |
yes | yes | yes |
Rows 1–2 going from yes to no is the change working, in both layouts, and the PR body says so: "a relative or absolute value meant require()ing a file out of whatever checkout the CLI was run in." Row 4 staying at yes is not a difference — it is the finding: a package name still reaches code the checkout controls whenever the CLI is a local dependency, and the docblock's sentence reads as though it does not. The third column being identical to the second is unintentional in the system rather than in this diff: [#5531]'s gate is documented to keep the CLI on built-in data and does not reach this resolver.
The harness was checked before the numbers were trusted, twice. The first run reported no warning on every row, because the warning goes to the child's stderr and the runner only surfaces stderr when the child throws; stderr was redirected to a file and every case re-run. And the checkout cannot set the environment variable itself and route around the whole thing: the CLI loads no dotenv, so that source really is operator-only.
Nothing here is touched, and this is what was checked. aria-|role=|useTranslator|t('@astryx → 0 across all three files.
- Rendered surface — not applicable rather than not looked at. No element is rendered, so there is no accessible name, no focus, no keyboard path, no live region, no contrast, no target size. CI's a11y job reports skipped on this head and adds no baseline entries; the baseline file is untouched by the diff.
-
Strings — the one row that could have fired, and it does not. The diff adds one user-visible English string, the warning at
:141. Checked against its siblings before anything was written: the CLI runtime has 20 raw Englishconsole.warn/console.errorstrings and zero uses of the translator hook, whose only appearance underpackages/cliis inside a doc asset describing the feature. The CLI has no string catalog by construction, and the new string matches all 20 siblings. The siblings agree with the author; the finding is dead. - Direction — no CSS, no glyph, no logical or physical property.
One thing the new string does well and is worth not flagging: it prints the offending value and names the way back, so the person who hits it can fix it without reading source.
comment. Nothing blocks, and the review looked for a reason to. Nobody is stuck, nothing is unreachable, no state is stranded, no data is lost; the worst outcome the diff makes possible is a theme not being applied to one command's text output, with a warning saying why.
Three of the four notes are the same note. The docblock stating a CLI-wide property that only this field has, row 4 of the probe table, and the open mechanism question are one observation seen from three angles: the boundary is being drawn at a field, and the claim being written down is about the CLI. They compound into a shape — a codebase that says, in a comment a future contributor will cite, that JSON cannot name executable code, when four other paths still let it.
1. A package name still reaches code the checkout controls
→ someone reading an unvetted checkout with `astryx component` still
executes a module that checkout chose, whenever the CLI is a local
dependency — which is the normal install
· resolve-theme.mjs:30 createRequire(import.meta.url)
confirmed twice: by reading :30 and :46-62 (the package branch
ignores cwd), and by the probe's row 4, yes before and after
2. Path-shaped astryx.theme stops resolving, on a CLI published at 0.5.0
→ a consumer with "theme": "./theme.cjs" loses the variant list in
`astryx component` and gets a notice naming the env var
· resolve-theme.mjs:139
Not raised on the PR: zero in-repo instances, undocumented, one
command, loud, one-line recovery. Recorded so approving is a choice.
The judgement that is Cindy's, and it is the whole reason this is a comment: the CLI's trust boundary is a new concept and the system has no position on it. Not "which regex", not "should paths be allowed" — does the CLI get one execute-project-code boundary, or a grammar per field? It binds the next PR immediately, since the same author has #5531 open answering it the other way, and there are four more pkg.astryx.* fields behind it.
What the review would decide, so the hold costs one word: take it. It is a strict improvement, it ships no new surface, it is cheap to reverse, and it does not foreclose the gate — the gate can absorb it later. The question put back to the author is only which of his two mechanisms the CLI is adopting, because right now he has built both.
The author has 17 PRs open from one night and this one carries the repo's code-review label, so silence would cost him more than the comment does. Two things found and not spent: Vercel is red on this head and on four of his other PRs while green on origin/main, so it is fork-wide rather than this PR — the other 19 checks are green — and the resolver's own tests pass 11 of 11 locally, matching the PR's test plan.
Not verified: whether any published consumer actually sets a path-shaped astryx.theme — the repo has none, the field is near-undocumented, and downstream package.jsons cannot be seen — and whether #5531 will land at all. Its gate's reach was measured against this head by setting the variable; the two branches were not merged together.
Thanks — the diagnosis is right, and the field really is read on every
astryx component.One gap, and it isn't yours to close: a package name still reaches code the checkout controls — an unvetted checkout can still get a module executed by
astryx component, it just has to sit innode_modulesnow. Repro: plant a package in the checkout, name it inastryx.theme, with the CLI installed locally.That plus #5531, which gates the same thing at the module loader this field never goes through — one decision rather than two, and a maintainer's to make. Which of them do you see as the one to carry?
If you'd rather talk it through with someone, we're in Discord.
Inline: packages/cli/clients/cli/lib/resolve-theme.mjs:30 — createRequire is bound here, not to cwd. Might be worth routing through module-loader.mjs instead.
One review, two gate passes.
- Gate 1 — failed on three counts, all wording. The middle paragraph explained the author's own mechanism back to him before reaching the finding; the finding is only the second clause. The inline was the summary's sentence with a line number on it, which is the duplication that makes reviews feel machine-made — an inline carries location and the pattern, the summary carries consequence. And the measured claim reached the author with no way to re-run it: the probe was banked, but on a security finding the author's first move is to see it himself, and one clause of reproduction is the difference between believable and checkable. The verdict and the finding were graded right; the wording cost a rewrite.
- Gate 2 — clean. The mechanism sentence is gone from the summary and lives only in the inline, where the author is looking at the line that causes it; the inline now names the location and points at the shared loader; and the comment carries a one-line reproduction. One further thing the rewrite fixed: round 1 said "one thing before it lands" and "nothing to change on your side" in the same comment, which is one clause implying he is blocked and another saying he is free. Round 2 holds one status honestly and never asks for a change.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inline. The presentation's merge trailer is a hold, and its content is the pairing — #5525 and #5531 are one decision, with the recommendation to take this one. #5531's own review is being written in another batch; whoever settles the mechanism settles both pages. If this is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5525) line goes in before the attribution.