Greptile Summary

This PR fixes a fallback stall issue in multi-model same-provider chains by capping transient cooldown probing (for rate_limit / overloaded reasons) to one attempt per provider per fallback run. Previously, every same-provider candidate could independently trigger a cooldown-bypass probe, causing the fallback loop to stall on providers with long internal retry timeouts before ever reaching cross-provider candidates.

Key changes:

• src/agents/model-fallback.ts: A cooldownProbeUsedProviders: Set<string> is initialized per-run and checked before allowing a transient cooldown probe. The first same-provider probe proceeds as before; subsequent same-provider candidates are skipped (logged as skip_candidate) and the loop moves on to the next candidate.
• src/agents/model-fallback.test.ts: A new regression test verifies the four-step chain — primary skipped → first same-provider fallback probed and fails → second same-provider fallback skipped → cross-provider fallback succeeds — and the two-call mock implicitly asserts haiku is never invoked.
• CHANGELOG.md: Unreleased fix entry added in the standard format.

Notable implementation detail: decision.markProbe is only ever true for primary candidates (markProbe: params.isPrimary && shouldProbe), so markProbeAttempt is never called for the non-primary candidates that the new guard skips — no probe-throttle slots are wasted on skipped fallbacks.

Minor observation: The billing case is included in the new guard's condition check (line 587–590), but resolveCooldownDecision already returns type: "skip" for all non-primary billing candidates before the guard is reached, so that branch is dead code in the current logic. It is harmless but may be slightly surprising to future readers.

Confidence Score: 5/5

• This PR is safe to merge — the change is well-scoped, correctly ordered relative to existing guards, and backed by a targeted regression test.
• The logic is minimal and contained to a single Set lookup inserted at the correct point in the fallback loop. The markProbeAttempt / probe-throttle interaction is unaffected because decision.markProbe is only true for primary candidates. The billing inclusion in the guard is dead code but harmless. The new test self-validates the skip by using a two-call mock that would fail if any additional same-provider candidate were unexpectedly executed.
• No files require special attention.

_{Last reviewed commit: 5d40a50}

🔒 Aisle Security Analysis

We found 2 potential security issue(s) in this PR:

1. 🔵 Cooldown probe limiter bypass via broad 'format' failure classification can stall cross-provider fallback (DoS)

Description

In runWithModelFallback, the new per-provider cooldown probe limiter is bypassed when the first transient cooldown probe fails with a failure reason classified as format.

This can lead to multiple same-provider probe attempts (one per same-provider candidate) during a single fallback run, delaying cross-provider fallback and increasing latency/compute.

Why this is attacker-influenceable:

• Probe-slot consumption is decided by describeFailoverError(err).reason.
• describeFailoverError derives reason via resolveFailoverReasonFromError, which classifies any HTTP 400 as format (unless billing).
• HTTP 400 responses can be triggered by untrusted/user-controlled content in many provider APIs (e.g., content/policy validation, malformed tool-call structures induced by prompt/tooling interactions), meaning a user can cause the probe to be labeled format.

Effect:

• When the provider is already in transient cooldown (rate_limit/overloaded) and allowTransientCooldownProbe: true is enabled, a user-triggered 400/format failure will preserve the probe slot, allowing repeated probes on additional same-provider models before trying other providers. If each attempt has long upstream timeouts/retries, this becomes a per-request DoS vector.

Vulnerable code:

const probeFailureReason = describeFailoverError(err).reason;
const shouldPreserveTransientProbeSlot =
  probeFailureReason === "model_not_found" ||
  probeFailureReason === "format" || // preserves slot
  probeFailureReason === "auth" ||
  probeFailureReason === "auth_permanent" ||
  probeFailureReason === "session_expired";
if (!shouldPreserveTransientProbeSlot) {
  cooldownProbeUsedProviders.add(transientProbeProviderForAttempt);
}

Supporting classification behavior:

• classifyFailoverReasonFromHttpStatus(400, ...) returns "format" for most 400s.

Recommendation

Tighten probe-slot preservation so that user-triggerable/broad classifications cannot bypass the per-provider probe limit.

Suggested changes:

1. Consume the probe slot for format by default, or only preserve it for very narrow, explicitly detected cases.
2. Alternatively, preserve based on explicit message patterns (e.g., only tool-call-id/schema errors), not on status === 400.
3. Add a hard cap on total transient cooldown probes per fallback run (e.g., maxTransientCooldownProbesPerRun) to prevent worst-case stalls.

