mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
feat(flows): let the copilot live test-run flows behind the approval gate (#5090)
This commit is contained in:
@@ -2,7 +2,7 @@ import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import type { WorkflowGraph, WorkflowNode } from '../../lib/flows/types';
|
||||
import type { WorkflowProposal } from '../../store/chatRuntimeSlice';
|
||||
import type { PendingApproval, WorkflowProposal } from '../../store/chatRuntimeSlice';
|
||||
import WorkflowCopilotPanel from './WorkflowCopilotPanel';
|
||||
|
||||
vi.mock('../../lib/i18n/I18nContext', () => ({ useT: () => ({ t: (key: string) => key }) }));
|
||||
@@ -21,10 +21,22 @@ vi.mock('../../features/conversations/components/ChatThreadView', () => ({
|
||||
),
|
||||
}));
|
||||
|
||||
// `ApprovalRequestCard` / `IntegrationConnectCard` (rendered for PR3:
|
||||
// flows-copilot-live-run-approval) dispatch via `useAppDispatch` internally —
|
||||
// stub the store hook rather than wrapping every render in a real Redux
|
||||
// `Provider`, since these tests only assert which card renders, not the
|
||||
// decide/connect flow those components own (covered by their own test files).
|
||||
vi.mock('../../store/hooks', () => ({ useAppDispatch: () => vi.fn() }));
|
||||
// Neither card calls this on mount (only on Approve/Deny/Connect click), but
|
||||
// stub it defensively so a real network call can never sneak into a render
|
||||
// test.
|
||||
vi.mock('../../services/coreRpcClient', () => ({ callCoreRpc: vi.fn() }));
|
||||
|
||||
const hookState = vi.hoisted(() => ({
|
||||
threadId: null as string | null,
|
||||
threadId: 'builder-1' as string | null,
|
||||
sending: false,
|
||||
proposal: null as WorkflowProposal | null,
|
||||
pendingApproval: null as PendingApproval | null,
|
||||
capped: false,
|
||||
error: null as string | null,
|
||||
send: vi.fn(),
|
||||
@@ -52,9 +64,10 @@ const baseGraph = graph(['a', 'b']);
|
||||
|
||||
describe('WorkflowCopilotPanel', () => {
|
||||
beforeEach(() => {
|
||||
hookState.threadId = null;
|
||||
hookState.threadId = 'builder-1';
|
||||
hookState.sending = false;
|
||||
hookState.proposal = null;
|
||||
hookState.pendingApproval = null;
|
||||
hookState.capped = false;
|
||||
hookState.error = null;
|
||||
hookState.send = vi.fn().mockResolvedValue({ outcome: 'dispatched', proposed: false });
|
||||
@@ -913,4 +926,94 @@ describe('WorkflowCopilotPanel', () => {
|
||||
|
||||
expect(hookState.send.mock.calls[0][0].request.mode).toBe('revise');
|
||||
});
|
||||
|
||||
// PR3 (flows-copilot-live-run-approval): `flows_build` now runs the
|
||||
// streaming turn under `AgentTurnOrigin::WebChat` + `APPROVAL_CHAT_CONTEXT`,
|
||||
// so a parked `run_flow` / `resume_flow_run` call surfaces here via the same
|
||||
// `pendingApproval` (sourced from `pendingApprovalByThread`) the main chat's
|
||||
// `Conversations.tsx` reads — reusing the EXISTING `ApprovalRequestCard` /
|
||||
// `IntegrationConnectCard`, no new component.
|
||||
describe('parked approval surface (PR3: flows-copilot-live-run-approval)', () => {
|
||||
function approvalOf(over: Partial<PendingApproval> = {}): PendingApproval {
|
||||
return {
|
||||
requestId: 'req-1',
|
||||
toolName: 'run_flow',
|
||||
message: 'Run the saved flow "Daily digest" to test it?',
|
||||
...over,
|
||||
};
|
||||
}
|
||||
|
||||
it('renders nothing when there is no pending approval', () => {
|
||||
hookState.pendingApproval = null;
|
||||
render(
|
||||
<WorkflowCopilotPanel
|
||||
graph={baseGraph}
|
||||
onProposal={vi.fn()}
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onClose={vi.fn()}
|
||||
/>
|
||||
);
|
||||
expect(screen.queryByTestId('workflow-copilot-approval')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('renders the shared ApprovalRequestCard for a parked run_flow/resume_flow_run/cancel_flow_run call', () => {
|
||||
hookState.pendingApproval = approvalOf({ toolName: 'run_flow' });
|
||||
render(
|
||||
<WorkflowCopilotPanel
|
||||
graph={baseGraph}
|
||||
onProposal={vi.fn()}
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onClose={vi.fn()}
|
||||
/>
|
||||
);
|
||||
expect(screen.getByTestId('workflow-copilot-approval')).toBeInTheDocument();
|
||||
// ApprovalRequestCard renders the parked call's message text verbatim.
|
||||
expect(screen.getByText('Run the saved flow "Daily digest" to test it?')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('renders IntegrationConnectCard (not ApprovalRequestCard) for a parked composio_connect call', () => {
|
||||
hookState.pendingApproval = approvalOf({
|
||||
toolName: 'composio_connect',
|
||||
toolkit: 'slack',
|
||||
message: 'Connect slack to complete your task',
|
||||
});
|
||||
render(
|
||||
<WorkflowCopilotPanel
|
||||
graph={baseGraph}
|
||||
onProposal={vi.fn()}
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onClose={vi.fn()}
|
||||
/>
|
||||
);
|
||||
const surface = screen.getByTestId('workflow-copilot-approval');
|
||||
expect(surface).toBeInTheDocument();
|
||||
// IntegrationConnectCard's affordance is a Connect button, not
|
||||
// Approve/Deny — assert the connect-specific copy is present and the
|
||||
// approve/deny copy is not, distinguishing it from ApprovalRequestCard.
|
||||
expect(screen.getByText('composio.connect.connect')).toBeInTheDocument();
|
||||
expect(screen.queryByText('chat.approval.approve')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('does not render the approval surface when threadId is not yet established', () => {
|
||||
// Guards the `pendingApproval && threadId` render condition: a parked
|
||||
// approval with no resolved thread id (shouldn't happen in practice —
|
||||
// an approval can only park on a thread `flows_build` already streamed
|
||||
// into — but defends against a stale/mismatched hook state).
|
||||
hookState.threadId = null;
|
||||
hookState.pendingApproval = approvalOf();
|
||||
render(
|
||||
<WorkflowCopilotPanel
|
||||
graph={baseGraph}
|
||||
onProposal={vi.fn()}
|
||||
onAccept={vi.fn()}
|
||||
onReject={vi.fn()}
|
||||
onClose={vi.fn()}
|
||||
/>
|
||||
);
|
||||
expect(screen.queryByTestId('workflow-copilot-approval')).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -36,7 +36,9 @@ import { diffGraphs } from '../../lib/flows/graphDiff';
|
||||
import type { WorkflowGraph } from '../../lib/flows/types';
|
||||
import { useT } from '../../lib/i18n/I18nContext';
|
||||
import type { WorkflowProposal } from '../../store/chatRuntimeSlice';
|
||||
import ApprovalRequestCard from '../chat/ApprovalRequestCard';
|
||||
import ChatComposer from '../chat/ChatComposer';
|
||||
import IntegrationConnectCard from '../chat/IntegrationConnectCard';
|
||||
import Button from '../ui/Button';
|
||||
|
||||
const log = createDebug('app:flows:copilot-panel');
|
||||
@@ -159,7 +161,7 @@ export default function WorkflowCopilotPanel({
|
||||
fullWidth = false,
|
||||
}: Props) {
|
||||
const { t } = useT();
|
||||
const { threadId, sending, proposal, capped, error, send, clearProposal } =
|
||||
const { threadId, sending, proposal, pendingApproval, capped, error, send, clearProposal } =
|
||||
useWorkflowBuilderChat(seedThreadId);
|
||||
const [text, setText] = useState('');
|
||||
|
||||
@@ -604,6 +606,36 @@ export default function WorkflowCopilotPanel({
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Parked ApprovalGate request for the copilot's dedicated thread (PR3:
|
||||
flows-copilot-live-run-approval). `flows_build` now runs under
|
||||
`AgentTurnOrigin::WebChat` + `APPROVAL_CHAT_CONTEXT` when streaming,
|
||||
so a `run_flow` / `resume_flow_run` call parks here instead of
|
||||
auto-allowing or being hidden — surfaced via the SAME
|
||||
`pendingApprovalByThread` slice / `approval_request` socket event
|
||||
`Conversations.tsx` reads for the main chat, reusing the identical
|
||||
cards (no new component, no new i18n keys). `composio_connect` parks
|
||||
on the same gate but needs a Connect button + OAuth poll rather than
|
||||
approve/deny, mirroring `Conversations.tsx`'s branch. Rendered above
|
||||
the composer, outside the scrollable transcript, so it stays visible
|
||||
regardless of scroll position. */}
|
||||
{pendingApproval && threadId && (
|
||||
<div data-testid="workflow-copilot-approval">
|
||||
{pendingApproval.toolName === 'composio_connect' ? (
|
||||
<IntegrationConnectCard
|
||||
key={pendingApproval.requestId}
|
||||
threadId={threadId}
|
||||
approval={pendingApproval}
|
||||
/>
|
||||
) : (
|
||||
<ApprovalRequestCard
|
||||
key={pendingApproval.requestId}
|
||||
threadId={threadId}
|
||||
approval={pendingApproval}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
<ChatComposer
|
||||
inputValue={text}
|
||||
setInputValue={setText}
|
||||
|
||||
@@ -2,7 +2,11 @@ import { act, renderHook, waitFor } from '@testing-library/react';
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import type { BuilderTurnResult } from '../services/api/flowsApi';
|
||||
import type { InferenceTurnLifecycle, WorkflowProposal } from '../store/chatRuntimeSlice';
|
||||
import type {
|
||||
InferenceTurnLifecycle,
|
||||
PendingApproval,
|
||||
WorkflowProposal,
|
||||
} from '../store/chatRuntimeSlice';
|
||||
import type { ThreadMessage } from '../types/thread';
|
||||
import { useWorkflowBuilderChat, type WorkflowBuilderSendResult } from './useWorkflowBuilderChat';
|
||||
|
||||
@@ -22,6 +26,7 @@ const selectorState = vi.hoisted(() => ({
|
||||
toolTimelineByThread: {} as Record<string, unknown[]>,
|
||||
streamingAssistantByThread: {} as Record<string, { content: string }>,
|
||||
inferenceTurnLifecycleByThread: {} as Record<string, InferenceTurnLifecycle>,
|
||||
pendingApprovalByThread: {} as Record<string, PendingApproval>,
|
||||
}));
|
||||
vi.mock('../store/hooks', () => ({
|
||||
useAppDispatch: () => dispatch,
|
||||
@@ -33,6 +38,7 @@ vi.mock('../store/hooks', () => ({
|
||||
toolTimelineByThread: selectorState.toolTimelineByThread,
|
||||
streamingAssistantByThread: selectorState.streamingAssistantByThread,
|
||||
inferenceTurnLifecycleByThread: selectorState.inferenceTurnLifecycleByThread,
|
||||
pendingApprovalByThread: selectorState.pendingApprovalByThread,
|
||||
},
|
||||
}),
|
||||
}));
|
||||
@@ -73,6 +79,7 @@ describe('useWorkflowBuilderChat', () => {
|
||||
selectorState.toolTimelineByThread = {};
|
||||
selectorState.streamingAssistantByThread = {};
|
||||
selectorState.inferenceTurnLifecycleByThread = {};
|
||||
selectorState.pendingApprovalByThread = {};
|
||||
dispatch.mockReset().mockImplementation((action: { type: string }) => {
|
||||
if (action.type === 'createNewThread') {
|
||||
return { unwrap: () => Promise.resolve({ id: 'builder-1' }) };
|
||||
@@ -462,6 +469,38 @@ describe('useWorkflowBuilderChat', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// PR3 (flows-copilot-live-run-approval): `pendingApproval` is a thin
|
||||
// thread-scoped selector over `pendingApprovalByThread` — the same slice
|
||||
// `Conversations.tsx` reads for the main chat's `ApprovalRequestCard`.
|
||||
describe('pendingApproval', () => {
|
||||
it('is null before a thread exists', () => {
|
||||
const { result } = renderHook(() => useWorkflowBuilderChat());
|
||||
expect(result.current.pendingApproval).toBeNull();
|
||||
});
|
||||
|
||||
it('surfaces the parked approval for the dedicated/seeded thread', () => {
|
||||
const approval: PendingApproval = {
|
||||
requestId: 'req-1',
|
||||
toolName: 'run_flow',
|
||||
message: 'Run the saved flow "Daily digest"?',
|
||||
};
|
||||
selectorState.pendingApprovalByThread = { 'builder-1': approval };
|
||||
const { result } = renderHook(() => useWorkflowBuilderChat('builder-1'));
|
||||
expect(result.current.pendingApproval).toEqual(approval);
|
||||
});
|
||||
|
||||
it('does not surface an approval parked on a DIFFERENT thread', () => {
|
||||
const approval: PendingApproval = {
|
||||
requestId: 'req-1',
|
||||
toolName: 'run_flow',
|
||||
message: 'Run the saved flow "Daily digest"?',
|
||||
};
|
||||
selectorState.pendingApprovalByThread = { 'some-other-thread': approval };
|
||||
const { result } = renderHook(() => useWorkflowBuilderChat('builder-1'));
|
||||
expect(result.current.pendingApproval).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('displayMessages', () => {
|
||||
it('excludes isInterim agent messages but keeps user + terminal agent messages (incl. a clarifying question)', () => {
|
||||
selectorState.messagesByThreadId = {
|
||||
|
||||
@@ -38,6 +38,7 @@ import {
|
||||
endInferenceTurn,
|
||||
fetchAndHydrateTurnHistory,
|
||||
fetchAndHydrateTurnState,
|
||||
type PendingApproval,
|
||||
setWorkflowProposalForThread,
|
||||
type ToolTimelineEntry,
|
||||
type WorkflowProposal,
|
||||
@@ -111,6 +112,18 @@ export interface UseWorkflowBuilderChat {
|
||||
turnActive: boolean;
|
||||
/** The latest proposal the agent returned on this thread, or `null`. */
|
||||
proposal: WorkflowProposal | null;
|
||||
/**
|
||||
* A parked `ApprovalGate` request for this thread (PR3:
|
||||
* flows-copilot-live-run-approval), or `null`. The copilot's `flows_build`
|
||||
* turn now runs `run_flow` / `resume_flow_run` under the same
|
||||
* `AgentTurnOrigin::WebChat` + `APPROVAL_CHAT_CONTEXT` scope a real
|
||||
* interactive chat turn uses, so a live test-run parks here instead of
|
||||
* either auto-allowing or being hidden outright. Sourced from the SAME
|
||||
* `pendingApprovalByThread` slice / `approval_request` socket event the
|
||||
* main chat's `ApprovalRequestCard` reads — no new plumbing, just scoped to
|
||||
* this hook's dedicated thread.
|
||||
*/
|
||||
pendingApproval: PendingApproval | null;
|
||||
/**
|
||||
* `true` when the most recently settled turn paused because it hit the
|
||||
* agent's tool-call budget with no proposal yet (B34) — the caller should
|
||||
@@ -214,6 +227,9 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
|
||||
const inferenceTurnLifecycleByThread = useAppSelector(
|
||||
state => state.chatRuntime.inferenceTurnLifecycleByThread
|
||||
);
|
||||
const pendingApprovalByThread = useAppSelector(
|
||||
state => state.chatRuntime.pendingApprovalByThread
|
||||
);
|
||||
|
||||
// A turn is in flight on this thread iff its lifecycle entry is `'started'`
|
||||
// or `'streaming'` — NOT `'interrupted'`, which `hydrateRuntimeFromSnapshot`
|
||||
@@ -236,6 +252,15 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
|
||||
[threadId, proposalsByThread]
|
||||
);
|
||||
|
||||
// PR3 (flows-copilot-live-run-approval): mirrors `proposal` above — read the
|
||||
// shared `pendingApprovalByThread` slice scoped to this hook's dedicated
|
||||
// thread, so a parked `run_flow`/`resume_flow_run` call surfaces here the
|
||||
// same way `Conversations.tsx` surfaces one for the main chat.
|
||||
const pendingApproval = useMemo(
|
||||
() => (threadId ? (pendingApprovalByThread[threadId] ?? null) : null),
|
||||
[threadId, pendingApprovalByThread]
|
||||
);
|
||||
|
||||
const messages = useMemo(
|
||||
() => (threadId ? (messagesByThreadId[threadId] ?? EMPTY_MESSAGES) : EMPTY_MESSAGES),
|
||||
[threadId, messagesByThreadId]
|
||||
@@ -473,6 +498,7 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
|
||||
sending,
|
||||
turnActive,
|
||||
proposal,
|
||||
pendingApproval,
|
||||
capped,
|
||||
messages,
|
||||
displayMessages,
|
||||
|
||||
@@ -14,9 +14,16 @@ max_result_chars = 12000
|
||||
sandbox_mode = "none"
|
||||
omit_identity = true
|
||||
omit_memory_context = true
|
||||
# Safety preamble stays OFF: this agent has zero real external effect (it only
|
||||
# proposes validated graphs and reads), so the general acting guard-rails would
|
||||
# be noise. The "propose, never persist" invariant is taught in prompt.md.
|
||||
# Safety preamble stays OFF: authoring itself (propose/revise/dry-run/reads)
|
||||
# has zero real external effect, so the general acting guard-rails would be
|
||||
# noise there — the "propose, never persist" invariant is taught in prompt.md.
|
||||
# The one real-effect surface, `run_flow` (a confirmed test-run of an already
|
||||
# saved flow), is gated by the `ApprovalGate` itself, not this preamble: on the
|
||||
# streaming copilot path (flows-copilot-live-run-approval, PR3) it parks behind
|
||||
# a real human approval card (`AgentTurnOrigin::WebChat` +
|
||||
# `APPROVAL_CHAT_CONTEXT`, same as main chat); on the headless `flows_build`
|
||||
# path it stays hidden entirely (`restrict_builder_toolset`, #4593/#4881) since
|
||||
# there is no approval surface to park against there.
|
||||
omit_safety_preamble = true
|
||||
omit_skills_catalog = true
|
||||
|
||||
|
||||
+131
-15
@@ -10,7 +10,9 @@ use serde_json::{json, Value};
|
||||
use tinyflows::model::{NodeKind, TriggerKind, WorkflowGraph};
|
||||
|
||||
use crate::openhuman::agent::turn_origin::{with_origin, AgentTurnOrigin, TrustedAutomationSource};
|
||||
use crate::openhuman::approval::{FlowRunContext, APPROVAL_FLOW_RUN_CONTEXT};
|
||||
use crate::openhuman::approval::{
|
||||
ApprovalChatContext, FlowRunContext, APPROVAL_CHAT_CONTEXT, APPROVAL_FLOW_RUN_CONTEXT,
|
||||
};
|
||||
use crate::openhuman::config::Config;
|
||||
use crate::openhuman::flows::bus;
|
||||
use crate::openhuman::flows::draft_store;
|
||||
@@ -4266,6 +4268,54 @@ fn restrict_builder_toolset(agent: &mut crate::openhuman::agent::Agent) {
|
||||
agent.hide_tools(FLOWS_BUILD_HIDDEN_TOOLS);
|
||||
}
|
||||
|
||||
/// Tools stripped from the `workflow_builder` belt on the STREAMING
|
||||
/// (copilot-pane) `flows_build` path — the reduced sibling of
|
||||
/// [`FLOWS_BUILD_HIDDEN_TOOLS`] used by [`restrict_builder_toolset`] on the
|
||||
/// headless path.
|
||||
///
|
||||
/// PR3 (flows-copilot-live-run-approval): when a chat thread is attached
|
||||
/// (`stream.is_some()`), `flows_build` now runs the builder under
|
||||
/// [`AgentTurnOrigin::WebChat`] with [`APPROVAL_CHAT_CONTEXT`] scoped
|
||||
/// alongside it — the exact same double-scope the main web-chat delegate uses
|
||||
/// (`web_chat::ops::run_turn_under_cancel_and_deadline`). Under that origin
|
||||
/// the [`crate::openhuman::approval::ApprovalGate`] no longer auto-allows
|
||||
/// `external_effect` tools; it PARKS them for a real human decision, routed
|
||||
/// back to this thread via the existing `approval_request` socket event and
|
||||
/// rendered with the existing `ApprovalRequestCard` in the copilot panel. So
|
||||
/// `run_flow` and `resume_flow_run` — both `external_effect() == true` — no
|
||||
/// longer need to be hidden on this path: they are reachable, but gated
|
||||
/// behind a real approval, exactly like a main-chat tool call.
|
||||
///
|
||||
/// `cancel_flow_run` stays HIDDEN on this path, though. It reports
|
||||
/// `external_effect() == false`, so `ApprovalSecurityMiddleware` would not park
|
||||
/// it behind the approval surface — and the tool cancels an arbitrary run id
|
||||
/// (e.g. one read from `list_flow_runs`) with no ownership check. An unhidden
|
||||
/// `cancel_flow_run` would therefore let a streaming copilot turn cancel ANY
|
||||
/// in-flight or approval-parked run, unapproved — far broader than the "stop a
|
||||
/// run the copilot itself started" companion use it was meant for. Until it
|
||||
/// gains an ownership/approval guard it is kept hidden here (a user can still
|
||||
/// cancel from the Runs rail). (codex review, #5090.)
|
||||
///
|
||||
/// `run_workflow` (the unrelated legacy skills-workflow runner sharing this
|
||||
/// belt) stays hidden on BOTH paths — belt-and-braces against a re-rename or
|
||||
/// the name ever leaking back onto the `workflow_builder` toolset; `hide_tools`
|
||||
/// no-ops on a name that isn't present.
|
||||
const FLOWS_BUILD_COPILOT_HIDDEN_TOOLS: &[&str] = &["run_workflow", "cancel_flow_run"];
|
||||
|
||||
/// Strip only [`FLOWS_BUILD_COPILOT_HIDDEN_TOOLS`] from `agent`'s callable set
|
||||
/// on the streaming `flows_build` path (copilot pane with a real approval
|
||||
/// surface) — see that constant's doc for the full safety rationale.
|
||||
fn restrict_builder_toolset_for_copilot(agent: &mut crate::openhuman::agent::Agent) {
|
||||
tracing::info!(
|
||||
target: "flows",
|
||||
hidden = ?FLOWS_BUILD_COPILOT_HIDDEN_TOOLS,
|
||||
"[flows] flows_build: streaming copilot turn — run_flow/resume_flow_run stay visible \
|
||||
(gated behind the WebChat approval surface); run_workflow + cancel_flow_run hidden \
|
||||
(cancel_flow_run has no external_effect to park and no run-ownership guard)"
|
||||
);
|
||||
agent.hide_tools(FLOWS_BUILD_COPILOT_HIDDEN_TOOLS);
|
||||
}
|
||||
|
||||
/// Runs the `workflow_builder` agent for one authoring turn and returns its
|
||||
/// proposal, invoking it as a first-class backend agent (exactly like the Flow
|
||||
/// Scout `flows_discover`) rather than routing a hand-crafted delegate prompt
|
||||
@@ -4316,13 +4366,35 @@ pub async fn flows_build(
|
||||
.map_err(|e| format!("failed to build workflow_builder agent: {e:#}"))?;
|
||||
agent.set_agent_definition_name("workflow_builder".to_string());
|
||||
|
||||
// Strip the live-run tool(s) from the belt on this direct RPC path: under
|
||||
// the `AgentTurnOrigin::Cli` origin below the approval gate auto-allows
|
||||
// every external_effect tool, so `run_flow` could execute a live saved flow
|
||||
// with no HITL confirmation (issue #4593). Restricting the visible set makes
|
||||
// it `Deny` at the tool-call boundary; the authoring tools are untouched so
|
||||
// the turn still runs headless without fail-closing.
|
||||
restrict_builder_toolset(&mut agent);
|
||||
// Restrict the visible run-advancing tools per path (PR3:
|
||||
// flows-copilot-live-run-approval). Streaming (copilot pane, real approval
|
||||
// surface below) only hides the always-hidden `run_workflow`; headless
|
||||
// (CLI / tests / no chat thread) keeps the full historical hide-list
|
||||
// (issue #4593 / #4881) since there is no routable approval surface there.
|
||||
//
|
||||
// The reduced (copilot) hide-list is safe ONLY when the process-global
|
||||
// `ApprovalGate` is actually installed to park the unhidden
|
||||
// `run_flow`/`resume_flow_run`. `flows_build` is a public RPC and the gate
|
||||
// can be opted out (`OPENHUMAN_APPROVAL_GATE=0` on CLI/docker leaves
|
||||
// `ApprovalGate::try_global()` == `None`; desktop always installs it) — and
|
||||
// `ApprovalSecurityMiddleware` skips interception entirely when the gate is
|
||||
// absent, so the WebChat origin below would NOT park and the unhidden
|
||||
// live-run tools would execute unapproved. Fall back to the full hide-list
|
||||
// whenever the gate is not installed, regardless of `stream`. (codex #5090)
|
||||
let approval_gate_active = crate::openhuman::approval::ApprovalGate::try_global().is_some();
|
||||
if stream.is_some() && approval_gate_active {
|
||||
restrict_builder_toolset_for_copilot(&mut agent);
|
||||
} else {
|
||||
if stream.is_some() {
|
||||
tracing::warn!(
|
||||
target: "flows",
|
||||
"[flows] flows_build: streaming turn but no ApprovalGate installed \
|
||||
(OPENHUMAN_APPROVAL_GATE off / headless) — keeping the full live-run \
|
||||
hide-list so run_flow/resume_flow_run cannot execute unapproved"
|
||||
);
|
||||
}
|
||||
restrict_builder_toolset(&mut agent);
|
||||
}
|
||||
|
||||
// When a chat thread is attached (the copilot pane), stream the builder turn
|
||||
// into it exactly like an interactive turn — text/tool deltas and the
|
||||
@@ -4332,21 +4404,65 @@ pub async fn flows_build(
|
||||
attach_flow_progress_bridge(&mut agent, target, "flows_build", config);
|
||||
}
|
||||
|
||||
// Run to completion under a CLI origin (internal, user-initiated — the
|
||||
// approval gate must not fail-closed), bounded by a wall-clock timeout. When
|
||||
// streaming, wrap the run in the thread-id scope so descendant turns tag
|
||||
// their trace + socket events with this thread.
|
||||
let run = with_origin(AgentTurnOrigin::Cli, agent.run_single(&prompt));
|
||||
let run = tokio::time::timeout(std::time::Duration::from_secs(FLOW_BUILD_TIMEOUT_SECS), run);
|
||||
// Run to completion, bounded by a wall-clock timeout. PR3
|
||||
// (flows-copilot-live-run-approval): the origin now depends on whether a
|
||||
// chat thread is attached.
|
||||
//
|
||||
// - Streaming (copilot pane): run under `AgentTurnOrigin::WebChat` with
|
||||
// `APPROVAL_CHAT_CONTEXT` scoped alongside it — the identical
|
||||
// double-scope pattern `web_chat::ops::run_turn_under_cancel_and_deadline`
|
||||
// uses for a real interactive chat turn. The approval gate then PARKS
|
||||
// (rather than auto-allows) any `external_effect` tool call instead of
|
||||
// failing closed, and the resulting `ApprovalRequested` event routes back
|
||||
// to this thread (`client_id: "system"` — every client auto-joins that
|
||||
// broadcast room, matching the progress bridge above) for the existing
|
||||
// `ApprovalRequestCard` to render. The run is additionally wrapped in the
|
||||
// thread-id scope so descendant turns tag their trace + socket events
|
||||
// with this thread.
|
||||
// - Headless (CLI / tests / no chat thread): unchanged `AgentTurnOrigin::Cli`
|
||||
// — the gate auto-allows `external_effect` tools under that origin, which
|
||||
// is why `restrict_builder_toolset` above must keep the full hide-list on
|
||||
// this path; there is no routable approval surface here to park against.
|
||||
let timed = match &stream {
|
||||
Some(target) => {
|
||||
let origin = AgentTurnOrigin::WebChat {
|
||||
thread_id: target.thread_id.clone(),
|
||||
client_id: "system".to_string(),
|
||||
request_id: Some(target.request_id.clone()),
|
||||
};
|
||||
let chat_ctx = ApprovalChatContext {
|
||||
thread_id: target.thread_id.clone(),
|
||||
client_id: "system".to_string(),
|
||||
};
|
||||
tracing::info!(
|
||||
target: "flows",
|
||||
thread_id = %target.thread_id,
|
||||
request_id = %target.request_id,
|
||||
"[flows] flows_build: streaming copilot turn — WebChat origin + \
|
||||
APPROVAL_CHAT_CONTEXT scoped, live-run tools park for approval instead \
|
||||
of auto-allowing"
|
||||
);
|
||||
let run = with_origin(
|
||||
origin,
|
||||
APPROVAL_CHAT_CONTEXT.scope(chat_ctx, agent.run_single(&prompt)),
|
||||
);
|
||||
let run =
|
||||
tokio::time::timeout(std::time::Duration::from_secs(FLOW_BUILD_TIMEOUT_SECS), run);
|
||||
crate::openhuman::inference::provider::thread_context::with_thread_id(
|
||||
target.thread_id.clone(),
|
||||
run,
|
||||
)
|
||||
.await
|
||||
}
|
||||
None => run.await,
|
||||
None => {
|
||||
tracing::debug!(
|
||||
target: "flows",
|
||||
"[flows] flows_build: headless/CLI turn — Cli origin, approval gate \
|
||||
auto-allows external_effect tools (run-advancing tools stay hidden)"
|
||||
);
|
||||
let run = with_origin(AgentTurnOrigin::Cli, agent.run_single(&prompt));
|
||||
tokio::time::timeout(std::time::Duration::from_secs(FLOW_BUILD_TIMEOUT_SECS), run).await
|
||||
}
|
||||
};
|
||||
let (assistant_text, run_error) = match timed {
|
||||
Ok(Ok(text)) => (text, None),
|
||||
|
||||
@@ -3886,6 +3886,86 @@ async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() {
|
||||
}
|
||||
}
|
||||
|
||||
/// Pins the exact contents of both `flows_build` hide-lists so a future edit
|
||||
/// can't silently narrow/widen either belt without a test catching it
|
||||
/// (PR3: flows-copilot-live-run-approval).
|
||||
#[test]
|
||||
fn flows_build_hide_lists_have_the_expected_contents() {
|
||||
assert_eq!(
|
||||
FLOWS_BUILD_COPILOT_HIDDEN_TOOLS,
|
||||
["run_workflow", "cancel_flow_run"],
|
||||
"the streaming (copilot) hide-list must hide the legacy `run_workflow` AND \
|
||||
`cancel_flow_run` — the latter has no external_effect to park and no \
|
||||
run-ownership guard (codex #5090), so it must NOT be exposed unapproved; \
|
||||
only `run_flow`/`resume_flow_run` stay visible, gated by the WebChat \
|
||||
approval surface"
|
||||
);
|
||||
for tool in [
|
||||
"run_workflow",
|
||||
"run_flow",
|
||||
"resume_flow_run",
|
||||
"cancel_flow_run",
|
||||
] {
|
||||
assert!(
|
||||
FLOWS_BUILD_HIDDEN_TOOLS.contains(&tool),
|
||||
"the headless hide-list must still contain `{tool}` (existing #4593/#4881 \
|
||||
contract) — {FLOWS_BUILD_HIDDEN_TOOLS:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Streaming (copilot) path: `restrict_builder_toolset_for_copilot` leaves
|
||||
/// `run_flow` / `resume_flow_run` visible on the builder's belt — they're gated
|
||||
/// by the WebChat approval surface, not hidden — while hiding the unrelated
|
||||
/// legacy `run_workflow` AND `cancel_flow_run` (the latter can't be parked and
|
||||
/// has no run-ownership guard — codex #5090) and keeping every authoring tool
|
||||
/// reachable (PR3: flows-copilot-live-run-approval).
|
||||
#[tokio::test]
|
||||
async fn flows_build_copilot_toolset_unhides_the_live_run_tools() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
crate::openhuman::agent::harness::AgentDefinitionRegistry::init_global(&config.workspace_dir)
|
||||
.expect("agent registry init");
|
||||
let mut agent =
|
||||
crate::openhuman::agent::Agent::from_config_for_agent(&config, "workflow_builder")
|
||||
.expect("build workflow_builder agent");
|
||||
agent.set_agent_definition_name("workflow_builder".to_string());
|
||||
|
||||
restrict_builder_toolset_for_copilot(&mut agent);
|
||||
|
||||
let visible = agent.visible_tool_names_for_test();
|
||||
for still_reachable in ["run_flow", "resume_flow_run"] {
|
||||
assert!(
|
||||
visible.contains(still_reachable),
|
||||
"`{still_reachable}` must stay reachable on the streaming copilot path — it \
|
||||
is gated behind the WebChat approval surface, not hidden; visible = {visible:?}"
|
||||
);
|
||||
}
|
||||
for hidden in ["run_workflow", "cancel_flow_run"] {
|
||||
assert!(
|
||||
!visible.contains(hidden),
|
||||
"`{hidden}` must stay hidden on the copilot path (legacy runner / \
|
||||
unparkable-and-unguarded cancel — codex #5090); visible = {visible:?}"
|
||||
);
|
||||
}
|
||||
for keep in [
|
||||
"propose_workflow",
|
||||
"revise_workflow",
|
||||
"save_workflow",
|
||||
"dry_run_workflow",
|
||||
"list_flows",
|
||||
"create_workflow",
|
||||
"duplicate_flow",
|
||||
] {
|
||||
assert!(
|
||||
visible.contains(keep),
|
||||
"authoring tool `{keep}` must remain visible on the copilot path; visible = \
|
||||
{visible:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression for issue #4868 (systemic fix, superseding the old B31
|
||||
/// per-caller `apply_builder_iteration_cap` override): `flows_build` must get
|
||||
/// an agent carrying the `workflow_builder` `AgentDefinition`'s
|
||||
|
||||
Reference in New Issue
Block a user