fix: close model registry review gaps - #2780
Conversation
stranske
added
automation
|
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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (7)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Automated Status SummaryHead SHA: bfa41d5
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 tightens the model-registry “reviewed selection” contract and aligns helper behavior with runtime slot resolution to close gaps identified during sync-wave review.
Changes:
- Fail closed when a reviewed model selection lacks
evidence_ids. - Make
configured_model_for_provider()honor the same runtime slot/env override resolution path as callers. - Update the registry freshness gate to compare explicit model pins without a profile against the default reviewed profile, with test coverage added.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/llm_registry.py | Enforces evidence-gated selections and reuses runtime slot resolution in configured_model_for_provider(). |
| tools/check_model_registry_freshness.py | Adjusts slot-pin evaluation to compare no-profile pins against the default reviewed profile. |
| tests/tools/test_llm_registry_selection.py | Adds regression coverage for evidence-less selections and runtime env slot override behavior. |
| tests/test_check_model_registry_freshness.py | Adds regression coverage for no-profile explicit pins being flagged vs the default selection. |
| templates/consumer-repo/tools/llm_registry.py | Mirrors the llm_registry changes into the consumer template to prevent drift. |
| for slot in resolve_slots(): | ||
| if slot.provider == normalized_provider and not is_model_blocked( | ||
| slot.provider, slot.model, registry=entries | ||
| ): | ||
| return slot.model |
| for slot in resolve_slots(): | ||
| if slot.provider == normalized_provider and not is_model_blocked( | ||
| slot.provider, slot.model, registry=entries | ||
| ): | ||
| return slot.model |
| # A slot without an explicit profile still resolves through the | ||
| # default reviewed profile at runtime. Compare it against that | ||
| # decision so an old model pin cannot silently bypass review. | ||
| effective_profile = profile or "verifier-balanced" | ||
| selected = selection_by_key.get((effective_profile, provider)) |
![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: 933228334f
ℹ️ 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".
| for slot in resolve_slots(): | ||
| if slot.provider == normalized_provider and not is_model_blocked( | ||
| slot.provider, slot.model, registry=entries | ||
| ): | ||
| return slot.model |
There was a problem hiding this comment.
When LANGCHAIN_MODEL is set while the registry/slot config is unavailable, resolve_slots() creates placeholder slots for the other providers with model == ""; this new loop treats those placeholders as valid because is_model_blocked(..., "") is false and returns the empty string before reaching the fallback argument. In that emergency-bootstrap scenario, calls such as configured_model_for_provider("anthropic", fallback="claude-fallback") now disable the provider instead of using the supplied fallback, so this should require a non-empty slot.model before returning.
Useful? React with 👍 / 👎.
Addresses active sync-wave review debt in the shared source of truth.\n\n- reject evidence-free reviewed selections\n- honor runtime slot environment overrides in configured model lookup\n- flag model pins without a profile against the default reviewed profile\n\nValidated: Python 3.12 focused registry/freshness tests; template sync/completeness; diff check.