Skip to content

ci(lint): enforce app→shared→shared module-boundary DAG (SF#1)#50

Merged
spencermarx merged 2 commits into
mainfrom
hotfix/enforce-module-boundaries
Jun 15, 2026
Merged

ci(lint): enforce app→shared→shared module-boundary DAG (SF#1)#50
spencermarx merged 2 commits into
mainfrom
hotfix/enforce-module-boundaries

Conversation

@spencermarx

Copy link
Copy Markdown
Owner

Enforce the app → shared → shared module-boundary DAG in CI

Implements SF#1 — the highest-leverage should-fix from the PR #49 code review — as its own small, focused change.

The review noted: the predecessor PR spent ~3,000 lines removing an inverted dashboard → cli dependency, yet the app → shared → shared rule was policed only by CLAUDE.md + human review. A single accidental import "@open-code-review/cli" in the dashboard would silently reinstate the cycle and CI would not catch it. This converts that invariant into a CI gate.

What this does

  • eslint.config.mjs (root, flat) — registers only @nx/enforce-module-boundaries, keyed off the scope:* tags every project.json already carries:
    • scope:shared → shared only
    • scope:cli → cli + shared + agents
    • scope:dashboard → dashboard + shared
    • scope:agents → agents
  • Deliberately a DAG gate, not a style lint — no typescript-eslint/react rule suites are enabled, so it does not flag the thousands of pre-existing style issues. Those plugins are registered with rules off purely so the codebase's existing intentional eslint-disable directives resolve (turning their suites on is a separate, intentional change).
  • checkDynamicDependenciesExceptions exempts @open-code-review/* from the "static import of a lazy-loaded library" facet — the CLI intentionally lazy-loads persistence on hot paths to defer node:sqlite; lazy-load consistency is a separate concern from the DAG.
  • nx.json registers @nx/eslint/plugin so the pre-existing lint scripts (nx run-many -t lint, nx lint cli) finally light up; ci.yml adds a Lint (module boundaries) job.

Verification

  • nx run-many -t lint green across all 8 projects on the clean tree.
  • ✅ Positive control: a planted dashboard → cli import fails with Imports of apps are forbidden @nx/enforce-module-boundaries.
  • nx run-many -t typecheck unaffected.
  • No source files changed — config + tooling only.

Scope intentionally minimal: enabling the broader React/TS rule suites (and fixing the lazy-load consistency the rule also surfaced) are follow-ups, called out in the config comments.

spencermarx and others added 2 commits June 15, 2026 15:10
Converts the CLAUDE.md "apps never depend on apps; shared depends only on
shared" invariant from a human-review rule into a CI gate — the highest-leverage
should-fix from the PR #49 code review (SF#1). A single accidental
`dashboard -> cli` (or `shared -> app`) import now fails the build, not just
review; the prior PR spent ~3,000 lines removing exactly that inverted edge.

- eslint.config.mjs (root, flat): registers ONLY @nx/enforce-module-boundaries,
  keyed off the scope:* tags every project.json already carries
  (scope:shared -> shared only; scope:cli -> cli+shared+agents; scope:dashboard
  -> dashboard+shared; scope:agents -> agents). No typescript-eslint/react
  rule SUITES enabled — this stays a dependency-graph gate, not a style lint.
- @typescript-eslint and react-hooks plugins are registered (rules OFF) only so
  the codebase's existing intentional inline disable directives resolve;
  reportUnusedDisableDirectives is off so those inert suppressions are not
  flagged. Turning those rule suites on is a separate, intentional change.
- checkDynamicDependenciesExceptions exempts @open-code-review/* from the
  "static import of a lazy-loaded library" facet: the CLI intentionally
  lazy-loads persistence on hot paths (deferring node:sqlite). Lazy-load
  consistency is a separate concern from the DAG this gate owns.
- nx.json registers @nx/eslint/plugin so the pre-existing `lint` scripts
  (`nx run-many -t lint`, `nx lint cli`) light up; ci.yml adds a Lint job.
- Verified: lint is green across all 8 projects on the clean tree, and a planted
  `dashboard -> cli` import fails with "Imports of apps are forbidden".

No source files changed — config + tooling only.

Co-Authored-By: claude-flow <ruv@ruv.net>
Genuinely worked the approved review's should-fixes + cheap suggestions.

Should Fix:
- SF#1 (durable canary): add a fitness-function self-test
  (packages/shared/platform/src/__tests__/module-boundary-gate.test.ts) that runs
  REAL eslint over a planted dashboard->cli import and asserts it fails, plus a
  legal dashboard->platform control that passes. Cross-platform (node + eslint's
  JS entry, not .bin) so it runs in the Windows unit leg too. Closes the "gate can
  silently disarm" gap the rule exists to prevent.
- SF#2: drop the unused jsonc-eslint-parser devDep (transitive of
  @nx/eslint-plugin anyway); refresh lockfile.
- SF#4: normalize @nx/eslint + @nx/eslint-plugin pins ~22.4.0 -> ^22.0.0 to match
  the other @nx/* deps (no skew on the next nx migrate).
- SF#3: comment that scope:cli covers cli-e2e and scope:dashboard covers
  dashboard-{api,ui}-e2e, with a note to add scope:e2e if they ever diverge.

Suggestions folded in:
- Architect F3: removed the blanket `packages/agents/**` ignore so the one real TS
  file there (release/version-actions.ts) IS boundary-checked; agents now lints
  (9 projects). Content assets are excluded by the `.ts` files glob, not an ignore.
- Architect F1: comment that app->app is blocked by BOTH the allow-lists AND the
  projectType:"application" default; the canary re-validates the combination.
- Architect F2: header note that the gate is single-axis (scope:* only) by design,
  leaving type:* for future rules.
- Fowler F2: note that no-control-regex is a core rule (needs no plugin).
- Quality Q6/Q7: ASCII alias for the app->shared->shared arrow; comment the
  default allow:[]/enforceBuildableLibDependency:false options.

Deferred (review agreed / explicitly future): the lint-cache `^default` input
tightening (Principal F5/F6 — defer until bake-in; tightening risks under-
invalidation since the rule reads dep tags), and React/TS rule SUITES.

Verified: lint + typecheck green across all 9 projects; canary passes (violation
fails, legal passes).

Note for the repo admin: add "Lint (module boundaries)" to main's required status
checks (Quality Q5) — a repo setting, not code.

Co-Authored-By: claude-flow <ruv@ruv.net>
@spencermarx

Copy link
Copy Markdown
Owner Author

Addressed round-1 review (APPROVE) — 1199d81

Worked the should-fixes + cheap suggestions genuinely; corroborated each first.

Should Fix

  • SF#1 (durable canary) — added a fitness-function self-test, packages/shared/platform/src/__tests__/module-boundary-gate.test.ts, that runs real ESLint over a planted dashboard → cli import and asserts it fails, plus a legal dashboard → platform control that passes. Cross-platform (node + eslint's JS entry, not .bin) so it also runs in the Windows unit leg. This closes the exact "gate silently disarms" failure mode the rule exists to prevent (option rename, error→warn, widened allow-list, projectType change all now caught). ~1.7s.
  • SF#2 — dropped the unused jsonc-eslint-parser devDep (already transitive of @nx/eslint-plugin); lockfile refreshed.
  • SF#4 — normalized @nx/eslint + @nx/eslint-plugin pins ~22.4.0 → ^22.0.0 to match the other @nx/* deps (no skew on the next nx migrate).
  • SF#3 — commented that scope:cli covers cli-e2e and scope:dashboard covers dashboard-{api,ui}-e2e, with a note to add scope:e2e if they ever need distinct rules.

Suggestions folded in

  • Architect F3 — removed the blanket packages/agents/** ignore so the one real TS file there (release/version-actions.ts) is boundary-checked; agents now lints (9 projects). Asset markdown/JSON are excluded by the .ts files glob, not an ignore — so a future workspace import from that file can't slip the gate.
  • Architect F1 — documented that app → app is blocked by both the allow-lists and the projectType:"application" default (the canary re-validates the combination).
  • Architect F2 — header note that the gate is single-axis (scope:*) by design, leaving type:* for future rules.
  • Fowler F2 — noted no-control-regex is a core rule (needs no plugin registration).
  • Quality Q6/Q7 — ASCII alias for the app->shared->shared arrow; commented the default allow:[] / enforceBuildableLibDependency:false options.

Deferred (with reason)

  • Principal F5/F6 (lint-cache ^default input tightening) — the review explicitly defers this until bake-in, and tightening risks under-invalidation since the boundary rule reads dependency tags. Left as a future targetDefaults.lint tweak.
  • React/TS rule suites — out of scope (separate intentional change), as in the original config comments.

One repo-setting follow-up (not code)

Quality Q5: please add Lint (module boundaries) to main's required status checks so a green build can't merge a boundary regression. I can't change branch protection from here.

Verified: nx run-many -t lint typecheck green across all 9 projects; canary passes.

@spencermarx spencermarx merged commit 276d97c into main Jun 15, 2026
6 checks passed
@spencermarx spencermarx deleted the hotfix/enforce-module-boundaries branch June 17, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant