Add GitHub Copilot provider support#1111
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:
📝 WalkthroughWalkthroughThis pull request adds GitHub Copilot as a third supported AI assistant alongside Claude and Codex. Changes span configuration systems, CLI setup, workflow execution engines, schema validation, API endpoints, web UI components, and documentation across multiple packages. Changes
Sequence DiagramsequenceDiagram
participant User
participant Workflow Loader
participant Executor
participant Config System
participant Copilot Client
participant Copilot SDK
participant User Output
User->>Workflow Loader: Load workflow with provider: 'copilot'
Workflow Loader->>Workflow Loader: Validate provider in schema
Workflow Loader->>Executor: Execute DAG with copilot provider
Executor->>Config System: Resolve provider/model
Config System-->>Executor: Return 'copilot' provider
Executor->>Executor: Build AssistantOptions with systemPrompt
Executor->>Copilot Client: sendQuery(prompt, cwd, resumeSessionId?, options)
Copilot Client->>Copilot Client: Validate options (reject forkSession)
Copilot Client->>Copilot SDK: Create/Resume session with model, system message
Copilot SDK-->>Copilot Client: Session created, ready for query
Copilot Client->>Copilot SDK: Send query, subscribe to events
loop Stream Events
Copilot SDK->>Copilot Client: SessionEvent (assistant, reasoning, tool, result)
Copilot Client->>Copilot Client: Convert event to MessageChunk
Copilot Client-->>Executor: Yield MessageChunk
Executor-->>User Output: Stream chunk
end
Copilot SDK-->>Copilot Client: Session idle result with sessionId
Copilot Client->>Copilot Client: Push final result chunk
Copilot Client->>Copilot SDK: Disconnect session
Copilot Client-->>Executor: Complete async generator
Executor-->>User: Workflow result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/setup.ts (1)
663-669:⚠️ Potential issue | 🟠 MajorDon't persist Claude as the default when the user picked no assistant.
This branch still returns
defaultAssistant: 'claude', sogenerateEnvContent()writesDEFAULT_AI_ASSISTANT=claudeeven after the wizard says no assistant was selected. That makes the saved config contradict the prompt flow and can force the runtime onto an unconfigured Claude default.Suggested shape change
interface SetupConfig { ai: { claude: boolean; claudeAuthType?: 'global' | 'apiKey' | 'oauthToken'; claudeApiKey?: string; claudeOauthToken?: string; codex: boolean; codexTokens?: CodexTokens; - defaultAssistant: 'claude' | 'codex' | 'copilot'; + defaultAssistant?: 'claude' | 'codex' | 'copilot'; }; } if (!hasClaude && !hasCodex && !hasCopilot) { log.warning('No AI assistant selected. You can add one later by running `archon setup` again.'); return { claude: false, codex: false, - defaultAssistant: 'claude', }; } // Default AI Assistant lines.push('# Default AI Assistant'); - lines.push(`DEFAULT_AI_ASSISTANT=${config.ai.defaultAssistant}`); - lines.push(''); + if (config.ai.defaultAssistant) { + lines.push(`DEFAULT_AI_ASSISTANT=${config.ai.defaultAssistant}`); + lines.push(''); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 663 - 669, The branch that triggers when hasClaude, hasCodex, and hasCopilot are all false must not return defaultAssistant: 'claude'; update that return to use a falsy/empty value (e.g., defaultAssistant: '' or null) so generateEnvContent() will skip writing DEFAULT_AI_ASSISTANT; locate the early-return in the setup flow where hasClaude/hasCodex/hasCopilot are checked and change defaultAssistant accordingly (ensure any callers of the returned object handle the empty/null value).
🧹 Nitpick comments (3)
packages/workflows/src/model-validation.test.ts (1)
63-80: Nice Copilot coverage addition; consider parity check for undefined-model behavior.For completeness, add
expect(isModelCompatible('copilot')).toBe(true)in the existing undefined-model test block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/model-validation.test.ts` around lines 63 - 80, Add a parity check for undefined model behavior by updating the undefined-model test case to include an expectation that isModelCompatible('copilot') returns true; specifically, in the test that currently asserts empty-string behavior for providers ('claude','codex','copilot'), also add expect(isModelCompatible('copilot')).toBe(true) so the undefined-model case is covered for the copilot provider (reference: isModelCompatible and the test block handling empty/undefined models).packages/workflows/src/dag-executor.test.ts (1)
4629-4661: Align the mocked client type with Copilot in this test.Line 4630 uses
createMockDeps()(defaultgetType: 'claude'). SettinggetTypeto'copilot'here makes this test resilient if execution logic branches on client type in addition to provider selection.Suggested patch
it('passes Copilot provider and systemPrompt through to sendQuery', async () => { + mockGetAssistantClientDag.mockReturnValue({ + sendQuery: mockSendQueryDag, + getType: () => 'copilot', + }); + const mockDeps = createMockDeps(); const platform = createMockPlatform(); const workflowRun = makeWorkflowRun();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 4629 - 4661, The test uses createMockDeps() which defaults to a Claude client type; update the test so the mocked client aligns with Copilot by calling createMockDeps with getType set to 'copilot' (so the mocked client returned by createMockDeps matches the provider under test), then run the existing assertions (this affects the setup used by executeDagWorkflow and ensures mockGetAssistantClientDag/mockSendQueryDag behavior matches the Copilot branch).packages/workflows/src/deps.ts (1)
278-280: Makeassistants.copilotrequired in this contract.
loadConfig()always materializesassistants.copilotfrom defaults, so marking it optional here weakens the type for no runtime benefit and makes provider-indexed access look less safe than it is.Suggested tweak
- copilot?: { + copilot: { model?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/deps.ts` around lines 278 - 280, The assistants.copilot property in the type is currently optional (copilot?) but loadConfig() always materializes it, so remove the optional marker and make it required by changing "copilot?: { model?: string; }" to "copilot: { model?: string; }" in the type definition in packages/workflows/src/deps.ts; leave the inner model field as-is (model?: string) unless you also want it required, and run type checks to ensure callers relying on optional copilot are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/clients/copilot.ts`:
- Around line 242-249: The current logic silently falls back to creating a new
session when client.resumeSession(resumeSessionId, sessionConfig) fails (setting
sessionResumeFailed and calling client.createSession), which loses prior
conversation context; change this so that when resumeSessionId is present and
client.resumeSession throws, you do not create a fresh session but instead
propagate a clear error or return a structured failure status (e.g. throw a
descriptive error or return { status: 'resume_failed', reason }) from the
surrounding function so callers must handle it explicitly; update any uses of
sessionResumeFailed/session to reflect the new error/structured-status flow and
ensure callers of the function handle the new failure case.
- Around line 176-203: collectUnsupportedOptionNames currently silently returns
unsupported option names and lets callers drop session-semantic flags; detect if
options.persistSession or options.forkSession are set inside
collectUnsupportedOptionNames and throw a clear Error (or a specific exception)
explaining that session-semantic options are unsupported here (mention
forkSession/persistSession) so callers like executeNodeInternal cannot
accidentally mutate the source session; keep returning the other unsupported
option names for non-session flags as before.
In `@packages/core/src/config/config-loader.ts`:
- Around line 91-92: The default config template currently shows a Copilot model
example using "gpt-5" (the commented block "# copilot:\n# model: gpt-5");
update that example to a validated model name "gpt-5-mini" so users aren't
pointed to an unavailable model; locate the default template or constant that
emits the commented Copilot example in config-loader.ts and replace "gpt-5" with
"gpt-5-mini" in that template string.
In `@packages/docs-web/src/content/docs/book/quick-reference.md`:
- Line 111: The docs entry for the `provider` field incorrectly states the
default is `claude`; update the description for `provider` so it states that
when omitted the runtime will use the configured default assistant (e.g., "uses
the configured default assistant" or "defaults to the configured default
assistant") instead of listing `claude` as the default; ensure the table row for
`provider` (the cell currently showing `claude`) is replaced with the corrected
wording so readers understand runtime behavior.
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md`:
- Around line 139-143: Update the YAML example under the assistants->copilot
section that currently shows only "model: gpt-5" to include a fallback note
mentioning "gpt-5-mini" so users with different Copilot entitlements won't fail;
specifically modify the example around the assistants.copilot.model line to
either show both options or add a short explanatory sentence that if "gpt-5" is
not available use "gpt-5-mini" as a fallback.
In `@packages/docs-web/src/content/docs/getting-started/configuration.md`:
- Around line 38-41: The YAML example has a duplicated top-level copilot key
(both showing "copilot:\n model: gpt-5"); remove the duplicate block so only
one copilot entry remains, and if the two blocks differed merge their settings
under a single copilot mapping (preserve "model: gpt-5" and any other fields) to
ensure a valid, unambiguous config.
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Line 405: Update the warning line that currently reads "Claude only — Codex
and Copilot nodes/steps emit a warning and continue (they don't support per-call
tool restrictions)" to remove the outdated "steps" term so it reads "Claude only
— Codex and Copilot nodes emit a warning and continue (they don't support
per-call tool restrictions)"; locate the exact string in the docs file and
replace "nodes/steps" with "nodes" to keep terminology consistent with the rest
of the guide.
In `@packages/docs-web/src/content/docs/guides/mcp-servers.md`:
- Around line 363-364: Update the remediation sentence that currently reads
"**Claude only** — Codex and Copilot nodes warn and ignore the `mcp` field.
Configure MCP servers globally in the Codex CLI config instead." to cover
Copilot users as well: either generalize to "Configure MCP servers globally in
the CLI config" or explicitly mention both tools (e.g., "Configure MCP servers
globally in the Codex or Copilot CLI config"). Keep the bold label `**Claude
only**`, retain mention of the `mcp` field, and replace the single-tool
instruction "Codex CLI config" with the generalized or dual-tool wording so
Copilot users are given correct remediation guidance.
In `@packages/docs-web/src/content/docs/guides/skills.md`:
- Line 16: The later sections ("Compatibility", "Troubleshooting", any
warnings/examples) still refer to Codex-only behavior; update those sections to
match the intro by explicitly mentioning both Codex and Copilot ignore the
skills field, change any "Codex-only" phrasing to "Codex and Copilot", update
examples/warnings that mention only Codex (including any code comments or bullet
points) to include Copilot, and ensure any troubleshooting steps that suggest
Codex-specific workarounds are broadened or split into Codex/Copilot variants so
the guidance is consistent with the intro.
In `@packages/docs-web/src/content/docs/reference/configuration.md`:
- Around line 54-55: The documentation row for DEFAULT_AI_ASSISTANT (and the
example defaultAssistant: claude) is out of sync: update the env-var docs where
DEFAULT_AI_ASSISTANT is listed to include 'copilot' alongside 'claude' and
'codex' so the examples and table match; ensure any examples or descriptive text
referencing defaultAssistant, DEFAULT_AI_ASSISTANT, or the sample values mention
'copilot' as an allowed value.
- Around line 69-72: The YAML example contains a duplicated top-level key
"copilot" which confuses readers and YAML parsers; remove the duplicate entry so
only a single "copilot" mapping remains (or merge the two mappings into one if
they contain different fields) and ensure the final example shows the intended
model setting (e.g., copilot: model: gpt-5) under a single "copilot" key.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 514-515: The PATCH settings handler in api.ts currently reads and
persists only the `claude`/`codex` fields and drops `copilot`, so the UI payload
(which includes `copilot`) is ignored; update the PATCH handler to accept
`copilot` from the request body and merge/persist it alongside `claude` and
`codex`. Concretely, locate the PATCH handler function in
packages/server/src/routes/api.ts (the route that updates user settings), add
`copilot` to the destructured/validated body fields, include it when building
the updated settings object (the same place `claude` and `codex` are set), and
ensure the persistence call (e.g., saveUserSettings/updateUser or whatever
persists settings) writes the `copilot` value so the setting survives reloads.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 405-420: The current branch that checks codexLikeProvider and
node.allowed_tools/denied_tools should not silently continue; instead, fail
fast: when codexLikeProvider is true and node.allowed_tools or node.denied_tools
are set, log an error (include node.id, workflowRunId, provider/providerLabel)
and either throw a clear Error to abort execution or mark the node run as failed
rather than proceeding with approveAll; still attempt to notify via
safeSendMessage (using conversationId/workflowRunId/node.id) but treat delivery
failure as secondary—do not proceed with execution of the node. Ensure you
update the logic around codexLikeProvider, the conditional that references
node.allowed_tools/node.denied_tools, and any downstream use of approveAll so
those paths are unreachable when this error is raised.
In `@packages/workflows/src/loader.ts`:
- Around line 273-276: The current assignment to the local variable provider
silently maps unsupported raw.provider values to undefined; instead, in the
loader code where provider is derived from raw.provider (the provider variable),
validate raw.provider explicitly: if raw.provider is undefined allow undefined,
if it equals one of the supported strings ('claude', 'codex', 'copilot') use it,
otherwise throw a validation error (or return a validation failure) with a clear
message that raw.provider is unsupported so callers fail fast rather than
falling back to an unintended default.
---
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 663-669: The branch that triggers when hasClaude, hasCodex, and
hasCopilot are all false must not return defaultAssistant: 'claude'; update that
return to use a falsy/empty value (e.g., defaultAssistant: '' or null) so
generateEnvContent() will skip writing DEFAULT_AI_ASSISTANT; locate the
early-return in the setup flow where hasClaude/hasCodex/hasCopilot are checked
and change defaultAssistant accordingly (ensure any callers of the returned
object handle the empty/null value).
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 4629-4661: The test uses createMockDeps() which defaults to a
Claude client type; update the test so the mocked client aligns with Copilot by
calling createMockDeps with getType set to 'copilot' (so the mocked client
returned by createMockDeps matches the provider under test), then run the
existing assertions (this affects the setup used by executeDagWorkflow and
ensures mockGetAssistantClientDag/mockSendQueryDag behavior matches the Copilot
branch).
In `@packages/workflows/src/deps.ts`:
- Around line 278-280: The assistants.copilot property in the type is currently
optional (copilot?) but loadConfig() always materializes it, so remove the
optional marker and make it required by changing "copilot?: { model?: string; }"
to "copilot: { model?: string; }" in the type definition in
packages/workflows/src/deps.ts; leave the inner model field as-is (model?:
string) unless you also want it required, and run type checks to ensure callers
relying on optional copilot are updated.
In `@packages/workflows/src/model-validation.test.ts`:
- Around line 63-80: Add a parity check for undefined model behavior by updating
the undefined-model test case to include an expectation that
isModelCompatible('copilot') returns true; specifically, in the test that
currently asserts empty-string behavior for providers
('claude','codex','copilot'), also add
expect(isModelCompatible('copilot')).toBe(true) so the undefined-model case is
covered for the copilot provider (reference: isModelCompatible and the test
block handling empty/undefined models).
🪄 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: b64de0cd-574d-4861-ad74-eeeb2f1c0b6d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
packages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/core/package.jsonpackages/core/src/clients/copilot.test.tspackages/core/src/clients/copilot.tspackages/core/src/clients/factory.test.tspackages/core/src/clients/factory.tspackages/core/src/clients/index.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/index.tspackages/core/src/types/index.tspackages/docs-web/src/content/docs/book/quick-reference.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/getting-started/overview.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/hooks.mdpackages/docs-web/src/content/docs/guides/mcp-servers.mdpackages/docs-web/src/content/docs/guides/skills.mdpackages/docs-web/src/content/docs/reference/architecture.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/server/src/routes/schemas/config.schemas.tspackages/web/src/components/workflows/BuilderToolbar.tsxpackages/web/src/components/workflows/NodeInspector.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/lib/api.generated.d.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/executor.test.tspackages/workflows/src/executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.test.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/setup.ts (1)
41-49:⚠️ Potential issue | 🟡 MinorPersist Copilot selection in
config.ai.
copilotis selectable in the wizard, but the setup state still only models Claude/Codex. If the user picks only Copilot, Line 1611 still rendersAI: None configured, and if they pick Copilot as a non-default assistant that choice is lost on the next setup run because neitherSetupConfignorExistingConfigcan represent it. Please either thread acopilot: booleanthrough the config/summaries, or make Copilot a pure default-assistant choice instead of a persisted multiselect option.Also applies to: 93-97, 545-552, 561-563, 690-722, 1397-1402, 1595-1612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 41 - 49, The config schema and summaries don't persist Copilot selection — add a copilot: boolean to the ai object and include 'copilot' in the defaultAssistant union so SetupConfig and ExistingConfig can represent it; update the wizard state/read/write logic that builds/loads config.ai (functions that create or merge setup state and save to disk), the summary/print routine that derives "AI: None configured" (where it checks claude/codex), and any UI state that toggles multi-select to read/write this new field so a user who selects only Copilot is persisted and shown on subsequent runs. Ensure serialization/deserialization and type definitions for ai (claude, codex, copilot, claudeAuthType, codexTokens, defaultAssistant) are updated consistently.
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.ts (1)
405-516: Normalize new log event names and context payloads.The newly added log keys use mixed formats (
dag_node_*anddag.*) and some records omit stable context IDs (workflowRunId,provider) that would help query/alert consistency.♻️ Suggested normalization pattern
- getLog().error({ nodeId: node.id, workflowRunId, provider, ... }, 'dag_node_tool_restrictions_unsupported'); + getLog().error({ nodeId: node.id, workflowRunId, provider, ... }, 'dag.tool_restrictions_failed'); - getLog().warn({ nodeId: node.id, provider }, 'dag_node_hooks_ignored'); + getLog().warn({ nodeId: node.id, workflowRunId, provider }, 'dag.hooks_ignored'); - getLog().warn({ nodeId: node.id }, 'dag.output_format_ignored_copilot'); + getLog().warn({ nodeId: node.id, workflowRunId, provider: 'copilot' }, 'dag.output_format_ignored'); - getLog().warn({ nodeId: node.id }, 'loop_node.system_prompt_ignored_codex'); + getLog().warn({ nodeId: node.id, workflowRunId: workflowRun.id, provider: 'codex' }, 'loop.system_prompt_ignored');As per coding guidelines, "Use structured logging with Pino; event naming format:
{domain}.{action}_{state}; ... include context IDs and error details."Also applies to: 1804-1816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 405 - 516, The log/event names and context payloads in the Codex/Copilot checks are inconsistent and omit stable IDs; update all getLog().warn/getLog().error calls in this block (the checks around getLog(), safeSendMessage, node.id, provider, providerLabel, workflowRunId) to use the normalized event naming pattern domain.action_state (e.g., dag.node_tool_restrictions_unsupported, dag.node_hooks_ignored, dag.mcp_ignored, dag.skills_ignored, dag.claude_options_ignored, dag.output_format_ignored) and ensure each log call includes the same structured context keys { nodeId: node.id, workflowRunId, provider } (add provider/workflowRunId where missing); also include error details when logging delivery failures by passing the delivery result/error into processLogger or getLog().error so the message contains actionable error info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Line 191: Update the "Provider and Model" example block and its inline comment
to match the table which lists providers 'claude', 'codex', and 'copilot';
locate the example that currently contains "provider: claude # 'claude' or
'codex'" and change the comment to include 'copilot' (e.g., "# 'claude', 'codex'
or 'copilot'") and, if the example uses a hard-coded provider value for
demonstration, consider showing a neutral example or mentioning all three
options so the guide is consistent with the table.
In `@packages/workflows/src/validator.ts`:
- Around line 247-253: The current resolveProvider function ignores node.model
and so can validate a different provider than the executor will use; update
resolveProvider (or extract a shared resolveProvider/resolveModel helper used by
both packages/workflows/src/validator.ts and
packages/workflows/src/dag-executor.ts) to apply the exact inference precedence
the executor uses: prefer explicit node.provider, then node.model-based mapping,
then workflow-level model config, then config-file defaults, then SDK defaults;
ensure the same resolver is called when performing provider-specific validations
for mcp, skills, hooks, allowed_tools/denied_tools and Copilot output_format so
load-time validation matches runtime behavior.
---
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 41-49: The config schema and summaries don't persist Copilot
selection — add a copilot: boolean to the ai object and include 'copilot' in the
defaultAssistant union so SetupConfig and ExistingConfig can represent it;
update the wizard state/read/write logic that builds/loads config.ai (functions
that create or merge setup state and save to disk), the summary/print routine
that derives "AI: None configured" (where it checks claude/codex), and any UI
state that toggles multi-select to read/write this new field so a user who
selects only Copilot is persisted and shown on subsequent runs. Ensure
serialization/deserialization and type definitions for ai (claude, codex,
copilot, claudeAuthType, codexTokens, defaultAssistant) are updated
consistently.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 405-516: The log/event names and context payloads in the
Codex/Copilot checks are inconsistent and omit stable IDs; update all
getLog().warn/getLog().error calls in this block (the checks around getLog(),
safeSendMessage, node.id, provider, providerLabel, workflowRunId) to use the
normalized event naming pattern domain.action_state (e.g.,
dag.node_tool_restrictions_unsupported, dag.node_hooks_ignored, dag.mcp_ignored,
dag.skills_ignored, dag.claude_options_ignored, dag.output_format_ignored) and
ensure each log call includes the same structured context keys { nodeId:
node.id, workflowRunId, provider } (add provider/workflowRunId where missing);
also include error details when logging delivery failures by passing the
delivery result/error into processLogger or getLog().error so the message
contains actionable error info.
🪄 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: 7d895e44-09bb-413d-9112-b46ee4bcb52e
📒 Files selected for processing (21)
packages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/core/src/clients/copilot.test.tspackages/core/src/clients/copilot.tspackages/core/src/config/config-loader.tspackages/docs-web/src/content/docs/book/quick-reference.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/mcp-servers.mdpackages/docs-web/src/content/docs/guides/skills.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/docs-web/src/content/docs/reference/security.mdpackages/server/src/routes/api.health.test.tspackages/server/src/routes/api.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
✅ Files skipped from review due to trivial changes (6)
- packages/docs-web/src/content/docs/getting-started/configuration.md
- packages/docs-web/src/content/docs/guides/mcp-servers.md
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
- packages/workflows/src/loader.ts
- packages/docs-web/src/content/docs/guides/skills.md
- packages/docs-web/src/content/docs/book/quick-reference.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/workflows/src/loader.test.ts
- packages/cli/src/commands/setup.test.ts
- packages/workflows/src/validator.test.ts
- packages/core/src/config/config-loader.ts
- packages/core/src/clients/copilot.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/validate.ts (1)
30-35: Consider a deterministic CI mode forvalidate.Using
loadConfig(cwd)pulls global (~/.archon) and env overrides, which can makearchon validateresults differ across machines. A repo-only mode (or explicit CI guidance) would reduce drift while keeping runtime-parity as default.Based on learnings: “Prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic — no flaky timing or network dependence without guardrails; ensure local validation commands (
bun run validate) map directly to CI expectations.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/validate.ts` around lines 30 - 35, The validate command currently calls loadConfig(cwd) which merges global (~/.archon) and env overrides and can cause non-deterministic CI results; add a deterministic "repo-only/CI" mode (e.g., respect an explicit flag like --ci or process.env.CI) in the validate command and, when set, call loadConfig in repo-only mode (or call a new loadRepoConfig/loadConfig(cwd, { repoOnly: true })) so it ignores user-global and env overrides; keep the existing default behavior when the flag/var is absent and ensure the returned mergedConfig fields (defaults.loadDefaultCommands, commands.folder, assistant) come from the repo-only config when CI mode is enabled.
🤖 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/cli/src/commands/validate.ts`:
- Around line 30-35: The validate command currently calls loadConfig(cwd) which
merges global (~/.archon) and env overrides and can cause non-deterministic CI
results; add a deterministic "repo-only/CI" mode (e.g., respect an explicit flag
like --ci or process.env.CI) in the validate command and, when set, call
loadConfig in repo-only mode (or call a new loadRepoConfig/loadConfig(cwd, {
repoOnly: true })) so it ignores user-global and env overrides; keep the
existing default behavior when the flag/var is absent and ensure the returned
mergedConfig fields (defaults.loadDefaultCommands, commands.folder, assistant)
come from the repo-only config when CI mode is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88536e8a-6513-4382-8287-82ef65744813
📒 Files selected for processing (8)
packages/cli/src/commands/validate.tspackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
✅ Files skipped from review due to trivial changes (3)
- packages/docs-web/src/content/docs/getting-started/configuration.md
- packages/docs-web/src/content/docs/reference/configuration.md
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/workflows/src/validator.test.ts
- packages/workflows/src/validator.ts
- packages/workflows/src/dag-executor.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/docs-web/src/content/docs/guides/authoring-workflows.md (1)
545-546: Consider making the model default comment provider-agnostic.
model: sonnet # ... assistants.claude.modelcan mislead in this generic section. Suggest wording likeassistants.<provider>.model(or “provider-specific config default”) to avoid copy/paste confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md` around lines 545 - 546, The inline comment next to the "model: sonnet" example is claude-specific and may mislead readers; update the comment to be provider-agnostic by replacing "assistants.claude.model" with a generic reference like "assistants.<provider>.model" or "provider-specific config default" so the example "model: sonnet" is clearly not tied to Claude; change the comment text next to the "model: sonnet" line in the authoring-workflows.md snippet accordingly.
🤖 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/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Around line 545-546: The inline comment next to the "model: sonnet" example is
claude-specific and may mislead readers; update the comment to be
provider-agnostic by replacing "assistants.claude.model" with a generic
reference like "assistants.<provider>.model" or "provider-specific config
default" so the example "model: sonnet" is clearly not tied to Claude; change
the comment text next to the "model: sonnet" line in the authoring-workflows.md
snippet accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8ec6cba-f76b-40e9-8402-2ec1e0b1603a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/core/package.jsonpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/docs-web/src/content/docs/reference/security.md
✅ Files skipped from review due to trivial changes (2)
- packages/core/package.json
- packages/docs-web/src/content/docs/reference/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/docs-web/src/content/docs/reference/security.md
260c500 to
d43ce62
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workflows/src/deps.ts (1)
261-270:⚠️ Potential issue | 🟠 Major
AgentProviderFactorymust accept'copilot'to match the rest of the workflow surface.The type excludes
'copilot', butWorkflowProvider(used at call sites indag-executor.ts) and bothAssistantClientFactoryandWorkflowConfig.assistantalready include it. This creates a type safety gap: passing aWorkflowProvidervalue todeps.getAgentProvider()will fail type-checking when the provider is'copilot'.Proposed fix
-export type AgentProviderFactory = (provider: 'claude' | 'codex') => IAgentProvider; +export type AgentProviderFactory = ( + provider: 'claude' | 'codex' | 'copilot' +) => IAgentProvider;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/deps.ts` around lines 261 - 270, AgentProviderFactory currently only allows 'claude' | 'codex' which conflicts with other types that include 'copilot'; update the AgentProviderFactory type to include 'copilot' (so getAgentProvider on WorkflowDeps accepts the same provider union as WorkflowProvider/WorkflowConfig.assistant/AssistantClientFactory) and run type-checking to ensure all call sites (e.g., usages in dag-executor.ts) compile without changes.
♻️ Duplicate comments (1)
packages/docs-web/src/content/docs/guides/authoring-workflows.md (1)
191-191:⚠️ Potential issue | 🟡 MinorThere is still a later provider example mismatch.
After expanding provider support at Line 191, the later “Provider and Model” example still says
# 'claude' or 'codex'(Line 545), which reintroduces mixed guidance. Please includecopilotthere too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md` at line 191, Update the “Provider and Model” example text that currently reads "# 'claude' or 'codex'" to include 'copilot' as well so the example matches the expanded provider support; locate the example comment in the same document where the table row for `provider` lists `'claude' | 'codex' | 'copilot'` and change the example annotation to "# 'claude', 'codex' or 'copilot'".
🧹 Nitpick comments (1)
packages/server/src/routes/api.health.test.ts (1)
468-474: Assert a single persistence write in this test.Add a call-count assertion so duplicate writes can’t slip through while still matching the expected payload.
Suggested diff
expect(response.status).toBe(200); + expect(mockUpdateGlobalConfig).toHaveBeenCalledTimes(1); expect(mockUpdateGlobalConfig).toHaveBeenCalledWith({ defaultAssistant: 'copilot', assistants: { copilot: { model: 'gpt-5-mini' }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.health.test.ts` around lines 468 - 474, The test currently asserts the payload passed to mockUpdateGlobalConfig but not the number of times it was called—add a call-count assertion to ensure only a single persistence write occurs; after the existing expect(mockUpdateGlobalConfig).toHaveBeenCalledWith(...) add expect(mockUpdateGlobalConfig).toHaveBeenCalledTimes(1) (referencing mockUpdateGlobalConfig in the same test in packages/server/src/routes/api.health.test.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/package.json`:
- Line 3: The package.json version in packages/core currently regresses to
"0.3.5"; update the "version" field in packages/core/package.json to a
forward-compatible release (at least "0.3.6", recommend "0.3.7") to restore
semver ordering and avoid breaking the publish flow, and then ensure any other
manifests or dependency references that expect the newer `@archon/core` version
are updated to match the bumped version.
- Line 26: The package.json "test" npm script is split into too many bun test
invocations; collapse and regroup the listed tests back into the repo's standard
7-batch pattern used for handling conflicting mock.module() calls. Update the
"test" script to reorganize the existing test file groups (the various src/...
bun test segments) into exactly seven bun test invocations that each contain the
appropriate related test files (preserving separation for files that use
mock.module()), ensuring the new grouping mirrors other packages' 7-batch layout
and keeps conflicting tests in separate invocations.
- Around line 31-32: Multiple files still import symbols removed with
`@archon/providers`; restore the public API by adding a
packages/core/src/clients/index.ts that re-exports or implements the missing
symbols used across the codebase: export getAgentProvider (function),
SendQueryOptions (type), ClaudeProviderDefaults and CodexProviderDefaults
(types), and MessageChunk (type), then update the imports in store-adapter.ts,
title-generator.ts, orchestrator-agent.ts, config-types.ts, and types/index.ts
to import those symbols from the new clients module (or alternatively re-export
them from core's public API by creating clients/index.ts that re-exports from
`@archon/providers` if you choose to keep that package); ensure types and
signatures match the existing call sites so type-check and runtime behavior
remain unchanged.
In `@packages/docs-web/src/content/docs/guides/skills.md`:
- Around line 209-214: The fenced code block containing the two "Warning: Node
'review'..." lines in packages/docs-web/src/content/docs/guides/skills.md is
missing a language tag; update that fence to include a language identifier
(e.g., change ``` to ```text) so the block is a tagged fence and satisfies MD040
linting while preserving the warning lines exactly.
In `@packages/providers/src/codex/copilot.test.ts`:
- Around line 13-15: Add copilot.test.ts to the package test runner and stop
using global mock.module() for shared modules: update
packages/providers/package.json test script to include a separate invocation for
this file (e.g., append "&& bun test src/codex/copilot.test.ts") so it runs in
isolation, then in src/codex/copilot.test.ts replace the
mock.module('@archon/paths', ...) call (and the similar call at line 75) with
targeted spying (e.g., spyOn the createLogger export to return mockLogger) where
possible; if isolation via mock.module is truly required, add a brief comment
mirroring binary-guard.test.ts:4 explaining why this test must run isolated and
why mock.module is used. Ensure references to createLogger and mockLogger in the
test are updated to use the spy or documented rationale.
In `@packages/providers/src/codex/copilot.ts`:
- Around line 297-304: In the handler for the 'tool.execution_complete' case,
using toolNamesByCallId.get(event.data.toolCallId) falls back to 'unknown' when
the start event is missing; change the fallback to use the completion payload's
tool name by checking event.data.toolName (i.e., compute toolName =
toolNamesByCallId.get(event.data.toolCallId) ?? event.data.toolName ??
'unknown'), still delete the call id from toolNamesByCallId, and push the queue
item with that resolved toolName so the completion event’s provided name is used
when available.
- Around line 181-191: The current check treats any provided
persistSession/forkSession flags (including false) as unsupported; update the
logic in the block that builds unsupportedSessionOptions (referencing
unsupportedSessionOptions, options.persistSession, options.forkSession) to only
include flags that actively request the behavior (i.e., check for true rather
than !== undefined) so callers spreading normalized defaults with false don’t
get rejected; adjust the filter to detect explicit true requests and leave
false/undefined alone.
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 836-837: The tests reference an undefined mock identifier
mockGetAssistantClientDag; replace both occurrences (the one around the 'fails
when a Codex DAG node sets denied_tools' test and the other at line ~4712) with
the actual mock name used in this suite, mockGetAgentProviderDag, or
alternatively define mockGetAssistantClientDag to point to
mockGetAgentProviderDag; update the two usages so they reference the existing
mock (mockGetAgentProviderDag) and ensure no other tests reference the undefined
identifier.
---
Outside diff comments:
In `@packages/workflows/src/deps.ts`:
- Around line 261-270: AgentProviderFactory currently only allows 'claude' |
'codex' which conflicts with other types that include 'copilot'; update the
AgentProviderFactory type to include 'copilot' (so getAgentProvider on
WorkflowDeps accepts the same provider union as
WorkflowProvider/WorkflowConfig.assistant/AssistantClientFactory) and run
type-checking to ensure all call sites (e.g., usages in dag-executor.ts) compile
without changes.
---
Duplicate comments:
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Line 191: Update the “Provider and Model” example text that currently reads "#
'claude' or 'codex'" to include 'copilot' as well so the example matches the
expanded provider support; locate the example comment in the same document where
the table row for `provider` lists `'claude' | 'codex' | 'copilot'` and change
the example annotation to "# 'claude', 'codex' or 'copilot'".
---
Nitpick comments:
In `@packages/server/src/routes/api.health.test.ts`:
- Around line 468-474: The test currently asserts the payload passed to
mockUpdateGlobalConfig but not the number of times it was called—add a
call-count assertion to ensure only a single persistence write occurs; after the
existing expect(mockUpdateGlobalConfig).toHaveBeenCalledWith(...) add
expect(mockUpdateGlobalConfig).toHaveBeenCalledTimes(1) (referencing
mockUpdateGlobalConfig in the same test in
packages/server/src/routes/api.health.test.ts).
🪄 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: 37d1677a-914b-4821-9428-2c97e75f1354
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
packages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/cli/src/commands/validate.tspackages/core/package.jsonpackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/docs-web/src/content/docs/book/quick-reference.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/getting-started/overview.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/hooks.mdpackages/docs-web/src/content/docs/guides/mcp-servers.mdpackages/docs-web/src/content/docs/guides/skills.mdpackages/docs-web/src/content/docs/reference/architecture.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/docs-web/src/content/docs/reference/security.mdpackages/providers/src/codex/copilot.test.tspackages/providers/src/codex/copilot.tspackages/server/src/routes/api.health.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/config.schemas.tspackages/web/src/components/workflows/BuilderToolbar.tsxpackages/web/src/components/workflows/NodeInspector.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/lib/api.generated.d.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/executor.test.tspackages/workflows/src/executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.test.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
✅ Files skipped from review due to trivial changes (10)
- packages/docs-web/src/content/docs/getting-started/overview.md
- packages/docs-web/src/content/docs/getting-started/configuration.md
- packages/docs-web/src/content/docs/guides/hooks.md
- packages/web/src/components/workflows/WorkflowBuilder.tsx
- packages/core/src/config/config-loader.test.ts
- packages/server/src/routes/api.ts
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
- packages/workflows/src/loader.test.ts
- packages/docs-web/src/content/docs/reference/configuration.md
- packages/docs-web/src/content/docs/book/quick-reference.md
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/docs-web/src/content/docs/guides/mcp-servers.md
- packages/workflows/src/executor.test.ts
- packages/docs-web/src/content/docs/reference/architecture.md
- packages/docs-web/src/content/docs/reference/security.md
- packages/cli/src/commands/validate.ts
- packages/workflows/src/model-validation.test.ts
- packages/web/src/components/workflows/NodeInspector.tsx
- packages/workflows/src/schemas/workflow.ts
- packages/workflows/src/schemas/dag-node.ts
- packages/web/src/routes/SettingsPage.tsx
- packages/workflows/src/executor.ts
- packages/web/src/components/workflows/BuilderToolbar.tsx
- packages/workflows/src/validator.test.ts
- packages/server/src/routes/schemas/config.schemas.ts
- packages/web/src/lib/api.generated.d.ts
- packages/core/src/config/config-loader.ts
- packages/cli/src/commands/setup.ts
- packages/core/src/config/config-types.ts
- packages/workflows/src/dag-executor.ts
- packages/workflows/src/model-validation.ts
- Add CopilotClient implementing IAgentProvider interface - Uses @github/copilot-sdk for session management - Supports model, modelReasoningEffort, systemPrompt options - Handles forkSession/persistSession errors appropriately - Maps SDK events to MessageChunk types (assistant, thinking, tool, tool_result, result) - Add copilot provider factory registration - Add copilot to WorkflowProvider type and deps - Add model validation for copilot models
d43ce62 to
e7acd06
Compare
- Fix persistSession check in CopilotProvider (check === true not !== undefined) - Make assistants.copilot required in WorkflowConfig type - Add CopilotProviderDefaults type and export from providers - Add copilot to all config types (GlobalConfig, MergedConfig, SafeConfig) - Update config-loader to handle copilot in defaults and merging - Update API schemas and handler to accept copilot in PATCH - Add copilot to workflow provider enum - Fail fast for invalid providers instead of silent undefined - Update docs to mention copilot alongside Claude/Codex - Fix tests to expect copilot in assistants config
- Restructure sendQuery to subscribe to events during sendAndWait, then yield content from sendAndWait's return value (which carries the full AssistantMessageEvent with response content) - Defensively yield result token chunk LAST to avoid dag-executor breaking on result before content is delivered - Add copilot to RepoConfig.assistant type union and merge logic - Log copilot.sendAndWait_completed at info level for observability
Resolved conflicts: - Integrated CopilotProvider into the dynamic registry system - Added CopilotProviderDefaults type and capabilities - Accepted origin/dev approach for registry-based provider resolution - Resolved type conflicts using string-based provider IDs - Removed deprecated factory.ts (superseded by registry) - Updated config types to use ProviderDefaultsMap
- Add GitHub Copilot as option in setup wizard - Add missing ProviderDefaults, ProviderDefaultsMap, ProviderRegistration, ProviderInfo types - Fix type errors in dag-executor, executor, and config-loader - Update tests for 3 built-in providers (claude, codex, copilot) - Fix model validation to throw on unknown providers
|
This PR appears to fully address #1115. Consider adding |
|
Any more follow-up on this one @mhingston ? |
|
@popemkt I don't have time to keep up with getting this merged if someone else wants to see it through. |
|
Please feel free to readd this behoiind the community provider registry Will close for now |
Summary
Validation
bun run cli workflow run copilot-smoke --no-worktree "Reply with exactly COPILOT_OK"model: gpt-5was unavailable in this Copilot account, so the smoke test was repeated successfully withgpt-5-mini.Summary by CodeRabbit
Release Notes
New Features
Documentation