Add oxlint plugin to @fedify/lint#760
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Oxlint subpath export and plugin implementation for ChangesOxlint Support Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/lint/oxlint/README.md`:
- Around line 6-8: Add a short stability caveat to the README.md's intro
paragraph explaining that Oxlint's JavaScript plugin support is currently
upstream alpha; update the opening lines where the example is described (the
paragraph referencing `@fedify/lint` and [oxlint]) to append a single-sentence
note like "Note: Oxlint's JS plugin support is upstream alpha and may be
unstable" so users are aware of adoption risk.
In `@packages/lint/src/tests/oxlint.test.ts`:
- Line 17: The test import uses the Node-only harness; replace the
runtime-specific import by importing the test harness from the repo-standard
package. Update the import statement that currently reads importing "test"
(symbol: test) from "node:test" to import "test" from "@fedify/fixture" so this
file (oxlint.test.ts) uses the repository-standard, runtime-agnostic test
harness.
- Around line 75-120: Wrap the temporary-dir usage in the test in a try/finally:
create dir with mkdtempSync as now, run the test logic (writes, spawnSync,
assertions) inside try, and in finally call rmSync(dir, { recursive: true,
force: true }) to always remove the fixture. If rmSync is not imported, add it
from 'fs' (or call fs.rmSync) so the cleanup compiles; reference the local
variable dir in the finally block and keep the existing assertions and error
handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b56fc3b9-8fb2-468b-b50d-f1ec0a5eccb9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
docs/manual/lint.mdexamples/lint/oxlint/.oxlintrc.jsonexamples/lint/oxlint/README.mdexamples/lint/oxlint/federation.fixed.tsexamples/lint/oxlint/federation.tsexamples/lint/oxlint/package.jsonpackages/lint/README.mdpackages/lint/package.jsonpackages/lint/src/oxlint.tspackages/lint/src/tests/oxlint.test.tspackages/lint/tsdown.config.tspnpm-workspace.yaml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for oxlint, a fast Rust-based linter, to the @fedify/lint package. It adds a new subpath export, @fedify/lint/oxlint, which exposes the existing Fedify lint rules in a format compatible with oxlint's JS plugin API. The changes include comprehensive documentation updates, a new example project demonstrating oxlint integration, and an integration test suite. Feedback from the reviewer identifies a documentation issue where a JSON code block containing comments should be labeled as jsonc to ensure syntactic correctness.
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Left several nitpicking comments. I guess @2chanhaeng, the original author of @fedify/lint, could give you more helpful feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/lint/src/tests/oxlint.test.ts (1)
72-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the temporary fixture directory after the test.
The test creates a temp directory at line 76 but never removes it, which will accumulate temp directories in local dev and CI environments over time.
🧹 Suggested cleanup patch
-import { existsSync, mkdtempSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";() => { const dir = mkdtempSync(join(tmpdir(), "fedify-lint-oxlint-")); - writeFileSync( - join(dir, ".oxlintrc.json"), - JSON.stringify({ - jsPlugins: [pluginPath], - rules: { - "@fedify/lint/actor-id-required": "error", - }, - }), - ); - writeFileSync(join(dir, "federation.ts"), BAD_CODE); - - const result = spawnSync( - oxlintBin!, - ["--format=json", "."], - { cwd: dir, encoding: "utf8" }, - ); - - ok( - result.status !== 0, - `Expected non-zero exit, got ${result.status}. stderr: ${result.stderr}`, - ); - - let payload: OxlintJsonReport; - try { - payload = JSON.parse(result.stdout) as OxlintJsonReport; - } catch (err) { - throw new Error( - `Failed to parse oxlint JSON output: ${(err as Error).message}\n` + - `stdout: ${result.stdout}\nstderr: ${result.stderr}`, - ); - } - - const codes = (payload.diagnostics ?? []).map((d) => d.code ?? ""); - const matched = codes.some((code) => - code === "@fedify/lint(actor-id-required)" || - code.includes("actor-id-required") - ); - ok( - matched, - `Expected `@fedify/lint`(actor-id-required) diagnostic, got: ${ - codes.join(", ") || "(none)" - }`, - ); + try { + writeFileSync( + join(dir, ".oxlintrc.json"), + JSON.stringify({ + jsPlugins: [pluginPath], + rules: { + "@fedify/lint/actor-id-required": "error", + }, + }), + ); + writeFileSync(join(dir, "federation.ts"), BAD_CODE); + + const result = spawnSync( + oxlintBin!, + ["--format=json", "."], + { cwd: dir, encoding: "utf8" }, + ); + + ok( + result.status !== 0, + `Expected non-zero exit, got ${result.status}. stderr: ${result.stderr}`, + ); + + let payload: OxlintJsonReport; + try { + payload = JSON.parse(result.stdout) as OxlintJsonReport; + } catch (err) { + throw new Error( + `Failed to parse oxlint JSON output: ${(err as Error).message}\n` + + `stdout: ${result.stdout}\nstderr: ${result.stderr}`, + ); + } + + const codes = (payload.diagnostics ?? []).map((d) => d.code ?? ""); + const matched = codes.some((code) => + code === "@fedify/lint(actor-id-required)" || + code.includes("actor-id-required") + ); + ok( + matched, + `Expected `@fedify/lint`(actor-id-required) diagnostic, got: ${ + codes.join(", ") || "(none)" + }`, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lint/src/tests/oxlint.test.ts` around lines 72 - 121, The test creates a temporary directory in the local variable "dir" via mkdtempSync but never removes it; wrap the test body in a try/finally (or add a teardown) and remove the temp directory in the finally block using fs.rmSync(dir, { recursive: true, force: true }) (or equivalent cleanup) to ensure the directory created by mkdtempSync is always deleted after the test (keep references to the existing symbols: mkdtempSync, dir, writeFileSync, spawnSync, and the test named "oxlint plugin: actor-id-required fires on missing id").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/lint/src/tests/oxlint.test.ts`:
- Around line 72-121: The test creates a temporary directory in the local
variable "dir" via mkdtempSync but never removes it; wrap the test body in a
try/finally (or add a teardown) and remove the temp directory in the finally
block using fs.rmSync(dir, { recursive: true, force: true }) (or equivalent
cleanup) to ensure the directory created by mkdtempSync is always deleted after
the test (keep references to the existing symbols: mkdtempSync, dir,
writeFileSync, spawnSync, and the test named "oxlint plugin: actor-id-required
fires on missing id").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e7360ef-6edd-4b6d-8e68-5eaba546c321
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.hongdown.tomldocs/manual/lint.mdexamples/lint/oxlint/README.mdpackages/lint/README.mdpackages/lint/deno.jsonpackages/lint/package.jsonpackages/lint/src/tests/oxlint.test.ts
Two review points on packages/lint/src/tests/oxlint.test.ts:
- Make the runtime story explicit. The test imports `node:*` modules
and spawns the oxlint binary, which is unusual for this package
compared with the in-process Deno.lint / ESLint Linter tests. The
header comment now spells out that Deno's node compat layer is what
keeps the same source running under both `pnpm test` and
`deno task test`, that subprocess-spawning is intentional (oxlint
is a Rust binary), and that the binary plus the built loader are
ignore-skip preconditions.
- Clean up the temp fixture directory. Wrap the test body in
`try/finally` so `rmSync(dir, { recursive: true, force: true })`
runs whether the assertions pass or throw. Avoids local/CI temp
buildup.
Refs: fedify-dev#760
Assisted-by: Claude Code:claude-opus-4-7
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manual/lint.md`:
- Line 318: Remove the unused reference-style link definition "[Rules]: `#rules`"
from docs/manual/lint.md; locate the literal "[Rules]: `#rules`" (the reference
link definition) and delete that line since the page already uses an inline
anchor "( *Rules* section](`#rules`)" and the reference is unused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dedc1a61-f7cd-4198-ad44-257bc24cc719
📒 Files selected for processing (1)
docs/manual/lint.md
There was a problem hiding this comment.
Thanks for contributions! I'm really sorry about late for review.
Would you add "oxlint" to *cspell.json" in root?
And if the Oxlint plugin supports Deno, would you add deno.json to the oxlint examples?
While the review was delayed, there was a minor update for oxlint. I'm truly sorry for the late review, again, but could you also update that package version to the latest? Thank you once again for your contribution!
Per fedify-dev#760 (review): - Bump the pnpm catalog `oxlint` entry from ^1.63.0 to ^1.66.0 so the example and the lint package's dev dependency pick up the upstream fixes that landed while review was in flight. - Add `oxlint` to *cspell.json* so the spell checker stops flagging it in docs and READMEs. - Add *examples/lint/oxlint/deno.json* (and register it in the root Deno workspace) so the example can also be run with `deno task lint` / `deno task lint:fixed`. Oxlint is a Rust binary distributed via npm, and Deno's npm shim runs it just fine; the JS plugin is still resolved out of *node_modules*, so `pnpm install` remains a prerequisite, which the README now mentions. Assisted-by: Claude Code:claude-opus-4-7
Picks up the nitpicks left on fedify-dev#760 that hadn't been folded in yet: - *packages/lint/src/oxlint.ts*: stop exporting `OxlintPlugin`. The interface only exists to type-annotate the default export and isn't meant for downstream import — keeping it private avoids drift from upstream Oxlint's `Plugin` definition in `@oxlint/plugins`. Also drops the redundant named `plugin` export so the file has a single default export, which is the shape Oxlint's JS plugin loader looks for. - *packages/lint/src/tests/oxlint.test.ts*: when the integration test is skipped because the built loader or the oxlint binary is missing, print a concrete remediation (`mise run install` or `pnpm install && pnpm --filter @fedify/lint build`) instead of silently ignoring it. - *cspell.json*: add `oxlintrc` (used in the README and test fixture filename) alongside the previously-added `oxlint`. - *packages/lint/deno.json*: map `oxlint` to `npm:oxlint@^1.66.0` in `imports`, mirroring how the example's *deno.json* already pulls Oxlint through Deno's npm shim. Assisted-by: Claude Code:claude-opus-4-7
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
Thanks for your contributions, again!
Sorry for late. I've been sick for the past few days and still haven't fully recovered, so my review is late. I should have at least let you know. I apologize if you've been waiting a long time.
Before getting into the review, I recommend that in the future, you leave a comment on the review explaining how you applied the feedback along with the relevant commit hash if you accepted it, or the reasoning behind your decision if you dismissed it. This will help the reviewer review your revisions more quickly.
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
Thanks for your awesome contributions! The code is almost complete. If there are no major issues, this will likely be my final review before approval. First, please add prepare as a dependency to the tasks."test:examples" entry in mise.toml.
[tasks."test:examples"]
description = "Run tests for all example projects"
+ depends = ["prepare"]
run = "deno run -A examples/test-examples/mod.ts"test:examples runs with Deno, and as you know, using Oxlint with Deno requires generating the Oxlint binary through a build process. It would be even better to create and add a separate task specifically for generating the Oxlint binary, but I am hesitant to ask because I am not sure how complex that might be. Please look into it and consider implementing it that way if you can.
Secondly, before merging, you need to organize the commit logs using git rebase to make them ready for merging. If you ask @daliah for help with the process, I am sure they will be happy to assist. I would love to help you myself, but @daliah knows much more than I do, whereas I am just barely following along with help from LLM agents. 😂
Anyway, thanks again. I hope this is my last review. See ya!
|
This pull request has some conflicts; could you resolve them? |
✅ Deploy Preview for fedify-json-schema ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Oxlint's JS plugin API can host the same rule set as the ESLint plugin, so projects that lint with oxlint can catch the same Fedify federation mistakes. This exposes the rules in oxlint's plugin shape under a new `@fedify/lint/oxlint` subpath export, configured via `.oxlintrc.json`'s `jsPlugins` field. Note that oxlint's JS plugin support is upstream alpha and may be unstable. Co-authored-by: Hong Minhee (洪 民憙) <hong@minhee.org> Assisted-by: Claude Code:claude-opus-4-8
Demonstrate the plugin end-to-end: federation.ts intentionally violates several rules (missing id, inbox, outbox, followers) while federation.fixed.ts passes them all. Register the example in the test runner so that `mise run test:examples` verifies the fixed fixture lints clean and the violating fixture fails. Assisted-by: Claude Code:claude-opus-4-8
oxlint is a Rust binary, so the only way to exercise the JS plugin loader is to spawn it as a subprocess against a real config file. Two lanes do this: - oxlint.test.ts asserts actor-id-required fires on a missing id. - integration.test.ts runs the whole rule suite through oxlint as an additional lane, so every rule is also checked through the oxlint adapter. The shared setup — locating the binary, the skip precondition (built loader plus binary), the tmpdir fixture, and the JSON parsing — lives in lib/oxlint.ts so both lanes reuse it. When the loader or the binary is missing, the oxlint lanes are skipped with a warning. Assisted-by: Claude Code:claude-opus-4-8
The lint/oxlint example needs two things the runner cannot produce on its own: @fedify/lint's built dist/oxlint.js loader and the oxlint binary in node_modules. `prepare` provides both — it builds the loader via build:self and, through its install dependency, provisions the prebuilt oxlint binary — so depend on it. Assisted-by: Claude Code:claude-opus-4-8
3 participants
fix #702
Expose the existing rule set through oxlint's JS plugin API at the new
@fedify/lint/oxlintsubpath export. The same rules used by the ESLint plugin are reused verbatim — oxlint claims an ESLint-compatible API, so no rule logic is duplicated.Includes an integration test that spawns the oxlint binary against a tmpdir fixture, plus an example project under examples/lint/oxlint/ that demonstrates the configuration with both a violating and a corrected fixture.
edit:
Deno support added!