Skip to content

fix(schedule): omit an untouched agent_id from a serialized CronJobPatch - #144

Merged
senamakel merged 1 commit into
mainfrom
cron-patch-agent-id
Oct 16, 2026
Merged

senamakel merged 1 commit into
mainfrom
cron-patch-agent-id

Conversation

@senamakel

@senamakel senamakel commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Why

CronJobPatch.agent_id is a double option whose custom deserializer reads a present null as "clear the agent definition". Serialization wrote "agent_id": null for None (leave untouched), so any patch that crossed the bus (OpenHuman #7397's cron cutover onto the Schedule contract) wiped the job's agent definition.

Change

skip_serializing_if = "Option::is_none" on agent_id, matching the sibling origin field.

Test

schedule::wire_tests::an_unrelated_patch_does_not_clear_the_agent_definition_on_a_round_trip fails without the change (verified) and passes with it. cargo test -p tinyflows-bus -p tinyflows-schedule is green.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed schedule updates so changing a setting such as whether a job is enabled no longer accidentally clears its agent assignment when the update is saved and restored.
    • Explicitly clearing an agent assignment still works as expected, preserving the distinction between leaving the assignment unchanged and clearing it.

The patch's agent_id is Option<Option<String>> with a deserializer that reads a present null as a clear. Without skip_serializing_if a patch that left the agent alone serialized agent_id: null, so any CronJobPatch sent over the bus cleared the job's agent definition.

Co-authored-by: Medulla <medulla@tinyhumans.ai>
@tinysweeper

tinysweeper Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper completed its review; deterministic results follow.

State: Ready for maintainer review
Priority: none
Reviewed head: f1a1fb262183
Updated: 2026-10-16T14:54:29Z

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 0
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

`CronJobPatch::agent_id` in `crates/tinyflows-bus/src/schedule/types.rs` now carries `skip_serializing_if = "Option::is_none"` alongside its existing `default` and `deserialize_with = "deserialize_double_option"` attributes, and gains doc text explaining that an omitted field means "leave the agent alone" while a present `null` means "clear the agent definition". Behaviourally, a patch that does not mention the agent definition no longer serializes `"agent_id": null` across a serialized boundary such as the bus, which previously would have been decoded as an explicit clear and turned every unrelated patch into a clear-the-agent operation. Explicit clears still serialize as `null`. A corresponding round-trip test was added in `crates/tinyflows-bus/src/schedule/wire_tests.rs`.

Features

  • Modified — Omit an untouched `agent_id` from a serialized `CronJobPatch`: A `CronJobPatch` whose `agent_id` is `None` (agent unchanged) no longer emits `"agent_id": null` when serialized, so patches crossing the bus cannot be misread by the double-option deserializer as an explicit clear of the agent definition; `Some(None)` still serializes as `null` to express a clear, preserving the existing clear semantics documented on the field. (crates/tinyflows-bus/src/schedule/types.rs#pub struct CronJobPatch {)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

How this fits together

flowchart LR
  n0["CronJobPatch<br/>changed"]:::changed
  n1["update_job"]:::impacted
  n2["add_shell_job"]:::impacted
  n3["get_job"]:::impacted
  n4["CronJob"]:::impacted
  n5["complete_run"]:::impacted
  n1 -->|uses| n0
  n1 -->|uses| n4
  n2 -->|uses| n4
  n3 -->|uses| n4
  n5 -->|uses| n4
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The critique and security lanes reviewed both changed files and reported zero findings and zero pending questions.
  • Lane summary: Reviewed 2 files; 0 findings. _The code index is behind this pull request (indexed at `62c3e78c3223`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 2 files; 0 findings. _The code index is behind this pull request (indexed at `62c3e78c3223`), so retrieved context may be out of date._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The new test asserts real, falsifiable behaviour rather than restating implementation details, and the tests lane found no defects.
  • Lane summary: The change adds `skip_serializing_if = "Option::is_none"` to `CronJobPatch::agent_id` (matching the existing `origin` field) and adds a round-trip test in `wire_tests.rs` that pins both the omit-when-`None` and the explicit-null-clear behaviours. The new test asserts real, falsifiable behaviour: without the attribute, `to_value` would emit `"agent_id": null` and the first `assert!` would fail. No defects found. _The code index is behind this pull request (indexed at `62c3e78c3223`), so retrieved context may be out of date._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane found nothing sensitive in what this pull request commits.
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The change matches its description, mirrors the sibling `origin` field's serialization treatment, and the affected files stay well under the repository's 500-line limit; nothing looks unsafe to merge.
  • Lane summary: Adds `skip_serializing_if = "Option::is_none"` to `CronJobPatch::agent_id` so an untouched field is omitted instead of written as `null`, which the double-option deserializer would otherwise read back as an explicit clear, plus a round-trip test covering both the untouched and explicit-clear cases. The change matches its description, mirrors the sibling `origin` field, and the affected files stay well under the repository's 500-line limit; nothing looks unsafe to merge. _The code index is behind this pull request (indexed at `62c3e78c3223`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.001342
  • Tokens: 152364 input · 7583 output · 31744 cached · 141 embedding
Head State Pass summary
f1a1fb262183 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-16T14:54:29Z)

tinysweeper 0.1.2

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ea647267-a691-41cb-ae74-661463b83c21
📥 Commits

Reviewing files that changed from the base of the PR and between fbeac7c and f1a1fb2.

📒 Files selected for processing: 2
  • crates/tinyflows-bus/src/schedule/types.rs
  • crates/tinyflows-bus/src/schedule/wire_tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

CronJobPatch.agent_id now omits an unset value during serialization. Explicit null still represents clearing the agent definition. A wire test covers both cases.

Changes: CronJobPatch wire behavior

Layer / File(s) Summary
Patch wire semantics
crates/tinyflows-bus/src/schedule/types.rs, crates/tinyflows-bus/src/schedule/wire_tests.rs
Serialization omits agent_id when its value is None. The test verifies that an unrelated update round-trips with None, while an explicit clear emits null and round-trips with Some(None).

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f1a1f

Unrelated schedule patches should no longer clear an agent assignment when sent over the bus. No actionable merge risk remains beyond normal checks.

Security architecture risk: ⚪ Minimal · up to f1a1f

The change prevents unrelated updates from accidentally clearing a job’s agent definition. Explicit clears and assignments remain supported, and the existing reader accepts the corrected representation. No material security risk was identified in this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed effect is bounded to serialized patches whose agent_id is untouched and the jobs those patches already address. It prevents unintended agent-definition removal without adding a new target-selection field or a new explicit agent mutation operation.

Trust Boundaries and Controls

  • observed — The serialized-input reader retains the existing distinction between omission, null, and a supplied identifier. Explicitly supplied null and identifiers reach the same assignment branch as before; this change corrects the emitter rather than broadening accepted input or bypassing the patch validation call.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: omitting an untouched agent_id when serializing CronJobPatch.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

I hop by the patch where agent_id can rest,
No null for the field when it is not addressed.
An explicit clear still sends null on its way,
The wire test checks both paths today.
Then I tuck into clover, content and blessed.

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T14:54:22.386685Z f1a1fb2 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tinysweeper found nothing blocking. Approving.

             $0.0013 · 152,364 in / 7,583 out · 31,744 cached (21%) · ladder/vectors-oai3, deepseek-v4.1-flash · 141 embedded
critique:    $0.0001 · 47,579 in  / 671 out   · 17,152 cached (36%) · deepseek-v4.1-flash
security:    $0.0000 · 45,306 in  / 439 out   · 5,504 cached (12%)  · deepseek-v4.1-flash
tests:       $0.0000 · 31,036 in  / 398 out   · 4,992 cached (16%)  · deepseek-v4.1-flash
description: $0.0010 · 26,347 in  / 4,499 out · 4,096 cached (16%)  · deepseek-v4.1-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 16, 2026
@senamakel
senamakel merged commit 77d288e into main Oct 16, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants