Broker error contract loses status/payload semantics and makes refresh clearing unsafe

[jumski]

Summary

The current broker chain collapses too many distinct failures into generic 401 / 502 responses and drops useful upstream payload details. The plugin client then clears local auth based on raw 400/401 status alone.

Taken together, this makes refresh-clearing logic unsafe and makes broker failures harder to diagnose and handle correctly.

This issue groups the contract problems that should be fixed together:

• P0-3 non-JSON failures rewritten to synthetic 502
• P1-3 local auth cleared on ambiguous 400/401
• P1-8 upstream 429 mapped to 502
• P1-11 upstream error payload discarded
• P2-4 response text failure detail masked

Expected Contract

docs/supabase-oauth-broker-contract.md:183-210 says the broker should return structured JSON errors with stable semantic codes and recommended status mapping:

• 400 = invalid request
• 401 = upstream auth rejection
• 429 = rate limited
• 502 = upstream transport/malformed upstream response

Actual Problems

1. Edge broker collapses upstream 400 and 401 into 401

supabase/functions/opencode-supabase-broker/supabase.ts

• 47-49: mapUpstreamStatus() returns 401 for both upstream 400 and 401, otherwise 502
• 79-80: exchange path emits code: "unauthorized" and generic message for upstream 400/401
• 112-117: refresh path does the same inline

This means malformed/invalid refresh requests and actual auth rejection are not clearly distinguished.

2. Edge broker turns upstream 429 into 502

supabase/functions/opencode-supabase-broker/supabase.ts

• 47-49: anything other than 400/401 becomes 502
• 112-117: refresh path repeats that behavior

Rate limiting loses its real status and semantics.

3. Edge broker discards upstream error payload details

supabase/functions/opencode-supabase-broker/supabase.ts

• 39-45: readUpstreamBody() throws a synthetic 502 on non-JSON body
• 77-80: exchange error path ignores parsed payload
• 110-117: refresh error path ignores parsed payload

So even when the upstream body is available, the broker emits only generic messages like:

• upstream token request was rejected
• upstream token request failed

4. Client broker parser rewrites non-JSON broker failures to synthetic 502

src/shared/broker.ts

• 130-144: if response.json() fails, client throws BrokerClientError({ code: "upstream_error", status: 502, message: "broker returned an invalid response" })
• 151-171: only after successful JSON parse does client preserve response.status and nested error.code / error.message

So a non-JSON broker failure loses both the original status and body.

5. Tool layer clears local auth from status alone

src/server/tools.ts

• 207-218: if broker refresh throws 400 or 401, plugin clears local auth with clearSavedAuth(input) and tells the user to reconnect

That is only safe if the broker contract guarantees those statuses exclusively mean "refresh token is invalid/revoked". The current broker implementation does not guarantee that.

6. Response text failure detail is masked

src/server/tools.ts

• 106: await response.text().catch(() => "")

If reading the response body fails, the original reason is dropped.

Reproduction Ideas

Non-JSON upstream failure

1. Make the broker receive an HTML/plaintext upstream error.
2. Observe edge broker/client behavior.

Expected

Preserve as much status/detail as possible under a structured broker error.

Actual

Client sees a generic synthetic 502.

Rate limiting

1. Force upstream 429.
2. Observe broker response.

Expected

429 plus stable semantic error code such as rate_limited.

Actual

Mapped to 502.

Refresh failure / local clear

1. Force a refresh failure that returns upstream 400 but does not semantically mean "reconnect required".
2. Observe ensureSupabaseToolAuth() failure path.

Expected

Plugin preserves local auth unless the broker explicitly says the refresh token is invalid/revoked.

Actual

Plugin clears local auth on raw 400/401 status.

Suggested Fix Boundary

This should be handled as one coordinated workstream with three subparts.

A. Edge broker contract

Files:

• supabase/functions/opencode-supabase-broker/supabase.ts

Goals:

• preserve 429 as 429
• stop collapsing all 400 into 401
• preserve useful upstream payload detail when safe
• emit stable semantic error codes

B. Client broker parser

Files:

• src/shared/broker.ts

Goals:

• preserve broker HTTP status whenever possible
• for non-JSON responses, fall back to raw text/status instead of synthetic blind 502
• keep machine-readable error codes stable

C. Auth-clearing policy

Files:

• src/server/tools.ts

Goals:

• clear local auth only on explicit broker semantics such as invalid_refresh_token / revoked refresh token
• stop clearing from status class alone

Acceptance Criteria

• Broker returns structured JSON errors consistent with docs/supabase-oauth-broker-contract.md.
• Upstream 429 is surfaced as 429.
• Upstream payload details are preserved when safe and useful.
• Non-JSON failures do not automatically become opaque synthetic 502s.
• Local auth is cleared only for explicit reconnect-required broker errors.
• Regression tests cover 400, 401, 429, non-JSON upstream errors, and refresh-token-invalid cases.

Severity

Under the current plugin-local-auth-only model this is primarily a local reliability / diagnostics issue, not a cross-workspace auth corruption issue. But it still matters because it can cause false disconnects and makes refresh handling brittle.

[jumski]

Status update from issue audit:

This issue is still valid, but one detail in the original body is now stale. Current src/server/tools.ts no longer clears local auth purely from raw 400/401 status. It clears when BrokerClientError.code === "unauthorized".

Core problem remains because broker still maps upstream 400/401 into generic unauthorized, maps upstream 429 to 502, drops useful upstream payload details, and client still rewrites non-JSON broker failures to synthetic 502.

Recommended adjustment:

• Keep this issue open.
• Update auth-clearing section from "raw status alone" to "generic broker unauthorized semantics are too broad because upstream error mapping is lossy".
• Regression coverage should still include 400, 401, 429, non-JSON upstream errors, and explicit invalid/revoked refresh token cases.