Skip to content

fix(cron): no-deliver runs now recover from tool warnings via final assistant text#90676

Open
openperf wants to merge 1 commit into
openclaw:mainfrom
openperf:fix/90664-no-channel-prefers-final-text
Open

fix(cron): no-deliver runs now recover from tool warnings via final assistant text#90676
openperf wants to merge 1 commit into
openclaw:mainfrom
openperf:fix/90664-no-channel-prefers-final-text

Conversation

@openperf

@openperf openperf commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Problem: --no-deliver cron runs that produce only non-fatal tool warnings followed by a valid final assistant reply are incorrectly recorded as fatal errors. The run ledger shows an error payload and hasFatalErrorPayload = true even though the agent recovered cleanly.
  • Root Cause: resolveCronChannelOutputPolicy returns { preferFinalAssistantVisibleText: false } when channel is undefined — which is exactly the value run.ts passes when deliveryPlan.mode === "none" sets resolvedDelivery.channel = undefined. Both downstream recovery gates in resolveCronPayloadOutcome require preferFinalAssistantVisibleText === true: hasRecoveredToolWarning (checks all error payloads are tool-warning strings plus a non-empty final text) and shouldUseFinalAssistantVisibleText. With the flag false, hasRecoveredToolWarning is always false, hasFatalStructuredErrorPayload includes the tool warnings as fatal, and the run is marked status: "error" regardless of whether the agent recovered.
  • Fix: Return { preferFinalAssistantVisibleText: true } when no channelId is resolved. Without a channel there is no structured delivery target; the final assistant text is the only meaningful output, so preferring it is unconditionally correct for the no-channel path. The downstream guards (!runLevelError, failureSignal?.fatalForCron !== true, !hasStructuredDeliveryPayloads) remain intact and prevent false recovery for genuine errors.
  • What changed:
    • src/cron/isolated-agent/channel-output-policy.ts — flip the no-channel early-exit from false to true; add a one-line comment explaining the semantic reason.
    • src/cron/isolated-agent/channel-output-policy.test.ts — add "prefers final assistant text when no channel is resolved" test case; asserts the channel plugin runtime is not consulted (function returns before the lazy import).
  • What did NOT change (scope boundary):
    • The has-channel path is unchanged: channel plugins that set preferFinalAssistantVisibleText: true or omit it still resolve exactly as before.
    • delivery-dispatch.ts dispatch is gated on params.deliveryRequested && …; since deliveryRequested: false in the no-deliver case, deliverViaDirect is never called even though resolvedDeliveryPayloads now contains the final text. No actual channel message is sent.
    • Config surface unchanged (no schema, defaults, doctor migrations, or docs/reference/config).
    • Plugin surface unchanged (no plugin SDK, manifest, extensions/* api/runtime-api, registry/loader).
    • Gateway protocol unchanged.

Reproduction

  1. Configure a cron job with --no-deliver (or deliveryPlan.mode = "none" with no explicit target).
  2. Run an agent that emits one or more ⚠️ 🛠️ … tool-warning payloads and then produces a non-empty final assistant reply.
  3. Inspect hasFatalErrorPayload / status from resolveCronPayloadOutcome.
  4. Before this PR: preferFinalAssistantVisibleText is false; hasRecoveredToolWarning is always false; hasFatalStructuredErrorPayload is true; run is recorded as status: "error" with the tool-warning text as embeddedRunError.
  5. After this PR: preferFinalAssistantVisibleText is true; hasRecoveredToolWarning is true; hasFatalStructuredErrorPayload is false; run is recorded as status: "ok" with the final assistant text as outputText.

Real behavior proof

Behavior addressed (#90664): resolveCronChannelOutputPolicy no longer returns false for the no-channel/no-deliver path; hasRecoveredToolWarning can now be satisfied; a no-deliver run that produces only non-fatal tool warnings followed by a final assistant reply is recorded as ok with the assistant text, not as a fatal error.

Real environment tested (Linux, Node 22 — Vitest against the production resolveCronChannelOutputPolicy, resolveCronPayloadOutcome, and the adjacent message-tool-policy helpers): the new test drives resolveCronChannelOutputPolicy(undefined) directly and asserts that the channel plugin runtime lazy import is never triggered; the existing helpers suite confirms that preferFinalAssistantVisibleText: true produces recovery as expected.

Exact steps or command run after this patch: node scripts/run-vitest.mjs src/cron/isolated-agent/channel-output-policy.test.ts src/cron/isolated-agent.helpers.test.ts src/cron/isolated-agent/run.message-tool-policy.test.ts

Evidence after fix (Vitest output for the three touched suites):

channel-output-policy.test.ts   Tests  3 passed (3)
isolated-agent.helpers.test.ts  Tests  60 passed (60)
run.message-tool-policy.test.ts Tests  7 passed (7)

Observed result after fix: resolveCronChannelOutputPolicy(undefined) resolves to { preferFinalAssistantVisibleText: true } without consulting the channel plugin runtime. The existing "lets preferred final assistant text recover a plain tool warning" case in the helpers suite (which passes preferFinalAssistantVisibleText: true explicitly) confirms the downstream recovery path works correctly once the flag is true.

What was not tested: a live gateway cron run with --no-deliver plus a tool-warning-emitting agent. The policy function, the recovery gate logic, and the helpers are covered deterministically against the production functions; a full gateway round-trip was not driven.

Repro confirmation: the new "prefers final assistant text when no channel is resolved" test case fails on origin/main before the patch (returns false) and passes after; the mock assertion that the channel plugin runtime is never called confirms the early-exit path is exercised, not the lazy-load path.

Risk / Mitigation

  • Risk: A no-deliver run with only non-tool-warning error payloads could be misclassified as recovered. Mitigation: hasRecoveredToolWarning requires errorPayloads.every(payload => isCronToolWarning(payload?.text)); any non-tool-warning error payload keeps the run fatal regardless of the flag.
  • Risk: resolvedDeliveryPayloads now contains the final text in no-deliver mode, which could trigger unexpected delivery. Mitigation: dispatchCronDelivery is gated on params.deliveryRequested && …; deliveryRequested is false in the no-deliver case, so deliverViaDirect is never called.

Change Type (select all)

  • Bug fix

Scope (select all touched areas)

  • cron

Linked Issue/PR

Fixes #90664

@openperf
fix(cron): no-deliver cron runs prefer final assistant text for recovery
b83756d 
When deliveryPlan.mode is "none" and no channel is resolved,
resolveCronChannelOutputPolicy returned preferFinalAssistantVisibleText:
false.  That blocked hasRecoveredToolWarning and shouldUseFinalAssistantVisibleText
in resolveCronPayloadOutcome (both gates require true), so a cron run that
produced valid final assistant text after only non-fatal tool warnings was
still marked as a fatal error.

With no channel there is no structured delivery target; the final assistant
text is the only meaningful output.  Returning true lets the recovery path
work correctly for --no-deliver runs.

Fixes openclaw#90664
@clawsweeper

clawsweeper Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 5, 2026, 9:56 AM ET / 13:56 UTC.

Summary
The PR changes cron channel output policy so unresolved/no-channel isolated cron runs prefer final assistant visible text and adds a regression test for the undefined-channel path.

PR surface: Source +1, Tests +7. Total +8 across 2 files.

Reproducibility: yes. from source inspection: bare no-deliver sets the resolved channel to undefined, current main returns the false policy for that value, and the recovery gate requires the flag to be true. I did not run a live cron job in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
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] No live gateway cron run was provided for --no-deliver plus a real tool-warning-emitting agent; merge confidence relies on source-path tracing and focused Vitest output rather than an end-to-end run.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the narrow policy and regression-test fix after normal maintainer CI or requested live cron proof, leaving channel-specific output policy unchanged.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is needed because the PR already contains the narrow code and test change; maintainers should review/land it or request live cron proof.

Security
Cleared: The diff only changes cron policy/test logic and does not touch dependencies, workflows, credentials, install scripts, or other supply-chain surfaces.

Review details

Best possible solution:

Merge the narrow policy and regression-test fix after normal maintainer CI or requested live cron proof, leaving channel-specific output policy unchanged.

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

Yes from source inspection: bare no-deliver sets the resolved channel to undefined, current main returns the false policy for that value, and the recovery gate requires the flag to be true. I did not run a live cron job in this read-only review.

Is this the best way to solve the issue?

Yes. The policy helper is the narrow shared callee for both runtime callers, and the existing helper guards still reject run-level errors, fatal failure signals, non-tool warnings, and structured delivery payloads; plumbing delivery mode through the outcome path would be broader without clear benefit.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority cron status/output classification bug limited to no-channel tool-warning recovery paths.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR author association is MEMBER, so the external-contributor real behavior proof gate is not applicable; the body reports focused Vitest proof but not a live cron run.
Evidence reviewed

PR surface:

Source +1, Tests +7. Total +8 across 2 files.

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

What I checked:

  • Repository policy read: Read the full root AGENTS.md and found no scoped AGENTS.md under src/cron, so the root review-depth, cron/core, and maintainer-authored-item rules apply. (AGENTS.md:1, 520992a1defd)
  • Current main still has the no-channel false policy: On current main, resolveCronChannelOutputPolicy(undefined) normalizes to no channel and returns preferFinalAssistantVisibleText: false, so the central bug is not already implemented on main. (src/cron/isolated-agent/channel-output-policy.ts:20, 520992a1defd)
  • No-deliver runtime path passes an undefined channel: The bare delivery.mode === "none" path builds resolvedDelivery.channel: undefined and deliveryRequested: false, which is the path this PR changes via the shared policy helper. (src/cron/isolated-agent/run.ts:382, 520992a1defd)
  • Outcome recovery is gated by the policy flag: hasRecoveredToolWarning requires preferFinalAssistantVisibleText === true, final assistant text, no structured delivery payloads, and all error payloads matching cron tool warnings before suppressing fatal structured-error status. (src/cron/isolated-agent/helpers.ts:289, 520992a1defd)
  • No-deliver does not trigger direct delivery: dispatchCronDelivery only enters direct fallback delivery when params.deliveryRequested is true, matching the PR body's claim that no runner send is introduced for bare no-deliver runs. (src/cron/isolated-agent/delivery-dispatch.ts:1219, 520992a1defd)
  • PR patch is narrow and test-backed: The PR head flips only the no-channel early return to true and adds a test that resolveCronChannelOutputPolicy(undefined) returns true without consulting channel plugin runtime. (src/cron/isolated-agent/channel-output-policy.ts:20, b83756ddd7b9)

Likely related people:

  • Peter Steinberger: Git blame shows the current channel-output-policy helper, cron outcome gate, and no-deliver resolved-delivery path in this checkout coming from 9fd5f9e. (role: introduced current behavior; confidence: medium; commits: 9fd5f9ee7ca0; files: src/cron/isolated-agent/channel-output-policy.ts, src/cron/isolated-agent/helpers.ts, src/cron/isolated-agent/run.ts)
  • Ayaan Zaidi: Recent cron history shows focused work on no-deliver/message-tool behavior in the same runtime surface, including keeping the message tool available without runner delivery. (role: recent adjacent owner; confidence: medium; commits: 49ae60d6cae1, 13a0d7a9e035; files: src/cron/isolated-agent/run.ts, src/cron/isolated-agent/run.message-tool-policy.test.ts)
  • welfo-beo: Recent helper history shows this contributor authored the cron final announce delivery fix that expanded resolveCronPayloadOutcome around final assistant text behavior. (role: adjacent feature contributor; confidence: medium; commits: 81c7304a18b8; files: src/cron/isolated-agent/helpers.ts, src/cron/isolated-agent.helpers.test.ts, src/cron/isolated-agent/run.ts)
  • @jalehman: The same cron final announce delivery commit records @jalehman as reviewer, making them a useful routing candidate for final-text recovery semantics. (role: reviewer; confidence: low; commits: 81c7304a18b8; files: src/cron/isolated-agent/helpers.ts, src/cron/isolated-agent.helpers.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: 🐚 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. P2 Normal backlog priority with limited blast radius. labels Jun 5, 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

P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

1 participant

@openperf