fix: Add basic auth support to server health checks and reconnection logic#3
Open
mskoryh wants to merge 1 commit intoAlaeddineMessadi:mainfrom
Open
fix: Add basic auth support to server health checks and reconnection logic#3mskoryh wants to merge 1 commit intoAlaeddineMessadi:mainfrom
mskoryh wants to merge 1 commit intoAlaeddineMessadi:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes introduce optional HTTP Basic Authentication support across client initialization and server management. Username and password credentials are now accepted by Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
tests/server-manager.test.ts (1)
270-291: Add one auth regression test for the auto-start polling path.This test validates the initial check, but not the post-spawn health polling path. Add a case where first check fails, server starts, and subsequent health polls require
Authorizationto pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server-manager.test.ts` around lines 270 - 291, Add a new test in tests/server-manager.test.ts that uses ensureServer to exercise the auto-start polling path: simulate the initial health check failing (use the existing mock helper like mockFetchUnhealthy or a custom mock), make the spawn/start path succeed (mock the spawn or startServer behavior), then have subsequent health poll responses require an Authorization header to return healthy (use mockFetchHealthy that asserts the Authorization header). Ensure the test asserts that ensureServer ultimately returns running: true and version, and that fetchMock calls for the post-spawn health polls include the Authorization header in the request options (referencing ensureServer, mockFetchHealthy, and the spawn/start mock helpers to locate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server-manager.ts`:
- Around line 55-70: The abort timeout (variable timeout) is not cleared if
fetch(url, { ..., signal: controller.signal }) throws, leaving timers
accumulating; wrap the fetch call in a try...finally (or ensure a finally block)
that calls clearTimeout(timeout) and optionally controller.abort() cleanup so
clearTimeout(timeout) always runs; update the block around url, controller,
timeout and the fetch call to move clearTimeout(timeout) into the finally to
guarantee the timer is cleared on success or error.
- Around line 149-154: startServer() still calls waitForHealthy() and
isServerRunning(baseUrl) without passing the new credentials, causing failures
against protected health endpoints; update startServer() (and any startup
polling path) to forward the username and password into waitForHealthy(baseUrl,
timeoutMs, username, password) and to call isServerRunning(baseUrl, username,
password) so auth is used during polling and the protected health check
succeeds.
---
Nitpick comments:
In `@tests/server-manager.test.ts`:
- Around line 270-291: Add a new test in tests/server-manager.test.ts that uses
ensureServer to exercise the auto-start polling path: simulate the initial
health check failing (use the existing mock helper like mockFetchUnhealthy or a
custom mock), make the spawn/start path succeed (mock the spawn or startServer
behavior), then have subsequent health poll responses require an Authorization
header to return healthy (use mockFetchHealthy that asserts the Authorization
header). Ensure the test asserts that ensureServer ultimately returns running:
true and version, and that fetchMock calls for the post-spawn health polls
include the Authorization header in the request options (referencing
ensureServer, mockFetchHealthy, and the spawn/start mock helpers to locate
code).
🪄 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
Run ID: 38a6664f-aabf-4260-853b-cf48363c1296
📒 Files selected for processing (4)
src/client.tssrc/index.tssrc/server-manager.tstests/server-manager.test.ts
Comment on lines
55
to
70
| const url = `${baseUrl.replace(/\/$/, "")}/global/health`; | ||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 3_000); | ||
|
|
||
| const headers: Record<string, string> = {}; | ||
| if (password) { | ||
| const user = username ?? "opencode"; | ||
| headers["Authorization"] = "Basic " + Buffer.from(`${user}:${password}`).toString("base64"); | ||
| } | ||
|
|
||
| const res = await fetch(url, { | ||
| method: "GET", | ||
| headers, | ||
| signal: controller.signal, | ||
| }); | ||
| clearTimeout(timeout); |
There was a problem hiding this comment.
Clear the health-check timeout on all paths.
If fetch() throws before Line 70, the abort timer is left pending. This can accumulate timers during repeated polling.
Suggested fix
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 3_000);
@@
- const res = await fetch(url, {
- method: "GET",
- headers,
- signal: controller.signal,
- });
- clearTimeout(timeout);
+ let res: Response;
+ try {
+ res = await fetch(url, {
+ method: "GET",
+ headers,
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeout);
+ }📝 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.
Suggested change
| const url = `${baseUrl.replace(/\/$/, "")}/global/health`; | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 3_000); | |
| const headers: Record<string, string> = {}; | |
| if (password) { | |
| const user = username ?? "opencode"; | |
| headers["Authorization"] = "Basic " + Buffer.from(`${user}:${password}`).toString("base64"); | |
| } | |
| const res = await fetch(url, { | |
| method: "GET", | |
| headers, | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| const url = `${baseUrl.replace(/\/$/, "")}/global/health`; | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 3_000); | |
| const headers: Record<string, string> = {}; | |
| if (password) { | |
| const user = username ?? "opencode"; | |
| headers["Authorization"] = "Basic " + Buffer.from(`${user}:${password}`).toString("base64"); | |
| } | |
| let res: Response; | |
| try { | |
| res = await fetch(url, { | |
| method: "GET", | |
| headers, | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server-manager.ts` around lines 55 - 70, The abort timeout (variable
timeout) is not cleared if fetch(url, { ..., signal: controller.signal })
throws, leaving timers accumulating; wrap the fetch call in a try...finally (or
ensure a finally block) that calls clearTimeout(timeout) and optionally
controller.abort() cleanup so clearTimeout(timeout) always runs; update the
block around url, controller, timeout and the fetch call to move
clearTimeout(timeout) into the finally to guarantee the timer is cleared on
success or error.
Comment on lines
+149
to
+154
| username?: string, | ||
| password?: string, | ||
| ): Promise<boolean> { | ||
| const deadline = Date.now() + timeoutMs; | ||
| while (Date.now() < deadline) { | ||
| const { healthy } = await isServerRunning(baseUrl); | ||
| const { healthy } = await isServerRunning(baseUrl, username, password); |
There was a problem hiding this comment.
Auth propagation is still incomplete in startup polling path.
waitForHealthy now accepts credentials (Line 149-154), but startServer() still calls it without auth and later calls isServerRunning(baseUrl) without auth. On protected health endpoints, startup can falsely fail.
Suggested fix
export async function startServer(
binaryPath: string,
baseUrl: string,
timeoutMs: number = DEFAULT_STARTUP_TIMEOUT_MS,
+ username?: string,
+ password?: string,
): Promise<{ version?: string }> {
@@
const healthy = await Promise.race([
- waitForHealthy(baseUrl, timeoutMs),
+ waitForHealthy(baseUrl, timeoutMs, username, password),
earlyExit.catch(() => false as const),
]);
@@
- const status = await isServerRunning(baseUrl);
+ const status = await isServerRunning(baseUrl, username, password);
return { version: status.version };
}
@@
- const result = await startServer(binaryPath, baseUrl, timeoutMs);
+ const result = await startServer(
+ binaryPath,
+ baseUrl,
+ timeoutMs,
+ opts.username,
+ opts.password,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server-manager.ts` around lines 149 - 154, startServer() still calls
waitForHealthy() and isServerRunning(baseUrl) without passing the new
credentials, causing failures against protected health endpoints; update
startServer() (and any startup polling path) to forward the username and
password into waitForHealthy(baseUrl, timeoutMs, username, password) and to call
isServerRunning(baseUrl, username, password) so auth is used during polling and
the protected health check succeeds.
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
1 participant
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.
Description:
This PR fixes an issue where the OpenCode client couldn't connect to remote OpenCode servers protected by HTTP basic authentication. Previously, the
isServerRunning()function made unauthenticated requests to the/global/healthendpoint, causing it to incorrectly report the server as unhealthy when auth was required.Issue: #4
Changes
isServerRunning()to accept optionalusernameandpasswordparameters and include anAuthorizationheader when credentials are providedensureServer()andwaitForHealthy()to propagate auth parameters through the server startup flowOpenCodeClientto store credentials and use them during reconnection attemptsKey Features
Testing
Fixes connectivity issues when using
OPENCODE_BASE_URLwith protected remote OpenCode servers.Summary by CodeRabbit