Refresh oauth tokens - #141
Conversation
|
OSSF Scorecard (PR vs base)
|
|
Warning Review limit reached
More reviews will be available in 50 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR adds MCP OAuth resource persistence, structured refresh failure reporting, manual access-token refresh UI, extra reauth-flow logging, and supporting docs, config, and version updates. ChangesMCP auth flow
Supporting docs and metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md (5)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant
?? undefinedat end of fallback chain.The
oauthCodeextraction ends with?? undefinedwhich is redundant since the preceding expression already evaluates toundefinedwhen the regex match fails. Remove the trailing?? undefinedfor clarity.- undefined; + ;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines 57 - 62, The oauthCode fallback chain in the catch block is needlessly ending with a redundant undefined fallback. Update the oauthCode extraction logic to stop after the regex match result in the existing catch clause, removing the trailing ?? undefined while keeping the same behavior when no code is found.
82-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the import instruction: remove reference to
deleteCachedValue.The instruction mentions importing
deleteCachedValuebut then immediately says to usedeleteMcpClientIdinstead. SincedeleteMcpClientIdencapsulatesdeleteCachedValue, remove the confusingdeleteCachedValuereference to avoid implementation ambiguity.- Import `getMcpAuthMode` and `deleteCachedValue` (or use the already-imported `getMcpClientId`/`saveMcpClientId` pattern — use `deleteMcpClientId` described below). + Import `getMcpAuthMode` and `deleteMcpClientId` from `./secure-storage`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines 82 - 83, The import guidance is ambiguous because it mentions deleteCachedValue even though the intended helper is deleteMcpClientId; update the instruction to reference getMcpAuthMode and deleteMcpClientId only, matching the existing getMcpClientId/saveMcpClientId pattern and avoiding any direct use of deleteCachedValue.
360-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate self-review to reflect current logging state and add test-design verification.
Remove "logging gap (has_client_id)" since current code already logs this field. Add a checklist item verifying the test mock strategy correctly intercepts module imports.
- - Spec coverage: logging gap (has_client_id), error code extraction, DCR stale-registration recovery, static-mode guard. + - Spec coverage: error code extraction, DCR stale-registration recovery, static-mode guard. (`has_client_id` logging already implemented.) + - Test design: mocks correctly intercept `secure-storage` imports used by `refreshMcpAccessToken`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines 360 - 364, The self-review text is outdated: remove the “logging gap (has_client_id)” note because the current logging already includes that field, and update the checklist in the planning doc to add a test-design verification item. Use the existing self-review section and the test coverage bullets around `has_client_id`, `deleteMcpClientId`, and the DCR/static mode checks to keep the wording aligned with the current implementation, and add a check confirming the test mock strategy intercepts module imports correctly.
155-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish already-implemented logging from new invalid_client recovery work.
Task 2 Steps 1-2 describe
has_client_idlogging andoauthCodeextraction that already exist inlib/mcp-oauth.ts(per Context snippet 1). Only Step 3 (clearing stale DCRclientId) is new. Update the task description to clarify which steps are already complete versus pending implementation, or restructure as "Verify existing logging" + "Add invalid_client recovery".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines 155 - 244, The task description in the `refreshMcpAccessToken` section mixes already-implemented logging changes with the new `invalid_client` recovery work, which is misleading. Update the plan text so `getMcpAuthMode`, `has_client_id` logging, and `oauthCode` extraction are clearly marked as already done, and keep only the DCR recovery behavior using `deleteMcpClientId` as the pending change. Reference `refreshMcpAccessToken` and the `lib/mcp-oauth.ts` import/logging/catch-block steps when rewriting the task to distinguish verify-vs-implement work.
112-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRephrase static-mode safety explanation for precision.
The current phrasing "clearing the DCR
clientIdis a no-op for static mode" is misleading because the clear is explicitly gated bymode === "dcr"and never attempted for static. Rephrase to clarify the guard prevents the operation rather than the operation being effectless.- This is safe for static mode because `oauthCode === "invalid_client"` in static mode means the user's configured credentials are wrong — clearing the DCR `clientId` is a no-op for static mode since `getMcpAuthMode` returns `"static"` and the branch is skipped. + This is safe for static mode because `getMcpAuthMode` returns `"static"`, so the `mode === "dcr"` guard skips the deletion. The `invalid_client` error for static connectors indicates wrong user-configured credentials and requires manual re-auth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines 112 - 113, Rephrase the static-mode safety note to make it clear the DCR clientId clear is never executed in static mode because the branch is guarded by mode === "dcr", not because the clear would have no effect. Update the wording in the explanatory text around getMcpAuthMode and the invalid_client handling to explicitly state that static mode skips the operation entirely.docs/superpowers/plans/2026-06-25-refresh-oauth-token.md (1)
67-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd logging for
getMcpExtensions()failures and preload metrics.The proposed
.catch(() => [] as McpExtensionRecord[])silently swallows errors and provides no observability into how many extensions were found or how many keys will be preloaded. Consider adding structured logging before and after the extension enumeration, matching the existing emoji-prefixed logging style ininitializeSecureStorageCache().For example, log:
- Number of extensions retrieved
- Number of MCP keys added to preload list
- Any
getMcpExtensions()failure (at warn level, not silently caught)This addresses the observability gap if the preload silently fails and users still hit re-auth prompts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-25-refresh-oauth-token.md` around lines 67 - 69, The `getMcpExtensions()` preload path is silently swallowing failures and lacks visibility into how many extensions and MCP keys are being processed. Update the enumeration around `getMcpExtensions()` to log a warning when it fails instead of using a silent catch, and add emoji-prefixed structured logs before and after the preload work to report the number of extensions retrieved and the number of MCP keys added, matching the logging style used in `initializeSecureStorageCache()`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md`:
- Around line 9-11: The RCA validation is outdated because
`refreshMcpAccessToken` in `lib/mcp-oauth.ts` already logs `has_client_id` and
`client_id_length` alongside `has_client_secret`. Update the plan to reflect the
current behavior of `refreshMcpAccessToken` and its “Refreshing access token”
logging block, or explicitly mark the note as historical instead of claiming the
client ID field is missing.
- Around line 349-355: The QA checklist mixes static OAuth and DCR behaviors in
the same item, so split the Brain3 MCP connector tests into separate steps by
auth mode. Keep the static OAuth refresh failure checks (invalid_client,
has_client_id) together under the refresh path, and move the DCR-specific
assertions (getMcpClientId returns null, stale entry cleared, re-auth triggers
fresh registration) into a separate DCR connector QA item. Use the existing
checklist entries around the Brain3 MCP connector and token refresh logs as the
anchors when editing.
- Around line 256-289: The `refreshMcpAccessToken` test is patching
`secure-storage` exports after import, but those functions are already closed
over in `oauthModule` so the mocks won’t be used. Update the test to use
`jest.mock` for `../secure-storage` and `expo-auth-session`, or re-import
`oauthModule` after applying the mocks so `getMcpAuthMode`, `deleteMcpClientId`,
and `refreshAsync` are actually intercepted. Also remove the unused `_orig`
import variable from the `refreshAsync` setup.
- Around line 291-322: The static-mode invalid_client test still patches
imported modules directly, so update it to use the same jest.mock/jest.spyOn
pattern as the DCR test and add cleanup with afterEach or equivalent restore
logic. In the refreshMcpAccessToken test, verify deleteMcpClientId was not
called via a mock assertion instead of checking a null sentinel, while keeping
the existing invalid_client setup and the getMcpAuthMode static override.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md`:
- Around line 57-62: The oauthCode fallback chain in the catch block is
needlessly ending with a redundant undefined fallback. Update the oauthCode
extraction logic to stop after the regex match result in the existing catch
clause, removing the trailing ?? undefined while keeping the same behavior when
no code is found.
- Around line 82-83: The import guidance is ambiguous because it mentions
deleteCachedValue even though the intended helper is deleteMcpClientId; update
the instruction to reference getMcpAuthMode and deleteMcpClientId only, matching
the existing getMcpClientId/saveMcpClientId pattern and avoiding any direct use
of deleteCachedValue.
- Around line 360-364: The self-review text is outdated: remove the “logging gap
(has_client_id)” note because the current logging already includes that field,
and update the checklist in the planning doc to add a test-design verification
item. Use the existing self-review section and the test coverage bullets around
`has_client_id`, `deleteMcpClientId`, and the DCR/static mode checks to keep the
wording aligned with the current implementation, and add a check confirming the
test mock strategy intercepts module imports correctly.
- Around line 155-244: The task description in the `refreshMcpAccessToken`
section mixes already-implemented logging changes with the new `invalid_client`
recovery work, which is misleading. Update the plan text so `getMcpAuthMode`,
`has_client_id` logging, and `oauthCode` extraction are clearly marked as
already done, and keep only the DCR recovery behavior using `deleteMcpClientId`
as the pending change. Reference `refreshMcpAccessToken` and the
`lib/mcp-oauth.ts` import/logging/catch-block steps when rewriting the task to
distinguish verify-vs-implement work.
- Around line 112-113: Rephrase the static-mode safety note to make it clear the
DCR clientId clear is never executed in static mode because the branch is
guarded by mode === "dcr", not because the clear would have no effect. Update
the wording in the explanatory text around getMcpAuthMode and the invalid_client
handling to explicitly state that static mode skips the operation entirely.
In `@docs/superpowers/plans/2026-06-25-refresh-oauth-token.md`:
- Around line 67-69: The `getMcpExtensions()` preload path is silently
swallowing failures and lacks visibility into how many extensions and MCP keys
are being processed. Update the enumeration around `getMcpExtensions()` to log a
warning when it fails instead of using a silent catch, and add emoji-prefixed
structured logs before and after the preload work to report the number of
extensions retrieved and the number of MCP keys added, matching the logging
style used in `initializeSecureStorageCache()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24f73390-0490-4a9a-92e2-8afadcdb74fc
📒 Files selected for processing (7)
app/index.tsxcomponents/settings/McpConnectorConfig.tsxcomponents/settings/McpExtensionsScreen.tsxdocs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.mddocs/superpowers/plans/2026-06-25-refresh-oauth-token.mdlib/mcp-oauth.tsmodules/vm-webrtc/src/ToolkitManager.ts
| test("clears DCR client_id when refresh fails with invalid_client", async () => { | ||
| secureState.clientId = "dcr-client-id"; | ||
| secureState.clientSecret = null; | ||
| secureState.refreshToken = "stored-refresh-token"; | ||
| secureState.tokenEndpoint = "https://provider.example.com/token"; | ||
|
|
||
| // Patch refreshAsync to throw with invalid_client | ||
| const { refreshAsync: _orig } = await import("expo-auth-session"); | ||
| const authSessionMod = await import("expo-auth-session"); | ||
| const origRefresh = authSessionMod.refreshAsync; | ||
| (authSessionMod as any).refreshAsync = async () => { | ||
| const err: any = new Error("invalid_client"); | ||
| err.error = "invalid_client"; | ||
| throw err; | ||
| }; | ||
|
|
||
| // Patch getMcpAuthMode to return "dcr" | ||
| const storageMod = await import("../secure-storage"); | ||
| const origAuthMode = storageMod.getMcpAuthMode; | ||
| (storageMod as any).getMcpAuthMode = async () => "dcr"; | ||
| let deletedId: string | null = null; | ||
| const origDeleteClientId = storageMod.deleteMcpClientId; | ||
| (storageMod as any).deleteMcpClientId = async (id: string) => { deletedId = id; }; | ||
|
|
||
| const token = await oauthModule.refreshMcpAccessToken("extension-dcr", "DCR Connector"); | ||
|
|
||
| expect(token).toBeNull(); | ||
| expect(deletedId).toBe("extension-dcr"); | ||
|
|
||
| // Restore | ||
| (authSessionMod as any).refreshAsync = origRefresh; | ||
| (storageMod as any).getMcpAuthMode = origAuthMode; | ||
| (storageMod as any).deleteMcpClientId = origDeleteClientId; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Test design flaw: module patches won't affect closed-over imports in refreshMcpAccessToken.
The test patches storageMod.getMcpAuthMode and storageMod.deleteMcpClientId after await import("../secure-storage"), but refreshMcpAccessToken imported these functions at module evaluation time. The patches won't be visible to the function under test. Restructure using jest.mock or re-import oauthModule after patching, or refactor to dependency injection.
- // Patch getMcpAuthMode to return "dcr"
- const storageMod = await import("../secure-storage");
- const origAuthMode = storageMod.getMcpAuthMode;
- (storageMod as any).getMcpAuthMode = async () => "dcr";
- let deletedId: string | null = null;
- const origDeleteClientId = storageMod.deleteMcpClientId;
- (storageMod as any).deleteMcpClientId = async (id: string) => { deletedId = id; };
-
- const token = await oauthModule.refreshMcpAccessToken("extension-dcr", "DCR Connector");
+ jest.mock("../secure-storage", () => ({
+ ...jest.requireActual("../secure-storage"),
+ getMcpAuthMode: async () => "dcr",
+ deleteMcpClientId: jest.fn(async (id: string) => { deletedId = id; }),
+ }));
+ const { refreshMcpAccessToken } = await import("../mcp-oauth");
+ const token = await refreshMcpAccessToken("extension-dcr", "DCR Connector");Also remove unused _orig at line 263.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines
256 - 289, The `refreshMcpAccessToken` test is patching `secure-storage` exports
after import, but those functions are already closed over in `oauthModule` so
the mocks won’t be used. Update the test to use `jest.mock` for
`../secure-storage` and `expo-auth-session`, or re-import `oauthModule` after
applying the mocks so `getMcpAuthMode`, `deleteMcpClientId`, and `refreshAsync`
are actually intercepted. Also remove the unused `_orig` import variable from
the `refreshAsync` setup.
| test("does not clear client_id for static mode on invalid_client", async () => { | ||
| secureState.clientId = "static-client-id"; | ||
| secureState.clientSecret = "stored-static-secret"; | ||
| secureState.refreshToken = "stored-refresh-token"; | ||
| secureState.tokenEndpoint = "https://provider.example.com/token"; | ||
|
|
||
| const authSessionMod = await import("expo-auth-session"); | ||
| const origRefresh = authSessionMod.refreshAsync; | ||
| (authSessionMod as any).refreshAsync = async () => { | ||
| const err: any = new Error("invalid_client"); | ||
| err.error = "invalid_client"; | ||
| throw err; | ||
| }; | ||
|
|
||
| const storageMod = await import("../secure-storage"); | ||
| const origAuthMode = storageMod.getMcpAuthMode; | ||
| (storageMod as any).getMcpAuthMode = async () => "static"; | ||
| let deletedId: string | null = null; | ||
| const origDeleteClientId = storageMod.deleteMcpClientId; | ||
| (storageMod as any).deleteMcpClientId = async (id: string) => { deletedId = id; }; | ||
|
|
||
| const token = await oauthModule.refreshMcpAccessToken("extension-static", "Static Connector"); | ||
|
|
||
| expect(token).toBeNull(); | ||
| expect(deletedId).toBeNull(); // must NOT clear for static mode | ||
|
|
||
| // Restore | ||
| (authSessionMod as any).refreshAsync = origRefresh; | ||
| (storageMod as any).getMcpAuthMode = origAuthMode; | ||
| (storageMod as any).deleteMcpClientId = origDeleteClientId; | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Same module-patching flaw in static-mode test; add mock cleanup and stronger assertions.
Apply the same jest.mock restructuring as the DCR test. Additionally:
- Use
jest.spyOnor mock function to verifydeleteMcpClientIdwas not called, rather than relying on null initialization. - Add
afterEach(() => jest.restoreAllMocks())or equivalent to prevent mock leakage.
- let deletedId: string | null = null;
- const origDeleteClientId = storageMod.deleteMcpClientId;
- (storageMod as any).deleteMcpClientId = async (id: string) => { deletedId = id; };
+ const deleteMcpClientId = jest.fn();
+ jest.mock("../secure-storage", () => ({
+ ...jest.requireActual("../secure-storage"),
+ getMcpAuthMode: async () => "static",
+ deleteMcpClientId,
+ }));And replace the weak assertion:
- expect(deletedId).toBeNull(); // must NOT clear for static mode
+ expect(deleteMcpClientId).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines
291 - 322, The static-mode invalid_client test still patches imported modules
directly, so update it to use the same jest.mock/jest.spyOn pattern as the DCR
test and add cleanup with afterEach or equivalent restore logic. In the
refreshMcpAccessToken test, verify deleteMcpClientId was not called via a mock
assertion instead of checking a null sentinel, while keeping the existing
invalid_client setup and the getMcpAuthMode static override.
| - [ ] Configure a Brain3 MCP connector in static OAuth mode. | ||
| - [ ] Manually corrupt the stored `client_secret` (via factory reset and re-save with wrong value) so the next refresh fails with `invalid_client`. | ||
| - [ ] Trigger a tool call that returns 401 to exercise the refresh path. | ||
| - [ ] Confirm Logfire shows `oauth_error_code: "invalid_client"` in the "Token refresh failed" log entry. | ||
| - [ ] Confirm `has_client_id: true` appears in the "Refreshing access token" log entry. | ||
| - [ ] For a DCR connector: confirm after `invalid_client` that `getMcpClientId` returns null (stale entry cleared) and re-auth triggers fresh DCR registration. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate DCR and static mode QA steps; fix mode mismatch.
Line 350 configures a "static OAuth mode" connector but then tests DCR-specific behavior (clearing stale entry, fresh DCR registration). Split into two distinct QA items:
- - [ ] Configure a Brain3 MCP connector in static OAuth mode.
- - [ ] Manually corrupt the stored `client_secret` (via factory reset and re-save with wrong value) so the next refresh fails with `invalid_client`.
- - [ ] Trigger a tool call that returns 401 to exercise the refresh path.
- - [ ] Confirm Logfire shows `oauth_error_code: "invalid_client"` in the "Token refresh failed" log entry.
- - [ ] Confirm `has_client_id: true` appears in the "Refreshing access token" log entry.
- - [ ] For a DCR connector: confirm after `invalid_client` that `getMcpClientId` returns null (stale entry cleared) and re-auth triggers fresh DCR registration.
+ - [ ] **Static mode QA:** Configure a Brain3 MCP connector in static OAuth mode. Corrupt the stored `client_secret` and confirm `invalid_client` appears in logs. Confirm re-auth with correct credentials fixes it (clientId is NOT cleared).
+ - [ ] **DCR mode QA:** Configure a Brain3 MCP connector in DCR OAuth mode. Corrupt or expire the DCR registration and confirm `invalid_client` appears in logs. Confirm `getMcpClientId` returns null after failure and re-auth triggers fresh DCR registration.
+ - [ ] Trigger a tool call that returns 401 to exercise the refresh path.
+ - [ ] Confirm Logfire shows `oauth_error_code: "invalid_client"` in the "Token refresh failed" log entry.
+ - [ ] Confirm `has_client_id: true` appears in the "Refreshing access token" log entry.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [ ] Configure a Brain3 MCP connector in static OAuth mode. | |
| - [ ] Manually corrupt the stored `client_secret` (via factory reset and re-save with wrong value) so the next refresh fails with `invalid_client`. | |
| - [ ] Trigger a tool call that returns 401 to exercise the refresh path. | |
| - [ ] Confirm Logfire shows `oauth_error_code: "invalid_client"` in the "Token refresh failed" log entry. | |
| - [ ] Confirm `has_client_id: true` appears in the "Refreshing access token" log entry. | |
| - [ ] For a DCR connector: confirm after `invalid_client` that `getMcpClientId` returns null (stale entry cleared) and re-auth triggers fresh DCR registration. | |
| - [ ] **Static mode QA:** Configure a Brain3 MCP connector in static OAuth mode. Corrupt the stored `client_secret` and confirm `invalid_client` appears in logs. Confirm re-auth with correct credentials fixes it (clientId is NOT cleared). | |
| - [ ] **DCR mode QA:** Configure a Brain3 MCP connector in DCR OAuth mode. Corrupt or expire the DCR registration and confirm `invalid_client` appears in logs. Confirm `getMcpClientId` returns null after failure and re-auth triggers fresh DCR registration. | |
| - [ ] Trigger a tool call that returns 401 to exercise the refresh path. | |
| - [ ] Confirm Logfire shows `oauth_error_code: "invalid_client"` in the "Token refresh failed" log entry. | |
| - [ ] Confirm `has_client_id: true` appears in the "Refreshing access token" log entry. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-25-fix-invalid-client-refresh.md` around lines
349 - 355, The QA checklist mixes static OAuth and DCR behaviors in the same
item, so split the Brain3 MCP connector tests into separate steps by auth mode.
Keep the static OAuth refresh failure checks (invalid_client, has_client_id)
together under the refresh path, and move the DCR-specific assertions
(getMcpClientId returns null, stale entry cleared, re-auth triggers fresh
registration) into a separate DCR connector QA item. Use the existing checklist
entries around the Brain3 MCP connector and token refresh logs as the anchors
when editing.
…to 0.3.4 Per MCP spec, the resource URI must be included in all token requests. Also adds client_id/secret hashes to refresh logs for credential debugging. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7NXmkDMaSkftFFEdcxVjs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/settings/McpExtensionDetailScreen.tsx`:
- Around line 228-258: The access-token reset action in McpExtensionDetailScreen
is shown for all connectors, but it only makes sense for OAuth flows and the
current copy assumes a refresh token exists. Update the UI around
handleResetAccessToken (and the related token action block it powers) to render
these actions only when the connector auth mode is OAuth, not for authMode:
"bearer". Make sure the gating uses the existing currentExtension/authMode logic
so bearer connectors do not show delete/reset token actions or the refresh-token
message.
In `@lib/mcp-oauth.ts`:
- Around line 301-305: The helper sha256Prefix currently lets
Crypto.digestStringAsync throw, which can break refreshMcpAccessTokenWithDetails
instead of returning a structured failure. Update sha256Prefix to handle hash
computation errors internally by wrapping the digest call in a try/catch and
falling back to a safe placeholder value, keeping OAuth refresh diagnostics
best-effort. Use the sha256Prefix symbol to locate the change and ensure the
refresh flow never rejects because of diagnostic hashing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0daa3937-61d8-442f-b4f1-5357d72af61f
📒 Files selected for processing (11)
.codex/config.tomlAGENTS.mdLICENSEapp.jsoncomponents/settings/McpExtensionDetailScreen.tsxdocs/plan-reset-access-token-button.mdlib/__tests__/mcp-oauth.test.tslib/app_version.tslib/mcp-oauth.tslib/secure-storage.tspackage.json
✅ Files skipped from review due to trivial changes (5)
- app.json
- package.json
- AGENTS.md
- lib/app_version.ts
- LICENSE
| const handleResetAccessToken = () => { | ||
| const doReset = async () => { | ||
| await deleteMcpBearerToken(currentExtension.id); | ||
| DeviceEventEmitter.emit(CONNECTOR_SETTINGS_CHANGED_EVENT); | ||
| }; | ||
|
|
||
| if (Platform.OS === "ios") { | ||
| ActionSheetIOS.showActionSheetWithOptions( | ||
| { | ||
| title: `Reset access token for "${currentExtension.name}"?`, | ||
| message: | ||
| "The access token will be dropped. The app will use the refresh token on the next MCP call.", | ||
| options: ["Cancel", "Reset Access Token"], | ||
| destructiveButtonIndex: 1, | ||
| cancelButtonIndex: 0, | ||
| }, | ||
| (buttonIndex) => { | ||
| if (buttonIndex === 1) doReset(); | ||
| } | ||
| ); | ||
| } else { | ||
| Alert.alert( | ||
| "Reset Access Token", | ||
| `Drop the access token for "${currentExtension.name}"? The app will use the refresh token on the next MCP call.`, | ||
| [ | ||
| { text: "Cancel", style: "cancel" }, | ||
| { text: "Reset Access Token", style: "destructive", onPress: doReset }, | ||
| ] | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hide OAuth-only token actions for bearer connectors.
These buttons are rendered unconditionally, but handleResetAccessToken deletes the stored bearer token. For authMode: "bearer", that is the only credential and the “use the refresh token” message is false. Gate these actions to OAuth modes.
Proposed fix
const [urlCopied, setUrlCopied] = useState(false);
const [refreshingAccessToken, setRefreshingAccessToken] = useState(false);
+ const supportsOAuthTokenActions =
+ currentExtension.authMode === "dcr" || currentExtension.authMode === "static";- <Pressable
- style={({ pressed }) => [
- styles.resetAccessTokenButton,
- pressed && styles.resetAccessTokenButtonPressed,
- ]}
- onPress={handleResetAccessToken}
- >
- <Text style={styles.resetAccessTokenButtonText}>Reset Access Token</Text>
- </Pressable>
- <Pressable
- disabled={refreshingAccessToken}
- style={({ pressed }) => [
- styles.refreshAccessTokenButton,
- pressed && styles.refreshAccessTokenButtonPressed,
- refreshingAccessToken && styles.refreshAccessTokenButtonDisabled,
- ]}
- onPress={handleRefreshAccessToken}
- >
- {refreshingAccessToken ? (
- <ActivityIndicator size="small" color="`#FFFFFF`" />
- ) : (
- <Text style={styles.refreshAccessTokenButtonText}>Refresh Access Token</Text>
- )}
- </Pressable>
+ {supportsOAuthTokenActions && (
+ <>
+ <Pressable
+ style={({ pressed }) => [
+ styles.resetAccessTokenButton,
+ pressed && styles.resetAccessTokenButtonPressed,
+ ]}
+ onPress={handleResetAccessToken}
+ >
+ <Text style={styles.resetAccessTokenButtonText}>Reset Access Token</Text>
+ </Pressable>
+ <Pressable
+ disabled={refreshingAccessToken}
+ style={({ pressed }) => [
+ styles.refreshAccessTokenButton,
+ pressed && styles.refreshAccessTokenButtonPressed,
+ refreshingAccessToken && styles.refreshAccessTokenButtonDisabled,
+ ]}
+ onPress={handleRefreshAccessToken}
+ >
+ {refreshingAccessToken ? (
+ <ActivityIndicator size="small" color="`#FFFFFF`" />
+ ) : (
+ <Text style={styles.refreshAccessTokenButtonText}>Refresh Access Token</Text>
+ )}
+ </Pressable>
+ </>
+ )}Also applies to: 397-419
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/settings/McpExtensionDetailScreen.tsx` around lines 228 - 258, The
access-token reset action in McpExtensionDetailScreen is shown for all
connectors, but it only makes sense for OAuth flows and the current copy assumes
a refresh token exists. Update the UI around handleResetAccessToken (and the
related token action block it powers) to render these actions only when the
connector auth mode is OAuth, not for authMode: "bearer". Make sure the gating
uses the existing currentExtension/authMode logic so bearer connectors do not
show delete/reset token actions or the refresh-token message.
| const sha256Prefix = async (value: string | null | undefined): Promise<string> => { | ||
| if (!value) return "(none)"; | ||
| const hash = await Crypto.digestStringAsync(Crypto.CryptoDigestAlgorithm.SHA256, value); | ||
| return hash.slice(0, 8); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n lib/mcp-oauth.ts | sed -n '295,315p'Repository: tleyden/arty
Length of output: 1074
🏁 Script executed:
cat -n lib/mcp-oauth.ts | sed -n '365,385p'Repository: tleyden/arty
Length of output: 845
🏁 Script executed:
cat -n lib/mcp-oauth.ts | sed -n '320,365p'Repository: tleyden/arty
Length of output: 2047
🏁 Script executed:
web_search
expo-crypto digestStringAsync throws error whenRepository: tleyden/arty
Length of output: 243
🌐 Web query:
expo-crypto digestStringAsync throws error when
💡 Result:
The expo-crypto digestStringAsync function can throw errors under several specific circumstances. Common causes include: 1. Web Origin Restrictions: When running in a web environment, this method must be called from a secure origin (HTTPS or localhost). If accessed from an insecure origin, it will throw an error [1][2]. 2. Native Module Configuration: If you encounter an error like "undefined is not an object" (referencing _ExpoCrypto), it typically indicates that the native module is not properly linked or the development client has not been rebuilt after installing the package. In Expo projects using custom native code or development clients, you must run npx expo run:android or npx expo run:ios (or rebuild your development client) after installation for the changes to take effect [3]. 3. Invalid Encoding: The function will throw an error if an invalid or unsupported encoding type is provided in the options [1][2]. 4. Improper Input Handling: While the function accepts a string, attempting to pass arbitrary byte arrays (such as buffers converted incorrectly to UTF-8 strings) can lead to incorrect hash results or errors if the bytes are invalid UTF-8 sequences. The function is designed specifically for string input [4][5]. If you are using Expo Go, ensure your app is fully updated, or if you have added new native dependencies, switch to a development client [3].
Citations:
- 1: https://docs.expo.dev/versions/latest/sdk/crypto/
- 2: https://docs.expo.dev/versions/unversioned/sdk/crypto/
- 3: undefined is not an object (evaluating '_ExpoCrypto.default.digestStringAsync')] expo/expo#18215
- 4: crypto SHA512 bug on encrypted values expo/expo#20706
- 5: https://stackoverflow.com/questions/57247825/how-to-hash-buffer-with-expo-crypto-digeststringasync
🏁 Script executed:
grep -n "refreshMcpAccessTokenWithDetails" lib/mcp-oauth.ts | head -20Repository: tleyden/arty
Length of output: 299
🏁 Script executed:
grep -n "sha256Prefix" lib/mcp-oauth.tsRepository: tleyden/arty
Length of output: 306
Wrap Crypto.digestStringAsync in sha256Prefix to prevent exceptions from blocking OAuth refresh diagnostics.
Crypto.digestStringAsync can throw in specific environments (e.g., insecure web origins, native module linkage issues). If it throws, refreshMcpAccessTokenWithDetails will reject instead of returning a structured failure result. Wrap the hash computation in a try block to ensure diagnostics remain best-effort and do not block the token refresh flow.
Apply the fix to sha256Prefix at lines 301–305.
Proposed fix
const sha256Prefix = async (value: string | null | undefined): Promise<string> => {
if (!value) return "(none)";
- const hash = await Crypto.digestStringAsync(Crypto.CryptoDigestAlgorithm.SHA256, value);
- return hash.slice(0, 8);
+ try {
+ const hash = await Crypto.digestStringAsync(Crypto.CryptoDigestAlgorithm.SHA256, value);
+ return hash.slice(0, 8);
+ } catch {
+ return "(unavailable)";
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sha256Prefix = async (value: string | null | undefined): Promise<string> => { | |
| if (!value) return "(none)"; | |
| const hash = await Crypto.digestStringAsync(Crypto.CryptoDigestAlgorithm.SHA256, value); | |
| return hash.slice(0, 8); | |
| }; | |
| const sha256Prefix = async (value: string | null | undefined): Promise<string> => { | |
| if (!value) return "(none)"; | |
| try { | |
| const hash = await Crypto.digestStringAsync(Crypto.CryptoDigestAlgorithm.SHA256, value); | |
| return hash.slice(0, 8); | |
| } catch { | |
| return "(unavailable)"; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mcp-oauth.ts` around lines 301 - 305, The helper sha256Prefix currently
lets Crypto.digestStringAsync throw, which can break
refreshMcpAccessTokenWithDetails instead of returning a structured failure.
Update sha256Prefix to handle hash computation errors internally by wrapping the
digest call in a try/catch and falling back to a safe placeholder value, keeping
OAuth refresh diagnostics best-effort. Use the sha256Prefix symbol to locate the
change and ensure the refresh flow never rejects because of diagnostic hashing.
…basic expo-auth-session sends credentials via Authorization Basic header when clientSecret is set directly. brain3-dev only accepts client_secret_post (credentials in POST body). Move client_secret into extraParams to force body inclusion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7NXmkDMaSkftFFEdcxVjs
1 participant
Summary by CodeRabbit