perf(providers): provider capabilities and default turn-session adapter (PR7) - #720
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds per-run turn-session abstractions and default provider adapters, projects model capabilities, routes agent-loop I/O through sessions, and reopens sessions during model escalation with lifecycle and compatibility tests. ChangesTurn session execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRun
participant TurnSessionProvider
participant TurnSession
participant ModelSessionSwitcher
AgentRun->>TurnSessionProvider: Open initial turn session
AgentRun->>TurnSession: Prewarm and stream completions
AgentRun->>ModelSessionSwitcher: Request target model session
ModelSessionSwitcher-->>AgentRun: Return switched session provider
AgentRun->>TurnSession: Close previous session
AgentRun->>TurnSession: Stream subsequent completions
Possibly related PRs
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.
Actionable comments posted: 2
🤖 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 `@internal/agent/loop.go`:
- Around line 748-768: The model-switch path in internal/agent/loop.go lines
748-768 must preserve the configured optimized TurnSessionProvider instead of
always wrapping the switched Provider with the default adapter; update the
ModelSwitcher contract or add a target-aware session resolver that returns the
appropriate provider/model session, while retaining prewarm, replacement, and
close behavior. In internal/agent/turn_session_test.go lines 190-215, add a
distinct switched session and assert it receives streams, is prewarmed, and is
closed exactly once.
In `@internal/providers/factory.go`:
- Around line 151-153: Update the capability construction loop in the provider
factory to populate caps.ReasoningEfforts from
Registry.ReasoningEfforts(strings.TrimSpace(profile.Model)) instead of
entry.ReasoningEfforts, preserving the string conversion and append behavior.
Add a fixture covering a registered model whose effective reasoning efforts
differ from the raw catalog entry.
🪄 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: CHILL
Plan: Pro
Run ID: 4ffc9921-f6f5-4099-b488-d975930776a9
📒 Files selected for processing (8)
internal/agent/loop.gointernal/agent/turn_session.gointernal/agent/turn_session_test.gointernal/agent/types.gointernal/providers/factory.gointernal/providers/factory_turn_session_test.gointernal/zeroruntime/session.gointernal/zeroruntime/session_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
Addressed the review in
CI is green on the head with the fix. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed the latest head (96ac868). PR7's required seam is implemented cleanly: every run routes provider I/O through a TurnSession without provider-specific loop branching; the default adapter preserves current behavior; lifecycle failures, prewarm/close behavior, capability projection, and model switching are covered by focused tests. The effective reasoning-effort projection fix is correct.\n\nValidated locally: git diff --check; go test ./internal/providers ./internal/zeroruntime ./internal/agent; and the broader agent/CLI/TUI suites on the preceding code commit. No blocking findings.\n\nNon-blocking PR8 follow-up: when an optimized session exists, extend the ModelSwitcher contract/session resolver so escalation preserves optimized session semantics instead of falling back to the default adapter, with lifecycle coverage for the switched session.
|
@coderabbitai review |
✅ Action performedReview finished.
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed the latest head (96ac868). The provider turn-session seam routes run I/O through the session while the default adapter preserves existing behavior. The reasoning-effort capability projection now uses Registry.ReasoningEfforts and includes coverage for inferred effective tiers. Focused lifecycle/provider validation and all CI checks pass. No blocking findings.\n\nNon-blocking PR8 follow-up: preserve optimized TurnSessionProvider semantics across ModelSwitcher escalation instead of falling back to the default adapter, with lifecycle coverage for the switched session.
|
Took the second suggestion after all —
The full escalation suite passes unchanged, so the fallback path is behavior-identical. CI running on the new head. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/turn_session_test.go (1)
283-321: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover failure while opening the switched session.
This tests a
ModelSessionSwitchercallback error, but not the distinct branch where it returns a provider whoseOpenTurnSessionfails. Add coverage verifying the original session remains active and unclosed until teardown, and that the failure note reaches the next request.🤖 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 `@internal/agent/turn_session_test.go` around lines 283 - 321, Add a test alongside TestRunModelSessionSwitcherErrorIsNonFatal that makes ModelSessionSwitcher return a provider whose OpenTurnSession fails. Verify Run remains non-fatal, continues using the original session for the recovery turn, closes that session only during teardown, and includes the switch-failure note in the next request.
🤖 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.
Nitpick comments:
In `@internal/agent/turn_session_test.go`:
- Around line 283-321: Add a test alongside
TestRunModelSessionSwitcherErrorIsNonFatal that makes ModelSessionSwitcher
return a provider whose OpenTurnSession fails. Verify Run remains non-fatal,
continues using the original session for the recovery turn, closes that session
only during teardown, and includes the switch-failure note in the next request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e438824a-68af-4878-8cb4-f4c5e2c0e217
📒 Files selected for processing (3)
internal/agent/loop.gointernal/agent/turn_session_test.gointernal/agent/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/loop.go
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed at 39d0136. No blocking findings. The optimized turn-session handoff and swap-time open-failure handling are sound, with focused regression coverage. Local race tests, vet, build, formatting, and static lint passed; all GitHub checks are green.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewed only the commits added since the dismissed approval, through head 39d0136. The new ModelSessionSwitcher preserves optimized turn-session semantics across escalation, and the added error/open-failure paths correctly retain and eventually close the original session. Focused race tests and vet pass for internal/agent, internal/providers, and internal/zeroruntime. No blocking findings.
3 participants
Summary
Implements PR7 of the Zero Agent Performance Program: provider capabilities and the default turn-session adapter — the architecture seam that PR8 (one optimized provider session) and a future native-compaction session plug into without touching the agent loop again.
Added
zeroruntime.TurnSession— one provider conversation per run:Prewarm(best-effort, no-op by default),Stream(signature-identical toProvider.StreamCompletion),Compact(returnsErrCompactionUnsupportedon the default adapter; dormant until a native-compaction session exists),Close(idempotent no-op by default).zeroruntime.TurnSessionProvider— opens a session for a run and exposesProviderCapabilities.zeroruntime.ProviderCapabilities— a flat projection of the resolved model's static capabilities (context window, max output tokens, vision/reasoning/prompt-cache flags, reasoning efforts). Deliberately plain data:modelregistryimportszeroruntime, so typed registry fields here would form an import cycle.providers.NewTurnSessionProviderdoes the projection instead.providers.NewTurnSessionProvider— builds the default adapter for every current provider kind (openai, openai-compatible, anthropic, anthropic-compatible, google, and the chatgpt path viaNew), populating capabilities from the same registry entryNewalready resolves.agent.Runopens a session at run start (before session hooks, so a failed open is a clean run-start error with nothing to unwind), routes all provider I/O through it via a one-lineProvidershim — per-turn streams, mid-stream retries, the compaction summarizer, and the final-answer request — and closes it at teardown. A mid-run model switch closes the old session and opens a fresh one over the escalated provider.Options.TurnSessionProvider(optional): nil keeps the default wrapper over the passed provider, so every existing caller is unaffected — no signature changes anywhere.No performance change expected
This PR is a pure extension point. The default session's
Streamisprovider.StreamCompletion;Prewarm/Closeare no-ops; no extra network round-trips, no hot-path allocation beyond a value-typed shim. Behavior identity is asserted by tests, not claimed: the same scripted run with the default path and an explicit adapter produces identical results and request counts.Safety properties
Preserves permissions, sandboxing, secret scrubbing, output ceilings, transcript ordering, exactly one result per tool call, reconnect/stall-retry behavior (
streamWithReconnectis unchanged and still wraps every stream), and cancellation semantics. ATurnSession.Closefailure never fails the run; aPrewarmfailure is ignored by contract.Scope boundaries
No connection reuse, prewarm implementation, request-prefix fingerprint reuse, provider-specific sessions, or native compaction — those are PR8 and later, behind this seam. The
ModelSwitchercontract still returns a bareProvider, so an optimized session is not carried across a mid-run swap (documented next to the existing context-window limitation; fixing both needs the same switcher contract change).Verification
go build ./...,go vet ./...,gofmt -lcleango test ./internal/zeroruntime/ ./internal/providers/... ./internal/agent/ ./internal/cli/ ./internal/tui/greenPrewarm/Closeno-op + idempotent,Compactunsupported; fault injection — a failedOpenTurnSessionsurfaces as a wrapped run-start error with zero provider requests, a failingCloseis swallowed; every request of a run flows through the session (prewarmed once, closed once); the default path is behavior-identical to an explicit adapter; a mid-run model switch closes the old session exactly once and continues on the new provider; every provider kind wraps; registry capabilities project correctly.Summary by CodeRabbit
TurnSessionProviderandModelSessionSwitcheroptions for target-aware mid-run escalation.