Skip to content

No test drives a 402 through a real escalation site — the downgrade guard is unpinned at the consumer #40

Description

@thegeorgepu

Split out of #33 / PR #38 so a correct fix was not held for scope creep. This is a coverage gap, not a live defect — the current behaviour is correct.

What is already covered

PR #38 pins the classifier: classifyVinciModelError returns "account" for every 402, and vinci/test/402-escalation-no-downgrade.mjs asserts it. That assertion is mutation-verified — flipping the classifier to "transient" fails it by name with exit 1.

What is not covered

Nothing drives a 402 through an actual escalation site. The four consumers are vinci-advisor.ts:105-106, vinci-council.ts:138-139, vinci-scope.ts:497-498, vinci-loopbreak.ts:484-485, all with the same shape:

const kind = classifyVinciModelError(error);
if (kind === "transient" && attempt < SAME_CLASS_ATTEMPTS) continue;   // retry same class
if (kind === "transient" || kind === "unavailable") {
  unavailableClasses.push(classId);
  break;                                                                // -> next, cheaper class
}
throw new Error(`... Vinci will not downgrade after an account or terminal error ...`);

"account" reaches the throw. Nothing tests that it does.

So the contract currently holds by a two-link chain — classifier says "account", and consumers happen to treat "account" as terminal — with only the first link pinned. A change to any of the four consumers can silently reintroduce downgrade-on-billing-error and every existing test stays green.

Why the current test does not close it

402-escalation-no-downgrade.mjs has an "ASSERTION 3" that appears to cover this but does not — it is a hand-copied reimplementation of advisor's control flow inside the test file:

const kind = inFlightKind;
if (kind === "transient" && attempt < SAME_CLASS_ATTEMPTS) throw new Error("DEFECT: would retry same class");
if (kind === "transient" || kind === "unavailable") { unavailableClasses.push("cheaper"); throw ... }
assert.deepEqual(unavailableClasses, [], "no downgrade attempted");

It asserts against its own copy of the logic, never importing or calling advisor. Editing vinci-advisor.ts cannot make it fail. A test that duplicates the code under test measures the duplicate. It should be deleted or replaced as part of this issue — leaving it is worse than an honest gap, because it reads as coverage.

The ask

One test that drives a real 402 through at least one escalation site (advisor is the simplest) with a stubbed model call that throws it, asserting:

  • the call THROWS rather than returning a result from a cheaper class
  • unavailableClasses stays EMPTY
  • no second model class is attempted (a recorder on the stub proves this)

Mutation-verify it: with the classifier returning "transient", the new test must fail. If it passes under that mutation, it is not testing the consumer.

Ideally parameterise over all four sites, since they share the shape and will drift independently.

Why it matters

The failure this guards is silent and expensive: a billing-shaped error on a strong model quietly falling back to a cheaper one, producing worse output with only a notice. That is exactly the class the no-downgrade rule exists for, and it was live in an earlier revision of PR #38 before review caught it.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions