feat(providers): add GitHub Copilot community provider #1505
Merged
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds a GitHub Copilot community provider and related artifacts: provider implementation, binary resolver, event bridge, config types, package exports, docs and env examples, shared structured-output and skills utilities, E2E workflows, and many tests. ChangesCopilot integration & shared tooling
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml:
- Around line 7-8: The GH_TOKEN/GITHUB_TOKEN note is misleading because the
Copilot provider (packages/providers/src/community/copilot/provider.ts) only
uses those tokens when useLoggedInUser is set to false; update the comment in
.archon/workflows/.../e2e-copilot-all-nodes-smoke.yaml to either (a) state that
generic GH tokens will only be used when the provider is configured with
useLoggedInUser: false, or (b) instruct users to set COPILOT_GITHUB_TOKEN (or
explicitly set useLoggedInUser=false in the test config) so the workflow will
use the token instead of the logged-in user. Mention the provider symbol
useLoggedInUser and the COPILOT_GITHUB_TOKEN/GH_TOKEN/GITHUB_TOKEN env names to
make the intended configuration clear.
In `@packages/providers/src/community/copilot/binary-resolver.test.ts`:
- Around line 146-181: The failing tests leak the resolveFromPath spy when an
assertion throws; for each test that creates a resolveFromPathSpy (the tests
calling resolver.resolveCopilotBinaryPath()), wrap the assertion and the
subsequent mockRestore() in a try/finally block so
resolveFromPathSpy.mockRestore() is always executed, e.g. create
resolveFromPathSpy = spyOn(resolver, 'resolveFromPath')..., then do try { await
expect(...).rejects.toThrow(...); } finally { resolveFromPathSpy.mockRestore();
}, applying the same pattern to both tests that currently call mockRestore()
only on the happy path to ensure the spy is always restored.
In `@packages/providers/src/community/copilot/binary-resolver.ts`:
- Around line 131-149: The vendor and autodetect resolution currently only use
fileExists() and can return non-executable paths; update the checks in the
vendor branch (getVendorBinaryName(), getArchonHome(), COPILOT_VENDOR_DIR,
vendorBinaryPath) and the autodetect loop (getAutodetectPaths(), probePath) to
call isExecutableFile(path) instead of or in addition to fileExists(), only
logging via getLog().info({ source: 'vendor' }, 'copilot.binary_resolved') or
getLog().info({ source: 'autodetect' }, 'copilot.binary_resolved') and returning
the path when isExecutableFile(...) is true; do not return paths that exist but
are not executable (optionally log a warning via getLog() if fileExists() &&
!isExecutableFile()).
- Around line 37-45: The PATH lookup in resolveFromPath uses 'copilot.exe' for
Windows which misses npm-installed shims; change the executable variable (used
by lookupCmd and _execFileSync) so on process.platform === 'win32' it is
'copilot' (not 'copilot.exe') so the where lookup can find the .cmd/.ps1 shim;
keep using lookupCmd and _execFileSync as-is and trim/split the output the same
way.
In `@packages/providers/src/community/copilot/event-bridge.ts`:
- Around line 372-390: The terminal result chunk is being marked as an error
even when the SDK auto-recovers and assistant content was delivered; change the
logic that sets result.isError and result.errors so they are only applied when
there is an actual failure that produced no assistant content (i.e., require
errorMessage && !sawAssistantContent). In the block that builds the MessageChunk
(result) using session.sessionId and capturedTokens, wrap the assignments
result.isError = true and result.errors = [errorMessage] with a conditional that
checks !sawAssistantContent so recovered turns with assistant content are not
flagged as failed.
In `@packages/providers/src/community/copilot/provider-hardening.test.ts`:
- Around line 281-292: The test currently calls collect(gen) twice and discards
the first result, which can lose the real error when firstNext resolves; modify
the try block around await firstNext so that you capture and inspect the result
of collect(gen) immediately (e.g., store the returned object from collect(gen)
and if it contains an error assign it to primaryError) rather than calling
collect(gen) again later—update references to primaryError, firstNext,
collect(gen), and gen accordingly so the first collect result's error is
preserved.
In `@packages/providers/src/shared/skills.ts`:
- Around line 62-70: The code currently joins unvalidated skill names (rawName
from skillNames) into filesystem paths (using join with roots and checking
SKILL.md), which allows path traversal like "../foo" or "nested/name"; update
the validation in the loop over skillNames (before using join/existsSync) to
reject any rawName containing path separators or parent-segment tokens (e.g.,
path.sep, '/' or '..'), empty strings, or leading slashes, or alternatively
normalize and ensure path.basename(rawName) === rawName; only proceed to create
candidate paths and call existsSync when rawName passes this validation to
prevent escaping the intended roots.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf356cbd-6001-4fd0-9bc8-d47c0057c320
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.archon/workflows/test-workflows/e2e-copilot-abort.yaml.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml.env.examplepackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/package.jsonpackages/providers/src/community/copilot/binary-resolver-dev.test.tspackages/providers/src/community/copilot/binary-resolver.test.tspackages/providers/src/community/copilot/binary-resolver.tspackages/providers/src/community/copilot/capabilities.tspackages/providers/src/community/copilot/config.test.tspackages/providers/src/community/copilot/config.tspackages/providers/src/community/copilot/event-bridge.test.tspackages/providers/src/community/copilot/event-bridge.tspackages/providers/src/community/copilot/index.tspackages/providers/src/community/copilot/provider-hardening.test.tspackages/providers/src/community/copilot/provider-lazy-load.test.tspackages/providers/src/community/copilot/provider.test.tspackages/providers/src/community/copilot/provider.tspackages/providers/src/community/copilot/registration.tspackages/providers/src/community/pi/event-bridge.test.tspackages/providers/src/community/pi/event-bridge.tspackages/providers/src/community/pi/options-translator.tspackages/providers/src/community/pi/provider.tspackages/providers/src/index.tspackages/providers/src/registry.test.tspackages/providers/src/registry.tspackages/providers/src/shared/skills.tspackages/providers/src/shared/structured-output.tspackages/providers/src/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/providers/src/community/copilot/binary-resolver.ts (1)
116-159: ⚡ Quick winUse guideline-compliant structured event names for resolver logs.
The resolver logs currently use
copilot.binary_resolved; this doesn’t follow the repository’s standard state suffix set. Rename to a standard_completedevent and emit a_failedevent before throwing to keep observability consistent.Suggested patch
- getLog().info({ source: 'env' }, 'copilot.binary_resolved'); + getLog().info({ source: 'env' }, 'copilot.binary_resolution_completed'); @@ - getLog().info({ source: 'config' }, 'copilot.binary_resolved'); + getLog().info({ source: 'config' }, 'copilot.binary_resolution_completed'); @@ - getLog().info({ source: 'vendor' }, 'copilot.binary_resolved'); + getLog().info({ source: 'vendor' }, 'copilot.binary_resolution_completed'); @@ - getLog().info({ source: 'autodetect' }, 'copilot.binary_resolved'); + getLog().info({ source: 'autodetect' }, 'copilot.binary_resolution_completed'); @@ - getLog().info({ source: 'path' }, 'copilot.binary_resolved'); + getLog().info({ source: 'path' }, 'copilot.binary_resolution_completed'); @@ - throw new Error( + getLog().error( + { source: 'resolver' }, + 'copilot.binary_resolution_failed' + ); + throw new Error(As per coding guidelines, structured logging must use
{domain}.{action}_{state}with standard states like_completedand_failed.Also applies to: 162-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/copilot/binary-resolver.ts` around lines 116 - 159, The resolver emits non-compliant event names like 'copilot.binary_resolved' in multiple spots; update all getLog().info calls in this module (e.g., the returns after envPath, configCliPath, vendorBinaryPath, autodetect probePath, and fromPath) to use a guideline-compliant name such as 'copilot.binary_resolve_completed', and for every place that throws an Error (for example the configCliPath non-executable check inside the function using isExecutableFile and any other pre-return error conditions) emit a failure event before throwing using 'copilot.binary_resolve_failed' with context, ensuring the same naming change is applied to the occurrences noted around lines 162-177 (e.g., where resolveFromPath is validated).packages/providers/src/community/copilot/event-bridge.ts (1)
391-399: ⚡ Quick winParse structured output from the final assistant payload when available.
This block always parses
assistantBuffer, even thoughsendAndWait()already returns the complete assistant message. If streaming delivers some deltas but misses the tail,structuredOutputis dropped even though the final payload is still parseable.Based on learnings: `packages/providers/src/**/*.ts` should handle session management and streaming (`for await (const event of events) { await platform.send(event) }`).♻️ Proposed fix
if (wantsStructured) { - const parsed = tryParseStructuredOutput(assistantBuffer); + const structuredOutputSource = sendResult?.data?.content ?? assistantBuffer; + const parsed = tryParseStructuredOutput(structuredOutputSource); if (parsed !== undefined) { result.structuredOutput = parsed; } else { log.warn( - { bufferLength: assistantBuffer.length, sessionId: session.sessionId }, + { + bufferLength: structuredOutputSource.length, + sessionId: session.sessionId, + }, 'copilot.structured_output_parse_failed' ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/copilot/event-bridge.ts` around lines 391 - 399, The current block only attempts to parse structured output from the incremental assistantBuffer (using wantsStructured, tryParseStructuredOutput, assistantBuffer) and can drop structuredOutput if streaming missed the tail; update the logic to prefer parsing the final assistant payload returned by sendAndWait (or the equivalent finalAssistantPayload/assistantMessage object returned by the send operation) and only fallback to parsing assistantBuffer if that final payload is unavailable, then assign result.structuredOutput accordingly; ensure session streaming loop handling (the for await (const event of events) { await platform.send(event) } pattern) remains intact so the final payload is always captured before parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/providers/src/community/copilot/binary-resolver.ts`:
- Around line 116-159: The resolver emits non-compliant event names like
'copilot.binary_resolved' in multiple spots; update all getLog().info calls in
this module (e.g., the returns after envPath, configCliPath, vendorBinaryPath,
autodetect probePath, and fromPath) to use a guideline-compliant name such as
'copilot.binary_resolve_completed', and for every place that throws an Error
(for example the configCliPath non-executable check inside the function using
isExecutableFile and any other pre-return error conditions) emit a failure event
before throwing using 'copilot.binary_resolve_failed' with context, ensuring the
same naming change is applied to the occurrences noted around lines 162-177
(e.g., where resolveFromPath is validated).
In `@packages/providers/src/community/copilot/event-bridge.ts`:
- Around line 391-399: The current block only attempts to parse structured
output from the incremental assistantBuffer (using wantsStructured,
tryParseStructuredOutput, assistantBuffer) and can drop structuredOutput if
streaming missed the tail; update the logic to prefer parsing the final
assistant payload returned by sendAndWait (or the equivalent
finalAssistantPayload/assistantMessage object returned by the send operation)
and only fallback to parsing assistantBuffer if that final payload is
unavailable, then assign result.structuredOutput accordingly; ensure session
streaming loop handling (the for await (const event of events) { await
platform.send(event) } pattern) remains intact so the final payload is always
captured before parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a293a45-36b2-49d8-b0c7-c4e3621d586c
📒 Files selected for processing (6)
.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yamlpackages/providers/src/community/copilot/binary-resolver.test.tspackages/providers/src/community/copilot/binary-resolver.tspackages/providers/src/community/copilot/event-bridge.tspackages/providers/src/community/copilot/provider-hardening.test.tspackages/providers/src/shared/skills.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/providers/src/community/copilot/provider-hardening.test.ts
- .archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml
- packages/providers/src/shared/skills.ts
- packages/providers/src/community/copilot/binary-resolver.test.ts
danielscholl
changed the title
Collaborator
|
@danielscholl related to #1115 — GitHub Copilot community provider. |
2 tasks
|
What's missing in this PR? |
Collaborator
Review SummaryVerdict: minor-fixes-needed This is a well-crafted PR — the Copilot provider is structurally sound, thoroughly tested (87 test cases across 7 files), and the shared utility extraction from Pi is clean with backward-compat re-exports. Six items need attention before merge: two syntax errors in re-export files, two missing test files for new shared modules, and three docs gaps (stale error message, missing model strings, missing Copilot config section). Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
Collaborator
|
@danielscholl — after a deep review pass, I'm picking this PR as the canonical Copilot community provider. Closing #1351 with credit to @popemkt; you're now the single PR to land. Before merge, here's the consolidated blocker list (all from the auto-posted maintainer-review above — none are architectural, all are addressable): Must fix (6 HIGH)
Should fix (3 MEDIUM — docs reference table)
Specifically flagging one non-obvious thing
LOW findings (log-level upgrades, optional system-chunk surfacing for Credit for @popemktI want to be explicit: @popemkt's #1351 did the foundational work here, addressed the three concerns you raised on their PR (post-#1463 cleanup, lazy-loading SDK imports, Ping me when the blockers are addressed and I'll do a final pass. |
This was referenced May 14, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/providers/src/shared/skills.test.ts (1)
13-29: ⚡ Quick winAdd explicit return types for local test helpers.
Line 13 and Line 20 rely on inference; this violates the strict TS annotation rule used in this repo.
Suggested patch
+type FakeWorld = { + root: string; + cwd: string; + home: string; + stageSkill: (under: 'cwd' | 'home', subdir: '.agents' | '.claude', name: string) => string; +}; + -function makeFakeWorld() { +function makeFakeWorld(): FakeWorld { const root = mkdtempSync(join(tmpdir(), 'archon-skills-test-')); const cwd = join(root, 'project'); const home = join(root, 'home'); mkdirSync(cwd, { recursive: true }); mkdirSync(home, { recursive: true }); - const stageSkill = (under: 'cwd' | 'home', subdir: '.agents' | '.claude', name: string) => { + const stageSkill = ( + under: 'cwd' | 'home', + subdir: '.agents' | '.claude', + name: string, + ): string => {As per coding guidelines, "**/*.{ts,tsx}: Use strict TypeScript configuration with complete type annotations for all functions; no
anytypes without explicit justification".🤖 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 `@packages/providers/src/shared/skills.test.ts` around lines 13 - 29, The helper functions are missing explicit TypeScript return types: add a return type annotation to makeFakeWorld (e.g. an object type with root: string, cwd: string, home: string, and stageSkill: (...)= > string) and add an explicit function type to the inner stageSkill (e.g. (under: 'cwd' | 'home', subdir: '.agents' | '.claude', name: string) => string); update the signature of makeFakeWorld and the stageSkill declaration so both have those explicit types matching their returned values.
🤖 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 `@packages/providers/src/shared/skills.test.ts`:
- Around line 88-93: The test titled "prefers cwd over home when the same name
exists in both" stages the home copy into the wrong directory ('.claude'), so
update the test to stage the home skill into '.agents' to match its intent;
modify the call to fake.stageSkill('home', '.claude', 'delta') to use '.agents'
instead, leaving the cwd staging (fake.stageSkill('cwd', '.agents', 'delta'))
and the assertions around resolveSkillDirectories(fake.cwd, ['delta'])
unchanged.
---
Nitpick comments:
In `@packages/providers/src/shared/skills.test.ts`:
- Around line 13-29: The helper functions are missing explicit TypeScript return
types: add a return type annotation to makeFakeWorld (e.g. an object type with
root: string, cwd: string, home: string, and stageSkill: (...)= > string) and
add an explicit function type to the inner stageSkill (e.g. (under: 'cwd' |
'home', subdir: '.agents' | '.claude', name: string) => string); update the
signature of makeFakeWorld and the stageSkill declaration so both have those
explicit types matching their returned values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65dfbdf6-189a-4766-8839-e99d0cfb579e
📒 Files selected for processing (5)
packages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/providers/package.jsonpackages/providers/src/shared/skills.test.tspackages/providers/src/shared/structured-output.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/docs-web/src/content/docs/guides/authoring-workflows.md
- packages/docs-web/src/content/docs/reference/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/providers/package.json
added 16 commits
May 20, 2026 09:04
Define CopilotProviderDefaults with model, reasoning effort, and auth options Include system message injection and CLI path configuration support
Implement full provider with session management, streaming, and binary resolution Include comprehensive test coverage and lazy-load SDK pattern
Export CopilotProvider, config parser, and binary resolver utilities Register Copilot provider in community providers initialization
Include streaming verification, token validation, and interrupt handling Verify connectivity, output plumbing, and session management
…ments Map Archon `max` effort to SDK `xhigh` and extend sendAndWait timeout to 60min Handle fork-session requests with fresh session creation fallback
…el default Add COPILOT_MODEL env var with envOverrides tracking across config system Update provider to default model to 'auto' and enhance settings UI
Implement full Copilot SDK feature translation including tool restrictions, session config assembly, and best-effort JSON parsing for structured output
test(copilot): cover env token precedence and override behavior
delete model-ref.ts and model-ref.test.ts update copilot index and registration to drop isCopilotModelCompatible export
…sing return undefined if parsed JSON is not an object add tests covering non-object JSON in structured output parsing
implement isExecutableFile using stat/access and use it in path resolution update errors to reference executable file and chmod guidance
export resolveFromPath and prefer PATH result when executable
- rename e2e-copilot-abort.yaml to test-workflows/e2e-copilot-abort.yaml - add e2e-copilot-all-features.yaml and relocate smoke workflow to test-workflows
update providers to re-export shared implementations expose shared utilities: tryParseStructuredOutput, augmentPromptForJsonSchema
update registry tests to cover copilot provider registration verify no collision with built-ins and copilot appears in lists
update event-bridge to emit no system chunk on session.error add provider-hardening tests for abort, trim model config and cleanup
refactor multiple files into sections for fixtures, demos, and checks
delete old e2e-copilot-smoke workflow extend Copilot smoke tests to cover all node types and structured outputs
use DEFAULT_AI_ASSISTANT env var to select default ai assistant update tests and docs to reflect new default and env var usage
introduce COPILOT_GITHUB_TOKEN and generic GH tokens; track tokenSource reorder provider registration to register Pi before Copilot
use isExecutableFile for vendor and autodetect checks validate skill names to reject absolute or traversal paths
- Add packages/providers/src/shared/structured-output.test.ts covering augmentPromptForJsonSchema, the happy-path clean parse, fence stripping (both ```json and bare ```), the forward-brace scan recovery for reasoning-model prose preamble, fence + preamble combo, whitespace trimming, invalid JSON, empty input, and the bare-primitive rejection contract (null/number/string/boolean). - Add packages/providers/src/shared/skills.test.ts covering empty/null inputs, non-string and empty-string skipping, missing skills, cwd vs home resolution order, cwd-shadows-home semantics, deduplication, and the name-only contract (rejection of absolute paths, nested paths, and parent traversal). Uses a staged temp HOME so reads are isolated. - Wire both new test files into packages/providers/package.json so they run in CI as separate bun test invocations. - Add `copilot` to the registered-providers list in the validation error example at guides/authoring-workflows.md, add a Copilot bullet to the Model strings section, and add an AI Providers -- Copilot env-var subsection plus DEFAULT_AI_ASSISTANT enumeration to reference/configuration.md. The two duplicate-import HIGH findings from the May 14 review were hallucinations — the imports don't exist in the current branch — so they need no fix.
- Update loadMcpConfig import to ../../mcp/config — coleam00#1459 (Codex MCP nodes) extracted it out of claude/provider.ts into its own module. - Regenerate bun.lock from current dev (configVersion: 1). Old commits on this branch carried configVersion: 0; rebased forward unchanged but produced different transitive resolution on install (telegram markdown tests fail locally despite identical telegramify-markdown pin). bun install re-adds @github/copilot-sdk on top of the fresh lockfile.
- Stage the home copy of `delta` in `.agents` (not `.claude`) so the "prefers cwd over home" precedence test actually verifies precedence within `.agents`. Previously the home copy was in `.claude`, which could not have beaten the cwd `.agents` copy regardless of the resolver's behavior. - Add explicit return types on `makeFakeWorld` and the inner `stageSkill` to satisfy the project's strict TS annotation rule.
danielscholl
force-pushed
the
personal/copilot-provider
branch
from
bd743e8 to
d2de69d
Compare
added 2 commits
May 20, 2026 09:45
- pi/event-bridge.ts: consolidate the `export-from` + `import-from`
pair on shared/structured-output into the idiomatic
`import { X }; export { X };` form. The preceding comment already
promised "import once for local use and re-export" but the prior
order said the opposite.
- authoring-workflows.md: add `copilot` to the prose listing of
registered providers (the example validation error string below it
already includes copilot).
coleam00#1459 (Codex MCP nodes) extracted loadMcpConfig out of claude/provider.ts into a shared mcp/config.ts module. Update the applyMcpServers docblock to reflect that the helper is shared, not Claude-specific.
Contributor
Author
Wirasm
added a commit
to cropse/Archon
that referenced
this pull request
May 25, 2026
Both PRs added a community provider, so they collide on: - providers/registry.ts (import + register block) - providers/registry.test.ts (per-provider describe blocks) - docs-web ai-assistants.md (frontmatter description) - bun.lock + bundled-defaults.generated.ts (regenerated post-merge) Resolution: kept both providers — opencode + copilot — alongside pi. Type-check + @archon/providers tests pass clean. Conflicts surfaced when coleam00#1505 squash-merged into dev as 6ccfb4b.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@github/copilot-sdk, but Archon currently has no path to it. PR feat(providers): add GitHub Copilot community provider (builtIn: false) #1351 already proposes a community-provider integration; this PR offers a smaller-scope alternative with the same goal.gpt-5,gpt-5-mini, and Anthropic via BYOK under existing GitHub billing. Second community provider after Pi (feat(providers): add Pi community provider (@mariozechner/pi-coding-agent) #1270), validating that the seam scales.packages/providers/src/community/copilot/wrapping@github/copilot-sdk, registered viaregisterCopilotProvider()withbuiltIn: false. ~3,500 lines across 32 files. Auth precedence flipped to safer default:copilot loginwins by default; genericGH_TOKEN/GITHUB_TOKENopt-in viauseLoggedInUser: false;COPILOT_GITHUB_TOKENalways honored as Copilot-specific intent.core/infrastructure changes. No envOverrides UI mechanism. No default-assistant config refactor. No promotion tobuiltIn: true. No hooks, no fallback model, no sandbox. Pi behavior preserved byte-for-byte via re-exports. Claude/Codex paths untouched.UX Journey
Before
After
[+]marks fields newly accepted on workflow nodes whenprovider: copilot.Architecture Diagram
Before
After
Connection inventory:
@archon/workflows@archon/providersIAgentProvidercontract unchanged@archon/providers/community/copilot@github/copilot-sdkimport()@archon/providers/community/pi@archon/providers/shared@archon/providers/community/copilot@archon/providers/shared@archon/core/config-loaderSAFE_ASSISTANT_FIELDSmirrorspi: ['model']Label Snapshot
risk: lowsize: Mcore, dependencies, docs, testsproviders:community/copilot,providers:shared,providers:registryChange Metadata
featuremulti— primarilyproviders, with secondary touches todocsandtests. One config-loader line addscopilot: ['model']toSAFE_ASSISTANT_FIELDS(mirrorspi: ['model']line above it). One config-types re-export ofCopilotProviderDefaults(mirrors Claude/Codex/Pi pattern).Linked Issue
shared/)Comparison to #1351
Both PRs deliver the same capability —
provider: copilotwithbuiltIn: falsethrough the community-provider seam. They differ in scope, auth default, and verification:copilot loginwins; env tokens opt-indb/codebases.ts,db/conversations.ts.claude/archon/plans/planning artifactThe auth-precedence flip is the most user-visible difference.
GH_TOKEN/GITHUB_TOKENare commonly set forghCLI / clone helpers / webhooks, and classic GitHub PATs lack Copilot entitlement — picking them up automatically yields a misleading "Session was not created with authentication info" SDK error. This PR ignores them by default;COPILOT_GITHUB_TOKENis treated as explicit Copilot intent and always wins.The
packages/providers/src/community/copilot/implementation itself is roughly equivalent — the divergence is in what surrounds it.Validation Evidence
bun run validate # all 10 packages green end-to-endbun run type-check— all 10 packages exit 0bun run lint— 0 errors, 0 warnings (max-warnings=0)bun run format:check— cleanbun --filter @archon/providers test— full Pi batch + Copilot batch all greencommunity/copilot/(config, binary-resolver + dev variant, event-bridge, provider, provider-hardening, provider-lazy-load), 3 new auth-precedence tests inprovider.test.ts(21 total), 3 new tests inregistry.test.tse2e-copilot-all-nodes-smoke.yaml— 12 nodes (prompt, command, loop, bash, script bun/uv, structured output viaoutput_format, depends_on, when, trigger_rule,$nodeId.outputJSON dot-access). PASS.e2e-copilot-abort.yaml— manual Ctrl-C verifiessession.abort()cleanup. PASS.Security Impact
copilot logincredentials win unless the user explicitly setsCOPILOT_GITHUB_TOKEN(intent signal) oruseLoggedInUser: falseto opt into generic GH tokens. Tokens read from per-request env +process.env, never persisted by Archon.cwdlike other providers.enableConfigDiscovery: falseis the documented default with a trust-boundary comment in the config block.isExecutableFilevalidates the resolved CLI path is a regular file with the exec bit set (POSIX), not a directory or non-executable.Compatibility / Migration
assistants.copilot.modelto.archon/config.yamland supply auth viacopilot login,COPILOT_GITHUB_TOKEN, oruseLoggedInUser: falseto opt intoGH_TOKEN/GITHUB_TOKEN.pi/options-translator.ts,pi/provider.ts, andpi/event-bridge.tsre-export the extractedshared/functions under their historical names. Existing Pi tests pass unchanged.Human Verification
What was personally validated beyond CI:
e2e-copilot-all-nodes-smoke.yaml— 12 nodes including 4 AI nodes, 3 deterministic nodes, 5 DAG-feature nodes. All passed.useLoggedInUserfrom config, verifiedGH_TOKENwas correctly ignored (provider falls back tocopilot login).COPILOT_GITHUB_TOKENround-trip:tokenSourcetelemetry reportscopilot-tokenwhen set.copilot: {}config — provider falls through tomodel: 'auto'anduseLoggedInUser: true.$structured-node.output.status(string, single-quoted byshellQuote) and$structured-node.output.value(number, unquoted) both round-trip correctly.bun --filter @archon/providers testruns the full Pi batch alongside Copilot batches; all green.os.platform()-aware tests + cross-platform contract of@github/copilot-sdkare the safety net.builtIn: trueand the four deferred capabilities.Side Effects / Blast Radius
@archon/providers— newcommunity/copilot/(provider, config, binary resolver, event bridge, capabilities, registration, 7 test files); newshared/(skills + structured-output) extracted from Pi; one registry entry; oneindex.tsexport.@archon/providersPi — three files trimmed (event-bridge.ts,options-translator.ts,provider.ts) to delegate toshared/, with re-exports preserving every public name and signature.@archon/coreconfig — one line inSAFE_ASSISTANT_FIELDS; one re-export ofCopilotProviderDefaultsinconfig-types.ts.packages/docs-web— Copilot section added to AI-assistants page, ordered after Pi.bun.lock— pulls in@github/copilot-sdk@^0.2.2and transitive deps.provider: copilotnow load successfully where they previously errored.provider.test.ts(auth precedence: 5 cases includingCOPILOT_GITHUB_TOKENintent anduseLoggedInUser:falseopt-in), 6 hardening tests, capability flags surfacing as warnings (not crashes) at workflow load.Rollback Plan
dev. Purely additive — no schema changes.provider: copilotfrom workflow YAML orassistants.copilotfrom.archon/config.yaml."Run \copilot login` (default), set COPILOT_GITHUB_TOKEN, or set `useLoggedInUser: false` ..."` message. Capability mismatches surface as system-chunk warnings.Risks and Mitigations
@github/copilot-sdkis pre-1.0 (v0.2.2) — breaking changes possible on minor bumps.^0.2.2. Type-check + Copilot tests gate any API-surface drift.AbortSignalcould leave a Copilot session holding resources up to 24 h.AbortSignal. Provider-hardening tests cover early-abort + cleanup paths.shared/structured-output.ts2-tier parser used by Pi (feat(providers/pi): best-effort structured output via prompt engineering #1297). Total parse failure degrades cleanly viadag.structured_output_missingwarning, not a hard error.enableConfigDiscovery: truewould let the Copilot CLI load repo-level config outside Archon's workflow validation surface.false. Inline config comment notes the trust boundary.os.platform()-aware tests cover the resolver paths; SDK's cross-platform contract is the production safety net.Summary by CodeRabbit
New Features
Workflows
Shared Utilities
Tests
Documentation