allod/tools
2
0
Fork 0
Code Issues 13 Pull requests Projects Releases Packages Wiki Activity Actions

Add forge pr close command #65

Merged
vnprc merged 6 commits from agent/pr-close into master 2026-06-23 19:26:26 +01:00
Conversation 3 Commits 6 Files changed 5 +263 -2
vnprc-agent commented 2026-06-23 14:44:12 +01:00
Contributor
Copy link

Closes #59

Adds forge pr close {<number> | <url> | <branch>} [-c <text>] [-d] [-R <owner/repo>]

  • Number, URL, or branch name as target (branch uses find-by-head lookup)
  • -c, --comment posts a closing comment before closing
  • -d, --delete-branch deletes the remote head branch after closing
  • URL parsing rejects foreign hosts and non-pulls paths
  • 26 pr-close tests + 4 validation tests, all existing tests still pass
Closes #59 Adds `forge pr close {<number> | <url> | <branch>} [-c <text>] [-d] [-R <owner/repo>]` - Number, URL, or branch name as target (branch uses find-by-head lookup) - `-c, --comment` posts a closing comment before closing - `-d, --delete-branch` deletes the remote head branch after closing - URL parsing rejects foreign hosts and non-pulls paths - 26 pr-close tests + 4 validation tests, all existing tests still pass
Implements #59: forge pr close {<number> | <url> | <branch>} with
-c/--comment, -d/--delete-branch, and -R/--repo flags.

Branch targets resolve via the existing find-by-head lookup.
URL targets extract owner/repo/number the same way issue close does.
vnprc-agent commented 2026-06-23 14:48:30 +01:00
Author
Contributor
Copy link

Code Review: PR #65 — forge pr close

Reviewed the full diff against master, the complete forge script for context, all test files, and the existing issue close implementation for consistency comparison.

What's done well

  • Consistent structure. pr_close follows the same arg-parsing pattern as every other subcommand — contains_help_flag, while-case loop, die on bad input, guard clause on missing target. Easy to read.
  • Reusable resolve_pr_target function. Clean separation between "figure out what PR we're talking about" and "do the thing." This will serve future commands (reopen, merge) without duplication.
  • URL validation is strict. The IFS=/ read decomposition with the extra check catches trailing path segments. The $resource == "pulls" check means issues/12 URLs correctly fail. Good.
  • jq for JSON construction. Comment bodies go through jq -n --arg, which handles all quoting. No injection there.
  • Thorough happy-path test coverage. Six distinct scenarios (plain, with comment, URL target, branch target, delete-branch, comment+delete-branch) plus the branch-not-found error path. The mock infrastructure is solid.
  • Validation tests added. Missing target, unknown option, foreign URL, wrong-resource URL — all covered.
  • Operation ordering is correct. Comment before close, close before branch delete. This matches gh pr close semantics.

Issues found

1. Branch name not URL-encoded in API query string — MAJOR

