feat(webapp): query boundary pinned end-to-end and a capped query retry - #4549
Conversation
The read-only guard was enforced by the TRQL grammar and a parser test, but nothing proved the route itself refuses a write; a route test now drives api.v1.query with a signed environment JWT and asserts nothing reaches ClickHouse. readonly=1 is no longer overridable by a caller's clickhouseSettings. run_query gives up after three consecutive failures so a broken query can't burn a whole agent turn. TRI-11165
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe webapp now rejects mutating query statements and enforces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…65' into feat/query-safety-tri-11165
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
…65' into feat/query-safety-tri-11165
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…x/watch-mode-keepalive-tri-13065
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
## What & why Test-only hardening for chat.agent durability. chat.agent gets its durability from the primitive — object-store snapshot + S2 `.in`/`.out` replay + continuation boot — but several store-level mechanisms that replay lands on had no regression test, and two of them were criticals in the 2026-06-10 chat.agent audit: **cross-tenant isolation** and **no duplicate mid-stream turn**. This adds those cases against a real Postgres table (testcontainers, no mocks). [TRI-11166](https://linear.app/triggerdotdev/issue/TRI-11166). ## Stack Stacked on **#4549** (query boundary). Merge that first. ## What's inside | # | Mechanism | Test | Coverage | Control-broken | |---|-----------|------|----------|----------------| | 1 | **Cross-tenant isolation** (audit critical) | `dashboardAgentTenantIsolation.test.ts` | Full at the store seam we own: getChatMessages / getSession / chatExists / listChats / countUserMessages / appendChatMessageOnce all refuse a foreign (org, user) — a foreign tenant reads not-found, never a transcript or the session's public access token | Yes | | 2 | **No duplicate mid-stream turn** (audit critical) | `dashboardAgentDurableResume.test.ts` | Full: a streamed-then-resumed turn finalises in place and appends nothing; row count and the position allocator both pinned | Yes | | 3 | **Crash-resume reconstructs state** | `dashboardAgentDurableResume.test.ts` | Full: replay keeps the mid-turn append, finalises the turn's own message, loses no messages, and rebuilds the session cursor read back via getSession | — | | 4 | **Mid-stream refresh resumes in-flight turn (Last-Event-ID)** | `dashboardAgentDurableResume.test.ts` | Seam-only: the cursor getSession hands a refreshed client, and a later turn advances (never appends) it. Client-side reconnect / Last-Event-ID replay is already covered in `packages/trigger-sdk/src/v3/chat.test.ts` — not duplicated | — | | 5 | **Snapshot write-failure path** | `dashboardAgentDurableResume.test.ts` | Full at this seam: a persistTurn that throws commits nothing (no rows, allocator untouched, cursor unchanged — the version-mismatch case), and the retry replays with no loss | — | | 6 | **`.out` trimming / OOM retry restarts cleanly** | `dashboardAgentDurableResume.test.ts` | Seam-only: a restarted turn that re-sends its snapshot loses no data and doubles nothing. `.out` trimming and the OOM restart itself are inside the closed primitive (not reachable) | — | The "Full" vs "Seam-only" column is the honest distinction: full means the whole mechanism is exercised from the repos we own; seam-only means we pin the store contract the primitive depends on, and the primitive-internal half lives where we can't reach it. ## Key decisions - **The two criticals were control-broken first.** Isolation: removing the `organizationId` filter from `getChatMessages` leaks org A's transcript to the owner's user id under another org — the test fails at `toBeNull()`. No-duplicate: removing the stored-id drop in `storeChatMessages` makes a replayed persistTurn over-reserve positions (next free slot jumps 3 → 7) — the test fails on the allocator assertion. Both reverted. - **Real Postgres, no mocks.** testcontainers against an actual table, so the store contract is proven, not asserted against a stub. ## Residual follow-ups These live inside the closed chat.agent primitive package and can't be unit-tested from the repos we own; the tests above are the store-level backstop they depend on: - The snapshot URL's own auth gate (the audit's snapshot-URL auth gap) — enforced in the primitive; here we prove the webapp store never hands a foreign tenant the PAT it would boot from. - Object-store snapshot write + S2 `.in`/`.out` replay at the transport level. - `.out` trimming never dropping in-flight data, and the OOM restart mechanism itself. ## Testing `pnpm run test --filter webapp` — `dashboardAgentTenantIsolation.test.ts` and `dashboardAgentDurableResume.test.ts`.
| import { | ||
| appendChatMessageOnceByChatId, | ||
| createChat, | ||
| createDashboardAgentDb, | ||
| getChatMessages, | ||
| getSession, | ||
| persistMessages, | ||
| persistTurn, | ||
| type DashboardAgentDb, | ||
| type DashboardAgentDbClient, | ||
| } from "@internal/dashboard-agent-db"; | ||
| import { postgresTest } from "@internal/testcontainers"; | ||
| import type { PrismaClient } from "@trigger.dev/database"; | ||
| import { readdirSync, readFileSync } from "node:fs"; | ||
| import path from "node:path"; | ||
| import { afterEach, describe, expect } from "vitest"; | ||
|
|
||
| /** | ||
| * Durability of a chat.agent turn across a crash and a resume, against a real table | ||
| * (TRI-11166). | ||
| * | ||
| * The primitive gives chat.agent durability by snapshotting the transcript and replaying it | ||
| * on the next boot. These tests pin the store seam that replay lands on: the completing turn | ||
| * re-sends its whole snapshot, so the store has to fold that replay into exactly one row per | ||
| * message — no double-appended turn, no lost mid-turn message — and reconstruct the session | ||
| * cursor a refreshed client resumes from. | ||
| * | ||
| * What is NOT covered here, because it lives inside the closed chat.agent primitive package | ||
| * (object-store snapshot write, S2 `.in`/`.out` replay, `.out` trimming, OOM restart): the | ||
| * transport-level replay and the snapshot URL's own auth. The client-side reconnect / Last- | ||
| * Event-ID replay is covered in packages/trigger-sdk/src/v3/chat.test.ts. These tests are the | ||
| * store-level backstop those depend on. See the PR body for the residual follow-ups. | ||
| */ |
There was a problem hiding this comment.
🟡 Pull request bundles unrelated chat-store tests with the query change
Two large test files covering chat storage durability and tenant isolation (apps/webapp/test/dashboardAgentDurableResume.test.ts:1) are added alongside the query read-only/retry work, so the change covers more than one issue, which the contribution rules disallow.
Impact: Reviewers must evaluate unrelated work in one pass, making the change slower and riskier to accept.
Rule text and scope mismatch
CONTRIBUTING.md: "We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features."
The PR body and .server-changes/query-boundary-and-retry-cap.md describe only the read-only query boundary (TRI-11165) and the run_query retry cap. The two added files instead exercise @internal/dashboard-agent-db (persistTurn, getSession, getChatMessages, …) and are annotated with a different issue, TRI-11166 (apps/webapp/test/dashboardAgentTenantIsolation.test.ts:21). They also test code that lives in internal-packages/dashboard-agent-db, while AGENTS.md asks for test files to sit next to the source they cover.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // 429 is the concurrency rejection and the rate limiter: nothing is wrong with the | ||
| // query, so it is not a query error. | ||
| if (res.status === 429) { | ||
| return { | ||
| ok: false, | ||
| kind: "busy", | ||
| error: `${data.error ?? "The query service is busy right now."} You can retry the same query shortly.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔍 429 from the shared rate limiter is folded into the same "busy" bucket
The client treats any 429 as busy. Besides the concurrency rejection, the route can also be rate limited by the shared API rate limiter, whose error body shape differs from { error }; in that case data.error is undefined and the model just gets the generic "The query service is busy right now. You can retry the same query shortly." Since busy never counts toward the retry cap, a persistently rate-limited environment lets the agent keep retrying queries until the turn's step budget is exhausted — the exact failure mode the cap was added to prevent. Worth confirming the rate-limit path is bounded elsewhere.
Was this helpful? React with 👍 or 👎 to provide feedback.
## What & why
The Free-plan agent-message allowance was only a client-side hint
(`useIsFreePlan()` was hardcoded to `undefined`, so nothing was
enforced) and the running count was a live `COUNT(*)` over
`chat_messages` — which meant deleting a chat silently freed quota. This
PR makes the allowance a real server-side limit with a durable counter,
so the cap holds regardless of the client and a deleted chat can't
reclaim messages within the period.
The cloud side that fills the actual per-plan number is a separate PR
(TRI-12863 P0). Until it deploys the `agentMessages` limit key is
absent, which resolves to the repo's unlimited sentinel — the correct
fail-open default, and why this ships independently.
## What's inside
- **New counter table** `trigger_dashboard_agent.agent_message_usage`,
keyed `(organization_id, period)` where `period` is a UTC calendar month
`"YYYY-MM"`. FK-free (`pgSchema` convention), plus a drizzle migration.
Deliberately **not** joined to chats — that closes the delete-a-chat
hole. Distinct from TRI-13068's `AiUsageEvent`; neither is derived from
the other.
- **New service** `dashboardAgentQuota.server.ts`: a pure
`checkAgentMessageQuota({ used, limit })` (so the MCP send path can
reuse the rule later) plus an org-scoped resolver that reads the period
counter and the cached plan limit, and a `recordAgentMessageSent`
increment.
- **Counting on send**: the create path (head start) and the `.in`
append path each increment one user message. The append path counts only
after the existing `trigger === "action"` 403, so **wakes never count**.
- **Enforcement**: at/over the limit both send routes return `403 {
error: "message_quota_reached", limit }`, which the client renders as
`AgentUpgradeBlock` rather than a generic failure. Never a silent drop.
- **Client**: `?quota=1` now reads the period counter; `useIsFreePlan()`
is a real read gated on **billing presence** (no subscription →
self-hosted → no cap, no upgrade UI), not on the plan value.
## Key decisions
- **Fails open.** The cap is a nudge, not a security boundary. An absent
limit (self-hosted, or pre-P0) resolves to the unlimited sentinel
`100_000_000` (never `Infinity` — that serializes to `null` in the Redis
limit cache), and a counter read that throws returns "no cap".
Self-hosted needs zero extra branching — it falls out of the fallback,
with a test to prove it.
- **Billing-presence gate.** The upgrade UI keys off whether a
subscription exists, so a self-hosted install with no billing shows no
cap and no upsell label.
- **The `(org, period)` table closes the delete hole.** A standalone
counter, not a count over chat rows, so deleting a chat can't free quota
inside the period.
## Testing
`apps/webapp/test/dashboardAgentQuota.test.ts` (testcontainers, no
mocks):
- pure `checkAgentMessageQuota` under/at/over/unlimited (control-breaks
the `>=`);
- `agentTurnCountsAgainstQuota` counts a message but not a wake
(control-breaks the wake exclusion);
- the counter accumulates and a **deleted chat can't free quota** within
the period; other periods/orgs start fresh;
- the resolver reports reached over the limit and never reached when
unlimited;
- fails open when the limit is absent (self-hosted) and when the counter
read throws.
`TRI-12863`
e2948f1
into
fix/watch-mode-keepalive-tri-13065
| const readLimit = | ||
| params.readLimit ?? | ||
| (async (organizationId: string) => { | ||
| const cached = await getCachedLimit( | ||
| organizationId, | ||
| AGENT_MESSAGE_LIMIT_KEY, | ||
| UNLIMITED_AGENT_MESSAGES | ||
| ); | ||
| // A cache error leaves `val` empty; fall open to unlimited. | ||
| return cached.val ?? UNLIMITED_AGENT_MESSAGES; | ||
| }); |
There was a problem hiding this comment.
🟡 A plan that allows zero agent messages is treated as unlimited
The monthly message allowance is looked up in a way that turns a plan value of zero into "no limit" (getCachedLimit at apps/webapp/app/services/dashboardAgentQuota.server.ts:47), so an organisation whose plan grants no agent messages can send them without restriction.
Impact: Organisations on a plan with the agent switched off would still be able to chat with the dashboard agent for free.
Mechanism: `getLimit`'s falsy fallback versus the zero-aware reader added for watches
getCachedLimit delegates to getLimit, which does if (!result) return fallback (apps/webapp/app/services/platform.v3.server.ts:427). A plan value of 0 is falsy, so the fallback UNLIMITED_AGENT_MESSAGES (100_000_000) is returned and checkAgentMessageQuota never reports reached.
The same PR introduces getCachedLimitAllowingZero / limitValueAllowingZero (apps/webapp/app/services/platform.v3.server.ts:489-514) precisely for this case and uses it for the watch limits (apps/webapp/app/services/dashboardAgentWatchLimits.server.ts:24, with the comment "A plan of 0 means zero, not absent"). The message-quota resolver was not migrated to it, so the two limit families disagree on what 0 means.
| const readLimit = | |
| params.readLimit ?? | |
| (async (organizationId: string) => { | |
| const cached = await getCachedLimit( | |
| organizationId, | |
| AGENT_MESSAGE_LIMIT_KEY, | |
| UNLIMITED_AGENT_MESSAGES | |
| ); | |
| // A cache error leaves `val` empty; fall open to unlimited. | |
| return cached.val ?? UNLIMITED_AGENT_MESSAGES; | |
| }); | |
| const readLimit = | |
| params.readLimit ?? | |
| (async (organizationId: string) => { | |
| const cached = await getCachedLimitAllowingZero( | |
| organizationId, | |
| AGENT_MESSAGE_LIMIT_KEY, | |
| UNLIMITED_AGENT_MESSAGES | |
| ); | |
| // A cache error leaves `val` empty; fall open to unlimited. | |
| return cached.val ?? UNLIMITED_AGENT_MESSAGES; | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| return resolveMessageQuota({ | ||
| isFreePlan, | ||
| used: usedElsewhere === undefined ? undefined : usedElsewhere + countUserMessages(messages), | ||
| }); | ||
| return resolveMessageQuota({ isFreePlan, used }); |
There was a problem hiding this comment.
🔍 The client caps at a hardcoded 20 while the server enforces the plan limit
useAgentMessageQuota now reads the org-wide server counter but resolves it against the client constant FREE_PLAN_MESSAGE_LIMIT = 20 (apps/webapp/app/components/dashboard-agent/message-quota.ts:5), whereas the server compares the same counter against the plan's agentMessages limit (apps/webapp/app/services/dashboardAgentQuota.server.ts:63). Today the plan key is absent and resolves to the unlimited sentinel, so on cloud free plans the composer will be hidden at 20 messages even though the server would happily accept more; conversely if the cloud limit is ever set below 20 the client will keep offering the composer until the server 403s. The ?quota=1 loader already knows the effective limit — returning it alongside used would keep the two in step.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // A concurrency rejection is "too busy", not a bad query: 429 so callers retry it | ||
| // instead of rewriting a query that was fine. | ||
| if (isQueryConcurrencyRejection(queryResult.error)) { | ||
| return json({ error: queryResult.error.message }, { status: 429 }); | ||
| } |
There was a problem hiding this comment.
🔍 429 is a new response status for an existing public endpoint
Concurrency rejections previously surfaced as 400 (they are QueryErrors) and now return 429. The OpenAPI spec is updated, but any existing SDK/client that branches on status === 400 for "query rejected" will now see an unhandled status; the agent client is updated (internal-packages/dashboard-agent/src/tool-api-client.ts:221-227) but packages/trigger-sdk's query.execute was not audited in this diff. Worth confirming the SDK surfaces the 429 with a sensible error rather than a generic failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
… — and fixes (#4516) Plan enforcement for the dashboard agent — message quota and watch limits — plus the component gallery, fixes and test hardening from the same stack (#4548, #4549, #4550, #4552, #4556 merged here). ## Plan enforcement ([TRI-12863](https://linear.app/triggerdotdev/issue/TRI-12863)) **Agent message quota.** The Free-plan allowance becomes a real server-side limit with a durable counter. New `agent_message_usage` table keyed `(organization_id, period)` — deliberately not joined to chats, so deleting a chat can't free quota within the period. Both send paths count one user message (wakes never count) and refuse at the cap with `403 message_quota_reached`, which the client renders as an upgrade panel, never a silent drop. The refusal code is a single shared constant on both sides. **Watch limits.** A watch whose window exceeds the plan's `agentWatchMaxHours`, or that would push the org past its `agentWatchers` count, is refused with `watch_limit_reached` (409 on the API, an upgrade hint on the card). Plan limits only tighten the existing code ceilings (`min(plan, 24h)`, per-chat cap of 3 still applies). A plan limit of zero means zero, not unlimited. Questions answerable instantly are answered before any plan refusal — a one-shot consumes no slot and never sees an upgrade nag. **Fails open by design.** Cloud ships the actual per-plan numbers separately (TRI-12863 P0). Until then absent limits resolve to the unlimited sentinel and the upgrade UI is gated on billing presence — self-hosted sees no cap, no upsell, with tests proving the fallback. Both quotas are nudges, not security boundaries: a failing limit read never blocks a send. ## Component gallery An admin-only gallery of every agent card state: five `storybook.agent-*` pages (chat UI, view blocks, report, investigation, watch) with their shared shell and manifest, demo fixtures, two demo-only cards, toast examples, and the screenshot script. No LLM and no data — every state renders from fixtures under `dashboard-agent/demo/`, never reachable from a production path. Designers and reviewers can look at every state, including the report states, without seeding anything. ## And fixes **SDK: watch-mode chat subscriptions survive quiet windows** (TRI-13065, TRI-13070) — watch mode keeps reconnecting across empty long-poll windows and only stops on abort or a settled session; a passive subscriber can no longer stop a turn it doesn't own (`stopOnAbort` is explicit, default off). Review findings fixed alongside: a superseded stream's async teardown no longer removes the live successor's abort controller or multi-tab claim, and stopping a generation hands the chat back to the user's other tabs. **Query boundary pinned end-to-end** ([TRI-11165](https://linear.app/triggerdotdev/issue/TRI-11165)) — a route-level test drives `api.v1.query` with a real signed environment JWT (writes refused before ClickHouse, a read passes); `readonly=1` made non-overridable; a per-turn cap stops the model burning a turn rewriting a query it can't fix (deterministic SQL errors only — busy/transport rejections don't count). **chat.agent durability regression suite** ([TRI-11166](https://linear.app/triggerdotdev/issue/TRI-11166)) — testcontainers-backed coverage of the two audit criticals (cross-tenant isolation, no duplicate mid-stream turn, both control-broken) plus crash-resume, cursor-based refresh, clean rollback of a mid-write turn failure (torn by a real constraint violation), and OOM-restart replay. **Investigation sweep backoff** — stale investigations get an attempt counter and backoff so a poison row can't pin the sweep queue head (migration `0005`: `sweep_attempts`, `last_sweep_attempt_at`). ## Screenshots <img width="1440" height="791" alt="Screenshot 2026-08-06 at 00 36 19" src="https://github.com/user-attachments/assets/6a68cd42-8580-469d-afe7-e28d1eef18e1" /> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 participants
What & why
The agent's query tool is read-only, but that was true by three separate facts and only one of them had a test. This proves the boundary holds by contract rather than by prompt, and stops a broken query from burning a whole agent turn.
Two small guards, the rest is tests. TRI-11165.
Stack
Stacked on #4548 (watch-mode keepalive). Merge that first.
What's inside
apps/webapp/test/queryRouteReadOnly.test.ts) that drivesapi.v1.querywith a real signed environment JWT: a multi-statement write and a mutating statement are both refused before anything reaches ClickHouse, and a plain read passes so the seam stays live.readonly=1made non-overridable inqueryService.server.ts— callerclickhouseSettingswere spread after the defaults and could clear it.run_querytool (internal-packages/dashboard-agent/src/tool-api.ts): three consecutive failures returns a terminal "stop and answer with what you have".Key decisions
readonly=1, and the org/project/env scoping is injected server-side from the credential. The request body can't widen scope or turn a read into a write.Testing
queryRouteReadOnly.test.ts— write statements → 400, ClickHouse never called; a read passes.tool-query-retry-cap.test.ts— terminal at the third consecutive failure, counter resets on a success.