feat(objectql): 悬空引用巡检纳入 readonly 溯源族,单独分桶为 provenance (#4743 事实二)

.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.

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 () => {

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<string, unknown>;
+ 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);
+ });
+});