Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions .reports/dub-61-adversarial-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Adversarial Review - DUB-61

Target: staged diff against `origin/main`
Focus: correctness, branch-safety side effects, test coverage
Review artifact: `/tmp/adversarial-review-h0FXjd/diff.patch`

## Summary

Iterations: 1

- Critical: 0 remaining
- Major: 0 remaining
- Minor: 0 remaining
- Nitpick: 0 remaining

## Review Notes

Reviewer A focus: command safety and mutation ordering.

- Confirmed the shared guard runs before undo entries, cleanup journals, state
writes, branch rewrites, submit pushes, and PR mutations for the mutating
paths covered by DUB-61.
- Flagged one no-op nuance in `move`: refusing a branch checked out elsewhere
before the existing no-op return would make a non-mutating no-op stricter than
necessary. Fixed before this report by moving the guard after the no-op
branch.

Reviewer B focus: coverage and user-facing recovery.

- Confirmed the new regression suite covers every named Tier 3 command plus
submit with real sibling git worktrees.
- Confirmed the `DubError.recovery` hints include the exact worktree path.
- Confirmed existing fold parent-worktree coverage was updated to the shared
error wording.

## Remaining Findings

None.

## Verification After Review

- `pnpm checks` passed.
- `pnpm typecheck` passed.
- `pnpm test` passed.
33 changes: 33 additions & 0 deletions .reports/dub-61-qa.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Self-QA fallback - DUB-61

> This work item has no useful browser recording surface, so this file replaces
> the video and records deterministic proof instead.

## Why no video

DUB-61 changes TypeScript CLI branch-safety behavior only. No `.tsx` files or
browser-demoable surfaces changed, so the useful proof is command-level test
coverage and repo gates.

## What was verified

- A shared worktree guard refuses branch mutations before undo, journal, state,
push, or PR side effects.
- Tier 3 commands `split`, `absorb`, `squash`, `fold`, `pop`, `rename`, `move`,
`reorder`, and `unlink` refuse when their target branch is checked out in a
sibling worktree.
- `submit` refuses before pushing when a branch in its resolved submit scope is
checked out in a sibling worktree.
- The `DubError` recovery hints include the exact sibling worktree path.

## Evidence

- `pnpm checks` passed.
- `pnpm typecheck` passed.
- `pnpm test` passed: 124 test files, 1354 tests.
- Focused regression coverage: `packages/cli/src/commands/worktree-guards.test.ts`
passed all 11 scenarios using real git worktrees.

## Follow-up flag

None.
15 changes: 15 additions & 0 deletions packages/cli/src/commands/absorb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
topologicalOrder,
} from '../lib/state';
import { saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';
import { restack } from './restack';

export type AbsorbMode = 'auto' | 'ai' | 'stack';
Expand Down Expand Up @@ -146,6 +147,14 @@ export async function absorb(

const mode: AbsorbMode = options.stack ? 'stack' : options.ai ? 'ai' : 'auto';

if (!options.dryRun && mode !== 'stack') {
await assertBranchesNotCheckedOutElsewhere(
cwd,
[originalBranch],
'dub absorb',
);
}

await saveUndoEntry(
{
operation: 'absorb',
Expand Down Expand Up @@ -385,6 +394,12 @@ async function runStackMode(
};
}

await assertBranchesNotCheckedOutElsewhere(
cwd,
crossFixups.flatMap((fixup) => [fixup.sourceBranch, fixup.targetBranch]),
'dub absorb --stack',
);

await writeAbsorbProgress(cwd, {
mode: 'stack',
originalBranch,
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/commands/move.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ vi.mock('../lib/git.js', async () => {
getBranchTip: vi.fn(),
getCurrentBranch: vi.fn(),
isWorkingTreeClean: vi.fn(),
listWorktreeCheckouts: vi.fn(),
};
});

Expand Down Expand Up @@ -52,6 +53,7 @@ import {
getBranchTip,
getCurrentBranch,
isWorkingTreeClean,
listWorktreeCheckouts,
} from '../lib/git';
import {
checkGhAuth,
Expand Down Expand Up @@ -88,6 +90,7 @@ function makeState(specs: BranchSpec[]): DubState {
beforeEach(() => {
vi.clearAllMocks();
vi.mocked(isWorkingTreeClean).mockResolvedValue(true);
vi.mocked(listWorktreeCheckouts).mockResolvedValue(new Map());
vi.mocked(branchExists).mockResolvedValue(true);
vi.mocked(getCurrentBranch).mockResolvedValue('main');
vi.mocked(getBranchTip).mockImplementation(async (name) => `sha:${name}`);
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/src/commands/move.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
writeState,
} from '../lib/state';
import { saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';
import { restack } from './restack';

export interface MoveOptions {
Expand Down Expand Up @@ -195,6 +196,11 @@ export async function move(
noOpReason: reason,
};
}
await assertBranchesNotCheckedOutElsewhere(
cwd,
[branch, target, ...reparents.map((reparent) => reparent.branch)],
'dub move',
);

// Validate the planned mutation is acyclic on a clone before touching disk.
const probeStack: Stack = structuredClone(stack);
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/commands/pop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '../lib/git';
import { getParent, readState } from '../lib/state';
import { saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';

interface PopOptions {
/** Number of commits to pop. Defaults to 1. */
Expand Down Expand Up @@ -63,6 +64,7 @@ export async function pop(
"Run 'dub log' to inspect the stack and confirm tracking state.",
]);
}
await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub pop');

const branchCommitCount = await countCommitsAhead(branch, parent, cwd);
if (branchCommitCount === 0) {
Expand Down
17 changes: 2 additions & 15 deletions packages/cli/src/commands/rename.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import {
getCurrentBranch,
isValidBranchName,
lastPushedRef,
listWorktreeCheckouts,
pushBranch,
readLastPushedSha,
renameBranch,
writeLastPushedSha,
} from '../lib/git';
import { findStackForBranch, readState, writeState } from '../lib/state';
import { clearUndoEntry, saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';

interface RenameOptions {
/**
Expand Down Expand Up @@ -119,20 +119,7 @@ export async function rename(
]);
}

// Refuse to rename a branch that's checked out in another worktree —
// `git branch -m` would fail mid-operation, and undo would point to a
// rename that never happened.
const worktreeCheckouts = await listWorktreeCheckouts(cwd);
const otherWorktree = worktreeCheckouts.get(oldName);
if (otherWorktree) {
throw new DubError(
`Branch '${oldName}' is checked out in another worktree (${otherWorktree}).`,
[
`Run 'git worktree remove ${otherWorktree}' to drop the worktree, then retry.`,
`Switch off '${oldName}' in the other worktree, then retry.`,
],
);
}
await assertBranchesNotCheckedOutElsewhere(cwd, [oldName], 'dub rename');

const childBranches = sourceStack.branches.filter(
(b) => b.parent === oldName,
Expand Down
29 changes: 6 additions & 23 deletions packages/cli/src/commands/reorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@ import chalk from 'chalk';
import { DubError } from '../lib/errors';
import { execa } from '../lib/exec';
import {
formatWorktreeCheckoutSkipMessage,
getBranchTip,
getCurrentBranch,
getMergeBase,
isWorkingTreeClean,
listWorktreeCheckouts,
} from '../lib/git';
import {
buildRebaseTodo,
Expand All @@ -27,6 +25,7 @@ import {
import { rollbackRestack } from '../lib/restack-rollback';
import { findStackForBranch, getParent, readState } from '../lib/state';
import { saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';
import { restack } from './restack';

/**
Expand Down Expand Up @@ -175,27 +174,11 @@ export async function reorder(
);
}

// Defence in depth: refuse to reorder a branch git reports as also checked
// out in another worktree. In practice git enforces that the same branch
// can't be checked out twice (and `getCurrentBranch` already errored on
// detached HEAD above), so this branch is rarely reached — keeping it
// guards against forced/inconsistent setups (e.g. `worktree add --force`
// followed by a manual ref edit) before `git rebase -i` would otherwise
// fail with a confusing message (Tier 3 pattern §5).
const worktreeCheckouts = await listWorktreeCheckouts(cwd);
const otherWorktree = worktreeCheckouts.get(currentBranch);
if (otherWorktree) {
throw new DubError(
`Cannot reorder '${currentBranch}': branch is checked out in another worktree.`,
[
formatWorktreeCheckoutSkipMessage(
currentBranch,
otherWorktree,
'dub reorder',
),
],
);
}
await assertBranchesNotCheckedOutElsewhere(
cwd,
[currentBranch],
'dub reorder',
);

const parent = getParent(state, currentBranch);
if (!parent) {
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/commands/split.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
readState,
writeState,
} from '../lib/state';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';
import { restack } from './restack';

export type SplitMode = 'by-commit' | 'by-file' | 'by-hunk' | 'ai';
Expand Down Expand Up @@ -191,6 +192,7 @@ export async function split(
]);
}
const parentBranch = sourceMeta.parent;
await assertBranchesNotCheckedOutElsewhere(cwd, [sourceBranch], 'dub split');
const parentTip = await getBranchTip(parentBranch, cwd);
const sourceTipBefore = await getBranchTip(sourceBranch, cwd);
const existingPrNumber = sourceMeta.pr_number ?? null;
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/commands/squash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
} from '../lib/git';
import { getParent, readState } from '../lib/state';
import { withTempMarkdownFile } from '../lib/temp-text-file';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';
import { restack } from './restack';

export interface SquashOptions {
Expand Down Expand Up @@ -114,6 +115,7 @@ export async function squash(
"Run 'dub log' to inspect the stack and confirm tracking state.",
]);
}
await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub squash');

const commitCount = await countCommitsAhead(branch, parent, cwd);
if (commitCount <= 1) {
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/src/commands/submit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ vi.mock('../lib/git.js', () => ({
getDiff: vi.fn(),
getDiffBetween: vi.fn(),
getLastCommitMessage: vi.fn(),
listWorktreeCheckouts: vi.fn(),
pushBranch: vi.fn(),
}));

Expand Down Expand Up @@ -53,6 +54,7 @@ import {
getDiff,
getDiffBetween,
getLastCommitMessage,
listWorktreeCheckouts,
pushBranch,
} from '../lib/git';
import {
Expand Down Expand Up @@ -83,6 +85,9 @@ const mockGetDiffBetween = getDiffBetween as ReturnType<typeof vi.fn>;
const mockGetLastCommitMessage = getLastCommitMessage as ReturnType<
typeof vi.fn
>;
const mockListWorktreeCheckouts = listWorktreeCheckouts as ReturnType<
typeof vi.fn
>;
const mockPushBranch = pushBranch as ReturnType<typeof vi.fn>;
const mockEnsureGhInstalled = ensureGhInstalled as ReturnType<typeof vi.fn>;
const mockCheckGhAuth = checkGhAuth as ReturnType<typeof vi.fn>;
Expand Down Expand Up @@ -196,6 +201,7 @@ beforeEach(() => {
mockGetDiff.mockResolvedValue('diff --git a/file.ts b/file.ts');
mockGetDiffBetween.mockResolvedValue('diff --git a/file.ts b/file.ts');
mockGetLastCommitMessage.mockResolvedValue('feat: existing title');
mockListWorktreeCheckouts.mockResolvedValue(new Map());
mockAddPrReviewers.mockResolvedValue(undefined);
mockGetRepositoryWebUrl.mockResolvedValue('https://github.com/o/r');
mockMarkPrReady.mockResolvedValue(undefined);
Expand Down
8 changes: 8 additions & 0 deletions packages/cli/src/commands/submit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
writeState,
} from '../lib/state';
import { withTempMarkdownFile } from '../lib/temp-text-file';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';

/** @deprecated Use SubmitScope. Retained for the v1 `--path` deprecation window. */
export type SubmitPathMode = 'current' | 'stack';
Expand Down Expand Up @@ -179,6 +180,13 @@ export async function submit(
}

const plan = await getSubmitPlan(cwd, options);
if (!dryRun) {
await assertBranchesNotCheckedOutElsewhere(
cwd,
plan.branches.map((branch) => branch.name),
'dub submit',
);
}
const config = await readConfig(cwd);
const lifecycle = await resolveSubmitLifecycle(
cwd,
Expand Down
8 changes: 7 additions & 1 deletion packages/cli/src/commands/unlink.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ vi.mock('../lib/git.js', async () => {
...actual,
getCurrentBranch: vi.fn(),
isWorkingTreeClean: vi.fn(),
listWorktreeCheckouts: vi.fn(),
};
});

Expand Down Expand Up @@ -41,7 +42,11 @@ import {
clearCleanupJournal,
startCleanupJournal,
} from '../lib/cleanup-journal';
import { getCurrentBranch, isWorkingTreeClean } from '../lib/git';
import {
getCurrentBranch,
isWorkingTreeClean,
listWorktreeCheckouts,
} from '../lib/git';
import {
checkGhAuth,
ensureGhInstalled,
Expand Down Expand Up @@ -76,6 +81,7 @@ function makeState(specs: BranchSpec[]): DubState {
beforeEach(() => {
vi.clearAllMocks();
vi.mocked(isWorkingTreeClean).mockResolvedValue(true);
vi.mocked(listWorktreeCheckouts).mockResolvedValue(new Map());
vi.mocked(getCurrentBranch).mockResolvedValue('main');
vi.mocked(writeState).mockResolvedValue(undefined);
vi.mocked(startCleanupJournal).mockResolvedValue({
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/commands/unlink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
writeState,
} from '../lib/state';
import { saveUndoEntry } from '../lib/undo-log';
import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards';

export interface UnlinkOptions {
/** Skip PR retargeting; print a warning that the PR base is now out of sync. */
Expand Down Expand Up @@ -110,6 +111,7 @@ export async function unlink(
`Run 'dub untrack ${branch}' to drop the root from tracking entirely.`,
]);
}
await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub unlink');

const previousParent = entry.parent;
if (!previousParent) {
Expand Down
Loading
Loading