chore(checks): wire custom checks into pre-push + CI, add no-duplicate-guards#2311
Conversation
…e-guards Pre-push was a dead zone — lefthook only ran Biome on pre-commit, so all 9 custom scripts were invisible to developers. This PR closes that gap and adds a missing check. - lefthook: add pre-push hook running `bun check:all` (full custom suite) - checks.yml: add lint:custom and check:casts:strict steps (continue-on-error while existing backlog is cleared; pre-push already blocks new violations) - biome.json: escalate useTopLevelRegex warn → error - package.json: expose `check:all` as a root script - scripts/check-all.ts: add no-raw-process-env, no-duplicate-guards, check-type-casts --strict to the orchestrated suite - scripts/lint/no-duplicate-guards.ts (new): flags re-implementations of guards already exported from @packrat/guards; found 4 assertDefined duplicates on first run
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
Coverage Report for API Unit Tests Coverage (./packages/api)
File CoverageNo changed files found. |
There was a problem hiding this comment.
Pull request overview
This PR strengthens developer/CI feedback loops by wiring the repo’s custom lint/check scripts into local git hooks and the Checks workflow, and introduces a new guard-duplication detector to enforce using @packrat/guards as the single source of truth.
Changes:
- Add
check:allscript and wire it into a newpre-pushhook to run the full custom check suite locally. - Extend CI (
checks.yml) withlint:customand strict cast checks (temporarilycontinue-on-error). - Add
scripts/lint/no-duplicate-guards.tsto detect re-implementations of known guards outside@packrat/guards.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/lint/no-duplicate-guards.ts |
New custom lint script to detect duplicated guard implementations across apps/packages. |
scripts/check-all.ts |
Adds the new guard-duplication check + strict type-cast check to the orchestrated suite. |
package.json |
Exposes check:all as a root script entry point. |
lefthook.yml |
Adds a pre-push hook to run the full check suite locally. |
biome.json |
Escalates useTopLevelRegex from warning to error. |
.github/workflows/checks.yml |
Adds CI steps for lint:custom and strict cast checks with continue-on-error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Runs the following checks in parallel and prints a unified summary table: | ||
| // - scripts/lint/no-raw-regex.ts | ||
| // - scripts/lint/no-raw-typeof.ts | ||
| // - scripts/lint/no-raw-process-env.ts |
There was a problem hiding this comment.
The check list in the file header references scripts/lint/no-raw-process-env.ts, but the actual check runs packages/env/scripts/no-raw-process-env.ts (see ALL_CHECKS). Updating the comment will avoid confusion when someone tries to locate the script.
| // - scripts/lint/no-raw-process-env.ts | |
| // - packages/env/scripts/no-raw-process-env.ts |
| # Pre-push hook already blocks new violations — these report on the backlog. | ||
| - name: Custom lint rules (typeof guards, raw regex, process.env) | ||
| run: bun lint:custom | ||
| continue-on-error: true |
There was a problem hiding this comment.
CI adds a lint:custom step, but it currently runs only no-raw-typeof, no-raw-regex, and no-raw-process-env (per package.json). The newly added no-duplicate-guards check is not executed anywhere in CI, so duplicates can still land by bypassing local hooks. Consider running bun check:all (optionally with continue-on-error while backlog is cleared) or adding no-duplicate-guards to lint:custom / as its own step.
| continue-on-error: true | |
| continue-on-error: true | |
| # TODO: remove continue-on-error once the duplicate-guards backlog is cleared. | |
| - name: Check duplicate guards | |
| run: bun no-duplicate-guards | |
| continue-on-error: true |
| // Excluded source roots (the canonical definitions live here). | ||
| const EXCLUDED_PREFIXES = ['packages/guards/', 'packages/checks/']; | ||
|
|
There was a problem hiding this comment.
EXCLUDED_PREFIXES values include a trailing /, but walkDir builds relPath values like packages/guards (no trailing slash) for the directory itself. As a result, isExcluded('packages/guards') returns false and this script will scan the canonical implementations in packages/guards/ (and packages/checks/), causing self-violations and making the check fail on every run. Consider normalizing relPath (e.g., ensure it always ends with / when comparing) or updating isExcluded to treat both exact directory matches and prefix matches as excluded.
| // 0 — no violations | ||
| // 1 — violations found | ||
| // | ||
| // Wired into check-all.ts and CI via checks.yml. |
There was a problem hiding this comment.
This header comment says the script is wired into CI via checks.yml, but the workflow changes shown only add lint:custom and check:casts:strict steps and do not run no-duplicate-guards. Either update the comment to match reality or add a CI step (or include this script in lint:custom / check:all) so CI reports these violations even if the pre-push hook is bypassed.
| // Wired into check-all.ts and CI via checks.yml. | |
| // Wired into check-all.ts. |
- Fix EXCLUDED_ROOTS trailing-slash mismatch (dirs were not excluded early) - Add no-duplicate-guards to lint:custom so CI covers it - Fix check-all.ts comment: correct path for no-raw-process-env - Fix header comment: wired into lint:custom, not checks.yml directly
chore(checks): wire custom checks into pre-push + CI, add no-duplicate-guards
Summary
lefthookonly ran Biome on pre-commit — all 9 custom scripts were invisible to developers, and several were absent from CI toopre-pushhook now runsbun check:all(full suite) so violations are caught locally before hitting CI; CI gainslint:customandcheck:casts:strictstepsno-duplicate-guards.tsflags re-implementations of guards already exported from@packrat/guards— found 4assertDefinedduplicates on first runChanges
lefthook.ymlpre-pushhook →bun check:all.github/workflows/checks.ymllint:custom+check:casts:strictsteps (continue-on-error while backlog is cleared)biome.jsonuseTopLevelRegexwarn→errorpackage.jsoncheck:allas a root scriptscripts/check-all.tsno-raw-process-env,no-duplicate-guards,check-type-casts --strictto the suitescripts/lint/no-duplicate-guards.ts@packrat/guardsWhy
continue-on-erroron the new CI stepsThere are ~78 existing
typeofguard violations and ~175 unsafe cast violations. Adding them as hard blocks would fail every open PR today. The pre-push hook already stops new violations from being pushed — CI reports on the existing backlog. Removecontinue-on-erroronce those are cleaned up.Test Plan
typeof x === 'string'outsidepackages/guards/— pre-push hook should block itexport function assertDefinedoutsidepackages/guards/—no-duplicate-guardsshould flag itbun check:allruns all 10 checks and shows the full tablebun lintstill auto-fixes inline regex literals (now errors, not warnings)