refactor: split the run orchestrator god-function into phase modules#154
Merged
Conversation
runAll was a ~275-line function (depth-10 nesting, a labeled break) fusing five jobs. Extract the two big inline blocks so runAll becomes a thin orchestrator — resolve -> preflight -> execute -> report — dropping 843 -> 278 lines: - baselineScanner.ts: the MCP pre-flight scans as a Chain of Responsibility (runBaselineScans + the resource/tool-description/rug-pull scanners). Each scanner contributes an EvaluatorResult or nothing; runAll pushes them in order. - evaluatorLoop.ts: runEvaluatorAttacks — the evaluator attack loop. The labeled `break evaluatorLoop` becomes a return; captureSessionContext moves in. The two verbatim-duplicated stop-error branches (isStopError vs TargetStopError) and the thrice-repeated evaluatorMeta object are consolidated (DRY), and makeFailedResult now uses errorJudge(). Backward-compatible: runAll(config, options) keeps its exact signature, so all callers (CLI, SDK, MCP, browser) are untouched. The RunEngine/RunRequest boundary from the plan is intentionally deferred — it would rewire the front-ends and change the public API. Behavior-preserving: evaluatorResults ordering (scans then evaluators), partial-report-on-stop, mcpTarget.close() in a finally, topo-sort, dependency-skip, and upstream-session threading are all reproduced; the runAll.smoke and orchestrator.equivalence drift guards stay green. New baselineScanner test covers the chain (the MCP pre-flight had no coverage). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR extracts MCP baseline pre-flight scanning and the evaluator attack loop from ChangesBaseline Scan and Evaluator Loop Extraction
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
core/src/execute/evaluatorLoop.ts (3)
200-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated turn-formatting logic between the multi-turn and single-turn fallback branches.
The
t.kind === "agent"vs. tool-call formatting (lines 210-219) is re-implemented almost identically in ther.kind === "agent"/r.kind === "mcp"fallback branches (lines 221-230). A small helper (e.g.formatTurnPair(kind, prompt/toolName/toolArguments, response)) would remove the duplication and prevent the two paths from drifting if the formatting ever changes.🤖 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 200 - 244, The session history formatting in captureSessionContext is duplicated between the multi-turn loop and the single-turn fallback branches, so refactor the shared user/assistant pair construction into a small helper near captureSessionContext and reuse it for both the t.kind === "agent" / tool-call path and the r.kind === "agent" / r.kind === "mcp" path. Keep the helper responsible for formatting prompt versus [tool:...] content so the behavior stays consistent and only one place needs updates if the history format changes.
133-144: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-null/type assertions bypass the invariant they depend on.
mcpTarget!(line 135) andconfig.target as AgentTargetConfig(line 142) both assumeattack.kindcorrectly predicts target shape/availability, but nothing in this function enforces that. If that invariant is ever violated (e.g. a future evaluator misconfiguration), the failure surfaces as a generic null-dereference/type error deep inrunMcpAttack/createAgentTargetrather than a message telling the caller what's wrong.🛡️ Suggested guard
result = attack.kind === "mcp" - ? await runMcpAttack(attack, mcpTarget!, attackModel, judgeLlmConfig) + ? await runMcpAttack( + attack, + mcpTarget ?? (() => { throw new Error(`MCP attack "${attack.id}" has no connected mcpTarget`); })(), + attackModel, + judgeLlmConfig + ) : await runAgentAttack(...)🤖 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 133 - 144, The evaluatorLoop execution path is relying on unsafe non-null/type assertions for the target selection logic. Update the attack.kind branch in evaluatorLoop to explicitly validate that the MCP path has a real mcpTarget and that the agent path has a valid AgentTargetConfig before calling runMcpAttack or createAgentTarget. If the expected target shape is missing or mismatched, fail early with a clear, descriptive error that names the attack kind and the missing/invalid target instead of using mcpTarget! or config.target as AgentTargetConfig.
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType-only import creates a reverse dependency on the orchestrator.
ProgressEventis imported from./runAll.js(the file that now importsrunEvaluatorAttacksfrom this module per therunAll.tssnippet). Since it'simport type, it's erased at compile time so there's no runtime cycle, but it still couples this extracted module back to the orchestrator it was pulled out of, undermining the stated goal of keepingrunAll.tsa thin orchestrator on top of lower-level modules. Consider hoistingProgressEventintotypes.ts(alongsideRunConfig,SessionContext, etc.) soevaluatorLoop.ts,baselineScanner.ts, andrunAll.tsall import it from a shared, dependency-free location.♻️ Suggested fix
-import type { ProgressEvent } from "./runAll.js"; +import type { ProgressEvent } from "./types.js";And move the
ProgressEventinterface definition fromrunAll.tsintotypes.ts, updatingrunAll.ts's own import accordingly.🤖 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 19 - 28, The evaluatorLoop.ts imports ProgressEvent from runAll.ts, which creates an unnecessary dependency back to the orchestrator. Move the ProgressEvent type definition into the shared types.ts module alongside RunConfig and SessionContext, then update evaluatorLoop.ts, baselineScanner.ts, and runAll.ts to import it from that shared location so runAll.ts stays a thin top-level coordinator.
🤖 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/baselineScanner.ts`:
- Around line 295-296: The baseline update flow in baselineScanner should not
overwrite the trusted baseline immediately after detecting drift or a rug pull;
keep the existing baseline unchanged and store the new snapshot as a separate
candidate instead. Update the logic around writeFile in the baselineScanner flow
so currentJson is written only to a candidate/temporary artifact, and reserve
any baselinePath overwrite for an explicit accept path. Apply the same change to
the other update site referenced in the scanner logic so both paths use the same
approved-baseline behavior.
- Around line 221-227: The drift check in baselineScanner currently fails on raw
JSON hash mismatches even when computeToolDiffs returns no real changes, so
reorder-only tools/list responses can incorrectly fail. Update the drift path
around currentSnapshot/currentHash and the FAIL decision to use semantic
comparison first: either canonicalize the tool snapshot before hashing or, at
minimum, treat computeToolDiffs(...).length === 0 as PASS before marking drift.
Keep the logic in baselineScanner aligned with the parsing/diffing flow so
snapshot order does not trigger false failures.
- Around line 286-290: The baseline parsing in baselineScanner should not assume
the stored JSON is valid. In the logic that reads baselineJson and builds
baselineSnapshot, first parse as unknown, validate the result with the existing
Zod schema, and if validation fails return an ERROR result with a clear
invalid-baseline message instead of letting JSON.parse or shape mismatches abort
the scan. Keep the fix localized around the baselineSnapshot creation path so
attack_done still gets emitted and progress state is not left hanging after
attack_start.
---
Nitpick comments:
In `@core/src/execute/evaluatorLoop.ts`:
- Around line 200-244: The session history formatting in captureSessionContext
is duplicated between the multi-turn loop and the single-turn fallback branches,
so refactor the shared user/assistant pair construction into a small helper near
captureSessionContext and reuse it for both the t.kind === "agent" / tool-call
path and the r.kind === "agent" / r.kind === "mcp" path. Keep the helper
responsible for formatting prompt versus [tool:...] content so the behavior
stays consistent and only one place needs updates if the history format changes.
- Around line 133-144: The evaluatorLoop execution path is relying on unsafe
non-null/type assertions for the target selection logic. Update the attack.kind
branch in evaluatorLoop to explicitly validate that the MCP path has a real
mcpTarget and that the agent path has a valid AgentTargetConfig before calling
runMcpAttack or createAgentTarget. If the expected target shape is missing or
mismatched, fail early with a clear, descriptive error that names the attack
kind and the missing/invalid target instead of using mcpTarget! or config.target
as AgentTargetConfig.
- Around line 19-28: The evaluatorLoop.ts imports ProgressEvent from runAll.ts,
which creates an unnecessary dependency back to the orchestrator. Move the
ProgressEvent type definition into the shared types.ts module alongside
RunConfig and SessionContext, then update evaluatorLoop.ts, baselineScanner.ts,
and runAll.ts to import it from that shared location so runAll.ts stays a thin
top-level coordinator.
🪄 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: dd7d43c8-1c1c-4661-a8b1-75f4091f3c2b
📒 Files selected for processing (4)
core/src/execute/baselineScanner.tscore/src/execute/evaluatorLoop.tscore/src/execute/runAll.tscore/tests/baselineScanner.test.ts
Comment on lines
+221
to
+227
| const currentSnapshot = tools.map((t) => ({ | ||
| name: t.name, | ||
| description: t.description ?? "", | ||
| inputSchema: t.inputSchema ?? null, | ||
| })); | ||
| const currentJson = JSON.stringify(currentSnapshot, null, 2); | ||
| const currentHash = createHash("sha256").update(currentJson).digest("hex"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use semantic diffs before failing drift.
Line 267 compares raw JSON hashes before parsing/diffing, so a different tools/list order can produce a FAIL with 0 actual diffs. Canonicalize snapshots or treat computeToolDiffs(...).length === 0 as PASS.
Also applies to: 266-291, 344-346
🤖 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/baselineScanner.ts` around lines 221 - 227, The drift check
in baselineScanner currently fails on raw JSON hash mismatches even when
computeToolDiffs returns no real changes, so reorder-only tools/list responses
can incorrectly fail. Update the drift path around currentSnapshot/currentHash
and the FAIL decision to use semantic comparison first: either canonicalize the
tool snapshot before hashing or, at minimum, treat computeToolDiffs(...).length
=== 0 as PASS before marking drift. Keep the logic in baselineScanner aligned
with the parsing/diffing flow so snapshot order does not trigger false failures.
Comment on lines
+286
to
+290
| const baselineSnapshot = JSON.parse(baselineJson) as Array<{ | ||
| name: string; | ||
| description: string; | ||
| inputSchema: unknown; | ||
| }>; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
nl -ba core/src/execute/baselineScanner.ts | sed -n '240,340p'
printf '\n--- search baselineJson usage ---\n'
rg -n "baselineJson|attack_start|JSON\.parse|Zod|z\." core/src/execute/baselineScanner.ts
printf '\n--- outline of file ---\n'
ast-grep outline core/src/execute/baselineScanner.ts --view expandedRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("core/src/execute/baselineScanner.ts")
lines = path.read_text().splitlines()
for start, end in [(240, 340)]:
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d} {lines[i-1]}")
PY
printf '\n--- search baselineJson usage ---\n'
rg -n "baselineJson|attack_start|JSON\.parse|Zod|z\." core/src/execute/baselineScanner.ts
printf '\n--- outline of file ---\n'
ast-grep outline core/src/execute/baselineScanner.ts --view expandedRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 5806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("core/src/execute/baselineScanner.ts")
lines = path.read_text().splitlines()
for start, end in [(90, 180), (180, 240), (240, 330), (330, 420)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d} {lines[i-1]}")
PY
printf '\n--- search for catch/finally in baselineScanner.ts ---\n'
rg -n "try|catch|finally|attack_done|notify\\(" core/src/execute/baselineScanner.ts
printf '\n--- search for baseline file validation elsewhere ---\n'
rg -n "baseline.*schema|Zod|zod|safeParse|JSON.parse\\(baselineJson\\)|computeToolDiffs|tool-description-drift" core/src -g '!**/dist/**'Repository: KeyValueSoftwareSystems/agent-opfor
Length of output: 15062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("core/src/execute/baselineScanner.ts")
lines = path.read_text().splitlines()
for start, end in [(1, 90), (60, 120), (120, 240)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d} {lines[i-1]}")
PY
printf '\n--- search for runBaselineScans callers / error handling ---\n'
rg -n "runBaselineScans|Promise\\.all|catch \\(|notify\\({ type: \"attack_done\"|attack_start" core/src -g '!**/dist/**'Repository: KeyValueSoftwareSystems/agent-opfor
Length of output: 11565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for rel in ["core/src/run/runAll.ts", "core/src/execute/baselineScanner.ts"]:
path = Path(rel)
if path.exists():
lines = path.read_text().splitlines()
print(f"\n--- {rel} ---")
for i, line in enumerate(lines, 1):
if "runBaselineScans" in line or "attack_start" in line or "attack_done" in line or "catch" in line or "finally" in line:
print(f"{i:4d} {line}")
PY
printf '\n--- search baseline scan callers ---\n'
rg -n "runBaselineScans\\(" core/srcRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 1180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("core/src/execute/runAll.ts")
lines = path.read_text().splitlines()
for start, end in [(1, 140), (140, 240)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d} {lines[i-1]}")
PYRepository: KeyValueSoftwareSystems/agent-opfor
Length of output: 9940
Validate the stored baseline before diffing it.
JSON.parse(baselineJson) can throw on a malformed baseline file, which aborts the scan before attack_done is emitted and leaves the progress state hanging after attack_start. Parse as unknown, validate with a Zod schema, and return an ERROR result with a clear message when the baseline is invalid.
🤖 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/baselineScanner.ts` around lines 286 - 290, The baseline
parsing in baselineScanner should not assume the stored JSON is valid. In the
logic that reads baselineJson and builds baselineSnapshot, first parse as
unknown, validate the result with the existing Zod schema, and if validation
fails return an ERROR result with a clear invalid-baseline message instead of
letting JSON.parse or shape mismatches abort the scan. Keep the fix localized
around the baselineSnapshot creation path so attack_done still gets emitted and
progress state is not left hanging after attack_start.
Source: Coding guidelines
Comment on lines
+295
to
+296
| await mkdir(baselinesDir, { recursive: true }); | ||
| await writeFile(baselinePath, currentJson, "utf8"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not auto-accept detected drift as the new baseline.
After detecting a rug pull, Line 296 overwrites the trusted baseline with the changed snapshot. A second run will then PASS without explicit approval. Keep the original baseline and write the new snapshot as a candidate, or only update it through an explicit accept flow.
Also applies to: 313-313
🤖 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/baselineScanner.ts` around lines 295 - 296, The baseline
update flow in baselineScanner should not overwrite the trusted baseline
immediately after detecting drift or a rug pull; keep the existing baseline
unchanged and store the new snapshot as a separate candidate instead. Update the
logic around writeFile in the baselineScanner flow so currentJson is written
only to a candidate/temporary artifact, and reserve any baselinePath overwrite
for an explicit accept path. Apply the same change to the other update site
referenced in the scanner logic so both paths use the same approved-baseline
behavior.
jithin23-kv
approved these changes
Jul 2, 2026
This was referenced Jul 8, 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
2 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
runAllwas a ~275-line function (depth-10 nesting, a labeledbreak) fusing fivejobs. This dissolves the god-function into a phase pipeline —
runAllis now athin orchestrator (resolve → preflight → execute → report), dropping
843 → 278 lines.
baselineScanner.ts(new) — the MCP pre-flight scans as a Chain ofResponsibility:
runBaselineScans+ resource / tool-description / rug-pullscanners; each contributes an
EvaluatorResultor nothing, pushed in order.evaluatorLoop.ts(new) —runEvaluatorAttacks, the evaluator attack loop.The labeled
break evaluatorLoopbecomes areturn;captureSessionContextmoves in.
Backward-compatible
runAll(config, options)keeps its exact signature — CLI, SDK, MCP, andbrowser callers are untouched. The
RunEngine/RunRequestboundary from the planis intentionally deferred: it would rewire the front-ends and change the public
API, conflicting with the backward-compat requirement. The phase pipeline delivers
the SRP/clean-code win without touching the API.
Behavior-preserving
evaluatorResultsordering (scans then evaluators), partial-report-on-stop,mcpTarget.close()in afinally, topo-sort order, dependency-skip, andupstream-session threading are all reproduced. Guarded by the
runAll.smoke+orchestrator.equivalencedrift tests (18/18 green).Clean-code / DRY
The two verbatim-duplicated stop-error branches (
isStopErrorvsTargetStopError) and the thrice-repeatedevaluatorMetaobject are consolidated;makeFailedResultnow useserrorJudge(). NewbaselineScannertest covers thechain (the MCP pre-flight had no coverage before).
Deferred (logged in the plan; pre-existing, out of a behavior-preserving refactor)
Three pre-existing baseline-scanner bugs surfaced by the review (kept out, need
their own PR + tests): the
"ERROR: "read-failure sentinel collision (asecurity false-negative), a zero-byte baseline treated as "first run", and an
unguarded
JSON.parseof the on-disk baseline. Also: consolidating therunAllBrowsertwin loop, and minor nits.Verification
full suite 121 tests, 0 fail (3 skips) · typecheck 0 · lint 0 · prettier clean ·
full build (incl. browser bundle).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests