Conversation
ApprovabilityVerdict: 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:
You can add or adjust custom eligibility rules. Learn more. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesForgejo merge behavior
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
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.
|
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. Behaviour
Live verification Forgejo 13.0.5 (
In the "before" rows Forgejo had already merged the pull request when T3 reported the failure. Focused tests
Not checked Gitea via |
Dismissing prior approval to re-evaluate 05bea10
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_mergesetting and pass it asdelete_branch_after_mergeto 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:
permissions.push) and the head branch is not itsdefault_branch.A fork the merger cannot push to, a deleted head repository, or a default branch merge with
delete_branch_after_merge: false.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 withdefault_delete_branch_after_merge: true.Each case ran
ForgejoPullRequestProvider.runAction({ action: "merge" })through the realForgejoClilayer (fj 0.6.0 token, HTTP API), on the earlier PR head 8f96363 and on 5aaa192.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.tsinapps/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
teawas not run live.A kept branch is only reported in the server log, since
runActionreturnsvoidthrough 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.