feat(platform): run_workflow agent tool with approval flow and output step type#718
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Add a new `run_workflow` agent tool that lets the AI agent trigger execution of existing workflow definitions from chat, with mandatory user approval via an approval card before execution begins. Backend: - Add `workflow_run` resource type to approval schema/validators - Create `run_workflow_tool` with org ownership and archived status validation - Add internal mutations for creating/updating workflow run approvals - Add internal action with idempotency guard (executedAt check) - Register tool in tool names and registry - Add public action for frontend approval execution Frontend: - Add WorkflowRunApprovalCard component with approve/reject flow - Add useWorkflowRunApprovals query hook (shares existing subscription) - Integrate into chat-interface, chat-messages, and test chat - Add i18n translations for workflowRunApproval namespace Tests: - 14 unit tests covering handler logic and Zod schema validation Refs #663
… and messageId Pass messageId from agent context through to approval mutations for proper message linking. Add rate limiting for workflow:run, idempotency guards to prevent double-execution, and link workflow_run approvals to messages.
Show real-time running/completed/failed states with elapsed timer, output preview, and link to execution details. Post system messages to the chat thread when workflows complete or fail so the agent can inform the user.
Workflows now support an 'end' step type that explicitly defines output via outputMapping, treating workflows like functions (start=input, end=output, variables=internal state). This prevents leaking internal state (including decrypted secrets) into workflow output. Existing workflows without end nodes continue to work with sanitized variable output as a backward-compatible fallback.
…gine Renames the "end" workflow step type to "output" for clarity, reflecting that this step defines workflow output mapping rather than just termination. Updates types, validators, execution logic, UI components, tests, and agent tool references.
…ions Enforce organization membership verification before executing approved operations and updating approval status. Also improves workflow run approval card with translated status labels, accessibility aria-labels, and safer error handling.
larryro
force-pushed
the
feat/663-run-workflow-tool
branch
from
1e17741 to
60f4ce1
Compare
Contributor
📝 WalkthroughWalkthroughThis PR introduces end-to-end support for a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/platform/convex/agent_tools/workflows/save_workflow_definition_tool.ts (1)
128-135:⚠️ Potential issue | 🟡 MinorUpdate the terminal-step guidance to prefer
outputwhen results must be returned.The new step list includes
output, but the prompt still frames workflow termination aroundnoop. With the new function-style workflow model, that can steer the agent to omit the explicit output mapping step and return nothing fromrun_workflowpaths.💡 Suggested prompt tweak
-- Use 'noop' in nextSteps to gracefully end workflow +- Route terminal branches to an `output` step when the workflow must return data. +- Use 'noop' only for branches that intentionally terminate without returning output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/agent_tools/workflows/save_workflow_definition_tool.ts` around lines 128 - 135, The prompt still emphasizes using 'noop' to terminate workflows which conflicts with the new step type 'output' and causes agents to omit explicit output mapping; update the guidance in the save_workflow_definition_tool prompt to prefer using an explicit output step mapping (stepType: "output") whenever the workflow must return results, mention that 'noop' is only for graceful non-returning ends, and show a short example of an output step and how it maps to run_workflow return paths (reference the stepType list including "output" and the run_workflow return expectations).services/platform/app/features/automations/components/next-steps-editor.tsx (1)
29-38:⚠️ Potential issue | 🟠 MajorNormalize
nextStepswhentransitionKeysshrink to prevent persisting invalid transitions.In
step-create-dialog, a user can set transitions foraction(success/failure), then switch tooutput(no transitions). The NextStepsEditor renders zero controls, but the component never clears the incompatible keys fromvalue. This leaves a hidden outgoing transition on what should be a terminal node. On submit, the invalid state is persisted—theupdateStepmutation acceptsnextStepswithout validating them against the step type.Add a
useEffectin NextStepsEditor to prune keys not intransitionKeys:Suggested fix
'use client'; import { StopCircle } from 'lucide-react'; -import { useMemo } from 'react'; +import { useEffect, useMemo } from 'react'; import { Select } from '@/app/components/ui/forms/select'; import { Stack } from '@/app/components/ui/layout/layout'; import { Doc } from '@/convex/_generated/dataModel'; import { useT } from '@/lib/i18n/client'; import { getStepIcon } from '../utils/step-icons'; interface AvailableStep { stepSlug: string; name: string; stepType?: Doc<'wfStepDefs'>['stepType']; actionType?: string; } interface NextStepsEditorProps { stepType: Doc<'wfStepDefs'>['stepType']; value: Record<string, string>; onChange: (value: Record<string, string>) => void; stepOptions: AvailableStep[]; currentStepSlug?: string; disabled?: boolean; } const TRANSITION_KEYS_BY_TYPE: Record<Doc<'wfStepDefs'>['stepType'], string[]> = { start: ['success'], trigger: ['success'], llm: ['success', 'failure'], condition: ['true', 'false'], action: ['success', 'failure'], loop: ['loop', 'done'], output: [], }; export function NextStepsEditor({ stepType, value, onChange, stepOptions, currentStepSlug, disabled = false, }: NextStepsEditorProps) { const { t } = useT('automations'); const transitionKeys = TRANSITION_KEYS_BY_TYPE[stepType] || ['success']; + useEffect(() => { + const normalized = Object.fromEntries( + Object.entries(value).filter(([key]) => transitionKeys.includes(key)), + ); + + if (Object.keys(normalized).length !== Object.keys(value).length) { + onChange(normalized); + } + }, [onChange, transitionKeys, value]); const selectOptions = useMemo(() => {
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx`:
- Around line 130-131: The hook useApprovalColumns currently treats
'workflow_run' only in the label switch but continues to include
recommendation-specific columns (purchase, recommendedProducts, confidence) and
leaves resolvedColumns hardcoded to t('types.recommendProduct'); update
useApprovalColumns to branch on resource type (check the existing switch/case
for 'workflow_run') and return a workflow-specific column set for workflow_run
that omits recommendation columns and instead shows workflow-relevant fields,
and change resolvedColumns (the resolvedColumns variable/return path) to
conditionally use a workflow-specific resolved label rather than always using
t('types.recommendProduct').
In `@services/platform/app/features/chat/components/chat-messages.tsx`:
- Around line 162-168: The memoized comparator for WorkflowRunApprovalCard
currently ignores the metadata prop so metadata-only changes won't trigger
rerenders; update the card's custom comparator (the equality function used when
memoizing WorkflowRunApprovalCard) to include a comparison of prevProps.metadata
and nextProps.metadata (use a deep equality check such as lodash/isEqual or a
stable JSON comparison rather than reference equality) alongside the existing
checks for status, executedAt, and executionError so metadata updates cause the
component to rerender.
In
`@services/platform/app/features/chat/components/workflow-run-approval-card.tsx`:
- Around line 404-415: The custom comparator used when memoizing
WorkflowRunApprovalCard (the second arg to memo in WorkflowRunApprovalCard)
omits comparing props.metadata, which can change (e.g., metadata.executionId)
and must trigger a rerender to start useExecutionStatus; update the comparator
to include a shallow compare of prevProps.metadata === nextProps.metadata or
compare the specific field used (prevProps.metadata?.executionId ===
nextProps.metadata?.executionId), or remove the custom comparator so React's
default shallow props compare is used instead.
- Around line 113-127: The current handleApprove flow calls
updateApprovalStatus(...) to set status:'approved' and then separately calls
executeApprovedRun(...), which can leave records approved but never executed if
the second RPC fails; change this to a single atomic backend call (e.g., create
API method updateApprovalAndExecute or approveAndStartWorkflow) that accepts
approvalId and performs both the approval update and workflow start in one
server transaction, then replace the two-client calls in handleApprove with a
single call to that new endpoint; alternatively, if changing the backend is not
possible, implement an explicit retry/reconciliation path on the server (or mark
the record with a pending_execution flag and store start errors) and update
handleApprove to call a new execute-retry endpoint (or set pending flag) so
approvals are never left in an unrecoverable approved-without-execution state.
In `@services/platform/app/features/chat/hooks/queries.ts`:
- Around line 264-295: The useWorkflowRunApprovals hook duplicates the approvals
filter/map pipeline used elsewhere; extract a reusable selector helper (e.g.,
selectApprovalsByResource) that accepts approvals, threadId, resourceType and a
metadata-typing mapper and returns the mapped approvals list; update
useWorkflowRunApprovals to call selectApprovalsByResource(approvals, threadId,
'workflow_run', mapper) (keep mapping to WorkflowRunApproval/WorkflowRunMetadata
and using toId<'approvals'>) and replace the near-identical logic in the other
hooks (those using useApprovals and mapping to
integration/workflowCreation/humanInput shapes) to call the same helper so
future approval-shape changes are centralized.
In `@services/platform/convex/agent_tools/workflows/helpers/syntax_reference.ts`:
- Around line 489-490: Clarify the distinction between omitting the entire
"output step" vs omitting the "outputMapping" field: update the sentence that
currently reads "If omitted, the workflow output falls back to sanitized
variables..." to explicitly say "If the output step is omitted entirely, the
workflow output falls back to sanitized variables (secrets and system fields
stripped)". Then update the sentence that reads "If outputMapping is empty or
omitted, the workflow output is null" to explicitly say "If an output step
exists but its outputMapping field is empty or omitted, the workflow output is
null". Ensure both updated phrases reference the exact tokens "output step" and
"outputMapping" so readers can find them.
In `@services/platform/convex/agent_tools/workflows/internal_actions.ts`:
- Around line 53-58: The current non-atomic check of approval.executedAt in
executeApprovedWorkflowCreation and executeApprovedWorkflowRun allows races that
create duplicate workflows; change the flow to first perform an atomic
compare-and-set mutation on the approval record (e.g., set approval.claimedAt or
set approval.executedAt only if it is null) inside a Convex mutation so the
“already executed?” guard is enforced server-side before any provisioning or
starting occurs, then perform external non-idempotent side effects (provision
workflow / start run), and finally update the stored approval with the final
executedAt/result (or clear the claim on failure) so the initial claim is
durable and prevents concurrent execution.
In `@services/platform/convex/agent_tools/workflows/internal_mutations.ts`:
- Around line 25-27: The update currently throws an Error when
ctx.db.get(args.approvalId) returns null, but these result-update mutations must
tolerate the approval being deleted concurrently; change the logic in the
approval result-update functions (the block that calls ctx.db.get with
args.approvalId and checks approval.executedAt) to return early when approval is
null instead of throwing or logging, and apply the same early-return pattern to
the other occurrence around the approval check at the later block referenced
(the one at lines approximately 179-181) so missing approvals become no-ops.
- Around line 135-170: createWorkflowRunApproval is missing an explicit returns
validator; add returns: v.id('approvals') to the internalMutation call (at the
same top level as args and handler) so the mutation's runtime contract documents
and validates that it returns an approval id; ensure the symbol name
createWorkflowRunApproval is updated accordingly and keep the handler return
type as Promise<Id<'approvals'>>.
In `@services/platform/convex/agent_tools/workflows/run_workflow_tool.ts`:
- Around line 72-79: The tool currently only guards for organizationId but not
ctx.messageId, which allows creating workflow-run approvals the UI will drop;
update the validation in run_workflow_tool to require messageId (alongside
organizationId) and return success: false with an explanatory message if
messageId is missing; also add the same guard where approvals are created later
in the file (the block referenced around lines 123-125) so any code paths that
create an approval check ctx.messageId before constructing or persisting the
approval card.
- Around line 83-86: The workflowId is currently validated as z.string().min(1)
at the tool boundary and then cast with toId<'wfDefinitions'>() before calling
internal.wf_definitions.internal_queries.resolveWorkflow, which defers ID format
checking to the query layer; update the tool's Zod validator for workflowId to
use v.id('wfDefinitions') instead of z.string().min(1), remove the runtime cast
toId<'wfDefinitions'>(args.workflowId) when calling ctx.runQuery (pass the
validated id directly), and ensure any error handling around ctx.runQuery
remains intact so malformed IDs are rejected with a clear tool-level validation
error rather than a downstream exception.
In `@services/platform/convex/agent_tools/workflows/workflow_examples_tool.ts`:
- Line 31: Add the missing 'output' category to the human-readable "SYNTAX
CATEGORIES" documentation block and to the fallback error message that
enumerates valid categories so the documentation and runtime error text match
the enum/arg description (ensure the string 'output' is inserted into the list
in both the SYNTAX CATEGORIES block and the fallback error message used by the
workflow_examples_tool module).
In `@services/platform/convex/approvals/internal_queries.ts`:
- Around line 21-35: verifyOrganizationMembership currently omits an explicit
returns validator; add a returns schema to match the pattern used by
getApprovalById so the internalQuery declaration is consistent. Update the
internalQuery call for verifyOrganizationMembership to include a returns:
v.void() (or the same validator used by getApprovalById) so the handler's
throw-or-succeed semantics are explicitly represented; ensure you import/use the
same validator helper (v) as in surrounding queries.
In `@services/platform/convex/wf_executions/queries.ts`:
- Around line 66-75: The returned execution payload currently includes optional
fields (currentStepSlug, completedAt, output, etc.) which may be materialized as
undefined; change the return to construct the object and strip undefined
properties (e.g., use Object.fromEntries with Object.entries(...).filter(([,v])
=> v !== undefined)) so only present fields from the document returned by
ctx.db.get(args.executionId) are included; locate the block that reads exec from
ctx.db.get(args.executionId) and replace the inline object literal containing
status, currentStepSlug, error, startedAt, completedAt, output with a filtered
object built via Object.fromEntries to omit undefined values.
In
`@services/platform/convex/workflow_engine/helpers/engine/on_workflow_complete.test.ts`:
- Around line 49-163: Add a new unit test for handleWorkflowComplete that
asserts no system message is posted when an approval exists but lacks a
threadId: create exec with triggerData.approvalId and an approval object missing
threadId, use createMockCtx to get ctx/runMutationArgs/dbGet, mock dbGet to
return the completed exec and the approval (without threadId), invoke
handleWorkflowComplete (e.g., success result), then filter runMutationArgs for
calls with threadId/content as in the other tests and assert the filtered length
is 0; reference handleWorkflowComplete, triggerData.approvalId, and
approval?.threadId when locating where to add this test.
In
`@services/platform/convex/workflow_engine/helpers/engine/on_workflow_complete.ts`:
- Around line 101-105: The code mixes using result.kind and the kind parameter
when building the failure/success message; standardize on the kind parameter
passed into onWorkflowComplete (use kind everywhere) by replacing the
result.kind reference used to set errorMsg with the kind parameter for
determining success/failure and ensure errorMsg still derives from result.error
(e.g., set errorMsg based on result.error) so only kind is used to select the
success vs failure branch in messageContent (references: result.kind, kind
parameter, errorMsg, messageContent, workflowName, exec._id).
In
`@services/platform/convex/workflow_engine/helpers/engine/serialize_and_complete_execution_handler.test.ts`:
- Around line 11-27: Remove the duplicated SENSITIVE_OUTPUT_KEYS array and
sanitizeOutputVariables function from the test and instead import the exported
symbols from the handler module; update the test to import SENSITIVE_OUTPUT_KEYS
and sanitizeOutputVariables (the same named exports used in the handler) and use
those imports in assertions so tests exercise the real implementation rather
than a stale copy.
In
`@services/platform/convex/workflow_engine/helpers/engine/serialize_and_complete_execution_handler.ts`:
- Around line 99-115: Export the SENSITIVE_OUTPUT_KEYS array and
sanitizeOutputVariables function so tests can import them instead of
duplicating; specifically, add exports for the symbols SENSITIVE_OUTPUT_KEYS and
sanitizeOutputVariables in serialize_and_complete_execution_handler.ts and
update the tests to import these exports, ensuring external behavior remains
unchanged and no other logic is modified.
In
`@services/platform/convex/workflow_engine/helpers/step_execution/execute_step_by_type.ts`:
- Around line 114-128: The catch block that handles replaceVariables(mapping,
variables) currently swallows errors and returns a success port with
mappedOutput null; update execute_step_by_type (the try/catch around
replaceVariables and the returned object using mappedOutput) to fail the step
instead: either rethrow the caught error or return a failure result (set port to
'failure' and include an error payload/metadata) so an invalid output mapping
does not produce a misleading success. Ensure you use the same symbols
mappedOutput and replaceVariables so callers and downstream code still handle
the failure path consistently.
In `@services/platform/convex/workflow_engine/helpers/validation/steps/output.ts`:
- Around line 31-34: The code currently treats matches of SECRETS_PATTERN in
outputMapping as a warning (warnings.push); instead, block them by recording a
validation error so workflows cannot surface secrets. Replace the warnings.push
call for SECRETS_PATTERN with an errors.push (or otherwise mark the validation
as failed) and use a clear message referencing the outputMapping key (e.g.,
`Output step outputMapping key "${key}" references secrets — exposing secrets in
workflow output is forbidden`), keeping SECRETS_PATTERN, outputMapping,
warnings/errors variables consistent with the surrounding validation logic.
In `@services/platform/convex/workflow_engine/types/workflow.ts`:
- Around line 55-62: The runtime validator workflowStepValidator currently
accepts the 'agent' step type while the WorkflowStep.stepType union (and
repo-wide StepType) no longer includes 'agent'; update workflowStepValidator to
remove 'agent' from its allowed values (and any related validators/branches) so
the runtime validation matches the WorkflowStep.stepType contract, or
alternatively reintroduce 'agent' into WorkflowStep.stepType and any other
StepType definitions across the codebase for consistency; check related
validator usage mentioned around the other occurrence (lines referenced in the
review) to apply the same change everywhere.
In `@services/platform/messages/en.json`:
- Around line 2061-2081: The workflowRunApproval messages object contains a
duplicated key "statusRejected" (appearing for "Workflow execution was
cancelled." and again later), which causes the first value to be unreachable;
remove or rename one of the entries so all keys are unique (e.g., keep
"statusRejected": "Rejected" and rename the other to "statusCancelled" or
combine the messages into a single "statusRejected" value), updating any
references that expect the removed/renamed key.
---
Outside diff comments:
In
`@services/platform/convex/agent_tools/workflows/save_workflow_definition_tool.ts`:
- Around line 128-135: The prompt still emphasizes using 'noop' to terminate
workflows which conflicts with the new step type 'output' and causes agents to
omit explicit output mapping; update the guidance in the
save_workflow_definition_tool prompt to prefer using an explicit output step
mapping (stepType: "output") whenever the workflow must return results, mention
that 'noop' is only for graceful non-returning ends, and show a short example of
an output step and how it maps to run_workflow return paths (reference the
stepType list including "output" and the run_workflow return expectations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d968b783-21f2-4454-a79a-04addf48c8e5
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (59)
services/platform/app/features/approvals/components/approvals/use-approval-columns.tsxservices/platform/app/features/approvals/hooks/actions.tsservices/platform/app/features/approvals/hooks/queries.tsservices/platform/app/features/automations/components/automation-step.tsxservices/platform/app/features/automations/components/next-steps-editor.tsxservices/platform/app/features/automations/components/step-create-dialog.tsxservices/platform/app/features/automations/utils/step-icons.tsxservices/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/features/chat/components/chat-messages.tsxservices/platform/app/features/chat/components/workflow-run-approval-card.tsxservices/platform/app/features/chat/hooks/queries.tsservices/platform/app/features/chat/hooks/use-execution-status.tsservices/platform/app/features/chat/hooks/use-merged-chat-items.tsservices/platform/app/features/custom-agents/components/test-chat-panel/test-message-list.tsxservices/platform/app/features/custom-agents/components/tool-selector.tsxservices/platform/app/features/custom-agents/hooks/use-test-chat.tsservices/platform/convex/agent_tools/tool_names.tsservices/platform/convex/agent_tools/tool_registry.tsservices/platform/convex/agent_tools/workflows/__tests__/create_workflow_tool.test.tsservices/platform/convex/agent_tools/workflows/__tests__/execute_approved_workflow_run.test.tsservices/platform/convex/agent_tools/workflows/__tests__/run_workflow_tool.test.tsservices/platform/convex/agent_tools/workflows/create_workflow_tool.tsservices/platform/convex/agent_tools/workflows/helpers/syntax_reference.tsservices/platform/convex/agent_tools/workflows/internal_actions.tsservices/platform/convex/agent_tools/workflows/internal_mutations.tsservices/platform/convex/agent_tools/workflows/run_workflow_tool.tsservices/platform/convex/agent_tools/workflows/save_workflow_definition_tool.tsservices/platform/convex/agent_tools/workflows/workflow_examples_tool.tsservices/platform/convex/approvals/actions.tsservices/platform/convex/approvals/helpers.tsservices/platform/convex/approvals/internal_queries.tsservices/platform/convex/approvals/mutations.tsservices/platform/convex/approvals/schema.tsservices/platform/convex/approvals/types.tsservices/platform/convex/approvals/validators.tsservices/platform/convex/lib/rate_limiter/index.tsservices/platform/convex/wf_executions/queries.tsservices/platform/convex/workflow_engine/helpers/engine/on_workflow_complete.test.tsservices/platform/convex/workflow_engine/helpers/engine/on_workflow_complete.tsservices/platform/convex/workflow_engine/helpers/engine/serialize_and_complete_execution_handler.test.tsservices/platform/convex/workflow_engine/helpers/engine/serialize_and_complete_execution_handler.tsservices/platform/convex/workflow_engine/helpers/step_execution/execute_step_by_type.test.tsservices/platform/convex/workflow_engine/helpers/step_execution/execute_step_by_type.tsservices/platform/convex/workflow_engine/helpers/step_execution/types.tsservices/platform/convex/workflow_engine/helpers/validation/constants.tsservices/platform/convex/workflow_engine/helpers/validation/steps/index.tsservices/platform/convex/workflow_engine/helpers/validation/steps/output.test.tsservices/platform/convex/workflow_engine/helpers/validation/steps/output.tsservices/platform/convex/workflow_engine/helpers/validation/validate_predefined_workflows.test.tsservices/platform/convex/workflow_engine/helpers/validation/validate_workflow_definition.tsservices/platform/convex/workflow_engine/helpers/validation/variables/step_schemas.tsservices/platform/convex/workflow_engine/internal_actions.tsservices/platform/convex/workflow_engine/types/nodes.tsservices/platform/convex/workflow_engine/types/workflow.tsservices/platform/convex/workflows/schema.tsservices/platform/convex/workflows/steps/types.tsservices/platform/convex/workflows/steps/validators.tsservices/platform/convex/workflows/validators.tsservices/platform/messages/en.json
Comment on lines
+130
to
+131
| case 'workflow_run': | ||
| return t('types.runWorkflow'); |
Contributor
There was a problem hiding this comment.
workflow_run is only wired into the first label, not the rest of the approvals table.
This hook still renders recommendation-specific columns (purchase, recommendedProducts, confidence), and resolvedColumns still hardcodes t('types.recommendProduct') at Lines 255-257. Newly approved/rejected workflow-run approvals will therefore show misleading or empty data instead of a workflow-specific view. Please branch the column set for workflow_run before surfacing this resource type here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx`
around lines 130 - 131, The hook useApprovalColumns currently treats
'workflow_run' only in the label switch but continues to include
recommendation-specific columns (purchase, recommendedProducts, confidence) and
leaves resolvedColumns hardcoded to t('types.recommendProduct'); update
useApprovalColumns to branch on resource type (check the existing switch/case
for 'workflow_run') and return a workflow-specific column set for workflow_run
that omits recommendation columns and instead shows workflow-relevant fields,
and change resolvedColumns (the resolvedColumns variable/return path) to
conditionally use a workflow-specific resolved label rather than always using
t('types.recommendProduct').
Comment on lines
+162
to
+168
| <WorkflowRunApprovalCard | ||
| approvalId={approval._id} | ||
| organizationId={organizationId} | ||
| status={approval.status} | ||
| metadata={approval.metadata} | ||
| executedAt={approval.executedAt} | ||
| executionError={approval.executionError} |
Contributor
There was a problem hiding this comment.
Include metadata in the memoized card’s equality check.
WorkflowRunApprovalCard’s custom comparator in services/platform/app/features/chat/components/workflow-run-approval-card.tsx:403-415 does not compare metadata. With this new prop wired through here, metadata-only updates will not rerender the card, so output preview/details can stay stale until status, executedAt, or executionError changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/chat-messages.tsx` around
lines 162 - 168, The memoized comparator for WorkflowRunApprovalCard currently
ignores the metadata prop so metadata-only changes won't trigger rerenders;
update the card's custom comparator (the equality function used when memoizing
WorkflowRunApprovalCard) to include a comparison of prevProps.metadata and
nextProps.metadata (use a deep equality check such as lodash/isEqual or a stable
JSON comparison rather than reference equality) alongside the existing checks
for status, executedAt, and executionError so metadata updates cause the
component to rerender.
Comment on lines
+113
to
+127
| const handleApprove = async () => { | ||
| if (!user?.userId) { | ||
| setError('User not authenticated'); | ||
| return; | ||
| } | ||
| setIsApproving(true); | ||
| setError(null); | ||
| try { | ||
| await updateApprovalStatus({ | ||
| approvalId, | ||
| status: 'approved', | ||
| }); | ||
| await executeApprovedRun({ | ||
| approvalId, | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't make approval depend on two separate client RPCs.
Line 121 marks the record approved before Line 125 dispatches the run. If the tab closes, navigation interrupts, or the second call fails before reaching the server, this approval becomes non-retryable approved state with no execution and no startup error recorded. This needs a single backend endpoint that both approves and starts the workflow, or an explicit retry path for approved-without-execution records.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/chat/components/workflow-run-approval-card.tsx`
around lines 113 - 127, The current handleApprove flow calls
updateApprovalStatus(...) to set status:'approved' and then separately calls
executeApprovedRun(...), which can leave records approved but never executed if
the second RPC fails; change this to a single atomic backend call (e.g., create
API method updateApprovalAndExecute or approveAndStartWorkflow) that accepts
approvalId and performs both the approval update and workflow start in one
server transaction, then replace the two-client calls in handleApprove with a
single call to that new endpoint; alternatively, if changing the backend is not
possible, implement an explicit retry/reconciliation path on the server (or mark
the record with a pending_execution flag and store start errors) and update
handleApprove to call a new execute-retry endpoint (or set pending flag) so
approvals are never left in an unrecoverable approved-without-execution state.
Comment on lines
+404
to
+415
| export const WorkflowRunApprovalCard = memo( | ||
| WorkflowRunApprovalCardComponent, | ||
| (prevProps, nextProps) => { | ||
| return ( | ||
| prevProps.approvalId === nextProps.approvalId && | ||
| prevProps.organizationId === nextProps.organizationId && | ||
| prevProps.status === nextProps.status && | ||
| prevProps.className === nextProps.className && | ||
| prevProps.executedAt === nextProps.executedAt && | ||
| prevProps.executionError === nextProps.executionError | ||
| ); | ||
| }, |
Contributor
There was a problem hiding this comment.
Include metadata in the memo key or drop the custom comparator.
Line 84 derives executionId from metadata.executionId, but this comparator never compares metadata. After approval, the backend can patch in the execution ID while status, executedAt, and executionError stay unchanged, so this comparator can suppress the rerender that should start useExecutionStatus and show live progress.
♻️ Safer fix
-export const WorkflowRunApprovalCard = memo(
- WorkflowRunApprovalCardComponent,
- (prevProps, nextProps) => {
- return (
- prevProps.approvalId === nextProps.approvalId &&
- prevProps.organizationId === nextProps.organizationId &&
- prevProps.status === nextProps.status &&
- prevProps.className === nextProps.className &&
- prevProps.executedAt === nextProps.executedAt &&
- prevProps.executionError === nextProps.executionError
- );
- },
-);
+export const WorkflowRunApprovalCard = memo(WorkflowRunApprovalCardComponent);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const WorkflowRunApprovalCard = memo( | |
| WorkflowRunApprovalCardComponent, | |
| (prevProps, nextProps) => { | |
| return ( | |
| prevProps.approvalId === nextProps.approvalId && | |
| prevProps.organizationId === nextProps.organizationId && | |
| prevProps.status === nextProps.status && | |
| prevProps.className === nextProps.className && | |
| prevProps.executedAt === nextProps.executedAt && | |
| prevProps.executionError === nextProps.executionError | |
| ); | |
| }, | |
| export const WorkflowRunApprovalCard = memo(WorkflowRunApprovalCardComponent); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/chat/components/workflow-run-approval-card.tsx`
around lines 404 - 415, The custom comparator used when memoizing
WorkflowRunApprovalCard (the second arg to memo in WorkflowRunApprovalCard)
omits comparing props.metadata, which can change (e.g., metadata.executionId)
and must trigger a rerender to start useExecutionStatus; update the comparator
to include a shallow compare of prevProps.metadata === nextProps.metadata or
compare the specific field used (prevProps.metadata?.executionId ===
nextProps.metadata?.executionId), or remove the custom comparator so React's
default shallow props compare is used instead.
Comment on lines
+264
to
+295
| export function useWorkflowRunApprovals( | ||
| organizationId: string, | ||
| threadId: string | undefined, | ||
| ) { | ||
| const { approvals, isLoading } = useApprovals(organizationId); | ||
|
|
||
| const workflowRunApprovals = useMemo((): WorkflowRunApproval[] => { | ||
| if (!approvals || !threadId) return []; | ||
| return approvals | ||
| .filter( | ||
| (a) => | ||
| a.threadId === threadId && | ||
| a.resourceType === 'workflow_run' && | ||
| a.metadata !== undefined, | ||
| ) | ||
| .map((a) => ({ | ||
| _id: toId<'approvals'>(a._id), | ||
| status: a.status, | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- Metadata shape is guaranteed by resourceType filter above | ||
| metadata: a.metadata as unknown as WorkflowRunMetadata, | ||
| executedAt: a.executedAt, | ||
| executionError: a.executionError, | ||
| _creationTime: a._creationTime, | ||
| messageId: a.messageId, | ||
| })); | ||
| }, [approvals, threadId]); | ||
|
|
||
| return { | ||
| approvals: workflowRunApprovals, | ||
| isLoading, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the shared approval-selector helper before adding a fourth copy.
This hook repeats the same useApprovals(...) -> filter by resourceType -> map shared fields pipeline already used for integrations, workflow creation, and human input. Pulling that into a generic helper will keep future approval-shape changes from drifting across four near-identical hooks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/hooks/queries.ts` around lines 264 - 295,
The useWorkflowRunApprovals hook duplicates the approvals filter/map pipeline
used elsewhere; extract a reusable selector helper (e.g.,
selectApprovalsByResource) that accepts approvals, threadId, resourceType and a
metadata-typing mapper and returns the mapped approvals list; update
useWorkflowRunApprovals to call selectApprovalsByResource(approvals, threadId,
'workflow_run', mapper) (keep mapping to WorkflowRunApproval/WorkflowRunMetadata
and using toId<'approvals'>) and replace the near-identical logic in the other
hooks (those using useApprovals and mapping to
integration/workflowCreation/humanInput shapes) to call the same helper so
future approval-shape changes are centralized.
Comment on lines
+99
to
+115
| const SENSITIVE_OUTPUT_KEYS = [ | ||
| 'secrets', | ||
| 'organizationId', | ||
| 'wfDefinitionId', | ||
| 'rootWfDefinitionId', | ||
| ]; | ||
|
|
||
| function sanitizeOutputVariables(vars: unknown): unknown { | ||
| if (typeof vars !== 'object' || vars === null || Array.isArray(vars)) { | ||
| return vars; | ||
| } | ||
| const sanitized = { ...vars } as Record<string, unknown>; | ||
| for (const key of SENSITIVE_OUTPUT_KEYS) { | ||
| delete sanitized[key]; | ||
| } | ||
| return sanitized; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exporting these for direct testing.
The test file duplicates SENSITIVE_OUTPUT_KEYS and sanitizeOutputVariables, creating maintenance risk if implementations diverge. Exporting these would allow direct imports in tests.
♻️ Suggested refactor for testability
-const SENSITIVE_OUTPUT_KEYS = [
+export const SENSITIVE_OUTPUT_KEYS = [
'secrets',
'organizationId',
'wfDefinitionId',
'rootWfDefinitionId',
];
-function sanitizeOutputVariables(vars: unknown): unknown {
+export function sanitizeOutputVariables(vars: unknown): unknown {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const SENSITIVE_OUTPUT_KEYS = [ | |
| 'secrets', | |
| 'organizationId', | |
| 'wfDefinitionId', | |
| 'rootWfDefinitionId', | |
| ]; | |
| function sanitizeOutputVariables(vars: unknown): unknown { | |
| if (typeof vars !== 'object' || vars === null || Array.isArray(vars)) { | |
| return vars; | |
| } | |
| const sanitized = { ...vars } as Record<string, unknown>; | |
| for (const key of SENSITIVE_OUTPUT_KEYS) { | |
| delete sanitized[key]; | |
| } | |
| return sanitized; | |
| } | |
| export const SENSITIVE_OUTPUT_KEYS = [ | |
| 'secrets', | |
| 'organizationId', | |
| 'wfDefinitionId', | |
| 'rootWfDefinitionId', | |
| ]; | |
| export function sanitizeOutputVariables(vars: unknown): unknown { | |
| if (typeof vars !== 'object' || vars === null || Array.isArray(vars)) { | |
| return vars; | |
| } | |
| const sanitized = { ...vars } as Record<string, unknown>; | |
| for (const key of SENSITIVE_OUTPUT_KEYS) { | |
| delete sanitized[key]; | |
| } | |
| return sanitized; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/convex/workflow_engine/helpers/engine/serialize_and_complete_execution_handler.ts`
around lines 99 - 115, Export the SENSITIVE_OUTPUT_KEYS array and
sanitizeOutputVariables function so tests can import them instead of
duplicating; specifically, add exports for the symbols SENSITIVE_OUTPUT_KEYS and
sanitizeOutputVariables in serialize_and_complete_execution_handler.ts and
update the tests to import these exports, ensuring external behavior remains
unchanged and no other logic is modified.
Comment on lines
+114
to
+128
| try { | ||
| mappedOutput = replaceVariables(mapping, variables); | ||
| } catch (error) { | ||
| console.error( | ||
| `[output step] Failed to apply output mapping template:`, | ||
| error, | ||
| ); | ||
| mappedOutput = null; | ||
| } | ||
| } | ||
| return { | ||
| port: 'success', | ||
| output: { type: 'output', data: mappedOutput }, | ||
| variables: { __workflowOutput: mappedOutput }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Fail the run when output mapping evaluation throws.
The catch path downgrades a broken output template into success with data: null. That hides misconfigured outputs as completed workflow runs and makes approval/system-message status incorrect. Please rethrow or return a failure result here so invalid output mappings fail the execution.
🧭 Suggested direction
if (isRecord(mapping) && Object.keys(mapping).length > 0) {
try {
mappedOutput = replaceVariables(mapping, variables);
} catch (error) {
- console.error(
- `[output step] Failed to apply output mapping template:`,
- error,
- );
- mappedOutput = null;
+ throw new Error(
+ `[output step] Failed to apply output mapping template for "${stepDef.stepSlug}": ${
+ error instanceof Error ? error.message : 'Unknown error'
+ }`,
+ );
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/convex/workflow_engine/helpers/step_execution/execute_step_by_type.ts`
around lines 114 - 128, The catch block that handles replaceVariables(mapping,
variables) currently swallows errors and returns a success port with
mappedOutput null; update execute_step_by_type (the try/catch around
replaceVariables and the returned object using mappedOutput) to fail the step
instead: either rethrow the caught error or return a failure result (set port to
'failure' and include an error payload/metadata) so an invalid output mapping
does not produce a misleading success. Ensure you use the same symbols
mappedOutput and replaceVariables so callers and downstream code still handle
the failure path consistently.
Comment on lines
+31
to
+34
| } else if (SECRETS_PATTERN.test(value)) { | ||
| warnings.push( | ||
| `Output step outputMapping key "${key}" references secrets — avoid exposing secrets in workflow output`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Block secret references instead of only warning.
This still validates {{secrets.*}} as a runnable output mapping. Because output mappings are later materialized and surfaced back into chat/approval previews, this allows workflows to exfiltrate secrets through their public result, which breaks the “no internal state leakage” contract.
🔒 Proposed fix
- } else if (SECRETS_PATTERN.test(value)) {
- warnings.push(
- `Output step outputMapping key "${key}" references secrets — avoid exposing secrets in workflow output`,
- );
+ } else if (SECRETS_PATTERN.test(value)) {
+ errors.push(
+ `Output step outputMapping key "${key}" must not reference secrets`,
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (SECRETS_PATTERN.test(value)) { | |
| warnings.push( | |
| `Output step outputMapping key "${key}" references secrets — avoid exposing secrets in workflow output`, | |
| ); | |
| } else if (SECRETS_PATTERN.test(value)) { | |
| errors.push( | |
| `Output step outputMapping key "${key}" must not reference secrets`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/workflow_engine/helpers/validation/steps/output.ts`
around lines 31 - 34, The code currently treats matches of SECRETS_PATTERN in
outputMapping as a warning (warnings.push); instead, block them by recording a
validation error so workflows cannot surface secrets. Replace the warnings.push
call for SECRETS_PATTERN with an errors.push (or otherwise mark the validation
as failed) and use a clear message referencing the outputMapping key (e.g.,
`Output step outputMapping key "${key}" references secrets — exposing secrets in
workflow output is forbidden`), keeping SECRETS_PATTERN, outputMapping,
warnings/errors variables consistent with the surrounding validation logic.
Comment on lines
+55
to
+62
| stepType: | ||
| | 'start' | ||
| | 'trigger' | ||
| | 'llm' | ||
| | 'condition' | ||
| | 'action' | ||
| | 'loop' | ||
| | 'output'; |
Contributor
There was a problem hiding this comment.
Remove 'agent' from workflowStepValidator or reintroduce it consistently everywhere.
WorkflowStep.stepType now matches the repo-wide union for workflow steps, but workflowStepValidator still accepts 'agent'. That leaves runtime validation broader than the TypeScript contract and the other StepType definitions, so unsupported data can pass validation here and fail later in execution/dispatch.
Suggested fix
stepType: v.union(
v.literal('start'),
v.literal('trigger'),
- v.literal('agent'),
v.literal('llm'),
v.literal('condition'),
v.literal('action'),
v.literal('loop'),
v.literal('output'),
),Also applies to: 147-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/workflow_engine/types/workflow.ts` around lines 55 -
62, The runtime validator workflowStepValidator currently accepts the 'agent'
step type while the WorkflowStep.stepType union (and repo-wide StepType) no
longer includes 'agent'; update workflowStepValidator to remove 'agent' from its
allowed values (and any related validators/branches) so the runtime validation
matches the WorkflowStep.stepType contract, or alternatively reintroduce 'agent'
into WorkflowStep.stepType and any other StepType definitions across the
codebase for consistency; check related validator usage mentioned around the
other occurrence (lines referenced in the review) to apply the same change
everywhere.
Comment on lines
+2061
to
+2081
| "workflowRunApproval": { | ||
| "approve": "Run workflow", | ||
| "reject": "Cancel", | ||
| "approveTooltip": "Approve and run this workflow", | ||
| "rejectTooltip": "Cancel workflow execution", | ||
| "statusApprovedSuccess": "Workflow execution started successfully.", | ||
| "statusApprovedFailed": "Workflow execution was approved but failed to start.", | ||
| "statusRejected": "Workflow execution was cancelled.", | ||
| "showParameters": "Show parameters", | ||
| "hideParameters": "Hide parameters", | ||
| "executionRunning": "Running...", | ||
| "executionRunningStep": "Running step: {step}", | ||
| "executionElapsed": "Running for {duration}", | ||
| "executionCompleted": "Completed successfully", | ||
| "executionFailed": "Execution failed", | ||
| "viewDetails": "View execution details", | ||
| "showOutput": "Show output", | ||
| "hideOutput": "Hide output", | ||
| "statusPending": "Pending", | ||
| "statusApproved": "Approved", | ||
| "statusRejected": "Rejected" |
Contributor
There was a problem hiding this comment.
Fix the duplicate statusRejected key.
Line 2068 and Line 2081 both declare statusRejected. In JSON the later value wins, so "Workflow execution was cancelled." is unreachable, and Biome already flags this block as an error.
🧰 Tools
🪛 Biome (2.4.4)
[error] 2068-2068: The key statusRejected was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/en.json` around lines 2061 - 2081, The
workflowRunApproval messages object contains a duplicated key "statusRejected"
(appearing for "Workflow execution was cancelled." and again later), which
causes the first value to be unreachable; remove or rename one of the entries so
all keys are unique (e.g., keep "statusRejected": "Rejected" and rename the
other to "statusCancelled" or combine the messages into a single
"statusRejected" value), updating any references that expect the removed/renamed
key.
This was referenced Mar 9, 2026
6 tasks
7 tasks
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
run_workflowagent tool that lets the AI agent trigger workflow execution from chat, with mandatory user approval before execution beginsoutputstep type for workflows to explicitly define output mapping, treating workflows like functions (start=input, output=output) and preventing internal state leakageDetails
Backend:
workflow_runresource type in approval schema/validatorsrun_workflow_toolwith org ownership, archived status validation, rate limiting, and idempotency guardsoutputstep type with outputMapping in workflow engine (types, validators, execution, validation)Frontend:
WorkflowRunApprovalCardcomponent with approve/reject flow, live execution status, elapsed timer, output preview, and execution details linkuseWorkflowRunApprovalsquery hook integrated into chat interface and test chatTests:
Test plan
run_workflowtool from chatbun run --filter @tale/platform testRefs #663
🤖 Generated with Claude Code
Summary by CodeRabbit