Add a npx hereby validate command to group all repo validations - #64161
Conversation
…lidations you otherwise do
typescript-automation
Bot
added
the
For Uncommitted Bug
|
|
||
| await runValidation("test", async () => { | ||
| await runTests(); | ||
| await runTestExtension(); |
There was a problem hiding this comment.
Can we take the time now to get these extension tests out too into their own task? I've been wanting hereby test to not run them.
There was a problem hiding this comment.
They already are. There's hereby test:extension defined already. I can (and just did) remove it from test so test just aliases test:tsc - CI doesn't use test (or validate, ofc) anyway, so this only impacts our muscle memory for what commands do what locally.
| } | ||
|
|
||
| export const validate = task({ | ||
| name: "validate", |
There was a problem hiding this comment.
We have a lot of terms, test, check, validate... Sort of wonder if we should somehow name this with "all" in the name to make it very clear what must be done
There was a problem hiding this comment.
I added a --all argument to validate that includes all the various tool/benchcmark/api tests and smoke test. Pretty sure that covers every test: subcommand listed in ci.yml.
| npx hereby test # Run tests | ||
| npx hereby lint # Run linters | ||
| npx hereby format # Format the code | ||
| npx hereby validate # Build, test, lint, and format the project |
There was a problem hiding this comment.
One of the things I want to do in this file is to make it clear that the build, test, lint, just matter when editing the tsc dir.
We can probably do that in a followup, as the "CRITICAL" language here should really be in the CCA file instead
There was a problem hiding this comment.
...isn't this the CCA file? It's copilot-instructions.md. The only other markdown file that ever even mentions hereby commands is the generic CONTRIBUTING.md (and the compiler test skill).
There was a problem hiding this comment.
This is the file used by all copilot stuff; they only recently added a file that is for CCA only. Unfortunately I now cannot find the docs for this. They do have https://github.blog/changelog/2025-11-12-copilot-code-review-and-coding-agent-now-support-agent-specific-instructions/ which lets you exclude certain things, but I know they have a separate CCA one somewhere...
There was a problem hiding this comment.
🟡 Changes recommended
Build must use the failure-capturing path so all validations run and failures are aggregated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a unified npx hereby validate command for repository checks.
Changes:
- Adds validation orchestration with optional API tests and aggregated failures.
- Updates Copilot instructions to use the consolidated workflow.
File summaries
| File | Description |
|---|---|
Herebyfile.mjs |
Implements the validation task and --api option. Build failures currently prevent remaining validations from running. |
.github/copilot-instructions.md |
Documents the unified validation command and checklist. |
Review details
Suppressed comments (1)
Herebyfile.mjs:1371
- With
--api, this callsrunTestAPIdirectly but never runs thebuildAPITestsprerequisite that the existingtest:apitask declares at lines 1250-1254. On a clean checkout, the generated API test artifacts therefore are not prepared, so the documented replacement fortest:apican fail or exercise stale output. Add that prerequisite conditionally when API validation is requested.
dependencies: [build],
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| export const validate = task({ | ||
| name: "validate", | ||
| description: "Builds, tests, lints, and formats the repo. Pass --api to include API tests.", | ||
| dependencies: [build], |
There was a problem hiding this comment.
I didn't notice this, but yeah build doesn't need to be a dep, right? nothing needs that to have happened? I guess the API does?
There was a problem hiding this comment.
The API needs it to work and I have doubts on if lint, test, and format would reliably work in the presence of a non-functioning build.
There was a problem hiding this comment.
So right, I was just surprised by it being a dep versus another func call, but it doesn't practically matter.
I have seen a lot of very dumb powershell trying to shorten the list of commands into "a single script" with appropriate error handling, and a lot of the time it does not work.
Better to just provide it upfront as a single command.