feat(agents): add Qwen Coder (qwen3-coder) and Oh My Pi (omp) agents#1138
feat(agents): add Qwen Coder (qwen3-coder) and Oh My Pi (omp) agents#1138jSydorowicz21 wants to merge 13 commits into
Conversation
Promotes the hidden qwen3-coder stub to a real, selectable BETA agent mirroring gemini-cli: stream-json output, --resume, and model-arg selection. Adds a registered QwenOutputParser, Qwen error patterns, NewInstanceModal entry, capabilities, context window, and README. Covered by parser + definition + capabilities + metadata tests; agent-completeness passes.
Adds omp (Oh My Pi, @oh-my-pi/pi-coding-agent) as a real, selectable BETA agent registered everywhere gemini-cli is. Spawns non-interactively as 'omp -p --mode json' with --resume and --model; new custom OmpOutputParser handles omp's JSON event stream (session/message_update[text_delta]/message_end[usage+cost]/agent_end), modeled on the sibling Pi parser. Includes capabilities, error patterns, path discovery, NewInstanceModal entry, README, and tests (parser fixture, definitions, capabilities, metadata, ids); agent-completeness passes.
…803) CI full suite caught that parsers/index.test.ts hardcodes the registered-parser count, which the new QwenOutputParser bumped 6 -> 7. Update the count assertions + title and add a 'should register Qwen parser' test mirroring the Pi one.
Review fixes: set supportsImageInput=false (no image args wired, attachments would be dropped); set supportsResume=true to match the wired --resume args; honor Qwen's is_error:true in QwenOutputParser (reclassify failed results to error events + emit structured AgentError); add a session_not_found error pattern for expired --resume sessions. Deferred plan-mode read-only (matches gemini convention) and native --append-system-prompt (Windows file-variant unverified). Tests updated.
…(#omp) Review fixes for the Oh My Pi agent: drop noPromptSeparator so omp uses Maestro's '--' end-of-options separator (verified omp supports it), protecting flag-like prompts while keeping @image args before the separator; add noToolsArgs ['--no-tools'] so tab-naming launches name-only (mirrors Pi); make OmpOutputParser fall back to assembled streamed text when agent_end has no/empty messages. Declined --append-system-prompt (Windows uses --append-system-prompt-file which omp lacks; cross-cutting) and model free-text (SELECT matches gemini/qwen convention); deferred read-only allowlist.
Round-2 review fixes: strip the inherited Claude 200000 fallback contextWindow from Qwen usage so the spawned process's configured 262144 window drives the context meter/summarization (genuinely larger reported windows are preserved); populate is_error result text from Qwen's error.message so failed runs surface the actionable provider message instead of an empty/generic error. Declined the stdin prompt-flag finding: per qwen-code source, piped child spawns are non-interactive (stdin.isTTY=false) and read stdin as the prompt, so adding -p would duplicate it and forcing it as an arg reintroduces the cmd.exe arg-length limit the stdin path avoids. +4 tests.
…fixed dropdown omp --model is fuzzy and multi-provider (opus, gpt-5.2, openai/gpt-5.2, ollama/..., 20+ providers). The hardcoded 6-item select (incl. stale gpt-5) couldn't represent that; switched to type:'text' matching its sibling pi/hermes/opencode/codex agents.
…pdown qwen-code's -m/--model is declared type:'string' with no choices enum and is an unconstrained pass-through for all non-OAuth auth types (API-key/third-party). It's multi-provider (OpenAI/Anthropic/Gemini/Vertex/OpenRouter/vLLM/Ollama). The hardcoded 5-item select omitted the actual default (coder-model) and the entire multi-provider space; switched to type:'text' like omp/codex/opencode. Test updated.
Adds an omp case to runModelDiscovery so the model combobox lists omp's full multi-provider catalog (provider-qualified selectors, de-duplicated). Without it the picker showed no models. Mirrors the OpenCode discovery pattern; parseJsonWithBom for robustness.
# Conflicts: # README.md # src/__tests__/main/parsers/index.test.ts # src/__tests__/shared/agentMetadata.test.ts # src/main/parsers/error-patterns.ts # src/main/parsers/index.ts # src/main/parsers/parser-factory.ts # src/renderer/components/NewInstanceModal/types.ts # src/shared/agentConstants.ts # src/shared/agentMetadata.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds support for Changesqwen3-coder and omp agent integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR adds Qwen3 Coder and Oh My Pi as beta agents. The main changes are:
Confidence Score: 5/5This looks safe to merge after a small model-discovery cache cleanup.
src/main/agents/detector.ts Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch 'feat/omp-agent' into feat/..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| LOG_CONTEXT, | ||
| { stderr: result.stderr } | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
When omp models --json fails once, this branch returns an empty list and the caller caches that result under the omp agent key for the normal model-cache window. A temporary CLI or parse failure can make the model picker stay empty for later successful retries until the cache expires or is manually cleared.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/main/agents/definitions.test.ts (1)
172-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
qwen3-coderin the “all known agents” list.This list was updated for
omp, but it still skips the other agent added in this PR, so this regression test would not catchgetAgentDefinition('qwen3-coder')returningundefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/main/agents/definitions.test.ts` around lines 172 - 181, The regression test in definitions.test.ts is missing the newly added agent in its knownAgents list, so update the “should return definition for all known agents” case to include qwen3-coder alongside the existing entries. Keep the change localized to the knownAgents array in the getAgentDefinition coverage so the test will fail if qwen3-coder is not registered.
🧹 Nitpick comments (2)
src/__tests__/main/parsers/index.test.ts (1)
149-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the same class/export assertions for Qwen.
This suite now fully exercises OMP, but
qwen3-coderis only checked viahasOutputParser(). AddinggetOutputParser('qwen3-coder')andnew QwenOutputParser().agentIdassertions would cover the new re-export and keep both agents symmetric in this layer.Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/main/parsers/index.test.ts` around lines 149 - 153, The parser tests for qwen3-coder only verify hasOutputParser(), so add matching coverage in the getOutputParser and class/export assertions used for OMP. Update the index.test.ts cases around getOutputParser and the Qwen re-export to call getOutputParser('qwen3-coder') and assert it returns a QwenOutputParser instance, and also verify new QwenOutputParser().agentId matches the expected agent id so the symmetry with OmpOutputParser is covered.src/main/parsers/omp-output-parser.ts (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared ANSI stripping helper here.
This introduces a separate ANSI-stripping path in
src/**. Please switch both the import and call site tostripAnsiCodes()fromsrc/shared/stringUtils.tsso this stays aligned with the shared utility contract.Suggested change
-import { stripAllAnsiCodes } from '../utils/terminalFilter'; +import { stripAnsiCodes } from '../../shared/stringUtils'; ... - const cleanedOutput = stripAllAnsiCodes(`${stderr}\n${stdout}`).trim(); + const cleanedOutput = stripAnsiCodes(`${stderr}\n${stdout}`).trim();As per coding guidelines, "Use
stripAnsiCodes()fromsrc/shared/stringUtils.tsfor removing ANSI escape codes. Do not create duplicate implementations."Also applies to: 267-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/parsers/omp-output-parser.ts` at line 22, The ANSI stripping logic in the parser is using a local helper instead of the shared utility. Update src/main/parsers/omp-output-parser.ts to import stripAnsiCodes from src/shared/stringUtils.ts and replace every stripAllAnsiCodes call in this parser with stripAnsiCodes so the parser follows the shared contract and avoids a duplicate ANSI-removal path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/main/parsers/omp-output-parser.test.ts`:
- Around line 6-20: The OMP parser fixture currently embeds a real local Windows
user path in OMP_SAMPLE, which leaks a developer identifier. Update the captured
JSONL sample in omp-output-parser.test.ts to use a sanitized placeholder path
instead of the concrete C:\\Users\\sydor\\... value, while keeping the rest of
the parser coverage data unchanged.
In `@src/main/parsers/omp-output-parser.ts`:
- Around line 98-105: The OMP parser is treating retrying agent_end events with
error payloads as final failures; update OmpOutputParser so willRetry-marked
events are filtered out before any error short-circuit in both the parse flow
and detectErrorFromParsed logic. Adjust the agent_end handling in the parser
methods that inspect event.error/event.message?.errorMessage and the willRetry
guard so they return a non-fatal path when willRetry is true, then add a
regression test covering agent_end with willRetry true plus error text to ensure
it is not surfaced as a user-facing error.
In `@src/main/parsers/qwen-output-parser.ts`:
- Around line 55-67: The Qwen output parser is deleting any usage.contextWindow
equal to the fallback value, which can նաև remove a legitimate 200000 limit from
real model payloads. Update qwen-output-parser’s event handling so the fallback
is stripped only when the original payload did not report a context window,
using the existing usage normalization path in
QwenOutputParser/aggregateModelUsage. Add a regression test covering an exact
200000 Qwen-reported context window to ensure it is preserved.
---
Outside diff comments:
In `@src/__tests__/main/agents/definitions.test.ts`:
- Around line 172-181: The regression test in definitions.test.ts is missing the
newly added agent in its knownAgents list, so update the “should return
definition for all known agents” case to include qwen3-coder alongside the
existing entries. Keep the change localized to the knownAgents array in the
getAgentDefinition coverage so the test will fail if qwen3-coder is not
registered.
---
Nitpick comments:
In `@src/__tests__/main/parsers/index.test.ts`:
- Around line 149-153: The parser tests for qwen3-coder only verify
hasOutputParser(), so add matching coverage in the getOutputParser and
class/export assertions used for OMP. Update the index.test.ts cases around
getOutputParser and the Qwen re-export to call getOutputParser('qwen3-coder')
and assert it returns a QwenOutputParser instance, and also verify new
QwenOutputParser().agentId matches the expected agent id so the symmetry with
OmpOutputParser is covered.
In `@src/main/parsers/omp-output-parser.ts`:
- Line 22: The ANSI stripping logic in the parser is using a local helper
instead of the shared utility. Update src/main/parsers/omp-output-parser.ts to
import stripAnsiCodes from src/shared/stringUtils.ts and replace every
stripAllAnsiCodes call in this parser with stripAnsiCodes so the parser follows
the shared contract and avoids a duplicate ANSI-removal path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c666217-ec3a-4f83-8679-3f4e8b8a9d46
📒 Files selected for processing (23)
README.mdsrc/__tests__/main/agents/capabilities.test.tssrc/__tests__/main/agents/definitions.test.tssrc/__tests__/main/agents/detector.test.tssrc/__tests__/main/parsers/index.test.tssrc/__tests__/main/parsers/omp-output-parser.test.tssrc/__tests__/main/parsers/qwen-output-parser.test.tssrc/__tests__/shared/agentIds.test.tssrc/__tests__/shared/agentMetadata.test.tssrc/main/agents/capabilities.tssrc/main/agents/definitions.tssrc/main/agents/detector.tssrc/main/agents/path-prober.tssrc/main/parsers/error-patterns.tssrc/main/parsers/index.tssrc/main/parsers/omp-output-parser.tssrc/main/parsers/parser-factory.tssrc/main/parsers/qwen-output-parser.tssrc/renderer/components/NewInstanceModal/types.tssrc/renderer/constants/agentIcons.tssrc/shared/agentConstants.tssrc/shared/agentIds.tssrc/shared/agentMetadata.ts
- omp discovery: don't cache transient empty results so a failed 'omp models --json' doesn't leave the picker empty for the TTL (greptile P2); report parse failures via captureException (CodeRabbit). - omp parser: suppress willRetry-marked agent_end events in both parseJsonObject and detectErrorFromParsed so retrying turns aren't surfaced as final errors (CodeRabbit major) + regression test. - omp parser: use shared stripAnsiCodes() from shared/stringUtils instead of a duplicate path (CodeRabbit). - omp test fixture: redact local username from the captured sample path (CodeRabbit security). - qwen parser: only strip the injected 200000 fallback when the raw payload reported no per-model context window; preserve a genuinely model-reported 200000 (CodeRabbit) + regression test. - tests: add qwen3-coder to definitions knownAgents; add getOutputParser/export symmetry for qwen; add detector cache-retry regression test.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10efc4f3e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event.usage && event.usage.contextWindow === FALLBACK_CONTEXT_WINDOW) { | ||
| const usage = { ...event.usage }; | ||
| delete usage.contextWindow; |
There was a problem hiding this comment.
When a Qwen agent uses the multi-provider model override, for example the OpenAI-compatible models allowed in definitions.ts:375, a real reported context window of 200000 or lower is collapsed to the Claude fallback by aggregateModelUsage and then removed here. StdoutHandler.buildUsageStats will fall back to the Qwen default/configured 262144 window, so context usage and compaction thresholds are too high for those models. Please distinguish "no context window was reported" from "the provider reported 200000 or a lower value" before deleting the field.
Useful? React with 👍 / 👎.
| args: [], | ||
| batchModePrefix: [], | ||
| batchModeArgs: ['-y'], | ||
| jsonOutputArgs: ['--output-format', 'stream-json'], |
There was a problem hiding this comment.
For Qwen runs this only requests --output-format stream-json, but the Qwen CLI docs say partial assistant events require --include-partial-messages and that it defaults off. In normal Maestro tabs that means Qwen is marked streaming/thinking-capable, yet the parser only gets the terminal result instead of live assistant/reasoning deltas, so users see no response content until exit and the Thought Stream cannot populate. Add the partial flag or lower the streaming/thinking capabilities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed valid against qwen-code v0.19.2 source: --output-format stream-json alone emits only the final complete assistant message plus result, and --include-partial-messages (default off) is required for live text/thinking deltas.
Decision for this PR: leaving it as-is. The behavior is functional today, qwen's full response still renders (the parser treats the final assistant message as partial text), just at turn-end rather than token-by-token.
We are not enabling --include-partial-messages here because qwen emits partials as Anthropic-style stream_event envelopes (event.type: content_block_delta, delta.type: text_delta/thinking_delta) that ClaudeOutputParser does not currently parse, and they arrive alongside the final complete message, so flipping the flag without new parser handling and de-duplication would double-render the response.
Live token/thinking streaming for qwen is tracked as a follow-up (add the flag plus a stream_event handler in QwenOutputParser and dedupe the final message), to be done against a live qwen CLI so the envelope shape and ordering can be validated.
| requiresPromptToStart: true, | ||
| supportsStreaming: true, | ||
| supportsResultMessages: true, | ||
| supportsModelSelection: true, |
There was a problem hiding this comment.
Declaring OMP model selection here also enables the SSH remote model picker, but discoverModelsRemote always runs <binary> models and splits stdout by line. The local OMP branch added in this commit has to run omp models --json and read models[].selector; on SSH remotes the picker will be filled with human table headings/box rows, and selecting one sends an invalid --model. Add an OMP-specific remote discovery path or avoid advertising model selection for remote OMP agents.
Useful? React with 👍 / 👎.
| if (event.message?.role === 'assistant') { | ||
| return { | ||
| type: 'usage', | ||
| sessionId, | ||
| usage: this.extractUsageFromMessage(event.message), |
There was a problem hiding this comment.
The captured OMP sample in the new test fixture includes two assistant message_end events carrying the same usage for one turn, but this parser emits a usage event for every assistant message_end. For OMP, StdoutHandler forwards each usage event as a delta and the renderer accumulates those deltas, so one OMP response can double-count tokens and cost in the session totals and Usage Dashboard. Gate this to the authoritative final usage per message or de-duplicate repeated usage payloads.
Useful? React with 👍 / 👎.
…overy) - qwen parser (C1): aggregateModelUsage only overrides the 200000 fallback with a LARGER per-model window, so a model reporting <=200000 (e.g. a custom OpenAI-compatible model) was collapsed to 200000. Now recover the real value from raw modelUsage (largest reported >0) and only strip the injected fallback when no window was reported. +2 tests (200000 and 128000 reported windows preserved). - omp SSH discovery (C3): discoverModelsRemote ran '<binary> models' and split stdout by line; for omp that yields the human table, so the remote picker filled with headings and produced invalid --model. Now request 'omp models --json' and parse selectors for omp, mirroring local discovery. +1 IPC test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/ipc/handlers/agents.ts`:
- Around line 650-681: The remote `omp` JSON branch in `agents.ts` is parsing
raw `result.stdout`, so any ANSI or shell noise can make `parseJsonWithBom()`
fail and cache an empty model list. Normalize the SSH output first by stripping
ANSI escape codes with `stripAnsiCodes()` before parsing, and use the cleaned
stdout consistently in the `agentId === 'omp'` path and the non-JSON fallback so
model discovery stays reliable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2007d45a-2dd4-47cb-9cd0-812a02212ee5
📒 Files selected for processing (4)
src/__tests__/main/ipc/handlers/agents.test.tssrc/__tests__/main/parsers/qwen-output-parser.test.tssrc/main/ipc/handlers/agents.tssrc/main/parsers/qwen-output-parser.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/main/parsers/qwen-output-parser.test.ts
- src/main/parsers/qwen-output-parser.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a24a058344
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| workingDirArgs: (dir: string) => ['--include-directories', dir], | ||
| imageArgs: undefined, | ||
| modelArgs: (modelId: string) => ['-m', modelId], | ||
| promptArgs: (prompt: string) => ['-p', prompt], |
There was a problem hiding this comment.
When a Qwen agent runs on an SSH remote, the SSH path in src/main/ipc/handlers/process.ts builds the remote command from finalArgs and sends effectivePrompt as stdinInput, so this promptArgs builder is not applied. That yields qwen -y --output-format stream-json with the prompt on stdin but no --prompt/-p; Qwen's CLI docs show stdin piping as echo ... | qwen --prompt and stream JSON as qwen --prompt ... --output-format stream-json (https://qwenlm-qwen-code.mintlify.app/cli/overview). In that context the remote process can enter interactive mode or fail instead of producing JSON, so Qwen SSH tabs and batch runs do not complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against qwen-code v0.19.2 source: this is a non-issue. qwen reads a piped (non-TTY) stdin as the prompt automatically; --prompt/-p is additive, not required (description: 'Prompt. Appended to input on stdin (if any).', config.ts:642). With no prompt flag and a non-TTY stdin, it runs non-interactively (interactive = process.stdin.isTTY ?? false, config.ts:1528-1532) and folds stdin into the prompt (gemini.tsx:967-972).
The one caveat is that stdin is consumed as the plain prompt only while --input-format is text (the default); --input-format stream-json would reserve stdin for the JSON protocol. Maestro passes --output-format stream-json but NOT --input-format stream-json, so the SSH stdin-as-prompt path is correct. No -p added.
| // Qwen Code is a Gemini CLI fork; read-only behavior is enforced via system prompt | ||
| // instructions. The -y flag is still needed for non-interactive execution to prevent | ||
| // approval prompts from hanging batch mode. | ||
| readOnlyArgs: ['-y'], |
There was a problem hiding this comment.
When maestro-cli send --read-only or any non-UI caller sets readOnlyMode for a Qwen agent, the definition appends -y, which Qwen documents as YOLO auto-approval, while --approval-mode plan is the mode that analyzes without modifying files or executing commands (https://raw.githubusercontent.com/QwenLM/qwen-code/main/docs/users/configuration/settings.md). That means a run the caller requested as read-only can auto-approve writes and shell commands instead of constraining them; wire Qwen read-only to --approval-mode plan and advertise the capability.
Useful? React with 👍 / 👎.
| */ | ||
| omp: { | ||
| supportsResume: true, | ||
| supportsReadOnlyMode: false, |
There was a problem hiding this comment.
OMP is now available through maestro-cli send, but if callers pass --read-only the generic spawner has no readOnlyArgs to add, so the run still gets OMP's full tool surface. OMP documents that its active tools can be pinned with --tools and lists read/search-only tools such as read, find, and search in its README (https://raw.githubusercontent.com/can1357/oh-my-pi/main/README.md), so the new agent should either wire a restricted --tools ... set and mark the capability true, or reject read-only instead of silently running with write and shell tools.
Useful? React with 👍 / 👎.
…ation) - N1 (CodeRabbit Major): sanitize remote SSH stdout with stripAnsi before parsing omp models --json, matching the sibling line-split branch (shell-integration noise no longer empties the picker). - N3 (Codex P2): qwen read-only used readOnlyArgs ['-y'] (YOLO auto-approve) which DEFEATS read-only. qwen-code v0.19.2 ships GA --approval-mode plan (denies write/shell/edit in non-interactive); switched to ['--approval-mode','plan'] + readOnlyCliEnforced true. buildAgentArgs skips batchModeArgs(-y) in read-only, so no -y/--approval-mode conflict. - N4 (Codex P2): omp had no readOnlyArgs, so --read-only ran with full write/shell tools. Added readOnlyArgs ['--tools','read,grep,glob'] (search->grep, find->glob aliases) + readOnlyCliEnforced true + supportsReadOnlyMode true. - N2 (Codex): INVALID, no change \u2014 qwen reads piped non-TTY stdin as the prompt automatically (input-format text default); Maestro only sets --output-format stream-json. Verified against qwen-code v0.19.2 / omp v16.2.2 source. +regression tests (definitions, capabilities, agent-args).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ad50cd20d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| workingDirArgs: (dir: string) => ['--include-directories', dir], | ||
| imageArgs: undefined, | ||
| modelArgs: (modelId: string) => ['-m', modelId], | ||
| promptArgs: (prompt: string) => ['-p', prompt], |
There was a problem hiding this comment.
Fresh evidence: the Windows raw-stdin path also skips promptArgs, so this is not limited to SSH. Qwen's headless docs list --prompt/-p as the flag that runs a prompt in headless mode and show stream JSON as qwen -p ... --output-format stream-json (https://qwenlm.github.io/qwen-code-docs/en/users/features/headless/); when Windows local or SSH spawns send the prompt through stdin, Maestro does not append this builder, so Qwen launches as qwen -y --output-format stream-json and can enter interactive mode or wait instead of emitting JSON. Add a stdin-safe headless flag for those paths, without duplicating the prompt text.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| if (event.type === 'result' && this.isFailedResult(parsed)) { |
There was a problem hiding this comment.
Qwen's own CI wrapper treats final result text containing [API Error as failed even when is_error is false, because provider/model connection failures can be rendered inline in a nominally successful result (https://raw.githubusercontent.com/QwenLM/qwen-code/main/.github/workflows/qwen-code-pr-review.yml). This branch only reclassifies is_error === true, so those Qwen runs will be recorded as successful and can advance queues/Auto Run while the response is actually an API error; inspect the result text for Qwen's inline API error marker before returning a result.
Useful? React with 👍 / 👎.
| hidden: true, // Not shipping. Kept for type/back-compat, hidden from UI. | ||
| binaryName: 'qwen3-coder', | ||
| command: 'qwen3-coder', | ||
| binaryName: 'qwen', |
There was a problem hiding this comment.
Once this visible agent resolves the qwen binary, detection depends on path-prober knowing where official installers place it. Qwen's docs say the recommended installer uses a standalone archive when available (https://qwenlm.github.io/qwen-code-docs/en/users/overview/), and the completed standalone-installer issue names ~/.qwen/bin/qwen with Windows ~/.qwen/bin/qwen.exe (QwenLM/qwen-code#3728); there is no qwen entry in getUnixKnownPaths or getWindowsKnownPaths, unlike OMP/Gemini, so packaged apps that cannot recover the user's shell PATH will mark Qwen unavailable after a supported install. Add those qwen paths to the direct probes.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #803.
Bundles the two new CLI agents into a single PR. They were previously split across #1128 (Qwen Coder) and #1132 (Oh My Pi), but each adds an agent to the same shared registry files, so they cannot merge independently without conflicts (parser registry, agent metadata/constants, error patterns, the supported-agents list, and the "register exactly N parsers" count). Combining them here resolves that once: both agents registered, parser count is 8.
This supersedes and replaces #1128 and #1132.
Qwen Coder (
qwen3-coder)qwen3-coderstub to a real, selectable BETA agent (binaryqwen), mirroring thegemini-cliintegration: definition, capabilities, metadata, 256K context window,QwenOutputParser, error patterns,--output-format stream-json,--resume.-m/--modelis an unconstrained, multi-provider string per its source; a fixed dropdown would exclude valid models).Oh My Pi (
omp)omp(@oh-my-pi/pi-coding-agent) as a real, selectable BETA agent registered everywheregemini-cliis: definition, capabilities, metadata,OmpOutputParser(modeled on the siblingpi-output-parser), error patterns,-p --mode json,--resume,--model.runModelDiscoveryrunsomp models --jsonto populate the model combobox with omp's full multi-provider catalog (provider-qualified selectors, de-duplicated, parsed withparseJsonWithBom).Verification
npm run lint(tsc x3): clean.vitest runacross parsers/index, agent definitions, agent-completeness, agentMetadata, detector (incl. omp discovery), capabilities: 252 tests pass.eslint+prettier: clean on the touched files.Notes
Summary by CodeRabbit
qwen3-coderandomp(beta), including UI display name/icon, agent setup, model discovery, and output parsing with session + streaming + usage/cost handling.ompto allow retries.