Skip to content

Commit ceaa38d

Browse files
committed
fix(webapp): scope the watch transcript's appends to the caller's organization
appendChatMessageOnce already verifies organizationId, but left it optional because these call sites never threaded one — so the wake and the consented investigation wrote durable user-facing messages with the organization check skipped. Both paths now pass it on every append, the retry repair included, and the chat id and the organization have to agree for a row to land.
1 parent f159c69 commit ceaa38d

2 files changed

Lines changed: 113 additions & 14 deletions

File tree

internal-packages/dashboard-agent/src/watch-actions.test.ts

Lines changed: 93 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,19 +27,25 @@ import {
2727
} from "./test-support";
2828

2929
/**
30-
* Stands in for `chat_messages`: the append is keyed on (chat_id, message_id) and does
31-
* nothing on conflict, so a repeated append is never a second row. Row counts are what
32-
* the History panel reads, so the retry tests assert those rather than call counts.
30+
* Stands in for `chats` + `chat_messages`, as `appendOneMessage`'s upsert sees them: the
31+
* insert is scoped to the owning user and — when the caller passes one — the owning
32+
* organization, and keyed on (chat_id, message_id) with nothing done on conflict. So a
33+
* repeat is never a second row and a foreign tenancy is no row at all. Row counts are
34+
* what the History panel reads, which is why these tests assert those, not call counts.
3335
*/
34-
function transcriptTable() {
36+
function transcriptTable(owner: { userId: string; organizationId: string }) {
3537
const rows: { chatId: string; messageId: string }[] = [];
3638
const countOf = (chatId: string, messageId: string) =>
3739
rows.filter((row) => row.chatId === chatId && row.messageId === messageId).length;
3840
return {
3941
countOf,
40-
insert(chatId: string, message: UIMessage) {
41-
if (countOf(chatId, message.id) > 0) return false;
42-
rows.push({ chatId, messageId: message.id });
42+
insert(args: { chatId: string; userId: string; organizationId?: string; message: UIMessage }) {
43+
if (args.userId !== owner.userId) return false;
44+
if (args.organizationId !== undefined && args.organizationId !== owner.organizationId) {
45+
return false;
46+
}
47+
if (countOf(args.chatId, args.message.id) > 0) return false;
48+
rows.push({ chatId: args.chatId, messageId: args.message.id });
4349
return true;
4450
},
4551
};
@@ -58,12 +64,19 @@ function appendingStore(
5864
await store.appendMessage(args);
5965
const message = args.message as UIMessage;
6066
if (failWhen(message)) throw new Error("the append lost the connection");
61-
return table.insert(args.chatId, message);
67+
return table.insert({ ...args, message });
6268
},
6369
};
6470
return { store: wrapped, calls };
6571
}
6672

73+
// Every organization a path's appends were scoped to, in order.
74+
function scopedTo(...stores: { calls: { appendMessage: unknown[] } }[]) {
75+
return stores.flatMap((store) =>
76+
store.calls.appendMessage.map((call) => (call as { organizationId?: string }).organizationId)
77+
);
78+
}
79+
6780
describe("watch wake narration", () => {
6881
let harness: MockChatAgentHarness | undefined;
6982

@@ -427,7 +440,7 @@ describe("watch wake narration", () => {
427440
* its history. Converging on the row is the retry's job.
428441
*/
429442
it("appends the display copy on a retry that finds the wake already narrated", async () => {
430-
const table = transcriptTable();
443+
const table = transcriptTable(CLIENT_DATA);
431444
const chatId = "chat_wake_retry";
432445
const wakeId = "wake:watch:watch_1:fired";
433446

@@ -474,6 +487,39 @@ describe("watch wake narration", () => {
474487
// A third delivery repairs nothing, because there is nothing left to repair.
475488
await harness.sendAction(WAKE);
476489
expect(table.countOf(chatId, wakeId)).toBe(1);
490+
491+
// Every write on this path is scoped to the organization the append verifies — the
492+
// repair included, or the repair would be the one write that skips the check.
493+
expect(scopedTo(failing, repairing)).toEqual([
494+
CLIENT_DATA.organizationId,
495+
CLIENT_DATA.organizationId,
496+
CLIENT_DATA.organizationId,
497+
]);
498+
});
499+
500+
/**
501+
* The chat id comes from the watch record and the tenancy from the session's
502+
* `clientData`. If those ever disagree the append has to write nothing, rather than put
503+
* a message in another organization's transcript.
504+
*/
505+
it("writes nothing when the wake's tenancy doesn't own the chat", async () => {
506+
const table = transcriptTable(CLIENT_DATA);
507+
const chatId = "chat_wake_other_org";
508+
const { store, calls } = appendingStore(table, () => false);
509+
harness = mockChatAgent(dashboardAgent, {
510+
chatId,
511+
clientData: { ...CLIENT_DATA, organizationId: "org_other" },
512+
setupLocals: ({ set }) => {
513+
set(dashboardAgentStoreKey, store);
514+
set(dashboardAgentModelKey, mockModel([textStep("never asked for")]));
515+
},
516+
});
517+
518+
await harness.sendAction(WAKE);
519+
520+
// Attempted, and refused by the scope the append carries.
521+
expect(calls.appendMessage).toHaveLength(1);
522+
expect(table.countOf(chatId, "wake:watch:watch_1:fired")).toBe(0);
477523
});
478524
});
479525

@@ -899,7 +945,7 @@ describe("watch investigation", () => {
899945
* History panel lost. The retry finds it already answered and must still land the row.
900946
*/
901947
it("appends the display copy on a retry that finds the investigation already answered", async () => {
902-
const table = transcriptTable();
948+
const table = transcriptTable(CLIENT_DATA);
903949
const chatId = "chat_investigate_retry";
904950
const findingsId = "investigate:watch:watch_1:fired:investigate";
905951
const seeded = { id: "inv_seeded", projectRef: "proj_abc", environmentRef: "env_abc" };
@@ -952,5 +998,42 @@ describe("watch investigation", () => {
952998
// A third delivery repairs nothing, because there is nothing left to repair.
953999
await harness.sendAction(INVESTIGATE);
9541000
expect(table.countOf(chatId, findingsId)).toBe(1);
1001+
1002+
// Every write on this path is scoped to the organization the append verifies — the
1003+
// repair included, or the repair would be the one write that skips the check.
1004+
expect(scopedTo(failing, repairing)).toEqual([
1005+
CLIENT_DATA.organizationId,
1006+
CLIENT_DATA.organizationId,
1007+
CLIENT_DATA.organizationId,
1008+
]);
1009+
});
1010+
1011+
// Same tenancy crossing as the wake's: the kick names the chat, the session names the
1012+
// organization, and a disagreement must not write into another organization's chat.
1013+
it("writes nothing when the kick's tenancy doesn't own the chat", async () => {
1014+
const table = transcriptTable(CLIENT_DATA);
1015+
const chatId = "chat_investigate_other_org";
1016+
const { store, calls } = appendingStore(table, () => false, {
1017+
openInvestigation: { id: "inv_seeded", projectRef: "proj_abc", environmentRef: "env_abc" },
1018+
});
1019+
harness = mockChatAgent(dashboardAgent, {
1020+
chatId,
1021+
clientData: { ...CLIENT_DATA_WITH_TOKEN, organizationId: "org_other" },
1022+
setupLocals: ({ set }) => {
1023+
set(dashboardAgentStoreKey, store);
1024+
set(
1025+
dashboardAgentModelKey,
1026+
recordingModel([
1027+
renderStep(concluded, "inv_seeded", "tc_verdict"),
1028+
textStep("The payload lost order.total."),
1029+
]).model
1030+
);
1031+
},
1032+
});
1033+
1034+
await harness.sendAction(INVESTIGATE);
1035+
1036+
expect(calls.appendMessage).toHaveLength(1);
1037+
expect(table.countOf(chatId, "investigate:watch:watch_1:fired:investigate")).toBe(0);
9551038
});
9561039
});

internal-packages/dashboard-agent/src/watch-actions.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,10 @@ async function narrateWatchWake(args: {
490490
// wake narrated and the display copy still owed. The append is id-deduped, so
491491
// repairing when nothing is broken writes nothing.
492492
const userId = args.clientData?.userId;
493-
if (userId) await getStore().appendMessage({ chatId, userId, message: narrated });
493+
const organizationId = args.clientData?.organizationId;
494+
if (userId) {
495+
await getStore().appendMessage({ chatId, userId, organizationId, message: narrated });
496+
}
494497
return;
495498
}
496499

@@ -531,8 +534,9 @@ async function narrateWatchWake(args: {
531534
// host-appended blocks (a card-born chat starts with only those) and
532535
// `persistMessages` would drop them.
533536
const userId = args.clientData?.userId;
537+
const organizationId = args.clientData?.organizationId;
534538
if (userId) {
535-
await getStore().appendMessage({ chatId, userId, message });
539+
await getStore().appendMessage({ chatId, userId, organizationId, message });
536540
} else {
537541
// A wake always carries its watch's tenancy, so reaching this means the
538542
// metadata contract broke. Deliver anyway: losing blocks beats losing the wake.
@@ -734,7 +738,14 @@ async function conductWatchInvestigation(args: {
734738
// Same window as the wake's: the findings streamed durably before the append, so a
735739
// retry can owe only the display copy. Id-deduped, so a repeat writes nothing.
736740
const userId = clientData?.userId;
737-
if (userId) await getStore().appendMessage({ chatId, userId, message: alreadyAnswered });
741+
if (userId) {
742+
await getStore().appendMessage({
743+
chatId,
744+
userId,
745+
organizationId: clientData?.organizationId,
746+
message: alreadyAnswered,
747+
});
748+
}
738749
const open = [...latestCards(uiMessages).values()].find(
739750
(card) => card.state === null || card.state.outcome === "in_progress"
740751
);
@@ -830,7 +841,12 @@ async function conductWatchInvestigation(args: {
830841
answered = message;
831842
const userId = clientData?.userId;
832843
if (userId) {
833-
await store.appendMessage({ chatId, userId, message });
844+
await store.appendMessage({
845+
chatId,
846+
userId,
847+
organizationId: clientData?.organizationId,
848+
message,
849+
});
834850
} else {
835851
logger.error("dashboard-agent watch investigation has no userId; skipping the append", {
836852
chatId,

0 commit comments

Comments
 (0)