Skip to content

fix: apply fallback chat models to background wakeups#8909

Open
zzz27578 wants to merge 1 commit into
AstrBotDevs:masterfrom
zzz27578:fix/proactive-fallback-chat-models
Open

fix: apply fallback chat models to background wakeups#8909
zzz27578 wants to merge 1 commit into
AstrBotDevs:masterfrom
zzz27578:fix/proactive-fallback-chat-models

Conversation

@zzz27578

@zzz27578 zzz27578 commented Jun 19, 2026

Copy link
Copy Markdown

Summary

  • Pass provider_settings into MainAgentBuildConfig for background task result wakeups.
  • Keep existing background wakeup streaming behavior while preserving fallback_chat_models and other provider settings.
  • Add a unit test covering provider_settings propagation for background task wakeups.

Why

#9054 already fixed the cron/future-task wakeup path. Background task result wakeups still built MainAgentBuildConfig without the full provider_settings, so fallback_chat_models were not available to the main agent runner after a background task completed.

Tests

  • uv run ruff format --check .
  • uv run ruff check .
  • uv run pytest tests\unit\test_astr_agent_tool_exec.py tests\unit\test_cron_manager.py tests\test_tool_loop_agent_runner.py -q

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. labels Jun 19, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the background and cron wakeup processes to extract and pass provider_settings to the MainAgentBuildConfig. It also adds corresponding unit tests to verify that these settings, such as fallback chat models, are correctly propagated. The feedback suggests adding defensive guarding when calling ctx.get_config() in astr_agent_tool_exec.py to prevent a potential AttributeError if it returns None.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread astrbot/core/astr_agent_tool_exec.py Outdated
Comment on lines +546 to +548
provider_settings = ctx.get_config(umo=event.unified_msg_origin).get(
"provider_settings", {}
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To prevent a potential AttributeError if ctx.get_config() returns None, it is safer to use defensive guarding (e.g., defaulting to an empty dictionary) before calling .get().

        cfg = ctx.get_config(umo=event.unified_msg_origin) or {}
        provider_settings = cfg.get("provider_settings", {})

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • In cron.manager._woke_main_agent, streaming_response remains hard-coded to False whereas _wake_main_agent_for_background_result now derives it from provider_settings; consider also honoring provider_settings.get("stream") for cron wakeups for consistency with normal and background flows.
  • The _DoneRunner helper is duplicated in both test_astr_agent_tool_exec.py and test_cron_manager.py; consider extracting it into a shared test utility or fixture to avoid duplication and keep behavior in sync.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `cron.manager._woke_main_agent`, `streaming_response` remains hard-coded to `False` whereas `_wake_main_agent_for_background_result` now derives it from `provider_settings`; consider also honoring `provider_settings.get("stream")` for cron wakeups for consistency with normal and background flows.
- The `_DoneRunner` helper is duplicated in both `test_astr_agent_tool_exec.py` and `test_cron_manager.py`; consider extracting it into a shared test utility or fixture to avoid duplication and keep behavior in sync.

## Individual Comments

### Comment 1
<location path="tests/unit/test_astr_agent_tool_exec.py" line_range="426-431" />
<code_context>
+        summary_name="BackgroundTask",
+    )
+
+    config = captured["config"]
+    assert config.tool_call_timeout == 456
+    assert config.provider_settings is provider_settings
+    assert config.provider_settings["fallback_chat_models"] == [
+        "fallback-provider"
+    ]
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions for streaming_response (and possibly other provider_settings-derived fields) to fully verify the new behavior.

Because `_wake_main_agent_for_background_result` now derives `config.streaming_response` from `provider_settings["stream"]`, please add an assertion for that here (e.g. `assert config.streaming_response is False`) so the new wiring is exercised. Likewise, if other fields are expected to propagate from `provider_settings` (e.g. `request_max_retries`), consider asserting them to strengthen this test against regressions.

```suggestion
    config = captured["config"]
    assert config.tool_call_timeout == 456
    assert config.provider_settings is provider_settings
    # streaming_response is derived from provider_settings["stream"]
    assert config.streaming_response == provider_settings["stream"]
    # request_max_retries should also propagate from provider_settings
    assert config.request_max_retries == provider_settings["request_max_retries"]
    assert config.provider_settings["fallback_chat_models"] == [
        "fallback-provider"
    ]
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread tests/unit/test_astr_agent_tool_exec.py Outdated
Comment on lines +426 to +431
config = captured["config"]
assert config.tool_call_timeout == 456
assert config.provider_settings is provider_settings
assert config.provider_settings["fallback_chat_models"] == [
"fallback-provider"
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Add assertions for streaming_response (and possibly other provider_settings-derived fields) to fully verify the new behavior.

Because _wake_main_agent_for_background_result now derives config.streaming_response from provider_settings["stream"], please add an assertion for that here (e.g. assert config.streaming_response is False) so the new wiring is exercised. Likewise, if other fields are expected to propagate from provider_settings (e.g. request_max_retries), consider asserting them to strengthen this test against regressions.

Suggested change
config = captured["config"]
assert config.tool_call_timeout == 456
assert config.provider_settings is provider_settings
assert config.provider_settings["fallback_chat_models"] == [
"fallback-provider"
]
config = captured["config"]
assert config.tool_call_timeout == 456
assert config.provider_settings is provider_settings
# streaming_response is derived from provider_settings["stream"]
assert config.streaming_response == provider_settings["stream"]
# request_max_retries should also propagate from provider_settings
assert config.request_max_retries == provider_settings["request_max_retries"]
assert config.provider_settings["fallback_chat_models"] == [
"fallback-provider"
]

@zzz27578 zzz27578 force-pushed the fix/proactive-fallback-chat-models branch 2 times, most recently from 372e497 to a54623d Compare June 19, 2026 18:36
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 21, 2026
@zzz27578 zzz27578 force-pushed the fix/proactive-fallback-chat-models branch from a54623d to e01cf9c Compare June 28, 2026 15:06
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 28, 2026
@zzz27578 zzz27578 force-pushed the fix/proactive-fallback-chat-models branch from e01cf9c to 0e8c7cb Compare June 30, 2026 08:47
@zzz27578 zzz27578 changed the title fix: apply fallback chat models to proactive wakeups fix: apply fallback chat models to background wakeups Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants