From 479bb485eb0e84242e4a57db5d4b08196e37722b Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 19 May 2026 20:34:34 -0400 Subject: [PATCH 01/23] Review tab parallel review organization --- .ade/ade.yaml | 11 +- .claude/skills/plan/SKILL.md | 175 ---- apps/ade-cli/src/adeRpcServer.test.ts | 45 + apps/desktop/scripts/ensure-ade-cli-build.cjs | 5 + apps/desktop/src/main/rendererCsp.test.ts | 7 + apps/desktop/src/main/rendererCsp.ts | 2 +- .../src/main/services/adeActions/registry.ts | 3 + .../services/ai/tools/ctoOperatorTools.ts | 6 +- .../services/git/gitOperationsService.test.ts | 114 ++- .../main/services/git/gitOperationsService.ts | 108 ++- .../src/main/services/ipc/registerIpc.ts | 143 +++- .../main/services/lanes/laneService.test.ts | 9 + .../src/main/services/lanes/laneService.ts | 1 + .../src/main/services/prs/prService.test.ts | 492 +++++++++++ .../src/main/services/prs/prService.ts | 706 ++++++++++++++- .../services/review/reviewService.test.ts | 146 +++- .../src/main/services/review/reviewService.ts | 803 +++++++++++++++--- apps/desktop/src/main/services/state/kvDb.ts | 86 ++ apps/desktop/src/preload/global.d.ts | 9 + apps/desktop/src/preload/preload.test.ts | 312 ++++++- apps/desktop/src/preload/preload.ts | 31 +- .../src/renderer/components/app/AppShell.tsx | 2 +- .../automations/adeActionSchemas.ts | 8 +- .../components/graph/WorkspaceGraphPage.tsx | 111 ++- .../components/lanes/LaneGitActionsPane.tsx | 18 +- .../renderer/components/lanes/LanesPage.tsx | 35 +- .../src/renderer/components/prs/PRsPage.tsx | 13 +- .../PrDetailPane.issueResolver.test.tsx | 58 +- .../components/prs/detail/PrDetailPane.tsx | 325 +++---- .../prs/detail/PrDetailTimelineRails.tsx | 81 +- .../renderer/components/prs/prsRouteState.ts | 5 +- .../components/prs/shared/PrTimeline.test.tsx | 20 +- .../components/prs/shared/PrTimeline.tsx | 182 +--- .../components/prs/state/PrsContext.test.tsx | 135 ++- .../components/prs/state/PrsContext.tsx | 234 ++++- .../prs/state/PrsContextWarmCache.test.tsx | 3 +- .../components/prs/tabs/GitHubTab.test.tsx | 233 ++++- .../components/prs/tabs/GitHubTab.tsx | 630 +++++++++++--- .../components/prs/tabs/IntegrationTab.tsx | 8 +- .../components/review/ReviewFindingCard.tsx | 193 +++-- .../components/review/ReviewPage.test.tsx | 189 ++++- .../renderer/components/review/ReviewPage.tsx | 584 ++++++++----- .../components/review/reviewFindingLabels.ts | 77 ++ .../shared/ReviewLaunchModelControls.tsx | 6 + apps/desktop/src/shared/ipc.ts | 2 + apps/desktop/src/shared/types/prs.ts | 61 +- apps/desktop/src/shared/types/review.ts | 58 +- apps/ios/ADE/Resources/DatabaseBootstrap.sql | 85 ++ scripts/dev-shared.mjs | 10 + 49 files changed, 5165 insertions(+), 1415 deletions(-) delete mode 100644 .claude/skills/plan/SKILL.md create mode 100644 apps/desktop/src/renderer/components/review/reviewFindingLabels.ts diff --git a/.ade/ade.yaml b/.ade/ade.yaml index f8abdd165..d7995eecf 100644 --- a/.ade/ade.yaml +++ b/.ade/ade.yaml @@ -1,11 +1,12 @@ version: 1 processes: - - id: xsi8oj88 - name: dogfood onboardinign fixes + - id: gq3sy4rj + name: npm run dev:desktop command: - - ./scripts/dogfood.sh - - onboarding-fixes - cwd: apps/desktop + - npm + - run + - dev:desktop + cwd: . gracefulShutdownMs: 7000 readiness: type: none diff --git a/.claude/skills/plan/SKILL.md b/.claude/skills/plan/SKILL.md deleted file mode 100644 index fe1d124b8..000000000 --- a/.claude/skills/plan/SKILL.md +++ /dev/null @@ -1,175 +0,0 @@ ---- -name: plan -description: Deliberate a feature or change in three locked rounds — functional requirements, then UI design with wireframes, then extras/quirks/out-of-the-box ideas. Each round asks the user clarifying questions via AskUserQuestion before proceeding. New functional scope at any point cascade-restarts the new piece through all three rounds and merges it into the locked plan. Use when the user invokes `/plan`, when the user is in plan mode and asks for a design/spec/feature breakdown, or when a non-trivial change needs structured deliberation before implementation. -metadata: - author: ade - version: "1.0" ---- - -# /plan — Three-Round Locked Deliberation - -A feature plan has three layers: **what it does**, **how it looks**, and **the delightful extras**. Cross-talk between them produces sloppy plans. This skill enforces a strict order: lock functional, then lock UI, then add extras. New functional scope discovered at any point cascades back through all three rounds *for that new piece only*, then merges into the locked plan. - -## Activation - -Activate when **any** of: -- The user invokes `/plan` (with or without extra context). -- The user is in plan mode and asks for a design, spec, breakdown, or feature plan. -- A non-trivial change request would benefit from structured deliberation (multi-component features, UX-sensitive work, anything spanning >2 files). - -## Pre-flight: plan mode gate - -Before Round 1, verify plan mode is active. If not: - -> This skill is meant to run in plan mode (read-only deliberation). Enter plan mode (Shift+Tab cycles to it) and re-invoke `/plan `. - -Then stop. Do not proceed in edit/full-auto modes — the deliberation contract assumes no files will be touched mid-plan. - -## State you must track - -Maintain these in your head (or in scratch) across the whole skill: - -- **LockedFunctional** — bullet list of functional requirements confirmed so far. -- **LockedUI** — bullet list of UI decisions confirmed so far (with wireframe sketches). -- **LockedExtras** — list of delightful extras confirmed. -- **CurrentRound** — 1 (Functional), 2 (UI), or 3 (Extras). -- **PendingNewPieces** — queue of new functional requirements detected mid-flight that need their own cascade. - -When a cascade fires, you snapshot CurrentRound, run the cascade for the new piece, merge results back into the Locked* sets, then resume from the snapshotted round. - -## Round 1 — Functional Requirements - -Silently deliberate on what the user asked for. Pull out: -- Core capabilities the feature must have. -- Boundaries (what it does *not* do). -- Inputs, outputs, edge cases that change behavior. -- Ambiguities where multiple interpretations exist. - -Then call **AskUserQuestion** with 1–4 questions that resolve the meaningful ambiguities. Each option should describe a concrete interpretation with its trade-off. Avoid asking the user to write prose — frame everything as concrete choices. - -### Tool reference: AskUserQuestion - -Use `AskUserQuestion(questions)` to gather structured choices during Round 1, Round 2, Round 3, and any cascade. `questions` is an array of question objects: - -- `id`: stable snake_case identifier. -- `title`: short user-facing label. -- `text`: the actual question. -- `multiSelect`: optional boolean for menus like LockedExtras. -- `allowOther`: optional boolean that permits an "Other" selection. -- `allowAnnotation`: optional boolean that permits free-text annotation alongside a selected option. -- `options`: array of `{ id, label, tradeoffs, description?, preview? }`. - -The return value is keyed by question id and contains `selectedOptionIds`, optional `otherText`, and optional `annotations` / `freeText`. Treat selected option ids as the locked choice. Treat `otherText` as a new option authored by the user; if it changes behavior, update **LockedFunctional** and trigger the **Cascade rule**. Treat annotations as refinements to the selected option; if an annotation adds behavior rather than clarifying wording or presentation, it also cascades. - -When the user responds: -- Treat selected options as additions to **LockedFunctional**. -- If the user's free-text (the "Other" reply or annotation) adds new scope → see **Cascade rule** below. -- If the user only clarifies an existing item → update **LockedFunctional** and proceed. - -Once questions are answered, **do not ask for explicit confirmation**. Silently move to Round 2. - -## Round 2 — UI Design - -Now deliberate on how the feature presents to the user. Generate 1–3 candidate UI directions. Round 2 candidates may be full-layout wireframes when the whole surface is up for decision, or component-level candidates when only one area is ambiguous. Be explicit which level each candidate represents. - -Call **AskUserQuestion** with options that carry **markdown wireframe previews** in the `preview` field. Wireframes are ASCII boxes: - -``` -┌─────────────────────────────────┐ -│ Header · filter ▾ · +new │ -├─────────────────────────────────┤ -│ ▣ item ⋯ │ -│ ▢ item ⋯ │ -│ ▢ item ⋯ │ -└─────────────────────────────────┘ -``` - -Keep each preview readable in a chat-width column (~70 chars). Show real labels, real density, real affordances — not lorem ipsum. - -Cover the meaningful axes (layout type, hierarchy of actions, empty/loading/error states, dense vs roomy) in **at most 4 questions total**. Don't waste a question on a decision that has one obvious answer. - -When the user responds: -- Selected wireframes/options → **LockedUI**. -- If candidates were component-level, merge the selected components into **LockedUI** as integration decisions. The Final output still needs one composed/merged inline wireframe derived from **LockedFunctional**, **LockedUI**, and any selected **LockedExtras**. -- Free-text that implies new functional behavior (e.g. "add export button" implies an export *flow*) → see **Cascade rule**. -- Pure UI refinements stay in Round 2. - -Silently proceed to Round 3. - -## Round 3 — Extras, Quirks, Out-of-the-Box - -Now generate 4–8 candidate ideas the user *didn't* ask for but might love: micro-interactions, keyboard shortcuts, empty-state delight, power-user features, animations, accessibility wins, surprise affordances, anti-aesthetics-of-AI touches (favor warmth and specificity over generic monochrome polish). - -Call **AskUserQuestion** with `multiSelect: true` for the menu of extras. Each option's `description` should be one sentence explaining the idea and one sentence on the cost/benefit. - -Selected items → **LockedExtras**. - -If the user adds a new functional idea in free-text → **Cascade rule**. - -## Cascade rule (the core contract) - -A **new functional requirement** is any input — from the user OR self-detected from your own proposals — that adds, removes, or changes scope of the feature. Not a UI refinement. Not an extras pick. *New behavior the system must perform.* - -When detected: - -1. **Snapshot** the current round. -2. **Queue** the new piece in **PendingNewPieces** with a one-line description. -3. **Announce briefly**: "New functional piece detected: . Cascading it through R1→R2→R3 before resuming Round ." -4. **Run a focused mini-cascade for that piece only**: - - **R1 for new piece**: AskUserQuestion scoped to just the new piece's functional ambiguities. - - **R2 for new piece**: AskUserQuestion with wireframes scoped to just how this new piece integrates into the already-locked UI (do NOT redesign locked layouts — show how the new piece slots in). - - **R3 for new piece**: AskUserQuestion with extras *for this piece only*. -5. **Merge** the results into the appropriate Locked* sets. -6. **Resume** from the snapshotted round. - -Multiple cascades may stack. Process them depth-first; never lose the snapshot. - -### Self-detecting new functional scope - -Before sending a UI option or an extra, ask yourself silently: *does this option require behavior that isn't already in LockedFunctional?* If yes, you have a choice: -- Drop the option (cleanest). -- Or, if it's genuinely a great idea, **fire the cascade yourself** before sending it. Don't sneak new functional scope into UI/extras questions. - -## Final output - -After Round 3 completes with no pending cascades: - -1. Print a tight consolidated plan in chat with three sections: - - **Functional** — bulleted LockedFunctional. - - **UI** — bulleted LockedUI with one inline wireframe of the final composed layout. - - **Extras** — bulleted LockedExtras. - Keep total under ~50 lines. No filler. -2. Call **ExitPlanMode** to formally request approval. - -### Tool reference: ExitPlanMode - -Use `ExitPlanMode(): void` immediately after the Final output. It has no parameters and returns no value. Calling it signals that **LockedFunctional**, **LockedUI**, and **LockedExtras** are complete, including the final composed wireframe, and asks the user to approve leaving plan mode. Do not call it before all pending cascades are processed. - -## Anti-patterns (do not do) - -- Asking the user to "describe what they want" in prose. Always frame as concrete options. -- Sending more than 4 questions in a single AskUserQuestion call (tool max). -- Slipping new functional behavior into UI or Extras rounds without cascading. -- Asking explicit "lock in? yes/no" questions between rounds — proceed silently unless interrupted. -- Re-litigating LockedFunctional during R2 or R3 unless a cascade explicitly opens that piece. -- Generic AI aesthetics in wireframes — no `font-mono` aesthetic apologies, no centered-everything, no purple gradients in mocked copy. Be specific and warm. -- Long preamble. The user invoked `/plan`; jump to Round 1. - -## Minimal example flow - -User: `/plan a markdown note app with tags` - -You: silent deliberation → AskUserQuestion (R1): -- Q1: "How should tags be created?" (inline `#tag` parsing / explicit tag field / both) -- Q2: "Search scope when filtering by tag?" (notes containing tag / notes tagged-only / both modes) -- Q3: "Multi-user or single-user?" - -User selects + adds "and they sync to iCloud" (new functional scope). - -You: "New piece detected: iCloud sync. Cascading." -→ R1-for-sync: conflict resolution? offline-first? -→ R2-for-sync: sync-status indicator placement? -→ R3-for-sync: optimistic UI? merge-conflict modal? -Merge → resume Round 1. - -… and so on through R2 and R3 of the main plan, then ExitPlanMode. diff --git a/apps/ade-cli/src/adeRpcServer.test.ts b/apps/ade-cli/src/adeRpcServer.test.ts index 76105b068..0cb6d57ad 100644 --- a/apps/ade-cli/src/adeRpcServer.test.ts +++ b/apps/ade-cli/src/adeRpcServer.test.ts @@ -4515,6 +4515,51 @@ describe("adeRpcServer", () => { }); + it("invokes review.startRun through ADE actions without dropping unlimited budgets", async () => { + const fixture = createRuntime(); + const startArgs = { + target: { mode: "lane_diff", laneId: "lane-1" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: "openai/gpt-5.4", + reasoningEffort: "medium", + budgets: { + unlimited: true, + maxFiles: Number.MAX_SAFE_INTEGER, + maxDiffChars: Number.MAX_SAFE_INTEGER, + maxPromptChars: Number.MAX_SAFE_INTEGER, + maxFindings: Number.MAX_SAFE_INTEGER, + maxFindingsPerPass: Number.MAX_SAFE_INTEGER, + maxPublishedFindings: Number.MAX_SAFE_INTEGER, + }, + publishBehavior: "local_only", + }, + }; + const startRun = vi.fn(async (args: typeof startArgs) => ({ + id: "review-run-1", + laneId: args.target.laneId, + config: args.config, + status: "queued", + })); + (fixture.runtime as any).reviewService = { startRun }; + const handler = createAdeRpcRequestHandler({ runtime: fixture.runtime, serverVersion: "test" }); + await initialize(handler, { callerId: "agent-1", role: "agent" }); + + const response = await callTool(handler, "run_ade_action", { + domain: "review", + action: "startRun", + args: startArgs, + }); + + expect(response?.isError).toBeUndefined(); + expect(startRun).toHaveBeenCalledWith(startArgs); + expect(startRun.mock.calls[0][0].config.budgets).toEqual(startArgs.config.budgets); + expect(response.structuredContent.result.config.budgets).toEqual(startArgs.config.budgets); + expect(response.structuredContent.result.config.budgets.unlimited).toBe(true); + }); + it("binds service method context when invoking dynamic ADE actions", async () => { const fixture = createRuntime(); const missionService = fixture.runtime.missionService as any; diff --git a/apps/desktop/scripts/ensure-ade-cli-build.cjs b/apps/desktop/scripts/ensure-ade-cli-build.cjs index dfb9c3f5f..14530480a 100644 --- a/apps/desktop/scripts/ensure-ade-cli-build.cjs +++ b/apps/desktop/scripts/ensure-ade-cli-build.cjs @@ -21,6 +21,11 @@ const sourceEntries = [ path.join(cliRoot, "package-lock.json"), path.join(cliRoot, "tsconfig.json"), path.join(cliRoot, "tsup.config.ts"), + path.join(desktopRoot, "src", "main"), + path.join(desktopRoot, "src", "shared"), + path.join(desktopRoot, "package.json"), + path.join(desktopRoot, "package-lock.json"), + path.join(desktopRoot, "tsconfig.json"), ]; function newestMtimeMs(entryPath) { diff --git a/apps/desktop/src/main/rendererCsp.test.ts b/apps/desktop/src/main/rendererCsp.test.ts index 4f860cd97..661e69888 100644 --- a/apps/desktop/src/main/rendererCsp.test.ts +++ b/apps/desktop/src/main/rendererCsp.test.ts @@ -19,4 +19,11 @@ describe("buildRendererCspPolicy", () => { expect(policy).toContain("frame-src 'self' file: app: http://localhost:* http://127.0.0.1:* about:"); }); + + it("allows GitHub PR bot images served from public Google Cloud Storage", () => { + const policy = buildRendererCspPolicy(false); + + expect(policy).toContain("img-src"); + expect(policy).toContain("https://storage.googleapis.com"); + }); }); diff --git a/apps/desktop/src/main/rendererCsp.ts b/apps/desktop/src/main/rendererCsp.ts index 132968afa..c561d0b54 100644 --- a/apps/desktop/src/main/rendererCsp.ts +++ b/apps/desktop/src/main/rendererCsp.ts @@ -5,7 +5,7 @@ export function buildRendererCspPolicy(isDevMode: boolean): string { const cspWsSources = isDevMode ? " ws://localhost:* ws://127.0.0.1:*" : ""; const cspLocalSources = " http://localhost:* http://127.0.0.1:*"; const cspConnectLocalSources = isDevMode ? "" : cspLocalSources; - const cspImageSources = `${cspSources}${cspLocalSources} https://avatars.githubusercontent.com https://*.githubusercontent.com https://github.githubassets.com https://opengraph.githubassets.com https://github.com https://vercel.com https://*.vercel.com https://img.shields.io https://*.s3.amazonaws.com`; + const cspImageSources = `${cspSources}${cspLocalSources} https://avatars.githubusercontent.com https://*.githubusercontent.com https://github.githubassets.com https://opengraph.githubassets.com https://github.com https://vercel.com https://*.vercel.com https://img.shields.io https://*.s3.amazonaws.com https://storage.googleapis.com`; return [ `default-src ${cspSources}`, `base-uri 'self'`, diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index 4c9e2b9cb..a8b99fb20 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -335,6 +335,7 @@ export const ADE_ACTION_ALLOWLIST: Partial gitGuard(() => deps.gitService!.stashPush({ laneId: resolveLaneId(laneId), ...(message?.trim() ? { message: message.trim() } : {}) })), }); tools.gitStashPop = tool({ - description: "Pop the latest stash in a lane.", + description: "Pop the latest stash saved for a lane branch.", inputSchema: z.object({ laneId: z.string().optional() }), execute: ({ laneId }) => gitGuard(() => deps.gitService!.stashPop({ laneId: resolveLaneId(laneId) })), }); tools.gitStashList = tool({ - description: "List stashes in a lane.", + description: "List stashes saved for a lane branch.", inputSchema: z.object({ laneId: z.string().optional() }), execute: ({ laneId }) => gitGuard(async () => { const stashes = await deps.gitService!.listStashes({ laneId: resolveLaneId(laneId) }); diff --git a/apps/desktop/src/main/services/git/gitOperationsService.test.ts b/apps/desktop/src/main/services/git/gitOperationsService.test.ts index e517234cf..dc9138af3 100644 --- a/apps/desktop/src/main/services/git/gitOperationsService.test.ts +++ b/apps/desktop/src/main/services/git/gitOperationsService.test.ts @@ -67,17 +67,37 @@ describe("gitOperationsService.stashClear", () => { vi.clearAllMocks(); }); - it("calls git stash clear with the lane worktree path and returns the action result", async () => { + it("drops only stashes saved for the lane branch", async () => { mockGit.getHeadSha.mockResolvedValue("abc123"); - mockGit.runGitOrThrow.mockResolvedValue(undefined); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => { + if (args[0] === "stash" && args[1] === "list") { + return [ + "stash@{0}\u001f2026-05-12T02:09:32-04:00\u001fOn feature/stash-test: keep for this lane", + "stash@{1}\u001f2026-05-12T02:08:32-04:00\u001fOn other-branch: leave alone", + "stash@{2}\u001f2026-05-12T02:07:32-04:00\u001fWIP on feature/stash-test: abc123 work", + ].join("\n"); + } + return undefined; + }); const { service, mockStart, mockFinish, mockInvalidateListCache } = createTestGitOperationsService(); const result = await service.stashClear({ laneId: "lane-1" }); - expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( - ["stash", "clear"], + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 1, + ["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], { cwd: "/tmp/ade-lane", timeoutMs: 15_000 }, ); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 2, + ["stash", "drop", "stash@{2}"], + { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, + ); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 3, + ["stash", "drop", "stash@{0}"], + { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, + ); expect(result).toEqual({ operationId: "op-1", preHeadSha: "abc123", @@ -133,19 +153,34 @@ describe("gitOperationsService.getSyncStatus", () => { }); describe("gitOperationsService stash item commands", () => { + const branchStashList = [ + "stash@{0}\u001f2026-05-12T02:09:32-04:00\u001fOn feature/stash-test: first", + "stash@{1}\u001f2026-05-12T02:08:32-04:00\u001fWIP on feature/stash-test: abc123 work", + "stash@{2}\u001f2026-05-12T02:07:32-04:00\u001fOn other-branch: hidden", + ].join("\n"); + beforeEach(() => { vi.clearAllMocks(); }); it("lists stashes with ordinal refs and ISO timestamps", async () => { - mockGit.runGitOrThrow.mockResolvedValue("stash@{0}\u001f2026-05-12T02:09:32-04:00\u001fOn main: test\n"); + mockGit.runGitOrThrow.mockResolvedValue([ + "stash@{0}\u001f2026-05-12T02:09:32-04:00\u001fOn feature/stash-test: test", + "stash@{1}\u001f2026-05-12T02:08:32-04:00\u001fWIP on feature/stash-test: abc123 work", + "stash@{2}\u001f2026-05-12T02:07:32-04:00\u001fOn other-branch: hidden", + ].join("\n")); const { service } = createTestGitOperationsService(); await expect(service.listStashes({ laneId: "lane-1" })).resolves.toEqual([ { ref: "stash@{0}", createdAt: "2026-05-12T02:09:32-04:00", - subject: "On main: test", + subject: "On feature/stash-test: test", + }, + { + ref: "stash@{1}", + createdAt: "2026-05-12T02:08:32-04:00", + subject: "WIP on feature/stash-test: abc123 work", }, ]); expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( @@ -156,20 +191,29 @@ describe("gitOperationsService stash item commands", () => { it("applies then drops a stash when restoring it", async () => { mockGit.getHeadSha.mockResolvedValue("abc123"); - mockGit.runGitOrThrow.mockResolvedValue(undefined); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => + args[0] === "stash" && args[1] === "list" ? branchStashList : undefined + ); const { service, mockStart, mockFinish } = createTestGitOperationsService(); const result = await service.stashPop({ laneId: "lane-1", stashRef: "stash@{1}" }); - expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 1, + ["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], + { cwd: "/tmp/ade-lane", timeoutMs: 15_000 }, + ); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 2, ["stash", "apply", "stash@{1}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); - expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 3, ["stash", "drop", "stash@{1}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); - expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(2); + expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(3); expect(result).toEqual({ operationId: "op-1", preHeadSha: "abc123", @@ -192,13 +236,18 @@ describe("gitOperationsService stash item commands", () => { it("keeps the stash when restore apply fails", async () => { mockGit.getHeadSha.mockResolvedValue("abc123"); - mockGit.runGitOrThrow.mockRejectedValueOnce(new Error("apply failed")); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => { + if (args[0] === "stash" && args[1] === "list") return branchStashList; + if (args[0] === "stash" && args[1] === "apply") throw new Error("apply failed"); + return undefined; + }); const { service } = createTestGitOperationsService(); await expect(service.stashPop({ laneId: "lane-1", stashRef: "stash@{1}" })).rejects.toThrow("apply failed"); - expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(1); - expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(2); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 2, ["stash", "apply", "stash@{1}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); @@ -206,20 +255,27 @@ describe("gitOperationsService stash item commands", () => { it("reports restore success when drop fails after apply", async () => { mockGit.getHeadSha.mockResolvedValue("abc123"); - mockGit.runGitOrThrow - .mockResolvedValueOnce(undefined) - .mockRejectedValueOnce(new Error("drop failed")); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => { + if (args[0] === "stash" && args[1] === "list") return branchStashList; + if (args[0] === "stash" && args[1] === "drop") throw new Error("drop failed"); + return undefined; + }); const { service, mockFinish, mockLogger } = createTestGitOperationsService(); const result = await service.stashPop({ laneId: "lane-1", stashRef: "stash@{1}" }); expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( 1, + ["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], + { cwd: "/tmp/ade-lane", timeoutMs: 15_000 }, + ); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 2, ["stash", "apply", "stash@{1}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( - 2, + 3, ["stash", "drop", "stash@{1}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); @@ -246,12 +302,20 @@ describe("gitOperationsService stash item commands", () => { it("calls git stash drop with the lane worktree path and stash ref", async () => { mockGit.getHeadSha.mockResolvedValue("abc123"); - mockGit.runGitOrThrow.mockResolvedValue(undefined); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => + args[0] === "stash" && args[1] === "list" ? branchStashList : undefined + ); const { service, mockStart, mockFinish } = createTestGitOperationsService(); const result = await service.stashDrop({ laneId: "lane-1", stashRef: "stash@{0}" }); - expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 1, + ["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], + { cwd: "/tmp/ade-lane", timeoutMs: 15_000 }, + ); + expect(mockGit.runGitOrThrow).toHaveBeenNthCalledWith( + 2, ["stash", "drop", "stash@{0}"], { cwd: "/tmp/ade-lane", timeoutMs: 30_000 }, ); @@ -274,6 +338,18 @@ describe("gitOperationsService stash item commands", () => { }), ); }); + + it("does not apply a stash from another branch", async () => { + mockGit.getHeadSha.mockResolvedValue("abc123"); + mockGit.runGitOrThrow.mockImplementation(async (args: string[]) => + args[0] === "stash" && args[1] === "list" ? branchStashList : undefined + ); + const { service } = createTestGitOperationsService(); + + await expect(service.stashApply({ laneId: "lane-1", stashRef: "stash@{2}" })) + .rejects.toThrow("Stash stash@{2} is not saved for branch feature/stash-test."); + expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(1); + }); }); describe("gitOperationsService.commit", () => { diff --git a/apps/desktop/src/main/services/git/gitOperationsService.ts b/apps/desktop/src/main/services/git/gitOperationsService.ts index 7a28467d8..189497cde 100644 --- a/apps/desktop/src/main/services/git/gitOperationsService.ts +++ b/apps/desktop/src/main/services/git/gitOperationsService.ts @@ -106,6 +106,43 @@ function parseDelimited(line: string): string[] { return line.split("\u001f"); } +function normalizeBranchForStashComparison(ref: string): string { + return localBranchNameForConfig(ref).trim(); +} + +function branchNameFromStashSubject(subject: string): string | null { + const trimmed = subject.trim(); + const onMatch = /^On ([^:]+):/.exec(trimmed); + if (onMatch) return onMatch[1]?.trim() || null; + const wipMatch = /^WIP on ([^:]+):/.exec(trimmed); + if (wipMatch) return wipMatch[1]?.trim() || null; + return null; +} + +function stashMatchesBranch(subject: string, branchRef: string): boolean { + const branch = normalizeBranchForStashComparison(branchRef); + if (!branch) return false; + return branchNameFromStashSubject(subject) === branch; +} + +function stashOrdinal(ref: string): number | null { + const match = /^stash@\{(\d+)\}$/.exec(ref.trim()); + if (!match) return null; + const ordinal = Number(match[1]); + return Number.isSafeInteger(ordinal) ? ordinal : null; +} + +function sortStashesForDrop(stashes: GitStashSummary[]): GitStashSummary[] { + return stashes.slice().sort((left, right) => { + const leftOrdinal = stashOrdinal(left.ref); + const rightOrdinal = stashOrdinal(right.ref); + if (leftOrdinal != null && rightOrdinal != null) return rightOrdinal - leftOrdinal; + if (leftOrdinal != null) return -1; + if (rightOrdinal != null) return 1; + return right.ref.localeCompare(left.ref); + }); +} + async function isWorktreeDirty(worktreePath: string): Promise { const res = await runGit(["status", "--porcelain=v1"], { cwd: worktreePath, timeoutMs: 8_000 }); if (res.exitCode !== 0) return false; @@ -164,6 +201,12 @@ export function createGitOperationsService({ } } + function invalidateStashReadCaches(): void { + for (const key of laneReadCache.keys()) { + if (key.startsWith("stashes:")) laneReadCache.delete(key); + } + } + async function readLaneCached(key: string, ttlMs: number, load: () => Promise): Promise { const now = Date.now(); const cached = laneReadCache.get(key) as CachedReadEntry | undefined; @@ -222,6 +265,37 @@ export function createGitOperationsService({ return cleaned.slice(0, 72).trimEnd(); } + async function listBranchStashes(lane: LaneInfo): Promise { + const out = await runGitOrThrow(["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], { + cwd: lane.worktreePath, + timeoutMs: 15_000 + }); + return out + .split("\n") + .map((line) => line.trim()) + .filter(Boolean) + .map((line): GitStashSummary | null => { + const [ref, createdAt, subject] = parseDelimited(line); + if (!ref) return null; + return { + ref, + createdAt: createdAt && createdAt.length ? createdAt : null, + subject: subject ?? "" + }; + }) + .filter((entry): entry is GitStashSummary => entry != null) + .filter((entry) => stashMatchesBranch(entry.subject, lane.branchRef)); + } + + async function requireBranchStash(lane: LaneInfo, stashRef: string): Promise { + const stash = (await listBranchStashes(lane)).find((entry) => entry.ref === stashRef); + if (!stash) { + const branch = normalizeBranchForStashComparison(lane.branchRef); + throw new Error(`Stash ${stashRef} is not saved for branch ${branch || lane.branchRef}.`); + } + return stash; + } + async function assertCommitMessageGenerationEnabled(): Promise { if (!aiIntegrationService.getFeatureFlag("commit_messages")) { throw new Error("AI commit messages are off. Enable Commit Messages in Settings or type a commit message manually."); @@ -913,6 +987,7 @@ export function createGitOperationsService({ async stashPush(args: GitStashPushArgs): Promise { const message = args.message?.trim(); + invalidateStashReadCaches(); const { action } = await runLaneOperation({ laneId: args.laneId, kind: "git_stash_push", @@ -930,6 +1005,7 @@ export function createGitOperationsService({ await runGitOrThrow(cmd, { cwd: lane.worktreePath, timeoutMs: 30_000 }); } }); + invalidateStashReadCaches(); return action; }, @@ -937,24 +1013,7 @@ export function createGitOperationsService({ const laneId = args.laneId.trim(); return readLaneCached(`stashes:${laneId}:default`, 1_500, async () => { const lane = laneService.getLaneBaseAndBranch(laneId); - const out = await runGitOrThrow(["stash", "list", "--format=%gd%x1f%cI%x1f%gs"], { - cwd: lane.worktreePath, - timeoutMs: 15_000 - }); - return out - .split("\n") - .map((line) => line.trim()) - .filter(Boolean) - .map((line): GitStashSummary | null => { - const [ref, createdAt, subject] = parseDelimited(line); - if (!ref) return null; - return { - ref, - createdAt: createdAt && createdAt.length ? createdAt : null, - subject: subject ?? "" - }; - }) - .filter((entry): entry is GitStashSummary => entry != null); + return listBranchStashes(lane); }); }, @@ -967,6 +1026,7 @@ export function createGitOperationsService({ reason: "stash_apply", metadata: { stashRef }, fn: async (lane) => { + await requireBranchStash(lane, stashRef); await runGitOrThrow(["stash", "apply", stashRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); } }); @@ -982,9 +1042,11 @@ export function createGitOperationsService({ reason: "stash_pop", metadata: { stashRef }, fn: async (lane) => { + await requireBranchStash(lane, stashRef); await runGitOrThrow(["stash", "apply", stashRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); try { await runGitOrThrow(["stash", "drop", stashRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); + invalidateStashReadCaches(); } catch (error) { const message = error instanceof Error ? error.message : String(error); logger.warn("git.stash_pop_drop_failed", { @@ -1001,28 +1063,36 @@ export function createGitOperationsService({ async stashDrop(args: GitStashRefArgs): Promise { const stashRef = args.stashRef.trim(); if (!stashRef.length) throw new Error("stashRef is required"); + invalidateStashReadCaches(); const { action } = await runLaneOperation({ laneId: args.laneId, kind: "git_stash_drop", reason: "stash_drop", metadata: { stashRef }, fn: async (lane) => { + await requireBranchStash(lane, stashRef); await runGitOrThrow(["stash", "drop", stashRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); } }); + invalidateStashReadCaches(); return action; }, async stashClear(args: { laneId: string }): Promise { + invalidateStashReadCaches(); const { action } = await runLaneOperation({ laneId: args.laneId, kind: "git_stash_clear", reason: "stash_clear", metadata: {}, fn: async (lane) => { - await runGitOrThrow(["stash", "clear"], { cwd: lane.worktreePath, timeoutMs: 15_000 }); + const stashes = sortStashesForDrop(await listBranchStashes(lane)); + for (const stash of stashes) { + await runGitOrThrow(["stash", "drop", stash.ref], { cwd: lane.worktreePath, timeoutMs: 30_000 }); + } } }); + invalidateStashReadCaches(); return action; }, diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 52ca5f9ae..7a350ddbe 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -146,6 +146,9 @@ import type { GitSyncArgs, GitHubRepoRef, GitHubStatus, + CreateLaneFromPrBranchArgs, + CreateLaneFromPrBranchPreflightResult, + CreateLaneFromPrBranchResult, CreatePrFromLaneArgs, CreateIntegrationPrArgs, CreateIntegrationPrResult, @@ -8276,6 +8279,26 @@ export function registerIpc({ return result; }); + const ensurePrMutationContext = (): AppContext => { + const ctx = getCtx(); + if (!ctx.prService || !ctx.prPollingService) { + throw new Error("PR service is not available for this project window."); + } + return ctx; + }; + + ipcMain.handle(IPC.prsPreflightCreateLaneFromPrBranch, async (_event, arg: CreateLaneFromPrBranchArgs): Promise => { + const ctx = ensurePrMutationContext(); + return await ctx.prService.preflightCreateLaneFromPrBranch(arg); + }); + + ipcMain.handle(IPC.prsCreateLaneFromPrBranch, async (_event, arg: CreateLaneFromPrBranchArgs): Promise => { + const ctx = ensurePrMutationContext(); + const result = await ctx.prService.createLaneFromPrBranch(arg); + ctx.prPollingService.poke(); + return result; + }); + const ensurePrPolling = () => { const ctx = getCtx(); // PR services are only attached to fully-initialised project contexts. When @@ -8287,6 +8310,26 @@ export function registerIpc({ ctx.prPollingService.start(); return ctx; }; + const emptyPrMergeContext = (prId: string): PrMergeContext => ({ + prId, + groupId: null, + groupType: null, + sourceLaneIds: [], + targetLaneId: null, + integrationLaneId: null, + members: [], + }); + const emptyPrDetail = (prId: string) => ({ + prId, + body: null, + labels: [], + assignees: [], + requestedReviewers: [], + author: { login: "", avatarUrl: null }, + isDraft: false, + milestone: null, + linkedIssues: [], + }); ipcMain.handle(IPC.prsGetForLane, async (_event, arg: { laneId: string }): Promise => { const ctx = getCtx(); @@ -8425,13 +8468,26 @@ export function registerIpc({ return result; }); - ipcMain.handle(IPC.prsGetConflictAnalysis, async (_event, arg: { prId: string }) => getCtx().prService.getConflictAnalysis(arg.prId)); + ipcMain.handle(IPC.prsGetConflictAnalysis, async (_event, arg: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return null; + return ctx.prService.getConflictAnalysis(arg.prId); + }); - ipcMain.handle(IPC.prsGetMergeContext, async (_event, arg: { prId: string }): Promise => getCtx().prService.getMergeContext(arg.prId)); + ipcMain.handle(IPC.prsGetMergeContext, async (_event, arg: { prId: string }): Promise => { + const ctx = ensurePrPolling(); + if (!ctx) return emptyPrMergeContext(arg.prId); + return ctx.prService.getMergeContext(arg.prId); + }); - ipcMain.handle(IPC.prsGetMergeContexts, async (_event, arg: { prIds?: string[] }): Promise> => - getCtx().prService.getMergeContexts(Array.isArray(arg?.prIds) ? arg.prIds : []) - ); + ipcMain.handle(IPC.prsGetMergeContexts, async (_event, arg: { prIds?: string[] }): Promise> => { + const prIds = Array.isArray(arg?.prIds) ? arg.prIds : []; + const ctx = ensurePrPolling(); + if (!ctx) { + return Object.fromEntries(prIds.map((prId) => [prId, emptyPrMergeContext(prId)])); + } + return ctx.prService.getMergeContexts(prIds); + }); ipcMain.handle(IPC.prsListWithConflicts, async (_event, arg?: { includeConflictAnalysis?: boolean }) => { const ctx = ensurePrPolling(); @@ -8441,9 +8497,11 @@ export function registerIpc({ }); }); - ipcMain.handle(IPC.prsListSnapshots, async (_event, arg?: { prId?: string }) => - getCtx().prService.listSnapshots({ prId: typeof arg?.prId === "string" ? arg.prId : undefined }) - ); + ipcMain.handle(IPC.prsListSnapshots, async (_event, arg?: { prId?: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.listSnapshots({ prId: typeof arg?.prId === "string" ? arg.prId : undefined }); + }); ipcMain.handle(IPC.prsGetGitHubSnapshot, async (_event, arg?: { force?: boolean; includeExternalClosed?: boolean }): Promise => { const ctx = ensurePrPolling(); @@ -8511,17 +8569,18 @@ export function registerIpc({ ipcMain.handle(IPC.prsStartQueueAutomation, async (_event, arg) => { const ctx = getCtx(); + if (!ctx.queueLandingService) throw new Error("Queue automation is unavailable in this runtime."); return await ctx.queueLandingService.startQueue(arg); }); - ipcMain.handle(IPC.prsPauseQueueAutomation, async (_event, arg) => getCtx().queueLandingService.pauseQueue(arg.queueId)); + ipcMain.handle(IPC.prsPauseQueueAutomation, async (_event, arg) => getCtx().queueLandingService?.pauseQueue(arg.queueId) ?? null); ipcMain.handle(IPC.prsResumeQueueAutomation, async (_event, arg) => { const ctx = getCtx(); - return ctx.queueLandingService.resumeQueue(arg); + return ctx.queueLandingService?.resumeQueue(arg) ?? null; }); - ipcMain.handle(IPC.prsCancelQueueAutomation, async (_event, arg) => getCtx().queueLandingService.cancelQueue(arg.queueId)); + ipcMain.handle(IPC.prsCancelQueueAutomation, async (_event, arg) => getCtx().queueLandingService?.cancelQueue(arg.queueId) ?? null); ipcMain.handle(IPC.prsReorderQueue, async (_event, arg: ReorderQueuePrsArgs): Promise => { const ctx = getCtx(); @@ -8529,13 +8588,29 @@ export function registerIpc({ ctx.prPollingService.poke(); }); - ipcMain.handle(IPC.prsGetHealth, async (_event, arg: { prId: string }): Promise => getCtx().prService.getPrHealth(arg.prId)); + ipcMain.handle(IPC.prsGetHealth, async (_event, arg: { prId: string }): Promise => { + const ctx = ensurePrPolling(); + if (!ctx) { + return { + prId: arg.prId, + laneId: "", + state: "open", + checksStatus: "none", + reviewStatus: "none", + conflictAnalysis: null, + rebaseNeeded: false, + behindBy: 0, + mergeContext: null, + }; + } + return ctx.prService.getPrHealth(arg.prId); + }); ipcMain.handle(IPC.prsGetQueueState, async (_event, arg: { groupId: string }): Promise => - getCtx().queueLandingService.getQueueStateByGroup(arg.groupId) + getCtx().queueLandingService?.getQueueStateByGroup(arg.groupId) ?? null ); - ipcMain.handle(IPC.prsListQueueStates, async (_event, arg = {}) => getCtx().queueLandingService.listQueueStates(arg)); + ipcMain.handle(IPC.prsListQueueStates, async (_event, arg = {}) => getCtx().queueLandingService?.listQueueStates(arg) ?? []); ipcMain.handle(IPC.prsCreateIntegrationLaneForProposal, async (_event, arg: CreateIntegrationLaneForProposalArgs): Promise => getCtx().prService.createIntegrationLaneForProposal(arg)); @@ -8887,11 +8962,31 @@ export function registerIpc({ ); }); - ipcMain.handle(IPC.prsGetDetail, (_e, args: { prId: string }) => getCtx().prService.getDetail(args.prId)); - ipcMain.handle(IPC.prsGetFiles, (_e, args: { prId: string }) => getCtx().prService.getFiles(args.prId)); - ipcMain.handle(IPC.prsGetCommits, (_e, args: { prId: string }): Promise => getCtx().prService.getCommits(args.prId)); - ipcMain.handle(IPC.prsGetActionRuns, (_e, args: { prId: string }) => getCtx().prService.getActionRuns(args.prId)); - ipcMain.handle(IPC.prsGetActivity, (_e, args: { prId: string }) => getCtx().prService.getActivity(args.prId)); + ipcMain.handle(IPC.prsGetDetail, (_e, args: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return emptyPrDetail(args.prId); + return ctx.prService.getDetail(args.prId); + }); + ipcMain.handle(IPC.prsGetFiles, (_e, args: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.getFiles(args.prId); + }); + ipcMain.handle(IPC.prsGetCommits, (_e, args: { prId: string }): Promise | PrCommit[] => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.getCommits(args.prId); + }); + ipcMain.handle(IPC.prsGetActionRuns, (_e, args: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.getActionRuns(args.prId); + }); + ipcMain.handle(IPC.prsGetActivity, (_e, args: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.getActivity(args.prId); + }); ipcMain.handle(IPC.prsAddComment, (_e, args) => getCtx().prService.addComment(args)); ipcMain.handle(IPC.prsReplyToReviewThread, (_e, args: ReplyToPrReviewThreadArgs) => getCtx().prService.replyToReviewThread(args)); ipcMain.handle(IPC.prsResolveReviewThread, (_e, args: ResolvePrReviewThreadArgs) => getCtx().prService.resolveReviewThread(args)); @@ -8906,9 +9001,13 @@ export function registerIpc({ ipcMain.handle(IPC.prsAiReviewSummary, (_e, args) => getCtx().prService.aiReviewSummary(args)); // PRs Tab redesign (Timeline + Rails) - ipcMain.handle(IPC.prsGetDeployments, (_e, args: { prId: string }) => getCtx().prService.getDeployments(args.prId)); - ipcMain.handle(IPC.prsGetAiSummary, (_e, args: { prId: string }) => getCtx().prSummaryService.getSummary(args.prId)); - ipcMain.handle(IPC.prsRegenerateAiSummary, (_e, args: { prId: string }) => getCtx().prSummaryService.regenerateSummary(args.prId)); + ipcMain.handle(IPC.prsGetDeployments, (_e, args: { prId: string }) => { + const ctx = ensurePrPolling(); + if (!ctx) return []; + return ctx.prService.getDeployments(args.prId); + }); + ipcMain.handle(IPC.prsGetAiSummary, (_e, args: { prId: string }) => getCtx().prSummaryService?.getSummary(args.prId) ?? null); + ipcMain.handle(IPC.prsRegenerateAiSummary, (_e, args: { prId: string }) => getCtx().prSummaryService?.regenerateSummary(args.prId) ?? null); ipcMain.handle(IPC.prsPostReviewComment, (_e, args: PostPrReviewCommentArgs) => getCtx().prService.postReviewComment(args)); ipcMain.handle( IPC.prsSetReviewThreadResolved, diff --git a/apps/desktop/src/main/services/lanes/laneService.test.ts b/apps/desktop/src/main/services/lanes/laneService.test.ts index 920d36fb3..dd249ec73 100644 --- a/apps/desktop/src/main/services/lanes/laneService.test.ts +++ b/apps/desktop/src/main/services/lanes/laneService.test.ts @@ -3153,6 +3153,14 @@ describe("laneService delete teardown + cancellation + streaming", () => { "insert into pr_issue_inventory(id, pr_id, source, type, external_id, headline, created_at, updated_at) values (?, ?, ?, ?, ?, ?, ?, ?)", ["issue-child", "pr-child", "review", "comment", "1", "Fix it", now, now], ); + db.run( + ` + insert into pr_auto_link_ignores( + project_id, repo_owner, repo_name, github_pr_number, lane_id, head_branch, created_at + ) values (?, ?, ?, ?, ?, ?, ?) + `, + [projectId, "acme", "demo", 1, "lane-child", "feature/child", now], + ); db.run("insert into pr_convergence_state(pr_id, active_lane_id, created_at, updated_at) values (?, ?, ?, ?)", ["pr-child", "lane-child", now, now]); db.run("insert into pr_convergence_state(pr_id, active_lane_id, created_at, updated_at) values (?, ?, ?, ?)", ["pr-parent", "lane-child", now, now]); @@ -3205,6 +3213,7 @@ describe("laneService delete teardown + cancellation + streaming", () => { expect(count("pull_request_snapshots", "pr_id = ?", ["pr-child"])).toBe(0); expect(count("pr_pipeline_settings", "pr_id = ?", ["pr-child"])).toBe(0); expect(count("pr_issue_inventory", "pr_id = ?", ["pr-child"])).toBe(0); + expect(count("pr_auto_link_ignores", "lane_id = ?", ["lane-child"])).toBe(0); expect(db.get<{ active_lane_id: string | null }>("select active_lane_id from pr_convergence_state where pr_id = ?", ["pr-parent"])?.active_lane_id).toBeNull(); expect(count("terminal_sessions", "lane_id = ?", ["lane-child"])).toBe(0); expect(count("session_deltas", "lane_id = ?", ["lane-child"])).toBe(0); diff --git a/apps/desktop/src/main/services/lanes/laneService.ts b/apps/desktop/src/main/services/lanes/laneService.ts index e0ab966f5..b563f3f73 100644 --- a/apps/desktop/src/main/services/lanes/laneService.ts +++ b/apps/desktop/src/main/services/lanes/laneService.ts @@ -1955,6 +1955,7 @@ export function createLaneService({ db.run("delete from pr_pipeline_settings where pr_id in (select id from pull_requests where lane_id = ? and project_id = ?)", [laneId, projectId]); db.run("delete from pr_issue_inventory where pr_id in (select id from pull_requests where lane_id = ? and project_id = ?)", [laneId, projectId]); db.run("delete from pull_requests where lane_id = ? and project_id = ?", [laneId, projectId]); + db.run("delete from pr_auto_link_ignores where lane_id = ? and project_id = ?", [laneId, projectId]); db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); db.run("delete from review_finding_feedback where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index 28fe8fe3c..a1133e770 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -143,6 +143,32 @@ function makeGitHubPull(overrides?: Partial>) { }; } +function makeUnmappedBranchPull(overrides?: Partial>) { + return makeGitHubPull({ + node_id: "PR_node_unmapped", + number: 404, + html_url: "https://github.com/test-owner/test-repo/pull/404", + title: "Unmapped branch PR", + base: { + ref: "main", + repo: { + owner: { login: REPO.owner }, + name: REPO.name, + }, + }, + head: { + ref: "feature/unmapped", + sha: "head-sha-unmapped", + user: { login: REPO.owner }, + repo: { + owner: { login: REPO.owner }, + name: REPO.name, + }, + }, + ...overrides, + }); +} + function makeGithubService(overrides?: Record) { return { getRepoOrThrow: vi.fn(async () => REPO), @@ -159,6 +185,7 @@ function makeLaneService(lanes?: unknown[]) { return { list: vi.fn(async () => lanes ?? [makeFakeLane()]), getLaneBaseAndBranch: vi.fn(), + delete: vi.fn(async () => undefined), } as any; } @@ -202,6 +229,10 @@ function buildService(opts: BuildServiceOpts = {}) { if (command === "fetch" || command === "push") { return { exitCode: 0, stdout: "", stderr: "" }; } + if (command === "ls-remote") { + const branch = String(args[3] ?? "feature/unmapped"); + return { exitCode: 0, stdout: `head-sha-unmapped\trefs/heads/${branch}\n`, stderr: "" }; + } // Make runGit succeed for upstream check (returns exitCode 0 → push path) return { exitCode: 0, stdout: "origin/my-feature", stderr: "" }; }); @@ -224,6 +255,96 @@ function buildService(opts: BuildServiceOpts = {}) { return { service, db, githubService, laneService, logger }; } +function serviceWithPrBranchActions(service: ReturnType["service"]) { + return service as typeof service & { + preflightCreateLaneFromPrBranch: (args: { prUrlOrNumber: string; laneName?: string }) => Promise; + createLaneFromPrBranch: (args: { prUrlOrNumber: string; laneName?: string }) => Promise; + }; +} + +function preflightDisposition(preflight: any): string { + if (typeof preflight?.status === "string") return preflight.status; + if (typeof preflight?.state === "string") return preflight.state; + if (typeof preflight?.ok === "boolean") return preflight.ok ? "ready" : "blocked"; + if (typeof preflight?.blocked === "boolean") return preflight.blocked ? "blocked" : "ready"; + return ""; +} + +function preflightConflicts(preflight: any): unknown[] { + if (Array.isArray(preflight?.blockingConflicts)) return preflight.blockingConflicts; + if (Array.isArray(preflight?.conflicts)) return preflight.conflicts; + if (Array.isArray(preflight?.blockers)) return preflight.blockers; + if (preflight?.blockingConflict) return [preflight.blockingConflict]; + return []; +} + +function installPullRequestRowStore(db: ReturnType, initialRows: any[] = []) { + const rows = [...initialRows]; + + db.get.mockImplementation((sql: string, params: unknown[] = []) => { + const text = String(sql); + if (!text.includes("from pull_requests")) return null; + if (text.includes("where id = ?")) { + return rows.find((row) => row.id === params[0] && row.project_id === params[1]) ?? null; + } + if (text.includes("lower(repo_owner)") && text.includes("github_pr_number")) { + const [projectIdParam, owner, name, prNumber] = params; + return rows.find((row) => + row.project_id === projectIdParam + && String(row.repo_owner).toLowerCase() === String(owner).toLowerCase() + && String(row.repo_name).toLowerCase() === String(name).toLowerCase() + && Number(row.github_pr_number) === Number(prNumber) + ) ?? null; + } + if (text.includes("where lane_id = ?")) { + return rows.find((row) => row.lane_id === params[0] && row.project_id === params[1]) ?? null; + } + return null; + }); + + db.all.mockImplementation((sql: string, params: unknown[] = []) => { + const text = String(sql); + if (!text.includes("from pull_requests")) return []; + if (text.includes("where lane_id = ?")) { + return rows.filter((row) => row.lane_id === params[0] && row.project_id === params[1]); + } + if (text.includes("where project_id = ?")) { + return rows.filter((row) => row.project_id === params[0]); + } + return rows; + }); + + db.run.mockImplementation((sql: string, params: unknown[] = []) => { + const text = String(sql); + if (!text.includes("insert into pull_requests(")) return undefined; + rows.push({ + id: params[0], + project_id: params[1], + lane_id: params[2], + repo_owner: params[3], + repo_name: params[4], + github_pr_number: params[5], + github_url: params[6], + github_node_id: params[7], + title: params[8], + state: params[9], + base_branch: params[10], + head_branch: params[11], + checks_status: params[12], + review_status: params[13], + additions: params[14], + deletions: params[15], + last_synced_at: params[16], + created_at: params[17], + updated_at: params[18], + creation_strategy: params[19] ?? null, + }); + return undefined; + }); + + return rows; +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -375,6 +496,115 @@ describe("prService.getGithubSnapshot", () => { vi.clearAllMocks(); }); + it("serves local PR rows on a cold cache without waiting for live GitHub status", async () => { + let resolveStatus!: (value: unknown) => void; + const githubStatusRequest = new Promise((resolve) => { + resolveStatus = resolve; + }); + const githubService = makeGithubService({ + getStatus: vi.fn(() => githubStatusRequest), + apiRequest: vi.fn(async () => ({ data: [] })), + }); + const db = makeMockDb(); + const cachedRow = makePrRow({ + github_pr_number: 321, + title: "Local cached PR", + last_synced_at: "2026-01-02T00:00:00Z", + updated_at: "2026-01-02T00:00:00Z", + }); + db.all.mockImplementation((sql: string) => { + const text = String(sql); + if (text.includes("from pull_requests")) return [cachedRow]; + return []; + }); + const { service } = buildService({ db, githubService, laneService: makeLaneService([makeFakeLane()]) }); + + const snapshot = await service.getGithubSnapshot(); + + expect(snapshot).toMatchObject({ + repo: REPO, + viewerLogin: null, + repoPullRequests: [ + expect.objectContaining({ + githubPrNumber: 321, + title: "Local cached PR", + linkedPrId: "pr-row-1", + linkedLaneId: LANE_ID, + linkedLaneName: "my-feature", + adeKind: "single", + }), + ], + externalPullRequests: [], + syncedAt: "2026-01-02T00:00:00Z", + }); + expect(githubService.getStatus).toHaveBeenCalledTimes(1); + expect(githubService.apiRequest).not.toHaveBeenCalled(); + + resolveStatus({ + tokenStored: true, + repo: REPO, + userLogin: "octocat", + }); + await flushMicrotasks(); + }); + + it("keeps branch PR auto-link running in the background after a local snapshot response", async () => { + const githubService = makeGithubService({ + getStatus: vi.fn(async () => ({ + tokenStored: true, + repo: REPO, + userLogin: "octocat", + })), + apiRequest: vi.fn(async (args: { path: string; query?: Record }) => { + if (args.path !== `/repos/${REPO.owner}/${REPO.name}/pulls`) { + throw new Error(`Unexpected GitHub API path: ${args.path}`); + } + if (args.query?.head === `${REPO.owner}:feature/missed`) { + return { + data: [ + makeGitHubPull({ + number: 654, + title: "Background linked PR", + head: { + ref: "feature/missed", + user: { login: REPO.owner }, + repo: { owner: { login: REPO.owner }, name: REPO.name }, + }, + }), + ], + }; + } + return { data: [] }; + }), + }); + const db = makeMockDb(); + db.all.mockImplementation((sql: string) => { + const text = String(sql); + if (text.includes("from pull_requests")) return [makePrRow({ title: "Already cached PR" })]; + return []; + }); + const laneService = makeLaneService([ + makeFakeLane(), + makeFakeLane({ id: "lane-missed", branchRef: "refs/heads/feature/missed" }), + ]); + const { service } = buildService({ db, githubService, laneService }); + + const snapshot = await service.getGithubSnapshot(); + + expect(snapshot.repoPullRequests[0]?.title).toBe("Already cached PR"); + expect(githubService.apiRequest).not.toHaveBeenCalled(); + + await flushMicrotasks(30); + + expect(githubService.apiRequest).toHaveBeenCalledWith(expect.objectContaining({ + query: expect.objectContaining({ head: `${REPO.owner}:feature/missed` }), + })); + expect(db.run).toHaveBeenCalledWith( + expect.stringContaining("insert into pull_requests("), + expect.arrayContaining(["lane-missed", REPO.owner, REPO.name, 654, "Background linked PR", "open", "main", "feature/missed"]), + ); + }); + it("returns stale cached data immediately while revalidating in the background", async () => { const nowSpy = vi.spyOn(Date, "now").mockReturnValue(Date.parse("2026-01-01T00:00:00Z")); let resolveRevalidation!: (value: unknown) => void; @@ -1349,6 +1579,268 @@ describe("prService merge contexts", () => { }); }); +describe("prService.createLaneFromPrBranch", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const prUrl = "https://github.com/test-owner/test-repo/pull/404"; + const primaryLane = makeFakeLane({ + id: "lane-primary", + name: "main", + laneType: "primary", + branchRef: "refs/heads/main", + baseRef: "refs/heads/main", + worktreePath: "/tmp/test-project", + parentLaneId: null, + }); + + function makeBranchPrGithubService(overrides?: Record) { + return makeGithubService({ + apiRequest: vi.fn(async (args: { path: string }) => { + if (args.path === `/repos/${REPO.owner}/${REPO.name}/pulls/404`) { + return { + data: makeUnmappedBranchPull(), + response: { status: 200, headers: new Headers() }, + }; + } + return { data: [], response: { status: 200, headers: new Headers() } }; + }), + ...overrides, + }); + } + + it("preflights an unmapped PR branch without creating a lane or PR row", async () => { + const githubService = makeBranchPrGithubService(); + const laneService = { + ...makeLaneService([primaryLane]), + importBranch: vi.fn(), + } as any; + const db = makeMockDb(); + installPullRequestRowStore(db); + const { service } = buildService({ db, githubService, laneService }); + + const result = await serviceWithPrBranchActions(service).preflightCreateLaneFromPrBranch({ + prUrlOrNumber: prUrl, + }); + + expect(result).toEqual(expect.objectContaining({ + preflight: expect.objectContaining({ + githubPrNumber: 404, + headBranch: "feature/unmapped", + baseBranch: "main", + }), + lane: null, + })); + expect(preflightDisposition(result.preflight)).toBe("ready"); + expect(preflightConflicts(result.preflight)).toEqual([]); + expect(laneService.importBranch).not.toHaveBeenCalled(); + expect(db.run).not.toHaveBeenCalledWith( + expect.stringContaining("insert into pull_requests("), + expect.anything(), + ); + }); + + it("creates a lane from the PR branch, maps the PR to that lane, and returns lane/pr summaries", async () => { + const importedLane = makeFakeLane({ + id: "lane-imported", + name: "Unmapped branch PR", + branchRef: "refs/heads/feature/unmapped", + baseRef: "refs/heads/main", + worktreePath: "/tmp/test-project/.ade/worktrees/feature-unmapped", + parentLaneId: null, + }); + let branchImported = false; + const laneService = { + ...makeLaneService(), + list: vi.fn(async () => branchImported ? [primaryLane, importedLane] : [primaryLane]), + importBranch: vi.fn(async () => { + branchImported = true; + return importedLane; + }), + } as any; + const githubService = makeBranchPrGithubService(); + const db = makeMockDb(); + installPullRequestRowStore(db); + const { service } = buildService({ db, githubService, laneService }); + + const result = await serviceWithPrBranchActions(service).createLaneFromPrBranch({ + prUrlOrNumber: prUrl, + laneName: "Unmapped branch PR", + }); + + expect(laneService.importBranch).toHaveBeenCalledWith(expect.objectContaining({ + name: "Unmapped branch PR", + baseBranch: "main", + })); + expect(laneService.importBranch.mock.calls[0]?.[0]?.branchRef).toMatch(/feature\/unmapped$/); + expect(db.run).toHaveBeenCalledWith( + expect.stringContaining("insert into pull_requests("), + expect.arrayContaining(["lane-imported", REPO.owner, REPO.name, 404, "Unmapped branch PR", "open", "main", "feature/unmapped"]), + ); + expect(result).toEqual(expect.objectContaining({ + preflight: expect.objectContaining({ + githubPrNumber: 404, + headBranch: "feature/unmapped", + baseBranch: "main", + }), + lane: expect.objectContaining({ + id: "lane-imported", + branchRef: "refs/heads/feature/unmapped", + }), + pr: expect.objectContaining({ + laneId: "lane-imported", + githubPrNumber: 404, + headBranch: "feature/unmapped", + baseBranch: "main", + }), + })); + expect(preflightDisposition(result.preflight)).toBe("ready"); + }); + + it("blocks create when the remote branch moves after preflight", async () => { + const laneService = { + ...makeLaneService([primaryLane]), + importBranch: vi.fn(), + } as any; + const githubService = makeBranchPrGithubService(); + const db = makeMockDb(); + installPullRequestRowStore(db); + const { service } = buildService({ db, githubService, laneService }); + let lsRemoteCalls = 0; + mockGit.runGit.mockImplementation(async (args: unknown[]) => { + const command = Array.isArray(args) ? args[0] : null; + if (command === "ls-remote") { + lsRemoteCalls += 1; + const sha = lsRemoteCalls === 1 ? "head-sha-unmapped" : "moved-sha"; + return { exitCode: 0, stdout: `${sha}\trefs/heads/feature/unmapped\n`, stderr: "" }; + } + if (command === "worktree") return { exitCode: 0, stdout: "", stderr: "" }; + if (command === "fetch" || command === "push") return { exitCode: 0, stdout: "", stderr: "" }; + return { exitCode: 0, stdout: "origin/my-feature", stderr: "" }; + }); + + await expect(serviceWithPrBranchActions(service).createLaneFromPrBranch({ + prUrlOrNumber: prUrl, + laneName: "Unmapped branch PR", + })).rejects.toThrow(/does not match the current PR head/i); + + expect(lsRemoteCalls).toBe(2); + expect(laneService.importBranch).not.toHaveBeenCalled(); + }); + + it("cleans up the imported lane when PR linking fails", async () => { + const importedLane = makeFakeLane({ + id: "lane-imported", + name: "Unmapped branch PR", + branchRef: "refs/heads/feature/unmapped", + baseRef: "refs/heads/main", + worktreePath: "/tmp/test-project/.ade/worktrees/feature-unmapped", + parentLaneId: null, + }); + const existingLane = makeFakeLane({ + id: "lane-raced", + name: "Raced lane", + branchRef: "refs/heads/feature/raced", + }); + let branchImported = false; + const laneService = { + ...makeLaneService(), + list: vi.fn(async () => branchImported ? [primaryLane, importedLane, existingLane] : [primaryLane]), + importBranch: vi.fn(async () => { + branchImported = true; + rows.push(makePrRow({ + id: "pr-raced", + lane_id: "lane-raced", + github_pr_number: 404, + head_branch: "feature/unmapped", + })); + return importedLane; + }), + delete: vi.fn(async () => undefined), + } as any; + const githubService = makeBranchPrGithubService(); + const db = makeMockDb(); + const rows = installPullRequestRowStore(db); + const { service } = buildService({ db, githubService, laneService }); + + await expect(serviceWithPrBranchActions(service).createLaneFromPrBranch({ + prUrlOrNumber: prUrl, + laneName: "Unmapped branch PR", + })).rejects.toThrow(/already mapped to lane/i); + + expect(laneService.importBranch).toHaveBeenCalled(); + expect(laneService.delete).toHaveBeenCalledWith({ + laneId: "lane-imported", + deleteBranch: false, + deleteRemoteBranch: false, + force: true, + }); + }); + + it("blocks when the GitHub PR is already mapped to an ADE lane", async () => { + const existingPr = makePrRow({ + id: "pr-existing", + lane_id: "lane-existing", + github_pr_number: 404, + head_branch: "feature/unmapped", + }); + const existingLane = makeFakeLane({ + id: "lane-existing", + name: "Existing lane", + branchRef: "refs/heads/feature/other", + }); + const laneService = { + ...makeLaneService([primaryLane, existingLane]), + importBranch: vi.fn(), + } as any; + const db = makeMockDb(); + installPullRequestRowStore(db, [existingPr]); + const { service } = buildService({ + db, + githubService: makeBranchPrGithubService(), + laneService, + }); + + const result = await serviceWithPrBranchActions(service).preflightCreateLaneFromPrBranch({ + prUrlOrNumber: prUrl, + }); + + expect(preflightDisposition(result.preflight)).toBe("blocked"); + expect(JSON.stringify(preflightConflicts(result.preflight))).toMatch(/already|mapped|linked|existing/i); + expect(result.lane ?? null).toBeNull(); + expect(laneService.importBranch).not.toHaveBeenCalled(); + }); + + it("blocks when another ADE lane already owns the PR head branch", async () => { + const branchOwner = makeFakeLane({ + id: "lane-branch-owner", + name: "Branch owner", + branchRef: "refs/heads/feature/unmapped", + }); + const laneService = { + ...makeLaneService([primaryLane, branchOwner]), + importBranch: vi.fn(), + } as any; + const db = makeMockDb(); + installPullRequestRowStore(db); + const { service } = buildService({ + db, + githubService: makeBranchPrGithubService(), + laneService, + }); + + const result = await serviceWithPrBranchActions(service).preflightCreateLaneFromPrBranch({ + prUrlOrNumber: prUrl, + }); + + expect(preflightDisposition(result.preflight)).toBe("blocked"); + expect(JSON.stringify(preflightConflicts(result.preflight))).toMatch(/branch owner|feature\/unmapped|owned|already/i); + expect(result.lane ?? null).toBeNull(); + expect(laneService.importBranch).not.toHaveBeenCalled(); + }); +}); + describe("prService.createFromLane", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 3444d794a..9d3ca89ca 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -4,6 +4,11 @@ import path from "node:path"; import { randomUUID } from "node:crypto"; import type { BranchPullRequest, + CreateLaneFromPrBranchArgs, + CreateLaneFromPrBranchBlock, + CreateLaneFromPrBranchPreflight, + CreateLaneFromPrBranchPreflightResult, + CreateLaneFromPrBranchResult, CreatePrFromLaneArgs, CreateQueuePrsArgs, CreateQueuePrsResult, @@ -171,6 +176,28 @@ type PullRequestRow = { creation_strategy: string | null; }; +type GithubPrCacheRow = { + project_id: string; + repo_owner: string; + repo_name: string; + github_pr_number: number; + item_json: string; + state: string; + head_branch: string | null; + updated_at: string; + synced_at: string; +}; + +type PrAutoLinkIgnoreRow = { + project_id: string; + repo_owner: string; + repo_name: string; + github_pr_number: number; + lane_id: string; + head_branch: string | null; + created_at: string; +}; + type LanePrLookupRow = { lane_type: string | null; branch_ref: string | null; @@ -1057,14 +1084,19 @@ export function createPrService({ } }; - const markHotRefresh = (prIds: string[]): void => { + const markHotRefresh = ( + prIds: string[], + options: { invalidateGithubSnapshot?: boolean } = {}, + ): void => { const nowMs = Date.now(); const uniquePrIds = [...new Set(prIds.map((prId) => String(prId ?? "").trim()).filter(Boolean))]; if (uniquePrIds.length === 0) return; for (const prId of uniquePrIds) { hotRefreshStartedAtByPrId.set(prId, nowMs); } - invalidateGithubSnapshotCache(); + if (options.invalidateGithubSnapshot !== false) { + invalidateGithubSnapshotCache(); + } onHotRefreshChanged?.(); }; @@ -1559,6 +1591,74 @@ export function createPrService({ return true; }; + const autoLinkIgnoreKey = (args: { + owner: string; + repo: string; + prNumber: number; + laneId: string; + }): string => `${repoPrKey(args.owner, args.repo, args.prNumber)}:${args.laneId}`; + + const listAutoLinkIgnores = (repo: GitHubRepoRef): Set => { + const rows = db.all( + `select project_id, repo_owner, repo_name, github_pr_number, lane_id, head_branch, created_at + from pr_auto_link_ignores + where project_id = ? + and lower(repo_owner) = lower(?) + and lower(repo_name) = lower(?)`, + [projectId, repo.owner, repo.name], + ); + return new Set(rows.map((row) => autoLinkIgnoreKey({ + owner: row.repo_owner, + repo: row.repo_name, + prNumber: Number(row.github_pr_number), + laneId: row.lane_id, + }))); + }; + + const rememberAutoLinkIgnore = (row: PullRequestRow): void => { + db.run( + ` + insert or replace into pr_auto_link_ignores( + project_id, + repo_owner, + repo_name, + github_pr_number, + lane_id, + head_branch, + created_at + ) values (?, ?, ?, ?, ?, ?, ?) + `, + [ + projectId, + row.repo_owner, + row.repo_name, + Number(row.github_pr_number), + row.lane_id, + row.head_branch, + nowIso(), + ], + ); + }; + + const clearAutoLinkIgnore = (args: { + repoOwner: string; + repoName: string; + githubPrNumber: number; + laneId: string; + }): void => { + db.run( + ` + delete from pr_auto_link_ignores + where project_id = ? + and lower(repo_owner) = lower(?) + and lower(repo_name) = lower(?) + and github_pr_number = ? + and lane_id = ? + `, + [projectId, args.repoOwner, args.repoName, Number(args.githubPrNumber), args.laneId], + ); + }; + const backfillLanePrRowsFromGithubPulls = (rawPulls: any[], repo: GitHubRepoRef, lanes: LaneSummary[]): number => { const activeLaneByBranch = new Map(); for (const lane of lanes) { @@ -1570,6 +1670,7 @@ export function createPrService({ if (activeLaneByBranch.size === 0) return 0; const backfilledIds: string[] = []; + const ignoredAutoLinks = listAutoLinkIgnores(repo); for (const rawPr of rawPulls) { const headBranch = rawPullHeadBranch(rawPr); const lane = headBranch ? activeLaneByBranch.get(headBranch) ?? null : null; @@ -1580,6 +1681,9 @@ export function createPrService({ const prNumber = asNumber(rawPr?.number); if (!prNumber) continue; + if (ignoredAutoLinks.has(autoLinkIgnoreKey({ owner: repo.owner, repo: repo.name, prNumber, laneId: lane.id }))) { + continue; + } const existingRepoRow = getRowForRepoPr(repo.owner, repo.name, prNumber); if (existingRepoRow && existingRepoRow.lane_id !== lane.id) continue; @@ -1610,12 +1714,18 @@ export function createPrService({ updatedAt: asString(rawPr?.updated_at) || nowIso(), creationStrategy: "pr_target", }; - backfilledIds.push(upsertRow(summary, { allowRepoPrAdoption: true })); + const prId = upsertRow(summary, { allowRepoPrAdoption: true }); + clearAutoLinkIgnore({ + repoOwner: repo.owner, + repoName: repo.name, + githubPrNumber: prNumber, + laneId: lane.id, + }); + backfilledIds.push(prId); } if (backfilledIds.length > 0) { - markHotRefresh(backfilledIds); - invalidateGithubSnapshotCache(); + markHotRefresh(backfilledIds, { invalidateGithubSnapshot: false }); } return backfilledIds.length; }; @@ -1877,6 +1987,265 @@ export function createPrService({ return data; }; + const createLaneFromPrBranchBlock = ( + code: CreateLaneFromPrBranchBlock["code"], + message: string, + extra: Omit = {}, + ): CreateLaneFromPrBranchBlock => ({ code, message, ...extra }); + + const resolveCreateLaneFromPrBranchLocator = async ( + args: CreateLaneFromPrBranchArgs, + ): Promise<{ repo: GitHubRepoRef; prNumber: number }> => { + const locatorText = asString(args.prUrlOrNumber).trim(); + if (locatorText) { + const locator = parsePrLocator(locatorText); + const repo = locator.owner && locator.repo + ? { owner: locator.owner, name: locator.repo } + : await githubService.getRepoOrThrow(); + return { repo, prNumber: locator.number }; + } + + const repoOwner = asString(args.repoOwner).trim(); + const repoName = asString(args.repoName).trim(); + const prNumber = Number(args.githubPrNumber ?? args.prNumber ?? 0); + if (!repoOwner || !repoName) throw new Error("Repository owner and name are required."); + if (!Number.isFinite(prNumber) || prNumber <= 0) throw new Error("GitHub PR number is required."); + return { repo: { owner: repoOwner, name: repoName }, prNumber }; + }; + + const parseRemoteBranchSha = (stdout: string, branchName: string): string | null => { + const suffix = `refs/heads/${branchName}`; + for (const line of stdout.split(/\r?\n/)) { + const trimmed = line.trim(); + if (!trimmed) continue; + const [sha, ref] = trimmed.split(/\s+/); + if (sha && (!ref || ref === suffix || ref.endsWith(suffix))) return sha; + } + const first = stdout.trim().split(/\s+/)[0]; + return first || null; + }; + + const findCheckedOutWorktreeForBranch = async (branchName: string): Promise => { + const res = await runGit(["worktree", "list", "--porcelain"], { + cwd: projectRoot, + timeoutMs: 10_000, + }); + if (res.exitCode !== 0) return null; + + for (const block of res.stdout.split(/\n\n+/)) { + const lines = block.split(/\r?\n/); + const worktreeLine = lines.find((line) => line.startsWith("worktree ")); + const branchLine = lines.find((line) => line.startsWith("branch ")); + const worktreePath = worktreeLine?.slice("worktree ".length).trim() ?? ""; + const checkedBranch = branchLine?.slice("branch ".length).trim() ?? ""; + if (!worktreePath || !checkedBranch) continue; + if (normalizeBranchName(branchNameFromRef(checkedBranch)) === branchName) { + return path.resolve(worktreePath); + } + } + return null; + }; + + const resolveConfiguredRemoteBranch = async (args: { + headBranch: string; + headSha: string | null; + sameRepoHead: boolean; + }): Promise => { + const remoteRef = `origin/${args.headBranch}`; + const lsRemote = await runGit(["ls-remote", "--heads", "origin", args.headBranch], { + cwd: projectRoot, + timeoutMs: 30_000, + }); + let remoteSha = lsRemote.exitCode === 0 + ? parseRemoteBranchSha(lsRemote.stdout, args.headBranch) + : null; + + if (!remoteSha) { + await runGit(["fetch", "--prune", "origin"], { + cwd: projectRoot, + timeoutMs: 60_000, + }).catch(() => null); + const cached = await runGit(["rev-parse", "--verify", `refs/remotes/${remoteRef}`], { + cwd: projectRoot, + timeoutMs: 8_000, + }); + remoteSha = cached.exitCode === 0 ? cached.stdout.trim() || null : null; + } + + if (!remoteSha) { + return createLaneFromPrBranchBlock( + args.sameRepoHead ? "remote_branch_missing" : "fork_unavailable", + args.sameRepoHead + ? `Remote branch '${remoteRef}' was not found. It may have been deleted.` + : `Fork PR branch '${args.headBranch}' is not fetchable from the configured remote.`, + ); + } + + if (args.headSha && remoteSha !== args.headSha) { + return createLaneFromPrBranchBlock( + args.sameRepoHead ? "remote_branch_mismatch" : "fork_unavailable", + args.sameRepoHead + ? `Remote branch '${remoteRef}' does not match the current PR head. Refresh the branch and try again.` + : `Fork PR branch '${args.headBranch}' does not match a branch on the configured remote.`, + ); + } + + return null; + }; + + const buildCreateLaneFromPrBranchPreflight = async ( + args: CreateLaneFromPrBranchArgs, + ): Promise => { + const { repo, prNumber } = await resolveCreateLaneFromPrBranchLocator(args); + const pr = await fetchPr(repo, prNumber); + const githubPrNumber = Number(pr?.number) || prNumber; + const title = asString(pr?.title) || `PR #${githubPrNumber}`; + const githubUrl = asString(pr?.html_url) || `https://github.com/${repo.owner}/${repo.name}/pull/${githubPrNumber}`; + const headBranch = rawPullHeadBranch(pr) || null; + const headRepoOwner = rawPullHeadOwner(pr) || null; + const headRepoName = rawPullHeadRepoName(pr) || null; + const baseBranch = asString(pr?.base?.ref) || null; + const headSha = asString(pr?.head?.sha) || null; + const targetLaneName = asString(args.laneName).trim() || title || headBranch || `PR #${githubPrNumber}`; + const importBranchRef = headBranch ? `origin/${headBranch}` : null; + + const finish = (block: CreateLaneFromPrBranchBlock | null): CreateLaneFromPrBranchPreflight => ({ + repoOwner: repo.owner, + repoName: repo.name, + githubPrNumber, + githubUrl, + title, + headBranch, + headSha, + headRepoOwner, + headRepoName, + remoteBranch: importBranchRef, + importBranchRef, + targetLaneName, + baseBranch, + canCreate: block == null, + status: block == null ? "ready" : "blocked", + blockingConflict: block, + blockingConflicts: block ? [block] : [], + }); + + const existingPr = getRowForRepoPr(repo.owner, repo.name, githubPrNumber); + if (existingPr) { + const lane = (await laneService.list({ includeArchived: true, includeStatus: false })) + .find((entry) => entry.id === existingPr.lane_id); + return finish(createLaneFromPrBranchBlock( + "already_mapped", + `PR #${githubPrNumber} is already mapped to lane '${lane?.name ?? existingPr.lane_id}'.`, + { laneId: existingPr.lane_id, laneName: lane?.name ?? null }, + )); + } + + if (!headBranch) { + return finish(createLaneFromPrBranchBlock( + "missing_head_branch", + `PR #${githubPrNumber} does not have a head branch to import.`, + )); + } + + const lanes = await laneService.list({ includeArchived: true, includeStatus: false }); + const branchOwner = lanes.find((lane) => normalizeBranchName(branchNameFromRef(lane.branchRef)) === headBranch); + if (branchOwner) { + const isPrimary = branchOwner.laneType === "primary"; + const archived = branchOwner.archivedAt ? " archived" : ""; + return finish(createLaneFromPrBranchBlock( + isPrimary ? "default_branch" : "branch_owned", + isPrimary + ? `Branch '${headBranch}' is the primary workspace branch and cannot be imported as a lane.` + : `Branch '${headBranch}' is already owned by${archived} lane '${branchOwner.name}'.`, + { laneId: branchOwner.id, laneName: branchOwner.name }, + )); + } + + if (baseBranch && normalizeBranchName(baseBranch) === headBranch) { + return finish(createLaneFromPrBranchBlock( + "default_branch", + `PR #${githubPrNumber} uses '${headBranch}' as both head and base; ADE will not import the default branch as a lane.`, + )); + } + + const sameRepoHead = rawPullHasSameRepoHead(pr, repo); + const remoteBlock = await resolveConfiguredRemoteBranch({ headBranch, headSha, sameRepoHead }); + if (remoteBlock) return finish(remoteBlock); + + const checkedOutPath = await findCheckedOutWorktreeForBranch(headBranch).catch(() => null); + if (checkedOutPath && path.resolve(checkedOutPath) !== path.resolve(projectRoot)) { + return finish(createLaneFromPrBranchBlock( + "worktree_collision", + `Branch '${headBranch}' is already checked out at '${checkedOutPath}'. Attach or remove that worktree before importing it as a lane.`, + { worktreePath: checkedOutPath }, + )); + } + + return finish(null); + }; + + const preflightCreateLaneFromPrBranch = async ( + args: CreateLaneFromPrBranchArgs, + ): Promise => { + const preflight = await buildCreateLaneFromPrBranchPreflight(args); + return { preflight, lane: null, pr: null }; + }; + + const createLaneFromPrBranch = async ( + args: CreateLaneFromPrBranchArgs, + ): Promise => { + const preflight = await buildCreateLaneFromPrBranchPreflight(args); + if (!preflight.canCreate || !preflight.importBranchRef || !preflight.headBranch) { + throw new Error(preflight.blockingConflict?.message || "PR branch cannot be imported as a lane."); + } + + const sameRepoHead = + Boolean(preflight.headRepoOwner && preflight.headRepoName) + && preflight.headRepoOwner?.toLowerCase() === preflight.repoOwner.toLowerCase() + && preflight.headRepoName?.toLowerCase() === preflight.repoName.toLowerCase(); + const remoteBlock = await resolveConfiguredRemoteBranch({ + headBranch: preflight.headBranch, + headSha: preflight.headSha, + sameRepoHead, + }); + if (remoteBlock) { + throw new Error(remoteBlock.message); + } + + let lane: Awaited> | null = null; + try { + lane = await laneService.importBranch({ + branchRef: preflight.importBranchRef, + name: preflight.targetLaneName, + ...(preflight.baseBranch ? { baseBranch: preflight.baseBranch } : {}), + }); + const pr = await linkToLane({ + laneId: lane.id, + prUrlOrNumber: preflight.githubUrl, + }); + return { preflight, lane, pr }; + } catch (error) { + if (!lane) throw error; + try { + await laneService.delete({ + laneId: lane.id, + deleteBranch: false, + deleteRemoteBranch: false, + force: true, + }); + } catch (cleanupError) { + const cleanupMessage = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); + logger.warn("prs.create_lane_from_pr_branch.cleanup_failed", { + laneId: lane.id, + error: cleanupMessage, + }); + const originalMessage = error instanceof Error ? error.message : String(error); + throw new Error(`${originalMessage} Imported lane cleanup failed: ${cleanupMessage}`); + } + throw error; + } + }; + const graphqlRequest = async (query: string, variables: Record): Promise => { const { data: payload } = await githubService.apiRequest<{ data?: T; @@ -2822,6 +3191,7 @@ export function createPrService({ const row = getRow(args.prId); if (!row) throw new Error(`PR not found: ${args.prId}`); const repo: GitHubRepoRef = { owner: row.repo_owner, name: row.repo_name }; + const localUnmapOnly = args.closeOnGitHub !== true && args.archiveLane !== true; let githubClosed = false; let githubCloseError: string | null = null; @@ -2843,6 +3213,10 @@ export function createPrService({ } } + if (localUnmapOnly) { + rememberAutoLinkIgnore(row); + } + db.run("delete from pr_group_members where pr_id = ?", [row.id]); db.run( ` @@ -2878,6 +3252,8 @@ export function createPrService({ } } + invalidateGithubSnapshotCache(); + return { prId: row.id, laneId: row.lane_id, @@ -3172,6 +3548,12 @@ export function createPrService({ // with an already-existing PR for this branch, we need to adopt the row // that represents that PR (regardless of prior lane attribution). const prId = upsertRow(summary, { allowRepoPrAdoption: true }); + clearAutoLinkIgnore({ + repoOwner: repo.owner, + repoName: repo.name, + githubPrNumber: prNumber, + laneId: lane.id, + }); markHotRefresh([prId]); return await refreshOne(prId); @@ -3200,6 +3582,13 @@ export function createPrService({ // behavior (follow-up 3) instead of being treated as "unset". The // upsertRow path uses COALESCE so we never clobber an existing value. const existingRow = getRowForRepoPr(repo.owner, repo.name, locator.number); + if (existingRow && existingRow.lane_id !== lane.id) { + const existingLane = (await laneService.list({ includeArchived: true, includeStatus: false })) + .find((entry) => entry.id === existingRow.lane_id); + throw new Error( + `Cannot link PR #${locator.number} to lane "${lane.name}" because it is already mapped to lane "${existingLane?.name ?? existingRow.lane_id}".` + ); + } const creationStrategy: PrCreationStrategy = normalizePrCreationStrategy(existingRow?.creation_strategy) ?? "pr_target"; @@ -3227,6 +3616,12 @@ export function createPrService({ }; const prId = upsertRow(summary); + clearAutoLinkIgnore({ + repoOwner: repo.owner, + repoName: repo.name, + githubPrNumber: locator.number, + laneId: lane.id, + }); markHotRefresh([prId]); return await refreshOne(prId); }; @@ -5061,27 +5456,20 @@ export function createPrService({ cachedGithubSnapshotAt = capturedAt; }; - const getGithubSnapshotUncached = async (): Promise => { - const githubStatus = await githubService.getStatus(); - if (!githubStatus.tokenStored) { - throw new Error("GitHub token missing. Set it in Settings to sync pull requests."); - } - - const repo = githubStatus.repo; - if (!repo) { - return { - repo: null, - viewerLogin: githubStatus.userLogin, - repoPullRequests: [], - externalPullRequests: [], - syncedAt: nowIso(), - }; - } + type GithubSnapshotMetadata = { + lanes: LaneSummary[]; + laneById: Map; + pullRequestRows: PullRequestRow[]; + linkedPrByRepoKey: Map; + groupByPrId: Map; + workflowByPrId: Map; + }; + const loadGithubSnapshotMetadata = async (): Promise => { const lanes = await laneService.list({ includeArchived: true, includeStatus: false }); const laneById = new Map(lanes.map((lane) => [lane.id, lane])); - let pullRequestRows = listRows(); - let linkedPrByRepoKey = new Map( + const pullRequestRows = listRows(); + const linkedPrByRepoKey = new Map( pullRequestRows.map((row) => [repoPrKey(row.repo_owner, row.repo_name, Number(row.github_pr_number)), row] as const) ); const groupRows = db.all<{ pr_id: string; group_id: string; group_type: "queue" | "integration" }>( @@ -5099,17 +5487,227 @@ export function createPrService({ if (linkedPrId) workflowByPrId.set(linkedPrId, row); } - const deriveAdeKind = ( - workflow: IntegrationProposalRow | null, - group: { group_type: string } | null | undefined, - linked: PullRequestRow | null, - ): GitHubPrListItem["adeKind"] => { - if (workflow) return "integration"; - if (group?.group_type === "queue") return "queue"; - if (group?.group_type === "integration") return "integration"; - if (linked) return "single"; + return { + lanes, + laneById, + pullRequestRows, + linkedPrByRepoKey, + groupByPrId, + workflowByPrId, + }; + }; + + const deriveGithubSnapshotAdeKind = ( + workflow: IntegrationProposalRow | null, + group: { group_type: string } | null | undefined, + linked: PullRequestRow | null, + ): GitHubPrListItem["adeKind"] => { + if (workflow) return "integration"; + if (group?.group_type === "queue") return "queue"; + if (group?.group_type === "integration") return "integration"; + if (linked) return "single"; + return null; + }; + + const selectLocalGithubSnapshotRepo = ( + cacheRows: GithubPrCacheRow[], + prRows: PullRequestRow[], + ): GitHubRepoRef | null => { + const firstCacheRow = cacheRows.find((row) => asString(row.repo_owner).trim() && asString(row.repo_name).trim()); + if (firstCacheRow) return { owner: firstCacheRow.repo_owner, name: firstCacheRow.repo_name }; + const firstPrRow = prRows.find((row) => asString(row.repo_owner).trim() && asString(row.repo_name).trim()); + return firstPrRow ? { owner: firstPrRow.repo_owner, name: firstPrRow.repo_name } : null; + }; + + const snapshotSyncedAtFromRows = (rows: Array): string => { + let latestValue: string | null = null; + let latestMs = 0; + for (const row of rows) { + const values = + "last_synced_at" in row + ? [row.last_synced_at, row.updated_at, row.created_at] + : [row.synced_at, row.updated_at]; + for (const value of values) { + const ms = parseIsoMs(value); + if (ms > latestMs) { + latestMs = ms; + latestValue = value ?? null; + } + } + } + return latestValue ?? nowIso(); + }; + + const overlayGithubSnapshotLinkage = ( + item: GitHubPrListItem, + metadata: GithubSnapshotMetadata, + ): GitHubPrListItem => { + const linkedPrRow = metadata.linkedPrByRepoKey.get(repoPrKey(item.repoOwner, item.repoName, Number(item.githubPrNumber))) ?? null; + const workflowRow = linkedPrRow ? metadata.workflowByPrId.get(linkedPrRow.id) ?? null : null; + const groupRow = linkedPrRow ? metadata.groupByPrId.get(linkedPrRow.id) ?? null : null; + return { + ...item, + linkedPrId: linkedPrRow?.id ?? null, + linkedGroupId: linkedPrRow ? (asString(workflowRow?.linked_group_id).trim() || groupRow?.group_id || null) : null, + linkedLaneId: linkedPrRow?.lane_id ?? null, + linkedLaneName: linkedPrRow ? (metadata.laneById.get(linkedPrRow.lane_id)?.name ?? linkedPrRow.lane_id) : null, + adeKind: deriveGithubSnapshotAdeKind(workflowRow, groupRow, linkedPrRow), + workflowDisplayState: workflowRow ? parseWorkflowDisplayState(workflowRow.workflow_display_state) : null, + cleanupState: workflowRow ? parseCleanupState(workflowRow.cleanup_state) : null, + }; + }; + + const parseGithubPrCacheItem = ( + row: GithubPrCacheRow, + metadata: GithubSnapshotMetadata, + ): GitHubPrListItem | null => { + try { + const parsed = JSON.parse(row.item_json) as GitHubPrListItem; + if (!parsed || typeof parsed !== "object") return null; + return overlayGithubSnapshotLinkage({ + ...parsed, + repoOwner: row.repo_owner, + repoName: row.repo_name, + githubPrNumber: Number(row.github_pr_number), + state: (row.state as PrState) ?? parsed.state, + headBranch: row.head_branch ?? parsed.headBranch ?? null, + updatedAt: row.updated_at || parsed.updatedAt, + }, metadata); + } catch { return null; + } + }; + + const listGithubPrCacheRows = (): GithubPrCacheRow[] => db.all( + `select project_id, repo_owner, repo_name, github_pr_number, item_json, state, head_branch, updated_at, synced_at + from github_pr_cache + where project_id = ? + order by datetime(updated_at) desc, github_pr_number desc`, + [projectId], + ); + + const upsertGithubPrCache = (items: GitHubPrListItem[], syncedAt = nowIso()): void => { + for (const item of items) { + db.run( + ` + insert or replace into github_pr_cache( + project_id, + repo_owner, + repo_name, + github_pr_number, + item_json, + state, + head_branch, + updated_at, + synced_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?) + `, + [ + projectId, + item.repoOwner, + item.repoName, + Number(item.githubPrNumber), + JSON.stringify(item), + item.state, + item.headBranch, + item.updatedAt, + syncedAt, + ], + ); + } + }; + + const rowToGithubSnapshotItem = ( + row: PullRequestRow, + metadata: GithubSnapshotMetadata, + ): GitHubPrListItem => { + const workflowRow = metadata.workflowByPrId.get(row.id) ?? null; + const groupRow = metadata.groupByPrId.get(row.id) ?? null; + const state: PrState = + row.state === "draft" || row.state === "merged" || row.state === "closed" + ? row.state + : "open"; + return { + id: row.github_node_id || `repo-${row.repo_owner}-${row.repo_name}-${Number(row.github_pr_number)}`, + scope: "repo", + repoOwner: row.repo_owner, + repoName: row.repo_name, + githubPrNumber: Number(row.github_pr_number), + githubUrl: row.github_url, + title: row.title || `PR #${Number(row.github_pr_number)}`, + state, + isDraft: row.state === "draft", + baseBranch: row.base_branch || null, + headBranch: row.head_branch || null, + headRepoOwner: row.repo_owner, + headRepoName: row.repo_name, + author: null, + createdAt: row.created_at, + updatedAt: row.updated_at || row.created_at, + linkedPrId: row.id, + linkedGroupId: asString(workflowRow?.linked_group_id).trim() || groupRow?.group_id || null, + linkedLaneId: row.lane_id, + linkedLaneName: metadata.laneById.get(row.lane_id)?.name ?? row.lane_id, + adeKind: deriveGithubSnapshotAdeKind(workflowRow, groupRow, row), + workflowDisplayState: workflowRow ? parseWorkflowDisplayState(workflowRow.workflow_display_state) : null, + cleanupState: workflowRow ? parseCleanupState(workflowRow.cleanup_state) : null, + labels: [], + isBot: false, + commentCount: 0, + }; + }; + + const getGithubSnapshotFromLocalCache = async (): Promise => { + const cacheRows = listGithubPrCacheRows(); + const metadata = await loadGithubSnapshotMetadata(); + const repo = selectLocalGithubSnapshotRepo(cacheRows, metadata.pullRequestRows); + if (!repo) return null; + + const repoPrefix = `${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}#`; + const repoCacheRows = cacheRows.filter((row) => + repoPrKey(row.repo_owner, row.repo_name, Number(row.github_pr_number)).startsWith(repoPrefix), + ); + const cachedItems = repoCacheRows + .map((row) => parseGithubPrCacheItem(row, metadata)) + .filter((item): item is GitHubPrListItem => item !== null); + const seenKeys = new Set(cachedItems.map((item) => repoPrKey(item.repoOwner, item.repoName, Number(item.githubPrNumber)))); + const repoPrRows = metadata.pullRequestRows + .filter((row) => repoPrKey(row.repo_owner, row.repo_name, Number(row.github_pr_number)).startsWith(repoPrefix)); + const linkedOnlyItems = repoPrRows + .filter((row) => !seenKeys.has(repoPrKey(row.repo_owner, row.repo_name, Number(row.github_pr_number)))) + .map((row) => rowToGithubSnapshotItem(row, metadata)); + const repoPullRequests = [...cachedItems, ...linkedOnlyItems] + .sort((left, right) => parseIsoMs(right.updatedAt) - parseIsoMs(left.updatedAt)); + + if (repoPullRequests.length === 0) return null; + + return { + repo, + viewerLogin: null, + repoPullRequests, + externalPullRequests: [], + syncedAt: snapshotSyncedAtFromRows([...repoCacheRows, ...repoPrRows]), }; + }; + + const getGithubSnapshotUncached = async (): Promise => { + const githubStatus = await githubService.getStatus(); + if (!githubStatus.tokenStored) { + throw new Error("GitHub token missing. Set it in Settings to sync pull requests."); + } + + const repo = githubStatus.repo; + if (!repo) { + return { + repo: null, + viewerLogin: githubStatus.userLogin, + repoPullRequests: [], + externalPullRequests: [], + syncedAt: nowIso(), + }; + } + + let metadata = await loadGithubSnapshotMetadata(); const toGitHubState = (rawPr: any): PrState => { if (rawPr?.merged_at) return "merged"; @@ -5127,9 +5725,9 @@ export function createPrService({ const repoOwner = asString(rawRepo?.owner?.login) || repositoryParts[0] || repo.owner; const repoName = asString(rawRepo?.name) || repositoryParts[1] || repo.name; const githubPrNumber = Number(rawPr?.number) || 0; - const linkedPrRow = linkedPrByRepoKey.get(repoPrKey(repoOwner, repoName, githubPrNumber)) ?? null; - const workflowRow = linkedPrRow ? workflowByPrId.get(linkedPrRow.id) ?? null : null; - const groupRow = linkedPrRow ? groupByPrId.get(linkedPrRow.id) ?? null : null; + const linkedPrRow = metadata.linkedPrByRepoKey.get(repoPrKey(repoOwner, repoName, githubPrNumber)) ?? null; + const workflowRow = linkedPrRow ? metadata.workflowByPrId.get(linkedPrRow.id) ?? null : null; + const groupRow = linkedPrRow ? metadata.groupByPrId.get(linkedPrRow.id) ?? null : null; return { id: asString(rawPr?.node_id) || `${scope}-${repoOwner}-${repoName}-${githubPrNumber}`, @@ -5151,8 +5749,8 @@ export function createPrService({ linkedPrId: linkedPrRow?.id ?? null, linkedGroupId: asString(workflowRow?.linked_group_id).trim() || groupRow?.group_id || null, linkedLaneId: linkedPrRow?.lane_id ?? null, - linkedLaneName: linkedPrRow ? (laneById.get(linkedPrRow.lane_id)?.name ?? linkedPrRow.lane_id) : null, - adeKind: deriveAdeKind(workflowRow, groupRow, linkedPrRow), + linkedLaneName: linkedPrRow ? (metadata.laneById.get(linkedPrRow.lane_id)?.name ?? linkedPrRow.lane_id) : null, + adeKind: deriveGithubSnapshotAdeKind(workflowRow, groupRow, linkedPrRow), workflowDisplayState: workflowRow ? parseWorkflowDisplayState(workflowRow.workflow_display_state) : null, cleanupState: workflowRow ? parseCleanupState(workflowRow.cleanup_state) : null, labels: Array.isArray(rawPr?.labels) @@ -5169,20 +5767,20 @@ export function createPrService({ path: `/repos/${repo.owner}/${repo.name}/pulls`, query: { state: "all", sort: "updated", direction: "desc" }, }); - repoPullRequestsRaw = await fetchMissingSameRepoLanePulls(repoPullRequestsRaw, repo, lanes); - if (backfillLanePrRowsFromGithubPulls(repoPullRequestsRaw, repo, lanes) > 0) { - pullRequestRows = listRows(); - linkedPrByRepoKey = new Map( - pullRequestRows.map((row) => [repoPrKey(row.repo_owner, row.repo_name, Number(row.github_pr_number)), row] as const) - ); + repoPullRequestsRaw = await fetchMissingSameRepoLanePulls(repoPullRequestsRaw, repo, metadata.lanes); + if (backfillLanePrRowsFromGithubPulls(repoPullRequestsRaw, repo, metadata.lanes) > 0) { + metadata = await loadGithubSnapshotMetadata(); } + const repoPullRequests = repoPullRequestsRaw.map((rawPr) => toGitHubItem(rawPr, "repo")); + const syncedAt = nowIso(); + upsertGithubPrCache(repoPullRequests, syncedAt); return { repo, viewerLogin: githubStatus.userLogin, - repoPullRequests: repoPullRequestsRaw.map((rawPr) => toGitHubItem(rawPr, "repo")), + repoPullRequests, externalPullRequests: [], - syncedAt: nowIso(), + syncedAt, }; }; @@ -5233,6 +5831,16 @@ export function createPrService({ } return cachedSnapshot; } + if (!force) { + const localSnapshot = await getGithubSnapshotFromLocalCache(); + if (localSnapshot) { + publishGithubSnapshot(localSnapshot); + if (!githubSnapshotInFlight) { + void startSnapshotRequest(true).catch(() => {}); + } + return localSnapshot; + } + } if (!force && githubSnapshotInFlight) { return githubSnapshotInFlight.request; } @@ -6089,6 +6697,14 @@ export function createPrService({ return await linkToLane(args); }, + async preflightCreateLaneFromPrBranch(args: CreateLaneFromPrBranchArgs): Promise { + return await preflightCreateLaneFromPrBranch(args); + }, + + async createLaneFromPrBranch(args: CreateLaneFromPrBranchArgs): Promise { + return await createLaneFromPrBranch(args); + }, + async cleanupBranch(args: CleanupPrBranchArgs): Promise { return await cleanupBranch(args); }, diff --git a/apps/desktop/src/main/services/review/reviewService.test.ts b/apps/desktop/src/main/services/review/reviewService.test.ts index bf1c46413..cbfa2aa72 100644 --- a/apps/desktop/src/main/services/review/reviewService.test.ts +++ b/apps/desktop/src/main/services/review/reviewService.test.ts @@ -146,6 +146,49 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { created_at text not null ) `); + raw.run(` + create table review_reviewer_runs( + id text primary key, + run_id text not null, + reviewer_key text not null, + label text not null, + focus text not null, + status text not null, + chat_session_id text, + prompt_artifact_id text, + output_artifact_id text, + findings_artifact_id text, + candidate_count integer not null default 0, + kept_count integer not null default 0, + summary text, + error_message text, + started_at text, + ended_at text, + created_at text not null, + updated_at text not null + ) + `); + raw.run(` + create table review_candidate_findings( + id text primary key, + run_id text not null, + reviewer_run_id text not null, + reviewer_key text not null, + title text not null, + severity text not null, + finding_class text, + body text not null, + confidence real not null, + evidence_json text, + file_path text, + line integer, + anchor_state text not null, + evidence_score real not null, + low_signal integer not null default 0, + score real not null, + created_at text not null + ) + `); raw.run(` create table review_run_publications( id text primary key, @@ -488,8 +531,7 @@ function createHarness(args: { mockContextBuilder.buildContext.mockResolvedValue(makeContextPacket()); const runSessionTurn = vi.fn(async () => { - const outputText = queuedOutputs.shift(); - if (!outputText) throw new Error("No mock review output left."); + const outputText = queuedOutputs.shift() ?? makeOutput("No specialist findings.", []); return { sessionId: `session-review-${sessionCount}`, provider: "codex", @@ -499,6 +541,17 @@ function createHarness(args: { }; }); + const createSession = vi.fn(async () => { + sessionCount += 1; + return { + id: `session-review-${sessionCount}`, + laneId: "lane-review", + provider: "codex", + model: "gpt-5.4", + modelId: "openai/gpt-5.4", + }; + }); + const service = createReviewService({ db: db as any, logger: { @@ -524,16 +577,7 @@ function createHarness(args: { listRecentCommits: vi.fn(async () => []), } as any, agentChatService: { - createSession: vi.fn(async () => { - sessionCount += 1; - return { - id: `session-review-${sessionCount}`, - laneId: "lane-review", - provider: "codex", - model: "gpt-5.4", - modelId: "openai/gpt-5.4", - }; - }), + createSession, getSessionSummary: vi.fn(async (sessionId: string) => ({ sessionId, laneId: "lane-review", @@ -597,6 +641,7 @@ function createHarness(args: { return { raw, service, + createSession, runSessionTurn, publishReviewPublication, start: (config?: Partial) => service.startRun({ @@ -642,9 +687,13 @@ describe("reviewService", () => { expect(detail?.findings[0]?.sourcePass).toBe("adjudicated"); expect(detail?.findings[0]?.originatingPasses).toEqual(["diff-risk", "cross-file-impact"]); expect(detail?.findings[0]?.adjudication?.mergedFindingIds).toHaveLength(2); - expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(3); - expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_output")).toHaveLength(3); - expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_findings")).toHaveLength(3); + expect(detail?.reviewerRuns).toHaveLength(5); + expect(detail?.candidateFindings).toHaveLength(2); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(5); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_output")).toHaveLength(5); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_findings")).toHaveLength(5); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "changed_file_manifest")).toBe(true); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "risk_map")).toBe(true); expect(detail?.artifacts.some((artifact) => artifact.artifactType === "adjudication_result")).toBe(true); expect(detail?.artifacts.some((artifact) => artifact.artifactType === "merged_findings")).toBe(true); @@ -654,6 +703,64 @@ describe("reviewService", () => { expect(String(persistedFindings[0]?.adjudication_json)).toContain("publicationEligible"); }); + it("passes Codex fast mode to the automation chat while keeping plan permissions", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("No direct findings.", []), + makeOutput("No cross-file findings.", []), + makeOutput("No checks findings.", []), + ], + config: { codexFastMode: true }, + }); + + const run = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs.some((entry) => entry.id === run.id && entry.status === "completed"), + ); + + expect(harness.createSession).toHaveBeenCalledWith(expect.objectContaining({ + codexFastMode: true, + permissionMode: "plan", + surface: "automation", + })); + }); + + it("preserves unlimited review budgets instead of clamping them", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("No direct findings.", []), + makeOutput("No cross-file findings.", []), + makeOutput("No checks findings.", []), + ], + config: { + budgets: { + unlimited: true, + maxFiles: 1, + maxDiffChars: 4_000, + maxPromptChars: 4_000, + maxFindings: 1, + maxFindingsPerPass: 1, + maxPublishedFindings: 1, + }, + }, + }); + + const run = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs.some((entry) => entry.id === run.id && entry.status === "completed"), + ); + + const saved = (await harness.service.listRuns()).find((entry) => entry.id === run.id); + expect(saved?.config.budgets).toMatchObject({ + unlimited: true, + maxFiles: Number.MAX_SAFE_INTEGER, + maxFindings: Number.MAX_SAFE_INTEGER, + maxPublishedFindings: Number.MAX_SAFE_INTEGER, + }); + }); + it("persists provenance, rules, and validation artifacts and keeps renderer findings on the normal evidence path", async () => { const harness = createHarness({ outputs: [ @@ -1062,9 +1169,13 @@ describe("reviewService", () => { makeOutput("First pass run.", [makeFinding()]), makeOutput("Cross-file overlap.", [makeFinding({ title: "Fallback path removed", confidence: 0.71 })]), makeOutput("Checks clear.", []), + makeOutput("First run security clear.", []), + makeOutput("First run UI clear.", []), makeOutput("Second run diff-risk.", [makeFinding()]), makeOutput("Second run cross-file.", [makeFinding({ title: "Fallback path removed", confidence: 0.71 })]), makeOutput("Second run checks.", []), + makeOutput("Second run security clear.", []), + makeOutput("Second run UI clear.", []), ], target: { mode: "commit_range", @@ -1090,9 +1201,10 @@ describe("reviewService", () => { (runs) => runs.some((run) => run.id === rerun.id && run.status === "completed"), ); - expect(harness.runSessionTurn).toHaveBeenCalledTimes(6); + expect(harness.runSessionTurn).toHaveBeenCalledTimes(10); const rerunDetail = await harness.service.getRunDetail({ runId: rerun.id }); - expect(rerunDetail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(3); + expect(rerunDetail?.reviewerRuns).toHaveLength(5); + expect(rerunDetail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(5); const rerunRecord = (await harness.service.listRuns()).find((entry) => entry.id === rerun.id); expect(rerunRecord?.target).toEqual({ diff --git a/apps/desktop/src/main/services/review/reviewService.ts b/apps/desktop/src/main/services/review/reviewService.ts index 29da7806d..9b175debc 100644 --- a/apps/desktop/src/main/services/review/reviewService.ts +++ b/apps/desktop/src/main/services/review/reviewService.ts @@ -1,4 +1,4 @@ -import { randomUUID } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import type { ReviewArtifactType, ReviewDiffContext, @@ -11,6 +11,7 @@ import type { ReviewFindingAdjudication, ReviewFindingClass, ReviewFindingSuppressionMatch, + ReviewCandidateFinding, ReviewListSuppressionsArgs, ReviewPublication, ReviewPublicationDestination, @@ -24,6 +25,8 @@ import type { ReviewRunConfig, ReviewRunDetail, ReviewRunStatus, + ReviewReviewerRun, + ReviewReviewerRunStatus, ReviewPassKey, ReviewSeverity, ReviewSeveritySummary, @@ -141,6 +144,47 @@ type ReviewRunPublicationRow = { completed_at: string | null; }; +type ReviewReviewerRunRow = { + id: string; + run_id: string; + reviewer_key: string; + label: string; + focus: string; + status: string; + chat_session_id: string | null; + prompt_artifact_id: string | null; + output_artifact_id: string | null; + findings_artifact_id: string | null; + candidate_count: number; + kept_count: number; + summary: string | null; + error_message: string | null; + started_at: string | null; + ended_at: string | null; + created_at: string; + updated_at: string; +}; + +type ReviewCandidateFindingRow = { + id: string; + run_id: string; + reviewer_run_id: string; + reviewer_key: string; + title: string; + severity: string; + finding_class: string | null; + body: string; + confidence: number; + evidence_json: string | null; + file_path: string | null; + line: number | null; + anchor_state: string; + evidence_score: number; + low_signal: number; + score: number; + created_at: string; +}; + const REVIEW_MODEL_FALLBACK_ID = "openai/gpt-5.4"; function resolveBuiltinReviewModelId(): string { @@ -170,11 +214,14 @@ const DEFAULT_BUDGETS: ReviewRunConfig["budgets"] = { maxFindingsPerPass: 6, maxPublishedFindings: 6, }; +const UNLIMITED_BUDGET_VALUE = Number.MAX_SAFE_INTEGER; const REVIEW_PASS_ORDER: ReviewPassKey[] = [ "diff-risk", "cross-file-impact", "checks-and-tests", + "security-data", + "ui-regression", ]; const SEVERITY_SCORE: Record = { @@ -202,6 +249,7 @@ type PassDefinition = { type PassCandidateFinding = { id: string; runId: string; + reviewerRunId: string | null; passKey: ReviewPassKey; title: string; severity: ReviewSeverity; @@ -218,6 +266,9 @@ type PassCandidateFinding = { }; type ReviewContextArtifactIds = { + manifestArtifactId: string; + riskMapArtifactId: string; + diffBundleArtifactId: string | null; provenanceArtifactId: string; rulesArtifactId: string; validationArtifactId: string; @@ -225,6 +276,8 @@ type ReviewContextArtifactIds = { type PassExecutionResult = { pass: PassDefinition; + reviewerRunId: string; + sessionId: string | null; summary: string | null; candidates: PassCandidateFinding[]; promptArtifactId: string; @@ -321,6 +374,17 @@ function mergeFindingClass(classes: Array } function normalizeBudgetConfig(budgets?: Partial | null): ReviewRunConfig["budgets"] { + if (budgets?.unlimited === true) { + return { + unlimited: true, + maxFiles: UNLIMITED_BUDGET_VALUE, + maxDiffChars: UNLIMITED_BUDGET_VALUE, + maxPromptChars: UNLIMITED_BUDGET_VALUE, + maxFindings: UNLIMITED_BUDGET_VALUE, + maxFindingsPerPass: UNLIMITED_BUDGET_VALUE, + maxPublishedFindings: UNLIMITED_BUDGET_VALUE, + }; + } return { maxFiles: clampNumber(Number(budgets?.maxFiles ?? DEFAULT_BUDGETS.maxFiles), 1, 500), maxDiffChars: clampNumber(Number(budgets?.maxDiffChars ?? DEFAULT_BUDGETS.maxDiffChars), 4_000, 1_000_000), @@ -445,6 +509,24 @@ const REVIEW_PASSES: PassDefinition[] = [ "Use check/test context when present, but do not invent failures that were not supplied.", ], }, + { + key: "security-data", + label: "Security and data", + focus: "security regressions, data loss, privacy-sensitive behavior, auth boundaries, persistence, secrets, and unsafe trust changes", + extraInstructions: [ + "Prioritize concrete security, privacy, persistence, and data-integrity failures over generic hardening suggestions.", + "Cite the exact changed path or validation signal that makes the risk plausible.", + ], + }, + { + key: "ui-regression", + label: "UI and regression", + focus: "user-visible regressions, broken UI state, stale hydration, disabled-looking actions, copy clarity, and cross-surface UX fallout", + extraInstructions: [ + "Focus on user-visible behavior and state transitions caused by the diff.", + "Avoid aesthetic nits unless they make a workflow misleading, inaccessible, or functionally broken.", + ], + }, ]; function buildChangedFilesSummary(changedFiles: Array<{ filePath: string }>): string { @@ -454,11 +536,71 @@ function buildChangedFilesSummary(changedFiles: Array<{ filePath: string }>): st return changedFilesSummary; } +function classifyChangedFileRisk(filePath: string): string[] { + const risks: string[] = []; + if (/\b(preload|ipc|shared\/types|types)\b/i.test(filePath)) risks.push("contract"); + if (/\b(kvDb|migration|sqlite|state|sync)\b/i.test(filePath)) risks.push("persistence"); + if (/\b(prs?|review|github|publish|merge|branch)\b/i.test(filePath)) risks.push("workflow"); + if (/\b(renderer|components|tsx$|css$)\b/i.test(filePath)) risks.push("ui"); + if (/\b(auth|credential|secret|token|security|permission|sandbox)\b/i.test(filePath)) risks.push("security"); + if (/\b(test|spec)\b/i.test(filePath)) risks.push("test"); + return risks.length > 0 ? risks : ["direct-diff"]; +} + +function buildChangedFileManifestPayload(args: { + targetLabel: string; + compareTarget: ReviewResolvedCompareTarget | null; + changedFiles: MaterializedChangedFile[]; + budgets: ReviewRunConfig["budgets"]; +}): Record { + return { + targetLabel: args.targetLabel, + compareTarget: args.compareTarget, + fileCount: args.changedFiles.length, + budgetMode: args.budgets.unlimited === true ? "unlimited" : "bounded", + files: args.changedFiles.map((file) => ({ + path: file.filePath, + changedLineCount: file.lineNumbers.length, + changedLineSample: file.lineNumbers.slice(0, 20), + riskTags: classifyChangedFileRisk(file.filePath), + excerpt: truncateText(file.excerpt, 1_500), + })), + }; +} + +function buildRiskMapPayload(changedFiles: MaterializedChangedFile[]): Record { + const groups = new Map(); + for (const file of changedFiles) { + for (const risk of classifyChangedFileRisk(file.filePath)) { + const current = groups.get(risk) ?? []; + current.push(file.filePath); + groups.set(risk, current); + } + } + return { + groups: Array.from(groups.entries()).map(([risk, files]) => ({ + risk, + files, + fileCount: files.length, + })), + }; +} + +function buildManifestPrompt(manifest: Record, riskMap: Record): string { + return JSON.stringify({ + changedFileManifest: manifest, + riskMap, + }, null, 2); +} + function buildContextArtifactHints(args: { artifactIds: ReviewContextArtifactIds; includeValidation: boolean; }): string[] { const lines = [ + `- changed_file_manifest artifact id: ${args.artifactIds.manifestArtifactId}`, + `- risk_map artifact id: ${args.artifactIds.riskMapArtifactId}`, + ...(args.artifactIds.diffBundleArtifactId ? [`- diff_bundle artifact id: ${args.artifactIds.diffBundleArtifactId}`] : []), `- provenance_brief artifact id: ${args.artifactIds.provenanceArtifactId}`, `- rule_overlays artifact id: ${args.artifactIds.rulesArtifactId}`, ]; @@ -471,7 +613,7 @@ function buildContextArtifactHints(args: { function buildPassPrompt(args: { run: ReviewRun; pass: PassDefinition; - diffText: string; + manifestPrompt: string; changedFiles: Array<{ filePath: string }>; context: ReviewContextPacket; contextArtifactIds: ReviewContextArtifactIds; @@ -482,8 +624,8 @@ function buildPassPrompt(args: { return [ "You are ADE's local code reviewer.", - "Review only the provided local diff bundle.", - `This pass is ${args.pass.label.toLowerCase()} and it focuses on ${args.pass.focus}.`, + "Review only the local target represented by the supplied changed-file manifest, risk map, and context artifacts.", + `This specialist is ${args.pass.label.toLowerCase()} and it focuses on ${args.pass.focus}.`, "Prioritize correctness, regressions, security, data loss, race conditions, risky migrations, and missing tests.", "Do not suggest style-only nits or speculative rewrites.", "Every finding must include concrete evidence from the diff bundle or supplied review context.", @@ -497,10 +639,12 @@ function buildPassPrompt(args: { '- "filePath": changed file path when known, otherwise null', '- "line": line number when known, otherwise null', '- "evidence": array of objects with {"summary": string, "quote": string|null, "filePath": string|null, "line": number|null, "artifactId": string|null}', - `Return at most ${args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings} findings.`, + args.run.config.budgets.unlimited === true + ? "Return every real issue you can substantiate from the supplied evidence." + : `Return at most ${args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings} findings.`, "If there are no real issues, return an empty findings array and explain that in summary.", "", - `Pass key: ${args.pass.key}`, + `Reviewer key: ${args.pass.key}`, `Review target: ${args.run.targetLabel}`, `Selection mode: ${args.run.config.selectionMode}`, `Publish behavior: ${args.run.config.publishBehavior}`, @@ -512,6 +656,9 @@ function buildPassPrompt(args: { "Changed files:", changedFilesSummary, "", + "Changed-file manifest and risk map:", + args.manifestPrompt, + "", "Context artifact ids you may cite in evidence when relevant:", ...buildContextArtifactHints({ artifactIds: args.contextArtifactIds, @@ -527,8 +674,8 @@ function buildPassPrompt(args: { "Checks and validation context:", includeValidation ? args.context.validation.prompt : "- Full validation evidence is reserved for the checks-and-tests pass.", "", - "Diff bundle:", - truncateText(args.diffText, args.run.config.budgets.maxPromptChars), + "Exact diff access:", + "The full diff was saved as a run artifact. Use the changed-file manifest excerpts above for first-pass review and cite the diff_bundle artifact when the exact hunk is needed.", ].join("\n"); } @@ -558,6 +705,14 @@ function parseEvidence(value: unknown): ReviewEvidence[] { }); } +function stableReviewId(prefix: string, parts: unknown[]): string { + const hash = createHash("sha256") + .update(JSON.stringify(parts)) + .digest("hex") + .slice(0, 24); + return `${prefix}-${hash}`; +} + function computeAnchorState(args: { filePath: string | null; line: number | null; @@ -723,12 +878,13 @@ function dedupeEvidenceEntries(evidence: ReviewEvidence[]): ReviewEvidence[] { function normalizeParsedFindings(args: { runId: string; + reviewerRunId: string; passKey: ReviewPassKey; parsed: Record | null; changedFilesByPath: Map }>; }): { summary: string | null; findings: PassCandidateFinding[] } { const findingsRaw = Array.isArray(args.parsed?.findings) ? args.parsed?.findings : []; - const findings = findingsRaw.flatMap((entry) => { + const findings = findingsRaw.flatMap((entry, index) => { if (!isRecord(entry)) return []; const title = cleanLine(String(entry.title ?? "")); const body = cleanLine(String(entry.body ?? "")); @@ -757,8 +913,9 @@ function normalizeParsedFindings(args: { const evidenceScore = scoreEvidence(evidence); const confidence = normalizeConfidence(entry.confidence); const finding: PassCandidateFinding = { - id: randomUUID(), + id: stableReviewId("candidate", [args.runId, args.passKey, index, title, filePath, line, body]), runId: args.runId, + reviewerRunId: args.reviewerRunId, passKey: args.passKey, title, severity: normalizeSeverity(entry.severity), @@ -835,7 +992,7 @@ function combineConfidence(findings: PassCandidateFinding[]): number { } function groupPassCandidates(candidates: PassCandidateFinding[]): PassCandidateFinding[][] { - const remaining = [...candidates].sort((left, right) => right.score - left.score); + const remaining = [...candidates].sort(compareCandidatesStable); const groups: PassCandidateFinding[][] = []; while (remaining.length > 0) { const seed = remaining.shift(); @@ -856,6 +1013,22 @@ function groupPassCandidates(candidates: PassCandidateFinding[]): PassCandidateF return groups; } +function compareCandidatesStable(left: PassCandidateFinding, right: PassCandidateFinding): number { + const scoreDelta = right.score - left.score; + if (scoreDelta !== 0) return scoreDelta; + const severityDelta = SEVERITY_SCORE[right.severity] - SEVERITY_SCORE[left.severity]; + if (severityDelta !== 0) return severityDelta; + const passDelta = REVIEW_PASS_ORDER.indexOf(left.passKey) - REVIEW_PASS_ORDER.indexOf(right.passKey); + if (passDelta !== 0) return passDelta; + const pathDelta = String(left.filePath ?? "").localeCompare(String(right.filePath ?? "")); + if (pathDelta !== 0) return pathDelta; + const lineDelta = (left.line ?? Number.MAX_SAFE_INTEGER) - (right.line ?? Number.MAX_SAFE_INTEGER); + if (lineDelta !== 0) return lineDelta; + const titleDelta = left.title.localeCompare(right.title); + if (titleDelta !== 0) return titleDelta; + return left.id.localeCompare(right.id); +} + function getCandidatePathSet(group: PassCandidateFinding[]): string[] { return Array.from(new Set( group.flatMap((candidate) => [ @@ -993,7 +1166,7 @@ function adjudicatePassFindings(args: { const passes = Array.from(new Set(group.map((candidate) => candidate.passKey))).sort( (left, right) => REVIEW_PASS_ORDER.indexOf(left) - REVIEW_PASS_ORDER.indexOf(right), ); - const bestCandidate = [...group].sort((left, right) => right.score - left.score)[0]; + const bestCandidate = [...group].sort(compareCandidatesStable)[0]; if (!bestCandidate) continue; const candidatePaths = getCandidatePathSet(group); const relevantOverlays = args.context.matchedRuleOverlays.filter((overlay) => @@ -1098,7 +1271,14 @@ function adjudicatePassFindings(args: { }; findings.push({ - id: randomUUID(), + id: stableReviewId("finding", [ + args.runId, + preferredAnchor.filePath, + preferredAnchor.line, + bestCandidate.title, + bestCandidate.body, + passes, + ]), runId: args.runId, title: bestCandidate.title, severity, @@ -1119,7 +1299,17 @@ function adjudicatePassFindings(args: { } const keptFindings = findings - .sort((left, right) => (right.adjudication?.score ?? 0) - (left.adjudication?.score ?? 0)) + .sort((left, right) => { + const scoreDelta = (right.adjudication?.score ?? 0) - (left.adjudication?.score ?? 0); + if (scoreDelta !== 0) return scoreDelta; + const severityDelta = SEVERITY_SCORE[right.severity] - SEVERITY_SCORE[left.severity]; + if (severityDelta !== 0) return severityDelta; + const pathDelta = String(left.filePath ?? "").localeCompare(String(right.filePath ?? "")); + if (pathDelta !== 0) return pathDelta; + const lineDelta = (left.line ?? Number.MAX_SAFE_INTEGER) - (right.line ?? Number.MAX_SAFE_INTEGER); + if (lineDelta !== 0) return lineDelta; + return left.title.localeCompare(right.title); + }) .slice(0, args.budgets.maxFindings); const keptIds = new Set(keptFindings.map((finding) => finding.id)); for (const finding of findings) { @@ -1276,6 +1466,51 @@ function mapPublicationRow(row: ReviewRunPublicationRow): ReviewPublication { }; } +function mapReviewerRunRow(row: ReviewReviewerRunRow): ReviewReviewerRun { + return { + id: row.id, + runId: row.run_id, + reviewerKey: row.reviewer_key as ReviewPassKey, + label: row.label, + focus: row.focus, + status: (row.status as ReviewReviewerRunStatus) ?? "failed", + chatSessionId: row.chat_session_id, + promptArtifactId: row.prompt_artifact_id, + outputArtifactId: row.output_artifact_id, + findingsArtifactId: row.findings_artifact_id, + candidateCount: Number(row.candidate_count ?? 0), + keptCount: Number(row.kept_count ?? 0), + summary: row.summary, + errorMessage: row.error_message, + startedAt: row.started_at, + endedAt: row.ended_at, + createdAt: row.created_at, + updatedAt: row.updated_at, + }; +} + +function mapCandidateFindingRow(row: ReviewCandidateFindingRow): ReviewCandidateFinding { + return { + id: row.id, + runId: row.run_id, + reviewerRunId: row.reviewer_run_id, + reviewerKey: row.reviewer_key as ReviewPassKey, + title: row.title, + severity: normalizeSeverity(row.severity), + findingClass: normalizeFindingClass(row.finding_class), + body: row.body, + confidence: clampNumber(Number(row.confidence ?? 0.5), 0, 1), + evidence: safeJsonParse(row.evidence_json, []), + filePath: row.file_path, + line: typeof row.line === "number" ? row.line : null, + anchorState: (row.anchor_state as ReviewCandidateFinding["anchorState"]) ?? "missing", + evidenceScore: clampNumber(Number(row.evidence_score ?? 0), 0, 1), + lowSignal: Number(row.low_signal ?? 0) === 1, + score: Number(row.score ?? 0), + createdAt: row.created_at, + }; +} + export function createReviewService({ db, logger, @@ -1462,6 +1697,101 @@ export function createReviewService({ return record; } + function insertReviewerRun(runId: string, pass: PassDefinition): ReviewReviewerRun { + const now = nowIso(); + const record: ReviewReviewerRun = { + id: randomUUID(), + runId, + reviewerKey: pass.key, + label: pass.label, + focus: pass.focus, + status: "queued", + chatSessionId: null, + promptArtifactId: null, + outputArtifactId: null, + findingsArtifactId: null, + candidateCount: 0, + keptCount: 0, + summary: null, + errorMessage: null, + startedAt: null, + endedAt: null, + createdAt: now, + updatedAt: now, + }; + db.run( + `insert into review_reviewer_runs ( + id, + run_id, + reviewer_key, + label, + focus, + status, + chat_session_id, + prompt_artifact_id, + output_artifact_id, + findings_artifact_id, + candidate_count, + kept_count, + summary, + error_message, + started_at, + ended_at, + created_at, + updated_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + record.id, + record.runId, + record.reviewerKey, + record.label, + record.focus, + record.status, + record.chatSessionId, + record.promptArtifactId, + record.outputArtifactId, + record.findingsArtifactId, + record.candidateCount, + record.keptCount, + record.summary, + record.errorMessage, + record.startedAt, + record.endedAt, + record.createdAt, + record.updatedAt, + ], + ); + return record; + } + + function updateReviewerRun( + reviewerRunId: string, + patch: Partial<{ + status: ReviewReviewerRunStatus; + chat_session_id: string | null; + prompt_artifact_id: string | null; + output_artifact_id: string | null; + findings_artifact_id: string | null; + candidate_count: number; + kept_count: number; + summary: string | null; + error_message: string | null; + started_at: string | null; + ended_at: string | null; + updated_at: string; + }>, + ): void { + const sets: string[] = []; + const params: Array = []; + for (const [key, value] of Object.entries(patch)) { + sets.push(`${key} = ?`); + params.push(value ?? null); + } + if (sets.length === 0) return; + params.push(reviewerRunId); + db.run(`update review_reviewer_runs set ${sets.join(", ")} where id = ?`, params); + } + function insertPublication(publication: ReviewPublication): void { db.run( `insert into review_run_publications ( @@ -1542,6 +1872,49 @@ export function createReviewService({ ); } + function insertCandidateFinding(reviewerRunId: string, candidate: PassCandidateFinding): void { + db.run( + `insert or replace into review_candidate_findings ( + id, + run_id, + reviewer_run_id, + reviewer_key, + title, + severity, + finding_class, + body, + confidence, + evidence_json, + file_path, + line, + anchor_state, + evidence_score, + low_signal, + score, + created_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + candidate.id, + candidate.runId, + reviewerRunId, + candidate.passKey, + candidate.title, + candidate.severity, + candidate.findingClass, + candidate.body, + candidate.confidence, + JSON.stringify(candidate.evidence), + candidate.filePath, + candidate.line, + candidate.anchorState, + candidate.evidenceScore, + candidate.lowSignal ? 1 : 0, + candidate.score, + nowIso(), + ], + ); + } + function updateFindingPublicationState(runId: string, findingId: string, publicationState: ReviewPublicationState): void { db.run( "update review_findings set publication_state = ? where id = ? and run_id = ?", @@ -1580,6 +1953,7 @@ export function createReviewService({ dirtyOnly: partial?.dirtyOnly ?? target.mode === "working_tree", modelId: partial?.modelId?.trim() || defaultReviewModelId, reasoningEffort: partial?.reasoningEffort?.trim() || null, + codexFastMode: partial?.codexFastMode === true, budgets: normalizeBudgetConfig(partial?.budgets), publishBehavior: target.mode === "pr" && partial?.publishBehavior === "auto_publish" ? "auto_publish" @@ -1664,94 +2038,225 @@ export function createReviewService({ async function executePass(args: { runId: string; run: ReviewRun; - sessionId: string; sessionTitle: string; descriptorId: string; + provider: string; + model: string; pass: PassDefinition; - diffText: string; + manifestPrompt: string; changedFiles: MaterializedChangedFile[]; changedFilesByPath: Map }>; context: ReviewContextPacket; contextArtifactIds: ReviewContextArtifactIds; }): Promise { - const prompt = buildPassPrompt({ - run: args.run, - pass: args.pass, - diffText: args.diffText, - changedFiles: args.changedFiles, - context: args.context, - contextArtifactIds: args.contextArtifactIds, - }); - const promptArtifact = insertArtifact(args.runId, { - artifactType: "pass_prompt", - title: `${args.pass.label} prompt`, - mimeType: "text/plain", - contentText: prompt, - metadata: { - passKey: args.pass.key, - modelId: args.descriptorId, - reasoningEffort: args.run.config.reasoningEffort, - matchedRuleIds: args.context.rules.metadata.matchedRuleIds ?? [], - }, - }); - const result = await agentChatService.runSessionTurn({ - sessionId: args.sessionId, - text: prompt, - displayText: `${args.sessionTitle} · ${args.pass.label}`, - reasoningEffort: args.run.config.reasoningEffort, - timeoutMs: 15 * 60 * 1000, - }); - const outputArtifact = insertArtifact(args.runId, { - artifactType: "pass_output", - title: `${args.pass.label} output`, - mimeType: "application/json", - contentText: result.outputText, - metadata: { - passKey: args.pass.key, - provider: result.provider, - model: result.model, - modelId: result.modelId ?? args.descriptorId, - }, + const reviewerRun = insertReviewerRun(args.runId, args.pass); + const startedAt = nowIso(); + updateReviewerRun(reviewerRun.id, { + status: "running", + started_at: startedAt, + updated_at: startedAt, }); - const parsed = extractJsonObject(result.outputText); - const normalized = normalizeParsedFindings({ + emit({ + type: "reviewer-updated", runId: args.runId, - passKey: args.pass.key, - parsed, - changedFilesByPath: args.changedFilesByPath, + laneId: args.run.laneId, + reviewerRunId: reviewerRun.id, + reviewerKey: args.pass.key, + status: "running", }); - const candidates = [...normalized.findings] - .sort((left, right) => right.score - left.score) - .slice(0, args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings); - const findingsArtifact = insertArtifact(args.runId, { - artifactType: "pass_findings", - title: `${args.pass.label} findings`, - mimeType: "application/json", - contentText: JSON.stringify({ + + let sessionId: string | null = null; + let promptArtifactId: string | null = null; + let outputArtifactId: string | null = null; + let findingsArtifactId: string | null = null; + + try { + const session = await agentChatService.createSession({ + laneId: args.run.laneId, + provider: args.provider as never, + model: args.model, + modelId: args.descriptorId, + reasoningEffort: args.run.config.reasoningEffort, + codexFastMode: args.run.config.codexFastMode === true, + permissionMode: "plan", + sessionProfile: "workflow", + surface: "automation", + }); + sessionId = session.id; + sessionService.updateMeta({ + sessionId, + title: `${args.sessionTitle} · ${args.pass.label}`, + }); + updateReviewerRun(reviewerRun.id, { + chat_session_id: sessionId, + updated_at: nowIso(), + }); + db.run( + "update review_runs set chat_session_id = coalesce(chat_session_id, ?), updated_at = ? where id = ? and project_id = ?", + [sessionId, nowIso(), args.runId, projectId], + ); + + const prompt = buildPassPrompt({ + run: args.run, + pass: args.pass, + manifestPrompt: truncateText(args.manifestPrompt, args.run.config.budgets.maxPromptChars), + changedFiles: args.changedFiles, + context: args.context, + contextArtifactIds: args.contextArtifactIds, + }); + const promptArtifact = insertArtifact(args.runId, { + artifactType: "pass_prompt", + title: `${args.pass.label} prompt`, + mimeType: "text/plain", + contentText: prompt, + metadata: { + passKey: args.pass.key, + modelId: args.descriptorId, + reasoningEffort: args.run.config.reasoningEffort, + matchedRuleIds: args.context.rules.metadata.matchedRuleIds ?? [], + }, + }); + promptArtifactId = promptArtifact.id; + updateReviewerRun(reviewerRun.id, { + prompt_artifact_id: promptArtifactId, + updated_at: nowIso(), + }); + const result = await agentChatService.runSessionTurn({ + sessionId, + text: prompt, + displayText: `${args.sessionTitle} · ${args.pass.label}`, + reasoningEffort: args.run.config.reasoningEffort, + timeoutMs: 15 * 60 * 1000, + }); + const outputArtifact = insertArtifact(args.runId, { + artifactType: "pass_output", + title: `${args.pass.label} output`, + mimeType: "application/json", + contentText: result.outputText, + metadata: { + passKey: args.pass.key, + provider: result.provider, + model: result.model, + modelId: result.modelId ?? args.descriptorId, + }, + }); + outputArtifactId = outputArtifact.id; + updateReviewerRun(reviewerRun.id, { + output_artifact_id: outputArtifactId, + updated_at: nowIso(), + }); + const parsed = extractJsonObject(result.outputText); + const normalized = normalizeParsedFindings({ + runId: args.runId, + reviewerRunId: reviewerRun.id, passKey: args.pass.key, + parsed, + changedFilesByPath: args.changedFilesByPath, + }); + const candidates = [...normalized.findings] + .sort(compareCandidatesStable) + .slice(0, args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings); + for (const candidate of normalized.findings) { + insertCandidateFinding(reviewerRun.id, candidate); + } + const findingsArtifact = insertArtifact(args.runId, { + artifactType: "pass_findings", + title: `${args.pass.label} findings`, + mimeType: "application/json", + contentText: JSON.stringify({ + passKey: args.pass.key, + summary: normalized.summary, + totalParsedCount: normalized.findings.length, + keptCount: candidates.length, + budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), + candidates, + }, null, 2), + metadata: { + passKey: args.pass.key, + summary: normalized.summary, + totalParsedCount: normalized.findings.length, + keptCount: candidates.length, + budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), + }, + }); + findingsArtifactId = findingsArtifact.id; + const endedAt = nowIso(); + updateReviewerRun(reviewerRun.id, { + status: cancelledRuns.has(args.runId) ? "cancelled" : "completed", + findings_artifact_id: findingsArtifactId, + candidate_count: normalized.findings.length, + kept_count: candidates.length, summary: normalized.summary, - totalParsedCount: normalized.findings.length, - keptCount: candidates.length, - budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), - candidates, - }, null, 2), - metadata: { - passKey: args.pass.key, + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ + type: "reviewer-updated", + runId: args.runId, + laneId: args.run.laneId, + reviewerRunId: reviewerRun.id, + reviewerKey: args.pass.key, + status: cancelledRuns.has(args.runId) ? "cancelled" : "completed", + }); + return { + pass: args.pass, + reviewerRunId: reviewerRun.id, + sessionId, summary: normalized.summary, - totalParsedCount: normalized.findings.length, - keptCount: candidates.length, + candidates, + promptArtifactId: promptArtifact.id, + outputArtifactId: outputArtifact.id, + findingsArtifactId: findingsArtifact.id, budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), - }, - }); - return { - pass: args.pass, - summary: normalized.summary, - candidates, - promptArtifactId: promptArtifact.id, - outputArtifactId: outputArtifact.id, - findingsArtifactId: findingsArtifact.id, - budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), - }; + }; + } catch (error) { + const endedAt = nowIso(); + const outputArtifact = insertArtifact(args.runId, { + artifactType: "pass_output", + title: `${args.pass.label} error`, + mimeType: "application/json", + contentText: JSON.stringify({ error: getErrorMessage(error) }, null, 2), + metadata: { + passKey: args.pass.key, + modelId: args.descriptorId, + failed: true, + }, + }); + outputArtifactId = outputArtifact.id; + updateReviewerRun(reviewerRun.id, { + status: cancelledRuns.has(args.runId) ? "cancelled" : "failed", + prompt_artifact_id: promptArtifactId, + output_artifact_id: outputArtifactId, + findings_artifact_id: findingsArtifactId, + error_message: getErrorMessage(error), + ended_at: endedAt, + updated_at: endedAt, + }); + logger.warn("review.reviewer_failed", { + runId: args.runId, + reviewerKey: args.pass.key, + error: getErrorMessage(error), + }); + emit({ + type: "reviewer-updated", + runId: args.runId, + laneId: args.run.laneId, + reviewerRunId: reviewerRun.id, + reviewerKey: args.pass.key, + status: cancelledRuns.has(args.runId) ? "cancelled" : "failed", + }); + return { + pass: args.pass, + reviewerRunId: reviewerRun.id, + sessionId, + summary: null, + candidates: [], + promptArtifactId: promptArtifactId ?? "", + outputArtifactId, + findingsArtifactId: findingsArtifactId ?? "", + budgetTrimmedCount: 0, + }; + } } async function executeRun(runId: string): Promise { @@ -1795,9 +2300,13 @@ export function createReviewService({ updated_at: nowIso(), }); + let diffBundleArtifactId: string | null = null; for (const artifact of materialized.artifacts) { if (disposed) return; - insertArtifact(runId, artifact); + const inserted = insertArtifact(runId, artifact); + if (inserted.artifactType === "diff_bundle") { + diffBundleArtifactId = inserted.id; + } } if (disposed) return; @@ -1822,33 +2331,42 @@ export function createReviewService({ throw new Error(`Unknown review model '${run.config.modelId}'.`); } const { provider, model } = resolveChatProviderForDescriptor(descriptor); - const session = await agentChatService.createSession({ - laneId: run.laneId, - provider, - model, - modelId: descriptor.id, - reasoningEffort: run.config.reasoningEffort, - permissionMode: "plan", - sessionProfile: "workflow", - surface: "automation", - }); - if (disposed) return; const sessionTitle = `Review: ${materialized.targetLabel}`; - sessionService.updateMeta({ - sessionId: session.id, - title: sessionTitle, - }); - updateRun(runId, { - chat_session_id: session.id, - updated_at: nowIso(), - }); const effectiveRun: ReviewRun = { ...run, targetLabel: materialized.targetLabel, compareTarget: materialized.compareTarget, }; - const diffText = truncateText(materialized.fullPatchText, effectiveRun.config.budgets.maxDiffChars); const changedFiles = materialized.changedFiles.slice(0, effectiveRun.config.budgets.maxFiles); + const manifestPayload = buildChangedFileManifestPayload({ + targetLabel: materialized.targetLabel, + compareTarget: materialized.compareTarget, + changedFiles, + budgets: effectiveRun.config.budgets, + }); + const riskMapPayload = buildRiskMapPayload(changedFiles); + const manifestArtifact = insertArtifact(runId, { + artifactType: "changed_file_manifest", + title: "Changed-file manifest", + mimeType: "application/json", + contentText: JSON.stringify(manifestPayload, null, 2), + metadata: { + fileCount: changedFiles.length, + totalFileCount: materialized.changedFiles.length, + budgetMode: effectiveRun.config.budgets.unlimited === true ? "unlimited" : "bounded", + }, + }); + const riskMapArtifact = insertArtifact(runId, { + artifactType: "risk_map", + title: "Review risk map", + mimeType: "application/json", + contentText: JSON.stringify(riskMapPayload, null, 2), + metadata: { + fileCount: changedFiles.length, + riskGroupCount: Array.isArray(riskMapPayload.groups) ? riskMapPayload.groups.length : 0, + }, + }); + const manifestPrompt = buildManifestPrompt(manifestPayload, riskMapPayload); const reviewContext = await contextBuilder.buildContext({ run: effectiveRun, materialized: { @@ -1878,16 +2396,20 @@ export function createReviewService({ metadata: reviewContext.validation.metadata, }); const contextArtifactIds: ReviewContextArtifactIds = { + manifestArtifactId: manifestArtifact.id, + riskMapArtifactId: riskMapArtifact.id, + diffBundleArtifactId, provenanceArtifactId: provenanceArtifact.id, rulesArtifactId: rulesArtifact.id, validationArtifactId: validationArtifact.id, }; insertArtifact(runId, { artifactType: "prompt", - title: "Review harness plan", + title: "Review orchestration plan", mimeType: "application/json", contentText: JSON.stringify({ targetLabel: materialized.targetLabel, + architecture: "parallel_specialist_reviewers", passKeys: REVIEW_PASSES.map((pass) => pass.key), budgets: effectiveRun.config.budgets, changedFiles: changedFiles.map((entry) => entry.filePath), @@ -1910,45 +2432,48 @@ export function createReviewService({ }, }); - const changedFilesByPath = new Map(changedFiles.map((entry) => [ + const changedFilesByPath = new Map }>(changedFiles.map((entry) => [ entry.filePath, { excerpt: entry.excerpt, lineNumbers: new Set(entry.lineNumbers), - diffPositionsByLine: entry.diffPositionsByLine, }, ])); - const passResults: PassExecutionResult[] = []; - for (const pass of REVIEW_PASSES) { - if (disposed) return; - if (cancelledRuns.has(runId)) { - cancelledRuns.delete(runId); - const endedAt = nowIso(); - updateRun(runId, { - status: "cancelled", - summary: "Run cancelled during review passes.", - error_message: null, - ended_at: endedAt, - updated_at: endedAt, - }); - emit({ type: "run-completed", runId, laneId: run.laneId, status: "cancelled" }); - emit({ type: "runs-updated", runId, laneId: run.laneId, status: "cancelled" }); - return; - } - const passResult = await executePass({ + const passResults = await Promise.all(REVIEW_PASSES.map((pass) => executePass({ runId, run: effectiveRun, - sessionId: session.id, sessionTitle, descriptorId: descriptor.id, + provider, + model, pass, - diffText, + manifestPrompt, changedFiles, changedFilesByPath, context: reviewContext, contextArtifactIds, + }))); + const firstSessionId = passResults.find((result) => result.sessionId)?.sessionId ?? null; + if (firstSessionId) { + updateRun(runId, { + chat_session_id: firstSessionId, + updated_at: nowIso(), }); - passResults.push(passResult); + } + if (disposed) return; + if (cancelledRuns.has(runId)) { + cancelledRuns.delete(runId); + const endedAt = nowIso(); + updateRun(runId, { + status: "cancelled", + summary: "Run cancelled during review passes.", + error_message: null, + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ type: "run-completed", runId, laneId: run.laneId, status: "cancelled" }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "cancelled" }); + return; } if (disposed) return; @@ -2238,6 +2763,18 @@ export function createReviewService({ "select * from review_run_artifacts where run_id = ? order by created_at asc", [args.runId], ).map(mapArtifactRow); + const reviewerRuns = db.all( + `select * from review_reviewer_runs + where run_id = ? + order by created_at asc, reviewer_key asc`, + [args.runId], + ).map(mapReviewerRunRow); + const candidateFindings = db.all( + `select * from review_candidate_findings + where run_id = ? + order by score desc, created_at asc, title asc`, + [args.runId], + ).map(mapCandidateFindingRow); const publications = db.all( "select * from review_run_publications where run_id = ? order by created_at asc", [args.runId], @@ -2249,6 +2786,8 @@ export function createReviewService({ ...run, findings, artifacts, + reviewerRuns, + candidateFindings, publications, chatSession, }; diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index 7a44998a6..f5c167a35 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -1404,6 +1404,40 @@ function migrate(db: MigrationDb) { try { db.run("alter table pull_requests add column head_sha text"); } catch {} try { db.run("alter table pull_requests add column creation_strategy text"); } catch {} + db.run(` + create table if not exists github_pr_cache ( + project_id text not null, + repo_owner text not null, + repo_name text not null, + github_pr_number integer not null, + item_json text not null, + state text not null, + head_branch text, + updated_at text not null, + synced_at text not null, + primary key(project_id, repo_owner, repo_name, github_pr_number), + foreign key(project_id) references projects(id) + ) + `); + db.run("create index if not exists idx_github_pr_cache_project_state on github_pr_cache(project_id, state, updated_at)"); + db.run("create index if not exists idx_github_pr_cache_project_repo on github_pr_cache(project_id, repo_owner, repo_name)"); + + db.run(` + create table if not exists pr_auto_link_ignores ( + project_id text not null, + repo_owner text not null, + repo_name text not null, + github_pr_number integer not null, + lane_id text not null, + head_branch text, + created_at text not null, + primary key(project_id, repo_owner, repo_name, github_pr_number, lane_id), + foreign key(project_id) references projects(id), + foreign key(lane_id) references lanes(id) + ) + `); + db.run("create index if not exists idx_pr_auto_link_ignores_project_repo on pr_auto_link_ignores(project_id, repo_owner, repo_name)"); + // Phase 21: AI PR summary cache (keyed by PR + headSha so pushes invalidate). db.run(` create table if not exists pull_request_ai_summaries ( @@ -3542,6 +3576,58 @@ function migrate(db: MigrationDb) { ) `); db.run("create index if not exists idx_review_run_artifacts_run on review_run_artifacts(run_id, created_at)"); + + db.run(` + create table if not exists review_reviewer_runs ( + id text primary key, + run_id text not null, + reviewer_key text not null, + label text not null, + focus text not null, + status text not null, + chat_session_id text, + prompt_artifact_id text, + output_artifact_id text, + findings_artifact_id text, + candidate_count integer not null default 0, + kept_count integer not null default 0, + summary text, + error_message text, + started_at text, + ended_at text, + created_at text not null, + updated_at text not null, + foreign key(run_id) references review_runs(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_reviewer_runs_run on review_reviewer_runs(run_id, created_at)"); + db.run("create index if not exists idx_review_reviewer_runs_run_key on review_reviewer_runs(run_id, reviewer_key)"); + + db.run(` + create table if not exists review_candidate_findings ( + id text primary key, + run_id text not null, + reviewer_run_id text not null, + reviewer_key text not null, + title text not null, + severity text not null, + finding_class text, + body text not null, + confidence real not null default 0.5, + evidence_json text, + file_path text, + line integer, + anchor_state text not null, + evidence_score real not null default 0, + low_signal integer not null default 0, + score real not null default 0, + created_at text not null, + foreign key(run_id) references review_runs(id) on delete cascade, + foreign key(reviewer_run_id) references review_reviewer_runs(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_candidate_findings_run on review_candidate_findings(run_id)"); + db.run("create index if not exists idx_review_candidate_findings_reviewer on review_candidate_findings(reviewer_run_id)"); try { db.run("alter table review_findings add column finding_class text"); } catch {} try { db.run("alter table review_findings add column originating_passes_json text"); } catch {} try { db.run("alter table review_findings add column adjudication_json text"); } catch {} diff --git a/apps/desktop/src/preload/global.d.ts b/apps/desktop/src/preload/global.d.ts index 3f43f17dc..2e622f993 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -294,6 +294,9 @@ import type { GitSyncArgs, GitHubRepoRef, GitHubStatus, + CreateLaneFromPrBranchArgs, + CreateLaneFromPrBranchPreflightResult, + CreateLaneFromPrBranchResult, CreatePrFromLaneArgs, CreateQueuePrsArgs, CreateQueuePrsResult, @@ -1970,6 +1973,12 @@ declare global { prs: { createFromLane: (args: CreatePrFromLaneArgs) => Promise; linkToLane: (args: LinkPrToLaneArgs) => Promise; + preflightCreateLaneFromPrBranch: ( + args: CreateLaneFromPrBranchArgs, + ) => Promise; + createLaneFromPrBranch: ( + args: CreateLaneFromPrBranchArgs, + ) => Promise; getForLane: (laneId: string) => Promise; listAll: () => Promise; listOpenForRepo: () => Promise; diff --git a/apps/desktop/src/preload/preload.test.ts b/apps/desktop/src/preload/preload.test.ts index ef71543ca..67a769fa9 100644 --- a/apps/desktop/src/preload/preload.test.ts +++ b/apps/desktop/src/preload/preload.test.ts @@ -174,6 +174,146 @@ describe("preload OAuth bridge", () => { expect(removeListener).toHaveBeenCalledWith(IPC.reviewEvent, listener); }); + it("routes review.startRun through a bound local runtime without dropping unlimited budgets", async () => { + const binding = { + kind: "local", + key: "local:/repo", + rootPath: "/repo", + displayName: "Project", + }; + const startArgs = { + target: { mode: "lane_diff", laneId: "lane-1" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: "openai/gpt-5.4", + reasoningEffort: "medium", + budgets: { + unlimited: true, + maxFiles: Number.MAX_SAFE_INTEGER, + maxDiffChars: Number.MAX_SAFE_INTEGER, + maxPromptChars: Number.MAX_SAFE_INTEGER, + maxFindings: Number.MAX_SAFE_INTEGER, + maxFindingsPerPass: Number.MAX_SAFE_INTEGER, + maxPublishedFindings: Number.MAX_SAFE_INTEGER, + }, + publishBehavior: "local_only", + }, + }; + const run = { + id: "review-run-1", + projectId: "project-1", + laneId: "lane-1", + target: startArgs.target, + config: startArgs.config, + targetLabel: "Lane 1", + compareTarget: null, + status: "queued", + summary: null, + errorMessage: null, + findingCount: 0, + severitySummary: {}, + chatSessionId: null, + createdAt: "2026-05-19T12:00:00.000Z", + startedAt: "2026-05-19T12:00:00.000Z", + endedAt: null, + updatedAt: "2026-05-19T12:00:00.000Z", + }; + const invoke = vi.fn(async (channel: string, arg?: unknown) => { + if (channel === IPC.appGetWindowSession) { + return { windowId: 1, project: { rootPath: "/repo", displayName: "Project" }, binding }; + } + if (channel === IPC.localRuntimeCallAction) { + const request = (arg as { request?: { domain?: string; action?: string; args?: unknown } } | undefined)?.request; + expect(request?.domain).toBe("review"); + expect(request?.action).toBe("startRun"); + expect(request?.args).toEqual(startArgs); + return { result: run }; + } + if (channel === IPC.reviewStartRun) { + throw new Error("runtime-bound review.startRun should not call desktop review IPC"); + } + throw new Error(`unexpected IPC: ${channel} ${JSON.stringify(arg)}`); + }); + const on = vi.fn(); + const removeListener = vi.fn(); + const exposeInMainWorld = vi.fn((name: string, value: unknown) => { + (globalThis as any).__bridgeName = name; + (globalThis as any).__adeBridge = value; + }); + + vi.doMock("electron", () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener }, + webFrame: { + getZoomLevel: vi.fn(() => 0), + setZoomLevel: vi.fn(), + getZoomFactor: vi.fn(() => 1), + }, + })); + + await import("./preload"); + + const bridge = (globalThis as any).__adeBridge; + await expect(bridge.review.startRun(startArgs)).resolves.toEqual(run); + + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "review", action: "startRun", args: startArgs }, + }); + expect(invoke).not.toHaveBeenCalledWith(IPC.reviewStartRun, expect.anything()); + }); + + it("does not fall through to in-process review IPC when a bound local runtime cannot call review.startRun", async () => { + const binding = { + kind: "local", + key: "local:/repo", + rootPath: "/repo", + displayName: "Project", + }; + const invoke = vi.fn(async (channel: string, arg?: unknown) => { + if (channel === IPC.appGetWindowSession) { + return { windowId: 1, project: { rootPath: "/repo", displayName: "Project" }, binding }; + } + if (channel === IPC.localRuntimeCallAction) { + const request = (arg as { request?: { domain?: string; action?: string } } | undefined)?.request; + throw new Error(`Action '${request?.domain}.${request?.action}' is not callable.`); + } + if (channel === IPC.reviewStartRun) { + throw new Error("runtime-bound review.startRun should not call desktop review IPC"); + } + throw new Error(`unexpected IPC: ${channel} ${JSON.stringify(arg)}`); + }); + const on = vi.fn(); + const removeListener = vi.fn(); + const exposeInMainWorld = vi.fn((name: string, value: unknown) => { + (globalThis as any).__bridgeName = name; + (globalThis as any).__adeBridge = value; + }); + + vi.doMock("electron", () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener }, + webFrame: { + getZoomLevel: vi.fn(() => 0), + setZoomLevel: vi.fn(), + getZoomFactor: vi.fn(() => 1), + }, + })); + + await import("./preload"); + + const bridge = (globalThis as any).__adeBridge; + const args = { target: { mode: "lane_diff", laneId: "lane-1" } }; + + await expect(bridge.review.startRun(args)).rejects.toThrow("not callable"); + + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "review", action: "startRun", args }, + }); + expect(invoke).not.toHaveBeenCalledWith(IPC.reviewStartRun, expect.anything()); + }); + it("exposes macOS VM IPC methods and cleans up listeners", async () => { const invoke = vi.fn(async () => undefined); const on = vi.fn(); @@ -580,7 +720,7 @@ describe("preload OAuth bridge", () => { expect(invoke).toHaveBeenCalledWith(IPC.agentChatModelCatalog, { mode: "cached" }); }); - it("uses in-process IPC for local PR tab reads instead of waiting on the runtime daemon", async () => { + it("routes local PR tab reads through the project runtime", async () => { const binding = { kind: "local", key: "local:/repo", @@ -634,11 +774,15 @@ describe("preload OAuth bridge", () => { return { windowId: 1, project: { rootPath: "/repo", displayName: "Project" }, binding }; } if (channel === IPC.localRuntimeCallAction) { - throw new Error("PR tab reads should not call the local runtime daemon"); + const request = (arg as { request?: { action?: string } } | undefined)?.request; + if (request?.action === "getDetail") return { result: detail }; + if (request?.action === "listWithConflicts") return { result: prs }; + if (request?.action === "getGithubSnapshot") return { result: snapshot }; + throw new Error(`unexpected local PR action: ${request?.action}`); + } + if (channel === IPC.prsGetDetail || channel === IPC.prsListWithConflicts || channel === IPC.prsGetGitHubSnapshot) { + throw new Error("local runtime PR reads should not call desktop PR IPC"); } - if (channel === IPC.prsGetDetail) return detail; - if (channel === IPC.prsListWithConflicts) return prs; - if (channel === IPC.prsGetGitHubSnapshot) return snapshot; throw new Error(`unexpected IPC: ${channel} ${JSON.stringify(arg)}`); }); const on = vi.fn(); @@ -665,13 +809,143 @@ describe("preload OAuth bridge", () => { await expect(bridge.prs.listWithConflicts()).resolves.toEqual(prs); await expect(bridge.prs.getGitHubSnapshot()).resolves.toEqual(snapshot); - expect(invoke).toHaveBeenCalledWith(IPC.prsGetDetail, { prId: "pr-1" }); - expect(invoke).toHaveBeenCalledWith(IPC.prsListWithConflicts, {}); - expect(invoke).toHaveBeenCalledWith(IPC.prsGetGitHubSnapshot, {}); - expect(invoke).not.toHaveBeenCalledWith( - IPC.localRuntimeCallAction, - expect.anything(), - ); + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "pr", action: "getDetail", arg: "pr-1" }, + }); + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "pr", action: "listWithConflicts", args: {} }, + }); + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "pr", action: "getGithubSnapshot", args: {} }, + }); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsGetDetail, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsListWithConflicts, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsGetGitHubSnapshot, expect.anything()); + }); + + it("does not fall through to in-process PR branch import IPC when a bound local runtime action is missing", async () => { + const binding = { + kind: "local", + key: "local:/repo", + rootPath: "/repo", + displayName: "Project", + }; + const invoke = vi.fn(async (channel: string, arg?: unknown) => { + if (channel === IPC.appGetWindowSession) { + return { windowId: 1, project: { rootPath: "/repo", displayName: "Project" }, binding }; + } + if (channel === IPC.localRuntimeCallAction) { + const request = (arg as { request?: { domain?: string; action?: string } } | undefined)?.request; + throw new Error(`Action '${request?.domain}.${request?.action}' is not callable.`); + } + if (channel === IPC.prsPreflightCreateLaneFromPrBranch || channel === IPC.prsCreateLaneFromPrBranch) { + throw new Error("runtime-bound PR branch import should not call desktop PR IPC"); + } + throw new Error(`unexpected IPC: ${channel} ${JSON.stringify(arg)}`); + }); + const on = vi.fn(); + const removeListener = vi.fn(); + const exposeInMainWorld = vi.fn((name: string, value: unknown) => { + (globalThis as any).__bridgeName = name; + (globalThis as any).__adeBridge = value; + }); + + vi.doMock("electron", () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener }, + webFrame: { + getZoomLevel: vi.fn(() => 0), + setZoomLevel: vi.fn(), + getZoomFactor: vi.fn(() => 1), + }, + })); + + await import("./preload"); + + const bridge = (globalThis as any).__adeBridge; + const args = { repoOwner: "owner", repoName: "repo", githubPrNumber: 12 }; + + await expect(bridge.prs.preflightCreateLaneFromPrBranch(args)).rejects.toThrow("not callable"); + await expect(bridge.prs.createLaneFromPrBranch(args)).rejects.toThrow("not callable"); + + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "pr", action: "preflightCreateLaneFromPrBranch", args }, + }); + expect(invoke).toHaveBeenCalledWith(IPC.localRuntimeCallAction, { + request: { domain: "pr", action: "createLaneFromPrBranch", args }, + }); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsPreflightCreateLaneFromPrBranch, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsCreateLaneFromPrBranch, expect.anything()); + }); + + it("falls back to in-process PR branch import IPC when no runtime is bound", async () => { + const args = { repoOwner: "owner", repoName: "repo", githubPrNumber: 12 }; + const preflightResult = { + preflight: { + repoOwner: "owner", + repoName: "repo", + githubPrNumber: 12, + githubUrl: "https://github.com/owner/repo/pull/12", + title: "Import branch", + headBranch: "feature/import", + headSha: "abc123", + headRepoOwner: "owner", + headRepoName: "repo", + remoteBranch: "origin/feature/import", + importBranchRef: "origin/feature/import", + targetLaneName: "Import branch", + baseBranch: "main", + canCreate: true, + status: "ready", + blockingConflict: null, + blockingConflicts: [], + }, + lane: null, + pr: null, + }; + const createResult = { + ...preflightResult, + lane: { id: "lane-created" }, + pr: { id: "pr-created" }, + }; + const invoke = vi.fn(async (channel: string, arg?: unknown) => { + if (channel === IPC.appGetWindowSession) { + return { windowId: 1, project: { rootPath: "/repo", displayName: "Project" }, binding: null }; + } + if (channel === IPC.prsPreflightCreateLaneFromPrBranch) return preflightResult; + if (channel === IPC.prsCreateLaneFromPrBranch) return createResult; + if (channel === IPC.localRuntimeCallAction || channel === IPC.remoteRuntimeCallAction) { + throw new Error("unbound project should not call runtime IPC"); + } + throw new Error(`unexpected IPC: ${channel} ${JSON.stringify(arg)}`); + }); + const on = vi.fn(); + const removeListener = vi.fn(); + const exposeInMainWorld = vi.fn((name: string, value: unknown) => { + (globalThis as any).__bridgeName = name; + (globalThis as any).__adeBridge = value; + }); + + vi.doMock("electron", () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener }, + webFrame: { + getZoomLevel: vi.fn(() => 0), + setZoomLevel: vi.fn(), + getZoomFactor: vi.fn(() => 1), + }, + })); + + await import("./preload"); + + const bridge = (globalThis as any).__adeBridge; + await expect(bridge.prs.preflightCreateLaneFromPrBranch(args)).resolves.toEqual(preflightResult); + await expect(bridge.prs.createLaneFromPrBranch(args)).resolves.toEqual(createResult); + + expect(invoke).toHaveBeenCalledWith(IPC.prsPreflightCreateLaneFromPrBranch, args); + expect(invoke).toHaveBeenCalledWith(IPC.prsCreateLaneFromPrBranch, args); + expect(invoke).not.toHaveBeenCalledWith(IPC.localRuntimeCallAction, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.remoteRuntimeCallAction, expect.anything()); }); it("keeps remote runtime routing for PR tab reads when the project is remote", async () => { @@ -1789,6 +2063,16 @@ describe("preload OAuth bridge", () => { await bridge.git.commit({ laneId: "lane-1", message: "checkpoint" }); await bridge.git.push({ laneId: "lane-1" }); await bridge.prs.createFromLane({ laneId: "lane-1", title: "Remote PR", body: "Proof" }); + await bridge.prs.preflightCreateLaneFromPrBranch({ + repoOwner: "owner", + repoName: "repo", + githubPrNumber: 12, + }); + await bridge.prs.createLaneFromPrBranch({ + repoOwner: "owner", + repoName: "repo", + githubPrNumber: 12, + }); const actions = invoke.mock.calls .filter(([channel]) => channel === IPC.remoteRuntimeCallAction) @@ -1800,12 +2084,16 @@ describe("preload OAuth bridge", () => { "git.commit", "git.push", "pr.createFromLane", + "pr.preflightCreateLaneFromPrBranch", + "pr.createLaneFromPrBranch", ]); expect(invoke).not.toHaveBeenCalledWith(IPC.lanesCreate, expect.anything()); expect(invoke).not.toHaveBeenCalledWith(IPC.agentChatCreate, expect.anything()); expect(invoke).not.toHaveBeenCalledWith(IPC.gitCommit, expect.anything()); expect(invoke).not.toHaveBeenCalledWith(IPC.gitPush, expect.anything()); expect(invoke).not.toHaveBeenCalledWith(IPC.prsCreateFromLane, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsPreflightCreateLaneFromPrBranch, expect.anything()); + expect(invoke).not.toHaveBeenCalledWith(IPC.prsCreateLaneFromPrBranch, expect.anything()); }); it("routes bulk PR merge context hydration with positional runtime args", async () => { diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index e4a85c98b..d06543cee 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -209,6 +209,9 @@ import type { GitSyncArgs, GitHubRepoRef, GitHubStatus, + CreateLaneFromPrBranchArgs, + CreateLaneFromPrBranchPreflightResult, + CreateLaneFromPrBranchResult, CreatePrFromLaneArgs, DeletePrArgs, DeletePrResult, @@ -1291,6 +1294,18 @@ async function callProjectRuntimeActionOr( return runtime.handled ? runtime.result : local(); } +async function callProjectRuntimeActionStrictOr( + domain: string, + action: string, + request: Omit, + local: () => Promise, +): Promise { + const remote = await callRemoteProjectActionIfBound(domain, action, request); + if (remote.handled) return remote.result; + const localRuntime = await callLocalProjectActionStrictIfBound(domain, action, request); + return localRuntime.handled ? localRuntime.result : local(); +} + async function callRemoteProjectRuntimeActionOr( domain: string, action: string, @@ -1310,7 +1325,7 @@ function callPrReadRuntimeActionOr( request: Omit, local: () => Promise, ): Promise { - return callRemoteProjectRuntimeActionOr("pr", action, request, local); + return callProjectRuntimeActionOr("pr", action, request, local); } async function callProjectFileRuntimeActionOr( @@ -3570,7 +3585,7 @@ contextBridge.exposeInMainWorld("ade", { () => ipcRenderer.invoke(IPC.reviewGetRunDetail, { runId }), ), startRun: async (args: ReviewStartRunArgs): Promise => - callProjectRuntimeActionOr("review", "startRun", { args }, () => + callProjectRuntimeActionStrictOr("review", "startRun", { args }, () => ipcRenderer.invoke(IPC.reviewStartRun, args), ), rerun: async (runId: string): Promise => @@ -7034,6 +7049,18 @@ contextBridge.exposeInMainWorld("ade", { callProjectRuntimeActionOr("pr", "linkToLane", { args }, () => ipcRenderer.invoke(IPC.prsLinkToLane, args), ), + preflightCreateLaneFromPrBranch: async ( + args: CreateLaneFromPrBranchArgs, + ): Promise => + callProjectRuntimeActionStrictOr("pr", "preflightCreateLaneFromPrBranch", { args }, () => + ipcRenderer.invoke(IPC.prsPreflightCreateLaneFromPrBranch, args), + ), + createLaneFromPrBranch: async ( + args: CreateLaneFromPrBranchArgs, + ): Promise => + callProjectRuntimeActionStrictOr("pr", "createLaneFromPrBranch", { args }, () => + ipcRenderer.invoke(IPC.prsCreateLaneFromPrBranch, args), + ), getForLane: async (laneId: string): Promise => callPrReadRuntimeActionOr("getForLane", { arg: laneId }, () => ipcRenderer.invoke(IPC.prsGetForLane, { laneId }), diff --git a/apps/desktop/src/renderer/components/app/AppShell.tsx b/apps/desktop/src/renderer/components/app/AppShell.tsx index 639fc532e..5efccb00d 100644 --- a/apps/desktop/src/renderer/components/app/AppShell.tsx +++ b/apps/desktop/src/renderer/components/app/AppShell.tsx @@ -1380,7 +1380,7 @@ export function AppShell({ children }: { children: React.ReactNode }) { toast.event.kind === "changes_requested" || toast.event.kind === "review_requested" ) { - detailTab = "activity"; + detailTab = "overview"; } const search = buildPrsRouteSearch({ activeTab: "normal", diff --git a/apps/desktop/src/renderer/components/automations/adeActionSchemas.ts b/apps/desktop/src/renderer/components/automations/adeActionSchemas.ts index a60f66bce..8b7aac26e 100644 --- a/apps/desktop/src/renderer/components/automations/adeActionSchemas.ts +++ b/apps/desktop/src/renderer/components/automations/adeActionSchemas.ts @@ -431,28 +431,28 @@ export const ADE_ACTION_SCHEMAS: readonly AdeActionSchema[] = [ domain: "git", action: "stashApply", label: "Stash apply", - description: "Apply a stash entry without removing it from the stash list.", + description: "Apply a branch stash entry without removing it from the stash list.", params: [LANE_ID_PARAM, { name: "stashRef", type: "string", required: true, description: "Stash ref e.g. stash@{0}." }], }, { domain: "git", action: "stashPop", label: "Stash pop", - description: "Apply a stash entry and remove it from the stash list.", + description: "Apply a branch stash entry and remove it from the stash list.", params: [LANE_ID_PARAM, { name: "stashRef", type: "string", required: true }], }, { domain: "git", action: "stashDrop", label: "Stash drop", - description: "Discard a single stash entry without applying it.", + description: "Discard a single branch stash entry without applying it.", params: [LANE_ID_PARAM, { name: "stashRef", type: "string", required: true }], }, { domain: "git", action: "stashClear", label: "Stash clear", - description: "Discard every stash entry on the lane.", + description: "Discard every stash entry saved for the lane branch.", params: [LANE_ID_PARAM], }, { diff --git a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx index a5cb762d8..c365c30b1 100644 --- a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx +++ b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx @@ -110,6 +110,7 @@ import { ConflictPanel as GraphConflictPanel } from "./graphDialogs/ConflictPane import { RiskEdge } from "./graphEdges/RiskEdge"; import { ConfirmDialog, useConfirmDialog } from "../shared/InlineDialogs"; import { PrDetailPane } from "../prs/detail/PrDetailPane"; +import { PrsProvider } from "../prs/state/PrsContext"; import { buildGraphPrOverlay } from "./graphPrData"; import { getPrChecksBadge, getPrReviewsBadge, InlinePrBadge } from "../prs/shared/prVisuals"; @@ -118,12 +119,45 @@ const edgeTypes = { custom: RiskEdge }; const MERGE_SUCCESS_ANIMATION_MS = 1200; const GRAPH_ACTIVITY_SESSION_LIMIT = 150; const GRAPH_ACTIVITY_OPERATION_LIMIT = 150; +const GRAPH_TOPOLOGY_CACHE_TTL_MS = 30_000; +const GRAPH_PR_CACHE_TTL_MS = 5 * 60_000; + +const graphTopologyRefreshedAtByProject = new Map(); +const graphPrCacheByProject = new Map(); + +function isGraphTopologyCacheFresh(projectRoot: string | null): boolean { + if (!projectRoot) return false; + const refreshedAt = graphTopologyRefreshedAtByProject.get(projectRoot) ?? 0; + return refreshedAt > 0 && Date.now() - refreshedAt < GRAPH_TOPOLOGY_CACHE_TTL_MS; +} + +function markGraphTopologyCacheFresh(projectRoot: string | null): void { + if (!projectRoot) return; + graphTopologyRefreshedAtByProject.set(projectRoot, Date.now()); +} + +function readGraphPrCache(projectRoot: string | null): PrWithConflicts[] { + if (!projectRoot) return []; + const cached = graphPrCacheByProject.get(projectRoot); + if (!cached) return []; + if (Date.now() - cached.cachedAt > GRAPH_PR_CACHE_TTL_MS) { + graphPrCacheByProject.delete(projectRoot); + return []; + } + return cached.prs; +} + +function writeGraphPrCache(projectRoot: string | null, prs: PrWithConflicts[]): void { + if (!projectRoot) return; + graphPrCacheByProject.set(projectRoot, { prs, cachedAt: Date.now() }); +} function GraphInner({ active = true }: { active?: boolean }) { const navigate = useNavigate(); const [searchParams, setSearchParams] = useSearchParams(); const reactFlow = useReactFlow, Edge>(); const project = useAppStore((s) => s.project); + const projectRoot = project?.rootPath ?? null; const isRemoteProject = useAppStore((s) => s.projectBinding?.kind === "remote"); const lanes = useAppStore((s) => s.lanes); const lanesKey = React.useMemo(() => lanes.map((l) => l.id).join(","), [lanes]); @@ -138,7 +172,7 @@ function GraphInner({ active = true }: { active?: boolean }) { [refreshLanes] ); const [environmentMappings, setEnvironmentMappings] = React.useState([]); - const [prs, setPrs] = React.useState([]); + const [prs, setPrs] = React.useState(() => readGraphPrCache(projectRoot)); const [syncByLaneId, setSyncByLaneId] = React.useState>({}); const [autoRebaseByLaneId, setAutoRebaseByLaneId] = React.useState>({}); const syncRefreshInFlightRef = React.useRef(false); @@ -155,11 +189,17 @@ function GraphInner({ active = true }: { active?: boolean }) { const nodesRef = React.useRef>>([]); const handledFocusLaneRef = React.useRef(null); const handledFocusProposalRef = React.useRef(null); + const projectRootRef = React.useRef(projectRoot); React.useEffect(() => { lanesRef.current = lanes; }, [lanes]); + React.useEffect(() => { + projectRootRef.current = projectRoot; + setPrs(readGraphPrCache(projectRoot)); + }, [projectRoot]); + const reportGraphIssue = React.useCallback((message: string, error?: unknown) => { if (error) console.warn(`[Graph] ${message}`, error); setErrorBanner((prev) => prev ?? message); @@ -175,7 +215,10 @@ function GraphInner({ active = true }: { active?: boolean }) { }, []); const refreshPrs = React.useCallback(async () => { - const next = await window.ade.prs.listWithConflicts(); + const requestProjectRoot = projectRootRef.current; + const next = await window.ade.prs.listWithConflicts({ includeConflictAnalysis: false }); + if (requestProjectRoot !== projectRootRef.current) return; + writeGraphPrCache(requestProjectRoot, next); setPrs(next); }, []); @@ -269,7 +312,7 @@ function GraphInner({ active = true }: { active?: boolean }) { }, [nodes]); const [batch, setBatch] = React.useState(null); const [batchProgress, setBatchProgress] = React.useState(null); - const [loadingTopology, setLoadingTopology] = React.useState(true); + const [loadingTopology, setLoadingTopology] = React.useState(() => lanes.length === 0); const [loadingRisk, setLoadingRisk] = React.useState(true); const [errorBanner, setErrorBanner] = React.useState(null); const [contextMenu, setContextMenu] = React.useState<{ laneId: string; x: number; y: number } | null>(null); @@ -772,26 +815,35 @@ function GraphInner({ active = true }: { active?: boolean }) { React.useEffect(() => { if (!active) return; if (!project?.rootPath) return; + const rootPath = project.rootPath; let cancelled = false; let riskTimer: number | null = null; let activityTimer: number | null = null; let syncTimer: number | null = null; let autoRebaseTimer: number | null = null; - setLoadingTopology(true); + const hasCachedTopology = lanesRef.current.length > 0; + setLoadingTopology(!hasCachedTopology); setLoadedGraphPreferences(false); setSessionState(createSessionState()); setViewMode("all"); setSelectedLaneIds([]); setShowFiltersPanel(false); - void refreshGraphLanes() - .catch((err) => { - console.warn("[Graph] refreshLanes failed:", err); - reportGraphIssue("The graph could not load the latest lanes.", err); - }) - .finally(() => { - if (!cancelled) setLoadingTopology(false); - }); + if (hasCachedTopology && isGraphTopologyCacheFresh(rootPath)) { + setLoadingTopology(false); + } else { + void refreshGraphLanes() + .then(() => { + markGraphTopologyCacheFresh(rootPath); + }) + .catch((err) => { + console.warn("[Graph] refreshLanes failed:", err); + if (!hasCachedTopology) reportGraphIssue("The graph could not load the latest lanes.", err); + }) + .finally(() => { + if (!cancelled) setLoadingTopology(false); + }); + } riskTimer = window.setTimeout(() => { if (cancelled || document.visibilityState !== "visible") return; @@ -1571,6 +1623,7 @@ function GraphInner({ active = true }: { active?: boolean }) { }; })); }, [ + active, baseGraph, connectedToHoveredNode, focusLaneId, @@ -2023,7 +2076,8 @@ function GraphInner({ active = true }: { active?: boolean }) { await refreshPrs(); return; } - const refreshedPrs = await window.ade.prs.listWithConflicts(); + const refreshedPrs = await window.ade.prs.listWithConflicts({ includeConflictAnalysis: false }); + writeGraphPrCache(projectRootRef.current, refreshedPrs); setPrs(refreshedPrs); const refreshed = refreshedPrs.find((entry) => entry.id === prDialog.existingPr?.id) ?? null; if (!refreshed) { @@ -3898,7 +3952,8 @@ function GraphInner({ active = true }: { active?: boolean }) { baseBranch: prDialog.baseBranch }) .then(async (created) => { - const refreshed = await window.ade.prs.listWithConflicts(); + const refreshed = await window.ade.prs.listWithConflicts({ includeConflictAnalysis: false }); + writeGraphPrCache(projectRootRef.current, refreshed); setPrs(refreshed); const createdPr = refreshed.find((entry) => entry.id === created.id) ?? null; const [status, checks, reviews, comments] = await Promise.all([ @@ -3968,19 +4023,21 @@ function GraphInner({ active = true }: { active?: boolean }) {
- navigate(path)} - onShowInGraph={(laneId) => navigate(`/graph?focusLane=${encodeURIComponent(laneId)}`)} - /> + + navigate(path)} + onShowInGraph={(laneId) => navigate(`/graph?focusLane=${encodeURIComponent(laneId)}`)} + /> +
) : null} diff --git a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx index eb39d6090..11d72f3b3 100644 --- a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx +++ b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx @@ -2528,7 +2528,7 @@ export function LaneGitActionsPane({ >
- STASHES + BRANCH STASHES {stashes.length === 0 ? "None saved" : `${stashes.length} saved`} @@ -2536,10 +2536,10 @@ export function LaneGitActionsPane({
{stashes.length > 0 && ( ", + effect: `Delete ${stashes.length} branch stash${stashes.length === 1 ? "" : "es"}`, warning: "This cannot be undone", }}> ) : null} - {/* Primary: house icon; Non-primary: conflict status dot */} + {/* Primary lane icon */} {isPrimary ? ( - ) : ( - - )} - {/* Terminal attention state */} - {!isDeleting && (laneRuntime.bucket === "running" || laneRuntime.bucket === "awaiting-input") ? ( - - ) : !isDeleting && laneRuntime.bucket === "ended" ? ( - ) : null} {!isDeleting ? ( (null); const [lastWorkflowTab, setLastWorkflowTab] = React.useState(() => readLastWorkflowTab(projectRoot)); const [integrationRefreshNonce, setIntegrationRefreshNonce] = React.useState(0); + const lastRouteLocationKeyRef = React.useRef(null); const [selectedDetailTab, setSelectedDetailTab] = React.useState(() => { try { return parsePrsRouteState({ search: window.location.search, hash: window.location.hash }).detailTab; @@ -186,6 +187,9 @@ function PRsPageInner() { React.useEffect(() => { const syncFromLocation = () => { try { + const locationKey = `${location.pathname}${location.search}${window.location.hash}`; + const locationChanged = lastRouteLocationKeyRef.current !== locationKey; + lastRouteLocationKeyRef.current = locationKey; const routeState = parsePrsRouteState({ search: location.search, hash: window.location.hash, @@ -203,11 +207,16 @@ function PRsPageInner() { setActiveTab(resolved.activeTab); if (!resolved.isWorkflowRoute) { + const hasExplicitPrSelection = Boolean(routeState.prId) || routeState.prNumber != null; const prNumberMatch = routeState.prNumber == null ? null : prs.find((pr) => pr.githubPrNumber === routeState.prNumber)?.id ?? null; - setSelectedPrId(routeState.prId ?? prNumberMatch); - setSelectedDetailTab(routeState.detailTab); + if (hasExplicitPrSelection || locationChanged) { + setSelectedPrId(routeState.prId ?? prNumberMatch); + } + if (hasExplicitPrSelection || locationChanged || routeState.detailTab) { + setSelectedDetailTab(routeState.detailTab); + } } if (resolved.effectiveWorkflow === "queue") { setSelectedQueueGroupId(routeState.queueGroupId ?? null); diff --git a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx index 284255242..ada5940d3 100644 --- a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx +++ b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx @@ -750,7 +750,7 @@ describe("PrDetailPane issue resolver CTA", () => { }); }); - it("hydrates checks and activity data from a cached snapshot", async () => { + it("hydrates checks and timeline data from a cached snapshot", async () => { const user = userEvent.setup(); const listSnapshots = vi.fn().mockResolvedValue([{ prId: "pr-80", @@ -779,15 +779,59 @@ describe("PrDetailPane issue resolver CTA", () => { expect(screen.getByText("Cached snapshot check")).toBeTruthy(); }); - await user.click(screen.getByRole("button", { name: /activity/i })); + await user.click(screen.getByRole("button", { name: /overview/i })); await waitFor(() => { expect(screen.getByText("Cached comment body")).toBeTruthy(); expect(screen.getByText("Cached review body")).toBeTruthy(); }); }); + it("does not start live detail work from a stale snapshot prefill request", async () => { + let resolveSnapshots!: (snapshots: PrSnapshotHydration[]) => void; + const listSnapshots = vi.fn().mockImplementation(() => new Promise((resolve) => { + resolveSnapshots = resolve; + })); + const getDetail = vi.fn().mockResolvedValue({ + prId: "pr-80", + body: "Live detail should not load", + labels: [], + assignees: [], + requestedReviewers: [], + author: { login: "octocat", avatarUrl: null }, + isDraft: false, + milestone: null, + linkedIssues: [], + }); + const getFiles = vi.fn().mockResolvedValue([]); + const getCommits = vi.fn().mockResolvedValue([]); + const getActionRuns = vi.fn().mockResolvedValue([]); + const { unmount } = renderPane({ + checks: [], + reviewThreads: [], + listSnapshots, + getDetail, + getFiles, + getCommits, + getActionRuns, + }); + + await waitFor(() => { + expect(listSnapshots).toHaveBeenCalledWith({ prId: "pr-80" }); + }); + + unmount(); + await act(async () => { + resolveSnapshots([]); + await Promise.resolve(); + }); + + expect(getDetail).not.toHaveBeenCalled(); + expect(getFiles).not.toHaveBeenCalled(); + expect(getCommits).not.toHaveBeenCalled(); + expect(getActionRuns).not.toHaveBeenCalled(); + }); + it("updates synthesized activity when selected PR detail inputs refresh", async () => { - const user = userEvent.setup(); const { rerenderPane } = renderPane({ checks: [makeCheck({ name: "Old CI", conclusion: "success" })], reviewThreads: [], @@ -797,7 +841,6 @@ describe("PrDetailPane issue resolver CTA", () => { }, }); - await user.click(screen.getByRole("button", { name: /activity/i })); await waitFor(() => { expect(screen.getByText("Old CI: success")).toBeTruthy(); }); @@ -819,7 +862,6 @@ describe("PrDetailPane issue resolver CTA", () => { it("synthesizes unique activity keys for duplicate review and check identities", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); try { - const user = userEvent.setup(); renderPane({ checks: [ makeCheck({ name: "CodeRabbit", detailsUrl: null, startedAt: null, completedAt: null }), @@ -832,8 +874,6 @@ describe("PrDetailPane issue resolver CTA", () => { reviewThreads: [], }); - await user.click(screen.getByRole("button", { name: /activity/i })); - await waitFor(() => { expect(screen.getAllByText("CodeRabbit: failure")).toHaveLength(2); expect(screen.getByText("First review")).toBeTruthy(); @@ -948,7 +988,7 @@ describe("PrDetailPane issue resolver CTA", () => { await user.click(screen.getByRole("button", { name: /ci \/ checks/i })); expect(screen.queryByText("Cached snapshot check")).toBeNull(); - await user.click(screen.getByRole("button", { name: /activity/i })); + await user.click(screen.getByRole("button", { name: /overview/i })); expect(screen.queryByText("Cached comment body")).toBeNull(); expect(screen.queryByText("Cached review body")).toBeNull(); }); @@ -1821,7 +1861,6 @@ describe("PrDetailPane issue resolver CTA", () => { }); it("renders review activity bodies as markdown instead of raw source text", async () => { - const user = userEvent.setup(); renderPane({ checks: [makeCheck()], reviewThreads: [], @@ -1838,7 +1877,6 @@ describe("PrDetailPane issue resolver CTA", () => { ], }); - await user.click(screen.getByRole("button", { name: /activity/i })); expect(await screen.findByText("Actionable comments posted: 3")).toBeTruthy(); expect(screen.queryByText(/\*\*Actionable comments posted: 3\*\*/)).toBeNull(); expect(screen.getByText("Prompt for AI Agents")).toBeTruthy(); diff --git a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx index ac49a9196..1e27a59ab 100644 --- a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx +++ b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx @@ -5,7 +5,7 @@ import rehypeRaw from "rehype-raw"; import rehypeSanitize from "rehype-sanitize"; import { GitBranch, GitMerge, GitCommit, GithubLogo, CheckCircle, XCircle, Circle, - CircleNotch, Sparkle, ArrowRight, Eye, ChatText, Code, ClockCounterClockwise, + CircleNotch, Sparkle, ArrowRight, Eye, ChatText, Code, PencilSimple, X, Check, ArrowsClockwise, Warning, Play, Rocket, Tag, CaretDown, CaretRight, UserCircle, DotsThreeVertical, Robot, Stack as Layers, } from "@phosphor-icons/react"; @@ -19,6 +19,8 @@ import type { PrConvergenceState, PrConvergenceStatePatch, PrSnapshotHydration, + PrChecksStatus, + PrReviewStatus, } from "../../../../shared/types"; import { DEFAULT_PR_TIMELINE_FILTERS, type PrTimelineFilters } from "../shared/PrTimeline"; import type { PaletteKind } from "../shared/PrCommandPalettes"; @@ -42,9 +44,14 @@ import { modifierKeyLabel } from "../../../lib/platform"; // ---- Sub-tab type ---- type DetailTab = PrDetailRouteTab; const DETAIL_TAB_STORAGE_KEY = "ade:prs:detailTabs:v1"; +const DETAIL_BACKGROUND_ACTIVITY_DELAY_MS = 250; function isDetailTab(value: unknown): value is DetailTab { - return value === "overview" || value === "convergence" || value === "files" || value === "checks" || value === "activity"; + return value === "overview" || value === "convergence" || value === "files" || value === "checks"; +} + +function normalizeDetailTab(tab: DetailTab | "activity" | null | undefined): DetailTab { + return tab === "activity" || tab == null ? "overview" : tab; } function isPrsRouteRuntime(): boolean { @@ -62,7 +69,7 @@ function readStoredDetailTab(prId: string): DetailTab | null { if (!raw) return null; const parsed = JSON.parse(raw) as Record; const value = parsed?.[prId]; - return isDetailTab(value) ? value : null; + return isDetailTab(value) ? normalizeDetailTab(value) : null; } catch { return null; } @@ -276,7 +283,7 @@ function CheckIcon({ check }: { check: PrCheck }) { return ; } -// ---- Shared activity event helpers (used by OverviewTab and ActivityTab) ---- +// ---- Shared activity event helpers for the overview thread ---- function activityEventColor(ev: PrActivityEvent): string { if (ev.type === "comment") return ev.metadata?.source === "review" ? COLORS.warning : COLORS.info; if (ev.type === "review") return COLORS.accent; @@ -509,6 +516,8 @@ type PrDetailPaneProps = { onOpenQueueView?: (groupId: string) => void; initialDetailTab?: DetailTab | null; onDetailTabChange?: (tab: DetailTab) => void; + onUnmap?: () => void; + unmapBusy?: boolean; }; export function PrDetailPane({ @@ -531,6 +540,8 @@ export function PrDetailPane({ onOpenQueueView, initialDetailTab, onDetailTabChange, + onUnmap, + unmapBusy = false, }: PrDetailPaneProps) { const { convergenceStatesByPrId, @@ -555,16 +566,17 @@ export function PrDetailPane({ setAiSummaryDismissed, regeneratePrAiSummary, } = usePrs(); + const initialSnapshotHydration = snapshotHydration?.prId === pr.id ? snapshotHydration : null; const [activeTab, setActiveTabState] = React.useState( - () => initialDetailTab ?? readStoredDetailTab(pr.id) ?? "overview", + () => normalizeDetailTab(initialDetailTab ?? readStoredDetailTab(pr.id)), ); - const [detail, setDetail] = React.useState(null); - const [files, setFiles] = React.useState([]); - const [commits, setCommits] = React.useState([]); - const [snapshotStatus, setSnapshotStatus] = React.useState(null); - const [snapshotChecks, setSnapshotChecks] = React.useState([]); - const [snapshotReviews, setSnapshotReviews] = React.useState([]); - const [snapshotComments, setSnapshotComments] = React.useState([]); + const [detail, setDetail] = React.useState(() => initialSnapshotHydration?.detail ?? null); + const [files, setFiles] = React.useState(() => initialSnapshotHydration?.files ?? []); + const [commits, setCommits] = React.useState(() => initialSnapshotHydration?.commits ?? []); + const [snapshotStatus, setSnapshotStatus] = React.useState(() => initialSnapshotHydration?.status ?? null); + const [snapshotChecks, setSnapshotChecks] = React.useState(() => initialSnapshotHydration?.checks ?? []); + const [snapshotReviews, setSnapshotReviews] = React.useState(() => initialSnapshotHydration?.reviews ?? []); + const [snapshotComments, setSnapshotComments] = React.useState(() => initialSnapshotHydration?.comments ?? []); const [actionRuns, setActionRuns] = React.useState([]); const [activity, setActivity] = React.useState([]); const [reviewThreads, setReviewThreads] = React.useState([]); @@ -586,22 +598,18 @@ export function PrDetailPane({ }, [onDetailTabChange, pr.id]); React.useEffect(() => { - const next = initialDetailTab ?? readStoredDetailTab(pr.id) ?? "overview"; + const next = normalizeDetailTab(initialDetailTab ?? readStoredDetailTab(pr.id)); setActiveTabState(next); - setSnapshotStatus(null); - setSnapshotChecks([]); - setSnapshotReviews([]); - setSnapshotComments([]); if (initialDetailTab) { - writeStoredDetailTab(pr.id, initialDetailTab); + writeStoredDetailTab(pr.id, normalizeDetailTab(initialDetailTab)); } }, [initialDetailTab, pr.id]); React.useEffect(() => { const onTourTab = (event: Event) => { - const tab = (event as CustomEvent).detail; + const tab = (event as CustomEvent).detail; if (tab === "overview" || tab === "convergence" || tab === "files" || tab === "checks" || tab === "activity") { - setActiveTab(tab); + setActiveTab(normalizeDetailTab(tab)); } }; window.addEventListener("ade:tour-pr-detail-tab", onTourTab); @@ -638,6 +646,10 @@ export function PrDetailPane({ const handleRegenerateAiSummary = React.useCallback(() => { void regeneratePrAiSummary?.(pr.id); }, [pr.id, regeneratePrAiSummary]); + const timelineAiSummary = React.useMemo( + () => (detailAiSummary?.prId === pr.id ? detailAiSummary : null), + [detailAiSummary, pr.id], + ); // Page-level keyboard shortcuts scoped to the Timeline+Rails overview. // Only attach listeners when the flag is on AND the overview tab is active. @@ -842,6 +854,9 @@ export function PrDetailPane({ const detailStatusRefreshKeyRef = React.useRef(null); const inventoryLoadSeqRef = React.useRef(0); const snapshotHydrationRef = React.useRef(snapshotHydration); + const snapshotPrefillPendingRef = React.useRef(false); + const visibleActivityCountRef = React.useRef(0); + const activityFetchKeyRef = React.useRef(null); const liveDetailLoadedForPrRef = React.useRef(null); const liveFilesLoadedForPrRef = React.useRef(null); const liveCommitsLoadedForPrRef = React.useRef(null); @@ -869,15 +884,16 @@ export function PrDetailPane({ } }, [applySnapshotHydration, pr.id, snapshotHydration]); - const loadDetail = React.useCallback(async (options: { hydrateSnapshot?: boolean } = {}) => { + const loadDetail = React.useCallback(async (options: { hydrateSnapshot?: boolean; forceLive?: boolean } = {}) => { const requestId = ++detailLoadSeqRef.current; try { if (options.hydrateSnapshot) { const contextSnapshot = snapshotHydrationRef.current?.prId === pr.id ? snapshotHydrationRef.current : null; - const cachedSnapshot = contextSnapshot ?? (!snapshotHydrationOwnedByContext && typeof window.ade.prs.listSnapshots === "function" + const cachedSnapshot = contextSnapshot ?? (typeof window.ade.prs.listSnapshots === "function" ? (await window.ade.prs.listSnapshots({ prId: pr.id }).catch(() => []))[0] : null); - if (cachedSnapshot && requestId === detailLoadSeqRef.current) { + if (requestId !== detailLoadSeqRef.current) return; + if (cachedSnapshot) { applySnapshotHydration(cachedSnapshot); } } @@ -911,8 +927,12 @@ export function PrDetailPane({ await Promise.allSettled([detailPromise, filesPromise, commitsPromise, actionRunsPromise]); } catch { // silently fail - basic data still available from context + } finally { + if (requestId === detailLoadSeqRef.current) { + snapshotPrefillPendingRef.current = false; + } } - }, [applySnapshotHydration, pr.id, snapshotHydrationOwnedByContext]); + }, [applySnapshotHydration, pr.id]); const refreshReviewThreads = React.useCallback(async () => { const requestId = detailLoadSeqRef.current; @@ -954,17 +974,23 @@ export function PrDetailPane({ setShowReviewerEditor(false); setShowReviewModal(false); setActivity([]); + activityFetchKeyRef.current = null; liveDetailLoadedForPrRef.current = null; liveFilesLoadedForPrRef.current = null; liveCommitsLoadedForPrRef.current = null; - setDetail(null); - setFiles([]); - setCommits([]); + const contextSnapshot = snapshotHydrationRef.current?.prId === pr.id ? snapshotHydrationRef.current : null; + if (contextSnapshot) { + applySnapshotHydration(contextSnapshot); + } else { + setDetail(null); + setFiles([]); + setCommits([]); + setSnapshotStatus(null); + setSnapshotChecks([]); + setSnapshotReviews([]); + setSnapshotComments([]); + } setActionRuns([]); - setSnapshotStatus(null); - setSnapshotChecks([]); - setSnapshotReviews([]); - setSnapshotComments([]); const requestId = ++convergenceLoadSeqRef.current; const cachedRuntime = cachedConvergenceRuntimeRef.current; @@ -981,13 +1007,14 @@ export function PrDetailPane({ } }); + snapshotPrefillPendingRef.current = true; void loadDetail({ hydrateSnapshot: true }); return () => { detailLoadSeqRef.current += 1; inventoryLoadSeqRef.current += 1; convergenceLoadSeqRef.current += 1; }; - }, [applyConvergenceRuntime, loadConvergenceState, loadDetail, pr.id]); + }, [applyConvergenceRuntime, applySnapshotHydration, loadConvergenceState, loadDetail, pr.id]); React.useEffect(() => { const key = [ @@ -1003,7 +1030,7 @@ export function PrDetailPane({ } if (prev === key) return; detailStatusRefreshKeyRef.current = key; - void loadDetail(); + void loadDetail({ forceLive: true }); void refreshReviewThreads(); }, [loadDetail, pr.checksStatus, pr.id, pr.reviewStatus, pr.updatedAt, refreshReviewThreads]); @@ -1014,25 +1041,39 @@ export function PrDetailPane({ const visibleActivity = activity.length > 0 ? activity : derivedActivity; React.useEffect(() => { - const shouldLoadImmediately = activeTab === "activity" || Boolean(deepLinkState.eventId); + visibleActivityCountRef.current = visibleActivity.length; + }, [visibleActivity.length]); + + React.useEffect(() => { + const shouldLoadImmediately = activeTab === "overview" || Boolean(deepLinkState.eventId); if (!shouldLoadImmediately) return undefined; + const key = `${pr.id}|${activeTab}|${deepLinkState.eventId ?? ""}`; + if (activityFetchKeyRef.current === key) return undefined; + activityFetchKeyRef.current = key; let cancelled = false; - window.ade.prs.getActivity(pr.id).then((events) => { - if (!cancelled) setActivity(events); - }).catch(() => {}); + const hasLocalActivity = visibleActivityCountRef.current > 0; + const delay = !deepLinkState.eventId && (hasLocalActivity || snapshotPrefillPendingRef.current) + ? DETAIL_BACKGROUND_ACTIVITY_DELAY_MS + : 0; + const timeoutId = window.setTimeout(() => { + window.ade.prs.getActivity(pr.id).then((events) => { + if (!cancelled) setActivity(events); + }).catch(() => {}); + }, delay); return () => { cancelled = true; + window.clearTimeout(timeoutId); }; }, [activeTab, deepLinkState.eventId, pr.id]); // Poll checks + actionRuns + reviewThreads every 60s so the // Path to Merge readiness panel stays fresh without requiring a manual refresh. // Full activity fetches include comments/reviews/checks again, so only do that - // while the Activity tab is actually visible. + // while the Overview thread is actually visible. React.useEffect(() => { let cancelled = false; const id = window.setInterval(() => { - const activityPromise = activeTab === "activity" + const activityPromise = activeTab === "overview" ? window.ade.prs.getActivity(pr.id) : Promise.resolve(null); Promise.allSettled([ @@ -1091,7 +1132,7 @@ export function PrDetailPane({ await window.ade.prs.addComment({ prId: pr.id, body: commentDraft }); setCommentDraft(""); await onRefresh(); - await loadDetail(); + await loadDetail({ forceLive: true }); }); }; @@ -1108,20 +1149,20 @@ export function PrDetailPane({ await window.ade.prs.updateBody({ prId: pr.id, body: bodyDraft }); setEditingBody(false); await onRefresh(); - await loadDetail(); + await loadDetail({ forceLive: true }); }); const handleSetLabels = (labels: string[]) => runAction(async () => { await window.ade.prs.setLabels({ prId: pr.id, labels }); setShowLabelEditor(false); - await loadDetail(); + await loadDetail({ forceLive: true }); }); const handleRequestReviewers = (reviewers: string[]) => runAction(async () => { await window.ade.prs.requestReviewers({ prId: pr.id, reviewers }); setShowReviewerEditor(false); await onRefresh(); - await loadDetail(); + await loadDetail({ forceLive: true }); }); const handleSubmitReview = () => runAction(async () => { @@ -1144,7 +1185,7 @@ export function PrDetailPane({ const handleRerunChecks = () => runAction(async () => { await window.ade.prs.rerunChecks({ prId: pr.id }); await onRefresh(); - await loadDetail(); + await loadDetail({ forceLive: true }); }); const handleAiSummary = async () => { @@ -1288,7 +1329,7 @@ export function PrDetailPane({ }, [checks, pr.id, pr.state]); const refreshDetailSurface = React.useCallback(async (options: { includeInventory?: boolean } = {}) => { - const tasks: Array> = [onRefresh({ prId: pr.id }), loadDetail(), refreshReviewThreads()]; + const tasks: Array> = [onRefresh({ prId: pr.id }), loadDetail({ forceLive: true }), refreshReviewThreads()]; if (options.includeInventory && pr.state !== "merged" && pr.state !== "closed") { tasks.push(syncInventory()); } @@ -2255,14 +2296,27 @@ export function PrDetailPane({ const localBehindCount = laneForPr?.status?.behind ?? 0; const sc = getPrStateBadge(pr.state); - const cc = getPrChecksBadge(pr.checksStatus); - const rc = getPrReviewsBadge(pr.reviewStatus); + const resolvedChecksStatus = React.useMemo(() => { + if (status?.checksStatus) return status.checksStatus; + if (checks.length === 0) return pr.checksStatus; + if (checks.some((check) => check.status === "queued" || check.status === "in_progress")) return "pending"; + if (checks.some((check) => check.conclusion === "failure" || check.conclusion === "cancelled")) return "failing"; + if (checks.some((check) => check.status === "completed")) return "passing"; + return pr.checksStatus; + }, [checks, pr.checksStatus, status?.checksStatus]); + const resolvedReviewStatus = React.useMemo(() => { + if (status?.reviewStatus) return status.reviewStatus; + if (reviews.some((review) => review.state === "changes_requested")) return "changes_requested"; + if (reviews.some((review) => review.state === "approved")) return "approved"; + return pr.reviewStatus; + }, [pr.reviewStatus, reviews, status?.reviewStatus]); + const cc = getPrChecksBadge(resolvedChecksStatus); + const rc = getPrReviewsBadge(resolvedReviewStatus); const TAB_ACTIVE_COLORS: Record = { overview: COLORS.accent, convergence: COLORS.accent, files: COLORS.info, checks: COLORS.success, - activity: COLORS.warning, }; const isTerminalPr = pr.state === "merged" || pr.state === "closed"; @@ -2289,11 +2343,12 @@ export function PrDetailPane({ { id: "convergence", label: "Path to Merge", icon: Sparkle, count: newIssueCount > 0 ? newIssueCount : undefined }, { id: "files", label: "Files", icon: Code, count: files.length }, { id: "checks", label: "CI / Checks", icon: Play, count: buildUnifiedChecks(checks, actionRuns).length }, - { id: "activity", label: "Activity", icon: ClockCounterClockwise, count: visibleActivity.length }, ]; + const overviewRailsActive = activeTab === "overview" && prsTimelineRailsEnabled; + return ( -
+
{/* ===== HEADER ===== */}
{/* Title row */} @@ -2401,6 +2456,22 @@ export function PrDetailPane({ + {onUnmap ? ( + + ) : null} {queueContext && onOpenQueueView ? (
0 ? activity.length : comments.length}) {(() => { - // Use full activity timeline if available, else fall back to comments. - // Filter out ci_run events — they're shown in the CI/Checks tab. + // Use the full activity timeline if available, else fall back to comments. const timeline = activity.length > 0 - ? [...activity].filter((ev) => ev.type !== "ci_run").sort((a, b) => { + ? [...activity].sort((a, b) => { const ta = a.timestamp ? new Date(a.timestamp).getTime() : 0; const tb = b.timestamp ? new Date(b.timestamp).getTime() : 0; return ta - tb; @@ -4083,140 +4150,6 @@ function ChecksTab({ checks, actionRuns, actionBusy, onRerunChecks, showIssueRes ); } -// ================================================================ -// ACTIVITY TAB -// ================================================================ - -function ActivityTab({ activity, comments, reviews, commentDraft, setCommentDraft, actionBusy, onAddComment }: { - activity: PrActivityEvent[]; - comments: PrComment[]; - reviews: PrReview[]; - commentDraft: string; - setCommentDraft: (v: string) => void; - actionBusy: boolean; - onAddComment: () => void; -}) { - // Merge comments and reviews into a timeline if activity is empty - const timeline = React.useMemo(() => { - if (activity.length > 0) return activity; - const events: PrActivityEvent[] = []; - for (const c of comments) { - events.push({ - id: c.id, type: "comment", author: c.author, avatarUrl: c.authorAvatarUrl || null, - body: c.body, timestamp: c.createdAt ?? "", metadata: { path: c.path, line: c.line }, - }); - } - for (let ri = 0; ri < reviews.length; ri++) { - const r = reviews[ri]; - events.push({ - id: `review-${r.reviewer}-${r.submittedAt}-${ri}`, type: "review", author: r.reviewer, avatarUrl: r.reviewerAvatarUrl || null, - body: r.body, timestamp: r.submittedAt ?? "", metadata: { state: r.state }, - }); - } - return events.sort((a, b) => new Date(b.timestamp).getTime() - new Date(a.timestamp).getTime()); - }, [activity, comments, reviews]); - - return ( -
- {/* Comment input at top */} -
-
-