Default PR merge messages to the PR body

stephen commented

2026-09-06 16:39:32 +00:00

Owner

fj pr merge sends an empty message body when --message is omitted, and Forgejo does not fall
back to the PR description the way its web merge dialog does. So the two merge paths produce
different commits from the same pull request, and the difference is the entire description. This
fleet merges through the CLI, so every PR body merged that way has been lost from main
permanently, silently: the merge succeeds and the commit looks normal.

Confirming test, both arms, on this repo about an hour apart with the same tool, style and account:

#258, no --message     git log -1 --format=%b 116be94 | wc -c  ->     1
#260, --message=body   git log -1 --format=%b 60b51d4 | wc -c  ->  1449

Fixes rasterstate/fleet#913, which counted the damage: ten consecutive merges on
fjord-os/repo-daemon-async all landed empty, and on rasterstate/fleet the empty-bodied commits
are exactly the CLI ones.

What changed

merge() defaults the merge-commit message to the pull request body when --message is not
given. An explicit --message still wins, including --message "" for a deliberately empty body.
Pull::body now deserializes JSON null to the default rather than relying on #[serde(default)]
alone, so a bodyless PR produces an empty message instead of leaking a placeholder.
Integration tests at the request boundary assert the body actually sent to the merge endpoint.

Testing

cargo fmt --all, cargo clippy --locked --all-targets and cargo test --locked pass. The new
test was run against the old merge behaviour and fails there, so it covers the defect rather than
the fix.

Open questions for review

Raising these because the author hit its context limit before answering them, and I pushed the
branch rather than lose the work.

The change sits in the shared merge(), so it applies to every style, not only squash. Squash is
where the loss was measured. Whether a merge or rebase-merge commit should also carry the PR
body is a real question and it is currently answered by construction rather than deliberately.
It costs one extra GET per merge that omits --message. That looks acceptable for an
interactive command, but nothing measured it.

`fj pr merge` sends an empty message body when `--message` is omitted, and Forgejo does not fall back to the PR description the way its web merge dialog does. So the two merge paths produce different commits from the same pull request, and the difference is the entire description. This fleet merges through the CLI, so every PR body merged that way has been lost from `main` permanently, silently: the merge succeeds and the commit looks normal. Confirming test, both arms, on this repo about an hour apart with the same tool, style and account: ``` #258, no --message git log -1 --format=%b 116be94 | wc -c -> 1 #260, --message=body git log -1 --format=%b 60b51d4 | wc -c -> 1449 ``` Fixes rasterstate/fleet#913, which counted the damage: ten consecutive merges on `fjord-os/repo-daemon-async` all landed empty, and on `rasterstate/fleet` the empty-bodied commits are exactly the CLI ones. ## What changed - `merge()` defaults the merge-commit message to the pull request body when `--message` is not given. An explicit `--message` still wins, including `--message ""` for a deliberately empty body. - `Pull::body` now deserializes JSON `null` to the default rather than relying on `#[serde(default)]` alone, so a bodyless PR produces an empty message instead of leaking a placeholder. - Integration tests at the request boundary assert the body actually sent to the merge endpoint. ## Testing `cargo fmt --all`, `cargo clippy --locked --all-targets` and `cargo test --locked` pass. The new test was run against the old merge behaviour and fails there, so it covers the defect rather than the fix. ## Open questions for review Raising these because the author hit its context limit before answering them, and I pushed the branch rather than lose the work. - The change sits in the shared `merge()`, so it applies to every style, not only squash. Squash is where the loss was measured. Whether a `merge` or `rebase-merge` commit should also carry the PR body is a real question and it is currently answered by construction rather than deliberately. - It costs one extra `GET` per merge that omits `--message`. That looks acceptable for an interactive command, but nothing measured it.

stephen added 1 commit

2026-09-06 16:39:32 +00:00

Default PR merge messages to the PR body

ci / check (pull_request) Successful in 11m22s

Details

ci / live-e2e (pull_request) Successful in 2m12s

Details

ci / coverage (pull_request) Successful in 2m19s

Details

cc8f1a99db

stephen commented

2026-09-06 16:52:04 +00:00

Author

Owner

The author answered the two open questions from the description before its context ran out.
Recording them here so they are not lost, and flagging that they are its account of its own change
rather than something I verified.

Styles. The defaulting sits in api::pull::merge, so it applies to every style and to
--auto, because they all share that request path. That is the "answered by construction" I
flagged, now stated deliberately. Whether a merge or rebase-merge commit should carry the PR
body is still a judgement call worth a reviewer's opinion; the measured loss was on squash.
Cost. The extra GET happens only when MergeOptions.message is None. An explicit
--message, including --message "", skips it.

