feat: numeric-id positional heuristic for PR commands (bb pr 42 from inside a checkout)

[daniel-pittman]

What this fixes

From inside a Bitbucket git checkout, bb pr 42 (or bb pr-merge 42, bb pr-diff 42, etc.) used to treat 42 as the repo slug, error with Usage: bb pr-view [repo] <pr-id>, and the user had to type the redundant bb pr workspace/repo 42 even when sitting in that exact checkout. The natural one-arg invocation now Just Works.

Approach

Same gotcha class as the v1.2.0 state-recognition heuristic for bb prs OPEN. Same fix shape.

New helper _resolve_pr_args centralizes the heuristic: if $1 is purely digits, treat it as the id (auto-detect repo from git); otherwise the existing positional form ([repo] <id>) is preserved unchanged. Sets caller-scope repo / pr_id / pr_args_consumed so callers with trailing args (cmd_pr_merge strategy, cmd_pr_comment_add body) shift $pr_args_consumed to reach extras uniformly.

Applied to all 8 PR-id commands:

• cmd_pr_view, cmd_pr_diff, cmd_pr_comments
• cmd_pr_approve, cmd_pr_unapprove, cmd_pr_decline
• cmd_pr_merge (uses shift $pr_args_consumed for the strategy slot)
• cmd_pr_comment_add (uses shift $pr_args_consumed for the body slot)

The MCP / Python side is unaffected because its tools take named kwargs (pr_show(pr_id=42, repo="")), not positionals, so this gotcha can't fire there.

Edge case (accepted)

A repo literally named with pure digits (slug 42) would be shadowed by the heuristic. Acceptable: Bitbucket slugs by convention are lowercase-hyphenated, pure-digit slugs are vanishingly rare, and the explicit workspace/42 <id> form is the escape hatch (the heuristic only triggers on the bare-digits form).

