fix(repo-monitor): honor AUTOMATION_MODEL profile - #606
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
There was a problem hiding this comment.
🟡 Changes recommended
The new tests cannot import github_client in a clean checkout because its shared-client directory is not added to sys.path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates github-repo-monitor to honor the configured AUTOMATION_MODEL profile, with fallback to the active profile.
Changes:
- Adds profile resolution through the Agent API.
- Adds fallback handling for unset or missing profiles.
- Adds regression tests for profile selection.
File summaries
| File | Description |
|---|---|
tests/test_github_repo_monitor.py |
Adds tests for profile resolution and fallback behavior. |
skills/github-repo-monitor/scripts/main.py |
Resolves profiles and forwards the selected LLM configuration. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Regarding the |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope is correctly limited to github-repo-monitor (issue #428 also covers github-pr-reviewer, which is handled separately). The change closes a real gap: _get_agent_dict previously always read agent_settings.llm from GET /api/settings, so monitor runs used whatever profile was active in the UI instead of the automation's configured profile.
What I verified on head 983e322:
- The profile-resolution branch mirrors the SDK's own reference implementation (
openhands/sdk/workspace/remote/base.py::_fetch_llm_profile_config/get_llm):GET /api/profiles/{name}withX-Expose-Secrets: plaintext,usage_id = profile:<name>, and 404/FileNotFoundErrorfalling back to the default. Matching that established contract rather than inventing a new one is the right call. - Fallback behavior is retained both when
AUTOMATION_MODELis unset and when the named profile 404s, so existing deployments do not regress. quote(profile, safe='')correctly encodes the profile name into the path segment.- Tests:
python -m pytest tests/test_github_repo_monitor.py -q-> 3 passed in the workspace; the skill's ownskills/github-repo-monitor/tests/test_main.pyplustests/test_slack_channel_monitor.py-> 46 passed, so the change does not disturb the existing_get_agent_dictregression coverage. The 3 new tests exercise the three branches (profile hit, 404 fallback, unset env with no profile request) against real code paths rather than mocking the code under test. - CI on the exact head SHA: only the
pull_request_targetchecks (PR Description Check, pr-title) report success; thepull_requestworkflows (Tests, Check Extensions, Deprecation deadlines) showaction_required, i.e. awaiting approval to run, not failing. Worth noting that CI has not independently executed the suite on this head, so the local run above is the substantive evidence.
No blocking or non-blocking correctness, security, or design issues are demonstrable on this head.
✅ APPROVED
|
Resolved the Copilot import-path thread as a false positive: repository-level AI disclosure: This comment was generated by OpenHands on behalf of @neubig. |
neubig
left a comment
There was a problem hiding this comment.
The monitor was demonstrably ignoring AUTOMATION_MODEL; this implementation follows the SDK profile-resolution contract, preserves the no-profile and deleted-profile fallbacks, and propagates genuine server failures. Focused tests cover all three paths. The Copilot import concern was a false positive under the repository pytest pythonpath configuration and has been resolved.
AI disclosure: This review was submitted by OpenHands on behalf of @neubig.
|
🚀 Released in v0.25.0. |
HUMAN:
I ran the targeted github-repo-monitor tests and confirmed the profile-resolution case fails on base but passes with the fix, with no new ruff findings beyond the existing ones.
Why
github-repo-monitorbuilds its conversation payload from the agent server's active profile and never readsAUTOMATION_MODEL, so monitor automations silently run on whatever profile is active in the UI instead of their configured one.Summary
_get_agent_dictnow resolvesAUTOMATION_MODELviaGET /api/profiles/{name}(plaintext secrets) and forwards that config withusage_id: profile:<name>; falls back to the active profile when unset or on 404.tests/test_github_repo_monitor.pywith 3 tests. Scoped to repo-monitor only per maintainer note on PR reviewer and repo monitor skills ignore AUTOMATION_MODEL — automations run on the active profile, not their configured one #428.Issue Number
Fixes #428
How to Test
python -m pytest tests/test_github_repo_monitor.py -q→ 3 passed. On base, the profile-resolution test fails (no profile request is made).python -m ruff checkon touched files: no new findings (2 pre-existing F541s on base);scripts/sync_extensions.py --checkfails identically on base (locale decode issue in this env), unrelated.Companion reproduction record with template sections: #605.
Type
Bug fix