-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(background-agent): add HTTP Basic Auth for background agent API calls #849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
(this is my local version, could be different from current upstream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files
Confidence score: 3/5
src/shared/authenticated-fetch.tsdrops any overrides supplied tofetch(Request, init), so headers, method, body, or abort signals can’t be customized when reusing a Request, which is a concrete user-facing regression.- Given the medium severity and high confidence of this behavior change, the current merge poses some risk until override handling matches native fetch semantics.
- Pay close attention to
src/shared/authenticated-fetch.ts– fix the missing init overrides logic.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/shared/authenticated-fetch.ts">
<violation number="1" location="src/shared/authenticated-fetch.ts:33">
P2: Init overrides are ignored when input is a Request, diverging from fetch(Request, init) semantics (headers/method/body/signal overrides are dropped).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
@Gladdonilli I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/shared/authenticated-fetch.ts">
<violation number="1" location="src/shared/authenticated-fetch.ts:48">
P2: Nullish coalescing prevents `init.body = null` from overriding the cloned body, so callers cannot clear an inherited body</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 issues found across 6 files
Confidence score: 2/5
- Rebuilding requests in
src/shared/authenticated-fetch.tscan crash Node fetch whenever a body stream is present becauseduplexisn’t set, so any non-empty POST/PUT fails outright. - The same file leaves multiple Authorization headers when overriding, meaning downstream services may receive malformed credentials and reject the call.
assets/oh-my-opencode.schema.jsondrops the documentedpreemptive-compactionenum value, which will make existing user configs invalid until they change their settings.- Pay close attention to
src/shared/authenticated-fetch.ts,assets/oh-my-opencode.schema.json- they’re introducing user-visible runtime and compatibility regressions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/delegate-task/tools.ts">
<violation number="1" location="src/tools/delegate-task/tools.ts:566">
P2: Misleading log tag: uses "[sisyphus_task]" instead of this tool’s "[delegate_task]", hindering correct log filtering/alerts.</violation>
</file>
<file name="assets/oh-my-opencode.schema.json">
<violation number="1" location="assets/oh-my-opencode.schema.json:75">
P2: Removed `preemptive-compaction` from `disabled_hooks` enum breaks schema compatibility; documented configs using it now fail validation</violation>
</file>
<file name="src/shared/authenticated-fetch.ts">
<violation number="1" location="src/shared/authenticated-fetch.ts:38">
P1: Authorization header not overridden: uppercase key is added alongside existing lowercase header, leading to combined Authorization values when Request is rebuilt.</violation>
<violation number="2" location="src/shared/authenticated-fetch.ts:48">
P2: Request body merging ignores null and may pass a body with GET/HEAD overrides, causing runtime errors</violation>
<violation number="3" location="src/shared/authenticated-fetch.ts:57">
P1: Reconstructed Request passes a streaming body without required `duplex`, causing Node fetch to throw for any non-empty body</violation>
</file>
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:90">
P2: Authenticated client captures server password only at construction, so if the password is populated after plugin init (as documented), background agent requests will keep using an unauthenticated client and persistently 401.</violation>
</file>
<file name="src/features/background-agent/manager.test.ts">
<violation number="1" location="src/features/background-agent/manager.test.ts:174">
P2: Tests construct BackgroundManager with a real server URL, causing resume/launch paths to fire real HTTP requests via authClient instead of the stubbed client</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
please resolve the cubic bot conversations @Gladdonilli |
im not sure If i actually recommend to merge this build even after fixes, I made this pr to show the one other person whos having auth issue and Q have you ever experienced the issues mentioned? |
|
I'll also add in my other patches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/lsp/client.ts">
<violation number="1" location="src/tools/lsp/client.ts:595">
P2: stop() now awaits shutdown/exit, but cleanup calls it without await before process.exit(), so graceful/forced termination may never run and LSP servers can be orphaned</violation>
<violation number="2" location="src/tools/lsp/client.ts:596">
P2: Shutdown request sends params as null, which violates JSON-RPC/LSP (params must be array/object or omitted) and may cause servers to reject shutdown.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Update: Rebased onto latest upstream/dev + added skills parsing fixThis PR has been rebased onto the latest upstream/dev branch (30+ new commits integrated) and includes an additional fix. Changes in this update:
Regarding cubic-dev-ai reviews:The reviews were on commits that are now obsolete after the rebase. Here's the status:
@cubic-dev-ai please re-review on the latest commits. |
@Gladdonilli I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 10 files
Confidence score: 3/5
- Creating the authenticated background client eagerly in
src/index.tsbypasses the intended lazy auth and risks caching an unauthenticated client, so user sessions could behave unexpectedly. src/features/background-agent/manager.tsstill mixesAuthorizationcasing, which can produce duplicate headers once merged by the SDK and break Basic Auth.- The polling loop in
src/tools/call-omo-agent/tools.tsignores API errors, so failures are reported as idle/timeout rather than surfaced to users, masking real outages. - Pay close attention to
src/index.ts,src/features/background-agent/manager.ts,src/tools/call-omo-agent/tools.ts- each needs fixes to avoid auth and error-reporting regressions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:231">
P1: Authenticated background client is now created during plugin init, defeating the intended post-init lazy auth and potentially caching an unauthenticated client</violation>
</file>
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:31">
P1: Use a consistent lowercase `authorization` key for the Basic Auth header to avoid duplicate/combined Authorization values when the SDK merges headers.</violation>
</file>
<file name="src/tools/call-omo-agent/tools.ts">
<violation number="1" location="src/tools/call-omo-agent/tools.ts:229">
P2: Polling loop ignores errors from status/messages calls, misreporting API failures as idle/timeout</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Saw a report about this problem. Prolly fix the cubic review then its lgtm |
|
@cubic-dev-ai please re-review on the latest commits. |
@Gladdonilli I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 12 files
Confidence score: 3/5
- Explicitly setting
init.bodytonullis ignored and reuses the previous payload, so requests meant to clear the body could inadvertently send stale data—worth fixing before release. - Pay close attention to
src/shared/authenticated-fetch.ts-nullbodies currently reuse previous payloads instead of clearing them.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/shared/authenticated-fetch.ts">
<violation number="1" location="src/shared/authenticated-fetch.ts:50">
P2: `init.body` can be `null`, but `init?.body ?? cloned.body` treats `null` as missing and reuses the original body. Use an `undefined` check so `null` correctly overrides the body.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I’m down to help test this, but I’m a bit stuck on the specific failure state. My recent PR cleared out a few of these bugs, but the auth header issue is definitely still there. @Gladdonilli what’s the actual indicator that auth is failing? I’d expect the agents to stop reporting back to the TUI, but I'm still seeing them check in. I’m clearly missing a piece of the puzzle here. Let me know what to look for so I can help knock this out. Thanks! |
Summary
This PR fixes background agent authentication failures. Background agents were failing silently because API calls to the OpenCode server lacked authentication headers.
Changes
1. Background Agent Authentication Fix (
src/features/background-agent/manager.ts)Problem: Background agent API calls (session.create, session.prompt, session.messages, etc.) were failing with 401 Unauthorized when OpenCode server has authentication enabled.
Root Cause: The
BackgroundManagerwas usingctx.clientdirectly, which doesn't include HTTP Basic Auth headers required by the server.Solution:
createAuthenticatedClient()helper function that creates an OpenCode SDK client with HTTP Basic Auth headersOPENCODE_SERVER_USERNAME(default: "opencode") andOPENCODE_SERVER_PASSWORDenvironment variablesauthClientinstance viagetAuthenticatedClient()methodAPI calls now authenticated:
session.get()- fetching parent session infosession.create()- creating background sessionssession.prompt()- sending prompts to background agentssession.todo()- checking session todossession.messages()- fetching session messagessession.status()- polling task status2. Tool Authentication Fixes
delegate_tasktool (src/index.ts)createDelegateTask()to usebackgroundManager.getAuthenticatedClient()instead ofctx.clientlook_attool (src/tools/look-at/tools.ts)client: SdkOpencodeClientparameter tocreateLookAt()session.get,session.create,session.prompt,session.messages) now use authenticated clientsrc/index.tsto passbackgroundManager.getAuthenticatedClient()call_omo_agenttool (src/tools/call-omo-agent/tools.ts)client: SdkOpencodeClientparameter toexecuteSync()backgroundManager.launch()which handles auth internallyBackground tools (
background_output,background_cancel)createBackgroundTools()call to use authenticated client3. Shared Authenticated Fetch Utility (
src/shared/authenticated-fetch.ts)New file providing a reusable
createAuthenticatedFetch()function:Requestobjects (SDK style) andurl + initstyle callsduplex: 'half'for streaming body supportauthorizationheader key to properly override existing headers4. LSP Client Shutdown Sequence Fix (
src/tools/lsp/client.ts)Problem: LSP client was using
notify("shutdown")which violates LSP spec.Solution:
shutdownto be a REQUEST (not notification) - must wait for responsesend("shutdown")→ wait for response →notify("exit")5. Schema and Migration Cleanup
assets/oh-my-opencode.schema.jsondisabled_hooksenum:sisyphus-task-retry→delegate-task-retrypreemptive-compactionfrom enum (feature was removed)src/shared/migration.ts../tools/sisyphus-task/constants→../tools/delegate-task/constants6. Improved Exception Logging (
src/tools/delegate-task/tools.ts)Added logging to the swallowed exception in parent session lookup:
7. Review Feedback Addressed
duplex: 'half'for streaming body in Request reconstructionauthorizationkey to properly override headers[sisyphus_task]to[delegate_task]preemptive-compactionto disabled_hooks enumTesting
Breaking Changes
None. Auth is only added when
OPENCODE_SERVER_PASSWORDis set.