Skip to content

chore(doctor): parity-test the base-path port, fix the versions false positive, validate config at boot #1300

Description

@vivek7405

Problem

Three cleanups in webjs doctor and the webjs config-validation story, found while shipping the doctor severity gate (#1257, PR #1296). They are one issue because they sit in two files and share one verification loop, so one PR carries them.

None is a regression from #1257. Each predates it; the gate merely made them visible by putting doctor in the required conventions CI job.

1. readAppBasePath is a second, hand-maintained implementation of the server's base-path normalization. packages/cli/lib/doctor.js:1274 re-implements readBasePath (packages/server/src/base-path.js:92): trim, empty / / to '', reject .. / :// / backslash / whitespace, reject a //host network-path reference BEFORE collapsing leading slashes, then collapse and strip a trailing slash. Change base-path semantics in one and the UNMARKED_ASSET_LINKS check silently disagrees with what the server serves. The drift risk is not hypothetical: base-path.js was created in #298 and changed again in #1237 (fix!: key the HTML cache by origin and centralize proxy trust), while the port landed later in #1244.

2. The webjs JSON Schema never validates anything outside an editor. packages/server/webjs-config.schema.json reaches users through exactly one wire, packages/cli/templates/.vscode/settings.json:9 ("$ref": "./node_modules/@webjsdev/server/webjs-config.schema.json"). So a typo'd key is caught only for a VS Code user with the file open. The module docblock of packages/core/src/webjs-config.d.ts states the schema's purpose as closing exactly that gap ("a typo'd key (e.g. redirect for redirects) was silently dropped and the feature stayed at its default with no diagnostic. This type plus the published JSON Schema close that gap"). Outside an editor it does not close it. This is why #1257 had to hand-write shape validation in the CLI for doctor.gate rather than lean on the schema.

3. WEBJS_VERSIONS false-positives on every in-repo app. checkWebjsVersions reads join(appDir, 'node_modules', dep, 'package.json') (packages/cli/lib/doctor.js:883). Under npm workspaces the @webjsdev/* deps hoist to the ROOT node_modules, so an app subdirectory has none and the check reports "N @webjsdev/* dependency not installed" for all four of examples/blog, website, docs, packages/ui/packages/website. The install is fine. The check is therefore ungatable today.

Design / approach

1. A parity test, NOT a dedupe

Read readAppBasePath's docblock (packages/cli/lib/doctor.js:1267-1270) before touching it. The port is deliberate and one of its two reasons is load-bearing: readBasePath is not on @webjsdev/server's public surface, AND doctor must stay usable when the framework does not resolve from the app dir at all, which is the #954 fresh-worktree case this very command exists to diagnose (checkFrameworkResolves, same file, is the check for it). Importing the server helper unconditionally would break doctor in the situation doctor is for.

So keep the port and make drift DETECTABLE: one parity test that feeds an identical input table through readBasePath and readAppBasePath and asserts identical output. That converts silent drift into a red test for a few lines. Record the test's name in the readAppBasePath docblock so the next person to edit either side finds it.

Rejected: a lazy import with fallback to the port. It doubles the code paths and still needs the parity test to prove the two agree.

2. Promote the validator that already exists, and WARN at boot

Two things make this much cheaper than it looks.

The validator is already written. packages/server/test/config/webjs-config-schema.test.js:220 defines validateWebjsBlock(schema, value), whose own docblock calls it "a tiny structural validator standing in for ajv (which the repo does not ship)". It checks unknown-key membership under additionalProperties: false, enum membership, and boolean / integer leaf types. That is exactly the typo case. It is trapped in a test file.

Prior art says warn, not throw. Next.js validates its config on every boot (validateConfigSchema, next.js/packages/next/src/server/config.ts:2500), running configSchema.safeParse(userConfig) and splitting the result: unknown or invalid options are warnings that print and let the boot continue; only required or migrated options are fatal.

So: move validateWebjsBlock into packages/server/src/, call it at server boot against the app's webjs block, and WARN on every problem without ever throwing. No new dependency, no build step. The test then imports the promoted function instead of keeping its own copy, which also removes a second implementation rather than adding one.

A warn is the right severity: a typo'd key is drift, not a broken toolchain, and the same judgement is why webjs check (correctness) is the wrong home for it.

3. Add version to the hoist-aware reader, then switch

A sibling check in the same file already solved hoisting: checkImportmapCoherence uses getPackageManifest(pkg, appDir) from @webjsdev/server (packages/cli/lib/doctor.js:740), falling back to the naive local read only when the server build predates it. Mirror that.

But getPackageManifest (packages/server/src/vendor.js:297, exported at packages/server/index.js:26) returns only { dependencies, peerDependencies }. The hoist-aware resolution actually lives in resolvePackageDir (vendor.js:238, createRequire from the app's package.json, then walk to the package root), which is NOT exported. So add a version field to getPackageManifest's return and use that, which is a smaller public-surface change than promoting resolvePackageDir.

Deliberately OUT of scope: the ELISION_CARRIERS warnings

The original filing listed six page/layout modules that ELISION_CARRIERS flags across examples/blog and website, proposing to fix them and then gate the code to error. Do not do this. The premise was checked and does not hold.

Both named blockers are pure. examples/blog/lib/utils/cn.ts and website/lib/ui/docs-shell.ts reference no browser global at all (grep for document / window / navigator / localStorage / matchMedia outside comments returns nothing in either).

They are flagged by hasModuleScopeSideEffect (packages/server/src/component-elision.js:312), which is a lexical scanner rather than an AST parse. Its own comment states the failure mode: regex bodies are not tracked, so a stray quote inside a regex literal shifts quote pairing, and at that point "the lexical state is unreliable below here, so ship conservatively" (it returns true). cn.ts is precisely that shape, a module-scope const GROUPS: Array<[RegExp, string]> = [...] of regex literals containing quotes.

So these are analyser false positives, not app debt. Editing the apps would mean contorting a pure utility to dodge a scanner limitation, and it would not generalise: the next pure module with a regex trips the same wire. The real work, if wanted, is analyser precision (track regex literals in the redaction pass), which changes what ships to browsers, is a surface where a miss is expensive, needs its own differential verification, and has nothing to do with doctor. File it separately if it matters.

Consequently ELISION_CARRIERS stays ungated in website/package.json and examples/blog/package.json, and that is the correct end state, not a deferral.

Implementation notes (for the implementing agent)

Where to edit:

  • packages/cli/lib/doctor.js: readAppBasePath at L1274 and its docblock at L1262-1272 (item 1); checkWebjsVersions at ~L843 with the naive read at L883 (item 3); the hoist-aware precedent to copy is at L740.
  • packages/server/src/base-path.js: readBasePath at L92 (item 1, the parity partner).
  • packages/server/test/config/webjs-config-schema.test.js: validateWebjsBlock at L220 (item 2, the function to promote out of the test).
  • packages/server/src/: new home for the promoted validator, plus the boot path that calls it (item 2).
  • packages/core/src/webjs-config.d.ts: the module docblock whose claim item 2 makes true.
  • packages/server/src/vendor.js: getPackageManifest at L297, resolvePackageDir at L238 (item 3).
  • packages/server/index.js: L26 exports getPackageManifest (item 3, the export surface to extend).

Landmines / gotchas:

  • Item 1 is not a dedupe. Importing the server helper breaks doctor in the fresh-worktree case (dogfood: a fresh git worktree can't resolve @webjsdev/* (no node_modules) #954) that doctor exists to diagnose. Read the docblock first.
  • Item 3's obvious fix is a trap. getPackageManifest returns no version, so a drop-in swap silently reads undefined and the check passes vacuously, which is worse than the current false positive. Add version first, then switch, and assert the version actually comes back.
  • Item 2 must not throw. A boot that aborts on a typo'd key is a bigger behaviour change than the problem warrants, and it diverges from the Next precedent this is modelled on. Warn and continue.
  • Do not add a validation dependency. The repo ships no ajv on purpose; the existing tiny validator is the whole point of reusing it.
  • A fresh worktree has no packages/core/dist. Do NOT symlink it from the primary checkout: that serves a bundle built at the primary's commit and reds roughly 29 e2e tests while CI is green. Build it in the worktree (npm run build:dist --workspace=@webjsdev/core).
  • A whole-directory node_modules symlink resolves @webjsdev/* to the PRIMARY checkout, so edits to packages/*/src are invisible to tsc and to spawned CLI runs. Link per package instead.
  • webjs doctor on website performs a live jspm resolve and takes about 6s; it is warn-only and cannot red CI (see test: keep a live jspm outage from redding the required CI job #1150 for the wider live-CDN-in-required-CI concern).
  • Touching a webjs.* key means the three-surface lockstep plus the KNOWN_KEYS drift test. The canonical reader inventory lives in packages/server/AGENTS.md; do not restate it elsewhere (four copies drifted before PR feat: declare per-check doctor severity in webjs.doctor.gate #1296 made it canonical).

Invariants to respect:

  • Root AGENTS.md: webjs check is correctness, webjs doctor is project health. Item 2 belongs to neither; it is a server-boot warning. Do not smuggle it into check.
  • packages/cli/lib/doctor.js stays PURE: no process.exit, no printing. The bin owns the exit.
  • packages/ is plain .js with JSDoc. No .ts files there.
  • Prose invariant 11 (no em-dashes, no space-hyphen or space-semicolon pauses, WebJs capitalized in prose) applies to every comment and doc touched.

Tests + docs surfaces:

  • test/cli/doctor.test.mjs: the base-path parity table (item 1) and a workspace-shaped fixture for WEBJS_VERSIONS (item 3). The fixture IS the counterfactual: it fails against the naive per-app read.
  • packages/server/test/config/webjs-config-schema.test.js: import the promoted validator rather than redefining it, and add boot-warning coverage (item 2).
  • Docs: packages/cli/AGENTS.md doctor row if the check's behaviour description changes; packages/core/src/webjs-config.d.ts docblock (item 2 makes its claim true, so it can finally be stated without qualification); .agents/skills/webjs/references/built-ins.md and website/app/docs/configuration/page.ts if boot-time config warnings become a documented behaviour.

Acceptance criteria

  • A parity test proves readAppBasePath and readBasePath normalize an identical input table identically, and it fails if either side drifts
  • The readAppBasePath docblock records that the port is intentional (dogfood: a fresh git worktree can't resolve @webjsdev/* (no node_modules) #954) and names the parity test that guards it
  • validateWebjsBlock lives in packages/server/src/, the schema test imports it instead of defining its own copy, and no validation dependency was added
  • A typo'd webjs key WARNS at server boot and the boot still completes
  • WEBJS_VERSIONS passes on all four in-repo apps, resolving the installed version hoist-aware, and the resolved version is asserted non-undefined
  • A workspace-shaped fixture is the counterfactual for the above, failing against the naive per-app read
  • ELISION_CARRIERS is left ungated and unedited, per the out-of-scope section
  • Tests cover the new behaviour at every layer it touches
  • Docs / AGENTS.md updated if the public surface changed

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

Status
Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions