chore: typecheck test files; add CI workflow - #3
Conversation
npm run typecheck (tsc --noEmit) ignored every test file because tsconfig.json excluded src/**/*.test.ts — type errors in tests were invisible. Split the config (the hadrontool-twilio pattern, 87f8659): tsconfig.json now excludes only node_modules/dist so typecheck covers everything; compilation moves to tsconfig.build.json, which keeps test files out of dist/. Also adds the family-standard CI workflow (this repo had none), with node-version pinned to the engines floor (>=22.12.0). Verified: 4 test files now typechecked (0 latent errors here), dist/ contains no test artifacts, all 56 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request separates the build configuration from the main TypeScript configuration by introducing a new tsconfig.build.json file and updating the build script in package.json. This ensures test files are excluded during the build while remaining available for type-checking. Feedback highlights a non-standard glob pattern in the TypeScript exclusion list and notes that the CI workflow file mentioned in the pull request description is missing from the changes.
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.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "extends": "./tsconfig.json", | |||
| "exclude": ["node_modules", "dist", "src/**/*.test.ts", "src/test/**"] | |||
There was a problem hiding this comment.
In TypeScript tsconfig.json glob patterns, ** is a recursive directory wildcard and should be followed by a slash and a pattern (e.g., **/*), or you can simply specify the directory path (e.g., "src/test") to exclude the directory and all of its contents. Using "src/test/**" is non-standard and may not be parsed correctly by tsc, which could result in files under src/test/ not being excluded during the build and being emitted to the dist/ directory.
| "exclude": ["node_modules", "dist", "src/**/*.test.ts", "src/test/**"] | |
| "exclude": ["node_modules", "dist", "src/**/*.test.ts", "src/test"] |
| "scripts": { | ||
| "dev": "tsx watch src/index.ts", | ||
| "build": "tsc", | ||
| "build": "tsc -p tsconfig.build.json", |
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7fc3230db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "scripts": { | ||
| "dev": "tsx watch src/index.ts", | ||
| "build": "tsc", | ||
| "build": "tsc -p tsconfig.build.json", |
There was a problem hiding this comment.
Add tsconfig.build.json to the Docker image
When this build script is invoked by the production Dockerfile, only package*.json, tsconfig.json, and src are copied before RUN npm run build (Dockerfile:28-32); the new tsconfig.build.json is never present in the image. Because this line now points tsc at that missing file, a clean docker build fails with a missing-config error before any deployment image can be produced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates TypeScript configuration so npm run typecheck includes test files while npm run build still excludes them from emitted output, and adds a basic CI workflow to run typechecking and tests on pushes/PRs.
Changes:
- Remove test-file excludes from
tsconfig.jsonso typechecking covers tests. - Add
tsconfig.build.jsonand pointnpm run buildat it to keep tests out ofdist/. - Add a GitHub Actions workflow to run
npm ci,npm run typecheck, andnpm test.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tsconfig.json | Stops excluding tests so tsc --noEmit typechecks them. |
| tsconfig.build.json | Build-only TS config that re-excludes tests from emitted output. |
| package.json | Updates build script to compile with the build tsconfig. |
| .github/workflows/ci.yml | Adds CI job to install deps, typecheck, and run tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '>=22.12.0' | ||
| cache: npm |
The build script now runs `tsc -p tsconfig.build.json`, but the Dockerfile only copied tsconfig.json before `npm run build`, so a clean image build failed with a missing-config error (Komodo builds the image on merge; CI does not, so this was uncaught). Copy tsconfig.build.json alongside it. Also drop the vestigial `src/test/**` exclude — there is no src/test/ dir; `src/**/*.test.ts` already excludes every test file from the emitted build (verified: dist has zero test artifacts). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed in the latest commit:
|
Summary
npm run typecheckignored every test file —tsconfig.jsonexcludedsrc/**/*.test.ts, so type errors in tests were invisible. This ports the split hadrontool-twilio established (87f8659), where enabling it immediately surfaced two real type errors in that repo's tests:tsconfig.jsonexcludes onlynode_modules/dist→ typecheck covers tests.tsconfig.build.json(extends the base, re-adds the test excludes) →npm run buildstill keeps test files out ofdist/.>=22.12.0).Verification
tsc --listFiles— zero latent errors in this repo (the tests were clean).npm run buildemits no test artifacts intodist/.npm test: 56 passed.🤖 Generated with Claude Code