fix(opencode): sync auth client hotfixes#487
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR consolidates server authentication across the app, CLI, and SDK layers. It introduces centralized authentication utilities for token encoding/decoding, exports a new server connection resolution function, implements a custom server middleware for Basic Auth validation, and integrates auth headers throughout CLI commands and the plugin system. ChangesCentralized Authentication System
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Code Review
This pull request centralizes server authentication logic, introducing utilities for encoding/decoding auth tokens and managing basic auth headers across the CLI, plugins, and web app. Key changes include refactoring terminal WebSocket URL generation, handling auth_token query parameters in the server middleware, and improving error formatting for opaque objects. Review feedback identifies a critical issue where mutating request headers in the middleware will cause a TypeError in Fetch-based runtimes like Bun. A more robust implementation using regex for credential extraction and avoiding direct header mutation was suggested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/src/util/error.ts (1)
41-43: 💤 Low value
return "unknown error"is essentially unreachable.
errorFormatonly returns an empty string forerrorFormat("")(a bare empty string as the thrown value). For every object and non-object input reachable here,formattedis always truthy, making the"unknown error"fallback dead code in all practical paths. Not a functional bug — the guard is strictly safer than the previous unconditionalreturn errorFormat(error)— but the comment that it guards against a real case is misleading if anyone reads this later.🤖 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 `@packages/opencode/src/util/error.ts` around lines 41 - 43, The fallback return "unknown error" after calling errorFormat is effectively unreachable and misleading; replace the ternary-style guard with a direct return of the formatted value (i.e., return formatted) or, if you intended to treat a bare empty-string throw specially, add an explicit check for that case (e.g., if (formatted === "") handle empty-string case) so the logic in errorFormat and the variable formatted are consistent and the dead-code fallback is removed.packages/app/src/utils/server.test.ts (1)
19-23: 💤 Low valueConsider adding test for explicit username and password with colons.
The test suite covers the default username case but misses testing explicit username encoding. Also, passwords containing colons are valid and should be handled correctly by
authFromToken(the colon separator usesindexOfwhich finds the first colon, so this should work).📝 Suggested additional tests
describe("authTokenFromCredentials", () => { test("encodes credentials with the default username", () => { expect(authTokenFromCredentials({ password: "secret" })).toBe(btoa("opencode:secret")) }) + + test("encodes credentials with explicit username", () => { + expect(authTokenFromCredentials({ username: "kit", password: "secret" })).toBe(btoa("kit:secret")) + }) }) + +describe("authFromToken edge cases", () => { + test("handles password containing colons", () => { + expect(authFromToken(btoa("user:pass:with:colons"))).toEqual({ + username: "user", + password: "pass:with:colons", + }) + }) +})🤖 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 `@packages/app/src/utils/server.test.ts` around lines 19 - 23, Add tests that pass an explicit username to authTokenFromCredentials and verify it encodes using btoa("username:password"), and add a test where the password contains one or more colons to ensure authFromToken correctly decodes using the first colon (it uses indexOf) — specifically add assertions calling authTokenFromCredentials({ username: "alice", password: "p:ass:word" }) and then decode with authFromToken(token) (or compare to btoa("alice:p:ass:word")) to confirm the round-trip; reference authTokenFromCredentials and authFromToken to locate the functions to test.
🤖 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 `@packages/opencode/src/server/middleware.ts`:
- Line 50: The auth-scheme check in middleware.ts uses header?.startsWith("Basic
") which rejects case-insensitive variants; update the conditional that checks
the header (the variable header in the middleware) to perform a case-insensitive
match (e.g., compare header.toLowerCase().startsWith("basic ") or use a
case-insensitive regex like /^basic /i) so valid "basic" tokens (e.g., "basic",
"BASIC") are accepted and still return c.text("Unauthorized", 401) when missing
or not a Basic auth header.
---
Nitpick comments:
In `@packages/app/src/utils/server.test.ts`:
- Around line 19-23: Add tests that pass an explicit username to
authTokenFromCredentials and verify it encodes using btoa("username:password"),
and add a test where the password contains one or more colons to ensure
authFromToken correctly decodes using the first colon (it uses indexOf) —
specifically add assertions calling authTokenFromCredentials({ username:
"alice", password: "p:ass:word" }) and then decode with authFromToken(token) (or
compare to btoa("alice:p:ass:word")) to confirm the round-trip; reference
authTokenFromCredentials and authFromToken to locate the functions to test.
In `@packages/opencode/src/util/error.ts`:
- Around line 41-43: The fallback return "unknown error" after calling
errorFormat is effectively unreachable and misleading; replace the ternary-style
guard with a direct return of the formatted value (i.e., return formatted) or,
if you intended to treat a bare empty-string throw specially, add an explicit
check for that case (e.g., if (formatted === "") handle empty-string case) so
the logic in errorFormat and the variable formatted are consistent and the
dead-code fallback is removed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7bdfcb9-65e9-4381-bdd6-6d7e3c6636a5
📒 Files selected for processing (18)
packages/app/src/components/terminal.tsxpackages/app/src/context/server.test.tspackages/app/src/context/server.tsxpackages/app/src/entry.tsxpackages/app/src/utils/server.test.tspackages/app/src/utils/server.tspackages/app/src/utils/terminal-websocket-url.test.tspackages/app/src/utils/terminal-websocket-url.tspackages/opencode/src/cli/cmd/acp.tspackages/opencode/src/cli/cmd/providers.tspackages/opencode/src/cli/cmd/run.tspackages/opencode/src/plugin/index.tspackages/opencode/src/server/auth.tspackages/opencode/src/server/middleware.tspackages/opencode/src/util/error.tspackages/opencode/test/server/auth.test.tspackages/opencode/test/util/error.test.tspackages/sdk/js/src/v2/client.ts
Root cause: - SessionProcessor correctly attached harness-owned diagnostics.loop to tool parts when tool-call events arrived. - Bash then streamed metadata updates through Tool.Context.metadata while the part was still running. - SessionPrompt replaced state.metadata with the tool-provided metadata object, so those Bash output/description updates erased diagnostics.loop before completion. - Because completed successful Bash parts no longer carried loop diagnostics, the existing successful exact-input loop strategy had no records to count, so Kimi K2.6 could repeat the same successful Bash command many times without reminder, block, or stop. User impact: - In issue #489, the review session repeated the same git diff command 39 times. - The expected gate sequence was reminder at the 3rd repeat, synthetic block at the 4th, and synthetic stop with a visible summary at the 5th. Fix: - Preserve harness-owned metadata in SessionPrompt's Tool.Context.metadata update path. - Reuse SessionDiagnostics.mergeMetadata so tool-owned fields such as Bash output, exit, description, truncated, and outputPath can still update while diagnostics.loop and diagnostics.failure survive. - Keep the fix at the shared session/tool metadata boundary instead of hard-coding loop diagnostics into Bash. Scope intentionally not changed: - No Bash permission, shell parsing, timeout, truncation, or process execution changes. - No loop threshold changes: reminder remains 3, block remains 4, stop remains 5. - No PR #487 auth/client code changes. - No metadata API migration or broader diagnostics refactor. Regression coverage: - Added a prompt-effect regression covering repeated successful Bash calls where Bash updates metadata during execution. - The test asserts the 3/4/5 reminder-block-stop behavior and verifies completed Bash parts retain both diagnostics.loop hashes and Bash metadata. Verification: - RED before fix: the new regression failed with 5 completed Bash calls instead of the expected 3, proving the old gate did not stop the loop. - GREEN after fix: the focused regression passed with 1 pass and 27 assertions. - Focused suite passed: bun test test/session/prompt-effect.test.ts test/session/diagnostics.test.ts test/session/loop-gate.test.ts test/tool/bash.test.ts, 129 pass, 0 fail, 388 assertions. - Typecheck passed: bun run typecheck from packages/opencode. - Diff whitespace check passed: git diff --check. - PR CI passed on GitHub before merge, including typecheck, unit-app, unit-opencode, unit-desktop, desktop-smoke, e2e-artifacts, CodeQL, dependency-review, commit-lint, and PR title checks. Closes #489.
Summary
Why
#477 tracks PawWork's upstream opencode sync after 17701628bd. PR #482 already landed the provider/model slice. This PR implements the next bounded auth/client slice while excluding provider/model, runtime/session/tool safety, desktop packaging, HttpApi listener, generated SDK, and dependency changes.
Related Issue
Refs #477
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Risk Notes
packages/opencode/src/cli/cmd/tui/*and HttpApi listener paths were behavior references only because PawWork does not have matching local paths.How To Verify
Screenshots or Recordings
Not applicable. This is credential handling and client auth behavior, with no visible UI change.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Improvements
Tests