test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics (#3792)

`attachment-read-visibility.test.ts` faked its engine with a matcher that
understood implicit equality and `$in` and nothing else — no `$and`, no
`$or`, no `$not`. Every assertion could therefore only check the *shape* of
the filter `computeParentVisibilityFilter` emits, never the rows that filter
selects. That is a poor bargain for a predicate whose whole job is
narrowing: it pairs a discriminator with an id list per branch, and anything
that widens `$or` loosens that pairing while leaving shape assertions green.

- The matcher now implements the protocol for real, mirroring
  driver-memory's `memory-matcher.ts` and formula's `matches-filter.ts`:
  every key in a filter object ANDs, `$or` ORs its branches, a branch's own
  contents still AND. Unimplemented operators now throw rather than being
  ignored — a silently under-implemented test double is the failure mode
  this change exists to close.
- New case asserts the ROWS a multi-parent-type scope returns: rows whose
  discriminator matches a branch but whose id is absent from that branch's
  id list — including one borrowing the sibling branch's id — are excluded,
  pinning the per-branch pairing itself.
- A conformance block pins the harness against the same 2x2 fixture and
  expectations as `memory-matcher-or-semantics.test.ts`,
  `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`,
  so the four cannot drift apart, plus a case proving the matcher tells a
  correctly paired scope from a widened one.

Test-only; no runtime change. service-storage: 212/212 green.

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/attachment-read-visibility-real-filter-semantics.md

@@ -0,0 +1,30 @@
+---
+---
+
+test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics
+
+Test-only — releases nothing.
+
+`attachment-read-visibility.test.ts` faked its engine with a matcher that
+understood implicit equality and `$in` and nothing else: no `$and`, no `$or`,
+no `$not`. Every assertion in the file could therefore only check the *shape*
+of the filter `computeParentVisibilityFilter` emits, never the rows that filter
+selects — a poor bargain for a predicate whose whole job is narrowing.
+
+Changes:
+
+- The harness matcher now implements the protocol for real, mirroring
+  `driver-memory/memory-matcher.ts` and `formula/matches-filter.ts`: every key
+  in a filter object ANDs, `$or` ORs its branches, and a branch's own contents
+  still AND. Operators it does not implement now **throw** instead of being
+  ignored — a silently under-implemented test double is the failure mode this
+  change exists to close.
+- A new case asserts the **rows** a multi-parent-type scope returns, not just
+  its shape: rows whose discriminator matches a branch but whose id is absent
+  from that branch's id list (including one that borrows the sibling branch's
+  id) are excluded, so the per-branch pairing itself is pinned.
+- A conformance block pins the harness matcher against the same 2x2 fixture and
+  expectations as `memory-matcher-or-semantics.test.ts`,
+  `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so
+  the four cannot drift apart, plus a case proving the matcher actually
+  distinguishes a correctly paired scope from a widened one.

packages/services/service-storage/src/attachment-read-visibility.test.ts

@@ -6,6 +6,77 @@ import type { AttachmentLifecycleEngine, AttachmentReadMiddlewareCtx } from './a
 
 const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() });
 
+/**
+ * Filter Protocol evaluator for the fake engine below — real `$and` / `$or` /
+ * `$not` semantics, not a shape check.
+ *
+ * Mirrors `driver-memory`'s `memory-matcher.ts` and `formula`'s
+ * `matches-filter.ts`: **every key inside one filter object ANDs**, a `$or`
+ * array ORs its branches, and a branch's own contents still AND.
+ *
+ * This used to understand neither logical operators nor anything but `$in`,
+ * so these tests could only assert the *shape* of the filter the middleware
+ * emits, never the rows that filter selects. That is a poor bargain for a
+ * predicate whose whole job is narrowing: `computeParentVisibilityFilter`
+ * pairs a discriminator with an id list per branch, and any bug that widens
+ * `$or` — the class #3774 fixed in the SQL compiler — turns that pairing into
+ * something looser while leaving every shape assertion green. A fake that
+ * silently under-implements the protocol cannot see a widening bug, so this
+ * one implements it for real and **throws** on anything it does not, rather
+ * than quietly matching.
+ *
+ * Values compare as strings on purpose: `computeParentVisibilityFilter`
+ * stringifies every `parent_id` when it builds the `$in` list, so a numeric
+ * row value still has to match its stringified filter value.
+ */
+function matchWhere(row: any, where: any): boolean {
+  if (where === null || where === undefined) return true;
+  if (typeof where !== 'object' || Array.isArray(where)) {
+    throw new Error(`harness matcher: a filter must be an object, got ${JSON.stringify(where)}`);
+  }
+  // A filter object is the AND of every one of its entries.
+  return Object.entries(where).every(([key, val]) => {
+    if (key === '$and') {
+      if (!Array.isArray(val)) throw new Error('harness matcher: $and expects an array');
+      return val.every((branch) => matchWhere(row, branch));
+    }
+    if (key === '$or') {
+      if (!Array.isArray(val)) throw new Error('harness matcher: $or expects an array');
+      // Each branch is evaluated whole — the keys inside it still AND.
+      return val.some((branch) => matchWhere(row, branch));
+    }
+    if (key === '$not') return !matchWhere(row, val);
+    if (key.startsWith('$')) {
+      throw new Error(`harness matcher: unsupported logical operator ${key}`);
+    }
+    return matchField(row[key], val);
+  });
+}
+
+/** One `field: <spec>` entry. Multiple operators on the same field AND together. */
+function matchField(actual: any, spec: any): boolean {
+  if (spec === null || spec === undefined) return actual === null || actual === undefined;
+  if (Array.isArray(spec)) {
+    throw new Error('harness matcher: a bare array is not a field spec — use { $in: [...] }');
+  }
+  if (typeof spec !== 'object') return String(actual) === String(spec);
+  return Object.entries(spec).every(([op, target]) => {
+    switch (op) {
+      case '$eq':
+        return String(actual) === String(target);
+      case '$ne':
+        return String(actual) !== String(target);
+      case '$in':
+        return (target as unknown[]).map(String).includes(String(actual));
+      case '$nin':
+        return !(target as unknown[]).map(String).includes(String(actual));
+      default:
+        // Loud on purpose — an ignored operator is how the original blind spot happened.
+        throw new Error(`harness matcher: unsupported operator ${op} — implement it, don't let it match silently`);
+    }
+  });
+}
+
 /**
  * Fake engine modelling: a sys_attachment table (system pre-scan) + a
  * per-parent-object visibility map keyed by userId. A parent find under a
@@ -19,16 +90,6 @@ function install(opts: {
   let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise<void>) => Promise<void>;
   const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> };
 
-  const matchWhere = (row: any, where: any): boolean => {
-    if (!where || typeof where !== 'object') return true;
-    return Object.entries(where).every(([k, v]) => {
-      if (v && typeof v === 'object' && Array.isArray((v as any).$in)) {
-        return (v as any).$in.map(String).includes(String(row[k]));
-      }
-      return String(row[k]) === String(v);
-    });
-  };
-
   const engine: AttachmentLifecycleEngine = {
     registerHook: () => {},
     registerMiddleware: (fn) => {
@@ -51,7 +112,18 @@ function install(opts: {
     update: async () => ({}),
   };
   installAttachmentReadVisibility(engine, silentLogger());
-  return { mw, calls };
+  return {
+    mw,
+    calls,
+    /**
+     * Ids the given `where` actually selects from the fixture — i.e. what a
+     * spec-conformant driver would return once the middleware has narrowed
+     * the query. Asserting on this (not just on the filter's shape) is what
+     * makes a widened scope visible.
+     */
+    selectIds: (where: unknown): string[] =>
+      opts.attachments.filter((r) => matchWhere(r, where)).map((r) => String(r.id)),
+  };
 }
 
 /** Drive a read op through the middleware and return the resulting where. */
