fix(mcp): persist SDK-resolved client identity so cold-start refresh works - #1970
Merged
Aaronontheweb merged 3 commits intoAug 17, 2026
Merged
Conversation
…works The persisted McpOAuthTokens record could end up with a null ClientId even when the SDK had resolved one internally (own dynamic registration or a client-metadata document), because CreateReplacement only ever wrote identity.ClientId — never tokens.ClientId. On restart CreateTokenCache then rebuilt an empty identity (DCR=false records were skipped entirely), so SDK 2.0's refresh gate (CachedTokensMatchClientCredentials) never matched and every token expiry fell through to interactive auth: the recurring 'AuthorizationCallbackHandler returned a null authorization result' loop that blocked the Immovlan nightly Jira PR review for a week. Two-part fix: - CreateReplacement falls back to the SDK-reported TokenContainer client id/secret when Netclaw's own identity is absent, and marks the record dynamic when the SDK supplied the id. - CreateTokenCache rebuilds the provider identity from any persisted ClientId, not only records flagged DynamicClientRegistration. Repro: McpOAuthTokens record for atlassian showed clientId=null, dynamicClientRegistration=false, authorizationServer=...mcp.atlassian.com, tokenEndpointAuthMethod=client_secret_post, refresh token present, expiry matching the observed 15:03 auth loss exactly.
Aaronontheweb
added a commit
to Aaronontheweb/netclaw
that referenced
this pull request
Aug 17, 2026
PR netclaw-dev#1970 changes only in-memory identity fallback logic and adds no logging, so it does not introduce a leak on its own. But an audit for the same issue found McpOAuthCredentialStore has the same shape of gap: it also logs a raw exception in its credentials-file load path. The secrets file is decrypted before that callback runs, so a malformed-JSON or decryption exception could in principle echo a fragment of plaintext credential content into the log. Move RedactForLogging out of McpClientManager and into SecretOutputRedactor, so both McpClientManager and McpOAuthCredentialStore route their exception logging through the same shared helper.
Aaronontheweb
approved these changes
Aug 17, 2026
| // own dynamic registration persist the SDK-resolved client id without the DCR | ||
| // marker; discarding it on restart made SDK 2.0 skip the refresh path | ||
| // entirely ("null authorization result" on every later expiry). | ||
| identity = new McpOAuthClientIdentity( |
Collaborator
There was a problem hiding this comment.
LGTM and nice catch lol
Aaronontheweb
enabled auto-merge (squash)
August 17, 2026 21:55
Aaronontheweb
added
mcp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The persisted
McpOAuthTokensrecord could end up with a null ClientId even when the SDK had resolved one internally (own dynamic registration or a client-metadata document), becauseCreateReplacementonly ever wroteidentity.ClientId— nevertokens.ClientId. On restartCreateTokenCachethen rebuilt an empty identity (DCR=false records were skipped entirely), so SDK 2.0's refresh gate (CachedTokensMatchClientCredentials) never matched and every token expiry fell through to interactive auth.This produced the recurring
AuthorizationCallbackHandler returned a null authorization resultloop that blocked the Immovlan nightly Jira PR review for a week (2026-08-08 → 08-15). Live diagnosis via read-only probe (private tool, not in this PR):Change
CreateReplacementfalls back to the SDK-reportedTokenContainerclient id/secret when Netclaw's own identity is absent, and marks the record dynamic when the SDK supplied the id.CreateTokenCacherebuilds the provider identity from any persisted ClientId, not only records flaggedDynamicClientRegistration.Companion diagnostics PR: #1969 (log OAuth refresh failure diagnostics on auth-loss).
Verification
Diagnostic probe confirmed the missing identity; the fix aligns write and read paths so the persisted record satisfies the SDK 2.0 refresh gate across daemon restarts. (Full daemon build of this snapshot is blocked by unrelated pre-existing ShellSyntaxTree restore errors in Netclaw.Security on this workspace; the change is isolated to McpOAuthCredentialStore.)