Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 164 additions & 17 deletions internal/agent/opencode_agent.go
Original file line number Diff line number Diff line change
@@ -1,559 +1,706 @@
package agent

import (
"bufio"
"context"
"encoding/json"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"regexp"
"runtime"
"strconv"
"strings"
"sync"
"time"

"github.com/qualitymax/qmax-code/internal/api"
"github.com/qualitymax/qmax-code/internal/tui"
)

// OpenCodeAgent orchestrates an opencode CLI subprocess for LLM inference.
// Inference runs through whichever provider the user opted into (Z.AI, Groq,
// OpenRouter, …) using the user's own key — qmax-code consumes no QM-held
// tokens. qmax tools are exposed to opencode via an MCP server entry in the
// managed opencode config, so opencode can call them natively.
//
// opencode supports native session resume (--session) and a rich NDJSON event
// stream (opencode run --format json), so it retains its provider session ID
// much like CodexAgent retains its exact native thread checkpoint.
//
// Per-message flow:
// 1. qmax-code writes ~/.qmax-code/opencode.json (provider blocks + qmax MCP)
// 2. qmax-code spawns: opencode run --format json --model <provider>/<model>
// [--session <id>] [--auto] -- "msg" with OPENCODE_CONFIG + key env set
// 3. opencode picks up the MCP config and spawns qmax-code serve --mcp
// 4. opencode runs the turn on the user's provider, using qmax tools via MCP
// 5. qmax-code parses opencode's NDJSON and renders it; session id → --session
type OpenCodeAgent struct {
openCodeBin string
modelID string // "provider/model"; "" lets opencode use its default
effort string // "low" | "medium" | "high"
outputVerbose bool
permissionMode string // "standard" | "unattended" (--auto)
sessionID string // opencode session id, for --session resume
cfg *api.Config
sctx *api.SessionContext
lastToolName string
fileSnaps map[string]fileSnapshot // opencode tool part id → pre-edit snapshot
lastTurnIn int // token usage of the most recent turn (from opencode's stream)
lastTurnOut int
lastTurnOK bool // true once a usage event carried tokens this turn
lastLimitHit bool // true if the plan limit was hit this turn
lastLimitReset time.Time // provider-reported reset time, zero if unknown
// lastOCErrorSeen/Msg capture the most recent error event of the current
// run attempt — even the status-code-less ones handleOCError treats as
// benign noise on successful turns. When a turn dies with no result, they
// are the only in-band clue to the real cause (e.g. a provider entitlement
// refusal that opencode masks as "Unexpected server error").
lastOCErrorSeen bool
lastOCErrorMsg string
// lastStderrTail holds the tail of the subprocess stderr for the current
// run attempt, so a hard crash that emits no stream events (opencode
// internal JS TypeError) can still be explained in the returned error.
lastStderrTail string
mu sync.Mutex
runMu sync.Mutex
runCancel context.CancelFunc // non-nil while Run() is active
}

// FindOpenCode returns the path to the opencode CLI binary, or "" if not found.
func FindOpenCode() string {
if path, err := exec.LookPath("opencode"); err == nil {
return path
}
for _, p := range []string{
filepath.Join(os.Getenv("HOME"), ".opencode/bin/opencode"),
"/usr/local/bin/opencode",
"/opt/homebrew/bin/opencode",
filepath.Join(os.Getenv("HOME"), ".local/bin/opencode"),
} {
if _, err := os.Stat(p); err == nil {
return p
}
}
return ""
}

// autoFlag caches the one-time probe for `opencode run --auto` support so we
// don't shell out to `--help` on every turn.
var (
autoFlagOnce sync.Once
autoFlagSupported bool
)

// openCodeSupportsAutoFlag reports whether the installed opencode accepts the
// `run --auto` flag. Older opencode used --auto to auto-approve tool calls in
// non-interactive `run` mode; opencode 1.x removed it and governs approvals
// through the config `permission` block instead. Passing --auto to a version
// that no longer knows it makes opencode print usage and exit 1 with no output,
// so probe `run --help` once and only pass the flag when it is advertised.
func openCodeSupportsAutoFlag(bin string) bool {
if bin == "" {
return false
}
autoFlagOnce.Do(func() {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
out, _ := exec.CommandContext(ctx, bin, "run", "--help").CombinedOutput()
autoFlagSupported = strings.Contains(string(out), "--auto")
})
return autoFlagSupported
}

// NewOpenCodeAgent creates an opencode subprocess orchestrator.
// modelID is the full "provider/model" string selected via the picker.
// effort is "low" | "medium" | "high" (empty defaults to "high").
// permissionMode is "standard" or "unattended" (adds --auto).
func NewOpenCodeAgent(bin, modelID, effort, permissionMode string, outputVerbose bool, cfg *api.Config, sctx *api.SessionContext) *OpenCodeAgent {
if effort == "" {
effort = "high"
}
if permissionMode == "" {
permissionMode = "standard"
}
return &OpenCodeAgent{
openCodeBin: bin,
modelID: modelID,
effort: effort,
outputVerbose: outputVerbose,
permissionMode: permissionMode,
cfg: cfg,
sctx: sctx,
}
}

// validOpenCodeSessionID guards the --session argument. opencode session ids
// look like "ses_0a91c2141ffe8FiFOZVFulDUUM": a "ses_" prefix followed by
// alphanumeric characters.
func validOpenCodeSessionID(id string) bool {
if !strings.HasPrefix(id, "ses_") || len(id) > 64 {
return false
}
rest := id[len("ses_"):]
if rest == "" {
return false
}
for _, r := range rest {
if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9')) {
return false
}
}
return true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 LOGICGROUNDED Race condition between parseStream completion and error classification in Run

The Run method spawns parseStream as a goroutine and later reads a.lastRunError() to decide whether the failure was a provider refusal (deterministic, no retry) or an internal crash (retry once). The goroutine is not waited on; runAttempt returns after cmd.Wait(), but the stream reader may still be processing buffered data. This creates a race: lastOCErrorSeen/lastOCErrorMsg may still be unset when Run checks them, causing a provider refusal to be misclassified as a crash, leading to an unwanted retry and a misleading error message.

Detailed reasoning

The fix is to have runAttempt wait for the parseStream goroutine to finish before returning, e.g., via a channel, so that the error-event state is guaranteed to be final when the caller inspects it.

More Info
  • Threat model: When the opencode subprocess exits with an error event near the end of the stream, the main goroutine may reach the lastRunError check before the parseStream goroutine has processed the final bytes, leading to a false negative on the provider refusal case.
  • Specific code citations: go a.parseStream(stdout, term) in runAttempt (line ~260); a.lastRunError() check in Run (line ~187); a.lastOCErrorSeen, a.lastOCErrorMsg set inside parseStream.
  • Existing protections: The fields are protected by a mutex, but that only prevents concurrent access, not the ordering guarantee. The mutex does not synchronize with the goroutine's completion.
  • Proposed mitigation: In runAttempt, use a channel to signal the goroutine's completion, and wait for it before returning. This ensures that lastOCErrorSeen and lastOCErrorMsg are final when the caller reads them.
  • Alternative mitigations considered: A channel-based signal from parseStream is the simplest; a WaitGroup is also appropriate. Relying on a short sleep would be fragile.
  • Severity calibration: A provider refusal could be retried, violating the no-retry policy and possibly causing a confusing error message. The race is window-dependent but plausible in production. Score 4 reflects a likely bug under normal scheduling.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 153

Comment:
**Race condition between parseStream completion and error classification in Run**

The `Run` method spawns `parseStream` as a goroutine and later reads `a.lastRunError()` to decide whether the failure was a provider refusal (deterministic, no retry) or an internal crash (retry once). The goroutine is not waited on; `runAttempt` returns after `cmd.Wait()`, but the stream reader may still be processing buffered data. This creates a race: `lastOCErrorSeen`/`lastOCErrorMsg` may still be unset when `Run` checks them, causing a provider refusal to be misclassified as a crash, leading to an unwanted retry and a misleading error message.

The fix is to have `runAttempt` wait for the `parseStream` goroutine to finish before returning, e.g., via a channel, so that the error-event state is guaranteed to be final when the caller inspects it.

Threat model:
When the opencode subprocess exits with an error event near the end of the stream, the main goroutine may reach the `lastRunError` check before the parseStream goroutine has processed the final bytes, leading to a false negative on the provider refusal case.

Specific code citations:
`go a.parseStream(stdout, term)` in runAttempt (line ~260); `a.lastRunError()` check in Run (line ~187); `a.lastOCErrorSeen`, `a.lastOCErrorMsg` set inside parseStream.

Existing protections:
The fields are protected by a mutex, but that only prevents concurrent access, not the ordering guarantee. The mutex does not synchronize with the goroutine's completion.

Proposed mitigation:
In `runAttempt`, use a channel to signal the goroutine's completion, and wait for it before returning. This ensures that `lastOCErrorSeen` and `lastOCErrorMsg` are final when the caller reads them.

Alternative mitigations considered:
A channel-based signal from parseStream is the simplest; a WaitGroup is also appropriate. Relying on a short sleep would be fragile.

Severity calibration:
A provider refusal could be retried, violating the no-retry policy and possibly causing a confusing error message. The race is window-dependent but plausible in production. Score 4 reflects a likely bug under normal scheduling.

How can I resolve this? If you propose a fix, please make it concise.

}

// Run executes one conversation turn through an opencode subprocess.
// Run executes one conversation turn through an opencode subprocess. When the
// subprocess dies with no result, it distinguishes two failure classes:
//
// - A provider error event was in the stream (auth, entitlement refusal,
// quota) — deterministic, so the real message is surfaced immediately
// instead of a bare "exit status 1". opencode masks these as a
// status-code-less "Unexpected server error", with the full text only in
// its own log, which the error message points at.
// - No error event at all — opencode itself crashed (e.g. an internal JS
// TypeError mid-stream). Transient, so the turn is retried once before
// giving up with the stderr tail included.
func (a *OpenCodeAgent) Run(userMsg string, term *tui.Terminal) (string, error) {
// Regenerate the managed config each turn so newly enabled/disabled
// providers (and the permission policy) take effect without a restart.
configPath, err := WriteOpenCodeConfig(a.cfg, a.sctx, a.permissionMode)
if err != nil {
return "", fmt.Errorf("opencode config: %w", err)
}

safeUserMsg, err := sanitizeCCUserPrompt(userMsg)
if err != nil {
return "", err
}

a.mu.Lock()
a.lastTurnIn, a.lastTurnOut, a.lastTurnOK = 0, 0, false
a.lastLimitHit, a.lastLimitReset = false, time.Time{}
sessionID := a.sessionID
a.mu.Unlock()

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
defer cancel()

a.runMu.Lock()
a.runCancel = cancel
a.runMu.Unlock()
defer func() {
a.runMu.Lock()
a.runCancel = nil
a.runMu.Unlock()
}()

result, err := a.runAttempt(ctx, safeUserMsg, configPath, term)
if err == nil {
return result, nil
}

// A provider refusal is deterministic — retrying would just repeat it.
// Surface the captured error-event message instead of the exit status.
if msg, seen := a.lastRunError(); seen {
return "", fmt.Errorf("opencode turn failed: %s (provider details: %s)", msg, openCodeLogPath())
}

// No provider error in the stream, yet the process died with no result:
// opencode itself crashed. One retry usually completes the turn.
term.PrintSystem("opencode exited unexpectedly — retrying turn…")
if result2, err2 := a.runAttempt(ctx, safeUserMsg, configPath, term); err2 == nil {
return result2, nil
}

msg, _ := a.lastRunError()
if msg == "" {
msg = "no error event in stream"
}
return "", fmt.Errorf("opencode turn failed: %s (stderr: %s; details: %s)", msg, a.stderrTailSnapshot(), openCodeLogPath())
}

// runAttempt spawns one `opencode run` subprocess for the turn, renders its
// NDJSON stream, and waits for exit. It returns a non-nil error only when the
// process failed AND produced no result text; a partial-result failure keeps
// the result, and an intentional cancel (ctx done) returns whatever streamed
// with a nil error.
func (a *OpenCodeAgent) runAttempt(ctx context.Context, safeUserMsg, configPath string, term *tui.Terminal) (string, error) {
a.mu.Lock()
a.lastOCErrorSeen, a.lastOCErrorMsg = false, ""
a.lastStderrTail = ""
// On the first turn of a session, prepend the QA system prompt + effort/output
// directives. opencode persists conversation state per session, so later turns
// resume via --session and don't need it re-injected.
// resume via --session and don't need it re-injected. A retry that already
// captured a session id re-evaluates this correctly.
message := safeUserMsg
if sessionID == "" {
if a.sessionID == "" {
message = cliQASystemPrompt(a.sctx, codexQASystemPrompt) + effortDirective(a.effort) + outputStyleDirective(a.outputVerbose) + "\n\n" + safeUserMsg
}
sessionID := a.sessionID
a.mu.Unlock()

args := []string{"run", "--format", "json"}
if a.modelID != "" {
args = append(args, "--model", a.modelID)
}
// --auto auto-approves anything not explicitly denied. In standard mode the
// managed config denies edits + destructive shell (openCodeStandardPermission),
// so --auto is safe there too; unattended has no denies (full autonomy).
// Older opencode needed --auto because `opencode run` is non-interactive —
// without it, tools that would prompt simply block. Newer opencode (1.x)
// REMOVED --auto and governs approvals purely through the config `permission`
// block; passing --auto there makes opencode print usage and exit 1 with no
// output. So only add it when the installed opencode still advertises it.
if openCodeSupportsAutoFlag(a.openCodeBin) {
args = append(args, "--auto")
}
if sessionID != "" && validOpenCodeSessionID(sessionID) {
args = append(args, "--session", sessionID)
}
// "--" terminates flag parsing so a message starting with "-" is treated as
// the positional prompt rather than an unknown flag. On Windows, opencode is
// typically an npm ".cmd" shim; Go's os/exec routes it through cmd.exe, which
// swallows the "--" separator and drops the positional message after it
// ("You must provide a message or a command"). There we pass the message with
// no "--" — sanitizeCCUserPrompt already stripped control bytes, and a lone
// positional is taken as the message.
if runtime.GOOS == "windows" {
args = append(args, message)
} else {
args = append(args, "--", message)
}

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
defer cancel()

a.runMu.Lock()
a.runCancel = cancel
a.runMu.Unlock()
defer func() {
a.runMu.Lock()
a.runCancel = nil
a.runMu.Unlock()
}()

cmd := exec.CommandContext(ctx, a.openCodeBin, args...)
cmd.Stdin = strings.NewReader("")
cmd.Stderr = term.Stderr()
tail := &stderrTailBuffer{}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 SECURITYGROUNDED Stderr tail redaction may leak secrets if opencode prints them in unexpected formats.

The redactStderrTail function applies a fixed set of regex patterns; if opencode outputs a credential in a format not covered (e.g., X-API-Key: secret, token=...), it will not be redacted and could appear in the error message shown to the user or logged.

More Info
  • Threat model: A crashing opencode subprocess might dump environment variables or debug info containing API keys in an unrecognized format, leading to secret exposure in the TUI or logs.
  • Specific code citations: Lines 333-342 define the patterns; line 271 calls redactStderrTail. The patterns may not cover all possible secret formats.
  • Existing protections: The tail is limited to 500 chars and only appears in error messages from a failed turn. opencode itself may not print secrets.
  • Proposed mitigation: Add more comprehensive patterns, especially for common env var formats (KEY=value). Consider redacting any line containing key, token, secret, password followed by an equals sign and a high-entropy string.
  • Alternative mitigations considered: Discard stderr entirely for crash cases; but then diagnostic info is lost.
  • Severity calibration: Score 2 because it's a low-probability hardening gap; opencode is unlikely to print secrets, but if it does, the impact is moderate (secret in error message).
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 276

Comment:
**Stderr tail redaction may leak secrets if opencode prints them in unexpected formats.**

The `redactStderrTail` function applies a fixed set of regex patterns; if opencode outputs a credential in a format not covered (e.g., `X-API-Key: secret`, `token=...`), it will not be redacted and could appear in the error message shown to the user or logged.

Threat model:
A crashing opencode subprocess might dump environment variables or debug info containing API keys in an unrecognized format, leading to secret exposure in the TUI or logs.

Specific code citations:
Lines 333-342 define the patterns; line 271 calls `redactStderrTail`. The patterns may not cover all possible secret formats.

Existing protections:
The tail is limited to 500 chars and only appears in error messages from a failed turn. opencode itself may not print secrets.

Proposed mitigation:
Add more comprehensive patterns, especially for common env var formats (`KEY=value`). Consider redacting any line containing `key`, `token`, `secret`, `password` followed by an equals sign and a high-entropy string.

Alternative mitigations considered:
Discard stderr entirely for crash cases; but then diagnostic info is lost.

Severity calibration:
Score 2 because it's a low-probability hardening gap; opencode is unlikely to print secrets, but if it does, the impact is moderate (secret in error message).

How can I resolve this? If you propose a fix, please make it concise.

cmd.Stderr = io.MultiWriter(term.Stderr(), tail)
cmd.Env = append(os.Environ(), "OPENCODE_CONFIG="+configPath)
for k, v := range OpenCodeProviderEnv(a.cfg) {
cmd.Env = append(cmd.Env, k+"="+v)
}

stdout, err := cmd.StdoutPipe()
if err != nil {
return "", fmt.Errorf("stdout pipe: %w", err)
}
if err := cmd.Start(); err != nil {
return "", fmt.Errorf("start opencode: %w", err)
}

result := a.parseStream(stdout, term)

if err := cmd.Wait(); err != nil {
// Intentional cancel (user pressed Enter to interrupt) — not an error.
if ctx.Err() != nil {
return result, nil
}
if result == "" {
a.mu.Lock()
a.lastStderrTail = tail.String()
a.mu.Unlock()
return "", fmt.Errorf("opencode exited with error: %w", err)
}
}
return result, nil
}

// lastRunError returns the message of the most recent error event captured by
// parseStream for the current run attempt.
func (a *OpenCodeAgent) lastRunError() (string, bool) {
a.mu.Lock()
defer a.mu.Unlock()
return a.lastOCErrorMsg, a.lastOCErrorSeen
}

// stderrTailSnapshot returns a bounded, redacted tail of the failed attempt's
// stderr so the returned error carries opencode's own crash output (e.g. a JS
// TypeError) instead of a bare exit status.
func (a *OpenCodeAgent) stderrTailSnapshot() string {
a.mu.Lock()
defer a.mu.Unlock()
s := strings.TrimSpace(a.lastStderrTail)
if len(s) > 500 {
s = "…" + s[len(s)-500:]
}
return redactStderrTail(s)
}

// stderrSecretPatterns match credential-shaped output a crashing subprocess
// could theoretically dump to stderr (env echo, auth debug lines). The tail
// ends up in a returned error, which lands in the TUI and logs — redact
// defensively rather than trust opencode never to print a key.
var stderrSecretPatterns = []struct {
re *regexp.Regexp
repl string
Comment on lines +330 to +335

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 SECURITYGROUNDED Credential redaction regex may miss edge cases, risking secret leakage in error logs.

The stderrSecretPatterns regex list aims to redact credentials from subprocess stderr before inclusion in error messages. However, the patterns are incomplete: they may miss non-standard credential formats, multi-line secrets, or credentials embedded in JSON/XML structures. An attacker who can induce a crash that prints a secret to stderr could bypass the redaction and leak it into the TUI or logs. This is a hardening gap (P2) because it requires the subprocess to already be leaking secrets and the attacker to have access to those logs.

Example:

stderr contains: `DEBUG: secret_key="sk_live_1234567890abcdef"`
The regex pattern for `api[_-]?key` does not match because of the underscore and equals sign.
Redacted output: `DEBUG: secret_key="sk_live_1234567890abcdef"` (secret leaked).

Suggested fix:

Add a more permissive pattern: `regexp.MustCompile(`(?i)(secret|key|token|password)[_\-]?key?\s*[=:]\s*\S+`)` and consider entropy-based detection for long random strings.
Detailed reasoning

The redactStderrTail function iterates over a fixed list of patterns; missing a pattern means the secret appears verbatim. The current patterns cover common shapes but are not exhaustive. For example, a secret like secret_key="abc123" may not match the api[_-]?key pattern, and a multi-line PEM-encoded key would be missed.

More Info
  • Threat model: An attacker who can control opencode's environment or inputs could cause a crash that prints a credential (e.g., from debug output) to stderr. The redacted stderr tail is included in error messages shown to the user and possibly logged. If the redaction misses the credential shape, the secret is leaked.
  • Specific code citations: Lines 335-345 define stderrSecretPatterns. The regex for api[_-]?key expects a space/colon after the keyword; secret_key="value" may not match. The JWT pattern (eyJ[A-Za-z0-9_\-.]{20,}) may miss compact JWTs or those with different padding.
  • Existing protections: The function attempts to redact known credential prefixes (AWS, GitHub tokens, JWTs). The test TestRedactStderrTailEdgeCases verifies some cases but does not guarantee completeness.
  • Proposed mitigation: Consider using a deny-list approach that redacts any high-entropy string (e.g., using Shannon entropy detection) in addition to pattern matching, or ensure the subprocess never logs secrets by configuring its log level.
  • Alternative mitigations considered: 1. Suppress stderr entirely for crashes, logging only to a secure audit log. 2. Use a library for secret detection (e.g., detect-secrets) to scan the tail. 3. Trust that opencode never logs secrets and remove the redaction (but this assumes upstream behavior).
  • Severity calibration: Score 3 (hardening gap) because exploitation requires: (1) opencode must leak a secret to stderr, (2) the leak must bypass the regex patterns, (3) the attacker must have access to the error output (TUI or logs). It's not a direct remote exploit but widens the attack surface.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 330-335

Comment:
**Credential redaction regex may miss edge cases, risking secret leakage in error logs.**

The `stderrSecretPatterns` regex list aims to redact credentials from subprocess stderr before inclusion in error messages. However, the patterns are incomplete: they may miss non-standard credential formats, multi-line secrets, or credentials embedded in JSON/XML structures. An attacker who can induce a crash that prints a secret to stderr could bypass the redaction and leak it into the TUI or logs. This is a hardening gap (P2) because it requires the subprocess to already be leaking secrets and the attacker to have access to those logs.

The `redactStderrTail` function iterates over a fixed list of patterns; missing a pattern means the secret appears verbatim. The current patterns cover common shapes but are not exhaustive. For example, a secret like `secret_key="abc123"` may not match the `api[_-]?key` pattern, and a multi-line PEM-encoded key would be missed.

Example:
stderr contains: `DEBUG: secret_key="sk_live_1234567890abcdef"`
The regex pattern for `api[_-]?key` does not match because of the underscore and equals sign.
Redacted output: `DEBUG: secret_key="sk_live_1234567890abcdef"` (secret leaked).

Threat model:
An attacker who can control opencode's environment or inputs could cause a crash that prints a credential (e.g., from debug output) to stderr. The redacted stderr tail is included in error messages shown to the user and possibly logged. If the redaction misses the credential shape, the secret is leaked.

Specific code citations:
Lines 335-345 define `stderrSecretPatterns`. The regex for `api[_-]?key` expects a space/colon after the keyword; `secret_key="value"` may not match. The JWT pattern (`eyJ[A-Za-z0-9_\-.]{20,}`) may miss compact JWTs or those with different padding.

Existing protections:
The function attempts to redact known credential prefixes (AWS, GitHub tokens, JWTs). The test `TestRedactStderrTailEdgeCases` verifies some cases but does not guarantee completeness.

Proposed mitigation:
Consider using a deny-list approach that redacts any high-entropy string (e.g., using Shannon entropy detection) in addition to pattern matching, or ensure the subprocess never logs secrets by configuring its log level.

Alternative mitigations considered:
1. Suppress stderr entirely for crashes, logging only to a secure audit log. 2. Use a library for secret detection (e.g., detect-secrets) to scan the tail. 3. Trust that opencode never logs secrets and remove the redaction (but this assumes upstream behavior).

Severity calibration:
Score 3 (hardening gap) because exploitation requires: (1) opencode must leak a secret to stderr, (2) the leak must bypass the regex patterns, (3) the attacker must have access to the error output (TUI or logs). It's not a direct remote exploit but widens the attack surface.

Suggested fix shape:
Add a more permissive pattern: `regexp.MustCompile(`(?i)(secret|key|token|password)[_\-]?key?\s*[=:]\s*\S+`)` and consider entropy-based detection for long random strings.

How can I resolve this? If you propose a fix, please make it concise.

}{
{regexp.MustCompile(`(?i)\b(api[_-]?key|token|secret|password|authorization)\b\s*[=:]\s*(bearer\s+)?\S+`), "${1}=<redacted>"},
{regexp.MustCompile(`(?i)\bbearer\s+\S+`), "bearer <redacted>"},
{regexp.MustCompile(`\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza|AKIA|ASIA)[A-Za-z0-9_\-]{16,}\b`), "<redacted>"},
{regexp.MustCompile(`\beyJ[A-Za-z0-9_\-.]{20,}\b`), "<redacted>"},
}

func redactStderrTail(s string) string {
for _, p := range stderrSecretPatterns {
s = p.re.ReplaceAllString(s, p.repl)
}
return s
}

// stderrTailBuffer keeps the last bytes written to it. One instance per run
// attempt; not safe for concurrent use.
type stderrTailBuffer struct{ buf []byte }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 SECURITYGROUNDED Subprocess stderr tail buffer retains sensitive error output in memory

The new stderrTailBuffer retains up to 8KB of subprocess stderr output in memory, which could include provider error messages containing API keys, tokens, or other secrets. While this improves diagnostics, it also increases the attack surface for memory inspection attacks.

More Info
  • Threat model: An attacker with memory access (local privilege escalation, core dump) could extract retained stderr containing provider secrets or sensitive error messages.
  • Specific code citations: stderrTailBuffer struct with buf []byte field; Write method appends up to stderrTailLimit (8KB); stderrTailSnapshot returns trimmed tail in error messages.
  • Existing protections: No explicit sanitization or redaction of secrets from stderr before retention.
  • Proposed mitigation: Consider truncating or redacting stderr output before storing, or clearing the buffer after use. Alternatively, store only a hash or fingerprint for crash detection.
  • Alternative mitigations considered: 1) Do not retain stderr at all, rely on external logs. 2) Retain only non-sensitive metadata (exit code, timestamp). 3) Encrypt the buffer in memory (complex).
  • Severity calibration: Score 3 (hardening gap) because exploitation requires memory access, but the retained data could include credentials. The blast radius is limited to the process memory.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 330

