fix: pass onWarn callback to BusClient for app.log routing (#21) - #44
Conversation
WalkthroughWalkthrough
ChangesWarning handler wiring from BusPublisher to plugin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related issues
Possibly related PRs
Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
Tip: You can configure your own custom pre-merge checks in the settings. Finishing TouchesGenerate docstrings
Generate unit tests (beta)
Simplify code
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bus-publisher.ts (1)
54-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReconnection loses the onWarn callback.
The reconnection logic calls
this.init()without preserving theoptsparameter. After a reconnection, bus warnings will no longer be routed through the application's structured logging system, silently breaking the feature introduced by this PR.Store the
optsparameter as a class field during the initialinit()call, then reuse it during reconnection.Proposed fix to preserve onWarn across reconnections
export class BusPublisher { private bus: BusClient | null = null; private lastPublish = 0; private publishInterval = 500; // ms — debounce private reconnecting = false; + private initOpts?: { onWarn?: (message: string, ...args: unknown[]) => void }; async init(opts?: { onWarn?: (message: string, ...args: unknown[]) => void }): Promise<void> { + this.initOpts = opts; try { this.bus = await BusClient.connect({ timeoutMs: 3000, onWarn: opts?.onWarn }); } catch { this.bus = null; } } // ... in reconnection logic: setTimeout(async () => { this.reconnecting = false; - await this.init(); + await this.init(this.initOpts); }, 5000);Prompt for 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. In `@src/bus-publisher.ts` around lines 54 - 57, The reconnection logic in the setTimeout callback is calling this.init() without passing the opts parameter, which causes the onWarn callback to be lost after reconnection. Store the opts parameter as a class field (e.g., this.opts) when init() is first called, then modify the setTimeout callback in the reconnection block to pass this stored opts value back to this.init() so that the onWarn callback is preserved across reconnections.src/four-opencode-token-budget-guard.ts (1)
80-81:⚠️ Potential issue | 🔴 CriticalPlugin does not register the required
chat.messageHook—currently useseventhook processingmessage.part.updated.The coding guideline requires registering a
chat.messageHook, but the plugin implementation at line 81 returns aneventhook that processes individualmessage.part.updatedevents. This causes the documented limitation noted in ROADMAP.md: the hook fires per-chunk and lacks full message context, preventing policies that inspect assembled message content from triggering. The plugin should be updated to register thechat.messageHook or a pre-request hook variant (e.g.,chat.message.before) that provides complete message context before token waste occurs.Prompt for 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. In `@src/four-opencode-token-budget-guard.ts` around lines 80 - 81, The plugin currently registers an event hook that processes message.part.updated events, which fires per-chunk and lacks full message context. Replace the event hook processing message.part.updated with a chat.message Hook (or a pre-request variant like chat.message.before) to receive complete message context before token consumption occurs. This change will allow policies to inspect the fully assembled message content and enforce token budget constraints appropriately.Source: Coding guidelines
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 `@src/four-opencode-token-budget-guard.ts`:
- Line 40: The current implementation uses `args.map(String).join(" ")` to
format the details in the extra object, which produces unhelpful output like
"[object Object]" for complex objects. Replace the String coercion with
JSON.stringify to properly serialize and preserve the structure of complex
objects in the log details, making debugging information more informative.
- Around line 33-44: The busPublisher.init() call is not being awaited, which
creates a race condition where bus events published before initialization
completes may be dropped. Add the await keyword before busPublisher.init() to
ensure the initialization completes before the plugin continues processing. If
the containing function is not already declared as async, make it async to allow
the await to work properly.
---
Outside diff comments:
In `@src/bus-publisher.ts`:
- Around line 54-57: The reconnection logic in the setTimeout callback is
calling this.init() without passing the opts parameter, which causes the onWarn
callback to be lost after reconnection. Store the opts parameter as a class
field (e.g., this.opts) when init() is first called, then modify the setTimeout
callback in the reconnection block to pass this stored opts value back to
this.init() so that the onWarn callback is preserved across reconnections.
In `@src/four-opencode-token-budget-guard.ts`:
- Around line 80-81: The plugin currently registers an event hook that processes
message.part.updated events, which fires per-chunk and lacks full message
context. Replace the event hook processing message.part.updated with a
chat.message Hook (or a pre-request variant like chat.message.before) to receive
complete message context before token consumption occurs. This change will allow
policies to inspect the fully assembled message content and enforce token budget
constraints appropriately.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4401af9-92c3-416d-8006-c932aec9a1b1
Files selected for processing (2)
src/bus-publisher.tssrc/four-opencode-token-budget-guard.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/four-opencode-token-budget-guard.ts">
<violation number="1" location="src/four-opencode-token-budget-guard.ts:40">
P2: `JSON.stringify(a)` throws on circular objects. If a warning arg is circular, the entire `onWarn` callback throws, potentially breaking BusClient's internal warning handler.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| service: "tbg", | ||
| level: "warn", | ||
| message: msg, | ||
| extra: { details: args.map(a => typeof a === 'object' ? JSON.stringify(a) : String(a)).join(" ") } |
There was a problem hiding this comment.
P2: JSON.stringify(a) throws on circular objects. If a warning arg is circular, the entire onWarn callback throws, potentially breaking BusClient's internal warning handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/four-opencode-token-budget-guard.ts, line 40:
<comment>`JSON.stringify(a)` throws on circular objects. If a warning arg is circular, the entire `onWarn` callback throws, potentially breaking BusClient's internal warning handler.</comment>
<file context>
@@ -30,14 +30,14 @@ let currentSessionID = "";
level: "warn",
message: msg,
- extra: { details: args.map(String).join(" ") }
+ extra: { details: args.map(a => typeof a === 'object' ? JSON.stringify(a) : String(a)).join(" ") }
}
}).catch(() => {});
</file context>
| extra: { details: args.map(a => typeof a === 'object' ? JSON.stringify(a) : String(a)).join(" ") } | |
| extra: { details: args.map(a => { try { return typeof a === 'object' ? JSON.stringify(a) : String(a); } catch { return String(a); } }).join(" ") } |
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 `@src/bus-publisher.ts`:
- Around line 56-59: The reconnection logic in the setTimeout callback sets
this.reconnecting to false before the reconnect attempt completes, causing
permanent disconnection if init fails. Wrap the await this.init call in a
try-catch block and only set this.reconnecting to false after the init call
succeeds. If init fails in the catch block, handle the error appropriately (such
as logging it) to allow subsequent reconnection attempts to be scheduled
properly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e93716c-395d-4deb-9e39-44177b31d578
Files selected for processing (2)
src/bus-publisher.tssrc/four-opencode-token-budget-guard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/four-opencode-token-budget-guard.ts
| setTimeout(async () => { | ||
| this.reconnecting = false; | ||
| await this.init(); | ||
| await this.init({ onWarn: this.onWarn }); | ||
| }, 5000); |
There was a problem hiding this comment.
Reconnect attempts can stop permanently after a failed retry.
Line 57 resets this.reconnecting before the reconnect attempt finishes. If init fails at Line 58, this.bus remains null and publish exits early at Line 27, so no further retries are scheduled.
Proposed fix
- setTimeout(async () => {
- this.reconnecting = false;
- await this.init({ onWarn: this.onWarn });
- }, 5000);
+ const reconnect = async (): Promise<void> => {
+ await this.init({ onWarn: this.onWarn });
+ if (!this.bus) {
+ setTimeout(reconnect, 5000);
+ return;
+ }
+ this.reconnecting = false;
+ };
+ setTimeout(reconnect, 5000);Committable suggestion
IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout(async () => { | |
| this.reconnecting = false; | |
| await this.init(); | |
| await this.init({ onWarn: this.onWarn }); | |
| }, 5000); | |
| const reconnect = async (): Promise<void> => { | |
| await this.init({ onWarn: this.onWarn }); | |
| if (!this.bus) { | |
| setTimeout(reconnect, 5000); | |
| return; | |
| } | |
| this.reconnecting = false; | |
| }; | |
| setTimeout(reconnect, 5000); |
Prompt for 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.
In `@src/bus-publisher.ts` around lines 56 - 59, The reconnection logic in the
setTimeout callback sets this.reconnecting to false before the reconnect attempt
completes, causing permanent disconnection if init fails. Wrap the await
this.init call in a try-catch block and only set this.reconnecting to false
after the init call succeeds. If init fails in the catch block, handle the error
appropriately (such as logging it) to allow subsequent reconnection attempts to
be scheduled properly.
1 participant
Closes #21
Changes
BusPublisher.init()now accepts optionalonWarncallback, passed through toBusClient.connect()busPublisher.init()to inside the Plugin callback, passingctx.client.app.logas the warning loggerconsole.warn— bus warnings are routed through opencode's structured loggingSummary by cubic
Routes bus warnings to structured logs by passing an
onWarncallback toBusClient, removingconsole.warnin favor ofctx.client.app.log. Persists the callback across reconnects and awaits bus initialization in the plugin.BusPublisher.init(opts?)storesonWarnand forwards it toBusClient.connect()and on reconnects.ctx.clientis available; warnings go toctx.client.app.logwith safer arg formatting (JSON for objects,Stringfor others).Written for commit 0416924. Summary will update on new commits.
Summary by CodeRabbit
Release Notes