fix: improve error messages for plugin installation failures#1
Conversation
Summary of ChangesHello @Barneyjm, 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 significantly improves the handling of authentication plugin installation failures by offering users clearer, more detailed error messages that include specific troubleshooting guidance. Additionally, it introduces new mechanisms, both programmatic and via a dedicated API endpoint, to expose information about failed plugins, aiding in diagnostics and debugging. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the error handling for plugin installation failures by providing clearer, more actionable error messages. The addition of the getFailedPlugins function and the /plugin/failed API endpoint are excellent for diagnostics and observability. The code is well-structured and the changes are easy to follow.
I have two main suggestions. First, a minor improvement for robustness in getFailedPlugins to prevent potential mutations of the internal state. Second, and more importantly, the new tests don't seem to cover the failure scenarios they are intended for, which leaves the core error-handling logic untested. Addressing this testing gap would significantly increase confidence in the new functionality.
| import { describe, expect, test, mock } from "bun:test" | ||
|
|
||
| // Mock the dependencies that would cause initialization issues | ||
| mock.module("../../src/bun/index", () => ({ | ||
| BunProc: { | ||
| install: async (pkg: string, _version?: string) => { | ||
| // Simulate installation failure for test packages | ||
| if (pkg.includes("test-fail")) { | ||
| throw new Error("ECONNREFUSED - simulated network failure") | ||
| } | ||
| return `/fake/path/${pkg}` | ||
| }, | ||
| which: () => process.execPath, | ||
| run: async () => ({ exitCode: 0 }), | ||
| }, | ||
| })) | ||
|
|
||
| // Import after mocks are set up | ||
| const { Plugin } = await import("../../src/plugin") | ||
|
|
||
| describe("Plugin.getFailedPlugins", () => { | ||
| test("returns an array", () => { | ||
| const failed = Plugin.getFailedPlugins() | ||
| expect(Array.isArray(failed)).toBe(true) | ||
| }) | ||
|
|
||
| test("returns readonly array", () => { | ||
| const failed = Plugin.getFailedPlugins() | ||
| // TypeScript ensures this is readonly, but we can verify it's not directly mutable | ||
| expect(typeof failed).toBe("object") | ||
| }) | ||
| }) | ||
|
|
||
| describe("Plugin.FailedPlugin interface", () => { | ||
| test("has expected structure", () => { | ||
| // This tests the type at runtime by creating a mock object | ||
| const mockFailedPlugin: Plugin.FailedPlugin = { | ||
| pkg: "test-package", | ||
| version: "1.0.0", | ||
| error: "Test error message", | ||
| authMethod: "Test OAuth", | ||
| } | ||
|
|
||
| expect(mockFailedPlugin.pkg).toBe("test-package") | ||
| expect(mockFailedPlugin.version).toBe("1.0.0") | ||
| expect(mockFailedPlugin.error).toBe("Test error message") | ||
| expect(mockFailedPlugin.authMethod).toBe("Test OAuth") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These tests are a good start, but they don't seem to be exercising the new failure-handling logic. The mock for BunProc.install is set up to throw an error for packages containing "test-fail", but the plugin installation logic that runs when ../../src/plugin is imported only attempts to install the BUILTIN plugins, none of which trigger the failure condition.
As a result, Plugin.getFailedPlugins() will always return an empty array in these tests, and the core logic for catching errors, populating the failedPlugins list, and formatting the error message remains untested.
To properly test this functionality, you could:
- Mock
Config.get()to return a configuration that includes a plugin designed to fail (e.g.,[email protected]). - Trigger the plugin installation logic again within a test case.
- Assert that
Plugin.getFailedPlugins()now contains the expected information about the failed plugin.
This would provide confidence that the error handling and diagnostic tracking work as intended.
| export function getFailedPlugins(): readonly FailedPlugin[] { | ||
| return failedPlugins | ||
| } |
There was a problem hiding this comment.
For better encapsulation and to prevent accidental mutations from consumers of this module, it's a good practice to return a shallow copy of the internal failedPlugins array instead of a direct reference.
While the readonly return type provides compile-time safety in TypeScript, it doesn't prevent mutation at runtime from JavaScript consumers or through type assertions (as any[]). Returning a copy ensures the internal state of the Plugin module remains fully protected.
| export function getFailedPlugins(): readonly FailedPlugin[] { | |
| return failedPlugins | |
| } | |
| export function getFailedPlugins(): readonly FailedPlugin[] { | |
| return [...failedPlugins] | |
| } | |
Barneyjm
force-pushed
the
claude/fix-issue-8297-krYLb
branch
from
4e74986 to
cb49b9d
Compare
When OAuth authentication plugins fail to install (e.g., due to corporate npm proxy issues), users now receive: - Clear explanation of which authentication method is unavailable - Actionable troubleshooting steps (check network, verify .npmrc, etc.) - Detailed error information for debugging Also adds: - `Plugin.getFailedPlugins()` function for programmatic access - `/plugin/failed` API endpoint for diagnostic purposes Fixes anomalyco#8297
Barneyjm
force-pushed
the
claude/fix-issue-8297-krYLb
branch
from
cb49b9d to
f59ebee
Compare
2 participants
When OAuth authentication plugins fail to install (e.g., due to
corporate npm proxy issues), users now receive:
Also adds:
Plugin.getFailedPlugins()function for programmatic access/plugin/failedAPI endpoint for diagnostic purposesFixes anomalyco#8297