diff --git a/packages/bb-plugin-plannotator/README.md b/packages/bb-plugin-plannotator/README.md index f59af7e..4ab563f 100644 --- a/packages/bb-plugin-plannotator/README.md +++ b/packages/bb-plugin-plannotator/README.md @@ -21,6 +21,8 @@ For development, offline use, or an independently updated upstream build, set ** Start a new agent session after installing or reloading. When the agent explicitly calls `plannotator_review_plan`, BB opens the upstream UI as a persistent right-panel tab. The tool returns a review ID promptly, and the agent ends its turn. Approve or annotate in the tab. BB then closes it and sends the decision as a new message in the same thread, which wakes the agent. The message is queued if the agent is still working. There is no BB review deadline or hidden interaction countdown: the review remains open until Plannotator returns a decision or you use **Cancel review** in the tab. Plannotator is optional, and its approval is not a permission grant. The child receives the current BB provider identity explicitly, rather than inferring it from unrelated host environment variables. +Only one review can be pending per thread. If you close its tab, open the **Plannotator** thread panel action to bring the pending review back. A second `plannotator_review_plan` call while a review is pending returns `status: "already_active"` with the pending review ID; it does not start a new review and is not a cancellation. + BB stores the review state until it delivers the decision. If the plugin restarts during a review, it sends a cancellation message after restarting. If delivery fails temporarily, it retries. The agent can call `plannotator_review_status` with the review ID to recover a missing result; it does not need to poll during a normal review. Plannotator's plan history and configuration are stored under the plugin's BB data directory. Local child sessions use BB's session-scoped same-origin relay, and BB acknowledges the current upstream look-and-feel announcement before mounting the iframe, so the standalone setup wizard does not interrupt each review. The upstream plan UI remains otherwise unmodified. **Open externally** is available as a fallback, but external browser tabs may have a separate cookie jar from the BB panel. diff --git a/packages/bb-plugin-plannotator/app.tsx b/packages/bb-plugin-plannotator/app.tsx index 9bd9bc1..0671e41 100644 --- a/packages/bb-plugin-plannotator/app.tsx +++ b/packages/bb-plugin-plannotator/app.tsx @@ -72,6 +72,67 @@ function parseReviewOpened(value: unknown): PlannotatorPayload | null { return parsePayload(value.payload); } +function parseReviewClosed( + value: unknown, +): { threadId: string; sessionId: string } | null { + if ( + !isRecord(value) || + value.kind !== "review-closed" || + typeof value.threadId !== "string" || + typeof value.sessionId !== "string" + ) { + return null; + } + return { threadId: value.threadId, sessionId: value.sessionId }; +} + +/** + * A panel opened from the thread action carries no params. Recover the + * thread's pending review from the server so a review whose tab was closed + * can still be approved, annotated, or cancelled. + */ +function useActiveReviewFallback( + threadId: string, + enabled: boolean, +): PlannotatorPayload | null { + const rpc = useRpc(); + const realtimeConnectionState = useRealtimeConnectionState(); + const [active, setActive] = useState(null); + + useRealtime(PLANNOTATOR_REALTIME_CHANNEL, (value) => { + if (!enabled) return; + const opened = parseReviewOpened(value); + if (opened && opened.threadId === threadId) { + setActive(opened); + return; + } + const closed = parseReviewClosed(value); + if (closed && closed.threadId === threadId) { + setActive((current) => + current?.sessionId === closed.sessionId ? null : current, + ); + } + }); + + useEffect(() => { + if (!enabled || realtimeConnectionState !== "connected") return; + let cancelled = false; + void rpc + .call("getActiveReview", { threadId }) + .then((value) => { + if (cancelled) return; + const payload = parsePayload(value); + setActive(payload && payload.threadId === threadId ? payload : null); + }) + .catch(() => {}); + return () => { + cancelled = true; + }; + }, [enabled, realtimeConnectionState, rpc, threadId]); + + return enabled ? active : null; +} + /** * Server-side tool calls can persist a tab, but only the app navigation API * can focus/open the side panel. This deliberately renders no control: it is @@ -139,7 +200,9 @@ function EmptyPanel() { } function PlannotatorPanel({ threadId, params }: PluginThreadPanelProps) { - const payload = parsePayload(params); + const paramsPayload = parsePayload(params); + const fallbackPayload = useActiveReviewFallback(threadId, !paramsPayload); + const payload = paramsPayload ?? fallbackPayload; const rpc = useRpc(); const [reloadKey, setReloadKey] = useState(0); const [loaded, setLoaded] = useState(false); @@ -165,6 +228,7 @@ function PlannotatorPanel({ threadId, params }: PluginThreadPanelProps) { // browser-facing hostname. This keeps the real upstream UI while removing // standalone-browser onboarding from the embedded workflow. useLayoutEffect(() => { + setCanceling(false); if (!payload) { setEmbeddedUrl(null); return; @@ -224,6 +288,9 @@ function PlannotatorPanel({ threadId, params }: PluginThreadPanelProps) { setCanceling(true); void rpc .call("cancelReview", { threadId, sessionId: payload.sessionId }) + .then((result) => { + if (!result.cancelled) setCanceling(false); + }) .catch(() => setCanceling(false)); }} > diff --git a/packages/bb-plugin-plannotator/package.json b/packages/bb-plugin-plannotator/package.json index fb6cc08..50adca9 100644 --- a/packages/bb-plugin-plannotator/package.json +++ b/packages/bb-plugin-plannotator/package.json @@ -1,6 +1,6 @@ { "name": "bb-plugin-plannotator", - "version": "0.3.0", + "version": "0.3.1", "type": "module", "engines": { "bb": ">=0.39", diff --git a/packages/bb-plugin-plannotator/server.ts b/packages/bb-plugin-plannotator/server.ts index 4ffabb0..598f305 100644 --- a/packages/bb-plugin-plannotator/server.ts +++ b/packages/bb-plugin-plannotator/server.ts @@ -336,6 +336,24 @@ function errorResponse(message: string) { }; } +// A refusal is not a review outcome. Report it with a distinct status so an +// agent cannot mistake it for the user cancelling the pending review. +function alreadyActiveResponse(reviewId: string | undefined) { + return { + content: [{ + type: "text" as const, + text: JSON.stringify({ + status: "already_active", + source: "plannotator", + ...(reviewId ? { reviewId } : {}), + error: "A Plannotator review is already pending in this thread. No new review was started.", + message: "Ask the user to finish or cancel the pending review. They can reopen it from the Plannotator thread panel action. Its decision will arrive as a new message in this thread.", + }), + }], + isError: true, + }; +} + async function getConfiguredPath(settings: { get(): Promise<{ binaryPath: string }> }): Promise { const configured = await settings.get(); return configured.binaryPath.trim() || BUNDLED_BINARY; @@ -494,11 +512,21 @@ export default async function plugin(bb: BbPluginApi) { } relaySessions.delete(active.sessionId); await closeReviewPanel(bb, threadId, active.sessionId); + // Panels opened manually from the thread action carry no session params, + // so closeReviewPanel cannot find them. Tell them the review ended. + bb.realtime.publish(PLANNOTATOR_REALTIME_CHANNEL, { + kind: "review-closed", + threadId, + sessionId: active.sessionId, + }); try { await active.review.stop(); } catch (error) { bb.log.warn(`Could not stop Plannotator review ${active.sessionId}: ${String(error)}`); } + // The review is over once the upstream process ends. Free the thread now + // so a slow or retried decision delivery does not block the next review. + if (activeReviews.get(threadId) === active) activeReviews.delete(threadId); if (disposing || active.deleted) return; const record: StoredReview = { id: active.sessionId, @@ -513,7 +541,6 @@ export default async function plugin(bb: BbPluginApi) { bb.log.warn(`Could not save Plannotator decision ${record.id}: ${String(error)}`); } await deliverReview(record); - if (activeReviews.get(threadId) === active) activeReviews.delete(threadId); } registerPlannotatorRelayRoutes(bb, relaySessions); @@ -558,7 +585,7 @@ export default async function plugin(bb: BbPluginApi) { parameters: reviewToolParametersSchema, async execute(params, context) { if (activeReviews.has(context.threadId) || startingReviews.has(context.threadId)) { - return errorResponse("A Plannotator review is already active in this thread."); + return alreadyActiveResponse(activeReviews.get(context.threadId)?.sessionId); } startingReviews.add(context.threadId); diff --git a/packages/bb-plugin-plannotator/skills/plan-review/SKILL.md b/packages/bb-plugin-plannotator/skills/plan-review/SKILL.md index 7f11974..637328d 100644 --- a/packages/bb-plugin-plannotator/skills/plan-review/SKILL.md +++ b/packages/bb-plugin-plannotator/skills/plan-review/SKILL.md @@ -31,6 +31,10 @@ The decision message has one of these results: - `decision=cancelled`: report the cancellation, then continue normally unless the user specifically asked to stop. +If the tool returns `status=already_active`, no new review started and nothing +was cancelled. Tell the user a review is still pending (give its `reviewId`) +and that they can reopen it from the Plannotator thread panel action. + Do not claim a review decision from the presence of a panel or the pending tool result alone. Only the later decision message describes what happened in Plannotator; it does not authorize or prohibit file changes. diff --git a/packages/bb-plugin-plannotator/src/app.test.tsx b/packages/bb-plugin-plannotator/src/app.test.tsx index b665a4d..22439e7 100644 --- a/packages/bb-plugin-plannotator/src/app.test.tsx +++ b/packages/bb-plugin-plannotator/src/app.test.tsx @@ -144,6 +144,63 @@ describe("Plannotator BB shell", () => { ); }); + it("recovers the pending review when the panel action opens without params", async () => { + const slot = renderSlot( + app.threadPanelActions[0]!, + { threadId: "thread-1", params: {} }, + { + rpc: { + getActiveReview: () => payload, + cancelReview: () => ({ cancelled: true }), + }, + }, + ); + + const iframe = await slot.findByTitle("Plannotator review"); + expect(iframe.getAttribute("src")).toContain("sessionId=session-1"); + expect(slot.rpcCalls).toContainEqual({ + method: "getActiveReview", + input: { threadId: "thread-1" }, + }); + expect(slot.queryByText(/Start a plan review from the agent/)).toBeNull(); + + fireEvent.click(slot.getByRole("button", { name: "Cancel review" })); + await waitFor(() => + expect(slot.rpcCalls).toContainEqual({ + method: "cancelReview", + input: { threadId: "thread-1", sessionId: "session-1" }, + }), + ); + }); + + it("follows review open and close signals in a param-less panel", async () => { + const slot = renderSlot( + app.threadPanelActions[0]!, + { threadId: "thread-1", params: {} }, + { rpc: { getActiveReview: () => null } }, + ); + + await slot.findByText(/Start a plan review from the agent/); + await slot.behavior.emitRealtime(PLANNOTATOR_REALTIME_CHANNEL, { + kind: "review-opened", + payload: { ...payload, threadId: "thread-2" }, + }); + expect(slot.queryByTitle("Plannotator review")).toBeNull(); + + await slot.behavior.emitRealtime(PLANNOTATOR_REALTIME_CHANNEL, { + kind: "review-opened", + payload, + }); + await slot.findByTitle("Plannotator review"); + + await slot.behavior.emitRealtime(PLANNOTATOR_REALTIME_CHANNEL, { + kind: "review-closed", + threadId: "thread-1", + sessionId: "session-1", + }); + await slot.findByText(/Start a plan review from the agent/); + }); + it("exposes explicit cancellation without a host interaction deadline", async () => { const slot = renderSlot( app.threadPanelActions[0]!, diff --git a/packages/bb-plugin-plannotator/src/server.test.ts b/packages/bb-plugin-plannotator/src/server.test.ts index b1908c3..31d1453 100644 --- a/packages/bb-plugin-plannotator/src/server.test.ts +++ b/packages/bb-plugin-plannotator/src/server.test.ts @@ -304,6 +304,108 @@ describe("BB upstream Plannotator bridge", () => { }); }); + it("refuses a second review with a distinct status instead of cancelled", async () => { + const binary = await fakePlannotatorBinary({ approveAfterMs: null }); + const host = createFakePluginHost({ + pluginId: "plannotator", + settings: { binaryPath: binary }, + }); + hosts.push(host); + await plugin(host.bb); + wireInteractionSdk(host); + + const first = JSON.parse(String(await host.harness.behavior.callAgentTool( + "plannotator_review_plan", + { planMarkdown: "# Plan\n\n- First" }, + { threadId: "thread-1", projectId: "project-1" }, + ))) as { status: string; reviewId: string }; + expect(first.status).toBe("pending"); + + const refusal = (await host.harness.behavior.callAgentTool( + "plannotator_review_plan", + { planMarkdown: "# Plan\n\n- Second" }, + { threadId: "thread-1", projectId: "project-1" }, + )) as { isError: boolean; content: Array<{ text: string }> }; + expect(refusal.isError).toBe(true); + const second = JSON.parse(refusal.content[0]!.text) as Record; + expect(second).toMatchObject({ + status: "already_active", + source: "plannotator", + reviewId: first.reviewId, + }); + expect(second).not.toHaveProperty("decision"); + + // The pending review is still recoverable and cancellable from the panel. + const active = (await host.harness.callRpc("getActiveReview", { + threadId: "thread-1", + })) as { sessionId: string }; + expect(active.sessionId).toBe(first.reviewId); + await expect( + host.harness.callRpc("cancelReview", { + threadId: "thread-1", + sessionId: active.sessionId, + }), + ).resolves.toEqual({ cancelled: true }); + await sentReview(host); + await expect.poll(() => host.harness.realtimeSignals).toContainEqual({ + channel: PLANNOTATOR_REALTIME_CHANNEL, + payload: { + kind: "review-closed", + threadId: "thread-1", + sessionId: first.reviewId, + }, + }); + }); + + it("frees the thread for a new review while a decision is still being delivered", async () => { + const binary = await fakePlannotatorBinary({ approveAfterMs: null }); + const host = createFakePluginHost({ + pluginId: "plannotator", + settings: { binaryPath: binary }, + }); + hosts.push(host); + await plugin(host.bb); + wireInteractionSdk(host); + let releaseSend!: () => void; + const sendBlocked = new Promise((resolve) => { + releaseSend = resolve; + }); + host.harness.sdk.stub("threads.send", async () => { + await sendBlocked; + return { ok: true, delivery: "queued" }; + }); + + const first = JSON.parse(String(await host.harness.behavior.callAgentTool( + "plannotator_review_plan", + { planMarkdown: "# Plan\n\n- First" }, + { threadId: "thread-1", projectId: "project-1" }, + ))) as { reviewId: string }; + await host.harness.callRpc("cancelReview", { + threadId: "thread-1", + sessionId: first.reviewId, + }); + // The decision send is now in flight and has not returned. + await sentReview(host); + + const second = JSON.parse(String(await host.harness.behavior.callAgentTool( + "plannotator_review_plan", + { planMarkdown: "# Plan\n\n- Second" }, + { threadId: "thread-1", projectId: "project-1" }, + ))) as { status: string; reviewId: string }; + expect(second.status).toBe("pending"); + expect(second.reviewId).not.toBe(first.reviewId); + + releaseSend(); + const active = (await host.harness.callRpc("getActiveReview", { + threadId: "thread-1", + })) as { sessionId: string }; + expect(active.sessionId).toBe(second.reviewId); + await host.harness.callRpc("cancelReview", { + threadId: "thread-1", + sessionId: second.reviewId, + }); + }); + it("rejects cancellation from another thread or session", async () => { const binary = await fakePlannotatorBinary(); const host = createFakePluginHost({