fix: don't expand numeric args that are flag values or branch names

[mroth]

Note

🤖 LLM Usage: I had previously attempted to tackle this years ago in #70 and got frustrated with the level of undocumented edge case tedium (see my comments in #69). With modern LLM models, it's actually a great use case for just tasking an agent loose in a container to exhaustively test different permutations and see their behavior. Thus the skipExpansion list in this PR is the results of Opus 4.6 spending a half hour trying all the flag combinations it could find from git help. I have verified the code produced and the test methodology, but additional human review on this one may be warranted. I'm still a little "meh" about introducing this level of git UI edge case parsing into scmpuff itself, so thoughts are welcome!

scmpuff exec blindly expands any bare integer to a $eN file shortcut.
This breaks commands where the integer is a flag value (git log -n 1
becomes git log -n <filepath>) or a branch name (git checkout -b 713
becomes git checkout -b <filepath>, or empty string if unset).

Add a skipExpansion check in Expand() that recognizes when an integer
token immediately follows a flag that expects a value. This is gated on
args[0] matching $SCMPUFF_GIT_CMD so it only activates for commands
routed through the shell wrapper. The flag allowlist is per-subcommand:

  log:      -n, --max-count, --skip, --min-parents, --max-parents
  checkout: -b, -B, --orphan
  blame:    -L
  rebase:   -C

Only flags that accept a space-separated numeric value on subcommands
intercepted by the shell wrapper need coverage. Glued forms like -n1
are already safe because the combined token doesn't match the digit
regex. Same-named flags on other subcommands (e.g. git rm -n which is
--dry-run) are intentionally not listed, so their following args still
expand normally.

Fixes #69. Fixes #81. Supersedes #70.

[Copilot]

Pull request overview

This PR fixes scmpuff exec numeric shortcut expansion for Git commands where a bare integer token is actually a flag value (e.g. git log -n 1) or an argument like a numeric branch name (e.g. git checkout -b 713), preventing accidental expansion into $eN placeholders.

Changes:

• Add skipExpansion logic in arguments.Expand to avoid expanding tokens that are values for a small allowlist of Git flags (scoped to commands where args[0] == SCMPUFF_GIT_CMD).
• Add Go unit tests covering protected numeric flag values and numeric branch names.
• Add a testscript .txtar integration test validating scmpuff expand output for these scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
internal/arguments/arguments.go	Adds skipExpansion and wires it into Expand to prevent incorrect numeric expansion for specific Git flag-value positions.
internal/arguments/arguments_test.go	Adds table-driven tests to ensure numeric flag values/branch names remain literal while still expanding true file shortcuts and ranges.
internal/cmd/testdata/script/command-expand-numeric-flags.txtar	Adds integration coverage verifying CLI-level expansion behavior via scmpuff expand.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mroth]

In the interest of soundness, I had Codex 5.3 (OpenAI) cross-review this PR, as the original implementation was produced by Claude Opus 4.6 (Anthropic). The review found some gaps. Below is the full review report from Codex, followed by the response plan Opus derived from it.

---

Codex 5.3 Review Report

Review of Fix Logic in PR #145

Scope and method

I reviewed PR #145 (fix: don't expand numeric args that are flag values or branch names) in detail, including:

• the full diff and new tests in the PR,
• linked issues git log -n 1 doesn't seem to work #69 and git checkout fails if trying to use raw branch name that is all digits #81 (plus prior superseded PR fix(arguments): handle numeric single flag edge cases in git porcelain #70 context),
• shell-wrapper dispatch behavior in docs/shell-integration.md and wrapper scripts,
• Git command help/behavior checks, and command execution in fresh temporary repos under /tmp.

I also validated command parsing behavior directly with Git 2.53.0 in clean /tmp repos to verify where Git accepts space-separated values and where it does not.

Root cause (confirmed)

The underlying bug is real and correctly identified:

• arguments.Expand() currently expands any bare numeric token (regex ^[0-9]{0,4}$) into $eN.
• Expansion happens before environment substitution.
• If $eN is unset, the token becomes empty (""), which corrupts command arguments.
• If $eN is set, the token becomes a path, which is also wrong when the token was meant as a flag value or branch name.

