Fix/mcp baseline scan llm error crash

[Arun-Sunny]

Problem

Running opfor run against an MCP target with a broken attackerLlm/judgeLlm model (e.g. an invalid model name behind an OpenAI-compatible gateway like LiteLLM) could intermittently crash the entire run with a raw, unhandled TypeError: fetch failed instead of the clean, actionable Error: LLM HTTP 400: ... message the gateway actually returned.

Root cause was two compounding issues:

1. chatCompletionJsonContent (core/src/llm/openaiCompatible.ts) retries up to 3 times on HTTP 400 (stripping json_object mode, then temperature) to work around providers that reject those params. It can't distinguish "this param isn't supported" from "this model doesn't exist" — an invalid model returns 400 on every attempt regardless of these params. Each retry fired immediately, back-to-back, without ever draining the previous (discarded) response body. An unconsumed fetch response body holds its connection open; issuing several rapid requests to the same host without draining prior bodies can exhaust the connection pool and throw an opaque TypeError: fetch failed that masks the real, informative LLM HTTP 400: ... error the function is designed to produce.
2. Independently, nothing between chatCompletionJsonContent and the CLI's top-level handler caught this failure. judgeToolResponse calls it with no try/catch, and both scanResources and scanToolDescriptions in core/src/execute/baselineScanner.ts called judgeToolResponse with no try/catch either — so any transient failure during the MCP pre-flight baseline scans (which run before the real evaluator suite even starts) took down the entire run/report instead of failing just that one scan.

Solution

• chatCompletionJsonContent now drains each discarded response's body (via a small drainBody helper) before firing the next retry attempt, preventing connection-pool exhaustion across the up-to-3 rapid requests.
• scanResources and scanToolDescriptions now wrap their judgeToolResponse calls in try/catch, converting a caught error into an ERROR-verdict result via the existing mcpErrorJudge helper — consistent with the error-handling pattern scanResources already used for target.readResource() failures. A judge/LLM failure on one resource or tool no longer aborts the whole run; it's now recorded as an ERROR and the scan continues to the next item.

Changes

• core/src/llm/openaiCompatible.ts — add drainBody(), call it before each 400-retry in chatCompletionJsonContent.
• core/src/execute/baselineScanner.ts — wrap the judgeToolResponse calls in scanResources and scanToolDescriptions in try/catch, falling back to mcpErrorJudge(message).

Issue

Closes #189

How to test

1. Configure an MCP target (suite selection, e.g. owasp-mcp-top10) with attackerLlm pointed at an openai-compatible gateway and an intentionally invalid model name.
2. Run opfor run --config <path>.
3. Expected: the Tool Description Poisoning Scan (and Resource Exposure Scan, if the target exposes resources) report each item as ERROR with the underlying LLM HTTP 400: ... message surfaced in the judge's errorMessage/reasoning, and the run continues through the rest of the evaluator suite (or completes/reports normally) instead of crashing with an unhandled exception.
4. npm run build && npm run typecheck && npm test --workspace=core all pass (verified locally — 158/158 core tests green).

Screenshots

N/A — CLI/log-output bug fix, no UI changes.

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of scan-time failures so affected checks now return a clear error verdict instead of stopping the scan.
  • Fixed retry handling for failed AI requests by fully consuming discarded responses, reducing issues during rapid retries.

[coderabbitai]

Walkthrough

Baseline scanner functions scanResources and scanToolDescriptions now catch judgeToolResponse exceptions and convert them into mcpErrorJudge results instead of crashing. Separately, the OpenAI-compatible client adds a drainBody helper that consumes discarded response bodies before retrying HTTP 400 requests.

Changes

Robust error handling for baseline scans and LLM retries

Layer / File(s)	Summary
Baseline scan error handling core/src/execute/baselineScanner.ts	scanResources and scanToolDescriptions wrap judgeToolResponse calls in try/catch, converting failures into mcpErrorJudge results so AttackResult and notifications continue to emit instead of crashing the run.
Response body draining on 400 retries core/src/llm/openaiCompatible.ts	A new drainBody helper consumes discarded response bodies via res.text(), and chatCompletionJsonContent calls it before retrying after disabling json_object mode or temperature on HTTP 400 responses.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• KeyValueSoftwareSystems/agent-opfor#154: Touches the same baseline pre-flight scanning logic in scanResources/scanToolDescriptions that this PR wraps with error handling.

Suggested reviewers: jithin23-kv

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: preventing MCP baseline scan LLM crashes.
Description check	✅ Passed	All required sections are present and filled with relevant problem, solution, changes, issue, test, and screenshot info.
Linked Issues check	✅ Passed	The changes match #189 by draining retry bodies and catching judgeToolResponse failures so baseline scans continue on errors.
Out of Scope Changes check	✅ Passed	The modified files are directly tied to the reported crash fix, with no unrelated changes evident in the summary.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

core/src/execute/baselineScanner.ts (1)

131-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a helper to eliminate duplicated catch logic.

Both scanResources and scanToolDescriptions use an identical try/catch pattern around judgeToolResponse with the same catch-block logic. Extracting a wrapper would remove the duplication and make the error-handling intent explicit:

♻️ Proposed helper extraction

+async function safeJudgeToolResponse(
+  args: Parameters<typeof judgeToolResponse>[0]
+): Promise<JudgeResult> {
+  try {
+    return await judgeToolResponse(args);
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err);
+    return mcpErrorJudge(message);
+  }
+}

Then both call sites simplify to:

-    let judgeResult;
-    try {
-      judgeResult = await judgeToolResponse({
-        model: judgeModelConfig,
-        evaluator: { ... },
-        ...
-      });
-    } catch (err) {
-      const message = err instanceof Error ? err.message : String(err);
-      judgeResult = mcpErrorJudge(message);
-    }
+    const judgeResult = await safeJudgeToolResponse({
+      model: judgeModelConfig,
+      evaluator: { ... },
+      ...
+    });

This also eliminates the untyped let judgeResult; (evolving any) in favor of a properly typed const.

Also applies to: 185-207

🤖 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 131 - 152, The try/catch
around judgeToolResponse in baselineScanner is duplicated across scanResources
and scanToolDescriptions, and the untyped let judgeResult can be avoided.
Extract the shared judge execution/error-handling into a small helper that
accepts the evaluator/tool metadata and returns the judge result, using the same
mcpErrorJudge fallback in one place. Then update both scanResources and
scanToolDescriptions to call that helper and use a typed const result instead of
evolving any.

🤖 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.

Nitpick comments:
In `@core/src/execute/baselineScanner.ts`:
- Around line 131-152: The try/catch around judgeToolResponse in baselineScanner
is duplicated across scanResources and scanToolDescriptions, and the untyped let
judgeResult can be avoided. Extract the shared judge execution/error-handling
into a small helper that accepts the evaluator/tool metadata and returns the
judge result, using the same mcpErrorJudge fallback in one place. Then update
both scanResources and scanToolDescriptions to call that helper and use a typed
const result instead of evolving any.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1bc75804-b9e5-40f3-9131-d3e0936f955a

📥 Commits

Reviewing files that changed from the base of the PR and between 2ff5624 and 39026fa.

⛔ Files ignored due to path filters (1)

• assets/opfor-high-level.svg is excluded by !**/*.svg

📒 Files selected for processing (2)

• core/src/execute/baselineScanner.ts
• core/src/llm/openaiCompatible.ts