Skip to content

fix(cron): publish agent response to outbound bus for cron-triggered jobs#2100

Merged
yinwm merged 5 commits intosipeed:mainfrom
ShenQingchuan:fix/cron-agent-outbound
Mar 29, 2026
Merged

fix(cron): publish agent response to outbound bus for cron-triggered jobs#2100
yinwm merged 5 commits intosipeed:mainfrom
ShenQingchuan:fix/cron-agent-outbound

Conversation

@ShenQingchuan
Copy link
Copy Markdown
Contributor

Problem

When a cron job triggers agent execution via ProcessDirectWithChannel, the agent's text response is silently discarded. The original code assumed AgentLoop would auto-publish the response:

// Response is automatically sent via MessageBus by AgentLoop
_ = response // Will be sent by AgentLoop

This assumption is incorrect — processMessage sets SendResponse: false, so runAgentLoop never publishes the final content to the outbound bus. As a result, users on channels like Telegram never receive the agent's response for cron-triggered tasks.

The command branch and the deliver=true branch of ExecuteJob both correctly call PublishOutbound, but the agent branch does not.

Fix

  • Export AgentLoop.PublishResponseIfNeeded (previously unexported publishResponseIfNeeded) and add it to the JobExecutor interface.
  • In CronTool.ExecuteJob, call PublishResponseIfNeeded after the agent returns a non-empty response.

PublishResponseIfNeeded checks MessageTool.HasSentInRound() before publishing — if the agent already delivered content through the message tool during this round, the final status response (e.g. "已发送。") is correctly skipped, preventing duplicate messages.

Additional: directive message type

Adds a Type field ("message" | "directive") to CronPayload:

  • "message" (default): static content, sent as-is when deliver=true.
  • "directive": content is treated as instructions for the agent to execute. Forces agent processing even when deliver=true.

This enables cron jobs like "check the weather and report" where the agent needs to perform work rather than echo a fixed string.

Changed files

File Change
pkg/agent/loop.go Export PublishResponseIfNeeded (rename only)
pkg/cron/service.go Add Type field to CronPayload
pkg/tools/cron.go Add PublishResponseIfNeeded to JobExecutor interface; call it in ExecuteJob; add type parameter and directive handling
pkg/tools/cron_test.go Add tests for agent response publishing, empty response skip, and message-tool-already-sent skip

Test plan

  • go test ./pkg/tools -run TestCronTool_ — all 13 tests pass
  • go build ./... — compiles cleanly
  • Manual verification: cron → agent → Telegram delivery works; no duplicate "已发送。" when agent uses message tool

…jobs

When a cron job triggers agent execution via ProcessDirectWithChannel,
the agent response was silently discarded — the code assumed AgentLoop
would auto-publish it, but SendResponse is false on this path.

Delegate to PublishResponseIfNeeded (exported from AgentLoop) so the
response reaches the originating channel (e.g. Telegram) only when the
message tool did not already deliver content in the same round.

Also adds a "directive" message type to CronPayload, allowing cron jobs
to instruct the agent to execute a task rather than echo static text.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 27, 2026

CLA assistant check
All committers have signed the CLA.

@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: corn domain: tool go Pull requests that update go code labels Mar 27, 2026
Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this well-structured fix — this is the cleanest approach among the five PRs addressing the same cron response bug. The PublishResponseIfNeeded delegation with the HasSentInRound() guard is the right pattern to prevent duplicate messages. The stubJobExecutor test design is also solid.

Two blocking issues before we can merge:

1. type parameter lacks server-side validation

The enum in Parameters() is only an LLM schema hint — any string value gets persisted. Please add a whitelist check in addJob:

msgType, _ := args["type"].(string)
if msgType != "" && msgType != "message" && msgType != "directive" {
    return ErrorResult(fmt.Sprintf("invalid type %q, must be 'message' or 'directive'", msgType))
}

2. Missing tests for the new directive feature

The core branch logic in ExecuteJob changed (deliver=true && !isDirective), but no test covers the directive path. Please add at least:

  • Test that type=directive adds the prompt prefix to ProcessDirectWithChannel
  • Test that deliver=true + type=directive routes through the agent (not direct publish)
  • Test that the directive prompt content is passed correctly

Non-blocking suggestions (happy to see these in a follow-up):

  • Merge the two UpdateJob calls in addJob into one to avoid redundant I/O
  • Consider whether PublishResponseIfNeeded should be internal to ProcessDirectWithChannel rather than exposed on the interface — the caller shouldn't need to know about the message-tool dedup internals
  • The omitempty on CronPayload.Type creates asymmetric JSON output (empty string omitted, "message" serialized) — consider a consistent default

One note: PR #1839 raises an additional concern about Peer not being set on the inbound message in ProcessDirectWithChannel, which causes peer-based bindings to never match for cron jobs. That's a separate bug worth tracking.

Looking forward to v2!

Address reviewer blocking feedback:

1. Server-side whitelist for `type` parameter — the `enum` in
   Parameters() is only an LLM schema hint; any string was persisted.
   Now `addJob` rejects values other than "message" and "directive".

2. Comprehensive test coverage for the directive code path:
   - directive adds prompt prefix to ProcessDirectWithChannel
   - deliver=true + directive routes through agent (not direct publish)
   - directive prompt content, sessionKey, channel, chatID are correct
   - invalid type is rejected; valid types ("", "message", "directive") pass
   - deliver=true message type goes directly to bus (regression)
   - agent error path does not trigger publish (regression)

