feat: add Qwen support to Archon#1050
Closed
LoneWolf36 wants to merge 5 commits intocoleam00:devfrom
Closed
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:
📝 WalkthroughWalkthroughAdds first-class Qwen assistant support across docs, CLI, core clients, workflows, web UI, config/types/schemas, tests, and packaging; implements Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Workflow Executor
participant Loader as Config Loader
participant Qwen as QwenClient
participant SDK as `@qwen-code/sdk`
participant DB as Conversation DB
Executor->>Loader: loadConfig(repo)
Loader-->>Executor: workflow + assistants.qwen config
Executor->>Qwen: sendQuery(prompt, cwd, resume?, options)
Qwen->>Loader: loadConfig(cwd)
Qwen->>SDK: query(prompt, constructedOptions)
SDK-->>Qwen: stream(partial / tool / result events)
Qwen-->>Executor: yield MessageChunk (assistant/tool/result)
Executor->>DB: persist chunk / update conversation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/setup.ts (1)
695-716:⚠️ Potential issue | 🟠 MajorOnly offer selected assistants as the default.
When the user picks any two assistants, this prompt still shows all three. For example, selecting Claude + Qwen can still save
defaultAssistant: 'codex', which writes an invalid.envbecause Codex was never configured.Suggested fix
// Determine default assistant let defaultAssistant: 'claude' | 'codex' | 'qwen' = 'claude'; if ([hasClaude, hasCodex, hasQwen].filter(Boolean).length > 1) { + const defaultOptions = [ + hasClaude ? { value: 'claude', label: 'Claude (Recommended)' } : null, + hasCodex ? { value: 'codex', label: 'Codex' } : null, + hasQwen ? { value: 'qwen', label: 'Qwen' } : null, + ].filter( + ( + option + ): option is { value: 'claude' | 'codex' | 'qwen'; label: string } => option !== null + ); + const defaultChoice = await select({ message: 'Which should be the default AI assistant?', - options: [ - { value: 'claude', label: 'Claude (Recommended)' }, - { value: 'codex', label: 'Codex' }, - { value: 'qwen', label: 'Qwen' }, - ], + options: defaultOptions, });🤖 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 695 - 716, The default-assistant prompt currently always offers 'claude','codex','qwen' even if some weren't configured, which can write an invalid .env; update the selection logic in the setup flow so the options passed to select(...) are built from the actual flags (hasClaude, hasCodex, hasQwen) instead of the hard-coded list. Concretely, construct an options array conditionally (e.g., push {value:'claude',label:...} only if hasClaude is true, etc.), call select with that filtered array, and keep assigning defaultAssistant = defaultChoice as before so only configured assistants can be chosen. Ensure the check that handles isCancel(defaultChoice) remains unchanged.packages/core/src/config/config-loader.ts (1)
496-500:⚠️ Potential issue | 🟠 MajorPersist
assistants.qwenin the write path.The new loader/UI/API flow reads and exposes
assistants.qwen, butupdateGlobalConfig()still deep-merges onlyclaudeandcodex. Any saved Qwen config is dropped when the file is written, so the new settings won't survive a reload.Proposed fix
if (updates.assistants) { merged.assistants = { claude: { ...current.assistants?.claude, ...updates.assistants.claude }, codex: { ...current.assistants?.codex, ...updates.assistants.codex }, + qwen: { ...current.assistants?.qwen, ...updates.assistants.qwen }, }; }Based on learnings:
.archon/config.yamlsupportsassistants.*defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.ts` around lines 496 - 500, The write path currently only merges claude and codex under merged.assistants so any updates.assistants.qwen are dropped; update the merged.assistants block in updateGlobalConfig (the if (updates.assistants) section that sets merged.assistants) to also merge qwen (e.g. qwen: { ...current.assistants?.qwen, ...updates.assistants.qwen }), or replace the block with a generic merge that preserves all assistant keys (e.g. merge each key from updates.assistants into current.assistants) so assistants.qwen is persisted.
🧹 Nitpick comments (2)
packages/cli/src/commands/setup.test.ts (1)
131-139: Add at least one real Qwen path test.These fixture updates keep the existing cases compiling, but they still never exercise the new Qwen branches. Please add a
qwen: truecase, plus a mixed-assistant case that asserts the chosenDEFAULT_AI_ASSISTANT, so the new setup wiring is actually covered.Also applies to: 157-165, 183-190, 223-234, 254-260, 276-282, 298-304, 328-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.test.ts` around lines 131 - 139, Add real Qwen-path tests by updating the fixtures that call generateEnvContent to include at least one case with ai: { qwen: true } and one mixed-assistant case (e.g., ai: { claude: true, qwen: true, defaultAssistant: 'qwen' }) and then assert the resolved DEFAULT_AI_ASSISTANT value in the test assertions; modify the test blocks that use generateEnvContent (and any assertions referencing DEFAULT_AI_ASSISTANT) so they validate the Qwen branch is exercised and that the chosen default assistant matches the expected value.packages/web/src/components/workflows/NodeInspector.tsx (1)
324-328: Avoid manual provider union casts here.Line 324-Line 328 duplicates provider literals. Prefer deriving from
DagNode['provider']/DagNodeData['provider']so future provider additions stay in sync automatically.Suggested refactor
- onUpdate({ - provider: (e.target.value || undefined) as - | 'claude' - | 'codex' - | 'qwen' - | undefined, - }); + const provider = (e.target.value || undefined) as DagNode['provider']; + onUpdate({ provider });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/NodeInspector.tsx` around lines 324 - 328, The code is manually casting provider to a literal union ('claude'|'codex'|'qwen'|undefined); change it to derive the type from your model type (e.g., DagNode['provider'] or DagNodeData['provider']) so new providers stay in sync: replace the manual union cast on provider (where you set provider: (e.target.value || undefined) as ... ) with a cast to the derived type (as DagNode['provider'] or DagNodeData['provider']), adjust imports to bring in DagNode/DagNodeData if not present, and keep the runtime behavior of mapping empty string to undefined.
🤖 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/cli/src/commands/setup.ts`:
- Around line 1439-1443: The code that rebuilds config.ai in add mode overwrites
any existing Qwen/default assistant settings (see the ai object creation where
claude/codex/qwen/defaultAssistant are set); change that construction to
preserve existing?.hasQwen (or equivalent flag) and existing?.defaultAssistant
when present instead of always setting qwen: false and defaultAssistant:
'claude' so the add flow merges new platforms into existing config rather than
dropping prior Qwen/default choices.
In `@packages/core/src/clients/qwen.ts`:
- Around line 278-290: When partial streaming is enabled, track tool-call IDs
emitted by emitPartialMessage to avoid re-emitting the same tool blocks during
the final assistant message; add a Set (e.g., emittedToolIds) in the surrounding
generator scope and have emitPartialMessage add each tool call id it emits, then
update emitAssistantMessageBlocks (invoked from the isSDKAssistantMessage
branch) to accept or reference that Set and skip yielding any
tool_use/tool_result blocks whose tool_call.id is already present in
emittedToolIds (leave shouldEmitText logic unchanged).
In `@packages/server/src/routes/schemas/config.schemas.ts`:
- Around line 53-57: The qwen schema currently only accepts an object with
model: z.string(), so it can't represent an explicit "unset" value; update the
qwen entry in config.schemas.ts to allow an explicit clear shape (e.g., permit
null, an object with model: z.string().optional().refine(...) that allows empty
string, or accept the literal null) so callers can send a remove signal, and
ensure the PATCH merge logic that consumes qwen (the code that merges config
defaults with workflow-level options) treats these new shapes as "unset" and
falls back to SDK defaults; target the qwen schema definition and the PATCH
merge handler that reads qwen to keep behavior consistent.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 504-510: The Qwen branch builds options but never forwards merged
env vars, so QwenClient only sees loadConfig(cwd).envVars and misses DB-backed
envs; update the branch that sets options for provider === 'qwen' (the object
with model, systemPrompt, allowedTools, disallowedTools) to include env (or
envVars) merged from config.envVars (the executor's merged env) — e.g., set
options.env = { ...(options.env || {}), ...config.envVars } or otherwise pass
config.envVars into the options passed to QwenClient so Qwen runs receive the
same env as Claude runs.
In `@packages/workflows/src/model-validation.ts`:
- Around line 21-25: The function inferProviderFromModel currently maps unknown
model strings to 'codex', which can silently misroute typos; update
inferProviderFromModel to return undefined for any model that is not recognized
by isClaudeModel or isQwenModel so callers retain the configured provider and
can fail-fast; modify the return behavior in inferProviderFromModel (replace the
default return 'codex' with return undefined) and add a brief comment explaining
intentional no-fallback behavior.
---
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 695-716: The default-assistant prompt currently always offers
'claude','codex','qwen' even if some weren't configured, which can write an
invalid .env; update the selection logic in the setup flow so the options passed
to select(...) are built from the actual flags (hasClaude, hasCodex, hasQwen)
instead of the hard-coded list. Concretely, construct an options array
conditionally (e.g., push {value:'claude',label:...} only if hasClaude is true,
etc.), call select with that filtered array, and keep assigning defaultAssistant
= defaultChoice as before so only configured assistants can be chosen. Ensure
the check that handles isCancel(defaultChoice) remains unchanged.
In `@packages/core/src/config/config-loader.ts`:
- Around line 496-500: The write path currently only merges claude and codex
under merged.assistants so any updates.assistants.qwen are dropped; update the
merged.assistants block in updateGlobalConfig (the if (updates.assistants)
section that sets merged.assistants) to also merge qwen (e.g. qwen: {
...current.assistants?.qwen, ...updates.assistants.qwen }), or replace the block
with a generic merge that preserves all assistant keys (e.g. merge each key from
updates.assistants into current.assistants) so assistants.qwen is persisted.
---
Nitpick comments:
In `@packages/cli/src/commands/setup.test.ts`:
- Around line 131-139: Add real Qwen-path tests by updating the fixtures that
call generateEnvContent to include at least one case with ai: { qwen: true } and
one mixed-assistant case (e.g., ai: { claude: true, qwen: true,
defaultAssistant: 'qwen' }) and then assert the resolved DEFAULT_AI_ASSISTANT
value in the test assertions; modify the test blocks that use generateEnvContent
(and any assertions referencing DEFAULT_AI_ASSISTANT) so they validate the Qwen
branch is exercised and that the chosen default assistant matches the expected
value.
In `@packages/web/src/components/workflows/NodeInspector.tsx`:
- Around line 324-328: The code is manually casting provider to a literal union
('claude'|'codex'|'qwen'|undefined); change it to derive the type from your
model type (e.g., DagNode['provider'] or DagNodeData['provider']) so new
providers stay in sync: replace the manual union cast on provider (where you set
provider: (e.target.value || undefined) as ... ) with a cast to the derived type
(as DagNode['provider'] or DagNodeData['provider']), adjust imports to bring in
DagNode/DagNodeData if not present, and keep the runtime behavior of mapping
empty string to undefined.
🪄 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: 5fc32d35-65aa-442d-b828-cfa05bc3dbcb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
CLAUDE.mdREADME.mdpackages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/core/package.jsonpackages/core/src/clients/factory.test.tspackages/core/src/clients/factory.tspackages/core/src/clients/index.tspackages/core/src/clients/qwen.test.tspackages/core/src/clients/qwen.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/conversations.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/services/title-generator.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-preamble.test.tspackages/workflows/src/executor.test.tspackages/workflows/src/executor.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/script-node-deps.test.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
Integrate Qwen as a first-class assistant across config, workflows, CLI setup, API schemas, and the web UI. Add a Qwen smoke test and tighten the docs so the remaining provider-specific limits are explicit. This keeps Archon aligned with the new Qwen path while documenting where behavior still differs from Claude and Codex. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Preserve existing Qwen settings when assistant config is updated so the web settings flow no longer drops qwen entries from global config. Stop forcing OpenAI auth in the Qwen client and allow the SDK to follow its native auth resolution unless Archon is configured explicitly. Also pass through OpenAI-compatible env vars and wire the Qwen smoke test into the package test script so the reviewed behavior stays covered. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Prevent duplicate tool-call emission when partial Qwen streaming emits a tool start before the final assistant message, and pass native Qwen auth environment variables through the subprocess allowlist. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
LoneWolf36
force-pushed
the
feat/qwen-support-boundaries
branch
from
d53f327 to
4e67ed1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/cli/src/commands/setup.ts (1)
1457-1464:⚠️ Potential issue | 🟠 MajorPreserve the existing Qwen/default-assistant values in add mode.
This seed object still hard-resets
qwentofalseanddefaultAssistantto'claude'before rewriting the full.env, so choosing Add platforms can silently strip an existing Qwen setup or default-assistant preference.🤖 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 1457 - 1464, The seed config object creation in setup.ts currently forces qwen: false and defaultAssistant: 'claude', which overwrites existing values when running in "Add platforms" mode; update the config initialization to preserve existing?.hasQwen (or existing?.qwen) and existing?.defaultAssistant when present (fall back to the current defaults only if those existing values are undefined), referencing the same config object construction where ai: { claude, codex, qwen, defaultAssistant } is set so the .env rewrite won't silently strip an existing Qwen setup or user default assistant preference.
🤖 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/qwen.test.ts`:
- Around line 67-74: Replace the process-global mock.module calls for internal
modules with spyOn to avoid test pollution: instead of
mock.module('../config/config-loader', ...) and mock.module('../db/codebases',
...), import spyOn from 'bun:test' and spyOn the specific exports loadConfig,
findCodebaseByDefaultCwd and findCodebaseByPathPrefix on their modules,
providing the same mock implementations; ensure you restore or clear those spies
after each test (or use sandbox/afterEach restore) and keep mock.module only for
external packages like '@qwen-code/sdk' that cannot be spied on.
In `@packages/core/src/clients/qwen.ts`:
- Around line 283-303: The current logic uses sawPartialAssistantMessage (set
for any partial event) to decide whether to suppress final assistant text in
emitAssistantMessageBlocks, which incorrectly hides final text when only a
tool_use content_block_start was streamed; modify the code to track a separate
flag (e.g., sawPartialTextOrThinking) that is set only when a partial assistant
message represents text/thinking deltas (not when event.content_block.type ===
'tool_use'), leave partialToolCallIds handling for tool starts as-is, use
sawPartialTextOrThinking in the call to emitAssistantMessageBlocks instead of
sawPartialAssistantMessage, and ensure emitPartialMessage and
isSDKPartialAssistantMessage behavior still yield chunks unchanged so only true
text/thinking partials suppress final text.
In `@packages/core/src/utils/env-allowlist.ts`:
- Around line 37-43: Remove provider-specific credentials from the shared
SUBPROCESS_ENV_ALLOWLIST (e.g., 'OPENAI_API_KEY', 'OPENAI_BASE_URL',
'OPENAI_MODEL', 'DASHSCOPE_API_KEY', 'BAILIAN_CODING_PLAN_API_KEY') so the
common allowlist stays minimal; instead add those env names only to the
provider-specific environment builders (e.g., the Qwen/Codex env builder
functions such as buildQwenEnv / buildCodexEnv or whatever factory constructs
subprocess env for those clients). Also add explicit validation in those
provider-specific builders that throws a clear error if required auth vars are
missing, rather than relying on the global allowlist to propagate secrets.
Ensure SUBPROCESS_ENV_ALLOWLIST contains only generic, non-provider secrets
after this change.
---
Duplicate comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 1457-1464: The seed config object creation in setup.ts currently
forces qwen: false and defaultAssistant: 'claude', which overwrites existing
values when running in "Add platforms" mode; update the config initialization to
preserve existing?.hasQwen (or existing?.qwen) and existing?.defaultAssistant
when present (fall back to the current defaults only if those existing values
are undefined), referencing the same config object construction where ai: {
claude, codex, qwen, defaultAssistant } is set so the .env rewrite won't
silently strip an existing Qwen setup or user default assistant preference.
🪄 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: 7250950d-8ace-43d7-a72d-3d4b4fb6f80f
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdpackages/cli/src/commands/setup.tspackages/core/package.jsonpackages/core/src/clients/qwen.test.tspackages/core/src/clients/qwen.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/utils/env-allowlist.test.tspackages/core/src/utils/env-allowlist.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/package.json
- packages/core/src/config/config-loader.test.ts
- README.md
Comment on lines
+67
to
+74
| mock.module('../config/config-loader', () => ({ | ||
| loadConfig: mockLoadConfig, | ||
| })); | ||
|
|
||
| mock.module('../db/codebases', () => ({ | ||
| findCodebaseByDefaultCwd: mock(async () => null), | ||
| findCodebaseByPathPrefix: mock(async () => null), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
fd -t f 'qwen.test.ts' packages/core/src/clients/Repository: coleam00/Archon
Length of output: 97
🏁 Script executed:
# If file exists, read lines around 67-74
if [ -f packages/core/src/clients/qwen.test.ts ]; then
wc -l packages/core/src/clients/qwen.test.ts
echo "---"
sed -n '55,85p' packages/core/src/clients/qwen.test.ts
fiRepository: coleam00/Archon
Length of output: 865
🏁 Script executed:
# Check the full test file structure to understand the mock setup
if [ -f packages/core/src/clients/qwen.test.ts ]; then
head -100 packages/core/src/clients/qwen.test.ts
fiRepository: coleam00/Archon
Length of output: 2650
🏁 Script executed:
# Check the imports to see if spyOn is available
sed -n '1,10p' packages/core/src/clients/qwen.test.tsRepository: coleam00/Archon
Length of output: 406
Use spyOn() for these internal module mocks to prevent test pollution.
mock.module('../config/config-loader', ...) and mock.module('../db/codebases', ...) are process-global in Bun and cannot be reversed. When this test runs alongside other tests in the same invocation, these mocks will leak into subsequent tests. Switch to spyOn() for internal modules (you'll need to import spyOn from bun:test), and reserve mock.module() only for external packages like @qwen-code/sdk that can't be safely spied on.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/clients/qwen.test.ts` around lines 67 - 74, Replace the
process-global mock.module calls for internal modules with spyOn to avoid test
pollution: instead of mock.module('../config/config-loader', ...) and
mock.module('../db/codebases', ...), import spyOn from 'bun:test' and spyOn the
specific exports loadConfig, findCodebaseByDefaultCwd and
findCodebaseByPathPrefix on their modules, providing the same mock
implementations; ensure you restore or clear those spies after each test (or use
sandbox/afterEach restore) and keep mock.module only for external packages like
'@qwen-code/sdk' that cannot be spied on.
Comment on lines
+37
to
+43
| // OpenAI-compatible auth/config used by Codex and Qwen | ||
| 'OPENAI_API_KEY', | ||
| 'OPENAI_BASE_URL', | ||
| 'OPENAI_MODEL', | ||
| // Native Qwen / DashScope auth | ||
| 'DASHSCOPE_API_KEY', | ||
| 'BAILIAN_CODING_PLAN_API_KEY', |
There was a problem hiding this comment.
Keep provider credentials out of the shared subprocess allowlist.
SUBPROCESS_ENV_ALLOWLIST is used for assistant subprocesses broadly, so adding Qwen/OpenAI keys here leaks those credentials into unrelated clients too. Please keep the common allowlist minimal and inject provider-specific auth vars only in the Qwen/Codex env builders that actually need them.
As per coding guidelines, "Prefer throwing early with a clear error for unsupported or unsafe states; never silently swallow errors or silently broaden permissions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/env-allowlist.ts` around lines 37 - 43, Remove
provider-specific credentials from the shared SUBPROCESS_ENV_ALLOWLIST (e.g.,
'OPENAI_API_KEY', 'OPENAI_BASE_URL', 'OPENAI_MODEL', 'DASHSCOPE_API_KEY',
'BAILIAN_CODING_PLAN_API_KEY') so the common allowlist stays minimal; instead
add those env names only to the provider-specific environment builders (e.g.,
the Qwen/Codex env builder functions such as buildQwenEnv / buildCodexEnv or
whatever factory constructs subprocess env for those clients). Also add explicit
validation in those provider-specific builders that throws a clear error if
required auth vars are missing, rather than relying on the global allowlist to
propagate secrets. Ensure SUBPROCESS_ENV_ALLOWLIST contains only generic,
non-provider secrets after this change.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/workflows/src/dag-executor.ts (2)
1709-1729:⚠️ Potential issue | 🟠 MajorQwen loop nodes drop DB-backed env vars too.
After widening this helper to
AssistantProvider, loop nodes can now run throughQwenClient, butbuildLoopNodeOptions()still never forwardsconfig.envVars. That means Qwen loop iterations won't see codebase env vars configured in the DB.Suggested fix
function buildLoopNodeOptions( provider: AssistantProvider, model: string | undefined, config: WorkflowConfig ): WorkflowAssistantOptions | undefined { + const qwenOptions = + provider === 'qwen' && config.envVars && Object.keys(config.envVars).length > 0 + ? { env: config.envVars } + : undefined; + const codexOptions = provider === 'codex' ? { modelReasoningEffort: config.assistants.codex.modelReasoningEffort, webSearchMode: config.assistants.codex.webSearchMode, @@ - if (!model && !codexOptions && !claudeOptions) return undefined; - return { ...(model ? { model } : {}), ...codexOptions, ...claudeOptions }; + if (!model && !codexOptions && !claudeOptions && !qwenOptions) return undefined; + return { ...(model ? { model } : {}), ...codexOptions, ...claudeOptions, ...qwenOptions }; }🤖 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 1709 - 1729, buildLoopNodeOptions currently omits forwarding DB-backed environment variables, so Qwen-backed loop iterations won't receive config.envVars; update buildLoopNodeOptions (the function itself) to include envVars from WorkflowConfig when present in the returned WorkflowAssistantOptions (alongside the existing model, codexOptions, and claudeOptions) so QwenClient and any other provider can access config.envVars during loop execution.
463-481:⚠️ Potential issue | 🟡 MinorDon't warn that
systemPromptis ignored for Qwen.The Qwen branch below forwards
systemPrompt, so this warning is false for Qwen nodes and will mislead users. KeepsystemPromptin the Codex-only unsupported set, or split the ignored-field lists per provider.Based on learnings, "Qwen does not support:
settingSources,effort,thinking,maxBudgetUsd,fallbackModel,betas,sandbox,hooks."🤖 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 463 - 481, The warning wrongly includes systemPrompt for Qwen nodes; update the logic in the provider === 'codex' || provider === 'qwen' block (symbols: provider, providerLabel, claudeOnlyFields, present, getLog, safeSendMessage, node.id) to treat Codex and Qwen separately: keep systemPrompt only in the Codex-only ignored list and ensure Qwen's ignored fields match the learned set (settingSources, effort, thinking, maxBudgetUsd, fallbackModel, betas, sandbox, hooks); alternatively split into two arrays (claudeOnlyFieldsForCodex and claudeOnlyFieldsForQwen) and compute present from the appropriate array so Qwen nodes are not warned about systemPrompt.
♻️ Duplicate comments (4)
packages/workflows/src/model-validation.ts (1)
21-25:⚠️ Potential issue | 🟠 MajorDon't treat every unknown model string as Codex.
This still makes typos and unsupported aliases silently route to the Codex path instead of failing during validation. Return
undefinedfor unrecognized models here, then let callers require an explicit provider or a real Codex-model detector.Suggested fix
export function inferProviderFromModel(model?: string): AssistantProvider | undefined { if (!model) return undefined; if (isClaudeModel(model)) return 'claude'; if (isQwenModel(model)) return 'qwen'; - return 'codex'; + return undefined; }Based on learnings, "Fail fast + explicit errors: Prefer throwing early with clear errors for unsupported or unsafe states — never silently swallow errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/model-validation.ts` around lines 21 - 25, The function inferProviderFromModel currently maps any unknown model string to 'codex'; change it to return undefined for unrecognized models so typos/unsupported aliases don't silently route to Codex: in inferProviderFromModel, keep the existing checks for isClaudeModel(model) and isQwenModel(model) but replace the final "return 'codex';" with "return undefined;" so callers must explicitly handle or validate Codex models (or use a dedicated codex detector) rather than assuming unknown strings are Codex.packages/workflows/src/dag-executor.ts (1)
504-510:⚠️ Potential issue | 🟠 MajorForward merged env vars to Qwen DAG nodes.
config.envVarsalready contains the file config plus DB-backed codebase env merged inexecuteWorkflow(), but this branch never passes it toQwenClient. SinceQwenClientonly combinesloadConfig(cwd).envVarswithoptions.env, Qwen nodes will miss values from/api/codebases/{id}/env.Suggested fix
} else if (provider === 'qwen') { options = { model, systemPrompt: node.systemPrompt, allowedTools: node.allowed_tools, disallowedTools: node.denied_tools, + ...(config.envVars && Object.keys(config.envVars).length > 0 + ? { env: config.envVars } + : {}), };🤖 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 504 - 510, The Qwen branch in dag-executor builds options without forwarding the merged environment variables from executeWorkflow(), so QwenClient only sees loadConfig(cwd).envVars and misses DB-backed envs; update the 'qwen' provider options (the options object created alongside model, node.systemPrompt, node.allowed_tools, node.denied_tools) to include env: config.envVars (or merge config.envVars with any existing options.env) so QwenClient receives the full merged environment; ensure this change is applied where provider === 'qwen' in executeWorkflow() so Qwen nodes get /api/codebases/{id}/env values.packages/core/src/clients/qwen.ts (1)
268-302:⚠️ Potential issue | 🟠 MajorDon't suppress final assistant text after a tool-start partial.
Any partial event flips
sawPartialAssistantMessage, so a lonecontent_block_startfortool_usemakes the final assistant message skip all text/thinking blocks. That loses output in mixed tool+text turns.Suggested fix
const shouldStreamPartials = includePartialMessages; - let sawPartialAssistantMessage = false; + let sawPartialTextOrThinking = false; const partialToolCallIds = new Set<string>(); @@ if (isSDKPartialAssistantMessage(message)) { - sawPartialAssistantMessage = true; if ( message.event.type === 'content_block_start' && message.event.content_block.type === 'tool_use' && message.event.content_block.id ) { partialToolCallIds.add(message.event.content_block.id); } + if ( + message.event.type === 'content_block_delta' && + (message.event.delta.type === 'text_delta' || + message.event.delta.type === 'thinking_delta') + ) { + sawPartialTextOrThinking = true; + } for (const chunk of emitPartialMessage(message)) { yield chunk; } @@ if (isSDKAssistantMessage(message)) { for (const chunk of emitAssistantMessageBlocks( message, - !shouldStreamPartials || !sawPartialAssistantMessage, + !shouldStreamPartials || !sawPartialTextOrThinking, partialToolCallIds )) { yield chunk; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/clients/qwen.ts` around lines 268 - 302, The code sets sawPartialAssistantMessage true for any isSDKPartialAssistantMessage (including tool_use starts) which causes the final assistant message to skip text/thinking blocks; change the logic in the isSDKPartialAssistantMessage handling so sawPartialAssistantMessage is only set when the partial actually represents assistant text content (e.g., message.event.content_block.type !== 'tool_use' or the partial includes text tokens) rather than on every partial; update the branch around isSDKPartialAssistantMessage (and the condition that collects partialToolCallIds) so tool_use-only partials do not flip sawPartialAssistantMessage while still adding tool IDs to partialToolCallIds and emitting the partial chunks.packages/core/src/clients/qwen.test.ts (1)
67-74:⚠️ Potential issue | 🟠 MajorReplace these internal
mock.module()calls with spies.
mock.module('../config/config-loader', ...)andmock.module('../db/codebases', ...)are still process-global in Bun, so this suite can leak those mocks into other tests in the same invocation. Please switch these internal dependencies tospyOn()and restore the spies in cleanup; keepmock.module()only for@qwen-code/sdk.As per coding guidelines, "
**/*.test.ts: UsespyOn()for internal module spies when other test files import the same module ... Usemock.module()only when necessary and ensure separatebun testinvocations to avoid cross-file mock pollution."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/clients/qwen.test.ts` around lines 67 - 74, Tests currently use process-global mock.module() for internal modules ('../config/config-loader' and '../db/codebases') which can leak across Bun test files; replace those with spies using spyOn() targeting the exported functions (e.g., mockLoadConfig, findCodebaseByDefaultCwd, findCodebaseByPathPrefix) and set their return values as needed, keep mock.module() only for the external '@qwen-code/sdk' import, and ensure each spy is restored in your test cleanup (afterEach/teardown) so the original modules are re-established between tests.
🧹 Nitpick comments (1)
packages/workflows/src/validator.ts (1)
340-348: Missing validation warnings for Qwen-incompatible features.The MCP, skills, hooks, and tool-restriction warnings only check for
provider === 'codex', but Qwen also doesn't support these Claude-only features. Per the documented Qwen support boundaries, Qwen does not support:settingSources,effort,thinking,maxBudgetUsd,fallbackModel,betas,sandbox,hooks.Consider extending these warnings to include Qwen:
♻️ Suggested approach for MCP warning (line 340)
- if (provider === 'codex') { + if (provider === 'codex' || provider === 'qwen') { issues.push({ level: 'warning', nodeId: node.id, field: 'mcp', - message: 'MCP servers are Claude-only per-node — this will be ignored on Codex', - hint: 'For Codex, configure MCP servers globally in ~/.codex/config.toml instead', + message: `MCP servers are Claude-only per-node — this will be ignored on ${provider === 'codex' ? 'Codex' : 'Qwen'}`, + hint: provider === 'codex' + ? 'For Codex, configure MCP servers globally in ~/.codex/config.toml instead' + : 'Qwen does not support per-node MCP configuration', }); }Apply similar changes to the skills (line 372), hooks (line 384), and tool restrictions (line 395) warnings. Based on learnings: "Qwen does not support: settingSources, effort, thinking, maxBudgetUsd, fallbackModel, betas, sandbox, hooks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/validator.ts` around lines 340 - 348, The validation currently only flags Claude-only features when provider === 'codex'; update the checks to also include Qwen (e.g., change conditions from provider === 'codex' to provider === 'codex' || provider === 'qwen') for the MCP warning (the block that pushes an issue with field 'mcp'), and make the same provider check change in the skills, hooks, and tool-restriction warning blocks so they trigger for Qwen as well; additionally add validation warnings for Qwen for the other unsupported fields (settingSources, effort, thinking, maxBudgetUsd, fallbackModel, betas, sandbox, hooks) by pushing issues (using the same issues.push pattern with node.id and appropriate field names/messages) wherever the validator currently checks or reads those fields.
🤖 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/config/config-types.ts`:
- Around line 34-35: Update the doc comment for the authType property to remove
the incorrect "defaults to `openai`" claim and instead state that authType is
optional and resolved by the SDK/QwenClient when not explicitly provided;
mention that Archon does not provision Qwen credentials. Target the authType
property declaration and ensure the wording clarifies that QwenClient only
forwards authType if explicitly configured.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Line 463: The state initialization reads config.assistants.qwen.model directly
which can be undefined at runtime; change it to safely handle missing qwen
(e.g., use optional chaining/defaults) so useState initializes with
config.assistants?.qwen?.model ?? '' and update the related useEffect in
SettingsPage (the effect that reads or writes qwen fields) to guard against
config.assistants?.qwen being undefined before accessing or setting properties;
ensure any setters (setQwenModel) and reads check existence or use defaults so
the UI won’t crash when qwen is omitted by the server.
In `@packages/workflows/src/loader.ts`:
- Around line 287-295: Normalize the input in isClaudeModel() the same way as
isQwenModel() by applying .trim().toLowerCase() to the model name before
performing checks; update isClaudeModel() to use the normalized string when
comparing against Claude patterns so whitespace/case like " claude-3 " is
recognized and provider inference no longer silently misclassifies to codex.
---
Outside diff comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1709-1729: buildLoopNodeOptions currently omits forwarding
DB-backed environment variables, so Qwen-backed loop iterations won't receive
config.envVars; update buildLoopNodeOptions (the function itself) to include
envVars from WorkflowConfig when present in the returned
WorkflowAssistantOptions (alongside the existing model, codexOptions, and
claudeOptions) so QwenClient and any other provider can access config.envVars
during loop execution.
- Around line 463-481: The warning wrongly includes systemPrompt for Qwen nodes;
update the logic in the provider === 'codex' || provider === 'qwen' block
(symbols: provider, providerLabel, claudeOnlyFields, present, getLog,
safeSendMessage, node.id) to treat Codex and Qwen separately: keep systemPrompt
only in the Codex-only ignored list and ensure Qwen's ignored fields match the
learned set (settingSources, effort, thinking, maxBudgetUsd, fallbackModel,
betas, sandbox, hooks); alternatively split into two arrays
(claudeOnlyFieldsForCodex and claudeOnlyFieldsForQwen) and compute present from
the appropriate array so Qwen nodes are not warned about systemPrompt.
---
Duplicate comments:
In `@packages/core/src/clients/qwen.test.ts`:
- Around line 67-74: Tests currently use process-global mock.module() for
internal modules ('../config/config-loader' and '../db/codebases') which can
leak across Bun test files; replace those with spies using spyOn() targeting the
exported functions (e.g., mockLoadConfig, findCodebaseByDefaultCwd,
findCodebaseByPathPrefix) and set their return values as needed, keep
mock.module() only for the external '@qwen-code/sdk' import, and ensure each spy
is restored in your test cleanup (afterEach/teardown) so the original modules
are re-established between tests.
In `@packages/core/src/clients/qwen.ts`:
- Around line 268-302: The code sets sawPartialAssistantMessage true for any
isSDKPartialAssistantMessage (including tool_use starts) which causes the final
assistant message to skip text/thinking blocks; change the logic in the
isSDKPartialAssistantMessage handling so sawPartialAssistantMessage is only set
when the partial actually represents assistant text content (e.g.,
message.event.content_block.type !== 'tool_use' or the partial includes text
tokens) rather than on every partial; update the branch around
isSDKPartialAssistantMessage (and the condition that collects
partialToolCallIds) so tool_use-only partials do not flip
sawPartialAssistantMessage while still adding tool IDs to partialToolCallIds and
emitting the partial chunks.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 504-510: The Qwen branch in dag-executor builds options without
forwarding the merged environment variables from executeWorkflow(), so
QwenClient only sees loadConfig(cwd).envVars and misses DB-backed envs; update
the 'qwen' provider options (the options object created alongside model,
node.systemPrompt, node.allowed_tools, node.denied_tools) to include env:
config.envVars (or merge config.envVars with any existing options.env) so
QwenClient receives the full merged environment; ensure this change is applied
where provider === 'qwen' in executeWorkflow() so Qwen nodes get
/api/codebases/{id}/env values.
In `@packages/workflows/src/model-validation.ts`:
- Around line 21-25: The function inferProviderFromModel currently maps any
unknown model string to 'codex'; change it to return undefined for unrecognized
models so typos/unsupported aliases don't silently route to Codex: in
inferProviderFromModel, keep the existing checks for isClaudeModel(model) and
isQwenModel(model) but replace the final "return 'codex';" with "return
undefined;" so callers must explicitly handle or validate Codex models (or use a
dedicated codex detector) rather than assuming unknown strings are Codex.
---
Nitpick comments:
In `@packages/workflows/src/validator.ts`:
- Around line 340-348: The validation currently only flags Claude-only features
when provider === 'codex'; update the checks to also include Qwen (e.g., change
conditions from provider === 'codex' to provider === 'codex' || provider ===
'qwen') for the MCP warning (the block that pushes an issue with field 'mcp'),
and make the same provider check change in the skills, hooks, and
tool-restriction warning blocks so they trigger for Qwen as well; additionally
add validation warnings for Qwen for the other unsupported fields
(settingSources, effort, thinking, maxBudgetUsd, fallbackModel, betas, sandbox,
hooks) by pushing issues (using the same issues.push pattern with node.id and
appropriate field names/messages) wherever the validator currently checks or
reads those fields.
🪄 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: 6ab6e2a2-494d-487e-937a-83ed348f2281
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
CLAUDE.mdREADME.mdpackages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/core/package.jsonpackages/core/src/clients/factory.test.tspackages/core/src/clients/factory.tspackages/core/src/clients/index.tspackages/core/src/clients/qwen.test.tspackages/core/src/clients/qwen.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/conversations.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/services/title-generator.tspackages/core/src/utils/env-allowlist.test.tspackages/core/src/utils/env-allowlist.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-preamble.test.tspackages/workflows/src/executor.test.tspackages/workflows/src/executor.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/script-node-deps.test.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
✅ Files skipped from review due to trivial changes (9)
- packages/workflows/src/executor-preamble.test.ts
- packages/web/src/components/workflows/NodeInspector.tsx
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/workflows/src/validator.test.ts
- packages/core/package.json
- packages/cli/src/commands/setup.test.ts
- packages/core/src/clients/index.ts
- packages/core/src/services/title-generator.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/core/src/index.ts
- packages/workflows/src/script-node-deps.test.ts
- packages/workflows/src/schemas/workflow.ts
- packages/workflows/src/executor.test.ts
- packages/core/src/clients/factory.test.ts
- packages/core/src/config/config-loader.test.ts
- packages/core/src/handlers/clone.test.ts
- packages/server/src/routes/api.ts
- packages/workflows/src/schemas/dag-node.ts
- packages/core/src/db/conversations.ts
- packages/core/src/handlers/clone.ts
- packages/core/src/clients/factory.ts
- packages/core/src/utils/env-allowlist.test.ts
- packages/workflows/src/dag-executor.test.ts
- packages/core/src/utils/env-allowlist.ts
- CLAUDE.md
- packages/cli/src/commands/setup.ts
Comment on lines
+287
to
295
| if (inferredProvider && model && !isModelCompatible(inferredProvider, model)) { | ||
| return { | ||
| workflow: null, | ||
| error: { | ||
| filename, | ||
| error: `Model "${model}" is not compatible with provider "${provider}"`, | ||
| error: `Model "${model}" is not compatible with provider "${inferredProvider}"`, | ||
| errorType: 'validation_error', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the asymmetric normalization in model-validation.ts
cat packages/workflows/src/model-validation.ts | head -40Repository: coleam00/Archon
Length of output: 1236
Normalize model input consistently in isClaudeModel() and isQwenModel() to prevent silent provider inference failures.
The asymmetric normalization in these functions creates a bug: isQwenModel() normalizes its input with .trim().toLowerCase(), but isClaudeModel() does not. This causes inputs like " claude-3 " (with whitespace) to fail the Claude check but be silently accepted as a 'codex' model instead of returning an error or inferring the intended provider.
To fix: Apply .trim().toLowerCase() in isClaudeModel() before checking the model name, ensuring consistent behavior across both functions and preventing silent provider mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/loader.ts` around lines 287 - 295, Normalize the input
in isClaudeModel() the same way as isQwenModel() by applying
.trim().toLowerCase() to the model name before performing checks; update
isClaudeModel() to use the normalized string when comparing against Claude
patterns so whitespace/case like " claude-3 " is recognized and provider
inference no longer silently misclassifies to codex.
- Config persistence: unconditionally preserve qwen in updateGlobalConfig() - CLI setup: filter default assistant options by actually configured assistants - Add mode: read hasQwen and defaultAssistant from existing config instead of hardcoding - Missing env vars: inject config.envVars into Qwen node options in dag-executor - Provider misrouting: return undefined instead of 'codex' for unknown models - Streaming dedup: track sawPartialText separately from tool_use partials - Security: scope Qwen credentials to QwenClient only, remove from shared allowlist - UI robustness: add optional chaining for config.assistants.qwen?.model in SettingsPage Co-Authored-By: Codex GPT-5 <noreply@openai.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
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/dag-executor.ts (1)
464-491:⚠️ Potential issue | 🟠 MajorRemove
systemPromptfrom the Claude-only warning list.
systemPromptis incorrectly included inclaudeOnlyFields(line 470), but it's actually used for Qwen at line 507. This causes a false warning telling users theirsystemPrompt"will be ignored" when it's actually being forwarded to Qwen.Per coding guidelines, the Claude-only options are:
settingSources,effort,thinking,maxBudgetUsd,fallbackModel,betas,sandbox,hooks,skills—systemPromptis not in that list.🐛 Proposed fix
const claudeOnlyFields = [ ['effort', node.effort ?? workflowLevelOptions.effort], ['thinking', node.thinking ?? workflowLevelOptions.thinking], ['maxBudgetUsd', node.maxBudgetUsd], - ['systemPrompt', node.systemPrompt], ['fallbackModel', node.fallbackModel ?? workflowLevelOptions.fallbackModel], ['betas', node.betas ?? workflowLevelOptions.betas], ['sandbox', node.sandbox ?? workflowLevelOptions.sandbox], ] as const;As per coding guidelines, "Claude-only options (
settingSources,effort,thinking,maxBudgetUsd,fallbackModel,betas,sandbox,hooks,skills) do not apply to Qwen" — note thatsystemPromptis not in this list.🤖 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 464 - 491, The warning incorrectly treats systemPrompt as a Claude-only option; update the claudeOnlyFields definition in the provider check (the array currently declared in the if block where provider === 'codex' || provider === 'qwen') to remove 'systemPrompt' and instead include the actual Claude-only keys per guidelines (e.g., include 'settingSources', 'hooks', 'skills' as applicable) so that present = claudeOnlyFields.filter(...) only reports true Claude-only fields; keep the existing logging (getLog().warn) and delivery via safeSendMessage/node.id/workflowRunId unchanged.
🧹 Nitpick comments (3)
packages/workflows/src/dag-executor.ts (1)
504-542: Consider warning whenoutput_formatis set on Qwen nodes.The Qwen options construction correctly omits
output_formatper the documented limitation, but users won't know theiroutput_formatis being silently ignored. A warning similar to the hooks/skills warnings would improve UX.💡 Suggested enhancement (optional)
Add after line 510, before the MCP block:
// Warn if Qwen node has output_format (not currently forwarded) if (node.output_format) { getLog().warn({ nodeId: node.id }, 'dag.output_format_ignored_qwen'); const delivered = await safeSendMessage( platform, conversationId, `Warning: Node '${node.id}' has output_format set but uses Qwen — structured output is not currently supported for Qwen and will be ignored.`, { workflowId: workflowRunId, nodeName: node.id } ); if (!delivered) { getLog().error({ nodeId: node.id, workflowRunId }, 'dag.output_format_warning_delivery_failed'); } }As per coding guidelines, "Structured
output_formatis not currently forwarded to Qwen workflow nodes."🤖 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 504 - 542, The Qwen branch (provider === 'qwen') silently drops node.output_format; add a warning and user notification when node.output_format is present so users know it's ignored. Inside the Qwen options construction (before the MCP block and after options is set) check node.output_format and call getLog().warn with a clear key (e.g., 'dag.output_format_ignored_qwen') and attempt to notify via safeSendMessage(platform, conversationId, ...) including workflowRunId and node.id; if safeSendMessage returns false, emit an error log (e.g., 'dag.output_format_warning_delivery_failed') to mirror existing MCP warning patterns.packages/core/src/config/config-loader.ts (2)
278-315: Minor inconsistency: conditional spread for qwen vs unconditional for claude/codex.The conditional
...(defaults.assistants.qwen ? { qwen: ... } : {})is functionally safe sincegetDefaults()always initializesqwen: {}, but differs stylistically from the unconditional spread used for claude and codex. Consider aligning the patterns for consistency.♻️ Optional: align with claude/codex pattern
const result: MergedConfig = { ...defaults, assistants: { claude: { ...defaults.assistants.claude }, codex: { ...defaults.assistants.codex }, - ...(defaults.assistants.qwen ? { qwen: { ...defaults.assistants.qwen } } : {}), + qwen: { ...defaults.assistants.qwen }, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.ts` around lines 278 - 315, In mergeGlobalConfig, make the qwen assistant spread consistent with claude/codex by unconditionally including qwen in the initial result (i.e., replace the conditional spread ...(defaults.assistants.qwen ? { qwen: { ...defaults.assistants.qwen } } : {}) with the same unconditional pattern used for claude and codex), and keep the later merge logic that applies global.assistants.qwen so result.assistants.qwen is always present; this touches the mergeGlobalConfig function and the defaults.assistants.qwen usage.
347-379: Same conditional spread inconsistency as mergeGlobalConfig.See earlier comment regarding aligning with the unconditional spread pattern used for claude/codex.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.ts` around lines 347 - 379, mergeRepoConfig currently conditionally creates result.assistants.qwen only when merged.assistants.qwen exists and later conditionally merges repo.assistants.qwen, causing inconsistency with the unconditional spreads used for claude/codex; fix by always initializing result.assistants.qwen in the result object (like claude/codex) as { ...merged.assistants.qwen } and then, instead of the existing if (repo.assistants?.qwen) block, perform a merge that spreads existing result.assistants.qwen and then spreads repo.assistants.qwen so repo-level fields override defaults (and mirror how claude/codex merges model/settingSources), ensuring consistent unconditional spread behavior for qwen inside mergeRepoConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 464-491: The warning incorrectly treats systemPrompt as a
Claude-only option; update the claudeOnlyFields definition in the provider check
(the array currently declared in the if block where provider === 'codex' ||
provider === 'qwen') to remove 'systemPrompt' and instead include the actual
Claude-only keys per guidelines (e.g., include 'settingSources', 'hooks',
'skills' as applicable) so that present = claudeOnlyFields.filter(...) only
reports true Claude-only fields; keep the existing logging (getLog().warn) and
delivery via safeSendMessage/node.id/workflowRunId unchanged.
---
Nitpick comments:
In `@packages/core/src/config/config-loader.ts`:
- Around line 278-315: In mergeGlobalConfig, make the qwen assistant spread
consistent with claude/codex by unconditionally including qwen in the initial
result (i.e., replace the conditional spread ...(defaults.assistants.qwen ? {
qwen: { ...defaults.assistants.qwen } } : {}) with the same unconditional
pattern used for claude and codex), and keep the later merge logic that applies
global.assistants.qwen so result.assistants.qwen is always present; this touches
the mergeGlobalConfig function and the defaults.assistants.qwen usage.
- Around line 347-379: mergeRepoConfig currently conditionally creates
result.assistants.qwen only when merged.assistants.qwen exists and later
conditionally merges repo.assistants.qwen, causing inconsistency with the
unconditional spreads used for claude/codex; fix by always initializing
result.assistants.qwen in the result object (like claude/codex) as {
...merged.assistants.qwen } and then, instead of the existing if
(repo.assistants?.qwen) block, perform a merge that spreads existing
result.assistants.qwen and then spreads repo.assistants.qwen so repo-level
fields override defaults (and mirror how claude/codex merges
model/settingSources), ensuring consistent unconditional spread behavior for
qwen inside mergeRepoConfig.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 504-542: The Qwen branch (provider === 'qwen') silently drops
node.output_format; add a warning and user notification when node.output_format
is present so users know it's ignored. Inside the Qwen options construction
(before the MCP block and after options is set) check node.output_format and
call getLog().warn with a clear key (e.g., 'dag.output_format_ignored_qwen') and
attempt to notify via safeSendMessage(platform, conversationId, ...) including
workflowRunId and node.id; if safeSendMessage returns false, emit an error log
(e.g., 'dag.output_format_warning_delivery_failed') to mirror existing MCP
warning patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76f0b4c6-3768-4b28-b555-afd7b21fd793
📒 Files selected for processing (7)
packages/cli/src/commands/setup.tspackages/core/src/clients/qwen.tspackages/core/src/config/config-loader.tspackages/core/src/utils/env-allowlist.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.tspackages/workflows/src/model-validation.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/utils/env-allowlist.ts
- packages/web/src/routes/SettingsPage.tsx
- packages/cli/src/commands/setup.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/routes/schemas/config.schemas.ts (1)
56-60:⚠️ Potential issue | 🟡 MinorAllow explicit Qwen unset in PATCH schema.
Line 56 currently requires
qwen.modelwhenqwenis present, which conflicts with partial-update semantics and prevents explicit clear/unset flows (qwen: null), even though safe config already supports nullable Qwen values.Proposed schema alignment
- qwen: z - .object({ - model: z.string(), - }) - .optional(), + qwen: z + .object({ + model: z.string().optional(), + }) + .optional() + .nullable(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/schemas/config.schemas.ts` around lines 56 - 60, The PATCH schema currently requires qwen.model when qwen is present; update the qwen schema in config.schemas.ts so callers can explicitly clear/unset Qwen by allowing qwen to be null and its model to be optional — e.g. make the qwen entry accept either null or an object with an optional model (or mark model as .optional() and allow qwen to be .nullable()); locate the qwen schema block and adjust its zod union/optional/nullable usage accordingly so partial updates and explicit null clearing are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server/src/routes/schemas/config.schemas.ts`:
- Around line 56-60: The PATCH schema currently requires qwen.model when qwen is
present; update the qwen schema in config.schemas.ts so callers can explicitly
clear/unset Qwen by allowing qwen to be null and its model to be optional — e.g.
make the qwen entry accept either null or an object with an optional model (or
mark model as .optional() and allow qwen to be .nullable()); locate the qwen
schema block and adjust its zod union/optional/nullable usage accordingly so
partial updates and explicit null clearing are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e07fbcf-7167-4bfe-b544-aefc0a7fe974
📒 Files selected for processing (3)
packages/core/src/config/config-types.tspackages/server/src/routes/schemas/config.schemas.tspackages/workflows/src/validator.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workflows/src/validator.ts
- packages/core/src/config/config-types.ts
This was referenced Apr 11, 2026
This comment was marked as outdated.
This comment was marked as outdated.
…ator warnings, add docs & tests - Schema: make qwen optional+nullable in safeConfigSchema for unset state - Docstring: fix authType docstring to reflect SDK-resolved default - Validator: add Qwen warnings for MCP, skills, hooks, and tool restrictions - Tests: Add 15 comprehensive test cases for Qwen model validation (25 total) - Docs: Create 300+ line Qwen setup guide (docs/qwen-setup.md) - README: Add Qwen as first-class AI assistant with feature comparison - Model recognition: Enhanced isQwenModel() for all Qwen variants Co-Authored-By: Codex GPT-5 <noreply@openai.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
LoneWolf36
force-pushed
the
feat/qwen-support-boundaries
branch
from
b7ab6ca to
98c4654
Compare
Author
✅ Update: Documentation & Tests AddedI have squashed the follow-up documentation and test work directly into this PR (last commit). What was added to this PR:Enhanced last commit to include:
Current PR Stats:
No separate follow-up PR needed!This PR is now comprehensive and includes everything:
Ready to merge as a complete Qwen support package. 🎉 |
Collaborator
|
thanks for the contribution @LoneWolf36 will be closing this, feel free to reopen following the community provider registry pattern keep in mind we will only accept coding agent SDK's as providers for now |
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
2 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
This PR adds Qwen as a first-class assistant option in Archon and documents where the current integration is intentionally narrower than Claude/Codex.
What changed
@qwen-code/sdk.README.mdandCLAUDE.mdto spell out the current Qwen support boundaries.Verification
bun run type-checkbun test packages/core/src/clients/qwen.test.tsbun --filter @archon/core testbun --filter @archon/workflows testbun --filter @archon/cli testNotes
The integration currently uses
@qwen-code/sdkand supports Archon configuration-driven Qwen usage, but provider-specific behavior is still documented where it differs from Claude/Codex.Summary by CodeRabbit
New Features
Documentation
Tests
Chores