Apply versioned shared Agent model selections at dispatch - #591
LikiosSedo wants to merge 5 commits into
Conversation
jacoblee-io
left a comment
There was a problem hiding this comment.
Reviewed 6c8c291a. The premise is right — A → B → A within one release genuinely defeats a release+fingerprint receipt, and a monotonic selection counter is the correct shape for it. Resolving at dispatch rather than at request build is also the right move, and task-coordinator's version of it is clean: releaseTurn?.() is in a finally, and binding.persistence is still read before getOrCreate.
All four PromptOptions builders (lark, dingtalk, chat.send params, task-coordinator) forward the new field, so no prompt path was missed. output-redactor.js is mocked in the tiers suite, so the new redaction assertion is real. 161 tests pass across the five focused suites on this head.
Three findings I would want fixed before merge, two of which I measured.
The dispatch block re-resolves the model but leaves the prompt stale
The new block in chat.send overwrites eight fields and not systemPromptTemplate, and the fallback below it only fires when the caller sent nothing:
if (promptOpts.systemPromptTemplate === undefined) {
promptOpts.systemPromptTemplate = await resolveAgentSystemPrompt(agentId, frontendClient);
}The comment on that fallback says an explicit params.systemPrompt comes from "the portal-standalone path [which] stamps it from the agent's model binding" — i.e. the same snapshot the model came from. So when a save lands while the request waits on the turn lock and changes both, the turn runs the new model with the old prompt. Measured on this head:
>>> PROBE A modelId= model-b | systemPromptTemplate= OLD PROMPT (stamped by the caller)
That is the same torn read the PR exists to fix, in the same block. ResolvedModelBinding already carries systemPrompt, and task-coordinator.ts:317 already uses it (systemPromptTemplate: binding.systemPrompt ?? undefined), so this looks like a one-line omission rather than a decision.
A config.getModelBinding hiccup now kills the chat turn
if (!binding) throw new Error(...), and the await itself is unguarded. Measured with the RPC rejecting:
>>> PROBE B promptCalls= 0 | events= ["chat.event","chat.event"]
The turn never reaches the box. Before this change it would have run with the binding the caller already stamped — which is a perfectly good, at-most-slightly-stale fallback for precisely this failure. Falling back to promptOpts as-is on a lookup failure keeps the fix and drops the new availability coupling.
Related: this adds one control-plane RPC to every chat turn, to close a window that only exists when a turn actually queued behind another. The code already knows whether that happened — it re-acquires the lock after waiting — so gating the re-resolution on a contended lock would get the same correctness for a fraction of the traffic. Worth considering if getModelBinding is not cheap on the control-plane side.
The deployment ordering is fail-closed and unstated
preparedModelSelectionVersion = binding.modelSelectionVersion ?? 0 with an exact !==. So if the control plane starts sending modelSelectionVersion in agent.reload before config.getModelBinding returns it, every model reload fails. Measured against a binding without the field:
>>> PROBE C reload result: model selection version does not match expected Agent configuration
The PR body's "Callers without selection metadata retain their existing behavior" covers the caller-omits case, but not this asymmetry — the caller sending it while the binding does not yet carry it is fail-closed, not backward compatible. getModelBinding has to carry the field before agent.reload starts asserting it, and that ordering belongs in the deployment note.
agent.syncStatus will report a false inconsistency for the whole AgentBox rollout
identity() takes modelSelectionVersion: status.model.modelSelectionVersion ?? 0, while normalizeBoxSyncStatus correctly omits the field for a box that cannot report it. So a pre-existing box reads 0 and a recycled box reads N — same release, same fingerprint, same actual model, different identity, consistent: false. http-server.ts is in the AgentBox image and imagePullPolicy: Always only pulls on pod create, so this persists until every box has recycled.
The comment immediately below the model block says a publisher gates on consistent and reads it as a green light, which is what makes the churn costly rather than cosmetic. The same file already treats absent-versus-present deliberately for tiers ("null (no turn observed yet) stays distinct from {both null}"); giving modelSelectionVersion the same treatment — carry it only when present, and do not let a one-sided absence count as a difference — avoids the window. Accepting it is also defensible since releaseId has the same property, but then it should be said out loud, because the rollout will look like a divergence.
Smaller
chat.send'smodelRoutingparam is now silently discarded when the caller also sends a selection version (promptOpts.modelRouting = binding.modelRouting; the new test pins it asundefinedafter the caller passed{enabled: true}). If the binding is authoritative for routing that is the right call, but it is a caller-visible behavior change that the PR body does not mention.params.modelSelectionVersionis compared without a cast, unlike the adjacentparams.releaseId as string | undefined/params.modelFingerprint as string | undefined. A JSON"3"therefore fails with "model selection version does not match expected Agent configuration", which misdescribes a type error as a staleness error.- The selection-version check runs before the release and fingerprint checks, so a version mismatch masks a release mismatch in the reported message.
observedModeluses>=, which correctly refreshesobservedAtfor the same selection. But a prompt carrying no version reads as 0 and can therefore never overwrite a versioned observation. That is self-consistent today because all four forwarders read the same binding — worth a comment saying so, since a future prompt path that forgets to forward the field would silently freeze the box's model observation rather than fail visibly.
Internal child executions bypass HTTP prompt accounting. Emit their independent usage delta once, and verify cached tokens, reasoning, restored history and model changes with the installed SDK. Exercise model-only invalidation against actual SDK tool wrappers so active turns retain usable tools.
|
Superseded. Siclaw delivery has moved off this public repository; this change is continuing on the internal Siclaw repo. Closing the public PR. |
Shared Agent model changes take effect at the next dispatch while a running turn keeps its captured configuration. Carry the optional selection version through prompts, reload preparation and observed box status, including A-to-B-to-A changes within one release.
Resolve selection-aware chat bindings after acquiring the turn lock on every turn. The dispatched binding owns the model, routing, fast sub-agent candidates, output redaction and Agent Addendum; a caller-stamped old prompt is replaced too. If the current binding cannot be resolved, fail explicitly before dispatch instead of silently using an older selection. Callers without selection metadata retain their existing prompt and routing behavior. Validate version types at RPC boundaries before checking staleness. Direct DingTalk turns also resolve the Addendum after acquiring the session lock, preferring the binding value, including an explicit empty prompt; control planes using config.getAgent retain that current lookup.
The companion control plane sends model-only invalidation for preference changes. A regression exercises real SDK tool wrappers to verify pending and subsequent tools remain usable until that turn ends. Reject stale reload receipts and prevent older completed turns from replacing newer model observations.
Internal spawned children also report their own token and cost deltas to the existing metrics path. Previously they bypassed HTTP prompt accounting and were missing from totals. Child and parent histories stay separate. Accounting regressions cover success/failure, cached and reasoning tokens, next-turn model changes, restored history and recorded compaction. Cost still depends on supplied model prices; this change does not provide billing reconciliation.
Rollout: upgrade Runtime and AgentBox first, then every control-plane replica so
config.getModelBindingreturns a selection version (including zero), before enabling selection writes or versioned reloads. Old bindings represent version zero and cannot acknowledge nonzero selections. Mixed old/new boxes can report inconsistent evidence until old boxes finish active work, are recycled normally and provide versioned real-turn observations. Missing version evidence must not certify a later A-to-B-to-A selection. Details:docs/design/agent-model-selections.md.Validation: 487 focused tests pass across DingTalk, Lark, chat dispatch, reload, sync status, HTTP, real SDK busy-session handling and usage accounting. Two additional internal-child usage tests pass for successful and failed execution. The three real-lock DingTalk regressions first reproduced the old Addendum with the new model and now pass, covering binding-provided, separately resolved and cleared prompts. Runtime and AgentBox TypeScript checks pass. No real-provider or deployed browser acceptance was performed.