Skip to content

fix: deliver background task completions through owning sessions#97963

Open
moeedahmed wants to merge 3 commits into
openclaw:mainfrom
moeedahmed:codex/upstream-background-task-heartbeat-delivery
Open

fix: deliver background task completions through owning sessions#97963
moeedahmed wants to merge 3 commits into
openclaw:mainfrom
moeedahmed:codex/upstream-background-task-heartbeat-delivery

Conversation

@moeedahmed

@moeedahmed moeedahmed commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Background task completion events can be queued on the real requester session, such as a Telegram topic session, while heartbeat isolation runs the wake in a separate :heartbeat session. That can leave the requester without a visible final or blocked completion message even though the background task finished.

Required subagent completions also had a second failure mode: progress-only child output could be presented as a successful handoff instead of being treated as blocked.

Why This Change Was Made

The fix keeps the behaviour narrow and native:

  • Background-task completion wake sources now inspect the owning session queue and bypass heartbeat isolation only for background-task and background-task-blocked delivery wakes.
  • Ordinary heartbeat isolation remains enabled for non-background-task wakes.
  • Required subagent completion formatting now reuses the existing required-completion terminal classifier so progress-only output becomes a blocked handoff.
  • The required-completion terminal result type now encodes the invariant that blocked completions always carry a status summary.

User Impact

Users waiting in the original requester session, including Telegram topic sessions, should receive the terminal completion/blocker path instead of losing it to an isolated heartbeat session. Parent agents should no longer treat a progress-only required subagent completion as a successful final deliverable.

Evidence

Local source proof after rebasing onto current origin/main:

  • git diff --check origin/main...HEAD
  • node scripts/run-vitest.mjs src/infra/heartbeat-runner.ghost-reminder.test.ts src/agents/subagent-announce.test.ts src/agents/subagent-announce-delivery.test.ts src/agents/subagent-announce-dispatch.test.ts src/agents/subagent-announce-output.test.ts src/agents/subagent-announce.capture-completion-reply.test.ts src/agents/subagent-announce.format.e2e.test.ts src/agents/subagent-announce.timeout.test.ts
    • Passed: 3 shards, 8 test files, 258 tests total.
  • PATH="/tmp/openclaw-pnpm-shims:$PATH" pnpm tsgo:prod
  • PATH="/tmp/openclaw-pnpm-shims:$PATH" pnpm check:test-types
  • node scripts/run-oxlint.mjs src/agents/subagent-announce.ts src/agents/subagent-announce.format.e2e.test.ts src/infra/heartbeat-runner.ts src/infra/heartbeat-runner.ghost-reminder.test.ts src/tasks/task-completion-contract.ts
  • PATH="/tmp/openclaw-pnpm-shims:$PATH" NODE_OPTIONS=--max-old-space-size=8192 pnpm build:plugin-sdk:strict-smoke
  • PATH="/tmp/openclaw-pnpm-shims:$PATH" pnpm lint:extensions:channels
  • PATH="/tmp/openclaw-pnpm-shims:$PATH" pnpm lint:extensions:bundled
  • node scripts/build-all.mjs gatewayWatch

Upstream CI passed on pushed head 47e4354a287c2148cbc04e863bad77dbc26c8e77, including check-prod-types, check-test-types, check-guards, check-lint, QA Smoke CI, and Real behavior proof.

Known proof gap: live Telegram/Mantis visible delivery proof is still pending. The PR is labelled for mantis: telegram-visible-proof; merge should wait for redacted live proof or an explicit maintainer decision to accept source-level proof only.

Risks and Counterexamples

  • Heartbeat isolation remains enabled for ordinary heartbeat wakes.
  • The owning-session wake bypass is limited to background-task completion delivery sources.
  • Progress-only required completion output is blocked, while normal successful worker output remains successful.
  • No config, migration, dependency, package, or runtime install changes are included.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 30, 2026
@clawsweeper

clawsweeper Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 7:51 AM ET / 11:51 UTC.

Summary
The PR routes background-task completion wakes through the owning session and reclassifies progress-only required subagent handoffs as blocked.

PR surface: Source +26, Tests +92. Total +118 across 5 files.

