Skip to content

fix(server): respect Forgejo branch deletion settings on merge - #14300

Open
pat-s wants to merge 5 commits into
pingdotgg:mainfrom
pat-s:t3code/verify-forgejo-branch-deletion
Open

pat-s wants to merge 5 commits into
pingdotgg:mainfrom
pat-s:t3code/verify-forgejo-branch-deletion

Conversation

@pat-s

@pat-s pat-s commented Sep 29, 2026 •

Copy link
Copy Markdown

Forgejo merges in T3 Code leave source branches behind even when the target repository enables deletion after merge.
Read the repository's current default_delete_branch_after_merge setting and pass it as delete_branch_after_merge to Forgejo's merge endpoint.

Disabled or absent settings retain branches.
A failed, malformed, or truncated read of the setting or the pull request prevents the merge, with a detail that says the merge did not run, rather than silently ignoring the repository's preference.

Forgejo deletes the head branch only after the merge lands, and reports a refused deletion as a failed merge (HTTP 403).
To avoid reporting a landed merge as failed:

  • Request deletion only when the head branch is deletable by the merger, as Forgejo's web merge form does: the head repository is writable (permissions.push) and the head branch is not its default_branch.
    A fork the merger cannot push to, a deleted head repository, or a default branch merge with delete_branch_after_merge: false.
  • Branch protection is not exposed on the pull request, so a protected head branch still reaches Forgejo's refusal.
    When a merge that requested deletion fails, re-read the pull request; if it is merged, report success and log the kept branch, otherwise report the original merge error.

Fixes #14296.

Verification

Live, against Forgejo 13.0.5 (codeberg.org/forgejo/forgejo:13) in Docker with default_delete_branch_after_merge: true.
Each case ran ForgejoPullRequestProvider.runAction({ action: "merge" }) through the real ForgejoCli layer (fj 0.6.0 token, HTTP API), on the earlier PR head 8f96363 and on 5aaa192.

Case Before: T3 reports After: T3 reports Forgejo PR Head branch
Same-repo branch, unprotected success success merged deleted
Same-repo branch, protected failed: HTTP 403 "the head branch is protected" success merged kept
Fork branch, non-admin merger without push to the fork failed: HTTP 403 "insufficient permission to delete head branch" success merged kept

The fork case needs a non-admin repository owner; a site admin may delete fork branches.

Focused tests: vp test run src/pullRequest/ForgejoPullRequestProvider.test.ts in apps/server, 24 passed, 14 of which fail against 8f96363.
They cover enabled, disabled, and missing settings for each merge method, non-deletable head branches, a fork with push access, a refused deletion after a landed merge, failed merges with and without a re-read, and failed settings reads.
Focused lint, format check, and server typecheck pass.

Not checked: Gitea via tea was not run live.
A kept branch is only reported in the server log, since runAction returns void through the contract.

Implemented with GPT-6-Astra through the Codex harness in T3 Code; the protected and fork branch handling with Claude Opus 5.5 through Claude Code in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 29, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 29, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR changes Forgejo merge behavior from always retaining head branches to conditionally deleting them based on repository settings, with additional handling for merges that land despite deletion refusal. Because this introduces a potentially irreversible production side effect on an existing path, human review is warranted.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 03ee9b0a-8e87-4f89-8da0-e93c242ebbd1

📥 Commits

Reviewing files that changed from the base of the PR and between f14d9ec and 05bea10.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5582ea10-6c44-4ab6-bca8-806590b30108

📥 Commits

Reviewing files that changed from the base of the PR and between 8f96363 and 5aaa192.

📒 Files selected for processing (3)
  • apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts
  • apps/server/src/pullRequest/ForgejoPullRequestProvider.ts
  • apps/server/src/pullRequest/forgejoPullRequestJson.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The Forgejo provider reads the repository’s branch-deletion setting before merging. When deletion is enabled, it checks whether the head branch can be deleted and includes the result in the merge request. If that request fails, the provider checks whether the pull request was merged.

Changes

Forgejo merge behavior

Layer / File(s) Summary
Read settings and submit merge
apps/server/src/pullRequest/forgejoPullRequestJson.ts, apps/server/src/pullRequest/ForgejoPullRequestProvider.ts, apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts
The repository schema accepts the optional default_delete_branch_after_merge setting. The provider reads repository settings and sends delete_branch_after_merge with the merge request. Tests cover the setting, default behavior, merge methods, and failures when required data cannot be read.
Check branch eligibility and merge outcome
apps/server/src/pullRequest/ForgejoPullRequestProvider.ts, apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts
The provider requests branch deletion only when the viewer can push to the head repository and the branch is not its default branch. Tests cover ineligible branches and follow-up checks after merge errors.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 5aaa1

The change respects repository deletion settings and preserves successful merges when branch deletion is refused. No actionable blocker remains; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 5aaa1

Branch deletion is limited by repository policy and reported push permissions, while unreadable prerequisites prevent the merge. No introduced authorization bypass was established. Some uncertainty remains around Forgejo’s behavior during partial failures, retries, and concurrent permission changes.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new destructive effect targets the source branch associated with the selected pull request, including a fork branch when the active credentials report push permission. It is requested through the merge endpoint, not an independently supplied arbitrary branch-deletion path.

Trust Boundaries and Controls

  • observed — The service resolves the project, validates supported actions and methods, and checks fresh host-backed viewer permissions before dispatching canonical repository and host coordinates. The provider obtains deletion policy and head permissions from decoded host responses rather than a client-supplied deletion preference.
  • inferred — Final deletion authorization and branch-protection enforcement remain Forgejo’s responsibility. Local policy and permission reads are not atomic with the mutation, so protection against concurrent changes depends on host-side enforcement; repository mocks do not verify that enforcement.

Resilience and Maintainability Implications

  • observed — The added prerequisite-read failure path preserves the error reason and retry time while identifying the operation as merge. It fails before mutation instead of silently treating unreadable policy as permission to proceed.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue [#14296]. ForgejoRepository decodes the optional default_delete_branch_after_merge field. The merge action reads the target repository before merging. It sends `delete_br…
Out of Scope Changes check ✅ Passed The changes stay within issue [#14296]. The head-branch eligibility checks and post-merge confirmation address Forgejo branch deletion permissions and protected-branch refusal. The schema change, merg…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Title check ✅ Passed The title clearly and concisely identifies the main change: respecting Forgejo branch deletion settings during server-side merges.
Description check ✅ Passed The description explains the problem, implementation, scope, verification results, limitations, linked issue, and agent usage. It is substantively complete, although it does not use the template's Pro…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

How is the protected or fork-branch case from the maintainer triage handled? The current merge path forwards the repository's deletion flag unconditionally, while the 15 tests cover settings and read failures. Please show what T3 reports when the merge succeeds but Forgejo rejects branch deletion, and how that meets the approved constraint and verification rule. Leaving this open for that explanation.

…refused

Forgejo deletes the head branch only after the merge lands, and reports a
refused deletion (protected branch, fork the merger cannot push to, default
branch) as the whole merge request failing with HTTP 403.

- Request deletion only when the head branch is deletable by the viewer, as
  Forgejo's web merge form does: head repository writable and not its
  default branch.
- When a merge that requested deletion fails, re-read the pull request and
  treat it as merged if Forgejo merged it, logging the kept branch.
- Report an unreadable deletion setting as a refused precondition, distinct
  from the host rejecting the merge.
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 1, 2026
@pat-s

pat-s commented Oct 1, 2026

Copy link
Copy Markdown
Author

Thanks, the previous revision did not handle it: it forwarded the repository default unconditionally, so a protected or fork head branch turned a merge that had landed into a reported failure.
5aaa192 handles both constraints from the triage.

Behaviour

  • When default_delete_branch_after_merge is on, T3 reads the pull request and only sends delete_branch_after_merge: true when the head branch is deletable by the merger, as Forgejo's web merge form does: the head repository's permissions.push is true and the head branch is not that repository's default_branch.
    A fork the merger cannot push to, a deleted head repository, or a default branch all merge with false.
  • Branch protection is not exposed on the pull request, so a protected branch (and any other post-merge deletion refusal, such as a race) still reaches Forgejo's 403.
    When a merge that requested deletion fails, T3 re-reads the pull request: if merged is true it reports the merge as successful, logs a warning with Forgejo's detail, and the branch stays, matching what the web UI does when it does not offer deletion.
    If the pull request is not merged, or the re-read fails, the original merge error is reported unchanged.
  • A failed, malformed, or truncated read of the repository setting or the pull request blocks the merge with operation: "merge" and the detail "The pull request was not merged because its branch deletion setting could not be read. …", keeping the original reason (rate limit, authentication), so it no longer looks like the host refused the merge.

Live verification

Forgejo 13.0.5 (codeberg.org/forgejo/forgejo:13) in Docker, default_delete_branch_after_merge: true on the base repository.
Each case ran ForgejoPullRequestProvider.runAction({ action: "merge" }) through the real ForgejoCli layer (fj 0.6.0 token, HTTP API), once on the previous PR head 8f96363 and once on 5aaa192.
"Head branch" is GET /repos/{head}/branches/{ref} afterwards.

Case Before: T3 reports After: T3 reports Forgejo PR Head branch
Same-repo branch, unprotected success success merged 404 (deleted)
Same-repo branch, protected failed: HTTP 403 {"message":"the head branch is protected"} success merged 200 (kept)
Fork branch, non-admin merger without push to the fork failed: HTTP 403 {"message":"insufficient permission to delete head branch"} success merged 200 (kept)

In the "before" rows Forgejo had already merged the pull request when T3 reported the failure.
The fork case had to use a non-admin repository owner; a site admin can delete fork branches, so Forgejo returns 200.

Focused tests

vp test run src/pullRequest/ForgejoPullRequestProvider.test.ts in apps/server: 24 passed; 14 of them fail against 8f96363.
They cover enabled, disabled, and missing settings for each merge method; fork without push, default branch, and deleted head repository (flag false); a fork with push (flag true); a refused deletion after a landed merge (success); a failed merge whose re-read shows it open or fails (original error); a failed merge without deletion requested (no re-read); and repository or pull request read failures (no merge request).
Focused lint, format check, and server typecheck pass.

Not checked

Gitea via tea: Gitea's merge API has the same flag semantics, but I did not run it live.
The kept branch is only visible in the server log; runAction returns void through the contract, so surfacing a non-fatal notice in the UI would need a contract change, which I left out of scope.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 2, 2026 07:09

Dismissing prior approval to re-evaluate 05bea10

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Forgejo merges ignore the repository's delete-branch-after-merge setting

2 participants