fix(core): fail fast in MessageBus.request() on publish failure#26225
fix(core): fail fast in MessageBus.request() on publish failure#26225Adib234 wants to merge 3 commits into
Conversation
Link: #22588 Modified MessageBus.publish to re-throw errors and MessageBus.request to catch them, preventing 60s silent hangs. Also updated floating publish calls to prevent unhandled promise rejections.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues related to error handling and promise management within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Size Change: +845 B (0%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request improves error handling within the MessageBus by ensuring that the publish method re-throws errors, allowing callers to handle them explicitly. Changes include adding .catch() blocks to fire-and-forget calls and updating tests to expect rejected promises. A security concern was raised regarding the potential leakage of sensitive tool arguments in error messages, suggesting that these messages should be sanitized before being thrown to prevent data exposure in logs or telemetry.
- google-gemini/gemini-cli#26225: MessageBus.publish now throws after emit('error'), MessageBus.request awaits and rejects on publish failure to fix latent hang where invalid/policy-denied requests would set up a response listener that never resolved (merge-after-nits) - QwenLM/qwen-code#3749: AlreadyReportedError marker + handleError/JsonAdapter short-circuit + parseAndFormatApiError idempotency net to stop double-wrap '[API Error: [API Error: ...]]' and duplicate stderr emission in non-interactive mode (merge-as-is) - INDEX.md: append drip-188 section (8 entries, 4 merge-as-is + 4 merge-after-nits, 5/6 repo coverage with goose skipped)
- Added sanitization to MessageBus.publish() logs and error messages using sanitizeToolArgs to prevent secret leakage. - Refactored floating publish() calls to use a type-safe resiliency pattern (instanceof Promise) to handle test mocks and sync throws. Fixes CI failures and addresses security review feedback.
|
✅ 66 tests passed successfully on gemini-3-flash-preview. 🧠 Model Steering GuidanceThis PR modifies files that affect the model's behavior (prompts, tools, or instructions).
This is an automated guidance message triggered by steering logic signatures. |
cocosheng-g
left a comment
There was a problem hiding this comment.
please add more unit test coverage for the changes
- Added test case to verify request() rejects immediately upon publication failure. - Added test cases to verify redaction of sensitive data in debug logs and error messages. - Added test cases for pattern resiliency to handle non-promise returns and sync throws.
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
- google-gemini/gemini-cli#26225 MessageBus fail-fast — merge-after-nits - QwenLM/qwen-code#3627 macOS desktop installer — merge-after-nits - aaif-goose/goose#9014 unread state for background chats — merge-after-nits - charmbracelet/crush#2520 Ctrl+D quit prompt — merge-as-is
Summary
Fixes a bug where MessageBus.request() would silently hang for 60 seconds if publish() failed. It also fixes a potential process crash caused by unhandled rejections from floating publish() calls, and addresses security concerns regarding sensitive data in logs.
Details
Related Issues
Fixes #22588
How to Validate
Run unit tests for the core package:
npm test -w @google/gemini-cli-core -- src/confirmation-bus/message-bus.test.tsVerify that all 22 tests pass. Also run other affected tests:
npm test -w @google/gemini-cli-core -- src/scheduler/policy.test.ts src/tools/message-bus-integration.test.ts src/tools/web-fetch.test.tsPre-Merge Checklist