feat: add Jujutsu (jj) support via VCSOperations abstraction#496
Draft
jucor wants to merge 6 commits into
Draft
Conversation
jucor
force-pushed
the
jj-compat
branch
4 times, most recently
from
ef2d8fd to
3e041d5
Compare
jucor
force-pushed
the
jj-compat
branch
3 times, most recently
from
ed38421 to
49dfab9
Compare
8 tasks
Adds end-to-end Jujutsu support for stacked PRs in colocated git+jj repos, preserving jj change IDs across all spr operations and using jj-native primitives (auto-rebase, conflicts as commits, non-blocking edits) instead of mimicking git's rebase-session model. Core architecture: - New vcs.VCSOperations interface — the abstraction boundary for history-rewriting operations (FetchAndRebase, GetLocalCommitStack, AmendInto, EditStart/Finish/Abort, Fetch, PushBranches, etc.). - vcs/git_ops.go: git implementation (extracted from spr.go, behavior- preserving). Improved EditFinish uses .git/REBASE_HEAD to distinguish conflict-resolution from initial edit stop and stages only tracked changes (subsumes upstream's 606df43 and 0767a45 fixes). - vcs/jj_ops.go: jj implementation. Uses jj-native commands: jj git fetch + jj rebase, jj describe (for auto-added commit-id trailers), jj squash --into, jj edit, jj bookmark set + jj git push. - Commit.ChangeID field (empty in git mode, populated in jj mode). - vcs/detect.go: factory chooses JjOps when .jj/ exists and User.NoJJ is false; respects --no-jj flag and SPR_NOJJ env var. Auto-revset (the key correctness fix): - JjOps.GetLocalCommitStack queries trunk()..(@:: | ::@) — the connected component containing @, excluding trunk — rather than the position-dependent trunk()..@. Result: spr finds the full stack regardless of where @ is, so 'spr update' from a mid-stack @ no longer silently truncates the stack and closes PRs for the missing commits. - JjOps.CheckStackCompleteness detects multi-head ambiguity via heads(trunk()..(@:: | ::@)) and refuses non-linear stacks with a clear error pointing at jj rebase / jj edit as the fix. - checkStackUsable (renamed from confirmIfIncompleteStack, used as guard in update/merge/status/check/amend) blocks rather than prompts on ambiguity — no safe 'continue anyway' answer exists. jj-mode behavioral divergences: - spr edit (jj): announces the action then runs 'jj edit <change-id>'. No state file, no op-id snapshot, no rebase-session machinery — jj is non-blocking and 'jj undo' handles revert. --done and --abort are echo-only deprecations that point users at native jj. - spr sync (jj): runs 'jj git fetch' (the literal 'git cherry-pick' was broken in jj). No rebase promise — that's spr update's job. VCS-layer integrations: - Configurable branch prefix (upstream feature b01881d): jj_ops threads cfg.User.BranchPrefix through PushBranches' bookmark glob. - Stdout/stderr separation in vcs/jj_cmd.go: replaced cmd.CombinedOutput() with separate capture so jj's stderr messages (e.g. 'Rebased 1 descendant commits onto updated working copy') don't corrupt parsed log output. Caught by the conflicted-push integration test. Testing: - Two suites: unit (mockjj-based, fast) and integration (real jj via the new vcs/jjtest package + a colocated test repo). - Integration build tag //go:build integration; SPR_SKIP_JJ_INTEGRATION=1 env var to skip cleanly. - jjtest.NewRepo creates a colocated git+jj repo in t.TempDir() with a bare-repo origin; helpers for stack topology (AddCommit, Edit, Fork, InsertAbove, SnapshotOpLog, AssertOpsSince). - Headline regression: TestSprIntegration_StatusFromMidStack_ShowsFullStack pins that GetLocalCommitStack from mid-stack @ returns the whole stack. - TestJjIntegration_PushBranches_ConflictedCommit_Refused pins jj's natural refusal to push conflicts (this is the test that revealed the stderr bug). - 100% statement coverage on jj-related production code (jj_ops.go, jj_cmd.go, jj_parse.go). - Op-log assertions verify that spr commands do exactly the expected jj operations and no destructive ones. Coverage tooling: - scripts/coverage-merge/main.go merges two coverprofile files (max hits per block, within and across profiles) and reports per-file attribution (unit %, integ %, merged %, plus 'only in X' sections). - Makefile targets: test-unit, test-integration, test-all, coverage-html. - CI installs pinned JJ_VERSION (0.40.0), runs both suites with -coverpkg=./..., merges, uploads as artifact. Documentation: - readme.md: new Jujutsu (jj) Support section covering setup, the jj spr command set, divergences for edit/sync, the multi-head error message, and the auto-revset behavior. - docs/jj-mode-design.md: full design rationale including alternatives considered (spr-tip bookmark, 3-option prompt), the stderr bug fix, testing strategy, and what's deferred. Files: +4500/-110 across 35 files.
When a stacked PR is squash-merged on GitHub, the resulting commit on
trunk absorbs the cumulative diff of all PRs at-or-below the merge
target. Local commits whose patches are subsumed by that squash should
drop out on the next fetch+rebase — git mode does this automatically
via patch-id detection in plain `git rebase`. jj mode did not: vanilla
`jj rebase` left them as [EMPTY] stubs cluttering the stack.
Passing --skip-emptied makes jj match git's self-healing behavior for
the clean form of the cascade (no local drift). It does NOT cover:
- User-edit drift (orphan commit amended locally before next update)
- Structural overlap (multiple PRs touching the same file with
cumulative diffs that don't bit-match individual slices)
Both cases still produce conflicts on rebase; the next step (orphan-
abandon in FetchAndRebase) closes them.
Adds a characterization test suite:
- vcs/gittest/ — bare remote + local clone harness, mirrors jjtest
- vcs/gittest/cascade_integration_test.go — git mode self-heals (asserted)
- vcs/jj_cascade_integration_test.go — 6 sub-tests covering vanilla
vs --skip-emptied across clean, drift, and structural-overlap variants
- TestJjOps_FetchAndRebase_SelfHealsCleanCascade — spr-API regression
guard confirming the flag is wired up
mockjj/mockjj.go ExpectRebase / ExpectRebaseAndFail updated to match
the new command form.
Phase 2a of the cascade-orphan fix: introduce the VCS-level plumbing
that abandons local commits whose PRs have been orphaned by an upstream
squash, before rebase has a chance to see them. Without this, --skip-
emptied alone resolves the clean case (orphan whose content is wholly
absorbed by trunk) but the polis production form (structural overlap on
shared files, or local drift on amended orphans) still produces
conflicts.
Interface changes:
- VCSOperations.FetchAndRebase now takes (cfg, orphanChangeIDs).
Callers pass nil for backwards compatibility; Phase 2c will populate.
- VCSOperations.AbandonChangeIDs([]string) — new hook.
JjOps:
- FetchAndRebase: fetch → AbandonChangeIDs(orphans) → rebase
--skip-emptied. Abandon errors short-circuit before rebase, so
rebasing with a still-orphan commit never happens.
- AbandonChangeIDs: jj abandon <id> per entry; empty input is no-op.
GitOps:
- FetchAndRebase: signature change; orphans ignored. Git's rebase
handles the clean case via patch-id; drift/overlap cases have no
git-side equivalent fix today.
- AbandonChangeIDs: no-op for interface compat.
Mocks: mockjj.ExpectAbandon + ExpectAbandonAndFail.
Tests:
- vcs/jj_ops_test.go: 5 unit tests covering AbandonChangeIDs (empty,
multiple ordered, error short-circuit) and FetchAndRebase (with
orphans, with abandon-fail, with NoRebase).
- vcs/git_ops_test.go: AbandonChangeIDs no-op + FetchAndRebase
ignores orphans (regression guard).
- vcs/jj_cascade_integration_test.go: 2 new spr-API regression
guards — OrphanPrune_ResolvesDrift and OrphanPrune_ResolvesStructur-
alOverlap — verifying the previously-stuck scenarios from the
characterization suite now resolve to a clean stack.
spr.fetchAndGetGitHubInfo passes nil orphans; populated in Phase 2c.
Phase 2b of the cascade-orphan fix: expose GitHub's view of orphan PRs
(state=CLOSED-not-merged whose head ref matches the spr branch prefix)
so the spr update flow can identify which local change IDs to abandon
before rebase.
GraphQL:
- New ClosedOrphanPullRequests query in queries.graphql, mirroring
PullRequests' shape but with states:[CLOSED]. GitHub's enum has
CLOSED (closed-not-merged) as distinct from MERGED, so this filter
alone is sufficient — no client-side dedup needed.
- gen/genclient/operations.go regenerated via `go generate`.
GitHubInterface:
- GetClosedOrphanPRs(ctx) []*PullRequest returns orphan candidates
with .Commit.CommitID populated from the HeadRefName, so callers
don't need to re-parse trailers. Nil on transport error; empty
slice on "no orphans found".
githubclient.client:
- GetClosedOrphanPRs delegates the per-node filter to
filterClosedOrphans (pure helper, no transport) so the matching
logic can be unit-tested without mocking the genclient interface
(14 methods, too many to stub for one test).
Mockclient:
- MockClient.GetClosedOrphanPRs + ExpectGetClosedOrphanPRs.
Canned response via package-level MockClientClosedOrphans slice
so test setup is a single assignment.
Tests:
- client_test.go: TestFilterClosedOrphans — 4 sub-tests covering
nil input, no-orphans, mixed match/skip, custom branch prefix.
Unrelated drive-by:
- go.mod: golang.org/x/tools upgraded so `go generate` works on
Go 1.26 (old version had constant-overflow build error).
- git/mockgit/mockgit.go: drop fmt.Sprintf in Mock.expect; new vet
flagged non-constant format strings. Callers already pre-format.
Phase 2c of the cascade-orphan fix: bridge the GitHub-side orphan
signal (Phase 2b) to the VCS-side abandon plumbing (Phase 2a) so
`spr update` automatically cleans up orphaned local commits before
rebase, avoiding the conflict cascade described in
docs/jj-mode-design.md (and witnessed on the polis stack).
Flow:
identifyOrphanChangeIDs(ctx):
1. honor cfg.User.NoPruneOrphans (opt-out, default false)
2. github.GetClosedOrphanPRs(ctx) — pre-fetch, only GitHub API
3. build orphan commit-id set from PR HeadRefName regex
4. vcsOps.GetLocalCommitStack — pre-fetch, but change IDs and
commit-id trailers are stable across fetches so this is safe
5. intersect local commits' CommitID with orphan set; collect
ChangeID for each match
6. nil safety — empty short-circuits skip GetLocalCommitStack
fetchAndGetGitHubInfo: now calls identifyOrphanChangeIDs first, then
passes the result into FetchAndRebase. JjOps abandons each ID before
rebase; GitOps ignores (no-op per its interface contract).
Config: NoPruneOrphans bool (default false). Set true to skip orphan
detection — useful for investigating closed-not-merged PRs manually
before they're dropped, or if a transient GitHub query failure should
not block updates.
Mockclient: GetClosedOrphanPRs is intentionally NOT expectation-
verified — it's a pure read of GitHub PR state that fetchAndGetGitHubInfo
calls on every `spr update`. Verifying would require adding
ExpectGetClosedOrphanPRs() to 17 existing test fixtures that don't care
about the call. Tests that DO care set
mockclient.MockClientClosedOrphans to the canned response.
Tests:
- spr/spr_test.go: TestIdentifyOrphanChangeIDs — 5 sub-tests
covering opt-out, no-orphans, match/no-match, missing-change-ID
(git-mode safety).
- Existing UpdatePullRequests fixtures unchanged: empty
MockClientClosedOrphans means the new call returns nil and the
rest of the flow matches pre-2c behavior.
User-facing docs to surface what the previous four commits actually do.
Without this, a user running 'jj spr update' on a stack with closed
orphan PRs would see commits silently abandoned with no obvious way to
look up why or how to disable it.
docs/jj-mode-design.md:
- New 'Cascade-orphan recovery' section between 'Conflicts' and
'VCS interface changes'. Covers: the problem (squashing mid-stack
leaves orphan PRs and their content in two places), git-mode self-
heal, jj-mode's two failure modes (empty stubs, conflicts), what
spr does about it (--skip-emptied + pre-rebase jj abandon), the
noPruneOrphans opt-out, and the scenarios where you'd still need
manual recovery. Points to handoffs/ for the polis post-mortem
that motivated the work.
readme.md:
- New 'Merging mid-stack and cascade-orphan recovery' section under
Jujutsu Support, between editing and non-linear stacks. Explains
the safety net in plain language for the user case who hits it
accidentally, and points to the design doc for mechanism.
- noPruneOrphans row added to the User Configuration table with a
short pointer back to the design-doc section.
No CLI flag added — yaml-only opt-out, matching noRebase/noFetch/noJJ
convention. The flag isn't a hot path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @ejoffe
Full transparency: I have used Claude Code (Opus 4.6 extended) a lot for this PR, just as I do for my own work nowadays. I held it on a tight leash, and will be beta-testing intensively on the large stack of commits I have in my current main work repo. This PR solves a real need I have for work.
However, I am accutely aware of the hot debate about using AI in open-source, and the risk of unwelcome "drive-by PRs". I do not want to be a bother: if you do not want AI-generated code here, please do feel free to say so, and I can just keep using my fork in my corner! That's also why I tag this PR as "draft". It's functional, just wanted to discuss first.
Summary
Adds Jujutsu (jj) support to spr. When a
.jj/directory is detected (jj-colocated repo), spr uses jj-native commands for history-rewriting operations instead ofgit rebase. This preserves jj change IDs across all spr operations.Key changes:
vcs/package with aVCSOperationsinterface abstracting the 7 operations where git and jj differGitOps: pure extraction of existing logic (zero behavior change for git-only repos)JjOps: jj-native implementation usingjj describe,jj rebase,jj squash,jj edit.jj/directory, with opt-out via--no-jjflag,SPR_NOJJenv var, ornoJJ: truein~/.spr.ymljj-setupcommand to register ajj spraliasMotivation
jj is gaining traction as a git-compatible VCS with an updated mental model (immutable commits, change IDs that survive rewrites). spr is the best stacked-PR tool for squash-merge workflows on GitHub. But currently, every
spr updaterunsgit rebase, which destroys jj change IDs in a colocated repo — making the two tools painful to combine.This PR makes them work together cleanly: spr's commit-id trailers + PR management coexist with jj's change IDs and operation model.
There exists a work-in-progress jj-spr tool, but it is lacking several features (need 4 manual commands for each merge or the stack breaks), so I figured it was probably better to adapt the battle-tested spr to
jjrather than have a new project from scratch.How it works
git fetch+git rebase --autostashjj git fetch+jj rebase -b @ -d main@origingit rebase -iwithspr_reword_helperjj describe -r <change-id>git commit --fixup+git rebase -i --autosquashjj squash --into <change-id>git rebase -iwitheditstopjj edit <change-id>git stash/git stash popGit push, branch management, and all GitHub API calls remain unchanged.
Backward compatibility
NewStackedPRconstructor acceptsvcsOpsas a variadic parameter — existing callers work without modification.jj/exists; plain git repos behave identically to before-race, zero modifications neededcommit-idtrailer format is unchanged (commit-id:XXXXXXXX)Test plan
go test -race ./...)