# resolve_pr_target, line 609
PR_NUMBER=$(api GET "/repos/$REPO/pulls?state=open&limit=50&head=${target}" \

The same pattern exists in pr_find_by_head (line 577), so this is a pre-existing issue, but pr close inherits it and should be noted. A branch named feature/foo&state=closed or containing %, +, #, or spaces will produce a malformed query string. The Forgejo API may misparse the parameters or return unexpected results.

The head= value should be percent-encoded before interpolation. A minimal fix:

local encoded_target
encoded_target=$(printf '%s' "$target" | jq -sRr @uri)

(Alternatively curl --data-urlencode in a GET, but that changes the api() call shape.)

The test suite only uses the branch name topic — a single ASCII word — so this is completely invisible to the current tests.

2. --delete-branch will delete any branch name the API returns, including master — MAJOR

head_ref=$(api GET "/repos/$REPO/pulls/$PR_NUMBER" | jq -r '.head.ref')
# ...later...
api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null

If someone accidentally runs forge pr close 42 -d on a PR whose head is master or main, this will delete the default branch. gh pr close --delete-branch refuses to delete the base branch. There is no guard here.

At minimum, fetch the base ref too and refuse if head_ref == base_ref:

local pr_json
pr_json=$(api GET "/repos/$REPO/pulls/$PR_NUMBER")
head_ref=$(echo "$pr_json" | jq -r '.head.ref')
base_ref=$(echo "$pr_json" | jq -r '.base.ref')
if [[ "$head_ref" == "$base_ref" ]]; then
  die "refusing to delete branch $head_ref: it is the PR base branch"
fi

3. No test for -R / --repo override with pr close — MINOR

Every other test uses the implicit -R acme/widget from run_ok. There is no test that pr close 12 -R acme/gadget actually targets the right repo, or that URL-based close overrides an earlier -R. The resolve_pr_target URL path sets REPO directly, but the interaction with a pre-set -R is untested.

4. head_ref containing / or special chars reaches DELETE URL unencoded — MINOR

api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null

Branch names like feature/close-button contain slashes. The Forgejo branch API endpoint uses {branch} as a path parameter and expects it URL-encoded (e.g., feature%2Fclose-button). The current code would produce /repos/acme/widget/branches/feature/close-button, which is a 404.

The mock only returns "ref":"topic" (no slashes), so this is invisible in tests.

Fix: head_ref=$(printf '%s' "$head_ref" | jq -sRr @uri)

5. No error handling if the PATCH (close) call fails but the comment already posted — MINOR

if [[ "$comment_set" == true ]]; then
  api POST "/repos/$REPO/issues/$PR_NUMBER/comments" ... >/dev/null
fi
# ...
url=$(api PATCH "/repos/$REPO/pulls/$PR_NUMBER" -d '{"state":"closed"}' | jq -r '.html_url')

If the PATCH fails (403, network error, etc.), set -e will abort the script, but the comment is already posted. The PR is now open with a stray "closing" comment on it. This is also how issue close works, so it is at least consistent, but worth noting as a known limitation. gh has the same behavior, so this is arguably acceptable.

6. Closing an already-closed PR silently succeeds — NITPICK

The Forgejo API will happily PATCH a closed PR to state:"closed" and return 200. gh pr close on an already-closed PR prints a warning. This command will succeed silently. Whether that matters depends on how agentic callers use it (idempotent close is arguably a feature), but it is a behavioral difference worth documenting.

7. Branch deletion result is not checked — NITPICK

api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null
echo "PR closed: $url (branch $head_ref deleted)"

If the DELETE returns a 404 (branch already deleted, or encoded wrong per issue #4), set -e will kill the script after the PR is already closed. The user sees no "PR closed" message at all — just a silent exit 1. Catching the DELETE error and printing a warning ("PR closed but branch deletion failed") would be more user-friendly.

8. Mock returns empty body for branch DELETE — NITPICK

# testlib.sh
*/api/v1/repos/acme/widget/branches/topic)
  ;;

The mock returns an empty response for branch deletion. The real Forgejo API returns 204 No Content with an empty body. This is fine since the code does >/dev/null, but if anyone adds response checking later, the mock should at minimum be commented explaining the 204 behavior.

9. resolve_pr_target sets global PR_NUMBER — NITPICK

The function communicates its result via the global PR_NUMBER (and possibly REPO). This is consistent with resolve_issue_target which uses ISSUE_NUMBER, so it is not a bug, but it would be worth a comment noting that resolve_pr_target may modify both REPO and PR_NUMBER as side effects — especially since a URL target silently overrides any -R flag.

10. Empty comment string edge case — NITPICK

[[ "$target_set" == true && -n "$target" ]] || { die "usage: ..." }

This guards against an empty target, but:

-c|--comment)
  require_option_value "$1" "$#"
  [[ "$comment_set" == false ]] || die "$1 specified more than once"
  comment="$2"

forge pr close 12 -c "" will set comment_set=true with an empty comment string. The API call will then POST {"body":""} — an empty comment. gh pr close -c "" rejects this. Consider: [[ -n "$comment" ]] || die "--comment cannot be empty".

Summary

The core logic is solid and well-structured. The arg parsing, URL validation, and operation ordering are all correct. The two MAJOR items (URL-encoding of branch names in query parameters, and the missing guard against deleting protected/base branches) are the ones I would want fixed before merge, since one silently breaks for common branch naming conventions and the other is a data-loss footgun. Everything else is polish or defense-in-depth.

Good work overall. The resolve_pr_target abstraction in particular will pay off when pr merge or pr reopen land.

