Skip to content

fix(cron): preflight implicit announce targets#78644

Merged
sallyom merged 1 commit intoopenclaw:mainfrom
sallyom:fix/issue-78608
May 7, 2026
Merged

fix(cron): preflight implicit announce targets#78644
sallyom merged 1 commit intoopenclaw:mainfrom
sallyom:fix/issue-78608

Conversation

@sallyom
Copy link
Copy Markdown
Contributor

@sallyom sallyom commented May 6, 2026

Summary

  • Problem: isolated cron agentTurn jobs can default to announce delivery with implicit delivery.channel=last even when there is no previous route.
  • Why it matters: the job can spend model tokens before failing with a permanent delivery-target error, then repeat on later cron fires.
  • What changed: the isolated runner now fails unresolved implicit announce delivery before calling the agent executor, while preserving the existing delivery-target error classification and delivery trace.
  • What did NOT change (scope boundary): explicit delivery targets still run through the existing execution and delivery-dispatch path; add/update rejection and auto-disable policy are left for separate maintainer direction.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause

  • Root cause: isolated cron delivery target resolution happened before execution, but runCronIsolatedAgentTurn still called executeCronRun even when implicit announce delivery had already resolved to a permanent last route failure.
  • Missing detection / guardrail: no pre-execution guard for the known-unroutable implicit announce target shape.
  • Contributing context: delivery dispatch already reports errorKind: "delivery-target", but only after the expensive agent turn.

Regression Test Plan

  • Coverage level that should have caught this:
    • Seam / integration test
  • Target test or file: src/cron/isolated-agent/run.message-tool-policy.test.ts
  • Scenario the test should lock in: implicit announce delivery with channel: "last" and no resolved route returns a delivery-target error without starting the agent executor, dispatching delivery, or firing onExecutionStarted.
  • Why this is the smallest reliable guardrail: it exercises the isolated runner boundary where delivery resolution and model execution meet.

User-visible / Behavior Changes

Recurring isolated cron announce jobs that have no implicit previous delivery route now fail before model execution instead of spending tokens and failing after delivery dispatch.

Diagram

Before:
cron fire -> resolve last route failure -> run model -> delivery-target error

After:
cron fire -> resolve last route failure -> delivery-target error

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

Repro + Verification

Environment

  • OS: macOS local maintainer worktree
  • Runtime/container: Node/pnpm repo test wrapper
  • Model/provider: mocked test harness
  • Integration/channel: cron isolated agent delivery
  • Relevant config: test fixtures only

Steps

  1. Run the focused cron regression suite.
  2. Run changed-surface checks locally.
  3. Attempt remote Crabbox/Blacksmith changed proof.

Expected

  • Unresolved implicit announce delivery fails before model execution.
  • Explicit delivery-target failure behavior continues through existing post-run dispatch tests.

Actual

  • Focused cron tests pass.
  • Local check:changed passed before rebase.
  • Remote Crabbox/Blacksmith proof was not available because Blacksmith was down.
  • Local test:changed hit unrelated workspace package-resolution failures in broader cron direct-delivery tests (openclaw/workspace package resolution), not assertions from this patch.

Evidence

  • Failing test/log before + passing after

Commands run:

pnpm test src/cron/isolated-agent/run.message-tool-policy.test.ts
pnpm exec oxfmt --check --threads=1 src/cron/isolated-agent/run.ts src/cron/isolated-agent/run.message-tool-policy.test.ts
pnpm check:changed
pnpm test:changed
pnpm test src/cron/isolated-agent/run.message-tool-policy.test.ts src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/cron/delivery-preview.test.ts
git diff --check origin/main...HEAD

Latest post-rebase focused proof:

Test Files  3 passed (3)
Tests  82 passed (82)

Human Verification

  • Verified scenarios: implicit unresolved announce delivery fails before agent execution; existing explicit unresolved delivery behavior still has test coverage; delivery preview and dispatch suites still pass.
  • Edge cases checked: explicit bad target test remains on the existing execution path; bare delivery.mode="none" behavior remains covered by existing tests.
  • What I did not verify: live cron execution against a real configured channel, due to Blacksmith outage and this being covered by the focused runner seam.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

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

Risks and Mitigations

  • Risk: a job with an explicit but invalid target still spends tokens before dispatch reports the delivery-target error.
    • Mitigation: this PR intentionally limits scope to the issue's implicit last footgun; explicit-target validation policy can be handled separately.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 6, 2026
@sallyom sallyom added the proof: override Maintainer override for the external PR real behavior proof gate. label May 6, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 6, 2026

Codex review: needs maintainer review before merge.

Summary
The PR adds an early delivery-target failure before isolated cron execution for unresolved implicit announce routes, updates message-tool policy tests, and adds a changelog entry.

Reproducibility: yes. source-reproducible. Current main resolves the delivery target during preparation and preview can already report the fail-closed last route, but runCronIsolatedAgentTurn still calls executeCronRun before delivery dispatch returns the delivery-target error.

Real behavior proof
Override: The PR carries proof: override, so the external contributor real-behavior proof gate is waived for this review.

