Skip to content

Commit bfc1539

Browse files
fix(plugin-approvals): the status mirror writes only a value the field declares (#24656)
Fixes #24577 Clause-②: no ## What changes The approval status mirror (`ApprovalService.mirrorStatusField`, `packages/plugins/plugin-approvals/src/approval-service.ts`) now writes only a value the status field declares. The rule, in one sentence (the changeset carries the same sentence): > When `approvalStatusField` is an option field (a `select`) whose options do not declare the status, a recall or the dead-run sweep writes the value the field held when the flow started (the request's submit snapshot), else the field's declared default, else empty, and every other transition writes nothing and logs a `warn` naming the object, field and value. - A status the field declares is written as before. A field with no declared value set (text, any type that is not an option type, an option type authored without options) gets every status verbatim, as before. - The declared options are read from `engine.getSchema(object)`, which answers `registry.getObject(object)`: the same object the engine's update path validates the write against. The option types are the spec's `SINGLE_OPTION_TYPES` / `MULTI_OPTION_TYPES`; the option codes are read the way the engine's record validator reads them. - The write's context is unchanged (#24519's `attributedMirrorContext`, or `SYSTEM_CTX` for the sweep). No `packages/spec` edit, no new stored column, no new spec key. ## Measured on a real stack (A1, A2) Real `ObjectKernel`, real ObjectQL engine and record validator, real record-change trigger and automation engine, real approval node and record-lock hook, driver-sql on better-sqlite3. Each door driven through `ApprovalService`. Three status fields: a readonly select with hotcrm's four options and default (`crm_opportunity.approval_status` in shape), an editable select with the same options, and a text field. | door | readonly select, `main` at 35450f0 | readonly select, this PR | editable select, `main` | editable select, this PR | text field (both) | |---|---|---|---|---|---| | submit | `pending` | `pending` | `pending` | `pending` | `pending` | | approve | `approved` | `approved` | `approved` | `approved` | `approved` | | reject | `rejected` | `rejected` | `rejected` | `rejected` | `rejected` | | recall | `recalled` (undeclared, stored) | `not_required` (its value at submit) | write refused by the validator: stays `pending`, warn | empty (its value at submit) | `recalled` | | send-back | `returned` (undeclared, stored) | `pending` (not written, warn) | refused: stays `pending`, warn | `pending` (not written, warn) | `returned` | | resubmit | `pending` | `pending` | `pending` | `pending` | `pending` | | dead-run sweep | `recalled` | `not_required` | refused: stays `pending`, warn | empty | `recalled` | Why `recalled` landed: the engine judges a `readonly` field's value for shape only (record validator, update walk), and the spec advises a readonly status field. On an editable select the engine refuses the undeclared value, and the mirror's `catch` logged it at `warn` and left the field reading `pending` with no request pending: the same defect class, also closed here. A2, the value the flow started from: `sys_approval_request.payload_json` already holds it. It is the snapshot of the run's `$record`, which the automation engine seeds once at run start (`seedRunVariables`), so a second approval node's request carries the same trigger record. No new column is needed. The card's case on the stack, with hotcrm's submit gate bound (`record-after-update`, admits a large deal at `not_required` or empty): on `main`, the owner raises the amount, a request opens; the recall leaves `recalled`; the owner's next save answers 200 and opens no request. With this PR the recall restores `not_required` and the deal re-enters approval. ## Deviations from the dispatch's suggested route **1. Send-back keeps the field (no write, `warn`) instead of restoring the submit value.** Measured with the suggested route: a one-off run with send-back passing the snapshot (a mutation through `scripts/ablation-replace.mjs`, restored to HEAD after, blob hash checked), hotcrm's gate bound, a revise-capable flow: - the send-back's own restore write re-admitted the gate, which opened a second request on the same record at once (`returned`, `pending`); - the record re-locked, so the submitter's revision was refused (`RECORD_LOCKED`); - the submitter's `resubmit` was refused `DUPLICATE_REQUEST`. With send-back keeping `pending` (this PR, pinned): the revision lands, no second request opens, and `resubmit` opens round 2. Four axes: - Real business need: measured above. hotcrm's flow has no `revise` edge today, so hotcrm itself is not affected either way; any app with a revise loop and a gate on the mirrored field is. - Long-term soundness: ADR-0044 (accepted) keeps the run alive in the revise window, and its Consequences tell authors to gate start conditions on the mirrored approval-status field. A restore there contradicts the ADR. `pending` is the true reading in a vocabulary without `returned`: the approval has not concluded. - Preventing AI mistakes: one rule. Only a transition that ends the request restores, and no transition ever writes an undeclared value. The `warn` names the remedy (declare the option). - Startup scope: no new key, column or gate. **2. The fallback after the snapshot is the field's declared default, then empty** (the suggested route said empty). The suggested route's own reason for empty is "what a never-submitted record carries". A field with a declared default carries that default from insert (hotcrm's `not_required`). It is a value the field declares, which triage allows. Reached only when the snapshot lacks the field or holds an undeclared value, such as a deal left at `recalled` by the old mirror. ## A consequence to know: a gate that admits the restored value re-enters at the recall itself The restore is itself an update of the record. A `record-after-update` gate that admits the restored value without comparing `previous` re-admits the record on that write. On the hotcrm-shaped fixture, the recall opens a new request at once and the record re-locks, so the owner cannot edit between the recall and the new request. This follows from every value triage allows: any value the gate admits re-admits it at once, and a value it does not admit never re-enters, which is the card's defect. The app's own start condition decides, as triage intended. An app that wants recall-then-edit can compare `previous` in its gate (for example, admit only when `previous.approval_status` is also at the entry value). This is for the hotcrm thread, not this repo. Measured on the fixture, not on hotcrm itself. ## Tests (at `cb6360bf1`, the final head, after merging `origin/main` `14ccb1da9`) - New `packages/plugins/plugin-approvals/src/status-mirror-declared-values.integration.test.ts`, seven pins on the real stack: - (a) recall on the hotcrm-shaped select: the field reads `not_required`, the owner's edit lands, and a new submit opens a new request; - (a) with hotcrm's gate bound: a recalled deal re-enters approval, and every value written is declared (`pending`, `not_required`, `pending`); - (b) send-back: the field keeps `pending`, the `warn` names `crm_deal` / `approval_status` / `returned`, the revision lands with no parallel request, and resubmit opens round 2; - (d) the dead-run sweep restores `not_required`; - a held value the select does not declare falls back to the declared default, then to empty, on an editable select; - a decision the select does not declare (`approved`) is not written, the skip is logged, and no undeclared write is attempted; - (c) a text field keeps `recalled` / `returned` verbatim. - `pnpm --filter @objectstack/plugin-approvals test`: 84 files, 1139 tests passed. `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0, test layer OK against `test-typecheck-debt.json`. Both ran on `cb6360bf1`. - Existing pins on a text status field (`status-mirror-cascade`, `status-mirror-envelope`, `approval-revise` "status mirror follows the rounds") pass unchanged. They are case (c). **Ablation** (committed first, through `scripts/ablation-replace.mjs`). `if (!declared || declared.includes(status)) return status;` was replaced with `return status;`, which is the verbatim write. 6 of 7 new pins went red: - recall: `expected 'recalled' to be 'not_required'`; - the gate pin timed out: the deal never re-entered, which is the card's symptom; - send-back: `expected 'returned' to be 'pending'`; - the sweep: `recalled`; - the fallback: `recalled`; - the decision pin: `['pending','approved']` was attempted. The text-field pin stayed green, as it should. Restore: blob `b417ef74320e` equals HEAD's and `git diff HEAD` is empty. The subject is imported from source (`./approval-service.js`), so no build sits between the mutation and the run. **Local gates** were derived by `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` at `cb6360bf1`: 69 commands. Each was run with its exit code captured before any pipe. 67 exited 0. 2 exited 3 (prerequisite not met), so they are NOT MEASURED: - `pnpm check:dual-build-cjs-loads` needs every package's `dist/`; - `pnpm check:i18n` needs the CLI's build closure. This diff touches no object metadata, label or translation. Both are declared to CI. `dispatch-gates --ran` reads: 69 derived, 67 run, 2 NOT-MEASURED, 0 UNRUN. ## Acceptance notes - The spec's TSDoc on `ApprovalNodeConfigSchema.approvalStatusField` names `pending`/`approved`/`rejected`/`recalled`. It omits `returned` (written since ADR-0044) and does not say that a select receives only its declared values. `skills/objectstack-automation/references/state-machines-and-approvals.md` carries the same four-value list, and `content/docs/automation/approvals.mdx` does not state the declared-values rule. Both are outside this card's surface (no `packages/spec/**`). Noted, not filed: doc drift, no carrier. - `test:repo` and dogfood were not run locally. Both are declared to CI. --- _Generated by [Claude Code](https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 31e0735 commit bfc1539

3 files changed

Lines changed: 447 additions & 3 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): the approval status mirror writes only a value the field declares, so a recalled record whose status field is a select goes back to where it stood before it entered approval
6+
7+
Clause-②: no
8+
9+
**The rule:** when `approvalStatusField` is an option field (a `select`) whose options do not declare the status, a recall or the dead-run sweep writes the value the field held when the flow started (the request's submit snapshot), else the field's declared default, else empty, and every other transition writes nothing and logs a `warn` naming the object, field and value.
10+
11+
- **Before.** Each status was written verbatim. The engine checks a `readonly` field's value for shape only, and the spec advises a readonly status field, so a select that did not declare `recalled` ended up holding it after a recall. An app whose submit gate admits only the select's own values (for example `not_required` or empty) then never re-entered approval. On an editable select the engine refused the undeclared value, and the field kept reading `pending` with no request pending.
12+
- **Now.** A recalled record, or one released by the dead-run sweep, reads its submit-time value again, so the app's own start condition decides whether it re-enters approval. A gate that admits that value re-admits the record at once, because the restore is itself an update.
13+
- **Send-back keeps the field.** A select that does not declare `returned` keeps reading `pending` through the revise window. The run is still alive there (ADR-0044), and a restored pre-approval value would re-admit the start condition and open a second, parallel approval of the same record.
14+
- **Unchanged.** A status the select declares is written as before. A field with no declared value set (text, or any type that is not an option type) gets every status verbatim, as before. The write's context is unchanged.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 100 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,9 @@ import type {
5858
// fields the caller had already supplied.
5959
import type { ExecutionContext } from '@objectstack/spec/kernel';
6060
import { RESUME_AUTHORITY_SERVICE } from '@objectstack/spec/contracts';
61-
import { canServeApiOperation, isFileIdToken, referenceTargetOf } from '@objectstack/spec/data';
61+
import {
62+
canServeApiOperation, isFileIdToken, referenceTargetOf, SINGLE_OPTION_TYPES, MULTI_OPTION_TYPES,
63+
} from '@objectstack/spec/data';
6264
import type { EnableLike, ObjectCapabilitiesParsed } from '@objectstack/spec/data';
6365
// [#11993] The SANCTIONED renderer for OPERATION-level refusal copy. The
6466
// Operation Message Catalog is the ONE seat for these sentences — its own
@@ -1150,6 +1152,31 @@ function parseJson<T = any>(raw: unknown, fallback: T): T {
11501152
return raw as T;
11511153
}
11521154

1155+
/** [#24577] What {@link ApprovalService.mirroredStatusValue} answers when a transition writes nothing. */
1156+
const STATUS_NOT_MIRRORED = Symbol('status-not-mirrored');
1157+
1158+
/**
1159+
* [#24577] The values a status field declares, read as the engine's record
1160+
* validator reads a select's options (objectql `record-validator.ts`,
1161+
* `optionValues`): the option codes of an option-typed field. `undefined` when
1162+
* the field declares no value set — text, any type that is not an option type,
1163+
* an option type authored without options (free-form `tags`), a field the
1164+
* registry does not hold — and then every status is written as it is.
1165+
*/
1166+
function declaredOptionValues(def: unknown): string[] | undefined {
1167+
if (!def || typeof def !== 'object') return undefined;
1168+
const { type, options } = def as { type?: unknown; options?: unknown };
1169+
if (typeof type !== 'string' || (!SINGLE_OPTION_TYPES.has(type) && !MULTI_OPTION_TYPES.has(type))) return undefined;
1170+
if (!Array.isArray(options) || options.length === 0) return undefined;
1171+
return options.map((o) => (typeof o === 'object' && o !== null ? String((o as { value?: unknown }).value) : String(o)));
1172+
}
1173+
1174+
/** [#24577] A held value (one code, or a multi-value field's codes) every part of which the field declares. */
1175+
function holdsDeclaredValues(value: unknown, declared: readonly string[]): boolean {
1176+
const values = Array.isArray(value) ? value : [value];
1177+
return values.length > 0 && values.every((v) => v != null && declared.includes(String(v)));
1178+
}
1179+
11531180
function csvSplit(raw: unknown): string[] {
11541181
if (!raw) return [];
11551182
if (Array.isArray(raw)) return raw.map(String).filter(Boolean);
@@ -3227,6 +3254,10 @@ export class ApprovalService implements IApprovalService {
32273254
* construction (#10101 stamps the request with the subject record's
32283255
* organisation), except for a record re-parented while its approval was
32293256
* pending, which the stamp would scope this write away from.
3257+
*
3258+
* **The value written is one the field declares (#24577).** What is written
3259+
* is {@link mirroredStatusValue}'s answer, and `abandoned` (the request's
3260+
* row, on a transition that abandons it) is what that answer may restore.
32303261
*/
32313262
private async mirrorStatusField(
32323263
object: string,
@@ -3235,17 +3266,78 @@ export class ApprovalService implements IApprovalService {
32353266
status: string,
32363267
actorId: string | null,
32373268
requestOrg: string | null | undefined,
3269+
abandoned?: { payloadJson: unknown },
32383270
): Promise<void> {
32393271
try {
3272+
const value = this.mirroredStatusValue(object, field, status, abandoned);
3273+
if (value === STATUS_NOT_MIRRORED) {
3274+
this.logger?.warn?.(
3275+
`[approvals] approval status '${status}' was not written to ${object}.${field}: the field's options do not `
3276+
+ `declare it, so the field keeps its value. Declare '${status}' among the field's options to mirror this transition.`,
3277+
{ object, record: recordId, field, value: status },
3278+
);
3279+
return;
3280+
}
32403281
const context = actorId
32413282
? await this.attributedMirrorContext(object, recordId, actorId, requestOrg)
32423283
: SYSTEM_CTX;
3243-
await this.engine.update(object, { id: recordId, [field]: status }, { context });
3284+
await this.engine.update(object, { id: recordId, [field]: value }, { context });
32443285
} catch (err: any) {
32453286
this.logger?.warn?.(`[approvals] mirrorStatusField failed: ${err?.message ?? err}`);
32463287
}
32473288
}
32483289

3290+
/**
3291+
* [#24577] The value a status mirror writes onto `object.field` for
3292+
* `status`, or {@link STATUS_NOT_MIRRORED} when it writes nothing.
3293+
*
3294+
* A field's declared options are its value set (the triage ruling on the
3295+
* card): the producer never writes outside them. The engine's validator does
3296+
* not hold that line for this write — it judges a `readonly` field's value
3297+
* for shape only, and the mirrored field is readonly by the spec's own advice
3298+
* — so a select that never declared `recalled` came to hold it, and an app
3299+
* whose submit gate admits only the select's own values never re-entered
3300+
* approval. The options are read from the registry the engine validates the
3301+
* same write against (`getSchema` answers `registry.getObject`).
3302+
*
3303+
* - A field with no declared value set ({@link declaredOptionValues}) and a
3304+
* status the field declares: the status, as before.
3305+
* - A transition that ABANDONS the request (a recall, the dead-run sweep),
3306+
* `abandoned` set: the record goes back to where it stood before it
3307+
* entered approval. That is the value the field held in the request's
3308+
* submit snapshot (`payload_json`, the trigger record the flow started
3309+
* from) when that is a declared value or none; else what a never-submitted
3310+
* record carries — the field's declared default, or empty. Whether the
3311+
* record re-enters approval is then the app's own gate's decision.
3312+
* - Every other transition (open, decide, send-back, the revision-limit
3313+
* auto-reject): nothing. Send-back is here, not with recall, because its
3314+
* run is still alive in the revise window (ADR-0044): a pre-approval value
3315+
* restored there re-admits the start condition ADR-0044 tells authors to
3316+
* gate on this field, and opens a parallel approval of the same record.
3317+
*/
3318+
private mirroredStatusValue(
3319+
object: string,
3320+
field: string,
3321+
status: string,
3322+
abandoned: { payloadJson: unknown } | undefined,
3323+
): unknown {
3324+
let def: { defaultValue?: unknown } | undefined;
3325+
try {
3326+
const schema = this.engine.getSchema?.(object) as { fields?: Record<string, unknown> } | undefined;
3327+
def = schema?.fields?.[field] as typeof def;
3328+
} catch { /* schema unavailable — no declared value set to hold the write to */ }
3329+
const declared = declaredOptionValues(def);
3330+
if (!declared || declared.includes(status)) return status;
3331+
if (!abandoned) return STATUS_NOT_MIRRORED;
3332+
const snapshot = parseJson<unknown>(abandoned.payloadJson, undefined);
3333+
if (snapshot && typeof snapshot === 'object' && Object.prototype.hasOwnProperty.call(snapshot, field)) {
3334+
const held = (snapshot as Record<string, unknown>)[field];
3335+
if (held == null) return null;
3336+
if (holdsDeclaredValues(held, declared)) return held;
3337+
}
3338+
return holdsDeclaredValues(def?.defaultValue, declared) ? def!.defaultValue : null;
3339+
}
3340+
32493341
/**
32503342
* The context a named actor's status mirror writes under (see
32513343
* {@link mirrorStatusField}): `{ isSystem: true, userId, tenantId }` and
@@ -4554,9 +4646,11 @@ export class ApprovalService implements IApprovalService {
45544646
}, { context: SYSTEM_CTX });
45554647
await this.syncApproverIndex(requestId, [], org, now);
45564648
if (config.approvalStatusField) {
4649+
// [#24577] A recall abandons the request: a field whose options do not
4650+
// declare `recalled` goes back to its value at submit.
45574651
await this.mirrorStatusField(
45584652
raw.object_name, raw.record_id, config.approvalStatusField, 'recalled',
4559-
actingUserId(context), org,
4653+
actingUserId(context), org, { payloadJson: raw.payload_json },
45604654
);
45614655
}
45624656

@@ -6609,8 +6703,11 @@ export class ApprovalService implements IApprovalService {
66096703
if (config.approvalStatusField) {
66106704
// No human did this — a sweep did. Left user-less on purpose (#3783): a
66116705
// flow that wants to react to a dead-run release declares runAs:'system'.
6706+
// [#24577] It abandons the request, as a recall does: a field whose
6707+
// options do not declare `recalled` goes back to its value at submit.
66126708
await this.mirrorStatusField(
66136709
raw.object_name, raw.record_id, config.approvalStatusField, 'recalled', null, org,
6710+
{ payloadJson: raw.payload_json },
66146711
);
66156712
}
66166713

0 commit comments

Comments
 (0)