Sign up
View stargazersSign up

feat(server): one access gate for every T3 MCP tool#15961

merged0
t3code/mcp-oauth/copy-url
t3code/mcp-oauth/access-gate
opened25d agobyJJulius Marminge
0 files changed
View on GitHubTwo-way sync enabledPR last imported 9d ago

Part 4 of the MCP stack (#15220 → #15222 → this). Based on #15222.

Problem

Each T3 MCP tool picked its own permission checks from about a dozen helpers, and they had drifted:

  • t3_thread_launch refused every caller below Full access with a hand-written check, so a Supervised or plan-mode thread couldn't do cross-project orchestration, even though an outside agent approved at Supervised could.
  • It also never capped the interaction mode it passed on.
  • "The caller's turn is still running" existed in six slightly different versions.
  • A new tool could ship without anyone deciding who may call it.

Fix

  • One table: every tool is listed in McpToolAccess by what it does:
    • reads
    • changes threads (and which input fields name them)
    • starts threads (and which input fields request modes)
    • changes the environment
    • acts as the calling thread
  • One gate: all registrations go through it, and it checks the table before any handler runs.
    • A caller never starts or changes a thread with broader runtime or interaction modes than its own: a T3 thread's own modes, or an outside agent's approved ceiling.
    • A thread caller must still own a live run.
    • A read-only client only reads.
    • Projects and environment settings need full access.
    • Tools that act as the calling thread need one.
    • If a thread lookup fails, the call is refused.
  • No new tool ships without a decision: a tool missing from the table fails registration, and a test checks the table matches the registered tools exactly.
  • Replaces the read-only wrapper from #15220, along with its ReadOnlyClientSafe escape hatch.
  • Visible change: a Supervised or plan-mode thread can now launch threads in any project at its own modes or below, and launch caps the interaction mode it passes on. The tool description and agent instructions say so.
  • Handlers keep the checks only they can make, such as a queued run or task belonging to its thread. This PR doesn't remove the per-handler checks the gate now duplicates; that can follow once this has run for a while.

Verification

  • vp test run src/mcp src/auth/McpOAuth.test.ts (server, in a PID namespace): 24 files, 212 tests pass.
  • New table test (McpToolAccess.test.ts): 41 caller × tool cases, covering:
    • Supervised and plan-mode threads launching within and above their modes
    • sending to threads above the caller's modes
    • a thread whose run ended
    • read-only, Supervised and Full-access outside agents
    • full-access-only tools
    • tools that act as the calling thread
  • New end-to-end test through the real MCP registration: a Supervised plan-mode thread launches into another project at its own modes, and is refused runtimeMode: full-access and interactionMode: default.
  • I checked the table against each tool's Readonly annotation. It differs on two tools, both on purpose:
    • t3_thread_read is a read here. It used to be callable by read-only agents through ReadOnlyClientSafe.
    • t3_worktree_status is "acts as the calling thread", which outside agents were already refused.
  • In the full src/provider run, 7 process-tree / OpenCode-server tests fail. The same 7 fail on the base branch in the same sandbox, so they're environmental.

Not a visual change.

Model: Claude Opus 5.5 (1M context) via T3 Code's Claude Code harness.

🤖 Generated with Claude Code

Devin Review

Checks

22 successful · 7 skipped
All checks passed
9bbabe9
Ggithub-actionsBot25d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire—5.0 KiB—6.8 KiB✅
CodexThread snapshot wire—3.8 KiB—4.9 KiB✅
CodexLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
CodexLive turn WebSocket decoded—20.9 KiB—29.3 KiB✅
CodexLive turn messages—2—8✅
ClaudeTotal thread wire—5.0 KiB—6.8 KiB✅
ClaudeThread snapshot wire—3.8 KiB—4.9 KiB✅
ClaudeLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
ClaudeLive turn WebSocket decoded—21.2 KiB—29.3 KiB✅
ClaudeLive turn messages—1—8✅

Baseline: unavailable · PR result: 9b2326d · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

apps/server/src/mcp/McpToolAccess.tslines 353–353Outdated · 9b2326dOpen thread
……@@ -0,0 +1,380 @@
350+ * `TOOL_ACCESS` is checked before its handler runs, and a tool missing from
351+ * the table fails registration.
352+ */
353+export const gatedServer = (
MmacroscopeappBoton lines 353–35324d ago

gatedServer constructs a production McpServer service by accepting another service instance and a callback closing over ThreadManagementService. This hides its dependencies behind factory parameters rather than acquiring them from Effect's environment. Consider making the decorator an Effect constructor that yield* acquires both services and performs the shell lookup there; the registration sites can then use that constructor directly. This spans the constructor and its call site, so there isn't a self-contained inline suggestion.

Posted via Macroscope — Effect Service Conventions

MmacroscopeappBot24d ago
Synced from GitHub

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces a substantial centralized authorization layer that changes runtime access behavior across every MCP tool and modifies thread-launch permissions. Because it affects security-sensitive authentication/authorization paths and includes an unresolved architectural concern, human review is warranted.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot24d ago
Synced from GitHub

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (2)
1docs/internals/effect-services.md — auto-discovered
2AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 5f5a9149-8430-4fd9-a40e-5c99b131f837
📥 Commits

Reviewing files that changed from the base of the PR and between 656af40a204d60f5fd99a119bec677f33986592b and 9b2326de2fe1d2972f19d45f43dcc4ef1aaa6e11.

📒 Files selected for processing (15)
  • apps/server/src/mcp/McpDeviceToolkit.test.ts
  • apps/server/src/mcp/McpHttpServer.test.ts
  • apps/server/src/mcp/McpHttpServer.ts
  • apps/server/src/mcp/McpInvocationContext.ts
  • apps/server/src/mcp/McpToolAccess.test.ts
  • apps/server/src/mcp/McpToolAccess.testkit.ts
  • apps/server/src/mcp/McpToolAccess.ts
  • apps/server/src/mcp/threadAccess.ts
  • apps/server/src/mcp/toolkits/core.test.ts
  • apps/server/src/mcp/toolkits/orchestrator/tools.ts
  • apps/server/src/mcp/toolkits/project/handlers.test.ts
  • apps/server/src/mcp/toolkits/project/handlers.ts
  • apps/server/src/mcp/toolkits/project/tools.ts
  • apps/server/src/provider/T3OrchestrationInstructions.ts
  • docs/internals/environment-auth.md
💤 Files with no reviewable changes (2)
  • apps/server/src/mcp/toolkits/orchestrator/tools.ts
  • apps/server/src/mcp/McpInvocationContext.ts

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

📝 Walkthrough

Walkthrough

MCP tool registration now applies table-driven access checks based on caller credentials, thread state, and mode limits. Thread launches resolve runtime and interaction modes within the caller’s limits.

Changes

MCP access and launch behavior

Layer / File(s)Summary
Define tool access policies and checks
apps/server/src/mcp/McpToolAccess.ts, apps/server/src/mcp/McpToolAccess.test.ts, apps/server/src/mcp/McpToolAccess.testkit.ts, apps/server/src/mcp/McpInvocationContext.ts, apps/server/src/mcp/threadAccess.ts, apps/server/src/mcp/toolkits/orchestrator/tools.ts		Tools receive access classifications and checks for credentials, thread state, and runtime or interaction-mode limits. Tests cover access outcomes and thread-shell lookup failures.
Apply access checks during tool registration
apps/server/src/mcp/McpHttpServer.ts, apps/server/src/mcp/McpHttpServer.test.ts, apps/server/src/mcp/McpDeviceToolkit.test.ts, apps/server/src/mcp/toolkits/core.test.ts, docs/internals/environment-auth.md		Toolkit and manually registered tools use the access-gated server. Test layers provide live thread shells, and the authorization documentation describes the access table.
Resolve launched-thread modes within caller limits
apps/server/src/mcp/toolkits/project/handlers.ts, apps/server/src/mcp/toolkits/project/handlers.test.ts, apps/server/src/mcp/toolkits/project/tools.ts, apps/server/src/provider/T3OrchestrationInstructions.ts		t3_thread_launch resolves requested modes within caller limits. Tests cover allowed launches and denials when a request exceeds those limits.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~30 minutes

Change: Feature

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant MCPClient
3 participant MCPServer
4 participant McpToolAccess
5 participant ThreadManagementService
6 participant ToolkitHandler
7 MCPClient->>MCPServer: Invoke registered tool with thread scope
8 MCPServer->>McpToolAccess: Check tool access
9 McpToolAccess->>ThreadManagementService: Get caller thread shell
10 ThreadManagementService-->>McpToolAccess: Return thread shell
11 McpToolAccess-->>MCPServer: Allow or return structured denial
12 MCPServer->>ToolkitHandler: Invoke handler when access is allowed

Merge Risk: ⚪ Minimal · up to 9b232

No concrete access regression is established for this change; it is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Description check⚠️ WarningThe description explains the problem, change, and verification in detail. It does not include the required scope and approval information, and the described behavior change is not presented as a small…Add the triaged issue or discussion that includes explicit maintainer approval of the direction and scope. If no prior approval is required, explain why this change qualifies for that exception.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly identifies the main change: one access gate for every T3 MCP tool.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, change, and verification in detail. It does not include the required scope and approval information, and the described behavior change is not presented as a small, obvious fix.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

JJulius Marminge24d ago

GitHub marked this merged by mistake: I force-pushed its branch while reordering the stack, the new head was already in its old base branch, and GitHub closed it as merged and deleted the branch. Nothing from it reached main. The work continues in #16335, now the bottom of a new stack (#16335 → #16336 → #16337), with access checked by the compiler instead of a runtime table.