fix: webjs check refuses to run outside an app - #1356
Conversation
|
Design rationale: why not exclude test directories, and why no Two alternatives look more obvious than the refusal, and both were measured before being dropped. Excluding It would also break a documented lockstep. The rule's comment says it scans every source file to stay in step with the editor's 9004 diagnostic, which runs over the whole TypeScript program and is not gated on the import graph. A normal tsconfig includes test files, so the editor would underline a tag the CLI called clean. And a duplicate tag inside a test file is a real hazard rather than a false positive: browser tests load in one realm alongside the components under test, and a second A Drift is handled by a test rather than a flag. One deviation from the plan, deliberate. The predicate uses |
|
Fallout: one existing fixture was not app-shaped The refusal turned up a fixture that had been relying on the old behaviour. Worth being precise about which side was wrong, because the easy read is that the guard is too strict. It is not. That fixture's own clean-app sibling twenty lines above it already writes That is the only fixture affected. Two unrelated failures in the same run, pre-existing on main. |
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff. The guard itself is right and I found nothing functionally wrong with it: the predicate matches what check.js already applies per-rule, it sits after the --rules branch so documentation still works anywhere, and the rule engine is genuinely untouched, which is what keeps this on the "where the command runs" side of the correctness-only line.
Two problems, both in the docs, and both the same underlying mistake: the change is more visible to an AGENT than the doc edits admit. One is a misplaced insertion that silently reassigned another module's tests. The other is that --json now has a second output shape and no doc surface says so, which matters more than it looks, because --json exists specifically so an agent can consume the result without scraping stdout. The thing I would keep an eye on generally is that pattern: a refusal added to a machine-readable command is a contract change, not just a message.
Running `webjs check` at the monorepo root reported 67 violations, and effectively all of them were false. That mattered because AGENTS.md tells every agent to run the check and fix what it finds, so an agent starting from the repo root was handed 67 findings with no signal that the scope was wrong, and the likely outcomes were a wasted investigation or renames in test fixtures to satisfy a checker that should never have looked at them. Every rule assumes ONE application: one module graph, one custom-element registry, one runtime. The repo root is none of those. The tag `my-counter` was reported as duplicated across a blog component, an editor-plugin fixture, two unit tests, and a type fixture, five files that never load together, so the hazard the rule states cannot occur. The rule is right; the scope it was pointed at was not. So the command now refuses in any directory with no `app/` directory, exits 1, and names the workspace member apps to run it in. The rule engine is untouched, so the rule set inside an app is byte-identical before and after; this scopes only where the command may be invoked. Next.js refuses on the same predicate in find-pages-dir.ts. Closes #1301
`webjs check` now refuses outside an app (#1301), and this fixture wrote only `components/broken.ts`, so the run refused instead of reporting the violation it exists to assert. Its clean-app sibling in the same file already writes `app/page.ts`; this makes the pair consistent. The assertion is unchanged, so it still fails if the projector stops reporting the violation.
Two review findings, both on doc surfaces. The check-target.js entry landed in the MIDDLE of the app-name.js entry, between its prose and its trailing test listing, so app-name.js lost its test references and check-target.js claimed three test files that have nothing to do with it. The module map exists to tell an agent where a module's tests live, so both halves were wrong. Moved it below. The refusal also gives `--json` a second output shape, and no doc surface said so. The one place describing that contract still promised only the violations array plus a summary count, which is what an agent reads before writing a consumer.
0aa2db6 to
63c63f0
Compare
Closes #1301
Summary
webjs checknow refuses to run in a directory that has noapp/, exits 1, and names the workspace member apps to run it in. Running it at the monorepo root reported 67 violations, effectively all of them false, which mattered because AGENTS.md tells every agent to run the check and fix what it finds. An agent starting from the repo root was handed 67 findings with no signal that the scope was wrong.Every rule assumes ONE application: one module graph, one custom-element registry, one runtime. The repo root is none of those, it is a workspace holding two apps plus every package's test suite plus editor fixtures plus the scaffold templates. The tag
my-counterwas reported as duplicated across a blog component, an editor-plugin fixture, two unit tests, and a type fixture, five files that never load together, so the hazard the rule states cannot occur. The rule is right; the scope it was pointed at was not.What changed
packages/cli/lib/check-target.js(new). Pure guard plus message renderers, the same shape as its siblingslib/node-preflight.jsandlib/app-name.js.findCheckTarget(cwd)returns{ isApp, workspaceApps }; the predicate is the presence of anapp/DIRECTORY and nothing else. Aworkspaceskey is not part of the predicate (a directory with noapp/is not an app either way), it only enriches the message with the members that ARE apps.packages/cli/bin/webjs.js. The guard sits after the--rulesbranch (pure documentation, must keep working anywhere) and beforecheckConventionsruns. Exit 1, because an agent gates on the exit status and 0 would read as "clean", the exact false signal this fixes. Under--jsonthe refusal is emitted as JSON carrying noviolationskey on purpose, so a consumer that ignores the exit code and readsreport.violations.lengththrows rather than being told the workspace is clean.packages/server/src/check.jsis untouched. The rule set inside an app is byte-identical before and after; this scopes only where the command may be invoked, so no rule-engine test changes.Next.js refuses on the identical predicate in
packages/next/src/lib/find-pages-dir.ts.Verification
npx webjs checkat the root: exit 1, refusal naming( cd examples/blog && npx webjs check )and( cd website && npx webjs check ), zero violation lines.--jsonat the root: exit 1,error.code === 'NOT_AN_APP', noviolationskey.--rulesat the root: exit 0, rules listed.webjs check: all checks pass ✓.webjs doctorat the root: unchanged, still exit 0.Test plan
test/cli/check-target.test.mjs, 13 cases: unit coverage of the predicate (app dir, workspace filtering, yarn's{ packages: [...] }form, a file namedapp, a missing or malformed manifest) and spawned-CLI coverage of the real bin.Two that carry the weight:
no-duplicate-tagviolation, proving the guard does not swallow real findings. Reverting the bin wiring at5227e14ereds three cases including the repo-root one; verified by toggling the file and re-running.for app in ...; doloop in thewebjs checkstep of.github/workflows/ci.ymland asserts set equality with the list the refusal derives. Both sides are['examples/blog', 'website']. This is what a--workspacesflag was rejected in favour of.Layers that do not apply: browser, e2e, and smoke (this is a CLI argv branch, no DOM, no request path; the spawned-CLI cases already exercise the real binary end to end). Bun parity is not required, judged by running both changed paths through the two greps in
.claude/hooks/require-bun-parity-with-runtime-src.sh:lib/check-target.jsmatches the prefix but none of the runtime-sensitive filenames, andbin/webjs.jsis not under a covered prefix at all. The surface isnode:fs,node:path, and argv, so there is nothing for a cross-runtime assertion to prove.Docs
AGENTS.md"Code workflow" item 4: run the check from inside an app, with the two commands.packages/cli/AGENTS.md: thewebjs checkrow of the commands table, pluscheck-target.jsin thelib/module map.framework-dev.md: a section recording the per-app invocation, the two apps, and the drift test.website/app/docs/conventions/page.ts: one paragraph after the invocation block, for the monorepo user outside this repo.Not changed, deliberately: the
HELP.checkentry and the AGENTS.md CLI reference block (no flag added, sowebjs help checkis still accurate),packages/server/AGENTS.md(check.jsuntouched), and the scaffold templates (every scaffolded app hasapp/at its root and its CI runs the check from there, so no scaffolded app can hit the refusal).Results
test/scaffolds/gallery-coverage.test.jsreding on a staleloadFramegallery-manifest entry left whenloadFramestopped being a@webjsdev/coreexport in fix: loadFrame is undefined from @webjsdev/core/client-router in prod #1346. I reproduced both gallery failures in a clean checkout at79fc28fcwith none of this branch applied. They are in a file this PR does not touch, so I have left them alone; main is currently red on that job independently of this change./,/docs/conventions,/ui,/ui/buttonall 200, 81 modulepreload hints probed, none broken.webjs doctorat the root still exits 0.One existing fixture needed shaping.
packages/mcp/test/check-report.test.mjsspawns the real bin against a temp directory that wrote onlycomponents/broken.ts, so the run refused and the assertion blew up onundefined.length. Its clean-app sibling in the same file already writesapp/page.ts; giving the violation fixture one makes the pair consistent, and the assertion is unchanged so it still fails if the projector stops reporting the violation. That file is the only place in the repo that spawnswebjs checkthrough the bin, and nothing in.github/workflows,scripts/,.hooks/, or the rootpackage.jsoninvokes the check at a workspace root.