feat(approvals): typed output pickers, quick-path guard, expression completion (framework#3447, #2829) (#2831)

Typed decision_output_defs render record pickers (user people-picker /
department/position/team lookups; multiple → id array). Quick decision
paths (inline a/r, hover, mobile, bulk) are guarded off requests that
declare decision outputs — only the drawer dialog collects them; buttons
disable with an explanation. The expression approver input gains a
three-group scope picker (current/trigger/vars) with inline root
validation; nodeOutputRefs models approval outputs (decision + declared
keys) so the previous stage's outputs are pickable, and vars.previous is
always listed so legitimate vars.* references never flag as unknown.
Browser-verified end to end (picker multi-select → co-sign slate).

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: os-zhuang

.changeset/approval-typed-picker-quickpath-completion.md

@@ -0,0 +1,10 @@
+---
+"@object-ui/app-shell": minor
+"@object-ui/console": minor
+---
+
+feat(approvals): typed decision-output pickers, quick-path guard, and approval-expression completion (framework#3447 follow-ups, #2829)
+
+- Decision dialogs render TYPED decision outputs as record pickers: `decision_output_defs` (`{ key, label?, type, multiple? }`) maps `user` to the sys_user people picker and `department`/`position`/`team` to the matching system-object lookup; `multiple` collects an id array. Bare keys keep the text input.
+- Quick decision paths (inline a/r keyboard, hover buttons, mobile card buttons, bulk apply) no longer decide a request whose node declares decision outputs (#2829) — only the drawer dialog collects those fields. Buttons render disabled with an explanation; bulk selection excludes such rows via the existing "N actionable" messaging.
+- The approval `expression` approver input now has a scope-aware data picker and inline root validation: three groups — `current.<field>` (live at node entry), `trigger.<field>` (submit snapshot), `vars.*` (flow variables) — built by `useFlowScope` from the same materials as the condition picker but with the approval root set. `nodeOutputRefs` now models approval nodes (`<nodeId>.decision` + declared `decisionOutputs` keys), so the previous stage's outputs are pickable, and `vars.previous` is always listed so a legitimate `vars.*` reference is never flagged as out of scope.

apps/console/src/pages/system/ApprovalsInboxPage.tsx

@@ -803,13 +803,29 @@ export function ApprovalsInboxPage() {
     return (r.pending_approvers || []).some(a => idSet.has(a));
   }, [identities]);
 
+  /**
+   * #2829 / framework#3447: a node that declares decision outputs expects the
+   * approver to fill them — only the drawer's declared-action dialog collects
+   * them, so the quick paths (inline a/r, hover buttons, bulk) must not decide
+   * such a request: a quick approve would silently hand the flow nothing and
+   * the next stage's expression approver would resolve an empty slate.
+   */
+  const needsDecisionInputs = useCallback((r: ApprovalRequestRow): boolean =>
+    Array.isArray(r.decision_outputs) && r.decision_outputs.length > 0, []);
+
+  /** Quick-decidable = actionable AND no declared decision outputs (#2829). */
+  const quickDecidable = useCallback((r: ApprovalRequestRow): boolean =>
+    isActionable(r) && !needsDecisionInputs(r), [isActionable, needsDecisionInputs]);
+
   /**
    * Rows the user is actually allowed to bulk-act on:
-   * status=pending AND one of the user's identities is in pending_approvers.
+   * status=pending AND one of the user's identities is in pending_approvers
+   * AND the node declares no decision outputs (#2829 — those need the drawer
+   * dialog; they surface in the bulk bar as skipped).
    */
   const actionableSelectedRows = useMemo(
-    () => filteredRows.filter(r => selectedRowIds.has(r.id) && isActionable(r)),
-    [filteredRows, selectedRowIds, isActionable],
+    () => filteredRows.filter(r => selectedRowIds.has(r.id) && quickDecidable(r)),
+    [filteredRows, selectedRowIds, quickDecidable],
   );
 
   const allFilteredSelectable = filteredRows.filter(r => r.status === 'pending');
@@ -942,17 +958,17 @@ export function ApprovalsInboxPage() {
       } else if ((e.key === 'x' || e.key === ' ') && idx >= 0 && list[idx] && tab === 'pending') {
         e.preventDefault();
         toggleRow(list[idx].id);
-      } else if (e.key === 'a' && idx >= 0 && list[idx] && isActionable(list[idx])) {
+      } else if (e.key === 'a' && idx >= 0 && list[idx] && quickDecidable(list[idx])) {
         e.preventDefault();
         setApproveTarget(list[idx]);
-      } else if (e.key === 'r' && idx >= 0 && list[idx] && isActionable(list[idx])) {
+      } else if (e.key === 'r' && idx >= 0 && list[idx] && quickDecidable(list[idx])) {
         e.preventDefault();
         setRejectTarget(list[idx]);
       }
     };
     window.addEventListener('keydown', onKey);
     return () => window.removeEventListener('keydown', onKey);
-  }, [selectedId, approveTarget, rejectTarget, tab, openDrawer, toggleRow, isActionable]);
+  }, [selectedId, approveTarget, rejectTarget, tab, openDrawer, toggleRow, quickDecidable]);
 
   // Drawer keyboard: ←/→ walk the visible list without going back to it.
   useEffect(() => {
@@ -1033,28 +1049,37 @@ export function ApprovalsInboxPage() {
   function InlineActions({ r }: { r: ApprovalRequestRow }) {
     if (!isActionable(r)) return null;
     const busy = inlineActing === r.id;
+    // #2829: a request whose node declares decision outputs must go through
+    // the drawer's dialog (the only place those fields are collected) — render
+    // the quick buttons disabled with an explanation instead of hiding them.
+    const needsInputs = needsDecisionInputs(r);
+    const needsInputsHint = tr(
+      'needsDecisionInputs',
+      'This approval collects decision outputs — open it to decide.',
+    );
     return (
       <div
         className="flex items-center gap-1 opacity-0 group-hover:opacity-100 focus-within:opacity-100 transition-opacity"
         onClick={(e) => e.stopPropagation()}
+        title={needsInputs ? needsInputsHint : undefined}
       >
         <Button
           size="sm"
           variant="ghost"
           className="h-7 px-2 text-emerald-700 hover:text-emerald-800 hover:bg-emerald-50 dark:text-emerald-400"
-          disabled={busy}
+          disabled={busy || needsInputs}
           onClick={() => setApproveTarget(r)}
-          aria-label={tr('approve', 'Approve')}
+          aria-label={needsInputs ? needsInputsHint : tr('approve', 'Approve')}
         >
           <CheckCircle2 className="h-4 w-4" />
         </Button>
         <Button
           size="sm"
           variant="ghost"
           className="h-7 px-2 text-red-600 hover:text-red-700 hover:bg-red-50 dark:text-red-400"
-          disabled={busy}
+          disabled={busy || needsInputs}
           onClick={() => setRejectTarget(r)}
-          aria-label={tr('reject', 'Reject')}
+          aria-label={needsInputs ? needsInputsHint : tr('reject', 'Reject')}
         >
           <XCircle className="h-4 w-4" />
         </Button>
@@ -1419,13 +1444,21 @@ export function ApprovalsInboxPage() {
                         </span>
                       </div>
                       {isActionable(r) && (
-                        <div className="flex gap-2 pt-1" onClick={(e) => e.stopPropagation()}>
-                          <Button size="sm" className="h-7 flex-1" disabled={inlineActing === r.id} onClick={() => setApproveTarget(r)}>
+                        // #2829: outputs-declaring requests decide only via the
+                        // drawer dialog — quick buttons disable with the hint.
+                        <div
+                          className="flex gap-2 pt-1"
+                          onClick={(e) => e.stopPropagation()}
+                          title={needsDecisionInputs(r)
+                            ? tr('needsDecisionInputs', 'This approval collects decision outputs — open it to decide.')
+                            : undefined}
+                        >
+                          <Button size="sm" className="h-7 flex-1" disabled={inlineActing === r.id || needsDecisionInputs(r)} onClick={() => setApproveTarget(r)}>
                             <CheckCircle2 className="h-3.5 w-3.5 mr-1" />{tr('approve', 'Approve')}
                           </Button>
                           <Button
                             size="sm" variant="outline" className="h-7 flex-1 border-destructive text-destructive"
-                            disabled={inlineActing === r.id} onClick={() => setRejectTarget(r)}
+                            disabled={inlineActing === r.id || needsDecisionInputs(r)} onClick={() => setRejectTarget(r)}
                           >
                             <XCircle className="h-3.5 w-3.5 mr-1" />{tr('reject', 'Reject')}
                           </Button>

apps/console/src/services/approvalsApi.ts

@@ -112,6 +112,19 @@ export interface ApprovalRequestRow {
    * `<nodeId>.<key>` variables. Absent when the node declares none.
    */
   decision_outputs?: string[];
+  /**
+   * framework#3447 follow-up: the normalized TYPED declarations behind
+   * `decision_outputs` — `{ key, label?, type?, multiple? }` — so the decide
+   * dialog renders a record picker (user / department / position / team; id
+   * values, `multiple` → id array) instead of free text. Prefer this and fall
+   * back to the bare key list (older backend).
+   */
+  decision_output_defs?: Array<{
+    key: string;
+    label?: string;
+    type?: 'text' | 'user' | 'department' | 'position' | 'team';
+    multiple?: boolean;
+  }>;
 }
 
 /**

packages/app-shell/src/views/DeclaredActionsBar.tsx

@@ -131,18 +131,44 @@ const DeclaredActionButton: React.FC<{
       // handler folds them into the nested `outputs` body the decide route
       // expects. Comma-separated values are legal (the service accepts CSV for
       // multi-id outputs).
-      const declaredOutputKeys: string[] =
-        Array.isArray(recordData.decision_outputs)
-        && /\/(approve|reject)$/.test(String((action as any).target ?? ''))
-          ? (recordData.decision_outputs as unknown[]).map(String)
-          : [];
-      const outputParams = declaredOutputKeys.map((key) => ({
-        name: `outputs.${key}`,
-        label: key.split(/[_-]+/).filter(Boolean).map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '),
-        type: 'text' as const,
-        required: false,
-        helpText: 'Handed to the flow as a decision output. Comma-separate multiple values.',
-      }));
+      const isDecideAction = /\/(approve|reject)$/.test(String((action as any).target ?? ''));
+      // Prefer the typed declarations (framework#3447 follow-up); fall back to
+      // the bare key list from an older backend.
+      const declaredOutputs: Array<{ key: string; label?: string; type?: string; multiple?: boolean }> =
+        isDecideAction && Array.isArray(recordData.decision_output_defs) && recordData.decision_output_defs.length
+          ? (recordData.decision_output_defs as Array<{ key: string; label?: string; type?: string; multiple?: boolean }>)
+          : isDecideAction && Array.isArray(recordData.decision_outputs)
+            ? (recordData.decision_outputs as unknown[]).map((k) => ({ key: String(k) }))
+            : [];
+      // Widget mapping: `user` has a dedicated sys_user picker widget; the
+      // other record kinds ride a lookup param pointed at the system object.
+      const OUTPUT_LOOKUP_OBJECTS: Record<string, string> = {
+        department: 'sys_business_unit',
+        position: 'sys_position',
+        team: 'sys_team',
+      };
+      const outputParams = declaredOutputs.filter((d) => d.key).map((d) => {
+        const label = d.label
+          ?? d.key.split(/[_-]+/).filter(Boolean).map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' ');
+        const base = { name: `outputs.${d.key}`, label, required: false } as Record<string, unknown>;
+        if (d.type === 'user') {
+          return {
+            ...base, type: 'user', multiple: d.multiple === true,
+            helpText: 'Handed to the flow as a decision output.',
+          };
+        }
+        const lookupObject = d.type ? OUTPUT_LOOKUP_OBJECTS[d.type] : undefined;
+        if (lookupObject) {
+          return {
+            ...base, type: 'lookup', referenceTo: lookupObject, multiple: d.multiple === true,
+            helpText: 'Handed to the flow as a decision output.',
+          };
+        }
+        return {
+          ...base, type: 'text',
+          helpText: 'Handed to the flow as a decision output. Comma-separate multiple values.',
+        };
+      });
       const dispatch: any = {
         ...rest,
         // Localized copies ride the dispatch: the runner reads `label` for the

packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx

@@ -34,9 +34,11 @@ export interface FlowNodeConfigFieldProps {
   context?: FlowReferenceContext;
   /** In-scope variable references for the data-picker (#1934). */
   scopeGroups?: ScopeGroup[];
+  /** #3447: approval-expression picker groups (current/trigger/vars roots). */
+  approvalScopeGroups?: ScopeGroup[];
 }
 
-export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, context, scopeGroups }: FlowNodeConfigFieldProps) {
+export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, context, scopeGroups, approvalScopeGroups }: FlowNodeConfigFieldProps) {
   const refMode: 'expression' | 'template' =
     field.refMode ?? (field.kind === 'expression' ? 'expression' : 'template');
   const control = (() => {
@@ -114,6 +116,7 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale,
             itemLabel={t('engine.inspector.flowNode.list.item', locale)}
             context={context}
             scopeGroups={scopeGroups}
+            approvalScopeGroups={approvalScopeGroups}
           />
         );
       case 'number':

packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx

@@ -136,7 +136,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
     [loc],
   );
   // In-scope variable references for this node, for the data-picker (#1934).
-  const { groups: scopeGroups } = useFlowScope(draft as Record<string, unknown>, loc?.scopeAnchorId, nestedLoopRefs);
+  const { groups: scopeGroups, approvalExpressionGroups } = useFlowScope(draft as Record<string, unknown>, loc?.scopeAnchorId, nestedLoopRefs);
   const fields = React.useMemo(() => {
     const schema = node?.type ? configSchemas[node.type] : undefined;
     const serverFields = schema !== undefined ? jsonSchemaToFlowFields(schema) : null;
@@ -364,6 +364,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
             locale={locale}
             context={{ draft, node }}
             scopeGroups={scopeGroups}
+            approvalScopeGroups={approvalExpressionGroups}
           />
         );
       })}

packages/app-shell/src/views/metadata-admin/inspectors/FlowObjectListField.tsx

@@ -103,6 +103,13 @@ export interface FlowObjectListFieldProps {
   context?: FlowReferenceContext;
   /** In-scope variable references for `expression` columns (#1934). */
   scopeGroups?: ScopeGroup[];
+  /**
+   * #3447: picker groups for approval `expression` approver cells — the
+   * closed current/trigger/vars root set. Regular flow scopeGroups must NOT
+   * be offered there (record.* / bare-field spellings are rejected at
+   * runtime), which is why this rides as its own prop.
+   */
+  approvalScopeGroups?: ScopeGroup[];
 }
 
 export function FlowObjectListField({
@@ -117,6 +124,7 @@ export function FlowObjectListField({
   itemLabel,
   context,
   scopeGroups,
+  approvalScopeGroups,
 }: FlowObjectListFieldProps) {
   const external = React.useMemo(
     () =>
@@ -271,12 +279,12 @@ export function FlowObjectListField({
                       // 'expression' authors a CEL expression over the approval
                       // roots (current/trigger/vars). Render the expression
                       // input (mono + syntax check) instead of a dead free-text
-                      // reference box. The flow-scope picker/roots are
-                      // deliberately NOT wired in: approval expressions have
-                      // their own root set, and offering flow-scope paths here
-                      // (record.x, bare fields) would teach exactly the
-                      // spelling the runtime rejects; root validation runs
-                      // server-side (os lint + node-entry pre-check).
+                      // reference box, with the APPROVAL scope groups — never
+                      // the regular flow scopeGroups, whose record.x /
+                      // bare-field spellings the runtime rejects. The same
+                      // groups feed FlowExprIssue, so an out-of-contract root
+                      // warns inline with a "did you mean" before os lint /
+                      // the node-entry pre-check reject it server-side.
                       if (resolved === undefined && disc === 'expression') {
                         const raw = typeof row.values[col.key] === 'string' ? (row.values[col.key] as string) : '';
                         return (
@@ -290,11 +298,11 @@ export function FlowObjectListField({
                               onKeyDown={(e) => {
                                 if (e.key === 'Enter') (e.target as HTMLInputElement).blur();
                               }}
-                              groups={[]}
+                              groups={approvalScopeGroups ?? []}
                               placeholder={col.placeholder ?? 'current.<field> · trigger.<field> · vars.<node>.<key>'}
                               disabled={disabled}
                             />
-                            <FlowExprIssue value={raw} role="value" />
+                            <FlowExprIssue value={raw} role="value" scopeGroups={approvalScopeGroups} />
                           </div>
                         );
                       }

packages/app-shell/src/views/metadata-admin/inspectors/flow-scope.ts

@@ -31,7 +31,13 @@
  */
 
 /** Which group a reference belongs to (drives the picker's section headers). */
-export type ScopeGroupId = 'variables' | 'outputs' | 'loop' | 'trigger';
+export type ScopeGroupId =
+  | 'variables' | 'outputs' | 'loop' | 'trigger'
+  // #3447: approval `expression` approvers see a DIFFERENT root set than flow
+  // conditions (current/trigger/vars — never `record`/bare fields). Their
+  // picker groups carry their own ids so they can never leak into the regular
+  // condition picker.
+  | 'approval_current' | 'approval_trigger' | 'approval_vars';
 
 /**
  * One pickable reference. `token` is the BARE form (no braces); the picker
@@ -186,6 +192,18 @@ export function nodeOutputRefs(node: FlowNodeLike): ScopeRef[] {
     for (const f of asArray(cfg.fields)) add(str(asRecord(f).name));
   }
 
+  // Approval (framework#3447): the resume envelope writes `<nodeId>.decision`,
+  // and each author-declared decision output (`decisionOutputs` — bare key or
+  // typed `{ key, … }`) lands as `<nodeId>.<key>` — the values a later
+  // approval node's `expression` approver reads as `vars.<nodeId>.<key>`.
+  if (type === 'approval' && nodeId) {
+    add(`${nodeId}.decision`);
+    for (const entry of asArray(cfg.decisionOutputs)) {
+      const key = typeof entry === 'string' ? entry : str(asRecord(entry).key);
+      if (key) add(`${nodeId}.${key}`);
+    }
+  }
+
   return out;
 }