From 1ddd6751d6e12e8fb27de76ff353643f18ad7daa Mon Sep 17 00:00:00 2001 From: Cyrus Gray <144336577+graycyrus@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:53:45 +0530 Subject: [PATCH] =?UTF-8?q?fix(flows):=20copilot=20proposals=20reach=20the?= =?UTF-8?q?=20canvas=20=E2=80=94=20recognition,=20race-guard,=20title=20(#?= =?UTF-8?q?4886)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/src/pages/FlowCanvasPage.tsx | 99 +++++- .../pages/__tests__/FlowCanvasPage.test.tsx | 284 ++++++++++++++++++ app/src/providers/ChatRuntimeProvider.tsx | 35 +-- .../__tests__/ChatRuntimeProvider.test.tsx | 82 +++++ app/src/store/chatRuntimeSlice.test.ts | 70 +++++ app/src/store/chatRuntimeSlice.ts | 11 +- 6 files changed, 550 insertions(+), 31 deletions(-) diff --git a/app/src/pages/FlowCanvasPage.tsx b/app/src/pages/FlowCanvasPage.tsx index 4123b9d95..cd81a4db3 100644 --- a/app/src/pages/FlowCanvasPage.tsx +++ b/app/src/pages/FlowCanvasPage.tsx @@ -158,6 +158,19 @@ function errorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } +/** + * True when `title` is "unclaimed" — either blank or exactly the localized + * generic placeholder (`t('flows.page.newWorkflow')`, "New workflow") — i.e. + * nothing user-meaningful has been set yet. Used to decide whether accepting + * a copilot proposal is allowed to adopt `proposal.name` as the flow title: + * it must never clobber a user-chosen or description-derived name. Pure and + * exported for direct unit testing without rendering `FlowEditor`. + */ +export function isPlaceholderTitle(title: string, placeholder: string): boolean { + const trimmed = title.trim(); + return trimmed === '' || trimmed === placeholder.trim(); +} + function BackIcon() { return ( (editorFlow.name); const commitRename = useCallback(async () => { const trimmed = titleDraft.trim(); @@ -311,6 +332,7 @@ function FlowEditor({ log('rename: flow id=%s name=%s', flowId, trimmed); await updateFlow(flowId, { name: trimmed }); setName(trimmed); + persistedNameRef.current = trimmed; } catch (err) { log('rename failed: id=%s err=%o', flowId, err); setTitleDraft(name); @@ -422,12 +444,44 @@ function FlowEditor({ [draftGraph] ); - const handleAcceptProposal = useCallback((proposal: WorkflowProposal) => { - log('copilot proposal accepted'); - setDraftGraph(proposal.graph as WorkflowGraph); - setPreview(null); - setCanvasVersion(v => v + 1); - }, []); + const handleAcceptProposal = useCallback( + (proposal: WorkflowProposal) => { + log('copilot proposal accepted'); + setDraftGraph(proposal.graph as WorkflowGraph); + setPreview(null); + setCanvasVersion(v => v + 1); + + // Adopt the proposal's name into the flow title, but ONLY while the + // title is still the generic placeholder — never clobber a user-chosen + // or description-derived meaningful name. This does not persist by + // itself (matching the "accept doesn't persist" invariant below) — it + // rides into the next Save via `name`/`handleSave`. + // + // Check the VISIBLE `titleDraft`, not the committed `name` — `name` + // only updates on blur/Enter via `commitRename`, so if the user is + // mid-typing a custom title (or a rename is still in flight) when a + // proposal is accepted, `name` can still read as the stale placeholder + // while `titleDraft` already holds the user's real input. Deciding off + // `name` would silently clobber that in-progress input. Also skip + // entirely while `renaming` is true — an in-flight `commitRename` + // persist must not race with a local proposal-driven rename. + const proposedName = proposal.name?.trim(); + if ( + proposedName && + !renaming && + isPlaceholderTitle(titleDraft, t('flows.page.newWorkflow')) + ) { + // Log shape, not the user-authored name (no PII in logs). + log( + 'copilot proposal accepted: adopting proposed name into placeholder title, isDraft=%s', + isDraft + ); + setName(proposedName); + setTitleDraft(proposedName); + } + }, + [titleDraft, renaming, t, isDraft] + ); const handleRejectProposal = useCallback(() => { log('copilot proposal rejected'); @@ -453,9 +507,15 @@ function FlowEditor({ () => ({ schema_version: graph.schema_version, id: flowId ?? undefined, name }), [graph.schema_version, flowId, name] ); + // Also dirty when a copilot-adopted proposal name has changed the flow's + // `name` without yet persisting it (`persistedNameRef` only advances on a + // real Save/rename) — a name-only proposal (same graph, new name) must + // still enable Save, or the adopted title can never be persisted. const initialDirty = useMemo( - () => JSON.stringify(editorGraph) !== JSON.stringify(persistedGraphRef.current), - [editorGraph] + () => + JSON.stringify(editorGraph) !== JSON.stringify(persistedGraphRef.current) || + name !== persistedNameRef.current, + [editorGraph, name] ); // Repair seed for the copilot: bind the run context to the CURRENT draft. @@ -503,11 +563,30 @@ function FlowEditor({ navigate(`/flows/${created.id}`, { replace: true }); return; } - log('save: flow id=%s nodes=%d edges=%d', flowId, next.nodes.length, next.edges.length); - const updated = await updateFlow(flowId, { graph: next }); + // Only include `name` in the update payload when it actually diverges + // from the last-known-persisted baseline (a manual rename already + // persisted it via `commitRename`; a copilot-adopted placeholder name + // has not) — keeps the update metadata-safe and avoids needless renames. + const nameChanged = name !== persistedNameRef.current; + log( + 'save: flow id=%s nodes=%d edges=%d nameChanged=%s', + flowId, + next.nodes.length, + next.edges.length, + nameChanged + ); + const updated = await updateFlow(flowId, { graph: next, ...(nameChanged ? { name } : {}) }); const persisted = updated.graph as WorkflowGraph; persistedGraphRef.current = persisted; + persistedNameRef.current = updated.name; setDraftGraph(persisted); + if (updated.name !== name) { + // Re-sync BOTH title states from the response — leaving `titleDraft` + // stale would show the pre-save value in the input and could + // resubmit it verbatim on a later blur. + setName(updated.name); + setTitleDraft(updated.name); + } setCanvasVersion(v => v + 1); log( 'save: flow id=%s persisted — canvas re-synced from response nodes=%d edges=%d', diff --git a/app/src/pages/__tests__/FlowCanvasPage.test.tsx b/app/src/pages/__tests__/FlowCanvasPage.test.tsx index 0303e31bb..62c416ce8 100644 --- a/app/src/pages/__tests__/FlowCanvasPage.test.tsx +++ b/app/src/pages/__tests__/FlowCanvasPage.test.tsx @@ -10,10 +10,12 @@ import { createMemoryRouter, MemoryRouter, Route, RouterProvider, Routes } from import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { Flow } from '../../services/api/flowsApi'; +import type { WorkflowProposal } from '../../store/chatRuntimeSlice'; import FlowCanvasPage, { asCopilotBuildSeed, asCopilotPrefillSeed, FlowCanvasDraftPage, + isPlaceholderTitle, } from '../FlowCanvasPage'; const getFlow = vi.hoisted(() => vi.fn()); @@ -388,6 +390,288 @@ describe('FlowCanvasPage', () => { }); }); +describe('isPlaceholderTitle', () => { + it('treats an empty or whitespace-only title as a placeholder', () => { + expect(isPlaceholderTitle('', 'New workflow')).toBe(true); + expect(isPlaceholderTitle(' ', 'New workflow')).toBe(true); + }); + + it('treats the localized generic placeholder as a placeholder', () => { + expect(isPlaceholderTitle('New workflow', 'New workflow')).toBe(true); + expect(isPlaceholderTitle(' New workflow ', 'New workflow')).toBe(true); + }); + + it('does not treat a user-chosen or description-derived name as a placeholder', () => { + expect(isPlaceholderTitle('My flow', 'New workflow')).toBe(false); + expect(isPlaceholderTitle('Standup reminder', 'New workflow')).toBe(false); + }); +}); + +// ----------------------------------------------------------------------------- +// Copilot proposal name adoption — accepting a `propose_workflow` proposal +// carries a top-level `name` the canvas previously dropped, leaving the flow +// titled the generic placeholder even when the agent proposed a real name. +// ----------------------------------------------------------------------------- +function makeProposal(overrides: Partial = {}): WorkflowProposal { + return { + name: 'Standup reminder', + graph: { + schema_version: 1, + name: 'Standup reminder', + nodes: [ + { + id: 't', + kind: 'trigger', + name: 'Start', + config: {}, + ports: [], + position: { x: 0, y: 0 }, + }, + { + id: 'a', + kind: 'agent', + name: 'Send reminder', + config: {}, + ports: [], + position: { x: 80, y: 80 }, + }, + ], + edges: [], + }, + requireApproval: false, + summary: { trigger: 'manual', steps: [] }, + ...overrides, + }; +} + +describe('FlowCanvasPage copilot proposal name adoption', () => { + beforeEach(() => { + copilotPanelProps.current = null; + getFlow.mockReset(); + updateFlow.mockReset(); + createFlow.mockReset(); + validateFlow.mockReset(); + listFlowConnections.mockReset(); + validateFlow.mockResolvedValue({ valid: true, errors: [], warnings: [] }); + listFlowConnections.mockResolvedValue([]); + }); + + function renderEditor(id = 'test-id') { + return render( + + + } /> + Flows list} /> + + + ); + } + + it('adopts the proposal name when the title is the generic placeholder', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'New workflow' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('New workflow'); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + }); + + it('adopts the proposal name when the title is blank', async () => { + getFlow.mockResolvedValue(makeFlow({ name: '' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + }); + + it('does not clobber a user-set title when accepting a proposal', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'My flow' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + + // Give any (incorrect) state update a chance to flush, then assert the + // title is unchanged. + await Promise.resolve(); + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('My flow'); + }); + + // Regression (CodeRabbit on #4886): the committed `name` only updates on + // blur/Enter (`commitRename`), so while the user is still typing a custom + // title the committed `name` can read as the stale placeholder even though + // the visible input already holds real user input. Adoption must check the + // VISIBLE `titleDraft`, or it clobbers in-progress typing. + it('does not clobber an in-progress (uncommitted) title edit when accepting a proposal', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'New workflow' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + // User is mid-typing a custom title — not yet committed via blur/Enter. + fireEvent.change(screen.getByTestId('flow-canvas-title'), { + target: { value: 'My in-progress title' }, + }); + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('My in-progress title'); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + + await Promise.resolve(); + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('My in-progress title'); + }); + + it('includes the adopted name in the flows_update payload on Save (persisted flow)', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'New workflow' })); + updateFlow.mockResolvedValue(makeFlow({ name: 'Standup reminder' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + + fireEvent.click(screen.getByTestId('flow-editor-save')); + fireEvent.click(screen.getByTestId('flow-action-confirm-accept')); + + await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1)); + const [calledId, update] = updateFlow.mock.calls[0]; + expect(calledId).toBe('test-id'); + expect(update.name).toBe('Standup reminder'); + expect(update.graph).toBeDefined(); + }); + + // Regression (CodeRabbit on #4886): accepting a proposal that changes only + // the top-level `name` (graph unchanged) previously left the editor's dirty + // state false — since the graph-only diff saw no change — so Save stayed + // disabled and the adopted title could never be persisted. + it('marks the editor dirty when an accepted proposal changes only the name', async () => { + const flow = makeFlow({ name: 'New workflow' }); + getFlow.mockResolvedValue(flow); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + // Clean on load. + expect(screen.queryByTestId('flow-editor-dirty')).not.toBeInTheDocument(); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)( + makeProposal({ name: 'Standup reminder', graph: flow.graph }) + ); + }); + + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + // Graph is byte-identical to the persisted baseline — only the name + // changed — but the editor must still report dirty so Save is enabled. + await waitFor(() => expect(screen.getByTestId('flow-editor-dirty')).toBeInTheDocument()); + + fireEvent.click(screen.getByTestId('flow-editor-save')); + fireEvent.click(screen.getByTestId('flow-action-confirm-accept')); + + await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1)); + const [, update] = updateFlow.mock.calls[0]; + expect(update.name).toBe('Standup reminder'); + }); + + // Regression (CodeRabbit on #4886): when the backend returns a name that + // differs from what was submitted (server-side normalization), the title + // input must re-sync to the persisted value too — not just the committed + // `name` — or the stale draft can be resubmitted verbatim on a later blur. + it('re-syncs titleDraft from the persisted response name on Save', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'New workflow' })); + updateFlow.mockResolvedValue(makeFlow({ name: 'Standup Reminder (normalized)' })); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + + fireEvent.click(screen.getByTestId('flow-editor-save')); + fireEvent.click(screen.getByTestId('flow-action-confirm-accept')); + + await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1)); + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup Reminder (normalized)') + ); + }); + + it('passes the adopted name to createFlow on Save (draft flow)', async () => { + createFlow.mockResolvedValue(makeFlow({ id: 'created-id', name: 'Standup reminder' })); + render( + + + } /> + } /> + + + ); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('New workflow'); + + act(() => { + (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => void)(makeProposal()); + }); + await waitFor(() => + expect(screen.getByTestId('flow-canvas-title')).toHaveValue('Standup reminder') + ); + + fireEvent.click(screen.getByTestId('flow-editor-save')); + fireEvent.click(screen.getByTestId('flow-action-confirm-accept')); + + await waitFor(() => expect(createFlow).toHaveBeenCalledTimes(1)); + const [name] = createFlow.mock.calls[0]; + expect(name).toBe('Standup reminder'); + expect(updateFlow).not.toHaveBeenCalled(); + }); +}); + describe('asCopilotBuildSeed', () => { it('accepts a copilotBuild state with a non-empty description', () => { expect(asCopilotBuildSeed({ copilotBuild: { description: 'digest my Slack' } })).toEqual({ diff --git a/app/src/providers/ChatRuntimeProvider.tsx b/app/src/providers/ChatRuntimeProvider.tsx index e93b98507..ba137661d 100644 --- a/app/src/providers/ChatRuntimeProvider.tsx +++ b/app/src/providers/ChatRuntimeProvider.tsx @@ -272,30 +272,27 @@ function chatTurnUsagePayload(event: ChatDoneEvent): { * card. */ /** - * Tool names whose successful `output` carries a `workflow_proposal` payload. - * `propose_workflow` (first draft) and `revise_workflow` (iterative refine) - * both return the identical wire shape (see `src/openhuman/flows/builder_tools.rs`), - * so the runtime surfaces a `WorkflowProposalCard` from either. These run inside - * the `workflow_builder` specialist — reached either as the main agent's own - * tool or, in the Flows copilot / prompt-bar flow, as a delegated subagent - * (`build_workflow`) — so BOTH `onToolResult` and `onSubagentToolResult` funnel - * through {@link maybeParseWorkflowProposalTool}. - */ -const WORKFLOW_PROPOSAL_TOOLS = new Set(['propose_workflow', 'revise_workflow']); - -/** - * If a completed tool result is a successful workflow-builder proposal - * (`propose_workflow`/`revise_workflow`), parse it. Returns `null` for anything - * else so callers can cheaply gate on it. Keyed by the tool NAME + success, not - * by agent, so a proposal surfaces whether the tool ran in the main agent or in - * the delegated `workflow_builder` worker. + * Recognition is content-based, not name-based: ANY workflow-builder tool + * (`propose_workflow`, `revise_workflow`, `edit_workflow`, and any future + * addition) whose successful `output` is `{ type: "workflow_proposal", ... }` + * (see `src/openhuman/flows/builder_tools.rs` / `ops::build_builder_proposal`) + * is surfaced as a `WorkflowProposalCard`. This mirrors the Rust-side blocking + * path's `extract_workflow_proposal`, which also scans tool results by + * payload `type` rather than tool name — a name allowlist here can silently + * drop proposals from newly added tools (as happened when `edit_workflow` was + * added without updating this list). `parseWorkflowProposal`'s own + * `obj.type !== 'workflow_proposal'` check is the only gate needed. These + * tools run inside the `workflow_builder` specialist — reached either as the + * main agent's own tool or, in the Flows copilot / prompt-bar flow, as a + * delegated subagent (`build_workflow`) — so BOTH `onToolResult` and + * `onSubagentToolResult` funnel through {@link maybeParseWorkflowProposalTool}. */ function maybeParseWorkflowProposalTool( - toolName: string, + _toolName: string, success: boolean, output: string | undefined ): WorkflowProposal | null { - if (!success || !WORKFLOW_PROPOSAL_TOOLS.has(toolName) || !output) return null; + if (!success || !output) return null; return parseWorkflowProposal(output); } diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx index 353a349a2..ed2500185 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx @@ -1490,6 +1490,88 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria summary: { trigger: 'signup.created' }, }); }); + + // Regression (#4876 fallout): `edit_workflow` is the prompt-preferred way + // to iterate on an existing draft, and returns the identical + // `{ type: "workflow_proposal", ... }` payload as `propose_workflow` / + // `revise_workflow`. Proposal recognition is content-based (on the + // payload's `type`), not gated on a fixed tool-name allowlist, so this + // must surface a proposal exactly like the other two tools do — a + // name-based allowlist previously dropped it silently. + it('surfaces a workflow proposal from the main-agent edit_workflow tool', () => { + const listeners = renderProvider(); + const threadId = 't-edit-workflow'; + + act(() => { + listeners.onToolCall?.({ + thread_id: threadId, + request_id: 'r1', + round: 0, + tool_name: 'edit_workflow', + skill_id: 'flows', + args: {}, + tool_call_id: 'call-edit-1', + }); + listeners.onToolResult?.({ + thread_id: threadId, + request_id: 'r1', + round: 0, + tool_name: 'edit_workflow', + skill_id: 'flows', + success: true, + output: JSON.stringify({ + type: 'workflow_proposal', + name: 'Digest patch', + graph: { nodes: [], edges: [] }, + require_approval: true, + draft_id: 'draft-42', + summary: { trigger: 'manual', steps: [] }, + }), + tool_call_id: 'call-edit-1', + }); + }); + + const proposal = store.getState().chatRuntime.pendingWorkflowProposalsByThread[threadId]; + expect(proposal).toMatchObject({ name: 'Digest patch', requireApproval: true }); + }); + + // Any future tool that returns `{ type: "workflow_proposal" }` must be + // recognised too — the gate is the payload shape, not a tool-name list. + it('surfaces a workflow proposal from an unrecognised future tool name', () => { + const listeners = renderProvider(); + const threadId = 't-future-tool'; + + act(() => { + listeners.onToolCall?.({ + thread_id: threadId, + request_id: 'r1', + round: 0, + tool_name: 'some_future_builder_tool', + skill_id: 'flows', + args: {}, + tool_call_id: 'call-future-1', + }); + listeners.onToolResult?.({ + thread_id: threadId, + request_id: 'r1', + round: 0, + tool_name: 'some_future_builder_tool', + skill_id: 'flows', + success: true, + output: JSON.stringify({ + type: 'workflow_proposal', + name: 'Future proposal', + graph: { nodes: [], edges: [] }, + require_approval: true, + summary: { trigger: 'manual', steps: [] }, + }), + tool_call_id: 'call-future-1', + }); + }); + + const proposal = store.getState().chatRuntime.pendingWorkflowProposalsByThread[threadId]; + expect(proposal).toMatchObject({ name: 'Future proposal' }); + }); }); // Regression: on Windows users report being "locked out" of the composer diff --git a/app/src/store/chatRuntimeSlice.test.ts b/app/src/store/chatRuntimeSlice.test.ts index b41ff7829..05c97de39 100644 --- a/app/src/store/chatRuntimeSlice.test.ts +++ b/app/src/store/chatRuntimeSlice.test.ts @@ -18,6 +18,7 @@ import chatRuntimeReducer, { setPendingApprovalForThread, setQueueStatusForThread, setToolTimelineForThread, + setWorkflowProposalForThread, } from './chatRuntimeSlice'; function makeRun(id: string, status: AgentRunStatus): AgentRun { @@ -689,6 +690,75 @@ describe('hydrateRuntimeFromSnapshot — live-driver guard', () => { }); }); +// Regression: `fetchAndHydrateTurnState` (via `hydrateRuntimeFromSnapshot`) +// fires on thread rehydration (e.g. the always-open Flows copilot re-opening +// a persisted thread, #4874). A workflow proposal is a client-only flag with +// no server-side record, so a rehydrate must not resurrect a *stale* one from +// a crashed prior session — but it must also not wipe a proposal the +// streaming/blocking path set THIS session, moments before a `completed` +// snapshot for the same settled turn lands. Only `interrupted` (genuine +// crashed-mid-flight cleanup) should clear it. +describe('hydrateRuntimeFromSnapshot — workflow proposal race guard', () => { + function makeProposal(name: string) { + return { + name, + graph: { nodes: [], edges: [] }, + requireApproval: true, + summary: { trigger: 'manual', steps: [] }, + }; + } + + function makeSnapshot( + threadId: string, + lifecycle: PersistedTurnState['lifecycle'] + ): PersistedTurnState { + return { + threadId, + requestId: 'req-1', + lifecycle, + iteration: 3, + maxIterations: 10, + streamingText: '', + thinking: '', + toolTimeline: [], + startedAt: '2026-06-23T00:00:00Z', + updatedAt: '2026-06-23T00:00:00Z', + }; + } + + it('clears a pending proposal on an interrupted (crashed prior-session) snapshot', () => { + const store = makeStore(); + store.dispatch( + setWorkflowProposalForThread({ threadId: 't-crashed', proposal: makeProposal('Stale') }) + ); + + store.dispatch( + hydrateRuntimeFromSnapshot({ snapshot: makeSnapshot('t-crashed', 'interrupted') }) + ); + + expect( + store.getState().chatRuntime.pendingWorkflowProposalsByThread['t-crashed'] + ).toBeUndefined(); + }); + + it('preserves a pending proposal on a completed snapshot from this session', () => { + const store = makeStore(); + // The streaming/blocking path just set this moments before the + // rehydration thunk's `completed` snapshot lands for the same turn. + store.dispatch( + setWorkflowProposalForThread({ threadId: 't-settled', proposal: makeProposal('Fresh') }) + ); + + store.dispatch( + hydrateRuntimeFromSnapshot({ snapshot: makeSnapshot('t-settled', 'completed') }) + ); + + expect(store.getState().chatRuntime.pendingWorkflowProposalsByThread['t-settled']).toEqual( + makeProposal('Fresh') + ); + }); +}); + describe('hydrateRuntimeFromSnapshot — persisted tool result output', () => { it('maps the persisted output onto parent and sub-agent rows as result', () => { const store = makeStore(); diff --git a/app/src/store/chatRuntimeSlice.ts b/app/src/store/chatRuntimeSlice.ts index 549daf451..5ca4e0689 100644 --- a/app/src/store/chatRuntimeSlice.ts +++ b/app/src/store/chatRuntimeSlice.ts @@ -1969,8 +1969,15 @@ const chatRuntimeSlice = createSlice({ delete state.pendingPlanReviewByThread[threadId]; // Same for a workflow proposal (B4) — it's a client-only "should the // card render" flag with no server-side record, so a rehydrate must - // not resurrect one left over from a previous session. - delete state.pendingWorkflowProposalsByThread[threadId]; + // not resurrect one left over from a previous session. But only clear + // it on a genuinely stale snapshot (`interrupted` = crashed mid-flight + // in a prior process): a `completed` snapshot can be this session's own + // just-settled turn, racing against the streaming/blocking path that + // set the proposal moments ago — clearing unconditionally here would + // wipe a proposal that's still pending the user's Accept/Reject. + if (snapshot.lifecycle === 'interrupted') { + delete state.pendingWorkflowProposalsByThread[threadId]; + } if (snapshot.taskBoard) { state.taskBoardByThread[threadId] = snapshot.taskBoard; }