Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ramnique
left a comment
There was a problem hiding this comment.
Key issues:
- session-level grant should be written to the run-log and incorporated within the run-state (instead of being stored in memory like it is currently)
- unclear whether command name extraction is using an idiomatic, documented algorithm
- frontend should not be sending command back as part of permission response, this is redundant and instead the backend should extract those from the linked tool-call id
apps/x/packages/shared/src/runs.ts
Outdated
| subflow: true, | ||
| toolCallId: true, | ||
| response: true, | ||
| }).extend({ |
There was a problem hiding this comment.
2 issues
- why extend the shape instead of adding these fields to the original shape?
- why do we need to add
commandhere? The response is already linked to a permission request throughtoolCallId, so the backend would be aware of the original command in the tool-call
| /** In-memory session allowlist — resets on app restart */ | ||
| const sessionAllowSet = new Set<string>(); | ||
|
|
||
| export function addToSessionAllowList(commands: string[]): void { |
There was a problem hiding this comment.
session means a specific run. This implementation assumes session to be the app-session, which is incorrect. Additionally, it is storing the session prefs in memory, which will not survive app-restarts as documented in comments above.
Instead, the agent runtime should simply incorporate session-wide permissions in its state (the state-builder can take care of this). This way, when the state is rebuilt from the run.jsonl file, the session-specific permission grants are restored automatically
|
|
||
| // Handle scope side-effects when approving | ||
| if (rest.response === "approve" && command && scope && scope !== "once") { | ||
| const commandNames = extractCommandNames(command); |
There was a problem hiding this comment.
command name(s) should be extracted from the original tool call, not from the auth response
| ensureSecurityConfigSync(); | ||
| try { | ||
| const stats = fs.statSync(SECURITY_CONFIG_PATH); | ||
| let fileList: string[]; |
There was a problem hiding this comment.
ideally we don't expect any code changes in this part of security.ts since no session-related data needs to be picked up from here.
| * Split a shell command string on command separators (||, &&, ;, |, \n) | ||
| * while respecting single and double quotes. | ||
| */ | ||
| function splitCommandSegments(command: string): string[] { |
There was a problem hiding this comment.
Is this approach following any documented practices for parsing commands? If so, can we include a reference to that? If not, can we use a proven parsing-based solution here?
|
Here's how each review comment was addressed:
Before: Session grants were stored in an in-memory Set in security.ts (sessionAllowSet) that lived After: Session grants are persisted as scope: "session" on the ToolPermissionResponseEvent in the
Before: The frontend extracted the command string from toolCall.arguments.command and sent it After: command is removed from ToolPermissionAuthorizePayload and all frontend handlers. The backend
Before: scope was added via .extend() on the ToolPermissionAuthorizePayload only — it wasn't part of After: scope: z.enum(["once", "session", "always"]).optional() is directly on
Before: security.ts had sessionAllowSet, addToSessionAllowList(), and getSecurityAllowList() merged After: All session logic removed from security.ts. getSecurityAllowList() reverted to only returning
Before: A hand-rolled splitCommandSegments() parser with manual quote tracking and sanitizeToken() After: Replaced with shell-quote's parse(). The new extractCommandNames() iterates parsed tokens, |
ramnique
left a comment
There was a problem hiding this comment.
Functionally this is fine and good to go.
However, left a couple of concerns in comments that you might want to look at.
| // For "always" scope, derive command from the run log and persist to security config | ||
| if (rest.response === "approve" && scope === "always") { | ||
| const repo = container.resolve<IRunsRepo>('runsRepo'); | ||
| const run = await repo.fetch(runId); |
There was a problem hiding this comment.
nit: We were already resolving IRunsRepo on line 51. Can't we just move that up and reuse instead of resolving twice?
shell-quote is a quoting/escaping library, not a shell command parser. Replace with a regex splitter that handles pipes, chains, subshells, and parenthesized groups. Strips () from tokens via sanitizeToken. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Feature: Scoped permission approvals for command execution
When the agent wants to run a blocked command, the user can now choose how broadly to approve it —
once, for the session, or permanently.
UI Layer
permission-request.tsx — Replaced the 4-button layout (Allow Once, Allow for Session, Always Allow,
Deny across two rows) with a split-button pattern:
tool permission requests, it's a plain Approve button with no dropdown.
rounded-l-none with a subtle border divider to form the split-button visual.
Frontend Wiring
App.tsx — handlePermissionResponse callback now accepts two additional optional params: scope ('once' |
'session' | 'always') and command (the raw command string). These are passed through to the
runs:authorizePermission IPC call. The PermissionRequest component gets onApproveSession and
onApproveAlways callbacks that extract the command string from toolCall.arguments.command and call
handlePermissionResponse with the appropriate scope.
chat-sidebar.tsx — Same changes as App.tsx — the onPermissionResponse prop type is extended with scope
and command, and the PermissionRequest component gets the same onApproveSession/onApproveAlways wiring.
This is the sidebar view that renders the same permission cards.
Schema
packages/shared/src/runs.ts — ToolPermissionAuthorizePayload (the Zod schema for the IPC payload) is
extended with:
These are transport-only fields. They're destructured out before the event is created, so they never
get persisted to the run log.
Backend — Permission Handling
packages/core/src/runs/runs.ts — authorizePermission() now:
- Calls extractCommandNames(command) to parse out the individual command names (e.g. git add . && git
commit → ["git", "commit"])
- If scope === "session" → calls addToSessionAllowList(commandNames)
- If scope === "always" → calls addToSecurityConfig(commandNames) (async, writes to disk)
to the run log as before
Backend — Security Config
packages/core/src/config/security.ts — Three additions:
Resets on app restart since it's just in-memory.
sessionAllowSet.
new commands (normalized), writes back sorted JSON, then calls resetSecurityAllowListCache() so the
next getSecurityAllowList() call picks up the new file.
returning. Both the cache-hit path and the error/fallback path handle the merge.
Backend — Command Parsing Fix
packages/core/src/application/lib/command-executor.ts — Two changes:
with splitCommandSegments(), a character-by-character parser that tracks single/double quote state. The
regex would incorrectly split on separators inside quoted strings — e.g. grep "foo || bar" file.txt
would be treated as two commands (grep "foo and bar" file.txt). The new parser correctly keeps that as
one segment.
subshell syntax like (cd foo && make).
parse commands for allow-listing.