Skip to content

fix(providers): restore live provider pricing by bumping the Copilot SDK - #418

Merged
Jason Robert (jrob5756) merged 3 commits into
mainfrom
fix/386-restore-live-provider-pricing
Aug 12, 2026
Merged

fix(providers): restore live provider pricing by bumping the Copilot SDK#418
Jason Robert (jrob5756) merged 3 commits into
mainfrom
fix/386-restore-live-provider-pricing

Conversation

@jrob5756

Copy link
Copy Markdown
Collaborator

Refs #386. Fixes the mechanism; the issue also asks about static-table entries, which this deliberately does not touch (see below).

What was actually wrong

CopilotProvider.get_model_pricing reads billing.token_prices. The pinned github-copilot-sdk 1.0.1 ships twoModelBilling classes:

filedeclaresfrom_dict
copilot/generated/rpc.pymultiplier, token_pricesparses tokenPrices
copilot/client.pymultiplier onlyreads multiplier, drops tokenPrices

client.list_models() returns the second one, which is what the hook reads. So the field never left the API and was still modelled in the generated types; the client dataclass just threw it away.

The effect is that the hook returned None for every model. The chain #265 built — workflow override, then hook, then static table, then unpriced — ran permanently on its fallback, and any model missing from DEFAULT_PRICING reported no cost at all. #388 made that visible; this makes it stop happening.

1.0.9 parses the field again. Verified against both wheels rather than taken on trust, and end to end: a model parsed by the real ModelInfo.from_dict yields $15.00 / $75.00 per million tokens for claude-opus-5, which is 1.5 credits per 1k tokens at 100 credits to the dollar.

Why the floor moves, not just the lock

>=1.0.0 is satisfied by 1.0.1, so an existing environment would keep dead pricing while reporting a perfectly healthy dependency. The feature genuinely requires 1.0.9, so the floor should say so.

Is the bump safe

I diffed every SDK method Conductor calls between the two versions by AST rather than by eye:

  • create_session and resume_session gained 16 optional kwargs and lost nothing
  • list_models, start, stop, abort, destroy, disconnect, on, rpc, send, RuntimeConnection.for_uri are unchanged
  • approve_all's invocation narrowed from dict[str, str] to a PermissionInvocation TypedDict

That last one is the ty error Frank Li (@franklixuefei) reported on #388. PermissionInvocation is a TypedDict, so it was always dict-compatible at runtime; the annotation now matches.

One thing worth flagging that the type error hid: 1.0.9 also gave approve_all behaviour it did not have before.

ifinvocation.get("managed_settings_enabled", False):
raiseRuntimeError("approve_all cannot be used when managed settings are enabled")

That flag is populated from enable_managed_settings, an opt-in kwarg on create_session that Conductor never passes, so the raise is unreachable. I recorded it in the handler docstring rather than leave the next person to trace it. If we ever do opt in, the blanket approve-all handler becomes a hard failure and will need to handle it.

The static table

claude-opus-5 and gpt-5.6-sol are still absent, on purpose. With the hook alive they price from the SDK, so #386's practical complaint is answered without anyone inventing per-token rates — which is the trade Frank Li (@franklixuefei) declined to make on #388, correctly. Guessed rates are worse than no rates.

Tests

The existing hook tests build their models from SimpleNamespace, so they assert what Conductor does with a billing object, never whether the SDK still hands it one. That is why 21 models silently went unpriced against a fully green suite.

Two new tests build the model through the SDK's own ModelInfo.from_dict. I confirmed they fail on 1.0.1 and pass on 1.0.9, so the next SDK release that drops the field fails the build instead of quietly reverting every run to static pricing.

Validation

6091 passed, 47 skipped. ruff check, ruff format --check and ty check src all clean.

