fix(checks): run Windows command shims through cmd - #6870
Conversation
Signed-off-by: HwangJohn <angelic805@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe checks script now exposes reusable, injectable runner APIs, builds platform-specific spawn invocations, preserves direct execution behavior, and adds tests for Windows, POSIX, and failure paths. ChangesChecks runner refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant runChecks
participant buildCheckSpawnInvocation
participant spawn
participant exit
runChecks->>buildCheckSpawnInvocation: build command and arguments
runChecks->>spawn: execute check with inherited stdio
spawn-->>runChecks: return status and error
runChecks->>exit: exit with returned status or 1
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/checks-runner.test.ts (1)
80-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a non-null failing status code.
The only failure-path test uses
status: null, which always collapses toexit(1)viaresult.status ?? 1. This can't catch a regression where the actual status code (e.g.,2) is dropped andexit(1)is hardcoded instead of passed through.✅ Suggested additional test
+ it("exits with the check's status code on failure", () => { + const spawn = vi.fn((_command: string, _args: string[], _options: SpawnSyncOptions) => ({ + status: 2, + })); + const exit = vi.fn((code?: number): never => { + throw new Error(`exit ${code}`); + }); + + expect(() => runChecks({ checks: [sampleCheck], platform: "linux", spawn, exit })).toThrow( + "exit 2", + ); + expect(exit).toHaveBeenCalledWith(2); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/checks-runner.test.ts` around lines 80 - 93, Add a test alongside the existing no-status case that configures the mocked spawn result with a non-null failing status such as 2, invokes runChecks with the same sampleCheck setup, and asserts exit is called with 2 while preserving the thrown-exit assertion. This should verify the actual status code is propagated rather than replaced with the default 1.scripts/checks/run.ts (1)
122-141: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpawn failures without a status code are logged with no diagnostic detail.
When
spawnfails to even start (e.g.,cmd.exe/shim not found →ENOENT),spawnSync-style results typically carry anerrorfield withstatus: null. The loop only logsCheck failed: ${check.name}and exits — the actual cause (result.error?.message) is discarded, making CI failures harder to diagnose.🩹 Proposed fix to surface the underlying error
if (result.status !== 0) { - console.error(`Check failed: ${check.name}`); + console.error(`Check failed: ${check.name}`, "error" in result ? result.error : ""); exit(result.status ?? 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/checks/run.ts` around lines 122 - 141, Update the failure handling in runChecks to include result.error?.message when a check has no status code, while preserving the existing check name and exit behavior. Ensure spawn failures such as ENOENT surface their underlying diagnostic in the console.error output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/checks/run.ts`:
- Around line 122-141: Update the failure handling in runChecks to include
result.error?.message when a check has no status code, while preserving the
existing check name and exit behavior. Ensure spawn failures such as ENOENT
surface their underlying diagnostic in the console.error output.
In `@test/checks-runner.test.ts`:
- Around line 80-93: Add a test alongside the existing no-status case that
configures the mocked spawn result with a non-null failing status such as 2,
invokes runChecks with the same sampleCheck setup, and asserts exit is called
with 2 while preserving the thrown-exit assertion. This should verify the actual
status code is propagated rather than replaced with the default 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e74e5e1c-0b90-486d-a412-c349d1defbd7
📒 Files selected for processing (2)
scripts/checks/run.tstest/checks-runner.test.ts
Signed-off-by: HwangJohn <angelic805@gmail.com>
wscurran
added
area: ci
|
✨ Thanks for the fix, @HwangJohn. Routing Windows command shims through cmd.exe should resolve the EINVAL issue on Windows. Ready for maintainer review. |
|
CI is green and the current revision is approved. |
3 participants
Summary
Repository checks now invoke Windows
.cmdshims throughcmd.exeinstead of spawning the shim directly. This keepsnpm run checksusable on Windows where directspawnSync('*.cmd')returnsEINVAL, while leaving POSIX execution direct.Changes
scripts/checks/run.tsinto testable invocation helpers while preserving the existing check list and script entry point.cmd.exe /d /s /c <shim> ...argssotsx.cmdcan run under Node on Windows.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/checks-runner.test.tspassed; Windows local mechanism check showed directspawnSync('npm.cmd')returnsEINVALwhilecmd.exe /d /s /c npm.cmd --versionexits 0.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: DGX Spark/Linuxnpm run checks,npm run typecheck:cli, andnpm run check:diffpassed.npm run checkwas attempted but the host lackshadolint, and the separate all-files manual coverage stage exceeded the 10-minute local timeout; no broad-gate success is claimed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: HwangJohn angelic805@gmail.com
Summary by CodeRabbit
Bug Fixes
ComSpec/cmd.exe routing.Refactor
Tests