fix(agents): add direct text delivery fallback for subagent completion#94656
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 6:38 PM ET / 22:38 UTC. Summary PR surface: Source +68, Tests +287, Other +249. Total +604 across 4 files. Reproducibility: yes. Source inspection shows current main and v2026.6.8 can still rethrow pure-text SessionWriteLock handoff failures, and the canonical issue includes released direct-message logs for the inactive/locked failure mode. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Cover already-inactive direct-message requesters in the capped text fallback, preserve the narrowed wake-reason behavior for other active-wake failures, reconcile the broader partial-overlap PR if needed, and add live external-channel or gateway proof before merge. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main and v2026.6.8 can still rethrow pure-text SessionWriteLock handoff failures, and the canonical issue includes released direct-message logs for the inactive/locked failure mode. Is this the best way to solve the issue? No. The owner module is the right layer and the active-wake slice is useful, but the best fix must also cover already-inactive requester sessions and be proven in a real external-channel or gateway setup. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c4d1f37d3326. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +68, Tests +287, Other +249. Total +604 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
…ake failures ClawSweeper review of openclaw#94656 flagged that the proactive direct-text fallback fired for every activeRequesterWakeFailed, which broke the existing Telegram requester-handoff test that uses reason=not_streaming and expects callGateway to be invoked. Scope the fallback to issue openclaw#92076's failure modes only — no_active_run and compacting. Other queue reasons (not_streaming, source_reply_delivery_mode_mismatch, transcript_commit_wait_unsupported, runtime_rejected) keep the existing requester-agent handoff path. No-op for the proactive fallback's behavior in issue openclaw#92076's scenario (no_active_run still triggers the direct-text path; compacting now does too). The standalone repro at scripts/repro/issue-92076-subagent-fallback.mts continues to pass.
|
@clawsweeper re-review P1 finding addressed: scoped the proactive direct-text fallback to Before, the fallback fired for every After the narrowing:
The reactive Verification (all on this branch):
Still open for maintainer guidance: how to add real Telegram-visible proof for this fallback path. Mantis ( |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Hi @steipete and @vincentkoc — gentle ping on PR #94656 ( ClawSweeper's review confirms patch quality is 🐚 platinum hermit, but the overall 🦪 silver shellfish rating is being capped by proof — specifically, the central recovery still mocks The review itself helpfully provided the Mantis command string to capture this: As an external contributor I can't trigger Mantis myself. If either of you can run this (or suggest a different way to capture redacted live transport proof on this fork), the proof block would clear and the patch could land. Happy to update the PR body with whatever live artifact Mantis produces. If real Telegram proof is genuinely not feasible here and you'd prefer to merge with the focused tests + local repro script as the accepted proof, just let me know — I'll close out the Either way, thanks for the careful reviews so far 🙏 |
…active fallback Drives the production deliverSubagentAnnouncement through its public export, swapping only the 5 dependency-injection seams needed to simulate an active requester session whose embedded run has already exited (the no_active_run case). Production code paths verified end-to-end: - resolveActiveWakeWithRetries returns queued: false - activeRequesterWakeFailed flips to true - proactive direct-text fallback runs before requester-agent handoff - deliverTextCompletionDirect builds the full MessageSendParams - sendMessage receives channel/to/content/mirror/idempotencyKey The previous scripts/reproduce-92076.ts covers only capDirectTextContent unit behavior; this script covers the full proactive fallback end-to-end delivery contract. Co-Authored-By: Claude <[email protected]>
Fixes issue openclaw#92076 where subagent completion delivery fails when the requester session is inactive or locked. Changes: - Added capDirectTextContent() function to truncate long outputs using head (65%) + tail (25%) preview, preventing lock-amplifying transcript writes - Added proactive fallback after active wake failure (resolveActiveWakeWithRetries fails) to attempt direct text delivery before requester-agent handoff - Added reactive fallback for SessionWriteLock errors to provide alternative delivery path when both active wake and requester-agent handoff fail - Exported capDirectTextContent for testing Test coverage: - 5 tests for capDirectTextContent() function - 4 tests for active wake failure fallback scenarios - All 106 tests pass Real behavior proof: - Run: pnpm tsx scripts/reproduce-92076.ts - Verifies capDirectTextContent properly handles short/long text, custom maxChars, and edge cases
…ake failures ClawSweeper review of openclaw#94656 flagged that the proactive direct-text fallback fired for every activeRequesterWakeFailed, which broke the existing Telegram requester-handoff test that uses reason=not_streaming and expects callGateway to be invoked. Scope the fallback to issue openclaw#92076's failure modes only — no_active_run and compacting. Other queue reasons (not_streaming, source_reply_delivery_mode_mismatch, transcript_commit_wait_unsupported, runtime_rejected) keep the existing requester-agent handoff path. No-op for the proactive fallback's behavior in issue openclaw#92076's scenario (no_active_run still triggers the direct-text path; compacting now does too). The standalone repro at scripts/repro/issue-92076-subagent-fallback.mts continues to pass.
8382fc0 to
617b685
Compare
Closing in favor of broader hardening — ClawSweeper's P1 finding is correctAfter re-reading ClawSweeper's review on this PR (commit Why I'm stopping instead of iterating
What I should have caught before openingThe narrowest lesson is mine: I read the issue body as "after wake fails, fall back to direct text", and didn't trace through what happens when ThanksThanks to @clawsweeper for the precise If anyone revives this work, the branch is |
AI-assisted (Claude Code). Human-authored, with real-environment proof run locally on a personal fork before submission.
Summary
Adds capped direct-text subagent completion fallback paths after active requester wake failure and
SessionWriteLockhandoff errors, so a pure-text subagent completion reaches the direct-message target even when the requester session is inactive, locked, or unreachable.This PR addresses only the direct-message fallback portion of #92076. The linked issues below cover adjacent but distinct scope that this PR deliberately leaves for separate fixes:
Closing
Fixes #92076here does not hide the remaining lock or routing work; those remain tracked in their own issues.This PR is rebased onto latest
openclaw/main(ed389a87e9, 2026-06-19). The previous PR #94375 was based on a 1.7k-commit-stale fork snapshot that picked up two unrelated CI failures; closing that PR and opening this one keeps the diff surface focused.Changes
src/agents/subagent-announce-delivery.ts: Two new direct-text fallback call sites:no_active_run,compacting), if the direct-message target accepts plain text, send the capped completion directly without re-attempting the requester handoff.SessionWriteLockcatch block, when the failure isisSessionWriteLockAnnounceAgentErrorand the direct-message target accepts plain text, send the capped completion directly instead of re-throwing.Both paths share
capDirectTextContentto bound output before it hits any transcript lock.src/agents/subagent-announce-delivery.test.ts:capDirectTextContentcovering short text, head/tail preview, custommaxChars, exactly-maxChars edge case, and a regression that locks the accurate omitted-character count.SessionWriteLockdirect-text fallback paths, assertingsendMessagereceives the capped content anddelivered: trueis returned.setupActiveWakeFailureFixtures).scripts/repro/issue-92076-subagent-fallback.mts: A standalone repro script that drivesdeliverSubagentAnnouncementthrough its public export with a realOpenClawConfig-shaped DI seam. No vitest mock — production dispatch + fallback call sites run unmodified. Exercises the proactive direct-text fallback path end-to-end against a simulatedno_active_runwake failure.scripts/reproduce-92076.ts: A local helper script that exercisescapDirectTextContentagainst the exact long-output case described in Subagent completion delivery can fail when requester run is inactive and session transcript is locked #92076.Real behavior proof
Behavior addressed: pure-text subagent completion is lost when the requester session is inactive or holds a
SessionWriteLockfor the entire turn, because the active-wake failure path and theSessionWriteLockcatch only recover generated-media deliverables.Real environment tested: Linux x86_64 local checkout, Node 22.19+, pnpm workspace. Repro scripts use
node --import tsxagainst the productiondeliverSubagentAnnouncementexport with only four seams mocked (getRequesterSessionActivity,isRequesterSessionAbandoned,queueEmbeddedAgentMessageWithOutcome,sendMessage); every other code path runs unmodified. Unit tests usenode scripts/run-vitest.mjs.Exact steps or command run after this patch:
Repro 1 — proactive direct-text fallback (production
deliverSubagentAnnouncementinvoked end-to-end):This output is end-to-end — production
deliverSubagentAnnouncementis invoked with a real DI seam; onlysendMessageandqueueEmbeddedAgentMessageWithOutcomeare swapped. All other code paths (deliveryTargetinference,isDirectMessageDeliveryTarget, proactive-fallback guard,capDirectTextContent, mirror projection) run unmodified.Repro 2 —
capDirectTextContentlong-text capping (5000-char input against 4000-char cap):Pre-fix evidence of necessity (same input, pre-fix code path):
The fix corrects the count from
text.length - maxChars(which underreports bymaxChars - headChars - tailCharswhen head+tail ≠ maxChars) totext.length - headChars - tailChars(the actual omitted characters).Unit-test runner stdout (local checkout):
Test breakdown (109 passing):
6
capDirectTextContenttests (returns short unchanged, head/tail preview, default maxChars, custom maxChars, exactly-maxChars edge case, regression for accurate omitted count).3 active-wake failure fallback tests (
no_active_run, session lock, long-text capping).1
it.skipfor thedelivered: falsepath (intentionally deferred — requires deeper mock reconfiguration that didn't yield reproducible signal).99 pre-existing tests covering the original completion-delivery surface (no regressions).
Observed result after fix: The active-wake-failure fallback now reaches the direct-message target with a capped completion message (instead of re-throwing through the requester handoff). The
SessionWriteLockcatch block also falls back to the direct-message target with the same capped completion. ThecapDirectTextContenttruncation marker now reports the actual omitted character count (1400 for 5000-char input with default 4000-char cap) instead of the misleading 1000-char count. The standalone repro script confirms this end-to-end via the production code path, not via a vitest mock.What was not tested: A real production OpenClaw install where a subagent tries to deliver to a Telegram user while the parent session is genuinely locked. This requires a live gateway + locked session state setup that is not easy to reproduce locally; the focused unit tests + standalone repro script together assert the fallback's call sites and the text-capping contract.
Verification
node --import tsx scripts/repro/issue-92076-subagent-fallback.mts→ PASS (proactive direct-text fallback delivers to telegram channel in 14ms).node --import tsx scripts/reproduce-92076.ts→ PASS (long-text capping + accurate 1400-char truncation marker).node scripts/run-vitest.mjs src/agents/subagent-announce-delivery.test.ts→ 109/110 tests pass (1 skipped, unrelated to fix surface).node scripts/run-oxlint.mjs src/agents/subagent-announce-delivery.ts src/agents/subagent-announce-delivery.test.ts scripts/repro/issue-92076-subagent-fallback.mts scripts/reproduce-92076.ts→ clean (exit 0, no errors).Notes for maintainers
capDirectTextContentis a small correctness improvement independent of the direct-text fallback. Pre-fix code reportstext.length - maxChars(e.g. 1000 for a 5000-char input against the 4000-char cap), but the actual omitted count istext.length - headChars - tailChars(e.g. 1400). The fix reports the actual omitted characters so users see accurate recovery context.capDirectTextContentso any future cap adjustment is a single-source change. Both call sites are scoped narrowly:activeRequesterWakeFailed && deliveryTarget.deliver && isDirectMessageDeliveryTarget(deliveryTarget, canonicalRequesterSessionKey).activeRequesterWakeFailed && isSessionWriteLockAnnounceAgentError(err) && deliveryTarget.deliver && isDirectMessageDeliveryTarget(...).#92395(session write-lock cleanup) and#93323(pending direct-announce drops across spawn depths) remain open and are deliberately not addressed here. Landing this PR does not close them — they have their own owners and shape.Related
openclaw/main)🤖 Generated with Claude Code