Skip to content

spec: ApprovalNodeConfigSchema.minApprovals describes "Default 1", but the quorum runtime defaults to ALL resolvable approvers #13809

Description

@huangyiirene

Filed unassigned by the flight-6 skills sweep (PR #13808, card #13793). Out of that PR's scope — the defect is in packages/spec, not in skills/** — so it is recorded here instead of ridden along.

The disagreement

packages/spec/src/automation/approval.zod.ts:

minApprovals: z.number().int().min(1).optional()
  .describe('Approvals required — total (quorum) or per group (per_group). Default 1'),

packages/plugins/plugin-approvals/src/approval-service.ts, isApprovalSatisfied:

if (behavior === 'quorum') {
  const n = original.length || 1;
  const need = Math.min(Math.max(1, config.minApprovals ?? n), n);   // omitted  =>  n, not 1
  return approved.size >= need;
}
if (behavior === 'per_group') {
  const perGroupNeed = Math.max(1, config.minApprovals ?? 1);        // omitted  =>  1
  ...
}

The same method's own doc comment states the runtime behaviour correctly — "quorum — at least minApprovals distinct approvals (default = all)" — so the two readings sit four lines apart in one file, and the schema's .describe() is the one that disagrees with them both.

The key carries no Zod .default(), so nothing materialises a value at parse time and the ?? on each branch is the whole story: the effective default is per behaviour, not a single number.

Why it matters

.describe() on this surface is not a comment — it lands in the generated reference docs and in the JSON schema published on the Approval action descriptor's configSchema, which is what the Studio property form and registerFlow() config validation are built from. So an author (or an AI author) reading the reference for behavior: 'quorum' is told that omitting minApprovals means one approval, when it actually means every resolvable approver must sign off.

The failure is quiet and in the unsafe direction: the node does not error, it simply waits for N approvals instead of 1, and the request looks "stuck" rather than misconfigured. quorum is also the one behaviour minApprovals exists for, so this is the default an author is most likely to lean on.

Suggested shape (not a decision — needs triage)

Two readings, and they are not equivalent:

  1. The text is wrong — correct the .describe() to state the per-behaviour default ("omitted: all resolvable approvers under quorum, 1 per group"). Cheapest, changes no behaviour, and makes declared match enforced.
  2. The runtime is wrong — give the key a real .default(1) so declared and enforced converge on the documented number. This CHANGES approval behaviour for every stored flow that omits the key under quorum (from unanimous-in-effect to first-approval), so it is a semantic migration, not a text fix.

Reading 1 is the one this filing recommends: the runtime's ?? n is deliberate (its doc comment names it, and the clamp beside it exists to stop a mis-set value deadlocking a request), so the text is the half that drifted. Reading 2 should not be taken without a maintainer ruling.

Related, already handled: the same wrong default had propagated into the published skill and is corrected in PR #13808 (skills/objectstack-automation/SKILL.md, the minApprovals node-config row). That correction is text-only and independent of whichever reading wins here.

Generated by Claude Code

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions