refactor(core,plugin-audit,service-storage,plugin-reports): give the __ operation-private-key convention a single owner (#7284)

.changeset/operation-private-keys-single-owner.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/core": patch
+"@objectstack/plugin-audit": patch
+"@objectstack/service-storage": patch
+"@objectstack/plugin-reports": patch
+---
+
+refactor(core,plugin-audit,service-storage,plugin-reports): give the `__` operation-private-key convention a single owner (#7284)
+
+`withoutOperationPrivateKeys` — the rule that a consumer forwarding a caller's
+execution envelope to a question about a DIFFERENT object must first drop the
+`__`-prefixed keys plugin-security stamped for the operation in flight — had been
+hand-copied into three packages: `plugin-audit`'s comment access hooks (#7141),
+`service-storage`'s attachment access hooks (#7145) and `plugin-reports`' report
+service (#7204). Each carried its own `OPERATION_PRIVATE_KEY_PREFIX` and its own
+doc block, and the prose had already diverged while the code still agreed — the
+shape that makes a later divergence in behaviour hard to notice.
+
+The helper now lives once, in `@objectstack/core`
+(`security/operation-private-keys.ts`), exported from the package root. Core is
+the only candidate all three consumers already depend on: `plugin-security` is
+the producer of the convention and the most honest owner, but none of the three
+depends on it and a string-prefix filter does not justify three new dependency
+edges onto a plugin; `@objectstack/spec` is fenced off by Prime Directive #2. The
+new home sits beside `assemble-execution-context.ts`, which owns the other end of
+the same lifecycle — that file is where an `ExecutionContext` is built at a
+transport entry point, this one is where it is stripped back down before being
+forwarded.
+
+The full reasoning moved with the code rather than being thinned: which keys the
+middleware stamps and why each is a widening input, why they are dropped by
+PREFIX and never by a name list, and why the fresh copy is load-bearing in both
+directions. Each consumer keeps only its own local half — which object *its*
+gates actually ask about — and points at the shared home.
+
+No behaviour change: the three copies were byte-equivalent, and all three
+packages' suites pass unchanged. Two new pins at the home cover it — the rule's
+own behaviour, which no package-level test had ever asserted directly, and a
+repository-shape pin that turns red if a fourth file declares its own copy.

packages/core/src/security/index.ts

@@ -131,3 +131,11 @@ export {
 
 // ADR-0091 D1/D2 — grant validity windows, the shared resolution-time predicate.
 export { isGrantActive, isGrantExpired, type GrantValidityWindow } from './grant-validity.js';
+
+// #7284 — the `__` operation-private-key convention, the CONSUMER half of the
+// ExecutionContext lifecycle `assemble-execution-context.ts` opens. One owner
+// for the rule three packages had hand-copied (#7141 / #7145 / #7204).
+export {
+  OPERATION_PRIVATE_KEY_PREFIX,
+  withoutOperationPrivateKeys,
+} from './operation-private-keys.js';

packages/core/src/security/operation-private-keys.pin.test.ts

@@ -0,0 +1,127 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#7284] The `__` operation-private-key convention has exactly ONE owner.
+ *
+ * This is the pin the extraction is worth having. The three copies #7284 found
+ * were byte-equivalent in behaviour and each was covered by its own package's
+ * tests, so nothing in the repository went red while the rule was being copied
+ * by hand a third time — the finding was made by a human reading three diffs
+ * months apart. A fourth consumer is written the same way the first three were:
+ * by opening the nearest existing one and copying the block out of it. Extracting
+ * the helper without pinning it just resets that counter to one.
+ *
+ * So the assertion is about the SHAPE of the repository, not about behaviour: no
+ * file outside this module may declare its own `OPERATION_PRIVATE_KEY_PREFIX` or
+ * its own `withoutOperationPrivateKeys`. A fourth author who copies the block
+ * turns this red the first time they run the suite, with a message naming the
+ * import to use instead.
+ *
+ * ⛔ Scope, deliberately narrow — this pin does NOT try to detect "a consumer
+ * that should have used the helper and did not". That is the interesting
+ * question and it is not decidable by scanning: forwarding an envelope is
+ * spelled a dozen ways, and a regex ambitious enough to catch them all would be
+ * a false-red generator, which is worse than the gap (an inert or noisy gate
+ * reads as a gate that is watching — `validate-security-posture.ts`'s hazard).
+ * What IS decidable is redeclaration, which is exactly how all three copies got
+ * here.
+ *
+ * Reworded freely: the pin matches DECLARATIONS, not mentions. Documentation,
+ * comments and tests may name either symbol as much as they like.
+ */
+
+import { readFileSync, readdirSync, statSync } from 'node:fs';
+import { dirname, join, relative, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+import { describe, it, expect } from 'vitest';
+
+const HERE = dirname(fileURLToPath(import.meta.url));
+/** …/packages/core/src/security → repo root */
+const REPO_ROOT = resolve(HERE, '../../../..');
+const PACKAGES = join(REPO_ROOT, 'packages');
+
+/** The one file allowed to declare the convention. */
+const HOME = join(HERE, 'operation-private-keys.ts');
+
+/**
+ * A DECLARATION of either symbol — `const OPERATION_PRIVATE_KEY_PREFIX =` or
+ * `function withoutOperationPrivateKeys(`, with or without `export`.
+ *
+ * Anchored at a statement start so that imports (`import { … }`), re-exports
+ * (`export { … } from`), calls and prose never match. Both spellings a copy
+ * could plausibly take are covered: a `function` declaration is what all three
+ * copies used, and `const … =` catches the arrow-function rewrite.
+ */
+const DECLARATION =
+  /^\s*(?:export\s+)?(?:const|let|var|function)\s+(OPERATION_PRIVATE_KEY_PREFIX|withoutOperationPrivateKeys)\b\s*[=(<]/gm;
+
+const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', '.turbo', 'coverage', '.next']);
+
+/** Every `.ts`/`.tsx` file under `packages/`, excluding build output. */
+function sourceFiles(dir: string, out: string[] = []): string[] {
+  for (const entry of readdirSync(dir)) {
+    if (SKIP_DIRS.has(entry)) continue;
+    const full = join(dir, entry);
+    if (statSync(full).isDirectory()) sourceFiles(full, out);
+    else if (/\.tsx?$/.test(entry) && !entry.endsWith('.d.ts')) out.push(full);
+  }
+  return out;
+}
+
+describe('the `__` operation-private-key convention has one owner (#7284)', () => {
+  it('is declared in exactly one file, and that file is the shared home', () => {
+    const offenders: string[] = [];
+
+    for (const file of sourceFiles(PACKAGES)) {
+      if (file === HOME) continue;
+      const text = readFileSync(file, 'utf8');
+      DECLARATION.lastIndex = 0;
+      if (DECLARATION.test(text)) offenders.push(relative(REPO_ROOT, file));
+    }
+
+    expect(
+      offenders,
+      offenders.length === 0
+        ? ''
+        : [
+            'These files declare their own copy of the `__` operation-private-key convention:',
+            ...offenders.map((f) => `  - ${f}`),
+            '',
+            'That rule has a single owner since #7284. Import it instead:',
+            '',
+            "  import { withoutOperationPrivateKeys } from '@objectstack/core';",
+            '',
+            'The reasoning — why a consumer must drop these keys, why by PREFIX and',
+            'never by a name list, and why the copy is load-bearing in both',
+            'directions — lives at packages/core/src/security/operation-private-keys.ts.',
+            'If you are adding a consumer, add it to that header\'s "Known consumers"',
+            'list rather than re-deriving the argument locally.',
+          ].join('\n'),
+    ).toEqual([]);
+  });
+
+  it('the home really does declare both symbols — the scan cannot pass vacuously', () => {
+    // #4690: a check that finds nothing because it is looking in the wrong place
+    // reads exactly like a check that found no violations. Anchor it.
+    const text = readFileSync(HOME, 'utf8');
+    const found = [...text.matchAll(DECLARATION)].map((m) => m[1]).sort();
+
+    expect(found).toEqual(['OPERATION_PRIVATE_KEY_PREFIX', 'withoutOperationPrivateKeys']);
+  });
+
+  it('the scan reaches the packages that used to hold the copies', () => {
+    // The second half of the same anti-vacuity guard: prove the walker actually
+    // descends into the three consumer packages, so a future refactor of
+    // SKIP_DIRS or the walk cannot silently narrow the scan to `packages/core`.
+    const scanned = sourceFiles(PACKAGES).map((f) => relative(REPO_ROOT, f));
+
+    for (const consumer of [
+      'packages/plugins/plugin-audit/src/comment-access-hooks.ts',
+      'packages/services/service-storage/src/attachment-access-hooks.ts',
+      'packages/plugins/plugin-reports/src/report-service.ts',
+    ]) {
+      expect(scanned).toContain(consumer);
+    }
+  });
+});

packages/core/src/security/operation-private-keys.test.ts

@@ -0,0 +1,112 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#7284] Behaviour of the `__` operation-private-key convention at its home.
+ *
+ * The three packages that hand-copied this helper each covered it only through
+ * their own gates — `sys_comment`'s access hooks, `sys_attachment`'s, the report
+ * runner's — so the RULE itself was never asserted anywhere, only its effect on
+ * three particular call sites. These are the assertions that belong to the rule.
+ */
+
+import { describe, it, expect } from 'vitest';
+
+import {
+  OPERATION_PRIVATE_KEY_PREFIX,
+  withoutOperationPrivateKeys,
+} from './operation-private-keys.js';
+
+describe('withoutOperationPrivateKeys', () => {
+  it('drops every key carrying the operation-private prefix', () => {
+    const out = withoutOperationPrivateKeys({
+      userId: 'u1',
+      tenantId: 't1',
+      __readScope: 'org',
+      __writeScope: 'org',
+      __delegatorReadScope: 'unit',
+      __delegatorWriteScope: 'unit',
+      __expandRead: true,
+      __referentialFieldClear: true,
+    });
+
+    expect(out).toEqual({ userId: 'u1', tenantId: 't1' });
+  });
+
+  it('preserves every principal field — it strips, it does not project', () => {
+    // The defect half of the five-field projections this helper replaced
+    // (#7141 / #7145 / #7204): these decide the verdict the gate then trusts.
+    const envelope = {
+      userId: 'u1',
+      tenantId: 't1',
+      positions: ['p1'],
+      permissions: ['read'],
+      isSystem: false,
+      onBehalfOf: { userId: 'agent-owner' },
+      principalKind: 'agent',
+      systemPermissions: ['x'],
+      accessible_org_ids: ['o1', 'o2'],
+      org_user_ids: ['u1'],
+      posture: 'group',
+      audience: 'api',
+      rlsMembership: { unit: 'u' },
+      timezone: 'Asia/Shanghai',
+      __readScope: 'org',
+    };
+
+    const out = withoutOperationPrivateKeys(envelope) as Record<string, unknown>;
+
+    const { __readScope: _dropped, ...everythingElse } = envelope;
+    expect(out).toEqual(everythingElse);
+  });
+
+  it('returns a FRESH object even when there is nothing to strip', () => {
+    // ⛔ The copy is the point, not an optimisation to skip on a clean envelope:
+    // a callee that stamps its own `__writeScope` onto what it receives must not
+    // be able to write back into the caller's operation context.
+    const envelope = { userId: 'u1' };
+    const out = withoutOperationPrivateKeys(envelope) as Record<string, unknown>;
+
+    expect(out).not.toBe(envelope);
+    expect(out).toEqual(envelope);
+
+    out.__writeScope = 'org';
+    expect(envelope).toEqual({ userId: 'u1' });
+  });
+
+  it('is a SHALLOW copy — nested values are forwarded by reference', () => {
+    // Stated so the boundary is a decision rather than an accident: the hazard
+    // this closes is a callee stamping TOP-LEVEL keys, which is all the
+    // middleware ever does.
+    const nested = { userId: 'agent-owner' };
+    const out = withoutOperationPrivateKeys({ onBehalfOf: nested }) as Record<string, unknown>;
+
+    expect(out.onBehalfOf).toBe(nested);
+  });
+
+  it('drops a key the middleware has not stamped yet, by prefix alone', () => {
+    // The whole reason the rule is a prefix and not a name list: a seventh
+    // operation-private key must be dropped by every consumer on the day it is
+    // stamped, with no consumer edited.
+    const out = withoutOperationPrivateKeys({ userId: 'u1', __someFutureMarker: true });
+
+    expect(out).toEqual({ userId: 'u1' });
+  });
+
+  it('leaves keys that merely CONTAIN the prefix, and single-underscore keys', () => {
+    const out = withoutOperationPrivateKeys({
+      _private: 1,
+      'field__with__dunders': 2,
+      org_user_ids: ['o1'],
+    });
+
+    expect(out).toEqual({ _private: 1, 'field__with__dunders': 2, org_user_ids: ['o1'] });
+  });
+
+  it('tolerates an empty envelope', () => {
+    expect(withoutOperationPrivateKeys({})).toEqual({});
+  });
+
+  it('pins the prefix itself — consumers and the middleware agree on `__`', () => {
+    expect(OPERATION_PRIVATE_KEY_PREFIX).toBe('__');
+  });
+});