Skip to content

feat(tui): persist exact repo-scoped allow grants#4863

Merged
Hmbown merged 5 commits into
mainfrom
codex/harvest-grants-v092
Jul 26, 2026
Merged

feat(tui): persist exact repo-scoped allow grants#4863
Hmbown merged 5 commits into
mainfrom
codex/harvest-grants-v092

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 26, 2026

Copy link
Copy Markdown
Owner

No-Issue: partial slice — #1186 and #2242 stay open by design (see Scope boundary below).

Summary

Harvested from PR #4761 by @greyfreedom, rebased onto current origin/main.

  • Lets approval cards remember eligible safe shell/file-write approvals.
  • Persists complete-command or exact workspace-relative file matches, scoped to one repository.
  • Preserves deny > ask > allow, safety floors, dangerous-command exclusions, and repo law.
  • Carries the contributor's Windows/PTY and portable path-scope fixes.

@greyfreedom's authorship is preserved on all four substantive commits; only the rebase-repair commit is mine.

Scope boundary — why this is No-Issue and not Closes #1186

The contributor scoped this as a partial slice and that is still accurate. Not included, so both issues stay open:

  • rule editing or removal,
  • glob or directory grants,
  • deny persistence.

Marking it Closes would close two issues that are only part-done, which is exactly the dishonesty the link gate exists to prevent.

Rebase repairs

The branch was 93 commits behind main and had four red checks that were really two problems:

  • Lint / Test (macos) / Test (windows) were one compile error: select_prev still passed &self.request.tool_name while this branch changes order_for to take &ApprovalRequest. Now matches select_next.
  • Version drift was a stale crates/tui/CHANGELOG.md slice, not a version mismatch. Resynced via scripts/sync-changelog.sh.

Verification (macOS, on the rebased head)

  • cargo clippy --workspace --all-features --locked with CI's exact allow set — clean.
  • codewhale-tui — 8340 passed / 0 failed.
  • codewhale-execpolicy — 89 unit + 1 doctest passed / 0 failed.
  • qa_pty — 25 passed / 0 failed, 1 ignored.
  • check-versions.sh — workspace 0.9.1, npm 0.9.1, lockfile in sync.
  • check-tui-locale-parity.py PASS; jq empty crates/tui/locales/*.json clean.
  • cargo fmt --all -- --check, git diff --check, check-coauthor-trailers.py --check-authors — clean.

Copilot AI review requested due to automatic review settings July 26, 2026 06:35
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 3m 0s —— View job


Reviewing PR #4863

  • Gather context (PR body, diff, repo instructions)
  • Read changed files (execpolicy, config, tui approval/engine)
  • Check correctness (panics, unwraps, exhaustive matches)
  • Check provider/model routing safety invariant
  • Check reuse/simplification and duplicated logic
  • Check test coverage
  • Check security (secret handling, exec policy)
  • Post final review

Summary

This is a well-scoped, carefully-guarded feature: approval cards can now save an exact, repo-scoped allow grant (command_exact + workspace) alongside the existing ask-rule save. I read the full diff (git diff origin/main...HEAD) with emphasis on crates/execpolicy/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/core/engine.rs/turn_loop.rs, and crates/tui/src/tui/approval.rs.

No provider/model routing code is touched — item 2 of the review checklist doesn't apply to this PR.

Correctness — looks solid

  • deny > ask > allow ordering is preserved end-to-end: the Allow decision in turn_loop.rs (crates/tui/src/core/engine/turn_loop.rs:1922-1931) only clears approval_required when !hook_requires_approval && !approval_force_prompt, and the auto-review/repo-law floors that run afterward can still force a prompt or hard-block. Good, matches the PR's stated invariant.
  • ExecPolicyEngine::matching_ask_rule correctly gates workspace scoping before command/path matching (crates/execpolicy/src/lib.rs:389-393), and command_exact short-circuits arity-aware matching so cargo test --workspace doesn't inherit a cargo test grant (verified by exact_workspace_allow_matches_only_the_same_command_and_repo).
  • file_tool_ask_rule_decision's aggregation over multiple apply_patch paths (crates/tui/src/core/engine.rs:4691-4719) is conservatively correct: any Block short-circuits, any Prompt wins over Allow, and a mix of Allow/unmatched (None) correctly falls through to the default approval flow rather than silently granting Allow — covered by apply_patch_allow_requires_every_touched_path_to_match.
  • ConfigStore::append_allow_rules (crates/config/src/lib.rs:3975-4013) validates workspace scoping, exact-match requirement, and non-empty command/path before writing, so a UI bug can't smuggle an unscoped or prefix-based allow rule into permissions.toml. Good defense-in-depth given append_permission_rules is otherwise a thin dispatcher.
  • persist_rules_from_approval (crates/tui/src/tui/ui.rs:14276-14281) infers the action to persist from rules.first().map(|r| r.action). This is safe today because persistent_ask_rules and persistent_allow_rules are always built homogeneously (all Ask or all Allow — see build_persistent_ask_rules / build_persistent_allow_rules), but it's an implicit invariant rather than a type-enforced one; worth a one-line comment if this file is touched again.
  • Match arms on the new ToolAskRuleDecision::Allow variant are exhaustive everywhere it's consumed (engine.rs:1292, engine.rs:4709, turn_loop.rs:1922) — no risk of non-exhaustive match breaking compilation.

Design note (not a bug)

  • workspace_scope_matches compares normalize_workspace_scope(rule.workspace) against normalize_workspace_scope(ctx.cwd) for exact equality (crates/execpolicy/src/lib.rs:846-854). This only works because ctx.cwd is always self.session.workspace — a value that's stable per session and only changes on session restore/switch (engine.rs:2175), not on shell cd. Confirmed this is consistent between the save path (ApprovalRequest::new's workspace: &Path param) and the check path (tool_ask_rule_decision_for_context's cwd). Fine as implemented, but it does mean a grant is scoped to "the session's workspace root at approval time," not to "the git repository" in general — if a user ever opens the same repo from a different subdirectory as the workspace root, saved grants won't carry over. Given the PR explicitly calls this "repo-scoped," this is worth a mental note but not a blocking issue.

Reuse / duplication

  • PermissionRuleSavePreview/build_permission_rule_save_preview/format_permission_rule_save_entry cleanly generalize the old ask-only preview machinery rather than duplicating it for allow rules. Good.
  • Minor: permission_rule_table and permission_rule_inline_table (crates/config/src/lib.rs) duplicate the same field-by-field construction for TOML table vs. inline-table — pre-existing pattern, not introduced by this PR, so not a blocker.

Tests

  • Coverage is thorough: execpolicy unit tests for workspace-scope normalization (POSIX case sensitivity, Windows drive/case folding, traversal rejection), config-store round-trip + rejection tests for malformed allow rules, engine-level tests for exec/file/apply_patch allow aggregation, and TUI approval-card tests for the new [p] option, its keybinding, and its interaction with critical/repo-law requests. I did not find gaps.

Housekeeping nit

  • CHANGELOG.md: the diff drops the blank line that used to separate the last ## [Unreleased] bullet from the ## [0.9.1] header (line ~100), so consecutive release sections are no longer visually separated. Cosmetic only.

Verification

I was not able to execute cargo test/cargo fmt/cargo check in this environment (shell commands require interactive approval I don't have here), so I relied on static review of the diff plus the test files added in the same commits. The PR body's reported verification (89 unit + 1 doctest passing, locale-parity script, cargo fmt --check, coauthor-trailer check) is consistent with what the diff itself shows (new tests added alongside every new branch of logic, all locale files updated in lockstep, Harvested from PR #4761 by @greyfreedom credit preserved in the body).

Nothing here blocks merge from a correctness/safety standpoint.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

greyfreedom and others added 5 commits July 26, 2026 11:06
Let approval cards remember eligible safe shell and file-write approvals as exact allow rules bound to one repository, without weakening deny, ask, or safety-floor precedence.

Refs #1186 and #2242.

Harvested from PR #4761 by @greyfreedom

Co-authored-by: greyfreedom <11493871+greyfreedom@users.noreply.github.com>
Update PTY approval expectations and keep Unix-only process support out of Windows warning builds.

Harvested from PR #4761 by @greyfreedom

Co-authored-by: greyfreedom <11493871+greyfreedom@users.noreply.github.com>
Assert persisted workspace scopes without assuming platform-specific leading separators.

Harvested from PR #4761 by @greyfreedom

Co-authored-by: greyfreedom <11493871+greyfreedom@users.noreply.github.com>
Run the POSIX case-distinction regression only on Linux; macOS and Windows intentionally fold paths in the production matcher.

Harvested from PR #4761 by @greyfreedom

Co-authored-by: greyfreedom <11493871+greyfreedom@users.noreply.github.com>
Rebasing onto current main left select_prev passing the tool name where
order_for now takes the whole request, so the branch did not compile. Match
select_next, which already passes the request.

Also resync crates/tui/CHANGELOG.md from the root slice; the stale copy was
what the Version drift gate was actually failing on, not a version mismatch.

Receipts: CI's exact clippy invocation clean; codewhale-tui 8340/8340;
codewhale-execpolicy 89 unit + 1 doctest; qa_pty 25/25; locale parity PASS
and jq empty clean; fmt and diff clean.
@Hmbown

Hmbown commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto current origin/main (5629f3429) — the branch was 93 commits behind — and fixed the two things the red checks were actually about.

Lint / Test (macos, windows) were all the same single compile error, not four independent failures: select_prev still passed &self.request.tool_name while this branch changes order_for to take &ApprovalRequest. select_next already passed the request; now both match.

Version drift was not a version mismatch — crates/tui/CHANGELOG.md was out of date with the root slice. Resynced via scripts/sync-changelog.sh; check-versions.sh now reports workspace 0.9.1, npm 0.9.1, lockfile in sync.

@greyfreedom's authorship is preserved on all four substantive commits; only the repair commit is mine.

Receipts on the rebased head:

  • cargo clippy with CI's exact invocation (its five -A allows) — clean.
  • codewhale-tui — 8340 passed / 0 failed.
  • codewhale-execpolicy — 89 unit + 1 doctest passed / 0 failed.
  • qa_pty — 25 passed / 0 failed, 1 ignored.
  • check-tui-locale-parity.py PASS; jq empty crates/tui/locales/*.json clean.
  • check-versions.sh, cargo fmt --all -- --check, git diff --check, and check-coauthor-trailers.py --check-authors all clean.

@Hmbown
Hmbown force-pushed the codex/harvest-grants-v092 branch from 843a969 to 7de92d7 Compare July 26, 2026 18:15
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 50s —— View job


Reviewing PR #4863 (post-rebase update)

  • Gather context (PR body, comments, rebase notes)
  • Diff current HEAD against origin/main
  • Verify the select_prev/order_for fix mentioned in the rebase comment
  • Verify changelog resync
  • Re-check correctness, tests, security on the rebased diff
  • Post final review

Summary

This update is the rebase-repair commit (7de92d7) on top of the four substantive commits I already reviewed. I diffed dc3eb1a~1..7de92d7 against origin/main (now 5629f34) and confirmed the four feature/fix commits are unchanged in scope from my prior review — only 7de92d7 is new, touching crates/tui/src/tui/approval.rs and crates/tui/CHANGELOG.md.

The compile fix is correct

select_prev (crates/tui/src/tui/approval.rs:1429) now calls ApprovalOption::order_for(&self.request), matching select_next (approval.rs:1434) and the order_for signature at approval.rs:1361, which takes &ApprovalRequest (needed so it can check request.can_save_allow_rule() for the new [p] row). This was a real bug — the pre-rebase code would not compile against current order_for, since apparently something on main changed order_for's signature from &str to &ApprovalRequest after this branch forked. Good catch, correctly fixed.

Changelog resync is correct

crates/tui/CHANGELOG.md now matches the root CHANGELOG.md's [Unreleased] section structure exactly (same entry text, same section placement). One pre-existing cosmetic nit persists in both files identically: the blank line before ## [0.9.1] - 2026-07-24 is missing (CHANGELOG.md:102-103, crates/tui/CHANGELOG.md same spot) — this was already flagged as non-blocking in my first review and is unrelated to this commit; it's just carried over from how the ## [Unreleased] bullet was removed.

Trivial rebase artifacts — benign

  • crates/tui/src/fleet/host.rs: only an import reordering (OnceLock moved below the #[cfg(windows)] imports) — no behavior change.
  • crates/tui/src/tui/views/mod.rs: persistent_ask_rules field renamed to persistent_rules on ViewEvent — cosmetic rename to match the broader ask+allow rule generalization, not new logic.
  • crates/tui/src/tui/widgets/mod.rs: matches what I already reviewed (new [p] row insertion, push_permission_rule_save_preview generalization, save_ask_rule_hint now returns Cow<'static, str> via tr() for localized text — single call site at widgets/mod.rs:2258, type checks out since Span::styled accepts Into<Cow<str>>).

Verification

I could not execute cargo check/cargo test/cargo clippy in this environment — shell commands beyond read-only git log/git diff require interactive approval I don't have here (same limitation as my first review). Static review of the diff shows the fix is consistent with the surrounding code and the invariant it needed to satisfy (order_for needs the full request to check can_save_allow_rule()). Hmbown's comment reports codewhale-tui 8340/8340, codewhale-execpolicy 89 unit + 1 doctest, qa_pty 25/25, clippy with CI's exact flags clean, locale parity PASS, and check-versions.sh/fmt/diff/coauthor-trailer checks clean on this exact head — consistent with what the diff shows.

Nothing here blocks merge. My substantive findings from the first review (workspace-scope-is-session-cwd design note, permission_rule_table/permission_rule_inline_table duplication, persist_rules_from_approval's implicit homogeneous-action assumption) still stand as non-blocking notes and weren't touched by this rebase.

@Hmbown
Hmbown merged commit 8733312 into main Jul 26, 2026
24 of 25 checks passed
@Hmbown
Hmbown deleted the codex/harvest-grants-v092 branch July 26, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants