Skip to content

fix(agents): skip empty allowlist guard for modelRun shapes#95605

Open
bobrenze-bot wants to merge 2 commits into
openclaw:mainfrom
bobrenze-bot:fix/modelrun-empty-allowlist-guard
Open

fix(agents): skip empty allowlist guard for modelRun shapes#95605
bobrenze-bot wants to merge 2 commits into
openclaw:mainfrom
bobrenze-bot:fix/modelrun-empty-allowlist-guard

Conversation

@bobrenze-bot

Copy link
Copy Markdown

What Problem This Solves

Embedded raw-model runs abort with "No callable tools remain after resolving explicit tool allowlist" before the model is ever reached. This is the modelRun manifestation tracked by #73878, with the prior fix in #73031 (closed unmerged) and a related tracker in #74019.

It still reproduces on 2026.6.9 (latest stable). Concretely, with the active-memory plugin enabled, every per-message summary probe fails:

active-memory: ... activeModel=glm-5.2:cloud start timeoutMs=60000
embedded run failover decision
lane task error: ... Error: No callable tools remain after resolving explicit tool allowlist (runtime toolsAllow: memory_search, memory_get); no registered tools matched
active-memory: ... done status=unavailable summaryChars=0

The probe runs as a raw model run (modelRun / promptMode: "none"), which intentionally zeroes plugin tools — but buildEmptyExplicitToolAllowlistError still evaluates the explicit allowlist (memory_search, memory_get) against the empty registered-tool set and fails closed. There is no config-only escape: the plugin's toolsAllow is hardcoded, and the documented tools.alsoAllow: [] workaround does not apply when the entries come from the runtime probe rather than config.

Why This Change Was Made

Fail-closed semantics are correct for real agent turns, but a raw model run has no tools by design, so the empty-allowlist guard should not fire for it. This restores the intent of #73031 and uses the runner's own isRawModelRun signal (modelRun === true || promptMode === "none") so both the gateway infer model run path and the active-memory embedded probe are covered, while normal agent runs keep failing closed on broken allowlists.

User Impact

  • active-memory (and other embedded model-run probes) work again instead of returning status=unavailable on every direct message.
  • No behavior change for normal agent turns — broken explicit allowlists still fail closed.
  • No new config or migration.

Evidence

  • buildEmptyExplicitToolAllowlistError now short-circuits to null when modelRun === true; call site in embedded-agent-runner/run/attempt.ts passes the existing isRawModelRun.
  • Added unit test ("skips the empty-allowlist guard for raw model-run shapes").
  • pnpm test src/agents/tool-allowlist-guard.test.ts8 passed.
  • pnpm tsgo:core → clean.

Refs #73878, #73031, #74019.

Raw model runs (infer model run, active-memory summary probes, and other
modelRun/promptMode:none shapes) intentionally run without plugin tools, so an
inherited explicit allowlist that resolves to no callable tools must not abort
the run before the model is reached. Fail-closed behavior is preserved for real
agent turns.

Revives the fix from openclaw#73031; addresses openclaw#73878 / openclaw#74019.
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jun 21, 2026
@clawsweeper

clawsweeper Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 9:39 PM ET / 01:39 UTC.

Summary
The PR adds a modelRun parameter to the empty explicit tool allowlist guard, passes raw-model-run state from the embedded runner, and adds a focused guard test.

PR surface: Source +9, Tests +13. Total +22 across 3 files.

Reproducibility: yes. for the review concern: current source shows active-memory passes toolsAllow without modelRun or promptMode, and the existing guard test encodes runtime toolsAllow fail-closed behavior. I did not run the live active-memory scenario in this read-only review.

Review metrics: 1 noteworthy metric.

  • Fail-closed guard paths: 1 guard path bypassed for modelRun. The patch changes runtime behavior before the existing disabled-tool source filtering, so green unit tests alone do not settle compatibility.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted after-fix terminal output, copied logs, or a recording from active-memory or infer model run on this patch.
  • [P1] Add or adjust coverage for the real active-memory runEmbeddedAgent parameters if active-memory is the intended fixed path.
  • Make the runtime toolsAllow contract explicit if raw model runs should override it.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing after-fix real behavior proof; the contributor should add redacted terminal output, copied logs, or a recording from active-memory or infer model run, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] The PR affects the real infer model run raw-run path, but current active-memory source does not show the raw-run flags claimed in the PR body.
  • [P1] The early modelRun bypass changes explicit runtime toolsAllow from fail-closed to silent text-only execution unless maintainers intentionally accept that contract change.
  • [P1] No after-fix real behavior proof currently shows active-memory or infer model run succeeding with this patch.

Maintainer options:

  1. Keep runtime allowlists fail-closed (recommended)
    Remove or narrow the modelRun early return so inherited config allowlists can be ignored without weakening explicit runtime toolsAllow enforcement.
  2. Accept raw-run override intentionally
    A maintainer may decide raw model runs override runtime toolsAllow, but the PR should update the existing fail-closed test and provide after-fix real behavior proof.
  3. Pause if active-memory is the only target
    If the branch is meant only to fix active-memory, pause it until the patch targets the current active-memory call shape or the reporter proves that path now sets modelRun.

Next step before merge

  • [P1] The remaining blocker is a maintainer contract decision on raw model runs versus runtime toolsAllow, plus contributor-side real behavior proof that automation cannot supply for their setup.

Security
Cleared: The diff only changes in-process TypeScript guard logic and a unit test; no dependency, workflow, credential, packaging, or supply-chain surface is added.

Review findings

  • [P1] Preserve runtime toolsAllow failures for model runs — src/agents/tool-allowlist-guard.ts:46-48
  • [P2] Target the active-memory call shape that actually runs — src/agents/embedded-agent-runner/run/attempt.ts:1827
Review details

Best possible solution:

Preserve runtime toolsAllow enforcement by default, or make a maintainer-explicit contract change with focused tests and redacted after-fix active-memory or infer model run proof.

Do we have a high-confidence way to reproduce the issue?

Yes for the review concern: current source shows active-memory passes toolsAllow without modelRun or promptMode, and the existing guard test encodes runtime toolsAllow fail-closed behavior. I did not run the live active-memory scenario in this read-only review.

Is this the best way to solve the issue?

No. The patch may help a raw infer model run surface, but it is broader than the merged source-distinction fix and does not match the cited active-memory call shape on current main.

Full review comments:

  • [P1] Preserve runtime toolsAllow failures for model runs — src/agents/tool-allowlist-guard.ts:46-48
    The new early return runs before the existing disabled-tool source filtering, so a raw model run with caller-supplied runtime toolsAllow and no callable tools now skips the fail-closed guard. Current main deliberately marks that source enforceWhenToolsDisabled and has a test expecting it to error, so this silently converts a bad explicit runtime allowlist into a text-only run.
    Confidence: 0.9
  • [P2] Target the active-memory call shape that actually runs — src/agents/embedded-agent-runner/run/attempt.ts:1827
    The cited active-memory path on current main calls runEmbeddedAgent with toolsAllow but without modelRun or promptMode: "none", so passing modelRun: isRawModelRun here will not affect that failure. If the shipped active-memory bug remains, the fix needs to cover why those memory tools are absent or filtered in the real embedded run, with a regression for that call shape.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a9eb0d7e9cae.

Label changes

Label justifications:

  • P2: This is a normal-priority agent runtime bug-fix PR with limited blast radius but real compatibility risk.
  • merge-risk: 🚨 compatibility: The patch can change explicit runtime toolsAllow from fail-closed to text-only execution for raw model-run shapes.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing after-fix real behavior proof; the contributor should add redacted terminal output, copied logs, or a recording from active-memory or infer model run, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +9, Tests +13. Total +22 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 9 0 +9
Tests 1 13 0 +13
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 22 0 +22

