From 4592b6aa97308d97b5b6cc8e7b299062ae701d98 Mon Sep 17 00:00:00 2001 From: sebi Date: Sat, 15 Aug 2026 06:20:30 -0500 Subject: [PATCH 1/2] feat(tier0-1): money carries a decimal scale, flags decide by any subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tier-0/1 gaps found by auditing five production codebases against the framework: treasury (Rails), tesote.ai, developerz.ai, equipo.tesote.com and bank-integrations. money/schema — sub-cent precision --------------------------------- `Money` was cents-only, so the framework's own AI cost path could not represent the cost of its cheapest call: `costOf` divides with a ceiling to whole minor units, recording a $0.00016 model call as 1c — ~50x over — and `gateway.ts` builds its budget ledger on that number. Three teams independently moved to sub-cent storage (tesote.ai `cost_micros` after a logged 20x incident, developerz.ai `numeric(12,6)`, treasury `decimal(14,10)`), and an author needing the precision today hits X_MONEY_NOT_INTEGER and declares a second money type — the axiom-1 violation the package exists to prevent. `MoneyValue` gains an optional `scale`: the decimal places `minor` counts, when they are not the currency's own. Absent — the shape every existing value and row already has — still means the currency's natural minor unit, so this is additive and no app changes anything. A *required* scale was rejected on merit, not semver: it would make every stored row and JSON payload restate a fact ISO 4217 already owns. `money()` is the one place canonical form is decided and drops a scale equal to the currency exponent, so existing JSON stays byte-identical. Arithmetic normalises to the finer scale by exact bigint widening; comparison reads the value rather than the encoding. Widening is exact and free, narrowing needs an explicit RoundingMode. Two defects found on the way: - the money JSON Schema emitted `additionalProperties: false`, so a scaled value would have been refused by every generated client and MCP tool even once the validator accepted it. - `allocate` computed `(magnitude * ratio) / total`, exact only under 2^53 — latent, and scale 6 makes it 10,000x easier to reach. Now exact BigInt largest-remainder, with tie-breaking and every existing property unchanged. flags — subjects ---------------- Targeting was `{ default, actors, roles, rollout }` with no tenant axis, and `bucketOf(key, actor.id)` split a single organisation across a percentage rollout: 3 of 30 members see the new flow, 27 do not, on the same day. Classifying all 209 `Flipper.enabled?` call sites in treasury: 90.4% decide by the workspace, 8.6% are global, 1.0% by a non-tenant record. And app/services/feature_flags.rb:22 shows why an org axis alone would be wrong — three classes implement `flipper_id` (`workspace:`, `bank_integration:`, `bank_connection:`) and Flipper ORs them. There is no org axis and no actor axis; there is one axis whose members are `kind:id`. So `subjects: Record` is the mechanism, and `actors`/`orgs` are shorthands for the built-in kinds. `assertTargeting` refuses `subjects.actor` and `subjects.org`, so the two spellings can never disagree, and built-in kinds resolve only from the Actor — never from the call-site map — so there is no precedence rule to get wrong. `roles` stays separate: a role is a predicate over the actor, not an identified record, so it has no id and cannot bucket. `bucketBy` selects which subject a rollout divides, defaulting to `actor`, so no shipped flag changes answer. A flag deciding by a subject the context does not carry throws rather than falling back, with a fix that branches on the kind: a missing org points at `userActor({ id, orgId })`, a missing record at `isEnabled(key, actor, { bank: '' })`. Ergonomics stay in the app. `isEnabled(key, actor, { bank: bank.id })` is the primitive; a project wraps it in its own helper, exactly as treasury does. New error codes: X_MONEY_SCALE_INVALID, X_FLAG_SUBJECT_REQUIRED. Deliberately not in this PR --------------------------- `packages/ai` still rounds cost up to whole cents. It must not adopt scaled money until `packages/entity` carries scale: money persists as `_minor` bigint + char(3), and `parseMoney`/`narrowMoney`/`MONEY_PARTS` know only those two, so a scaled value round-trips as if it were at the currency's scale. Nothing writes scaled values yet, so nothing is wrong today — but adopting in the wrong order would make the ledger wrong in a quieter way than the 50x rounding it fixes. `packages/realtime` and `packages/admin` also read only minor/currency. Gate: bun run verify — 14 of 17 passed, 3 skipped (drift, contract-diff, budgets). 230 schema+money tests, 88 flags tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- framework.manifest.json | 12 +- packages/entity/src/type-pins.ts | 48 ++++- packages/flags/CLAUDE.md | 38 +++- packages/flags/README.md | 73 ++++++- packages/flags/src/bucket.test.ts | 26 +++ packages/flags/src/bucket.ts | 17 +- packages/flags/src/errors.test.ts | 2 + packages/flags/src/errors.ts | 34 +++- packages/flags/src/evaluate.test.ts | 29 +++ packages/flags/src/evaluate.ts | 14 +- packages/flags/src/index.ts | 5 +- packages/flags/src/subject.test.ts | 105 ++++++++++ packages/flags/src/subject.ts | 68 +++++++ packages/flags/src/targeting.test.ts | 257 ++++++++++++++++++++++++ packages/flags/src/targeting.ts | 119 ++++++++++- packages/money/CLAUDE.md | 28 ++- packages/money/README.md | 30 ++- packages/money/src/allocate.test.ts | 29 +++ packages/money/src/allocate.ts | 79 +++++--- packages/money/src/arithmetic.test.ts | 45 +++++ packages/money/src/arithmetic.ts | 39 +++- packages/money/src/errors.ts | 46 ++++- packages/money/src/format.ts | 23 ++- packages/money/src/index.ts | 5 + packages/money/src/money.test.ts | 55 ++++- packages/money/src/money.ts | 64 ++++-- packages/money/src/rescale.test.ts | 68 +++++++ packages/money/src/rescale.ts | 29 +++ packages/money/src/scale.test.ts | 57 ++++++ packages/money/src/scale.ts | 50 +++++ packages/schema/CLAUDE.md | 14 +- packages/schema/README.md | 1 + packages/schema/src/builder.ts | 5 + packages/schema/src/coerce.test.ts | 7 + packages/schema/src/coerce.ts | 13 +- packages/schema/src/index.ts | 3 +- packages/schema/src/json-schema.test.ts | 8 + packages/schema/src/json-schema.ts | 9 + packages/schema/src/money-value.test.ts | 97 +++++++++ packages/schema/src/money-value.ts | 116 +++++++++++ packages/schema/src/t.ts | 9 +- packages/schema/src/validators.test.ts | 45 ----- packages/schema/src/validators.ts | 65 +----- wiki/Error-Codes.md | 4 +- 44 files changed, 1664 insertions(+), 226 deletions(-) create mode 100644 packages/flags/src/subject.test.ts create mode 100644 packages/flags/src/subject.ts create mode 100644 packages/money/src/rescale.test.ts create mode 100644 packages/money/src/rescale.ts create mode 100644 packages/money/src/scale.test.ts create mode 100644 packages/money/src/scale.ts create mode 100644 packages/schema/src/money-value.test.ts create mode 100644 packages/schema/src/money-value.ts diff --git a/framework.manifest.json b/framework.manifest.json index 78ef8608..676e8446 100644 --- a/framework.manifest.json +++ b/framework.manifest.json @@ -1,6 +1,6 @@ { "version": 1, - "buildId": "27802f515afaf2c6010e9695512bb4798b0156aac6e194f3ae2f1b239acacea9", + "buildId": "079a1d2632bb005306c965d1d331c38e09e62ac2024bc63aeb10826d82cf33aa", "tiers": { "0": [ "core", @@ -700,6 +700,11 @@ "owner": "flags", "at": "packages/flags/src/errors.ts" }, + { + "code": "X_FLAG_SUBJECT_REQUIRED", + "owner": "flags", + "at": "packages/flags/src/errors.ts" + }, { "code": "X_FLAG_TARGETING_INVALID", "owner": "flags", @@ -1010,6 +1015,11 @@ "owner": "money", "at": "packages/money/src/errors.ts" }, + { + "code": "X_MONEY_SCALE_INVALID", + "owner": "money", + "at": "packages/money/src/errors.ts" + }, { "code": "X_N_PLUS_ONE_QUERY", "owner": "entity", diff --git a/packages/entity/src/type-pins.ts b/packages/entity/src/type-pins.ts index e2c6912a..2cca18b2 100644 --- a/packages/entity/src/type-pins.ts +++ b/packages/entity/src/type-pins.ts @@ -252,9 +252,51 @@ type _MoneyValueIsSchemasDeclaration = Assert; -/** Immutable, enforced: a mutable `minor` is a rounding bug with a place to hide. */ -type _MoneyIsReadonly = Assert< - Identical +// The shape is pinned as independent properties rather than as one literal snapshot of the whole +// interface. The snapshot said the same thing, but every additive change had to be hand-edited +// past it — and a pin the next reader learns to hand-edit reflexively has stopped being a check. +// Only the key set moves when a field is added, which is the one place that decision belongs. + +/** No field but these three, ever: a fourth is a shape nobody declared. */ +type _MoneyHasNoOtherField = Assert< + [keyof MoneyValue] extends ['minor' | 'currency' | 'scale'] ? true : false +>; + +/** …and none of the three may go — the pin must not pass by the type shrinking instead. */ +type _MoneyHasEveryField = Assert< + ['minor' | 'currency' | 'scale'] extends [keyof MoneyValue] ? true : false +>; + +// Immutable, enforced, field by field: a mutable `minor` is a rounding bug with a place to hide. +// `Pick` carries `readonly` and optionality through, so each of these is exact about one field +// and says nothing about the others. + +type _MoneyMinorIsAReadonlyNumber = Assert< + Identical, { readonly minor: number }> +>; + +type _MoneyCurrencyIsAReadonlyString = Assert< + Identical, { readonly currency: string }> +>; + +/** + * `scale` is the decimal exponent `minor` counts in — `{ minor: 2, currency: 'USD', scale: 6 }` is + * $0.000002. Optional, and pinned optional, because a cents-only `Money` could not name a + * sub-cent amount at all: the AI cost path rounded a $0.0002 call up to a whole cent, ~50x, and + * the alternative to this field was a second money type. + */ +type _MoneyScaleIsAReadonlyOptionalNumber = Assert< + Identical, { readonly scale?: number }> +>; + +/** + * The additive half, and the pin that decides the semver: a value carrying no scale is still a + * `MoneyValue`, meaning the currency's own minor unit. Every amount already stored, serialized + * and asserted against in every app is that shape — so the day this fails, the change that made + * it fail is a breaking one and needs a major, not a fix here. + */ +type _MoneyWithoutAScaleIsStillMoney = Assert< + { readonly minor: number; readonly currency: string } extends MoneyValue ? true : false >; /** diff --git a/packages/flags/CLAUDE.md b/packages/flags/CLAUDE.md index 3d1c074f..4fe8b329 100644 --- a/packages/flags/CLAUDE.md +++ b/packages/flags/CLAUDE.md @@ -12,6 +12,7 @@ what lets `policy` (tier 2) call it from inside a predicate. | Exports | `src/index.ts`, explicit, no `export *` | | Errors | `src/errors.ts`, subclass `UltimateError`, never a bare `Error` | | Files | one responsibility each, < 200 lines, tests beside the source | +| Subjects | `src/subject.ts` — one resolver; never a second allow list per record kind | ## Invariants @@ -36,10 +37,39 @@ what lets `policy` (tier 2) call it from inside a predicate. Never name a vendor here (axiom 7). - **The rate limit is keyed on `clock.monotonic()`**, not wall time: an NTP correction or a container resuming must not reopen the window and flood the monitor. -- **Buckets are `fnv1a(key + ':' + actorId) % 100`.** Never `Math.random()`, and never the actor id - alone — hashing the actor by itself puts the same cohort in the first slice of every rollout the - app ever runs. -- **Allow lists beat the rollout.** An operator who named an actor is not overruled by a hash. +- **Buckets are `fnv1a(key + ':' + subjectId) % 100`.** Never `Math.random()`, and never the + subject id alone — hashing the subject by itself puts the same cohort in the first slice of every + rollout the app ever runs. **The assignments are pinned in `bucket.test.ts`**: a rollout already + live is a promise to the subjects inside it, so changing the hash must break that test, never + move the boundary silently. +- **A flag decides about a SUBJECT, and the actor is one kind of subject.** `subject.ts` owns the + resolution. `actor` and `org` come off the `Actor`; every other kind comes from the `subjects` + argument at the call site. `actors` and `orgs` in targeting are shorthands for the first two + kinds, not separate mechanisms — do not add a fourth parallel allow list for a new record kind, + it already works through `subjects`. `roles` is NOT a subject: a role is a predicate over the + actor, has no id, and cannot bucket. +- **One source per kind.** A built-in kind is never read from the call-site map, so there is no + precedence rule and no second place a tenant comes from. Passing `org` at a call site is dead + data; with no `actor.orgId` the evaluation raises and the fix line says to mint the actor. + `assertTargeting` refuses `subjects.actor` / `subjects.org` for the same reason. +- **The kind space is open, like the flag key space.** No registry of kinds: a typo raises + `X_FLAG_SUBJECT_REQUIRED` at the first evaluation, the same loud failure `X_FLAG_UNKNOWN` already + gives an undeclared key. Do not add a `defineSubjectKinds()` — it is a second declaration surface + buying a check evaluation already makes. +- **Allow lists beat the rollout.** An operator who named a subject is not overruled by a hash. + `actors`, `roles`, `orgs` and `subjects` are one rank — any hit is `true`, so their order is + unobservable. That is the same OR Flipper applies across the actors passed to one `enabled?`. +- **The subject axis throws rather than degrades.** A kind the evaluation context does not carry + raises `X_FLAG_SUBJECT_REQUIRED`. Never fall back to the actor axis or to `default`: an answer + about a record computed from whoever was calling looks like it worked, which is the whole bug + class. **Every declared kind is resolved before any can answer**, so the raise never depends on + declaration order. A `null` actor is the one exception and still gets `default` — no evaluation + context at all, every such call answers alike, so no single subject is split. +- **`bucketBy` defaults to `actor`.** The subject axes are opt-in; a flag declared before they + existed must answer identically, which is why the default is not `org`. +- **`subjectIdOf` is called only on branches that need a subject**, so a plain + `{ default, rollout }` flag still allocates nothing. Keep it that way — no closures, no + normalisation pass, no `Object.entries` on the common path. - **An unknown key throws.** Answering `false` is a branch that never runs and never says so. - `default: true` beside a `rollout` is refused: the two answer the same actors and disagree. diff --git a/packages/flags/README.md b/packages/flags/README.md index d44e2682..ef79128d 100644 --- a/packages/flags/README.md +++ b/packages/flags/README.md @@ -54,6 +54,11 @@ import { isEnabled } from '@ultimat3/flags'; if (isEnabled('checkout.new-tax-engine', actor)) { // … } + +// with the app's own records in play +if (isEnabled('scraper.persist-profile', actor, { bank: bank.id })) { + // … +} ``` Synchronous, for the same reason `can()` is: this runs inside policy predicates and render passes, @@ -68,11 +73,64 @@ silently never runs in production. | `default` | the answer when no allow list and no rollout claims this actor | | `actors` | actor ids that are always on, ahead of any rollout | | `roles` | actor roles that are always on, ahead of any rollout | -| `rollout` | whole percentage 0-100, stable per actor | +| `orgs` | org ids that are always on — shorthand for the `org` subject kind, read from `actor.orgId` | +| `subjects` | allow lists for the app's own record kinds: `{ bank: ['bank_integration:bbva'] }` | +| `rollout` | whole percentage 0-100, stable per bucketing subject | +| `bucketBy` | which subject kind the rollout divides: `'actor'` (default), `'org'`, or any kind the call site carries | + +Order is allow lists → rollout → default. `actors`, `roles`, `orgs` and `subjects` are one rank — +any hit is `true`. An operator who names a subject is not overruled by a hash. A rollout buckets +`fnv1a(key + ':' + subjectId) % 100`, never `Math.random()`: one subject gets one answer on every +call, in every process, without the nodes talking to each other. + +## Subjects — what a flag decides about + +A flag decides about an **identified record**: a user, a tenant, a bank integration, a device. The +actor is one subject kind among several, not a privileged one. + +| Kind | Where its id comes from | +|---|---| +| `actor` | `actor.id` — spelled `actors` in targeting | +| `org` | `actor.orgId` — spelled `orgs` in targeting | +| anything else | the `subjects` argument at the call site | + +```ts +// whole tenants, named — the 90% case, which is why it has a shorthand +targeting: { default: false, orgs: ['org_acme'] } + +// 10% of tenants, each one whole +targeting: { default: false, rollout: 10, bucketBy: 'org' } + +// the app's own record kind +targeting: { default: false, subjects: { bank: ['bank_integration:bbva'] } } +isEnabled('scraper.persist-profile', actor, { bank: 'bank_integration:bbva' }); + +// 10% of banks, each bank whole +targeting: { default: false, rollout: 10, bucketBy: 'bank' } +``` + +`actor` and `org` are resolved from the `Actor` and **never** from the call-site map — one source +per kind, so there is no precedence rule to remember and no second place a tenant can come from. +Every other kind is the app's vocabulary; the kind space is open, exactly like the flag key space. + +Bucketing by a record is what keeps it **whole**. An actor-bucketed rollout cuts through a tenant: +3 of an org's 30 members on the new export path and 27 on the old, sharing documents, filing a bug +nobody can reproduce. `bucketBy` puts the whole subject on one side. `'actor'` stays the default, +so every flag declared before this axis answers exactly as it did. + +`roles` is deliberately **not** a subject kind: a role is a predicate over the actor, not an +identified record, so it has no id to hash and cannot bucket a rollout. + +### A missing subject is an error, not a fallback + +If targeting decides by a kind the evaluation context does not carry, `isEnabled()` throws +`X_FLAG_SUBJECT_REQUIRED`. It never falls back to the actor axis or to `default`: an answer about a +record computed from whoever happened to be calling is the exact failure this axis removes, and it +looks like it worked. Every declared kind is resolved before any of them can answer, so the raise +does not depend on the order the keys sit in. -Order is allow lists → rollout → default. An operator who names an actor is not overruled by a -hash. A rollout buckets `fnv1a(key + ':' + actor.id) % 100`, never `Math.random()`: one actor gets -one answer on every call, in every process, without the nodes talking to each other. +A `null` actor is the one exception and still gets `default` — it says there is no evaluation +context at all, every such call answers alike, and no single subject is split. ## Overrides, out of band @@ -113,7 +171,8 @@ and the manifest should all read, so none of them recomputes "expired". |---|---| | `src/flag.ts` | the two kinds, the compile-time expiry rule, normalisation | | `src/targeting.ts` | who a flag is on for; declaration-time validation | -| `src/bucket.ts` | the stable `(flag, actor)` bucket — FNV-1a, never `Math.random()` | +| `src/subject.ts` | what a flag decides about — subject kinds and how each resolves to an id | +| `src/bucket.ts` | the stable `(flag, subject)` bucket — FNV-1a, never `Math.random()` | | `src/registry.ts` | `defineFlag`, key → flag, `applyFlagSnapshot` | | `src/runtime.ts` | the clock and the per-flag report rate limit over core's `reportError` | | `src/evaluate.ts` | `isEnabled()` — the one way to ask | @@ -126,5 +185,5 @@ Tier 1. May import tiers 0-0 only — enforced by `bun run scripts/boundaries.ts ## Errors -`X_FLAG_DUPLICATE` · `X_FLAG_EXPIRED` · `X_FLAG_EXPIRY_INVALID` · `X_FLAG_TARGETING_INVALID` · -`X_FLAG_UNKNOWN` +`X_FLAG_DUPLICATE` · `X_FLAG_EXPIRED` · `X_FLAG_EXPIRY_INVALID` · `X_FLAG_SUBJECT_REQUIRED` · +`X_FLAG_TARGETING_INVALID` · `X_FLAG_UNKNOWN` diff --git a/packages/flags/src/bucket.test.ts b/packages/flags/src/bucket.test.ts index 973e8716..bd46161b 100644 --- a/packages/flags/src/bucket.test.ts +++ b/packages/flags/src/bucket.test.ts @@ -57,6 +57,32 @@ describe('unit · bucketOf', () => { }); }); +describe('unit · pinned assignments', () => { + test('a known (flag, subject) pair keeps its bucket, so a hash change cannot re-roll everyone', () => { + // These literals are the contract. A rollout already in production is a promise to the orgs + // and actors inside it: changing the hash silently moves the boundary under a live rollout, + // switching a feature off for tenants who had it. Breaking this test is the intended alarm — + // never re-pin it to whatever the new hash says. + expect(bucketOf('billing.export', 'org-42')).toBe(13); + expect(bucketOf('billing.export', 'org-12')).toBe(0); + expect(bucketOf('billing.export', 'org-1')).toBe(78); + expect(bucketOf('billing.export', 'user-1')).toBe(51); + expect(bucketOf('search.rerank', 'org-42')).toBe(87); + // A record subject is the same hash — `flipper_id`-shaped ids included. + expect(bucketOf('scraper.persist-profile', 'bank_integration:bbva')).toBe(39); + expect(bucketOf('scraper.persist-profile', 'bank_integration:santander')).toBe(86); + }); + + test('one org gets one bucket for one flag, in this process and any other', () => { + // Determinism is what makes "whole org in or whole org out" true across nodes and restarts: + // the hash is pure, so a second process computing it agrees without being asked. + const first = bucketOf('billing.export', 'org-42'); + for (let call = 0; call < 1_000; call += 1) { + expect(bucketOf('billing.export', 'org-42')).toBe(first); + } + }); +}); + describe('unit · fnv1a', () => { test('is the published 32-bit FNV-1a, so two nodes agree without talking', () => { // Reference vectors from the FNV specification. diff --git a/packages/flags/src/bucket.ts b/packages/flags/src/bucket.ts index 06215b99..cb3990f0 100644 --- a/packages/flags/src/bucket.ts +++ b/packages/flags/src/bucket.ts @@ -1,4 +1,5 @@ -// Single responsibility: the stable bucket a (flag, actor) pair falls into. Never `Math.random()`: +// Single responsibility: the stable bucket a (flag, subject) pair falls into. The subject is an +// actor id or an org id — same hash either way, so a tenant is whole. Never `Math.random()`: // a rollout that re-rolls per call shows one user the new experience on one request and the old // one on the next, which is a worse product than no rollout at all — and untestable besides. @@ -23,9 +24,13 @@ export function fnv1a(text: string): number { } /** - * The flag key is hashed WITH the actor id, not the actor id alone: hashing the actor by itself - * would put the same unlucky cohort in the first 10% of every 10% rollout the app ever runs, so - * one group of users would meet every half-finished feature in the product. + * The flag key is hashed WITH the subject id, not the subject id alone: hashing the subject by + * itself would put the same unlucky cohort in the first 10% of every 10% rollout the app ever + * runs, so one group of users — or one group of tenants — would meet every half-finished feature. + * + * `subjectId` is whatever axis the targeting buckets by: an actor id, or an org id when + * `bucketBy: 'org'` keeps a tenant on one side of the boundary. Pure, so two nodes agree about a + * subject without talking, and a restart does not re-roll anyone. */ -export const bucketOf = (key: string, actorId: string): number => - fnv1a(`${key}:${actorId}`) % BUCKETS; +export const bucketOf = (key: string, subjectId: string): number => + fnv1a(`${key}:${subjectId}`) % BUCKETS; diff --git a/packages/flags/src/errors.test.ts b/packages/flags/src/errors.test.ts index 613c4a45..6816fcef 100644 --- a/packages/flags/src/errors.test.ts +++ b/packages/flags/src/errors.test.ts @@ -6,6 +6,7 @@ import { flagDuplicate, flagExpired, flagExpiryInvalid, + flagSubjectRequired, flagTargetingInvalid, flagUnknown, } from './errors'; @@ -29,6 +30,7 @@ describe('unit · @ultimat3/flags errors', () => { flagDuplicate('search.rerank'), flagUnknown('search.rerank', ['search.rerank']), flagTargetingInvalid('search.rerank', 'rollout is 0.5'), + flagSubjectRequired({ key: 'search.rerank', kind: 'org', actorId: 'user-7', via: 'orgs' }), flagExpiryInvalid('search.rerank', undefined), flagExpired({ key: 'search.rerank', diff --git a/packages/flags/src/errors.ts b/packages/flags/src/errors.ts index 3c811887..49c90f84 100644 --- a/packages/flags/src/errors.ts +++ b/packages/flags/src/errors.ts @@ -8,6 +8,7 @@ export const FLAGS_ERROR_CODES = [ 'X_FLAG_DUPLICATE', 'X_FLAG_EXPIRED', 'X_FLAG_EXPIRY_INVALID', + 'X_FLAG_SUBJECT_REQUIRED', 'X_FLAG_TARGETING_INVALID', 'X_FLAG_UNKNOWN', ] as const; @@ -18,6 +19,7 @@ export const FLAGS_ERROR_TITLES: Readonly> = { X_FLAG_DUPLICATE: 'two flags were declared with the same key', X_FLAG_EXPIRED: 'a temporary flag is past its expiry and is still being evaluated', X_FLAG_EXPIRY_INVALID: 'a temporary flag has no usable expiry date', + X_FLAG_SUBJECT_REQUIRED: 'a flag decides by a subject the evaluation context does not carry', X_FLAG_TARGETING_INVALID: 'flag targeting is out of range or malformed', X_FLAG_UNKNOWN: 'no flag is declared under this key', }; @@ -63,14 +65,42 @@ export const flagUnknown = (key: string, known: readonly string[]): FlagsError = meta: { key }, }); -export const flagTargetingInvalid = (key: string, problem: string): FlagsError => +/** `fix` is a parameter because a bad `bucketBy` is not repaired by editing `rollout` — axiom 4. */ +export const flagTargetingInvalid = (key: string, problem: string, fix?: string): FlagsError => new FlagsError({ code: 'X_FLAG_TARGETING_INVALID', cause: `${key}: ${problem}`, - fix: `set rollout to an integer 0-100 in defineFlag({ key: '${key}' })`, + fix: fix ?? `set rollout to an integer 0-100 in defineFlag({ key: '${key}' })`, meta: { key }, }); +/** Which targeting field asked for the subject, so the fix names an edit rather than a mechanism. */ +export type FlagSubjectVia = 'orgs' | 'subjects' | 'bucketBy'; + +/** + * Thrown, never softened into a fallback. Answering a subject-scoped flag from the actor axis — or + * from the declared default — is the exact failure the subject axis exists to remove: it looks + * like it worked, and the record finds out when half of it is on a different code path. + * + * The fix differs by kind because the edit does: a missing org is repaired where the actor is + * minted, a missing record is repaired at the call site that already holds it. + */ +export const flagSubjectRequired = (init: { + key: string; + kind: string; + actorId: string; + via: FlagSubjectVia; +}): FlagsError => + new FlagsError({ + code: 'X_FLAG_SUBJECT_REQUIRED', + cause: `${init.key} decides by the "${init.kind}" subject (targeting.${init.via}) but the evaluation context carries no ${init.kind} id for actor "${init.actorId}", so there is nothing to decide about`, + fix: + init.kind === 'org' + ? `mint the actor with its tenant — userActor({ id: '${init.actorId}', orgId: '' }) — before the isEnabled('${init.key}') call, or drop ${init.via} from defineFlag({ key: '${init.key}' })` + : `pass the record at the call site — isEnabled('${init.key}', actor, { ${init.kind}: '' }) — or drop the "${init.kind}" ${init.via} entry from defineFlag({ key: '${init.key}' })`, + meta: { key: init.key, kind: init.kind, actorId: init.actorId, via: init.via }, + }); + export const flagExpiryInvalid = (key: string, given: unknown): FlagsError => new FlagsError({ code: 'X_FLAG_EXPIRY_INVALID', diff --git a/packages/flags/src/evaluate.test.ts b/packages/flags/src/evaluate.test.ts index 1c2ab98a..52355f70 100644 --- a/packages/flags/src/evaluate.test.ts +++ b/packages/flags/src/evaluate.test.ts @@ -160,3 +160,32 @@ describe('unit · an undeclared key', () => { expect((thrown as UltimateError).fix).toContain('defineFlag('); }); }); + +describe('unit · isEnabled with record subjects', () => { + test('passes the call-site subjects through to targeting', () => { + defineFlag({ + kind: 'permanent', + key: 'scraper.persist-profile', + description: 'per-bank scraper switch', + targeting: { default: false, subjects: { bank: ['bank_integration:bbva'] } }, + }); + expect(isEnabled('scraper.persist-profile', actor, { bank: 'bank_integration:bbva' })).toBe( + true, + ); + expect( + isEnabled('scraper.persist-profile', actor, { bank: 'bank_integration:santander' }), + ).toBe(false); + }); + + test('a flag deciding by a record raises when the call site forgot to pass it', () => { + defineFlag({ + kind: 'permanent', + key: 'scraper.persist-profile', + description: 'per-bank scraper switch', + targeting: { default: false, subjects: { bank: ['bank_integration:bbva'] } }, + }); + expect(caught(() => isEnabled('scraper.persist-profile', actor))).toBeUltimateError( + 'X_FLAG_SUBJECT_REQUIRED', + ); + }); +}); diff --git a/packages/flags/src/evaluate.ts b/packages/flags/src/evaluate.ts index bb95e108..c1e4f8fd 100644 --- a/packages/flags/src/evaluate.ts +++ b/packages/flags/src/evaluate.ts @@ -7,6 +7,7 @@ import { flagExpired } from './errors'; import type { Flag } from './flag'; import { flagFor } from './registry'; import { flagsClock, reportOnce } from './runtime'; +import type { FlagSubjects } from './subject'; import { evaluateTargeting } from './targeting'; const MS_PER_DAY = 24 * 60 * 60 * 1000; @@ -37,9 +38,18 @@ function reportIfOverdue(flag: Flag): void { * `actor` is passed rather than read from the ambient context on purpose: a policy predicate, * a job and a render pass each already hold the actor they are deciding about, and reading an * ambient one would let a job evaluate a flag for whoever enqueued it. + * + * `subjects` carries the app's own records in play — `{ bank: 'bank_integration:bbva' }` — for a + * flag targeted at something other than the caller. Same reasoning as `actor`: the call site + * already holds the record it is deciding about. A flag that needs a kind the call site did not + * pass raises `X_FLAG_SUBJECT_REQUIRED` rather than quietly deciding about somebody else. */ -export function isEnabled(key: string, actor: Actor | null): boolean { +export function isEnabled( + key: string, + actor: Actor | null, + subjects?: FlagSubjects | undefined, +): boolean { const flag = flagFor(key); reportIfOverdue(flag); - return evaluateTargeting(flag.key, flag.targeting, actor); + return evaluateTargeting(flag.key, flag.targeting, actor, subjects); } diff --git a/packages/flags/src/index.ts b/packages/flags/src/index.ts index c623d8d9..bf846c2c 100644 --- a/packages/flags/src/index.ts +++ b/packages/flags/src/index.ts @@ -1,7 +1,7 @@ // Public API of @ultimat3/flags. Explicit re-exports only. export { BUCKETS, bucketOf, fnv1a } from './bucket'; -export type { FlagsErrorCode } from './errors'; +export type { FlagSubjectVia, FlagsErrorCode } from './errors'; export { FLAGS_ERROR_CODES, FLAGS_ERROR_TITLES, @@ -9,6 +9,7 @@ export { flagDuplicate, flagExpired, flagExpiryInvalid, + flagSubjectRequired, flagTargetingInvalid, flagUnknown, } from './errors'; @@ -30,4 +31,6 @@ export type { FlagsRuntimeOptions } from './runtime'; // The reporter seam is `@ultimat3/core`'s `ErrorReporter`, wired once with // `configureErrorReporting()`. This package deliberately re-exports none of it. export { configureFlags, DEFAULT_REPORT_INTERVAL_MS, resetFlagReporting } from './runtime'; +export type { BuiltInSubjectKind, FlagSubjects } from './subject'; +export { BUILT_IN_SUBJECT_KINDS } from './subject'; export type { FlagTargeting } from './targeting'; diff --git a/packages/flags/src/subject.test.ts b/packages/flags/src/subject.test.ts new file mode 100644 index 00000000..bee5c16f --- /dev/null +++ b/packages/flags/src/subject.test.ts @@ -0,0 +1,105 @@ +// A subject is what a flag decides ABOUT. The tests that must be able to fail are the resolution +// ones: a kind the evaluation context does not carry has to raise, because the alternative — a +// fallback to the actor, or to the declared default — is an answer that looks like it worked. + +import { describe, expect, test } from 'bun:test'; +import { userActor } from '@ultimat3/core'; +import { BUILT_IN_SUBJECT_KINDS, subjectIdOf } from './subject'; + +const caught = (run: () => unknown): unknown => { + try { + run(); + } catch (thrown) { + return thrown; + } + return undefined; +}; + +const actor = userActor({ id: 'user-1', orgId: 'org-a' }); + +describe('unit · subjectIdOf', () => { + test('resolves the two built-in kinds off the actor, which already carries both', () => { + expect( + subjectIdOf({ key: 'a.flag', kind: 'actor', actor, subjects: undefined, via: 'orgs' }), + ).toBe('user-1'); + expect( + subjectIdOf({ key: 'a.flag', kind: 'org', actor, subjects: undefined, via: 'orgs' }), + ).toBe('org-a'); + }); + + test('resolves an app kind from the subjects passed at the call site', () => { + const subjects = { bank: 'bank_integration:bbva' }; + expect(subjectIdOf({ key: 'a.flag', kind: 'bank', actor, subjects, via: 'subjects' })).toBe( + 'bank_integration:bbva', + ); + }); + + test('throws naming the missing kind when the context does not carry it', () => { + const thrown = caught(() => + subjectIdOf({ key: 'a.flag', kind: 'bank', actor, subjects: {}, via: 'subjects' }), + ); + expect(thrown).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + expect((thrown as { cause: string }).cause).toContain('bank'); + }); + + test('throws when the actor carries no orgId, rather than answering off the actor id', () => { + expect( + caught(() => + subjectIdOf({ + key: 'a.flag', + kind: 'org', + actor: userActor({ id: 'user-2' }), + subjects: undefined, + via: 'orgs', + }), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an empty id is absent, not a subject — it would otherwise hash to a real bucket', () => { + expect( + caught(() => + subjectIdOf({ + key: 'a.flag', + kind: 'bank', + actor, + subjects: { bank: '' }, + via: 'subjects', + }), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('a built-in kind is never read from the map — the actor is its one source', () => { + // Passing `org` at the call site is dead data, not a second way to supply the tenant. It + // cannot produce a wrong answer: with no `actor.orgId` this raises, and the fix line says so. + const thrown = caught(() => + subjectIdOf({ + key: 'a.flag', + kind: 'org', + actor: userActor({ id: 'user-2' }), + subjects: { org: 'org-z' }, + via: 'orgs', + }), + ); + expect(thrown).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('the fix names the edit that resolves it, and the edit differs by kind', () => { + const orgless = userActor({ id: 'user-2' }); + const orgFix = caught(() => + subjectIdOf({ key: 'a.flag', kind: 'org', actor: orgless, subjects: {}, via: 'orgs' }), + ) as { fix: string }; + expect(orgFix.fix).toContain('orgId'); + + const bankFix = caught(() => + subjectIdOf({ key: 'a.flag', kind: 'bank', actor, subjects: {}, via: 'subjects' }), + ) as { fix: string }; + expect(bankFix.fix).toContain("isEnabled('a.flag'"); + expect(bankFix.fix).toContain('bank'); + }); + + test('actor and org are the built-ins, and nothing else is', () => { + expect([...BUILT_IN_SUBJECT_KINDS]).toEqual(['actor', 'org']); + }); +}); diff --git a/packages/flags/src/subject.ts b/packages/flags/src/subject.ts new file mode 100644 index 00000000..b3c6fe07 --- /dev/null +++ b/packages/flags/src/subject.ts @@ -0,0 +1,68 @@ +// Single responsibility: what a flag decides ABOUT, and how a subject kind resolves to the one id +// that gets matched or hashed. A subject is any identified record — a tenant, a bank integration, +// a device — which is the generalisation of the actor axis, not a second mechanism beside it. + +import type { Actor } from '@ultimat3/core'; +import type { FlagSubjectVia } from './errors'; +import { flagSubjectRequired } from './errors'; + +/** + * The records in play at ONE evaluation, keyed by kind: `{ bank: 'bank_integration:bbva' }`. + * + * A map rather than a list of `{ kind, id }` because a single evaluation has a single bank, a + * single project, a single device: the shape makes a duplicate kind unrepresentable instead of a + * rule nothing enforces. The id is the app's — an opaque string, never parsed here. + */ +export type FlagSubjects = Readonly>; + +/** + * The two kinds every app has, and the two an `Actor` already carries. Everything else is the + * app's own vocabulary and arrives in `FlagSubjects`. + * + * A built-in kind is resolved from the actor and NEVER from the map. That is what keeps this a + * single mechanism rather than two: one source per kind, so there is no precedence rule to + * remember and no second place a tenant can come from. Passing `org` at a call site is dead data, + * and it cannot produce a wrong answer — without `actor.orgId` the evaluation raises, and the fix + * line says to mint the actor with its org. + */ +export const BUILT_IN_SUBJECT_KINDS = ['actor', 'org'] as const; + +export type BuiltInSubjectKind = (typeof BUILT_IN_SUBJECT_KINDS)[number]; + +export const isBuiltInSubjectKind = (kind: string): kind is BuiltInSubjectKind => + (BUILT_IN_SUBJECT_KINDS as readonly string[]).includes(kind); + +/** + * The id for `kind`, or a loud failure — never a fallback to the actor and never the declared + * default. An answer about a record computed from whoever happened to be calling is the bug this + * axis removes: it looks like it worked. An empty string is absent, not an id; it would otherwise + * match an allow list entry or hash to a real bucket. + * + * The kind space is open, exactly like the flag key space. A typo'd kind raises here on the first + * evaluation, which is the same loud failure an undeclared key already gets from `X_FLAG_UNKNOWN` + * — a registry of kinds would be a second declaration surface buying a check this already makes. + */ +export function subjectIdOf(init: { + readonly key: string; + readonly kind: string; + readonly actor: Actor; + readonly subjects: FlagSubjects | undefined; + readonly via: FlagSubjectVia; +}): string { + const { key, kind, actor, subjects, via } = init; + const id = resolve(kind, actor, subjects); + if (id === undefined || id === '') { + throw flagSubjectRequired({ key, kind, actorId: actor.id, via }); + } + return id; +} + +function resolve( + kind: string, + actor: Actor, + subjects: FlagSubjects | undefined, +): string | undefined { + if (kind === 'actor') return actor.id; + if (kind === 'org') return actor.orgId; + return subjects?.[kind]; +} diff --git a/packages/flags/src/targeting.test.ts b/packages/flags/src/targeting.test.ts index 1444e2d0..408a45e8 100644 --- a/packages/flags/src/targeting.test.ts +++ b/packages/flags/src/targeting.test.ts @@ -3,7 +3,9 @@ // like "half" is the silent wrong answer this package is meant to design out. import { describe, expect, test } from 'bun:test'; +import type { Actor } from '@ultimat3/core'; import { userActor } from '@ultimat3/core'; +import type { FlagTargeting } from './targeting'; import { assertTargeting, evaluateTargeting } from './targeting'; const caught = (run: () => unknown): unknown => { @@ -61,6 +63,201 @@ describe('unit · precedence', () => { }); }); +/** + * The tenant axis. The bug it removes is the one an actor-bucketed rollout creates: 3 of an org's + * 30 members on the new path and 27 on the old, sharing documents, filing a bug nobody can + * reproduce. Every assertion below is about a whole org landing on one side. + */ +describe('unit · the org axis', () => { + const inA = userActor({ id: 'user-1', orgId: 'org-a' }); + const alsoInA = userActor({ id: 'user-2', orgId: 'org-a' }); + const inB = userActor({ id: 'user-3', orgId: 'org-b' }); + const orgless = userActor({ id: 'user-4' }); + + test('an actor with no orgId throws rather than being answered off the actor axis', () => { + expect( + caught(() => evaluateTargeting('a.flag', { default: false, orgs: ['org-a'] }, orgless)), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an org-bucketed rollout throws when the actor carries no orgId', () => { + expect( + caught(() => + evaluateTargeting('a.flag', { default: false, rollout: 50, bucketBy: 'org' }, orgless), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an empty orgId is absent, not a tenant — it would hash to a real bucket', () => { + expect( + caught(() => + evaluateTargeting( + 'a.flag', + { default: false, orgs: ['org-a'] }, + userActor({ id: 'user-5', orgId: '' }), + ), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an org allow list is on for every member of that org and off for another org', () => { + const targeting: FlagTargeting = { default: false, orgs: ['org-a'] }; + expect(evaluateTargeting('a.flag', targeting, inA)).toBe(true); + expect(evaluateTargeting('a.flag', targeting, alsoInA)).toBe(true); + expect(evaluateTargeting('a.flag', targeting, inB)).toBe(false); + }); + + test('an allow-listed org beats a rollout that excludes it, like actors and roles do', () => { + expect(evaluateTargeting('a.flag', { default: false, rollout: 0, orgs: ['org-a'] }, inA)).toBe( + true, + ); + }); + + test('bucketBy org puts a whole org on one side of a 10% rollout', () => { + // `org-12` buckets at 0 and `org-1` at 78 for this key — see the pins in bucket.test.ts. + const targeting: FlagTargeting = { default: false, rollout: 10, bucketBy: 'org' }; + const members = (orgId: string): Actor[] => + Array.from({ length: 30 }, (_unused, index) => + userActor({ id: `member-${orgId}-${index}`, orgId }), + ); + for (const member of members('org-12')) { + expect(evaluateTargeting('billing.export', targeting, member)).toBe(true); + } + for (const member of members('org-1')) { + expect(evaluateTargeting('billing.export', targeting, member)).toBe(false); + } + }); + + test('the same (flag, org) buckets identically on every call', () => { + const targeting: FlagTargeting = { default: false, rollout: 50, bucketBy: 'org' }; + const first = evaluateTargeting('billing.export', targeting, inA); + for (let call = 0; call < 500; call += 1) { + expect(evaluateTargeting('billing.export', targeting, alsoInA)).toBe(first); + } + }); + + test('the default axis is still the actor, so a declared rollout is unchanged', () => { + // `user-1` and `user-2` share an org and land on opposite sides — that is actor bucketing, + // and it stays the default so no shipped flag changes answer. + const targeting: FlagTargeting = { default: false, rollout: 60 }; + expect(evaluateTargeting('billing.export', targeting, inA)).toBe(true); + expect(evaluateTargeting('billing.export', targeting, alsoInA)).toBe(false); + }); + + test('a null actor still gets the default — there is no context at all to be wrong about', () => { + expect(evaluateTargeting('a.flag', { default: false, orgs: ['org-a'] }, null)).toBe(false); + expect( + evaluateTargeting('a.flag', { default: false, rollout: 100, bucketBy: 'org' }, null), + ).toBe(false); + }); +}); + +/** + * The general axis. Treasury's `flipper_id` is `":"` and its gates OR across whichever + * records are in play — a workspace, a bank integration, a bank connection. `subjects` is that, + * with `orgs` kept as the shorthand for the 90% case. + */ +describe('unit · arbitrary record subjects', () => { + const actor = userActor({ id: 'user-1', orgId: 'org-a' }); + const bbva = { bank: 'bank_integration:bbva' }; + const santander = { bank: 'bank_integration:santander' }; + + test('a record kind the evaluation context does not carry throws', () => { + expect( + caught(() => + evaluateTargeting( + 'a.flag', + { default: false, subjects: { bank: ['bank_integration:bbva'] } }, + actor, + undefined, + ), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an allow list on a record kind is on for that record and off for another', () => { + const targeting: FlagTargeting = { + default: false, + subjects: { bank: ['bank_integration:bbva'] }, + }; + expect(evaluateTargeting('a.flag', targeting, actor, bbva)).toBe(true); + expect(evaluateTargeting('a.flag', targeting, actor, santander)).toBe(false); + }); + + test('several record kinds are ORed, the way Flipper ORs the actors passed to one call', () => { + const targeting: FlagTargeting = { + default: false, + subjects: { bank: ['bank_integration:bbva'], device: ['device-9'] }, + }; + expect( + evaluateTargeting('a.flag', targeting, actor, { + bank: 'bank_integration:x', + device: 'device-9', + }), + ).toBe(true); + expect( + evaluateTargeting('a.flag', targeting, actor, { + bank: 'bank_integration:bbva', + device: 'device-1', + }), + ).toBe(true); + }); + + test('a missing kind throws even when an earlier kind already matched', () => { + // Order independence. If the match short-circuited, whether this call answered `true` or + // raised would depend on the order the keys happen to sit in the declaration — the same + // input giving two different behaviours, which is worse than either one alone. + const targeting: FlagTargeting = { + default: false, + subjects: { bank: ['bank_integration:bbva'], device: ['device-9'] }, + }; + expect(caught(() => evaluateTargeting('a.flag', targeting, actor, bbva))).toBeUltimateError( + 'X_FLAG_SUBJECT_REQUIRED', + ); + }); + + test('bucketBy a record kind puts a whole record on one side of a rollout', () => { + // `bank_integration:bbva` buckets at 39 and `:santander` at 86 for this key — pinned below. + const targeting: FlagTargeting = { default: false, rollout: 50, bucketBy: 'bank' }; + expect(evaluateTargeting('scraper.persist-profile', targeting, actor, bbva)).toBe(true); + expect(evaluateTargeting('scraper.persist-profile', targeting, actor, santander)).toBe(false); + }); + + test('every actor on the same record lands on the same side, whoever is calling', () => { + const targeting: FlagTargeting = { default: false, rollout: 50, bucketBy: 'bank' }; + for (let index = 0; index < 50; index += 1) { + const caller = userActor({ id: `user-${index}`, orgId: `org-${index}` }); + expect(evaluateTargeting('scraper.persist-profile', targeting, caller, bbva)).toBe(true); + } + }); + + test('bucketBy a record kind the context does not carry throws rather than bucketing the actor', () => { + expect( + caught(() => + evaluateTargeting( + 'a.flag', + { default: false, rollout: 50, bucketBy: 'bank' }, + actor, + undefined, + ), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('orgs is the same mechanism as subjects, spelled shorter', () => { + const shorthand: FlagTargeting = { default: false, orgs: ['org-a'] }; + expect(evaluateTargeting('a.flag', shorthand, actor, undefined)).toBe(true); + expect( + evaluateTargeting( + 'a.flag', + shorthand, + userActor({ id: 'user-2', orgId: 'org-b' }), + undefined, + ), + ).toBe(false); + }); +}); + describe('unit · assertTargeting', () => { test('refuses a fractional rollout, which reads as half and means nobody', () => { const thrown = caught(() => { @@ -84,7 +281,67 @@ describe('unit · assertTargeting', () => { ).toBeUltimateError('X_FLAG_TARGETING_INVALID'); }); + test('refuses a subjects entry naming a built-in kind — orgs and actors are the one spelling', () => { + expect( + caught(() => assertTargeting('a.flag', { default: false, subjects: { org: ['org-a'] } })), + ).toBeUltimateError('X_FLAG_TARGETING_INVALID'); + expect( + caught(() => assertTargeting('a.flag', { default: false, subjects: { actor: ['user-1'] } })), + ).toBeUltimateError('X_FLAG_TARGETING_INVALID'); + }); + + test('refuses subject ids a store snapshot can carry but nothing can match', () => { + const emptyKind: unknown = { default: false, subjects: { '': ['x'] } }; + expect(caught(() => assertTargeting('a.flag', emptyKind as FlagTargeting))).toBeUltimateError( + 'X_FLAG_TARGETING_INVALID', + ); + const notAList: unknown = { default: false, subjects: { bank: 'bbva' } }; + expect(caught(() => assertTargeting('a.flag', notAList as FlagTargeting))).toBeUltimateError( + 'X_FLAG_TARGETING_INVALID', + ); + const emptyId: unknown = { default: false, subjects: { bank: [''] } }; + expect(caught(() => assertTargeting('a.flag', emptyId as FlagTargeting))).toBeUltimateError( + 'X_FLAG_TARGETING_INVALID', + ); + }); + + test('refuses a blank bucketBy, which would name no kind at all', () => { + const blank: unknown = { default: false, rollout: 10, bucketBy: ' ' }; + expect(caught(() => assertTargeting('a.flag', blank as FlagTargeting))).toBeUltimateError( + 'X_FLAG_TARGETING_INVALID', + ); + }); + + test('refuses a bucketBy with no rollout — it divides nothing', () => { + expect( + caught(() => assertTargeting('a.flag', { default: false, bucketBy: 'org' })), + ).toBeUltimateError('X_FLAG_TARGETING_INVALID'); + }); + + test('accepts an app-declared bucketBy kind — the kind space is open, like a flag key', () => { + // There is no registry of kinds to check against, deliberately: a typo raises + // X_FLAG_SUBJECT_REQUIRED at the first evaluation, the same loud failure an undeclared flag + // key already gets from X_FLAG_UNKNOWN. A second declaration surface would buy little. + expect( + caught(() => assertTargeting('a.flag', { default: false, rollout: 10, bucketBy: 'bank' })), + ).toBeUndefined(); + }); + test('accepts the shapes a real declaration uses', () => { + expect( + caught(() => assertTargeting('a.flag', { default: false, orgs: ['org-a'] })), + ).toBeUndefined(); + expect( + caught(() => assertTargeting('a.flag', { default: false, rollout: 10, bucketBy: 'org' })), + ).toBeUndefined(); + expect( + caught(() => + assertTargeting('a.flag', { + default: false, + subjects: { bank: ['bank_integration:bbva'] }, + }), + ), + ).toBeUndefined(); expect(caught(() => assertTargeting('a.flag', { default: true }))).toBeUndefined(); expect( caught(() => assertTargeting('a.flag', { default: false, rollout: 25 })), diff --git a/packages/flags/src/targeting.ts b/packages/flags/src/targeting.ts index 1ef065c8..a3e0e685 100644 --- a/packages/flags/src/targeting.ts +++ b/packages/flags/src/targeting.ts @@ -5,29 +5,76 @@ import type { Actor } from '@ultimat3/core'; import { hasRole } from '@ultimat3/core'; import { BUCKETS, bucketOf } from './bucket'; import { flagTargetingInvalid } from './errors'; +import type { FlagSubjects } from './subject'; +import { BUILT_IN_SUBJECT_KINDS, isBuiltInSubjectKind, subjectIdOf } from './subject'; export interface FlagTargeting { /** The answer when no allow list and no rollout claims this actor. `false` is off, `true` is on. */ readonly default: boolean; - /** Actor ids that are always on, ahead of any rollout. */ + /** Actor ids that are always on, ahead of any rollout — shorthand for the `actor` subject kind. */ readonly actors?: readonly string[] | undefined; - /** Actor roles that are always on, ahead of any rollout. */ + /** Actor roles that are always on. NOT a subject: a role is a predicate, not an identified record. */ readonly roles?: readonly string[] | undefined; - /** Percentage of actors, 0-100 inclusive. Stable: one actor, one answer, every call. */ + /** Org ids that are always on — shorthand for the `org` subject kind, read from `actor.orgId`. */ + readonly orgs?: readonly string[] | undefined; + /** + * Allow lists for the app's own record kinds: `{ bank: ['bank_integration:bbva'] }`. One rank + * with `actors`, `roles` and `orgs` — any hit is `true`, which is the same OR Flipper applies + * across the actors handed to one `enabled?` call. + * + * Built-in kinds are refused here: `actors` and `orgs` are their one spelling. + */ + readonly subjects?: Readonly> | undefined; + /** Percentage of the bucketing subject, 0-100 inclusive. Stable: one subject, one answer. */ readonly rollout?: number | undefined; + /** + * Which subject kind the `rollout` divides — `'actor'` (the default), `'org'`, or any kind the + * call site carries. The kind space is open on purpose, like the flag key space. + * + * Bucketing by a record is what keeps it whole: an org whose members share documents, or a bank + * integration whose connections share a scraper, must be entirely on the new path or entirely + * on the old one. `'actor'` stays the default, so every flag declared before this axis existed + * answers exactly as it did. + */ + readonly bucketBy?: string | undefined; } /** * Declaration-time validation, the way `can()` validates its permission rather than waiting for a - * request. Two rules, each closing a way for a flag to look wired and decide nothing: + * request. Each rule closes a way for a flag to look wired and decide nothing: * * | Rejected | Why | * |---|---| * | `rollout: 0.5` | read as a fraction it means "half", read as a percentage it means "nobody" | * | `default: true` with a `rollout` | the two answer the same actors and disagree; there is no reading of "on for everyone, and also on for 10%" | + * | `bucketBy` with no `rollout` | it names what a rollout divides, and there is no rollout to divide | + * | a blank `bucketBy` | names no kind at all | + * | `subjects.actor` / `subjects.org` | `actors` and `orgs` are the one spelling; two would disagree | + * | a `subjects` entry that is not a list of non-empty ids | reachable from a store snapshot, and it matches nothing while reading as an allow list | + * + * The `subjects` checks narrow by hand rather than through a schema: this package's other runtime + * re-checks (`Number.isInteger`, `Date.parse`) do the same, and a dependency here would buy one + * validation on a path that must stay allocation-free. */ export function assertTargeting(key: string, targeting: FlagTargeting): void { - const { rollout } = targeting; + const { bucketBy, rollout } = targeting; + if (targeting.subjects !== undefined) assertSubjects(key, targeting.subjects); + if (bucketBy !== undefined) { + if (typeof bucketBy !== 'string' || bucketBy.trim() === '') { + throw flagTargetingInvalid( + key, + `bucketBy is ${JSON.stringify(bucketBy)}, which names no subject kind`, + `set bucketBy to a subject kind — '${BUILT_IN_SUBJECT_KINDS.join("', '")}', or one your call site passes — in defineFlag({ key: '${key}' })`, + ); + } + if (rollout === undefined) { + throw flagTargetingInvalid( + key, + `bucketBy is '${bucketBy}' with no rollout, so it divides nothing`, + `add a rollout to defineFlag({ key: '${key}' }), or remove bucketBy`, + ); + } + } if (rollout === undefined) return; if (!Number.isInteger(rollout)) { const problem = `rollout is ${rollout}; a rollout is a whole percentage, not a fraction`; @@ -41,24 +88,76 @@ export function assertTargeting(key: string, targeting: FlagTargeting): void { } } +function assertSubjects(key: string, subjects: Readonly>): void { + const fix = `give each subjects entry a kind and a list of ids — { bank: ['bank_integration:bbva'] } — in defineFlag({ key: '${key}' })`; + for (const [kind, ids] of Object.entries(subjects)) { + if (kind.trim() === '') throw flagTargetingInvalid(key, 'a subjects kind is blank', fix); + if (isBuiltInSubjectKind(kind)) { + throw flagTargetingInvalid( + key, + `subjects.${kind} restates a built-in kind`, + `use ${kind === 'org' ? 'orgs' : 'actors'} instead of subjects.${kind} in defineFlag({ key: '${key}' })`, + ); + } + if (!Array.isArray(ids)) { + throw flagTargetingInvalid(key, `subjects.${kind} is not a list of ids`, fix); + } + for (const id of ids as readonly unknown[]) { + if (typeof id !== 'string' || id === '') { + throw flagTargetingInvalid(key, `subjects.${kind} holds an id that is not a string`, fix); + } + } + } +} + /** - * Allow lists first, rollout second, declared default last. That order is the contract: an actor - * an operator explicitly named must not depend on where a hash happened to put them, which is the - * whole reason an allow list exists. + * Allow lists first, rollout second, declared default last. That order is the contract: a subject + * an operator explicitly named must not depend on where a hash happened to put it, which is the + * whole reason an allow list exists. `actors`, `roles`, `orgs` and `subjects` are ONE rank — any + * hit is `true`, so their order among themselves is not observable, which is the same OR Flipper + * applies across the actors passed to a single `enabled?` call. * * A `null` actor gets the default and nothing else. There is no id to hash, so a rollout could * only be answered by re-rolling per call — the one thing this file refuses to do. An anonymous * `Actor` DOES have an id (`anonymous`), so every anonymous visitor shares one bucket: one - * identity, one answer, which is what the anonymous actor already means everywhere else. + * identity, one answer, which is what the anonymous actor already means everywhere else. `null` + * does NOT raise `X_FLAG_SUBJECT_REQUIRED`: it says there is no evaluation context at all, and + * every such call gets the same answer, so no single subject is split — which is the failure being + * designed out. A context that exists but lacks the kind is the ambiguous case, and that throws. + * + * `subjectIdOf` is called only on the branches that need a subject, so a plain + * `{ default, rollout }` flag still allocates nothing. */ export function evaluateTargeting( key: string, targeting: FlagTargeting, actor: Actor | null, + subjects?: FlagSubjects | undefined, ): boolean { if (actor === null) return targeting.default; if (targeting.actors?.includes(actor.id) === true) return true; if (targeting.roles?.some((role) => hasRole(actor, role)) === true) return true; + if ( + targeting.orgs?.includes(subjectIdOf({ key, kind: 'org', actor, subjects, via: 'orgs' })) === + true + ) { + return true; + } + if (targeting.subjects !== undefined) { + // Every declared kind is resolved before any of them can answer, so a call site missing one + // raises whatever order the keys sit in. Short-circuiting on the first match would make the + // same inputs sometimes answer and sometimes throw, decided by declaration order. + let matched = false; + for (const [kind, ids] of Object.entries(targeting.subjects)) { + const id = subjectIdOf({ key, kind, actor, subjects, via: 'subjects' }); + if (ids.includes(id)) matched = true; + } + if (matched) return true; + } if (targeting.rollout === undefined) return targeting.default; - return bucketOf(key, actor.id) < targeting.rollout; + const subjectId = + targeting.bucketBy === undefined + ? actor.id + : subjectIdOf({ key, kind: targeting.bucketBy, actor, subjects, via: 'bucketBy' }); + return bucketOf(key, subjectId) < targeting.rollout; } diff --git a/packages/money/CLAUDE.md b/packages/money/CLAUDE.md index 8bf45b4a..b3ac798f 100644 --- a/packages/money/CLAUDE.md +++ b/packages/money/CLAUDE.md @@ -1,8 +1,11 @@ # @ultimat3/money — agent notes **Tier 1.** May import `@ultimat3/core`, `@ultimat3/schema`. No external deps, ever. -`Money = { readonly minor: number; readonly currency: string }` is the shape the whole framework -passes around. +`Money = { readonly minor: number; readonly currency: string; readonly scale?: number }` is the +shape the whole framework passes around. `scale` is the decimal exponent `minor` counts in when it +is not the currency's own — absent on every value that predates it, and absent again whenever it +would only restate the currency, so there is exactly one encoding of an amount at the natural +scale and existing JSON is untouched. **`Money` is an alias, not a declaration.** It is `@ultimat3/schema`'s `MoneyValue` — tier 0, the only tier every package may import — and `@ultimat3/entity`'s `MoneyValue` is the same alias. Never @@ -10,14 +13,18 @@ restate the shape here: it was three structural copies, the entity layer's had a and a row that layer decoded therefore threw inside `JSON.stringify` and failed `t.money`. That is also why `minor` is a `number` and stays one — money crosses every wire this framework projects, and `JSON.stringify` refuses a bigint. `packages/entity/src/type-pins.ts` fails the build if the -alias is re-declared, if `minor` widens back to a `bigint`, or if either field loses `readonly`. +alias is re-declared, if `minor` widens back to a `bigint`, if any field loses `readonly`, if a +fourth field appears — or if `scale` ever stops being optional, which is the pin that says the +shape is still additive and this is still a minor version. ## Boundary | File | Single responsibility | |---|---| | `money.ts` | the value type + constructors (`money`, `fromDecimal`, `toDecimalString`) | -| `currency.ts` | ISO-4217 table + minor-unit exponent. Every scale derives from here. | +| `currency.ts` | ISO-4217 table + minor-unit exponent. Every natural scale derives from here. | +| `scale.ts` | what decimal place a value's `minor` counts (`moneyScale`), which scales are legal (`assertScale`), and the exact bigint widening every comparison starts with (`minorAt`) | +| `rescale.ts` | moving between scales: widening exact, narrowing only with a named mode | | `arithmetic.ts` | add/subtract/multiply/compare, refuses mixed currencies | | `allocate.ts` | largest-remainder splits that preserve the total | | `factor.ts` | the exact fraction a scaling factor's decimal spelling names. `factorFraction` is internal — never exported; the `Fraction` **type** is public, because `ExchangeRate.ratio` is one | @@ -28,7 +35,18 @@ alias is re-declared, if `minor` widens back to a `bigint`, or if either field l ## Rules - Never a float in a stored or returned amount. `fromDecimal` takes a **string**. -- Never `/ 100`. Use `scaleOf(currency)` / `exponentOf(currency)`. +- Never `/ 100`, and never `exponentOf(amount.currency)` for a value's own precision — that is + `moneyScale(amount)`, which falls back to the currency and is right for both. `exponentOf` and + `scaleOf` still answer for a *currency*, which is a different question. +- **Two scales meet at the finer one, never the coarser.** `add`, `subtract` and `compare` + widen through `minorAt` (bigint, exact) before they do anything else, so a sub-cent fee added to + a cent survives and a comparison answers where storing the widened value would rightly be + refused. Rounding down to the coarser scale would silently delete the smaller operand. +- **Narrowing a scale names its mode at the call.** `rescale(m, 2)` throws rather than drop a + digit; `rescale(m, 2, 'half-up')` is the same rule `fromDecimal` applies to excess precision. +- **`money()` is the only place the canonical form is decided.** It drops a `scale` equal to the + currency's exponent, so every constructor, every arithmetic result and every allocation part + agree on one encoding without any of them repeating the rule. - Never combine currencies without `convert()` first. - Never round without naming a `RoundingMode` in the call or accepting the stated default. - **Never scale in floats and round after.** `multiply`, `divide` and `convert` take the factor's diff --git a/packages/money/README.md b/packages/money/README.md index 1e5504dc..23f3221d 100644 --- a/packages/money/README.md +++ b/packages/money/README.md @@ -15,7 +15,7 @@ read rather than rounding it. → [Money](https://github.com/developerz-ai/ultim |---|---|---| | Amount | integer minor units (`1299`) | `Intl.NumberFormat`, `style: 'currency'` | | Currency | ISO-4217 code (`'EUR'`) | fraction digits derived from its exponent | -| Scale | never | `10 ** exponentOf(currency)` — never a literal `/ 100` | +| Scale | only when finer than the currency's (`scale: 6`) | `10 ** moneyScale(amount)` — never a literal `/ 100` | | FX rate | explicit argument + timestamp | recorded on the converted value | ## Use @@ -37,6 +37,31 @@ add(price, money(500, 'USD')); // throws X_CURRENCY_MISMATCH `fromDecimal` scales by it (`'1.234'` KWD → 1234), `toDecimalString` reverses it, and `formatMoney` sets the fraction digits from it. Hardcoding `/ 100` is a JPY bug and a KWD bug. +## Sub-cent amounts carry a scale + +`money(2, 'USD', 6)` is $0.000002 — `minor` counting 10⁻⁶ instead of the currency's own 10⁻². +A value that names no scale means the currency's, which is every amount that already exists, so +nothing about `{ minor, currency }` changes: same shape, same JSON, same columns. + +```ts +moneyScale(money(1299, 'EUR')); // 2 — the currency's own +moneyScale(money(2, 'USD', 6)); // 6 +rescale(money(80, 'USD'), 8); // $0.80 as 80,000,000 hundred-millionths +rescale(money(1_234_567, 'USD', 6), 2); // throws X_MONEY_NOT_INTEGER — digits would go +rescale(money(1_234_567, 'USD', 6), 2, 'half-up'); // 123¢, the loss named at the call +fromDecimal('0.000002', 'USD', { scale: 6 }); +add(money(1, 'USD'), money(2, 'USD', 6)); // meets at scale 6: 10002, nothing lost +``` + +Arithmetic normalises to the *finer* of two scales, never the coarser — adding a sub-cent fee to +a cent cannot round the fee away. `compare` and `equals` read the value rather than the encoding, +so 1299 EUR and 12,990,000 EUR at scale 6 are one amount. `multiply`, `divide`, `negate` and +`allocate` keep the scale they were handed. Widening is exact and free; narrowing needs a +`RoundingMode` at the call site, exactly as excess precision does in `fromDecimal`. + +It exists because whole cents could not name the cost of a model call: $0.0002 rounded up to 1¢ +is ~50x, and a budget built on that number is fiction. The alternative was a second money type. + ## Allocation `allocate(money(100, 'USD'), 3)` → `34, 33, 33`. Largest-remainder split: floor every part, @@ -67,7 +92,8 @@ stays the readable number the audit trail records. | Code | When | |---|---| -| `X_MONEY_NOT_INTEGER` | fractional minor units, or a decimal string more precise than the currency | +| `X_MONEY_NOT_INTEGER` | fractional minor units, a decimal string more precise than the scale, or a `rescale` that would drop a digit with no mode named | +| `X_MONEY_SCALE_INVALID` | a scale that is not a whole number of decimal places in 0…15 | | `X_CURRENCY_UNKNOWN` | code not in the ISO-4217 table | | `X_CURRENCY_MISMATCH` | arithmetic across two currencies | | `X_ALLOCATION_INVALID` | bad part count, empty/negative/all-zero ratios, percentages ≠ 100 | diff --git a/packages/money/src/allocate.test.ts b/packages/money/src/allocate.test.ts index 1e9a3d32..fe28e3f4 100644 --- a/packages/money/src/allocate.test.ts +++ b/packages/money/src/allocate.test.ts @@ -71,6 +71,35 @@ describe('allocateByPercentages', () => { }); }); +describe('allocation at a scale of its own', () => { + test('the 100.01-across-3 property holds at scale 2 and at scale 6 alike', () => { + const cents = allocate(money(10_001, 'USD'), 3); + expect(minors(cents)).toEqual([3334, 3334, 3333]); + expect(sum(cents)).toEqual({ minor: 10_001, currency: 'USD' }); + + const micros = allocate(money(100_010_000, 'USD', 6), 3); + expect(minors(micros)).toEqual([33_336_667, 33_336_667, 33_336_666]); + expect(sum(micros)).toEqual({ minor: 100_010_000, currency: 'USD', scale: 6 }); + }); + + test('every part carries the total’s scale', () => { + for (const part of allocateByRatios(money(100, 'USD', 6), [70, 20, 10])) { + expect(part.scale).toBe(6); + } + }); + + test('the split stays exact past 2^53, where the float product silently was not', () => { + // `magnitude * ratio` overflowed the exact-integer range and floored to the wrong part — + // a scale of 6 makes an amount that large 10,000x easier to reach. + const total = money(9_007_199_254_740_991, 'USD', 6); + const parts = allocateByRatios(total, [1, 1, 1]); + expect(sum(parts)).toEqual(total); + expect(minors(parts)).toEqual([ + 3_002_399_751_580_331, 3_002_399_751_580_330, 3_002_399_751_580_330, + ]); + }); +}); + function codeOf(run: () => unknown): string { try { run(); diff --git a/packages/money/src/allocate.ts b/packages/money/src/allocate.ts index 941f849e..8c3f08a5 100644 --- a/packages/money/src/allocate.ts +++ b/packages/money/src/allocate.ts @@ -10,7 +10,9 @@ import { assertSameCurrency } from './arithmetic'; import { allocationInvalid } from './errors'; -import { type Money, money } from './money'; +import { factorFraction } from './factor'; +import { formatMoneyDebug, type Money, money } from './money'; +import { minorAt, moneyScale } from './scale'; /** Split into `parts` equal shares. `allocate(money(100,'USD'), 3)` → 34, 33, 33. */ export function allocate(amount: Money, parts: number): Money[] { @@ -26,26 +28,22 @@ export function allocate(amount: Money, parts: number): Money[] { */ export function allocateByRatios(amount: Money, ratios: readonly number[]): Money[] { if (ratios.length === 0) throw allocationInvalid('ratios must not be empty'); - let total = 0; - for (const ratio of ratios) { - if (!Number.isFinite(ratio) || ratio < 0) { - throw allocationInvalid(`ratios must be finite and non-negative, got ${String(ratio)}`); - } - total += ratio; - } - if (total <= 0) throw allocationInvalid('ratios must not all be zero'); + const weights = weigh(ratios); + let total = 0n; + for (const weight of weights) total += weight; + if (total <= 0n) throw allocationInvalid('ratios must not all be zero'); const sign = amount.minor < 0 ? -1 : 1; - const magnitude = Math.abs(amount.minor); + const magnitude = BigInt(Math.abs(amount.minor)); - const floors: number[] = []; - const remainders: number[] = []; - let assigned = 0; - for (const ratio of ratios) { - const exact = (magnitude * ratio) / total; - const floor = Math.floor(exact); + const floors: bigint[] = []; + const remainders: bigint[] = []; + let assigned = 0n; + for (const weight of weights) { + const exact = magnitude * weight; + const floor = exact / total; floors.push(floor); - remainders.push(exact - floor); + remainders.push(exact % total); assigned += floor; } @@ -54,14 +52,38 @@ export function allocateByRatios(amount: Money, ratios: readonly number[]): Mone let leftover = magnitude - assigned; const order = remainders .map((remainder, index) => ({ remainder, index })) - .sort((a, b) => b.remainder - a.remainder || a.index - b.index); + .sort((a, b) => + a.remainder === b.remainder ? a.index - b.index : a.remainder < b.remainder ? 1 : -1, + ); for (const { index } of order) { - if (leftover <= 0) break; - floors[index] = (floors[index] ?? 0) + 1; - leftover -= 1; + if (leftover <= 0n) break; + floors[index] = (floors[index] ?? 0n) + 1n; + leftover -= 1n; } - return floors.map((minor) => money(sign * minor, amount.currency)); + return floors.map((minor) => money(sign * Number(minor), amount.currency, amount.scale)); +} + +/** + * The ratios as exact integer weights over one common denominator. + * + * `(magnitude * ratio) / total` in floats was only exact while the product stayed under 2^53 — + * true of most cent amounts and false of the same invoice held in micros, where the floor came + * out one unit low and largest-remainder handed the difference to the wrong part. Every + * denominator `factorFraction` produces is a power of ten, so the common one is just the largest. + */ +function weigh(ratios: readonly number[]): bigint[] { + const fractions = ratios.map((ratio) => { + if (!Number.isFinite(ratio) || ratio < 0) { + throw allocationInvalid(`ratios must be finite and non-negative, got ${String(ratio)}`); + } + return factorFraction(ratio); + }); + let common = 1n; + for (const fraction of fractions) { + if (fraction.denominator > common) common = fraction.denominator; + } + return fractions.map((fraction) => fraction.numerator * (common / fraction.denominator)); } /** @@ -78,14 +100,19 @@ export function allocateByPercentages(amount: Money, percentages: readonly numbe /** Guard for callers building their own splits: parts must reconstruct the whole. */ export function assertAllocationSums(amount: Money, parts: readonly Money[]): void { - let total = 0; + let scale = moneyScale(amount); for (const part of parts) { assertSameCurrency(amount, part); - total += part.minor; + scale = Math.max(scale, moneyScale(part)); } - if (total !== amount.minor) { + // Summed at the finest scale present, so parts split finer than the whole still reconcile + // against it rather than reading as a total that lost everything below a cent. + let total = 0n; + for (const part of parts) total += minorAt(part, scale); + const whole = minorAt(amount, scale); + if (total !== whole) { throw allocationInvalid( - `allocation of ${amount.currency} ${amount.minor} sums to ${total} — ${amount.minor - total} minor unit(s) lost`, + `allocation of ${formatMoneyDebug(amount)} sums to ${total} at scale ${scale} — ${whole - total} minor unit(s) lost`, ); } } diff --git a/packages/money/src/arithmetic.test.ts b/packages/money/src/arithmetic.test.ts index 48d35d54..3daeff6d 100644 --- a/packages/money/src/arithmetic.test.ts +++ b/packages/money/src/arithmetic.test.ts @@ -100,3 +100,48 @@ describe('scaling is exact, not an IEEE-754 product', () => { expect(codeOf(() => multiply(money(1_000_000_000, 'EUR'), 1e9))).toBe('X_MONEY_NOT_INTEGER'); }); }); + +describe('mixed scales', () => { + test('add and subtract normalise to the finer scale, losing nothing', () => { + // 1¢ + $0.000002 is not 1¢, and it is not two decisions either. + expect(add(money(1, 'USD'), money(2, 'USD', 6))).toEqual({ + minor: 10_002, + currency: 'USD', + scale: 6, + }); + expect(subtract(money(1, 'USD'), money(2, 'USD', 6))).toEqual({ + minor: 9998, + currency: 'USD', + scale: 6, + }); + expect(sum([money(2, 'USD', 6), money(1, 'USD')])).toEqual({ + minor: 10_002, + currency: 'USD', + scale: 6, + }); + }); + + test('two currencies still refuse each other, whatever their scales', () => { + expect(codeOf(() => add(money(2, 'USD', 6), money(1, 'EUR')))).toBe('X_CURRENCY_MISMATCH'); + }); + + test('compare reads the value, so a finer encoding is not automatically larger', () => { + expect(compare(money(1, 'USD'), money(10_000, 'USD', 6))).toBe(0); + expect(compare(money(1, 'USD'), money(10_001, 'USD', 6))).toBe(-1); + expect(compare(money(1, 'USD'), money(9999, 'USD', 6))).toBe(1); + // A comparison must not throw where the widened value would leave the safe-integer range. + expect(compare(money(Number.MAX_SAFE_INTEGER, 'USD'), money(1, 'USD', 6))).toBe(1); + expect(max(money(1, 'USD'), money(10_001, 'USD', 6))).toEqual({ + minor: 10_001, + currency: 'USD', + scale: 6, + }); + }); + + test('multiply, divide, negate and absolute keep the scale they were handed', () => { + expect(multiply(money(2, 'USD', 6), 3)).toEqual({ minor: 6, currency: 'USD', scale: 6 }); + expect(divide(money(10, 'USD', 6), 4)).toEqual({ minor: 3, currency: 'USD', scale: 6 }); + expect(negate(money(2, 'USD', 6))).toEqual({ minor: -2, currency: 'USD', scale: 6 }); + expect(isZero(money(0, 'USD', 6))).toBe(true); + }); +}); diff --git a/packages/money/src/arithmetic.ts b/packages/money/src/arithmetic.ts index 3abd647b..b05422fb 100644 --- a/packages/money/src/arithmetic.ts +++ b/packages/money/src/arithmetic.ts @@ -7,6 +7,7 @@ import { allocationInvalid, currencyMismatch, currencyRequired } from './errors' import { factorFraction } from './factor'; import { type Money, money } from './money'; import { DEFAULT_ROUNDING, type RoundingMode, roundRatio } from './rounding'; +import { commonScale, minorAt } from './scale'; /** Throws `X_CURRENCY_MISMATCH` unless both operands carry the same currency. */ export function assertSameCurrency(left: Money, right: Money): string { @@ -14,14 +15,20 @@ export function assertSameCurrency(left: Money, right: Money): string { return left.currency; } +/** + * Two operands meet at the finer of their scales, which is exact for both — the coarser one is + * widened, never the finer one rounded, so adding a sub-cent fee to a cent cannot lose the fee. + */ export function add(left: Money, right: Money): Money { const currency = assertSameCurrency(left, right); - return money(left.minor + right.minor, currency); + const scale = commonScale(left, right); + return money(Number(minorAt(left, scale) + minorAt(right, scale)), currency, scale); } export function subtract(left: Money, right: Money): Money { const currency = assertSameCurrency(left, right); - return money(left.minor - right.minor, currency); + const scale = commonScale(left, right); + return money(Number(minorAt(left, scale) - minorAt(right, scale)), currency, scale); } /** Every addend must share one currency; an empty list needs an explicit currency. */ @@ -44,10 +51,12 @@ export function multiply( factor: number, mode: RoundingMode = DEFAULT_ROUNDING, ): Money { - const scale = factorFraction(factor); + const ratio = factorFraction(factor); + // Scale-preserving: a fee on a micro-priced amount stays a micro-priced amount. return money( - roundRatio(BigInt(amount.minor) * scale.numerator, scale.denominator, mode), + roundRatio(BigInt(amount.minor) * ratio.numerator, ratio.denominator, mode), amount.currency, + amount.scale, ); } @@ -64,26 +73,34 @@ export function divide( if (divisor === 0) { throw allocationInvalid('cannot divide money by zero — use allocate() to split a total'); } - const scale = factorFraction(divisor); + const ratio = factorFraction(divisor); return money( - roundRatio(BigInt(amount.minor) * scale.denominator, scale.numerator, mode), + roundRatio(BigInt(amount.minor) * ratio.denominator, ratio.numerator, mode), amount.currency, + amount.scale, ); } export function negate(amount: Money): Money { - return money(-amount.minor, amount.currency); + return money(-amount.minor, amount.currency, amount.scale); } export function absolute(amount: Money): Money { - return money(Math.abs(amount.minor), amount.currency); + return money(Math.abs(amount.minor), amount.currency, amount.scale); } -/** `-1 | 0 | 1`, comparable currencies only. */ +/** + * `-1 | 0 | 1`, comparable currencies only — and comparing the value, not the encoding, so a + * finer scale is not automatically the larger number. Widened as bigints on purpose: a comparison + * must answer where storing the widened value would rightly be refused. + */ export function compare(left: Money, right: Money): -1 | 0 | 1 { assertSameCurrency(left, right); - if (left.minor < right.minor) return -1; - return left.minor > right.minor ? 1 : 0; + const scale = commonScale(left, right); + const leftMinor = minorAt(left, scale); + const rightMinor = minorAt(right, scale); + if (leftMinor < rightMinor) return -1; + return leftMinor > rightMinor ? 1 : 0; } export function isZero(amount: Money): boolean { diff --git a/packages/money/src/errors.ts b/packages/money/src/errors.ts index 81b624a3..bbacf27a 100644 --- a/packages/money/src/errors.ts +++ b/packages/money/src/errors.ts @@ -4,6 +4,7 @@ */ import { registerErrorCodes, UltimateError } from '@ultimat3/core'; +import { MAX_MONEY_SCALE } from '@ultimat3/schema'; export const MONEY_ERROR_CODES = [ 'X_MONEY_NOT_INTEGER', @@ -11,6 +12,7 @@ export const MONEY_ERROR_CODES = [ 'X_CURRENCY_MISMATCH', 'X_ALLOCATION_INVALID', 'X_RATE_MISSING', + 'X_MONEY_SCALE_INVALID', ] as const; export type MoneyErrorCode = (typeof MONEY_ERROR_CODES)[number]; @@ -21,6 +23,7 @@ export const MONEY_ERROR_TITLES: Readonly> = { X_CURRENCY_MISMATCH: 'two Money values in different currencies', X_ALLOCATION_INVALID: 'split ratios or part count are unusable', X_RATE_MISSING: 'no FX rate for the pair', + X_MONEY_SCALE_INVALID: 'a Money.scale that is not a usable decimal exponent', }; // Titles must be registered for `format()` to render the contract's first line. Every code above is @@ -64,8 +67,47 @@ export function notRoundable(value: number): MoneyError { export function decimalTooPrecise(value: string, currency: string, exponent: number): MoneyError { return new MoneyError({ code: 'X_MONEY_NOT_INTEGER', - cause: `"${value}" has more fraction digits than ${currency} has minor units (${exponent})`, - fix: `pass { rounding: 'half-up' } to fromDecimal to accept the loss of precision on purpose`, + cause: `"${value}" has more than ${exponent} fraction digit(s), which is all ${currency} is being counted in`, + fix: `fromDecimal('${value}', '${currency}', { scale: ${countFractionDigits(value)} }) to keep every digit, or { rounding: 'half-up' } to lose them on purpose`, + }); +} + +function countFractionDigits(value: string): number { + return value.trim().split('.')[1]?.length ?? 0; +} + +/** A scale outside 0…MAX_MONEY_SCALE names no decimal place a `minor` could count in. */ +export function scaleInvalid(scale: number): MoneyError { + return new MoneyError({ + code: 'X_MONEY_SCALE_INVALID', + cause: `a money scale must be a whole number of decimal places between 0 and ${MAX_MONEY_SCALE}, got ${String(scale)}`, + fix: `use a scale in range — money(minor, currency, 6) for micros, or omit it for the currency's own minor unit`, + }); +} + +/** Widening is exact; narrowing is a rounding decision, and `minorAt` does not make those. */ +export function scaleNotWidening(from: number, to: number): MoneyError { + return new MoneyError({ + code: 'X_MONEY_SCALE_INVALID', + cause: `cannot restate a value at scale ${from} as scale ${to} without dropping digits`, + fix: `rescale(amount, ${to}, 'half-up') — narrowing needs the mode named at the call`, + }); +} + +/** + * A narrowing that would drop a non-zero digit. Reported as `X_MONEY_NOT_INTEGER` because that is + * literally what it would produce — a fractional count of minor units — and the same situation + * `fromDecimal` already answers with that code. + */ +export function rescaleNotExact( + amount: { readonly minor: number; readonly currency: string }, + from: number, + to: number, +): MoneyError { + return new MoneyError({ + code: 'X_MONEY_NOT_INTEGER', + cause: `${amount.currency} ${amount.minor} at scale ${from} is not a whole number of minor units at scale ${to}`, + fix: `rescale(amount, ${to}, 'half-up') — name the mode, or keep the value at scale ${from}`, }); } diff --git a/packages/money/src/format.ts b/packages/money/src/format.ts index 6a43645a..92773647 100644 --- a/packages/money/src/format.ts +++ b/packages/money/src/format.ts @@ -5,6 +5,7 @@ import { exponentOf } from './currency'; import { type Money, toDecimalNumber } from './money'; +import { moneyScale } from './scale'; export interface FormatMoneyOptions { /** How the currency appears: `€1,299.00` / `EUR 1,299.00` / `1,299.00 euros`. */ @@ -16,7 +17,8 @@ export interface FormatMoneyOptions { accounting?: boolean; /** Drop `.00` on whole amounts — price lists, never invoices. */ trimZeroFraction?: boolean; - /** Force a digit count; defaults to the currency's minor-unit exponent. */ + /** Force a digit count; defaults to the value's own scale, which is the currency's unless + * the amount names a finer one. */ fractionDigits?: number; /** `never` disables grouping separators. */ grouping?: 'auto' | 'never'; @@ -52,18 +54,25 @@ export function formatMoneyParts( locale: string, options: FormatMoneyOptions = {}, ): Intl.NumberFormatPart[] { - return formatterFor(amount.currency, locale, options).formatToParts(toDecimalNumber(amount)); + return formatterFor(amount.currency, locale, options, moneyScale(amount)).formatToParts( + toDecimalNumber(amount), + ); } /** The symbol alone, e.g. for an input prefix: `€`, `¥`, `KD`. */ export function currencySymbol(currency: string, locale: string): string { - const parts = formatterFor(currency, locale, { display: 'narrowSymbol' }).formatToParts(0); + const parts = formatterFor( + currency, + locale, + { display: 'narrowSymbol' }, + exponentOf(currency), + ).formatToParts(0); return parts.find((part) => part.type === 'currency')?.value ?? currency; } /** Digits only, no symbol — for editable inputs and CSV exports. */ export function formatMoneyDecimal(amount: Money, locale: string): string { - const digits = exponentOf(amount.currency); + const digits = moneyScale(amount); return new Intl.NumberFormat(locale, { style: 'decimal', minimumFractionDigits: digits, @@ -74,12 +83,16 @@ export function formatMoneyDecimal(amount: Money, locale: string): string { const cache = new Map(); +/** + * `scale` is the amount's own, not the currency's: rendering $0.000002 with two digits shows + * `$0.00`, which is the sub-cent bug back again, in the one place a human would read it. + */ function formatterFor( currency: string, locale: string, options: FormatMoneyOptions, + exponent: number, ): Intl.NumberFormat { - const exponent = exponentOf(currency); const digits = options.fractionDigits ?? (options.trimZeroFraction === true ? undefined : exponent); const sign = options.accounting === true ? 'accounting' : 'standard'; diff --git a/packages/money/src/index.ts b/packages/money/src/index.ts index 496a6d20..57fccda8 100644 --- a/packages/money/src/index.ts +++ b/packages/money/src/index.ts @@ -56,6 +56,9 @@ export { type MoneyErrorCode, moneyNotInteger, rateMissing, + rescaleNotExact, + scaleInvalid, + scaleNotWidening, } from './errors'; /** `ExchangeRate.ratio` is one of these; a provider with an exact rate writes the pair itself. */ export type { Fraction } from './factor'; @@ -79,6 +82,7 @@ export { toDecimalString, zero, } from './money'; +export { rescale } from './rescale'; export { DEFAULT_ROUNDING, ROUNDING_MODES, @@ -86,3 +90,4 @@ export { roundToDigits, roundToInteger, } from './rounding'; +export { assertScale, commonScale, MAX_MONEY_SCALE, minorAt, moneyScale } from './scale'; diff --git a/packages/money/src/money.test.ts b/packages/money/src/money.test.ts index fca110dd..25d43adf 100644 --- a/packages/money/src/money.test.ts +++ b/packages/money/src/money.test.ts @@ -1,5 +1,14 @@ import { describe, expect, test } from 'bun:test'; -import { fromDecimal, isMoney, money, toDecimalString, zero } from './money'; +import { + equals, + formatMoneyDebug, + fromDecimal, + isMoney, + money, + toDecimalNumber, + toDecimalString, + zero, +} from './money'; describe('money', () => { test('rejects a fractional minor amount with X_MONEY_NOT_INTEGER', () => { @@ -70,6 +79,50 @@ describe('isMoney', () => { expect(isMoney({ minor: 100 })).toBe(false); expect(isMoney(null)).toBe(false); }); + + test('a scale is optional, and an unusable one is not money', () => { + expect(isMoney({ minor: 2, currency: 'USD', scale: 6 })).toBe(true); + expect(isMoney({ minor: 2, currency: 'USD', scale: 6.5 })).toBe(false); + expect(isMoney({ minor: 2, currency: 'USD', scale: -1 })).toBe(false); + expect(isMoney({ minor: 2, currency: 'USD', scale: '6' })).toBe(false); + }); +}); + +describe('a money value at a scale of its own', () => { + test('the constructor takes one, and omits the key at the currency’s own scale', () => { + expect(money(2, 'USD', 6)).toEqual({ minor: 2, currency: 'USD', scale: 6 }); + // Canonical: one encoding per value at the natural scale, so existing JSON is untouched. + expect(JSON.stringify(money(1299, 'EUR', 2))).toBe('{"minor":1299,"currency":"EUR"}'); + expect(codeOf(() => money(2, 'USD', 2.5))).toBe('X_MONEY_SCALE_INVALID'); + }); + + test('equals compares the value, not the encoding', () => { + expect(equals(money(1299, 'EUR'), money(12_990_000, 'EUR', 6))).toBe(true); + expect(equals(money(1299, 'EUR'), money(12_990_001, 'EUR', 6))).toBe(false); + expect(equals(money(1299, 'EUR'), money(1299, 'USD'))).toBe(false); + }); + + test('the decimal projections read the value’s own scale, never the currency’s', () => { + expect(toDecimalString(money(2, 'USD', 6))).toBe('0.000002'); + expect(toDecimalString(money(-2, 'USD', 6))).toBe('-0.000002'); + expect(toDecimalNumber(money(2, 'USD', 6))).toBe(0.000002); + expect(formatMoneyDebug(money(2, 'USD', 6))).toBe('USD 2e-6'); + // Unchanged for every value that carries no scale. + expect(formatMoneyDebug(money(1299, 'EUR'))).toBe('EUR 1299'); + }); + + test('fromDecimal accepts the extra digits when a scale is named for them', () => { + expect(codeOf(() => fromDecimal('0.000002', 'USD'))).toBe('X_MONEY_NOT_INTEGER'); + expect(fromDecimal('0.000002', 'USD', { scale: 6 })).toEqual({ + minor: 2, + currency: 'USD', + scale: 6, + }); + expect(toDecimalString(fromDecimal('0.000002', 'USD', { scale: 6 }))).toBe('0.000002'); + // A scale coarser than the digits still needs an explicit rounding mode. + expect(codeOf(() => fromDecimal('0.0000025', 'USD', { scale: 6 }))).toBe('X_MONEY_NOT_INTEGER'); + expect(fromDecimal('0.0000025', 'USD', { scale: 6, rounding: 'half-up' }).minor).toBe(3); + }); }); function codeOf(run: () => unknown): string { diff --git a/packages/money/src/money.ts b/packages/money/src/money.ts index 97518f79..7e44cccc 100644 --- a/packages/money/src/money.ts +++ b/packages/money/src/money.ts @@ -3,10 +3,11 @@ * There is no float anywhere in this package, and no amount without a currency. */ -import type { MoneyValue } from '@ultimat3/schema'; -import { assertCurrency, type CurrencyCode, exponentOf, scaleOf } from './currency'; +import { isMoneyScale, type MoneyValue } from '@ultimat3/schema'; +import { assertCurrency, type CurrencyCode, exponentOf } from './currency'; import { decimalNotNumeric, decimalTooPrecise, moneyNotInteger } from './errors'; import { type RoundingMode, roundToInteger } from './rounding'; +import { assertScale, minorAt, moneyScale } from './scale'; /** * `{ minor: 129900, currency: 'EUR' }` is €1,299.00. Instances are immutable, and now enforced @@ -21,11 +22,20 @@ export type Money = MoneyValue; const DECIMAL = /^([+-])?(\d+)(?:\.(\d+))?$/; -/** The only constructor. Validates the currency and rejects fractional minor units. */ -export function money(minor: number, currency: string): Money { +/** + * The only constructor. Validates the currency and rejects fractional minor units. + * + * `scale` names how many decimal places `minor` counts when the currency's own are not enough: + * `money(2, 'USD', 6)` is $0.000002. Omitted — and canonically omitted again when it says nothing + * the currency does not already say — so a value at the natural scale serializes byte-for-byte as + * it always has, and there is exactly one encoding of it. + */ +export function money(minor: number, currency: string, scale?: number): Money { const code = assertCurrency(currency); if (!Number.isSafeInteger(minor)) throw moneyNotInteger(minor, code); - return { minor, currency: code }; + if (scale === undefined) return { minor, currency: code }; + assertScale(scale); + return scale === exponentOf(code) ? { minor, currency: code } : { minor, currency: code, scale }; } export function zero(currency: string): Money { @@ -33,8 +43,13 @@ export function zero(currency: string): Money { } export interface FromDecimalOptions { - /** Required to accept a value with more precision than the currency has. */ + /** Required to accept a value with more precision than the target scale has. */ rounding?: RoundingMode; + /** + * Decimal places to keep, when the currency's own are not enough: + * `fromDecimal('0.000002', 'USD', { scale: 6 })`. Omitted, the currency decides, as before. + */ + scale?: number; } /** @@ -54,7 +69,7 @@ export function fromDecimal( throw decimalNotNumeric(value, code); } - const exponent = exponentOf(code); + const exponent = options.scale === undefined ? exponentOf(code) : assertScale(options.scale); const negative = match[1] === '-'; const fractionPart = match[3] ?? ''; @@ -70,13 +85,12 @@ export function fromDecimal( minor = roundToInteger(kept + remainder, mode); } - if (!Number.isSafeInteger(minor)) throw moneyNotInteger(minor, code); - return { minor: negative ? -minor : minor, currency: code }; + return money(negative ? -minor : minor, code, options.scale); } -/** `1299 EUR` → `'12.99'`; `1200 JPY` → `'1200'`; `1234 KWD` → `'1.234'`. */ +/** `1299 EUR` → `'12.99'`; `1200 JPY` → `'1200'`; `2 USD @ scale 6` → `'0.000002'`. */ export function toDecimalString(amount: Money): string { - const exponent = exponentOf(amount.currency); + const exponent = moneyScale(amount); const sign = amount.minor < 0 ? '-' : ''; const digits = Math.abs(amount.minor) .toString() @@ -90,23 +104,37 @@ export function toDecimalString(amount: Money): string { * currency scale is legitimate, because `Intl.NumberFormat` takes a number. */ export function toDecimalNumber(amount: Money): number { - return amount.minor / scaleOf(amount.currency); + return amount.minor / 10 ** moneyScale(amount); } export function isMoney(value: unknown): value is Money { if (value === null || typeof value !== 'object') return false; - const candidate = value as { minor?: unknown; currency?: unknown }; - return Number.isSafeInteger(candidate.minor) && typeof candidate.currency === 'string'; + const candidate = value as { minor?: unknown; currency?: unknown; scale?: unknown }; + if (!Number.isSafeInteger(candidate.minor) || typeof candidate.currency !== 'string') + return false; + return candidate.scale === undefined || isMoneyScale(candidate.scale); } -/** Same currency, same minor units. */ +/** + * Same currency, same value. Same *value*, not the same encoding: 1299 EUR and 12,990,000 EUR at + * scale 6 are one amount written two ways, and a ledger that called them different would + * reconcile against itself. + */ export function equals(left: Money, right: Money): boolean { - return left.currency === right.currency && left.minor === right.minor; + if (left.currency !== right.currency) return false; + const scale = Math.max(moneyScale(left), moneyScale(right)); + return minorAt(left, scale) === minorAt(right, scale); } -/** Stable serialization for logs, JSON columns and the manifest: `EUR 1299`. */ +/** + * Stable serialization for logs, JSON columns and the manifest: `EUR 1299`, and `USD 2e-6` for a + * value whose scale is not the currency's — unchanged for every value that carries none. + */ export function formatMoneyDebug(amount: Money): string { - return `${amount.currency} ${amount.minor}`; + const scale = amount.scale; + return scale === undefined + ? `${amount.currency} ${amount.minor}` + : `${amount.currency} ${amount.minor}e-${scale}`; } export function currencyOf(amount: Money): CurrencyCode { diff --git a/packages/money/src/rescale.test.ts b/packages/money/src/rescale.test.ts new file mode 100644 index 00000000..fa840c77 --- /dev/null +++ b/packages/money/src/rescale.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from 'bun:test'; +import { money } from './money'; +import { rescale } from './rescale'; + +describe('rescale', () => { + test('refuses to drop digits unless a rounding mode says so', () => { + const micros = money(1_234_567, 'USD', 6); + expect(codeOf(() => rescale(micros, 2))).toBe('X_MONEY_NOT_INTEGER'); + expect(causeOf(() => rescale(micros, 2))).toContain('6'); + }); + + test('widening is exact and needs no mode', () => { + expect(rescale(money(1299, 'EUR'), 6)).toEqual({ + minor: 12_990_000, + currency: 'EUR', + scale: 6, + }); + expect(rescale(money(1200, 'JPY'), 3)).toEqual({ minor: 1_200_000, currency: 'JPY', scale: 3 }); + }); + + test('narrowing with an explicit mode rounds the way the mode says', () => { + const micros = money(1_234_567, 'USD', 6); + expect(rescale(micros, 2, 'down').minor).toBe(123); + expect(rescale(micros, 2, 'up').minor).toBe(124); + expect(rescale(money(1_235_000, 'USD', 6), 2, 'half-up').minor).toBe(124); + expect(rescale(money(1_225_000, 'USD', 6), 2, 'half-even').minor).toBe(122); + expect(rescale(money(1_235_000, 'USD', 6), 2, 'half-even').minor).toBe(124); + }); + + test('a narrowing that loses nothing is exact, mode or no mode', () => { + expect(rescale(money(1_200_000, 'USD', 6), 2)).toEqual({ minor: 120, currency: 'USD' }); + }); + + test('back at the currency’s own scale the key is gone, so the value serializes as it always did', () => { + const there = rescale(money(1299, 'EUR'), 6); + expect(JSON.stringify(rescale(there, 2))).toBe('{"minor":1299,"currency":"EUR"}'); + }); + + test('refuses a scale that names no decimal place', () => { + expect(codeOf(() => rescale(money(100, 'USD'), 99))).toBe('X_MONEY_SCALE_INVALID'); + }); + + test('the sub-cent value the AI cost path could not hold: $0.80/Mtok over 200 tokens', () => { + // Truly $0.00016. Whole cents rounded it up to 1¢ — ~50x — and the budget ledger built on + // that number was fiction. + const perMillion = rescale(money(80, 'USD'), 8); + expect(perMillion.minor).toBe(80_000_000); + expect(rescale(perMillion, 2, 'half-up')).toEqual({ minor: 80, currency: 'USD' }); + }); +}); + +function codeOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return String((error as { code?: unknown }).code); + } + return 'no-throw'; +} + +function causeOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return String((error as { cause?: unknown }).cause); + } + return 'no-throw'; +} diff --git a/packages/money/src/rescale.ts b/packages/money/src/rescale.ts new file mode 100644 index 00000000..ad677220 --- /dev/null +++ b/packages/money/src/rescale.ts @@ -0,0 +1,29 @@ +/** + * Moving a money value between decimal scales. + * Widening is exact and free. Narrowing throws away digits, so it happens only when the caller + * names the rounding mode — the same bar `fromDecimal` sets for excess precision. + */ + +import { rescaleNotExact } from './errors'; +import { type Money, money } from './money'; +import { type RoundingMode, roundRatio } from './rounding'; +import { assertScale, minorAt, moneyScale } from './scale'; + +/** + * `rescale(money(80, 'USD'), 8)` → 80,000,000 hundred-millionths, the granularity a per-token + * price needs. `rescale(m, 2, 'half-up')` brings it back to cents, having named who pays for the + * digits that go. + */ +export function rescale(amount: Money, scale: number, mode?: RoundingMode): Money { + assertScale(scale); + const from = moneyScale(amount); + if (scale >= from) return money(Number(minorAt(amount, scale)), amount.currency, scale); + + const divisor = 10n ** BigInt(from - scale); + const numerator = BigInt(amount.minor); + // An exact narrowing needs no mode: nothing is being decided, so nothing has to be declared. + if (mode === undefined && numerator % divisor !== 0n) { + throw rescaleNotExact(amount, from, scale); + } + return money(roundRatio(numerator, divisor, mode), amount.currency, scale); +} diff --git a/packages/money/src/scale.test.ts b/packages/money/src/scale.test.ts new file mode 100644 index 00000000..c03161ec --- /dev/null +++ b/packages/money/src/scale.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, test } from 'bun:test'; +import { money } from './money'; +import { assertScale, MAX_MONEY_SCALE, minorAt, moneyScale } from './scale'; + +describe('moneyScale', () => { + test('a value without a scale carries its currency’s own', () => { + expect(moneyScale(money(1299, 'EUR'))).toBe(2); + expect(moneyScale(money(1200, 'JPY'))).toBe(0); + expect(moneyScale(money(1234, 'KWD'))).toBe(3); + }); + + test('an explicit scale wins, and is what makes a sub-cent amount expressible', () => { + // The AI cost path's real number: $0.000002, which cents alone rounded up to a whole 1¢. + expect(moneyScale({ minor: 2, currency: 'USD', scale: 6 })).toBe(6); + }); +}); + +describe('assertScale', () => { + test('refuses a scale that names no decimal place with X_MONEY_SCALE_INVALID', () => { + expect(codeOf(() => assertScale(-1))).toBe('X_MONEY_SCALE_INVALID'); + expect(codeOf(() => assertScale(2.5))).toBe('X_MONEY_SCALE_INVALID'); + expect(codeOf(() => assertScale(Number.NaN))).toBe('X_MONEY_SCALE_INVALID'); + }); + + test('refuses a scale past 10^15, the last power of ten that is a safe integer', () => { + expect(codeOf(() => assertScale(MAX_MONEY_SCALE + 1))).toBe('X_MONEY_SCALE_INVALID'); + expect(assertScale(MAX_MONEY_SCALE)).toBe(MAX_MONEY_SCALE); + expect(assertScale(0)).toBe(0); + }); +}); + +describe('minorAt', () => { + test('widens exactly, as a bigint, so a comparison never overflows a double', () => { + expect(minorAt(money(1299, 'EUR'), 6)).toBe(12_990_000n); + expect(minorAt({ minor: 2, currency: 'USD', scale: 6 }, 6)).toBe(2n); + // Past 2^53: the point of the bigint. `money()` would refuse the widened value, a + // comparison must not. + expect(minorAt(money(Number.MAX_SAFE_INTEGER, 'USD'), 6)).toBe( + BigInt(Number.MAX_SAFE_INTEGER) * 10_000n, + ); + }); + + test('refuses to narrow — that is a rounding decision, and rescale() owns it', () => { + expect(codeOf(() => minorAt({ minor: 2, currency: 'USD', scale: 6 }, 2))).toBe( + 'X_MONEY_SCALE_INVALID', + ); + }); +}); + +function codeOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return String((error as { code?: unknown }).code); + } + return 'no-throw'; +} diff --git a/packages/money/src/scale.ts b/packages/money/src/scale.ts new file mode 100644 index 00000000..7366b1f5 --- /dev/null +++ b/packages/money/src/scale.ts @@ -0,0 +1,50 @@ +/** + * What decimal place a money value's `minor` counts, and which scales are legal. + * A value naming none counts the currency's own minor unit — the shape every amount already had, + * which is why nothing that predates this file has to change to keep meaning what it meant. + */ + +import { isMoneyScale, MAX_MONEY_SCALE } from '@ultimat3/schema'; +import { exponentOf } from './currency'; +import { scaleInvalid, scaleNotWidening } from './errors'; +import type { Money } from './money'; + +export { MAX_MONEY_SCALE }; + +/** + * The decimal exponent this value's `minor` counts in — its own, or the currency's. + * + * Not to be confused with `scaleOf(currency)`, which is the multiplier `10 ** exponentOf(currency)`. + * This one is a count of digits, like `exponentOf`. + */ +export function moneyScale(amount: Money): number { + return amount.scale ?? exponentOf(amount.currency); +} + +/** A scale that names no decimal place is a data bug, not a formatting preference. */ +export function assertScale(scale: number): number { + if (!isMoneyScale(scale)) throw scaleInvalid(scale); + return scale; +} + +/** + * `amount.minor` restated at `scale`, exactly, as a bigint. + * + * A bigint because widening is what a comparison does first, and a comparison must not throw: + * `MAX_SAFE_INTEGER` cents restated in micros is past 2^53, which `money()` rightly refuses to + * *store* and which says nothing about whether it is larger than the value beside it. + * + * Widening only. Narrowing drops digits, and which digits go is a decision with a mode attached — + * `rescale()` owns that, out loud. + */ +export function minorAt(amount: Money, scale: number): bigint { + assertScale(scale); + const from = moneyScale(amount); + if (scale < from) throw scaleNotWidening(from, scale); + return BigInt(amount.minor) * 10n ** BigInt(scale - from); +} + +/** The finer of two scales: where two values have to meet before they can be one number. */ +export function commonScale(left: Money, right: Money): number { + return Math.max(moneyScale(left), moneyScale(right)); +} diff --git a/packages/schema/CLAUDE.md b/packages/schema/CLAUDE.md index 2fc942ff..178c931b 100644 --- a/packages/schema/CLAUDE.md +++ b/packages/schema/CLAUDE.md @@ -12,7 +12,7 @@ Tier 0. **Imports no `@ultimat3/*` package — not even `@ultimat3/core`.** | Exports | explicit in `src/index.ts`; no `export *`; a namespace member and its free function ship together (`t.nullable`/`nullableSchema`) | | Re-exports | `action`, `query`, `jobs`, `entity` re-export `t` verbatim so an authoring file imports one package — never let them wrap or copy it | -Module order (no cycles): `node → builder → validators → provider → t`. +Module order (no cycles): `node → builder → money-value → validators → provider → t`. `standard.ts` and `errors.ts` depend on nothing but each other. `SCHEMA_ERROR_CODES` in `errors.ts` is data, not a `registerErrorCodes()` call — this package is @@ -21,13 +21,23 @@ carries a duplicate of these titles and registers them unconditionally, so every real titles just by importing core. Add a code here **and** update that duplicate in the same change — `schema-error-codes-pin.test.ts` in `@ultimat3/cli` fails the build if they disagree. -`MoneyValue` in `validators.ts` is the framework's **one** declaration of a money value — tier 0 is +`MoneyValue` in `money-value.ts` — its own file, because it is the only builtin whose *shape* other +packages alias — is the framework's **one** declaration of a money value. Tier 0 is the only tier every package may import, and `@ultimat3/money`'s `Money` and `@ultimat3/entity`'s `MoneyValue` are aliases of it. Never let either restate the shape: it was three structural copies, entity's had a `bigint` `minor`, and a row that layer decoded then failed both `t.money` and `JSON.stringify`. `minor` stays a `number` for the same reason it is a `number` here — this node is the OpenAPI contract, and money crosses every wire the framework projects. +`MoneyValue.scale` is the **optional** decimal exponent `minor` counts in, `0…MAX_MONEY_SCALE` +(15, the last power of ten that is itself a safe integer). Absent means the currency's own minor +unit, which is every value that predates it — so `{ minor, currency }` parses to exactly +`{ minor, currency }`, key for key, and the validator adds nothing. What a legal scale is lives in +`isMoneyScale` here and nowhere else; `@ultimat3/money` imports it rather than restating the +bound. Adding it to the type means adding it in three more places in the same change — the node's +`properties`, `json-schema.ts` (optional, never `required`, or a generated client refuses a value +this validator accepts) and `coerce.ts` (a query string carries it as text like everything else). + `t` delegates through `schemaProvider()` on every property access — that is what makes `configureSchemaProvider()` work for modules that already imported `t`. Do not cache members. diff --git a/packages/schema/README.md b/packages/schema/README.md index 92e8bdca..b99b7dab 100644 --- a/packages/schema/README.md +++ b/packages/schema/README.md @@ -26,6 +26,7 @@ export const publishPost = t.object({ title: t.string.min(3).max(80), tags: t.array(t.slug), price: t.money, // { minor: 1999, currency: 'EUR' } — never a float + // { minor: 2, currency: 'USD', scale: 6 } is $0.000002 timeZone: t.timezone, // real IANA validation, not an annotation cursor: t.optional(t.cursor), }); diff --git a/packages/schema/src/builder.ts b/packages/schema/src/builder.ts index bbcba852..f5f73cd9 100644 --- a/packages/schema/src/builder.ts +++ b/packages/schema/src/builder.ts @@ -43,6 +43,11 @@ export function failWith(issues: readonly StandardIssue[]): CheckErr { } /** `expected uuid, received "abc"` — the message an agent can act on without guessing. */ +/** An object with own keys — not null, not an array. The gate every object-ish check opens with. */ +export function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + export function expected(what: string, value: unknown): string { return `expected ${what}, received ${describeValue(value)}`; } diff --git a/packages/schema/src/coerce.test.ts b/packages/schema/src/coerce.test.ts index a8e6552e..79bea61d 100644 --- a/packages/schema/src/coerce.test.ts +++ b/packages/schema/src/coerce.test.ts @@ -46,6 +46,13 @@ describe('coerceQuery', () => { minor: 1999, currency: 'EUR', }); + // A query string carries every field as text, scale included — leaving it a string would + // fail validation on a value the same request's `minor` was accepted for. + expect(coerceNode(t.money.node, { minor: '2', currency: 'USD', scale: '6' })).toEqual({ + minor: 2, + currency: 'USD', + scale: 6, + }); const nested = t.object({ page: t.number, inner: t.object({ live: t.boolean }) }); expect(coerceInput(nested, { page: '2', inner: { live: 'true' } })).toEqual({ page: 2, diff --git a/packages/schema/src/coerce.ts b/packages/schema/src/coerce.ts index 33d588bf..786d1a2a 100644 --- a/packages/schema/src/coerce.ts +++ b/packages/schema/src/coerce.ts @@ -8,6 +8,13 @@ import { tryIntrospect } from './provider'; const TRUE_VALUES = new Set(['1', 'true', 'yes', 'on']); const FALSE_VALUES = new Set(['0', 'false', 'no', 'off', '']); +/** A numeric string as a number, or `undefined` for anything that is not confidently one. */ +function numeric(raw: unknown): number | undefined { + if (typeof raw !== 'string' || raw.trim() === '') return undefined; + const value = Number(raw); + return Number.isFinite(value) ? value : undefined; +} + export type QuerySource = | URLSearchParams | Readonly>; @@ -63,7 +70,11 @@ export function coerceNode(node: SchemaNode, raw: unknown): unknown { if (typeof raw !== 'object') return raw; const source = raw as Record; const minor = typeof source['minor'] === 'string' ? Number(source['minor']) : source['minor']; - return Number.isFinite(minor) ? { ...source, minor } : raw; + if (!Number.isFinite(minor)) return raw; + // `scale` arrives as text from a query string exactly as `minor` does. Left a string it + // would fail validation on a value whose `minor` the same request just had converted. + const scale = numeric(source['scale']); + return { ...source, minor, ...(scale === undefined ? {} : { scale }) }; } case 'union': { // Only unambiguous single-kind unions (e.g. `number | undefined`) are safe to coerce. diff --git a/packages/schema/src/index.ts b/packages/schema/src/index.ts index f494fed3..8d0d058c 100644 --- a/packages/schema/src/index.ts +++ b/packages/schema/src/index.ts @@ -46,6 +46,8 @@ export type { ToJsonSchemaOptions, } from './json-schema'; export { nodeToJsonSchema, toJsonSchema, toMcpInputSchema } from './json-schema'; +export type { MoneyValue } from './money-value'; +export { isMoneyScale, MAX_MONEY_SCALE } from './money-value'; export type { SchemaFormat, SchemaKind, SchemaNode } from './node'; export { isSchemaNode, nodeOf, requiredKeys } from './node'; export type { SchemaProvider } from './provider'; @@ -83,7 +85,6 @@ export { export type { Infer } from './t'; export { t } from './t'; export type { - MoneyValue, NumberSchema, ObjectSchema, StringSchema, diff --git a/packages/schema/src/json-schema.test.ts b/packages/schema/src/json-schema.test.ts index 80e77633..00b5f0fa 100644 --- a/packages/schema/src/json-schema.test.ts +++ b/packages/schema/src/json-schema.test.ts @@ -51,6 +51,14 @@ describe('toJsonSchema', () => { }); }); + test('money admits an optional scale without requiring one', () => { + const money = toJsonSchema(t.money, { includeDialect: false }); + // `additionalProperties: false` is what made a scaled value fail a generated client's own + // check while the framework's validator accepted it. + expect(money.properties?.['scale']).toMatchObject({ type: 'integer', minimum: 0 }); + expect(money.required).toEqual(['minor', 'currency']); + }); + test('dialects: 2020-12 by default, draft-07 for MCP tools', () => { expect(toJsonSchema(t.object({ id: t.uuid })).$schema).toBe( 'https://json-schema.org/draft/2020-12/schema', diff --git a/packages/schema/src/json-schema.ts b/packages/schema/src/json-schema.ts index 2f0107a9..66b8557a 100644 --- a/packages/schema/src/json-schema.ts +++ b/packages/schema/src/json-schema.ts @@ -2,6 +2,7 @@ // bodies and MCP tool `inputSchema` are both this function's output, so an agent's view of an // action and an HTTP client's view can never drift. +import { MAX_MONEY_SCALE } from './money-value'; import { requiredKeys, type SchemaNode } from './node'; import { introspect } from './provider'; @@ -131,6 +132,14 @@ function convert(node: SchemaNode): JsonSchema { maximum: Number.MAX_SAFE_INTEGER, }, currency: { type: 'string', pattern: '^[A-Z]{3}$' }, + // Optional, never required: `additionalProperties: false` alone would make a generated + // client refuse a scaled amount the framework's own validator accepts. + scale: { + type: 'integer', + description: 'decimal places `minor` counts; absent means the currency’s own', + minimum: 0, + maximum: MAX_MONEY_SCALE, + }, }, required: ['minor', 'currency'], additionalProperties: false, diff --git a/packages/schema/src/money-value.test.ts b/packages/schema/src/money-value.test.ts new file mode 100644 index 00000000..20b47a6c --- /dev/null +++ b/packages/schema/src/money-value.test.ts @@ -0,0 +1,97 @@ +// Single responsibility: pins the money value's accept/reject contract at the public `validate()` +// boundary. Its own file beside `money-value.ts`, and not a block inside `validators.test.ts`, +// because this is the one builtin whose shape other packages alias — so what it accepts is a +// contract three packages read, not one validator's behaviour. + +import { describe, expect, test } from 'bun:test'; +import { MAX_MONEY_SCALE } from './money-value'; +import { validate } from './standard'; +import { builtinT } from './validators'; + +describe('builtinT.money', () => { + test('accepts a valid Money value', () => { + const result = validate(builtinT.money, { minor: 1999, currency: 'EUR' }); + expect(result.issues).toBeUndefined(); + if (result.issues === undefined) expect(result.value).toEqual({ minor: 1999, currency: 'EUR' }); + }); + + test('rejects a non-integer minor amount', () => { + const result = validate(builtinT.money, { minor: 19.99, currency: 'EUR' }); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['minor']); + }); + + test('rejects a malformed currency code', () => { + const result = validate(builtinT.money, { minor: 1999, currency: 'eur' }); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['currency']); + }); + + test('reports both issues together when minor and currency are both invalid', () => { + const result = validate(builtinT.money, { minor: 19.99, currency: 'eur' }); + expect(result.issues?.length).toBe(2); + expect(result.issues?.map((issue) => issue.path)).toEqual([['minor'], ['currency']]); + }); + + test('rejects a non-object', () => { + expect(validate(builtinT.money, 'money').issues).toBeDefined(); + }); + + test('the money node declares exactly minor, currency and an optional scale', () => { + // The runtime half of `@ultimat3/entity`'s `type-pins.ts`: that file fails the build when the + // TYPE grows a field, this fails the suite when the IR every generator reads does not grow + // the same one. A field in one and not the other is a contract two surfaces disagree about. + const properties = builtinT.money.node.properties ?? {}; + expect(Object.keys(properties)).toEqual(['minor', 'currency', 'scale']); + expect(properties['scale']?.optional).toBe(true); + expect(properties['minor']?.optional).toBeUndefined(); + expect(properties['currency']?.optional).toBeUndefined(); + }); + + test('rejects a scale that is not a whole number of decimal places', () => { + const result = validate(builtinT.money, { minor: 2, currency: 'USD', scale: 6.5 }); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['scale']); + expect(validate(builtinT.money, { minor: 2, currency: 'USD', scale: -1 }).issues).toBeDefined(); + expect( + validate(builtinT.money, { minor: 2, currency: 'USD', scale: '6' }).issues, + ).toBeDefined(); + }); + + test('rejects a scale past the representable maximum', () => { + // 10^16 is not a safe integer, so a value at that scale could not name its own unit. + expect( + validate(builtinT.money, { minor: 2, currency: 'USD', scale: MAX_MONEY_SCALE + 1 }).issues, + ).toBeDefined(); + expect( + validate(builtinT.money, { minor: 2, currency: 'USD', scale: MAX_MONEY_SCALE }).issues, + ).toBeUndefined(); + }); + + test('carries an explicit scale through, and adds none to a value without one', () => { + const scaled = validate(builtinT.money, { minor: 2, currency: 'USD', scale: 6 }); + expect(scaled.issues).toBeUndefined(); + // The sub-cent value the AI cost path could not express: $0.000002, not a rounded-up cent. + if (scaled.issues === undefined) { + expect(scaled.value).toEqual({ minor: 2, currency: 'USD', scale: 6 }); + } + const plain = validate(builtinT.money, { minor: 1999, currency: 'EUR' }); + expect(plain.issues).toBeUndefined(); + if (plain.issues === undefined) expect(Object.keys(plain.value)).toEqual(['minor', 'currency']); + }); + + test('rejects a minor amount past the safe-integer range', () => { + // `Number.isInteger(2**53)` is true and `money()`/`parseMinor` both refuse it, so accepting + // it here turned a 422 with a field path into a 500 at the row write. + const result = validate(builtinT.money, { minor: 9_007_199_254_740_992, currency: 'EUR' }); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['minor']); + expect( + validate(builtinT.money, { minor: -9_007_199_254_740_992, currency: 'EUR' }).issues, + ).toBeDefined(); + // The largest amount that IS representable still passes. + expect( + validate(builtinT.money, { minor: Number.MAX_SAFE_INTEGER, currency: 'EUR' }).issues, + ).toBeUndefined(); + }); +}); diff --git a/packages/schema/src/money-value.ts b/packages/schema/src/money-value.ts new file mode 100644 index 00000000..a2d5e546 --- /dev/null +++ b/packages/schema/src/money-value.ts @@ -0,0 +1,116 @@ +// Single responsibility: the framework's ONE money declaration and the validator that guards it. +// Split out of `validators.ts` because it is the only builtin whose *shape* other packages alias +// — `@ultimat3/money`'s `Money`, `@ultimat3/entity`'s `MoneyValue` — so it earns a file a reader +// can open by name instead of scrolling to. + +import { expected, fail, failWith, isPlainObject, makeSchema, pass, type Schema } from './builder'; +import type { StandardIssue } from './standard'; + +const CURRENCY_RE = /^[A-Z]{3}$/; + +/** + * The framework's ONE declaration of a money value. `@ultimat3/money`'s `Money` and + * `@ultimat3/entity`'s `MoneyValue` are aliases of this type, not copies of its shape — three + * structural restatements are how `minor` became a `number` here and a `bigint` there, which made + * a row the entity layer produced fail `t.money` and throw inside `JSON.stringify`. + * + * It lives at tier 0 because that is the only tier every other package may import, and `number` + * rather than `bigint` because money crosses the wire on every surface this framework projects — + * `JSON.stringify` refuses a bigint, and this node is also the OpenAPI contract. A value past + * `Number.MAX_SAFE_INTEGER` is refused HERE, at the boundary, with the field path — and again + * where it is decoded; it is never widened. + * + * Never a float, and never an amount without its currency. + */ +export interface MoneyValue { + readonly minor: number; + readonly currency: string; + /** + * Decimal places `minor` counts, when they are not the currency's own. Absent — the shape every + * existing value and every existing row still has — means the currency's natural minor unit: 2 + * for USD, 0 for JPY, 3 for KWD. `{ minor: 2, currency: 'USD', scale: 6 }` is $0.000002. + * + * It exists because a cents-only value could not name a sub-cent amount at all, so the one + * place that needed one (a model call costing $0.0002) rounded it up to a whole cent and + * reported ~50x the real spend. The alternative was a second money type, which is the axiom-1 + * violation this declaration exists to prevent. + */ + readonly scale?: number; +} + +/** + * The largest decimal exponent a money value may carry. 10^15 is the last power of ten that is + * itself a safe integer, so a finer scale could not name its own unit inside the range `minor` is + * already checked against. + */ +export const MAX_MONEY_SCALE = 15; + +/** What a legal `MoneyValue.scale` is — declared once, here, beside the type that carries it. */ +export function isMoneyScale(value: unknown): value is number { + return ( + typeof value === 'number' && Number.isInteger(value) && value >= 0 && value <= MAX_MONEY_SCALE + ); +} + +export const moneySchema: Schema = makeSchema( + { + kind: 'money', + description: 'integer minor units plus an ISO 4217 currency code', + properties: { + minor: { + kind: 'number', + integer: true, + minimum: -Number.MAX_SAFE_INTEGER, + maximum: Number.MAX_SAFE_INTEGER, + }, + currency: { kind: 'string', pattern: CURRENCY_RE.source }, + scale: { + kind: 'number', + integer: true, + optional: true, + minimum: 0, + maximum: MAX_MONEY_SCALE, + description: 'decimal places `minor` counts; absent means the currency’s own', + }, + }, + }, + (value, path) => { + if (!isPlainObject(value)) return fail(path, expected('a Money object', value)); + const minor = value['minor']; + const currency = value['currency']; + const scale = value['scale']; + const issues: StandardIssue[] = []; + // Safe, not merely whole: `money()` and `entity`'s `parseMinor` both demand a safe integer, so + // `Number.isInteger` here let 2^53 through the boundary as a 200 and failed at the row write + // as a 500 — the same value refused twice, once with a field path and once without. + if (typeof minor !== 'number' || !Number.isSafeInteger(minor)) { + issues.push({ + message: expected('a safe integer number of minor units', minor), + path: [...path, 'minor'], + }); + } + if (typeof currency !== 'string' || !CURRENCY_RE.test(currency)) { + issues.push({ + message: expected('a 3-letter ISO 4217 code', currency), + path: [...path, 'currency'], + }); + } + if (scale !== undefined && !isMoneyScale(scale)) { + issues.push({ + message: expected( + `a whole number of decimal places between 0 and ${MAX_MONEY_SCALE}`, + scale, + ), + path: [...path, 'scale'], + }); + } + if (issues.length > 0) return failWith(issues); + // The key is carried only when it was sent: a value at the currency's own scale must + // round-trip byte-for-byte, or every stored amount in every app changes shape on one parse. + return pass({ + minor: minor as number, + currency: currency as string, + ...(scale === undefined ? {} : { scale: scale as number }), + }); + }, +); diff --git a/packages/schema/src/t.ts b/packages/schema/src/t.ts index df694b77..aa039b02 100644 --- a/packages/schema/src/t.ts +++ b/packages/schema/src/t.ts @@ -3,15 +3,10 @@ // even for modules that captured `t` at import time. import type { AnySchema, Schema, Shape } from './builder'; +import type { MoneyValue } from './money-value'; import { schemaProvider } from './provider'; import type { InferInput, InferOutput, StandardSchemaV1 } from './standard'; -import type { - MoneyValue, - NumberSchema, - ObjectSchema, - StringSchema, - TNamespace, -} from './validators'; +import type { NumberSchema, ObjectSchema, StringSchema, TNamespace } from './validators'; function provider(): TNamespace { return schemaProvider().t; diff --git a/packages/schema/src/validators.test.ts b/packages/schema/src/validators.test.ts index 49712491..7dca490d 100644 --- a/packages/schema/src/validators.test.ts +++ b/packages/schema/src/validators.test.ts @@ -396,51 +396,6 @@ describe('builtinT.date', () => { }); }); -describe('builtinT.money', () => { - test('accepts a valid Money value', () => { - const result = validate(builtinT.money, { minor: 1999, currency: 'EUR' }); - expect(result.issues).toBeUndefined(); - if (result.issues === undefined) expect(result.value).toEqual({ minor: 1999, currency: 'EUR' }); - }); - - test('rejects a non-integer minor amount', () => { - const result = validate(builtinT.money, { minor: 19.99, currency: 'EUR' }); - expect(result.issues?.length).toBe(1); - expect(result.issues?.[0]?.path).toEqual(['minor']); - }); - - test('rejects a malformed currency code', () => { - const result = validate(builtinT.money, { minor: 1999, currency: 'eur' }); - expect(result.issues?.length).toBe(1); - expect(result.issues?.[0]?.path).toEqual(['currency']); - }); - - test('reports both issues together when minor and currency are both invalid', () => { - const result = validate(builtinT.money, { minor: 19.99, currency: 'eur' }); - expect(result.issues?.length).toBe(2); - expect(result.issues?.map((issue) => issue.path)).toEqual([['minor'], ['currency']]); - }); - - test('rejects a non-object', () => { - expect(validate(builtinT.money, 'money').issues).toBeDefined(); - }); - - test('rejects a minor amount past the safe-integer range', () => { - // `Number.isInteger(2**53)` is true and `money()`/`parseMinor` both refuse it, so accepting - // it here turned a 422 with a field path into a 500 at the row write. - const result = validate(builtinT.money, { minor: 9_007_199_254_740_992, currency: 'EUR' }); - expect(result.issues?.length).toBe(1); - expect(result.issues?.[0]?.path).toEqual(['minor']); - expect( - validate(builtinT.money, { minor: -9_007_199_254_740_992, currency: 'EUR' }).issues, - ).toBeDefined(); - // The largest amount that IS representable still passes. - expect( - validate(builtinT.money, { minor: Number.MAX_SAFE_INTEGER, currency: 'EUR' }).issues, - ).toBeUndefined(); - }); -}); - describe('builtinT.timezone', () => { test('accepts a valid IANA time zone', () => { expect(validate(builtinT.timezone, 'America/New_York').issues).toBeUndefined(); diff --git a/packages/schema/src/validators.ts b/packages/schema/src/validators.ts index 77f76c2b..ff0d58ac 100644 --- a/packages/schema/src/validators.ts +++ b/packages/schema/src/validators.ts @@ -9,6 +9,7 @@ import { expected, fail, failWith, + isPlainObject, makeSchema, pass, type Schema, @@ -16,33 +17,14 @@ import { type ShapeInput, type ShapeOutput, } from './builder'; +import { type MoneyValue, moneySchema } from './money-value'; import type { SchemaNode } from './node'; import type { InferInput, InferOutput, StandardIssue } from './standard'; -/** - * The framework's ONE declaration of a money value. `@ultimat3/money`'s `Money` and - * `@ultimat3/entity`'s `MoneyValue` are aliases of this type, not copies of its shape — three - * structural restatements are how `minor` became a `number` here and a `bigint` there, which made - * a row the entity layer produced fail `t.money` and throw inside `JSON.stringify`. - * - * It lives at tier 0 because that is the only tier every other package may import, and `number` - * rather than `bigint` because money crosses the wire on every surface this framework projects — - * `JSON.stringify` refuses a bigint, and this node is also the OpenAPI contract. A value past - * `Number.MAX_SAFE_INTEGER` is refused HERE, at the boundary, with the field path — and again - * where it is decoded; it is never widened. - * - * Never a float, and never an amount without its currency. - */ -export interface MoneyValue { - readonly minor: number; - readonly currency: string; -} - const EMAIL_RE = /^[^\s@]+@[^\s@.]+(\.[^\s@.]+)+$/; const SLUG_RE = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; const CURSOR_RE = /^[A-Za-z0-9_-]+$/; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; -const CURRENCY_RE = /^[A-Z]{3}$/; export interface StringSchema extends Schema { min(length: number): StringSchema; @@ -143,10 +125,6 @@ function makeNumberSchema(node: SchemaNode): NumberSchema { }; } -function isPlainObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value); -} - export function objectSchema(shape: S): ObjectSchema { const properties: Record = {}; const checks: [string, Check][] = []; @@ -333,45 +311,6 @@ const dateSchema: Schema = makeSchema = makeSchema( - { - kind: 'money', - description: 'integer minor units plus an ISO 4217 currency code', - properties: { - minor: { - kind: 'number', - integer: true, - minimum: -Number.MAX_SAFE_INTEGER, - maximum: Number.MAX_SAFE_INTEGER, - }, - currency: { kind: 'string', pattern: CURRENCY_RE.source }, - }, - }, - (value, path) => { - if (!isPlainObject(value)) return fail(path, expected('a Money object', value)); - const minor = value['minor']; - const currency = value['currency']; - const issues: StandardIssue[] = []; - // Safe, not merely whole: `money()` and `entity`'s `parseMinor` both demand a safe integer, so - // `Number.isInteger` here let 2^53 through the boundary as a 200 and failed at the row write - // as a 500 — the same value refused twice, once with a field path and once without. - if (typeof minor !== 'number' || !Number.isSafeInteger(minor)) { - issues.push({ - message: expected('a safe integer number of minor units', minor), - path: [...path, 'minor'], - }); - } - if (typeof currency !== 'string' || !CURRENCY_RE.test(currency)) { - issues.push({ - message: expected('a 3-letter ISO 4217 code', currency), - path: [...path, 'currency'], - }); - } - if (issues.length > 0) return failWith(issues); - return pass({ minor: minor as number, currency: currency as string }); - }, -); - /** The shape a schema provider must implement to back `t`. */ export interface TNamespace { readonly string: StringSchema; diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index ced482a1..b350e092 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -275,6 +275,7 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | `X_FLAG_DUPLICATE` | two flags were declared with the same key | a copy-pasted `defineFlag()`, so one of the two declarations decides nothing | rename one of the two `defineFlag({ key: '' })` declarations | | `X_FLAG_EXPIRED` | a temporary flag is past its expiry and is still being evaluated | scaffolding that outlived the change it was wrapping; `cause` names the owner and how overdue it is | delete the branch and its `defineFlag()` declaration, or move it to `kind: 'permanent'` if it is a real product switch | | `X_FLAG_EXPIRY_INVALID` | a temporary flag has no usable expiry date | `expiresAt` absent or unparseable — reachable from a store snapshot or plain JS; in TypeScript the union already refuses it | set `expiresAt` to an ISO-8601 date such as `'2026-12-01'` in `defineFlag({ key: '' })` | +| `X_FLAG_SUBJECT_REQUIRED` | a flag decides by a subject the evaluation context does not carry | targeting names a subject kind — an org, or an app record such as `bank` — but the actor carries no `orgId`, or the call site passed no record for that kind. Answered as an error rather than a silent default, which would decide the wrong way and never say so | mint the actor with its tenant — `userActor({ id, orgId })` — for `org`; for an app kind, pass the record at the call site: `isEnabled('', actor, { : '' })` | | `X_FLAG_TARGETING_INVALID` | flag targeting is out of range or malformed | `rollout: 0.5` (a fraction read as a percentage means nobody), a rollout outside 0-100, or `default: true` beside a rollout | set `rollout` to a whole percentage 0-100 in `defineFlag({ key: '' })`, and leave `default` false when a rollout decides | | `X_FLAG_UNKNOWN` | no flag is declared under this key | a typo at the `isEnabled()` call site — answered as an error rather than a silent `false`, which would be a branch that never runs and never says so | declare it with `defineFlag({ key: '', … })`, or correct the key at the call site | @@ -338,7 +339,8 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | `X_CATALOG_INVALID` | a catalog entry is malformed | bad interpolation or a non-string value | `x i18n check --json` | | `X_CURRENCY_UNKNOWN` | currency code not in the currency table | a typo, or a currency the table lacks | `x money add-currency --exponent ` | | `X_CURRENCY_MISMATCH` | two `Money` values in different currencies | adding EUR to USD | `convert(value, '', rate)` first | -| `X_MONEY_NOT_INTEGER` | a `Money.minor` that is not an integer | a float leaked in | `fromDecimal('12.99', 'USD')`, or pass an explicit rounding mode | +| `X_MONEY_NOT_INTEGER` | a `Money.minor` that is not an integer | a float leaked in, or a `rescale()` that would drop minor units | `fromDecimal('12.99', 'USD')`, or pass an explicit rounding mode | +| `X_MONEY_SCALE_INVALID` | a `Money.scale` that is not a usable decimal exponent | a fractional, negative, or out-of-range scale — 15 is the last exponent whose power of ten is still a safe integer | use a scale in range — `money(minor, currency, 6)` for micros, or omit it for the currency's own minor unit | | `X_ALLOCATION_INVALID` | split ratios or part count are unusable | zero parts, or all-zero ratios | pass a positive part count, or finite non-negative ratios | | `X_RATE_MISSING` | no FX rate for the pair | no `RateProvider` registered | register one — there is no default, because a wrong rate is worse than a missing one | | `X_TIMEZONE_INVALID` | not an IANA zone | `CET`, `+02:00`, or a typo | use `Europe/Berlin`, `America/New_York`, `UTC` | From 4509e4d54b176101809f056412ddc5b335d7fc13 Mon Sep 17 00:00:00 2001 From: sebi Date: Sat, 15 Aug 2026 06:44:55 -0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(tier0-1):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20formatter=20cache,=20unrunnable=20fixes,=20early-re?= =?UTF-8?q?turn=20targeting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen CodeRabbit findings; all accepted after verifying each against the code, one with a different remedy. flags ----- `evaluateTargeting` returned true from the `actors` and `roles` allow lists BEFORE `orgs`, `subjects` and `bucketBy` were resolved. The result was not order-dependence but caller-dependence: given `{ actors: ['user-1'], subjects: { bank: [...] } }`, a call site that forgot to pass the bank answered true for user-1 and threw for everyone else — so a missing record ships green through whoever is on the allow list and surfaces in production only for users who are not. Every declared kind now resolves before anything answers. `subjects?.[kind]` walked the prototype chain: a kind named `toString` resolved to `Object.prototype.toString`, and `bucketOf` would have hashed a function source string instead of raising. Now `Object.hasOwn` plus a `typeof` re-check, so a non-string own value is absent rather than hashed. Fix strings were not parseable for a hyphenated kind — `{ bank-integration: '' }` is not JS, and treasury's real flipper_ids (`bank_integration:`, `bank_connection:`) make that the realistic shape. App-supplied strings now go through JSON.stringify with a computed key; the org branch had the same defect for an actor id containing a quote. Note error-contract.ts does NOT catch this class: staticFix() blanks every `${…}` before its rule runs, so it checks actionability, never parseability. The allocation-free claim was already false before this PR — `roles?.some(...)` allocates a closure per call. Removed the `Object.entries` pass (`for…in` + `Object.hasOwn` allocates no pair arrays and doubles as the prototype guard) and narrowed the invariant to what actually holds. money / schema -------------- The formatter cache key lost `exponent`: on the `trimZeroFraction` path `digits` is undefined, so the key carried 'auto' while the formatter used `maximumFractionDigits: exponent` — one cached formatter for every scale of a currency. Formatting 1299 EUR then 12_990_001 EUR@6 returned "12,99 €" instead of "12,990001 €". The test formats coarse -> fine -> coarse, since the order dependence is the bug. Two fix: lines could not run. `fromDecimal('1.0000000000000000001', 'USD')` suggested `{ scale: 19 }`, past MAX_MONEY_SCALE; past the maximum the offer is now withdrawn rather than clamped, because no scale keeps every digit and suggesting one would be a lie. And widening overflow — `add(MAX_SAFE_INTEGER USD, 1 USD@6)` — suggested a `fromDecimal` that throws the same error again. One shared `toMinor()` in scale.ts is now the single conversion point for `add`, `subtract` and `rescale`, and names the finest scale that fits. No second error code minted: X_MONEY_SCALE_INVALID is unshipped and already means "the scale is not usable", which covers the scale two operands must meet at. `{ minor: '' }` coerced to 0, booking an empty price field as free. Pre-existing, on a line this PR touched; routed through the same numeric() helper scale uses. `equals` restated the scale-normalisation rule instead of calling commonScale. CLAUDE.md and README restated the MoneyValue shape and had already drifted — the README's opening line omitted `scale`. Both now point at money-value.ts. The rescale test claimed a per-token calculation it never performed. Rewritten to multiply a per-token rate by 200 tokens and assert both the exact $0.00016 and the 1c that 'up' produces; proved it catches a regression by temporarily dropping scale preservation from multiply — the old test passed, the new one fails. Corrected a figure this PR asserted three times: the AI cost overstatement is 62x, not ~50x. 200 tokens at $0.80/Mtok is $0.00016, billed as 1c. The claim now matches the test that proves it. Gate: bun run verify — 14 of 17 passed, 3 skipped. 233 money+schema tests, 97 flags tests, 0 failures. Every new test run red first. Co-Authored-By: Claude Opus 5 (1M context) --- packages/entity/src/type-pins.ts | 2 +- packages/flags/CLAUDE.md | 30 +++++++++----- packages/flags/src/errors.test.ts | 52 ++++++++++++++++++++++++ packages/flags/src/errors.ts | 8 +++- packages/flags/src/subject.test.ts | 33 ++++++++++++++- packages/flags/src/subject.ts | 12 +++++- packages/flags/src/targeting.test.ts | 58 +++++++++++++++++++++++++++ packages/flags/src/targeting.ts | 36 ++++++++--------- packages/money/CLAUDE.md | 28 ++++++++----- packages/money/README.md | 22 +++++----- packages/money/src/arithmetic.test.ts | 27 +++++++++++++ packages/money/src/arithmetic.ts | 14 +++++-- packages/money/src/errors.ts | 32 ++++++++++++++- packages/money/src/format.test.ts | 13 ++++++ packages/money/src/format.ts | 6 +++ packages/money/src/money.test.ts | 22 ++++++++++ packages/money/src/money.ts | 4 +- packages/money/src/rescale.test.ts | 25 +++++++++--- packages/money/src/rescale.ts | 6 ++- packages/money/src/scale.test.ts | 5 +++ packages/money/src/scale.ts | 25 +++++++++++- packages/schema/src/coerce.test.ts | 6 +++ packages/schema/src/coerce.ts | 8 +++- packages/schema/src/money-value.ts | 4 +- wiki/Error-Codes.md | 2 +- 25 files changed, 409 insertions(+), 71 deletions(-) diff --git a/packages/entity/src/type-pins.ts b/packages/entity/src/type-pins.ts index 2cca18b2..dff6c0b2 100644 --- a/packages/entity/src/type-pins.ts +++ b/packages/entity/src/type-pins.ts @@ -282,7 +282,7 @@ type _MoneyCurrencyIsAReadonlyString = Assert< /** * `scale` is the decimal exponent `minor` counts in — `{ minor: 2, currency: 'USD', scale: 6 }` is * $0.000002. Optional, and pinned optional, because a cents-only `Money` could not name a - * sub-cent amount at all: the AI cost path rounded a $0.0002 call up to a whole cent, ~50x, and + * sub-cent amount at all: the AI cost path rounded a $0.00016 call up to a whole cent, 62x, and * the alternative to this field was a second money type. */ type _MoneyScaleIsAReadonlyOptionalNumber = Assert< diff --git a/packages/flags/CLAUDE.md b/packages/flags/CLAUDE.md index 4fe8b329..9c919ce1 100644 --- a/packages/flags/CLAUDE.md +++ b/packages/flags/CLAUDE.md @@ -20,9 +20,13 @@ what lets `policy` (tier 2) call it from inside a predicate. schema and no surface of its own, so there is nothing for `primitiveRegistrar` to project. The eight kinds in `PRIMITIVE_KINDS` stay eight. A capability that *does* need a handler arrives as a factory over an existing primitive — `llm()` returns an `action`. -- **Evaluation is synchronous and allocation-free on the hot path.** It runs inside policy - predicates and render passes. No `await`, no I/O, no date parsing (`expiresAtMs` is precomputed), - and the expired-flag error is built lazily so a rate-limited call costs a map lookup. +- **Evaluation is synchronous, and allocates nothing per declared subject kind.** It runs inside + policy predicates and render passes. No `await`, no I/O, no date parsing (`expiresAtMs` is + precomputed), and the expired-flag error is built lazily so a rate-limited call costs a map + lookup. It is **not literally allocation-free** — `roles` allocates a closure for `some()`, and + `subjectIdOf` takes an options object — so do not restate that stronger claim. What is + guaranteed: no `Object.entries`/`Object.keys` pass, no normalisation pass, and nothing at all + allocated for a flag declaring no subject axis. - **A temporary flag without an expiry must not be declarable.** `FlagExpiryIsMandatory` in `flag.ts` is a compile-time assertion: loosen the union and `tsc -b packages/flags` fails on that line. `toFlag()` re-checks at runtime for snapshots and JS callers. Do not replace either with a @@ -62,14 +66,22 @@ what lets `policy` (tier 2) call it from inside a predicate. - **The subject axis throws rather than degrades.** A kind the evaluation context does not carry raises `X_FLAG_SUBJECT_REQUIRED`. Never fall back to the actor axis or to `default`: an answer about a record computed from whoever was calling looks like it worked, which is the whole bug - class. **Every declared kind is resolved before any can answer**, so the raise never depends on - declaration order. A `null` actor is the one exception and still gets `default` — no evaluation - context at all, every such call answers alike, so no single subject is split. + class. **Every declared kind is resolved before any branch answers** — allow lists included — so + the raise depends only on the flag and the context, never on declaration order and never on which + list happened to match. An early `return true` on an allow-list hit is the regression to watch + for: it hides a missing record from exactly the callers who are on the list. A `null` actor is + the one exception and still gets `default` — no evaluation context at all, every such call + answers alike, so no single subject is split. - **`bucketBy` defaults to `actor`.** The subject axes are opt-in; a flag declared before they existed must answer identically, which is why the default is not `org`. -- **`subjectIdOf` is called only on branches that need a subject**, so a plain - `{ default, rollout }` flag still allocates nothing. Keep it that way — no closures, no - normalisation pass, no `Object.entries` on the common path. +- **Subject lookups are own-property only.** `subjects[kind]` goes through `Object.hasOwn` and a + `typeof` re-check, so a kind named `toString` or `constructor` is absent rather than resolving to + an inherited function, and a non-string id never reaches `bucketOf`. Same rule for the targeting + map, which is why the loop is `for…in` + `Object.hasOwn` and not `Object.entries`. +- **Every app-supplied string in a `fix:` goes through `JSON.stringify`, and a subject kind becomes + a computed key.** A `bank-integration` kind is not a valid identifier, so `{ bank-integration: … }` + would be a fix that does not parse — axiom 4 wants an instruction that runs. Pinned by + `errors.test.ts` running the generated snippet through `new Function`. - **An unknown key throws.** Answering `false` is a branch that never runs and never says so. - `default: true` beside a `rollout` is refused: the two answer the same actors and disagree. diff --git a/packages/flags/src/errors.test.ts b/packages/flags/src/errors.test.ts index 6816fcef..daa86d89 100644 --- a/packages/flags/src/errors.test.ts +++ b/packages/flags/src/errors.test.ts @@ -61,3 +61,55 @@ describe('unit · @ultimat3/flags errors', () => { expect(error.fix).toContain("kind: 'permanent'"); }); }); + +/** + * Axiom 4: a `fix:` is an instruction, and an instruction that does not parse is not one. The + * subject kind and the actor id are app-supplied, so neither can be pasted into a JS literal + * unquoted — treasury's own ids are `bank_integration:`, and a `bank-integration` kind is + * one hyphen away from an invalid object key. + */ +describe('unit · the subject fix is executable JavaScript', () => { + /** `new Function` parses without running, which is exactly the question being asked. */ + const parses = (snippet: string): boolean => { + try { + new Function('actor', 'isEnabled', 'userActor', snippet); + return true; + } catch { + return false; + } + }; + + const snippetOf = (fix: string, call: string): string => { + const found = fix.match(new RegExp(`${call}\\([^—]*\\}\\)`))?.[0]; + expect(found).toBeDefined(); + return found ?? ''; + }; + + test('a kind that is not a valid identifier still yields a parseable call', () => { + const error = flagSubjectRequired({ + key: 'scraper.persist-profile', + kind: 'bank-integration', + actorId: 'user-7', + via: 'subjects', + }); + expect(parses(snippetOf(error.fix, 'isEnabled'))).toBe(true); + }); + + test('a key or actor id carrying a quote does not break the fix', () => { + const record = flagSubjectRequired({ + key: 'flag\'with"quotes', + kind: 'bank-integration', + actorId: 'actor\'with"quotes', + via: 'subjects', + }); + expect(parses(snippetOf(record.fix, 'isEnabled'))).toBe(true); + + const org = flagSubjectRequired({ + key: 'flag\'with"quotes', + kind: 'org', + actorId: 'actor\'with"quotes', + via: 'orgs', + }); + expect(parses(snippetOf(org.fix, 'userActor'))).toBe(true); + }); +}); diff --git a/packages/flags/src/errors.ts b/packages/flags/src/errors.ts index 49c90f84..fa04bbcd 100644 --- a/packages/flags/src/errors.ts +++ b/packages/flags/src/errors.ts @@ -94,10 +94,14 @@ export const flagSubjectRequired = (init: { new FlagsError({ code: 'X_FLAG_SUBJECT_REQUIRED', cause: `${init.key} decides by the "${init.kind}" subject (targeting.${init.via}) but the evaluation context carries no ${init.kind} id for actor "${init.actorId}", so there is nothing to decide about`, + // Every app-supplied string goes through JSON.stringify, and the kind becomes a COMPUTED key: + // a `bank-integration` kind — the realistic shape, next to treasury's `bank_integration:` ids + // — is not a valid identifier, so `{ bank-integration: … }` would hand the reader a fix that + // does not parse. Axiom 4: an instruction that cannot be run is not one. fix: init.kind === 'org' - ? `mint the actor with its tenant — userActor({ id: '${init.actorId}', orgId: '' }) — before the isEnabled('${init.key}') call, or drop ${init.via} from defineFlag({ key: '${init.key}' })` - : `pass the record at the call site — isEnabled('${init.key}', actor, { ${init.kind}: '' }) — or drop the "${init.kind}" ${init.via} entry from defineFlag({ key: '${init.key}' })`, + ? `mint the actor with its tenant — userActor({ id: ${JSON.stringify(init.actorId)}, orgId: '' }) — before the isEnabled(${JSON.stringify(init.key)}) call, or drop ${init.via} from defineFlag({ key: ${JSON.stringify(init.key)} })` + : `pass the record at the call site — isEnabled(${JSON.stringify(init.key)}, actor, { [${JSON.stringify(init.kind)}]: '' }) — or drop the ${JSON.stringify(init.kind)} ${init.via} entry from defineFlag({ key: ${JSON.stringify(init.key)} })`, meta: { key: init.key, kind: init.kind, actorId: init.actorId, via: init.via }, }); diff --git a/packages/flags/src/subject.test.ts b/packages/flags/src/subject.test.ts index bee5c16f..a149fa65 100644 --- a/packages/flags/src/subject.test.ts +++ b/packages/flags/src/subject.test.ts @@ -4,6 +4,7 @@ import { describe, expect, test } from 'bun:test'; import { userActor } from '@ultimat3/core'; +import type { FlagSubjects } from './subject'; import { BUILT_IN_SUBJECT_KINDS, subjectIdOf } from './subject'; const caught = (run: () => unknown): unknown => { @@ -95,10 +96,40 @@ describe('unit · subjectIdOf', () => { const bankFix = caught(() => subjectIdOf({ key: 'a.flag', kind: 'bank', actor, subjects: {}, via: 'subjects' }), ) as { fix: string }; - expect(bankFix.fix).toContain("isEnabled('a.flag'"); + expect(bankFix.fix).toContain('isEnabled("a.flag"'); + // A computed key, so a kind that is not an identifier still parses — see errors.test.ts. + expect(bankFix.fix).toContain('{ ["bank"]:'); expect(bankFix.fix).toContain('bank'); }); + test('a kind named after an Object.prototype member is absent, not inherited', () => { + // `subjects.toString` walks the prototype chain and finds a function. Without an own-property + // check the resolver returns it instead of raising, and the failure downstream is a weird one + // — a function where an id belongs — rather than the clean error this package designed. + for (const inherited of ['toString', 'constructor', 'valueOf', 'hasOwnProperty']) { + expect( + caught(() => + subjectIdOf({ key: 'a.flag', kind: inherited, actor, subjects: {}, via: 'subjects' }), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + } + }); + + test('an own value that is not a string is absent — an id is a string or it is nothing', () => { + const notAString: unknown = { bank: 42 }; + expect( + caught(() => + subjectIdOf({ + key: 'a.flag', + kind: 'bank', + actor, + subjects: notAString as FlagSubjects, + via: 'subjects', + }), + ), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + test('actor and org are the built-ins, and nothing else is', () => { expect([...BUILT_IN_SUBJECT_KINDS]).toEqual(['actor', 'org']); }); diff --git a/packages/flags/src/subject.ts b/packages/flags/src/subject.ts index b3c6fe07..a88a26b6 100644 --- a/packages/flags/src/subject.ts +++ b/packages/flags/src/subject.ts @@ -57,6 +57,14 @@ export function subjectIdOf(init: { return id; } +/** + * Own properties only. `subjects['toString']` would otherwise walk the prototype chain and hand + * back a function where an id belongs — a weird downstream failure instead of the clean + * `X_FLAG_SUBJECT_REQUIRED` this package designed for exactly that case. + * + * The `typeof` re-check is for JS callers and store-shaped data: an id is a string or it is + * nothing, and a number reaching `bucketOf` would hash to a real bucket rather than raise. + */ function resolve( kind: string, actor: Actor, @@ -64,5 +72,7 @@ function resolve( ): string | undefined { if (kind === 'actor') return actor.id; if (kind === 'org') return actor.orgId; - return subjects?.[kind]; + if (subjects === undefined || !Object.hasOwn(subjects, kind)) return undefined; + const id = subjects[kind]; + return typeof id === 'string' ? id : undefined; } diff --git a/packages/flags/src/targeting.test.ts b/packages/flags/src/targeting.test.ts index 408a45e8..e89923ca 100644 --- a/packages/flags/src/targeting.test.ts +++ b/packages/flags/src/targeting.test.ts @@ -216,6 +216,64 @@ describe('unit · arbitrary record subjects', () => { ); }); + /** + * The raise must not depend on WHO is calling either. If an allow-list hit answers before the + * declared record is resolved, a call site that forgot to pass the record ships green — it only + * blows up for the users who are not allow-listed, which is the delayed version of the silent + * wrong answer this axis exists to remove. + */ + test('an allow-listed actor does not skip resolving a declared record kind', () => { + const targeting: FlagTargeting = { + default: false, + actors: ['user-1'], + subjects: { bank: ['bank_integration:bbva'] }, + }; + expect( + caught(() => evaluateTargeting('a.flag', targeting, actor, undefined)), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an allow-listed role does not skip resolving a declared record kind', () => { + const staff = userActor({ id: 'user-9', orgId: 'org-a', roles: ['staff'] }); + const targeting: FlagTargeting = { + default: false, + roles: ['staff'], + subjects: { bank: ['bank_integration:bbva'] }, + }; + expect( + caught(() => evaluateTargeting('a.flag', targeting, staff, undefined)), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an allow-listed actor does not skip resolving the bucketBy kind', () => { + const targeting: FlagTargeting = { + default: false, + actors: ['user-1'], + rollout: 10, + bucketBy: 'bank', + }; + expect( + caught(() => evaluateTargeting('a.flag', targeting, actor, undefined)), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an allow-listed actor does not skip resolving the org axis', () => { + const orgless = userActor({ id: 'user-1' }); + const targeting: FlagTargeting = { default: false, actors: ['user-1'], orgs: ['org-a'] }; + expect( + caught(() => evaluateTargeting('a.flag', targeting, orgless, undefined)), + ).toBeUltimateError('X_FLAG_SUBJECT_REQUIRED'); + }); + + test('an allow-list hit still answers true once every declared kind resolved', () => { + const targeting: FlagTargeting = { + default: false, + actors: ['user-1'], + subjects: { bank: ['bank_integration:zzz'] }, + }; + expect(evaluateTargeting('a.flag', targeting, actor, santander)).toBe(true); + }); + test('bucketBy a record kind puts a whole record on one side of a rollout', () => { // `bank_integration:bbva` buckets at 39 and `:santander` at 86 for this key — pinned below. const targeting: FlagTargeting = { default: false, rollout: 50, bucketBy: 'bank' }; diff --git a/packages/flags/src/targeting.ts b/packages/flags/src/targeting.ts index a3e0e685..08327d0d 100644 --- a/packages/flags/src/targeting.ts +++ b/packages/flags/src/targeting.ts @@ -125,8 +125,9 @@ function assertSubjects(key: string, subjects: Readonly hasRole(actor, role)) === true) return true; - if ( - targeting.orgs?.includes(subjectIdOf({ key, kind: 'org', actor, subjects, via: 'orgs' })) === - true - ) { - return true; + // Nothing answers until every declared kind has resolved. Returning early on an allow-list hit + // would hide a missing record from exactly the callers who are on the list: the call site ships + // green, and raises later only for everybody else. `allowed` accumulates instead of returning. + let allowed = targeting.actors?.includes(actor.id) === true; + if (targeting.roles?.some((role) => hasRole(actor, role)) === true) allowed = true; + if (targeting.orgs !== undefined) { + const orgId = subjectIdOf({ key, kind: 'org', actor, subjects, via: 'orgs' }); + if (targeting.orgs.includes(orgId)) allowed = true; } if (targeting.subjects !== undefined) { - // Every declared kind is resolved before any of them can answer, so a call site missing one - // raises whatever order the keys sit in. Short-circuiting on the first match would make the - // same inputs sometimes answer and sometimes throw, decided by declaration order. - let matched = false; - for (const [kind, ids] of Object.entries(targeting.subjects)) { + // `for…in` + `Object.hasOwn` rather than `Object.entries`: own keys only, and no array pair + // allocated per declared kind on a path that runs inside policy predicates. + for (const kind in targeting.subjects) { + if (!Object.hasOwn(targeting.subjects, kind)) continue; const id = subjectIdOf({ key, kind, actor, subjects, via: 'subjects' }); - if (ids.includes(id)) matched = true; + if (targeting.subjects[kind]?.includes(id) === true) allowed = true; } - if (matched) return true; } - if (targeting.rollout === undefined) return targeting.default; + if (targeting.rollout === undefined) return allowed || targeting.default; const subjectId = targeting.bucketBy === undefined ? actor.id : subjectIdOf({ key, kind: targeting.bucketBy, actor, subjects, via: 'bucketBy' }); - return bucketOf(key, subjectId) < targeting.rollout; + return allowed || bucketOf(key, subjectId) < targeting.rollout; } diff --git a/packages/money/CLAUDE.md b/packages/money/CLAUDE.md index b3ac798f..45816f80 100644 --- a/packages/money/CLAUDE.md +++ b/packages/money/CLAUDE.md @@ -1,11 +1,13 @@ # @ultimat3/money — agent notes **Tier 1.** May import `@ultimat3/core`, `@ultimat3/schema`. No external deps, ever. -`Money = { readonly minor: number; readonly currency: string; readonly scale?: number }` is the -shape the whole framework passes around. `scale` is the decimal exponent `minor` counts in when it -is not the currency's own — absent on every value that predates it, and absent again whenever it -would only restate the currency, so there is exactly one encoding of an amount at the natural -scale and existing JSON is untouched. +`Money` is the shape the whole framework passes around, and the shape itself is declared once, in +`packages/schema/src/money-value.ts` — read it there rather than trusting a copy here, which is +the same reason `type-pins.ts` pins invariants instead of a snapshot. What this package adds is +the *meaning* of the optional `scale`: the decimal exponent `minor` counts in when it is not the +currency's own. Absent on every value that predates it, and absent again whenever it would only +restate the currency, so an amount at the natural scale has exactly one encoding and existing +JSON is untouched. **`Money` is an alias, not a declaration.** It is `@ultimat3/schema`'s `MoneyValue` — tier 0, the only tier every package may import — and `@ultimat3/entity`'s `MoneyValue` is the same alias. Never @@ -24,7 +26,7 @@ shape is still additive and this is still a minor version. | `money.ts` | the value type + constructors (`money`, `fromDecimal`, `toDecimalString`) | | `currency.ts` | ISO-4217 table + minor-unit exponent. Every natural scale derives from here. | | `scale.ts` | what decimal place a value's `minor` counts (`moneyScale`), which scales are legal (`assertScale`), and the exact bigint widening every comparison starts with (`minorAt`) | -| `rescale.ts` | moving between scales: widening exact, narrowing only with a named mode | +| `rescale.ts` | moving between scales: widening exact, lossy narrowing only with a named mode | | `arithmetic.ts` | add/subtract/multiply/compare, refuses mixed currencies | | `allocate.ts` | largest-remainder splits that preserve the total | | `factor.ts` | the exact fraction a scaling factor's decimal spelling names. `factorFraction` is internal — never exported; the `Fraction` **type** is public, because `ExchangeRate.ratio` is one | @@ -42,11 +44,17 @@ shape is still additive and this is still a minor version. widen through `minorAt` (bigint, exact) before they do anything else, so a sub-cent fee added to a cent survives and a comparison answers where storing the widened value would rightly be refused. Rounding down to the coarser scale would silently delete the smaller operand. -- **Narrowing a scale names its mode at the call.** `rescale(m, 2)` throws rather than drop a - digit; `rescale(m, 2, 'half-up')` is the same rule `fromDecimal` applies to excess precision. +- **Lossy narrowing names its mode at the call.** `rescale(m, 2)` throws rather than drop a + non-zero digit; `rescale(m, 2, 'half-up')` is the same rule `fromDecimal` applies to excess + precision. A narrowing that loses nothing needs no mode — nothing is being decided. - **`money()` is the only place the canonical form is decided.** It drops a `scale` equal to the - currency's exponent, so every constructor, every arithmetic result and every allocation part - agree on one encoding without any of them repeating the rule. + currency's exponent — only equal, so a deliberately *coarser* scale (`money(5, 'USD', 0)`, whole + dollars) is kept exactly as a finer one is. Every constructor, every arithmetic result and every + allocation part therefore agree on one encoding without any of them repeating the rule. +- **A widened value that will not fit is a scale error, not a fractional-minor one.** `add`, + `subtract` and `rescale` convert through `toMinor`, which throws `X_MONEY_SCALE_INVALID` naming + the finest scale that fits. Letting `money()` refuse the raw number reported a fractional minor + nobody wrote, with a `fromDecimal` fix line that threw the same error again. - Never combine currencies without `convert()` first. - Never round without naming a `RoundingMode` in the call or accepting the stated default. - **Never scale in floats and round after.** `multiply`, `divide` and `convert` take the factor's diff --git a/packages/money/README.md b/packages/money/README.md index 23f3221d..17318fdd 100644 --- a/packages/money/README.md +++ b/packages/money/README.md @@ -1,8 +1,8 @@ # 💶 @ultimat3/money **Golden rule: integer minor units, currency always attached, `Intl` at the edge.** -`0.1 + 0.2 !== 0.3`, so no amount is ever a float. `Money` is `{ readonly minor, readonly currency }` -— the two travel together, and arithmetic across two currencies throws instead of guessing. +`0.1 + 0.2 !== 0.3`, so no amount is ever a float. `Money` carries `minor` and `currency` together +— plus an optional `scale` — and arithmetic across two currencies throws instead of guessing. `Money` **is** `@ultimat3/schema`'s `MoneyValue`, and so is `@ultimat3/entity`'s: one declaration at tier 0, aliased twice, never restated. A row a `money()` column decodes is therefore a `Money` @@ -15,7 +15,7 @@ read rather than rounding it. → [Money](https://github.com/developerz-ai/ultim |---|---|---| | Amount | integer minor units (`1299`) | `Intl.NumberFormat`, `style: 'currency'` | | Currency | ISO-4217 code (`'EUR'`) | fraction digits derived from its exponent | -| Scale | only when finer than the currency's (`scale: 6`) | `10 ** moneyScale(amount)` — never a literal `/ 100` | +| Scale | whenever it differs from the currency's, finer or coarser (`scale: 6`) | `10 ** moneyScale(amount)` — never a literal `/ 100` | | FX rate | explicit argument + timestamp | recorded on the converted value | ## Use @@ -41,7 +41,9 @@ add(price, money(500, 'USD')); // throws X_CURRENCY_MISMATCH `money(2, 'USD', 6)` is $0.000002 — `minor` counting 10⁻⁶ instead of the currency's own 10⁻². A value that names no scale means the currency's, which is every amount that already exists, so -nothing about `{ minor, currency }` changes: same shape, same JSON, same columns. +nothing about `{ minor, currency }` changes: same shape, same JSON, same columns. Only a scale +*equal* to the currency's is dropped, so a deliberately coarser one is kept too: `money(5, 'USD', 0)` +is $5 counted in whole dollars, and `rescale()` produces such values legitimately. ```ts moneyScale(money(1299, 'EUR')); // 2 — the currency's own @@ -56,11 +58,13 @@ add(money(1, 'USD'), money(2, 'USD', 6)); // meets at scale 6: 10002, nothing Arithmetic normalises to the *finer* of two scales, never the coarser — adding a sub-cent fee to a cent cannot round the fee away. `compare` and `equals` read the value rather than the encoding, so 1299 EUR and 12,990,000 EUR at scale 6 are one amount. `multiply`, `divide`, `negate` and -`allocate` keep the scale they were handed. Widening is exact and free; narrowing needs a -`RoundingMode` at the call site, exactly as excess precision does in `fromDecimal`. +`allocate` keep the scale they were handed. Widening is exact and free; a *lossy* narrowing needs +a `RoundingMode` at the call site, exactly as excess precision does in `fromDecimal` — a narrowing +that drops only zeros is exact and needs no mode. -It exists because whole cents could not name the cost of a model call: $0.0002 rounded up to 1¢ -is ~50x, and a budget built on that number is fiction. The alternative was a second money type. +It exists because whole cents could not name the cost of a model call: 200 tokens at $0.80 per +million is $0.00016, and rounding that up to 1¢ bills 62x — a budget built on that number is +fiction. The alternative was a second money type. ## Allocation @@ -93,7 +97,7 @@ stays the readable number the audit trail records. | Code | When | |---|---| | `X_MONEY_NOT_INTEGER` | fractional minor units, a decimal string more precise than the scale, or a `rescale` that would drop a digit with no mode named | -| `X_MONEY_SCALE_INVALID` | a scale that is not a whole number of decimal places in 0…15 | +| `X_MONEY_SCALE_INVALID` | a scale that is not a whole number of decimal places in 0…15, or a widening whose result no longer fits a safe integer | | `X_CURRENCY_UNKNOWN` | code not in the ISO-4217 table | | `X_CURRENCY_MISMATCH` | arithmetic across two currencies | | `X_ALLOCATION_INVALID` | bad part count, empty/negative/all-zero ratios, percentages ≠ 100 | diff --git a/packages/money/src/arithmetic.test.ts b/packages/money/src/arithmetic.test.ts index 3daeff6d..f47c38b5 100644 --- a/packages/money/src/arithmetic.test.ts +++ b/packages/money/src/arithmetic.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test'; import { add, compare, divide, isZero, max, multiply, negate, subtract, sum } from './arithmetic'; import { money } from './money'; +import { rescale } from './rescale'; describe('cross-currency safety', () => { test('add refuses two currencies with X_CURRENCY_MISMATCH', () => { @@ -121,6 +122,23 @@ describe('mixed scales', () => { }); }); + test('a common scale that will not fit reports a scale error, with a fix that runs', () => { + const huge = money(Number.MAX_SAFE_INTEGER, 'USD'); + const fine = money(1, 'USD', 6); + // Not X_MONEY_NOT_INTEGER: nobody wrote a fractional minor. The widening is what does not + // fit, and `fromDecimal('90071992547409900000', 'USD')` — the old fix — throws again. + expect(codeOf(() => add(huge, fine))).toBe('X_MONEY_SCALE_INVALID'); + expect(causeOf(() => add(huge, fine))).toContain('scale 6'); + const fix = fixOf(() => add(huge, fine)); + expect(fix).toContain('rescale'); + expect(fix).toContain('2'); + // Following it works, which is the whole point of an executable fix line. + expect(add(huge, rescale(fine, 2, 'half-up'))).toEqual({ + minor: Number.MAX_SAFE_INTEGER, + currency: 'USD', + }); + }); + test('two currencies still refuse each other, whatever their scales', () => { expect(codeOf(() => add(money(2, 'USD', 6), money(1, 'EUR')))).toBe('X_CURRENCY_MISMATCH'); }); @@ -145,3 +163,12 @@ describe('mixed scales', () => { expect(isZero(money(0, 'USD', 6))).toBe(true); }); }); + +function fixOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return String((error as { fix?: unknown }).fix); + } + return 'no-throw'; +} diff --git a/packages/money/src/arithmetic.ts b/packages/money/src/arithmetic.ts index b05422fb..491d374e 100644 --- a/packages/money/src/arithmetic.ts +++ b/packages/money/src/arithmetic.ts @@ -7,7 +7,7 @@ import { allocationInvalid, currencyMismatch, currencyRequired } from './errors' import { factorFraction } from './factor'; import { type Money, money } from './money'; import { DEFAULT_ROUNDING, type RoundingMode, roundRatio } from './rounding'; -import { commonScale, minorAt } from './scale'; +import { commonScale, minorAt, toMinor } from './scale'; /** Throws `X_CURRENCY_MISMATCH` unless both operands carry the same currency. */ export function assertSameCurrency(left: Money, right: Money): string { @@ -22,13 +22,21 @@ export function assertSameCurrency(left: Money, right: Money): string { export function add(left: Money, right: Money): Money { const currency = assertSameCurrency(left, right); const scale = commonScale(left, right); - return money(Number(minorAt(left, scale) + minorAt(right, scale)), currency, scale); + return money( + toMinor(minorAt(left, scale) + minorAt(right, scale), scale, currency), + currency, + scale, + ); } export function subtract(left: Money, right: Money): Money { const currency = assertSameCurrency(left, right); const scale = commonScale(left, right); - return money(Number(minorAt(left, scale) - minorAt(right, scale)), currency, scale); + return money( + toMinor(minorAt(left, scale) - minorAt(right, scale), scale, currency), + currency, + scale, + ); } /** Every addend must share one currency; an empty list needs an explicit currency. */ diff --git a/packages/money/src/errors.ts b/packages/money/src/errors.ts index bbacf27a..a4722324 100644 --- a/packages/money/src/errors.ts +++ b/packages/money/src/errors.ts @@ -65,10 +65,18 @@ export function notRoundable(value: number): MoneyError { } export function decimalTooPrecise(value: string, currency: string, exponent: number): MoneyError { + const digits = countFractionDigits(value); + // Past MAX_MONEY_SCALE no scale keeps every digit, so the offer is withdrawn rather than + // clamped: `{ scale: 19 }` was a fix line that answered X_MONEY_SCALE_INVALID, and an + // instruction that throws is not one. + const keepThemAll = + digits <= MAX_MONEY_SCALE + ? `fromDecimal('${value}', '${currency}', { scale: ${digits} }) to keep every digit, or ` + : ''; return new MoneyError({ code: 'X_MONEY_NOT_INTEGER', cause: `"${value}" has more than ${exponent} fraction digit(s), which is all ${currency} is being counted in`, - fix: `fromDecimal('${value}', '${currency}', { scale: ${countFractionDigits(value)} }) to keep every digit, or { rounding: 'half-up' } to lose them on purpose`, + fix: `${keepThemAll}pass { rounding: 'half-up' } to fromDecimal to lose the extra digits on purpose`, }); } @@ -85,6 +93,28 @@ export function scaleInvalid(scale: number): MoneyError { }); } +/** + * A widened value that no longer fits a safe integer. Reported under `X_MONEY_SCALE_INVALID` + * rather than `X_MONEY_NOT_INTEGER` because the caller never wrote a fractional minor — the scale + * the operation had to meet at is what does not fit, and that code's fix line + * (`fromDecimal('90071992547409900000', …)`) throws again. Same code as the other scale faults, so + * the reader lands on the page about scales, which is where the answer is. + */ +export function scaleOverflow( + scale: number, + currency: string, + fits: number | undefined, +): MoneyError { + return new MoneyError({ + code: 'X_MONEY_SCALE_INVALID', + cause: `this ${currency} amount needs more digits at scale ${scale} than a safe integer holds`, + fix: + fits === undefined + ? `the amount is too large for any scale — split it, or carry it as two ${currency} values` + : `rescale(theFinerOperand, ${fits}, 'half-up') before combining — scale ${fits} is the finest that fits`, + }); +} + /** Widening is exact; narrowing is a rounding decision, and `minorAt` does not make those. */ export function scaleNotWidening(from: number, to: number): MoneyError { return new MoneyError({ diff --git a/packages/money/src/format.test.ts b/packages/money/src/format.test.ts index 0d017836..3b5eb423 100644 --- a/packages/money/src/format.test.ts +++ b/packages/money/src/format.test.ts @@ -33,6 +33,19 @@ describe('formatMoney', () => { ); }); + test('a finer scale is never served the coarser scale’s cached formatter', () => { + // Order-dependent by construction: the formatter is memoised per currency, so the coarse + // value has to be formatted FIRST for the collision to exist at all. A single-value + // assertion passes against the broken cache and proves nothing. + const trimmed = { trimZeroFraction: true } as const; + expect(normalize(formatMoney(money(1299, 'EUR'), 'de-DE', trimmed))).toBe('12,99 €'); + expect(normalize(formatMoney(money(12_990_001, 'EUR', 6), 'de-DE', trimmed))).toBe( + '12,990001 €', + ); + // …and back again, so the finer entry cannot capture the coarser one either. + expect(normalize(formatMoney(money(1299, 'EUR'), 'de-DE', trimmed))).toBe('12,99 €'); + }); + test('display modes and digit-only output', () => { expect(normalize(formatMoney(money(1299, 'USD'), 'en-US', { display: 'code' }))).toBe( 'USD 12.99', diff --git a/packages/money/src/format.ts b/packages/money/src/format.ts index 92773647..d270c272 100644 --- a/packages/money/src/format.ts +++ b/packages/money/src/format.ts @@ -96,11 +96,17 @@ function formatterFor( const digits = options.fractionDigits ?? (options.trimZeroFraction === true ? undefined : exponent); const sign = options.accounting === true ? 'accounting' : 'standard'; + // `exponent` is in the key because it stopped being derivable from `currency` the moment it + // started coming from the amount's own scale. On the `trimZeroFraction` path `digits` is + // `undefined`, so without it every scale of one currency shared a formatter: format 12.99 EUR + // first and 12.990001 EUR then rendered as `12,99 €` — the sub-cent bug back, silently, in the + // one place a human reads the number. const key = [ locale, currency, options.display ?? 'symbol', digits ?? 'auto', + exponent, options.grouping ?? 'auto', sign, ].join('|'); diff --git a/packages/money/src/money.test.ts b/packages/money/src/money.test.ts index 25d43adf..1b0281c7 100644 --- a/packages/money/src/money.test.ts +++ b/packages/money/src/money.test.ts @@ -46,6 +46,19 @@ describe('fromDecimal', () => { expect(fromDecimal('1200.4', 'JPY', { rounding: 'half-up' }).minor).toBe(1200); }); + test('the excess-precision fix line is a command that runs', () => { + // Axiom 4: an error whose `fix:` throws a second error is not an instruction. + const suggested = /\{ scale: (\d+) \}/.exec(fixOf(() => fromDecimal('12.99999', 'EUR'))); + expect(suggested?.[1]).toBe('5'); + expect(fromDecimal('12.99999', 'EUR', { scale: Number(suggested?.[1]) }).minor).toBe(1_299_999); + + // Past MAX_MONEY_SCALE there is no scale that keeps every digit, so the fix must stop + // offering one — `{ scale: 19 }` answered X_MONEY_SCALE_INVALID. + const tooDeep = fixOf(() => fromDecimal('1.0000000000000000001', 'USD')); + expect(tooDeep).not.toContain('scale:'); + expect(tooDeep).toContain('rounding'); + }); + test('rejects formatted input instead of guessing', () => { expect(codeOf(() => fromDecimal('1,299.00', 'EUR'))).toBe('X_MONEY_NOT_INTEGER'); expect(codeOf(() => fromDecimal('€12.99', 'EUR'))).toBe('X_MONEY_NOT_INTEGER'); @@ -133,3 +146,12 @@ function codeOf(run: () => unknown): string { } return 'no-throw'; } + +function fixOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return String((error as { fix?: unknown }).fix); + } + return 'no-throw'; +} diff --git a/packages/money/src/money.ts b/packages/money/src/money.ts index 7e44cccc..8d1a48ac 100644 --- a/packages/money/src/money.ts +++ b/packages/money/src/money.ts @@ -7,7 +7,7 @@ import { isMoneyScale, type MoneyValue } from '@ultimat3/schema'; import { assertCurrency, type CurrencyCode, exponentOf } from './currency'; import { decimalNotNumeric, decimalTooPrecise, moneyNotInteger } from './errors'; import { type RoundingMode, roundToInteger } from './rounding'; -import { assertScale, minorAt, moneyScale } from './scale'; +import { assertScale, commonScale, minorAt, moneyScale } from './scale'; /** * `{ minor: 129900, currency: 'EUR' }` is €1,299.00. Instances are immutable, and now enforced @@ -122,7 +122,7 @@ export function isMoney(value: unknown): value is Money { */ export function equals(left: Money, right: Money): boolean { if (left.currency !== right.currency) return false; - const scale = Math.max(moneyScale(left), moneyScale(right)); + const scale = commonScale(left, right); return minorAt(left, scale) === minorAt(right, scale); } diff --git a/packages/money/src/rescale.test.ts b/packages/money/src/rescale.test.ts index fa840c77..6d5cbd32 100644 --- a/packages/money/src/rescale.test.ts +++ b/packages/money/src/rescale.test.ts @@ -1,5 +1,10 @@ +// Single responsibility: pins the one asymmetry in rescaling — widening is exact and free, +// narrowing destroys digits and so must name a rounding mode. A silent narrowing is the sub-cent +// bug this whole file exists to make impossible, so the refusal is the contract under test. + import { describe, expect, test } from 'bun:test'; -import { money } from './money'; +import { multiply } from './arithmetic'; +import { money, toDecimalString } from './money'; import { rescale } from './rescale'; describe('rescale', () => { @@ -41,11 +46,19 @@ describe('rescale', () => { }); test('the sub-cent value the AI cost path could not hold: $0.80/Mtok over 200 tokens', () => { - // Truly $0.00016. Whole cents rounded it up to 1¢ — ~50x — and the budget ledger built on - // that number was fiction. - const perMillion = rescale(money(80, 'USD'), 8); - expect(perMillion.minor).toBe(80_000_000); - expect(rescale(perMillion, 2, 'half-up')).toEqual({ minor: 80, currency: 'USD' }); + // $0.80 per million tokens is $0.0000008 per token — 80 units at scale 8, a rate cents + // cannot express at all. + const perToken = money(80, 'USD', 8); + expect(toDecimalString(perToken)).toBe('0.00000080'); + + const cost = multiply(perToken, 200); + expect(cost).toEqual({ minor: 16_000, currency: 'USD', scale: 8 }); + expect(toDecimalString(cost)).toBe('0.00016000'); + + // The bug, reproduced: rounding that up to whole cents bills 1¢ for $0.00016 — 62x. The + // point of the scale is that the exact figure above survives to the ledger instead. + expect(rescale(cost, 2, 'up')).toEqual({ minor: 1, currency: 'USD' }); + expect(rescale(cost, 2, 'half-up')).toEqual({ minor: 0, currency: 'USD' }); }); }); diff --git a/packages/money/src/rescale.ts b/packages/money/src/rescale.ts index ad677220..93f2ece4 100644 --- a/packages/money/src/rescale.ts +++ b/packages/money/src/rescale.ts @@ -7,7 +7,7 @@ import { rescaleNotExact } from './errors'; import { type Money, money } from './money'; import { type RoundingMode, roundRatio } from './rounding'; -import { assertScale, minorAt, moneyScale } from './scale'; +import { assertScale, minorAt, moneyScale, toMinor } from './scale'; /** * `rescale(money(80, 'USD'), 8)` → 80,000,000 hundred-millionths, the granularity a per-token @@ -17,7 +17,9 @@ import { assertScale, minorAt, moneyScale } from './scale'; export function rescale(amount: Money, scale: number, mode?: RoundingMode): Money { assertScale(scale); const from = moneyScale(amount); - if (scale >= from) return money(Number(minorAt(amount, scale)), amount.currency, scale); + if (scale >= from) { + return money(toMinor(minorAt(amount, scale), scale, amount.currency), amount.currency, scale); + } const divisor = 10n ** BigInt(from - scale); const numerator = BigInt(amount.minor); diff --git a/packages/money/src/scale.test.ts b/packages/money/src/scale.test.ts index c03161ec..84812e4f 100644 --- a/packages/money/src/scale.test.ts +++ b/packages/money/src/scale.test.ts @@ -1,3 +1,8 @@ +// Single responsibility: pins what a money value's scale MEANS — that an absent one is the +// currency's own, and that widening to a finer scale is exact. Both are load-bearing: the first +// is why every amount that predates scale still reads correctly, the second is why a comparison +// can answer where storing the widened value would rightly be refused. + import { describe, expect, test } from 'bun:test'; import { money } from './money'; import { assertScale, MAX_MONEY_SCALE, minorAt, moneyScale } from './scale'; diff --git a/packages/money/src/scale.ts b/packages/money/src/scale.ts index 7366b1f5..151768eb 100644 --- a/packages/money/src/scale.ts +++ b/packages/money/src/scale.ts @@ -6,7 +6,7 @@ import { isMoneyScale, MAX_MONEY_SCALE } from '@ultimat3/schema'; import { exponentOf } from './currency'; -import { scaleInvalid, scaleNotWidening } from './errors'; +import { scaleInvalid, scaleNotWidening, scaleOverflow } from './errors'; import type { Money } from './money'; export { MAX_MONEY_SCALE }; @@ -48,3 +48,26 @@ export function minorAt(amount: Money, scale: number): bigint { export function commonScale(left: Money, right: Money): number { return Math.max(moneyScale(left), moneyScale(right)); } + +/** + * A widened bigint back to a storable `minor`, or a scale error naming the finest scale that + * would fit. The one place that conversion happens, because `Number(widened)` alone reached + * `money()` as a plain out-of-range amount — reported as a fractional minor nobody wrote, with a + * `fromDecimal` fix line that throws the same error again. + */ +export function toMinor(widened: bigint, scale: number, currency: string): number { + const minor = Number(widened); + if (Number.isSafeInteger(minor)) return minor; + throw scaleOverflow(scale, currency, finestFitting(widened, scale)); +} + +/** How coarse the scale has to get before the magnitude fits; `undefined` if it never does. */ +function finestFitting(widened: bigint, scale: number): number | undefined { + const limit = BigInt(Number.MAX_SAFE_INTEGER); + let magnitude = widened < 0n ? -widened : widened; + for (let fitted = scale; fitted >= 0; fitted -= 1) { + if (magnitude <= limit) return fitted; + magnitude /= 10n; + } + return undefined; +} diff --git a/packages/schema/src/coerce.test.ts b/packages/schema/src/coerce.test.ts index 79bea61d..43e532da 100644 --- a/packages/schema/src/coerce.test.ts +++ b/packages/schema/src/coerce.test.ts @@ -46,6 +46,12 @@ describe('coerceQuery', () => { minor: 1999, currency: 'EUR', }); + // A blank field is an amount nobody typed. `Number('')` is 0, so converting it would hand + // validation a legitimate-looking zero and book an empty price input as free. + expect(coerceNode(t.money.node, { minor: '', currency: 'USD' })).toEqual({ + minor: '', + currency: 'USD', + }); // A query string carries every field as text, scale included — leaving it a string would // fail validation on a value the same request's `minor` was accepted for. expect(coerceNode(t.money.node, { minor: '2', currency: 'USD', scale: '6' })).toEqual({ diff --git a/packages/schema/src/coerce.ts b/packages/schema/src/coerce.ts index 786d1a2a..b77759ba 100644 --- a/packages/schema/src/coerce.ts +++ b/packages/schema/src/coerce.ts @@ -69,8 +69,12 @@ export function coerceNode(node: SchemaNode, raw: unknown): unknown { case 'money': { if (typeof raw !== 'object') return raw; const source = raw as Record; - const minor = typeof source['minor'] === 'string' ? Number(source['minor']) : source['minor']; - if (!Number.isFinite(minor)) return raw; + // Through `numeric` for the same reason `scale` is: `Number('')` is 0, so a blank amount + // field converted here would reach the validator as a legitimate zero and book an empty + // price input as free. A blank stays a blank and fails validation, which is the real error. + const rawMinor = source['minor']; + const minor = typeof rawMinor === 'string' ? numeric(rawMinor) : rawMinor; + if (typeof minor !== 'number' || !Number.isFinite(minor)) return raw; // `scale` arrives as text from a query string exactly as `minor` does. Left a string it // would fail validation on a value whose `minor` the same request just had converted. const scale = numeric(source['scale']); diff --git a/packages/schema/src/money-value.ts b/packages/schema/src/money-value.ts index a2d5e546..d4bd9694 100644 --- a/packages/schema/src/money-value.ts +++ b/packages/schema/src/money-value.ts @@ -31,8 +31,8 @@ export interface MoneyValue { * for USD, 0 for JPY, 3 for KWD. `{ minor: 2, currency: 'USD', scale: 6 }` is $0.000002. * * It exists because a cents-only value could not name a sub-cent amount at all, so the one - * place that needed one (a model call costing $0.0002) rounded it up to a whole cent and - * reported ~50x the real spend. The alternative was a second money type, which is the axiom-1 + * place that needed one — a model call costing $0.00016 — rounded it up to a whole cent and + * reported 62x the real spend. The alternative was a second money type, which is the axiom-1 * violation this declaration exists to prevent. */ readonly scale?: number; diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index b350e092..f5185721 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -340,7 +340,7 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | `X_CURRENCY_UNKNOWN` | currency code not in the currency table | a typo, or a currency the table lacks | `x money add-currency --exponent ` | | `X_CURRENCY_MISMATCH` | two `Money` values in different currencies | adding EUR to USD | `convert(value, '', rate)` first | | `X_MONEY_NOT_INTEGER` | a `Money.minor` that is not an integer | a float leaked in, or a `rescale()` that would drop minor units | `fromDecimal('12.99', 'USD')`, or pass an explicit rounding mode | -| `X_MONEY_SCALE_INVALID` | a `Money.scale` that is not a usable decimal exponent | a fractional, negative, or out-of-range scale — 15 is the last exponent whose power of ten is still a safe integer | use a scale in range — `money(minor, currency, 6)` for micros, or omit it for the currency's own minor unit | +| `X_MONEY_SCALE_INVALID` | a `Money.scale` that is not usable — either the exponent itself, or the one two operands would have to meet at | a fractional, negative or out-of-range scale (15 is the last exponent whose power of ten is still a safe integer); or adding a coarse value to a fine one, where widening to their common scale overflows a safe integer | use a scale in range — `money(minor, currency, 6)` for micros, or omit it for the currency's own minor unit. On an overflow the `cause` names the finest scale that fits: `rescale(value, , 'half-up')` | | `X_ALLOCATION_INVALID` | split ratios or part count are unusable | zero parts, or all-zero ratios | pass a positive part count, or finite non-negative ratios | | `X_RATE_MISSING` | no FX rate for the pair | no `RateProvider` registered | register one — there is no default, because a wrong rate is worse than a missing one | | `X_TIMEZONE_INVALID` | not an IANA zone | `CET`, `+02:00`, or a typo | use `Europe/Berlin`, `America/New_York`, `UTC` |