fix: skip response body cloning in logResult when debug logging is disabled - #8
Open
peeyush-ciridae wants to merge 1 commit into
Open
peeyush-ciridae wants to merge 1 commit into
peeyush-ciridae wants to merge 1 commit into
Conversation
…sabled
logResult() unconditionally cloned and read result.response before
passing it to logInject(), which itself is a no-op unless
OPENCODE_INTERCOM_DEBUG=1. By the time client.session.list(),
client.tui.appendPrompt(), etc. return, the SDK has already consumed
the underlying response body, so .clone() throws
'TypeError: Body is disturbed or locked' (ERR_BODY_ALREADY_USED).
This throw is uncaught at two call sites with concrete user-facing
impact:
- resolveActiveSessionID(): the exception aborts the function before
setActiveSession() runs, so the boot-time session resolution in the
top-level connect() IIFE always fails, logging 'Failed to start
OpenCode intercom listener' on every plugin start.
- injectInbound(): in a TTY session, client.tui.appendPrompt()
succeeds (the inbound message text is appended to the prompt input),
but the immediately following 'await logResult("inject.append", ...)'
throws before the 'if (appended.data === true)' check that guards
client.tui.submitPrompt(). The catch block only calls the
debug-gated logInject(), so the failure is silent and submitPrompt()
is never invoked -- the message sits in the input box requiring a
manual Enter instead of being submitted automatically.
Guard logResult() the same way logInject() already guards itself, so
the unnecessary response clone/read never runs when debug logging is
off.
Verified: dist rebuilt via 'npm run build', 'npm run typecheck' clean,
'npm test' unaffected (3 pre-existing broker/*.integration.test.ts
failures reproduce identically on unpatched main; unrelated to this
change).
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
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
logResult()unconditionally clones and readsresult.responsebefore handing it tologInject(), which is itself already a no-op unlessOPENCODE_INTERCOM_DEBUG=1:By the time
client.session.list(),client.tui.appendPrompt(),client.tui.submitPrompt(), etc. resolve, the SDK has already consumed the underlying response body while parsing.data. Calling.clone()on an already-consumedResponsethrows synchronously:The trailing
.catch()is attached to.clone().text(), so it doesn't catch a synchronous throw from.clone()itself. This propagates out oflogResultuncaught.Impact
Two concrete, user-facing consequences of the same root cause:
resolveActiveSessionID()— called once at boot inside the top-levelconnect()IIFE. The exception aborts the function beforesetActiveSession()runs, so this always fails, logging"Failed to start OpenCode intercom listener"on every single plugin start (regardless ofOPENCODE_INTERCOM_DEBUG).injectInbound()— in a TTY session,client.tui.appendPrompt()succeeds (the inbound message text is appended to the user's prompt input), but the very next line,await logResult("inject.append", ...), throws before theif (appended.data === true)check that guardsclient.tui.submitPrompt(). Thecatchblock only calls the debug-gatedlogInject(), so the failure is silent —submitPrompt()is never invoked. The inbound message sits in the prompt box requiring the user to manually press Enter, instead of being auto-submitted.Fix
Guard
logResult()the same waylogInject()already guards itself, so the unnecessary response clone/read (and its crash) never happens when debug logging is off:Minimal, single call site, matches the existing
logInjectearly-return pattern already in the file.Testing
npm run build— regenerateddist/plugin.mjsanddist/index.mjsfrom source via the project's ownscripts/build.mjs.npm run typecheck— clean, no errors.npm test— 140/143 pass. The 3 failures (broker/remote-access.integration.test.ts,broker/session-collision.integration.test.ts) reproduce identically on unpatchedmainin this sandbox (child-process broker spawn failing,"broker exited early: 1") — pre-existing environment issue unrelated to this change, verified by stashing the fix and re-running the same files.0.10.0build (before rebasing this fix onto source): applying the equivalent one-line guard to the installeddist/plugin.mjseliminated the"Failed to start OpenCode intercom listener"crash on everyopencode runinvocation.Notes
Verifying the
injectInbound→submitPromptfix end-to-end requires a live inbound Intercom message in a TTY session; I confirmed the mechanism precisely from source (this PR's description above) and confirmed the boot-time crash is eliminated, but haven't captured a recording of the TUI auto-submit specifically.