Next step before merge
This is a protected-label PR with no actionable repair finding, so the remaining action is maintainer review plus required CI rather than a ClawSweeper repair job.

Security
Cleared: The diff only changes cron runtime control flow, focused tests, and changelog text, with no dependency, workflow, permission, secret, network, or package-resolution surface.

Review details

Best possible solution:

Land the narrow runtime preflight after maintainer approval and required CI, leaving add/update rejection and auto-disable policy to separate follow-up decisions.

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

Yes, source-reproducible. Current main resolves the delivery target during preparation and preview can already report the fail-closed last route, but runCronIsolatedAgentTurn still calls executeCronRun before delivery dispatch returns the delivery-target error.

Is this the best way to solve the issue?

Yes. The PR takes the narrowest maintainable path by failing only unresolved implicit announce delivery before model execution while preserving explicit bad targets on the existing execution and dispatch path.

Acceptance criteria:

  • pnpm test src/cron/isolated-agent/run.message-tool-policy.test.ts src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/cron/delivery-preview.test.ts
  • pnpm check:changed

What I checked:

  • Current main defaults isolated agent-turn delivery to announce: resolveInitialCronDelivery returns { mode: "announce" } for isolated agentTurn jobs without explicit delivery, creating the implicit announce route involved in the linked bug. (src/cron/service/initial-delivery.ts:7, 372e270871a2)
  • Current main can identify the unroutable route before execution: Delivery preview formats unresolved implicit last routes as last -> no route, will fail-closed, showing this target failure is knowable before the model run. (src/cron/delivery-preview.ts:24, 372e270871a2)
  • Current main still runs the agent first: prepareCronRunContext resolves delivery context before execution, but runCronIsolatedAgentTurn proceeds to load and call executeCronRun before final delivery dispatch handles the unresolved target. (src/cron/isolated-agent/run.ts:1064, 372e270871a2)
  • Existing error classification is preserved: dispatchCronDelivery already returns unresolved delivery as status: "error" with errorKind: "delivery-target", which is the classification the PR keeps for the early return. (src/cron/isolated-agent/delivery-dispatch.ts:505, 372e270871a2)
  • PR head adds the narrow pre-execution guard: The PR adds shouldFailImplicitAnnounceDeliveryBeforeExecution and returns a delivery-target error before executeCronRun only when announce delivery was requested, no explicit target exists, and target resolution failed. (src/cron/isolated-agent/run.ts:328, 111d11c16d2e)
  • PR head covers implicit and explicit paths: The new regression test asserts unresolved implicit last delivery does not call the agent executor, delivery dispatch, or onExecutionStarted, while the adjacent explicit bad-target case remains on the existing post-run path. (src/cron/isolated-agent/run.message-tool-policy.test.ts:740, 111d11c16d2e)

Likely related people:

  • steipete: Feature history ties the isolated delivery default, centralized initial delivery defaults, and delivery tool-policy helper extraction to recent cron commits by Peter Steinberger; current-line blame also points central cron delivery lines to his recent main commit. (role: introduced behavior and recent cron maintainer; confidence: high; commits: 4e07bdbdfd1b, 6b18ec479c0f, 9b99787c3197; files: src/cron/service/initial-delivery.ts, src/cron/isolated-agent/run.ts, src/cron/isolated-agent/run.message-tool-policy.test.ts)
  • Anandesh Sharma: Authored a nearby cron delivery policy fix in run.ts that conditions message-tool targeting on resolved delivery, directly adjacent to the unresolved-target behavior under review. (role: adjacent delivery-policy contributor; confidence: medium; commits: 28519263145a; files: src/cron/isolated-agent/run.ts)
  • vincentkoc: Local current-line history for central cron delivery files includes Vincent Koc's recent main commit, but the commit title is adjacent infrastructure rather than the stronger feature-origin trail. (role: current-line history candidate; confidence: low; commits: aa9247e0cef3; files: src/cron/isolated-agent/run.ts, src/cron/delivery-preview.ts, src/cron/service/initial-delivery.ts)

Remaining risk / open question:

  • I did not run tests in this read-only review; merge should still wait for required CI or maintainer-run validation.

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

@sallyom sallyom force-pushed the fix/issue-78608 branch from f7c31a8 to 39bc0d7 Compare May 6, 2026 22:46
Signed-off-by: sallyom <somalley@redhat.com>
@sallyom sallyom force-pushed the fix/issue-78608 branch from 39bc0d7 to 111d11c Compare May 7, 2026 00:07
@sallyom sallyom merged commit 20c34b8 into openclaw:main May 7, 2026
166 of 170 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026
Signed-off-by: sallyom <somalley@redhat.com>
rogerdigital pushed a commit to rogerdigital/openclaw that referenced this pull request May 9, 2026
Signed-off-by: sallyom <somalley@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer Maintainer-authored PR proof: override Maintainer override for the external PR real behavior proof gate. size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent isolated cron announce jobs from repeatedly running when implicit last delivery has no route

1 participant