Skip to content

fix(agents): stabilize message send loop detection#89143

Closed
849261680 wants to merge 3 commits into
openclaw:mainfrom
849261680:fix/message-loop-detection-hash
Closed

fix(agents): stabilize message send loop detection#89143
849261680 wants to merge 3 commits into
openclaw:mainfrom
849261680:fix/message-loop-detection-hash

Conversation

@849261680

@849261680 849261680 commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

AI-assisted: yes (Codex prepared the patch and verification).

Summary

  • Normalize volatile delivery fields from message(action=send) tool outcomes before loop-detection result hashing, including qa-channel message-object id/timestamp fields while preserving stable routing facts such as conversation.id.
  • Keep loop-detector vetoes out of tool outcome history so a critical tool-loop block stays sticky instead of being treated as fresh progress.
  • Add focused regression coverage for direct, Gateway-shaped, plugin-shaped, qa-channel message-object, and createMessageTool() action-runner send results.

Fixes #89090.

Linked context

Issue #89090 reports that repeated message.send calls can warn forever without escalating to a critical block because each successful send returns a unique delivery id, producing a different resultHash every time.

Real behavior proof

Behavior addressed: Repeated message(action=send) calls with identical arguments now escalate to a critical loop block even when every successful send returns a different delivery id, and repeated calls after the first block stay blocked instead of sending more messages.

Real environment tested: Local full QA runtime with qa-lab bus, qa-channel plugin, real Gateway child process, and a local OpenAI-compatible provider that intentionally keeps requesting the same message.send tool call. Also tested focused local Vitest coverage for the message tool action-runner and before-tool-call loop-detection runtime paths.

Exact steps or command run after this patch:

pnpm tsx .artifacts/proof-89143-message-loop-runtime.ts
node scripts/run-vitest.mjs src/agents/tool-loop-detection.test.ts src/agents/tools/message-tool.test.ts src/agents/agent-tools.before-tool-call.e2e.test.ts
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

Evidence after fix:

proof=message-loop-runtime
provider_requests=27
gateway_observed_block=true
qa_channel_outbound_marker_count=20
run_id=e04535c3-f55c-494d-9620-f94f6fd46292
run_status=critical_block_observed
tool_loop_log_tail:
[agents/tools] Blocking message due to critical loop: CRITICAL: Called message with identical arguments and identical outcomes 20 times. Session execution blocked to prevent runaway loops.
[diagnostic] tool loop: sessionId=bb60d9da-bbdf-40db-b162-1b78f8bacb84 sessionKey=agent:qa:message-loop-proof-89143 tool=message level=critical action=block detector=generic_repeat count=20 message="CRITICAL: Called message with identical arguments and identical outcomes 20 times. Session execution blocked to prevent runaway loops."

src/agents/tool-loop-detection.test.ts: 45 passed
src/agents/tools/message-tool.test.ts: 90 passed
src/agents/agent-tools.before-tool-call.e2e.test.ts: 50 passed
tsgo core test tsconfig: passed
git diff --check: clean
autoreview: clean, no accepted/actionable findings reported

Observed result after fix: In the full Gateway/qa-channel proof, the first 20 repeated sends were delivered to the qa-channel bus, then Gateway emitted a critical tool=message level=critical action=block detector=generic_repeat count=20 event. The same looping provider continued requesting message.send, and qa-channel delivery stayed at exactly 20 outbound marker messages, proving later retries were blocked rather than sent.

What was not tested: No live Feishu/Lark/Telegram external transport send was run. The real-process proof used qa-channel because it exercises the Gateway child, channel plugin action dispatch, qa-lab bus, and message tool loop detector without live credentials.

Tests and validation

  • pnpm tsx .artifacts/proof-89143-message-loop-runtime.ts
  • node scripts/run-vitest.mjs src/agents/tool-loop-detection.test.ts src/agents/tools/message-tool.test.ts src/agents/agent-tools.before-tool-call.e2e.test.ts
  • node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local

Risk checklist

  • No config, env, migration, or public API surface changes.
  • No changelog update; normal PR release notes live here.
  • No live credentials, transcripts, screenshots, or proof artifacts committed.
  • Maintainer edits left enabled by using the GitHub CLI default.

Current review state

Fresh local autoreview passed with no accepted/actionable findings. The earlier proof gap was closed with full local Gateway/qa-channel runtime proof; the remaining disclosed gap is live external Feishu/Lark/Telegram transport replay.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: M proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 1:45 PM ET / 17:45 UTC.

Summary
The PR normalizes volatile fields from message(action=send) tool results before loop-detection hashing, avoids recording tool-loop vetoes as progress, and adds regression coverage for direct, gateway/plugin-shaped, qa-channel, and action-runner send paths.

PR surface: Source +78, Tests +224. Total +302 across 4 files.

Reproducibility: yes. Current main hashes full message send details/text while critical loop paths require stable result hashes, and the linked issue provides production warning-only duplicate-send logs; I did not run commands in this read-only review.

Review metrics: 1 noteworthy metric.

  • Side-effecting hash path: 1 production tool outcome path changed. The patch changes when repeated message sends are considered no-progress, which directly affects duplicate-message suppression before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • [P2] Choose this PR or the broader sibling as the canonical fix before merge.

Risk before merge

  • [P1] Merging intentionally changes user-visible delivery behavior: once the same message(action=send) call returns stable delivery facts at the critical threshold, later identical sends are blocked instead of delivered.
  • [P1] There is a competing open fix for the same linked issue at fix(agents): block message-tool spam loops defeated by volatile message ids #89109, so maintainers should choose one canonical implementation before landing to avoid overlapping loop-detector changes.

Maintainer options:

  1. Land the proof-backed narrow fix (recommended)
    If maintainers accept the existing critical-threshold duplicate-send policy for message(action=send), this PR has the focused code path and full Gateway/qa-channel proof needed for ordinary merge review.
  2. Consolidate with the broader sibling
    If maintainers want reply, broadcast, or session-send loops handled in the same landing, fold the desired coverage from fix(agents): block message-tool spam loops defeated by volatile message ids #89109 into one canonical PR before merge.
  3. Pause for delivery policy
    If legitimate repeated sends should remain deliverable past the critical threshold, pause this PR until the loop-detection policy is clarified.

Next step before merge

  • [P2] The code repair appears reviewable; the remaining action is maintainer choice of the canonical fix and acceptance of the message-delivery suppression behavior, not an automated code repair.

Security
Cleared: The diff only changes agent loop-detection code and focused tests; it does not add dependencies, workflows, permissions, secrets handling, or package execution surface.

Review details

Best possible solution:

Land one canonical, proof-backed fix for the message-send result-hash path, preserving stable routing facts while stripping volatile delivery ids and keeping blocked loop outcomes from resetting the detector.

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

Yes. Current main hashes full message send details/text while critical loop paths require stable result hashes, and the linked issue provides production warning-only duplicate-send logs; I did not run commands in this read-only review.

Is this the best way to solve the issue?

Yes. The targeted shared result-hash normalization is the narrowest maintainable fix for message(action=send), and the added sticky-block change closes the full-runtime proof gap without moving ownership to channel plugins.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR now supplies after-fix full local Gateway/qa-channel runtime output showing repeated sends stop at 20 and later retries are blocked by the critical loop detector.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR now supplies after-fix full local Gateway/qa-channel runtime output showing repeated sends stop at 20 and later retries are blocked by the critical loop detector.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P1: The linked bug can flood real chat channels with duplicate messages until retry exhaustion, so this is an urgent agent/channel workflow fix.
  • merge-risk: 🚨 message-delivery: The PR changes when repeated message sends are blocked, which can intentionally suppress user-visible channel delivery after the critical threshold.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR now supplies after-fix full local Gateway/qa-channel runtime output showing repeated sends stop at 20 and later retries are blocked by the critical loop detector.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR now supplies after-fix full local Gateway/qa-channel runtime output showing repeated sends stop at 20 and later retries are blocked by the critical loop detector.
Evidence reviewed

PR surface:

Source +78, Tests +224. Total +302 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 85 7 +78
Tests 2 224 0 +224
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 309 7 +302