Comment:
**Subprocess stderr tail buffer retains sensitive error output in memory**

The new `stderrTailBuffer` retains up to 8KB of subprocess stderr output in memory, which could include provider error messages containing API keys, tokens, or other secrets. While this improves diagnostics, it also increases the attack surface for memory inspection attacks.

Threat model:
An attacker with memory access (local privilege escalation, core dump) could extract retained stderr containing provider secrets or sensitive error messages.

Specific code citations:
`stderrTailBuffer` struct with `buf []byte` field; `Write` method appends up to `stderrTailLimit` (8KB); `stderrTailSnapshot` returns trimmed tail in error messages.

Existing protections:
No explicit sanitization or redaction of secrets from stderr before retention.

Proposed mitigation:
Consider truncating or redacting stderr output before storing, or clearing the buffer after use. Alternatively, store only a hash or fingerprint for crash detection.

Alternative mitigations considered:
1) Do not retain stderr at all, rely on external logs. 2) Retain only non-sensitive metadata (exit code, timestamp). 3) Encrypt the buffer in memory (complex).

Severity calibration:
Score 3 (hardening gap) because exploitation requires memory access, but the retained data could include credentials. The blast radius is limited to the process memory.

How can I resolve this? If you propose a fix, please make it concise.


// stderrTailLimit bounds the retained stderr tail.
const stderrTailLimit = 8 << 10

func (s *stderrTailBuffer) Write(p []byte) (int, error) {
s.buf = append(s.buf, p...)
if len(s.buf) > stderrTailLimit {
s.buf = s.buf[len(s.buf)-stderrTailLimit:]
}
return len(p), nil
}

func (s *stderrTailBuffer) String() string { return string(s.buf) }

// openCodeLogPath returns opencode's own log file. The NDJSON stream carries
// only a generic "Unexpected server error" for provider refusions; the full
// text (entitlement message, provider status) lands here.
func openCodeLogPath() string {
if base := os.Getenv("XDG_DATA_HOME"); base != "" {
return filepath.Join(base, "opencode", "log", "opencode.log")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 SECURITYGROUNDED Log file path exposure in error messages could aid path traversal

The openCodeLogPath function returns opencode's log file path, which is included in error messages. While not a secret, revealing the exact path could assist an attacker in constructing path traversal attacks if the log file is accessible.

Example:

Error message: 'opencode turn failed: Unexpected server error (provider details: /home/user/.local/share/opencode/log/opencode.log)'

Suggested fix:

return 'opencode turn failed: Unexpected server error (see opencode log for details)'
More Info
  • Threat model: An attacker who can read error messages (e.g., via logs) learns the exact path to opencode's log file, which could be targeted for reading or writing if file permissions are weak.
  • Specific code citations: openCodeLogPath returns ~/.local/share/opencode/log/opencode.log or $XDG_DATA_HOME/opencode/log/opencode.log; included in error strings via opencode.log pointer.
  • Existing protections: The log file is presumably user-owned and not world-readable, but path disclosure still reduces attacker effort.
  • Proposed mitigation: Omit the full path from user-facing errors; use a generic message like 'see opencode log for details'.
  • Alternative mitigations considered: Keep the path for debugging but only log it at debug level, not in user-facing errors.
  • Severity calibration: Score 2 (small hardening improvement) because the path is predictable anyway (~/.local/share/opencode/log/opencode.log), but explicit disclosure slightly increases risk.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 350

Comment:
**Log file path exposure in error messages could aid path traversal**

The `openCodeLogPath` function returns opencode's log file path, which is included in error messages. While not a secret, revealing the exact path could assist an attacker in constructing path traversal attacks if the log file is accessible.

Example:
Error message: 'opencode turn failed: Unexpected server error (provider details: /home/user/.local/share/opencode/log/opencode.log)'

Threat model:
An attacker who can read error messages (e.g., via logs) learns the exact path to opencode's log file, which could be targeted for reading or writing if file permissions are weak.

Specific code citations:
`openCodeLogPath` returns `~/.local/share/opencode/log/opencode.log` or `$XDG_DATA_HOME/opencode/log/opencode.log`; included in error strings via `opencode.log` pointer.

Existing protections:
The log file is presumably user-owned and not world-readable, but path disclosure still reduces attacker effort.

Proposed mitigation:
Omit the full path from user-facing errors; use a generic message like 'see opencode log for details'.

Alternative mitigations considered:
Keep the path for debugging but only log it at debug level, not in user-facing errors.

Severity calibration:
Score 2 (small hardening improvement) because the path is predictable anyway (~/.local/share/opencode/log/opencode.log), but explicit disclosure slightly increases risk.

Suggested fix shape:
return 'opencode turn failed: Unexpected server error (see opencode log for details)'

How can I resolve this? If you propose a fix, please make it concise.

}
home, err := os.UserHomeDir()
if err != nil {
return "opencode.log"
}
return filepath.Join(home, ".local", "share", "opencode", "log", "opencode.log")
}

// --- NDJSON stream parsing ---

type ocEvent struct {
Type string `json:"type"`
Timestamp int64 `json:"timestamp"`
SessionID string `json:"sessionID"`
Part ocPart `json:"part"`
Error *ocError `json:"error,omitempty"`
// Token usage may appear at the top level of a completion/step event or on
// the message part; opencode/provider field names vary by version, so both
// the "tokens" and "usage" shapes are captured and whichever is populated
// wins. (Confirm exact names against a successful `opencode run --format
// json` sample; the time-based window works regardless of these.)
Tokens *ocTokens `json:"tokens,omitempty"`
Usage *ocTokens `json:"usage,omitempty"`
}

type ocPart struct {
ID string `json:"id"`
Type string `json:"type"`
Text string `json:"text"`
Tool string `json:"tool"`
State string `json:"state,omitempty"` // tool parts: pending|running|completed|error
Input json.RawMessage `json:"input,omitempty"` // tool parts: tool input (has file path)
Tokens *ocTokens `json:"tokens,omitempty"`
Usage *ocTokens `json:"usage,omitempty"`
}

// ocTokens tolerates the common token-count field names emitted by opencode and
// its providers (input/output vs prompt/completion).
type ocTokens struct {
Input int `json:"input"`
Output int `json:"output"`
Prompt int `json:"prompt"`
Completion int `json:"completion"`
}

// tokens returns the one canonical usage payload carried by an event. The four
// locations are the same numbers reported by different opencode/provider
// versions, so the first populated shape wins rather than being summed.
func (e *ocEvent) tokens() (in, out int, ok bool) {
for _, tk := range []*ocTokens{e.Tokens, e.Usage, e.Part.Tokens, e.Part.Usage} {
if i, o := tk.in(), tk.out(); i > 0 || o > 0 {
return i, o, true
}
}
return 0, 0, false
}

// usageKey identifies the step an event's usage belongs to, so a re-emitted
// step is not counted twice. An empty result means the event carries nothing
// stable to key on and must be counted as its own step — undercounting a
// multi-step turn is the failure this accumulation exists to prevent.
func (e *ocEvent) usageKey() string {
if e.Part.ID != "" {
return "part:" + e.Part.ID
}
if e.Timestamp != 0 {
return "ts:" + e.Type + ":" + strconv.FormatInt(e.Timestamp, 10)
}
return ""
}

func (t *ocTokens) in() int {
if t == nil {
return 0
}
if t.Input > 0 {
return t.Input
}
return t.Prompt
}

func (t *ocTokens) out() int {
if t == nil {
return 0
}
if t.Output > 0 {
return t.Output
}
return t.Completion
}

// ocError is the payload of a `{"type":"error", ...}` event. opencode emits
// these when the provider refuses a turn — a retired model, an auth failure,
// or (the case that matters for plan tracking) a 429 when the coding-plan usage
// limit is reached. Before this was parsed, such events fell through the stream
// switch and the turn ended with nothing shown to the user.
type ocError struct {
Name string `json:"name"`
Data ocErrorData `json:"data"`
}

type ocErrorData struct {
Message string `json:"message"`
StatusCode int `json:"statusCode"`
ResponseHeaders map[string]string `json:"responseHeaders"`
ResponseBody string `json:"responseBody"`
}

// parseStream reads opencode's NDJSON output, renders it, captures the session
// id for --session resume, and returns the full text of the final response.
func (a *OpenCodeAgent) parseStream(stdout interface{ Read([]byte) (int, error) }, term *tui.Terminal) string {
scanner := bufio.NewScanner(stdout)
scanner.Buffer(make([]byte, 1<<20), 1<<20)

textByPart := map[string]string{} // part id → latest full text
var order []string // text part ids in first-seen order
seenTool := map[string]bool{} // tool part ids already announced
countedUsage := map[string]bool{} // usage keys already folded into the turn total

for scanner.Scan() {
line := scanner.Bytes()
if len(line) == 0 {
continue
}
var ev ocEvent
if err := json.Unmarshal(line, &ev); err != nil {
continue
}

if ev.SessionID != "" && validOpenCodeSessionID(ev.SessionID) {
a.mu.Lock()
a.sessionID = ev.SessionID
a.mu.Unlock()
}

// Surface provider errors that would otherwise be silently dropped,
// and detect a coding-plan limit hit for the usage-window tracker.
// Record the message even when handleOCError suppresses the print
// (status-code-less events are benign noise on successful turns) —
// it is the only in-band clue when the turn later dies with no result.
if ev.Error != nil {
a.handleOCError(ev.Error, term)
msg := strings.TrimSpace(ev.Error.Data.Message)
if msg == "" {
msg = ev.Error.Name
}
a.mu.Lock()
a.lastOCErrorSeen = true
a.lastOCErrorMsg = msg
a.mu.Unlock()
}

// Accumulate token usage across the turn. opencode reports usage once
// per step (step-finish), so a turn that calls tools carries several
// token-bearing events; overwriting would keep only the final step and
// undercount every multi-step turn. The four shapes below are the same
// usage in different places rather than separate counts, so exactly one
// canonical payload is taken per event, and any step already counted is
// skipped in case opencode re-emits it.
if in, out, ok := ev.tokens(); ok {
key := ev.usageKey()
if key == "" || !countedUsage[key] {
if key != "" {
countedUsage[key] = true
}
a.mu.Lock()
a.lastTurnIn += in
a.lastTurnOut += out
a.lastTurnOK = true
a.mu.Unlock()
}
}

switch {
case ev.Type == "text" || ev.Part.Type == "text":
id := ev.Part.ID
text := ev.Part.Text
if text == "" {
continue
}
prev, seen := textByPart[id]
if !seen {
if len(order) > 0 {
// A new part starting after another already streamed (e.g. GLM's
// separate reasoning/commentary steps) has no separator of its
// own; without one its first word runs directly into the
// previous part's last word on screen.
term.StreamText("\n\n")
}
order = append(order, id)
}
// opencode may re-emit a growing snapshot for the same part id;
// stream only the delta to avoid duplication.
if strings.HasPrefix(text, prev) {
if delta := text[len(prev):]; delta != "" {
term.StreamText(delta)
}
} else {
term.StreamText(text)
}
textByPart[id] = text

case ev.Part.Type == "tool" || ev.Type == "tool":
if ev.Part.Tool == "" {
continue
}
// A tool part reaching a terminal state may have changed a file —
// render the live diff before the next output block streams.
if ev.Part.State == "completed" || ev.Part.State == "error" {
a.mu.Lock()
snap, haveSnap := a.fileSnaps[ev.Part.ID]
delete(a.fileSnaps, ev.Part.ID)
a.mu.Unlock()
if haveSnap {
printFileDiff(term, snap)
}
}
if seenTool[ev.Part.ID] {
continue
}
seenTool[ev.Part.ID] = true
displayName := stripMCPPrefix(ev.Part.Tool)
a.mu.Lock()
a.lastToolName = displayName
// Snapshot only while the edit is still ahead of us; a part first
// seen in a terminal state has already run (nothing to diff).
if ev.Part.State != "completed" && ev.Part.State != "error" {
if snap := takeFileSnapshotRaw(ev.Part.Tool, toolPathFromRaw(ev.Part.Input)); snap.path != "" {
if a.fileSnaps == nil {
a.fileSnaps = map[string]fileSnapshot{}
}
a.fileSnaps[ev.Part.ID] = snap
}
}
a.mu.Unlock()
term.PrintToolIcon(displayName)
if !a.outputVerbose {
term.EndLine()
}
}
}

var sb strings.Builder
for i, id := range order {
if i > 0 {
// Match the separator streamed live between parts above so the
// returned/rendered result doesn't run parts together either.
sb.WriteString("\n\n")
}
sb.WriteString(textByPart[id])
}
finalResult := sb.String()
term.FinishMarkdown(finalResult)
return finalResult
}

// handleOCError renders an opencode error event and, when it signals the
// subscription plan's usage limit, records the hit (and any reset time) so the
// REPL can mark the coding-plan window exhausted.
func (a *OpenCodeAgent) handleOCError(e *ocError, term *tui.Terminal) {
msg := strings.TrimSpace(e.Data.Message)
if msg == "" {
msg = e.Name
}
if e.Data.StatusCode == 429 || isPlanLimitMessage(msg) || isPlanLimitMessage(e.Data.ResponseBody) {
reset := parseResetTime(e.Data.ResponseHeaders)
a.mu.Lock()
a.lastLimitHit = true
a.lastLimitReset = reset
a.mu.Unlock()
term.PrintError("Coding-plan limit reached — " + msg + " (see /plan)")
return
}
if e.Data.StatusCode > 0 {
term.PrintError(fmt.Sprintf("opencode provider error (%d): %s", e.Data.StatusCode, msg))
return
}
// opencode 1.0.x emits benign internal errors even on successful turns — e.g.
// an "UnknownError" schema-validation gripe on the trailing step event — with
// no HTTP status code. Real provider failures (auth, quota, 5xx) all carry a
// status code and are handled above, so surfacing these status-code-less
// events on every turn would just be noise. Show them only in verbose mode.
if a.outputVerbose {
term.PrintError("opencode error: " + msg)
}
}

// LastTurnStats returns opencode's token usage for the most recent turn, when
// its stream carried any. Satisfies TurnStatsProvider.
func (a *OpenCodeAgent) LastTurnStats() (inputTokens, outputTokens int, ok bool) {
a.mu.Lock()
defer a.mu.Unlock()
return a.lastTurnIn, a.lastTurnOut, a.lastTurnOK
}

// LastPlanLimit reports whether the plan limit was hit on the most recent turn
// and, when known, the provider-reported reset time. Satisfies PlanLimitReporter.
func (a *OpenCodeAgent) LastPlanLimit() (reset time.Time, hit bool) {
a.mu.Lock()
defer a.mu.Unlock()
return a.lastLimitReset, a.lastLimitHit
}

// ClearSession forgets the opencode session id so the next turn starts fresh
// (used when the user types /clear).
func (a *OpenCodeAgent) ClearSession() {
a.mu.Lock()
a.sessionID = ""
a.mu.Unlock()
}

// ResetConversation implements ConversationResetter.
func (a *OpenCodeAgent) ResetConversation() {
a.ClearSession()
}

func (a *OpenCodeAgent) SetOutputVerbose(verbose bool) {
a.mu.Lock()
a.outputVerbose = verbose
a.mu.Unlock()
}

// Cancel interrupts a Run call that is in progress. Safe to call from any goroutine.
func (a *OpenCodeAgent) Cancel() {
a.runMu.Lock()
if a.runCancel != nil {
a.runCancel()
}
a.runMu.Unlock()
}

// Cleanup is a no-op: the managed opencode config is persistent and syncable,
// not a per-session temp file.
func (a *OpenCodeAgent) Cleanup() {}
41 changes: 35 additions & 6 deletions internal/agent/opencode_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,38 @@ func OpenCodeProviderEnv(cfg *api.Config) map[string]string {
// the provider API keys (from OpenCodeProviderEnv): opencode 1.17 reports
// "Provider not found" for known providers like groq/openrouter when their key
// env var is absent, so without it those providers would never reach the
// picker. Returns nil on error.
func OpenCodeModels(bin, configPath string, providerEnv map[string]string, providerID string) []string {
// picker.
//
// The query is a live network round-trip (models.dev / provider catalog) that
// takes seconds warm and can take far longer cold. It used to return nil
// silently on timeout or failure, which made /orch show a partial model list
// with no explanation — new models would "go missing" for no visible reason.
// It now retries once and returns the error so callers can surface it.
func OpenCodeModels(bin, configPath string, providerEnv map[string]string, providerID string) ([]string, error) {
if bin == "" || providerID == "" {
return nil
return nil, fmt.Errorf("opencode binary or provider id is missing")
}
var (
models []string
err error
)
for attempt := 0; attempt < 2; attempt++ {
models, err = listOpenCodeModelsOnce(bin, configPath, providerEnv, providerID)
if err == nil {
return models, nil
}
}
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
return nil, err
}

// openCodeModelsTimeout bounds a single `opencode models` query — a live
// network round-trip (models.dev / provider catalog). A variable rather than
// a constant so tests can exercise the timeout path quickly.
var openCodeModelsTimeout = 30 * time.Second

// listOpenCodeModelsOnce runs a single `opencode models <provider>` query.
func listOpenCodeModelsOnce(bin, configPath string, providerEnv map[string]string, providerID string) ([]string, error) {
ctx, cancel := context.WithTimeout(context.Background(), openCodeModelsTimeout)
defer cancel()

cmd := exec.CommandContext(ctx, bin, "models", providerID)
Expand All @@ -252,7 +278,10 @@ func OpenCodeModels(bin, configPath string, providerEnv map[string]string, provi
var out bytes.Buffer
cmd.Stdout = &out
if err := cmd.Run(); err != nil {
return nil
if ctx.Err() == context.DeadlineExceeded {
return nil, fmt.Errorf("opencode models %s: timed out after %s", providerID, openCodeModelsTimeout)
}
return nil, fmt.Errorf("opencode models %s: %w", providerID, err)
}

var models []string
Expand All @@ -266,5 +295,5 @@ func OpenCodeModels(bin, configPath string, providerEnv map[string]string, provi
}
models = append(models, line)
}
return models
return models, nil
}
Loading
Loading