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: 3f409a6 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".
| 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".
| '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 👍 / 👎.
…om/MoonshotAI/kimi-code into codex/permission-decision-policies
…om/MoonshotAI/kimi-code into codex/permission-decision-policies
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98d5e79740
ℹ️ 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 (execution.matchesRule !== undefined) { | ||
| return execution.matchesRule(parsed.argPattern) | ||
| ? { rule, strategy: 'matches_rule', hasRuleArgs: true } |
There was a problem hiding this comment.
matchPermissionRule calls execution.matchesRule(parsed.argPattern) without the rule decision, so tool-specific matchers cannot enable conservative case-folding for deny rules. All file tools currently route through matchesAnyPathRuleSubject with conservativeCaseFold defaulting to false, which makes deny path rules case-sensitive on POSIX. On case-insensitive filesystems (for example the default macOS setup), a deny rule like Write(/repo/Secret.txt) can be bypassed by writing /repo/secret.txt, even though both names resolve to the same file.
Useful? React with 👍 / 👎.
| ...message, | ||
| toolCalls, | ||
| ...appendMessageRecord.message, | ||
| toolCalls: appendMessageRecord.message.toolCalls.map(migrateToolCall), |
There was a problem hiding this comment.
migrateV1_0ToV1_1 now unconditionally maps every message.toolCalls entry through migrateToolCall, which dereferences toolCall.function.name. When restoring transcripts with missing/invalid metadata (readVersion === undefined), migrateWireRecords applies all migrations to already-flattened tool calls, so this path can throw at runtime and abort session restore instead of leaving non-legacy entries unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ced3793a3
ℹ️ 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".
| // auto mode → approve (any auto-mode block must be a deny rule above this). | ||
| new AutoModeApprovePermissionPolicy(agent), | ||
| // User-configured ask rule matches → ask. | ||
| new UserConfiguredAskPermissionPolicy(agent), |
There was a problem hiding this comment.
Move session-approval matching ahead of UserConfiguredAskPermissionPolicy; with the current order, any matching ask rule short-circuits first, so even after the user selects “approve for session”, subsequent matching calls still prompt again instead of reusing that session grant. This regresses the expected behavior for configs that intentionally use broad/narrow ask rules but rely on temporary in-session approval to avoid repeated prompts.
Useful? React with 👍 / 👎.
| has_options: args.options !== undefined && args.options.length >= 2, | ||
| }); | ||
|
|
||
| this.agent.telemetry.track('plan_resolved', { outcome: 'auto_approved' }); |
There was a problem hiding this comment.
plan_resolved with outcome: 'auto_approved' is tracked before exitPlanMode() runs. If exiting plan mode fails, telemetry still records a successful resolution even though the tool returns an error and plan mode remains active, which produces incorrect plan-resolution analytics for this failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 801faef7d7
ℹ️ 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".
| for (const patternVariant of pathVariants(pattern, semantics, options.pathOptions)) { | ||
| for (const valueVariant of pathVariants(value, semantics, pathOptions)) { | ||
| for (const patternVariant of pathVariants(pattern, semantics, pathOptions)) { | ||
| if (globMatch(valueVariant, patternVariant, { nocase })) return true; |
There was a problem hiding this comment.
Do not default nocase to true for all path-rule matches here. With this setting, matchesPathRuleSubject treats every path rule as case-insensitive unless callers explicitly override caseInsensitivePaths, so on case-sensitive filesystems (common Linux setups) an allow rule like Write(/repo/Secret.txt) will also match /repo/secret.txt and auto-approve a different file than the user intended. This broadens permission rules and can silently bypass expected manual prompts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bda09e85fa
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d691570b92
ℹ️ 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".
…om/MoonshotAI/kimi-code into codex/permission-decision-policies
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3864ce43a4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51112cf256
ℹ️ 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.
Move permission authorization ahead of resolveExecution, because resolveExecution now runs before runAuthorizeToolExecutionHook. If a custom or future tool performs I/O or other side effects while constructing its execution plan, those effects will happen even when the call is later denied by permission policy. This weakens deny/ask guardrails for any tool whose resolveExecution is not purely declarative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecdc73bb79
ℹ️ 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".
| resolveExecution(args: TaskStopInput): ToolExecution { | ||
| return { | ||
| description: `Stopping task ${args.task_id}`, | ||
| approvalRule: this.name, |
There was a problem hiding this comment.
When a user chooses “approve for session” for TaskStop, this stores the bare TaskStop rule, so SessionApprovalHistoryPermissionPolicy will approve later stops for any other task_id before the fallback/ask path runs. This is inconsistent with the task-id-aware matcher on the next line and can let one approved stop operation authorize unrelated background tasks in the same session; the approval rule should include the current args.task_id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 088969dba5
ℹ️ 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".
| return { | ||
| description: `Invoke skill ${args.skill}`, | ||
| display: { kind: 'skill_call', skill_name: args.skill, args: args.args }, | ||
| approvalRule: this.name, |
There was a problem hiding this comment.
When a user-configured ask rule such as Skill(deploy) prompts and the user chooses “approve for session”, this stores the bare Skill rule. SessionApprovalHistoryPermissionPolicy treats bare rules as tool-name-only matches, so the grant then auto-approves later Skill calls for any other skill name and bypasses their own ask rules. Use a subject-specific approval rule for the current args.skill, matching the matcher added here.
Useful? React with 👍 / 👎.
| // Approve-for-session memorized rule matches → approve. Runs before user-configured ask rules so an in-session grant beats a still-matching ask rule on later calls. | ||
| new SessionApprovalHistoryPermissionPolicy(agent), | ||
| // User-configured ask rule matches → ask. | ||
| new UserConfiguredAskPermissionPolicy(agent), |
There was a problem hiding this comment.
When a configured ask rule matches ExitPlanMode (for example a broad * rule), this policy short-circuits before ExitPlanModeReviewAskPermissionPolicy. The approval still shows the plan_review display, but it uses generic approval handling, so approved option selections are not converted into the “execute only the selected approach” output and revise/reject-specific plan handling is skipped; move the plan-review policy ahead of user ask rules as well.
Useful? React with 👍 / 👎.
| // Approve-for-session memorized rule matches → approve. Runs before user-configured ask rules so an in-session grant beats a still-matching ask rule on later calls. | ||
| new SessionApprovalHistoryPermissionPolicy(agent), | ||
| // User-configured ask rule matches → ask. | ||
| new UserConfiguredAskPermissionPolicy(agent), |
There was a problem hiding this comment.
With /yolo enabled and a configured ask rule such as Bash, this policy runs before YoloModeApprovePermissionPolicy, so ordinary calls still open approval prompts. That regresses the previous mode overlay and the documented yolo behavior of skipping ordinary approvals; keep deny and special file/plan guards, but don't let user ask rules interrupt yolo sessions.
Useful? React with 👍 / 👎.
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f409a6110
ℹ️ 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 access = fileAccesses(context).find((fileAccess) => | ||
| isSensitiveFile(fileAccess.path), | ||
| ); | ||
| if (access === undefined) return; | ||
| return { | ||
| kind: 'ask', |
There was a problem hiding this comment.
For real Read/Write/Edit executions on paths such as .env, the file tools still call resolvePathAccessPath with the default sensitive-file check, so resolveExecution throws PathSecurityError before authorizeToolExecution invokes this new policy. That means the new sensitive-file approval path only works for direct manager tests or fake executions; users still get an immediate sensitive-file error instead of the intended approval prompt unless the file tools resolve sensitive paths without throwing or permission is checked before path resolution.
Useful? React with 👍 / 👎.
| const LEGACY_SESSION_APPROVAL_ACTION_TO_PATTERN: Readonly<Record<string, string>> = { | ||
| 'run command': 'Bash', | ||
| 'stop background task': 'TaskStop', | ||
| 'edit file': 'Write', |
There was a problem hiding this comment.
When restoring a v1.1 approve for session record from an Edit call, the legacy action is edit file, but this migration stores the session rule as Write. Session approval replay now matches the rule against the current tool name, so later Edit calls do not match that restored grant and will prompt again despite the prior session approval; use the recorded toolName (or map legacy Edit records to Edit) instead of collapsing all edit file actions to Write.
Useful? React with 👍 / 👎.
| prompt: args.prompt, | ||
| background: args.run_in_background, | ||
| }, | ||
| approvalRule: this.name, |
There was a problem hiding this comment.
When a user-configured ask rule such as Agent(review-*) prompts and the user chooses approve-for-session, this records the bare Agent approval rule. Because session approval history runs before user ask rules and bare patterns match every Agent call, approving one profile (for example a review agent) will auto-approve later launches of unrelated profiles in the same session; store a rule for the resolved profileName to match the matcher below.
Useful? React with 👍 / 👎.
| return { | ||
| kind: 'ask', | ||
| reason: { | ||
| has_options: display.options !== undefined, | ||
| }, | ||
| resolveApproval: (result) => |
There was a problem hiding this comment.
When the plan-review approval RPC rejects (for example the client disconnects or cannot show the approval surface), this ask result has no resolveError, so the shared approval helper rethrows and the tool batch aborts without a synthetic tool result. The previous plan-specific policy caught that error and returned Plan approval failed: ..., so add a resolveError here to preserve graceful ExitPlanMode failure handling.
Useful? React with 👍 / 👎.
| resolveExecution(args: TaskOutputInput): ToolExecution { | ||
| return { | ||
| description: `Reading output of task ${args.task_id}`, | ||
| approvalRule: this.name, |
There was a problem hiding this comment.
When a configured ask rule like TaskOutput(task_1) prompts and the user chooses approve-for-session, this stores the bare TaskOutput rule even though the matcher below is task-id aware. Since session approval history runs before user ask rules, that one approval will auto-approve output reads for every other background task in the session; include the current args.task_id in the approval rule.
Useful? React with 👍 / 👎.
2 participants
Summary
Testing