Sign up
View stargazersSign up

fix(server): refresh MCP credentials only from trusted lifecycle#30589

open3
main
GitHub fork: draft/mcp-credential-liveness
opened11d agobyMmahruskazi
2 files changed
View on GitHubTwo-way sync enabledPR last imported 4h ago

Problem

Resolving an MCP credential refreshes its liveness, so client requests can keep it valid after trusted provider activity stops. Expected behavior is for only trusted provider lifecycle refreshes to extend that window.

Change

Make credential resolution read-only with respect to liveness. Retain trusted refresh and revocation behavior.

Scope and approval

This is a small, focused correction to the existing credential-liveness contract. No new capability or default is introduced. No prior maintainer approval is claimed. Submitted as a draft for review.

Verification

The recorded regression fails before the fix and passes afterward: a request at 99ms no longer keeps a 100ms credential valid at 101ms. Trusted refresh and revocation tests continue to pass. The prepared candidate passed vp run check:changed with 10 tests across 2 files. This publication preserves the tested tree; repo-wide CI has not been run locally. Final independent review is pending.

Models: GPT-6.1 Sol and Claude Opus 5.5. Harnesses: Codex and Claude Code in T3 Code.

Checks

21 successful · 11 skipped
All checks passed
6a71ace

Added 2 commits

  • fix(server): refresh MCP credentials only from trusted lifecycleccf28ab
  • Merge branch 'main' into draft/mcp-credential-livenessbade789
CcoderabbitaiBot9d ago
Synced from GitHub

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Valid token resolution no longer refreshes the credential’s liveness timestamp. The expiration test checks that the token resolves after 99 ms and expires after 101 ms.

Changes: MCP credential liveness

Layer / File(s)Summary
Credential resolution and expiration test
apps/server/src/mcp/McpSessionRegistry.ts, apps/server/src/mcp/McpSessionRegistry.test.ts		Valid token resolution returns the recorded scope without updating lastAlive. The test checks resolution before the liveness window expires and expiration after it.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: maria-rcks, juliusmarminge

Merge Risk: External image Minimal · up to 6a71a

MCP credentials no longer stay alive just because they are used for requests; only trusted lifecycle events refresh them. The change is narrow and tested, and no merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check nameStatusExplanation
Title checkPassedThe title states the main change: credential refresh is restricted to trusted lifecycle events. The raw summary confirms that resolve no longer refreshes lastAlive and only touch refreshes liveness, w…
Description checkPassedThe description follows the template. It has Problem, Change, Scope, and Verification sections with substantive content. The Problem and Change sections state the bug and the fix. The Scope section sa…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title states the main change: credential refresh is restricted to trusted lifecycle events. The raw summary confirms that resolve no longer refreshes lastAlive and only touch refreshes liveness, which matches the title. The title uses the conventional commit form and is specific.

Full details: Description check

Explanation

The description follows the template. It has Problem, Change, Scope, and Verification sections with substantive content. The Problem and Change sections state the bug and the fix. The Scope section says it is a bug fix and states that no prior maintainer approval is claimed. The Verification section gives a specific regression result and the checks that ran. It also states what was not checked. The template's optional trailer about the agent model is present. The description does not link an issue, and the Scope section does not explain why no issue is needed. That gap is minor, so the description is mostly complete.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between bade789580b108fa9f3a7f680fd4d64a4fc589a4 and 6a71acecb0c732302c4fc561943c936132346e76.

External image Files selected for processing: 2

  • apps/server/src/mcp/McpSessionRegistry.test.ts
  • apps/server/src/mcp/McpSessionRegistry.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Mmaria8h ago

Note

Written by gpt-6.1-sol on behalf of Maria

merged current main a5d21a3ce466d143c74f7bb3810b290d9e99eb51 into the existing head without rebasing. pushed 6a71acecb0c732302c4fc561943c936132346e76 to the same contributor branch without force.

resolved the registry conflict by keeping main's monotonic-clock and sticky-expiry protections while retaining this pr's rule that credential reads cannot refresh liveness. trusted lifecycle refresh and recovery remain intact. the diff against main remains two files, four additions and four deletions. no actionable bot findings were present.

blacksmith verification on the merged head: all 52 existing registry/http tests passed. vp run check:changed a5d21a3ce466d143c74f7bb3810b290d9e99eb51 passed format, lint, knip, graphql, all three affected package typechecks, and 12 scoped tests. two independent source reviewers approved this same head without findings.

the real-provider evidence below belongs to the previous head and was preserved. the merged head has not been rerun through the live provider/client path.

Verified in a real dev environment

verified unchanged head bade789580b108fa9f3a7f680fd4d64a4fc589a4 against github/main at 6615615b1259c3ae6c1fdd0b2e8b70650289ce85 on a disposable linux vm.

used repository playwright with /usr/bin/google-chrome, added a real git repository through the web app, and submitted real codex gpt-6.1-sol turns through the local proxy. the provider called orchestrator_capabilities through its injected t3 mcp connection. no mocks, fixtures, seeded threads, or source edits.

paused time sync and advanced the actual vm clock to exercise the unchanged 24-hour window. clock and time sync were restored afterward.

  • main: baseline, +23h, and +25h requests all returned http 200. the +23h request incorrectly extended credential validity.
  • pr: baseline and +23h returned http 200; +25h returned http 401, "authentication required", despite the intervening request.
  • nearby behavior: a new trusted user turn in the same thread at +25h recovered mcp access. an invalid bearer still returned http 401.
  • blacksmith: 38 existing credential/http tests passed. vp run check:changed 25b53299b75d4d8d1d0667ad47e9cfed3749776e passed format, lint, knip, graphql, affected typechecks, and 11 scoped tests. the explicit ref is the pr base commit.

short runtime transcript

main reproduction:

main accepts the credential at 25 hours after a request at 23 hours

pr result:

pr rejects the expired credential through the real codex mcp connection

short animation inspecting the actual expiry failure and subsequent trusted-turn recovery in the app tool history. this shows recorded turn results, not 25 hours of elapsed playback:

inspecting the real mcp authentication failure and trusted-turn recovery

Not verified

native desktop/mobile clients, other providers, remote/relay connections, natural 24-hour idleness, and the hourly background refresh were not exercised. live revocation of a previously valid bearer was not exercised; existing registry tests covering revocation passed. the clock checks advance wall time and do not simulate monotonic time or host suspension. no performance claim is made.

Added 1 commit

  • chore: merge main into draft/mcp-credential-liveness6a71ace