Add max-params checker and aggregate checks runner; wire into lint pipeline#2445
Open
andrew-bierman wants to merge 1 commit into
Open
Add max-params checker and aggregate checks runner; wire into lint pipeline#2445andrew-bierman wants to merge 1 commit into
andrew-bierman wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR introduces a multi-parameter function checker that scans the codebase for functions with more than one parameter and enforces a single-parameter (typed object) pattern. The check is wired into the root ChangesMax-params check system
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new check-max-params script that flags any function with more than one parameter (with an @allow-multi-param opt-out), plus a check-all aggregator that runs all custom checks, and wires the aggregator into lint:custom.
Changes:
- New
packages/checks/src/check-max-params.tswalkingapps/andpackages/for.ts(x)files and exiting non-zero on multi-param functions. - New
packages/checks/src/check-all.tsrunner that sequentially shells out tocheck:casts,check:magic-strings, andcheck:max-params. - Adds
check,check:max-params, andcheck:customscripts, and appendsbun run check:customto the rootlint:custom.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/checks/src/check-max-params.ts |
New AST-based scanner that uses ts.isFunctionLike and reports functions with parameters.length > 1. |
packages/checks/src/check-all.ts |
Sequential runner over the three individual check scripts via Bun.spawnSync. |
packages/checks/package.json |
Registers check:max-params and aggregate check. |
package.json |
Adds check:max-params / check:custom (appended out of alphabetical order) and chains check:custom into lint:custom. |
Comments suppressed due to low confidence (1)
packages/checks/src/check-max-params.ts:87
ts.isFunctionLikereturns true for type-only constructs such asFunctionType,MethodSignature,CallSignature,ConstructSignature, andIndexSignaturenodes. That means TypeScript type/interface declarations liketype Handler = (req: Request, res: Response) => voidorinterface Foo { bar(a: number, b: number): void }will be reported as violations even though they are purely type-level and cannot be rewritten as a single object parameter without changing the public type contract. Consider narrowing the check to actual implementation nodes (e.g.,FunctionDeclaration,FunctionExpression,ArrowFunction,MethodDeclaration,ConstructorDeclaration,GetAccessor/SetAccessor) and skipping signature-only nodes.
if (
ts.isFunctionLike(node) &&
node.parameters.length > 1 &&
!hasAllowAnnotation(source, node) &&
!isCallbackForInvocation(node)
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "lefthook": "lefthook install", | ||
| "lint": "biome check --write", | ||
| "lint:custom": "bun run scripts/lint/no-raw-typeof.ts && bun run scripts/lint/no-raw-regex.ts && bun run packages/env/scripts/no-raw-process-env.ts && bun run scripts/lint/no-duplicate-guards.ts && bun run scripts/lint/no-unauth-routes.ts", | ||
| "lint:custom": "bun run scripts/lint/no-raw-typeof.ts && bun run scripts/lint/no-raw-regex.ts && bun run packages/env/scripts/no-raw-process-env.ts && bun run scripts/lint/no-duplicate-guards.ts && bun run scripts/lint/no-unauth-routes.ts && bun run check:custom", |
Comment on lines
+52
to
+54
| "web": "bun run --cwd apps/web dev", | ||
| "check:max-params": "bun run --cwd packages/checks check:max-params", | ||
| "check:custom": "bun run --cwd packages/checks check" |
Comment on lines
+59
to
+63
| function isCallbackForInvocation(node: ts.Node): boolean { | ||
| const parent = node.parent; | ||
| if (!parent || !ts.isCallExpression(parent)) return false; | ||
| return parent.arguments.some((arg) => arg === node); | ||
| } |
Comment on lines
+3
to
+14
| const checks = [ | ||
| { name: 'check:casts', cmd: ['bun', './check-type-casts.ts'] }, | ||
| { name: 'check:magic-strings', cmd: ['bun', './check-magic-strings.ts'] }, | ||
| { name: 'check:max-params', cmd: ['bun', './check-max-params.ts'] }, | ||
| ]; | ||
|
|
||
| let hasFailure = false; | ||
|
|
||
| for (const check of checks) { | ||
| console.log(`\n▶ Running ${check.name}`); | ||
| const proc = Bun.spawnSync(check.cmd, { | ||
| cwd: import.meta.dir, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/checks/src/check-max-params.ts`:
- Around line 21-32: The exclusion regexes in EXCLUDED_FILE_PATTERNS will fail
on Windows because they expect forward slashes; update isTargetFile to normalize
incoming file paths (e.g., use path.normalize or replace all backslashes with
'/') and convert path separators to forward slashes before testing against
EXCLUDED_FILE_PATTERNS so patterns like /\/node_modules\// match on Windows as
well.
- Around line 76-105: In collectViolations replace the brittle string replace of
filePath (filePath.replace(`${ROOT}/`, '')) with a robust relative path using
path.relative(ROOT, filePath); ensure you import/require path if not present and
use that value when setting the violation.file property so ROOT, filePath and
collectViolations are referenced and path normalization is handled
cross-platform.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f39c0ec8-29d3-422d-a261-23813d1b90a4
📒 Files selected for processing (4)
package.jsonpackages/checks/package.jsonpackages/checks/src/check-all.tspackages/checks/src/check-max-params.ts
Comment on lines
+21
to
+32
| const EXCLUDED_FILE_PATTERNS = [ | ||
| /\.test\./, | ||
| /\.spec\./, | ||
| /\.stories\./, | ||
| /\.d\.ts$/, | ||
| /\/__tests__\//, | ||
| /\/test\//, | ||
| /\/tests\//, | ||
| /\/node_modules\//, | ||
| /\/dist\//, | ||
| /\/build\//, | ||
| ]; |
Contributor
There was a problem hiding this comment.
Path patterns may not work correctly on Windows.
The regex patterns use forward slashes (e.g., /\/node_modules\//), but on Windows, file paths use backslashes. This could cause the exclusion patterns to fail, leading to unnecessary scanning or false positives. Normalize paths to use forward slashes before testing against these patterns.
🔧 Suggested fix
Update isTargetFile to normalize the path separator:
function isTargetFile(filePath: string): boolean {
if (!TARGET_EXTENSIONS.has(extname(filePath))) return false;
- return !EXCLUDED_FILE_PATTERNS.some((p) => p.test(filePath));
+ const normalized = filePath.replace(/\\/g, '/');
+ return !EXCLUDED_FILE_PATTERNS.some((p) => p.test(normalized));
}🤖 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/checks/src/check-max-params.ts` around lines 21 - 32, The exclusion
regexes in EXCLUDED_FILE_PATTERNS will fail on Windows because they expect
forward slashes; update isTargetFile to normalize incoming file paths (e.g., use
path.normalize or replace all backslashes with '/') and convert path separators
to forward slashes before testing against EXCLUDED_FILE_PATTERNS so patterns
like /\/node_modules\// match on Windows as well.
Comment on lines
+76
to
+105
| function collectViolations(filePath: string): Violation[] { | ||
| const code = readFileSync(filePath, 'utf8'); | ||
| const source = ts.createSourceFile(filePath, code, ts.ScriptTarget.Latest, true); | ||
| const violations: Violation[] = []; | ||
|
|
||
| const visit = (node: ts.Node): void => { | ||
| if ( | ||
| ts.isFunctionLike(node) && | ||
| node.parameters.length > 1 && | ||
| !hasAllowAnnotation(source, node) && | ||
| !isCallbackForInvocation(node) | ||
| ) { | ||
| const start = source.getLineAndCharacterOfPosition(node.getStart()); | ||
| const end = Math.min(node.getEnd(), node.getStart() + 120); | ||
| const snippet = source.getText().slice(node.getStart(), end).replace(/\s+/g, ' ').trim(); | ||
| violations.push({ | ||
| file: filePath.replace(`${ROOT}/`, ''), | ||
| line: start.line + 1, | ||
| col: start.character + 1, | ||
| params: node.parameters.length, | ||
| kind: getKindLabel(node), | ||
| snippet, | ||
| }); | ||
| } | ||
| ts.forEachChild(node, visit); | ||
| }; | ||
|
|
||
| visit(source); | ||
| return violations; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use path.relative for more robust path handling.
Line 92 uses string replacement to make paths relative (filePath.replace(\${ROOT}/`, '')). This approach can fail if there are path normalization differences (e.g., symbolic links, double slashes, or case differences on case-insensitive filesystems). Use path.relative(ROOT, filePath)` instead for robust, cross-platform path relativization.
♻️ Refactor with path.relative
+import { extname, join, relative } from 'node:path';
function collectViolations(filePath: string): Violation[] {
// ...
violations.push({
- file: filePath.replace(`${ROOT}/`, ''),
+ file: relative(ROOT, filePath),
line: start.line + 1,
col: start.character + 1,
params: node.parameters.length,
kind: getKindLabel(node),
snippet,
});🤖 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/checks/src/check-max-params.ts` around lines 76 - 105, In
collectViolations replace the brittle string replace of filePath
(filePath.replace(`${ROOT}/`, '')) with a robust relative path using
path.relative(ROOT, filePath); ensure you import/require path if not present and
use that value when setting the violation.file property so ROOT, filePath and
collectViolations are referenced and path normalization is handled
cross-platform.
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
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.
Motivation
lint:customincludes the new checks.Description
packages/checks/src/check-max-params.ts, a TypeScript script that scansappsandpackagesfor*.ts/*.tsxfiles (excluding tests, build dirs, and node_modules) and reports functions with more than one parameter while allowing exceptions via a leading@allow-multi-paramcomment, exiting non-zero on violations.packages/checks/src/check-all.ts, a small runner that executes the individual check scripts (check:casts,check:magic-strings,check:max-params) and exits non-zero if any check fails.packages/checks/package.jsonto includecheck:max-paramsand acheckscript that runs the aggregated checks, and update the repo rootpackage.jsonto addcheck:max-params, acheck:customshortcut wired topackages/checks, and includebun run check:custominlint:customso the checks run with the lint task.Testing
Codex Task
Summary by CodeRabbit
New Features
Chores