Repository navigation
Use engine-compatible endpoints for routed models - #66956
Conversation
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #66956: no 'implementation' label (has_implementation_label=false) and 0 new lines in default business logic directories (threshold 100, no custom .design-gate.yml).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
There was a problem hiding this comment.
🟡 Changes recommended
Incorrect AWF reflection metadata paths cause endpoint overrides and routed Pi runs to fail closed.
2 open findings
What changed in this PR
Updates non-Copilot model routing to select engine-compatible endpoints from AWF metadata.
Changes:
- Adds endpoint override resolution for Claude, Codex, and Pi.
- Preserves AWF-selected endpoints in diagnostics.
- Documents endpoint validation and effort-routing limitations.
| File | Description |
|---|---|
docs/src/content/docs/reference/model-routing.md |
Documents routing behavior and diagnostics. |
docs/adr/66723-extend-model-routing-to-claude-codex-pi.md |
Updates the routing decision record. |
actions/setup/js/awf_model_routing.cjs |
Resolves engine-compatible endpoints. |
actions/setup/js/awf_model_routing.test.cjs |
Tests shared endpoint resolution. |
actions/setup/js/claude_harness.cjs |
Applies and logs Claude endpoints. |
actions/setup/js/claude_harness.test.cjs |
Tests Claude endpoint overrides. |
actions/setup/js/codex_harness.cjs |
Applies and logs Codex endpoints. |
actions/setup/js/codex_harness.test.cjs |
Tests Codex endpoint overrides. |
actions/setup/js/pi_models_json.cjs |
Resolves and validates Pi APIs. |
actions/setup/js/pi_models_json.test.cjs |
Tests Pi API routing behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
The new endpoint-override path is still not safe when the routed model is advertised by more than one configured Copilot endpoint.
Blocking theme
- Endpoint compatibility is now verified via a helper that picks the first matching
routing_modelsentry for the selected model, not the specific reflected endpoint that produced the routing decision. That makes the override logic nondeterministic across multi-endpoint Copilot configurations and can turn a valid route into either a false failure or a misrouted request.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 42.1 AIC · ⌖ 5.41 AIC · ⊞ 19.8K
Comment /review to run again
|
@copilot Tested this PR at head Cause: This is the actual {
"models_fetch_complete": true,
"endpoints": [{
"provider": "copilot", "configured": true,
"routing_models": [
{"model_id": "claude-opus-5", "source": "provider", "supported_endpoints": ["/v1/messages", "/chat/completions"], "supported_reasoning_efforts": ["low","medium","high","xhigh","max"], "context_window_tokens": 1000000, "candidate_metadata_complete": true},
{"model_id": "claude-haiku-4.5", "source": "provider", "supported_endpoints": ["/chat/completions", "/v1/messages"], "supported_reasoning_efforts": [], "context_window_tokens": 200000, "candidate_metadata_complete": true}
]
}],
"routing": {"status": "selected", "selection": {"…": "…"}}
}Please read |
There was a problem hiding this comment.
Impeccable Review — harden + audit (bug_fix: endpoint-routing correctness)
Traced the new endpoint-compatibility logic end to end (awf_model_routing.cjs, claude_harness.cjs, codex_harness.cjs, pi_models_json.cjs) and smoke-tested resolveAWFModelRoutingSelection, getAWFRoutingModel, resolvePiApiForModel, and resolvePiRoutingEndpoint directly in Node against the scenarios the PR's new tests cover (compatible-endpoint override, incomplete metadata, no-compatible-endpoint, Pi catalog-vs-reflect wire_api fallback).
Findings:
- Fail-closed behavior is correct in all paths: incomplete
candidate_metadata_complete/supported_endpointsand no-compatible-endpoint cases both return explicit errors instead of silently picking an endpoint. selected_endpointvsendpointis threaded consistently through Claude, Codex, and Pi logging, matching the documented log-line examples indocs/src/content/docs/reference/model-routing.md.- ADR and reference docs were updated to reflect the new "AWF selection is advisory for non-Copilot engines" model, and the new troubleshooting table rows match the actual thrown error strings (
advertises endpoints,candidate metadata is incomplete). - Test coverage (
awf_model_routing.test.cjs,claude_harness.test.cjs,codex_harness.test.cjs,pi_models_json.test.cjs) exercises both the happy-path override and all fail-closed branches added inpi_models_json.cjs/awf_model_routing.cjs.
No blocking issues found. Sandbox network restrictions prevented running npm ci/vitest directly, so correctness was verified by isolated Node smoke tests of the exported functions against the PR's own fixtures — results matched the new test expectations in the diff.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 76.5 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
Fixed in |
|
@copilot Verified
I couldn't test the Claude engine end to end yet. It still fails AWF's pre-flight on |
|
@copilot Please address the open review thread on
|
|
@copilot /pr-finisher For the unresolved |
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
The ambiguity guard and regression tests are on |
|
Ready for review. Verified in the private sandbox (pi engine, routing, AWF v0.28.44):
All review threads are resolved; I resolved the last one after the guard landed in |
|
🎉 This pull request is included in a new release. Release: |
AWF may select an endpoint incompatible with Claude, Codex, or pi, causing routed runs to fail before inference. Non-Copilot engines need to verify the selected model’s supported APIs and use their own compatible endpoint.
/reflectrouting metadata and use the first compatible engine endpoint. Preserve AWF’s original selection inselected_endpoint; fail closed when metadata is incomplete or there is no compatible endpoint./reflectbefore writingmodels.json.AWF still ranks efforts without engine-specific filtering, so unsupported efforts continue to fail closed pending an upstream AWF capability.