Review 5525

Review #5525 — CLI theme config resolution

Versions: Review Loop 1.0.0 · Component Audit Rubric 1.10
Verdict: request changes

PR

#5525 fix(cli): astryx.theme takes npm package names only by bhamodi (bucket: contributor)

HEAD REVIEWED

68f02280f973a1a8f65cb41dded728cef22fcb26 <- every claim below was verified at this commit

VERSIONS

LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10

PROBLEM

A builder following the official Theming Infrastructure guide can set "astryx": {"theme": "./src/theme.ts"} so astryx component shows that local theme's custom variants. Published @astryxdesign/cli@0.5.0 loads that file. This head rejects the documented value, silently drops the custom variants from stdout, and exits 0 after a warning; astryx doctor can simultaneously report the same field as correctly wired.

The security problem motivating the PR is real but broader: a checkout-controlled package name still executes when the CLI is installed locally. The package-name-only rule removes one route while leaving the same execution class live.

VERDICT: BLOCKS — a documented, released configuration stops working and the stated trust boundary remains open through package resolution

SOLUTION

The resolver labels its input as either operator-provided (ASTRYX_THEME) or checkout-provided (package.json). Operator input keeps every existing path and package form. Checkout input must match a local npm-name regex; rejected values warn and behave as no theme, while accepted names follow the existing package-resolution branches.

SOLUTION (1 runtime decision · ~8 runtime statements of 43 added lines in the resolver; 120 additions across the PR)

Narrow package.json#astryx.theme from file paths/package specifiers to bare or scoped package names. Tests and the changeset evidence that decision; docblock and formatting edits are not separate runtime decisions.

The decision traces to the stated untrusted-checkout problem, but it does not close that problem: installed package names still execute. It also changes a documented released contract without updating its source-of-truth documentation or release category.

VERDICT: BLOCKS — the field-level grammar is a partial trust fix that breaks the documented path contract

ARCHITECTURE

OWNER: theme selection belongs to resolveTheme; the execute-project-code trust boundary belongs to the shared CLI module-loading policy. TIER 1: CLI foundation/module loading; no component-system owner applies. TIER 2: none. SEAMS: package.json vs ASTRYX_THEME; text vs --json; local vs external CLI install; package resolution; astryx doctor. BEHAVIOR UNIT: pure resolver helper — focused tests are appropriate, but trust policy duplicated here does not cover the package loader or the sibling project-code loaders.

The exact same installed-package probe writes its marker before and after this PR. #5531 is already attempting the cross-cutting gate in the shared loader, though its current head still misses direct imports. The bounded direction is one complete opt-in project-code gate, with this resolver participating, while preserving default documented behavior.

VERDICT: BLOCKS — security policy is placed on one config field instead of the execution boundary

IMPACT

Builders using the documented local custom-theme setup lose their custom variant annotations from text astryx component output after upgrading. In a real pnpm install, file-variant* appeared under published 0.5.0 and disappeared on this head; the process still exited 0 and doctor still said the field was wired when an official theme package was installed.

Builders using bare/scoped package names keep working: probe-theme-pkg, @probe/theme, neutral, and @astryxdesign/theme-neutral all loaded in the installed-like matrix. Repo-local compatibility search found 0 committed package.json files using this field, but the public guide has prescribed the path form since June 2026.

VERDICT: BLOCKS — the upgrade breaks the documented custom-theme workflow and gives contradictory diagnostics

API

Real documented call site:

{
  "astryx": {"theme": "./src/theme.ts"}
}

	change	public?	class	doc'd?	verdict
`~`	`package.json#astryx.theme: string` narrows from file/package specifiers to bare or scoped package names	yes — consumed by published CLI 0.5.0	existing theme-source selection beside `ASTRYX_THEME`	yes — official Theming Infrastructure guide documents `./src/theme.ts`	breaking; BLOCKS

Ossification: no new surface is added, but existing released surface is narrowed. The old path form shipped in 0.5.0 and was introduced as an explicit resolver strategy in #790. The PR's [fix]/patch changeset does not match the repo's rule that a changed meaning on published surface is [breaking]/minor; preserving the path form avoids that migration entirely.

VERDICT: BLOCKS — released config meaning changes without the breaking category, synchronized docs, or a coherent migration

THEMING

No component theme targets, tokens, CSS, or runtime app theming change. This changes only how the CLI discovers a theme object for custom-variant documentation.

VERDICT: clear — no theme surface is added or removed

BREAKING

API/config: yes — the documented ./src/theme.ts value is rejected.
Visual: no — no rendered component or DOM changes.
Theme: yes, in CLI introspection only — custom theme variants disappear from text component docs.
Behavior: yes — a previously loaded file now warns, returns no theme, and exits 0; --json is unchanged because it returns before resolveTheme on both revisions.

This is reachable in the current published package: npm reports @astryxdesign/cli@0.5.0 as latest, its tarball contains path loading, and the official guide tells builders to use the path form.

VERDICT: BLOCKS — documented released behavior is removed under a patch changeset

PERFORMANCE & RESOURCES

Effects: zero. No subscriptions, listeners, observers, layout reads, dependency, or bundle-bearing import is added. The only new work is one module-level regex and one string test per text-mode component invocation; no work scales with user-controlled N.

No performance measurement was needed because the blocking findings are contract and architecture defects, not a claimed degradation.

VERDICT: clear — no meaningful performance or resource regression

VISUAL EVIDENCE

No frames: this is a Node CLI resolver. The diff contains no JSX, CSS, StyleX, DOM, geometry, or color; visual/a11y/RTL CI jobs correctly skipped on this head.

VERDICT: clear — nothing renderable changed

A11Y & I18N

No rendered or assistive-technology surface is reachable. The one new terminal warning is raw English, matching the CLI's existing uncatalogued terminal-output convention, and it names both the invalid value and the migration knob.

Platform forms were exercised as literal CLI inputs: POSIX relative/absolute, Windows relative/drive/UNC, file: URL, package subpath, bare package, scoped package, and the neutral shorthand. All path-shaped package values were rejected on this head; package names still loaded.

VERDICT: clear — no accessibility or localization regression found

JUDGEMENT

slot	verdict
PROBLEM	BLOCKS — documented configuration is removed; package execution remains
SOLUTION	BLOCKS — partial trust fix breaks the public contract
ARCHITECTURE	BLOCKS — policy is below the shared execution boundary
IMPACT	BLOCKS — documented custom variants disappear while doctor reports wired
API	BLOCKS — released config meaning changes under a patch
THEMING	clear
BREAKING	BLOCKS — config/theme-tool behavior
PERFORMANCE	clear
VISUAL EVIDENCE	clear
A11Y & I18N	clear

GOAL: partly met — path-shaped package.json values stopped executing in every probed form, but a package named by the same checkout still executed before and after.

DISPOSITION:

documented ./src/theme.ts stops loading and its custom variant disappears → blocks now; confirmed by the official guide plus published-0.5.0/head CLI runs
doctor reports the rejected field as wired → blocks with the contract change; confirmed by the independent doctor and component invocations
valid package names still execute checkout-selected code → blocks this as the trust-boundary solution; confirmed in both extracted and real pnpm-installed layouts
regex differs from npm's validator on legacy/special/over-214 names → omitted from the public review; no demonstrated theme consumer and no additional user harm beyond resolution failure

ADVICE: bounded direction — preserve default file-path support and finish one explicit project-code gate at the shared execution boundary, including resolveTheme. Acceptance: the documented path still prints file-variant* by default; with the gate enabled, neither path nor package markers execute and diagnostics agree.

WORST OUTCOME: “a builder following the official theme config loses custom variants while astryx doctor says the field is wired” → request changes.

The existing cixzhang comment correctly identified that package names still execute, but it concluded there was no defect in this diff and said every astryx component reads the field. This pass reverses the first conclusion because it found the documented/released path contract; it narrows the second because component --json returns before resolveTheme and never executes the configured theme on either revision.

JUDGEMENT NEEDED: none — the public contract, false diagnostic, and incomplete trust boundary are reproducible defects rather than an API taste call.

request changes

