Skip to content

ENG-1687: pin the verifier eval's served model so an alias repoint cannot pass silently - #434

Merged
alecantu7 merged 2 commits into
stagingfrom
alejandrocantu/eng-1687-harness-evals-pin-a-behavioural-population-to-an-alias-name
Sep 3, 2026
Merged

ENG-1687: pin the verifier eval's served model so an alias repoint cannot pass silently#434
alecantu7 merged 2 commits into
stagingfrom
alejandrocantu/eng-1687-harness-evals-pin-a-behavioural-population-to-an-alias-name

Conversation

@alecantu7

Copy link
Copy Markdown
Contributor

What

tests/test_verifier_verdict_live.py selects its two models by alias name (haiku, mindshub_air). An alias is a catalog pointer, not a model — the auth catalog can repoint one with no PR here and no drift detection. mindshub_air was repointed off Kimi K2.6 to gpt-5.6-luna around 2026-08-10, so both matrix slots now hold the same behaviour and the gate has been reporting green while covering one population twice.

This adds an identity check: _EXPECTED_SERVED records what each alias resolved to when its slot was last justified, and _check_served_model verifies it on every response. Plus the served map in the run output and the step summary, so a repoint is visible in the run that first sees it.

Why an identity check and not the behavioural probe the ticket specified

The ticket asked for a population guard that re-measures the property each alias was chosen for. Four reasons this is the better shape:

  1. Behaviour is already guarded. test_verdict asserts the verdicts themselves — a model that starts judging the fixtures differently reds with or without a repoint. Identity was the missing half; a property probe is a second behavioural guard.
  2. It is free and cannot flake. LLMResponse.model is on every response since ENG-1638, so nothing extra is called. A stochastic property would need N runs to be trustworthy — this file's own non-determinism policy says one run of a stochastic property is a coin flip, which is why the STUCK case runs 6×.
  3. It catches drift nobody predicted. A probe only finds drift in the property you thought to probe. kimi moved Moonshot → Fireworks since the ticket was filed with no narration change at all, but per ENG-1095 that flips its tool_choice failure mode. A narration probe stays green through it.
  4. It is the ticket's own argument finished. The ticket rejects dynamic alias selection because "the guard should force a human to re-pick — it should not re-pick itself." A pin is the minimal form of that.

What is deliberately NOT done

The second population is not re-picked, because there is nothing to re-pick to. Re-measured 2026-09-03 against prod through this file's own call path: 0 narration characters on eight aliases (air, haiku, kimi, deepseek, qwen, glm, grok, gpt-luna), and identically 0 with _VERIFIER_NO_PREAMBLE stripped — so it is the models that changed, not the prompt suppressing them. Prod agrees: turn_completed.verifier_failure = 'truncated' is 0 over 30 days.

Two corrections to the ticket's description, both filed as a comment on ENG-1687:

  • It claims _build_verify_request adds no anti-preamble suffix. It does, and has since fc832d5d (2026-07-27) — three weeks before the sweep it justifies. The conclusion survives the correction; the stated reason for it did not.
  • It budgets "one extra call per alias" for the guard. Not needed, per (2) above.

So the docstring now describes the matrix as two provider shapes (Anthropic-native haiku, OpenAI-native mindshub_air), which is what is true, with the dead rationale kept as dated history so nobody restores it without re-measuring.

Verification

  • Watched failing live, against prod, on today's mindshub_air — the pin set to kimi-k2p6 (what the old docstring assumed) raises AliasRepointed naming both ids. That satisfies the ticket's "watched failing before any re-pick" Done when.
  • Every new unit test watched failing under a mutation of the logic it covers: never-raise, record-before-guard, raise-on-unpinned, and stop-recording each killed exactly the intended test(s), suite green on restore.
  • Full live eval green: 13 passed in 83s, unchanged from the ~1m35s CI baseline — the shim adds no measurable cost.
  • Full unit suite green with no key: 2889 passed, 31 skipped.
  • Report verified in both sinks: terminal (via the terminal reporter — a plain print is captured and discarded on a passing test, so it only showed on red runs) and GITHUB_STEP_SUMMARY.

Comments corrected

