fix: deliver cron active agent fallback response

[fxquarter]

Summary

• add a fallback send path for active-agent cron jobs when the agent returns final assistant text but does not call send_message_to_user
• detect existing send-message tool output to avoid duplicate delivery
• cover fallback send and duplicate-skip behavior in cron manager tests

Validation

• ruff format .
• ruff check .
• uv run pytest tests/unit/test_cron_manager.py

Summary by Sourcery

Ensure active-agent cron jobs always deliver assistant responses by adding a fallback send path and avoiding duplicate deliveries when the agent already sent a message.

Bug Fixes:

• Add a fallback delivery path for active-agent cron jobs when the agent returns final assistant text without explicitly sending a user message.
• Prevent duplicate message delivery by detecting prior send-message tool outputs in provider request results.

Enhancements:

• Log detailed outcomes for active-agent cron fallback behavior, including skips, sends, and exceptions.

Tests:

• Add unit tests covering active-agent cron fallback delivery and the skip path when a message has already been sent.

[gemini-code-assist]

Code Review

This pull request introduces a fallback delivery mechanism for cron jobs in CronJobManager, ensuring that assistant responses are sent to users if they weren't already delivered via tool calls. The changes include logic to detect prior message delivery and new unit tests for the fallback behavior. Feedback focuses on optimizing the execution flow by moving the fallback call after the response validation check and improving log clarity by avoiding redundant stack traces during exception handling.

[gemini-code-assist]

The call to _send_active_agent_fallback_if_needed is currently placed before the null check for llm_resp. Since the fallback mechanism requires a valid response to proceed, it should be called after ensuring llm_resp is not None. This also prevents redundant warning logs when the agent fails to provide any response, as both the fallback function and the current method would log a warning.

Suggested change

	await self._send_active_agent_fallback_if_needed(
	session=session,
	req=req,
	llm_resp=llm_resp,
	cron_meta=cron_meta,
	)
	if not llm_resp:
	logger.warning("Cron job agent got no response")
	return
	if not llm_resp:
	logger.warning("Cron job agent got no response")
	return
	await self._send_active_agent_fallback_if_needed(
	session=session,
	req=req,
	llm_resp=llm_resp,
	cron_meta=cron_meta,
	)

[gemini-code-assist]

The exception is logged here with exc_info=True and then re-raised. The top-level caller _run_job (line 224) also catches this exception and logs it with exc_info=True. This results in duplicate stack traces in the logs for the same failure. Consider removing exc_info=True from this warning to keep the logs cleaner while still preserving the specific context of the fallback failure.

Suggested change

	except Exception as e: # noqa: BLE001
	logger.warning(
	"cron active agent fallback send exception session=%s job_id=%s err=%r",
	session,
	cron_meta.get("id"),
	e,
	exc_info=True,
	)
	raise
	except Exception as e: # noqa: BLE001
	logger.warning(
	"cron active agent fallback send exception session=%s job_id=%s err=%r",
	session,
	cron_meta.get("id"),
	e,
	)
	raise

[sourcery-ai]

Hey - I've left some high level feedback:

• The _agent_sent_message_to_user heuristic depends on the substring "Message sent to session" in tool call outputs, which is brittle; consider detecting sends via a more structured signal (e.g., tool name, fields on the result, or a dedicated flag) so this doesn’t silently break if that text changes.
• In _send_active_agent_fallback_if_needed, you catch a broad Exception only to log and re-raise; if the goal is to avoid cron failures on send issues, consider either narrowing the exception type or swallowing/logging expected send failures instead of propagating them.
• The fallback helper currently hardcodes role == "assistant" and stringifies completion_text; if other response types or roles are introduced, it may be safer to centralize this assistant-text extraction logic or make the role/text checks reusable to avoid drift with other parts of the agent pipeline.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `_agent_sent_message_to_user` heuristic depends on the substring `"Message sent to session"` in tool call outputs, which is brittle; consider detecting sends via a more structured signal (e.g., tool name, fields on the result, or a dedicated flag) so this doesn’t silently break if that text changes.
- In `_send_active_agent_fallback_if_needed`, you catch a broad `Exception` only to log and re-raise; if the goal is to avoid cron failures on send issues, consider either narrowing the exception type or swallowing/logging expected send failures instead of propagating them.
- The fallback helper currently hardcodes `role == "assistant"` and stringifies `completion_text`; if other response types or roles are introduced, it may be safer to centralize this assistant-text extraction logic or make the role/text checks reusable to avoid drift with other parts of the agent pipeline.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}