feat(lint): enforce kebab-case filenames with ESLint#4797
feat(lint): enforce kebab-case filenames with ESLint#4797ZijianZhang989 wants to merge 1 commit into
Conversation
📋 Review SummaryThis PR adds an ESLint rule to enforce kebab-case filenames for TypeScript files in 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
|
@/tmp/stage-1.md |
| vitest framework | ||
| - **File naming**: `PascalCase.tsx` for React components, `kebab-case.ts` for | ||
| new non-component files. Leave existing `camelCase` files alone — renaming breaks `git blame` and imports. | ||
| all `.ts` files (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. |
There was a problem hiding this comment.
[Suggestion] Test
— qwen3.7-max via Qwen Code /review
Dismissing: review was based on local diff including already-merged commits. Re-submitting with correct scope.
wenshao
left a comment
There was a problem hiding this comment.
Note: the PR branch contains commit a93314e (--list-extensions flag handler) which is already merged into main via PR #4673. Consider rebasing onto the latest main to remove the already-merged commit and get clean CI.
— qwen3.7-max via Qwen Code /review
| vitest framework | ||
| - **File naming**: `PascalCase.tsx` for React components, `kebab-case.ts` for | ||
| new non-component files. Leave existing `camelCase` files alone — renaming breaks `git blame` and imports. | ||
| all `.ts` files (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. |
There was a problem hiding this comment.
[Suggestion] This states "kebab-case.ts for all .ts files (enforced by ESLint)" but the rule only covers packages/core/src/**/*.ts and packages/cli/src/**/*.ts. Other packages (acp-bridge, channels, daemon, sdk-typescript, etc.) are not covered.
| all `.ts` files (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. | |
| - **File naming**: `PascalCase.tsx` for React components, `kebab-case.ts` for | |
| `.ts` files in `packages/core` and `packages/cli` (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. |
— qwen3.7-max via Qwen Code /review
465ad14 to
010f52d
Compare
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] No automated allowlist drift detection
There is no test or CI check that verifies legacyFilenames entries still correspond to actual files on disk. As files are renamed to kebab-case, stale entries will accumulate silently — ESLint does not warn about unused ignore patterns. Consider adding a lightweight validation script (e.g., a node script that globs packages/{core,cli}/src/**/*.ts and asserts every allowlist entry matches at least one file) to lint:ci or as a pre-commit hook.
— qwen3.7-max via Qwen Code /review
| 'responseParsingOptions', | ||
| 'restoreCommand', | ||
| 'restoreGoal', | ||
| 'retryContext', |
There was a problem hiding this comment.
[Suggestion] Allowlist sort order errors
retryContext (here) should come after resumeHistoryUtils, not before resumeCommand. Alphabetically: restore* < resume* < retry*.
Similarly, userPromptExpansionHook (line 511) should come before useWelcomeBack — userP... < useW... in case-insensitive sort.
— qwen3.7-max via Qwen Code /review
| 'abortController', | ||
| 'aboutCommand', | ||
| 'acpAgent', | ||
| 'acpAgent.worktree', |
There was a problem hiding this comment.
[Suggestion] 13 redundant sub-entries covered by parent prefix
Since the ignore pattern is **/${name}* (prefix match), sub-entries whose parent already exists are redundant. For example, acpAgent.worktree is already covered by acpAgent because **/acpAgent* matches acpAgent.worktree.ts.
Redundant entries (13 total): acpAgent.worktree, arenaCommand.agentComplete, chatRecordingService.autoTitle, chatRecordingService.customTitle, forkedAgent.agent, forkedAgent.cache, gitWorktreeService.hooks.integ, gitWorktreeService.symlinks.integ, runtimeStatus.config, sessionService.corruption, sessionService.rename, Session.worktree, StreamJsonOutputAdapter.dualOutput.
These can be removed to slim down the allowlist.
— qwen3.7-max via Qwen Code /review
| 'editHelper', | ||
| 'editorCommand', | ||
| 'editorSettingsManager', | ||
| 'enter-worktree.session.integ', |
There was a problem hiding this comment.
[Suggestion] 5 entries are already kebab-case and don't need allowlisting
With ignoreMiddleExtensions: true, the rule only checks the first segment (before the first dot) against KEBAB_CASE. These entries already pass:
enter-worktree.session.integ(line 110) —enter-worktreeis kebab-case ✓exit-worktree.session.integ(line 120) —exit-worktree✓integration.test.circular(line 193) —integration✓loggers.test.circular(line 210) —loggers✓pipeline.concurrent(line 273) —pipeline✓
These can be removed from the allowlist.
— qwen3.7-max via Qwen Code /review
| { | ||
| // Enforce kebab-case filenames | ||
| files: ['packages/core/src/**/*.ts', 'packages/cli/src/**/*.ts'], | ||
| ignores: legacyFilenames.map((name) => `**/${name}*`), // Exclude pre-existing camelCase files |
There was a problem hiding this comment.
[Suggestion] The ignores glob uses **/${name}* — the trailing * creates a prefix match that silently exempts any new file whose basename starts with a legacy allowlist entry. For example, sessionServiceV2.ts or useAuthHelper.ts would both bypass the kebab-case rule because **/sessionService* and **/useAuth* match them.
Consider using exact patterns that cover source files and their test/integ variants without over-matching:
| ignores: legacyFilenames.map((name) => `**/${name}*`), // Exclude pre-existing camelCase files | |
| ignores: legacyFilenames.flatMap((name) => [ | |
| `**/${name}.ts`, | |
| `**/${name}.*.ts`, | |
| ]), |
**/${name}.*.ts matches fooBar.test.ts and fooBar.integ.ts but does NOT match fooBarV2.ts or fooBarHelper.ts.
— qwen3.7-max via Qwen Code /review
| * | ||
| * See: https://github.com/QwenLM/qwen-code/issues/4419 | ||
| */ | ||
| export const legacyFilenames = [ |
There was a problem hiding this comment.
[Suggestion] ~15 entries are redundant sub-entries already covered by a parent entry. Because the ignores use a * suffix, dotted/suffixed variants like chatRecordingService.autoTitle are already matched by **/chatRecordingService* (from the chatRecordingService entry).
Redundant entries include: acpAgent.worktree, arenaCommand.agentComplete, chatRecordingService.autoTitle, chatRecordingService.customTitle, FileCommandLoader-extension, FileCommandLoader-markdown, forkedAgent.agent, forkedAgent.cache, gitWorktreeService.hooks.integ, gitWorktreeService.symlinks.integ, memoryDiagnosticsDumper, nonInteractiveCliCommands, runtimeStatus.config, Session.worktree, sessionService.corruption, sessionService.rename, StreamJsonOutputAdapter.dualOutput, systemInfoFields, terminalSetupCommand, useInputHistoryStore.
Removing them would shrink the allowlist from 519 to ~499 entries. Note: this cleanup depends on the * suffix remaining — if you switch to exact matching (see other comment), these dotted entries would need different handling.
— qwen3.7-max via Qwen Code /review
…posing methods from the IDE server (QwenLM#4797)
010f52d to
ae8f951
Compare
Local Verification ReportBranch: TypeScript Compilation (
|
| Package | PR Branch | main | Status |
|---|---|---|---|
packages/core |
0 errors | 0 errors | ✅ Clean |
ESLint check-file/filename-naming-convention Rule Verification
| Check | Result |
|---|---|
| Existing kebab-case files pass | ✅ No violations |
| Legacy camelCase/PascalCase files pass (allowlisted) | ✅ No violations |
packages/core/src/**/*.ts full scan |
✅ 0 filename violations |
packages/cli/src/**/*.ts full scan |
✅ 0 filename violations |
| New non-kebab file correctly rejected | ✅ BadFileName.ts → error filename-naming-convention |
Tested by creating a temporary packages/core/src/BadFileName.ts — the rule correctly reported:
error The filename "BadFileName.ts" does not match the "KEBAB_CASE" pattern check-file/filename-naming-convention
Allowlist Completeness
All existing non-kebab-case .ts files in packages/core/src and packages/cli/src are covered by eslint.legacy-filenames.mjs (~515 entries). No false positives found during full-scope lint scan.
Code Review
eslint.config.js:
- New config block targets
packages/core/src/**/*.tsandpackages/cli/src/**/*.ts - Uses
legacyFilenames.flatMap()to generate ignores in the pattern**/${name}.tsand**/${name}.*.ts(covers both source and test files) eslint-plugin-check-filewithKEBAB_CASEpattern andignoreMiddleExtensions: true(sofoo.test.tschecksfoo, notfoo.test)
eslint.legacy-filenames.mjs:
- 525 lines, ~515 basename entries (no extensions)
- Covers all existing camelCase/PascalCase files in scope
- Clean format: one name per line, alphabetically sorted
AGENTS.md:
- Updated file naming guidance to reference the ESLint enforcement and allowlist file
package.json:
- Added
eslint-plugin-check-file@^3.3.1as devDependency
Verdict
✅ Ready to merge — Rule correctly enforces kebab-case on new files, allowlist covers all existing files (zero false positives), TSC clean. No behavior changes to runtime code.
| 'useToolScheduler', | ||
| 'useTrustModify', | ||
| 'useTurnDiffs', | ||
| 'userPromptExpansionHook', |
There was a problem hiding this comment.
[Suggestion] userPromptExpansionHook is out of alphabetical order. It currently sits between useTurnDiffs (line 504) and useWelcomeBack (line 506), but should be placed between useResumeCommand (line 483) and userStartupWarnings (line 484). In ASCII order: useR… < user… < useS….
| 'userPromptExpansionHook', | |
| 'useResumeCommand', | |
| 'userPromptExpansionHook', | |
| 'userStartupWarnings', |
— qwen3.7-max via Qwen Code /review
| vitest framework | ||
| - **File naming**: `PascalCase.tsx` for React components, `kebab-case.ts` for | ||
| new non-component files. Leave existing `camelCase` files alone — renaming breaks `git blame` and imports. | ||
| `.ts` files in `packages/core` and `packages/cli` (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. |
There was a problem hiding this comment.
[Suggestion] The new text drops the explicit warning about git blame history loss and import breakage that the original had ("renaming breaks git blame and imports"). The replacement ("rename opportunistically") is ambiguous — it could encourage drive-by file renames in unrelated feature PRs without signaling the cost.
| `.ts` files in `packages/core` and `packages/cli` (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them. | |
| `.ts` files in `packages/core` and `packages/cli` (enforced by ESLint). Existing camelCase files are allowlisted in `eslint.legacy-filenames.mjs`; rename opportunistically when touching them, updating all imports in the same commit (note: renames lose `git blame` history). |
— qwen3.7-max via Qwen Code /review
ae8f951 to
4258651
Compare
| // For more info, see https://github.com/storybookjs/eslint-plugin-storybook#configuration-flat-config-format | ||
| import storybook from 'eslint-plugin-storybook'; | ||
| import checkFile from 'eslint-plugin-check-file'; | ||
| import { legacyFilenames } from './eslint.legacy-filenames.mjs'; |
There was a problem hiding this comment.
[Suggestion] No automated test coverage for this rule or the allowlist.
No vitest test validates that:
- The
check-file/filename-naming-conventionrule correctly accepts kebab-case and rejects camelCase filenames - Every
legacyFilenamesentry maps to an actual file on disk
A plugin upgrade or allowlist typo could silently break enforcement. Consider adding a test using ESLint's RuleTester API plus a filesystem cross-reference check.
— qwen3.7-max via Qwen Code /review
|
Assigning this to @pomelo-nwu to own. The implementation here is solid — verified the allowlist is complete (full ESLint run over all 1554 in-scope The open question is the convention decision itself, which belongs with the RFC in #4419: that issue is still |
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds an ESLint rule (check-file/filename-naming-convention) to enforce kebab-case filenames in packages/core/src and packages/cli/src, with a legacy allowlist for ~517 existing non-compliant files.
CI status: All 11 checks passing (Lint, Tests on all 3 platforms, CodeQL).
Deterministic analysis: 0 findings from tsc and eslint on the changed files.
Code review: The implementation is structurally sound. The ESLint flat-config integration is correct, the ignoreMiddleExtensions: true option properly handles .test.ts files, and .tsx files are correctly excluded from the rule (preserving PascalCase for React components).
The 10 existing inline comments from @wenshao provide comprehensive coverage of the open design questions: allowlist maintenance (redundant entries, sort-order issues, already-compliant entries), the prefix-match risk in the ignores glob, the wording precision in AGENTS.md, and the gap in automated test coverage for the rule/allowlist itself. I concur with these observations and have no additional findings to add.
— qwen-code via Qwen Code /review
|
The rule and allowlist look correct, and the prefix-match issue from the earlier round is fixed — The real gate here isn't the code — it's that this is a CI hard-gate locking kebab-case onto a codebase that's currently majority camelCase. That direction, and the migration commitment behind it, should be ratified in #4419 before the enforcement lands; otherwise the 515-entry allowlist just becomes permanent debt. That's already been routed to the RFC author. Non-blocking, but worth tightening before merge:
One design note for the #4419 discussion, not this PR: the rule only catches uppercase in the first segment of a 中文规则和 allowlist 本身没问题，上一轮的 prefix-match 也修了—— 真正的关卡不在代码，而在于这是一道 CI 硬卡，把 kebab-case 锁死在一个当前以 camelCase 为主的代码库上。这个方向以及背后的迁移承诺，应该先在 #4419 里拍板再落地，否则这份 515 条的 allowlist 就会变成永久技术债。该决策已经转给 RFC 作者。 非阻塞，但合并前值得收紧：
一个留给 #4419 讨论、不针对本 PR 的设计提醒：规则只检查 |
What this PR does
Adds an ESLint rule (
check-file/filename-naming-convention) to enforce kebab-case for all.tsfiles inpackages/core/srcandpackages/cli/src. Existing non-kebab-case files are excluded viaeslint.legacy-filenames.mjsfor gradual migration. Updates AGENTS.md to reflect the new convention.Why it's needed
File naming in the codebase is split between camelCase and kebab-case with no enforcement. Newer modules already converged to kebab-case, but there's nothing preventing new camelCase files from being added. This formalizes the trend and prevents backsliding via a CI hard gate.
Reviewer Test Plan
How to verify
echo "// test" > packages/core/src/testFoo.tsnpx eslint packages/core/src/testFoo.ts— should report an errorecho "// test" > packages/core/src/test-foo.tsnpx eslint packages/core/src/test-foo.ts— should passnpm run lint— should pass with zero new errorsrm packages/core/src/testFoo.ts packages/core/src/test-foo.tsEvidence (Before & After)
N/A — lint rule enforcement, no UI changes.
Tested on
Environment (optional)
N/A — unit-level ESLint check only.
Risk & Scope
vscode-ide-companion,sdk-typescript, etc.) also have non-kebab-case files but are out of scope per issue Standardize file naming to kebab-case with ESLint enforcement and AGENTS.md documentation #4419.Linked Issues
Closes #4419 (infrastructure part — ESLint rule and allowlist; batch renaming will follow in separate PRs)
中文说明
这个 PR 做了什么
为
packages/core/src和packages/cli/src下的所有.ts文件添加 ESLint 规则（check-file/filename-naming-convention），强制使用 kebab-case 命名。现有的非 kebab-case 文件通过eslint.legacy-filenames.mjs排除，支持渐进式迁移。同时更新了 AGENTS.md 中的文件命名约定。为什么需要
代码库中文件命名混用 camelCase 和 kebab-case，且没有强制机制。较新的模块已经收敛到 kebab-case，但没有任何措施阻止新增 camelCase 文件。此 PR 通过 CI 硬卡将这一趋势正式化，防止倒退。
Reviewer Test Plan
How to verify
echo "// test" > packages/core/src/testFoo.tsnpx eslint packages/core/src/testFoo.ts——应报错echo "// test" > packages/core/src/test-foo.tsnpx eslint packages/core/src/test-foo.ts——应通过npm run lint——应无新增错误rm packages/core/src/testFoo.ts packages/core/src/test-foo.tsEvidence (Before & After)
N/A——lint 规则强制，无 UI 变更。
Risk & Scope
vscode-ide-companion、sdk-typescript等）也有非 kebab-case 文件，但根据 issue Standardize file naming to kebab-case with ESLint enforcement and AGENTS.md documentation #4419 不在本次范围内。