Skip to content

ci(lint): add oxlint and fail CI on any warning (#828) - #839

Merged
jeff-r2026 merged 17 commits into
Tencent:mainfrom
SaulMoro:ci/oxlint
Sep 26, 2026
Merged

jeff-r2026 merged 17 commits into
Tencent:mainfrom
SaulMoro:ci/oxlint

Conversation

@SaulMoro

@SaulMoro SaulMoro commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds oxlint with its default rules and fails CI on any warning, as agreed in #828. main had 145 warnings. This PR fixes all of them without suppression comments and changes no runtime behavior.

--dry-run for tags subscribe, tags unsubscribe and roles set is delivered by #837, now merged into main. This branch first carried #837's commit (224d459), then reverted it (a288564) and renamed the unused parameter to _options (6fbf636). Since #837 merged, those functions read options again, so the rename is gone and git diff origin/main...HEAD has no dry-run change. Please squash-merge.

main is merged in through a47bb7ea (#833, #834, #835, #837, #840). The code it brought had 5 new warnings and 1 new disable comment, fixed in the merge commits (row below).

 CI job lint-and-test (ci.yml and .coding-ci.yaml)
   npm ci
+  npm run lint        # oxlint --deny-warnings
   npx tsc --noEmit
   npx vitest run --coverage

Warnings after each commit, counted with npx oxlint at that commit, on the stack that still carried #837's commit:

Commit What Warnings
fix(tags,roles) (#837) main has 145; this commit fixes 2 143
chore(lint) Exact-pinned oxlint devDependency, lint script, .oxlintrc.json (defaults plus ignoreRestSiblings, so the rest-omit in dashboard.ts keeps config and roots out of /api/workspaces) 141
style(lint) oxlint --fix: redundant regex escapes, ...(x ?? {}) → ...x, anchored regex → startsWith / endsWith 111
refactor(lint) Unused imports (--fix-suggestions, reviewed by hand; every removed whole import has no side effects) 46
refactor(lint) Dead code: unused locals and functions, options on read-only tagsList / rolesList / generateDigest, the never-read interactive option of importFromRepo, the empty test/e2e.mjs, a try/catch that only rethrew 23
test(lint) Test hits (below) 4
refactor(lint) Control-character classes rewritten as \p{Cc} 0
refactor(lint) 4 eslint-disable comments for rules this repo never ran (no-await-in-loop, @typescript-eslint/no-explicit-any), found by the flag below 0
ci(lint) --deny-warnings --report-unused-disable-directives, both CI files, and npm run lint in CLAUDE.md, AGENTS.md, CONTRIBUTING.md and the PR template gate
Revert + refactor(lint) Drop #837's dry-run commit; rename the 2 unused params to _options 2 → 0
docs(contributing), docs(agents) Node version npm run lint needs, in CONTRIBUTING.md, AGENTS.md and CLAUDE.md (see Review notes) 0
Merge origin/main (8eb4c8b, 3f76815) Conflicts in recall.ts, tags.ts, import-local.ts. New hits in tags.ts: unused path, YAML, readFileSafe imports, unused totalRules, and options on read-only tagsList (its 4 test calls drop {}); a no-await-in-loop disable comment in import-local.ts 5 → 0

The repo ends with zero disable comments. --report-unused-disable-directives makes a suppression fail CI once it no longer suppresses anything, so none can outlive the code it was written for.

Each no-unused-vars hit was checked against its callers and git history before deletion, to tell dead code from missing wiring. Only the --dry-run bug turned up (#837). The other issues found along the way are tracked in #836, together with the next rule sets to evaluate, starting with anti-slop.

Test fixes worth a look:

  • contribute "handles dry-run mode" asserted nothing. It now checks that the run leaves the repo/HOME tree unchanged.
  • expect(result).not.toThrow; on a string was a no-op, and is removed.
  • Two (x?.y as T).z casts would throw a TypeError instead of failing the assertion. They now keep undefined in the type.

Evidence

  • Before: npx oxlint on main at 7c834ce4 gives 145 warnings and exit 0, so nothing enforces them.
    After: npm run lint → oxlint --deny-warnings --report-unused-disable-directives gives 0 warnings and exit 0. Each throwaway probe file gives exit 1:
    src/zz-gate-probe.ts:2:9: warning eslint(no-unused-vars): Variable 'unused' is declared but never used. Unused variables should start with a '_'. help: Consider removing this declaration.
    lint exit=1
    src/zz-directive-probe.ts:2:3: warning: Unused oxlint-disable directive (no problems were reported).
    lint exit=1
    
  • No behavior change:
    • npx tsc --noEmit is clean, npm run build succeeds, and npx vitest run gives 5122 passed, 1 skipped (329 files) after merging main.
    • Every non-test source change was re-read for behavior: removed dead locals and imports, ...(x ?? {}) → ...x (spreading undefined is a no-op), redundant regex escapes, anchored regex → startsWith / endsWith, new Array(n) → Array.from, a rethrow-only try/catch, resolveBaseDir(localConfig) → the baseDir const computed from it at the top of buildRemovalPlan, and the \p{Cc} rewrites below. None changes output.
    • The \p{Cc} rewrites were compared with the old classes on every code point from U+0000 to U+10FFFF. manifest-schema and agent-format match exactly, and the contribute-check normalization pipeline gives identical output. Only the test assertion is stricter.
    • The new contribute dry-run assertion fails when the dry-run early return is removed:
      AssertionError: expected [ '.teamai', 'notes.md', …(1) ] to deeply equal [ 'notes.md' ]
      
  • Real CLI (npm run build, isolated HOME, local team repo). These are the commands whose code or signatures changed:
    $ teamai tags list            → table printed, exit=0
    $ teamai tags                 → same (default action), exit=0
    $ teamai roles list           → manifest and primary role printed, exit=0
    $ teamai roles                → same (default action), exit=0
    $ teamai source               → "No sources configured…", exit=0
    $ teamai import --cache-status → cache root printed, exit=0
    $ teamai digest               → reaches its git step; the fixture is not a git repo, exit=0
    
    Since then the only runtime-code change is the revert (back to main's code) and the _options rename; teamai tags subscribe hai on the rebuilt dist/ reaches tagsSubscribe (it stops at the uninitialized-HOME check, as on main).
    After merging main, teamai tags list and teamai tags subscribe hai --dry-run on the rebuilt dist/ reach requireInit and stop at teamai is not initialized, as on main.
    I did not run digest against a real team repo: the sandbox here doesn't allow creating a git repo outside the worktree. Its only change is dropping an unused parameter, and tsc checks the call site.

Review notes

  • oxlint 1.85.0 needs Node ^20.19.0 || >=22.12.0 (P2): kept on purpose. oxlint is a devDependency only, so it never reaches users who install teamai-cli, and the runtime minimum stays Node ≥ 20. CI's node-version: 20 resolves to the latest 20.x, which satisfies it. .github/CONTRIBUTING.md says npm run lint needs Node ^20.19 or >=22.12 while the CLI still supports Node 20, and the toolchain line in AGENTS.md and CLAUDE.md now says the same. The Node 20+ mentions in skill-data/setup/ are for end users installing the CLI and stay as they are.

Merge Danger

Door: two-way

Blast Radius: contributors

Runtime behavior is unchanged. Every PR opened after this one must pass npm run lint. The warning text says what to change, and npm run lint -- --fix handles most mechanical cases. oxlint is pinned exactly, so a new default rule arrives only through a deliberate version bump. Reverting is a single revert.

…and roles set (Tencent#836)

The three commands saved the local config and reset lastPullRev even
under the global --dry-run, which is documented as "Preview mode, no
changes made". Their siblings (tags add/remove, roles init/add/remove/
update) already return early with a [dry-run] message.

The roles set preview names the additional roles it would save,
including none, because a real run replaces the existing list.

oxlint reported the unused options parameter in tagsSubscribe and
tagsUnsubscribe; rolesSet has the same bug but reads options.add.
Pinned to an exact version so a new default rule arrives in its own PR,
not as a CI failure on an unrelated one. The no-unused-vars options
keep oxlint's _ ignore patterns and add ignoreRestSiblings, which the
rest-omit in dashboard.ts relies on to keep config and roots out of
/api/workspaces.
Drop redundant escapes in regex character classes and template literals,
empty-object fallbacks in object spreads (spreading undefined adds
nothing), and anchored regexes that are plain startsWith/endsWith
checks. No behavior change.
Applied with oxlint --fix-suggestions and reviewed by hand. Every
removed whole import is a library module with no import-time side
effects.
Each hit was checked against its callers and git history; none is
missing wiring (the two that were, tags subscribe/unsubscribe, are fixed
in the preceding commit). Removed: unused locals and functions, the
options parameter of tagsList, rolesList and generateDigest (read-only
commands), the never-read interactive option of importFromRepo, the
empty test/e2e.mjs left over from the E2E migration, and a try/catch
that only rethrew. new Array(n) becomes Array.from. No behavior change.
- contribute dry-run test asserted nothing; it now checks that the run
  leaves the repo/HOME tree unchanged (verified to fail when the dry-run
  early return is removed).
- Drop a no-op expect(result).not.toThrow on a string.
- Keep undefined in two optional-chain casts so a regression fails the
  assertion instead of throwing a TypeError.
- Remove unused locals, helpers and imports; new Array(n) becomes
  Array.from.
no-control-regex flags literal control ranges. \p{Cc} names the same
set (C0, DEL, C1) and reads as what it means. Checked against the old
classes on every code point from U+0000 to U+10FFFF: manifest-schema and
agent-format match exactly, and contribute-check's normalization
pipeline produces the same output. The test assertion is now stricter
and checks every control character the sanitizer removes.
Four eslint-disable comments named rules this repo never ran
(no-await-in-loop, @typescript-eslint/no-explicit-any), so they
suppressed nothing.
npm run lint runs oxlint --deny-warnings and runs before the type check
in both GitHub Actions and Coding CI. The repo is at zero warnings, so
new code must stay clean. --report-unused-disable-directives also fails
on a disable comment that suppresses nothing, so a suppression cannot
outlive the code it was written for. CLAUDE.md, AGENTS.md,
CONTRIBUTING.md and the PR template list the command so contributors
and agents run it before opening a PR.

Closes Tencent#828
@github-actions

Copy link
Copy Markdown
  • [P1 blocking] src/tags.ts:122 — This introduces runtime behavior for tags subscribe/unsubscribe --dry-run (and roles set --dry-run), but the PR’s real-CLI evidence exercises only the list commands. The Code Review Rules require a representative real-CLI verification of the changed runtime path; add a built-CLI run confirming these commands preview without modifying config/state.
  • [P2 non-blocking] package.json:86 — oxlint@1.85.0 requires Node ^20.19.0 || >=22.12.0 (package-lock.json:4225), while the project documents Node.js ≥20. On Node 20.0–20.18, installations using engine-strict=true fail, and other installations use an unsupported runtime. Either select a compatible oxlint release or raise the documented minimum Node version consistently.

…20.0

oxlint 1.17.0 and later declare engines.node ^20.19.0 || >=22.12.0,
while the repo supports Node >=20. 1.16.0 declares >=8, supports
--deny-warnings and --report-unused-disable-directives, and reports 0
warnings on this branch.
…nd unsubscribe

With the Tencent#837 dry-run fix reverted out of this PR, both functions no
longer read options. The underscore prefix keeps the signature and call
sites unchanged, so Tencent#837 can rebase onto it by renaming the parameter
back.
This reverts commit e2347ef. oxlint is a devDependency, so its Node
requirement (^20.19.0 || >=22.12.0) never reaches users installing
teamai-cli, and CI's node-version 20 resolves to the latest 20.x.
Staying on 1.85.0 keeps the Tencent#836 warning counts and the planned
type-aware follow-up on the same version.
@github-actions

Copy link
Copy Markdown
  • [P2 non-blocking] AGENTS.md:5 — The contributor toolchain still says “Node 20+” while listing npm run lint, but oxlint@1.85.0 requires ^20.19.0 || >=22.12.0 (package-lock.json:4225). Contributors using Node 20.0–20.18 may get an unsupported lint runtime or an engine-strict installation failure. Mirror the exact development minimum here and in CLAUDE.md, or use a compatible oxlint version.

The earlier [P1 blocking] dry-run finding is resolved: those runtime changes are absent from the final diff, and the PR description includes sufficient real-CLI verification for the remaining command-signature cleanup.

@github-actions

Copy link
Copy Markdown

No new findings.

  • Resolved: The earlier [P1 blocking] dry-run concern is absent from the final diff, and the PR description provides representative real-CLI verification for the remaining signature cleanup.
  • Resolved: The oxlint Node requirement is now documented in AGENTS.md:5, CLAUDE.md:5, and .github/CONTRIBUTING.md:24.
  • Testing: The documented lint, typecheck, unit-test, build, and real-CLI evidence is sufficient under the repository’s review rules.

Resolve src/recall.ts and src/tags.ts; drop the unused imports, variable
and tagsList parameter the merged code brought in.

@jeff-r2026 jeff-r2026 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the conflicts.

Resolve src/import-local.ts: keep Tencent#840's English prompt and drop the
no-await-in-loop disable comment, as this PR does for the others.
@github-actions

Copy link
Copy Markdown

No new findings.

  • Resolved: The earlier [P1 blocking] dry-run concern is absent from the current diff.
  • Resolved: The oxlint Node requirement is documented consistently in AGENTS.md:5, CLAUDE.md:5, and .github/CONTRIBUTING.md:24.
  • Testing: The PR description includes sufficient lint, typecheck, unit-test, build, and representative real-CLI verification.

@jeff-r2026
jeff-r2026 merged commit f7da1bb into Tencent:main Sep 26, 2026
11 checks passed
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.

2 participants