ADR-309: implement rebase mechanic (rebase_onto) - #80
Conversation
…ed — clean rebase (returns "rebased", verified force-pushed-with-lease reaches origin), genuine conflict (returns "conflict", rebase-in-progress marker left in place, no push to origin), fetch making `origin/<new_base>` the rebase source of truth over a stale local ref, self-checkout of `working_branch` when the worktree starts on a different branch, and non-conflict git failures (fetch) propagating as `CalledProcessError` rather than being swallowed. No logging, no CLI wrapper, and no additional invalid-input branches in scope for this component.)
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against the ADR-309 task brief and _spec_ConcurrentDevelopment.md. All five exit criteria are met: rebase_onto(working_branch, new_base, worktree) -> Literal["rebased", "conflict"] fetches, rebases onto the freshly-fetched origin/<new_base>, force-pushes with lease on success, and leaves a conflicted rebase untouched (no --continue/--skip/--abort) on failure. Tests build real fixture git repos (bare origin + working clone) covering clean rebase, genuine conflict, stale-local-base-vs-fresh-fetch, self-checkout onto a different starting branch, and fetch-failure propagation. Ran the suite locally: 5/5 new tests pass, and the full workflow-orchestrate scripts suite (165 tests) passes with no regressions. CONTRIBUTING.md guidelines are followed: timeout= is passed on every subprocess.run, CalledProcessError is never silently swallowed (fetch/checkout/push propagate; only the rebase step's failure is deliberately reinterpreted as the business outcome "conflict"), and test naming/AAA structure match convention. Two small non-blocking suggestions left inline below. Approving.
Review comment: the caught CalledProcessError from the rebase step had no stderr/stdout to inspect, so a non-content rebase-start failure (e.g. a dirty working tree) was indistinguishable from a genuine conflict. Capture the rebase subprocess output and log e.stderr at debug level before returning "conflict", giving a future caller something to diagnose with.
Review comment: the stale-local-base-vs-fresh-fetch test only checked for tundra.txt's presence post-rebase, not the function's return value. Capture and assert result == "rebased" for a stronger, self-documenting assertion.
Work item: ADR-309
Implement the rebase mechanic: a standalone
rebase_onto(working_branch, new_base, worktree) -> Literal["rebased", "conflict"]function that fetches, rebases a working branch onto an updated base, and force-pushes with lease on success. This is the first git-operation primitive that later Concurrent Development spec tasks (dev-team:watch-pr,resolve-rebase-conflict, the base-branch resolver) will build on.Changes
plugins/dev-team/skills/workflow-orchestrate/scripts/rebase_mechanic.py—rebase_onto(working_branch, new_base, worktree). Fetchesorigininworktree, checks outworking_branch, rebases onto the freshly-fetchedorigin/<new_base>; on success force-pushes with lease (git push --force-with-lease origin <working_branch>) and returns"rebased"; on rebase failure, leaves the rebase in progress untouched (no--continue/--skip/--abort) and returns"conflict"without pushing._run_git()helper that always passescwd=worktree,timeout=30, andcheck=True— no module-levelREPO_ROOT, per the brief's explicit-worktree-parameter requirement.plugins/dev-team/skills/workflow-orchestrate/scripts/test_rebase_mechanic.py— pytest suite building real local bare "origin" repos and working clones (no git mocking) covering: clean rebase, genuine conflict, freshly-fetched base overriding a stale local ref, self-checkout when the worktree starts on a different branch, and fetch failure propagating assubprocess.CalledProcessError.Design decisions
origin/<new_base>(the freshly-fetched remote-tracking ref), not the bare local branch name, so a stale localnew_basebranch can't cause an outdated rebase target.git rebasestep is treated as"conflict"— no further disambiguation (e.g. dirty working tree vs. genuine content conflict) is attempted, since both exit-criteria scenarios are satisfied by this simpler behavior. The conflict-path test independently confirms a real rebase-in-progress marker (.git/rebase-merge/.git/rebase-apply) is left behind. Flagged as a known, non-blocking ambiguity for reviewers.rebase_ontoperforms its own checkout ofworking_branchrather than assuming the caller's worktree is already on it, since it will later be invoked from a long-liveddev-team:watch-prmonitor whose current branch isn't guaranteed.main()CLI wrapper shipped — no current skill or script invokes the mechanic yet; deferred to whichever later task (dev-team:watch-pr) first needs to call it viaBash.Testing completed
plugins/dev-team/skills/workflow-orchestrate/scripts/suite (165 tests) passes with no regressions.