Reproducibility: yes. at source level: current main queues background-task events on the owner session while isolated heartbeat runs can execute under a separate :heartbeat session, and the PR's new classifier gap is visible from the source. I did not establish a live Telegram reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Wake Sources Rerouted: 2 sources: background-task and background-task-blocked. These wake classes bypass heartbeat isolation in the PR, so maintainers should notice the targeted session-routing change before merge.

Stored data model
Persistent data-model change detected: serialized state: src/infra/heartbeat-runner.ghost-reminder.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
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:

  • [P1] Fix the required-completion classifier call to pass raw child completion text and add an empty-output regression assertion.
  • [P1] Add redacted Telegram Desktop, gateway-log, terminal/live-output, or linked-artifact proof showing owner-topic delivery with heartbeat isolation enabled.
  • [P1] Refresh on current main after the fix so the final heartbeat merge result is reviewed.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only tests/CI are present and the PR body says live Telegram/Mantis proof is pending; add a redacted Telegram Desktop recording, gateway logs, terminal/live output, or linked artifact, then update the PR body so ClawSweeper re-reviews.

Mantis proof suggestion
A native Telegram Desktop capture would materially show whether the final background-task completion lands in the owning topic/session; the earlier Mantis request has not produced visible proof. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify a background-task completion reaches the owning Telegram topic with heartbeat isolation enabled and ordinary heartbeat wakes remain isolated.

Risk before merge

  • [P1] Real Telegram, gateway log, terminal/live output, or linked-artifact proof is still missing for the user-visible owner-session completion path.
  • [P1] The PR changes which session drains queued completion events, so a mistake can drop, duplicate, or associate final messages with the wrong requester/topic.
  • [P1] The branch is behind current main; after fixing the classifier gap, the final merge result should be checked against the current heartbeat runScope code.

Maintainer options:

  1. Fix Classifier Gap And Require Delivery Proof (recommended)
    Before merge, fix the empty required-completion classifier path and add redacted live Telegram/gateway proof that background-task completions reach the owning topic with heartbeat isolation enabled.
  2. Accept Source-Only Delivery Risk
    Maintainers can intentionally merge with source and regression-test evidence only, but then own the risk that real transport/session delivery still drops or misroutes completion messages.
  3. Pause For Broader Delivery Contract
    If maintainers want one canonical task-completion wake contract across background-task, ACP, exec, and subagent paths, pause this PR and fold the useful tests into that design.

Next step before merge

  • [P1] Contributor or maintainer follow-up is needed because the patch has a concrete classifier fix plus a real Telegram proof gate that automation cannot supply for the contributor's setup.

Security
Cleared: The diff changes runtime and test logic only and does not add dependencies, workflows, permissions, secrets handling, package resolution, or downloaded code.

Review findings

  • [P2] Pass raw completion text into the required-completion classifier — src/agents/subagent-announce.ts:474
Review details

Best possible solution:

Land a refreshed PR that passes raw completion text into the required-completion classifier, preserves the targeted background-task owner-session wake behavior, and includes redacted real Telegram/gateway proof.

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

Yes at source level: current main queues background-task events on the owner session while isolated heartbeat runs can execute under a separate :heartbeat session, and the PR's new classifier gap is visible from the source. I did not establish a live Telegram reproduction in this read-only review.

Is this the best way to solve the issue?

No, not yet. The owner-session wake approach is the narrow right layer, but the required-completion classifier should receive raw child output before the display fallback and the Telegram delivery path still needs real proof.

Full review comments:

  • [P2] Pass raw completion text into the required-completion classifier — src/agents/subagent-announce.ts:474
    The new classifier call receives findings, but findings has already converted an empty child result into the display fallback "(no output)". That means a required completion with no child output still reaches the requester prompt as ok / completed; ready for parent review, even though resolveRequiredCompletionTerminalResult() defines missing required output as blocked. This was visible at the previous reviewed head as well; the earlier review and current head are both 47e4354a287c.
    Confidence: 0.87
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only tests/CI are present and the PR body says live Telegram/Mantis proof is pending; add a redacted Telegram Desktop recording, gateway logs, terminal/live output, or linked artifact, then update the PR body so ClawSweeper re-reviews.
  • remove status: 🛠️ actively grinding: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P1: The PR targets a broken user-visible agent/channel workflow where completed background work may not deliver a final message to the requester.
  • merge-risk: 🚨 message-delivery: The diff changes which session drains and sends background-task completion wakes, so mistakes can drop or misroute final messages.
  • merge-risk: 🚨 session-state: Bypassing isolatedSession for this wake class changes which session receives the model run and consumes queued owner-session events.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only tests/CI are present and the PR body says live Telegram/Mantis proof is pending; add a redacted Telegram Desktop recording, gateway logs, terminal/live output, or linked artifact, then update the PR body so ClawSweeper re-reviews.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The PR changes visible Telegram topic/session completion delivery, which is suitable for a short Telegram Desktop proof.
Evidence reviewed

PR surface:

Source +26, Tests +92. Total +118 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 43 17 +26
Tests 2 92 0 +92
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 135 17 +118

What I checked:

  • Repository policy read and applied: Root AGENTS.md and src/agents/AGENTS.md were read fully; their review-depth, session/message-delivery, and real Telegram proof guidance apply to this PR. (AGENTS.md:1, 58891c85b68b)
  • Current main queues terminal background-task events on the owner session: Task terminal delivery enqueues background-task and background-task-blocked events for the resolved owner session key and requests an immediate heartbeat for that owner key. (src/tasks/task-registry.ts:1356, 58891c85b68b)
  • Current main still isolates those wakes: Heartbeat wake classification on current main does not include background-task sources, and isolatedSession is applied unconditionally before the PR changes it. (src/infra/heartbeat-runner.ts:1035, 58891c85b68b)
  • PR implements targeted owner-session delivery for background-task wakes: The PR marks background-task/background-task-blocked as wake payloads, inspects those pending owner-session events, and disables isolatedSession only for that wake class. (src/infra/heartbeat-runner.ts:1062, 47e4354a287c)
  • Classifier gap in PR head: The PR calls resolveRequiredCompletionTerminalResult after replacing empty child output with the display fallback '(no output)', while the shared classifier only blocks missing required completion when it receives an empty value. (src/agents/subagent-announce.ts:471, 47e4354a287c)
  • Proof remains source-only: Live PR metadata shows tests and CI evidence plus an explicit PR-body proof gap for live Telegram/Mantis delivery; the visible Mantis proof check was skipped or cancelled and no artifact was present in the discussion. (47e4354a287c)

Likely related people:

  • steipete: Recent GitHub path history shows repeated heartbeat and subagent delivery work, including UTF-16 heartbeat cleanup and subagent delivery state changes adjacent to this PR's session/message path. (role: adjacent owner; confidence: high; commits: a9582a1bb62a, 589b1f6aec69, 465a5456fe0f; files: src/infra/heartbeat-runner.ts, src/agents/subagent-announce.ts, src/agents/subagent-registry-lifecycle.ts)
  • vincentkoc: Recent task/subagent history includes silent subagent completion handoff work and task registry refactors around the same required-completion and terminal-delivery area. (role: recent task and subagent completion contributor; confidence: high; commits: 68a1e00b73bd, 273eed4c51cb, 745b01163276; files: src/agents/subagent-announce.ts, src/tasks/task-registry.ts, src/tasks/task-completion-contract.ts)
  • IWhatsskill: The shared required-completion progress-only and empty-output classifier used by this PR was introduced in the task completion contract history. (role: completion classifier introducer; confidence: high; commits: 36e76ef42409; files: src/tasks/task-completion-contract.ts, src/agents/subagent-registry-completion.ts)
  • agocs: Recent heartbeat path history shows delivery cleanup around pending final delivery and successful heartbeat send behavior adjacent to owner-session completion delivery. (role: recent heartbeat delivery contributor; confidence: medium; commits: 305fa9c4ddbe; files: src/infra/heartbeat-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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-30T01:42:37.277Z sha 47e4354 :: needs real behavior proof before merge. :: none

@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. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 30, 2026
@moeedahmed
moeedahmed force-pushed the codex/upstream-background-task-heartbeat-delivery branch from 400666a to 47e4354 Compare June 30, 2026 01:28
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 30, 2026
@moeedahmed ChatGPT Codex Connector

moeedahmed commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@openclaw-mantis telegram desktop proof: verify a background-task completion reaches the owning Telegram topic with heartbeat isolation enabled and ordinary heartbeat wakes remain isolated.

@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 2, 2026
@samiralibabic samiralibabic mentioned this pull request Jul 7, 2026
Open
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jul 8, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 23, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 23, 2026
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. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@moeedahmed