feat: declare per-check doctor severity in webjs.doctor.gate - #1296
Conversation
`webjs doctor` could only be all-or-nothing: the default exit fails on a broken toolchain, and `--strict` makes every warning fatal, including four that are environment-shaped and would red a clean CI run (the git hook, env drift, vendor-pin freshness, and framework resolvability). So no CI ran it, and every advisory it carries was findable but not unmissable. Per-check severity is now config rather than a flag, on the three-level scale ESLint uses and `eslint-plugin-next` ships as a rule-id-keyed map. An app declares it once in package.json, so its workflow, its `npm run doctor`, and an agent's `--json` loop read the same policy instead of three copies of a code list. Four rules keep it safe. A code with no entry keeps its default, so an app with no block produces byte-identical counts and the exit formula is untouched. A result's `severity` is the effective level, so a passing check reports `pass` even when its code is gated `error`. A best-effort result (a network or toolchain outage) is capped at `warn` and can never be escalated, which is what makes doctor safe inside a required job. And an unknown code or severity exits 1 naming it, because a silently-ignored typo would leave CI un-gated while looking gated. Refs #1257
Nothing ran `webjs doctor`, so the advisory that exists because an un-versioned stylesheet url shipped a visible regression on webjs.dev could not have caught that regression. Now the repo's own `conventions` job runs doctor over the same four in-repo apps as its `webjs check` loop, and the scaffold's CI template runs `npm run doctor`. Both `website` and `examples/blog` gate `UNMARKED_ASSET_LINKS` to error, and the scaffold emits the same gate. Every other code stays at its default warn, so the environment-shaped checks (git hook, env drift, vendor-pin freshness over the network, framework resolvability) cannot red a clean run. Measured before choosing: all four apps report zero fails today and pass `UNMARKED_ASSET_LINKS`, while `ELISION_CARRIERS`, `WEBJS_VERSIONS`, and `ENV_DRIFT` warn here, so gating any of those would have redded main. The step goes INSIDE the existing `conventions` job rather than a new one. Branch protection matches the required context by display name, so a new job would gate nothing and a rename would silently un-require the job. Refs #1257
|
Design rationale: the three calls the diff does not explain on its own
The help gained a generic Counterfactual, proven at |
The `off` test hard-coded "3 silenced", which is the number of checks that happen to warn in a tmp fixture on Node. Under Bun the count is 2, because `FRAMEWORK_RESOLVE` resolves `@webjsdev/core` from a tmp dir there and passes. So the assertion redded the Bun matrix over a difference the test is not about. Assert the outcome instead: nothing left to warn about, and the summary reports some checks silenced. That is what "off silences a warn even under --strict" actually claims, and it holds on both runtimes. Refs #1257
|
Bun parity: a test of mine, not the feature, diverged The Bun matrix went red on the first push and it was worth chasing rather than re-running, because a doctor gate is exactly the kind of thing that could behave differently per runtime. It did not. Fixed in Worth noting for anyone extending these tests: the environment-shaped checks ( |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff. The mechanism itself holds up: policy is folded in at the summary layer, the exit formula is untouched, and the bestEffort cap is the right shape for putting a network-touching check inside a required job. The counts and the exit are byte-identical without a gate block, which is the promise that matters most here.
Where it falls down is the prose. Three problems, all doc surfaces, and one of them is a real behaviour mismatch rather than a wording nit: four separate places say off "silences the check entirely" while the renderer still prints the finding and its Fix line. If we are going to lean on the ESLint comparison, off there removes the message; it does not print it with an [off] marker. Either the render changes or all four strings do.
The other two are misses in files the diff did not touch but should have. packages/server/AGENTS.md still calls dev / start "the one exception" for a CLI-read key, which this PR makes false, and it is exactly the file packages/cli/AGENTS.md now points the reader at. And the root AGENTS.md config-block bullet enumerates every webjs.* key and does not list doctor, so an agent reading only that bullet cannot learn the key exists.
Two of the three are path-level (the lines are outside the diff), noted here rather than inline.
Path-level: packages/server/AGENTS.md (L158-166). Says the dev / start task keys are "the one exception" for a CLI-read key living in the webjs block. doctor is now a second one, so that sentence and the reader enumeration above it are wrong.
Path-level: AGENTS.md (L527). The config-block bullet lists headers, csp, redirects, trailingSlash, basePath, the ingress caps, and the dev/start tasks. webjs.doctor is missing from the inventory.
Four doc surfaces said a gated `off` "silences the check entirely", and it did not: the renderer still printed the finding and its `Fix:` line, because that guard keys off `status`, which an off result still fails. Only the counts and the exit were suppressed. An app that turns a code off has asked not to hear about it, so printing the message and a remedy on every run is exactly the noise it just silenced. ESLint's `off` drops the message too, and the whole gate is modelled on that scale. So the render now prints the `[off]` line alone. The check stays on the checklist and in the summary's silenced count, and `--json` still carries the whole result, so a silenced check is never invisible to a person or to tooling. Also corrects two config-inventory surfaces the key was added to without their prose being updated. `packages/server/AGENTS.md` called the dev/start task keys "the one exception" for a CLI-read key in the `webjs` block, which `doctor` makes false, and that is the exact file `packages/cli/AGENTS.md` points the reader at. Root `AGENTS.md`'s config-block bullet enumerates every `webjs.*` key and did not list `doctor`, so an agent reading only that bullet could not learn it exists. Refs #1257
|
Both path-level findings fixed in
The second one is the finding I would not have caught on my own. I updated the CLI-table row for |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 1f99ddda and its blast radius. The off render change itself is right, and the test covers both halves (a silenced check prints nothing, a warned one still prints its finding and Fix line). Two problems, both in what that commit reached for and did not finish.
Path-level: .agents/skills/webjs/references/built-ins.md (L11). The "What This Covers" bullet enumerates the config block (headers, CSP, redirects, trailing-slash, basePath, allowed origins, client-router, ingress caps, dev/start tasks) and does not list the doctor gate, even though this PR adds a whole ### Doctor severity gate subsection further down the same file. This is the identical defect the commit fixed one file over in root AGENTS.md, and it is the worse of the two: root AGENTS.md routes an agent to this reference for the webjs block, so this is the inventory that actually gets read.
Path-level: packages/server/AGENTS.md (L159-163). The rewrite says "Two keys are CLI-read rather than server-read: the dev / start task keys ... and the doctor severity gate". That is false for dev. webjs.dev.regenerate is read by the SERVER, by readRegenerateRules (packages/server/src/dev-regenerate.js:53, called from dev.js:802 and dev.js:1163). The same package's schema and the WebjsConfig type both say so. The old text was already loose, but this hunk hardened it into a two-key classification and left a real server reader off a list whose stated job is to tell you which reader to update.
The skill reference's "What This Covers" bullet enumerates the `webjs` block and did not list the doctor gate, even though the same file now carries a whole section on it. That is the same defect already fixed in root AGENTS.md, and it is the worse copy: root AGENTS.md routes an agent to this reference for the config block, so this is the inventory that actually gets read. The other correction is mine. Rewriting the server's lockstep note to name both CLI-read keys hardened a loose sentence into a false one: `dev` is not CLI-read, it is SPLIT. Its task sub-keys go through `readAppTasks` in the CLI, but `dev.regenerate` is read by the server's `readRegenerateRules`, which the schema and the `WebjsConfig` type both already said and which the reader list omitted. An agent adding a `dev.*` sub-key follows that list, so leaving a real server reader off it is the failure the list exists to prevent. It now says the question is per sub-key, not per top-level key. Refs #1257
|
Both fixed in The skill reference's "What This Covers" bullet now lists the doctor gate. Fair catch that this was the worse of the two parallel inventories: root The second one is the more useful finding, because I introduced it. The original sentence was loose and I rewrote it into something firmer and wrong: |
Four places enumerated which function reads each `webjs.*` key: the server AGENTS.md procedure, the JSON Schema's `$comment`, the `WebjsConfig` docblock, and the drift test's docblock. They drifted independently, and three review rounds on this branch each found a different stale copy, so patching the third one would only have queued up the fourth. The server AGENTS.md list is now the canonical one and the other three point at it instead of repeating it. That list also gains the two server readers every copy had lost track of, `readAllowedOrigins` and `readDevWatchPathsFromApp`, and states the two things the copies kept getting wrong: the CLI-vs-server split is per SUB-KEY (`dev` has CLI-read task keys AND two server-read ones), and where a reader lives changes nothing about what the lockstep owes. Also corrects the skill reference's claim that a malformed config entry is always dropped with a warning. That holds for the keys the server reads, and is the opposite of what `doctor.gate` does on purpose: a typo quietly ignored would leave CI un-gated while looking gated. And teaches the generated app about its own new gate. The scaffold ships CI that runs `npm run doctor` and a gate that makes `UNMARKED_ASSET_LINKS` fatal, but its workflow rules, both playbooks, and its PR template still listed only `npm run check`, so an agent following them would push green locally and red in CI. Both templates were generated and verified: `doctor` and `check` both exit 0 on a fresh app. Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 3b11b549 and its blast radius. Six problems, and the shape of them is the finding: five are the SAME defect in five different files, which says the previous two rounds were treating instances rather than the class.
Path-level: packages/core/src/webjs-config.d.ts (L15-24). A fourth copy of the reader inventory. It lists no readDoctorPolicy, no readAppTasks, no readRegenerateRules, and still opens "The server reads this object key by key", which doctor makes false. This PR edits this file and left it.
Path-level: packages/server/webjs-config.schema.json (L5-6). The description says each key maps to "a server reader, or for dev/start the CLI's readAppTasks", and the $comment re-enumerates the readers with the same omission. The PR adds the doctor property to this same file, whose own per-key description says it is CLI-read, so the file contradicts its header.
Path-level: packages/server/test/config/webjs-config-schema.test.js (L32, L55-57). The docblock still says dev / start are CLI-read rather than server, which is the exact framing the previous commit declared false for dev, and the same file already contradicts it further down. The new doctor entry was added into that stale framing.
Path-level: packages/server/AGENTS.md (L152-172). The list I just "completed" is still incomplete. dev has a THIRD sub-key, dev.watch (#894), read by the server's readDevWatchPathsFromApp, which the new sentence assigns to neither named reader. readAllowedOrigins (csrf.js:149, in KNOWN_KEYS) is missing too. By the previous commit's own standard, leaving a real server reader off this list is the failure the list exists to prevent.
Path-level: .agents/skills/webjs/references/built-ins.md (L148). The config-block section opens "a malformed entry is dropped at boot with a warning, never crashing the pipeline". False for the key this PR adds: a malformed doctor.gate exits 1 without running a check, which the new section 65 lines below presents as the whole point.
Path-level: packages/cli/templates/.agents/rules/workflow.md (L53). The generated app's "Every code change" gate list says only npm run check must pass, while this same PR adds a failing-capable npm run doctor step to the scaffold's CI and a gate that makes UNMARKED_ASSET_LINKS fatal. An agent in a scaffolded app following its own rules pushes green locally and reds CI on the new gate.
|
All six fixed in Five of the six were the same defect in five files: a duplicated enumeration of which function reads each So the server AGENTS.md list is now canonical and the other three point at it rather than repeating it. That list also gains the two server readers every copy had lost ( The The scaffold finding was the one with real user impact, and it was a genuine miss on my part: I added a failing-capable CI step to generated apps without telling those apps' own rules about it, so an agent following them would push green locally and red in CI. Routed through the scaffold-sync skill and fixed in lockstep across the workflow rules, both playbooks, and the PR template. Both templates generated and verified: |
`readDoctorPolicy` validated what was INSIDE the gate object and nothing
about the object itself, so `"gate": "error"`, `"gate": ["CODE"]`, a
misspelled `"gates": {...}`, or a non-object `doctor` all returned an empty
policy with no complaint. The package.json looked gated, CI was not, and
nothing anywhere said so.
That is the one failure mode this mechanism cannot have. An un-gated build
that reports itself gated is worse than no gate at all, because nobody goes
looking. The per-entry check already hard-errored on a typo'd CODE for
exactly that reason; the container shape had simply been left out. The JSON
Schema does describe the shape, but it is wired up through the scaffold's
editor settings only, so it never runs in CI and can never be the
enforcement.
The reader now reports a wrong shape in `malformed` and an unrecognised
sibling of `gate` in `unknownKeys`, and the CLI prints both with the expected
shape and exits 1 without running a check. Tests cover all six spellings of
the mistake.
Also finishes the inventory canonicalization from the previous commit: the
`dev` sub-key split was still stated without `watch` in the type and the
schema, the CLI's own doc named the lockstep trio wrong (schema + type +
reader, with `KNOWN_KEYS` a separate drift test), and the canonical reader
list missed that `lib/doctor.js` re-reads `dev.regenerate` and carries a
hand-maintained port of `readBasePath`, so a change to either key needs two
implementations updated.
Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over ea0dd691 and its blast radius. One real bug in the source, and four more restatements of the inventory the previous commit set out to canonicalize.
packages/cli/lib/doctor.js:163, and this one is a bug, not prose. readDoctorPolicy fails OPEN on a malformed gate container. The guard is if (!declared || typeof declared !== 'object' || Array.isArray(declared)) return empty;, so "gate": "error", "gate": ["UNMARKED_ASSET_LINKS"], a misspelled "gates": {...}, and a non-object doctor all return an empty policy with no unknownCodes and no badSeverities. The package.json looks gated, CI is not, and nothing says so. That is precisely the outcome the per-entry check hard-errors to prevent, and the sentence added to built-ins.md:148 in this very commit asserts it cannot happen. Nothing else catches the shape either: webjs-config.schema.json is wired up only through the scaffold's .vscode/settings.json, so it is editor-only and never runs in CI. There is no test for a non-object gate.
Path-level: packages/core/src/webjs-config.d.ts:90. The WebjsDevTasks docblock still says "before/parallel read by the CLI, regenerate by the server", omitting watch, which is server-read via readDevWatchPathsFromApp and which this commit added to the canonical list as the second server-read dev sub-key. The commit rewrote this file's top docblock to say the inventory is not repeated here, then left a copy of it 60 lines below.
Path-level: packages/server/webjs-config.schema.json:163. The dev property description makes the same incomplete two-way claim, with no watch, while watch's own description names its reader. The commit stripped the enumeration from the file's top-level description and $comment but not from the one property whose split it had just declared people keep getting wrong.
Path-level: packages/cli/AGENTS.md:165. Names the lockstep trio wrong: "JSON Schema + WebjsConfig type + the KNOWN_KEYS drift test". Per the canonical procedure the three surfaces are schema + type + READER, with KNOWN_KEYS called out separately as the additional drift test.
Path-level: packages/server/AGENTS.md:162. The canonical list is itself incomplete: lib/doctor.js reads two more webjs.* keys, dev.regenerate in checkStaticAssetFreshness and basePath in readAppBasePath, the latter a hand-maintained port of readBasePath's normalization by its own admission. A change to basePath semantics needs both implementations updated, and the list whose job is to say which reader to update names one.
|
All five fixed in The gate one was a real bug and the best find on this PR. I validated what was inside the gate object and never the object itself, so four different mistakes all landed in the same silent empty policy. I reproduced each before fixing: The reader now reports a wrong shape in The four inventory findings are fixed as described. Two of them are the previous commit's own canonicalization left half-done: I stripped the enumeration from the schema's top-level |
Broadening the hard-error trigger to cover a wrong shape and a misspelled
sibling of `gate` changed what five doc surfaces promise, and only the CLI's
own AGENTS.md was updated. The skill reference, the docs site, the
`WebjsConfig` type, the JSON Schema, and `webjs help doctor` all still said
the trigger was "an unknown code or severity", so a reader of any of them
believed `"gates": {...}` slipped through. They now describe all four kinds.
The `--json` config-error path was undocumented anywhere. It is the stated
agent-loop contract and it emits a third top-level `configErrors` key with
four `kind` discriminants, so it is now described where the payload is
described, and a test pins the whole set rather than one instance.
Two smaller corrections. The error header hard-coded `webjs.doctor.gate`, but
two of the four kinds report `webjs.doctor` or a misspelled sibling, so it
named a key the package.json need not contain; it names the block instead.
And the claim that each `dev` sub-key "names its own reader below" was only
true of the two server-read ones, so `before` and `parallel` now name
`readAppTasks` as well.
Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over b6bd17ff and its blast radius. Nothing functionally wrong with the shape validation itself, which I confirmed behaves as documented across the shape checks, the sibling-key scan, the empty-doctor: {} case, the bestEffort clamp, and the effective-severity counting. Five problems, all downstream of the commit broadening what counts as a config error.
packages/cli/lib/doctor.js:138-141. The docblock sentence explaining the new validation is broken: an open paren is never closed and a stray comma follows the doctor / gate clause, so it reads "a wrong SHAPE (a non-object doctor or gate, in malformed, and a misspelled sibling ... in unknownKeys". Line 140 also runs well past the file's wrap. This is the canonical explanation of the thing the commit added.
packages/core/src/webjs-config.d.ts:90. The commit added the claim "(each sub-key names its own reader below)", which is true of regenerate and watch and false of before and parallel, whose docblocks name no reader. The commit's whole purpose was accurate per-sub-key attribution, and this is the one new claim it added that the file does not satisfy.
The hard-error trigger broadened from "unknown code or severity" to four kinds, and only packages/cli/AGENTS.md was updated. Five surfaces still state only the narrow trigger, so a reader of any of them believes a misspelled "gates" or a "gate": "error" is not caught: .agents/skills/webjs/references/built-ins.md:228, website/app/docs/configuration/page.ts:61, packages/core/src/webjs-config.d.ts:154, packages/server/webjs-config.schema.json:226, and the webjs help doctor Config notes at packages/cli/bin/webjs.js:157. This is the same drift class the two commits immediately before it were fixing.
packages/cli/bin/webjs.js:725, the --json configErrors array is documented nowhere, and this commit added two more kind values to it. Every surface describes the payload as { results, summary } only: root AGENTS.md:509, the packages/cli/AGENTS.md doctor row, website/app/docs/configuration/page.ts:62, and the help text. --json is the stated agent-loop contract and the tests assert this array, so it is a real contract rather than an accident.
packages/cli/bin/webjs.js:729. The non-JSON error header is hard-coded to invalid "webjs.doctor.gate", but two of the four kinds report webjs.doctor itself or a misspelled sibling, so the header names a key the user's package.json need not contain. Only the detail line is asserted, so the mismatch is unguarded.
|
All five fixed in The five-surface finding is the one that matters. Broadening what counts as a config error silently invalidated a sentence in five places, and I updated the one file I happened to be editing. All five now describe the four kinds. The Two small ones with real consequences. The error header named |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 7e336954 and its blast radius. Clean, nothing to fix.
That is the first round on this branch to find nothing, after four in a row that each did, so I am not treating it as the end. A whole-diff pass follows on this head.
Four places said everything outside the gate map is a warning that cannot fail the build. That is false: `NODE_VERSION` and `TSCONFIG_ERASABLE` return a `fail` status, which defaults to severity `error`, so they red the exit with no gate entry naming them. The behaviour is right, since either would 500 the app at runtime, but the sentence would have an agent diagnose that red CI as impossible. Two of the four ship into every generated app, and one of them is the workflow file an agent reads to learn what the gate does, so the wrong version was the one most likely to be believed. A test pins it now, asserting a hard check fails the exit while the gate names only an unrelated code, and that `off` still silences it, which is the uniform rule the carve-out must not contradict. Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff at head. One finding.
Path-level: the claim that everything outside the gate map is a non-fatal warning is false, and two copies of it ship into every generated app. NODE_VERSION and TSCONFIG_ERASABLE return status: 'fail', which defaults to severity error, so they red the exit with no gate entry naming them. Four places say otherwise: packages/cli/templates/.agents/rules/workflow.md:56-57, packages/cli/templates/.github/workflows/ci.yml:42-43, .github/workflows/ci.yml:46, and packages/cli/lib/create.js:502. The behaviour is right (an app that loses erasableSyntaxOnly should red its conventions job) but workflow.md is the file an agent reads to learn what the gate does, so an agent hitting that failure would conclude it is impossible.
Everything else checked out: the counts fold is byte-identical without a gate, bestEffort sits on exactly the four could-not-check branches, DOCTOR_CODES covers every shipped check so no code is ungatable, the schema/type/KNOWN_KEYS lockstep is complete, both gated apps already wrap their only stylesheet link in asset() and all four set erasableSyntaxOnly so the required step is green on day one, and the bun CI rewrite handles the new step.
|
Fixed in Reproduced it first, since a claim about what can fail the build is worth checking rather than taking on trust: a fixture gating only All four now carve out the two hard toolchain checks and say why they default to Both templates were regenerated and verified: |
The previous commit corrected four statements of when doctor fails and left five more, three of them shipping into generated apps. Both scaffold playbooks and the generated PR template said doctor fails on the gate map alone, so a generated app carried a `workflow.md` naming the two hard checks beside an AGENTS.md playbook implying nothing outside the gate can fail, and the playbook is the surface an agent reads first. The other two are the mirror image, still describing the pre-gate world. `packages/cli/AGENTS.md` said the exit is non-zero "iff a HARD check fails", nine lines above its own paragraph explaining that a gated warn fails it too, and the docs site said the same thing directly under the new gate section. Also adds doctor to the repo's own mandatory code-workflow checklist, which still said to run `webjs check` alone. This PR puts a gated doctor in the required `conventions` job over four in-repo apps, two of them gating `UNMARKED_ASSET_LINKS` to `error`, so an agent following that checklist would push green locally and red in CI. The scaffold got this treatment two commits ago; the framework repo, whose CI actually changed, did not. Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fix-check on 9c68952f. The mechanism is right (NODE_VERSION and TSCONFIG_ERASABLE are the only two status: 'fail' sites, and the new test's summary.fail === 1 holds on Bun too since it reports a Node 24 version). Four things the fix left behind, all the same sentence in different clothes.
Path-level: packages/cli/AGENTS.md:156. Still says the exit is non-zero "iff a HARD check fails ... and warns never fail the exit", contradicted nine lines below by the same file's gate paragraph, and by this repo's own CI, which depends on a gated warn failing.
Path-level: website/app/docs/configuration/page.ts:62. The same stale sentence, on the public docs site, directly under the newly added gate paragraph that says the opposite.
Path-level: three scaffold surfaces the fix skipped, all shipping into generated apps, each stating doctor's failure condition as the gate map alone: packages/cli/templates/partials/agents-playbook-fullstack.md:101-102, packages/cli/templates/partials/agents-playbook-api.md:50-51, and packages/cli/templates/.github/pull_request_template.md:10. So a generated app ships a workflow.md naming the hard checks next to a playbook implying nothing outside the gate can fail, and the playbook is read first.
Path-level: root AGENTS.md:88. The mandatory code-workflow checklist still says "Run webjs check and fix violations". This PR adds a gated doctor to the required conventions job over four in-repo apps, two of which gate UNMARKED_ASSET_LINKS to error, so an agent following the repo's own checklist pushes green locally and reds CI. ea0dd691 fixed exactly this for the scaffold and not for the repo whose CI actually changed.
|
All four fixed in The root The other three are one sentence I kept rewriting in one place at a time. The two playbooks and the generated PR template still described the failure condition as the gate map alone, while Both templates were regenerated and verified: |
Two comments inside the doctor branch still described the pre-gate rule, one of them eight lines above a comment saying the opposite: the exit is not non-zero "iff a HARD check fails" once a gated warn folds into the same count, and "default keeps warnings non-fatal" is only true of an UNGATED warn. The `doctor.js` entry in the CLI module map said the bin owns the exit "on a hard fail" and described the module's exported surface without the two gate functions the bin now composes. The root CLI reference line said the same thing about the exit. Both are files the previous commit edited elsewhere. `--strict` was also described against the old baseline in the help text and the docs site: it now adds every REMAINING warning on top of hard failures and gated errors, rather than being the only way a warning becomes fatal. Refs #1257
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second fix-check, on eca8f674. Everything that commit asserts is factually right, and I verified the load-bearing parts independently: NODE_VERSION and TSCONFIG_ERASABLE are the only two status: 'fail' branches, the conventions job is still a required context under its unchanged name, website and examples/blog are the only gated apps and both already wrap their stylesheet link, and the generated app's skill copy comes from the repo-root file this PR updated. The scaffold surfaces now agree with each other.
Four places still state the old exit condition.
packages/cli/bin/webjs.js:700 and :762. Two comments inside the doctor branch. The first says the exit is non-zero "iff a HARD check FAILS", which the gate makes false, eight lines above the comment introducing the gate. The second, Default keeps warnings non-fatal., sits directly on the failing line and is contradicted by the counts comment above it, since a gated warn resolves to error and lands in fail.
packages/cli/AGENTS.md:47. The doctor.js module-map entry still says the bin owns the exit "on a hard fail", and describes the module's exports as runDoctorChecks plus the two test seams, with no readDoctorPolicy / applyDoctorPolicy / DOCTOR_SEVERITIES. The commit corrected the command row 110 lines below in this same file.
Root AGENTS.md:509. The CLI reference line still reads "non-zero exit on a hard fail". The gate appears later on the same line as a separate clause, so the sentence that actually states the exit condition is the pre-gate one. The commit amended item 4 of this file but not this line.
packages/cli/bin/webjs.js:152 and website/app/docs/configuration/page.ts:49. --strict is described as failing "on warnings, not just hard failures", which was the whole story before the gate and is not now. The docs-site one sits three paragraphs above the corrected text this commit wrote.
|
All four fixed in This is the second fix-check in a row to find the same class, so I am stopping the review cycle here rather than buying another round, and I am NOT marking the PR ready. These last fixes are on the branch unreviewed. My read on why it kept going: this was never a repair breaking something new. It is one sentence, "what makes doctor exit non-zero", that existed in more places than I enumerated, and I kept correcting the copies a reviewer named instead of the set. Round 4 was where I should have generalised: when the third stale copy of the reader inventory turned up I made one canonical and pointed the rest at it, and that class has not come back since. I did not do the same for the exit-condition sentence, so it kept surfacing one or two copies at a time. What that means for confidence in the change itself: the mechanism has now been read by nine independent reviewers and the last finding against its BEHAVIOUR was the fail-open gate in round 4, fixed with tests covering every spelling. Everything after that has been prose describing it. The remaining risk is that some surface I have not grepped still describes the pre-gate exit rule, which is a docs accuracy problem rather than a correctness one. |
Four review rounds each named one or two copies of the same sentence, so this time I swept for the pattern instead of fixing what was pointed at. Four more descriptions of `--strict` were stating its delta against the pre-gate baseline: the root and CLI AGENTS.md doctor entries, the usage banner, and a comment in the doctor branch. Post-gate, `--strict` is not what makes a warning fatal, it is what makes EVERY remaining warning fatal on top of hard failures and gated errors. All four say that now, and a grep for both shapes of the old claim comes back empty across the repo. Refs #1257
1 participant
Closes #1257
Summary
webjs doctorruns in no CI, in this repo or in a scaffolded app, so every advisory it carries is findable but not unmissable. TheUNMARKED_ASSET_LINKSadvisory exists precisely because an unhashed stylesheet link shipped a visible deploy-staleness regression on webjs.dev, yet nothing runs doctor, so that advisory could not have caught the original regression.The blocker was that doctor's exit is all-or-nothing. The default fails only on a broken toolchain, and
--strictmakes every warning fatal, including four checks that are environment-shaped and would red a clean CI run:GIT_HOOKwants a local pre-commit hook a runner has no reason to have,ENV_DRIFTcompares against a.envCI does not carry,VENDOR_PINfetches the network, andFRAMEWORK_RESOLVEdepends on the environment.This makes per-check severity CONFIG rather than a flag. An app declares it once in
package.json, so its CI workflow, itsnpm run doctor, and an agent's--jsonloop all read one policy that travels with the repo.{ "webjs": { "doctor": { "gate": { "UNMARKED_ASSET_LINKS": "error", // fail the exit on this one "ELISION_CARRIERS": "off" // silence it, even under --strict } } } }Three levels, the same scale ESLint uses and
eslint-plugin-nextships as a rule-id-keyed severity map. A--gate=CODE,CODEflag was rejected because policy would then live in workflow YAML, so a local run and CI disagree about what is fatal and the code list is duplicated per caller.Why it is safe in a required job
errorfor a hard fail,warnotherwise), so an app with nowebjs.doctorblock produces byte-identical counts, andfailing = fail > 0 || (strict && warn > 0)is textually unchanged.severityis the EFFECTIVE level, so a PASSING check reportspasseven when its code is gatederror. That keepsresults.some((r) => r.severity === 'error')honest with no false positive.bestEffortresult (the four could-not-check branches: two in vendor-pin, two in importmap-coherence) is CAPPED atwarnand can never be escalated. Doctor's live jspm resolve therefore cannot red the requiredconventionsjob, which matters because test: keep a live jspm outage from redding the required CI job #1150 is open on exactly that flake class.--jsonshape is additive only (severityper result,offin the summary). The summary key staysfail, noterror, so an existing consumer keeps working.Usage: webjs doctor [--json] [--strict]is unchanged.Measured starting state
Doctor over all four in-repo apps before this change, which is what the gate choice was made against:
examples/blogENV_DRIFT,WEBJS_VERSIONS,ELISION_CARRIERS(3 modules)websiteENV_DRIFT,WEBJS_VERSIONS,ELISION_CARRIERS(2 modules)docsWEBJS_VERSIONSpackages/ui/packages/websiteWEBJS_VERSIONS,ENV_DRIFTUNMARKED_ASSET_LINKSpasses on all four, so gating iterroris green on day one.ELISION_CARRIERS,WEBJS_VERSIONS, andENV_DRIFTwarn here, so gating any of those would red main on merge.Test plan
node --test test/cli/doctor.test.mjs(91/91 on Node AND underbun test): pure policy, CLI integration, and the counterfactual pair where the same fixture exits 0 ungated and 1 gatednode --test test/cli/help.test.mjs(14/14)node --test packages/server/test/config/webjs-config-schema.test.js(theKNOWN_KEYSdrift assertions cover the new key in both directions)node --test test/types/type-fixtures.test.mjs(adoctor.gateliteral type-checks; a bad severity and an unknowndoctorsub-key do not)node --test test/scaffolds/scaffold-template-validation.test.js(both templates emit the gate and anpm run doctorCI step)npm test: 3914 pass, 0 failnode scripts/run-bun-tests.js): 284 pass, 0 genuine failures, 28 documented node-only skipsWEBJS_E2E=1 node --test test/e2e/e2e.test.mjs: 91 pass, 0 fail98116bc5: neutering only the gate lookup inapplyDoctorPolicyreds 5 tests (headline0 !== 1on the gated-warn exit) and leaves the other 86 greenwebsite/app/layout.ts's stylesheet link flipswebjs doctorto exit 1 namingUNMARKED_ASSET_LINKS; restoredwebjs create, thennpm run doctorandwebjs checkboth exit 0, then unmarking its layout link flips doctor to exit 1websiteserves 200 on/,/docs/configuration,/ui,/ui/buttonin prod mode with zero broken modulepreload hintsBrowser, Bun-parity, and MCP surfaces: N/A. The change adds a
package.jsonread and an exit-code computation in the CLI, with no serializer, listener, SSR / action / CSRF dispatch, stream,node:crypto, TS-stripper, or auth / session / cors involvement, so notest/bun/**cross-runtime assertion applies (the Bun matrix was still run, and it caught a brittle assertion of mine). The MCP exposes no doctor tool, and the resources it does serve areAGENTS.mdplus the skill references, both updated here and bundled at prepack.Review
Nine rounds by fresh one-shot reviewers: a whole-diff pass, five delta passes, a clean round, the final whole-diff pass, and two fix-checks. One behavioural defect was found and fixed (the gate failed OPEN on a malformed container, so
"gate": "error"or a misspelled"gates"left CI un-gated while the package.json looked gated). Everything after that was prose accuracy, mostly one sentence about what makes doctor exit non-zero appearing in more surfaces than any single round enumerated; the last commit closes that class by grep rather than by instance.The cycle stopped at its two-consecutive-fix-check limit. The final two commits are doc-wording corrections that no fresh reviewer has read.
Docs
packages/cli/AGENTS.md(the doctor row plus a severity-model paragraph),packages/cli/README.md, rootAGENTS.md,.agents/skills/webjs/references/built-ins.md(a new doctor-gate section plus theasset()paragraph),website/app/docs/configuration/page.ts, and the CLI usage banner pluswebjs help doctor. Browser / e2e / Bun parity are N/A: the change adds apackage.jsonread and an exit-code computation in the CLI, with no runtime-sensitive surface involved.