fix(promotion): refuse to call a candidate cheaper on dollars nobody measured - #949
Conversation
9122439 to
2784ddb
Compare
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 2784ddbb
This PR was opened by the trusted drewstone account.
This approval is provisional and was applied by the local stand-in because the pr-reviewer webhook host is unreachable (2026-08-21). CI on this head is fully green. The full PR reviewer audit re-runs via the resweep when the service returns and will publish findings if it detects issues.
…measured BenchmarkCell carries usd and usdKnown as required siblings. promotionGate read usd at both arms and never read usdKnown, while non-inferiority mode promotes on a significant paired cost saving. A candidate whose dollars were a catalog estimate or an unreported floor could therefore be promoted for being cheaper than a receipted incumbent. Spend.usdKnown states the rule this gate has to obey: a false value must not be treated as a measurement when enforcing a dollar-denominated comparison or limit. Promotion on cost savings is that comparison. The verdict now carries cost-unknown and names the tasks that caused it, so the refusal says which evidence to go and measure.
2784ddb to
9ab6064
Compare
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 9ab60645
This PR was opened by the trusted drewstone account.
This approval is provisional and was applied by the local stand-in because the pr-reviewer webhook host is unreachable (2026-08-21). CI on this head is fully green. The full PR reviewer audit re-runs via the resweep when the service returns and will publish findings if it detects issues.
check:version-bump compared export names only, so it reported "consumer surface unchanged" for #946, which removed WorkerView.cwd from a public interface, and for #949, which added a member to the public PromotionVerdict.reason union. Neither moved a name, so api-surface.json was byte-identical across both and the 0.154.0 bump was a human judgement rather than a gate result. The record now states a shape digest beside each kind, taken over the built declaration with comments removed, whitespace collapsed, the declaration's own local name blanked, and every type reference rewritten to a stable token. A reference to a symbol this package exports contributes that symbol's public name, so an edit is reported once, on the line of the symbol that changed. A shape move is classified breaking: telling an added optional field from a removed required one is a subtyping question, and the record states structure. Refs #946, #949
Problem
BenchmarkCell(run-benchmark.ts:77) carriesusd: numberandusdKnown: booleanas required siblings.promotionGatereadusdfrom both arms (:88-89) and never readusdKnown— zero occurrences in the file.In
non-inferioritymode a significant paired cost saving is a promotion criterion (:151): "(b) cost SAVINGS (incumbent − candidate, usd/task) significantly > 0". So a candidate whose dollars were a catalog estimate or an unreported floor could be promoted for being cheaper than a receipted incumbent — comparing a number no provider ever billed against one that was.Spend.usdKnownstates the rule this gate has to obey, in its own words: "A false value must not be treated as $0 when enforcing a dollar-denominated comparison or limit." Promotion on cost savings is that comparison.budget.tsalready refuses unknown dollars under a dollar cap; this gate did not.Measured: with one of 24 paired tasks marked
usdKnown: falseon the candidate arm, the gate returnedpromoted: true, reason: 'non-inferior-and-cheaper'.Change
Non-inferiority mode collects the tasks where either arm reports
usdKnown === falseand refuses before running the cost bootstrap, with a newreason: 'cost-unknown'and acostUnknownTaskslist naming them.Why long-term right
This is the refusal the type was already asking for. The alternative — comparing anyway and hoping the estimate is close — is precisely the failure mode
usdKnownexists to make impossible, and it fails in the expensive direction: a candidate looks cheaper because its cost went unmeasured. Naming the offending tasks makes the refusal actionable: it says which evidence to go and measure, rather than returning a bare no.superioritymode is untouched, because it never reads dollars.Cost
3 files. This is a consumer-visible behaviour change: a caller that exhaustively switches on
PromotionVerdict.reasonmust handle'cost-unknown', and a non-inferiority promotion built on unmeasured dollars now refuses. Version bumped 0.153.1 → 0.154.0 with a CHANGELOG entry and thedocs/canonical-api.mdpin.Worth flagging:
check:version-bumpreported "consumer surface unchanged" both before and after the bump — it compares export names, not their shapes, so it does not see a union gaining a member. The bump here is a judgement call, not a gate result.Proof
pnpm run typecheck,pnpm run lint(613 files),pnpm run docs:check,pnpm run check:version-bump— clean.tests/kernel/strategy-suite.test.ts— 38 passed, including the existing four non-inferiority cases, which still promote/refuse exactly as before.promoted: true(expected true to be false).src/improvement/improve.test.tsshows 4 failures; that file is first on the clean-origin/mainbaseline failing-file list (the documented macOS git-worktree class) and is untouched here.Simplification
Simplification: none — this is a net addition, and the addition is irreducible. The measurement that found it was the
usd/usdKnownflag-pair census (187usdKnownoccurrences over 30 files, 56 read sites guarding on it, and 10 files that read.usdwithout ever mentioningusdKnown); this is the one of those ten where the unguarded read decides a promotion. Collapsing the flag pair into a cost discriminated union would make the omission unrepresentable rather than caught by hand, but that migration touches 143 files and remains out of scope.Net: +38 / -2 lines, 3 files, 0 copies removed.
Not done here:
waterfall.ts:86is the other consequential unguarded read —span.usd = p.spent?.usd ?? 0, rendered as$0.0000. That one is a display concern whose fix is to label the number unmeasured rather than to refuse, so it is a different change with a different test.Tests: +1 (a candidate cannot be promoted as cheaper when an arm's dollars were never measured — the exact defect, promotes without this change), -0 deleted.