What I checked:

  • Repository policy read: Root AGENTS.md and scoped src/agents guidance were read; the review applied the repo-specific requirements for full path review, real behavior proof, and message-delivery merge risk. (AGENTS.md:7, ba88b7a1780f)
  • Current main hashes full generic tool outcomes: On current main, hashToolOutcome falls back to hashing full details and text, so message send payloads containing fresh delivery ids produce different result hashes. (src/agents/tool-loop-detection.ts:229, ba88b7a1780f)
  • Critical block paths require stable no-progress hashes: The global circuit breaker and generic critical detector depend on noProgressStreak, which only grows when the same args and result hash repeat. (src/agents/tool-loop-detection.ts:488, ba88b7a1780f)
  • Message sends expose volatile delivery ids: MessageSendResult includes a nested result.messageId, and the message tool returns jsonResult(result.payload) when no plugin tool result overrides it. (src/infra/outbound/message.ts:88, ba88b7a1780f)
  • PR adds targeted message-send hashing: The head commit adds hashMessageSendToolOutcome, strips volatile delivery keys such as messageId, receipt, and message-object id/timestamp, and invokes it only for toolName === "message" with params.action === "send". (src/agents/tool-loop-detection.ts:217, 97944d9b67c5)
  • PR keeps critical loop blocks sticky: The head commit skips recordLoopOutcome for deniedReason === "tool-loop", preventing the blocked result from replacing the repeated successful outcome that triggered the block. (src/agents/agent-tools.before-tool-call.ts:1143, 97944d9b67c5)

Likely related people:

  • Sk Akram: Authored the original stuck loop detection and exponential backoff infrastructure that introduced the central loop-detection file. (role: introduced behavior; confidence: high; commits: e5eb5b3e43d7; files: src/agents/tool-loop-detection.ts, src/agents/tool-loop-detection.test.ts)
  • steipete: Recent history shows tool-loop configuration and current loop-detection file rewrites/refactors in this area, making this a likely routing candidate for threshold and policy review. (role: recent area contributor; confidence: high; commits: 076df941a326, 83a8b78a4248, e8c126eaf2c3; files: src/agents/tool-loop-detection.ts, src/agents/agent-tools.before-tool-call.ts, src/agents/tools/message-tool.ts)
  • gumadeiras: Recent message discovery, dispatch, and action-runner commits touch the outbound/message surfaces that produce the results being normalized. (role: adjacent owner; confidence: medium; commits: 3ca8ad38459c, 951f3f992b68, 03f18ec043de; files: src/agents/tools/message-tool.ts, src/infra/outbound/message-action-runner.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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 1, 2026
@849261680

849261680 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

I added full local Gateway/qa-channel runtime proof and pushed an additional fix found by that proof:

proof=message-loop-runtime
provider_requests=27
gateway_observed_block=true
qa_channel_outbound_marker_count=20
run_status=critical_block_observed
[diagnostic] tool loop: sessionId=bb60d9da-bbdf-40db-b162-1b78f8bacb84 sessionKey=agent:qa:message-loop-proof-89143 tool=message level=critical action=block detector=generic_repeat count=20 message="CRITICAL: Called message with identical arguments and identical outcomes 20 times. Session execution blocked to prevent runaway loops."

The proof starts a real Gateway child process, qa-channel plugin, qa-lab bus, and local OpenAI-compatible provider that keeps requesting the same message.send. It verifies only 20 marker messages reach qa-channel and later retries are blocked.

Also rerun:

node scripts/run-vitest.mjs src/agents/tool-loop-detection.test.ts src/agents/tools/message-tool.test.ts src/agents/agent-tools.before-tool-call.e2e.test.ts
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@849261680
849261680 force-pushed the fix/message-loop-detection-hash branch from 78750dd to 97944d9 Compare June 1, 2026 17:35
@849261680

849261680 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Re-requesting after a no-content force-push to retrigger CI on the same patch. Current head includes the Gateway/qa-channel runtime proof described in the PR body and the comment above.

Key proof lines:

proof=message-loop-runtime
gateway_observed_block=true
qa_channel_outbound_marker_count=20
run_status=critical_block_observed
[diagnostic] tool loop: sessionId=bb60d9da-bbdf-40db-b162-1b78f8bacb84 sessionKey=agent:qa:message-loop-proof-89143 tool=message level=critical action=block detector=generic_repeat count=20 message="CRITICAL: Called message with identical arguments and identical outcomes 20 times. Session execution blocked to prevent runaway loops."

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 1, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 2, 2026
Merged
2 tasks
@steipete

steipete commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by the landed canonical fix in #89109.

#89109 merged as 697d2d0 and fixes #89090 by normalizing volatile delivery fields for send-like message outcomes across the core message tool, sessions_send, and provider-docked messaging tools. It also includes the wrapped message-tool action-runner proof that this PR supplied as its strongest runtime coverage signal.

Thanks for the focused fix and proof here; that evidence helped shape the final landed coverage.

@steipete steipete closed this Jun 7, 2026
@849261680
849261680 deleted the fix/message-loop-detection-hash branch June 7, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: loopDetection cannot block message tool loops — volatile messageId in result defeats all critical-level detection paths

2 participants

@849261680 @steipete