Skip to content

[finding] approvals.mdx:302 says the fallback pairing is "refused at authoring time" — nothing parses the node config before flow execution, so the refusal fires at node entry #18551

Description

@os-litant

Named by the contract review of PR #18525 (card #17931) as a required correction, and filed rather than fixed in-branch because the fix would move the head and void a review record that is otherwise complete. ⛔ Not a defect in #18525's accept set, runtime or semver level — the review returned PASS.

The false sentence

content/docs/automation/approvals.mdx:302 says:

fallback without a fallbackApprovers list is refused at authoring time

It is not refused at authoring time. Measured by the review at PR #18525's head:

  • registerFlow accepts the flow. Its only config check is the undeclared-key walk (engine.ts:8807), and after feat(spec): approval onEmptyApprovers gains 'fallback' with a sibling fallbackApprovers #18525 fallbackApprovers is a declared key, so the walk passes it.
  • os lint has no pairing arm for this constraint.
  • defineFlow leaves node config as an open record (flow.zod.ts:431 types node config as z.record(z.string(), z.unknown())).
  • The only non-test parser of ApprovalNodeConfigSchema in packages, apps and examples is plugin-approvals/src/approval-node.ts:133 — the executor, at node entry.

⇒ the refusal fires at flow execution, when the approval node's config is parsed — loudly, naming both keys, before any approval request opens. The contract is enforced; the sentence is wrong about when.

⚠️ Why this misleads rather than merely being imprecise: the same guide uses "authoring time" for os lint at lines 25 and 102. A reader who has just read those two will expect os lint to catch a mis-paired fallback. It will not.

Two ways to make it true — this card should pick one, not both by default

  • (a) Reword the sentence to say the refusal happens when the node's config is parsed at flow execution. Doc-only, no changeset impact, minutes.
  • (b) Deliver a lint pairing arm so the sentence becomes true as written. The pairing IS statically enforceable on shape — both keys sit in the same object — so this is buildable, and it would move a whole typo class from run time to author time. Larger, and it needs its own judgement about rule id, severity and the existing approval-approvers-may-resolve-empty rule's scope.

⛔ Do not do (a) and silently drop (b): if (b) is wanted, (a)'s wording has to be re-reverted when it lands. Whoever takes this should say which one they are doing.

One more thing to correct while in this file

The review also measured the PR-body narrative of #18525 as over-stated: "a stored node declaring the policy with no list … degrades to admin_rescue". Through the flow executor such a node fails at the Zod parse with the refinement's message; the degrade branch is reachable only by direct openNodeRequest callers — which the in-code comment ("a caller that bypassed the schema") states correctly. The code comment is right; only the PR body was loose. Nothing in the tree needs changing for this, but do not inherit the PR body's framing when writing the replacement sentence.

Dedupe words: approvals.mdx refused at authoring time, fallbackApprovers pairing lint arm, checkFallbackApproversPairing when it fires, approval node entry parse not registerFlow, onEmptyApprovers fallback doc timing.


Generated by Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions