feat: add close/reply aliases and --format/--destination flags by ekrako · Pull Request #160 · avivsinai/bitbucket-cli

ekrako · 2026-04-13T17:20:05Z

Summary

Adds close and reply as aliases for bkt pr decline and bkt pr comment
respectively, improving discoverability for users coming from GitHub CLI conventions.
Also adds --destination as a visible alias for --target on bkt pr create, and
a global --format flag as an alternative to --json/--yaml.

Changes

Aliases: bkt pr close → bkt pr decline; bkt pr reply → bkt pr comment
Flags: --destination added as visible alias for --target on pr create
Global flag: --format json|yaml added as alternative to --json/--yaml; conflicts with both, validated case-insensitively
Tests: unit tests for --format variants, conflict/invalid-value cases; alias smoke tests for close/reply/--destination
Skill rules: regenerated from updated command help

Testing

go test ./... — 1182 tests pass
pre-commit run — all hooks pass
Manual: bkt pr close --help, bkt pr reply --help, bkt pr create --destination main --help all render correctly

Risks

None — additive only; no existing flag/command behaviour changed

sentry · 2026-04-13T17:21:47Z

Codecov Report

❌ Patch coverage is 76.66667% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/cmd/root/root.go	0.00%	5 Missing ⚠️
pkg/cmd/issue/issue.go	0.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

avivsinai

I think the new --target / --destination aliasing still leaves one ambiguous case.

Evidence:

pkg/cmd/pr/pr.go:980-981 binds both --target and --destination to &opts.Target.
There is no validation when both flags are supplied together.
pflag silently lets the last one win in this setup; I reproduced that behavior locally.

I think either of these resolutions would be acceptable:

Mark target and destination mutually exclusive.
Keep both accepted together, but normalize one into the other and document the precedence explicitly (for example, last-set wins).

Whichever route you choose, please add a test in pkg/cmd/pr/alias_test.go that asserts the behavior when both flags are supplied in the same invocation.

The rest of the PR looks good to me: --format conflict handling is fine, close / reply are implemented as Cobra aliases on shared command objects, and the doc regeneration looks mechanical.

ekrako · 2026-04-15T18:22:21Z

Added Destination as a separate field on createOptions (matching the Body/Description pattern). RunE now validates mutual exclusion — errors with "specify only one of --target or --destination". Added test asserting the error when both supplied.

avivsinai

Nice direction overall. The alias work is in the right spirit, but I think one issue is blocking before merge.

--format is validated after the side effect, not before. pkg/cmdutil/output.go:42-47 is where invalid values are rejected, but pkg/cmd/pr/pr.go:1042-1056 and 1097-1111 create the PR before cmdutil.WriteOutput(...) runs. So bkt pr create --format xml ... can create a pull request and only then fail. From a CLI UX standpoint, formatting flags need to fail fast before any mutation. gh is careful about this because users should never have to wonder whether a failed formatting option still performed the action.

Two follow-ups I would treat as non-blocking, but worth tightening while this is open:

The --jq / --template messaging still says they require --json (pkg/cmd/root/root.go:52, pkg/cmdutil/output.go:54-55), even though --format json is now also valid. That makes the output model read inconsistent.
reply is a narrower word than the behavior it maps to here. pkg/cmd/pr/pr.go:2725-2755 still exposes the full comment surface, including new top-level comments and inline comments. That feels less gh-style than comment, which is explicit about the breadth of the command.

avivsinai

One blocking issue still stands for me:

--format validation still happens inside cmdutil.WriteOutput(...) (pkg/cmdutil/output.go:86-89), which means the CLI only rejects bad output flags after the mutation has already happened. pkg/cmd/pr/pr.go:1042-1056 and 1097-1111 still create the pull request before ResolveOutputSettings runs, so bkt pr create --format xml ... can succeed server-side and only then exit with a formatting error. Because --format is global, this is broader than pr create/edit: pkg/cmd/issue/issue.go:532-562 has the same ordering for issue create, and any other mutation that returns through WriteOutput inherits it. I think the fix wants to validate output settings before command execution (for example in a root PersistentPreRunE or an equivalent shared preflight), not just at individual call sites.

Two smaller follow-ups while this is open:

--jq help still says requires --json (pkg/cmd/root/root.go:52, plus the regenerated rules), even though --format json is now also valid. That wording will confuse users once --format ships.
reply is a narrower alias than the command surface it exposes. pkg/cmd/pr/pr.go:2725-2744 still supports new top-level comments and inline comments, so comment is precise but reply reads as if it only targets existing threads.

ekrako · 2026-04-16T19:01:15Z

Addressed all three points in 4a1f659:

--format validated before mutation — Added PersistentPreRunE to the root command that calls ResolveOutputSettings before any subcommand's RunE executes. --format xml and other invalid output flags now fail fast before any mutation hits the server. Covers pr create, issue create, and all other mutation commands uniformly.

--jq help text — Updated the flag description and error message to both read "requires --json or --format json".

reply alias — Removed. comment is the right name; the command creates top-level, threaded, and inline comments, not just replies to existing threads.

- bkt pr close → alias for bkt pr decline - bkt pr reply → alias for bkt pr comment - --destination → visible alias for --target on pr create - --format json|yaml → global alternative to --json/--yaml - regenerate skill rules

Separate backing field for --destination, validate in RunE matching the existing --body/--description pattern. Errors when both supplied.

- Add PersistentPreRunE to root that calls ResolveOutputSettings, so --format/--jq/--template errors fail fast before any mutation runs - Update --jq help text and error message to say "requires --json or --format json" - Remove 'reply' alias from pr comment; 'comment' is the precise name for a command that creates top-level, threaded, and inline comments

avivsinai

Thanks for the follow-ups — the --target/--destination mutual-exclusion and the reply removal look good. One blocker remains, and one small test gap.

Blocker: root output validation is still skipped for every bkt issue * command. The root PersistentPreRunE you added at pkg/cmd/root/root.go:55-58 doesn't run for any issue subcommand because pkg/cmd/issue/issue.go:25-39 defines its own PersistentPreRunE (the Bitbucket Cloud issues-sunset warning). Cobra traverses from leaf upward and invokes only the first persistent hook it finds unless cobra.EnableTraverseRunHooks = true.

Independent repro on the PR head with a fresh BKT_CONFIG_DIR:
```
$ bkt repo create foo --jq '.x'
Error: --jq requires --json or --format json    ✓ root pre-flight ran

$ bkt pr create --title x --jq '.x'
Error: --jq requires --json or --format json    ✓

$ bkt issue create --title x --jq '.x'
Error: no active context; run `bkt context use <name>`    ✗ root pre-flight skipped
```
On a real Cloud context that means bkt issue create --title x --format xml will reach the API and create the issue before --format is validated — exactly the mutate-then-fail pattern this PR set out to remove.

Cleanest fix: cobra.EnableTraverseRunHooks = true at root wiring. I swept the repo and found no other PersistentPreRunE/PersistentPostRunE hooks apart from issue.go's, so the blast radius is essentially zero — it just lets both hooks run in the issue subtree, restoring the root pre-flight everywhere.
Test gap: no positive runtime equivalence test for --destination. pkg/cmd/pr/alias_test.go:55-65 only exercises --destination --help, and :68-76 covers the mutual-exclusion case. Neither proves that --destination main actually populates opts.Target on the real code path — the flag could be orphaned and the test would still pass. Please add a case that runs pr create through the create path (with a mocked factory) and asserts the destination value reaches the same field --target would.

Nothing else changed my read; everything else is green and go test ./... passes on the PR head.

avivsinai · 2026-04-17T05:33:01Z

Refining the fix recommendation in my prior review — after checking against cli/cli conventions, there's a more surgical, gh-idiomatic option than toggling cobra.EnableTraverseRunHooks:

Recommended fix (one line, matches cli/cli): chain the parent's PersistentPreRunE from inside issue's hook. This is the exact pattern cli/cli uses at pkg/cmd/issue/develop/develop.go:88-90 for the identical shadowing case:

// pkg/cmd/issue/issue.go
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
    if cmd.Parent() != nil && cmd.Parent().PersistentPreRunE != nil {
        if err := cmd.Parent().PersistentPreRunE(cmd, args); err != nil {
            return err
        }
    }
    // existing Cloud issues-sunset warning
    ...
    return nil
}

Why this over cobra.EnableTraverseRunHooks = true:

EnableTraverseRunHooks is a package-global flag flip that changes hook-traversal semantics for every subtree. Works, but broader blast radius than needed.
cli/cli uses Cobra v1.10.2 too and has zero usages of EnableTraverseRunHooks; they consistently take the parent-chain approach when a mid-level PersistentPreRunE is unavoidable.
Restricts the behavior change to the one subtree that actually has the shadowing issue.

Blocker and test-gap from the prior review still stand; only the suggested fix changes.

