From 490f922c15e29403bbb51eb4d689e08841f2bb06 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 2 Sep 2026 13:55:21 +0200 Subject: [PATCH 1/3] fix(agent): surface opencode model/turn failures and retry internal crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - opencode models query: 30s timeout, one retry, error returned — /orch now prints why a provider's rows are missing instead of silently dropping them - failed turns: the provider-refusal event message is surfaced with a pointer to opencode's log; deterministic refusals are not retried - internal opencode crashes (empty-result exit, no stream error): retry the turn once; if both attempts die the error carries the stderr tail --- internal/agent/opencode_agent.go | 159 ++++++++++++++++++++++--- internal/agent/opencode_config.go | 36 +++++- internal/agent/opencode_config_test.go | 59 ++++++++- internal/agent/opencode_run_test.go | 130 ++++++++++++++++++++ internal/agent/opencode_stream_test.go | 21 ++++ internal/repl/repl.go | 29 +++-- 6 files changed, 402 insertions(+), 32 deletions(-) create mode 100644 internal/agent/opencode_run_test.go diff --git a/internal/agent/opencode_agent.go b/internal/agent/opencode_agent.go index 1b34ddb..a3a47b2 100644 --- a/internal/agent/opencode_agent.go +++ b/internal/agent/opencode_agent.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -51,6 +52,17 @@ type OpenCodeAgent struct { 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 @@ -141,7 +153,17 @@ func validOpenCodeSessionID(id string) bool { return true } -// 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. @@ -158,16 +180,64 @@ func (a *OpenCodeAgent) Run(userMsg string, term *tui.Terminal) (string, error) 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 != "" { @@ -200,21 +270,10 @@ func (a *OpenCodeAgent) Run(userMsg string, term *tui.Terminal) (string, error) 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{} + 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) @@ -236,12 +295,67 @@ func (a *OpenCodeAgent) Run(userMsg string, term *tui.Terminal) (string, error) 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 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 s +} + +// stderrTailBuffer keeps the last bytes written to it. One instance per run +// attempt; not safe for concurrent use. +type stderrTailBuffer struct{ buf []byte } + +// 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") + } + 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 { @@ -371,8 +485,19 @@ func (a *OpenCodeAgent) parseStream(stdout interface{ Read([]byte) (int, error) // 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 diff --git a/internal/agent/opencode_config.go b/internal/agent/opencode_config.go index 90404b8..976d7fb 100644 --- a/internal/agent/opencode_config.go +++ b/internal/agent/opencode_config.go @@ -236,12 +236,33 @@ 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 +} + +// listOpenCodeModelsOnce runs a single `opencode models ` query. +func listOpenCodeModelsOnce(bin, configPath string, providerEnv map[string]string, providerID string) ([]string, error) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() cmd := exec.CommandContext(ctx, bin, "models", providerID) @@ -252,7 +273,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 30s", providerID) + } + return nil, fmt.Errorf("opencode models %s: %w", providerID, err) } var models []string @@ -266,5 +290,5 @@ func OpenCodeModels(bin, configPath string, providerEnv map[string]string, provi } models = append(models, line) } - return models + return models, nil } diff --git a/internal/agent/opencode_config_test.go b/internal/agent/opencode_config_test.go index 4e7adbb..8255423 100644 --- a/internal/agent/opencode_config_test.go +++ b/internal/agent/opencode_config_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/qualitymax/qmax-code/internal/api" @@ -119,13 +120,69 @@ func TestOpenCodeModelsPassesProviderEnv(t *testing.T) { t.Fatal(err) } - got := OpenCodeModels(stub, filepath.Join(dir, "cfg.json"), map[string]string{"GROQ_API_KEY": "gsk_regress"}, "groq") + got, err := OpenCodeModels(stub, filepath.Join(dir, "cfg.json"), map[string]string{"GROQ_API_KEY": "gsk_regress"}, "groq") + if err != nil { + t.Fatalf("OpenCodeModels: unexpected error: %v", err) + } want := "groq/model-gsk_regress" if len(got) != 1 || got[0] != want { t.Fatalf("OpenCodeModels did not pass provider env: got %v, want [%s]", got, want) } } +// TestOpenCodeModelsRetriesTransientFailure pins the /orch regression where a +// transiently failing `opencode models ` (cold-start slowness, catalog +// hiccup) made the provider's models silently vanish from the picker — glm-5.3 +// "could not be added" until /orch was re-run minutes later. The stub fails the +// first invocation and succeeds on the second; one retry must recover the list. +func TestOpenCodeModelsRetriesTransientFailure(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stub uses a shell script") + } + dir := t.TempDir() + stub := filepath.Join(dir, "opencode-stub") + marker := filepath.Join(dir, "failed-once") + script := "#!/bin/sh\n" + + "if [ ! -f \"" + marker + "\" ]; then touch \"" + marker + "\"; exit 1; fi\n" + + "echo \"groq/llama-4\"\n" + if err := os.WriteFile(stub, []byte(script), 0755); err != nil { + t.Fatal(err) + } + + got, err := OpenCodeModels(stub, filepath.Join(dir, "cfg.json"), nil, "groq") + if err != nil { + t.Fatalf("OpenCodeModels should recover via retry: %v", err) + } + if len(got) != 1 || got[0] != "groq/llama-4" { + t.Fatalf("OpenCodeModels = %v, want [groq/llama-4]", got) + } +} + +// TestOpenCodeModelsReturnsErrorWhenAllAttemptsFail ensures that when every +// attempt fails the error is returned instead of the old silent nil that hid +// the failure from the picker entirely. +func TestOpenCodeModelsReturnsErrorWhenAllAttemptsFail(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stub uses a shell script") + } + dir := t.TempDir() + stub := filepath.Join(dir, "opencode-stub") + if err := os.WriteFile(stub, []byte("#!/bin/sh\nexit 3\n"), 0755); err != nil { + t.Fatal(err) + } + + got, err := OpenCodeModels(stub, filepath.Join(dir, "cfg.json"), nil, "groq") + if err == nil { + t.Fatal("OpenCodeModels must return an error when all attempts fail") + } + if got != nil { + t.Errorf("OpenCodeModels = %v, want nil on failure", got) + } + if !strings.Contains(err.Error(), "groq") { + t.Errorf("error should name the provider: %v", err) + } +} + func TestOpenCodeProviderEnvInjectsKeys(t *testing.T) { oldLoadProviderKey := loadProviderKey t.Cleanup(func() { loadProviderKey = oldLoadProviderKey }) diff --git a/internal/agent/opencode_run_test.go b/internal/agent/opencode_run_test.go new file mode 100644 index 0000000..33252b0 --- /dev/null +++ b/internal/agent/opencode_run_test.go @@ -0,0 +1,130 @@ +package agent + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/qualitymax/qmax-code/internal/api" + "github.com/qualitymax/qmax-code/internal/tui" +) + +// The Run-level tests drive the real spawn/parse/wait path through a stub +// `opencode` binary. The stub must answer the `run --help` probe +// (openCodeSupportsAutoFlag) with exit 0 so the flag support check is +// deterministic regardless of the package-global sync.Once state, and it logs +// every non-help invocation to a counts file so retry behaviour is assertable. + +func writeOpenCodeStub(t *testing.T, dir, name, body string) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("stub uses a shell script") + } + stub := filepath.Join(dir, name) + script := "#!/bin/sh\nif [ \"$2\" = \"--help\" ]; then exit 0; fi\n" + body + if err := os.WriteFile(stub, []byte(script), 0755); err != nil { + t.Fatal(err) + } + return stub +} + +func stubInvocationCount(t *testing.T, path string) int { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + return 0 + } + return strings.Count(strings.TrimSpace(string(data)), "\n") + 1 +} + +// TestOpenCodeRunRetriesAfterInternalCrash pins the resilience fix for the +// opencode internal JS crash ("G.includes is not a function", upstream +// anomalyco/opencode#28117 class): the process dies mid-turn with an empty +// result and no error event in the stream. The turn must be retried exactly +// once, and the retry must complete it. +func TestOpenCodeRunRetriesAfterInternalCrash(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "crashed-once") + counts := filepath.Join(dir, "counts") + stub := writeOpenCodeStub(t, dir, "opencode-stub", + "echo x >> \""+counts+"\"\n"+ + "if [ ! -f \""+marker+"\" ]; then\n"+ + " echo 'Error: Unexpected error G.includes is not a function' >&2\n"+ + " touch \""+marker+"\"\n"+ + " exit 1\n"+ + "fi\n"+ + `echo '{"type":"text","timestamp":1784907424583,"sessionID":"ses_retry0001","part":{"id":"prt_b","type":"text","text":"recovered"}}'`+"\n") + + a := &OpenCodeAgent{openCodeBin: stub, cfg: &api.Config{}} + result, err := a.Run("hello", &tui.Terminal{}) + if err != nil { + t.Fatalf("Run should recover via retry: %v", err) + } + if result != "recovered" { + t.Fatalf("result = %q, want %q", result, "recovered") + } + if n := stubInvocationCount(t, counts); n != 2 { + t.Fatalf("stub invoked %d times, want exactly 2 (crash + retry)", n) + } + if a.sessionID != "ses_retry0001" { + t.Errorf("sessionID = %q, want captured from the retry stream", a.sessionID) + } +} + +// TestOpenCodeRunSurfacesProviderRefusalWithoutRetry pins the diagnostics fix +// for masked provider refusals (model not in the subscription plan): the +// stream carries a status-code-less "Unexpected server error" event and the +// process exits 1 with no result. The real message must be surfaced — with a +// pointer to opencode's log where the entitlement text lives — and the turn +// must NOT be retried, because a provider refusal is deterministic. +func TestOpenCodeRunSurfacesProviderRefusalWithoutRetry(t *testing.T) { + dir := t.TempDir() + counts := filepath.Join(dir, "counts") + stub := writeOpenCodeStub(t, dir, "opencode-stub", + "echo x >> \""+counts+"\"\n"+ + `echo '{"type":"error","timestamp":1788348788916,"sessionID":"ses_refuse01","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_1f79e3db"}}}'`+"\n"+ + "exit 1\n") + + a := &OpenCodeAgent{openCodeBin: stub, cfg: &api.Config{}} + result, err := a.Run("hello", &tui.Terminal{}) + if err == nil { + t.Fatal("Run must fail when the provider refuses the turn") + } + if result != "" { + t.Errorf("result = %q, want empty on refusal", result) + } + for _, want := range []string{"Unexpected server error", "opencode.log"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q must mention %q so the real cause is findable", err.Error(), want) + } + } + if n := stubInvocationCount(t, counts); n != 1 { + t.Fatalf("stub invoked %d times, want exactly 1 — a provider refusal must not be retried", n) + } +} + +// TestOpenCodeRunCrashTwiceIncludesStderrTail ensures that when both attempts +// die in an internal crash, the returned error carries the stderr tail — the +// only witness to what opencode actually printed before dying. +func TestOpenCodeRunCrashTwiceIncludesStderrTail(t *testing.T) { + dir := t.TempDir() + counts := filepath.Join(dir, "counts") + stub := writeOpenCodeStub(t, dir, "opencode-stub", + "echo x >> \""+counts+"\"\n"+ + "echo 'Error: Unexpected error G.includes is not a function' >&2\n"+ + "exit 1\n") + + a := &OpenCodeAgent{openCodeBin: stub, cfg: &api.Config{}} + _, err := a.Run("hello", &tui.Terminal{}) + if err == nil { + t.Fatal("Run must fail when both attempts crash") + } + if !strings.Contains(err.Error(), "G.includes is not a function") { + t.Errorf("error should include the stderr tail, got: %v", err) + } + if n := stubInvocationCount(t, counts); n != 2 { + t.Fatalf("stub invoked %d times, want exactly 2 (crash + one retry)", n) + } +} diff --git a/internal/agent/opencode_stream_test.go b/internal/agent/opencode_stream_test.go index 23e1890..39464c0 100644 --- a/internal/agent/opencode_stream_test.go +++ b/internal/agent/opencode_stream_test.go @@ -160,3 +160,24 @@ func TestOpenCodeCountsOneCanonicalPayloadPerEvent(t *testing.T) { t.Fatalf("LastTurnStats = %d/%d, want 80/10 — the shapes are one payload", in, out) } } + +// ocRefusalStream is the real event captured when the provider refused a turn +// because the model is not in the subscription plan: opencode masks the actual +// entitlement error as a status-code-less UnknownError whose full text only +// exists in opencode's own log. handleOCError deliberately suppresses such +// events as noise on successful turns — but they must still be recorded so +// Run can surface the real cause when the turn dies with no result. +const ocRefusalStream = `{"type":"error","timestamp":1788348788916,"sessionID":"ses_refuse01","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_1f79e3db"}}}` + +func TestOpenCodeParseRecordsErrorForDiagnostics(t *testing.T) { + a := &OpenCodeAgent{} + a.parseStream(strings.NewReader(ocRefusalStream), &tui.Terminal{}) + + msg, seen := a.lastRunError() + if !seen { + t.Fatal("a status-code-less error event must still be recorded for failed-turn diagnostics") + } + if msg != "Unexpected server error. Check server logs for details." { + t.Fatalf("lastRunError = %q, want the masked refusal message", msg) + } +} diff --git a/internal/repl/repl.go b/internal/repl/repl.go index 54bec73..0cb012a 100644 --- a/internal/repl/repl.go +++ b/internal/repl/repl.go @@ -515,7 +515,10 @@ func Run(ag *agent.Agent, cliAgent agent.CLIAgent, quietMode bool, version strin if len(cfg.ActiveProviders()) > 0 && agent.FindOpenCode() != "" { term.PrintSystem("Querying opencode models for your enabled providers…") } - ocModels := buildOpenCodeModelEntries(cfg, ag.Cfg.Context) + ocModels, modelQueryWarnings := buildOpenCodeModelEntries(cfg, ag.Cfg.Context) + for _, w := range modelQueryWarnings { + term.PrintError(w) + } if len(ocModels) == 0 { ocModels = openCodeSetupEntries() } @@ -2150,25 +2153,35 @@ func anthropicBackendAvailable(ag *agent.Agent, term *tui.Terminal) bool { // It queries `opencode models ` 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 { +// +// A provider whose model query fails (timeout, opencode error) previously made +// its models silently vanish from the picker. The failure is now returned as a +// warning string so the caller can tell the user why rows are missing. +func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) ([]tui.OpenCodeModelEntry, []string) { bin := agent.FindOpenCode() if bin == "" { - return nil + return nil, nil } active := cfg.ActiveProviders() if len(active) == 0 { - return nil + return nil, nil } path, err := agent.WriteOpenCodeConfig(cfg, sctx, cfg.OrchPermissionMode) if err != nil { - return nil + return nil, []string{fmt.Sprintf("Model list unavailable — opencode config write failed: %v", err)} } // Provider keys must be present in the env or `opencode models ` // reports "Provider not found" for known providers (groq/openrouter). env := agent.OpenCodeProviderEnv(cfg) var entries []tui.OpenCodeModelEntry + var warnings []string for _, p := range active { - for _, full := range agent.OpenCodeModels(bin, path, env, p.ID) { + models, err := agent.OpenCodeModels(bin, path, env, p.ID) + if err != nil { + warnings = append(warnings, fmt.Sprintf("Model list for %s unavailable (%v) — re-run /orch to retry", p.DisplayName, err)) + continue + } + for _, full := range models { label := full if i := strings.Index(full, "/"); i >= 0 { label = full[i+1:] @@ -2181,7 +2194,7 @@ func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) []tui. }) } } - return entries + return entries, warnings } // resolveOpenCodeModelOverride returns a valid "provider/model" for the current @@ -2190,7 +2203,7 @@ func buildOpenCodeModelEntries(cfg *api.Config, sctx *api.SessionContext) []tui. // model (so a stale override from another backend is never sent to opencode). // Returns false when no models are available at all. func resolveOpenCodeModelOverride(cfg *api.Config, sctx *api.SessionContext) (string, bool) { - entries := buildOpenCodeModelEntries(cfg, sctx) + entries, _ := buildOpenCodeModelEntries(cfg, sctx) if len(entries) == 0 { return "", false } From d7f93a432726f02100e3151d8b512463073c17a9 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 2 Sep 2026 14:17:07 +0200 Subject: [PATCH 2/3] fix(agent): redact stderr tails and cover model-query timeout path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #181: - stderrTailSnapshot now redacts credential-shaped output (api keys, bearer tokens, JWTs, sk-/gsk-/ghp- style prefixes) before the tail lands in a returned error — defense in depth against a crash dumping auth state - openCodeModelsTimeout is an injectable variable and the timeout path has a fast test (sleeping stub + 100ms deadline) asserting the 'timed out' report --- internal/agent/opencode_agent.go | 28 +++++++++++++++++++++++--- internal/agent/opencode_config.go | 9 +++++++-- internal/agent/opencode_config_test.go | 28 ++++++++++++++++++++++++++ internal/agent/opencode_run_test.go | 23 +++++++++++++++++++++ 4 files changed, 83 insertions(+), 5 deletions(-) diff --git a/internal/agent/opencode_agent.go b/internal/agent/opencode_agent.go index a3a47b2..51ffcb3 100644 --- a/internal/agent/opencode_agent.go +++ b/internal/agent/opencode_agent.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "runtime" "strconv" "strings" @@ -312,9 +313,9 @@ func (a *OpenCodeAgent) lastRunError() (string, bool) { return a.lastOCErrorMsg, a.lastOCErrorSeen } -// stderrTailSnapshot returns a bounded 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. +// 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() @@ -322,6 +323,27 @@ func (a *OpenCodeAgent) stderrTailSnapshot() string { 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 +}{ + {regexp.MustCompile(`(?i)\b(api[_-]?key|token|secret|password|authorization)\b\s*[=:]\s*(bearer\s+)?\S+`), "${1}="}, + {regexp.MustCompile(`(?i)\bbearer\s+\S+`), "bearer "}, + {regexp.MustCompile(`\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza)[A-Za-z0-9_\-]{16,}\b`), ""}, + {regexp.MustCompile(`\beyJ[A-Za-z0-9_\-.]{20,}\b`), ""}, +} + +func redactStderrTail(s string) string { + for _, p := range stderrSecretPatterns { + s = p.re.ReplaceAllString(s, p.repl) + } return s } diff --git a/internal/agent/opencode_config.go b/internal/agent/opencode_config.go index 976d7fb..764690b 100644 --- a/internal/agent/opencode_config.go +++ b/internal/agent/opencode_config.go @@ -260,9 +260,14 @@ func OpenCodeModels(bin, configPath string, providerEnv map[string]string, provi 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 ` query. func listOpenCodeModelsOnce(bin, configPath string, providerEnv map[string]string, providerID string) ([]string, error) { - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), openCodeModelsTimeout) defer cancel() cmd := exec.CommandContext(ctx, bin, "models", providerID) @@ -274,7 +279,7 @@ func listOpenCodeModelsOnce(bin, configPath string, providerEnv map[string]strin cmd.Stdout = &out if err := cmd.Run(); err != nil { if ctx.Err() == context.DeadlineExceeded { - return nil, fmt.Errorf("opencode models %s: timed out after 30s", providerID) + return nil, fmt.Errorf("opencode models %s: timed out after %s", providerID, openCodeModelsTimeout) } return nil, fmt.Errorf("opencode models %s: %w", providerID, err) } diff --git a/internal/agent/opencode_config_test.go b/internal/agent/opencode_config_test.go index 8255423..fe7efe6 100644 --- a/internal/agent/opencode_config_test.go +++ b/internal/agent/opencode_config_test.go @@ -7,6 +7,7 @@ import ( "runtime" "strings" "testing" + "time" "github.com/qualitymax/qmax-code/internal/api" ) @@ -183,6 +184,33 @@ func TestOpenCodeModelsReturnsErrorWhenAllAttemptsFail(t *testing.T) { } } +// TestOpenCodeModelsReportsTimeout covers the query-timeout path (review +// follow-up on PR #181): a slow provider query must be reported as a timeout, +// not as an opaque exec failure. The timeout is shortened via the package +// variable so the test stays fast. +func TestOpenCodeModelsReportsTimeout(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stub uses a shell script") + } + dir := t.TempDir() + stub := filepath.Join(dir, "opencode-stub") + if err := os.WriteFile(stub, []byte("#!/bin/sh\nsleep 2\n"), 0755); err != nil { + t.Fatal(err) + } + + old := openCodeModelsTimeout + openCodeModelsTimeout = 100 * time.Millisecond + defer func() { openCodeModelsTimeout = old }() + + _, err := OpenCodeModels(stub, filepath.Join(dir, "cfg.json"), nil, "groq") + if err == nil { + t.Fatal("OpenCodeModels must fail when the query exceeds the timeout") + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("error should report a timeout, got: %v", err) + } +} + func TestOpenCodeProviderEnvInjectsKeys(t *testing.T) { oldLoadProviderKey := loadProviderKey t.Cleanup(func() { loadProviderKey = oldLoadProviderKey }) diff --git a/internal/agent/opencode_run_test.go b/internal/agent/opencode_run_test.go index 33252b0..a205893 100644 --- a/internal/agent/opencode_run_test.go +++ b/internal/agent/opencode_run_test.go @@ -11,6 +11,29 @@ import ( "github.com/qualitymax/qmax-code/internal/tui" ) +// TestRedactStderrTail pins the defensive redaction applied to the stderr tail +// before it lands in a returned error (review finding on PR #181): a crashing +// subprocess must never echo a credential into the TUI or logs. +func TestRedactStderrTail(t *testing.T) { + in := "Authorization: Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.payload.sig\n" + + "api_key=gsk_0123456789abcdefghij\n" + + "sk-proj-0123456789abcdefghij\n" + + "Error: Unexpected error G.includes is not a function" + got := redactStderrTail(in) + + for _, leaked := range []string{"gsk_0123456789", "sk-proj-0123456789", "eyJhbGciOiJIUzI1NiIs"} { + if strings.Contains(got, leaked) { + t.Errorf("stderr tail leaked credential %q: %s", leaked, got) + } + } + if !strings.Contains(got, "G.includes is not a function") { + t.Errorf("redaction must keep the diagnostic text, got: %s", got) + } + if !strings.Contains(got, "") { + t.Errorf("redaction should mark removed values, got: %s", got) + } +} + // The Run-level tests drive the real spawn/parse/wait path through a stub // `opencode` binary. The stub must answer the `run --help` probe // (openCodeSupportsAutoFlag) with exit 0 so the flag support check is From 6ad977dea811c55f638e25426e5143158fbbcafa Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 2 Sep 2026 15:49:18 +0200 Subject: [PATCH 3/3] fix(agent): cover AWS key ids in stderr redaction and pin edge cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up round 2 on #181: - add AKIA/ASIA prefixes to the credential patterns (the one real gap) - TestRedactStderrTailEdgeCases pins both directions: hex git SHAs survive redaction (a SHA cannot start with a credential prefix), while X-API-Key:, token=…, and AWS access-key ids are redacted --- internal/agent/opencode_agent.go | 2 +- internal/agent/opencode_run_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/internal/agent/opencode_agent.go b/internal/agent/opencode_agent.go index 51ffcb3..550ba35 100644 --- a/internal/agent/opencode_agent.go +++ b/internal/agent/opencode_agent.go @@ -336,7 +336,7 @@ var stderrSecretPatterns = []struct { }{ {regexp.MustCompile(`(?i)\b(api[_-]?key|token|secret|password|authorization)\b\s*[=:]\s*(bearer\s+)?\S+`), "${1}="}, {regexp.MustCompile(`(?i)\bbearer\s+\S+`), "bearer "}, - {regexp.MustCompile(`\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza)[A-Za-z0-9_\-]{16,}\b`), ""}, + {regexp.MustCompile(`\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza|AKIA|ASIA)[A-Za-z0-9_\-]{16,}\b`), ""}, {regexp.MustCompile(`\beyJ[A-Za-z0-9_\-.]{20,}\b`), ""}, } diff --git a/internal/agent/opencode_run_test.go b/internal/agent/opencode_run_test.go index a205893..bbea7ed 100644 --- a/internal/agent/opencode_run_test.go +++ b/internal/agent/opencode_run_test.go @@ -34,6 +34,32 @@ func TestRedactStderrTail(t *testing.T) { } } +// TestRedactStderrTailEdgeCases pins the review findings on PR #181 from both +// directions: non-secret identifiers (git SHAs are hex and cannot start with a +// credential prefix) must survive redaction, while header/env-var credential +// shapes (X-API-Key:, token=…) and AWS access-key ids must be redacted. +func TestRedactStderrTailEdgeCases(t *testing.T) { + sha := "abcdef1234567890abcdef1234567890abcdef12" + in := "Commit: " + sha + "\n" + + "X-API-Key: k_abc123def456ghi789\n" + + "token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpX\n" + + "AKIAIOSFODNN7EXAMPLE\n" + + "Error: Unexpected error G.includes is not a function" + got := redactStderrTail(in) + + if !strings.Contains(got, sha) { + t.Errorf("a hex git SHA is not a credential and must survive redaction, got: %s", got) + } + for _, leaked := range []string{"k_abc123def456ghi789", "eyJhbGciOiJIUzI1NiIsInR5cCI6", "AKIAIOSFODNN7EXAMPLE"} { + if strings.Contains(got, leaked) { + t.Errorf("stderr tail leaked credential %q: %s", leaked, got) + } + } + if !strings.Contains(got, "G.includes is not a function") { + t.Errorf("redaction must keep the diagnostic text, got: %s", got) + } +} + // The Run-level tests drive the real spawn/parse/wait path through a stub // `opencode` binary. The stub must answer the `run --help` probe // (openCodeSupportsAutoFlag) with exit 0 so the flag support check is