feat(commit-push): auto-detect cohesive chunks and propose multi-commit splits#26
Conversation
…it splits
- Add auto-detect phase that groups changed files by inferred Angular scope (skill subdir, then top-level dir, then root-files bucket); silently produce one commit when only one group results
- Add three-option AskUserQuestion proposal when two or more groups are detected: confirm split, combine into one, or fall through to interactive --select
- Recognize --single / -1 flag and natural-language phrases ("single commit", "one commit", "as one", "all together", "commit this as one", "as a single commit") as overrides that skip auto-detect; the override is acknowledged in the reply
- Restructure step 5 as a per-commit loop with per-iteration pre-commit hook recovery that creates NEW commits (never --amend) so earlier iterations are preserved
- Surface a cap warning when the proposed split exceeds 5 groups
- Report all commit SHAs (one per line) instead of a single SHA
- Add evals/evals.json with 7 scenarios covering single-group default, two-group split, both override mechanisms, --select no-regression, cap warning, and pre-commit hook failure mid-split
- Update frontmatter description to mention the new auto-split behavior so the skill triggers correctly
📝 WalkthroughSummary by CodeRabbit
WalkthroughCompute candidate files (args / interactive select / changed), apply secret-file exclusions, auto-detect or accept overrides for commit groups, run per-group stage→commit loops with hook-fix retries, perform post-loop status normalization and conditional reconciliation commit, then push once and report all created SHAs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Skill as Commit-Push Skill
participant Git as Git (index/worktree)
participant Hooks as Pre-commit Hooks
rect rgba(135,206,250,0.5)
User->>Skill: invoke /commit-push (flags / natural language)
Skill->>Skill: compute candidate set\n(`@file` / --select / changed)
Skill->>Skill: apply secret exclusions\nand detect commit groups
end
rect rgba(144,238,144,0.5)
alt multi-group detected
Skill->>User: AskUserQuestion (propose split/combine/adjust)
User->>Skill: selection/adjust
loop per-group
Skill->>Git: git restore --staged :/ (clear index)
Skill->>Git: stage group's files
Skill->>Git: git commit (create new SHA)
Git->>Hooks: run pre-commit hooks
alt hook fails and fixes
Hooks->>Git: modify working tree
Skill->>Git: stage group's files
Skill->>Git: git commit (new SHA, do not amend)
end
end
else single-group
Skill->>Git: stage candidate files
Skill->>Git: git commit (single SHA)
end
end
rect rgba(255,182,193,0.5)
Skill->>Git: snapshot pre-loop status
Skill->>Git: compute post-loop delta
alt delta within original candidate set
Skill->>Git: create reconciliation commit
else
Skill->>User: warn about out-of-scope changes (no auto-stage)
end
Skill->>Git: git push (single push)
Skill->>User: report created SHAs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
- Decouple file scoping from commit-count mode in step 2 so --single (and natural-language overrides) no longer bypass explicit @file args or the secret filter — secret filtering now applies on every path including overrides [P1] - Add iterative custom-groupings sub-flow at step 4b so "Adjust groupings" can produce multi-commit batches; previously it fell through to --select which only yielded one commit and left remaining changes uncommitted [P1] - Refine the auto-detect heuristic to attribute test files (tests/, test/, __tests__/, *_test.*, test_*.*) to their paired implementation scope by path-component match or sole-non-test-scope fallback so impl + its tests stay one cohesive commit [P2] - Enumerate concrete secret-filter patterns (.env, *.pem, id_rsa*, credentials*, *token*, *secret*) and require listing exclusions in the reply so the user can re-include false positives - Add evals 8-11 covering: --single + explicit args (file scoping preserved), --single + .env (secret filter preserved), Adjust groupings producing 2 custom commits, code+tests grouping into a single commit - Update frontmatter description to flag that file scoping and secret filtering apply on every path including the override
- Add eval 12 covering the `-1` short-flag end-to-end (parallel to eval 3 for `--single`); closes the only genuine gap in the round-2 PR review - Tighten step 2a's conflict warning to spell out file-scoping precedence: explicit `@file` args > `--select`/`-i` > default "all changed files"; removes the residual ambiguity round-2 reviewer found in the prior list-order-only formulation - Round-2 review comments on "Adjust groupings collapses to one commit" and "first-match-wins contradicts explicit precedence" were already addressed structurally in the prior fix commit (da07a4b): Adjust groupings now routes to step 4b's iterative custom-groupings sub-flow that loops until every candidate file is assigned, and the "first match wins" mode-selection language was removed when step 2 was decoupled into 2a (file scoping) / 2b (secret filter) / 2c (commit-count mode)
- Insert explicit `.github/` rule (scope `ci`) before the generic top-level-dir rule in step 3 so `.github/` files reach `ci` scope instead of being captured as scope `.github` by the prior unreachable-rule layout (closes Devin BUG_0002) - Rewrite step 3 root-files rule from "one group with mixed per-file scope inference" to "group by inferred scope" — multiple root files now form one group per inferred scope (Makefile/Dockerfile → `build`, README/top-level *.md → `docs`, etc.); resolves the contradiction between "one group" and per-file scope mapping (closes CodeRabbit major and Devin ANALYSIS_0001) - Add step 6 reconciliation: after the per-commit loop, scan `git status` for unstaged residue from repo-wide pre-commit hook auto-fixes that touched files outside the current iteration's group; create one final chore commit covering the residue and mention it in the reply so the hook fixes are not silently dropped (closes Codex P1 #2 and Devin ANALYSIS_0002); renumber existing steps 6→7 (push) and 7→8 (report) - Fix eval 1 path inconsistency: `commit-push/notes.md` → `skills/commit-push/notes.md` so the example matches the surrounding "two files under skills/commit-push/" prose (closes CodeRabbit minor) - Add missing `"files": []` field to eval 7 to match the schema used by every other eval (closes Devin BUG_0001) - Add eval 13 (root-files multi-scope: Makefile + README.md → `build` + `docs`), eval 14 (.github/workflows file → scope `ci`), and eval 15 (cross-group hook reconciliation produces one chore commit so no residue is lost)
|
@codex review |
- Refine step 6 reconciliation to use a delta against a pre-loop `git status --porcelain` snapshot so pre-existing unrelated unstaged changes (files the user already had dirty in their working tree before invoking the skill) are NOT swept into the reconciliation commit; only files newly modified during the per-commit loop count as residue (closes round-3 review comment 3106487523)
- If the delta contains files outside the original candidate set (a hook touched something the user wasn't trying to commit), warn the user instead of auto-staging — auto-committing un-opted-in files would violate the candidate-set principle
- Add eval 16 covering the 'Cancel and return to the proposal' path of step 4b's iterative custom-grouping sub-flow: user starts a custom grouping, cancels mid-flow, returns to the original 3-option proposal, and accepts the auto-detected split (closes round-3 review comment 3106487274)
- Reword eval 6 prompt: "6 distinct grouping scopes" instead of "6 distinct top-level directories" since `skills/spring-init/` and `skills/commit-push/` share the `skills/` parent and `Makefile` has no parent — clarifies the intent without changing the eval's expectations (closes round-3 review comment 3106487636)
- Add a trade-off note under step 3's test-refinement rules explaining that rule 3b ("sole non-test scope") means adding a 2nd non-test scope can split test files off; intentional design (arbitrary attachment would be more surprising); user can override with --select or Adjust groupings (closes round-3 review comment 3106488221 with documentation rather than code change)
…d cross-rule scope merging
- Drop bare "as one" and "one commit" from step 2c's override phrase list — they false-positive on common prose like "commit this as one last sanity check" or "this should be one commit per scope"; require unambiguous multi-word phrases ("single commit", "as one commit", "as a single commit", "commit this as one", "all together") that the user has to express deliberately (closes CodeRabbit review nit on lines 36-38)
- Switch step 5.1 from `git reset HEAD` to `git restore --staged .` per Git 2.23+ guidance to avoid confusion with `git reset --hard`; legacy form noted parenthetically for older Git (closes CodeRabbit nit on line 81)
- Add explicit cross-rule scope merging clarification to step 3: groups are keyed by *resolved scope name*, so the same scope name produced by different rules (e.g., `docs` from `docs/README.md` via rule 3.3 plus `docs` from a root-level `README.md` via rule 3.4) merges into one group; closes the latent ambiguity CodeRabbit flagged for cross-rule scope collisions on lines 42-46
- Add eval 17 covering the cross-rule scope merge end-to-end: docs/README.md + root README.md → 1 commit with scope (docs), no split proposal shown
|
@coderabbitai — addressed all 3 nitpicks from your CHANGES_REQUESTED review on Lines 36–38 — Override phrases too permissive: Dropped bare Line 81 — Lines 42–46 — Cross-rule scope name collisions: Added a "Cross-rule scope merging" paragraph to step 3 stating that groups are keyed by resolved scope name, not by the rule that produced them. So LanguageTool nits (skipped, both false positives): the Threads/branch: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the changes in the current state of the file. 🏁 Script executed: Length of output: 12965
The LanguageTool skips are reasonable — LGTM — happy to approve once you're ready. |
…wording Old step 2b text contained a latent contradiction between line 29 (filter applies on every path including --single / explicit @file) and line 32 (users can re-include excluded files via --select or explicit args). Both statements cannot be simultaneously true; eval 9 and eval 18 pin the actual behavior to "filter is hard-enforced regardless of flags." - Extend line 29's parenthetical to explicitly list every flag the filter overrides (--single, -1, --select, explicit @file) and state the filter cannot be bypassed by any flag. - Rewrite line 32 to drop the misleading "re-include via --select or explicit args" suggestion (which doesn't actually work) and replace it with the three real workarounds for false-positive filter hits: rename the file, adjust the filter rules, or commit manually outside the skill. - Explicitly negate the prior suggestion so no LLM reader defaults to the removed behavior.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/commit-push/SKILL.md (1)
29-32:⚠️ Potential issue | 🔴 CriticalSecret filter bypass claim contradicts the absolute safety invariant.
Line 32 states users can "re-include them with
--selector explicit args if the exclusion was a false positive." This directly contradicts:
- Line 29's claim that the filter applies "on every path, including when
--singleor explicit@fileargs are used"- The retrieved learning documenting that the filter "cannot be bypassed by any flag" and is "enforced by evals 9 and 18"
- The past review thread where this exact issue was supposedly fixed in commit ae511e9
Based on learnings, the correct behavior is: the filter is an absolute safety invariant with no bypass mechanism. Users hitting false positives have three workarounds: (a) rename the file, (b) adjust filter rules in the skill, or (c) commit manually outside the skill. Re-invoking with
--selector explicit args will NOT re-include a filtered file.🔒 Required fix to match the documented invariant
- - List any excluded files explicitly in your reply so the user can re-include them with `--select` or explicit args if the exclusion was a false positive. + - List any excluded files explicitly in your reply so the user knows what was omitted. If the exclusion was a false positive (e.g., a genuine test fixture whose filename matches the secret pattern), the user's workarounds are (a) rename the file so it no longer matches, (b) adjust the filter rules above if the mis-match is structural, or (c) commit the file manually outside this skill (`git add` + `git commit` + `git push`). Re-invoking `/commit-push` with `--select` or explicit `@file` args will NOT re-include the filtered file — the filter is deliberately absolute as a safety invariant, and no flag overrides it.Based on learnings: In
skills/commit-push/SKILL.md, the secret filter (step 2b) is an absolute safety invariant that cannot be bypassed by any flag, with evals 9 and 18 enforcing this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/commit-push/SKILL.md` around lines 29 - 32, Update the "b) Secret filter" section to remove the claim that filtered files can be re-included with `--select` or explicit args (the phrase "so the user can re-include them with `--select` or explicit args" must be deleted) and instead state that the secret filter is an absolute safety invariant (applies on every path and cannot be bypassed by any flag), reference that this behavior is enforced by evals 9 and 18, and list the three supported workarounds for false positives: rename the file, adjust the filter rules in the skill, or commit manually outside the skill; ensure this change is applied to the "b) Secret filter" paragraph and any nearby text that contradicts the invariant.
🧹 Nitpick comments (1)
skills/commit-push/SKILL.md (1)
1-4: Consider shortening the description field for better readability.The description on line 3 is over 400 characters and packs multiple concepts (auto-detect, overrides, file scoping, secret filtering, triggers) into a single YAML field. While comprehensive, this density may harm readability when the skill is listed in tooling or parsed by systems with line-length limits.
📝 Suggested refactor to split concerns
Move detailed behavior to the Workflow section and keep the description field focused on the primary use case:
-description: Commit staged/unstaged changes and push to origin with an Angular-style commit message. By default, auto-detects when changes span multiple cohesive scopes and proposes splitting them into multiple commits; user can override with `--single`/`-1` or natural-language phrases like "as one commit" or "single commit". File scoping (explicit `@file` args, `--select` interactive picker) and secret-file filtering apply on every path, including the override. Use when the user says "commit and push", "commit this", "/commit-push", or after completing work that should be committed. Accepts optional file arguments to scope the commit, or interactive selection to pick files from the working tree. Also use when the user wants to choose, select, or pick which changed files to include in a commit. +description: Commit changed files and push to origin with Angular-style messages. Auto-detects cohesive scopes and proposes multi-commit splits; supports overrides, file scoping, and interactive selection. Use for "commit and push", "commit this", or when choosing files to commit.Then add a "Key Features" subsection under Workflow that enumerates:
- Auto-detect vs. single-commit modes
- File scoping mechanisms (explicit args, --select, default)
- Secret filtering (always enforced)
- Multi-commit proposal with preview
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/commit-push/SKILL.md` around lines 1 - 4, The description for the skill "commit-push" is too long and dense; shorten the top-level YAML description field to a single-sentence summary of the primary use case and move detailed behavior into a new "Workflow" section (or a "Key Features" subsection) inside the skill body; specifically, trim the current long description in SKILL.md under name: commit-push to a concise phrase (e.g., "Commit staged/unstaged changes with an Angular-style message and push to origin"), then create bullets in Workflow that enumerate Auto-detect vs --single behavior, file scoping (explicit `@file` args and --select), secret-file filtering, and multi-commit proposal/preview so all details are removed from the description field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@skills/commit-push/SKILL.md`:
- Around line 29-32: Update the "b) Secret filter" section to remove the claim
that filtered files can be re-included with `--select` or explicit args (the
phrase "so the user can re-include them with `--select` or explicit args" must
be deleted) and instead state that the secret filter is an absolute safety
invariant (applies on every path and cannot be bypassed by any flag), reference
that this behavior is enforced by evals 9 and 18, and list the three supported
workarounds for false positives: rename the file, adjust the filter rules in the
skill, or commit manually outside the skill; ensure this change is applied to
the "b) Secret filter" paragraph and any nearby text that contradicts the
invariant.
---
Nitpick comments:
In `@skills/commit-push/SKILL.md`:
- Around line 1-4: The description for the skill "commit-push" is too long and
dense; shorten the top-level YAML description field to a single-sentence summary
of the primary use case and move detailed behavior into a new "Workflow" section
(or a "Key Features" subsection) inside the skill body; specifically, trim the
current long description in SKILL.md under name: commit-push to a concise phrase
(e.g., "Commit staged/unstaged changes with an Angular-style message and push to
origin"), then create bullets in Workflow that enumerate Auto-detect vs --single
behavior, file scoping (explicit `@file` args and --select), secret-file
filtering, and multi-commit proposal/preview so all details are removed from the
description field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0e9ebec-f14c-4dbc-a05b-c8e1666d7b1e
📒 Files selected for processing (1)
skills/commit-push/SKILL.md
📜 Review details
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:29-33
Timestamp: 2026-04-19T12:13:30.522Z
Learning: In `skills/commit-push/SKILL.md`, the secret filter (step 2b) is an absolute safety invariant: it cannot be bypassed by any flag (`--single`, `-1`, `--select`, or explicit `file` args). Users who hit a false positive have three workarounds: (a) rename the file so it no longer matches the pattern, (b) adjust the filter rules in the skill, or (c) commit the file manually outside the skill (`git add` + `git commit` + `git push`). Re-invoking `/commit-push` with `--select` or explicit `file` args will NOT re-include a filtered file. This is enforced by evals 9 and 18 in `skills/commit-push/evals/evals.json`.
📚 Learning: 2026-04-19T12:13:30.522Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:29-33
Timestamp: 2026-04-19T12:13:30.522Z
Learning: In `skills/commit-push/SKILL.md`, the secret filter (step 2b) is an absolute safety invariant: it cannot be bypassed by any flag (`--single`, `-1`, `--select`, or explicit `file` args). Users who hit a false positive have three workarounds: (a) rename the file so it no longer matches the pattern, (b) adjust the filter rules in the skill, or (c) commit the file manually outside the skill (`git add` + `git commit` + `git push`). Re-invoking `/commit-push` with `--select` or explicit `file` args will NOT re-include a filtered file. This is enforced by evals 9 and 18 in `skills/commit-push/evals/evals.json`.
Applied to files:
skills/commit-push/SKILL.md
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Apply instructions in `AGENTS.md` only within `hooks/claude-notify/` directory, not repository-wide
Applied to files:
skills/commit-push/SKILL.md
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Applies to hooks/claude-notify/Tests/required-cases.txt : List every test scenario in `Tests/required-cases.txt` (Case-ID gate)
Applied to files:
skills/commit-push/SKILL.md
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Applies to hooks/claude-notify/README.md : Keep `README.md` synchronized with behavior, environment variables, and test changes in hooks/claude-notify
Applied to files:
skills/commit-push/SKILL.md
🪛 LanguageTool
skills/commit-push/SKILL.md
[uncategorized] ~44-~44: The official name of this software platform is spelled with a capital “H”.
Context: ... 2. CI/infra directory: files under .github/... group as scope ci. This rule fir...
(GITHUB)
[uncategorized] ~44-~44: The official name of this software platform is spelled with a capital “H”.
Context: ...neric top-level-directory rule below so .github doesn't end up as a literal directory ...
(GITHUB)
[uncategorized] ~48-~48: The official name of this software platform is spelled with a capital “H”.
Context: ...s(scope)commit). Same forci(e.g.,.github/workflows/test.yml` from rule 3.2 plus ...
(GITHUB)
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...1 group → silently jump to step 5. - If 2+ groups → continue to step 4. 4. **P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.0)
skills/commit-push/SKILL.md
[warning] 96-96: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 97-97: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 98-98: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 99-99: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 99-99: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 102-102: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 103-103: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 104-104: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 109-109: Spaces inside code span elements
(MD038, no-space-in-code)
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
1. Lines 29–32 — Stale duplicate: Confirmed. My review 2. Lines 1–4 — Dense description: Point well taken. If the frontmatter Let me resolve both threads: 🏁 Script executed: Length of output: 261
The only remaining unresolved thread from me is a separate one: ✏️ Learnings added
🧠 Learnings used |
Step 5.4 previously said "run the fixer and create NEW commit" with no exit condition — an LLM could loop indefinitely if the fixer didn't resolve the failure, or silently drop a failed group. Tightens the hook-failure path to a bounded recovery with clear abort semantics: - Step 5.4a: determine a fixer from hook stderr + file types, with common-language examples (prettier, ruff/black, gofmt, markdownlint, shfmt). Explicit "skip to 4c" if the hook is a spec check without an auto-fix equivalent. - Step 5.4b: bounded retry — up to 2 fixer-driven retries (total ≤3 `git commit` attempts per iteration). Two retries absorbs multi-pass fixers like eslint --fix. Every retry creates a NEW commit (never --amend, preserving earlier iterations' commits). - Step 5.4c: on exhausted retries OR no fixer determinable, mark the group FAILED and abort the loop. Do not attempt remaining groups, do not reconcile, do not push. Earlier successful commits stay local; user resolves manually and re-invokes. Downstream updates to keep the abort consistent: - Step 6 now explicitly skips on abort (its delta rule assumes every candidate file was committed, which isn't true after 5.4c). - Step 7 skips the push on abort (would otherwise surface partial results while silently dropping the failed group — the opposite of the atomic-batch invariant). - Step 8 reports the failure context on abort: failing group + files, hook stderr, unprocessed groups, and guidance to fix + re-invoke. Add eval 21 covering the abort path with a spec-check hook that has no auto-fix (simulating pytest/jsonschema/integrity-check semantics). Eval 7 continues to cover the retry-succeeds path from the opposite side, so the two together bracket the hook-failure behavior space.
|
@CodeRabbit pause |
✅ Actions performedReviews paused. |
- Eval 21 setup was unsatisfiable: "hook that ALWAYS fails" would fail the first (spring-init) commit too, making the expected "1 commit created" unreachable. Reworded to "succeeds the first time, fails thereafter" so group 1 commits normally and group 2 exercises the abort path as intended. - Eval 21 expected_output incorrectly narrated "attempts step 5.4b's bounded retry" for a spec check, but step 5.4a routes spec checks directly to 5.4c without retries. Rewrote the narrative to match the actual path (5.4a recognition → direct to 5.4c, no 5.4b attempts). - Eval 19's "no warning about fileA.md" expectation was too broad; step 2a's pre-existing-staging warning mentions fileA too but is orthogonal to the step-6 reconciliation path under test. Scoped the expectation to "no step-6 reconciliation warning" and noted the step-2a warning is expected and allowed. - SKILL.md:48 cross-rule scope merging text said "one `docs(scope)` commit" which conflates the Angular pattern syntax with a literal scope name. Changed to `docs(docs): ...` (and `ci(ci): ...` for the parallel example) so the scope-name-resolving-to-same-value case is unambiguous. - SKILL.md step 6 normalization rules used `M <path>` with ONE space between status and path, but porcelain v1 is `XY<SP><path>` (2 chars + separator = 2 visible spaces for single-column states like `M <path>`). Added a preamble documenting the format and corrected M/D/A examples to use the two-space form so a literal parser sees the rules unambiguously.
Targeted simplification pass based on 3-lens review (reuse/quality/efficiency), then a triggering fix informed by skill-creator guidance. Prose cuts (-176 words / -1,109 chars, ~5.5% reduction in spec body): - Step 2 preamble: 3-sentence philosophy → single clause retaining the load-bearing "why overrides can't bypass the secret filter". - Step 2b secret-filter false-positive enumeration: (a)/(b)/(c) list plus re-assertion of invariant → comma-separated workarounds + one-sentence "no flag re-includes" closer. - Step 5.1 git-command rationale: two parentheticals and an all-caps prohibition → one parenthetical + a single sentence against `git restore --staged .` with the CWD reason. - Step 6 normalization tail paragraph: "Without this normalization, a user who ran `git add fileA`..." historical rationale deleted; the rule statement is self-sufficient. Frontmatter description: broadened trigger coverage per skill-creator "pushy triggering" guidance. Added explicit triggers for "check in these changes", "save my work", "wrap this up", "push my changes", and "after completing any task that produced file changes the user will want to land" — addressing Claude's well-documented tendency to undertrigger skills. Core behavior wording unchanged.
Matches the existing pattern for spring-init-workspace/. Contents are reproducible eval outputs (Python .venv, auto-generated HTML viewer, subagent transcripts, run_loop logs) — not source to track.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
skills/commit-push/evals/evals.json (1)
113-123:⚠️ Potential issue | 🟠 MajorRemove the secret-file “override” expectation.
Line 122 contradicts the skill’s absolute filter rule: excluded files should be reported with the documented workarounds, not described as overrideable. Based on learnings, re-invoking with
--selector explicit file args will NOT re-include a filtered file.Suggested eval wording
- "The skill's reply lists which files were excluded so the user can override if needed", + "The skill's reply lists which files were excluded and explains that no flag re-includes them; false positives require renaming the file, adjusting the filter rules, or committing manually outside the skill",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/commit-push/evals/evals.json` around lines 113 - 123, Update the eval for id 9 to remove the incorrect "override" expectation: delete the expectation that says "The skill's reply lists which files were excluded so the user can override if needed" and instead ensure the remaining expectations reflect that excluded files are reported with documented workarounds (e.g., re-invoke with --select or pass explicit file args) rather than being re-includable; edit the expectations array in the eval entry (id 9) to remove the contradictory line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/commit-push/evals/evals.json`:
- Around line 236-237: Update the "expected_output" JSON for Scenario 18 to
remove the incorrect assertion that the secret-filter excluded .env; instead
state that .env started clean so it was not in the changed-file candidate set,
the per-group loop produced the usual skills/foo commit, and after the loop step
6 detects .env as post-loop residue classified as outside the candidate set so
the warn-branch fires — the tool warns the user that .env was touched by a
repo-wide hook, does NOT auto-stage or auto-commit it, and leaves the decision
to the user. Reference the "prompt" and "expected_output" keys in this JSON
object when making the edit.
- Around line 195-203: The scenario's pre-commit hook currently mutates "ALL
tracked .md files", which can make reconciliation non-deterministic if other
tracked markdown exists; update the eval entry (the JSON object containing the
"prompt" string and associated "files"/"expectations") so the hook only targets
the candidate files or explicitly guarantee the scratch repo contains no other
tracked .md files. Concretely, modify the "prompt" text to either (a) change
"ALL tracked .md files" to a precise list like "skills/spring-init/SKILL.md and
hooks/notes.txt" or (b) add a setup line stating "the repo has no other tracked
.md files", or instead add those two files to the eval's "files" fixture to
ensure determinism; keep the "expectations" unchanged except to ensure they
reflect the deterministic reconciliation behavior.
In `@skills/commit-push/SKILL.md`:
- Around line 46-48: The cross-rule `ci` example conflicts with the root-file
scope mapping in rule 3.4 (the list that maps root files to
`build`/`docs`/`chore`), so update the spec so the example is consistent: either
extend rule 3.4 to include root CI config files (e.g., map `.codecov.yml` and
other top-level CI configs to the `ci` scope) or remove the `ci` example from
the cross-rule scope merging paragraph; edit the text where rule 3.4 enumerates
root-file mappings and/or the cross-rule example sentence referencing `ci` to
reflect the chosen fix (look for the phrases "Root files (no directory prefix)"
and "Same for `ci`" to locate the lines to change).
- Line 10: The secret-filtering logic must expand untracked directory entries
reported by git status --porcelain into individual file paths before applying
the secret filter and before staging; update the candidate-file determination so
when an entry like "?? dir/" is encountered you run git ls-files --others
--exclude-standard -- <dir>/ to enumerate its constituent files, apply the
existing step 2b secret filter to each returned path, and then stage only the
explicit file paths in step 5 (do not run git add on the directory pathspec);
ensure this change is applied to the code that builds the candidate set and to
the staging routine so every commitable path (not just directory names) is
individually checked.
---
Duplicate comments:
In `@skills/commit-push/evals/evals.json`:
- Around line 113-123: Update the eval for id 9 to remove the incorrect
"override" expectation: delete the expectation that says "The skill's reply
lists which files were excluded so the user can override if needed" and instead
ensure the remaining expectations reflect that excluded files are reported with
documented workarounds (e.g., re-invoke with --select or pass explicit file
args) rather than being re-includable; edit the expectations array in the eval
entry (id 9) to remove the contradictory line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3871653e-6199-460b-ba5d-045b2f5f9a83
📒 Files selected for processing (3)
.gitignoreskills/commit-push/SKILL.mdskills/commit-push/evals/evals.json
✅ Files skipped from review due to trivial changes (1)
- .gitignore
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:82-93
Timestamp: 2026-04-19T22:41:52.404Z
Learning: In `skills/commit-push/SKILL.md` (PR `#26`), the hook-failure policy in Step 5.4 was tightened in commit 5a27de3: (a) fixer is determined from hook stderr + file types with concrete examples (prettier, ruff/black, gofmt, markdownlint-cli, shfmt); checks with no auto-fix go directly to abort; (b) bounded retry of ≤2 fixer-driven retries (≤3 total git commit attempts per iteration — two retries because eslint --fix can need multiple passes for cascading autofixes); (c) on exhausted retries OR no determinable fixer, the group is marked FAILED and the loop aborts — do NOT attempt remaining groups, do NOT reconcile, do NOT push. Abort was chosen over skip to preserve the atomic-batch invariant (consistent with the secret filter's design philosophy). Steps 6, 7, and 8 all condition on the abort flag. Eval 21 covers the always-fail no-fixer path; eval 7 covers retry-succeeds.
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 0
File: :0-0
Timestamp: 2026-04-19T22:28:43.115Z
Learning: In `skills/commit-push/SKILL.md`, the YAML frontmatter `description` field is intentionally dense (~400 chars) because it is the primary signal read by skill-matcher models when deciding whether to invoke `/commit-push` for a user utterance. Each phrase covers a distinct invocation pattern (e.g., "auto-detect and multi-commit splits", "--single/-1", "as one commit", "file args", "--select", "secret-file filtering"). Trimming for human readability would reduce trigger recall. This density is a deliberate design choice, not a documentation defect.
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:29-33
Timestamp: 2026-04-19T12:13:30.522Z
Learning: In `skills/commit-push/SKILL.md`, the secret filter (step 2b) is an absolute safety invariant: it cannot be bypassed by any flag (`--single`, `-1`, `--select`, or explicit `file` args). Users who hit a false positive have three workarounds: (a) rename the file so it no longer matches the pattern, (b) adjust the filter rules in the skill, or (c) commit the file manually outside the skill (`git add` + `git commit` + `git push`). Re-invoking `/commit-push` with `--select` or explicit `file` args will NOT re-include a filtered file. This is enforced by evals 9 and 18 in `skills/commit-push/evals/evals.json`.
📚 Learning: 2026-04-19T22:41:52.404Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:82-93
Timestamp: 2026-04-19T22:41:52.404Z
Learning: In `skills/commit-push/SKILL.md` (PR `#26`), the hook-failure policy in Step 5.4 was tightened in commit 5a27de3: (a) fixer is determined from hook stderr + file types with concrete examples (prettier, ruff/black, gofmt, markdownlint-cli, shfmt); checks with no auto-fix go directly to abort; (b) bounded retry of ≤2 fixer-driven retries (≤3 total git commit attempts per iteration — two retries because eslint --fix can need multiple passes for cascading autofixes); (c) on exhausted retries OR no determinable fixer, the group is marked FAILED and the loop aborts — do NOT attempt remaining groups, do NOT reconcile, do NOT push. Abort was chosen over skip to preserve the atomic-batch invariant (consistent with the secret filter's design philosophy). Steps 6, 7, and 8 all condition on the abort flag. Eval 21 covers the always-fail no-fixer path; eval 7 covers retry-succeeds.
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
📚 Learning: 2026-04-19T12:13:30.522Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 26
File: skills/commit-push/SKILL.md:29-33
Timestamp: 2026-04-19T12:13:30.522Z
Learning: In `skills/commit-push/SKILL.md`, the secret filter (step 2b) is an absolute safety invariant: it cannot be bypassed by any flag (`--single`, `-1`, `--select`, or explicit `file` args). Users who hit a false positive have three workarounds: (a) rename the file so it no longer matches the pattern, (b) adjust the filter rules in the skill, or (c) commit the file manually outside the skill (`git add` + `git commit` + `git push`). Re-invoking `/commit-push` with `--select` or explicit `file` args will NOT re-include a filtered file. This is enforced by evals 9 and 18 in `skills/commit-push/evals/evals.json`.
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
📚 Learning: 2026-04-19T22:28:43.115Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 0
File: :0-0
Timestamp: 2026-04-19T22:28:43.115Z
Learning: In `skills/commit-push/SKILL.md`, the YAML frontmatter `description` field is intentionally dense (~400 chars) because it is the primary signal read by skill-matcher models when deciding whether to invoke `/commit-push` for a user utterance. Each phrase covers a distinct invocation pattern (e.g., "auto-detect and multi-commit splits", "--single/-1", "as one commit", "file args", "--select", "secret-file filtering"). Trimming for human readability would reduce trigger recall. This density is a deliberate design choice, not a documentation defect.
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Applies to hooks/claude-notify/README.md : Keep `README.md` synchronized with behavior, environment variables, and test changes in hooks/claude-notify
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
📚 Learning: 2026-03-20T08:18:17.492Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 17
File: skills/spring-init/SKILL.md:74-89
Timestamp: 2026-03-20T08:18:17.492Z
Learning: For the `skills/spring-init/SKILL.md` skill in the `giwankim/my-claude-code` repo: Spring Boot version options listed in Step 3 (`4.0.4`, `3.5.12`, `4.0.5-SNAPSHOT`, `4.1.0-M3`, `4.1.0-SNAPSHOT`) are intentionally hardcoded as a convenience defaults prompt. Always verify Spring Boot milestone version availability against the live Spring Initializr API or the Spring Boot GitHub wiki release notes rather than relying on search results, which can be stale. The skill fetches the live dependency catalog at runtime via `spring init --list`, which is the primary freshness mechanism.
Applied to files:
skills/commit-push/evals/evals.json
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Apply instructions in `AGENTS.md` only within `hooks/claude-notify/` directory, not repository-wide
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
📚 Learning: 2026-02-23T12:48:28.418Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: hooks/claude-notify/AGENTS.md:0-0
Timestamp: 2026-02-23T12:48:28.418Z
Learning: Applies to hooks/claude-notify/Tests/required-cases.txt : List every test scenario in `Tests/required-cases.txt` (Case-ID gate)
Applied to files:
skills/commit-push/evals/evals.jsonskills/commit-push/SKILL.md
🪛 LanguageTool
skills/commit-push/SKILL.md
[uncategorized] ~44-~44: The official name of this software platform is spelled with a capital “H”.
Context: ... 2. CI/infra directory: files under .github/... group as scope ci. This rule fir...
(GITHUB)
[uncategorized] ~44-~44: The official name of this software platform is spelled with a capital “H”.
Context: ...neric top-level-directory rule below so .github doesn't end up as a literal directory ...
(GITHUB)
[uncategorized] ~48-~48: The official name of this software platform is spelled with a capital “H”.
Context: ...olved scope name). Same for ci (e.g., .github/workflows/test.yml from rule 3.2 plus ...
(GITHUB)
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...1 group → silently jump to step 5. - If 2+ groups → continue to step 4. 4. **P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~93-~93: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...wfor Go,markdownlint-cli --fixfor markdown,shfmt -w` for shell. If the hook is a...
(MARKDOWN_NNP)
[style] ~95-~95: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ps. Do NOT run step 6 (reconciliation). Do NOT run step 7 (git push). Earlier su...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.0)
skills/commit-push/SKILL.md
[warning] 99-99: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 100-100: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 101-101: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 102-102: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 103-103: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 103-103: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 106-106: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 107-107: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 108-108: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 113-113: Spaces inside code span elements
(MD038, no-space-in-code)
|
@CodeRabbit pause |
✅ Actions performedReviews paused. |



Adds an auto-detect phase to the
commit-pushskill that groups changed files by inferred Angular scope (skill subdir → top-level dir → root-files bucket) and proposes a multi-commit split via a 3-optionAskUserQuestionwhen 2+ groups are detected, replacing the prior single-commit-per-invocation behavior. Users can override with--single/-1or natural-language phrases like "commit as one"; the override is acknowledged in the reply, and the per-commit loop creates NEW commits even after pre-commit hook failures (never--amend) so earlier iterations are preserved. The skill now reports all SHAs and warns when the proposed split exceeds 5 groups. Also addsskills/commit-push/evals/evals.jsonwith 7 scenarios (single-group default, two-group split, both override mechanisms,--selectno-regression, cap warning, mid-split hook failure); iteration-1 grading on 4 smoke scenarios shows 100% pass rate vs 73% on the snapshot of the prior skill. Existing--selectinteractive picker and explicit@filearg flows are preserved with no regression.