feat(spec,service-automation): a flow variable can declare a defaultValue, so "declared" means "bound" (#4697) - #6998
Merged
Conversation
…Value` (#4697) `FlowVariableSchema` gains an optional `defaultValue`, and the engine binds it whenever no parameter supplies one — so a declared variable is bound on every path and "declared" finally means "bound". Before this, the engine bound an `isInput` variable only when the caller supplied it (`params[name] !== undefined`), so every path that omitted the parameter left the name unbound. Conditions are strict CEL, where an unbound name does not read as `false` — it aborts the predicate and stops the run (hotcrm#643: a screen collecting an optional checkbox, whose untouched path aborted the outgoing edge and persisted nothing). Both run entry points (`execute` and `executeWithoutRetry`) now seed from one shared `seedDeclaredVariables` site, so the retry path behaves identically. Additive: a declaration without `defaultValue` behaves exactly as before. Also lands the docs half of the same gap — under strict CEL only the `vars.`-scoped `has(vars.X)` tests bindedness; the bare `has(X.f)` aborts on the very case it is written for. That truth table is measured against the live evaluator in a test rather than asserted.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 113 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
documentation
…lue (#4697) Generator output only (`gen:schema && gen:docs`), never hand-edited: the new key's row on the FlowVariable table, and the inline `variables` summary on the two Flow tables gaining its `…` truncation marker now that the shape has a fifth key. `check:docs` reports `230 generated files in sync with packages/spec`.
This was referenced Aug 9, 2026
os-zhuang
marked this pull request as ready for review
August 9, 2026 08:52
This was referenced Aug 9, 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
2 participants
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.
Fixes #4697
Plan A, as ruled.
FlowVariableSchemagains an optionaldefaultValue, and the engine binds it whenparams[name] === undefined— so a declared variable is bound on every path and "declared" finally means "bound". The docs half of the same gap lands with it.Premise: verified on origin/main, all three points hold
The issue predates a week of merges, so every claim was re-measured at
97b0798before a line was written.FlowVariableSchemais a strict{ name, type, isInput, isOutput }with no default keypackages/spec/src/automation/flow.zod.ts:116—strictObject, four keysexecutebinds a declared input only whencontext.params[name] !== undefinedengine.ts:2768(execute) andengine.ts:5723(executeWithoutRetry, the retry path) — identical copiesThe truth table, re-measured (both load-bearing rows, plus the composite guard):
So the issue's parenthetical is confirmed: the guard an author reaches for first —
has(X.f)— aborts on the very case it is written for, and only thevars.-scoped form tests bindedness.What changed
packages/spec—FlowVariableSchematakesdefaultValue: z.unknown().optional().defaultandinitialValuejoin the alias table, so the two words an author reaches for are rejected with a pointer rather than with a bare unknown-key error. One key joins the authorable surface:automation/FlowVariable:defaultValue.packages/services/service-automation— the two hand-copied seeding loops collapse into oneseedDeclaredVariablessite that bothexecuteandexecuteWithoutRetrycall, so the retry path cannot drift from the first attempt. Its rules:!== undefined—false,null,0and''are answers, not absences. Only a genuinely missing parameter falls through.defaultValuebinds when nothing was supplied.isInput: falsemeans no parameter can reach the name, so the default is the only thing that can bind it, and otherwise "declared means bound" would hold for only half the declarations.Additive. A declaration without
defaultValuebehaves exactly as before — existing flows parse and run unchanged, which the "leaves the variable UNBOUND when no default is declared" test pins directly.content/docs/automation/flows.mdx— the docs half. A newdefaultValuesection states the three rules above, the screen-flow case they exist for, and the guard table verbatim, with the prescription thathas(vars.X)is the only spelling that works and thatdefaultValueis still the better answer. Cross-linked from the screen-field note and from "Passing inputs".The contract question, answered from existing norms rather than guessed
Whether
defaultValuemust be validated against the variable's declaredtypewas the one question that could have been a new public-contract shape. It is not validated, and that is determined by the repo, not chosen here:typeis an openz.string()with no closed vocabulary to check against, and every otherdefaultValueon the authoring surface takesz.unknown()uncross-checked — a mapping's, an action param's, a page state slot's, a screen field's. A test pins the permissiveness explicitly, so adding such a check later reads as the deliberate new validation surface it would be rather than as a tightening nobody notices.Tests
packages/services/service-automation/src/flow-variable-default.test.ts(new, 16 cases) and four cases added topackages/spec/src/automation/flow.test.ts.Reverse verification, direction predicted first: with
engine.tsrestored toorigin/mainand the tests left in place, 5 of 16 go red — precisely the ones asserting that a default binds:The other 11 stay green, and that is the honest reading rather than a weaker result: the four
!== undefinedboundary cases pin that a supplied falsy value wins, which the old code already did — they would go red under aparams[name] || defaultValueimplementation, which is the mistake they exist to catch, not under the fix's absence. The truth-table cases measure the evaluator, not the change. The "WITHOUT a default the untouched path aborts" case pins unchanged behaviour and must stay green in both directions.That last pair is the defect end to end, reduced from hotcrm#643: a screen collects an optional field, the runner returns only what the user touched, and the outgoing edge reads the variable. Without a default the resume fails with
Unknown variable: createOpportunityand the downstream node never runs; with one it routes and completes.Verification run
pnpm --filter @objectstack/spec test— 349 files / 9094 tests passedpnpm --filter @objectstack/service-automation test— 72 files / 885 tests passedpnpm --filter @objectstack/spec typecheck— green (incl.check:scripts-typecheck,check:test-typecheck)pnpm --workspace-concurrency=2 --filter '...@objectstack/spec' typecheck— EXIT=0 across every consumer (runtime, rest, cli, client, client-react, all plugins/connectors/triggers/drivers, the three example apps, qa/dogfood)pnpm build --concurrency=2— 71 successful, 71 totalcheck:*step enumerated from.github/workflows/lint.yml(both jobs), plus the pr-automation blind spotnode scripts/check-adr-0087-registration.mjs --base origin/mainpackages/spec/authorable-surface/automation.jsongains exactly one line — the expected generated product of the new key, not a hand edit.authorable-surface.base.jsonis untouched: re-anchoring is a deliberate act with its own reviewed diff, never a side effect of a build.Generated by Claude Code