Skip to content

fix(proxy): forward PriorServedModel on regular session-pin usage writeback#752

Open
rohith500 wants to merge 2 commits into
workweave:mainfrom
rohith500:fix/session-pin-prior-served-model
Open

fix(proxy): forward PriorServedModel on regular session-pin usage writeback#752
rohith500 wants to merge 2 commits into
workweave:mainfrom
rohith500:fix/session-pin-prior-served-model

Conversation

@rohith500

Copy link
Copy Markdown
Contributor

Summary

recordTurnUsage (the regular per-turn session-pin writeback) never forwarded PriorServedModel into sessionpin.Usage, unlike recordHMMTurnHistory (the HMM writeback), which has passed it correctly since #677. Because of that, a tier-role pin row whose first-ever write happens on the regular path can never latch has_ever_switched = true from prior evidence — even when the session demonstrably switched models, as long as that knowledge only exists in the row's <role>_hmm_history sibling.

This is invisible while the _hmm_history sibling is alive (a read-time OR masks it). It surfaces once that sibling expires or is swept: a later same-model turn then forwards a stale-signed Anthropic thinking block from an earlier cross-model excursion straight to the upstream request body — reopening the exact failure class #383 fixed ("Invalid signature in thinking block").

Full root-cause analysis, live repro with captured request bytes, and consequence trace: #751

Fix

One field, mirroring the existing recordHMMTurnHistory line:

// internal/proxy/service.go, recordTurnUsage
usage := sessionpin.Usage{
    InputTokens:       in,
    CachedReadTokens:  cacheRead,
    CachedWriteTokens: cacheCreation,
    OutputTokens:      out,
    EndedAt:           time.Now(),
    ServedModel:       servedModel,
    PriorServedModel:  res.PriorServedModel,   // added
}

res.PriorServedModel is already computed every turn regardless of HMM classification (switchHistoryFromPins, turnloop.go:340) — this fix doesn't add new computation, it just stops dropping a value that was already available.

Why this is safe (not a behavior change)

  • For sessions that never touch HMM, hmmHistory is always a zero-value sessionpin.Pin{}, so PriorServedModel resolves to the pin's own existing LastServedModel (or "" on a first write) — identical to today's behavior. The fix is a no-op there.
  • For an existing (non-fresh) row, the forwarded value is redundant with the SQL's own last_served_model <> '' AND last_served_model <> @last_served_model clause, which reads the column fresh at UPDATE time — no new false-positive risk.
  • Searched the full test suite for any assertion that PriorServedModel stays empty on a regular-path UpdateUsage call — none found. Nothing depends on the old (broken) shape.

Tests

TestRecordTurnUsage_ForwardsPriorServedModel (internal/proxy/turnloop_internal_test.go), added next to the existing TestRecordTurnUsage_WritesToStore / TestRecordTurnUsage_HMMDecisionWritesHistoryOnly coverage, using the same newStubPinStore() test double already used there.

  • Fails on main: PriorServedModel empty in the recorded Usage.
  • Passes with the fix: asserts store.lastUsage.PriorServedModel == "claude-fable-5", and separately confirms the write lands on the active tier role (store.usageRoles == [role]) and explicitly not on hmmHistoryRole(role) — so the test is provably exercising the regular path, not the HMM one.
    The hand-set PriorServedModel: "claude-fable-5" fixture value matches what switchHistoryFromPins actually produces for a fresh active pin (LastServedModel == "") with a switched hmm_history sibling — verified by re-reading turnloop.go:1020-1026 against the test setup, not assumed.

Live verification (beyond the unit test)

Re-ran the full repro from #751 against this branch's actual committed code (git diff HEAD on the changed file confirmed empty throughout — not a locally re-applied patch), using a local mock Anthropic upstream and mock HMM sidecar:

  1. HMM turn switches role default_low from claude-haiku-4-5claude-fable-5. default_low_hmm_history.has_ever_switched = true.
  2. Regular (non-HMM) turn — first write to default_low itself, served by claude-haiku-4-5 again.
   role                     | last_served_model | has_ever_switched
   default_low              | claude-haiku-4-5  | t   ← now true (was false pre-fix)
   default_low_hmm_history  | claude-fable-5    | t
  1. Deleted default_low_hmm_history (simulating TTL drift + sweep, per session-pin has_ever_switched latch never fires on the regular (non-HMM) writeback path #751's "why it's latent" analysis).
  2. Same-model turn on default_low, client transcript carrying an assistant thinking block signed for claude-fable-5. Captured outbound body to mock Anthropic: no thinking block present — correctly stripped, matching the "fixed" capture from session-pin has_ever_switched latch never fires on the regular (non-HMM) writeback path #751's original verification.

Regression sweep

Check Result
go test ./internal/proxy/ ./internal/router/sessionpin/ PASS
make check All checks passed
Tests depending on PriorServedModel staying empty (regular path) None found
Other sessionpin.Usage{} constructions/assertions affected None outside internal/proxy; all already expect PriorServedModel set where relevant

Hygiene

  • gofmt -l clean on both changed files
  • Both commits DCO signed-off (Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com>)
  • Branch based on current main tip (f1e5e93), confirmed via git ls-remote immediately before verification — main had not moved

Commits

  1. bc51dd9test(proxy): require PriorServedModel on regular session-pin usage writeback
  2. dbb8864fix(proxy): forward PriorServedModel on regular session-pin usage writeback
    Fixes session-pin has_ever_switched latch never fires on the regular (non-HMM) writeback path #751

rohith500 and others added 2 commits July 16, 2026 14:49
…iteback

A regular (non-HMM) UpdateUsage must carry PriorServedModel so a new
tier-role pin can latch has_ever_switched from sibling hmm_history
evidence already surfaced on the turnLoopResult.

Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…teback

Mirror the HMM history path so UpdateSessionPinUsage can latch
has_ever_switched when the first write to a tier-role pin already has
prior-served evidence from switchHistoryFromPins.

Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown

PR author is not in the allowed authors list.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Thanks for this, @rohith500 — clean, focused fix. 🙏

Forwarding PriorServedModel on the regular (non-HMM) usage writeback so a brand-new tier-role row can latch has_ever_switched from sibling hmm_history evidence is exactly the kind of minimal, targeted change the repo likes. I checked it against the conventions (AGENTS.md) and it conforms — no changes needed:

  • Minimal, scoped — a single field forwarded in recordTurnUsage, no incidental churn.
  • Tests — non-tautological: TestRecordTurnUsage_ForwardsPriorServedModel asserts the value reaches UpdateUsage on the active tier role and that regular turns don't write the HMM history role, and the comment explains why that value matters.

Approving from a conventions standpoint — thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

session-pin has_ever_switched latch never fires on the regular (non-HMM) writeback path

1 participant