Skip to content

Review 5531

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5531 — fix(cli): ASTRYX_NO_PROJECT_CODE=1 keeps the CLI on built-in data

bhamodi · open, request changes drafted · reviewed at bb37fd62ce0 · view on GitHub

Verdict: request changes — the gate sits at one of about nine loaders, so the capability the README documents does not exist

The review asks for the remaining loaders to route through importUserModule too, because astryx doctor re-imports the config directly and astryx component runs a checkout doc module through loadDocs, so under the gate both execute code the CLI has just announced it is ignoring. It did not merge because the README now teaches a trust boundary that is not there, and because the build is red on a missing @param on the PR's own new function.

This and #5525 are a set on the same ground, and the choice between the two mechanisms is one decision, not two — and it is Cindy's. This PR puts a gate at the CLI's shared module loader; #5525 narrows one field's grammar instead. They do not meet: with this gate's environment variable set, resolveTheme still loads and executes a checkout-named package, because that resolver has its own createRequire and never touches the loader. Deciding one settles the other — see Review-5525.

Problem

Someone running astryx component Button in CI over a fork's branch, or an agent triaging an arbitrary repo, executes that repo's astryx.config.mjs — and the integration manifests and doc modules it leads to — as themselves, with their credentials in the environment. Loading a config is running it, and every discovery-backed command loads one. There is no way to ask for the CLI's built-in data instead.

That is a real problem, correctly stated in the body, and it is the problem this PR sets out to close.

Solution

(2 decisions · ~30 runtime lines of 130)

  1. ASTRYX_NO_PROJECT_CODE=1 makes importUserModule refuse — the gate.
  2. Project.load acknowledges a present config on stderr and skips it, so read commands degrade to built-ins instead of erroring.

Reading a project file means running it, because a config is a program. The change adds an off switch you set in the environment before you start. When the switch is on, the CLI is supposed to answer out of the data it shipped with rather than out of the checkout in front of it — and a config that is sitting there gets announced and stepped over instead of run, so the commands keep working with less information rather than failing.

Both decisions trace to the stated problem and nothing is piggybacked; the README, the changeset and the two probe-based tests are evidence, not further decisions. The enumeration is clean and this is a well-shaped PR on that axis. What decision 1 rests on is false: the CLI reads project files in about nine places, and the switch is checked in one of them.

The unit is right and the placement is wrong. projectCodeAllowed() is a pure environment read and a correct, testable boundary; importUserModule is one of several loaders, and the others each open the checkout's files directly and never consult it. findCoreDir() (foundation/fs/paths.mjs:22) walks up from the invocation cwd to packages/core or node_modules/@astryxdesign/core, so "core" is inside the untrusted checkout, and its doc modules are loaded with a bare await import().

Each seam was driven in a scratch checkout whose astryx.config.mjs and packages/core/src/Probe/Probe.doc.mjs write a marker file when they execute, with the gate on:

seam driven result
Project.load reading astryx.config.mjs holds — marker absent, stderr notice fires
astryx doctor config check (api/doctor/doctor.mjs:328) fails — prints "ignoring …; running on built-in data only", then executes that same config
astryx component Probe doc load (foundation/discovery/component-loader.mjs:141) fails — the doc module executes and its content is printed
astryx search / astryx discover doc catalog (via importUserModule) holds

The finding is the class, not the two instances — bare executors of paths inside the invoked checkout, none of which consult the gate:

api/doctor/doctor.mjs:328                          await import(pathToFileURL(ctx.configPath).href)
foundation/discovery/component-loader.mjs:141      await import(pathToFileURL(readmePath).href)   loadDocs
foundation/text/string-utils.mjs:134               await import(pathToFileURL(docPath).href)      searchComponents
api/docs/_adapter.mjs:58, :68                      await import(pathToFileURL(docPath).href)
api/docs/list/list.mjs:33                          await import(pathToFileURL(entry.path).href)
api/search/search.mjs:347                          await import(pathToFileURL(docPath).href)
api/theme/build/build.mjs:233                      await import(pathToFileURL(docPath).href)
foundation/discovery/template-adapter.mjs:227-228  getJiti().import() / await import(`file://…`)
clients/cli/lib/resolve-theme.mjs:37, :45          _require()

