Repository navigation
Commit d3175d5
fix(plugin-approvals)!: judge typed decision outputs against their declared type and multiple (#29372)
domain:server#5 · os-steve · session_01ADHkmQKwKFCp1m6Pb6Q8c1
<!-- claude-projects-attribution: {"github_login":"os-steve"} -->
_Requested by **Steve** · [project
thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi969Unb1SZT95qxxthkfnkim)_
Fixes #28658
Clause-②: no (narrowing)
Metadata-protocol: none
Ruling-ref: 6566731522
Skills-docs: follow-up #29512 —
skills/objectstack-automation/references/state-machines-and-approvals.md
## 维护者速读
- 改了什么:审批「决策输出」声明了 `type`(user / department / team / position)和
`multiple` 时,审批人提交的值现在当场按声明校验:选人选部门选团队要能查到真实记录,岗位要是有人担任的岗位名;`multiple:
true` 必须给列表且不能重复,没声明 `multiple` 只能给单个值。不合格就 400 拒绝、点名
`outputs.键名`,什么都不记录,请求保持待审。
- 为什么改:#28658 裁决 A。以前填一个不存在的用户 id,审批照样记为通过,下一步表达式审批人解析不到人,流程 500
卡死(请求显示「已通过」,流程已失败)。
- 风险与回滚:收窄所有决策输出的形状(`multiple: true`
必须是不重复的列表,否则只能一个值),四种选人类型还要能查到记录。Console 同步改:声明了 `multiple: true`
的文本输出改成标签输入,发送字符串数组;单值文本仍发一个字符串,逗号原样保留。驳回时带了不合格输出也会被拒。回滚即撤掉本 PR(两处一起撤)。
- 先看:`ui/packages/app-shell/src/utils/decisionOutputParams.ts`(Console
发列表);`packages/plugins/plugin-approvals/src/approval-service.ts` 的
`assertDecisionOutputsMatchDeclarations` 与
`decisionOutputValueRefusal`(各类型复用表达式审批人的同一组查找);`decideNode` 里
`required` 检查之后的调用点。
- 两分钟验证:`pnpm --filter @objectstack/plugin-approvals exec vitest run
src/approval-node.test.ts -t "typed user output"`,应看到 1 条通过:错
id、单值、重复都被拒且流程仍停在第一步,合法选择后两步走完。
- 需要你做的:无,席位落地(规格文案卡 #29340 已由 #29371 合入)。
## What changes
- **The decision door** (`ApprovalService.decideNode`,
`packages/plugins/plugin-approvals/src/approval-service.ts`): after the
`required` check and before `assertRunResumable` and any write, each
admitted output is judged by the new
`assertDecisionOutputsMatchDeclarations`. One `VALIDATION_FAILED` names
every `outputs.KEY` that does not match; the approvals door already maps
that prefix to `400` (`handleApprovalError` in
`packages/rest/src/approvals-routes.ts`, unchanged). Nothing is written:
no `sys_approval_action` row, no `__decisionOutputs` snapshot, the
request stays `pending` and its slate unchanged.
- `multiple: true` takes a list and only a list, with no entry twice;
`multiple` absent or `false` takes one value; each value is non-blank
text.
- Each value resolves through the lookup the matching `expression`
approver's `resolveAs` uses (`decisionOutputValueRefusal`), in the
request's organization:
- `user`: `reassignTargetRefusal(value, null)`, the predicate a `user` /
`expression` approver asks (no organization, #27397). A slot address
(`KIND:VALUE`) or an email is refused as "not a user id": the user
picker commits record ids.
- `department`: `businessUnitIsLive`, the seed check of
`expandBusinessUnitUsers`, extracted into its own method (the expansion
now calls it; behaviour unchanged).
- `team`: `readTeamRow`, the `sys_team` read of
`teamIsProvablyOutsideOrg`, extracted (that screen now calls it;
behaviour unchanged). Refused when no row exists, or the row names
another organization.
- `position`: `expandPositionUsers(name, org)`, the expansion the
`expression` approver uses, which matches `sys_user_position.position`
by machine name. A `sys_position` record id has no holder under that
name and is refused.
- A directory read that fails proves nothing and refuses nothing, as
every resolution in this file reads one.
- **The Console**
(`ui/packages/app-shell/src/utils/decisionOutputParams.ts`, the one
place both decision surfaces build their decision-output params): a
`text` or untyped output declared `multiple: true` (and a `position`
output with no registry reader) renders as a `tags` param instead of one
comma text box. `TagsField` stores a `string[]`, trims each entry and
skips a blank or repeated one, so the decide body carries a list the
duplicate refusal never trips on. Both folds hand the value on
untouched: `foldDecisionOutputs` (record header) and the runtime api
handler's `outputs.KEY` fold
(`packages/runtime/src/action-execution.ts`, Approval Center), whose
list pass-through is already pinned in `api-action-forward.test.ts`. No
fold changed. A single-value output stays a `text` box; its comma is
literal. Every output's help now reads `actions.decisionOutput.help`;
the comma hint `actions.decisionOutput.helpMultiValue` leaves all ten
locale packs (its only call site is gone).
- **`text` and bare keys**: judged on shape only (seat answer Q4 = B).
`multiple: true` takes a list with no entry twice; `multiple` absent or
false takes one value. A bare key (`decisionOutputs: ['note']`)
therefore takes one value, and a list is what an output declared
`multiple: true` collects. Text values are not resolved. A blank value
(`''`, `[]`, `null`) is `required`'s question only.
- **Reject**: a reject carrying a mismatched output is refused too. The
outputs ride the reject's resume and snapshot like an approve's, and the
ruling speaks of any admitted output. A reject carrying no outputs is
unchanged.
- **Changeset** `.changeset/28658-decision-output-type-multiple.md`:
`patch` for `@objectstack/plugin-approvals`, the two declaration lines,
the ADR-0087 disposition `not-required (no-migration-prescription)`, and
a **BREAKING** paragraph naming which decisions are now refused.
- **Changesets**: add `.changeset/28658-console-decision-output-list.md`
(`@object-ui/app-shell`, `@object-ui/i18n` patch, Clause-② no
(narrowing), Metadata-protocol none, ADR-0087 `not-required
(no-migration-prescription)`, BREAKING only for the deleted key in
`TranslationKeys`). The plugin-approvals changeset now names the CSV
string the spec on main admitted for a list output, pending bare-key
snapshots, the ADR-0105 D9 organization note, and a remedy that
prescribes no rewrite.
The spec docblock and `content/docs/automation/approvals.mdx` text are
#29340 (spec lane, landed as #29371); this PR touches no
`packages/spec/**`.
## Tests
- `approval-service.test.ts`, new describe `typed decision outputs are
judged against their declaration (#28658)` (22 tests; the former `text
unchanged` test is replaced by four: text values free in the declared
shape; text `multiple: true` refuses a scalar; text, declared or bare
key, without `multiple` refuses a list; a text list refuses a repeated
entry): an unknown user id refused naming `outputs.next_reviewers` with
nothing recorded; slot address and email refused; scalar for `multiple:
true`; list for absent and for `false` `multiple`; a duplicated entry; a
non-text entry; two bad outputs named in one refusal; a valid list
recorded and handed on; position by machine name (held name passes, a
`sys_position` id refused, a name held only in another organization
refused); department (active passes; unknown, inactive, other
organization refused); team (row passes; unknown and other organization
refused); a reject with a bad output refused, one with none goes
through; a mid-tally co-sign vote judged before it is counted; `text`
unchanged; a blank optional picker output not judged.
- `approval-node.test.ts`, new test: the showcase dynamic-approval shape
(typed required `user` list at stage 1, `expression` approver with
`onEmptyApprovers: 'fail'` at stage 2) through the real automation
engine. A wrong id, a scalar and a duplicate are refused with stage 1
still pending, no decision row and the run still suspended; a valid pick
opens stage 2 on the picked users and the run completes.
- `approval-service.test.ts`, two more tests in that describe: "the
Console's decide body passes" (user list, text tags list, untyped tags
list, bare key with a literal comma, all recorded and handed on) and
"the comma string the Console used to send for a list output is
refused".
- `ui/packages/app-shell/src/utils/decisionOutputParams.test.ts`, new
describe "a free-value output is sent in its declared shape
(objectstack#28658)" (6 tests: text, untyped and degraded-position lists
reach the widget as `tags`; a bare key and `multiple: false` stay
`text`; the fold keeps a list a list and a comma literal). Two help-text
expectations there and one in `DeclaredActionsBar.test.tsx` move from
`helpMultiValue` to `help` (forced: the key is retired).
-
`ui/packages/app-shell/src/views/ActionParamDialog.decisionOutputList-28658.test.tsx`:
the real dialog, fed the synthesized params through
`resolveActionParams`, collects `' alpha '`, `'alpha'`, a blank and
`'beta'` by Enter and `'gamma'` by blur, and resolves `{
'outputs.labels': ['alpha', 'beta', 'gamma'], 'outputs.note': 'a, b' }`,
folded to the same list.
- **Forced test edits** (the first two because the old values never
named anything; the last two because a bare key that was sent a list now
declares `multiple: true`):
- `required outputs: a filled approve goes through…` sent `type:
'position'` values `pos_1` / `pos_2` that no one holds. It now seeds two
`sys_user_position` holders under those names.
- `required outputs: every approver of a multi-approver node must supply
them` sent `type: 'user'` values `u7` / `u8`, which are not accounts in
that double. It now adds the two accounts.
- `approval-service.test.ts` \"decision outputs: accepted keys return
from decideNode and snapshot as __decisionOutputs\": `next_reviewers` is
declared `{ key: 'next_reviewers', multiple: true }`.
- `approval-node.test.ts` \"decide outputs feed the NEXT approval node
via vars.NODE.KEY (#3447 P2)\": the same change.
## Acceptance notes
- **Refusal code.** Seat answer Q1 = A: `400 VALIDATION_FAILED`, the
door's one validation code (the ruling's `VALIDATION_ERROR` spelling was
a slip); the spec text card #29340 carries the same spelling. Q2 and Q3
= A, as delivered.
- **Organization.** The department, team and position checks resolve in
the request's organization. A downstream `expression` approver that
declares `organization` (ADR-0105 D9) resolves in its directory
organization instead, which the decision cannot know. Raised in
`open_questions`.
- **Position = held.** A position name nobody in the organization holds
is refused, since the only position resolver the `expression` approver
has is the holder expansion; no `sys_position` registry lookup exists on
this path, and the card says not to write a second one.
## Verification
Final head `c7ca5d18f` (`a913892b7` plus a merge of `origin/main`
including spec #29371, plus changeset wording): `dispatch-gates` 67/67
derived exit 0 (`--ran`: 67 run, 0 NOT-MEASURED); full dogfood 3277
passed / 0 failed; plugin-approvals 1404 passed with tsc green; ui
app-shell 18804 passed; ui i18n 1443 passed; app-shell and i18n
type-checks, eslint on the touched files, and the i18n keys, drift,
dead-keys and eager-locale-catalogues checks exit 0. The older readings
below are kept for the record: they were taken at head `59bdcb56` unless
they say otherwise, and at round-2 head `3eb0a962` (seat answer Q4 = B)
the derived gates read 63/63 exit 0, plugin-approvals 1402/1402 with
typecheck green, and the full dogfood battery 3243 passed / 0 failed.
<details><summary>Ablations: 16 legs, each red, each restored
byte-identical</summary>
The eleven unit legs ran at `0b23046c` and the end-to-end leg at
`665d0599`; `approval-service.ts` is byte-identical at `59bdcb56` (the
merge of `main` touched no `plugin-approvals` file). Each leg mutated
`approval-service.ts` through `scripts/ablation-replace.mjs` (anchor hit
x1, restore proved by blob == HEAD and an empty `git diff HEAD`), ran
the typed-output block (`vitest run src/approval-service.test.ts -t
typed`, 19 tests), and was restored. The tests import the service from
`src`, so no build leg applies.
| Leg | Mutation | Red |
|:--|:--|:--|
| call | the `assertDecisionOutputsMatchDeclarations` call removed | 14
failed |
| approve_only | judged on approve only | 1 failed: the reject pin |
| scalar_for_list | the `multiple: true` list check off | 2 failed |
| list_for_scalar | the single-value check off | 2 failed |
| duplicates | the repeated-entry check off | 1 failed |
| user_lookup | the user record lookup answers "resolves" | 4 failed |
| position_by_name | the holder expansion answers "held" | 2 failed |
| user_not_address | the slot-address / email guard removed | 1 failed |
| department | the department check answers "live" | 1 failed |
| team_missing | a missing team row passes | 1 failed |
| team_foreign | another organization's team passes | 1 failed |
| end to end | the call removed, `approval-node.test.ts -t "typed user
output"` | 1 failed, quoted below |
| text_skipped (at `3eb0a962`) | text outputs skipped as before | 3
failed |
| text_duplicates (at `3eb0a962`) | the text repeat check off | 1 failed
|
| console_tags (at `a913892b7`) | `type: 'tags'` → `'text'` in
`decisionOutputParams.ts` | 4 failed, the dialog test quoting `expected
{ 'outputs.labels': 'gamma', …(1) } to deeply equal { 'outputs.labels':
[ …(3) ], …(1) }` |
| console_csv (at `a913892b7`) | the scalar-for-list check off | 1
failed: "the comma string the Console used to send … is refused"
(`Error: expected a refusal`) |
The end-to-end leg reproduces the card's defect exactly:
```text
AssertionError: expected [Function] to throw error matching /^VALIDATION_FAILED: `outputs\.next_re…/ but got 'RESUME_FAILED: the approve decision w…'
```
</details>
<details><summary>Tests and gates</summary>
- `pnpm --filter @objectstack/plugin-approvals test` → Test Files 107
passed (107), Tests 1399 passed (1399); `typecheck` (tsc, scripts
tsconfig, `check:test-typecheck` OK: 8 files / 321 errors held in the
ledger, unchanged) → lock VERDICT command-exit 0.
- Full dogfood (`Clause-②` carries `(narrowing)`): closure build `turbo
run build --filter='@objectstack/dogfood^...'` then `pnpm --filter
@objectstack/dogfood test` → Test Files 393 passed, 3 skipped (396);
Tests 3243 passed, 28 skipped (3271); lock VERDICT command-exit 0.
- Lint: root eslint `--no-inline-config` over
`packages/plugins/plugin-approvals` → exit 0. `check:nul-bytes` → exit
0.
- `dispatch-gates --commands` derived 63 commands; all 63 recorded exit
0. Three first answered exit 3 (prerequisites, nothing measured) and
were re-run green: `check-plugin-teardown-shape --self-test` after
fetching its positive-control commit, `check:type-check-debt` and
`check:dual-build-cjs-loads` after the closure builds. `dispatch-gates
--ran` → "63 derived famil(ies) accounted for — 63 run, 0 NOT-MEASURED".
- CI families (required contexts) are read from this PR's check runs,
not re-run locally.
</details>
---
_Generated by [Claude
Code](https://claude.ai/code/session_01ADHkmPp7x4FxopdKR7keh7)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent b16ee8b commit d3175d5
19 files changed
Lines changed: 734 additions & 41 deletions
File tree
- .changeset
- packages/plugins/plugin-approvals/src
- ui/packages
- app-shell/src
- utils
- views
- __tests__
- i18n/src/locales
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
265 | 265 | | |
266 | 266 | | |
267 | 267 | | |
268 | | - | |
| 268 | + | |
| 269 | + | |
269 | 270 | | |
270 | 271 | | |
271 | 272 | | |
| |||
311 | 312 | | |
312 | 313 | | |
313 | 314 | | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
314 | 380 | | |
315 | 381 | | |
316 | 382 | | |
| |||
0 commit comments