Skip to content

refactor(cron): remove deliver and type params, unify agent execution path#2147

Open
yinwm wants to merge 1 commit intosipeed:mainfrom
yinwm:fix/remove-cron-deliver-param
Open

refactor(cron): remove deliver and type params, unify agent execution path#2147
yinwm wants to merge 1 commit intosipeed:mainfrom
yinwm:fix/remove-cron-deliver-param

Conversation

@yinwm
Copy link
Copy Markdown
Collaborator

@yinwm yinwm commented Mar 29, 2026

Summary

Breaking change

Old persisted jobs with deliver: true or type: "directive" will silently ignore these fields (Go json.Unmarshal behavior). All jobs now execute through the agent instead of bypassing it. This is intentional.

Test plan

  • make test passes (all packages ok)
  • Verify existing cron jobs execute correctly through agent path
  • Confirm agent responses are published to outbound bus as expected

🤖 Generated with Claude Code

… 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>
Copilot AI review requested due to automatic review settings March 29, 2026 07:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Removes the legacy deliver and type parameters from the cron system and routes non-command cron jobs through the agent execution path, aligning cron behavior with the post-#2100 outbound publishing flow.

Changes:

  • Remove deliver/type fields from cron payloads and tool schema; remove directive/direct-delivery execution branches.
  • Update cron job creation APIs/signatures and CLI to drop deliver.
  • Delete related tests that asserted deliver/type behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/tools/cron.go Removes deliver/type parameters and simplifies job execution to always call the agent for non-command jobs.
pkg/tools/cron_test.go Removes tests for deliver/type/directive behaviors; retains agent-response publishing coverage.
pkg/cron/service.go Removes persisted payload fields and updates AddJob signature accordingly.
pkg/cron/service_test.go Updates tests to match new AddJob signature.
cmd/picoclaw/internal/cron/add.go Removes --deliver flag and updates job creation call.
cmd/picoclaw/internal/cron/add_test.go Updates CLI flag assertions to match removed --deliver.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +347 to 353
// Call agent with the job message
response, err := t.executor.ProcessDirectWithChannel(
ctx,
prompt,
job.Payload.Message,
sessionKey,
channel,
chatID,
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

Cron execution is invoked with context.Background() (see gateway setup), so this agent call (and the subsequent outbound publish via PublishResponseIfNeeded) currently has no deadline. Since MessageBus.PublishOutbound can block when buffers fill, please consider wrapping cron job execution/publish in a timeout context to prevent the cron loop from getting stuck behind a slow agent run or blocked outbound consumer.

Copilot uses AI. Check for mistakes.
"command": map[string]any{
"type": "string",
"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message. 'deliver' will be forced to false for commands.",
"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message.",
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

The command parameter description says “the agent will run this command”, but ExecuteJob runs scheduled commands by calling t.execTool.Execute(...) directly and then publishing to the outbound bus (no agent loop involved). Please update the schema description to match actual behavior (e.g., “Cron executes the command via the exec tool and publishes the output”).

Suggested change
"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message.",
"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, Cron executes this command via the exec tool and publishes the output instead of just showing the message.",

Copilot uses AI. Check for mistakes.
@sipeed-bot sipeed-bot bot added type: enhancement New feature or request domain: corn labels Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: corn type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants