Skip to content

fix(plugin-approvals): the decision that ends an approval tells its submitter the outcome - #24816

Merged
objectstack-fleet[bot] merged 6 commits into
mainfrom
claude/issue-24753-approval-decision-notifies-submitter
Oct 20, 2026
Merged

objectstack-fleet[bot] merged 6 commits into
mainfrom
claude/issue-24753-approval-decision-notifies-submitter

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

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:

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

…submitter the outcome

approval.approved / approval.rejected reach the request's submitter once
the decision ends the approval process: not on a co-sign vote that leaves
the request pending, not when the resume opened a further approval step on
the same run, not when the submitter decided it, and not for the SLA
sweep's auto-decision, which the escalation's notifySubmitter notice tells.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Oct 20, 2026
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 7 documentable anchor(s).

5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via approval.approved (literal, a string literal in notifyDecisionOutcome), approval.rejected (literal, a string literal in notifyDecisionOutcome), sys_approval_request (literal, a string literal in notifyDecisionOutcome))
  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class), sys_approval_request (literal, a string literal in notifyDecisionOutcome))
  • content/docs/automation/workflows.mdx (via sys_approval_request (literal, a string literal in notifyDecisionOutcome))
  • content/docs/plugins/packages.mdx (via sys_approval_request (literal, a string literal in notifyDecisionOutcome))
  • content/docs/ui/translations.mdx (via sys_approval_request (literal, a string literal in notifyDecisionOutcome))

⛔ 2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via sys_approval_request (literal, a string literal in notifyDecisionOutcome))
  • content/docs/releases/v17/17-6.mdx (via ApprovalService (symbol, a top-level class), sys_approval_request (literal, a string literal in notifyDecisionOutcome))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json c56115bf962cb301fb046c54680620d37e5ab2fb → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 1c0d4620b5a9521e0b61464aad9a6aa38e4715da — the merge of head c16244e6a46c4a12be703a07d1a5b8343639ce9c into base c56115bf962cb301fb046c54680620d37e5ab2fb, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 1c0d4620b5a9521e0b61464aad9a6aa38e4715da && git checkout 1c0d4620b5a9521e0b61464aad9a6aa38e4715da
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c56115bf962cb301fb046c54680620d37e5ab2fb c16244e6a46c4a12be703a07d1a5b8343639ce9c && git checkout -B drift-repro c56115bf962cb301fb046c54680620d37e5ab2fb && git merge --no-ff c16244e6a46c4a12be703a07d1a5b8343639ce9c

node scripts/docs-audit/affected-docs.mjs --json c56115bf962cb301fb046c54680620d37e5ab2fb

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs c56115bf962cb301fb046c54680620d37e5ab2fb → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…use-② yes (widening)

The two new subscribable topics follow the in-lane precedent for a new
approval topic. The changeset also states what git grep measured: no
example flow tells the submitter the decision with a notify node.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
…itter as the design

A later approval step recording the previous decider as its submitter is
a defect filed on its own card, so neither the changeset nor the page
states it as how the notice behaves. The changeset summary reads feat,
matching its minor bump.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

Served-tier: TIER_DEFAULT
Head-sha: c16244e6a46c4a12be703a07d1a5b8343639ce9c
Local-runs: none

① Derived judgments

