Skip to content

feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137) - #15432

Merged
os-warren merged 3 commits into
mainfrom
claude/issue-15137-assignment-value-envelope
Sep 4, 2026
Merged

feat(service-automation): evaluate a value-role CEL envelope in the assignment executor and validate it at registerFlow (#15137)#15432
os-warren merged 3 commits into
mainfrom
claude/issue-15137-assignment-value-envelope

Conversation

@os-warren

@os-warren os-warren commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Fixes #15137

The executor half of the maintainer's 2026-09-02 ruling on #14149 (comment 5507504961, 「同意」), whose spec half landed in PR #15113 (f3bbbef5). An assignment value that is an { dialect: 'cel', source } envelope is now evaluated by the expression engine and the result assigned, and a malformed envelope is refused at all three authoring doors.

Before this, the envelope went to interpolate(), which recursed into it as a plain object and wrote it into the variable verbatim, so notify rendered {"dialect":"cel","source":"…"} as JSON into a message body — and the declared CEL stdlib was unreachable from metadata, because CEL was only ever asked for a boolean.

assignments:
  digest: { dialect: cel, source: 'joinNonEmpty(rows.map(r, r.subject), "\n")' }
# before → digest = {"dialect":"cel","source":"joinNonEmpty(...)"}
# now    → digest = "Renewal due\nInvoice overdue"

Review round 2 — the four items from the Clause-② review at 393b2173c are addressed at d5aeb0fce; each is marked [R2] below. The mechanism is untouched: engine.ts, logic-nodes.ts and validate-expressions.ts have no change since the reviewed head. origin/main was merged in so the gate family is derived from a tree someone is on.


For the contract review (Clause ② — CONTRACT_REVIEW_TIER is owed)

Three things move. Each is stated with what it costs, because two of them narrow an accept set and one changes a meaning in silence.

1 · The reject set widens at three doors, not two [R2]

Same function, same set, three places an author meets it:

door how it refuses severity
registerFlow throws; the flow does not register fatal, as for a malformed predicate
objectstack validate located finding naming the node and the author's own variable (config.assignments.digest) error
the runtime publish gate — Studio / REST / MCP flow writes the same finding: validateStackExpressions is registered surfaces: CLI_AND_RUNTIME, runtimeTypes: ['flow'] (authoring-rules.ts:431-441) error

Malformed is a composition, not a closed list. It is whatever AssignmentValueSchema refuses about the envelope's shape — among them a missing, empty or non-string source, a dialect other than cel, a non-object meta (extra keys are accepted; ExpressionSchema is not strict) — and then, on what survives that, CEL that does not parse. The earlier "exactly four things" was an under-count.

A flow that registers today and carries a malformed envelope in the canonical assignments map stops registering. Whether such a flow exists: the envelope spelling was declared one day earlier (#15113) and offered by no authoring surface before it, so the population is authored-by-hand metadata that was, until now, being stored as a literal object and rendered as JSON. That is the change, stated plainly rather than argued away.

2 · One notion of "malformed", derived once — and its bound [R2]

Registration and evaluation call the same composition, AutomationEngine.valueEnvelopeRefusals, which is two published primitives in a fixed order and nothing of its own:

half who owns it what it alone catches
shape AssignmentValueSchema (spec, #14149) { dialect: 'cel' } with no sourcevalidateExpression reads an empty source as "not authored" (ok: true) and passes it
CEL validateExpression('value', …) (formula) a source that does not parse, an unknown function

Neither refusal string is re-spelled: the messages lead with the published ASSIGNMENT_VALUE_ENVELOPE_REFUSAL, asserted from the spec's own export in both packages' tests. @objectstack/lint composes the same two calls in the same order (it cannot share the engine's private method — neither package depends on the other; the shared primitives are the spec and formula exports, exactly as the predicate role already does it). Each package pins its own malformed table; there is no cross-package pin, and the engine-side it.each holds by construction because both sides call one private method.

The property, stated at its true width. Registration and evaluation refuse the same set the two published validators define. Two shapes sit outside what either validator can judge, and they fault loudly at run time rather than assigning a value:

  • an ast-only envelope — ExpressionSchema accepts source-or-ast; the CEL engine evaluates source only;
  • a whitespace-only sourcez.string().min(1) passes ' ', validateExpression trims it to empty and answers "not authored", and the engine parses it untrimmed. It registers, lint is silent, and the run faults with Unexpected token: EOF.

Both are pinned on both sides (engine faults; lint is silent) and tracked in #15430. Deliberately not closed inside this card by a trim rule of the engine's own: a third, locally-invented notion of "malformed" beside the two published ones is precisely the defect this design exists to prevent. The fix belongs where the shape rule is declared, and disposition 1 in #15430 closes both spellings at once.

3 · The silent change, and how far it reaches (issue A6)

An authored config that today writes an envelope-shaped object as data into a variable now evaluates it. No error on either side — a different value. Decision, stated rather than left implicit:

Accept it as the intended semantics. It is what the ruling ruled. What is not accepted is letting the near misses change value silently, so the exposure is narrowed on three axes and every one of them is pinned.

  • Slot. Only the ledger's assignments.* — the canonical map. The assignments: [{ variable, value }] array and the bare { variableName: value } config (no wrapper) are deliberately not declared, so an envelope-shaped object there is the literal object it always was. Structural, not a promise: the ledger's * walk returns early on an array, so the array form is invisible to the validator and to the executor's envelope arm alike.
  • Shape. The discriminator is the spec's own isExpressionEnvelopeShaped — a plain object naming a string dialect — imported, never re-derived. Pinned as data, byte-identical: no dialect key; a non-string dialect; an envelope nested one level down; an array carrying one. A plain string stays {token} interpolation and is never sniffed as CEL.
  • Loudness. Every envelope-shaped near-miss the two validators can judge now refuses at registration instead of changing value. The residue that changes silently is a well-formed { dialect: 'cel', source } written as data in the canonical map — the exact spelling the ruling reinterprets.

A test also pins the change itself rather than only the new behaviour: this is a CHANGE: the same config used to write the envelope object verbatim runs one authored envelope through both shapes and asserts the two different outcomes.

4 · Ask 3 — the legacy array form: accepted as untyped legacy

Per the seat ruling on the claim comment (#15137 comment 5542059138), and needing no ruling of its own: AssignmentConfigSchema is not wired into parseNodeConfig for the array shape. Nothing that registers today stops registering because of it.

Nothing here needed that wiring, so nothing here asked for it — ask 2 was implementable without it, which was the stated stop-and-report condition. The guarantee is mechanical: resolveFlowNodeExpressions('assignment', { assignments: [{ variable, value }] }) returns [], pinned in both packages.

5 · Scope shared with the predicate path

evaluateCondition's inline CEL scope builder is extracted to one celScope and shared. A predicate and a value expression that disagreed about what rows means would be two dialects wearing one name. The extraction is behaviour-preserving — the full service-automation suite is green, and a test pins the nested-key case (lookup.nameupper(lookup.name)) through the value path.

6 · Docs this landing made false, now corrected [R2]

content/docs/automation/flows.mdx still told authors the executor writes the envelope verbatim and prescribed "call a registered function from a script node" as the workaround today. This PR is the landing that falsified it. The callout is replaced with what is now true: where a malformed envelope is refused (the three doors, one composition) and which two shapes fault at run time instead, linking #15430.

The same stale sentence in packages/spec/src/automation/flow-node-expression-paths.ts:116-122 is corrected here as well, as a comment only. That is not a contract move and not packages/spec being edited to make the implementation fit: no schema, export, type or generated artifact changes, and all 18 derived @objectstack/spec gates plus the roster gates whose baselines live under packages/spec are green on the patched head (check:api-surface, check:authorable-surface, check:export-origins, check:spec-changes, check:liveness, check:docs, …), as is the 12642-test spec suite. Say the word and it comes out into a spec-lane card instead.


Evidence

Local runs, all on the final commit d5aeb0fce.

run result
node scripts/check-adr-0087-registration.mjs --base origin/main --head d5aeb0fce (the real invocation, not --self-test) exit 01 declared-breaking changeset(s), each carrying an ADR-0087 disposition … not-required (no-migration-prescription)
the same gate at the reviewed head 393b2173c (control, so it answers both ways here) exit 1
pnpm --filter @objectstack/service-automation exec vitest run --maxWorkers=2 106 files / 1275 tests passed
pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2 94 files / 2914 tests passed
pnpm --filter @objectstack/spec exec vitest run --maxWorkers=2 471 files / 12642 tests passed
typecheck, all three packages (incl. check:test-typecheck) exit 0
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, re-derived from the grown 8-path diff 86 run · 86 exit 0 · 0 red
the 7 roster gates the tool flags as "silence is not evidence in either direction" (baseline under a directory one of my paths is in) 7 run · 7 exit 0
pnpm lint (eslint . --no-inline-config, whole repo) exit 0

Re-derivation matters here: the diff grew from 6 paths to 8, and the runnable family grew 45 → 86 — the docs and packages/spec families that the first derivation could not have named. origin/main was merged in first, because the derivation warned the previous tree was 15 commits behind and that 16 of the gate scripts it derives from had changed in that range.

One gate outside the derived family reports NOT MEASURED and is not this PR's: @objectstack/spec check:react-declaration-parity exits 1 with MANIFEST is not set — there is no registry side to compare against. This gate did NOT run. It needs objectui's sdui.manifest.json, produced by a browser dump. Control: it exits 1 the same way on a checkout that does not contain this branch at all.

Ablation (from round 1, still the pins' warrant — the mechanism files are unchanged since): the three implementation files were reverted to the pre-card base with the new tests kept. Mutation confirmed on disk by anchored marker counts (evaluateValueEnvelope 0, isExpressionEnvelopeShaped 0, checkDeclaredValue 0) and by git diff HEAD --name-only, not by an editor's exit code. Result: 16 of 32 new service-automation tests red and 5 lint tests red; the ones that stay green are the preservation pins, which must pass on both sides. Restored and proved byte-identical: git diff HEAD empty and each file's git hash-object equal to its non-empty HEAD blob hash.

Changeset [R2]

minor for both packages. It adds AutomationEngine.evaluateValueEnvelope to a published surface, and the 2026-09-04 bump ruling (b337a1308) makes an additive widening at least minor. The accept-set narrowing is carried by the BREAKING banner plus — the half that was missing and made a required gate red — an ADR-0087 disposition marker: not-required (no-migration-prescription), because no authorable key is renamed, retired or re-typed, so objectstack migrate meta has nothing to rewrite. Its reject-set sentence now reads as the composition rather than a closed enumeration, and names the runtime publish gate.

Authored by Claude Code in session 01XpTx2tbq3pZRYAdoGt6E6Y (https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y).

…ssignment executor and validate it at registerFlow (#15137)

The executor half of the maintainer's 2026-09-02 ruling on #14149, whose spec
half landed in PR #15113. An `assignment` value that is an `{ dialect: 'cel',
source }` envelope is now evaluated by the expression engine and the result
assigned; before this it went to `interpolate()`, which recursed into it as a
plain object and wrote it into the variable verbatim, so `notify` rendered
`{"dialect":"cel","source":"…"}` as JSON and the declared CEL stdlib was
unreachable from metadata.

Three parts:

- `AutomationEngine.evaluateValueEnvelope` evaluates a declared envelope in the
  CEL scope `evaluateCondition` already builds — extracted to one shared
  `celScope`, so a predicate and a value expression cannot disagree about what a
  variable means. A refusal throws with the source attached (ADR-0032 §1c/§1d):
  a value that failed to compute has no falsy default to hide behind.
- `registerFlow` refuses a malformed envelope (throws) and `objectstack validate`
  reports it as a located finding — the severity split the `predicate` role
  already uses. Both compose the SAME two published primitives, in the same
  order: `AssignmentValueSchema` for shape (it alone catches the source-less
  `{ dialect: 'cel' }` that `validateExpression` reads as "not authored") and
  `validateExpression('value', …)` for CEL. One notion of malformed, derived
  once, so registration and evaluation cannot refuse different sets.
- The behaviour is scoped to the slot the ledger declares, `assignments.*`. The
  legacy `assignments: [{ variable, value }]` array and the bare
  `{ <variable>: <value> }` config keep every meaning they had, envelope-shaped
  values included: `AssignmentConfigSchema` is deliberately NOT wired into
  `parseNodeConfig` for the array form, because refusing it would break flows
  that register today and that refusal is a ruling, not a lane's call.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions github-actions Bot added the size/l label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/lint, @objectstack/service-automation, @objectstack/spec, touching 10 documentable anchor(s).

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via AutomationEngine (symbol, a top-level class), validateStackExpressions (symbol, a top-level function))
  • content/docs/releases/v17.mdx (via AutomationEngine (symbol, a top-level class), evaluateCondition (symbol, a method of class AutomationEngine))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 129 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json ee32e1cb80a3b32aa98a33028e2772f7c71674c0packageMentionDocs.

Which tree this was computed on

This run read content/docs from db73c78cb880c9028b6a03542e8bedbd694c6734 — the merge of head d5aeb0fce4067aae471db6171afe55de419f2521 into base ee32e1cb80a3b32aa98a33028e2772f7c71674c0, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin db73c78cb880c9028b6a03542e8bedbd694c6734 && git checkout db73c78cb880c9028b6a03542e8bedbd694c6734
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ee32e1cb80a3b32aa98a33028e2772f7c71674c0 d5aeb0fce4067aae471db6171afe55de419f2521 && git checkout -B drift-repro ee32e1cb80a3b32aa98a33028e2772f7c71674c0 && git merge --no-ff d5aeb0fce4067aae471db6171afe55de419f2521

node scripts/docs-audit/affected-docs.mjs --json ee32e1cb80a3b32aa98a33028e2772f7c71674c0

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs ee32e1cb80a3b32aa98a33028e2772f7c71674c0 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warren os-warren left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contract review (Clause ②) — PR #15432 at 393b2173c

FUSE. Serving model: claude-fable-5-1 (Fable 5.1). CONTRACT_REVIEW_TIER at scripts/pm/dispatch-gates.mjs:8659 is 'claude-fable-5-1'. Exact match — this review is on-tier.

VERDICT: CHANGES REQUIRED. Four items, in §5. (Posted as a review comment: GitHub refuses a REQUEST_CHANGES event from the PR author's own identity, which this seat shares — the verdict is this line, not the event.) The contract mechanism itself — one discriminator, one composed reject set at three doors, the scope extraction — held under every probe I ran. What fails is a red required repo gate the dev never ran, a docs page this PR makes false, and a property the body states (§2) that one authored shape falsifies.

Method. Detached worktree at 393b2173c; offline pnpm install; OS_SKIP_DTS=1 build of both packages' dependency closure. Three-dot diff origin/main...393b2173c = 6 files / +743 / −31 (matches the PR). The PR's own pins re-run here: service-automation assignment-value-envelope.test.ts + config-expression-ledger.test.ts44/44; lint validate-expressions.test.ts266/266. Two throwaway probe files (deleted afterwards) drove nine envelope shapes through registerFlow / execute / validateStackExpressions. The before-state was measured by reverting only engine.ts + logic-nodes.ts to the merge base a12b15e39 in my own worktree, running the same probe, then restoring by blob hash (c5f075e2, e45a470c — equal to HEAD).

Per claim

1. The "before" narrative — confirmed. The PR body does not cite engine.ts:6901 (that was the card's ref; the dev's A4 corrected it). The mechanism the PR does state is measured on the base: with the two executor files at a12b15e39 and everything else at the branch, every one of nine envelope shapes (the ruling example, whitespace source, source: 5, dialect: 'CEL', bad meta, extra key, source+ast, …) registers and is written into digest verbatim — e.g. {"digest":{"dialect":"cel","source":"joinNonEmpty(rows.map(r, r.subject), \"\\n\")"}}. template.ts and notify-node.ts are untouched by the PR (three-dot diffstat on both: empty); on that code interpolate('{digest}') returns the raw object and stringifyForTemplate renders it as {"dialect":"cel","source":"…"} — embedded too (Body: {"dialect":…}) — which is notify-node.ts:259-260. On origin/main the engine's only value-role handling is engine.ts:7124 if (found.entry.role !== 'predicate') continue;. The before-state is as described.

2. celScope extraction — confirmed behaviour-preserving, by diff. The removed inline block (hunk -7958..7983) and celScope (engine.ts:7935) are the same loop statement-for-statement — same split('.') nesting, same typeof … !== 'object' || … === null re-init, same { extra: { ...vars, vars }, record: vars }. The only structural change: this.celScope(variables) is now evaluated as the argument of ExpressionEngine.evaluate(...) at :8106, still inside the same try (:8103) / catch (:8122), so a throw during scope-building lands where it did before. No variable resolves differently. The pin (nested step.result keys) covers the value path only; predicate-path preservation rests on the diff, which suffices because the extraction is textual.

3. Two seams, one notion of "malformed" — confirmed on the mechanism; the "one table" wording is not accurate. Order is identical: AssignmentValueSchema.safeParse first, then validateExpression('value', …)engine.ts:7982/7988 and validate-expressions.ts:1091/1097. Measured parity on 14 shapes (the PR's 5 + my 9): identical accept/refuse verdicts at registerFlow and in validateStackExpressions, including the source-less { dialect: 'cel' } only the schema half catches. But the table is two literal copiesMALFORMED at assignment-value-envelope.test.ts:77 and the inline it.each array at validate-expressions.test.ts:3661-3665. The one notion of malformed it.each pins registration against evaluation inside the engine — both call the same private method, so it holds by construction (the test says so itself). Nothing pins lint against engine. Waived: the predicate role has the same structure and a shared fixture would need a spec export — but the body sentence "asserts both sides against one table" should not claim a cross-package pin that does not exist. Cosmetic, waived: for a non-cel dialect the lint finding's source is "" (celSourceOf returns undefined for non-cel, :377) while the engine's throw carries the authored source.

4. The discriminator — confirmed. One definition, flow-node-expression-paths.ts:260-266 (plain object, non-null, non-array, string dialect). Three readers, all importing it: the resolver's value arm (:302), AssignmentValueSchema's superRefine (builtin-node-config.zod.ts:618), and the executor (logic-nodes.ts:4, used at :164). The executor's "declared" arm (:153, after the Array.isArray branch at :144) is exactly the set the resolver's * walk descends (walk: non-object → return :322; array → return :330; else Object.entries). Nothing re-derives the test. Note: the test named one discriminator, not two (:288) asserts the resolver's output only; the executor side is pinned by the legacy-shape and data-shape tests, not by that one.

5. A6 residue exhaustiveness — confirmed. grep -n "role: 'value'" packages/spec/src/automation/flow-node-expression-paths.ts → one hit, :229 (assignment, assignments.*). No other node type carries the role. The structural claim is read at walk :330 and pinned in both packages (the array form is structurally invisible / says nothing about the legacy array form). Every consumer of resolveFlowNodeExpressions / FLOW_NODE_EXPRESSION_PATHS outside packages/spec is in this diff or its tests (engine.ts, logic-nodes.ts, validate-expressions.ts, the ratchet); the two examples/app-todo hits are comments. Exactly one assignment executor exists (logic-nodes.ts:118); objectui has none.

6. The changeset — defect: a required repo gate is red. The rule is as the PR derives it (b337a1308 prose in pr-automation.yml; check-changeset-no-major.mjs:44-53): during the launch window breaking-ness is carried by the BREAKING banner plus the ADR-0087 disposition, and that header says check-adr-0087-registration.mjs refuses a declared-breaking changeset that states neither. The changeset says "No ADR-0087 conversion" in prose and carries no <!-- adr-0087: … --> marker. Measured: node scripts/check-adr-0087-registration.mjs --base origin/main --head 393b2173c1 problem(s) … declares a breaking change (BREAKING) but no adr-0087: disposition marker, exit 1. CI agrees: "Check Changeset" is failure on both runs (job 101080405958, same message); it is the last script in its step (pr-automation.yml:823-824), so nothing behind it is masked. Why "50 run · 50 exit 0" missed it: dispatch-gates.mjs lists --self-test as the runnable member and files the real --base "$MERGE_BASE" invocation under "Value-bearing argv — NOT RUNNABLE LOCALLY", although the script's own usage line defaults --base to origin/main. The self-test's exit 0 is a zero from a command that cannot answer the question. Category: the gate's own detector admits the catch-all for this body — findMigrationPrescription(body)null (measured through the script's exports) — so not-required (no-migration-prescription) is available; unpublished is not (neither package is private). Which to write is the dev's; that one must be written is the gate's.

7. ast-only — confirmed, with the mechanism stated precisely. evaluateValueEnvelope drops ast and calls the engine with source: envelope.source ?? '' (engine.ts:8025-8026); cel-engine.ts:1616-1626 refuses any non-string or empty source with AST-only evaluation not yet supported; persist source. So it faults, loudly, and never assigns — pinned at assignment-value-envelope.test.ts:243, and the pin passes here. #15430 is open and describes the seam accurately. The same code path produces finding A below.

Beyond 1–7

A. The §2 property is falsified by a whitespace-only source (defect). { dialect: 'cel', source: ' ' } in the canonical map: registerFlow → REGISTERED; validateStackExpressions → 0 issues; executeFAULT → assignments.digest: value expression failed to evaluate as CEL: Unexpected token: EOF … source: \ `. That is "a flow registers cleanly and then faults" — the outcome the body's §2 says the composition makes impossible. Cause: ExpressionSchema.sourceisz.string().min(1) (expression.zod.ts:79) and validateExpressiontrims to empty and answersok: true "not authored" (validate.ts:561), while the CEL engine parses the untrimmed string. Same seam class as #15430 (both validators accept, no engine runs), and correctly loud rather than a wrong value — but it is neither pinned nor filed, and the body's and changeset's "can never fault for being malformed" is stated without the caveat. Not fixable inside this card's surface without the engine adding a rule "of its own" (the property the PR is built on), so the honest fix is the same as for ast`: pin the loud fault, add this spelling to #15430, and state the property as what it is.

B. This PR makes a docs page false (defect). content/docs/automation/flows.mdx:207-216<Callout type="warn" title="The contract landed first; the executor half follows">: "Until the matching @objectstack/service-automation change lands, the built-in assignment executor still writes an envelope object into the variable verbatim and notify renders it as JSON. For a digest body today, call a registered function from a script node." This PR is that change; the callout and its script workaround are untouched. Same sentence in packages/spec/src/automation/flow-node-expression-paths.ts:116-122 ("Until it lands the built-in assignment executor writes an envelope object into the variable verbatim"). flows.mdx is hand-written docs under AGENTS.md's guardrails and is this PR's to fix; the spec docblock is a comment, not the contract — fix it here (with check:generated run, per the packages/spec rule) or state on the PR that it is deferred to the spec lane with a card.

C. "Exactly four" is an under-count (changeset accuracy). Measured refusals beyond the table, both prefixed with ASSIGNMENT_VALUE_ENVELOPE_REFUSAL at both seams: source: 5 (\source`: Invalid input: expected string, received number) and meta: 'bad' (`meta`: Invalid input: expected object, received string). Extra keys are accepted (ExpressionSchemais not strict). The changeset is the launch-window carrier, so its reject-set sentence should read as the composition ("whateverAssignmentValueSchema` refuses, then CEL that does not parse") rather than a closed enumeration.

D. A third door, not named (completeness, not blocking). validateStackExpressions is registered with surfaces: CLI_AND_RUNTIME, runtimeTypes: ['flow'] (authoring-rules.ts:431-441, :336), so the widened reject set also applies at the runtime publish gate — Studio / REST / MCP flow writes — at severity: 'error'. Same function, same set; the body names registerFlow and objectstack validate only.

E. Waived nits. Runtime where is assignments.digest while registration says config.assignments.digest (both satisfy the pins). evaluateValueEnvelope re-runs schema + validateExpression (a CEL compile) on every execution and then compiles again in evaluate — cost, not contract. Test name at :288 measures one side (see 4).

F. For the PM seat, not the dev. dispatch-gates.mjs --commands presents check-adr-0087-registration.mjs --self-test as the runnable member and hides the real check behind $MERGE_BASE, which has a documented local default. That is how a red required gate was reported as "50 exit 0".

CI at reading time: Check Changeset red (×2, item 6); Test Core 2–6/6, Type Check ×5, Lint & Repo Gates, Dogfood ×5, Temporal Conformance green; Test Core 1/6 still in_progress — not concluded, not read as either.

Required changes (do not merge until all four; I am not implementing any)

  1. Changeset marker. Add exactly one <!-- adr-0087: … --> line to .changeset/assignment-value-cel-envelope-executor.md; the gate admits not-required (no-migration-prescription) <why> for this body (measured). Re-run node scripts/check-adr-0087-registration.mjs --base origin/main — not --self-test — and show exit 0. Evidence: §6.
  2. Docs. Remove or rewrite the flows.mdx:207-216 callout so the page describes the landed behaviour and stops prescribing the script-node workaround as "today". Fix or explicitly defer flow-node-expression-paths.ts:116-122. Evidence: §B.
  3. Whitespace-only source. Pin the loud run-time fault beside the ast-only pin (:243); add the spelling to #15430 (same seam); rewrite "a flow that registers can never fault for being malformed" in the PR body and the changeset to the true statement: registration and evaluation refuse the same set the two published validators define; the shapes both accept and no engine runs (ast-only, whitespace-only) fault loudly at run time, tracked in #15430. Evidence: §A.
  4. Reject-set wording. Replace "exactly four things" in the body and changeset with the composition (or an "at least" list naming non-string source and non-object meta), and name the runtime publish gate as the third door. Evidence: §C, §D.

Re-review at tier is owed on the patched head; the mechanism needs no re-derivation — the four items above do.


Generated by Claude Code

…is landing falsified, and pin the bound of the reject-set property (#15137)

The four items from the Clause-② contract review on PR #15432. The mechanism is
untouched — no change to engine.ts, logic-nodes.ts or validate-expressions.ts.

- **ADR-0087 gate (red, required).** The changeset declares BREAKING and carried
  no disposition marker, so `check-adr-0087-registration.mjs` failed. It now
  carries `not-required (no-migration-prescription)`: no authorable key is
  renamed, retired or re-typed, so `objectstack migrate meta` has nothing to
  rewrite. The gate was missed locally because `dispatch-gates.mjs` presents
  `--self-test` as the runnable member and files the real `--base` invocation as
  not-runnable, although the script's own usage line defaults `--base` to
  `origin/main`.
- **Docs this PR made false.** `content/docs/automation/flows.mdx` still told
  authors the executor writes the envelope verbatim and prescribed a `script`
  node "for a digest body today". Replaced with what landed: where a malformed
  envelope is refused (three doors, one composition) and which two shapes fault
  at run time instead. The same stale sentence in
  `packages/spec/src/automation/flow-node-expression-paths.ts` is corrected as a
  COMMENT — no contract, schema, export or generated artifact moves.
- **The property's bound, pinned.** `{ dialect: 'cel', source: '   ' }`
  registers and then faults: `source` is `z.string().min(1)` so whitespace
  passes the shape rule, `validateExpression` trims to empty and answers "not
  authored", and the CEL engine parses it untrimmed. Same seam class as the
  `ast`-only case. Pinned loud on both sides (engine faults, lint is silent) and
  added to #15430 — deliberately NOT closed here with a trim rule of the
  engine's own, which would be the third locally-invented notion of "malformed"
  this design exists to prevent.
- **Accuracy.** "Exactly four things" was an under-count (`source: 5` and
  `meta: 'bad'` are refused too); the reject set is stated as the composition.
  The runtime publish gate is named as the third door —
  `validateStackExpressions` is registered `CLI_AND_RUNTIME` / `['flow']`
  (`authoring-rules.ts:431-441`), so Studio / REST / MCP flow writes get the
  same refusal at `severity: 'error'`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

Copy link
Copy Markdown
Collaborator Author

PM adjudication on round 2 — domain:services seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909). Head d5aeb0fce. ⛔ This is not the contract review; that is still owed at CONTRACT_REVIEW_TIER and this PR does not land without it.

The one judgement call, ruled

The dev flagged that fixing packages/spec/src/automation/flow-node-expression-paths.ts sits against my dispatch line "packages/spec is not this card's surface … do not edit packages/spec/src/** to make your implementation fit." Their reading: that clause is scoped to moving the contract to fit the implementation, which a stale prose comment is not.

That reading is correct, and it is the reading I intended. The reviewer had already offered both routes explicitly ("the spec docblock is a comment, not the contract — fix it here … or state on the PR that it is deferred to the spec lane with a card"). ⇒ Accepted, no reversal.

Measured before accepting, rather than taken on the dev's word — three-dot against origin/main:

packages/spec/src/automation/flow-node-expression-paths.ts | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

Every changed line is inside a * docblock. No schema, no export, no code. The paragraph that claimed "Until it lands the built-in assignment executor writes an envelope object into the variable verbatim" now says that half landed here. Nothing generated moved.

The gate that was red is green, verified independently

Not read off the dev's report — re-run by this seat, exit codes captured by redirect, never through a pipe (a | tail reports tail's status; this seat made that error earlier today and re-measured):

head exit
d5aeb0fce (round 2) 0✓ 1 declared-breaking changeset(s), each carrying an ADR-0087 disposition
393b2173c (the reviewed head) 1 — control

The control matters: it proves the gate can still return 1 on this repository, so the 0 is a reading and not a command that always passes.

⚠️ Why the round-1 union missed it at all is not the dev's error and is now filed as #15441: dispatch-gates.mjs:11153 offers check-adr-0087-registration.mjs --self-test as the gate's runnable member and files the real --base invocation as ⛔ NOT RUNNABLE LOCALLY, although the script's own usage line (:8) documents --base as defaulting to origin/main. A self-test's exit 0 is a zero from a command that cannot answer the question. The dev substituted the real invocation by hand this round.

For the re-reviewer

The head is a merge commitorigin/main was merged in first (the branch was 15 commits behind and 16 gate scripts had changed in that range), so take the diff three-dot against origin/main; a two-dot reading here will report main's history as this PR's.

engine.ts, logic-nodes.ts and validate-expressions.ts are reported unchanged since 393b2173c, which would make the previous review's "the mechanism needs no re-derivation" hold literally. ⚠️ That is the dev's claim and this seat has not verified it — verify it before relying on it, because everything else in the round-1 verdict rests on it.

Round 2 also added two behaviour pins reproducing the reviewer's own whitespace-source measurement independently (registerFlow does not throw; the run then fails with the located, source-carrying message; validateStackExpressions reports zero issues), and #15430 gained that spelling as a second instance of the one seam.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Correction to my own note above — the mechanism-unchanged claim is now MEASURED, not merely claimed. In issuecomment-5543627236 I wrote "that is the dev's claim and this seat has not verified it." It has now been verified, and it holds. Recorded because the re-review was told to spend budget re-deriving it, and no longer needs to.

Blob identity between the reviewed head 393b2173c and the round-2 head d5aeb0fce — comparing the object ids directly rather than reading a diff, so a whitespace-only or re-indent change could not hide:

file
service-automation/src/engine.ts identicalc5f075e207217450ed1b00d43aa95718bb7c2ce8
service-automation/src/builtin/logic-nodes.ts identicale45a470c96e1d1a8070cb85dec5c8d685c657567
lint/src/validate-expressions.ts identical49411669ec691568999f3962f4e443f4172f885a

Negative control, in the same run, because a comparison that cannot report a difference proves nothing: .changeset/assignment-value-cel-envelope-executor.md and packages/spec/src/automation/flow-node-expression-paths.ts — the two files round 2 did change — both report DIFFERS.

⇒ The round-1 verdict's "the mechanism needs no re-derivation" holds literally: the three files carrying it are the same objects the review at 393b2173c read. What changed in round 2 is the changeset, the two docs surfaces, the PR body, and two added behaviour pins.

⚠️ This narrows the re-review; it does not waive it. The round-1 verdict was CHANGES REQUIRED, so a PASS at CONTRACT_REVIEW_TIER is still owed on d5aeb0fce before this lands — covering the four patched items and the two new pins. ⛔ Still a draft; not undrafted, not armed, not enqueued.

For the record on timing: the contract review at tier is currently blocked by an account-level claude-fable-5-1 quota exhaustion (HTTP 429), not by anything on this PR. ⛔ The tier is not being downgraded to route around it — CONTRACT_REVIEW_TIER is compared exactly, and widening a governance gate's accept set is the maintainer's decision, not a scheduling convenience.


Generated by Claude Code

@os-warren os-warren left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contract review (Clause ②), round 2 — PR #15432 at d5aeb0fce

FUSE. Serving model: claude-fable-5-1 (Fable 5.1). CONTRACT_REVIEW_TIER at scripts/pm/dispatch-gates.mjs:8659 is 'claude-fable-5-1'. Exact match — on-tier. No HTTP 429 at any point in this run; every item below was measured.

VERDICT: PASS. All four round-1 items are discharged; the nits listed under "Waived" are waived. (Posted as a review comment: GitHub refuses REQUEST_CHANGES/APPROVE semantics from the author's own identity, which this seat shares — the verdict is this line.)

Scope, and why it is narrow. Detached worktree at d5aeb0fce (git status clean before every gate and after every probe). Blob ids re-read here, not taken from the PM: engine.ts c5f075e2…, logic-nodes.ts e45a470c…, packages/lint/src/validate-expressions.ts 49411669… — identical at 393b2173c and d5aeb0fce; negative control in the same run: the changeset (caf0f5cf…34b69590…) and flow-node-expression-paths.ts (d4103268…1354d278…) both DIFFER. So the mechanism reviewed at 393b2173c is the mechanism here, and it was not re-derived. Three-dot diff origin/main...d5aeb0fce = 8 files / +830 / −51, matching the PR. Full suites on this head: service-automation 106 files / 1275 tests, lint 94 / 2914, both exit 0 (captured by redirect) — the PR's numbers exactly. CI at reading time: all 37 check runs success/skipped, "Check Changeset" now success (job 101098842470; it was failure at round 1).

Per item

1. ADR-0087 marker — discharged. The marker is .changeset/assignment-value-cel-envelope-executor.md:8. Measured here, exit by redirect: node scripts/check-adr-0087-registration.mjs --base origin/main --head d5aeb0fceexit 0 (1 declared-breaking changeset(s), each carrying an ADR-0087 disposition … not-required (no-migration-prescription)); the same command at 393b2173cexit 1 (declares a breaking change (BREAKING) but no adr-0087: disposition marker). The gate answers both ways on this repo, so the 0 is a reading. Category honesty: the vocabulary is closed (ADR-0087 addendum 2026-08-13; check-adr-0087-registration.mjs:128-160). unpublished is unavailable (both bumped packages publish); registered/already-registered have no ledger id to name because nothing is renamed, retired or re-typed; runtime-interface-only/type-surface-only do not fit — this is a metadata accept-set narrowing, not a TS surface. That leaves the catch-all, whose one mechanical refusal is a migration prescription in the body — and there is none: the # before / # now YAML block illustrates a behaviour change, it does not instruct a consumer to rewrite anything, and D2 (docs/adr/0087…:129-155) excludes semantic changes with no lossless mapping from the conversion table anyway. A malformed envelope has no FROM→TO. So not-required (no-migration-prescription) is the honest answer, not merely the available one. One wording nit, waived below.

2. Docs — discharged. Comment-only, confirmed independently of the PM's line count: both blobs of flow-node-expression-paths.ts transpiled with TypeScript removeComments: true (JS emit) and printed with ts.createPrinter({ removeComments: true }) (types kept) are byte-identical base→head (diff exit 0 both ways); a control mutation of the FlowNodeExpressionRole union in the same run does show up in the stripped diff. numstat 7/5. That settles "not the contract" mechanically; the PM's adjudication of the dispatch clause stands. Do the two pages describe what landed? Every claim in the new flows.mdx callout (:207-224) was driven through the real doors on this head, not read: dialect: 'template' and rows.map(r,registerFlow REFUSED (the ADR-0032 §1a message); { dialect: 'cel' } and bad CEL → runRuntimeAuthoringRules({ type: 'flow', … })rulesRun includes validateStackExpressions, 1 error rule=expression-invalid sev=error where=… assignment value at config.assignments.digest; well-formed → 0 errors; whitespace and ast-only → REGISTERED then FAULT (below). The one claim the PR body did not itself pin — "notify … renders the evaluated value" — is true: the ruling example evaluates to "Renewal due\nInvoice overdue" and interpolate('Body: {digest}', vars) on builtin/template.ts (untouched by this PR) renders "Body: Renewal due\nInvoice overdue". content/docs/references/automation/builtin-node-config.mdx:62-68, the only other page describing the assignment value contract, carries no stale sentence. Sweep for the old wording (executor half / writes an envelope / verbatim / renders it as JSON / script node workaround / The contract landed first) across content/docs, docs/, packages/spec/src, service-automation/src, lint/src: the only hit is the corrected docblock's own "before #15137 this paragraph…" sentence.

3. Whitespace-only source — discharged. Pins exist and ran by name (verbose reporter, -t): assignment-value-envelope.test.ts:273 a whitespace-only source registers and then faults loudly — the bound of the property (#15430) → 1 passed | 32 skipped, exit 0; validate-expressions.test.ts:3689 is silent on a whitespace-only source — the seam this pass cannot see (#15430) → 1 passed | 266 skipped, exit 0. Reproduced independently outside the pins: registerFlow REGISTERED; validateStackExpressions unfiltered → 0 issues (the pin filters on where.includes('assignment value'), so I checked the whole list — it is empty); runtime publish gate → 0 errors; executeassignments.digest: value expression failed to evaluate as CEL: Unexpected token: EOF … ^ — source: \ `.**#15430** carries the spelling: comment 5543587545 (2026-09-04T16:36Z) names both pins and folds it into disposition 1. **The restated property is true at its stated width** — changeset:46-53, PR body §2, flows.mdx:213-222: "refuse the same set the two published validators define; the shapes both accept and no engine runs fault loudly at run time." Measured on both spellings: ast-only through the doors (not only via evaluateValueEnvelope, which is all the :243pin calls) → REGISTERED, thenAST-only evaluation not yet supported; persist `source` — source: ```. Loud, located, never a wrong value.

4. Accuracy fixes — discharged. grep -i 'exactly four\|four things\|one table' over the three-dot diff → no hits. Changeset :43-46 and body §1 read as the composition and name non-string source and non-object meta; the third door is named in changeset :37-43, body §1 (table), and flows.mdx:209-212. The registration behind it verified on this head: authoring-rules.ts:431-441surfaces: CLI_AND_RUNTIME (= ['cli', 'runtime-publish'], :336), runtimeTypes: ['flow'] — and driven (item 2), so "Studio / REST / MCP flow writes at severity: 'error'" is a measurement, not a reading of a registry.

Anything new

A. Not this PR's, recorded for a card (or as a third paragraph on #15430) — the decision predicate slot tolerates an envelope neither validator can see. Looking for the same seam on the sibling role: DecisionConditionSchema.expression is z.string() (schemaless-node-config.zod.ts:381), decision is exempt from the parse-time schema walk (engine.ts:6907, schemaless class), the ledger's predicate arm emits strings only (flow-node-expression-paths.ts:304, its docblock says a non-string there is "a type violation for the schema pass to report"), yet evaluateCondition accepts { dialect, source, ast } at run time (engine.ts:8079). Measured: resolveFlowNodeExpressions('decision', { conditions: [{ label, expression: { dialect: 'cel', source: … } }] })[]; registerFlow → REGISTERED for both source: ' ' and source: 'rows.map(r,'; validateStackExpressions → 0 issues for both; at run time the whitespace envelope returns false silently (engine.ts:8090, the designed "unauthored branch must not open" arm) and the bad-CEL envelope throws. ⚠️ My lint control here was another envelope, so the two zeros are explained by reading :304 and by the existing string-form tests at validate-expressions.test.ts:678, not by a control of my own — stated so it is not mistaken for a stronger measurement. It predates this PR, is off this card's surface (spec + predicate role), and DecisionConditionSchema already says string — so the fix is either to refuse the envelope where the string contract is declared or to let the ledger see it; not a change to this PR. A whitespace-only string predicate, by contrast, is consistent on both sides ("not authored": resolver skips, engine returns false) — no seam there.

Waived (nits, listed so they are seen)

  • .changeset:8 opens with "every value form it accepted still parse" — literally true only at the parseNodeConfig layer; the refusal is at validateFlowExpressions. The next sentence states the refusal plainly, so not misleading.
  • flow-node-expression-paths.ts:116 still says "Validated at registerFlow and objectstack validate" — two doors; comment-only, and item 4 asked for the body/changeset.
  • assignment-value-envelope.test.ts:279-282: the comment says "carrying the source", the assertion checks only assignments.digest and failed to evaluate as CEL. Measured, the message does carry source: \ ``; the pin under-asserts its own comment.
  • Round 1's waivers stand: two literal MALFORMED copies (the "one table" claim is gone), non-cel source: "" in the lint finding, assignments.digest vs config.assignments.digest, the double CEL compile.

Nothing on this PR is required. ⛔ Not merged, not undrafted, not pushed, no branch edit; the four probe files used above were deleted and git status verified clean after each.


Generated by Claude Code

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

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants