feat(permissions): add typed persistent tool permission rules by greyfreedom · Pull Request #2242 · Hmbown/CodeWhale

greyfreedom · 2026-05-27T02:42:11Z

Summary

Adds an end-to-end typed tool permission system.

This PR supersedes the earlier split PRs for typed execpolicy rules, approval-flow integration, and TUI persistence UI.

What changed

Add typed permission rules in execpolicy
- tool name matching
- shell command matching
- workspace-relative file path glob matching
- allow, deny, and ask decisions
- layered ruleset support with deny precedence and specificity handling
Preserve compatibility with legacy config
- auto_allow is converted into exec_shell allow rules
- auto_deny is converted into exec_shell deny rules
Route tool execution through typed rules
- shell tools
- file read/write/edit tools
- apply_patch
- nested multi_tool_use.parallel calls
Add persistent approval UI support
- approval prompts can produce persistent permission rules
- preview shows the exact rule that will be saved
- persisted rules are deduplicated
Store user-generated permission rules in permissions.toml
- avoids bloating config.toml
- preserves existing comments and unrelated config entries
- config.toml still supports manual [[permissions.rules]]
Harden path and patch handling
- shared permission path normalization
- workspace-relative absolute path handling
- symlink-aware matching
- unified diff path extraction, including timestamps and quoted paths
- project-level config cannot grant allow rules

Closes #1186

Validation

cargo fmt --all
cargo test -p codewhale-execpolicy --all-features
cargo test -p codewhale-tui --all-features typed_permission
cargo test -p codewhale-tui --all-features persistent_permission
cargo test -p codewhale-tui --all-features persist_permission
cargo test -p codewhale-tui --all-features config_loads_sibling_permissions_toml_for_exec_policy_engine
cargo test -p codewhale-tui --all-features project_overlay_applies_only_tightening_permission_rules
cargo clippy -p codewhale-execpolicy -p codewhale-tui --all-targets --all-features -- -D warnings

Greptile Summary

This PR adds a comprehensive end-to-end typed tool permission system: execpolicy gains ToolPermissionRule structs with allow/deny/ask decisions, shell and file tools are routed through these rules before falling back to approval prompts, and user-approved persistent rules are written to a sibling permissions.toml file rather than polluting config.toml. Legacy auto_allow/auto_deny lists are converted into typed exec_shell rules for backwards compatibility.

execpolicy gains ToolPermissionRule, Ruleset::with_rules, and check_tool_permission with deny-always-wins and specificity-based precedence; a DiffHeaderState machine in dispatch.rs and approval.rs correctly ignores ---/+++ lines inside hunk bodies when extracting paths from unified diffs.
Persistence writes deduplicated rules to permissions.toml via toml_edit (preserving comments), sets 0o600 permissions, and adds the in-memory ruleset to the live engine on approval so the new rule takes effect immediately in the current session.
Project-level configs are restricted to tightening rules only — auto_allow and Allow-decision rules from project overlays are silently dropped.

Confidence Score: 5/5

Safe to merge. The permission rule routing, persistence, and deny-wins priority logic are all correct and well-tested. The two flagged issues are narrow edge cases (octal escape values that git never generates) that carry no security or correctness risk on real input.

The DiffHeaderState machine correctly tracks hunk boundaries so diff-body lines can no longer be misread as file headers. Path-traversal rules are properly rejected before persisting. The deny-always-wins and specificity-based priority logic is consistent throughout the engine, config layer, and approval UI. Only two minor issues were found: an out-of-range octal byte value is silently clamped to 0xFF (a code path git never exercises) and a doc comment names the wrong file. Neither affects production behaviour.

The duplicated parse_quoted_diff_path implementation in approval.rs and dispatch.rs each contain the same octal clamping nit — both should be fixed together. All other files look good.

Important Files Changed

Filename	Overview
crates/execpolicy/src/lib.rs	Core permission engine rewrite: adds typed rules, specificity-based priority, deny-always-wins semantics, path normalization, and extensive tests. Well-structured and correct.
crates/tui/src/tui/approval.rs	Persistent rule generation with DiffHeaderState machine (hunk-body false-positives addressed), path-traversal guard, glob filtering, and octal-escape quoted-path parser. Minor octal clamping edge case in parse_quoted_diff_path.
crates/tui/src/core/engine/dispatch.rs	Routes tool calls through typed permission rules with multi-path file tools handled conservatively (all paths must match), parallel tool nesting, and workspace-relative path candidate generation. Same minor octal-escape clamping issue as approval.rs.
crates/tui/src/commands/config.rs	Adds persist_permission_rules using toml_edit to write deduplicated rules to permissions.toml while preserving existing comments; permissions file is created with 0o600 via write_atomic + set_permissions.
crates/config/src/lib.rs	Adds PermissionsToml, loads sibling permissions.toml, filters Allow rules from project overlays, and wires exec_policy_engine into the config API.
crates/tui/src/core/engine/approval.rs	Threads persistent_rules through the approval channel and adds the returned ruleset to the live engine on Approved so rules take effect within the current session.
crates/tui/src/commands/mod.rs	Re-exports persist_permission_rules with a doc comment that names the wrong target file (says config.toml, should be permissions.toml).
crates/tui/src/core/engine/turn_loop.rs	Integrates tool_permission_override_for_call into the per-tool dispatch: Allow clears approval_required, Ask forces it, Deny sets a blocked_error before hydration.

Comments Outside Diff (1)

crates/tui/src/tui/approval.rs, line 4123-4142 (link)

Simplified diff parser misses hunk-content false positives

parse_unified_diff_permission_paths in this file scans every line for --- / +++ prefixes, but has no state-machine to track when the cursor is inside a hunk body. A removed line whose original content starts with -- (e.g., a SQL comment -- SELECT foo) appears in the patch as --- SELECT foo and will be treated as a file header, injecting SELECT foo as a path into the persistent allow rule. A crafted patch can exploit this to silently extend a saved rule to cover paths the user never intended to allow.

dispatch.rs has a DiffHeaderState machine (ExpectOld → ExpectNew → AfterHeader → InHunk) that correctly ignores lines within hunk bodies; this function should use the same logic or share the implementation.
crates/tui/src/tui/approval.rs, line 4144-4154 (link)

Quoted diff path syntax not parsed when generating persistent rules

normalize_diff_permission_path in this file splits on tab then trims, but does not handle Git's quoted path syntax ("a/path with spaces/file.txt"). dispatch.rs handles this via diff_permission_path_token → parse_quoted_diff_path. When apply_patch is called with a patch containing a quoted filename, the approval-side rule generator extracts only up to the first whitespace, producing an incorrect persistent allow rule that will never match the actual path.
crates/tui/src/tui/approval.rs, line 3948-3953 (link)

Shell alias variants save the exact tool name, not the canonical exec_shell

When exec_shell_wait is approved with 's', the generated rule sets tool = "exec_shell_wait". Future calls via the plain exec_shell tool won't match this rule. Consider normalising all shell-variant tool names to "exec_shell" when building the persistent rule so the saved rule covers all invocation styles.
crates/tui/src/tui/approval.rs, line 4011-4017 (link)

permission_decision_label duplicated across two modules

An identical helper is already defined in crates/tui/src/commands/config.rs. Moving it to codewhale_execpolicy and re-exporting would eliminate the duplication.

_{Reviews (5): Last reviewed commit: "fix(tui): remove unused approval constru..." | Re-trigger Greptile}

gemini-code-assist

Code Review

This pull request introduces typed, persistent tool permission rules and an execution policy engine to manage shell and file tool execution approvals. It replaces legacy flat allow/deny lists with structured rulesets across builtin, agent, and user layers, and adds support for saving approved rules to a sibling permissions.toml file. One issue was identified in crates/tui/src/tui/approval.rs where the unified diff path parser is too naive and does not track hunk state, potentially leading to incorrect permission rules if a patch contains diff-like content.

Hmbown · 2026-05-27T13:20:38Z

Independent review (Devin): merge-tested against origin/main (54151a4).

Schema / serde — ToolPermissionRule round-trips cleanly; #[serde(default, skip_serializing_if)] on optional fields is correct. PermissionDecision uses rename_all = snake_case consistently.

Precedence — deny-always-wins across layers is implemented correctly in compare_rule_priority. The specificity formula (tool-len + 1000 + non-wildcard-len) is sound and tested. Minor doc gap: the precedence contract isn't yet stated in docs/TOOL_SURFACE.md.

Persistence — writes go through write_atomic (rename-over), which is correct. The sibling permissions.toml file does not receive 0o600 mode-clamp that config.toml gets on Unix; it could hold allow-rules the user wants kept private. Recommend applying the same chmod in persist_permission_rules.

Security / project-local escalation — typed Allow rules from project-local config are filtered in ConfigToml::merge_project_overrides (line ~425). However, the flat auto_allow list is not filtered — a repo-local config can still inject plain string allow-entries that bypass the typed guard. The existing pre-PR surface had the same gap, but closing the typed path while leaving the flat path open is a regression in the stated security model.

Ask-rule interaction — when AskForApproval::UnlessTrusted matches an Ask rule, the new code correctly promotes it to NeedsApproval. Good. The AskForApproval::OnRequest + Allow branch also correctly still asks.

Test coverage — precedence, word-boundary deny, and legacy-to-typed migration are all covered. The permissions-file chmod gap and the project auto_allow bypass are not tested.

v0.8.48 (fix/1909-windows-logging-alt-screen) — merge was automatic and clean; the only overlap is a trivial cosmetic change in commands/config.rs that auto-resolved. No conflict risk.

greyfreedom · 2026-05-27T14:02:16Z

Independent review (Devin): merge-tested against origin/main (54151a4).

Schema / serde — ToolPermissionRule round-trips cleanly; #[serde(default, skip_serializing_if)] on optional fields is correct. PermissionDecision uses rename_all = snake_case consistently.

Precedence — deny-always-wins across layers is implemented correctly in compare_rule_priority. The specificity formula (tool-len + 1000 + non-wildcard-len) is sound and tested. Minor doc gap: the precedence contract isn't yet stated in docs/TOOL_SURFACE.md.

Persistence — writes go through write_atomic (rename-over), which is correct. The sibling permissions.toml file does not receive 0o600 mode-clamp that config.toml gets on Unix; it could hold allow-rules the user wants kept private. Recommend applying the same chmod in persist_permission_rules.

Security / project-local escalation — typed Allow rules from project-local config are filtered in ConfigToml::merge_project_overrides (line ~425). However, the flat auto_allow list is not filtered — a repo-local config can still inject plain string allow-entries that bypass the typed guard. The existing pre-PR surface had the same gap, but closing the typed path while leaving the flat path open is a regression in the stated security model.

Ask-rule interaction — when AskForApproval::UnlessTrusted matches an Ask rule, the new code correctly promotes it to NeedsApproval. Good. The AskForApproval::OnRequest + Allow branch also correctly still asks.

Test coverage — precedence, word-boundary deny, and legacy-to-typed migration are all covered. The permissions-file chmod gap and the project auto_allow bypass are not tested.

v0.8.48 (fix/1909-windows-logging-alt-screen) — merge was automatic and clean; the only overlap is a trivial cosmetic change in commands/config.rs that auto-resolved. No conflict risk.

Hi @Hmbown , thanks for the detailed review. I addressed the remaining issues:

Documented the typed permission precedence contract in docs/TOOL_SURFACE.md
- deny wins across all layers
- user > agent > builtin defaults
- more specific rules win within the same layer
- ask wins over allow on equal specificity
- project-local allow rules are ignored
Hardened permissions.toml persistence
- permissions.toml is still written atomically
- on Unix, it is now chmod-clamped to 0o600, matching the privacy posture of config.toml
Closed the project-local auto_allow gap
- repo-local auto_allow is explicitly ignored
- repo-local auto_deny is still honored
- typed project-local allow rules remain filtered out
- typed project-local ask / deny rules are still allowed because they only tighten policy
Added regression coverage for:
- permissions.toml owner-only permissions
- project-local auto_allow not being merged
- project-local tightening rules still being preserved

Hmbown · 2026-05-28T06:34:32Z

thank you for this!

Hmbown · 2026-05-31T03:20:47Z

Thanks @greyfreedom — typed persistent permission rules are a strong long-term direction. The shape is valuable because permissions should become explainable state, not just scattered booleans.

I’m leaving the full PR open because approval semantics are high-blast-radius. The safest next slice is the foundation around AskForApproval::Never plus typed “ask” records while preserving today’s allow/deny behavior exactly. That gives us the structure without surprising anyone mid-session.

greyfreedom · 2026-05-31T08:01:14Z

Thanks @greyfreedom — typed persistent permission rules are a strong long-term direction. The shape is valuable because permissions should become explainable state, not just scattered booleans.

I’m leaving the full PR open because approval semantics are high-blast-radius. The safest next slice is the foundation around AskForApproval::Never plus typed “ask” records while preserving today’s allow/deny behavior exactly. That gives us the structure without surprising anyone mid-session.

Thanks, that makes sense. I’ll split out a smaller foundation PR based on current main that only introduces typed ask records around AskForApproval::Never, while keeping the existing allow/deny behavior unchanged. I’ll leave the larger PR as a reference for the full direction.

greyfreedom · 2026-05-31T08:18:29Z

Thanks @greyfreedom — typed persistent permission rules are a strong long-term direction. The shape is valuable because permissions should become explainable state, not just scattered booleans.

I’m leaving the full PR open because approval semantics are high-blast-radius. The safest next slice is the foundation around AskForApproval::Never plus typed “ask” records while preserving today’s allow/deny behavior exactly. That gives us the structure without surprising anyone mid-session.

Hi @Hmbown , thanks for the guidance. I split out the safer foundation slice into a smaller PR here: #2401

The new PR focuses only on typed ask records plus AskForApproval::Never handling, and intentionally preserves the existing allow/deny behavior exactly. It does not include the broader approval-flow integration or TUI persistence from this larger PR.

I’ll keep this larger PR open stale and use the smaller PR as the next review target.

Hmbown · 2026-05-31T08:37:54Z

Quick harvest update: the safer foundation slice from this work landed in #2404. That gives us typed ask records plus AskForApproval::Never handling while keeping current allow/deny behavior unchanged.

I’m still not merging this broader PR as-is because the remaining persistent permission UI/routing changes are the high-blast-radius part. The best next step is to build on #2404 in smaller follow-ups: one focused PR for persistence format/loading, then one for approval-flow/TUI wiring with fixtures around deny/trust/ask precedence. This PR is still a useful reference for that shape.

Hmbown · 2026-05-31T10:29:43Z

Thanks again @greyfreedom. The safer typed-ask foundation from this work landed in #2404 with credit, so I’m going to keep this larger PR as the reference branch rather than merge it directly.

For the next slice, the safest path looks like config/schema-only support for permissions.toml typed ask records, without typed allow/deny and without approval UI persistence yet. That keeps #1186 moving while avoiding a large approval-semantics change in one step.

Hmbown · 2026-05-31T18:15:43Z

Thanks again @greyfreedom. I rechecked #2242 against current main. The full branch now conflicts with several areas that have moved on, and the safest typed-ask foundation from this work already landed in #2404 with credit.

I’m going to keep this PR open as the reference branch rather than merge it directly. The next harvestable slice is ask-only config/schema loading for permissions.toml, without typed allow/deny or approval UI persistence yet. After that, the persistence UI and runtime routing can come through as smaller reviewable follow-ups. The direction is good; it just needs to keep landing in smaller permission-safe pieces.

Hmbown · 2026-06-01T02:38:17Z

Thank you for this. Typed persistent permission rules fit very naturally with the CodeWhale story: users should be able to make deliberate choices once and have the tool remember them transparently.

I am leaving this unmerged in the 0.8.48 pass because the branch is dirty against main, not because the idea is off-track. A clean rebase plus a focused verification pass around migration, doctor/config visibility, and the exact wording of any permission prompts would make this a strong next candidate.

Hmbown · 2026-06-05T04:28:07Z

Thanks @greyfreedom. We have now harvested the v0.9-safe pieces of this broader permissions work without merging the original branch wholesale.

What landed: the typed ToolAskRule foundation from #2404 (8f095b882) and the ask-only sibling permissions.toml schema/loading slice from #2491 (3df018994), including docs and tests for typed ask rules, sibling loading, and rejecting typed allow/deny shapes for now.

What remains intentionally out of this harvest: typed allow/deny records, approval-flow routing through persistent typed rules, TUI persistence UI, runtime writing of approval rules, and full glob semantics. Those are still broader persistent-permissions work, so #1186 stays open. Closing this PR as partially harvested/superseded, with credit preserved for the original design and implementation direction.

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/tui/src/tui/approval.rs

greyfreedom mentioned this pull request May 27, 2026

feat(execpolicy): add typed persistent permission rules #1186

Open

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/execpolicy/src/lib.rs

Hmbown mentioned this pull request May 27, 2026

Add file policy engine for file tool harness #1974

Closed

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/tui/src/tui/approval.rs

greyfreedom added 21 commits May 31, 2026 15:34

feat(execpolicy): add typed permission rules

25eed55

fix(execpolicy): harden path permission matching

488817d

fix(execpolicy): preserve on-request approval semantics

df87ff4

fix(pandoc): validate binary output before resolving binary

aee8a2f

fix(vision): use platform absolute path in boundary test

f431395

test(tui): avoid instant underflow in sidebar test

9d61d39

ci: avoid caching cargo bin shims

b5aaf0b

fix(config): update rebased execpolicy references

0b82725

fix(tui): bound composer history writer batches

a4ea5ce

fix(execpolicy): respect deny command boundaries

a442810

fix(execpolicy): normalize workspace paths for rules

ad403bf

feat(tui): route shell and file tools through typed permission rules

f2dbeb6

feat(tui): persist permission rules from approvals

46e644c

fix(tui): address permission rule review findings

c5af5eb

test(tui): keep comment assertions scoped to permission rules

c375356

fix(tui): adapt persisted permission UI to codewhale crates

67f668e

fix(tui): harden persisted permission rule handling

cd487c7

fix(tui): pass approval input through events

ab0d2fa

fix(tui): persist permission rules to resolved config

164d052

fix(tui): persist approval rules separately

8c30974

fix(execpolicy): share permission path normalization

1534c27

greyfreedom added 6 commits May 31, 2026 15:45

fix(tui): satisfy rebased clippy lints

a42b243

fix(tui): harden approval diff path parsing

f9f49cf

fix(permissions): close project allow and chmod gaps

4570389

fix(tui): silence non-unix permission path warning

391433c

fix(tui): reject traversal permission paths

ef224d0

fix(tui): remove unused approval constructors

4808647

greyfreedom force-pushed the feat/execpolicy-persist-rules-ui branch from 677035e to 4808647 Compare May 31, 2026 07:50

greyfreedom mentioned this pull request May 31, 2026

feat(execpolicy): add typed ask rule foundation #2401

Closed

Hmbown mentioned this pull request May 31, 2026

feat(execpolicy): add typed ask rule foundation #2404

Merged

greyfreedom mentioned this pull request Jun 1, 2026

feat(config): load typed ask permissions.toml schema #2491

Closed

Hmbown closed this Jun 5, 2026

This was referenced Jun 7, 2026

feat(execpolicy): wire ask-only permissions into runtime #2885

Closed

feat(execpolicy): expose matched approval rule metadata #2971

Merged

Conversation

greyfreedom commented May 27, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Validation

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Comments Outside Diff (1)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

greyfreedom commented May 27, 2026

Uh oh!

Uh oh!

Hmbown commented May 28, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

greyfreedom commented May 31, 2026

Uh oh!

greyfreedom commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Hmbown commented Jun 1, 2026

Uh oh!

Hmbown commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greyfreedom commented May 27, 2026 •

edited by greptile-apps Bot

Loading

greyfreedom commented May 31, 2026 •

edited

Loading