The face is the Clause-②: yes (widening) declaration alone. No published schema, no governed path: governedPathsIn answers [], and nothing under packages/spec/src/** moves. What the declaration widens:

② Semver level

③ Boundary flags

All six dev open_questions are answered.

Each deviation is accepted:

  • the wider build to clear three PREREQUISITE NOT MET gates;
  • the stray empty files, deleted before any commit;
  • the three extra guard ablations;
  • the throwaway probe, never committed;
  • the model-free trailers.

Nothing is escalated.

Implemented-by: claude/issue-24753-approval-decision-notifies-submitter
Reviewed-by: session_011TdZ8GjixNZFLz1xJD9zzK

VERDICT: PASS

Rendered in-seat at TIER_DEFAULT by the domain:services lane seat, the declaration face's reviewer of record · 2026-10-20T08:18Z.


Generated by Claude Code

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

席位核对 · ACCEPT · domain:services seat 1 · 2026-10-20T08:46Z

os-justin · session_011TdZ8GjixNZFLz1xJD9zzK (seat post #6021). Reviewed against GitHub and origin/main 87c09ebb7a, not against the report (os-dev-report on #24753). The declaration face's review of record is 6286636628 (TIER_DEFAULT, PASS on this head). Head under review:

c16244e6a46c4a12be703a07d1a5b8343639ce9c
  1. Shape. Draft, base main, first line Fixes #24753, line-start Clause-②: yes in the body (seat edit after claim amendment 6286428768), and Clause-②: yes (widening) in the changeset. The body names plugin-approvals: an approval request's notification names the record by its raw id (crm_opportunity/<id>), and opening one on an empty slot logs "cannot advance" just before routing to the administrators #23556 and approvals: every approval notification reaches the recipient with an empty body — the service puts its text in payload.message, messaging reads payload.body (comment and request-info text are lost) #21847 with no keyword beside either. 5 files, none on a governed surface. The trial merge onto origin/main is clean.
  2. The code, read.
    • decide() now runs decideAndResume(..., 'outcome'). After the resume it calls notifyDecisionOutcome only when the decision finalised the request.
    • That function returns early in three cases:
      • the submitter is the decider;
      • a pending request remains on the same flow_run_id (a later step opened);
      • that read itself fails, which is logged and silent, so the decision is never failed by its notice.
    • Otherwise it calls the existing soft-failing notify() on approval.approved / approval.rejected, with the record named and the comment as its own paragraph.
    • The SLA sweep calls decideAndResume(..., 'escalation'), so approval.sla_breached stays its one submitter message.
  3. Pins.
    • approval-decision-outcome-notification.integration.test.ts covers 8 cases on a real ApprovalService + AutomationEngine over SqlDriver:
      • approved;
      • rejected with a comment;
      • self-decided, with its control;
      • a co-sign vote;
      • a two-step flow with no notice at step 1;
      • a reject at step 1;
      • a gateway skip;
      • SLA true/false, with a human control.
    • The body battery now drives the new site.
    • The four ablations (the call, the self guard, the next-step guard, the SLA switch) each turn red on their own case, with every restore proven by blob equality.
  4. Prose read sentence by sentence against the diff.
  5. Gates. At ACCEPT, every check-run on this head is success or an expected skip, including TypeScript Type Check, Lint & Repo Gates, Test Core 1–6/6, Build Core and Governed Surface Queue Guard. 18.x upgrade harness (opt-in) is on the expected-skip roster (check-expected-skips.mjs, label-opt-in). Locally, 98 of 98 derived families exited 0 at 2a13930f15. The two prose-only rounds re-ran the changeset gates and the 40 docs families at c16244e6a4, all exit 0.
  6. The dev's open questions are answered in the review of record; Clause-② is re-declared B. The stale-tree note (the branch is behind main) is left to the merge group, which builds the combination; the trial merge is clean.
  7. Accepted deviations.
    • The wider build to clear prerequisites.
    • Stray empty files, deleted before any commit.
    • Extra guard ablations.
    • A throwaway probe, never committed.
    • Model-free trailers.
  8. Out-of-scope dispositions.
  9. Landing clause. Tier S, after the declaration face's PASS on this head. Next step: ready + auto-merge through the relay once every check on the current head is green or an expected skip. A push resets this reading.

Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 20, 2026 08:48
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit 7282a95 Oct 20, 2026
43 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/issue-24753-approval-decision-notifies-submitter branch October 20, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plugin-approvals: the submitter is never notified of the final decision — approving or rejecting a request emits no notification to them

2 participants