Skip to content

Commit f63c955

Browse files
committed
fix(webapp,agent): watch lifecycle hardening (review F1-F4, F11-F15)
- creation binds to the token's env scope; client mismatch is rejected; the chat snapshot is never consulted (watches + alert endpoints) - the 3-active-watch cap is atomic per chat (advisory lock + concurrent test) - tick generation travels in the payload and is claimed atomically — a retry can't fork the chain; the check endpoint no longer bumps the counter - unknown 401/403 on check retries instead of stranding an active row; a 5-minute sweeper expires overdue watches as unverified_at_expiry - chat deletion cancels its watches in one transaction - unread wake signals require delivered status - the silent dashboard watch-creation action is removed - alert fanout is one job per channel with a stable webhook event id - expired banners with terminal_unsatisfied say the condition can no longer happen instead of 'no answer'
1 parent 85464c2 commit f63c955

23 files changed

Lines changed: 1535 additions & 423 deletions

‎apps/webapp/app/components/dashboard-agent/WakeBanner.tsx‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,21 @@ export type WakeWatch = {
2626
kind: string;
2727
note: string;
2828
identity: string;
29+
/**
30+
* Why the watch ended, from its last result — `terminal_unsatisfied` when the
31+
* condition became impossible (a cancelled run, a deleted queue). Absent when
32+
* the host doesn't carry it, and the banner falls back to the plain expiry.
33+
*/
34+
endedReason?: string | null;
2935
};
3036

37+
/**
38+
* A watch that expired because its condition can never happen now. That IS an
39+
* answer, so it gets its own headline instead of "no answer" — the watcher writes
40+
* this reason when a check comes back `terminal_unsatisfied`.
41+
*/
42+
const TERMINAL_UNSATISFIED = "terminal_unsatisfied";
43+
3144
export type WakeRef = { watchId: string; outcome: WakeOutcome };
3245

3346
/**
@@ -62,8 +75,17 @@ function toneFor(outcome: WakeOutcome, kind: string | undefined): AgentTone {
6275
return "neutral";
6376
}
6477

65-
function headlineFor(tone: AgentTone, outcome: WakeOutcome): string {
66-
if (outcome === "expired") return "Watch expired — no answer";
78+
/** The banner's headline. Exported for the tests; the component owns the tone. */
79+
export function wakeHeadline(
80+
outcome: WakeOutcome,
81+
watch: Pick<WakeWatch, "endedReason"> | undefined,
82+
tone: AgentTone
83+
): string {
84+
if (outcome === "expired") {
85+
return watch?.endedReason === TERMINAL_UNSATISFIED
86+
? "Watch ended — the condition can no longer happen"
87+
: "Watch expired — no answer";
88+
}
6789
switch (tone) {
6890
case "success":
6991
return "Watch update — all clear";
@@ -113,7 +135,7 @@ export function WakeBanner({
113135
>
114136
<Icon className={cn("mt-0.5 size-4 shrink-0", TONE_ICON_COLOR[tone])} />
115137
<div className="min-w-0">
116-
<p className="text-sm font-medium text-text-bright">{headlineFor(tone, outcome)}</p>
138+
<p className="text-sm font-medium text-text-bright">{wakeHeadline(outcome, watch, tone)}</p>
117139
<p className="truncate text-xs text-text-dimmed">{subline(watch)}</p>
118140
</div>
119141
</div>

‎apps/webapp/app/components/dashboard-agent/WatchChips.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ export type WatchChip = {
2525
note: string;
2626
checkEveryMinutes: number;
2727
expiresAt: string;
28+
/** Last check's reason — the wake banner distinguishes terminal_unsatisfied. */
29+
endedReason?: string | null;
2830
};
2931

3032
const STATUS_TONE: Record<WatchStatus, AgentTone> = {
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// The wake banner's headline: what the user reads before the narration. The rule
2+
// pinned here is that an expiry with an ANSWER ("it can't happen any more") never
3+
// reads as the silent "no answer" expiry.
4+
import { describe, expect, it } from "vitest";
5+
import { wakeHeadline } from "./WakeBanner";
6+
7+
describe("wakeHeadline", () => {
8+
it("says the condition can no longer happen when the watch ended terminally", () => {
9+
expect(wakeHeadline("expired", { endedReason: "terminal_unsatisfied" }, "neutral")).toBe(
10+
"Watch ended — the condition can no longer happen"
11+
);
12+
});
13+
14+
it("keeps the plain expiry when the watch simply ran out of time", () => {
15+
for (const watch of [undefined, { endedReason: null }, { endedReason: "not_met_by_expiry" }]) {
16+
expect(wakeHeadline("expired", watch, "neutral")).toBe("Watch expired — no answer");
17+
}
18+
});
19+
20+
it("words a fire by its tone", () => {
21+
expect(wakeHeadline("fired", undefined, "success")).toBe("Watch update — all clear");
22+
expect(wakeHeadline("fired", undefined, "error")).toBe("Watch update — needs your attention");
23+
expect(wakeHeadline("fired", undefined, "neutral")).toBe("Watch update — condition met");
24+
});
25+
});

‎apps/webapp/app/routes/api.v1.dashboard-agent.alerts.$channelId.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ export async function action({ request, params }: ActionFunctionArgs) {
3333
return json({ error: "Not allowed", code: "forbidden_client" }, { status: 403 });
3434
}
3535
const userId = authentication.userActor.userId;
36+
// The turn's environment scope — the authority for the chat's project below.
37+
const environmentId = authentication.userActor.environmentId;
38+
if (!environmentId) {
39+
return json(
40+
{ error: "This chat has no environment context.", code: "invalid_target" },
41+
{ status: 400 }
42+
);
43+
}
3644

3745
const parsedParams = ParamsSchema.safeParse(params);
3846
if (!parsedParams.success) return json({ error: "Invalid params" }, { status: 400 });
@@ -48,9 +56,18 @@ export async function action({ request, params }: ActionFunctionArgs) {
4856
return json({ error: "Invalid request", code: "invalid_request" }, { status: 400 });
4957
}
5058

51-
const context = await resolveAgentAlertContext({ userId, ...body });
59+
const context = await resolveAgentAlertContext({
60+
userId,
61+
environmentId,
62+
chatId: body.chatId,
63+
claimedEnvironmentId: body.environmentId,
64+
claimedProjectRef: body.projectRef,
65+
});
5266
if (!context.ok) {
53-
return json({ error: context.error, code: context.code }, { status: 404 });
67+
return json(
68+
{ error: context.error, code: context.code },
69+
{ status: context.code === "environment_mismatch" ? 400 : 404 }
70+
);
5471
}
5572

5673
const result = await unsubscribeChannelFromWatchAlerts(parsedParams.data.channelId, {

‎apps/webapp/app/routes/api.v1.dashboard-agent.alerts.ts‎

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@ import {
55
ProjectAlertEmailProperties,
66
ProjectAlertSlackProperties,
77
} from "~/models/projectAlert.server";
8-
import { resolveAgentAlertContext } from "~/services/dashboardAgentAlertContext.server";
8+
import {
9+
resolveAgentAlertContext,
10+
type AgentAlertContextError,
11+
} from "~/services/dashboardAgentAlertContext.server";
912
import {
1013
canUseDashboardAgentEmailAlerts,
1114
DASHBOARD_AGENT_WATCH_ALERT_TYPE,
@@ -21,7 +24,8 @@ import { CreateAlertChannelService } from "~/v3/services/alerts/createAlertChann
2124
*
2225
* Accepts ONLY the dashboard agent's delegated user-actor token, like the watches
2326
* endpoint: POST creates something that later mails a person, so the only caller
24-
* it trusts is the agent acting for the signed-in user in a live chat.
27+
* it trusts is the agent acting for the signed-in user in a live chat. The
28+
* environment is the token's, not the body's (see `resolveAgentAlertContext`).
2529
*
2630
* The plan/flag gate is enforced here AND at delivery, and its denial carries a
2731
* machine-readable `reason` so the agent can say why instead of guessing.
@@ -42,18 +46,38 @@ const CreateBodySchema = z.object({
4246
projectRef: z.string().min(1).optional(),
4347
});
4448

45-
async function authenticate(request: Request) {
49+
/**
50+
* The preamble both handlers share: a dashboard-agent token, plus the environment
51+
* scope the dashboard minted it with — which is the authority for everything here,
52+
* so a token without one can't be used at all.
53+
*/
54+
async function authenticate(
55+
request: Request
56+
): Promise<{ userId: string; environmentId: string } | { error: Response }> {
4657
const authentication = await authenticateUatOrApiRequest(request);
47-
if (!authentication?.userActor) return undefined;
48-
if (authentication.userActor.client !== "dashboard-agent") return undefined;
49-
return authentication.userActor.userId;
58+
const actor = authentication?.userActor;
59+
if (!actor || actor.client !== "dashboard-agent") {
60+
return { error: json({ error: "Invalid or missing access token" }, { status: 401 }) };
61+
}
62+
if (!actor.environmentId) {
63+
return {
64+
error: json(
65+
{ error: "This chat has no environment context.", code: "invalid_target" },
66+
{ status: 400 }
67+
),
68+
};
69+
}
70+
return { userId: actor.userId, environmentId: actor.environmentId };
71+
}
72+
73+
/** Context failures: a mismatched claim is the caller's error, the rest are 404s. */
74+
function contextStatus(code: AgentAlertContextError) {
75+
return code === "environment_mismatch" ? 400 : 404;
5076
}
5177

5278
export async function loader({ request }: LoaderFunctionArgs) {
53-
const userId = await authenticate(request);
54-
if (!userId) {
55-
return json({ error: "Invalid or missing access token" }, { status: 401 });
56-
}
79+
const auth = await authenticate(request);
80+
if ("error" in auth) return auth.error;
5781

5882
const query = ListQuerySchema.safeParse(
5983
Object.fromEntries(new URL(request.url).searchParams.entries())
@@ -62,9 +86,18 @@ export async function loader({ request }: LoaderFunctionArgs) {
6286
return json({ error: "Invalid request", code: "invalid_request" }, { status: 400 });
6387
}
6488

65-
const context = await resolveAgentAlertContext({ userId, ...query.data });
89+
const context = await resolveAgentAlertContext({
90+
userId: auth.userId,
91+
environmentId: auth.environmentId,
92+
chatId: query.data.chatId,
93+
claimedEnvironmentId: query.data.environmentId,
94+
claimedProjectRef: query.data.projectRef,
95+
});
6696
if (!context.ok) {
67-
return json({ error: context.error, code: context.code }, { status: 404 });
97+
return json(
98+
{ error: context.error, code: context.code },
99+
{ status: contextStatus(context.code) }
100+
);
68101
}
69102

70103
const channels = await $replica.projectAlertChannel.findMany({
@@ -92,10 +125,9 @@ export async function action({ request }: ActionFunctionArgs) {
92125
return json({ error: "Method not allowed" }, { status: 405 });
93126
}
94127

95-
const userId = await authenticate(request);
96-
if (!userId) {
97-
return json({ error: "Invalid or missing access token" }, { status: 401 });
98-
}
128+
const auth = await authenticate(request);
129+
if ("error" in auth) return auth.error;
130+
const { userId } = auth;
99131

100132
let body: z.infer<typeof CreateBodySchema>;
101133
try {
@@ -110,12 +142,16 @@ export async function action({ request }: ActionFunctionArgs) {
110142

111143
const context = await resolveAgentAlertContext({
112144
userId,
145+
environmentId: auth.environmentId,
113146
chatId: body.chatId,
114-
environmentId: body.environmentId,
115-
projectRef: body.projectRef,
147+
claimedEnvironmentId: body.environmentId,
148+
claimedProjectRef: body.projectRef,
116149
});
117150
if (!context.ok) {
118-
return json({ error: context.error, code: context.code }, { status: 404 });
151+
return json(
152+
{ error: context.error, code: context.code },
153+
{ status: contextStatus(context.code) }
154+
);
119155
}
120156
const { environment } = context;
121157

‎apps/webapp/app/routes/api.v1.dashboard-agent.watches.$watchId.check.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { json, type ActionFunctionArgs } from "@remix-run/server-runtime";
2-
import { cancelWatch, getWatch, recordWatchTick } from "@internal/dashboard-agent-db";
2+
import { cancelWatch, getWatch, recordWatchCheck } from "@internal/dashboard-agent-db";
33
import { z } from "zod";
44
import { dashboardAgentDb } from "~/services/dashboardAgentDb.server";
55
import { logger } from "~/services/logger.server";
@@ -26,10 +26,12 @@ import {
2626
* 3. the USER is re-authorized against that snapshot on EVERY call, and a revoked
2727
* user gets the watch cancelled here, before any environment data is read.
2828
*
29-
* The route does NOT transition the watch to fired/expired. It records the tick
30-
* (which is also how the row keeps `lastResult` for the notification) and returns
31-
* the verdict; the watcher task owns the fire/expire transition and delivery, so
32-
* exactly one component decides when the user gets told.
29+
* The route does NOT transition the watch to fired/expired, and does NOT advance
30+
* the tick counter. It records what the check observed (`lastCheckedAt` plus the
31+
* `lastResult` the notification reads) and returns the verdict; the watcher task
32+
* owns the fire/expire transition and the delivery, so exactly one component
33+
* decides when the user gets told. The tick counter likewise has exactly one
34+
* writer — the task's generation claim — so nothing here can fork the tick chain.
3335
*/
3436

3537
const ParamsSchema = z.object({ watchId: z.string().min(1) });
@@ -142,10 +144,11 @@ export async function action({ request, params }: ActionFunctionArgs) {
142144
(error) => logger.error("Dashboard agent watch check failed", { watchId, error })
143145
);
144146

145-
// Record the tick even on the final evaluation: it stamps `lastCheckedAt` and
147+
// Record the check even on the final evaluation: it stamps `lastCheckedAt` and
146148
// parks `lastResult` on the row, which is the payload the notification reads.
147149
// Guarded on `active`, so a concurrent fire/expire simply wins and this no-ops.
148-
await recordWatchTick(dashboardAgentDb, {
150+
// Never touches `tickCount` — see the note above.
151+
await recordWatchCheck(dashboardAgentDb, {
149152
id: watchId,
150153
lastResult: { result: outcome.result, facts: outcome.facts, final: body.final === true },
151154
});

‎apps/webapp/app/routes/api.v1.dashboard-agent.watches.ts‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -23,25 +23,29 @@ import { authenticateUatOrApiRequest } from "~/services/uatRoutePreamble.server"
2323
* later runs in the background on a user's behalf, so the only caller it trusts is
2424
* the agent acting for the signed-in user in a live chat.
2525
*
26-
* The token carries a userId and nothing else, so everything else is a claim to be
27-
* proven:
26+
* Order of authority:
2827
*
29-
* - the CHAT must be a live chat owned by that user, or a watch could be bound
30-
* to (and later wake) someone else's conversation. That same scoped read also
31-
* yields the chat's org and its project/environment context.
32-
* - the ENVIRONMENT — from the body when the caller sends one, otherwise the
33-
* chat's stored context — is re-authorized through the same path a background
34-
* check uses, so the agent can't name an environment its user can't reach, and
35-
* must belong to the chat's org.
28+
* - the ENVIRONMENT comes from the TOKEN, which the dashboard minted for the
29+
* environment the current turn is being taken in. Nothing in the request body
30+
* can widen or move it: an `environmentId`/`projectRef` that disagrees is a
31+
* 400, and a token with no environment scope can't create a watch at all. A
32+
* chat's stored context is never consulted — it's a snapshot from whenever the
33+
* chat started, and would silently bind a watch to a stale environment.
34+
* - the CHAT must still be a live chat owned by that user, or a watch could be
35+
* bound to (and later wake) someone else's conversation. That scoped read also
36+
* yields the chat's org, which the token's environment must match.
37+
* - the environment is then re-authorized through the same path a background
38+
* check uses, so a watch is only created for an environment its user can reach
39+
* right now.
3640
*/
3741

3842
const BodySchema = z.object({
3943
spec: watchSpecSchema,
4044
chatId: z.string().min(1),
4145
/**
42-
* Optional overrides for the chat's stored context. `environmentId` is
43-
* `RuntimeEnvironment.id` — the canonical environment identity (VERDICTS §3).
44-
* Both are re-authorized, never trusted.
46+
* Echoes of the turn's environment, if the caller sends them. Not overrides:
47+
* they're only ever checked against the token's environment scope, which is the
48+
* canonical `RuntimeEnvironment.id` (VERDICTS §3).
4549
*/
4650
projectRef: z.string().min(1).optional(),
4751
environmentId: z.string().min(1).optional(),
@@ -60,6 +64,15 @@ export async function action({ request }: ActionFunctionArgs) {
6064
return json({ error: "Not allowed", code: "forbidden_client" }, { status: 403 });
6165
}
6266
const userId = authentication.userActor.userId;
67+
// The environment this turn is scoped to. Absent means the turn was minted
68+
// without one, and there is nothing to fall back to that we'd trust.
69+
const environmentId = authentication.userActor.environmentId;
70+
if (!environmentId) {
71+
return json(
72+
{ error: "This chat has no environment context to watch in.", code: "invalid_target" },
73+
{ status: 400 }
74+
);
75+
}
6376

6477
let parsed: z.infer<typeof BodySchema>;
6578
try {
@@ -72,22 +85,26 @@ export async function action({ request }: ActionFunctionArgs) {
7285
return json({ error: "Invalid watch request", code: "invalid_request" }, { status: 400 });
7386
}
7487

88+
// A body that names a different environment than the turn is a bug or an
89+
// attempt to move the watch — either way, refuse rather than silently pick one.
90+
if (parsed.environmentId && parsed.environmentId !== environmentId) {
91+
return json(
92+
{
93+
error: "That environment isn't the one this chat is open in.",
94+
code: "environment_mismatch",
95+
},
96+
{ status: 400 }
97+
);
98+
}
99+
75100
try {
76-
// Ownership first — a chat this user doesn't own doesn't exist as far as this
101+
// Ownership — a chat this user doesn't own doesn't exist as far as this
77102
// endpoint is concerned, and nothing is written.
78103
const chat = await resolveChatWatchContext({ chatId: parsed.chatId, userId });
79104
if (!chat) {
80105
return json({ error: "Chat not found", code: "chat_not_found" }, { status: 404 });
81106
}
82107

83-
const environmentId = parsed.environmentId ?? chat.environmentId;
84-
if (!environmentId) {
85-
return json(
86-
{ error: "This chat has no environment context to watch in.", code: "invalid_target" },
87-
{ status: 400 }
88-
);
89-
}
90-
91108
// The same authorization a background check applies, so a watch is only ever
92109
// created for an environment its user can reach right now.
93110
const environment = await authorizeWatchEnvironmentById({ userId, environmentId });
@@ -98,9 +115,15 @@ export async function action({ request }: ActionFunctionArgs) {
98115
if (environment.organizationId !== chat.organizationId) {
99116
return json({ error: "Environment not found", code: "invalid_target" }, { status: 404 });
100117
}
101-
const projectRef = parsed.projectRef ?? chat.projectRef;
102-
if (projectRef && environment.project.externalRef !== projectRef) {
103-
return json({ error: "Environment not found", code: "invalid_target" }, { status: 404 });
118+
// Same check as `environmentId`, for callers that send the project instead.
119+
if (parsed.projectRef && environment.project.externalRef !== parsed.projectRef) {
120+
return json(
121+
{
122+
error: "That project isn't the one this chat is open in.",
123+
code: "environment_mismatch",
124+
},
125+
{ status: 400 }
126+
);
104127
}
105128

106129
const result = await createDashboardAgentWatch({

0 commit comments

Comments
 (0)