fix(F167): scheduler wake row is a valid approval origin only for producers that declare authenticated binding

packages/api/src/domains/approval-hub/ApprovalIngress.ts

@@ -2,6 +2,7 @@ import type { ApprovalEnvelope, ApprovalOriginRef, ApprovalProducerId, CatId, Ri
 import { approvalProducerMeta, validateApprovalEnvelope, validateApprovalOriginRef } from '@cat-cafe/shared';
 import type { SocketManager } from '../../infrastructure/websocket/index.js';
 import type { IMessageStore, StoredMessage } from '../cats/services/stores/ports/MessageStore.js';
+import { isSystemUserMessage } from '../cats/services/stores/visibility.js';
 import { approvalCardIdempotencyKey, buildApprovalCardBlock } from './buildApprovalCardBlock.js';
 import type { ApprovalPublicationStore } from './ports/ApprovalPublicationStore.js';
 
@@ -218,7 +219,60 @@ export class ApprovalIngress {
     const origin = await this.deps.messageStore.getById(draft.originRef.messageId);
     if (!origin || origin.deletedAt || origin._tombstone) throw new Error('Approval origin message not found');
     if (origin.threadId !== draft.originRef.threadId) throw new Error('Approval origin message thread mismatch');
-    if (origin.userId !== draft.ownerUserId) throw new Error('Approval origin message owner mismatch');
+    // Cross-tenant isolation here is a CONJUNCTION of two parts, and neither part
+    // is sufficient on its own (@codex-luna's review, rounds 2-4):
+    //
+    //   (1) CALLER: originRef.threadId / .messageId and ownerUserId must be bound
+    //       to an authenticated record. This class CANNOT verify that binding.
+    //   (2) THIS CLASS: the assertion above checks that the origin message really
+    //       does live in the thread the draft names.
+    //
+    // (2) alone proves nothing about tenancy — a draft naming a thread of its own
+    // choosing satisfies the comparison trivially. It carries weight only because
+    // (1) fixes which thread may be named. On the F225 session-handoff path (1)
+    // holds: that producer derives all three from an authenticated
+    // InvocationRecord, and a request body cannot rewrite them.
+    //
+    // ApprovalIngress serves several producers, so (1) is a property of a PATH and
+    // never of this class. An earlier revision only documented that and let the
+    // exemption apply to every message-origin producer — which made the exemption
+    // as wide as the shared ingress while the argument for it covered one caller
+    // (maintainer review, PR #1347 gate 2).
+    //
+    // So the exemption is no longer global: it is gated per producer by
+    // `systemOriginExemption` in the producer catalog, and a required field means a
+    // new producer cannot inherit it by omission.
+    //
+    // Be precise about what that gate is (@codex-luna, PR #1349 P2). It is a
+    // CAPABILITY GATE over a DECLARATION — the catalog says which producers claim
+    // the binding of (1). It is NOT a runtime proof that the named adapter really
+    // binds: this class cannot verify that, and a wrong `server_attested` value
+    // would not be caught here. The proof lives in the per-entry audit trail plus
+    // route-level coverage. So the gate narrows the blast radius from "every
+    // message-origin producer" to "the ones someone audited and signed for" — which
+    // is a real reduction, not a verification.
+    //
+    // Given (1) AND (2), what the comparison below still decides is narrower: who
+    // may have authored a row inside an already-verified thread — and a system
+    // pseudo-user speaking in that thread is not another tenant.
+    //
+    // Without the exemption this rejected precisely the sessions that need it most.
+    // A session-handoff proposal anchors on the message that triggered the
+    // invocation, and for any long-running session that message IS the scheduler's
+    // wake row ("持球唤醒"), persisted as userId='scheduler' / catId=null. So a
+    // timer-woken cat could never hand off, while the same proposal from an
+    // A2A-triggered turn (authored by the real owner) went through — the failure
+    // was invisible except as a 500 with no actionable text.
+    //
+    // isSystemUserMessage is the store layer's existing predicate for exactly this
+    // distinction, and it is deliberately reused rather than re-derived here: it
+    // requires BOTH a system userId AND a system/null catId, so a cat-authored row
+    // wearing a system userId stays rejected. A second, private definition of "is
+    // this the system" is how the two drift apart.
+    const systemOriginExempt = approvalProducerMeta(draft.producerId).systemOriginExemption === 'server_attested';
+    if (origin.userId !== draft.ownerUserId && !(systemOriginExempt && isSystemUserMessage(origin))) {
+      throw new Error('Approval origin message owner mismatch');
+    }
   }
 
   private async findPersistedCard(

packages/api/test/approval-hub/approval-ingress.test.js

@@ -41,14 +41,15 @@ class FakePublicationStore {
   }
 }
 
-function appendOrigin(messageStore) {
+function appendOrigin(messageStore, authorOverrides = {}) {
   messageStore.append({
     userId: ownerUserId,
     catId: null,
     content: 'please propose this',
     mentions: [],
     timestamp: 1_721_111_110_000,
     threadId: originRef.threadId,
+    ...authorOverrides,
   });
   const stored = messageStore.getByThread(originRef.threadId, 1)[0];
   stored.id = originRef.messageId;
@@ -76,9 +77,9 @@ function makeDraft(overrides = {}) {
   };
 }
 
-function makeHarness() {
+function makeHarness(originAuthorOverrides = {}) {
   const messageStore = new MessageStore();
-  appendOrigin(messageStore);
+  appendOrigin(messageStore, originAuthorOverrides);
   const broadcasts = [];
   const userEvents = [];
   const socketManager = {
@@ -100,6 +101,97 @@ function findApprovalCard(messageStore) {
 }
 
 describe('ApprovalIngress', () => {
+  // F167 — a session-handoff proposal anchors on the message that triggered the
+  // invocation, and for a long-running session that message IS the scheduler's
+  // wake row ("持球唤醒"), persisted under the `scheduler` system pseudo-user with
+  // `catId: null`. A bare `origin.userId !== ownerUserId` check therefore rejected
+  // exactly the sessions most in need of a handoff: every attempt driven by a
+  // timer wake failed with "Approval origin message owner mismatch", while the
+  // same proposal from an A2A-triggered turn (userId = the real owner) succeeded.
+  //
+  // The threadId assertion does NOT by itself pin the origin to the caller's own
+  // thread: it only proves the stored message belongs to the thread the draft
+  // NAMES. Tenancy holds as a conjunction — the producer binds originRef and
+  // ownerUserId to an authenticated InvocationRecord (fixing which thread may be
+  // named), and this ingress then checks the stored origin is consistent with it.
+  //
+  // Given both, what the userId comparison still decides is narrower: WHO may
+  // author a row inside an already-bound thread — and a system pseudo-user
+  // speaking in that thread is not another tenant.
+  //
+  // This comment previously stated the opposite. It survived four review rounds
+  // that corrected the same claim in ApprovalIngress.ts, because every round
+  // fixed the instance being quoted and nobody grepped for the paraphrase living
+  // here (maintainer review, PR #1347).
+  it('accepts a scheduler-authored origin for a server-attested producer', async () => {
+    const harness = makeHarness({ userId: 'scheduler', catId: null });
+    const store = new FakePublicationStore();
+
+    const envelope = await harness.ingress.publish(makeDraft({ producerId: 'F225' }), store);
+
+    assert.equal(store.publication.state, 'anchored');
+    assert.equal(envelope.approvalCardRef.threadId, 'source-thread');
+  });
+
+  // The exemption is scoped to producers that DECLARE the authenticated origin
+  // binding, because ApprovalIngress is shared and the argument only ever covered
+  // one caller. Without this case the scoping and no scoping are indistinguishable.
+  //
+  // F221 is the honest negative: its `sourceMessageId` may be supplied by the
+  // request body and the derive path does not tie it back to the InvocationRecord,
+  // so it really has not established the binding.
+  //
+  // Two review layers landed on this one case. The maintainer (PR #1347) caught that
+  // the exemption was as wide as the shared ingress while the argument covered one
+  // caller. Then @codex-luna (PR #1349) caught that my first negative used F128 —
+  // a producer that DOES bind (record.threadId / record.originTriggerMessageId /
+  // record.userId, verified at the creation site) — so the case was fossilising a
+  // regression of the very 500 this branch exists to fix, and its green read as
+  // proof. A negative case is only evidence if its subject genuinely lacks the
+  // property; picking the wrong subject makes the assertion cosmetic.
+  it('rejects a scheduler-authored origin for a producer without the attestation', async () => {
+    const harness = makeHarness({ userId: 'scheduler', catId: null });
+    const store = new FakePublicationStore();
+
+    await assert.rejects(
+      () => harness.ingress.publish(makeDraft({ producerId: 'F221' }), store),
+      /Approval origin message owner mismatch/,
+    );
+  });
+
+  // Regression guard for the producer Luna's audit reclassified: F128 binds, so a
+  // scheduler-authored origin must be ACCEPTED for it. If someone flips F128 back to
+  // `forbidden`, this fails instead of silently restoring the owner-mismatch 500.
+  it('accepts a scheduler-authored origin for F128, whose binding is transitive but record-derived', async () => {
+    const harness = makeHarness({ userId: 'scheduler', catId: null });
+    const store = new FakePublicationStore();
+
+    const envelope = await harness.ingress.publish(makeDraft({ producerId: 'F128' }), store);
+
+    assert.equal(store.publication.state, 'anchored');
+    assert.equal(envelope.approvalCardRef.threadId, 'source-thread');
+  });
+
+  // The exemption must not become a hole. A different HUMAN owner is a genuine
+  // cross-tenant anchor and stays rejected — without this, the fix above would be
+  // indistinguishable from deleting the check.
+  it('still rejects an origin authored by another human user', async () => {
+    const harness = makeHarness({ userId: 'user-2', catId: null });
+    const store = new FakePublicationStore();
+
+    await assert.rejects(() => harness.ingress.publish(makeDraft(), store), /Approval origin message owner mismatch/);
+  });
+
+  // isSystemUserMessage requires BOTH a system userId AND a system/null catId, so
+  // a cat-authored row wearing a system userId is not a system message. Pinning it
+  // here keeps the exemption tied to that predicate rather than to the userId alone.
+  it('rejects a system userId carried by a cat-authored message', async () => {
+    const harness = makeHarness({ userId: 'scheduler', catId: 'codex-sol' });
+    const store = new FakePublicationStore();
+
+    await assert.rejects(() => harness.ingress.publish(makeDraft(), store), /Approval origin message owner mismatch/);
+  });
+
   it('persists one card, commits its exact envelope, then broadcasts', async () => {
     const harness = makeHarness();
     const store = new FakePublicationStore();