feat(llm): add evidence-gated model evaluation pipeline (#2710) - #2740
Merged
Conversation
📝 WalkthroughWalkthroughThe PR replaces model-quality ranking with explicit profile-based registry selections, adds policy-driven freshness and catalog-drift checks, introduces paired benchmark evaluation and a manual pilot workflow, updates provider resolution, and adds GitHub pull-request API helpers. ChangesModel governance and runtime selection
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 86f893c3de
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the model-update redesign from #2710 by (a) moving slot “currency” to the registry via tier-based slots and (b) adding a configurable cost-efficiency floor so tier selection doesn’t always pick the priciest max-quality model.
Changes:
- Added
cost_scoretoModelRegistryEntryand introducedLANGCHAIN_MIN_COST_SCORE/DEFAULT_MIN_COST_SCOREwith floor+relax selection logic inselect_model_for_tier. - Converted
config/llm_slots.jsonfrom model-pinned slots to tier-based slots (quality_tier). - Added focused unit tests for cost-floor behavior and updated the Anthropic fallback expectation in the LangChain client test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tools/llm_registry.py | Adds cost-efficiency metadata + floor-based tier selection; retains last-resort defaults. |
| config/llm_slots.json | Migrates slots from model pins to tier-based selection (quality_tier). |
| tests/tools/test_llm_registry_cost_floor.py | New unit coverage for cost-floor parsing/selection + shipped-config assertions. |
| tests/tools/test_langchain_client.py | Updates Anthropic fallback expectation to match default cost-floor behavior. |
Comments suppressed due to low confidence (1)
tools/llm_registry.py:278
- Issue #2710’s acceptance criteria calls for removing hardcoded model-version strings outside the registry/slots configs, but
default_slots()still hardcodesgpt-5.4andclaude-sonnet-4-6(andtools/llm_provider.pystill has DEFAULT_*_ANALYSIS_MODEL). That means the “no-config” / fallback path can still ossify on these strings over time; either derive these defaults from the registry too, or explicitly de-scope/update the acceptance criteria before closing the issue.
def default_slots(*, github_default_model: str) -> list[SlotDefinition]:
# LAST-RESORT only (used when config/llm_slots.json is absent). Values are
# kept consistent with the registry's cost-floored tier picks
# (DEFAULT_MIN_COST_SCORE): openai->gpt-5.4, anthropic->claude-sonnet-4-6.
# The canonical, auto-updating path is tier-based slots in llm_slots.json.
return [
SlotDefinition(name="slot1", provider=PROVIDER_OPENAI, model="gpt-5.4"),
SlotDefinition(name="slot2", provider=PROVIDER_ANTHROPIC, model="claude-sonnet-4-6"),
SlotDefinition(name="slot3", provider=PROVIDER_GITHUB, model=github_default_model),
Contributor
Automated Status SummaryHead SHA: 7a9e289
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeThe canonical LLM model configuration ossifies: old models stay primary indefinitely with no mechanism to refresh them. The config is data-driven but several things defeat that:
Increment 1 (detection) is already up: PR #2709 adds Context for AgentRelated Issues/PRsTasks
Acceptance criteria
|
Stops primary models ossifying: all slots in config/llm_slots.json are now tier-based (provider + quality_tier, no pinned model), so model currency comes from the registry (copy-synced to consumers via maint-68) instead of frozen pins. Adds a cost floor so selection does not always grab the priciest model: - ModelRegistryEntry now carries cost_score (cost-EFFICIENCY; higher = cheaper). - select_model_for_tier(min_cost_score=...) excludes models below the floor; env LANGCHAIN_MIN_COST_SCORE (default 0.12), 0 = pure max-quality. A missing cost_score never excludes; if the floor would drop everything it relaxes (availability beats the preference). Verified effect against the shipped registry: openai->gpt-5.4 and github->codex-mini-latest unchanged; anthropic default moves opus-4-6 (0.98q, priciest) -> sonnet-4-6 (0.95q, ~4x cheaper). default_slots()/llm_provider.py last-resort constants already match these picks. ruff/black/mypy (CI-pinned) clean; 293 tools tests pass. Updated test_build_chat_client_anthropic_fallback to the cost-floored expectation. Freshness gate (maint-77) unaffected (tier-based slots skip pin checks). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
stranske
force-pushed
the
model-registry-cost-floor-tier-slots
branch
from
July 13, 2026 01:55
86f893c to
7d01e86
Compare
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 01:56 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
stranske
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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)
361-375: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject provider-only slot overrides.
A
LANGCHAIN_SLOT<n>_PROVIDERoverride changes the provider while retaining the old slot model whenLANGCHAIN_SLOT<n>_MODELis unset. For example, changing Anthropic to OpenAI can send aclaude-*model ID to OpenAI. Require a paired model override, or resolve a reviewed model for the replacement provider.Suggested guard
if idx == 1: model_override = model_override or os.environ.get(env_model_name) + if provider_override and not (model_override or "").strip(): + logger.warning( + "Ignoring provider-only LLM slot override; supply a matching model override" + ) + updated.append(slot) + continue provider = provider_override or slot.providerAs 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/llm_registry.py` around lines 361 - 375, Update the slot override logic around provider_override, model_override, and the SlotDefinition construction to reject provider-only overrides instead of combining the new provider with slot.model. Require a matching model override, or resolve an approved provider-specific model before appending the slot; preserve existing slots when the override is invalid and add tests covering provider-only changes such as Anthropic to OpenAI.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/workflows/maint-77-model-registry-freshness.yml:
- Around line 103-105: Update the “Upload catalog discovery” workflow step to
replace the mutable actions/upload-artifact@v7 reference with the approved full
commit SHA, preserving the existing step conditions and behavior.
In @.github/workflows/maint-78-model-evaluation-pilot.yml:
- Around line 16-19: Update the workflow’s actions/checkout, astral-sh/setup-uv,
and actions/upload-artifact uses to approved immutable 40-character release
commit SHAs instead of mutable version tags, while preserving their existing
configuration and behavior.
- Line 22: Update the workflow’s GH_TOKEN configuration to use a GitHub App
installation token or dedicated read token with access to every repository
listed in config/model_eval_pilot.json, instead of the repository-scoped
github.token. Ensure the replacement secret or token is configured for the
workflow and retains permission to publish the pilot results.
In `@docs/MODEL_SELECTION_FRAMEWORK.md`:
- Around line 3-7: Remove the blank blockquote line in the introductory
historical-design notice of MODEL_SELECTION_FRAMEWORK.md, or prefix it with “>”
only if the following content continues the same blockquote, so the blockquote
remains markdownlint MD028-compliant.
In `@tests/test_check_model_registry_freshness.py`:
- Around line 85-188: Extend the tests around gate.evaluate to cover each
untested finding branch: missing_source, missing_pricing_date,
invalid_selection_status, unknown_profile, inactive_selection, missing_evidence,
duplicate_selection, and slot unknown_pin, blocked_pin, and missing_profile
cases, asserting the corresponding finding kinds. Update test_main_exit_codes to
include a valid input that produces findings and assert exit code 1, while
preserving the existing success and invalid-date assertions.
In `@tests/tools/test_model_eval_pilot.py`:
- Around line 10-24: The test currently validates only corpus shape, so its
contents can change unnoticed. Update the assertions in the corpus validation
test to require the fixed corpus_version and verify a canonical mapping or
normalized-content digest covering each case’s identity, verdict, and category,
while preserving the existing structural checks.
In `@tests/tools/test_run_model_eval_pilot.py`:
- Around line 10-11: Apply Black formatting to the inline case dictionaries in
the test data, reflowing each dictionary across multiple lines as required by
the configured line-length rules while preserving all keys and values.
In `@tools/evaluate_model_benchmark.py`:
- Around line 96-103: Update _validate_paired_cases to verify each candidate is
a dictionary before calling candidate.get. Raise the same intended ValueError
configuration error for non-object candidates, preserving the existing
cases-list and case-ID validation for valid candidate objects.
- Around line 70-71: Validate the cost and latency values accumulated in the
benchmark evaluation flow before adding them to totals or ranking data: reject
non-finite and negative values parsed from each case, including JSON NaN and
Infinity. Update the relevant metric-processing logic in the benchmark evaluator
and add a regression test covering invalid cost or latency input.
In `@tools/llm_registry.py`:
- Around line 273-285: Update the slot model resolution around
select_model_for_profile and is_model_blocked so an explicit slot-level model
cannot bypass the reviewed selection for slot_profile; use the reviewed profile
decision, or fail closed when the pin mismatches it. Apply this consistently to
the related resolution path around the referenced logic, and add a regression
test covering legacy create-only slots with stale model pins.
In `@tools/run_model_eval_pilot.py`:
- Around line 70-82: Add a failure-path test for the evaluation flow surrounding
the exception handler that builds the result row, using a fetcher or evaluator
that raises. Assert the emitted row preserves the case and candidate metadata,
sets actual_verdict to NON_PASS and schema_valid to false, and records the
raised error.
- Around line 25-34: Replace the direct urllib requests in the pull request
metadata and diff retrieval flow with the repository-approved GitHub API
transport wrapper. Update both calls associated with the request and
diff_request variables, and remove the need for the current guard exceptions
while preserving their existing endpoints, headers, timeout behavior, and
response handling.
---
Outside diff comments:
In `@tools/llm_registry.py`:
- Around line 361-375: Update the slot override logic around provider_override,
model_override, and the SlotDefinition construction to reject provider-only
overrides instead of combining the new provider with slot.model. Require a
matching model override, or resolve an approved provider-specific model before
appending the slot; preserve existing slots when the override is invalid and add
tests covering provider-only changes such as Anthropic to OpenAI.
🪄 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: b4fd1ccc-2f26-4df2-86b0-70a7fe0f449b
📒 Files selected for processing (30)
.github/sync-manifest.yml.github/workflows/maint-77-model-registry-freshness.yml.github/workflows/maint-78-model-evaluation-pilot.ymlREADME.mdconfig/llm_slots.jsonconfig/model_eval_candidates.jsonconfig/model_eval_pilot.jsonconfig/model_registry.jsonconfig/model_selection_policy.jsondocs/MODEL_SELECTION_FRAMEWORK.mddocs/MODEL_SELECTION_POLICY.mddocs/ci/WORKFLOWS.mddocs/ci/WORKFLOW_SYSTEM.mdtemplates/consumer-repo/config/llm_slots.jsontests/test_check_model_registry_freshness.pytests/tools/test_discover_model_catalog.pytests/tools/test_evaluate_model_benchmark.pytests/tools/test_langchain_client.pytests/tools/test_llm_provider.pytests/tools/test_llm_registry_selection.pytests/tools/test_model_eval_pilot.pytests/tools/test_run_model_eval_pilot.pytests/workflows/test_workflow_naming.pytools/check_model_registry_freshness.pytools/discover_model_catalog.pytools/evaluate_model_benchmark.pytools/langchain_client.pytools/llm_provider.pytools/llm_registry.pytools/run_model_eval_pilot.py
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:05 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/api_client.py`:
- Around line 182-184: Add a test for fetch_pull_request that mocks
_request_json to return a non-dict payload, such as a list, and asserts that
RuntimeError is raised. Place it with the existing API client tests and preserve
the current guard behavior.
In `@templates/consumer-repo/tools/llm_provider.py`:
- Around line 38-51: Update each provider’s is_available() checks, including the
locations around the affected provider implementations, to require both valid
credentials and a non-empty resolved model from _configured_langchain_model()
(or the equivalent configured model source). Ensure forced-provider selection
cannot report availability when _get_client() would return None because no model
is configured, while preserving existing credential validation.
In `@templates/consumer-repo/tools/llm_registry.py`:
- Around line 295-305: Update default_slots() so each provider retains its fixed
slot identity even when select_model_for_profile() returns no model, preserving
placeholders or equivalent slot-index metadata for omitted providers. Ensure
apply_slot_env_overrides() uses the slot identity rather than list position, so
LANGCHAIN_SLOT1_* and LANGCHAIN_SLOT2_* continue targeting their intended
providers.
- Around line 275-291: Update the model resolution paths using
select_model_for_profile so an explicitly configured slot_profile or profile
that cannot resolve does not fall back to the default-profile model. Emit a
warning and leave that slot unresolved instead; preserve blocking checks for
successfully resolved models and only use fallback where no explicit profile was
configured.
🪄 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: 3364d7ed-9b8c-4c60-a7ac-be21dcb7269e
📒 Files selected for processing (10)
.github/workflows/maint-77-model-registry-freshness.yml.github/workflows/maint-78-model-evaluation-pilot.ymlscripts/api_client.pytemplates/consumer-repo/tools/llm_provider.pytemplates/consumer-repo/tools/llm_registry.pytests/scripts/test_api_client.pytests/tools/test_discover_model_catalog.pytests/tools/test_run_model_eval_pilot.pytools/discover_model_catalog.pytools/run_model_eval_pilot.py
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:11 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:14 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/tools/test_llm_registry_selection.py (2)
138-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSelect the lifecycle-test target by
model_id, not list position.
payload["models"][1]relies on the fixture’s current ordering. If that order changes, the test may mutate the wrong model and stop exercising the selected model’s lifecycle gate. Locatemodel-balancedby itsmodel_idbefore changing its lifecycle.As per path instructions,
**/*.py: Prioritize correctness, error handling, and test coverage.🤖 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 138 - 140, Update the lifecycle test setup to locate the model entry whose model_id is "model-balanced" instead of indexing payload["models"] by position, then set that entry’s lifecycle to "compatibility" before writing the registry. Preserve the existing JSON load and write behavior while ensuring the selected model is always mutated.Source: Path instructions
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the literal-removal check to consumer templates.
This test scans only
tools/llm_registry.pyandtools/llm_provider.py. Add the correspondingtemplates/consumer-repo/tools/copies so stale hardcoded model versions cannot remain in the canonical consumer template.As per coding guidelines,
templates/consumer-repo/**: Consumer repo templates live undertemplates/consumer-repo/and should be treated as the canonical consumer-facing template source.🤖 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 161 - 168, Extend test_runtime_helpers_contain_no_model_version_literals to also read and scan the corresponding tools/llm_registry.py and tools/llm_provider.py files under templates/consumer-repo/, preserving the existing forbidden literal assertions so stale model versions are rejected in both runtime helpers and canonical consumer templates.Source: Coding guidelines
tools/llm_provider.py (1)
651-655: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant model lookup in
getattrdefault.
getattr's third argument is always evaluated, so_configured_langchain_model("openai", …)runs (re-reading slot/registry config) on everyanalyze_completioneven though_get_client()has already setself._model_namebefore this point. Prefer reading the resolved value directly.Also applies to lines 739-745 (Anthropic).
♻️ Avoid the eager default call
- model_name=getattr( - self, - "_model_name", - _configured_langchain_model("openai", fallback=DEFAULT_OPENAI_ANALYSIS_MODEL), - ), + model_name=self._model_name,🤖 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_provider.py` around lines 651 - 655, Update the model_name lookups in analyze_completion and the corresponding Anthropic path to read the already-resolved self._model_name directly instead of using getattr with _configured_langchain_model as an eager default; preserve the existing resolved model value set by _get_client()..github/workflows/maint-78-model-evaluation-pilot.yml (1)
22-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when the cross-repository token is unavailable.
If
OWNER_PR_PATis unset or unusable,run_pilotcatches fetch failures and serializes them asschema_valid: falserows; this workflow can then still upload the resulting failure-only artifact. Add a token/read-access preflight before running the pilot.As per path instructions, this workflow is synced across consumer repos, so failed evidence should not be silently published fleet-wide.
🤖 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 @.github/workflows/maint-78-model-evaluation-pilot.yml around lines 22 - 27, Add a preflight step before the run_model_eval_pilot.py invocation that verifies OWNER_PR_PAT is present and usable for cross-repository reads, and fail the workflow immediately when validation fails. Keep the existing GH_TOKEN wiring and pilot command unchanged, ensuring failure-only results are not uploaded.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 `@templates/consumer-repo/scripts/api_client.py`:
- Around line 165-205: Add focused tests for fetch_pull_request and
fetch_pull_request_diff covering their distinct contracts: validate that
fetch_pull_request returns JSON objects and raises RuntimeError for non-object
responses, and verify fetch_pull_request_diff sends the diff Accept media type
and returns response.text. Use mocked shared request helpers and cover both
success and validation/error behavior.
In `@tests/tools/test_llm_registry_selection.py`:
- Around line 53-54: Update the fixture’s model assignment to check whether
model is not None rather than relying on truthiness, preserving explicitly empty
string values while still omitting absent model values.
In `@tools/evaluate_model_benchmark.py`:
- Around line 70-75: Update the metric extraction in the benchmark case
validation flow to require both total_cost_usd and latency_ms, rather than
defaulting missing values to zero. Normalize missing, null, and non-numeric
values into the existing invalid-metric ValueError messages, while preserving
finite and non-negative checks; add regression coverage for missing and null
cost and latency fields.
---
Outside diff comments:
In @.github/workflows/maint-78-model-evaluation-pilot.yml:
- Around line 22-27: Add a preflight step before the run_model_eval_pilot.py
invocation that verifies OWNER_PR_PAT is present and usable for cross-repository
reads, and fail the workflow immediately when validation fails. Keep the
existing GH_TOKEN wiring and pilot command unchanged, ensuring failure-only
results are not uploaded.
In `@tests/tools/test_llm_registry_selection.py`:
- Around line 138-140: Update the lifecycle test setup to locate the model entry
whose model_id is "model-balanced" instead of indexing payload["models"] by
position, then set that entry’s lifecycle to "compatibility" before writing the
registry. Preserve the existing JSON load and write behavior while ensuring the
selected model is always mutated.
- Around line 161-168: Extend
test_runtime_helpers_contain_no_model_version_literals to also read and scan the
corresponding tools/llm_registry.py and tools/llm_provider.py files under
templates/consumer-repo/, preserving the existing forbidden literal assertions
so stale model versions are rejected in both runtime helpers and canonical
consumer templates.
In `@tools/llm_provider.py`:
- Around line 651-655: Update the model_name lookups in analyze_completion and
the corresponding Anthropic path to read the already-resolved self._model_name
directly instead of using getattr with _configured_langchain_model as an eager
default; preserve the existing resolved model value set by _get_client().
🪄 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: 0036f769-e19a-413f-9a6f-c51429e19b78
📒 Files selected for processing (16)
.github/workflows/maint-78-model-evaluation-pilot.ymldocs/MODEL_SELECTION_FRAMEWORK.mdlangsmith-fleet-worker-attempt.jsontemplates/consumer-repo/scripts/api_client.pytemplates/consumer-repo/tools/llm_provider.pytemplates/consumer-repo/tools/llm_registry.pytests/scripts/test_api_client.pytests/test_check_model_registry_freshness.pytests/tools/test_evaluate_model_benchmark.pytests/tools/test_llm_provider.pytests/tools/test_llm_registry_selection.pytests/tools/test_model_eval_pilot.pytests/tools/test_run_model_eval_pilot.pytools/evaluate_model_benchmark.pytools/llm_provider.pytools/llm_registry.py
💤 Files with no reviewable changes (1)
- docs/MODEL_SELECTION_FRAMEWORK.md
Comment on lines
+165
to
+205
| def fetch_pull_request( | ||
| repo: str, | ||
| pull_number: int, | ||
| token: str, | ||
| *, | ||
| retry_attempts: int | None = None, | ||
| retry_backoff: float | None = None, | ||
| ) -> dict[str, Any]: | ||
| """Fetch pull-request metadata through the shared retrying API client.""" | ||
| url = f"{GITHUB_API}/repos/{repo}/pulls/{pull_number}" | ||
| data = _request_json( | ||
| "GET", | ||
| url, | ||
| token, | ||
| payload=None, | ||
| **_retry_kwargs(retry_attempts, retry_backoff), | ||
| ) | ||
| if not isinstance(data, dict): | ||
| raise RuntimeError("GitHub API did not return a JSON object for the pull request.") | ||
| return data | ||
|
|
||
|
|
||
| def fetch_pull_request_diff( | ||
| repo: str, | ||
| pull_number: int, | ||
| token: str, | ||
| *, | ||
| retry_attempts: int | None = None, | ||
| retry_backoff: float | None = None, | ||
| ) -> str: | ||
| """Fetch a pull-request diff through the shared retrying API client.""" | ||
| url = f"{GITHUB_API}/repos/{repo}/pulls/{pull_number}" | ||
| response = _request_response( | ||
| "GET", | ||
| url, | ||
| token, | ||
| payload=None, | ||
| accept="application/vnd.github.diff", | ||
| **_retry_kwargs(retry_attempts, retry_backoff), | ||
| ) | ||
| return response.text |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add direct tests for both pull-request helpers.
The supplied coverage verifies the default JSON Accept header, but not fetch_pull_request’s object validation or fetch_pull_request_diff’s diff media type and text response. These helpers feed tools/run_model_eval_pilot.py:20-44, so add focused tests for those contracts.
As per path instructions, **/*.py: Prioritize correctness, error handling, and test coverage.
🤖 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 `@templates/consumer-repo/scripts/api_client.py` around lines 165 - 205, Add
focused tests for fetch_pull_request and fetch_pull_request_diff covering their
distinct contracts: validate that fetch_pull_request returns JSON objects and
raises RuntimeError for non-object responses, and verify fetch_pull_request_diff
sends the diff Accept media type and returns response.text. Use mocked shared
request helpers and cover both success and validation/error behavior.
Source: Path instructions
Comment on lines
+53
to
+54
| if model: | ||
| slot["model"] = model |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve explicitly empty model values in the fixture.
Because model is typed as str | None, "" is distinct from None; if model: currently drops that value. Use if model is not None so tests can represent an explicit empty pin.
As per path instructions, **/*.py: Prioritize correctness, error handling, and test coverage.
🤖 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 53 - 54, Update the
fixture’s model assignment to check whether model is not None rather than
relying on truthiness, preserving explicitly empty string values while still
omitting absent model values.
Source: Path instructions
Comment on lines
+70
to
+75
| cost = float(case.get("total_cost_usd", 0.0)) | ||
| latency = float(case.get("latency_ms", 0.0)) | ||
| if not math.isfinite(cost) or cost < 0: | ||
| raise ValueError(f"case {case_id} has invalid total_cost_usd") | ||
| if not math.isfinite(latency) or latency < 0: | ||
| raise ValueError(f"case {case_id} has invalid latency_ms") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject missing benchmark metrics instead of treating them as zero.
case.get(..., 0.0) makes an artifact with missing cost or latency look free/instant and can incorrectly win recommendation ranking. Require both fields and normalize malformed values to the intended validation error; add regression coverage for missing and null metrics.
As per path instructions, prioritize correctness, error handling, and test coverage for Python changes. The PR objective also requires cost capture before model recommendation.
Proposed fix
- cost = float(case.get("total_cost_usd", 0.0))
- latency = float(case.get("latency_ms", 0.0))
+ if "total_cost_usd" not in case or "latency_ms" not in case:
+ raise ValueError(f"case {case_id} requires cost and latency metrics")
+ try:
+ cost = float(case["total_cost_usd"])
+ latency = float(case["latency_ms"])
+ except (TypeError, ValueError) as exc:
+ raise ValueError(f"case {case_id} has non-numeric metrics") from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cost = float(case.get("total_cost_usd", 0.0)) | |
| latency = float(case.get("latency_ms", 0.0)) | |
| if not math.isfinite(cost) or cost < 0: | |
| raise ValueError(f"case {case_id} has invalid total_cost_usd") | |
| if not math.isfinite(latency) or latency < 0: | |
| raise ValueError(f"case {case_id} has invalid latency_ms") | |
| if "total_cost_usd" not in case or "latency_ms" not in case: | |
| raise ValueError(f"case {case_id} requires cost and latency metrics") | |
| try: | |
| cost = float(case["total_cost_usd"]) | |
| latency = float(case["latency_ms"]) | |
| except (TypeError, ValueError) as exc: | |
| raise ValueError(f"case {case_id} has non-numeric metrics") from exc | |
| if not math.isfinite(cost) or cost < 0: | |
| raise ValueError(f"case {case_id} has invalid total_cost_usd") | |
| if not math.isfinite(latency) or latency < 0: | |
| raise ValueError(f"case {case_id} has invalid latency_ms") |
🤖 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/evaluate_model_benchmark.py` around lines 70 - 75, Update the metric
extraction in the benchmark case validation flow to require both total_cost_usd
and latency_ms, rather than defaulting missing values to zero. Normalize
missing, null, and non-numeric values into the existing invalid-metric
ValueError messages, while preserving finite and non-negative checks; add
regression coverage for missing and null cost and latency fields.
Source: Path instructions
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 02:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Jul 13, 2026
stranske
temporarily deployed
to
agent-high-privilege
July 13, 2026 03:22 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
13 tasks
13 tasks
This was referenced Jul 15, 2026
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2710
Decision
Do not choose replacement models from hand-authored quality or cost scores. Keep the current production models as provisional incumbents until a reproducible evaluation produces comparative evidence.
What this PR adds
Pilot corpus
The pilot contains 30 merged historical PRs across Workflows and supported consumers, spanning clean PASS cases, missing acceptance criteria, stale verifier claims, review-thread debt, and follow-up-required outcomes. Every referenced PR was checked live before publication.
The pilot is a screening stage only. It records verdict agreement, schema validity, confidence, latency, and errors. It cannot approve a model or migrate consumers. Approval requires the documented 75-case benchmark with cost evidence and category-level regression gates.
Runtime safety
Validation
Reviewer decisions requested
Scope boundary
This PR builds the evaluation and evidence gate. It does not claim that any challenger is better, does not change an incumbent based on unmeasured scores, and does not migrate consumer runtimes before evidence exists.
Automated Status Summary
Scope
The canonical LLM model configuration ossifies: old models stay primary indefinitely with no mechanism to refresh them. The config is data-driven but several things defeat that:
config/llm_slots.jsonpins explicit models (gpt-5.4,claude-sonnet-4-6,codex-mini-latest).tools/llm_registry.pyconfigured_model_for_provider()only calls the quality-basedselect_model_for_tier()when a slot has nomodel. So the registry's "pick the best model for this tier" logic is short-circuited by the pins.create_only..github/sync-manifest.yml:743marksconfig/llm_slots.jsonsync_mode: create_only(so consumers can customize provider preference) — but because slots also carry the model version, that version freezes on every consumer once seeded and never updates.config/model_registry.jsonis hand-curated (last_updated: 2026-04-14). New GA models only enter it when someone remembers. Its own data can already be self-contradictory: it listsclaude-opus-4-6(T5 0.98) yet slot2 pinsclaude-sonnet-4-6(0.95).tools/llm_registry.pydefault_slots()(gpt-5.4/claude-sonnet-4-6) andtools/llm_provider.py:43-44(DEFAULT_OPENAI_ANALYSIS_MODEL/DEFAULT_ANTHROPIC_ANALYSIS_MODEL) repeat model strings in code — a second place that drifts.Increment 1 (detection) is already up: PR #2709 adds
tools/check_model_registry_freshness.py+maint-77(flagsreview_overdue/blocked_pin/unknown_pin/dominated_pin; opens a tracking issue on staleness). This issue tracks the rest of the update system.Context for Agent
Related Issues/PRs
Tasks
config/llm_slots.jsonso each slot carriesprovider+quality_tier(no hardcodedmodel, or model as an explicit opt-in override only).configured_model_for_provider()/resolve_slots()already derive from the registry when a slot lacks a model — so currency then comes frommodel_registry.json(copy-synced) whilecreate_onlyslots safely carry only provider/tier preference. Verify the resolved models for each tier are sensible/current before merge (e.g. confirm the anthropic tier resolves to the intended model, since the registry currently ratesclaude-opus-4-6aboveclaude-sonnet-4-6).tools/llm_registry.pydefault_slots()andtools/llm_provider.py:43-44; derive defaults from the registry (keep one clearly-marked last-resort constant if a no-config bootstrap is required, and ensure the freshness gate covers it).maint-77ormaint-39-test-llm-providers) that queries the OpenAI/Anthropic model lists and flags GA models newer/higher than the registry's current best per provider, attaching the diff to the freshness tracking issue. Gated behind available secrets; offline gate (feat(llm): model-registry freshness gate (maint-77) — stop old models ossifying as primary #2709) remains the default.model_registry.json(+review_by) and merges; maint-68 propagates. Document this loop in the LLM/CI docs.Acceptance criteria
config/model_registry.json(e.g. raising a newer model's quality or adding a GA model) changes the resolved primary model for the affected tier without editingllm_slots.json— proven by a test that resolves slots before/after a registry edit.tools/llm_provider.py/tools/llm_registry.pyoutside the registry/slots configs (grep gate or test).maint-77freshness gate is green after the registry is refreshed andreview_bybumped.scripts/validate_template_sync.py; consumers receive registry updates on the next sync.review_bylapse) → the freshness gate flags it; revert → green.Head SHA: 1181153
Latest Runs: ✅ success — Gate
Required: gate: ✅ success
Summary by CodeRabbit