Skip to content

fix(agents): classify embedded prompt lock-changed error as permanent announce failure (fixes #91527)#91641

Closed
zenglingbiao wants to merge 2 commits into
openclaw:mainfrom
zenglingbiao:fix/issue-91527-subagent-announce-lock-error
Closed

fix(agents): classify embedded prompt lock-changed error as permanent announce failure (fixes #91527)#91641
zenglingbiao wants to merge 2 commits into
openclaw:mainfrom
zenglingbiao:fix/issue-91527-subagent-announce-lock-error

Conversation

@zenglingbiao

@zenglingbiao zenglingbiao commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: Subagent completion announcements produce 3× duplicate Telegram messages when the transcript mirror fails with EmbeddedPromptLockFileChangedError after a successful outbound send.
  • Root Cause: PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS in subagent-announce-delivery.ts does not include the lock-changed error, so the announce retry loop classifies it as retriable and re-sends the completion up to maxAnnounceRetryCount times.
  • Fix: Add /session file changed while embedded prompt lock was released/i to PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS so the error is treated as terminal, matching the pattern established by PR fix(outbound): keep channel send durable when transcript mirror fails (#89626) #89812 in the general outbound delivery path.
  • What changed:
    • src/agents/subagent-announce-delivery.ts — added one regex pattern to the permanent error array
  • What did NOT change (scope boundary):

Reproduction

  1. From a parent session, call sessions_spawn(taskName="...", mode="run") with a long-running subagent task
  2. Parent calls sessions_yield to wait for the auto-announce
  3. The parent session is concurrently modified by deferred turn maintenance while the subagent completion attempts to mirror to the transcript
  4. The transcript mirror throws EmbeddedPromptLockFileChangedError
  5. Before this PR: The announce retry loop fires 3 times, each retry independently triggering an outbound send → user receives 3 identical messages
  6. After this PR: The lock-changed error is classified as permanent, preventing retry → user receives exactly 1 message

Real behavior proof

Behavior or issue addressed (#91527): When a subagent completion has already been sent to the external channel but the transcript mirror fails with EmbeddedPromptLockFileChangedError, the announce delivery layer now treats the error as terminal instead of retrying, preventing duplicate message delivery.

Real environment tested: Linux, Node 22 — deliverSlackChannelAnnouncement exercised through the completion delivery path with Gateway and channel send mocks

Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/subagent-announce-delivery.test.ts src/agents/subagent-announce-dispatch.test.ts src/agents/subagent-registry-lifecycle.test.ts

Evidence after fix:

✓ src/agents/subagent-announce-delivery.test.ts (98 tests)
✓ src/agents/subagent-announce-dispatch.test.ts (22 tests)  
✓ src/agents/subagent-registry-lifecycle.test.ts (19 tests)
Test Files  3 passed (3)
Tests  139 passed (139)

Observed result after fix: The new test case "treats embedded prompt lock-changed error as permanent during completion delivery" verifies that when callGateway throws the lock-changed error during a text completion delivery, the result correctly has delivered: false with the error message preserved, and sendMessage is never called — confirming no duplicate outbound send.

What was not tested: Live Telegram end-to-end delivery with concurrent session modifications was not driven; the fix was validated at the announce delivery unit level with mocked Gateway and channel sends, matching the existing test patterns in the file.

Repro confirmation: The new test case throws the exact error message from the reporter's logs (session file changed while embedded prompt lock was released: /tmp/session.jsonl) and confirms the result is delivered: false with no sendMessage call — on origin/main without this patch, the same error would have been classified as retriable and the announce lifecycle would have retried up to 3 times.

Risk / Mitigation

  • Risk: Classifying the lock-changed error as permanent means the subagent completion will not be retried if the transcript mirror fails, potentially losing the completion from the session transcript.
    Mitigation: The outbound message has already been sent successfully before the mirror failure ([telegram] outbound send ok appears in logs before the error). The user still receives the completion — only the transcript archival is skipped, and the next turn will naturally include the context.

  • Risk: The regex pattern may match unrelated errors containing the same substring.
    Mitigation: The error message is specific (session file changed while embedded prompt lock was released) and produced by a single code path in EmbeddedPromptLockFileChangedError. The existing permanent patterns in the same array use similar specificity (e.g., /unsupported channel/i, /chat not found/i).

Change Type (select all)

  • Bug fix

Scope (select all touched areas)

  • agents — subagent announce delivery and error classification

Regression Test Plan

  • node scripts/run-vitest.mjs src/agents/subagent-announce-delivery.test.ts
  • node scripts/run-vitest.mjs src/agents/subagent-announce-dispatch.test.ts
  • node scripts/run-vitest.mjs src/agents/subagent-registry-lifecycle.test.ts

Review Findings Addressed

N/A (initial submission)

Linked Issue/PR

Fixes #91527

@zenglingbiao
fix(agents): classify embedded prompt lock-changed error as permanent…
1e8e94d 
… announce failure (fixes openclaw#91527)

When a subagent completion is successfully sent to an external channel
but the subsequent transcript mirror fails with EmbeddedPromptLockFileChangedError,
the announce retry loop re-sends the completion, causing duplicate messages.

Add the lock-changed error pattern to PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS
so the announce delivery layer treats it as terminal instead of retrying,
mirroring the pattern established by PR openclaw#89812 in the general outbound path.
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 9, 2026
@clawsweeper

clawsweeper Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 9, 2026, 3:04 AM ET / 07:04 UTC.

Summary
The PR adds the embedded prompt lock-change message to subagent announce permanent error patterns and adds a mocked delivery regression test.

PR surface: Source +1, Tests +46. Total +47 across 2 files.

Reproducibility: yes. source inspection plus the linked live report gives a high-confidence path: the lock-change error is classified in announce delivery, then a non-terminal failed direct result can flow into fallback or lifecycle retry. I did not run a live Telegram reproduction in this read-only review.

Review metrics: none identified.

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

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

Rank-up moves:

  • Propagate terminal or already-visible-send state through direct delivery, dispatch, and lifecycle.
  • [P2] Add a regression that proves no fallback steering or pending final-delivery retry occurs for the lock-change case.
  • [P1] Add redacted live Telegram, runtime-log, or comparable real behavior proof showing one completion message after the fix.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies mocked Vitest output only; it does not show the reported Telegram-visible duplicate-message scenario after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A live Telegram transcript is the most direct proof that this change prevents duplicate completion delivery in the reported transport. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

telegram live: reproduce subagent completion with embedded prompt lock-change and verify the Telegram DM receives one completion message, not three.

Risk before merge

  • [P2] Merging as-is can still leave a post-send lock-change failure as a non-terminal direct failure, allowing fallback steering or lifecycle retry to replay a visible completion message.
  • [P1] The contributor proof is mocked unit output only, so it does not demonstrate the reported Telegram duplicate-message scenario after the patch.

Maintainer options:

  1. Propagate already-sent terminal state (recommended)
    Return terminal or already-delivered semantics for this permanent post-send mirror failure and cover that dispatch/lifecycle path before merge.
  2. Require real transport proof
    After the code path is fixed, require redacted live Telegram or runtime-log proof showing one completion delivery instead of duplicate sends.
  3. Accept partial mitigation only
    Maintainers could land the regex as an inner-retry mitigation, but it should be accepted as incomplete for the linked duplicate-delivery bug.

Next step before merge

  • [P1] Human review and contributor action are needed because the PR has a concrete P1 message-delivery finding and still lacks real behavior proof.

Security
Cleared: The diff only adds one TypeScript regex and a unit test, with no dependency, CI, package, permission, secret, or code-execution surface changed.

Review findings

  • [P1] Propagate the lock-change failure as terminal — src/agents/subagent-announce-delivery.ts:381
Review details

Best possible solution:

Carry post-send lock-change errors as terminal or already visibly delivered through direct delivery, dispatch, and lifecycle, with regression coverage plus redacted real Telegram or runtime-log proof showing one completion.

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

Yes, source inspection plus the linked live report gives a high-confidence path: the lock-change error is classified in announce delivery, then a non-terminal failed direct result can flow into fallback or lifecycle retry. I did not run a live Telegram reproduction in this read-only review.

Is this the best way to solve the issue?

No. Adding the regex is a plausible inner-retry mitigation, but it is not the best complete fix unless the permanent post-send failure also stops dispatch fallback and lifecycle replay.

Full review comments:

  • [P1] Propagate the lock-change failure as terminal — src/agents/subagent-announce-delivery.ts:381
    Adding this regex only stops the inner transient retry loop. The permanent error is rethrown into the outer direct-delivery catch, which still returns a plain non-terminal direct failure; completion dispatch then may fallback-steer, and lifecycle can mark pending final delivery for retry. Carry terminal or already-visible-send state through dispatch/lifecycle and cover that path in tests.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The PR targets an active duplicate message delivery bug in subagent completion announcements and still leaves the urgent replay path incomplete.
  • merge-risk: 🚨 message-delivery: The diff changes announce retry classification, but current dispatch and lifecycle behavior can still replay or reroute a completion after a visible send.
  • 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: The PR body supplies mocked Vitest output only; it does not show the reported Telegram-visible duplicate-message scenario after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The affected behavior is visible duplicate Telegram completion messages, so Telegram proof would materially help review.
Evidence reviewed

PR surface:

Source +1, Tests +46. Total +47 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 0 +1
Tests 1 46 0 +46
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 47 0 +47

What I checked:

Likely related people:

  • Dallin Romney: Local blame on the permanent-pattern list, dispatch terminal gate, and lifecycle pending retry state points to the current source snapshot commit that last touched these files in this checkout. (role: recent current-main source snapshot contributor; confidence: medium; commits: 5097749de32c; files: src/agents/subagent-announce-delivery.ts, src/agents/subagent-announce-dispatch.ts, src/agents/subagent-registry-lifecycle.ts)
  • steipete: GitHub file history shows repeated recent work on subagent completion delivery, dispatch routing, lifecycle state, and related docs in the affected files. (role: feature-history contributor; confidence: high; commits: 67436918f388, 7f28c8bd07d3, beb42b12c989; files: src/agents/subagent-announce-delivery.ts, src/agents/subagent-announce-dispatch.ts, src/agents/subagent-registry-lifecycle.ts)
  • vincentkoc: GitHub file history shows recent fixes in subagent completion handoff and skipped delivery state near the same announce delivery and lifecycle surfaces. (role: recent area contributor; confidence: medium; commits: e0018382eb00, 7e0d275f7add; files: src/agents/subagent-announce-delivery.ts, src/agents/subagent-registry-lifecycle.ts)
  • joshavant: GitHub file history links prior subagent completion announce delivery timing and DM completion delivery fixes to the dispatch and delivery surfaces involved here. (role: feature-history contributor; confidence: medium; commits: 903d9c13f3ee, 5d3a6909fb20; files: src/agents/subagent-announce-dispatch.ts, src/agents/subagent-announce-delivery.ts)
  • harjothkhara: The related merged outbound mirror fix that this PR tries to mirror credited this contributor and changed the sibling general outbound delivery path. (role: adjacent outbound mirror fix contributor; confidence: medium; commits: 79896a24d9bb; files: src/infra/outbound/deliver.ts, src/infra/outbound/deliver.test.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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. 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 9, 2026
@zenglingbiao

zenglingbiao commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 9, 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:

This was referenced Jun 17, 2026
Merged
Closed
@zenglingbiao
zenglingbiao deleted the fix/issue-91527-subagent-announce-lock-error branch June 22, 2026 03:42
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 mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. 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

Milestone

No milestone

1 participant

@zenglingbiao