Skip to content

docs(agents): mandate rebase onto upstream/main before every PR push#2945

Closed
M8WLO wants to merge 1 commit into
aethersdr:mainfrom
M8WLO:docs/pr-branch-rebase-hygiene
Closed

docs(agents): mandate rebase onto upstream/main before every PR push#2945
M8WLO wants to merge 1 commit into
aethersdr:mainfrom
M8WLO:docs/pr-branch-rebase-hygiene

Conversation

@M8WLO
Copy link
Copy Markdown
Contributor

@M8WLO M8WLO commented May 22, 2026

Summary

  • Adds a PR branch hygiene rule to AGENTS.md: always git rebase upstream/main before pushing a PR branch, then git push --force-with-lease

Why

PR #2859 (MIDI MOX fix) had a stale base — the QSO MIDI commit from #2845 (already merged on upstream/main) still appeared in the diff, making the PR look like 47+/1- when the intended change was 1+/1-. A rebase fixed it immediately, but the rule wasn't documented. This prevents the same issue on future PRs.

No code changes

Documentation only — AGENTS.md updated with a "PR branch hygiene — mandatory before every push" block under the CI/CD section.

🤖 Generated with Claude Code

PR branch hygiene rule: always `git rebase upstream/main` on the PR
branch before pushing, then `git push --force-with-lease`. Prevents
scope-creep diffs (commits already on upstream are automatically
skipped), keeps PR titles honest about what actually changed, and
avoids review friction from stale branch bases.

Prompted by PR aethersdr#2859 review where a QSO MIDI commit that was already
merged (aethersdr#2845) appeared as new additions because the branch was not
rebased before submission.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @M8WLO — the rule is sound and the motivation (PR #2859's stale-base diff inflation) is a real workflow problem worth documenting. Placement under CI/CD right after git ship reads naturally.

One small thing worth confirming with the maintainers before merge:

upstream/main (ten9876/AetherSDR) parenthetical — The PR base for this very PR is aethersdr:main, i.e. the canonical upstream is aethersdr/AetherSDR (the org). ten9876 appears in AGENTS.md only as the owner of the CI Docker image (ghcr.io/ten9876/aethersdr-ci), not as the host of the source-of-truth repo. If a contributor literally follows git remote add upstream against ten9876/AetherSDR, they may be rebasing onto a different ref than the one CI/branch-protection actually merges into.

Suggested tweak (only if my reading is right):

Always rebase each PR branch onto `upstream/main` (the canonical
`aethersdr/AetherSDR` repo) before pushing or force-pushing.

Otherwise this looks good to ship. Non-blocking from my side — defer to @jensenpat / @ten9876 on the upstream-owner naming since they'd know which fork is authoritative for the rebase target.

@ten9876
Copy link
Copy Markdown
Collaborator

ten9876 commented May 23, 2026

Thanks for opening this @M8WLO — the underlying problem you're solving is real, and #2859's diff inflation was a genuinely confusing review experience. The documentation impulse is the right one.

Closing this PR rather than merging it, because the policy our project actually wants is a little more nuanced than "always rebase," and I'd rather not codify the rule as worded:

How we got here on rebase vs. merge

I had your exact instinct a few weeks back. Then I got bitten by it on a PR touching src/gui/MainWindow.cpp — one of the file's hottest spots, where multiple branches commonly converge on overlapping additions.

What happened: I did git rebase upstream/main on a stale branch. Git's three-way rebase silently dropped a small block of lines that another already-merged PR had added adjacent to my new code. No conflict markers, no warning — the lines just disappeared, because rebase semantics around adjacent additions in the same context block can be ambiguous and git made a defensible-but-wrong choice. The PR built clean, CI passed, and I only caught the regression because I happened to grep -c for the original function and saw the count off by one.

The same scenario with git merge upstream/main would have surfaced the overlap as a conflict block I had to resolve by hand — annoying, but visible. So:

  • Default: rebase is fine — keeps history linear, already-merged commits drop out cleanly. This is what your PR documents and it's right for the common case.
  • Hot files (src/gui/MainWindow.cpp, anything with wide touch surface): prefer git merge upstream/main over rebase. Annoying conflict markers > silent drops.
  • Always: verify with grep -c on functions you expected to keep, especially after a rebase.

This is the kind of rule that's hard to discover by reading — you only learn it by getting burned. Which is exactly why I should have documented it months ago and didn't.

Two smaller things

  • The parenthetical (ten9876/AetherSDR) is now stale — the repo moved to aethersdr/AetherSDR on 2026-05-16. The redirect still works but the canonical is the org account.
  • The git ship alias mentioned right above your new block handles the rebase automatically for the simple case, so the policy applies more to manual contributor pushes (git push origin branch --force-with-lease) than to maintainers using git ship. Worth mentioning when we re-document.

What's next

I'll write up the full rebase/merge/hot-files policy in AGENTS.md myself, citing this PR for the original prompt and crediting you in the commit. The spirit of your contribution lands either way — just with the carveout that took me one bad rebase to learn.

Sorry for the close — it's not a reflection on your contribution. The motivation was right; we just need to capture the nuance so the rule doesn't push the next contributor into the failure mode I already walked into. Hope you'll keep contributing.

73,
Jeremy KK7GWY & Claude (AI dev partner)

@ten9876 ten9876 closed this May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants