Self-review: jj-compat against latest master

[jucor]

Note: this is a self-PR on my own fork (jucor:jj-compat → jucor:master), opened purely as a review surface for GitHub Copilot. The canonical PR is #496 against ejoffe/spr. Do not merge.

---

Hi @ejoffe (and Copilot reviewer)

Full transparency: I have used Claude Code (Opus 4.7 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 of git rebase. This preserves jj change IDs across all spr operations.

Key changes:

• New vcs/ package with a VCSOperations interface abstracting the operations where git and jj differ
• GitOps: pure extraction of existing logic (zero behavior change for git-only repos) — includes the recent git add -u and REBASE_HEAD-based conflict detection upstream merged
• JjOps: jj-native implementation using jj describe, jj rebase, jj squash, jj edit
• Auto-detection of .jj/ directory, with opt-out via --no-jj flag, SPR_NOJJ env var, or noJJ: true in ~/.spr.yml
• jj-setup command to register a jj spr alias
• Two divergent behaviors where jj's non-blocking model lets us simplify: jj spr edit runs jj edit <change-id> directly (no session machinery, no --done/--abort flags), and jj spr sync runs jj git fetch (the git cherry-pick path is broken in jj)
• Configurable BranchPrefix (your recent feature) threaded through jj-mode too
• 60+ new tests across two suites (unit + integration); 100% statement coverage on jj-related production code

Motivation

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 update runs git 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 jj rather than have a new project from scratch.

How it works

spr operation	git (unchanged)	jj
Fetch + rebase	git fetch + git rebase --autostash	jj git fetch + jj rebase -b @ -d main@origin
Fetch only (spr sync)	git cherry-pick ..<last-PR> (existing)	jj git fetch (no cherry-pick — change IDs align)
Discover commit stack	git log origin/main..HEAD	jj log -r 'trunk()..(@:: | ::@)' (position-independent)
Add commit-id trailers	git rebase -i with spr_reword_helper	jj describe -r <change-id>
Amend into commit	git commit --fixup + git rebase -i --autosquash	jj squash --into <change-id>
Edit a commit (spr edit)	git rebase -i with edit stop, then --done	jj edit <change-id> and exit; jj undo to revert
Stash for push	git stash / git stash pop	No-op (jj working copy is always a commit)

Git push, branch management, and all GitHub API calls remain unchanged.

Auto-revset (the key correctness fix)

The jj-mode stack discovery uses trunk()..(@:: | ::@) — the connected component containing @, excluding trunk — rather than the position-dependent trunk()..@. This means spr update finds the full stack regardless of where @ is. Without this, a user who ran jj edit to a mid-stack commit would have spr update silently truncate the stack and close PRs for the missing commits with "commit has gone away." Multi-head ambiguity (e.g. forks) is detected via heads(trunk()..(@:: | ::@)) and produces a clear error pointing at jj rebase / jj edit to consolidate.

This bug was caught early via integration tests against real jj.

Why spr edit and spr sync diverge in jj mode

Git's spr edit model relies on git rebase -i putting the repo into a "rebase paused" state that locks out other git operations. jj has no such state — jj edit is instant, descendants are auto-rebased, conflicts are stored in commits as first-class objects, and the user can navigate freely. Trying to enforce git's sequentiality through bookkeeping in jj mode is futile (the user can sidestep it with raw jj) and dangerous (any "safety net" we built would have to undo arbitrary user operations on --abort). So jj spr edit is a thin guidance layer: announce, run jj edit <change-id>, exit. jj undo handles revert.

spr sync's git implementation does git cherry-pick ..<last-PR-commit> to pull remote changes into the local stack. In jj, change IDs naturally align local and remote commits, so the cherry-pick logic is unnecessary; jj spr sync runs jj git fetch and points users at spr update for the rebase.

Full design rationale (including alternatives considered and rejected) is in docs/jj-mode-design.md.

Backward compatibility

• The NewStackedPR constructor accepts vcsOps as a variadic parameter — existing callers work without modification
• Auto-detection only activates when .jj/ exists; plain git repos behave identically to before
• All existing tests pass with -race, zero modifications needed
• commit-id trailer format is unchanged (commit-id:XXXXXXXX)
• cfg.User.BranchPrefix (your recent feature) is honored in jj mode too

Test plan

• Unit suite: ~50 tests covering detection, factory, opt-out, git ops, jj parser, jj ops error paths, MergeCheck gates, edit-flow branches, trailer-parsing edge cases. Uses mockjj and mockgit (ordered expectation-based mocks).
• Integration suite (new, //go:build integration): ~30 tests against a real jj binary in t.TempDir()-isolated colocated repos. Build tag means default go test ignores them; SPR_SKIP_JJ_INTEGRATION=1 skips cleanly when jj is unavailable. CI installs a pinned jj version. New vcs/jjtest package provides the fixture (NewRepo, AddCommit, Edit, Fork, SnapshotOpLog, AssertOpsSince, ...).
• Bug caught by integration tests: vcs/jj_cmd.go originally used cmd.CombinedOutput(), which merged jj's stderr (e.g. "Rebased 1 descendant commits onto updated working copy") into the parsed stdout. After an operation that triggered auto-rebase, the next jj log would have the rebase message prepended and parseJjLogOutput would silently produce garbage commit hashes. Fixed by separating stdout/stderr capture. This is exactly the class of bug that mock-based unit tests can't catch.
• All existing tests pass unchanged (go test -race ./...).
• Headline regression test: TestSprIntegration_StatusFromMidStack_ShowsFullStack pins the original mid-stack PR-closing bug stays fixed.
• Op-log assertions verify that jj spr commands run exactly the expected jj operations and no destructive ones.
• Coverage: 100% statement coverage on vcs/jj_ops.go, vcs/jj_cmd.go, vcs/jj_parse.go. A scripts/coverage-merge tool produces per-file attribution (unit % / integration % / merged %).
• Manual testing on a real jj-colocated repo with GitHub PRs (in progress — beta testing on my own repos)

CI

.github/workflows/ci.yml now:

1. Installs a pinned JJ_VERSION (0.40.0) from upstream releases.
2. Runs unit tests with -coverpkg=./..., producing cov-unit.out.
3. Runs integration tests with -tags=integration -coverpkg=./..., producing cov-integration.out.
4. Merges via go run ./scripts/coverage-merge and uploads all three profiles as a workflow artifact.

Documentation

• readme.md: new Jujutsu (jj) Support section integrated with your recent readme rewrite — covers setup, the jj spr command set, the divergences for edit and sync, the multi-head error, and the auto-revset behavior.
• docs/jj-mode-design.md: standalone design rationale for future contributors. Covers every decision (auto-revset, multi-head, spr edit/sync redesign, checkStackUsable, interface choices, the stderr fix, testing strategy).

Deferred / known limitations

Threads I deliberately kept out of scope — flagging them here so future contributors can find them:

• jj version matrix in CI. CI pins one JJ_VERSION (currently 0.40.0). A matrix across the last 2-3 minor versions would catch regressions when bumping. Triples CI time; not warranted until we have a concrete drift signal or jj 1.0 lands.
• GitHub-side PR verification. Integration tests use mockclient.MockClient — they verify spr's intent (which API calls in which order), not the GitHub-side outcome. A separate layer using a real GitHub test repo (or VCR-style fixtures) would close that gap. Big project (auth tokens, rate limits, cleanup, flake control). Defer until there's a concrete bug this would have caught.
• PushBranches with conflicted commits, GitHub side. TestJjIntegration_PushBranches_ConflictedCommit_Refused pins jj's local refusal to push conflicted commits. We don't verify GitHub's response to a half-pushed stack. Tied to the GitHub-side item above.
• spr update end-to-end orchestration tests with real jj. Reordering detection, WIP truncation at the spr layer, and --count boundary tests are currently exercised only via unit tests with mockgit+mockclient. Integration variants with real JjOps would lift confidence but the test code is fragile (full mockclient expectation sequences with synthetic github.PullRequest objects). Defer until a real bug appears in this seam.
• RunMergeCheck signal pipeline. Most of spr/spr.go::RunMergeCheck is uncovered today — it spawns a child process for the user-configured check command and handles SIGINT/SIGTERM. Testing this needs a controllable child process. Pre-existing gap, not introduced by this PR.
• git/helpers.go::GetLocalCommitStack and friends. The git-mode helpers (GetLocalBranchName, BranchNameFromCommit, GetLocalTopCommit, GetLocalCommitStack) currently have 0% direct coverage — they're exercised indirectly via mockgit but never against real git. A gittest package mirroring vcs/jjtest would close this; not a priority because git changes are rare. Pre-existing gap.
• addReviewers last ~9%. Probably an error path (GetAssignableUsers failure or assignee-not-found). Worth investigating when next touching that code. Pre-existing gap.

[Copilot]

Pull request overview

This PR integrates the jj-compat branch against latest upstream master, introducing a vcs.VCSOperations abstraction and a full Jujutsu (jj) backend (including fixtures, integration tests, and CI/coverage plumbing) so spr can operate safely in jj-colocated repositories while preserving jj change IDs.

Changes:

• Introduces vcs.VCSOperations with GitOps and JjOps, and wires spr commands through this abstraction (including stack-usability guarding for multi-head ambiguity).
• Adds jj command execution/parsing, jj-specific test fixtures (vcs/jjtest), plus unit + integration tests (build-tagged) and merged coverage tooling.
• Updates CLI/docs/CI to support jj mode (--no-jj, jj-setup, README + design doc, pinned jj version, coverage artifacts).

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
vcs/mockjj/mockjj.go	Adds expectation-based jj mock executor for unit tests.
vcs/jjtest/jjtest.go	Provides real jj+git colocated repo fixture + helpers for integration tests.
vcs/jjtest/doc.go	Documents jjtest fixture package usage and requirements.
vcs/jj_parse.go	Parses jj log output into commit structs (change IDs, trailers, WIP).
vcs/jj_parse_test.go	Unit tests for jj log parsing and trailer edge cases.
vcs/jj_ops.go	Implements VCSOperations using jj primitives (fetch/rebase/edit/squash/bookmarks).
vcs/jj_ops_test.go	Unit tests for JjOps behavior with mockjj.
vcs/jj_integration_test.go	End-to-end jj integration tests against a real jj binary.
vcs/jj_cmd.go	Executes jj with stdout/stderr separation to avoid output corruption.
vcs/jj_cmd_test.go	Tests around argument splitting limitations (strings.Fields) and usage guidance.
vcs/interface.go	Defines VCSOperations interface and repo-mode selection (NewVCSOperations).
vcs/git_ops.go	Extracts existing git logic into a GitOps implementation of VCSOperations.
vcs/git_ops_test.go	Unit tests for GitOps behavior (fetch/rebase, stash/no-op, etc.).
vcs/detect.go	Adds jj-colocated repo detection helper.
vcs/detect_test.go	Tests for detection and NewVCSOperations selection behavior.
spr/spr.go	Wires spr commands through VCSOperations; adds stack usability guard; jj-native edit/sync flows.
spr/spr_test.go	Adds tests for jj-vs-git UX paths (edit, sync, stack guard, merge-check gate, etc.).
spr/jj_integration_test.go	Integration tests for spr-layer behavior in a real jj repo.
scripts/coverage-merge/main.go	Adds tool to merge unit+integration coverprofiles and report attribution.
readme.md	Documents jj support, --no-jj, and jj-mode command semantics.
Makefile	Adds unit vs integration test targets and merged coverage report targets.
git/mockgit/mockgit.go	Adds ExpectFetchTags helper for tests.
git/helpers_test.go	Expands parser tests for git commit stack parsing edge cases.
git/commit.go	Adds ChangeID field to commit model for jj mode.
docs/jj-mode-design.md	Provides design rationale and safety model for jj support.
config/config.go	Adds User.NoJJ config knob (and another repo flag).
cmd/spr/main.go	Creates vcsOps, adds --no-jj and jj-setup command.
cmd/amend/main.go	Wires amend command through VCSOperations.
.github/workflows/ci.yml	Installs pinned jj and runs unit+integration coverage with merged reporting/artifacts.

Comments suppressed due to low confidence (2)

vcs/interface.go:48

• The jj-mode doc for EditAbort says jj op restore <saved-op-id>, but the current implementation makes EditAbort a no-op in jj mode and spr prints echo-only guidance instead. Please update this comment to match the implementation.

	// EditAbort cancels an edit session.
	// Git: git rebase --abort
	// jj:  jj op restore <saved-op-id>
	EditAbort() error

vcs/detect_test.go:43

• TestNewVCSOperations_JJRepo only sets up .jj/. If jj mode requires a colocated git repo, the fixture should create both .jj/ and .git/ so this test doesn’t encode the weaker “any jj repo” behavior.

	cfg := config.EmptyConfig()
	dir := t.TempDir()
	os.Mkdir(filepath.Join(dir, ".jj"), 0755)
	gitmock := &mockRootDirOnly{rootDir: dir}
	ops := NewVCSOperations(cfg, gitmock)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (2)

vcs/interface.go:48

• Interface comments for EditFinish/EditAbort describe jj implementations (jj new <original-@>, jj op restore <saved-op-id>), but the actual jj mode behavior is echo-only in the spr layer and JjOps.EditFinish/EditAbort are no-ops. Updating these comments would prevent contributors from implementing callers based on outdated semantics.

	// EditStart checks out a commit for editing (interactive edit session).
	// Git: git rebase -i with 'edit' stop
	// jj:  jj edit <change-id>
	EditStart(commit git.Commit) error

	// EditFinish completes an edit session.
	// Git: git add -A + git commit --amend --no-edit + git rebase --continue
	// jj:  jj new <original-@> (changes are auto-captured)
	EditFinish() error

	// EditAbort cancels an edit session.
	// Git: git rebase --abort
	// jj:  jj op restore <saved-op-id>
	EditAbort() error

vcs/interface.go:70

• CheckStackCompleteness’s comment still references mid-stack / descendant-exclusion scenarios (e.g. "@ has descendants in jj"), but jj mode now uses the connected-component revset and this method is primarily a multi-head (non-linear stack) detector. Please update the comment so callers understand what conditions it actually checks for in each VCS mode.

	// CheckStackCompleteness checks whether the current working copy position
	// might cause spr to see an incomplete stack. Returns a non-empty warning
	// string if there are commits that would be excluded (e.g. @ has descendants
	// in jj, or HEAD is detached in git). Returns "" if everything looks fine.
	CheckStackCompleteness() string