Skip to content

refactor(server): extract the change-request step - #20752

Draft
THEjacob1000 wants to merge 4 commits into
pingdotgg:mainfrom
THEjacob1000:refactor/change-request-step
Draft

THEjacob1000 wants to merge 4 commits into
pingdotgg:mainfrom
THEjacob1000:refactor/change-request-step

Conversation

@THEjacob1000

@THEjacob1000 THEjacob1000 commented Oct 17, 2026 •

Copy link
Copy Markdown

Problem

PR creation lives inside GitManager's stacked action, along with Git-specific reads. Wiring jj into it directly would duplicate provider selection, PR reuse and generated text. This extracts that policy without changing Git behavior.

Change

The change-request step moves to sourceControl/ChangeRequestStep.ts behind ChangeRequestVcsReads, so jj doesn't import the GitManager module to get it. It owns provider lookup, existing-PR detection, fork-aware head/base selection, template and history context, generated title/body and progress reporting. Git supplies the same reads as before and #20753 supplies jj ones.

@omarshaarawi pointed out a mechanical rebase would silently drop #20412 or #19605, so this was re-extracted from main's current bodies instead, and both are in: fork detection with HostProcess.Environment and gh-resolved, and includePatch in prepareCommitContext. The unused exports and the mock-only test are gone. The only Git-visible deltas are trace span names. No Git commands change.

Scope and approval

Approval is pending in #18468. Commits 1 to 6 are dormant on their own, #20756 is the one that turns jj on, and #20755 (idle workspace cleanup) sits on top so it can wait until the rest lands. Tracks #1126, the originals #11373, #11382, #11383, #11384, and ports #13816, #16380. @gergesh's port was a reference.

GitHub can't create stacked PRs from a fork, so the diff carries the PRs below. Review the top commit only.

Verification

Old and new PR steps were replayed side by side with real Git, a local bare remote and a fake gh: creation, existing-PR reuse, failure cleanup and dirty worktrees matched, and #20412's routing matched across seven remote setups. This PR has no UI of its own, so there are no screenshots.

@omarshaarawi

Copy link
Copy Markdown

Behavior looks neutral for git apart from trace span names and projectSettingsFor now forwarding the whole input.

The problem is the rebase. Main has #20412 (fork detection needs HostProcess.Environment and gh-resolved) and #19605 (prepareCommitContext grew includePatch). The extracted step has no way to express either, so a mechanical rebase drops one of them without a conflict. Diff the extracted bodies against main, not against the parent.

Also: ~14 new exports have no user anywhere in the stack. The step lives in the 3.3k-line GitManager.ts, so jj imports the git module to get it; sourceControl/ChangeRequestStep.ts would fix that. changeRequestStep.test.ts is all Layer.mock.

@THEjacob1000
THEjacob1000 force-pushed the refactor/change-request-step branch 2 times, most recently from 02e40df to fe50bb6 Compare October 19, 2026 18:00
THEjacob1000 and others added 4 commits October 20, 2026 06:45
Co-authored-by: Лев Федорец <152093961+knocking4thcylinder@users.noreply.github.com>
Co-authored-by: Лев Федорец <152093961+knocking4thcylinder@users.noreply.github.com>
Co-authored-by: Omar Shaarawi <16183117+omarshaarawi@users.noreply.github.com>
…vcs kind

Co-authored-by: Лев Федорец <152093961+knocking4thcylinder@users.noreply.github.com>
@THEjacob1000
THEjacob1000 force-pushed the refactor/change-request-step branch from fe50bb6 to 503cd4e Compare October 19, 2026 20:12

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants