Skip to content

fix(curiocity): add command evaluator timeout - #338

Open
atomicdjt wants to merge 2 commits into
griddynamics:mainfrom
atomicdjt:fix/302-command-evaluator-timeout
Open

fix(curiocity): add command evaluator timeout#338
atomicdjt wants to merge 2 commits into
griddynamics:mainfrom
atomicdjt:fix/302-command-evaluator-timeout

Conversation

@atomicdjt

@atomicdjt atomicdjt commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #302

Summary

  • add bounded execution to the Curiocity command evaluator;
  • expose the established timeoutSec option with a 60-second default;
  • preserve the command evaluator's existing clean pass: false failure semantics.

Root cause

command invoked ctx.exec(..., { shell: true }) without a timeout, so a hanging build, test, or lint command could block a trial indefinitely. The sibling external evaluator provides the repository precedent for timeout configuration and Execa timeout reporting.

Behavior

timeoutSec must be positive and defaults to 60 seconds. A timeout is returned as a normal failed EvalResult with details identifying the configured duration; it does not throw, preserving the command evaluator's existing failure contract. The evaluator entry's configured gate value remains owned by the pipeline and is applied there.

With shell: true, terminating the shell alone does not reliably terminate descendants that retain inherited output handles. Hosted Ubuntu CI exposed this after the initial Execa-only timeout implementation continued waiting on a nested child. The final implementation therefore creates a detached POSIX process group and terminates that group at the configured deadline; on Windows it uses taskkill /t /f. Execa retains a short reporting grace period after tree termination.

Tests

  • npm run lint — pass
  • npm run build — pass
  • focused command evaluator tests — 9 pass, 13 skipped by the targeted filter
  • git diff --check — pass
  • npm test on the local Windows host — 414 pass, 23 pre-existing platform-sensitive failures in PTY/path tests and the POSIX-only diagnostic command; all new timeout tests pass
  • the repository's Ubuntu CI workflow (npm ci, npm run lint, npm run build, npm run test) is the hosted verification gate

The focused regression coverage proves real bounded execution, default and fractional timeout handling, invalid timeout rejection, and unchanged success/ordinary nonzero-exit behavior.

Compatibility and scope

Existing command definitions without timeoutSec continue to use the 60-second default. Tiny positive values are clamped to a 1 ms Execa deadline because Execa treats a zero timeout as disabled. The external evaluator and unrelated Curiocity behavior are unchanged.

Signed-off-by: David Turner <davidelsey9513@gmail.com>
Signed-off-by: David Turner <davidelsey9513@gmail.com>
@atomicdjt

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ROSETTA] curiocity command evaluator has no execution timeout

1 participant