fix: wrap ExternalPrincipalExchange application_id-not-found as *OAuthError

[rsharath]

Summary

ExternalPrincipalExchange (the external-principal path of the token_exchange grant, when actor_token is empty) had two bugs at internal/service/oauth.go:621-630:

1. Wire-shape bug (the one originally fixed): a plain fmt.Errorf("invalid_request: ...") falls through extractOAuthError's errors.As(*OAuthError) check and emits 500 server_error instead of the intended 400 invalid_request.
2. Error-classification bug (caught during PR-169's own review by Copilot): all GetIdentity failures were lumped into the same 400 message, including transient infrastructure errors (DB unreachable, query timeout, etc.) that should be 500-retriable.

Final shape (after Copilot review fixes — commit 483816e)

resolved, err := s.identitySvc.GetIdentity(ctx, req.ApplicationID, req.AccountID, req.ProjectID)
if err != nil {
    // Distinguish "row not found / wrong tenant" (client error, 400
    // invalid_request) from transient infrastructure failures (server
    // error, 500 — retriable by the client). The bun repository wraps
    // sql.ErrNoRows for "no row matches id + account_id + project_id" —
    // that covers both genuine-missing and IDOR-protected cross-tenant
    // lookups. Anything else is an infra problem the client can't act on.
    if errors.Is(err, sql.ErrNoRows) {
        return nil, oauthBadRequestCause(
            oautherror.InvalidRequest,
            fmt.Sprintf("application_id %s not found or access denied", req.ApplicationID),
            err,
        )
    }
    return nil, oauthServerError(
        fmt.Sprintf("failed to look up application_id %s", req.ApplicationID),
        err,
    )
}

Net wire-behavior change vs the original buggy code:

• HTTP status on "not found": 500 → 400 (the intended status)
• HTTP status on transient DB error: 500 → 500 (unchanged, but now with a meaningful description AND retriable signal)
• Error code on body for "not found": {"error":"server_error"} → {"error":"invalid_request"}
• Error code on body for transient DB error: {"error":"server_error"} → {"error":"server_error"} (unchanged but now via the right helper)
• Internal logs: gain access to the underlying GetIdentity error via errors.Unwrap (previously discarded)
• Uses oautherror.InvalidRequest (PR-167's RFC-anchored constant), not a bare string

Commit history

Commit	Purpose
3e417f5	Original fix — wrap with oauthBadRequestCause. Used bare "invalid_request" string because PR-167 wasn't merged into main yet.
571368f	Merge from main, bringing in PR-167's oautherror package.
483816e	Address Copilot findings: swap bare string to oautherror.InvalidRequest, distinguish sql.ErrNoRows from transient errors.

Origin and scope

The original wire-shape bug was from commit d5bedb44 (March 2026 — Yash Datta). Caught by Copilot during PR-167's review; split out of that PR to keep its scope clean (constants sweep, not error-wrapping fixes).

A grep for fmt\.Errorf\("invalid_ across internal/ returned this as the only site with this anti-pattern. Confirmed the same shape doesn't appear via errors.New either.

Test plan

• go build ./... clean
• go vet ./... clean
• Full integration suite passes (go test ./tests/integration/... — ~36s, no regressions)
• No new test added — ExternalPrincipalExchange has no integration coverage today; the path requires a TrustedServiceValidator that isn't wired in any test setup. Adding the full harness for a 1-line bug fix is disproportionate; whoever next touches this code path should add coverage.

What this PR is NOT

• Not a refactor — single function call site + comment changes
• Not a wire-API change beyond fixing the bug

[gemini-code-assist]

Code Review

This pull request updates the error handling in ExternalPrincipalExchange when retrieving an identity fails. It replaces a plain fmt.Errorf with oauthBadRequestCause to ensure the token endpoint correctly returns an invalid_request (400) status instead of a server_error (500), while preserving the underlying error for logging. There are no review comments, so I have no additional feedback to provide.

[Copilot]

Pull request overview

Fixes OAuth token exchange behavior for the ExternalPrincipalExchange path by ensuring an application_id lookup failure returns a structured *service.OAuthError, so handlers emit the intended RFC-compliant invalid_request (HTTP 400) instead of defaulting to server_error (HTTP 500).

Changes:

• Replace the fmt.Errorf("invalid_request: ...") return with oauthBadRequestCause(...) so extractOAuthError can errors.As the error properly.
• Preserve the underlying GetIdentity error in the wrapped cause for logging/diagnostics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.