Skip to content

oxlint config does not inherit core's no-restricted-imports, so the node:assert/strict ban is unenforced in Pro #736

Description

@dawsontoth

core/.oxlintrc.json carries a no-restricted-imports rule rejecting node:assert/strict and assert/strict:

{
    "name": "node:assert/strict",
    "message": "Use plain node:assert; call assert.strictEqual/deepStrictEqual for strict checks (AGENTS.md test style)."
}

harper-pro's .oxlintrc.json has no such rule — its rules block holds seven entries (no-undef, no-unused-vars, no-console, no-constant-condition, prefer-const, typescript/no-explicit-any, typescript/no-var-requires) and nothing about restricted imports. So npm run lint:required in this repo is clean regardless of which assert module a test imports, and the convention documented in core/AGENTS.md is unenforced on the Pro side.

How it surfaced

A new test file added in #594 imported node:assert/strict. Lint was clean locally and in CI; a reviewer caught it by eye. core/AGENTS.md is explicit about the rule and about the trap — it names unitTests/security/ and unitTests/utility/ as legacy directories that still import /strict but are "not the target shape", which is exactly where the new file landed, so the neighbouring files read as precedent when they aren't. Fixed in b135ac4, but the next new test file will hit the same gap.

Why this isn't a one-line config change

Adding the rule as-is fails lint on 42 existing files:

directory files
integrationTests/ 29
unitTests/ 9
smokeTests/ 2
stressTests/ 2

So it needs a decision about sequencing rather than just a config edit. Options, roughly in increasing order of effort:

  1. Add the rule scoped to new code only — oxlint supports per-directory overrides, so the rule could apply to unitTests/** while the other trees are exempted, then narrowed further over time. Cheapest thing that stops the bleeding.
  2. Add the rule and migrate all 42 files in one pass. Mechanical — assert.equalassert.strictEqual, assert.notEqualassert.notStrictEqual, deepEqualdeepStrictEqual, leaving ok/match/rejects/throws alone (identical on plain assert). Every conversion tightens rather than loosens, so it can't turn a passing assertion into a wrong-but-passing one — but it's a large diff across the integration suite and would want its own PR.
  3. Add the rule at warn severity so it's visible without breaking CI, and let the count drift down.

A related question worth settling in the same pass: whether Pro's oxlint config should inherit from core's generally, rather than the two drifting field by field. This is the instance we noticed; there may be others.

Note on the docs

The harper-engineering-guidelines skill's rules/testing.md currently states the opposite of core/AGENTS.md ("Assertions: node:assert/strict"), which is plausibly why agents keep reaching for it. That lives outside this repo and is being fixed separately, but it's worth knowing that the lint gap and the docs conflict are two halves of the same problem — neither alone explains why this keeps happening.

Provenance

Found while addressing review feedback on #594. Cross-ref: #594, and core/AGENTS.md (test style section) for the convention itself.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:ciCI workflows, GitHub Actions, release automationtestsMostly focused on tests, testing infrastructure, etc.

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions