fix(automation,lint,spec): validate the expression slots a node's configSchema declares (#4027) - #4040
Merged
Merged
Conversation
…figSchema declares (#4027) A node type's designer `configSchema` and the config keys its validators traverse were two unreconciled lists. The engine's `registerFlow` pass and the author-time `objectstack validate` pass each hardcoded `config.condition` / `edge.condition` and assumed every other node string was a `{var}` template, so a declared expression property outside that hardcoded set was validated by nobody. That is how #3528 shipped. `screen.fields[].visibleWhen` has been on the `screen` descriptor since #3304 — typed `xExpression: 'expression'` (bare CEL), documented as such, offered to authors in Studio — and neither validator traversed it. An app authored the predicate in the other dialect (`'{createOpportunity} == true'`, the `{var}` form its sibling config keys use correctly) and it passed tsc, passed `objectstack validate`, passed registration, and failed silently at run time. Because `required` IS enforced, a field the author had made conditional rendered unconditionally and blocked Submit on an input the user was never shown: the run paused forever and no resume was ever issued. - `FLOW_NODE_EXPRESSION_PATHS` (spec) is the declared ledger of expression-bearing node config paths, each recording the dialect it takes, with a pure resolver that expands `fields[].visibleWhen` over a real config. - Both validators read it. A malformed predicate is now located and quoted at registration and at `objectstack validate`: `node 'screen_1' (screen) screen field visibleWhen at config.fields[1].visibleWhen`. - A reconciliation ratchet derives the expression properties from the LIVE descriptors and fails in both directions — a new `xExpression` property with no ledger entry, or a stale entry no descriptor declares. It walks every registered builtin, which is the audit the original triage never did. Dialects are recorded rather than assumed because there are three and two of them disagree about braces: bare CEL (`{…}` is the #1491 brace-trap), single-brace `{var}` flow interpolation (`{…}` is correct), and ADR-0032 §3's double-brace text template. Only bare-CEL slots are checked; `loop.collection` / `map.collection` are recorded as `flow-template` and skipped, since no validator implements their dialect and checking them under either of the others would reject every valid flow. A test pins that. `ActionDescriptor.configSchema` no longer claims `registerFlow()` validates `config` against it. It never did — `FlowNodeSchema.config` is `z.record(z.unknown())`, so types, `required`, `enum` and unknown keys remain unenforced. The TSDoc now states what is checked and what is designer-facing only, per Prime Directive #10: don't advertise a capability the runtime doesn't deliver. 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
documentation
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…ma describe change `content/docs/references/` is generated from the Zod source by `packages/spec/scripts/build-docs.ts` and gated by `check:docs`. Correcting `ActionDescriptor.configSchema`'s `.describe()` in the previous commit changed its generated row, so the checked-in reference went stale. Regenerated with `gen:schema && gen:docs` — a one-line diff, no other file touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
…apshot `check:api-surface` gates `@objectstack/spec`'s public exports against a checked-in snapshot. The expression-slot ledger adds five — `FLOW_NODE_EXPRESSION_PATHS`, `FlowNodeExpressionPath`, `FlowNodeExpressionRole`, `ResolvedFlowNodeExpression` and `resolveFlowNodeExpressions` — so the snapshot needed regenerating. Additive only: 0 breaking (nothing removed or narrowed), 5 added. `+5` lines and nothing else changed. Every step of the `TypeScript Type Check` job now passes locally: spec `tsc --noEmit`, `check:docs`, `check:skill-refs`, `check:react-blocks`, the workspace build, examples typecheck, downstream-contract typecheck, `check:api-surface` and `check:skill-examples` — plus the other six spec `check:*` gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
os-zhuang
marked this pull request as ready for review
July 30, 2026 06:21
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…o regenerate it (#4046) `packages/spec` has eight checked-in generated artifacts, each with its own CI gate, split across two jobs that run their gates sequentially — so the first stale artifact masks every one behind it and you get one red build per artifact. AGENTS.md documented the auto-gen boundary but never mapped a change to the generator it invalidates. #4040 paid that twice, neither time a logic error: a `.describe()` string landed in content/docs/references/ (check:docs), and one new export landed in api-surface.json (check:api-surface) — the second only visible once the first was fixed. Adds a change → gate → regenerate table plus a loop that runs all eight, and records that check:liveness / check:empty-state / check:skill-examples / check:react-conformance have no generator (a failure there is a real finding). Also documents a trap found while verifying it: check:api-surface reads the built dist/*.d.ts, so a stale dist reports exports as REMOVED when nothing was removed.
This was referenced Jul 30, 2026
Merged
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…emas (#4001 Tier-A) (#4071) Zod's default .strip silently discarded undeclared keys on authorable schemas — the instance kept parsing, so a mis-spelled or wrong-layer key shipped as metadata that quietly ignored the author's config (#3405, #1535). This lands the #4001 Tier-A slice inside the v17 breaking window. - docs/audits/2026-07-unknown-key-strictness-ledger.md: the authorable / wire / open triage (#4001 step 1), classified by "who writes this schema's input", with the ratchet's verified next targets. - shared/suggestions.zod.ts: new strictUnknownKeyError factory generalizing the #3746 hand-rolled map (alias table + length-relative edit distance + tombstone/wrong-layer guidance); ui/action.zod.ts re-homes onto it with byte-identical messages. - security/permission.zod.ts: PermissionSetSchema, ObjectPermissionSchema, FieldPermissionSchema, AdminScopeSchema are .strict(). EffectiveObjectPermissionSchema explicitly .strip()s back — response shapes stay wire-tolerant. - automation/flow.zod.ts: FlowSchema, FlowNodeSchema, FlowEdgeSchema, FlowVariableSchema are .strict(); a node's config record stays open (the executor's configSchema, #4027/#4040, and the ADR-0087 conversion layer own it). Three real defects the gate caught in its first two runs, all previously invisible — each key written by real code, silently dropped at parse: PermissionSetSchema could represent neither `description` (authored by the built-in default sets, read by the Setup projection) nor the ADR-0010 runtime protection envelope (stamped by applyProtection on every metadata type and round-tripped through getMetaItemLayered -> saveMetaItem; every sibling registered type already spread MetadataProtectionFields, permission was the outlier); and a dogfood fixture declared the object-level `sharingModel` on a flow. Two of the three are inverse drift — the direction the per-property liveness ledger cannot see. Migration is behavior-preserving by construction: any key now rejected was previously stripped and therefore never had a runtime effect.
This was referenced Jul 30, 2026
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…4001 step 3) (#4119) Third click of the unknown-key strictness ratchet (flow + permission in #4071, RLS / sharing / position in #4099). Approval is a v17-new authoring surface — tightened while young, before stored volume exists: - ApprovalNodeConfigSchema, ApprovalNodeApproverSchema, ApprovalEscalationSchema and DecisionOutputDefSchema are .strict() with fixable errors via the shared strictUnknownKeyError factory. - ApprovalNodeConfig's guidance is the ADR-0019 re-home map: steps -> successive approval nodes on the canvas, entryCriteria -> the entering edge's condition, onApprove/onReject -> the approve/reject out-edges, rejectionBehavior -> a declared back-edge (ADR-0044) with maxRevisions. - The published JSON schema (getApprovalNodeConfigJsonSchema) carries additionalProperties:false into the Studio property form AND registerFlow's per-node config validation (#4027/#4040). Verified the #3746 hazard: z.toJSONSchema on the strict lazySchema does not throw. Verified: spec 6995 tests + tsc clean; all 12 check gates; plugin-approvals 326 / service-automation 457; dogfood 72 files / 418 tests; showcase / crm / todo validate clean.
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
#4183) `packages/spec` has eight checked-in generated artifacts, each with its own gate, split across two CI jobs that run their gates SEQUENTIALLY. The first stale artifact therefore masks every one behind it: you fix it, push, and learn about the next on the following run. Two pushes on #4040 (`check:docs`, then `check:api-surface`), two more on #4161 (`check:spec-changes`, then `check:upgrade-guide`) — four round trips spent discovering something one local run could have said at once. Every gate runs; a failure does not stop the rest. The summary lists all stale artifacts with the exact `gen:` command for each. `--fix` regenerates ONLY what this run proved stale. Deliberately not a regenerate-everything button: blanket regeneration rewrites artifacts whose staleness you never saw, which is how a real semantic change lands silently inside a mechanical diff. The gate -> generator ledger reconciles against `package.json` on EVERY run, in both directions, rather than behind a `--self-test` flag. An unclassified `check:`/`gen:` script fails the run instead of quietly dropping out of coverage — otherwise the summary would still say "all artifacts up to date" while checking fewer, the exact class of lie this exists to remove. It proved itself by rejecting its own `package.json` entry on the first run. Two things it refuses to be silent about: the `check:api-surface` stale-`dist` trap is printed inline when that gate is the one failing (it reads the built `.d.ts`, so an unbuilt tree reports newly-added exports as breaking removals), and the four source audits it does NOT run are named, so "all up to date" never reads as "everything passed". It also reports that `gen:openapi` and `gen:sbom` have no gate at all. Verified both directions: clean built tree -> all 8 pass; a change injected into a migration step's `rationale` -> `check:upgrade-guide` fails and `--fix` regenerates that artifact and no other. That run also corrected an earlier reading of mine: the two ADR-0087 gates have different inputs — the conversion registry drives `spec-changes.json`, the rationale prose drives `protocol-upgrade-guide.md` — they had only appeared to fail together because #4161 changed both. AGENTS.md's hand-rolled loop over eight hardcoded gate names is replaced by the command; that list could not survive a ninth artifact, and the ledger can. 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.
Closes #4027. Follow-up to the #3528 line (#3771 / objectui#2899 / #3788 / hotcrm#505).
The gap
A node type's designer
configSchemaand the config keys its validators traverse are two hand-written lists that nothing reconciled. Both validators hardcoded the same two keys and assumed the rest:AutomationEngine.validateFlowExpressions(registration)config.condition,edge.conditionvalidateStackExpressions(objectstack validate)config.condition,edge.conditionIts own TSDoc stated the assumption outright — "node string fields are templates (a different dialect) and are validated by the template engine, not as CEL". A declared expression property outside those two keys was therefore validated by nobody.
That is how #3528 shipped.
screen.fields[].visibleWhenhas been on thescreendescriptor since #3304 — typedxExpression: 'expression', documented as bare CEL one comment above, offered to authors in Studio — and neither validator traversed it. HotCRM authored the predicate in the other dialect,'{createOpportunity} == true', the{var}form its sibling config keys use correctly. It passedtsc, passedobjectstack validate, passedregisterFlow, and failed silently at run time.Not cosmetic, because
requiredis enforced: a field the author had made conditional rendered unconditionally and blocked Submit on an input the user was never shown. The run paused forever, no resume was ever issued, and no layer produced a diagnostic.This PR
FLOW_NODE_EXPRESSION_PATHS(@objectstack/spec) — the declared ledger of expression-bearing node config paths, each recording the dialect it takes, plus a pure resolver that expandsfields[].visibleWhenover a concrete config (array indices filled in).Both validators read the ledger. A malformed predicate is now located and quoted at registration and at
objectstack validate:A reconciliation ratchet derives the expression properties from the live descriptors and fails in both directions — a new
xExpressionproperty with no ledger entry (the Console: screen-flow Submit never calls the resume endpoint — every screen flow is un-completable from the UI #3528 shape), or a stale entry no descriptor declares. It walks every registered builtin, which is the audit the original triage never did, and asserts the derivation is non-empty so it can't pass vacuously.Three dialects, not two — the part that nearly broke this
xExpressiontakes two values that disagree about braces, and a third dialect exists invalidateExpression:{x}screen.fields[].visibleWhenloop.collection,map.collection{var}flow interpolation{{ }}My first cut routed
xExpression: 'template'slots throughvalidateExpression's'template'role — which enforces the double-brace dialect and rejects a single{x}. A test I'd written for exactly this caught it: it would have failed every validloop.collectionin every existing flow.So the ledger records the dialect and only bare-CEL slots are checked.
loop/mapcollections are recorded asflow-templateand deliberately skipped — no validator implements their dialect. That is a real remaining gap, stated in the type's TSDoc rather than papered over, and the ledger gives a future validator one place to hook in. A test pins that a correct{tasks}still registers cleanly.Correcting a false contract claim
ActionDescriptor.configSchema's TSDoc claimedregisterFlow()"checks … thatconfigsatisfiesconfigSchema". It never did.FlowNodeSchema.configisz.record(z.unknown()), so types,required,enumand unknown keys are unenforced. Per Prime Directive #10 — never advertise a capability the runtime doesn't deliver — the doc now states exactly what is checked (declared bare-CEL slots) and what is designer-facing only.Test plan
@objectstack/spec@objectstack/service-automation@objectstack/linttsc --noEmit— spec, lintRatchet verified to fail without the fix. Deleting the
screenledger entry turns 3 tests red, reporting the missing path by name:New behavioural coverage asserts the exact HotCRM predicate is rejected, that the message names the node / label / indexed path / source, that the corrected bare CEL registers cleanly, that all malformed fields are reported rather than just the first, and that a screen's field names are not checked against the trigger object's schema (no hint is passed — otherwise every correct predicate would carry a spurious "unknown field" finding).
Findings not fixed here
tsc --noEmitonservice-automationhas 2 pre-existing errors inengine.test.ts(resumeAuthoritymissing from two descriptor literals, from automation: the generic run-resume route needs an authorization gate keyed on the suspended node #3801). Confirmed pre-existing by stashing this branch's changes and re-running — identical 2 errors. Untouched: the package'stestscript isvitest run, so tsc isn't gated there. Left alone per multi-agent discipline Add metamodel interfaces for ObjectQL/ObjectUI contract #1.notifyreadscfg.sourcebut itsconfigSchemadoesn't declare it (declared:sourceObject). A real instance of the same declared-vs-read drift in the opposite direction — the executor honours a nestedsource: {object, id}shape the designer never offers, via acfg.sourceObject ?? src?.objectconsumer fallback of exactly the kind Prime Directive Add comprehensive test suite for Zod schema validation #12 calls debt. Pre-existing and out of scope.configSchemaand the executor's wire payload are two unchecked lists — nothing validates flow node config keys at author time #4027 also asks that every property a designer declares be reconciled against what the executor reads. Only the expression subset has a ratchet here; a full one needs either static analysis of executors or a per-node hand-maintained ledger, and the two drift instances found (above) are designer-form changes I shouldn't make blind. Worth its own issue — thevisibleWhenclass specifically is now closed.🤖 Generated with Claude Code
https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
Generated by Claude Code