## Code Review: PR #65 — `forge pr close` Reviewed the full diff against `master`, the complete `forge` script for context, all test files, and the existing `issue close` implementation for consistency comparison. --- ### What's done well - **Consistent structure.** `pr_close` follows the same arg-parsing pattern as every other subcommand — `contains_help_flag`, while-case loop, `die` on bad input, guard clause on missing target. Easy to read. - **Reusable `resolve_pr_target` function.** Clean separation between "figure out what PR we're talking about" and "do the thing." This will serve future commands (reopen, merge) without duplication. - **URL validation is strict.** The `IFS=/ read` decomposition with the `extra` check catches trailing path segments. The `$resource == "pulls"` check means `issues/12` URLs correctly fail. Good. - **jq for JSON construction.** Comment bodies go through `jq -n --arg`, which handles all quoting. No injection there. - **Thorough happy-path test coverage.** Six distinct scenarios (plain, with comment, URL target, branch target, delete-branch, comment+delete-branch) plus the branch-not-found error path. The mock infrastructure is solid. - **Validation tests added.** Missing target, unknown option, foreign URL, wrong-resource URL — all covered. - **Operation ordering is correct.** Comment before close, close before branch delete. This matches `gh pr close` semantics. --- ### Issues found #### 1. Branch name not URL-encoded in API query string — MAJOR ```bash # resolve_pr_target, line 609 PR_NUMBER=$(api GET "/repos/$REPO/pulls?state=open&limit=50&head=${target}" \ ``` The same pattern exists in `pr_find_by_head` (line 577), so this is a pre-existing issue, but `pr close` inherits it and should be noted. A branch named `feature/foo&state=closed` or containing `%`, `+`, `#`, or spaces will produce a malformed query string. The Forgejo API may misparse the parameters or return unexpected results. The `head=` value should be percent-encoded before interpolation. A minimal fix: ```bash local encoded_target encoded_target=$(printf '%s' "$target" | jq -sRr @uri) ``` (Alternatively `curl --data-urlencode` in a GET, but that changes the `api()` call shape.) The test suite only uses the branch name `topic` — a single ASCII word — so this is completely invisible to the current tests. #### 2. `--delete-branch` will delete any branch name the API returns, including `master` — MAJOR ```bash head_ref=$(api GET "/repos/$REPO/pulls/$PR_NUMBER" | jq -r '.head.ref') # ...later... api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null ``` If someone accidentally runs `forge pr close 42 -d` on a PR whose head is `master` or `main`, this will delete the default branch. `gh pr close --delete-branch` refuses to delete the base branch. There is no guard here. At minimum, fetch the base ref too and refuse if `head_ref == base_ref`: ```bash local pr_json pr_json=$(api GET "/repos/$REPO/pulls/$PR_NUMBER") head_ref=$(echo "$pr_json" | jq -r '.head.ref') base_ref=$(echo "$pr_json" | jq -r '.base.ref') if [[ "$head_ref" == "$base_ref" ]]; then die "refusing to delete branch $head_ref: it is the PR base branch" fi ``` #### 3. No test for `-R` / `--repo` override with `pr close` — MINOR Every other test uses the implicit `-R acme/widget` from `run_ok`. There is no test that `pr close 12 -R acme/gadget` actually targets the right repo, or that URL-based close overrides an earlier `-R`. The `resolve_pr_target` URL path sets `REPO` directly, but the interaction with a pre-set `-R` is untested. #### 4. `head_ref` containing `/` or special chars reaches `DELETE` URL unencoded — MINOR ```bash api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null ``` Branch names like `feature/close-button` contain slashes. The Forgejo branch API endpoint uses `{branch}` as a path parameter and expects it URL-encoded (e.g., `feature%2Fclose-button`). The current code would produce `/repos/acme/widget/branches/feature/close-button`, which is a 404. The mock only returns `"ref":"topic"` (no slashes), so this is invisible in tests. Fix: `head_ref=$(printf '%s' "$head_ref" | jq -sRr @uri)` #### 5. No error handling if the PATCH (close) call fails but the comment already posted — MINOR ```bash if [[ "$comment_set" == true ]]; then api POST "/repos/$REPO/issues/$PR_NUMBER/comments" ... >/dev/null fi # ... url=$(api PATCH "/repos/$REPO/pulls/$PR_NUMBER" -d '{"state":"closed"}' | jq -r '.html_url') ``` If the PATCH fails (403, network error, etc.), `set -e` will abort the script, but the comment is already posted. The PR is now open with a stray "closing" comment on it. This is also how `issue close` works, so it is at least consistent, but worth noting as a known limitation. `gh` has the same behavior, so this is arguably acceptable. #### 6. Closing an already-closed PR silently succeeds — NITPICK The Forgejo API will happily PATCH a closed PR to `state:"closed"` and return 200. `gh pr close` on an already-closed PR prints a warning. This command will succeed silently. Whether that matters depends on how agentic callers use it (idempotent close is arguably a feature), but it is a behavioral difference worth documenting. #### 7. Branch deletion result is not checked — NITPICK ```bash api DELETE "/repos/$REPO/branches/$head_ref" >/dev/null echo "PR closed: $url (branch $head_ref deleted)" ``` If the DELETE returns a 404 (branch already deleted, or encoded wrong per issue #4), `set -e` will kill the script *after* the PR is already closed. The user sees no "PR closed" message at all — just a silent exit 1. Catching the DELETE error and printing a warning ("PR closed but branch deletion failed") would be more user-friendly. #### 8. Mock returns empty body for branch DELETE — NITPICK ```bash # testlib.sh */api/v1/repos/acme/widget/branches/topic) ;; ``` The mock returns an empty response for branch deletion. The real Forgejo API returns 204 No Content with an empty body. This is fine since the code does `>/dev/null`, but if anyone adds response checking later, the mock should at minimum be commented explaining the 204 behavior. #### 9. `resolve_pr_target` sets global `PR_NUMBER` — NITPICK The function communicates its result via the global `PR_NUMBER` (and possibly `REPO`). This is consistent with `resolve_issue_target` which uses `ISSUE_NUMBER`, so it is not a bug, but it would be worth a comment noting that `resolve_pr_target` may modify both `REPO` and `PR_NUMBER` as side effects — especially since a URL target silently overrides any `-R` flag. #### 10. Empty comment string edge case — NITPICK ```bash [[ "$target_set" == true && -n "$target" ]] || { die "usage: ..." } ``` This guards against an empty target, but: ```bash -c|--comment) require_option_value "$1" "$#" [[ "$comment_set" == false ]] || die "$1 specified more than once" comment="$2" ``` `forge pr close 12 -c ""` will set `comment_set=true` with an empty comment string. The API call will then POST `{"body":""}` — an empty comment. `gh pr close -c ""` rejects this. Consider: `[[ -n "$comment" ]] || die "--comment cannot be empty"`. --- ### Summary The core logic is solid and well-structured. The arg parsing, URL validation, and operation ordering are all correct. The two MAJOR items (URL-encoding of branch names in query parameters, and the missing guard against deleting protected/base branches) are the ones I would want fixed before merge, since one silently breaks for common branch naming conventions and the other is a data-loss footgun. Everything else is polish or defense-in-depth. Good work overall. The `resolve_pr_target` abstraction in particular will pay off when `pr merge` or `pr reopen` land.
vnprc force-pushed agent/pr-close from 9a28519578 to 8f36bccc04 2026-06-23 14:58:32 +01:00 Compare
vnprc-agent commented 2026-06-23 19:21:06 +01:00
Author
Contributor
Copy link

Second-Pass Review: PR #65 — forge pr close

Reviewed by a greybeard bash scripter who dreams in POSIX.

Prior Review Fixes — Status

All MAJOR findings from the first review were properly addressed:

  • URL-encoding of branch names: FIXED (urlencode helper via jq @uri)
  • Base-branch deletion guard: FIXED (warns and skips instead of deleting master)
  • Non-fatal branch DELETE: FIXED (failure prints warning, still reports PR closed)
  • Empty comment rejection: FIXED (--comment "" now dies)
  • Repo override test, mock 204 comment: FIXED

Items intentionally left as-is (consistent with issue close and gh): comment-before-failed-PATCH, idempotent close of already-closed PR, global-setting resolve_pr_target.

New Findings

MAJOR

M1. urlencode() uses jq -R instead of jq -sR
jq -R reads one line at a time. If input contains a newline, only the first line is encoded; the rest becomes a separate unencoded output line. Git branch names can't contain newlines so this is low practical risk, but jq -sRr @uri is strictly safer and a one-character fix.

MINOR

m1. pr close --help not tested in help.sh
Every other subcommand has a --help test. pr close is the only one missing.

m2. No test for --delete-branch with a branch-name target
All -d tests use numeric targets. The full resolve-by-branch + fetch-refs + close + delete pipeline (the most complex code path) has no dedicated test.

