feat(approvals): block a decision on a required output (#2955)

The platform half of this issue gives `decisionOutputs` a `required` flag: the
server rejects an approve that carries no value for one, before any write, so
the run can never resume past the node with the key a downstream `expression`
approver reads still missing.

The console mirrors it where the approver actually is. A required output is
marked required on the param it synthesizes, so the dialog flags the empty
field and refuses Confirm instead of sending a decision the server will 400.
APPROVE only, matching the server: a reject leaves down the reject edge where
nothing reads the outputs, so the reject dialog offers the same fields without
blocking on them — the two dialogs now differ in exactly that flag. Both
decision surfaces get it from the one shared helper, and a backend that
predates the field requires nothing, which is the behaviour of the previous
commit unchanged.

Verified against the showcase runtime (its dynamic-approval flow declares the
flag): the record header's Approve refuses with "Next Reviewers 为必填项" and
sends no request, its Reject offers the field unmarked, the Approval Center
drawer agrees on both, and a filled approve routes the co-sign node to the
picked user.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: baozhoutao

.changeset/decision-outputs-reach-both-decision-surfaces.md

@@ -41,7 +41,13 @@ The widget mapping now lives in one place (`utils/decisionOutputParams`), so
 the two surfaces cannot drift apart again, and the round trip through param
 resolution — the stage that actually broke — is pinned by tests.
 
-Not fixed here: `DecisionOutputDef` has no `required`, so a flow author still
-cannot demand that an approver fill an output before approving. That needs the
-spec-side field first (framework), and `onEmptyApprovers` remains the only
-backstop until then.
+**And a `required` output is now enforced at the field.** The spec grew
+`decisionOutputs[].required` (the platform half of this issue, shipping in
+`@objectstack/spec` + `@objectstack/plugin-approvals`) — the server rejects an
+approve carrying no value for one, before any write. The dialog marks those params
+required, so the approver is stopped at the empty field with the Confirm button
+refusing rather than by a 400 after the round trip. Only on APPROVE: the server
+never requires them on a reject (the run leaves down the reject edge, where
+nothing reads the outputs), so the two dialogs differ in exactly that flag. On a
+backend that predates the field nothing is required, which is the behavior above
+unchanged.

packages/app-shell/src/utils/decisionOutputParams.test.ts

@@ -93,10 +93,31 @@ describe('decisionOutputParams — declaration → param', () => {
     expect(free.helpText).toBe('t:actions.decisionOutput.helpMultiValue');
   });
 
-  it('declares no output as required — the flow author cannot demand one yet', () => {
-    // `DecisionOutputDef` carries no `required` flag (objectui#2955 follow-up);
-    // until it does, a hard-required picker would be unreachable metadata.
-    expect(decisionOutputParams([{ key: 'r', type: 'user' }], t)[0].required).toBe(false);
+  it('leaves an undeclared output optional', () => {
+    expect(decisionOutputParams([{ key: 'r', type: 'user' }], t, { decision: 'approve' })[0].required)
+      .toBe(false);
+  });
+
+  it('marks a `required` output required on the APPROVE dialog', () => {
+    // The server rejects a blank required output before any write; marking it
+    // here stops the approver at the field instead of at a 400.
+    expect(
+      decisionOutputParams([{ key: 'r', type: 'position', required: true }], t, { decision: 'approve' })[0].required,
+    ).toBe(true);
+  });
+
+  it('never marks it required on the REJECT dialog', () => {
+    // Mirrors the server: a reject leaves down the reject edge where nothing
+    // reads the outputs, so demanding them would trap the rejection.
+    expect(
+      decisionOutputParams([{ key: 'r', type: 'position', required: true }], t, { decision: 'reject' })[0].required,
+    ).toBe(false);
+  });
+
+  it('requires nothing when the caller names no decision', () => {
+    // A surface that has not said which decision it is collecting for must not
+    // block — and a pre-`required` backend sends the flag on nothing anyway.
+    expect(decisionOutputParams([{ key: 'r', required: true }], t)[0].required).toBe(false);
   });
 });
 
@@ -122,6 +143,16 @@ describe('decisionOutputParams — the params survive resolution (objectui#2955)
     expect(widgetFor(param)).toMatchObject({ type: 'user', multiple: true });
   });
 
+  it('the required flag survives resolution, so the dialog can block on it', () => {
+    // `paramToField` feeds `ActionParamDialog`'s required check — if the flag
+    // were dropped in resolution the dialog would submit a blank value and let
+    // the server 400 instead.
+    const [param] = decisionOutputParams(
+      [{ key: 'k', type: 'position', required: true }], t, { decision: 'approve' },
+    );
+    expect(widgetFor(param)).toMatchObject({ type: 'lookup', required: true });
+  });
+
   it('an untyped output stays free text', () => {
     const [param] = decisionOutputParams([{ key: 'note' }], t);
     expect(widgetFor(param)).toMatchObject({ type: 'text' });

packages/app-shell/src/utils/decisionOutputParams.ts

@@ -41,6 +41,13 @@ export interface DecisionOutputDef {
   /** Record kind to pick (`user` / `department` / `position` / `team`); absent → free text. */
   type?: string;
   multiple?: boolean;
+  /**
+   * The approver must supply this one to APPROVE. The server enforces it
+   * (`decide()` rejects a blank required output before any write); the dialog
+   * mirrors it so the approver is stopped at the field rather than by a 400.
+   * Absent on a backend that predates the flag — then nothing is required.
+   */
+  required?: boolean;
 }
 
 /** Param-name prefix the api handler folds back into the nested `outputs` body. */
@@ -91,19 +98,23 @@ function humanizeKey(key: string): string {
  * so no `_actions.<action>.params.*` bundle entry can ever match them and the
  * literal we pass IS what renders (objectui#2762 P0-3).
  *
- * Every output is optional: `DecisionOutputDef` carries no `required` flag, so
- * a flow author cannot yet demand one (objectui#2955 follow-up) — an omitted
- * output simply isn't sent.
+ * A `required` output is marked required on the APPROVE dialog only — pass
+ * `{ decision: 'approve' }`. That mirrors the server, which enforces `required`
+ * on approve and never on reject (the run leaves down the reject edge, where
+ * nothing reads the outputs, so demanding routing data to say "no" would trap
+ * the rejection). Marking it here is what turns the server's 400 into a
+ * blocked Confirm button with the field flagged, instead of a round trip.
  */
 export function decisionOutputParams(
   defs: DecisionOutputDef[],
   t: (key: string) => string,
+  opts: { decision?: 'approve' | 'reject' } = {},
 ): Array<Record<string, unknown>> {
   return defs.filter((d) => d && d.key).map((d) => {
     const base = {
       name: `${DECISION_OUTPUT_PARAM_PREFIX}${d.key}`,
       label: d.label ?? humanizeKey(d.key),
-      required: false,
+      required: opts.decision === 'approve' && d.required === true,
     };
     if (d.type === 'user') {
       return {

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

@@ -145,12 +145,19 @@ const DeclaredActionButton: React.FC<{
       // handler folds them into the nested `outputs` body the decide route
       // expects. A free-text output accepts comma-separated values (the
       // service accepts CSV for multi-id outputs).
-      const isDecideAction = /\/(approve|reject)$/.test(String((action as any).target ?? ''));
+      // Which decision this action records — `required` outputs are enforced
+      // on approve only (server and dialog agree), so the reject dialog offers
+      // the same fields without blocking on them.
+      const decision = /\/approve$/.test(String((action as any).target ?? ''))
+        ? 'approve' as const
+        : /\/reject$/.test(String((action as any).target ?? ''))
+          ? 'reject' as const
+          : undefined;
       // Widget mapping (typed picker vs free text) lives in the shared helper,
       // so the record header's Approve/Reject renders the same controls
       // (objectui#2955).
-      const outputParams = isDecideAction
-        ? decisionOutputParams(decisionOutputDefs(recordData), t)
+      const outputParams = decision
+        ? decisionOutputParams(decisionOutputDefs(recordData), t, { decision })
         : [];
       const dispatch: any = {
         ...rest,

packages/app-shell/src/views/RecordDetailView.approvalDecisionActions.test.tsx

@@ -103,6 +103,25 @@ describe('buildApprovalDecisionActions — decision outputs (objectui#2955)', ()
     });
   });
 
+  it('requires a `required` output to approve, but never to reject', () => {
+    const actions = byName(
+      buildApprovalDecisionActions(
+        pending({
+          decision_output_defs: [
+            { key: 'parallel_positions', type: 'position', multiple: true, required: true },
+          ],
+        }),
+        t,
+      ) as any[],
+    );
+    const paramOf = (name: string) =>
+      (actions[name].actionParams as any[]).find((p) => p.name === 'outputs.parallel_positions');
+    expect(paramOf('approve_request').required).toBe(true);
+    // Mirrors the server, which enforces `required` on approve only — a reject
+    // blocked on routing data the reject edge never reads would trap it.
+    expect(paramOf('reject_request').required).toBe(false);
+  });
+
   it('collects nothing extra when there is no pending request', () => {
     const [approve] = buildApprovalDecisionActions(null, t) as any[];
     expect((approve.actionParams as any[]).map((p) => p.name)).toEqual(['comment']);

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

@@ -172,9 +172,13 @@ export function buildApprovalDecisionActions(
     // intent has to ride the type.
     type: 'textarea',
   };
-  const decisionParams = [
+  // `required` outputs are enforced on approve only — the server rejects a
+  // blank one there and never on reject, so the two dialogs differ in exactly
+  // that flag and nothing else.
+  const defs = decisionOutputDefs(pendingRequest);
+  const decisionParams = (decision: 'approve' | 'reject') => [
     commentParam,
-    ...decisionOutputParams(decisionOutputDefs(pendingRequest), (key: string) => t(key)),
+    ...decisionOutputParams(defs, (key: string) => t(key), { decision }),
   ];
   // A pending approval is THE decision the approver came to make, so the
   // decision buttons must outrank app `record_header` actions rather than being
@@ -194,7 +198,7 @@ export function buildApprovalDecisionActions(
       order: -100,
       locations: ['record_header'],
       refreshAfter: true,
-      actionParams: decisionParams,
+      actionParams: decisionParams('approve'),
       successMessage: t('approvals.approveSuccess', { defaultValue: 'Approved' }),
     },
     {
@@ -208,7 +212,7 @@ export function buildApprovalDecisionActions(
       locations: ['record_header'],
       refreshAfter: true,
       confirmText: t('approvals.rejectConfirm', { defaultValue: 'Reject this approval request?' }),
-      actionParams: decisionParams,
+      actionParams: decisionParams('reject'),
       successMessage: t('approvals.rejectSuccess', { defaultValue: 'Rejected' }),
     },
   ] as unknown as ActionDef[];

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

@@ -269,6 +269,31 @@ describe('DeclaredActionsBar chrome localization (objectui#2762)', () => {
     expect(screen.getByRole('toolbar')).toHaveAttribute('aria-label', '决策');
   });
 
+  it.each([
+    ['/api/v1/approvals/requests/{id}/approve', true],
+    ['/api/v1/approvals/requests/{id}/reject', false],
+  ])('marks a required output required on %s → %s', async (target, expected) => {
+    // The server enforces `required` on approve and never on reject; the two
+    // dialogs must differ in exactly that flag (objectui#2955).
+    render(
+      <DeclaredActionsBar
+        objectName="sys_approval_request"
+        record={{
+          ...REQUEST,
+          decision_output_defs: [{ key: 'parallel_positions', type: 'position', required: true }],
+        }}
+        location="record_section"
+        actions={[{
+          name: 'decide', type: 'api', label: 'Decide', target, locations: ['record_section'],
+        }] as any}
+      />,
+    );
+    fireEvent.click(screen.getByText('Decide'));
+    await waitFor(() => expect(executeSpy).toHaveBeenCalled());
+    const params = executeSpy.mock.calls[0][0].actionParams as Array<Record<string, unknown>>;
+    expect(params[0].required).toBe(expected);
+  });
+
   it('hands the picker target under `reference`, the key that survives resolution', async () => {
     // objectui#2955: the params are spelled for `resolveActionParams`, which
     // rebuilds an inline param from a fixed key list and reads the target from