-
Notifications
You must be signed in to change notification settings - Fork 7
chore: add CI validation for worker registration #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! 👏🏻
I have mixed feelings about using string-based detection instead of parsing the config files into JSON or JS objects, but I think this is good enough for start and we can easily improve it later if needed.
| - name: Validate worker registration | ||
| run: npm run validate-new-workers | ||
| working-directory: worker | ||
|
|
||
| - run: npm run lint | ||
| - run: npm test | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run the new script as part of npm run lint, so that we can catch errors before opening a pull request.
| // 2. Validate each worker is registered everywhere | ||
| for (const worker of workers) { | ||
| if (!vitest.includes(worker)) { | ||
| fail(`${worker} missing from vitest.workspace.js`); | ||
| } | ||
|
|
||
| if (!tsconfig.includes(worker)) { | ||
| fail(`${worker} missing from tsconfig.json project references (run npm run update-ts-project)`); | ||
| } | ||
|
|
||
| if (!ci.includes(worker)) { | ||
| fail(`${worker} missing from ci.yml deploy job`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't abort on the first problem, print all issues encountered. At the end, exit with code 0 if no issue was found or code 1 if some issues were detected.
| ], | ||
| "scripts": { | ||
| "build:types": "npm run build:types --workspaces --if-present", | ||
| "validate-new-workers": "node scripts/validate-new-workers.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We execute this for every CI run, even for workers that are no longer "new". Let's find a better name, please.
For example: check-worker-registries
| if (!ci.includes(worker)) { | ||
| fail(`${worker} missing from ci.yml deploy job`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add one more validation step for this part:
worker/docs/adding-new-worker.md
Line 55 in 8140c4a
| 5. **`CLAUDE.md`** - Add the new worker to the "Monorepo Structure" list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a CI-time validation to ensure every npm workspace “worker” is registered consistently across the monorepo (Vitest workspace config, TS project references, and CI workflow), enforcing the steps in docs/adding-new-worker.md (Fixes #375).
Changes:
- Add
scripts/validate-new-workers.jsto validate workspace registration across key config files. - Add an npm script (
validate-new-workers) to run the validation. - Invoke the validation from the CI workflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/validate-new-workers.js |
New validation script that compares package.json workspaces against vitest.workspace.js, tsconfig.json, and .github/workflows/ci.yml. |
package.json |
Adds a script entry to run the validation. |
.github/workflows/ci.yml |
Runs the new validation step during CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Root files (script runs inside worker/) | ||
| const rootPkg = JSON.parse(read("../package.json")); | ||
| const vitest = read("../vitest.workspace.js"); | ||
| const tsconfig = read("../tsconfig.json"); | ||
| const ci = read("../.github/workflows/ci.yml"); |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script reads root files via "../..." paths, but npm run validate-new-workers runs with the repo root as the working directory (package.json script), so read("../package.json")/etc will resolve outside the repo and fail with ENOENT. Resolve paths relative to the repo root (e.g., process.cwd()) or relative to the script location (import.meta.url + path.resolve).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid point. PTAL, @FAIZ1409
An idea to consider: you can use dynamic imports for JS & JSON files:
await import('../vitest.workspace.js')
await import('../tsconfig.json', with: { type: "json" })
Then you can inspect the objects to verify that each worker is registered at the correct place.
That does not solve the problem of finding the correct path of the GH workflow file, so maybe it's not worth it at this time.
| if (!ci.includes(worker)) { | ||
| fail(`${worker} missing from ci.yml deploy job`); | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ci.includes(worker) scans the entire workflow file, but the failure message claims the worker is "missing from ci.yml deploy job". This can produce false positives if the worker name appears elsewhere (or as a substring) while still being absent from the deploy job. Consider narrowing the check to the jobs.deploy section (e.g., extract that block and search within it, or parse the YAML and verify deploy step workingDirectory values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think such false positives are very unlikely and we don't need to worry about them right now. We can fix this check after it starts reporting false positives.
|
|
||
| - name: Validate worker registration | ||
| run: npm run validate-new-workers | ||
| working-directory: worker |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working-directory: worker points to a directory that doesn't exist in this repo (the monorepo root contains package.json, scripts/, etc.). This step will fail to locate package.json and run the script. Run it from the repo root (remove working-directory) or set it to the correct path.
| working-directory: worker |
Adds a CI validation script to ensure all workspace workers are registered in:
This enforces docs/adding-new-worker.md automatically.
Fixes #375