diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index 9cc1d57e50..db6df76cc1 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -1035,6 +1035,13 @@ describe.sequential("agent skill routes", () => { }), expect.any(Object), ); + expect(mockAgentInstructionsService.materializeManagedBundle).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ + "AGENTS.md": expect.stringMatching(/PUT \/issues\/\{id\}\/documents\/plan[\s\S]*Re-`GET \/documents\/plan`, assert it returns `200`[\s\S]*latestRevisionId[\s\S]*target=\{ type: 'issue_document', key: 'plan', revisionId: latestRevisionId \}[\s\S]*Never present a plan only in a thread comment or through `ask_user_questions`/), + }), + expect.any(Object), + ); expect(mockAgentInstructionsService.materializeManagedBundle).toHaveBeenCalledWith( expect.any(Object), expect.objectContaining({ diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index afea3529f6..bc2b3dfaef 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -1043,6 +1043,47 @@ describe.sequential("issue thread interaction routes", () => { expect(mockInteractionService.create).not.toHaveBeenCalled(); }); + it("forwards plan-document confirmations to the interaction service for revision validation", async () => { + const app = await createApp(); + + const res = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions") + .send({ + kind: "request_confirmation", + payload: { + version: 1, + prompt: "Approve the plan?", + target: { + type: "issue_document", + issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + documentId: "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb", + key: "plan", + revisionId: "cccccccc-cccc-4ccc-8ccc-cccccccccccc", + revisionNumber: 1, + }, + }, + }); + + // The route delegates plan-target validation to the service, which rejects a + // stale/missing revision atomically inside its insert transaction + // (assertRequestConfirmationTargetIsCurrent). The route must pass the target + // through unchanged rather than pre-checking it non-atomically. + expect(res.status).toBe(201); + expect(mockInteractionService.create).toHaveBeenCalledWith( + expect.objectContaining({ id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }), + expect.objectContaining({ + kind: "request_confirmation", + payload: expect.objectContaining({ + target: expect.objectContaining({ + key: "plan", + revisionId: "cccccccc-cccc-4ccc-8ccc-cccccccccccc", + }), + }), + }), + expect.anything(), + ); + }); + it("accepts request checkbox confirmations with selected option ids and wakes the assignee", async () => { mockInteractionService.acceptInteraction.mockResolvedValueOnce({ interaction: { diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index 364d2bbe64..7a4d0d081a 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -2524,6 +2524,130 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { }); }); + it("rejects creating a plan confirmation against a stale document revision and accepts the current one", async () => { + const companyId = randomUUID(); + const goalId = randomUUID(); + const issueId = randomUUID(); + const documentId = randomUUID(); + const revisionId = randomUUID(); + const nextRevisionId = randomUUID(); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await instanceSettingsService(db).updateExperimental({ enableIsolatedWorkspaces: false }); + await db.insert(goals).values({ + id: goalId, + companyId, + title: "Stale plan confirmation", + level: "task", + status: "active", + }); + await db.insert(issues).values({ + id: issueId, + companyId, + goalId, + title: "Parent issue", + status: "in_progress", + priority: "medium", + }); + // Document is already at revision 2 — revision 1 is stale. + await db.insert(documents).values({ + id: documentId, + companyId, + title: "Plan", + format: "markdown", + latestBody: "v2", + latestRevisionId: nextRevisionId, + latestRevisionNumber: 2, + }); + await db.insert(issueDocuments).values({ + companyId, + issueId, + documentId, + key: "plan", + }); + await db.insert(documentRevisions).values([ + { + id: revisionId, + companyId, + documentId, + revisionNumber: 1, + title: "Plan", + format: "markdown", + body: "v1", + }, + { + id: nextRevisionId, + companyId, + documentId, + revisionNumber: 2, + title: "Plan", + format: "markdown", + body: "v2", + }, + ]); + + const staleTarget = { + type: "issue_document" as const, + issueId, + documentId, + key: "plan", + revisionId, + revisionNumber: 1, + }; + + // The revision check runs inside the create transaction (locking the + // document row), so a target pointing at an older revision is rejected + // atomically with the would-be insert rather than by a racy pre-check. + await expect(interactionsSvc.create({ + id: issueId, + companyId, + }, { + kind: "request_confirmation", + continuationPolicy: "wake_assignee", + payload: { + version: 1, + prompt: "Apply the plan document?", + target: staleTarget, + }, + }, { + userId: "local-board", + })).rejects.toMatchObject({ + status: 422, + message: expect.stringContaining("current issue document revision"), + }); + + const noRows = await db + .select({ id: issueThreadInteractions.id }) + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.issueId, issueId)); + expect(noRows).toHaveLength(0); + + const created = await interactionsSvc.create({ + id: issueId, + companyId, + }, { + kind: "request_confirmation", + continuationPolicy: "wake_assignee", + payload: { + version: 1, + prompt: "Apply the plan document?", + target: { + ...staleTarget, + revisionId: nextRevisionId, + revisionNumber: 2, + }, + }, + }, { + userId: "local-board", + }); + expect(created).toMatchObject({ status: "pending", kind: "request_confirmation" }); + }); + it("preserves resolved request_item_verdicts items when the watched issue document revision changes", async () => { const companyId = randomUUID(); const goalId = randomUUID(); diff --git a/server/src/onboarding-assets/ceo/AGENTS.md b/server/src/onboarding-assets/ceo/AGENTS.md index 1254f43742..6e09de67f3 100644 --- a/server/src/onboarding-assets/ceo/AGENTS.md +++ b/server/src/onboarding-assets/ceo/AGENTS.md @@ -34,7 +34,12 @@ You MUST delegate work rather than doing it yourself. When a task is assigned to - If the board asks you to do something and you're unsure who should own it, default to the CTO for technical work. - Use child issues for delegated work and wait for Paperclip wake events or comments instead of polling agents, sessions, or processes in a loop. - Create child issues directly when ownership and scope are clear. Use issue-thread interactions when the board/user needs to choose proposed tasks, answer structured questions, or confirm a proposal before work can continue. -- Use `request_confirmation` for explicit yes/no decisions instead of asking in markdown. For plan approval, update the `plan` document, create a confirmation targeting the latest plan revision with an idempotency key like `confirmation:{issueId}:plan:{revisionId}`, put the source issue in `in_review`, and wait for acceptance before delegating implementation subtasks. +- Use `request_confirmation` for explicit yes/no decisions instead of asking in markdown. Before presenting a plan for review, you MUST complete this publish contract: + 1. `PUT /issues/{id}/documents/plan` with `{ format: 'markdown', body, changeSummary }`. + 2. Re-`GET /documents/plan`, assert it returns `200`, and capture its `latestRevisionId`. + 3. Only then create `request_confirmation` with `target={ type: 'issue_document', key: 'plan', revisionId: latestRevisionId }` and `idempotencyKey=confirmation:{issueId}:plan:{revisionId}`. + 4. Put the source issue in `in_review` and wait for acceptance before delegating implementation subtasks. + Never present a plan only in a thread comment or through `ask_user_questions`; comments are supporting context and questions are for gathering input, not plan review. - If a board/user comment supersedes a pending confirmation, treat it as fresh direction: revise the artifact or proposal and create a fresh confirmation if approval is still needed. - Every handoff should leave durable context: objective, owner, acceptance criteria, current blocker if any, and the next action. - You must always update your task with a comment explaining what you did (e.g., who you delegated to and why). diff --git a/server/src/onboarding-assets/default/AGENTS.md b/server/src/onboarding-assets/default/AGENTS.md index 47a6d41ddf..a462c8c9d8 100644 --- a/server/src/onboarding-assets/default/AGENTS.md +++ b/server/src/onboarding-assets/default/AGENTS.md @@ -11,7 +11,12 @@ You are an agent at Paperclip company. - Final disposition checklist: mark `done` when complete and verified; use `in_review` only with a real reviewer, approval, interaction, or monitor path; use `blocked` only with first-class blockers or a named unblock owner/action; create delegated follow-up issues with blockers when another agent owns the next step; keep `in_progress` only when a live continuation path exists. - Use child issues for parallel or long delegated work instead of polling agents, sessions, or processes. - Create child issues directly when you know what needs to be done. If the board/user needs to choose suggested tasks, answer structured questions, or confirm a proposal first, create an issue-thread interaction on the current issue with `POST /api/issues/{issueId}/interactions` using `kind: "suggest_tasks"`, `kind: "ask_user_questions"`, or `kind: "request_confirmation"`. -- Use `request_confirmation` instead of asking for yes/no decisions in markdown. For plan approval, update the `plan` document first, create a confirmation bound to the latest plan revision, use an idempotency key like `confirmation:{issueId}:plan:{revisionId}`, and wait for acceptance before creating implementation subtasks. +- Use `request_confirmation` instead of asking for yes/no decisions in markdown. Before presenting a plan for review, you MUST complete this publish contract: + 1. `PUT /issues/{id}/documents/plan` with `{ format: 'markdown', body, changeSummary }`. + 2. Re-`GET /documents/plan`, assert it returns `200`, and capture its `latestRevisionId`. + 3. Only then create `request_confirmation` with `target={ type: 'issue_document', key: 'plan', revisionId: latestRevisionId }` and `idempotencyKey=confirmation:{issueId}:plan:{revisionId}`. + 4. Wait for acceptance before creating implementation subtasks. + Never present a plan only in a thread comment or through `ask_user_questions`; comments are supporting context and questions are for gathering input, not plan review. - `ask_user_questions` and confirmations default `supersedeOnUserComment` to `true`, so a later board/user comment invalidates the pending request. Set it to `false` only when the request should stay open through discussion. If you wake up from a superseding comment, revise the artifact, question set, or proposal and create a fresh interaction if input is still needed. - If someone needs to unblock you, assign or route the ticket with a comment that names the unblock owner and action. - Respect budget, pause/cancel, approval gates, and company boundaries. diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 036ea45b22..01ad5a06b9 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -10042,6 +10042,12 @@ export function issueRoutes( throw unprocessable("payload.toolAction is server-owned metadata and cannot be supplied when creating an interaction"); } + // Plan-document confirmation targets are validated authoritatively inside + // issueThreadInteractionService.create, which re-reads the plan document's + // latest revision and rejects a stale/missing target under the same insert + // transaction (see assertRequestConfirmationTargetIsCurrent). We deliberately + // do not pre-check the revision here: a separate route-level read would be + // non-atomic with the insert and only duplicate the service gate. const interaction = await issueThreadInteractionService(db).create(issue, { ...req.body, sourceRunId: req.actor.type === "agent" ? agentSourceRunId : req.body.sourceRunId ?? null, diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index b9af68eea2..3a2e5661d3 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -1009,10 +1009,14 @@ async function getIssueDocumentTargetSnapshot(db: Db | any, args: { companyId: string; issueId: string; target: RequestConfirmationTarget; + // When true, take a FOR UPDATE row lock on the joined document so a concurrent + // revision publish (which updates documents.latestRevisionId) must serialize + // behind the caller's transaction. Only meaningful inside a transaction. + lockForUpdate?: boolean; }) { if (args.target.type !== "issue_document") return null; const targetIssueId = args.target.issueId ?? args.issueId; - const row = await db + const query = db .select({ issueId: issueDocuments.issueId, documentId: issueDocuments.documentId, @@ -1026,7 +1030,8 @@ async function getIssueDocumentTargetSnapshot(db: Db | any, args: { eq(issueDocuments.companyId, args.companyId), eq(issueDocuments.issueId, targetIssueId), eq(issueDocuments.key, args.target.key), - )) + )); + const row = await (args.lockForUpdate ? query.for("update", { of: documents }) : query) .then((rows: Array<{ issueId: string; documentId: string; @@ -1080,6 +1085,10 @@ async function assertRequestConfirmationTargetIsCurrent(db: Db | any, args: { companyId: string; issueId: string; target?: RequestConfirmationTarget | null; + // Forwarded to getIssueDocumentTargetSnapshot; pass true when validating + // inside the create transaction so the revision read locks the document row + // and stays atomic with the interaction insert. + lockForUpdate?: boolean; }) { if (!args.target) return; if (args.target.type !== "issue_document") return; @@ -1087,6 +1096,7 @@ async function assertRequestConfirmationTargetIsCurrent(db: Db | any, args: { companyId: args.companyId, issueId: args.issueId, target: args.target, + lockForUpdate: args.lockForUpdate, }); if (!snapshot || snapshot.latestRevisionId !== args.target.revisionId) { throw unprocessable("request_confirmation target must reference the current issue document revision"); @@ -1812,17 +1822,10 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti } } - if ( + const requiresCurrentTarget = data.kind === "request_confirmation" || data.kind === "request_checkbox_confirmation" - || data.kind === "request_item_verdicts" - ) { - await assertRequestConfirmationTargetIsCurrent(db, { - companyId: issue.companyId, - issueId: issue.id, - target: data.payload.target ?? null, - }); - } + || data.kind === "request_item_verdicts"; let created: IssueThreadInteractionRow; let superseded: IssueThreadInteractionRow[] = []; @@ -1841,6 +1844,19 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti if (!issueRow || isTerminalIssueStatus(issueRow.status)) { throw conflict("Cannot create an interaction on a closed issue"); } + // Validate the plan/document confirmation target inside the same + // transaction (locking the document row) so the latest-revision check + // is atomic with the insert below. A concurrent revision publish can no + // longer slip between the check and the insert to leave a confirmation + // pointing at a stale revision. + if (requiresCurrentTarget) { + await assertRequestConfirmationTargetIsCurrent(tx, { + companyId: issue.companyId, + issueId: issue.id, + target: data.payload.target ?? null, + lockForUpdate: true, + }); + } const [row] = await tx .insert(issueThreadInteractions) .values({ diff --git a/ui/src/components/DocumentAnnotationPanel.tsx b/ui/src/components/DocumentAnnotationPanel.tsx index e9fdfc5283..40305f72b9 100644 --- a/ui/src/components/DocumentAnnotationPanel.tsx +++ b/ui/src/components/DocumentAnnotationPanel.tsx @@ -58,6 +58,8 @@ export interface AnnotationPanelProps { isMobile?: boolean; /** Desktop panel width calculated by the document frame. */ desktopWidth?: number; + /** Render as a full-width card in a constrained host instead of a floating side panel. */ + inline?: boolean; className?: string; /** Resolve `` to a display name. */ agentMap?: ReadonlyMap & Partial>>; @@ -92,7 +94,8 @@ export function DocumentAnnotationPanel(props: AnnotationPanelProps) { aria-label={`Annotations for ${props.documentKey.toUpperCase()}, revision ${props.documentRevisionNumber}`} data-testid="document-annotation-panel" className={cn( - "isolate flex h-full max-h-(--sz-80vh) w-(--sz-360px) shrink-0 flex-col overflow-hidden rounded-none border border-border bg-popover text-popover-foreground shadow-xl", + "isolate flex h-full max-h-(--sz-80vh) shrink-0 flex-col overflow-hidden rounded-none border border-border bg-popover text-popover-foreground shadow-xl", + props.inline ? "w-full" : "w-(--sz-360px)", props.className, )} style={props.desktopWidth ? { width: props.desktopWidth, maxWidth: props.desktopWidth } : undefined} diff --git a/ui/src/components/IssueDocumentAnnotations.test.tsx b/ui/src/components/IssueDocumentAnnotations.test.tsx index 1af735cd53..f83694c19a 100644 --- a/ui/src/components/IssueDocumentAnnotations.test.tsx +++ b/ui/src/components/IssueDocumentAnnotations.test.tsx @@ -1,7 +1,7 @@ // @vitest-environment jsdom import { useState } from "react"; -import { createRoot } from "react-dom/client"; +import { createRoot as createReactRoot } from "react-dom/client"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import type { DocumentAnnotationThreadWithComments, @@ -138,10 +138,47 @@ async function act(callback: () => void | Promise) { await new Promise((resolve) => setTimeout(resolve, 0)); } +// Track every root so afterEach can unmount it. Tests mount into a throwaway +// container but never unmount, so without this the panel's window scroll/resize +// listeners and react-query subscriptions from earlier tests stay live and can +// recompute positioning against a detached host — an order-dependent flake that +// only surfaced under CI's fuller suite run. +const activeRoots: Array> = []; +function createRoot(node: Parameters[0]) { + const root = createReactRoot(node); + activeRoots.push(root); + return root; +} + +async function unmountActiveRoots() { + if (activeRoots.length === 0) return; + const roots = activeRoots.splice(0); + await act(() => { + for (const root of roots) root.unmount(); + }); +} + async function flush() { await act(() => {}); } +// Poll an assertion across React flushes until it passes or times out. The panel +// positions itself and loads threads through effects + react-query, so a fixed +// number of flushes can race on a loaded machine (CI). Waiting on the assertion +// itself is deterministic regardless of how many turns the settle takes. +async function waitFor(assertion: () => void, { timeout = 2000 }: { timeout?: number } = {}) { + const start = Date.now(); + for (;;) { + try { + assertion(); + return; + } catch (error) { + if (Date.now() - start > timeout) throw error; + await flush(); + } + } +} + function setTextareaValue(textarea: HTMLTextAreaElement, value: string) { const setter = Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")?.set; setter?.call(textarea, value); @@ -249,6 +286,7 @@ function Harness({ historicalPreview = false, locationHash = "", initialPanelOpen = false, + panelPlacement, }: { doc: IssueDocument; draftDirty?: boolean; @@ -256,6 +294,7 @@ function Harness({ historicalPreview?: boolean; locationHash?: string; initialPanelOpen?: boolean; + panelPlacement?: "floating" | "inline"; }) { const [open, setOpen] = useState(initialPanelOpen); return ( @@ -276,6 +315,7 @@ function Harness({ locationHash={locationHash} panelOpen={open} onPanelOpenChange={setOpen} + panelPlacement={panelPlacement} >

Body content

@@ -292,7 +332,8 @@ describe("IssueDocumentAnnotations", () => { vi.clearAllMocks(); }); - afterEach(() => { + afterEach(async () => { + await unmountActiveRoots(); container.remove(); }); @@ -329,6 +370,30 @@ describe("IssueDocumentAnnotations", () => { expect(anchor?.className).toContain("z-(--z-60)"); }); + it("stacks an inline panel below the document instead of floating over its host", async () => { + mockAnnotationsApi.list.mockResolvedValue([makeThread()]); + const root = createRoot(container); + const queryClient = makeQueryClient(); + const doc = makeDoc(); + + await act(async () => { + root.render( + + + , + ); + }); + await flush(); + await flush(); + + const inlinePanel = container.querySelector('[data-testid="document-annotation-panel-inline"]'); + const floatingAnchor = container.querySelector('[data-testid="document-annotation-panel-anchor"]'); + const panel = container.querySelector('[data-testid="document-annotation-panel"]'); + expect(inlinePanel).not.toBeNull(); + expect(floatingAnchor).toBeNull(); + expect(panel?.className).toContain("w-full"); + }); + it("keeps the desktop annotation panel inside the issue content area when properties are visible", async () => { mockAnnotationsApi.list.mockResolvedValue([makeThread()]); const originalGetBoundingClientRect = HTMLElement.prototype.getBoundingClientRect; @@ -370,17 +435,16 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - - const anchor = container.querySelector('[data-testid="document-annotation-panel-anchor"]') as HTMLElement | null; - const panel = container.querySelector('[data-testid="document-annotation-panel"]') as HTMLElement | null; - expect(anchor).not.toBeNull(); - expect(panel).not.toBeNull(); - expect(anchor!.style.left).toBe("524px"); - expect(anchor!.style.width).toBe("360px"); - expect(panel!.style.width).toBe("360px"); - expect(parseFloat(anchor!.style.left) + parseFloat(anchor!.style.width)).toBeLessThanOrEqual(884); + await waitFor(() => { + const anchor = container.querySelector('[data-testid="document-annotation-panel-anchor"]') as HTMLElement | null; + const panel = container.querySelector('[data-testid="document-annotation-panel"]') as HTMLElement | null; + expect(anchor).not.toBeNull(); + expect(panel).not.toBeNull(); + expect(anchor!.style.left).toBe("524px"); + expect(anchor!.style.width).toBe("360px"); + expect(panel!.style.width).toBe("360px"); + expect(parseFloat(anchor!.style.left) + parseFloat(anchor!.style.width)).toBeLessThanOrEqual(884); + }); } finally { rectSpy.mockRestore(); } @@ -427,15 +491,14 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - - const anchor = container.querySelector('[data-testid="document-annotation-panel-anchor"]') as HTMLElement | null; - expect(anchor).not.toBeNull(); - // The document body ends at 640; the panel should clear it with a margin - // rather than sitting flush against the document's right edge. - expect(parseFloat(anchor!.style.left)).toBeGreaterThan(640); - expect(anchor!.style.left).toBe("664px"); + await waitFor(() => { + const anchor = container.querySelector('[data-testid="document-annotation-panel-anchor"]') as HTMLElement | null; + expect(anchor).not.toBeNull(); + // The document body ends at 640; the panel should clear it with a margin + // rather than sitting flush against the document's right edge. + expect(parseFloat(anchor!.style.left)).toBeGreaterThan(640); + expect(anchor!.style.left).toBe("664px"); + }); } finally { rectSpy.mockRestore(); } @@ -454,13 +517,12 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - - const panel = container.querySelector('[data-testid="document-annotation-panel"]'); - expect(panel).not.toBeNull(); - const focusedThread = container.querySelector('[data-thread-id="thread-99"][data-focused]'); - expect(focusedThread).not.toBeNull(); + await waitFor(() => { + const panel = container.querySelector('[data-testid="document-annotation-panel"]'); + expect(panel).not.toBeNull(); + const focusedThread = container.querySelector('[data-thread-id="thread-99"][data-focused]'); + expect(focusedThread).not.toBeNull(); + }); }); it("shows a disabled reason in the panel when the draft is dirty", async () => { @@ -476,14 +538,13 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - - const reason = container.querySelector( - '[data-testid="document-annotation-disabled-reason"]', - ); - expect(reason).not.toBeNull(); - expect(reason!.textContent).toMatch(/draft/i); + await waitFor(() => { + const reason = container.querySelector( + '[data-testid="document-annotation-disabled-reason"]', + ); + expect(reason).not.toBeNull(); + expect(reason!.textContent).toMatch(/draft/i); + }); }); it("shows open and resolved threads together in a single list (no filter tabs)", async () => { @@ -503,12 +564,11 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - // Open + resolved both render without any filter interaction. - expect(container.querySelector('[data-thread-id="open-1"]')).not.toBeNull(); - expect(container.querySelector('[data-thread-id="resolved-1"]')).not.toBeNull(); + await waitFor(() => { + expect(container.querySelector('[data-thread-id="open-1"]')).not.toBeNull(); + expect(container.querySelector('[data-thread-id="resolved-1"]')).not.toBeNull(); + }); // Orphaned threads can't be anchored in the doc, so they stay hidden. expect(container.querySelector('[data-thread-id="orphan-1"]')).toBeNull(); @@ -537,12 +597,11 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - - const order = Array.from(container.querySelectorAll("[data-thread-id]")) - .map((el) => el.getAttribute("data-thread-id")); - expect(order).toEqual(["thread-early", "thread-mid", "thread-late"]); + await waitFor(() => { + const order = Array.from(container.querySelectorAll("[data-thread-id]")) + .map((el) => el.getAttribute("data-thread-id")); + expect(order).toEqual(["thread-early", "thread-mid", "thread-late"]); + }); }); it("renders author name + role from agent and user maps", async () => { @@ -615,12 +674,11 @@ describe("IssueDocumentAnnotations", () => { , ); }); - await flush(); - await flush(); - // Click the open thread to expand it. + await waitFor(() => { + expect(container.querySelector('[data-thread-id="open-1"]')).not.toBeNull(); + }); const threadCard = container.querySelector('[data-thread-id="open-1"]') as HTMLElement | null; - expect(threadCard).not.toBeNull(); await act(async () => threadCard!.click()); await flush(); diff --git a/ui/src/components/IssueDocumentAnnotations.tsx b/ui/src/components/IssueDocumentAnnotations.tsx index dcc7bb5d62..d7b1beeb40 100644 --- a/ui/src/components/IssueDocumentAnnotations.tsx +++ b/ui/src/components/IssueDocumentAnnotations.tsx @@ -42,6 +42,8 @@ export interface IssueDocumentAnnotationsProps { /** Controlled panel state. Caller owns this so the count chip can live in the doc header. */ panelOpen: boolean; onPanelOpenChange: (open: boolean) => void; + /** Keep the panel in document flow for narrow hosts such as the task properties pane. */ + panelPlacement?: "floating" | "inline"; agentMap?: ReadonlyMap & Partial>>; userProfileMap?: ReadonlyMap; /** Seed which thread is focused on mount. Used by Storybook/screenshot harness. */ @@ -66,6 +68,7 @@ export function IssueDocumentAnnotations({ locationHash, panelOpen, onPanelOpenChange, + panelPlacement = "floating", agentMap, userProfileMap, defaultFocusedThreadId, @@ -104,7 +107,7 @@ export function IssueDocumentAnnotations({ }, []); useEffect(() => { - if (!panelOpen || isMobile || typeof window === "undefined") { + if (!panelOpen || panelPlacement === "inline" || isMobile || typeof window === "undefined") { setDesktopPanelFrame(null); return; } @@ -169,7 +172,7 @@ export function IssueDocumentAnnotations({ window.removeEventListener("scroll", updatePanelFrame, true); resizeObserver?.disconnect(); }; - }, [doc.key, isMobile, panelOpen]); + }, [doc.key, isMobile, panelOpen, panelPlacement]); const annotationsQuery = useQuery({ queryKey: target?.kind === "routine" @@ -284,7 +287,7 @@ export function IssueDocumentAnnotations({ ); const fallbackDesktopPanelFrame = useMemo(() => { - if (!panelOpen || isMobile || desktopPanelFrame || typeof window === "undefined") return null; + if (!panelOpen || panelPlacement === "inline" || isMobile || desktopPanelFrame || typeof window === "undefined") return null; const width = Math.min( DESKTOP_ANNOTATION_PANEL_WIDTH, Math.max( @@ -304,7 +307,7 @@ export function IssueDocumentAnnotations({ ), width, }; - }, [desktopPanelFrame, isMobile, panelOpen]); + }, [desktopPanelFrame, isMobile, panelOpen, panelPlacement]); const renderedDesktopPanelFrame = desktopPanelFrame ?? fallbackDesktopPanelFrame; const annotationPanel = panelOpen ? ( @@ -338,6 +341,7 @@ export function IssueDocumentAnnotations({ newCommentDisabled={newCommentDisabled} newCommentDisabledReason={newCommentDisabledReason} isMobile={isMobile} + inline={panelPlacement === "inline"} desktopWidth={renderedDesktopPanelFrame?.width} agentMap={agentMap} userProfileMap={userProfileMap} @@ -374,6 +378,11 @@ export function IssueDocumentAnnotations({ /> ) : null} + {panelOpen && panelPlacement === "inline" && !isMobile ? ( +
+ {annotationPanel} +
+ ) : null} {panelOpen && !isMobile && renderedDesktopPanelFrame ? (
({ const mockIssuesApi = vi.hoisted(() => ({ list: vi.fn(), + getDocument: vi.fn(), + listAcceptedPlanDecompositions: vi.fn(), + listAttachments: vi.fn(), + listInteractions: vi.fn(), listLabels: vi.fn(), createLabel: vi.fn(), upsertWatchdog: vi.fn(), @@ -136,6 +140,7 @@ vi.mock("./AgentIconPicker", () => ({ vi.mock("@/lib/router", () => ({ Link: ({ children, to, ...props }: { children: ReactNode; to: string } & ComponentProps<"a">) => {children}, useCaseHref: () => (caseId: string) => `/cases/${caseId}`, + useLocation: () => ({ hash: "", pathname: "/", search: "", state: null, key: "test" }), })); vi.mock("@/components/ui/separator", () => ({ @@ -435,6 +440,10 @@ describe("IssueProperties", () => { mockProjectsApi.list.mockResolvedValue([]); mockExecutionWorkspacesApi.controlRuntimeCommands.mockReset(); mockIssuesApi.list.mockResolvedValue([]); + mockIssuesApi.getDocument.mockResolvedValue(null); + mockIssuesApi.listAcceptedPlanDecompositions.mockResolvedValue([]); + mockIssuesApi.listAttachments.mockResolvedValue([]); + mockIssuesApi.listInteractions.mockResolvedValue([]); mockIssuesApi.listLabels.mockResolvedValue([]); mockIssuesApi.createLabel.mockResolvedValue(createLabel({ id: "label-new", @@ -468,6 +477,43 @@ describe("IssueProperties", () => { document.body.innerHTML = ""; }); + it("keeps the Plan tab visible for a planning-mode issue without a plan document", async () => { + mockInstanceSettingsApi.getExperimental.mockResolvedValue({ + enableTaskWatchdogs: false, + enableTaskChatRedesign: true, + }); + mockIssuesApi.listInteractions.mockResolvedValue([ + { + kind: "request_confirmation", + status: "pending", + payload: { target: { type: "issue_document", key: "plan" } }, + }, + ]); + const root = renderProperties(container, { + issue: createIssue({ workMode: "planning" }), + childIssues: [], + onUpdate: vi.fn(), + inline: true, + }); + + await waitForAssertion(() => { + expect(Array.from(container.querySelectorAll("button")).some((button) => button.textContent === "Plan")).toBe(true); + }); + + const planTab = Array.from(container.querySelectorAll("button")).find((button) => button.textContent === "Plan"); + await act(async () => { + // Radix Tabs triggers select on mousedown (button 0), not on click. + planTab!.dispatchEvent(new MouseEvent("mousedown", { bubbles: true, button: 0 })); + }); + + await waitForAssertion(() => { + expect(container.textContent).toContain("This task is in plan mode but no plan document has been written yet."); + expect(container.textContent).toContain("A plan confirmation is pending, but the plan document it should confirm is missing."); + }); + + act(() => root.unmount()); + }); + it("shows assignee and originating without responsible wording", async () => { mockAgentsApi.list.mockResolvedValue([{ id: "agent-1", name: "CodexCoder", status: "active", adapterType: "codex_local" }]); const root = renderProperties(container, { diff --git a/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.test.tsx b/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.test.tsx new file mode 100644 index 0000000000..9293348316 --- /dev/null +++ b/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.test.tsx @@ -0,0 +1,117 @@ +// @vitest-environment jsdom + +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { MemoryRouter } from "react-router-dom"; +import type { Issue, RequestConfirmationInteraction } from "@paperclipai/shared"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { queryKeys } from "@/lib/queryKeys"; +import { IssuePropertiesPlansTab } from "./IssuePropertiesPlansTab"; +import { IssuePlanConfirmationActionBar } from "./IssuePlanConfirmationActionBar"; + +const mockIssuesApi = vi.hoisted(() => ({ + listInteractions: vi.fn(), + listAcceptedPlanDecompositions: vi.fn(), + acceptInteraction: vi.fn(), + rejectInteraction: vi.fn(), +})); + +vi.mock("@/api/issues", () => ({ issuesApi: mockIssuesApi })); +vi.mock("@/hooks/useIssuePlanDocument", () => ({ + useIssuePlanDocument: () => ({ data: undefined, isLoading: false }), +})); +vi.mock("../PropertiesPanel", () => ({ + PROPERTIES_PANE_FOOTER_SLOT_ID: "properties-pane-footer-slot", +})); + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +const issue = { + id: "issue-1", + identifier: "PAP-1", +} as Issue; + +const confirmation = { + id: "confirmation-1", + companyId: "company-1", + issueId: issue.id, + kind: "request_confirmation", + status: "pending", + continuationPolicy: "wake_assignee", + resolverPolicy: "board_only", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + createdAt: "2026-08-05T00:00:00.000Z", + updatedAt: "2026-08-05T00:00:00.000Z", + payload: { + version: 1, + prompt: "Approve this plan?", + acceptLabel: "Approve plan", + rejectLabel: "Request changes", + rejectRequiresReason: true, + allowDeclineReason: true, + target: { type: "issue_document", key: "plan", revisionId: "rev-1" }, + }, +} satisfies RequestConfirmationInteraction; + +let root: ReturnType | null = null; +let container: HTMLDivElement | null = null; +let client: QueryClient | null = null; + +function render(element: React.ReactElement) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + client = new QueryClient({ defaultOptions: { queries: { retry: false, staleTime: Infinity } } }); + client.setQueryData(queryKeys.issues.interactions(issue.id), [confirmation]); + client.setQueryData(queryKeys.issues.acceptedPlanDecompositions(issue.id), []); + act(() => root?.render( + + {element} + , + )); + return container; +} + +afterEach(() => { + if (root) act(() => root?.unmount()); + root = null; + container?.remove(); + container = null; + client?.clear(); + client = null; + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe("IssuePlanConfirmationActionBar", () => { + it("renders a pending plan confirmation even before its plan document exists", () => { + const rendered = render(); + + expect(rendered.querySelector('[data-testid="plan-pane-action-bar"]')).not.toBeNull(); + expect(rendered.textContent).toContain("Approve plan"); + expect(rendered.textContent).toContain("Request changes"); + }); + + it("moves into a footer slot that mounts on the next paint", () => { + let resolveNextPaint: FrameRequestCallback | undefined; + vi.stubGlobal("requestAnimationFrame", vi.fn((callback: FrameRequestCallback) => { + resolveNextPaint = callback; + return 1; + })); + vi.stubGlobal("cancelAnimationFrame", vi.fn()); + + const rendered = render(); + const footer = document.createElement("div"); + footer.id = "properties-pane-footer-slot"; + document.body.appendChild(footer); + + act(() => resolveNextPaint?.(0)); + + expect(rendered.querySelector('[data-testid="plan-pane-action-bar"]')).toBeNull(); + expect(footer.querySelector('[data-testid="plan-pane-action-bar"]')).not.toBeNull(); + footer.remove(); + }); +}); diff --git a/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.tsx b/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.tsx index e679989f08..e3da084106 100644 --- a/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.tsx +++ b/ui/src/components/issue-properties/IssuePlanConfirmationActionBar.tsx @@ -51,8 +51,18 @@ export function IssuePlanConfirmationActionBar({ issue, inline }: IssuePlanConfi setFooterSlot(null); return; } - setFooterSlot(document.getElementById(PROPERTIES_PANE_FOOTER_SLOT_ID)); - }, [inline]); + + const resolveFooterSlot = () => { + setFooterSlot(document.getElementById(PROPERTIES_PANE_FOOTER_SLOT_ID)); + }; + + // The properties pane and this action bar can mount in either order. Check + // once synchronously, then again on the next paint so the footer slot is + // found when it is mounted later in the same commit. + resolveFooterSlot(); + const frame = requestAnimationFrame(resolveFooterSlot); + return () => cancelAnimationFrame(frame); + }, [confirmation?.id, inline]); const [rejecting, setRejecting] = useState(false); const [rejectReason, setRejectReason] = useState(""); @@ -113,7 +123,9 @@ export function IssuePlanConfirmationActionBar({ issue, inline }: IssuePlanConfi onChange={(event) => setRejectReason(event.target.value)} placeholder={ confirmation.payload.declineReasonPlaceholder - ?? "Optional: what would you like revised?" + ?? (confirmation.payload.acceptLabel === "Approve plan" + ? "Optional: what would you like revised?" + : "Optional: tell the agent what you'd change.") } aria-invalid={rejectAttempted && reasonInvalid} className={cn( diff --git a/ui/src/components/issue-properties/IssueProperties.tsx b/ui/src/components/issue-properties/IssueProperties.tsx index b63581c329..45a115beb7 100644 --- a/ui/src/components/issue-properties/IssueProperties.tsx +++ b/ui/src/components/issue-properties/IssueProperties.tsx @@ -186,8 +186,9 @@ export function IssueProperties({ } setPaneHeaderSlot(document.getElementById(PROPERTIES_PANE_HEADER_SLOT_ID)); }, [taskChatRedesignEnabled, inline]); - // Plan/Artifacts only earn a tab when they have content; with neither, the - // header bar shows a plain "Properties" title instead of a one-tab strip. + // Plan earns a tab as soon as an issue is in planning mode, even before the + // plan document arrives. This keeps an expected plan surface visible and + // lets its diagnostic empty state explain what is missing. // Same query keys as the tab bodies, so these share their cached fetches. const { data: paneTabPlanDocument } = useIssuePlanDocument( taskChatRedesignEnabled ? issue.id : null, @@ -202,7 +203,10 @@ export function IssueProperties({ queryFn: () => issuesApi.listAttachments(issue.id), enabled: taskChatRedesignEnabled, }); - const hasPlanTab = Boolean(paneTabPlanDocument) || (paneTabAcceptedPlans?.length ?? 0) > 0; + const hasPlanTab = + Boolean(paneTabPlanDocument) + || (paneTabAcceptedPlans?.length ?? 0) > 0 + || issue.workMode === "planning"; const hasArtifactsTab = (paneTabAttachments?.length ?? 0) > 0; const [paneTab, setPaneTab] = useState("properties"); const [assigneeOpen, setAssigneeOpen] = useState(false); diff --git a/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx b/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx index 0e40995f68..db42607c6a 100644 --- a/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesPlansTab.tsx @@ -1,10 +1,13 @@ +import { useState } from "react"; import { useQuery } from "@tanstack/react-query"; -import type { Issue } from "@paperclipai/shared"; +import type { Issue, IssueThreadInteraction } from "@paperclipai/shared"; import { issuesApi } from "@/api/issues"; import { queryKeys } from "@/lib/queryKeys"; import { IssuePlanDecompositionsSection } from "@/components/IssuePlanDecompositionsSection"; import { MarkdownBody } from "@/components/MarkdownBody"; +import { DocumentAnnotationsCountChip, IssueDocumentAnnotations } from "@/components/IssueDocumentAnnotations"; import { useIssuePlanDocument } from "@/hooks/useIssuePlanDocument"; +import { useLocation } from "@/lib/router"; import { IssuePlanConfirmationActionBar } from "./IssuePlanConfirmationActionBar"; interface IssuePropertiesPlansTabProps { @@ -14,6 +17,16 @@ interface IssuePropertiesPlansTabProps { inline?: boolean; } +function hasPendingPlanConfirmation(interactions: IssueThreadInteraction[] | undefined): boolean { + return (interactions ?? []).some( + (interaction) => + interaction.kind === "request_confirmation" + && interaction.status === "pending" + && interaction.payload.target?.type === "issue_document" + && interaction.payload.target.key === "plan", + ); +} + /** * Plans tab of the redesigned properties pane (flag: enableTaskChatRedesign). * @@ -25,19 +38,42 @@ interface IssuePropertiesPlansTabProps { */ export function IssuePropertiesPlansTab({ issue, inline }: IssuePropertiesPlansTabProps) { const { data: planDocument, isLoading: planDocumentLoading } = useIssuePlanDocument(issue.id); + const location = useLocation(); + const [annotationPanelOpen, setAnnotationPanelOpen] = useState(false); const { data } = useQuery({ queryKey: queryKeys.issues.acceptedPlanDecompositions(issue.id), queryFn: () => issuesApi.listAcceptedPlanDecompositions(issue.id), }); + const { data: interactions } = useQuery({ + queryKey: queryKeys.issues.interactions(issue.id), + queryFn: () => issuesApi.listInteractions(issue.id), + }); const hasPlans = (data?.length ?? 0) > 0; + const pendingPlanConfirmation = hasPendingPlanConfirmation(interactions); if (!planDocument && !hasPlans) { return ( -
- {planDocumentLoading - ? "Loading plan…" - : "No plan yet. The plan document, accepted plans, and their revisions will appear here."} -
+ <> + {/* This is deliberately outside the plan-document gate: an interaction + can arrive before its plan document query resolves or persists. */} + +
+ {planDocumentLoading ? ( + "Loading plan…" + ) : issue.workMode === "planning" ? ( +
+

This task is in plan mode but no plan document has been written yet.

+ {pendingPlanConfirmation ? ( +

+ A plan confirmation is pending, but the plan document it should confirm is missing. +

+ ) : null} +
+ ) : ( + "No plan yet. The plan document, accepted plans, and their revisions will appear here." + )} +
+ ); } @@ -45,18 +81,41 @@ export function IssuePropertiesPlansTab({ issue, inline }: IssuePropertiesPlansT
{/* Pending plan confirmation: its CTAs pin to the pane's footer slot so they stay visible while the plan scrolls. */} - {planDocument ? : null} + {planDocument ? (
-
+
{`Revision ${planDocument.latestRevisionNumber ?? 1} · updated ${new Date(planDocument.updatedAt).toLocaleString([], { month: "short", day: "numeric", hour: "numeric", minute: "2-digit", })}`} + setAnnotationPanelOpen((open) => !open)} + />
- {planDocument.body} + + {planDocument.body} +
) : null} {hasPlans ? (