Skip to content

Add Rust model allowlist support on pinned SDK base - #2513

Draft
jpbufe3 wants to merge 5 commits into
github:mainfrom
jpbufe3:jobufe-microsoft-pinned-sdk-model-allowlist
Draft

Add Rust model allowlist support on pinned SDK base#2513
jpbufe3 wants to merge 5 commits into
github:mainfrom
jpbufe3:jobufe-microsoft-pinned-sdk-model-allowlist

Conversation

@jpbufe3

@jpbufe3 jpbufe3 commented Sep 4, 2026

Copy link
Copy Markdown

Integrations pinned to SDK commit c672e716473da0be4da7ad807df8d21259623640 need the Rust model allowlist contract without pulling in later unrelated SDK changes. This compatibility PR ports only that surface onto the exact pinned base.

Summary

  • Add optional allowed_models fields and fluent builders to SessionConfig and ResumeSessionConfig, serialized as allowedModels and omitted when unrestricted.
  • Add the generated typed session.model.setAllowedModels RPC request/result surface for replacing or clearing a running session's restriction.
  • Cover defaults, Debug output, create/resume wire conversion, request serialization, method routing, session ID injection, and typed result deserialization.

The SDK preserves exact caller-provided IDs; the runtime remains responsible for validation, policy intersection, fallback selection, and enforcement across child, sub-agent, Auto, and remote execution paths.

Testing

  • COPILOT_SKIP_CLI_DOWNLOAD=1 cargo test --all-features allowed_models
  • cargo +nightly-2026-04-14 fmt --check

Copilot AI balanced review requested due to automatic review settings September 4, 2026 01:56
@jpbufe3
jpbufe3 requested a review from a team as a code owner September 4, 2026 01:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review tier: Lite
Findings: 1 Low severity

New issues introduced by this change (1)
Severity Finding
Low severity rust/​src/​wire.rs — The PR contract depends on this field serializing as allowedModels. This currently relies on any…
What changed in this PR

Ports the Rust “model allowlist” contract onto an SDK base pinned to a specific commit, adding config fields and an RPC method to set/clear allowed model IDs without pulling additional SDK changes.

Changes:

  • Add allowed_models to SessionConfig and ResumeSessionConfig, including builders and wire conversion.
  • Add typed session.model.setAllowedModels RPC method plus request/result types and method constant.
  • Add unit/integration tests covering serialization, wire conversion, routing, and result deserialization.
File Description
rust/​src/​types.rs Adds allowed_models fields/builders and wires them into create/resume payloads; adds unit tests.
rust/​src/​wire.rs Adds allowed_models to create/resume wire structs for JSON serialization.
rust/​src/​generated/​api_types.rs Introduces RPC method constant and request/result types for setAllowedModels.
rust/​src/​generated/​rpc.rs Adds the session.model.setAllowedModels RPC call surface with sessionId injection.
rust/​tests/​session_test.rs Adds integration tests for request/response serialization and RPC dispatch.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rust/src/wire.rs Outdated
@@ -53,6 +53,8 @@ pub(crate) struct SessionCreateWire {
#[serde(skip_serializing_if = "Option::is_none")]
pub model: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Thanks for the contribution! I looked into this and found session.model.setAllowedModels isn't implemented by the shipped Copilot CLI runtime (v1.0.81-11) — calling it against a real session returns -32601 Unhandled method, unlike other session methods (e.g. session.abort) which work fine. The codegen change also hand-injects this method into the schema rather than reading it from the real runtime contract, and it's Rust-only even though model-allowlisting would need to be consistent across all SDK languages.

Could you clarify the plan for landing this in the runtime first? Once the runtime actually implements session.model.setAllowedModels and it shows up in the real api.schema.json, we can regenerate for all six SDKs together (with a real end-to-end test against the runtime, not just mocked dispatch tests) rather than hand-injecting a Rust-only stub here.

I'm moving this to draft for tracking — please mark it ready for review once the runtime side has landed.

@SteveSandersonMS
SteveSandersonMS marked this pull request as draft September 4, 2026 08:06
@jpbufe3

jpbufe3 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Could you clarify the plan for landing this in the runtime first?

Agreed on the sequencing. The runtime implementation is github/copilot-agent-runtime#18943 and is still open; this SDK compatibility branch supports the dependent github/github-app#14584 work and should remain draft until the runtime PR lands.

The shipped runtime returning -32601 is expected until that implementation is merged and released. Once the runtime publishes the canonical api.schema.json, the live RPC should be regenerated from that schema, the temporary Rust-only overlay removed, and broader cross-SDK generation/E2E coverage handled from the real runtime contract before this is marked ready.

dmytrostruk and others added 4 commits September 4, 2026 09:06
Embed the full Copilot CLI separately from the managed runtime bundle so explicit callers receive the Node SEA while normal SDK resolution continues to use copilot-runtime.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jpbufe3
jpbufe3 force-pushed the jobufe-microsoft-pinned-sdk-model-allowlist branch from 1b83eac to 67519be Compare September 4, 2026 20:33
@jpbufe3

jpbufe3 commented Sep 8, 2026

Copy link
Copy Markdown
Author

https://github.com/github/copilot-agent-runtime/pull/18943has merged and is being staged for prerelease! Reopening for review cc @SteveSandersonMS

@jpbufe3
jpbufe3 marked this pull request as ready for review September 8, 2026 12:47
@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Thanks for the update, and glad to see github/copilot-agent-runtime#18943 landed! I re-validated against the newest available runtime build (@github/copilot@1.0.84-1, published 2026-09-04) since that's the most recent CLI/runtime the SDK could actually pull:

  • schemas/api.schema.json in that package still has no session.model.setAllowedModels entry and no allowedModels field anywhere.
  • I spun up the real copilot --server --stdio binary from that package, created a live session, and called the method directly — same result as before:
    → session.model.setAllowedModels {"sessionId": "<real-id>", "allowedModels": ["gpt-5"]}
    ← {"error": {"code": -32601, "message": "Unhandled method session.model.setAllowedModels"}}
    

That's expected, since the runtime PR merged 2026-09-08, four days after that CLI build was cut — the fix hasn't shipped in any published runtime/CLI package yet ("staged for prerelease" isn't the same as available). The SDK's own codegen still fabricates the schema entry itself (addModelSetAllowedModelsToApiSchema) rather than reading it from a real, shipped api.schema.json, so nothing here would catch drift if the eventual real contract differs from what's hand-coded (field names, error semantics, etc.).

I'd like to hold this in draft until a published @github/copilot release actually contains the runtime change, at which point we should regenerate the Rust types directly from the real schema (dropping addModelSetAllowedModelsToApiSchema) and add a real end-to-end test that exercises session.model.setAllowedModels against that shipped runtime rather than only the mocked mock-server tests. Please re-open once you can point at a published CLI/runtime version where the real schema includes this method — happy to take another look right away at that point.

@SteveSandersonMS
SteveSandersonMS marked this pull request as draft September 8, 2026 13:24
Integrate d5c9d06 while preserving the existing canonical Rust model allowlist patch and tests. Keep the incoming CLI 1.0.83 pin and published compatibility history.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to 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.

4 participants