test(automation): pin the control-flow trio's designer forms (#4045) - #4064
Merged
Conversation
`loop` / `parallel` / `try_catch` had NO descriptor assertions — `config-schemas.test.ts`
covered the CRUD quartet, `assignment` and `screen`, so the designer's input for the
entire control-flow trio was unguarded and a change to those literals could not be
noticed.
These assertions pin intent, not just current bytes. All three publish a form
description that is deliberately SHALLOWER and LOOSER than its Zod counterpart in
`control-flow.zod.ts`:
- Region keys — `loop.body`, `parallel.branches[].nodes/edges`, `try_catch.try/catch`
— publish `{ type: 'array' }` with NO `items`, so a sub-graph stays opaque: it is
edited on the canvas, not in a property form. The Zod side is `FlowRegionSchema`
(`z.array(FlowNodeSchema)`), which would emit the whole FlowNode/FlowEdge
definition nested there.
- `collection` is `z.string().min(1)` and `iteratorVariable` is `.default('item')` in
Zod, yet the form publishes neither `minLength` nor `default`.
That difference corrects a premise this plan was resting on. #4045 — and my own
analysis on it — treated these three as redundant copies awaiting de-duplication by
a single Zod source, "pure deletion, most省力". They are not: they are a second
artifact with a different job (drive a form vs. validate a value), and the gaps are
deliberate. Naively generating them would nest a form for every node inside a loop
body. With the shapes pinned, any move toward a generated configSchema either keeps
these green or has to say out loud why the designer contract changed.
Verified to bite rather than merely pass: simulating what a naive `z.toJSONSchema`
swap would emit — deep `items` under `body.nodes` plus the Zod-only
`minLength`/`default` — turns both new `loop` tests red, naming
`loop.body.nodes must stay opaque (no items)`.
Test plan: service-automation 452 passed (4 new), ESLint clean. Tests only; empty
changeset since nothing releases.
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
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 6 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 08:02
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
…4091) `affected-docs.mjs` derived changed-package roots from every file under `packages/`, tests included. A test observes behaviour rather than defining it, so it cannot make an implementation-accuracy doc stale — yet every tests-only PR lit up its packages' whole doc set. Three in a row did it (#4064, #4078 and one before), each flagging 6 packages/services docs for a diff with no production code. This is the one place the mapper's deliberate over-inclusion actively hurt: a category that is ALWAYS false teaches the reader to skip the comment, and then it fails on the PR where it is right. - Test files are dropped before the roots are derived: *.test.* / *.spec.* at any depth (covering .integration.test.ts and .conformance.test.ts) plus __tests__ / __mocks__ / __fixtures__. - The narrowing is NOT silent — the excluded count rides the summary line and `testFilesSkipped` in --json. - `--self-test` pins the matcher (the check-error-code-casing / check-route-envelope convention), run by the workflow before it computes the mapping. - The workflow also triggers on the mapper's own paths, so editing it runs its own guard. CI surfaced that gap: on the first push the drift job never ran at all, which meant the self-test had been added but was structurally unreachable by the change most likely to break it. Verified against real history: 46e86ba (tests only) → 0 docs, was 6; 4965bfa (engine.ts + formula) → the same 11 docs as before; --all unchanged at 178. Swapping the anchored match for a naive path.includes('test') fails 7 self-test cases, including control-flow.zod.ts and latest.ts being swallowed as tests.
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
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 A of #4045, after #4050 (step 1) and #4059 (step 3a). Tests only, no production code.
The gap
config-schemas.test.tscovered the CRUD quartet,assignmentandscreen.loop/parallel/try_catchhad no descriptor assertions at all — the designer's input for the entire control-flow trio was unguarded, so a change to thoseconfigSchemaliterals could not be noticed by anything.…and a premise this plan had wrong
#4045 — and my own analysis on it — treated these three as the easy part: redundant copies of Zod that already exists in
control-flow.zod.ts, so step 4 would be "pure deletion, put it last". Reading them side by side to write these assertions shows that is wrong, and it would have been an expensive thing to discover mid-refactor.The literals are deliberately shallower and looser than the Zod:
loop.body,parallel.branches[].nodes/edges,try_catch.try/catch{ type: 'array' }, noitemsFlowRegionSchema=z.array(FlowNodeSchema)collectiontype: 'string'z.string().min(1)minLength: 1iteratorVariabletype: 'string'.default('item')default: 'item'The region case is the load-bearing one. A loop body is edited on the canvas; publishing its element schema would invite the designer to render a property form for every node inside the sub-graph. The absence of
itemsis the contract, not an oversight.So these are not duplicate sources pending merge — they are a second artifact with a different job (drive a form vs. validate a value), and the gaps are intentional. That reopens the option-1-vs-option-3 question for these nodes specifically: where two artifacts legitimately differ, what you want is the difference declared and enforced, not the difference erased.
What this PR does
Pins the intent, so the next person (or the next refactor) has an oracle:
expectOpaqueRegion()asserts each region key is an object whosenodes/edgesare arrays withitemsundefined — a named, explained assertion rather than a bare deep-equal.loop.collection.xExpression === 'template', themaxIterationsbounds againstLOOP_MAX_ITERATIONS_CEILING,parallel.branches.minItems === 2,try_catch'serrorVariableand theretrypolicy bounds.minLength/defaulton loop's string keys, so publishing the Zod-only constraints becomes a decision rather than a side effect.Verified to bite, not merely to pass
An assertion that never fails is decoration. I simulated what a naive
z.toJSONSchemaswap would emit — deepitemsunderbody.nodes, plusminLength/defaulton the string keys — and both newlooptests went red with the intended message:Test plan
@objectstack/service-automationEmpty changeset — tests only, nothing releases.
What this changes about the next step
Step B is now worth doing before any swap: run
z.toJSONSchema(LoopConfigSchema)for real and diff it against these pinned shapes, so the size of the gap is measured rather than inferred from reading. My table above is read from source, not from generated output. Depending on that result, the honest answer for region-bearing nodes may be a reconciliation ratchet (option 1) while the flat nodes —screen, CRUD,http,notify— go the single-Zod-source route (option 3), rather than one approach forced on all thirteen.🤖 Generated with Claude Code
https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
Generated by Claude Code