Expose enableOnDemandInstructionDiscovery across all SDK SessionConfig types#1323
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new session configuration flag (enableOnDemandInstructionDiscovery) across the in-repo SDKs so consumers can opt into (or explicitly disable) runtime on-demand custom-instruction discovery via the JSON-RPC wire key enableOnDemandInstructionDiscovery.
Changes:
- Exposes the new flag on SessionConfig + ResumeSessionConfig surfaces (and forwards it on
session.create/session.resume) for Node, Python, Go, .NET, and Rust. - Adds/extends unit + E2E coverage to verify
true/falseserialization and key omission when unset. - Adds inline API documentation for the new option in each language’s config surface.
Show a summary per file
| File | Description |
|---|---|
| rust/tests/session_test.rs | Adds serde wire-format tests for enableOnDemandInstructionDiscovery on SessionConfig/ResumeSessionConfig. |
| rust/src/types.rs | Adds enable_on_demand_instruction_discovery: Option<bool> + builder methods + debug/default updates and unit-test coverage. |
| python/test_client.py | Adds forwarding tests ensuring explicit False is sent on create/resume. |
| python/e2e/test_client_options_e2e.py | Extends E2E options propagation test to include the new flag. |
| python/copilot/client.py | Adds new kwargs to create/resume APIs, docs, and wire payload serialization. |
| nodejs/test/e2e/client_options.e2e.test.ts | Extends E2E options propagation test to assert the new flag is present. |
| nodejs/test/client.test.ts | Adds unit tests verifying forwarding on session.create and session.resume. |
| nodejs/src/types.ts | Adds enableOnDemandInstructionDiscovery?: boolean to SessionConfig and includes it in ResumeSessionConfig pick. |
| nodejs/src/client.ts | Forwards the new flag in create/resume request payloads. |
| go/types.go | Adds *bool config fields and wires them into create/resume request structs (plus gofmt alignment). |
| go/internal/e2e/client_options_e2e_test.go | Extends Go E2E options propagation assertions to include the new flag. |
| go/client.go | Forwards EnableOnDemandInstructionDiscovery into create/resume wire requests. |
| go/client_test.go | Adds wire-format unit tests for explicit true/false and omission behavior. |
| dotnet/test/Unit/SerializationTests.cs | Adds serialization tests validating true/false/omitted behavior for create/resume requests. |
| dotnet/test/Unit/CloneTests.cs | Ensures SessionConfig/ResumeSessionConfig clone operations copy/preserve the new nullable property. |
| dotnet/src/Types.cs | Adds bool? EnableOnDemandInstructionDiscovery to SessionConfig/ResumeSessionConfig and copies it in clone constructors. |
| dotnet/src/Client.cs | Threads the new option through Create/Resume request construction and record types. |
Copilot's findings
- Files reviewed: 17/17 changed files
- Comments generated: 2
This comment has been minimized.
This comment has been minimized.
Mirrors the existing enableConfigDiscovery and remoteSession precedents (PRs #1044 and #1295). Exposes the SDK option that lets the runtime discover custom instruction files on demand after the agent reads or views files, complementing the existing up-front load of `.github/copilot-instructions.md`, `AGENTS.md`, etc. Wire key (camelCase, identical across all 5 SDKs): enableOnDemandInstructionDiscovery Type shapes: Node enableOnDemandInstructionDiscovery?: boolean Python enable_on_demand_instruction_discovery: bool | None = None Go EnableOnDemandInstructionDiscovery *bool .NET bool? EnableOnDemandInstructionDiscovery Rust Option<bool> with #[serde(skip_serializing_if = "Option::is_none")] Wire semantics: when set, the wire payload carries the literal value (including explicit `false`); when omitted, the key is dropped. Applies to both session.create and session.resume so callers can toggle the setting on a resumed session. Runtime-gated. The runtime honors the option only when custom instructions are enabled and the connected runtime supports on-demand custom instruction discovery; otherwise the option is accepted but no-ops. The SDK does not attempt to detect the runtime gate. Requires @github/copilot ^1.0.49-1 (the runtime change shipped in github/copilot#7759). Security: discovered instruction files are treated as model instructions and may be stored or replayed with session history. Docstrings caution against enabling for untrusted content, CI jobs processing untrusted forks, or directories writable by untrusted users or processes. Go shape note: uses *bool (not bool) so consumers can disable a previously-enabled session on resume. Reuses the precedent already set by EnableSessionTelemetry *bool and IncludeSubAgentStreamingEvents *bool. Does not retrofit the existing EnableConfigDiscovery bool field (that would be a separate breaking source change). Tests: each SDK adds tests for the new field on both create and resume, asserting that explicit `false` is serialized as `false` and that omission drops the key from the payload. Mirrors the test patterns already in place for enable_session_telemetry, include_sub_agent_streaming_events, and enable_config_discovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on PR #1323: the create-session API docs for enable_on_demand_instruction_discovery / EnableOnDemandInstructionDiscovery should not include resume-specific behavior. That note already lives on the resume-side configs (Python resume_session and Go ResumeSessionConfig).
07bfd0a to
e9e961a
Compare
This comment has been minimized.
This comment has been minimized.
| public bool? EnableConfigDiscovery { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// When <see langword="true"/>, requests on-demand discovery of custom instruction |
There was a problem hiding this comment.
What does "on-demand discovery" actually mean? Demanded by what / when, discovered where, etc.
There was a problem hiding this comment.
@stephentoub thanks for feedback. I improved the docs so they explain how the on-demand discovery works and how the customer instructions are injected into the context 8586e6a
Explain the concrete discovery mechanic (directory walk from the accessed file up to the repo root, applyTo glob filtering, once-per-session delivery) and align the runtime-gating and security wording across all five SDKs. Addresses review feedback on PR #1323.
…nd-instruction-discovery # Conflicts: # dotnet/src/Client.cs # dotnet/src/Types.cs # dotnet/test/Unit/CloneTests.cs # dotnet/test/Unit/SerializationTests.cs # go/client.go # go/types.go # nodejs/src/client.ts # nodejs/src/types.ts # python/copilot/client.py # rust/src/types.rs
|
Resolved the merge conflicts by merging latest Heads up for reviewers: the As a result, the remaining diff of this branch against
Verified locally: Rust (lib + session integration), Node (field tests + full typecheck), Python (field tests), and .NET (Clone/Serialization suites) all pass; Since the implementation is now superseded by #1474, this PR is effectively test-only. Maintainers may want to either merge it for the added regression coverage or close it as superseded. |
SDK Consistency Review ✅This PR adds Java SDK: Not included in this PR, but the Java SDK ( No cross-SDK consistency issues found. 🎉
|
Exposes the new
enableOnDemandInstructionDiscoveryconfiguration flag across all five in-repo SDKs (Node, Python, Go, .NET, Rust). Mirrors the precedent set by #1044 (enableConfigDiscovery) and #1295 (remoteSession).The runtime change shipped in github/copilot#7759 and is available in
@github/copilot@^1.0.49-1.Cross-language consistency matrix
falsebehaviorenableOnDemandInstructionDiscovery?: booleanenableOnDemandInstructionDiscoveryfalseenable_on_demand_instruction_discovery: bool | None = NoneenableOnDemandInstructionDiscoveryfalseEnableOnDemandInstructionDiscovery *boolenableOnDemandInstructionDiscoverynil)falsebool? EnableOnDemandInstructionDiscoveryenableOnDemandInstructionDiscoveryfalseenable_on_demand_instruction_discovery: Option<bool>enableOnDemandInstructionDiscoveryfalseRuntime-gated availability
When set to
true, the SDK asks the runtime to discover custom instruction files on demand after the agent successfully reads or views files. The option is runtime-gated: it takes effect only when custom instructions are enabled and the connected runtime supports and enables on-demand custom instruction discovery. Otherwise the runtime accepts the option but performs no on-demand discovery (silent no-op, no error or warning).The SDK does not attempt to detect runtime gating; it forwards the option unconditionally. On
session.resume, omitting the option leaves the existing session setting unchanged, and passing an explicitfalsedisables future on-demand discovery for that resumed session.Trust and security
Enable only for trusted repositories or workspaces. Discovered instruction files are treated as model instructions, may affect agent behavior, and may be stored or replayed with session history. Do not enable for untrusted content, CI jobs processing untrusted forks, or directories writable by untrusted users or processes.
Go shape
Uses
*bool(notbool) on bothSessionConfigandResumeSessionConfigso callers can disable a previously-enabled session on resume. Reuses the precedent already set byEnableSessionTelemetry *boolandIncludeSubAgentStreamingEvents *bool. This PR does not retrofit the existingEnableConfigDiscovery boolfield — that would be a separate breaking source change with broader blast radius.Tests
Each SDK adds tests for the new field on both
session.createandsession.resume, asserting that explicitfalseis serialized on the wire and that omission drops the key entirely. Mirrors the test patterns already in place forenable_session_telemetry,include_sub_agent_streaming_events, andenable_config_discovery.nodejs/test/e2e/client_options.e2e.test.tshappy-path; adds 2 unit tests innodejs/test/client.test.ts(create + resume, explicitfalse).python/e2e/test_client_options_e2e.pyhappy-path; adds 2 unit tests inpython/test_client.py(create + resume, explicitFalse).go/internal/e2e/client_options_e2e_test.gohappy-path; adds 6 wire-format unit tests ingo/client_test.go(create + resume;Bool(true),Bool(false), and omitted).dotnet/test/Unit/SerializationTests.cs(create + resume; true, false, omitted) and 4 clone tests indotnet/test/Unit/CloneTests.cscovering the copy-ctor.rust/tests/session_test.rs(create + resume;Some(true),Some(false), omitted) and extends the existing builder unit-tests inrust/src/types.rsto call the new.with_enable_on_demand_instruction_discovery(...)method on bothSessionConfigandResumeSessionConfig.Documentation
The new field is documented inline in each SDK's typed config surface using the language-appropriate doc-comment style (JSDoc, Google-style, godoc, XML, Rustdoc). README and CHANGELOG are intentionally not updated, matching the precedent of #1044 and #1295.
Notes for reviewers
go/types.goshows ~170 line changes; the bulk is gofmt-driven struct-tag column re-alignment because the new field name is longer than any existing field. The only semantic changes are the additions ofEnableOnDemandInstructionDiscoverytoSessionConfig,ResumeSessionConfig, and the two wire-request structs.enableConfigDiscoverydocstring statement that custom instruction files "are always loaded from the working directory regardless of this setting" remains accurate — the new option adds on-demand discovery on top of the up-front load; it does not replace it.python/copilot/session.pyTypedDicts do not include the new field. They are also missing existing fields (enable_config_discovery,remote_session); closing those gaps is best handled in a follow-up so this PR stays narrow.