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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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->>CLI: 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 -->
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
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
opfor run.stopReason: "user-interrupted").