(MOT-4378) feat(provider-github-copilot): Copilot subscription models behind one GitHub sign-in - #746
Conversation
… behind one GitHub sign-in Sign in with GitHub once and the models a Copilot subscription grants appear in the model picker — the one-login story mainstream harnesses ship. Wire is OpenAI Chat Completions (provider-openrouter lineage); what is new is the worker-owned credential lifecycle: - Device-flow sign-in surface (login::start returns the code to type at the verification URL; login::poll stores the GitHub OAuth token in iii-state and refreshes the catalog the moment it lands). Machines already signed in through an editor need no login: read-only import of ~/.config/github-copilot/apps.json or pi's auth store, with env overrides for direct tokens and an opt-out for the import. - The long-lived GitHub token is exchanged at copilot_internal/v2/token for a short-lived Copilot bearer that also names the API endpoint (GitHub Enterprise tenants land on their own endpoint automatically). Cached in-memory, refreshed inside a 2-minute margin, invalidated when a stream dies with an auth error so the next call re-exchanges. - Catalog ids prefixed copilot/ — Copilot serves several vendors' models under bare ids that would collide with the sibling providers'. Admission = chat type + tool_calls + plan-enabled (model_picker_enabled); windows and capability flags come from the listing's capabilities tree. No pricing: subscriptions meter in premium requests, so records carry no per-token cost. - Client-identity headers on every call (integration id, editor version), with X-Initiator: agent so agent turns are billed per Copilot's convention. - Errors on the shared taxonomy with subscription semantics: 401 drops the cached bearer and maps auth_expired; 403 (no access / model not authorized) is permanent; numeric error.code envelopes win over the transport status. - No count_tokens surface: no tokenizer endpoint upstream, and one local tokenizer cannot honestly meter several vendors' vocabularies — the harness falls back to its own estimate. The integration suite drives the real engine and router with a stub upstream; a ready-bearer env path short-circuits the exchange so no external API is called, and a suite-wide lock serializes the tests' credential env management.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a complete GitHub Copilot Rust provider worker. It includes credential resolution, device-flow login, bearer exchange, live model discovery, router registration, OpenAI-compatible requests, SSE streaming, schemas, documentation, and integration tests. ChangesGitHub Copilot provider
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 57 skipped (no docs/).
Four for four. Nicely done. |
…h client id The device/code endpoint answered "Not Found" to the previous id; the live flow now mints codes. Verified against three independent public Copilot client implementations.
…an can actually call Live verification against a real subscription found the catalog full of models the account may not use. Two problems, both fixed here. Admission gated on `model_picker_enabled`, which turns out to be an editor-side picker preference: it reads false for every row on accounts that have never toggled models in an editor, so the catalog came back empty. The real listing gates are a non-disabled `policy.state` and a declared `/chat/completions` endpoint; the editor's internal feature models (preview rows with no picker category — search, compaction, exec agents) are dropped as well. That still over-admits, because entitlement is invisible in the API. Two models can match on every field it exposes — same vendor, same `policy: enabled`, same picker category — and one answers while the other returns `model_not_supported`; a free or educational plan carries the base families but no premium requests. Discovery therefore verifies instead of guessing: each admitted model gets a one-token probe, four at a time, and only what answered is reconciled. Refusals are rejected before generation so they cost no quota, and successes cost a single token on models the plan already includes. On the verification account that is 11 usable models out of 53 listed, and enabling more upstream needs no code change — the next refresh picks them up. A refusal between refreshes now also self-heals: the stream returns an actionable permanent error naming the model and the row is pruned from the catalog, so the picker never offers the same dead model twice.
…dpoint outranking the sign-in's The declaration carried `defaults.api_url`, and llm-router's resolve step falls back to that value whenever the operator has not set one — so the declared generic host always won and the endpoint named by the token exchange was never used. Individual accounts happen to route through the generic host, which is why it worked; a GitHub Enterprise tenant, whose exchange reply names its own endpoint, would have been pinned to the wrong host despite the README promising otherwise. Dropping the default restores the documented precedence: operator override, then the endpoint the sign-in names, then the public default as a last resort. The console form renders a fixed field set per provider, so this changes no operator-facing field — only which endpoint a never-configured slice resolves to.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (7)
provider-github-copilot/src/stream_fn.rs (2)
180-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated
matcharms.Both arms call
pump_with_auth_invalidationwith the same arguments.abort_regalready has the type the parameter needs.♻️ Proposed simplification
- match abort_reg { - Some(g) => { - pump_with_auth_invalidation( - rx, - sink, - PING_INTERVAL, - Some(g), - &cache_on_auth, - iii, - &model_id, - ) - .await; - } - None => { - pump_with_auth_invalidation( - rx, - sink, - PING_INTERVAL, - None, - &cache_on_auth, - iii, - &model_id, - ) - .await - } - } + pump_with_auth_invalidation( + rx, + sink, + PING_INTERVAL, + abort_reg, + &cache_on_auth, + iii, + &model_id, + ) + .await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/stream_fn.rs` around lines 180 - 205, Collapse the duplicated match on abort_reg in the stream setup by calling pump_with_auth_invalidation once and passing abort_reg directly as its abort-registry argument, preserving all other arguments and the existing await behavior.
345-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not verify cache invalidation, and it contains a dead block.
Lines 350-363 build
seededand then discard it withlet _ = &seeded;. The comment states that the cache cannot be seeded from here. The assertions only check that the error frame reaches the sink, so the test nameauth_expired_error_invalidates_the_bearer_cachedescribes behavior that is not asserted.Choose one of two fixes:
- Add a
#[cfg(test)]seed method onBearerCache, seed a bearer, run the pump, and assert the cache is empty afterwards.- Delete the dead block and rename the test to describe what it checks, for example
auth_expired_error_frame_is_forwarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/stream_fn.rs` around lines 345 - 399, Make the test accurately reflect its coverage: either add a test-only BearerCache seeding method, seed the cache in auth_expired_error_invalidates_the_bearer_cache, and assert it is empty after pump_with_auth_invalidation, or remove the unused seeded block and rename the test to auth_expired_error_frame_is_forwarded.provider-github-copilot/src/upstream.rs (2)
42-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
data_linediscards all but the lastdata:line of a block.The SSE specification joins multiple
data:lines in one block with\nto form the payload. This function keeps only the final line. If any gateway reachable throughapi_urlsplits a JSON chunk over severaldata:lines, the payload becomes invalid JSON andrun_upstreamdrops the chunk silently at Line 158.♻️ Concatenate the data lines instead
-fn data_line(block: &str) -> Option<&str> { - block - .lines() - .filter_map(|l| { - let rest = l.trim_end_matches('\r').strip_prefix("data:")?; - Some(rest.strip_prefix(' ').unwrap_or(rest)) - }) - .next_back() -} +fn data_line(block: &str) -> Option<String> { + let parts: Vec<&str> = block + .lines() + .filter_map(|l| { + let rest = l.trim_end_matches('\r').strip_prefix("data:")?; + Some(rest.strip_prefix(' ').unwrap_or(rest)) + }) + .collect(); + if parts.is_empty() { + return None; + } + Some(parts.join("\n")) +}Update the caller and the
data == "[DONE]"comparison accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/upstream.rs` around lines 42 - 50, Update data_line to concatenate all SSE data: lines in order with newline separators instead of returning only the final line, and adjust its return type or ownership as needed. Update the caller in run_upstream to consume the combined payload and perform the [DONE] comparison against the reconstructed data value.
187-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stub reads the request once, which can make these tests flaky.
sock.read(&mut buf)returns after the first TCP segment. The POST body can arrive in a later segment. The stub then writes the response and callsshutdown()while the client is still writing, which can produce a connection reset on some platforms. Read until the end of the request head and the declaredcontent-length, or at least loop the read until the body is consumed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/upstream.rs` around lines 187 - 199, Update the async stub function to read the complete HTTP request before responding: parse the headers to find the end of the request head and declared content length, then loop reads until the full body is consumed. Only write the response and shut down the socket after the request is complete, preserving the existing response and endpoint behavior.provider-github-copilot/src/config.rs (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared
api_urlprecedence.
discovery.rslines 203-207 repeats this exact chain (resolved.api_url→bearer.api_url→DEFAULT_API_URL). A shared helper keeps the stream path and the discovery path from diverging later.♻️ Suggested helper
// in exchange.rs or config.rs pub fn effective_api_url(resolved: &ProviderResolveResponse, bearer: &CopilotBearer) -> String { resolved .api_url .clone() .or_else(|| bearer.api_url.clone()) .unwrap_or_else(|| DEFAULT_API_URL.to_string()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/config.rs` around lines 50 - 54, Extract the repeated resolved.api_url → bearer.api_url → DEFAULT_API_URL precedence into a shared effective_api_url helper in the existing exchange.rs or config.rs module. Update the current config construction and the discovery.rs path to call this helper, preserving the same fallback behavior and avoiding duplicated chains.provider-github-copilot/src/register.rs (1)
106-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tracingfor operational output instead ofprintln!andeprintln!.Cargo.tomllines 31-32 declaretracingandtracing-subscriberwith theenv-filterfeature, andmain.rsinitializes a subscriber. Direct stdout and stderr writes bypass that pipeline, so operators cannot filter these messages by level or target, and the output carries no structured fields.
provider-github-copilot/src/register.rs#L106-L110: replace the registration successprintln!withtracing::info!and the retryeprintln!withtracing::warn!.provider-github-copilot/src/discovery.rs#L216-L219: replace the plan-availabilityprintln!withtracing::info!and recordcountandlistedas fields.provider-github-copilot/src/router_client.rs#L86-L92: replace the prune failureeprintln!withtracing::warn!and the prune successprintln!withtracing::info!, recordingmodel_idas a field.Apply the same change to the remaining
println!andeprintln!sites in these files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/register.rs` around lines 106 - 110, Replace direct println! and eprintln! operational output with tracing macros across the specified files: in provider-github-copilot/src/register.rs lines 106-110, use tracing::info! for successful registration and tracing::warn! for retry failures; in provider-github-copilot/src/discovery.rs lines 216-219, use tracing::info! for plan availability and record count and listed fields; in provider-github-copilot/src/router_client.rs lines 86-92, use tracing::warn! for prune failures and tracing::info! for successful pruning with model_id; update all remaining println! and eprintln! sites in these three files similarly.provider-github-copilot/src/router_client.rs (1)
76-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the silent early returns in
prune_model.The list call failure at line 77 and the parse failure at line 83 return with no output. The reconcile failure at line 87 logs. A prune that never happens leaves an uncallable model in the picker with no diagnostic trace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-github-copilot/src/router_client.rs` around lines 76 - 84, Update prune_model’s early-return branches for the failed model-list request and failed models parsing to emit diagnostic logs before returning. Match the existing logging approach used for the reconcile failure, clearly distinguishing the list-call and parse failures while preserving the current return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@provider-github-copilot/README.md`:
- Around line 19-25: Update the shell command code fence in the README around
the GitHub Copilot login examples to use the bash language tag by changing its
opening fence to ```bash, while leaving the example contents unchanged.
- Around line 121-125: Update the CLI environment-variable documentation in the
README to match the actual variable bound to --url in main.rs: document III_URL
instead of III_WS_URL, while preserving the described default and flag behavior.
In `@provider-github-copilot/src/auth.rs`:
- Around line 123-130: Update the credential resolution flow around
load_stored_oauth so the non-empty GITHUB_COPILOT_OAUTH_TOKEN is checked before
persisted OAuth state, while preserving GITHUB_COPILOT_TOKEN as the
highest-priority source. Revise the resolution-chain documentation and tests to
cover the environment OAuth override over stored credentials.
In `@provider-github-copilot/src/discovery.rs`:
- Around line 85-132: Bound entitlement probing in retain_callable by caching
each model’s probe result per bearer identity, with re-probing only after a long
interval or an explicit refresh; preserve the existing model-order behavior.
Update the refresh flow so router::ready rebinds skip the probe sweep and rely
on router_client.rs::prune_model for self-healing, while boot and explicitly
requested refreshes retain the bounded probe behavior.
- Around line 22-27: Update models_url so custom api_url values never fall back
to the public GitHub host: derive the models endpoint from the configured
origin/path, or return an error for unrecognized URL shapes. Preserve the
existing /chat/completions mapping, ensure callers such as the models request
handle the chosen error behavior, and update the models_url unit test
accordingly.
- Around line 224-227: Update the FetchOutcome::AuthFailed branch in the
discovery flow to invalidate the cached bearer via BearerCache::invalidate
before reconciling and returning. Preserve the existing router_client::reconcile
call and Ok(0) result.
In `@provider-github-copilot/src/errors.rs`:
- Around line 31-33: Update the inline comment for the Some(403) =>
ErrorKind::Permanent match arm to state that Copilot rejected the call because
the current plan does not authorize it, matching the module header; leave the
error mapping unchanged.
- Around line 104-114: Update is_model_not_supported so its non-JSON fallback no
longer matches model_not_supported anywhere in arbitrary text; restrict
detection to the expected JSON envelope or require the literal’s surrounding
quoted form. Preserve the existing parsed /error/code check and return false for
unrelated upstream text.
In `@provider-github-copilot/src/login.rs`:
- Around line 118-127: Update the device-flow response handling around
LoginPollResponse to preserve GitHub’s slow_down interval instead of collapsing
it into pending. Extend the response model and LOGIN_POLL_DESC to expose the
revised interval or a distinct slow_down result, update callers to use it, and
regenerate the provider.github-copilot.login.poll golden schema.
In `@provider-github-copilot/src/register.rs`:
- Around line 229-234: Handle the Result returned by register_trigger in
register_provider instead of discarding it: log registration failures and
propagate the error so provider registration fails when the router::ready rebind
handler cannot be installed. Preserve the existing trigger configuration and
success path.
In `@provider-github-copilot/src/router_client.rs`:
- Around line 69-93: Protect the read-modify-write sequence in prune_model with
a process-wide async mutex, acquiring the guard before calling
router::models::list and holding it through reconcile. This must serialize
concurrent prune_model calls; use a module-level tokio::sync::Mutex and preserve
the existing filtering and error-reporting behavior.
In `@provider-github-copilot/src/sse.rs`:
- Around line 100-116: Update the function-call conversion loop that parses
fc.args_json so truncated or invalid JSON is not converted to Value::Null and
emitted in ContentBlock::FunctionCall. Reject the incomplete call before pushing
it, or emit explicitly degraded arguments together with a trace error, ensuring
plan_calls never receives null arguments for partial provider-side tool streams.
- Around line 305-345: Track which tool-call indices have already emitted
FunctioncallStart, rather than using only state.open_block in the tool-call
delta handling. Emit Start once per index while still closing and switching the
active open block for interleaved calls, and add a test covering delta order 0,
1, 0 that asserts exactly two functioncall_start events.
In `@provider-github-copilot/src/upstream.rs`:
- Around line 121-142: In the SSE parsing loop around find_block_end, enforce a
maximum size for buf after appending each chunk; if the limit is exceeded, send
a synthetic_error_event describing the oversized or unterminated SSE block with
ErrorKind::Transient and return. Keep normal block draining and stream-read
error handling unchanged.
- Around line 92-105: Update the non-success response handling around
resp.text() and synthetic_error_event so the upstream error body is truncated to
a bounded length before being used as the user-facing msg. Preserve status-based
classification and the existing fallback for empty bodies, and ensure the
bounded message is what gets sent through tx.
- Around line 172-178: Update the connection-close fallback after the streaming
loop to send AssistantMessageEvent::Stop before sending
AssistantMessageEvent::Done, matching the [DONE] path’s completion framing while
preserving the existing final message payload.
- Around line 20-38: Update the injected HTTP client configuration in
register.rs near make_stream to add a read_timeout close to the existing
30-second heartbeat window, while preserving the current connect_timeout and
client reuse through spawn_upstream. This must bound silent send/body-chunk
reads in run_upstream without changing the receiver-closure handling.
In `@provider-github-copilot/src/wire/names.rs`:
- Around line 4-10: Replace the lossy encode_tool_name/decode_tool_name
replacement scheme with a reversible encoding, or have decode_tool_name resolve
against the encoded tool names actually emitted. Ensure names containing both
"::" and "__" round-trip exactly before populating function_id and preserve
correct routing in the SSE function-call path.
In `@provider-github-copilot/tests/integration.rs`:
- Around line 238-267: Update stub_upstream’s per-connection handler to read the
complete HTTP request before routing: continue reading until the header
terminator is found, parse Content-Length, then consume the declared body bytes
before evaluating the GET /models and premium-only conditions. Preserve the
existing response selection and connection shutdown behavior.
---
Nitpick comments:
In `@provider-github-copilot/src/config.rs`:
- Around line 50-54: Extract the repeated resolved.api_url → bearer.api_url →
DEFAULT_API_URL precedence into a shared effective_api_url helper in the
existing exchange.rs or config.rs module. Update the current config construction
and the discovery.rs path to call this helper, preserving the same fallback
behavior and avoiding duplicated chains.
In `@provider-github-copilot/src/register.rs`:
- Around line 106-110: Replace direct println! and eprintln! operational output
with tracing macros across the specified files: in
provider-github-copilot/src/register.rs lines 106-110, use tracing::info! for
successful registration and tracing::warn! for retry failures; in
provider-github-copilot/src/discovery.rs lines 216-219, use tracing::info! for
plan availability and record count and listed fields; in
provider-github-copilot/src/router_client.rs lines 86-92, use tracing::warn! for
prune failures and tracing::info! for successful pruning with model_id; update
all remaining println! and eprintln! sites in these three files similarly.
In `@provider-github-copilot/src/router_client.rs`:
- Around line 76-84: Update prune_model’s early-return branches for the failed
model-list request and failed models parsing to emit diagnostic logs before
returning. Match the existing logging approach used for the reconcile failure,
clearly distinguishing the list-call and parse failures while preserving the
current return behavior.
In `@provider-github-copilot/src/stream_fn.rs`:
- Around line 180-205: Collapse the duplicated match on abort_reg in the stream
setup by calling pump_with_auth_invalidation once and passing abort_reg directly
as its abort-registry argument, preserving all other arguments and the existing
await behavior.
- Around line 345-399: Make the test accurately reflect its coverage: either add
a test-only BearerCache seeding method, seed the cache in
auth_expired_error_invalidates_the_bearer_cache, and assert it is empty after
pump_with_auth_invalidation, or remove the unused seeded block and rename the
test to auth_expired_error_frame_is_forwarded.
In `@provider-github-copilot/src/upstream.rs`:
- Around line 42-50: Update data_line to concatenate all SSE data: lines in
order with newline separators instead of returning only the final line, and
adjust its return type or ownership as needed. Update the caller in run_upstream
to consume the combined payload and perform the [DONE] comparison against the
reconstructed data value.
- Around line 187-199: Update the async stub function to read the complete HTTP
request before responding: parse the headers to find the end of the request head
and declared content length, then loop reads until the full body is consumed.
Only write the response and shut down the socket after the request is complete,
preserving the existing response and endpoint behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c31f38bf-2386-4f6e-937a-2377aed1772f
⛔ Files ignored due to path filters (1)
provider-github-copilot/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/release-workers.yamlREADME.mdprovider-github-copilot/Cargo.tomlprovider-github-copilot/README.mdprovider-github-copilot/build.rsprovider-github-copilot/config.yamlprovider-github-copilot/iii-permissions.yamlprovider-github-copilot/iii.worker.yamlprovider-github-copilot/prompts/identity.txtprovider-github-copilot/src/auth.rsprovider-github-copilot/src/catalog.rsprovider-github-copilot/src/config.rsprovider-github-copilot/src/discovery.rsprovider-github-copilot/src/errors.rsprovider-github-copilot/src/exchange.rsprovider-github-copilot/src/lib.rsprovider-github-copilot/src/login.rsprovider-github-copilot/src/main.rsprovider-github-copilot/src/manifest.rsprovider-github-copilot/src/register.rsprovider-github-copilot/src/request.rsprovider-github-copilot/src/router_client.rsprovider-github-copilot/src/sse.rsprovider-github-copilot/src/state.rsprovider-github-copilot/src/stream_fn.rsprovider-github-copilot/src/surface.rsprovider-github-copilot/src/upstream.rsprovider-github-copilot/src/wire/messages.rsprovider-github-copilot/src/wire/mod.rsprovider-github-copilot/src/wire/names.rsprovider-github-copilot/src/wire/tools.rsprovider-github-copilot/tests/golden/schemas/provider.github-copilot.abort.jsonprovider-github-copilot/tests/golden/schemas/provider.github-copilot.login.poll.jsonprovider-github-copilot/tests/golden/schemas/provider.github-copilot.login.start.jsonprovider-github-copilot/tests/golden/schemas/provider.github-copilot.on_router_ready.jsonprovider-github-copilot/tests/golden/schemas/provider.github-copilot.refresh_models.jsonprovider-github-copilot/tests/golden/schemas/provider.github-copilot.stream.jsonprovider-github-copilot/tests/integration.rsprovider-github-copilot/tests/schemas.rsprovider-github-copilot/tests/support/mod.rs
| pub fn encode_tool_name(name: &str) -> String { | ||
| name.replace("::", "__") | ||
| } | ||
|
|
||
| pub fn decode_tool_name(name: &str) -> String { | ||
| name.replace("__", "::") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for function ids containing double underscores and for shared name-encoding helpers.
rg -n --type=rs -C2 'function_id\s*[:=]\s*"' | rg -n '__'
rg -n --type=rs -C3 'fn (encode|decode)_tool_name'Repository: iii-hq/workers
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant files"
fd -a 'names\.rs|sse\.rs' . | sed 's#^\./##'
echo
echo "Search for function_id assignments and tool name helpers"
if command -v rg >/dev/null 2>&1; then
rg -n -C2 'function_id\s*[:=]\s*["'\'']' -g '*.rs' . | sed -n '1,220p'
echo
rg -n -C3 'fn (encode|decode)_tool_name|encode_tool_name|decode_tool_name' -g '*.rs' .
else
grep -RIn -n -C2 'function_id' -g '*.rs' . | sed -n '1,220p'
echo
grep -RIn -n -C3 'encode_tool_name\|decode_tool_name' -g '*.rs' .
fi
echo
echo "Inspect names.rs"
path=$(fd -a 'names\.rs' . | head -n1 || true)
if [ -n "${path:-}" ]; then
nl -ba "$path" | sed -n '1,80p'
fi
echo
echo "Inspect sse.rs around function call"
path2=$(fd -a 'sse\.rs' . | head -n1 || true)
if [ -n "${path2:-}" ]; then
nl -ba "$path2" | sed -n '300,350p'
fi
echo
echo "Probe replace behavior"
python3 - <<'PY'
vals = ["shell::run__fast", "a__b__c", "no-underscore", "x__"]
for v in vals:
enc = v.replace("::", "__")
dec = enc.replace("__", "::")
print(f"{v!r} -> enc={enc!r} -> dec={dec!r} reversible={v==dec}")
PYRepository: iii-hq/workers
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "provider-github-copilot names.rs"
nl -ba provider-github-copilot/src/wire/names.rs | sed -n '1,80p'
echo
echo "provider-github-copilot sse.rs relevant area"
nl -ba provider-github-copilot/src/sse.rs | sed -n '300,345p'
echo
echo "Search for function ids containing '__' in Rust source"
python3 - <<'PY'
from pathlib import Path
pat = []
for p in Path('.').rglob('*.rs'):
if any(part.startswith('.') for part in p.parts):
continue
text = p.read_text(errors='replace')
for i, line in enumerate(text.splitlines(), 1):
if 'function_id' in line and '__' in line:
pat.append((str(p), i, line[:220]))
print(len(pat))
for item in pat[:120]:
print(f"{item[0]}:{item[1]}: {item[2]}")
PY
echo
echo "Search for encode/decode tool helpers outside shared names"
python3 - <<'PY'
from pathlib import Path
for p in Path('.')rglob('provider-github-copilot/src/**').filter(lambda p: p.suffix == '.rs'):
text = p.read_text(errors='replace')
for target in ['encode_tool_name', 'decode_tool_name']:
if target in text:
print(f"{p}: contains {target}")
PYRepository: iii-hq/workers
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
shopt -s globstar extglob
echo "provider-github-copilot names.rs"
awk '{printf "%6d\t%s\n", NR, $0}' provider-github-copilot/src/wire/names.rs | sed -n '1,80p'
echo
echo "provider-github-copilot sse.rs relevant area"
awk '{printf "%6d\t%s\n", NR, $0}' provider-github-copilot/src/sse.rs | sed -n '300,345p'
echo
echo "Search for function_ids containing '__' in Rust source"
python3 - <<'PY'
from pathlib import Path
hits=[]
for p in list(Path('.').glob('**/*.rs')):
if any(part.startswith('.') for part in p.parts):
continue
try:
text=p.read_text(errors='replace')
except Exception:
continue
for i,line in enumerate(text.splitlines(),1):
if 'function_id' in line and '__' in line:
hits.append((str(p), i, line[:220]))
print(len(hits))
for item in hits[:160]:
print(f"{item[0]}:{item[1]}: {item[2]}")
PY
echo
echo "Search for encode_tool_name/decode_tool_name in provider-github-copilot Rust source"
python3 - <<'PY'
from pathlib import Path
for p in list(Path('./provider-github-copilot').glob('**/*.rs')):
try:
text=p.read_text(errors='replace')
except Exception:
continue
for target in ['encode_tool_name','decode_tool_name']:
if target in text:
print(f"{p}: contains {target}")
PYRepository: iii-hq/workers
Length of output: 4343
🌐 Web query:
OpenAI function call tool name regex ^[a-zA-Z0-9_-]{1,64}$
💡 Result:
The OpenAI API strictly requires function names to match the regular expression ^[a-zA-Z0-9_-]{1,64}$ [1][2][3]. This validation pattern ensures that function names contain only ASCII letters (a-z, A-Z), digits (0-9), underscores (_), and dashes (-) [4][5][6], with a total length between 1 and 64 characters [4][7][8]. If a function name fails to comply with these constraints, the OpenAI API will reject the request, typically returning a 400 Bad Request error indicating that the name does not match the required pattern [2][3]. Developers integrating with the OpenAI API—or using frameworks that wrap it—must ensure their function naming conventions adhere to this format to avoid runtime validation errors [9][1][2].
Citations:
- 1: .Net: The function name validation in tool call is different from the API spec microsoft/semantic-kernel#5320
- 2: .Net: Function names are triggering errors on Azure OpenAI microsoft/semantic-kernel#5825
- 3: https://portkey.ai/error-library/input-validation-error-10049
- 4: https://apis.io/schemas/vapi/vapi-openaifunction/
- 5: https://github.com/openai/openai-python/blob/5e8f09c2/src/openai/types/shared/function_definition.py
- 6: https://github.com/openai/openai-java/blob/main/openai-java-core/src/main/kotlin/com/openai/models/FunctionDefinition.kt
- 7: https://javadoc.io/static/com.openai/openai-java/4.6.1/com/openai/models/FunctionDefinition.html
- 8: https://docs.rs/openai-openapi-types/latest/openai_openapi_types/struct.FunctionObject.html
- 9: fix(mcp): alias-rewrite tool names rejected by OpenAI-style regex agentscope-ai/QwenPaw#4958
🌐 Web query:
GitHub Copilot Chat API function call tool call result tool name schema id name
💡 Result:
GitHub Copilot Chat implements tool calling through a structured API that allows Large Language Models (LLMs) to request the execution of predefined tools (functions) [1][2][3]. The architecture relies on an orchestration loop—often managed by the Copilot CLI or the VS Code extension host—which interprets these requests, executes the corresponding tool, and returns the result to the model for further processing [4][2][5]. Tool Call Structure and Schema When a model determines a tool should be used, it issues a tool call request containing specific identifying information [4][2]. Key components include: - Name: A unique string identifier for the tool (e.g., "bash", "grep", or custom names) [6]. - ID: A unique identifier for the specific tool invocation (e.g., a UUID), used to correlate the request with the eventual result [7][8][9]. - Arguments: A JSON-encoded string representing the input parameters, which must conform to the schema defined for that tool [5][8][9]. Tools are typically defined using JSON schemas [10][11]. In the Copilot SDK and VS Code API, this includes an inputSchema that details the expected properties, their types, and which are required [2][11][12]. Tool Call Result When a tool is invoked, the handler must return a result to the Copilot orchestrator to be passed back to the LLM [10][4][2]. This result structure generally includes: - Tool Use ID (tool_use_id): The original ID from the request, ensuring the result is correctly routed [9][12]. - Content: The payload returned by the tool, which can be a simple string, a structured object (e.g., ToolResultObject), or specific types like terminal output, images, or references [10][7][9]. - Success Status: An indicator (e.g., a boolean success field) that reports whether the tool execution completed as expected [6]. Implementation - Copilot SDK (Node.js): Developers use the defineTool function to register tools, providing Zod schemas for type-safe definitions [10][13]. The SDK automatically handles the invocation of your handler when the model requests the tool [10][13]. - VS Code Extension API: Extension developers register tools via contributes.languageModelTools in package.json [11]. The implementation involves creating a class that adheres to vscode.LanguageModelTool, which must process the input parameters and return a vscode.LanguageModelToolResult [2][11]. The overall cycle consists of the model requesting a tool, the orchestrator executing it via the registered handler, and the orchestrator feeding the resulting output back into the chat context as a "tool_result" block [4][2][9].
Citations:
- 1: https://devblogs.microsoft.com/visualstudio/function-calling-is-now-enabled-in-github-copilot/
- 2: https://github.com/microsoft/vscode-docs/blob/main/api/extension-guides/ai/tools.md
- 3: https://code.visualstudio.com/docs/chat/chat-tools
- 4: https://github.com/github/copilot-sdk/blob/main/docs/features/agent-loop.md
- 5: https://github.com/microsoft/vscode-copilot-chat/blob/main/src/extension/intents/node/toolCallingLoop.ts
- 6: https://github.com/github/copilot-sdk/blob/40887393/nodejs/src/generated/rpc.ts
- 7: https://github.com/github/copilot-sdk/blob/584a239e/nodejs/src/generated/rpc.ts
- 8: https://github.com/microsoft/vscode-copilot-chat/blob/a9900c5f/src/platform/networking/common/fetch.ts
- 9: https://github.com/microsoft/vscode-copilot-chat/blob/5863f5a7/src/platform/endpoint/node/messagesApi.ts
- 10: https://github.com/github/copilot-sdk/blob/ea90f076/nodejs/README.md
- 11: https://github.com/microsoft/vscode-copilot-chat/blob/main/docs/tools.md
- 12: https://github.com/microsoft/vscode-copilot-chat/blob/a9900c5f/src/platform/networking/common/anthropic.ts
- 13: https://github.com/github/copilot-sdk/tree/main/nodejs
Make the name decode reversible before it populates function_id.
encode_tool_name("directory::__on_worker_added") becomes directory______on_worker_added, and decode_tool_name turns that into directory:::on::worker:::added. If such iii function ids ever reach this provider, provider-github-copilot/src/sse.rs:331 routes the function call to a wrong id because :: and __ encode to the same symbol. Key the decode by the encoded tool names that were actually sent, or use a reversible escape/encoding instead of a direct replace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-github-copilot/src/wire/names.rs` around lines 4 - 10, Replace the
lossy encode_tool_name/decode_tool_name replacement scheme with a reversible
encoding, or have decode_tool_name resolve against the encoded tool names
actually emitted. Ensure names containing both "::" and "__" round-trip exactly
before populating function_id and preserve correct routing in the SSE
function-call path.
…atalog writes, and errors - Serialize whole-slice catalog writes. `reconcile` replaces the entire provider slice, so a prune's read-modify-write racing another prune or a refresh silently dropped one result; both paths now go through one lock. - Type the login poll status as an enum and keep `slow_down` distinct from `pending`, carrying the extra wait GitHub asks callers to add — the two were collapsed, so a poller kept hammering at its original interval. - Stop putting upstream bodies in error text: the unrecognised access-token reply now reports only its keys, and the token-exchange rejection truncates what it quotes. Either could have carried a credential into logs and error frames. - An empty stored registration token now reads as absent rather than being presented and rejected as a mismatch, and a failed state read is logged instead of silently looking like "never signed in". - Only plausible HTTP statuses take the status path when classifying a numeric error envelope; application codes and values past u16 were truncated into one. - The integration stub reads a complete request before routing on it, rather than assuming one read captured the body. Skipped, with reasons: the `::`↔`__` tool-name encoding and the null-arguments-on-unparseable-tool-call behaviour are rig-wide conventions shared with the harness and sibling providers (changing one provider would break the symmetry); `models_url`'s fallback, the empty-tool-content case, and the identity prompt are shared-lineage code whose shape belongs to every provider at once.
…hten the failure paths The entitlement probe ran on every refresh — at boot, on each `router::ready` rebind, and after every sign-in — and a probe that succeeds on a premium model spends a premium request, not the single token the comment claimed. Verdicts now persist with a 24h TTL, so a model is probed once and answered from cache after that; a restart loop costs nothing. The self-healing prune records its verdict too, so a model that failed mid-refresh is never probed again either. Also from review: - Keep a custom `api_url`'s origin when deriving the listing endpoint. Anything not ending in `/chat/completions` fell back to the public host, so an operator routing Copilot through a gateway had the listing request — and the bearer on it — sent to GitHub instead. - Invalidate the cached bearer when the models fetch returns 401/403. The dead bearer was otherwise reused by every stream call until it hit the expiry margin. - Let an explicit `GITHUB_COPILOT_OAUTH_TOKEN` outrank the persisted token. A past sign-in masked the override, which could keep using a stale account. - Match `model_not_supported` on the error envelope only. The substring fallback also fired on a body that merely echoed the phrase, and this verdict prunes a model — a false positive removed a working one. - Truncate an oversized upstream error body instead of forwarding a whole gateway HTML page as the error frame, and bound the un-framed SSE buffer so a stream that never sends a boundary fails fast instead of growing while every chunk rescans it. - Emit `Stop` before `Done` when a stream ends without the `[DONE]` sentinel, so both endings look the same downstream. - Correct the 403 comment: on this wire it is plan authorization, not moderation. Skipped: the `::`↔`__` tool-name encoding and null-arguments-on-a- truncated-tool-call are rig-wide conventions shared with the harness and every sibling provider; per-index tool-call start/end events are the shared SSE state machine, and this wire delivers tool-call deltas grouped by index; the stream client's timeouts are the router's stream budget to own, as in the sibling providers.
|
Worked through all 19. Fixed in Fixed
Skipped
Two findings pointed at the framework rather than this worker, so they are not addressed here: |
…nce, and probe description The README named III_WS_URL, but the CLI binds --url to III_URL, so a user setting only the documented variable got the default URL. Tags the sign-in fence as bash, and rewrites the discovery section, which still claimed a probe costs one token per refresh — verdicts are cached for 24h now, and a successful probe on a premium model spends a premium request.
|
Correction to my previous comment: I listed the two README items as fixed, but had not actually applied them. They are in That commit also rewrites the discovery section, which still claimed a probe costs one token per refresh. It no longer does: verdicts are cached for 24 hours, and a probe that succeeds on a premium model spends a premium request. On the failing |
1 participant
Sign in with GitHub once (device code) and the models a Copilot subscription grants appear in the model picker — the same one-login story pi and other mainstream harnesses ship.
What
New
provider-github-copilotworker behind llm-router. Wire = OpenAI Chat Completions (provider-openrouter lineage); auth = subscription OAuth with a worker-owned credential lifecycle (provider-openai-codex patterns, no vault dependency):provider::github-copilot::login::startreturns the code to enter at the verification URL;login::pollstores the GitHub OAuth token in iii-state and refreshes the catalog the moment it lands. Machines already signed in through an editor need no login: read-only import of~/.config/github-copilot/apps.jsonor pi's auth store (GITHUB_COPILOT_NO_LOCAL_IMPORT=1opts out;GITHUB_COPILOT_OAUTH_TOKEN/GITHUB_COPILOT_TOKENsupply tokens directly).copilot_internal/v2/tokenfor a short-lived Copilot bearer (~25 min) that also names the API endpoint, so GitHub Enterprise tenants land on their own endpoint automatically. The bearer is cached in-memory, refreshed proactively inside a 2-minute margin, and invalidated when a stream dies with an auth error.gpt-5.2,claude-sonnet-4.6) that would collide with the sibling single-vendor providers'. Catalog ids arecopilot/<id>; the prefix is stripped on the wire.type: chat+tool_calls+ plan-enabled (model_picker_enabled: falserows fail with "model not supported" and would be dead picker rows). Windows, ceilings, and flags come from the listing'scapabilitiestree. No pricing: subscriptions meter in premium requests, so records carry no per-token cost andusage.cost_usdstays unset.X-Initiator: agentso agent-initiated turns are billed per Copilot's convention.auth_expired; 403 (no Copilot access / model not authorized) ispermanent; numericerror.codeenvelopes win over the transport status.count_tokenssurface — no tokenizer endpoint upstream, and one local tokenizer cannot honestly meter several vendors' vocabularies; the harness falls back to its own estimate.Carries the transport hardening from the openrouter provider (byte-buffered SSE decode for CRLF framing and split multibyte characters, bounded tool-call indices, credential-redacting Debug).
Also adds the worker to the modules table and
release-workers.yaml.Testing
cargo fmt --check,clippy --all-targets --all-features -D warnings, 86 tests: unit (auth chain parsing, exchange reply/cache, device-flow stubs, catalog mapping, admission, wire assembly incl. required headers) + golden wire schemas for all six functions (typed-schema rule enforced).auth_expired, admission + live-metadata reconcile incl. plan-disabled filtering, re-declare onrouter::ready. A suite-wide lock makes plaincargo testsafe.login::start→ device code → catalog fill → picker chat).Fixes MOT-4378
Summary by CodeRabbit