feat: extract merge operation to core/operations/merge.ts - #93
Open
2witstudios wants to merge 3 commits into
Open
feat: extract merge operation to core/operations/merge.ts#932witstudios wants to merge 3 commits into
2witstudios wants to merge 3 commits into
Conversation
Move merge orchestration (state transitions, git merge, cleanup) from commands/merge.ts into a reusable performMerge() function. The CLI command becomes a thin wrapper for arg parsing and output formatting. This enables both CLI and future REST routes to share the same merge logic with typed options and structured results. Closes #59
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughExtracted merge orchestration logic from the CLI command into a reusable Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Op as performMerge()
participant Manifest as Manifest
participant Worktree as Worktree Mgmt
participant Git as Git Ops
participant Agent as Agent Check
participant Cleanup as Cleanup Module
participant TMux as TMux/Panes
CLI->>Op: worktreeRef, strategy, cleanup, dryRun, force
Op->>Manifest: Load & refresh manifest
Op->>Worktree: Resolve worktree by ref
alt dryRun mode
Op-->>CLI: Return summary (no changes)
else normal flow
Op->>Agent: Check if agents running
alt force not set && agents running
Op-->>CLI: Return AGENTS_RUNNING error
else proceed
Manifest->>Manifest: Update state: active → merging
Op->>Git: Fetch + checkout base branch
Op->>Git: Perform merge (squash/no-ff)
alt merge fails
Manifest->>Manifest: Update state: merging → failed
Op-->>CLI: Return MergeFailedError
else merge succeeds
Manifest->>Manifest: Update state: merging → merged
alt cleanup enabled
Op->>TMux: Capture current pane context
Op->>Cleanup: cleanupWorktree(pane context)
Cleanup-->>Op: selfProtected flag
Manifest->>Manifest: Update state: merged → cleaned
else cleanup disabled
Op->>Manifest: Mark cleaned=false
end
Op-->>CLI: Return MergeResult
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/operations/merge.test.ts (2)
156-177: The transition test title includescleaned, but cleanup is not asserted.Either assert cleanup phase execution explicitly or rename the test title to avoid overstating coverage.
✏️ Minimal alignment change
- test('state transitions: active → merging → merged → cleaned', async () => { + test('state transitions: active → merging → merged', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/operations/merge.test.ts` around lines 156 - 177, The test title says it covers "cleaned" but there is no assertion that the worktree reached 'cleaned'; update the test in merge.test.ts to either assert the cleanup happened (e.g., add expect(statusLog).toContain('cleaned') and ensure its index is after 'merged') or change the test title to remove "→ cleaned"; locate the mocked updateManifest and performMerge invocation (and the cleanupWorktree behavior) to add the assertion or rename the test accordingly so the test title matches its assertions.
113-345: Align test case names with the repository’s naming convention.Current test titles are clear, but they don’t follow the
given X, should Ystyle required for test organization consistency.As per coding guidelines
src/**/*.test.ts: "Usedescribe('unitName')→test('given X, should Y')pattern for test organization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/operations/merge.test.ts` around lines 113 - 345, The tests in merge.test.ts use freeform titles and must follow the repository convention "given X, should Y"; rename each test title surrounding the performMerge cases to the `given ..., should ...` pattern (e.g., change "performs squash merge and returns result" to "given squash strategy, should perform merge and return result", "defaults to squash strategy" to "given no strategy, should default to squash", "supports no-ff merge strategy" to "given strategy no-ff, should perform no-ff merge", "state transitions: active → merging → merged → cleaned" to "given active worktree, should transition through merging→merged→cleaned", "sets status to failed on git merge error" to "given git merge conflicts, should set status to failed", "throws AGENTS_RUNNING when agents still running" to "given agents running, should throw AGENTS_RUNNING", "force bypasses running agent check" to "given force flag and running agents, should bypass running check and merge", "throws WorktreeNotFoundError for invalid ref" to "given invalid worktreeRef, should throw WorktreeNotFoundError", "dry run returns early without modifying state" to "given dryRun, should return early without state changes", "skips cleanup when cleanup=false" to "given cleanup=false, should skip cleanup", "switches to base branch if not already on it" to "given not on base branch, should checkout base branch", "skips checkout when already on base branch" to "given already on base branch, should skip checkout", "passes self-protection context to cleanup" to "given tmux context, should pass self-protection to cleanup", "reports selfProtected when cleanup skips targets" to "given cleanup reports selfProtected, should surface selfProtected in result", and "sets mergedAt timestamp on successful merge" to "given successful merge, should set mergedAt timestamp"; update only the test title strings (keep test bodies and references to performMerge, mockExeca, cleanupWorktree, getCurrentBranch, getCurrentPaneId, listSessionPanes, updateManifest unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/merge.ts`:
- Around line 17-57: The command currently always emits human logs
(info/success/warn) and returns early on dry-run before writing JSON; update the
post-merge flow to honor options.json: stop calling info/success/warn when
options.json is true and instead call output(...) with the same payloads;
specifically, for the dry-run branch (check result.dryRun) write a JSON payload
via output({ success: true, dryRun: true, worktreeId: result.worktreeId, branch:
result.branch, baseBranch: result.baseBranch, strategy: result.strategy,
cleaned: result.cleaned, selfProtected: result.selfProtected || undefined },
true) and return, and for the non-dry-run path replace the human success/cleanup
messages with a single output(...) call matching the schema already used
(include success, worktreeId, branch, baseBranch, strategy, cleaned,
selfProtected) when options.json is true; leave info/success/warn calls in place
only when options.json is falsy.
---
Nitpick comments:
In `@src/core/operations/merge.test.ts`:
- Around line 156-177: The test title says it covers "cleaned" but there is no
assertion that the worktree reached 'cleaned'; update the test in merge.test.ts
to either assert the cleanup happened (e.g., add
expect(statusLog).toContain('cleaned') and ensure its index is after 'merged')
or change the test title to remove "→ cleaned"; locate the mocked updateManifest
and performMerge invocation (and the cleanupWorktree behavior) to add the
assertion or rename the test accordingly so the test title matches its
assertions.
- Around line 113-345: The tests in merge.test.ts use freeform titles and must
follow the repository convention "given X, should Y"; rename each test title
surrounding the performMerge cases to the `given ..., should ...` pattern (e.g.,
change "performs squash merge and returns result" to "given squash strategy,
should perform merge and return result", "defaults to squash strategy" to "given
no strategy, should default to squash", "supports no-ff merge strategy" to
"given strategy no-ff, should perform no-ff merge", "state transitions: active →
merging → merged → cleaned" to "given active worktree, should transition through
merging→merged→cleaned", "sets status to failed on git merge error" to "given
git merge conflicts, should set status to failed", "throws AGENTS_RUNNING when
agents still running" to "given agents running, should throw AGENTS_RUNNING",
"force bypasses running agent check" to "given force flag and running agents,
should bypass running check and merge", "throws WorktreeNotFoundError for
invalid ref" to "given invalid worktreeRef, should throw WorktreeNotFoundError",
"dry run returns early without modifying state" to "given dryRun, should return
early without state changes", "skips cleanup when cleanup=false" to "given
cleanup=false, should skip cleanup", "switches to base branch if not already on
it" to "given not on base branch, should checkout base branch", "skips checkout
when already on base branch" to "given already on base branch, should skip
checkout", "passes self-protection context to cleanup" to "given tmux context,
should pass self-protection to cleanup", "reports selfProtected when cleanup
skips targets" to "given cleanup reports selfProtected, should surface
selfProtected in result", and "sets mergedAt timestamp on successful merge" to
"given successful merge, should set mergedAt timestamp"; update only the test
title strings (keep test bodies and references to performMerge, mockExeca,
cleanupWorktree, getCurrentBranch, getCurrentPaneId, listSessionPanes,
updateManifest unchanged).
Comment on lines
17
to
57
| if (options.dryRun) { | ||
| info('Dry run — no changes will be made'); | ||
| info(`Would merge branch ${wt.branch} into ${wt.baseBranch} using ${options.strategy ?? 'squash'} strategy`); | ||
| if (options.cleanup !== false) { | ||
| info(`Would remove worktree ${wt.id} and delete branch ${wt.branch}`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Set worktree status to merging | ||
| await updateManifest(projectRoot, (m) => { | ||
| if (m.worktrees[wt.id]) { | ||
| m.worktrees[wt.id].status = 'merging'; | ||
| } | ||
| return m; | ||
| const result = await performMerge({ | ||
| projectRoot, | ||
| worktreeRef: worktreeId, | ||
| strategy: options.strategy, | ||
| cleanup: options.cleanup, | ||
| dryRun: options.dryRun, | ||
| force: options.force, | ||
| }); | ||
|
|
||
| const strategy = options.strategy ?? 'squash'; | ||
|
|
||
| try { | ||
| const currentBranch = await getCurrentBranch(projectRoot); | ||
| if (currentBranch !== wt.baseBranch) { | ||
| info(`Switching to base branch ${wt.baseBranch}`); | ||
| await execa('git', ['checkout', wt.baseBranch], { ...execaEnv, cwd: projectRoot }); | ||
| } | ||
|
|
||
| info(`Merging ${wt.branch} into ${wt.baseBranch} (${strategy})`); | ||
|
|
||
| if (strategy === 'squash') { | ||
| await execa('git', ['merge', '--squash', wt.branch], { ...execaEnv, cwd: projectRoot }); | ||
| await execa('git', ['commit', '-m', `ppg: merge ${wt.name} (${wt.branch})`], { | ||
| ...execaEnv, | ||
| cwd: projectRoot, | ||
| }); | ||
| } else { | ||
| await execa('git', ['merge', '--no-ff', wt.branch, '-m', `ppg: merge ${wt.name} (${wt.branch})`], { | ||
| ...execaEnv, | ||
| cwd: projectRoot, | ||
| }); | ||
| if (result.dryRun) { | ||
| info(`Would merge branch ${result.branch} into ${result.baseBranch} using ${result.strategy} strategy`); | ||
| if (options.cleanup !== false) { | ||
| info(`Would remove worktree ${result.worktreeId} and delete branch ${result.branch}`); | ||
| } | ||
|
|
||
| success(`Merged ${wt.branch} into ${wt.baseBranch}`); | ||
| } catch (err) { | ||
| await updateManifest(projectRoot, (m) => { | ||
| if (m.worktrees[wt.id]) { | ||
| m.worktrees[wt.id].status = 'failed'; | ||
| } | ||
| return m; | ||
| }); | ||
| throw new MergeFailedError( | ||
| `Merge failed: ${err instanceof Error ? err.message : err}`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Mark as merged | ||
| await updateManifest(projectRoot, (m) => { | ||
| if (m.worktrees[wt.id]) { | ||
| m.worktrees[wt.id].status = 'merged'; | ||
| m.worktrees[wt.id].mergedAt = new Date().toISOString(); | ||
| } | ||
| return m; | ||
| }); | ||
|
|
||
| // Cleanup with self-protection | ||
| let selfProtected = false; | ||
| if (options.cleanup !== false) { | ||
| info('Cleaning up...'); | ||
|
|
||
| const selfPaneId = getCurrentPaneId(); | ||
| let paneMap: Map<string, PaneInfo> | undefined; | ||
| if (selfPaneId) { | ||
| paneMap = await listSessionPanes(manifest.sessionName); | ||
| } | ||
|
|
||
| const cleanupResult = await cleanupWorktree(projectRoot, wt, { selfPaneId, paneMap }); | ||
| selfProtected = cleanupResult.selfProtected; | ||
| success(`Merged ${result.branch} into ${result.baseBranch}`); | ||
|
|
||
| if (selfProtected) { | ||
| warn(`Some tmux targets skipped during cleanup — contains current ppg process`); | ||
| if (result.cleaned) { | ||
| if (result.selfProtected) { | ||
| warn('Some tmux targets skipped during cleanup — contains current ppg process'); | ||
| } | ||
| success(`Cleaned up worktree ${wt.id}`); | ||
| success(`Cleaned up worktree ${result.worktreeId}`); | ||
| } | ||
|
|
||
| if (options.json) { | ||
| output({ | ||
| success: true, | ||
| worktreeId: wt.id, | ||
| branch: wt.branch, | ||
| baseBranch: wt.baseBranch, | ||
| strategy, | ||
| cleaned: options.cleanup !== false, | ||
| selfProtected: selfProtected || undefined, | ||
| worktreeId: result.worktreeId, | ||
| branch: result.branch, | ||
| baseBranch: result.baseBranch, | ||
| strategy: result.strategy, | ||
| cleaned: result.cleaned, | ||
| selfProtected: result.selfProtected || undefined, | ||
| }, true); | ||
| } |
There was a problem hiding this comment.
--json mode currently emits human logs and skips dry-run JSON output.
info/success/warn are emitted even when options.json is true, and the dry-run branch returns before the JSON payload is written. This breaks machine-readable command output.
✅ Suggested flow fix
export async function mergeCommand(worktreeId: string, options: MergeCommandOptions): Promise<void> {
const projectRoot = await getRepoRoot();
- const strategy = options.strategy ?? 'squash';
- if (options.dryRun) {
+ if (options.dryRun && !options.json) {
info('Dry run — no changes will be made');
}
const result = await performMerge({
projectRoot,
worktreeRef: worktreeId,
strategy: options.strategy,
cleanup: options.cleanup,
dryRun: options.dryRun,
force: options.force,
});
+ if (options.json) {
+ output({
+ success: true,
+ worktreeId: result.worktreeId,
+ branch: result.branch,
+ baseBranch: result.baseBranch,
+ strategy: result.strategy,
+ dryRun: result.dryRun,
+ merged: result.merged,
+ cleaned: result.cleaned,
+ selfProtected: result.selfProtected || undefined,
+ }, true);
+ return;
+ }
+
if (result.dryRun) {
info(`Would merge branch ${result.branch} into ${result.baseBranch} using ${result.strategy} strategy`);
if (options.cleanup !== false) {
info(`Would remove worktree ${result.worktreeId} and delete branch ${result.branch}`);
}
return;
}
success(`Merged ${result.branch} into ${result.baseBranch}`);
if (result.cleaned) {
if (result.selfProtected) {
warn('Some tmux targets skipped during cleanup — contains current ppg process');
}
success(`Cleaned up worktree ${result.worktreeId}`);
}
-
- if (options.json) {
- output({
- success: true,
- worktreeId: result.worktreeId,
- branch: result.branch,
- baseBranch: result.baseBranch,
- strategy: result.strategy,
- cleaned: result.cleaned,
- selfProtected: result.selfProtected || undefined,
- }, true);
- }
}As per coding guidelines src/commands/**/*.ts: "Support --json flag on every command and use output(data, json) and outputError(error, json) for dual output".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/merge.ts` around lines 17 - 57, The command currently always
emits human logs (info/success/warn) and returns early on dry-run before writing
JSON; update the post-merge flow to honor options.json: stop calling
info/success/warn when options.json is true and instead call output(...) with
the same payloads; specifically, for the dry-run branch (check result.dryRun)
write a JSON payload via output({ success: true, dryRun: true, worktreeId:
result.worktreeId, branch: result.branch, baseBranch: result.baseBranch,
strategy: result.strategy, cleaned: result.cleaned, selfProtected:
result.selfProtected || undefined }, true) and return, and for the non-dry-run
path replace the human success/cleanup messages with a single output(...) call
matching the schema already used (include success, worktreeId, branch,
baseBranch, strategy, cleaned, selfProtected) when options.json is true; leave
info/success/warn calls in place only when options.json is falsy.
- Remove unused imports (type MergeResult, WorktreeEntry, Manifest, CleanupResult) - Remove unused `strategy` variable in commands/merge.ts - Fix AGENTS_RUNNING test: single invocation instead of double call - Fix misleading comment about cleanup state transition in mocked test - Remove unnecessary vi.mock passthrough for errors.js
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.
Summary
commands/merge.tsintocore/operations/merge.tsperformMerge()function with typedMergeOptions/MergeResultinterfaces, usable by both CLI and future REST routescommands/merge.tssimplified to a thin wrapper (~50 lines) that handles arg parsing and output formattingTest plan
npm run typecheckpasses (no new errors)core/operations/merge.test.tspasscron-parserdep are unrelated)Closes #59
Summary by CodeRabbit
Refactor
Tests