Add permission request notification tool and skill#1
Add permission request notification tool and skill#1davidkelley wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new MCP tool for sending “permission request” desktop notifications (with command/reason/risk metadata) and introduces a corresponding agent skill document.
Changes:
- Added
notify_permission_requesttool schema + server-side handler path in the MCPtools/callendpoint. - Added validation helper for permission-request fields and included the new tool in
tools/list. - Added a new skill doc describing how/when to use the permission-request notification tool.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src-tauri/src/lib.rs |
Implements the new MCP tool, request parsing/validation, and notification formatting. |
skills/notify-on-permission-request/SKILL.md |
Documents usage guidelines and an example call for the new tool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let timeout_seconds = arguments.get("timeoutSeconds").and_then(Value::as_i64); | ||
| let context_url = arguments.get("contextUrl").and_then(Value::as_str); |
There was a problem hiding this comment.
timeoutSeconds/contextUrl are treated as optional by using and_then(...), but if the caller provides them with the wrong JSON type (e.g. string instead of integer), they’ll be silently ignored and the request will still succeed. Consider explicitly rejecting requests where these keys are present but not the expected type, to match the stricter behavior used for required string fields.
| let timeout_seconds = arguments.get("timeoutSeconds").and_then(Value::as_i64); | |
| let context_url = arguments.get("contextUrl").and_then(Value::as_str); | |
| let timeout_seconds = match arguments.get("timeoutSeconds") { | |
| Some(v) => { | |
| if let Some(t) = v.as_i64() { | |
| Some(t) | |
| } else { | |
| return ( | |
| StatusCode::OK, | |
| Json(jsonrpc_error( | |
| Some(id), | |
| -32602, | |
| "Invalid params: permission request fields are required and must be within limits", | |
| )), | |
| ) | |
| .into_response(); | |
| } | |
| } | |
| None => None, | |
| }; | |
| let context_url = match arguments.get("contextUrl") { | |
| Some(v) => { | |
| if let Some(u) = v.as_str() { | |
| Some(u) | |
| } else { | |
| return ( | |
| StatusCode::OK, | |
| Json(jsonrpc_error( | |
| Some(id), | |
| -32602, | |
| "Invalid params: permission request fields are required and must be within limits", | |
| )), | |
| ) | |
| .into_response(); | |
| } | |
| } | |
| None => None, | |
| }; |
| .unwrap_or_default(); | ||
|
|
||
| let mut lines = vec![ | ||
| format!("{agent} needs approval"), |
There was a problem hiding this comment.
The permission-request notification body includes a first line with the agent name ("{agent} needs approval"), but dispatch_notification already prefixes the body with <agent>:. This results in duplicated agent text in the displayed notification (e.g. codex: codex needs approval ...). Consider removing the agent from the body lines (or adjusting dispatch_notification for this tool) so the agent only appears once.
| format!("{agent} needs approval"), | |
| "Permission request".to_string(), |
| let content = lines.join("\n"); | ||
|
|
||
| if let Err(err) = | ||
| dispatch_notification(&state, "Permission needed", &content, &agent) |
There was a problem hiding this comment.
Unlike the notify tool, the permission-request path doesn’t enforce the existing soft content limit (SOFT_CONTENT_LIMIT_CHARS) before calling dispatch_notification. As a result, long command/reason values can be silently truncated by MAX_NOTIFICATION_BODY_CHARS, potentially dropping the most important context. Consider validating the constructed content length (accounting for the <agent>: prefix) and returning -32602 when it exceeds the limit.
| "risk": { "type": "string", "enum": ["low", "medium", "high"] }, | ||
| "timeoutSeconds": { "type": "integer", "minimum": 1, "maximum": 86400 }, | ||
| "contextUrl": { "type": "string", "format": "uri" } | ||
| }, |
There was a problem hiding this comment.
The tool schema declares contextUrl with JSON Schema format: "uri", but the server-side validation currently only trims the string and doesn’t verify it’s a valid URI. To keep the contract accurate, either validate contextUrl as a URI (and reject invalid values) or remove the format constraint from the schema.
| - `reason` (string, required): why this is needed. | ||
| - `agent` (string, required): short id of the asking agent/workflow. | ||
| - `risk` ("low" | "medium" | "high", required): risk level. | ||
| - `timeoutSeconds` (int, optional): seconds to wait before timing out/auto-deny. |
There was a problem hiding this comment.
timeoutSeconds is documented as "seconds to wait before timing out/auto-deny", but this MCP tool only sends a notification and doesn’t implement any timeout/auto-deny behavior. Consider rewording this to clarify it’s a response deadline for the user/agent workflow (enforcement happens elsewhere), to avoid implying the server will auto-deny.
| - `timeoutSeconds` (int, optional): seconds to wait before timing out/auto-deny. | |
| - `timeoutSeconds` (int, optional): number of seconds after which the permission request should be treated as expired by the user/agent workflow; this tool only sends the notification (any timeout/auto-deny is handled elsewhere). |
No description provided.