@@ -111,6 +183,34 @@ describe('installAttachmentReadVisibility', () => {
     });
   });
 
+  it('the multi-parent $or admits only rows matching BOTH keys of one branch', async () => {
+    // Each parent type has a visible and an invisible record, plus a row whose
+    // parent_object belongs to one branch while its parent_id appears only in
+    // the OTHER branch's id list. Nothing but a genuine per-branch AND excludes
+    // all three, so this pins the pairing itself rather than the filter's shape.
+    const { mw, selectIds } = install({
+      attachments: [
+        { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, // visible case
+        { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, // case u1 cannot read
+        { id: 'a3', parent_object: 'att_todo', parent_id: 't1' }, // visible todo
+        { id: 'a4', parent_object: 'att_todo', parent_id: 't2' }, // todo u1 cannot read
+        { id: 'a5', parent_object: 'att_case', parent_id: 't1' }, // object of branch 1, id of branch 2
+      ],
+      visible: { att_case: { u1: ['c1'] }, att_todo: { u1: ['t1'] } },
+    });
+    const { where } = await runRead(mw, {});
+
+    expect(where).toEqual({
+      $or: [
+        { parent_object: 'att_case', parent_id: { $in: ['c1'] } },
+        { parent_object: 'att_todo', parent_id: { $in: ['t1'] } },
+      ],
+    });
+    // The rows that filter actually returns — a2/a4 (right parent type, wrong
+    // id) and a5 (right type, an id borrowed from the sibling branch) are out.
+    expect(selectIds(where)).toEqual(['a1', 'a3']);
+  });
+
   it('denies all when no candidate parent is visible', async () => {
     const { mw } = install({
       attachments: [{ id: 'a3', parent_object: 'att_secret', parent_id: 's1' }],
@@ -178,3 +278,100 @@ describe('installAttachmentReadVisibility', () => {
     expect(calls.parentFinds).toHaveLength(0);
   });
 });
+
+/**
+ * Who tests the test double? The row assertions above are only worth as much
+ * as the matcher evaluating them, so it gets the same 2x2 fixture and the same
+ * expectations as the three production backends:
+ * `driver-memory/memory-matcher-or-semantics.test.ts`,
+ * `formula/matches-filter-or-semantics.test.ts`, and
+ * `driver-sql/sql-driver-or-filter.test.ts`. If this harness ever drifts from
+ * them, a read scope it declares safe would not be safe in the engine.
+ */
+describe('harness matcher — Filter Protocol $and/$or/$not semantics', () => {
+  const ROWS = [
+    { id: '1', a: 'x', b: 'y', c: 'z' },
+    { id: '2', a: 'x', b: 'zz', c: 'z' },
+    { id: '3', a: 'qq', b: 'y', c: 'z' },
+    { id: '4', a: 'qq', b: 'zz', c: 'z' },
+  ];
+  const ids = (filter: any): string[] => ROWS.filter((r) => matchWhere(r, filter)).map((r) => r.id);
+
+  it('ANDs the keys of a single multi-key branch', () => {
+    expect(ids({ $or: [{ a: 'x', b: 'y' }] })).toEqual(['1']);
+  });
+
+  it('ANDs the keys of each branch independently', () => {
+    expect(ids({ $or: [{ a: 'x', b: 'y' }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']);
+  });
+
+  it('ANDs operator-object keys within a branch', () => {
+    expect(ids({ $or: [{ a: { $eq: 'x' }, b: { $ne: 'zz' } }] })).toEqual(['1']);
+  });
+
+  it('ANDs keys inside a $or nested in a $or branch', () => {
+    expect(ids({ $or: [{ $or: [{ a: 'x', b: 'zz' }] }] })).toEqual(['2']);
+  });
+
+  it('ANDs multiple operators on ONE field within a branch', () => {
+    expect(ids({ $or: [{ a: { $ne: 'qq', $eq: 'x' }, b: 'y' }] })).toEqual(['1']);
+  });
+
+  it('OR-s a $and branch against a sibling multi-key branch', () => {
+    expect(ids({ $or: [{ $and: [{ a: 'x' }, { b: 'y' }] }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']);
+  });
+
+  it('ANDs a $and with a sibling key in the same branch, either order', () => {
+    expect(ids({ $or: [{ c: 'nope' }, { $and: [{ a: 'qq' }], b: 'y' }] })).toEqual(['3']);
+    expect(ids({ $or: [{ c: 'nope' }, { b: 'y', $and: [{ a: 'qq' }] }] })).toEqual(['3']);
+  });
+
+  it('keeps single-key $or branches as a plain OR', () => {
+    expect(ids({ $or: [{ a: 'x' }, { b: 'y' }] })).toEqual(['1', '2', '3']);
+  });
+
+  it('ANDs a $or with a sibling top-level field key', () => {
+    expect(ids({ $or: [{ a: 'x' }, { b: 'y' }], b: 'zz' })).toEqual(['2']);
+  });
+
+  it('negates with $not', () => {
+    expect(ids({ $not: { a: 'x' } })).toEqual(['3', '4']);
+  });
+
+  it('tells a correctly paired scope apart from a widened one', () => {
+    // The proof this matcher can go red: the same clauses, once AND-ed per
+    // branch and once flattened to key-level ORs. If both returned the same
+    // rows, every row assertion in this file would be decorative.
+    const attachments = [
+      { id: 'a1', parent_object: 'att_case', parent_id: 'c1' },
+      { id: 'a2', parent_object: 'att_case', parent_id: 'c2' },
+      { id: 'a3', parent_object: 'att_todo', parent_id: 't1' },
+      { id: 'a4', parent_object: 'att_todo', parent_id: 't2' },
+    ];
+    const pick = (f: any) => attachments.filter((r) => matchWhere(r, f)).map((r) => r.id);
+
+    const correct = {
+      $or: [
+        { parent_object: 'att_case', parent_id: { $in: ['c1'] } },
+        { parent_object: 'att_todo', parent_id: { $in: ['t1'] } },
+      ],
+    };
+    const widened = {
+      $or: [
+        { parent_object: 'att_case' },
+        { parent_id: { $in: ['c1'] } },
+        { parent_object: 'att_todo' },
+        { parent_id: { $in: ['t1'] } },
+      ],
+    };
+    expect(pick(correct)).toEqual(['a1', 'a3']);
+    expect(pick(widened)).toEqual(['a1', 'a2', 'a3', 'a4']); // every row in the tenant
+  });
+
+  it('throws instead of silently ignoring an operator it does not implement', () => {
+    // The original blind spot in one line: an unimplemented operator must not
+    // quietly evaluate to a match.
+    expect(() => ids({ a: { $gt: 'x' } })).toThrow(/unsupported operator \$gt/);
+    expect(() => ids({ $nor: [{ a: 'x' }] })).toThrow(/unsupported logical operator \$nor/);
+  });
+});