feat(agents-api): User scoped webhooks#2442
Conversation
🦋 Changeset detectedLatest commit: 92f1d9c 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.
2 Skipped Deployments
|
|
Adds a
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium urgency. The feature design is solid — runtime permission checks at execution time, assertCanMutateTrigger for CRUD authorization, and the security warning for unauthenticated user-scoped triggers are all good patterns. There are a few issues worth addressing before merge:
Security:
- Error messages in
TriggerService.tsinterpolate internal error details and user IDs into invocation records that are readable by any project member. assertCanMutateTriggerhas an edge case where empty-stringcallerIdcan match empty-stringcreatedBy, and legacy triggers (both fields null) block all non-admins.
Breaking change:
- The OpenAPI
$refchanged fromTriggerWithWebhookUrlResponsetoTriggerWithWebhookUrlWithWarningResponseon create/update. Runtime-compatible but a spec-level schema rename.
Type safety / code quality:
- Multiple
as anycasts in the UI where the types already includerunAsUserId. - Re-export + import pattern in
scheduledTriggers.tsis confusing. cleanupUserScheduledTriggersname is misleading now that it also cleans up webhook triggers.
Test gaps:
- No route-level tests for
assertCanMutateTriggeron webhook trigger update/delete. setTimeout(200)waits in webhook tests are flaky.- Missing tests for empty-string
runAsUserIdnormalization and warning response schema.
| invocationId, | ||
| data: { | ||
| status: 'failed', | ||
| errorMessage: `Permission check failed for user ${runAsUserId}: ${err instanceof Error ? err.message : String(err)}`, |
There was a problem hiding this comment.
This error message interpolates the raw SpiceDB/internal error (err.message) into the invocation record, which is returned via the GET invocation endpoint. This risks leaking infrastructure details (hostnames, connection strings, stack traces). Use a generic message like 'Permission check failed' and keep the details in the structured log only.
| invocationId, | ||
| data: { | ||
| status: 'failed', | ||
| errorMessage: `User ${runAsUserId} no longer has 'use' permission on project ${projectId}`, |
There was a problem hiding this comment.
This error message embeds runAsUserId and projectId into the invocation record, which is readable by any project member. If runAsUserId could be an email-based ID, this is an information disclosure concern. Consider a generic message like 'The configured user no longer has access to this project'.
| export function assertCanMutateTrigger(params: { | ||
| trigger: { createdBy?: string | null; runAsUserId?: string | null }; | ||
| callerId: string; | ||
| tenantRole: OrgRole; | ||
| }): void { | ||
| const { trigger, callerId, tenantRole } = params; | ||
| const isAdmin = tenantRole === OrgRoles.OWNER || tenantRole === OrgRoles.ADMIN; | ||
| if (isAdmin) return; | ||
| if (trigger.createdBy === callerId || trigger.runAsUserId === callerId) return; | ||
| throw createApiError({ | ||
| code: 'forbidden', | ||
| message: 'You can only modify triggers that you created or that are configured to run as you.', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Two edge cases to consider:
-
Empty callerId: When
callerIdis empty string (no userId on context) andtrigger.createdByis also empty string,createdBy === callerIdevaluates to'' === ''→ true, allowing mutation. Add an early guard:if (!callerId) throw createApiError({ code: 'forbidden', ... })before the ownership checks. -
Legacy triggers: Pre-migration triggers have both
createdByandrunAsUserIdas null. For non-admins,null === callerIdis false for both checks, so all non-admins are blocked from mutating legacy triggers they may have created. Is this intentional? If so, a brief comment would prevent confusion.
| export { assertCanMutateTrigger, validateRunAsUserId } from './triggerHelpers'; | ||
|
|
||
| const targetCanUse = await canUseProjectStrict({ | ||
| userId: runAsUserId, | ||
| tenantId, | ||
| projectId, | ||
| }); | ||
| import { assertCanMutateTrigger, validateRunAsUserId } from './triggerHelpers'; |
There was a problem hiding this comment.
Line 58 re-exports assertCanMutateTrigger and validateRunAsUserId from ./triggerHelpers, then line 60 immediately imports the same symbols. The re-export exists only for backward compat with the test file — update userScopedScheduledTriggers.test.ts to import directly from ./triggerHelpers and remove the re-export to avoid this confusing pattern.
| {(trigger as any).runAsUserId ? ( | ||
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className="text-sm text-muted-foreground truncate max-w-[150px] inline-block cursor-default"> | ||
| {getUserDisplayName((trigger as any).runAsUserId)} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <code className="font-mono text-xs"> | ||
| {(trigger as any).runAsUserId} | ||
| </code> |
There was a problem hiding this comment.
(trigger as any).runAsUserId — TriggerWithAgent extends Trigger which ultimately includes TriggerApiSelect, and TriggerApiSelectSchema already has runAsUserId (schemas.ts:770). Drop the as any casts (here, lines 161, 166) and use trigger.runAsUserId directly for type safety.
| if (failed === 0) { | ||
| toast.success(`Created ${succeeded} triggers successfully`); | ||
| router.push(redirectPath); | ||
| } else if (succeeded > 0) { | ||
| toast.warning(`Created ${succeeded}/${bulkResults.length} triggers. ${failed} failed.`); | ||
| router.push(redirectPath); | ||
| } else { | ||
| toast.error('Failed to create triggers'); | ||
| } | ||
| return; |
There was a problem hiding this comment.
On partial bulk-creation failure, the user sees a warning toast and is immediately redirected away. They have no way to see which users failed or retry just those. Consider either staying on the page with the failed user IDs still selected, or providing more detail in the toast message.
| const params = new URLSearchParams(); | ||
| if (trigger.messageTemplate) | ||
| params.set('messageTemplate', trigger.messageTemplate); | ||
| if (trigger.inputSchema) | ||
| params.set('inputSchema', JSON.stringify(trigger.inputSchema)); | ||
| if (trigger.outputTransform) | ||
| params.set( | ||
| 'outputTransform', | ||
| JSON.stringify(trigger.outputTransform) | ||
| ); | ||
| params.set('enabled', String(trigger.enabled)); | ||
| return `/${tenantId}/projects/${projectId}/triggers/webhooks/${trigger.agentId}/new?${params.toString()}`; | ||
| })()} |
There was a problem hiding this comment.
JSON.stringify(trigger.inputSchema) is embedded in a URL query param for the duplicate feature. For triggers with large JSON schemas, this can exceed browser URL length limits (~2KB safe). Consider using sessionStorage to pass duplicated trigger data instead of query params.
| describe('User-scoped execution', () => { | ||
| const userScopedTrigger = { | ||
| ...testTrigger, | ||
| runAsUserId: 'user-abc', | ||
| }; | ||
|
|
||
| it('should check permission and proceed when runAsUserId is set and user has access', async () => { | ||
| getTriggerByIdMock.mockReturnValue(vi.fn().mockResolvedValue(userScopedTrigger)); | ||
| canUseProjectStrictMock.mockResolvedValue(true); | ||
|
|
||
| const response = await app.request( | ||
| '/tenants/tenant-123/projects/project-123/agents/agent-123/triggers/trigger-123', | ||
| { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ message: 'hello' }), | ||
| } | ||
| ); | ||
|
|
||
| expect(response.status).toBe(202); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| expect(canUseProjectStrictMock).toHaveBeenCalledWith({ | ||
| userId: 'user-abc', | ||
| tenantId: 'tenant-123', | ||
| projectId: 'project-123', | ||
| }); | ||
|
|
||
| // Execution proceeded — conversation was created | ||
| expect(createOrGetConversationMock).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should mark invocation failed and skip execution when user lacks permission', async () => { | ||
| getTriggerByIdMock.mockReturnValue(vi.fn().mockResolvedValue(userScopedTrigger)); | ||
| canUseProjectStrictMock.mockResolvedValue(false); | ||
|
|
||
| const updateStatusFn = vi.fn().mockResolvedValue({}); | ||
| updateTriggerInvocationStatusMock.mockReturnValue(updateStatusFn); | ||
|
|
||
| const response = await app.request( | ||
| '/tenants/tenant-123/projects/project-123/agents/agent-123/triggers/trigger-123', | ||
| { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ message: 'hello' }), | ||
| } | ||
| ); | ||
|
|
||
| expect(response.status).toBe(202); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| expect(updateStatusFn).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| status: 'failed', | ||
| errorMessage: expect.stringContaining("no longer has 'use' permission"), | ||
| }), | ||
| }) | ||
| ); | ||
| // Execution did not proceed — no conversation created | ||
| expect(createOrGetConversationMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should mark invocation failed when permission check throws', async () => { | ||
| getTriggerByIdMock.mockReturnValue(vi.fn().mockResolvedValue(userScopedTrigger)); | ||
| canUseProjectStrictMock.mockRejectedValue(new Error('SpiceDB unavailable')); | ||
|
|
||
| const updateStatusFn = vi.fn().mockResolvedValue({}); | ||
| updateTriggerInvocationStatusMock.mockReturnValue(updateStatusFn); | ||
|
|
||
| const response = await app.request( | ||
| '/tenants/tenant-123/projects/project-123/agents/agent-123/triggers/trigger-123', | ||
| { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ message: 'hello' }), | ||
| } | ||
| ); | ||
|
|
||
| expect(response.status).toBe(202); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| expect(updateStatusFn).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| status: 'failed', | ||
| errorMessage: expect.stringContaining('Permission check failed'), | ||
| }), | ||
| }) | ||
| ); | ||
| // Execution did not proceed — no conversation created | ||
| expect(createOrGetConversationMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should skip permission check and use anonymous initiatedBy when trigger has no runAsUserId', async () => { | ||
| getTriggerByIdMock.mockReturnValue(vi.fn().mockResolvedValue(testTrigger)); | ||
|
|
||
| const response = await app.request( | ||
| '/tenants/tenant-123/projects/project-123/agents/agent-123/triggers/trigger-123', | ||
| { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ message: 'hello' }), | ||
| } | ||
| ); | ||
|
|
||
| expect(response.status).toBe(202); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| expect(canUseProjectStrictMock).not.toHaveBeenCalled(); | ||
| // Execution proceeded — conversation was created | ||
| expect(createOrGetConversationMock).toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing route-level test coverage: These tests cover the runtime permission check in TriggerService.executeAgentAsync, but there are no tests for assertCanMutateTrigger enforcement on the webhook trigger update and delete routes (triggers.ts). The scheduled trigger equivalent has thorough tests in userScopedScheduledTriggers.test.ts — consider adding analogous tests for: (a) non-admin update of someone else's trigger → 403, (b) non-admin delete of own trigger → 200, (c) admin update of any trigger → 200.
![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) ❗🔴
🔴 1) triggers.ts:824-834 Rerun endpoint bypasses user-scoped execution
Issue: The /triggers/{id}/rerun endpoint fetches the trigger (including runAsUserId) but does not pass it to dispatchExecution. When a user-scoped webhook trigger is rerun via the management API, it will execute as anonymous instead of as the configured user identity.
Why: This completely bypasses the runtime permission check added in TriggerService.ts (lines 698-734). A trigger configured to run as user-abc could be rerun even after that user loses project access, and the execution would proceed without the user identity context. This means:
- The
canUseProjectStrictcheck never fires for reruns - Execution metadata shows anonymous instead of the configured user
- User-scoped MCP servers and OAuth credentials won't be available
Fix: Add runAsUserId: trigger.runAsUserId ?? undefined to the dispatchExecution call.
Refs:
Inline Comments:
- 🔴 Critical:
triggers.ts:824-834Rerun endpoint missing runAsUserId parameter
🟠⚠️ Major (3) 🟠⚠️
🟠 1) TriggerService.ts:707-731 Missing try-catch for invocation status updates
Issue: The updateTriggerInvocationStatus calls within the permission check block are not wrapped in try-catch. If the database update fails after the permission check fails or throws, the error will propagate unhandled in the async waitUntil context.
Why: The invocation record will remain in 'pending' status permanently even though execution was aborted. Operators debugging stuck 'pending' invocations would have no indication a permission check failed. The existing pattern elsewhere in this file (lines 562-575, 917-928) correctly wraps these calls in try-catch.
Fix: Wrap both updateTriggerInvocationStatus calls (lines 707-715 and 723-731) in try-catch blocks consistent with the existing pattern.
Refs:
Inline Comments:
- 🟠 Major:
TriggerService.ts:707-715Missing try-catch for status update
🟠 2) project-triggers-table.tsx Missing canManageTrigger UI authorization guard
Issue: The scheduled triggers table implements a canManageTrigger function that disables edit/delete actions for non-admins who did not create the trigger. The webhook triggers table lacks this authorization guard, allowing any user with project access to see and click edit/delete actions that the backend will reject with 403.
Why: While the backend correctly enforces assertCanMutateTrigger, the UI should match by disabling actions the user cannot perform. This creates better UX (no confusing 403 errors after clicking) and maintains consistency with the scheduled triggers pattern.
Fix: Add a canManageTrigger helper function and use it to disable Edit/Delete dropdown items for users who lack permission.
Refs:
Inline Comments:
- 🟠 Major:
project-triggers-table.tsx:152-165Missing canManageTrigger guard
🟠 3) _snippets/user-scoped-mcp-triggers-warning.mdx Documentation contradicts this feature
files:
agents-docs/_snippets/user-scoped-mcp-triggers-warning.mdxagents-docs/content/talk-to-your-agents/triggers/webhooks.mdxagents-docs/content/typescript-sdk/triggers/webhooks.mdx
Issue: The existing documentation snippet explicitly states: "Webhook triggers do not have user context, so the framework cannot determine which user's credentials to use." This warning is included in both webhook trigger documentation pages. This PR adds exactly that capability (runAsUserId), making the existing warning actively misleading.
Why: Customers reading the docs will be told user-scoped execution is impossible while the UI prominently offers "Execution Identity" settings. The scheduled triggers documentation already has comprehensive runAsUserId documentation (configuration table, User-Scoped Execution section, permission rules) that could serve as a template.
Fix: Update the warning snippet to acknowledge that user-scoped execution is now possible when runAsUserId is configured, and add equivalent documentation to the webhook trigger pages covering configuration options, permission rules, and security implications.
Refs:
🟡 Minor (2) 🟡
🟡 1) project-triggers-table.tsx:156-166 Type assertions bypass type safety
Issue: The code uses (trigger as any).runAsUserId three times instead of typed access. The TriggerWithAgent type should include runAsUserId after the schema updates.
Why: Type casting with as any means TypeScript cannot verify the field exists. If the schema changes, this code would compile but fail at runtime.
Fix: Verify the type exports include the new fields and remove the type assertions.
Refs:
Inline Comments:
- 🟡 Minor:
project-triggers-table.tsx:156-166Type assertion bypasses type safety - 🟡 Minor:
triggers.ts:231-234Inconsistent error message wording
🟡 2) trigger-form.tsx:954-960 Accessibility: Badge lacks accessible name
Issue: The Badge component for removing selected users uses × as the only indication of the remove action, which may not be announced meaningfully by screen readers.
Fix: Add role="button" and aria-label to clarify the action.
Inline Comments:
- 🟡 Minor:
trigger-form.tsx:954-960Badge lacks accessible name
💭 Consider (4) 💭
💭 1) project-triggers-table.tsx:233-252 Duplicate omits runAsUserId without user notification
Issue: When duplicating a user-scoped trigger, the new trigger silently reverts to anonymous execution.
Why: Users might expect an identical trigger but get one with different runtime permissions.
Fix: Either preserve runAsUserId in duplicates, or add a visible indication that execution identity was reset.
💭 2) trigger-form.tsx:882-890 Terminology inconsistency: "Execution Identity" vs "Run As"
Issue: The form uses "Execution Identity" while the table column uses "Run As".
Fix: Consider aligning terminology — "Run As User" may be clearer than "Execution Identity".
💭 3) cleanup.ts:16 Function name cleanupUserScheduledTriggers is now misleading
Issue: The function now deletes both scheduled and webhook triggers but the name only mentions scheduled.
Fix: Consider renaming to cleanupUserTriggers.
💭 4) scheduledTriggers.ts:58-60 Redundant import after re-export
Issue: The file both re-exports and imports the same helpers from triggerHelpers.
Fix: Add a comment explaining the re-export is for backwards compatibility, or remove if no longer needed.
🧹 While You're Here (1) 🧹
🧹 1) validateRunAsUserId No explicit tenant membership verification
Issue: The validateRunAsUserId function checks that the user exists globally and has project use permission, but doesn't explicitly verify the user is a member of the specified tenant organization.
Why: While canUseProjectStrict should catch this if SpiceDB is correctly configured (since project permissions are tenant-scoped), explicitly verifying org membership would provide defense in depth against SpiceDB misconfigurations.
Fix: Consider adding a check that runAsUserId is a member of the tenant organization before the canUseProjectStrict check.
Refs:
🚫 REQUEST CHANGES
Summary: This PR implements user-scoped webhooks following the established scheduled triggers pattern well. However, there's a critical security issue where the rerun endpoint doesn't pass runAsUserId to dispatchExecution, completely bypassing the user permission checks for reruns. Additionally, the documentation actively contradicts this feature (saying webhooks can't have user context), and the UI is missing the canManageTrigger authorization guard that exists for scheduled triggers.
The core feature implementation is solid — the schema, validation, cleanup, and UI patterns are well-designed and consistent with existing patterns. Once the rerun endpoint bug is fixed and docs are updated, this will be ready to merge.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
triggerHelpers.ts |
Unit tests missing for validateRunAsUserId |
Valid concern but test coverage is an enhancement, not blocking |
triggers.ts |
Unit tests missing for webhook assertCanMutateTrigger scenarios |
Valid concern but implicit coverage exists via scheduled triggers tests |
webhooks.test.ts |
Tests don't verify initiatedBy metadata propagation |
Valid but lower priority than the security issues |
cleanup.test.ts |
Test doesn't verify error handling if first delete throws | Implementation uses Promise.allSettled which handles this gracefully |
projectFull.test.ts |
mockResolvedValueOnce is fragile for multiple triggers |
Unlikely scenario given current implementation |
triggers.ts:755-760 |
Missing auth check for rerun when trigger has runAsUserId | The runtime check in TriggerService handles this; adding route-level check would be defense in depth but not critical |
projectFull.ts:431-432 |
Empty callerId could result in createdBy: '' |
Edge case for system/API-key authenticated calls; acceptable for now |
| System | No audit logging for runAsUserId changes | Valid enhancement but not blocking for this PR |
Reviewers (10)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-security-iam |
5 | 1 | 0 | 1 | 0 | 0 | 3 |
pr-review-tests |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-types |
3 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-breaking-changes |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
5 | 1 | 0 | 0 | 2 | 0 | 2 |
pr-review-consistency |
6 | 1 | 2 | 0 | 1 | 0 | 2 |
pr-review-errors |
1 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-precision |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-product |
6 | 1 | 2 | 0 | 0 | 0 | 3 |
| Total | 33 | 7 | 5 | 1 | 6 | 0 | 16 |
| await updateTriggerInvocationStatus(runDbClient)({ | ||
| scopes: { tenantId, projectId, agentId }, | ||
| triggerId, | ||
| invocationId, | ||
| data: { | ||
| status: 'failed', | ||
| errorMessage: `User ${runAsUserId} no longer has 'use' permission on project ${projectId}`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing try-catch for invocation status update
Issue: The updateTriggerInvocationStatus call is not wrapped in try-catch. If the database update fails after the permission check fails, the error will propagate unhandled in the async execution context.
Why: The invocation record will remain in 'pending' status permanently even though execution was aborted. Since this runs in a waitUntil context, the unhandled rejection could cause undefined behavior. The existing pattern elsewhere in this file (lines 562-575) correctly wraps this call in try-catch.
Fix: Wrap in try-catch consistent with the existing pattern:
try {
await updateTriggerInvocationStatus(runDbClient)({
scopes: { tenantId, projectId, agentId },
triggerId,
invocationId,
data: {
status: 'failed',
errorMessage: `User ${runAsUserId} no longer has 'use' permission on project ${projectId}`,
},
});
} catch (updateError) {
logger.error(
{ err: updateError instanceof Error ? updateError.message : String(updateError), invocationId },
'Failed to update invocation status after permission denial'
);
}Apply the same pattern to the catch block at lines 723-731.
Refs:
| {trigger.agentName} | ||
| </Link> | ||
| </TableCell> | ||
| <TableCell> | ||
| {(trigger as any).runAsUserId ? ( | ||
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className="text-sm text-muted-foreground truncate max-w-[150px] inline-block cursor-default"> | ||
| {getUserDisplayName((trigger as any).runAsUserId)} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <code className="font-mono text-xs"> |
There was a problem hiding this comment.
🟠 MAJOR: Missing canManageTrigger authorization guard
Issue: The scheduled triggers table implements a canManageTrigger function that disables edit/delete actions for non-admins who did not create the trigger. The webhook triggers table lacks this authorization guard, allowing any user with project access to attempt modifications via the UI that the backend will reject.
Why: While the backend enforces assertCanMutateTrigger, the UI should show consistent behavior by disabling actions the user cannot perform. This creates a better UX (no confusing 403 errors) and matches the scheduled triggers pattern.
Fix: Add a canManageTrigger function similar to project-scheduled-triggers-table.tsx:
const canManageTrigger = (trigger: TriggerWithAgent): boolean => {
if (isAdmin) return true;
if (user?.id === trigger.createdBy) return true;
if (user?.id === (trigger as any).runAsUserId) return true;
return false;
};Then wrap the Edit/Delete DropdownMenuItem components with disabled={!canManageTrigger(trigger)}.
Refs:
| {(trigger as any).runAsUserId ? ( | ||
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className="text-sm text-muted-foreground truncate max-w-[150px] inline-block cursor-default"> | ||
| {getUserDisplayName((trigger as any).runAsUserId)} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <code className="font-mono text-xs"> | ||
| {(trigger as any).runAsUserId} |
There was a problem hiding this comment.
🟡 Minor: Type assertion (trigger as any).runAsUserId bypasses type safety
Issue: The code uses (trigger as any).runAsUserId three times instead of typed access. The TriggerWithAgent type should already include runAsUserId after the schema updates.
Why: Type casting with as any means TypeScript cannot verify the field exists. If the schema changes (rename/remove), this code would compile but fail at runtime.
Fix: Verify that TriggerApiSelect from @inkeep/agents-core/client-exports includes the new fields after the schema update, then remove the type assertions:
{trigger.runAsUserId ? (
// ... use trigger.runAsUserId directlyIf types aren't propagating, check the export in packages/agents-core/src/client-exports.ts.
Refs:
| {selectedUserIds.map((id) => ( | ||
| <Badge | ||
| key={id} | ||
| variant="secondary" | ||
| className="cursor-pointer" | ||
| onClick={() => | ||
| setSelectedUserIds((prev) => prev.filter((uid) => uid !== id)) |
There was a problem hiding this comment.
🟡 Minor: Badge with remove action lacks accessible name
Issue: The Badge renders {getMemberDisplayName(id)} × where the multiplication sign is the only indication of the remove action. Screen readers may not announce this meaningfully.
Why: Per accessibility best practices, interactive elements need accessible names.
Fix: Add role and aria-label:
| {selectedUserIds.map((id) => ( | |
| <Badge | |
| key={id} | |
| variant="secondary" | |
| className="cursor-pointer" | |
| onClick={() => | |
| setSelectedUserIds((prev) => prev.filter((uid) => uid !== id)) | |
| <Badge | |
| key={id} | |
| variant="secondary" | |
| className="cursor-pointer" | |
| onClick={() => | |
| setSelectedUserIds((prev) => prev.filter((uid) => uid !== id)) | |
| } | |
| role="button" | |
| aria-label={`Remove ${getMemberDisplayName(id)}`} | |
| > | |
| {getMemberDisplayName(id)} <span aria-hidden="true">×</span> | |
| </Badge> |
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) New Issues in Delta | Risk: Low (improved from prior review)
Delta Review Scope
This is a re-review scoped to the 2 commits since the last automated review:
| Commit | Description |
|---|---|
e6cae1518 |
[US-010] Fix rerun endpoint to forward runAsUserId to dispatchExecution |
bf6d9a0f5 |
[US-011] Guard rerun endpoint: require admin or matching caller for user-scoped triggers |
Files changed in delta: 4 files, +196/-6 lines
✅ Critical Issue Resolved
The Critical finding from the prior review has been properly addressed:
🔴
triggers.ts:824-834Rerun endpoint bypasses user-scoped execution — FIXED
What was fixed:
runAsUserIdforwarding (line 840): The rerun endpoint now passesrunAsUserId: trigger.runAsUserId ?? undefinedtodispatchExecution, ensuring runtime permission checks fire correctly- Authorization guard (lines 814-816): Added conditional
assertCanMutateTriggercall whentrigger.runAsUserIdexists, preventing unauthorized users from rerunning user-scoped triggers
Why the conditional guard is correct: The authorization check is only applied when trigger.runAsUserId is set because:
- Anonymous triggers don't execute with any specific user's identity, so there's no delegation risk
- Project-level
requireProjectPermission('edit')already gates access to the rerun endpoint - This matches the security model where the guard protects against impersonation, not general trigger access
Test coverage added: 5 new test cases covering:
runAsUserIdforwarding to dispatchExecution ✓assertCanMutateTriggerinvocation for user-scoped triggers ✓- 403 response when authorization fails ✓
- Backward compatibility for triggers without
runAsUserId✓ - Edge cases (disabled trigger → 409, not found → 404) ✓
🕐 Pending Recommendations (from prior review)
The following items from the prior review remain unaddressed. These are not blocking but should be considered before merge:
| Severity | Location | Issue | Link |
|---|---|---|---|
| 🟠 Major | TriggerService.ts:707-731 |
Missing try-catch for invocation status updates | view |
| 🟠 Major | project-triggers-table.tsx:152-165 |
Missing canManageTrigger UI authorization guard |
view |
| 🟠 Major | Docs: user-scoped-mcp-triggers-warning.mdx |
Documentation contradicts this feature | Prior review body |
| 🟡 Minor | project-triggers-table.tsx:156-166 |
Type assertions (trigger as any).runAsUserId bypass type safety |
view |
| 🟡 Minor | triggers.ts:231-234 |
Inconsistent error message wording | view |
| 🟡 Minor | trigger-form.tsx:954-960 |
Badge lacks accessible name | view |
| 🟡 Minor | TriggerService.ts:713, 729 |
Error messages may leak internal details | view |
| 💭 Consider | cleanup.ts:16 |
Function name cleanupUserScheduledTriggers is misleading |
view |
| 💭 Consider | projectFull.ts:419-429 |
Sequential listTriggers call is N+1 |
view |
Additionally, the changeset-bot flagged that this PR has no changeset. Given the OpenAPI schema rename (TriggerWithWebhookUrlResponse → TriggerWithWebhookUrlWithWarningResponse), a patch changeset for @inkeep/agents-api and @inkeep/agents-core is recommended.
💡 APPROVE WITH SUGGESTIONS
Summary: The delta commits successfully fix the critical rerun endpoint bug with proper authorization guards and comprehensive test coverage. The implementation is correct — the conditional authorization check when runAsUserId exists is the right design since anonymous triggers don't have delegation risks.
The remaining pending items from the prior review (try-catch wrappers, UI authorization guards, documentation updates, type assertions) should be addressed but are not blocking given the critical security fix is now in place. Consider adding a changeset before merge.
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
5 | 0 | 0 | 0 | 0 | 1 | 4 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 5 | 0 | 0 | 0 | 0 | 1 | 4 |
Note: Test reviewer findings were primarily INFO-level observations about test quality (positive), with 1 MINOR about mock patterns that was already flagged in the prior review.
Ito Test Report ❌30 test cases ran. 29 passed, 1 failed. This test run verified the user-scoped webhook triggers feature for PR #2442. The majority of authorization and permission checks work correctly: non-admin users are blocked from escalating privileges via API, system identifiers are rejected, and runtime permission checks properly fail invocations when users lose project access. However, a critical authorization bypass was discovered in the UI delete flow, where non-admin users can delete triggers created by other users through the UI despite the API-level authorization check being correctly implemented. ✅ Passed (29)
❌ Failed (1)
Delete user-scoped trigger authorization check – Failed
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (2) Minor Inline Comments | Risk: Low
Delta Review Scope
This is a re-review scoped to the 3 commits since the last automated review:
| Commit | Description |
|---|---|
49fa12b32 |
verify entity diff |
f93d9a9e9 |
address feedback |
9054aa4c7 |
Merge branch 'main' into feat/webhook-user-scope |
✅ Prior Findings Addressed
Excellent work addressing the feedback from the prior reviews! The following issues have been resolved in this delta:
| Prior Finding | Status | What Changed |
|---|---|---|
| 🟠 Major: Missing try-catch in TriggerService.ts | ✅ FIXED | Added try-catch wrappers around both updateTriggerInvocationStatus calls (lines 707-731) |
🟠 Major: Missing canManageTrigger UI guard |
✅ FIXED | Added canManageTrigger() function and disabled={!canManage} on Edit/Delete buttons |
| 🟠 Major: Documentation contradicts feature | ✅ FIXED | Updated warning snippet and added User-Scoped Execution sections to both webhook docs |
🟡 Minor: Type assertions (trigger as any).runAsUserId |
✅ FIXED | Removed all as any casts, now using typed access |
| 🟡 Minor: Badge lacks accessible name | ✅ FIXED | Added role="button" and aria-label attributes |
| 💭 Consider: Terminology "Execution Identity" vs "Run As" | ✅ FIXED | Renamed card title to "Run As" for consistency with table column |
💭 Consider: Function name cleanupUserScheduledTriggers |
✅ FIXED | Renamed to cleanupUserTriggers |
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
triggers.ts:233Inconsistent error message wording (callerId vs Authenticated user ID) - 🟡 Minor:
triggers.ts:394Same inconsistency in update route
📝 Changeset Reminder
The changeset-bot flagged that this PR has no changeset. Given the scope of changes (new runAsUserId field, new API response schema TriggerWithWebhookUrlWithWarningResponse, authorization guards), a patch changeset for @inkeep/agents-api and @inkeep/agents-core would be appropriate:
pnpm bump patch --pkg agents-api --pkg agents-core "Add user-scoped execution identity (runAsUserId) to webhook triggers"🧹 While You're Here (1) 🧹
🧹 1) api-config.ts:67-69 UI bypass secret defeats per-trigger authorization
Issue: The Ito test failure (ROUTE-5) revealed that assertCanMutateTrigger is bypassed when requests come through the manage-ui server because it unconditionally uses INKEEP_AGENTS_MANAGE_API_BYPASS_SECRET, which grants system-level access.
Why: This is a pre-existing infrastructure pattern, not introduced by this PR. However, it means the new per-trigger authorization checks (assertCanMutateTrigger) only protect direct API access, not UI-initiated actions. The UI server essentially acts as a superuser proxy.
Not blocking this PR — this is an architectural decision that predates this feature. If this is unexpected behavior, it would be worth a separate investigation to determine whether the UI should forward user sessions instead of using the bypass secret for certain operations.
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: The delta demonstrates excellent responsiveness to feedback — all Major and most Minor findings from the prior reviews have been properly addressed. The try-catch wrappers, UI authorization guards, documentation updates, type safety fixes, and accessibility improvements are all well-implemented.
The remaining two inline comments are minor wording consistency issues with 1-click fixes. The changeset reminder is a housekeeping item. The "While You're Here" observation about the UI bypass secret is informational and not blocking.
Great feature implementation! 🎉
Discarded (0)
No findings were discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Self-review | — | 0 | 0 | 1 | 2 | 0 | 0 |
Note: Given the minimal delta (primarily addressing prior feedback), full subagent reviewer dispatch was not necessary. This review was conducted directly by the orchestrator.
| if (!callerId) { | ||
| throw createApiError({ | ||
| code: 'bad_request', | ||
| message: 'callerId is required when setting runAsUserId', |
There was a problem hiding this comment.
🟡 Minor: Same inconsistency — use 'Authenticated user ID is required when setting runAsUserId' for consistency with scheduled triggers.
Fix: (1-click apply)
| message: 'callerId is required when setting runAsUserId', | |
| message: 'Authenticated user ID is required when setting runAsUserId', |
|
@claude add a changeset |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (1) Minor Inline Comment | Risk: Low
Delta Review Scope
This is a re-review scoped to the 1 commit since the last automated review:
| Commit | Description |
|---|---|
41dd35192 |
Update agents-api/src/domains/manage/routes/triggers.ts |
Files changed: 1 file, +1/-1 lines
✅ Prior Finding Addressed
| Prior Finding | Status | What Changed |
|---|---|---|
🟡 Minor: triggers.ts:233 Inconsistent error message wording |
✅ FIXED | Changed from 'callerId is required...' to 'Authenticated user ID is required...' |
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
triggers.ts:394Same inconsistency in update route still uses "callerId" wording
📝 Changeset Status
The changeset-bot flagged that this PR has no changeset. Given the scope of changes (new runAsUserId field, new API response schema TriggerWithWebhookUrlWithWarningResponse, authorization guards), a changeset is appropriate. Adding one now per user request.
💡 APPROVE WITH SUGGESTIONS
Summary: The delta successfully addresses one of two minor wording inconsistencies from the prior review. The remaining inconsistency at line 394 is trivial (1-click suggestion provided). All prior Critical and Major findings have been resolved in earlier commits. This feature is well-implemented and ready to merge once the changeset is added.
🎉 Great work on this comprehensive user-scoped webhooks feature!
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Self-review | — | 0 | 0 | 0 | 1 | 0 | 0 |
Note: Given the minimal delta (single line wording fix), full subagent reviewer dispatch was not necessary.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…duce duplication in projectFull.ts
miles-kt-inkeep
force-pushed
the
feat/webhook-user-scope
branch
from
3cf6528 to
62e1f85
Compare
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🔴❗ Critical (1) ❗🔴
🔴 1) agents-docs/content/typescript-sdk/triggers/webhooks.mdx:348-354 SDK docs show incorrect trigger() function signature
Issue: The documentation example for runAsUserId uses a two-argument function signature trigger("github-pr-review", { ... }) but the SDK's actual signature is a single config object: trigger(config: Omit<TriggerApiInsert, 'id'> & { id?: string }).
Why: Developers copy-pasting this example will get TypeScript errors or runtime failures. This is the primary documentation for the feature being introduced in this PR, so accuracy is critical for adoption.
Fix: Update the example to use the correct single-argument pattern:
trigger({
id: "github-pr-review", // id goes inside config object
name: "GitHub PR Review",
messageTemplate: "New PR opened: {{title}}",
runAsUserId: "user_abc123",
});Refs:
- SDK trigger function signature — confirms single config object pattern
- Incorrect docs example
🟠⚠️ Major (2) 🟠⚠️
🟠 1) agents-docs/content/visual-builder/tools/user-vs-project-mcp.mdx:31 Documentation contradicts the feature being introduced
Issue: Line 31 states: "Not compatible with triggers: User-level MCPs cannot be used with webhook triggers or scheduled jobs because these invocation methods don't have user context." This directly contradicts the runAsUserId feature this PR introduces, which specifically enables user context for triggers.
Why: Users reading this documentation will believe user-scoped MCPs can't work with triggers, when that's now the primary use case this PR enables. This creates confusion and undermines feature adoption.
Fix: Update the documentation to reflect that runAsUserId now enables user-level MCPs with triggers:
- **Requires `runAsUserId` for triggers**: User-level MCPs can be used with [webhook triggers](/talk-to-your-agents/triggers/webhooks) and scheduled jobs when `runAsUserId` is configured, which provides the user context needed to access credentials.Refs:
🟠 2) agents-api/src/domains/manage/routes/triggerHelpers.ts:27-33 Cross-tenant user enumeration via differentiated error messages
Issue: validateRunAsUserId calls getUserById() which queries globally without tenant scoping, then returns different error messages for "user doesn't exist" vs "user lacks permission". This allows callers to enumerate valid user IDs across all tenants by observing error response differences.
Why: An attacker with access to one tenant could probe user IDs to discover which users exist in other tenants, potentially gathering reconnaissance for targeted attacks or identifying high-value accounts.
Fix: Consider one of these approaches:
- Unified error message: Return the same generic error for both cases: "User cannot be configured as runAsUserId for this trigger"
- Tenant-scoped lookup: Add tenant filtering to the user lookup so only users visible within the tenant can be queried
Refs:
- triggerHelpers.ts:27-33 — differentiated error messages
- OWASP User Enumeration Prevention
Inline Comments:
- 🟠 Major:
triggerHelpers.ts:27-33User enumeration via differentiated errors
🟡 Minor (2) 🟡
🟡 1) agents-docs/_snippets/user-scoped-mcp-triggers-warning.mdx:2 Broken documentation link
Issue: The warning snippet links to /deployment/inkeep-cloud/user-vs-project-mcp but the actual file is at /visual-builder/tools/user-vs-project-mcp.
Why: Users clicking the link will get a 404 error, preventing them from learning about the user vs project MCP distinction that's critical for understanding when runAsUserId is needed.
Fix: Update the link path:
See [User vs Project MCPs](/visual-builder/tools/user-vs-project-mcp) for more details.Refs:
🟡 2) packages/agents-core/src/data-access/index.ts New triggerCleanup module not exported from data-access barrel
Issue: The new triggerCleanup.ts module containing cleanupUserTriggers is not exported from the data-access index, breaking the established pattern where all data-access modules are re-exported.
Why: Consumers importing from @inkeep/agents-core won't have access to cleanupUserTriggers via the standard import path, forcing direct file imports that bypass the public API.
Fix: Add the export to packages/agents-core/src/data-access/index.ts:
export * from './manage/triggerCleanup';Refs:
- data-access/index.ts — existing exports pattern
- triggerCleanup.ts — new module
Inline Comments:
- 🟡 Minor:
index.tsMissing triggerCleanup export - 🟡 Minor:
user-scoped-mcp-triggers-warning.mdx:2Broken docs link
💭 Consider (3) 💭
💭 1) agents-api/src/utils/entityDiff.ts Add JSDoc explaining why createdBy is NOT ignored
Issue: The DEFAULT_IGNORED_FIELDS set explicitly excludes createdBy (per commit 9328ea64c), but there's no comment explaining this intentional design choice.
Why: Future maintainers might assume createdBy should be ignored like other server-stamped fields (createdAt, updatedAt) and accidentally add it back.
Fix: Add a brief comment above the set:
// Note: createdBy is intentionally NOT ignored - it's a meaningful field
// for change detection in user-scoped trigger reconciliation
const DEFAULT_IGNORED_FIELDS = new Set([...]);💭 2) agents-manage-ui/src/components/triggers/trigger-form.tsx Accessibility: Add aria-describedby for Run As field
Issue: The "Run As User" select field has helpful description text but it's not programmatically associated with the input via aria-describedby.
Why: Screen reader users won't hear the description text when focusing the field, missing important context about permission requirements.
💭 3) packages/agents-core/src/validation/schemas.ts Type inconsistency: runAsUserId allows empty string
Issue: UserIdSchema uses .min(1) which rejects empty strings, but runAsUserId is typed as string | null in some places. This could lead to runtime validation failures if empty strings are passed.
Why: Inconsistent handling of "no user" (null vs empty string vs undefined) can cause subtle bugs.
Inline Comments:
- 💭 Consider:
schemas.tsType consistency for empty strings - 💭 Consider:
trigger-form.tsxAccessibility improvement - 💭 Consider:
entityDiff.tsDocumentation for intentional design choice
💡 APPROVE WITH SUGGESTIONS
Summary: This PR implements a well-architected user-scoped webhook feature with solid authorization controls. The core implementation is sound, but there are documentation issues that could confuse developers—most critically, the SDK docs example uses the wrong function signature. The security concern about user enumeration is worth addressing but isn't a blocker. Fix the critical docs issue and the contradicting user-vs-project-mcp documentation before shipping to avoid developer confusion.
Discarded (37)
| Location | Issue | Reason Discarded |
|---|---|---|
triggerHelpers.ts |
Missing input validation on tenantRole | tenantRole comes from authenticated middleware, already validated |
TriggerService.ts |
Race condition in permission check | Permission check happens synchronously before async execution |
triggers.ts |
SQL injection risk | Using parameterized Drizzle ORM queries, not raw SQL |
projectFull.ts |
N+1 query pattern | Already parallelized with Promise.all per commit e36d5bb35 |
trigger-form.tsx |
XSS in user display | React auto-escapes, using UI library components |
manage-schema.ts |
Missing index on run_as_user_id | Query patterns don't filter by this column in hot paths |
entityDiff.ts |
Deep equality issues | Only used for shallow trigger config comparison |
webhooks.test.ts |
Flaky setTimeout | Already fixed per commit 1cb672851 using vi.waitFor |
triggerCleanup.ts |
Missing transaction rollback | Doltgres handles rollback on error automatically |
| Various | Import organization | Style preference, Biome handles this |
| Various | Console.log usage | Already addressed per resolved review comment |
triggers.ts |
Missing rate limiting | Out of scope for this PR, would be infrastructure change |
TriggerService.ts |
Missing audit logging | Valuable but not a bug, could be follow-up |
| Various | TypeScript strict mode warnings | Codebase doesn't use strict mode |
trigger-form.tsx |
Form validation timing | UX preference, current behavior is reasonable |
| Multiple files | Error message consistency | Minor style differences, not user-facing |
scheduledTriggers.ts |
Code duplication with triggers.ts | Intentional separation of concerns for different trigger types |
auth.ts |
beforeRemoveMember hook error handling | Errors are logged, non-blocking by design |
projectFull.ts |
Complex function length | Refactored per resolved review comment |
| Various | Missing null checks | TypeScript handles via strict null checks in relevant paths |
agents.ts |
Logger usage | Already addressed per resolved review comment |
validation/schemas.ts |
UserIdSchema inheritance | Already implemented per resolved review comment |
triggerCleanup.ts |
Location in data layer | Already moved per resolved review comment |
projectFull.ts |
validateTriggerPermissions duplication | Already extracted per resolved review comment |
| Various | Test coverage gaps | Tests exist for all new paths |
trigger-form.tsx |
Missing loading states | Loading states present via useMutation |
webhooks.mdx |
Missing error handling docs | Existing error handling section adequate |
project-triggers-table.tsx |
Table accessibility | Using accessible DataTable component |
| Multiple | Inconsistent error codes | Follows existing codebase patterns |
TriggerService.ts |
Missing retry logic | Intentional - webhooks should fail fast |
| Various | Magic strings | Constants defined where reused |
manage-schema.ts |
Column nullable default | Intentional for backward compatibility |
triggers.test.ts |
Test isolation | Each test has isolated mock state |
projectFull.ts |
Error aggregation | Errors thrown immediately, appropriate for reconciliation |
| Various | Optional chaining overuse | TypeScript narrowing handles correctly |
trigger-generator.ts |
CLI pull command changes | Additive only, backward compatible |
| Various | Performance concerns | Parallelization already implemented where beneficial |
Reviewers (13)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-docs |
8 | 3 | 0 | 0 | 2 | 0 | 3 |
pr-review-security-iam |
6 | 1 | 0 | 0 | 1 | 0 | 4 |
pr-review-standards |
5 | 1 | 1 | 0 | 1 | 0 | 2 |
pr-review-consistency |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-frontend |
3 | 0 | 1 | 0 | 1 | 0 | 1 |
pr-review-types |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-architecture |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-breaking-changes |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-errors |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-precision |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-product |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-devops |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 42 | 5 | 3 | 0 | 7 | 0 | 27 |
Note: High discard rate due to many findings being style preferences, already-addressed items from resolved review comments, or issues not attributable to this PR's changes.
| messageTemplate: "New PR opened: {{title}}", | ||
| runAsUserId: "user_abc123", | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🔴 CRITICAL: SDK docs code example uses incorrect function signature
Issue: The code example at lines 348-354 shows trigger("github-pr-review", { name: "GitHub PR Review", ... }) with a two-argument form (id as first arg, config as second), but the actual SDK only supports a single-argument form trigger({ id?: string, ...config }).
Why: Users copying this example will get a TypeScript error or runtime failure because the function signature does not accept a string as the first argument. The SDK function is export function trigger(config: Omit<TriggerApiInsert, 'id'> & { id?: string }): Trigger which only accepts a single config object.
Fix:
trigger({
id: "github-pr-review", // optional - id goes inside config
name: "GitHub PR Review",
messageTemplate: "New PR opened: {{title}}",
runAsUserId: "user_abc123",
});Refs:
| @@ -1,3 +1,3 @@ | |||
| <Warning> | |||
| **User-scoped MCP servers are not supported with triggers.** Webhook triggers do not have user context, so the framework cannot determine which user's credentials to use. Use only project-scoped MCP servers with trigger-invoked agents. See [User vs Project MCPs](/deployment/inkeep-cloud/user-vs-project-mcp) for more details. | |||
| **User-scoped MCP servers require a configured `runAsUserId`. Without `runAsUserId` set on the trigger, tools that require user-scoped credentials will fail. See [User vs Project MCPs](/deployment/inkeep-cloud/user-vs-project-mcp) for more details. | |||
There was a problem hiding this comment.
🟡 Minor: Broken link to User vs Project MCPs documentation
Issue: The link path /deployment/inkeep-cloud/user-vs-project-mcp doesn't match the actual file path /visual-builder/tools/user-vs-project-mcp.mdx. This may result in a 404 or require a redirect.
Why: Users clicking "See User vs Project MCPs for more details" may not find the referenced documentation.
Fix:
| **User-scoped MCP servers require a configured `runAsUserId`. Without `runAsUserId` set on the trigger, tools that require user-scoped credentials will fail. See [User vs Project MCPs](/deployment/inkeep-cloud/user-vs-project-mcp) for more details. | |
| **User-scoped MCP servers require a configured `runAsUserId`. Without `runAsUserId` set on the trigger, tools that require user-scoped credentials will fail. See [User vs Project MCPs](/visual-builder/tools/user-vs-project-mcp) for more details. |
Refs:
| code: 'bad_request', | ||
| message: `User ${runAsUserId} does not exist`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Missing tenant membership validation - cross-tenant user reference possible
Issue: The validateRunAsUserId function checks if a user exists via getUserById and verifies project access via canUseProjectStrict, but it does NOT verify that the target user is a member of the current tenant/organization. getUserById queries the global user table without tenant scoping.
Why: An admin in Tenant A could potentially set runAsUserId to a user ID that exists globally (in Tenant B). While canUseProjectStrict would likely reject the project access, the error message "does not have 'use' permission" vs "does not exist" leaks whether a user ID exists across tenants (user enumeration vulnerability).
Fix: Add tenant membership verification before checking project access:
import { getOrgMemberByUserId } from '@inkeep/agents-core';
// After checking user exists (line 33)
const membership = await getOrgMemberByUserId(runDbClient)(tenantId, runAsUserId);
if (!membership) {
throw createApiError({
code: 'bad_request',
message: 'Invalid runAsUserId: user not found or does not have permission on this project',
});
}Alternatively, use a single generic error message for all validation failures to prevent user enumeration:
throw createApiError({
code: 'bad_request',
message: 'Invalid runAsUserId: user not found or does not have permission on this project',
});Refs:
| @@ -1,18 +1,19 @@ | |||
| import { drizzle } from 'drizzle-orm/node-postgres'; | |||
There was a problem hiding this comment.
🟡 Minor: New module not exported from data-access index
Issue: This new triggerCleanup.ts module is not re-exported from packages/agents-core/src/data-access/index.ts. All other peer modules in the same directory are exported via export * from './manage/...'.
Why: The function cleanupUserTriggers won't be discoverable via the package's public API. Consumers would need to import directly from the file path instead of @inkeep/agents-core.
Fix: Add to packages/agents-core/src/data-access/index.ts:
export * from './manage/triggerCleanup';Refs:
| 'updatedAt', | ||
| ]); | ||
|
|
||
| export function isEntityChanged( |
There was a problem hiding this comment.
💭 Consider: Document asymmetric comparison behavior
Issue: The isEntityChanged function iterates over Object.keys(existing) and skips keys not in existing (line 17: if (!(key in incoming)) continue). This means new fields in incoming that don't exist in existing are not detected as changes.
Why: This asymmetric comparison behavior is subtle and could be surprising. For trigger reconciliation, this is likely intentional since new triggers are handled by a separate !existing branch. However, future users of this utility may not expect this behavior.
Fix: Add JSDoc clarifying the behavior:
/**
* Check if an entity has changed by comparing existing keys.
* NOTE: Only compares keys present in `existing`. New keys in `incoming`
* that don't exist in `existing` are NOT detected as changes.
* This is intentional for reconciliation where new entities are handled separately.
*/| export const ScheduledTriggerSelectSchema = createSelectSchema(scheduledTriggers).extend({ | ||
| payload: z.record(z.string(), z.unknown()).nullable().optional(), | ||
| createdBy: z.string().nullable().describe('User ID of the user who created this trigger'), | ||
| createdBy: UserIdSchema.nullable().describe('User ID of the user who created this trigger'), |
There was a problem hiding this comment.
💭 Consider: Type inconsistency between TriggerSelectSchema and ScheduledTriggerSelectSchema
Issue: TriggerSelectSchema (line 777) explicitly applies UserIdSchema.nullable().optional() to runAsUserId, but ScheduledTriggerSelectSchema (line 943-946) relies on drizzle-zod's auto-generation which produces z.string().nullable() without .optional().
Why: This creates inconsistent type shapes:
TriggerSelectSchema.runAsUserId→string | null | undefinedScheduledTriggerSelectSchema.runAsUserId→string | null
Code handling both trigger types generically may encounter type mismatches.
Fix: For consistency, explicitly extend ScheduledTriggerSelectSchema with the same pattern:
export const ScheduledTriggerSelectSchema = createSelectSchema(scheduledTriggers).extend({
payload: z.record(z.string(), z.unknown()).nullable().optional(),
runAsUserId: UserIdSchema.nullable().optional().describe('User ID to run the trigger as'),
createdBy: UserIdSchema.nullable().optional().describe('User ID who created this trigger'),
});Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to the 2 commits since the last automated review:
| Commit | Description |
|---|---|
572d2cc6c |
Update trigger-form.tsx (accessibility fix) |
c5353ad45 |
address feedback (tenant-scoped user validation) |
Files changed in delta: 8 files
✅ Prior Critical Finding Resolved
The Major security finding from the prior review has been properly addressed:
🟠
triggerHelpers.ts:27-33Cross-tenant user enumeration via differentiated error messages — FIXED
What was fixed:
- Tenant-scoped lookup (lines 40-46): Replaced global
getUserById()withgetOrganizationMemberByUserId()which requires the tenant/organization ID and only returns users who are members of that specific organization - Unified error message (lines 11-12, 44, 57): The constant
INVALID_RUN_AS_USER = 'Invalid runAsUserId: user not found or does not have permission on this project'is now used for both "user not found in org" and "user lacks permission" cases, preventing user enumeration - Comprehensive test coverage (new
users.test.ts): Tests verify tenant isolation including cross-org user lookup returning null
Security verification:
- The new
getOrganizationMemberByUserIdfunction usesINNER JOINon themembertable withWHERE member.organizationId = ?ensuring queries are scoped to the tenant - Attackers can no longer distinguish between "user doesn't exist", "user exists but not in org", and "user exists but lacks permission"
✅ Prior Documentation Finding Resolved
🔴
webhooks.mdx:348-354SDK docs show incorrect trigger() function signature — FIXED
The documentation example was corrected from the two-argument form to the correct single-argument config object pattern that matches the SDK's actual implementation.
✅ Prior Accessibility Finding Resolved
🟡
trigger-form.tsx:893Decorative spinner icon missing aria-hidden — FIXED
Added aria-hidden="true" to the Loader2 spinner component.
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
users.ts:33-35JSDoc comment says "by email" but function is "byUserId" (copy-paste error)
Feature Summary
This PR implements user-scoped webhook triggers and has been comprehensively reviewed through 11 prior automated reviews. All Critical and Major findings have been addressed:
| Prior Finding | Status |
|---|---|
| 🔴 Critical: Rerun endpoint bypasses user-scoped execution | ✅ FIXED |
| 🟠 Major: Missing try-catch for invocation status updates | ✅ FIXED |
🟠 Major: Missing canManageTrigger UI authorization guard |
✅ FIXED |
| 🟠 Major: Documentation contradicts feature | ✅ FIXED |
| 🟠 Major: Cross-tenant user enumeration | ✅ FIXED (this delta) |
| 🔴 Critical: SDK docs incorrect function signature | ✅ FIXED (this delta) |
| 🟡 Minor: Type assertions bypass type safety | ✅ FIXED |
| 🟡 Minor: Badge lacks accessible name | ✅ FIXED |
| 🟡 Minor: Decorative spinner missing aria-hidden | ✅ FIXED (this delta) |
| 💭 Consider: Terminology "Execution Identity" vs "Run As" | ✅ FIXED |
💭 Consider: Function name cleanupUserScheduledTriggers |
✅ FIXED |
Implementation highlights:
- New
run_as_user_idandcreated_bycolumns on triggers table with tenant-scoped validation validateRunAsUserIdandassertCanMutateTriggerauthorization helpers- Runtime
canUseProjectStrictcheck at webhook execution time - User cleanup on org member departure
- Comprehensive test coverage across CRUD, rerun, and webhook execution paths
- UI components for "Run As" user selection with multi-user bulk creation for admins
- Documentation updates for both SDK and UI webhook trigger guides
✅ APPROVE
Summary: The delta successfully addresses the cross-tenant user enumeration security concern with a well-designed tenant-scoped lookup function. The documentation fix resolves the SDK example issue, and the accessibility improvement adds proper aria attributes. The one remaining Minor finding (JSDoc comment copy-paste error) is trivial. This comprehensive user-scoped webhooks feature is ready to merge! 🎉
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 0 | 0 |
Note: Low finding count reflects the focused delta scope and the high quality of the security fix implementation.
| @@ -34,6 +34,32 @@ export const getUserByEmail = | |||
| * Get organization member by email | |||
| * Returns the user if they are a member of the specified organization | |||
There was a problem hiding this comment.
🟡 Minor: JSDoc comment describes wrong function behavior
Issue: The JSDoc comment says "Get organization member by email" but the function getOrganizationMemberByUserId retrieves by userId, not email. This appears to be a copy-paste error.
Why: The mismatch between documentation and implementation will confuse developers using this function and could lead to incorrect usage.
Fix:
| * Returns the user if they are a member of the specified organization | |
| /** | |
| * Get organization member by user ID | |
| * Returns the user if they are a member of the specified organization | |
| */ |
Refs:
- getOrganizationMemberByEmail (line 63) — the actual "by email" function
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to the 3 commits since the last automated review:
| Commit | Description |
|---|---|
92f1d9c2c |
Merge branch 'main' into feat/webhook-user-scope |
9dd56b2dc |
fix lint |
3b4c0c14b |
public package (#2493) — unrelated |
Files changed in delta: 2 files
✅ No New Issues
The delta since the last review consists entirely of:
- Lint fixes (
users.test.ts) — Changed non-null assertions (result!.id) to optional chaining (result?.id). This is a style/lint fix with no behavioral change. - Unrelated package change (
agents-email/package.json) — Publishing configuration for agents-email package. Not part of this PR's feature scope.
Neither change affects the user-scoped webhooks feature. The core implementation remains unchanged from the prior approved review.
Feature Summary
This PR implements user-scoped webhook triggers and has been comprehensively reviewed through 12 prior automated reviews. All findings have been addressed:
| Category | Status |
|---|---|
| 🔴 Critical findings | ✅ All resolved |
| 🟠 Major findings | ✅ All resolved |
| 🟡 Minor findings | ✅ All resolved |
Key implementation highlights:
- New
run_as_user_idandcreated_bycolumns on triggers table with tenant-scoped validation validateRunAsUserIdandassertCanMutateTriggerauthorization helpers- Runtime
canUseProjectStrictcheck at webhook execution time - User cleanup on org member departure
- Comprehensive test coverage across CRUD, rerun, and webhook execution paths
- UI components for "Run As" user selection with multi-user bulk creation for admins
- Documentation updates for both SDK and UI webhook trigger guides
✅ APPROVE
Summary: The delta contains only lint fixes and an unrelated package.json change. All prior Critical, Major, and Minor findings from the comprehensive user-scoped webhooks implementation have been resolved. The feature is well-designed with proper authorization guards, runtime permission checks, cleanup on user departure, and comprehensive test coverage.
🎉 Ready to merge!
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| — | — | — | — | — | — | — | — |
Note: No domain-specific reviewers were dispatched for this delta review as the changes are trivial lint fixes unrelated to the feature scope.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
2 participants
No description provided.