fix(automation,spec): graduate notify's nested source into the conversion layer (#4045) - #4050
Merged
Merged
Conversation
…ersion layer (#4045) The `notify` executor tolerated a second spelling of its click-through target with a bare consumer-side fallback — `cfg.sourceObject ?? src?.object`. Its own doc comment named `sourceObject`/`sourceId` canonical (they mirror the `sys_notification.source_object`/`source_id` columns), so the nested `source: { object, id }` form was an alias held up by exactly the mechanism Prime Directive #12 calls debt, and the one alias on this executor that #3796 missed when it moved `to`/`subject`/`body`/`url` into `flow-node-notify-config-aliases`. It graduates the same way `filters` → `filter` (#2645) and `object` → `objectName` (#3796) did: lifted onto the canonical pair at load, including the `AutomationEngine.registerFlow` rehydration seam, and the executor fallback deleted. `resolveSource` now reads only `cfg.sourceObject` / `cfg.sourceId`. Unlike the four renames this is a 1→2 destructuring, which `renameFlowConfigAliases`' pair mechanism cannot express, so it is a small custom transform on the existing entry (same retirement window — retires at 18). It mirrors the `??` precedence exactly: a canonical key already present WINS and its nested counterpart is left shadowed, matching `renameConfigKey`'s treatment of a shadowed alias. `source` is dropped once at least one part is lifted, since every part is then either lifted or shadowed; a `source` that is not a dict, or carries neither key, is left untouched rather than silently deleted. The fixture covers both shapes — a clean lift and a partially-shadowed one — at 7 expected notices. The pre-existing executor test for the nested form now proves the CONVERSION rather than executor tolerance, and is annotated to say so. Verified by disabling the lift: `source` comes back `undefined` and that test fails, so its pass is a real end-to-end check of the load seam, not a vacuous one. Regenerated the ADR-0087 artifacts the summary change invalidates (`spec-changes.json`, `docs/protocol-upgrade-guide.md`) per the table added in #4046. Test plan: spec 6934 passed, service-automation 440 passed, spec tsc clean, ESLint clean on changed files, and all eight spec generated-artifact gates PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
size/m
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 30, 2026 07:21
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…4045) (#4059) `FlowNodeSchema.config` is `z.record(z.unknown())`, so a misspelled or invented config key was accepted in total silence: `visibleIf` instead of `visibleWhen` registered cleanly, was never read, and the only symptom was a feature that quietly did not happen — the diagnostic vacuum that made #3528 take three passes. `registerFlow` now walks each node's `config` against its descriptor's configSchema and warns on anything undeclared, located to the exact path with the declared set listed. The walk descends where the schema declares structure and STOPS at free-form keyValue maps, whose keys are author data. Descending is load-bearing: `visibleWhen` is a property of screen's `fields[].items`, so a top-level-only check would miss the exact typo class this exists to catch. Warn, never reject. An undeclared key is an author typo, a key the executor reads that its configSchema never declared (`notify.source` was exactly this until #4050), or dead config — and only 4 of 13 schema-carrying builtins have been audited for the middle case. This warning measures the distribution; tightening is a later per-key decision. The published configSchema is unchanged, so no consumer sees a different shape. `@objectstack/formula` exports `nearestName`, the edit-distance helper already behind unknown-field/role suggestions. It is a bonus, not the mechanism: `visibleIf` → `visibleWhen` is distance 4 against a threshold of 3, so the declared set is printed always rather than only as a fallback. Measured over 30 flows in app-crm + app-showcase: exactly one warning, and it was a real finding — `showcase_inquiry_purge`'s get_record carried `mode: 'records'`, which no executor reads (`limit > 1` selects the list read), with a comment crediting `mode` for what `limit` actually does. Fixed here.
This was referenced Jul 30, 2026
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…heir Zod (#4045) (#4078) `loop` / `parallel` / `try_catch` each carry two descriptions of the same config — a hand-written configSchema on the descriptor (drives the Studio form) and a Zod schema in control-flow.zod.ts (validates the value) — and nothing compared them. #4064 pinned the shape of the first; this pins the relationship, so neither side can gain or lose a key without the other noticing. Both directions are asserted. A key the Zod accepts but the form omits is #3528's failure inverted: a real config key with no way to author it in Studio, discoverable only by reading the executor — exactly how notify.source sat unnoticed until #4050. A key the form offers but the Zod rejects means the author fills in a field whose value is then dropped. The two sources are deliberately NOT merged, now measured rather than assumed: generating the form from the Zod emits 9-17x more schema at +9 levels of depth (loop 597 -> 5,537 chars / 25 -> 201 keys; parallel 294 -> 5,112; try_catch 737 -> 10,739) with NO $defs and NO $ref, because FlowNodeSchema is inlined in full at every region key. A loop body would reach the designer as the entire node/edge definition instead of the opaque array a canvas-edited sub-graph needs. A projection pruning ~90% back off leaves three things to maintain instead of two. This corrects the premise #4045 opened with. Since the divergence is legitimate, what is enforced is that it stays DECLARED: a DELIBERATELY_SHALLOW ledger names each shallow key with a reason, and every entry must name a key both sides still declare, so it cannot rot. Compares key sets rather than generated shapes on purpose: generating needs z.toJSONSchema and service-automation does not depend on zod. `schema.shape` gives the key set without that dependency, and the key set is where the drift that matters lives. Verified to bite in all three directions — a Zod key the form omits, a form key the Zod rejects, and a ledger entry pointing at a renamed key each turn the suite red with the offending name in the message.
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…ntract only (#4045) (#4161) * fix(spec,service-automation): the wait executor reads its declared contract only (#4045) `wait` keeps its contract in `waitEventConfig` — a declared, `.describe()`-annotated block on `FlowNodeSchema` that sits in the authorable-field list, reaches the generated reference, and is what the showcase actually authors. Its descriptor publishes no `configSchema`, which is by design rather than the gap it looks like. The executor nevertheless also read six loose `config` keys behind `wec.X ?? loose.X`, two of them (`duration`, `signal`) spellings the spec never declared anywhere. That is the `notify.source` shape #4050 retired: a second de-facto contract announced only by a code comment, so an author who wrote it got a flow that worked forever and was never steered to the declared spelling (PD #12). Nothing in-repo authors it. - New ADR-0087 D2 conversion `flow-node-wait-event-config-lift` lifts config.{eventType,timerDuration,duration,timeoutMs,signalName,signal} onto the declared block, in the executor's own `??` precedence — a declared value wins and its loose counterpart is left shadowed, as `renameConfigKey` treats a shadowed alias. - `eventType` is stamped `'timer'` when the lift would otherwise leave the block without one. Load-bearing, not tidiness: the loader parses the CONVERTED flow (`applyConversionsToFlow` → `FlowSchema.parse`) and `waitEventConfig.eventType` is required once the block exists, so a stored flow carrying only `config: { duration: 'PT1M' }` would have gone from working to failing to load. `'timer'` is the exact default the executor applied to that shape. - The six `?? loose.*` fallbacks are deleted. The surviving `?? 'timer'` is not one: `waitEventConfig` is itself optional, and a node without it is a valid timer wait. Registered in both lists the layer keeps: `CONVERSIONS_BY_MAJOR[17]` and `step17.conversionIds`. The first draft missed the second and the repo's own ratchet caught it — `migrations.test.ts` replays every conversion fixture through the chain, so an unwired conversion fails there rather than shipping inert. Verified at the real seam: the new executor tests author the legacy shape and go through `registerFlow`, which is what applies the conversion. Negative control run by unregistering the conversion — two of the three fail without it; the third passes either way because it pins the executor's half of the precedence, and its name says so. A second negative control pins the `eventType` default: deleting it from the converted output makes `FlowSchema.parse` throw. Regenerated `spec-changes.json` and `docs/protocol-upgrade-guide.md` (the step rationale is a generated-artifact input). All 12 spec gates and 8 root gates pass; spec suite 7097 tests green. Deliberately unchanged and filed as #4158: `waitEventConfig.timeoutMs` is declared a timeout guard but read as a timer duration, and `onTimeout` has zero readers — `wait` has no timeout implementation at all, while the showcase authors `onTimeout: 'continue'`. Implementing or retracting that is a behaviour change, not a contract cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq * fix(examples): the showcase authors `wait` through the declared block (#4045) Corrects a factual claim in the previous commit. It said "Nothing in-repo authors it" of the loose `config` back door — that is wrong. The showcase's own `wait_revision` node authored exactly that shape: config: { eventType: 'signal', signalName: 'budget_revision' } so the back door was not hypothetical, and the example that demonstrates `wait` was itself on the spelling this PR retires. It moves to `waitEventConfig`. The conversion already handled this shape correctly — the executor's behaviour is identical either way, and a new test pins the exact combination the showcase hit: the DECLARED key names sitting in the UNDECLARED location, which is what the candidate ordering has to get right (`signalName` before `signal`). It also asserts the converted flow still parses, since the lift creates the required block. Found while checking whether a stuck `objectstack verify` CI step could be caused by this change rather than by the runner — it could not, but the audit that ruled it out is what surfaced the false claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Step 1 of #4045's staged plan. The
visibleWhenhalf of #4027 landed in #4040; this closes the one concrete declared-vs-read drift that investigation turned up.The drift
notify's executor tolerated a second spelling of its click-through target with a bare consumer-side fallback:Its own doc comment already named the winner —
sourceObject/sourceIdare "canonical — mirrors thesys_notification.source_object/source_idcolumns". Sosource: { object, id }was never a peer spelling; it was an alias held up by exactly the mechanism Prime Directive #12 calls debt:And it was the one alias on this executor that #3796 missed when it moved
to/subject/body/urlintoflow-node-notify-config-aliases. Consequences of it sitting outside the schema: an author using Studio's generated form could never produce the nested shape, hand-authored metadata carrying it worked with nothing describing it, and #4045's audit found it only by reading the executor.Why the conversion layer rather than declaring
sourceinconfigSchemaThe test is which direction reduces the number of contracts. Declaring it means the platform permanently admits two spellings — the designer then either offers one (and the schema lies) or offers both (and every author picks between a canonical and a non-canonical key). Converting means the load path normalises to one, the executor reads one, the
??dies, and the alias has a retirement date. Contracts stay at 1.This PR
Graduates it exactly like
filters→filter(#2645) andobject→objectName(#3796): lifted at load — including theAutomationEngine.registerFlowrehydration seam — and the executor fallback deleted.resolveSourcenow reads onlycfg.sourceObject/cfg.sourceId.It rides the existing entry rather than adding a new id, so it inherits that window (retires at 18) and needs no new migration-registry row.
Unlike the four renames, this is a 1→2 destructuring, which
renameFlowConfigAliases' pair mechanism cannot express — so it is a small custom transform,liftNotifySourceShape. It mirrors the old??precedence exactly:source: {object, id}, no canonical keyssourcedropped, 2 noticessourceObjectpresent +source: {object, id}idlifted (canonical wins, nestedobjectleft shadowed → no notice),sourcedroppedsourceleft untouched, no notice — matchesrenameConfigKey's shadowed-alias behavioursourcenot a dict, or carrying neither keysourceis dropped once at least one part is lifted, because by then every part is either lifted or shadowed and the executor only ever read.object/.id. The fixture covers a clean lift and a partially-shadowed node, at 7 expected notices.The pre-existing test now proves the conversion — verified, not assumed
notify-node.test.tsalready had a nested-sourcecase, and it goes throughregisterFlow+executerather than calling the executor directly. So with the fallback deleted it still passes — but only because the conversion runs. That makes it an end-to-end check of the load seam, and it is annotated to say so.I confirmed the pass is not vacuous by disabling the lift:
Test plan
@objectstack/spec@objectstack/service-automationtsc --noEmitRegenerated
spec-changes.jsonanddocs/protocol-upgrade-guide.md, which the summary change invalidates — caught up front using the table added in #4046 rather than by a red build.Remaining in #4045
Steps 2–4 are unstarted and need your call on the approach (my analysis and its verification data are in the issue). Headline from the feasibility probe: a Zod-first
configSchemareproduces the nestedxExpressionmarker, object-valuedxRef,format,enum+defaultand array item types correctly — butz.record()emitsadditionalProperties: {}rather than the load-bearingtrue, affecting 7 keyValue slots..meta({ additionalProperties: true })overrides it, and objectui accepts{}as keyValue anyway (json-schema-to-fields.ts:456), so it is the platform's ownconfig-schemas.test.tsthat is stricter than the consumer it protects.🤖 Generated with Claude Code
https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
Generated by Claude Code