component-loader.mjs carries both twins: loadComponentDoc at :37 goes through importUserModule and is gated; loadDocs at :141 does not, and it is the one astryx component actually calls. The last line of the list, resolve-theme.mjs, is the arm #5525 is already closing, so it is not this PR's to carry.

Impact

The person this reaches is the operator who reads the new README section and puts ASTRYX_NO_PROJECT_CODE=1 in front of a CLI call in CI. Today they know the CLI runs the checkout's code, so they sandbox it or they do not run it. After this lands they have a documented reason to believe they do not have to — and astryx doctor will tell them, on stderr, that it is ignoring the config on the line above the one where it runs it.

Nobody's behaviour changes with the variable unset, so the reach is exactly the people who opt in, and what they get is a belief rather than a boundary. Landing this newly exposes nothing that was hidden; it newly claims something that is not true, and that claim is the harm.

API

No new props or exports on a public barrel. Two additions, one of them public in the way that matters for a CLI — the call site, from the new README section:

ASTRYX_NO_PROJECT_CODE=1 astryx component Button
change public? doc'd?
+ ASTRYX_NO_PROJECT_CODE environment variable, '1' is the only truthy spelling yes — documented in the shipped packages/cli/README.md README, English only; the CLI has no zh docs
+ projectCodeAllowed(): boolean from foundation/fs/module-loader.mjs internal — foundation, not the package barrel code comment

The variable's shape is fine and matches an established family: ASTRYX_THEME, ASTRYX_LATEST_VERSION and ASTRYX_VERSION__ are all environment-variable-as-CLI-switch. Only '1' is accepted — true, yes, or an empty-but-set value all silently mean "off". That is consistent with nothing in particular, since the other three carry values rather than act as flags, but it costs a reader one README word and no consumer can be wrong about it today, so it is left to the author.

Theme targets

n/a — CLI foundation, structural only. No new theme targets, no tokens, no styling: the style grep returns 4 hits in project.mjs and all four are issue-number #NNNN references in comments, 0 real.

Ossification

The environment variable's name is frozen the day it is documented, and that is the cheap half. The expensive permanence is not the variable, it is the sentence next to it: a documented trust boundary is very hard to narrow later. The README currently states one the code does not implement, which is why the ask is "widen the gate or narrow the sentence" rather than a rename.

Breaking

@astryxdesign/cli is published and not private, so there is a consumer who can be broken.

  • API — no. With the variable unset, projectCodeAllowed() returns true, importUserModule is byte-identical, and Project.load takes its original path. Existing call sites compile and behave the same.
  • Visual — no. CLI text only, and the one new line is stderr and only under the gate.
  • Theme — no.
  • Behaviour — the states this diff moves through: no config present is unchanged, because findConfigPath returns null either way; config present with the gate off is unchanged; config present with the gate on is new — the built-in shape is returned, one stderr line is printed and the config is never imported, verified both by the PR's own test and by driving it. A command that must import a workspace module now throws with the variable named. Empty, loading, error, disabled and boundary states are not reachable, since the diff adds no state, no default and no prop.

One behaviour note that is not a break: under the gate, a command whose whole job is a workspace module — astryx validate-integration, a codemod — throws rather than degrading, while read commands degrade. Both are defensible and the PR chose them deliberately. Worth a sentence in the README; not a finding.

Performance & resources

Zero effects — no React in this diff, so no hook, no render pass, no listener, no observer and no layout read. The added work is one process.env.ASTRYX_NO_PROJECT_CODE !== '1' read per importUserModule call, plus one module-scope boolean (configSkipNoted) per process. No new dependency and no bundle delta: @astryxdesign/cli ships no browser bundle. Nothing here grows with N.

