Skip to content

Commit db633d1

Browse files
henrikjeclaude
andcommitted
fix(rebase): add missing dirty check for merged-new-work branches
When a branch was merged into its base with new commits on top, assessMergedNewWork promoted the assessment to will-operate without checking for dirty working tree. This caused git rebase to refuse (exit 1) and arb to misinterpret it as a conflict. Also adds defensive detectOperation check after non-zero git exit so errors without actual conflict state are reported as errors (not conflicts), and tracks errored repos in the summary line. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9c12a42 commit db633d1

4 files changed

Lines changed: 299 additions & 17 deletions

File tree

src/lib/sync/classify-integrate.test.ts

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,138 @@ describe("assessIntegrateRepo", () => {
494494
expect(a.skipFlag).toBe("already-merged");
495495
});
496496

497+
// ── merged-new-work dirty checks ──
498+
499+
test("merged-new-work with dirty worktree and no autostash returns skip (dirty)", async () => {
500+
const status = makeRepo({
501+
base: {
502+
remote: "origin",
503+
ref: "main",
504+
configuredRef: null,
505+
resolvedVia: "remote",
506+
ahead: 4,
507+
behind: 3,
508+
merge: { kind: "merge", newCommitsAfter: 2 },
509+
baseMergedIntoDefault: null,
510+
},
511+
local: { staged: 1, modified: 0, untracked: 0, conflicts: 0 },
512+
});
513+
const a = await assessIntegrateRepo(
514+
status,
515+
DIR,
516+
"feature",
517+
[],
518+
defaultOptions({ autostash: false, mode: "rebase" }),
519+
mockDeps({ git: async () => ({ exitCode: 0, stdout: "deadbeef\n", stderr: "" }) }),
520+
);
521+
expect(a.outcome).toBe("skip");
522+
expect(a.skipFlag).toBe("dirty");
523+
});
524+
525+
test("merged-new-work with dirty worktree and autostash returns will-operate with needsStash (rebase)", async () => {
526+
const status = makeRepo({
527+
base: {
528+
remote: "origin",
529+
ref: "main",
530+
configuredRef: null,
531+
resolvedVia: "remote",
532+
ahead: 4,
533+
behind: 3,
534+
merge: { kind: "merge", newCommitsAfter: 2 },
535+
baseMergedIntoDefault: null,
536+
},
537+
local: { staged: 1, modified: 0, untracked: 0, conflicts: 0 },
538+
});
539+
const a = await assessIntegrateRepo(
540+
status,
541+
DIR,
542+
"feature",
543+
[],
544+
defaultOptions({ autostash: true, mode: "rebase" }),
545+
mockDeps({ git: async () => ({ exitCode: 0, stdout: "deadbeef\n", stderr: "" }) }),
546+
);
547+
expect(a.outcome).toBe("will-operate");
548+
expect(a.needsStash).toBe(true);
549+
expect(a.retarget?.from).toBe("deadbeef");
550+
});
551+
552+
test("merged-new-work with dirty worktree and autostash returns will-operate with needsStash (merge)", async () => {
553+
const status = makeRepo({
554+
base: {
555+
remote: "origin",
556+
ref: "main",
557+
configuredRef: null,
558+
resolvedVia: "remote",
559+
ahead: 4,
560+
behind: 3,
561+
merge: { kind: "merge", newCommitsAfter: 2 },
562+
baseMergedIntoDefault: null,
563+
},
564+
local: { staged: 0, modified: 1, untracked: 0, conflicts: 0 },
565+
});
566+
const a = await assessIntegrateRepo(
567+
status,
568+
DIR,
569+
"feature",
570+
[],
571+
defaultOptions({ autostash: true, mode: "merge" }),
572+
mockDeps(),
573+
);
574+
expect(a.outcome).toBe("will-operate");
575+
expect(a.needsStash).toBe(true);
576+
});
577+
578+
test("merged-new-work in merge mode with behind=0 returns up-to-date even when dirty", async () => {
579+
const status = makeRepo({
580+
base: {
581+
remote: "origin",
582+
ref: "main",
583+
configuredRef: null,
584+
resolvedVia: "remote",
585+
ahead: 4,
586+
behind: 0,
587+
merge: { kind: "merge", newCommitsAfter: 2 },
588+
baseMergedIntoDefault: null,
589+
},
590+
local: { staged: 1, modified: 0, untracked: 0, conflicts: 0 },
591+
});
592+
const a = await assessIntegrateRepo(
593+
status,
594+
DIR,
595+
"feature",
596+
[],
597+
defaultOptions({ autostash: false, mode: "merge" }),
598+
mockDeps(),
599+
);
600+
expect(a.outcome).toBe("up-to-date");
601+
});
602+
603+
test("merged-new-work with untracked-only changes and autostash does not set needsStash", async () => {
604+
const status = makeRepo({
605+
base: {
606+
remote: "origin",
607+
ref: "main",
608+
configuredRef: null,
609+
resolvedVia: "remote",
610+
ahead: 4,
611+
behind: 3,
612+
merge: { kind: "merge", newCommitsAfter: 2 },
613+
baseMergedIntoDefault: null,
614+
},
615+
local: { staged: 0, modified: 0, untracked: 3, conflicts: 0 },
616+
});
617+
const a = await assessIntegrateRepo(
618+
status,
619+
DIR,
620+
"feature",
621+
[],
622+
defaultOptions({ autostash: true, mode: "rebase" }),
623+
mockDeps({ git: async () => ({ exitCode: 0, stdout: "deadbeef\n", stderr: "" }) }),
624+
);
625+
expect(a.outcome).toBe("will-operate");
626+
expect(a.needsStash).toBeUndefined();
627+
});
628+
497629
test("baseMergedIntoDefault returns skip", async () => {
498630
const status = makeRepo({
499631
base: {

src/lib/sync/classify-integrate.ts

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,9 @@ export async function assessIntegrateRepo(
177177
mode: options.mode,
178178
repoDir,
179179
deps,
180+
status,
181+
branch,
182+
autostash: options.autostash,
180183
});
181184
if (mergedNewWorkAssessment) return mergedNewWorkAssessment;
182185

@@ -283,26 +286,46 @@ async function assessMergedNewWork(input: {
283286
mode: IntegrateMode;
284287
repoDir: string;
285288
deps: IntegrateClassifierDependencies;
289+
status: RepoStatus;
290+
branch: string;
291+
autostash: boolean;
286292
}): Promise<RepoAssessment | null> {
287-
const { classified, base, isMergedNewWork, mode, repoDir, deps } = input;
293+
const { classified, base, isMergedNewWork, mode, repoDir, deps, status, branch, autostash } = input;
288294
if (!isMergedNewWork || !base) return null;
289295

296+
// Up-to-date check before dirty (per GUIDELINES.md § Classification check order).
297+
// Only merge mode has an up-to-date case — rebase always retargets.
298+
if (mode === "merge" && base.behind === 0) {
299+
return {
300+
...withoutSkipFields(classified),
301+
outcome: "up-to-date",
302+
baseBranch: base.ref,
303+
behind: 0,
304+
ahead: base.ahead,
305+
};
306+
}
307+
308+
// All remaining paths need work — check dirty before will-operate
309+
const flags = computeFlags(status, branch);
310+
if (flags.isDirty && !autostash) {
311+
return {
312+
...classified,
313+
outcome: "skip" as const,
314+
skipReason: "uncommitted changes (use --autostash)",
315+
skipFlag: "dirty" as const,
316+
};
317+
}
318+
const needsStash =
319+
flags.isDirty && autostash && (status.local.staged > 0 || status.local.modified > 0) ? true : undefined;
320+
290321
if (mode === "merge") {
291-
if (base.behind === 0) {
292-
return {
293-
...withoutSkipFields(classified),
294-
outcome: "up-to-date",
295-
baseBranch: base.ref,
296-
behind: 0,
297-
ahead: base.ahead,
298-
};
299-
}
300322
return {
301323
...withoutSkipFields(classified),
302324
outcome: "will-operate",
303325
baseBranch: base.ref,
304326
behind: base.behind,
305327
ahead: base.ahead,
328+
needsStash,
306329
};
307330
}
308331

@@ -316,6 +339,7 @@ async function assessMergedNewWork(input: {
316339
baseBranch: base.ref,
317340
behind: base.behind,
318341
ahead: replayCount,
342+
needsStash,
319343
retarget: {
320344
from: boundarySha,
321345
to: base.ref,

src/lib/sync/integrate.ts

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,14 @@ import {
1515
withReflogAction,
1616
writeOperationRecord,
1717
} from "../core/operation";
18-
import { getCommitsBetweenFull, getDiffShortstat, getMergeBase, gitLocal, parseGitStatus } from "../git/git";
18+
import {
19+
detectOperation,
20+
getCommitsBetweenFull,
21+
getDiffShortstat,
22+
getMergeBase,
23+
gitLocal,
24+
parseGitStatus,
25+
} from "../git/git";
1926
import type { GitCache } from "../git/git-cache";
2027
import { buildConflictReport, buildStashPopFailureReport } from "../render/conflict-report";
2128
import { type IntegrateActionDesc, integrateActionCell } from "../render/integrate-cells";
@@ -232,6 +239,7 @@ export async function integrate(
232239

233240
// Phase 6: execute sequentially
234241
let succeeded = 0;
242+
let errored = 0;
235243
const conflicted: { assessment: RepoAssessment; stdout: string; stderr: string }[] = [];
236244
const stashPopFailed: RepoAssessment[] = [];
237245
await withReflogAction(`arb-${mode}`, async () => {
@@ -313,17 +321,25 @@ export async function integrate(
313321
inlineResult(a.repo, doneMsg);
314322
succeeded++;
315323
} else {
316-
// For rebase mode, git rebase --autostash handles stash internally.
317-
// For merge mode with stash, do NOT pop if merge conflicted.
324+
// Check if git actually entered conflict state. If no rebase/merge is in progress,
325+
// this was an error (dirty tree, timeout, etc.) — not a conflict that --continue can resolve.
326+
const op = await detectOperation(a.repoDir);
327+
const isActualConflict = op === "rebase" || op === "merge";
318328
const existing = record.repos[a.repo];
319329
if (existing) {
320330
const errorOutput = result.stderr.trim().slice(0, 4000) || undefined;
321-
record.repos[a.repo] = { ...existing, status: "conflicting", errorOutput };
331+
record.repos[a.repo] = { ...existing, status: isActualConflict ? "conflicting" : "skipped", errorOutput };
322332
}
323333
writeOperationRecord(wsDir, record);
324334

325-
inlineResult(a.repo, yellow("conflict"));
326-
conflicted.push({ assessment: a, stdout: result.stdout, stderr: result.stderr });
335+
if (isActualConflict) {
336+
inlineResult(a.repo, yellow("conflict"));
337+
conflicted.push({ assessment: a, stdout: result.stdout, stderr: result.stderr });
338+
} else {
339+
const firstLine = result.stderr.trim().split("\n")[0] ?? "git failed";
340+
inlineResult(a.repo, yellow(`error: ${firstLine}`));
341+
errored++;
342+
}
327343
}
328344
}
329345
});
@@ -369,10 +385,11 @@ export async function integrate(
369385
const parts: string[] = [];
370386
parts.push(`${verbed} ${plural(succeeded, "repo")}`);
371387
if (conflicted.length > 0) parts.push(`${conflicted.length} conflicted`);
388+
if (errored > 0) parts.push(`${errored} errored`);
372389
if (stashPopFailed.length > 0) parts.push(`${stashPopFailed.length} stash pop failed`);
373390
if (upToDate.length > 0) parts.push(`${upToDate.length} up to date`);
374391
if (skipped.length > 0) parts.push(`${skipped.length} skipped`);
375-
finishSummary(parts, conflicted.length > 0 || stashPopFailed.length > 0);
392+
finishSummary(parts, conflicted.length > 0 || stashPopFailed.length > 0 || errored > 0);
376393
}
377394

378395
export async function maybeWriteBaseFallbackConfig(options: {

test/integration/rebase-operation.test.ts

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,115 @@ describe("rebase autostash + conflict + undo", () => {
567567
}));
568568
});
569569

570+
// ── merged-new-work + dirty tree ────────────────────────────────
571+
572+
/**
573+
* Simulate a merge on the bare remote: clone, merge source into target, push.
574+
*/
575+
async function simulateMerge(
576+
env: { testDir: string; originDir: string },
577+
repo: string,
578+
source: string,
579+
target: string,
580+
) {
581+
const bareDir = join(env.originDir, `${repo}.git`);
582+
const tmpClone = join(env.testDir, `tmp-merge-${repo}-${Date.now()}`);
583+
await git(env.testDir, ["clone", bareDir, tmpClone]);
584+
await git(tmpClone, ["checkout", target]);
585+
await git(tmpClone, ["merge", `origin/${source}`]);
586+
await git(tmpClone, ["push"]);
587+
}
588+
589+
describe("merged-new-work with dirty tree", () => {
590+
/** Advance main via a temp clone (avoids stale canonical repo after simulateMerge). */
591+
async function advanceMainViaClone(
592+
env: { testDir: string; originDir: string },
593+
repo: string,
594+
file: string,
595+
content: string,
596+
) {
597+
const tmpClone = join(env.testDir, `tmp-advance-${repo}-${Date.now()}`);
598+
await git(env.testDir, ["clone", join(env.originDir, `${repo}.git`), tmpClone]);
599+
await git(tmpClone, ["checkout", "main"]);
600+
await write(join(tmpClone, file), content);
601+
await git(tmpClone, ["add", file]);
602+
await git(tmpClone, ["commit", "-m", `advance main: ${file}`]);
603+
await git(tmpClone, ["push"]);
604+
}
605+
606+
test("merged-new-work with dirty tree and no autostash skips (not a false conflict)", () =>
607+
withEnv(async (env) => {
608+
await arb(env, ["create", "my-feature", "repo-a", "--base", "main"]);
609+
const ws = join(env.projectDir, "my-feature");
610+
const wt = join(ws, "repo-a");
611+
612+
// Make a commit on feature and push so remote knows about the branch
613+
await write(join(wt, "feature.txt"), "feature");
614+
await git(wt, ["add", "feature.txt"]);
615+
await git(wt, ["commit", "-m", "feature"]);
616+
await git(wt, ["push", "-u", "origin", "my-feature"]);
617+
618+
// Simulate merge of feature branch into main on the remote
619+
await simulateMerge(env, "repo-a", "my-feature", "main");
620+
621+
// Add new commit after the merge (the "new work")
622+
await write(join(wt, "new-feature.txt"), "new feature");
623+
await git(wt, ["add", "new-feature.txt"]);
624+
await git(wt, ["commit", "-m", "new feature work"]);
625+
626+
// Advance main separately so there's behind count
627+
await advanceMainViaClone(env, "repo-a", "main-advance.txt", "main advance");
628+
629+
// Create dirty changes (unstaged modified file)
630+
await write(join(wt, "feature.txt"), "dirty modification");
631+
632+
// Rebase without --autostash — should skip with "uncommitted changes", not report false conflict
633+
const r = await arb(env, ["rebase", "--yes"], { cwd: ws });
634+
const output = r.stderr;
635+
expect(output).toContain("uncommitted changes");
636+
expect(output).not.toContain("conflict");
637+
}));
638+
639+
test("merged-new-work with dirty tree and --autostash succeeds", () =>
640+
withEnv(async (env) => {
641+
await arb(env, ["create", "my-feature", "repo-a", "--base", "main"]);
642+
const ws = join(env.projectDir, "my-feature");
643+
const wt = join(ws, "repo-a");
644+
645+
// Make a commit on feature and push
646+
await write(join(wt, "feature.txt"), "feature");
647+
await git(wt, ["add", "feature.txt"]);
648+
await git(wt, ["commit", "-m", "feature"]);
649+
await git(wt, ["push", "-u", "origin", "my-feature"]);
650+
651+
// Simulate merge of feature branch into main on the remote
652+
await simulateMerge(env, "repo-a", "my-feature", "main");
653+
654+
// Add new commit after the merge
655+
await write(join(wt, "new-feature.txt"), "new feature");
656+
await git(wt, ["add", "new-feature.txt"]);
657+
await git(wt, ["commit", "-m", "new feature work"]);
658+
659+
// Advance main separately
660+
await advanceMainViaClone(env, "repo-a", "main-advance.txt", "main advance");
661+
662+
// Create dirty changes on a file not touched by rebased commits
663+
await write(join(wt, "dirty.txt"), "uncommitted work");
664+
await git(wt, ["add", "dirty.txt"]);
665+
666+
// Rebase with --autostash — should succeed
667+
const r = await arb(env, ["rebase", "--yes", "--autostash"], { cwd: ws });
668+
if (r.exitCode !== 0) {
669+
throw new Error(`arb rebase failed (exit ${r.exitCode}):\nstderr: ${r.stderr}\nstdout: ${r.stdout}`);
670+
}
671+
expect(r.stderr).toContain("Rebased 1 repo");
672+
expect(r.stderr).not.toContain("conflicted");
673+
674+
// Dirty file should be preserved
675+
expect(existsSync(join(wt, "dirty.txt"))).toBe(true);
676+
}));
677+
});
678+
570679
// ── continue-then-undo end-to-end ────────────────────────────────
571680

572681
describe("rebase continue then undo", () => {

0 commit comments

Comments
 (0)