diff --git a/packages/api/src/domains/approval-hub/ApprovalIngress.ts b/packages/api/src/domains/approval-hub/ApprovalIngress.ts index 597b661495..f7ce66ebce 100644 --- a/packages/api/src/domains/approval-hub/ApprovalIngress.ts +++ b/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( diff --git a/packages/api/test/approval-hub/approval-ingress.test.js b/packages/api/test/approval-hub/approval-ingress.test.js index bb1c4f97a6..f2d07a5875 100644 --- a/packages/api/test/approval-hub/approval-ingress.test.js +++ b/packages/api/test/approval-hub/approval-ingress.test.js @@ -41,7 +41,7 @@ class FakePublicationStore { } } -function appendOrigin(messageStore) { +function appendOrigin(messageStore, authorOverrides = {}) { messageStore.append({ userId: ownerUserId, catId: null, @@ -49,6 +49,7 @@ function appendOrigin(messageStore) { 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(); diff --git a/packages/api/test/approval-hub/scheduler-origin-callback-integration.test.js b/packages/api/test/approval-hub/scheduler-origin-callback-integration.test.js new file mode 100644 index 0000000000..be4af70aa7 --- /dev/null +++ b/packages/api/test/approval-hub/scheduler-origin-callback-integration.test.js @@ -0,0 +1,370 @@ +/** + * F167 — the system-origin exemption, exercised through real callback routes. + * + * Why this file exists (@codex-luna PR #1349 P2, and the clowder-ai maintainer on #1347): + * approval-ingress.test.js proves the exemption against FABRICATED drafts. A fabricated + * draft cannot show that a producer's real adapter ever puts the scheduler's wake row into + * originRef — and that gap already produced a wrong test once: the original negative case + * asserted rejection for F128, a producer that DOES bind, so it pinned a regression of the + * very 500 this branch removes, and its green read as confirmation. + * + * So nothing here is constructed at the ingress boundary. Each case drives the real HTTP + * route, authenticated by a real InvocationRegistry record, whose origin trigger is a real + * scheduler wake row appended the way infrastructure/scheduler/delivery.ts appends it. + * + * The five `server_attested` producers must publish. F221 — deliberately `forbidden` — must + * still be rejected on the identical path, which is what keeps the exemption a boundary + * rather than a blanket. + */ + +import '../helpers/setup-cat-registry.js'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, it } from 'node:test'; +import Fastify from 'fastify'; +import { proposedReviewAction } from './helpers.js'; + +const OWNER = 'user-1'; +const CAT = 'opus'; + +const load = (path) => import(`../../dist/${path}.js`); +const socket = () => ({ emitToUser() {}, broadcastToRoom() {}, broadcastAgentMessage() {} }); + +/** + * The exact row infrastructure/scheduler/delivery.ts persists for a 持球唤醒 wake: + * authored by the `scheduler` pseudo-user with a null catId, never by the session owner. + */ +async function appendSchedulerWakeRow(messageStore, threadId) { + const row = await messageStore.append({ + userId: 'scheduler', + catId: null, + content: '[hold-ball] the condition you were waiting on expired', + mentions: [], + origin: 'callback', + timestamp: Date.now(), + threadId, + source: { connector: 'scheduler', label: '定时任务', icon: 'scheduler' }, + extra: { scheduler: { hiddenTrigger: true } }, + }); + // Fixture self-check. If this ever drifts back to an owner-authored row, every positive + // case below keeps passing with the exemption deleted — the file would assert nothing. + assert.equal(row.userId, 'scheduler', 'the wake row must be scheduler-authored'); + assert.equal(row.catId, null, 'a cat-authored row wearing a system userId must not be the fixture'); + assert.notEqual(row.userId, OWNER, 'owner-authored origins never needed the exemption'); + return row; +} + +/** + * invoke-single-cat.ts passes the turn's trigger id as arg 7 (originTriggerMessageId) and + * leaves args 4-6 undefined on the scheduler path. Arg 5 is a2aTriggerMessageId: putting the + * wake id there also compiles, yields an A2A-shaped record, and would test nothing. + * + * The record's userId is the OWNER. Only the message row is the scheduler's — that + * asymmetry between record and row is the entire bug. + */ +const schedulerWokenAuth = (registry, threadId, wakeMessageId) => + registry.create(OWNER, CAT, threadId, undefined, undefined, undefined, wakeMessageId); + +const post = (app, url, auth, payload) => + app.inject({ + method: 'POST', + url, + headers: { 'x-invocation-id': auth.invocationId, 'x-callback-token': auth.callbackToken }, + payload, + }); + +async function assertAnchoredToWakeRow(store, proposalId, threadId, wakeRow) { + const publication = await store.getPublication(proposalId); + assert.equal(publication?.state, 'anchored', 'the proposal must reach an anchored publication'); + assert.deepEqual( + publication.envelope.originRef, + { kind: 'message', threadId, messageId: wakeRow.id }, + 'the published origin must be the scheduler wake row itself, not a substitute', + ); +} + +describe('F167 scheduler-origin exemption over real callback routes', () => { + it('F225 — a timer-woken session can hand off (the exact failure this branch removes)', async () => { + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { InMemorySessionHandoffProposalStore } = await load( + 'domains/cats/services/stores/ports/SessionHandoffProposalStore', + ); + const { callbacksRoutes } = await load('routes/index'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const store = new InMemorySessionHandoffProposalStore(); + const session = { id: 'sess_active', status: 'active', catId: CAT, threadId: 'thread_1', userId: OWNER }; + + const app = Fastify(); + await app.register(callbacksRoutes, { + registry, + messageStore, + socketManager: socket(), + handoffProposalStore: store, + sessionChainStore: { + getActive: async (catId, threadId) => + catId === session.catId && threadId === session.threadId ? session : null, + }, + evidenceStore: { ingestRaw() {}, search: () => [] }, + markerQueue: { enqueue() {} }, + reflectionService: { reflect() {} }, + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, 'thread_1'); + const auth = await schedulerWokenAuth(registry, 'thread_1', wake.id); + + const res = await post(app, '/api/callbacks/propose-session-handoff', auth, { + done: 'ran long enough to be driven by the timer', + nextSteps: 'hand the rest to the next session', + }); + + assert.equal(res.statusCode, 200, res.body); + assert.equal(res.json().status, 'pending'); + await assertAnchoredToWakeRow(store, res.json().proposalId, 'thread_1', wake); + } finally { + await app.close(); + } + }); + + it('F128 — a scheduler-woken turn can propose a thread', async () => { + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { ThreadStore } = await load('domains/cats/services/stores/ports/ThreadStore'); + const { InMemoryProposalStore } = await load('domains/cats/services/stores/ports/ProposalStore'); + const { callbacksRoutes } = await load('routes/index'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const threadStore = new ThreadStore(); + const store = new InMemoryProposalStore(); + const source = await threadStore.create(OWNER, 'Source'); + + const app = Fastify(); + await app.register(callbacksRoutes, { + registry, + messageStore, + socketManager: socket(), + threadStore, + proposalStore: store, + evidenceStore: { ingestRaw() {}, search: () => [] }, + markerQueue: { enqueue() {} }, + reflectionService: { reflect() {} }, + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, source.id); + const auth = await schedulerWokenAuth(registry, source.id, wake.id); + + const res = await post(app, '/api/callbacks/propose-thread', auth, { + title: 'Split the timer-driven work out', + reason: 'The wake turn found a second track worth its own thread', + }); + + assert.equal(res.statusCode, 200, res.body); + await assertAnchoredToWakeRow(store, res.json().proposalId, source.id, wake); + } finally { + await app.close(); + } + }); + + it('F193 — a scheduler-woken turn can propose a cross-thread dispatch', async () => { + const { ApprovalIngress } = await load('domains/approval-hub/ApprovalIngress'); + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { ThreadStore } = await load('domains/cats/services/stores/ports/ThreadStore'); + const { InMemoryDispatchProposalStore } = await load('domains/approval-hub/stores/ports/IDispatchProposalStore'); + const { callbacksRoutes } = await load('routes/callbacks'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const threadStore = new ThreadStore(); + const store = new InMemoryDispatchProposalStore(); + const source = await threadStore.create(OWNER, 'Source'); + const target = await threadStore.create(OWNER, 'Target'); + await threadStore.addParticipants(source.id, [CAT]); + await threadStore.addParticipants(target.id, ['sonnet']); + + const app = Fastify(); + await app.register(callbacksRoutes, { + registry, + messageStore, + threadStore, + socketManager: socket(), + router: { async *routeExecution() {}, getExecutions: () => [] }, + invocationRecordStore: { create: () => ({ outcome: 'created' }), update() {}, get: () => null }, + dispatchProposalStore: store, + approvalIngress: new ApprovalIngress({ messageStore, socketManager: socket() }), + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, source.id); + const auth = await schedulerWokenAuth(registry, source.id, wake.id); + + const res = await post(app, '/api/callbacks/post-message', auth, { + threadId: target.id, + content: '@sonnet\nThe timer woke me and this needs another pair of paws', + targetCats: ['sonnet'], + effectClass: 'assign_work', + proposedAction: proposedReviewAction(), + clientMessageId: 'f167-scheduler-origin-dispatch', + }); + + assert.equal(res.statusCode, 200, res.body); + const [proposal] = await store.listPendingByUser(OWNER); + assert.ok(proposal, 'the dispatch intercept must have created a proposal'); + await assertAnchoredToWakeRow(store, proposal.proposalId, source.id, wake); + } finally { + await app.close(); + } + }); + + it('F231 — a scheduler-woken turn can propose a profile update', async () => { + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { InMemoryProfileUpdateProposalStore } = await load( + 'domains/cats/services/stores/ports/ProfileUpdateProposalStore', + ); + const { FileProfileRepository } = await load('domains/cats/services/profile/ProfileRepository'); + const { registerCallbackAuthHook } = await load('routes/callback-auth-prehandler'); + const { registerCallbackProposeProfileUpdateRoutes } = await load('routes/callback-propose-profile-update-routes'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const store = new InMemoryProfileUpdateProposalStore(); + const dataDir = mkdtempSync(join(tmpdir(), 'f167-scheduler-origin-')); + + const app = Fastify(); + registerCallbackAuthHook(app, registry); + registerCallbackProposeProfileUpdateRoutes(app, { + registry, + proposalStore: store, + messageStore, + socketManager: socket(), + repository: new FileProfileRepository({ dataDir, relationshipKeyForCat: () => 'ragdoll' }), + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, 'thread_1'); + const auth = await schedulerWokenAuth(registry, 'thread_1', wake.id); + + const res = await post(app, '/api/callbacks/propose-profile-update', auth, { + afterContent: 'NEW primer written during a timer-driven turn', + rationale: 'Observed while the scheduler had the ball', + signalKind: 'cat-declared', + }); + + assert.equal(res.statusCode, 200, res.body); + const [proposal] = await store.listPending(OWNER); + assert.ok(proposal, 'the profile-update route must have created a proposal'); + await assertAnchoredToWakeRow(store, proposal.proposalId, 'thread_1', wake); + } finally { + await app.close(); + rmSync(dataDir, { recursive: true, force: true }); + } + }); + + it('F260 — a scheduler-woken turn can propose an entity', async () => { + const { ApprovalIngress } = await load('domains/approval-hub/ApprovalIngress'); + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { InMemoryEntityProposalStore } = await load('domains/approval-hub/stores/ports/IEntityProposalStore'); + const { callbackProposeEntityRoutes } = await load('routes/callback-propose-entity-routes'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const store = new InMemoryEntityProposalStore(); + + const app = Fastify(); + await app.register(callbackProposeEntityRoutes, { + registry, + entityProposalStore: store, + messageStore, + socketManager: socket(), + approvalIngress: new ApprovalIngress({ messageStore, socketManager: socket() }), + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, 'thread_1'); + const auth = await schedulerWokenAuth(registry, 'thread_1', wake.id); + + const res = await post(app, '/api/callbacks/propose-entity', auth, { + entityId: 'concept:hold-ball-wake', + entityType: 'concept', + canonicalName: 'Hold-ball wake', + aliases: ['持球唤醒'], + stance: 'endorsed', + visibilityScope: 'workspace', + provenance: [{ source: 'f167-scheduler-origin-integration' }], + rationale: 'Named during a timer-driven turn', + clientRequestId: 'f167-scheduler-origin-entity', + }); + + assert.equal(res.statusCode, 200, res.body); + await assertAnchoredToWakeRow(store, res.json().proposalId, 'thread_1', wake); + } finally { + await app.close(); + } + }); + + it('F221 — a producer whose adapter permits a body-chosen origin gets no exemption on the same path', async () => { + // Not "this request's origin is unbound" — here it comes off the record exactly as F225's + // does. The catalog gate is per PRODUCER, not per request: F221's deriveTasteOriginRef + // prefers a body-supplied sourceMessageId with no equality check, so no request of its + // shape can be attested. This is the negative the earlier fabricated-F128 case should + // have been, and it is the case that dies if anyone widens the exemption back to global. + const { ApprovalIngress } = await load('domains/approval-hub/ApprovalIngress'); + const { InvocationRegistry } = await load('domains/cats/services/agents/invocation/InvocationRegistry'); + const { MessageStore } = await load('domains/cats/services/stores/ports/MessageStore'); + const { InMemoryTasteProposalStore } = await load('domains/taste/stores/InMemoryTasteProposalStore'); + const { callbackProposeTasteRoutes } = await load('routes/callback-propose-taste-routes'); + + const registry = new InvocationRegistry(); + const messageStore = new MessageStore(); + const store = new InMemoryTasteProposalStore(); + + const app = Fastify(); + await app.register(callbackProposeTasteRoutes, { + registry, + tasteProposalStore: store, + socketManager: socket(), + approvalIngress: new ApprovalIngress({ messageStore, socketManager: socket() }), + }); + await app.ready(); + + try { + const wake = await appendSchedulerWakeRow(messageStore, 'thread_1'); + const auth = await schedulerWokenAuth(registry, 'thread_1', wake.id); + + const res = await post(app, '/api/callbacks/propose-taste', auth, { + scene: 'A timer-driven turn tried to record a taste', + quote: 'The exemption must not follow the origin across producers', + tags: ['cognitive-honesty'], + dimension: 'cognitive-honesty', + privacy: 'public', + clientRequestId: 'f167-scheduler-origin-taste', + }); + + assert.ok(res.statusCode >= 500, `expected the ingress to reject, got ${res.statusCode}`); + assert.match( + res.body, + /owner mismatch/i, + 'must fail on the owner check specifically — any other 500 would pass this case for the wrong reason', + ); + for (const proposal of await store.listPending(OWNER)) { + assert.notEqual((await store.getPublication(proposal.id))?.state, 'anchored'); + } + } finally { + await app.close(); + } + }); +}); diff --git a/packages/api/test/propose-session-handoff-route.test.js b/packages/api/test/propose-session-handoff-route.test.js index 37c770da63..a59bffd34f 100644 --- a/packages/api/test/propose-session-handoff-route.test.js +++ b/packages/api/test/propose-session-handoff-route.test.js @@ -281,4 +281,133 @@ describe('propose-session-handoff route (F225 ②a)', () => { const active = await ctx.handoffStore.listActiveBySession('sess_active'); assert.equal(active.length, 0, 'phantom proposal deleted after card-append failure'); }); + + // ── F167: the scheduler-origin path the catalog gate exists for ────────────────────── + // + // approval-ingress.test.js proves validateOrigin DECIDES correctly given a producerId + // and an origin row, but every case there synthesises the draft directly. None of them + // reaches the premise the design rests on: that THIS route derives threadId, messageId + // and ownerUserId from the authenticated InvocationRecord, which is the only reason a + // system pseudo-user row is safe to accept as an origin at all. + // + // `systemOriginExemption: 'server_attested'` is a hand-written DECLARATION. `required` + // stops an omission; it cannot stop a wrong one. So these cases test the BINDING. + // (maintainer gate 2 + @codex-luna P2 on PR #1349; tracked as #1350) + // + // Companion file: approval-hub/scheduler-origin-callback-integration.test.js walks the + // same wake row ACROSS producers (F128/F193/F231/F260 accept, F221 forbidden rejects). + // This block goes deep on the F225 route instead — the body-override case below is the + // one assertion neither that matrix nor the ingress unit suite makes. Neither file is + // redundant; delete either and a distinct failure mode stops being covered. + describe('scheduler-woken origin (F167)', () => { + async function proposeWithOrigin(ctx, { author, payload, threadId = 'thread_1' } = {}) { + const origin = await ctx.messageStore.append({ + ...author, + content: '⏰ 定时唤醒：持球检查 CI', + mentions: [], + timestamp: Date.now(), + threadId, + }); + // 7th arg is originTriggerMessageId: the EXACT turn origin, record-side only. + const auth = await ctx.registry.create('user_1', 'opus', 'thread_1', undefined, undefined, undefined, origin.id); + const response = await ctx.app.inject({ + method: 'POST', + url: '/api/callbacks/propose-session-handoff', + headers: { 'x-invocation-id': auth.invocationId, 'x-callback-token': auth.callbackToken }, + payload: payload ?? { done: 'held the ball through CI', nextSteps: 'read the verdict' }, + }); + return { origin, response }; + } + + // The reported defect (#1348): for any long-running session the message that triggered + // the invocation IS the scheduler wake row, so this was the one handoff that could never + // be proposed — 500 with no actionable text, while the same call from an A2A turn worked. + it('accepts a handoff anchored to the scheduler wake row', async () => { + const ctx = await buildCtx(); + const { origin, response } = await proposeWithOrigin(ctx, { author: { userId: 'scheduler', catId: null } }); + + assert.equal(response.statusCode, 200, 'scheduler-woken handoff is no longer rejected as a cross-tenant origin'); + const stored = await ctx.handoffStore.get(response.json().proposalId); + assert.equal(stored.status, 'pending'); + assert.deepEqual( + stored.publication.envelope.originRef, + { kind: 'message', threadId: 'thread_1', messageId: origin.id }, + 'origin stays anchored to the exact scheduler wake row', + ); + assert.equal(stored.publication.envelope.ownerUserId, 'user_1', 'owner is the record user, not the pseudo-user'); + }); + + it('also accepts the historical catId:"system" wake shape', async () => { + const ctx = await buildCtx(); + const { response } = await proposeWithOrigin(ctx, { author: { userId: 'system', catId: 'system' } }); + assert.equal(response.statusCode, 200, 'both persisted system shapes are accepted (visibility.ts predicate)'); + }); + + // The assertion that actually distinguishes a bound route from an unbound one. A status + // code cannot: a route that ignores these fields and a route that validates them both + // answer 200. Only the persisted originRef separates them. + it('the origin is record-derived — a body naming another thread/message/owner cannot move it', async () => { + const ctx = await buildCtx(); + const foreign = await ctx.messageStore.append({ + userId: 'user_2', + catId: null, + content: 'another tenant thread', + mentions: [], + timestamp: Date.now(), + threadId: 'thread_foreign', + }); + const { origin, response } = await proposeWithOrigin(ctx, { + author: { userId: 'scheduler', catId: null }, + payload: { + done: 'held the ball through CI', + nextSteps: 'read the verdict', + // None of these are inputs to the origin. If any ever becomes one, this test fails. + threadId: 'thread_foreign', + sourceThreadId: 'thread_foreign', + messageId: foreign.id, + sourceMessageId: foreign.id, + userId: 'user_2', + ownerUserId: 'user_2', + }, + }); + + assert.equal(response.statusCode, 200); + const envelope = (await ctx.handoffStore.get(response.json().proposalId)).publication.envelope; + assert.deepEqual( + envelope.originRef, + { kind: 'message', threadId: 'thread_1', messageId: origin.id }, + 'body-supplied thread/message are ignored; originRef is derived from the InvocationRecord', + ); + assert.equal(envelope.ownerUserId, 'user_1', 'body-supplied owner cannot reassign the proposal'); + }); + + // The exemption widens WHO may author a row inside an already-bound thread. It must not + // widen it to another human: cross-tenant isolation has to survive end to end, not just + // in the ingress unit case. + it('still rejects a foreign human origin in the same thread', async () => { + const ctx = await buildCtx(); + const { response } = await proposeWithOrigin(ctx, { author: { userId: 'user_2', catId: null } }); + + assert.equal(response.statusCode, 500, 'a non-system foreign author is still a cross-tenant origin'); + assert.match( + response.json().message ?? '', + /Approval origin message owner mismatch/, + 'fails for the tenancy reason, not some unrelated 500', + ); + }); + + // A system userId worn by a cat-authored row must not reach the exemption. Asserting the + // reason matters here too: this input is one typo in the predicate away from passing. + it('still rejects a system userId carried by a cat-authored row', async () => { + const ctx = await buildCtx(); + const { response } = await proposeWithOrigin(ctx, { author: { userId: 'scheduler', catId: 'opus' } }); + + assert.equal( + response.statusCode, + 500, + 'isSystemUserMessage requires BOTH a system userId and a system/null catId', + ); + assert.match(response.json().message ?? '', /Approval origin message owner mismatch/); + }); + }); }); diff --git a/packages/shared/src/approval-producer-catalog.ts b/packages/shared/src/approval-producer-catalog.ts index 09f6756351..75017a13cf 100644 --- a/packages/shared/src/approval-producer-catalog.ts +++ b/packages/shared/src/approval-producer-catalog.ts @@ -16,6 +16,41 @@ export interface ApprovalProducerCatalogEntry { /** Feature-owned approve/reject endpoint base. */ decisionEndpointBase: string; sourcePolicy: 'message-required' | 'message-or-event'; + /** + * Whether this producer may use the system-authored origin exemption in + * `ApprovalIngress.validateOrigin`. + * + * `server_attested` asserts the producer derives `originRef.threadId`, + * `originRef.messageId` and `ownerUserId` from an authenticated + * InvocationRecord that a request body cannot rewrite. Only then is a system + * pseudo-user row inside the already-bound owner thread safe to accept as an + * origin, because the caller — not the request — fixed which thread and owner + * may be named. + * + * `forbidden` is the default for every producer that has not demonstrated that + * binding. The field is REQUIRED so that adding a producer without deciding + * this is a compile error rather than a silent inherited exemption. + * + * WHAT THIS FIELD IS NOT (@codex-luna, PR #1349 P2): it is a policy DECLARATION, + * not a runtime proof that the named adapter actually binds. `required` stops an + * omission; it cannot stop a WRONG declaration. Proof has to come from route-level + * / integration coverage or a typed adapter binding — so treat a `server_attested` + * value as a claim that must have an audit trail next to it, which is why each + * entry below records who walked which creation site. + * + * Current `forbidden` values are deliberate, each for its own reason: + * F139 — publication always constructs an EVENT origin, and validateOrigin + * returns early for `kind === 'event'`, so the exemption is inert here. + * F221 — genuinely unbound: `sourceMessageId` may be supplied by the request + * body and the derive path does NOT tie it back to the InvocationRecord. + * This is the honest negative case for the ingress test. + * F276 — candidate `sourceRef` can come from an owner-validated deferred receipt + * or owner evidence rather than one InvocationRecord; needs scheduler / + * deferred route coverage before it could be attested. + * F292 — MeetingIntake lands through signal admission and never calls + * ApprovalIngress.publish, so this value has no effect on this ingress. + */ + systemOriginExemption: 'server_attested' | 'forbidden'; history: boolean; /** Null keeps the producer on its existing binary reject path. */ humanDispositionReasonCodes: readonly HumanDispositionReasonCode[] | null; @@ -35,6 +70,12 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-info)', decisionEndpointBase: '/api/proposals', sourcePolicy: 'message-required', + // TRANSITIVE binding. callback-propose-thread-routes writes the proposal row + // entirely off the authenticated record — sourceThreadId=record.threadId, + // sourceMessageId=record.originTriggerMessageId ?? record.a2aTriggerMessageId, + // createdBy=record.userId — and then builds originRef from that row. A request + // body cannot rewrite any of the three. Verified at the creation site by opus5. + systemOriginExemption: 'server_attested', history: true, humanDispositionReasonCodes: null, }, @@ -44,6 +85,7 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-warning, #f59e0b)', decisionEndpointBase: '/api/schedule-proposals', sourcePolicy: 'message-or-event', + systemOriginExemption: 'forbidden', history: true, humanDispositionReasonCodes: null, }, @@ -53,6 +95,11 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-success, #22c55e)', decisionEndpointBase: '/api/dispatch-proposals', sourcePolicy: 'message-or-event', + // deriveCallbackOriginRef takes messageId off the authenticated record; threadId + // is either the authenticated actor's (DIRECT) or the persisted proposal's + // sourceThreadId (TRANSITIVE, same shape as F128). Creation-site audit by + // @codex-luna, PR #1349 review — not independently re-walked by opus5. + systemOriginExemption: 'server_attested', history: true, humanDispositionReasonCodes: null, }, @@ -62,6 +109,7 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--accent-taste, #e879f9)', decisionEndpointBase: '/api/taste-proposals', sourcePolicy: 'message-or-event', + systemOriginExemption: 'forbidden', history: true, humanDispositionReasonCodes: null, }, @@ -71,6 +119,10 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-secondary, #8b5cf6)', decisionEndpointBase: '/api/session-handoff', sourcePolicy: 'message-required', + // callback-propose-session-handoff reads originTriggerMessageId / + // a2aTriggerMessageId, threadId and userId off the authenticated + // InvocationRecord; the request body cannot rewrite any of them. + systemOriginExemption: 'server_attested', history: true, humanDispositionReasonCodes: HUMAN_DISPOSITION_REASON_CODES, }, @@ -80,6 +132,11 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-warning, #f59e0b)', decisionEndpointBase: '/api/profile-updates', sourcePolicy: 'message-required', + // DIRECT binding, and the strictest of the set: a body-supplied sourceMessageId + // is rejected unless it equals the record-derived originMessageId + // (callback-propose-profile-update-routes.ts: `sourceMessageId !== originMessageId` + // → reject); omitting it falls back to the record value. Verified by opus5. + systemOriginExemption: 'server_attested', history: true, humanDispositionReasonCodes: null, }, @@ -89,6 +146,11 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--accent-entity, #06b6d4)', decisionEndpointBase: '/api/entity-proposals', sourcePolicy: 'message-or-event', + // deriveEntityOriginRef is the same shape as F193: messageId off the + // authenticated record, threadId from the actor or the persisted proposal row. + // Creation-site audit by @codex-luna, PR #1349 review — not independently + // re-walked by opus5. + systemOriginExemption: 'server_attested', history: true, humanDispositionReasonCodes: null, }, @@ -98,6 +160,7 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--accent-people, #14b8a6)', decisionEndpointBase: '/api/person-memory-proposals', sourcePolicy: 'message-required', + systemOriginExemption: 'forbidden', history: true, humanDispositionReasonCodes: ['not_important', 'wrong_lane', 'bad_evidence', 'wrong', 'other'], }, @@ -107,6 +170,7 @@ export const APPROVAL_PRODUCER_CATALOG = { colorToken: 'var(--semantic-info, #3b82f6)', decisionEndpointBase: '/api/meeting-intakes', sourcePolicy: 'message-or-event', + systemOriginExemption: 'forbidden', history: true, humanDispositionReasonCodes: null, },