feat(webapp): enforce watch plan limits - #4556
Conversation
Refuse a watch whose window exceeds the plan's agentWatchMaxHours, or that would push the org past its agentWatchers count, with a new watch_limit_reached result carrying an upgrade hint. Plan limits are a floor below the existing code ceilings (min(plan, WATCH_MAX_HOURS=24) and the per-chat cap of 3, which still apply independently). Fails open: an absent limit resolves to unlimited, so self-hosted is unaffected and the upgrade nudge is gated on billing presence. TRI-12863
|
|
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 pull request adds plan-based limits for dashboard agent watches. It limits watch duration and organization-wide active watches, adds billing-aware refusal messages, and maps limit refusals to HTTP 409. It also adds sweep-attempt persistence for stale investigations. Failed settlements are retried up to five times before force abandonment as inconclusive. Integration tests cover both watch-limit enforcement and poison-investigation handling. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
…63' into feat/agent-watch-limits-tri-12863
![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.
@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: |
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.
The card-submit route's status ladder didn't handle watch_limit_reached, so a plan-limit refusal fell through to HTTP 500. Match the MCP route and return 409.
drizzle-kit generated them unformatted, failing the oxfmt --check code-quality gate.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…at/agent-message-quota-tri-12863
| process.env.SESSION_SECRET = "test-session-secret-for-watch-limit-status"; | ||
|
|
||
| const { action } = | ||
| await import("~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent"); |
There was a problem hiding this comment.
🔍 New status-ladder test may never reach the plan-limit branch without DASHBOARD_AGENT_SECRET_KEY
This test drives the real watch-create route down to createDashboardAgentWatch, which returns not_configured (mapped to HTTP 501) whenever DASHBOARD_AGENT_SECRET_KEY is unset (apps/webapp/app/services/dashboardAgentWatches.server.ts:307-314, apps/webapp/app/services/dashboardAgent.server.ts:83-85). That check runs before the plan-window check, so the assertion expect(response.status).toBe(409) only passes if the env var happens to be set. Every other watch suite in the repo sets it explicitly at module scope (e.g. apps/webapp/test/dashboardAgentWatches.test.ts:113, apps/webapp/test/dashboardAgentWatchAlertOwnerScope.test.ts:77); this file only sets SESSION_SECRET. On CI, where apps/webapp/.env is absent, the route would answer 501 and the test would fail.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch (error) { | ||
| // The settle rolled back, so the row is still `in_progress`. Record the attempt in | ||
| // its own write — this rotates the row to the back of the sweep order (see | ||
| // `listStaleOpenInvestigations`) so it can't pin the head and starve newer rows. | ||
| let attempts: number | null = null; | ||
| try { | ||
| attempts = await recordAttempt({ id: investigation.id }); | ||
| } catch (recordError) { | ||
| logger.error("Dashboard agent investigation sweep: failed to record a sweep attempt", { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| error: recordError, | ||
| }); | ||
| } | ||
|
|
||
| // Past the cap the card will never render; force it terminal without the render | ||
| // path so it leaves the queue instead of looping forever. | ||
| if (attempts !== null && attempts >= MAX_SWEEP_ATTEMPTS) { | ||
| try { | ||
| await forceAbandon({ id: investigation.id, note: UNSETTLED_INVESTIGATION_NOTE }); | ||
| result.abandoned++; | ||
| logger.warn( | ||
| "Dashboard agent investigation sweep: abandoned a card past the attempt cap", | ||
| { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| attempts, | ||
| } | ||
| ); | ||
| continue; | ||
| } catch (abandonError) { | ||
| logger.error("Dashboard agent investigation sweep: failed to abandon a poison card", { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| error: abandonError, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Transient settle failures share the same attempt budget as permanently unrenderable states
The attempt counter is incremented for any thrown error from settleAndClose, not just the "state isn't renderable" case the cap is designed for. Because the run rethrows on failure and the job retries, five consecutive transient failures (a DB blip, a connection reset during the append) will burn the budget and force-abandon the row via settleInvestigationAsInconclusive without ever appending the closing card — exactly the permanent spinner the transactional settle exists to prevent. Distinguishing the non-renderable error (which settleInvestigationAndCloseCard throws deliberately at internal-packages/dashboard-agent-db/src/queries.ts:1156) from infrastructure errors would keep the cap targeted.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts (1)
133-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the null return from
forceAbandon.
settleInvestigationAsInconclusivereturnsnullwhen the row is no longerin_progress. The code discards the return value and incrementsabandonedin both cases. If a turn settles the row between the failed settle and the force-abandon, the counter reports an abandonment that did not happen. The row is terminal either way, so behavior is correct; only the counter is affected.♻️ Proposed change to attribute the race to `alreadySettled`
- await forceAbandon({ id: investigation.id, note: UNSETTLED_INVESTIGATION_NOTE }); - result.abandoned++; - logger.warn( - "Dashboard agent investigation sweep: abandoned a card past the attempt cap", - { - investigationId: investigation.id, - chatId: investigation.chatId, - attempts, - } - ); - continue; + const abandoned = await forceAbandon({ + id: investigation.id, + note: UNSETTLED_INVESTIGATION_NOTE, + }); + if (abandoned) { + result.abandoned++; + logger.warn( + "Dashboard agent investigation sweep: abandoned a card past the attempt cap", + { + investigationId: investigation.id, + chatId: investigation.chatId, + attempts, + } + ); + } else { + result.alreadySettled++; + } + continue;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82038eb3-0c2f-4692-9347-5ddb7dcef0f6
📒 Files selected for processing (15)
.server-changes/agent-watch-plan-limits.mdapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentWatches.server.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsinternal-packages/dashboard-agent-db/drizzle/0005_ambitious_mordo.sqlinternal-packages/dashboard-agent-db/drizzle/meta/0005_snapshot.jsoninternal-packages/dashboard-agent-db/drizzle/meta/_journal.jsoninternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/schema.tsinternal-packages/dashboard-agent-db/src/watch-queries.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 12)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamicimport(); use dynamic imports only for unresolvable circular dependencies, genuine performance code splitting, or conditional runtime loading.
Import Trigger.dev tasks from@trigger.dev/sdk; never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Add agentcrumbs while writing code using approved namespaces; mark lines with//@Crumbsor blocks with `// `#region` `@crumbs, and strip them before merging.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathDo not reintroduce the removed v1 execution path;
RunEngineVersion.V1branches may only reject or finalize gracefully so v3 clients receive a clean 4xx, never a 5xx.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
For apps, use
typecheckfor verification and never usebuildas the correctness check.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.{ts,tsx}: For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
UseuseCallbackanduseMemoonly for context provider values, expensive derived data used as a dependency, or stable references required by dependency arrays; do not wrap ordinary event handlers or trivial computations.
Use named constants for sentinel or placeholder values instead of scattering raw string literals across comparisons.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
apps/webapp/app/routes/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/routes/**/*.ts: Use Remix flat-file route conventions with dot-separated segments; for example,api.v1.tasks.$taskId.trigger.tsmaps to/api/v1/tasks/:taskId/trigger.
PAT-authenticated API routes must resolve their target organization or project within the caller's membership scope, using a membership filter or a helper such asfindProjectByReforresolveOrganizationForApiUser; RBAC authorization alone is insufficient.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.ts: Never userequest.signalto detect client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.ts, which is wired to Express response close events.
Access environment variables through theenvexport fromapp/env.server.ts; never useprocess.envdirectly.
Always use PrismafindFirstinstead offindUnique.
Always use the$transactionhelper from~/db.server, never callprisma.$transactionor$replica.$transactiondirectly. Pass isolation levels as strings, useSerializablefor correctness-critical read-then-write invariants, and guard possibly undefined helper results when a definite value is required.
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: Use Vitest exclusively and never mock dependencies; use Testcontainers for integration dependencies.
Place test files next to the source files they test.
Files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
apps/webapp/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Test files must not import
app/env.server.ts; pass configuration as options instead.
Files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
internal-packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
For internal packages, use
typecheckfor verification and never usebuildas the correctness check.
Files:
internal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsinternal-packages/dashboard-agent-db/src/schema.ts
🧠 Learnings (22)
📚 Learning: 2026-05-14T14:54:39.095Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3545
File: .server-changes/agent-view-sessions.md:10-10
Timestamp: 2026-05-14T14:54:39.095Z
Learning: In the `trigger.dev` repository, do not flag inconsistent dot vs slash notation in route/path strings inside `.server-changes/*.md` files. These markdown files are consumed verbatim into the changelog, so the mixed notation (e.g., `resources.orgs.../runs.$runParam/...`) is intentional and should be preserved as-is.
Applied to files:
.server-changes/agent-watch-plan-limits.md
📚 Learning: 2026-07-26T13:14:02.968Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 4378
File: .server-changes/realtime-run-reads-from-primary.md:0-0
Timestamp: 2026-07-26T13:14:02.968Z
Learning: For files in the .server-changes directory, the body text is published verbatim as dashboard-facing user release notes. Write entries in terms of user-visible behavior (what users can do/see), and avoid implementation-oriented details such as environment-variable names, internal mechanisms, or configuration knobs. If you need to include operational/configuration specifics, put those details in the PR description instead of the .server-changes entry.
Applied to files:
.server-changes/agent-watch-plan-limits.md
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-13T19:53:13.759Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-17T17:13:49.929Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-25T18:21:51.905Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-revoke.tsx:0-0
Timestamp: 2026-06-25T18:21:51.905Z
Learning: During the Zod v4 migration in the triggerdotdev/trigger.dev webapp, ensure any imports from `conform-to/zod` use the Zod-4 subpath: `conform-to/zod/v4` (e.g., `import { parseWithZod } from "conform-to/zod/v4"`). Do not import from the package root `conform-to/zod`, because it is the Zod 3 implementation and may load Zod-3-only symbols (e.g., `ZodBranded`, `ZodEffects`), which can throw at module load (notably with `zod4.4.3`). This should be enforced across `apps/webapp/**/*` where helpers like `parseWithZod` and `conformZodMessage` are used.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-07-03T17:10:21.498Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4148
File: apps/webapp/app/models/orgMember.server.ts:149-168
Timestamp: 2026-07-03T17:10:21.498Z
Learning: In triggerdotdev/trigger.dev, `User.email` (Prisma schema: `internal-packages/database/prisma/schema.prisma`) currently does NOT use `citext` and does NOT have a `lower(email)` functional unique index. Therefore, do not introduce Prisma queries like `where: { email: { equals: <value>, mode: "insensitive" } }` (or any case-insensitive lookup) against `User.email`, because it can force sequential scans of the `users` table under load. During review, ensure email is normalized (e.g., lowercased/trimmed) before both writes and subsequent lookups, and if true case-insensitive behavior/uniqueness is required, implement it via a separate app-wide migration (e.g., switch to `citext` and/or add a functional unique index with backfill) rather than bolting it onto individual feature PRs.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-06-09T17:58:04.699Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.tsapps/webapp/app/routes/api.v1.dashboard-agent.watches.tsapps/webapp/test/dashboardAgentWatchLimitStatus.test.tsinternal-packages/dashboard-agent-db/src/queries.tsinternal-packages/dashboard-agent-db/src/watch-queries.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.tsapps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsinternal-packages/dashboard-agent-db/src/schema.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-05-07T12:25:18.271Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3531
File: apps/webapp/test/sentryTraceContext.server.test.ts:9-47
Timestamp: 2026-05-07T12:25:18.271Z
Learning: In the triggerdotdev/trigger.dev webapp test suite, it is acceptable to leave `createInMemoryTracing()` calls that register a global `NodeTracerProvider` without `afterEach`/`afterAll` teardown. Do not flag this as a test-ordering risk when the code follows the established pattern used across webapp tests (e.g., replication service/benchmark/backfiller tests). This is considered safe because `trace.getActiveSpan()` when called outside a `context.with(...)` block reads `AsyncLocalStorage.getStore()` (undefined when no `run()` scope exists), so it falls back to `ROOT_CONTEXT` with no attached span—regardless of which provider is registered.
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-07-30T18:43:56.874Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4426
File: apps/webapp/test/memberDevEnvironments.server.test.ts:124-125
Timestamp: 2026-07-30T18:43:56.874Z
Learning: In the `apps/webapp` test suite (`apps/webapp/test/**`), respect the established test harness in `apps/webapp/test/setup.ts`: it loads `.env` and provides default values for required environment variables so that transitive imports (e.g., `~/env.server`) work without production-style wiring.
During code review, do not require dependency injection/refactoring solely to avoid this existing import path. Only introduce configuration injection if it delivers production-level value (for example, a more general `createEnvironment` abstraction that improves runtime behavior beyond test setup).
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-08-08T12:49:17.489Z
Learnt from: kathiekiwi
Repo: triggerdotdev/trigger.dev PR: 4525
File: apps/webapp/test/dashboardAgentWatchAlertOwnerScope.test.ts:101-109
Timestamp: 2026-08-08T12:49:17.489Z
Learning: In apps/webapp/test, every test suite that creates a client with createDashboardAgentDb(connectionUri, ...) must close the DashboardAgentDbClient in afterEach. Although postgresTest drops cloned databases with WITH (FORCE), it does not clean up client-side postgres-js pool sockets or idle timers, so explicitly closing the client prevents resource leaks and test interference.
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
apps/webapp/test/dashboardAgentWatchLimitStatus.test.tsapps/webapp/test/dashboardAgentInvestigationPoison.test.tsapps/webapp/test/dashboardAgentWatchLimits.test.ts
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
📚 Learning: 2026-05-05T09:38:02.512Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3523
File: apps/webapp/app/routes/api.v3.batches.ts:178-181
Timestamp: 2026-05-05T09:38:02.512Z
Learning: When reviewing code that catches `ServiceValidationError` in `*.server.ts` files, do not blindly forward `error.status` to HTTP responses, because SVEs may be thrown with non-default statuses (e.g., 400/500) and forwarding them can cause client-visible behavioral regressions (e.g., surfacing 500s to clients). Prefer a safe default response status of `error.status ?? 422`, but only after confirming via the reachable call graph that the caught `ServiceValidationError` instances are expected to carry those non-default statuses; otherwise, normalize to `422` to avoid unexpected client-visible 5xx behavior.
Applied to files:
apps/webapp/app/services/dashboardAgentWatchLimits.server.tsapps/webapp/app/services/dashboardAgentInvestigationSweep.server.tsapps/webapp/app/services/dashboardAgentWatches.server.ts
🔇 Additional comments (21)
apps/webapp/app/services/dashboardAgentWatchLimits.server.ts (1)
1-53: LGTM!apps/webapp/app/services/dashboardAgentWatches.server.ts (1)
13-13: LGTM!Also applies to: 72-77, 175-175, 284-305, 339-349
internal-packages/dashboard-agent-db/src/watch-queries.ts (1)
497-511: LGTM!apps/webapp/app/routes/api.v1.dashboard-agent.watches.ts (1)
114-116: LGTM!apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts (1)
548-548: LGTM!apps/webapp/test/dashboardAgentWatchLimits.test.ts (1)
50-328: LGTM!apps/webapp/test/dashboardAgentWatchLimitStatus.test.ts (1)
57-161: LGTM!.server-changes/agent-watch-plan-limits.md (1)
1-6: LGTM!internal-packages/dashboard-agent-db/drizzle/0005_ambitious_mordo.sql (1)
1-2: LGTM!internal-packages/dashboard-agent-db/drizzle/meta/0005_snapshot.json (1)
631-643: LGTM!internal-packages/dashboard-agent-db/drizzle/meta/_journal.json (1)
39-45: LGTM!internal-packages/dashboard-agent-db/src/schema.ts (1)
173-176: LGTM!internal-packages/dashboard-agent-db/src/queries.ts (2)
1066-1091: LGTM!
1093-1118: LGTM!apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts (3)
116-129: LGTM!
28-33: LGTM!Also applies to: 61-64, 80-92
43-44: 🗄️ Data Integrity & IntegrationNo issue found:
InvestigationSweepResulthas one producer, and it initializesabandoned.> Likely an incorrect or invalid review comment.apps/webapp/test/dashboardAgentInvestigationPoison.test.ts (4)
1-31: LGTM!
62-103: LGTM!
105-166: LGTM!
33-45: 📐 Maintainability & Code QualityKeep the local migration helper.
| // Counted only now the immediate check didn't answer: a one-shot creates no row and so | ||
| // consumes no watcher slot. The per-chat cap of 3 still applies independently, in | ||
| // `createWatch`. | ||
| const activeCount = await countActiveWatches(environment.organizationId); | ||
| if (activeCount >= planLimits.watchers) { | ||
| return { | ||
| ok: false, | ||
| code: "watch_limit_reached", | ||
| error: hint("You've reached the number of active watches your plan allows."), | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the organization watcher quota atomic with watch creation.
The count runs before createWatch. Two requests for different chats can both read a count below the limit and both insert an active watch. The organization can then exceed planLimits.watchers.
Move the count check and insert into one transaction with an organization-scoped lock or equivalent atomic database guard. Add a concurrent-create test.
| vi.mock("~/db.server", () => { | ||
| const proxy = new Proxy( | ||
| {}, | ||
| { get: (_target, prop) => (ctx.prisma as unknown as Record<string, unknown>)[prop as string] } | ||
| ); | ||
| return { prisma: proxy, $replica: proxy, sqlDatabaseSchema: undefined }; | ||
| }); | ||
|
|
||
| vi.mock("~/services/session.server", () => ({ | ||
| requireUser: async () => ({ id: ctx.userId, admin: false, isImpersonating: false }), | ||
| })); | ||
|
|
||
| vi.mock("~/v3/canAccessDashboardAgent.server", () => ({ | ||
| canAccessDashboardAgent: async () => true, | ||
| })); | ||
|
|
||
| vi.mock("~/services/dashboardAgentDb.server", () => ({ | ||
| get dashboardAgentDb() { | ||
| return ctx.agentDb; | ||
| }, | ||
| })); | ||
|
|
||
| // The only stub: the plan floor billing would resolve. A 1-hour window makes a 2-hour watch | ||
| // exceed the plan, so the real submit path returns `watch_limit_reached`. Everything else runs. | ||
| vi.mock("~/services/dashboardAgentWatchLimits.server", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof WatchLimitsModule>(); | ||
| return { | ||
| ...actual, | ||
| resolveWatchPlanLimits: async () => ({ | ||
| maxHours: 1, | ||
| watchers: actual.UNLIMITED_WATCH_LIMIT, | ||
| }), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove module mocks from these integration tests.
Both suites use PostgreSQL Testcontainers but still replace production dependencies with vi.mock(...). Use a test harness or injected production-independent dependencies instead.
apps/webapp/test/dashboardAgentWatchLimitStatus.test.ts#L22-L55: remove the mocked database, session, access, dashboard database, and plan-limit modules.apps/webapp/test/dashboardAgentWatchLimits.test.ts#L26-L48: remove the mocked database, dashboard database, and SDK modules.
As per coding guidelines, “Use Vitest exclusively and never mock dependencies; use Testcontainers for integration dependencies.”
📍 Affects 2 files
apps/webapp/test/dashboardAgentWatchLimitStatus.test.ts#L22-L55(this comment)apps/webapp/test/dashboardAgentWatchLimits.test.ts#L26-L48
Source: Coding guidelines
| async function readLimit(organizationId: string, key: keyof Limits): Promise<number> { | ||
| const cached = await getCachedLimit(organizationId, key, UNLIMITED_WATCH_LIMIT); | ||
| // A cache error leaves `val` empty; fall open to unlimited. | ||
| return cached.val ?? UNLIMITED_WATCH_LIMIT; | ||
| } |
There was a problem hiding this comment.
🟡 A plan that allows zero watches is treated as allowing unlimited watches
A plan value of zero for either watch limit is read as "no limit set" and silently replaced by the unlimited sentinel (getCachedLimit(...) at apps/webapp/app/services/dashboardAgentWatchLimits.server.ts:22), so an organization meant to have watches turned off gets them without restriction.
Impact: If a plan is ever configured to allow zero watches, users on that plan can create unlimited watches instead of none.
Falsy-zero handling in the shared limit reader
getLimit in apps/webapp/app/services/platform.v3.server.ts:421-430 does const result = limits[limit]; if (!result) return fallback; — a numeric 0 is falsy, so it returns the caller's fallback rather than 0. Here the fallback is UNLIMITED_WATCH_LIMIT (100_000_000), so agentWatchers: 0 or agentWatchMaxHours: 0 on a plan resolves to unlimited, and both floors in createDashboardAgentWatch (apps/webapp/app/services/dashboardAgentWatches.server.ts:342 and :368) become no-ops. The readLimit helper cannot distinguish "limit absent" (intended fail-open) from "limit is zero" (intended hard block).
Prompt for agents
resolveWatchPlanLimits/readLimit in apps/webapp/app/services/dashboardAgentWatchLimits.server.ts relies on getCachedLimit -> getLimit (apps/webapp/app/services/platform.v3.server.ts), which returns the caller-supplied fallback for any falsy limit value, including a legitimate 0. Because the fallback here is the unlimited sentinel, a plan configured with agentWatchers: 0 or agentWatchMaxHours: 0 would be interpreted as unlimited instead of blocking watches entirely. Consider reading the raw limits (e.g. via getLimits) and distinguishing undefined from 0, or documenting/asserting that 0 will never be configured for these keys.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Plan floors sit below the code ceilings (min(plan, ceiling)). Fails open: an absent | ||
| // limit resolves to unlimited, so neither floor bites on self-hosted. | ||
| const planLimits = await resolveLimits(environment.organizationId); | ||
| if (spec.maxHours > effectiveWatchMaxHours(planLimits.maxHours)) { | ||
| return { | ||
| ok: false, | ||
| code: "watch_limit_reached", | ||
| error: hint("That watch window is longer than your plan allows."), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Questions that could be answered instantly are refused with an upgrade nag on limited plans
The requested watch window is rejected against the plan allowance (spec.maxHours > effectiveWatchMaxHours(...) at apps/webapp/app/services/dashboardAgentWatches.server.ts:342) before the instant check that can answer in the same turn, so a user on a limited plan gets an upgrade prompt instead of the answer they asked for, even though nothing would have been created.
Impact: On plans with a shorter allowed window, asking about something that already happened returns "That watch window is longer than your plan allows" rather than the immediate result.
Why the window check needs the same placement as the count check
createDashboardAgentWatch runs checkWatch right after the window check (apps/webapp/app/services/dashboardAgentWatches.server.ts:355-362); when it returns satisfied or terminal_unsatisfied the function returns watching: false and persists nothing at all — no row, no expiry, no polling. The org watcher-count floor is deliberately evaluated only after this point (apps/webapp/app/services/dashboardAgentWatches.server.ts:364-374, comment: "a one-shot creates no row and so consumes no watcher slot"). The same reasoning applies to the window floor: a one-shot never runs for any window, so refusing it on window length denies an answer that costs the plan nothing. Since the model picks maxHours (up to the code ceiling of 24) without knowing the plan, this will fire routinely for e.g. run_finished on an already-finished run.
Prompt for agents
In createDashboardAgentWatch (apps/webapp/app/services/dashboardAgentWatches.server.ts), the plan window floor is enforced before the immediate checkWatch call, while the org watcher-count floor is deliberately enforced after it because a one-shot creates no row. A one-shot equally never occupies the requested window, so refusing it on window length denies a free answer. Consider moving the maxHours plan check to sit alongside the watcher-count check (after the `satisfied` / `terminal_unsatisfied` early return), so plan floors only apply when a persistent watch is actually about to be created. Keep the resolveLimits call where it is (or move it too) so it is only paid once.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
What & why. Watches now honour a plan's watch limits. A watch whose window exceeds the plan's
agentWatchMaxHours, or that would push the org past itsagentWatcherscount, is refused with a newwatch_limit_reachedresult and an upgrade hint (a chat line on the card, HTTP 409 on the API).Key decisions.
min(plan, WATCH_MAX_HOURS=24)for the window, and the per-chat cap of 3 still applies independently. Plans only tighten, never loosen.isBillingConfigured().TRI-12863