feat(commit-push): improve push-permission UX on sandbox denial by giwankim · Pull Request #27 · giwankim/my-claude-code

giwankim · 2026-04-21T07:59:45Z

Summary

Addresses the UX issue where /commit-push tells the user to type git push themselves when the sandbox permission layer denies the push. Step 7 now emits a pre-push summary line (→ pushing <N> commit(s) to <remote>/<branch>) immediately before invoking git push. New step 7a handles denial by emitting a scoped permissions.allow snippet (Bash(git push) / Bash(git push origin) / Bash(git push origin HEAD)) and instructing re-invocation — no manual shell-out, no loop-retry. A trailing anti-pattern note documents why AskUserQuestion cannot pre-authorize a push (it's a conversation-layer tool, orthogonal to the permission layer) and why the shell-out fallback is forbidden. Evals 22 and 23 lock the behavior in: old skill 69% pass (reproduces the anti-pattern verbatim), new skill 100%.

Test plan

skill-creator eval harness run on evals 22 and 23 against both HEAD baseline and new version — benchmark.md shows 69% → 100% pass rate delta
Regression sweep on evals 1–21 (not yet run; available on request)
Live end-to-end /commit-push run in a workspace with skipAutoPermissionPrompt: true to confirm denial-handling text renders as written

- Add "Permissions (one-time setup)" section up front with scoped `Bash(git push)` / `Bash(git push origin)` / `Bash(git push origin HEAD)` allow snippet - Require one-line pre-push summary (`→ pushing <N> commit(s) to <remote>/<branch>`) immediately before `git push` - Add step 7a: on permission-layer denial, emit the scoped allow snippet + re-invocation instruction instead of falling back to manual shell-out - Extend step 8 with a push-denial report branch - Add trailing anti-pattern note forbidding both `AskUserQuestion`-for-push and "please run `git push` yourself" fallback - Add evals 22 (pre-push summary + no AskUserQuestion for push) and 23 (denial → snippet, no manual shell-out)

chatgpt-codex-connector · 2026-04-21T07:59:53Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

coderabbitai · 2026-04-21T07:59:58Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 93f90929-ddc3-4068-a46c-727ba805fb45

📥 Commits

Reviewing files that changed from the base of the PR and between 900f2c7 and 5431307.

📒 Files selected for processing (2)

skills/commit-push/SKILL.md
skills/commit-push/evals/evals.json

📝 Walkthrough

Summary by CodeRabbit

Release Notes

Documentation
- Enhanced commit-push workflow with improved permission setup guidance for sandbox environments
- Added pre-push summary displaying branch and commit count before pushing
- Improved error handling and recovery instructions for permission and hook-related push failures
- Clarified recommended workflow recovery steps after push rejection

Walkthrough

Updated the commit-push skill documentation and evals: added one-time push permission JSON guidance, a pre-push one-line summary, explicit push-failure classification and handling (no same-turn/manual git push instructions), and new eval cases validating these behaviors.

Changes

Cohort / File(s)	Summary
Commit-push documentation `skills/commit-push/SKILL.md`	Added "Permissions (one-time setup)" with exact vs prefix `permissions.allow` forms, emit a one-line pre-push summary before `git push`, replaced push-failure handling with a sub-step that classifies denials (allowlist vs PreToolUse hook vs ambiguous), provides an Option A merge-form fragment for allowlist denials, prohibits same-turn/manual `git push` retries, extends outcome reporting with push-rejection branches, and documents anti-patterns forbidding `AskUserQuestion` gating and manual-push fallbacks.
Commit-push evaluations `skills/commit-push/evals/evals.json`	Updated evals 1–2 to require the pre-push one-line summary; added evals 22–27 covering: pre-push summary/no `AskUserQuestion`, allowlist denial (single push attempt, Option A merge-fragment, re-run guidance, commits preserved), push-only fast path (skip commit steps, still emit summary), first-push/no-upstream (`git push -u origin HEAD`), PreToolUse hook rejection classification, and guard for zero unique commits (no push).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant Skill
  participant GitTool as "git (Bash)"
  participant Sandbox as "Permissions/Sandbox"
  participant Hooks as "hooks.PreToolUse"

  User->>Skill: invoke /commit-push
  Skill->>Skill: compute commit candidates & N
  Skill->>User: emit "→ pushing N commit(s) to remote/branch"
  Skill->>Sandbox: attempt Bash(git push ...) (1st/only attempt)
  Sandbox-->>Skill: allowed OR denied (allowlist)
  alt allowed
    Skill->>GitTool: run git push (or git push -u origin HEAD)
    GitTool-->>Skill: success OR hook rejection
    alt hook rejection
      Hooks-->>Skill: reject (PreToolUse)
      Skill->>User: report hook rejection (no Option A snippet)
    else success
      Skill->>User: report push success
    end
  else allowlist denied
    Sandbox-->>Skill: deny
    Skill->>User: emit Option A merge-form fragment + re-run instruction
    Skill->>User: list local commit SHAs, avoid advising manual git push
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat(skills): add install-skills target and commit-push skill #14: Modifies the same skills/commit-push/SKILL.md area; related to push/permission behavior.

Suggested labels

enhancement

Poem

🐰 I counted hops and named the branch tonight,
A tiny pre-push whisper—clear and bright.
If gates say no, I hand you JSON neat,
Come back with /commit-push — your commits stay sweet.
Hooray for safer pushes and tidy feet!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat(commit-push): improve push-permission UX on sandbox denial' directly and clearly describes the main change: improving the user experience when sandbox permission denies a push operation.
Description check	✅ Passed	The description is directly related to the changeset, providing detailed context about the UX improvements, step-by-step implementation details, and test results that align with the actual modifications in the files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch giwankim/push-permission-ux

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

…ast path - Split Permissions section into Option A (exact-match, recommended) and Option B (`Bash(git push:*)` wildcard) with explicit trade-off documentation — the original single-snippet advice relied on exact-match semantics that didn't cover common invocations like `git push origin main` - Document Claude Code's Bash pattern matcher semantics (`:*` suffix = prefix-match glob, `Bash(cmd)` = exact match) so users can pick knowingly - Add Option B + `deny` layered-allow example for users who want wildcard coverage minus force-push authorization - Add step 1a push-only fast path: clean working tree + branch ahead of upstream → skip steps 2-6, jump to step 7. Fixes the re-invocation gap after step 7a denial — previously the skill would have computed an empty candidate set on recovery and aborted - Update step 7a and step 8's push-denial report to emit Option A by default and point at Option B for users who want the wildcard - Fix step 7 phrasing: the pre-push summary gives context to the user, not to "the permission layer" (the matcher doesn't read chat text) - Tighten eval 22 assertion from subjective ("question whose purpose is to confirm") to positional ("no AskUserQuestion between the pre-push summary and the git push Bash call") - Reshape eval 23 snippet assertion to accept Option A, Option B with deny, or Option B with an explicit destructive-push flag - Add eval 24 covering the step 1a push-only fast path

giwankim · 2026-04-21T10:51:41Z

@CodeRabbit pause

coderabbitai · 2026-04-21T10:51:48Z

✅ Actions performed

Reviews paused.

…ntries Round-6 restructuring (dc5eeeb) split the Permissions section into Options A and B, and updated eval 23's structured `expectations` array to reference the new Option A entries (`Bash(git push)` and `Bash(git push -u origin HEAD)`). The `expected_output` prose was missed in that update and still cited the old shape (`Bash(git push origin)` / `Bash(git push origin HEAD)`). - Rewrite eval 23's `expected_output` to match Option A's actual entries verbatim - Mention that the skill also points at Option B as an alternative (without emitting it inline), consistent with step 7a's current instruction

- Eval 23 expectation 4: lock in "Option A inline, Option B by pointer only". Previous wording accepted either Option A or Option B inline as valid output, which contradicted SKILL.md step 7a's "do not emit Option B inline unless the user asks for it" rule and eval 23's own expected_output prose. An implementation emitting the `Bash(git push:*)` wildcard snippet inline would have passed the eval despite violating the skill. (Devin BUG) - Permissions Option B deny list: remove the redundant `Bash(git push --force-with-lease:*)` entry. `Bash(git push --force:*)` is a prefix glob, so it already covers `git push --force-with-lease ...`. Added a one-line inline note explaining the prefix coverage so readers auditing the deny list see why no separate `--force-with-lease` entry is needed. (Devin flag) - Step 1a exit branch: when the tree is clean but the branch isn't ahead, propagate any relevant state from `git status` in the exit message (behind-upstream, diverged, no upstream configured) so a silent exit becomes an informative one. Skill still does not pull/rebase/set-upstream — that's deliberately out of scope — but the user isn't left guessing why the skill noop'd. (Devin info)

giwankim · 2026-04-21T13:00:04Z

@CodeRabbit resume

coderabbitai · 2026-04-21T13:00:12Z

✅ Actions performed

Reviews resumed.

… hook-vs-allowlist split, merge-form snippets Three Codex findings, all real: - [P1] Step 1a no-upstream case. Previously step 1a's "not ahead of upstream" branch exited silently on any non-ahead clean tree, including the important first-push recovery case — a brand-new branch with local commits but no upstream configured. Re-invoking /commit-push after a step 7a denial on such a branch would have exited silently and left the commits stranded. Split step 1a into three explicit cases (ahead / no upstream / behind-diverged-upToDate). The no-upstream case now routes to step 7 as a first-push recovery path. - [P1-adjacent] Step 7 push-command selection. Step 7 previously always ran bare `git push`, which fails with "no upstream branch" on a fresh branch. Now step 7 explicitly chooses: bare `git push` when upstream is set, `git push -u origin HEAD` when it isn't. Same decision point handles routine first-push AND step 1a's first-push recovery in one rule. - [P2] Hook-rejection vs allowlist-denial distinction. Step 7a previously lumped PreToolUse hook rejections into the same bucket as allowlist denials and emitted Option A for both. But `permissions.allow` entries do not unblock hook rejections — hooks run independently. Step 7a now distinguishes the two sources from the error text and emits different guidance: Option A snippet for allowlist / sandbox / skipAutoPermissionPrompt denials, audit-hooks.PreToolUse guidance for hook rejections. Ambiguous errors get Option A with a hook-as-fallback note. - [P2] Merge-form vs whole-file snippets. Option A and Option B were emitted as top-level `{ "permissions": { ... } }` objects, which silently clobber an existing settings.json if pasted as-is. Reframed both as merge-form (bare entries to add to `permissions.allow` / `permissions.deny` arrays), with the whole-file wrapper kept as a labeled fallback for empty configs. Step 7a and step 8's push-rejection report also updated to emit the merge-form fragment and only fall back to the whole-file form when the target is confirmed empty. - Evals: tightened eval 23 to require merge-form emission and scope it to allowlist denials only. Added eval 25 (first-push recovery via step 1a no-upstream branch) and eval 26 (hook-rejection distinguished from allowlist denial, Option A not emitted).

Closes a spec gap flagged in round-9 review: step 7 requires the summary `→ pushing <N> commit(s) to <remote>/<branch>` but did not say how to compute N when step 1a's first-push recovery fires on a branch with no upstream (`@{u}` is unresolvable). Added a **Computing N** paragraph to step 7: - has upstream → `git rev-list --count @{u}..HEAD` - no upstream → `git rev-list --count HEAD --not --remotes` - no remote refs at all → `git rev-list --count HEAD` The `--not --remotes` form matches what `git push -u origin HEAD` actually publishes, so the summary stays truthful across every path. Eval 25 remains tolerant of the specific N value; the tightening is for implementers, not the eval contract.

…, merge safety, per-remote count Four real issues surfaced in round-9 review: 1. Option B's `deny` recipe was leaky. Prefix-glob matching only anchors at the start of the command, so `Bash(git push --force:*)` and friends never catch remote-first forms like `git push origin --delete X` or `git push origin --force-with-lease Y`. Retracted the false-safety recipe; now explicitly recommends Option A (exact-match) for destructive-push gating, which doesn't rely on deny patterns at all. 2. Step 1a's no-upstream branch treated every clean + no-upstream state as first-push recovery and always pushed with `-u origin HEAD`. A branch with zero commits unique to origin was silently creating empty `origin/<branch>` pollution. Added an N>0 guard: compute `git rev-list --count HEAD --not --remotes=origin` first, exit cleanly if 0. 3. The whole-file fallback snippets were conditioned on "no permissions key yet" — but a settings.json with `hooks` (or any other sibling key) but no `permissions` would still be clobbered by a whole-file paste. Narrowed the condition to "file does not exist yet or is `{}`" in the Permissions section, step 7a's merge-form guidance, and eval 23's expectation. 4. `git rev-list --count HEAD --not --remotes` excluded commits on *any* remote, not just origin. In multi-remote forks (branch based on `upstream/X`), it reported 0 even when origin would receive real commits. Changed to `--remotes=origin` in both step 1a's guard and step 7's Computing N, with a note about the multi-remote case. Added eval 27 locking in the N==0 guard (branch off main with no new commits → no push, no branch on origin).

…h rule CodeRabbit caught a factual error in the Option B deny-list caveat: `Bash(git push -f:*)` IS a prefix-glob, so it does match `git push -f origin main` (which starts with `git push -f`). The commentary claiming otherwise was wrong and weakened the argument. Fixed by: - Replacing the bad example `git push -f origin main` with the actual bypass vector `git push origin main -f` (remote-first form, which doesn't start with `git push -f` and so isn't matched). - Adding a clarifying paragraph that spells out the pattern explicitly: prefix-glob catches flag-first forms across the board, misses remote-first forms across the board. This makes the deny-list leak a systematic property of the matcher rather than a per-example surprise. - Tagging the fence as ```text (was untagged) per a separate markdownlint / Devin note. Also tightened eval 26's hook-branch expectation per CodeRabbit: the scenario sets up an error text that unambiguously names the hook (`PreToolUse hook rejected: ...`), so the ambiguous-branch "Option A as a hedge" exception doesn't apply. Eval now forbids Option A at all for this scenario, matching SKILL.md step 7a's strict hook-branch rule.

giwankim · 2026-04-21T15:19:28Z

@CodeRabbit pause

coderabbitai · 2026-04-21T15:19:35Z

✅ Actions performed

Reviews paused.

Devin round-10 review flagged that "inspect the branch's relationship to its upstream and pick a branch:" uses the word "branch" twice with different senses (git branch vs. decision-tree branch) in a git-heavy context. Replaced the second sense with "take the matching case below:" to remove the momentary reader confusion. No behavior change.

sonarqubecloud · 2026-04-21T15:20:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

devin-ai-integration Bot reviewed Apr 21, 2026

View reviewed changes

coderabbitai Bot approved these changes Apr 21, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

giwankim self-assigned this Apr 21, 2026

giwankim added the bug Something isn't working label Apr 21, 2026

This comment was marked as resolved.

Sign in to view

coderabbitai Bot approved these changes Apr 21, 2026

View reviewed changes

giwankim merged commit 5a905d4 into main Apr 21, 2026
6 checks passed

giwankim deleted the giwankim/push-permission-ux branch April 21, 2026 15:26

Conversation

giwankim commented Apr 21, 2026 • edited by devin-ai-integration Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

chatgpt-codex-connector Bot commented Apr 21, 2026

Uh oh!

coderabbitai Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Summary by CodeRabbit

Release Notes

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

giwankim commented Apr 21, 2026

Uh oh!

coderabbitai Bot commented Apr 21, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

giwankim commented Apr 21, 2026

Uh oh!

coderabbitai Bot commented Apr 21, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

giwankim commented Apr 21, 2026

Uh oh!

coderabbitai Bot commented Apr 21, 2026

Uh oh!

sonarqubecloud Bot commented Apr 21, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

giwankim commented Apr 21, 2026 •

edited by devin-ai-integration Bot

Loading

coderabbitai Bot commented Apr 21, 2026 •

edited

Loading