End-to-end verification (against daniel-pittman/bb-cli-test PR #2)

$ bb pr 2                          # new form: auto-detect, show PR
PR #2 - test: numeric-id heuristic
  State: OPEN  ...

$ bb pr-diff 2                     # new form: show diff
diff --git a/NOTES2.md b/NOTES2.md ...

$ bb pr daniel-pittman/bb-cli-test 2   # old form: still works
PR #2 - test: numeric-id heuristic ...

$ bb pr-merge 2                    # new form + v1.4.3 POST fix
Merged PR #2 (merge_commit)
# state=MERGED, merge_commit=46945c6827bb confirmed via API

$ bb pr                            # usage error still fires, message updated
Usage: bb pr-view [repo] <pr-id>   (or: bb pr <id> from inside a checkout)

Tests

• bash side: smoke-tested as above (no unit harness on bash — matches the repo's convention).
• Python side: 565 pytest tests pass (unchanged; no Python touched).

Docs

cmd_help NOTES section gets a one-paragraph callout describing the heuristic so it's discoverable from bb help. cmd_pr_view usage error mentions the new short form.

Release

Bundle with the v1.4.3 patch already on develop (PR #39 / commit 138c236 — the bb pr-merge POST fix). v1.4.3 carries both fixes.

[claude]

Claude finished @daniel-pittman's task in 4m 20s —— View job

---

Code Review — feat: numeric-id positional heuristic for PR commands

• Read diff against origin/develop
• Analyze _resolve_pr_args heuristic + all 8 call sites
• Trace set -euo pipefail interaction with shift $pr_args_consumed
• Factor in Semgrep (clean) + CI Tests (green) comments
• Post review

---

This is a clean, well-scoped change: the heuristic is centralized in one helper, documented with the tradeoff spelled out, and applied consistently to all 8 PR-id commands (view, diff, comments, approve, unapprove, decline, merge, comment). pr_create (no id) and pr_list (state heuristic) are correctly left alone. The Python side is genuinely unaffected — its tools take named kwargs, so the positional gotcha can't fire there. The pure-digit-slug shadowing tradeoff is real and the workspace/slug <id> escape hatch is the right call.

I found one behavioral regression worth fixing before merge.

🟠 MEDIUM — shift runs before the usage check → silent abort on missing id (merge / comment)

In the two commands that consume trailing args, the shift happens before the empty-pr_id check:

cmd_pr_merge() {
    local repo pr_id pr_args_consumed
    _resolve_pr_args "$@"
    shift $pr_args_consumed     # <-- line 1154, runs first
    local strategy="${1:-merge_commit}"

    if [[ -z "$pr_id" ]]; then  # <-- line 1157, too late
        echo "Usage: bb pr-merge [repo] <pr-id> [strategy]" >&2
        exit 1
    fi

When a user supplies a single non-numeric arg — i.e. a repo without a PR id, e.g. bb pr-merge myrepo or bb pr-merge ws/repo — _resolve_pr_args takes the *[!0-9]* branch and sets pr_args_consumed=2, but only 1 positional exists. shift 2 with $# == 1 returns non-zero, and under the script's set -euo pipefail (line 21) that aborts the script at the shift — before the friendly Usage: message can print. The user gets a silent exit code 1 where they previously got a clear usage hint.

cmd_pr_comment_add (line 1262) has the identical ordering and the same regression (bb pr-comment myrepo).

This is on a user-error path (id forgotten), not a common success path — hence MEDIUM, non-blocking — but it's a real UX regression versus pre-PR behavior, and the smoke tests in the PR description only exercised the happy paths (bb pr-merge 2), so it wasn't caught.

Fix: move the empty-pr_id check above the shift in both functions. Once pr_id is known non-empty, the consumed args are guaranteed present, so the shift is always safe:

    _resolve_pr_args "$@"
    if [[ -z "$pr_id" ]]; then
        echo "Usage: bb pr-merge [repo] <pr-id> [strategy]" >&2
        echo "  Strategies: merge_commit (default), squash, fast_forward" >&2
        exit 1
    fi
    shift $pr_args_consumed
    local strategy="${1:-merge_commit}"

Fix this →

Nits (non-blocking)

• The 6 non-shifting commands (view, diff, comments, approve, unapprove, decline) set pr_args_consumed but never read it. Harmless — it's the cost of a shared helper, and consistency reads fine. No change needed.

Test coverage

No automated test changes needed: the diff is bash-only, and the repo has no bash unit-test harness by design (CLAUDE.md / CONTRIBUTING note bash is smoke-tested manually; the bash32-floor CI job only parses for syntax). Python is untouched, and the CI Tests comment confirms the full pytest suite (3.10/3.11/3.12) plus bash syntax checks are green on 2624804b0. Semgrep (OSS) scan is clean.

That said, the heuristic adds genuinely new branching, and the one path the author's manual smoke test didn't cover (single non-numeric arg → missing id) is exactly where the MEDIUM regression lives. Worth a manual bb pr-merge myrepo / bb pr-comment myrepo smoke check after the fix.

Verdict

Solid change with good parity reasoning. One non-blocking MEDIUM (reorder the usage check above shift in cmd_pr_merge and cmd_pr_comment_add) and you're good.
· feat/pr-numeric-id-heuristic

[github-actions]

Semgrep (OSS): no findings

Clean scan. Posted before the Claude review.

[github-actions]

CI Tests

✅ Tests passed: the pytest suite across Python 3.10/3.11/3.12 (and bash syntax checks) is green on 63920308c.

Posted before the Claude review so the test outcome is folded into the review.

[claude]

Claude finished @daniel-pittman's task in 1m 44s —— View job

---

Code Review — feat: numeric-id positional heuristic for PR commands (re-review after fix)

• Read diff against origin/develop
• Verify the fix commit 6392030 addresses the prior MEDIUM
• Re-trace shift $pr_args_consumed under set -euo pipefail for all arg shapes
• Factor in Semgrep (clean) + CI Tests (green on 63920308c)
• Post review

---

This synchronize push adds commit 6392030, which addresses the MEDIUM I flagged last round (usage check ordering vs. shift). The fix is correct and complete. Re-tracing both consume-trailing-args commands across every arg shape:

cmd_pr_merge (bb:1151-1165) — the empty-pr_id check (line 1159) now runs before shift $pr_args_consumed (line 1164). Once pr_id is known non-empty, the consumed positionals are guaranteed present, so the shift can never fail:

• bb pr-merge myrepo → pr_id="" → friendly usage prints, exit 1 (was: silent abort). ✅ regression fixed
• bb pr-merge 42 → digit branch, consumed=1, shift 1 safe, strategy=merge_commit. ✅
• bb pr-merge 42 squash → consumed=1, shift 1, strategy=squash. ✅
• bb pr-merge ws/repo 42 squash → repo branch, consumed=2, shift 2, strategy=squash. ✅

cmd_pr_comment_add (bb:1263-1283) — same ordering: pr_id check (1276) precedes shift (1279), and the body emptiness check happens after the shift. All shapes hold (bb pr-comment myrepo, bb pr-comment 42, bb pr-comment 42 'hi', bb pr-comment ws/repo 42 'hi'). ✅ The added comments on both functions explain why the ordering matters, which is exactly the kind of footgun worth a comment.

No new findings. The heuristic itself is unchanged from the prior review and remains sound — centralized in _resolve_pr_args, applied to all 8 PR-id commands, with pr_create/pr_list correctly left alone, and the Python side genuinely unaffected (named kwargs).

Nits (non-blocking, no change required)

• Nested function leaks to global scope — _usage_pr_comment (bb:1269) is defined inside cmd_pr_comment_add. Bash functions aren't lexically scoped, so this leaks into the global namespace and is redefined on each call. Harmless (it's only ever called from within the same function), and it neatly avoids duplicating the 5-line usage block. Fine as-is; flagging only for awareness.
• The 6 non-shifting commands (view, diff, comments, approve, unapprove, decline) still set pr_args_consumed without reading it — the cost of a shared helper, and consistency reads fine. No change needed (carried over from prior review).

Test coverage

No test changes needed: the diff is bash-only, and the repo has no bash unit-test harness by design (CLAUDE.md/CONTRIBUTING note bash is smoke-tested manually; the bash32-floor CI job only parses for syntax). Python is untouched. The CI Tests comment confirms pytest (3.10/3.11/3.12) plus bash syntax checks are green on 63920308c, and the Semgrep (OSS) scan is clean. The single path the original smoke test missed (single non-numeric arg → missing id) is precisely what this fix commit closes; a manual bb pr-merge myrepo / bb pr-comment myrepo confirms the friendly usage now fires.

Verdict

The prior MEDIUM is resolved correctly. No blocking findings. 👍
· feat/pr-numeric-id-heuristic

[github-actions]

Review Gate

✅ Gate: PASS — Review reported no HIGH/CRITICAL findings.

Observe-only until review-gate is added to the branch's required status checks.