Repository navigation
Commit 7282a95
fix(plugin-approvals): the decision that ends an approval tells its submitter the outcome (#24816)
Fixes #24753
Clause-②: yes
## What this changes
Approving or rejecting a request notified nobody. A send-back
(`approval.returned`) and a question (`approval.request_info`) reached
the submitter; the decision did not. Now `ApprovalService.decide()`
tells the submitter the outcome once the decision ends the approval
process:
- **Topics:** `approval.approved` ("Request approved") and
`approval.rejected` ("Request rejected"), one per outcome.
- **Shape:** the same as the sibling notices. It goes to the request's
own `submitter_id`. The record is named by `nameSubjectRecord`, the
label-and-id rule from #23556 (`Your Deal D_123 was rejected.`). The
body is never empty, the rule from #21847. When the decider wrote a
comment, it follows the sentence as its own paragraph. `notify()`
deep-links the notice to `/system/approvals?request=ID`. The actor is
the person the decision's context vouches for. The organization is the
request's.
- **Who is not told:**
- a submitter who decided their own request;
- anyone, on a co-sign vote that leaves the request pending;
- anyone, when the resume opened a further approval step on the same
run;
- anyone, for the SLA sweep's `auto_approve` / `auto_reject`. The
escalation's own submitter notice (`approval.sla_breached`, which names
the action) remains the one message, so `notifySubmitter: false` still
silences it. The sweep now calls a private `decideAndResume(...,
'escalation')`. The public `decide()` delegates to the same method with
`'outcome'`.
- **Docs:** a new callout in `content/docs/automation/approvals.mdx`.
The old advice to "keep notify nodes that report the decision" is
narrowed to recipients other than the submitter.
- **Changeset:** `@objectstack/plugin-approvals` `minor`, `Clause-②: yes
(widening)`. *(Seat edit: the declaration was re-made by the seat, claim
amendment 6286428768, to match this lane's precedent #22607, which
shipped the new `approval.requested` topic as `yes (widening)` and
`minor`. The dev's patch round at `0110fecb03` moved the changeset; this
line and line 3 are the seat's.)*
## Readings taken before writing it (origin/main a43405d)
- **H1 holds.** The send-back paths send `approval.returned` (in
`sendBack`, both the revision-limit auto-reject and the plain
send-back), `requestInfo` sends `approval.request_info`, and
`escalateRequest` notifies the submitter under `esc.notifySubmitter !==
false`. All of them address `raw.submitter_id` and use the closed `{
title, body, actionUrl }` payload. No approval `notify()` call site
reads the caller's locale; every body is one English sentence.
- **H2, refined.** The status write for approve and reject is shared, in
`decideNode`'s finalizing branch. The notice cannot be sent there,
because only the resume in `decide()` shows whether the run moved on to
another approval step. So the notice sits in `decide()`, after the
resume. This also means a decision whose resume strands sends no notice,
the same as a send-back in that state.
- **No registry is needed.** `service-messaging` has no topic registry.
`PreferenceResolver` matches the exact topic or `*` (default on,
fail-open), so the new topics are picked up with no wiring, and a user
can mute one outcome and keep the other. objectui (local checkout
90518cd) groups inbox arrivals by `type` generically and holds no
approval topic list.
## Tests
Measured at head 2a13930.
- New `approval-decision-outcome-notification.integration.test.ts`, 8
cases. It uses the real `ApprovalService` on a real `ObjectQL` over
`SqlDriver` (better-sqlite3 in memory), with the real `AutomationEngine`
running the flows and the real approval node opening each step. The
cases:
- approved;
- rejected with a comment;
- self-decided, with a control;
- a unanimous co-sign;
- a two-step ladder;
- a rejection at step one;
- a decision gateway that skips the second step;
- an SLA auto-approve under `notifySubmitter` true and false, with a
human-decision control.
- `approval-notification-body.integration.test.ts`: the battery drives
the new call site, so its "every call site" claim still holds (now
thirteen of fourteen). The body, record-name and organization pins cover
both new topics.
- `pnpm --filter @objectstack/plugin-approvals test`: 87 files, 1155
tests passed. `typecheck`: exit 0, and `check:test-typecheck` is OK with
the new file unledgered and error-free.
- **Ablations.** Each leg used `scripts/ablation-replace.mjs`, which
checks the anchor (1 to 0 hits), checks that the blob changed, and
restores with `git checkout HEAD -- ABS_PATH`. Each restore was proven:
blob equals HEAD, and `git diff HEAD` is empty.
- Dropping the `notifyDecisionOutcome` call: 11 tests red across both
files, including every negative case through its positive control.
- Removing the self-decider guard: only the self-decided test is red.
- Removing the next-step guard: only the two-step test is red.
- Passing `'outcome'` from the SLA sweep: only the SLA test is red.
## Gates (local subset)
All of these were run at head 2a13930. Each exit code was captured
before any pipe.
- **Derived list.** `node scripts/pm/dispatch-gates.mjs --commands
--repo objectstack-ai/objectstack` derived 98 commands from the 5-path
diff. All 98 exited 0.
- **Reconciliation.** `--ran` (with `:: exit N` records) answered: 98
derived, 98 run, 0 NOT-MEASURED, a derived zero.
- **First pass, at 9827a64.** Three gates stopped with PREREQUISITE
NOT MET (exit 3), so they measured nothing: `check:skill-examples`,
`check:dual-build-cjs-loads` and `check:i18n`. I built their
prerequisites and re-ran them; all three exited 0.
`check:query-options-erasure` was red (exit 1), because the new test
file's two query-option bags were erased with an `any` cast (test
surface 236 to 238). It is green at 236 since commit 2a13930, which
types them (`satisfies EngineQueryOptions`).
- **Not run locally.** `test:repo` is not run here; this diff touches
none of its tests, so it is left to CI. The full-battery dogfood pin
(`approval-notification-body.dogfood.test.ts`) is also left to CI. The
CI families are left to the PR's own check-runs.
## Acceptance notes
- **Who a multi-step notice reaches.** A later approval step's request
records the PREVIOUS step's decider as its `submitter_id`. Measured: on
a manager-then-executive flow, step 1 holds `u_sub` and step 2 holds
`u_mgr`. The cause is that `approval-node.ts` passes `context.userId` as
`submitterId`, and the resume-as-decider hand-off has switched that to
the decider. So on a multi-step flow the final notice reaches that
person, as step 2's send-back, request-info and SLA submitter notices
already do. The pin asserts the notice follows the terminal request's
stored `submitter_id`, so it stays green whichever person that field
ends up naming. Reported to the seat as a finding and left unchanged
here: it is outside this card's file surface and is a different defect.
- **The revision-limit auto-reject keeps its old topic.** It still tells
the submitter on `approval.returned` ("Approval auto-rejected"), not on
`approval.rejected`. Unchanged here.
- **Subflow boundary.** The "is there a next step" read covers the
request's own run. If an approval inside a subflow is followed by
another approval in the parent run, the notice is sent at the child's
decision. This case was not measured.
- **The action-link comment.** A decision through the action link
records the comment "Via action link", and the outcome notice carries it
as the comment paragraph.
- **No resend after repair.** A decision whose resume strands sends no
notice, and the operator repair path (`continueRestoredRun`) does not
send one later.
## Out of scope
#22607 is the precedent for adding an approval topic. It is referenced,
not touched. #23556 and #21847 are the families whose rules this notice
follows.
---
_Generated by [Claude
Code](https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c3a8e07 commit 7282a95
5 files changed
Lines changed: 518 additions & 9 deletions
File tree
- .changeset
- content/docs/automation
- 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 | + | |
| 13 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
536 | 536 | | |
537 | 537 | | |
538 | 538 | | |
539 | | - | |
540 | | - | |
| 539 | + | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
541 | 568 | | |
542 | 569 | | |
543 | 570 | | |
| |||
0 commit comments