feat: multi-user webhook triggers#3037
Conversation
🦋 Changeset detectedLatest commit: ac325fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — A single webhook trigger can now be associated with multiple users. When the webhook fires, it fans out one execution per user (with optional stagger delay), creating separate invocation records and conversations for each. This replaces the old UI pattern of bulk-creating N duplicate triggers for N users, bringing webhook triggers to parity with the multi-user pattern already established for scheduled triggers. Key changes
Summary ｜ 35 files ｜ 1 commit ｜ base: New
|
| Scenario | Behavior |
|---|---|
| Join table has N users | Fan-out to N executions |
Join table empty, legacy runAsUserId set |
Single execution (backward compat) |
| Neither | Single execution without user context |
TriggerService.ts · webhooks.ts
Sub-resource API and updated CRUD for trigger users
Before: User association was a single
runAsUserIdstring on the trigger row.
After: Four new sub-resource endpoints (GET/PUT/POST/DELETEon/{id}/users) manage the join table directly. Create and update routes accept arunAsUserIdsarray (mutually exclusive with deprecatedrunAsUserId). Responses now includerunAsUserIdsanduserCountfields.
The rerun endpoint (POST /{id}/rerun) now requires an explicit runAsUserId in the body for multi-user triggers (returns 400 otherwise). Single-user and no-user triggers retain existing behavior. A validateRunNowDelegation helper enforces that non-admins can only rerun as themselves.
triggers.ts (routes) · schemas.ts
Data access layer for trigger_users
Before: No DAL functions for the join table.
After: New DAL functions:getTriggerUsers,getTriggerUsersBatch,createTriggerUser,deleteTriggerUser,setTriggerUsers,getTriggerUserCount,getWebhookTriggerIdsWithUser,removeUserFromProjectTriggerUsers, andcreateTriggerWithUsers(transactional create + user insert).
getTriggerUsersBatch efficiently loads users for a list of trigger IDs in one query (used by the list endpoint). removeUserFromProjectTriggerUsers handles the cascading auto-disable when the last user is removed.
triggers.ts (DAL) · triggerUsers.test.ts
User lifecycle cleanup on project/org removal
Before: Removing a user from a project did not clean up webhook trigger associations (gap noted in the spec).
After: The project member DELETE handler now callsremoveUserFromProjectTriggerUsers(), and the org-levelcleanupUserTriggers()also removestrigger_usersrows alongside the existing legacy cleanup.
Triggers left with zero users after removal are auto-disabled (enabled = false).
projectMembers.ts · triggerCleanup.ts · triggerCleanup.test.ts
UI: single-trigger multi-user form and batch-grouped invocations
Before: The trigger form bulk-created N separate triggers for N users; the invocations table displayed a flat list.
After: The form sends a singlerunAsUserIdsarray with adispatchDelayMsfield. The triggers table shows a "N users" badge with tooltip. The invocations table groups entries bybatchIdwith collapsible rows, auto-polls for pending statuses, and shows per-invocationrunAsUserId.
The "Duplicate" action was removed from the triggers table since multi-user triggers make it unnecessary. The "Run As" column header was renamed to "Execution Identity" in the form.
trigger-form.tsx · project-triggers-table.tsx · invocations-table.tsx · actions/triggers.ts · api/triggers.ts
Observability and documentation
Before: No OTel attributes for batch correlation or per-user trigger identity.
After: New span attributestrigger.run_as_user_idandtrigger.batch_idare set on execution spans, with corresponding constants inSPAN_KEYS.
Documentation updated to reflect multi-user semantics: "Run As User" → "Run As Users", dispatch delay field documented, processing flow updated to mention invocation IDs in the 202 response.
otel-attributes.ts · webhooks.mdx · SPEC.md
Tests updated for multi-user semantics
Before: Tests asserted single-user
{ invocationId, conversationId }responses andassertCanMutateTriggercalls.
After: Tests createtrigger_usersrows, assert the{ invocations: [...] }array response shape, verify 400 for multi-user reruns withoutrunAsUserId, and validate the fan-out dispatch with correct per-user arguments.
triggers.test.ts · webhooks.test.ts
Claude Opus ｜ 𝕏
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured feature that closely follows the established scheduled trigger multi-user pattern and the accompanying spec. The data model, API design, auth rules, and user lifecycle cleanup are all consistent. A few items need attention — one correctness issue in the fan-out that could swallow total failures, and a few lower-priority cleanups.
Claude Opus ｜ 𝕏
| }) | ||
| ) | ||
| ).filter((invocation): invocation is NonNullable<typeof invocation> => invocation !== null); | ||
|
|
There was a problem hiding this comment.
Bug (medium): If every dispatchExecution call throws, invocations ends up as an empty array and the webhook returns { success: true, invocations: [] }. A 202 with zero accepted invocations is misleading — callers have no signal that nothing was dispatched.
Consider returning a failure response (e.g. 500 or the existing { success: false, error, status } shape) when invocations.length === 0 && executionUsers.length > 0.
| const invocations = ( | ||
| await Promise.all( | ||
| executionUsers.map(async (runAsUserId, index) => { | ||
| try { | ||
| const { invocationId, conversationId } = await dispatchExecution({ | ||
| tenantId, | ||
| projectId, | ||
| agentId, | ||
| triggerId, | ||
| resolvedRef, | ||
| payload, | ||
| transformedPayload, | ||
| messageParts, | ||
| userMessageText, | ||
| runAsUserId: runAsUserId ?? undefined, | ||
| batchId, | ||
| delayBeforeExecutionMs: (trigger.dispatchDelayMs ?? 0) * index, | ||
| }); | ||
|
|
||
| return { | ||
| invocationId, | ||
| conversationId, | ||
| runAsUserId, | ||
| }; | ||
| } catch (error) { | ||
| logger.error( | ||
| { | ||
| tenantId, | ||
| projectId, | ||
| agentId, | ||
| triggerId, | ||
| runAsUserId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }, | ||
| 'Failed to dispatch trigger execution for user' | ||
| ); | ||
| return null; | ||
| } | ||
| }) | ||
| ) | ||
| ).filter((invocation): invocation is NonNullable<typeof invocation> => invocation !== null); |
There was a problem hiding this comment.
Nit: Promise.all starts all dispatch calls concurrently, so the dispatchDelayMs * index delay is purely in-promise via setTimeout. This is correct for the stagger semantics (each promise sleeps before executing), but it means all invocation records are created nearly simultaneously in the DB. Worth confirming that's the intended behaviour — the spec says "each execution promise sleeps position × dispatchDelayMs before starting", which matches what happens here.
| forwardedHeaders: params.forwardedHeaders, | ||
| }); | ||
|
|
||
| //Note: This is a best-effor implementation. If use-cases require a large amount of users to be executed, this should be replaced with a queue/workflow-backed design. |
There was a problem hiding this comment.
Typo: best-effor → best-effort.
| {status} | ||
| </Badge> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistency: The single-user code path (line ~472) uses this local StatusBadge with minimal styling, while the multi-user code path uses getInvocationStatusBadge() from @/lib/utils/invocation-display which includes icons and richer badge variants. Consider replacing StatusBadge with getInvocationStatusBadge in the single-user path too for a consistent look.
| if (existing.runAsUserId && existingRunAsUserIds.length === 1) { | ||
| await setTriggerUsers(tx)({ | ||
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId: id, | ||
| userIds: existingRunAsUserIds, | ||
| }); |
There was a problem hiding this comment.
Subtle edge case: When existing.runAsUserId is set and this is the first POST /{id}/users call, this block migrates the legacy scalar user into the join table. However, if the userId being added is the same as existing.runAsUserId, setTriggerUsers writes it, then createTriggerUser with onConflictDoNothing is a no-op — net result is correct (1 row in join table). Just confirming this was intentional.
A second POST with a different user will skip this block (because existingRunAsUserIds.length will already be > 1 from the first call having populated the join table), which is also correct. Looks fine.
| triggerId: varchar('trigger_id', { length: 256 }).notNull(), | ||
| conversationId: varchar('conversation_id', { length: 256 }), | ||
| runAsUserId: varchar('run_as_user_id', { length: 256 }), | ||
| batchId: varchar('batch_id', { length: 256 }), |
There was a problem hiding this comment.
The batchId column is not mentioned in the spec (which only describes runAsUserId on trigger_invocations and the response shape). Adding it is pragmatic for the grouped UI view, but it's a new runtime DB column that lives beyond the spec — worth noting in the changeset or PR description.
| if (existing.runAsUserId && existingRunAsUserIds.length === 1) { | ||
| await setTriggerUsers(tx)({ | ||
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId: id, | ||
| userIds: existingRunAsUserIds, | ||
| }); |
There was a problem hiding this comment.
Same legacy migration block as POST /{id}/users — when the only user in the trigger is the legacy scalar runAsUserId, this migrates it to the join table before deleting the requested user. If the user being deleted is the legacy user, setTriggerUsers writes 1 row, then deleteTriggerUser removes it, and the trigger is auto-disabled. Correct, but the transient write-then-delete is a little wasteful.
Consider skipping the setTriggerUsers call if userId === existingRunAsUserIds[0] and existingRunAsUserIds.length === 1 — the trigger will be disabled either way.
| ...body, | ||
| authentication: hashedAuthentication as any, | ||
| signatureVerification: body.signatureVerification as any, | ||
| ...(runAsUserId !== undefined || runAsUserIds !== undefined ? { runAsUserId: null } : {}), |
There was a problem hiding this comment.
When runAsUserIds is provided on update, runAsUserId is unconditionally set to null. When only runAsUserId is provided (without runAsUserIds), it's also set to null. This is correct — the join table is now the source of truth and the scalar column is cleared. Good.
| if (prevInitial !== initialInvocations) { | ||
| setPrevInitial(initialInvocations); | ||
| setInvocations(initialInvocations); | ||
| } |
There was a problem hiding this comment.
React pattern concern: Comparing prevInitial !== initialInvocations by reference during render to sync derived state is a known-tricky pattern. It works but can cause unnecessary setState calls if the parent re-renders with a new array reference that has identical content (e.g. RSC re-render). Since there's polling via useEffect, this is probably fine in practice, but consider using a key or useEffect for the sync instead.
| try { | ||
| const db = c.get('db'); | ||
| await removeUserFromProjectTriggerUsers(db)({ | ||
| tenantId, | ||
| projectId, | ||
| userId, | ||
| }); | ||
| } catch (err) { | ||
| logger.error( | ||
| { tenantId, projectId, userId, error: err }, | ||
| 'Failed to clean up user from webhook triggers after project removal' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Good: the error is caught and logged rather than failing the member removal. One note — this runs outside withRef(), so db here is the request-scoped manage DB client. The removeUserFromProjectTriggerUsers DAL function operates on the same DB connection, which should work since the project member routes already have branch context set up. Just confirming this doesn't need an explicit withRef() wrapper (the scheduled trigger cleanup at line ~260 uses the runtime DB so doesn't need it).
|
TL;DR — A single webhook trigger can now be associated with multiple users. When the webhook fires, it fans out one execution per user (with optional stagger delay), creating separate invocation records and conversations for each. This replaces the old UI pattern of bulk-creating N duplicate triggers for N users, bringing webhook triggers to parity with the multi-user pattern already established for scheduled triggers. Key changes
Summary ｜ 37 files ｜ 12 commits ｜ base: New
|
| Scenario | Behavior |
|---|---|
| Join table has N users | Fan-out to N executions |
Join table empty, legacy runAsUserId set |
Single execution (backward compat) |
| Neither | Single execution without user context |
| All dispatches fail | 500 error response |
TriggerService.ts · webhooks.ts
Sub-resource API and updated CRUD for trigger users
Before: User association was a single
runAsUserIdstring on the trigger row.
After: Four new sub-resource endpoints (GET/PUT/POST/DELETEon/{id}/users) manage the join table directly. Create and update routes accept arunAsUserIdsarray (mutually exclusive with deprecatedrunAsUserId). Responses now includerunAsUserIdsanduserCountfields.
The rerun endpoint (POST /{id}/rerun) now requires an explicit runAsUserId in the body for multi-user triggers (returns 400 otherwise), checks canUseProjectStrict to verify the caller still has project access, and runs validateRunNowDelegation to ensure non-admins can only rerun as themselves. Single-user and no-user triggers retain existing behavior.
triggers.ts (routes) · schemas.ts
Data access layer for trigger_users
Before: No DAL functions for the join table.
After: New DAL functions:getTriggerUsers,getTriggerUsersBatch,createTriggerUser,deleteTriggerUser,setTriggerUsers,getTriggerUserCount,getWebhookTriggerIdsWithUser,removeUserFromProjectTriggerUsers, andcreateTriggerWithUsers(transactional create + user insert).
setTriggerUsers expects caller-provided transaction context rather than managing its own transaction, allowing it to compose cleanly with other operations (e.g. clearing legacy runAsUserId atomically). getTriggerUsersBatch efficiently loads users for a list of trigger IDs in one query (used by the list endpoint). removeUserFromProjectTriggerUsers handles the cascading cleanup when users are removed from a project.
triggers.ts (DAL) · triggerUsers.test.ts
Full agent push/pull supports runAsUserIds
Before:
createFullAgentServerSideandupdateFullAgentServerSidespread trigger data directly into the upsert;getFullAgentDefinitiondid not includerunAsUserIdsordispatchDelayMs.
After: Both create and update paths extractrunAsUserIdsfrom trigger data before upserting the trigger row, then callsetTriggerUsersto sync the join table. The full agent definition response now includesrunAsUserIds(loaded viagetTriggerUsersBatch) anddispatchDelayMsfor each trigger.
This ensures the SDK-driven full agent push (PUT /agents-full/{agentId}) correctly round-trips multi-user trigger configuration, keeping parity with the REST sub-resource API.
keepExisting flag for authentication header updates
Before: Updating a trigger's authentication headers required resending the plaintext secret value every time, even if it hadn't changed.
After:TriggerAuthenticationUpdateSchema(distinct from the create-timeTriggerAuthenticationInputSchema) allows each header to setkeepExisting: true, preserving the existing hashed value without requiring the plaintext secret to be retransmitted.
This prevents accidental secret loss during partial trigger updates and aligns the update path with the stored hashed representation.
User lifecycle cleanup on project/org removal
Before: Removing a user from a project did not clean up webhook trigger associations.
After: The project member DELETE handler now callsremoveUserFromProjectTriggerUsers(), and the org-levelcleanupUserTriggers()also removestrigger_usersrows alongside the existing legacy cleanup.
Triggers are not auto-disabled when left with zero users — they remain enabled and will execute without user context (matching the behavior of triggers that never had users assigned).
projectMembers.ts · triggerCleanup.ts · triggerCleanup.test.ts
UI: single-trigger multi-user form and batch-grouped invocations
Before: The trigger form bulk-created N separate triggers for N users; the invocations table displayed a flat list.
After: The form sends a singlerunAsUserIdsarray with adispatchDelayMsfield. The triggers table shows a "N users" badge with tooltip. The invocations table groups entries bybatchIdwith collapsible rows, auto-polls for pending statuses, and shows per-invocationrunAsUserId.
The "Duplicate" action was removed from the triggers table since multi-user triggers make it unnecessary. The form preserves dispatchDelayMs: 0 correctly (no longer coerced to undefined by falsy check).
trigger-form.tsx · project-triggers-table.tsx · invocations-table.tsx · api/triggers.ts
Observability and documentation
Before: No OTel attributes for batch correlation or per-user trigger identity.
After: New span attributestrigger.run_as_user_idandtrigger.batch_idare set on execution spans, with corresponding constants inSPAN_KEYS.
Documentation and spec updated to reflect multi-user semantics: "Run As User" to "Run As Users", dispatch delay field documented, processing flow updated to mention invocation IDs in the 202 response. The spec now explicitly states that triggers remain enabled even with an empty user set (no auto-disable on user removal).
otel-attributes.ts · webhooks.mdx · SPEC.md
Tests for multi-user sub-resource API and fan-out dispatch
Before: No tests for sub-resource endpoints; webhook tests asserted single-user
{ invocationId, conversationId }responses.
After: Full integration test suites for all four trigger users sub-resource endpoints (GET/PUT/POST/DELETE) covering CRUD, legacy migration, authorization (includingcanUseProjectStrictandvalidateRunAsUserIdsmocks), and edge cases. Webhook tests verify multi-user fan-out, partial failure handling, and total failure (500) scenarios.
The test harness now uses mockReset() (instead of mockClear()) for cleaner test isolation, resetting both call history and implementation.
triggers.test.ts · webhooks.test.ts
Claude Opus ｜ 𝕏
miles-kt-inkeep
force-pushed
the
feat/multi-user-webhooks
branch
from
9b2def5 to
d4d040c
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
TriggerService.ts:201Fan-out returns{ success: true, invocations: [] }when all dispatches fail
🟠⚠️ Major (3) 🟠⚠️
🟠 1) scope Missing test coverage for multi-user fan-out scenarios
Issue: The webhooks.test.ts mocks getTriggerUsersMock to return an empty array by default but never tests the multi-user scenario where getTriggerUsers returns multiple users. The Promise.all fan-out logic has no test coverage for partial failures or all-fail scenarios.
Why: If the fan-out logic has bugs in error handling or result aggregation, these issues would go undetected. The critical bug (success with empty invocations) exists partly because there's no test asserting the expected behavior.
Fix: Add tests for:
'should fan out to multiple users when trigger has multiple trigger_users'— verify invocations array has N entries'should return success with partial invocations when some dispatches fail'— mock one dispatch to throw'should return failure when all dispatches fail'— verify 500 response, not 202
Refs:
🟠 2) scope Missing integration tests for sub-resource endpoints
Issue: No integration tests for the new sub-resource endpoints: GET /{id}/users, PUT /{id}/users, POST /{id}/users, DELETE /{id}/users/{userId}. These endpoints (lines 800-1166) include complex authorization logic (assertCanMutateTrigger), validation (validateRunAsUserIds), and transactional operations.
Why: Authorization bypass risks if assertCanMutateTrigger is incorrectly implemented. PUT /{id}/users with empty array disables the trigger (line 946), but this auto-disable behavior is untested.
Fix: Add integration tests covering:
- Each CRUD operation on
/{id}/users - Legacy
runAsUserId→trigger_usersmigration when first user added - Auto-disable when last user removed
- Authorization: non-admin cannot modify other users' trigger associations
Refs:
Inline Comments:
- 🟠 Major:
triggers.ts:241Nested transaction insetTriggerUsersmay cause unexpected behavior - 🟠 Major:
webhooks.ts:52-62Breaking API response shape change needs migration path
🟡 Minor (2) 🟡
🟡 1) triggers.ts:1410 Rerun endpoint doesn't verify caller's own project access
Issue: validateRunNowDelegation checks non-admins can only rerun as themselves, but doesn't verify the caller still has 'use' permission on the project. A user who lost project access could potentially invoke rerun if they have a valid session.
Why: The runtime executeAgentAsync does check canUseProjectStrict for runAsUserId, but not for the caller's orchestration access. Defense-in-depth gap.
Fix: Before validateRunNowDelegation, verify caller has project 'use' permission via canUseProjectStrict({ userId: callerId, tenantId, projectId }).
Refs:
- TriggerService.ts:820-876 — runtime check for runAsUserId
Inline Comments:
- 🟡 Minor:
TriggerService.ts:191Error logging loses stack trace
💭 Consider (2) 💭
💭 1) triggers.ts:136-150 Include user IDs in delegation error message
Issue: validateRunNowDelegation throws generic "Only org admins..." message without context about which user IDs were involved.
Fix: Include runAsUserId and callerId in error message for easier debugging.
💭 2) triggers.ts:206-222 createTriggerUser return value ambiguity
Issue: createTriggerUser uses onConflictDoNothing().returning() and returns result[0], which is undefined if user already exists. Caller at line 1041 doesn't check the return value.
Fix: Document that undefined return means "already existed" or return discriminated result { created: true/false }.
🕐 Pending Recommendations (2)
- 🟠
TriggerService.ts:200Fan-out swallowing total failures (pullfrog) — addressed by Critical finding above - 🟡
invocations-table.tsx:554StatusBadge inconsistency between single/multi-user paths (pullfrog)
🚫 REQUEST CHANGES
Summary: Well-structured feature that follows the established scheduled trigger multi-user pattern. The core data model, API design, and auth rules are consistent with the spec. However, the critical issue where webhook returns success even when all dispatches fail must be fixed before merge — this could cause silent data loss for webhook callers. Additionally, the breaking API response shape change needs explicit migration guidance, and the lack of test coverage for the new fan-out logic creates risk for future regressions.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
manage-schema.ts:170 |
Join table in manage DB vs runtime DB asymmetry | Documented design choice per spec Decision 1 — architecturally appropriate |
TriggerService.ts:175 |
Stagger delay timing semantics | Consistent with scheduled trigger pattern, appropriately documented as best-effort |
runtime-schema.ts:244 |
batchId column not in original spec | Pragmatic addition for UI grouping — spec is not exhaustive |
projectMembers.ts:273 |
Cleanup catches errors instead of failing | Matches scheduled trigger pattern, acceptable for eventual consistency |
TriggerApiInsertSchema |
.refine() changes schema type |
Semantically correct validation, OpenAPI impact is minor |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-security-iam |
2 | 1 | 0 | 0 | 0 | 0 | 1 |
pr-review-breaking-changes |
6 | 0 | 0 | 0 | 1 | 0 | 5 |
pr-review-tests |
6 | 2 | 0 | 0 | 0 | 0 | 4 |
pr-review-errors |
5 | 0 | 2 | 0 | 2 | 0 | 1 |
pr-review-architecture |
7 | 0 | 0 | 0 | 0 | 0 | 7 |
| Total | 27 | 3 | 2 | 0 | 4 | 0 | 18 |
Note: Multiple reviewers flagged the same total-failure issue (deduplicated to single Critical finding). Architecture review confirmed design decisions align with spec.
| ).filter((invocation): invocation is NonNullable<typeof invocation> => invocation !== null); | ||
|
|
||
| return { success: true, invocationId, conversationId }; | ||
| return { success: true, invocations }; |
There was a problem hiding this comment.
🔴 CRITICAL: Fan-out returns success with empty invocations when all dispatches fail
Issue: When all user dispatches fail in the Promise.all fan-out (lines 159-199), errors are caught and logged but filtered out, resulting in { success: true, invocations: [] }. The webhook then returns HTTP 202 with success: true even though zero invocations were created.
Why: This misleads webhook callers into thinking their request was accepted when it completely failed. Callers have no signal that nothing was dispatched. This masks infrastructure failures (DB outage, etc.) from external systems that depend on webhook acknowledgment semantics.
Fix:
| return { success: true, invocations }; | |
| ).filter((invocation): invocation is NonNullable<typeof invocation> => invocation !== null); | |
| if (invocations.length === 0 && executionUsers.length > 0) { | |
| return { success: false, error: 'All trigger executions failed to dispatch', status: 500 }; | |
| } | |
| return { success: true, invocations }; |
Refs:
- SPEC.md Decision 12 — states "202 = accepted" for returned invocations but is silent on total-failure behavior
- Also flagged by pullfrog review
| agentId, | ||
| triggerId, | ||
| runAsUserId, | ||
| error: error instanceof Error ? error.message : String(error), |
There was a problem hiding this comment.
🟡 Minor: Error logging loses stack trace
Issue: The catch block logs error.message only, losing the stack trace which is critical for debugging where in dispatchExecution the failure occurred.
Why: When debugging production dispatch failures, engineers will only see messages like "Connection timeout" without knowing whether the failure was in createTriggerInvocation, the execution promise setup, or elsewhere.
Fix:
| error: error instanceof Error ? error.message : String(error), | |
| logger.error( | |
| { | |
| tenantId, | |
| projectId, | |
| agentId, | |
| triggerId, | |
| runAsUserId, | |
| error: error instanceof Error ? { message: error.message, stack: error.stack } : String(error), | |
| }, | |
| 'Failed to dispatch trigger execution for user' | |
| ); |
| export const setTriggerUsers = | ||
| (db: AgentsManageDatabaseClient) => | ||
| async (params: { scopes: AgentScopeConfig; triggerId: string; userIds: string[] }) => { | ||
| await db.transaction(async (tx) => { |
There was a problem hiding this comment.
🟠 MAJOR: Nested transaction may cause unexpected behavior
Issue: setTriggerUsers starts its own transaction internally (line 241), but it's called from within an existing transaction in triggers.ts at lines 667-671 and 934-949. Drizzle's PostgreSQL driver does not support true nested transactions (savepoints) by default.
Why: When setTriggerUsers(tx) is called with a transaction client and then calls tx.transaction(), the behavior depends on driver configuration and may either: (1) create a nested savepoint, (2) throw an error, or (3) reuse the outer transaction ignoring the inner boundary. A failure in updateTrigger after setTriggerUsers completes could leave trigger_users rows committed while the trigger update is rolled back.
Fix: Remove the internal transaction since callers already provide transaction context:
| await db.transaction(async (tx) => { | |
| async (params: { scopes: AgentScopeConfig; triggerId: string; userIds: string[] }) => { | |
| // Expects caller to provide transaction context | |
| await db | |
| .delete(triggerUsers) | |
| .where( | |
| and( | |
| agentScopedWhere(triggerUsers, params.scopes), | |
| eq(triggerUsers.triggerId, params.triggerId) | |
| ) | |
| ); | |
| if (params.userIds.length > 0) { | |
| await db.insert(triggerUsers).values( | |
| params.userIds.map((userId) => ({ | |
| tenantId: params.scopes.tenantId, | |
| projectId: params.scopes.projectId, | |
| agentId: params.scopes.agentId, | |
| triggerId: params.triggerId, | |
| userId, | |
| })) | |
| ); | |
| } | |
| }; |
Refs:
- triggers.ts:667-671 — caller already wraps in transaction
| 'application/json': { | ||
| schema: z.object({ | ||
| success: z.boolean(), | ||
| invocationId: z.string(), | ||
| conversationId: z.string(), | ||
| invocations: z.array( | ||
| z.object({ | ||
| invocationId: z.string(), | ||
| conversationId: z.string(), | ||
| runAsUserId: z.string().nullable(), | ||
| }) | ||
| ), | ||
| }), |
There was a problem hiding this comment.
🟠 MAJOR: Breaking API response shape change
Issue: The webhook response schema changes from { success, invocationId, conversationId } to { success, invocations: [{ invocationId, conversationId, runAsUserId }] }. This is a breaking change for any API consumer parsing the webhook response.
Why: Existing webhook callers that expect response.invocationId or response.conversationId will receive undefined after this change. SDK consumers, external integrations, and internal tooling using the webhook endpoint will break silently (no runtime error, just missing data).
Fix: Consider one of:
- API versioning (e.g.,
/run/v2/webhooks/...) to preserve backward compatibility - Transition period with deprecated top-level fields:
{ success, invocationId: invocations[0]?.invocationId, // deprecated conversationId: invocations[0]?.conversationId, // deprecated invocations: [...] }
- Clear migration documentation in changelog and SDK release notes
Refs:
- SPEC.md Decision 4, 10 — documents this as intentional breaking change
- product-surface-areas skill — identifies Webhook Format as customer-facing contract
Ito Test Report ❌13 test cases ran. 1 failed, 12 passed. Across the unified run, 13 executable test cases were completed with 12 passing and 1 failing (plus two reports that produced no executable cases), so the overall outcome is failed due to a single confirmed production defect. The key finding is a medium-severity regression where Dispatch Delay (ms) = 0 is accepted by validation but not persisted because the UI submit payload drops zero via a falsy check (despite backend support, and likely introduced by this PR), while all other verified behaviors worked as expected: multi-user webhook create/edit and legacy migration persistence, mobile and navigation resilience, webhook 202 contract and unauthenticated acceptance when auth/signature are intentionally unset, proper 400/422 invalid-input handling with no invocations, rerun identity enforcement/forwarding with rapid-click guards, and membership-revocation cleanup that disables empty triggers and blocks stale reruns. ❌ Failed (1)
🟠 Dispatch delay numeric boundaries
Relevant code:
const payload: any = {
id: data.id,
name: data.name,
description: data.description || undefined,
enabled: data.enabled,
dispatchDelayMs: data.dispatchDelayMs || undefined,
// Send null to explicitly clear messageTemplate, undefined to keep existing
};
runAsUserIds: z.array(z.string()).default([]),
dispatchDelayMs: z.coerce.number().int().min(0).max(600_000).optional(),
});
signatureVerification: body.signatureVerification as any,
...(runAsUserId !== undefined || runAsUserIds !== undefined ? { runAsUserId: null } : {}),
...(body.dispatchDelayMs !== undefined ? { dispatchDelayMs: body.dispatchDelayMs } : {}),
},✅ Passed (12)
Commit: Tell us how we did: Give Ito Feedback |
miles-kt-inkeep
force-pushed
the
feat/multi-user-webhooks
branch
from
527baa7 to
ddede7f
Compare
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
trigger-form.tsx:741dispatchDelayMs: 0gets converted toundefineddue to falsy check
🟠⚠️ Major (5) 🟠⚠️
🟠 1) scope Missing test coverage for multi-user fan-out scenarios
Issue: The webhooks.test.ts mocks getTriggerUsersMock to return an empty array by default but never tests the multi-user scenario where getTriggerUsers returns multiple users. The Promise.all fan-out logic has no test coverage for partial failures or all-fail scenarios.
Why: If the fan-out logic has bugs in error handling or result aggregation, these issues would go undetected. The partial failure handling (returning success with partial invocations) is a key business requirement that lacks verification.
Fix: Add tests for:
'should fan out to multiple users when trigger has multiple trigger_users'— verify invocations array has N entries'should return success with partial invocations when some dispatches fail'— mock one dispatch to throw'should return 500 when all dispatches fail'— verify error response, not 202
Refs:
🟠 2) scope Missing integration tests for sub-resource endpoints
Issue: No integration tests for the new sub-resource endpoints: GET /{id}/users, PUT /{id}/users, POST /{id}/users, DELETE /{id}/users/{userId}. These endpoints (lines 800-1166) include complex authorization logic (assertCanMutateTrigger), validation (validateRunAsUserIds), and transactional operations.
Why: Authorization bypass risks if assertCanMutateTrigger is incorrectly implemented. PUT /{id}/users with empty array disables the trigger (line 946), but this auto-disable behavior is untested.
Fix: Add integration tests covering:
- Each CRUD operation for /{id}/users
- Authorization rejection when caller lacks permission
- Auto-disable when user list becomes empty
- Validation rejection for invalid user IDs
Refs:
Inline Comments:
- 🟠 Major:
invocations-table.tsx:348Clickable table rows lack keyboard accessibility (onKeyDown,tabIndex,role) - 🟠 Major:
TriggerService.ts:204Partial dispatch failures swallowed — caller only sees successful invocations
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
TriggerService.ts:175Unbounded delay accumulation (dispatchDelayMs * index) could cause very long waits with many users - 🟡 Minor:
schemas.ts:829runAsUserIdsarray lacks duplicate validation — duplicates silently create redundant executions
🕐 Pending Recommendations (2)
- 🟠 Missing test coverage for multi-user fan-out — raised in prior review, still unaddressed
- 🟠 Missing integration tests for sub-resource endpoints — raised in prior review, still unaddressed
🚫 REQUEST CHANGES
Summary: The multi-user webhook trigger implementation is well-structured and follows established patterns. However, there's a critical bug in the UI form where dispatchDelayMs: 0 is converted to undefined due to a falsy check (data.dispatchDelayMs || undefined), breaking the ability to set zero delay. This was confirmed by Ito test report. Additionally, the test coverage gaps for the fan-out logic and sub-resource endpoints (raised in the prior review) remain unaddressed. The partial failure handling in processWebhook silently swallows errors — callers have no visibility into which dispatches failed.
Discarded (12)
| Location | Issue | Reason Discarded |
|---|---|---|
TriggerService.ts:204-210 |
Fan-out returns success with empty invocations when all fail | Addressed in latest commit — now returns 500 error |
triggers.ts:setTriggerUsers |
Nested transaction in update handler | Addressed — setTriggerUsers no longer wraps in internal transaction |
triggers.ts:1426-1432 |
Rerun endpoint doesn't verify caller's project access | Addressed — added permission check |
manage-schema.ts |
Migration adds nullable columns | Safe — additive change with defaults |
runtime-schema.ts |
Migration adds nullable column | Safe — additive change |
triggers.ts |
Breaking API response shape | Already documented in prior review, spec acknowledges this |
trigger-form.tsx:152 |
useSearchParams without Suspense | Pre-existing pattern in codebase, not introduced by this PR |
invocations-table.tsx:152 |
useSearchParams without Suspense | Pre-existing pattern in codebase |
trigger-form.tsx |
Missing aria-describedby on form fields | Pre-existing pattern, not introduced by this PR |
schemas.ts |
Legacy field validation | Zod coerce handles the edge cases adequately |
TriggerService.ts |
User ID exposure in logs | Only logs to internal telemetry, not user-facing |
triggers.ts |
Auto-disable behavior undocumented | Behavior is correct per spec, documentation is separate concern |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
10 | 0 | 0 | 0 | 1 | 0 | 9 |
pr-review-tests |
7 | 2 | 0 | 0 | 0 | 2 | 3 |
pr-review-breaking-changes |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-standards |
4 | 0 | 0 | 0 | 3 | 0 | 1 |
pr-review-security-iam |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-errors |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
| Total | 33 | 2 | 0 | 0 | 5 | 2 | 24 |
Note: Critical bug (dispatchDelayMs=0) was identified from Ito test report, not sub-agent findings.
| name: data.name, | ||
| description: data.description || undefined, | ||
| enabled: data.enabled, | ||
| dispatchDelayMs: data.dispatchDelayMs || undefined, |
There was a problem hiding this comment.
🔴 CRITICAL: dispatchDelayMs = 0 is converted to undefined, not persisted
Issue: The falsy check data.dispatchDelayMs || undefined treats 0 as falsy, so explicitly setting dispatch delay to 0ms results in the field being omitted from the payload.
Why: Users who want immediate per-user fan-out (0ms delay) cannot configure this. The Ito tests confirmed this bug — form accepts 0, but saved trigger does not retain 0.
Fix: (1-click apply)
| dispatchDelayMs: data.dispatchDelayMs || undefined, | |
| dispatchDelayMs: data.dispatchDelayMs !== undefined ? data.dispatchDelayMs : undefined, |
Refs:
- Ito test report — confirmed regression
|
|
||
| return ( | ||
| <Fragment key={group.key}> | ||
| <tr |
There was a problem hiding this comment.
🟠 MAJOR: Accessibility — clickable table rows lack keyboard support
Issue: Table rows with onClick handlers are not focusable and do not respond to keyboard events. Screen reader users and keyboard-only users cannot expand/collapse grouped invocations.
Why: Violates WCAG 2.1 SC 2.1.1 (Keyboard) at Level A — interactive elements must be keyboard accessible.
Fix: Add keyboard support and proper ARIA attributes:
<tr
className="... cursor-pointer"
onClick={() => toggleGroup(group.key)}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
toggleGroup(group.key);
}
}}
tabIndex={0}
role="button"
aria-expanded={isExpanded}
>Or move the click handler to a proper <button> element within the expand cell.
| ) | ||
| ).filter((invocation): invocation is NonNullable<typeof invocation> => invocation !== null); | ||
|
|
||
| if (invocations.length === 0) { |
There was a problem hiding this comment.
🟠 MAJOR: Partial dispatch failures provide no indication of which users failed
Issue: When some (but not all) dispatchExecution calls fail, the function returns { success: true, invocations } with only successful invocations. There's no indication to the webhook caller which users failed or how many.
Why: A webhook caller triggering a 10-user fan-out where 3 users fail receives a 202 with 7 invocations and has no way to know 3 failed. They cannot retry just those users, and failures are only discoverable in logs.
Fix: Include partial failure information in the response:
// Track failures alongside successes
const failures: Array<{ runAsUserId: string | null; error: string }> = [];
const invocations = (await Promise.all(
executionUsers.map(async (runAsUserId, index) => {
try {
// ... existing dispatch logic
} catch (error) {
failures.push({ runAsUserId, error: error instanceof Error ? error.message : 'dispatch failed' });
return null;
}
})
)).filter(Boolean);
return {
success: invocations.length > 0,
invocations,
...(failures.length > 0 && { partialFailures: failures }),
};Refs:
- SPEC.md Decision 12 — states "202 = accepted" but is silent on partial-failure visibility
| userMessageText, | ||
| runAsUserId: runAsUserId ?? undefined, | ||
| batchId, | ||
| delayBeforeExecutionMs: (trigger.dispatchDelayMs ?? 0) * index, |
There was a problem hiding this comment.
🟡 Minor: Unbounded delay accumulation for large user lists
Issue: delayBeforeExecutionMs is calculated as (trigger.dispatchDelayMs ?? 0) * index. With max delay of 600,000ms (10 min) and N users, the last user's delay is 600,000 * (N-1) ms. For 10 users = 90 minutes; 100 users = 15 hours held in memory.
Why: The comment at line 628 acknowledges this is "best-effort". However, there's no cap on accumulated delay. Combined with high dispatchDelayMs, this could exhaust server resources (setTimeout handles, memory) or cause executions that outlive server restarts.
Fix: Consider adding a cap on total accumulated delay or user count:
const MAX_TOTAL_DELAY_MS = 600_000; // 10 minutes total
const MAX_USERS_PER_TRIGGER = 50;
if (executionUsers.length > MAX_USERS_PER_TRIGGER) {
return { success: false, error: `Trigger has ${executionUsers.length} users, max is ${MAX_USERS_PER_TRIGGER}`, status: 422 };
}
const totalDelay = (trigger.dispatchDelayMs ?? 0) * (executionUsers.length - 1);
if (totalDelay > MAX_TOTAL_DELAY_MS) {
return { success: false, error: `Total dispatch delay ${totalDelay}ms exceeds max ${MAX_TOTAL_DELAY_MS}ms`, status: 422 };
}| export const TriggerApiInsertSchema = createAgentScopedApiInsertSchema(TriggerInsertSchema) | ||
| .extend({ | ||
| id: ResourceIdSchema.optional(), | ||
| runAsUserIds: z |
There was a problem hiding this comment.
🟡 Minor: No validation for duplicate user IDs in runAsUserIds array
Issue: The schema accepts an array of strings without uniqueness validation. Duplicate user IDs would create duplicate entries via onConflictDoNothing (silent) but cause duplicate fan-out executions at runtime.
Why: At runtime (TriggerService.ts:154-155), executionUsers is constructed directly from runAsUserIds without deduplication. Duplicates would cause the same user to receive multiple invocations from a single webhook.
Fix: Add a Zod refinement or transform:
runAsUserIds: z.array(z.string()).optional()
.refine(
(ids) => !ids || new Set(ids).size === ids.length,
{ message: 'runAsUserIds must not contain duplicates' }
),Refs:
- TriggerService.ts:154-155 — fan-out uses array directly
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since the last automated review (commit ddede7ffd). The latest commit (d6856c389 — "feedback and lint") successfully addresses the critical and major issues raised in prior reviews.
✅ Issues Resolved in This Delta
| Prior Issue | Resolution |
|---|---|
🔴 Critical: dispatchDelayMs: 0 converted to undefined |
Fixed — Changed data.dispatchDelayMs || undefined → data.dispatchDelayMs !== undefined ? data.dispatchDelayMs : undefined at line 741 |
🟡 Minor: runAsUserIds array lacks duplicate validation |
Fixed — Added .refine() for uniqueness check on TriggerApiInsertSchema at lines 832-834 |
| 🟠 Major: Missing test coverage for multi-user fan-out | Addressed in prior commit — Tests exist at webhooks.test.ts:2036-2126 covering fan-out, partial failure, and total failure scenarios |
| 🟠 Major: Missing integration tests for sub-resource endpoints | Addressed in prior commit — Tests exist for GET/PUT/POST/DELETE /{id}/users at triggers.test.ts:1276-1714 |
| Lint: Non-null assertions in tests | Fixed — Changed ! to ?. for safer optional chaining (6 instances in triggers.test.ts) |
💭 Consider (1)
Inline Comments:
- 💭 Consider:
schemas.ts:850Missing duplicate validation in Update schema (consistency with Insert schema)
🕐 Pending Recommendations (2)
Issues from prior reviews that remain unaddressed but are low priority:
- 🟡
invocations-table.tsx:554— StatusBadge inconsistency between single/multi-user paths (pullfrog) — cosmetic - 🟡
TriggerService.ts:175— Unbounded delay accumulation for large user lists — documented as "best-effort" in spec
✅ APPROVE
Summary: The critical dispatchDelayMs: 0 bug (confirmed by Ito tests) has been correctly fixed with proper handling of falsy zero values. The duplicate validation refinement improves input hygiene. All major feedback from prior reviews (test coverage gaps, total-failure handling) was addressed in earlier commits. This PR is ready to merge.
The multi-user webhook trigger feature is well-structured, follows established patterns from scheduled triggers, and now has comprehensive test coverage for the fan-out logic, sub-resource endpoints, and edge cases.
Reviewers (0)
This was a delta review — no sub-agents dispatched. The delta contained only feedback fixes, not new feature code.
| runAsUserIds: z | ||
| .array(z.string()) | ||
| .optional() | ||
| .describe('Array of user IDs to run this trigger as (multi-user)'), |
There was a problem hiding this comment.
💭 Consider: Missing duplicate validation in Update schema
Issue: The duplicate validation refinement was added to TriggerApiInsertSchema (line 832-834) but not to TriggerApiUpdateSchema here. While updates through the sub-resource endpoints (PUT /{id}/users) use setTriggerUsers which handles duplicates at the DAL level, direct updates through PATCH /triggers/{id} with runAsUserIds could accept duplicates.
Why: Consistency — the same validation should apply to both create and update paths. The DAL's onConflictDoNothing handles DB-level duplicates, but explicit schema validation provides earlier, clearer error messages.
Fix:
export const TriggerApiUpdateSchema = createAgentScopedApiUpdateSchema(TriggerUpdateSchema)
.extend({
runAsUserIds: z
.array(z.string())
.optional()
.refine((ids) => !ids || new Set(ids).size === ids.length, {
message: 'runAsUserIds must not contain duplicates',
})
.describe('Array of user IDs to run this trigger as (multi-user)'),
})
// ... rest unchangedRefs:
- TriggerApiInsertSchema:832-834 — Insert schema has the refinement
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
schemas.ts:726Schema mismatch preventskeepExistingfrom working on PATCH trigger updates
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
triggers.ts:672Missing auto-disable when PATCH update setsrunAsUserIdsto empty array
🟡 Minor (0) 🟡
No minor issues identified in this delta review.
💭 Consider (0) 💭
No consider items in this delta review.
🕐 Pending Recommendations (2)
Issues from prior reviews that remain unaddressed but are low priority:
- 🟡
invocations-table.tsx:554— StatusBadge inconsistency between single/multi-user paths (pullfrog) — cosmetic - 🟡
invocations-table.tsx:348— Clickable table rows lack keyboard accessibility — accessibility improvement
💡 APPROVE WITH SUGGESTIONS
Summary: The multi-user webhook trigger feature is well-structured and follows established patterns from scheduled triggers. Test coverage is comprehensive (fan-out scenarios, sub-resource endpoints, authorization). However, the Ito test suite identified two production bugs that should be fixed:
-
Schema mismatch (Critical): The PATCH trigger update schema uses
TriggerAuthenticationInputSchemawhich requiresvalue, but the route handler expectskeepExistingsupport. Users cannot update triggers with existing auth headers without re-entering secrets. -
Missing auto-disable (Major): PATCH update with
runAsUserIds: []doesn't auto-disable the trigger (unlikePUT /{id}/userswhich does). This allows a trigger with zero users to remain enabled and accept invocations.
Both issues were confirmed by Ito integration tests and are documented in the inline comments with suggested fixes.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
TriggerService.ts:199-205 |
Fan-out returns success with empty invocations when all fail | Addressed — now returns 500 error |
trigger-form.tsx:741 |
dispatchDelayMs: 0 converted to undefined |
Addressed — now uses !== undefined check |
schemas.ts:816-822 |
runAsUserIds array lacks duplicate validation |
Addressed — .refine() added |
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
5 | 0 | 0 | 0 | 2 | 2 | 3 |
| Total | 5 | 0 | 0 | 0 | 2 | 2 | 3 |
Note: Sub-agent reviewers failed due to environment permission issues. Review was completed by orchestrator with manual analysis + Ito test report validation.
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId: id, | ||
| userIds: runAsUserIds, | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing auto-disable when PATCH update sets runAsUserIds to empty array
Issue: When runAsUserIds is set to an empty array via PATCH update, the trigger is not automatically disabled. This is inconsistent with the PUT /{id}/users endpoint (line 947) which correctly auto-disables triggers with zero users.
Why: Users can update a trigger to have no associated users via PATCH, leaving it enabled but non-functional. Per the Ito test report, the trigger then accepts rerun/webhook invocations with 202 responses when it should be blocked.
Fix: Add auto-disable logic after setting trigger users:
await setTriggerUsers(tx)({
scopes: { tenantId, projectId, agentId },
triggerId: id,
userIds: runAsUserIds,
});
// Auto-disable if no users after update (consistent with PUT /{id}/users)
if (runAsUserIds.length === 0) {
await updateTrigger(tx)({
scopes: { tenantId, projectId, agentId },
triggerId: id,
data: { enabled: false },
});
}Refs:
- triggers.ts:947 — PUT /{id}/users correctly auto-disables
- Ito test report — confirmed regression
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: High
This is a delta review scoped to changes since the last automated review (commit db840a09a964). The latest commit (77f3b9967 — "dont disable when users are removed") introduces a spec deviation that requires resolution before merge.
🔴❗ Critical (2) ❗🔴
Inline Comments:
- 🔴 Critical:
triggers.ts:944Auto-disable removed fromPUT /{id}/userswhen setting empty user list - 🔴 Critical:
triggers.ts:1152Auto-disable removed fromDELETE /{id}/users/{userId}when removing last user
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
triggers.test.ts:1395Test expectations changed to contradict spec
Analysis
The commit message "dont disable when users are removed" indicates this is an intentional change. However, this directly contradicts:
| Document | Requirement |
|---|---|
| SPEC.md Decision 6 | "Auto-disable on last user removed | LOCKED" |
| SPEC.md Section 6.5 | "If PUT/DELETE results in empty user set → auto-disable trigger" |
| SPEC.md Section 6.6 | "If last user removed → auto-disable trigger (enabled = false)" |
| Ito test report | Flagged as production bug when trigger stayed enabled with empty users |
Without auto-disable, a trigger with zero associated users:
- Remains enabled and accepts webhook calls
- Executes without user context (potential security/audit gap)
- Rerun behavior becomes ambiguous
Resolution Options
Option A (Recommended): Restore the auto-disable behavior to match the spec. The spec decision is marked LOCKED and follows the established scheduled trigger pattern.
Option B: If there's a legitimate reason to change this behavior, update the spec first:
- Add a new decision entry explaining why auto-disable is no longer desired
- Update Sections 6.5 and 6.6 to reflect the new behavior
- Document the expected behavior for triggers with empty user sets
🕐 Pending Recommendations (2)
Low-priority items from prior reviews that remain unaddressed:
- 🟡
invocations-table.tsx:554— StatusBadge inconsistency between single/multi-user paths (cosmetic) - 🟡
invocations-table.tsx:348— Clickable table rows lack keyboard accessibility
🚫 REQUEST CHANGES
Summary: The latest commit removes the auto-disable behavior that was explicitly specified and previously flagged by Ito tests as required. This is a spec deviation on a LOCKED decision. Either restore the auto-disable logic or update the spec with justification before merging.
Discarded (0)
No items discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 0 | 0 | 3 | 2 | 0 |
| Total | 3 | 0 | 0 | 0 | 3 | 2 | 0 |
Note: Delta review — analyzed commit diff directly without dispatching sub-agents (small, focused change).
|
|
||
| await updateTrigger(tx)({ | ||
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId: id, |
There was a problem hiding this comment.
🔴 CRITICAL: Spec deviation — auto-disable on empty user set removed
Issue: This commit removes the auto-disable behavior when PUT /{id}/users sets the user list to empty. The spec explicitly states this is required behavior.
Why: The SPEC.md Decision 6 states:
"Auto-disable on last user removed | LOCKED | Reversible"
"Consistent with scheduled trigger behavior (Decision 2 from prior spec)."
Section 6.5 also states: "If PUT/DELETE results in empty user set → auto-disable trigger"
Without auto-disable:
- A trigger with zero associated users remains enabled and can accept webhook calls
- Rerun/invocations will either fail silently or execute without user context unexpectedly
- This was already flagged by Ito integration tests as a production bug
Fix: If this is an intentional spec change, the spec needs to be updated first with justification. If unintentional, restore the auto-disable logic:
data: {
runAsUserId: null,
...(userIds.length === 0 ? { enabled: false } : {}),
},Refs:
- SPEC.md Decision 6
- SPEC.md Section 6.5 — "If PUT/DELETE results in empty user set → auto-disable trigger"
- Ito test report — confirmed this as a bug
|
|
||
| await updateTrigger(tx)({ | ||
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId: id, |
There was a problem hiding this comment.
🔴 CRITICAL: Spec deviation — same issue on DELETE endpoint
Issue: Same auto-disable removal on DELETE /{id}/users/{userId} when the last user is removed. Per SPEC.md Section 6.6:
"If last user removed → auto-disable trigger (
enabled = false)"
Fix: Restore the auto-disable logic:
data: {
runAsUserId: null,
...(remainingUserIds.length === 0 ? { enabled: false } : {}),
},| expect(rows.map((r) => r.userId).sort()).toEqual(['new-user-1', 'new-user-2']); | ||
| }); | ||
|
|
||
| it('should keep trigger enabled when setting users to empty array', async () => { |
There was a problem hiding this comment.
🟠 MAJOR: Test expectations contradict spec
Issue: These test expectations were changed to expect enabled: true when users are removed, but the spec mandates auto-disable. If the spec has changed, update it first — tests should follow spec, not vice versa.
If spec is unchanged, revert these tests:
it('should disable trigger when setting users to empty array', async () => {
// ...
expect(updated?.enabled).toBe(false);
});Refs:
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
schemas.ts:726Schema mismatch preventskeepExistingfrom working on PATCH trigger updates
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
invocations-table.tsx:350Clickable table rows lack keyboard accessibility
✅ Prior Issues Resolved
| Prior Issue | Status |
|---|---|
🔴 Critical: dispatchDelayMs: 0 falsy check bug |
✅ Fixed — uses !== undefined check |
| 🔴 Critical: Fan-out returns success when all fail | ✅ Fixed — returns 500 error |
| 🟠 Major: Missing multi-user fan-out tests | ✅ Addressed — webhooks.test.ts:2046-2136 |
| 🟠 Major: Missing sub-resource endpoint tests | ✅ Addressed — triggers.test.ts:1276-1714 |
🟡 Minor: runAsUserIds duplicate validation |
✅ Fixed — .refine() added |
| 🔴 Spec deviation: Auto-disable removed | ✅ Not a bug — Spec Decision 6 updated to "No auto-disable on empty user set" |
🕐 Pending Recommendations (1)
- 🟡
invocations-table.tsx:472—StatusBadgein single-user path vsgetInvocationStatusBadge()in multi-user path (cosmetic inconsistency from pullfrog review)
💡 APPROVE WITH SUGGESTIONS
Summary: The multi-user webhook trigger feature is well-structured, follows established patterns from scheduled triggers, and has comprehensive test coverage (fan-out scenarios, sub-resource endpoints, rerun authorization). All critical issues from prior reviews have been addressed.
One remaining bug (schema mismatch for keepExisting on PATCH updates) was confirmed by Ito integration tests — users cannot update triggers with existing auth headers without re-entering secrets. This is a usability issue but doesn't block the core multi-user functionality.
The spec was appropriately updated (Decision 6: "No auto-disable on empty user set") to reflect the intended design — triggers with zero users remain enabled and fall back to legacy runAsUserId or execute without user context.
Highlights:
- ✅ Fan-out execution with proper partial failure handling and 500 on total failure
- ✅ Comprehensive sub-resource API (
GET/PUT/POST/DELETE /{id}/users) - ✅ Rerun authorization with
runAsUserIdrequirement for multi-user triggers - ✅ User lifecycle cleanup on project/org member removal
- ✅ Breaking response shape change (
{ invocations: [...] }) is documented
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
TriggerService.ts:199-205 |
Fan-out returns success with empty invocations when all fail | Addressed — now returns 500 error |
trigger-form.tsx:741 |
dispatchDelayMs: 0 converted to undefined |
Addressed — now uses !== undefined check |
triggers.ts:944,1152 |
Auto-disable on empty user set removed | Not a bug — Spec Decision 6 updated to reflect intended design |
triggers.test.ts:1395 |
Test expectations contradict spec | Not a bug — Tests updated to match spec change |
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
6 | 0 | 0 | 0 | 2 | 1 | 4 |
| Total | 6 | 0 | 0 | 0 | 2 | 1 | 4 |
Note: Sub-agent reviewers failed due to environment permission issues. Review was completed by orchestrator with manual analysis + Ito test report validation.
| <Fragment key={group.key}> | ||
| <tr | ||
| className="border-b transition-colors hover:bg-muted/50 cursor-pointer" | ||
| onClick={() => toggleGroup(group.key)} |
There was a problem hiding this comment.
🟡 Minor: Accessibility — clickable table rows lack keyboard support
Issue: Table rows with onClick handlers (lines 348-350) are not focusable and do not respond to keyboard events. Screen reader users and keyboard-only users cannot expand/collapse grouped invocations.
Why: Violates WCAG 2.1 SC 2.1.1 (Keyboard) at Level A — interactive elements must be keyboard accessible.
Fix: Add keyboard support and proper ARIA attributes:
<tr
className="border-b transition-colors hover:bg-muted/50 cursor-pointer"
onClick={() => toggleGroup(group.key)}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
toggleGroup(group.key);
}
}}
tabIndex={0}
role="button"
aria-expanded={isExpanded}
>Or move the click handler to a proper <button> element within the expand cell (preferred for semantic correctness).
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since the last automated review (commit 66ba47fa5f72). The delta contains a single, targeted fix that resolves the critical schema mismatch issue.
✅ Issue Resolved in This Delta
| Prior Issue | Resolution |
|---|---|
🔴 Critical: Schema mismatch prevents keepExisting from working on PATCH trigger updates |
✅ Fixed — TriggerUpdateSchema now extends with authentication: TriggerAuthenticationUpdateSchema.optional() (line 829-830) |
Analysis
What changed:
export const TriggerUpdateSchema = TriggerInsertSchema.extend({
enabled: z.boolean().optional().describe('Whether the trigger is enabled'),
+ // Override authentication to use the update schema that supports keepExisting
+ authentication: TriggerAuthenticationUpdateSchema.optional(),
}).partial();Why this fix is correct:
TriggerAuthHeaderInputSchema(used byTriggerInsertSchema) requiresvalueto be presentTriggerAuthHeaderUpdateSchemamakesvalueoptional and addskeepExistingflag- The route handler at triggers.ts:612-648 already implements
keepExistinglogic — this schema change makes it reachable
Impact:
- Users can now update webhook triggers (e.g., changing
dispatchDelayMs) without re-entering authentication header secrets - PATCH requests with
{ authentication: { headers: [{ name: "X-API-Key", keepExisting: true }] } }now pass validation
✅ All Prior Critical/Major Issues Addressed
| Prior Issue | Status |
|---|---|
🔴 Critical: dispatchDelayMs: 0 falsy check bug |
✅ Fixed in trigger-form.tsx:741 |
| 🔴 Critical: Fan-out returns success when all fail | ✅ Fixed — returns 500 error |
| 🟠 Major: Missing multi-user fan-out tests | ✅ Addressed — webhooks.test.ts:2046-2136 |
| 🟠 Major: Missing sub-resource endpoint tests | ✅ Addressed — triggers.test.ts:1276-1714 |
🟡 Minor: runAsUserIds duplicate validation |
✅ Fixed — .refine() added |
| Spec deviation: Auto-disable on empty user set | ✅ Not a bug — Spec Decision 6 updated |
🕐 Pending Recommendations (2)
Low-priority items from prior reviews that remain unaddressed but do not block merge:
- 🟡
invocations-table.tsx:472—StatusBadgein single-user path vsgetInvocationStatusBadge()in multi-user path (cosmetic inconsistency from pullfrog review) - 🟡
invocations-table.tsx:348— Clickable table rows lack keyboard accessibility (tabIndex,role,onKeyDown)
✅ APPROVE
Summary: The delta contains a targeted fix that correctly resolves the critical schema mismatch issue identified by Ito integration tests. Users can now update protected webhook triggers without re-entering authentication secrets. All prior critical and major issues have been addressed. The multi-user webhook trigger feature is well-structured, follows established patterns from scheduled triggers, and has comprehensive test coverage. 🚀
Feature highlights:
- ✅
trigger_usersjoin table for many-to-many relationship - ✅ Fan-out execution with proper partial failure handling (500 on total failure)
- ✅ Comprehensive sub-resource API (
GET/PUT/POST/DELETE /{id}/users) - ✅ Rerun authorization with
runAsUserIdrequirement for multi-user triggers - ✅ User lifecycle cleanup on project/org member removal
- ✅ Breaking response shape change (
{ invocations: [...] }) documented in spec
Discarded (0)
No items discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta review of a single-file schema fix — no sub-agents dispatched. The fix correctly addresses the critical issue identified in prior reviews and confirmed by Ito integration tests.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
1 participant
No description provided.