fix(channels): add idempotency guard to nack() callback (#104903) by evan-YM · Pull Request #104928 · openclaw/openclaw

evan-YM · 2026-07-12T03:26:01Z

Summary

Repeated sequential calls to a channel message receive context's nack() invoke onNack more than once. Add an already-nacked guard so completed nack side effects run once, while preserving the existing ack-to-nack transition and callback-retry contract.

Problem: nack() unconditionally calls await params.onNack?.(error) and only sets ackState = "nacked" after the callback succeeds. A second sequential call repeats the completed callback.
Solution: Add if (ctx.ackState === "nacked") return; before the callback — the narrowest guard that preserves ack-to-nack and retry-after-rejection.
What changed: src/channels/message/receive.ts (+5 guard + comment), src/channels/message/lifecycle.test.ts (+54: duplicate-nack test, rejected-callback retry test), scripts/verify-nack-idempotency.ts (+109: standalone tsx verification script)
What did NOT change: The ack-to-nack transition (ack → nack still works), retry-after-rejection (a rejected callback keeps the context pending), the public SDK contract shape

Change Type & Scope

Change Type

Scope

Linked Issue

Fixes fix(channels): nack() 缺少幂等性保护，重复调用会多次触发 onNack 回调 #104903
This PR fixes a bug or regression

Motivation

The shared nack() implementation in the channel message receive context is part of the public plugin SDK used by bundled channels (Telegram, LINE) and third-party plugins. The ack() method already has an idempotency guard (if (ctx.ackState === "acked") return;), but nack() was shipped without equivalent protection. Receive pipelines that revisit a completed failed stage can trigger duplicate onNack callbacks, leading to repeated error handling, cleanup, or logging.

Review Contract

Ref: #104903 / PR #104919 (competing, needs proof)
Surface: channels / message receive lifecycle

Bug: Completed sequential nack() calls invoke onNack callback more than once.
Cause: nack() unconditionally calls await onNack before setting ackState to "nacked",
       with no already-nacked guard. Confirmed in both v2026.6.11 and current main.
Provenance:
  introduced by: 8bfabd6bb139 — 2026-05-06
  made visible by: v2026.6.11 (shipped)
  Confidence: clear

Best fix: Yes. The shared receive-context guard is the narrowest owner-level fix.
  A pending-only guard (ackState !== "pending") would be broader but breaks the
  existing ack-to-nack transition tested since introduction.
Refactor: No
Proof: Real terminal output (tsx production-module verification + vitest suite)
Risk: None — the guard gates only completed nack paths; the Telegram
  bot-update-tracker and LINE webhook callers all invoke nack() exactly once
  per context, so production behavior is unchanged.

Real Behavior Proof

Behavior addressed: Completed sequential nack() calls invoked onNack repeatedly; a second call after a successful first nack would re-fire the callback.

Real environment tested: Linux x86_64, Node 24.13.1, fix/nack-idempotency-guard-104903 branch, production TS module loaded via node --import tsx.

Exact steps or command run after this patch:

# Standalone production-module verification (no vitest)
$ node --import tsx scripts/verify-nack-idempotency.ts

# Focused lifecycle regression suite
$ node scripts/run-vitest.mjs src/channels/message/lifecycle.test.ts

# Real Telegram caller regression suite
$ node scripts/run-vitest.mjs extensions/telegram/src/bot-update-tracker.test.ts

Evidence after fix:

$ node --import tsx scripts/verify-nack-idempotency.ts

=== nack() idempotency real-behavior verification ===

Test 1: Duplicate sequential nack calls onNack only once
  ✓ onNack called once after first nack
  ✓ ackState is 'nacked'
  ✓ nackErrorMessage preserved
  ✓ onNack still called only once after duplicate nack
  ✓ ackState remains 'nacked'
  ✓ first error message preserved

Test 2: Rejected onNack callback leaves context retryable
  ✓ onNack called once (first attempt rejected)
  ✓ ackState stays 'pending' after rejection
  ✓ nackErrorMessage not set after rejection
  ✓ onNack called again on retry
  ✓ ackState transitions to 'nacked' after success
  ✓ nackErrorMessage set after success

Test 3: ack-to-nack transition preserved
  ✓ ackState is 'acked' after ack()
  ✓ onNack called once after ack-to-nack transition
  ✓ ackState transitions to 'nacked' from 'acked'
  ✓ nackErrorMessage set

=== Results: 16 passed, 0 failed ===

$ node scripts/run-vitest.mjs src/channels/message/lifecycle.test.ts

 Test Files  1 passed (1)
      Tests  18 passed (18)
   Duration  852ms

[test] passed 1 Vitest shard in 6.11s

$ node scripts/run-vitest.mjs extensions/telegram/src/bot-update-tracker.test.ts

 Test Files  1 passed (1)
      Tests  8 passed (8)
   Duration  15.44s

[test] passed 1 Vitest shard in 21.48s

Matrix evidence:

nack call sequence                    => onNack calls  ackState
nack("err1")                          => 1             "nacked"
nack("err1") → nack("err2")          => 1             "nacked"
ack() → nack("err")                   => 1             "nacked"
nack("err") → reject → nack("err")   => 2             "nacked"

Observed result after fix:

Duplicate successful sequential nack calls invoke onNack exactly once
First nack's error message and state are preserved for duplicate calls
A rejected onNack callback leaves ackState as "pending", allowing retry
The ack-to-nack transition (tested since the SDK was introduced) still works

What was not tested:

Live Telegram/LINE webhook end-to-end delivery (requires production bot tokens)
Concurrent nack calls from separate async contexts (single-context by design)
Broad remote changed-files gate (Blacksmith Testbox unavailable locally)

Verification environment

OS: Linux x86_64 (4.19.112-2.el8)
Runtime: Node 24.13.1
Integration/channel: Telegram tracker, LINE webhook (via vitest suites)
Relevant config (redacted): N/A — no credentials needed for this code path

Risks and Mitigations

Highest risk area: None — the guard gates only already-completed nack paths. The three production callers (Telegram bot-update-tracker.ts, LINE webhook.ts, LINE webhook-node.ts) each call nack() exactly once per receive context, so no existing behavior changes.
Mitigation: The guard intentionally preserves both the ack-to-nack transition and retry-after-rejection because state mutation remains after await onNack. All 26 focused tests (18 lifecycle + 8 Telegram tracker) pass.
Compatibility impact: None. The public MessageReceiveContext type and createMessageReceiveContext factory signature are unchanged. Third-party plugins using the SDK see the same contract with fixed idempotency.

Security Impact

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No

Compatibility / Migration

Backward compatible? Yes
Config/env changes? No
Migration needed? No

Human Verification

Verified scenarios: Duplicate sequential nack, rejected callback retry, ack-to-nack transition, Telegram failure-completion path
Edge cases checked: nack after ack, nack after nack, nack with throwing callback, nack with undefined error
What you did NOT verify: Live channel webhook delivery (requires real bot credentials), concurrent multi-context nack scenarios

AI Assistance 🤖

AI-assisted: Yes
Co-Authored-By: Claude [email protected]
Human confirmed understanding of code changes: Yes
AI prompts / session excerpts: Identified missing idempotency guard by comparing ack() vs nack() implementations in src/channels/message/receive.ts. ClawSweeper issue review confirmed the already-nacked approach over a broader pending-only guard.

) The shared nack() implementation in the channel message receive context lacked an already-nacked guard, so repeated sequential calls to nack() invoked onNack more than once. Add a guard that preserves the existing ack-to-nack transition and leaves rejected nack callbacks retryable. Add regression coverage: duplicate sequential nack idempotency, retry after a rejected onNack callback, and ack-to-nack transition. Add a standalone tsx verification script (scripts/verify-nack-idempotency.ts) that exercises the production receive module directly and reports 16/16 assertions passing as real-behavior proof. Co-Authored-By: Claude <[email protected]>

clawsweeper · 2026-07-12T03:55:03Z

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 11:53 PM ET / July 12, 2026, 03:53 UTC.

Summary
The PR adds a completed-nack() idempotency guard in the shared channel receive context, two lifecycle regression tests, and a standalone production-module verification script.

PR surface: Source +5, Tests +54, Other +111. Total +170 across 3 files.

Reproducibility: yes. at source level. Current main deterministically re-enters onNack when nack() is called twice after a successful first callback, and both candidate PRs provide matching after-fix production-module output.

Review metrics: 1 noteworthy metric.

Long-lived verification surface: 1 standalone script added. The competing fix has equivalent production-module proof without adding an unintegrated permanent script under scripts/.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #104903
Summary: The canonical issue has two open, proof-positive candidate fixes implementing the same shared-helper invariant.

Members:

canonical: fix(channels): nack() 缺少幂等性保护，重复调用会多次触发 onNack 回调 #104903 - It reports and reproduces the exact duplicate onNack callback behavior addressed by both PRs.
same_root_cause: fix(channels): make nack callbacks idempotent #104919 - It implements the same nacked-only guard and retry contract with a narrower repository diff.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

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:

none.

Risk before merge

[P1] Two proof-positive contributor PRs remain open for the same canonical issue, so maintainers must select one landing path before either branch is treated as canonical.
[P1] This branch promotes a one-off runtime proof into a permanent script without an ongoing package/check integration; the narrower competing PR provides equivalent behavioral proof without that maintenance surface.

Maintainer options:

Decide the mitigation before merge
Land one nacked-only shared-helper guard with focused lifecycle tests, preferably through the narrower branch unless the verifier is intentionally adopted as maintained tooling, then close the redundant PR and canonical issue with credit to both investigations.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P2] No automated code repair is needed; a maintainer should choose the canonical proof-positive branch, then resolve the overlapping PR and linked issue after landing.

Maintainer decision needed

Question: Should fix(channels): make nack callbacks idempotent #104919 be the canonical fix, or does the standalone verifier in this PR provide enough ongoing repository value to prefer this branch?
Rationale: Both runtime patches are correct and proof-positive; choosing between overlapping contributor branches and accepting a permanent one-off script requires maintainer ownership rather than automated repair.
Likely owner: steipete — The original shared lifecycle API and state-transition contract trace directly to this feature contributor.
Options:
- Prefer the narrower PR (recommended): Land fix(channels): make nack callbacks idempotent #104919, then close this PR with credit for its equivalent investigation and stronger scripted proof.
- Prefer this verifier branch: Keep this PR as canonical only if maintainers want scripts/verify-nack-idempotency.ts retained and maintained as an ongoing verification surface.

Security
Cleared: The runtime, tests, and local verifier introduce no concrete dependency, secret, permission, network, artifact-download, or supply-chain concern.

Review details

Best possible solution:

Land one nacked-only shared-helper guard with focused lifecycle tests, preferably through the narrower branch unless the verifier is intentionally adopted as maintained tooling, then close the redundant PR and canonical issue with credit to both investigations.

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

Yes at source level. Current main deterministically re-enters onNack when nack() is called twice after a successful first callback, and both candidate PRs provide matching after-fix production-module output.

Is this the best way to solve the issue?

Yes for the runtime approach: the shared receive context owns this invariant, and a nacked-only guard preserves the existing ack-to-nack transition and retry after callback rejection. The narrower competing branch is the cleaner landing vehicle unless maintainers explicitly want the standalone verifier.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

P2: The PR fixes duplicate callback side effects in a shared channel lifecycle helper with a bounded, well-defined blast radius.
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 (terminal): The exact-head terminal transcript imports the production receive module and demonstrates completed-call suppression, callback-rejection retry, and the preserved ack-to-nack transition; focused lifecycle and Telegram tests supplement that proof.
proof: sufficient: Contributor real behavior proof is sufficient. The exact-head terminal transcript imports the production receive module and demonstrates completed-call suppression, callback-rejection retry, and the preserved ack-to-nack transition; focused lifecycle and Telegram tests supplement that proof.

Evidence reviewed

PR surface:

Source +5, Tests +54, Other +111. Total +170 across 3 files.

View PR surface stats

Area	Files	Added	Net
Source	1	5	+5
Tests	1	54	+54
Docs	0	0	0
Config	0	0	0
Generated	0	0	0
Other	1	111	+111
Total	3	170	+170

What I checked:

Current-main defect: Current main calls onNack before recording the nacked state and has no completed-state guard, so a second sequential successful nack() call repeats the callback. (src/channels/message/receive.ts:90, 6b95f98fe70d)
Existing lifecycle contract: The adjacent lifecycle test permits an ack-to-nack transition, ruling out the issue's broader non-pending guard and supporting this PR's narrower nacked-only check. (src/channels/message/lifecycle.test.ts:346, 6b95f98fe70d)
Shared callers: Telegram and LINE use the shared receive context and invoke nack() on failure paths, confirming that the invariant belongs in the shared helper rather than one channel. (extensions/telegram/src/bot-update-tracker.ts:251, 6b95f98fe70d)
Feature provenance: Commit 8bfabd6 introduced the receive-context lifecycle API and lifecycle tests together, including the current ack/nack state model. (src/channels/message/receive.ts:58, 8bfabd6bb139)
Competing viable PR: The open competing PR implements the same nacked-only guard and retry regression coverage in two files, is mergeable, and has sufficient production-module proof. (src/channels/message/receive.ts:90, 4583b9c0edb5)
Exact branch scope: This branch adds the same runtime guard and overlapping tests, plus a 111-line standalone verifier with no discovered package-script or check integration. (scripts/verify-nack-idempotency.ts:1, 595c6010a45b)

Likely related people:

steipete: Authored the commit that introduced the shared channel message lifecycle receive context and its lifecycle tests. (role: original feature contributor; confidence: high; commits: 8bfabd6bb139; files: src/channels/message/receive.ts, src/channels/message/lifecycle.test.ts)
Vincent Koc: Has substantial sampled history across the relevant current channel caller files, including LINE webhook ownership and channel/plugin boundary work. (role: adjacent channel owner; confidence: medium; commits: 213198123042, 57c47d8c7fbf; files: extensions/line/src/webhook.ts, extensions/line/src/webhook-node.ts, extensions/telegram/src/bot-update-tracker.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.

steipete · 2026-07-12T17:13:00Z

Closing as superseded by #104919, which is now the canonical fix for #104903.

Thank you @evan-YM—the sequential idempotency and retry-after-failure cases here helped validate the lifecycle contract. The landed branch keeps those guarantees, also coalesces overlapping nack() calls onto one in-flight rejection, preserves the first successful rejection error, and covers the shared lifecycle plus Telegram adapter without adding a standalone verifier script.

Canonical landing: #104919

openclaw-barnacle Bot added scripts Repository scripts size: S labels Jul 12, 2026

evan-YM force-pushed the fix/nack-idempotency-guard-104903 branch from a4d0433 to 595c601 Compare July 12, 2026 03:43

clawsweeper Bot mentioned this pull request Jul 12, 2026

fix(channels): make nack callbacks idempotent #104919

Merged

clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 12, 2026

clawsweeper Bot mentioned this pull request Jul 12, 2026

fix(channels): add idempotency guard to nack() (#104903) #105009

Closed

steipete closed this Jul 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(channels): add idempotency guard to nack() callback (#104903)#104928

fix(channels): add idempotency guard to nack() callback (#104903)#104928
evan-YM wants to merge 1 commit into
openclaw:mainfrom
evan-YM:fix/nack-idempotency-guard-104903

evan-YM commented Jul 12, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 12, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

evan-YM commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type & Scope

Change Type

Scope

Linked Issue

Motivation

Review Contract

Real Behavior Proof

Verification environment

Risks and Mitigations

Security Impact

Compatibility / Migration

Human Verification

AI Assistance 🤖

Uh oh!

clawsweeper Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

evan-YM commented Jul 12, 2026 •

edited

Loading

clawsweeper Bot commented Jul 12, 2026 •

edited

Loading