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
42 changes: 42 additions & 0 deletions .claude/commands/finalize.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ It guarantees three outcomes:
2. Docs are current
3. Local CI checks pass

It does **not** guarantee that remote PR review is complete after a push. GitHub's
first visible check list can look quiet before delayed checks, bot reviews, and
inline comments arrive. After pushing a finalized branch, hand off to
`/shipLane` or an equivalent PR poll loop. Use the ship-lane cadence: poll
immediately after a push, wait 270s if CI has not registered, wait 720s while CI
is running, and wait 1800s only when CI is done and the PR is just waiting on
review.

**Usage:** `/finalize`

## Execution Mode: Autonomous
Expand Down Expand Up @@ -412,6 +420,34 @@ Kill selectively only if the parent is clearly gone (PPID == 1 on macOS/Linux).

Report killed PIDs in the Phase 4 summary under "Cleanup" so the user can see what happened.

### 3k. Remote PR poll handoff

If this finalize run is followed by a push or PR update, do not treat the first
`gh pr checks` result as authoritative proof that remote review is done. Some
checks and bot review systems appear late or post comments after the initial CI
surface looks complete. In particular:

- `gh pr checks` can omit delayed or still-registering provider checks.
- Bot reviewers can post inline comments after CI jobs have already gone green.
- The absence of new comments immediately after a push is not evidence that no
more comments are coming.

Handoff rule:

```bash
# After the branch is pushed, continue with /shipLane or equivalent:
# - poll PR checks, status rollup, review comments, issue comments, and reviews
# - poll immediately after a push so early CI registration/failures are visible
# - if CI has not started yet, wait 270s
# - if any check is QUEUED/IN_PROGRESS/PENDING, wait 720s
# - if CI is done and the PR is only waiting on review, wait 1800s
# - poll again before declaring the PR clean or ready for human merge
```

If `/finalize` is running as a sub-step inside `/shipLane`, return a summary that
explicitly says remote checks/comments still require the ship-lane poll loop.
Do not report "PR clean" from `/finalize` alone.

---

## Phase 4: Summary
Expand Down Expand Up @@ -447,6 +483,11 @@ Report killed PIDs in the Phase 4 summary under "Cleanup" so the user can see wh
### Cleanup:
- Orphan processes killed: N (PIDs: [list] or "none")

### Remote PR Handoff:
- Post-push polling required: YES
- Poll loop: `/shipLane` branch-specific cadence
- Reason: delayed checks and bot comments may arrive after first visible green state

### Status: Ready to push / Issues found
```

Expand All @@ -466,3 +507,4 @@ Before marking complete:
- [ ] All apps build successfully
- [ ] Doc validation passed
- [ ] Orphan worker processes cleaned up (vitest/tsup/tsc) — scoped to apps/ paths only
- [ ] Remote PR review is not declared clean by finalize alone; after push, `/shipLane` or an equivalent poll loop must use the branch-specific cadence and re-check comments/reviews
24 changes: 12 additions & 12 deletions apps/desktop/src/main/services/lanes/autoRebaseService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,17 @@ function blockedMessage(
laneId: string | null,
reason: "conflict" | "manual" | "lookup" | "failed" | "unavailable" | null,
): string {
if (!laneId) return "Pending: auto-rebase stopped at an earlier lane. Open the Rebase tab to continue.";
if (!laneId) return "Pending: auto-rebase stopped at an earlier lane. Open the Rebase/Merge tab to continue.";
if (reason === "manual") {
return `Pending: ancestor lane '${laneId}' has a fixed PR base. Rebase that lane manually from the Rebase tab before descendants can continue.`;
return `Pending: ancestor lane '${laneId}' has a fixed PR base. Rebase that lane manually from the Rebase/Merge tab before descendants can continue.`;
}
if (reason === "lookup" || reason === "unavailable") {
return `Pending: ancestor lane '${laneId}' needs review before descendants can continue. Open the Rebase tab to inspect it.`;
return `Pending: ancestor lane '${laneId}' needs review before descendants can continue. Open the Rebase/Merge tab to inspect it.`;
}
if (reason === "failed") {
return `Pending: ancestor lane '${laneId}' failed automatic rebase. Open the Rebase tab to retry.`;
return `Pending: ancestor lane '${laneId}' failed automatic rebase. Open the Rebase/Merge tab to retry.`;
}
return `Pending: ancestor lane '${laneId}' has unresolved rebase conflicts. Open the Rebase tab to continue.`;
return `Pending: ancestor lane '${laneId}' has unresolved rebase conflicts. Open the Rebase/Merge tab to continue.`;
}

function resolveAffectedChainLaneId(
Expand Down Expand Up @@ -459,7 +459,7 @@ export function createAutoRebaseService(args: {
parentHeadSha: null,
state: "rebasePending",
conflictCount: 0,
message: "Pending: parent lane is unavailable. Open the Rebase tab to review the lane."
message: "Pending: parent lane is unavailable. Open the Rebase/Merge tab to review the lane."
});
blocked = true;
blockedLaneId = lane.id;
Expand Down Expand Up @@ -522,7 +522,7 @@ export function createAutoRebaseService(args: {
parentHeadSha,
state: "rebaseConflict",
conflictCount: Math.max(1, need.conflictingFiles.length),
message: `Auto-rebase blocked: ${Math.max(1, need.conflictingFiles.length)} conflict(s) expected. Open the Rebase tab to resolve and publish.`
message: `Auto-rebase blocked: ${Math.max(1, need.conflictingFiles.length)} conflict(s) expected. Open the Rebase/Merge tab to resolve and publish.`
});
continue;
}
Expand All @@ -541,7 +541,7 @@ export function createAutoRebaseService(args: {
parentHeadSha,
state: "rebasePending",
conflictCount: 0,
message: "PR carries an immutable base — drift detected. Rebase manually from the Rebase tab when ready."
message: "PR carries an immutable base — drift detected. Rebase manually from the Rebase/Merge tab when ready."
});
continue;
}
Expand Down Expand Up @@ -572,8 +572,8 @@ export function createAutoRebaseService(args: {
state: conflictHint ? "rebaseConflict" : "rebaseFailed",
conflictCount: conflictHint ? 1 : 0,
message: conflictHint
? "Auto-rebase stopped due to conflicts. Open the Rebase tab to resolve, then publish."
: `Auto-rebase failed: ${rebaseRun.run.error}. Open the Rebase tab to retry.`
? "Auto-rebase stopped due to conflicts. Open the Rebase/Merge tab to resolve, then publish."
: `Auto-rebase failed: ${rebaseRun.run.error}. Open the Rebase/Merge tab to retry.`
});
continue;
}
Expand Down Expand Up @@ -625,8 +625,8 @@ export function createAutoRebaseService(args: {
state: "rebaseFailed",
conflictCount: 0,
message: rollbackError
? `Auto-push failed: ${pushError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase tab to retry.`
: `Auto-push failed: ${pushError}. The lane was restored to its pre-rebase state. Open the Rebase tab to retry.`
? `Auto-push failed: ${pushError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase/Merge tab to retry.`
: `Auto-push failed: ${pushError}. The lane was restored to its pre-rebase state. Open the Rebase/Merge tab to retry.`
});
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ describe("launchRebaseResolutionChat", () => {
expect(sendMessage).toHaveBeenCalledWith(
expect.objectContaining({
sessionId: "session-rebase-1",
displayText: "Rebase feature/rebase-target onto main",
reasoningEffort: "high",
displayText: "Rebase feature/rebase-target onto main",
}),
);
expect(result).toEqual({
Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/src/main/services/prs/prRebaseResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ export async function launchRebaseResolutionChat(
await deps.agentChatService.sendMessage({
sessionId: session.id,
text: prompt,
// Show the short human-readable title in the transcript instead of the
// full composed prompt (which is long and technical). Mirrors
// prIssueResolver's pattern; agentChatService falls back to `text` when
// displayText is absent, which would surface the raw prompt to the user.
displayText: title,
...(reasoningEffort ? { reasoningEffort } : {}),
});
Expand Down
6 changes: 3 additions & 3 deletions apps/desktop/src/main/services/prs/prService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,7 @@ export function createPrService({
parentHeadSha: null,
state: "rebaseFailed",
conflictCount: 0,
message: `Auto-rebase failed after '${args.landedLaneName}' merged because ADE could not find a new parent lane. Open the Rebase tab to recover this lane.`,
message: `Auto-rebase failed after '${args.landedLaneName}' merged because ADE could not find a new parent lane. Open the Rebase/Merge tab to recover this lane.`,
}, child.id);
}
return {
Expand Down Expand Up @@ -979,8 +979,8 @@ export function createPrService({
state: "rebaseFailed",
conflictCount: 0,
message: rollbackError
? `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase tab to recover this lane.`
: `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. The lane was restored to its pre-rebase state. Open the Rebase tab to recover this lane.`,
? `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase/Merge tab to recover this lane.`
: `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. The lane was restored to its pre-rebase state. Open the Rebase/Merge tab to recover this lane.`,
}, child.id);
failedLaneIds.push(child.id);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ describe("LaneGitActionsPane rescue action", () => {
expect(syncButton.getAttribute("title")).toMatch(/before rebasing and pushing/i);
});

it("treats auto-rebase conflicts as failures and links to the Rebase tab", async () => {
it("treats auto-rebase conflicts as failures and links to the Rebase/Merge tab", async () => {
const user = userEvent.setup();
const resolveRebaseConflict = vi.fn();
mockAutoRebaseStatuses = [
Expand All @@ -345,7 +345,7 @@ describe("LaneGitActionsPane rescue action", () => {

renderPane({ onResolveRebaseConflict: resolveRebaseConflict });

const rebaseTabButton = await screen.findByRole("button", { name: /open rebase tab/i });
const rebaseTabButton = await screen.findByRole("button", { name: /open rebase\/merge tab/i });
screen.getByText("AUTO-REBASE FAILED");
screen.getByText(/auto-rebase failed\. files need follow-up before this lane can be pushed\./i);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ export function LaneGitActionsPane({
onResolveRebaseConflict(laneId, rebaseConflictParentLaneId);
return;
}
const search = new URLSearchParams({ tab: "rebase", laneId });
const search = new URLSearchParams({ tab: "workflows", workflow: "rebase", laneId });
if (rebaseConflictParentLaneId) search.set("parentLaneId", rebaseConflictParentLaneId);
navigate(`/prs?${search.toString()}`);
};
Expand All @@ -1571,8 +1571,8 @@ export function LaneGitActionsPane({
{autoRebaseStatus.state !== "autoRebased" ? (
isAutoRebaseFailure ? (
<SmartTooltip content={{
label: "Open Rebase Tab",
description: "View detailed rebase conflict information and resolve issues.",
label: "Open Rebase/Merge Tab",
description: "View detailed rebase information and resolve issues.",
effect: "Navigate to the rebase details view",
}}>
<button
Expand All @@ -1581,7 +1581,7 @@ export function LaneGitActionsPane({
disabled={!laneId || busyAction != null}
onClick={openRebaseTab}
>
OPEN REBASE TAB
OPEN REBASE/MERGE TAB
</button>
</SmartTooltip>
) : (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ export function LaneRebaseBanner({
</div>
</div>
<div className="flex shrink-0 flex-wrap items-center gap-1.5">
<SmartTooltip content={{ label: "View in Rebase tab", description: "Open the Rebase tab for this lane." }}>
<SmartTooltip content={{ label: "View in Rebase/Merge tab", description: "Open the Rebase/Merge tab for this lane." }}>
<button
type="button"
style={primaryButton({ height: 24, padding: "0 8px", fontSize: 10 })}
onClick={() => onViewRebaseDetails(s.laneId)}
>
View in Rebase tab
View in Rebase/Merge tab
</button>
</SmartTooltip>
<SmartTooltip content={{ label: "Dismiss", description: "Remove this rebase suggestion permanently until new parent commits arrive." }}>
Expand Down Expand Up @@ -120,13 +120,13 @@ export function LaneRebaseBanner({
</div>
</div>
<div className="shrink-0 flex items-center gap-1.5">
<SmartTooltip content={{ label: "View in Rebase tab", description: "Open the Rebase tab for this lane." }}>
<SmartTooltip content={{ label: "View in Rebase/Merge tab", description: "Open the Rebase/Merge tab for this lane." }}>
<button
type="button"
style={primaryButton({ height: 24, padding: "0 8px", fontSize: 10 })}
onClick={() => onViewRebaseDetails(status.laneId)}
>
View in Rebase tab
View in Rebase/Merge tab
</button>
</SmartTooltip>
<SmartTooltip content={{ label: "Dismiss", description: "Hide this alert until the parent or base changes again." }}>
Expand Down
4 changes: 2 additions & 2 deletions apps/desktop/src/renderer/components/lanes/LanesPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1101,7 +1101,7 @@ export function LanesPage() {
const failedLane = start.run.failedLaneId ? lanesById.get(start.run.failedLaneId)?.name ?? start.run.failedLaneId : null;
const detail = start.run.error ?? "Rebase failed.";
setRebaseSuggestionError(`Rebase needs attention${failedLane ? ` for ${failedLane}` : ""}. ${detail}`);
navigate("/prs?tab=rebase");
navigate("/prs?tab=workflows&workflow=rebase");
return;
}

Expand All @@ -1121,7 +1121,7 @@ export function LanesPage() {
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
setRebaseSuggestionError(message);
navigate("/prs?tab=rebase");
navigate("/prs?tab=workflows&workflow=rebase");
}
}, [lanesById, navigate, refreshLanes, requestPushSelection, requestRebaseScope]);

Expand Down
15 changes: 10 additions & 5 deletions apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ import React from "react";
import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { MemoryRouter } from "react-router-dom";
import type { LaneSummary } from "../../../shared/types";

function renderWithRouter(ui: React.ReactElement) {
return render(<MemoryRouter>{ui}</MemoryRouter>);
}

function makeLane(overrides: Partial<LaneSummary> = {}): LaneSummary {
return {
id: "lane-1",
Expand Down Expand Up @@ -117,7 +122,7 @@ describe("CreatePrModal queue workflow", () => {

it("adds selected lanes to the queue order and removes them from the queue builder", async () => {
const user = userEvent.setup();
render(<CreatePrModal open onOpenChange={vi.fn()} />);
renderWithRouter(<CreatePrModal open onOpenChange={vi.fn()} />);

await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!);
await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i }));
Expand All @@ -136,7 +141,7 @@ describe("CreatePrModal queue workflow", () => {

it("uses the dragged queue order when creating queue PRs", async () => {
const user = userEvent.setup();
render(<CreatePrModal open onOpenChange={vi.fn()} />);
renderWithRouter(<CreatePrModal open onOpenChange={vi.fn()} />);

await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!);
await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i }));
Expand Down Expand Up @@ -165,7 +170,7 @@ describe("CreatePrModal queue workflow", () => {

it("lets single-PR creation target a different branch than Primary's current branch", async () => {
const user = userEvent.setup();
render(<CreatePrModal open onOpenChange={vi.fn()} />);
renderWithRouter(<CreatePrModal open onOpenChange={vi.fn()} />);

// Select source lane
const comboboxes = screen.getAllByRole("combobox");
Expand All @@ -191,7 +196,7 @@ describe("CreatePrModal queue workflow", () => {

it("warns when the PR target branch differs from the lane base branch", async () => {
const user = userEvent.setup();
render(<CreatePrModal open onOpenChange={vi.fn()} />);
renderWithRouter(<CreatePrModal open onOpenChange={vi.fn()} />);

// Select source lane
const comboboxes = screen.getAllByRole("combobox");
Expand All @@ -210,7 +215,7 @@ describe("CreatePrModal queue workflow", () => {

it("lets queue creation target a different branch than Primary's current branch", async () => {
const user = userEvent.setup();
render(<CreatePrModal open onOpenChange={vi.fn()} />);
renderWithRouter(<CreatePrModal open onOpenChange={vi.fn()} />);

await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!);
await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i }));
Expand Down
9 changes: 6 additions & 3 deletions apps/desktop/src/renderer/components/prs/CreatePrModal.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import { useNavigate } from "react-router-dom";
import * as Dialog from "@radix-ui/react-dialog";
import { GitPullRequest, GitMerge, Stack as Layers, CheckCircle, Warning, CircleNotch, X, GitBranch, Sparkle, ArrowRight, ArrowLeft, Check, DotsSixVertical, Trash, ArrowUp, ArrowDown } from "@phosphor-icons/react";
import { useAppStore } from "../../state/appStore";
Expand Down Expand Up @@ -446,7 +447,7 @@ function LaneWarningPanel({
cursor: "pointer",
}}
>
Open Rebase Tab
Open Rebase/Merge tab
</button>
<span style={{ fontSize: 10, color: C.textMuted, fontFamily: MONO_FONT }}>
Review rebase status before PR creation{rebaseLaneIds.length > 1 ? ` (${rebaseLaneIds.length} lanes)` : ""}.
Expand All @@ -470,6 +471,7 @@ export function CreatePrModal({
onOpenChange: (open: boolean) => void;
onCreated?: (created: PrSummary[]) => void | Promise<void>;
}) {
const navigate = useNavigate();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🟡 Medium] [🔵 Bug]

CreatePrModal now calls useNavigate() unconditionally during render, which changes its contract from a plain component to a router-bound component. The existing test file still renders it directly five times with render(<CreatePrModal open onOpenChange={vi.fn()} />) and no MemoryRouter, so those cases will now fail with React Router's invariant before any assertions run. This is introduced by the new hook usage, not a pre-existing test problem. Fix by wrapping the modal's tests in router context (or by avoiding a hard router dependency in this component).

// apps/desktop/src/renderer/components/prs/CreatePrModal.tsx
export function CreatePrModal({
  open,
  onOpenChange,
  onCreated,
}: {
  const navigate = useNavigate();

const lanes = useAppStore((s) => s.lanes);
const primaryLane = React.useMemo(() => lanes.find((l) => l.laneType === "primary") ?? null, [lanes]);

Expand Down Expand Up @@ -596,8 +598,9 @@ export function CreatePrModal({

const openRebaseTab = React.useCallback((laneId: string) => {
onOpenChange(false);
window.location.hash = `#/prs?tab=rebase&laneId=${encodeURIComponent(laneId)}`;
}, [onOpenChange]);
const search = new URLSearchParams({ tab: "workflows", workflow: "rebase", laneId });
navigate({ pathname: "/prs", search: `?${search.toString()}` });
}, [navigate, onOpenChange]);

// Reset on close
React.useEffect(() => {
Expand Down
Loading
Loading