fix: restore LLM slot bootstrap compatibility - #2796
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 #2796 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. |
📝 WalkthroughWalkthroughThe changes initialize loaded registry entries with ChangesLLM registry updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (2)
tools/llm_registry.py (1)
438-444: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject whitespace-only emergency model values in both registry implementations.
Both
resolve_slots()guards use raw environment-variable truthiness while downstream slot resolution strips the model. A whitespace-onlyLANGCHAIN_MODELtherefore triggers bootstrap but produces only empty models.
tools/llm_registry.py#L438-L444: checkos.environ.get(env_model_name, "").strip()before bootstrapping.templates/consumer-repo/tools/llm_registry.py#L438-L444: apply the identical non-blank guard to keep the canonical template synchronized.As per path instructions:
**/*.py: Prioritize correctness, error handling, and test coverage. Flag new or changed behavior with no accompanying test, silently swallowed exceptions, and unguarded NaN/None propagation in numeric or scoring code.🤖 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 438 - 444, Update the resolve_slots() emergency bootstrap guard in tools/llm_registry.py (438-444) to require os.environ.get(env_model_name, "").strip() before creating slots, and apply the identical change in templates/consumer-repo/tools/llm_registry.py (438-444) to keep both registry implementations synchronized.Source: Path instructions
tests/tools/test_llm_registry_selection.py (1)
210-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the emergency model is actually applied.
The test checks only provider names, so it would pass even if bootstrap returned three placeholder slots with
model=""and never applied"emergency-model".Proposed test improvement
- assert [slot.provider for slot in registry.resolve_slots()] == [ + slots = registry.resolve_slots() + assert [slot.provider for slot in slots] == [ "openai", "anthropic", "github-models", ] + assert slots[0].model == "emergency-model"As per path instructions:
**/*.py: Prioritize correctness, error handling, and test coverage. Flag new or changed behavior with no accompanying test, silently swallowed exceptions, and unguarded NaN/None propagation in numeric or scoring code.🤖 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 210 - 226, Strengthen test_unusable_bundled_slot_config_bootstraps_explicit_env_model by asserting the resolved slot models include the LANGCHAIN_MODEL value "emergency-model", not only their providers. Preserve the existing provider assertions while verifying bootstrap applies the explicit environment model rather than returning placeholder slots.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.
Outside diff comments:
In `@tests/tools/test_llm_registry_selection.py`:
- Around line 210-226: Strengthen
test_unusable_bundled_slot_config_bootstraps_explicit_env_model by asserting the
resolved slot models include the LANGCHAIN_MODEL value "emergency-model", not
only their providers. Preserve the existing provider assertions while verifying
bootstrap applies the explicit environment model rather than returning
placeholder slots.
In `@tools/llm_registry.py`:
- Around line 438-444: Update the resolve_slots() emergency bootstrap guard in
tools/llm_registry.py (438-444) to require os.environ.get(env_model_name,
"").strip() before creating slots, and apply the identical change in
templates/consumer-repo/tools/llm_registry.py (438-444) to keep both registry
implementations synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab9e74aa-021a-4159-8792-ab2870a79d07
📒 Files selected for processing (4)
templates/consumer-repo/tools/llm_registry.pytests/tools/test_langchain_client.pytests/tools/test_llm_registry_selection.pytools/llm_registry.py
![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: fbd2f8fcea
ℹ️ 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".
| and not _slot_path().exists() | ||
| and os.environ.get(env_model_name) | ||
| ): | ||
| if not slots and not os.environ.get(ENV_SLOT_CONFIG) and os.environ.get(env_model_name): |
There was a problem hiding this comment.
When a consumer has customized the create-only config/llm_slots.json at the default path so no slots are usable (for example an empty slot list or only disabled/unknown providers) while leaving LANGCHAIN_SLOT_CONFIG unset, this now expands that empty allowlist into OpenAI/Anthropic/GitHub whenever LANGCHAIN_MODEL is present. The synced agents-issue-optimizer.yml and agents-auto-pilot.yml workflows set LANGCHAIN_MODEL, so those repos can start invoking LLM clients despite their local default slot file intentionally restricting or disabling slots; keep the emergency bootstrap limited to a missing default slot file, or otherwise distinguish an intentionally empty default config from a missing registry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Restores the documented “emergency” LANGCHAIN_MODEL bootstrap path when the bundled/default slot config resolves to no usable slots, while keeping fail-closed behavior for explicitly configured LANGCHAIN_SLOT_CONFIG. It also re-establishes an explicit (empty) v1-compatibility quality mapping on loaded registry entries.
Changes:
- Ensure
load_model_registry()emits entries withquality == {}(compatibility surface) instead ofNone. - Allow
resolve_slots()to bootstrap placeholder slots when bundled/default slots are unusable andLANGCHAIN_MODELis set (but still not whenLANGCHAIN_SLOT_CONFIGis explicitly set). - Update test expectations and scenarios to cover the restored bootstrap behavior and the empty
qualitymapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/llm_registry.py | Restores env-model bootstrap on empty bundled slot resolution; sets quality={} on loaded registry entries. |
| templates/consumer-repo/tools/llm_registry.py | Mirrors the same registry + slot bootstrap behavior for consumer template sync. |
| tests/tools/test_llm_registry_selection.py | Updates tests to expect quality == {} and to assert bundled-slot unusable case bootstraps env model. |
| tests/tools/test_langchain_client.py | Updates registry-loading tests to expect quality == {} for malformed/legacy inputs. |
| # Preserve an explicit runtime override as an emergency bootstrap when the | ||
| # registry file is unavailable. Empty models are never invoked directly; | ||
| # langchain_client skips them when the override cannot serve that provider. |
| # Preserve an explicit runtime override as an emergency bootstrap when the | ||
| # registry file is unavailable. Empty models are never invoked directly; | ||
| # langchain_client skips them when the override cannot serve that provider. |
Automated Status SummaryHead SHA: 6e67d37
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
|
Restores the documented emergency LANGCHAIN_MODEL bootstrap when bundled slots yield no usable entries, while retaining fail-closed behavior for explicit LANGCHAIN_SLOT_CONFIG. Loaded registry entries again expose an empty compatibility quality mapping.\n\nValidation:\n- python -m pytest tests/tools/test_langchain_client.py tests/tools/test_llm_registry_selection.py -q\n- python scripts/validate_template_sync.py
Summary by CodeRabbit