Also merge the two UpdateJob calls in addJob into one to avoid
redundant disk I/O (non-blocking suggestion from review).
@ShenQingchuan
Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! Both blocking issues are addressed in the latest commit.

Changes in v2

1. Server-side validation for type parameter

Added a whitelist check in addJob before any further processing — values other than "message" and "directive" are now rejected with a clear error. This runs regardless of command or deliver state (no order dependency).

2. Directive test coverage

Added 7 new tests covering the full ExecuteJob path matrix:

Test Scenario
ExecuteJobDirectiveAddsPromptPrefix type=directive prepends the instruction prefix
ExecuteJobDirectiveWithDeliverRoutesToAgent deliver=true + directive bypasses direct publish, routes through agent
ExecuteJobDirectivePassesCorrectContent Full verification of prompt, sessionKey, channel, chatID
ExecuteJobDeliverMessageDirectlyToBus Regression: deliver=true message type goes straight to bus
ExecuteJobReturnsErrorWithoutPublish Regression: agent error does not trigger publish
AddJobRejectsInvalidType Invalid type string is rejected
AddJobAcceptsValidTypes Subtests for "", "message", "directive"

3. Merged redundant UpdateJob calls (non-blocking suggestion)

The two sequential UpdateJob calls for command and type are now consolidated into a single call, eliminating an intermediate persist with partial state.


Regarding the other non-blocking suggestions — happy to address in a follow-up:

  • PublishResponseIfNeeded encapsulation: Agree that ideally ProcessDirectWithChannel should own the publish decision internally. The current export is a pragmatic minimal fix; internalizing it would change the method's contract for all callers, so it warrants a separate PR.
  • omitempty on CronPayload.Type: Noted — will consider normalizing to always serialize the field for consistency.
  • PR fix(cron): route cron jobs to correct agent and publish response to channel #1839 Peer issue: Tracked separately — the missing Peer on inbound messages in ProcessDirectWithChannel is an orthogonal bug.

Empty string and "message" are semantically equivalent defaults;
always serializing the field avoids asymmetric JSON output.
@ShenQingchuan ShenQingchuan force-pushed the fix/cron-agent-outbound branch from 15d1c0f to ea2a3f7 Compare March 28, 2026 16:23
@ShenQingchuan
Copy link
Copy Markdown
Contributor Author

Small follow-up: also removed the omitempty tag from CronPayload.Type per your suggestion, so the JSON output is now consistent — empty string is serialized as "type":"" rather than being omitted entirely.

Also dropped the inline comment on that field (// "message" (default) or "directive") to stay consistent with the rest of the structs in this file, which use no trailing comments.

@ShenQingchuan ShenQingchuan requested a review from yinwm March 28, 2026 16:24
- Remove ExecuteJobDirectivePassesCorrectContent: its assertions on
  sessionKey/channel/chatID duplicate ExecuteJobPublishesAgentResponse;
  its prompt check duplicates DirectiveAddsPromptPrefix.
- Strengthen DirectiveAddsPromptPrefix with exact prompt match and
  publish response assertion.
- Fix ReturnsErrorWithoutPublish: set non-empty stub response so the
  test verifies the error branch early-return, not the response==""
  guard.
@ShenQingchuan
Copy link
Copy Markdown
Contributor Author

ShenQingchuan commented Mar 28, 2026

One more follow-up: reviewed the new tests for redundancy and made a cleanup pass (5d803d3).

  • Removed ExecuteJobDirectivePassesCorrectContent — its sessionKey/channel/chatID assertions duplicated ExecuteJobPublishesAgentResponse, and its prompt check duplicated DirectiveAddsPromptPrefix. Coverage stayed identical at 69.2% for ExecuteJob after removal, confirming it exercised no unique code path.
  • Strengthened DirectiveAddsPromptPrefix — upgraded from Contains to exact match on the full prompt string, and added a publish response assertion (absorbing the useful parts of the removed test).
  • Fixed ReturnsErrorWithoutPublish — the stub now returns a non-empty response alongside the error, so the test actually verifies the error branch early-return rather than silently passing due to the response != "" guard.

Net result: 7 tests → 6 tests, same coverage, stronger assertions.

Coverage progression across revisions:

Function main (before PR) v1 (b682610) v2 (a58d15c) v2 trimmed (5d803d3)
addJob 0% 77.1% 83.0% 83.0%
ExecuteJob 0% 53.8% 69.2% 69.2%
Tests 0 13 20 19

Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All clear — linter fixes look good, CI is green across the board (tests, linter, security check). No remaining blocking issues. Ready to merge. Please close the duplicate PRs (#1839, #1766, #757).

@yinwm yinwm merged commit e414b82 into sipeed:main Mar 29, 2026
4 checks passed
yinwm added a commit to yinwm/picoclaw that referenced this pull request Mar 29, 2026
… path

The agent path now publishes to outbound bus directly (since sipeed#2100),
making the deliver=true direct-to-bus shortcut and the directive type
prompt wrapping redundant. All cron jobs now uniformly route through
the agent. This is an intentional behavior change: old jobs with
deliver=true will execute through the agent instead of bypassing it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ShenQingchuan ShenQingchuan deleted the fix/cron-agent-outbound branch March 29, 2026 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: corn domain: tool go Pull requests that update go code type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants