🔒 Aisle Security Analysis

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

const timeoutCircuitBreaker = new Map<string, CircuitBreakerEntry>();
function buildCircuitBreakerKey(agentId: string, provider?: string, model?: string): string {
  return `${agentId}:${provider ?? "unknown"}/${model ?? "unknown"}`;
}
...
if (isCircuitBreakerOpen(cbKey, ...)) {
  const result: ActiveRecallResult = { status: "timeout", elapsedMs: 0, summary: null };
  return result;
}
...
recordCircuitBreakerTimeout(cbKey);

function buildCircuitBreakerKey(params: {
  agentId: string;
  sessionKey?: string;
  messageProvider?: string;
  channelId?: string;
  provider?: string;
  model?: string;
}) {
  const sessionPart = params.sessionKey ?? `${params.messageProvider ?? "unknown"}:${params.channelId ?? "unknown"}`;
  return `${params.agentId}:${sessionPart}:${params.provider ?? "unknown"}/${params.model ?? "unknown"}`;
}

Analyzed PR: #74158 at commit e061b01

_{Last updated on: 2026-04-29T06:42:32Z}

Greptile Summary

This PR adds a per-agent/model circuit breaker to the Active Memory plugin that skips recall after circuitBreakerMaxTimeouts consecutive timeouts, preventing repeated token waste against slow-responding models. The implementation is well-scoped, backward-compatible, and covered by four focused new tests.

Confidence Score: 4/5

Safe to merge; only minor style/design P2 observations, no functional defects.

All findings are P2. The logic is correct, tests cover the key paths, and the feature is additive with sensible defaults.

No files require special attention.

This is a comment left during a code review.
Path: extensions/active-memory/index.ts
Line: 297-306

Comment:
**Side-effecting predicate mutates state on cooldown expiry**

`isCircuitBreakerOpen` deletes the map entry as a side-effect of checking. In the presence of concurrent async calls, both callers can pass the check: Call A deletes the entry and returns `false`; while Call A's `await` is pending, Call B finds the entry gone and also returns `false`, letting two "trial" requests through instead of one. For a best-effort plugin circuit breaker this is tolerable, but extracting the deletion into the call-site (or returning an enum like `open | half-open | closed`) would make the half-open transition explicit and easier to reason about.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/active-memory/index.ts
Line: 2404-2419

Comment:
**`unavailable` errors leave circuit breaker counter unchanged**

Non-timeout errors (network failures, parse errors, etc.) reach the `status: "unavailable"` branch without calling either `recordCircuitBreakerTimeout` or `resetCircuitBreaker`. The counter is silently frozen at its previous value. This is defensible — a crash isn't the same as a slow model — but a run of intermixed timeouts and errors will never reset the counter either. A comment explaining the intentional non-interaction would prevent future confusion.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "feat(active-memory): add timeout circuit..." | Re-trigger Greptile}

Codex review: needs maintainer review before merge.

Keep this PR open. Current main still lacks the proposed Active Memory consecutive-timeout circuit breaker and config knobs, while the linked #74054 report remains plausible because timeout results are intentionally not cached and repeated eligible prompts can launch new recall attempts. The PR is a relevant implementation candidate, but it needs maintainer review for breaker scope/security plus docs/changelog coverage before merge.

Maintainer follow-up before merge:

Keep this PR open for maintainer review. The best path is to finish an Active Memory-owned circuit breaker in the plugin, decide whether breaker state must include session/channel scope, document and changelog the new config knobs, then validate with targeted Active Memory tests and the changed gate before landing.

Best possible solution:

Keep this PR open for maintainer review. The best path is to finish an Active Memory-owned circuit breaker in the plugin, decide whether breaker state must include session/channel scope, document and changelog the new config knobs, then validate with targeted Active Memory tests and the changed gate before landing.

Acceptance criteria:

• pnpm test extensions/active-memory
• pnpm check:changed in Testbox before handoff/merge

What I checked:

• Current config has no breaker knobs: ActiveRecallPluginConfig and ResolvedActiveRecallPluginConfig include timeoutMs and cacheTtlMs but no circuitBreakerMaxTimeouts or circuitBreakerCooldownMs on current main. (extensions/active-memory/index.ts:91, dc810437e767)
• Runtime normalization has no breaker defaults: normalizePluginConfig clamps timeoutMs and cacheTtlMs only; there is no consecutive-timeout state or cooldown configuration on current main. (extensions/active-memory/index.ts:699, dc810437e767)
• Recall path is per-attempt bounded, not circuit-broken: maybeResolveActiveRecall checks the cache, starts a watchdog/Promise.race recall, returns timeout or timeout_partial on timeout, and caches only cacheable success/empty results; there is no cross-prompt consecutive-timeout state before starting recall. (extensions/active-memory/index.ts:2133, dc810437e767)
• Timeouts are deliberately not cached: The existing regression test calls the same timeout-triggering prompt twice, expects two session-store updates, and asserts no cached log line, supporting the linked repeated-timeout behavior. (extensions/active-memory/index.test.ts:2031, dc810437e767)
• Manifest rejects unknown fields and lacks proposed knobs: The Active Memory manifest has additionalProperties: false and lists timeoutMs/cacheTtlMs but not the proposed circuit-breaker fields on current main. (extensions/active-memory/openclaw.plugin.json:8, dc810437e767)
• Docs lack circuit-breaker configuration: The public Active Memory docs list timeoutMs and cacheTtlMs and suggest lowering timeoutMs for slow recall; they do not document circuitBreakerMaxTimeouts or circuitBreakerCooldownMs. Public docs: docs/concepts/active-memory.md. (docs/concepts/active-memory.md:561, dc810437e767)

Likely related people:

• vincentkoc: Remote commit metadata shows df9d26e changed the Active Memory prompt hook timeout and tests shortly before this PR, and b85edb3 backfilled the changelog entry for that Active Memory timeout behavior. (role: recent timeout-surface maintainer; confidence: high; commits: df9d26eb4335, b85edb3f0cf6; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts, CHANGELOG.md)
• Takhoffman: Remote commit metadata identifies b83726d as the original Active Memory plugin feature PR, adding the plugin runtime, tests, manifest, docs, and changelog surface that this PR extends. (role: original feature introducer; confidence: medium; commits: b83726d13e33; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts, extensions/active-memory/openclaw.plugin.json)
• luyao618: Remote commit metadata shows fbefbf0 added the Active Memory Promise.race hard-deadline behavior and tests, which is adjacent to the repeated-timeout circuit-breaker request. (role: adjacent hard-timeout contributor; confidence: medium; commits: fbefbf05bd74; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)

Remaining risk / open question:

• The PR's agent/model-scoped module-level breaker may let one session suppress recall for other sessions using the same agent/model during the cooldown window.
• The new user-facing config knobs need docs and changelog coverage because the manifest is strict and operators need to understand the degradation behavior.
• Returning synthetic status=timeout for a skipped recall may blur actual provider timeout telemetry unless maintainers decide that status is intentional.

Codex review notes: model gpt-5.5, reasoning high; reviewed against dc810437e767.