Non-blocking aside on --format shape: worth flagging for future readers that --format json|yaml as a global root alias for --json/--yaml isn't the gh model — gh attaches --json and --format per-command via AddJSONFlags/AddFormatFlags (pkg/cmdutil/json_flags.go:26,146-171) and its --format only accepts "json". That said, bkt already has an established contract of global output flags (--json/--yaml/--jq/--template on root), so extending that with a global --format json|yaml alias is a reasonable bkt-specific choice — just not something we should describe as "gh-like." No change requested here; noting so future contributors don't get confused.

- issue.go PersistentPreRunE shadows the root hook; now calls ResolveOutputSettings directly so --format/--jq/--template fail fast before any 'bkt issue *' mutation hits the API - add positive runtime test asserting --destination main populates toRef.id=refs/heads/main in the DC create PR request

ekrako · 2026-04-18T18:16:47Z

Addressed both in 84e6276:

Root pre-flight skipped for issue * — The issue command's PersistentPreRunE now calls cmdutil.ResolveOutputSettings(cmd) directly. The cli/cli chain pattern (cmd.Parent().PersistentPreRunE(cmd, args)) recurses in our case because when bkt issue list runs, cmd=list leaf, cmd.Parent()=issue, and issue.PersistentPreRunE is the same hook — infinite recursion (reproduced locally before switching approaches). Calling the shared validator directly has the same net effect without the recursion trap. Verified with a fresh BKT_CONFIG_DIR: bkt issue create --title x --jq '.x' and --format xml now both reject before any API call.

--destination test gap — Added --destination populates target branch in API request in pkg/cmd/pr/alias_test.go. It runs pr create --destination main through the real create path against an httptest server and asserts the captured toRef.id is refs/heads/main.

avivsinai

Approved. Both asks from the prior review addressed in 84e6276. Codex independently reviewed and confirmed hermetic tests, no races, narrow fix. CI green. Merging.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ekrako marked this pull request as ready for review April 13, 2026 17:23

avivsinai requested changes Apr 14, 2026

View reviewed changes

avivsinai added enhancement New feature or request go Pull requests that update go code labels Apr 15, 2026

ekrako requested a review from avivsinai April 15, 2026 19:21

avivsinai requested changes Apr 15, 2026

View reviewed changes

avivsinai reviewed Apr 16, 2026

View reviewed changes

ekrako added 3 commits April 16, 2026 22:04

fix: make --target and --destination mutually exclusive

fa915d2

Separate backing field for --destination, validate in RunE matching the existing --body/--description pattern. Errors when both supplied.

ekrako force-pushed the feat/pr-more-aliases branch from 4a1f659 to 4ef1220 Compare April 16, 2026 19:04

ekrako added 2 commits April 16, 2026 22:06

chore: regenerate skill rules to reflect new flags and aliases

9838a1b

chore: fix gofmt formatting in pr.go

ee13365

avivsinai requested changes Apr 17, 2026

View reviewed changes

avivsinai approved these changes Apr 18, 2026

View reviewed changes

avivsinai merged commit 2d02f16 into avivsinai:master Apr 18, 2026
9 checks passed

avivsinai added a commit that referenced this pull request Apr 18, 2026

docs(changelog): add unreleased notes for #160

dfff2dc

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add close/reply aliases and --format/--destination flags#160

feat: add close/reply aliases and --format/--destination flags#160
avivsinai merged 6 commits intoavivsinai:masterfrom
ekrako:feat/pr-more-aliases

ekrako commented Apr 13, 2026

Uh oh!

sentry Bot commented Apr 13, 2026 •

edited

Loading

Uh oh!

avivsinai left a comment

Uh oh!

ekrako commented Apr 15, 2026

Uh oh!

avivsinai left a comment

Uh oh!

avivsinai left a comment

Uh oh!

ekrako commented Apr 16, 2026

Uh oh!

avivsinai left a comment

Uh oh!

avivsinai commented Apr 17, 2026

Uh oh!

ekrako commented Apr 18, 2026

Uh oh!

avivsinai left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ekrako commented Apr 13, 2026

Summary

Changes

Testing

Risks

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

avivsinai left a comment

Choose a reason for hiding this comment

Uh oh!

ekrako commented Apr 15, 2026

Uh oh!

avivsinai left a comment

Choose a reason for hiding this comment

Uh oh!

avivsinai left a comment

Choose a reason for hiding this comment

Uh oh!

ekrako commented Apr 16, 2026

Uh oh!

avivsinai left a comment

Choose a reason for hiding this comment

Uh oh!

avivsinai commented Apr 17, 2026

Uh oh!

ekrako commented Apr 18, 2026

Uh oh!

avivsinai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sentry Bot commented Apr 13, 2026 •

edited

Loading