fix(app-shell): a published configSchema can no longer delete a node's sibling-block editors (objectstack#4045)

.changeset/flow-inspector-sibling-block-preservation.md

@@ -0,0 +1,42 @@
+---
+"@object-ui/app-shell": patch
+---
+
+fix(flow-designer): a published `configSchema` can no longer delete a node's sibling-block editors — objectstack#4045
+
+`FlowNodeInspector` resolved its form as `serverFields ?? fieldsForNodeType(type)`,
+so an engine-published `configSchema` **replaced the hand-written field group
+wholesale**. But a `configSchema` describes `node.config` and nothing else
+(ADR-0018), and `jsonSchemaToFlowFields` roots every field it emits at
+`['config', key]` — so the replacement silently deleted every editor rooted
+anywhere else.
+
+18 fields sit in that blast radius: `connectorConfig.*` (3), `waitEventConfig.*`
+(5), `boundaryConfig.*` (6) and the top-level `timeoutMs` (4). For `wait` and
+`boundary_event` those blocks are the node's **entire** contract.
+
+This already happened once. `connector_action`'s descriptor published a schema
+declaring `connectorId` / `actionId` / `input` as CONFIG keys, so against a live
+backend the generated form replaced the `connectorConfig` group — connector and
+action pickers included — and an author configuring a connector node in Studio
+wrote the trio to `node.config`, which the executor never reads. The node then
+refused to dispatch with `connectorConfig.connectorId and .actionId are
+required`. objectstack#4210 retired that schema on the server; this change is
+what stops the next mis-rooted one from doing the same to `wait` or
+`boundary_event`.
+
+New `mergeServerFlowFields()` splits the resolution by root:
+
+- **the server owns the config-rooted fields** — it is the authority on what the
+  executor actually reads, so its set replaces the hand-written config fields
+  rather than merging with them (a stale client key must not linger);
+- **non-config fields are always preserved** from the hand-written group, in
+  declared order;
+- a server field duplicating a preserved sibling key is **dropped**, not rendered
+  twice — two editors for one value, one of them writing where nothing reads, is
+  the same bug wearing a different hat.
+
+With no published schema the hand-written group is still used whole, unchanged.
+
+Verified by mutation: reverting to the old replacement turns all three new
+assertions red, one of which replays the `connector_action` incident directly.

packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx

@@ -31,7 +31,7 @@ import {
   InspectorEmptyState,
 } from './_shared';
 import {
-  fieldsForNodeType,
+  mergeServerFlowFields,
   localizeFlowFields,
   isFieldVisible,
   getFieldValue,
@@ -140,7 +140,11 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection,
   const fields = React.useMemo(() => {
     const schema = node?.type ? configSchemas[node.type] : undefined;
     const serverFields = schema !== undefined ? jsonSchemaToFlowFields(schema) : null;
-    const resolved = serverFields ?? fieldsForNodeType(node?.type);
+    // A published configSchema describes `node.config` ONLY, so it replaces just
+    // the config-rooted fields — the spec-structured sibling blocks
+    // (connectorConfig / waitEventConfig / boundaryConfig) and top-level
+    // `timeoutMs` are always kept from the hand-written group (framework#4045).
+    const resolved = mergeServerFlowFields(serverFields, node?.type);
     // Localize both the hardcoded table and the engine-published configSchema
     // fields (they share field ids for built-in nodes); no-op for English.
     return localizeFlowFields(node?.type, resolved, locale);

packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts

@@ -1,7 +1,14 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { describe, it, expect } from 'vitest';
-import { fieldsForNodeType, isFieldVisible, getFieldValue, configKeyOf } from './flow-node-config';
+import {
+  fieldsForNodeType,
+  mergeServerFlowFields,
+  isFieldVisible,
+  getFieldValue,
+  configKeyOf,
+  type FlowConfigField,
+} from './flow-node-config';
 
 describe('start node trigger-field gating (#5)', () => {
   const fields = fieldsForNodeType('start');
@@ -234,3 +241,85 @@ describe('notify node — first-class static config editor (#1895)', () => {
     expect(severity.options!.map((o) => o.value)).toEqual(['info', 'warning', 'critical']);
   });
 });
+
+/**
+ * A published `configSchema` describes `node.config` and nothing else, so it must
+ * not be allowed to delete the editors for a node's spec-structured SIBLING
+ * blocks (framework#4045).
+ *
+ * This is a real incident, not a hypothetical: `connector_action` shipped a
+ * descriptor whose `configSchema` declared `connectorId` / `actionId` / `input`
+ * as CONFIG keys. Against a live backend the generated form replaced this
+ * table's `connectorConfig.*` group — connector and action pickers included —
+ * so an author configuring a connector node in Studio wrote the trio to
+ * `node.config`, which the executor never reads, and the node refused to
+ * dispatch with "connectorConfig.connectorId and .actionId are required".
+ * framework#4210 retired that schema; these tests are what stop the next
+ * mis-rooted schema from doing the same to `wait` or `boundary_event`, whose
+ * whole contract lives in a sibling block.
+ */
+describe('server configSchema ↔ hand-written field merge (framework#4045)', () => {
+  const serverConnectorFields: FlowConfigField[] = [
+    { id: 'connectorId', path: ['config', 'connectorId'], label: 'Connector', kind: 'text' },
+    { id: 'actionId', path: ['config', 'actionId'], label: 'Action', kind: 'text' },
+    { id: 'input', path: ['config', 'input'], label: 'Input', kind: 'keyValue' },
+  ];
+
+  it('with no published schema, the hand-written group is used whole', () => {
+    expect(mergeServerFlowFields(null, 'connector_action')).toEqual(fieldsForNodeType('connector_action'));
+    expect(mergeServerFlowFields(undefined, 'wait')).toEqual(fieldsForNodeType('wait'));
+  });
+
+  it('the exact incident: a config-rooted connector schema cannot delete the connectorConfig pickers', () => {
+    const merged = mergeServerFlowFields(serverConnectorFields, 'connector_action');
+
+    // The structured editors survive, with their reference pickers intact.
+    const connectorId = merged.find((f) => f.path.join('.') === 'connectorConfig.connectorId');
+    const actionId = merged.find((f) => f.path.join('.') === 'connectorConfig.actionId');
+    expect(connectorId, 'connectorConfig.connectorId must survive a published schema').toBeDefined();
+    expect(actionId, 'connectorConfig.actionId must survive a published schema').toBeDefined();
+    expect(connectorId!.kind).toBe('reference');
+    expect(actionId!.ref?.kind).toBe('connector-action');
+
+    // And the server's mis-rooted duplicates do NOT ALSO appear — two editors for
+    // one value, one of them writing where nothing reads, is the same bug wearing
+    // a different hat.
+    expect(merged.filter((f) => f.path.join('.') === 'config.connectorId')).toEqual([]);
+    expect(merged.filter((f) => f.path.join('.') === 'config.actionId')).toEqual([]);
+    expect(merged.filter((f) => f.path.join('.') === 'config.input')).toEqual([]);
+  });
+
+  it('every node type with a sibling-block or top-level field keeps it under any schema', () => {
+    // The latent blast radius, enumerated: if a schema is ever published for one
+    // of these, this is what a plain replacement would have silently dropped.
+    const serverFields: FlowConfigField[] = [
+      { id: 'whatever', path: ['config', 'whatever'], label: 'Whatever', kind: 'text' },
+    ];
+    for (const type of ['connector_action', 'wait', 'boundary_event', 'subflow', 'script', 'http_request']) {
+      const nonConfig = fieldsForNodeType(type).filter((f) => f.path[0] !== 'config');
+      expect(nonConfig.length, `${type} should have sibling/top-level fields to protect`).toBeGreaterThan(0);
+      const merged = mergeServerFlowFields(serverFields, type);
+      for (const f of nonConfig) {
+        expect(
+          merged.some((m) => m.path.join('.') === f.path.join('.')),
+          `${type}: ${f.path.join('.')} was dropped by the published schema`,
+        ).toBe(true);
+      }
+    }
+  });
+
+  it('the server still owns the config-rooted fields', () => {
+    // The other direction: the engine is the authority on what the executor
+    // reads, so its config fields replace the hand-written ones rather than
+    // merging with them — a stale client key must not linger.
+    const merged = mergeServerFlowFields(
+      [{ id: 'brandNew', path: ['config', 'brandNew'], label: 'Brand new', kind: 'text' }],
+      'http_request',
+    );
+    expect(merged.some((f) => f.path.join('.') === 'config.brandNew')).toBe(true);
+    // `url` is in the hand-written http_request group but not in this schema.
+    expect(merged.some((f) => f.path.join('.') === 'config.url')).toBe(false);
+    // …while the top-level timeoutMs, which no configSchema can describe, stays.
+    expect(merged.some((f) => f.path.join('.') === 'timeoutMs')).toBe(true);
+  });
+});

packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts

@@ -881,6 +881,55 @@ export function fieldsForNodeType(type?: string): FlowConfigField[] {
   return FLOW_NODE_CONFIG[canonical] ?? [];
 }
 
+/** A field that edits `node.config.<key>` rather than a spec-structured sibling block. */
+function isConfigRooted(field: FlowConfigField): boolean {
+  return field.path[0] === 'config';
+}
+
+/**
+ * Merge the engine-published field set with the hand-written group for a node
+ * type (framework#4045).
+ *
+ * A published `configSchema` describes **`node.config` and nothing else** — that
+ * is what the descriptor's contract says (ADR-0018) and what
+ * {@link jsonSchemaToFlowFields} produces, since it roots every field it emits
+ * at `['config', key]`. But several node types keep part (or all) of their
+ * contract in a spec-structured SIBLING block on the node — `connectorConfig`
+ * (connector_action), `waitEventConfig` (wait), `boundaryConfig`
+ * (boundary_event) — or at the node top level (`timeoutMs`, four types).
+ *
+ * Replacing the whole group with the server's therefore deletes editors the
+ * server never claimed to describe. That is not hypothetical: `connector_action`
+ * shipped a `configSchema` declaring `connectorId`/`actionId`/`input` as CONFIG
+ * keys, and against a live backend it replaced this table's `connectorConfig.*`
+ * fields — connector and action pickers included — so an author writing a
+ * connector node online filled in keys the executor never reads, and the node
+ * refused to dispatch. framework#4210 retired that schema; this merge is what
+ * stops the next one from doing the same to `wait` or `boundary_event`.
+ *
+ * So the server owns the config-rooted fields (it is the authority on what the
+ * executor reads), and the hand-written non-config fields are always preserved,
+ * in their declared order, after them. When no schema is published the
+ * hand-written group is used whole, unchanged.
+ */
+export function mergeServerFlowFields(
+  serverFields: FlowConfigField[] | null | undefined,
+  type?: string,
+): FlowConfigField[] {
+  const handWritten = fieldsForNodeType(type);
+  if (!serverFields) return handWritten;
+  // A sibling-block field the server also described (by id) is not duplicated —
+  // the server's config-rooted version is dropped in favour of the structured
+  // editor, since a `config`-rooted duplicate would write where nothing reads.
+  const serverConfigFields = serverFields.filter(isConfigRooted);
+  const preserved = handWritten.filter((f) => !isConfigRooted(f));
+  const preservedKeys = new Set(preserved.map((f) => f.path[f.path.length - 1]));
+  return [
+    ...serverConfigFields.filter((f) => !preservedKeys.has(f.path[f.path.length - 1])),
+    ...preserved,
+  ];
+}
+
 /** Overlay a column's zh label / option labels (English is the fallback). */
 function localizeColumn(
   col: FlowConfigColumn,