chore(tooling): adopt the anti-slop oxlint rules - #164
Conversation
Vendor the anti-slop oxlint JS plugin into tools/lint/anti-slop and register every generic rule in the root oxlint config. Rules the codebase already satisfies are errors; the rest warn until the pull request that clears each one flips it to error. Bump oxlint to 1.78.0 with a matching @oxlint/plugins, and run the vendored rule tests in test:tools.
jakubfilinger-b
left a comment
jakubfilinger-b
left a comment
There was a problem hiding this comment.
I'm in favour of the premise. "Review catches some of it, but not consistently" is the honest version of where we are, and type-evidence rules are the right subset to mechanise — as with no stated reason and unknown parameters are exactly the patterns that read as fine in a diff and cost you later.
The execution is careful in the ways that matter: the licence is vendored alongside the code, the upstream commit and every local change are recorded, the nested eslint-stylistic vendor carries its own LICENSE and UPSTREAM.md, the rule tests actually run in test:tools, and the published consumer config is untouched. I checked that last one — packages/core/oxlint/oxlintrc.json is not in the diff, so a consumer sees nothing. And since check:lint is a bare oxlint . with no --deny-warnings, none of the 7k warnings can fail anyone's build. Good.
Not blocking anything. Three thoughts, one of which I'd act on before merge (the require-readable-spacing sequencing) and one of which is bigger than this PR (the public-API follow-ups).
| "anti-slop/no-unknown-type-aliases": "error", | ||
| "anti-slop/no-unsafe-dictionary-type": "warn", | ||
| "anti-slop/no-widen-then-assert": "error", | ||
| "anti-slop/require-readable-spacing": "warn", |
There was a problem hiding this comment.
This rule alone is 6.3k of the 7k warnings, and the plan is to autofix it in a separate PR "at a quiet moment". That leaves an interim state where pnpm check:lint prints seven thousand lines, and I don't think anyone reads the seven-thousand-and-first — including the warnings from the rules that were already there and were previously actionable.
It also sits badly with the policy this PR writes into enforcement.md: "a warn is known debt, not a suggestion to ignore". 6.3k sites of a purely stylistic rule will be ignored, by construction, and the sentence loses force for the rules where it matters.
The sequencing that gets the same end state without the blind window is to register this one as off here, land the autofix PR, then flip it straight to warn (or error, since after an autofix the codebase is clean of it, which is your own bar for error). Nothing else in the PR has to change, and the conflict-with-open-branches argument for deferring the fix is unaffected.
Separate question while you're here: blank lines between statements are normally the formatter's business, and this repo already runs oxfmt. Is there a reason this is a lint rule rather than an .oxfmtrc.json setting? If both end up with an opinion about the same whitespace, fix:lint and fix:format can disagree.
| "anti-slop/no-conditional-empty-object-spread": "warn", | ||
| "anti-slop/no-known-value-widening": "warn", | ||
| "anti-slop/no-module-mocking": "warn", | ||
| "anti-slop/no-object-parameters": "warn", |
There was a problem hiding this comment.
This is the one I'd want a second opinion on before the follow-ups start, and it applies to no-unknown-parameters at :44 too. The Risks section says clearing these will change "parts of the public API (EventHandler, AdminGuard.assert)".
Those are published surfaces that consumers implement against, so a change there is a breaking release for them regardless of how good the reason is. I'm not against the rules — I'm against the ordering of the argument. "The linter flags it" should not be what carries a public signature change; the change should stand on its own (this is the better shape for consumers, here is what breaks, here is the migration), and the rule flip should follow it rather than motivate it.
Concretely: when those follow-ups land, could they be framed as API PRs with their own changesets and a stated migration, with the warn -> error flip as a one-line afterthought? If a rule turns out not to be worth a breaking change, the honest outcome is that the rule stays warn on the public surface, not that the surface moves.
| "tools/templates", | ||
| "tools/lint/anti-slop/**", | ||
| ".agents/**", | ||
| ".claude/**", |
There was a problem hiding this comment.
These ignore entries aren't mentioned anywhere in the description, which took me a detour to work out. They're the generated agent mirrors — nothing under them is committed on dev, so this is housekeeping to stop oxlint walking generated files in a working tree, not a suppression. Worth a clause in the body saying so.
One forward-looking note: .github/hooks/** is on the list. Nothing lives there today, but hooks are executable scripts, and if one ever lands it will be unlinted with no visible reason why. If the intent is "generated agent instructions", it might be safer to scope these to the generated file patterns rather than to whole directories that could later hold real code.
| - Two-layer boundaries - per-edit oxlint (`oss-boundaries/*`) plus the whole-graph dependency-cruiser (`pnpm check:boundaries`, catches transitive edges, barrel laundering, dynamic `import()`). Don't work around a violation; fix the import. | ||
| - Module structure + naming are lint-enforced (`oss-module-shape/*` oxlint JS plugin, `tools/lint/oxlint-module-shape-plugin.mjs`): files sit in a canonical layer dir, `service/` files end `.service.ts`, `__tests__/` files end `.test.ts`, an infra-backed test ends `.int.test.ts`, filenames kebab-case, no inline `pgEnum` value arrays. | ||
| - oxlint config is split: the published `@openora/core/oxlint/oxlintrc.json` holds the universal, stack-agnostic rules (base rules, `typescript/no-explicit-any`, `typescript/no-non-null-assertion`, `typescript/consistent-type-definitions`, `import/no-cycle`, `import/no-duplicates`) - the single source of truth a consumer extends via `"extends": ["./node_modules/@openora/core/oxlint/oxlintrc.json"]`. The root `.oxlintrc.json` here `extends` that shared config and adds only OSS-internal rules (`oss-boundaries/*`, `oss-module-shape/*`, `unicorn/filename-case`) that need the local `jsPlugins`. | ||
| - Low-evidence TypeScript patterns are lint-enforced by the vendored `anti-slop` oxlint JS plugin (`tools/lint/anti-slop/`, upstream commit and local changes in its README). Every generic rule is registered in the root `.oxlintrc.json`. A rule is `error` once the codebase is clean of it and `warn` until the pull request that fixes its remaining sites flips it, so a `warn` is known debt, not a suggestion to ignore. Its rule tests run in `pnpm test:tools`. |
There was a problem hiding this comment.
"A rule is error once the codebase is clean of it and warn until the pull request that fixes its remaining sites flips it" is a good, checkable policy — worth having written down.
What it doesn't say is what stops the debt growing while a rule sits at warn. With ~7k warnings in the output, a newly introduced violation of a warn rule is invisible: nobody diffs the warning list, and nothing fails. So the twelve rules parked at warn can quietly get worse between now and the PR that is supposed to clear them, which makes each of those PRs bigger than it needs to be.
A per-rule warning baseline that CI compares against (fail if the count for rule X went up) would make the policy self-enforcing. Even without tooling, recording today's count per rule in this document — you already have the five biggest — gives the next person a number to check against rather than a vibe.
2 participants
Summary
anti-slopoxlint JS plugin with upstream'sinstall-anti-slopskill intotools/oxlint/anti-slop, and registers all 18 generic rules plusoxc/no-accumulating-spreadin the root.oxlintrc.json. Source commit and deviations are inUPSTREAM.mdnext to the plugin.errornow. The rest staywarnuntil the PR that clears each one flips it.enforcement.mdexplains how that works.oxlintto 1.78.0 with a matching@oxlint/plugins.Why
A large share of our code and most of our tests are now written with agents, and nothing mechanical stops the low-evidence patterns they tend to produce:
ascasts with no stated reason,unknownparameters, module mocks instead of real seams. Review catches some of it, but not consistently. This lands the rules first, so each follow-up can clear one rule and flip it toerror.The largest remaining warnings are
require-safety-comment-for-type-assertion(348),no-unknown-parameters(148),no-conditional-empty-object-spread(68),no-unsafe-dictionary-type(63) andno-runtime-typeof(57).Alternatives considered
no-module-mocking). Most of the value is in the type-evidence rules, so we would rather adapt our conventions to the full set.erroron install, as the skill does. That is around a thousand sites failingdevat once, so we ratchet rule by rule instead.Risks
pnpm check:lintnow prints around 7k warnings, 6.3k of them fromrequire-readable-spacing. That rule is autofixable, but it touches most files, so it gets its own PR at a quiet moment to avoid conflicts with open branches.types.mdandfunctions.md, and parts of the public API (EventHandler,AdminGuard.assert). Those land as separate PRs with their own discussion.@openora/core/oxlint/oxlintrc.jsonis untouched, so consumers see no change.