fix(auth): classify OAuth token errors as connection/login, not internal - #2788
fix(auth): classify OAuth token errors as connection/login, not internal#2788dhnihaoya wants to merge 2 commits into
Conversation
OAuth token-fetch failures (transport timeout, unauthorized refresh) reached the UI as `[internal]` on the default v2 engine and the v1 daemon path, because the token providers handed out by `AuthService.resolveTokenProvider` (agent-core-v2) and `ManagedAuthFacade.resolveOAuthTokenProvider` (agent-core) propagated raw `OAuthConnectionError`s. The classification only existed in node-sdk's facade. Wrap both facades so `getAccessToken` maps the oauth error classes to the public coded errors at the auth-domain boundary (v2 `_base/errors` forbids importing business domains, so the translation belongs here): - OAuthConnectionError / RetryableRefreshError -> provider.connection_error (retryable) - OAuthUnauthorizedError -> auth.login_required This restores the retryable flag and the dedicated pause reason, so a transient auth-host outage during a long task surfaces correctly instead of breaking the session. Refs: MoonshotAI#2786
🦋 Changeset detectedLatest commit: 87e2efb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a020fc4bec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Classify OAuth token failures into coded `Error2`s at the auth-domain | ||
| // boundary (`_base/errors` deliberately never imports a business domain, so | ||
| // the translation must happen here — see `_base/errors/serialize.ts`). |
There was a problem hiding this comment.
This new explanatory comment sits inside the implementation body, but the v2 package convention keeps comments solely in the top-of-file header and never beside methods/statements. The rationale here belongs in the commit/tests or, if it changes the module's external role, in the file header; leaving it inline makes the file violate the scoped comment rule.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
| /** | ||
| * Classify an OAuth token-fetch failure into the public {@link KimiError} | ||
| * protocol so callers (turn serialization, SDK clients) can react on `code` | ||
| * rather than on class identity. | ||
| * |
There was a problem hiding this comment.
This added block comment documents an internal helper in packages/agent-core/src/services, where the scoped guide explicitly forbids block/paragraph docstrings on internal helpers. Please remove it or reduce it to a single non-obvious invariant if one is truly needed, so the source does not accumulate rationale that belongs in the change history.
AGENTS.md reference: packages/agent-core/src/services/AGENTS.md:L170-L173
Useful? React with 👍 / 👎.
agent-core-v2 keeps comments in the top-of-file header only (never beside functions/statements), and agent-core/services forbids block docstrings on internal helpers and diff-narrating comments. Remove the inline rationale added with the OAuth error classification so the new code matches each package's convention.
1 participant
Related Issue
Resolve #2786
Problem
See linked issue (#2786). In short: on the default v2 engine (and the v1 daemon / legacy path), an OAuth token-fetch failure during a running task — e.g. a transient auth-host connect timeout — surfaced to the user as
[internal]instead of a retryableprovider.connection_error. That lost the retryable flag and routed the goal to the generic runtime pause reason instead of the dedicated connection pause, so a brief network blip broke long-running tasks.Root cause: the OAuth-error → public-error-code classification (
mapOAuthTokenError) existed only in@moonshot-ai/kimi-code-sdk'sAuthFacade. The token providers handed out by agent-core'sManagedAuthFacade.resolveOAuthTokenProviderand agent-core-v2'sOAuthService.resolveTokenProviderpropagated rawOAuthConnectionErrors, which serialize asinternal.What changed
Wrap the two unwrapped token-provider factories so
getAccessTokenclassifies OAuth failures into the public coded errors at the auth-domain boundary (v2_base/errorsforbids importing business domains, so the translation belongs at the boundary, not in the central serializer):packages/agent-core/src/services/auth/managedAuth.ts—resolveOAuthTokenProvidernow wraps the provider; addedmapOAuthTokenError(mirrors the SDK helper; kept local because agent-core cannot import the SDK).packages/agent-core-v2/src/app/auth/authService.ts—resolveTokenProvidernow wraps the provider; addedclassifyOAuthTokenError.Mapping (matches the existing SDK behavior):
OAuthConnectionError/RetryableRefreshError→provider.connection_error(retryable)OAuthUnauthorizedError→auth.login_requiredgetCachedAccessTokenis intentionally left unwrapped: it only reads local token storage (no HTTP / refresh), so it cannot produce these OAuth errors — wrapping it would be a no-op.node-sdk is untouched (its facade already classifies). The now-triplicated classification logic is tracked for consolidation in #2787.
Tests added to the existing test files (v1
oauth-service.test.ts, v2auth.test.ts): each path covers the connection mapping, the unauthorized mapping, and the "rethrow unrecognized raw" case. 81 relevant tests pass; both packagestsc --noEmitclean.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.