feat: opencode backend + per-user opt-in providers (Z.AI/Groq/OpenRouter)#141
Merged
Conversation
…OpenRouter)
Adds "opencode" as a fifth inference backend, driven as a subprocess CLI like
the cc/codex backends. It inherits opencode's broad provider support, but
providers are gated per-user: nothing is enabled for everyone. Each user opts
into providers individually via a new /providers command, and only their
enabled providers appear in their /orch model picker.
Backend (mirrors CCAgent — native session resume + rich NDJSON stream):
- internal/agent/opencode_agent.go: OpenCodeAgent implements CLIAgent, drives
`opencode run --format json --model <provider>/<model> [--session <id>]`,
parses NDJSON (text/tool/step events), resumes via --session.
- internal/agent/opencode_config.go: writes a qmax-managed opencode config
(~/.qmax-code/opencode.json) pointed at the subprocess via OPENCODE_CONFIG so
the user's own opencode.jsonc is never clobbered. qmax MCP server entry lets
opencode call qmax tools natively.
Per-user providers:
- internal/api/providers.go: registry (zai-coding-plan custom, groq, openrouter),
per-provider keychain storage, ValidateProviderKey, Config.EnabledProviders,
and ProviderAllowed() — the single entitlement seam (returns true locally
today; a cloud/QualityMax check plugs in later without UI changes). Visible
set is always (entitled AND enabled).
- Keys are bring-your-own, stored in the OS keychain. They never touch disk in
the managed config: custom providers reference {env:VAR} and the real key is
injected into the subprocess env at launch (dynamic model list via
`opencode models <provider>`).
Wiring: --backend flag + config validation, startup routing + construction
(main.go), /orch picker section + dynamic entries (tui_backend.go), /providers
and /opencode commands + slash menu (repl.go, input.go), consent branch
(consent.go), config show (config_command.go).
Tests: provider registry/keychain/validation, managed-config generation
(asserts no plaintext key leak, provider block + MCP present), env injection.
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushAdds the opencode subprocess backend with per-user opt-in providers (Z.AI, Groq, OpenRouter), a Important files
Sequence diagramsequenceDiagram
participant User
participant REPL
participant Providers as /providers
participant Config as WriteOpenCodeConfig
participant Keychain
participant OpenCode as opencode CLI
User->>REPL: /providers enable groq
REPL->>Keychain: SaveProviderKey(groq, key)
REPL->>Config: WriteOpenCodeConfig(providers)
Config->>Config: Write {env:VAR} refs
User->>REPL: /opencode
REPL->>Config: WriteOpenCodeConfig(providers, session, mode)
REPL->>OpenCode: OpenCodeModels(bin, config, env, provider)
OpenCode-->>REPL: [groq/model-a, groq/model-b]
REPL->>REPL: Show model picker
User->>REPL: Select model
REPL->>OpenCode: NewOpenCodeAgent(bin, model, mode)
Confidence: 3/5Synchronous subprocess calls in the TUI picker path and untested REPL handler logic introduce latency and regression risk that should be validated before merge.
Suggested labels: Phase: Degraded |
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| a.mu.Unlock() | ||
| } | ||
|
|
||
| func (a *OpenCodeAgent) SetOutputVerbose(verbose bool) { |
There was a problem hiding this comment.
Unlocked read of outputVerbose races with SetOutputVerbose
SetOutputVerbose writes outputVerbose under a.mu, but parseStream reads it without the mutex when deciding whether to emit the trailing newline, and Run reads it without the mutex when building the system prompt. This breaks the intended synchronization and is a data race under Go's memory model; a concurrent verbose toggle could stream stale state or be flagged by the race detector.
Example:
input: user toggles verbose mode while a long opencode turn is streaming
actual: race detector reports read/write race on outputVerbose
expected: all reads and writes of outputVerbose are synchronized
Current:
func (a *OpenCodeAgent) SetOutputVerbose(verbose bool) {Proposed:
if !a.outputVerboseLocked() {
Suggested change
| func (a *OpenCodeAgent) SetOutputVerbose(verbose bool) { | |
| if !a.outputVerboseLocked() { |
More Info
- Threat model: A concurrent call to
SetOutputVerbose(e.g., from a TUI/config toggle) while a turn is running reads a value that is not happens-before ordered with the write, producing undefined behavior in Go. - Specific code citations:
SetOutputVerboselocksa.muaround the write;parseStreamaccessesa.outputVerbosedirectly at the tool-icon newline branch;Runpassesa.outputVerbosetooutputStyleDirective. - Existing protections: Other mutable fields (
sessionID,lastToolName) are correctly accessed undera.mu; onlyoutputVerboseis read outside the lock. - Proposed mitigation: Add a locked getter for
outputVerboseand use it in bothRunandparseStream, or wrap each read ina.mu.Lock()/Unlock(). - Alternative mitigations considered: Remove the mutex from
SetOutputVerboseif the field is never mutated concurrently, but that contradicts the existing lock and leaves the method racy if callers do use it concurrently. - Severity calibration: Score 3: the blast radius is bounded to output formatting, but it is a genuine data race on mutable agent state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 288
Comment:
**Unlocked read of `outputVerbose` races with `SetOutputVerbose`**
`SetOutputVerbose` writes `outputVerbose` under `a.mu`, but `parseStream` reads it without the mutex when deciding whether to emit the trailing newline, and `Run` reads it without the mutex when building the system prompt. This breaks the intended synchronization and is a data race under Go's memory model; a concurrent verbose toggle could stream stale state or be flagged by the race detector.
Example:
input: user toggles verbose mode while a long opencode turn is streaming
actual: race detector reports read/write race on outputVerbose
expected: all reads and writes of outputVerbose are synchronized
Threat model:
A concurrent call to `SetOutputVerbose` (e.g., from a TUI/config toggle) while a turn is running reads a value that is not happens-before ordered with the write, producing undefined behavior in Go.
Specific code citations:
`SetOutputVerbose` locks `a.mu` around the write; `parseStream` accesses `a.outputVerbose` directly at the tool-icon newline branch; `Run` passes `a.outputVerbose` to `outputStyleDirective`.
Existing protections:
Other mutable fields (`sessionID`, `lastToolName`) are correctly accessed under `a.mu`; only `outputVerbose` is read outside the lock.
Proposed mitigation:
Add a locked getter for `outputVerbose` and use it in both `Run` and `parseStream`, or wrap each read in `a.mu.Lock()/Unlock()`.
Alternative mitigations considered:
Remove the mutex from `SetOutputVerbose` if the field is never mutated concurrently, but that contradicts the existing lock and leaves the method racy if callers do use it concurrently.
Severity calibration:
Score 3: the blast radius is bounded to output formatting, but it is a genuine data race on mutable agent state.
Suggested fix shape:
func (a *OpenCodeAgent) outputVerboseLocked() bool {
a.mu.Lock()
defer a.mu.Unlock()
return a.outputVerbose
}
How can I resolve this? If you propose a fix, please make it concise.
| if len(active) == 0 { | ||
| return nil | ||
| } | ||
| path, err := agent.WriteOpenCodeConfig(cfg, sctx) |
There was a problem hiding this comment.
Sequential opencode models subprocess calls block the model picker
buildOpenCodeModelEntries loops over each active provider and calls agent.OpenCodeModels, which shells out to opencode models <provider> with a 15-second timeout per call. With N enabled providers the picker opening takes the sum of N subprocess latencies — up to N * 15 s worst case — when these independent calls could run concurrently, reducing wait to the max single-call latency. With 2–3 providers and typical CLI startup of 1–3 s each, the picker opens in 3–9 s instead of 1–3 s.
More Info
- Threat model: Any user who enables 2+ opencode providers and opens /orch experiences a multiplied delay before the picker renders.
- Specific code citations: internal/repl/repl.go
buildOpenCodeModelEntries— thefor _, p := range activeloop callingagent.OpenCodeModels(bin, path, p.ID)sequentially; internal/agent/opencode_config.goOpenCodeModels—exec.CommandContext(ctx, bin, 'models', providerID)with15*time.Secondtimeout. - Existing protections: The 15 s per-call timeout prevents infinite hangs but does not reduce the additive latency across providers.
- Proposed mitigation: Run the
OpenCodeModelscalls concurrently (e.g.,errgroup.GroupwithSetLimit(len(active))) and collect results intoentrieswith a mutex. The rest ofbuildOpenCodeModelEntries(config write, label extraction) stays sequential. - Alternative mitigations considered: Cache model lists with a short TTL so repeated /orch opens skip the subprocess; still needs concurrent cold-fill.
- Severity calibration: Score 3 because the delay is user-facing and multiplicative with provider count, but the current provider catalogue is small (3 built-in) and the action is user-initiated — not a production-down or throughput issue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1784
Comment:
**Sequential `opencode models` subprocess calls block the model picker**
`buildOpenCodeModelEntries` loops over each active provider and calls `agent.OpenCodeModels`, which shells out to `opencode models <provider>` with a 15-second timeout per call. With N enabled providers the picker opening takes the sum of N subprocess latencies — up to N * 15 s worst case — when these independent calls could run concurrently, reducing wait to the max single-call latency. With 2–3 providers and typical CLI startup of 1–3 s each, the picker opens in 3–9 s instead of 1–3 s.
Threat model:
Any user who enables 2+ opencode providers and opens /orch experiences a multiplied delay before the picker renders.
Specific code citations:
internal/repl/repl.go `buildOpenCodeModelEntries` — the `for _, p := range active` loop calling `agent.OpenCodeModels(bin, path, p.ID)` sequentially; internal/agent/opencode_config.go `OpenCodeModels` — `exec.CommandContext(ctx, bin, 'models', providerID)` with `15*time.Second` timeout.
Existing protections:
The 15 s per-call timeout prevents infinite hangs but does not reduce the additive latency across providers.
Proposed mitigation:
Run the `OpenCodeModels` calls concurrently (e.g., `errgroup.Group` with `SetLimit(len(active))`) and collect results into `entries` with a mutex. The rest of `buildOpenCodeModelEntries` (config write, label extraction) stays sequential.
Alternative mitigations considered:
Cache model lists with a short TTL so repeated /orch opens skip the subprocess; still needs concurrent cold-fill.
Severity calibration:
Score 3 because the delay is user-facing and multiplicative with provider count, but the current provider catalogue is small (3 built-in) and the action is user-initiated — not a production-down or throughput issue.
How can I resolve this? If you propose a fix, please make it concise.
| continue | ||
| } | ||
| seenTool[ev.Part.ID] = true | ||
| displayName := stripMCPPrefix(ev.Part.Tool) |
There was a problem hiding this comment.
parseStream silently drops oversized NDJSON lines
The scanner is capped at 1 MiB per line, but parseStream never checks scanner.Err() after the loop. If opencode emits a single NDJSON event larger than that, Scan() stops and the remainder of the stream is discarded without surfacing an error.
Example:
input: opencode emits a 2 MiB tool-result event
actual: scanner stops, remaining response is lost, no error returned
expected: the oversized-line error is propagated or logged
Current:
displayName := stripMCPPrefix(ev.Part.Tool)Proposed:
term.FinishMarkdown(finalResult)
return finalResult
Suggested change
| displayName := stripMCPPrefix(ev.Part.Tool) | |
| term.FinishMarkdown(finalResult) | |
| return finalResult |
More Info
- Threat model: An oversized tool-result event from opencode causes silent truncation of the assistant's response.
- Specific code citations:
parseStreamcreatesbufio.ScannerwithBuffer(make([]byte, 1<<20), 1<<20)but lacks ascanner.Err()check after thefor scanner.Scan()loop. - Existing protections: None — the error is silently swallowed.
- Proposed mitigation: Check
scanner.Err()after the loop and return an error wrapping it. - Alternative mitigations considered: Increase the buffer cap, but that only raises the threshold; the error check is still required.
- Severity calibration: Score 2: requires an unusually large single event to trigger, but when it does the failure is silent data loss.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 260
Comment:
**`parseStream` silently drops oversized NDJSON lines**
The scanner is capped at 1 MiB per line, but `parseStream` never checks `scanner.Err()` after the loop. If opencode emits a single NDJSON event larger than that, `Scan()` stops and the remainder of the stream is discarded without surfacing an error.
Example:
input: opencode emits a 2 MiB tool-result event
actual: scanner stops, remaining response is lost, no error returned
expected: the oversized-line error is propagated or logged
Threat model:
An oversized tool-result event from opencode causes silent truncation of the assistant's response.
Specific code citations:
`parseStream` creates `bufio.Scanner` with `Buffer(make([]byte, 1<<20), 1<<20)` but lacks a `scanner.Err()` check after the `for scanner.Scan()` loop.
Existing protections:
None — the error is silently swallowed.
Proposed mitigation:
Check `scanner.Err()` after the loop and return an error wrapping it.
Alternative mitigations considered:
Increase the buffer cap, but that only raises the threshold; the error check is still required.
Severity calibration:
Score 2: requires an unusually large single event to trigger, but when it does the failure is silent data loss.
Suggested fix shape:
if err := scanner.Err(); err != nil {
return finalResult, fmt.Errorf("opencode stream: %w", err)
}
How can I resolve this? If you propose a fix, please make it concise.
| // "provider/model" strings. It shells out to `opencode models <providerID>` | ||
| // with OPENCODE_CONFIG pointed at the managed config so custom providers | ||
| // (whose models come from our seeded block) resolve too. Returns nil on error. | ||
| func OpenCodeModels(bin, configPath, providerID string) []string { |
There was a problem hiding this comment.
OpenCodeModels ignores scanner errors
Like parseStream, OpenCodeModels never checks scanner.Err() after scanning opencode models output. An oversized or malformed line would silently truncate the model list returned to the picker.
Example:
input: `opencode models groq` emits a line > 1 MiB
actual: scanning stops and the function returns the partial list
expected: scanner error is checked and surfaced
Current:
func OpenCodeModels(bin, configPath, providerID string) []string {Proposed:
return models
Suggested change
| func OpenCodeModels(bin, configPath, providerID string) []string { | |
| return models |
More Info
- Threat model: A provider with many models or a malformed line causes the picker to show an incomplete model list without warning.
- Specific code citations:
OpenCodeModelscreates a scanner withBuffer(make([]byte, 1<<16), 1<<20)but lacks ascanner.Err()check after thefor scanner.Scan()loop. - Existing protections: None — the error is silently swallowed.
- Proposed mitigation: Check
scanner.Err()after the loop and returnnilon error (consistent with the function's existing error behavior). - Alternative mitigations considered: Return the partial list with an error, but the function signature returns
[]stringonly. - Severity calibration: Score 2: silent truncation of model list is a correctness bug, but the 15 s subprocess timeout and typical model list sizes make oversized lines unlikely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_config.go
Line: 142
Comment:
**`OpenCodeModels` ignores scanner errors**
Like `parseStream`, `OpenCodeModels` never checks `scanner.Err()` after scanning `opencode models` output. An oversized or malformed line would silently truncate the model list returned to the picker.
Example:
input: `opencode models groq` emits a line > 1 MiB
actual: scanning stops and the function returns the partial list
expected: scanner error is checked and surfaced
Threat model:
A provider with many models or a malformed line causes the picker to show an incomplete model list without warning.
Specific code citations:
`OpenCodeModels` creates a scanner with `Buffer(make([]byte, 1<<16), 1<<20)` but lacks a `scanner.Err()` check after the `for scanner.Scan()` loop.
Existing protections:
None — the error is silently swallowed.
Proposed mitigation:
Check `scanner.Err()` after the loop and return `nil` on error (consistent with the function's existing error behavior).
Alternative mitigations considered:
Return the partial list with an error, but the function signature returns `[]string` only.
Severity calibration:
Score 2: silent truncation of model list is a correctness bug, but the 15 s subprocess timeout and typical model list sizes make oversized lines unlikely.
Suggested fix shape:
if err := scanner.Err(); err != nil {
return nil
}
return models
How can I resolve this? If you propose a fix, please make it concise.
| } | ||
| cliAgent = ca | ||
| case "opencode": | ||
| oc := agent.NewOpenCodeAgent(agent.FindOpenCode(), appConfig.ModelOverride, appConfig.Effort, appConfig.OrchPermissionMode, appConfig.OutputVerbose, appConfig, ctx) |
There was a problem hiding this comment.
Validated opencode binary path is discarded and re-looked up
In main.go, the opencode path validates agent.FindOpenCode() into openCodeBin and exits if it is empty, but then discards that validated path and calls agent.FindOpenCode() again when constructing NewOpenCodeAgent. This breaks the invariant that the agent receives the same binary path that was just confirmed to exist. Pass the already-validated openCodeBin to the constructor instead.
Why this wasn't caught: Existing tests verify the picker signature change, not the main.go agent construction path.
Current:
oc := agent.NewOpenCodeAgent(agent.FindOpenCode(), appConfig.ModelOverride, appConfig.Effort, appConfig.OrchPermissionMode, appConfig.OutputVerbose, appConfig, ctx)Proposed:
oc := agent.NewOpenCodeAgent(openCodeBin, appConfig.ModelOverride, appConfig.Effort, appConfig.OrchPermissionMode, appConfig.OutputVerbose, appConfig, ctx)
Suggested change
| oc := agent.NewOpenCodeAgent(agent.FindOpenCode(), appConfig.ModelOverride, appConfig.Effort, appConfig.OrchPermissionMode, appConfig.OutputVerbose, appConfig, ctx) | |
| oc := agent.NewOpenCodeAgent(openCodeBin, appConfig.ModelOverride, appConfig.Effort, appConfig.OrchPermissionMode, appConfig.OutputVerbose, appConfig, ctx) |
More Info
- Threat model: If the binary disappears between the two lookups (unlikely but possible), the agent receives an empty string and fails later with a less clear error.
- Specific code citations: main.go lines 300-324 validate
openCodeBin := agent.FindOpenCode()and exit on empty; line 435 callsagent.FindOpenCode()again forNewOpenCodeAgent. - Existing protections: The first lookup guards the backend activation; the second is redundant.
- Proposed mitigation: Pass the validated
openCodeBinvariable toNewOpenCodeAgent. - Alternative mitigations considered: None needed — this is a straightforward variable reuse.
- Severity calibration: Score 2: low likelihood of divergence, but breaks a clear validation invariant and duplicates work.
Prompt To Fix With AI
This is a comment left during a code review.
Path: main.go
Line: 435
Comment:
**Validated opencode binary path is discarded and re-looked up**
In main.go, the opencode path validates `agent.FindOpenCode()` into `openCodeBin` and exits if it is empty, but then discards that validated path and calls `agent.FindOpenCode()` again when constructing `NewOpenCodeAgent`. This breaks the invariant that the agent receives the same binary path that was just confirmed to exist. Pass the already-validated `openCodeBin` to the constructor instead.
Threat model:
If the binary disappears between the two lookups (unlikely but possible), the agent receives an empty string and fails later with a less clear error.
Specific code citations:
main.go lines 300-324 validate `openCodeBin := agent.FindOpenCode()` and exit on empty; line 435 calls `agent.FindOpenCode()` again for `NewOpenCodeAgent`.
Existing protections:
The first lookup guards the backend activation; the second is redundant.
Proposed mitigation:
Pass the validated `openCodeBin` variable to `NewOpenCodeAgent`.
Alternative mitigations considered:
None needed — this is a straightforward variable reuse.
Severity calibration:
Score 2: low likelihood of divergence, but breaks a clear validation invariant and duplicates work.
Why this wasn't caught:
Existing tests verify the picker signature change, not the main.go agent construction path.
How can I resolve this? If you propose a fix, please make it concise.
…sion/teardown Addresses review of the opencode backend: - BLOCKER: OpenCodeModels invoked `opencode models <provider>` without the provider key env, so known providers (groq/openrouter) returned "Provider not found" and never reached the picker. Thread OpenCodeProviderEnv through to the listing call. Regression test with a stub binary asserts the key is passed. - /opencode could launch opencode with a model from a different backend (e.g. gpt-5.6 left in ModelOverride by Codex). Add resolveOpenCodeModelOverride: keep the override only if it's a real provider/model for an enabled provider, else fall back to the first available model. - Standard permission mode was a no-op for opencode (only --auto for unattended). Write an explicit permission policy in standard mode (deny edits + destructive shell) and always launch with --auto so non-denied tools run in the non-interactive `opencode run`. This reproduces CC-standard's intent (reads/tests allowed, no file mutation / destructive shell). Test asserts the policy is present in standard and absent in unattended. - A failed /opencode switch tore down the active agent first. Move all validation (CLI present, providers enabled, model resolvable, consent) into a pre-flight before teardown, so a failed switch leaves the current backend intact.
| switch { | ||
| case ev.Type == "text" || ev.Part.Type == "text": | ||
| id := ev.Part.ID | ||
| text := ev.Part.Text |
There was a problem hiding this comment.
opencode NDJSON parser drops text payloads that arrive on tool events
parseStream scans NDJSON and discriminates ev.Type == "text" || ev.Part.Type == "text" as one case, and ev.Part.Type == "tool" || ev.Type == "tool" as the other. A tool event that carries both a Part.Tool and a non-empty Part.Text (for example, a tool result event where opencode emits type:"tool" plus a text payload containing the tool output) is consumed only by the tool branch, and the text payload is silently dropped — the new text never enters textByPart and never appears in the final result. This silently loses tool-result content. The fix is to check ev.Part.Text regardless of Type; the most natural forward-proof shape is a switch on ev.Part.Type that falls through to a shared if text := ev.Part.Text; text != "" append.
Example:
input: NDJSON line `{"type":"tool","part":{"id":"p1","type":"tool","tool":"edit","text":"Applied edit..."}}`
actual: tool icon shown; "Applied edit..." never rendered and not contributed to finalResult
expected: text is streamed/accumulated and included in finalResult alongside any streamed text from prior text-only parts
Suggested fix:
switch ev.Part.Type {
case "text":
// handle text-by-part accumulator and delta streaming as today
case "tool":
// handle tool-icon display
}
// shared: always process text regardless of Part.Type
if text := ev.Part.Text; text != "" {
// accumulate and stream delta
}More Info
- Threat model: A user asking opencode to run a tool and incorporate its output in the conversation receives an answer that is missing the tool result. The impact is correctness — assistant reasoning that depends on the tool output degrades or is lost. This is not a security or auth boundary.
- Specific code citations:
parseStreamininternal/agent/opencode_agent.golines 217–260: the top-levelswitchbranches are exclusive — thetextbranch only runs whenev.Type == "text" || ev.Part.Type == "text", and thetoolbranch only runs whenev.Part.Type == "tool" || ev.Type == "tool". No common path processesev.Part.Textwhenev.Part.Type == "tool" && ev.Part.Text != "". - Existing protections: None — the parser has no fallback that captures text on tool-typed events.
- Proposed mitigation: Refactor the event loop to always process
ev.Part.Textwhen non-empty, independent ofev.Part.Type. Aswitchonev.Part.Typefor type-specific handling (tool icon, etc.) followed by a shared text-accumulation block is the cleanest fix. - Alternative mitigations considered: Keep the current branch structure but duplicate the text-accumulation logic in the tool branch — more error-prone and harder to maintain.
- Severity calibration: Score 4 because this is a reachable correctness bug on a core conversation path (tool use) with no test coverage; it silently corrupts assistant output when opencode emits tool-result text.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 239
Comment:
**opencode NDJSON parser drops text payloads that arrive on tool events**
`parseStream` scans NDJSON and discriminates `ev.Type == "text" || ev.Part.Type == "text"` as one case, and `ev.Part.Type == "tool" || ev.Type == "tool"` as the other. A tool event that carries both a `Part.Tool` and a non-empty `Part.Text` (for example, a tool result event where opencode emits `type:"tool"` plus a text payload containing the tool output) is consumed only by the tool branch, and the text payload is silently dropped — the new text never enters `textByPart` and never appears in the final result. This silently loses tool-result content. The fix is to check `ev.Part.Text` regardless of `Type`; the most natural forward-proof shape is a `switch` on `ev.Part.Type` that falls through to a shared `if text := ev.Part.Text; text != ""` append.
Example:
input: NDJSON line `{"type":"tool","part":{"id":"p1","type":"tool","tool":"edit","text":"Applied edit..."}}`
actual: tool icon shown; "Applied edit..." never rendered and not contributed to finalResult
expected: text is streamed/accumulated and included in finalResult alongside any streamed text from prior text-only parts
Threat model:
A user asking opencode to run a tool and incorporate its output in the conversation receives an answer that is missing the tool result. The impact is correctness — assistant reasoning that depends on the tool output degrades or is lost. This is not a security or auth boundary.
Specific code citations:
`parseStream` in `internal/agent/opencode_agent.go` lines 217–260: the top-level `switch` branches are exclusive — the `text` branch only runs when `ev.Type == "text" || ev.Part.Type == "text"`, and the `tool` branch only runs when `ev.Part.Type == "tool" || ev.Type == "tool"`. No common path processes `ev.Part.Text` when `ev.Part.Type == "tool" && ev.Part.Text != ""`.
Existing protections:
None — the parser has no fallback that captures text on tool-typed events.
Proposed mitigation:
Refactor the event loop to always process `ev.Part.Text` when non-empty, independent of `ev.Part.Type`. A `switch` on `ev.Part.Type` for type-specific handling (tool icon, etc.) followed by a shared text-accumulation block is the cleanest fix.
Alternative mitigations considered:
Keep the current branch structure but duplicate the text-accumulation logic in the tool branch — more error-prone and harder to maintain.
Severity calibration:
Score 4 because this is a reachable correctness bug on a core conversation path (tool use) with no test coverage; it silently corrupts assistant output when opencode emits tool-result text.
Suggested fix shape:
switch ev.Part.Type {
case "text":
// handle text-by-part accumulator and delta streaming as today
case "tool":
// handle tool-icon display
}
// shared: always process text regardless of Part.Type
if text := ev.Part.Text; text != "" {
// accumulate and stream delta
}
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1793
to
+1816
| if bin == "" { | ||
| return nil | ||
| } | ||
| active := cfg.ActiveProviders() | ||
| if len(active) == 0 { | ||
| return nil | ||
| } | ||
| path, err := agent.WriteOpenCodeConfig(cfg, sctx, cfg.OrchPermissionMode) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| // Provider keys must be present in the env or `opencode models <provider>` | ||
| // reports "Provider not found" for known providers (groq/openrouter). | ||
| env := agent.OpenCodeProviderEnv(cfg) | ||
| var entries []tui.OpenCodeModelEntry | ||
| for _, p := range active { | ||
| for _, full := range agent.OpenCodeModels(bin, path, env, p.ID) { | ||
| label := full | ||
| if i := strings.Index(full, "/"); i >= 0 { | ||
| label = full[i+1:] | ||
| } | ||
| entries = append(entries, tui.OpenCodeModelEntry{ | ||
| ProviderID: p.ID, | ||
| ProviderName: p.DisplayName, |
There was a problem hiding this comment.
No test or guard for the synchronous multi-provider model-list resolution during /orch
resolveOpenCodeModelOverride calls buildOpenCodeModelEntries, which shells out to opencode models <provider> for every active provider serially and synchronously, with the full REPL input loop blocked. Each call has a 15-second timeout (opencode_config.go:185), and with multiple enabled providers the total blocking time can exceed 30 seconds. This is not a performance bug (the underlying work is necessary); it is a missing test for the usability contract: no test asserts that the model list is resolved quickly (e.g. N providers within 3×15s) or that it signals progress so the user is not staring at a frozen terminal wondering if qmax hung. A small integration test — or at minimum a documented cap such as cancelling the context when the REPL's own cancellation signal fires — would guard the contract.
Example:
input: user enables 4 providers, opens /orch
expected: model list appears within a few seconds or a progress indicator shows
actual: terminal blocks for up to 60 seconds with no feedback; if one provider is unreachable, the 15-second timeout stalls each subsequent call before any models are rendered
Suggested fix:
Insert a context cancellation tied to a terminal-level interrupt signal, and/or test that the model resolution function returns within an upper-bound time budget when all providers are reachable.More Info
- Threat model: Human user — this is a UX failure, not a system boundary. Indefinite blocking in a TUI makes qmax appear hung.
- Specific code citations:
buildOpenCodeModelEntriesininternal/repl/repl.golines 1786–1806: thefor _, p := range activeloop callingagent.OpenCodeModelssequentially.OpenCodeModelsininternal/agent/opencode_config.golines 186–207: spawnsexec.CommandContextwith 15 s timeout per provider. - Existing protections: Each
OpenCodeModelscall has a 15 scontext.WithTimeout, preventing an infinite hang per provider, but the timeouts are additive across the sequential loop. No test exercises a multi-provider scenario, and no bounded-time contract is asserted. - Proposed mitigation: Add a test that exercises multi-provider model resolution and asserts an upper-bound wall-clock time (e.g. 3 providers resolve within 10 s). Optionally, tie the subprocess context to a REPL-level cancellation signal so Ctrl-C or terminal interrupt aborts the model fetch.
- Alternative mitigations considered: Parallelize the subprocess calls (see performance finding) which reduces wall-clock time but does not replace the need for a contract test.
- Severity calibration: Score 4 because the blocking path is on the primary interactive command (
/orch), affects every user with >1 enabled provider, and has zero test coverage for the multi-producer case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1793-1816
Comment:
**No test or guard for the synchronous multi-provider model-list resolution during /orch**
`resolveOpenCodeModelOverride` calls `buildOpenCodeModelEntries`, which shells out to `opencode models <provider>` for every active provider *serially* and *synchronously*, with the full REPL input loop blocked. Each call has a 15-second timeout (`opencode_config.go:185`), and with multiple enabled providers the total blocking time can exceed 30 seconds. This is not a performance bug (the underlying work is necessary); it is a missing test for the usability contract: no test asserts that the model list is resolved quickly (e.g. N providers within 3×15s) or that it signals progress so the user is not staring at a frozen terminal wondering if qmax hung. A small integration test — or at minimum a documented cap such as cancelling the context when the REPL's own cancellation signal fires — would guard the contract.
Example:
input: user enables 4 providers, opens /orch
expected: model list appears within a few seconds or a progress indicator shows
actual: terminal blocks for up to 60 seconds with no feedback; if one provider is unreachable, the 15-second timeout stalls each subsequent call before any models are rendered
Threat model:
Human user — this is a UX failure, not a system boundary. Indefinite blocking in a TUI makes qmax appear hung.
Specific code citations:
`buildOpenCodeModelEntries` in `internal/repl/repl.go` lines 1786–1806: the `for _, p := range active` loop calling `agent.OpenCodeModels` sequentially. `OpenCodeModels` in `internal/agent/opencode_config.go` lines 186–207: spawns `exec.CommandContext` with 15 s timeout per provider.
Existing protections:
Each `OpenCodeModels` call has a 15 s `context.WithTimeout`, preventing an infinite hang per provider, but the timeouts are additive across the sequential loop. No test exercises a multi-provider scenario, and no bounded-time contract is asserted.
Proposed mitigation:
Add a test that exercises multi-provider model resolution and asserts an upper-bound wall-clock time (e.g. 3 providers resolve within 10 s). Optionally, tie the subprocess context to a REPL-level cancellation signal so Ctrl-C or terminal interrupt aborts the model fetch.
Alternative mitigations considered:
Parallelize the subprocess calls (see performance finding) which reduces wall-clock time but does not replace the need for a contract test.
Severity calibration:
Score 4 because the blocking path is on the primary interactive command (`/orch`), affects every user with >1 enabled provider, and has zero test coverage for the multi-producer case.
Suggested fix shape:
Insert a context cancellation tied to a terminal-level interrupt signal, and/or test that the model resolution function returns within an upper-bound time budget when all providers are reachable.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1786
to
+1800
| // buildOpenCodeModelEntries resolves the picker rows for the opencode backend: | ||
| // one per model exposed by each provider the user enabled AND is entitled to. | ||
| // It queries `opencode models <provider>` at call time so the list is live | ||
| // (models.dev-backed for Groq/OpenRouter, seeded config for custom providers). | ||
| // Returns nil when opencode isn't installed or no providers are active. | ||
| func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) []tui.OpenCodeModelEntry { | ||
| bin := agent.FindOpenCode() | ||
| if bin == "" { | ||
| return nil | ||
| } | ||
| active := cfg.ActiveProviders() | ||
| if len(active) == 0 { | ||
| return nil | ||
| } | ||
| path, err := agent.WriteOpenCodeConfig(cfg, sctx, cfg.OrchPermissionMode) |
There was a problem hiding this comment.
Sequential opencode models subprocess calls block the interactive picker
buildOpenCodeModelEntries loops over active providers and calls agent.OpenCodeModels for each one sequentially. Each call spawns an opencode models <provider> subprocess with a 15-second timeout. With N enabled providers, the caller blocks for up to N×15 s before the picker renders. The /orch command already prints 'Querying opencode models for your enabled providers…' and then freezes the TUI while these subprocesses run. Under normal conditions (1–3 s per call, 3–4 providers) this adds 3–12 s of latency; if opencode is slow or a provider is unreachable, it can approach the full 60 s timeout budget. The fix is to fan out the OpenCodeModels calls concurrently (e.g. errgroup with a semaphore) and collect results, cutting wall-clock time to roughly one round-trip instead of N.
Suggested fix:
var eg errgroup.Group
results := make([][]string, len(active))
for i, p := range active {
i, p := i, p
eg.Go(func() error {
results[i] = agent.OpenCodeModels(bin, path, env, p.ID)
return nil
})
}
eg.Wait()
for _, ms := range results {
for _, full := range ms {
// ... build entries
}
}More Info
- Threat model: Interactive CLI user types
/orchor/opencodeand experiences multi-second to multi-tens-of-seconds freeze before the picker appears. - Specific code citations:
buildOpenCodeModelEntriesininternal/repl/repl.golines 1786–1806: thefor _, p := range activeloop callingagent.OpenCodeModels(bin, path, env, p.ID)sequentially.OpenCodeModelsininternal/agent/opencode_config.golines 186–207: spawnsexec.CommandContextwith 15 s timeout per provider. - Existing protections: Each
OpenCodeModelscall has a 15 scontext.WithTimeout, preventing an infinite hang per provider, but the timeouts are additive across the sequential loop. - Proposed mitigation: Run the
OpenCodeModelscalls concurrently using anerrgroup.Group(or goroutines + a sync mechanism), then collect and flatten results. Preserve the per-call 15 s timeout but run them in parallel so wall-clock time is max(one call) rather than sum(all calls). - Alternative mitigations considered: Cache model lists with a short TTL (e.g. 5 min) so repeated
/orchinvocations skip the subprocess calls — but this adds cache-invalidation complexity and doesn't help the first call. Parallel execution is simpler and addresses the root cause. - Severity calibration: Score 3 because this is a latency regression under plausible but not typical load (3+ providers, slow opencode). Under normal conditions with 1–2 providers it adds only 1–6 s, which is noticeable but not blocking. It would become score 4+ if the provider count grew or if this were a server request path rather than an interactive CLI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1786-1800
Comment:
**Sequential `opencode models` subprocess calls block the interactive picker**
`buildOpenCodeModelEntries` loops over `active` providers and calls `agent.OpenCodeModels` for each one sequentially. Each call spawns an `opencode models <provider>` subprocess with a 15-second timeout. With N enabled providers, the caller blocks for up to N×15 s before the picker renders. The `/orch` command already prints 'Querying opencode models for your enabled providers…' and then freezes the TUI while these subprocesses run. Under normal conditions (1–3 s per call, 3–4 providers) this adds 3–12 s of latency; if opencode is slow or a provider is unreachable, it can approach the full 60 s timeout budget. The fix is to fan out the `OpenCodeModels` calls concurrently (e.g. `errgroup` with a semaphore) and collect results, cutting wall-clock time to roughly one round-trip instead of N.
Threat model:
Interactive CLI user types `/orch` or `/opencode` and experiences multi-second to multi-tens-of-seconds freeze before the picker appears.
Specific code citations:
`buildOpenCodeModelEntries` in `internal/repl/repl.go` lines 1786–1806: the `for _, p := range active` loop calling `agent.OpenCodeModels(bin, path, env, p.ID)` sequentially. `OpenCodeModels` in `internal/agent/opencode_config.go` lines 186–207: spawns `exec.CommandContext` with 15 s timeout per provider.
Existing protections:
Each `OpenCodeModels` call has a 15 s `context.WithTimeout`, preventing an infinite hang per provider, but the timeouts are additive across the sequential loop.
Proposed mitigation:
Run the `OpenCodeModels` calls concurrently using an `errgroup.Group` (or goroutines + a sync mechanism), then collect and flatten results. Preserve the per-call 15 s timeout but run them in parallel so wall-clock time is max(one call) rather than sum(all calls).
Alternative mitigations considered:
Cache model lists with a short TTL (e.g. 5 min) so repeated `/orch` invocations skip the subprocess calls — but this adds cache-invalidation complexity and doesn't help the first call. Parallel execution is simpler and addresses the root cause.
Severity calibration:
Score 3 because this is a latency regression under plausible but not typical load (3+ providers, slow opencode). Under normal conditions with 1–2 providers it adds only 1–6 s, which is noticeable but not blocking. It would become score 4+ if the provider count grew or if this were a server request path rather than an interactive CLI.
Suggested fix shape:
var eg errgroup.Group
results := make([][]string, len(active))
for i, p := range active {
i, p := i, p
eg.Go(func() error {
results[i] = agent.OpenCodeModels(bin, path, env, p.ID)
return nil
})
}
eg.Wait()
for _, ms := range results {
for _, full := range ms {
// ... build entries
}
}
How can I resolve this? If you propose a fix, please make it concise.
…enabling a provider
When a provider is enabled via /providers, qmax now manages its key in the OS
keychain (injected at launch). Detect any plaintext apiKey still sitting in the
user's own ~/.config/opencode config ({env:}/{file:} refs are safe) and warn
them to rotate/remove it. Detector respects XDG_CONFIG_HOME (opencode's path,
not os.UserConfigDir). Tested against no-config, {env:} reference, and a literal key.
Comment on lines
+1791
to
+1806
| func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) []tui.OpenCodeModelEntry { | ||
| bin := agent.FindOpenCode() | ||
| if bin == "" { | ||
| return nil | ||
| } | ||
| active := cfg.ActiveProviders() | ||
| if len(active) == 0 { | ||
| return nil | ||
| } | ||
| path, err := agent.WriteOpenCodeConfig(cfg, sctx, cfg.OrchPermissionMode) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| // Provider keys must be present in the env or `opencode models <provider>` | ||
| // reports "Provider not found" for known providers (groq/openrouter). | ||
| env := agent.OpenCodeProviderEnv(cfg) |
There was a problem hiding this comment.
Sequential subprocess fan-out in buildOpenCodeModelEntries delays model picker
buildOpenCodeModelEntries calls agent.OpenCodeModels sequentially for each active provider, and each call spawns an opencode models <provider> subprocess with a 15-second timeout. With N providers, worst-case latency before the /orch picker appears is N × 15s. The calls are independent — they read different provider model lists — and could run concurrently. This also affects the /opencode quick-switch path via resolveOpenCodeModelOverride, which calls the same function.
More Info
- Threat model: A user with multiple enabled providers waits for all
opencode modelssubprocess calls to complete sequentially before the model picker renders. If any provider endpoint is slow or hung, the delay stacks across all subsequent providers. - Specific code citations:
for _, p := range activeloop inbuildOpenCodeModelEntries(repl.go ~L1806) callsagent.OpenCodeModels(bin, path, env, p.ID)per provider.OpenCodeModels(opencode_config.go) runsexec.CommandContextwith a 15s timeout per call. - Existing protections: The 15s
context.WithTimeoutinOpenCodeModelscaps each individual call, but the sequential loop means timeouts stack multiplicatively rather than being bounded by a single deadline. - Proposed mitigation: Run the
OpenCodeModelscalls concurrently using a goroutine per provider with async.WaitGroupandsync.Mutex-protected results slice, bounded by a single sharedcontext.WithTimeoutmatching the 15s cap. - Alternative mitigations considered: Cache model lists with a short TTL so repeated picker opens skip subprocess calls entirely. This eliminates subprocess overhead on repeated
/orchinvocations but adds cache-invalidation complexity. - Severity calibration: Score 3 because typical provider counts are small (1–3) and subprocess calls are usually fast (<1s), but a slow or hung provider endpoint can cause multi-second delays that stack sequentially. Not production-down, but a meaningful latency regression under plausible conditions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1791-1806
Comment:
**Sequential subprocess fan-out in `buildOpenCodeModelEntries` delays model picker**
`buildOpenCodeModelEntries` calls `agent.OpenCodeModels` sequentially for each active provider, and each call spawns an `opencode models <provider>` subprocess with a 15-second timeout. With N providers, worst-case latency before the `/orch` picker appears is N × 15s. The calls are independent — they read different provider model lists — and could run concurrently. This also affects the `/opencode` quick-switch path via `resolveOpenCodeModelOverride`, which calls the same function.
Threat model:
A user with multiple enabled providers waits for all `opencode models` subprocess calls to complete sequentially before the model picker renders. If any provider endpoint is slow or hung, the delay stacks across all subsequent providers.
Specific code citations:
`for _, p := range active` loop in `buildOpenCodeModelEntries` (repl.go ~L1806) calls `agent.OpenCodeModels(bin, path, env, p.ID)` per provider. `OpenCodeModels` (opencode_config.go) runs `exec.CommandContext` with a 15s timeout per call.
Existing protections:
The 15s `context.WithTimeout` in `OpenCodeModels` caps each individual call, but the sequential loop means timeouts stack multiplicatively rather than being bounded by a single deadline.
Proposed mitigation:
Run the `OpenCodeModels` calls concurrently using a goroutine per provider with a `sync.WaitGroup` and `sync.Mutex`-protected results slice, bounded by a single shared `context.WithTimeout` matching the 15s cap.
Alternative mitigations considered:
Cache model lists with a short TTL so repeated picker opens skip subprocess calls entirely. This eliminates subprocess overhead on repeated `/orch` invocations but adds cache-invalidation complexity.
Severity calibration:
Score 3 because typical provider counts are small (1–3) and subprocess calls are usually fast (<1s), but a slow or hung provider endpoint can cause multi-second delays that stack sequentially. Not production-down, but a meaningful latency regression under plausible conditions.
How can I resolve this? If you propose a fix, please make it concise.
| // It queries `opencode models <provider>` at call time so the list is live | ||
| // (models.dev-backed for Groq/OpenRouter, seeded config for custom providers). | ||
| // Returns nil when opencode isn't installed or no providers are active. | ||
| func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) []tui.OpenCodeModelEntry { |
There was a problem hiding this comment.
New /providers command and opencode backend switching in REPL ship without tests
The REPL gains a new /providers command (list, enable, disable) and opencode backend switching logic (/opencode, /orch picker integration, pre-flight validation) — all non-trivial runtime behavior with multiple branches (missing CLI, no providers, stale model override, consent gate, key validation, config regeneration). None of this is covered by a test. The [TESTING EVIDENCE] block shows siblingMapping:internal/repl/repl.go->NONE — no sibling test file exists in this PR. The handleProviders, enableProvider, disableProvider, buildOpenCodeModelEntries, and resolveOpenCodeModelOverride functions all contain reachable error paths and state transitions that can regress silently.
More Info
- Threat model: A regression in the provider enable/disable flow (e.g., a key saved but the provider not marked enabled, or the managed config not regenerated) would silently break the opencode backend for users. A regression in the pre-flight validation (stale model override from another backend passed to opencode) would cause opencode to fail with an unrecognized model. Without tests, these regressions are invisible until a user hits them.
- Specific code citations:
handleProviders(line ~1783),enableProvider(line ~1895),disableProvider(line ~1950),buildOpenCodeModelEntries(line ~1793),resolveOpenCodeModelOverride(line ~1833), and the/opencodepre-flight block (line ~630). - Existing protections: The config-generation layer (
WriteOpenCodeConfig) and provider registry (internal/api/providers.go) have unit tests, but the REPL orchestration that wires them together — the command parsing, key prompting, config save, and config regeneration — has no test coverage. - Proposed mitigation: Add a table-driven test for
handleProviders(or extract the enable/disable logic into a testable function) covering: enabling a provider with a valid key, enabling with an invalid key, enabling an unknown provider, enabling an unentitled provider, disabling a provider, and the plaintext-key warning path. Add a test forresolveOpenCodeModelOverridecovering: stale override from another backend, valid override, and no models available. - Alternative mitigations considered: An integration test that exercises the full
/providers enable groqflow end-to-end would be ideal but requires a keychain mock. Unit-testing the extracted logic is the smallest practical step. - Severity calibration: Score 4: the new REPL commands are user-facing, reachable, and contain multiple branches and error paths. A regression here directly breaks the opencode backend for users. The fix is adding tests, not changing production behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1791
Comment:
**New `/providers` command and opencode backend switching in REPL ship without tests**
The REPL gains a new `/providers` command (list, enable, disable) and opencode backend switching logic (`/opencode`, `/orch` picker integration, pre-flight validation) — all non-trivial runtime behavior with multiple branches (missing CLI, no providers, stale model override, consent gate, key validation, config regeneration). None of this is covered by a test. The `[TESTING EVIDENCE]` block shows `siblingMapping:internal/repl/repl.go->NONE` — no sibling test file exists in this PR. The `handleProviders`, `enableProvider`, `disableProvider`, `buildOpenCodeModelEntries`, and `resolveOpenCodeModelOverride` functions all contain reachable error paths and state transitions that can regress silently.
Threat model:
A regression in the provider enable/disable flow (e.g., a key saved but the provider not marked enabled, or the managed config not regenerated) would silently break the opencode backend for users. A regression in the pre-flight validation (stale model override from another backend passed to opencode) would cause opencode to fail with an unrecognized model. Without tests, these regressions are invisible until a user hits them.
Specific code citations:
`handleProviders` (line ~1783), `enableProvider` (line ~1895), `disableProvider` (line ~1950), `buildOpenCodeModelEntries` (line ~1793), `resolveOpenCodeModelOverride` (line ~1833), and the `/opencode` pre-flight block (line ~630).
Existing protections:
The config-generation layer (`WriteOpenCodeConfig`) and provider registry (`internal/api/providers.go`) have unit tests, but the REPL orchestration that wires them together — the command parsing, key prompting, config save, and config regeneration — has no test coverage.
Proposed mitigation:
Add a table-driven test for `handleProviders` (or extract the enable/disable logic into a testable function) covering: enabling a provider with a valid key, enabling with an invalid key, enabling an unknown provider, enabling an unentitled provider, disabling a provider, and the plaintext-key warning path. Add a test for `resolveOpenCodeModelOverride` covering: stale override from another backend, valid override, and no models available.
Alternative mitigations considered:
An integration test that exercises the full `/providers enable groq` flow end-to-end would be ideal but requires a keychain mock. Unit-testing the extracted logic is the smallest practical step.
Severity calibration:
Score 4: the new REPL commands are user-facing, reachable, and contain multiple branches and error paths. A regression here directly breaks the opencode backend for users. The fix is adding tests, not changing production behavior.
How can I resolve this? If you propose a fix, please make it concise.
| } | ||
| models = append(models, line) | ||
| } | ||
| return models |
There was a problem hiding this comment.
OpenCodeModels subprocess call has no test for the error path (timeout, non-zero exit, empty output)
OpenCodeModels shells out to opencode models <provider> with a 15-second timeout. The existing test (TestOpenCodeModelsPassesProviderEnv) only covers the happy path where the stub binary prints a model line. The error paths — subprocess timeout, non-zero exit (e.g., provider not found, config parse error), and empty output — are untested. A regression in error handling (e.g., swallowing a real failure and returning nil silently) would make the model picker appear empty with no diagnostic.
More Info
- Threat model: If
opencode modelsfails (timeout, config error, provider not found),OpenCodeModelsreturns nil. The caller (buildOpenCodeModelEntries) treats nil as 'no models' and the picker shows an empty opencode section. The user sees no models and no error — a silent failure. - Specific code citations:
OpenCodeModels(line ~230-263):cmd.Run()error path returns nil; timeout viacontext.WithTimeoutreturns nil; empty output returns an empty slice. - Existing protections:
TestOpenCodeModelsPassesProviderEnvtests the happy path with a stub binary. No test covers the timeout, non-zero exit, or empty-output paths. - Proposed mitigation: Add test cases for: (1) stub binary that exits non-zero → expect nil; (2) stub binary that sleeps longer than the test timeout → expect nil; (3) stub binary that prints no matching lines → expect empty slice.
- Alternative mitigations considered: Could also test at the
buildOpenCodeModelEntrieslevel, but unit-testingOpenCodeModelsdirectly is simpler and more targeted. - Severity calibration: Score 3: the error paths are reachable (network issues, config errors, provider key problems) and the silent-failure behavior is a real UX gap, but the blast radius is limited to the model picker appearing empty rather than a crash or data loss.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_config.go
Line: 262
Comment:
**`OpenCodeModels` subprocess call has no test for the error path (timeout, non-zero exit, empty output)**
`OpenCodeModels` shells out to `opencode models <provider>` with a 15-second timeout. The existing test (`TestOpenCodeModelsPassesProviderEnv`) only covers the happy path where the stub binary prints a model line. The error paths — subprocess timeout, non-zero exit (e.g., provider not found, config parse error), and empty output — are untested. A regression in error handling (e.g., swallowing a real failure and returning nil silently) would make the model picker appear empty with no diagnostic.
Threat model:
If `opencode models` fails (timeout, config error, provider not found), `OpenCodeModels` returns nil. The caller (`buildOpenCodeModelEntries`) treats nil as 'no models' and the picker shows an empty opencode section. The user sees no models and no error — a silent failure.
Specific code citations:
`OpenCodeModels` (line ~230-263): `cmd.Run()` error path returns nil; timeout via `context.WithTimeout` returns nil; empty output returns an empty slice.
Existing protections:
`TestOpenCodeModelsPassesProviderEnv` tests the happy path with a stub binary. No test covers the timeout, non-zero exit, or empty-output paths.
Proposed mitigation:
Add test cases for: (1) stub binary that exits non-zero → expect nil; (2) stub binary that sleeps longer than the test timeout → expect nil; (3) stub binary that prints no matching lines → expect empty slice.
Alternative mitigations considered:
Could also test at the `buildOpenCodeModelEntries` level, but unit-testing `OpenCodeModels` directly is simpler and more targeted.
Severity calibration:
Score 3: the error paths are reachable (network issues, config errors, provider key problems) and the silent-failure behavior is a real UX gap, but the blast radius is limited to the model picker appearing empty rather than a crash or data loss.
How can I resolve this? If you propose a fix, please make it concise.
Desperado
added a commit
that referenced
this pull request
Jul 13, 2026
) * fix(skills): support opencode backend so /orch activation stops failing The opencode backend (1.20.8, #141) routes skill install through the generic InstallSkillsReport(result.Backend) call in the /orch picker, but the skills materializer only knew about "cc" and "codex". Activating opencode crashed with: skills: unknown backend "opencode". Add opencode as a first-class skills backend: - BackendOpenCode writes to ~/.config/opencode/skills/<name>/SKILL.md (opencode's global discovery dir), rendering name+description only — opencode ignores allowed-tools and has no openai.yaml sibling. - main.go: sync skills on every opencode activation (idempotent), matching the cc/codex startup sync. opencode has no global-install prompt, so this is unconditional. - InstallSkillsBoth -> InstallSkillsAll (now three backends); /skills status gains an "oc" column. - Tests: opencode materialization + a guard that all catalog names satisfy opencode's stricter name regex (no underscores). * fix(orch): sync opencode skills on /orch activation regardless of GlobalInstall The /orch picker gated InstallSkillsReport behind consent.GlobalInstall, which opencode never sets (its consent flow skips the global-install prompt). So opencode-only users activating via /orch got no skills, even though WriteOpenCodeConfig already ran unconditionally in the same flow and main.go syncs opencode skills on every startup. Decouple the skills install from the GlobalInstall gate for opencode only: cc/codex keep their explicit opt-in; opencode matches its managed-config model (and main.go). RunOrch stays gated — IsOrchInstalled("opencode") already returns true, so it never runs for opencode anyway. * test(skills): cover unknown-backend path + opencode name-regex negatives; surface startup error Address review (sigilix): - TestSkillsDirRejectsUnknownBackend: assert invalid backends error and that Materialize propagates it (guards the default branch). - TestOpenCodeNameRegexRejectsInvalid: underscores, leading/trailing/double hyphens, uppercase, empty all rejected — confirms the regex itself. - main.go: log the opencode InstallSkills error to stderr instead of discarding it, matching the adjacent WriteOpenCodeConfig warning.
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
1 participant
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.
What
Adds opencode as a fifth inference backend, driven as a subprocess CLI just like the existing
cc/codexbackends. It inherits opencode's broad provider support — but providers are gated per-user: nothing is enabled for everyone. Each user turns providers on individually via a new/providerscommand, and only their enabled providers show up in their/orchmodel picker.Requested: make qmax-code work on the opencode harness, and let users selectively enable extra providers/models (Z.AI coding plan, Groq, Cerebras, OpenRouter) individually rather than for everyone.
Design decisions
cc/codex). opencode supports native session resume + rich NDJSON, so it mirrors CCAgent, not Codex's self-managed history.ProviderAllowed()entitlement seam so a QualityMax cloud check can gate it later with no UI change. Visible =entitled ∧ enabled.~/.qmax-code/opencode.json, pointed at the subprocess viaOPENCODE_CONFIG— opencode merges it on top of the user's ownopencode.jsonc, never clobbering it.{env:VAR}, and the real key is injected into the subprocess env at launch.opencode models <provider>, filtered to enabled+entitled providers./providersfor discoverability only.New surface
/providers— list opt-in providers;/providers enable <id>(prompts + validates key),/providers disable <id>./opencode— switch to the opencode backend; opencode models appear as a new section in/orch.qmax-code config set backend opencode;--backend opencode.Files
New:
internal/agent/opencode_agent.go(OpenCodeAgent/CLIAgent, NDJSON parse, session resume),internal/agent/opencode_config.go(managed config + MCP + dynamic model listing),internal/api/providers.go(registry + keychain + entitlement seam), plus tests.Edited:
main.go(flag + routing + construction),internal/repl/repl.go(picker wiring,/providers,/opencode,/clearreset, help),internal/tui/tui_backend.go(opencode picker section + dynamic entries),internal/tui/input.go(slash menu),internal/setup/consent.go(opencode consent, skips external global-install),config_command.go,internal/api/config.go(EnabledProviders).Testing
go build ./... && go vet ./... && go test ./...— all green.mcplocal server +providerblock) andopencode models <provider>lists models through it; config CLI round-trip verified.Notes / follow-ups
/providersshow an install hint if the binary is missing.opencode modelsreturns the live list where models.dev knows the provider.--variantmapping is intentionally omitted in v1 (provider-specific values); effort still drives the QA system-prompt directive.