Skip to content

fix(plugin-approvals): an SLA reassign to a target that resolves to no one keeps the slate - #25600

Merged
objectstack-fleet[bot] merged 1 commit into
mainfrom
claude/project-thread-h5z02d
Oct 24, 2026
Merged

objectstack-fleet[bot] merged 1 commit into
mainfrom
claude/project-thread-h5z02d

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Requested by Sales · project thread

Refs #25598 (the runtime leg; the registration-time leg is a packages/spec change, left for the domain:spec lane — claim 6379436604, "Not this claim")

Clause-②: no
Metadata-protocol: none

What changed

An approval node's escalation: { action: 'reassign', escalateTo } expands escalateTo as a position. When nobody holds it, the sweep falls back to the literal so that a config naming a user id keeps working. Nothing judged that literal, so an unstaffed or mistyped position, a user id with no account, or a team: literal moved the overdue request off its real approvers and onto a slate nobody could decide. That is the state #25592 closed for the reassign door.

The literal is now judged by the door's own predicate:

escalateTo with action: 'reassign' Before After
a position name with holders here slate = holders unchanged
a user id an account carries (member here, or no memberships) slate = id unchanged
an email an account carries slate = email unchanged
position:<p> that someone here holds slate = position:<p> unchanged
an unstaffed / mistyped name, a user id with no account slate = the literal, request stranded slate kept, approvers notified
a user whose memberships are all in other organizations stranded slate kept, approvers notified
position:<p> nobody holds here, a team: / department: / org_membership_level: / role: literal stranded slate kept, approvers notified
  • What happens when the target resolves to no one.
    • pending_approvers, pending_approver_labels and the approver index are left as they were.
    • The sweep takes the notify branch: approval.sla_breached goes to the current slate, and the unresolvable literal is not in the audience.
    • No approval.escalated notice is sent.
    • The escalate audit row reads reassign → <target> — not applied: <why> The pending approvers were kept and notified. The row is still the single-shot marker, so the escalation fires once.
  • The submitter notice names the action taken. It used to report the configured action (escalation action 'reassign' was taken) even when no reassign happened. Now it reports notify in that case, and in the existing case where no escalateTo was set.
  • A failed sys_user read throws before the audit row. The sweep's per-request isolation logs it and leaves the request untouched and unmarked, and the next sweep retries it.
  • action: 'notify' is unchanged, including its audience, which still lists a literal escalateTo. So is every other escalation action.
  • No new error code or export, and nothing in packages/spec or packages/rest changes. Every escalation block that registers today still registers.

Tests

  • approval-service.test.ts gains SLA reassign target resolution (#25598), 6 cases.
    • Refusal cases. Each asserts the slate, labels and index unchanged, the audit comment verbatim, the sla_breached audience (the current slate only), no approval.escalated, the submitter notice saying 'notify', and that a second sweep is a no-op. The cases are:
      • a user id with no account;
      • a position name held only in another organization;
      • a user whose only membership is another organization;
      • a team: literal and a position:ghost nobody holds.
    • A failed sys_user read: nothing moves, no escalate row, no notice, and the next sweep with the directory back escalates.
    • Preservation: an account's user id, its email, and a held position:finance. The slate becomes the target, the audit comment is unchanged, and the submitter notice says 'reassign'. The position's holder then decides with the default actor.
  • Two existing tests that escalated to boss, an id with no account, now give it one (staffDirectory): runEscalations: reassign replaces the approver set with escalateTo and reassign and SLA-reassign rewrite the index rows.
  • New packages/qa/dogfood/test/approval-sla-reassign-target-resolves.dogfood.test.ts runs on a booted stack with the REST door and the real ObjectQL. Two approval flows reassign after one hour, one to a position nobody holds and one to a member's email. Both requests are made overdue (created_at moved back), and the sweep runs through the registered approvals service.
    • The first keeps its approver, who still finds it in "My Pending". Its escalate row says why, and only approval.sla_breached is recorded for it.
    • The second moves to the member, and the member's "My Pending" lists it.

Verification

All on 4525d322, whose tree is identical to the pushed head d7a204a5 (the amend reworded the commit message only). That commit is one commit on main a57643d3, after #25592 merged.

  • pnpm --filter @objectstack/plugin-approvals exec vitest run: 95 files, 1250 tests, all passed. pnpm --filter @objectstack/plugin-approvals typecheck passed, check:test-typecheck included (the ledger held at 324). pnpm --filter @objectstack/dogfood typecheck passed. eslint on the 3 changed source and test files: clean.
  • Ablation, run before the rebase with approval-service.ts restored from fix(plugin-approvals)!: refuse a reassign target that resolves to no one #25592's head 00d5635a:
    • All 5 refusal and failed-read pins in the new block went red. The preservation pin stayed green, as expected.
    • After the plugin's dist was rebuilt, the dogfood pin went red on its first assertion: expected 'sla_reassign_nobody' to be 'sla-reassign-approver@example.com'.
    • After the restore, the tree matched HEAD and git status --porcelain was empty.
  • Local gates (dispatch-gates --commands, 76 families): all 76 exited 0.
  • Full dogfood battery: 296 files, of which 292 passed, 1 failed and 3 were skipped. Tests: 2508 passed, 1 failed, 18 skipped.
    • The new pin passed.
    • The one failure is not this PR's: showcase-d3-d4-capabilities.dogfood.test.ts:135 ([#9237] the grant the rule reconciles…) expects /SHARING_NOT_ENABLED: 'showcase_project' is not under record-sharing enforcement/. main answers …has a public sharing model, so a share row on it is consulted only for an external member of the record's organization…, the text feat(sharing): a share or sharing rule on a public object reaches an external member of the record's organization #25591 introduced, and it reproduces with that file run alone.
    • This diff touches no sharing code. It was reported to the lane for routing.

Acceptance notes


Generated by Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01HopxxRtFE6z3z91wUD54tp

…o one keeps the slate

An approval node's `escalation: { action: 'reassign', escalateTo }` falls
back to the literal `escalateTo` when its position expansion is empty. A
literal that resolved to no one (an unstaffed or mistyped position, a user
id with no account, a `team:` literal) was written onto the slate on
timeout, stranding the request the way the reassign door did before #25592.

The fallback is now judged by the door's own predicate, split out as
`reassignTargetRefusal` (the door's assert is a thin throw over it). A
target that does not resolve leaves the slate, labels and approver index
as they were, takes the `notify` branch to the current slate, and the
`escalate` audit row records why. The submitter notice names the action
actually taken. A failed `sys_user` read throws before the audit row, so
the next sweep retries.

Claude-Session: https://claude.ai/code/session_01HopxxRtFE6z3z91wUD54tp
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 24, 2026
@objectstack-fleet
objectstack-fleet Bot requested a review from os-sales October 24, 2026 04:29
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class))

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

  • content/docs/releases/v17/17-6.mdx (via ApprovalService (symbol, a top-level class))

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
  • 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 a57643d3de7067cb7d292b4802773bdf938fd2cb → packageMentionDocs.

Which tree this was computed on

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

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

⚠️ 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 a57643d3de7067cb7d292b4802773bdf938fd2cb → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: d7a204a5150c743ee4dec70f80783384444dc3b7
Local-runs: none
Scope: packages/plugins/plugin-approvals/src/approval-service.ts, packages/plugins/plugin-approvals/src/approval-service.test.ts, packages/qa/dogfood/test/approval-sla-reassign-target-resolves.dogfood.test.ts, .changeset/25598-sla-reassign-target-resolves.md + packages/plugins/plugin-approvals/src/approval-service.ts (notify, expandPositionUsers, reassign, assertReassignTargetResolves docblock, runEscalations, slateLabels, approverRequestIds), packages/plugins/plugin-approvals/src/approver-address.ts (addressedPosition), packages/plugins/plugin-approvals/src/translations/messages.ts (approvalSlaBreachedSubmitterBody), packages/spec/src/automation/approval.zod.ts (ApprovalEscalationSchema.escalateTo), packages/spec/src/automation/flow-node-config-refusals.ts (the approval contract row), packages/qa/dogfood/test/fixtures/admin-rescue-slate-fixture.ts, packages/qa/dogfood/test/approval-admin-rescue-slate.dogfood.test.ts and approval-reassign-target-resolves.dogfood.test.ts (the approverId list form), packages/services/service-messaging/src/messaging-service.ts (source_id), packages/plugins/plugin-sharing/src/sharing-service.ts (the #25591 text), .github/workflows/ci.yml (the dogfood pull_request skip)

Inputs: card #25598 body and both comments (claim 6379436604, spec-lane pointer 6379444004); PR #25600 body, file list and the net diff a57643d3..d7a204a5 (4 files, +392/-23, one commit; the tree of d7a204a5 is bfa5e7fb, identical to the tree of the 4525d322 the Verification section names); the 42 check-runs on the head: every gate-carrying run success, the dogfood shards, Verify CLI, Console Pin Gate, Temporal Conformance and the two opt-in harnesses skipped on pull_request by design (ci.yml if: on the dogfood matrix). Read-only: nothing built, run or re-run.

① Derived judgments

Accept set: unchanged — Clause-②: no with no arm is right. Nothing the diff touches registers or refuses metadata: packages/spec is untouched, ApprovalEscalationSchema.escalateTo stays z.string().optional(), flow-node-config-refusals.ts still judges the approval node whole against ApprovalNodeConfigSchema and no new check is added to it. The REST door's refusal is byte-identical to #25592's: assertReassignTargetResolves became a throw over reassignTargetRefusal, whose branches and sentences are the old ones moved, so no door narrows. Nothing widens either: no new accepted value, no new export (both methods are private), no new error code, no route change. What moves is the runtime outcome of one path, an overdue request whose reassign target names no one, and that path was a defect (the request left on a slate nobody could decide, the state #25485 closed for the door), not a contract an author could rely on. A (narrowing) arm would claim a refusal that does not exist; its absence is right.

Reusing the door predicate for escalateTo's bare-name / user-id contract: sound. The order of judgment preserves the published contract ("user id or position machine name"): the sweep still tries the bare value as a position first (expandPositionUsers over sys_user_position plus the membership-tier source, organization-scoped); only an EMPTY expansion reaches the predicate, and the predicate then reads the bare value as the door does: position: plus a name resolves through addressedPosition to holders in the request's organization; any other type:value literal is refused outright (no actor acts under team: / department: / org_membership_level: / stored role:, per approver-address.ts); a bare value is a sys_user id or, with an @, an email, then the "not provably a member of other organizations only" membership test. So a staffed position name never reaches the predicate, a user id with an account still reassigns, and both callers yield the same five outcomes. Two postures ride in from the door unchanged and are the door's stated contract, not new: an email is matched as stored (case-exact, the spelling a default actor takes a slot under), and an unreadable sys_member reads as no tenancy fact (resolves). The predicate's own expandPositionUsers swallows a failed sys_user_position read as "no holder" inside it, again as the door does.

Submitter notice reporting the action TAKEN: right. The template is a statement of fact ("escalation action 'ACTION' was taken", four locales), and it was false whenever reassign fell to the notify branch, both in the new unresolved case and in the pre-existing no-escalateTo case; taken fixes both. The configured action is not lost: the escalate audit row still leads with it (reassign → target) and gains the not applied: why suffix only when the slate was kept, so the row distinguishes a reassign that happened from one that was declined. In the unresolved case the approval.sla_breached audience is the pending slate only (escalatees cleared), which is right: the literal names no one to tell, and notify would drop a type:value address anyway.

Failed sys_user read: throw before the audit row is right. The predicate's sys_user read is outside any catch, so it propagates out of escalateRequest before the sys_approval_action insert; nothing is written, runEscalations' per-request try logs at warn and continues to the next row; the next sweep finds no escalate row and retries. ADR-0042 §1 (the row is the marker, written before any mutation) is honoured because no mutation happened. warn is the right level under AGENTS.md's one question: nothing claims to be persisted, the system is visibly behind (the request is still overdue and pending) and a retry is scheduled. The cost is one warn per overdue reassign request per sweep while the directory is down; no durability is at stake.

Tests. The two edits to existing fixtures (staffDirectory(engine, 'boss')) make a precondition explicit that the predicate now enforces; the PR names both. The new block pins, per refusal case, the slate CSV, the labels, the approver index, the audit sentence verbatim, both audiences, the submitter's 'notify', the absence of approval.escalated, and a second sweep as a no-op; the failed-read case pins no row, no notice, and the retry; the preservation case pins id, email and position: plus a held name, with the holder deciding as the default actor. The dogfood pin boots the real composition, drives the sweep through the registered approvals service, and uses two shapes already in service: the approverId=id,email list form that approval-admin-rescue-slate and approval-reassign-target-resolves use, and sys_notification.source_id, which messaging-service writes from source.id. Because the dogfood matrix is skipped on pull_request, this pin's first CI execution is on main's full battery after merge; until then the author's local battery (new pin green) is the only evidence, which is the repo's design for dogfood pins, not a gap in this PR.

Two observations, neither blocking. (a) The spec's escalateTo description names "user id or position machine name" and not the position: plus name and email spellings the runtime (door and sweep alike) judges; the spec-lane seat taking the registration leg should read it. (b) notify drops type:value audiences, so a reassign to a held position: literal announces itself to no one; pre-existing, pinned as it is today, and not a strand (the holder finds the request through the index fold and can decide it).

② Semver level

@objectstack/plugin-approvals: patch is right: a bug fix in a released package, kind none. Metadata-protocol: none is right: no key, accepted value or type is added or removed, the key's published text and default are unchanged, and the outcome that changes is for a value outside the key's stated contract (one naming neither a position nor a user). Clause-②: no with no arm, as judged above; both declaration lines are identical in the PR body and the changeset body. No BREAKING banner and no ADR-0087 marker is right: there is no (narrowing) arm, no removal, no rename, no alias. Check Changeset is green on the head. The changeset's body carries no tracker number in its prose beyond the filename, and its last line tells an author what to write to make a timeout move.

③ Boundary flags

Implemented-by: claude/project-thread-h5z02d
Reviewed-by: session_01HopxxRtFE6z3z91wUD54tp

VERDICT: PASS


Generated by Claude Code

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

席位核对 · ACCEPT · 2026-10-24T05:03Z

domain:services seat 1 (seat post #6021) · os-sales · session_016g7nYP9wdNx1GDa5XDr7Uy · card #25598, runtime leg (claim 6379436604; os-dev-report 6380152448; card session branch claude/project-thread-h5z02d) · reviewed head d7a204a515 · merge base a57643d3de


Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 24, 2026 09:21
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge October 24, 2026 09:21
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 2351c8d Oct 24, 2026
43 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/project-thread-h5z02d branch October 24, 2026 09:48
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

Development

Successfully merging this pull request may close these issues.

2 participants