feat(onboard): add GitHub Copilot provider (Refs #5799) - #6671
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGitHub Copilot is added as a first-class inference provider with aliases, credential staging, onboarding menu support, managed inference routing, model metadata, tests, installer help, and documentation. ChangesGitHub Copilot provider
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant buildInferenceProviderMenu
participant stageRemoteProviderKeyBridge
participant stageCopilotCredentialEnv
User->>buildInferenceProviderMenu: select copilot
buildInferenceProviderMenu->>stageRemoteProviderKeyBridge: pass Copilot credential environment
stageRemoteProviderKeyBridge->>stageCopilotCredentialEnv: stage GH_TOKEN or GITHUB_TOKEN
stageCopilotCredentialEnv-->>stageRemoteProviderKeyBridge: set COPILOT_GITHUB_TOKEN
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
131f743 to
dbf2650
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/providers.test.ts (1)
191-204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
GITHUB_TOKENfallback andGH_TOKEN-precedence case.Only the
GH_TOKEN-present and explicit-token cases are tested.COPILOT_CREDENTIAL_SOURCE_ENVSorder (GH_TOKENbeforeGITHUB_TOKEN) implies a precedence contract that isn't exercised — e.g. staging when onlyGITHUB_TOKENis set, and staging when bothGH_TOKENandGITHUB_TOKENare set (to proveGH_TOKENwins).✅ Suggested additional cases
withProviderEnv({ COPILOT_GITHUB_TOKEN: "explicit-token", GH_TOKEN: "ignored-token" }, () => { expect(stageCopilotCredentialEnv()).toBe(false); expect(process.env.COPILOT_GITHUB_TOKEN).toBe("explicit-token"); }); + + withProviderEnv({ GITHUB_TOKEN: "fallback-token" }, () => { + expect(stageCopilotCredentialEnv()).toBe(true); + expect(process.env.COPILOT_GITHUB_TOKEN).toBe("fallback-token"); + }); + + withProviderEnv({ GH_TOKEN: "gh-token", GITHUB_TOKEN: "github-token" }, () => { + expect(stageCopilotCredentialEnv()).toBe(true); + expect(process.env.COPILOT_GITHUB_TOKEN).toBe("gh-token"); + }); });🤖 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 `@src/lib/onboard/providers.test.ts` around lines 191 - 204, Add test coverage in the “stages GitHub token aliases into the Copilot credential env” test for staging from only GITHUB_TOKEN and for both GH_TOKEN and GITHUB_TOKEN being set, asserting staging succeeds and GH_TOKEN is selected according to COPILOT_CREDENTIAL_SOURCE_ENVS order.
🤖 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.
Nitpick comments:
In `@src/lib/onboard/providers.test.ts`:
- Around line 191-204: Add test coverage in the “stages GitHub token aliases
into the Copilot credential env” test for staging from only GITHUB_TOKEN and for
both GH_TOKEN and GITHUB_TOKEN being set, asserting staging succeeds and
GH_TOKEN is selected according to COPILOT_CREDENTIAL_SOURCE_ENVS order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bc29014-64b9-4495-b170-3b21f14e0d23
📒 Files selected for processing (12)
docs/inference/choose-inference-provider.mdxdocs/reference/commands.mdxsrc/lib/inference/config.test.tssrc/lib/inference/config.tssrc/lib/inference/model-prompts.tssrc/lib/onboard.tssrc/lib/onboard/provider-menu.test.tssrc/lib/onboard/provider-menu.tssrc/lib/onboard/providers.test.tssrc/lib/onboard/providers.tstest/onboard-resume-provider-recovery.test.tstest/onboard-selection.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/onboard-provider-key-bridge.test.ts (1)
202-217: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression for legacy-key precedence.
This case only supplies
GH_TOKEN, so it cannot detect whether the legacy provider-key path still runs after Copilot staging. Add a public-boundary scenario with bothGH_TOKENandNEMOCLAW_PROVIDER_KEY, then assert the documented credential precedence.As per path instructions, test behavioral outcomes rather than only the happy-path implementation.
🤖 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 `@test/onboard-provider-key-bridge.test.ts` around lines 202 - 217, The existing Copilot staging test does not cover precedence when both credential sources are present. Update the scenario in runSetupNimBridgeScenario to provide both GH_TOKEN and NEMOCLAW_PROVIDER_KEY, then assert the documented outcome and credentialEnv/value showing which credential wins; keep assertions focused on the public behavioral result rather than internal staging.Source: Path instructions
🤖 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 `@test/onboard-provider-key-bridge.test.ts`:
- Around line 18-21: Update the spawned-scenario environment setup and its
tracked key list to include NEMOCLAW_PROVIDER_KEY, ensuring it is removed by
default from inherited environments and passed explicitly only by scenarios that
require it; verify the credential assertions remain deterministic through the
public bridge boundary.
---
Nitpick comments:
In `@test/onboard-provider-key-bridge.test.ts`:
- Around line 202-217: The existing Copilot staging test does not cover
precedence when both credential sources are present. Update the scenario in
runSetupNimBridgeScenario to provide both GH_TOKEN and NEMOCLAW_PROVIDER_KEY,
then assert the documented outcome and credentialEnv/value showing which
credential wins; keep assertions focused on the public behavioral result rather
than internal staging.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4027232-c659-4748-ab9d-7489f9ae412b
📒 Files selected for processing (12)
docs/inference/choose-inference-provider.mdxdocs/reference/commands.mdxsrc/lib/inference/config.test.tssrc/lib/inference/config.tssrc/lib/inference/model-prompts.tssrc/lib/onboard/provider-key-bridge.tssrc/lib/onboard/provider-menu.test.tssrc/lib/onboard/provider-menu.tssrc/lib/onboard/providers.test.tssrc/lib/onboard/providers.tstest/onboard-provider-key-bridge.test.tstest/onboard-resume-provider-recovery.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/inference/choose-inference-provider.mdx
- test/onboard-resume-provider-recovery.test.ts
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/inference/model-prompts.ts
- src/lib/onboard/provider-menu.ts
- src/lib/inference/config.test.ts
- src/lib/inference/config.ts
- src/lib/onboard/providers.ts
- src/lib/onboard/provider-menu.test.ts
- src/lib/onboard/providers.test.ts
Fixes NVIDIA#5799 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
861b9c8 to
6ba6807
Compare
|
Rebased on current main and kept the Copilot provider changes intact. Focused onboard/inference tests, source-shape, and lint/checks pass; refreshed PR checks are green. |
|
Blocking runtime issue before merge: NemoClaw always reaches Primary evidence: OpenShell v0.0.72 inference type normalization and the current server rejection path. This needs either upstream OpenShell inference support for Copilot, or a separately reviewed direct OpenClaw |
wscurran
added
area: docs
|
Verified the OpenShell contract and this should stay blocked for now. The installer failure is only the provider-list assertion seeing copilot, but fixing that would make CI greener while real onboarding still hits an unsupported inference-provider path. I updated the PR body/title to use Refs instead of Fixes and removed the stale generated summary so the status is clearer. |
deepujain
changed the title
|
Closing this PR because the current implementation depends on a capability OpenShell does not provide. OpenShell's This is not a rejection of #5799; that feature request should remain open. Copilot onboarding can return once there is an implemented architecture: either OpenShell supports Copilot cluster inference with the required authentication and routing contract, or NemoClaw adopts a reviewed direct OpenClaw Copilot route with explicit credential and network boundaries plus provider-specific live E2E coverage. A new PR, or this PR reopened if still applicable, can then target the actual supported contract. The closure is based on the runtime architecture blocker, not CI status. |
4 participants
Summary
Refs #5799.
Current status: blocked on the OpenShell inference-provider contract. OpenShell v0.0.72 and current main do not accept Copilot as a cluster inference provider yet, so this branch should not merge as-is.
Adds GitHub Copilot provider wiring in onboarding, including aliases, model choices, docs, and token alias staging into
COPILOT_GITHUB_TOKEN. The managed inference route is not merge-ready until OpenShell supports Copilot as an inference provider or NemoClaw uses a reviewed direct route.Changes
copilotprovider key,github/github-copilotaliases, curated Copilot model choices, and provider-selection config.Testing
npm run build:clipassednpm run typecheck:clipassednpm run source-shape:checkpassednpm run lintpassed, with the existing unrelatedsrc/lib/onboard/child-exit-tracker.test.tsunused suppression warningnpm run --silent test -- test/onboard-provider-key-bridge.test.ts src/lib/onboard/provider-menu.test.ts src/lib/onboard/providers.test.ts src/lib/inference/config.test.ts test/onboard-resume-provider-recovery.test.tspassednpm run --silent test -- src/lib/actions/inference-set-provider-alias.test.ts src/lib/actions/inference-set-compatible-provider.test.ts src/lib/actions/inference-set-local-provider.test.tspassednpm run --silent test -- src/lib/registry-recovery-action.test.ts src/lib/actions/sandbox/policy-channel-conflict.test.ts src/lib/shields/index.test.ts src/lib/shields/timer.test.tspassednpm testwas attempted, but this local machine still fails unrelated broad-suite tests around missing Pythonyamlplus several long-running sandbox/base-image fixtures timing out. The Copilot/provider and affected onboard paths above are green.Evidence it works
NEMOCLAW_PROVIDER=copilotis accepted and resolves to thegithub-copilotOpenShell provider name.GH_TOKENandGITHUB_TOKENare staged intoCOPILOT_GITHUB_TOKENwhen the canonical env is not already set.Signed-off-by: Deepak Jain deepujain@gmail.com