fix(agent): surface opencode model/turn failures and retry internal crashes - #181
Open
Desperado wants to merge 3 commits into
Open
fix(agent): surface opencode model/turn failures and retry internal crashes#181Desperado wants to merge 3 commits into
Desperado wants to merge 3 commits into
Conversation
…rashes - 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
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushSurfaces previously silent opencode model-query timeouts and provider-refusal errors, and adds a single retry for internal opencode crashes (e.g. JS TypeErrors) that kill the process mid-turn. The change also defensively redacts credential-shaped strings from stderr tails before they reach the TUI or logs, addressing a security concern raised during review. Important files
Confidence: 3/5The stderr redaction regex uses a literal
Suggested labels: Phase: Specialists |
![qualitymaxapp[bot]](https://avatars.githubusercontent.com/in/2748338?s=60&v=4){kind=small}
QualityMax ReviewVerdict: COMMENT · Confidence: evidence-backed scan Files eligible: 6 · Files reviewed: 6 · Files with findings: 0 · Findings: 0 · Inline cards: 0 Priority findings
Review gates
Important files
Change diagram — Flowflowchart TD
Start[Run] --> Attempt1[runAttempt]
Attempt1 --> Success1{Success?}
Success1 -- Yes --> Return[Return Result]
Success1 -- No --> ProviderErr{Provider Error?}
ProviderErr -- Yes --> Fail[Return Error with Details]
ProviderErr -- No --> Retry[Retry runAttempt]
Retry --> Success2{Success?}
Success2 -- Yes --> Return
Success2 -- No --> FailTail[Return Error with Stderr Tail]
Review lifecycleUse the inline cards to inspect evidence and suggested remediation. Re-run the QualityMax review after pushing a fix; unchanged cards are identified by their stable finding marker. Dismiss with a reason through the existing QualityMax/GitHub review feedback flow. 0 prior card(s) are stale/resolved on this head. Proof legend: VERIFIED independently judged patch · REPRODUCED verified finding · GROUNDED deterministic evidence · MODEL-ONLY model judgment. QualityMax project results are available in the configured project. Receipt · commit |
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| @@ -141,7 +153,17 @@ func validOpenCodeSessionID(id string) bool { | |||
| return true | |||
There was a problem hiding this 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.
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
lastRunErrorcheck 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.lastOCErrorMsgset 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 thatlastOCErrorSeenandlastOCErrorMsgare 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.
|
|
||
| // stderrTailBuffer keeps the last bytes written to it. One instance per run | ||
| // attempt; not safe for concurrent use. | ||
| type stderrTailBuffer struct{ buf []byte } |
There was a problem hiding this 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.
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:
stderrTailBufferstruct withbuf []bytefield;Writemethod appends up tostderrTailLimit(8KB);stderrTailSnapshotreturns 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.
| // 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") |
There was a problem hiding this 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)'
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:
openCodeLogPathreturns~/.local/share/opencode/log/opencode.logor$XDG_DATA_HOME/opencode/log/opencode.log; included in error strings viaopencode.logpointer. - 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.
|
| Gate | Result |
|---|---|
| 🔍 AI diff review | ✅ Clean · gemini-3.1-flash-lite · completed · 6 eligible / 6 reviewed · gemini-3.1-flash-lite |
| 🔍 SAST | completed · 6 eligible / 6 reviewed · qwen3.7-plus |
| 🔍 Canonical PR review delivery | completed · 0 eligible / 0 reviewed · exact-head review #5090516911 and overview #5509156304 confirmed |
| 🧪 Repo Tests | ✅ 722/722 passed (go) |
Powered by QualityMax — AI-Powered Test Automation
Contributor
Author
|
@Qmax please review it |
|
👀 Review request accepted; a fresh exact-head Gate Alpha review has started. View it in PR checks. GitHub App: @qualitymaxapp ( |
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
Contributor
Author
Review triage (Sigilix findings + QualityMax clean)QualityMax gates: clean, 0 findings — no action. Sigilix (model-only, 3 findings) triaged: Fixed in d7f93a4:
Dismissed:
Full suite re-run on d7f93a4: 18 packages, 0 failures; gofmt/vet clean on touched files. |
Comment on lines
+333
to
+339
| 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}=<redacted>"}, | ||
| {regexp.MustCompile(`(?i)\bbearer\s+\S+`), "bearer <redacted>"}, | ||
| {regexp.MustCompile(`\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza)[A-Za-z0-9_\-]{16,}\b`), "<redacted>"}, |
There was a problem hiding this comment.
Secret redaction regex patterns may match non-secret values, causing false positives and potential data loss.
The stderrSecretPatterns regexes are overly broad and could match non-secret strings like random base64-encoded data, commit hashes, or UUIDs, leading to unnecessary redaction in error messages. This could obscure diagnostic information. The patterns also may miss some credential formats (e.g., sk_live_ for Stripe, AKIA for AWS).
Example:
Stderr contains: `Commit: abcdef1234567890abcdef1234567890abcdef12` (a git SHA). The regex matches `abcdef1234567890abcdef1234567890abcdef12` (40 chars, starts with hex) and redacts it, losing the commit hash.
Suggested fix:
Add a pattern to exclude git commit SHAs (40 hex chars). Tighten the generic token pattern to require a known prefix followed by a more specific pattern (e.g., `sk_live_` for Stripe).Why this wasn't caught: The existing TestRedactStderrTail does not test false positives for non-secret values like git SHAs or UUIDs.
More Info
- Threat model: If the redaction regex incorrectly redacts non-secret stderr output, debugging becomes harder. If it misses a credential format, a secret could leak into logs/TUI.
- Specific code citations: Lines 333-342 define
stderrSecretPatterns. The regex\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza)[A-Za-z0-9_\-]{16,}\bmatches many 16+ character alphanumeric strings starting with those prefixes, which could include non-secrets. - Existing protections: The redaction runs only on stderr tail included in error messages, not on all logs. The tail is limited to 500 chars.
- Proposed mitigation: Tighten regex patterns to be more specific to known credential formats, and consider using a allowlist of known safe patterns (like commit SHAs) to avoid false positives. Add tests for edge cases.
- Alternative mitigations considered: Remove redaction entirely and rely on opencode not to leak secrets; this is risky. Use a library for secret detection; adds dependency.
- Severity calibration: Score 3 because it's a hardening gap that could affect debuggability and might not catch all real secrets, but the blast radius is limited to error messages from a crashing subprocess.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/opencode_agent.go
Line: 333-339
Comment:
**Secret redaction regex patterns may match non-secret values, causing false positives and potential data loss.**
The `stderrSecretPatterns` regexes are overly broad and could match non-secret strings like random base64-encoded data, commit hashes, or UUIDs, leading to unnecessary redaction in error messages. This could obscure diagnostic information. The patterns also may miss some credential formats (e.g., `sk_live_` for Stripe, `AKIA` for AWS).
Example:
Stderr contains: `Commit: abcdef1234567890abcdef1234567890abcdef12` (a git SHA). The regex matches `abcdef1234567890abcdef1234567890abcdef12` (40 chars, starts with hex) and redacts it, losing the commit hash.
Threat model:
If the redaction regex incorrectly redacts non-secret stderr output, debugging becomes harder. If it misses a credential format, a secret could leak into logs/TUI.
Specific code citations:
Lines 333-342 define `stderrSecretPatterns`. The regex `\b(sk|gsk|rk|ghp|gho|ghu|ghs|xox[bpars]|AIza)[A-Za-z0-9_\-]{16,}\b` matches many 16+ character alphanumeric strings starting with those prefixes, which could include non-secrets.
Existing protections:
The redaction runs only on stderr tail included in error messages, not on all logs. The tail is limited to 500 chars.
Proposed mitigation:
Tighten regex patterns to be more specific to known credential formats, and consider using a allowlist of known safe patterns (like commit SHAs) to avoid false positives. Add tests for edge cases.
Alternative mitigations considered:
Remove redaction entirely and rely on opencode not to leak secrets; this is risky. Use a library for secret detection; adds dependency.
Severity calibration:
Score 3 because it's a hardening gap that could affect debuggability and might not catch all real secrets, but the blast radius is limited to error messages from a crashing subprocess.
Suggested fix shape:
Add a pattern to exclude git commit SHAs (40 hex chars). Tighten the generic token pattern to require a known prefix followed by a more specific pattern (e.g., `sk_live_` for Stripe).
Why this wasn't caught:
The existing TestRedactStderrTail does not test false positives for non-secret values like git SHAs or UUIDs.
How can I resolve this? If you propose a fix, please make it concise.
| cmd := exec.CommandContext(ctx, a.openCodeBin, args...) | ||
| cmd.Stdin = strings.NewReader("") | ||
| cmd.Stderr = term.Stderr() | ||
| tail := &stderrTailBuffer{} |
There was a problem hiding this 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.
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 containingkey,token,secret,passwordfollowed 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.
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
Contributor
Author
Sigilix round-2 triage (6ad977d)P2 — redaction false-positives on git SHAs — P3 — secrets in unexpected formats (X-API-Key:, token=…) — Suite on 6ad977d: 18 packages, 0 failures. |
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.
Problem
Three user-visible failures from one qmax-code session (opencode 1.17.18, Z.AI coding plan):
glm-5.3was absent from the picker on the first/orchinvocation, appeared minutes later. User: "check why we can't add glm-5.3".✗ opencode exited with error: exit status 1with no cause. Every prompt failed with zero diagnostics.G.includes is not a functioncrash. Typing a short reply ("3") killed the turn mid-stream; process died with nothing in opencode's log.Root causes (all verified against live logs/repro)
OpenCodeModelsshells out toopencode models <provider>— a live network query (~7s warm, slower cold) — with a 15s timeout and silentnilon error (internal/agent/opencode_config.go). A transient failure made the provider's rows vanish with no warning.opencode run --model zai-coding-plan/glm-5v-turbo:当前订阅套餐暂未开放GLM-5V-Turbo权限— model not in the subscription plan) exists only in opencode's log; the stream event is status-code-less, andhandleOCErrordeliberately suppresses status-code-less events as benign 1.0.x noise on successful turns → on a failed turn nothing surfaces → generic exit error.Fix
opencode_config.go— model query: 30s timeout, one retry, returns the error; timeout is reported as a timeout.repl.go—/orchprints per-provider failures:Model list for Z.AI unavailable (…) — re-run /orch to retry.opencode_agent.go— failed turns explain themselves:Tests
TestOpenCodeModelsRetriesTransientFailure— transient list failure recovers via retryTestOpenCodeModelsReturnsErrorWhenAllAttemptsFail— silent-nil regressionTestOpenCodeParseRecordsErrorForDiagnostics— real captured refusal event is recordedTestOpenCodeRunRetriesAfterInternalCrash— replays the exactG.includescrash, retry recovers, exactly 2 invocationsTestOpenCodeRunSurfacesProviderRefusalWithoutRetry— refusal surfaced, exactly 1 invocationTestOpenCodeRunCrashTwiceIncludesStderrTail— both attempts die → stderr tail in errorTest plan
go test ./...— 18 packages, 0 failuresgofmtclean on touched files,go vetclean