fix: preserve consumer slot compatibility - #2781
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Workflow source neededPR #2781 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request updates GitHub diff media headers, permits provider fallback when GitHub model selection is unavailable, changes slot allowlist and pin resolution, adjusts freshness validation, trims benchmark model identifiers, and updates worker metadata. ChangesAPI and model-selection maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Triage as _get_llm_client
participant Registry as configured_model_for_provider
participant GitHub as GitHub provider
participant OpenAI as OpenAI provider
Triage->>Registry: Resolve GitHub model
Registry-->>Triage: Empty model
Triage->>Registry: Resolve OpenAI model
Registry-->>Triage: OpenAI model
Triage->>OpenAI: Create ChatOpenAI client
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Automated Status SummaryHead SHA: 48b3cbf
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts the LLM registry/selection and supporting tooling to preserve compatibility with legacy consumer slot pins, enforce stricter slot allowlisting behavior, improve provider fallback behavior in CI triage, normalize benchmark model IDs, and update GitHub diff media type usage.
Changes:
- Update slot-config resolution to honor legacy explicit model pins when no profile is declared, and to treat presence of a slot config as an allowlist (fail-closed semantics).
- Make CI failure triage fall through from GitHub Models to OpenAI when GitHub Models is unavailable/unconfigured.
- Normalize benchmark
model_idvalues (trim whitespace) and request PR diffs using the GitHub v3 diff media type.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/llm_registry.py | Adjusts slot resolution rules, including legacy pin compatibility and allowlist/fail-closed behavior. |
| templates/consumer-repo/tools/llm_registry.py | Mirrors the slot resolution behavior changes for consumer templates. |
| tools/ci_failure_triage.py | Enables fallback from GitHub Models to OpenAI when the GitHub Models selection can’t be made. |
| templates/consumer-repo/tools/ci_failure_triage.py | Mirrors CI triage fallback behavior for consumer templates. |
| tools/evaluate_model_benchmark.py | Trims model_id strings to avoid whitespace mismatches during benchmark evaluation. |
| scripts/api_client.py | Uses application/vnd.github.v3.diff for PR diff fetches. |
| templates/consumer-repo/scripts/api_client.py | Mirrors GitHub v3 diff media type usage for consumer templates. |
| tests/tools/test_llm_registry_selection.py | Updates/extends tests to reflect revised slot pin and allowlist semantics. |
| tests/scripts/test_api_client.py | Updates expected Accept header for PR diff requests. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e3ebfd89e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/llm_registry.py (1)
322-367: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTreat
slots: []as an explicit allowlist
_slot_entries()maps both a missing"slots"key and{"slots": []}to[], soreturn slots if slot_entries else fallback_slotsstill falls back to default providers when an operator explicitly declares zero slots. Base the final return on key presence instead of list truthiness in bothtools/llm_registry.pyandtemplates/consumer-repo/tools/llm_registry.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/llm_registry.py` around lines 322 - 367, Update the final return logic in _slot_entries in both tools/llm_registry.py (lines 322-367) and templates/consumer-repo/tools/llm_registry.py (lines 322-367) to distinguish a missing slots key from an explicitly empty slots list. Return fallback_slots only when the configuration omits the slots key; preserve an explicit slots: [] as an empty allowlist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/llm_registry.py`:
- Around line 322-367: Update the final return logic in _slot_entries in both
tools/llm_registry.py (lines 322-367) and
templates/consumer-repo/tools/llm_registry.py (lines 322-367) to distinguish a
missing slots key from an explicitly empty slots list. Return fallback_slots
only when the configuration omits the slots key; preserve an explicit slots: []
as an empty allowlist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb1d800a-4699-4252-b7f0-65a98962ba69
📒 Files selected for processing (9)
scripts/api_client.pytemplates/consumer-repo/scripts/api_client.pytemplates/consumer-repo/tools/ci_failure_triage.pytemplates/consumer-repo/tools/llm_registry.pytests/scripts/test_api_client.pytests/tools/test_llm_registry_selection.pytools/ci_failure_triage.pytools/evaluate_model_benchmark.pytools/llm_registry.py
|
Runner dispatch state for autofix on PR #2781. Do not edit. |
github-actions
Bot
added
the
autofix
|
Runner dispatch state for codex on PR #2781. Do not edit. |
|
Autofix updated these files:
|
|
Addressed the four inline findings in c0c8b35: readable slot configs now remain strict allowlists (including empty/invalid-only entries), CI triage cannot fall through to an unlisted provider, and the freshness gate accepts current unblocked legacy pins without weakening explicit-profile checks. Focused validation: 133 passed; template sync and completeness validation passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tools/test_ci_failure_triage.py`:
- Around line 40-57: The test
test_llm_triage_does_not_fall_back_outside_slot_allowlist currently leaves both
providers unavailable and does not exercise fallback. Configure GitHub to
resolve to an empty model and OpenAI to resolve to a valid model, then assert
_get_llm_client() returns the OpenAI provider and expected model while verifying
ChatOpenAI receives that model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 899b92c1-3cba-4a97-a873-dc72adb3ffd9
📒 Files selected for processing (6)
templates/consumer-repo/tools/llm_registry.pytests/test_check_model_registry_freshness.pytests/tools/test_ci_failure_triage.pytests/tools/test_llm_registry_selection.pytools/check_model_registry_freshness.pytools/llm_registry.py
|
Addressed the remaining CodeRabbit finding in 6634d13: added a focused regression that verifies GitHub Models resolving empty falls through to the configured OpenAI model, constructs |
Fix shared review debt from the current consumer sync wave. Preserves current, unblocked legacy model pins when no profile is declared; treats a present slot config as an allowlist; falls through from unavailable GitHub Models to OpenAI; normalizes benchmark model IDs; and uses GitHub v3 diff media type.\n\nValidation:
python3.12 -m pytest -q tests/tools/test_llm_registry_selection.py tests/tools/test_langchain_client.py tests/tools/test_ci_failure_triage.py tests/tools/test_evaluate_model_benchmark.py tests/scripts/test_api_client.py(110 passed); template sync and completeness checks passed.Summary by CodeRabbit