From 0410379737190ea70a909daaa6937d7eb251d6ee Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 12:39:00 +0000 Subject: [PATCH 1/2] fix(data): deal seeded reviews to the desk whose stage they are All 60 seeded clm_review rows named the dev admin as reviewer, a choice forced while it was the only account at seed time. Since PR #171 the demo staff exist before the seed runs, so reviewerOf deals each review by stage: legal to the contract's legal_owner (head of legal where there is none), finance to the finance controller, compliance to the head of legal, and business to the launching requester. A finance review on a contract before `approved` goes to the head of legal: clm_review is controlled_by_parent and finance edits contracts only from approved onward, so the finance controller's PATCH of such a review answers 403 on 18.2.0. No row depends on the dev admin's name any more, so scripts/demo.mjs drops the name check from its priming boot; the sign-in check stays. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017nzWFRUo2U7evMkPdVniAn --- README.md | 19 +++--- scripts/demo.mjs | 81 ++++++------------------- src/data/keys.ts | 113 +++++++++++++++++++++++++++++------ src/data/negotiation.seed.ts | 47 ++++++--------- 4 files changed, 142 insertions(+), 118 deletions(-) diff --git a/README.md b/README.md index 9d5f836..a306dcc 100644 --- a/README.md +++ b/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 | @@ -169,10 +169,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 diff --git a/scripts/demo.mjs b/scripts/demo.mjs index 73150ae..176302e 100644 --- a/scripts/demo.mjs +++ b/scripts/demo.mjs @@ -18,15 +18,18 @@ // 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 @@ -34,12 +37,7 @@ // 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); diff --git a/src/data/keys.ts b/src/data/keys.ts index 62285a9..2cedac9 100644 --- a/src/data/keys.ts +++ b/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. diff --git a/src/data/negotiation.seed.ts b/src/data/negotiation.seed.ts index 2ce7f5a..76fc4fb 100644 --- a/src/data/negotiation.seed.ts +++ b/src/data/negotiation.seed.ts @@ -9,7 +9,7 @@ import { Signature } from '../objects/signature.object.js'; import { dayOffset, fixtureFile } from './_shared.js'; import { STRINGS } from './demo-locale.js'; -import { DEMO_USER, deviationDeciderOf, titleOfIndex, versionFilerOf } from './keys.js'; +import { DEMO_USER, deviationDeciderOf, reviewerOf, titleOfIndex, versionFilerOf } from './keys.js'; import { CONTRACT_PLAN, DEVIATION_PLAN, typeOf } from './plan-contracts.js'; import { REVIEW_PLAN, SIGNATURE_PLAN, VERSION_PLAN } from './plan-children.js'; import { contactEmailFor } from './plan.js'; @@ -18,34 +18,16 @@ import { contactEmailFor } from './plan.js'; * What negotiation leaves behind: 300 contract versions, 60 reviews, 25 * deviations and 30 signature rounds (DESIGN.md §10). * - * ## `clm_review.reviewer` is why `pnpm demo` boots twice + * ## `clm_review.reviewer` names the desk that decides the review * * `reviewer` is `required: true` and is a `user` reference the loader resolves - * against `sys_user.name`. The declarative seed runs BEFORE the dev admin is - * minted — measured on 17.3.0: the seed's last write lands at T, the admin - * account is created at T+1.8s — so on a first boot the name resolves to - * nothing, the loader defers the column, and the engine refuses the row: - * - * WARN [SeedLoader] clm_review.reviewer is `required: true`, but record #1 - * defers it to pass 2: 'Dev Admin' names no sys_user that exists yet. - * ERROR [SeedLoader] Failed to write clm_review record #1: Reviewer is required - * - * DESIGN.md §10 forbids seeding users, so the fixture cannot supply one. What - * it can do is arrange for the account to already exist, which is what - * `scripts/demo.mjs` does: a quiet priming boot with the seed OFF mints the - * dev admin and founds the organization, then the real boot seeds against a - * database where `Dev Admin` resolves in pass 1. Measured on the same - * database: 13 ok / 4 errors on the first boot became 14 ok / 2 errors on the - * second, with the review row landing and its `display_name` mirror reading - * "Finance · Dev Admin". - * - * Every seeded reviewer is therefore the demo account itself. That was the - * honest choice while it was the only user in the database. Since #28 - * (PR #171) the demo staff — Legal Counsel 1 and 2 among them — exist before - * the seed runs, so the constraint above no longer forces the dev admin; which - * account each review STAGE should name (legal, finance, compliance, - * business) is open and not decided here, and the README tells the evaluator - * to reassign a review to put it on a lawyer's desk. + * against `sys_user.name`, so a name no account carries costs the whole row. + * Until #186 every review named the dev admin for that reason: it was the only + * account at seed time, and `scripts/demo.mjs` primed it and checked its name + * before the demo boot seeded anything. Since #28 (PR #171) the demo staff + * (`demoStaff`) are created by the platform on the demo boot before the seed + * runs, so each review is dealt to the person whose stage it is — + * {@link reviewerOf} carries the rule and where it bends. */ /** @@ -88,12 +70,21 @@ export const contractVersionSeed = defineSeed(ContractVersion, { }), }); +/** + * 60 reviews, matched on `(contract, stage)`. + * + * `reviewer` is NOT `applyOn: 'create'`: like `legal_owner` and `owner_id` + * (`contract.seed.ts`) it is how the book is dealt to the demo staff, so a + * re-run re-deals it — which is also what moves the reviews of a database + * seeded before #186 off the dev admin. `review_display_name` re-stamps the + * `" · "` mirror on that write. + */ export const reviewSeed = defineSeed(Review, { externalId: ['contract', 'stage'], mode: 'upsert', records: REVIEW_PLAN.map((review) => ({ contract: titleOfIndex(review.contractIndex), - reviewer: DEMO_USER, + reviewer: reviewerOf(review), stage: review.stage, decision: review.decision, risk_level_assessed: review.riskLevelAssessed, From 5d7606e8bb74964e7d8e5b957c1868316eecd867 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 12:57:54 +0000 Subject: [PATCH 2/2] docs(data): record the re-run reading for seeded reviewers A re-run re-deals `reviewer` and re-stamps the review's title, measured on 18.2.0; it also puts a decided review back to the fixture's decision, which the README now lists beside deviations and signature rounds. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017nzWFRUo2U7evMkPdVniAn --- README.md | 7 ++++--- src/data/negotiation.seed.ts | 9 ++++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index a306dcc..7311a79 100644 --- a/README.md +++ b/README.md @@ -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`. diff --git a/src/data/negotiation.seed.ts b/src/data/negotiation.seed.ts index 76fc4fb..4cf8dfb 100644 --- a/src/data/negotiation.seed.ts +++ b/src/data/negotiation.seed.ts @@ -77,7 +77,14 @@ export const contractVersionSeed = defineSeed(ContractVersion, { * (`contract.seed.ts`) it is how the book is dealt to the demo staff, so a * re-run re-deals it — which is also what moves the reviews of a database * seeded before #186 off the dev admin. `review_display_name` re-stamps the - * `" · "` mirror on that write. + * `" · "` mirror on that write. Measured on 18.2.0: three + * reviews reassigned to the dev admin read `Legal · Legal Counsel 1`, + * `Finance · Finance Controller` and `Business · Business Requester 2` again + * after one more `pnpm demo:zh`, and the re-run refused no row. + * + * `decision` and `decided_at` are not `applyOn: 'create'` either, so a re-run + * puts a review someone decided back to the fixture's decision; the README + * says so beside the deviations and signature rounds that share the limit. */ export const reviewSeed = defineSeed(Review, { externalId: ['contract', 'stage'],