Repository navigation
Commit c9c555a
fix(plugin-approvals)!: retire the role: position-address arm and write the canonical fallback slot literal (ADR-0090 D3) (#21770)
Fixes #21387
Clause-②: no (narrowing)
Executes ruling A2 (record 5979854058, director batch #278, maintainer
「同意」) as one PR. Draft. The ADR-0087 disposition step runs in the `Check
Changeset` job, which is NOT one of the seven required contexts. It read
red at `022bb0fa` because the honest disposition needed a ledger entry
outside the card's first surface. Patch round 1 (below, surface revision
2 in seat verdict 5982629646) adds that entry, and the step is now
green: `Check Changeset` read success on `3f4333ef`, and
`check-adr-0087-registration --base origin/main` exits 0 at `24f5c5a9`.
The patch reaches `packages/spec/src/**`, so an at-tier contract review
PASS is owed before landing. The review of `24f5c5a9` failed (record
5983490043) on the entry's step and the changeset's package list; patch
round 2 (below) fixes both, and a fresh review is owed on `d0a53cc6`.
## What changes
- **The equivalence.** `role:` comes out of `POSITION_ADDRESS_PREFIXES`
(`packages/plugins/plugin-approvals/src/approver-address.ts`).
`position:POSITION` is now the one spelling of a position address. Every
reader already takes its addresses from this one module: `resolveActor`,
the "My Pending" filter (`approverRequestIds`), the participant gate
(`visibleRequestIds`), `can_act` (`attachViewers`), and every decision's
slot test (`takenSlot` / `heldSlot`). So the prefix is the only line of
logic that changes on the reader side. No `named === role:...` compare
existed on `main`: `resolveActor` already read `positionAddresses`.
- **The writer.** `resolveApproverSpec`'s empty-lookup fallback returned
`${a.type}:${a.value}`, which is the AUTHORED type. It now returns
`${type}:${a.value}`, where `type` is
`canonicalApproverType(String(a.type))`. A flow that still authors the
deprecated `{ type: 'role', value: X }` and whose membership-tier lookup
finds no one now opens an `org_membership_level:X` slot, never `role:X`.
`DEPRECATED_APPROVER_TYPES` holds only `role`, so no other type's
literal moves. No stored slot is rewritten, there is no alias window,
and the spec's `ApproverType` `role` alias is untouched.
- **Comments.** Every comment in `approver-address.ts` and
`approval-service.ts` that called `role:POSITION` a live spelling is
rewritten. The two exemplar comments (remind fan-out, display-name
lookup) now use `position:` as their example of a `type:value` literal.
- **Bounded in-place fix, same defect class.**
`sys-approval-approver.object.ts` named `role:` as the example slot
literal in its docblock and in the `approver` field's `description`.
Both now say `position:`. All four conditions hold: it is the same class
(role: called a live slot spelling); the fix is mechanical; the dispatch
reported no claim on `plugin-approvals`; and it is in the same package
and gate family. `git grep` finds the description string in no generated
artifact or translation bundle. The claim's file surface did not list
this file.
- **Docs.** `content/docs/automation/approvals.mdx` had four sentences
that said a position matches, acts or takes a slot "under either
spelling". They now name `position:` as the one address. The
deprecated-type callout states which slot the fallback leaves and gives
the one-line fix. The admin-override callout names both request classes.
The retired prefix is named in words, because `check:role-word` holds
that page at its one existing occurrence.
- **Changeset.** `.changeset/21387-retire-role-arm.md` is `minor`, with
the `Clause-②: no (narrowing)` line, the FROM `role:POSITION` → TO
`position:POSITION` line, the author's one-line fix `{ type: 'position',
value: ... }`, and the admin's one-line handling for both request
classes.
## Pins
In `plugin-approvals`. Every turned pin keeps its `expect`s, flipped to
the retired answer.
- **The card's three pins.** These are in the new block "the role: arm
retired; the override decides what it leaves (ADR-0090 D3)" in
`approval-service.test.ts`, plus the enumeration in
`approver-address-readers.test.ts`.
1. A `position:POSITION` slot is listed, counted, `can_act: true`, and
decided by its holder with the default actor. The decision records
`actor_id` as the holder, `acted_as` as the slot, and `via_override:
false`.
2. `role:POSITION` is no longer an address of that slot. It is not
listed and not counted for the holder, not matched for SYS, and a named
actor `role:POSITION` is refused (`FORBIDDEN: cannot act as`). The slot
and the action log are untouched.
3. **Enumeration.** No `plugin-approvals` writer produces a `role:`
slot, and no reader compares one. There are two scans over every
non-test `.ts` under `src/`, `approver-address.ts` included:
- the string literals, read from the syntax tree, so comments are masked
by construction. No literal may carry a `role:` prefix. The positive
control is the scan finding `position:` in `approver-address.ts`.
- every template of the shape `${X}:${Y}`, classified in a ledger. None
may interpolate an `X.type` property (the authored type), and
`resolveApproverSpec`'s writer is pinned by name to
`canonicalApproverType(...)`.
A planted-source case shows both scans catch what they name, and that a
comment does not trip them.
- **The admin rescue, permanent.** This is the measurement from
`5979222082`. Two request classes run, each against three admin doors:
- The two classes: a stored 15.x-era `role:POSITION` slot, planted in
the CSV and then indexed by the plugin's own `rebuildApproverIndex`; and
a new `{ type: 'role', value: POSITION }` request whose tier lookup
finds no one.
- The three doors: `admin_full_access`, `PLATFORM_ADMIN` posture, and a
same-org `TENANT_ADMIN` posture.
Each admin decides through `decide()` with `actorId` set to the caller,
which is what `POST .../approve` passes. The result is `finalized`,
`approved` and `resumed: true`, with the resume signal `branchLabel:
approve`, and the action records `actor_id` as the admin, `acted_as:
null` and `via_override: true`. A reassign to the holder, after which
the holder decides with the default actor, also rescues both classes.
The holder of the same-named position is refused on both classes: the
request is not listed, not visible, `can_act` is false, and the default
actor and `role:POSITION` are both refused. **Ruling A's stop condition
is not triggered:** the rescue decides both classes on the post-change
code.
- **Turned pins.** `approver-address.test.ts`: the prefix list, the
fold, the colon split, the acting addresses, the named actor and
`heldSlot`, with `role:` added to the "equivalent only to itself" table.
`approval-service.test.ts`: the #21350 block (list under `role:` is
empty; a stored `role:` slot is invisible to the holder; the SYS filter
miss), the `resolveActor` oracle (the pre-extraction predicate minus its
`role:` arm; admitted count pinned at exactly 5), and the #21379
decision block (the holder naming `role:` is refused, and a stored
`role:` slot is refused to the default actor). The fallback pin "keeps
its legacy literal" is now "falls back to the canonical literal, in the
slate and in the index": `['org_membership_level:admin']`, plus the
index row.
- **Dogfood (cross-lane `domain:cli`, declared on #6024 `5981995488`).**
In `my-pending-position-address`, the reviewer and submitter rows under
`role:` now assert no rows, `role:` joins the admin's "must not fold"
list, and the reviewer approving as `role:POSITION` asserts `[403,
'FORBIDDEN']`. In `position-address-readers`, the holder's `can_act` row
and the bystander row ask under `position:`, the holder under `role:`
lists nothing, and approving as `role:POSITION` asserts `[403,
'FORBIDDEN']` with nothing recorded. The holder then decides the same
request under `position:`. Every `position:` row is unchanged.
## Ablation (one-shot, not kept)
Both legs use `scripts/ablation-replace.mjs` in wrap mode on committed
HEAD `bce0f4a2`, under the verify lock. Each leg proves the anchor count
1 → 0, the replacement count 0 → 1 and the blob change, and each restore
proves the blob equals HEAD and `git diff HEAD` is empty. Both suites
resolve `plugin-approvals` SOURCE, through relative imports in the
package and the dogfood vitest alias to `src/index.ts`, so there is no
dist leg.
1. **The `role:` arm put back** (`['position:']` → `['position:',
'role:']`, blob `c5691d3d` → `8afd6cd0`): the unit suites went red, 3
files, 16 failed / 347 passed. The dogfood went red, 2 of 2 failed. For
example, the readers scan reported `approver-address.ts:64 · "role:"`,
and the dogfood reported `reviewer under 'role:my_pending_reviewer':
expected [ {...} ] to deeply equal []`. Restored to `c5691d3d`.
2. **The authored fallback put back** (`${type}` → `${a.type}`, blob
`3f5ec962` → `27d7355e`): the unit suites had 4 failed / 359 passed. The
template scan reported `approval-service.ts · resolveApproverSpec ·
${a.type}:${a.value}` as unclassified, and the fallback pin got
`expected [ 'role:admin' ] to deeply equal [
'org_membership_level:admin' ]`. The dogfood stayed green, as expected:
both fixtures route `type: 'position'`. Restored to `3f5ec962`.
## Tests and gates
All of these were run at `022bb0fa` (the round-0 head; the
plugin-approvals code half has not changed since):
- `plugin-approvals`, full: 59 files, 891 passed.
- The two dogfood files: 2 passed.
- `pnpm --filter @objectstack/plugin-approvals typecheck`: tsc, the
scripts project and `check:test-typecheck` all OK, with no new pinned
signature.
- `dispatch-gates --commands`: 95 derived commands. 94 exit 0 and 1
exits 1, `check-adr-0087-registration --base origin/main` (below). The
`--ran` reconciliation reads 95 derived, 95 run, 0 NOT-MEASURED, 0
UNRUN.
- Narrowed eslint over the 8 changed `.ts` files: 8 files, 0 errors, 0
warnings (`--format json`). All 8 are in the config's population
(`--print-config` resolves each). Type-aware linting is not enabled
anywhere in `eslint.config.mjs` (no `parserOptions.project`, as its own
header states), so this diff cannot move any untouched file's verdict.
## The ADR-0087 disposition (resolved)
_Resolved by the seat: verdict 5982629646 answered A, patch round 1
added the entry, and patch round 2 moved it to step 18. The round-0
analysis is kept below as written._
The changeset is declared-breaking: it has the `(narrowing)` arm and the
`!`. It therefore needs exactly one ADR-0087 marker. The gate reads the
ruling-mandated FROM → TO and the author's fix as a migration
prescription (`--list` shows `prescription=yes`). That refuses
`not-required (no-migration-prescription)` as a self-contradiction, and
`runtime-interface-only` inherits the same refusal. `unpublished` is
false, and no existing ledger id covers approval slot addresses, so
`already-registered` would be a dishonest claim. The honest disposition
is `registered` with a new D3 semantic ledger entry under
`packages/spec/src/migrations/entries/semantic/`. This is the pattern of
the ADR-0090 runtime faces `actor-user-roles-to-positions` and
`action-session-roles-to-positions`. That entry is a `packages/spec`
edit, and this card's claim and dispatch declare ⛔ no `packages/spec`
edit, so the marker is left absent rather than written false. The
question is in the dev report on the card.
## Acceptance notes
- **Pre-snapshot multi-approver requests.** A `unanimous`, `quorum` or
`per_group` request opened before the 17.0 open-time snapshot
re-resolves its slate at every tally (`decideNode`'s back-compat path).
If such a request has a deprecated `role` approver whose lookup was
empty, the re-resolved slot now spells `org_membership_level:X` while
the stored one is `role:X`, and the tally writes the re-resolved slate
back. Decidability is the same: only an admin can decide it either way.
One edge is new: an approval a position holder recorded under `role:X`
before the upgrade no longer satisfies the re-resolved slot, so the
request needs the admin override to finalize. This falls within the
accepted class of stored 15.x-era `role:` slots. It is the pre-existing
re-resolution, not a rewrite this PR adds. carrier: this PR, noted, not
filed.
- **Free-text slots are not covered.** A reassign's `to` and a `user`
approver's `value` are free text. A caller or author can still name the
literal `role:X` there, which makes a slot nobody takes. This is the
same pre-existing class as any `type:value` literal handed over there,
and the admin rescue covers it. The enumeration pin scans code literals
and templates, not data. carrier: none, noted, not filed.
- **Comment drift outside this surface.** These comments in other
packages now describe the old spelling:
- the `packages/lint/src/validate-approval-approvers.ts` docblock (about
:11) says the deprecated type "falls back to the `role:sales_manager`
literal". The literal is now `org_membership_level:sales_manager`.
- a `packages/rest/src/rest-server.ts` comment (about :12865) lists
`role:` among the console's identities.
- `packages/spec/src/contracts/approval-service.ts` (about :532) names
`role:` as a stored `acted_as` spelling. That is still true for
historical rows.
carrier: whoever next touches each file. Noted, not filed.
- **Unreleased changesets disagree.**
`.changeset/21350-my-pending-position-address.md` and
`.changeset/21379-position-address-readers.md` are still unreleased, and
they describe `role:` as a second spelling. If they ship in the same
version as this changeset, both statements reach the CHANGELOG, and this
one's FROM → TO is the later fact. They are other cards' release inputs,
so they are not edited here.
- **How the refusals are asserted.** At the service layer the refusal is
asserted by the service's own code prefix (`FORBIDDEN:`), because the
service throws plain errors and the REST door assigns the status. The
`[status, code]` pair `[403, 'FORBIDDEN']` is asserted at the REST door
in both dogfood pins. A `role:` ask on the list door is a filter miss:
200 with no rows, not an error. Those rows assert the 200 and the empty
set.
## Patch round 1 (head 24f5c5a)
_Appended by the seat (`domain:services#1`,
`session_011K3zqE8Pv1Evw5hc8tZCnN`) from the dev's patch-round report
`5983095337`, after seat verdict `5982629646`. The opening paragraph
above was corrected in the same act._
Seat verdict 5982629646 answered the ADR-0087 question with A (surface
revision 2). The cross-lane declaration is on #6017 (5982635753).
- **One D3 semantic ledger entry**,
`approval-position-address-role-retired`, in
`packages/spec/src/migrations/entries/semantic/17.approval-position-address-role-retired.ts`
_(moved to `18.approval-position-address-role-retired.ts` in patch round
2)_. It is modelled on `actor-user-roles-to-positions` and
`action-session-roles-to-positions`.
- Surface: the approvals position address `role:POSITION`, meaning the
`approverId` filter, a decision's `actorId`, and a stored
`pending_approvers` slot.
- Replacement: `position:POSITION`. An author's `{ type: 'role', value:
POSITION }` becomes `{ type: 'position', value: POSITION }`.
- Reason: the ADR-0090 D3 retirement; the deprecated `role` type's
fallback now writing `org_membership_level:VALUE`; the two request
classes that become admin-decided; no stored-slot rewrite; and why this
is a D3 entry and not D2 (the address is runtime data, and which type
the author meant is the author's judgment).
- Acceptance criteria: what an upgrader verifies.
- **Generated with the repo's own tooling, never by hand:**
`gen:migration-registry` (the `registry.ts` region), `gen:spec-changes`
(`packages/spec/spec-changes.json`) and `gen:upgrade-guide`
(`docs/protocol-upgrade-guide.md`). `check:migration-registry`,
`check:spec-changes` and `check:upgrade-guide` exit 0. _(After patch
round 2, `spec-changes.json` and the upgrade guide are back to `main`'s
bytes: neither projects step 18.)_
- **The changeset's ADR-0087 marker** now reads `registered
approval-position-address-role-retired`, spelled from the gate's own
output. `check-adr-0087-registration --base origin/main` exits 0.
- **Nothing else changes in `packages/spec`:** no Zod schema, no
`ApproverType` alias, no contract docblock, no stored slot. _(Superseded
in patch round 2: the changeset now also names `"@objectstack/spec":
patch`.)_ The entry reaches 17.x upgraders through `os migrate meta
--from 17`, which composes step 18; measured on the built spec at
`d0a53cc6`, `composeMigrationChain(17, 18)` lists
`approval-position-address-role-retired`. `spec-changes.json` and
`docs/protocol-upgrade-guide.md` project only up to `PROTOCOL_MAJOR`
(17), so after patch round 2 they carry no row for it and are
byte-identical to `main`.
- **Hot file.** `origin/main` was merged at `3f4333ef`. #21764 landed
after that, and `24f5c5a9` merges it: a clean merge that keeps both
sides. The three ledger checks, `check-adr-0087-registration` and
`check-changeset-no-major` exit 0 at `24f5c5a9`.
- **Tests at `3f4333ef`:**
- the spec build;
- the 58 spec test files that read the ledger: 2219 passed;
- the CLI unit-tier ledger readers: 22 passed;
- driver-sql `sql-driver-query-signature`: 15 passed.
Declared to CI: the full spec suite, which runs past this container's
10-minute foreground cap, and the CLI integration-tier ledger readers.
- **Gates at `3f4333ef`:** `dispatch-gates` derives 117 commands (22 new
spec families) and all 117 exit 0. `--ran` reads 117 derived, 117 run, 0
NOT-MEASURED, 0 UNRUN. CI on `3f4333ef`: 35 of 35 check runs completed,
`Check Changeset` success.
- **The Clause-② gate's path limb now hits `packages/spec/src/**`**, so
a contract review is owed. The seat runs it.
## Patch round 2 (head d0a53cc)
_Appended by the seat (`domain:services#1`,
`session_011K3zqE8Pv1Evw5hc8tZCnN`) from the dev's patch-round report
`5983807880`. Stale statements above were annotated in the same act._
The contract review of `24f5c5a9` (record 5983490043) returned FAIL on
two defects. The seat adopted it in 5983502467 and ordered this round.
Cross-lane addenda: #6017 5983506965 and #6024 5983511854.
- **The ledger entry moves to step 18.** `git mv` renames
`entries/semantic/17.approval-position-address-role-retired.ts` to
`18.approval-position-address-role-retired.ts`. The id and the content
are unchanged; the prose names no step-17 boundary, so it needed no
edit.
- The `step18` docblock says that a narrowing which lands after the
v17.0.0 cut is told "where `migrate meta` users are told, at the major
boundary where they look".
- `pnpm --filter @objectstack/spec gen:migration-registry` regenerated
`registry.ts`, and the entry now sits in the `step18` block. Against
`origin/main` (`025008ae`), `registry.ts` reads `1 file changed, 59
insertions(+)`.
- **The projections, regenerated and measured, not predicted.**
`check:generated` named two stale artifacts, `spec-changes.json` and
`docs/protocol-upgrade-guide.md`, and `check:generated --fix`
regenerated them from a fresh spec build. Both are now **byte-identical
to `origin/main`**: `git diff --shortstat origin/main HEAD` prints
nothing for either, and both have left the PR's file list.
- Step 18 is projected by **neither** generator. Both loop up to and
including `PROTOCOL_MAJOR` (17): `build-spec-changes.ts:254` and
`build-upgrade-guide.ts:78`. Neither file carries any `18.*` id; for
example, `ui-action-group-menu-members-typed` appears 0 times in each.
- **The channel, measured on the built `packages/spec` dist.**
- `composeMigrationChain(17, 18)` lists the entry. That is the chain `os
migrate meta --from 17` composes, because `CHAIN_TERMINUS_MAJOR` =
max(`PROTOCOL_MAJOR`, `MIGRATION_MAJORS`) = 18 and `MIGRATION_MAJORS` =
[17, 18].
- `composeMigrationChain(16, 17)` no longer lists it.
- Control: the step-18 entry `ui-action-group-menu-members-typed` is
listed by `(17, 18)`.
- **The changeset now names `"@objectstack/spec": patch`** next to
`"@objectstack/plugin-approvals": minor`. Every other line is unchanged:
the `Clause-②: no (narrowing)` line, FROM → TO, the author's fix, the
admin handling, and the marker `registered
approval-position-address-role-retired`. `check-changeset-no-major
--base origin/main` exits 0 with `patch`. `check-adr-0087-registration
--base origin/main` exits 0: the id resolves at HEAD and is new in the
diff.
- **Bounded comment fix.** The header of
`packages/qa/dogfood/test/fixtures/my-pending-position-fixture.ts`
(about :10) no longer says "under both approver-address spellings". It
now says that `position:POSITION` lists the request and the retired
`role:POSITION` spelling (ADR-0090 D3) does not. The change is comment
only.
- Nothing else changed: no Zod schema, no `ApproverType` alias, no
contract docblock, no stored-slot rewrite, and no plugin-approvals code.
- **Hot file.** `origin/main` `025008ae` was merged at `d0a53cc6`. The
merge was clean, and main brought no change to `packages/spec`.
- **Gates at `d0a53cc6`:**
- `check:migration-registry`, `check:spec-changes`,
`check:upgrade-guide`, `check-adr-0087-registration --base origin/main`
and `check-changeset-no-major --base origin/main` all exit 0.
- `dispatch-gates` derives 116 commands. `check:generated` dropped out,
because no generated artifact is in the diff any more. All 116 exit 0,
and `--ran` reads 116 derived, 116 run, 0 NOT-MEASURED, 0 UNRUN.
- **Tests at `d0a53cc6`:**
- 57 of round 1's 58 ledger-reading spec test files: 2134 passed.
- The CLI unit-tier ledger readers: 22 passed.
- driver-sql `sql-driver-query-signature`: 15 passed.
- Declared to CI: `scripts/build-schemas-check-mode.test.ts` (it spawns
full schema builds and runs past the 10-minute foreground cap; it was
green in round 1's run at `3f4333ef`), the full spec suite, and the CLI
integration-tier ledger readers.
- The plugin-approvals code half is unchanged and reused from round 0.
- **A fresh at-tier contract review is owed on `d0a53cc6`.** The seat
runs it once CI on this head has settled; landing waits for its PASS.
---
_Generated by [Claude
Code](https://claude.ai/code/session_011K3zqE8Pv1Evw5hc8tZCnN)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 025008a commit c9c555a
13 files changed
Lines changed: 726 additions & 160 deletions
File tree
- .changeset
- content/docs/automation
- packages
- plugins/plugin-approvals/src
- qa/dogfood/test
- fixtures
- spec/src/migrations
- entries/semantic
| 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
64 | | - | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
65 | 68 | | |
66 | 69 | | |
67 | 70 | | |
| |||
417 | 420 | | |
418 | 421 | | |
419 | 422 | | |
420 | | - | |
421 | | - | |
422 | | - | |
423 | | - | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
424 | 428 | | |
425 | 429 | | |
426 | 430 | | |
| |||
429 | 433 | | |
430 | 434 | | |
431 | 435 | | |
432 | | - | |
| 436 | + | |
433 | 437 | | |
434 | 438 | | |
435 | 439 | | |
| |||
503 | 507 | | |
504 | 508 | | |
505 | 509 | | |
506 | | - | |
507 | | - | |
508 | | - | |
509 | | - | |
510 | | - | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
511 | 517 | | |
512 | 518 | | |
513 | 519 | | |
| |||
624 | 630 | | |
625 | 631 | | |
626 | 632 | | |
627 | | - | |
628 | | - | |
| 633 | + | |
| 634 | + | |
629 | 635 | | |
630 | 636 | | |
631 | 637 | | |
| |||
655 | 661 | | |
656 | 662 | | |
657 | 663 | | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
658 | 672 | | |
659 | 673 | | |
660 | 674 | | |
| |||
0 commit comments