feat: add MCP permission grants#40
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR introduces persistent MCP tool permission management. New file-based storage ( ChangesMCP Persistent Permission Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
There was a problem hiding this comment.
Review: MCP permission grants
No blockers. Well-designed persistent permission store with identity enforcement and autonomy ceilings.
What's good
- Dual grant model — Server-level grants (blanket approval for all tools on a server) and tool-level grants (specific tool). Both identity-bound.
- Identity enforcement —
isGrantAllowedchecksserverIdentity. If server transport changes (new command/URL), identity changes and old grants don't apply. No stale permission leaks. - Autonomy ceiling —
low < medium < highhierarchy viaAUTONOMY_ORDER. Amediumgrant allowslowandmediumrequests, denieshigh. Clean ordinal comparison. - Atomic writes — Temp file + rename pattern with PID/timestamp/random suffix. Write lock via promise chain serializes concurrent operations.
- JSON schema mutation fix —
delete jsonSchema.$schema→ destructuring without$schema. Custom MCP tool schemas are no longer mutated. Tested. - Registry registration atomicity — Two-pass: first collects names and checks duplicates, second registers. No partial registration state on duplicate errors.
unregisteron ToolRegistry — Tests clean up after themselves. Prevents global registry pollution.- Session vs persistent grants —
createToolGrantKeycreatesmcp:{server}:{identity}:{tool}keys for MCP tools, orthogonal to session-level{permission}:{sideEffect}keys. Checked in order: persistent first, then session, then prompt. - CLI commands —
list,revoke <server> [tool],clear --confirmwith--jsonoutput.clearrequires explicit--confirmto prevent accidental nuke. validateToolName— Basic non-empty check. Acceptable since tool names come from MCP sanitization.- Test coverage — 361 new lines: grant/check/revoke with identity mismatch and autonomy ceiling enforcement, server grant covers unnamed tools, server revocation cascades, list ordering, clear with/without confirm, CLI integration, agent loop integration with persisted grants bypassing prompts.
Observations (non-blocking)
readStateparse errors — Re-throws with file path context but doesn't use{ cause: err }. OriginalSyntaxErrorline/col info is lost.validateToolName— No character validation (contrast withvalidateZeroMcpServerNameregex). Acceptable since tool names are already sanitized bycreateZeroMcpToolName.clear --confirmtruthy check —if (!options.confirm). Commander boolean flags aretrueorundefined, so this is correct.
|
Blockers Non-Blocking
Looks Good
Verdict: Approve — permission persistence foundation is identity-safe, test-backed, and still passes on current main. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Blockers
None found.
Non-Blocking
- Persistent grants are backend/CLI-management only in this PR: list/revoke/clear are wired, while creating persistent grants still depends on code paths/future UI, so the feature is safe but not fully user-facing yet.
Looks Good
- Clean MCP permission model: grants are bound to server identity, per-tool grants and server-wide grants are separated, autonomy ceilings fail closed, and server revocation cascades tool grants.
- Agent execution keeps MCP prompts gated unless a matching persistent grant exists, still routes all prompt/deny tools through
ToolRegistry.run, and serializes approval prompts so multi-tool turns cannot overwrite the TUI resolver. - MCP registry registration now refuses duplicate generated tool names before mutating the registry, and provider tool schema preparation avoids mutating custom MCP schemas.
- PR branch validation passed: typecheck, full tests
253 pass / 0 fail, build, smoke build, MCP permissions CLI help/list smoke, andgit diff --check HEAD. - Latest-main test merge also passed: clean merge onto
origin/main, typecheck, full tests253 pass / 0 fail, build, and smoke build.
Verdict: Approve — permission persistence foundation is identity-safe, test-backed, and still passes on current main.
Summary
Adds the M4 MCP permissions backend so Zero can persist trusted MCP server/tool grants and manage them from the CLI.
What changed
ZeroMcpPermissionStorefor durable per-server and per-tool MCP grants.low,medium,high) for persistent approvals.zero mcp permissions list,revoke, andclearCLI commands with JSON output support.Validation
npx --yes bun install --frozen-lockfilenpx --yes bun run typechecknpx --yes bun test ./tests --timeout 15000(253 pass,0 fail)npx --yes bun run buildnpx --yes bun run smoke:build./zero --help./zero mcp --help./zero mcp permissions --helpZERO_MCP_PERMISSIONS_PATH=/tmp/zero-mcp-permissions-pr-smoke.json ./zero mcp permissions list --jsongit diff --check HEADSummary by CodeRabbit
New Features
zero mcp permissionsCLI commands: list permissions, revoke server or tool grants, and clear all approvals.Bug Fixes