Skip to content

fix(server): preserve Claude read-only MCP hints with native permission checks - #32230

Open
Sidreyas wants to merge 2 commits into
pingdotgg:mainfrom
Sidreyas:sidreyas/fix-32185-claude-mcp-permission-checks
Open

Sidreyas wants to merge 2 commits into
pingdotgg:mainfrom
Sidreyas:sidreyas/fix-32185-claude-mcp-permission-checks

Conversation

@Sidreyas

@Sidreyas Sidreyas commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Problem

An unrelated Claude MCP deny rule such as mcp__buildkite__create_build strips the read-only hint from Datadog graph helpers in Full Access threads. The card prompts repeatedly, and the adapter's conservative deny guard refuses the call. Fixes #32185.

Change

Use Claude's advertised mcp_call_check_permissions_v1 capability for read-only MCP App calls in Full Access when no possibly covering ask rule is present. Preserve their read-only hints and send check_permissions: true, so Claude evaluates the actual tool, hidden server aliases, and wildcards before execution.

Keep the conservative fallback for older CLIs, unreadable rules, ask rules, other permission modes, and tools without read-only annotations. Server-prefix overlap and normalized tool-name collision checks remain in place. No client, wire-schema, default, or model-facing instruction changes.

Scope and approval

This is a focused adapter fix for the reproducible cross-server downgrade in #32185. It qualifies for the small, obvious bug exception. It retains the hidden-alias protection deliberately added in #31131; it does not infer rule coverage from visible server names. The new path relies on Claude's native permission evaluator, observed against the actual installed runtime.

Verification

Windows, Node 26.12.1, pinned Claude Agent SDK 0.3.316, installed Claude CLI 2.1.319.

  • Test first, from apps/server: vp test run src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts -t "permission rules" --maxWorkers 1. Baseline: 22 passed and the new unrelated-deny case failed because readOnlyHint was absent. Fixed: 27 passed, including legacy alias handling, native-capability fallback, unreadable rules, ask rules, approval mode, and a tool without a read-only annotation.
  • Full selected tests, from apps/server: vp test run --passWithNoTests --maxWorkers 1 src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts src/provider/AgentContextBudget.test.ts: 348 passed, one existing Unix-only case skipped on Windows. Both files passed. All three selected package typechecks passed; the server recheck followed a test-only optional-property typing correction.
  • vp run check:changed origin/main --concurrency 1 hit the known Windows pnpm-junction input scan defect (perf(scripts): check:changed on Windows no longer spends minutes walking node_modules (2,500 s → 5.0 s) #32023) before stages, with repeated Filename too long errors. Ran every selected equivalent from scripts/check-changed.ts: changed-file format/lint, both unchanged Knip tasks, server generate --check, affected/dependent typechecks for t3, @t3tools/evals, and @t3tools/scripts, plus both selected test files. Checks ran serially with RAYON_NUM_THREADS=1; Knip used its supported KNIP_DISABLE_RAW_TRANSFER=1 transport with unchanged analysis. Tests used the owning server package to avoid the unrelated root browser-port issue (fix(web): browser tests no longer pick a port Windows has reserved #24007).

Integrated runtime verification used the production T3 CLI/server with isolated state, real project/thread commands, OAuth-authenticated WebSocket RPC, the production Claude adapter and actual SDK/CLI, and a native stdio MCP fixture. A loopback Anthropic HTTP/SSE fixture supplied synthetic model responses; Claude itself performed permission checks. No paid model turn or live userdata was used.

Native model tool result -> persisted MCP App -> authenticated mcpApps RPC
unrelated Buildkite deny: toolInfo.readOnly=true; call succeeds; 1 MCP call
exact Datadog deny: native "Permission ... denied"; 0 MCP calls
Datadog tool wildcard deny: native "Permission ... denied"; 0 MCP calls
ask rule for hidden-alias-shaped server: toolInfo.readOnly=false
isolated server shutdown completed; E2E runner exit 0

Separate no-model native CLI probes also confirmed that a matching ask invokes can_use_tool, while a checked matching deny refuses execution. The strict fallback remains necessary on runtimes without the capability. This verifies backend metadata and call enforcement; it does not claim a live Datadog account or macOS/browser visual pass.

Implemented with OpenAI GPT-6 via the Codex harness.

Copilot AI balanced review requested due to automatic review settings November 6, 2026 22:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 6, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at c077362

Macroscope's review found this PR approvable — Read-only MCP card calls can retain their hints while using Claude's native permission checks. Capability, permission-mode, and rule gates limit the change, while unsupported paths retain existing conservative handling.

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

@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The Claude adapter now evaluates when MCP app tools can use Claude-side permission checks. It conditionally exposes read-only hints and forwards permission-check requests for eligible calls. Tests cover capability, permission mode, rule, and tool-annotation conditions.

Changes: Claude MCP permission checks

Layer / File(s) Summary
Permission-check eligibility and tool listings
apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts
The adapter checks SDK capability, permission mode, and rules before exposing readOnlyHint. Tests cover unrelated deny rules, hidden aliases, unreadable rules, approval-required mode, and tools without read-only annotations.
Conditional permission checks on app calls
apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts
The query-session call accepts optional checkPermissions. Eligible read-only tool calls set it, and the mcp_call control request includes check_permissions: true when requested. The local deny-rule guard remains for calls that do not use permission checks.

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant MCPApp
  participant ClaudeAdapterV2
  participant ClaudeAgentSdkQuerySession
  participant Claude
  MCPApp->>ClaudeAdapterV2: Request tool call
  ClaudeAdapterV2->>ClaudeAdapterV2: Check read-only annotation and permission eligibility
  ClaudeAdapterV2->>ClaudeAgentSdkQuerySession: Call tool with checkPermissions when eligible
  ClaudeAgentSdkQuerySession->>Claude: Send mcp_call with check_permissions when requested
Loading

Possibly related PRs

  • pingdotgg/t3code#24402: Changes how managed policy switches select bypassPermissions, a permission mode used by this PR’s eligibility checks.

Suggested labels: bugfix

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 5a749

No actionable issue remains from this review. The change is mergeable after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the main server change: preserving Claude read-only MCP hints through native permission checks.
Description check Passed The description includes all required sections and provides clear problem context, implementation details, scope justification, focused test results, runtime verification, and known limitations.
Linked Issues check Passed [#32185] The adapter enables Claude's mcp_call_check_permissions_v1 only for annotated read-only tools in Full Access, with readable rules and no possibly covering ask rule. It preserves `readOnly…
Out of Scope Changes check Passed The changes are limited to Claude MCP App permission evaluation, SDK request plumbing, and focused adapter tests. These changes directly support [#32185]. No unrelated client, wire-schema, default, or…
Full details: Linked Issues check

Explanation

[#32185] The adapter enables Claude's mcp_call_check_permissions_v1 only for annotated read-only tools in Full Access, with readable rules and no possibly covering ask rule. It preserves readOnlyHint and sends check_permissions: true for that path. Claude therefore evaluates unrelated, exact, and wildcard rules against the actual tool. The adapter retains the conservative deny guard for legacy, unreadable, approval-required, ask-rule, and non-read-only cases. Added tests cover the unrelated deny regression and these fallback cases.

Full details: Out of Scope Changes check

Explanation

The changes are limited to Claude MCP App permission evaluation, SDK request plumbing, and focused adapter tests. These changes directly support [#32185]. No unrelated client, wire-schema, default, or model-instruction changes are shown.

Finishing Touches

Generate unit tests (beta)

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

Run configuration

Commits

Reviewing files that changed from the base of the PR and between c077362 and 5a749c4.

Files selected for processing: 2

  • apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts

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

Wladefant

This comment was marked as spam.

@Sidreyas

Sidreyas commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the scoped review. To clarify the existing runtime evidence: verification used an isolated production T3 CLI server, authenticated WebSocket mcpApps.toolInfo/callTool, the real Claude SDK and CLI 2.1.319, and a native stdio MCP fixture. A local Anthropic protocol fixture supplied the synthetic tool-use turn; it did not implement permission decisions. An unrelated deny allowed exactly one read-only MCP call, while exact and wildcard denies produced native Permission denied with zero MCP tool calls. A hidden-alias-shaped ask rule suppressed the read-only hint, and no app call was sent without approval.

This does not establish every hidden-alias deny spelling or a live Datadog/browser session. I am keeping that distinction explicit rather than treating the existing exact/wildcard cases as exhaustive hidden-alias qualification. The conservative alias/ask and legacy capability fallbacks remain in place; no broader runtime acceptance is claimed.

Copy link
Copy Markdown
Member

Evidence

Evidence uses a synthetic local MCP graph fixture through the real Claude CLI and real T3 web app; Datadog itself was not available. Both sides used fresh isolated databases, Full Access, the same fixture, a 1280×800 viewport, and light theme.

Steps: configure an unrelated mcp__buildkite__create_build deny rule with no ask rules; request the native graph app; click Load / reload graph; confirm if prompted. Then deny mcp__graph__get_graph_data, start a new thread, and repeat.

Before After
b3fdf2b10f 5a749c4245
Before: unrelated deny causes helper refusal After: unrelated deny allows prompt-free graph loading
Recording: confirmation followed by refusal Recording: graph loads without confirmation
Before: explicit helper deny After: explicit helper deny remains enforced
Explicit-deny recording Explicit-deny recording

Reproduction seed script · Synthetic MCP server and graph app

AI-assisted; verified with the tests and tools listed above.

Not shown:

  • Datadog's actual MCP server and native graph were not exercised: no Datadog configuration or credentials were available. The observed before/after difference uses a clearly labeled synthetic MCP graph fixture.
  • Alias and wildcard deny matching were not exercised; the runtime denial check covered the actual helper's exact tool name.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unrelated Claude MCP deny rules strip read-only hints from Datadog graphs, causing repeated approvals

4 participants