Five sites asserted in the present tense that mindshub_air narrates, which is the belief that produced this bug: anton/core/session.py, anton/core/llm/structured.py, anton/core/llm/provider.py, anton/core/memory/consolidator.py, anton/core/memory/cortex.py. The measurements are kept as dated history — they are why the budget ladders are what they are, and deleting the rationale would invite someone to lower them. provider.py:76 already documented the repoint correctly and is untouched.

Security check

Performed. Read-only test change plus comment edits: no new endpoint, no new credential, no auth surface, no change to any request anton sends in production. Uses the existing MINDSHUB_API_KEY CI secret from ENG-1334. _check_served_model interpolates the gateway-reported model string into an exception message only — it is never written to disk, never sent anywhere, and the step-summary write is wrapped so an unwritable summary cannot fail a green eval. Fixtures are authored, not captured production payloads.

Out of scope

  • Reviving ENG-1023's billing-side alias governance.
  • Selecting the slot on the catalog's declared supports_named_tool_choice capability instead of a name — the version that stops rotting, per registry.py:117. Different ticket.

Closes ENG-1687.

alecantu7 and others added 2 commits September 3, 2026 01:21
…silently

The verdict eval picks its two models by alias NAME. An alias is a catalog
pointer, not a model: `mindshub_air` was repointed off Kimi K2.6 to
`gpt-5.6-luna` around 2026-08-10, so both slots now hold the same behaviour and
the gate has reported green while covering one population twice (ENG-1687).

Adds an identity check rather than the behavioural probe the ticket proposed.
`LLMResponse.model` is already on every response (ENG-1638), so `_check_served_model`
costs no extra call, cannot flake, and catches repoints whose consequence nobody
predicted — kimi's Moonshot -> Fireworks move changed no narration but flips its
tool_choice failure mode. Behaviour was already guarded by `test_verdict`;
identity was the missing half.

Watched failing live against prod on today's `mindshub_air` before the pin was
set to its current value, and each new unit test watched failing under a
mutation of the logic it covers.

The ticket's other steps are not done, deliberately: there is no narrating alias
left to re-pin the second slot to. Re-measured 2026-09-03, 0 narration characters
on eight aliases, identical with `_VERIFIER_NO_PREAMBLE` stripped, and prod's
`verifier_failure = 'truncated'` is 0 over 30 days. The docstring now says the
matrix covers two provider shapes, which is what is true.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l pin

Adversarial self-review of #434 at e4f34cc. All three are in the new code.

1. The pin caught the repoint but not the state it CAUSED. ENG-1687 is "both
   slots ended up holding one model and the gate stayed green" — and the obvious
   way to clear the pin's red is to update the map to the new id, which lands
   right back there if that id is the other slot's model. Distinctness now has a
   test instead of holding by accident, covering both routes in: identical
   aliases, and distinct aliases pinned to one model.

2. Only the coding provider was shimmed. `from_settings` builds planning and
   coding as separate objects even when both roles name the same alias, so any
   future `generate_object` or `chat` call in this file would have been silently
   unpinned — the same quiet-gap class the pin exists to close. Both providers
   are wrapped now, deduplicated by identity, each closing over its own inner.

3. `response.model` is remote text reaching an exception message and
   GITHUB_STEP_SUMMARY, and a newline in it injects lines into a CI artifact —
   a bogus "✅" row for an alias never checked. Reuses
   `identity.sanitize_model_name`, which anton already applies to this exact
   field before it reaches a prompt, rather than the hand-rolled isinstance
   check.

Fix 3 initially passed with the sanitiser removed, so its test was written
until it failed for the right reason. Each fix is mutation-verified: pin-to-
duplicate and same-alias both red their test, and the sanitiser's removal reds
the injection test on both the recorded value and the raise. Verified live that
the planning path is now counted (3 raw completes -> 3 guard invocations across
2 verdict calls and 1 `generate_object`) with distinct inner bindings.

Full unit suite 2892 passed / 31 skipped; full live eval 13 passed in 84s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alecantu7

Copy link
Copy Markdown
Contributor Author

Self-review (convention 6)

At d8633c87. Two rounds, both in the commit messages; consolidating here because the substance of this PR is a deliberate deviation from the ticket, and that is a decision for the reviewer to accept or reject rather than something to find buried in a commit body.

The deviation, stated plainly

ENG-1687 asked for a behavioural population guard — re-measure the property each alias was chosen for, one extra call per alias. This ships an identity pin (_EXPECTED_SERVED + _check_served_model) instead, and explicitly does not do the ticket's Done when item "the matrix's second population is re-picked deliberately."

Four reasons, in descending order of how much they should matter to you:

  1. The identity check catches drift nobody predicted. A probe only finds drift in the property you thought to probe. kimi moved Moonshot → Fireworks since the ticket was filed — it now serves accounts/fireworks/models/kimi-k3 — with no narration change at all, but per ENG-1095 that flips its tool_choice failure mode. A narration probe stays green straight through it.
  2. Behaviour is already guarded. test_verdict asserts the verdicts themselves, so a model that starts judging the fixtures differently reds with or without a repoint. Identity was the missing half, not behaviour.
  3. It is free and cannot flake. LLMResponse.model rides every response since ENG-1638, so nothing extra is called. A stochastic property would need N runs to be trustworthy — this file's own non-determinism policy is why the STUCK case runs 6×.
  4. It is the ticket's own argument finished. The ticket rejects dynamic alias selection because "the guard should force a human to re-pick — it should not re-pick itself." A pin is the minimal form of that.

The test that actually closes ENG-1687

test_the_matrix_slots_resolve_to_different_models is the one to read first, because the pin alone does not fix this ticket.

ENG-1687 is not "an alias moved" — it is "both slots ended up holding one model and the gate stayed green." _EXPECTED_SERVED reds on the move, but the obvious way to clear that red is to update the map to the new id, and if the new id is the other slot's model the eval goes straight back to running one model twice with a perfectly green pin. That test asserts distinctness, covering both routes in: identical aliases (both VERIFIER_EVAL_*_MODEL set the same) and distinct aliases pinned to one served id.

That is the coverage-collapse guard, expressed as identity rather than behaviour. If you reject the deviation, this is the test to argue with.

Why the second population is not re-picked

There is nothing to re-pick to. Re-measured 2026-09-03 against prod through this file's own call path (_build_verify_request, the real fixture, the real 2048 budget): 0 narration characters on kimi, mindshub_air and haiku over 4 runs each, and 0 across the wider alias sweep.

One clarification worth making explicitly, because the phrasing invites a wrong reading. "0 narration characters" is a claim about this call shape, not about the models in general. Measured on a deliberately short 76-char system prompt instead of the real 439-char verifier prompt, kimi narrates on ~3 of 4 runs and up to 1,295 characters — squarely inside ENG-1081's 245–1,654 range. So the narrating population has not disappeared from the catalog; it is suppressed by this call's prompt. Two consequences the next reader should have:

  • The ENG-1081 truncation risk is currently prevented by the prompt, not by the models having changed. If _VERIFIER_NO_PREAMBLE or the verifier system prompt is ever shortened, narration returns and there is no eval slot covering it.
  • Re-picking kimi as the narrating slot would therefore need a prompt change too, plus the N-of-N treatment the STUCK case gets, since its narration is stochastic. That is a design decision, not a one-line pin edit — which is the honest reason it is not in this PR.

turn_completed.verifier_failure = 'truncated' is 0 over 30 days in prod, so nothing is currently being lost.

Two corrections filed against the ticket

  • It claims _build_verify_request adds no anti-preamble suffix. It does, and has since fc832d5d (2026-07-27) — three weeks before the sweep that justified the claim. The conclusion survives; the stated reason for it did not.
  • It budgets one extra call per alias for the guard. Not needed, per reason 3.

Round 2 findings (d8633c87), all in round 1's own new code

  1. The pin caught the repoint but not the state it caused — fixed by the distinctness test above.
  2. Only the coding provider was shimmed. from_settings builds planning and coding as separate objects even when both roles name the same alias, so any future generate_object or chat call in this file would have been silently unpinned — the same quiet-gap class the pin exists to close. Both providers are wrapped now, deduplicated by identity.
  3. The served id is remote text landing in an exception message and in GITHUB_STEP_SUMMARY, so it goes through identity.sanitize_model_name first — a value carrying newlines would otherwise inject lines into a CI artifact.

Verification

  • Live eval green against prod: 13 passed in 99s, and the served map prints on a passing run:
    verifier eval served models:
      haiku        -> claude-haiku-4-5-20251001
      mindshub_air -> gpt-5.6-luna
    
    (A plain print is captured and discarded on a passing test, so this goes through the terminal reporter — verified in both sinks, terminal and GITHUB_STEP_SUMMARY.)
  • Watched failing live, which is the ticket's "watched failing before any re-pick" Done when. With the pin set to kimi-k2p6 (what the old docstring assumed):
    AliasRepointed: alias 'mindshub_air' now serves 'gpt-5.6-luna', but this eval's
    matrix slot was measured on 'kimi-k2p6'. ... re-read why this slot exists, decide
    whether 'gpt-5.6-luna' still covers it, then update _EXPECTED_SERVED and the
    module docstring in the same commit. See ENG-1687.
    
  • Four mutations of the guard, each killed by the intended tests: never-raise (1), record-before-guard (2), raise-on-unpinned (1), stop-recording (5). The deliberate served is None fail-open is itself pinned — making it raise fails 4 tests.
  • Gate suite 28 passed with no key or network. Full unit suite 2931 passed / 31 skipped, live eval correctly skipped without a key.

Known residual, deliberately accepted

LLMResponse.model is str | None — None when the provider omits it — and _check_served_model returns silently in that case. A repoint onto a provider that does not echo model would therefore disarm the pin, which is the same shape as the bug this closes. Refusing to run instead would turn a missing echo into a fake repoint, and MindsHub does echo the resolved id, which is what makes it safe today. Named here rather than left to be discovered.

Comments corrected

Five sites asserted in the present tense that mindshub_air narrates — the belief that produced this bug: session.py, structured.py, provider.py, consolidator.py, cortex.py. Every one is converted to dated history rather than deleted: those measurements are why the budget ladders are what they are, and removing the rationale invites someone to lower them. provider.py:76 already documented the repoint correctly and is untouched.

Possible follow-up, out of scope here

In the alias sweep, deepseek and qwen returned 255 and 256 output tokens against a 256 cap with 0 narration characters — the shape of reasoning-token truncation at the budget, i.e. ENG-1081's failure mode on aliases nothing currently watches. Not this PR's job; worth a look.

@alecantu7
alecantu7 requested a review from pnewsam September 3, 2026 19:22

@pnewsam pnewsam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verdict: APPROVE
Model: claude-opus-4-8[1m]

Reviewed the served-model pin added to tests/test_verifier_verdict_live.py, its unit coverage in tests/test_verifier_eval_gate.py, and the five comment corrections. I traced the shim's actual call path (generate_object_code_generate_object_withprovider.complete), confirmed the exception design against _verdict's catch clauses, and checked the sanitizer's behavior against every test input. The change is read-only test/comment work, correctly wired, well-tested, and all CI is green. No actionable findings.

Validation

  • Confirmed _pinned wraps the correct chokepoint: the verdict call routes through provider.complete(**kwargs) (all-kwargs, client.py:384), so the shim's async def complete(_inner=inner, **kwargs) captures every call; _inner=inner default-binds per iteration, so no closure-over-loop-variable bug, and dedup-by-id() handles shared provider objects.
  • Confirmed AliasRepointed propagates as intended: _verdict catches only StructuredOutputError/TokenLimitExceeded ("anything else propagates"), and _call_with_auth_confirmation re-raises everything but ProviderAuthError — so a repoint fails the eval and cannot be absorbed by the retry/skip paths. The gate test asserts the subclass relationships that guarantee this.
  • Verified sanitize_model_name returns None for non-str/empty (covers the [None, "", 0, object()] case), strips \r\n (the CI-artifact injection defense), and caps at 80 chars — matching every assertion in the injection and missing-id tests. Record-before-raise ordering in _check_served_model matches the test that requires a failing run to still report the served id.
  • Confirmed the eval's only LLM call is generate_object_code (coding provider); the planning-provider wrap is defensive future-proofing, not dead-but-wrong code.
  • CI: all checks pass, including verdict-eval (1m53s) and run-tests (3m44s). Mergeable, CLEAN.

One out-of-scope note, not a finding: _pinned intentionally does not wrap _router_provider, which is correct because the eval makes no router calls — worth keeping in mind only if a future case adds one.

@alecantu7
alecantu7 merged commit 1933d81 into staging Sep 3, 2026
13 checks passed
@alecantu7
alecantu7 deleted the alejandrocantu/eng-1687-harness-evals-pin-a-behavioural-population-to-an-alias-name branch September 3, 2026 19:39
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants