Repository navigation
Conversation
…'s T3 tool access A thread kept a single MCP credential. Opening a second provider instance on the same thread revoked and replaced it, so a provider process that still held the old token got 401 errors after switching back, or read a token scoped to the other instance. Credentials are now kept per thread and provider instance. Every adapter reads its own instance's credential, rotation revokes only the credential being replaced, and release only defers revocation to other live sessions of the same instance. Terminal archive/delete still revokes every credential for the thread. Kept html_render references are dropped only when the thread's last credential goes, so releasing one instance no longer discards another's pages. Replaces pingdotgg#19413, rebuilt on current main from the maintainer takeover. Fixes pingdotgg#16565 Refs: pingdotgg#18852 Co-Authored-By: Julius Marminge <julius0216@outlook.com> Co-Authored-By: Claude Opus 5.5 (implementation and integration) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 (review subagent) <noreply@anthropic.com> Co-Authored-By: GPT-6.1 Sol (rebase draft) <noreply@openai.com>
|
Closing: Alb11747 already has 3 or more open PRs here, which is the limit for contributors who are not vouched. Once one of them is merged or closed, open this again. See the open PR limit. |
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| const current = yield* Ref.get(sessions); | ||
| if (current.get(config.threadId)?.providerSessionId !== config.providerSessionId) { | ||
| const previous = current.get(config.threadId)?.get(config.providerInstanceId); | ||
| if (previous?.providerSessionId !== config.providerSessionId) { |
There was a problem hiding this comment.
🟡 Medium server/McpProviderSessions.ts:114
Registering or rotating provider instance B deletes every pending HTML render for the thread even while instance A still has a live session, so a late ACP completion for A gets undefined from takeHtmlRender and loses its page attachment. The condition checks only B's previous session, which is undefined on first registration; invalidate renders only when no other instance has a session.
| if (previous?.providerSessionId !== config.providerSessionId) { | |
| if ( | |
| previous?.providerSessionId !== config.providerSessionId && | |
| !Array.from(current.get(config.threadId) ?? []).some( | |
| ([instanceId]) => instanceId !== config.providerInstanceId, | |
| ) | |
| ) { |
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/provider-core/src/server/McpProviderSessions.ts around line 114:
Registering or rotating provider instance B deletes every pending HTML render for the thread even while instance A still has a live session, so a late ACP completion for A gets `undefined` from `takeHtmlRender` and loses its page attachment. The condition checks only B's previous session, which is `undefined` on first registration; invalidate renders only when no other instance has a session.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — Provider switching now preserves each provider instance’s MCP tool credentials while retaining targeted revocation and full-thread cleanup. A reported HTML attachment-loss issue remains subject to the separate blocking correctness gate. Needs human approval because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
Fold fork-only follow-ups into the entry of the feature they extend, and point the Claude native-steer and MCP credential entries at their replacement upstream PRs (pingdotgg#35462, pingdotgg#35331) while keeping the old URLs. Sync now leaves lines that name a merged PR only as `Related PR:`, so a fork-only follow-up listed under a change with an Upstream-PR trailer is no longer deleted while its commit stays. The guide says when to combine entries and when a follow-up keeps its own line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
A thread holds only one T3 tool (MCP) credential. Opening a second provider instance on the same thread revokes that credential and replaces it. The first provider's process keeps the old token. When the thread switches back (A → B → A), that provider's T3 tool calls fail with
401 Authentication requiredor read a token scoped to the other instance (parent_not_active).Change
Credentials are kept per thread and provider instance:
McpProviderSessionsstores one config per thread and provider instance.readtakes the instance id, andclearcan remove a single credential.readMcpSessionhelper, including thehasPendingWakeread added since fix(server): switching providers no longer revokes the other provider's T3 tool access #19413.ProviderSessionManagerrevokes only the credential it is replacing. On release, it defers revocation only to other live sessions of the same instance. Archive and delete still revoke every credential on the thread.html_renderreferences are dropped only when the thread's last credential goes. Otherwise, releasing one instance would discard pages that another instance had not yet consumed.Scope
This is a fresh PR from current
mainthat replaces #19413, as the closing comment asked. It starts from @juliusmarminge's maintainer takeover of #19413 ata569956, which preserved the original fix. It adds three things on top: the Codex read site that landed onmainsince then, a regression test that switches providers the way the orchestrator does, and thehtml_rendercleanup fix from review.Fixes #16565. Refs #18852.
Verification
The regression test is
ProviderSessionManagerV2 preserves MCP access through A → B → A and isolates cleanupinapps/server/src/orchestration-v2/ProviderSessionManager.test.ts. It follows the orchestrator's real switch flow:It checks that both instances' tokens still resolve to the thread, that A gets back its original credential on the existing runtime, and that closing B revokes only B's credential and keeps a render that A recorded. Reopening B issues a fresh token.
mainat 2482b03, with only the test file applied):vp test run src/orchestration-v2/ProviderSessionManager.test.tsgives 1 failed, 65 passed. The new test fails at the first access check after B opens. A's token resolves toundefinedbecause opening B revoked it. This is the failure bompus reported when the automated capture missed it.html_rendercleanup: with only that change reverted, the test fails on the kept-render assertion.vp run check:changed: format, lint, knip, typecheck (27 packages) and tests beside the change (8 files, 880 tests) all pass.What I could not check: authenticated Claude/Codex turns that switch providers in the UI were not run live. The test drives the session manager and credential registry with fake adapters. There is no UI change.
Implementation and integration: Claude Opus 5.5 in Claude Code, through T3 Code. Rebase draft: GPT-6.1 Sol. Review: Claude Opus 5.5 (fresh-context subagent). It is built on the original fix by Alb11747 and Julius Marminge's takeover.
🤖 Generated with Claude Code