Skip to content

fix(tools): preserve attach_to_session through cron handler - #93854

Closed
Qewler wants to merge 3 commits into
NousResearch:mainfrom
Qewler:fix/cron-attach-session-readback
Closed

Qewler wants to merge 3 commits into
NousResearch:mainfrom
Qewler:fix/cron-attach-session-readback

Conversation

@Qewler

@Qewler Qewler commented Aug 24, 2026 •

Copy link
Copy Markdown

Summary

  • forward attach_to_session from the registered tool handler into cronjob()
  • include explicitly stored true and false values in agent-facing cron job output
  • add regression coverage for handler propagation, persistence, update read-back, and list read-back

Problem

The cronjob schema and implementation accept and persist attach_to_session, but the registered tool handler did not forward the argument. Agent-originated attach-only updates therefore reached cronjob() without any update value and returned No updates provided.

Separately, _format_job() omitted the field from tool responses. Even values written through direct callers were therefore invisible in update and list read-back.

Why this change

The registered handler is the path used by agent tool calls, so accepting a field in the schema is not sufficient unless the handler forwards it. Agent-facing output must then faithfully expose both explicit boolean values: false is meaningful because it can override globally enabled delivery mirroring. The field remains omitted for legacy jobs where it was never set, preserving the distinction between "unset" and an explicit boolean value.

Reproduction

  1. Create a cron job without attach_to_session.
  2. Through the registered tool handler, update only attach_to_session.
  3. Observe No updates provided. because the handler drops the argument.
  4. Through a direct caller, persist the value and observe that update and list responses omit it.

Testing

  • ruff check tools/cronjob_tools.py tests/tools/test_cronjob_tools.py
  • scripts/run_tests.sh tests/tools/test_cronjob_tools.py tests/cron/test_scheduler.py tests/cron/test_cronjob_schema.py — 175 passed

Related work

Related to #57556, which identified the missing handler forwarding. This focused change also preserves an explicit false value and covers both booleans through the registered handler.

Platform tested

  • macOS
  • Python 3.11

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 24, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: open #57556 already includes the same _format_job() read-back behavior along with the handler passthrough. This PR is the focused output-only subset.

@Qewler Qewler changed the title fix(tools): expose attach_to_session in cron output fix(tools): preserve attach_to_session through cron handler Aug 24, 2026
@Qewler

Qewler commented Aug 24, 2026

Copy link
Copy Markdown
Author

Thanks — #57556 exposed an important second layer of this bug. I have incorporated the missing registered-handler forwarding into this PR and added handler-level regression coverage for both attach_to_session=True and False.

This PR's formatter preserves either explicit boolean value rather than only truthy state, because false can override globally enabled delivery mirroring. The focused cron suite now passes 175 tests.

@kshitijk4poor

Copy link
Copy Markdown

The attach_to_session handler-forward hole this PR fixes has been salvaged onto current main via #95366 (from #84829, the variant with the explicit-false _format_job guard plus real-store round-trip tests — several open PRs fixed the same one-line hole, and one had to be picked). Closing as a duplicate; thank you for finding and fixing the real bug, and sorry it collided with other submissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants