Seeded reviews name the Dev Admin as reviewer instead of the legal staff

README.md

@@ -92,10 +92,10 @@ console does not list them; this table is where they are named.
 
 | Account | Sign in as | Position | What it holds in the demo |
 |---|---|---|---|
-| `Business Requester 1` · `2` · `3` | `requester1@example.com` · `requester2@example.com` · `requester3@example.com` | none — every member holds `clm_requester` | the 120 contracts' `owner_id`, dealt by counterparty — 43 / 43 / 34 · and the 143 delivery and reporting obligations under them — 50 / 53 / 40 |
-| `Legal Counsel 1` · `2` | `counsel1@example.com` · `counsel2@example.com` | `clm_legal_counsel` | receives contracts on submission (round-robin by open load) and owns review · the `legal_owner` of every contract legal has accepted — 34 / 34 · all 56 compliance obligations — 29 / 27 · the `decided_by` of all 17 decided deviations — 10 / 7 |
-| `Head of Legal` | `legal.head@example.com` | `clm_legal_head` | rung 2 of the approval ladder whenever `route_legal_head` is stamped |
-| `Finance Controller` | `finance@example.com` | `clm_finance_controller` | rung 2/3, reads every contract with `route_finance`, and the only position that may edit bank details |
+| `Business Requester 1` · `2` · `3` | `requester1@example.com` · `requester2@example.com` · `requester3@example.com` | none — every member holds `clm_requester` | the 120 contracts' `owner_id`, dealt by counterparty — 43 / 43 / 34 · and the 143 delivery and reporting obligations under them — 50 / 53 / 40 · the 2 business reviews — 0 / 1 / 1 |
+| `Legal Counsel 1` · `2` | `counsel1@example.com` · `counsel2@example.com` | `clm_legal_counsel` | receives contracts on submission (round-robin by open load) and owns review · the `legal_owner` of every contract legal has accepted — 34 / 34 · all 56 compliance obligations — 29 / 27 · the `decided_by` of all 17 decided deviations — 10 / 7 · the reviewer of their contracts' 34 legal reviews — 16 / 18, the 7 still pending among them — 2 / 5 |
+| `Head of Legal` | `legal.head@example.com` | `clm_legal_head` | rung 2 of the approval ladder whenever `route_legal_head` is stamped · the reviewer of 8 reviews: the legal review of the contract handed back to the queue, and the 7 finance reviews on contracts not yet approved |
+| `Finance Controller` | `finance@example.com` | `clm_finance_controller` | rung 2/3, reads every contract with `route_finance`, and the only position that may edit bank details · the reviewer of the 16 finance reviews on approved-or-later contracts |
 | `Executive` | `executive@example.com` | `clm_executive` | rung 4, and reads every contract with `route_executive` |
 | `General Manager` | `gm@example.com` | `clm_general_manager` | rung 5, and reads every contract with `route_gm` |
 | `Records Manager` | `records@example.com` | `clm_records_manager` | execution formalities, the executed copy and the archive number |
@@ -146,9 +146,10 @@ instalment finance has marked paid. Measured after one run of every daily job
 plus those four desk actions: the re-run refused no row and dropped none.
 
 Three limits. Any other column you edited, such as a summary or a reassigned
-legal owner, goes back to the fixture's value. A deviation or signature round
-you decided is not covered yet: the re-run puts its fixture status back, and the
-state machine may refuse that. The lifecycle columns also keep the dates they
+legal owner or a review's reviewer, goes back to the fixture's value. A
+deviation, signature round or review you decided is not covered yet: the re-run
+puts its fixture status or decision back, and for a deviation or a signature
+round the state machine may refuse that. The lifecycle columns also keep the dates they
 were stamped with, while the fixture's own dates (start, end, due and planned
 dates) move forward on each re-run. To get back to the corpus exactly as
 shipped, start from a clean database: `rm -rf .objectstack/data && pnpm demo`.
@@ -169,10 +170,13 @@ the dev admin cannot open, and as a business owner or an obligation's owner it
 fills the setup account's 我的合同 instead of the requester's — and its
 reminders go to somebody who is not going to act on them.
 
-`clm_review.reviewer` is the one reference that still names the dev admin. It is
-`required: true`, so a name that resolves to nothing takes the row with it, and
-the fixture names the account `pnpm demo` makes sure of before anything is
-seeded. Reassign it if you want the reviews on a lawyer's desk.
+The 60 reviews are dealt to the desk whose stage they are. A legal review is the
+contract's lawyer's, or the head of legal's on the one contract handed back to
+the queue. A finance review is the finance controller's once its contract is
+approved, and the head of legal's before that, because finance edits a contract
+(and so its reviews) only from `approved` onward. A business review is the
+requester's who launched the contract. A compliance review would be the head of
+legal's; the demo deals none. `src/data/keys.ts` (`reviewerOf`) says why each.
 
 ### Rung 1 of the approval ladder is a manager
 

scripts/demo.mjs

@@ -18,28 +18,26 @@
 // write landed at 2026-09-09T14:26:53.833Z and the account was created at
 // 2026-09-09T14:26:57.396Z, 3.563s later. This IS the interval — it is
 // measured here and nowhere else, so there is one place to be wrong.
-// Two things follow, and both are silent:
 //
-//   1. `clm_review.reviewer` is `required: true` and resolves against
-//      `sys_user.name`. On a first boot the name matches nothing, the loader
-//      defers the column to pass 2, and the engine refuses the row:
-//      "Failed to write clm_review record #1: Reviewer is required". All 60
-//      reviews are lost and the seed still reports success for everything else.
-//   2. Rows written before the organization exists carry `organization_id:
-//      null`, which is not what an org-scoped deployment expects of its data.
+// What follows from it now is that rows written before the organization
+// exists carry `organization_id: null`, which is not what an org-scoped
+// deployment expects of its data. Silently: the seed reports success.
+//
+// It used to be a second thing too. `clm_review.reviewer` is `required: true`
+// and resolved against the dev admin's name, which matched nothing on a first
+// boot, so all 60 reviews were refused while the seed reported success for
+// everything else. The reviews now name the demo staff (issue #186), which
+// the platform creates on the demo boot before that boot's seed runs (issue
+// #28), so no row depends on the admin's NAME any more and this script no
+// longer checks it. See `reviewerOf` in `src/data/keys.ts`.
 //
 // DESIGN.md §10 forbids seeding users, so the fixture cannot supply the
 // account itself. What this script does instead is sequence the two boots the
 // evaluator would otherwise have to know about: once with the demo OFF, which
 // mints the admin and founds the organization, then again with it ON. The
 // first boot is quiet. The handover between them is VERIFIED — a real sign-in
 // against the priming server — because the failure being guarded against is a
-// seeded database with sixty missing reviews, which looks exactly like a
-// working one.
-//
-// Measured on the same database: 13 ok / 4 errors on the first boot became
-// 14 ok / 2 errors on the second, the difference being the review row that
-// could finally resolve its reviewer.
+// seeded database that looks exactly like a working one.
 //
 // ── Why both boots pass `--compile` ────────────────────────────────────────
 //
@@ -68,31 +66,6 @@ const SEED_LOCALE_ENV_VAR = 'OS_SEED_LOCALE';
 // reminders reach the inbox; `pnpm dev` keeps the default (issue #82, ruling C).
 const SCHEDULED_WORK_ENV_VAR = 'OS_AUTOMATION_SCHEDULED_WORK_ENABLED';
 
-/**
- * The account name the fixture's `clm_review.reviewer` resolves against.
- *
- * It is the ONLY reference this boot has to satisfy, and the reason is the
- * `required: true` on that one column: a name resolving to nothing costs the
- * whole row, so it must name an account that exists while the seed runs, and
- * on a first boot the dev admin is the only account there is.
- *
- * NOT `clm_contract.owner_id`, NOT `clm_contract.legal_owner`, NOT
- * `clm_obligation.owner` and NOT `clm_deviation.decided_by`. All four name
- * accounts of DESIGN.md §10's persona mix — three business requesters and two
- * legal counsel — which no seed may create and this script does not mint
- * either: the stack DECLARES them in `demoStaff` (objectstack.config.ts) and
- * the platform creates them on the demo boot, before that boot's seed runs
- * (issue #28, ruling D). See `src/data/keys.ts`.
- *
- * ⚠️ This MIRRORS `DEMO_USER` in `src/data/keys.ts`. They have to agree, and
- * they cannot be one constant: this file is plain `.mjs` that runs before
- * anything is compiled, and that one is TypeScript baked into the artifact.
- * The check below is what keeps a drift from being silent — a fixture pointed
- * at a name no account carries loses every row that references it, and the
- * seed reports success for the rest.
- */
-const DEMO_USER = 'Dev Admin';
-
 // The same credentials and env overrides `@objectstack/plugin-auth` itself
 // reads, so an operator who has changed them is not silently probed for an
 // account that was never going to exist.
@@ -130,8 +103,8 @@ const freePort = () =>
  * The handover check: can you actually log in yet?
  *
  * Not "did the server start" and not "did a line appear in the log" — the
- * whole point of the priming boot is a loginable account whose name the
- * fixture can resolve, so that is what is asserted. `localhost` (not
+ * whole point of the priming boot is a loginable account in a founded
+ * organization, so that is what is asserted. `localhost` (not
  * `127.0.0.1`) with a matching `Origin`: dev trusts `http://localhost:*` and
  * better-auth rejects anything else with 403 INVALID_ORIGIN.
  */
@@ -144,13 +117,10 @@ const signIn = async (port) => {
       body: JSON.stringify({ email: ADMIN_EMAIL, password: ADMIN_PASSWORD }),
       signal: AbortSignal.timeout(5_000),
     });
-    if (response.status !== 200) return null;
-    const body = await response.json().catch(() => ({}));
-    const user = body?.user ?? {};
-    return { name: typeof user.name === 'string' ? user.name : null };
+    return response.status === 200;
   } catch {
     // Not up yet, or up and not answering. Either way: not ready.
-    return null;
+    return false;
   }
 };
 
@@ -235,27 +205,10 @@ const primeAdminAccount = async () => {
         log,
       );
     }
-    const session = await signIn(port);
-    if (session) {
+    if (await signIn(port)) {
       // Stop the priming server FIRST: it is detached and holding the
       // database, so exiting around it would orphan a server nobody can see.
       await stop();
-      if (session.name !== DEMO_USER) {
-        fail(
-          `the account you log in as is named ${JSON.stringify(session.name)}, not ${JSON.stringify(DEMO_USER)}.`,
-          [
-            `The demo fixture resolves \`clm_review.reviewer\` against a \`sys_user\` named`,
-            `${JSON.stringify(DEMO_USER)}. Against any other name that reference resolves to`,
-            'nothing, and every review row is refused while the rest of the seed reports',
-            'success — so nothing was seeded and the database is exactly as it was.',
-            '',
-            'Start over on a clean database:',
-            '',
-            '    rm -rf .objectstack/data && pnpm demo',
-          ],
-          log,
-        );
-      }
       return;
     }
     await sleep(1_000);

src/data/keys.ts

@@ -1,7 +1,8 @@
 // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { STRINGS } from './demo-locale.js';
-import type { ObligationPlan, VersionPlan } from './plan-children.js';
+import { POST_APPROVAL_STATUSES } from '../sharing/_lifecycle.js';
+import type { ObligationPlan, ReviewPlan, VersionPlan } from './plan-children.js';
 import { CONTRACT_PLAN, type ContractPlan, type DeviationPlan } from './plan-contracts.js';
 import { assertUniqueKeys } from './_shared.js';
 
@@ -30,23 +31,14 @@ import { assertUniqueKeys } from './_shared.js';
 /**
  * The account the demo is driven as — the platform's dev admin.
  *
- * `clm_review.reviewer` is `required: true`, so a name resolving to nothing
- * costs the whole row, and the dev admin is the account `scripts/demo.mjs`
- * verifies exists before the demo boot seeds anything. Since #28 (PR #171) the
- * demo staff exist at seed time too (`demoStaff`, created by the platform
- * before the seed runs); whether `reviewer` should name a lawyer instead is
- * open and is not decided here.
- *
- * `clm_review.reviewer` is now the last REFERENCE that has to name it, and
- * the only one left that spells this constant at all.
- * `clm_contract.legal_owner` left for {@link LEGAL_OWNERS},
- * `clm_obligation.owner` for {@link obligationOwnerOf} and
- * `clm_deviation.decided_by` for {@link deviationDeciderOf} — all three
- * optional, so all three may name accounts that do not exist yet. Enumerated
- * rather than sampled: `grep -rn DEMO_USER src` over this tree returns the
- * declaration, this file's four cross-references, `reviewer`, and one thing
- * that is not a reference at all — the `our`-side entry of
- * `clm_signature.signers`, a JSON string that resolves against nothing.
+ * No REFERENCE names it any more. `clm_contract.legal_owner` left for
+ * {@link LEGAL_OWNERS}, `clm_obligation.owner` for {@link obligationOwnerOf},
+ * `clm_deviation.decided_by` for {@link deviationDeciderOf}, and the last one,
+ * the `required` `clm_review.reviewer`, for {@link reviewerOf} (#186).
+ * Enumerated rather than sampled: `grep -rn DEMO_USER src` over this tree
+ * returns the declaration, this file's cross-references, and one thing that is
+ * not a reference at all — the `our`-side entry of `clm_signature.signers`, a
+ * JSON string that resolves against nothing.
  */
 export const DEMO_USER = 'Dev Admin';
 
@@ -319,6 +311,91 @@ export const obligationOwnerOf = (obligation: ObligationPlan): string | null =>
 export const deviationDeciderOf = (deviation: DeviationPlan): string | null =>
   deviation.status === 'open' ? null : counselFor(CONTRACT_PLAN[deviation.contractIndex]!);
 
+/**
+ * The head of legal and the finance controller, as the ACCOUNT NAMES the
+ * loader resolves `clm_review.reviewer` against.
+ *
+ * ⚠️ These two MIRROR the `name` of their `demoStaff` entries in
+ * `objectstack.config.ts`, which spells them as literals rather than from
+ * here. `reviewer` is `required: true`, so a drift between the two is not a
+ * NULL but a lost row: the loader refuses every review that names an account
+ * nobody carries, and the seed reports success for the rest.
+ */
+export const LEGAL_HEAD = 'Head of Legal';
+export const FINANCE_CONTROLLER = 'Finance Controller';
+
+/**
+ * Who a review belongs to — `clm_review.reviewer`, the person who decides it
+ * (`review_decision_stamp` in `contract.hook.ts`: a review has no
+ * `decided_by`, its reviewer IS the decider).
+ *
+ * ## Why not {@link DEMO_USER}, which is what this column held before
+ *
+ * Measured on `main` @ `37ce10c`, clean database, one `pnpm demo:zh`: **all 60
+ * reviews named the dev admin** (`display_name` `Legal · Dev Admin` ×35,
+ * `Finance · Dev Admin` ×23, `Business · Dev Admin` ×2). The column was dealt
+ * that way while the dev admin was the only account at seed time and
+ * `reviewer` is `required`, so any other name cost the row. Since #28 (PR #171)
+ * the demo staff exist before the seed runs, so the constraint is gone and
+ * the argument {@link LEGAL_OWNERS} makes for `legal_owner` applies: the dev
+ * admin holds no `clm_*` set beyond the default, so a review on its desk is a
+ * review nobody who can act on it is named for.
+ *
+ * ## The rule: the desk §04 gives the stage to
+ *
+ *  - **legal** — the contract's lawyer, {@link legalOwnerOf}: the counsel who
+ *    accepted it into review is the one who reviews it. Where F2 assigned
+ *    nobody (the contract handed back to the queue), the head of legal, who
+ *    receives that queue's escalations.
+ *  - **finance** — the finance controller, whose `clm_finance` holds
+ *    `RCU（stage=finance）` on this object.
+ *  - **compliance** — the head of legal: compliance is legal's work (the
+ *    argument {@link obligationOwnerOf} makes for compliance obligations) and
+ *    a high-risk contract's compliance sign-off is the senior lawyer's.
+ *    The plan deals no compliance review today; the rule is stated so the
+ *    first one has an owner.
+ *  - **business** — the requester who launched the contract, {@link ownerOf}:
+ *    the business review is the launching desk's own sign-off.
+ *
+ * ## Where the rule bends, measured on 18.2.0
+ *
+ * A finance review on a contract that has not reached `approved` goes to the
+ * head of legal, not the finance controller. `clm_review` is
+ * `controlled_by_parent`, so writing one needs EDIT on its contract, and
+ * finance edits contracts only from `approved` onward
+ * (`contract_finance_post_approval`); before that it READS the contracts the
+ * matrix routed to it (`contract_finance_routed`) and no more. Measured: the
+ * finance controller's `PATCH` of a finance review on an `in_review` or an
+ * `in_approval` contract answers `403 PERMISSION_DENIED` ("requires edit
+ * access to its master record"), the same write on an `active` contract `200`.
+ * So on those rows the finance controller could neither have decided the
+ * review it would be named for nor change it now; the head of legal can
+ * (`review_legal_update_any_stage`, and `contract_legal_all_head` edits every
+ * contract), and is the account the rule already falls back to. 7 of the 23
+ * finance reviews sit on such a contract.
+ *
+ * A business review stays the requester's although a requester cannot change
+ * it: `clm_requester` holds `R` on this object (§04), and both seeded business
+ * reviews are already decided. The requester sees it on their own contract.
+ *
+ * Structure only, so `demo-en` and `demo-zh` deal identically, and every name
+ * is declared demo staff (`demoStaff`), created by the platform before the
+ * seed runs.
+ */
+export const reviewerOf = (review: ReviewPlan): string => {
+  const contract = CONTRACT_PLAN[review.contractIndex]!;
+  switch (review.stage) {
+    case 'legal':
+      return legalOwnerOf(contract) ?? LEGAL_HEAD;
+    case 'finance':
+      return POST_APPROVAL_STATUSES.includes(contract.status) ? FINANCE_CONTROLLER : LEGAL_HEAD;
+    case 'compliance':
+      return LEGAL_HEAD;
+    case 'business':
+      return ownerOf(contract);
+  }
+};
+
 /**
  * Who filed a contract version (`clm_contract_version.submitted_by`), or
  * `null` for an executed copy.