Stephansama/ste 292 create glob based linter#311
Open
stephansama wants to merge 20 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 8a12398 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 ChangesMonorule Core Implementation
Sequence DiagramsequenceDiagram
participant CLI
participant LoadConfig
participant CheckRules
participant ApplyRules
participant FS
CLI->>LoadConfig: loadConfig(cli flags)
LoadConfig->>CheckRules: return config with rules
CLI->>CheckRules: checkRules(config)
CheckRules->>ApplyRules: dirty files (if --fix)
ApplyRules->>FS: writeFile(path, output)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 |
![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 @stephansama/monorule, a monorepo linting and fixing CLI tool that validates and applies custom rules across workspace packages. The review feedback highlights several critical and high-severity issues, including a race condition in applyRules when concurrently writing to the same file, disabled rules still being executed during checks, and potential crashes from unhandled exceptions when parsing malformed package.json files. Additionally, the reviewer suggested avoiding synchronous file system calls to prevent blocking the event loop and removing a debug log that clutters the CLI output.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+6
to
+65
| import { stringify } from "./parse"; | ||
|
|
||
| export async function applyRules(dirtyFiles: DirtyFile[], rules: RuleMap) { | ||
| await Promise.all( | ||
| dirtyFiles.map(async (file) => { | ||
| console.log(`applying ${file.rule} to ${file.relativePath}`); | ||
|
|
||
| const currentRule = rules[file.rule]; | ||
| if (currentRule.apply) { | ||
| const result = currentRule.apply( | ||
| file.content as object | string, | ||
| file, | ||
| ); | ||
|
|
||
| if (result instanceof Promise) { | ||
| console.info( | ||
| `stringifying ${currentRule.name} for ${file.absolutePath}`, | ||
| ); | ||
| const string_ = stringify( | ||
| await result, | ||
| typeof currentRule.parse === "function" | ||
| ? "json" | ||
| : currentRule.parse, | ||
| ); | ||
|
|
||
| await fs.promises.writeFile( | ||
| file.absolutePath, | ||
| string_, | ||
| "utf8", | ||
| ); | ||
|
|
||
| console.info( | ||
| `applied lint rule ${currentRule.name} to ${file.absolutePath}`, | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| console.info( | ||
| `stringifying ${currentRule.name} for ${file.absolutePath}`, | ||
| ); | ||
|
|
||
| const string_ = stringify( | ||
| result, | ||
| typeof currentRule.parse === "function" | ||
| ? "json" | ||
| : currentRule.parse, | ||
| ); | ||
|
|
||
| await fs.promises.writeFile(file.absolutePath, string_, "utf8"); | ||
|
|
||
| console.info( | ||
| `applied lint rule ${currentRule.name} to ${file.absolutePath}`, | ||
| ); | ||
| } | ||
| }), | ||
| ); | ||
| console.info({ dirty: dirtyFiles }); | ||
| console.info(rules); | ||
| } |
There was a problem hiding this comment.
Race Condition & Redundant Code in Rule Application
There is a critical race condition here. When multiple rules are dirty for the same file, they are processed concurrently via Promise.all. Each rule applies its changes to its own stale copy of file.content and writes to the same file.absolutePath concurrently. This means whichever write finishes last will overwrite the others, leading to lost fixes.
Additionally, the code for handling promises vs. synchronous results is highly duplicated. Since await seamlessly handles both promises and non-promises, we can unify them.
We can resolve both issues by grouping the dirty files by their absolute path and applying their rules sequentially per file.
import { parse, stringify } from "./parse";
export async function applyRules(dirtyFiles: DirtyFile[], rules: RuleMap) {
const filesGrouped = new Map<string, DirtyFile[]>();
for (const file of dirtyFiles) {
const list = filesGrouped.get(file.absolutePath) || [];
list.push(file);
filesGrouped.set(file.absolutePath, list);
}
for (const [absolutePath, files] of filesGrouped) {
try {
let raw = await fs.promises.readFile(absolutePath, "utf8");
for (const file of files) {
const currentRule = rules[file.rule];
if (!currentRule || !currentRule.apply) continue;
console.log("applying " + currentRule.name + " to " + file.relativePath);
const parsed = parse(raw, currentRule.parse);
const result = await currentRule.apply(parsed as any, file);
raw = stringify(
result,
typeof currentRule.parse === "function"
? "json"
: currentRule.parse,
);
}
await fs.promises.writeFile(absolutePath, raw, "utf8");
} catch (error) {
console.error("Failed to apply rules to " + absolutePath + ":", error);
}
}
}
Comment on lines
+20
to
+21
| Object.values(rules).map(async (rule) => { | ||
| const cwd = process.cwd(); |
There was a problem hiding this comment.
Disabled Rules Are Still Executed
Currently, checkRules iterates over all rules in rules without checking if they are enabled. If a rule has enabled: false (like the json rule in the hello example), it will still be executed.
Filter out disabled rules before running the glob and checks.
Suggested change
| Object.values(rules).map(async (rule) => { | |
| const cwd = process.cwd(); | |
| Object.values(rules) | |
| .filter((rule) => rule.enabled !== false) | |
| .map(async (rule) => { | |
| const cwd = process.cwd(); |
Comment on lines
+40
to
+42
| const closestPackageJson = closestPackageFile | ||
| ? (JSON.parse(closestPackageFile) as PackageJSON) | ||
| : undefined; |
There was a problem hiding this comment.
Unhandled Exception on Malformed package.json
If any package.json file encountered during the traversal is malformed or contains invalid JSON, JSON.parse will throw an unhandled exception and crash the entire linter. Wrapping this in a try-catch block ensures the linter is robust against malformed files.
let closestPackageJson: PackageJSON | undefined;
if (closestPackageFile) {
try {
closestPackageJson = JSON.parse(closestPackageFile) as PackageJSON;
} catch {
console.warn("Failed to parse package.json at " + closestPackagePath);
}
}
Comment on lines
+12
to
+14
| if (!fs.existsSync(absolute)) return []; | ||
|
|
||
| const files = await fs.promises.readdir(absolute); |
There was a problem hiding this comment.
Avoid Blocking the Event Loop with Synchronous FS Methods
Using synchronous file system methods like fs.existsSync inside an asynchronous function blocks the Node.js event loop. It is more idiomatic and performant to attempt the asynchronous readdir directly and handle the ENOENT error if the directory does not exist.
let files: string[];
try {
files = await fs.promises.readdir(absolute);
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
return [];
}
throw error;
}
Comment on lines
+64
to
+66
| export async function run() { | ||
| console.info(cliArguments.command); | ||
| if (cliArguments.command && cliArguments.command in actions) { |
There was a problem hiding this comment.
Clean Up Debug Log
console.info(cliArguments.command); prints undefined to the console on every normal run of the linter (when no subcommand is specified). This clutters the CLI output and should be removed.
export async function run() {
if (cliArguments.command && cliArguments.command in actions) {
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@core/monorule/example/monorule.config.ts`:
- Line 1: The example config imports the built module ("import { defineConfig,
defineRule } from \"../dist/index.mjs\"") which breaks if users run the example
before building; update the file (monorule.config.ts) to either import directly
from the source alias used by rules (e.g., replace the "../dist/index.mjs"
import with the project's source alias/import path used elsewhere such as
"`@/src`") or add a clear top-of-file comment explaining this example depends on a
prior tsdown/build step and must import from ../dist only for demonstrating
end-user usage.
In `@core/monorule/example/rules/hello.ts`:
- Around line 39-41: The example txtRule.when currently returns undefined (body
only has a commented line) so the rule is a no-op; change txtRule.when to return
an errors array (empty array when no problems, non-empty array of string
messages when it should flag the input) instead of undefined/boolean — e.g.
implement when(input) to check the condition (like
input.includes("stephansama")) and return [] if fine or ['describe problem
here'] when the rule should fire so check.ts will treat it correctly and allow
txtRule.apply to run.
In `@core/monorule/README.md`:
- Line 1: The README.md starts without a top-level heading and then jumps from
H1 to an H5 at the section around line 16; update the file so the first line
begins with a top-level heading (add a "# " title) and change the heading at the
H5 spot to an appropriate hierarchical level (e.g., "##" or "###") so headings
flow H1 -> H2/ H3 rather than H1 -> H5, ensuring all headings in README.md have
consistent levels.
In `@core/monorule/src/apply.ts`:
- Around line 13-15: The code dereferences rules[file.rule] without checking it;
change the apply logic in apply.ts to first look up const currentRule =
rules[file.rule] and if it's undefined throw or return a clear error (e.g.,
throw new Error(`Unknown rule: ${file.rule}`) or a structured failure) before
using currentRule.apply; alternatively, enforce key/name consistency where
RuleMap is constructed so DirtyFile.rule matches RuleMap keys, but at minimum
add the explicit guard around currentRule and fail with a descriptive message.
- Around line 63-64: Remove the raw debug dumps that print full repository data:
delete the console.info calls that log the variables dirtyFiles and rules and
replace them with minimal structured outputs (e.g., counts or sanitized
identifiers). Specifically, in apply.ts stop logging the full dirtyFiles object
and the full rules map; instead log something like the number of dirty files and
the number of rules or a list of file paths/ rule IDs only (use the variables
dirtyFiles and rules to derive counts/IDs). Ensure no raw file contents or full
rule objects are emitted to stdout.
In `@core/monorule/src/check.ts`:
- Around line 74-83: The code assumes rule.when(...) returns an Error[] but that
can be any truthy value; update the handling around the const errors =
rule.when(parsed, context) call to first check for falsy and then distinguish
array vs non-array: if errors is falsy return false; if Array.isArray(errors)
treat it as before (check length, map id/message, etc.); otherwise treat the
single non-array truthy value as a single error (stringify or log its
message/property) and return true — make these checks where errors is declared
so rule.when, errors, parsed and context are the referenced symbols.
In `@core/monorule/src/cli.ts`:
- Around line 46-51: The CLI parses a dryRun flag (dryRun /
cliArguments.flags.dryRun) but never honors it — the code that applies rule
changes (the write/apply-writes step around line 77) executes unconditionally.
Before calling the function that writes or applies rules, add a branch checking
cliArguments.flags.dryRun: if true, do NOT perform file modifications; instead
gather and print/report the list of impacted files and the rule changes that
would be made (same format currently used after writes), and only perform the
actual apply-writes path when dryRun is false. Ensure the check references
cliArguments.flags.dryRun and keeps existing reporting code so dry-run output
matches real-run output.
In `@core/monorule/src/commands/generate.ts`:
- Around line 9-12: The generate command currently no-ops; update the act
function in core/monorule/src/commands/generate.ts (the exported async function
act(arguments_ : typeof cliArguments)) to either (A) fail-fast by throwing a
clear, non-ambiguous error like "Generate command not implemented" (so the CLI
returns a non-zero exit) or (B) implement the minimal scaffold: use arguments_
(e.g., target path or cwd) to write a default configuration file (starter
config) and create a rules directory scaffold (empty directory plus a sample
rule file and README), ensuring filesystem errors are propagated and logged;
pick one approach and make sure act returns a rejecting Promise when not
implemented or resolves only after the files/directories are created.
In `@core/monorule/src/commands/list.ts`:
- Around line 40-45: The --affected flag is declared but never used in the list
command; update the command handler in core/monorule/src/commands/list.ts to
respect the affected flag (or remove the flag if you prefer to postpone
support). Concretely: in the function that executes the list command, check
options.affected and when true, compute and include the affected files for each
rule (call or implement a helper like getAffectedFilesForRule or reuse existing
affected-file logic) and append that data to the output instead of the current
default listing; if no helper exists, implement a small routine that resolves
affected file paths per rule and integrate it into the list flow. If you choose
to remove support, delete the affected flag definition (alias "a") and any
references so the CLI won’t advertise an unsupported option.
In `@core/monorule/src/config.ts`:
- Around line 19-29: The code incorrectly sets options.searchPlaces =
[arguments_.flags.config] and still calls explorer.search(); instead, when a
--config path is provided (check arguments_.flags.config), call
explorer.load(arguments_.flags.config) to load that explicit filepath and use
its returned config/filepath in the existing merge flow; do not override
options.searchPlaces for this case, keep the default options otherwise, and
replace the await explorer.search() use with the load result when a config flag
is present (referencing variables explorer, arguments_.flags.config, options,
moduleName and the existing merge logic).
In `@core/monorule/src/load.ts`:
- Around line 14-23: The loader currently imports every entry returned by files
= await fs.promises.readdir(absolute), which can include directories or non-rule
files and cause failures; update the logic that builds loadedRules to first
filter directory entries to only regular files with the rule file extensions you
expect (e.g., .js, .ts) — use fs.promises.readdir with { withFileTypes: true }
and Dirent.isFile (or stat) and check path.extname before importing — then
inside the files.map async callback around import(path.join(absolute, file))
wrap the import and ruleMapSchema.parse(...) in a try/catch so a single bad file
doesn’t reject the entire Promise.all, returning an empty array or skipping that
file on error; reference symbols: files, register(), loadedRules,
import(path.join(absolute, file)), and ruleMapSchema.parse.
- Around line 16-27: The current code calls register() and later awaits
unregister(), but if any import or ruleMapSchema.parse in the files.map async
loop throws the unregister() call at the end is skipped; wrap the registration
scope in try/finally: call const unregister = register() then run the
Promise.all(files.map(...)) inside a try block and call await unregister() in
the finally block so unregister() always runs even on import/parse errors
(references: register(), unregister, files.map async import(...), and
ruleMapSchema.parse).
In `@core/monorule/src/schema.ts`:
- Line 10: The schema's when property currently uses z.function() with no return
constraint, which lets any return value slip through and can crash code in
core/monorule/src/check.ts where the result is treated as Array<Error> | void
(used with errors.map). Fix by constraining the when function's return to either
an array of Error instances or undefined — e.g., use z.function().returns(...)
with z.array(z.instanceof(Error)) wrapped as optional/union with z.undefined()
(or z.optional) so callers of when can be safely treated as Array<Error> | void.
- Line 5: The schema currently requires an apply function (apply: z.function())
which rejects valid check-only rules; update the contract in
core/monorule/src/schema.ts to make the apply property optional (e.g., change
the apply schema to an optional function) so that rules without an apply are
accepted, and ensure any related types/usages (e.g., the rule type exported from
schema and consumers like core/monorule/src/apply.ts) handle the possibly
undefined apply correctly.
In `@core/monorule/src/type.ts`:
- Around line 59-61: Remove the accidental public export `t` from the module:
locate the exported symbol `t` (declared as Undefinable<{ test: string }>) in
type.ts and either delete the declaration or change it to a non-exported/local
variable or move it into test code so it does not appear in the package API;
ensure no other code depends on the exported `t` and run the build/tests after
removal.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5730cda-8508-437c-8246-070fc92e5a5f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.config/.cspell.jsonREADME.mdcore/monorule/README.mdcore/monorule/__snapshots__/tsnapi/cli.snapshot.d.tscore/monorule/__snapshots__/tsnapi/cli.snapshot.jscore/monorule/__snapshots__/tsnapi/index.snapshot.d.tscore/monorule/__snapshots__/tsnapi/index.snapshot.jscore/monorule/__snapshots__/tsnapi/schema.snapshot.d.tscore/monorule/__snapshots__/tsnapi/schema.snapshot.jscore/monorule/example/data/linted.jsoncore/monorule/example/data/linted.txtcore/monorule/example/index.tscore/monorule/example/monorule.config.tscore/monorule/example/rules/hello.tscore/monorule/package.jsoncore/monorule/src/apply.tscore/monorule/src/check.tscore/monorule/src/cli.tscore/monorule/src/commands/generate.tscore/monorule/src/commands/index.tscore/monorule/src/commands/list.tscore/monorule/src/config.tscore/monorule/src/index.tscore/monorule/src/load.tscore/monorule/src/log.tscore/monorule/src/parse.tscore/monorule/src/rule.tscore/monorule/src/schema.tscore/monorule/src/type.tscore/monorule/tsconfig.jsoncore/monorule/tsdown.config.tspnpm-workspace.yamltsconfig.base.jsontsconfig.json
| @@ -0,0 +1,31 @@ | |||
| <div align="center"> | |||
There was a problem hiding this comment.
Fix heading structure to satisfy markdownlint rules.
Line 1 should start with a top-level heading, and Line 16 jumps from H1 to H5. This can trip markdown linting and reduce doc consistency.
Suggested diff
-<div align="center">
-
-# `@stephansama/monorule`
+ # `@stephansama/monorule`
+
+<div align="center">
@@
-##### Table of contents
+## Table of contentsAlso applies to: 16-16
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 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 `@core/monorule/README.md` at line 1, The README.md starts without a top-level
heading and then jumps from H1 to an H5 at the section around line 16; update
the file so the first line begins with a top-level heading (add a "# " title)
and change the heading at the H5 spot to an appropriate hierarchical level
(e.g., "##" or "###") so headings flow H1 -> H2/ H3 rather than H1 -> H5,
ensuring all headings in README.md have consistent levels.
Comment on lines
+13
to
+15
| const currentRule = rules[file.rule]; | ||
| if (currentRule.apply) { | ||
| const result = currentRule.apply( |
There was a problem hiding this comment.
Guard missing rule lookups before dereferencing.
At Line 13, rules[file.rule] can be undefined (the RuleMap key is not type-coupled to rule.name), and Line 14 will throw on currentRule.apply. Add an explicit guard and fail with a clear error, and/or enforce key/name consistency when rules are built so DirtyFile.rule and RuleMap keys use the same identifier.
🤖 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 `@core/monorule/src/apply.ts` around lines 13 - 15, The code dereferences
rules[file.rule] without checking it; change the apply logic in apply.ts to
first look up const currentRule = rules[file.rule] and if it's undefined throw
or return a clear error (e.g., throw new Error(`Unknown rule: ${file.rule}`) or
a structured failure) before using currentRule.apply; alternatively, enforce
key/name consistency where RuleMap is constructed so DirtyFile.rule matches
RuleMap keys, but at minimum add the explicit guard around currentRule and fail
with a descriptive message.
Comment on lines
+14
to
+23
| const files = await fs.promises.readdir(absolute); | ||
|
|
||
| const unregister = register(); | ||
|
|
||
| const loadedRules = await Promise.all( | ||
| files.map(async (file) => { | ||
| console.info(`loading ${file}`); | ||
| // convert to object to remove module namespace key | ||
| const imported = (await import(path.join(absolute, file))) as {}; | ||
| return Object.values(ruleMapSchema.parse({ ...imported })); |
There was a problem hiding this comment.
Filter directory entries before dynamic import.
Line 14-23 imports every entry from readdir, including non-rule files and subdirectories. One unsupported file can fail the entire rules load path.
🤖 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 `@core/monorule/src/load.ts` around lines 14 - 23, The loader currently imports
every entry returned by files = await fs.promises.readdir(absolute), which can
include directories or non-rule files and cause failures; update the logic that
builds loadedRules to first filter directory entries to only regular files with
the rule file extensions you expect (e.g., .js, .ts) — use fs.promises.readdir
with { withFileTypes: true } and Dirent.isFile (or stat) and check path.extname
before importing — then inside the files.map async callback around
import(path.join(absolute, file)) wrap the import and ruleMapSchema.parse(...)
in a try/catch so a single bad file doesn’t reject the entire Promise.all,
returning an empty array or skipping that file on error; reference symbols:
files, register(), loadedRules, import(path.join(absolute, file)), and
ruleMapSchema.parse.
Comment on lines
+16
to
+27
| const unregister = register(); | ||
|
|
||
| const loadedRules = await Promise.all( | ||
| files.map(async (file) => { | ||
| console.info(`loading ${file}`); | ||
| // convert to object to remove module namespace key | ||
| const imported = (await import(path.join(absolute, file))) as {}; | ||
| return Object.values(ruleMapSchema.parse({ ...imported })); | ||
| }), | ||
| ); | ||
|
|
||
| await unregister(); |
There was a problem hiding this comment.
Always unregister tsx hooks via try/finally.
If any import in Line 18-25 throws, unregister() at Line 27 is skipped. Wrap registration scope in try/finally to avoid leaking runtime hooks.
Suggested diff
const unregister = register();
-
- const loadedRules = await Promise.all(
- files.map(async (file) => {
- console.info(`loading ${file}`);
- // convert to object to remove module namespace key
- const imported = (await import(path.join(absolute, file))) as {};
- return Object.values(ruleMapSchema.parse({ ...imported }));
- }),
- );
-
- await unregister();
+ try {
+ const loadedRules = await Promise.all(
+ files.map(async (file) => {
+ console.info(`loading ${file}`);
+ const imported = (await import(path.join(absolute, file))) as {};
+ return Object.values(ruleMapSchema.parse({ ...imported }));
+ }),
+ );
+ return loadedRules.flat();
+ } finally {
+ await unregister();
+ }
-
- return loadedRules.flat();🤖 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 `@core/monorule/src/load.ts` around lines 16 - 27, The current code calls
register() and later awaits unregister(), but if any import or
ruleMapSchema.parse in the files.map async loop throws the unregister() call at
the end is skipped; wrap the registration scope in try/finally: call const
unregister = register() then run the Promise.all(files.map(...)) inside a try
block and call await unregister() in the finally block so unregister() always
runs even on import/parse errors (references: register(), unregister, files.map
async import(...), and ruleMapSchema.parse).
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@stephansama/catppuccin-jsonresume-theme
@stephansama/catppuccin-opml
@stephansama/catppuccin-rss
@stephansama/catppuccin-typedoc
@stephansama/catppuccin-xsl
@stephansama/eslint-config
create-stephansama-example
@stephansama/find-makefile-targets
@stephansama/github-env
@stephansama/inline-image
@stephansama/monorule
@stephansama/multipublish
@stephansama/pnpm-hooks
@stephansama/prettier-plugin-handlebars
@stephansama/remark-asciinema
@stephansama/single-file
@stephansama/svelte-social-share-links
@stephansama/typed-env
@stephansama/typed-events
@stephansama/typed-nocodb-api
@stephansama/typed-templates
@stephansama/types-github-action-env
@stephansama/types-lhci
commit: |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/monorule/example/rules/hello.ts (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the unresolved example import.
@/srcis not resolvable in the example lint/test context, and CI is already failing withimport-x/no-unresolved. Import the package entry through a path that is valid from this file instead of the internal alias.🤖 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 `@core/monorule/example/rules/hello.ts` at line 3, The import using the internal alias (import { defineRule } from "`@/src`") is unresolved in the example context; replace it with an import from the package entry point that is resolvable from this file (i.e., import defineRule from the published package name or a relative path that reaches the package root) so the symbol defineRule is imported from a valid module instead of the "`@/src`" alias.core/monorule/src/apply.ts (1)
25-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCustom parsers are always reserialized as JSON.
When
currentRule.parseis a function, this code hardcodesstringify(..., "json"). Butcore/monorule/src/schema.tsexplicitly allows function parsers for any rule shape, not just JSON-backed ones. A rule that parses YAML/TOML/text through a custom function will be rewritten as JSON here, which breaks the file format. The rule contract needs an explicit output format/stringifier, or fix mode needs to reject function-based parsers.Also applies to: 49-53
🤖 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 `@core/monorule/src/apply.ts` around lines 25 - 29, When applying fixes, do not unconditionally stringify outputs from function-based parsers as JSON; detect when currentRule.parse is a function and refuse/fail fix-mode instead of calling stringify(..., "json"). Update the logic around the stringify call (the use of result and currentRule.parse in apply.ts) to throw or skip with a clear error message instructing that function-based parsers need an explicit output stringifier (e.g., a new currentRule.stringify or outputFormat property) or cannot be auto-fixed; apply the same guard where stringify is used later (the same pattern around lines 49-53) so function parsers are not silently reserialized to JSON.
♻️ Duplicate comments (3)
core/monorule/example/rules/hello.ts (1)
36-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
txtRule.whenreturn actual violations.This still returns
undefined, socheckRules()treats every.txtfile as clean andapplynever runs. The example rule is effectively a no-op.Possible fix
- when() { - // return !input.includes("stephansama"); + when(input) { + if (input.startsWith("stephansama")) return; + return [ + { + id: "missing_prefix", + message: "text has not been prefixed", + }, + ]; },🤖 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 `@core/monorule/example/rules/hello.ts` around lines 36 - 38, txtRule.when currently returns undefined so checkRules treats files as clean; update txtRule.when to return actual violation(s) when the rule matches (e.g., return a non-empty array/object describing the violation or a boolean true depending on the rule contract) and an empty value when clean. Locate the txtRule.when function in hello.ts and replace the commented stub with a real return that checks input (e.g., if !input.includes("stephansama") then return a violation descriptor like { message: "missing stephansama", location: ... } or an array containing that object) so checkRules sees a violation and txtRule.apply will run. Ensure the returned shape matches what checkRules expects.core/monorule/README.md (1)
1-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the README heading hierarchy.
The file still starts with a
<div>instead of an H1, and the next section jumps straight to H5. That keeps the markdown heading issue from the earlier review unresolved.Suggested diff
-<div align="center"> - -# `@stephansama/monorule` +# `@stephansama/monorule` + +<div align="center"> @@ -##### Table of contents +## Table of contents🤖 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 `@core/monorule/README.md` around lines 1 - 16, Replace the top-level HTML block "<div align="center">" and the centered title with a proper Markdown H1 header (e.g., "# `@stephansama/monorule`") and adjust the following "##### Table of contents" to a sensible next-level heading such as "## Table of contents" so heading hierarchy is correct; ensure any remaining decorative HTML is removed or moved below the H1 and that subsequent section headings follow a logical H2/H3 sequence instead of jumping to H5.core/monorule/src/check.ts (1)
96-100:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNormalize
rule.when(...)before treating it asRuleError[].This still crashes if a dynamically loaded rule returns any truthy non-array value:
.lengthand the spread both assumeRuleError[]. Tightening the TypeScript type incore/monorule/src/type.tsdoes not protect this runtime boundary.Suggested fix
- // `@ts-expect-error` works - const errors = rule.when(parsed, context); - if (!errors || errors.length === 0) return false; - - checkedErrors.push(...errors); + // `rule.when` is user code loaded at runtime, so normalize before using it. + const result = rule.when(parsed, context as never); + if (!result) return false; + + if (!Array.isArray(result)) { + throw new TypeError( + `Rule "${rule.name}" must return RuleError[] or void from when()`, + ); + } + + if (result.length === 0) return false; + checkedErrors.push(...result);🤖 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 `@core/monorule/src/check.ts` around lines 96 - 100, Normalize the return value of rule.when(parsed, context) before assuming it's a RuleError[]: call rule.when and store its result (e.g., const result = rule.when(parsed, context)); if result is falsy skip, if Array.isArray(result) push its elements into checkedErrors, otherwise coerce a single truthy value into an array (e.g., [result]) before spreading; use the symbols rule.when, parsed, context, and checkedErrors to locate and update the code path so non-array truthy returns won't cause .length or spread runtime errors.
🤖 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 `@core/monorule/src/apply.ts`:
- Around line 10-12: The code currently uses Promise.all over dirtyFiles which
can run multiple fixes concurrently against the same file; group dirtyFiles by
file.absolutePath so each unique path's entries are processed sequentially
(e.g., build a map from absolutePath to array of DirtyFile entries), then for
each group iterate with a for...of and apply rules one-by-one (reparsing the
file state between each rule application) instead of running them in parallel;
use the DirtyFile.rule identifier and the existing apply logic to ensure you
serialize writes per absolutePath and avoid concurrent writes to the same file.
In `@core/monorule/src/check.ts`:
- Around line 26-27: The aggregated errors currently push only RuleError into
the checkedErrors array (symbol: checkedErrors) which loses file/rule context;
change the collection to store an object like { error: RuleError, relativePath:
string, rule: string } or group by DirtyFile before formatting (affecting the
logic around where checkedErrors is populated and where the summary is printed,
also lines around 100-121). Update the type of checkedErrors, adjust all push
sites (where RuleError instances are added) to include relativePath and rule (or
attach the DirtyFile reference), and then modify the summary/formatting code to
use these fields so output shows the originating file and rule for each error;
update any related imports/types to match the new structure.
In `@core/monorule/src/cli/run.ts`:
- Around line 24-25: The CLI currently ignores the dryRun flag; update run() to
read getFlag(cliArguments, "dryRun") and branch accordingly: when getFlag(...,
"fix") is true call applyRules(dirty, config.rules) as before, but when
getFlag(..., "dryRun") is true do not apply changes—either call an existing
non-mutating function to compute/report fixes or extend applyRules to accept a
dryRun boolean (e.g., applyRules(dirty, config.rules, { dryRun: true })) and
ensure it only reports the fixes without writing them; reference getFlag,
cliArguments, "fix", "dryRun", applyRules, config.rules and the run function
when making the change.
- Around line 20-26: The CLI currently returns exit code 0 when
checkRules(config) finds violations and --fix is not provided; modify the run
logic in run.ts to emit a non-zero exit when dirty.length > 0 and the fix flag
is not set: log a clear error (e.g., list/count of dirty files) and call
process.exit(1) (or throw an Error) to fail CI. Locate the block using
checkRules, getFlag(cliArguments, "fix"), applyRules and ensure the new
early-return path triggers process.exit(1) when dirty exists and fix is false.
In `@core/monorule/src/commands/list.ts`:
- Around line 72-85: The current JSON branch mutates the live rule objects by
deleting fields on each rule in rules.map; instead create and return a derived
DTO per rule so config.rules is not modified and function-valued fields are
normalized. In the rules.map callback (the anonymous mapper that currently
deletes when/apply/context), construct a new object (e.g., const dto = { ...rule
}) and delete dto.when/dto.apply/dto.context from that DTO; if dto.parse (or
other parser fields) is a function, replace it with a serializable placeholder
or normalized value (e.g., a string) rather than letting JSON.stringify drop it,
then return dto for JSON.stringify.
---
Outside diff comments:
In `@core/monorule/example/rules/hello.ts`:
- Line 3: The import using the internal alias (import { defineRule } from
"`@/src`") is unresolved in the example context; replace it with an import from
the package entry point that is resolvable from this file (i.e., import
defineRule from the published package name or a relative path that reaches the
package root) so the symbol defineRule is imported from a valid module instead
of the "`@/src`" alias.
In `@core/monorule/src/apply.ts`:
- Around line 25-29: When applying fixes, do not unconditionally stringify
outputs from function-based parsers as JSON; detect when currentRule.parse is a
function and refuse/fail fix-mode instead of calling stringify(..., "json").
Update the logic around the stringify call (the use of result and
currentRule.parse in apply.ts) to throw or skip with a clear error message
instructing that function-based parsers need an explicit output stringifier
(e.g., a new currentRule.stringify or outputFormat property) or cannot be
auto-fixed; apply the same guard where stringify is used later (the same pattern
around lines 49-53) so function parsers are not silently reserialized to JSON.
---
Duplicate comments:
In `@core/monorule/example/rules/hello.ts`:
- Around line 36-38: txtRule.when currently returns undefined so checkRules
treats files as clean; update txtRule.when to return actual violation(s) when
the rule matches (e.g., return a non-empty array/object describing the violation
or a boolean true depending on the rule contract) and an empty value when clean.
Locate the txtRule.when function in hello.ts and replace the commented stub with
a real return that checks input (e.g., if !input.includes("stephansama") then
return a violation descriptor like { message: "missing stephansama", location:
... } or an array containing that object) so checkRules sees a violation and
txtRule.apply will run. Ensure the returned shape matches what checkRules
expects.
In `@core/monorule/README.md`:
- Around line 1-16: Replace the top-level HTML block "<div align="center">" and
the centered title with a proper Markdown H1 header (e.g., "#
`@stephansama/monorule`") and adjust the following "##### Table of contents" to a
sensible next-level heading such as "## Table of contents" so heading hierarchy
is correct; ensure any remaining decorative HTML is removed or moved below the
H1 and that subsequent section headings follow a logical H2/H3 sequence instead
of jumping to H5.
In `@core/monorule/src/check.ts`:
- Around line 96-100: Normalize the return value of rule.when(parsed, context)
before assuming it's a RuleError[]: call rule.when and store its result (e.g.,
const result = rule.when(parsed, context)); if result is falsy skip, if
Array.isArray(result) push its elements into checkedErrors, otherwise coerce a
single truthy value into an array (e.g., [result]) before spreading; use the
symbols rule.when, parsed, context, and checkedErrors to locate and update the
code path so non-array truthy returns won't cause .length or spread runtime
errors.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1aa215b6-f77a-457d-abef-911018c85cc0
📒 Files selected for processing (25)
.changeset/cold-ants-occur.mdcore/monorule/README.mdcore/monorule/__snapshots__/tsnapi/cli.snapshot.d.tscore/monorule/__snapshots__/tsnapi/cli.snapshot.jscore/monorule/__snapshots__/tsnapi/index.snapshot.d.tscore/monorule/__snapshots__/tsnapi/index.snapshot.jscore/monorule/example/index.tscore/monorule/example/rules/hello.tscore/monorule/package.jsoncore/monorule/src/apply.tscore/monorule/src/check.tscore/monorule/src/cli/arguments.tscore/monorule/src/cli/flags.tscore/monorule/src/cli/index.tscore/monorule/src/cli/run.tscore/monorule/src/commands/generate.tscore/monorule/src/commands/list.tscore/monorule/src/config.tscore/monorule/src/index.tscore/monorule/src/load.tscore/monorule/src/log.tscore/monorule/src/paths.tscore/monorule/src/type.tscore/monorule/tsconfig.jsoncore/monorule/tsdown.config.ts
Comment on lines
+10
to
+12
| await Promise.all( | ||
| dirtyFiles.map(async (file) => { | ||
| info(`applying ${file.rule} to ${file.relativePath}`); |
There was a problem hiding this comment.
Serialize fixes per file instead of writing them concurrently.
DirtyFile.rule in core/monorule/src/type.ts is the violated rule identifier, so the same absolutePath can appear multiple times when one file fails multiple rules. Promise.all then writes competing outputs to the same path in parallel, and the last writer wins. Group by file.absolutePath and apply that file’s fixes sequentially, reparsing between steps if later rules depend on earlier mutations.
Also applies to: 32-56
🤖 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 `@core/monorule/src/apply.ts` around lines 10 - 12, The code currently uses
Promise.all over dirtyFiles which can run multiple fixes concurrently against
the same file; group dirtyFiles by file.absolutePath so each unique path's
entries are processed sequentially (e.g., build a map from absolutePath to array
of DirtyFile entries), then for each group iterate with a for...of and apply
rules one-by-one (reparsing the file state between each rule application)
instead of running them in parallel; use the DirtyFile.rule identifier and the
existing apply logic to ensure you serialize writes per absolutePath and avoid
concurrent writes to the same file.
Comment on lines
+20
to
+26
| const dirty = await checkRules(config); | ||
|
|
||
| if (dirty.length === 0) return console.info("no files to change"); | ||
|
|
||
| if (getFlag(cliArguments, "fix")) { | ||
| await applyRules(dirty, config.rules); | ||
| } |
There was a problem hiding this comment.
Return a failing exit code when violations are found.
When checkRules(config) returns dirty files and --fix is not set, this function falls off the end with exit code 0. That makes CI/hooks pass even though the linter found violations.
Suggested fix
const dirty = await checkRules(config);
if (dirty.length === 0) return console.info("no files to change");
if (getFlag(cliArguments, "fix")) {
await applyRules(dirty, config.rules);
+ return;
}
+
+ process.exitCode = 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const dirty = await checkRules(config); | |
| if (dirty.length === 0) return console.info("no files to change"); | |
| if (getFlag(cliArguments, "fix")) { | |
| await applyRules(dirty, config.rules); | |
| } | |
| const dirty = await checkRules(config); | |
| if (dirty.length === 0) return console.info("no files to change"); | |
| if (getFlag(cliArguments, "fix")) { | |
| await applyRules(dirty, config.rules); | |
| return; | |
| } | |
| process.exitCode = 1; |
🤖 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 `@core/monorule/src/cli/run.ts` around lines 20 - 26, The CLI currently returns
exit code 0 when checkRules(config) finds violations and --fix is not provided;
modify the run logic in run.ts to emit a non-zero exit when dirty.length > 0 and
the fix flag is not set: log a clear error (e.g., list/count of dirty files) and
call process.exit(1) (or throw an Error) to fail CI. Locate the block using
checkRules, getFlag(cliArguments, "fix"), applyRules and ensure the new
early-return path triggers process.exit(1) when dirty exists and fix is false.
Comment on lines
+24
to
+25
| if (getFlag(cliArguments, "fix")) { | ||
| await applyRules(dirty, config.rules); |
There was a problem hiding this comment.
--dryRun is currently a no-op.
core/monorule/src/cli/arguments.ts declares dryRun with user-facing behavior, but run() never reads that flag. Right now --dryRun is indistinguishable from the normal non---fix path, so the advertised CLI contract is not implemented.
🤖 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 `@core/monorule/src/cli/run.ts` around lines 24 - 25, The CLI currently ignores
the dryRun flag; update run() to read getFlag(cliArguments, "dryRun") and branch
accordingly: when getFlag(..., "fix") is true call applyRules(dirty,
config.rules) as before, but when getFlag(..., "dryRun") is true do not apply
changes—either call an existing non-mutating function to compute/report fixes or
extend applyRules to accept a dryRun boolean (e.g., applyRules(dirty,
config.rules, { dryRun: true })) and ensure it only reports the fixes without
writing them; reference getFlag, cliArguments, "fix", "dryRun", applyRules,
config.rules and the run function when making the change.
Comment on lines
+72
to
+85
| if (json) { | ||
| return console.info( | ||
| JSON.stringify( | ||
| rules.map((rule) => { | ||
| // @ts-expect-error need to delete anyway | ||
| delete rule.when; | ||
| delete rule.apply; | ||
| delete rule.context; | ||
| return rule; | ||
| }), | ||
| undefined, | ||
| 2, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Serialize a derived rule DTO instead of the live rule object.
This branch mutates config.rules with delete, and any rule whose parse is a function will lose that field entirely because JSON.stringify drops functions. The text path already normalizes function values at Lines 93-95, so list --json currently emits incomplete rule data for custom parsers.
Suggested fix
if (json) {
return console.info(
JSON.stringify(
- rules.map((rule) => {
- // `@ts-expect-error` need to delete anyway
- delete rule.when;
- delete rule.apply;
- delete rule.context;
- return rule;
- }),
+ rules.map(({ when, apply, context, ...rule }) => ({
+ ...rule,
+ parse:
+ typeof rule.parse === "function" ? "function" : rule.parse,
+ })),
undefined,
2,
),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (json) { | |
| return console.info( | |
| JSON.stringify( | |
| rules.map((rule) => { | |
| // @ts-expect-error need to delete anyway | |
| delete rule.when; | |
| delete rule.apply; | |
| delete rule.context; | |
| return rule; | |
| }), | |
| undefined, | |
| 2, | |
| ), | |
| ); | |
| if (json) { | |
| return console.info( | |
| JSON.stringify( | |
| rules.map(({ when, apply, context, ...rule }) => ({ | |
| ...rule, | |
| parse: | |
| typeof rule.parse === "function" ? "function" : rule.parse, | |
| })), | |
| undefined, | |
| 2, | |
| ), | |
| ); | |
| } |
🤖 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 `@core/monorule/src/commands/list.ts` around lines 72 - 85, The current JSON
branch mutates the live rule objects by deleting fields on each rule in
rules.map; instead create and return a derived DTO per rule so config.rules is
not modified and function-valued fields are normalized. In the rules.map
callback (the anonymous mapper that currently deletes when/apply/context),
construct a new object (e.g., const dto = { ...rule }) and delete
dto.when/dto.apply/dto.context from that DTO; if dto.parse (or other parser
fields) is a function, replace it with a serializable placeholder or normalized
value (e.g., a string) rather than letting JSON.stringify drop it, then return
dto for JSON.stringify.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/monorule/example/index.ts (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
zodtodependencies(or remove runtimezodimports from the shipped examples).
core/monorule/example/index.tsimportszod, and the same runtime import pattern exists incore/monorule/example/rules/hello.ts. However,core/monorule/package.jsondeclareszodonly underdevDependencies("zod": "catalog:schema") and not underdependencies, even though the package ships theexample/directory ("files": ["dist", "example"]). Addzodtodependencies, or rewrite the examples to avoid runtimezodimports (e.g., inline schema / type-only usage where possible).import * as z from "zod"; import { defineConfig, defineRule, parsers } from "../dist/index.mjs";🤖 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 `@core/monorule/example/index.ts` around lines 1 - 3, The example files import zod at runtime (e.g., the `import * as z from "zod";` lines in core/monorule/example/index.ts and core/monorule/example/rules/hello.ts) while core/monorule/package.json lists zod only as a devDependency; either add zod to package.json "dependencies" or remove the runtime import by switching to type-only usage or inlining validation. Concretely: if you want examples to ship without adding a runtime dependency, change runtime `import * as z from "zod"` to type-only imports (e.g., `import type { ZodSchema } from "zod"`) and rewrite any runtime schema validation in those example modules (used by `defineConfig`, `defineRule`, `parsers`) to be pure types or simple inline checks; otherwise add `"zod": \"catalog:schema\"` (or the appropriate version) to core/monorule/package.json dependencies so the shipped example imports resolve at runtime.core/monorule/README.md (1)
27-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the
zoddependency used in the usage example.Line 34 imports
zod, but the Installation section only documents@stephansama/monorule. Addzodto the install instructions or use only built-in parsers in the example.🤖 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 `@core/monorule/README.md` around lines 27 - 37, The README usage imports zod (import * as z from "zod") but the install instructions only list `@stephansama/monorule`; update the installation line so it installs zod as well (e.g., add zod to the pnpm install command) or alternatively remove the zod import and adjust the example to use only built-in parsers (references: the import line for zod and the usage of defineConfig, defineRule, parsers) so the example and the Installation section remain consistent.core/monorule/package.json (1)
70-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLower
engines.noderequirement (currently>=24) to avoid unnecessary user restriction.
core/monorule’s source/build output imports only standard built-ins (node:fs,node:path,node:url,node:tty,node:util) and does not reference Node-24+ specific APIs (e.g.,node:test,node:timers/promises,node:stream/web,node:worker_threads). If there’s no other dependency/runtime requirement, setengines.nodeto>=18(or>=20) instead of>=24.🤖 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 `@core/monorule/package.json` at line 70, The package.json in core/monorule currently pins engines.node to ">=24", which needlessly restricts users; update the "engines.node" field in core/monorule's package.json (the engines.node entry) to a lower version such as ">=18" or ">=20" after confirming no runtime APIs require Node 24+ (the package only imports node:fs, node:path, node:url, node:tty, node:util). Change the value of the engines.node property accordingly and run a quick local install/build to verify compatibility.
♻️ Duplicate comments (2)
core/monorule/src/schema.ts (1)
13-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain
parse/whenfunction contracts in schema validation.With unconstrained
z.function()on Line 13 and Line 14, invalid rule signatures pass schema validation, butcheck.tsconsumeswhenasArray<Error> | voidand can break at runtime.Proposed fix
export const ruleSchema = z.object({ apply: z.function().optional(), enabled: z.boolean().default(true), exclude: z.array(z.string().trim()).or(z.string().trim()).optional(), include: z.union([z.array(z.string().trim()), z.string().trim()]), name: z.string().trim(), - parse: z.function().or(builtinParseEnumSchema), - when: z.function(), + parse: z + .function({ + input: [z.string()], + output: z.unknown(), + }) + .or(builtinParseEnumSchema), + when: z.function({ + input: [z.unknown(), z.unknown().optional()], + output: z.union([ + z.array( + z.object({ + id: z.string(), + message: z.string(), + fixable: z.boolean().optional(), + }), + ), + z.void(), + ]), + }), });#!/bin/bash # Verify runtime expectations vs schema function constraints rg -nP 'parse\(raw,\s*rule\.parse\)|rule\.when\(|errors\.length|push\(\.\.\.errors\)' core/monorule/src/check.ts rg -nP 'parse:\s*z\.function|when:\s*z\.function' core/monorule/src/schema.ts🤖 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 `@core/monorule/src/schema.ts` around lines 13 - 14, The schema's parse and when are currently unconstrained z.function() which allows invalid signatures; update both in schema.ts to explicitly require a function taking (raw: unknown) and returning Array<Error> | void so runtime consumers in check.ts (which treat rule.when/parse results as Array<Error> | void) don't break: replace parse: z.function() and when: z.function() with e.g. z.function().args(z.unknown()).returns(z.union([z.array(z.instanceof(Error)), z.undefined()])), ensuring the function signature matches what check.ts expects.core/monorule/src/rules/load.ts (1)
15-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways filter import candidates and guarantee
unregister()viafinally.Line 15 reads all directory entries (files + directories), and any throw in Lines 19-25 currently bypasses cleanup because
unregister()is outside afinally. One bad entry can fail rule loading and leave thetsxhook registered.Suggested fix
- const files = await fs.promises.readdir(absolute); + const entries = await fs.promises.readdir(absolute, { withFileTypes: true }); + const files = entries + .filter((entry) => entry.isFile()) + .map((entry) => entry.name) + .filter((file) => + [".js", ".mjs", ".cjs", ".ts", ".mts", ".cts"].includes( + path.extname(file), + ), + ); const unregister = register(); - const loadedRules = await Promise.all( - files.map(async (file) => { - info(`loading ${file}`); - // convert to object to remove module namespace key - const imported = (await import(path.join(absolute, file))) as {}; - return Object.values(ruleMapSchema.parse({ ...imported })); - }), - ); - - await unregister(); - - return loadedRules.flat(); + try { + const loadedRules = await Promise.all( + files.map(async (file) => { + info(`loading ${file}`); + // convert to object to remove module namespace key + const imported = (await import(path.join(absolute, file))) as {}; + return Object.values(ruleMapSchema.parse({ ...imported })); + }), + ); + + return loadedRules.flat(); + } finally { + await unregister(); + }#!/bin/bash # Verify the current implementation reads all entries and does not protect unregister with finally. rg -n -C3 'readdir\(|register\(|unregister\(|Promise\.all|import\(' core/monorule/src/rules/load.ts🤖 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 `@core/monorule/src/rules/load.ts` around lines 15 - 29, The code currently reads all directory entries and registers a hook but may skip cleanup if an import throws; change readdir to use fs.promises.readdir(absolute, { withFileTypes: true }) and filter to regular files with allowed extensions (e.g., .js/.cjs/.mjs/.ts/.tsx) before mapping, so only valid import candidates are passed to import(path.join(absolute, file)). Move the unregister() call into a finally block that surrounds the await Promise.all(...) so register()/unregister() are always paired, and inside the Promise.all mapping catch and handle per-file import/parse errors (so a single bad module doesn't reject the whole Promise.all) while still returning parsed rule objects to be flattened via ruleMapSchema.parse and Object.values.
🤖 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 `@core/monorule/package.json`:
- Around line 36-37: Remove the TS-only example from the published package by
deleting "example" from the package.json "files" array (leaving only "dist"), or
alternatively replace it with a documented JS/docs build of the examples; update
package.json to remove "example" from "files" and, if those examples are still
needed for local dev scripts (cli:config, cli:list, cli:rules), keep them in
repo but exclude them from publishing or add a build step that emits a
consumer-ready JS/docs copy to be published instead.
In `@core/monorule/src/commands/generate.ts`:
- Around line 34-39: The exported helper fileExists is unused and its sole call
is incorrect; either remove the export (make fileExists internal) or delete the
function entirely and fix any remaining references in the generate command
implementation (remove or replace the incorrect call). Locate the fileExists
function and either change "export async function fileExists" to a non-exported
local function or remove the function and ensure generate (and any other code)
no longer relies on it.
- Around line 11-24: The flags object defines two flags, config and
ruleDirectory, that currently both use the alias "c" and the ruleDirectory
description is wrong; update ruleDirectory to use a unique alias (e.g., alias:
"r") and change its description to something like "rules directory location" (or
"rules directory") so it no longer conflicts with config and accurately
describes the flag; keep the existing defaults and types and only modify the
alias and description for the ruleDirectory entry.
- Around line 28-32: In act (function act) remove the incorrect
fileExists(arguments_.flags.help + "") call and replace it with the real
behavior: either remove the placeholder entirely or check a proper path flag
(e.g., arguments_.flags.path or arguments_.flags.out) if such exists; if the
command should generate defaults, implement the TODOs to create a default config
and rules directory (use fileExists only when validating a real path flag) and
ensure you reference the correct flag name instead of arguments_.flags.help
(help is a boolean from Cleye).
- Around line 8-9: Remove the unused constant _dirname (declared as const
_dirname = path.dirname(url.fileURLToPath(import.meta.url))); from generate.ts;
delete that line and, if doing so renders the imported symbols path or url
unused in this module, remove their imports as well to keep the file free of
unused variables/imports.
In `@core/monorule/src/parse.ts`:
- Around line 10-12: The txt parser currently uses a blind TypeScript cast
("txt: (input) => input as string") which can leak non-string values later;
change the txt handler in the builtins map to actually convert/validate input:
if typeof input === "string" return it, if input == null return empty string (or
a safe default), otherwise return a deterministic string representation (e.g.,
JSON.stringify(input) or String(input)) so non-string rule outputs don't break
the apply pipeline — update the txt entry in the same map (the
BuiltinParseEnumSchema record) accordingly while preserving the function
signature.
In `@core/monorule/src/rules/type.ts`:
- Around line 3-4: The current Rule type uses ReturnType<typeof defineRule>,
which collapses to the last overload and drops the FunctionParserRule shape;
change Rule to explicitly represent both overload returns (e.g. export type Rule
= FunctionParserRule | DefaultParserRule) and update RuleMap accordingly so maps
accept rules that implement either the functional parse signature (parse(input)
{...}) or the default parser shape; reference the defineRule overloads and the
FunctionParserRule and DefaultParserRule types when making the change.
In `@core/monorule/src/type.ts`:
- Around line 15-16: DefaultRuleParseTypeExtract currently maps non-"txt" parse
types to object, which is too narrow for parsers that can return primitives
(e.g., JSON/YAML); change the mapped type to a wider safe type (e.g., unknown)
so callbacks like when and apply receive a correct, non-unsafe type. Update the
type alias DefaultRuleParseTypeExtract<T extends BuiltinParseEnumSchema> to use
unknown (or a union of primitive|object) for the non-"txt" branch and adjust any
dependent signatures (when/apply) to accept that widened type so the compiler no
longer assumes object for non-txt parsers.
---
Outside diff comments:
In `@core/monorule/example/index.ts`:
- Around line 1-3: The example files import zod at runtime (e.g., the `import *
as z from "zod";` lines in core/monorule/example/index.ts and
core/monorule/example/rules/hello.ts) while core/monorule/package.json lists zod
only as a devDependency; either add zod to package.json "dependencies" or remove
the runtime import by switching to type-only usage or inlining validation.
Concretely: if you want examples to ship without adding a runtime dependency,
change runtime `import * as z from "zod"` to type-only imports (e.g., `import
type { ZodSchema } from "zod"`) and rewrite any runtime schema validation in
those example modules (used by `defineConfig`, `defineRule`, `parsers`) to be
pure types or simple inline checks; otherwise add `"zod": \"catalog:schema\"`
(or the appropriate version) to core/monorule/package.json dependencies so the
shipped example imports resolve at runtime.
In `@core/monorule/package.json`:
- Line 70: The package.json in core/monorule currently pins engines.node to
">=24", which needlessly restricts users; update the "engines.node" field in
core/monorule's package.json (the engines.node entry) to a lower version such as
">=18" or ">=20" after confirming no runtime APIs require Node 24+ (the package
only imports node:fs, node:path, node:url, node:tty, node:util). Change the
value of the engines.node property accordingly and run a quick local
install/build to verify compatibility.
In `@core/monorule/README.md`:
- Around line 27-37: The README usage imports zod (import * as z from "zod") but
the install instructions only list `@stephansama/monorule`; update the
installation line so it installs zod as well (e.g., add zod to the pnpm install
command) or alternatively remove the zod import and adjust the example to use
only built-in parsers (references: the import line for zod and the usage of
defineConfig, defineRule, parsers) so the example and the Installation section
remain consistent.
---
Duplicate comments:
In `@core/monorule/src/rules/load.ts`:
- Around line 15-29: The code currently reads all directory entries and
registers a hook but may skip cleanup if an import throws; change readdir to use
fs.promises.readdir(absolute, { withFileTypes: true }) and filter to regular
files with allowed extensions (e.g., .js/.cjs/.mjs/.ts/.tsx) before mapping, so
only valid import candidates are passed to import(path.join(absolute, file)).
Move the unregister() call into a finally block that surrounds the await
Promise.all(...) so register()/unregister() are always paired, and inside the
Promise.all mapping catch and handle per-file import/parse errors (so a single
bad module doesn't reject the whole Promise.all) while still returning parsed
rule objects to be flattened via ruleMapSchema.parse and Object.values.
In `@core/monorule/src/schema.ts`:
- Around line 13-14: The schema's parse and when are currently unconstrained
z.function() which allows invalid signatures; update both in schema.ts to
explicitly require a function taking (raw: unknown) and returning Array<Error> |
void so runtime consumers in check.ts (which treat rule.when/parse results as
Array<Error> | void) don't break: replace parse: z.function() and when:
z.function() with e.g.
z.function().args(z.unknown()).returns(z.union([z.array(z.instanceof(Error)),
z.undefined()])), ensuring the function signature matches what check.ts expects.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1e7d592-342d-4c48-8fce-94c629cf272e
📒 Files selected for processing (22)
core/monorule/README.mdcore/monorule/__snapshots__/tsnapi/index.snapshot.d.tscore/monorule/__snapshots__/tsnapi/index.snapshot.jscore/monorule/__snapshots__/tsnapi/schema.snapshot.d.tscore/monorule/__snapshots__/tsnapi/schema.snapshot.jscore/monorule/example/index.tscore/monorule/example/rules/hello.tscore/monorule/package.jsoncore/monorule/src/apply.tscore/monorule/src/check.tscore/monorule/src/commands/generate.tscore/monorule/src/commands/list.tscore/monorule/src/config.tscore/monorule/src/index.tscore/monorule/src/parse.tscore/monorule/src/paths.tscore/monorule/src/rules/define.tscore/monorule/src/rules/index.tscore/monorule/src/rules/load.tscore/monorule/src/rules/type.tscore/monorule/src/schema.tscore/monorule/src/type.ts
Comment on lines
+8
to
+9
| const _dirname = path.dirname(url.fileURLToPath(import.meta.url)); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove unused _dirname variable.
The _dirname variable is declared but never referenced in this file.
♻️ Proposed fix
-const _dirname = path.dirname(url.fileURLToPath(import.meta.url));
-
export const meta = command({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const _dirname = path.dirname(url.fileURLToPath(import.meta.url)); |
🤖 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 `@core/monorule/src/commands/generate.ts` around lines 8 - 9, Remove the unused
constant _dirname (declared as const _dirname =
path.dirname(url.fileURLToPath(import.meta.url))); from generate.ts; delete that
line and, if doing so renders the imported symbols path or url unused in this
module, remove their imports as well to keep the file free of unused
variables/imports.
Comment on lines
+11
to
+24
| flags: { | ||
| config: { | ||
| alias: "c", | ||
| default: "monorule.config.ts", | ||
| description: "config file location", | ||
| type: String, | ||
| }, | ||
| ruleDirectory: { | ||
| alias: "c", | ||
| default: "rules", | ||
| description: "config file location", | ||
| type: String, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix duplicate flag alias and incorrect description.
Both config and ruleDirectory flags use alias "c", which creates a conflict. Additionally, Line 21 incorrectly describes ruleDirectory as "config file location" instead of describing the rules directory.
🐛 Proposed fix
flags: {
config: {
alias: "c",
default: "monorule.config.ts",
description: "config file location",
type: String,
},
ruleDirectory: {
- alias: "c",
+ alias: "r",
default: "rules",
- description: "config file location",
+ description: "rules directory location",
type: String,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| flags: { | |
| config: { | |
| alias: "c", | |
| default: "monorule.config.ts", | |
| description: "config file location", | |
| type: String, | |
| }, | |
| ruleDirectory: { | |
| alias: "c", | |
| default: "rules", | |
| description: "config file location", | |
| type: String, | |
| }, | |
| }, | |
| flags: { | |
| config: { | |
| alias: "c", | |
| default: "monorule.config.ts", | |
| description: "config file location", | |
| type: String, | |
| }, | |
| ruleDirectory: { | |
| alias: "r", | |
| default: "rules", | |
| description: "rules directory location", | |
| type: String, | |
| }, | |
| }, |
🤖 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 `@core/monorule/src/commands/generate.ts` around lines 11 - 24, The flags
object defines two flags, config and ruleDirectory, that currently both use the
alias "c" and the ruleDirectory description is wrong; update ruleDirectory to
use a unique alias (e.g., alias: "r") and change its description to something
like "rules directory location" (or "rules directory") so it no longer conflicts
with config and accurately describes the flag; keep the existing defaults and
types and only modify the alias and description for the ruleDirectory entry.
Comment on lines
+28
to
+32
| export async function act(arguments_: CliArguments) { | ||
| await fileExists(arguments_.flags.help + ""); | ||
| // TODO: generate default configuration | ||
| // TODO: generate rules directory | ||
| } |
There was a problem hiding this comment.
Incorrect usage of arguments_.flags.help as a file path.
Line 29 calls fileExists(arguments_.flags.help + ""), but help is a boolean flag from Cleye, so this will check for a file named "true" or "false" rather than a meaningful path. This appears to be placeholder code that should be removed or replaced with the actual implementation logic.
🐛 Proposed fix to remove the incorrect call
export async function act(arguments_: CliArguments) {
- await fileExists(arguments_.flags.help + "");
// TODO: generate default configuration
// TODO: generate rules directory
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function act(arguments_: CliArguments) { | |
| await fileExists(arguments_.flags.help + ""); | |
| // TODO: generate default configuration | |
| // TODO: generate rules directory | |
| } | |
| export async function act(arguments_: CliArguments) { | |
| // TODO: generate default configuration | |
| // TODO: generate rules directory | |
| } |
🤖 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 `@core/monorule/src/commands/generate.ts` around lines 28 - 32, In act
(function act) remove the incorrect fileExists(arguments_.flags.help + "") call
and replace it with the real behavior: either remove the placeholder entirely or
check a proper path flag (e.g., arguments_.flags.path or arguments_.flags.out)
if such exists; if the command should generate defaults, implement the TODOs to
create a default config and rules directory (use fileExists only when validating
a real path flag) and ensure you reference the correct flag name instead of
arguments_.flags.help (help is a boolean from Cleye).
Comment on lines
+34
to
+39
| export async function fileExists(file: string) { | ||
| return await fs.promises | ||
| .access(file) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider removing or internalizing fileExists helper.
The fileExists helper is exported but not used anywhere in the codebase (its only call on Line 29 is incorrect). If this helper is not needed for the public API, consider removing the export keyword or removing the function entirely until the generate command is implemented.
🤖 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 `@core/monorule/src/commands/generate.ts` around lines 34 - 39, The exported
helper fileExists is unused and its sole call is incorrect; either remove the
export (make fileExists internal) or delete the function entirely and fix any
remaining references in the generate command implementation (remove or replace
the incorrect call). Locate the fileExists function and either change "export
async function fileExists" to a non-exported local function or remove the
function and ensure generate (and any other code) no longer relies on it.
Comment on lines
+10
to
+12
| txt: (input) => input as string, | ||
| yaml: yaml.stringify, | ||
| } as const satisfies Record<BuiltinParseEnumSchema, (input: unknown) => string>; |
There was a problem hiding this comment.
Validate txt stringify input instead of blind casting.
Line 10 uses a TypeScript cast, not conversion. If a rule returns a non-string for txt, this can surface later as a write failure in the apply pipeline.
Proposed fix
export const stringifiers = {
json: JSON.stringify,
toml: toml.stringify,
- txt: (input) => input as string,
+ txt: (input) => {
+ if (typeof input !== "string") {
+ throw new TypeError("txt stringify expects a string input");
+ }
+ return input;
+ },
yaml: yaml.stringify,
} as const satisfies Record<BuiltinParseEnumSchema, (input: unknown) => string>;🤖 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 `@core/monorule/src/parse.ts` around lines 10 - 12, The txt parser currently
uses a blind TypeScript cast ("txt: (input) => input as string") which can leak
non-string values later; change the txt handler in the builtins map to actually
convert/validate input: if typeof input === "string" return it, if input == null
return empty string (or a safe default), otherwise return a deterministic string
representation (e.g., JSON.stringify(input) or String(input)) so non-string rule
outputs don't break the apply pipeline — update the txt entry in the same map
(the BuiltinParseEnumSchema record) accordingly while preserving the function
signature.
Comment on lines
+15
to
+16
| export type DefaultRuleParseTypeExtract<T extends BuiltinParseEnumSchema> = | ||
| T extends "txt" ? string : object; |
There was a problem hiding this comment.
Widen non-text parse output typing to avoid unsafe assumptions.
Line 16 forces every non-"txt" parse result to object, but built-in parsers (notably JSON/YAML) can return primitives. That makes when/apply callbacks type-safe on paper and unsafe at runtime.
Proposed fix
export type DefaultRuleParseTypeExtract<T extends BuiltinParseEnumSchema> =
- T extends "txt" ? string : object;
+ T extends "txt" ? string : unknown;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export type DefaultRuleParseTypeExtract<T extends BuiltinParseEnumSchema> = | |
| T extends "txt" ? string : object; | |
| export type DefaultRuleParseTypeExtract<T extends BuiltinParseEnumSchema> = | |
| T extends "txt" ? string : unknown; |
🤖 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 `@core/monorule/src/type.ts` around lines 15 - 16, DefaultRuleParseTypeExtract
currently maps non-"txt" parse types to object, which is too narrow for parsers
that can return primitives (e.g., JSON/YAML); change the mapped type to a wider
safe type (e.g., unknown) so callbacks like when and apply receive a correct,
non-unsafe type. Update the type alias DefaultRuleParseTypeExtract<T extends
BuiltinParseEnumSchema> to use unknown (or a union of primitive|object) for the
non-"txt" branch and adjust any dependent signatures (when/apply) to accept that
widened type so the compiler no longer assumes object for non-txt parsers.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
mainhave been mergedmain