fix(opencode): drain stdout/stderr via StdoutPipe to capture full discovery output - #11
fix(opencode): drain stdout/stderr via StdoutPipe to capture full discovery output#11inola-ww wants to merge 2 commits into
Conversation
…covery output runOpenCode set command.Stdout = &buf and let exec.Run() drain the pipes itself. For the opencode db discovery query this returned a truncated buffer: when the user's SQLite store has many sessions with multi-KB first/last user-message previews, the JSON output crosses ~4 MB and os/exec's copy goroutine loses the tail after the child exits. discoverOpenCode then sees a parse error, deliberately swallows it (so discovery is best-effort), and returns zero opencode rows. The picker ends up showing only the providers that read their stores directly off disk (claude/pi/codex/gemini/jcode). Switch the helper to command.StdoutPipe() + StderrPipe() and copy both into cappedBuffer / bytes.Buffer from goroutines, waiting on the drain before command.Wait() so the buffer holds the full payload. Fixes aytzey#10
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesOpenCode subprocess output
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/session/opencode.go`:
- Line 466: Replace the unbounded stderr bytes.Buffer in the opencode command
execution flow with cappedBuffer, configured using the existing maxOpenCodeError
limit, matching the stdout safeguard and preserving the existing stderr capture
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b5b216c-158f-4640-8e7a-c27de5ce86a3
📒 Files selected for processing (2)
internal/session/opencode.gointernal/session/opencode_test.go
…eError limit The pipe-drain conversion in the previous commit dropped the cappedBuffer on stderr in favor of an unbounded bytes.Buffer, so a misbehaving opencode CLI could grow stderr without bound (and allocate until OOM). Reuse the existing maxOpenCodeError cap, matching the stdout safeguard and the original pre-fix code path. Caught by CodeRabbit review on PR aytzey#11.
|
The 66.67% docstring coverage is computed across Functions/methods in my PR diff:
The remaining ~32% are pre-existing symbols this PR did not touch:
Adding docstrings to those here would expand the PR scope beyond the |
1 participant
Fixes #10.
Summary
internal/session/opencode.go:runOpenCodesetcommand.Stdout = &bufandlet
command.Run()drain the pipes itself. When the opencode db discoveryquery produced multi-megabyte JSON (a user with many sessions and
non-trivial first/last user-message previews), the captured buffer held
~2-4 MB instead of ~8 MB.
discoverOpenCodethen hit a truncated-JSONparse error, deliberately swallowed it (so discovery is best-effort), and
returned zero opencode rows. The picker showed every other provider and
not the opencode ones.
The fix drains stdout and stderr via
command.StdoutPipe()/StderrPipe()and copies them into the existingcappedBuffer/ a plainbytes.Bufferfrom goroutines, waiting on both beforecommand.Wait().This is the same pattern the codebase already uses (and the only call
site that did not).
cappedBuffer's enforcedmaxOpenCodeOutputcap ispreserved so a runaway CLI still gets rejected.
Verification
go build ./...cleango vet ./...cleango test ./internal/session/ -run "OpenCode|TestRunOpenCode|TestCappedBuffer|TestDiscoverFindsOpenCode|TestDecodeOpenCode"passes (12/12)~8.3 MB discovery JSON),
showagent list --jsonreturned 0 opencoderows before the fix and 137 after.
showagent listnow shows thesessions grouped under
/home/ww.The new
TestRunOpenCodeDrainsLargeOutputis a smoke test that pipes96 KiB through a fake
opencodeand asserts the full payload iscaptured. The exact race in
os/execis timing-dependent and notdeterministically reproducible in a unit test at this scale; the PR's
manual verification covers the failure mode that issue #10 reported.
Risk
Lowest possible: the helper is private and only called from the opencode
provider. The new path preserves the timeout, the
maxOpenCodeOutputcap, the stderr-detail-on-error rendering, and the partial-output
behavior when the child exits with a non-zero status. No other provider
is touched and no public surface changes.
Summary by CodeRabbit