get_model_pricing reads billing.token_prices, and github-copilot-sdk 1.0.1 --
the version uv.lock pinned -- parsed models.list with a hand-written
client.ModelBilling that declared only `multiplier` and discarded the
`tokenPrices` wire field. The field never left the API: generated/rpc.py still
models it, and only the client dataclass drops it. So the hook returned None
for every model, the chain #265 built (workflow override -> hook -> static
table -> unpriced) ran permanently on its fallback, and any model missing from
DEFAULT_PRICING reported no cost at all.
1.0.9 parses the field again. Verified against both wheels rather than assumed,
and end to end: a model parsed by the real ModelInfo.from_dict now yields
$15.00/$75.00 per million tokens for claude-opus-5 (1.5 credits per 1k tokens
at 100 credits to the dollar).
The floor moves to >=1.0.9 rather than relocking alone. The old >=1.0.0 is
satisfied by 1.0.1, so an existing environment would keep dead pricing while
reporting a healthy dependency.
Diffed every SDK method conductor calls between the two versions by AST rather
than by eye. create_session and resume_session gained 16 optional kwargs and
lost nothing; list_models, start, stop, abort, destroy, disconnect, on, rpc,
send and RuntimeConnection.for_uri are unchanged. The one narrowing is
approve_all's `invocation`, now a PermissionInvocation TypedDict, which the
default permission handler picks up. 1.0.9 also made approve_all raise when
managed_settings_enabled is set; that is unreachable here, since the flag comes
from the enable_managed_settings opt-in on create_session and conductor never
passes it. Recorded in the docstring so the next reader does not have to trace
it again.
No per-token rates invented for claude-opus-5 or gpt-5.6-sol. With the hook
alive they price from the SDK, which is the outcome #386 wanted without anyone
guessing numbers.
The existing hook tests build their models from SimpleNamespace, so they assert
what conductor does with a billing object rather than whether the SDK still
supplies one -- which is why 21 models went unpriced against a green suite. Two
tests now build the model through the SDK's own ModelInfo.from_dict. Confirmed
they fail on 1.0.1 and pass on 1.0.9, so the next release that drops the field
breaks the build instead of quietly reverting every run to static pricing.
6091 passed, ruff and ty clean.
Refs #386
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robertand others added 2 commits August 12, 2026 08:51
…ache reads
Review of the SDK bump turned up three things the bump itself introduced or
exposed.
approve_all is no longer unconditional in >=1.0.9. It abstains with
PermissionNoResult when the runtime marks a request managed_approval_required,
and raises when managed settings are enabled. The first is the dangerous one:
managed_approval_required is a wire field the server sets, not an opt-in
Conductor controls, so it can arrive on any run. The sentinel exists so a
second connected client can answer instead -- and Conductor is the only client,
so forwarding it leaves the request unanswered. The CLI blocks, idle recovery
spends five inert prompts on a session that is waiting for a permission
decision rather than for text, and ~9 minutes later the run dies with an error
naming the network, the SDK and the agent, none of which are at fault. Both
branches now decline explicitly and say why.
Declining rather than approving is deliberate. Managed approval is a policy
control; approving through it would convert a hang into a bypass.
The RuntimeError is guarded for a different reason than it looks. It is
unreachable today -- the flag comes from an enable_managed_settings opt-in on
create_session that Conductor never passes -- but the SDK catches exceptions
raised inside this callback, logs them to its own logger, and answers
PermissionDecisionUserNotAvailable itself. Since Conductor installs no logging
handlers, an unguarded raise would surface as every tool being silently denied.
Documenting the unreachability was not enough.
1.0.9 also deprecates token_prices.cache_price in favour of separate
cache_read_price and cache_write_price. The hook read only the deprecated
field, so a model shipping just the replacements priced cache reads at $0.00
with every other figure correct -- #386 one field over. Read the new fields,
keep the old one as the fallback, and populate cache writes, which were
hardcoded to zero on the claim that Copilot has no write rate.
The CI step that strips the bundled CLI binary had quietly become a no-op: the
1.0.9 wheel is pure-Python, 478KB against six ~90MB platform wheels, and
fetches the CLI on first use. A test reaching the real CLI path would now
download it inside a 10-minute job before hitting the auth hang the step
existed to prevent. COPILOT_SKIP_CLI_DOWNLOAD blocks that; the step stays for
venvs restored from an older lock, with a comment that matches reality.
CHANGELOG corrected on two counts: token_prices was restored in 1.0.7, not
1.0.9, so the floor is the tested version rather than the forced one, and the
user-visible effect now leads instead of trailing six lines of SDK internals.
Tests: the permission handler had no behavioural coverage at all -- every
reference asserted only that it was passed to create_session, which is exactly
why the abstention went unnoticed. All three branches are pinned now, plus both
cache-field shapes. Verified by mutation: reverting each of the three fixes
fails a test, and each fails only its own.
6096 passed, ruff and ty clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…provider-pricing
# Conflicts:
#	.github/workflows/ci.yml
#	CHANGELOG.md
@jrob5756

Copy link
Copy Markdown
CollaboratorAuthor

Merged main in and resolved two conflicts. One of them was load-bearing, so flagging it rather than burying it in a merge commit.

#390 hardened the "Remove bundled Copilot CLI binary" step to fail when find matches nothing:

removed=$(find .venv -path '*/copilot/bin/copilot*' -print -delete)if [ -z"$removed" ];thenecho"::error::No bundled Copilot CLI matched ..."exit 1
fi

The reasoning is right, and it is the same defect I found from the other end: a silent no-op reinstates the auth hang the step exists to prevent. But the assertion stops being true with this PR. SDK 1.0.1 ships per-platform wheels carrying copilot/bin/; 1.0.9 is a single pure-Python wheel that fetches the CLI on first use, so there is nothing to delete. Confirmed against both venvs.

So taking that side unchanged would have turned main red on the merge commit, for a condition that is correct rather than broken.

Resolution keeps everything #390 added — shell: bash, set -euo pipefail, and the pattern matching both lib/pythonX.Y/ and Windows Lib/ with the .exe suffix — and drops only the hard failure, replacing it with a message that says an empty match is expected. What now guarantees the original intent is COPILOT_SKIP_CLI_DOWNLOAD=1 in the test step's env, which blocks the on-demand fetch that replaced the bundled binary. The step is now only there for a venv restored from an older lock.

Exercised the resolved script against all three layouts before pushing: old POSIX (copilot/bin/copilot), old Windows (Lib/site-packages/copilot/bin/copilot.exe), and 1.0.9 with no binary. All exit 0, and the first two still delete.

The env var sits on the test step, so it covers the Windows matrix entry #390 added as well as the Linux ones.

CHANGELOG conflict was the ordinary both-sides-appended kind; kept both, no wording changed.

6129 passed locally after the merge, ruff and ty clean.

@jrob5756
Jason Robert (jrob5756) merged commit bb49740 into mainAug 12, 2026
11 checks passed
@jrob5756
Jason Robert (jrob5756) deleted the fix/386-restore-live-provider-pricing branch August 12, 2026 13:24
Sign up for freeto 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.

1 participant

@jrob5756