feat(permissions): real tool-permission system (macro-1509)#4201
Open
ehayes2000 wants to merge 1 commit into
Open
feat(permissions): real tool-permission system (macro-1509)#4201ehayes2000 wants to merge 1 commit into
ehayes2000 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements an end-to-end tool permission system. In the Rust agent crate, a new 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
rust/cloud-storage/agent/src/agent_loop.rs (1)
267-267: ⚡ Quick winInclude
erron theinvoke_agentspan.Line 267 instruments a
Result-returning function but omitserr, so failures fromsend_messagewon’t be captured on the span.As per coding guidelines, “Include
errwhen adding thetracing::instrumentattribute to functions that returnResult”.Suggested change
- #[tracing::instrument(name = "invoke_agent", skip_all)] + #[tracing::instrument(name = "invoke_agent", skip_all, err)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/agent/src/agent_loop.rs` at line 267, The tracing::instrument attribute on the invoke_agent function is missing the err parameter, which prevents error information from being captured on the span. Add err to the tracing::instrument macro (after skip_all) so that when the function returns an error (particularly from send_message), the error details are properly captured and recorded on the span for better observability and debugging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app/packages/core/component/AI/component/tool/PermissionRequest.tsx`:
- Around line 24-26: The handleDeny function in PermissionRequest component only
updates the local UI state with setDenied(true) but does not send a backend
signal to resolve the server-side pending call. To fix this, modify the
handleDeny function to not only update the local denied state but also send a
request or signal to the backend that communicates the permission denial. This
ensures the server-side pending call is properly resolved and the stream is
resumed instead of remaining suspended until timeout.
- Around line 14-22: The handleGrant function sets the granted state to true
immediately without waiting for the API response or checking the actual result
from cognitionApiServiceClient.grantTool(). Refactor the function to await the
grantTool call, capture the response object, and only set setGranted to true if
the response.granted field indicates success. This ensures the UI state
accurately reflects whether the permission grant was actually confirmed by the
server.
- Around line 2-3: The PermissionRequest component is making direct network
calls to cognitionApiServiceClient.grantTool instead of using TanStack Query
mutations. Create a new TanStack Query mutation in the queries package that
wraps the grantTool API call, then remove the direct cognitionApiServiceClient
import from PermissionRequest and replace the direct grantTool calls (lines
16-21) with an imported call to the new mutation hook. This ensures all network
operations follow the established pattern of going through TanStack Query.
In `@rust/cloud-storage/agent/src/agent_loop.rs`:
- Around line 443-455: The grant waiting loop in the agent_loop.rs file only
handles approval (matching tool_call_id), stale grants, and channel closure, but
lacks support for user denial cases which causes the stream to hang until outer
timeout/drop. Modify the grant message structure to include a denial variant (in
addition to approval), then update the match statement within the loop (around
the Some(g) if g.tool_call_id == pending_call.tool_call_id check) to handle the
denial case by breaking with an appropriate status code or flag that allows the
code to return a denied tool result promptly instead of continuing to wait.
- Around line 367-379: The multi_turn(max_turns) method is being called inside
the loop on each iteration, which means the turn counter is reset every time the
stream is recreated after a permission approval. This allows an indefinite
sequence of permission requests to bypass the max_turns limit. Move the turn
limit tracking outside the loop by introducing a mutable variable to track
remaining turns, then on each iteration pass the remaining turn budget to
multi_turn() and decrement the counter accordingly, ensuring the maximum turn
limit is preserved across permission resumes and the method call within the
scope where rig_stream is created uses the updated budget instead of the
original max_turns variable.
In `@rust/cloud-storage/agent/src/convert.rs`:
- Around line 176-182: The PermissionRequest branch in the match statement is
not applying ID normalization that other tool-call-like parts use, which can
cause call/result ID desynchronization on replayed history. Apply the same
replay_item_id normalization to the id parameter and use .with_call_id() on the
ToolCall before pushing it to assistant_parts, consistent with how other
tool-call handling branches process their IDs.
In `@rust/cloud-storage/agent/src/hook.rs`:
- Around line 117-119: The
serde_json::from_str(args).unwrap_or(serde_json::Value::Null) call silently
converts JSON parsing failures to Value::Null, which causes malformed arguments
to be treated as valid empty/default values in the permission flow. Replace the
unwrap_or pattern with explicit error handling that rejects or returns an error
when JSON parsing fails on the args parameter, ensuring malformed tool-call JSON
is properly rejected rather than coerced to null.
In `@rust/cloud-storage/agent/src/permission.rs`:
- Around line 89-94: The PermissionGate struct currently uses an unbounded MPSC
channel (mpsc::UnboundedSender and mpsc::UnboundedReceiver for ToolGrant) which
can accumulate grants indefinitely under heavy traffic, causing memory
exhaustion. Replace the unbounded_channel call that creates the tx and rx fields
with mpsc::channel providing a reasonable bounded capacity, then update all code
paths that send grants through the tx field to use try_send instead of send to
handle the case where the channel is full without blocking, allowing stale or
overflowing grants to be dropped gracefully.
In `@rust/cloud-storage/ai_toolset/Cargo.toml`:
- Line 20: The rmcp dependency addition to ai_toolset violates project policy
which strictly prohibits modifications to this crate. Revert the change to the
Cargo.toml file in ai_toolset by removing the rmcp dependency, or alternatively,
move the rmcp = { workspace = true } dependency wiring to a different crate that
is allowed to be modified per project guidelines.
In `@rust/cloud-storage/ai_toolset/src/tool.rs`:
- Around line 45-53: The `annotations()` method in the Tool trait defaults to
ToolAnnotations::default() which maps to NeedsPermission, causing tools in
non-interactive agent flows (memory, scheduled_action, structured_completion) to
block on grant_rx and timeout. Either modify the annotations() method to detect
non-interactive execution context and return appropriate annotations that bypass
the permission requirement, or add explicit documentation and enforcement
requiring all tools used in non-interactive flows to override annotations() and
explicitly set read_only(true) or other appropriate non-blocking permission
states.
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message/mod.rs`:
- Around line 523-529: The grant sender registration via
ctx.permission_grants.insert(stream_id.clone(), grant_tx) is performed before
stream startup and only cleaned up after persistence, creating stale entries
during startup failures and post-teardown. Move the insertion of grant_tx into
ctx.permission_grants to occur immediately before the stream becomes active
(right before the stream loop/processing begins), and move the corresponding
removal/cleanup to occur immediately after the stream completes (before any
persistence step). Apply the same scoping fix to the related registration code
blocks mentioned in the comment.
In `@rust/cloud-storage/document_cognition_service/src/api/stream/grant.rs`:
- Around line 44-55: The grant_tool function forwards permissions based only on
the client-provided stream_id without verifying that the authenticated user owns
that stream, creating a security vulnerability. Before calling
state.permission_grants.send() with the request.stream_id, add an ownership
check that retrieves the stream entry from the registry to verify the
authenticated user is the owner, and only proceed with the grant if ownership is
confirmed. This requires updating the PermissionGrantRegistry to store owner
identity alongside each stream entry.
In
`@rust/cloud-storage/document_cognition_service/src/api/structured_completion.rs`:
- Around line 115-117: In the structured_completion handler, the grant channel
sender (_grant_tx) is bound from the session call but never explicitly closed.
Since this is a non-interactive handler that cannot process permission grants,
dropping the sender will allow the channel to close properly. Add an explicit
drop call for _grant_tx immediately after the session creation in the
agent_loop.session() call to ensure that if phase 1 requests a permission-gated
tool, the session will not wait indefinitely for grants that cannot be
fulfilled.
In `@rust/cloud-storage/memory/src/domain/service.rs`:
- Around line 177-179: The _grant_tx binding in the agent_loop.session() call is
unused but keeps the grant channel open for the entire non-interactive memory
run. If any tool requests permission, the process will hang indefinitely waiting
on grant_rx since no endpoint owns this sender. Remove the _grant_tx binding
from the destructuring assignment, or explicitly drop it immediately after the
await to ensure the channel is properly closed and the generation can complete
without hanging.
In
`@rust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs`:
- Around line 138-140: The grant sender (_grant_tx) from the
agent_loop.session() call is being kept alive but is unused in automation runs,
causing permission-gated tool calls to stall until idle timeout. Drop the grant
sender immediately after obtaining the session by wrapping it in a drop() call
or explicitly destructuring only the session part and dropping the grant sender
right away, so that permission-gated tool calls fail fast or apply the
automation policy instead of hanging indefinitely.
---
Nitpick comments:
In `@rust/cloud-storage/agent/src/agent_loop.rs`:
- Line 267: The tracing::instrument attribute on the invoke_agent function is
missing the err parameter, which prevents error information from being captured
on the span. Add err to the tracing::instrument macro (after skip_all) so that
when the function returns an error (particularly from send_message), the error
details are properly captured and recorded on the span for better observability
and debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 569364a5-7f31-4f00-80cc-c18bf8bdc983
⛔ Files ignored due to path filters (68)
js/app/packages/service-clients/service-cognition/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/accessLevel.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/aiFeature.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePart.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfEightType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfFiveType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfOnefour.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfOnefourType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfOnetwoType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfOnezeroType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfThreeType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/assistantMessagePartOneOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfFive.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfFiveType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfNineType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfSeven.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfSevenType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfThreeType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentMetadataOneOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/attachmentType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/channelType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatMessageWithAttachments.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreview.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreviewOneOfAllOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreviewOneOfFour.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreviewOneOfFourAllOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreviewOneOfSeven.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatPreviewOneOfSevenAllOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStream.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOfAllOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOfEightType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOfFour.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOfFourType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/chatStreamOneOfSixType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/completionUsage.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/deleteMcpServerParams.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/documentCognitionServiceApiVersion.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/documentReferenceOneOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/documentReferenceOneOfAllOfKind.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/entityType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/featureUsage.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/fileType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/getChatResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/grantToolRequest.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/grantToolResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/mcpAuthCallbackParams.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/model.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/newChatMessage.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/patchChatRequestSharePermission.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/role.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/sharePermissionV2ChannelSharePermissions.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/streamError.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/streamErrorOneOfFiveStreamError.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/streamErrorOneOfStreamError.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/streamErrorOneOfThreeStreamError.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/structuredCompletionRequest.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/toolSetOneOfThreeType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/toolSetOneOfType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/updateOperation.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/updateSharePermissionRequestV2ChannelSharePermissions.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/updateSharePermissionRequestV2PublicAccessLevel.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/userToolResponseValue.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (37)
js/app/packages/core/component/AI/component/message/AssistantMessageParts.tsxjs/app/packages/core/component/AI/component/tool/PermissionRequest.tsxjs/app/packages/service-clients/service-cognition/client.tsjs/app/packages/service-clients/service-cognition/openapi.jsonrust/cloud-storage/agent/Cargo.tomlrust/cloud-storage/agent/src/accumulator.rsrust/cloud-storage/agent/src/agent_loop.rsrust/cloud-storage/agent/src/convert.rsrust/cloud-storage/agent/src/hook.rsrust/cloud-storage/agent/src/hook/test.rsrust/cloud-storage/agent/src/lib.rsrust/cloud-storage/agent/src/permission.rsrust/cloud-storage/agent/src/permission/test.rsrust/cloud-storage/agent/src/stream.rsrust/cloud-storage/agent/src/types.rsrust/cloud-storage/ai_toolset/Cargo.tomlrust/cloud-storage/ai_toolset/src/annotations.rsrust/cloud-storage/ai_toolset/src/lib.rsrust/cloud-storage/ai_toolset/src/tool.rsrust/cloud-storage/ai_toolset/src/toolset/tool_object/object.rsrust/cloud-storage/ai_toolset/src/toolset/tool_object/tool_async.rsrust/cloud-storage/ai_toolset/src/toolset/traits.rsrust/cloud-storage/document_cognition_service/Cargo.tomlrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message/mod.rsrust/cloud-storage/document_cognition_service/src/api/stream/grant.rsrust/cloud-storage/document_cognition_service/src/api/stream/mod.rsrust/cloud-storage/document_cognition_service/src/api/structured_completion.rsrust/cloud-storage/document_cognition_service/src/api/swagger.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/document_cognition_service/src/service/mod.rsrust/cloud-storage/document_cognition_service/src/service/permission_grants.rsrust/cloud-storage/instructions.mdrust/cloud-storage/mcp_client/src/domain/service/toolset.rsrust/cloud-storage/memory/src/domain/service.rsrust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs
💤 Files with no reviewable changes (1)
- rust/cloud-storage/agent/Cargo.toml
Comment on lines
+2
to
+3
| import { cognitionApiServiceClient } from '@service-cognition/client'; | ||
| import { createSignal, Show } from 'solid-js'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct service-client calls from UI components in AI surface
rg -n --type=tsx 'cognitionApiServiceClient\.' js/app/packages/core/component/AIRepository: macro-inc/macro
Length of output: 87
🏁 Script executed:
cat -n js/app/packages/core/component/AI/component/tool/PermissionRequest.tsxRepository: macro-inc/macro
Length of output: 2480
🏁 Script executed:
find js/app/packages/queries -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "grantTool\|cognitionApiServiceClient" {} \;Repository: macro-inc/macro
Length of output: 221
🏁 Script executed:
cat -n js/app/packages/queries/cognition/chat-data.tsRepository: macro-inc/macro
Length of output: 1094
🏁 Script executed:
ls -la js/app/packages/queries/cognition/Repository: macro-inc/macro
Length of output: 289
🏁 Script executed:
rg -n "grantTool" js/app/packages/queries/Repository: macro-inc/macro
Length of output: 41
Create a TanStack Query mutation for grantTool in the queries package and use it here.
This component directly calls cognitionApiServiceClient.grantTool(...) on lines 16-21, which violates the requirement that all network calls must go through TanStack Query. Move this to a mutation in the queries package and call it from the component instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/core/component/AI/component/tool/PermissionRequest.tsx`
around lines 2 - 3, The PermissionRequest component is making direct network
calls to cognitionApiServiceClient.grantTool instead of using TanStack Query
mutations. Create a new TanStack Query mutation in the queries package that
wraps the grantTool API call, then remove the direct cognitionApiServiceClient
import from PermissionRequest and replace the direct grantTool calls (lines
16-21) with an imported call to the new mutation hook. This ensures all network
operations follow the established pattern of going through TanStack Query.
Source: Coding guidelines
Comment on lines
+14
to
+22
| const handleGrant = async () => { | ||
| setGranted(true); | ||
| await cognitionApiServiceClient.grantTool({ | ||
| stream_id: props.stream_id, | ||
| tool_call_id: props.id, | ||
| tool_name: props.name, | ||
| args: props.json, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Grant is marked successful before delivery is actually confirmed.
Line 15 sets granted immediately, and the response (granted: boolean) is ignored. If the request fails or returns granted: false, the UI still shows “Granted”.
Suggested fix
export function PermissionRequest(props: {
id: string;
name: string;
json: unknown;
stream_id: string;
}) {
const [granted, setGranted] = createSignal(false);
const [denied, setDenied] = createSignal(false);
+ const [isSubmitting, setIsSubmitting] = createSignal(false);
const handleGrant = async () => {
- setGranted(true);
- await cognitionApiServiceClient.grantTool({
+ setIsSubmitting(true);
+ const result = await cognitionApiServiceClient.grantTool({
stream_id: props.stream_id,
tool_call_id: props.id,
tool_name: props.name,
args: props.json,
});
+ setIsSubmitting(false);
+ if (result.isOk() && result.value.granted) {
+ setGranted(true);
+ return;
+ }
+ // keep actions visible so user can retry/fallback
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/core/component/AI/component/tool/PermissionRequest.tsx`
around lines 14 - 22, The handleGrant function sets the granted state to true
immediately without waiting for the API response or checking the actual result
from cognitionApiServiceClient.grantTool(). Refactor the function to await the
grantTool call, capture the response object, and only set setGranted to true if
the response.granted field indicates success. This ensures the UI state
accurately reflects whether the permission grant was actually confirmed by the
server.
Comment on lines
+24
to
+26
| const handleDeny = () => { | ||
| setDenied(true); | ||
| }; |
Contributor
There was a problem hiding this comment.
Deny only updates local UI and does not resolve the server-side pending call.
Line 24-Line 26 sets local denied state, but no request is sent to the backend permission flow. In the current architecture, stream resumption is grant-driven; deny needs a backend signal too, otherwise the stream can remain suspended until timeout/teardown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/core/component/AI/component/tool/PermissionRequest.tsx`
around lines 24 - 26, The handleDeny function in PermissionRequest component
only updates the local UI state with setDenied(true) but does not send a backend
signal to resolve the server-side pending call. To fix this, modify the
handleDeny function to not only update the local denied state but also send a
request or signal to the backend that communicates the permission denial. This
ensures the server-side pending call is properly resolved and the stream is
resumed instead of remaining suspended until timeout.
Comment on lines
+367
to
+379
| loop { | ||
| let (bridge, mut hook_rx) = | ||
| StreamBridge::new(annotations_fn.clone(), routing.clone()); | ||
| let pending = bridge.pending.clone(); | ||
|
|
||
| // Drive the rig stream within this scope so the borrow is released | ||
| // before the next loop iteration creates a fresh stream. | ||
| { | ||
| let mut rig_stream = agent | ||
| .stream_prompt(prompt.clone()) | ||
| .with_history(history.clone()) | ||
| .multi_turn(max_turns) | ||
| .with_hook(bridge) |
Contributor
There was a problem hiding this comment.
Preserve the max-turn guard across permission resumes.
multi_turn(max_turns) is recreated after every approved permission-gated tool, so a sequence of permission requests can reset Rig’s turn counter indefinitely as long as grants keep arriving. Track an outer resume count or pass a remaining turn budget into each restart.
One possible guard
let mut prompt = initial_prompt;
let mut history = initial_history;
let mut thinking_buf = String::new();
+ let mut permission_resumes = 0usize;
loop {
@@
let Some(pending_call) = pending_call else {
break; // Normal completion.
};
+ if permission_resumes >= max_turns {
+ yield Err(AgentError::Other(anyhow::anyhow!(
+ "maximum permission-gated tool turns exceeded"
+ )));
+ break;
+ }
+ permission_resumes += 1;
// Wait for a grant for THIS pending call. Channel close means theAlso applies to: 437-455
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/agent/src/agent_loop.rs` around lines 367 - 379, The
multi_turn(max_turns) method is being called inside the loop on each iteration,
which means the turn counter is reset every time the stream is recreated after a
permission approval. This allows an indefinite sequence of permission requests
to bypass the max_turns limit. Move the turn limit tracking outside the loop by
introducing a mutable variable to track remaining turns, then on each iteration
pass the remaining turn budget to multi_turn() and decrement the counter
accordingly, ensuring the maximum turn limit is preserved across permission
resumes and the method call within the scope where rig_stream is created uses
the updated budget instead of the original max_turns variable.
Comment on lines
+443
to
+455
| // Wait for a grant for THIS pending call. Channel close means the | ||
| // stream was dropped (user sent a new message) — end the stream. | ||
| // Drain any stale grants that don't match the pending call id. | ||
| let granted = loop { | ||
| match grant_rx.recv().await { | ||
| Some(g) if g.tool_call_id == pending_call.tool_call_id => break true, | ||
| Some(_) => continue, | ||
| None => break false, | ||
| } | ||
| }; | ||
| if !granted { | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
Add a denial signal to unblock permission waits.
This loop only exits on a matching approval or channel close. In the current grant-only flow, a user denial has no message to send through grant_rx, so the stream remains pending until an outer timeout/drop instead of promptly returning a denied tool result or ending cleanly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/agent/src/agent_loop.rs` around lines 443 - 455, The grant
waiting loop in the agent_loop.rs file only handles approval (matching
tool_call_id), stale grants, and channel closure, but lacks support for user
denial cases which causes the stream to hang until outer timeout/drop. Modify
the grant message structure to include a denial variant (in addition to
approval), then update the match statement within the loop (around the Some(g)
if g.tool_call_id == pending_call.tool_call_id check) to handle the denial case
by breaking with an appropriate status code or flag that allows the code to
return a denied tool result promptly instead of continuing to wait.
Comment on lines
+523
to
+529
| let (mut session, grant_tx) = agent_loop | ||
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | ||
| .await; | ||
|
|
||
| // Register the grant sender so the grant endpoint can send | ||
| // approvals to this running stream. | ||
| ctx.permission_grants.insert(stream_id.clone(), grant_tx); |
Contributor
There was a problem hiding this comment.
Scope grant registration to the actual active stream window.
The sender is registered before stream startup and unregistered only after an awaited persistence step. That leaves stale/late-delivery windows on startup failure and post-stream teardown.
Suggested fix
- // Register the grant sender so the grant endpoint can send
- // approvals to this running stream.
- ctx.permission_grants.insert(stream_id.clone(), grant_tx);
-
// Create the AI stream - yield error if it fails
let mut ai_stream = match session.send_message(rig_messages).await {
Ok(stream) => stream,
Err(e) => {
tracing::error!(error=?e, chat_id = %chat_id, user_id = %user_id, stream_id = %stream_id, "failed to create AI stream");
@@
return;
}
};
+ // Register only after startup succeeds.
+ ctx.permission_grants.insert(stream_id.clone(), grant_tx);
@@
drop(ai_stream);
+ // Clean up registration immediately when streaming work ends.
+ ctx.permission_grants.remove(&stream_id);
@@
- // Clean up the permission grant registration for this stream.
- ctx.permission_grants.remove(&stream_id);
-Also applies to: 532-543, 648-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message/mod.rs`
around lines 523 - 529, The grant sender registration via
ctx.permission_grants.insert(stream_id.clone(), grant_tx) is performed before
stream startup and only cleaned up after persistence, creating stale entries
during startup failures and post-teardown. Move the insertion of grant_tx into
ctx.permission_grants to occur immediately before the stream becomes active
(right before the stream loop/processing begins), and move the corresponding
removal/cleanup to occur immediately after the stream completes (before any
persistence step). Apply the same scoping fix to the related registration code
blocks mentioned in the comment.
Comment on lines
+44
to
+55
| pub async fn grant_tool( | ||
| State(state): State<ApiContext>, | ||
| Json(request): Json<GrantToolRequest>, | ||
| ) -> impl IntoResponse { | ||
| let grant = agent::ToolGrant { | ||
| tool_call_id: request.tool_call_id, | ||
| tool_name: request.tool_name, | ||
| args: request.args, | ||
| }; | ||
|
|
||
| let granted = state.permission_grants.send(&request.stream_id, grant); | ||
|
|
Contributor
There was a problem hiding this comment.
Enforce stream ownership before forwarding grants.
Grant routing is keyed only by client-provided stream_id; without an owner check, any authenticated user who learns another stream ID can approve that stream’s pending tool call.
Suggested direction
-pub async fn grant_tool(
- State(state): State<ApiContext>,
- Json(request): Json<GrantToolRequest>,
-) -> impl IntoResponse {
+pub async fn grant_tool(
+ State(state): State<ApiContext>,
+ Extension(user_context): Extension<UserContext>,
+ Json(request): Json<GrantToolRequest>,
+) -> impl IntoResponse {
@@
- let granted = state.permission_grants.send(&request.stream_id, grant);
+ let granted = state.permission_grants.send_for_user(
+ &request.stream_id,
+ user_context.user_id.as_str(),
+ grant,
+ );(Requires storing owner identity alongside each registered stream entry in PermissionGrantRegistry.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/document_cognition_service/src/api/stream/grant.rs` around
lines 44 - 55, The grant_tool function forwards permissions based only on the
client-provided stream_id without verifying that the authenticated user owns
that stream, creating a security vulnerability. Before calling
state.permission_grants.send() with the request.stream_id, add an ownership
check that retrieves the stream entry from the registry to verify the
authenticated user is the owner, and only proceed with the grant if ownership is
confirmed. This requires updating the PermissionGrantRegistry to store owner
identity alongside each stream entry.
Comment on lines
115
to
117
| let (mut session, _grant_tx) = agent_loop | ||
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Close the grant channel for this non-interactive handler.
structured_completion never registers _grant_tx, but binding it keeps the sender alive. If phase 1 requests a permission-gated tool, the stream can wait forever because no grant can reach this session.
Suggested change
- let (mut session, _grant_tx) = agent_loop
+ let (mut session, grant_tx) = agent_loop
.session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx)
.await;
+ drop(grant_tx);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (mut session, _grant_tx) = agent_loop | |
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | |
| .await; | |
| let (mut session, grant_tx) = agent_loop | |
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | |
| .await; | |
| drop(grant_tx); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rust/cloud-storage/document_cognition_service/src/api/structured_completion.rs`
around lines 115 - 117, In the structured_completion handler, the grant channel
sender (_grant_tx) is bound from the session call but never explicitly closed.
Since this is a non-interactive handler that cannot process permission grants,
dropping the sender will allow the channel to close properly. Add an explicit
drop call for _grant_tx immediately after the session creation in the
agent_loop.session() call to ensure that if phase 1 requests a permission-gated
tool, the session will not wait indefinitely for grants that cannot be
fulfilled.
Comment on lines
177
to
179
| let (mut session, _grant_tx) = agent_loop | ||
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Drop the unused grant sender in memory generation.
Binding _grant_tx keeps the channel open for the whole non-interactive memory run. If a tool needs permission, run_stream waits on grant_rx but no endpoint owns this sender, so generation can hang indefinitely.
Suggested change
- let (mut session, _grant_tx) = agent_loop
+ let (mut session, grant_tx) = agent_loop
.session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx)
.await;
+ drop(grant_tx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/memory/src/domain/service.rs` around lines 177 - 179, The
_grant_tx binding in the agent_loop.session() call is unused but keeps the grant
channel open for the entire non-interactive memory run. If any tool requests
permission, the process will hang indefinitely waiting on grant_rx since no
endpoint owns this sender. Remove the _grant_tx binding from the destructuring
assignment, or explicitly drop it immediately after the await to ensure the
channel is properly closed and the generation can complete without hanging.
Comment on lines
138
to
140
| let (mut session, _grant_tx) = agent_loop | ||
| .session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Don’t keep an unused grant sender in automation runs.
This background flow has no user grant path. Keeping _grant_tx alive makes permission-gated tool calls stall until the idle timeout instead of terminating or applying an explicit automation policy.
Suggested change
- let (mut session, _grant_tx) = agent_loop
+ let (mut session, grant_tx) = agent_loop
.session(toolset, Arc::new(tool_context), &system_prompt, usage_ctx)
.await;
+ drop(grant_tx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs`
around lines 138 - 140, The grant sender (_grant_tx) from the
agent_loop.session() call is being kept alive but is unused in automation runs,
causing permission-gated tool calls to stall until idle timeout. Drop the grant
sender immediately after obtaining the session by wrapping it in a drop() call
or explicitly destructuring only the session part and dropping the grant sender
right away, so that permission-gated tool calls fail fast or apply the
automation policy instead of hanging indefinitely.
ehayes2000
force-pushed
the
ehayes2000/macro-1509-good-tool-permissions
branch
from
fb0f998 to
d3243ed
Compare
ehayes2000
commented
Jun 22, 2026
ehayes2000
left a comment
ehayes2000
left a comment
Contributor
Author
There was a problem hiding this comment.
We got a lot of things wrong here.
The permissions domain modeling is alright but the accept/reject mechanism is totally wrong.
It currently depends on stateful tokio channels which will close when the user disconnects. Permission requests / grants must be stateless.
The agent loop needs to emit tool calls then exit if they require permission. The chat will resume through an http endpoint (see the entry point for chat) but this endpoint will grant or deny permissions.
It is important that if a user ignores the request to accept / reject a tool call, the chat can continue to function. We'll achieve this using a placeholder return for the tool call.
| })()} | ||
| </Match> | ||
| <Match when={part().type === 'permissionRequest'}> | ||
| {(() => { |
Contributor
Author
There was a problem hiding this comment.
the match statement will do the type narrowing in the argument of closure. no need to use an Extract here
Contributor
Author
There was a problem hiding this comment.
The rabbit is right about all of these comments. The UX here is also wrong. A tool call request shouldn't look like a shield it should look like a grayed out version of the tool call with the accept/reject dialog on it.
| system_prompt: &str, | ||
| usage_ctx: UsageContext, | ||
| ) -> Session | ||
| ) -> (Session, mpsc::UnboundedSender<ToolGrant>) |
Contributor
Author
There was a problem hiding this comment.
what the chud nuts is this? why are we returning an unbounded sender. Tool permission grants must be stateless
|
|
||
| /// Placeholder text inserted as the tool result when a tool is denied | ||
| /// or the stream is abandoned while a permission request is pending. | ||
| pub const PENDING_PERMISSION_PLACEHOLDER: &str = r#"{"status":"pending_permission","message":"This tool requires user permission before it can be executed."}"#; |
Contributor
Author
There was a problem hiding this comment.
this is clearly wrong.
- we don't use json literals
- a tool response must be a valid response within the message chain. tool response must include the metadata to ensure this is a valid response. this includes the tool call id.
Comment on lines
+43
to
+81
| pub const PENDING_PERMISSION_PLACEHOLDER: &str = r#"{"status":"pending_permission","message":"This tool requires user permission before it can be executed."}"#; | ||
|
|
||
| /// A tool call that was stopped because it needs permission. | ||
| #[derive(Debug, Clone)] | ||
| pub struct PendingToolCall { | ||
| /// The provider-assigned tool call ID. | ||
| pub tool_call_id: String, | ||
| /// The tool name. | ||
| pub tool_name: String, | ||
| /// The JSON arguments the agent passed. | ||
| pub args: serde_json::Value, | ||
| } | ||
|
|
||
| /// A previously-pending tool call that the user has approved. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ToolGrant { | ||
| /// The provider-assigned tool call ID. | ||
| pub tool_call_id: String, | ||
| /// The tool name to execute. | ||
| pub tool_name: String, | ||
| /// The JSON arguments to pass to the tool. | ||
| pub args: serde_json::Value, | ||
| } | ||
|
|
||
| /// Executes a granted tool out-of-band by name + JSON args, returning the | ||
| /// serialized result (or an error string) as text. | ||
| /// | ||
| /// The agent loop builds one of these per session over the request's toolset | ||
| /// and context. It is invoked from the stream loop after a user grants a | ||
| /// pending permission, since the permission gate terminates rig's loop before | ||
| /// it can run the call itself. | ||
| pub type ToolExecutor = dyn Fn( | ||
| String, | ||
| serde_json::Value, | ||
| ) -> Pin<Box<dyn std::future::Future<Output = Result<String, String>> + Send>> | ||
| + Send | ||
| + Sync; | ||
|
|
||
| /// Coordinates delivery of [`ToolGrant`]s from the grant endpoint to a running |
Contributor
Author
There was a problem hiding this comment.
Obviously wrong. Grants must persist between connects / disconnects like our stream does. A grant must be stateless. Granting permission must come from a stateless http endpoint not a channel.
…acro-1509) Rework the accept/reject mechanism to be stateless per review on PR #4201. The agent loop now emits a permission request and suspends the turn instead of holding a tokio channel; pending state is derived from the conversation chain (a permissionRequest part with no tool result). Resume happens through the chat HTTP entry point (/stream/chat/message) carrying per-call grant/deny decisions: granted calls are executed and denied/ignored calls get a valid placeholder tool result (typed ToolCallErr carrying the tool_call_id) so the provider message chain stays valid. Removes the PermissionGrantRegistry, the /stream/chat/grant endpoint, and all live grant channels/senders. Frontend grant/deny now goes through a TanStack mutation and the pending UI renders as a grayed-out version of the real tool-call view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ehayes2000
force-pushed
the
ehayes2000/macro-1509-good-tool-permissions
branch
from
d3243ed to
930b8fc
Compare
ehayes2000
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks the tool-permission accept/reject mechanism on this PR to be fully stateless: the agent loop suspends a turn by emitting a permission request (no live tokio channel), pending state is derived from the conversation chain, and the chat resumes through the /stream/chat/message entry point carrying per-call grant/deny decisions where granted calls are executed and denied/ignored calls receive a valid placeholder tool result carrying the originating tool_call_id. (macro-1509)