[BLOCKS] the documented local-theme config is removed under a patch → a builder following the guide loses custom variants while doctor reports the field wired · packages/cli/clients/cli/lib/resolve-theme.mjs:139 — if (!fromEnv && !PACKAGE_NAME.test(specifier)) { confirmed independently by the public guide and before/after installed CLI probes
[BLOCKS] package names still execute code selected by the checkout → the untrusted-checkout workflow remains executable under the new accepted grammar · packages/cli/clients/cli/lib/resolve-theme.mjs:167 — mod = tryLoadModule(`@astryxdesign/theme-${specifier}`, cwd); confirmed independently by source resolution and marker-writing package probes in two install layouts

REVIEW

Thanks — I need to correct my earlier comment. I missed that the public theme guide explicitly tells builders to use "astryx": {"theme":"./src/theme.ts"}. I ran that exact setup against published 0.5.0 and this head: 0.5.0 prints the custom variant; this head drops it and warns, while astryx doctor still reports the field as wired when an official theme package is installed.

The field-level check also leaves the trust problem open: a locally installed package named by the checkout executes before and after. Ideally keep documented path support here and make #5531's shared opt-in gate cover this resolver too. Could we take that direction—or talk it through in Discord?

INLINE

.changeset/cli-theme-package-names-only.md:2 — This removes a documented config value, so patch understates the migration.
packages/cli/clients/cli/lib/resolve-theme.mjs:139 — The public theme guide still configures ./src/theme.ts; this guard rejects its exact example.

EVIDENCE I DID NOT SPEND

component --json never calls resolveTheme on either revision; the PR body's “every invocation” claim is broader than runtime.
Repo search found 0 committed package.json instances of astryx.theme; this limits in-repo impact but does not erase the shipped documented contract.
The regex admits names npm rejects for new publication (foo~bar, 215 characters) and rejects some legacy package names; no Astryx theme consumer was found in those cases.

Round 2 — R1e after author update

PR

#5525 fix(cli): theme loading resolves from the project and obeys the safe-mode gate by bhamodi (bucket: contributor)

HEAD REVIEWED

d841caba9209a9288409bd08dc306af05beae2e5

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: Security/trust behavior, an unresolved prior changes-requested review, public CLI config behavior, and paired PR #5531 require more than one decisive check.

PROBLEM

WHY 1: Theme resolution used the CLI installation as the package lookup root and had no way to avoid executing checkout-selected theme modules. WHY 2: Builders could not load a theme package installed only in their project, while CI/agents inspecting an arbitrary checkout could execute its file or package theme. WHY 3: The CLI must describe the project it is run in accurately without making untrusted checkout code execution unavoidable.

USER-FACING PROBLEM: A builder can lose project theme data because lookup starts in the wrong dependency tree, and an operator inspecting an untrusted checkout cannot safely opt out of its theme code. PROBLEM SEVERITY: broken task — project-local package themes do not resolve on warm main, and the untrusted-checkout path has no theme-code gate there.

R1e: the prior Robohands reviews asked to preserve the documented {"astryx":{"theme":"./src/theme.ts"}} workflow and gate both file and package theme loading. This head satisfies both asks; the new finding below extends the trust-boundary review rather than contradicting it.

VERDICT: clear — the problem and the prior asks are now explicit and reproducible

SOLUTION

The resolver loads file themes with the existing user-module loader and starts package lookup from the project. Before either route, one environment switch returns theme-less without importing the configured module. The component command now waits for that asynchronous result before formatting its text output.

SOLUTION (3 decisions · ~35 runtime lines of 327 changed lines)

Load documented file themes through the shared TypeScript-capable module loader.
Resolve package themes from the project's dependency tree.
Gate config-file, config-package, and ASTRYX_THEME loading before execution; await the result at the only caller.

BURDEN: low — one async boundary, one environment check, one process-local notice flag, no listener/timer/observer/global service, and focused resolver/CLI tests. BURDEN MATCH: proportionate — the machinery directly restores the documented workflow and closes all theme-module execution paths under the named gate.

VERDICT: clear — each decision traces to a stated failure and the burden is bounded

ARCHITECTURE

OWNER: resolveTheme owns theme-source selection; importUserModule owns file-module execution; project-bound createRequire owns package resolution. TIER 1: shared CLI module loading is reused for file themes; #5531 supplies the same environment policy to config/doc/integration loaders. TIER 2: none. SEAMS: package config vs ASTRYX_THEME; file vs package; default vs safe mode; resolver vs component command; standalone merge vs paired #5531. BEHAVIOR UNIT: pure async resolver — its source-selection and gate matrix is directly testable without a server.

seam	driven result
documented `./src/theme.ts`	loads through jiti and returns `button: review-probe-variant`
project-only `@acme/theme`	resolves on head; does not resolve on warm main
config file / config package / `ASTRYX_THEME` under gate	all return null; all execution markers remain absent
real `component Button --detail full`	default shows the custom variant; safe mode omits it and leaves the marker absent
terminal notice	checkout-controlled newline and OSC control bytes pass through unchanged

The loading ownership is now correct. The remaining defect is at the trust boundary's output seam: safe mode executes no module, but writes an untrusted config value directly to the terminal.

VERDICT: BLOCKS — the safe-mode diagnostic permits checkout-controlled terminal output injection

IMPACT

Default users keep the documented TypeScript file workflow, and project-installed package themes now work. Operators setting ASTRYX_NO_PROJECT_CODE=1 get theme-less output with no file, package, or environment-selected theme execution.

A malicious checkout can put newline or terminal-control bytes in package.json#astryx.theme; the new warning emits them verbatim. In the mode intended for untrusted checkout review, that checkout can reshape or spoof the diagnostic stream even though its module never runs.

VERDICT: BLOCKS — untrusted checkout data can alter the safe-mode terminal output

API

No exported package API changes. resolveTheme(cwd) changes internally from a synchronous value to a Promise; the only production caller now awaits it.

	change	public?	class	doc'd?	verdict
`~`	internal `resolveTheme(cwd): ThemeData	null`→`Promise<ThemeData	null>`	no — CLI-private module	one production caller, updated
`+`	`ASTRYX_NO_PROJECT_CODE=1` applies to file, package, and `ASTRYX_THEME` theme sources	public CLI behavior; concept owned by paired #5531	existing safe-mode concept across project-code loaders	#5531 README; this PR changeset	ok, prior review explicitly required participation

OSSIFICATION: this PR does not introduce a second gate name or public config spelling. It makes theme loading participate in the paired, already-selected safe-mode concept. Being wrong would create divergent trust behavior; the direct three-source probe shows one behavior today.

VERDICT: clear — no new independent API concept or unreachable public type

THEMING

No component target, token, CSS variable, generated theme rule, or rendered theme contract changes. The CLI's existing theme-source config retains its documented meaning and gains correct project package lookup plus an opt-in execution gate.

VERDICT: clear — no theme surface is added, removed, or renamed

BREAKING

BEHAVIOR: default behavior is preserved for documented file themes and package themes; the opt-in safe mode intentionally returns no theme data. The real CLI probe confirmed both endpoints. API: no exported signature; the internal async conversion has one caller and it awaits. VISUAL: no JSX, DOM, CSS, StyleX, component output, or pixel path changed. THEME: no target/token/override change; custom variants remain available by default.

VERDICT: clear — no default-path break found

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no React/render path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no new dependency; existing jiti loader reused. Theme resolution remains one package-json read plus one module lookup per text-mode component invocation.

VERDICT: clear — no scaling or persistent-resource change

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: changed paths are Node CLI source, one Node test, and a changeset; there is no rendered component, CSS, DOM, geometry, color, or interactive visual endpoint.

Visual solution exploration: 0 minutes. Screenshot capture: none. Browser inspection: none.

VERDICT: clear — no visual claim exists

A11Y & I18N

No DOM, ARIA, focus, keyboard, direction, or localized application string changes. The changed terminal notice follows the CLI's existing English-output convention, but its raw interpolation is handled as the security/output-integrity block above rather than an i18n finding.

CI's pr-a11y, pr-rtl, and Stable visual regression correctly skipped because no component changed; visual acceptance reported success with “No stable visual scope.”

VERDICT: clear — no accessibility or localization regression found

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	BLOCKS — raw untrusted value reaches terminal
IMPACT	BLOCKS — safe-mode output can be spoofed
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear — not applicable
A11Y & I18N	clear

GOAL: partly met — the documented TypeScript path, project package resolution, and all three no-execution paths pass, but the safe-mode diagnostic still emits checkout-controlled terminal controls.

DISPOSITION: raw astryx.theme text in the safe-mode warning → blocks now; reproduced with both an OSC sequence and newline reaching the captured warning unchanged.

ADVICE: proven acceptance criteria — omit the configured value or render an escaped representation; a malicious value must remain one inert physical line with no terminal control byte in output.

AUTHOR CAN PROCEED: yes — keep the restored loading/gate behavior and make the safe-mode notice inert for arbitrary checkout strings.

WORST OUTCOME: “the checkout can reshape or spoof the diagnostic stream in the mode intended for untrusted checkout review” → request changes.

JUDGEMENT NEEDED: none — output neutralization at a trust boundary is a correctness requirement, not an API/design choice.

request changes

noteThemeSkipped interpolates checkout-controlled astryx.theme directly → an untrusted checkout can inject new lines or terminal controls into safe-mode output · packages/cli/clients/cli/lib/resolve-theme.mjs:107

REVIEW

Thanks — I rechecked the two prior Robohands asks. This head now preserves the documented TypeScript theme path, resolves packages from the project, and keeps file, package, and ASTRYX_THEME modules unexecuted under the gate.

One security edge remains: resolve-theme.mjs:107 writes the checkout-controlled theme value directly to the terminal. A value containing a newline or ANSI control sequence can spoof the output in the untrusted-checkout mode; I reproduced both reaching the warning unchanged. Could we escape or omit that value so the notice stays one inert line? If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

INLINE

packages/cli/clients/cli/lib/resolve-theme.mjs:107 — Could we escape or omit this checkout-controlled value? Newlines and terminal controls pass through.

EVIDENCE I DID NOT SPEND

The head's focused resolver suite passes 12/12; all substantive remote CI checks passed.
The paired #5531 currently uses the same exact environment switch and can land in either order without a code conflict.
The component command diff is one semantic await; the remaining line changes are formatting.

TIME

TIME total: 133m

setup/rules: 6m — full kit, private controls, repo rules, official wiki; warm main reused
install/build/server: 2m — guarded fast-install; no branch build or server needed
visual exploration: 0m — nonvisual change
screenshot capture: 0m — not applicable
browser/a11y: 0m — not applicable
focused tests/probes: 2m — 12-test file plus three guarded direct probes
code/history: 120m — exact diff, prior reviews, paired PR, call sites, main movement; includes 118m stalled GitHub read
critique + wiki: 3m — one clean critic pass, immutable gate 3, serialized wiki update
CI wait: 0m — current exact-head CI recorded without waiting
waste: 118m — paired-PR GitHub history read stalled; no result was needed for the verdict

WHAT I COULD NOT VERIFY

Nothing material to this verdict.

What changed before posting

Not posted — read-only R1e review; no GitHub review, comment, merge, or auto-merge action was taken.

Review 5525

Review #5525 — CLI theme config resolution

PR

HEAD REVIEWED

VERSIONS

PROBLEM

SOLUTION

ARCHITECTURE

IMPACT

API

THEMING

BREAKING

PERFORMANCE & RESOURCES

VISUAL EVIDENCE

A11Y & I18N

JUDGEMENT

REVIEW

INLINE

EVIDENCE I DID NOT SPEND

Round 2 — R1e after author update

PR

HEAD REVIEWED

VERSIONS

LANE

PROBLEM

SOLUTION

ARCHITECTURE

IMPACT

API

THEMING

BREAKING

PERFORMANCE & RESOURCES

VISUAL EVIDENCE

A11Y & I18N

JUDGEMENT

REVIEW

INLINE

EVIDENCE I DID NOT SPEND

TIME

WHAT I COULD NOT VERIFY

What changed before posting

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!