What I checked:

  • Repository policy applied: Root and scoped agent runner policies were read; they require deep source review, compatibility caution for fail-closed runtime behavior, and focused helper tests where appropriate. (AGENTS.md:1, a9eb0d7e9cae)
  • PR diff: The PR changes only the embedded attempt guard call, the guard signature/early return, and one guard unit test. (src/agents/tool-allowlist-guard.ts:46, 320fc5965340)
  • Current runtime allowlist contract: Current tests explicitly expect runtime toolsAllow with disabled tools and no callable tools to produce the fail-closed error. (src/agents/tool-allowlist-guard.test.ts:22, a9eb0d7e9cae)
  • Runtime source is enforceable: The embedded attempt source collector marks runtime toolsAllow with enforceWhenToolsDisabled: true, so it remains the caller-supplied allowlist source preserved by the merged source-distinction fix. (src/agents/embedded-agent-runner/run/attempt.ts:798, a9eb0d7e9cae)
  • Active-memory current call shape: Current active-memory passes toolsAllow into runEmbeddedAgent but does not set modelRun or promptMode: "none", so the proposed modelRun branch does not cover the cited active-memory path as written. (extensions/active-memory/index.ts:2980, a9eb0d7e9cae)
  • Raw CLI model run path: The infer model run gateway path does send modelRun: true and promptMode: "none", so the PR affects that raw-run surface even though active-memory differs. (src/cli/capability-cli.ts:848, a9eb0d7e9cae)

Likely related people:

  • steipete: Related review history identifies commit 61ee67a as the empty explicit allowlist guard introduction, and the earlier modelRun-only bypass was closed as superseded after the generic fix landed. (role: introduced guard behavior and prior disposition participant; confidence: high; commits: 61ee67aecc3f; files: src/agents/tool-allowlist-guard.ts, src/agents/tool-allowlist-guard.test.ts)
  • amknight: Authored merged fix(tools): allow no-tool llm-task runs #76686, which established the inherited-config versus runtime toolsAllow distinction this PR changes. (role: source-distinction fix author; confidence: high; commits: 9c3e5f773e66, 349106065a47; files: src/agents/tool-allowlist-guard.ts, src/agents/tool-allowlist-guard.test.ts, src/agents/pi-embedded-runner/run/attempt.ts)
  • hclsys: Authored the merged active-memory missing-memory-tools handling that overlaps this PR's active-memory symptom and current catch path. (role: active-memory related fix author; confidence: high; commits: 0cf6694317af, d0cae0d9500e; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)
  • Takhoffman: Authored follow-up active-memory commits in merged fix(active-memory): skip sub-agent gracefully when no memory tools registered (#77506) #77515, and related history ties this handle to the infer CLI raw model-run entry point. (role: active-memory repair contributor and infer CLI surface owner; confidence: medium; commits: f383456c9a2f, 5294ca347fb0, 97c031a8db48; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts, src/cli/capability-cli.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 21, 2026
@bobrenze-bot

Copy link
Copy Markdown
Author

Responding to the superseded assessment with live reproduction evidence.

The claim that #76686 and #77515 cover this path does not hold for the configuration reproduced here. Both PRs are in 2026.6.9 (confirmed shipped). The bug still reproduces on 2026.6.9 with memory tools registered:

2026-06-21T15:40:46  active-memory: agent=main activeModel=glm-5.2:cloud start timeoutMs=60000 queryChars=758
2026-06-21T15:40:46  [tools] No callable tools remain after resolving explicit tool allowlist
                      (runtime toolsAllow: memory_search, memory_get); no registered tools matched.
2026-06-21T15:40:46  lane task error: Error: No callable tools remain after resolving explicit tool
                      allowlist (runtime toolsAllow: memory_search, memory_get); no registered tools matched.
2026-06-21T15:40:46  active-memory: done status=unavailable elapsedMs=412 summaryChars=0

Why #77515 doesn't cover this: #77515 catches the error and returns NONE gracefully — it does not prevent the guard from firing. It handles the "memory tools absent" case; this is the "memory tools registered but suppressed by modelRun" case. memory_search and memory_get are live (confirmed ~140ms in normal turns via memory-core). They fail only inside the active-memory embedded probe because the probe runs as modelRun:true, zeroing plugin tools before the guard runs.

Why #76686 doesn't cover this: #76686 distinguishes enforceWhenToolsDisabled sources from inherited config sources. The active-memory probe sets its allowlist at runtime (toolsAllow: [memory_search, memory_get] hardcoded in the plugin), which the runner marks as enforceWhenToolsDisabled: true — so it is not filtered out by #76686's source-level fix. The probe still hits the guard with zero callable tools and fails.

The fix in this PR (short-circuit when modelRun === true) is a distinct and narrower condition than either merged fix. Reopening for reconsideration.

@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 21, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@bobrenze-bot thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants