Skip to content

fix(plugin-approvals): every step of a multi-step approval names the run's submitter, not the previous step's approver - #24860

Merged
objectstack-fleet[bot] merged 4 commits into
mainfrom
claude/issue-24817-later-step-original-submitter
Oct 20, 2026
Merged

objectstack-fleet[bot] merged 4 commits into
mainfrom
claude/issue-24817-later-step-original-submitter

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

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 feat(automation)!: an approval decision resumes a flow as the person who decided #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 fix(plugin-approvals): the decision that ends an approval tells its submitter the outcome #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

…n's trigger user

Reproduction first: on a real AutomationEngine + ApprovalService over
SqlDriver, a later approval step opens with the previous step's decider
as submitter_id. These pins go red on the unfixed approval node.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
…user as its submitter

Since a decision resumes its flow as the decider, the approval node's
context.userId on a later step is the previous step's decider. The node
now opens each step with the run's trigger user, the engine's own
triggerUserId reading, and openNodeRequest treats an explicit null
submitter as no submitter instead of falling back to the decider.

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 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 7cacad5348d0f2665f1c069eccb5131bd955d768 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from d8320a16d266278d7ace5d6e58e953f6e441ce05 — the merge of head bc2d3afd94e902bf97ac911df7607ab27943f34b into base 7cacad5348d0f2665f1c069eccb5131bd955d768, 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 d8320a16d266278d7ace5d6e58e953f6e441ce05 && git checkout d8320a16d266278d7ace5d6e58e953f6e441ce05
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7cacad5348d0f2665f1c069eccb5131bd955d768 bc2d3afd94e902bf97ac911df7607ab27943f34b && git checkout -B drift-repro 7cacad5348d0f2665f1c069eccb5131bd955d768 && git merge --no-ff bc2d3afd94e902bf97ac911df7607ab27943f34b

node scripts/docs-audit/affected-docs.mjs --json 7cacad5348d0f2665f1c069eccb5131bd955d768

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

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

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

os-justin · session_011TdZ8GjixNZFLz1xJD9zzK (seat post #6021). Reviewed against GitHub and origin/main 9a06f0be0f, not against the report (os-dev-report on #24817). Head under review:

bc2d3afd94e902bf97ac911df7607ab27943f34b
  1. Shape.

  2. The fix, read in the code.

    • approval-node.ts opens every step with runTriggerUser(context), not context.userId. When the context carries triggerUserId, the function reads it, and a recorded null stays null (a hasOwnProperty read, never ??). Otherwise it reads userId, which is still the trigger user when no decision switch happened.
    • This mirrors the engine's own runTriggerUserId (service-automation runtime-identity.ts). The engine records triggerUserId at the first decision switch and persists it through each pause. The plugin does not depend on service-automation, and nestVariables is the precedent for such a mirror.
    • openNodeRequest now honours an explicit submitterId: null (a user-less trigger) instead of falling back to the acting context. An omitted value still falls back. The only caller is the approval node.
    • The execution context handed to openNodeRequest is unchanged, so the decider stays the actor of the resumed segment, as A resumed flow run writes as its trigger user (the submitter), not the approver who resumed it, so post-decision writes in an approval flow name the submitter as Last Modified By and in the audit log #24321's ruling requires.
    • No spec edit and no stored key.
  3. Pins and ablations. later-step-original-submitter.integration.test.ts runs a real AutomationEngine + ApprovalService over SqlDriver, 8 cases:

    • three steps on both runAs postures, each naming the trigger user;
    • the step after a gateway-skipped step;
    • a user-less run, whose later step has no submitter;
    • recall and resubmit on step 2: the step-1 decider is refused FORBIDDEN and the submitter is admitted;
    • the two-step run's final notice reaching the original submitter;
    • a one-step control.

    Before the fix the suite read 7 failed | 1 passed, with expected 'u_mgr' to be 'u_sub'. Three ablation legs (the node's old line, the service's old fallback, a ?? reading) each turned red exactly where predicted, and each restore was proven by blob equality.

  4. Changeset, @objectstack/plugin-approvals patch, read sentence by sentence against 2 and 3:

  5. Gates. At ACCEPT, every check-run on this head is success or an expected skip, among them TypeScript Type Check, Lint & Repo Gates, Test Core 1–6/6, Build Core and Governed Surface Queue Guard.

    • Locally, 68 of 70 derived families exited 0. 2 were NOT MEASURED (check:dual-build-cjs-loads, check:i18n, exit 3 on a missing prerequisite); CI's lint lane measures them.
    • plugin-approvals: 1172 of 1172.
  6. The dev's open question: A, no spec key now. Declaring triggerUserId on spec's AutomationContext would add contract surface with one consumer. The source is already engine-owned and persisted. The one misreading is pinned by ablation leg C. Revisit when a second executor needs the trigger user.

  7. Accepted deviations.

    • approval-service.ts openNodeRequest, outside the claim's named surface. The card's rule needs it, ablation leg B shows it, and the PR body declares it.
    • The refusal code is asserted at the service seam, through the FORBIDDEN: prefix that handleApprovalError maps. The 403 status is pinned live in packages/rest/src/rest-approvals-wire-codes.test.ts, because plugin-approvals cannot import rest.
    • A test-helper default parameter was corrected before the repro.
    • The ablations ran before one clean merge of main that touched no plugin-approvals file.
    • Model-free trailers.
  8. Out-of-scope dispositions. AutomationContext does not declare triggerUserId (6 above): Acceptance notes.

  9. Landing clause. Tier S: not governed, under the human-merge threshold. 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

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/m tests tooling

Projects

None yet

2 participants