feat: graceful shutdown on ctrl+c during opfor run#211
Conversation
Thread an AbortSignal through RunAllOptions → EvaluatorLoopContext so the evaluator loop checks for cancellation between evaluators and between attacks. The in-flight attack finishes, completed results are kept, and a partial report is written with stopReason: "user-interrupted". CLI behavior: - First Ctrl+C: abort signal fires, engine finishes current work, writes a partial report, exits cleanly. - Second Ctrl+C: process.exit(130) — immediate force kill. - Handler is cleaned up in a finally block. Also fixes the customer-support test agent Dockerfile which referenced a workspace not registered in the root package.json. Closes #209 Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughAdds graceful Ctrl+C cancellation to ChangesGraceful run cancellation
Customer-support E2E build
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runAll
participant EvaluatorLoop
CLI->>runAll: start with AbortSignal
runAll->>EvaluatorLoop: execute evaluators and attacks
CLI->>AbortSignal: abort on first Ctrl+C
EvaluatorLoop-->>runAll: return partial results
runAll-->>CLI: partial report with user-interrupted stopReason
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)tests/e2e/agents/customer-support/DockerfileTraceback (most recent call last): tests/e2e/agents/customer-support/docker-compose.ymlTraceback (most recent call last): 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/execute/evaluatorLoop.ts (1)
237-238: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate
stopReasonif interrupted during the last evaluator.If the user interrupts the run during the final evaluator's attacks, the inner loop
breaks, and the outer loop finishes its final iteration. The function then reaches this return statement without returning astopReasonor emitting therun_stoppedevent. This causes the CLI to silently treat the cancelled run as completed.Check the signal state at the end of the function to capture this scenario.
🐛 Proposed fix
- return { evaluatorResults }; + if (signal?.aborted) { + const stopReason = "user-interrupted"; + notify({ type: "run_stopped", reason: stopReason }); + return { evaluatorResults, stopReason }; + } + + return { evaluatorResults };🤖 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 `@core/src/execute/evaluatorLoop.ts` around lines 237 - 238, Update the evaluator loop function before its final return to check the signal/interruption state after the last evaluator completes. If interrupted, propagate the appropriate stopReason and emit the run_stopped event using the existing interruption-handling path; otherwise preserve the current evaluatorResults return behavior.
🧹 Nitpick comments (2)
tests/e2e/agents/customer-support/Dockerfile (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
npm ciinstead ofnpm installfor reproducible builds.If a
package-lock.jsonis available in this local directory, usingnpm ciis recommended in CI/CD and Docker environments to ensure a clean, reproducible installation matching the lockfile exactly.♻️ Proposed refactor
-RUN npm install --ignore-scripts +RUN npm ci --ignore-scripts🤖 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 `@tests/e2e/agents/customer-support/Dockerfile` around lines 7 - 11, Update the Dockerfile’s dependency installation command from npm install to npm ci while preserving the existing --ignore-scripts option, so the build uses the package-lock.json deterministically.docs/cli.md (1)
210-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Adding a language tag (like
text) resolves markdown linter warnings and improves rendering consistency.♻️ Proposed fix
-``` +```text ⚠️ Run interrupted — results are partial. Re-run for a complete assessment.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/cli.mdaround lines 210 - 212, Update the fenced code block containing
the interrupted-run message in the CLI documentation to specify the text
language, using a text fence while preserving the message content unchanged.</details> <!-- cr-comment:v1:8cbc233c1a71209bd5541851 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@runners/cli/src/commands/run.ts:
- Around line 222-223: Update the isPartial calculation in the run command to
return true only when report.stopReason equals "user-interrupted", rather than
for any truthy stop reason. Preserve the existing conditional flow so
target-related stop reasons continue to reach the later unreachable-target
warnings.
Outside diff comments:
In@core/src/execute/evaluatorLoop.ts:
- Around line 237-238: Update the evaluator loop function before its final
return to check the signal/interruption state after the last evaluator
completes. If interrupted, propagate the appropriate stopReason and emit the
run_stopped event using the existing interruption-handling path; otherwise
preserve the current evaluatorResults return behavior.
Nitpick comments:
In@docs/cli.md:
- Around line 210-212: Update the fenced code block containing the
interrupted-run message in the CLI documentation to specify the text language,
using a text fence while preserving the message content unchanged.In
@tests/e2e/agents/customer-support/Dockerfile:
- Around line 7-11: Update the Dockerfile’s dependency installation command from
npm install to npm ci while preserving the existing --ignore-scripts option, so
the build uses the package-lock.json deterministically.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `77c2dc9d-02cc-4afa-b89b-e71be93b73a7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c8c9f72c4d7246716208804457226a687dc31a89 and db11d39f273258c9dd9d97310ade5b8f942e275e. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `AGENTS.md` * `core/src/execute/evaluatorLoop.ts` * `core/src/execute/runAll.ts` * `docs/cli.md` * `runners/cli/src/commands/run.ts` * `tests/e2e/agents/customer-support/Dockerfile` * `tests/e2e/agents/customer-support/docker-compose.yml` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| const isPartial = !!(report as { stopReason?: string }).stopReason; | ||
| if (isPartial) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow isPartial to avoid suppressing target-error warnings.
The engine can also populate stopReason with target-related error messages (e.g., when a TargetStopError halts the run early). Checking for any truthy stopReason will falsely flag error-halted runs as "user-interrupted" and suppress the helpful else if warnings at the bottom about the target potentially being unreachable.
Check specifically for "user-interrupted".
🐛 Proposed fix
- const isPartial = !!(report as { stopReason?: string }).stopReason;
+ const isPartial = (report as { stopReason?: string }).stopReason === "user-interrupted";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isPartial = !!(report as { stopReason?: string }).stopReason; | |
| if (isPartial) { | |
| const isPartial = (report as { stopReason?: string }).stopReason === "user-interrupted"; | |
| if (isPartial) { |
🤖 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 `@runners/cli/src/commands/run.ts` around lines 222 - 223, Update the isPartial
calculation in the run command to return true only when report.stopReason equals
"user-interrupted", rather than for any truthy stop reason. Preserve the
existing conditional flow so target-related stop reasons continue to reach the
later unreachable-target warnings.
1 participant
Problem
opfor runhas no SIGINT handler. When a user presses Ctrl+C mid-run, Node terminates immediately — all progress is lost, no partial report is written, therunAllfinally block (MCP target cleanup) may not execute, and the--eventsNDJSON stream is not flushed.Solution
Thread an
AbortSignalthrough the core engine so the evaluator loop checks for cancellation at safe points (between evaluators, between attacks). The in-flight attack is allowed to finish, completed results are preserved, and a partial report is written withstopReason: "user-interrupted".The CLI wires SIGINT to the abort controller with two-press behavior:
process.exit(130)— immediate force kill.The
signallives onRunAllOptions(notRunConfig), keeping the core engine transport-agnostic — the SDK can reuse it for programmatic cancellation.Changes
core/src/execute/runAll.ts—signal?: AbortSignaladded toRunAllOptions, passed through torunEvaluatorAttackscore/src/execute/evaluatorLoop.ts— Two abort checkpoints: before each evaluator, before each attack within an evaluatorrunners/cli/src/commands/run.ts— SIGINT handler with two-press behavior, partial report messaging, handler cleanup infinallytests/e2e/agents/customer-support/Dockerfile— Fixed broken build (referenced workspace not in rootpackage.json); simplified to standalone installtests/e2e/agents/customer-support/docker-compose.yml— Build context scoped to agent directorydocs/cli.md— Added "Graceful shutdown (Ctrl+C)" sectionAGENTS.md— Added cancellation note to run loop docsIssue
Closes #209
How to test
opfor run --config tests/e2e/agents/customer-support/opfor.config.json # wait for an attack to start, then press Ctrl+C"Caught interrupt — finishing in-flight attack, skipping remaining evaluators…".opfor/reports/"Run interrupted — results are partial. Re-run for a complete assessment."Screenshots
Made with Cursor
Summary by CodeRabbit
New Features
Documentation
Chores