Add setup/teardown script discovery and execution#49
Conversation
Enables running setup scripts before races and teardown scripts after
races (even on failure). This allows starting local services, installing
dependencies, or preparing test fixtures before racing.
Convention-based discovery:
- setup.sh / setup.js runs before race starts
- teardown.sh / teardown.js runs after race completes
Config-based (in settings.json):
- setup: "./my-setup.sh" or { command, timeout, waitFor }
- teardown: same format
- waitFor supports polling a URL until service is ready
https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
Extends setup/teardown to support per-racer scripts in addition to
global scripts. This allows each racer to have its own initialization
and cleanup logic.
Convention-based discovery:
- {racer-name}.setup.sh / {racer-name}.setup.js
- {racer-name}.teardown.sh / {racer-name}.teardown.js
Config-based (in settings.json):
- racers.{name}.setup / racers.{name}.teardown
Execution order:
1. Global setup
2. Per-racer setups (in order)
3. Race
4. Per-racer teardowns (in order)
5. Global teardown
https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
Add dedicated test file for setup/teardown functionality: - Edge cases for global and per-racer script discovery - Handling of special characters in racer names (hyphens, dots, underscores) - Settings configuration edge cases - Integration tests verifying script execution - Execution order verification tests https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
Resolved conflicts in race.js imports to include both: - Setup/teardown functions (discoverSetupTeardown, discoverRacerSetupTeardown) - New imports from main (FORMAT_EXTENSIONS, getPlacementOrder, findMedianRunIndex) https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
Resolved three conflicts in race.js: - Imports: keep setup/teardown functions + add findMedianRunIndexPerRacer - Setup/teardown block placed before new settings defaults section from main - main() call: combine serve-mode condition with error .catch() handler https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds discovery helpers for global and per-racer setup/teardown scripts, extends runner to execute those scripts with timeouts and optional readiness polling, and updates tests to validate filename conventions, settings overrides, execution behavior, and ordering. Teardowns run from finally blocks; setup failures propagate. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Runner
participant Config as Discovery (cli/config.js)
participant FS as File System
participant Script as Script Process
participant HTTP as Readiness Poll
participant Race as Race Execution
CLI->>Config: discoverSetupTeardown(raceDir, settings)
Config->>FS: list & stat candidate files
Config-->>CLI: {setup, teardown}
CLI->>Script: runScript(globalSetup, "global setup")
Script->>Script: spawn (bash/node), enforce timeout
alt has waitFor
Script->>HTTP: poll waitFor.url until ready or timeout
end
Script-->>CLI: result / error
loop Per-racer
CLI->>Config: discoverRacerSetupTeardown(raceDir, racer, settings)
Config->>FS: stat racer-specific files
Config-->>CLI: {setup, teardown}
CLI->>Script: runScript(racerSetup, "racer setup")
Script->>Script: execute with timeout & optional polling
Script-->>CLI: result / error
end
CLI->>Race: start benchmark runs
Race-->>CLI: results
CLI->>Script: runScript(racerTeardown) in finally (log errors)
Script-->>CLI: completion
CLI->>Script: runScript(globalTeardown) in finally (log errors)
Script-->>CLI: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@race.js`:
- Around line 566-570: The timeout handler currently kills the child and
immediately calls progress.done and reject; instead, send SIGTERM
(child.kill('SIGTERM')) but do not reject immediately—listen for the child's
'close' (or 'exit') event and only call progress.done and reject after the
process has actually exited; also implement a short fallback timer (e.g., a few
hundred ms) that sends child.kill('SIGKILL') if the child hasn't exited, and
ensure both timers (the original timeoutId and the SIGKILL fallback) are cleared
when the process closes to avoid leaks; update logic around timeoutId,
child.kill, and reject to coordinate with the 'close' listener.
- Around line 645-665: The current pattern calls process.exit() inside per-step
catch blocks (during runScript for setupScript and racer.setup), which prevents
the surrounding finally teardown from running; instead, wrap all setup steps
(the global runScript(setupScript, 'Setup') and the loop over racerScripts
invoking runScript(racer.setup, ...)) inside a single try block that captures
any thrown error, avoid calling process.exit() inside those catch blocks
(re-throw or store the error), and perform teardown in the finally block; after
the finally completes, if an error was captured, call process.exit(1) (or
re-throw) so cleanup always runs — reference runScript, setupScript,
racerScripts, racer.setup, and process.exit in your changes.
- Around line 538-553: Validate the incoming script/config before resolving and
spawning: ensure the input (script or config) yields a non-empty string command
(check typeof and presence of config.command when script is an object), then
resolve scriptPath and use fs.statSync or fs.promises.stat to confirm scriptPath
exists and is a regular file (isFile) rather than a directory; also validate the
extension (ext from path.extname) against supported runners (e.g., '.sh' ->
'bash', '.js'/.mjs -> 'node') and reject with a clear error/reject if the
command is missing, not a file, or has an unsupported extension before calling
startProgress or spawning the child process (referencing variables/functions:
script, config, command, scriptPath, raceDir, ext, isShell, startProgress).
- Around line 584-599: The poll loop's fetch can hang and block the overall
deadline because the elapsed-time check runs only after fetch settles; update
the call inside poll to pass an AbortSignal timeout so each request is bounded:
call fetch(url, { signal: AbortSignal.timeout(remaining) }) where remaining =
Math.max(0, waitTimeout - (Date.now() - startTime)) so the per-request deadline
respects the overall waitTimeout, and ensure the existing try/catch around fetch
continues to handle abort/errors and lets the subsequent elapsed-time check,
waitProgress.done, resolve, and reject logic behave unchanged (symbols: poll,
fetch, AbortSignal.timeout, waitTimeout, startTime, waitProgress, resolve,
reject).
In `@tests/setup-teardown.test.js`:
- Around line 264-357: The tests currently spawn bash/node directly and bypass
the code under test; update the suite to exercise the real wrapper by importing
and calling runScript(...) (or exporting it from race.js) or by invoking race.js
against a temporary race directory so path resolution, timeout handling, waitFor
polling, and env injection are tested; change the test cases that use
spawn('bash'...) / spawn('node'...) to call runScript with appropriate
args/env/cwd (or run the race.js entrypoint) and add assertions for timeout and
waitFor behaviors to cover the new orchestration logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26b8dafe-bfaa-44e2-90ec-3c5669355b87
📒 Files selected for processing (4)
cli/config.jsrace.jstests/race-discovery.test.jstests/setup-teardown.test.js
There was a problem hiding this comment.
Pull request overview
Adds support for discovering and running optional global/per-racer setup & teardown scripts around a race run, with settings.json overrides and initial test coverage.
Changes:
- Added discovery helpers for global and per-racer setup/teardown scripts in
cli/config.js. - Integrated setup/teardown discovery + execution into the main CLI flow in
race.js, including optionaltimeoutandwaitForpolling. - Expanded test coverage for discovery behavior and added a new setup/teardown-focused test suite.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
cli/config.js |
Adds discoverSetupTeardown() and discoverRacerSetupTeardown() for convention- and settings-based script discovery. |
race.js |
Runs discovered/configured setup & teardown scripts before/after the race, with timeout + waitFor support and updated CLI help text. |
tests/setup-teardown.test.js |
New tests for discovery edge cases and basic script execution behaviors. |
tests/race-discovery.test.js |
Adds discovery tests for setup/teardown in the existing config/discovery test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes from PR #49 review: 1. **Process exit skipping teardown** - Moved setup into main try block; errors now propagate to finally block ensuring teardown always runs 2. **Timeout handler race condition** - Now waits for child 'close' event before rejecting; adds SIGKILL fallback after 5s grace period 3. **Fetch hanging indefinitely** - Added AbortSignal.timeout() to enforce per-request deadlines based on remaining wait time 4. **Missing command validation** - Validates command is non-empty string, path is a regular file, and extension is .sh or .js before spawning 5. **Directory detection gap** - Added isFile() check in discovery functions to filter out directories matching script names 6. **Platform-specific preference** - .js scripts now preferred on Windows, .sh elsewhere (via getScriptOrder helper) 7. **Test improvements** - Added helper functions to reduce duplication, changed chmod to 0o700 for secure test permissions https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
race.js (1)
591-597:⚠️ Potential issue | 🟠 Major
child.killedsuppresses the SIGKILL fallback.After Line 593 sends
SIGTERM,child.killedbecomestrueonce the signal is delivered, so the guard at Line 596 never escalates if the process ignoresSIGTERM. A timed-out hook can still hang indefinitely.♻️ Proposed fix
- const timeoutId = setTimeout(() => { + let killTimeoutId; + let exited = false; + const timeoutId = setTimeout(() => { timedOut = true; child.kill('SIGTERM'); - // Give process 5s to clean up after SIGTERM, then SIGKILL - setTimeout(() => { - if (!child.killed) child.kill('SIGKILL'); - }, 5000); + killTimeoutId = setTimeout(() => { + if (!exited) child.kill('SIGKILL'); + }, 5000); }, timeout); child.on('close', code => { + exited = true; clearTimeout(timeoutId); + clearTimeout(killTimeoutId); if (timedOut) { progress.done(`${label} timed out after ${timeout}ms`); reject(new Error(`${label} script timed out after ${timeout}ms`)); return; @@ child.on('error', err => { clearTimeout(timeoutId); + clearTimeout(killTimeoutId); progress.done(`${label} error: ${err.message}`); reject(err); });In Node.js `child_process`, when does `ChildProcess.killed` become `true`? Does it mean the child has exited, or only that `kill()` successfully sent a signal?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race.js` around lines 591 - 597, The guard using child.killed after sending SIGTERM is incorrect because ChildProcess.killed becomes true when kill() was called, not when the child has exited, so the SIGKILL fallback may be suppressed; change the escalation to detect whether the process is still running (e.g. check child.exitCode === null or attempt process.kill(child.pid, 0) in a try/catch) and only send SIGKILL if the child is still alive; update the block using the child variable (the timeout callback that currently checks child.killed) to use child.exitCode or process.kill(pid,0) and handle missing/invalid pid safely before calling child.kill('SIGKILL').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/config.js`:
- Around line 99-105: The getScriptOrder function currently prefers .js on
Windows which violates the contract to prefer .sh; update getScriptOrder(base)
so it always returns [`${base}.sh`, `${base}.js`] regardless of
process.platform, ensuring .sh is checked first for hook discovery.
- Around line 107-110: The legacy JS fallback in discoverRacers() is currently
matching hook files like setup.js/teardown.js as racer modules; update the
filtering logic in the discoverRacers() function to ignore conventional hook
filenames (setup.js, teardown.js, and their shell equivalents setup.sh,
teardown.sh) when you scan with f.endsWith('.js') (and the similar block around
the 137-141 area). Concretely, add an exclusion check for those exact basenames
before treating a file as a racer so setup/teardown scripts are not considered
racer candidates.
---
Duplicate comments:
In `@race.js`:
- Around line 591-597: The guard using child.killed after sending SIGTERM is
incorrect because ChildProcess.killed becomes true when kill() was called, not
when the child has exited, so the SIGKILL fallback may be suppressed; change the
escalation to detect whether the process is still running (e.g. check
child.exitCode === null or attempt process.kill(child.pid, 0) in a try/catch)
and only send SIGKILL if the child is still alive; update the block using the
child variable (the timeout callback that currently checks child.killed) to use
child.exitCode or process.kill(pid,0) and handle missing/invalid pid safely
before calling child.kill('SIGKILL').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44e1eca7-9ee0-4ed6-b011-88b8145b2dcb
📒 Files selected for processing (3)
cli/config.jsrace.jstests/setup-teardown.test.js
✅ Files skipped from review due to trivial changes (1)
- tests/setup-teardown.test.js
- Exclude setup/teardown hook files (setup.js, teardown.js, *.setup.js, *.teardown.js) from .js racer fallback in discoverRacers - Add settled flag in runScript to prevent double-resolve/reject when timeout fires and close event both trigger Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
race.js (1)
592-610: Minor: Clear the SIGKILL fallback timer when process closes.The SIGKILL fallback timer (lines 596-598) is not cleared when the process exits, which could result in calling
child.kill('SIGKILL')on an already-exited process. While this is harmless (kill on exited process is a no-op), it's cleaner to track and clear the timer.♻️ Suggested cleanup
let timedOut = false; let settled = false; + let sigkillTimeoutId = null; child.stdout.on('data', d => { stdout += d; }); child.stderr.on('data', d => { stderr += d; }); const timeoutId = setTimeout(() => { timedOut = true; child.kill('SIGTERM'); // Give process 5s to clean up after SIGTERM, then SIGKILL - setTimeout(() => { + sigkillTimeoutId = setTimeout(() => { if (!child.killed) child.kill('SIGKILL'); }, 5000); }, timeout); child.on('close', (code) => { clearTimeout(timeoutId); + if (sigkillTimeoutId) clearTimeout(sigkillTimeoutId); if (settled) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race.js` around lines 592 - 610, The SIGKILL fallback setTimeout created inside the main timeout handler isn't tracked or cleared, so add a variable (e.g., fallbackKillId) to store that inner timer returned by setTimeout inside the timeout handler (where child.kill('SIGTERM') is called), and then clearTimeout(fallbackKillId) in the child.on('close') handler (before returning/after clearTimeout(timeoutId)) so the SIGKILL fallback is cancelled when the process exits; adjust variable scope so fallbackKillId is accessible to both the timeout callback and the child.on('close') callback (referencing timeoutId, child.kill('SIGTERM'), child.kill('SIGKILL'), timedOut, settled, progress.done, reject).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@race.js`:
- Around line 596-598: The SIGKILL fallback's condition is wrong: replace the
check of child.killed in the setTimeout callback with a running-state check
using child.exitCode === null so the SIGKILL is sent only if the process hasn't
exited; locate the setTimeout that currently does setTimeout(() => { if
(!child.killed) child.kill('SIGKILL'); }, 5000) and change the condition to use
child.exitCode === null (optionally wrap child.kill('SIGKILL') in a try/catch if
you want to ignore errors from kill()).
---
Nitpick comments:
In `@race.js`:
- Around line 592-610: The SIGKILL fallback setTimeout created inside the main
timeout handler isn't tracked or cleared, so add a variable (e.g.,
fallbackKillId) to store that inner timer returned by setTimeout inside the
timeout handler (where child.kill('SIGTERM') is called), and then
clearTimeout(fallbackKillId) in the child.on('close') handler (before
returning/after clearTimeout(timeoutId)) so the SIGKILL fallback is cancelled
when the process exits; adjust variable scope so fallbackKillId is accessible to
both the timeout callback and the child.on('close') callback (referencing
timeoutId, child.kill('SIGTERM'), child.kill('SIGKILL'), timedOut, settled,
progress.done, reject).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bf82f1a-9f06-4209-8cff-5b85c7f37a29
📒 Files selected for processing (2)
cli/config.jsrace.js
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/config.js
- Merge with origin/main: integrate loadRaceDir(), buildMedianOutput(), pauseBetweenRuns mode with setup/teardown feature - Fix SIGKILL fallback: use child.exitCode === null instead of child.killed, track and clear sigkillTimeoutId on close/error (CodeRabbit) - Fix teardown guarantee: move setup execution inside try block so finally block always runs teardown even if setup fails (CodeRabbit/Copilot) - Fix getScriptOrder(): always prefer .sh over .js regardless of platform (CodeRabbit) - Fix SonarCloud: use explicit file mode 0o755/0o644 instead of default world-accessible permissions in test helpers - Fix macOS test: use fs.realpathSync for /var vs /private/var symlink
- Extract common runScript helper from runShellScript/runNodeScript - Use 0o700 for shell scripts (owner-only execute) - Use 0o600 for node scripts (no execute bit needed) - Simplify conditional logic in close handler https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/setup-teardown.test.js (1)
18-49:⚠️ Potential issue | 🟠 MajorThese execution tests still bypass the real script runner.
This helper writes files and calls
spawn()directly, and its contract (cmd,scriptPath,content,expectFailure) is different from the newrunScript()API. That means the suites below can stay green whilerace.jsbreaks command-object handling, relative path resolution, timeout/waitForlogic,RACE_DIRinjection, or teardown-in-finallybehavior. Please drive the exported wrapper or invokerace.jsagainst a temp race directory instead.Based on learnings: Cover changes with tests - new functionality needs tests, bug fixes need regression tests. Run npm test before considering changes complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/setup-teardown.test.js` around lines 18 - 49, The tests currently bypass the real runner by writing files and calling spawn directly in runScript/runShellScript/runNodeScript; update these helpers to either call the exported test runner wrapper or invoke the actual race.js entrypoint against a temporary race directory so the real command-object handling, relative path resolution, timeout/waitFor logic, RACE_DIR injection, and teardown-in-finally behavior are exercised. Replace the direct spawn usage inside runScript with a call that matches the new runScript API contract (preserve mode handling for shell vs node) and ensure options support expectFailure, cwd, timeout/waitFor semantics, and that env merges in RACE_DIR; update runShellScript/runNodeScript to delegate to the revised runScript wrapper so tests exercise the real runner rather than bypassing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/setup-teardown.test.js`:
- Around line 307-314: The test currently runs setup.js in tmpDir where Node
treats it as CommonJS; update the test so the hook runs under ESM like
production by creating an ESM context before calling runNodeScript: either write
a package.json with "type":"module" into tmpDir or use an .mjs module variant
for setupScript, and adjust the script content to use ESM import syntax
(referencing runNodeScript, setupScript, tmpDir, and markerFile) so the
runtime/parser path matches repo-owned races/**/setup.js hooks.
---
Duplicate comments:
In `@tests/setup-teardown.test.js`:
- Around line 18-49: The tests currently bypass the real runner by writing files
and calling spawn directly in runScript/runShellScript/runNodeScript; update
these helpers to either call the exported test runner wrapper or invoke the
actual race.js entrypoint against a temporary race directory so the real
command-object handling, relative path resolution, timeout/waitFor logic,
RACE_DIR injection, and teardown-in-finally behavior are exercised. Replace the
direct spawn usage inside runScript with a call that matches the new runScript
API contract (preserve mode handling for shell vs node) and ensure options
support expectFailure, cwd, timeout/waitFor semantics, and that env merges in
RACE_DIR; update runShellScript/runNodeScript to delegate to the revised
runScript wrapper so tests exercise the real runner rather than bypassing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad23251d-e35f-4d11-9ef1-4f7a080a4f87
📒 Files selected for processing (1)
tests/setup-teardown.test.js
- Add package.json with type:module to tmpDir so Node treats .js hooks as ESM (matching production where the repo has type:module) - Use ESM import syntax in the test fixture instead of require() - Extract shared runScript helper to deduplicate runShellScript/runNodeScript - Extract writeRacerStubs() helper to reduce repeated boilerplate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Validate waitFor config is a plain object with required url string and optional positive-number timeout/interval before entering polling loop - Check bash availability on Windows before running .sh scripts, with a clear error message suggesting .js scripts or Git Bash/WSL - Skip shell-dependent tests on Windows (via it.skipIf/describe.skipIf) - Replace throw in main() catch with process.exitCode=1 to avoid unhandled rejection stack trace after teardown runs https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
Merge conflicts resolved (imports + setup/teardown code). Security fixes in runScript(): - Path traversal: validate resolved path stays within race directory - Symlink rejection: use lstatSync instead of statSync - Timeout bounds: validate timeout is a positive finite number - NOSONAR for intentional PATH-resolved bash exec Robustness fixes: - waitFor polling: add settled flag to prevent double resolution - Refactor duplicated per-racer discovery tests to use it.each() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Check if a path is a regular file (not a directory). | ||
| */ | ||
| function isFile(filePath) { | ||
| try { | ||
| return fs.statSync(filePath).isFile(); |
There was a problem hiding this comment.
isFile() uses fs.statSync(), which follows symlinks. As a result, a symlink like setup.sh -> /tmp/outside.sh will be considered a file during discovery, but later runScript() uses lstatSync() + isFile() and will reject the same path as “not a regular file”. Consider switching isFile() to use lstatSync() (and/or explicitly reject isSymbolicLink()) so discovery and execution agree and symlink hook files don’t get selected in the first place.
| * Check if a path is a regular file (not a directory). | |
| */ | |
| function isFile(filePath) { | |
| try { | |
| return fs.statSync(filePath).isFile(); | |
| * Check if a path is a regular file (not a directory or symlink). | |
| */ | |
| function isFile(filePath) { | |
| try { | |
| const stats = fs.lstatSync(filePath); | |
| return stats.isFile() && !stats.isSymbolicLink(); |
| process.exit(1); | ||
| const phase = setupCompleted ? 'Race' : 'Setup'; | ||
| console.error(`\n${c.red}${c.bold}${phase} failed:${c.reset} ${e.message}\n`); | ||
| if (!process.exitCode) process.exitCode = 1; |
There was a problem hiding this comment.
The new error handling sets process.exitCode instead of exiting, but this file later calls process.exit(0) when settings.noServe || settings.noRecording is true. That will override the failure exit code in common CI usage (e.g. --no-serve) and report success even when setup/race failed. Consider rethrowing after logging, or guarding the final process.exit(0) so it only runs when process.exitCode is not set (or is 0).
| if (!process.exitCode) process.exitCode = 1; | |
| if (!process.exitCode) process.exitCode = 1; | |
| // Rethrow so callers can see the failure and avoid overriding it with process.exit(0) | |
| throw e; |
| const { setup: setupScript, teardown: teardownScript } = discoverSetupTeardown(raceDir, settings); | ||
|
|
||
| // Per-racer setup/teardown (e.g., lauda.setup.sh, hunt.teardown.js) | ||
| const racerScripts = racerNames.map(name => ({ | ||
| name, | ||
| ...discoverRacerSetupTeardown(raceDir, name, settings), | ||
| })); |
There was a problem hiding this comment.
settings passed into discoverSetupTeardown(raceDir, settings) has already been run through applyDefaults(), which currently strips top-level null values. That means settings.json cannot reliably use "setup": null / "teardown": null to disable convention-based discovery (the null gets dropped and discovery falls back to convention). If null is intended as a supported disable value, consider preserving null for these keys in applyDefaults, or call discovery using the raw parsed settings before defaults are applied.
| throw new Error(`${label} script path must be within race directory: ${command}`); | ||
| } | ||
|
|
||
| // Validate script exists and is a regular file (lstatSync rejects symlinks) |
There was a problem hiding this comment.
The comment says lstatSync rejects symlinks, but fs.lstatSync() does not reject symlinks—it returns a Stats object for the symlink itself. The current stat.isFile() check does effectively disallow symlinks (since isFile() will be false for a symlink), so please update the comment to match the actual behavior (or explicitly check stat.isSymbolicLink() if you want a clearer error).
| // Validate script exists and is a regular file (lstatSync rejects symlinks) | |
| // Validate script exists and is a regular file (symlinks are rejected by the isFile() check below) |
| child.on('close', (code) => { | ||
| clearTimeout(timeoutId); | ||
| if (sigkillTimeoutId) clearTimeout(sigkillTimeoutId); | ||
| if (settled) return; | ||
| settled = true; | ||
|
|
||
| if (timedOut) { | ||
| progress.done(`${label} timed out after ${timeout}ms`); | ||
| reject(new Error(`${label} script timed out after ${timeout}ms`)); | ||
| return; | ||
| } | ||
|
|
||
| if (code === 0) { | ||
| progress.done(`${label} completed`); | ||
|
|
||
| // If waitFor is specified, poll for the condition | ||
| if (waitFor) { | ||
| if (typeof waitFor !== 'object' || Array.isArray(waitFor)) { | ||
| reject(new Error(`${label} waitFor must be an object with a 'url' field`)); | ||
| return; | ||
| } | ||
| if (typeof waitFor.url !== 'string' || !waitFor.url.trim()) { | ||
| reject(new Error(`${label} waitFor.url must be a non-empty string`)); | ||
| return; | ||
| } | ||
| if (waitFor.timeout !== undefined && (!Number.isFinite(waitFor.timeout) || waitFor.timeout <= 0)) { | ||
| reject(new Error(`${label} waitFor.timeout must be a positive number`)); | ||
| return; | ||
| } | ||
| if (waitFor.interval !== undefined && (!Number.isFinite(waitFor.interval) || waitFor.interval <= 0)) { | ||
| reject(new Error(`${label} waitFor.interval must be a positive number`)); | ||
| return; | ||
| } | ||
| const { url, timeout: waitTimeout = 30000, interval = 1000 } = waitFor; | ||
| if (url) { | ||
| const waitProgress = startProgress(`Waiting for ${url}…`); | ||
| const startTime = Date.now(); | ||
| let waitSettled = false; | ||
|
|
||
| const poll = async () => { | ||
| if (waitSettled) return; | ||
| const remaining = waitTimeout - (Date.now() - startTime); | ||
| if (remaining <= 0) { | ||
| if (waitSettled) return; | ||
| waitSettled = true; | ||
| waitProgress.done(`Timeout waiting for ${url}`); | ||
| reject(new Error(`Timeout waiting for ${url} after ${waitTimeout}ms`)); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const res = await fetch(url, { | ||
| signal: AbortSignal.timeout(Math.min(remaining, interval * 2)), | ||
| }); | ||
| if (res.ok) { | ||
| if (waitSettled) return; | ||
| waitSettled = true; | ||
| waitProgress.done(`Service ready at ${url}`); | ||
| resolve(); | ||
| return; | ||
| } | ||
| } catch { | ||
| // Connection failed or timed out, will retry | ||
| } | ||
|
|
||
| if (!waitSettled) setTimeout(poll, interval); | ||
| }; | ||
| poll(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| resolve(); | ||
| } else { | ||
| progress.done(`${label} failed (exit code ${code})`); | ||
| if (stderr) console.error(`${c.dim}${stderr}${c.reset}`); | ||
| reject(new Error(`${label} script exited with code ${code}`)); | ||
| } |
There was a problem hiding this comment.
child.on('close', (code) => { ... }) can receive code === null when the process exits due to a signal. In that case the current messaging logs exit code null and throws exited with code null, which is confusing. Consider handling the signal argument from the close event and producing a clearer error message for signaled exits (and/or mapping null to a non-zero code).
Document the new setup/teardown feature including: - Convention-based script discovery (setup.sh/js, teardown.sh/js) - Per-racer scripts (name.setup.sh, name.teardown.sh) - Settings-based configuration with timeout and waitFor polling - Execution order guarantees - Updated race folder structure example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When per-racer setup scripts exist (e.g., racer-a.setup.sh), automatically use split execution so each setup runs right before that racer's runs: setupA → A₁, A₂, …, Aₙ → setupB → B₁, B₂, …, Bₙ Previously all per-racer setups ran upfront before any races. Also passes race context (name, vars) to race scripts in runner.cjs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Supported extensions: `.sh` (shell, requires bash) and `.js` (Node.js, runs as ESM). When both exist, `.sh` takes priority. | ||
|
|
There was a problem hiding this comment.
Docs say .js setup/teardown scripts “run as ESM”, but race.js executes them via node <scriptPath> with no module flags. Whether a .js file is ESM depends on the nearest package.json ("type": "module") relative to the script file, so this statement isn’t always true for arbitrary race directories. Please clarify the documentation (or adjust execution to reliably run hooks as ESM, e.g., by supporting .mjs).
| Supported extensions: `.sh` (shell, requires bash) and `.js` (Node.js, runs as ESM). When both exist, `.sh` takes priority. | |
| Supported extensions: `.sh` (shell, requires bash) and `.js` (Node.js). When both exist, `.sh` takes priority. | |
| `.js` scripts are executed with `node <scriptPath>` and will run as CommonJS or ESM according to the nearest `package.json` and Node's module resolution rules (for example, `"type": "module"`). |
| |---|---|---| | ||
| | `setup` / `teardown` | `string` or `object` | Script path (relative to race dir) or config object | | ||
| | `command` | `string` | Script path when using object form | | ||
| | `timeout` | `number` | Max execution time in ms (default: 60000) | |
There was a problem hiding this comment.
The table lists the default hook timeout as 60000ms, but runScript() defaults timeout to 300000ms. Please make the documentation and implementation consistent so users aren’t surprised by long/short default timeouts.
| | `timeout` | `number` | Max execution time in ms (default: 60000) | | |
| | `timeout` | `number` | Max execution time in ms (default: 300000) | |
| const scripts = racerFiles.map((f, i) => { | ||
| const name = racerNames[i]; | ||
| const racerScript = settings.racers?.[name]?.script; | ||
| const file = racerScript || f; | ||
| return fs.readFileSync(path.join(raceDir, file), 'utf-8'); | ||
| }); | ||
|
|
||
| const ctx = buildRaceContext({ racerNames, scripts, settings, rootDir: __dirname, raceDir, racerFiles }); |
There was a problem hiding this comment.
In loadRaceDir(), settings.racers?.[name]?.script can override which file is read into scripts[], but ctx.racerFiles is still the originally discovered racerFiles. Downstream, copyRaceAssets() and the results player use ctx.racerFiles to copy/display race scripts, so the saved results can show/copy the wrong source files when an override is used. Consider computing an effectiveRacerFiles array that matches what you actually loaded (or disallow racers.*.script here) and pass that into buildRaceContext() so results always reflect the executed script files.
| const scripts = racerFiles.map((f, i) => { | |
| const name = racerNames[i]; | |
| const racerScript = settings.racers?.[name]?.script; | |
| const file = racerScript || f; | |
| return fs.readFileSync(path.join(raceDir, file), 'utf-8'); | |
| }); | |
| const ctx = buildRaceContext({ racerNames, scripts, settings, rootDir: __dirname, raceDir, racerFiles }); | |
| // Determine the effective script file for each racer, taking into account | |
| // any per-racer script overrides from settings.racers.*.script. | |
| const effectiveRacerFiles = racerFiles.map((f, i) => { | |
| const name = racerNames[i]; | |
| const racerScript = settings.racers?.[name]?.script; | |
| return racerScript || f; | |
| }); | |
| const scripts = effectiveRacerFiles.map(file => { | |
| return fs.readFileSync(path.join(raceDir, file), 'utf-8'); | |
| }); | |
| const ctx = buildRaceContext({ racerNames, scripts, settings, rootDir: __dirname, raceDir, racerFiles: effectiveRacerFiles }); |
| const phase = setupCompleted ? 'Race' : 'Setup'; | ||
| console.error(`\n${c.red}${c.bold}${phase} failed:${c.reset} ${e.message}\n`); | ||
| if (!process.exitCode) process.exitCode = 1; | ||
| } finally { |
There was a problem hiding this comment.
main() now sets process.exitCode = 1 on failure, but the module still calls process.exit(0) later when settings.noServe || settings.noRecording is true. That unconditional process.exit(0) will mask failures in those modes (exit status 0 even when setup/race failed). Update the exit logic so an existing non-zero process.exitCode is preserved (e.g., only process.exit(0) when no error occurred).
| throw new Error(`${label} script path must be within race directory: ${command}`); | ||
| } | ||
|
|
||
| // Validate script exists and is a regular file (lstatSync rejects symlinks) |
There was a problem hiding this comment.
The comment says “lstatSync rejects symlinks”, but fs.lstatSync() does not reject symlinks; it returns a Stats object for the symlink itself. The actual symlink rejection here comes from the subsequent stat.isFile() check (which will be false for a symlink). Please adjust the comment to describe the real behavior to avoid confusion during future security reviews.
| // Validate script exists and is a regular file (lstatSync rejects symlinks) | |
| // Validate script exists and is a regular (non-symlink) file: | |
| // lstatSync retrieves stats for the path, and the isFile() check below rejects symlinks and other non-regular types. |
- Fix lstatSync comment: clarify isFile() rejects symlinks, not lstatSync - Guard process.exit(0) to preserve failure exit codes (use exitCode || 0) - Fix racerFiles mismatch: compute effectiveRacerFiles after script overrides - Fix timeout documentation: default is 300000ms, not 60000ms - Clarify ESM docs: .js module type depends on nearest package.json - Handle null exit code from signals with clearer error messages - Preserve null values in applyDefaults for setup/teardown to allow explicit disabling of convention-based discovery - Use lstatSync in discovery isFile() for consistent symlink rejection https://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg
|
Summary
This PR adds support for global and per-racer setup/teardown scripts in race directories. Scripts can be discovered by convention (setup.sh, setup.js, teardown.sh, teardown.js) or configured via settings.json, and are executed before and after races with proper error handling and timeout support.
Key Changes
Setup/Teardown Discovery Functions (
cli/config.js)discoverSetupTeardown()to find global setup/teardown scripts by conventiondiscoverRacerSetupTeardown()to find per-racer setup/teardown scripts.shover.jsfilesScript Execution (
race.js)runScript()function to execute setup/teardown scripts{ command, timeout, waitFor }RACE_DIRenvironment variable to scriptswaitForpolling for service readiness (e.g., health check URLs)Integration (
race.js)Comprehensive Test Coverage (
tests/setup-teardown.test.js,tests/race-discovery.test.js)Notable Implementation Details
setup/teardownto a string, object, null, or falsesettings.racers.{name}.setup/teardownonly affect that racerRACE_DIRenvironment variable for contexthttps://claude.ai/code/session_01ExiTYEscL4QVvKcc3Lh6qg