Example (simplest safe behavior — consume slot on format):

const probeFailureReason = describeFailoverError(err).reason;
const shouldPreserveTransientProbeSlot =
  probeFailureReason === "model_not_found" ||
  probeFailureReason === "auth" ||
  probeFailureReason === "auth_permanent" ||
  probeFailureReason === "session_expired";

cooldownProbeUsedProviders.add(transientProbeProviderForAttempt);
​// Optionally: if shouldPreserveTransientProbeSlot, remove/add back based on your intended semantics.

Or, if you need to preserve for some format errors:

const described = describeFailoverError(err);
const preserveForFormat =
  described.reason === "format" &&
  ​/tool_use\.id|tool call id was/i.test(described.message);

if (!(preserveForFormat || described.reason === "model_not_found")) {
  cooldownProbeUsedProviders.add(provider);
}

2. 🔵 Potential DoS via unguarded describeFailoverError() call on unknown thrown values

Description

The new transient cooldown probe tracking logic calls describeFailoverError(err).reason on an unknown caught value (err).

While describeFailoverError() looks safe for typical Error instances, it can throw for some valid JavaScript thrown values because it falls back to String(err) when it cannot extract a message:

• describeFailoverError() computes const message = getErrorMessage(err) || String(err).
• String(err) can throw (e.g., objects with no usable primitive conversion such as Object.create(null) or objects with toString/valueOf set to non-callable values).
• This invocation is not wrapped in a try/catch, so a malformed/unexpected thrown value can crash the fallback loop during error handling (availability/DoS).

Vulnerable code (new call site):

const probeFailureReason = describeFailoverError(err).reason;

Relevant utility:

const message = getErrorMessage(err) || String(err);

Recommendation

Wrap the probe-reason extraction in a defensive try/catch (or use a safe formatter) so error handling can never throw while processing an already-failed attempt.

Example:

let probeFailureReason: FailoverReason | undefined;
try {
  probeFailureReason = describeFailoverError(err).reason;
} catch {
  probeFailureReason = undefined;
}

const shouldPreserveTransientProbeSlot =
  probeFailureReason === "model_not_found" ||
  probeFailureReason === "format" ||
  probeFailureReason === "auth" ||
  probeFailureReason === "auth_permanent" ||
  probeFailureReason === "session_expired";

Optionally harden describeFailoverError() itself by guarding the String(err) conversion:

let message = getErrorMessage(err);
if (!message) {
  try { message = String(err); } catch { message = Object.prototype.toString.call(err); }
}

Analyzed PR: #41711 at commit 8be8967

_{Last updated on: 2026-03-10T12:35:59Z}

Thanks for the review pass and the note about the billing branch in the probe guard.

I followed up in d9609d2 to make the intent explicit:

• keep allowTransientCooldownProbe for primary billing probe paths (needed by existing billing recovery tests), and
• apply the one-probe-per-provider cap only for transient cooldown reasons (rate_limit / overloaded).

Local verification after the change:

• pnpm test src/agents/model-fallback.test.ts src/agents/model-fallback.probe.test.ts
• pnpm check

Thanks for the security review and detailed write-up.

Triage outcome for this PR:

• I validated the described routing behavior locally and in tests (pnpm test src/agents/model-fallback.test.ts).
• This PR does not introduce new cross-provider fallback capability; it changes cooldown probe pacing within an already explicit fallback chain.

Security-model assessment (per SECURITY.md):

• OpenClaw’s gateway model is trusted-operator, not adversarial multi-tenant on one shared gateway/agentDir.
• Explicit fallback entries are treated as explicit operator intent (including cross-provider), and are intentionally not silently filtered by the model allowlist.
• Risks that depend on untrusted operators sharing one gateway/config are listed out of scope in this repo’s security policy.

Given that, I’m treating this as policy/hardening guidance rather than a security bug in this patch. No additional code change is required in this PR for the documented trust boundaries.

If maintainers want, we can still track optional hardening separately (for example explicit cross-provider fallback gating) as a product policy feature.

Merged via squash.

• Prepared head SHA: 8be8967bcb4be81f6abc5ff078644ec4efcfe7a0
• Merge commit: 048e25c2b21d56962c482ea63f7fe78795194609

Thanks @cgdusek!