Skip to content

Add --fail-fast flag for early cancellation on test failure#111

Merged
DanverImbue merged 18 commits intomainfrom
worktree-fail-fast
Mar 18, 2026
Merged

Add --fail-fast flag for early cancellation on test failure#111
DanverImbue merged 18 commits intomainfrom
worktree-fail-fast

Conversation

@jsk11235
Copy link
Copy Markdown
Collaborator

@jsk11235 jsk11235 commented Mar 17, 2026

Summary

  • Add --fail-fast CLI flag to offload run that cancels all remaining batches and terminates sandboxes when a test failure is detected
  • Reuses the existing CancellationToken infrastructure — in-flight executions abort via select! against token.cancelled(), queued batches are skipped at pull time
  • Failed batch results are written to the shared MasterJunitReport before cancellation triggers, so the failing test's results are always captured in the final junit.xml

Test plan

  • 4 new unit tests for MasterJunitReport::has_any_failures() (empty, all-pass, with-failure, flaky-not-failure)
  • cargo fmt --check passes
  • cargo clippy --all-targets --all-features passes
  • cargo nextest run passes (132/132)
  • Manual: offload run --fail-fast with a failing test confirms early cancellation and correct junit.xml output

🤖 Generated with Claude Code

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 1 issue.


[commit_message_mismatch] (severity 3/5) (confidence 0.92)

The diff includes test output artifacts (test-results-fail-fast/logs/batch-*.log) that appear to be generated from a manual test run and should not be committed to the repository. These contain machine-specific paths (e.g., /Users/jacobkirmayer/imbue/offload/.claude/worktrees/fail-fast) and are transient build artifacts.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 1 issue.

@@ -236,7 +245,7 @@ mod tests {
let fw = PytestFramework::new(config)?;
let record = TestRecord::new("tests/test_a.py::test_one", "test-group");
let tests = vec![TestInstance::new(&record)];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[test_coverage] (severity 2/5) (confidence 0.85)

The diff adds fail-fast flag support to the framework execution commands (pytest -x, cargo nextest --fail-fast, vitest --bail), but there are no unit tests verifying that these flags are correctly added to the command when fail_fast: true is passed. The existing tests in pytest.rs, vitest.rs only pass false for the new parameter. Tests should verify the fail_fast: true path produces the correct command arguments.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 1 issue.

@@ -236,7 +245,7 @@ mod tests {
let fw = PytestFramework::new(config)?;
let record = TestRecord::new("tests/test_a.py::test_one", "test-group");
let tests = vec![TestInstance::new(&record)];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[test_coverage] (severity 3/5) (confidence 0.90)

The diff adds fail-fast flag threading through multiple framework implementations (pytest -x, cargo nextest --fail-fast, vitest --bail), but no tests verify that these framework-specific flags are correctly added to the command when fail_fast=true. The existing tests were only updated to pass false. Tests for fail_fast=true should be added for each framework.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

@jsk11235 jsk11235 force-pushed the worktree-fail-fast branch from 67f342c to b933322 Compare March 18, 2026 17:45
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 0 issues.

Copy link
Copy Markdown
Collaborator

@DanverImbue DanverImbue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 1 issue.


[test_coverage] (severity 3/5) (confidence 0.80)

The user request mentions '4 new unit tests for MasterJunitReport::has_any_failures()' but the diff does not include any tests for has_any_failures(). The diff adds tests for framework-level fail-fast flags (pytest -x, nextest --fail-fast, vitest --bail) but the has_any_failures() method tests mentioned in the test plan are missing from the diff.

jacobkirmayer-imbue and others added 15 commits March 18, 2026 12:40
…e-106)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add --fail-fast bool flag to the Run CLI command and thread it through
the full call chain: run_tests() → dispatch_framework() → run_all_tests()
→ Orchestrator::new() → SpawnConfig. The flag is plumbed but not yet
active; cancellation logic follows in the next commit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cation

If --fail-fast works, the run finishes in seconds. Without it, 10 minutes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When --fail-fast is enabled, inject the framework's native stop-on-failure
flag into the sandbox command: pytest gets -x, cargo nextest gets
--fail-fast (replacing --no-fail-fast), vitest gets --bail. The default
framework is unchanged since the user controls run_command directly.

Also bump ratchets budget for examples/tests_fail time.sleep usage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jacobkirmayer-imbue and others added 3 commits March 18, 2026 12:41
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vet found 1 issue.


[commit_message_mismatch] (severity 3/5) (confidence 0.90)

The user request explicitly mentions '4 new unit tests for MasterJunitReport::has_any_failures() (empty, all-pass, with-failure, flaky-not-failure)' in the test plan, and issue code-105 describes adding a 'has_any_failures() method to MasterJunitReport in junit.rs'. However, the diff does not contain any changes to junit.rs - neither the has_any_failures() method implementation nor the 4 unit tests for it. The diff only contains framework-level tests for the fail-fast flag in command generation. The fail-fast cancellation logic in spawn.rs relies on BatchOutcome::Failure matching, but there are no integration-level tests verifying the fail-fast cancellation behavior in spawn_task either.

@DanverImbue DanverImbue merged commit c105bfa into main Mar 18, 2026
4 checks passed
@DanverImbue DanverImbue deleted the worktree-fail-fast branch March 18, 2026 20:57
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.

3 participants