fix: apply fallback chat models to background wakeups#8909
Conversation
There was a problem hiding this comment.
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.
| provider_settings = ctx.get_config(umo=event.unified_msg_origin).get( | ||
| "provider_settings", {} | ||
| ) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
cron.manager._woke_main_agent,streaming_responseremains hard-coded toFalsewhereas_wake_main_agent_for_background_resultnow derives it fromprovider_settings; consider also honoringprovider_settings.get("stream")for cron wakeups for consistency with normal and background flows. - The
_DoneRunnerhelper is duplicated in bothtest_astr_agent_tool_exec.pyandtest_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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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" | ||
| ] |
There was a problem hiding this comment.
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.
| 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" | |
| ] |
372e497 to
a54623d
Compare
a54623d to
e01cf9c
Compare
e01cf9c to
0e8c7cb
Compare
Summary
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