From 6e1b4c415fc38cac27acd4f4d1b64162e9742abe Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 01:28:20 +0000 Subject: [PATCH 1/2] fix(app-shell): declare the spec defaults lockRecord and interrupting already apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `approval.config.lockRecord` and `boundary_event.boundaryConfig.interrupting` declared no `defaultValue` while the installed `@objectstack/spec` (17.4.0) materialises `true` for both keys when a node omits them. Since the boolean config control began seeding its checked state from `defaultValue`, declaring nothing is not a missing claim on these two controls — it is the wrong one. Both boxes drew UNCHECKED: the inspector told the author the record stays editable while an approval is pending (the runtime locks it) and that a boundary event leaves its host activity running (it cancels it). Both values are derived from the installed spec, and the acceptance pin re-derives them from `ApprovalNodeConfigSchema` / `FlowNodeSchema` at assertion time rather than restating the literal, so an upstream flip reddens instead of drifting. Three rows per field: the declaration against the contract, the RENDERED checkbox (which reads the spec directly, so dropping a declaration reddens it on the control the author sees), and the stored-`false` row that an always-checked control fails. The exact-count declaration pin moves with them, count and comment together. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ --- ...ockrecord-interrupting-declared-default.md | 32 ++++ ...FlowNodeInspector.declaredDefault.test.tsx | 138 +++++++++++++++++- .../inspectors/flow-node-config.ts | 21 ++- 3 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 .changeset/9277-flownode-lockrecord-interrupting-declared-default.md diff --git a/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md b/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md new file mode 100644 index 0000000000..db8e7dc02a --- /dev/null +++ b/.changeset/9277-flownode-lockrecord-interrupting-declared-default.md @@ -0,0 +1,32 @@ +--- +'@object-ui/app-shell': patch +--- + +Flow-node inspector: `approval.lockRecord` and `boundary_event.boundaryConfig.interrupting` +now declare the `defaultValue` the installed spec applies, so their checkboxes stop +asserting the opposite of what the runtime does (objectui#9277). + +Both keys default to `true` in `@objectstack/spec` (measured on 17.4.0): +`ApprovalNodeConfigSchema` materialises `lockRecord: true` for an approval config that +omits the key, and the `FlowNodeSchema` boundary block materialises +`interrupting: true`. The hand-written descriptor table declared neither. Since +objectui#8451 a boolean config control seeds its checked state from `defaultValue`, so +declaring nothing does not render as "no claim" — it renders as UNCHECKED. An author +opening a node that omits the key read `Lock record` unchecked and believed the record +stayed editable while the request was pending (the runtime locks it), and read +`Interrupting` unchecked and believed the boundary event would leave its host activity +running (it cancels it). + +Both values are derived from the installed spec rather than typed from taste, and the +acceptance pin re-derives them from `ApprovalNodeConfigSchema` / `FlowNodeSchema` at +assertion time instead of restating the literal `'true'`, so the next upstream flip +reddens there rather than drifting silently the way objectui#6620 did. + +Nothing is written. `defaultValue` is shown, never committed (objectui#6263's standing +ruling), and a stored `false` still beats the declaration on both fields — a +deliberately non-locking approval and a non-interrupting boundary event stay authorable +and keep rendering unchecked. + +`patch`, not `minor`: no prop, option or metadata key is added, and no authored document +changes meaning. What changes is that two controls stop contradicting what the runtime +already does with metadata that already parses. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx index ef99494e06..5218275c80 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx @@ -83,9 +83,11 @@ import type { MetadataSelection } from '../preview-registry'; // The runtime's own answer for an omitted `escalation` key. Imported so the // #6620 rows below compare the RENDERED control against the installed contract // instead of against a literal this file would then own a second copy of. +// `ApprovalNodeConfigSchema` and `FlowNodeSchema` serve the objectui#9277 rows +// the same way, for `config.lockRecord` and `boundaryConfig.interrupting`. // ⛔ The subpath is load bearing: `ApprovalEscalationSchema` is NOT on the // package root, where it reads `undefined` and any `.parse` on it throws. -import { ApprovalEscalationSchema } from '@objectstack/spec/automation'; +import { ApprovalEscalationSchema, ApprovalNodeConfigSchema, FlowNodeSchema } from '@objectstack/spec/automation'; /* ── The `meta/*` double (objectui#7307) ─────────────────────────────── * `FlowNodeInspector` renders `FlowReferenceField` for every reference-kind key @@ -382,6 +384,123 @@ describe('boolean: a declared defaultValue seeds the control (objectui#8451, arm 'the control carries its label and nothing else — no caption naming a default', ).toBe('SLA escalation'); }); + + /* ── objectui#9277: the other direction of the same ledger, twice ───────── + * `approval.config.lockRecord` and `boundary_event.boundaryConfig.interrupting` + * declared NOTHING while the installed spec materialises `true` for both. + * + * Since objectui#8451 that is not a missing claim — it is a WRONG one: a + * boolean control seeds its checked state from `defaultValue`, so an absent + * declaration draws an UNCHECKED box. The author read `Lock record` unchecked + * and believed the record stayed editable while the node was pending, and + * read `Interrupting` unchecked and believed the boundary event would leave + * its host activity running. The runtime does the opposite of both. + * + * ⭐ Every expectation below is DERIVED from the installed spec, never the + * literal `true`, for the reason the #6620 row above states: a literal + * restates the very claim that drifted and passes just as happily on the next + * upstream flip. Each derivation carries the same vacuity guard — a spec that + * stopped materialising the key would otherwise turn both sides into + * `undefined` and the comparison into a tautology. + * + * Three rows per field, and they are not interchangeable: the DECLARATION row + * pins the table against the contract, the SEEDING row pins the control the + * author actually sees (it reads the spec directly, so it reddens on its own + * when the declaration is dropped), and the STORED-FALSE row is the only one + * an always-checked control — an implementation strictly worse than the bug — + * fails. + * ──────────────────────────────────────────────────────────────── */ + + /** The `lockRecord` the installed spec applies to an approval config that omits it. */ + function specLockRecord(): boolean { + const parsed = ApprovalNodeConfigSchema.safeParse({ approvers: [{ type: 'user', value: 'u1' }] }); + expect(parsed.success, 'a minimal approval config must parse').toBe(true); + const value = (parsed.data as { lockRecord?: unknown } | undefined)?.lockRecord; + expect(typeof value, 'the spec still materialises `lockRecord` from an omitted key').toBe('boolean'); + return value as boolean; + } + + /** The `interrupting` the installed spec applies to a boundaryConfig that omits it. */ + function specInterrupting(): boolean { + const parsed = FlowNodeSchema.safeParse({ + id: 'n1', + type: 'boundary_event', + label: 'Node', + boundaryConfig: { attachedToNodeId: 'host', eventType: 'error' }, + }); + expect(parsed.success, 'a minimal boundary_event node must parse').toBe(true); + const value = (parsed.data as { boundaryConfig?: { interrupting?: unknown } } | undefined) + ?.boundaryConfig?.interrupting; + expect(typeof value, 'the spec still materialises `interrupting` from an omitted key').toBe('boolean'); + return value as boolean; + } + + /** A boundary_event node whose `boundaryConfig` carries only its required keys. */ + const boundaryDraft = (extra: Record = {}) => + draftWith('boundary_event', { boundaryConfig: { attachedToNodeId: 'host', eventType: 'error', ...extra } }); + + it('boolean: approval.lockRecord declares the default the spec applies (objectui#9277)', () => { + const field = fieldsForNodeType('approval').find((f) => f.id === 'lockRecord'); + expect(field?.kind, 'lockRecord is a boolean control').toBe('boolean'); + // Defaults are strings in this table — the spelling `controllerAdmits` compares. + expect( + field?.defaultValue, + 'the table declares the default the spec applies, and it used to declare nothing', + ).toBe(String(specLockRecord())); + }); + + it('boolean: approval.lockRecord draws a CHECKED box for an unset key (objectui#9277)', () => { + // The seeding half, and the ablation target: this row reads the SPEC, not + // the table, so dropping the declaration reddens it here on the rendered + // control rather than only on the row above. + const expected = specLockRecord(); + renderInspector(draftWith('approval', { config: {} })); + const box = checkbox('Lock record'); + expect(box, 'the Lock record control is on screen').not.toBeNull(); + expect( + box!.checked, + 'the RENDERED box carries the state the runtime applies — the defect was that it drew the opposite', + ).toBe(expected); + }); + + it('boolean: a stored `false` still beats lockRecord\'s declaration (objectui#9277)', () => { + // ⛔ Load bearing, and not interchangeable with the row above: an + // ALWAYS-CHECKED control satisfies "an absent key shows checked" and is + // refused only here. + renderInspector(draftWith('approval', { config: { lockRecord: false } })); + expect( + checkbox('Lock record')!.checked, + 'a deliberate false is the author\'s answer and outranks the declaration', + ).toBe(false); + }); + + it('boolean: boundaryConfig.interrupting declares the default the spec applies (objectui#9277)', () => { + const field = fieldsForNodeType('boundary_event').find((f) => f.id === 'boundaryConfig.interrupting'); + expect(field?.kind, 'interrupting is a boolean control').toBe('boolean'); + expect( + field?.defaultValue, + 'the table declares the default the spec applies, and it used to declare nothing', + ).toBe(String(specInterrupting())); + }); + + it('boolean: boundaryConfig.interrupting draws a CHECKED box for an unset key (objectui#9277)', () => { + const expected = specInterrupting(); + renderInspector(boundaryDraft()); + const box = checkbox('Interrupting'); + expect(box, 'the Interrupting control is on screen').not.toBeNull(); + expect( + box!.checked, + 'the RENDERED box says the host activity IS cancelled, which is what the runtime does', + ).toBe(expected); + }); + + it('boolean: a stored `false` still beats interrupting\'s declaration (objectui#9277)', () => { + renderInspector(boundaryDraft({ interrupting: false })); + expect( + checkbox('Interrupting')!.checked, + 'a non-interrupting boundary event is authorable, and the declaration must not overwrite it', + ).toBe(false); + }); }); describe('the online writer of defaultValue reaches the same repaired control', () => { @@ -588,9 +707,16 @@ describe('non-regression — a change that deletes the control must not pass thi describe('the declaration surface this card names', () => { /** - * The ten declaring fields, as `.`. Triage named this + * The twelve declaring fields, as `.`. Triage named this * list the acceptance surface, so it is pinned: a PR that retires the - * property, or that adds an eleventh declaration, moves this line. + * property, or that adds a thirteenth declaration, moves this line. + * + * ⚠️ The count is a LIVE base plus this card's additions, never a constant + * carried over from a card's own arithmetic: objectui#9277 added two + * (`approval.lockRecord`, `boundary_event.boundaryConfig.interrupting`) on + * top of the ten this line read on `origin/main`, and objectui#9278 was in + * flight against the same line with an eleventh of its own. Whoever lands + * next re-reads this number here before adding to it. * * Swept over the picker's node types plus the four that carry config but are * not offered in the picker (ADR-0031 import/export-only, and the legacy @@ -599,7 +725,7 @@ describe('the declaration surface this card names', () => { */ const OFF_PICKER_TYPES = ['boundary_event', 'parallel_gateway', 'join_gateway', 'legacy_action', 'notify']; - it('exactly ten fields declare a defaultValue, and these are they', () => { + it('exactly twelve fields declare a defaultValue, and these are they', () => { const swept = [...FLOW_NODE_TYPE_OPTIONS, ...OFF_PICKER_TYPES]; expect( FLOW_NODE_TYPE_OPTIONS.every((t) => swept.includes(t)), @@ -617,9 +743,11 @@ describe('the declaration surface this card names', () => { 'approval.escalation.enabled', 'approval.escalation.notifySubmitter', 'approval.behavior', + 'approval.lockRecord', 'approval.maxRevisions', 'approval.onEmptyApprovers', 'boundary_event.boundaryConfig.eventType', + 'boundary_event.boundaryConfig.interrupting', 'http_request.method', 'screen.mode', 'wait.waitEventConfig.eventType', @@ -660,7 +788,7 @@ describe('the declaration surface this card names', () => { expect( cases.map((c) => c.id).sort(), - 'the select-kind half of the declaration surface — seven of the ten', + 'the select-kind half of the declaration surface — seven of the twelve', ).toEqual([ 'approval.behavior', 'approval.escalation.action', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts index 9aec028b1a..64d676e203 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts @@ -763,7 +763,17 @@ const FLOW_NODE_CONFIG: Record = { placeholder: '1', help: 'Approvals required — total for quorum, per group for per_group. Clamped server-side so it can never deadlock.', }), + // `defaultValue` mirrors the spec's `.default(true)` (objectui#9277): an + // approval config that OMITS `lockRecord` parses as LOCKED, so a table + // declaring nothing drew the box UNCHECKED — the inspector told the author + // the record stayed editable while the node was pending, and the runtime + // locked it. Since objectui#8451 seeded the boolean control from this + // property, an absent declaration is not a missing claim here; it is the + // wrong one. Derived from the installed `ApprovalNodeConfigSchema`, never + // from taste — `FlowNodeInspector.declaredDefault.test.tsx` re-derives it + // from that schema rather than restating the literal. cfg('lockRecord', 'Lock record', 'boolean', { + defaultValue: 'true', help: 'Lock the triggering record from edits while this node is pending.', }), cfg('approvalStatusField', 'Status field', 'reference', { @@ -924,7 +934,16 @@ const FLOW_NODE_CONFIG: Record = { ], defaultValue: 'error', }), - at('boundaryConfig', 'interrupting', 'Interrupting', 'boolean', { help: 'Cancel the host activity when this event fires.' }), + // `defaultValue` mirrors the spec's `.default(true)` (objectui#9277): a + // `boundaryConfig` that OMITS `interrupting` parses as INTERRUPTING, so a + // table declaring nothing drew the box UNCHECKED and told the author the + // host activity would keep running — it is cancelled. Same shape as + // `lockRecord` above, derived from the installed `FlowNodeSchema` boundary + // block and re-derived in `FlowNodeInspector.declaredDefault.test.tsx`. + at('boundaryConfig', 'interrupting', 'Interrupting', 'boolean', { + defaultValue: 'true', + help: 'Cancel the host activity when this event fires.', + }), at('boundaryConfig', 'errorCode', 'Error code', 'text', { placeholder: 'TIMEOUT (empty = all)', showWhen: { field: 'boundaryConfig.eventType', equals: ['error'] }, From 7cae0d19c2219d5501880cf2bed4bec4016a45aa Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 04:40:55 +0000 Subject: [PATCH 2/2] docs(app-shell): say why the offline undeclared boolean is undeclared correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge commit's comment names `screen.waitForInput` as the offline undeclared boolean the old sentence claimed did not exist. Naming it without measuring it lays a trap: the next reader sees an undeclared boolean in a file whose whole subject is missing declarations, and declares one. Measured on the installed `@objectstack/spec` (17.4.0): `waitForInput` is `z.boolean().optional()` with no `.default(...)`. An omitted key materialises nothing, so under this file's own stated convention — a `defaultValue` mirrors what the spec applies to an omitted key — there is nothing for a declaration to mirror, and writing one would invent a default the runtime never applies. That is the opposite direction from `approval.config.lockRecord` and `boundary_event.boundaryConfig.interrupting`, where the spec does materialise `true` and the declaration was therefore missing. Comment only; no assertion moves. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ --- .../FlowNodeInspector.declaredDefault.test.tsx | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx index e1c700c1d8..6ccb7f0fc7 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx @@ -345,9 +345,17 @@ describe('boolean: a declared defaultValue seeds the control (objectui#8451, arm // declared the middle two — so the count it reported was the DECLARING // booleans mislabelled as the whole boolean surface. // ⇒ `screen.waitForInput` is the offline undeclared boolean that sentence - // says does not exist. An offline twin of this row is therefore writable - // today; it is deliberately NOT written here, because that is a new - // assertion rather than the merge this file is being changed for. + // says does not exist — and it is undeclared CORRECTLY, so ⛔ do not read + // this as an objectui#9277-class omission waiting to be declared. Measured + // on the installed `@objectstack/spec` (17.4.0): `waitForInput` is typed + // `z.boolean().optional()` with no `.default(...)`, so an omitted key + // materialises nothing and there is no spec answer for a declaration to + // mirror. Declaring one here would invent a default the runtime does not + // apply — the opposite of what objectui#9277 did for `lockRecord` and + // `interrupting`, where the spec DOES materialise `true`. + // An offline twin of this row is therefore writable today; it is + // deliberately NOT written here, because that is a new assertion rather + // than the merge this file is being changed for. stubs.configSchemas = { approval: { type: 'object',