Uh oh!
There was an error while loading. Please reload this page.
fix(engine): warn when the provider pricing hook returns nothing for every model - #388
Conversation
…hing microsoft#265 added the get_model_pricing hook and warns once if it RAISES. It does not notice the companion case: a hook that never raises and returns None for every model. That is the case in practice. github-copilot-sdk 1.0.1 removed ModelBilling.token_prices, which is the field CopilotProvider's hook reads, so it resolves None for all 21 models it lists. Verified directly against the SDK: billing attrs: ['from_dict', 'multiplier', 'to_dict'] models=21 with token_prices=0 with multiplier=0 Nothing raises, nothing logs, and models that happen to be in the static table still report a plausible cost -- so the fact that live pricing is dead is invisible. The only symptom is newer models showing as unpriced, which looks like two missing table entries rather than a broken mechanism. It cost me an afternoon of probing to tell those apart. Warn once per run when the hook has returned None for more than one distinct model and has never priced anything, naming the models. The thresholds matter: a single None is ordinary (that model just is not priced) and any successful pricing means the hook works, so both stay quiet. Only 'None for everything, nothing ever priced' is reported. Deliberately NOT adding claude-opus-5 / gpt-5.6-sol to DEFAULT_PRICING in this change. I do not have authoritative per-token rates for them, and inventing numbers would produce confidently wrong costs -- which is worse than unpriced and is precisely what microsoft#265 set out to stop. That wants real pricing data, not a guess from me. Tests assert both the warning and, as importantly, the two silences. Verified by neutering the warning: 2 of 5 fail. tests/test_engine matches the pre-existing baseline exactly (442 failures before and after; sandbox lacks pytest-asyncio). ruff clean. Refs microsoft#386
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #388 +/- ##
=======================================
Coverage ? 91.72% =======================================
Files ? 109 Lines ? 17836 Branches ? 0 =======================================
Hits ? 16360 Misses ? 1476 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Reviewed both the concept and the implementation. Thanks again for your contributions! The problem is real and worth fixing: a hook that goes quiet is genuinely indistinguishable from a model that simply isn't priced, and #265 only covers the raising case. The latch design is sound, and the negative test cases are well chosen.
Three things I'd want resolved before this lands, all clustered around who sees the warning and when.
It fires for behaviour that is correct on four of the five providers.AgentProvider.get_model_pricing returns None by default and only Copilot overrides it, so claude, hermes, aca and claude-agent-sdk users get told their SDK dropped a field. I reproduced this against a provider inheriting the base hook, with two models the static table prices correctly. Every clause of the message is false for that run. The cost is worse than noise: people learn within two runs to scroll past it, and then miss it when the real Copilot regression arrives.
It also fires early._pricing_hook_priced_any is read on the second None, so a working hook that declines two models before pricing a third has already warned. Because _ensure_pricing_resolved locks per model, arrival order under parallel and for_each groups varies, so this is nondeterministic. Deferring the verdict to end of run fixes that, removes the need for the two-model floor, and puts the conclusion next to the cost total.
Which is the third point.cli/run.py:1190 isn't touched here, and a model priced from the static table has a cost_usd, so it never enters unpriced_models. The summary keeps printing a confident number sourced from a possibly stale table, while the explanation goes to lastResort stderr. Threading a live_pricing_degraded flag into the usage data the summary already reads would put the caveat where people actually look.
Smaller items: there's no CHANGELOG entry (the #265 commit added two), and ty check reports 11 errors on the new test file, which CI won't catch because it only checks src.
One thing that isn't a blocker for this PR but changes the picture on #386: tokenPrices is still on the wire and still modelled in copilot/generated/rpc.py. It's copilot/client.py's hand-written ModelBilling that discards it, and client.list_models() returns that one. The field wasn't removed, so #386 may be a much smaller fix than it looks. Probably worth a comment on the issue.
| # The companion case: a hook that never raises but returns ``None`` for | ||
| # everything. That is what happens when the provider SDK stops | ||
| # publishing the field the hook reads — ``github-copilot-sdk`` 1.0.1 | ||
| # dropped ``ModelBilling.token_prices``, so the hook resolves ``None`` | ||
| # for all models and every run silently falls back to the static table. | ||
| # A single ``None`` is ordinary (one unknown model); ``None`` for | ||
| # everything with nothing ever priced means the mechanism is dead, and | ||
| # the two are indistinguishable without this. See #386. |
There was a problem hiding this comment.
The SDK didn't remove this field. copilot/generated/rpc.py still defines ModelBilling with token_prices, parsed from the wire key tokenPrices. What actually happens is that copilot/client.py declares a second, hand-written ModelBilling whose from_dict reads only multiplier and throws tokenPrices away, and client.list_models() returns that one.
The distinction matters because the two diagnoses send you to different places. "The SDK dropped it" points at pinning an older version or hunting for a rename, and both are dead ends. The real fix is upstream in client.ModelBilling.from_dict, or a local workaround that bypasses the lossy dataclass.
The same claim appears at line 1935, in the log message at 1964, and in the test module docstring, so all four need to move together.
| # The companion case: a hook that never raises but returns ``None`` for | |
| # everything. That is what happens when the provider SDK stops | |
| # publishing the field the hook reads — ``github-copilot-sdk`` 1.0.1 | |
| # dropped ``ModelBilling.token_prices``, so the hook resolves ``None`` | |
| # for all models and every run silently falls back to the static table. | |
| # A single ``None`` is ordinary (one unknown model); ``None`` for | |
| # everything with nothing ever priced means the mechanism is dead, and | |
| # the two are indistinguishable without this. See #386. | |
| # The companion case: a hook that never raises but returns ``None`` for | |
| # everything, which is indistinguishable from "these models are simply | |
| # unpriced" unless it is tracked. Live today on Copilot: the SDK's | |
| # hand-written ``client.ModelBilling`` parses only ``multiplier`` and | |
| # discards the ``tokenPrices`` wire field that ``generated.rpc`` still | |
| # models, so the hook resolves ``None`` for every model. See #386. |
| return None | ||
| return self._single_provider | ||
| def _note_pricing_hook_result(self, model: str, pricing: object | None) -> None: |
There was a problem hiding this comment.
object | None collapses to plain object, so anything type-checks here and the annotation carries no information. ModelPricing is already imported at line 26, and the caller holds exactly that type (set_provider_pricing declares it in engine/usage.py).
| def_note_pricing_hook_result(self, model: str, pricing: object|None) ->None: | |
| def_note_pricing_hook_result(self, model: str, pricing: ModelPricing|None) ->None: |
| # Require more than one distinct model before concluding the hook is | ||
| # systemically silent, so a single unpriced model stays quiet. | ||
| if ( | ||
| self._pricing_hook_silent_warned | ||
| or self._pricing_hook_priced_any | ||
| or len(self._pricing_hook_none_models) < 2 | ||
| ): | ||
| return |
There was a problem hiding this comment.
_pricing_hook_priced_any is read at the moment the second None arrives, so this decides the question before the answer exists. A hook that declines models A and B and then prices C has already warned, and nothing takes it back. Since _ensure_pricing_resolved locks per model rather than globally, arrival order under a parallel or for_each group varies, so the same workflow can warn on one run and stay quiet on the next.
Accumulating is fine; it's the verdict that's early. Consider keeping _pricing_hook_none_models and _pricing_hook_priced_any as pure bookkeeping and evaluating once where the usage summary is built, near line 6712.
That would also let you drop the two-model floor. Right now a single-model workflow can never warn no matter how dead the hook is, and 36 of the 40 shipped examples resolve to exactly one model. #386's own reproduction is a single-model run.
| "Provider pricing hook returned no pricing for any of %d models (%s); " | ||
| "live pricing is unavailable and costs fall back to the static table " | ||
| "or show as unpriced. This usually means the provider SDK no longer " | ||
| "publishes the pricing field the hook reads.", |
There was a problem hiding this comment.
Two problems with the text, and one with where it goes.
The root-cause sentence is wrong (see the comment on line 420), and the engine can't know the cause in any case. On top of that, this fires on four of the five providers for behaviour providers/base.py:406 documents as correct, so "usually" points the wrong way: the common case is a provider that never implemented the hook.
"falls back to the static table or show as unpriced" also leaves readers unable to tell which happened to their run, and gives them nothing to do about it.
Separately, Conductor installs no logging handlers, so this reaches logging.lastResort as an unattributed stderr line, absent from the JSONL log and the dashboard. Under --web-bg it lands in a temp file nobody was told to read. executor/agent.py:597 hit the same wall and settled on log plus _emit, following checkpoint_save_failed. Worth emitting an event here too.
| "Provider pricing hook returned no pricing for any of %d models (%s); " | |
| "live pricing is unavailable and costs fall back to the static table " | |
| "or show as unpriced. This usually means the provider SDK no longer " | |
| "publishes the pricing field the hook reads.", | |
| "Provider pricing hook returned no pricing for any of the %d models " | |
| "resolved so far (%s). Costs for models in the static pricing table " | |
| "are estimates from that table; models missing from it are reported " | |
| "as unpriced. Set `cost.pricing` in the workflow to supply rates.", |
| ) | ||
| else: | ||
| self.usage_tracker.set_provider_pricing(model, pricing) | ||
| self._note_pricing_hook_result(model, pricing) |
There was a problem hiding this comment.
The base get_model_pricing returns None for every model by design, and providers/base.py:406 says so: providers whose SDK exposes no pricing "should return None and let the static table handle it, which is exactly what this default does." Only CopilotProvider overrides it. So this call treats the documented, correct behaviour of claude, claude-agent-sdk, hermes and aca as evidence that their SDK broke.
I confirmed it with a provider inheriting the base hook and two models the static table prices correctly. The warning fires and the run is fully priced.
A pricing field on ProviderCapabilities would be the tidier long-term shape, matching how skills is declared, but the identity check below is enough to stop the false positive.
One more thing about this line: it sits in the else: rather than inside the try:, and _ensure_pricing_resolved promises never to raise. Nothing in the helper can raise today, so this is latent rather than live. It stops being latent the moment _emit goes in and puts subscriber code on this path.
| self._note_pricing_hook_result(model, pricing) | |
| iftype(provider).get_model_pricingisnotAgentProvider.get_model_pricing: | |
| self._note_pricing_hook_result(model, pricing) |
| That is not hypothetical. ``github-copilot-sdk`` 1.0.1 removed | ||
| ``ModelBilling.token_prices``, which is the field ``CopilotProvider``'s hook | ||
| reads, so it resolves ``None`` for all 21 models it can list. Nothing raises, |
There was a problem hiding this comment.
Same correction as workflow.py:420: the field is still on the wire and still modelled in copilot/generated/rpc.py. It's the client's hand-written dataclass that drops it.
The model count will go stale too, and nothing in the tests depends on it.
| Thatisnothypothetical. ``github-copilot-sdk``1.0.1removed | |
| ``ModelBilling.token_prices``, whichisthefield``CopilotProvider``'shook | |
| reads, soitresolves``None``forall21modelsitcanlist. Nothingraises, | |
| Thatisnothypothetical. TheCopilotSDKclient'shand-written``ModelBilling`` | |
| discardsthe``tokenPrices``wirefield, so``CopilotProvider``'shookresolves | |
| ``None``foreverymodelitlists. Nothingraises, |
| class _Recorder: | ||
| """Minimal stand-in exposing just the latch state the helper touches.""" | ||
| def __init__(self) -> None: | ||
| self._pricing_hook_none_models: set[str] = set() | ||
| self._pricing_hook_priced_any = False | ||
| self._pricing_hook_silent_warned = False | ||
| # Bind the real implementation so these tests exercise production code. | ||
| from conductor.engine.workflow import WorkflowEngine | ||
| _note_pricing_hook_result = WorkflowEngine._note_pricing_hook_result |
There was a problem hiding this comment.
Binding the real method onto a stand-in that re-declares the real state means these tests pass whether or not the feature is wired up. I checked both mutations against this branch:
- Delete the call site at
workflow.py:2040, and the whole engine suite is still green (942 passed). - Delete the three
__init__fields atworkflow.py:428-430, and all five tests here still pass, because_Recorder.__init__supplies its own.
So the guard arithmetic is well covered and the wiring isn't covered at all. "Verified by neutering the warning: 2 of 5 fail" measures the first and is silent on the second, which is the one a reviewer cares about.
The sibling file already solved this. tests/test_engine/test_pricing_hook.py::test_systemic_hook_failure_warns_once tests the raise-path latch, the direct counterpart of this one, by building a real WorkflowEngine with CopilotProvider(mock_handler=...) and awaiting _ensure_pricing_resolved twice. Its _make_config and _priced_provider helpers are reusable, and the parallel-group configs there cover the concurrency case. I'd move these five into that file rather than keep a second file for the same method.
The three negative tests survive the move essentially unchanged. They're well chosen, and the docstrings explaining why each silence matters are the best thing in this PR.
For the false-positive case, reproduce the non-overriding providers faithfully rather than hand-rolling a None hook:
provider.get_model_pricing = AgentProvider.get_model_pricing.__get__(provider)
Addresses the review on microsoft#388. Root cause corrected in all four places. `tokenPrices` is still on the wire; the SDK did not remove it. In the pinned github-copilot-sdk 1.0.1 the hand-written `client.ModelBilling` declares only `multiplier` and its `from_dict` drops `tokenPrices`, so the hook resolves None for every model. I pulled both wheels to check, which turned up something beyond the review: 1.0.9 declares `token_prices` and parses `tokenPrices`, so the field is handled again upstream. The project floor is >=1.0.0 and uv.lock pins 1.0.1, so microsoft#386 may reduce to a version bump. Noted on the issue rather than acted on here. The warning fired for behaviour four of the five providers implement correctly. `AgentProvider.get_model_pricing` returns None by design and providers/base.py documents that as right for a provider whose SDK exposes no pricing; only Copilot overrides it. Tracking is now gated on an identity check against the base implementation, so a provider inheriting the default is never counted. Moved inside the `try:` as well -- it was in the `else:` on the strength of "the helper cannot raise", which stops being true now that `_emit` puts subscriber code on this path. The verdict was also drawn too early. `_pricing_hook_priced_any` was read on the second None, so a hook that declines two models and prices a third had already warned, and nothing took it back. Because `_ensure_pricing_resolved` locks per model, arrival order varies under parallel and for_each groups, so the same workflow warned intermittently. `_note_pricing_hook_result` is now pure bookkeeping and `_warn_if_pricing_hook_silent` draws the conclusion once, where the usage summary is built. That also removes the need for the two-model floor, which exempted exactly the runs most likely to hit this: most shipped examples resolve a single model, and microsoft#386's own reproduction is a single-model run. The message no longer guesses at a cause the engine cannot know, and says which of the two outcomes applies -- static-table estimate versus unpriced -- plus what to do about it. It is emitted as a `pricing_hook_silent` event as well: conductor installs no logging handlers, so a log line alone reaches `logging.lastResort` as unattributed stderr, absent from the JSONL log and the dashboard, and under --web-bg written to a temp file nobody was told to read. The run summary gains `usage.live_pricing_degraded`, since a table-priced model has a cost_usd and never enters `unpriced_models`, so the summary otherwise prints a confident number with the caveat somewhere else entirely. `object | None` collapses to `object` and typed nothing; it is `ModelPricing | None` now. The 11 `ty` errors on the test file are gone -- it type-checks clean. Tests rewritten for the deferred verdict, including the ordering case that an early verdict gets wrong, the single-model case the floor used to exempt, the event emission, and the base-vs-override identity check. 9 passed. CHANGELOG entry added. ruff check unchanged at 2 pre-existing findings, `ty check src` unchanged at 64, my files ruff-format and `ty` clean.
Frank Li (franklixuefei)
commented
Aug 10, 2026
Thanks — every one of these held up, and chasing the first one turned up something that changes the picture on #386. Addressed in The root cause, corrected — and it goes further than the reviewYou were right that the SDK did not remove the field, and I pulled both wheels to be sure rather than take either version on trust:
So the diagnosis is confirmed andit appears fixed upstream. Corrected in all four places you listed. Firing at four of the five providersConfirmed: Also moved it inside the The verdict was early, and nondeterministic
Two-model floor dropped, for your reason: most shipped examples resolve one model and #386's own reproduction is single-model, so the floor exempted precisely the runs most likely to hit this. Running to completion having priced nothing is the evidence; a count threshold adds nothing. Message, reach, and the summaryRewrote it as suggested — no guess at a cause the engine cannot know, and it now distinguishes the two outcomes (static-table estimate vs unpriced) and says what to do. On reach: took the And the third point — Smaller items
Verification9 tests pass. One thing worth stating plainly: my first attempt at the identity check broke 228 tests — Final: 1,446 passed across
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Went through this against 245c9d8. The rework is real, and the design feedback landed properly: splitting bookkeeping from the verdict is the right shape, and dropping the two-model floor means single-model runs are covered now.
The 1.0.9 catch is the best thing in this update, and I checked it separately rather than take it on trust. In the 1.0.9 wheel client.ModelBilling declares token_prices and its from_dict reads tokenPrices, and the field names line up with what copilot.py pulls out via getattr. Lock pins 1.0.1. So #386 may well come down to a lockfile bump, which is worth saying on the issue itself.
Two things still need fixing before this goes in.
The first is a regression the refactor introduced. _warn_if_pricing_hook_silent() only has one caller, get_execution_summary(), and the CLI reaches that after the except BaseException: raise block, guarded by cost.show_summary. Three cases on this branch:
| scenario | warning |
|---|---|
| run completes, summary built | 1 |
run completes, show_summary: false | 0 |
| run fails part way through | 0 |
The old eager version caught all three. A long run that dies half way is when I'd most want to know the numbers came out of the static table, and it now says nothing at all there.
The second is that live_pricing_degraded gets written and nothing reads it. cli/run.py isn't in the diff, so display_usage_summary prints the same confident total it did before, which was the whole reason for adding the flag. The event has the same problem in a milder form. It does land in the JSONL log, because that subscriber writes whatever it's handed, but the console dispatch is an explicit elif t == ... chain with no branch for it. checkpoint_save_failed, which the comment names as the model, has one at cli/run.py:1050. The CHANGELOG's "reaches the event log and dashboard" is half right.
Then one repeat from last time, which I'd call a recommendation rather than a blocker: _Recorder is unchanged, and the refactor turned one untested wiring point into three. Details inline on the test file.
Everything else came back clean. 1475 passed, 4 skipped across test_engine and test_cli, ruff clean, ty check src clean.
| # point at which the run has finished asking the hook, so "it priced | ||
| # nothing" is finally answerable. It also puts the caveat next to the | ||
| # cost total it qualifies. | ||
| self._warn_if_pricing_hook_silent() |
There was a problem hiding this comment.
This is the only caller of the verdict, and it sits past the point where a failing run has already re-raised. cli/run.py calls get_execution_summary() after the except BaseException: raise block and only when cost.show_summary is on.
I ran the three cases on this branch with a hook that returns None for everything. A completed run with the summary built warns once. A completed run where the summary is never built stays quiet. A run that raises part way through stays quiet too. The version I reviewed last week caught all three, so this is a regression rather than a pre-existing gap.
The failing run is the one I care about most. That's when someone is squinting at cost numbers trying to work out what they spent before it died.
_pricing_hook_silent_warned already makes this idempotent, so calling it from the failure path as well is safe and needs no extra guard.
| # never appears in ``unpriced_models``. Without this flag the | ||
| # summary prints a confident number sourced from a possibly stale | ||
| # table while the explanation goes only to stderr. | ||
| "live_pricing_degraded": self._pricing_hook_silent_warned, |
There was a problem hiding this comment.
Nothing consumes this. cli/run.py isn't in the diff, so display_usage_summary still prints the total with no caveat attached, which is what the flag was for.
The event a few lines up has a softer version of the same problem. It reaches the JSONL log fine, since EventLogSubscriber writes every event it gets. But the console side is an explicit elif t == ... chain and there's no arm for pricing_hook_silent. checkpoint_save_failed, which the comment cites as the pattern being followed, has one at cli/run.py:1050.
Worth trimming the CHANGELOG line to match whatever you decide here. As it stands it claims the dashboard, and that isn't wired.
| AgentProvider as _AgentProviderBase, | ||
| ) | ||
| if type(provider).get_model_pricing is not _AgentProviderBase.get_model_pricing: |
There was a problem hiding this comment.
The gate is correct. I checked it against the real classes rather than a stand-in: ClaudeProvider, HermesProvider, AcaRuntimeProvider and ClaudeAgentSdkProvider all inherit the base and get skipped, and Copilot is the only one tracked. That closes the false positive properly.
It just isn't covered. I swapped this whole condition for an unconditional call and both pricing test files stayed green at 16 passed.
TestBaseHookProvidersAreNotAccused reads like the test for it, but what it actually asserts is _Inherits.get_model_pricing is AgentProvider.get_model_pricing on two throwaway subclasses. That's Python attribute lookup, which will hold whether or not this line exists.
| class _Recorder: | ||
| """Minimal stand-in exposing just the latch state the helpers touch.""" | ||
| def __init__(self) -> None: | ||
| self._pricing_hook_none_models: set[str] = set() | ||
| self._pricing_hook_priced_any = False | ||
| self._pricing_hook_silent_warned = False | ||
| self.events: list[tuple[str, dict]] = [] | ||
| def _emit(self, event_type: str, data: dict) -> None: | ||
| self.events.append((event_type, data)) | ||
| # Bind the real implementations so these tests exercise production code. | ||
| from conductor.engine.workflow import WorkflowEngine | ||
| _note_pricing_hook_result = WorkflowEngine._note_pricing_hook_result | ||
| _warn_if_pricing_hook_silent = WorkflowEngine._warn_if_pricing_hook_silent |
There was a problem hiding this comment.
Still the stand-in, and there are three wiring points now where there was one. Mutations on this branch, running both pricing test files together:
| mutation | result |
|---|---|
| drop the provider gate | 16 passed |
drop the _warn_if_pricing_hook_silent() call | 16 passed |
drop the _note_pricing_hook_result() call | 16 passed |
drop live_pricing_degraded | 16 passed |
drop the _emit | 1 failed |
Only the _emit mutation dies, and it dies because _Recorder supplies its own _emit. The rest are invisible for the reason they were before: _Recorder.__init__ declares the state itself, and nothing here calls the resolver or the summary.
Both of the blocking items I raised live in that blind spot, which is the part that bothers me. The failure-path gap and the unread flag are exactly the kind of thing an engine-level test catches for free.
test_pricing_hook.py already has the scaffolding. It builds a real engine, awaits _ensure_pricing_resolved, and calls get_execution_summary() in six of its tests, so both new seams are reachable there without writing anything new.
…ummary is asked for Deferring the verdict to \get_execution_summary()\ was a regression. The CLI calls it after its \�xcept BaseException: raise\ block and only when \cost.show_summary\ is set, so of the three cases the eager version covered, the refactor left one: | scenario | before | after | |---------------------------------|--------|-------| | run completes, summary built | warns | warns | | run completes, show_summary off | warns | quiet | | run dies part way | warns | quiet | The failing run is the one that matters most -- that is when someone is looking at a partial cost total trying to work out what it cost before it broke, and it is exactly where a summary-time call can never reach. Draw it in a \ inally\ around the execution loop in \ un()\ and \ esume()\, so it fires however the run ends. The verdict is idempotent, so the summary-time call stays as a safety net for callers that drive the engine without those entry points. Wire up \live_pricing_degraded\, which was written and read by nothing: \display_usage_summary\ now prints a caveat under the total, and the console event chain gains the \pricing_hook_silent\ arm it was missing (the JSONL log already had it, being type-agnostic). CHANGELOG corrected -- it claimed the dashboard, which is not wired. Tests now go through \WorkflowEngine.run\ rather than a stand-in that declares the latch state itself. The stand-in left four of the five wiring points untestable; the mutation matrix is now 6 of 6 caught, was 1 of 5. \ est_event_ordering\ gains \pricing_hook_silent\: a mock handler bypasses the SDK, so the hook is asked and prices nothing, which is the condition the verdict reports. It cannot be suppressed for mock runs without making test and production behaviour diverge -- a real SDK that prices nothing (microsoft#386) looks identical at this layer.
Frank Li (franklixuefei)
commented
Aug 11, 2026
You are right on both, and the first one is a regression I introduced while addressing your last round. Fixed in The verdict was attached to the wrong thingI moved it to The fix is to hang it off the run ending rather than off anyone asking: try:
result=awaitself._execute_loop(current_agent_name)
finally:
self._warn_if_pricing_hook_silent()in both One thing this surfaced that I did not expect:
|
| mutation | before | now |
|---|---|---|
| drop the provider gate | 16 passed | caught |
drop the _warn_if_pricing_hook_silent() call | 16 passed | caught |
drop the _note_pricing_hook_result() call | 16 passed | caught |
drop live_pricing_degraded | 16 passed | caught |
| drop the display caveat | n/a | caught |
drop the _emit | caught | caught |
The base-hook gate is now covered by a provider that reaches the engine and returns None from the base implementation, rather than by asserting is on two throwaway subclasses -- you were right that the old assertion only tested Python's attribute lookup.
One detail worth recording: my first attempt at the failing-run test had the first agent raise, and it correctly stayed quiet -- a run that dies before anything asked the hook has nothing to conclude. It needs a run that dies half way, which is your scenario anyway.
On #386
Agreed, and I will say it on the issue. One extra data point for whoever picks it up: I had 1.0.9 installed locally against a lock pinning 1.0.1, and ty check src reported approve_all receiving dict[str, str] where PermissionInvocation is expected. So the bump may not be a pure lockfile change -- copilot.py looks like it needs a look at the same time.
1479 passed across tests/test_engine and tests/test_cli, ruff and format clean.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
Two things left over from review, plus one the merge with main surfaced. resume() carries its own _warn_if_pricing_hook_silent() call, separate from run()'s, and nothing exercised it -- reverting that line kept the whole suite green. A resumed run is still a run that priced nothing, and AGENTS.md treats run/resume as a parity pair, so the gap was free to widen. The new test drives engine.resume() with a restored context and limits, following the pattern in test_resume.py, and it kills that mutation and only that mutation. The markup failure is drift rather than an authoring mistake: main gained test_markup_guards.py (microsoft#406) after this branch last merged, and the new live_pricing_degraded caveat interpolates nothing, so it wants Text.from_markup() like its neighbours in display_usage_summary rather than a bare markup string. Verified the rendered output is unchanged and no markup leaks. Deliberately left alone: pricing_hook_silent is absent from _REPLAY_ROOT_SKIP_TYPES while checkpoint_save_failed, the event it is modelled on, is in it -- so a resumed dashboard replays a stale pricing warning from the previous attempt. It is cosmetic (no frontend handler, no latch) and fixing it would pull web/server.py into a PR that does not otherwise touch it. 5952 passed, ruff and ty clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
commented
Aug 11, 2026
Pushed three things to your branch ( CHANGELOG conflict. Main had moved and both sides appended to The A markup guard you couldn't have known about. One thing I deliberately left for you to decide. On your mock-handler question: I think you called it right. Everything else held up under re-checking. The failure-path verdict fires (I ran a workflow that dies after pricing resolves), Nice work on the 1.0.9 dig, and the |
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
Resolves a CHANGELOG.md ordering conflict with #414/#413/#388, which merged into main while this PR's review was in progress. No other files conflicted; workflow.py's auto-merge is clean (this branch's _context_window_anomaly_warned latch and main's new _pricing_hook_silent_warned latch are independent additions to the same __init__). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refs #386 — addresses the diagnosability half. Deliberately not
Fixes, see below.What #386 turned out to be
I filed #386 thinking two models were missing from
DEFAULT_PRICING. Probing the SDK directly showed something more useful: the provider hook that #265 added is reading a field that no longer exists.On
github-copilot-sdk == 1.0.1:There is no
token_pricesattribute at all, sogetattr(billing, "token_prices", None)returnsNoneunconditionally. Across the whole listing:Not a per-model gap, not auth, not a matching failure —
match_model_idresolves all 21. The documented chainis in practice
override → (structurally always None) → DEFAULT_PRICING, so the static table #265 demoted to a fallback is quietly carrying 100% of the load.Why this was invisible
#265 warns once if the hook raises. A hook that never raises and returns
Nonefor everything logs nothing at all. And because models that are in the static table still report a plausible cost, the run looks fine — the only symptom is that newer models show up as unpriced, which reads as "two missing table entries" rather than "the mechanism is dead".Those two conditions are genuinely different and were indistinguishable. Telling them apart cost me an afternoon of probing, which is the actual bug from a user's point of view.
The change
Warn once per run when the hook has returned
Nonefor more than one distinct model and has never priced anything, naming the models.Both thresholds are load-bearing:
Nonestays quiet — one model the provider cannot price is ordinary, and the table covers it.Only "
Nonefor everything, nothing ever priced" is reported. It sits next to the existing_pricing_hook_failed_warnedlatch and follows the same once-per-run pattern.What I deliberately did not do
I did not add
claude-opus-5/gpt-5.6-soltoDEFAULT_PRICING. I do not have authoritative per-token rates for them, and inventing numbers would produce confidently wrong costs — which is worse than unpriced, and is precisely what #265 set out to stop. That change wants real pricing data rather than a guess from me, which is why this isRefsand notFixes.I did not chase the SDK. If
token_pricesmoved rather than being dropped, repointing the hook would restore pricing for all 21 models at once and would be the better fix. I could not find a replacement on the model object; someone with visibility into the SDK changelog would know quickly.So #386 stays open for the pricing data and the SDK question. This PR only ensures the next person does not have to reverse-engineer why everything is unpriced.
Tests
Five tests, and the two negative cases matter as much as the positive one — a warning that fires on a single unpriced model would be noise, and noise gets ignored:
Nonefor 3 models, none pricedNonefor 1 modelNoneVerified by neutering the warning: 2 of 5 fail.
Validation
tests/test_enginematches the pre-existing baseline exactly — 442 failures before and after, measured by stashing the change rather than assumed. (My sandbox has no PyPI access sopytest-asynciois unavailable; those async tests error identically either way.)ruff checkandruff format --checkclean.One note on that measurement: my first attempt at the baseline was invalid —
git stashsilently failed on CRLF (fatal: CRLF would be replaced by LF) so I was comparing the tree against itself. Normalising line endings first gave the real numbers.Environment
Related
#265 built the hook and the
~$X (N unpriced)display — both working as designed; this is the layer beneath. #137 (fuzzy-match inget_pricing), #301 (doctorsurfacing per-model metadata — a natural home for pricing coverage).