This directly explains:

• git log -n 1 (issue git log -n 1 doesn't seem to work #69),
• git checkout -b 713 (issue git checkout fails if trying to use raw branch name that is all digits #81).

What PR #145 gets right

The fix is directionally sound and addresses the reported regressions:

1. Adds a targeted skipExpansion(args, pos, gitCmd) check in Expand().
2. Applies only when command routing came through the shell wrapper (args[0] == $SCMPUFF_GIT_CMD).
3. Uses subcommand-scoped allowlists to avoid false positives like git rm -n (dry-run) where -n does not take a value.
4. Adds both unit coverage and script-level command expansion coverage for core reported scenarios.

This is a good and pragmatic improvement over unconditional numeric expansion.

Soundness assessment

Strong points

• Correctly fixes both known user-facing bugs.
• Keeps expansion behavior for true shortcut operands after protected values (e.g. git log -n 1 2 still expands 2).
• Avoids broad suppression that would break normal shortcut usage.

Limitations in current logic

The fix remains incomplete for the generalized class of bug ("numeric token that is actually a flag value / ref argument"), because coverage is only a small hand-picked subset.

The wrapper intercepts these subcommands:

• commit, blame, log, rebase, merge, checkout, diff, rm, reset, restore, add

But PR #145 only protects:

• log: -n, --max-count, --skip, --min-parents, --max-parents
• checkout: -b, -B, --orphan
• blame: -L
• rebase: -C

Rule-by-rule verdict (pointed)

Rules in PR #145 that are correct/necessary

These are good additions and should stay:

• log: -n, --max-count, --skip
• checkout: -b, -B, --orphan
• blame: -L
• rebase: -C

Rules in PR #145 that appear unnecessary (on tested Git 2.53.0)

These do not appear to be accepted in space-separated form in current Git, so skip rules are likely dead/harmless coverage rather than needed fixes:

• log: --min-parents
• log: --max-parents

Evidence from fresh /tmp repo runs:

• git log --min-parents 0 -n 1 fails (ambiguous argument '0'), while --min-parents=0 works.
• git log --max-parents 1 -n 1 fails (ambiguous argument '1'), while --max-parents=1 works.

So I would classify these as unnecessary today, not "wrong" in a harmful sense.

Rules definitely missing (and currently still broken)

Using the PR logic exactly as written, these still mis-expand numeric values:

• git commit -m 123 → 123 becomes $e123 (wrong; message text).
• git merge -m 123 topic → 123 becomes $e123 (wrong; message text).
• git log --grep 1 → 1 becomes $e1 (wrong; grep pattern).
• git checkout -b new 713 → 713 becomes $e713 (wrong; start-point ref).
• git rebase --onto 713 main topic → 713 becomes $e713 (wrong; onto ref).

Still unresolved (likely intentional, but worth explicitly documenting):

• git checkout 713 (positional branch/ref) remains ambiguous and still expands as shortcut today.

Git help/docs + behavior findings from /tmp testing

In fresh /tmp repos with Git 2.53.0, I verified:

• Valid and accepted with space-separated values:

  • git log -n 1
  • git log --max-count 1
  • git log --skip 1
  • git commit --allow-empty -m 123
  • git merge --no-ff -m 123 topic
  • git rebase -C 3 HEAD~1

• Important nuance:

  • git log --min-parents 0 and --max-parents 1 (space form) were not accepted in my Git version; = forms were accepted.
  • git merge --log 1 treated 1 as merge target; --log=1 worked.

This means some entries in the PR allowlist may be harmless over-coverage for current Git, while other real value-taking flags are still unprotected.

Test coverage review

PR tests are good for listed cases, but currently miss:

• value-taking flags on other intercepted commands (notably commit/merge/rebase/checkout variants),
• multi-value-option scenarios (e.g., checkout -b <branch> <start-point> where second value can be numeric),
• explicit documentation/tests of known non-goals (e.g. positional numeric checkout refs if intentionally unsupported).

Overall verdict

PR #145 is a meaningful, correct partial fix for #69 and #81, but it is not yet a complete fix for the broader logic class it targets.

I would rate it as:

• Safe to merge as a targeted bugfix for the two reported issues,
• but incomplete for general "numeric flag/ref values" handling across all wrapped commands.

Pointed recommendation

If you want a minimal, high-confidence follow-up to PR #145:

1. Keep these existing rules: log(-n,--max-count,--skip), checkout(-b,-B,--orphan), blame(-L), rebase(-C).
2. Optional cleanup: remove log(--min-parents,--max-parents) from the skip list (or keep with a comment saying "legacy/defensive").
3. Add missing protections now:
   • commit: -m, --message
   • merge: -m, --message
   • log: --grep
   • rebase: --onto
   • checkout: protect the argument immediately after a -b/-B/--orphan value when used as <start-point> (e.g. git checkout -b new 713).
4. Add regression tests for each missing case above.

If you intentionally do not want to support ambiguous positional refs (e.g. git checkout 713), document that explicitly as out-of-scope.

---

Claude Opus 4.6 Response Plan

Address PR #145 review findings

Context

Codex 5.3 reviewed PR #145 and found:

1. Two flags (--min-parents, --max-parents) that don't actually accept space-separated values on current Git — dead entries
2. Several missing flag protections: commit/merge -m, log --grep, rebase --onto
3. A positional edge case (checkout -b <branch> <start-point>) that's out of scope for prev-arg matching

Changes to internal/arguments/arguments.go

Remove from skipExpansion:

• log: --min-parents, --max-parents — not accepted in space-separated form

Add to skipExpansion:

• commit: -m, --message
• merge: -m, --message
• log: --grep
• rebase: --onto

Decisions on review recommendations:

• --min-parents/--max-parents: Remove. Keep the allowlist honest — only list flags verified to accept space-separated values.
• commit/merge -m, log --grep, rebase --onto: Add all of these. The full list is small and avoids future reports.
• checkout -b <branch> <start-point>: Out of scope. The simple prev-arg approach can't handle positional arguments cleanly, and this would require deeper tracking. Documented as a known limitation in the skipExpansion doc comment.
• Positional git checkout 713: Ambiguous by nature (could be a branch name or a file shortcut) — intentionally left as-is.

Verification

1. go test ./internal/arguments/ — unit tests
2. go test ./internal/cmd/ -run TestScripts/command-expand-numeric-flags — integration test
3. go_diagnostics on modified files

[mroth]

Followup cross-analysis from Opus 4.6, independently verifying the Codex findings and testing additional flags not covered in the original review:

Codex findings — all confirmed:

• --min-parents/--max-parents space form fails — confirmed, we correctly removed these
• commit -m 123 / commit --message 456 — confirmed broken, we correctly added protection
• merge -m 789 — confirmed broken, we correctly added protection
• log --grep 123 — confirmed broken, we correctly added protection
• rebase --onto — confirmed broken, we correctly added protection

Additional flags I tested that Codex didn't mention:

Flag	Space form works?	Risk?
log --author 1	Yes	Very low — author is almost never a bare number
log --format 1	No (equals only)	Not a problem
log -S 1 (pickaxe)	Yes	Low — searching for the string "1" is unusual but possible
log -G 1 (regex pickaxe)	Yes	Same as above
commit -C HEAD	Yes (takes ref)	Low — ref is rarely a bare number, and -C is uncommon
diff -U 3	No (glued only)	Not a problem
merge -s 1	Yes but fails (invalid strategy)	Not a problem — would error either way

The pickaxe flags (-S, -G) and --author technically accept space-separated values, but their arguments being a bare integer is extremely unlikely in practice. commit -C takes a ref which could theoretically be numeric, but it's the same class of edge case as positional refs.

My opinion: What we implemented covers all the realistic cases. The remaining flags (-S, -G, --author, -C) are so unlikely to receive bare numeric arguments that adding them would be over-engineering the allowlist. I wouldn't change anything from what we did.