feat(lint): multi: true 且无 filter 的 delete/update 在 authoring 期告警(#5482)

.changeset/flow-unbounded-bulk-write-lint.md

@@ -0,0 +1,70 @@
+---
+"@objectstack/lint": patch
+---
+
+feat(lint): warn when a `multi: true` delete/update is bounded by nothing — the declared whole-object write (#5482)
+
+A `delete_record` / `update_record` node that declares `multi: true` with no
+`filter` (or an empty one) writes the **whole object**: the executor forwards
+`where: {}` plus the bulk intent, the data engine classifies that as a legal
+`multi` call, and it lands on `driver.deleteMany` / `driver.updateMany` with no
+predicate. Every row, every run.
+
+That path only became authorable with #5393, which gave these nodes a bulk
+declaration at all — before it the executor never passed `options.multi`, so the
+engine refused every predicate write (`Delete requires an ID or
+options.multi=true`) and "empty filter + bulk intent" was not a reachable shape.
+Since then it has been reachable and **silent**: `filter` is optional, `multi` is
+optional, nothing related the two, and the author's only feedback was the step's
+`acted` row count — reported after the rows were gone. The common way to get
+here is not malice but an omission: declaring the bulk intent and forgetting the
+constraint.
+
+`os validate` / `os build` now report `flow-multi-write-unfiltered` for it:
+
+```
+flow 'nightly_purge' · node 'purge' (delete_record)
+  declares `multi: true` with no `filter` key — this is a WHOLE-OBJECT write,
+  by declaration: every row of 'lead' is deleted on every run. …
+```
+
+**A warning, not a gate.** An explicit whole-object purge is something the
+platform grants on purpose — the data engine's own dispatch case-set lists "bulk
+intent with no predicate at all" as a valid call — so the shape has a legitimate
+reading and the run-time path stays open. What was missing was only that the
+author hears about it *before* the rows go. For the same reason the fix is not a
+schema `refine`: forbidding the shape would delete an intent the engine grants.
+
+Two ways to satisfy the warning: write the constraint you mean into `filter`
+(the bounded-bulk reference shape is app-showcase's `showcase_inquiry_purge`), or
+confirm that emptying the object is the intent and keep it.
+
+**It does not duplicate the #3810 run-time guard, which judges a different
+fact.** That guard refuses a node when a condition the author *wrote*
+interpolated to nothing (`{record.ownr}` — a typo — leaving `{}`), and it is
+deliberately keyed on "a written condition is gone" rather than on "the filter is
+empty", because losing one of two conditions also widens the blast radius. So:
+
+| fact                          | judged by          | when      | verdict |
+|-------------------------------|--------------------|-----------|---------|
+| a written condition vanished  | #3810 filter guard | run time  | refuse  |
+| no condition was ever written | this rule          | authoring | warn    |
+
+A node with `filter: { owner: '{record.ownr}' }` is silent for this rule (a
+condition *is* written) and refused by that one; a node with no `filter` at all
+is warned about here and — correctly — allowed there. The diagnostic names the
+run-time guard so the two are not mistaken for one check.
+
+Reported at every nesting depth, which matters because a scheduled sweep whose
+per-item work sits in a `loop` body is the standard janitor shape: a finding
+inside a region carries the region scope (`flow 'x' · loop 'sweep' body · node
+'purge' (delete_record)`), on the traversal #5383/#5635 added to this family.
+
+Deliberately out of range: an empty **combinator** array (`{ $and: [] }`,
+`{ $or: [] }`). #5322/#5134 ruled those and every driver implements the ruling —
+empty `$and` is TRUE (so it *is* a whole-object write), empty `$or` is FALSE (so
+it matches nothing and must never be warned about) — but telling them apart
+requires the boolean-identity reduction, which already exists producer-side in
+each driver. A hand-written fourth copy inside a linter is how a scan and a
+validator come to answer with two different predicates, so that case is tracked
+separately instead.

packages/lint/src/index.ts

@@ -468,6 +468,7 @@ export {
   FLOW_DEFAULT_EDGE_WITH_CONDITION,
   FLOW_MULTIPLE_DEFAULT_EDGES,
   FLOW_INERT_NODE_CONDITION,
+  FLOW_MULTI_WRITE_UNFILTERED,
 } from './lint-flow-patterns.js';
 
 export { lintLivenessProperties } from './lint-liveness-properties.js';

packages/lint/src/lint-flow-patterns.test.ts

@@ -20,6 +20,7 @@ import {
   FLOW_DEFAULT_EDGE_WITH_CONDITION,
   FLOW_MULTIPLE_DEFAULT_EDGES,
   FLOW_INERT_NODE_CONDITION,
+  FLOW_MULTI_WRITE_UNFILTERED,
 } from './lint-flow-patterns.js';
 
 const CEL = (source: string) => ({ dialect: 'cel', source });
@@ -995,3 +996,219 @@ describe('#5383 — a recursive config scan does not double-report the container
     expect(fnds[0].where).not.toContain("node 'loop_leads'");
   });
 });
+
+/**
+ * #5482 — the declared WHOLE-OBJECT write: `multi: true` on a
+ * `delete_record` / `update_record` with nothing bounding it.
+ *
+ * Reachable only since #5393 gave these nodes a bulk declaration: before it the
+ * executor never passed `options.multi`, the engine refused every predicate
+ * write, and "empty filter + bulk" was not an authoring surface at all. Measured
+ * on `origin/main` before this rule existed, all four shapes below — top-level
+ * delete, empty-object filter, update, and the same node inside a `loop` body —
+ * returned `[]` from `lintFlowPatterns`. The only feedback an author got was the
+ * step's `acted` row count, after the rows were gone.
+ */
+
+/** A janitor flow: one bulk write node, scheduled, correctly `runAs: 'system'`. */
+function purgeFlow(nodeType: string, config: unknown) {
+  return {
+    flows: [{
+      name: 'nightly_purge',
+      runAs: 'system',
+      nodes: [
+        { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 3 * * *' } },
+        { id: 'purge', type: nodeType, config },
+      ],
+      edges: [{ id: 'e1', source: 'start', target: 'purge' }],
+    }],
+  };
+}
+
+describe('lintFlowPatterns — unbounded bulk write (#5482)', () => {
+  it('flags a delete_record with `multi: true` and NO filter', () => {
+    const fnds = lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', multi: true }));
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED);
+    expect(fnds[0].where).toBe("flow 'nightly_purge' · node 'purge' (delete_record)");
+    // Advisory: the engine's dispatch table grants "bulk intent, no predicate"
+    // on purpose, so the shape is not provably wrong (severity policy at the top
+    // of lint-flow-patterns.ts). `undefined` is how this family spells warning.
+    expect(fnds[0].severity).toBeUndefined();
+    // Says WHAT it does — the object by name, and that it is every row.
+    expect(fnds[0].message).toContain('no `filter` key');
+    expect(fnds[0].message).toContain('WHOLE-OBJECT write');
+    expect(fnds[0].message).toContain("every row of 'lead' is deleted");
+    expect(fnds[0].message).toContain('driver.deleteMany');
+    // The authority it cites is the delete dispatch that is actually extracted
+    // and case-set-pinned — not a hand-waved "the engine allows it".
+    expect(fnds[0].message).toContain('delete-dispatch case-set');
+    expect(fnds[0].message).toContain('multi with no predicate at all');
+    // …and that the only run-time feedback arrives too late to help.
+    expect(fnds[0].message).toMatch(/`acted` row count/);
+    expect(fnds[0].message).toMatch(/AFTER the rows are gone/);
+  });
+
+  it('flags an EMPTY filter the same way, and says which of the two it saw', () => {
+    const fnds = lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', filter: {}, multi: true }));
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED);
+    expect(fnds[0].message).toContain('an EMPTY `filter`');
+    expect(fnds[0].message).not.toContain('no `filter` key');
+  });
+
+  it('flags an update_record too, in the words of an overwrite', () => {
+    const fnds = lintFlowPatterns(
+      purgeFlow('update_record', { objectName: 'lead', fields: { status: 'stale' }, multi: true }),
+    );
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED);
+    expect(fnds[0].where).toBe("flow 'nightly_purge' · node 'purge' (update_record)");
+    expect(fnds[0].message).toContain("every row of 'lead' is overwritten");
+    expect(fnds[0].message).toContain('driver.updateMany');
+    // Update has no extracted dispatch module, so the message cites the branch
+    // itself rather than borrowing delete's case-set.
+    expect(fnds[0].message).toContain('bulk branch on `options.multi`');
+    expect(fnds[0].message).not.toContain('delete-dispatch case-set');
+  });
+
+  it('names the #3810 run-time guard and says the two judge DIFFERENT facts', () => {
+    const [f] = lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', multi: true }));
+    // Cross-naming, not duplication: the run-time guard refuses "a condition you
+    // WROTE is gone"; this rule warns "no condition was ever written".
+    expect(f.hint).toContain('#3810');
+    expect(f.hint).toMatch(/REFUSES this node at run time/);
+    expect(f.hint).toMatch(/a written condition is gone/);
+    expect(f.hint).toMatch(/the filter is empty/);
+    // Both ways out are offered, and the run-time path is explicitly NOT closed.
+    expect(f.hint).toMatch(/Write the constraint you mean/);
+    expect(f.hint).toMatch(/warning, not a gate/);
+    expect(f.hint).toContain('showcase_inquiry_purge');
+  });
+
+  describe('does NOT flag (false-positive guards)', () => {
+    it('a bulk write BOUNDED by a filter — the showcase purge shape', () => {
+      expect(
+        lintFlowPatterns(purgeFlow('delete_record', {
+          objectName: 'showcase_inquiry', filter: { status: 'closed' }, multi: true,
+        })),
+      ).toHaveLength(0);
+    });
+
+    it('a filter whose only condition is a TEMPLATE — that is #3810\'s fact, at run time', () => {
+      // `{record.ownr}` (a typo) interpolates to nothing and the run-time guard
+      // REFUSES the node. At authoring time the condition is written, so warning
+      // "nothing bounds this" here would be false — and would put two diagnostics
+      // on one defect, one of them wrong about what the author did.
+      expect(
+        lintFlowPatterns(purgeFlow('delete_record', {
+          objectName: 'lead', filter: { owner: '{record.ownr}' }, multi: true,
+        })),
+      ).toHaveLength(0);
+    });
+
+    it('no `multi` at all — the engine refuses that call BY NAME already', () => {
+      // `Delete requires an ID or options.multi=true`. Nothing silent to warn
+      // about, and #5482 is scoped to the declared-bulk shape.
+      expect(lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead' }))).toHaveLength(0);
+      expect(lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', filter: {} }))).toHaveLength(0);
+    });
+
+    it('`multi: false` — the declaration says the opposite', () => {
+      expect(lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', multi: false }))).toHaveLength(0);
+    });
+
+    it('`multi: \'true\'` (a string) — the schema refuses the node, so it cannot run', () => {
+      // The executor tests `cfg.multi === true` and the schema types the key
+      // `z.boolean()`; a string is a parse refusal, not declared bulk intent.
+      expect(lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', multi: 'true' }))).toHaveLength(0);
+    });
+
+    it('a node type that carries no `multi` declaration', () => {
+      // `get_record` does not write and has no bulk intent; `create_record` has
+      // neither `filter` nor `multi`. A stray key there is the schema's business.
+      expect(lintFlowPatterns(purgeFlow('get_record', { objectName: 'lead', multi: true }))).toHaveLength(0);
+      expect(lintFlowPatterns(purgeFlow('create_record', { objectName: 'lead', multi: true }))).toHaveLength(0);
+    });
+
+    it('a non-object `filter` — refused by name at execute time, so no run to describe', () => {
+      expect(
+        lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', filter: 'status = closed', multi: true })),
+      ).toHaveLength(0);
+    });
+
+    it('an empty COMBINATOR array — deliberately out of range, both directions', () => {
+      // #5322/#5134 ruled these and every driver implements the ruling: `$and: []`
+      // is TRUE (this one IS a whole-object write and goes unwarned — filed as a
+      // follow-up), `$or: []` is FALSE (matches nothing — warning about it would
+      // be a false alarm). Telling them apart needs the identity REDUCTION, which
+      // already exists three times producer-side; a fourth hand-written copy in a
+      // linter is the divergence `engine-delete-dispatch.ts` exists to prevent.
+      expect(
+        lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', filter: { $and: [] }, multi: true })),
+      ).toHaveLength(0);
+      expect(
+        lintFlowPatterns(purgeFlow('delete_record', { objectName: 'lead', filter: { $or: [] }, multi: true })),
+      ).toHaveLength(0);
+    });
+  });
+
+  /**
+   * The rule's main habitat. A scheduled sweep whose per-item work sits in a
+   * `loop` body is the standard shape for a janitor flow, so a rule that only
+   * saw top-level nodes would miss the case it was written for — the #5383/#5635
+   * blind spot, in the exact family that closed it.
+   */
+  describe('inside a nested region (#5383 / #5635)', () => {
+    it('flags a loop-body sweep, scoped to the region, exactly once', () => {
+      const fnds = lintFlowPatterns(loopBodyFlow({
+        nodes: [
+          { id: 'sweep', type: 'delete_record', config: { objectName: 'campaign_member', multi: true } },
+        ],
+        edges: [],
+      }));
+      expect(fnds).toHaveLength(1);
+      expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED);
+      expect(fnds[0].where).toBe(
+        "flow 'campaign_enrollment' · loop 'loop_leads' body · node 'sweep' (delete_record)",
+      );
+      // Not attributed to the enclosing container: the `loop`'s own config
+      // CONTAINS the body, but this rule reads named keys (`multi`, `filter`) off
+      // each node, and a `loop` declares neither — so there is no second copy.
+      expect(fnds[0].where).not.toContain("node 'loop_leads'");
+      expect(fnds[0].message).toContain("every row of 'campaign_member' is deleted");
+    });
+
+    it('flags an update_record two regions deep', () => {
+      const fnds = lintFlowPatterns(loopBodyFlow({
+        nodes: [{
+          id: 'loop_touchpoints', type: 'loop', label: 'Loop Touchpoints',
+          config: {
+            collection: '{lead.touchpoints}', itemVar: 'tp',
+            body: {
+              nodes: [{ id: 'reset', type: 'update_record', config: { objectName: 'touchpoint', fields: { done: false }, multi: true } }],
+              edges: [],
+            },
+          },
+        }],
+        edges: [],
+      }));
+      expect(fnds).toHaveLength(1);
+      expect(fnds[0].rule).toBe(FLOW_MULTI_WRITE_UNFILTERED);
+      expect(fnds[0].where).toBe(
+        "flow 'campaign_enrollment' · loop 'loop_leads' body → loop 'loop_touchpoints' body · " +
+        "node 'reset' (update_record)",
+      );
+    });
+
+    it('leaves a BOUNDED loop-body sweep alone', () => {
+      expect(lintFlowPatterns(loopBodyFlow({
+        nodes: [{
+          id: 'sweep', type: 'delete_record',
+          config: { objectName: 'campaign_member', filter: { lead_id: '{lead.id}' }, multi: true },
+        }],
+        edges: [],
+      }))).toHaveLength(0);
+    });
+  });
+});