Repository navigation
Conversation
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently propagates serving identity and includes comprehensive regression coverage for pricing, fallback, concurrency, telemetry, and UI behavior.
0 open findings
What changed in this PR
Adds request-bound serving-model metadata so routed turns are priced and attributed to the provider that successfully served them.
Changes:
- Propagates serving identity and configuration through routed and instrumented streams.
- Uses serving-model pricing, endpoint, cache rates, and service tier throughout runtime accounting.
- Adds routing, fallback, concurrency, telemetry, and TUI regression coverage.
| File | Description |
|---|---|
pkg/runtime/streaming.go |
Carries serving metadata in stream results. |
pkg/runtime/routing_pricing_test.go |
Tests routed pricing and fallback attribution. |
pkg/runtime/loop.go |
Prices and attributes turns using serving metadata. |
pkg/runtime/fallback.go |
Captures serving metadata and emits identity updates. |
pkg/model/provider/serving_test.go |
Tests metadata propagation through the registry. |
pkg/model/provider/rulebased/serving_test.go |
Tests router serving metadata behavior. |
pkg/model/provider/rulebased/client.go |
Wraps routed streams with serving identity and configuration. |
pkg/model/provider/rulebased/client_test.go |
Adapts selection tests to the updated API. |
pkg/model/provider/rulebased/client_race_test.go |
Tests request-bound identity under concurrency. |
pkg/model/provider/provider.go |
Exposes serving metadata contracts. |
pkg/model/provider/instrument.go |
Preserves metadata through telemetry wrapping. |
pkg/model/provider/contracts/contracts.go |
Defines serving-model and stream contracts. |
pkg/leantui/serving_model_test.go |
Tests fallback model display reconciliation. |
docs/configuration/routing/index.md |
Documents routed cost and attribution semantics. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment was marked as resolved.
This comment was marked as resolved.
Checked the current PR commit list and each commit's GitHub verification record. All five commits already report
The referenced |
|
please review @dgageot |
aheritier
left a comment
There was a problem hiding this comment.
No blocking findings. The request-bound serving metadata improves on mutable last-selection state, and the pricing changes preserve explicit/free overrides, cache rates, endpoint policy, and fallback cooldown behavior. Linked issue #4576 is addressed with regression coverage for serving-model pricing, routing, and fallback. No demonstrated security or material performance regression was found.
One nonblocking attribution inconsistency is noted inline. The required CI gate and reported build, lint, test, and CodeQL checks passed; the race-test job was skipped. No local tests or benchmarks were run. GitHub reports mergeable_state=blocked, so this is not a merge-ready declaration.
|
|
||
| idleTimeout := streamIdleTimeoutFor(modelEntry.provider) | ||
| res, err := handleStream(streamCtx, streamCancel, stream, a, attemptTools, sess, events, idleTimeout) | ||
| res.Serving = servingModelFrom(stream) |
There was a problem hiding this comment.
[should-fix] Use the serving identity for the fallback span's final-model attribute too.
This assignment captures the routed leaf, but the success branches at lines 438 and 456 still call fbSpan.SetFinalModel(modelEntry.provider.ID().Model). For openai/gpt-4o-mini routing to anthropic/claude-haiku-4-5, cagent.fallback.final_model therefore remains gpt-4o-mini, while messages, hooks, and token-usage accounting identify the serving leaf.
Use res.Serving.ID.Model when metadata is present, falling back to the dispatched provider's model, in both success branches. Keep retry/cooldown keys attached to the dispatched provider. Add an exported-span assertion to a routed regression test; the current recordingTelemetry checks cover token usage but not this OTel attribute.
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Fixed in 0404f5c. Both normal success and delegated idle-retry success now use the successful stream's serving model for cagent.fallback.final_model, falling back to the dispatched provider for absent/empty model metadata. The attribute remains a bare model name; dispatched retry/cooldown ownership is unchanged.
Added six serial, exported-OTel-span regression cases for routed/plain success, same identity, idle-retry routed/plain success, and empty metadata, including attempts/outcome/primary attributes. Both routed cases fail against the prior implementation and pass with the fix.
Validation passed: task build, task test, task lint, full task test-race, focused race tests, and runtime/genai race + shuffle. Fresh independent review approved the exact pushed commit with no findings. Local and remote branch heads match; GitHub reports the new commit's signature valid. New-head CI is left to the normal checks.
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Fixes #4576.
Summary
cost:overrides, cache-read/write rates and actual response service tier, including same-ID routing and resolved custom endpoints.Validation
task build,task test,task lint,task test-race— pass.go test -count=1 ./pkg/model/provider/ ./pkg/model/provider/rulebased/ ./pkg/runtime/ -run 'TestServingModel|TestLastSelectedModelID_Concurrent|TestRouted|TestFallbackPricing|TestRunStreamServiceTierCost'— pass.go test -race -count=2 ./pkg/model/provider/... ./pkg/runtime/ ./pkg/leantui/ -run 'TestServingModel|TestLastSelectedModelID_Concurrent|TestRouted|TestInstrument'— pass.go test -race -shuffle=on ./pkg/runtime/ ./pkg/model/provider/... ./pkg/leantui/— pass.New tests use local fake providers and a fixed catalogue. They check attribution and matching costs across hooks, session messages, usage, budgets and telemetry, plus free/unpriced models, endpoint policy, no-match routing and stale selections after successful outer fallback. The pricing regressions fail against the unmodified base; the no-match and outer-fallback controls pass there.
No live/paid inference or SDK changes. This addresses accounting, not a demonstrated concurrent accounting defect. The pre-existing no-match provider construction behavior is not changed. Serving metadata is available at stream creation and stable after close; fallback identity notifications now arrive at stream open.