Add Copilot runtime connection support and update documentation - #283
Conversation
🤖 Multi-Agent PR Review #1PR: #283 — Add Copilot runtime connection support and update documentation 📋 SummaryThis PR adds a well-scoped, well-documented feature: connecting the Copilot provider to an already-running 🔍 Consensus FindingsAll four findings below were agreed by both reviewers.
💡 Suggested fix for R1-001try:
from copilot import CopilotClient
from copilot.session import PermissionHandler
COPILOT_SDK_AVAILABLE = True
except ImportError:
COPILOT_SDK_AVAILABLE = False
CopilotClient = None # type: ignore[misc, assignment]
PermissionHandler = None # type: ignore[misc, assignment]
+# RuntimeConnection was added to the SDK after CopilotClient; import it
+# separately so an older-but-present SDK still enables the default provider.
+try:
+ from copilot.client import RuntimeConnection
+except ImportError:
+ RuntimeConnection = None # type: ignore[misc, assignment]💡 Suggested fix for R1-002+ if self.runtime_url is not None:
+ if self.runtime_url == "":
+ raise ValueError(
+ "'runtime_url' is empty; remove the key or supply a value "
+ "(typo / unset env interpolation?)"
+ )💡 Suggested fix for R2-001 url, token = connection
+ if self._provider_settings is not None and self._provider_settings.has_custom_routing():
+ raise ProviderError(
+ "COPILOT_PROVIDER_RUNTIME_URL cannot be combined with custom endpoint "
+ "routing in runtime.provider; connecting to an existing runtime and "
+ "custom endpoint routing are mutually exclusive (the runtime is the endpoint).",
+ suggestion=(
+ "Unset COPILOT_PROVIDER_RUNTIME_URL or remove base_url/api_key/"
+ "bearer_token/type/wire_api/headers/azure from runtime.provider."
+ ),
+ is_retryable=False,
+ )💡 Suggested fix for R2-002 if token is None:
token = os.environ.get("COPILOT_PROVIDER_RUNTIME_TOKEN")
+ if url is not None:
+ url = url.strip()
+ if not url:
+ raise ProviderError(
+ "'runtime_url' is empty; remove it or supply a value.",
+ suggestion=(
+ "Set runtime.provider.runtime_url or COPILOT_PROVIDER_RUNTIME_URL "
+ "to a valid port, host:port, or full URL."
+ ),
+ is_retryable=False,
+ )
+ if token is not None:
+ token = token.strip() or None
+ if token is not None and url is None:
+ raise ProviderError(
+ "'runtime_token' requires 'runtime_url' to also be set",
+ suggestion=(
+ "Set COPILOT_PROVIDER_RUNTIME_URL alongside "
+ "COPILOT_PROVIDER_RUNTIME_TOKEN, or remove the token."
+ ),
+ is_retryable=False,
+ )
+
if url is None:
return None
return (url, token)🔎 Unique / Disputed FindingsNone. All four findings reached mutual consensus in round 1, with no open disagreements. 📊 Model Comparison
📐 Impact DiagramThe findings all center on the runtime-connection resolution decision (YAML → env fallback → spawn vs. connect, plus mutual-exclusion). This flow clarifies where each fix applies: flowchart TD
A[Provider start] --> B{runtime_url resolved?<br/>YAML then env}
B -->|No| C[Spawn nested copilot runtime]
B -->|Empty / typo| D[R1-002 & R2-002:<br/>reject, don't silently spawn]
B -->|Yes| E{Custom routing also active?}
E -->|Yes via env| F[R2-001:<br/>raise ProviderError<br/>mutually exclusive]
E -->|No| G[Connect to existing<br/>copilot --server runtime]
🏁 VerdictBoth reviewers agree: this is a clean, well-documented, low-risk feature that is production-safe once the robustness gaps are closed. There are no critical findings and no disputes — the four consensus warnings all cluster around SDK-import isolation and making the environment-variable activation path enforce the same validation and mutual-exclusion rules the YAML path already guarantees. None is a blocker, and each has a concrete suggested fix. Merge readiness: |
Shazwazza
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent PR Review — Inline Findings
Posted by the pr-reviewer agent. See the summary comment for the full report.
…001, R2-002) - R1-001: import RuntimeConnection in its own try/except so an older-but-present SDK still enables the default Copilot provider - R1-002: reject empty runtime_url at schema validation - R2-001: raise ProviderError (not warn) when an external runtime is combined with custom endpoint routing - R2-002: normalize/validate env-resolved runtime_url/runtime_token (empty URL raises, token-only raises, empty token -> None) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 Multi-Agent PR Review #2 — Update since Review #1PR: #283 — Add Copilot runtime connection support and update documentation 📋 SummarySince Review #1, the author resolved all four prior warnings — the 🆕 New Findings
💡 Suggested fix for the
|
| Previous Severity | File | Finding | Status |
|---|---|---|---|
| 🟡 | src/conductor/providers/copilot.py:82 |
RuntimeConnection imported inside the shared SDK-availability try, so an older-but-present SDK silently disabled the entire Copilot provider |
Resolved |
| 🟡 | src/conductor/config/schema.py:1881-1904 |
Empty-string runtime_url not rejected at schema validation (fell back to env and silently spawned a nested runtime) |
Resolved |
| 🟡 | src/conductor/providers/copilot.py:2082 |
COPILOT_PROVIDER_RUNTIME_URL + custom endpoint routing only warned instead of enforcing mutual exclusion |
Resolved |
| 🟡 | src/conductor/providers/copilot.py:493-497 |
Env-resolved runtime values not validated/stripped like YAML (empty URL, token-without-URL) | Resolved |
📣 Author Feedback Considered
Findings from Review #1 where the author or a maintainer responded (all four prior threads were marked resolved):
| Finding | File:Line | Prior Status | Human Reply (excerpt) | This Review's Action |
|---|---|---|---|---|
| R1-001 | src/conductor/providers/copilot.py |
resolved | — (thread marked resolved, no comment) | Dropped — import isolation landed; no matching finding at this line in Review #2 |
| R1-002 | src/conductor/config/schema.py:1891 |
resolved | — (thread marked resolved, no comment) | Dropped — empty-string runtime_url rejection landed |
| R2-001 | src/conductor/providers/copilot.py:2117 |
resolved | — (thread marked resolved, no comment) | Dropped — env-path mutual-exclusion enforcement landed |
| R2-002 | src/conductor/providers/copilot.py:500 |
resolved | — (thread marked resolved, no comment) | Dropped — env-resolved value validation/strip landed |
🔄 Changed Findings
No findings changed. The two current findings are new follow-ons rather than re-graded versions of prior findings (different file locations and categories).
📊 Delta Summary
- Unchanged findings: 0
- New findings: 2
- Resolved findings: 4
- Changed findings: 0
- Dropped via author feedback: 4
- Downgraded via author feedback: 0
- Re-raised with acknowledgement: 0
🏁 Verdict
Merge readiness: RuntimeConnection is None guard in _build_client() (R1-001/R2-003) that breaks the documented error contract on older SDKs, and the whitespace-only runtime_url / runtime_token bypass (R2-004). Both are trivially fixable and have concrete suggested fixes.
Review confidence: 78/100 🟡 Moderate · reviewer agreement & finding strength — not a PR-quality score
…e secrets (R2-003, R2-004) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eda8ff9d-e004-446f-9900-31b2add1be5b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9bdd92b-5644-413d-a2c2-c1e5090b81f0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9bdd92b-5644-413d-a2c2-c1e5090b81f0
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
=======================================
Coverage ? 89.72%
=======================================
Files ? 72
Lines ? 13055
Branches ? 0
=======================================
Hits ? 11713
Misses ? 1342
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This pull request adds support for connecting the Copilot provider to an already-running Copilot runtime, rather than always spawning a nested process. This enables external orchestrators to share a single authenticated Copilot session across all agents and models, with configuration available via YAML or environment variables. The schema enforces mutual exclusivity between this connection method and custom endpoint routing, and provides clear error handling and documentation.
Copilot existing runtime connection support:
ProviderSettingsschema (src/conductor/config/schema.py): Addedruntime_urlandruntime_tokenfields to allow connecting to an already-running Copilot runtime instead of spawning a new one. These fields can be set via YAML or the environment (COPILOT_PROVIDER_RUNTIME_URL,COPILOT_PROVIDER_RUNTIME_TOKEN). [1] [2]src/conductor/config/schema.py): Enforces thatruntime_urlis mutually exclusive with custom endpoint routing fields, and thatruntime_tokenrequiresruntime_url. Empty tokens are rejected. [1] [2]has_external_runtime()method toProviderSettingsto distinguish this mode from custom routing.Documentation and examples:
docs/configuration.md,AGENTS.md): Added detailed documentation and step-by-step instructions for connecting to an existing Copilot runtime, including configuration via YAML or environment variables, rules, and usage notes. [1] [2] [3]examples/copilot-existing-runtime.yaml): Added a new example YAML workflow demonstrating how to connect to an already-running Copilot runtime.CHANGELOG.md): Added entry describing the new feature and its configuration.