Skip to content

fix(claude-permissions): deny git restore path discards like checkout - #351

Merged
kyle-sexton merged 2 commits into
mainfrom
cursor/claude-permissions-git-restore-deny-f19e
Aug 12, 2026
Merged

fix(claude-permissions): deny git restore path discards like checkout#351
kyle-sexton merged 2 commits into
mainfrom
cursor/claude-permissions-git-restore-deny-f19e

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Closes the asymmetry where git checkout -- <path> was denied but git restore <path> was not.

Adds mirror deny rules for both spellings of discarding uncommitted working-tree changes:

  • -- separator forms: git restore -- *, git restore * -- *
  • Bare-path catch-all: git restore * (restore has no branch/ref ambiguity)
  • Matching PowerShell(...) counterparts, including git * restore prefixed forms

Extends the component test to pin the new rules in CI. Bare git checkout <path> without -- is left unchanged — the glob grammar cannot distinguish a path from a branch name.

Test plan

  • bash harness/shell/run-tests.sh components/claude-permissions/claude-permissions.test.sh — pass
  • jq empty components/claude-permissions/claude-permissions.json — valid JSON
  • CI green on merge

Related

Closes #334

Open in Web Open in Cursor 

@kyle-sexton
kyle-sexton marked this pull request as ready for review August 11, 2026 23:33
@cursor

cursor Bot commented Aug 11, 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 Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 53s —— View job


Review: PR #351 — deny git restore path discards

Scope: components/claude-permissions/claude-permissions.json and components/claude-permissions/claude-permissions.test.sh (the only two files changed by this PR's single commit, verified via git show HEAD).

  • Gather context (diff, CLAUDE.md, REVIEW.md, component README)
  • Review claude-permissions.json rule additions
  • Review claude-permissions.test.sh test additions
  • Check for gaps / asymmetries in glob coverage
  • Post findings

Posted 3 inline comments on claude-permissions.json:

  1. 🔴 Important (L201) — the new bare Bash(git restore *) catch-all is a whole-string glob (per the component README's threat-model section), so it also denies git restore --staged <path>, a non-destructive unstage that never touches the working tree. That's broader than the PR's stated "discard" parity goal with checkout, and inconsistent with git reset <path> (the equivalent unstage form) staying ungated.
  2. 🔴 Important (L239) — PowerShell(git * restore *) has no flag anchor (unlike sibling prefixed rules like git * push --force/git * reset --hard), so as an unanchored whole-string glob it matches any PowerShell git command whose args merely contain " restore " as a substring — e.g. git commit -m "please restore old default" — since "restore" is an ordinary word rather than a distinctive flag.
  3. 🟡 Nit (L193) — Bash(git restore * -- *) and Bash(git restore -- *) (L196) are dead on arrival: both are fully subsumed by the bare Bash(git restore *) added in the same commit (L201). Same redundancy repeats in the PowerShell(git * restore …) block (L236 vs L239) and the PowerShell(git restore …) block (L317/L320 vs L325).

Additional non-blocking observation (not inline-commented, spans multiple lines): the file otherwise maintains strict ASCII-alphabetical sort within each array (confirmed against the pre-existing reset/switch deny groups, where *-prefixed forms sort before -/./: forms). The three new bare catch-alls (Bash(git restore *) [L201], PowerShell(git * restore *) [L239], PowerShell(git restore *) [L325]) are appended at the end of their respective restore subgroups instead of sorted in — each should sort immediately after the * :/) line and before the -- *) line, since * (0x2A) < - (0x2D) in ASCII.

Not reviewed: security-lane concerns are out of scope for this lane per REVIEW.md (this repo has no claude-security-review workflow gate mentioned in scope docs for this change, but the change itself is permissions-floor configuration, not application code, so no security-lane findings apply regardless).

Test coverage: claude-permissions.test.sh pins presence of the 8 new deny strings via jq -e ... index(...), consistent with the file's existing shallow existence-check style for other deny families — reasonable given the file's own precedent, though it wouldn't have caught findings #1/#2 above (those are about over-matching, not missing entries). The PR's test-plan checkboxes (harness test run, jq empty validity) are author-claimed and were not independently re-run by this review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebf74fdd20

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread components/claude-permissions/claude-permissions.json Outdated
@cursor
cursor Bot force-pushed the cursor/claude-permissions-git-restore-deny-f19e branch from ebf74fd to 7dbd4a9 Compare August 11, 2026 23:37
Comment thread components/claude-permissions/claude-permissions.json Outdated
Comment thread components/claude-permissions/claude-permissions.json Outdated
Comment thread components/claude-permissions/claude-permissions.json
@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

Mirror checkout discard coverage for git restore: add -- separator
forms, bare-path catch-all, and PowerShell counterparts so
`git restore <path>` is blocked the same way as
`git checkout -- <path>`.

Closes #334

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/claude-permissions-git-restore-deny-f19e branch from 7dbd4a9 to b818457 Compare August 11, 2026 23:42
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Addressed the Important reviews: dropped catch-all restore denies that blocked --staged / over-matched PowerShell; kept ---separator forms; documented the bare-path grammar limit. Re-running CI after the transient eol-renormalize TLS checkout failure. 🤖

Drop catch-all `git restore *` rules that also blocked `--staged` unstage
and over-broad PowerShell `git * restore *`. Document the bare-path gap
as the same glob/precedence limit as --force-with-lease.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@kyle-sexton
kyle-sexton merged commit e47450b into main Aug 12, 2026
42 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/claude-permissions-git-restore-deny-f19e branch August 12, 2026 00:13
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.

claude-permissions: git restore <path> is not denied while git checkout -- <path> is

2 participants