ci: gate every PR on tsc-cli regardless of touched paths - #2460
Conversation
Prevents the class of regression that required #2458. The prek tsc-cli hook had files: ^(bin|scripts)/, so a PR touching only test/ or src/ skipped the check entirely. That let #2130 (test-only) land without typecheck, then #2422 tightened strict: true and the latent errors surfaced for every downstream PR — six open PRs were stuck on the same cluster by the time it was noticed. Two changes, belt-and-suspenders: - New explicit Typecheck CLI + tests (strict) step in basic-checks action. Runs npm run typecheck:cli unconditionally. Independent of how anyone configures prek, so future hook-filter drift can't hide a tsc error from CI. - Widen the prek tsc-cli hook's files: to match what tsconfig.cli.json actually covers (bin/, scripts/, src/, test/, tsconfig.*.json). Local pre-push now gates the same class of change that CI does. Zero runtime impact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
📝 WalkthroughWalkthroughThe changes expand TypeScript strict-mode type checking coverage by introducing a dedicated CI step for CLI typechecking and broadening the pre-commit hook's file-pattern matcher to include Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 245-246: The pre-commit hook currently uses both the files pattern
and the types_or filter so tsconfig.*\.json (matched by the files regex) never
triggers because types_or: [ts, tsx] restricts to TypeScript file types; update
the .pre-commit-config.yaml by either removing the types_or key entirely from
this hook or expanding/restructuring it so JSON config files are included (for
example add "json" to types_or or separate the tsconfig.*\.json pattern into its
own hook entry), ensuring the "files" regex for tsconfig.*\.json and the
types_or setting no longer block each other.
🪄 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: CHILL
Plan: Enterprise
Run ID: ad954283-8ff1-423c-b837-c3630e1396b8
📒 Files selected for processing (2)
.github/actions/basic-checks/action.yaml.pre-commit-config.yaml
| files: ^(bin/|scripts/|src/|test/|tsconfig.*\.json$) | ||
| types_or: [ts, tsx] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In pre-commit/prek hook matching, when both filesandtypes_or are set, must a changed file satisfy both filters to trigger the hook?
💡 Result:
Yes, a changed file must satisfy both the files filter (if specified) and the types_or filter to trigger the hook. All top-level filters (files, types, types_or, exclude_types) are combined with AND logic after the initial files/exclude regex filtering.
Citations:
- 1: https://pre-commit.com/index.html
- 2: https://pre-commit.com/
- 3: https://github.com/pre-commit/pre-commit/blob/master/pre_commit/commands/run.py
Remove types_or filter or restructure to include tsconfig.*.json files.
The files pattern includes tsconfig.*\.json on Line 245, but types_or: [ts, tsx] on Line 246 prevents JSON files from triggering the hook. Pre-commit applies both filters with AND logic, so a file must match both patterns; since tsconfig.*.json has type json, not ts/tsx, changes to config files skip this hook.
Suggested fix
- files: ^(bin/|scripts/|src/|test/|tsconfig.*\.json$)
- types_or: [ts, tsx]
+ files: ^((bin|scripts|src|test)/.*\.(ts|tsx)$|tsconfig.*\.json$)📝 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.
| files: ^(bin/|scripts/|src/|test/|tsconfig.*\.json$) | |
| types_or: [ts, tsx] | |
| files: ^((bin|scripts|src|test)/.*\.(ts|tsx)$|tsconfig.*\.json$) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml around lines 245 - 246, The pre-commit hook
currently uses both the files pattern and the types_or filter so
tsconfig.*\.json (matched by the files regex) never triggers because types_or:
[ts, tsx] restricts to TypeScript file types; update the .pre-commit-config.yaml
by either removing the types_or key entirely from this hook or
expanding/restructuring it so JSON config files are included (for example add
"json" to types_or or separate the tsconfig.*\.json pattern into its own hook
entry), ensuring the "files" regex for tsconfig.*\.json and the types_or setting
no longer block each other.
|
Superseded by #2461 — trimmed to just the CI step per review (the prek hook-filter widening was nice-to-have but not load-bearing since |
2 participants
Summary
Prevents the class of regression that required #2458. Adds an unconditional
typecheck:clistep to the shared CI action, and broadens the prek hook filter to match whattsconfig.cli.jsonactually typechecks.Related
Follow-up to #2458 (which fixed the symptom — this addresses the root cause).
Root cause recap
.pre-commit-config.yamlhadfiles: ^(bin|scripts)/on thetsc-clihook.test/orsrc/never triggered the check.strict: true, and the latent errors surfaced — but for every downstream PR, not the PR that introduced them.Changes
.github/actions/basic-checks/action.yaml— new Typecheck CLI + tests (strict) step runningnpm run typecheck:cliunconditionally. Independent of prek hook configuration, so future filter drift can't hide a tsc error from CI..pre-commit-config.yaml— widentsc-clihook'sfiles:from^(bin|scripts)/to^(bin/|scripts/|src/|test/|tsconfig.*\.json$). Local pre-push now gates the same surface CI does.Belt-and-suspenders. Either one would have caught #2130's regression on its own; running both means local hook and CI are always in sync on this class of check.
Type of Change
Verification
npm run typecheck:cliexits 0 on this branch (rebased on main after fix(test): satisfy strict typecheck on Slack token validation tests #2458 lands will confirm)npx prek run --all-filespassesAI Disclosure
Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit