fix: harden synced registry contracts - #2806
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 #2806 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. |
|
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)
📝 WalkthroughWalkthroughCapability bundle validation rejects invalid gate and playbook values. Model registry selection normalizes profiles, slot loading fails closed when configuration is unavailable, freshness checks classify invalid pins, and unresolved model slots emit warnings. Consumer templates and tests are updated. ChangesCapability bundle validation
Model registry resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
0879079 to
e6c53d3
Compare
Automated Status SummaryHead SHA: 801bed8
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 hardens the model registry / slot selection contracts and capability-bundle validation, tightening “fail closed” behavior around legacy pins and improving diagnostics. It also mirrors the relevant contract changes into the consumer template copy to avoid drift.
Changes:
- Normalize selection profile inputs (trim whitespace / default to
DEFAULT_SELECTION_PROFILE) and tighten legacy slot-pin behavior to only honor current, unblocked pins. - Improve client/runtime diagnostics when skipping unresolved slot models and strengthen capability-bundle string validation (gates + optional playbooks).
- Update tests and synchronize the affected template copies under
templates/consumer-repo/.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/llm_registry.py | Normalizes profile selection and hardens slot-config behavior around advisory legacy pins. |
| tools/langchain_client.py | Adds explicit warning when a slot has no resolved model before skipping it. |
| tools/check_model_registry_freshness.py | Tightens freshness gate behavior for legacy pins; now rejects unknown/blocked pins and uses shared default profile constant. |
| tests/tools/test_llm_registry_selection.py | Adds coverage ensuring profile selection normalizes whitespace. |
| tests/test_check_model_registry_freshness.py | Updates expectations: unknown/blocked legacy pins are now rejected (new findings asserted). |
| templates/consumer-repo/tools/llm_registry.py | Mirrors the registry/profile/slot hardening changes into the consumer template copy. |
| .github/scripts/capability_bundle.js | Strengthens schema validation: gates must be non-empty strings; playbooks (if present) must be non-empty strings. |
| templates/consumer-repo/.github/scripts/capability_bundle.js | Mirrors capability-bundle validation hardening into the consumer template copy. |
| .github/scripts/tests/capability-bundle-contract.test.js | Adds tests for non-string gates and invalid playbooks entries. |
| if selected.get("model_id") != explicit_model: | ||
| if not profile: | ||
| continue | ||
| findings.append( | ||
| _finding( |
![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: 08790793cd
ℹ️ 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".
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from tools.llm_registry import DEFAULT_SELECTION_PROFILE |
There was a problem hiding this comment.
When Maint 77 invokes this file as python3 tools/check_model_registry_freshness.py --json (as in .github/workflows/maint-77-model-registry-freshness.yml), Python puts tools/ rather than the repo root on sys.path, so this new absolute import fails before any JSON is written: ModuleNotFoundError: No module named 'tools'. I verified the command from the repo root and it exits 1, which makes PR freshness checks fail and scheduled runs create malformed/empty reports instead of evaluating the registry.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/check_model_registry_freshness.py (1)
343-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicate invalid-pin checks.
Lines 343-356 now emit
unknown_pin/blocked_pinfor every explicit pin, but Lines 367-380 emit the same finding again whenprofileis set. Remove the retained profile-specific block and assert each finding kind occurs once in the rejection tests.Proposed fix
- if profile and model is None: - findings.append( - _finding( - "unknown_pin", - f"slot {name!r} pins absent model {provider}/{explicit_model}.", - ) - ) - elif profile and model.get("blocked"): - findings.append( - _finding( - "blocked_pin", - f"slot {name!r} pins blocked model {provider}/{explicit_model}.", - ) - ) continueAs per path instructions, “Prioritize correctness, error handling, and test coverage. Flag new or changed behavior with no accompanying test.”
🤖 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/check_model_registry_freshness.py` around lines 343 - 380, Remove the redundant profile-gated unknown_pin/blocked_pin block after the selection_override handling, keeping the earlier checks as the single source of these findings for all explicit pins. Update the rejection tests to assert each finding kind is emitted only once, including cases with a profile.Source: Path instructions
🤖 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 @.github/scripts/__tests__/capability-bundle-contract.test.js:
- Around line 123-130: Add a contract assertion alongside the existing
validateCapabilityBundle cases that passes validBundle({ gates: [] }) and
expects the “at least one gate ref” validation error, covering the newly
rejected empty-gates boundary.
In `@tests/tools/test_llm_registry_selection.py`:
- Around line 71-78: Extend test_profile_selection_normalizes_whitespace to
assert that an empty or whitespace-only profile falls back to
DEFAULT_SELECTION_PROFILE and returns the expected default model. Reuse the
existing registry setup and select_model_for_profile call, covering the
profile.strip() or default branch without changing the valid-profile assertion.
---
Outside diff comments:
In `@tools/check_model_registry_freshness.py`:
- Around line 343-380: Remove the redundant profile-gated
unknown_pin/blocked_pin block after the selection_override handling, keeping the
earlier checks as the single source of these findings for all explicit pins.
Update the rejection tests to assert each finding kind is emitted only once,
including cases with a profile.
🪄 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: 181e1a4f-952c-4d23-a5f2-53cbb357ad49
📒 Files selected for processing (9)
.github/scripts/__tests__/capability-bundle-contract.test.js.github/scripts/capability_bundle.jstemplates/consumer-repo/.github/scripts/capability_bundle.jstemplates/consumer-repo/tools/llm_registry.pytests/test_check_model_registry_freshness.pytests/tools/test_llm_registry_selection.pytools/check_model_registry_freshness.pytools/langchain_client.pytools/llm_registry.py
| def test_profile_selection_normalizes_whitespace( | ||
| monkeypatch: pytest.MonkeyPatch, tmp_path: Path | ||
| ) -> None: | ||
| registry_path = tmp_path / "registry.json" | ||
| _write_registry(registry_path) | ||
| monkeypatch.setenv(registry.ENV_MODEL_REGISTRY_CONFIG, str(registry_path)) | ||
|
|
||
| assert registry.select_model_for_profile(provider="openai", profile=" verifier-balanced ") == "model-balanced" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the whitespace-only default-profile branch.
This verifies trimming around a valid profile, but not profile.strip() or DEFAULT_SELECTION_PROFILE. Add an assertion for profile="" or " " so the fallback cannot regress.
🤖 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 `@tests/tools/test_llm_registry_selection.py` around lines 71 - 78, Extend
test_profile_selection_normalizes_whitespace to assert that an empty or
whitespace-only profile falls back to DEFAULT_SELECTION_PROFILE and returns the
expected default model. Reuse the existing registry setup and
select_model_for_profile call, covering the profile.strip() or default branch
without changing the valid-profile assertion.
Source: Path instructions
|
Runner dispatch state for autofix on PR #2806. Do not edit. |
github-actions
Bot
added
the
autofix
|
Runner dispatch state for codex on PR #2806. Do not edit. |
|
Autofix updated these files:
|
Summary
Validation
python -m pytest tests/test_check_model_registry_freshness.py tests/tools/test_llm_registry_selection.py tests/tools/test_langchain_client.py -qnode --test .github/scripts/__tests__/capability-bundle-contract.test.jspython scripts/validate_template_sync.pySummary by CodeRabbit
gatesentries and malformed/emptyplaybooks.