Repository navigation
Commit 28ff8a9
fix(plugin-approvals): every step of a multi-step approval names the run's submitter, not the previous step's approver (#24860)
Fixes #24817
Clause-②: no
## What was wrong
Since an approval decision resumes its flow as the person who decided
(`ResumeSignal.resumedBy`; the resume-as-decider change of PR #24527,
still unreleased), the segment after a decision runs with
`context.userId` set to the DECIDER. `approval-node.ts` opened every
approval step with `submitterId: context?.userId ?? null`, so every step
after the first recorded the previous step's decider as its
`submitter_id`. On every later step:
- the "I Submitted" filter (`listRequests({ submitterId })`) listed the
step under the previous approver;
- recall and resubmit, which are submitter-only, admitted the previous
approver and refused the real submitter;
- every notice meant for the submitter (`approval.returned`,
`approval.request_info`, `approval.sla_breached`, `approval.approved`,
`approval.rejected`) went to the previous approver.
The governing text is `ApprovalEscalationSchema.notifySubmitter` in
`packages/spec/src/automation/approval.zod.ts:742`:
> `notifySubmitter: z.boolean().default(true).describe('Notify the
original submitter on escalation')`
Shipped flows on this path: the showcase's `showcase_budget_approval`
(`manager_review`, then `exec_review` above 500000) and
`showcase_dynamic_approval` (`lead_review`, then `co_sign`).
## The rule
Every step of a run records the run's ORIGINAL submitter, the user who
triggered the run, as `submitter_id`, whoever resumed the run onto that
step. The decider stays the actor of the resumed segment: the ruling
behind the resume-as-decider change is not reopened, and nothing here
changes the context a segment runs with.
## Where the original submitter comes from (mechanism assumption H2,
measured)
The engine already owns "who started this run".
`AutomationEngine.resolveDecisionSegmentContext`
(`packages/services/service-automation/src/engine.ts:6623`) records
`triggerUserId` on the switched context at the first decision switch,
keeps it on every later switch, and persists it through each later
pause; `null` means the trigger resolved no user, and an absent key
means no switch happened, so `userId` is still the trigger user. The
engine's own reader is `runTriggerUserId` (`runtime-identity.ts:188`),
which `buildRunTrigger` uses for the run history. That context is the
one every node executor receives (`executor.execute(node, variables,
context)`, `engine.ts:11958`).
The two other candidates were not needed: the engine seeds no flow
variable for the trigger user (`seedRunVariables` sets `$record`,
`record`, `previous`, `$runId`, `$flowName` and `$flowLabel` only), and
a lookup of step 1's request row would be a sibling read where the
engine already answers.
Mechanism assumption H1 held: `approval-node.ts:165` read
`context?.userId` at `41646a5788`.
## What changes
- **`packages/plugins/plugin-approvals/src/approval-node.ts`** — the
producer. A new local `runTriggerUser(context)` gives the engine's
reading: `triggerUserId` when the engine recorded it (`null` stays
`null`), else `userId`. It is mirrored here, not imported, because this
plugin deliberately does not depend on `service-automation` (the file's
`nestVariables` is the precedent). The node opens each step with it. The
execution context handed to `openNodeRequest` keeps `userId:
context?.userId`, so the decider still acts there.
- **`packages/plugins/plugin-approvals/src/approval-service.ts`,
`openNodeRequest` only** (not `remind()`): an explicit `submitterId:
null` now means no submitter. Before, `null` fell back to
`context.userId` like an omitted value. On a later step of a run whose
trigger resolved no user, that fallback named the decider as submitter
even with the node fixed. An omitted `submitterId` still falls back.
This file is outside the claim's declared surface; it is declared here.
The hunks (~:3453, ~:3740, ~:3773) are disjoint from `remind()` and from
the ranges other open work edits.
- **`.changeset/24817-later-step-original-submitter.md`** — `patch` for
`@objectstack/plugin-approvals`. It says this corrects the unreleased
later-step submitter that PR #24527 introduced, and that compared with
the last release nothing moves.
What follows `submitter_id` by design moves with it on later steps, back
to its released-18.0 value: the step's `submit` timeline row
(`actor_id`), and the attribution of the `pending` status mirror written
when the step opens. The latter is attributed to whoever the row calls
the submitter (`approval-service.ts`, the comment above
`mirrorStatusField`). The flow's own nodes after a decision still run as
the decider.
## Tests
New file
`packages/plugins/plugin-approvals/src/later-step-original-submitter.integration.test.ts`.
It uses a real `ObjectQL` over `SqlDriver` (better-sqlite3, in-memory,
real DDL), a real `AutomationEngine` with the built-in logic nodes, a
real `ApprovalService`, and the real approval and revise nodes, built
the way `approval-decision-outcome-notification.integration.test.ts`
builds its stack. Eight cases:
1. `deal_chain` (`runAs` default): steps 1, 2 and 3 each have
`submitter_id` = the trigger user, and so does each step's `submit` row.
Steps 2 and 3 open after decisions by two different people. The "I
Submitted" filter finds all three steps under the trigger user and none
under either decider.
2. `deal_chain_system` (`runAs: 'system'`): the same.
3. A gateway-skipped step: the decision node routes around `exec`, and
`cfo` still names the trigger user.
4. A run whose trigger resolved no user: step 2 has no submitter, never
the decider.
5. Recall on step 2: the step-1 decider is refused with code `FORBIDDEN`
and the request stays `pending`. Then the original submitter is admitted
and the request is `recalled`.
6. Resubmit on step 2, after a send-back from step 2: the send-back's
`approval.returned` reaches the original submitter. The step-1 decider's
resubmit is refused with `FORBIDDEN`, the request stays `returned` and
no round opens. Then the original submitter is admitted, and round 2
opens on the same run naming them.
7. The decision that ends a two-step run sends `approval.approved` to
the original submitter, and step 1 sends nothing (the notice from PR
#24816).
8. Control: a one-step run names its trigger user, refuses another
user's recall and admits the submitter's.
Refusals are asserted on their service-seam ADR-0112 form, the
`FORBIDDEN:` code prefix that `@objectstack/rest`'s
`handleApprovalError` maps to `code: 'FORBIDDEN'` and status 403. That
mapping is pinned live in
`packages/rest/src/rest-approvals-wire-codes.test.ts`. Each refusal has
its admitted control on the same request.
### Runs (all under `scripts/pm/os-verify-lock.sh`)
- **Reproduction, before the fix, at `e3ac5acecd`**: `pnpm --filter
@objectstack/plugin-approvals exec vitest run --maxWorkers=2
src/later-step-original-submitter.integration.test.ts` gave `Tests 7
failed | 1 passed (8)`. Step 2 read `expected 'u_mgr' to be 'u_sub'`,
and the recall case read `expected undefined to be 'FORBIDDEN'`: the
step-1 decider's recall was admitted. The one-step control passed. The
user-less case failed on a test-helper default-parameter slip, which was
corrected at the next commit; its red under the old code is shown by
ablation legs B and C below.
- **Final head `bc2d3afd94`** (after merging `origin/main`
`b3fc122f07`):
- `pnpm --filter '@objectstack/plugin-approvals^...' build`: VERDICT
command-exit 0.
- `pnpm --filter @objectstack/plugin-approvals exec vitest run
--maxWorkers=2`: `Test Files 89 passed (89)`, `Tests 1172 passed
(1172)`.
- `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0,
including `check:test-typecheck: OK … 8 file(s) / 324 error(s) / 27
pinned signature(s) held`. The new file adds no debt.
- The new file and
`approval-decision-outcome-notification.integration.test.ts`, verbose:
`Tests 16 passed (16)`.
### Ablation (one-time proof; nothing left in the tree)
Each leg was mutated through `node scripts/ablation-replace.mjs`, so the
anchor had to hit once and the write was verified on disk. Each leg ran
the new file and was restored with `git checkout HEAD -- PATH`. The
restore was proven by blob equality with `HEAD:PATH` and an empty `git
diff HEAD`. The tests import `./approval-node.js` and
`./approval-service.js` from source, so no `dist/` is involved. All
three legs ran at `bbbbbe4590`.
| leg | mutation | result | restore |
|:--|:--|:--|:--|
| A | `approval-node.ts`: `submitterId: runTriggerUser(context),` →
`submitterId: context?.userId ?? null,` (blob `e972e15b` → `24a2845b`) |
`7 failed / 1 passed`: every later-step case red, the one-step control
green | blob `e972e15b` == HEAD, `git diff HEAD` empty |
| B | `approval-service.ts`: the explicit-null line → `input.submitterId
?? context.userId ?? null` (blob `02fd843f` → `d282e59d`) | `1 failed /
7 passed`: only the user-less case red | blob `02fd843f` == HEAD, `git
diff HEAD` empty |
| C | `approval-node.ts`: a recorded `null` trigger read as
`triggerUserId ?? userId` (blob `e972e15b` → `916125e4`) | `1 failed / 7
passed`: only the user-less case red | blob `e972e15b` == HEAD, `git
diff HEAD` empty |
Leg C is why the reader is not `triggerUserId ?? userId`: under that
reading, a run nobody triggered names its first decider as submitter.
## Gates
Gates are derived from this diff by `node scripts/pm/dispatch-gates.mjs
--commands --repo objectstack-ai/objectstack` at `bc2d3afd94`: 70
commands. The local run and the `--ran` reconciliation are recorded in
the `os-dev-report` comment on the card. CI's check runs on this PR are
the record for every family CI runs.
## Route chosen, read on the four axes
The option taken is to mirror the engine's own `triggerUserId` reading
inside the approval node. The options not taken: declare `triggerUserId`
on spec's `AutomationContext` (that is `packages/spec`, a hard stop for
this card), give `plugin-approvals` a runtime dependency on
`service-automation`, or look up step 1's request on the same
`flow_run_id`.
- **Real business need.** Measured: the two showcase multi-step flows,
and every multi-step approval after the resume-as-decider change.
- **Long-term soundness.** The source is the one the engine owns and
already persists, and no stored key or column is added. The mirror is
one function documented against `runTriggerUserId`. If the engine
renames or reshapes the key, the real-engine pins turn red.
- **Making AI mistakes structurally harder.** No authoring surface
moves. The one tempting misreading (`??`) is named in the code and
pinned by leg C.
- **Startup-stage focus.** No new capability, key, gate or dependency.
## Acceptance notes
- `triggerUserId` is declared on `service-automation`'s internal
`SeededRunContext`, and spec's `AutomationContext` does not declare it,
although every node executor receives it. A plugin that wants "who
started this run" therefore mirrors the engine's reader, as this one now
does. Not filed: no second consumer exists. Carrier: none.
- The 403 half of the refusal envelope is asserted at the REST door, not
in this package: `@objectstack/rest` is not a dependency of
`plugin-approvals`, and the unaliased-import ledger in
`scripts/check-test-source-alias.mjs` is shrink-only.
- `origin/main` moved to `abd5acdaf7` (plugin-email and service-sms
test-send recipients) after the merge. It touches none of this diff's
packages.
Related: PR #24527 (the resume-as-decider change this corrects; its
changeset `.changeset/24321-resume-as-decider.md` is still pending) and
#24321 (its ruling, not reopened here).
---
_Generated by [Claude
Code](https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 06cab48 commit 28ff8a9
4 files changed
Lines changed: 438 additions & 3 deletions
File tree
- .changeset
- packages/plugins/plugin-approvals/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
83 | 83 | | |
84 | 84 | | |
85 | 85 | | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
86 | 114 | | |
87 | 115 | | |
88 | 116 | | |
| |||
162 | 190 | | |
163 | 191 | | |
164 | 192 | | |
165 | | - | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
166 | 197 | | |
167 | 198 | | |
168 | 199 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3445 | 3445 | | |
3446 | 3446 | | |
3447 | 3447 | | |
| 3448 | + | |
| 3449 | + | |
| 3450 | + | |
| 3451 | + | |
| 3452 | + | |
| 3453 | + | |
| 3454 | + | |
3448 | 3455 | | |
3449 | 3456 | | |
3450 | 3457 | | |
| |||
3725 | 3732 | | |
3726 | 3733 | | |
3727 | 3734 | | |
| 3735 | + | |
3728 | 3736 | | |
3729 | 3737 | | |
3730 | 3738 | | |
3731 | 3739 | | |
3732 | 3740 | | |
3733 | | - | |
| 3741 | + | |
3734 | 3742 | | |
3735 | 3743 | | |
3736 | 3744 | | |
| |||
3757 | 3765 | | |
3758 | 3766 | | |
3759 | 3767 | | |
3760 | | - | |
| 3768 | + | |
3761 | 3769 | | |
3762 | 3770 | | |
3763 | 3771 | | |
| |||
0 commit comments