Skip to content

Commit 3beb224

Browse files
fix(lint): approver verdicts hand an empty slate to onEmptyApprovers instead of saying the request stalls (#27248)
<!-- claude-projects-attribution: {"github_login":"os-support-ai"} --> _Requested by **Yibo Wang** · [project thread](https://claude.ai/code/project/chan_01BVUyyPie8gHwoPM18iABwB?thread=cmsg_01BVUyyPie8gHwoPM18iABwBDiHvKSDRPiRqt9k9Arw6pw)_ Fixes #27101 Claim: 6468213761 Clause-②: no Metadata-protocol: none Skills-docs: none — REASON: copy-only change to two lint verdicts, one `os explain` paragraph and one guide sentence. `git grep -n -e "request stalls" -e approval-approver-type-unsupported -e approval-approver-not-membership-tier -- skills` on base finds 0 hits, so no skill teaches either verdict or the stalled outcome. ## 维护者速读 - **改了什么:** `os validate` 的两条审批人警告(`approval-approver-not-membership-tier`、`approval-approver-type-unsupported`)不再说「请求会卡住」,改为只陈述这个槽位匹配不到人,并以「onEmptyApprovers decides an empty slate」收尾;不支持类型的提示改为从审批人绑定表推导可用路由;同步修正 `os explain` 一段与审批指南一句。 - **为什么改:** 自 `onEmptyApprovers` 默认值变为 `admin_rescue` 起,空审批人名单会转给组织管理员(或按节点声明的策略走),「卡住」是错误描述,会误导作者去「修」正常行为。 - **风险与回滚:** 只改文案与提示,规则触发条件不变,运行时行为不变;回滚 = revert 本 PR。 - **先看:** `packages/lint/src/validate-approval-approvers.ts`(`EMPTY_SLATE_DECIDES` 与 `EXPANDABLE_GROUP_TYPES` 两处)、`packages/lint/src/validate-approval-approvers.unsupported-type.test.ts`。 - **两分钟验证:** `pnpm --filter @objectstack/lint exec vitest run src/validate-approval-approvers.test.ts src/validate-approval-approvers.unsupported-type.test.ts` → 全绿;`git grep -n "and the request stalls" -- packages/lint/src content/docs/automation` → 0 行(main 上 3 行)。 - **需要你做的:** 无,席位落地。 ## What changes File surface (claim 6468213761, plus the three additions seat 1 approved): - `packages/lint/src/validate-approval-approvers.ts`. The two verdicts that ended on "the request stalls" now state the slot and end on one shared clause, `EMPTY_SLATE_DECIDES` = "onEmptyApprovers decides an empty slate": - `approval-approver-not-membership-tier`: "approver { type: 'org_membership_level', value: 'sales_manager' } names no membership tier (owner/admin/delegated_admin/member/external), so it matches nobody; onEmptyApprovers decides an empty slate". - `approval-approver-type-unsupported`: "… the slot resolves to nobody, and onEmptyApprovers decides an empty slate." - Why this wording and not a per-policy outcome: the triage asked for copy that cannot drift. One approver slot cannot see the rest of the slate, so the request's fate is not the verdict's to state. The clause names no outcome and no default, so it stays true under all four `onEmptyApprovers` values and under any later change to the default. This matches the runtime's own expansion warnings in `resolveApproverSpec`, which "state the SLOT, never the request's fate". A per-policy outcome would also not fit: the membership-tier verdict is pinned to 200 characters, and the tier list (now five tiers) leaves no room for it. - "org-membership tier" became "membership tier" in that verdict, to stay within the same pin. The type name `org_membership_level` is already in the sentence. - The `approval-approver-type-unsupported` hint now builds its route list from `APPROVER_VALUE_BINDINGS` (`EXPANDABLE_GROUP_TYPES`: the group-routed types whose binding is not `unsupported`). The hardcoded list used to offer `queue` while `queue` was the one unsupported type. With this change the hint can never offer an unsupported type, including the flagged one. `queue` is supported today (ADR-0142 D6), so it stays in the list. - Code comments: the arm's comment now says no binding is `unsupported` on main and that the arm stays as the guard for the next one. The file header no longer describes a "stuck request". - `packages/lint/src/rule-explanations.ts` (addition 1). The `approval-approver-not-membership-tier` explanation said "the request waits on an approver that can never act". It now says the literal is a slot no one can act on, and a slate that names no one else is decided by the node's `onEmptyApprovers`. - `packages/lint/src/validate-approval-approvers.test.ts`. The membership-tier verdict's exact-string pin is updated. A new case drives the verdict under an omitted policy and under `admin_rescue`, `fail` and `auto_approve`, and asserts that it names no outcome under any of them. - `packages/lint/src/validate-approval-approvers.unsupported-type.test.ts` (addition 2, new). No binding is `unsupported` on main, so this arm is reachable only against a binding table that declares one. The file mocks `@objectstack/spec/automation` with `team` marked `unsupported`, in its own module graph, and pins the message and the hint. The hint pin is `'position' | 'department' | 'queue'`, so the flagged `team` is not offered. - `content/docs/automation/approvals.mdx` (addition 3, seat 1's request). The `position` vs `org_membership_level` callout said "matches nobody and the request stalls". It now says the approver adds no one, and a slate left naming no one goes to the node's `onEmptyApprovers`. - `.changeset/27101-approver-lint-empty-slate-copy.md`: `@objectstack/lint` patch, with both declaration lines. No rule's firing condition changes. ## Why The default `onEmptyApprovers: 'admin_rescue'` opens a slate that names no one on the organization's administrators (`openNodeRequest` in `packages/plugins/plugin-approvals/src/approval-service.ts`, `slateNamesAPerson` branch). Other policies fail the node, auto-approve, or open on `fallbackApprovers`. "The request stalls" is true under none of them. An author who believes it either "fixes" something that works or misreads what happens at runtime. ## Tests <details><summary>Readings (head 484c616, base origin/main dfe85c9)</summary> - `pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2 src/validate-approval-approvers.test.ts src/validate-approval-approvers.unsupported-type.test.ts src/rule-explanations` → Test Files 3 passed (3), Tests 73 passed (73). - `pnpm --filter @objectstack/lint test` → Test Files 173 passed (173), Tests 7533 passed, 5 skipped (7538). - `pnpm --filter @objectstack/lint typecheck` → exit 0 (`tsc --noEmit` and `check:test-typecheck` OK). - Ablation, one time, on the committed head: `node scripts/ablation-replace.mjs` changed the `EXPANDABLE_GROUP_TYPES` filter to `… !== 'unsupported' || true` (anchor 1 → 0, blob 7ee40cc → 510c23d1fd9a). The unsupported-type test went red on "never offers an unsupported type as the route, the flagged one included". After the restore, `git diff HEAD` was empty. The suite imports `./validate-approval-approvers.js` from source, so no dist rebuild was involved. </details> ## Acceptance notes - `content/docs/automation/approvals.mdx` L58 still says the tier's "only values are `owner`/`admin`/`member`". `BUILTIN_MEMBERSHIP_ROLES` also has `delegated_admin` and `external`. This is a separate stale fact, not touched here. - Sibling verdicts in the same file still spell the default outcome as fixed text. The `approval-approvers-may-resolve-empty` group and manager arms say "the request opens on the organization's administrators" even when the node declares another `onEmptyApprovers`. `approval-expression-no-empty-policy` restates `'admin_rescue'`, but it fires only when the policy is omitted. Neither is in this card's scope, and their messages are pinned, so they are noted here only. ## Verification <details><summary>Derived gates (dispatch-gates --commands at 484c616, 87 families)</summary> `dispatch-gates --ran` → "87 derived famil(ies) accounted for — 83 run, 4 NOT-MEASURED (4 DERIVED from a recorded exit 3)". - 83 exit 0. That includes `check-plugin-teardown-shape --self-test` (48 cases), which passed after fetching its pinned fixture commit, and `lint check:doc-formula-expressions` and `check:doc-security-posture`, which passed after `pnpm --filter @objectstack/lint build`. - NOT MEASURED (exit 3, a full-repo build is missing): `spec check:skill-examples`, `check:docs-transcript-drift`, `check:dual-build-cjs-loads`, `check:lean-entry-closure`. CI runs them. - Dogfood: n/a. The title has no `!` and there is no `(narrowing)` arm. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (Not touched.) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01WGufzifjQqjY4ZFCkVmUgJ --- _Generated by [Claude Code](https://claude.ai/code/session_01WGufzifjQqjY4ZFCkVmUgJ)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7858c1d commit 3beb224

6 files changed

Lines changed: 124 additions & 17 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/lint': patch
3+
---
4+
5+
fix(lint): two approval-approver warnings no longer say "the request stalls"
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
`approval-approver-not-membership-tier` (an `org_membership_level` approver whose value names no tier) and `approval-approver-type-unsupported` ended on "the request stalls". That has not been true since an approval node's `onEmptyApprovers` default became `admin_rescue`: a slate that names no one opens on the organization's administrators, or takes whatever route the node declares. Both verdicts now state only the slot ("it matches nobody" / "the slot resolves to nobody") and end on "onEmptyApprovers decides an empty slate". That stays true under every policy and whatever the default is, because it names no outcome. The `os explain approval-approver-not-membership-tier` text gets the same correction.
11+
12+
The `approval-approver-type-unsupported` hint now builds its "route to people the engine can expand" list from the approver bindings, so it can never offer a type that is itself unsupported. It used to offer `queue` while `queue` was the one unsupported type. Today `queue` is supported, so it stays in the list. No approver type is unsupported today, so the warning fires on no current type.
13+
14+
No rule fires on anything new or stops firing on anything it fired on before. Only the message and hint text changes.

‎content/docs/automation/approvals.mdx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,9 @@ defineFlow({
5757
**`position` vs `org_membership_level`.** `{ type: 'position', value: 'finance_manager' }` routes
5858
to the holders of a position (`sys_user_position`, ADR-0090 D3). `org_membership_level` is a
5959
different thing — the better-auth **org-membership tier**, whose only values are
60-
`owner`/`admin`/`member`. A position name authored there matches nobody and the request stalls;
61-
`os lint` flags it (`approval-approver-not-membership-tier`).
60+
`owner`/`admin`/`member`. A position name authored there matches nobody, so that approver adds no one
61+
to the request, and a slate left naming no one goes to the node's `onEmptyApprovers`; `os lint` flags it
62+
(`approval-approver-not-membership-tier`).
6263

6364
Authored `type: 'role'` on 15.x? That is the deprecated spelling of `org_membership_level`
6465
(ADR-0090 D3): it still resolves, warns at runtime, and is removed in the next major. It resolves

‎packages/lint/src/rule-explanations.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2779,8 +2779,9 @@ const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER_EXPLANATION: RuleExplanation = {
27792779
'case-insensitively.',
27802780
'After ADR-0090 D3 renamed `sys_role` to `sys_position`, an approver authored as ' +
27812781
'`{ type: \'role\', value: \'sales_manager\' }` finds no member row: the expansion falls back ' +
2782-
'to the `role:sales_manager` literal, and the request waits on an approver that can never ' +
2783-
'act. A business role is always a position, resolved through `sys_user_position`.',
2782+
'to the `role:sales_manager` literal, a slot no one can act on. A slate that names no one ' +
2783+
'else is decided by the node\'s `onEmptyApprovers`, as any empty slate is. A business role is ' +
2784+
'always a position, resolved through `sys_user_position`.',
27842785
'A warning, not an error: the runtime keeps its literal fallback rather than refusing the node. ' +
27852786
'When the value is not a tier, this finding is reported instead of ' +
27862787
'`approval-approver-type-deprecated` on the same approver: the bad value is the more serious ' +

‎packages/lint/src/validate-approval-approvers.test.ts‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -784,11 +784,31 @@ describe('[#22161] one-line verdicts — approval-approvers-may-resolve-empty, a
784784
stackWithApprovers([{ type: 'org_membership_level', value: 'sales_manager' }]),
785785
);
786786
expect(finding.message).toBe(
787-
"approver { type: 'org_membership_level', value: 'sales_manager' } names no org-membership tier " +
788-
`(${BUILTIN_MEMBERSHIP_ROLES.join('/')}), so it matches nobody and the request stalls`,
787+
"approver { type: 'org_membership_level', value: 'sales_manager' } names no membership tier " +
788+
`(${BUILTIN_MEMBERSHIP_ROLES.join('/')}), so it matches nobody; onEmptyApprovers decides an empty slate`,
789789
);
790790
});
791791

792+
it('the membership-tier verdict names no empty-slate outcome, so no onEmptyApprovers policy makes it false', () => {
793+
// The slot is the verdict's subject; the request's fate is the node's
794+
// policy, whose default the verdict must not restate.
795+
for (const onEmptyApprovers of [undefined, 'admin_rescue', 'fail', 'auto_approve']) {
796+
const [finding] = validateApprovalApprovers({
797+
flows: [{
798+
name: 'expense_approval',
799+
nodes: [{
800+
id: 'step1',
801+
type: 'approval',
802+
config: { approvers: [{ type: 'org_membership_level', value: 'sales_manager' }], onEmptyApprovers },
803+
}],
804+
edges: [],
805+
}],
806+
});
807+
expect(finding.rule, String(onEmptyApprovers)).toBe(APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER);
808+
expect(finding.message, String(onEmptyApprovers)).not.toMatch(/stall|administrators|auto-approv|fails/);
809+
}
810+
});
811+
792812
it('`os explain` carries what the verdicts no longer say', () => {
793813
const facts: Record<string, string[]> = {
794814
[APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY]: [

‎packages/lint/src/validate-approval-approvers.ts‎

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@
88
* — it is NOT a position. After ADR-0090 D3 renamed `sys_role` → `sys_position`,
99
* downstream apps that authored `{ type: 'role', value: 'sales_manager' }`
1010
* silently route the approval to nobody: the expansion finds no member row,
11-
* falls back to the `role:sales_manager` literal, and the request waits on an
12-
* approver that can never act. These rules move that failure from a stuck
13-
* request at runtime to a located fix-it at author time.
11+
* falls back to the `role:sales_manager` literal, a slot no one can act on,
12+
* and a slate that names no one else lands on the node's `onEmptyApprovers`
13+
* instead of the people the author meant. These rules move that failure from
14+
* runtime to a located fix-it at author time.
1415
*
1516
* Rules:
1617
*
@@ -231,6 +232,27 @@ const GROUP_ONLY_FIX =
231232
"Staff at least one target, add an approver that cannot resolve empty (e.g. { type: 'org_membership_level', " +
232233
"value: 'owner' }), or name who decides in fallbackApprovers with onEmptyApprovers: 'fallback'";
233234

235+
/**
236+
* [#27101] The clause both single-slot verdicts end on — one wording, so the
237+
* two arms cannot drift. It states the SLOT, never the request's fate, the
238+
* runtime's own rule for its expansion warnings (`resolveApproverSpec` in
239+
* plugin-approvals): one approver cannot see the rest of the slate, and what an
240+
* empty slate does is the node's `onEmptyApprovers`, whose default can move.
241+
* ⛔ Never name an outcome or the default here: "the request stalls" was such
242+
* a restated outcome, and it went false when the default became `admin_rescue`.
243+
*/
244+
const EMPTY_SLATE_DECIDES = 'onEmptyApprovers decides an empty slate';
245+
246+
/**
247+
* [#27101] The routes the unsupported-type hint offers: the group-routed types
248+
* whose binding the runtime implements, derived so the hint can never offer a
249+
* type that is itself `unsupported` (it offered `queue` while `queue` was the
250+
* one unsupported type, ADR-0142 D6).
251+
*/
252+
const EXPANDABLE_GROUP_TYPES = [...GROUP_ROUTED_TYPES].filter(
253+
(t) => (APPROVER_VALUE_BINDINGS as Record<string, { source: string }>)[t]?.source !== 'unsupported',
254+
);
255+
234256
export type ApprovalApproverSeverity = 'error' | 'warning' | 'info';
235257

236258
export interface ApprovalApproverFinding {
@@ -458,8 +480,8 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin
458480
// [#22161] One verdict; what the type resolves against and why a
459481
// business role is a position is `os explain` text.
460482
message:
461-
`approver { type: '${type}', value: '${value}' } names no org-membership tier ` +
462-
`(${MEMBERSHIP_TIER_LIST}), so it matches nobody and the request stalls`,
483+
`approver { type: '${type}', value: '${value}' } names no membership tier ` +
484+
`(${MEMBERSHIP_TIER_LIST}), so it matches nobody; ${EMPTY_SLATE_DECIDES}`,
463485
// [#22161] One remedy line: the verdict already lists the tiers,
464486
// and why a business role is always a position (the closed
465487
// ADR-0108 vocabulary, `sys_user_position`) is the explanation's.
@@ -483,19 +505,24 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin
483505
(APPROVER_VALUE_BINDINGS as Record<string, { source: string }>)[canonical]?.source === 'unsupported'
484506
) {
485507
// Declared-but-unenforced (#3508): the runtime has no resolution for
486-
// this type — the slot degrades to an inert `type:value` literal and
487-
// the request routes to nobody. Say it at authoring time instead of
488-
// letting the request stall silently (Prime Directive #10).
508+
// this type — the slot degrades to an inert `type:value` literal that
509+
// names no one, and a slate left naming no one takes the node's
510+
// `onEmptyApprovers` route. Say it at authoring time instead of
511+
// letting the slot fail silently (Prime Directive #10). No binding is
512+
// `unsupported` today (ADR-0142 D6 implemented the last, `queue`);
513+
// the arm stays as the guard for the next one.
489514
findings.push({
490515
severity: 'warning',
491516
rule: APPROVAL_APPROVER_TYPE_UNSUPPORTED,
492517
where,
493518
path: `${path}.type`,
494519
message:
495520
`approver type '${type}' is declared but not implemented by the runtime: it was deprecated ` +
496-
`rather than built, and is no longer offered for authoring — the slot resolves to nobody and ` +
497-
`the request stalls.`,
498-
hint: `Route to people the engine can expand: { type: 'team' | 'department' | 'position' | 'queue', ... }.`,
521+
`rather than built, and is no longer offered for authoring — the slot resolves to nobody, and ` +
522+
`${EMPTY_SLATE_DECIDES}.`,
523+
hint:
524+
`Route to people the engine can expand: ` +
525+
`{ type: ${EXPANDABLE_GROUP_TYPES.map((t) => `'${t}'`).join(' | ')}, ... }.`,
499526
});
500527
}
501528

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, expect, it, vi } from 'vitest';
4+
5+
// No approver binding is `unsupported` today (ADR-0142 D6 implemented the
6+
// last one, `queue`), so the `approval-approver-type-unsupported` arm is
7+
// reachable only against a binding table that declares one. This file marks
8+
// `team` unsupported — its own module graph, so the main suite's real table
9+
// is untouched.
10+
vi.mock('@objectstack/spec/automation', async (importOriginal) => {
11+
const actual = await importOriginal<typeof import('@objectstack/spec/automation')>();
12+
return {
13+
...actual,
14+
APPROVER_VALUE_BINDINGS: { ...actual.APPROVER_VALUE_BINDINGS, team: { source: 'unsupported' } },
15+
};
16+
});
17+
18+
const { validateApprovalApprovers, APPROVAL_APPROVER_TYPE_UNSUPPORTED } = await import(
19+
'./validate-approval-approvers.js'
20+
);
21+
22+
describe('approval-approver-type-unsupported', () => {
23+
const [finding] = validateApprovalApprovers({
24+
flows: [{
25+
name: 'expense_approval',
26+
nodes: [{ id: 'step1', type: 'approval', config: { approvers: [{ type: 'team', value: 't1' }] } }],
27+
edges: [],
28+
}],
29+
}).filter((f) => f.rule === APPROVAL_APPROVER_TYPE_UNSUPPORTED);
30+
31+
it('states the slot and hands an empty slate to onEmptyApprovers, naming no outcome', () => {
32+
expect(finding.message).toBe(
33+
"approver type 'team' is declared but not implemented by the runtime: it was deprecated rather " +
34+
'than built, and is no longer offered for authoring — the slot resolves to nobody, and ' +
35+
'onEmptyApprovers decides an empty slate.',
36+
);
37+
});
38+
39+
it('never offers an unsupported type as the route, the flagged one included', () => {
40+
expect(finding.hint).toBe(
41+
"Route to people the engine can expand: { type: 'position' | 'department' | 'queue', ... }.",
42+
);
43+
});
44+
});

0 commit comments

Comments
 (0)