Not measured, and it does not need to be: an environment read is not a cost you can measure against process startup, and there is no N.

Visual evidence

Not renderable, for the acceptable reason — a CLI change with no geometry, colour or DOM. The equivalent frame is the terminal, so it was captured live at this head rather than described. The scratch checkout's astryx.config.mjs and packages/core/src/Probe/Probe.doc.mjs each write a marker file when executed:

$ ASTRYX_NO_PROJECT_CODE=1 astryx doctor
astryx: ASTRYX_NO_PROJECT_CODE=1 — ignoring <checkout>/astryx.config.mjs; running on built-in data only
astryx doctor - diagnosing your setup
status:  [ok]
check:   Node.js version
...
$ cat PWNED-config.txt
config executed                      ← the file it just said it was ignoring

$ ASTRYX_NO_PROJECT_CODE=1 astryx component Probe
astryx: ASTRYX_NO_PROJECT_CODE=1 — ignoring <checkout>/astryx.config.mjs; running on built-in data only
# Probe
probe
$ cat PWNED-doc.txt
doc module executed

The reproduction is banked in the review kit as a script that builds the scratch checkout, runs component, doctor, search and discover under the gate, and prints which markers appeared. Its current output is two lines: astryx component Probe -> doc module EXECUTED and astryx doctor -> config module EXECUTED.

Every difference here is unintentional. The PR's own summary says the gate keeps the whole CLI on built-in data, and no sentence in the body scopes the gate to the config, so these two are the PR failing its stated goal rather than a deliberate scope line.

A11y & i18n

Nothing is rendered — no ARIA, no roles, no focus, no keyboard — and the grep for aria-|role=|useTranslator returns 0, so the rendered-surface rules are not reachable by this diff.

One line is reachable and was checked: the new stderr notice at project.mjs:47 is hardcoded English. The CLI has no message catalog — 19 bare console.error/console.warn calls already ship on main across foundation, api and clients — so the string rules do not bind here and the new line matches the house pattern. No direction, no glyphs, no logical properties.

Judgement

request changes. The goal is not met against the standard the PR sets for itself: the body's claim is that the gate "keeps the whole CLI on built-in data", and driven at this head, ASTRYX_NO_PROJECT_CODE=1 astryx doctor executes the checkout's astryx.config.mjs and ASTRYX_NO_PROJECT_CODE=1 astryx component Probe executes Probe.doc.mjs. Both are commands the body names as covered. It is repairable — the mechanism is right and only its placement is short — so this is request changes rather than a redirect or a close.

1. astryx doctor re-imports the config the gate just declined
   → the operator who set the variable in CI runs the untrusted repo's
     astryx.config.mjs, one line under a message saying it was ignored
     `const mod = await import(pathToFileURL(ctx.configPath).href);`
                    · api/doctor/doctor.mjs:328

2. loadDocs opens a checkout doc module with a bare await import()
   → ASTRYX_NO_PROJECT_CODE=1 astryx component <Name> executes
     <checkout>/packages/core/src/<Name>/<Name>.doc.mjs; its gated twin
     loadComponentDoc sits 100 lines above it
     `const mod = await import(pathToFileURL(readmePath).href);`
                    · foundation/discovery/component-loader.mjs:141

3. [not blocking] noteConfigSkipped has no @param, so tsc --project
   tsconfig.api-dts.json fails TS7006 and takes build, test,
   build-storybook and build-sandbox with it
   → they cannot merge it as it stands, but it is one line and it is not
     why this review is request changes
     `function noteConfigSkipped(configPath) {`
                    · packages/cli/foundation/config/project.mjs:43

The two blocks and the README's sentence are the same defect from three angles — the gate misses doctor, the gate misses loadDocs, and the README states a boundary that is not one — so they carry one ask: widen the gate or narrow the sentence. Both blocks were confirmed two ways: by reading, grepping for bare await import(pathToFileURL and finding 9 sites of which none consult projectCodeAllowed, and by driving, with the transcript above and reruns from the banked probe. A grep of the diff for the fix returns nothing, so the author has not already done it. The third finding was confirmed from CI's own log on this head.

The worst outcome the diff makes possible is the one sentence that cannot coexist with an approve: under the gate it prints "ignoring …; running on built-in data only", and then executes that same config. An operator who trusts that sentence runs untrusted code believing they did not.

No design call is inside this PR — the standard it fails is its own stated goal and the fix is inside its own design. The judgement that is a maintainer's sits one level up and is shared with #5525: does the CLI get one execute-project-code boundary, or a grammar per field? Two mechanisms from one author are in flight for one problem, and only Cindy can pick.

Three things found and not spent. resolve-theme.mjs _require()s a cwd-relative astryx.theme value — the same class, already closed by #5525. template-adapter.mjs:228 builds file://${docPath} by hand and vite warns that the URL must not contain a hostname — pre-existing, not this author's. And the gate throwing for codemods and validate-integration rather than degrading is deliberate and defensible, not worth a round trip. Only '1' being truthy is accepted rather than raised, for the same reason.

Not verified: whether astryx template and astryx theme build execute checkout files under the gate. Both code paths are in the class list above, but only component, doctor, search and discover were driven.

The review, as drafted

Thanks for this — the trade you're naming is real and importUserModule is the right place for the switch.

It isn't holding yet. astryx doctor re-imports the config directly at doctor.mjs:328, so under the gate it prints "ignoring …; running on built-in data only" and then executes that same file. astryx component <Name> runs a doc module from the checkout through loadDocs at component-loader.mjs:141 — its neighbour loadComponentDoc goes through the gate, loadDocs doesn't. About nine bare await import()s of checkout paths remain.

ASTRYX_NO_PROJECT_CODE=1 astryx doctor   # config runs anyway

Ideally I'd like those loaders routed through importUserModule too, so the README sentence is true when it ships. Also CI is red on a missing @param for noteConfigSkipped (project.mjs:43).

Is routing them all through the one loader workable, or is something in loadDocs making that hard?

If you'd rather talk it through with someone, we're in Discord.

Inline: packages/cli/foundation/config/project.mjs:43 — Needs a @param {string} configPath.

Rounds

One review, two gate passes. There was no prior review on the PR to reconcile with — 0 reviews and 0 review comments — and main did not move under the review: the head was re-fetched in the final round and was unmoved at bb37fd62ce0, with every anchor re-opened at it and its text pasted into the findings.

  • Gate 1 — failed on five, two of them substantive. The mechanical @param finding was marked as a block alongside the two about the gate's correctness, which inflates the verdict and hides which finding is actually carrying it — this went to request changes because of the gate, not because of a missing @param. And the closing question offered "scope the README and take the loaders as a follow-up" while the paragraph above it stated the opposite preference, so the author would have answered the smaller of two asks. The other three were wording: an aphorism in the third paragraph that supplied urgency rather than information, an inline that repeated the summary's consequence on top of its location, and an opening clause that explained the author's own design decision back to him.
  • Gate 2 — clean, no further round required. The mechanical item now reads as not blocking and says so in its own words; the closing question and the stated preference name the same outcome and it is a real question the author may answer "no" to; every noun in the comment is named, so someone who has not read the diff can resolve all of them. The checks that passed in round 1 were re-run rather than assumed — the anchors, the second confirmation method for both blocks, the class enumeration, and that main had not moved.

Status

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 whose content is the first finding — the gate does not hold, because doctor and component still execute checkout code — with the red build beside it.

The decision that unblocks this page is not a wording call: #5531 and #5525 are one decision about the CLI's trust boundary, and whoever settles the mechanism settles both pages. Review-5525 carries the other half. If this is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5531) line goes in before the attribution.

Clone this wiki locally