m3. No test for duplicate --comment flag rejection
The code guards against pr close 12 -c "first" -c "second" but no test exercises it. Compare with the duplicate --title test for pr create.

m4. -R override test uses run_capture instead of run_ok
The repo-override test doesn't assert exit code 0, unlike every other happy-path test. The assert_request provides indirect validation but the exit code isn't checked.

NITPICK

n1. Warning message for head==base guard is not asserted in tests
The test checks request count but not that the stderr warning actually appears.

n2. Minor formatting inconsistency: top-of-file comment uses {<number>|<url>|<branch>} (no spaces) while the die/usage messages use spaces around pipes.

Verdict

Solid work. The code is well-structured, follows existing conventions, and the prior MAJOR issues are properly resolved. None of the new findings are merge-blockers. The urlencode fix (M1) is worth doing before merge; the test gaps are good follow-up items.

## Second-Pass Review: PR #65 — `forge pr close` Reviewed by a greybeard bash scripter who dreams in POSIX. ### Prior Review Fixes — Status All MAJOR findings from the first review were properly addressed: - URL-encoding of branch names: FIXED (urlencode helper via `jq @uri`) - Base-branch deletion guard: FIXED (warns and skips instead of deleting master) - Non-fatal branch DELETE: FIXED (failure prints warning, still reports PR closed) - Empty comment rejection: FIXED (`--comment ""` now dies) - Repo override test, mock 204 comment: FIXED Items intentionally left as-is (consistent with `issue close` and `gh`): comment-before-failed-PATCH, idempotent close of already-closed PR, global-setting `resolve_pr_target`. --- ### New Findings #### MAJOR **M1. `urlencode()` uses `jq -R` instead of `jq -sR`** `jq -R` reads one line at a time. If input contains a newline, only the first line is encoded; the rest becomes a separate unencoded output line. Git branch names can't contain newlines so this is low practical risk, but `jq -sRr @uri` is strictly safer and a one-character fix. #### MINOR **m1. `pr close --help` not tested in `help.sh`** Every other subcommand has a `--help` test. `pr close` is the only one missing. **m2. No test for `--delete-branch` with a branch-name target** All `-d` tests use numeric targets. The full resolve-by-branch + fetch-refs + close + delete pipeline (the most complex code path) has no dedicated test. **m3. No test for duplicate `--comment` flag rejection** The code guards against `pr close 12 -c "first" -c "second"` but no test exercises it. Compare with the duplicate `--title` test for `pr create`. **m4. `-R` override test uses `run_capture` instead of `run_ok`** The repo-override test doesn't assert exit code 0, unlike every other happy-path test. The `assert_request` provides indirect validation but the exit code isn't checked. #### NITPICK **n1. Warning message for head==base guard is not asserted in tests** The test checks request count but not that the stderr warning actually appears. **n2. Minor formatting inconsistency:** top-of-file comment uses `{<number>|<url>|<branch>}` (no spaces) while the die/usage messages use spaces around pipes. --- ### Verdict Solid work. The code is well-structured, follows existing conventions, and the prior MAJOR issues are properly resolved. None of the new findings are merge-blockers. The `urlencode` fix (M1) is worth doing before merge; the test gaps are good follow-up items. 
jq -R reads line-by-line, so multi-line input would silently
truncate after the first newline. The -sR form slurps all input
into a single string first.
- Add pr close --help test to help.sh
- Add branch-name + delete-branch combo test (the most complex path)
- Add duplicate --comment flag rejection test
- Fix repo-override test to check exit code (run_capture -> assert)
- Add base.ref to PR #31 mock for completeness
vnprc approved these changes 2026-06-23 19:26:18 +01:00
vnprc merged commit 44b638f8eb into master 2026-06-23 19:26:26 +01:00
vnprc deleted branch agent/pr-close 2026-06-23 19:26:26 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
vnprc
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
allod/tools!65
No description provided.