Add Agent permission approval flow - #826
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds agent permission modes and approval-gated tool execution. It persists permission preferences, creates approval sessions, transports approval requests, blocks terminal and file actions until decisions, and adds approval prompts, status indicators, token refresh, cleanup, and validation. ChangesAgent approval and permission flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Chat
participant TriggerAgentLong
participant ApprovalSession
participant Tool
User->>Chat: select ask_approval mode
Chat->>TriggerAgentLong: start agent run with approval session
TriggerAgentLong->>Tool: request approval-gated action
Tool->>TriggerAgentLong: requestToolApproval(...)
TriggerAgentLong->>ApprovalSession: wait for approval input
ApprovalSession-->>Chat: stream approval request
User->>Chat: approve or deny
Chat->>ApprovalSession: submit agent-tool-approval input
ApprovalSession-->>TriggerAgentLong: approval decision
TriggerAgentLong-->>Tool: continue or return denial
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/components/AgentPermissionSelector.tsx (1)
75-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing selection state to assistive tech.
The option buttons rely solely on a visual checkmark to indicate the selected mode; screen reader users get no indication of which option is currently selected.
♿ Optional a11y improvement
- {options.map((option) => { + {options.map((option) => { const OptionIcon = option.icon; const selected = option.id === agentPermissionMode; return ( <button key={option.id} type="button" + role="option" + aria-selected={selected} onClick={() => {🤖 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 `@app/components/AgentPermissionSelector.tsx` around lines 75 - 101, The option buttons in AgentPermissionSelector rely only on the visual checkmark to show the current mode, so expose the selected state to assistive tech. Update the button in the options map to reflect selection semantically, such as by adding the appropriate pressed/selected state and ensuring the control communicates the active option alongside the existing Check icon. Keep the change localized to the option rendering logic that uses option.id, selected, and setAgentPermissionMode.lib/api/__tests__/agent-long-contracts.test.ts (2)
472-474: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the approval-token assertion to scope it inside the response object.
The check at Line 474 only verifies
approvalSessionPublicAccessTokenappears somewhere inresumeSrc, decoupled from theNextResponse.json({...})match at Lines 472-473. This weakens the contract test versus the prior single regex — a stray reference toapprovalSessionPublicAccessTokenanywhere else in the file (e.g. in a comment or unrelated object) would still pass, without verifying it's actually part of the JSON response payload asserted by this test.🔧 Suggested tightening
- expect(resumeSrc).toMatch( - /NextResponse\.json\(\{[\s\S]*runId,[\s\S]*publicAccessToken,[\s\S]*chatId,/, - ); - expect(resumeSrc).toMatch(/approvalSessionPublicAccessToken/); + expect(resumeSrc).toMatch( + /NextResponse\.json\(\{[\s\S]*runId,[\s\S]*publicAccessToken,[\s\S]*chatId,[\s\S]*approvalSessionPublicAccessToken/, + );🤖 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 `@lib/api/__tests__/agent-long-contracts.test.ts` around lines 472 - 474, The contract test in the agent-long response assertion is too loose because `approvalSessionPublicAccessToken` is checked separately from the `NextResponse.json(...)` payload match. Update the test around the `resumeSrc` expectation so the token is asserted inside the same regex that matches the response object, using the existing `NextResponse.json`/`resumeSrc` check as the single source of truth. Keep the assertion scoped to the JSON payload rather than any standalone occurrence in the file.
559-560: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSame scoping concern as the resume-route assertion.
This assertion, like Lines 472-474, only checks
runId/publicAccessToken/chatIdare present in the sameNextResponse.jsoncall but doesn't tieapprovalSessionPublicAccessTokento it for the start route response. If that field is expected here too, consider extending this regex similarly for consistency.🤖 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 `@lib/api/__tests__/agent-long-contracts.test.ts` around lines 559 - 560, The start-route response assertion in the test is too loose because it only scopes `runId`, `publicAccessToken`, and `chatId` to the same `NextResponse.json` call; update the regex in `agent-long-contracts.test.ts` near the existing start-route expectation so it also requires `approvalSessionPublicAccessToken` in the same response, matching the scoping used by the resume-route assertion and keeping the coverage consistent.app/api/agent-long/route.ts (1)
66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSessions is GA in
@trigger.dev/sdk@4.5.0, so thetriggerSdk as unknown as { sessions?: ... }feature-detection cast and the runtime "unavailable" throw can likely be replaced with a direct typed import. Optional cleanup; the current defensive approach is functionally fine.🤖 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 `@app/api/agent-long/route.ts` around lines 66 - 76, The sessions API is now GA, so remove the feature-detection cast around triggerSdk.sessions in the route handler and switch to using the SDK’s direct typed sessions import/API instead. Update the TriggerSessionsApi/triggerSessions usage in this file so the code no longer relies on "as unknown as" or an optional sessions property, and delete the runtime "unavailable" fallback throw if it becomes unnecessary.lib/ai/tools/index.ts (1)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
createToolsnow has 18 positional parameters.Pre-existing debt, but this PR adds one more. Consider migrating to a single options object in a future refactor to reduce call-site error risk (easy to swap adjacent same-typed params).
🤖 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 `@lib/ai/tools/index.ts` at line 64, createTools is now taking too many positional parameters, increasing the risk of call-site mistakes in adjacent same-typed arguments. Update the createTools API in index.ts to accept a single options object instead of the long positional list, and adjust the function signature and all internal references accordingly so future call sites can pass named fields safely.
🤖 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 `@app/api/agent-long/cancel/route.ts`:
- Around line 47-52: The cancel flow in setActiveTriggerRun only clears the
stored approvalSessionId pointer, but it does not close the underlying Trigger
session. Update the cancel handler in the route that processes the run
cancellation to also close the approval session (or enqueue the same cleanup)
alongside clearing triggerRunId and approvalSessionId, using the existing
chatId/runId context so abandoned sessions are not left open.
In `@app/api/agent-long/route.ts`:
- Around line 295-338: The approval-session branch in the agent-long route is
not forwarding the trigger region, so the `sessions.start` path behaves
differently from `tasks.trigger`. Update the `triggerConfig` passed to
`triggerSessions.start` in the `approvalSessionId` flow to include `region`
alongside `basePayload` and `tags`, using the existing `triggerRegion` value so
ask_approval preserves routing.
In `@app/contexts/AgentApprovalContext.tsx`:
- Around line 63-68: The sendTriggerSessionInput call in AgentApprovalContext is
missing an abort/timeout, which can leave ToolApprovalControls stuck in the
sending state indefinitely. Update the approval submission flow around
sendTriggerSessionInput to use a timeout or abort signal, and ensure the
existing session.sessionId, session.publicAccessToken, and approvalId-based
request path still work while failing cleanly if the request hangs.
In `@lib/chat/trigger-browser-realtime.ts`:
- Around line 253-279: `sendTriggerSessionInput` can hang forever because it
only respects a caller-provided signal, and
`AgentApprovalProvider.sendToolApproval` does not pass one. Add an internal
default timeout/abort path inside `sendTriggerSessionInput` (or in the approval
call site) so the POST is automatically canceled after a reasonable duration,
and make sure the existing fetch call uses that timeout signal when no external
signal is supplied.
In `@trigger/agent-long.ts`:
- Around line 217-234: The approval wait in buildAgentToolApprovalRequester
still blocks on session.in.wait without being tied to the AbortSignal, so
approvals can continue after the run is stopped. Update the waiting logic in
buildAgentToolApprovalRequester (and the related approval handling block around
the aborted cleanup path) to race the wait against signal.aborted or otherwise
short-circuit immediately when the signal fires, following the same pattern used
elsewhere in the agent-long transport flow. Make sure the blocked approval
promise is cancelled/rejected on abort so the cleanup branch becomes reachable.
- Around line 279-300: requestToolApproval currently shares one
approvalSessionId across concurrent tool calls, so a decision for one call can
be consumed and discarded by another waiter. Fix this in trigger/agent-long.ts
by serializing approval handling per tool call or giving each invocation its own
isolated session listener in requestToolApproval/open session flow, and ensure
the approvalId/toolCallId matching logic cannot cause a valid approval to be
lost before the intended waiter receives it.
---
Nitpick comments:
In `@app/api/agent-long/route.ts`:
- Around line 66-76: The sessions API is now GA, so remove the feature-detection
cast around triggerSdk.sessions in the route handler and switch to using the
SDK’s direct typed sessions import/API instead. Update the
TriggerSessionsApi/triggerSessions usage in this file so the code no longer
relies on "as unknown as" or an optional sessions property, and delete the
runtime "unavailable" fallback throw if it becomes unnecessary.
In `@app/components/AgentPermissionSelector.tsx`:
- Around line 75-101: The option buttons in AgentPermissionSelector rely only on
the visual checkmark to show the current mode, so expose the selected state to
assistive tech. Update the button in the options map to reflect selection
semantically, such as by adding the appropriate pressed/selected state and
ensuring the control communicates the active option alongside the existing Check
icon. Keep the change localized to the option rendering logic that uses
option.id, selected, and setAgentPermissionMode.
In `@lib/ai/tools/index.ts`:
- Line 64: createTools is now taking too many positional parameters, increasing
the risk of call-site mistakes in adjacent same-typed arguments. Update the
createTools API in index.ts to accept a single options object instead of the
long positional list, and adjust the function signature and all internal
references accordingly so future call sites can pass named fields safely.
In `@lib/api/__tests__/agent-long-contracts.test.ts`:
- Around line 472-474: The contract test in the agent-long response assertion is
too loose because `approvalSessionPublicAccessToken` is checked separately from
the `NextResponse.json(...)` payload match. Update the test around the
`resumeSrc` expectation so the token is asserted inside the same regex that
matches the response object, using the existing `NextResponse.json`/`resumeSrc`
check as the single source of truth. Keep the assertion scoped to the JSON
payload rather than any standalone occurrence in the file.
- Around line 559-560: The start-route response assertion in the test is too
loose because it only scopes `runId`, `publicAccessToken`, and `chatId` to the
same `NextResponse.json` call; update the regex in
`agent-long-contracts.test.ts` near the existing start-route expectation so it
also requires `approvalSessionPublicAccessToken` in the same response, matching
the scoping used by the resume-route assertion and keeping the coverage
consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc6cdf14-4094-468e-b09a-fe3e3d364054
📒 Files selected for processing (33)
app/api/agent-long/cancel/route.tsapp/api/agent-long/resume/route.tsapp/api/agent-long/route.tsapp/components/AgentPermissionSelector.tsxapp/components/AgentsTab.tsxapp/components/ChatInput/ChatInputToolbar.tsxapp/components/ChatInput/__tests__/ChatInputToolbar.test.tsxapp/components/MessagePartHandler.tsxapp/components/chat.tsxapp/components/tools/FileHandler.tsxapp/components/tools/TerminalToolHandler.tsxapp/components/tools/ToolApprovalControls.tsxapp/contexts/AgentApprovalContext.tsxapp/contexts/GlobalState.tsxapp/hooks/__tests__/useAutoContinue.test.tsapp/hooks/useAutoContinue.tsapp/hooks/useChatHandlers.tsapp/layout.tsxconvex/chats.tsconvex/schema.tslib/ai/tools/file.tslib/ai/tools/index.tslib/ai/tools/interact-terminal-session.tslib/ai/tools/run-terminal-cmd.tslib/api/__tests__/agent-long-contracts.test.tslib/chat/agent-long-transport.tslib/chat/trigger-browser-realtime.tslib/db/actions.tslib/utils/accumulate-ui-chunks.tslib/utils/client-storage.tstrigger/agent-long.tstypes/agent.tstypes/chat.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/agent-long/cancel/route.ts (1)
43-52: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClose the approval session before the early return.
runIdcan be missing whilechat.active_agent_approval_session_idis still set, so this branch skips bothtriggerSessions.close(...)and theapprovalSessionId: nullupdate. That leaves the Trigger session open and the chat record pointing at a stale session. Move the cleanup ahead of the!runIdreturn, or run the same cleanup in that branch too.🤖 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 `@app/api/agent-long/cancel/route.ts` around lines 43 - 52, The early return in the cancel route leaves `chat.active_agent_approval_session_id` uncleared and skips `triggerSessions.close(...)` when `getActiveTriggerRun` returns no `runId`. Update the cancel flow in the route handler so the approval-session cleanup runs before returning, or duplicate the same cleanup in the `!runId` branch, using `approvalSessionId`, `triggerSessions.close`, and the `approvalSessionId: null` update to ensure the stored session is always closed and cleared.
🧹 Nitpick comments (2)
app/api/agent-long/cancel/route.ts (2)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dual import from the same module.
triggerSdk(namespace) andruns(named) are both imported from@trigger.dev/sdk;runs.cancelcould just betriggerSdk.runs.cancelto avoid two import statements for one module.🤖 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 `@app/api/agent-long/cancel/route.ts` around lines 2 - 3, There is a redundant dual import from `@trigger.dev/sdk` in the cancel route. Remove the separate named import of runs and use the existing triggerSdk namespace in the route handler, updating any runs.cancel usage to triggerSdk.runs.cancel so the module is only imported once.
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
sessionsdirectly and share the typed helper
trigger/agent-long.ts,app/api/agent-long/route.ts, andapp/api/agent-long/cancel/route.tsall repeat the sametriggerSdk as unknown as { sessions?: ... }shim. Importsessionsdirectly, or centralize a typed helper, so the close call stays typed and the SDK shape can’t drift silently.🤖 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 `@app/api/agent-long/cancel/route.ts` around lines 15 - 21, The session client typing is duplicated across the agent-long route handlers, which makes the SDK access brittle and prone to drifting. Replace the repeated `triggerSdk as unknown as { sessions?: ... }` shim in `app/api/agent-long/cancel/route.ts` and the related `trigger/agent-long.ts` and `app/api/agent-long/route.ts` usage with a shared typed helper or a direct `sessions` import, and update the `close` call to use that centralized typed reference so the SDK shape is enforced in one place.
🤖 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.
Outside diff comments:
In `@app/api/agent-long/cancel/route.ts`:
- Around line 43-52: The early return in the cancel route leaves
`chat.active_agent_approval_session_id` uncleared and skips
`triggerSessions.close(...)` when `getActiveTriggerRun` returns no `runId`.
Update the cancel flow in the route handler so the approval-session cleanup runs
before returning, or duplicate the same cleanup in the `!runId` branch, using
`approvalSessionId`, `triggerSessions.close`, and the `approvalSessionId: null`
update to ensure the stored session is always closed and cleared.
---
Nitpick comments:
In `@app/api/agent-long/cancel/route.ts`:
- Around line 2-3: There is a redundant dual import from `@trigger.dev/sdk` in the
cancel route. Remove the separate named import of runs and use the existing
triggerSdk namespace in the route handler, updating any runs.cancel usage to
triggerSdk.runs.cancel so the module is only imported once.
- Around line 15-21: The session client typing is duplicated across the
agent-long route handlers, which makes the SDK access brittle and prone to
drifting. Replace the repeated `triggerSdk as unknown as { sessions?: ... }`
shim in `app/api/agent-long/cancel/route.ts` and the related
`trigger/agent-long.ts` and `app/api/agent-long/route.ts` usage with a shared
typed helper or a direct `sessions` import, and update the `close` call to use
that centralized typed reference so the SDK shape is enforced in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c606347e-2aca-446a-abc1-a7bc813290f1
📒 Files selected for processing (6)
app/api/agent-long/cancel/route.tsapp/api/agent-long/route.tsapp/components/AgentPermissionSelector.tsxlib/api/__tests__/agent-long-contracts.test.tslib/chat/trigger-browser-realtime.tstrigger/agent-long.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/chat/trigger-browser-realtime.ts
- app/api/agent-long/route.ts
- app/components/AgentPermissionSelector.tsx
- lib/api/tests/agent-long-contracts.test.ts
- trigger/agent-long.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/api/agent-trigger-route.ts`:
- Around line 405-446: The approval-session branch in agent-trigger-route.ts is
generating a fresh externalId with randomUUID(), which breaks deduplication
across retries. Update the sessions.start call in the approvalSessionId path to
use the same deterministic turn key used elsewhere in this flow, so the
externalId remains stable and retries reuse the existing session/run. Keep the
change localized around sessions.start and the approvalSessionPublicAccessToken
handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dd31572-59bc-4062-8038-95690bb96ef8
📒 Files selected for processing (11)
app/components/chat.tsxapp/hooks/useChatHandlers.tsconvex/chats.tsconvex/schema.tslib/api/__tests__/agent-long-contracts.test.tslib/api/agent-cancel-route.tslib/api/agent-resume-route.tslib/api/agent-trigger-route.tslib/chat/agent-long-transport.tslib/db/actions.tstrigger/agent-long.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- convex/schema.ts
- lib/chat/agent-long-transport.ts
- trigger/agent-long.ts
- app/hooks/useChatHandlers.ts
- convex/chats.ts
…rmission-selector # Conflicts: # lib/api/agent-trigger-route.ts # trigger/agent-long.ts
… into codex/pr-826-agent-permission-selector # Conflicts: # app/hooks/__tests__/useChatHandlers.regenerate-model.test.tsx
This was referenced Jul 14, 2026
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.
Summary
Behavior
Yesapproves one operationYes, and don't ask againstores the server-derived exact command or file target on that chat; the grant has no time expiry and is capped to the 100 most recent entriesTesting
pnpm typecheckgit diff --checkVisual verification
How should HackerAI actions be approved?printf approval-persistence-testfor the chat, reloaded, and repeated the request; it executed with no second approval promptprintf sidebar-latest-testin the computer sidebar, regenerated its response, and confirmed the sidebar immediately fell back to the previousprintf approval-persistence-testoutputManual verification
Summary by CodeRabbit