Skip to content

webjs check at the monorepo root reports 61 false violations #1301

Description

@vivek7405

Problem

Running webjs check from the monorepo ROOT reports 61 violations, and effectively all of them are false positives. That matters more than the count, because AGENTS.md tells every agent to run webjs check and fix violations as step 4 of the mandatory code workflow. An agent following that from the repo root today is handed 61 findings, none of which are real, with no signal telling it so. The likely outcomes are a wasted investigation or, worse, renames in test fixtures to satisfy a checker that should never have looked at them.

Breakdown as of 5ac991ce:

Rule Count
no-duplicate-tag 47
no-static-properties 7
no-interpolation-in-raw-text-element 6
no-browser-globals-in-render 1

The mechanism is that the monorepo root is not an app, and webjs check is an app-level tool. Every rule assumes one application: one module graph, one custom-element registry, one runtime. The root is a workspace holding four separate apps plus every package's test suite plus editor fixtures, none of which ever coexist in a single runtime.

no-duplicate-tag shows it most clearly. The tag my-counter is reported as duplicated across five files:

  • examples/blog/components/counter.ts (a real app component)
  • packages/editors/intellisense/test/plugin/intellisense.test.mjs (an editor-plugin fixture)
  • packages/server/test/instrumentation/instrumentation.test.js (a unit test)
  • packages/server/test/scanner/component-scanner.test.js (a unit test)
  • test/types/component-types.test-d.ts (a type fixture)

Those five never load together. The rule's stated hazard, that SSR keeps the last registration while the browser keeps the first, cannot occur, because no single browser or SSR pass sees more than one of them. The rule is correct; the scope it was pointed at is not.

CI is unaffected and green, which is why this has gone unnoticed. .github/workflows/ci.yml:35 runs the check PER APP, looping over examples/blog, website, docs, and packages/ui/packages/website, each from its own directory. That is the correct invocation and it reports clean. Only the root-level invocation is misleading, and the root-level invocation is exactly the one the workflow instruction implies.

Design / approach

The goal is that an agent following the workflow from the repo root gets a truthful answer. Three directions, in increasing cost:

  1. Refuse to run at a non-app root, naming the right invocation. Detect that the current directory has no app/ and is a workspace root (a package.json carrying workspaces), then exit with guidance, or add a --workspaces mode looping the way CI already does. Treats the root invocation as a usage error rather than emitting 61 findings nobody should act on. Cheapest, and it fixes the misleading instruction directly.
  2. Exclude test and fixture files from the app-level rules. scanBareImports in packages/server/src/vendor.js already skips test/ and tests/ for exactly this class of reason. The checker's own walker (packages/server/src/check.js, around L1038-L1046) skips only node_modules, dist, build, .git, .next, _private. Adding test directories removes most of the 47 duplicate-tag findings even at the root, and is arguably right for a real app too, since a component registered only inside a test file is not part of the shipped registry. Care needed, because some projects legitimately keep components beside tests.
  3. Both, with the first as the primary fix.

Recommendation is the first, since it addresses the actual harm with the least behavioural risk to real apps. The second is a separate judgement about rule scope and deserves its own decision.

Whichever is chosen, the workflow instruction should say WHERE to run the check in this repo, because that ambiguity is the root cause.

Implementation notes (for the implementing agent)

Where to look:

  • packages/server/src/check.js is the rule engine. Its file walker sits around L1038-L1046 and currently skips node_modules, dist, build, .git, .next, _private. Rule descriptions are near L116, and webjs check --rules lists them all.
  • packages/cli/bin/webjs.js dispatches the check subcommand.
  • .github/workflows/ci.yml:35 is the per-app loop CI uses, and is the reference for a correct multi-app invocation. A --workspaces mode should match it so the two cannot drift.
  • packages/server/src/vendor.js scanBareImports (around L100) is the precedent for skipping test/ and tests/, with its rationale.
  • AGENTS.md, Code workflow (mandatory) item 4, is the instruction needing the WHERE.

Reproduce first:

cd <repo root> && npx webjs check      # 61 violations
cd examples/blog && npx webjs check    # clean

Landmines:

  • Do not fix the reported files. Renaming tags in test fixtures to satisfy a root-level run would churn dozens of files and fix nothing real. Several of those tags are asserted on BY NAME in the very tests that register them, so a rename breaks them.
  • The four in-repo apps must keep checking clean. Re-run the CI loop above by hand.
  • no-duplicate-tag is genuine and valuable inside one app. Do not weaken it there. If test directories are excluded, make that a stated scope decision, not a silent loosening.
  • packages/editors/nvim/vendor/** holds a generated verbatim copy of the intellisense source, so it can legitimately contain a second registration of the same tag. A drift test enforces that copy, so it must never be hand-edited.

Tests and docs surfaces:

  • Unit: packages/server/test/check/** for rule-engine behaviour, plus a counterfactual proving the new guidance or exclusion actually fires.
  • test/cli/** if the CLI gains a flag or a refusal path.
  • Docs: AGENTS.md item 4, framework-dev.md if a workspace-level invocation is introduced, and the docs site page covering the check command if the CLI surface changes.

Invariants:

  • The checker is correctness-only and runs unconditionally with no per-project disabling (AGENTS.md, Conventions section). A fix must not add a per-project opt-out for a real rule. Scoping WHICH FILES constitute an app is a different thing from letting an app disable a rule.

Acceptance criteria

  • Running the check from the monorepo root either reports clean or explains that the root is not an app and names the correct invocation
  • The four in-repo apps still check clean via the CI per-app loop
  • no-duplicate-tag still fires for a genuine duplicate WITHIN one app (counterfactual: register the same tag twice in examples/blog and confirm it reds)
  • AGENTS.md item 4 says where to run the check in this repo
  • No test fixture was renamed to satisfy the checker
  • Tests cover the new behaviour at the layer it touches

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions