refactor(agent-core-v2): plug llm credentials in through request config - #3682
Conversation
…ai formats The requester already forwarded LlmRequestConfig.toolMessageConversion into formatRequest, but the openai chat and openai-responses formats only read the trait hook, so the explicit request config was silently ignored. Resolve the mode as request config, then trait default, then the protocol default, and pass the resolved value into lowering.
…ialects and provider connection
The ProtocolTrait interface bundled endpoint/headers connection config,
model capability, message/params conversion hooks, and error
classification into one bag, and every format received the whole bag
whether or not it consumed each hook — hooks a protocol ignores were
accepted by the type system and silently dead at runtime.
Split it by consumer:
- ProviderConnection (protocol/connection.ts): endpoint env
declaration and default headers, still resolved per request inside
generate.
- Per-protocol typed dialects (OpenAIDialect, OpenAIResponsesDialect,
AnthropicDialect, GoogleGenAIDialect): only the customization points
each protocol actually consumes; data-shaped hooks (reasoningKey,
toolCallIdPolicy, toolMessageConversion, strictThinkingValidation)
are data fields, and message/history hooks use the protocol wire
types instead of Record<string, unknown>.
- convertError moves to a requester option, capability to a provider
variant field.
Dialects are bound when the format is created (createOpenAIFormat and
siblings), so FormatRequestInput and the stream parser carry request
data only; ProtocolTrait is deleted. The thinking hook returns the
kwargs and the preserveThinking flag together as ThinkingApplication,
and the kimi dialect emits the final thinking params directly instead
of routing them through an extra_body flatten in buildParams. The
llm-adapter layer is migrated to the same {connection, dialect,
convertError} assembly.
…he requester pipeline
- move CONTEXT_MANAGEMENT_BETA into anthropic/contract and have the kimi trait import wire types from each protocol's contract.ts instead of format/lower; export the four contract modules from human/index.ts and guard the boundary in check-import-boundaries - rename protocol/trait.ts to protocol/thinking.ts and move TraitContext to protocol/base.ts; clean up the remaining dialect-era test variable - type extractUsage as OpenAIRawUsage / OpenAIResponsesRawUsage instead of Record<string, unknown> and drop the requester-side casts
… dead format types - the four requesters spread LlmRequestConfig into the plan input instead of hand-enumerating fields, so a new cross-protocol config field cannot be silently dropped per protocol - ProtocolFormat loses the phantom TRequest/TResponse type parameters; the dead OpenAIRawResponse/AnthropicRawResponse/GoogleRawChunk contract types go with them - the thinking test now names the extra_body flatten behavior explicitly
… requester pipeline - drop the four format barrels (and openai/reasoning-key) from human/index.ts; each base's public seam is now exactly contract / trait / requester - stop re-exporting contract wire types from format/lower modules so the neutral vocabulary has a single home; move the *LoweredMessage staging types out of contract into the owning format module - guard the seam in check-import-boundaries: only llm/requester/bases code and tests may import format/lower/patterns/reasoning-key
Replace the withAuth/withAuthUpload requester decorators with a credential contribution point on LlmRequestConfig: the llm machine's request actor (machine path) and ModelRequesterImpl (direct path) resolve credentials per attempt, and a recoverable 401 is recovered by invalidating and re-resolving — emitted as llm.recovering on the turn machine's credentials branch.
|
|
@codex review |
![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: 3effd9bae0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ? input.config | ||
| : { | ||
| ...input.config, | ||
| model: await resolveModelCredentials(input.config.model, input.config.credentials), |
There was a problem hiding this comment.
When credentials.resolve() rejects—for example because OAuth refresh, credential storage, or login resolution fails—the rejection occurs inside this detached void async function. A callback actor cannot propagate that promise rejection to the invocation's onError, so the turn remains in thinking indefinitely and the rejection may surface only as an unhandled promise rejection. Use an actor whose returned promise is supervised, or catch this rejection and emit/forward a terminal failure.
Useful? React with 👍 / 👎.
A rejection inside the llm machine's detached request actor (credential resolution, message resolvers, or a throwing requester) could not reach the machine, leaving the turn stuck in thinking. Convert it to an llm.failed.remote event so the turn lands in its failed state.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bb2f049d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const credential = await this.model.credentials.resolve(); | ||
| await requester.generate( | ||
| { ...config, model: applyCredential(resolved.model, credential) }, |
There was a problem hiding this comment.
When an OAuth token is revoked while still cached, direct consumers such as ModelCatalog.ping() receive the first 401 from this request and fail without invalidating or retrying the credential. The turn machine now supplies that recovery for agent turns, but ping() calls ModelRequesterImpl directly, and the previous withAuth wrapper performed the missing forced refresh; wrap this direct request path with the new single-recovery helper or implement equivalent recovery here.
Useful? React with 👍 / 👎.
| credentials.invalidate?.(); | ||
| return attempt(); |
There was a problem hiding this comment.
When an operation is canceled as its first attempt returns a recoverable 401, this helper still invalidates credentials and unconditionally starts a second attempt. This affects the media uploader at mediaToolsRegistrar.ts: ModelRequesterImpl.uploadVideo() resolves credentials before passing its signal to the uploader, so a canceled upload can force an OAuth refresh and potentially begin another upload; the removed credential wrapper explicitly suppressed retries when its signal was aborted.
Useful? React with 👍 / 👎.
| currentCredentials(): LlmCredentialProvider | undefined { | ||
| if (!this.profile.hasProvider()) return undefined; | ||
| return this.modelCatalog.get(this.profile.resolveModelContext().modelAlias).credentials; |
There was a problem hiding this comment.
If the profile model is changed while a turn is already running, prepareTurnConfig() keeps that turn on its previously snapshotted model, but this method returns credentials for the newly current profile model. A 401 from the old model therefore causes the turn recovery branch to invalidate the new model's provider, while the retried request resolves the old provider's unchanged cached token and fails again; credential lookup needs to use the same turn/model context as resolveRequest().
Useful? React with 👍 / 👎.
…, turn-snapshotted credentials
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7ffce0623
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| usage = undefined; | ||
| finish = undefined; | ||
| messageId = undefined; | ||
| const credential = await this.model.credentials.resolve(); |
There was a problem hiding this comment.
When an OAuth token is revoked while cached, global.models.generate() still reaches ModelRequesterImpl.request() directly through packages/klient/src/transports/memory/dispatcher.ts:475-498; this one-shot credential resolution sends the stale token, and the resulting 401 is returned without invalidating or retrying, so subsequent facade requests keep failing until another flow refreshes the token. Fresh evidence beyond the prior comment is that the current dispatcher route remains unwrapped while recovery was added only to specific callers such as catalog ping; apply the single credential-recovery loop to this direct route or the requester boundary.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb61141cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : credential instanceof Promise | ||
| ? { | ||
| ...input.config, | ||
| model: applyCredential(input.config.model, await credential), |
There was a problem hiding this comment.
When the first request for a queued turn uses OAuth and credential resolution rejects (for example, login is required or token refresh fails), this await is caught and converted to llm.failed.remote before requester.generate() runs. In the agent-core-v2 loop, createMachineRequester.generate() is where gate() binds pendingMachineTurn; without that call, loopService.ts:764-768 merely clears the pending marker on turnSettled and never removes or settles the queued reservation, leaving the submitter's ready/result promises and settled() hanging. Fresh evidence beyond the earlier comment is that the new catch now terminates the machine, but this ordering still bypasses the binding gate; bind the pending turn before credential resolution or explicitly settle its reservation on this failure path.
Useful? React with 👍 / 👎.
Remove the attemptWithCredentialRecovery/streamWithCredentialRecovery helpers from human/credentials; the single-retry recovery is now written out at each direct call site (catalog ping/generate, full compaction, media upload) so the control flow reads linearly without a wrapper hop. The credentials module keeps only the provider factories and the credential application helpers; recoverability and invalidation stay on the LlmCredentialProvider itself.
…s before gating When the llm machine fails before requester.generate() runs — for example when OAuth credential resolution rejects — the loop cleared the pending machine turn marker on turnSettled without ever binding or settling the queued reservation, leaving the submitter's ready/result promises and settled() hanging. The loop now settles the unbound reservation explicitly: cancelled on abort, failed otherwise, mirroring the evaluateSettle error propagation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc59aecffb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const error = outcome.error ?? new Error2(ErrorCodes.INTERNAL, 'Turn ended before first step'); | ||
| reservation.controller.abort(error); | ||
| reservation.turn.state = 'failed'; | ||
| reservation.ready.reject(error); | ||
| reservation.result.resolve({ type: 'failed', steps: 0, error }); |
There was a problem hiding this comment.
When credential resolution rejects before the gate for a normal user prompt, this direct settlement resolves turn.result without passing through endTurn(), which is the path that dispatches TurnEnded and AgentErrorEvent. AgentPromptService therefore emits only prompt.completed with a generic failed reason and no error payload, so clients never receive the OAuth/login error needed to explain or recover from the accepted prompt's failure. Fresh evidence beyond the earlier hanging report is this newly added settlement path; it should publish the equivalent failure lifecycle events or bind the turn before failing it.
Useful? React with 👍 / 👎.
… turn lifecycle The direct settlement resolved turn.result without endTurn(), so no TurnPrompt/TurnStarted/TurnEnded/AgentErrorEvent was published and the next queued reservation was never launched. The unbound reservation is now bound via beginActiveTurn and ended via endTurn (failed, or cancelled on abort), chained onto afterChain when the previous turn is still settling, so pre-gate failures get the same lifecycle events and queue progression as any other failed turn.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 426e0ba757
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pending: { readonly id: number; readonly queueItemId?: string }, | ||
| outcome: { readonly outcome: MachineTurnOutcome; readonly error?: unknown }, | ||
| ): void { | ||
| if (pending.queueItemId === undefined) return; |
There was a problem hiding this comment.
When an idle notify() carrying a context message starts a turn and credential resolution fails before gate(), the pending machine turn has no queueItemId, so this return clears pendingMachineTurn without consuming or dropping the nudge. The nudge remains marked sentToMachine and hasPendingRequests() stays true, while the machine is already idle and will not resend it; consequently settled() hangs until another prompt arrives or the caller explicitly drops the notification. Handle notification-backed pending turns here as well as queued reservations.
Useful? React with 👍 / 👎.
| }, | ||
| ); | ||
| } catch (error) { | ||
| sendBack({ type: 'llm.failed.remote', error: toLlmErrorMessage(error) }); |
There was a problem hiding this comment.
When credentials.resolve() rejects with the Error2 produced by ModelOAuthTokenAdapter for a missing or revoked login, this conversion discards its auth.login_required code and details before the pre-gate settlement path receives it. endTurn() then serializes the plain LLM-message object as an internal error (even reducing its message to [object Object]), so the TUI check in apps/kimi-code/src/tui/controllers/session-event-handler.ts:988-989 cannot show the login-required recovery notice. Fresh evidence beyond the earlier lifecycle comment is that the turn now emits failure events, but those events still contain the lossy converted error; retain the original error alongside the LLM event or restore its coded payload when settling.
Useful? React with 👍 / 👎.
| target: 'thinking', | ||
| reenter: true, |
There was a problem hiding this comment.
When an OAuth-backed request emits llm.sent and then returns a recoverable 401, this re-entry emits recovering, but AgentLoopService.projectMachineEvent() handles only retrying by closing and clearing the current MachineStepState. The next attempt therefore passes through gate() with the old step still active, overwrites it with a newly numbered step, and leaves the original step.begin without a matching step.end; the recovered request is also counted as an extra turn step and can be rejected by a low maxStepsPerTurn. Close/reset the loop step for credential recovery just as the retry path does before re-entering thinking.
Useful? React with 👍 / 👎.
… coded credential errors, close steps on recovering - Notification-seeded machine turns that fail before gating now bind a seeded reservation and end through endTurn like queued ones, instead of leaving the nudge pending and settled() hanging - The llm machine's request actor attaches the raw error to llm.failed.remote and the turn machine prefers it for its terminal failure, so coded errors (e.g. auth.login_required) survive to endTurn's error payload - projectMachineEvent now closes the current machine step on recovering exactly as it does on retrying, keeping step.begin/step.end balanced when credential recovery re-enters the request
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ional Model.credentials, abort-safe request actor
- oauthCredentials.invalidate() now starts the forced refresh eagerly and
hands the pending token to the next resolve(), so the force signal can no
longer leak into or be consumed by unrelated resolves
- Model.credentials becomes optional, dropping the {} as never /
staticCredentials(undefined) filler in test fakes
- the llm machine request actor swallows abort outcomes instead of
reporting llm.failed.remote, so an aborted turn settles as aborted
rather than failed
…credential-recovery executors, single-proposal failure triage
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d78c485838
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const seeded = this.nudges.slice(this.nudgeCursor).find( | ||
| (nudge) => !nudge.dropped && nudge.contextMessage !== undefined && nudge.contextMessage.content.length > 0, | ||
| ); | ||
| if (seeded === undefined) return; |
There was a problem hiding this comment.
When an idle turn is started by loop.notify() without a message and credential resolution fails before gate(), this return leaves the corresponding nudge unconsumed after pendingMachineTurn is cleared. Real callers use this form in features/externalHooks/agent/agentExternalHooksService.ts:258 and features/goal/goalService.ts:607; because hasPendingRequests() continues to see the nudge while the machine has already drained its notification, settled() hangs until unrelated work arrives. Fresh evidence beyond the prior context-notification report is that the current branch explicitly excludes empty nudges even though these message-less call sites exist.
Useful? React with 👍 / 👎.
| const model = await resolveModelCredentials(resolved.model, this.model.credentials); | ||
| return uploader(video, { model, signal: options?.signal }); |
There was a problem hiding this comment.
When an OAuth token is rejected while still cached, uploadVideo() now performs only one credential resolution and one upload attempt. The wrapper added in mediaToolsRegistrar.ts covers the read-media tool, but agent/media/mediaResolverService.ts:258-280 creates this requester-backed uploader directly while preprocessing user-supplied videos and explicitly rethrows authentication failures; that path therefore neither invalidates nor retries the stale credential, so the user's video request fails until another operation refreshes the token. Apply the shared single-recovery executor at this remaining caller or retain recovery at the upload boundary.
Useful? React with 👍 / 👎.
…lure, recover credentials in media resolver uploads
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7086d06b2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| signal: input.signal, | ||
| }); | ||
| try { | ||
| const credential = input.config.credentials?.resolve(); |
There was a problem hiding this comment.
When oauthCredentials wraps a token source that returns the refreshed token only for { force: true }, invalidate() reserves that token for the next resolve(), but this call consumes it into config before createMachineRequester.generate() discards _config; ModelRequesterImpl.runRequest() then resolves the provider again normally. The credential-recovery attempt can therefore send the stale token and repeat the 401 despite a successful refresh. Pass the resolved configuration through the machine requester or otherwise ensure credentials are resolved exactly once per attempt.
AGENTS.md reference: packages/agent-core-v2/src/human/llm/AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
Keep the per-protocol typed trait structure and port main's changes onto it: - llm/protocol/trait.ts (deleted here, widened on main with acceptedImageMimes): the hook now lives on AnthropicTrait, resolved in planAnthropicRequest and threaded through lowerAnthropicRequest; kimiAnthropicTrait supplies the Kimi set via providerImagePolicy - openai/format.ts: fold the reasoning_details round-trip into the requester-composed stream parser; the explicit reasoning key reaches the parser through OpenAIStreamParserOptions - protocolAdapterRegistry.resolveCapability: drop the removed explain/trace stack, keep the definition-level capability hook - adjust thinking, anthropic-lower, and sessionMediaStore tests to the requester pipeline shapes
Resolve the overlap with the protocol trait refactor (#3641) and the event-sourced agent store (#3691): - engine.ts: keep both additions at the machine wiring site — the turn-aware credential provider feeding input.request.credentials, and the journal-backed AgentEventStore now required by AgentInput - docs/{en,zh}/llm.md: unify the request lifecycle paragraph — the credential resolution flow alongside the requester plan* composition, and the turn-side emptyResponseError / recovery-chain wording that matches the merged code
commit: |
Resolve the overlap with the llm-machine removal (#3710): - the machine's createRequestActor is gone; the credential resolution and the abort-guarded error boundary move into llm/requester/actor.ts, kept synchronous on the credential-less path so the turn startup cascade keeps its ordering - docs/{en,zh}/llm.md: unify principles and the request lifecycle with the turn-driven orchestration — credential resolution in the request actor, the recovery strategy chain and empty-response judgment in the turn - turn.test.ts: drive the request actor through a harness machine instead of the deleted llm machine
2 participants
Related Issue
N/A — internal refactor, no linked issue.
Problem
OAuth handling in the
humanlayer lived inwithAuth/withAuthUploadrequester decorators wrappinggenerate: they resolved credentials per call, intercepted thellm.failed.remoteevent stream, swallowed the first 401, and silently retried once. The same policy existed in two parallel copies (event-based for generate, throw-based for upload), the upload copy'scanRecovercould never fire on raw SDK errors (so the upload 401 retry was dead code), and the silent retry was invisible to the event contract — nollm.retrying/llm.recoveringever fires for an auth refresh. A second auth home (AuthProvider/StaticAuthProvider) also sat in llm-adapter, so credential logic was split across layers.What changed
Auth becomes part of the request:
LlmRequestConfig.credentialsis the credential contribution point (resolve/canRecover/invalidate), and the attempt loops — not a decorator — own resolution and recovery.human/credentials/is the single home for credential providers:staticCredentials(apiKey)andoauthCredentials(getToken)(force-refresh oninvalidate, 401 detection via the sharederrorStatusCodeinllm/errors.ts), plus theapplyCredential/resolveModelCredentialshelpers.kimi-oauthdropswithAuth/withAuthUpload/CredentialSource; it is now a one-line adapter overoauthCredentials.AuthProvider/StaticAuthProvider/ProviderRequestAuthare gone:Model.authProviderbecomesModel.credentials, built by the catalog through the factories above.ModelRequesterImplis pure transport — it resolves credentials per attempt and never retries.config.credentialsinto a fully-credentialed model before each attempt (zero ticks when resolution is synchronous, so wire event ordering is unchanged), and the turn state machine owns 401 recovery: a recoverable 401 emitsllm.recovering {strategy:'credentials', action:'refresh'}, callscredentials.invalidate(), and re-entersthinking(once per step, via the existingappliedRecoveriesreset). The state machine can now drive OAuth natively.ping/generate(the newIModelCatalog.generatebacking the klient facade), full compaction, media upload — write the same single-retry recovery out inline at the call site, sharing the state machine's provider instance and honoring abort. No wrapper layer: the control flow reads linearly where it happens.beginActiveTurn+endTurn, with failure events and queue progression) instead of hanging the submitter'sready/result/settled(). The raw error ridesllm.failed.remoteasrawError, so coded errors likeauth.login_requiredreach the UI intact, andrecoveringnow closes the current machine step exactly likeretrying(balancedstep.begin/step.end).Verified: agent-core-v2 suite 6426 tests green (new loop-level regressions for pre-gate settle of queued and notification-seeded turns, coded-error identity, and recovering step balance — each shown to fail without its fix), repo typecheck green, lint clean.
Architecture diagram (dot source)
Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.