feat: rework permission decision policies#26
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44be73568f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import type { PermissionRule, PermissionRuleDecision, PermissionRuleScope } from '../types'; | ||
|
|
||
| const USER_CONFIGURED_SCOPES = new Set<PermissionRuleScope>([ | ||
| 'turn-override', | ||
| 'project', |
There was a problem hiding this comment.
Rule matching now filters to only turn-override, project, and user scopes, so existing session-runtime rules are ignored. That drops previously persisted session approvals on resume/inheritance paths, causing calls that were already approved for-session to prompt again after reload even though the rule data is still present.
Useful? React with 👍 / 👎.
🦋 Changeset detectedLatest commit: 9ad3a9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f0679b9de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const pattern = actionToRulePattern(record.action, record.toolName); | ||
| this.sessionApprovedActions.add(record.action); | ||
| if (pattern === undefined) return; | ||
| if (record.sessionApprovalKey === undefined) return; |
There was a problem hiding this comment.
Do not drop approved-for-session records that were written before sessionApprovalKey existed. recordApprovalResult now ignores session approvals when that field is absent, so resuming an older session (same wire protocol version, no migration path) loses previously granted session approvals and re-prompts users for calls that were already approved in-session.
Useful? React with 👍 / 👎.
| const cwd = this.agent.config.cwd; | ||
| if (cwd.length === 0) return undefined; | ||
| const pathClass = this.agent.runtime.kaos.pathClass(); | ||
| const marker = await this.findGitMarker(cwd); |
There was a problem hiding this comment.
Move git-marker lookup behind a file-access check (and/or cache negative lookups), because this call currently runs for every evaluated tool call in manual/yolo mode. In non-git directories, repeated findGitWorkTreeMarker traversals add avoidable filesystem overhead and increase permission-check latency even for tools that do not touch files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cb74ffced
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let execution: ToolExecution; | ||
| try { | ||
| execution = call.tool.resolveExecution(effectiveArgs); | ||
| execution = await call.tool.resolveExecution(effectiveArgs); |
There was a problem hiding this comment.
resolveExecution is awaited before runAuthorizeToolExecutionHook, but permission policies now host the PreToolUse block and deny logic. That means side effects in resolveExecution can happen before authorization (for example, ExitPlanMode.resolveExecution reads plan data), and calls that fail during resolve never hit pre-tool hooks at all. In environments relying on PreToolUse for strict pre-execution enforcement/auditing, blocked calls are no longer consistently intercepted before tool setup work runs.
Useful? React with 👍 / 👎.
| execution.display ?? { | ||
| kind: 'generic', | ||
| summary: execution.description ?? `Approve ${toolName}`, | ||
| } |
There was a problem hiding this comment.
The generic approval fallback now only sets a summary and drops tool input details, so approval prompts for tools without a custom display no longer show the actual arguments being approved. In manual-mode ask flows (including third-party/custom tools and built-ins lacking a rich display), users can’t review critical parameters before approving, which weakens approval safety compared with the previous behavior that included full input detail.
Useful? React with 👍 / 👎.
| new UserConfiguredAskPermissionPolicy(agent), | ||
| new ExitPlanModeReviewAskPermissionPolicy(agent), |
There was a problem hiding this comment.
Placing UserConfiguredAskPermissionPolicy ahead of ExitPlanModeReviewAskPermissionPolicy lets generic ask rules short-circuit the specialized ExitPlanMode review flow. When that happens, approval metadata like the selected plan option is never attached, so ExitPlanMode cannot enforce or echo the chosen approach and loses the revise/reject-specific handling provided by the plan-review policy. This regression appears whenever users configure an ask rule that matches ExitPlanMode (for example, broad ask patterns).
Useful? React with 👍 / 👎.
| const executionMetadata = authorization?.executionMetadata ?? decision.metadata; | ||
| await dispatchToolCall(step, call, effectiveArgs, displayFields); |
There was a problem hiding this comment.
The new authorizeToolExecution hook shares PrepareToolExecutionResult, but updatedArgs from that hook are ignored and execution always continues with effectiveArgs from the earlier prepare phase. If an integration uses authorize-time rewriting (for example sanitizing or constraining arguments after inspecting resolved execution metadata), the rewrite is silently dropped and the original arguments still run, which can invalidate host safety assumptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67c3d326f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (this.gitMarkerCache.has(cwd)) return this.gitMarkerCache.get(cwd) ?? null; | ||
| const marker = await findGitWorkTreeMarker(this.agent.runtime.kaos, cwd); | ||
| this.gitMarkerCache.set(cwd, marker); |
There was a problem hiding this comment.
Avoid storing null in the git marker cache here: when a session starts in a non-git directory and the user later runs git init in the same cwd, this policy will keep returning the cached miss and never re-check the filesystem, so Write/Edit calls continue to bypass the intended git-cwd auto-approval path and keep prompting. Caching only positive markers (or expiring negative entries) preserves expected behavior after repository initialization.
Useful? React with 👍 / 👎.
| this.rules = [...(options.initialRules ?? [])]; | ||
| this.parent = options.parent; | ||
| this.policies = options.policies ?? createBuiltinPermissionPolicies(); | ||
| this.policies = createPermissionDecisionPolicies(this.agent); |
There was a problem hiding this comment.
Restore support for caller-supplied permission policies in PermissionManagerOptions; this constructor now always installs createPermissionDecisionPolicies(...), so hosts that previously passed custom options.policies lose their enforcement/approval logic at runtime and silently fall back to the built-in queue. That is a behavioral regression for embedded deployments that relied on bespoke policy gates.
Useful? React with 👍 / 👎.
| if (this.gitMarkerCache.has(cwd)) return this.gitMarkerCache.get(cwd) ?? null; | ||
| const marker = await findGitWorkTreeMarker(this.agent.runtime.kaos, cwd); | ||
| this.gitMarkerCache.set(cwd, marker); |
There was a problem hiding this comment.
This policy also caches null git-marker lookups indefinitely, so a session that starts outside a repo and later enters a repo state (for example after git init or after switching to a linked worktree layout) can keep skipping marker-based control-dir detection for the rest of the session. That weakens the git_control_path ask guard for paths that do not literally contain a .git segment and depend on controlDirPath resolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ad3a9bace
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const sessionApprovalRule = | ||
| response.decision === 'approved' && response.scope === 'session' | ||
| ? context.execution.approvalRule | ||
| : undefined; |
There was a problem hiding this comment.
Session-scoped approvals are now cached only from context.execution.approvalRule, and if that field is absent the cache is never written even when the user explicitly chooses “approve for session”. Because approvalRule is optional on tool executions, this causes repeated prompts for tools that do not provide it (notably custom/third-party tools), which is a regression from the previous behavior where session approvals still persisted via coarse action/tool matching.
Useful? React with 👍 / 👎.
| 'edit file': 'Write', | ||
| 'edit file outside of working directory': 'Write', |
There was a problem hiding this comment.
The migration maps the legacy "edit file" action to Write for all records, but v1.1 approval records also include toolName and can legitimately come from Edit. After migration, those Edit approvals become Write-only rules and stop matching future Edit calls, so resumed sessions lose previously granted session approval and re-prompt users unexpectedly.
Useful? React with 👍 / 👎.
|
|
||
| const LEGACY_SESSION_APPROVAL_ACTION_TO_PATTERN: Readonly<Record<string, string>> = { | ||
| 'run command': 'Bash', | ||
| 'run background command': 'BackgroundRun', |
There was a problem hiding this comment.
The v1.2 migration rewrites "run background command" approvals to BackgroundRun, but there is no BackgroundRun tool in this codebase, so migrated session rules can never match any future call. Legacy sessions that previously approved background shell execution for the session will therefore lose that approval after replay and prompt again on every run.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing