Conversation
|
This pull request targeted The base branch has been automatically changed to |
![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: 1c6c7af8c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Went through the review. Four of the five are real and I've fixed them. The Makefile one is real too but I'm leaving it alone; reasoning at the bottom. Session identity. Confirmed, and it's a bit worse than described. With no session header, count_tokens burning a slot. Also confirmed. I tried the obvious thing first (infer "this is a count call" from That does change the rule I wrote in the PR description: it's now "advance once per changed conversation state", not "advance on every selection". Retries and parallel calls with the same history land on the same provider on purpose, which the prompt cache absorbs. README updated to say so. Reset ordering. Yep. README pointing at scripts that don't exist. My fault, those live in my deployment tree and were never meant to be the documented path. Replaced with the plain module commands: mkdir -p ../../bin
go test -race ./...
go build -buildmode=c-shared -o ../../bin/model-sequence-router-go.so .The Makefile. The observation is correct: the matrix crosses every Two things I hit while doing this that I haven't touched, in case they're useful:
One thing I added beyond the review: a plugin-level Tests, |
An automated review of router-for-me#5163 reported 5 findings against the model-sequence-router plugin. 4 identify plugin-local defects, corrected below; the 5th concerns the shared plugin build matrix. Sequence ownership now follows the observed conversation state, so auxiliary calls and retries hold the selected slot while genuine turns continue through the configured order. - finding 1, a headerless conversation took a different identity on each of its first 2 turns: derive the identity from the system content and first history item, which hold constant for the life of a conversation, and report the identity origin on every route record. - finding 2, a token count consumed a sequence position: key selection on the observed history so a repeated state replays its slot, which keeps call classification out of plugin policy. - finding 3, reconfiguration published a generation before clearing state: clear cursor and lane state first so a concurrent route keeps its reservation. - finding 4, the README named absent build, launcher, and deployment assets: document the direct Go build, artifact deployment, and jq inspection of the diagnostics file. - finding 5, the plugin build matrix multiplies Go-only entries by every language: this stays shared repository scope and keeps the registration line already present there. - add the plugin-level on_unavailable setting, selecting between a forward scan that emits 1 notice per passed-over position and a held position answered with a retryable HTTP 529. - separate request inspection and plugin RPC into their own files, and derive the executor capability from the compiled policy so registration matches the configured behavior. - extend routing, cursor, configuration, identity, and RPC tests over replay and unavailability. - bump the reported plugin version to 0.9.0 for the changed advancement contract. Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
|
pushed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7fbc257a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63ae213b55
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Addressed both P1 findings in commit 4870a22.
Credential affinity still keeps its primary and fallback aliases in the authentication selector, where they select credentials within the provider and model already chosen by YAML. This preserves provider-local cache affinity without allowing cache identity to pin or replace provider order. The plugin race suite and focused identity, sequence, and cleanup regression tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebe94442c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
This plugin will be even more effective once #4731 and/or #5446 land. If you're alternating between models, typically five minutes is plenty of cash for the cash to stay hot. But if you're alternating across several different models across several different providers, it starts to spread out far enough that you might have cache misses. If the one hour cache management pull requests land, then everything will work just fine, even across more complicated round-robin models like the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2353d9f28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The shared examples/plugin/Makefile treated every example as buildable in Go, C, and Rust, but several examples (frontend-auth-exclusive, codex-service-tier, scheduler, host-callback-auth-files, host-model-callback, claude-web-search-router, and the new model-sequence-router) are Go-only, so the flat EXAMPLES list produced targets and list output for languages those examples cannot build. - split EXAMPLES into MULTI_LANGUAGE_EXAMPLES and GO_ONLY_EXAMPLES, deriving EXAMPLES from both so existing consumers keep working - limit the build and list targets to Go-only artifacts for GO_ONLY_EXAMPLES instead of iterating all three languages - make BIN_DIR relative instead of anchored to $(CURDIR) - document the new model-sequence-router Go-only example in the plugin README Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
The router previously gated cursor advancement on request operation (generate vs count_tokens), letting count_tokens requests peek at the current position without moving it. This split the cursor and observation stores' advance behavior across an operation-derived flag threaded through selectTarget and observe, adding a code path that diverged from the simpler contract of "each successful selection advances." - drop the operation/advance plumbing from routeWithCallback, selectTarget, and observe; every successful selection now advances and refreshes its cursor unconditionally - derive the logged "advanced" field from session presence instead of operation type, since stateless routing is the only case that does not move a cursor - drop ResponseModel from the route result; response model presentation stays with the host and upstream provider - add an explicit event=config discriminator to the startup/reload log record - update README to match the simplified advance semantics and log fields Fixes: router-for-me#4698 Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
…te split The router previously carried a peek-versus-generate distinction through the cursor store and only forwarded a caller thinking suffix verbatim to the selected slot model. This left no way for one rotation slot to answer a requested effort with a different model or a different level, and the extra peek/generate parameter shape gave no behavioral value since every code path advanced the cursor the same way. - add an `efforts` map to target configuration, refining a rotation slot per requested level with an optional model of the same provider and an optional effort, validated for known keys, the reserved `max` level, and non-decreasing effort across untiered-model tiers - resolve the emitted model through `compiledTarget.effectiveModel`, replacing the inline suffix-append in the router - drop the `selectTarget` peek/generate boolean; every selection advances the cursor, since no caller relied on a non-advancing read - fold the alias, sequence position, and requested effort into the debug log message, since a host chooses independently which structured fields to print - bump the plugin version to 0.7.0 and add the transitive `gorilla/websocket` dependency pulled in by the change - document the effort-tier resolution table, reserved `max` semantics, and the expanded debug log format in the README Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
An automated review of router-for-me#5163 reported 5 findings against the model-sequence-router plugin. 4 identify plugin-local defects, corrected below; the 5th concerns the shared plugin build matrix. Sequence ownership now follows the observed conversation state, so auxiliary calls and retries hold the selected slot while genuine turns continue through the configured order. - finding 1, a headerless conversation took a different identity on each of its first 2 turns: derive the identity from the system content and first history item, which hold constant for the life of a conversation, and report the identity origin on every route record. - finding 2, a token count consumed a sequence position: key selection on the observed history so a repeated state replays its slot, which keeps call classification out of plugin policy. - finding 3, reconfiguration published a generation before clearing state: clear cursor and lane state first so a concurrent route keeps its reservation. - finding 4, the README named absent build, launcher, and deployment assets: document the direct Go build, artifact deployment, and jq inspection of the diagnostics file. - finding 5, the plugin build matrix multiplies Go-only entries by every language: this stays shared repository scope and keeps the registration line already present there. - add the plugin-level on_unavailable setting, selecting between a forward scan that emits 1 notice per passed-over position and a held position answered with a retryable HTTP 529. - separate request inspection and plugin RPC into their own files, and derive the executor capability from the compiled policy so registration matches the configured behavior. - extend routing, cursor, configuration, identity, and RPC tests over replay and unavailability. - bump the reported plugin version to 0.9.0 for the changed advancement contract. Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
The plugin-level setting deciding what happens when the next sequence position names a provider the host has not registered was spelled as an event handler, and its second value named a status code rather than a behavior. Neither spelling matched the convention used by the other plugin examples, where settings are noun phrases naming their subject. The setting now names the subject the selection code actually consults, which is the provider availability set supplied by the host, and its second value names what the plugin does with an unavailable provider, which is to report an error. Routing behavior, the retryable status, and the envelope error code are unchanged. - rename the setting key to unavailable_provider - rename its second accepted value to error - rename the action constants and the compiled configuration field - restate the capability description and the README policy section - update the tests and documentation covering the policy Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
The plugin keyed its sequence cursor on the credential-affinity identity exported by core. That accessor returns only the primary value of a paired extraction, so a conversation that first carried conversation.id and later added prompt_cache_key changed key from conv: to pck: and restarted its sequence mid-conversation. The same ladder exposed transport and credential selection concerns to the routing layer. Cursor identity now comes from the shared protocol aware conversation derivation, which reads caller scope, leading instructions, and the first user input across every inbound format. Diagnostic lane snapshots separately accumulated until reconfiguration, because the periodic loop swept cursors alone. - Derive conversation identity through sdk/cliproxy/session, so no transport header, cache lane, affinity group, or account identifier reaches a cursor key - Collapse conversationIdentity to a string and compute its diagnostic source at use, because the origin is no longer independent state - Add an expiringStore contract so one periodic sweep releases both cursors and diagnostic lane observations under a single interval - Group the lane store methods beside their type and add size readouts that controlled-clock tests observe directly - Split router and state tests by purpose into cursor probe, observation, and identity files to keep each file within the module size limit - Share one conversation transcript fixture across tests so identity cases exercise the derivation rather than request headers - Raise the plugin version to 0.10.0 for the changed identity contract - Document YAML-first provider selection, credential-only affinity, and the content-derived cursor limitation - Tidy the plugin module graph after the credential-affinity import was dropped Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
Scalar input on the Responses API bypassed history comparison, so a repeated scalar prompt was treated as a new turn rather than a replay of the same conversation state. - fold scalar Responses `input` into the same user-message shape used by array input so scalar and array requests share one observed turn - add a test verifying scalar and array input replay identically before an extended transcript advances the cursor Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
…istory A single Interactions turn is valid as an object carrying steps, and the shared protocol derivation flattens that object into a conversation identity. Turn observation accepted only arrays and scalar strings, so such a request received a cursor while carrying no observed history. Identical retries then presented no matching turn and consumed the next sequence position, moving the conversation to another provider between attempts. - Recognize a non-empty object under input as one observed history item, so every encoding of one conversation state resolves to the same turn - Restructure the history converter as an exhaustive type switch, so absent keys, empty values, and unsupported shapes each reach a stated outcome - Cover object retry, single-element array equivalence, and later transcript advancement through route records - Share one route record assertion across the observation and identity tests, so expected outcome and position travel as one named record - Name the conversation identity contract version 0.11.0 Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
…hain Requests naming previous_response_id carry only their newest items while the provider holds the rest of the transcript, so deriving identity from body content alone assigned each such request a distinct conversation, breaking sequence routing and diagnostics for incremental continuation. - add continuationChainStore holding in-flight requests by request ID and provider responses by response ID, so a later request naming a response resolves the conversation that response belongs to - register request and stream-chunk interceptor capabilities and observe their payloads without modifying them - derive requestObservation.PreviousResponseID as trimmed text instead of a presence boolean, letting the router look up its bound conversation - add response_diagnostics.go logging replay-input and reasoning-item shapes at stream boundaries, hashing identifiers so no payload content reaches the journal - filter model_sequence_observation_test assertions to route-only records now that context diagnostics share the same log stream - bump plugin version to 0.16.0 Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
a2353d9 to
ae1cbdf
Compare
|
rebased and force-pushed |
Non-streamed responses reach the plugin only once, as a complete reply alongside the originating client request, so the earlier stream-chunk-only binding path left continuation turns on non-streaming providers unable to find their conversation. - add a response interceptor capability and RPC handler so the host delivers the complete reply and originating request together - factor bindResponseIdentity out of observeStreamChunk so both streamed and non-streamed paths share the same conversation-binding logic - add observeCompleteResponse to resolve the conversation from the originating request and bind the reply id to it in one call - bump plugin version to 0.17.0 - add tests covering conversation binding and sequence walking across non-streamed continuation turns Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
Context diagnostic records reached the journal whenever a context alias was requested, with no configuration flag to suppress them. This exposed reasoning-shape records by default instead of only on request. - add diagnostics.context config field, defaulting off, to gate context record emission - check cfg.Diagnostics.Context in observeRequestContext and observeResponseContext before emitting - thread compiledConfig into observeResponseContext so the stream-chunk path can read the flag - document the diagnostics.context field and its journal-only, no-path behavior in README - extract newContextDiagnosticPayloads helper so both gated and ungated tests share one payload set - add TestContextDiagnosticsStaySilentWhenUnrequested to prove silence when the flag is unset - bump plugin version to 0.18.0 Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
A conversation rotating between providers replays the reasoning one provider emitted into the next provider input. The receiving upstream rejects it, for example Codex answering 400 Unknown parameter: 'input[1].status' after a Claude turn, and the turn is lost. The router now records which lane produced each reasoning item and gives every target a projection of the transcript that keeps its own reasoning and withholds the reasoning of other lanes. All other items stay in place, so the answers of both providers stay interleaved and the cached prefix of each lane grows only at its end. - Record each finished reasoning item against the provider and base model that served it, so ownership rests on observed dispatch rather than on reading signatures. - Remove reasoning items owned by another lane or by no recorded lane after credential selection, because a foreign item fails the turn while a withheld one costs one cache miss. - Keep reasoning items that carry no identifier, since nothing shows where they came from. - Resolve the lane of a request through one mapping shared by recording and removal, and resolve none when zero or several providers serve the model, so a degenerate sequence attributes nothing. - Extend an ownership record on each lookup, so reasoning expires after its conversation idles for the session TTL rather than a fixed time after it was produced. - Return an edit failure as a hook error, so the host logs it and forwards the request unchanged. - Clear ownership records on reconfiguration and shutdown and sweep them beside cursors and chains, so memory tracks live conversations. - Share one history field order between request observation and rewriting, and one expiry check across stored entry types, so the new store reuses both. - Add the target lane, withheld and kept counts, and producing lane to the context diagnostics, so each replay shows the projection it received. - Promote gjson and sjson to direct requirements, since removal edits arrays in place. - Cover withholding with byte-identical survivors, return to the producing lane, each evidence state, renewal, and idle expiry through the plugin methods. - Document the projection and its lifetime, and bump the plugin version to 0.20.0. Signed-off-by: Eric Wheeler <cliproxy@z.ewheeler.org>
|
@codex review |
1 participant
Description
A model alias rotates a conversation across an ordered list of provider and model targets, so each turn is answered by a different model family and catches what the last one missed. Load spreads across providers, too.
Rotation needs every position at a comparable level, but an effort token means a different amount of reasoning on each family. For example, Claude Opus 5 at
mediummatches GPT‑5.6 Sol athigh, and Opus 5 athighmatches Sol atxhigh. Each slot therefore takes aneffortsmap that remaps a level, redirects it to another model of the same provider, or both. Unwritten levels pass through.Type of Change
Implementation Details
Routing
alias(high)andaliasreach one configuration entryEffort tiers
targets[].effortskeys a discrete level to an optionalmodelof the same slot provider and an optionaleffort; a bare level is shorthand for effort onlycompiledTarget.effectiveModelis the single producer of the emitted model string; a suffix written on the model outranks the tier effort, which outranks the requestnone,auto, and an absent suffix carry no level, match no tier, and pass through on the slot modelValidation during configuration compile
maxis reserved: a tier emitsmaxfor amaxrequest and for no other, so a model reaches its own ceiling exactly when askedWorked example
Nothing below is a default. The plugin ships no aliases, no models, and no effort tiers; every value here illustrates the mechanism, and the model names stand in for whatever a deployment has credentials for.
Illustrative intelligence scores
An example measurement set, used to derive the tiers that follow.
example alias: mist — entry tier, no remapping
Haiku 4.5 carries no effort ladder: one non-reasoning point at 24 and one reasoning point at 37, with nothing between. A tier map built from a two-point curve would collapse every sub-
maxrequest onto a single Luna setting and discard the requested level. Luna spans 33 atlowto 51 atmaxon its own, so passthrough preserves the caller intent that any remap here would erase.example alias: jade — balanced tier, no remapping
Terra tracks its Claude counterpart at the only level both publish: Terra
maxscores 55 against Sonnet 5maxat 53. No level shows Terra trailing, so a tier would raise cost without raising parity. Terrahighalready returns 49 for $495.77 against Terramediumat 46 for $240.23, making an upward remap here expensive for three points.example alias: opal — frontier tier, GPT slot retuned
Resolution for each requested level:
lowgpt-5.6-sol(low)claude-opus-5(low)mediumgpt-5.6-sol(high)claude-opus-5(medium)highgpt-5.6-sol(xhigh)claude-opus-5(high)xhighgpt-5.6-sol(xhigh)claude-opus-5(xhigh)maxgpt-5.6-sol(max)claude-opus-5(max)8000gpt-5.6-sol(8000)claude-opus-5(8000)gpt-5.6-solclaude-opus-5The Sol ladder rises steeply at the bottom and flattens at the top: 49, 54, 56, 58, 59 across
lowthroughmax. Two adjacent steps in the middle span two points each, so a request landing on Solmediumsits five points below the same request landing on Solxhigh, while the Claude model in the same rotation answers near its own ceiling at every label. Shiftingmediumandhighone rung up the Sol ladder closes that band without touching the ends.The ends stay unwritten deliberately.
lowpasses through because the 49 rung is the cheapest frontier entry and remapping it upward would nearly triple its cost.maxreaches Solmaxat 59 under the reservation rule.xhighneeds no entry because it already resolves to the 58 rung thathighwas raised to.Replacing the model for one effort
A tier may name a different model of the same provider instead of, or in addition to, an effort, which suits a model whose strength sits at one point of the ladder rather than across it. Two tiers on Opal's Claude slot substitute the prior generation, leaving the codex slot above as written:
lowclaude-opus-5(low)mediumclaude-opus-4-8(medium)modelandefforthighclaude-opus-4-8(high)modelonly, caller effort carried acrossmaxclaude-opus-5(max)example alias: ruby — maximum tier, both GPT slots retuned
Ruby interleaves five positions so a long conversation alternates providers four times before reaching Fable 5, whose
maxscore of 60 is the highest recorded and whose cost of $5630.52 is likewise the highest. Placing it last spreads four cheaper turns ahead of every expensive one.Both Sol slots carry identical tiers for the reason given under Opal, written out rather than shared through a YAML anchor because a management-panel rewrite of the configuration would not preserve an anchor. The Claude slots carry no tiers and answer at the requested level.
Reproduction
Requesting
opal(high)on successive turns of one conversation, taken from a running deployment:gpt-5.6-sol(xhigh)xhighclaude-opus-5(high)highTesting
go test -race ./...underexamples/plugin/model-sequence-router/gocovers cursor wrap, TTL expiry, random start, provider skipping, concurrent reservation, the four tier forms, and each rejection case listed above.