refactor: add a run-listener observer and adopt it in the cli#156
Merged
Conversation
Introduce RunListener — an Observer over the run lifecycle — so reporters, progress UIs, and telemetry become listeners and a new output format needs no engine edits. The engine keeps its single ProgressEvent stream; dispatchProgress adapts it to the typed hooks. - core/src/execute/runListener.ts: RunListener interface (all hooks optional) with payloads derived from ProgressEvent (Omit<Extract<…>>), so they can't drift; a notifyListeners helper that isolates a throwing listener (logged and skipped) so a buggy reporter/telemetry adapter can never abort the run; dispatchProgress with a compile-time exhaustiveness guard. - core/src/execute/runAll.ts: listeners?: RunListener[] on RunAllOptions (additive); notify fans out to onProgress AND listeners; onRunStart before the loop, onRunFinish after the report, and onRunError in a catch so the lifecycle is symmetric (onRunStart always pairs with a terminal hook). - ProgressEvent moves to execute/types.ts (single source of truth) and is re-exported from runAll.ts, breaking the runListener<->runAll module cycle while keeping every existing importer working. - runners/cli: ConsoleProgressListener implements RunListener; run.ts swaps its inline onProgress switch for listeners: [new ConsoleProgressListener()] — the first adopter, proving the pattern end-to-end. Same progress output. - lib/verdictIcon.ts: shared verdict->glyph helper, replacing copy-pasted ternaries in the listener and evaluatorLoop. Backward-compatible: onProgress fires exactly as before; listeners is additive; the SDK's own ProgressEvent type is untouched. New tests pin the event->hook mapping and that a throwing listener is isolated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesRun lifecycle listener support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SDK
participant run
participant runAll
participant notifyListeners
participant dispatchProgress
participant MCP
SDK->>run: options.listeners
run->>runAll: listeners + onProgress
runAll->>notifyListeners: onRunStart / onRunFinish / onRunError
runAll->>MCP: connect and trace curation
runAll->>dispatchProgress: ProgressEvent
dispatchProgress->>notifyListeners: evaluator/attack hooks
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
jithin23-kv
reviewed
Jul 2, 2026
| (report as UnifiedRunReport & { stopReason?: string }).stopReason = stopReason; | ||
| } | ||
|
|
||
| notifyListeners(listeners, (listener) => listener.onRunFinish?.(report)); |
Collaborator
There was a problem hiding this comment.
buildReport and this finish hook are outside the try/catch, so if report-building throws, a listener gets onRunStart but no finish or error.
Collaborator
Author
There was a problem hiding this comment.
Fixed in d62623a — report-building and onRunFinish are now inside the try, so onRunError covers a throw from buildReport too. onRunStart is now always paired with exactly one terminal hook.
| onAttackDone?(info: Payload<"attack_done">): void; | ||
| onEvaluatorDone?(info: Payload<"evaluator_done">): void; | ||
| /** Fired when a non-retryable error halts the run gracefully (partial report). */ | ||
| onRunStopped?(info: Payload<"run_stopped">): void; |
Collaborator
There was a problem hiding this comment.
On a stop, listeners get both onRunStopped and then onRunFinish — this one isn't the final signal.
Collaborator
Author
There was a problem hiding this comment.
Clarified in d62623a. This is intentional — a reporter should still receive the partial report on a stop — but the docs were misleading. onRunStopped is now documented as a non-terminal notice (onRunFinish still follows with the partial report); onRunFinish/onRunError are the two terminal hooks.
| // evaluatorCount is the attack-evaluator count; MCP baseline pre-flight scans | ||
| // (run below) contribute extra results not reflected here. | ||
| notifyListeners(listeners, (listener) => | ||
| listener.onRunStart?.({ evaluatorCount: ordered.length }) |
Collaborator
There was a problem hiding this comment.
This fires after the MCP connect + trace curation, so anything watching for the run to begin won't see it during those steps.
Collaborator
Author
There was a problem hiding this comment.
Fixed in d62623a — onRunStart now fires at the true start, before the MCP connect and trace curation, so watchers see the run begin during those steps. Everything after it moved inside the try to keep the lifecycle symmetric (and the finally now also closes the MCP target if listTools() throws after connect).
Address review feedback on the RunListener wiring: - onRunStart now fires at the true start, before the MCP connect and trace curation, so watchers see the run begin during those (possibly slow) steps. - The MCP connect, trace curation, baseline scans, attack loop, AND report building now all run inside the try, so onRunError covers a throw from any of them — onRunStart is always paired with exactly one terminal hook (previously buildReport/onRunFinish sat outside the try and could be skipped on a throw). - As a side effect the finally now closes the MCP target even when listTools() throws after a successful connect (a latent leak). - Clarify the lifecycle contract in the interface docs: onRunStopped is a non-terminal notice (onRunFinish still follows with the partial report); onRunFinish and onRunError are the terminal hooks. Behavior-preserving for the success and graceful-stop paths (smoke/equivalence green); only the error path and event ordering change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@core/src/execute/runAll.ts`:
- Around line 77-86: Move the lifecycle guard in runAll so resolveModel,
resolveEvaluators, and topoSortEvaluators are inside the same try/catch as the
rest of the run setup, or wrap them with an outer catch, so any pre-run failure
still triggers onRunError and the listener contract is preserved. Keep
onRunStart as the first lifecycle event, but ensure the try begins before the
setup work and that the terminal error hook is reached for every throw.
In `@core/src/execute/runListener.ts`:
- Around line 20-31: `notifyListeners` only guards synchronous throws, so
rejected promises from async hooks can still escape and break listener
isolation. Update the `notifyListeners` flow in `runListener.ts` to await or
otherwise normalize each listener result so both thrown errors and promise
rejections are caught and logged. Keep the warning tied to the specific listener
method being invoked, and make the message actionable by telling the listener
author to fix that hook’s implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d2d8968-ff76-407f-9131-d988a7993ae9
📒 Files selected for processing (2)
core/src/execute/runAll.tscore/src/execute/runListener.ts
…steners
Address review feedback on the RunListener wiring:
- runAll now runs model + evaluator resolution and the topo-sort INSIDE the try, so
a failure there reaches onRunError too (previously they ran before the try and
bypassed the terminal hook). onRunStart still fires once the evaluator count is
known, just before the MCP connect; a setup failure throws before it, so
listeners get onRunError with no preceding onRunStart — documented in the contract.
- notifyListeners now isolates async listeners: a hook that rejects its returned
promise (not just a synchronous throw) is caught and skipped, so a buggy
reporter/telemetry adapter still can't abort the run. The warning is actionable
("fix the listener implementation").
Behavior-preserving for successful and graceful-stop runs (smoke/equivalence green).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
I think we need to do the same in the MCP runner ( The SDK currently only exposes We can merge this PR and do it separately if neeeded |
…he sdk Extend the RunListener adoption beyond the CLI, per review: - runners/mcp: the MCP runner had the same inline onProgress switch this PR replaced in the CLI. Convert it to a RunListener (onEvaluatorStart / onEvaluatorDone) so both runners consume engine events through the SPI. - runners/sdk: run() only forwarded onProgress (per-attack events). Add a listeners?: RunListener[] option, forward it to runAll, and re-export the RunListener type so SDK users can observe the full run lifecycle (onRunStart / onRunFinish / onRunError), not just per-attack progress. Additive and backward-compatible: onProgress is unchanged on every path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Both valid — done in 272d7fc.
|
achuvyas-kv
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Introduce
RunListener— an Observer over the run lifecycle — so reporters,progress UIs, and telemetry become listeners and a new output format needs
no engine edits. The engine keeps its single
ProgressEventstream;dispatchProgressadapts it to typed hooks.core/src/execute/runListener.ts(new) —RunListener(all hooks optional:onRunStart/onEvaluatorStart/onAttackStart/onAttackDone/onEvaluatorDone/onRunStopped/onRunError/onRunFinish) with payloads derived fromProgressEvent(Omit<Extract<…>>) so they can't drift;notifyListeners(error-isolating fan-out);
dispatchProgresswith a compile-time exhaustiveness guard.runAll.ts—listeners?: RunListener[](additive);notifyfans out toonProgressand listeners;onRunStartbefore the loop,onRunFinishafterthe report,
onRunErrorin acatch.ConsoleProgressListener implements RunListener;run.tsswapsits inline
onProgressswitch forlisteners: [new ConsoleProgressListener()],proving the pattern end-to-end (identical ▶/✓/✗/⚠ output).
lib/verdictIcon.ts— shared verdict→glyph helper.Robustness (from the review)
abort the run (critical for telemetry/reporter adopters that do I/O).
onRunStartalways pairs with a terminal hook:onRunFinish(success/stop) oronRunError(unexpected throw).ProgressEventmoved toexecute/types.ts(single sourceof truth), re-exported from
runAll.ts;runListenerimports fromtypes.js.ProgressEventvariant is a compile error, not asilently-dropped event.
Backward compatibility
onProgressfires exactly as before;listenersis purely additive; theProgressEventre-export keeps every existing importer working; the SDK's ownProgressEventis untouched.Scope
This is the create + adopt-in-engine increment. Reporter/telemetry-as-listener
adoption is deliberately deferred (the HTML/JSON report is written outside
runAlltoday).
baselineScanner's two verdict-icon spots were left to avoid a conflictwith in-flight #155 — a small follow-up.
Verification
full suite 124 tests, 0 fail (3 skips) · typecheck 0 · lint 0 · build OK.
🤖 Generated with Claude Code
Summary by CodeRabbit
RunListener, enabling observers for run start, finish, and error events.listenerssupport in run options; progress updates are now delivered to both the existing progress callback and registered listeners.RunListenerhooks.