Both are checkable against the diff and should be checked rather than taken from this comment.

The author answered the two open questions from the description before its context ran out. Recording them here so they are not lost, and flagging that they are its account of its own change rather than something I verified. - **Styles.** The defaulting sits in `api::pull::merge`, so it applies to every style and to `--auto`, because they all share that request path. That is the "answered by construction" I flagged, now stated deliberately. Whether a `merge` or `rebase-merge` commit *should* carry the PR body is still a judgement call worth a reviewer's opinion; the measured loss was on squash. - **Cost.** The extra `GET` happens only when `MergeOptions.message` is `None`. An explicit `--message`, including `--message ""`, skips it. Both are checkable against the diff and should be checked rather than taken from this comment.

stephen commented

2026-09-06 17:09:28 +00:00

Author

Owner

APPROVE
Head reviewed: cc8f1a99db

Stephen pushed this branch and opened PR #262 after codex-8 wrote the commit and hit its context limit. I reviewed the code as the first gate on that pushed head.

The behavior is covered at the request boundary, not only in a helper. I ran cargo test --locked pull_merge_: 38 merge-related tests passed, including pull_merge_defaults_message_to_pull_body, pull_merge_explicit_empty_message_overrides_pull_body_default, pull_merge_defaults_null_pull_body_to_empty_message, and pull_merge_auto_posts_merge_when_checks_succeed. I also ran cargo test --locked pull_get_decodes_null_body_as_empty: 1 passed. If the defaulting were not affecting the actual merge endpoint request, the wiremock POST /api/v1/repos/o/r/pulls/12/merge expectations containing MergeMessageField from the PR body would have missed and produced 404s. If --message "" still consulted the PR body, the test's GET expectation with .expect(0) would have failed and the POST body would not have matched the empty message. If JSON null were not handled, the decode test would have failed before merge.

I proved the tests are not vacuous by temporarily reverting only the implementation hunks in src/api/pull_core.rs while leaving the new tests in place, then restoring them. With the old merge behavior, cargo test --locked pull_merge_ failed three tests: pull_merge_defaults_message_to_pull_body, pull_merge_defaults_null_pull_body_to_empty_message, and pull_merge_auto_posts_merge_when_checks_succeed; each failed because the expected merge POST was not matched and wiremock returned 404. With the old deserialize behavior, cargo test --locked pull_get_decodes_null_body_as_empty failed with invalid type: null, expected a string. After restoration, both commands passed again. If the tests were vacuous, the reverted implementation would have stayed green.

The open question about styles is answered by construction and I think the construction is acceptable. CLI parsing turns --style merge, --style rebase, --style rebase-merge, and --style squash into the same api::pull::merge call. In that function the default body is fetched before MergeBody is assembled, and style.as_str() is only the Do field. --auto is also just merge_when_checks_succeed on the same body. I ran cargo test --locked merge_, which includes merge_style_as_str_maps_each_variant, merge_style_maps_to_api_style, and merge_body_sets_auto_field_when_scheduling. If another path bypassed api::pull::merge, these tests would not prove the end-to-end CLI path, so I also checked src/cli/pr.rs: the merge command calls only api::pull::merge unless --disable-auto is used, which calls cancel_auto_merge instead. Applying the default to all merge styles is the right default for this CLI: an absent message should preserve the PR description rather than silently discard it, while --message "" remains the explicit opt-out. For rebase-like styles, this is no worse than the explicit message field users could already send; if Forgejo ignores it for a style, the behavior is still not silent data loss.

The extra GET fires only when MergeOptions.message is None. The code is a direct match opts.message: the Some arm returns the supplied value and does not call get; the None arm calls get(...).await?.body. The explicit-empty test proves the Some("") case by mounting a GET with .expect(0) and passing. If the extra GET also fired for explicit messages, that test would fail despite the final POST body being correct. One extra request is acceptable for an interactive merge command because it prevents permanent loss of the PR body, and explicit --message, including the empty string, keeps the old no-GET path reachable.

The new GET fails closed. get(client, owner, name, number).await?.body uses ?, so a failed PR read aborts before the merge POST is sent. That is the right failure mode: proceeding with an empty message would recreate the silent loss this PR fixes. If the code failed open, there would be a fallback branch or unwrap_or_default around the GET and a later POST with MergeMessageField: ""; there is none.

I ran cargo check --locked: passed. I did not push or merge from this throwaway clone.

APPROVE Head reviewed: cc8f1a99db3e281fa77f6a6b41edfe72456683c4 Stephen pushed this branch and opened PR #262 after codex-8 wrote the commit and hit its context limit. I reviewed the code as the first gate on that pushed head. The behavior is covered at the request boundary, not only in a helper. I ran `cargo test --locked pull_merge_`: 38 merge-related tests passed, including `pull_merge_defaults_message_to_pull_body`, `pull_merge_explicit_empty_message_overrides_pull_body_default`, `pull_merge_defaults_null_pull_body_to_empty_message`, and `pull_merge_auto_posts_merge_when_checks_succeed`. I also ran `cargo test --locked pull_get_decodes_null_body_as_empty`: 1 passed. If the defaulting were not affecting the actual merge endpoint request, the wiremock `POST /api/v1/repos/o/r/pulls/12/merge` expectations containing `MergeMessageField` from the PR body would have missed and produced 404s. If `--message ""` still consulted the PR body, the test's `GET` expectation with `.expect(0)` would have failed and the POST body would not have matched the empty message. If JSON null were not handled, the decode test would have failed before merge. I proved the tests are not vacuous by temporarily reverting only the implementation hunks in `src/api/pull_core.rs` while leaving the new tests in place, then restoring them. With the old merge behavior, `cargo test --locked pull_merge_` failed three tests: `pull_merge_defaults_message_to_pull_body`, `pull_merge_defaults_null_pull_body_to_empty_message`, and `pull_merge_auto_posts_merge_when_checks_succeed`; each failed because the expected merge POST was not matched and wiremock returned 404. With the old deserialize behavior, `cargo test --locked pull_get_decodes_null_body_as_empty` failed with `invalid type: null, expected a string`. After restoration, both commands passed again. If the tests were vacuous, the reverted implementation would have stayed green. The open question about styles is answered by construction and I think the construction is acceptable. CLI parsing turns `--style merge`, `--style rebase`, `--style rebase-merge`, and `--style squash` into the same `api::pull::merge` call. In that function the default body is fetched before `MergeBody` is assembled, and `style.as_str()` is only the `Do` field. `--auto` is also just `merge_when_checks_succeed` on the same body. I ran `cargo test --locked merge_`, which includes `merge_style_as_str_maps_each_variant`, `merge_style_maps_to_api_style`, and `merge_body_sets_auto_field_when_scheduling`. If another path bypassed `api::pull::merge`, these tests would not prove the end-to-end CLI path, so I also checked `src/cli/pr.rs`: the merge command calls only `api::pull::merge` unless `--disable-auto` is used, which calls `cancel_auto_merge` instead. Applying the default to all merge styles is the right default for this CLI: an absent message should preserve the PR description rather than silently discard it, while `--message ""` remains the explicit opt-out. For rebase-like styles, this is no worse than the explicit message field users could already send; if Forgejo ignores it for a style, the behavior is still not silent data loss. The extra GET fires only when `MergeOptions.message` is `None`. The code is a direct `match opts.message`: the `Some` arm returns the supplied value and does not call `get`; the `None` arm calls `get(...).await?.body`. The explicit-empty test proves the `Some("")` case by mounting a GET with `.expect(0)` and passing. If the extra GET also fired for explicit messages, that test would fail despite the final POST body being correct. One extra request is acceptable for an interactive merge command because it prevents permanent loss of the PR body, and explicit `--message`, including the empty string, keeps the old no-GET path reachable. The new GET fails closed. `get(client, owner, name, number).await?.body` uses `?`, so a failed PR read aborts before the merge POST is sent. That is the right failure mode: proceeding with an empty message would recreate the silent loss this PR fixes. If the code failed open, there would be a fallback branch or `unwrap_or_default` around the GET and a later POST with `MergeMessageField: ""`; there is none. I ran `cargo check --locked`: passed. I did not push or merge from this throwaway clone.

stephen merged commit 5c6d099df5 into main

2026-09-06 17:09:29 +00:00

stephen deleted branch fix/merge-default-message

2026-09-06 17:09:30 +00:00

stephen referenced this pull request from a commit

2026-09-06 17:09:31 +00:00

Default PR merge messages to the PR body (#262)

stephen referenced this pull request from a commit

2026-09-06 18:18:17 +00:00

Keep the strict coverage report on the tested surface

~~stephen referenced this pull request 2026-09-06 18:18:29 +00:00~~

Cover git config and editor glue #264