From 165c1a4582ff00f7894fa1f918dc6aead9376918 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 02:02:33 +0000 Subject: [PATCH] feat(objectql): audit readonly references into their own `provenance` bucket (#4743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dangling-reference audit dropped every `readonly` reference field before reading a row. That skip had two grounds and #4556 removed one of them: the platform no longer writes a non-id into a reference column (the `sys_metadata_history.recorded_by` sentinel is NULL now). What the skip still covered was one family — the audit-provenance fields `created_by` / `updated_by` / `organization_id` injected by `applySystemFields` — and those hold genuine ids that genuinely dangle when a user or organization is deleted. They are audited now, and filed apart from `dangling`: a deleted actor and a broken business foreign key are different findings with different remedies. Two additive, optional report keys carry the new class, mirroring the unknown/absent split the report already makes: `provenance` (rows, same shape as `dangling`) and `provenanceUndetermined` (probes that could not run). `provenance` alone does not raise the summary warning — on an aged database it is non-empty on every healthy run, which is the #4747 broken alarm. The counts ride along whenever the line fires for a real finding. Scan order gained a third tier (security surface, business references, provenance-only) so a finite row budget still reaches the business findings it was built for, now that nearly every object has an auditable field. Part of #4743 (fact 2). engine.ts (fact 1) deliberately untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx --- .../dangling-audit-provenance-bucket.md | 61 +++++ .../engine-dangling-reference-audit.test.ts | 28 +- .../dangling-reference-audit.test.ts | 247 ++++++++++++++++-- .../src/integrity/dangling-reference-audit.ts | 205 ++++++++++++--- 4 files changed, 483 insertions(+), 58 deletions(-) create mode 100644 .changeset/dangling-audit-provenance-bucket.md diff --git a/.changeset/dangling-audit-provenance-bucket.md b/.changeset/dangling-audit-provenance-bucket.md new file mode 100644 index 0000000000..38c038bf7b --- /dev/null +++ b/.changeset/dangling-audit-provenance-bucket.md @@ -0,0 +1,61 @@ +--- +"@objectstack/objectql": minor +--- + +feat(objectql): the dangling-reference audit stops skipping `readonly` +references and files them in their own `provenance` bucket (#4743) + +`auditDanglingReferences` used to drop every `readonly` reference field before +reading a single row. That skip rested on two grounds, and #4556 removed one of +them: the platform no longer writes a NON-ID into a reference column +(`sys_metadata_history.recorded_by` stored the sentinel string `'system'`; it +stores `NULL` now). What the skip still covered afterwards was exactly one +family — the audit-provenance fields `created_by` / `updated_by` / +`organization_id` that `applySystemFields` injects, all `readonly: true`. + +Those hold **genuine ids, and genuine ids dangle**: delete one user and every +row they ever created points `created_by` at a row that is gone. "Who did this" +failing to resolve is precisely the question an audit trail exists to answer, +so the remaining skip was blindness rather than economy. The audit now probes +them. + +**They do not join `dangling`.** A deleted actor and a broken business foreign +key are different findings with different remedies (usually nothing to do vs. +re-seed the target or clear the link), and merging them would bury the second +under the first. Two new report keys carry the new class, mirroring the +unknown/absent split the report already makes everywhere else: + +| Key | Means | +|:--|:--| +| `provenance: DanglingReference[]` | a `readonly` provenance reference that resolves to nothing — same row shape as `dangling` | +| `provenanceUndetermined: number` | a provenance reference whose target could not be probed at all | + +Both are **additive and optional in the type**, exactly like `aborted`: an +existing consumer keeps compiling and keeps reading `dangling` with its meaning +unchanged (a link the model *declares* is broken). Every report this module +produces sets both explicitly. + +⚠️ **Expect `provenance` to be large on the first run against an aged +database.** One deleted user dangles every row they ever touched. That number +is pre-existing state being reported for the first time — not damage the audit +caught being done, and not a regression introduced by looking at it. + +For the same reason `provenance` **alone does not raise the summary warning**. +On a database of any age it is non-empty on every healthy run, and a line that +always fires is the #4747 broken alarm again — it would train its reader +straight past the run where `dangling` had something in it. The counts ride +along in the payload whenever the line fires for a real finding, and the +itemised rows are always in the returned report. `provenanceUndetermined` is +separate from `undetermined` for the same reason: on a stack that never +registers `sys_user`, every provenance value probes "cannot tell", and that is +a fact about which platform tables are mounted, not about the audited data. + +Scan order gained a third tier to keep the change from costing the signal it +sits next to: security surface, then objects carrying a business reference, +then the provenance-only remainder. Admitting the family means nearly every +object now has an auditable field, so without the tier a bounded run would +spend its row budget on tables carrying only provenance and never reach the +business findings the budget was built for. + +Part of #4743 (fact 2). The stale `assertReferencesResolve` comment in +`engine.ts` (fact 1) is deliberately untouched here. diff --git a/packages/objectql/src/engine-dangling-reference-audit.test.ts b/packages/objectql/src/engine-dangling-reference-audit.test.ts index ec74f007a4..8dee2a776f 100644 --- a/packages/objectql/src/engine-dangling-reference-audit.test.ts +++ b/packages/objectql/src/engine-dangling-reference-audit.test.ts @@ -50,7 +50,10 @@ const history = { id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, note: { name: 'note', label: 'Note', type: 'text' as const }, // `sys_metadata_history.recorded_by` in miniature: a readonly lookup the - // platform fills with the sentinel string `actor ?? 'system'`. + // platform USED to fill with the sentinel string `actor ?? 'system'` (NULL + // since #4556). The stored `'system'` below is therefore a legacy row — + // still the honest specimen for what a readonly reference that resolves to + // nothing looks like (#4743). recorded_by: { name: 'recorded_by', label: 'Recorded By', type: 'lookup' as const, reference: 'aud_permission_set', readonly: true, @@ -207,16 +210,31 @@ describe('[#4551] the engine reports the dangling rows its own `isSystem` exempt expect(snapshot()).toBe(before); }); - it('a readonly lookup holding a SENTINEL string is not reported', async () => { - // `recorded_by: 'system'` is not a user id and never was. #4441 skips it on - // the write path; the audit must not undo that by reporting the same value - // from the other side. + it('[#4743] a readonly lookup that resolves to nothing lands in `provenance`, not `dangling`', async () => { + // This case used to assert "not reported at all", on the grounds that the + // platform wrote the SENTINEL STRING `actor ?? 'system'` here. #4556 made + // that write NULL, so the only readonly references left are real ids — and + // a real id that names no row is a finding. It is filed apart from + // `dangling` because it answers a different question: not "a declared link + // is broken" but "the actor this row records is gone". + // + // Note what the retired assertion would have done: `dangling: []` still + // passes, because the finding MOVED rather than vanished. Asserting where + // it moved to is the only version of this test that can go red. await engine.insert( 'aud_history', { id: 'h1', note: 'n', recorded_by: 'system' }, { context: { isSystem: true } } as any, ); const out = await engine.inspectDanglingReferences({ objects: ['aud_history'] }); + expect(out.dangling).toEqual([]); expect(out.undetermined).toBe(0); + expect(out.provenance).toEqual([{ + objectName: 'aud_history', + recordId: 'h1', + field: 'recorded_by', + target: 'aud_permission_set', + value: 'system', + }]); }); it('an unregistered TARGET is `undetermined`, not a finding', async () => { diff --git a/packages/objectql/src/integrity/dangling-reference-audit.test.ts b/packages/objectql/src/integrity/dangling-reference-audit.test.ts index 54e9ae3889..378dfd0e3b 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.test.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.test.ts @@ -16,8 +16,10 @@ * - condemn on a failed probe → "an unprobeable target is undetermined" fails * - report on an existing row → "does NOT report a reference that resolves" fails * - add any write → "NEVER rewrites" fails (data JSON compared) - * - drop the readonly skip → "a readonly reference is not audited" fails * - drop the empty-value skip → "empty is not a reference" fails + * - restore the readonly SKIP → every [#4743] test below fails: the provenance + * bucket goes empty and the probe is never issued + * - merge provenance into `dangling` → the [#4743] separation tests fail */ import { describe, it, expect } from 'vitest'; @@ -48,10 +50,23 @@ const task: AuditableObject = { project: { type: 'lookup', reference: 'showcase_project' }, tags: { type: 'lookup', reference: 'showcase_tag', multiple: true }, // Audit-provenance shape: readonly, platform-minted (`applySystemFields` - // stamps `created_by` exactly like this). #4441 skips it on the write path - // because the value there is never the caller's; the audit skips it for the - // same reason — `sys_metadata_history.recorded_by` legitimately holds the - // SENTINEL STRING 'system'. + // stamps `created_by` exactly like this). #4441 still skips it on the WRITE + // path — the value there is never the caller's. The audit no longer skips + // it (#4743): it holds a genuine user id, and a deleted user dangles it. + created_by: { type: 'lookup', reference: 'sys_user', readonly: true }, + }, +}; + +/** + * [#4743] A table whose ONLY reference field is the injected provenance family + * — what `applySystemFields` leaves on an object that declares no lookup of its + * own. Before #4743 the audit read zero rows of a table shaped like this. + */ +const note: AuditableObject = { + name: 'showcase_note', + fields: { + id: { type: 'text', primaryKey: true }, + body: { type: 'text' }, created_by: { type: 'lookup', reference: 'sys_user', readonly: true }, }, }; @@ -218,21 +233,12 @@ describe('[#4551] dangling stored references are reported, never rewritten', () expect(JSON.stringify(rows)).toBe(before); // …and changed none of them }); - it('a READONLY reference field is not audited — its value was minted by the platform', async () => { - // Same judgment #4441 makes on the write path, and for the same reason: - // `stripReadonlyFields` removes a caller's value first, so what remains is - // the platform's. `sys_metadata_history.recorded_by` is the real case — a - // `lookup('sys_user')` filled with the SENTINEL STRING `actor ?? 'system'`. - const port = makePort({ - objects: [task], - rows: { showcase_task: [{ id: 't1', title: 'T', created_by: 'system' }] }, - }); - - const out = await auditDanglingReferences(port); - expect(out.dangling).toEqual([]); - // Not merely unreported — never even probed. - expect(port.probes).not.toContain('sys_user system'); - }); + // The former "a READONLY reference field is not audited" case lived here. It + // was retired by #4743 rather than re-spelled: its `dangling: []` assertion + // still PASSES under the new behaviour — the finding simply moved one bucket + // over — so keeping it would have been a test that is green because nothing + // is produced rather than because the logic is right. Its replacements, which + // assert where the value actually goes, are in the [#4743] block below. it('empty values are not references — null / "" / [] are skipped', async () => { // `deleteBehavior: 'set_null'` writes exactly these. Matching #4441's @@ -531,3 +537,204 @@ describe('[#4747] a run that was called off is not a finding about the data', () expect(noSignal.aborted).toBe(false); }); }); + +/** + * [#4743] "The user who created this is gone" is a finding, and it is not the + * same finding as a broken business foreign key. + * + * The `readonly` family used to be skipped whole. Two grounds; #4556 deleted + * one of them (the platform stopped writing the `recorded_by` SENTINEL STRING + * into a lookup column), and what the skip covered afterwards was only the + * audit-provenance family — genuine user/organization ids that genuinely + * dangle the moment their target row is deleted. + * + * Reverse verification, direction predicted BEFORE running it: restoring the + * wholesale skip turns every test in this block RED — `provenance` goes empty + * and the probe is never issued. Note the direction the *retired* test would + * have gone instead: its `dangling: []` assertion stays GREEN under the new + * behaviour, because the finding moved rather than vanished. That is exactly + * the "green because nothing is produced" trap, which is why it was replaced + * rather than re-spelled. + */ +describe('[#4743] provenance references are audited, in their OWN bucket', () => { + it('a dangling `created_by` is reported — in `provenance`, not in `dangling`', async () => { + // Delete a user and every row they created points at a row that is gone. + // "Who made this" failing to resolve is what an audit trail exists for. + const port = makePort({ + objects: [task], + rows: { + showcase_task: [ + { id: 't1', title: 'T', project: 'proj_real', created_by: 'usr_deleted' }, + ], + }, + existing: new Set(['showcase_project proj_real']), + }); + + const out = await auditDanglingReferences(port); + + // The business link is fine, so `dangling` must stay empty — and it must be + // empty for the RIGHT reason, which the bucket below is what proves. + expect(out.dangling).toEqual([]); + expect(out.provenance).toEqual([{ + objectName: 'showcase_task', + recordId: 't1', + field: 'created_by', + target: 'sys_user', + value: 'usr_deleted', + }]); + // …and it really was probed, which the old wholesale skip never did. + expect(port.probes).toContain('sys_user usr_deleted'); + }); + + it('a resolvable `created_by` is not reported at all', async () => { + const port = makePort({ + objects: [task], + rows: { showcase_task: [{ id: 't1', title: 'T', created_by: 'usr_alive' }] }, + existing: new Set(['sys_user usr_alive']), + }); + + const out = await auditDanglingReferences(port); + expect(out.provenance).toEqual([]); + expect(out.dangling).toEqual([]); + // Silent because it RESOLVED, not because nobody looked — without this the + // case would pass just as well under the old wholesale skip. + expect(port.probes).toContain('sys_user usr_alive'); + }); + + it('the two buckets never merge — a broken FK and a deleted user are filed apart', async () => { + // The whole point of B over C: one report, two questions, two remedies + // (re-seed the project vs. nothing to do about a user who left). + const port = makePort({ + objects: [task], + rows: { + showcase_task: [ + { id: 't1', title: 'T', project: 'proj_gone', created_by: 'usr_deleted' }, + ], + }, + }); + + const out = await auditDanglingReferences(port); + + expect(out.dangling.map((d) => d.field)).toEqual(['project']); + expect(out.provenance!.map((d) => d.field)).toEqual(['created_by']); + }); + + it('an unprobeable provenance target counts in `provenanceUndetermined`, never in `undetermined`', async () => { + // `sys_user` unregistered is a fact about which platform tables are + // mounted, not about the audited data — and `undetermined` raises the + // summary warning, so folding it in there would ring an alarm about the + // wrong thing on every run of a stack that never mounts `sys_user`. + const port = makePort({ + objects: [task], + rows: { + showcase_task: [ + { id: 't1', title: 'T', project: 'proj_real', created_by: 'usr_x' }, + ], + }, + unprobeable: new Set(['sys_user']), + existing: new Set(['showcase_project proj_real']), + }); + + const out = await auditDanglingReferences(port); + + expect(out.provenanceUndetermined).toBe(1); + expect(out.undetermined).toBe(0); + expect(out.provenance).toEqual([]); // unknown is not a verdict, here either + }); + + it('a table whose ONLY reference is provenance is now read — it used to be skipped whole', async () => { + const reads: string[] = []; + const port = makePort({ + objects: [note], + rows: { showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_deleted' }] }, + }); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { reads.push(o); return findSpy(o, opts); }; + + const out = await auditDanglingReferences(port); + + expect(reads).toEqual(['showcase_note']); + expect(out.scanned).toBe(1); + expect(out.provenance).toHaveLength(1); + }); + + it('provenance-only tables are scanned LAST, so a finite budget still answers the business question', async () => { + // Admitting the family means nearly every object has an auditable field, + // so without this the budget would be spent on tables carrying only + // provenance and the business findings would be what a bounded run never + // reached. Same argument as the security surface, one tier down. + const reads: string[] = []; + const port = makePort({ + // Registration order is deliberately the WORST case: provenance-only + // first, security surface last. + objects: [note, task, binding], + rows: { + showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_deleted' }], + showcase_task: [{ id: 't1', title: 'T', project: 'proj_gone' }], + sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_gone' }], + }, + }); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { reads.push(o); return findSpy(o, opts); }; + + await auditDanglingReferences(port); + + expect(reads).toEqual(['sys_position_permission_set', 'showcase_task', 'showcase_note']); + }); + + it('provenance ALONE does not raise the summary warning', async () => { + // On a database of any age this bucket is non-empty on every healthy run. + // A line that always fires is #4747's broken alarm, and it would train its + // reader straight past the run where `dangling` had something in it. + const port = makePort({ + objects: [note], + rows: { showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_deleted' }] }, + }); + + const out = await auditDanglingReferences(port); + + expect(out.provenance).toHaveLength(1); // found, and reported in the report + expect(port.warnings).toEqual([]); // …just not shouted about + }); + + it('…but rides along in the payload once the line fires for a real finding', async () => { + const port = makePort({ + objects: [task], + rows: { + showcase_task: [ + { id: 't1', title: 'T', project: 'proj_gone', created_by: 'usr_deleted' }, + ], + }, + unprobeable: new Set(['sys_user']), + }); + + const out = await auditDanglingReferences(port); + + const summary = port.warnings.find((w) => w[0].includes('#4551')); + expect(summary).toBeDefined(); + const meta = summary![1] as Record; + expect(meta.dangling).toBe(1); + expect(meta.provenanceUndetermined).toBe(1); + // Counted, never itemised: on an aged database this outnumbers every other + // finding, and the rows themselves are in the returned report. + expect(meta.provenance).toBe(0); + expect(meta.references).toEqual([ + 'showcase_task#t1.project → showcase_project#proj_gone', + ]); + expect(out.provenanceUndetermined).toBe(1); + }); + + it('a run with nothing to say still states both provenance buckets explicitly', async () => { + // Same reason `aborted: false` is explicit: a consumer must never have to + // guess whether `undefined` meant "clean" or "old report shape". + const port = makePort({ + objects: [binding], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_real' }] }, + existing: new Set(['sys_permission_set ps_real']), + }); + + const out = await auditDanglingReferences(port); + expect(out.provenance).toEqual([]); + expect(out.provenanceUndetermined).toBe(0); + }); +}); diff --git a/packages/objectql/src/integrity/dangling-reference-audit.ts b/packages/objectql/src/integrity/dangling-reference-audit.ts index 2cd4e931df..19cdec1e63 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.ts @@ -16,12 +16,15 @@ import { PLATFORM_OBJECTS_BY_PACKAGE } from '@objectstack/spec/system'; * kind: **the platform itself can still write a reference into the void, and * nothing says so.** * - * Removing the exemption is not the fix. Beyond the boot-ordering problem, the - * platform has legitimate non-id writes of its own — `sys_metadata_history. - * recorded_by` is a `lookup('sys_user')` the metadata repository fills with the - * SENTINEL STRING `actor ?? 'system'` (that one is already out of scope via - * #4441's `readonly` narrowing). Rejecting the platform's own write is not the - * right way to report the problem. Making it VISIBLE is. + * Removing the exemption is not the fix. Rejecting the platform's own write is + * not the right way to report the problem. Making it VISIBLE is. + * + * (Historical note, because it is load-bearing for the scope section below: the + * platform used to have a legitimate NON-ID write of its own — + * `sys_metadata_history.recorded_by`, a `lookup('sys_user')` the metadata + * repository filled with the SENTINEL STRING `actor ?? 'system'`. #4556 + * replaced that sentinel with NULL. Nothing writes a non-id into a reference + * column any more, which is what #4743 re-scoped the `readonly` skip on.) * * ## Reports; never rewrites * @@ -70,14 +73,55 @@ import { PLATFORM_OBJECTS_BY_PACKAGE } from '@objectstack/spec/system'; * datasource. `aborted` keeps the incompleteness loud (the report can never be * read as a clean bill of health) without spending the finding bucket on it. * + * ## …and PROVENANCE is a fourth answer (#4743) + * + * `readonly` reference fields used to be skipped outright, on two grounds. The + * first is #4441's and still holds: a non-system caller's value is stripped + * before the write (`stripReadonlyFields` / `stripReadonlyForInsert`), so what + * survives was minted by the platform and was never the caller's to answer for. + * The second was the `recorded_by` sentinel above — the platform wrote a + * NON-ID into a reference column, and probing it could only ever produce a + * finding about a value that was never an id. **#4556 deleted that ground**, + * and with it the only argument the skip ever had beyond "not the caller's + * fault". + * + * What the skip covered after that was exactly one family: the audit-provenance + * fields (`created_by` / `updated_by` / `organization_id`, all `readonly: true` + * from `applySystemFields`). Those hold GENUINE ids, and genuine ids dangle — + * delete one user and every row they ever created points `created_by` at a row + * that is gone. "Who did this" failing to resolve is precisely what an audit + * trail exists to answer, so skipping it is blindness rather than economy. + * + * They are still not the SAME finding as a broken business foreign key, so they + * do not share its bucket. `dangling` keeps meaning "a link the model declares + * is broken"; {@link DanglingReferenceReport.provenance} means "the actor this + * row records no longer exists". Same discipline, same reason, as the + * unknown/absent split above — mixing them would drown the business findings in + * the provenance ones, which is the failure mode this whole file is written + * against. + * + * ⚠️ **Expect `provenance` to be BIG the first time you look at it**, on any + * database that has ever deleted a user or an organization: one deleted user + * dangles every row they ever touched. That number is PRE-EXISTING STATE being + * reported for the first time — not damage this audit caught being done, and + * not a regression introduced by looking. It is also why `provenance` alone + * does NOT raise the summary warning: an alarm that fires on every run of an + * ordinarily-aged database is #4747's broken alarm all over again, and it would + * cost the buckets next to it the attention they were built for. The count + * rides along whenever the line fires for a real finding, and the itemised rows + * are always in the returned report for a caller that asked the question. + * + * The unknown/absent split applies INSIDE the new bucket too, and it bites at + * once: `undetermined` counts probes that could not run, and on a stack where + * `sys_user` is not registered every provenance value probes `null`. Folding + * those into `undetermined` would inflate a bucket that DOES raise the warning, + * with a fact about which platform tables are mounted rather than about the + * data being audited. Hence + * {@link DanglingReferenceReport.provenanceUndetermined}, on its own side of + * the same line. + * * ## Scope — the same judgments #4441 already made, not new ones * - * - **`readonly` reference fields are skipped**, exactly as the write-path - * check skips them: a non-system caller's value is stripped before the write - * (`stripReadonlyFields` / `stripReadonlyForInsert`), so what remains was - * minted by the platform — including the audit-provenance family - * (`created_by` / `updated_by` / `organization_id`, all `readonly: true` from - * `applySystemFields`) and the `recorded_by` sentinel above. * - **Which fields are references** is `referenceTargetOf` — the single * arbiter the write-path check and the expand gate already share, covering * `lookup` / `master_detail` / `user` / `tree`. A hand-written type list here @@ -134,6 +178,38 @@ export interface DanglingReferenceReport { * it explicitly. */ aborted?: boolean; + /** + * [#4743] Audit-provenance references that resolve to nothing: `created_by` / + * `updated_by` / `organization_id` — the `readonly` family `applySystemFields` + * injects — naming a user or organization that no longer exists. + * + * Its own bucket rather than an entry in `dangling`, because it answers a + * different question with a different remedy: `dangling` says a link the + * model DECLARES is broken (re-seed the target, or clear the link); + * `provenance` says the actor a row records has since been deleted, which is + * usually nothing to fix and everything to know. + * + * **Read a large number here as history, not as damage.** Every row a deleted + * user ever created lands in it, and this is the first release that reports + * them at all (before #4743 the whole family was skipped) — so the first run + * on an aged database measures accumulated state, not a new defect. + * + * Same optionality as {@link DanglingReferenceReport.aborted}, for the same + * reason: hand-written reports (test doubles) predate the key. Every report + * this module produces sets it. + */ + provenance?: DanglingReference[]; + /** + * [#4743] Provenance reference values whose target could NOT be probed — the + * `undetermined` axis, kept on the provenance side of the line. + * + * Separate from {@link DanglingReferenceReport.undetermined} because that + * count raises the summary warning and this one must not: on a stack that + * never registers `sys_user`, EVERY provenance value probes `null`, and a + * fact about which platform tables are mounted would otherwise arrive + * disguised as a fact about the audited data. + */ + provenanceUndetermined?: number; } /** Minimal object shape the audit reads — duck-typed so tests need no registry. */ @@ -218,33 +294,75 @@ function isEmptyStoredReference(v: unknown): boolean { return v === null || v === undefined || v === ''; } +/** One reference field worth reading an object for, and which bucket it feeds. */ +interface AuditableField { + name: string; + target: string; + /** + * [#4743] `true` for a `readonly` reference — the audit-provenance family. + * Findings on it are still findings; they just answer a different question, + * so they land in {@link DanglingReferenceReport.provenance}. + */ + provenance: boolean; +} + /** - * Reference fields worth auditing on one object: declared target, not - * `readonly`. Returns `[]` for an object with none, which is how the audit - * avoids reading a single row of the vast majority of tables. + * Reference fields worth auditing on one object: everything with a declared + * target, each tagged with the bucket its findings belong in. Returns `[]` for + * an object with none, which is how the audit avoids reading a single row of + * the tables that have nothing referential on them at all. + * + * `readonly` is no longer a reason to drop a field (#4743) — it is the reason + * to file its findings separately. See the provenance section in the module + * header for why the skip's second ground stopped existing at #4556. */ -function auditableReferenceFields(obj: AuditableObject): Array<{ name: string; target: string }> { +function auditableReferenceFields(obj: AuditableObject): AuditableField[] { const fields = obj?.fields; if (!fields || typeof fields !== 'object') return []; - const out: Array<{ name: string; target: string }> = []; + const out: AuditableField[] = []; for (const [name, def] of Object.entries(fields)) { - if ((def as { readonly?: unknown })?.readonly === true) continue; const target = referenceTargetOf(def); if (!target) continue; - out.push({ name, target }); + out.push({ name, target, provenance: (def as { readonly?: unknown })?.readonly === true }); } return out; } +/** An object paired with the reference fields the audit will read it for. */ +interface AuditTarget { + obj: AuditableObject; + refFields: AuditableField[]; +} + /** - * Security-surface objects first, everything else after, each group keeping - * registration order so a run is deterministic. + * Scan order for a finite budget: security surface, then objects carrying a + * business reference, then the provenance-only remainder. Each group keeps + * registration order so a run is deterministic, and objects with no reference + * field at all are dropped here — they are the ones the audit reads zero rows + * of. + * + * The third tier is #4743's doing and is the reason it is safe. Admitting the + * provenance family means nearly EVERY object now has an auditable field + * (`applySystemFields` injects `created_by` almost everywhere), so without an + * ordering rule a bounded scan would start spending its row budget on tables + * that carry only provenance — and the business findings that budget was built + * for would be the ones it ran out before reaching. Same argument as + * {@link SECURITY_SURFACE_OBJECTS}, one tier down: when the budget is finite, + * order is what decides which question actually gets answered. */ -function prioritise(objects: AuditableObject[]): AuditableObject[] { - const security: AuditableObject[] = []; - const rest: AuditableObject[] = []; - for (const o of objects) (SECURITY_SURFACE_OBJECTS.has(o?.name) ? security : rest).push(o); - return [...security, ...rest]; +function prioritise(objects: AuditableObject[]): AuditTarget[] { + const security: AuditTarget[] = []; + const business: AuditTarget[] = []; + const provenanceOnly: AuditTarget[] = []; + for (const obj of objects) { + const refFields = auditableReferenceFields(obj); + if (refFields.length === 0) continue; // nothing referential here — read nothing + const tier = SECURITY_SURFACE_OBJECTS.has(obj?.name) + ? security + : refFields.some((f) => !f.provenance) ? business : provenanceOnly; + tier.push({ obj, refFields }); + } + return [...security, ...business, ...provenanceOnly]; } /** @@ -257,8 +375,13 @@ export async function auditDanglingReferences( port: DanglingReferenceAuditPort, options?: DanglingReferenceAuditOptions, ): Promise { - const report: DanglingReferenceReport = { + // The optional keys are optional in the TYPE only (hand-written test doubles + // predate them); every report this function produces sets them, so the local + // view of it requires them and no call site below has to guard. + const report: DanglingReferenceReport & + Required> = { scanned: 0, dangling: [], undetermined: 0, unreadableObjects: [], truncatedObjects: [], + provenance: [], provenanceUndetermined: 0, aborted: false, }; @@ -317,15 +440,13 @@ export async function auditDanglingReferences( return answer; }; - objects: for (const obj of prioritise(all)) { + objects: for (const { obj, refFields } of prioritise(all)) { if (report.scanned >= maxRows) break; // Called off before this object was read: it was never attempted, so it is // not a finding about the object — the run reports that it stopped instead. if (calledOff()) { report.aborted = true; break; } const name = obj?.name; if (!name || (only && !only.has(name))) continue; - const refFields = auditableReferenceFields(obj); - if (refFields.length === 0) continue; // nothing referential here — read nothing const budget = Math.min(rowsPerObject, maxRows - report.scanned); let rows: Array>; @@ -353,7 +474,7 @@ export async function auditDanglingReferences( if (rows.length >= budget) report.truncatedObjects.push(name); for (const row of rows) { - for (const { name: field, target } of refFields) { + for (const { name: field, target, provenance } of refFields) { const raw = row?.[field]; if (isEmptyStoredReference(raw)) continue; const values = Array.isArray(raw) ? raw : [raw]; @@ -363,9 +484,16 @@ export async function auditDanglingReferences( if (typeof v === 'object') continue; const answer = await exists(target, v); if (answer === 'called-off') { report.aborted = true; break objects; } - if (answer === null) { report.undetermined++; continue; } + // [#4743] Both verdicts are routed by the field's class, not merged: + // a provenance answer never lands in a bucket a business reference + // shares, in EITHER direction (absent or unknown). + if (answer === null) { + if (provenance) report.provenanceUndetermined++; + else report.undetermined++; + continue; + } if (answer) continue; - report.dangling.push({ + (provenance ? report.provenance : report.dangling).push({ objectName: name, recordId: String(row?.id ?? ''), field, @@ -377,6 +505,12 @@ export async function auditDanglingReferences( } } + // [#4743] The provenance buckets deliberately do NOT appear in this + // condition. On a database of any age they are non-empty on every healthy + // run, and a line that always fires is the #4747 broken alarm — it would + // train its reader past the one run where `dangling` had something in it. + // They ride along whenever the line fires for a real finding; the full + // report always carries them for a caller that came looking. if (report.dangling.length || report.undetermined || report.unreadableObjects.length) { port.warn?.('[integrity] stored references that resolve to nothing (#4551)', { scanned: report.scanned, @@ -387,6 +521,11 @@ export async function auditDanglingReferences( // Carried into the log line too: findings from a run that stopped early // are real, but its silence about everything else is not a verdict. aborted: report.aborted, + // Counted here, never itemised: on a database that has deleted users this + // can outnumber every other finding by orders of magnitude, and the rows + // themselves are in the returned report. + provenance: report.provenance.length, + provenanceUndetermined: report.provenanceUndetermined, references: report.dangling.map( (d) => `${d.objectName}#${d.recordId}.${d.field} → ${d.target}#${d.value}`, ),