mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(flows): gate flows_create so saving a scheduled/side-effecting flow can't silently arm it (#4835)
This commit is contained in:
@@ -13,15 +13,18 @@ vi.mock('../../lib/i18n/I18nContext', () => ({ useT: () => ({ t: (key: string) =
|
||||
// time the (hoisted) factory runs. (These specific names happened to work
|
||||
// without it, since Vitest's compiler special-cases `mock`-prefixed
|
||||
// identifiers, but that's an incidental heuristic, not a guarantee.)
|
||||
const { mockCreateFlow, mockUpdateFlow, mockDispatch, mockNavigate } = vi.hoisted(() => ({
|
||||
mockCreateFlow: vi.fn(),
|
||||
mockUpdateFlow: vi.fn(),
|
||||
mockDispatch: vi.fn(),
|
||||
mockNavigate: vi.fn(),
|
||||
}));
|
||||
const { mockCreateFlow, mockUpdateFlow, mockSetFlowEnabled, mockDispatch, mockNavigate } =
|
||||
vi.hoisted(() => ({
|
||||
mockCreateFlow: vi.fn(),
|
||||
mockUpdateFlow: vi.fn(),
|
||||
mockSetFlowEnabled: vi.fn(),
|
||||
mockDispatch: vi.fn(),
|
||||
mockNavigate: vi.fn(),
|
||||
}));
|
||||
vi.mock('../../services/api/flowsApi', () => ({
|
||||
createFlow: (...args: unknown[]) => mockCreateFlow(...args),
|
||||
updateFlow: (...args: unknown[]) => mockUpdateFlow(...args),
|
||||
setFlowEnabled: (...args: unknown[]) => mockSetFlowEnabled(...args),
|
||||
}));
|
||||
vi.mock('../../store/hooks', () => ({ useAppDispatch: () => mockDispatch }));
|
||||
vi.mock('react-router-dom', () => ({ useNavigate: () => mockNavigate }));
|
||||
@@ -44,8 +47,11 @@ function proposal(partial: Partial<WorkflowProposal> = {}): WorkflowProposal {
|
||||
|
||||
describe('WorkflowProposalCard', () => {
|
||||
beforeEach(() => {
|
||||
mockCreateFlow.mockReset().mockResolvedValue({ id: 'f1', name: 'Daily standup summary' });
|
||||
mockCreateFlow
|
||||
.mockReset()
|
||||
.mockResolvedValue({ id: 'f1', name: 'Daily standup summary', enabled: true });
|
||||
mockUpdateFlow.mockReset();
|
||||
mockSetFlowEnabled.mockReset().mockResolvedValue({ id: 'f1', enabled: true });
|
||||
mockDispatch.mockReset();
|
||||
mockNavigate.mockReset();
|
||||
});
|
||||
@@ -74,6 +80,8 @@ describe('WorkflowProposalCard', () => {
|
||||
expect(mockCreateFlow).toHaveBeenCalledWith(p.name, p.graph, p.requireApproval)
|
||||
);
|
||||
expect(mockDispatch).toHaveBeenCalledTimes(1);
|
||||
// createFlow already came back enabled — no need for a follow-up arm.
|
||||
expect(mockSetFlowEnabled).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('shows a loading state while saving', async () => {
|
||||
@@ -86,7 +94,7 @@ describe('WorkflowProposalCard', () => {
|
||||
render(<WorkflowProposalCard threadId="t1" proposal={proposal()} />);
|
||||
fireEvent.click(screen.getByText('chat.flowProposal.save'));
|
||||
await waitFor(() => expect(screen.getByText('chat.flowProposal.saving')).toBeInTheDocument());
|
||||
resolveCreate({ id: 'f1' });
|
||||
resolveCreate({ id: 'f1', enabled: true });
|
||||
});
|
||||
|
||||
it('surfaces an error and stays mounted when createFlow fails', async () => {
|
||||
@@ -96,6 +104,51 @@ describe('WorkflowProposalCard', () => {
|
||||
await waitFor(() => expect(screen.getByText(/chat\.flowProposal\.error/)).toBeInTheDocument());
|
||||
// Not cleared on failure.
|
||||
expect(mockDispatch).not.toHaveBeenCalled();
|
||||
expect(mockSetFlowEnabled).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Issue B29 Rule 1: an automatic-trigger graph (schedule/app_event/webhook)
|
||||
// comes back from `flows_create` disabled, regardless of what the caller
|
||||
// asked for. "Save & enable" is the user's own explicit arming click, so
|
||||
// the card must follow up with `setFlowEnabled` to actually arm it —
|
||||
// otherwise the CTA's own label would be a lie and the flow would never
|
||||
// fire despite the user clicking "enable".
|
||||
it('explicitly enables via setFlowEnabled when createFlow returns a disabled auto-trigger flow', async () => {
|
||||
mockCreateFlow.mockResolvedValueOnce({
|
||||
id: 'f1',
|
||||
name: 'Daily standup summary',
|
||||
enabled: false,
|
||||
});
|
||||
const p = proposal();
|
||||
render(<WorkflowProposalCard threadId="t1" proposal={p} />);
|
||||
fireEvent.click(screen.getByText('chat.flowProposal.save'));
|
||||
await waitFor(() => expect(mockSetFlowEnabled).toHaveBeenCalledWith('f1', true));
|
||||
expect(mockDispatch).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('keeps the flow saved and lets the user retry just the enable step if setFlowEnabled fails', async () => {
|
||||
mockCreateFlow.mockResolvedValueOnce({
|
||||
id: 'f1',
|
||||
name: 'Daily standup summary',
|
||||
enabled: false,
|
||||
});
|
||||
mockSetFlowEnabled.mockRejectedValueOnce(new Error('enable boom'));
|
||||
render(<WorkflowProposalCard threadId="t1" proposal={proposal()} />);
|
||||
fireEvent.click(screen.getByText('chat.flowProposal.save'));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByText(/chat\.flowProposal\.enableError/)).toBeInTheDocument()
|
||||
);
|
||||
// The flow was already persisted — the card must not be cleared, and a
|
||||
// retry must not re-create it (which would duplicate the flow).
|
||||
expect(mockDispatch).not.toHaveBeenCalled();
|
||||
|
||||
mockSetFlowEnabled.mockResolvedValueOnce({ id: 'f1', enabled: true });
|
||||
fireEvent.click(screen.getByText('chat.flowProposal.save'));
|
||||
await waitFor(() => expect(mockDispatch).toHaveBeenCalledTimes(1));
|
||||
// Only ever created once, even though "Save & enable" was clicked twice.
|
||||
expect(mockCreateFlow).toHaveBeenCalledTimes(1);
|
||||
expect(mockSetFlowEnabled).toHaveBeenCalledTimes(2);
|
||||
expect(mockSetFlowEnabled).toHaveBeenLastCalledWith('f1', true);
|
||||
});
|
||||
|
||||
it('opens the proposed graph in the canvas as an unsaved draft without persisting', () => {
|
||||
|
||||
@@ -5,7 +5,7 @@ import { useNavigate } from 'react-router-dom';
|
||||
import { FLOW_CANVAS_DRAFT_ROUTE, type FlowCanvasDraftState } from '../../lib/flows/canvasDraft';
|
||||
import type { WorkflowGraph } from '../../lib/flows/types';
|
||||
import { useT } from '../../lib/i18n/I18nContext';
|
||||
import { createFlow } from '../../services/api/flowsApi';
|
||||
import { createFlow, setFlowEnabled } from '../../services/api/flowsApi';
|
||||
import {
|
||||
clearWorkflowProposalForThread,
|
||||
type WorkflowProposal,
|
||||
@@ -36,6 +36,18 @@ interface Props {
|
||||
* the client; the agent has no way to reach that RPC on its own. "Dismiss"
|
||||
* just clears the proposal without saving anything.
|
||||
*
|
||||
* B29 (save/enable safety) Rule 1 forces `flows_create` to persist an
|
||||
* automatic-trigger graph (`schedule` / `app_event` / `webhook`) DISABLED,
|
||||
* no matter what the caller passed — that's what stops a copilot
|
||||
* `save_workflow` autosave from silently arming an unattended automation.
|
||||
* But "Save & enable" is the user's own explicit arming click, not a silent
|
||||
* autosave, so when `createFlow` hands back a disabled flow here, `save`
|
||||
* follows up with an explicit {@link setFlowEnabled} call — the same toggle
|
||||
* `flows_set_enabled` exposes everywhere else — so the button does what it
|
||||
* says. If that follow-up enable call itself fails, the flow stays saved
|
||||
* (disabled) and the card keeps `savedFlowId` around so a retry re-enables
|
||||
* instead of re-creating (which would duplicate the flow).
|
||||
*
|
||||
* Mirrors {@link PlanReviewCard}'s placement/chrome above the composer, and
|
||||
* the tool-timeline `StatusTag`/detail-chip visual language for the
|
||||
* node-kind badges + config hints in the step list.
|
||||
@@ -46,6 +58,11 @@ export const WorkflowProposalCard: React.FC<Props> = ({ threadId, proposal, onSa
|
||||
const navigate = useNavigate();
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [errorMsg, setErrorMsg] = useState<string | null>(null);
|
||||
// Set once `createFlow` has persisted the flow but it came back disabled
|
||||
// (B29 Rule 1) and the follow-up `setFlowEnabled` call hasn't succeeded
|
||||
// yet. Non-null means a retry of `save` should skip `createFlow` entirely
|
||||
// and only retry the enable step.
|
||||
const [savedFlowId, setSavedFlowId] = useState<string | null>(null);
|
||||
|
||||
const dismiss = () => {
|
||||
dispatch(clearWorkflowProposalForThread({ threadId }));
|
||||
@@ -80,13 +97,37 @@ export const WorkflowProposalCard: React.FC<Props> = ({ threadId, proposal, onSa
|
||||
if (saving) return;
|
||||
setSaving(true);
|
||||
setErrorMsg(null);
|
||||
// Track persistence locally (not via `savedFlowId` state) because a
|
||||
// `setState` call doesn't apply synchronously — reading `savedFlowId`
|
||||
// itself in the `catch` below would see this render's stale value, not
|
||||
// what just happened in this attempt.
|
||||
let flowId = savedFlowId;
|
||||
let flowPersisted = flowId !== null;
|
||||
try {
|
||||
await createFlow(proposal.name, proposal.graph, proposal.requireApproval);
|
||||
if (!flowId) {
|
||||
const flow = await createFlow(proposal.name, proposal.graph, proposal.requireApproval);
|
||||
flowId = flow.id;
|
||||
flowPersisted = true;
|
||||
if (flow.enabled) {
|
||||
log('save: createFlow returned enabled — nothing further to arm id=%s', flow.id);
|
||||
dispatch(clearWorkflowProposalForThread({ threadId }));
|
||||
onSaved?.();
|
||||
return;
|
||||
}
|
||||
// B29 Rule 1 saved this automatic-trigger flow disabled. This click
|
||||
// is the user's own explicit "Save & enable" — not the copilot's
|
||||
// silent autosave Rule 1 guards against — so arm it now.
|
||||
log('save: createFlow returned disabled (Rule 1) — arming explicitly id=%s', flow.id);
|
||||
setSavedFlowId(flow.id);
|
||||
}
|
||||
await setFlowEnabled(flowId, true);
|
||||
dispatch(clearWorkflowProposalForThread({ threadId }));
|
||||
onSaved?.();
|
||||
} catch (e) {
|
||||
log('createFlow failed: %o', e);
|
||||
setErrorMsg(t('chat.flowProposal.error'));
|
||||
log('save failed (createFlow/setFlowEnabled): %o', e);
|
||||
setErrorMsg(
|
||||
flowPersisted ? t('chat.flowProposal.enableError') : t('chat.flowProposal.error')
|
||||
);
|
||||
setSaving(false);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -3175,6 +3175,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'فتح في اللوحة',
|
||||
'chat.flowProposal.dismiss': 'إغلاق',
|
||||
'chat.flowProposal.error': 'تعذّر حفظ سير العمل. حاول مرة أخرى.',
|
||||
'chat.flowProposal.enableError':
|
||||
'تم حفظ سير العمل، لكن تعذّر تفعيله. حاول مرة أخرى، أو فعّله من صفحة سير العمل.',
|
||||
'channels.authMode.managed_dm': 'قم بتسجيل الدخول باستخدام OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth تسجيل الدخول',
|
||||
'channels.authMode.bot_token': 'استخدم رمز الروبوت الخاص بك',
|
||||
|
||||
@@ -3244,6 +3244,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'ক্যানভাসে খুলুন',
|
||||
'chat.flowProposal.dismiss': 'খারিজ করুন',
|
||||
'chat.flowProposal.error': 'ওয়ার্কফ্লো সংরক্ষণ করা যায়নি। আবার চেষ্টা করুন।',
|
||||
'chat.flowProposal.enableError':
|
||||
'ওয়ার্কফ্লো সংরক্ষিত হয়েছে, কিন্তু সক্ষম করা যায়নি। আবার চেষ্টা করুন, বা Workflows পৃষ্ঠা থেকে সক্ষম করুন।',
|
||||
'channels.authMode.managed_dm': 'OpenHuman দিয়ে লগইন করুন',
|
||||
'channels.authMode.oauth': 'OAuth সাইন-ইন করুন',
|
||||
'channels.authMode.bot_token': 'আপনার নিজের বট টোকেন ব্যবহার করুন',
|
||||
|
||||
@@ -3326,6 +3326,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.dismiss': 'Verwerfen',
|
||||
'chat.flowProposal.error':
|
||||
'Der Workflow konnte nicht gespeichert werden. Bitte versuchen Sie es erneut.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Workflow gespeichert, konnte aber nicht aktiviert werden. Versuchen Sie es erneut oder aktivieren Sie ihn auf der Workflows-Seite.',
|
||||
'channels.authMode.managed_dm': 'Mit OpenHuman anmelden',
|
||||
'channels.authMode.oauth': 'OAuth-Anmeldung',
|
||||
'channels.authMode.bot_token': 'Eigenen Bot-Token verwenden',
|
||||
|
||||
@@ -3660,6 +3660,8 @@ const en: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Open in canvas',
|
||||
'chat.flowProposal.dismiss': 'Dismiss',
|
||||
'chat.flowProposal.error': 'Could not save the workflow. Please try again.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Workflow saved, but could not enable it. Try again, or enable it from the Workflows page.',
|
||||
|
||||
// Auth mode labels
|
||||
'channels.authMode.managed_dm': 'Login with OpenHuman',
|
||||
|
||||
@@ -3301,6 +3301,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Abrir en el lienzo',
|
||||
'chat.flowProposal.dismiss': 'Descartar',
|
||||
'chat.flowProposal.error': 'No se pudo guardar el flujo de trabajo. Inténtalo de nuevo.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Flujo de trabajo guardado, pero no se pudo activar. Inténtalo de nuevo o actívalo desde la página de Workflows.',
|
||||
'channels.authMode.managed_dm': 'Iniciar sesión con OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Iniciar sesión',
|
||||
'channels.authMode.bot_token': 'Utilice su propio token de bot',
|
||||
|
||||
@@ -3315,6 +3315,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Ouvrir dans le canevas',
|
||||
'chat.flowProposal.dismiss': 'Ignorer',
|
||||
'chat.flowProposal.error': "Impossible d'enregistrer le workflow. Veuillez réessayer.",
|
||||
'chat.flowProposal.enableError':
|
||||
"Workflow enregistré, mais impossible de l'activer. Réessayez, ou activez-le depuis la page Workflows.",
|
||||
'channels.authMode.managed_dm': 'Connectez-vous avec OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Connectez-vous',
|
||||
'channels.authMode.bot_token': 'Utiliser votre propre jeton de robot',
|
||||
|
||||
@@ -3243,6 +3243,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'कैनवास में खोलें',
|
||||
'chat.flowProposal.dismiss': 'खारिज करें',
|
||||
'chat.flowProposal.error': 'वर्कफ़्लो सहेजा नहीं जा सका। कृपया फिर से प्रयास करें।',
|
||||
'chat.flowProposal.enableError':
|
||||
'वर्कफ़्लो सहेजा गया, लेकिन सक्षम नहीं किया जा सका। फिर से प्रयास करें, या Workflows पेज से इसे सक्षम करें।',
|
||||
'channels.authMode.managed_dm': 'OpenHuman से लॉगिन करें',
|
||||
'channels.authMode.oauth': 'OAuth साइन-इन करें',
|
||||
'channels.authMode.bot_token': 'अपने स्वयं के बॉट टोकन का उपयोग करें',
|
||||
|
||||
@@ -3256,6 +3256,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Buka di kanvas',
|
||||
'chat.flowProposal.dismiss': 'Abaikan',
|
||||
'chat.flowProposal.error': 'Alur kerja tidak dapat disimpan. Silakan coba lagi.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Alur kerja disimpan, tetapi tidak dapat diaktifkan. Coba lagi, atau aktifkan dari halaman Workflows.',
|
||||
'channels.authMode.managed_dm': 'Masuk dengan OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Masuk',
|
||||
'channels.authMode.bot_token': 'Gunakan Token Bot Anda sendiri',
|
||||
|
||||
@@ -3297,6 +3297,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Apri nel canvas',
|
||||
'chat.flowProposal.dismiss': 'Ignora',
|
||||
'chat.flowProposal.error': 'Impossibile salvare il workflow. Riprova.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Workflow salvato, ma non è stato possibile attivarlo. Riprova, oppure attivalo dalla pagina Workflows.',
|
||||
'channels.authMode.managed_dm': 'Accedi con OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Accedi',
|
||||
'channels.authMode.bot_token': 'Usa il tuo token Bot',
|
||||
|
||||
@@ -3212,6 +3212,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': '캔버스에서 열기',
|
||||
'chat.flowProposal.dismiss': '닫기',
|
||||
'chat.flowProposal.error': '워크플로를 저장할 수 없습니다. 다시 시도하세요.',
|
||||
'chat.flowProposal.enableError':
|
||||
'워크플로는 저장되었지만 활성화할 수 없습니다. 다시 시도하거나 Workflows 페이지에서 활성화하세요.',
|
||||
'channels.authMode.managed_dm': 'OpenHuman로 로그인',
|
||||
'channels.authMode.oauth': 'OAuth 로그인',
|
||||
'channels.authMode.bot_token': '자체 봇 토큰 사용',
|
||||
|
||||
@@ -3281,6 +3281,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Otwórz na kanwie',
|
||||
'chat.flowProposal.dismiss': 'Odrzuć',
|
||||
'chat.flowProposal.error': 'Nie udało się zapisać przepływu pracy. Spróbuj ponownie.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Przepływ pracy zapisany, ale nie udało się go włączyć. Spróbuj ponownie lub włącz go na stronie Workflows.',
|
||||
'channels.authMode.managed_dm': 'Zaloguj się z OpenHuman',
|
||||
'channels.authMode.oauth': 'Logowanie OAuth',
|
||||
'channels.authMode.bot_token': 'Użyj własnego tokena bota',
|
||||
|
||||
@@ -3299,6 +3299,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Abrir no canvas',
|
||||
'chat.flowProposal.dismiss': 'Dispensar',
|
||||
'chat.flowProposal.error': 'Não foi possível salvar o fluxo de trabalho. Tente novamente.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Fluxo de trabalho salvo, mas não foi possível ativá-lo. Tente novamente ou ative-o na página Workflows.',
|
||||
'channels.authMode.managed_dm': 'Faça login com OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Faça login',
|
||||
'channels.authMode.bot_token': 'Use seu próprio token de bot',
|
||||
|
||||
@@ -3271,6 +3271,8 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': 'Открыть на холсте',
|
||||
'chat.flowProposal.dismiss': 'Скрыть',
|
||||
'chat.flowProposal.error': 'Не удалось сохранить рабочий процесс. Попробуйте еще раз.',
|
||||
'chat.flowProposal.enableError':
|
||||
'Рабочий процесс сохранён, но не удалось его включить. Попробуйте ещё раз или включите его на странице Workflows.',
|
||||
'channels.authMode.managed_dm': 'Войдите с помощью OpenHuman',
|
||||
'channels.authMode.oauth': 'OAuth Вход в систему',
|
||||
'channels.authMode.bot_token': 'Используйте свой собственный токен бота',
|
||||
|
||||
@@ -3078,6 +3078,7 @@ const messages: TranslationMap = {
|
||||
'chat.flowProposal.openInCanvas': '在画布中打开',
|
||||
'chat.flowProposal.dismiss': '忽略',
|
||||
'chat.flowProposal.error': '无法保存该工作流。请重试。',
|
||||
'chat.flowProposal.enableError': '工作流已保存,但无法启用。请重试,或在“工作流”页面手动启用。',
|
||||
'channels.authMode.managed_dm': '使用 OpenHuman 登录',
|
||||
'channels.authMode.oauth': 'OAuth 登录',
|
||||
'channels.authMode.bot_token': '使用你自己的 Bot Token',
|
||||
|
||||
@@ -253,6 +253,18 @@ function unwrapCliEnvelope<T>(payload: unknown): T {
|
||||
* `false` when omitted, but the B4 proposal flow always passes it explicitly
|
||||
* (defaulting to `true` on the Rust tool side) so a saved agent-proposed flow
|
||||
* starts with its outbound-action approval gate on.
|
||||
*
|
||||
* B29 (save/enable safety) Rule 1: when `graph`'s trigger fires without a
|
||||
* human in the loop (`schedule` / `app_event` / `webhook`), the server
|
||||
* ALWAYS persists the flow `enabled: false`, regardless of what the caller
|
||||
* intended — no creation path may silently hand back an armed, unattended
|
||||
* automation. The returned {@link Flow}'s `enabled` field reflects this, so
|
||||
* every caller MUST check it: if the caller represents an explicit
|
||||
* user-arming action (e.g. `WorkflowProposalCard`'s "Save & enable" click),
|
||||
* follow up with {@link setFlowEnabled} to actually arm it — that is a
|
||||
* legitimate explicit enable, not the silent copilot auto-arm Rule 1 guards
|
||||
* against. A caller with no such explicit intent (e.g. a background/copilot
|
||||
* save) should leave it disabled and let the user arm it later.
|
||||
*/
|
||||
export async function createFlow(
|
||||
name: string,
|
||||
|
||||
@@ -55,6 +55,7 @@ use tinyflows::model::WorkflowGraph;
|
||||
use crate::openhuman::config::Config;
|
||||
use crate::openhuman::flows::ops;
|
||||
use crate::openhuman::flows::ops::validate_and_migrate_graph;
|
||||
use crate::openhuman::flows::tools;
|
||||
use crate::openhuman::security::{AutonomyLevel, SecurityPolicy};
|
||||
use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolResult};
|
||||
|
||||
@@ -1847,6 +1848,34 @@ impl Tool for SaveWorkflowTool {
|
||||
enabled = flow.enabled,
|
||||
"[flows] save_workflow: persisted"
|
||||
);
|
||||
// Issue B29 (save/enable safety), Rule 3: `flows_create` only
|
||||
// gates the FIRST creation of a flow — an agent `save_workflow`
|
||||
// targets an EXISTING flow via `flows_update`, which preserves
|
||||
// whatever `enabled` state the flow already had. If the user
|
||||
// already armed this flow (enabled it) and it has an automatic
|
||||
// trigger, saving a new graph onto it re-arms it live with no
|
||||
// further confirmation. Surface that loudly so the copilot
|
||||
// relays it to the user instead of staying silent.
|
||||
if flow.enabled && ops::trigger_is_automatic(&flow.graph) {
|
||||
let trigger_desc = flow
|
||||
.graph
|
||||
.trigger()
|
||||
.map(tools::describe_trigger)
|
||||
.unwrap_or_else(|| "automatic".to_string());
|
||||
let warning = format!(
|
||||
"WARNING: this flow is ENABLED with an automatic trigger \
|
||||
({trigger_desc}). It is now LIVE and will fire on its own — tell the \
|
||||
user, and offer to disable it (flows_set_enabled) if that's not what \
|
||||
they intended."
|
||||
);
|
||||
tracing::warn!(
|
||||
target: "flows",
|
||||
%flow_id,
|
||||
trigger = %trigger_desc,
|
||||
"[flows] save_workflow: saved onto an enabled auto-trigger flow — now LIVE"
|
||||
);
|
||||
warnings.push(warning);
|
||||
}
|
||||
Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
|
||||
"type": "workflow_saved",
|
||||
"flow_id": flow.id,
|
||||
|
||||
+132
-11
@@ -130,6 +130,52 @@ fn trigger_kind_fires(kind: &TriggerKind) -> bool {
|
||||
)
|
||||
}
|
||||
|
||||
/// Whether `graph`'s trigger fires **without a human in the loop** — i.e. on
|
||||
/// a timer, an inbound webhook, or a connected-app event, as opposed to
|
||||
/// `manual` (only ever fired by an explicit `flows_run`). Used by
|
||||
/// [`flows_create`] (issue B29 — save/enable safety, Rule 1) to decide
|
||||
/// whether a freshly-saved flow may persist `enabled: true` or must persist
|
||||
/// `enabled: false` until the user arms it explicitly via
|
||||
/// `flows_set_enabled`.
|
||||
///
|
||||
/// Deliberately broader than [`trigger_kind_fires`]: `webhook` is not yet
|
||||
/// wired to auto-dispatch in this host (see that fn's doc), but it WILL fire
|
||||
/// unattended the moment it is — so a webhook-trigger flow must not be handed
|
||||
/// to the user pre-armed either. Returns `false` for a graph with no single
|
||||
/// resolvable trigger node or no `trigger_kind` discriminator (never a
|
||||
/// surprise — it never self-fires).
|
||||
pub(crate) fn trigger_is_automatic(graph: &WorkflowGraph) -> bool {
|
||||
let Some(trigger) = graph.trigger() else {
|
||||
return false;
|
||||
};
|
||||
let Some(kind_value) = trigger.config.get("trigger_kind") else {
|
||||
return false;
|
||||
};
|
||||
let Ok(kind) = serde_json::from_value::<TriggerKind>(kind_value.clone()) else {
|
||||
return false;
|
||||
};
|
||||
matches!(
|
||||
kind,
|
||||
TriggerKind::Schedule | TriggerKind::AppEvent | TriggerKind::Webhook
|
||||
)
|
||||
}
|
||||
|
||||
/// Whether `graph` contains a node that can produce a real outbound side
|
||||
/// effect — `tool_call` (a curated integration action), `http_request`, or
|
||||
/// `code` (sandboxed but Turing-complete, can reach the network). Used by
|
||||
/// [`flows_create`] (issue B29, Rule 2) to force `require_approval: true` on
|
||||
/// any graph that can act on the world, regardless of what the caller
|
||||
/// passed. A graph built only from `trigger` / `agent` / `transform` /
|
||||
/// `condition` / data-flow nodes is read-only and unaffected.
|
||||
pub(crate) fn graph_has_outbound_side_effect(graph: &WorkflowGraph) -> bool {
|
||||
graph.nodes.iter().any(|n| {
|
||||
matches!(
|
||||
n.kind,
|
||||
NodeKind::ToolCall | NodeKind::HttpRequest | NodeKind::Code
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
/// Produces host-side, **non-fatal** validation warnings for a graph — today
|
||||
/// exactly one: "this trigger kind does not fire automatically yet". Returns
|
||||
/// an empty vec when the trigger fires (`manual`/`schedule`/`app_event`), when
|
||||
@@ -1462,13 +1508,42 @@ pub fn flows_import(
|
||||
|
||||
/// Creates a new flow from a name and a raw graph JSON value.
|
||||
///
|
||||
/// `store::create_flow` defaults new flows to `enabled = true` — this binds
|
||||
/// the flow's automatic-dispatch side effect (e.g. registers the
|
||||
/// schedule-trigger cron job) immediately, reusing the same [`bind_trigger`]
|
||||
/// helper `flows_set_enabled` uses. Without this, a freshly-created enabled
|
||||
/// schedule flow would silently never fire until an app restart (boot
|
||||
/// reconcile) or a manual disable→enable toggle. Best-effort, same as
|
||||
/// `flows_set_enabled`: a binding failure is logged, not fatal to create.
|
||||
/// Issue B29 (save/enable safety) — two server-side rules apply here,
|
||||
/// authoritative regardless of what the caller passed, so no creation path
|
||||
/// (prompt bar, scratch/template modal, proposal "save & enable", copilot
|
||||
/// `save_workflow`, …) can silently hand the user an armed, unattended
|
||||
/// automation:
|
||||
///
|
||||
/// - **Rule 1** ([`trigger_is_automatic`]): a graph whose trigger fires
|
||||
/// without a human in the loop (`schedule` / `app_event` / `webhook`)
|
||||
/// persists **disabled**. The user arms it explicitly via
|
||||
/// `flows_set_enabled` — the same toggle already used everywhere else. A
|
||||
/// `manual` trigger (or no trigger-kind discriminator at all) still
|
||||
/// persists enabled: it only ever runs via an explicit `flows_run`, so
|
||||
/// there is no surprise, and gating it would just add friction.
|
||||
///
|
||||
/// This means a caller that represents an explicit user-arming action
|
||||
/// (e.g. `WorkflowProposalCard`'s "Save & enable" click,
|
||||
/// `app/src/components/chat/WorkflowProposalCard.tsx`) must check the
|
||||
/// returned [`Flow`]'s `enabled` field and follow up with
|
||||
/// `flows_set_enabled(id, true)` when it comes back `false` — otherwise
|
||||
/// the button's own label lies to the user. That follow-up call is a
|
||||
/// legitimate, explicit enable, not the silent copilot auto-arm this rule
|
||||
/// exists to prevent (the copilot's `save_workflow` path has no such
|
||||
/// follow-up and stays disabled).
|
||||
/// - **Rule 2** ([`graph_has_outbound_side_effect`]): a graph containing any
|
||||
/// `tool_call` / `http_request` / `code` node — the three kinds that can
|
||||
/// produce a real outbound effect — forces `require_approval: true`,
|
||||
/// overriding whatever the caller passed. A read-only graph (only
|
||||
/// `trigger` / `agent` / `transform` / `condition` / data-flow nodes) is
|
||||
/// unaffected.
|
||||
///
|
||||
/// An enabled flow still has its automatic-dispatch side effect bound
|
||||
/// immediately (e.g. the schedule-trigger cron job registered), reusing the
|
||||
/// same [`bind_trigger`] helper `flows_set_enabled` uses — but per Rule 1
|
||||
/// that now only happens for a `manual`-triggered (or trigger-kind-less)
|
||||
/// flow. Best-effort, same as `flows_set_enabled`: a binding failure is
|
||||
/// logged, not fatal to create.
|
||||
pub async fn flows_create(
|
||||
config: &Config,
|
||||
name: String,
|
||||
@@ -1476,16 +1551,62 @@ pub async fn flows_create(
|
||||
require_approval: bool,
|
||||
) -> Result<RpcOutcome<Flow>, String> {
|
||||
let graph = validate_and_migrate_graph(graph_json)?;
|
||||
tracing::debug!(target: "flows", %name, node_count = graph.nodes.len(), require_approval, "[flows] flows_create: persisting new flow");
|
||||
let flow =
|
||||
store::create_flow(config, name, graph, require_approval).map_err(|e| e.to_string())?;
|
||||
|
||||
// Rule 1: automatic triggers create DISABLED — the user must arm them
|
||||
// explicitly.
|
||||
let enabled = !trigger_is_automatic(&graph);
|
||||
|
||||
// Rule 2: any outbound side-effect node forces require_approval, no
|
||||
// matter what the caller asked for.
|
||||
let has_side_effect = graph_has_outbound_side_effect(&graph);
|
||||
let effective_require_approval = require_approval || has_side_effect;
|
||||
if has_side_effect && !require_approval {
|
||||
tracing::info!(
|
||||
target: "flows",
|
||||
%name,
|
||||
"[flows] flows_create: forcing require_approval=true — graph contains outbound \
|
||||
side-effect node(s) (tool_call / http_request / code)"
|
||||
);
|
||||
}
|
||||
|
||||
tracing::debug!(
|
||||
target: "flows",
|
||||
%name,
|
||||
node_count = graph.nodes.len(),
|
||||
enabled,
|
||||
require_approval = effective_require_approval,
|
||||
"[flows] flows_create: persisting new flow"
|
||||
);
|
||||
let flow = store::create_flow(config, name, graph, effective_require_approval, enabled)
|
||||
.map_err(|e| e.to_string())?;
|
||||
|
||||
if flow.enabled {
|
||||
tracing::debug!(target: "flows", flow_id = %flow.id, "[flows] flows_create: flow is enabled — binding automatic-dispatch trigger");
|
||||
bind_trigger(config, &flow);
|
||||
}
|
||||
|
||||
Ok(RpcOutcome::single_log(flow, "flow created"))
|
||||
let mut logs = vec!["flow created".to_string()];
|
||||
if !enabled {
|
||||
let trigger_label = flow
|
||||
.graph
|
||||
.trigger()
|
||||
.and_then(|t| t.config.get("trigger_kind"))
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or("automatic");
|
||||
logs.push(format!(
|
||||
"Flow created DISABLED because it has an automatic trigger ({trigger_label}). \
|
||||
Enable it explicitly (flows_set_enabled) when you are ready for it to fire."
|
||||
));
|
||||
}
|
||||
if effective_require_approval && !require_approval {
|
||||
logs.push(
|
||||
"require_approval forced to true because the graph contains outbound side-effect \
|
||||
nodes (tool_call / http_request / code)."
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(RpcOutcome::new(flow, logs))
|
||||
}
|
||||
|
||||
/// Duplicates a saved flow: creates an independent copy of its graph under a
|
||||
|
||||
@@ -362,15 +362,21 @@ async fn flows_run_populates_error_when_a_continue_policy_node_errors() {
|
||||
assert_eq!(reloaded.value.last_status.as_deref(), Some("failed"));
|
||||
}
|
||||
|
||||
// ── automatic-dispatch binding (issue B2 finding #1) ─────────────────────
|
||||
// ── automatic-dispatch binding (issue B2 finding #1, revised by B29) ──────
|
||||
//
|
||||
// Live testing found that `flows_create` persisted a freshly-created,
|
||||
// `enabled = true` schedule flow WITHOUT registering its cron job — only
|
||||
// `flows_set_enabled` bound it. So a brand-new enabled schedule flow would
|
||||
// silently never fire until an app restart (boot reconcile) or a manual
|
||||
// disable→enable toggle. These tests exercise the fix directly against the
|
||||
// real `cron` store (not a mock), the same way `bind_schedule_trigger`
|
||||
// itself does.
|
||||
// disable→enable toggle.
|
||||
//
|
||||
// Issue B29 (save/enable safety) then found the OTHER half of that same bug:
|
||||
// `flows_create` used to default a schedule flow straight to `enabled: true`
|
||||
// on create, arming it live before the user ever saw a toggle. Rule 1 now
|
||||
// creates an automatic-trigger flow DISABLED — so these tests explicitly
|
||||
// enable via `flows_set_enabled` (the real caller-facing arming path) before
|
||||
// exercising the cron-binding behavior below, against the real `cron` store
|
||||
// (not a mock), the same way `bind_schedule_trigger` itself does.
|
||||
|
||||
fn schedule_trigger_graph(cron_expr: &str) -> Value {
|
||||
json!({
|
||||
@@ -400,13 +406,27 @@ async fn flows_create_binds_schedule_cron_job_for_an_enabled_flow() {
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(created.value.enabled, "flows_create defaults to enabled");
|
||||
assert!(
|
||||
!created.value.enabled,
|
||||
"issue B29: a schedule-trigger flow must create DISABLED, not armed"
|
||||
);
|
||||
assert!(
|
||||
crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id)
|
||||
.unwrap()
|
||||
.is_none(),
|
||||
"a disabled-on-create schedule flow must not have its cron job bound yet"
|
||||
);
|
||||
|
||||
// The user arms it explicitly — this is where the cron job binds.
|
||||
let enabled = flows_set_enabled(&config, &created.value.id, true)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(enabled.value.enabled);
|
||||
|
||||
let job = crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id).unwrap();
|
||||
assert!(
|
||||
job.is_some(),
|
||||
"an enabled schedule flow must have its cron job bound immediately on create, not only \
|
||||
after a set_enabled toggle"
|
||||
"an enabled schedule flow must have its cron job bound immediately on enable"
|
||||
);
|
||||
assert_eq!(job.unwrap().expression, "0 9 * * *");
|
||||
}
|
||||
@@ -423,11 +443,14 @@ async fn flows_delete_unbinds_schedule_cron_job() {
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
flows_set_enabled(&config, &created.value.id, true)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id)
|
||||
.unwrap()
|
||||
.is_some(),
|
||||
"precondition: cron job bound on create"
|
||||
"precondition: cron job bound on enable"
|
||||
);
|
||||
|
||||
flows_delete(&config, &created.value.id).await.unwrap();
|
||||
@@ -453,9 +476,12 @@ async fn flows_update_rebinds_schedule_cron_job_when_trigger_schedule_changes()
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
flows_set_enabled(&config, &created.value.id, true)
|
||||
.await
|
||||
.unwrap();
|
||||
let old_job = crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id)
|
||||
.unwrap()
|
||||
.expect("cron job bound on create");
|
||||
.expect("cron job bound on enable");
|
||||
assert_eq!(old_job.expression, "0 9 * * *");
|
||||
|
||||
flows_update(
|
||||
@@ -497,9 +523,12 @@ async fn flows_update_does_not_rebind_when_graph_is_not_supplied() {
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
flows_set_enabled(&config, &created.value.id, true)
|
||||
.await
|
||||
.unwrap();
|
||||
let old_job = crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id)
|
||||
.unwrap()
|
||||
.expect("cron job bound on create");
|
||||
.expect("cron job bound on enable");
|
||||
|
||||
// Name-only update: no graph_json supplied, so the trigger cannot have
|
||||
// changed — the existing binding must be left untouched.
|
||||
@@ -1446,7 +1475,9 @@ async fn flows_set_enabled_surfaces_unfired_trigger_warning_at_enable() {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
// Re-enable (create already enables) to exercise the enable path's warning.
|
||||
// A webhook trigger is automatic (B29 Rule 1) so `flows_create` leaves it
|
||||
// disabled — enable it explicitly here to exercise the enable path's
|
||||
// warning.
|
||||
let enabled = flows_set_enabled(&config, &created.value.id, true)
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -3195,3 +3226,356 @@ async fn flows_create_rejects_condition_edges_with_branch_label_on_to_port() {
|
||||
"expected an InvalidConditionRouting-style error, got: {err}"
|
||||
);
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Issue B29 — save/enable safety: `flows_create` gating (Rule 1 + Rule 2)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
//
|
||||
// Saving a scheduled/automatic flow used to silently arm it live and
|
||||
// unattended: `store::create_flow` hardcoded `enabled: true`, and
|
||||
// `require_approval` defaulted to `false` on most creation paths. These
|
||||
// tests exercise the two server-side rules `flows_create` now enforces,
|
||||
// regardless of what the caller passed.
|
||||
|
||||
fn app_event_trigger_graph() -> Value {
|
||||
json!({
|
||||
"name": "app-event",
|
||||
"nodes": [
|
||||
{
|
||||
"id": "t",
|
||||
"kind": "trigger",
|
||||
"name": "Trigger",
|
||||
"config": { "trigger_kind": "app_event", "toolkit": "gmail", "event": "GMAIL_NEW_GMAIL_MESSAGE" }
|
||||
}
|
||||
],
|
||||
"edges": []
|
||||
})
|
||||
}
|
||||
|
||||
fn manual_trigger_graph() -> Value {
|
||||
json!({
|
||||
"name": "manual",
|
||||
"nodes": [
|
||||
{
|
||||
"id": "t",
|
||||
"kind": "trigger",
|
||||
"name": "Trigger",
|
||||
"config": { "trigger_kind": "manual" }
|
||||
}
|
||||
],
|
||||
"edges": []
|
||||
})
|
||||
}
|
||||
|
||||
fn tool_call_graph() -> Value {
|
||||
json!({
|
||||
"name": "with-tool-call",
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Trigger" },
|
||||
{
|
||||
"id": "post",
|
||||
"kind": "tool_call",
|
||||
"name": "Post",
|
||||
"config": { "slug": "SLACK_SEND_MESSAGE", "args": { "channel": "general" } }
|
||||
}
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "post" } ]
|
||||
})
|
||||
}
|
||||
|
||||
fn http_request_graph() -> Value {
|
||||
json!({
|
||||
"name": "with-http",
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Trigger" },
|
||||
{
|
||||
"id": "call",
|
||||
"kind": "http_request",
|
||||
"name": "Call",
|
||||
"config": { "method": "GET", "url": "https://example.com" }
|
||||
}
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "call" } ]
|
||||
})
|
||||
}
|
||||
|
||||
fn code_graph() -> Value {
|
||||
json!({
|
||||
"name": "with-code",
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Trigger" },
|
||||
{
|
||||
"id": "run",
|
||||
"kind": "code",
|
||||
"name": "Run",
|
||||
"config": { "language": "javascript", "source": "return {};" }
|
||||
}
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "run" } ]
|
||||
})
|
||||
}
|
||||
|
||||
fn readonly_graph() -> Value {
|
||||
json!({
|
||||
"name": "readonly",
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Trigger" },
|
||||
{ "id": "a", "kind": "agent", "name": "Summarize", "config": { "prompt": "hi" } },
|
||||
{ "id": "x", "kind": "transform", "name": "Reshape", "config": { "expression": "=item" } }
|
||||
],
|
||||
"edges": [
|
||||
{ "from_node": "t", "to_node": "a" },
|
||||
{ "from_node": "a", "to_node": "x" }
|
||||
]
|
||||
})
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_schedule_trigger_creates_disabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(
|
||||
&config,
|
||||
"scheduled".to_string(),
|
||||
schedule_trigger_graph("30 7 * * 1-5"),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
!created.value.enabled,
|
||||
"a schedule-trigger flow must create disabled"
|
||||
);
|
||||
assert!(
|
||||
crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id)
|
||||
.unwrap()
|
||||
.is_none(),
|
||||
"no cron job may be bound for a disabled-on-create schedule flow"
|
||||
);
|
||||
assert!(
|
||||
created
|
||||
.logs
|
||||
.iter()
|
||||
.any(|l| l.starts_with("Flow created DISABLED")),
|
||||
"flows_create must loudly log the disabled-on-create decision: {:?}",
|
||||
created.logs
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_app_event_trigger_creates_disabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(
|
||||
&config,
|
||||
"app-event".to_string(),
|
||||
app_event_trigger_graph(),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
!created.value.enabled,
|
||||
"an app_event-trigger flow must create disabled"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_manual_trigger_creates_enabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(&config, "manual".to_string(), manual_trigger_graph(), false)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
created.value.enabled,
|
||||
"a manual-trigger flow only ever fires via explicit flows_run — it must create enabled"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_no_trigger_kind_creates_enabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(&config, "legacy".to_string(), trigger_only_graph(), false)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
created.value.enabled,
|
||||
"a trigger with no trigger_kind discriminator never self-fires — not a surprise, must \
|
||||
create enabled"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_outbound_node_forces_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(&config, "tool-flow".to_string(), tool_call_graph(), false)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
created.value.require_approval,
|
||||
"a graph with a tool_call node must force require_approval, even though the caller \
|
||||
passed false"
|
||||
);
|
||||
assert!(
|
||||
created
|
||||
.logs
|
||||
.iter()
|
||||
.any(|l| l.contains("require_approval forced to true")),
|
||||
"flows_create must loudly log the forced require_approval: {:?}",
|
||||
created.logs
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_outbound_http_forces_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(
|
||||
&config,
|
||||
"http-flow".to_string(),
|
||||
http_request_graph(),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
created.value.require_approval,
|
||||
"a graph with an http_request node must force require_approval"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_outbound_code_forces_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(&config, "code-flow".to_string(), code_graph(), false)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
created.value.require_approval,
|
||||
"a graph with a code node must force require_approval"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_readonly_graph_respects_caller_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let created = flows_create(
|
||||
&config,
|
||||
"readonly-flow".to_string(),
|
||||
readonly_graph(),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
!created.value.require_approval,
|
||||
"a read-only graph (no tool_call/http_request/code) must not have require_approval \
|
||||
forced — the caller's choice stands"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn flows_create_schedule_outbound_creates_disabled_and_approval() {
|
||||
// The exact bug scenario from the ticket: a scheduled flow that posts to
|
||||
// Slack, saved with `require_approval: false` — it must come back BOTH
|
||||
// disabled AND with require_approval forced true.
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let graph = json!({
|
||||
"name": "scheduled-slack-post",
|
||||
"nodes": [
|
||||
{
|
||||
"id": "t",
|
||||
"kind": "trigger",
|
||||
"name": "Trigger",
|
||||
"config": { "trigger_kind": "schedule", "schedule": "30 7 * * 1-5" }
|
||||
},
|
||||
{
|
||||
"id": "post",
|
||||
"kind": "tool_call",
|
||||
"name": "Post",
|
||||
"config": { "slug": "SLACK_SEND_MESSAGE", "args": { "channel": "general" } }
|
||||
}
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "post" } ]
|
||||
});
|
||||
|
||||
let created = flows_create(&config, "scheduled-slack".to_string(), graph, false)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
!created.value.enabled,
|
||||
"a scheduled flow with an outbound node must still create disabled (Rule 1)"
|
||||
);
|
||||
assert!(
|
||||
created.value.require_approval,
|
||||
"a scheduled flow with an outbound node must force require_approval (Rule 2)"
|
||||
);
|
||||
}
|
||||
|
||||
// ── graph_has_outbound_side_effect / trigger_is_automatic helper tests ────
|
||||
|
||||
#[test]
|
||||
fn graph_has_outbound_side_effect_detects_tool_call() {
|
||||
let g = graph(tool_call_graph());
|
||||
assert!(graph_has_outbound_side_effect(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn graph_has_outbound_side_effect_detects_http_request() {
|
||||
let g = graph(http_request_graph());
|
||||
assert!(graph_has_outbound_side_effect(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn graph_has_outbound_side_effect_detects_code() {
|
||||
let g = graph(code_graph());
|
||||
assert!(graph_has_outbound_side_effect(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn graph_has_outbound_side_effect_false_for_agent_only() {
|
||||
let g = graph(readonly_graph());
|
||||
assert!(!graph_has_outbound_side_effect(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trigger_is_automatic_schedule() {
|
||||
let g = graph(schedule_trigger_graph("0 9 * * *"));
|
||||
assert!(trigger_is_automatic(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trigger_is_automatic_manual() {
|
||||
let g = graph(manual_trigger_graph());
|
||||
assert!(!trigger_is_automatic(&g));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trigger_is_automatic_no_trigger_kind() {
|
||||
let g = graph(trigger_only_graph());
|
||||
assert!(!trigger_is_automatic(&g));
|
||||
}
|
||||
|
||||
@@ -212,17 +212,24 @@ pub fn insert_duplicate_flow(config: &Config, source: &Flow, new_name: String) -
|
||||
|
||||
/// Creates a brand-new [`Flow`] row from a name + validated graph, stamping
|
||||
/// fresh id/timestamps, and returns the persisted record.
|
||||
///
|
||||
/// `enabled` is decided by the caller ([`crate::openhuman::flows::ops::flows_create`],
|
||||
/// issue B29 — save/enable safety): a graph with an automatic trigger
|
||||
/// (`schedule` / `app_event` / `webhook`) is created disabled so it cannot
|
||||
/// silently arm itself live and unattended; a `manual`-triggered graph is
|
||||
/// created enabled since it only ever runs on explicit `flows_run`.
|
||||
pub fn create_flow(
|
||||
config: &Config,
|
||||
name: String,
|
||||
graph: tinyflows::model::WorkflowGraph,
|
||||
require_approval: bool,
|
||||
enabled: bool,
|
||||
) -> Result<Flow> {
|
||||
let now = Utc::now().to_rfc3339();
|
||||
let flow = Flow {
|
||||
id: Uuid::new_v4().to_string(),
|
||||
name,
|
||||
enabled: true,
|
||||
enabled,
|
||||
graph,
|
||||
created_at: now.clone(),
|
||||
updated_at: now,
|
||||
|
||||
@@ -34,7 +34,7 @@ fn create_get_list_delete_roundtrip() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
assert_eq!(flow.name, "demo");
|
||||
assert!(flow.enabled);
|
||||
|
||||
@@ -70,7 +70,7 @@ fn remove_flow_errors_when_not_found() {
|
||||
fn set_enabled_toggles_and_persists() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
assert!(flow.enabled);
|
||||
|
||||
let disabled = set_enabled(&config, &flow.id, false).unwrap();
|
||||
@@ -87,7 +87,7 @@ fn set_enabled_toggles_and_persists() {
|
||||
fn update_flow_graph_bumps_updated_at_and_preserves_created_at() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
let mut new_graph = trigger_graph();
|
||||
new_graph.name = "renamed-graph".to_string();
|
||||
@@ -103,7 +103,7 @@ fn update_flow_graph_bumps_updated_at_and_preserves_created_at() {
|
||||
fn record_run_sets_last_run_fields() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
assert!(flow.last_run_at.is_none());
|
||||
|
||||
record_run(&config, &flow.id, "completed").unwrap();
|
||||
@@ -176,7 +176,7 @@ fn create_flow_persists_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), true).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), true, true).unwrap();
|
||||
assert!(flow.require_approval);
|
||||
|
||||
let reloaded = get_flow(&config, &flow.id).unwrap().unwrap();
|
||||
@@ -187,7 +187,7 @@ fn create_flow_persists_require_approval() {
|
||||
fn update_flow_graph_can_change_require_approval() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
assert!(!flow.require_approval);
|
||||
|
||||
let updated =
|
||||
@@ -229,9 +229,16 @@ fn list_enabled_flows_excludes_disabled() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
|
||||
let enabled_flow = create_flow(&config, "enabled".to_string(), trigger_graph(), false).unwrap();
|
||||
let disabled_flow =
|
||||
create_flow(&config, "disabled".to_string(), trigger_graph(), false).unwrap();
|
||||
let enabled_flow =
|
||||
create_flow(&config, "enabled".to_string(), trigger_graph(), false, true).unwrap();
|
||||
let disabled_flow = create_flow(
|
||||
&config,
|
||||
"disabled".to_string(),
|
||||
trigger_graph(),
|
||||
false,
|
||||
true,
|
||||
)
|
||||
.unwrap();
|
||||
set_enabled(&config, &disabled_flow.id, false).unwrap();
|
||||
|
||||
let enabled = list_enabled_flows(&config).unwrap();
|
||||
@@ -245,7 +252,7 @@ fn list_enabled_flows_excludes_disabled() {
|
||||
fn flow_run_insert_finish_get_round_trip() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
let thread_id = format!("flow:{}:run-1", flow.id);
|
||||
insert_flow_run(
|
||||
@@ -299,7 +306,7 @@ fn flow_run_insert_finish_get_round_trip() {
|
||||
fn finish_flow_run_records_error_on_failure() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
let thread_id = format!("flow:{}:run-2", flow.id);
|
||||
insert_flow_run(
|
||||
&config,
|
||||
@@ -337,8 +344,8 @@ fn get_flow_run_returns_none_for_unknown_id() {
|
||||
fn list_flow_runs_orders_newest_first_and_is_scoped_to_flow() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow_a = create_flow(&config, "a".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow_b = create_flow(&config, "b".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow_a = create_flow(&config, "a".to_string(), trigger_graph(), false, true).unwrap();
|
||||
let flow_b = create_flow(&config, "b".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
insert_flow_run(
|
||||
&config,
|
||||
@@ -385,7 +392,7 @@ fn insert_duplicate_flow_makes_a_disabled_copy_with_new_id_and_same_graph() {
|
||||
// Enabled source with require_approval + a distinctive graph name.
|
||||
let mut graph = trigger_graph();
|
||||
graph.name = "original-graph".to_string();
|
||||
let source = create_flow(&config, "My Flow".to_string(), graph, true).unwrap();
|
||||
let source = create_flow(&config, "My Flow".to_string(), graph, true, true).unwrap();
|
||||
assert!(source.enabled);
|
||||
record_run(&config, &source.id, "completed").unwrap();
|
||||
let source = get_flow(&config, &source.id).unwrap().unwrap();
|
||||
@@ -437,7 +444,7 @@ fn seed_run(config: &Config, flow_id: &str, id: &str, day: u32, status: &str) {
|
||||
fn prune_flow_runs_keeps_newest_n_terminal_runs() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
// 5 completed runs on ascending days.
|
||||
for i in 1..=5 {
|
||||
@@ -456,7 +463,7 @@ fn prune_flow_runs_keeps_newest_n_terminal_runs() {
|
||||
fn prune_flow_runs_never_removes_pending_approval_run() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
// An OLD parked pending_approval run (day 1) plus newer completed runs.
|
||||
seed_run(&config, &flow.id, "parked", 1, "pending_approval");
|
||||
@@ -482,7 +489,7 @@ fn prune_flow_runs_never_removes_pending_approval_run() {
|
||||
fn prune_flow_runs_leaves_running_rows_alone() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
seed_run(&config, &flow.id, "live", 1, "running");
|
||||
for i in 2..=4 {
|
||||
@@ -499,7 +506,7 @@ fn prune_flow_runs_leaves_running_rows_alone() {
|
||||
fn insert_flow_run_auto_prunes_beyond_retention_cap() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
// Seed exactly MAX_FLOW_RUNS_PER_FLOW completed runs.
|
||||
let cap = MAX_FLOW_RUNS_PER_FLOW;
|
||||
@@ -543,7 +550,7 @@ fn insert_flow_run_auto_prunes_beyond_retention_cap() {
|
||||
fn list_flow_runs_respects_limit() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let config = test_config(&tmp);
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false).unwrap();
|
||||
let flow = create_flow(&config, "demo".to_string(), trigger_graph(), false, true).unwrap();
|
||||
|
||||
for i in 0..3 {
|
||||
let id = format!("run-{i}");
|
||||
|
||||
@@ -422,7 +422,11 @@ fn node_kind_str(kind: &NodeKind) -> String {
|
||||
/// One-line human description of a trigger node, for the summary's
|
||||
/// `"trigger"` field — e.g. `"schedule: 0 9 * * *"`, `"app event:
|
||||
/// gmail/GMAIL_NEW_GMAIL_MESSAGE"`, `"manual"`.
|
||||
fn describe_trigger(node: &Node) -> String {
|
||||
///
|
||||
/// `pub(crate)` so [`crate::openhuman::flows::builder_tools::SaveWorkflowTool`]
|
||||
/// reuses it verbatim for its enabled+auto-trigger arming warning (issue
|
||||
/// B29) instead of re-deriving the same human string.
|
||||
pub(crate) fn describe_trigger(node: &Node) -> String {
|
||||
let trigger_kind = node
|
||||
.config
|
||||
.get("trigger_kind")
|
||||
|
||||
Reference in New Issue
Block a user